[Intel-gfx] [PATCH] drm/i915/dp/mst: Fix MST logic in intel_dp_long_pulse()

[Ander Conselvan De Oliveira]

On Thu, 2016-04-07 at 09:20 -0700, Jim Bride wrote:
> On Thu, Apr 07, 2016 at 11:15:38AM +0300, Ander Conselvan De Oliveira wrote:
> > On Wed, 2016-04-06 at 16:00 -0700, Jim Bride wrote:
> > > In commit 7d23e3c3 ("drm/i915: Cleaning up intel_dp_hpd_pulse") some
> > > much needed clean-up was done, but unfortunately part of the change
> > > broke DP MST.  The real issue was setting the connector state to
> > > disconnected in the MST case, which is good, but the code then (after
> > > a goto) checks if the connector state is not connected and shuts down
> > > MST if this is the case, which is bad.  With this change both SST and
> > > MST seem to be happy.
> > > 
> > > cc: Sivakumar Thulasimani <sivakumar.thulasimani at intel.com>
> > > cc: Shubhangi Shrivastava <shubhangi.shrivastava at intel.com>
> > > cc: Ander Conselvan de Oliveira <conselvan2 at gmail.com>
> > > cc: Nathan D Ciobanu <nathan.d.ciobanu at intel.com>
> > 
> > Fixes: commit 7d23e3c3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> > 
> > > Signed-off-by: Jim Bride <jim.bride at linux.intel.com>
> > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 15 ++-------------
> > >  1 file changed, 2 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index da0c3d2..2d8783e 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4665,20 +4665,9 @@ intel_dp_long_pulse(struct intel_connector
> > > *intel_connector)
> > >  	}
> > >  
> > >  out:
> > > -	if (status != connector_status_connected) {
> > > +	if ((status != connector_status_connected) &&
> > > +	    (intel_dp->is_mst == false))
> > >  		intel_dp_unset_edid(intel_dp);
> > > -		/*
> > > -		 * If we were in MST mode, and device is not there,
> > > -		 * get out of MST mode
> > > -		 */
> > > -		if (intel_dp->is_mst) {
> > > -			DRM_DEBUG_KMS("MST device may have disappeared %d
> > > vs
> > > %d\n",
> > > -				      intel_dp->is_mst, intel_dp
> > > ->mst_mgr.mst_state);
> > > -			intel_dp->is_mst = false;
> > > -			drm_dp_mst_topology_mgr_set_mst(&intel_dp
> > > ->mst_mgr,
> > > -							intel_dp
> > > ->is_mst);
> > > -		}
> > > -	}
> > 
> > The point of that code was to get out of MST mode in case of a
> > disconnection,
> > but it does the wrong thing there. With the deletion, the device would be is
> > MST
> > until the next time something is plugged to the port. I haven't checked what
> > are
> > the consequences of that though.
> > 
> > So maybe move that code before the first "goto out". That should cover the
> > was
> > mst and is now disconnected case.
> 
> I don't think that would work either.  For what you're talking about it seems
> to
> me that the most reliable way to catch this case would be to check that we can
> read the branch OUI, and if we cannot then we should shut down MST.
> 
> Thoughts?

There's intel_dp_probe_mst() to determine if MST should be used when there is
something plugged in. I don't know if that should consider branch OUI. But for
the case where the branch device was disconnected, any DPCD read will fail, so
there is no point in reading the OUI. The current code just skips it and reports
it as disconnected.

I've tried the patch below locally, and it seems to do the right thing.

[drm:intel_hpd_irq_storm_detect] Received HPD interrupt on PIN 5 - cnt: 0
[drm:intel_dp_hpd_pulse] got hpd irq on port B - long
[drm:intel_dp_long_pulse] MST device may have disappeared 1 vs 1


diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index da0c3d2..8e725d7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4608,6 +4608,14 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
                intel_dp->compliance_test_type = 0;
                intel_dp->compliance_test_data = 0;
 
+               if (intel_dp->is_mst) {
+                       DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
+                                     intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
+                       intel_dp->is_mst = false;
+                       drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+                                                       intel_dp->is_mst);
+               }
+
                goto out;
        }
 
@@ -4667,17 +4675,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 out:
        if (status != connector_status_connected) {
                intel_dp_unset_edid(intel_dp);
-               /*
-                * If we were in MST mode, and device is not there,
-                * get out of MST mode
-                */
-               if (intel_dp->is_mst) {
-                       DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
-                                     intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
-                       intel_dp->is_mst = false;
-                       drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
-                                                       intel_dp->is_mst);
-               }
        }
 
        intel_display_power_put(to_i915(dev), power_domain);