* [PATCH] drm/i915: More cautious with pch fifo underruns @ 2014-11-24 16:02 Daniel Vetter 2014-11-25 3:53 ` shuang.he ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Daniel Vetter @ 2014-11-24 16:02 UTC (permalink / raw) To: Intel Graphics Development; +Cc: Daniel Vetter, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: More cautious with pch fifo underruns 2014-11-24 16:02 [PATCH] drm/i915: More cautious with pch fifo underruns Daniel Vetter @ 2014-11-25 3:53 ` shuang.he 2014-11-25 8:38 ` Daniel Vetter 2014-11-26 15:37 ` Paulo Zanoni 2 siblings, 0 replies; 11+ messages in thread From: shuang.he @ 2014-11-25 3:53 UTC (permalink / raw) To: shuang.he, intel-gfx, daniel.vetter 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: More cautious with pch fifo underruns 2014-11-24 16:02 [PATCH] drm/i915: More cautious with pch fifo underruns Daniel Vetter 2014-11-25 3:53 ` shuang.he @ 2014-11-25 8:38 ` Daniel Vetter 2014-11-26 15:37 ` Paulo Zanoni 2 siblings, 0 replies; 11+ messages in thread From: Daniel Vetter @ 2014-11-25 8:38 UTC (permalink / raw) To: Intel Graphics Development; +Cc: Daniel Vetter, 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: More cautious with pch fifo underruns 2014-11-24 16:02 [PATCH] drm/i915: More cautious with pch fifo underruns Daniel Vetter 2014-11-25 3:53 ` shuang.he 2014-11-25 8:38 ` Daniel Vetter @ 2014-11-26 15:37 ` Paulo Zanoni 2014-11-26 16:49 ` Ville Syrjälä 2014-11-26 18:17 ` Daniel Vetter 2 siblings, 2 replies; 11+ messages in thread From: Paulo Zanoni @ 2014-11-26 15:37 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: More cautious with pch fifo underruns 2014-11-26 15:37 ` Paulo Zanoni @ 2014-11-26 16:49 ` Ville Syrjälä 2014-11-26 18:17 ` Daniel Vetter 1 sibling, 0 replies; 11+ messages in thread From: Ville Syrjälä @ 2014-11-26 16:49 UTC (permalink / raw) To: Paulo Zanoni; +Cc: Daniel Vetter, Intel Graphics Development, 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? 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: More cautious with pch fifo underruns 2014-11-26 15:37 ` Paulo Zanoni 2014-11-26 16:49 ` Ville Syrjälä @ 2014-11-26 18:17 ` Daniel Vetter 2014-12-01 13:41 ` Paulo Zanoni 1 sibling, 1 reply; 11+ messages in thread From: Daniel Vetter @ 2014-11-26 18:17 UTC (permalink / raw) To: Paulo Zanoni; +Cc: Daniel Vetter, Intel Graphics Development, 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: More cautious with pch fifo underruns 2014-11-26 18:17 ` Daniel Vetter @ 2014-12-01 13:41 ` Paulo Zanoni 2014-12-01 16:36 ` Daniel Vetter 0 siblings, 1 reply; 11+ messages in thread From: Paulo Zanoni @ 2014-12-01 13:41 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: More cautious with pch fifo underruns 2014-12-01 13:41 ` Paulo Zanoni @ 2014-12-01 16:36 ` Daniel Vetter 2014-12-01 17:04 ` Paulo Zanoni 0 siblings, 1 reply; 11+ messages in thread From: Daniel Vetter @ 2014-12-01 16:36 UTC (permalink / raw) To: Paulo Zanoni; +Cc: Daniel Vetter, Intel Graphics Development, 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: More cautious with pch fifo underruns 2014-12-01 16:36 ` Daniel Vetter @ 2014-12-01 17:04 ` Paulo Zanoni 2014-12-01 17:27 ` Daniel Vetter 0 siblings, 1 reply; 11+ messages in thread From: Paulo Zanoni @ 2014-12-01 17:04 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: More cautious with pch fifo underruns 2014-12-01 17:04 ` Paulo Zanoni @ 2014-12-01 17:27 ` Daniel Vetter 2014-12-02 13:22 ` Jani Nikula 0 siblings, 1 reply; 11+ messages in thread From: Daniel Vetter @ 2014-12-01 17:27 UTC (permalink / raw) To: Paulo Zanoni; +Cc: Daniel Vetter, Intel Graphics Development, 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: More cautious with pch fifo underruns 2014-12-01 17:27 ` Daniel Vetter @ 2014-12-02 13:22 ` Jani Nikula 0 siblings, 0 replies; 11+ messages in thread From: Jani Nikula @ 2014-12-02 13:22 UTC (permalink / raw) To: Daniel Vetter, Paulo Zanoni Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-12-02 13:22 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-11-24 16:02 [PATCH] drm/i915: More cautious with pch fifo underruns Daniel Vetter 2014-11-25 3:53 ` shuang.he 2014-11-25 8:38 ` Daniel Vetter 2014-11-26 15:37 ` Paulo Zanoni 2014-11-26 16:49 ` Ville Syrjälä 2014-11-26 18:17 ` Daniel Vetter 2014-12-01 13:41 ` Paulo Zanoni 2014-12-01 16:36 ` Daniel Vetter 2014-12-01 17:04 ` Paulo Zanoni 2014-12-01 17:27 ` Daniel Vetter 2014-12-02 13:22 ` Jani Nikula
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.