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

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

[Daniel Vetter]

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. 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. So not a terrible reduction in test
coverage.

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@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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: More cautious with pch fifo underruns

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  367/367              367/367
ILK                 -12              375/375              363/375
SNB                                  450/450              450/450
IVB                 -2              503/503              501/503
BYT                                  289/289              289/289
HSW                 -3              567/567              564/567
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 ILK  igt_gem_reset_stats_close-pending-fork-render      TIMEOUT(9, M37M26)PASS(1, M26)      TIMEOUT(1, M26)
*ILK  igt_kms_3d      PASS(3, M37M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_nonexisting-fb      DMESG_WARN(2, M26)PASS(3, M37M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_rcs-wf_vblank-vs-modeset      PASS(2, M37M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_blocking-absolute-wf_vblank-interruptible      DMESG_WARN(2, M26)PASS(2, M37M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_flip-vs-dpms      DMESG_WARN(1, M26)PASS(4, M37M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_flip-vs-rmfb-interruptible      DMESG_WARN(1, M26)PASS(2, M37M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_plain-flip      DMESG_WARN(1, M26)PASS(2, M37M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_rcs-flip-vs-modeset-interruptible      PASS(3, M37M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_vblank-vs-hang      TIMEOUT(9, M37M26)PASS(1, M26)      TIMEOUT(1, M26)
*ILK  igt_kms_flip_wf_vblank-ts-check      PASS(3, M37M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_wf_vblank-ts-check-interruptible      DMESG_WARN(1, M26)PASS(3, M37M26)      DMESG_WARN(1, M26)
 IVB  igt_gem_bad_reloc_negative-reloc      NSPT(14, M34M21M4)PASS(1, M21)      NSPT(1, M4)
 IVB  igt_gem_bad_reloc_negative-reloc-lut      NSPT(3, M21M34M4)PASS(15, M21M34M4)      NSPT(1, M4)
 HSW  igt_gem_bad_reloc_negative-reloc-lut      NSPT(24, M40M20M19)PASS(1, M20)      NSPT(1, M19)
*HSW  igt_kms_rotation_crc_primary-rotation      PASS(23, M20M40M19)      DMESG_WARN(1, M19)
*HSW  igt_pm_rc6_residency_rc6-accuracy      PASS(25, M20M40M19)      FAIL(1, M19)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: More cautious with pch fifo underruns

[Daniel Vetter]

On Mon, Nov 24, 2014 at 05:02:45PM +0100, Daniel Vetter wrote:
> 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. 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. So not a terrible reduction in test
> coverage.
> 
> 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@intel.com>

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85898
Tested-by: lu hua <huax.lu@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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: More cautious with pch fifo underruns

[Paulo Zanoni]

2014-11-24 14:02 GMT-02:00 Daniel Vetter <daniel.vetter@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.

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


> So not a terrible reduction in test
> coverage.

Yeah, I agree, but please provide a nice reason for it :)

>
> 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@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: More cautious with pch fifo underruns

[Ville Syrjälä]

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

IIRC the conclusion I came to when looking at my IVB was that the underruns
were expected when fdi was disabled but the pch transcoder was enabled,
or maybe it was between pipe disable and pch transcoder disable. Either
or both would make sense to me.

But we already have the underrun reporting disabled around those parts,
so my memory might be failing me here. Also my IVB doesn't seem to
trigger these anymore. Not sure what exactly has changed.

Or maybe I used to get them in the crtc_enable path. We do enable the
underrun reporting already before fdi or pch transcoder are enabled.
Although we do the enable in order, so if my memory about the conditions
is correct we shouldn't get any there either.

> 
> Please explain a little more.
> 
> > 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?
> 
> 
> > So not a terrible reduction in test
> > coverage.
> 
> Yeah, I agree, but please provide a nice reason for it :)
> 
> >
> > 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@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@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: More cautious with pch fifo underruns

[Paulo Zanoni]

2014-11-26 16:17 GMT-02:00 Daniel Vetter <daniel@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@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@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: More cautious with pch fifo underruns

[Daniel Vetter]

On Mon, Dec 01, 2014 at 11:41:42AM -0200, Paulo Zanoni wrote:
> 2014-11-26 16:17 GMT-02:00 Daniel Vetter <daniel@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@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.

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: More cautious with pch fifo underruns

[Paulo Zanoni]

2014-12-01 14:36 GMT-02:00 Daniel Vetter <daniel@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@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@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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@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@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@intel.com>.

Since this dmesg noise is a regression, also Cc: stable@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: More cautious with pch fifo underruns

[Jani Nikula]

On Mon, 01 Dec 2014, Daniel Vetter <daniel@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@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@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@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@intel.com>.
>
> Since this dmesg noise is a regression, also Cc: stable@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx