Re: [PATCH 05/14] drm/i915: Re-enable PCH FIO underrun reporting after pipe has been disabled

Ville Syrjälä
2015-10-29T21:39:21Z

On Thu, Oct 29, 2015 at 12:36:34PM -0700, Jesse Barnes wrote:
> On 10/29/2015 12:25 PM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Some hardware (IVB/HSW and CPT/PPT) have a shared error interrupt for
> > all the relevant underrun bits, so in order to keep the error interrupt
> > enabled, we need to have underrun reporting enabled on all PCH
> > transocders. Currently we leave the underrun reporting disabled when
> > the pipe is off, which means we won't get any underrun interrupts
> > when only a subset of the pipes are active.
> >
> > Fix the problem by re-enabling the underrun reporting after the pipe has
> > been disabled. And to avoid the spurious underruns during pipe enable,
> > disable the underrun reporting before embarking on the pipe enable
> > sequence. So this way we have the error reporting disabled while
> > running through the modeset sequence.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 4fc3d24..c7cd9f7 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4857,6 +4857,9 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> > return;
> >
> > if (intel_crtc->config->has_pch_encoder)
> > + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> > +
> > + if (intel_crtc->config->has_pch_encoder)
> > intel_prepare_shared_dpll(intel_crtc);
>
> I guess these could be combined under the conditional, but no biggie.

I did notice the same thing just before sending the patch, but then I
convinced myself that having all the pch underrun enable/disable calls
as clearly separate steps is perhaps nicer, and so didn't bother
changing it.

>
> >
> > if (intel_crtc->config->has_dp_encoder)
> > @@ -4939,6 +4942,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
> > if (WARN_ON(intel_crtc->active))
> > return;
> >
> > + if (intel_crtc->config->has_pch_encoder)
> > + intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
> > + false);
> > +
> > if (intel_crtc_to_shared_dpll(intel_crtc))
> > intel_enable_shared_dpll(intel_crtc);
> >
> > @@ -5086,6 +5093,9 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
> >
> > ironlake_fdi_pll_disable(intel_crtc);
> > }
> > +
> > + if (intel_crtc->config->has_pch_encoder)
> > + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
> > }
> >
> > static void haswell_crtc_disable(struct drm_crtc *crtc)
> > @@ -5133,6 +5143,10 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
> > for_each_encoder_on_crtc(dev, crtc, encoder)
> > if (encoder->post_disable)
> > encoder->post_disable(encoder);
> > +
> > + if (intel_crtc->config->has_pch_encoder)
> > + intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
> > + true);
> > }
> >
> > static void i9xx_pfit_enable(struct intel_crtc *crtc)
> >
>
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

--
Ville Syrjälä
Intel OTC