ILK+ interrupt improvements

PATCH In review Paulo Zanoni 3 participants 24 messages Jan 22, 2014 – Jan 29, 2014

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Paulo Zanoni

author

2014-01-22 19:53 UTC #0 lore raw

cover ILK+ interrupt improvements

From: Paulo Zanoni <[email protected]>

Back in July 2013 I sent an email asking about interrupts and suggesting ways to improve our code [0]. Based on those discussions, I submitted a patch series proposing some changes [1]. I received some reviews and the general idea was accepted, but due to priority changes I couldn't update the series and resend it.

After this, BDW code got merged and, with it, some BDW-specific IRQ macros that do basically what I had proposed earlier. So this series tries to reuse those BDW macros on ILK+ code.

I think this series is an improvement because now, if we want to change how we treat interrupts, we just need to change the macro and everything gets automatically updated. Also, our code is now smaller :)

In some of the patches I had to decide between adding POSTING_READ calls to the macros or leave them on the callers. Adding the calls to the macros means we'll call them more times than what is strictly necessary. Not doing that means we may forget some POSTING_READ calls at some point. I chose to follow the BDW example and leave the task of doing POSTING_READ outside the macros. If we don't think this is the best way, I can change it, no problem.

I also considered doing the exact same changes to the previous Gens, but I don't have hardware to test them, and there's a potential to use some different macros due to PIPESTAT handling. So adding Gen2+ support is left as an exercise to developers from the future :)

Thanks,
Paulo

[0]: [Intel-gfx] proposals/questions about the IRQ registers

     http://lists.freedesktop.org/archives/intel-gfx/2013-July/029807.html
[1]: [Intel-gfx] [PATCH 00/15] Unify interrupt register init/reset
     http://lists.freedesktop.org/archives/intel-gfx/2013-July/030556.html

*** BLURB HERE ***

Paulo Zanoni (19):
drm/i915: add GEN5_IRQ_INIT macro
drm/i915: also use GEN5_IRQ_INIT with south display interrupts drm/i915: use GEN8_IRQ_INIT on GEN5
drm/i915: add GEN5_IRQ_FINI
drm/i915: don't forget to uninstall the PM IRQs drm/i915: properly clear IIR at irq_uninstall on Gen5+ drm/i915: add GEN5_IRQ_INIT
drm/i915: check if IIR is still zero at postinstall on Gen5+ drm/i915: fix SERR_INT init/reset code
drm/i915: fix GEN7_ERR_INT init/reset code
drm/i915: fix open coded gen5_gt_irq_preinstall drm/i915: extract ibx_irq_uninstall
drm/i915: call ibx_irq_uninstall from gen8_irq_uninstall drm/i915: enable SDEIER later
drm/i915: remove ibx_irq_uninstall
drm/i915: add missing intel_hpd_irq_uninstall drm/i915: add ironlake_irq_reset
drm/i915: add gen8_irq_reset
drm/i915: only enable HWSTAM interrupts on postinstall on ILK+

 drivers/gpu/drm/i915/i915_irq.c | 270 ++++++++++++++++++----------------------
 1 file changed, 124 insertions(+), 146 deletions(-)

Signature · 1 line

1.8.4.2

Paulo Zanoni

author

2014-01-22 19:53 UTC #1 lore raw

PATCH 1/19 drm/i915: add GEN5_IRQ_INIT macro

From: Paulo Zanoni <[email protected]>

The goal is to reuse the GEN8 macros, but a few changes are needed, so let's make things easier to review.

I could also use these macros on older code, but since I plan to change how the interrupts are initialized, we'll risk breaking the older code in the next commits, so I'll leave this out for now.

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

---
 drivers/gpu/drm/i915/i915_irq.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +10 ~~-14~~

index 01a8686..9838296 100644
@@ -80,6 +80,12 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
+#define GEN5_IRQ_INIT(type) do { \
+	I915_WRITE(type##IMR, 0xffffffff); \
+	I915_WRITE(type##IER, 0); \
+	POSTING_READ(type##IER); \
+} while (0)
+
 /* For display hotplug interrupt */
 static void
 ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)

@@ -2613,17 +2619,9 @@ 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);
-
-	if (INTEL_INFO(dev)->gen >= 6) {
-		/* and PM */
-		I915_WRITE(GEN6_PMIMR, 0xffffffff);
-		I915_WRITE(GEN6_PMIER, 0x0);
-		POSTING_READ(GEN6_PMIER);
-	}
+	GEN5_IRQ_INIT(GT);
+	if (INTEL_INFO(dev)->gen >= 6)
+		GEN5_IRQ_INIT(GEN6_PM);
 }
 
 /* drm_dma.h hooks

@@ -2634,9 +2632,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);
+	GEN5_IRQ_INIT(DE);
 
 	gen5_gt_irq_preinstall(dev);

Signature · 1 line

1.8.4.2

Paulo Zanoni

author

2014-01-22 19:53 UTC #2 lore raw

PATCH 2/19 drm/i915: also use GEN5_IRQ_INIT with south display interrupts

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

This interrupt gets initialized with a different IER value, so it was not using the macro. The problem is that we plan to modify the macro to make it do additional things, and we want the SDE interrupts updated too. So let's make sure we call the macro, then, after it, we do the necessary SDE-specific changes.

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 9838296..8bbaa3c 100644
@@ -2603,8 +2603,7 @@ static void ibx_irq_preinstall(struct drm_device *dev)
 	if (HAS_PCH_NOP(dev))
 		return;
 
-	/* south display irq */
-	I915_WRITE(SDEIMR, 0xffffffff);
+	GEN5_IRQ_INIT(SDE);
 	/*
 	 * SDEIER is also touched by the interrupt handler to work around missed
 	 * PCH interrupts. Hence we can't update it after the interrupt handler

Signature · 1 line

1.8.4.2

Paulo Zanoni

author

2014-01-22 19:53 UTC #3 lore raw

PATCH 3/19 drm/i915: use GEN8_IRQ_INIT on GEN5

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

And rename is to GEN5_IRQ_INIT.

We have discussed doing equivalent changes on July 2013, and I even sent a patch series for this: "[PATCH 00/15] Unify interrupt register init/reset". Now that the BDW code was merged, I have one more argument in favor of these changes.

Here's what really changes with the Gen 5 IRQ init code:
- We now clear the IIR registers at preinstall (they are also cleared at postinstall, but we will change that later).
- We have an additional POSTING_READ at the IMR register.

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

---
 drivers/gpu/drm/i915/i915_irq.c | 49 +++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 26 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +23 ~~-26~~

index 8bbaa3c..8fdbb5b 100644
@@ -80,12 +80,30 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
+/*
+ * IIR can theoretically queue up two events. Be paranoid.
+ * Also, make sure callers of these macros have something equivalent to a
+ * POSTING_READ on the IIR register.
+ * */
+#define GEN8_IRQ_INIT_NDX(type, which) do { \
+	I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \
+	POSTING_READ(GEN8_##type##_IMR(which)); \
+	I915_WRITE(GEN8_##type##_IER(which), 0); \
+	I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
+	POSTING_READ(GEN8_##type##_IIR(which)); \
+	I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
+} while (0)
+
 #define GEN5_IRQ_INIT(type) do { \
 	I915_WRITE(type##IMR, 0xffffffff); \
+	POSTING_READ(type##IMR); \
 	I915_WRITE(type##IER, 0); \
-	POSTING_READ(type##IER); \
+	I915_WRITE(type##IIR, 0xffffffff); \
+	POSTING_READ(type##IIR); \
+	I915_WRITE(type##IIR, 0xffffffff); \
 } while (0)
 
+
 /* For display hotplug interrupt */
 static void
 ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)

@@ -2621,6 +2639,7 @@ static void gen5_gt_irq_preinstall(struct drm_device *dev)
 	GEN5_IRQ_INIT(GT);
 	if (INTEL_INFO(dev)->gen >= 6)
 		GEN5_IRQ_INIT(GEN6_PM);
+	POSTING_READ(GTIIR);
 }
 
 /* drm_dma.h hooks

@@ -2675,25 +2694,6 @@ static void gen8_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(GEN8_MASTER_IRQ, 0);
 	POSTING_READ(GEN8_MASTER_IRQ);
 
-	/* IIR can theoretically queue up two events. Be paranoid */
-#define GEN8_IRQ_INIT_NDX(type, which) do { \
-		I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \
-		POSTING_READ(GEN8_##type##_IMR(which)); \
-		I915_WRITE(GEN8_##type##_IER(which), 0); \
-		I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
-		POSTING_READ(GEN8_##type##_IIR(which)); \
-		I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
-	} while (0)
-
-#define GEN8_IRQ_INIT(type) do { \
-		I915_WRITE(GEN8_##type##_IMR, 0xffffffff); \
-		POSTING_READ(GEN8_##type##_IMR); \
-		I915_WRITE(GEN8_##type##_IER, 0); \
-		I915_WRITE(GEN8_##type##_IIR, 0xffffffff); \
-		POSTING_READ(GEN8_##type##_IIR); \
-		I915_WRITE(GEN8_##type##_IIR, 0xffffffff); \
-	} while (0)
-
 	GEN8_IRQ_INIT_NDX(GT, 0);
 	GEN8_IRQ_INIT_NDX(GT, 1);
 	GEN8_IRQ_INIT_NDX(GT, 2);

@@ -2703,12 +2703,9 @@ static void gen8_irq_preinstall(struct drm_device *dev)
 		GEN8_IRQ_INIT_NDX(DE_PIPE, pipe);
 	}
 
-	GEN8_IRQ_INIT(DE_PORT);
-	GEN8_IRQ_INIT(DE_MISC);
-	GEN8_IRQ_INIT(PCU);
-#undef GEN8_IRQ_INIT
-#undef GEN8_IRQ_INIT_NDX
-
+	GEN5_IRQ_INIT(GEN8_DE_PORT_);
+	GEN5_IRQ_INIT(GEN8_DE_MISC_);
+	GEN5_IRQ_INIT(GEN8_PCU_);
 	POSTING_READ(GEN8_PCU_IIR);
 
 	ibx_irq_preinstall(dev);

Signature · 1 line

1.8.4.2

Paulo Zanoni

author

2014-01-22 19:53 UTC #4 lore raw

PATCH 4/19 drm/i915: add GEN5_IRQ_FINI

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Same as the _INIT macro: the goal is to reuse the GEN8 macros, but there are still some slight differences.

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

---
 drivers/gpu/drm/i915/i915_irq.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

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

index 8fdbb5b..d58c392 100644
@@ -103,6 +103,11 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	I915_WRITE(type##IIR, 0xffffffff); \
 } while (0)
 
+#define GEN5_IRQ_FINI(type) do { \
+	I915_WRITE(type##IMR, 0xffffffff); \
+	I915_WRITE(type##IER, 0); \
+	I915_WRITE(type##IIR, I915_READ(type##IIR)); \
+} while (0)
 
 /* For display hotplug interrupt */
 static void

@@ -3060,22 +3065,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));
+	GEN5_IRQ_FINI(DE);
 	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));
+	GEN5_IRQ_FINI(GT);
 
 	if (HAS_PCH_NOP(dev))
 		return;
 
-	I915_WRITE(SDEIMR, 0xffffffff);
-	I915_WRITE(SDEIER, 0x0);
-	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
+	GEN5_IRQ_FINI(SDE);
 	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
 		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
 }

Signature · 1 line

1.8.4.2

Paulo Zanoni

author

2014-01-22 19:53 UTC #5 lore raw

PATCH 5/19 drm/i915: don't forget to uninstall the PM IRQs

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

We basically never test the uninstall path today, that's why this works.

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

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

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

index d58c392..65dec2f 100644
@@ -3070,6 +3070,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
 
 	GEN5_IRQ_FINI(GT);
+	if (INTEL_INFO(dev)->gen >= 6)
+		GEN5_IRQ_FINI(GEN6_PM);
 
 	if (HAS_PCH_NOP(dev))
 		return;

Signature · 1 line

1.8.4.2

Paulo Zanoni

author

2014-01-22 19:53 UTC #6 lore raw

PATCH 6/19 drm/i915: properly clear IIR at irq_uninstall on Gen5+

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

The IRQ_INIT and IRQ_FINI macros are basically the same thing, with the exception that IRQ_FINI doesn't properly clear IIR twice and doesn't have as many POSTING_READs as IRQ_INIT. So rename the macro to IRQ_RESET and use it everywhere.

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

---
 drivers/gpu/drm/i915/i915_irq.c | 76 +++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 48 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +28 ~~-48~~

index 65dec2f..b77cbb8 100644
@@ -85,7 +85,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
  * Also, make sure callers of these macros have something equivalent to a
  * POSTING_READ on the IIR register.
  * */
-#define GEN8_IRQ_INIT_NDX(type, which) do { \
+#define GEN8_IRQ_RESET_NDX(type, which) do { \
 	I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \
 	POSTING_READ(GEN8_##type##_IMR(which)); \
 	I915_WRITE(GEN8_##type##_IER(which), 0); \

@@ -94,7 +94,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
 } while (0)
 
-#define GEN5_IRQ_INIT(type) do { \
+#define GEN5_IRQ_RESET(type) do { \
 	I915_WRITE(type##IMR, 0xffffffff); \
 	POSTING_READ(type##IMR); \
 	I915_WRITE(type##IER, 0); \

@@ -103,12 +103,6 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	I915_WRITE(type##IIR, 0xffffffff); \
 } while (0)
 
-#define GEN5_IRQ_FINI(type) do { \
-	I915_WRITE(type##IMR, 0xffffffff); \
-	I915_WRITE(type##IER, 0); \
-	I915_WRITE(type##IIR, I915_READ(type##IIR)); \
-} while (0)
-
 /* For display hotplug interrupt */
 static void
 ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)

@@ -2626,7 +2620,7 @@ static void ibx_irq_preinstall(struct drm_device *dev)
 	if (HAS_PCH_NOP(dev))
 		return;
 
-	GEN5_IRQ_INIT(SDE);
+	GEN5_IRQ_RESET(SDE);
 	/*
 	 * SDEIER is also touched by the interrupt handler to work around missed
 	 * PCH interrupts. Hence we can't update it after the interrupt handler

@@ -2641,9 +2635,9 @@ static void gen5_gt_irq_preinstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	GEN5_IRQ_INIT(GT);
+	GEN5_IRQ_RESET(GT);
 	if (INTEL_INFO(dev)->gen >= 6)
-		GEN5_IRQ_INIT(GEN6_PM);
+		GEN5_IRQ_RESET(GEN6_PM);
 	POSTING_READ(GTIIR);
 }

@@ -2655,7 +2649,7 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 
 	I915_WRITE(HWSTAM, 0xeffe);
 
-	GEN5_IRQ_INIT(DE);
+	GEN5_IRQ_RESET(DE);
 
 	gen5_gt_irq_preinstall(dev);

@@ -2699,18 +2693,18 @@ static void gen8_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(GEN8_MASTER_IRQ, 0);
 	POSTING_READ(GEN8_MASTER_IRQ);
 
-	GEN8_IRQ_INIT_NDX(GT, 0);
-	GEN8_IRQ_INIT_NDX(GT, 1);
-	GEN8_IRQ_INIT_NDX(GT, 2);
-	GEN8_IRQ_INIT_NDX(GT, 3);
+	GEN8_IRQ_RESET_NDX(GT, 0);
+	GEN8_IRQ_RESET_NDX(GT, 1);
+	GEN8_IRQ_RESET_NDX(GT, 2);
+	GEN8_IRQ_RESET_NDX(GT, 3);
 
 	for_each_pipe(pipe) {
-		GEN8_IRQ_INIT_NDX(DE_PIPE, pipe);
+		GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
 	}
 
-	GEN5_IRQ_INIT(GEN8_DE_PORT_);
-	GEN5_IRQ_INIT(GEN8_DE_MISC_);
-	GEN5_IRQ_INIT(GEN8_PCU_);
+	GEN5_IRQ_RESET(GEN8_DE_PORT_);
+	GEN5_IRQ_RESET(GEN8_DE_MISC_);
+	GEN5_IRQ_RESET(GEN8_PCU_);
 	POSTING_READ(GEN8_PCU_IIR);
 
 	ibx_irq_preinstall(dev);

@@ -3000,32 +2994,17 @@ static void gen8_irq_uninstall(struct drm_device *dev)
 
 	I915_WRITE(GEN8_MASTER_IRQ, 0);
 
-#define GEN8_IRQ_FINI_NDX(type, which) do { \
-		I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \
-		I915_WRITE(GEN8_##type##_IER(which), 0); \
-		I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
-	} while (0)
-
-#define GEN8_IRQ_FINI(type) do { \
-		I915_WRITE(GEN8_##type##_IMR, 0xffffffff); \
-		I915_WRITE(GEN8_##type##_IER, 0); \
-		I915_WRITE(GEN8_##type##_IIR, 0xffffffff); \
-	} while (0)
+	GEN8_IRQ_RESET_NDX(GT, 0);
+	GEN8_IRQ_RESET_NDX(GT, 1);
+	GEN8_IRQ_RESET_NDX(GT, 2);
+	GEN8_IRQ_RESET_NDX(GT, 3);
 
-	GEN8_IRQ_FINI_NDX(GT, 0);
-	GEN8_IRQ_FINI_NDX(GT, 1);
-	GEN8_IRQ_FINI_NDX(GT, 2);
-	GEN8_IRQ_FINI_NDX(GT, 3);
-
-	for_each_pipe(pipe) {
-		GEN8_IRQ_FINI_NDX(DE_PIPE, pipe);
-	}
+	for_each_pipe(pipe)
+		GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
 
-	GEN8_IRQ_FINI(DE_PORT);
-	GEN8_IRQ_FINI(DE_MISC);
-	GEN8_IRQ_FINI(PCU);
-#undef GEN8_IRQ_FINI
-#undef GEN8_IRQ_FINI_NDX
+	GEN5_IRQ_RESET(GEN8_DE_PORT_);
+	GEN5_IRQ_RESET(GEN8_DE_MISC_);
+	GEN5_IRQ_RESET(GEN8_PCU_);
 
 	POSTING_READ(GEN8_PCU_IIR);
 }

@@ -3065,18 +3044,19 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	I915_WRITE(HWSTAM, 0xffffffff);
 
-	GEN5_IRQ_FINI(DE);
+	GEN5_IRQ_RESET(DE);
 	if (IS_GEN7(dev))
 		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
 
-	GEN5_IRQ_FINI(GT);
+	GEN5_IRQ_RESET(GT);
 	if (INTEL_INFO(dev)->gen >= 6)
-		GEN5_IRQ_FINI(GEN6_PM);
+		GEN5_IRQ_RESET(GEN6_PM);
+	POSTING_READ(GTIIR);
 
 	if (HAS_PCH_NOP(dev))
 		return;
 
-	GEN5_IRQ_FINI(SDE);
+	GEN5_IRQ_RESET(SDE);
 	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
 		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
 }

Signature · 1 line

1.8.4.2

Paulo Zanoni

author

2014-01-22 19:53 UTC #7 lore raw

PATCH 7/19 drm/i915: add GEN5_IRQ_INIT

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

And the equivalent GEN8_IRQ_INIT_NDX macro. These macros are for the postinstall functions. The next patch will improve this macro.

Notice that I could have included POSTING_READ calls to the macro, but that would mean the code would do a few more POSTING_READs than necessary. OTOH it would be more fail-proof. I can change that if needed.

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

---
 drivers/gpu/drm/i915/i915_irq.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +18 ~~-15~~

index b77cbb8..095baa7 100644
@@ -103,6 +103,16 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	I915_WRITE(type##IIR, 0xffffffff); \
 } while (0)
 
+#define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) do { \
+	I915_WRITE(GEN8_##type##_IMR(which), (imr_val)); \
+	I915_WRITE(GEN8_##type##_IER(which), (ier_val)); \
+} while (0)
+
+#define GEN5_IRQ_INIT(type, imr_val, ier_val) do { \
+	I915_WRITE(type##IMR, (imr_val)); \
+	I915_WRITE(type##IER, (ier_val)); \
+} while (0)
+
 /* For display hotplug interrupt */
 static void
 ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)

@@ -2789,9 +2799,7 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
 	}
 
 	I915_WRITE(GTIIR, I915_READ(GTIIR));
-	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
-	I915_WRITE(GTIER, gt_irqs);
-	POSTING_READ(GTIER);
+	GEN5_IRQ_INIT(GT, dev_priv->gt_irq_mask, gt_irqs);
 
 	if (INTEL_INFO(dev)->gen >= 6) {
 		pm_irqs |= GEN6_PM_RPS_EVENTS;

@@ -2801,10 +2809,9 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
 
 		dev_priv->pm_irq_mask = 0xffffffff;
 		I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
-		I915_WRITE(GEN6_PMIMR, dev_priv->pm_irq_mask);
-		I915_WRITE(GEN6_PMIER, pm_irqs);
-		POSTING_READ(GEN6_PMIER);
+		GEN5_IRQ_INIT(GEN6_PM, dev_priv->pm_irq_mask, pm_irqs);
 	}
+	POSTING_READ(GTIER);
 }
 
 static int ironlake_irq_postinstall(struct drm_device *dev)

@@ -2837,9 +2844,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 
 	/* 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);
+	GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);
 
 	gen5_gt_irq_postinstall(dev);

@@ -2935,8 +2940,7 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
 		if (tmp)
 			DRM_ERROR("Interrupt (%d) should have been masked in pre-install 0x%08x\n",
 				  i, tmp);
-		I915_WRITE(GEN8_GT_IMR(i), ~gt_interrupts[i]);
-		I915_WRITE(GEN8_GT_IER(i), gt_interrupts[i]);
+		GEN8_IRQ_INIT_NDX(GT, i, ~gt_interrupts[i], gt_interrupts[i]);
 	}
 	POSTING_READ(GEN8_GT_IER(0));
 }

@@ -2959,13 +2963,12 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
 		if (tmp)
 			DRM_ERROR("Interrupt (%d) should have been masked in pre-install 0x%08x\n",
 				  pipe, tmp);
-		I915_WRITE(GEN8_DE_PIPE_IMR(pipe), dev_priv->de_irq_mask[pipe]);
-		I915_WRITE(GEN8_DE_PIPE_IER(pipe), de_pipe_enables);
+		GEN8_IRQ_INIT_NDX(DE_PIPE, pipe, dev_priv->de_irq_mask[pipe],
+				  de_pipe_enables);
 	}
 	POSTING_READ(GEN8_DE_PIPE_ISR(0));
 
-	I915_WRITE(GEN8_DE_PORT_IMR, ~GEN8_AUX_CHANNEL_A);
-	I915_WRITE(GEN8_DE_PORT_IER, GEN8_AUX_CHANNEL_A);
+	GEN5_IRQ_INIT(GEN8_DE_PORT_, ~GEN8_AUX_CHANNEL_A, GEN8_AUX_CHANNEL_A);
 	POSTING_READ(GEN8_DE_PORT_IER);
 }

Signature · 1 line

1.8.4.2

Paulo Zanoni

author

2014-01-22 19:53 UTC #8 lore raw

PATCH 8/19 drm/i915: check if IIR is still zero at postinstall on Gen5+

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Instead of trying to clear it again. It should already be masked and disabled and zeroed at preinstall/uninstall.

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

---
 drivers/gpu/drm/i915/i915_irq.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +15 ~~-17~~

index 095baa7..fb6ebd1 100644
@@ -103,12 +103,24 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	I915_WRITE(type##IIR, 0xffffffff); \
 } while (0)
 
+/*
+ * We should clear IMR at preinstall/uninstall, and just check at postinstall.
+ */
+#define GEN5_ASSERT_IIR_IS_ZERO(reg) do { \
+	u32 val = I915_READ(reg); \
+	if (val) \
+		DRM_ERROR("Interrupt register 0x%x is not zero: 0x%08x\n", \
+			  (reg), val); \
+} while (0)
+
 #define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) do { \
+	GEN5_ASSERT_IIR_IS_ZERO(GEN8_##type##_IIR(which)); \
 	I915_WRITE(GEN8_##type##_IMR(which), (imr_val)); \
 	I915_WRITE(GEN8_##type##_IER(which), (ier_val)); \
 } while (0)
 
 #define GEN5_IRQ_INIT(type, imr_val, ier_val) do { \
+	GEN5_ASSERT_IIR_IS_ZERO(type##IIR); \
 	I915_WRITE(type##IMR, (imr_val)); \
 	I915_WRITE(type##IER, (ier_val)); \
 } while (0)

@@ -2772,7 +2784,7 @@ static void ibx_irq_postinstall(struct drm_device *dev)
 		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
 	}
 
-	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
+	GEN5_ASSERT_IIR_IS_ZERO(SDEIIR);
 	I915_WRITE(SDEIMR, ~mask);
 }

@@ -2798,7 +2810,6 @@ 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));
 	GEN5_IRQ_INIT(GT, dev_priv->gt_irq_mask, gt_irqs);
 
 	if (INTEL_INFO(dev)->gen >= 6) {

@@ -2808,7 +2819,6 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
 			pm_irqs |= PM_VEBOX_USER_INTERRUPT;
 
 		dev_priv->pm_irq_mask = 0xffffffff;
-		I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
 		GEN5_IRQ_INIT(GEN6_PM, dev_priv->pm_irq_mask, pm_irqs);
 	}
 	POSTING_READ(GTIER);

@@ -2842,8 +2852,6 @@ 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));
 	GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);
 
 	gen5_gt_irq_postinstall(dev);

@@ -2935,13 +2943,8 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
 		GT_RENDER_USER_INTERRUPT << GEN8_VECS_IRQ_SHIFT
 		};
 
-	for (i = 0; i < ARRAY_SIZE(gt_interrupts); i++) {
-		u32 tmp = I915_READ(GEN8_GT_IIR(i));
-		if (tmp)
-			DRM_ERROR("Interrupt (%d) should have been masked in pre-install 0x%08x\n",
-				  i, tmp);
+	for (i = 0; i < ARRAY_SIZE(gt_interrupts); i++)
 		GEN8_IRQ_INIT_NDX(GT, i, ~gt_interrupts[i], gt_interrupts[i]);
-	}
 	POSTING_READ(GEN8_GT_IER(0));
 }

@@ -2958,14 +2961,9 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
 	dev_priv->de_irq_mask[PIPE_B] = ~de_pipe_masked;
 	dev_priv->de_irq_mask[PIPE_C] = ~de_pipe_masked;
 
-	for_each_pipe(pipe) {
-		u32 tmp = I915_READ(GEN8_DE_PIPE_IIR(pipe));
-		if (tmp)
-			DRM_ERROR("Interrupt (%d) should have been masked in pre-install 0x%08x\n",
-				  pipe, tmp);
+	for_each_pipe(pipe)
 		GEN8_IRQ_INIT_NDX(DE_PIPE, pipe, dev_priv->de_irq_mask[pipe],
 				  de_pipe_enables);
-	}
 	POSTING_READ(GEN8_DE_PIPE_ISR(0));
 
 	GEN5_IRQ_INIT(GEN8_DE_PORT_, ~GEN8_AUX_CHANNEL_A, GEN8_AUX_CHANNEL_A);

Signature · 1 line

1.8.4.2

Paulo Zanoni

author

2014-01-22 19:53 UTC #9 lore raw

PATCH 9/19 drm/i915: fix SERR_INT init/reset code

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

The SERR_INT register is very similar to the other IIR registers, so let's zero it at preinstall/uninstall and WARN for a non-zero value at postinstall, just like we do with the other IIR registers. For this one, there's no need to double-clear since it can't store more than one interrupt.

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

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

drivers/gpu/drm/i915/i915_irq.c +6 -2

index fb6ebd1..5b60de5 100644
@@ -2643,6 +2643,10 @@ static void ibx_irq_preinstall(struct drm_device *dev)
 		return;
 
 	GEN5_IRQ_RESET(SDE);
+
+	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
+		I915_WRITE(SERR_INT, 0xffffffff);
+
 	/*
 	 * SDEIER is also touched by the interrupt handler to work around missed
 	 * PCH interrupts. Hence we can't update it after the interrupt handler

@@ -2781,7 +2785,7 @@ static void ibx_irq_postinstall(struct drm_device *dev)
 	} else {
 		mask = SDE_GMBUS_CPT | SDE_AUX_MASK_CPT | SDE_ERROR_CPT;
 
-		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
+		GEN5_ASSERT_IIR_IS_ZERO(SERR_INT);
 	}
 
 	GEN5_ASSERT_IIR_IS_ZERO(SDEIIR);

@@ -3059,7 +3063,7 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	GEN5_IRQ_RESET(SDE);
 	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
-		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
+		I915_WRITE(SERR_INT, 0xffffffff);
 }
 
 static void i8xx_irq_preinstall(struct drm_device * dev)

Signature · 1 line

1.8.4.2

Paulo Zanoni

author

2014-01-22 19:53 UTC #10 lore raw

PATCH 10/19 drm/i915: fix GEN7_ERR_INT init/reset code

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Same as SERR_INT and the other IIR registers: reset on preinstall/uninstall and WARN for non-zero values at postinstall. This one also doesn't need double-clear.

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

---
 drivers/gpu/drm/i915/i915_irq.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +5 -2

index 5b60de5..9a323a9 100644
@@ -2677,6 +2677,9 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 
 	GEN5_IRQ_RESET(DE);
 
+	if (IS_GEN7(dev))
+		I915_WRITE(GEN7_ERR_INT, 0xffffffff);
+
 	gen5_gt_irq_preinstall(dev);
 
 	ibx_irq_preinstall(dev);

@@ -2843,7 +2846,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 		extra_mask = (DE_PIPEC_VBLANK_IVB | DE_PIPEB_VBLANK_IVB |
 			      DE_PIPEA_VBLANK_IVB);
 
-		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
+		GEN5_ASSERT_IIR_IS_ZERO(GEN7_ERR_INT);
 	} else {
 		display_mask = (DE_MASTER_IRQ_CONTROL | DE_GSE | DE_PCH_EVENT |
 				DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE |

@@ -3051,7 +3054,7 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	GEN5_IRQ_RESET(DE);
 	if (IS_GEN7(dev))
-		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
+		I915_WRITE(GEN7_ERR_INT, 0xffffffff);
 
 	GEN5_IRQ_RESET(GT);
 	if (INTEL_INFO(dev)->gen >= 6)

Signature · 1 line

1.8.4.2

Paulo Zanoni

author

2014-01-22 19:53 UTC #11 lore raw

PATCH 11/19 drm/i915: fix open coded gen5_gt_irq_preinstall

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

The duplicate was at an _uninstall function, so rename it to gen5_gt_irq_reset.

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

---
 drivers/gpu/drm/i915/i915_irq.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

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

index 9a323a9..175d592 100644
@@ -2657,7 +2657,7 @@ static void ibx_irq_preinstall(struct drm_device *dev)
 	POSTING_READ(SDEIER);
 }
 
-static void gen5_gt_irq_preinstall(struct drm_device *dev)
+static void gen5_gt_irq_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;

@@ -2680,7 +2680,7 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 	if (IS_GEN7(dev))
 		I915_WRITE(GEN7_ERR_INT, 0xffffffff);
 
-	gen5_gt_irq_preinstall(dev);
+	gen5_gt_irq_reset(dev);
 
 	ibx_irq_preinstall(dev);
 }

@@ -2700,7 +2700,7 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(GTIIR, I915_READ(GTIIR));
 	I915_WRITE(GTIIR, I915_READ(GTIIR));
 
-	gen5_gt_irq_preinstall(dev);
+	gen5_gt_irq_reset(dev);
 
 	I915_WRITE(DPINVGTT, 0xff);

@@ -3056,10 +3056,7 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 	if (IS_GEN7(dev))
 		I915_WRITE(GEN7_ERR_INT, 0xffffffff);
 
-	GEN5_IRQ_RESET(GT);
-	if (INTEL_INFO(dev)->gen >= 6)
-		GEN5_IRQ_RESET(GEN6_PM);
-	POSTING_READ(GTIIR);
+	gen5_gt_irq_reset(dev);
 
 	if (HAS_PCH_NOP(dev))
 		return;

Signature · 1 line

1.8.4.2

Paulo Zanoni

author

2014-01-22 19:53 UTC #12 lore raw

PATCH 12/19 drm/i915: extract ibx_irq_uninstall

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Just like ibx_irq_preinstall. We'll call this from somewhere else in the next patch.

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

---
 drivers/gpu/drm/i915/i915_irq.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

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

index 175d592..128dbbc 100644
@@ -2676,7 +2676,6 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(HWSTAM, 0xeffe);
 
 	GEN5_IRQ_RESET(DE);
-
 	if (IS_GEN7(dev))
 		I915_WRITE(GEN7_ERR_INT, 0xffffffff);

@@ -2992,6 +2991,19 @@ static int gen8_irq_postinstall(struct drm_device *dev)
 	return 0;
 }
 
+static void ibx_irq_uninstall(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (HAS_PCH_NOP(dev))
+		return;
+
+	GEN5_IRQ_RESET(SDE);
+
+	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
+		I915_WRITE(SERR_INT, 0xffffffff);
+}
+
 static void gen8_irq_uninstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;

@@ -3058,12 +3070,7 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	gen5_gt_irq_reset(dev);
 
-	if (HAS_PCH_NOP(dev))
-		return;
-
-	GEN5_IRQ_RESET(SDE);
-	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
-		I915_WRITE(SERR_INT, 0xffffffff);
+	ibx_irq_uninstall(dev);
 }
 
 static void i8xx_irq_preinstall(struct drm_device * dev)

Signature · 1 line

1.8.4.2

Paulo Zanoni

author

2014-01-22 19:53 UTC #13 lore raw

PATCH 13/19 drm/i915: call ibx_irq_uninstall from gen8_irq_uninstall

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

After all, we call ibx_irq_preinstall from gen8_irq_preinstall.

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

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

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

index 128dbbc..60f4c46 100644
@@ -3027,6 +3027,8 @@ static void gen8_irq_uninstall(struct drm_device *dev)
 	GEN5_IRQ_RESET(GEN8_PCU_);
 
 	POSTING_READ(GEN8_PCU_IIR);
+
+	ibx_irq_uninstall(dev);
 }
 
 static void valleyview_irq_uninstall(struct drm_device *dev)

Signature · 1 line

1.8.4.2

Paulo Zanoni

author

2014-01-22 19:53 UTC #14 lore raw

PATCH 14/19 drm/i915: enable SDEIER later

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

On the preinstall stage we should just disable all the interrupts, but we currently enable all the south display interrupts due to the way we touch SDEIER at the IRQ handlers (note: they are still masked and our IRQ handler is disabled). Instead of doing that, let's make the preinstall stage just disable all the south interrupts, and do the proper interrupt dance/ordering at the postinstall stage, including an assert to check if everything is behaving as expected.

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

---
 drivers/gpu/drm/i915/i915_irq.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +21 -6

index 60f4c46..ce6a8f3 100644
@@ -2646,13 +2646,24 @@ static void ibx_irq_preinstall(struct drm_device *dev)
 
 	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
 		I915_WRITE(SERR_INT, 0xffffffff);
+}
 
-	/*
-	 * SDEIER is also touched by the interrupt handler to work around missed
-	 * PCH interrupts. Hence we can't update it after the interrupt handler
-	 * is enabled - instead we unconditionally enable all PCH interrupt
-	 * sources here, but then only unmask them as needed with SDEIMR.
-	 */
+/*
+ * SDEIER is also touched by the interrupt handler to work around missed PCH
+ * interrupts. Hence we can't update it after the interrupt handler is enabled -
+ * instead we unconditionally enable all PCH interrupt sources here, but then
+ * only unmask them as needed with SDEIMR.
+ *
+ * This function needs to be called before interrupts are enabled.
+ */
+static void ibx_irq_pre_postinstall(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (HAS_PCH_NOP(dev))
+		return;
+
+	WARN_ON(I915_READ(SDEIER) != 0);
 	I915_WRITE(SDEIER, 0xffffffff);
 	POSTING_READ(SDEIER);
 }

@@ -2858,6 +2869,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 
 	dev_priv->irq_mask = ~display_mask;
 
+	ibx_irq_pre_postinstall(dev);
+
 	GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);
 
 	gen5_gt_irq_postinstall(dev);

@@ -2980,6 +2993,8 @@ static int gen8_irq_postinstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	ibx_irq_pre_postinstall(dev);
+
 	gen8_gt_irq_postinstall(dev_priv);
 	gen8_de_irq_postinstall(dev_priv);

Signature · 1 line

1.8.4.2

Paulo Zanoni

author

2014-01-22 19:53 UTC #15 lore raw

PATCH 15/19 drm/i915: remove ibx_irq_uninstall

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

After the latest changes, ibx_irq_preinstall and ibx_irq_uninstall are the same, so remove one of the copies and rename the other to ibx_irq_reset (since we're using the "reset" name for things which are called both at preinstall and uninstall).

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

---
 drivers/gpu/drm/i915/i915_irq.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +5 ~~-18~~

index ce6a8f3..5d8d262 100644
@@ -2635,7 +2635,7 @@ void i915_queue_hangcheck(struct drm_device *dev)
 		  round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES));
 }
 
-static void ibx_irq_preinstall(struct drm_device *dev)
+static void ibx_irq_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;

@@ -2692,7 +2692,7 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 
 	gen5_gt_irq_reset(dev);
 
-	ibx_irq_preinstall(dev);
+	ibx_irq_reset(dev);
 }
 
 static void valleyview_irq_preinstall(struct drm_device *dev)

@@ -2746,7 +2746,7 @@ static void gen8_irq_preinstall(struct drm_device *dev)
 	GEN5_IRQ_RESET(GEN8_PCU_);
 	POSTING_READ(GEN8_PCU_IIR);
 
-	ibx_irq_preinstall(dev);
+	ibx_irq_reset(dev);
 }
 
 static void ibx_hpd_irq_setup(struct drm_device *dev)

@@ -3006,19 +3006,6 @@ static int gen8_irq_postinstall(struct drm_device *dev)
 	return 0;
 }
 
-static void ibx_irq_uninstall(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	if (HAS_PCH_NOP(dev))
-		return;
-
-	GEN5_IRQ_RESET(SDE);
-
-	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
-		I915_WRITE(SERR_INT, 0xffffffff);
-}
-
 static void gen8_irq_uninstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;

@@ -3043,7 +3030,7 @@ static void gen8_irq_uninstall(struct drm_device *dev)
 
 	POSTING_READ(GEN8_PCU_IIR);
 
-	ibx_irq_uninstall(dev);
+	ibx_irq_reset(dev);
 }
 
 static void valleyview_irq_uninstall(struct drm_device *dev)

@@ -3087,7 +3074,7 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	gen5_gt_irq_reset(dev);
 
-	ibx_irq_uninstall(dev);
+	ibx_irq_reset(dev);
 }
 
 static void i8xx_irq_preinstall(struct drm_device * dev)

Signature · 1 line

1.8.4.2

Paulo Zanoni

author

2014-01-22 19:53 UTC #16 lore raw

PATCH 16/19 drm/i915: add missing intel_hpd_irq_uninstall

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Missing from gen8_irq_uninstall.

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

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

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

index 5d8d262..02594f9 100644
@@ -3014,6 +3014,8 @@ static void gen8_irq_uninstall(struct drm_device *dev)
 	if (!dev_priv)
 		return;
 
+	intel_hpd_irq_uninstall(dev_priv);
+
 	I915_WRITE(GEN8_MASTER_IRQ, 0);
 
 	GEN8_IRQ_RESET_NDX(GT, 0);

Signature · 1 line

1.8.4.2

Paulo Zanoni

author

2014-01-22 19:53 UTC #17 lore raw

PATCH 17/19 drm/i915: add ironlake_irq_reset

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

To merge the common code of ironlake_irq_preinstall and ironlake_irq_uninstall.

We should also probably do something about that HSWSTAM write on a later commit.

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

---
 drivers/gpu/drm/i915/i915_irq.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +12 ~~-11~~

index 02594f9..042bda5 100644
@@ -2680,11 +2680,9 @@ static void gen5_gt_irq_reset(struct drm_device *dev)
 
 /* drm_dma.h hooks
 */
-static void ironlake_irq_preinstall(struct drm_device *dev)
+static void ironlake_irq_reset(struct drm_device *dev)
 {
-	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
-
-	I915_WRITE(HWSTAM, 0xeffe);
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	GEN5_IRQ_RESET(DE);
 	if (IS_GEN7(dev))

@@ -2695,6 +2693,15 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 	ibx_irq_reset(dev);
 }
 
+static void ironlake_irq_preinstall(struct drm_device *dev)
+{
+	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
+
+	I915_WRITE(HWSTAM, 0xeffe);
+
+	ironlake_irq_reset(dev);
+}
+
 static void valleyview_irq_preinstall(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;

@@ -3070,13 +3077,7 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	I915_WRITE(HWSTAM, 0xffffffff);
 
-	GEN5_IRQ_RESET(DE);
-	if (IS_GEN7(dev))
-		I915_WRITE(GEN7_ERR_INT, 0xffffffff);
-
-	gen5_gt_irq_reset(dev);
-
-	ibx_irq_reset(dev);
+	ironlake_irq_reset(dev);
 }
 
 static void i8xx_irq_preinstall(struct drm_device * dev)

Signature · 1 line

1.8.4.2

Paulo Zanoni

author

2014-01-22 19:53 UTC #18 lore raw

PATCH 18/19 drm/i915: add gen8_irq_reset

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

So we can merge all the common code from postinstall and uninstall.

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

---
 drivers/gpu/drm/i915/i915_irq.c | 26 +++++++-------------------
 1 file changed, 7 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +7 ~~-19~~

index 042bda5..461c9a5 100644
@@ -2731,7 +2731,7 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	POSTING_READ(VLV_IER);
 }
 
-static void gen8_irq_preinstall(struct drm_device *dev)
+static void gen8_irq_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int pipe;

@@ -2756,6 +2756,11 @@ static void gen8_irq_preinstall(struct drm_device *dev)
 	ibx_irq_reset(dev);
 }
 
+static void gen8_irq_preinstall(struct drm_device *dev)
+{
+	gen8_irq_reset(dev);
+}
+
 static void ibx_hpd_irq_setup(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;

@@ -3016,30 +3021,13 @@ static int gen8_irq_postinstall(struct drm_device *dev)
 static void gen8_irq_uninstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int pipe;
 
 	if (!dev_priv)
 		return;
 
 	intel_hpd_irq_uninstall(dev_priv);
 
-	I915_WRITE(GEN8_MASTER_IRQ, 0);
-
-	GEN8_IRQ_RESET_NDX(GT, 0);
-	GEN8_IRQ_RESET_NDX(GT, 1);
-	GEN8_IRQ_RESET_NDX(GT, 2);
-	GEN8_IRQ_RESET_NDX(GT, 3);
-
-	for_each_pipe(pipe)
-		GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
-
-	GEN5_IRQ_RESET(GEN8_DE_PORT_);
-	GEN5_IRQ_RESET(GEN8_DE_MISC_);
-	GEN5_IRQ_RESET(GEN8_PCU_);
-
-	POSTING_READ(GEN8_PCU_IIR);
-
-	ibx_irq_reset(dev);
+	gen8_irq_reset(dev);
 }
 
 static void valleyview_irq_uninstall(struct drm_device *dev)

Signature · 1 line

1.8.4.2

Paulo Zanoni

author

2014-01-22 19:53 UTC #19 lore raw

PATCH 19/19 drm/i915: only enable HWSTAM interrupts on postinstall on ILK+

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

We should only enable interrupts at postinstall.

And now on ILK/SNB/IVB/HSW the irq_preinstall and irq_postinstall functions leave the hardware in the same state.

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

---
 drivers/gpu/drm/i915/i915_irq.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +4 -6

index 461c9a5..dd8e8b6 100644
@@ -2684,6 +2684,8 @@ static void ironlake_irq_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	I915_WRITE(HWSTAM, 0xffffffff);
+
 	GEN5_IRQ_RESET(DE);
 	if (IS_GEN7(dev))
 		I915_WRITE(GEN7_ERR_INT, 0xffffffff);

@@ -2695,10 +2697,6 @@ static void ironlake_irq_reset(struct drm_device *dev)
 
 static void ironlake_irq_preinstall(struct drm_device *dev)
 {
-	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
-
-	I915_WRITE(HWSTAM, 0xeffe);
-
 	ironlake_irq_reset(dev);
 }

@@ -2881,6 +2879,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 
 	dev_priv->irq_mask = ~display_mask;
 
+	I915_WRITE(HWSTAM, 0xeffe);
+
 	ibx_irq_pre_postinstall(dev);
 
 	GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);

@@ -3063,8 +3063,6 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	intel_hpd_irq_uninstall(dev_priv);
 
-	I915_WRITE(HWSTAM, 0xffffffff);
-
 	ironlake_irq_reset(dev);
 }

Signature · 1 line

1.8.4.2

Daniel Vetter

2014-01-22 21:08 UTC #20 lore raw

In reply to Paulo Zanoni

Quoted 18 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
Hi
Back in July 2013 I sent an email asking about interrupts and suggesting ways to improve our code [0]. Based on those discussions, I submitted a patch series proposing some changes [1]. I received some reviews and the general idea was accepted, but due to priority changes I couldn't update the series and resend it.
After this, BDW code got merged and, with it, some BDW-specific IRQ macros that do basically what I had proposed earlier. So this series tries to reuse those BDW macros on ILK+ code.
I think this series is an improvement because now, if we want to change how we treat interrupts, we just need to change the macro and everything gets automatically updated. Also, our code is now smaller :)

Quoted 6 lines

In some of the patches I had to decide between adding POSTING_READ calls to the macros or leave them on the callers. Adding the calls to the macros means we'll call them more times than what is strictly necessary. Not doing that means we may forget some POSTING_READ calls at some point. I chose to follow the BDW example and leave the task of doing POSTING_READ outside the macros. If we don't think this is the best way, I can change it, no problem.

I think for init/fini code which isn't really perf critical it's better to do the posting reads in the common macros, now that we have them. Imo that makes them a notch clearer to understand, and the occasionaly superflous mmio read shouldn't really be noticeable. We have mucher bigger fish to fry first.

But no need to redo this series, maybe just a todo item for a boring day once this has all settled a bit.

Quoted 4 lines

I also considered doing the exact same changes to the previous Gens, but I don't have hardware to test them, and there's a potential to use some different macros due to PIPESTAT handling. So adding Gen2+ support is left as an exercise to developers from the future :)

Yeah, pipestat is special and I think it's good if we keep that separate. But the other registers mostly work the same, so I guess we could reuse that. Although if we really aim for gen2+ and not gen3+ then we need 16bit versions ;-)

Again something for a really rainy day, in case someone gets truly upset with our vlv irq code.

Just figured I'll comment on these two issues, patches themselves look really nice \o/

Cheers, Daniel

Signature · 3 lines

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

Jani Nikula

maintainer

2014-01-23 06:07 UTC #21 lore raw

Quoted 2 lines · Daniel Vetter

Just figured I'll comment on these two issues, patches themselves look really nice \o/

Look nice they do, but it also makes me a little sad that neither git grep nor my source code tagging system will no longer find where the registers are accessed. They are quite powerful tools.

BR,
Jani.

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

Daniel Vetter

2014-01-23 08:22 UTC #22 lore raw

Quoted 7 lines · Jani Nikula

Quoted 2 lines · Daniel Vetter
Just figured I'll comment on these two issues, patches themselves look really nice \o/
Look nice they do, but it also makes me a little sad that neither git grep nor my source code tagging system will no longer find where the registers are accessed. They are quite powerful tools.

Hm, tbh I haven't considered that. But even with that I think the consistency we gain is worth it, especially since it's all contained to i915_irq.c. And it's not really the first time we break grep for register definitions, we have lots of base address + offset all over the place, sometimes even with magic offset only (e.g. the dp aux code).

Also ime the most useful case for grepping is for the random obscure registers which tend to pop up only on a few platforms (or get moved around like crazy), where we need to frob some specific bits for e.g. a w/a. For those it's really useful to reliable be able to grep all use-sites. The I[ISME]R registers imo don't fit into this pattern really.

So still in favour.
-Daniel

Signature · 3 lines

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

Paulo Zanoni

author

2014-01-29 20:08 UTC #23 lore raw

[PATCH 20/19] drm/i915: add POSTING_READs to the IRQ init/reset macros

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

I previously chose to keep the POSTING_READ calls as something to be done by the macro callers, but the conclusion after discussing this on the mailing list is that leaving the POSTING_READ calls to the macros makes the code safer, and the additional useless register reads shouldn't be noticeable. So move the POSTING_READ calls to the callers.

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

---
 drivers/gpu/drm/i915/i915_irq.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +5 ~~-11~~

index 4608f92..7eb5e60 100644
@@ -80,11 +80,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
-/*
- * IIR can theoretically queue up two events. Be paranoid.
- * Also, make sure callers of these macros have something equivalent to a
- * POSTING_READ on the IIR register.
- * */
+/* IIR can theoretically queue up two events. Be paranoid. */
 #define GEN8_IRQ_RESET_NDX(type, which) do { \
 	I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \
 	POSTING_READ(GEN8_##type##_IMR(which)); \

@@ -92,6 +88,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
 	POSTING_READ(GEN8_##type##_IIR(which)); \
 	I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
+	POSTING_READ(GEN8_##type##_IIR(which)); \
 } while (0)
 
 #define GEN5_IRQ_RESET(type) do { \

@@ -101,6 +98,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	I915_WRITE(type##IIR, 0xffffffff); \
 	POSTING_READ(type##IIR); \
 	I915_WRITE(type##IIR, 0xffffffff); \
+	POSTING_READ(type##IIR); \
 } while (0)
 
 /*

@@ -117,12 +115,14 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	GEN5_ASSERT_IIR_IS_ZERO(GEN8_##type##_IIR(which)); \
 	I915_WRITE(GEN8_##type##_IMR(which), (imr_val)); \
 	I915_WRITE(GEN8_##type##_IER(which), (ier_val)); \
+	POSTING_READ(GEN8_##type##_IER(which)); \
 } while (0)
 
 #define GEN5_IRQ_INIT(type, imr_val, ier_val) do { \
 	GEN5_ASSERT_IIR_IS_ZERO(type##IIR); \
 	I915_WRITE(type##IMR, (imr_val)); \
 	I915_WRITE(type##IER, (ier_val)); \
+	POSTING_READ(type##IER); \
 } while (0)
 
 /* For display hotplug interrupt */

@@ -2728,7 +2728,6 @@ static void gen5_gt_irq_reset(struct drm_device *dev)
 	GEN5_IRQ_RESET(GT);
 	if (INTEL_INFO(dev)->gen >= 6)
 		GEN5_IRQ_RESET(GEN6_PM);
-	POSTING_READ(GTIIR);
 }
 
 /* drm_dma.h hooks

@@ -2802,7 +2801,6 @@ static void gen8_irq_reset(struct drm_device *dev)
 	GEN5_IRQ_RESET(GEN8_DE_PORT_);
 	GEN5_IRQ_RESET(GEN8_DE_MISC_);
 	GEN5_IRQ_RESET(GEN8_PCU_);
-	POSTING_READ(GEN8_PCU_IIR);
 
 	ibx_irq_reset(dev);
 }

@@ -2901,7 +2899,6 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
 		dev_priv->pm_irq_mask = 0xffffffff;
 		GEN5_IRQ_INIT(GEN6_PM, dev_priv->pm_irq_mask, pm_irqs);
 	}
-	POSTING_READ(GTIER);
 }
 
 static int ironlake_irq_postinstall(struct drm_device *dev)

@@ -3029,7 +3026,6 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
 
 	for (i = 0; i < ARRAY_SIZE(gt_interrupts); i++)
 		GEN8_IRQ_INIT_NDX(GT, i, ~gt_interrupts[i], gt_interrupts[i]);
-	POSTING_READ(GEN8_GT_IER(0));
 }
 
 static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)

@@ -3048,10 +3044,8 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
 	for_each_pipe(pipe)
 		GEN8_IRQ_INIT_NDX(DE_PIPE, pipe, dev_priv->de_irq_mask[pipe],
 				  de_pipe_enables);
-	POSTING_READ(GEN8_DE_PIPE_ISR(0));
 
 	GEN5_IRQ_INIT(GEN8_DE_PORT_, ~GEN8_AUX_CHANNEL_A, GEN8_AUX_CHANNEL_A);
-	POSTING_READ(GEN8_DE_PORT_IER);
 }
 
 static int gen8_irq_postinstall(struct drm_device *dev)

Signature · 1 line

1.8.5.3