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

[Daniel Vetter]

On Mon, Dec 01, 2014 at 03:04:50PM -0200, Paulo Zanoni wrote:
> 2014-12-01 14:36 GMT-02:00 Daniel Vetter <daniel at ffwll.ch>:
> > On Mon, Dec 01, 2014 at 11:41:42AM -0200, Paulo Zanoni wrote:
> >> 2014-11-26 16:17 GMT-02:00 Daniel Vetter <daniel at ffwll.ch>:
> >> > 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 ;-)
> >>
> >> But why does he say it works? Aren't we just delaying the DRM_ERROR message?
> >
> > Before we only disabled pch underruns while we disable the pch. But at the
> > end of the ->crtc_disable hook pch underrun reporting is enabled.
> >
> > With my patch we keep pch underrun reporting disabled until ->crtc_enable.
> > It seems like doing a modeset on the other pipe also gives us underruns on
> > disabled pipes somehow. Or at least that's my (bad) theory.
> 
> I guess you convinced me on IRC that this is better than reverting the
> DRM_ERROR to DRM_DEBUG_KMS.
> 
> Anyway, the patch does what it says and doesn't seem to add any
> regressions, so Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>.

Since this dmesg noise is a regression, also Cc: stable at vger.kernel.org

Jani, can you pls pick this patch up? Perhaps for the record it would be
best to paste the entire discussion here into the commit log, too.

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