[PATCH v4] drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"

[PATCH v4] drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"

[Lyude Paul]

From: Jan-Marek Glogowski <glogow@fbihome.de>

This re-applies the workaround for "some DP sinks, [which] are a
little nuts" from commit 1a36147bb939 ("drm/i915: Perform link
quality check unconditionally during long pulse").
It makes the secondary AOC E2460P monitor connected via DP to an
acer Veriton N4640G usable again.

This hunk was dropped in commit c85d200e8321 ("drm/i915: Move SST
DP link retraining into the ->post_hotplug() hook")

Fixes: c85d200e8321 ("drm/i915: Move SST DP link retraining into the ->post_hotplug() hook")
[Cleaned up commit message, added stable cc]
Signed-off-by: Lyude Paul <lyude@redhat.com>
Signed-off-by: Jan-Marek Glogowski <glogow@fbihome.de>
Cc: stable@vger.kernel.org
---
Resending this to update patchwork; will push in a little bit

 drivers/gpu/drm/i915/intel_dp.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b3f6f04c3c7d..db8515171270 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4333,18 +4333,6 @@ intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
 	return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
 }
 
-/*
- * If display is now connected check links status,
- * there has been known issues of link loss triggering
- * long pulse.
- *
- * Some sinks (eg. ASUS PB287Q) seem to perform some
- * weird HPD ping pong during modesets. So we can apparently
- * end up with HPD going low during a modeset, and then
- * going back up soon after. And once that happens we must
- * retrain the link to get a picture. That's in case no
- * userspace component reacted to intermittent HPD dip.
- */
 int intel_dp_retrain_link(struct intel_encoder *encoder,
 			  struct drm_modeset_acquire_ctx *ctx)
 {
@@ -5031,7 +5019,8 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
 }
 
 static int
-intel_dp_long_pulse(struct intel_connector *connector)
+intel_dp_long_pulse(struct intel_connector *connector,
+		    struct drm_modeset_acquire_ctx *ctx)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	struct intel_dp *intel_dp = intel_attached_dp(&connector->base);
@@ -5090,6 +5079,22 @@ intel_dp_long_pulse(struct intel_connector *connector)
 		 */
 		status = connector_status_disconnected;
 		goto out;
+	} else {
+		/*
+		 * If display is now connected check links status,
+		 * there has been known issues of link loss triggering
+		 * long pulse.
+		 *
+		 * Some sinks (eg. ASUS PB287Q) seem to perform some
+		 * weird HPD ping pong during modesets. So we can apparently
+		 * end up with HPD going low during a modeset, and then
+		 * going back up soon after. And once that happens we must
+		 * retrain the link to get a picture. That's in case no
+		 * userspace component reacted to intermittent HPD dip.
+		 */
+		struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
+
+		intel_dp_retrain_link(encoder, ctx);
 	}
 
 	/*
@@ -5151,7 +5156,7 @@ intel_dp_detect(struct drm_connector *connector,
 				return ret;
 		}
 
-		status = intel_dp_long_pulse(intel_dp->attached_connector);
+		status = intel_dp_long_pulse(intel_dp->attached_connector, ctx);
 	}
 
 	intel_dp->detect_done = false;
-- 
2.17.1

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

✓ Fi.CI.BAT: success for drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse" (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse" (rev4)
URL   : https://patchwork.freedesktop.org/series/47694/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4704 -> Patchwork_10016 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/47694/revisions/4/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10016:

  === IGT changes ===

    ==== Possible regressions ====

    igt@drv_selftest@live_evict:
      {fi-bsw-kefka}:     PASS -> DMESG-WARN

    
== Known issues ==

  Here are the changes found in Patchwork_10016 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_hangcheck:
      {fi-kbl-8809g}:     PASS -> DMESG-FAIL (fdo#106947, fdo#106560)

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       PASS -> DMESG-WARN (fdo#107139, fdo#105128)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         PASS -> INCOMPLETE (fdo#103927)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      fi-skl-guc:         DMESG-FAIL (fdo#107174) -> PASS

    igt@kms_pipe_crc_basic@hang-read-crc-pipe-b:
      fi-skl-guc:         FAIL (fdo#103191) -> PASS +1
      {fi-byt-clapper}:   FAIL (fdo#103191, fdo#107362) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-skl-6260u:       INCOMPLETE (fdo#104108) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139
  fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362


== Participating hosts (54 -> 49) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4704 -> Patchwork_10016

  CI_DRM_4704: eb9a20b20d4790be1561235f45e209fba02dc6c0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4609: 0bc9763af77bbb37f2ed65cc39c398e88db7d8e3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10016: ebfb02d78b5f8a9db39e0e2b8d0992b9956f4dd3 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

ebfb02d78b5f drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10016/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse" (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse" (rev4)
URL   : https://patchwork.freedesktop.org/series/47694/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4704_full -> Patchwork_10016_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_10016_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ctx_isolation@bcs0-s3:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@kms_draw_crc@draw-method-rgb565-mmap-wc-xtiled:
      shard-glk:          PASS -> FAIL (fdo#103184)

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-glk:          PASS -> FAIL (fdo#105363)

    igt@kms_setmode@basic:
      shard-kbl:          PASS -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@kms_cursor_legacy@cursor-vs-flip-toggle:
      shard-hsw:          FAIL (fdo#103355) -> PASS

    igt@kms_vblank@pipe-a-ts-continuation-suspend:
      shard-hsw:          FAIL (fdo#104894) -> PASS

    
  fdo#103184 https://bugs.freedesktop.org/show_bug.cgi?id=103184
  fdo#103355 https://bugs.freedesktop.org/show_bug.cgi?id=103355
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#104894 https://bugs.freedesktop.org/show_bug.cgi?id=104894
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4704 -> Patchwork_10016

  CI_DRM_4704: eb9a20b20d4790be1561235f45e209fba02dc6c0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4609: 0bc9763af77bbb37f2ed65cc39c398e88db7d8e3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10016: ebfb02d78b5f8a9db39e0e2b8d0992b9956f4dd3 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10016/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4] drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"

[Jani Nikula]

On Sat, 25 Aug 2018, Lyude Paul <lyude@redhat.com> wrote:
> From: Jan-Marek Glogowski <glogow@fbihome.de>
>
> This re-applies the workaround for "some DP sinks, [which] are a
> little nuts" from commit 1a36147bb939 ("drm/i915: Perform link
> quality check unconditionally during long pulse").
> It makes the secondary AOC E2460P monitor connected via DP to an
> acer Veriton N4640G usable again.
>
> This hunk was dropped in commit c85d200e8321 ("drm/i915: Move SST
> DP link retraining into the ->post_hotplug() hook")
>
> Fixes: c85d200e8321 ("drm/i915: Move SST DP link retraining into the ->post_hotplug() hook")
> [Cleaned up commit message, added stable cc]
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Jan-Marek Glogowski <glogow@fbihome.de>
> Cc: stable@vger.kernel.org
> ---
> Resending this to update patchwork; will push in a little bit

Is there a bugzilla? Reference to a list discussion? Something with a
dmesg where someone can actually verify this is the right fix?

IMO needs an ack from Ville too. He should be in Cc: in the first place
as the author of the regressing commit.

'dim fixes c85d200e8321' gives you the output:

Fixes: c85d200e8321 ("drm/i915: Move SST DP link retraining into the ->post_hotplug() hook")
Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v4.17+

BR,
Jani.

>
>  drivers/gpu/drm/i915/intel_dp.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b3f6f04c3c7d..db8515171270 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4333,18 +4333,6 @@ intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
>  	return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
>  }
>  
> -/*
> - * If display is now connected check links status,
> - * there has been known issues of link loss triggering
> - * long pulse.
> - *
> - * Some sinks (eg. ASUS PB287Q) seem to perform some
> - * weird HPD ping pong during modesets. So we can apparently
> - * end up with HPD going low during a modeset, and then
> - * going back up soon after. And once that happens we must
> - * retrain the link to get a picture. That's in case no
> - * userspace component reacted to intermittent HPD dip.
> - */
>  int intel_dp_retrain_link(struct intel_encoder *encoder,
>  			  struct drm_modeset_acquire_ctx *ctx)
>  {
> @@ -5031,7 +5019,8 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>  }
>  
>  static int
> -intel_dp_long_pulse(struct intel_connector *connector)
> +intel_dp_long_pulse(struct intel_connector *connector,
> +		    struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	struct intel_dp *intel_dp = intel_attached_dp(&connector->base);
> @@ -5090,6 +5079,22 @@ intel_dp_long_pulse(struct intel_connector *connector)
>  		 */
>  		status = connector_status_disconnected;
>  		goto out;
> +	} else {
> +		/*
> +		 * If display is now connected check links status,
> +		 * there has been known issues of link loss triggering
> +		 * long pulse.
> +		 *
> +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> +		 * weird HPD ping pong during modesets. So we can apparently
> +		 * end up with HPD going low during a modeset, and then
> +		 * going back up soon after. And once that happens we must
> +		 * retrain the link to get a picture. That's in case no
> +		 * userspace component reacted to intermittent HPD dip.
> +		 */
> +		struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> +
> +		intel_dp_retrain_link(encoder, ctx);
>  	}
>  
>  	/*
> @@ -5151,7 +5156,7 @@ intel_dp_detect(struct drm_connector *connector,
>  				return ret;
>  		}
>  
> -		status = intel_dp_long_pulse(intel_dp->attached_connector);
> +		status = intel_dp_long_pulse(intel_dp->attached_connector, ctx);
>  	}
>  
>  	intel_dp->detect_done = false;

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH v4] drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"

[Ville Syrjälä]

On Sat, Aug 25, 2018 at 03:10:35PM -0400, Lyude Paul wrote:
> From: Jan-Marek Glogowski <glogow@fbihome.de>
> 
> This re-applies the workaround for "some DP sinks, [which] are a
> little nuts" from commit 1a36147bb939 ("drm/i915: Perform link
> quality check unconditionally during long pulse").
> It makes the secondary AOC E2460P monitor connected via DP to an
> acer Veriton N4640G usable again.
> 
> This hunk was dropped in commit c85d200e8321 ("drm/i915: Move SST
> DP link retraining into the ->post_hotplug() hook")
> 
> Fixes: c85d200e8321 ("drm/i915: Move SST DP link retraining into the ->post_hotplug() hook")
> [Cleaned up commit message, added stable cc]
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Jan-Marek Glogowski <glogow@fbihome.de>
> Cc: stable@vger.kernel.org
> ---
> Resending this to update patchwork; will push in a little bit
> 
>  drivers/gpu/drm/i915/intel_dp.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b3f6f04c3c7d..db8515171270 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4333,18 +4333,6 @@ intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
>  	return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
>  }
>  
> -/*
> - * If display is now connected check links status,
> - * there has been known issues of link loss triggering
> - * long pulse.
> - *
> - * Some sinks (eg. ASUS PB287Q) seem to perform some
> - * weird HPD ping pong during modesets. So we can apparently
> - * end up with HPD going low during a modeset, and then
> - * going back up soon after. And once that happens we must
> - * retrain the link to get a picture. That's in case no
> - * userspace component reacted to intermittent HPD dip.
> - */
>  int intel_dp_retrain_link(struct intel_encoder *encoder,
>  			  struct drm_modeset_acquire_ctx *ctx)
>  {
> @@ -5031,7 +5019,8 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>  }
>  
>  static int
> -intel_dp_long_pulse(struct intel_connector *connector)
> +intel_dp_long_pulse(struct intel_connector *connector,
> +		    struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	struct intel_dp *intel_dp = intel_attached_dp(&connector->base);
> @@ -5090,6 +5079,22 @@ intel_dp_long_pulse(struct intel_connector *connector)
>  		 */
>  		status = connector_status_disconnected;
>  		goto out;
> +	} else {
> +		/*
> +		 * If display is now connected check links status,
> +		 * there has been known issues of link loss triggering
> +		 * long pulse.
> +		 *
> +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> +		 * weird HPD ping pong during modesets. So we can apparently
> +		 * end up with HPD going low during a modeset, and then
> +		 * going back up soon after. And once that happens we must
> +		 * retrain the link to get a picture. That's in case no
> +		 * userspace component reacted to intermittent HPD dip.
> +		 */
> +		struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> +
> +		intel_dp_retrain_link(encoder, ctx);

We should really have a comment here that this is purely duct tape for
sinks that fail to signal a hpd when the link goes bad (either that or
we fail to process the hpd correctly).

I suppose a better way to do this hack would be to do the link quality
check at the end of modeset, or from a delayed work. As is this depends
on userspace/fbdev doing an explicit probe after the modeset which seems
pretty fragile.

>  	}
>  
>  	/*
> @@ -5151,7 +5156,7 @@ intel_dp_detect(struct drm_connector *connector,
>  				return ret;
>  		}
>  
> -		status = intel_dp_long_pulse(intel_dp->attached_connector);
> +		status = intel_dp_long_pulse(intel_dp->attached_connector, ctx);
>  	}
>  
>  	intel_dp->detect_done = false;
> -- 
> 2.17.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel

Re: [PATCH v4] drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"

[Jan-Marek Glogowski]

Am 27.08.2018 um 10:43 schrieb Jani Nikula:
> On Sat, 25 Aug 2018, Lyude Paul <lyude@redhat.com> wrote:
>> From: Jan-Marek Glogowski <glogow@fbihome.de>
>>
>> This re-applies the workaround for "some DP sinks, [which] are a
>> little nuts" from commit 1a36147bb939 ("drm/i915: Perform link
>> quality check unconditionally during long pulse").
>> It makes the secondary AOC E2460P monitor connected via DP to an
>> acer Veriton N4640G usable again.
>>
>> This hunk was dropped in commit c85d200e8321 ("drm/i915: Move SST
>> DP link retraining into the ->post_hotplug() hook")
>>
>> Fixes: c85d200e8321 ("drm/i915: Move SST DP link retraining into the ->post_hotplug() hook")
>> [Cleaned up commit message, added stable cc]
>> Signed-off-by: Lyude Paul <lyude@redhat.com>
>> Signed-off-by: Jan-Marek Glogowski <glogow@fbihome.de>
>> Cc: stable@vger.kernel.org
>> ---
>> Resending this to update patchwork; will push in a little bit
> 
> Is there a bugzilla? Reference to a list discussion? Something with a
> dmesg where someone can actually verify this is the right fix?

The bug is at https://bugs.freedesktop.org/show_bug.cgi?id=107446
The original thread starts here:
https://lists.freedesktop.org/archives/intel-gfx/2018-August/172940.html

Jan-Marek

Re: [PATCH v4] drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"

[Lyude Paul]

On Mon, 2018-08-27 at 11:43 +0300, Jani Nikula wrote:
> On Sat, 25 Aug 2018, Lyude Paul <lyude@redhat.com> wrote:
> > From: Jan-Marek Glogowski <glogow@fbihome.de>
> > 
> > This re-applies the workaround for "some DP sinks, [which] are a
> > little nuts" from commit 1a36147bb939 ("drm/i915: Perform link
> > quality check unconditionally during long pulse").
> > It makes the secondary AOC E2460P monitor connected via DP to an
> > acer Veriton N4640G usable again.
> > 
> > This hunk was dropped in commit c85d200e8321 ("drm/i915: Move SST
> > DP link retraining into the ->post_hotplug() hook")
> > 
> > Fixes: c85d200e8321 ("drm/i915: Move SST DP link retraining into the
> > ->post_hotplug() hook")
> > [Cleaned up commit message, added stable cc]
> > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > Signed-off-by: Jan-Marek Glogowski <glogow@fbihome.de>
> > Cc: stable@vger.kernel.org
> > ---
> > Resending this to update patchwork; will push in a little bit
> 
> Is there a bugzilla? Reference to a list discussion? Something with a
> dmesg where someone can actually verify this is the right fix?
This patch has actually been on the list for a while now-I have had mdnavare
take a look at it as well (they said it looked fine with the only change being
in regards to the comment), and it'd been on the list for a while already.

> 
> IMO needs an ack from Ville too. He should be in Cc: in the first place
> as the author of the regressing commit.
> 
> 'dim fixes c85d200e8321' gives you the output:
aaah-I had thought it was just for generating the Fixes line, I will be more
careful about that in the future
> 
> Fixes: c85d200e8321 ("drm/i915: Move SST DP link retraining into the
> ->post_hotplug() hook")
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v4.17+
> 
> BR,
> Jani.
> 
> > 
> >  drivers/gpu/drm/i915/intel_dp.c | 33 +++++++++++++++++++--------------
> >  1 file changed, 19 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index b3f6f04c3c7d..db8515171270 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4333,18 +4333,6 @@ intel_dp_needs_link_retrain(struct intel_dp
> > *intel_dp)
> >  	return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
> >  }
> >  
> > -/*
> > - * If display is now connected check links status,
> > - * there has been known issues of link loss triggering
> > - * long pulse.
> > - *
> > - * Some sinks (eg. ASUS PB287Q) seem to perform some
> > - * weird HPD ping pong during modesets. So we can apparently
> > - * end up with HPD going low during a modeset, and then
> > - * going back up soon after. And once that happens we must
> > - * retrain the link to get a picture. That's in case no
> > - * userspace component reacted to intermittent HPD dip.
> > - */
> >  int intel_dp_retrain_link(struct intel_encoder *encoder,
> >  			  struct drm_modeset_acquire_ctx *ctx)
> >  {
> > @@ -5031,7 +5019,8 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
> >  }
> >  
> >  static int
> > -intel_dp_long_pulse(struct intel_connector *connector)
> > +intel_dp_long_pulse(struct intel_connector *connector,
> > +		    struct drm_modeset_acquire_ctx *ctx)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> >  	struct intel_dp *intel_dp = intel_attached_dp(&connector->base);
> > @@ -5090,6 +5079,22 @@ intel_dp_long_pulse(struct intel_connector
> > *connector)
> >  		 */
> >  		status = connector_status_disconnected;
> >  		goto out;
> > +	} else {
> > +		/*
> > +		 * If display is now connected check links status,
> > +		 * there has been known issues of link loss triggering
> > +		 * long pulse.
> > +		 *
> > +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> > +		 * weird HPD ping pong during modesets. So we can apparently
> > +		 * end up with HPD going low during a modeset, and then
> > +		 * going back up soon after. And once that happens we must
> > +		 * retrain the link to get a picture. That's in case no
> > +		 * userspace component reacted to intermittent HPD dip.
> > +		 */
> > +		struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)-
> > >base;
> > +
> > +		intel_dp_retrain_link(encoder, ctx);
> >  	}
> >  
> >  	/*
> > @@ -5151,7 +5156,7 @@ intel_dp_detect(struct drm_connector *connector,
> >  				return ret;
> >  		}
> >  
> > -		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > +		status = intel_dp_long_pulse(intel_dp->attached_connector,
> > ctx);
> >  	}
> >  
> >  	intel_dp->detect_done = false;
> 
> 
-- 
Cheers,
	Lyude Paul

Re: [Intel-gfx] [PATCH v4] drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"

[Lyude Paul]

On Mon, 2018-08-27 at 15:08 +0300, Ville Syrjälä wrote:
> On Sat, Aug 25, 2018 at 03:10:35PM -0400, Lyude Paul wrote:
> > From: Jan-Marek Glogowski <glogow@fbihome.de>
> > 
> > This re-applies the workaround for "some DP sinks, [which] are a
> > little nuts" from commit 1a36147bb939 ("drm/i915: Perform link
> > quality check unconditionally during long pulse").
> > It makes the secondary AOC E2460P monitor connected via DP to an
> > acer Veriton N4640G usable again.
> > 
> > This hunk was dropped in commit c85d200e8321 ("drm/i915: Move SST
> > DP link retraining into the ->post_hotplug() hook")
> > 
> > Fixes: c85d200e8321 ("drm/i915: Move SST DP link retraining into the
> > ->post_hotplug() hook")
> > [Cleaned up commit message, added stable cc]
> > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > Signed-off-by: Jan-Marek Glogowski <glogow@fbihome.de>
> > Cc: stable@vger.kernel.org
> > ---
> > Resending this to update patchwork; will push in a little bit
> > 
> >  drivers/gpu/drm/i915/intel_dp.c | 33 +++++++++++++++++++--------------
> >  1 file changed, 19 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index b3f6f04c3c7d..db8515171270 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4333,18 +4333,6 @@ intel_dp_needs_link_retrain(struct intel_dp
> > *intel_dp)
> >  	return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
> >  }
> >  
> > -/*
> > - * If display is now connected check links status,
> > - * there has been known issues of link loss triggering
> > - * long pulse.
> > - *
> > - * Some sinks (eg. ASUS PB287Q) seem to perform some
> > - * weird HPD ping pong during modesets. So we can apparently
> > - * end up with HPD going low during a modeset, and then
> > - * going back up soon after. And once that happens we must
> > - * retrain the link to get a picture. That's in case no
> > - * userspace component reacted to intermittent HPD dip.
> > - */
> >  int intel_dp_retrain_link(struct intel_encoder *encoder,
> >  			  struct drm_modeset_acquire_ctx *ctx)
> >  {
> > @@ -5031,7 +5019,8 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
> >  }
> >  
> >  static int
> > -intel_dp_long_pulse(struct intel_connector *connector)
> > +intel_dp_long_pulse(struct intel_connector *connector,
> > +		    struct drm_modeset_acquire_ctx *ctx)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> >  	struct intel_dp *intel_dp = intel_attached_dp(&connector->base);
> > @@ -5090,6 +5079,22 @@ intel_dp_long_pulse(struct intel_connector
> > *connector)
> >  		 */
> >  		status = connector_status_disconnected;
> >  		goto out;
> > +	} else {
> > +		/*
> > +		 * If display is now connected check links status,
> > +		 * there has been known issues of link loss triggering
> > +		 * long pulse.
> > +		 *
> > +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> > +		 * weird HPD ping pong during modesets. So we can apparently
> > +		 * end up with HPD going low during a modeset, and then
> > +		 * going back up soon after. And once that happens we must
> > +		 * retrain the link to get a picture. That's in case no
> > +		 * userspace component reacted to intermittent HPD dip.
> > +		 */
> > +		struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)-
> > >base;
> > +
> > +		intel_dp_retrain_link(encoder, ctx);
> 
> We should really have a comment here that this is purely duct tape for
> sinks that fail to signal a hpd when the link goes bad (either that or
> we fail to process the hpd correctly).
I'm fine with adding another patch to clarify that comment as well

> 
> I suppose a better way to do this hack would be to do the link quality
> check at the end of modeset, or from a delayed work. As is this depends
> on userspace/fbdev doing an explicit probe after the modeset which seems
> pretty fragile.
I think having that at the end of a modeset in addition to this would be a
good idea as well, I don't see any harm in having an additional check in the
long pulse handler in case something causes the link to become unstable at
some point in the future after the initial modeset

Jan, do you think you could provide some dmesg logs for this issue?
> 
> >  	}
> >  
> >  	/*
> > @@ -5151,7 +5156,7 @@ intel_dp_detect(struct drm_connector *connector,
> >  				return ret;
> >  		}
> >  
> > -		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > +		status = intel_dp_long_pulse(intel_dp->attached_connector,
> > ctx);
> >  	}
> >  
> >  	intel_dp->detect_done = false;
> > -- 
> > 2.17.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
-- 
Cheers,
	Lyude Paul

Re: [Intel-gfx] [PATCH v4] drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"

[Ville Syrjälä]

On Mon, Aug 27, 2018 at 01:45:48PM -0400, Lyude Paul wrote:
> On Mon, 2018-08-27 at 15:08 +0300, Ville Syrjälä wrote:
> > On Sat, Aug 25, 2018 at 03:10:35PM -0400, Lyude Paul wrote:
> > > From: Jan-Marek Glogowski <glogow@fbihome.de>
> > > 
> > > This re-applies the workaround for "some DP sinks, [which] are a
> > > little nuts" from commit 1a36147bb939 ("drm/i915: Perform link
> > > quality check unconditionally during long pulse").
> > > It makes the secondary AOC E2460P monitor connected via DP to an
> > > acer Veriton N4640G usable again.
> > > 
> > > This hunk was dropped in commit c85d200e8321 ("drm/i915: Move SST
> > > DP link retraining into the ->post_hotplug() hook")
> > > 
> > > Fixes: c85d200e8321 ("drm/i915: Move SST DP link retraining into the
> > > ->post_hotplug() hook")
> > > [Cleaned up commit message, added stable cc]
> > > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > > Signed-off-by: Jan-Marek Glogowski <glogow@fbihome.de>
> > > Cc: stable@vger.kernel.org
> > > ---
> > > Resending this to update patchwork; will push in a little bit
> > > 
> > >  drivers/gpu/drm/i915/intel_dp.c | 33 +++++++++++++++++++--------------
> > >  1 file changed, 19 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index b3f6f04c3c7d..db8515171270 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4333,18 +4333,6 @@ intel_dp_needs_link_retrain(struct intel_dp
> > > *intel_dp)
> > >  	return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
> > >  }
> > >  
> > > -/*
> > > - * If display is now connected check links status,
> > > - * there has been known issues of link loss triggering
> > > - * long pulse.
> > > - *
> > > - * Some sinks (eg. ASUS PB287Q) seem to perform some
> > > - * weird HPD ping pong during modesets. So we can apparently
> > > - * end up with HPD going low during a modeset, and then
> > > - * going back up soon after. And once that happens we must
> > > - * retrain the link to get a picture. That's in case no
> > > - * userspace component reacted to intermittent HPD dip.
> > > - */
> > >  int intel_dp_retrain_link(struct intel_encoder *encoder,
> > >  			  struct drm_modeset_acquire_ctx *ctx)
> > >  {
> > > @@ -5031,7 +5019,8 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
> > >  }
> > >  
> > >  static int
> > > -intel_dp_long_pulse(struct intel_connector *connector)
> > > +intel_dp_long_pulse(struct intel_connector *connector,
> > > +		    struct drm_modeset_acquire_ctx *ctx)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > >  	struct intel_dp *intel_dp = intel_attached_dp(&connector->base);
> > > @@ -5090,6 +5079,22 @@ intel_dp_long_pulse(struct intel_connector
> > > *connector)
> > >  		 */
> > >  		status = connector_status_disconnected;
> > >  		goto out;
> > > +	} else {
> > > +		/*
> > > +		 * If display is now connected check links status,
> > > +		 * there has been known issues of link loss triggering
> > > +		 * long pulse.
> > > +		 *
> > > +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> > > +		 * weird HPD ping pong during modesets. So we can apparently
> > > +		 * end up with HPD going low during a modeset, and then
> > > +		 * going back up soon after. And once that happens we must
> > > +		 * retrain the link to get a picture. That's in case no
> > > +		 * userspace component reacted to intermittent HPD dip.
> > > +		 */
> > > +		struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)-
> > > >base;
> > > +
> > > +		intel_dp_retrain_link(encoder, ctx);
> > 
> > We should really have a comment here that this is purely duct tape for
> > sinks that fail to signal a hpd when the link goes bad (either that or
> > we fail to process the hpd correctly).
> I'm fine with adding another patch to clarify that comment as well
> 
> > 
> > I suppose a better way to do this hack would be to do the link quality
> > check at the end of modeset, or from a delayed work. As is this depends
> > on userspace/fbdev doing an explicit probe after the modeset which seems
> > pretty fragile.
> I think having that at the end of a modeset in addition to this would be a
> good idea as well, I don't see any harm in having an additional check in the
> long pulse handler in case something causes the link to become unstable at
> some point in the future after the initial modeset

We already have that via the hotplug hook. The problem here is that there
is apparently no hpd signalled by the sink.

Someone should probably rename intel_dp_long_pulse() to something
else so that people don't get the wrong impression that it's somehow
dedicated to handling long hpds. In fact just 
s/intel_dp_long_pulse()/intel_dp_detect()/ and sucking the few lines of
extra code into the function might be the right way to go.

> 
> Jan, do you think you could provide some dmesg logs for this issue?
> > 
> > >  	}
> > >  
> > >  	/*
> > > @@ -5151,7 +5156,7 @@ intel_dp_detect(struct drm_connector *connector,
> > >  				return ret;
> > >  		}
> > >  
> > > -		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > > +		status = intel_dp_long_pulse(intel_dp->attached_connector,
> > > ctx);
> > >  	}
> > >  
> > >  	intel_dp->detect_done = false;
> > > -- 
> > > 2.17.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 
> -- 
> Cheers,
> 	Lyude Paul

-- 
Ville Syrjälä
Intel

Re: [PATCH v4] drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"

[Rodrigo Vivi]

On Mon, Aug 27, 2018 at 01:39:02PM -0400, Lyude Paul wrote:
> On Mon, 2018-08-27 at 11:43 +0300, Jani Nikula wrote:
> > On Sat, 25 Aug 2018, Lyude Paul <lyude@redhat.com> wrote:
> > > From: Jan-Marek Glogowski <glogow@fbihome.de>
> > > 
> > > This re-applies the workaround for "some DP sinks, [which] are a
> > > little nuts" from commit 1a36147bb939 ("drm/i915: Perform link
> > > quality check unconditionally during long pulse").
> > > It makes the secondary AOC E2460P monitor connected via DP to an
> > > acer Veriton N4640G usable again.
> > > 
> > > This hunk was dropped in commit c85d200e8321 ("drm/i915: Move SST
> > > DP link retraining into the ->post_hotplug() hook")
> > > 
> > > Fixes: c85d200e8321 ("drm/i915: Move SST DP link retraining into the
> > > ->post_hotplug() hook")
> > > [Cleaned up commit message, added stable cc]
> > > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > > Signed-off-by: Jan-Marek Glogowski <glogow@fbihome.de>
> > > Cc: stable@vger.kernel.org
> > > ---
> > > Resending this to update patchwork; will push in a little bit
> > 
> > Is there a bugzilla? Reference to a list discussion? Something with a
> > dmesg where someone can actually verify this is the right fix?
> This patch has actually been on the list for a while now-I have had mdnavare
> take a look at it as well (they said it looked fine with the only change being
> in regards to the comment), and it'd been on the list for a while already.
> 
> > 
> > IMO needs an ack from Ville too. He should be in Cc: in the first place
> > as the author of the regressing commit.
> > 
> > 'dim fixes c85d200e8321' gives you the output:
> aaah-I had thought it was just for generating the Fixes line, I will be more
> careful about that in the future

Hi Lyude,

I understand that this patch was a while on the mailing list and no
proper attention, but besides the commit message information that
dim is generating I have a feeling now that this patch wasn't quite
ready yet and shouldn't be rushed in like it was.

Since you modified few versions ideally it would be good to have extra eyes
and the mandatory "Reviewed-by" there.

Also it is a good idea to give few days for different timezones to
comment out (after the re-sping I mean)

And finally based on the discussions it seems that there is still
some work to get done or a different way to handle this other
this back and forth of reapply-revert-reapply...

I just noticed it when preparing drm-intel-fixes pull request.

Since I'm not 100% confident this patch should be here I'm removing it
from this week's pull request.

But probably adding it next week if Jani and Ville give and ack
or indication that it is right or at least he best we could do now.

thanks,
Rodrigo.


> > 
> > Fixes: c85d200e8321 ("drm/i915: Move SST DP link retraining into the
> > ->post_hotplug() hook")
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org
> > Cc: <stable@vger.kernel.org> # v4.17+
> > 
> > BR,
> > Jani.
> > 
> > > 
> > >  drivers/gpu/drm/i915/intel_dp.c | 33 +++++++++++++++++++--------------
> > >  1 file changed, 19 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index b3f6f04c3c7d..db8515171270 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4333,18 +4333,6 @@ intel_dp_needs_link_retrain(struct intel_dp
> > > *intel_dp)
> > >  	return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
> > >  }
> > >  
> > > -/*
> > > - * If display is now connected check links status,
> > > - * there has been known issues of link loss triggering
> > > - * long pulse.
> > > - *
> > > - * Some sinks (eg. ASUS PB287Q) seem to perform some
> > > - * weird HPD ping pong during modesets. So we can apparently
> > > - * end up with HPD going low during a modeset, and then
> > > - * going back up soon after. And once that happens we must
> > > - * retrain the link to get a picture. That's in case no
> > > - * userspace component reacted to intermittent HPD dip.
> > > - */
> > >  int intel_dp_retrain_link(struct intel_encoder *encoder,
> > >  			  struct drm_modeset_acquire_ctx *ctx)
> > >  {
> > > @@ -5031,7 +5019,8 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
> > >  }
> > >  
> > >  static int
> > > -intel_dp_long_pulse(struct intel_connector *connector)
> > > +intel_dp_long_pulse(struct intel_connector *connector,
> > > +		    struct drm_modeset_acquire_ctx *ctx)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > >  	struct intel_dp *intel_dp = intel_attached_dp(&connector->base);
> > > @@ -5090,6 +5079,22 @@ intel_dp_long_pulse(struct intel_connector
> > > *connector)
> > >  		 */
> > >  		status = connector_status_disconnected;
> > >  		goto out;
> > > +	} else {
> > > +		/*
> > > +		 * If display is now connected check links status,
> > > +		 * there has been known issues of link loss triggering
> > > +		 * long pulse.
> > > +		 *
> > > +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> > > +		 * weird HPD ping pong during modesets. So we can apparently
> > > +		 * end up with HPD going low during a modeset, and then
> > > +		 * going back up soon after. And once that happens we must
> > > +		 * retrain the link to get a picture. That's in case no
> > > +		 * userspace component reacted to intermittent HPD dip.
> > > +		 */
> > > +		struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)-
> > > >base;
> > > +
> > > +		intel_dp_retrain_link(encoder, ctx);
> > >  	}
> > >  
> > >  	/*
> > > @@ -5151,7 +5156,7 @@ intel_dp_detect(struct drm_connector *connector,
> > >  				return ret;
> > >  		}
> > >  
> > > -		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > > +		status = intel_dp_long_pulse(intel_dp->attached_connector,
> > > ctx);
> > >  	}
> > >  
> > >  	intel_dp->detect_done = false;
> > 
> > 
> -- 
> Cheers,
> 	Lyude Paul
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4] drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"

[Lyude Paul]

On Wed, 2018-08-29 at 14:22 -0700, Rodrigo Vivi wrote:
> On Mon, Aug 27, 2018 at 01:39:02PM -0400, Lyude Paul wrote:
> > On Mon, 2018-08-27 at 11:43 +0300, Jani Nikula wrote:
> > > On Sat, 25 Aug 2018, Lyude Paul <lyude@redhat.com> wrote:
> > > > From: Jan-Marek Glogowski <glogow@fbihome.de>
> > > > 
> > > > This re-applies the workaround for "some DP sinks, [which] are a
> > > > little nuts" from commit 1a36147bb939 ("drm/i915: Perform link
> > > > quality check unconditionally during long pulse").
> > > > It makes the secondary AOC E2460P monitor connected via DP to an
> > > > acer Veriton N4640G usable again.
> > > > 
> > > > This hunk was dropped in commit c85d200e8321 ("drm/i915: Move SST
> > > > DP link retraining into the ->post_hotplug() hook")
> > > > 
> > > > Fixes: c85d200e8321 ("drm/i915: Move SST DP link retraining into the
> > > > ->post_hotplug() hook")
> > > > [Cleaned up commit message, added stable cc]
> > > > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > > > Signed-off-by: Jan-Marek Glogowski <glogow@fbihome.de>
> > > > Cc: stable@vger.kernel.org
> > > > ---
> > > > Resending this to update patchwork; will push in a little bit
> > > 
> > > Is there a bugzilla? Reference to a list discussion? Something with a
> > > dmesg where someone can actually verify this is the right fix?
> > 
> > This patch has actually been on the list for a while now-I have had mdnavare
> > take a look at it as well (they said it looked fine with the only change
> > being
> > in regards to the comment), and it'd been on the list for a while already.
> > 
> > > 
> > > IMO needs an ack from Ville too. He should be in Cc: in the first place
> > > as the author of the regressing commit.
> > > 
> > > 'dim fixes c85d200e8321' gives you the output:
> > 
> > aaah-I had thought it was just for generating the Fixes line, I will be more
> > careful about that in the future
> 
> Hi Lyude,
> 
> I understand that this patch was a while on the mailing list and no
> proper attention, but besides the commit message information that
> dim is generating I have a feeling now that this patch wasn't quite
> ready yet and shouldn't be rushed in like it was.
> 
> Since you modified few versions ideally it would be good to have extra eyes
> and the mandatory "Reviewed-by" there.
> 
> Also it is a good idea to give few days for different timezones to
> comment out (after the re-sping I mean)
> 
> And finally based on the discussions it seems that there is still
> some work to get done or a different way to handle this other
> this back and forth of reapply-revert-reapply...
> 
> I just noticed it when preparing drm-intel-fixes pull request.
> 
> Since I'm not 100% confident this patch should be here I'm removing it
> from this week's pull request.
> 
> But probably adding it next week if Jani and Ville give and ack
> or indication that it is right or at least he best we could do now.
   I understand! Sorry about all of the trouble, I will be much more careful in
   the future
> 
> thanks,
> Rodrigo.
> 
> 
> > > 
> > > Fixes: c85d200e8321 ("drm/i915: Move SST DP link retraining into the
> > > ->post_hotplug() hook")
> > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Lyude Paul <lyude@redhat.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: intel-gfx@lists.freedesktop.org
> > > Cc: <stable@vger.kernel.org> # v4.17+
> > > 
> > > BR,
> > > Jani.
> > > 
> > > > 
> > > >  drivers/gpu/drm/i915/intel_dp.c | 33 +++++++++++++++++++--------------
> > > >  1 file changed, 19 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index b3f6f04c3c7d..db8515171270 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -4333,18 +4333,6 @@ intel_dp_needs_link_retrain(struct intel_dp
> > > > *intel_dp)
> > > >  	return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
> > > >  }
> > > >  
> > > > -/*
> > > > - * If display is now connected check links status,
> > > > - * there has been known issues of link loss triggering
> > > > - * long pulse.
> > > > - *
> > > > - * Some sinks (eg. ASUS PB287Q) seem to perform some
> > > > - * weird HPD ping pong during modesets. So we can apparently
> > > > - * end up with HPD going low during a modeset, and then
> > > > - * going back up soon after. And once that happens we must
> > > > - * retrain the link to get a picture. That's in case no
> > > > - * userspace component reacted to intermittent HPD dip.
> > > > - */
> > > >  int intel_dp_retrain_link(struct intel_encoder *encoder,
> > > >  			  struct drm_modeset_acquire_ctx *ctx)
> > > >  {
> > > > @@ -5031,7 +5019,8 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
> > > >  }
> > > >  
> > > >  static int
> > > > -intel_dp_long_pulse(struct intel_connector *connector)
> > > > +intel_dp_long_pulse(struct intel_connector *connector,
> > > > +		    struct drm_modeset_acquire_ctx *ctx)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = to_i915(connector-
> > > > >base.dev);
> > > >  	struct intel_dp *intel_dp = intel_attached_dp(&connector->base);
> > > > @@ -5090,6 +5079,22 @@ intel_dp_long_pulse(struct intel_connector
> > > > *connector)
> > > >  		 */
> > > >  		status = connector_status_disconnected;
> > > >  		goto out;
> > > > +	} else {
> > > > +		/*
> > > > +		 * If display is now connected check links status,
> > > > +		 * there has been known issues of link loss triggering
> > > > +		 * long pulse.
> > > > +		 *
> > > > +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> > > > +		 * weird HPD ping pong during modesets. So we can
> > > > apparently
> > > > +		 * end up with HPD going low during a modeset, and then
> > > > +		 * going back up soon after. And once that happens we
> > > > must
> > > > +		 * retrain the link to get a picture. That's in case no
> > > > +		 * userspace component reacted to intermittent HPD dip.
> > > > +		 */
> > > > +		struct intel_encoder *encoder =
> > > > &dp_to_dig_port(intel_dp)-
> > > > > base;
> > > > 
> > > > +
> > > > +		intel_dp_retrain_link(encoder, ctx);
> > > >  	}
> > > >  
> > > >  	/*
> > > > @@ -5151,7 +5156,7 @@ intel_dp_detect(struct drm_connector *connector,
> > > >  				return ret;
> > > >  		}
> > > >  
> > > > -		status = intel_dp_long_pulse(intel_dp-
> > > > >attached_connector);
> > > > +		status = intel_dp_long_pulse(intel_dp-
> > > > >attached_connector,
> > > > ctx);
> > > >  	}
> > > >  
> > > >  	intel_dp->detect_done = false;
> > > 
> > > 
> > 
> > -- 
> > Cheers,
> > 	Lyude Paul
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4] drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"

[Rodrigo Vivi]

On Wed, Aug 29, 2018 at 05:28:23PM -0400, Lyude Paul wrote:
> On Wed, 2018-08-29 at 14:22 -0700, Rodrigo Vivi wrote:
> > On Mon, Aug 27, 2018 at 01:39:02PM -0400, Lyude Paul wrote:
> > > On Mon, 2018-08-27 at 11:43 +0300, Jani Nikula wrote:
> > > > On Sat, 25 Aug 2018, Lyude Paul <lyude@redhat.com> wrote:
> > > > > From: Jan-Marek Glogowski <glogow@fbihome.de>
> > > > > 
> > > > > This re-applies the workaround for "some DP sinks, [which] are a
> > > > > little nuts" from commit 1a36147bb939 ("drm/i915: Perform link
> > > > > quality check unconditionally during long pulse").
> > > > > It makes the secondary AOC E2460P monitor connected via DP to an
> > > > > acer Veriton N4640G usable again.
> > > > > 
> > > > > This hunk was dropped in commit c85d200e8321 ("drm/i915: Move SST
> > > > > DP link retraining into the ->post_hotplug() hook")
> > > > > 
> > > > > Fixes: c85d200e8321 ("drm/i915: Move SST DP link retraining into the
> > > > > ->post_hotplug() hook")
> > > > > [Cleaned up commit message, added stable cc]
> > > > > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > > > > Signed-off-by: Jan-Marek Glogowski <glogow@fbihome.de>
> > > > > Cc: stable@vger.kernel.org
> > > > > ---
> > > > > Resending this to update patchwork; will push in a little bit
> > > > 
> > > > Is there a bugzilla? Reference to a list discussion? Something with a
> > > > dmesg where someone can actually verify this is the right fix?
> > > 
> > > This patch has actually been on the list for a while now-I have had mdnavare
> > > take a look at it as well (they said it looked fine with the only change
> > > being
> > > in regards to the comment), and it'd been on the list for a while already.
> > > 
> > > > 
> > > > IMO needs an ack from Ville too. He should be in Cc: in the first place
> > > > as the author of the regressing commit.
> > > > 
> > > > 'dim fixes c85d200e8321' gives you the output:
> > > 
> > > aaah-I had thought it was just for generating the Fixes line, I will be more
> > > careful about that in the future

no worries and definitely no trouble at all! :)

> > 
> > Hi Lyude,
> > 
> > I understand that this patch was a while on the mailing list and no
> > proper attention, but besides the commit message information that
> > dim is generating I have a feeling now that this patch wasn't quite
> > ready yet and shouldn't be rushed in like it was.
> > 
> > Since you modified few versions ideally it would be good to have extra eyes
> > and the mandatory "Reviewed-by" there.
> > 
> > Also it is a good idea to give few days for different timezones to
> > comment out (after the re-sping I mean)
> > 
> > And finally based on the discussions it seems that there is still
> > some work to get done or a different way to handle this other
> > this back and forth of reapply-revert-reapply...
> > 
> > I just noticed it when preparing drm-intel-fixes pull request.
> > 
> > Since I'm not 100% confident this patch should be here I'm removing it
> > from this week's pull request.
> > 
> > But probably adding it next week if Jani and Ville give and ack
> > or indication that it is right or at least he best we could do now.
>    I understand! Sorry about all of the trouble, I will be much more careful in
>    the future
> > 
> > thanks,
> > Rodrigo.
> > 
> > 
> > > > 
> > > > Fixes: c85d200e8321 ("drm/i915: Move SST DP link retraining into the
> > > > ->post_hotplug() hook")
> > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: intel-gfx@lists.freedesktop.org
> > > > Cc: <stable@vger.kernel.org> # v4.17+
> > > > 
> > > > BR,
> > > > Jani.
> > > > 
> > > > > 
> > > > >  drivers/gpu/drm/i915/intel_dp.c | 33 +++++++++++++++++++--------------
> > > > >  1 file changed, 19 insertions(+), 14 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index b3f6f04c3c7d..db8515171270 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -4333,18 +4333,6 @@ intel_dp_needs_link_retrain(struct intel_dp
> > > > > *intel_dp)
> > > > >  	return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
> > > > >  }
> > > > >  
> > > > > -/*
> > > > > - * If display is now connected check links status,
> > > > > - * there has been known issues of link loss triggering
> > > > > - * long pulse.
> > > > > - *
> > > > > - * Some sinks (eg. ASUS PB287Q) seem to perform some
> > > > > - * weird HPD ping pong during modesets. So we can apparently
> > > > > - * end up with HPD going low during a modeset, and then
> > > > > - * going back up soon after. And once that happens we must
> > > > > - * retrain the link to get a picture. That's in case no
> > > > > - * userspace component reacted to intermittent HPD dip.
> > > > > - */
> > > > >  int intel_dp_retrain_link(struct intel_encoder *encoder,
> > > > >  			  struct drm_modeset_acquire_ctx *ctx)
> > > > >  {
> > > > > @@ -5031,7 +5019,8 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
> > > > >  }
> > > > >  
> > > > >  static int
> > > > > -intel_dp_long_pulse(struct intel_connector *connector)
> > > > > +intel_dp_long_pulse(struct intel_connector *connector,
> > > > > +		    struct drm_modeset_acquire_ctx *ctx)
> > > > >  {
> > > > >  	struct drm_i915_private *dev_priv = to_i915(connector-
> > > > > >base.dev);
> > > > >  	struct intel_dp *intel_dp = intel_attached_dp(&connector->base);
> > > > > @@ -5090,6 +5079,22 @@ intel_dp_long_pulse(struct intel_connector
> > > > > *connector)
> > > > >  		 */
> > > > >  		status = connector_status_disconnected;
> > > > >  		goto out;
> > > > > +	} else {
> > > > > +		/*
> > > > > +		 * If display is now connected check links status,
> > > > > +		 * there has been known issues of link loss triggering
> > > > > +		 * long pulse.
> > > > > +		 *
> > > > > +		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> > > > > +		 * weird HPD ping pong during modesets. So we can
> > > > > apparently
> > > > > +		 * end up with HPD going low during a modeset, and then
> > > > > +		 * going back up soon after. And once that happens we
> > > > > must
> > > > > +		 * retrain the link to get a picture. That's in case no
> > > > > +		 * userspace component reacted to intermittent HPD dip.
> > > > > +		 */
> > > > > +		struct intel_encoder *encoder =
> > > > > &dp_to_dig_port(intel_dp)-
> > > > > > base;
> > > > > 
> > > > > +
> > > > > +		intel_dp_retrain_link(encoder, ctx);
> > > > >  	}
> > > > >  
> > > > >  	/*
> > > > > @@ -5151,7 +5156,7 @@ intel_dp_detect(struct drm_connector *connector,
> > > > >  				return ret;
> > > > >  		}
> > > > >  
> > > > > -		status = intel_dp_long_pulse(intel_dp-
> > > > > >attached_connector);
> > > > > +		status = intel_dp_long_pulse(intel_dp-
> > > > > >attached_connector,
> > > > > ctx);
> > > > >  	}
> > > > >  
> > > > >  	intel_dp->detect_done = false;
> > > > 
> > > > 
> > > 
> > > -- 
> > > Cheers,
> > > 	Lyude Paul
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx