[Intel-gfx] [PATCH] drm/i915: More cautious with pch fifo underruns

[Daniel Vetter]

On Wed, Nov 26, 2014 at 01:37:07PM -0200, Paulo Zanoni wrote:
> 2014-11-24 14:02 GMT-02:00 Daniel Vetter <daniel.vetter at ffwll.ch>:
> > Apparently PCH fifo underruns are tricky, we have plenty reports that
> > we see the occasional underrun (especially at boot-up).
> >
> > So for a change let's see what happens when we don't re-enable pch
> > fifo underrun reporting when the pipe is disabled.
> 
> Does that mean you don't really know if this patch is going to fix something?
> 
> I see what this patch does, but I don't really see what is its
> benefit, besides "we'll get less bug reports". Is there any reason why
> the underruns are expected to happen at this time?
> 
> Please explain a little more.

No reason really beyond "less bug reports" and "no reduction in underrun
reporting abilities when the pipe is actually enabled". Only a reduction
in how quickly we'll notice an underrun, but since we mostly need cpu fifo
underruns for debugging wm issues I don't think that has an impact for
developers either. fifo underruns are useful for debugging some modeset
issues, but as soon as you do modeset we'll spot the underrun.

> > This means that the
> > kernel can't catch pch fifo underruns when they happen (except when
> > all pipes are on on the pch). But we'll still catch underruns when
> > disabling the pipe again.
> 
> Are you sure the sentences above are correct?

We always re-enable underrun reporting in the crtc_enable hooks. That
still doesn't enable the interrupts (when some other pch pipe is off), but
it updates the sw tracking.

When we again disable the fifo underrun reporting we do check the status
bits, so if an underrun happened we won't get the interrupt right away.
But when you shut down the pipe we'll notice that an interrupt happened.

So yeah, the above claim should be correct.

> > So not a terrible reduction in test
> > coverage.
> 
> Yeah, I agree, but please provide a nice reason for it :)

See my reply to this patch, a bug reporter came around and tested this as
"it works". I really do send out patches without testing them at all for
bug team work ;-)

So if I adjust the patch to reflect that (and upgrade the relevant bz link
from References: to Bugzilla: plus add the tested by) are you ok with
merging this one?.

Cheers, Daniel


> 
> >
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=85898
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=86233
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=86478
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 320bf4c78c8c..a4106049e158 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4501,7 +4501,6 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
> >                 ironlake_fdi_disable(crtc);
> >
> >                 ironlake_disable_pch_transcoder(dev_priv, pipe);
> > -               intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
> >
> >                 if (HAS_PCH_CPT(dev)) {
> >                         /* disable TRANS_DP_CTL */
> > @@ -4572,8 +4571,6 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
> >
> >         if (intel_crtc->config.has_pch_encoder) {
> >                 lpt_disable_pch_transcoder(dev_priv);
> > -               intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
> > -                                                     true);
> >                 intel_ddi_fdi_disable(crtc);
> >         }
> >
> > --
> > 2.1.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch