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

[Jani Nikula]

On Mon, 01 Dec 2014, Daniel Vetter <daniel at ffwll.ch> wrote:
> 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.

Pushed to drm-intel-fixes, thanks for the patch and review.

BR,
Jani.


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

-- 
Jani Nikula, Intel Open Source Technology Center