diff mbox

drm/i915/dp/mst: Fix MST logic in intel_dp_long_pulse()

Message ID 1459983616-28975-1-git-send-email-jim.bride@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

jim.bride@linux.intel.com April 6, 2016, 11 p.m. UTC
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@intel.com>
cc: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
cc: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

Comments

Ander Conselvan de Oliveira April 7, 2016, 8:15 a.m. UTC | #1
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@intel.com>
> cc: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> cc: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>

Fixes: commit 7d23e3c3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")

> Signed-off-by: Jim Bride <jim.bride@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.


Ander

>  
>  	intel_display_power_put(to_i915(dev), power_domain);
>  	return;
jim.bride@linux.intel.com April 7, 2016, 4:20 p.m. UTC | #2
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@intel.com>
> > cc: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> > cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> > cc: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> 
> Fixes: commit 7d23e3c3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> 
> > Signed-off-by: Jim Bride <jim.bride@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?

Jim


> 
> 
> Ander
> 
> >  
> >  	intel_display_power_put(to_i915(dev), power_domain);
> >  	return;
jim.bride@linux.intel.com April 12, 2016, 8:26 p.m. UTC | #3
On Tue, Apr 12, 2016 at 07:39:35AM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915/dp/mst: Fix MST logic in intel_dp_long_pulse() (rev3)
> URL   : https://patchwork.freedesktop.org/series/5390/
> State : failure
> 
> == Summary ==
> 
> Series 5390v3 drm/i915/dp/mst: Fix MST logic in intel_dp_long_pulse()
> http://patchwork.freedesktop.org/api/1.0/series/5390/revisions/3/mbox/
> 
> Test gem_basic:
>         Subgroup create-fd-close:
>                 incomplete -> PASS       (bsw-nuc-2)
> Test gem_ctx_param_basic:
>         Subgroup invalid-param-get:
>                 incomplete -> PASS       (bsw-nuc-2)
> Test gem_exec_basic:
>         Subgroup basic-bsd1:
>                 incomplete -> SKIP       (bsw-nuc-2)
> Test gem_exec_store:
>         Subgroup basic-bsd1:
>                 incomplete -> SKIP       (bsw-nuc-2)
>         Subgroup basic-bsd2:
>                 incomplete -> SKIP       (bsw-nuc-2)
> Test gem_ringfill:
>         Subgroup basic-default-hang:
>                 dmesg-warn -> PASS       (bsw-nuc-2)
> Test gem_storedw_loop:
>         Subgroup basic-bsd2:
>                 incomplete -> SKIP       (bsw-nuc-2)
> Test kms_addfb_basic:
>         Subgroup bad-pitch-1024:
>                 dmesg-warn -> PASS       (bsw-nuc-2)
>         Subgroup basic-y-tiled:
>                 incomplete -> PASS       (bsw-nuc-2)
>         Subgroup small-bo:
>                 incomplete -> PASS       (bsw-nuc-2)
>         Subgroup unused-handle:
>                 incomplete -> PASS       (bsw-nuc-2)
> Test kms_force_connector_basic:
>         Subgroup prune-stale-modes:
>                 pass       -> SKIP       (snb-x220t)
> Test kms_pipe_crc_basic:
>         Subgroup read-crc-pipe-a:
>                 incomplete -> SKIP       (bsw-nuc-2)
>         Subgroup read-crc-pipe-b:
>                 incomplete -> SKIP       (bsw-nuc-2)
> Test prime_self_import:
>         Subgroup basic-with_one_bo:
>                 incomplete -> PASS       (bsw-nuc-2)
>         Subgroup basic-with_two_bos:
>                 incomplete -> PASS       (bsw-nuc-2)
> 
> bdw-nuci7        total:202  pass:190  dwarn:0   dfail:0   fail:0   skip:12 
> bdw-ultra        total:202  pass:179  dwarn:0   dfail:0   fail:0   skip:23 
> bsw-nuc-2        total:201  pass:162  dwarn:0   dfail:0   fail:0   skip:39 
> byt-nuc          total:201  pass:163  dwarn:0   dfail:0   fail:0   skip:38 
> hsw-brixbox      total:202  pass:178  dwarn:0   dfail:0   fail:0   skip:24 
> ilk-hp8440p      total:202  pass:134  dwarn:0   dfail:0   fail:0   skip:68 
> ivb-t430s        total:202  pass:174  dwarn:0   dfail:0   fail:0   skip:28 
> skl-i7k-2        total:202  pass:177  dwarn:0   dfail:0   fail:0   skip:25 
> snb-x220t        total:202  pass:163  dwarn:0   dfail:0   fail:1   skip:38 
> BOOT FAILED for snb-dellxps

Although I'm not sure whether it's the one snb-x220t case listed above or
the boot failure for the system snb-dellxps that caused this to be flagged
as failing by CI, I don't see how either of these things could be caused
by this patch set.

Jim

> 
> Results at /archive/results/CI_IGT_test/Patchwork_1864/
> 
> dc5380b5263ebb0bf251bb09db542585702b528b drm-intel-nightly: 2016y-04m-11d-19h-43m-10s UTC integration manifest
> e9b4dd341189637119d15df79a72a9a1f53c62fa drm/i915/dp/mst: Fix MST logic in intel_dp_long_pulse()
jim.bride@linux.intel.com April 13, 2016, 6:06 p.m. UTC | #4
On Tue, Apr 12, 2016 at 07:39:35AM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915/dp/mst: Fix MST logic in intel_dp_long_pulse() (rev3)
> URL   : https://patchwork.freedesktop.org/series/5390/
> State : failure
> 
> == Summary ==
> 
> Series 5390v3 drm/i915/dp/mst: Fix MST logic in intel_dp_long_pulse()
> http://patchwork.freedesktop.org/api/1.0/series/5390/revisions/3/mbox/
> 
> Test gem_basic:
>         Subgroup create-fd-close:
>                 incomplete -> PASS       (bsw-nuc-2)
> Test gem_ctx_param_basic:
>         Subgroup invalid-param-get:
>                 incomplete -> PASS       (bsw-nuc-2)
> Test gem_exec_basic:
>         Subgroup basic-bsd1:
>                 incomplete -> SKIP       (bsw-nuc-2)
> Test gem_exec_store:
>         Subgroup basic-bsd1:
>                 incomplete -> SKIP       (bsw-nuc-2)
>         Subgroup basic-bsd2:
>                 incomplete -> SKIP       (bsw-nuc-2)
> Test gem_ringfill:
>         Subgroup basic-default-hang:
>                 dmesg-warn -> PASS       (bsw-nuc-2)
> Test gem_storedw_loop:
>         Subgroup basic-bsd2:
>                 incomplete -> SKIP       (bsw-nuc-2)
> Test kms_addfb_basic:
>         Subgroup bad-pitch-1024:
>                 dmesg-warn -> PASS       (bsw-nuc-2)
>         Subgroup basic-y-tiled:
>                 incomplete -> PASS       (bsw-nuc-2)
>         Subgroup small-bo:
>                 incomplete -> PASS       (bsw-nuc-2)
>         Subgroup unused-handle:
>                 incomplete -> PASS       (bsw-nuc-2)
> Test kms_force_connector_basic:
>         Subgroup prune-stale-modes:
>                 pass       -> SKIP       (snb-x220t)
> Test kms_pipe_crc_basic:
>         Subgroup read-crc-pipe-a:
>                 incomplete -> SKIP       (bsw-nuc-2)
>         Subgroup read-crc-pipe-b:
>                 incomplete -> SKIP       (bsw-nuc-2)
> Test prime_self_import:
>         Subgroup basic-with_one_bo:
>                 incomplete -> PASS       (bsw-nuc-2)
>         Subgroup basic-with_two_bos:
>                 incomplete -> PASS       (bsw-nuc-2)
> 
> bdw-nuci7        total:202  pass:190  dwarn:0   dfail:0   fail:0   skip:12 
> bdw-ultra        total:202  pass:179  dwarn:0   dfail:0   fail:0   skip:23 
> bsw-nuc-2        total:201  pass:162  dwarn:0   dfail:0   fail:0   skip:39 
> byt-nuc          total:201  pass:163  dwarn:0   dfail:0   fail:0   skip:38 
> hsw-brixbox      total:202  pass:178  dwarn:0   dfail:0   fail:0   skip:24 
> ilk-hp8440p      total:202  pass:134  dwarn:0   dfail:0   fail:0   skip:68 
> ivb-t430s        total:202  pass:174  dwarn:0   dfail:0   fail:0   skip:28 
> skl-i7k-2        total:202  pass:177  dwarn:0   dfail:0   fail:0   skip:25 
> snb-x220t        total:202  pass:163  dwarn:0   dfail:0   fail:1   skip:38 

Ok, it looks like this is https://bugs.freedesktop.org/show_bug.cgi?id=93123
which pre-dates this patch.

Jim

> BOOT FAILED for snb-dellxps
> Results at /archive/results/CI_IGT_test/Patchwork_1864/
> 
> dc5380b5263ebb0bf251bb09db542585702b528b drm-intel-nightly: 2016y-04m-11d-19h-43m-10s UTC integration manifest
> e9b4dd341189637119d15df79a72a9a1f53c62fa drm/i915/dp/mst: Fix MST logic in intel_dp_long_pulse()
diff mbox

Patch

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);
-		}
-	}
 
 	intel_display_power_put(to_i915(dev), power_domain);
 	return;