[PATCH] drm/i915: improve SERR_INT clearing for fifo underrun reporting

From: Daniel Vetter <daniel.vetter@ffwll.ch>
To: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: [PATCH] drm/i915: improve SERR_INT clearing for fifo underrun reporting
Date: Wed, 10 Jul 2013 08:30:23 +0200	[thread overview]
Message-ID: <1373437823-9399-1-git-send-email-daniel.vetter@ffwll.ch> (raw)
In-Reply-To: <CA+gsUGRFrugUeAV16Cb2Cxb5Vpx6sT1KftGmcm7G3rDn0gdGfw@mail.gmail.com>

The current code won't report any fifo underruns on cpt if just one
pipe has fifo underrun reporting disabled. We can't enable the
interrupts, but we can still check the per-transcoder bits and so
report the underrun delayed if:
- We always clear the transcoder's bit (and none of the other bits)
  when enabling.
- We check the transcoder's bit after disabling (to avoid racing with
  the interrupt handler).

v2: I've forgotten to actually remove the old SERR_INT clearing.

v3: Use transcoder_name as suggested by Paulo Zanoni. Paulo also
noticed a logic bug: When an underrun interrupt fires we report it
both in the interrupt handler and when checking for underruns when
disabling it in cpt_set_fifo_underrun_reporting. But that second check
is only required if the interrupt is disabled and we're switching of
underrun reporting (e.g. because we're disabling the crtc). Hence
check for that condition.

At first I wanted to rework the code to pass that bit of information
from the uppper functions down to cpt_set_fifo_underrun_reporting. But
that turned out too messy. Hence the quick&dirty check whether the
south error interrupt source is masked off or not.

v4: Streamline the control flow a bit.

v5: s/pipe/pch transcoder/ in the dmesg output, suggested by Paulo.

v6: Review from Paulo:
- Reorder the was_enabled assignment to only read the register when we
  need it. Also add a comment that we need to do that before updating
  the register.
- s/%i/%c/ fix for the debug output.
- Fix the checkpath complaint in the SERR_INT_TRANS_FIFO_UNDERRUN
  #define.

v7: Hopefully put that elusive SERR hunk back into this patch, spotted
by Paulo.

Cc: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 17 +++++++++++++----
 drivers/gpu/drm/i915/i915_reg.h |  1 +
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 80b88c8..dd9d999 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -217,16 +217,25 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	if (enable) {
+		I915_WRITE(SERR_INT,
+			   SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
+
 		if (!cpt_can_enable_serr_int(dev))
 			return;
 
-		I915_WRITE(SERR_INT, SERR_INT_TRANS_A_FIFO_UNDERRUN |
-				     SERR_INT_TRANS_B_FIFO_UNDERRUN |
-				     SERR_INT_TRANS_C_FIFO_UNDERRUN);
-
 		ibx_enable_display_interrupt(dev_priv, SDE_ERROR_CPT);
 	} else {
+		uint32_t tmp = I915_READ(SERR_INT);
+		bool was_enabled = !(I915_READ(SDEIMR) & SDE_ERROR_CPT);
+
+		/* Change the state _after_ we've read out the current one. */
 		ibx_disable_display_interrupt(dev_priv, SDE_ERROR_CPT);
+
+		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));
+		}
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e9c50fa..7e2684f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3882,6 +3882,7 @@
 #define  SERR_INT_TRANS_C_FIFO_UNDERRUN	(1<<6)
 #define  SERR_INT_TRANS_B_FIFO_UNDERRUN	(1<<3)
 #define  SERR_INT_TRANS_A_FIFO_UNDERRUN	(1<<0)
+#define  SERR_INT_TRANS_FIFO_UNDERRUN(pipe)	(1<<(pipe*3))
 
 /* digital port hotplug */
 #define PCH_PORT_HOTPLUG        0xc4030		/* SHOTPLUG_CTL */
-- 
1.8.1.4
