On Fri, Oct 30, 2015 at 12:06:09PM +0200, Jani Nikula wrote:
> On Thu, 29 Oct 2015, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > We get spurious PCH FIFO underruns if we enable the reporting too soon
> > after enabling the crtc. Move it to be the last step, after the encoder
> > enable. Additionally we need an extra vblank wait, otherwise we still
> > get the underruns. Presumably the pipe/fdi isn't yet fully up and running
> > otherwise.
> >
> > For symmetry, disable the PCH underrun reporting as the first thing,
> > just before encoder disable, when shutting down the crtc.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 13 +++++++++----
> > 1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 99fb33f..d5cb899 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4874,7 +4874,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> > intel_crtc->active = true;
> >
> > intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> > - intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
> >
> > for_each_encoder_on_crtc(dev, crtc, encoder)
> > if (encoder->pre_enable)
> > @@ -4912,6 +4911,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> >
> > if (HAS_PCH_CPT(dev))
> > cpt_verify_modeset(dev, intel_crtc->pipe);
> > +
> > + if (intel_crtc->config->has_pch_encoder) {
> > + /* Must wait for vblank to avoid spurious PCH FIFO underruns */
> > + intel_wait_for_vblank(dev, pipe);
> > + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
>
> Nitpick, moving this within the if (has_pch_encoder) isn't documented in
> the commit message. Does that change have an impact?
I don't much of a real concern here. I think the following might
happen (all on the same pipe):
1. enable PCH port
2. disable PCH port
3. PCH FIFO underrun just after we've re-enabled the PCH
underrun reporting
4. enable port A
5. PCH FIFO underrun reporting isn't enabled anymore for this pipe
But since it's driving a non-PCH port anyway, that doesn't seem like
a huge worry. But I suppose I could change it to always enable PCH
FIFO underrun reporting even for port A. It should do no harm at least.
>
> BR,
> Jani.
>
> > + }
> > }
> >
> > /* IPS only exists on ULT machines and is tied to pipe A. */
> > @@ -5040,15 +5045,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
> > int pipe = intel_crtc->pipe;
> > u32 reg, temp;
> >
> > + if (intel_crtc->config->has_pch_encoder)
> > + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> > +
> > for_each_encoder_on_crtc(dev, crtc, encoder)
> > encoder->disable(encoder);
> >
> > drm_crtc_vblank_off(crtc);
> > assert_vblank_disabled(crtc);
> >
> > - if (intel_crtc->config->has_pch_encoder)
> > - intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> > -
> > intel_disable_pipe(intel_crtc);
> >
> > ironlake_pfit_disable(intel_crtc, false);
>
> --
> Jani Nikula, Intel Open Source Technology Center
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
