[PATCH 0/7] Unclaimed register reporting V2

[PATCH 0/7] Unclaimed register reporting V2

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Hi

This is new version of 2 patches I sent last week. First we change how unclaimed
registers are currently reported by our driver and in the end we enable gen 7
error interrupts.

Here's what we do:
  - Make the current code a little bit more organized
  - Use FPGA_DBG instead of GEN7_ERR_INT for the polling code
  - Initialize FPGA_DBG to clear one "unclaimed register" message
  - Check unclaimed registers on I915_READ
  - Replace the DRM_ERROR with WARNs
  - Only print uncnlaimed registers if drm_debug is set
  - Finally, print Gen 7 error interrupts

We had some discussion regarding these patches, and there is still no consensus
on what exactly we should do, so feel free to nack patches. Still, the patches
here are to some extent based on the comments I received (mainly from Ben,
Daniel and Chris), so I hope they're not too bad. Also, I really think that at
least the first 3 patches should be merged, and also some equivalent form of the
last patch should be merged.

Another thing I have to add: after patch 7 you may see a few dozen "unclaimed
register" messages when booting if you have the power well disabled. All these
messages go away if you blacklist snd_hda_intel. Still, I like to have messages
reminding me that something is wrong in some driver. And you'll only see those
messages if you have drm_debug, because of patch 0006.

Thanks,
Paulo

Paulo Zanoni (7):
  drm/i915: create macros for the "unclaimed register" checks
  drm/i915: use FPGA_DBG for the "unclaimed register" checks
  drm/i915: clear the FPGA_DBG_RM_NOCLAIM bit at driver init
  drm/i915: check for unclaimed registers on I915_READ too
  drm/i915: WARN on unclaimed registers
  drm/i915: only check for unclaimed registers if drm_debug
  drm/i915: print Gen 7 error interrupts

 drivers/gpu/drm/i915/i915_dma.c |    4 ++++
 drivers/gpu/drm/i915/i915_drv.c |   26 ++++++++++++++++++--------
 drivers/gpu/drm/i915/i915_irq.c |   28 +++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_reg.h |    5 ++++-
 4 files changed, 53 insertions(+), 10 deletions(-)

-- 
1.7.10.4

[PATCH 1/7] drm/i915: create macros for the "unclaimed register" checks

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

This avoids polluting i915_write##x and also allows us to reuse code
on i915_read##x.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 9cc8f87..ae0a55a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1224,6 +1224,20 @@ ilk_dummy_write(struct drm_i915_private *dev_priv)
 	I915_WRITE_NOTRACE(MI_MODE, 0);
 }
 
+#define UNCLAIMED_REG_CLEAR(dev_priv, reg) \
+	if (IS_HASWELL(dev_priv->dev) && \
+	    (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
+		DRM_ERROR("Unknown unclaimed register before writing to %x\n", reg); \
+		I915_WRITE_NOTRACE(GEN7_ERR_INT, ERR_INT_MMIO_UNCLAIMED); \
+	}
+
+#define UNCLAIMED_REG_CHECK(dev_priv, reg) \
+	if (IS_HASWELL(dev_priv->dev) && \
+	    (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
+		DRM_ERROR("Unclaimed write to %x\n", reg); \
+		writel(ERR_INT_MMIO_UNCLAIMED, dev_priv->regs + GEN7_ERR_INT);	\
+	}
+
 #define __i915_read(x, y) \
 u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg) { \
 	u##x val = 0; \
@@ -1262,10 +1276,7 @@ void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \
 	} \
 	if (IS_GEN5(dev_priv->dev)) \
 		ilk_dummy_write(dev_priv); \
-	if (IS_HASWELL(dev_priv->dev) && (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
-		DRM_ERROR("Unknown unclaimed register before writing to %x\n", reg); \
-		I915_WRITE_NOTRACE(GEN7_ERR_INT, ERR_INT_MMIO_UNCLAIMED); \
-	} \
+	UNCLAIMED_REG_CLEAR(dev_priv, reg); \
 	if (IS_VALLEYVIEW(dev_priv->dev) && IS_DISPLAYREG(reg)) { \
 		write##y(val, dev_priv->regs + reg + 0x180000);		\
 	} else {							\
@@ -1274,10 +1285,7 @@ void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \
 	if (unlikely(__fifo_ret)) { \
 		gen6_gt_check_fifodbg(dev_priv); \
 	} \
-	if (IS_HASWELL(dev_priv->dev) && (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
-		DRM_ERROR("Unclaimed write to %x\n", reg); \
-		writel(ERR_INT_MMIO_UNCLAIMED, dev_priv->regs + GEN7_ERR_INT);	\
-	} \
+	UNCLAIMED_REG_CHECK(dev_priv, reg); \
 }
 __i915_write(8, b)
 __i915_write(16, w)
-- 
1.7.10.4

[PATCH 2/7] drm/i915: use FPGA_DBG for the "unclaimed register" checks

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We plan to treat GEN7_ERR_INT as an interrupt, so use this register
for the checks inside I915_WRITE. This way we can have the best of
both worlds: the error message with a register address and the
interrupt.

V2: Split in 2 patches: one for the macro, one for changing the
register, as requested by Ben.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c |    8 ++++----
 drivers/gpu/drm/i915/i915_reg.h |    3 +++
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ae0a55a..f47de4d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1226,16 +1226,16 @@ ilk_dummy_write(struct drm_i915_private *dev_priv)
 
 #define UNCLAIMED_REG_CLEAR(dev_priv, reg) \
 	if (IS_HASWELL(dev_priv->dev) && \
-	    (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
+	    (I915_READ_NOTRACE(FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) { \
 		DRM_ERROR("Unknown unclaimed register before writing to %x\n", reg); \
-		I915_WRITE_NOTRACE(GEN7_ERR_INT, ERR_INT_MMIO_UNCLAIMED); \
+		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM); \
 	}
 
 #define UNCLAIMED_REG_CHECK(dev_priv, reg) \
 	if (IS_HASWELL(dev_priv->dev) && \
-	    (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
+	    (I915_READ_NOTRACE(FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) { \
 		DRM_ERROR("Unclaimed write to %x\n", reg); \
-		writel(ERR_INT_MMIO_UNCLAIMED, dev_priv->regs + GEN7_ERR_INT);	\
+		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM); \
 	}
 
 #define __i915_read(x, y) \
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 15c15a5..ee30fb9 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -515,6 +515,9 @@
 #define GEN7_ERR_INT	0x44040
 #define   ERR_INT_MMIO_UNCLAIMED (1<<13)
 
+#define FPGA_DBG		0x42300
+#define FPGA_DBG_RM_NOCLAIM	(1<<31)
+
 /* GM45+ chicken bits -- debug workaround bits that may be required
  * for various sorts of correct behavior.  The top 16 bits of each are
  * the enables for writing to the corresponding low bit.
-- 
1.7.10.4

[PATCH 3/7] drm/i915: clear the FPGA_DBG_RM_NOCLAIM bit at driver init

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Otherwise, if the BIOS did anything wrong, our first I915_{READ,WRITE}
will give us "unclaimed regsiter" messages.

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

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 11c7aa8..505d7eb 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1578,6 +1578,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	/* This must be called before any calls to HAS_PCH_* */
 	intel_detect_pch(dev);
 
+	/* This must happen before any I915_{READ,WRITE}: */
+	if (IS_HASWELL(dev))
+		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
+
 	intel_irq_init(dev);
 	intel_gt_init(dev);
 
-- 
1.7.10.4

[PATCH 4/7] drm/i915: check for unclaimed registers on I915_READ too

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

This will transform a few "Unclaimed register before XXX" into the
more precise version "Unclaimed register YYY (R)", where YYY is the
droid we're looking for, not XXX.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f47de4d..422dfc6 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1224,17 +1224,17 @@ ilk_dummy_write(struct drm_i915_private *dev_priv)
 	I915_WRITE_NOTRACE(MI_MODE, 0);
 }
 
-#define UNCLAIMED_REG_CLEAR(dev_priv, reg) \
+#define UNCLAIMED_REG_CLEAR(dev_priv, reg, op) \
 	if (IS_HASWELL(dev_priv->dev) && \
 	    (I915_READ_NOTRACE(FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) { \
-		DRM_ERROR("Unknown unclaimed register before writing to %x\n", reg); \
+		DRM_ERROR("Unclaimed register before %x (%c)\n", reg, op); \
 		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM); \
 	}
 
-#define UNCLAIMED_REG_CHECK(dev_priv, reg) \
+#define UNCLAIMED_REG_CHECK(dev_priv, reg, op) \
 	if (IS_HASWELL(dev_priv->dev) && \
 	    (I915_READ_NOTRACE(FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) { \
-		DRM_ERROR("Unclaimed write to %x\n", reg); \
+		DRM_ERROR("Unclaimed register %x (%c)\n", reg, op); \
 		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM); \
 	}
 
@@ -1243,6 +1243,7 @@ u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg) { \
 	u##x val = 0; \
 	if (IS_GEN5(dev_priv->dev)) \
 		ilk_dummy_write(dev_priv); \
+	UNCLAIMED_REG_CLEAR(dev_priv, reg, 'R'); \
 	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
 		unsigned long irqflags; \
 		spin_lock_irqsave(&dev_priv->gt_lock, irqflags); \
@@ -1257,6 +1258,7 @@ u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg) { \
 	} else { \
 		val = read##y(dev_priv->regs + reg); \
 	} \
+	UNCLAIMED_REG_CHECK(dev_priv, reg, 'R'); \
 	trace_i915_reg_rw(false, reg, val, sizeof(val)); \
 	return val; \
 }
@@ -1276,7 +1278,7 @@ void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \
 	} \
 	if (IS_GEN5(dev_priv->dev)) \
 		ilk_dummy_write(dev_priv); \
-	UNCLAIMED_REG_CLEAR(dev_priv, reg); \
+	UNCLAIMED_REG_CLEAR(dev_priv, reg, 'W'); \
 	if (IS_VALLEYVIEW(dev_priv->dev) && IS_DISPLAYREG(reg)) { \
 		write##y(val, dev_priv->regs + reg + 0x180000);		\
 	} else {							\
@@ -1285,7 +1287,7 @@ void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \
 	if (unlikely(__fifo_ret)) { \
 		gen6_gt_check_fifodbg(dev_priv); \
 	} \
-	UNCLAIMED_REG_CHECK(dev_priv, reg); \
+	UNCLAIMED_REG_CHECK(dev_priv, reg, 'W'); \
 }
 __i915_write(8, b)
 __i915_write(16, w)
-- 
1.7.10.4

[PATCH 5/7] drm/i915: WARN on unclaimed registers

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

While debugging these "unclaimed register" problems I concluded that
having a backtrace is way much more useful than having the register
address, since in a lot of cases the register address print on the
message is not the register we're looking for.

We must fix all the "unclaimed register" problems, so if dmesg gets
too polluted it means we're too bugged.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 422dfc6..bc0eb88 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1227,14 +1227,14 @@ ilk_dummy_write(struct drm_i915_private *dev_priv)
 #define UNCLAIMED_REG_CLEAR(dev_priv, reg, op) \
 	if (IS_HASWELL(dev_priv->dev) && \
 	    (I915_READ_NOTRACE(FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) { \
-		DRM_ERROR("Unclaimed register before %x (%c)\n", reg, op); \
+		WARN(1, "Unclaimed register before %x (%c)\n", reg, op); \
 		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM); \
 	}
 
 #define UNCLAIMED_REG_CHECK(dev_priv, reg, op) \
 	if (IS_HASWELL(dev_priv->dev) && \
 	    (I915_READ_NOTRACE(FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) { \
-		DRM_ERROR("Unclaimed register %x (%c)\n", reg, op); \
+		WARN(1, "Unclaimed register %x (%c)\n", reg, op); \
 		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM); \
 	}
 
-- 
1.7.10.4

[PATCH 6/7] drm/i915: only check for unclaimed registers if drm_debug

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

This is a debug feature that may pollute dmesg and scare users, so
leave it for the debug mode only. We usually ask for bug reporters to
use the debug flags, so we will still have the opportunity to catch a
lot of errors on users' machines.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index bc0eb88..a401ec8 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1225,14 +1225,14 @@ ilk_dummy_write(struct drm_i915_private *dev_priv)
 }
 
 #define UNCLAIMED_REG_CLEAR(dev_priv, reg, op) \
-	if (IS_HASWELL(dev_priv->dev) && \
+	if (drm_debug && IS_HASWELL(dev_priv->dev) && \
 	    (I915_READ_NOTRACE(FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) { \
 		WARN(1, "Unclaimed register before %x (%c)\n", reg, op); \
 		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM); \
 	}
 
 #define UNCLAIMED_REG_CHECK(dev_priv, reg, op) \
-	if (IS_HASWELL(dev_priv->dev) && \
+	if (drm_debug && IS_HASWELL(dev_priv->dev) && \
 	    (I915_READ_NOTRACE(FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) { \
 		WARN(1, "Unclaimed register %x (%c)\n", reg, op); \
 		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM); \
-- 
1.7.10.4

[PATCH 7/7] drm/i915: print Gen 7 error interrupts

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

If we get one of these messages it means we did something wrong, and
the first step to fix wrong things is to detect them and recognize
they exist.

For now, leave most messages as DRM_DEBUG_KMS. After we conclude that
a certain message should not be print since our code is correct, then
we can promote that specific message to DRM_ERROR.

Also notice that on Haswell the only interrupt I ever get is for
"unclaimed registers", so it looks like at least on Haswell we're in a
good shape.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c |   28 +++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_reg.h |    2 +-
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 943db10..c2136cd 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -697,6 +697,17 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
 					 I915_READ(FDI_RX_IIR(pipe)));
 }
 
+static void ivb_err_int_handler(struct drm_device *dev)
+{
+	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
+	u32 err_int = I915_READ(GEN7_ERR_INT);
+
+	if (err_int)
+		DRM_DEBUG_KMS("Error interrupt: 0x%08x\n", err_int);
+
+	I915_WRITE(GEN7_ERR_INT, err_int);
+}
+
 static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
 {
 	struct drm_device *dev = (struct drm_device *) arg;
@@ -707,6 +718,14 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
 
 	atomic_inc(&dev_priv->irq_received);
 
+	/* We get interrupts on unclaimed registers, so check this before we do
+	 * any I915_{READ,WRITE}. */
+	if (drm_debug && IS_HASWELL(dev) &&
+	    (I915_READ_NOTRACE(FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) {
+		DRM_ERROR("Unclaimed register before interrupt\n");
+		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
+	}
+
 	/* disable master interrupt before clearing iir  */
 	de_ier = I915_READ(DEIER);
 	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
@@ -720,6 +739,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
 
 	de_iir = I915_READ(DEIIR);
 	if (de_iir) {
+		if (de_iir & DE_ERR_INT_IVB)
+			ivb_err_int_handler(dev);
+
 		if (de_iir & DE_AUX_CHANNEL_A_IVB)
 			dp_aux_irq_handler(dev);
 
@@ -2006,7 +2028,8 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 		DE_PLANEC_FLIP_DONE_IVB |
 		DE_PLANEB_FLIP_DONE_IVB |
 		DE_PLANEA_FLIP_DONE_IVB |
-		DE_AUX_CHANNEL_A_IVB;
+		DE_AUX_CHANNEL_A_IVB |
+		DE_ERR_INT_IVB;
 	u32 render_irqs;
 	u32 hotplug_mask;
 	u32 pch_irq_mask;
@@ -2014,6 +2037,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 	dev_priv->irq_mask = ~display_mask;
 
 	/* should always can generate irq */
+	I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
 	I915_WRITE(DEIIR, I915_READ(DEIIR));
 	I915_WRITE(DEIMR, dev_priv->irq_mask);
 	I915_WRITE(DEIER,
@@ -2177,6 +2201,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 	I915_WRITE(DEIMR, 0xffffffff);
 	I915_WRITE(DEIER, 0x0);
 	I915_WRITE(DEIIR, I915_READ(DEIIR));
+	if (IS_GEN7(dev))
+		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
 
 	I915_WRITE(GTIMR, 0xffffffff);
 	I915_WRITE(GTIER, 0x0);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index ee30fb9..86cace1 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3379,7 +3379,7 @@
 #define DE_PIPEA_FIFO_UNDERRUN  (1 << 0)
 
 /* More Ivybridge lolz */
-#define DE_ERR_DEBUG_IVB		(1<<30)
+#define DE_ERR_INT_IVB			(1<<30)
 #define DE_GSE_IVB			(1<<29)
 #define DE_PCH_EVENT_IVB		(1<<28)
 #define DE_DP_A_HOTPLUG_IVB		(1<<27)
-- 
1.7.10.4

Re: [PATCH 1/7] drm/i915: create macros for the "unclaimed register" checks

[Ben Widawsky]

On Fri, 25 Jan 2013 18:57:36 -0200
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This avoids polluting i915_write##x and also allows us to reuse code
> on i915_read##x.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c |   24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 9cc8f87..ae0a55a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1224,6 +1224,20 @@ ilk_dummy_write(struct drm_i915_private *dev_priv)
>  	I915_WRITE_NOTRACE(MI_MODE, 0);
>  }
>  
> +#define UNCLAIMED_REG_CLEAR(dev_priv, reg) \
> +	if (IS_HASWELL(dev_priv->dev) && \
> +	    (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
> +		DRM_ERROR("Unknown unclaimed register before writing to %x\n", reg); \
> +		I915_WRITE_NOTRACE(GEN7_ERR_INT, ERR_INT_MMIO_UNCLAIMED); \
> +	}
> +

You don't really need the if here, do you? I think the way it worked
originally was fine.

> +#define UNCLAIMED_REG_CHECK(dev_priv, reg) \
> +	if (IS_HASWELL(dev_priv->dev) && \
> +	    (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
> +		DRM_ERROR("Unclaimed write to %x\n", reg); \
> +		writel(ERR_INT_MMIO_UNCLAIMED, dev_priv->regs + GEN7_ERR_INT);	\
> +	}
> +
>  #define __i915_read(x, y) \
>  u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg) { \
>  	u##x val = 0; \

I suppose you could add some 'unlikely' predictions in another patch if
you wanted. I really don't know what the consensus is on using those
these day.

> @@ -1262,10 +1276,7 @@ void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \
>  	} \
>  	if (IS_GEN5(dev_priv->dev)) \
>  		ilk_dummy_write(dev_priv); \
> -	if (IS_HASWELL(dev_priv->dev) && (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
> -		DRM_ERROR("Unknown unclaimed register before writing to %x\n", reg); \
> -		I915_WRITE_NOTRACE(GEN7_ERR_INT, ERR_INT_MMIO_UNCLAIMED); \
> -	} \
> +	UNCLAIMED_REG_CLEAR(dev_priv, reg); \
>  	if (IS_VALLEYVIEW(dev_priv->dev) && IS_DISPLAYREG(reg)) { \
>  		write##y(val, dev_priv->regs + reg + 0x180000);		\
>  	} else {			\

Here's what I meant above, you replace 2 lines of code with 4 from the
macro. (Including an extra MMIO read).

> @@ -1274,10 +1285,7 @@ void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \
>  	if (unlikely(__fifo_ret)) { \
>  		gen6_gt_check_fifodbg(dev_priv); \
>  	} \
> -	if (IS_HASWELL(dev_priv->dev) && (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
> -		DRM_ERROR("Unclaimed write to %x\n", reg); \
> -		writel(ERR_INT_MMIO_UNCLAIMED, dev_priv->regs + GEN7_ERR_INT);	\
> -	} \
> +	UNCLAIMED_REG_CHECK(dev_priv, reg); \
>  }
>  __i915_write(8, b)
>  __i915_write(16, w)



-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 2/7] drm/i915: use FPGA_DBG for the "unclaimed register" checks

[Ben Widawsky]

On Fri, 25 Jan 2013 18:57:37 -0200
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We plan to treat GEN7_ERR_INT as an interrupt, so use this register
> for the checks inside I915_WRITE. This way we can have the best of
> both worlds: the error message with a register address and the
> interrupt.
> 
> V2: Split in 2 patches: one for the macro, one for changing the
> register, as requested by Ben.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Assuming we can come to a consensus on patch 1, it's
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 3/7] drm/i915: clear the FPGA_DBG_RM_NOCLAIM bit at driver init

[Ben Widawsky]

On Fri, 25 Jan 2013 18:57:38 -0200
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Otherwise, if the BIOS did anything wrong, our first I915_{READ,WRITE}
> will give us "unclaimed regsiter" messages.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 11c7aa8..505d7eb 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1578,6 +1578,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	/* This must be called before any calls to HAS_PCH_* */
>  	intel_detect_pch(dev);
>  
> +	/* This must happen before any I915_{READ,WRITE}: */
> +	if (IS_HASWELL(dev))
> +		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
> +
>  	intel_irq_init(dev);
>  	intel_gt_init(dev);
>  

This is too late, I think. It should happen as soon as we map the MMIO.
Just in case stupid people (like me) add in stuff at driver load. You
say it in the comment, but it would be clearer in code if you did it
immediately after:

dev_priv->regs = pci_iomap(dev->pdev, mmio_bar, mmio_size);

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 5/7] drm/i915: WARN on unclaimed registers

[Ben Widawsky]

On Fri, 25 Jan 2013 18:57:40 -0200
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> While debugging these "unclaimed register" problems I concluded that
> having a backtrace is way much more useful than having the register
> address, since in a lot of cases the register address print on the
> message is not the register we're looking for.
> 
> We must fix all the "unclaimed register" problems, so if dmesg gets
> too polluted it means we're too bugged.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

As I mentioned internally, I'd still prefer
if (WARN_ON(I915_READ_NOTRACE(FPGA_DBG) & FPGA_DBG_RM_NOCLAIM))

because I don't think the message is particularly useful with a
backtrace in hand.

Also on second thought since the internal review, it's probably not
super useful to have the WARN in the UNCLAIMED_REG_CLEAR, because as
you've mentioned before, it's usually from snd-hda or something like
that. So I'd probably get rid of that unless someone else sees a super
cool usage for it.


With or without painting my color:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> ---
>  drivers/gpu/drm/i915/i915_drv.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 422dfc6..bc0eb88 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1227,14 +1227,14 @@ ilk_dummy_write(struct drm_i915_private *dev_priv)
>  #define UNCLAIMED_REG_CLEAR(dev_priv, reg, op) \
>  	if (IS_HASWELL(dev_priv->dev) && \
>  	    (I915_READ_NOTRACE(FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) { \
> -		DRM_ERROR("Unclaimed register before %x (%c)\n", reg, op); \
> +		WARN(1, "Unclaimed register before %x (%c)\n", reg, op); \
>  		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM); \
>  	}
>  
>  #define UNCLAIMED_REG_CHECK(dev_priv, reg, op) \
>  	if (IS_HASWELL(dev_priv->dev) && \
>  	    (I915_READ_NOTRACE(FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) { \
> -		DRM_ERROR("Unclaimed register %x (%c)\n", reg, op); \
> +		WARN(1, "Unclaimed register %x (%c)\n", reg, op); \
>  		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM); \
>  	}
>  



-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 7/7] drm/i915: print Gen 7 error interrupts

[Ben Widawsky]

On Fri, 25 Jan 2013 18:57:42 -0200
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> If we get one of these messages it means we did something wrong, and
> the first step to fix wrong things is to detect them and recognize
> they exist.
> 
> For now, leave most messages as DRM_DEBUG_KMS. After we conclude that
> a certain message should not be print since our code is correct, then
> we can promote that specific message to DRM_ERROR.
> 
> Also notice that on Haswell the only interrupt I ever get is for
> "unclaimed registers", so it looks like at least on Haswell we're in a
> good shape.
Please take some of my cynicism, you are way too optimistic :p

I have a lot of concerns with this patch touch FPGA_DEBUG and races.

Third, I think to be super paranoid you might want to disable the ERR_INT while
handling interrupts.

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c |   28 +++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_reg.h |    2 +-
>  2 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 943db10..c2136cd 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -697,6 +697,17 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>  					 I915_READ(FDI_RX_IIR(pipe)));
>  }
>  
> +static void ivb_err_int_handler(struct drm_device *dev)
> +{
> +	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> +	u32 err_int = I915_READ(GEN7_ERR_INT);
> +
> +	if (err_int)
> +		DRM_DEBUG_KMS("Error interrupt: 0x%08x\n", err_int);
> +
> +	I915_WRITE(GEN7_ERR_INT, err_int);
> +}
> +
>  static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>  {
>  	struct drm_device *dev = (struct drm_device *) arg;
> @@ -707,6 +718,14 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>  
>  	atomic_inc(&dev_priv->irq_received);
>  
> +	/* We get interrupts on unclaimed registers, so check this before we do
> +	 * any I915_{READ,WRITE}. */
> +	if (drm_debug && IS_HASWELL(dev) &&
> +	    (I915_READ_NOTRACE(FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) {
> +		DRM_ERROR("Unclaimed register before interrupt\n");
> +		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
> +	}
> +
>  	/* disable master interrupt before clearing iir  */
>  	de_ier = I915_READ(DEIER);
>  	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);

I don't really understand what you're trying to do with this, and I
think this is quite racy since you're usually protecting FPGA_DBG with
struct_mutex, but not here. Since it's mostly for debug, maybe you can
convince me why it should be here, and I'll drop my complaint.

> @@ -720,6 +739,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>  
>  	de_iir = I915_READ(DEIIR);
>  	if (de_iir) {
> +		if (de_iir & DE_ERR_INT_IVB)
> +			ivb_err_int_handler(dev);
> +
>  		if (de_iir & DE_AUX_CHANNEL_A_IVB)
>  			dp_aux_irq_handler(dev);
>  
> @@ -2006,7 +2028,8 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>  		DE_PLANEC_FLIP_DONE_IVB |
>  		DE_PLANEB_FLIP_DONE_IVB |
>  		DE_PLANEA_FLIP_DONE_IVB |
> -		DE_AUX_CHANNEL_A_IVB;
> +		DE_AUX_CHANNEL_A_IVB |
> +		DE_ERR_INT_IVB;
>  	u32 render_irqs;
>  	u32 hotplug_mask;
>  	u32 pch_irq_mask;
> @@ -2014,6 +2037,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>  	dev_priv->irq_mask = ~display_mask;
>  
>  	/* should always can generate irq */
> +	I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>  	I915_WRITE(DEIIR, I915_READ(DEIIR));
>  	I915_WRITE(DEIMR, dev_priv->irq_mask);
>  	I915_WRITE(DEIER,
> @@ -2177,6 +2201,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>  	I915_WRITE(DEIMR, 0xffffffff);
>  	I915_WRITE(DEIER, 0x0);
>  	I915_WRITE(DEIIR, I915_READ(DEIIR));
> +	if (IS_GEN7(dev))
> +		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>  
>  	I915_WRITE(GTIMR, 0xffffffff);
>  	I915_WRITE(GTIER, 0x0);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index ee30fb9..86cace1 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3379,7 +3379,7 @@
>  #define DE_PIPEA_FIFO_UNDERRUN  (1 << 0)
>  
>  /* More Ivybridge lolz */
> -#define DE_ERR_DEBUG_IVB		(1<<30)
> +#define DE_ERR_INT_IVB			(1<<30)
>  #define DE_GSE_IVB			(1<<29)
>  #define DE_PCH_EVENT_IVB		(1<<28)
>  #define DE_DP_A_HOTPLUG_IVB		(1<<27)



-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 1/7] drm/i915: create macros for the "unclaimed register" checks

[Ben Widawsky]

On Fri, Jan 25, 2013 at 05:02:01PM -0800, Ben Widawsky wrote:
> On Fri, 25 Jan 2013 18:57:36 -0200
> Paulo Zanoni <przanoni@gmail.com> wrote:
> 
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > This avoids polluting i915_write##x and also allows us to reuse code
> > on i915_read##x.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> > ---
> >  drivers/gpu/drm/i915/i915_drv.c |   24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 9cc8f87..ae0a55a 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1224,6 +1224,20 @@ ilk_dummy_write(struct drm_i915_private *dev_priv)
> >  	I915_WRITE_NOTRACE(MI_MODE, 0);
> >  }
> >  
> > +#define UNCLAIMED_REG_CLEAR(dev_priv, reg) \
> > +	if (IS_HASWELL(dev_priv->dev) && \
> > +	    (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
> > +		DRM_ERROR("Unknown unclaimed register before writing to %x\n", reg); \
> > +		I915_WRITE_NOTRACE(GEN7_ERR_INT, ERR_INT_MMIO_UNCLAIMED); \
> > +	}
> > +
> 
> You don't really need the if here, do you? I think the way it worked
> originally was fine.
I misread this the first time around. Please ignore this comment.
Apologies.
> 
> > +#define UNCLAIMED_REG_CHECK(dev_priv, reg) \
> > +	if (IS_HASWELL(dev_priv->dev) && \
> > +	    (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
> > +		DRM_ERROR("Unclaimed write to %x\n", reg); \
> > +		writel(ERR_INT_MMIO_UNCLAIMED, dev_priv->regs + GEN7_ERR_INT);	\
> > +	}
> > +
> >  #define __i915_read(x, y) \
> >  u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg) { \
> >  	u##x val = 0; \
> 
> I suppose you could add some 'unlikely' predictions in another patch if
> you wanted. I really don't know what the consensus is on using those
> these day.
> 
> > @@ -1262,10 +1276,7 @@ void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \
> >  	} \
> >  	if (IS_GEN5(dev_priv->dev)) \
> >  		ilk_dummy_write(dev_priv); \
> > -	if (IS_HASWELL(dev_priv->dev) && (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
> > -		DRM_ERROR("Unknown unclaimed register before writing to %x\n", reg); \
> > -		I915_WRITE_NOTRACE(GEN7_ERR_INT, ERR_INT_MMIO_UNCLAIMED); \
> > -	} \
> > +	UNCLAIMED_REG_CLEAR(dev_priv, reg); \
> >  	if (IS_VALLEYVIEW(dev_priv->dev) && IS_DISPLAYREG(reg)) { \
> >  		write##y(val, dev_priv->regs + reg + 0x180000);		\
> >  	} else {			\
> 
> Here's what I meant above, you replace 2 lines of code with 4 from the
> macro. (Including an extra MMIO read).
> 

I misread this the first time around. Please ignore this comment.
Apologies.

> > @@ -1274,10 +1285,7 @@ void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \
> >  	if (unlikely(__fifo_ret)) { \
> >  		gen6_gt_check_fifodbg(dev_priv); \
> >  	} \
> > -	if (IS_HASWELL(dev_priv->dev) && (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
> > -		DRM_ERROR("Unclaimed write to %x\n", reg); \
> > -		writel(ERR_INT_MMIO_UNCLAIMED, dev_priv->regs + GEN7_ERR_INT);	\
> > -	} \
> > +	UNCLAIMED_REG_CHECK(dev_priv, reg); \
> >  }
> >  __i915_write(8, b)
> >  __i915_write(16, w)
> 
> 
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH 3/3] drm/i915: clear the FPGA_DBG_RM_NOCLAIM bit at driver init

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Otherwise, if the BIOS did anything wrong, our first I915_{WRITE,READ}
will give us "unclaimed register"  messages.

V2: Even earlier.

Bugzilla: http://bugs.freedesktop.org/show_bug.cgi?id=58897
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c |    4 ++++
 1 file changed, 4 insertions(+)

If no one is opposed, I'd like to suggest applying patches 1-3 from this series
first, then we worry about the others later.

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index cf06103..d295445 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1542,6 +1542,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto put_gmch;
 	}
 
+	/* This must happen before any I915_{READ,WRITE}: */
+	if (IS_HASWELL(dev))
+		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
+
 	aperture_size = dev_priv->gtt.mappable_end;
 
 	dev_priv->gtt.mappable =
-- 
1.7.10.4

Re: [PATCH 7/7] drm/i915: print Gen 7 error interrupts

[Paulo Zanoni]

Hi

2013/1/25 Ben Widawsky <ben@bwidawsk.net>:
> On Fri, 25 Jan 2013 18:57:42 -0200
> Paulo Zanoni <przanoni@gmail.com> wrote:
>
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> If we get one of these messages it means we did something wrong, and
>> the first step to fix wrong things is to detect them and recognize
>> they exist.
>>
>> For now, leave most messages as DRM_DEBUG_KMS. After we conclude that
>> a certain message should not be print since our code is correct, then
>> we can promote that specific message to DRM_ERROR.
>>
>> Also notice that on Haswell the only interrupt I ever get is for
>> "unclaimed registers", so it looks like at least on Haswell we're in a
>> good shape.
> Please take some of my cynicism, you are way too optimistic :p

The new patch won't just print everything.

>
> I have a lot of concerns with this patch touch FPGA_DEBUG and races.

Please explain.

>
> Third, I think to be super paranoid you might want to disable the ERR_INT while
> handling interrupts.

Yes, we need to mask ERR_INT inside ivybridge_irq_handler, otherwise
we may get an infinite loop in case we write to an unclaimed register
inside the irq handler. This will be fixed in the next version. The
good thing is that even if ERR_INT is masked, we can still detect
unclaimed registers inside the irq handler because we still check
FPGA_DBG (which doesn't give us interrputs) :)

>
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c |   28 +++++++++++++++++++++++++++-
>>  drivers/gpu/drm/i915/i915_reg.h |    2 +-
>>  2 files changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 943db10..c2136cd 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -697,6 +697,17 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>>                                        I915_READ(FDI_RX_IIR(pipe)));
>>  }
>>
>> +static void ivb_err_int_handler(struct drm_device *dev)
>> +{
>> +     drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>> +     u32 err_int = I915_READ(GEN7_ERR_INT);
>> +
>> +     if (err_int)
>> +             DRM_DEBUG_KMS("Error interrupt: 0x%08x\n", err_int);
>> +
>> +     I915_WRITE(GEN7_ERR_INT, err_int);
>> +}
>> +
>>  static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>>  {
>>       struct drm_device *dev = (struct drm_device *) arg;
>> @@ -707,6 +718,14 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>>
>>       atomic_inc(&dev_priv->irq_received);
>>
>> +     /* We get interrupts on unclaimed registers, so check this before we do
>> +      * any I915_{READ,WRITE}. */
>> +     if (drm_debug && IS_HASWELL(dev) &&
>> +         (I915_READ_NOTRACE(FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) {
>> +             DRM_ERROR("Unclaimed register before interrupt\n");
>> +             I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
>> +     }
>> +
>>       /* disable master interrupt before clearing iir  */
>>       de_ier = I915_READ(DEIER);
>>       I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
>
> I don't really understand what you're trying to do with this, and I
> think this is quite racy since you're usually protecting FPGA_DBG with
> struct_mutex, but not here. Since it's mostly for debug, maybe you can
> convince me why it should be here, and I'll drop my complaint.

On i915_write##x we do:
1 - writel(reg_that_doesnt_exist)
2 - read FPGA_DBG
3 - print error message
4 - clear FPGA_DBG

The problem is that sometimes we get the interrupt between steps 1 and
2, or 2 and 3, or 3 and 4. And since the interrupt handler also does
I915_WRITE, we may do the "FPGA_DBG" check inside the IRQ handler
before the "original" i915_WRITE check because the interrupt arrived
too fast.

>
>> @@ -720,6 +739,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>>
>>       de_iir = I915_READ(DEIIR);
>>       if (de_iir) {
>> +             if (de_iir & DE_ERR_INT_IVB)
>> +                     ivb_err_int_handler(dev);
>> +
>>               if (de_iir & DE_AUX_CHANNEL_A_IVB)
>>                       dp_aux_irq_handler(dev);
>>
>> @@ -2006,7 +2028,8 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>>               DE_PLANEC_FLIP_DONE_IVB |
>>               DE_PLANEB_FLIP_DONE_IVB |
>>               DE_PLANEA_FLIP_DONE_IVB |
>> -             DE_AUX_CHANNEL_A_IVB;
>> +             DE_AUX_CHANNEL_A_IVB |
>> +             DE_ERR_INT_IVB;
>>       u32 render_irqs;
>>       u32 hotplug_mask;
>>       u32 pch_irq_mask;
>> @@ -2014,6 +2037,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>>       dev_priv->irq_mask = ~display_mask;
>>
>>       /* should always can generate irq */
>> +     I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>>       I915_WRITE(DEIIR, I915_READ(DEIIR));
>>       I915_WRITE(DEIMR, dev_priv->irq_mask);
>>       I915_WRITE(DEIER,
>> @@ -2177,6 +2201,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>>       I915_WRITE(DEIMR, 0xffffffff);
>>       I915_WRITE(DEIER, 0x0);
>>       I915_WRITE(DEIIR, I915_READ(DEIIR));
>> +     if (IS_GEN7(dev))
>> +             I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>>
>>       I915_WRITE(GTIMR, 0xffffffff);
>>       I915_WRITE(GTIER, 0x0);
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index ee30fb9..86cace1 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -3379,7 +3379,7 @@
>>  #define DE_PIPEA_FIFO_UNDERRUN  (1 << 0)
>>
>>  /* More Ivybridge lolz */
>> -#define DE_ERR_DEBUG_IVB             (1<<30)
>> +#define DE_ERR_INT_IVB                       (1<<30)
>>  #define DE_GSE_IVB                   (1<<29)
>>  #define DE_PCH_EVENT_IVB             (1<<28)
>>  #define DE_DP_A_HOTPLUG_IVB          (1<<27)
>
>
>
> --
> Ben Widawsky, Intel Open Source Technology Center



-- 
Paulo Zanoni