Unclaimed register reporting V2

PATCH In review Paulo Zanoni 2 participants 16 messages Jan 25, 2013 – Feb 8, 2013

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Paulo Zanoni

author

2013-01-25 20:57 UTC #0 lore raw

cover Unclaimed register reporting V2

From: Paulo Zanoni <[email protected]>

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(-)

Signature · 1 line

1.7.10.4

Paulo Zanoni

author

2013-01-25 20:58 UTC #1 lore raw

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

From: Paulo Zanoni <[email protected]>

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

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.c |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

drivers/gpu/drm/i915/i915_drv.c +16 -8

index 9cc8f87..ae0a55a 100644
@@ -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)

Signature · 1 line

1.7.10.4

Ben Widawsky

2013-01-26 01:00 UTC #2 lore raw

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

Quoted 15 lines · On Fri, 25 Jan 2013 18:57:36 -0200 Paulo Zanoni <[email protected]> wrote:

From: Paulo Zanoni <[email protected]>
This avoids polluting i915_write##x and also allows us to reuse code on i915_read##x.
Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_drv.c |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)
drivers/gpu/drm/i915/i915_drv.c +0 -0
index 9cc8f87..ae0a55a 100644

drivers/gpu/drm/i915/i915_drv.c ilk_dummy_write(struct drm_i915_private *dev_priv) · line 1233

Quoted 10 lines

drivers/gpu/drm/i915/i915_drv.c +7 -0

@@ -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.

drivers/gpu/drm/i915/i915_drv.c ilk_dummy_write(struct drm_i915_private *dev_priv) · line 1243

Quoted 10 lines

drivers/gpu/drm/i915/i915_drv.c +7 -0

+#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.

drivers/gpu/drm/i915/i915_drv.c void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \ · line 1282

Quoted 12 lines

drivers/gpu/drm/i915/i915_drv.c +1 -4

@@ -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).

Quoted 12 lines

drivers/gpu/drm/i915/i915_drv.c +1 -4

@@ -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)

Signature · 1 line

Ben Widawsky, Intel Open Source Technology Center

Ben Widawsky

Reviewed-by

2013-01-28 20:16 UTC #3 lore raw

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

Quoted 9 lines · Ben Widawsky

Quoted 6 lines · On Fri, 25 Jan 2013 18:57:36 -0200 Paulo Zanoni <[email protected]> wrote:
From: Paulo Zanoni <[email protected]>
This avoids polluting i915_write##x and also allows us to reuse code on i915_read##x.
Signed-off-by Paulo Zanoni <[email protected]>

Reviewed-by Ben Widawsky <[email protected]>

Quoted 12 lines

---
 drivers/gpu/drm/i915/i915_drv.c |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

drivers/gpu/drm/i915/i915_drv.c +0 -0

index 9cc8f87..ae0a55a 100644
@@ -1224,6 +1224,20 @@ ilk_dummy_write(struct drm_i915_private *dev_priv)
 	I915_WRITE_NOTRACE(MI_MODE, 0);
 }

Quoted 10 lines

drivers/gpu/drm/i915/i915_drv.c ilk_dummy_write(struct drm_i915_private *dev_priv) · line 1233
Quoted 7 lines
drivers/gpu/drm/i915/i915_drv.c +7 -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.

Quoted 16 lines

drivers/gpu/drm/i915/i915_drv.c ilk_dummy_write(struct drm_i915_private *dev_priv) · line 1243
Quoted 11 lines
drivers/gpu/drm/i915/i915_drv.c +7 -0
+#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.

Quoted 16 lines

drivers/gpu/drm/i915/i915_drv.c void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \ · line 1282

Quoted 12 lines

drivers/gpu/drm/i915/i915_drv.c +1 -4

@@ -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.

Quoted 21 lines

Quoted 12 lines

drivers/gpu/drm/i915/i915_drv.c +1 -4

@@ -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)

Signature · 5 lines

Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 1 line

Ben Widawsky, Intel Open Source Technology Center

Paulo Zanoni

author

2013-01-25 20:58 UTC #4 lore raw

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

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

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 <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.c |    8 ++++----
 drivers/gpu/drm/i915/i915_reg.h |    3 +++
 2 files changed, 7 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/i915_drv.c +4 -4

index ae0a55a..f47de4d 100644
@@ -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) \

drivers/gpu/drm/i915/i915_reg.h +3 -0

index 15c15a5..ee30fb9 100644
@@ -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.

Signature · 1 line

1.7.10.4

Ben Widawsky

Reviewed-by on v1 2/7

2013-01-26 01:01 UTC #5 lore raw

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

Quoted 11 lines · On Fri, 25 Jan 2013 18:57:37 -0200 Paulo Zanoni <[email protected]> wrote:

From: Paulo Zanoni <[email protected]>
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 <[email protected]>

Assuming we can come to a consensus on patch 1, it's

Reviewed-by Ben Widawsky <[email protected]>

Signature · 1 line

Ben Widawsky, Intel Open Source Technology Center

Paulo Zanoni

author

2013-01-25 20:58 UTC #6 lore raw

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

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

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

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_dma.c |    4 ++++
 1 file changed, 4 insertions(+)

drivers/gpu/drm/i915/i915_dma.c +4 -0

index 11c7aa8..505d7eb 100644
@@ -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);

Signature · 1 line

1.7.10.4

Ben Widawsky

2013-01-26 01:04 UTC #7 lore raw

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

Quoted 15 lines · On Fri, 25 Jan 2013 18:57:38 -0200 Paulo Zanoni <[email protected]> wrote:

From: Paulo Zanoni <[email protected]>
Otherwise, if the BIOS did anything wrong, our first I915_{READ,WRITE} will give us "unclaimed regsiter" messages.
Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_dma.c |    4 ++++
 1 file changed, 4 insertions(+)
drivers/gpu/drm/i915/i915_dma.c +0 -0
index 11c7aa8..505d7eb 100644

drivers/gpu/drm/i915/i915_dma.c int i915_driver_load(struct drm_device *dev, unsigned long flags) · line 1587

Quoted 10 lines

drivers/gpu/drm/i915/i915_dma.c +4 -0

@@ -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);

Signature · 1 line

Ben Widawsky, Intel Open Source Technology Center

Paulo Zanoni

author

2013-01-29 19:03 UTC #8 lore raw

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

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

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 <[email protected]>

---
 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.

drivers/gpu/drm/i915/i915_dma.c +4 -0

index cf06103..d295445 100644
@@ -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 =

Signature · 1 line

1.7.10.4

Paulo Zanoni

author

2013-01-25 20:58 UTC #9 lore raw

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

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

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 <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/i915_drv.c +8 -6

index f47de4d..422dfc6 100644
@@ -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)

Signature · 1 line

1.7.10.4

Paulo Zanoni

author

2013-01-25 20:58 UTC #10 lore raw

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

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

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 <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/i915_drv.c +2 -2

index 422dfc6..bc0eb88 100644
@@ -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); \
 	}

Signature · 1 line

1.7.10.4

Ben Widawsky

Reviewed-by on v1 5/7

2013-01-26 01:10 UTC #11 lore raw

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

Quoted 11 lines · On Fri, 25 Jan 2013 18:57:40 -0200 Paulo Zanoni <[email protected]> wrote:

From: Paulo Zanoni <[email protected]>
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 <[email protected]>

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 <[email protected]>

Quoted 25 lines

---
 drivers/gpu/drm/i915/i915_drv.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/i915_drv.c +2 -2

index 422dfc6..bc0eb88 100644
@@ -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); \
 	}

Signature · 1 line

Ben Widawsky, Intel Open Source Technology Center

Paulo Zanoni

author

2013-01-25 20:58 UTC #12 lore raw

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

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

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 <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/i915_drv.c +2 -2

index bc0eb88..a401ec8 100644
@@ -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); \

Signature · 1 line

1.7.10.4

Paulo Zanoni

author

2013-01-25 20:58 UTC #13 lore raw

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

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

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 <[email protected]>

---
 drivers/gpu/drm/i915/i915_irq.c |   28 +++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_reg.h |    2 +-
 2 files changed, 28 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +27 -1

index 943db10..c2136cd 100644
@@ -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);

drivers/gpu/drm/i915/i915_reg.h +1 -1

index ee30fb9..86cace1 100644
@@ -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)

Signature · 1 line

1.7.10.4

Ben Widawsky

2013-01-26 01:23 UTC #14 lore raw

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

Quoted 6 lines · On Fri, 25 Jan 2013 18:57:42 -0200 Paulo Zanoni <[email protected]> wrote:

From: Paulo Zanoni <[email protected]>
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.

Quoted 7 lines

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.

Quoted 34 lines

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_irq.c |   28 +++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_reg.h |    2 +-
 2 files changed, 28 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +12 -0

index 943db10..c2136cd 100644
@@ -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

drivers/gpu/drm/i915/i915_irq.c static irqreturn_t ivybridge_irq_handler(int irq, void *arg) · line 731

Quoted 10 lines

drivers/gpu/drm/i915/i915_irq.c +7 -0

+	 * 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.

Quoted 50 lines

drivers/gpu/drm/i915/i915_irq.c +8 -1

@@ -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);

drivers/gpu/drm/i915/i915_reg.h +1 -1

index ee30fb9..86cace1 100644
@@ -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)

Signature · 1 line

Ben Widawsky, Intel Open Source Technology Center

Paulo Zanoni

author

2013-02-08 18:22 UTC #15 lore raw

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

2013/1/25 Ben Widawsky <[email protected]>:

Quoted 9 lines

Quoted 6 lines · On Fri, 25 Jan 2013 18:57:42 -0200 Paulo Zanoni <[email protected]> wrote:
From: Paulo Zanoni <[email protected]>
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.

Quoted 8 lines

Quoted 7 lines
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.

Quoted 2 lines

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

Please explain.

Quoted 3 lines

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) :)

Quoted 34 lines

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_irq.c |   28 +++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_reg.h |    2 +-
 2 files changed, 28 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +11 -0

index 943db10..c2136cd 100644
@@ -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);

Quoted 16 lines

drivers/gpu/drm/i915/i915_irq.c static irqreturn_t ivybridge_irq_handler(int irq, void *arg) · line 731
Quoted 11 lines
drivers/gpu/drm/i915/i915_irq.c +8 -0
+     /* 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.

Quoted 56 lines

Quoted 43 lines

drivers/gpu/drm/i915/i915_irq.c +8 -1

@@ -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);

drivers/gpu/drm/i915/i915_reg.h +0 -0

index ee30fb9..86cace1 100644

drivers/gpu/drm/i915/i915_reg.h line 3385

Quoted 8 lines

drivers/gpu/drm/i915/i915_reg.h +1 -1

@@ -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

Signature · 1 line

Paulo Zanoni