[Intel-gfx] [PATCH 2/5] drm/i915: Fix up fifo underrun tracking, take N

[Daniel Vetter]

So apparently this is tricky.

We need to consider:
- We start out with all the hw enabling bits disabled, both the
  individual fifo underrun interrupts and the shared display error
  interrupts masked. Otherwise if the bios config is broken we'll blow
  up with a NULL deref in our interrupt handler since the crtc
  structures aren't set up yet at driver load time.
- On gmch we need to mask fifo underruns on the sw side, so always
  need to set that in sanitize_crtc for those platforms.
- On other platforms we try to set the sw tracking so that it reflects
  the real state. But since a few platforms have shared bits we must
  _not_ disable fifo underrun reporting. Otherwise we'll never enable
  the shared error interrupt.

This is the state before out patch, but unfortunately this is not good
enough. But after a suspend resume operation this is broken:
1. We don't enable the hw interrupts since the same code runs on
resume as on driver load.
2. The fifo underrun state adjustments we do in sanitize_crtc doesn't
fire on resume since (except for hilarious firmware) all pipes are off
at that point. But they also don't hurt since the subsequent crtc
enabling due to force_restore will enable fifo underruns.

Which means when we enable fifo underrun reporting we notice that the
per-crtc state is already correct and short-circuit everthing out. And
the interrupt doesn't get enabled.

A similar problem would happen if the bios doesn't light up anything
when the driver loads. Which is exactly what happens when we reload
the driver since our unload functions disables all outputs.

Now we can't just rip out the short-circuit logic and unconditionally
update the fifo underrun reporting interrupt masking: We have some
checks for shared error interrupts to catch issues that happened when
the shared error interrupt was disabled.

The right fix is to push down this logic so that we can always update
the hardware state, but only check for missed fifo underruns on a real
enabled->disabled transition and ignore them when we're already
disabled.

On platforms with shared error interrupt the pipe CRC interrupts are
grouped together with the fifo underrun reporting this fixes pipe CRC
support after suspend and driver reloads.

Testcase: igt/kms_pipe_crc_basic/suspend-*
Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 44 ++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 304f86a3162c..4d44f09eb833 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -335,7 +335,8 @@ void i9xx_check_fifo_underruns(struct drm_device *dev)
 }
 
 static void i9xx_set_fifo_underrun_reporting(struct drm_device *dev,
-					     enum pipe pipe, bool enable)
+					     enum pipe pipe,
+					     bool enable, bool old)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 reg = PIPESTAT(pipe);
@@ -347,7 +348,7 @@ static void i9xx_set_fifo_underrun_reporting(struct drm_device *dev,
 		I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
 		POSTING_READ(reg);
 	} else {
-		if (pipestat & PIPE_FIFO_UNDERRUN_STATUS)
+		if (old && pipestat & PIPE_FIFO_UNDERRUN_STATUS)
 			DRM_ERROR("pipe %c underrun\n", pipe_name(pipe));
 	}
 }
@@ -366,7 +367,8 @@ static void ironlake_set_fifo_underrun_reporting(struct drm_device *dev,
 }
 
 static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
-						  enum pipe pipe, bool enable)
+						  enum pipe pipe,
+						  bool enable, bool old)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	if (enable) {
@@ -379,7 +381,8 @@ static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
 	} else {
 		ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
 
-		if (I915_READ(GEN7_ERR_INT) & ERR_INT_FIFO_UNDERRUN(pipe)) {
+		if (old &&
+		    I915_READ(GEN7_ERR_INT) & ERR_INT_FIFO_UNDERRUN(pipe)) {
 			DRM_ERROR("uncleared fifo underrun on pipe %c\n",
 				  pipe_name(pipe));
 		}
@@ -444,7 +447,7 @@ static void ibx_set_fifo_underrun_reporting(struct drm_device *dev,
 
 static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
 					    enum transcoder pch_transcoder,
-					    bool enable)
+					    bool enable, bool old)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -459,7 +462,8 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
 	} else {
 		ibx_disable_display_interrupt(dev_priv, SDE_ERROR_CPT);
 
-		if (I915_READ(SERR_INT) & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)) {
+		if (old && I915_READ(SERR_INT) &
+		    SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)) {
 			DRM_ERROR("uncleared pch fifo underrun on pch transcoder %c\n",
 				  transcoder_name(pch_transcoder));
 		}
@@ -486,28 +490,23 @@ static bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	bool ret;
+	bool old;
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	ret = !intel_crtc->cpu_fifo_underrun_disabled;
-
-	if (enable == ret)
-		goto done;
-
+	old = !intel_crtc->cpu_fifo_underrun_disabled;
 	intel_crtc->cpu_fifo_underrun_disabled = !enable;
 
 	if (INTEL_INFO(dev)->gen < 5 || IS_VALLEYVIEW(dev))
-		i9xx_set_fifo_underrun_reporting(dev, pipe, enable);
+		i9xx_set_fifo_underrun_reporting(dev, pipe, enable, old);
 	else if (IS_GEN5(dev) || IS_GEN6(dev))
 		ironlake_set_fifo_underrun_reporting(dev, pipe, enable);
 	else if (IS_GEN7(dev))
-		ivybridge_set_fifo_underrun_reporting(dev, pipe, enable);
+		ivybridge_set_fifo_underrun_reporting(dev, pipe, enable, old);
 	else if (IS_GEN8(dev))
 		broadwell_set_fifo_underrun_reporting(dev, pipe, enable);
 
-done:
-	return ret;
+	return old;
 }
 
 bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
@@ -556,7 +555,7 @@ bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
 	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pch_transcoder];
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	unsigned long flags;
-	bool ret;
+	bool old;
 
 	/*
 	 * NOTE: Pre-LPT has a fixed cpu pipe -> pch transcoder mapping, but LPT
@@ -569,21 +568,16 @@ bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 
-	ret = !intel_crtc->pch_fifo_underrun_disabled;
-
-	if (enable == ret)
-		goto done;
-
+	old = !intel_crtc->pch_fifo_underrun_disabled;
 	intel_crtc->pch_fifo_underrun_disabled = !enable;
 
 	if (HAS_PCH_IBX(dev))
 		ibx_set_fifo_underrun_reporting(dev, pch_transcoder, enable);
 	else
-		cpt_set_fifo_underrun_reporting(dev, pch_transcoder, enable);
+		cpt_set_fifo_underrun_reporting(dev, pch_transcoder, enable, old);
 
-done:
 	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
-	return ret;
+	return old;
 }
 
 
-- 
1.8.4.rc3