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

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

[Jim Bride]

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(-)

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;
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/i915/dp/mst: Fix MST logic in intel_dp_long_pulse()

[Patchwork]

== Series Details ==

Series: drm/i915/dp/mst: Fix MST logic in intel_dp_long_pulse()
URL   : https://patchwork.freedesktop.org/series/5390/
State : warning

== Summary ==

Series 5390v1 drm/i915/dp/mst: Fix MST logic in intel_dp_long_pulse()
http://patchwork.freedesktop.org/api/1.0/series/5390/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s3:
                incomplete -> PASS       (hsw-gt2)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (ilk-hp8440p) UNSTABLE
Test pm_rpm:
        Subgroup basic-rte:
                pass       -> DMESG-WARN (bsw-nuc-2)

bdw-nuci7        total:196  pass:184  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:196  pass:175  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:196  pass:158  dwarn:1   dfail:0   fail:0   skip:37 
byt-nuc          total:196  pass:161  dwarn:0   dfail:0   fail:0   skip:35 
hsw-brixbox      total:196  pass:174  dwarn:0   dfail:0   fail:0   skip:22 
hsw-gt2          total:196  pass:179  dwarn:0   dfail:0   fail:0   skip:17 
ilk-hp8440p      total:196  pass:132  dwarn:0   dfail:0   fail:0   skip:64 
ivb-t430s        total:196  pass:171  dwarn:0   dfail:0   fail:0   skip:25 
skl-i7k-2        total:196  pass:173  dwarn:0   dfail:0   fail:0   skip:23 
skl-nuci5        total:196  pass:185  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:196  pass:162  dwarn:0   dfail:0   fail:0   skip:34 
snb-x220t        total:196  pass:162  dwarn:0   dfail:0   fail:1   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1821/

c44bc2c9b26070f007fcff708589bf4631165644 drm-intel-nightly: 2016y-04m-06d-18h-03m-27s UTC integration manifest
de6fbae1a450cf961001074de688ef3a3bc3a90e drm/i915/dp/mst: Fix MST logic in intel_dp_long_pulse()

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ander Conselvan De Oliveira]

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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jim Bride]

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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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?

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);

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jim Bride]

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.

v2: Add removed check further up in the function to be sure that MST
    is shut down when we lose the link. (Ander)

Fixes: commit 7d23e3c3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
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 | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index da0c3d2..31b222a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4608,6 +4608,15 @@ 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;
 	}
 
@@ -4665,20 +4674,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;
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/dp/mst: Fix MST logic in intel_dp_long_pulse()

[Lyude Paul]

NAK. Try plugging in an MST display, suspending the machine, then resuming it.
Hotplugging still breaks (which I've traced down to this patch)

I wouldn't worry about fixing this up. I'm probably going to be sending a revert
for this anyway soon along with probably some of the other patches.

On Mon, 2016-04-11 at 10:11 -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.
> 
> v2: Add removed check further up in the function to be sure that MST
>     is shut down when we lose the link. (Ander)
> 
> Fixes: commit 7d23e3c3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> 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 | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index da0c3d2..31b222a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4608,6 +4608,15 @@ 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;
>  	}
>  
> @@ -4665,20 +4674,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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/dp/mst: Fix MST logic in intel_dp_long_pulse()

[Ander Conselvan De Oliveira]

On Mon, 2016-04-11 at 10:11 -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.
> 
> v2: Add removed check further up in the function to be sure that MST
>     is shut down when we lose the link. (Ander)
> 
> Fixes: commit 7d23e3c3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> 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>

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index da0c3d2..31b222a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4608,6 +4608,15 @@ 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;
>  	}
>  
> @@ -4665,20 +4674,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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915/dp/mst: Fix MST logic in intel_dp_long_pulse() (rev3)

[Patchwork]

== 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

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()

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/dp/mst: Fix MST logic in intel_dp_long_pulse()

[Lyude]

I'm going to take back the NAK on this, apparently this hotplugging
issue has been around longer then this patchset.

Reviewed-by: Lyude <cpaul@redhat.com>


On Tue, 2016-04-12 at 10:11 +0300, Ander Conselvan De Oliveira wrote:
> On Mon, 2016-04-11 at 10:11 -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.
> > 
> > v2: Add removed check further up in the function to be sure that
> > MST
> >     is shut down when we lose the link. (Ander)
> > 
> > Fixes: commit 7d23e3c3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> > 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>
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> 
> > 
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 24 +++++++++++-------------
> >  1 file changed, 11 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index da0c3d2..31b222a 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4608,6 +4608,15 @@ 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;
> >  	}
> >  
> > @@ -4665,20 +4674,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;
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- 
Cheers,
	Lyude

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: failure for drm/i915/dp/mst: Fix MST logic in intel_dp_long_pulse() (rev3)

[Jim Bride]

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()
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: failure for drm/i915/dp/mst: Fix MST logic in intel_dp_long_pulse() (rev3)

[Jim Bride]

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()
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx