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

[Paulo Zanoni]

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>.

>
> So no we're not just delaying them since the time when they're block is
> now _much_ longer (the entire time when the pch transcoder is off).
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni