diff mbox

[1/4] drm/i915: promote FIFO underruns to DRM_ERROR

Message ID 1379620838-1491-2-git-send-email-przanoni@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paulo Zanoni Sept. 19, 2013, 8 p.m. UTC 
From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Linus recently complained about screen corruption when coming out of
DPMS on his Haswell machine, and he also mentioned there were no error
messages on the log. I think I can reproduce this problem, and when it
happens I get a "FIFO underrun" message, but since it's just
DRM_DEBUG_DRIVER it's hard to notice. So promote underruns to error
messages because reports containing "I'm getting a FIFO underrun on
pipe A" are way much better than "I'm getting a screen corruption".

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 44 ++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

Comments

Chris Wilson Sept. 19, 2013, 8:16 p.m. UTC | #1 
On Thu, Sep 19, 2013 at 05:00:35PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Linus recently complained about screen corruption when coming out of
> DPMS on his Haswell machine, and he also mentioned there were no error
> messages on the log. I think I can reproduce this problem, and when it
> happens I get a "FIFO underrun" message, but since it's just
> DRM_DEBUG_DRIVER it's hard to notice. So promote underruns to error
> messages because reports containing "I'm getting a FIFO underrun on
> pipe A" are way much better than "I'm getting a screen corruption".

Sadly FIFO underruns on quite a few platforms are expected and
unfixable. Others they are just expected. We reduced the error level
because they are too noisy, and we were not in a position where the
underrun was actionable. As we do not yet have the underrun feedback in
place, I do not think we are ready for the onslaught of errors.

At minimum, you should only set the error message for the platforms you
intend to fix.
-Chris
Paulo Zanoni Sept. 19, 2013, 8:20 p.m. UTC | #2 
2013/9/19 Chris Wilson <chris@chris-wilson.co.uk>:
> On Thu, Sep 19, 2013 at 05:00:35PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Linus recently complained about screen corruption when coming out of
>> DPMS on his Haswell machine, and he also mentioned there were no error
>> messages on the log. I think I can reproduce this problem, and when it
>> happens I get a "FIFO underrun" message, but since it's just
>> DRM_DEBUG_DRIVER it's hard to notice. So promote underruns to error
>> messages because reports containing "I'm getting a FIFO underrun on
>> pipe A" are way much better than "I'm getting a screen corruption".
>
> Sadly FIFO underruns on quite a few platforms are expected and
> unfixable. Others they are just expected. We reduced the error level
> because they are too noisy, and we were not in a position where the
> underrun was actionable. As we do not yet have the underrun feedback in
> place, I do not think we are ready for the onslaught of errors.
>
> At minimum, you should only set the error message for the platforms you
> intend to fix.

Hmmmm, yeah, I should have thought about this a little bit more. Sorry :(

At least on ILK/SNB/IVB/HSW we have the code to stop flooding dmesg
and the code to ignore the expected underruns, so I guess a patch
touching just these platforms would be ok?

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
Chris Wilson Sept. 19, 2013, 8:27 p.m. UTC | #3 
On Thu, Sep 19, 2013 at 05:20:49PM -0300, Paulo Zanoni wrote:
> 2013/9/19 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Thu, Sep 19, 2013 at 05:00:35PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> Linus recently complained about screen corruption when coming out of
> >> DPMS on his Haswell machine, and he also mentioned there were no error
> >> messages on the log. I think I can reproduce this problem, and when it
> >> happens I get a "FIFO underrun" message, but since it's just
> >> DRM_DEBUG_DRIVER it's hard to notice. So promote underruns to error
> >> messages because reports containing "I'm getting a FIFO underrun on
> >> pipe A" are way much better than "I'm getting a screen corruption".
> >
> > Sadly FIFO underruns on quite a few platforms are expected and
> > unfixable. Others they are just expected. We reduced the error level
> > because they are too noisy, and we were not in a position where the
> > underrun was actionable. As we do not yet have the underrun feedback in
> > place, I do not think we are ready for the onslaught of errors.
> >
> > At minimum, you should only set the error message for the platforms you
> > intend to fix.
> 
> Hmmmm, yeah, I should have thought about this a little bit more. Sorry :(
> 
> At least on ILK/SNB/IVB/HSW we have the code to stop flooding dmesg
> and the code to ignore the expected underruns, so I guess a patch
> touching just these platforms would be ok?

No worries, Mr Underrun.
-Chris
Ville Syrjälä Sept. 20, 2013, 6:32 a.m. UTC | #4 
On Thu, Sep 19, 2013 at 05:20:49PM -0300, Paulo Zanoni wrote:
> 2013/9/19 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Thu, Sep 19, 2013 at 05:00:35PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> Linus recently complained about screen corruption when coming out of
> >> DPMS on his Haswell machine, and he also mentioned there were no error
> >> messages on the log. I think I can reproduce this problem, and when it
> >> happens I get a "FIFO underrun" message, but since it's just
> >> DRM_DEBUG_DRIVER it's hard to notice. So promote underruns to error
> >> messages because reports containing "I'm getting a FIFO underrun on
> >> pipe A" are way much better than "I'm getting a screen corruption".
> >
> > Sadly FIFO underruns on quite a few platforms are expected and
> > unfixable. Others they are just expected. We reduced the error level
> > because they are too noisy, and we were not in a position where the
> > underrun was actionable. As we do not yet have the underrun feedback in
> > place, I do not think we are ready for the onslaught of errors.
> >
> > At minimum, you should only set the error message for the platforms you
> > intend to fix.
> 
> Hmmmm, yeah, I should have thought about this a little bit more. Sorry :(
> 
> At least on ILK/SNB/IVB/HSW we have the code to stop flooding dmesg
> and the code to ignore the expected underruns, so I guess a patch
> touching just these platforms would be ok?

I would limit it to HSW before my ILK/SNB/IVB conversion to HSW
watermark code gets in. The current code apparently produces somewhat
incorrect results since according to Imre it even fixes some kind of
rare hang on ILK.
Paulo Zanoni Sept. 20, 2013, 6:38 p.m. UTC | #5 
2013/9/20 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Thu, Sep 19, 2013 at 05:20:49PM -0300, Paulo Zanoni wrote:
>> 2013/9/19 Chris Wilson <chris@chris-wilson.co.uk>:
>> > On Thu, Sep 19, 2013 at 05:00:35PM -0300, Paulo Zanoni wrote:
>> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >>
>> >> Linus recently complained about screen corruption when coming out of
>> >> DPMS on his Haswell machine, and he also mentioned there were no error
>> >> messages on the log. I think I can reproduce this problem, and when it
>> >> happens I get a "FIFO underrun" message, but since it's just
>> >> DRM_DEBUG_DRIVER it's hard to notice. So promote underruns to error
>> >> messages because reports containing "I'm getting a FIFO underrun on
>> >> pipe A" are way much better than "I'm getting a screen corruption".
>> >
>> > Sadly FIFO underruns on quite a few platforms are expected and
>> > unfixable. Others they are just expected. We reduced the error level
>> > because they are too noisy, and we were not in a position where the
>> > underrun was actionable. As we do not yet have the underrun feedback in
>> > place, I do not think we are ready for the onslaught of errors.
>> >
>> > At minimum, you should only set the error message for the platforms you
>> > intend to fix.
>>
>> Hmmmm, yeah, I should have thought about this a little bit more. Sorry :(
>>
>> At least on ILK/SNB/IVB/HSW we have the code to stop flooding dmesg
>> and the code to ignore the expected underruns, so I guess a patch
>> touching just these platforms would be ok?
>
> I would limit it to HSW before my ILK/SNB/IVB conversion to HSW
> watermark code gets in. The current code apparently produces somewhat
> incorrect results since according to Imre it even fixes some kind of
> rare hang on ILK.

Oh, adding "if IS_HASWELL" checks to that code will make it so much uglier :(

I guess I'll wait your ILK/SNB/IVB watermark fixes first :)

>
> --
> Ville Syrjälä
> Intel OTC
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a42f30b..9ecc9d9 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -263,8 +263,8 @@  static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
 
 		if (!was_enabled &&
 		    (I915_READ(GEN7_ERR_INT) & ERR_INT_FIFO_UNDERRUN(pipe))) {
-			DRM_DEBUG_KMS("uncleared fifo underrun on pipe %c\n",
-				      pipe_name(pipe));
+			DRM_ERROR("uncleared fifo underrun on pipe %c\n",
+				  pipe_name(pipe));
 		}
 	}
 }
@@ -339,8 +339,8 @@  static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
 
 		if (!was_enabled &&
 		    (tmp & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder))) {
-			DRM_DEBUG_KMS("uncleared pch fifo underrun on pch transcoder %c\n",
-				      transcoder_name(pch_transcoder));
+			DRM_ERROR("uncleared pch fifo underrun on pch transcoder %c\n",
+				  transcoder_name(pch_transcoder));
 		}
 	}
 }
@@ -1115,8 +1115,8 @@  static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 			 */
 			if (pipe_stats[pipe] & 0x8000ffff) {
 				if (pipe_stats[pipe] & PIPE_FIFO_UNDERRUN_STATUS)
-					DRM_DEBUG_DRIVER("pipe %c underrun\n",
-							 pipe_name(pipe));
+					DRM_ERROR("pipe %c underrun\n",
+						  pipe_name(pipe));
 				I915_WRITE(reg, pipe_stats[pipe]);
 			}
 		}
@@ -1206,12 +1206,12 @@  static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
 	if (pch_iir & SDE_TRANSA_FIFO_UNDER)
 		if (intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A,
 							  false))
-			DRM_DEBUG_DRIVER("PCH transcoder A FIFO underrun\n");
+			DRM_ERROR("PCH transcoder A FIFO underrun\n");
 
 	if (pch_iir & SDE_TRANSB_FIFO_UNDER)
 		if (intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_B,
 							  false))
-			DRM_DEBUG_DRIVER("PCH transcoder B FIFO underrun\n");
+			DRM_ERROR("PCH transcoder B FIFO underrun\n");
 }
 
 static void ivb_err_int_handler(struct drm_device *dev)
@@ -1224,15 +1224,15 @@  static void ivb_err_int_handler(struct drm_device *dev)
 
 	if (err_int & ERR_INT_FIFO_UNDERRUN_A)
 		if (intel_set_cpu_fifo_underrun_reporting(dev, PIPE_A, false))
-			DRM_DEBUG_DRIVER("Pipe A FIFO underrun\n");
+			DRM_ERROR("Pipe A FIFO underrun\n");
 
 	if (err_int & ERR_INT_FIFO_UNDERRUN_B)
 		if (intel_set_cpu_fifo_underrun_reporting(dev, PIPE_B, false))
-			DRM_DEBUG_DRIVER("Pipe B FIFO underrun\n");
+			DRM_ERROR("Pipe B FIFO underrun\n");
 
 	if (err_int & ERR_INT_FIFO_UNDERRUN_C)
 		if (intel_set_cpu_fifo_underrun_reporting(dev, PIPE_C, false))
-			DRM_DEBUG_DRIVER("Pipe C FIFO underrun\n");
+			DRM_ERROR("Pipe C FIFO underrun\n");
 
 	I915_WRITE(GEN7_ERR_INT, err_int);
 }
@@ -1248,17 +1248,17 @@  static void cpt_serr_int_handler(struct drm_device *dev)
 	if (serr_int & SERR_INT_TRANS_A_FIFO_UNDERRUN)
 		if (intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A,
 							  false))
-			DRM_DEBUG_DRIVER("PCH transcoder A FIFO underrun\n");
+			DRM_ERROR("PCH transcoder A FIFO underrun\n");
 
 	if (serr_int & SERR_INT_TRANS_B_FIFO_UNDERRUN)
 		if (intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_B,
 							  false))
-			DRM_DEBUG_DRIVER("PCH transcoder B FIFO underrun\n");
+			DRM_ERROR("PCH transcoder B FIFO underrun\n");
 
 	if (serr_int & SERR_INT_TRANS_C_FIFO_UNDERRUN)
 		if (intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_C,
 							  false))
-			DRM_DEBUG_DRIVER("PCH transcoder C FIFO underrun\n");
+			DRM_ERROR("PCH transcoder C FIFO underrun\n");
 
 	I915_WRITE(SERR_INT, serr_int);
 }
@@ -1321,11 +1321,11 @@  static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
 
 	if (de_iir & DE_PIPEA_FIFO_UNDERRUN)
 		if (intel_set_cpu_fifo_underrun_reporting(dev, PIPE_A, false))
-			DRM_DEBUG_DRIVER("Pipe A FIFO underrun\n");
+			DRM_ERROR("Pipe A FIFO underrun\n");
 
 	if (de_iir & DE_PIPEB_FIFO_UNDERRUN)
 		if (intel_set_cpu_fifo_underrun_reporting(dev, PIPE_B, false))
-			DRM_DEBUG_DRIVER("Pipe B FIFO underrun\n");
+			DRM_ERROR("Pipe B FIFO underrun\n");
 
 	if (de_iir & DE_PLANEA_FLIP_DONE) {
 		intel_prepare_page_flip(dev, 0);
@@ -2566,8 +2566,8 @@  static irqreturn_t i8xx_irq_handler(int irq, void *arg)
 			 */
 			if (pipe_stats[pipe] & 0x8000ffff) {
 				if (pipe_stats[pipe] & PIPE_FIFO_UNDERRUN_STATUS)
-					DRM_DEBUG_DRIVER("pipe %c underrun\n",
-							 pipe_name(pipe));
+					DRM_ERROR("pipe %c underrun\n",
+						  pipe_name(pipe));
 				I915_WRITE(reg, pipe_stats[pipe]);
 			}
 		}
@@ -2737,8 +2737,8 @@  static irqreturn_t i915_irq_handler(int irq, void *arg)
 			/* Clear the PIPE*STAT regs before the IIR */
 			if (pipe_stats[pipe] & 0x8000ffff) {
 				if (pipe_stats[pipe] & PIPE_FIFO_UNDERRUN_STATUS)
-					DRM_DEBUG_DRIVER("pipe %c underrun\n",
-							 pipe_name(pipe));
+					DRM_ERROR("pipe %c underrun\n",
+						  pipe_name(pipe));
 				I915_WRITE(reg, pipe_stats[pipe]);
 				irq_received = true;
 			}
@@ -2979,8 +2979,8 @@  static irqreturn_t i965_irq_handler(int irq, void *arg)
 			 */
 			if (pipe_stats[pipe] & 0x8000ffff) {
 				if (pipe_stats[pipe] & PIPE_FIFO_UNDERRUN_STATUS)
-					DRM_DEBUG_DRIVER("pipe %c underrun\n",
-							 pipe_name(pipe));
+					DRM_ERROR("pipe %c underrun\n",
+						  pipe_name(pipe));
 				I915_WRITE(reg, pipe_stats[pipe]);
 				irq_received = 1;
 			}