ILK+ interrupt improvements, v2

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 +0 -0

index 852844d..7be7da1 100644

drivers/gpu/drm/i915/i915_irq.c +18 -26

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

@@ -2789,6 +2807,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

@@ -2843,25 +2862,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);

@@ -2871,12 +2871,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 · 6 lines

1.8.5.3

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

Quoted 27 lines · 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 +0 -0

index 852844d..7be7da1 100644
@@ -80,12 +80,30 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 

drivers/gpu/drm/i915/i915_irq.c static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */ · line 87

Quoted 5 lines

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

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

I don't understand what you mean in this comment. If you're always going to sending a posting read after the second IIR write, why not just put it in the macro?

The reason it wasn't in my original macro is because we've done the posting read on IER, and IMR - so we're not going to get new interrupts. When the second IIR write lands is irrelevant. The POSTING_READ in between is to prevent the [probably impossible] case of the writes getting collapsed into one write.

Quoted 80 lines

drivers/gpu/drm/i915/i915_irq.c +18 -26

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

@@ -2789,6 +2807,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

@@ -2843,25 +2862,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);

@@ -2871,12 +2871,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 · 6 lines

1.8.5.3

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

Signature · 5 lines

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

Quoted 27 lines

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 +0 -0

index 852844d..7be7da1 100644
@@ -80,12 +80,30 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
      [HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 

drivers/gpu/drm/i915/i915_irq.c static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */ · line 87

Quoted 5 lines

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

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

I don't understand what you mean in this comment. If you're always going to sending a posting read after the second IIR write, why not just put it in the macro?

The reason it wasn't in my original macro is because we've done the posting read on IER, and IMR - so we're not going to get new interrupts. When the second IIR write lands is irrelevant. The POSTING_READ in between is to prevent the [probably impossible] case of the writes getting collapsed into one write.

Quoted 77 lines

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

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

@@ -2789,6 +2807,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

@@ -2843,25 +2862,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);

@@ -2871,12 +2871,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);
--

1.8.5.3

Quoted 4 lines

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

--
Ben Widawsky, Intel Open Source Technology Center

Anyway, we have discussed this and it seems everybody prefers to have the POSTING_READ calls on the macro, so I'll just change the patches to do this, and then the comment will disappear.

Quoted 3 lines

Anyway, we have discussed this and it seems everybody prefers to have the POSTING_READ calls on the macro, so I'll just change the patches to do this, and then the comment will disappear.

I think for consistency we should do the same change in gen8. So maybe as a follow-up patch on top of this which changes everything? Since this unifies the irq setup code it's slightly irksome if we deviate right away ;-)

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

From: Paulo Zanoni <[email protected]>

It's the only thihg missing, apparently.

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 a9f173c..f681462 100644
@@ -3314,6 +3314,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 · 6 lines

1.8.5.3

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

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.

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 f681462..73f1125 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)

@@ -2794,7 +2788,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

@@ -2809,9 +2803,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);
 }
 

@@ -2823,7 +2817,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);
 

@@ -2867,18 +2861,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);

@@ -3237,32 +3231,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);
 }

@@ -3309,18 +3288,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 · 6 lines

1.8.5.3

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

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 +0 -0

index 73f1125..6d4daf2 100644
@@ -103,6 +103,16 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	I915_WRITE(type##IIR, 0xffffffff); \
 } while (0)
 

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

 /* For display hotplug interrupt */
 static void
 ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)

@@ -2957,9 +2967,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;

@@ -2969,10 +2977,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)

@@ -3005,9 +3012,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);
 

@@ -3172,8 +3177,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));
 }

@@ -3196,13 +3200,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 · 6 lines

1.8.5.3

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

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 +10 -0

index 6d4daf2..4d0a8b1 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 { \

drivers/gpu/drm/i915/i915_irq.c +3 -17

@@ -2940,7 +2952,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);
 }
 

@@ -2966,7 +2978,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) {

@@ -2976,7 +2987,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);

@@ -3010,8 +3020,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);

@@ -3172,13 +3180,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));
 }
 

@@ -3195,14 +3198,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 · 6 lines

1.8.5.3

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

Quoted 29 lines · 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 +10 -0

index 6d4daf2..4d0a8b1 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 { \

drivers/gpu/drm/i915/i915_irq.c static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */ · line 126

Quoted 10 lines

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

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

Okay, this is replacing a POSTED_WRITE, with a (slower) POSTING_READ which gives an error that we can do nothing about other than clear it anyway.

I'd be in favor of dropping this patch.

Quoted 73 lines

drivers/gpu/drm/i915/i915_irq.c +3 -17

@@ -2940,7 +2952,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);
 }
 

@@ -2966,7 +2978,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) {

@@ -2976,7 +2987,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);

@@ -3010,8 +3020,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);

@@ -3172,13 +3180,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));
 }
 

@@ -3195,14 +3198,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 · 6 lines

1.8.5.3

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

Signature · 5 lines

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

Quoted 45 lines · Ben Widawsky

Quoted 29 lines · 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 +10 -0

index 6d4daf2..4d0a8b1 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 { \

drivers/gpu/drm/i915/i915_irq.c static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */ · line 126

Quoted 10 lines

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

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

Okay, this is replacing a POSTED_WRITE, with a (slower) POSTING_READ which gives an error that we can do nothing about other than clear it anyway.

Quoted 1 line

I'd be in favor of dropping this patch.

The point of the assert is to make sure that the new IIR clearing logic with blocking everything+clearing in the preinstall hook actually does what it's supposed to do.

Since the point of this exercise is to reuse this code for runtime suspend/resume where races are much easier to hit I think this is a good self-check of the code.
-Daniel

Quoted 48 lines · Ben Widawsky

Quoted 46 lines · Paulo Zanoni

Quoted 29 lines

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 +10 -0

index 6d4daf2..4d0a8b1 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 { \

drivers/gpu/drm/i915/i915_irq.c static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */ · line 126

Quoted 10 lines

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

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

Okay, this is replacing a POSTED_WRITE, with a (slower) POSTING_READ which gives an error that we can do nothing about other than clear it anyway.

I'd be in favor of dropping this patch.

Quoted 7 lines

The point of the assert is to make sure that the new IIR clearing logic with blocking everything+clearing in the preinstall hook actually does what it's supposed to do.

Since the point of this exercise is to reuse this code for runtime suspend/resume where races are much easier to hit I think this is a good self-check of the code.

Quoted 2 lines

-Daniel

Okay, I am feeling somewhat pressured to stick a reviewed-by on this since Daniel likes it.

Change the macro to WARN instead of DRM_ERROR, and, clear the IIR if it's non-zero. With that change, it's:
Reviewed-by-with-reservations: Ben Widawsky <[email protected]>

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

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 4d0a8b1..d295624 100644
@@ -2811,6 +2811,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

@@ -2949,7 +2953,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);

@@ -3303,7 +3307,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 · 6 lines

1.8.5.3

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

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 d295624..02eb493 100644
@@ -2845,6 +2845,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);

@@ -3011,7 +3014,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 |

@@ -3295,7 +3298,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 · 6 lines

1.8.5.3

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

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 +1 -5

index 95f535b..4479e29 100644
@@ -2814,13 +2814,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.

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

+{
+	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);
 }

@@ -3026,6 +3037,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);

@@ -3217,6 +3230,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 · 6 lines

1.8.5.3

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

Quoted 6 lines

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

I think this statement is false. The interrupt is enabled right after preinstall(). For the nomodeset case, this actually seems to make some difference. It still looks fine to me though.

Quoted 30 lines

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 +4 -6

index 95f535b..4479e29 100644
@@ -2814,13 +2814,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 -

drivers/gpu/drm/i915/i915_irq.c static void ibx_irq_preinstall(struct drm_device *dev) · line 2827

Quoted 6 lines

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

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

sde_irq_postinstall()?

Quoted 32 lines

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

+{
+     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);
 }

@@ -3026,6 +3037,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);

@@ -3217,6 +3230,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);
 
--

1.8.5.3

Quoted 4 lines

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

--
Ben Widawsky, Intel Open Source Technology Center

2014-03-18 17:29 GMT-03:00 Ben Widawsky <[email protected]>:

Quoted 8 lines · Paulo Zanoni

Quoted 6 lines

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

Quoted 3 lines

I think this statement is false. The interrupt is enabled right after preinstall(). For the nomodeset case, this actually seems to make some difference. It still looks fine to me though.

Could you please clarify this paragraph?

We currently enable SDEIER at ibx_irq_preinstall, but the reason why we do this is because we change SDEIER at the IRQ handler. So if we move that code from preinstall to postinstall, but at a point where no interrupts are enabled yet, we should be safe, and this is what the patch does.

Quoted 30 lines

Quoted 30 lines

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 +4 -6

index 95f535b..4479e29 100644
@@ -2814,13 +2814,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 -

Quoted 8 lines

drivers/gpu/drm/i915/i915_irq.c static void ibx_irq_preinstall(struct drm_device *dev) · line 2827

Quoted 6 lines

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

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

sde_irq_postinstall()?

I agree the current name is bad, but we use the IBX naming for everything on the PCH at i915_irq.c, and ironlake_irq_postinstall() already calls a function named ibx_irq_postinstall(), so I needed a name that means "call this just before the postinstall() steps". If we rename it to sde_irq_postinstall we may confuse users due to the fact that we also have ibx_irq_postinstall. So with the current patch, we have this:

void ironlake_irq_postinstall()

{
    /* We have to call this before enabling the interrupts */
    ibx_irq_pre_postinstall();
    /* Enable the interrupts */
    GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);
    /* Now do the necessary postinstall adjustments to the PCH stuff */
    ibx_irq_postinstall();
}

But I'm still open to new naming suggestions.

Quoted 39 lines

Quoted 32 lines

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

+{
+     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);
 }

@@ -3026,6 +3037,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);

@@ -3217,6 +3230,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);
 
--

1.8.5.3

Quoted 4 lines

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

--
Ben Widawsky, Intel Open Source Technology Center

Signature · 1 line

Paulo Zanoni

Quoted 9 lines · Paulo Zanoni

2014-03-18 17:29 GMT-03:00 Ben Widawsky <[email protected]>:

Quoted 8 lines · Paulo Zanoni

Quoted 6 lines

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

Quoted 5 lines

Quoted 3 lines

I think this statement is false. The interrupt is enabled right after preinstall(). For the nomodeset case, this actually seems to make some difference. It still looks fine to me though.

Could you please clarify this paragraph?

The point was, "IRQ handler is disabled" is false. At least when I last checked. We've registered the interrupt by then, which means we can potentially get interrupts from the hardware.

I think we just might disagree on what "enabled" means. Perhaps this is due to me working for too long with buggy hardware. In any event, as I said, it does look fine to me as is.

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 3584a16d..1e5cc5b 100644
@@ -3251,6 +3251,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 · 6 lines

1.8.5.3

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

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 +6 -10

index 4917a8c..d6723ab 100644
@@ -2899,7 +2899,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;

@@ -2924,6 +2924,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;

@@ -3253,30 +3258,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)

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

 }
 
 static void valleyview_irq_uninstall(struct drm_device *dev)

Signature · 6 lines

1.8.5.3

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

Quoted 58 lines

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 +6 -15

index 4917a8c..d6723ab 100644
@@ -2899,7 +2899,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;

@@ -2924,6 +2924,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;

@@ -3253,30 +3258,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_);

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

Quoted 5 lines

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

-
-     POSTING_READ(GEN8_PCU_IIR);
-
-     ibx_irq_reset(dev);
+     gen8_irq_reset(dev);

BTW: This looks like a bad hunk. I've merged up to this point, and I do not have ibx_irq_reset().

Quoted 8 lines

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

 }
 
 static void valleyview_irq_uninstall(struct drm_device *dev)
--

1.8.5.3

Quoted 4 lines

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

--
Ben Widawsky, Intel Open Source Technology Center

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 79a8196..dee3a3b 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 */

@@ -2843,7 +2843,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

@@ -2917,7 +2916,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);
 }

@@ -3016,7 +3014,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)

@@ -3213,7 +3210,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)

@@ -3232,10 +3228,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 · 6 lines

1.8.5.3

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

From: Paulo Zanoni <[email protected]>

Hi

This is basically a rebase of "[PATCH 00/19] ILK+ interrupt improvements", which was sent to the mailing list on January 22. There are no real differences, except for the last patch, which is new.

Original cover letter:
http://lists.freedesktop.org/archives/intel-gfx/2014-January/038679.html

The idea behind this series is that at some point our runtime PM code will just call our irq_preinstall, irq_postinstall and irq_uninstall functions instead of using dev_priv->pc8.regsave, so I decided to audit, cleanup and add a few WARNs to our code before we do that change. We gotta be in shape if we want to be exposed to runtime!

Thanks,
Paulo

Paulo Zanoni (20):
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+ drm/i915: add POSTING_READs to the IRQ init/reset macros

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

Quoted 42 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Hi

This is basically a rebase of "[PATCH 00/19] ILK+ interrupt improvements", which was sent to the mailing list on January 22. There are no real differences, except for the last patch, which is new.

Original cover letter:
http://lists.freedesktop.org/archives/intel-gfx/2014-January/038679.html

The idea behind this series is that at some point our runtime PM code will just call our irq_preinstall, irq_postinstall and irq_uninstall functions instead of using dev_priv->pc8.regsave, so I decided to audit, cleanup and add a few WARNs to our code before we do that change. We gotta be in shape if we want to be exposed to runtime!

Thanks,
Paulo

Paulo Zanoni (20):
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+ drm/i915: add POSTING_READs to the IRQ init/reset macros

Quoted 3 lines

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

Okay, here is the summary of my review. At first I was complaining to myself about how many patches you used to do a simple thing. But, I must admit it made reviewing the thing a lot easier, and when I look back at how much stuff you combined, I'm really glad you did it this way. I'm sure I've missed something silly though, since every patch looks so similar :P

1-5: Reviewed-by: Ben Widawsky <[email protected]> (with possible comment improvement on #3)

7: I don't like. Can we drop? I guess doing this would make a decent amount of churn, so if you don't want to drop it, that's fine, and it's functionally correct:

Reviewed-by Ben Widawsky <[email protected]>

8: I'd really like to drop this one.

9-10: Reviewed-by: Ben Widawsky <[email protected]>

12-13: I wouldn't mind cpt_irq_* rename, but either way

Reviewed-by Ben Widawsky <[email protected]>

14: With the requested change in the mail:

Reviewed-by Ben Widawsky <[email protected]>

16: Reviewed-by: Ben Widawsky <[email protected]>

20: Should be squashed, but

Reviewed-by Ben Widawsky <[email protected]>

6, 11, 15, 17, 18, 19: You introduce the term _reset as a verb which seems to always mean "disable." I think disable makes the code so much clearer, and would really love if you can apply this simple rename. With the rename, they're:

Reviewed-by Ben Widawsky <[email protected]>

Quoted 62 lines · Ben Widawsky

Quoted 42 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Hi

This is basically a rebase of "[PATCH 00/19] ILK+ interrupt improvements", which was sent to the mailing list on January 22. There are no real differences, except for the last patch, which is new.

Original cover letter:
http://lists.freedesktop.org/archives/intel-gfx/2014-January/038679.html

The idea behind this series is that at some point our runtime PM code will just call our irq_preinstall, irq_postinstall and irq_uninstall functions instead of using dev_priv->pc8.regsave, so I decided to audit, cleanup and add a few WARNs to our code before we do that change. We gotta be in shape if we want to be exposed to runtime!

Thanks,
Paulo

Paulo Zanoni (20):
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+ drm/i915: add POSTING_READs to the IRQ init/reset macros

Quoted 3 lines

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

Okay, here is the summary of my review. At first I was complaining to myself about how many patches you used to do a simple thing. But, I must admit it made reviewing the thing a lot easier, and when I look back at how much stuff you combined, I'm really glad you did it this way. I'm sure I've missed something silly though, since every patch looks so similar :P

1-5: Reviewed-by: Ben Widawsky <[email protected]> (with possible comment improvement on #3)

7: I don't like. Can we drop? I guess doing this would make a decent amount of churn, so if you don't want to drop it, that's fine, and it's functionally correct:

Reviewed-by Ben Widawsky <[email protected]>

Quoted 1 line

8: I'd really like to drop this one.

Comment on this and I think with a pimped commit message this is good to go imo. I really think the added self-checks are required to start using this code for runtime pm.

Quoted 10 lines

9-10: Reviewed-by: Ben Widawsky <[email protected]>

12-13: I wouldn't mind cpt_irq_* rename, but either way

Reviewed-by Ben Widawsky <[email protected]>

14: With the requested change in the mail:

Reviewed-by Ben Widawsky <[email protected]>

16: Reviewed-by: Ben Widawsky <[email protected]>

Quoted 8 lines

20: Should be squashed, but

Reviewed-by Ben Widawsky <[email protected]>

6, 11, 15, 17, 18, 19: You introduce the term _reset as a verb which seems to always mean "disable." I think disable makes the code so much clearer, and would really love if you can apply this simple rename. With the rename, they're:

Reviewed-by Ben Widawsky <[email protected]>

Paulo's using "reset" functions/macros both in the preinstall hooks and in the uninstall/disable code. We already use reset for stuff run before init/enable code to get the hw in a state we expect it to, so I think Paulo's naming choice is accurate and a plain "disable" more misleading.

I think you raise some good points in your review, and besides the 3 cases I commented on I lack the detailed knowledge to avoid looking like a fool ;-) So I think I'll wait for Paulo's comments before pulling this all in.

Thanks,
Daniel

Signature · 3 lines

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

Quoted 63 lines · Ben Widawsky

Quoted 62 lines · Paulo Zanoni

Quoted 42 lines

From: Paulo Zanoni <[email protected]>

Hi

This is basically a rebase of "[PATCH 00/19] ILK+ interrupt improvements", which was sent to the mailing list on January 22. There are no real differences, except for the last patch, which is new.

Original cover letter:
http://lists.freedesktop.org/archives/intel-gfx/2014-January/038679.html

The idea behind this series is that at some point our runtime PM code will just call our irq_preinstall, irq_postinstall and irq_uninstall functions instead of using dev_priv->pc8.regsave, so I decided to audit, cleanup and add a few WARNs to our code before we do that change. We gotta be in shape if we want to be exposed to runtime!

Thanks,
Paulo

Paulo Zanoni (20):
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+ drm/i915: add POSTING_READs to the IRQ init/reset macros

Quoted 3 lines

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

Okay, here is the summary of my review. At first I was complaining to myself about how many patches you used to do a simple thing. But, I must admit it made reviewing the thing a lot easier, and when I look back at how much stuff you combined, I'm really glad you did it this way. I'm sure I've missed something silly though, since every patch looks so similar :P

1-5: Reviewed-by: Ben Widawsky <[email protected]> (with possible comment improvement on #3)

7: I don't like. Can we drop? I guess doing this would make a decent amount of churn, so if you don't want to drop it, that's fine, and it's functionally correct:

Reviewed-by Ben Widawsky <[email protected]>

Quoted 6 lines

Quoted 1 line

8: I'd really like to drop this one.

Comment on this and I think with a pimped commit message this is good to go imo. I really think the added self-checks are required to start using this code for runtime pm.

So you don't need my reviewed-by then. I don't like it...

Quoted 19 lines

Quoted 18 lines

9-10: Reviewed-by: Ben Widawsky <[email protected]>

12-13: I wouldn't mind cpt_irq_* rename, but either way

Reviewed-by Ben Widawsky <[email protected]>

14: With the requested change in the mail:

Reviewed-by Ben Widawsky <[email protected]>

16: Reviewed-by: Ben Widawsky <[email protected]>

20: Should be squashed, but

Reviewed-by Ben Widawsky <[email protected]>

6, 11, 15, 17, 18, 19: You introduce the term _reset as a verb which seems to always mean "disable." I think disable makes the code so much clearer, and would really love if you can apply this simple rename. With the rename, they're:

Reviewed-by Ben Widawsky <[email protected]>

Quoted 5 lines

Paulo's using "reset" functions/macros both in the preinstall hooks and in the uninstall/disable code. We already use reset for stuff run before init/enable code to get the hw in a state we expect it to, so I think Paulo's naming choice is accurate and a plain "disable" more misleading.

I cannot disagree more. Every time I read "reset" it confuses me. But it seems like I am the minority.

Quoted 7 lines

I think you raise some good points in your review, and besides the 3 cases I commented on I lack the detailed knowledge to avoid looking like a fool ;-) So I think I'll wait for Paulo's comments before pulling this all in.

Thanks,
Daniel

Once Paulo responds, I'll make it a top priority to re-review whatever is needed. Sorry for the original delay.

Quoted 4 lines

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

--
Ben Widawsky, Intel Open Source Technology Center

2014-03-19 14:25 GMT-03:00 Ben Widawsky <[email protected]>:

Quoted 92 lines · Daniel Vetter

Quoted 63 lines · Ben Widawsky

Quoted 62 lines · Paulo Zanoni

Quoted 42 lines

From: Paulo Zanoni <[email protected]>

Hi

This is basically a rebase of "[PATCH 00/19] ILK+ interrupt improvements", which was sent to the mailing list on January 22. There are no real differences, except for the last patch, which is new.

Original cover letter:
http://lists.freedesktop.org/archives/intel-gfx/2014-January/038679.html

The idea behind this series is that at some point our runtime PM code will just call our irq_preinstall, irq_postinstall and irq_uninstall functions instead of using dev_priv->pc8.regsave, so I decided to audit, cleanup and add a few WARNs to our code before we do that change. We gotta be in shape if we want to be exposed to runtime!

Thanks,
Paulo

Paulo Zanoni (20):
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+ drm/i915: add POSTING_READs to the IRQ init/reset macros

Quoted 3 lines

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

Okay, here is the summary of my review. At first I was complaining to myself about how many patches you used to do a simple thing. But, I must admit it made reviewing the thing a lot easier, and when I look back at how much stuff you combined, I'm really glad you did it this way. I'm sure I've missed something silly though, since every patch looks so similar :P

1-5: Reviewed-by: Ben Widawsky <[email protected]> (with possible comment improvement on #3)

7: I don't like. Can we drop? I guess doing this would make a decent amount of churn, so if you don't want to drop it, that's fine, and it's functionally correct:

Reviewed-by Ben Widawsky <[email protected]>

Quoted 6 lines

Quoted 1 line

8: I'd really like to drop this one.

Comment on this and I think with a pimped commit message this is good to go imo. I really think the added self-checks are required to start using this code for runtime pm.

So you don't need my reviewed-by then. I don't like it...

Quoted 19 lines

Quoted 18 lines

9-10: Reviewed-by: Ben Widawsky <[email protected]>

12-13: I wouldn't mind cpt_irq_* rename, but either way

Reviewed-by Ben Widawsky <[email protected]>

14: With the requested change in the mail:

Reviewed-by Ben Widawsky <[email protected]>

16: Reviewed-by: Ben Widawsky <[email protected]>

20: Should be squashed, but

Reviewed-by Ben Widawsky <[email protected]>

6, 11, 15, 17, 18, 19: You introduce the term _reset as a verb which seems to always mean "disable." I think disable makes the code so much clearer, and would really love if you can apply this simple rename. With the rename, they're:

Reviewed-by Ben Widawsky <[email protected]>

Quoted 8 lines

Quoted 5 lines

Paulo's using "reset" functions/macros both in the preinstall hooks and in the uninstall/disable code. We already use reset for stuff run before init/enable code to get the hw in a state we expect it to, so I think Paulo's naming choice is accurate and a plain "disable" more misleading.

I cannot disagree more. Every time I read "reset" it confuses me. But it seems like I am the minority.

I understand "reset" may not be the best name, I was already expecting to see suggestions on the naming here. IMHO "disable" is also usable, and I could rename, but Daniel just called it "misleading". So how about we rename it to "clear" instead?

(let's see if I can make Ben and Daniel agree on something!)

I'll leave discussion of the other topics to the other emails.

Quoted 18 lines

Quoted 7 lines

I think you raise some good points in your review, and besides the 3 cases I commented on I lack the detailed knowledge to avoid looking like a fool ;-) So I think I'll wait for Paulo's comments before pulling this all in.

Thanks,
Daniel

Once Paulo responds, I'll make it a top priority to re-review whatever is needed. Sorry for the original delay.

Quoted 4 lines

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

--
Ben Widawsky, Intel Open Source Technology Center

Signature · 5 lines

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

Quoted 16 lines

Quoted 8 lines

Quoted 5 lines

Paulo's using "reset" functions/macros both in the preinstall hooks and in the uninstall/disable code. We already use reset for stuff run before init/enable code to get the hw in a state we expect it to, so I think Paulo's naming choice is accurate and a plain "disable" more misleading.

I cannot disagree more. Every time I read "reset" it confuses me. But it seems like I am the minority.

I understand "reset" may not be the best name, I was already expecting to see suggestions on the naming here. IMHO "disable" is also usable, and I could rename, but Daniel just called it "misleading". So how about we rename it to "clear" instead?

(let's see if I can make Ben and Daniel agree on something!)

Quoted 2 lines

I'll leave discussion of the other topics to the other emails.

"clear" has a distinct definition, and in the case of the mask, you are not clearing. It's better than "reset"

I still like, "disable"
I can live with "disable_and_mask"