Re: [PATCH 2/2] drm/i915: Suppress underruns during DP link retraining

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Oct 14, 2016 at 06:27:49PM +0100, Chris Wilson wrote:
> On Fri, Oct 14, 2016 at 08:02:54PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > DP link retraining causes (spurious?) underruns. We can't really avoid
> > them, except perhaps by doing a full modeset (which has its own underrun
> > suppression anyway). So let's just hide them.
> > 
> > MST still has its own logic for retrainin, but a bigger hpd handling
> > cleanup/unification is needed there anyway, so let's leave that be for now.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=98251
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 29 +++++++++++++++++++++++++++--
> >  1 file changed, 27 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index bc03f61d94f1..780691a34133 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3988,6 +3988,31 @@ go_again:
> >  }
> >  
> >  static void
> > +intel_dp_retrain_link(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > +	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> > +
> > +	/* Suppress underruns caused by re-training */
> > +	intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
> > +	if (crtc->config->has_pch_encoder)
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv,
> > +						      intel_crtc_pch_transcoder(crtc), false);
> > +
> > +	intel_dp_start_link_train(intel_dp);
> > +	intel_dp_stop_link_train(intel_dp);
> > +
> > +	/* Keep underrun reporting disabled until things are stable */
> > +	intel_wait_for_vblank(&dev_priv->drm, crtc->pipe);
> 
> I panicked over the irqreturn for hpd_pulse. That's a scary way of
> saying the return type is boolean.

It's been known to cause bugs too when people don't remember what it
means.

> 
> > +	intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true);
> > +	if (crtc->config->has_pch_encoder)
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv,
> > +						      intel_crtc_pch_transcoder(crtc), true);
> 
> These are becoming quite popular. Is has_pch_encoder generic enough for
> all crtc?
> 
> intel_crtc_suppress_underruns(struct intel_crtc *crtc, bool state)
> {
> 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> 
> 	if (!state)
> 		intel_wait_for_vblank(&dev_priv->drm, crtc->pipe);
> 
> 	intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, state);
> 	if (crtc->config->has_pch_encoder)
> 		intel_set_pch_fifo_underrun_reporting(dev_priv,
> 						      intel_crtc_pch_transcoder(crtc),
> 						      state);
> }

Hmm. I suppose we could replace a the ones in dp/hdmi/sdvo code
with something like that. Would need to use
intel_wait_for_vblank_if_active() though.

The ones directly in the crtc enable/disable paths paths aren't quite
that straightforward though. Making them simpler might ease the
maintenance burden though, with the cost of a slight loss in coverage.

> 
> If this was igt, we would be writing all kinds of silly
> 
> #define intel_crtc_suppress_underruns(crtc, state) \
> 	for (state = true; __intel_crtc_suppress_underruns(crtc, state); state = false)
> 
> 
> intel_crtc_suppress_underruns(crtc, state) {
> 	intel_dp_start_link_train(intel_dp);
> 	intel_dp_stop_link_train(intel_dp);
> }
> 
> Other than bikeshedding to see if we can trim down the profileration,
> this looks inline with the suppression everywhere else, so
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Main concern was over the vblank wait making short/long pulse handling
> slower, but the code looks like it will survive - and the only
> noticeable artifacts would be if link training failed anyway with the
> small delay before recovery.

The pipe should be running still so at least it shouldn't have to wait
for a timeout.

In general the retraining doesn't feel entirely robust. If I try force a
retraining on my ILK or IVB when the link is actually good, it generally
doesn't succeed. If I toggle the port state at the start of the
retraining it starts to work. The spec even says we should do that, so
might be we should go for it. I think I'll need to digest this idea a
bit more thogh, as I haven't pondered the ramifications thorougly.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx