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@xxxxxxxxxx> wrote:
> > From: Jan-Marek Glogowski <glogow@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>
> > Signed-off-by: Jan-Marek Glogowski <glogow@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> > 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@xxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Ville SyrjÃlà <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Lyude Paul <lyude@xxxxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: <stable@xxxxxxxxxxxxxxx> # 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