Skip to contentSkip to conversation

Unify interrupt register init/reset

PATCH In review Paulo Zanoni 4 participants 28 messages Jul 23, 2013 – Jul 29, 2013

Subsystems (2)
DRM drivers Intel i915
lore
Share & tools
lore

Paulo Zanoni

author
#0 lore raw

cover Unify interrupt register init/reset

From: Paulo Zanoni <[email protected]>

Hi

This patch series is based on emails I sent a few days ago, with subject "proposals/questions about the IRQ registers". The biggest goal of the series is to have an unified way to initialize/reset IRQ regsiters. After this series, when you add new interrupts to our driver you won't need to think about doing the correct reads, writes and posting_reads, all you'll need to worry about is to call the macros everyone else calls. So now either all the registers are initialized correctly or all the registers are initialized wrongly.

The first 10 patches do all the rework on i915+. The changes should be incremental and any non-trivial changes are on separate patches, so bisecting regressions should be very easy. It is worth mentioning that patches 2, 6, 9 and 10 touch VLV-specific code, and I don't have a way to test this code.

On my intial patches the i8xx code was changed along with the i915+ code, but Ben suggested I shouldn't be touching i8xx code, so patches 11-15 do the i8xx work. I don't know if we really want these patches, but they're here anyway, so feel free to give them a NACK, I won't complain. In case we decide to merge the patches, git bisect should easily spot regressions.

Patches tested on Haswell (constant use for 2 weeks), SandyBridge (a few hours of usage) and Atom (only booted and then suspend/resumed once).

Thanks,
Paulo

Paulo Zanoni (15):
drm/i915: add INTEL_IRQ_REG_RESET
drm/i915: change how VLV_IIR is reset
drm/i915: port i965_irq_uninstall go INTEL_IRQ_REG_RESET drm/i915: really clear the IIR registers
drm/i915: add INTEL_IRQ_REG_INIT
drm/i915: use INTEL_IRQ_REG_INIT on VLV too
drm/i915: reset the IIR registers at preinstall drm/i915: WARN if IIR is not zero at irq_postinstall drm/i915: remove additional zerogin of VLV_IIR at postinstall drm/i915: remove extra clearing of GTIIR from VLV irq preinstall drm/i915: add INTEL_IRQ_REG_RESET16
drm/i915: really clear the IIR registers on i8xx drm/i915: add INTEL_IRQ_REG_INIT16
drm/i915: reset the i8xx IIR registers at preinstall drm/i915: WARN if IIR is not zero at i8xx irq_postinstall

 drivers/gpu/drm/i915/i915_irq.c | 144 +++++++++++++++++-----------------------
 1 file changed, 62 insertions(+), 82 deletions(-)
Signature · 1 line
1.8.1.2
author
#1 lore raw

PATCH 1/15 drm/i915: add INTEL_IRQ_REG_RESET

From: Paulo Zanoni <[email protected]>

The goal is to standardize the way we reset IRQ registers and also reduce source code size. The new functions should touch IMR, IER and IIR in the same way we're currently doing.

There are a few more cases where we could call the new functions or set IIR to non-zero, but these represent real changes, so I'll leave them to future patches for better bisectability.

Idea for the macro implementation shamelessly copied from Ben/Daniel.

v2: - Convert functions to macros (Ben and Daniel)
- Don't touch i8xx code (Ben)

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_irq.c | 58 +++++++++++++++--------------------------
 1 file changed, 21 insertions(+), 37 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +21 -37
index f708e4e..351e30a 100644
@@ -79,6 +79,15 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
+#define INTEL_IRQ_REG_RESET(type, do_iir) do { \
+	I915_WRITE(type##MR, 0xffffffff); \
+	I915_WRITE(type##ER, 0); \
+	if (do_iir) \
+		I915_WRITE(type##IR, I915_READ(type##IR)); \
+	else \
+		POSTING_READ(type##ER); \
+} while (0)
+
 /* For display hotplug interrupt */
 static void
 ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)

@@ -2003,17 +2012,10 @@ static void gen5_gt_irq_preinstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	/* and GT */
-	I915_WRITE(GTIMR, 0xffffffff);
-	I915_WRITE(GTIER, 0x0);
-	POSTING_READ(GTIER);
+	INTEL_IRQ_REG_RESET(GTI, false);
 
-	if (INTEL_INFO(dev)->gen >= 6) {
-		/* and PM */
-		I915_WRITE(GEN6_PMIMR, 0xffffffff);
-		I915_WRITE(GEN6_PMIER, 0x0);
-		POSTING_READ(GEN6_PMIER);
-	}
+	if (INTEL_INFO(dev)->gen >= 6)
+		INTEL_IRQ_REG_RESET(GEN6_PMI, false);
 }
 
 /* drm_dma.h hooks

@@ -2026,9 +2028,7 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 
 	I915_WRITE(HWSTAM, 0xeffe);
 
-	I915_WRITE(DEIMR, 0xffffffff);
-	I915_WRITE(DEIER, 0x0);
-	POSTING_READ(DEIER);
+	INTEL_IRQ_REG_RESET(DEI, false);
 
 	gen5_gt_irq_preinstall(dev);
 

@@ -2061,9 +2061,7 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
 	I915_WRITE(VLV_IIR, 0xffffffff);
-	I915_WRITE(VLV_IMR, 0xffffffff);
-	I915_WRITE(VLV_IER, 0x0);
-	POSTING_READ(VLV_IER);
+	INTEL_IRQ_REG_RESET(VLV_I, false);
 }
 
 static void ibx_hpd_irq_setup(struct drm_device *dev)

@@ -2286,9 +2284,7 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
 	I915_WRITE(VLV_IIR, 0xffffffff);
-	I915_WRITE(VLV_IMR, 0xffffffff);
-	I915_WRITE(VLV_IER, 0x0);
-	POSTING_READ(VLV_IER);
+	INTEL_IRQ_REG_RESET(VLV_I, false);
 }
 
 static void ironlake_irq_uninstall(struct drm_device *dev)

@@ -2302,22 +2298,16 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	I915_WRITE(HWSTAM, 0xffffffff);
 
-	I915_WRITE(DEIMR, 0xffffffff);
-	I915_WRITE(DEIER, 0x0);
-	I915_WRITE(DEIIR, I915_READ(DEIIR));
+	INTEL_IRQ_REG_RESET(DEI, true);
 	if (IS_GEN7(dev))
 		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
 
-	I915_WRITE(GTIMR, 0xffffffff);
-	I915_WRITE(GTIER, 0x0);
-	I915_WRITE(GTIIR, I915_READ(GTIIR));
+	INTEL_IRQ_REG_RESET(GTI, true);
 
 	if (HAS_PCH_NOP(dev))
 		return;
 
-	I915_WRITE(SDEIMR, 0xffffffff);
-	I915_WRITE(SDEIER, 0x0);
-	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
+	INTEL_IRQ_REG_RESET(SDEI, true);
 	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
 		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
 }

@@ -2491,9 +2481,7 @@ static void i915_irq_preinstall(struct drm_device * dev)
 	I915_WRITE16(HWSTAM, 0xeffe);
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0);
-	I915_WRITE(IMR, 0xffffffff);
-	I915_WRITE(IER, 0x0);
-	POSTING_READ(IER);
+	INTEL_IRQ_REG_RESET(I, false);
 }
 
 static int i915_irq_postinstall(struct drm_device *dev)

@@ -2693,10 +2681,8 @@ static void i915_irq_uninstall(struct drm_device * dev)
 		I915_WRITE(PIPESTAT(pipe), 0);
 		I915_WRITE(PIPESTAT(pipe), I915_READ(PIPESTAT(pipe)));
 	}
-	I915_WRITE(IMR, 0xffffffff);
-	I915_WRITE(IER, 0x0);
 
-	I915_WRITE(IIR, I915_READ(IIR));
+	INTEL_IRQ_REG_RESET(I, true);
 }
 
 static void i965_irq_preinstall(struct drm_device * dev)

@@ -2712,9 +2698,7 @@ static void i965_irq_preinstall(struct drm_device * dev)
 	I915_WRITE(HWSTAM, 0xeffe);
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0);
-	I915_WRITE(IMR, 0xffffffff);
-	I915_WRITE(IER, 0x0);
-	POSTING_READ(IER);
+	INTEL_IRQ_REG_RESET(I, false);
 }
 
 static int i965_irq_postinstall(struct drm_device *dev)
Signature · 1 line
1.8.1.2
author
#2 lore raw

PATCH 2/15 drm/i915: change how VLV_IIR is reset

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

In the current code VLV_IIR is reset before VLV_IER and VLV_IMR. This looks wrong because after we reset VLV_IIR and before we clear IMR/IER we can still get more interrupts, so when we finally disable IMR and IER, there's no guaranteee that IIR will be clean. So in this patch we use intel_irq_reg_reset which is the function that is used by everybody else and should be correct.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_irq.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +2 -4
index 351e30a..292337b 100644
@@ -2060,8 +2060,7 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
-	I915_WRITE(VLV_IIR, 0xffffffff);
-	INTEL_IRQ_REG_RESET(VLV_I, false);
+	INTEL_IRQ_REG_RESET(VLV_I, true);
 }
 
 static void ibx_hpd_irq_setup(struct drm_device *dev)

@@ -2283,8 +2282,7 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
-	I915_WRITE(VLV_IIR, 0xffffffff);
-	INTEL_IRQ_REG_RESET(VLV_I, false);
+	INTEL_IRQ_REG_RESET(VLV_I, true);
 }
 
 static void ironlake_irq_uninstall(struct drm_device *dev)
Signature · 1 line
1.8.1.2
author
#3 lore raw

PATCH 3/15 drm/i915: port i965_irq_uninstall go INTEL_IRQ_REG_RESET

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

The problem here is that we have the PIPESTAT registers between IER and IIR, so when we use intel_irq_reg_reset we flip the order used to reset IIR and PIPESTAT. That should be safe since after we clear IMR/IER we won't get any other IIR/PIPESTAT interrupts. Still, the change is on its own patch, so it should be easy to bisect and revert if needed.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_irq.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +1 -3
index 292337b..b1b6552 100644
@@ -2920,13 +2920,11 @@ static void i965_irq_uninstall(struct drm_device * dev)
 	I915_WRITE(HWSTAM, 0xffffffff);
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0);
-	I915_WRITE(IMR, 0xffffffff);
-	I915_WRITE(IER, 0x0);
+	INTEL_IRQ_REG_RESET(I, true);
 
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe),
 			   I915_READ(PIPESTAT(pipe)) & 0x8000ffff);
-	I915_WRITE(IIR, I915_READ(IIR));
 }
 
 static void i915_reenable_hotplug_timer_func(unsigned long data)
Signature · 1 line
1.8.1.2
#4 lore raw

Re: [PATCH 03/15] drm/i915: port i965_irq_uninstall go INTEL_IRQ_REG_RESET

Quoted 8 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

The problem here is that we have the PIPESTAT registers between IER and IIR, so when we use intel_irq_reg_reset we flip the order used to reset IIR and PIPESTAT. That should be safe since after we clear IMR/IER we won't get any other IIR/PIPESTAT interrupts. Still, the change is on its own patch, so it should be easy to bisect and revert if needed.

This is wrong. PIPESTAT needs to be cleared before IIR. -Chris

Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
author
#5 lore raw

Re: [PATCH 03/15] drm/i915: port i965_irq_uninstall go INTEL_IRQ_REG_RESET

2013/7/24 Chris Wilson <[email protected]>:

Quoted 11 lines · Paulo Zanoni
Quoted 8 lines

From: Paulo Zanoni <[email protected]>

The problem here is that we have the PIPESTAT registers between IER and IIR, so when we use intel_irq_reg_reset we flip the order used to reset IIR and PIPESTAT. That should be safe since after we clear IMR/IER we won't get any other IIR/PIPESTAT interrupts. Still, the change is on its own patch, so it should be easy to bisect and revert if needed.

This is wrong. PIPESTAT needs to be cleared before IIR.

Yeah, you're right. I see that if we want to make all the code touching pipestat consistent we could probably dedicate a full patch series to it. The fact that PIPESTAT needs to be disabled after IER/IMR but before IIR also kinda breaks my macros, I'll have to rethink them now.

Thanks for the review!

Quoted 4 lines

-Chris

--
Chris Wilson, Intel Open Source Technology Centre

Signature · 1 line
Paulo Zanoni
#6 lore raw

Re: [PATCH 03/15] drm/i915: port i965_irq_uninstall go INTEL_IRQ_REG_RESET

Quoted 11 lines · Paulo Zanoni

2013/7/24 Chris Wilson <[email protected]>:

Quoted 10 lines · Paulo Zanoni
Quoted 8 lines

From: Paulo Zanoni <[email protected]>

The problem here is that we have the PIPESTAT registers between IER and IIR, so when we use intel_irq_reg_reset we flip the order used to reset IIR and PIPESTAT. That should be safe since after we clear IMR/IER we won't get any other IIR/PIPESTAT interrupts. Still, the change is on its own patch, so it should be easy to bisect and revert if needed.

Quoted 7 lines
Quoted 1 line

This is wrong. PIPESTAT needs to be cleared before IIR.

Yeah, you're right. I see that if we want to make all the code touching pipestat consistent we could probably dedicate a full patch series to it. The fact that PIPESTAT needs to be disabled after IER/IMR but before IIR also kinda breaks my macros, I'll have to rethink them now.

Why would it need to be cleared before IMR/IER? Clearing PIPESTAT first, them IER/IMR, and finally IIR should be OK.

Signature · 2 lines
Ville Syrjälä
Intel OTC
author
#7 lore raw

PATCH 4/15 drm/i915: really clear the IIR registers

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

As written on our docs, the IIR registers are capable of storing 2 interrupts, so if we write once to them there's no guarantee they will become zero. So on this patch we write to the register, read to check if it's zero, and then write again in case it's needed.

Also replace I915_WRITE(iir, I915_READ(iir)) with I915_WRITE(iir, 0xffffffff), and then move the POSTING_READs on IER because we removed the extra IIR read.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +8 -4
index b1b6552..29eac7a 100644
@@ -82,10 +82,14 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 #define INTEL_IRQ_REG_RESET(type, do_iir) do { \
 	I915_WRITE(type##MR, 0xffffffff); \
 	I915_WRITE(type##ER, 0); \
-	if (do_iir) \
-		I915_WRITE(type##IR, I915_READ(type##IR)); \
-	else \
-		POSTING_READ(type##ER); \
+	POSTING_READ(type##ER); \
+	if (do_iir) { \
+		I915_WRITE(type##IR, 0xffffffff); \
+		if (I915_READ(type##IR)) { \
+			I915_WRITE(type##IR, 0xffffffff); \
+			POSTING_READ(type##IR); \
+		} \
+	} \
 } while (0)
 
 /* For display hotplug interrupt */
Signature · 1 line
1.8.1.2
#8 lore raw

Re: [PATCH 04/15] drm/i915: really clear the IIR registers

Quoted 10 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

As written on our docs, the IIR registers are capable of storing 2 interrupts, so if we write once to them there's no guarantee they will become zero. So on this patch we write to the register, read to check if it's zero, and then write again in case it's needed.

Also replace I915_WRITE(iir, I915_READ(iir)) with I915_WRITE(iir, 0xffffffff), and then move the POSTING_READs on IER because we removed the extra IIR read.

Just write(read(iir)) twice and add a comment why. -Chris

Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
author
#9 lore raw

Re: [PATCH 04/15] drm/i915: really clear the IIR registers

2013/7/24 Chris Wilson <[email protected]>:

Quoted 8 lines · Paulo Zanoni
Quoted 7 lines

From: Paulo Zanoni <[email protected]>

As written on our docs, the IIR registers are capable of storing 2 interrupts, so if we write once to them there's no guarantee they will become zero. So on this patch we write to the register, read to check if it's zero, and then write again in case it's needed.

Quoted 5 lines
Quoted 3 lines

Also replace I915_WRITE(iir, I915_READ(iir)) with I915_WRITE(iir, 0xffffffff), and then move the POSTING_READs on IER because we removed the extra IIR read.

Just write(read(iir)) twice and add a comment why.

Just to clarify: why not write(iir, 0xffffffff) twice to save the 2 reads?

Quoted 4 lines

-Chris

--
Chris Wilson, Intel Open Source Technology Centre

Signature · 1 line
Paulo Zanoni
#10 lore raw

Re: [PATCH 04/15] drm/i915: really clear the IIR registers

Quoted 9 lines · Paulo Zanoni

2013/7/24 Chris Wilson <[email protected]>:

Quoted 8 lines · Paulo Zanoni
Quoted 7 lines

From: Paulo Zanoni <[email protected]>

As written on our docs, the IIR registers are capable of storing 2 interrupts, so if we write once to them there's no guarantee they will become zero. So on this patch we write to the register, read to check if it's zero, and then write again in case it's needed.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

Also replace I915_WRITE(iir, I915_READ(iir)) with I915_WRITE(iir, 0xffffffff), and then move the POSTING_READs on IER because we removed the extra IIR read.

Just write(read(iir)) twice and add a comment why.

Just to clarify: why not write(iir, 0xffffffff) twice to save the 2 reads?

Force of habit... You occassionally come across some register that mixes a toggle with a status pin - so you only set what is asserted so that you clear the status pins without enabling anything else. I don't think that applies to these but you are covering over a decade of hw engineering with many lost secrets...
-Chris

Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
author
#11 lore raw

PATCH 5/15 drm/i915: add INTEL_IRQ_REG_INIT

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Same reason as intel_irq_reg_reset: let's standardize the way we init registers so we make sure all the code is doing the same thing, and then we can also change everybody at the same time if we need. This function is for irq_postinstall functions. Again, this patch only converts the cases where the new code perfectly matches the old one, other cases will be done in separate patches for better bisectability.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_irq.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +14 -16
index 29eac7a..e416848 100644
@@ -92,6 +92,14 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	} \
 } while (0)
 
+#define INTEL_IRQ_REG_INIT(type, do_iir, ier_val, imr_val) do { \
+	if (do_iir) \
+		I915_WRITE(type##IR, I915_READ(type##IR)); \
+	I915_WRITE(type##MR, (imr_val)); \
+	I915_WRITE(type##ER, (ier_val)); \
+	POSTING_READ(type##ER); \
+} while (0)
+
 /* For display hotplug interrupt */
 static void
 ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)

@@ -2145,10 +2153,7 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
 		gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
 	}
 
-	I915_WRITE(GTIIR, I915_READ(GTIIR));
-	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
-	I915_WRITE(GTIER, gt_irqs);
-	POSTING_READ(GTIER);
+	INTEL_IRQ_REG_INIT(GTI, true, gt_irqs, dev_priv->gt_irq_mask);
 
 	if (INTEL_INFO(dev)->gen >= 6) {
 		pm_irqs |= GEN6_PM_RPS_EVENTS;

@@ -2156,10 +2161,7 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
 		if (HAS_VEBOX(dev))
 			pm_irqs |= PM_VEBOX_USER_INTERRUPT;
 
-		I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
-		I915_WRITE(GEN6_PMIMR, 0xffffffff);
-		I915_WRITE(GEN6_PMIER, pm_irqs);
-		POSTING_READ(GEN6_PMIER);
+		INTEL_IRQ_REG_INIT(GEN6_PMI, true, pm_irqs, 0xffffffff);
 	}
 }
 

@@ -2189,11 +2191,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 
 	dev_priv->irq_mask = ~display_mask;
 
-	/* should always can generate irq */
-	I915_WRITE(DEIIR, I915_READ(DEIIR));
-	I915_WRITE(DEIMR, dev_priv->irq_mask);
-	I915_WRITE(DEIER, display_mask | extra_mask);
-	POSTING_READ(DEIER);
+	INTEL_IRQ_REG_INIT(DEI, true, display_mask | extra_mask,
+			   dev_priv->irq_mask);
 
 	gen5_gt_irq_postinstall(dev);
 

@@ -2519,9 +2518,7 @@ static int i915_irq_postinstall(struct drm_device *dev)
 		dev_priv->irq_mask &= ~I915_DISPLAY_PORT_INTERRUPT;
 	}
 
-	I915_WRITE(IMR, dev_priv->irq_mask);
-	I915_WRITE(IER, enable_mask);
-	POSTING_READ(IER);
+	INTEL_IRQ_REG_INIT(I, false, enable_mask, dev_priv->irq_mask);
 
 	i915_enable_asle_pipestat(dev);
 

@@ -2751,6 +2748,7 @@ static int i965_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(IMR, dev_priv->irq_mask);
 	I915_WRITE(IER, enable_mask);
 	POSTING_READ(IER);
+	INTEL_IRQ_REG_INIT(I, false, enable_mask, dev_priv->irq_mask);
 
 	I915_WRITE(PORT_HOTPLUG_EN, 0);
 	POSTING_READ(PORT_HOTPLUG_EN);
Signature · 1 line
1.8.1.2
#12 lore raw

Re: [PATCH 05/15] drm/i915: add INTEL_IRQ_REG_INIT

Quoted 12 lines · Paulo Zanoni
drivers/gpu/drm/i915/i915_irq.c +1 -3
@@ -2519,9 +2518,7 @@ static int i915_irq_postinstall(struct drm_device *dev)
 		dev_priv->irq_mask &= ~I915_DISPLAY_PORT_INTERRUPT;
 	}
 
-	I915_WRITE(IMR, dev_priv->irq_mask);
-	I915_WRITE(IER, enable_mask);
-	POSTING_READ(IER);
+	INTEL_IRQ_REG_INIT(I, false, enable_mask, dev_priv->irq_mask);
 
 	i915_enable_asle_pipestat(dev);

drivers/gpu/drm/i915/i915_irq.c static int i965_irq_postinstall(struct drm_device *dev) · line 2754

Quoted 7 lines
drivers/gpu/drm/i915/i915_irq.c +1 -0
@@ -2751,6 +2748,7 @@ static int i965_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(IMR, dev_priv->irq_mask);
 	I915_WRITE(IER, enable_mask);
 	POSTING_READ(IER);
+	INTEL_IRQ_REG_INIT(I, false, enable_mask, dev_priv->irq_mask);
 
 	I915_WRITE(PORT_HOTPLUG_EN, 0);
 	POSTING_READ(PORT_HOTPLUG_EN);

Change missing?
-Chris

Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
author
#13 lore raw

PATCH 6/15 drm/i915: use INTEL_IRQ_REG_INIT on VLV too

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

This is a case where the code written doesn't match INTEL_IRQ_REG_INIT perfectly. Call it after clearing PIPESTAT so we make sure that it is cleared while the interrupts are disabled.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_irq.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +1 -4
index e416848..37420b5 100644
@@ -2236,12 +2236,9 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(PORT_HOTPLUG_EN, 0);
 	POSTING_READ(PORT_HOTPLUG_EN);
 
-	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
-	I915_WRITE(VLV_IER, enable_mask);
-	I915_WRITE(VLV_IIR, 0xffffffff);
 	I915_WRITE(PIPESTAT(0), 0xffff);
 	I915_WRITE(PIPESTAT(1), 0xffff);
-	POSTING_READ(VLV_IER);
+	INTEL_IRQ_REG_INIT(VLV_I, true, enable_mask, dev_priv->irq_mask);
 
 	/* Interrupt setup is already guaranteed to be single-threaded, this is
 	 * just to make the assert_spin_locked check happy. */
Signature · 1 line
1.8.1.2
author
#14 lore raw

PATCH 7/15 drm/i915: reset the IIR registers at preinstall

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

The long-term goal is to make preinstall and uninstall almost the same thing, so we will be able to call uninstall and then postinstall if needed.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_irq.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +16 -19
index 37420b5..400aa63 100644
@@ -79,16 +79,13 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
-#define INTEL_IRQ_REG_RESET(type, do_iir) do { \
+#define INTEL_IRQ_REG_RESET(type) do { \
 	I915_WRITE(type##MR, 0xffffffff); \
 	I915_WRITE(type##ER, 0); \
-	POSTING_READ(type##ER); \
-	if (do_iir) { \
+	I915_WRITE(type##IR, 0xffffffff); \
+	if (I915_READ(type##IR)) { \
 		I915_WRITE(type##IR, 0xffffffff); \
-		if (I915_READ(type##IR)) { \
-			I915_WRITE(type##IR, 0xffffffff); \
-			POSTING_READ(type##IR); \
-		} \
+		POSTING_READ(type##IR); \
 	} \
 } while (0)
 

@@ -2024,10 +2021,10 @@ static void gen5_gt_irq_preinstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	INTEL_IRQ_REG_RESET(GTI, false);
+	INTEL_IRQ_REG_RESET(GTI);
 
 	if (INTEL_INFO(dev)->gen >= 6)
-		INTEL_IRQ_REG_RESET(GEN6_PMI, false);
+		INTEL_IRQ_REG_RESET(GEN6_PMI);
 }
 
 /* drm_dma.h hooks

@@ -2040,7 +2037,7 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 
 	I915_WRITE(HWSTAM, 0xeffe);
 
-	INTEL_IRQ_REG_RESET(DEI, false);
+	INTEL_IRQ_REG_RESET(DEI);
 
 	gen5_gt_irq_preinstall(dev);
 

@@ -2072,7 +2069,7 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
-	INTEL_IRQ_REG_RESET(VLV_I, true);
+	INTEL_IRQ_REG_RESET(VLV_I);
 }
 
 static void ibx_hpd_irq_setup(struct drm_device *dev)

@@ -2282,7 +2279,7 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
-	INTEL_IRQ_REG_RESET(VLV_I, true);
+	INTEL_IRQ_REG_RESET(VLV_I);
 }
 
 static void ironlake_irq_uninstall(struct drm_device *dev)

@@ -2296,16 +2293,16 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	I915_WRITE(HWSTAM, 0xffffffff);
 
-	INTEL_IRQ_REG_RESET(DEI, true);
+	INTEL_IRQ_REG_RESET(DEI);
 	if (IS_GEN7(dev))
 		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
 
-	INTEL_IRQ_REG_RESET(GTI, true);
+	INTEL_IRQ_REG_RESET(GTI);
 
 	if (HAS_PCH_NOP(dev))
 		return;
 
-	INTEL_IRQ_REG_RESET(SDEI, true);
+	INTEL_IRQ_REG_RESET(SDEI);
 	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
 		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
 }

@@ -2479,7 +2476,7 @@ static void i915_irq_preinstall(struct drm_device * dev)
 	I915_WRITE16(HWSTAM, 0xeffe);
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0);
-	INTEL_IRQ_REG_RESET(I, false);
+	INTEL_IRQ_REG_RESET(I);
 }
 
 static int i915_irq_postinstall(struct drm_device *dev)

@@ -2678,7 +2675,7 @@ static void i915_irq_uninstall(struct drm_device * dev)
 		I915_WRITE(PIPESTAT(pipe), I915_READ(PIPESTAT(pipe)));
 	}
 
-	INTEL_IRQ_REG_RESET(I, true);
+	INTEL_IRQ_REG_RESET(I);
 }
 
 static void i965_irq_preinstall(struct drm_device * dev)

@@ -2694,7 +2691,7 @@ static void i965_irq_preinstall(struct drm_device * dev)
 	I915_WRITE(HWSTAM, 0xeffe);
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0);
-	INTEL_IRQ_REG_RESET(I, false);
+	INTEL_IRQ_REG_RESET(I);
 }
 
 static int i965_irq_postinstall(struct drm_device *dev)

@@ -2919,7 +2916,7 @@ static void i965_irq_uninstall(struct drm_device * dev)
 	I915_WRITE(HWSTAM, 0xffffffff);
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0);
-	INTEL_IRQ_REG_RESET(I, true);
+	INTEL_IRQ_REG_RESET(I);
 
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe),
Signature · 1 line
1.8.1.2
#15 lore raw

Re: [PATCH 07/15] drm/i915: reset the IIR registers at preinstall

Quoted 5 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

The long-term goal is to make preinstall and uninstall almost the same thing, so we will be able to call uninstall and then postinstall if needed.

write(read(iir)) as before
-Chris

Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
author
#16 lore raw

PATCH 8/15 drm/i915: WARN if IIR is not zero at irq_postinstall

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Because now we're clearing the IIR register at preinstall and uninstall, so let's just check if it's still zero at postinstall. Since we should be disabling the interrupts at preinstall, there is no way the IIR could become non-zero at postinstall, so this patch may uncover bugs (e.g., missing code at preinstall).

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_irq.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +1 -2
index 400aa63..6793b30 100644
@@ -90,8 +90,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 } while (0)
 
 #define INTEL_IRQ_REG_INIT(type, do_iir, ier_val, imr_val) do { \
-	if (do_iir) \
-		I915_WRITE(type##IR, I915_READ(type##IR)); \
+	WARN(I915_READ(type##IR) != 0, "Register 0x%x is not 0\n", type##IR); \
 	I915_WRITE(type##MR, (imr_val)); \
 	I915_WRITE(type##ER, (ier_val)); \
 	POSTING_READ(type##ER); \
Signature · 1 line
1.8.1.2
author
#17 lore raw

PATCH 9/15 drm/i915: remove additional zerogin of VLV_IIR at postinstall

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

There is no comment explaining why it's needed and I don't see a reason for it to exist. Also, a few lines above we call INTEL_IRQ_REG_INIT and pass VLV_IIR, so we should print a WARN in case VLV_IIR is not zero at that point.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_irq.c | 3 ---
 1 file changed, 3 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +0 -3
index 6793b30..5ca50a0 100644
@@ -2244,9 +2244,6 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
 	i915_enable_pipestat(dev_priv, 1, pipestat_enable);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
-	I915_WRITE(VLV_IIR, 0xffffffff);
-	I915_WRITE(VLV_IIR, 0xffffffff);
-
 	gen5_gt_irq_postinstall(dev);
 
 	/* ack & enable invalid PTE error interrupts */
Signature · 1 line
1.8.1.2
author
#18 lore raw

PATCH 10/15 drm/i915: remove extra clearing of GTIIR from VLV irq preinstall

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

After the last changes, gen5_gt_irq_preinstall should now guarantee that GTIIR will be zero. Also, at valleyview_irq_postinstall we call gen5_gt_irq_postinstall which calls intel_irq_reg_init, which will print a nice WARN in case GTIIR is not zero.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_irq.c | 4 ----
 1 file changed, 4 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +0 -4
index 5ca50a0..9684f1e 100644
@@ -2056,10 +2056,6 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(RING_IMR(GEN6_BSD_RING_BASE), 0);
 	I915_WRITE(RING_IMR(BLT_RING_BASE), 0);
 
-	/* and GT */
-	I915_WRITE(GTIIR, I915_READ(GTIIR));
-	I915_WRITE(GTIIR, I915_READ(GTIIR));
-
 	gen5_gt_irq_preinstall(dev);
 
 	I915_WRITE(DPINVGTT, 0xff);
Signature · 1 line
1.8.1.2
author
#19 lore raw

PATCH 11/15 drm/i915: add INTEL_IRQ_REG_RESET16

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Same thing as INTEL_IRQ_REG_RESET, but now on a separate patch, as requested by Ben.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_irq.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +11 -6
index 9684f1e..7588ad3 100644
@@ -79,6 +79,15 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
+#define INTEL_IRQ_REG_RESET16(type, do_iir) do { \
+	I915_WRITE16(type##MR, 0xffff); \
+	I915_WRITE16(type##ER, 0); \
+	if (do_iir) \
+		I915_WRITE16(type##IR, I915_READ16(type##IR)); \
+	else \
+		POSTING_READ16(type##ER); \
+} while (0)
+
 #define INTEL_IRQ_REG_RESET(type) do { \
 	I915_WRITE(type##MR, 0xffffffff); \
 	I915_WRITE(type##ER, 0); \

@@ -2308,9 +2317,7 @@ static void i8xx_irq_preinstall(struct drm_device * dev)
 
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0);
-	I915_WRITE16(IMR, 0xffff);
-	I915_WRITE16(IER, 0x0);
-	POSTING_READ16(IER);
+	INTEL_IRQ_REG_RESET16(I, false);
 }
 
 static int i8xx_irq_postinstall(struct drm_device *dev)

@@ -2448,9 +2455,7 @@ static void i8xx_irq_uninstall(struct drm_device * dev)
 		I915_WRITE(PIPESTAT(pipe), 0);
 		I915_WRITE(PIPESTAT(pipe), I915_READ(PIPESTAT(pipe)));
 	}
-	I915_WRITE16(IMR, 0xffff);
-	I915_WRITE16(IER, 0x0);
-	I915_WRITE16(IIR, I915_READ16(IIR));
+	INTEL_IRQ_REG_RESET16(I, true);
 }
 
 static void i915_irq_preinstall(struct drm_device * dev)
Signature · 1 line
1.8.1.2
#20 lore raw

Re: [PATCH 11/15] drm/i915: add INTEL_IRQ_REG_RESET16

Quoted 4 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Same thing as INTEL_IRQ_REG_RESET, but now on a separate patch, as requested by Ben.

Maybe generate two functions (32/16 bit variants) from the macros instead. The macros are becoming quite a chunk of code.
-Chris

Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
author
#21 lore raw

PATCH 12/15 drm/i915: really clear the IIR registers on i8xx

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Same thing as "drm/i915: really clear the IIR registers", but on a separate patch for easier bisecting.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +8 -4
index 7588ad3..a936c59 100644
@@ -82,10 +82,14 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 #define INTEL_IRQ_REG_RESET16(type, do_iir) do { \
 	I915_WRITE16(type##MR, 0xffff); \
 	I915_WRITE16(type##ER, 0); \
-	if (do_iir) \
-		I915_WRITE16(type##IR, I915_READ16(type##IR)); \
-	else \
-		POSTING_READ16(type##ER); \
+	POSTING_READ16(type##ER); \
+	if (do_iir) { \
+		I915_WRITE16(type##IR, 0xffff); \
+		if (I915_READ16(type##IR)) { \
+			I915_WRITE16(type##IR, 0xffff); \
+			POSTING_READ16(type##IR); \
+		} \
+	} \
 } while (0)
 
 #define INTEL_IRQ_REG_RESET(type) do { \
Signature · 1 line
1.8.1.2
author
#22 lore raw

PATCH 13/15 drm/i915: add INTEL_IRQ_REG_INIT16

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Same thing as INTEL_IRQ_REG_INIT, but on a separate patch.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_irq.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +14 -8
index a936c59..d32042d 100644
@@ -102,6 +102,14 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	} \
 } while (0)
 
+#define INTEL_IRQ_REG_INIT16(type, do_iir, ier_val, imr_val) do { \
+	if (do_iir) \
+		I915_WRITE16(type##IR, I915_READ16(type##IR)); \
+	I915_WRITE16(type##MR, (imr_val)); \
+	I915_WRITE16(type##ER, (ier_val)); \
+	POSTING_READ16(type##ER); \
+} while (0)
+
 #define INTEL_IRQ_REG_INIT(type, do_iir, ier_val, imr_val) do { \
 	WARN(I915_READ(type##IR) != 0, "Register 0x%x is not 0\n", type##IR); \
 	I915_WRITE(type##MR, (imr_val)); \

@@ -2327,6 +2335,10 @@ static void i8xx_irq_preinstall(struct drm_device * dev)
 static int i8xx_irq_postinstall(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
+	uint32_t enable_mask = (I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
+				I915_DISPLAY_PIPE_B_EVENT_INTERRUPT |
+				I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT |
+				I915_USER_INTERRUPT);
 
 	I915_WRITE16(EMR,
 		     ~(I915_ERROR_PAGE_TABLE | I915_ERROR_MEMORY_REFRESH));

@@ -2338,14 +2350,8 @@ static int i8xx_irq_postinstall(struct drm_device *dev)
 		  I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
 		  I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT |
 		  I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT);
-	I915_WRITE16(IMR, dev_priv->irq_mask);
-
-	I915_WRITE16(IER,
-		     I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
-		     I915_DISPLAY_PIPE_B_EVENT_INTERRUPT |
-		     I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT |
-		     I915_USER_INTERRUPT);
-	POSTING_READ16(IER);
+
+	INTEL_IRQ_REG_INIT16(I, false, enable_mask, dev_priv->irq_mask);
 
 	return 0;
 }
Signature · 1 line
1.8.1.2
author
#23 lore raw

PATCH 14/15 drm/i915: reset the i8xx IIR registers at preinstall

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Same thing as the equivalent commit that touched INTEL_IRQ_REG_RESET.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_irq.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +6 -9
index d32042d..4f0bc26 100644
@@ -79,16 +79,13 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
-#define INTEL_IRQ_REG_RESET16(type, do_iir) do { \
+#define INTEL_IRQ_REG_RESET16(type) do { \
 	I915_WRITE16(type##MR, 0xffff); \
 	I915_WRITE16(type##ER, 0); \
-	POSTING_READ16(type##ER); \
-	if (do_iir) { \
+	I915_WRITE16(type##IR, 0xffff); \
+	if (I915_READ16(type##IR)) { \
 		I915_WRITE16(type##IR, 0xffff); \
-		if (I915_READ16(type##IR)) { \
-			I915_WRITE16(type##IR, 0xffff); \
-			POSTING_READ16(type##IR); \
-		} \
+		POSTING_READ16(type##IR); \
 	} \
 } while (0)
 

@@ -2329,7 +2326,7 @@ static void i8xx_irq_preinstall(struct drm_device * dev)
 
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0);
-	INTEL_IRQ_REG_RESET16(I, false);
+	INTEL_IRQ_REG_RESET16(I);
 }
 
 static int i8xx_irq_postinstall(struct drm_device *dev)

@@ -2465,7 +2462,7 @@ static void i8xx_irq_uninstall(struct drm_device * dev)
 		I915_WRITE(PIPESTAT(pipe), 0);
 		I915_WRITE(PIPESTAT(pipe), I915_READ(PIPESTAT(pipe)));
 	}
-	INTEL_IRQ_REG_RESET16(I, true);
+	INTEL_IRQ_REG_RESET16(I);
 }
 
 static void i915_irq_preinstall(struct drm_device * dev)
Signature · 1 line
1.8.1.2
author
#24 lore raw

PATCH 15/15 drm/i915: WARN if IIR is not zero at i8xx irq_postinstall

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Same thing as the previous non-i8xx commit.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +2 -2
index 4f0bc26..8b12caa 100644
@@ -100,8 +100,8 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 } while (0)
 
 #define INTEL_IRQ_REG_INIT16(type, do_iir, ier_val, imr_val) do { \
-	if (do_iir) \
-		I915_WRITE16(type##IR, I915_READ16(type##IR)); \
+	WARN(I915_READ16(type##IR) != 0, "Register 0x%x is not 0\n", \
+	     type##IR); \
 	I915_WRITE16(type##MR, (imr_val)); \
 	I915_WRITE16(type##ER, (ier_val)); \
 	POSTING_READ16(type##ER); \
Signature · 1 line
1.8.1.2
#25 lore raw
In reply to Paulo Zanoni
Quoted 17 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Hi

This patch series is based on emails I sent a few days ago, with subject "proposals/questions about the IRQ registers". The biggest goal of the series is to have an unified way to initialize/reset IRQ regsiters. After this series, when you add new interrupts to our driver you won't need to think about doing the correct reads, writes and posting_reads, all you'll need to worry about is to call the macros everyone else calls. So now either all the registers are initialized correctly or all the registers are initialized wrongly.

The first 10 patches do all the rework on i915+. The changes should be incremental and any non-trivial changes are on separate patches, so bisecting regressions should be very easy. It is worth mentioning that patches 2, 6, 9 and 10 touch VLV-specific code, and I don't have a way to test this code.

Quoted 8 lines

On my intial patches the i8xx code was changed along with the i915+ code, but Ben suggested I shouldn't be touching i8xx code, so patches 11-15 do the i8xx work. I don't know if we really want these patches, but they're here anyway, so feel free to give them a NACK, I won't complain. In case we decide to merge the patches, git bisect should easily spot regressions.

Patches tested on Haswell (constant use for 2 weeks), SandyBridge (a few hours of usage) and Atom (only booted and then suspend/resumed once).

My first question was why you didn't use name##IMR, name##IER, name##IIR in the macros, since that would match what we have in -internal and imo would look more natural. But then I've noticed that for IIR, IER & IMR we don't have a prefix ... I still think the I in the name looks a bit funny though (e.g. GTI). Maybe we should add an I915_ prefix then we could remove the I.

The other thing I'm unclear about is the do_iir flag, which in the end seems to clear IIR bits in uninstall but not in preinstall. Iirc Ben's proposal was to do the double IIR clear in preinstall, and I tend to agree with him. Can you please elaborate on this?

Maybe one patch on top to add some kerneldoc to the new magic macros would be good so that we can explain once what the thinking between the sequence is.

Cheers, Daniel

Quoted 32 lines

Thanks,
Paulo

Paulo Zanoni (15):
drm/i915: add INTEL_IRQ_REG_RESET
drm/i915: change how VLV_IIR is reset
drm/i915: port i965_irq_uninstall go INTEL_IRQ_REG_RESET drm/i915: really clear the IIR registers
drm/i915: add INTEL_IRQ_REG_INIT
drm/i915: use INTEL_IRQ_REG_INIT on VLV too
drm/i915: reset the IIR registers at preinstall drm/i915: WARN if IIR is not zero at irq_postinstall drm/i915: remove additional zerogin of VLV_IIR at postinstall drm/i915: remove extra clearing of GTIIR from VLV irq preinstall drm/i915: add INTEL_IRQ_REG_RESET16
drm/i915: really clear the IIR registers on i8xx drm/i915: add INTEL_IRQ_REG_INIT16
drm/i915: reset the i8xx IIR registers at preinstall drm/i915: WARN if IIR is not zero at i8xx irq_postinstall

 drivers/gpu/drm/i915/i915_irq.c | 144 +++++++++++++++++-----------------------
 1 file changed, 62 insertions(+), 82 deletions(-)
Signature · 6 lines
1.8.1.2

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
author
#26 lore raw

2013/7/24 Daniel Vetter <[email protected]>:

Quoted 27 lines · Paulo Zanoni
Quoted 17 lines

From: Paulo Zanoni <[email protected]>

Hi

This patch series is based on emails I sent a few days ago, with subject "proposals/questions about the IRQ registers". The biggest goal of the series is to have an unified way to initialize/reset IRQ regsiters. After this series, when you add new interrupts to our driver you won't need to think about doing the correct reads, writes and posting_reads, all you'll need to worry about is to call the macros everyone else calls. So now either all the registers are initialized correctly or all the registers are initialized wrongly.

The first 10 patches do all the rework on i915+. The changes should be incremental and any non-trivial changes are on separate patches, so bisecting regressions should be very easy. It is worth mentioning that patches 2, 6, 9 and 10 touch VLV-specific code, and I don't have a way to test this code.

Quoted 8 lines

On my intial patches the i8xx code was changed along with the i915+ code, but Ben suggested I shouldn't be touching i8xx code, so patches 11-15 do the i8xx work. I don't know if we really want these patches, but they're here anyway, so feel free to give them a NACK, I won't complain. In case we decide to merge the patches, git bisect should easily spot regressions.

Patches tested on Haswell (constant use for 2 weeks), SandyBridge (a few hours of usage) and Atom (only booted and then suspend/resumed once).

Quoted 6 lines

My first question was why you didn't use name##IMR, name##IER, name##IIR in the macros, since that would match what we have in -internal and imo would look more natural. But then I've noticed that for IIR, IER & IMR we don't have a prefix ... I still think the I in the name looks a bit funny though (e.g. GTI). Maybe we should add an I915_ prefix then we could remove the I.

Yeah, you guessed the reason. If we use name##IMR then we could replace "INTEL_IRQ_REG_INIT(I, false, enable_mask, dev_priv->irq_mask);" with "INTEL_IRQ_REG_INIT(, false, enable_mask, dev_priv->irq_mask);", but I guess your suggestion of renaming is probably better. The problem with renaming to I915_IIR is that the registers also exists on i8xx, so I'd vote for another name (GEN2_IIR?).

Quoted 5 lines

The other thing I'm unclear about is the do_iir flag, which in the end seems to clear IIR bits in uninstall but not in preinstall. Iirc Ben's proposal was to do the double IIR clear in preinstall, and I tend to agree with him. Can you please elaborate on this?

Please look at the final result of the series, we remove the flag on later patches (you just made me notice I forgot to remove it on the reg_init functions, but we don't really use it anymore). At the end of the series we do the double-IIR clear at both preinstall and uninstall. I wanted to keep one functional change at each patch.

Quoted 4 lines

Maybe one patch on top to add some kerneldoc to the new magic macros would be good so that we can explain once what the thinking between the sequence is.

Will do.

Thanks Daniel and Chris for the fast reviews!

Quoted 39 lines

Cheers, Daniel

Quoted 25 lines

Thanks,
Paulo

Paulo Zanoni (15):
drm/i915: add INTEL_IRQ_REG_RESET
drm/i915: change how VLV_IIR is reset
drm/i915: port i965_irq_uninstall go INTEL_IRQ_REG_RESET drm/i915: really clear the IIR registers
drm/i915: add INTEL_IRQ_REG_INIT
drm/i915: use INTEL_IRQ_REG_INIT on VLV too
drm/i915: reset the IIR registers at preinstall drm/i915: WARN if IIR is not zero at irq_postinstall drm/i915: remove additional zerogin of VLV_IIR at postinstall drm/i915: remove extra clearing of GTIIR from VLV irq preinstall drm/i915: add INTEL_IRQ_REG_RESET16
drm/i915: really clear the IIR registers on i8xx drm/i915: add INTEL_IRQ_REG_INIT16
drm/i915: reset the i8xx IIR registers at preinstall drm/i915: WARN if IIR is not zero at i8xx irq_postinstall

 drivers/gpu/drm/i915/i915_irq.c | 144 +++++++++++++++++-----------------------
 1 file changed, 62 insertions(+), 82 deletions(-)
Quoted 7 lines

--
1.8.1.2

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Signature · 1 line
Paulo Zanoni
#27 lore raw
Quoted 7 lines · Paulo Zanoni
Quoted 6 lines

My first question was why you didn't use name##IMR, name##IER, name##IIR in the macros, since that would match what we have in -internal and imo would look more natural. But then I've noticed that for IIR, IER & IMR we don't have a prefix ... I still think the I in the name looks a bit funny though (e.g. GTI). Maybe we should add an I915_ prefix then we could remove the I.

Quoted 7 lines

Yeah, you guessed the reason. If we use name##IMR then we could replace "INTEL_IRQ_REG_INIT(I, false, enable_mask, dev_priv->irq_mask);" with "INTEL_IRQ_REG_INIT(, false, enable_mask, dev_priv->irq_mask);", but I guess your suggestion of renaming is probably better. The problem with renaming to I915_IIR is that the registers also exists on i8xx, so I'd vote for another name (GEN2_IIR?).

Hm, I've have thought that an empty macro param would trip up the cpp. But for sure it looks funny. I guess we can do the renaming later on, once a bit of -internal has landed.

Quoted 10 lines
Quoted 4 lines

The other thing I'm unclear about is the do_iir flag, which in the end seems to clear IIR bits in uninstall but not in preinstall. Iirc Ben's proposal was to do the double IIR clear in preinstall, and I tend to agree with him. Can you please elaborate on this?

Please look at the final result of the series, we remove the flag on later patches (you just made me notice I forgot to remove it on the reg_init functions, but we don't really use it anymore). At the end of the series we do the double-IIR clear at both preinstall and uninstall. I wanted to keep one functional change at each patch.

Yeah, killing do_iir would be good at the end of this series, maybe together with the kerneldoc patch?

Quoted 5 lines
Quoted 3 lines

Maybe one patch on top to add some kerneldoc to the new magic macros would be good so that we can explain once what the thinking between the sequence is.

Will do.

Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch