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@xxxxxxxxxxxxxxx wrote: > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
