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. > + 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); } 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. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
