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

[Paulo Zanoni]

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?

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

I still fail to understand how this patch is an improvement to the code base.

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



-- 
Paulo Zanoni