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@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
> > > 
> > >  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@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 
> -- 
> Cheers,
> 	Lyude Paul

-- 
Ville Syrjälä
Intel