[PATCH 09/19] drm/i915: fix SERR_INT init/reset code

[PATCH 09/19] drm/i915: fix SERR_INT init/reset code

[Paulo Zanoni]

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

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 <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index fb6ebd1..5b60de5 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -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)
-- 
1.8.4.2

[PATCH 00/19] ILK+ interrupt improvements, v3

[Paulo Zanoni]

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

Hi

This is a follow up to "ILK+ interrupt improvements, v3", which was reveiwed by
Ben.

The biggest difference is that patch 20 was squashed into the previous patch, so
different patches received different chunks of patch 20. This caused the need to
rebase pretty much every patch of the series. There were also some small
changes, like typos on commit messages, changes on comments and the removal of
some assertions.

We had some discussions regarding renaming, where Ben proposed renames but
Daniel disagreed with the proposals. Ben expressed he was not comfortable with
giving Reviewed-by tags on some patches without the renames and I understand his
point of view, so I just did not add the tags for those patches. The patches
that still do not have Reviewed-by tags are: 6, 8, 11, 14, 15, 17, 18, 19.

Thanks,
Paulo

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 | 281 ++++++++++++++++++----------------------
 1 file changed, 126 insertions(+), 155 deletions(-)

-- 
1.8.5.3

[PATCH 01/19] drm/i915: add GEN5_IRQ_INIT macro

[Paulo Zanoni]

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

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.

v2: - Rebase.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 2163f18..f487068 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -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(struct drm_i915_private *dev_priv, u32 mask)
@@ -2837,17 +2843,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
@@ -2858,9 +2856,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);
 
-- 
1.8.5.3

[PATCH 02/19] drm/i915: also use GEN5_IRQ_INIT with south display interrupts

[Paulo Zanoni]

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

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.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index f487068..af1d43c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2827,8 +2827,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
-- 
1.8.5.3

[PATCH 03/19] drm/i915: use GEN8_IRQ_INIT on GEN5

[Paulo Zanoni]

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

And rename it 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.

v2: - Fix typo in commit message.
    - Add POSTING_READ calls to the macros (Ben, Daniel, Jani).

Reviewed-by: Ben Widawsky <ben@bwidawsk.net> (v1)
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 46 +++++++++++++++++------------------------
 1 file changed, 19 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index af1d43c..bc7e230 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -80,10 +80,25 @@ 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. */
+#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); \
+	POSTING_READ(GEN8_##type##_IIR(which)); \
+} 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); \
+	POSTING_READ(type##IIR); \
 } while (0)
 
 /* For display hotplug interrupt */
@@ -2899,25 +2914,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);
@@ -2927,13 +2923,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
-
-	POSTING_READ(GEN8_PCU_IIR);
+	GEN5_IRQ_INIT(GEN8_DE_PORT_);
+	GEN5_IRQ_INIT(GEN8_DE_MISC_);
+	GEN5_IRQ_INIT(GEN8_PCU_);
 
 	ibx_irq_preinstall(dev);
 }
-- 
1.8.5.3

[PATCH 04/19] drm/i915: add GEN5_IRQ_FINI

[Paulo Zanoni]

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

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

v2: - Rebase.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bc7e230..26bfe1b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -101,6 +101,12 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	POSTING_READ(type##IIR); \
 } 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(struct drm_i915_private *dev_priv, u32 mask)
@@ -3353,22 +3359,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));
 }
-- 
1.8.5.3

[PATCH 05/19] drm/i915: don't forget to uninstall the PM IRQs

[Paulo Zanoni]

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

It's the only thing missing, apparently.

v2: - Fix typo (Ben).

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 26bfe1b..72bb443 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3364,6 +3364,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;
-- 
1.8.5.3

[PATCH 06/19] drm/i915: properly clear IIR at irq_uninstall on Gen5+

[Paulo Zanoni]

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

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 INIT
macro to IRQ_RESET and use it everywhere.

v2: - Fix error in the commit message (Chris).
    - Adjust to the new POSTING_READ scheme (Ben).

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 72bb443..a2a2d51 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -81,7 +81,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 };
 
 /* IIR can theoretically queue up two events. Be paranoid. */
-#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); \
@@ -91,7 +91,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	POSTING_READ(GEN8_##type##_IIR(which)); \
 } 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); \
@@ -101,12 +101,6 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	POSTING_READ(type##IIR); \
 } 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(struct drm_i915_private *dev_priv, u32 mask)
@@ -2848,7 +2842,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
@@ -2863,9 +2857,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);
 }
 
 /* drm_dma.h hooks
@@ -2876,7 +2870,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);
 
@@ -2920,18 +2914,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_);
 
 	ibx_irq_preinstall(dev);
 }
@@ -3287,34 +3281,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);
-	}
-
-	GEN8_IRQ_FINI(DE_PORT);
-	GEN8_IRQ_FINI(DE_MISC);
-	GEN8_IRQ_FINI(PCU);
-#undef GEN8_IRQ_FINI
-#undef GEN8_IRQ_FINI_NDX
+	for_each_pipe(pipe)
+		GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
 
-	POSTING_READ(GEN8_PCU_IIR);
+	GEN5_IRQ_RESET(GEN8_DE_PORT_);
+	GEN5_IRQ_RESET(GEN8_DE_MISC_);
+	GEN5_IRQ_RESET(GEN8_PCU_);
 }
 
 static void valleyview_irq_uninstall(struct drm_device *dev)
@@ -3359,18 +3336,18 @@ 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);
 
 	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));
 }
-- 
1.8.5.3

[PATCH 07/19] drm/i915: add GEN5_IRQ_INIT

[Paulo Zanoni]

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

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

v2: - Adjust to the new POSTING_READ scheme (Ben).

Reviewed-by: Ben Widawsky <ben@bwidawsk.net> (v1)
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a2a2d51..ef4adf6 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -101,6 +101,18 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	POSTING_READ(type##IIR); \
 } 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)); \
+	POSTING_READ(GEN8_##type##_IER(which)); \
+} while (0)
+
+#define GEN5_IRQ_INIT(type, imr_val, ier_val) do { \
+	I915_WRITE(type##IMR, (imr_val)); \
+	I915_WRITE(type##IER, (ier_val)); \
+	POSTING_READ(type##IER); \
+} while (0)
+
 /* For display hotplug interrupt */
 static void
 ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask)
@@ -3008,9 +3020,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 |= dev_priv->pm_rps_events;
@@ -3020,9 +3030,7 @@ 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);
 	}
 }
 
@@ -3055,9 +3063,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);
 
@@ -3222,10 +3228,8 @@ 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));
 }
 
 static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
@@ -3246,14 +3250,11 @@ 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);
-	POSTING_READ(GEN8_DE_PORT_IER);
+	GEN5_IRQ_INIT(GEN8_DE_PORT_, ~GEN8_AUX_CHANNEL_A, GEN8_AUX_CHANNEL_A);
 }
 
 static int gen8_irq_postinstall(struct drm_device *dev)
-- 
1.8.5.3

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

[Paulo Zanoni]

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

It should already be masked and disabled and zeroed at the preinstall
and uninstall stages. Also, the current code just writes to IIR once,
and this is not a guarantee that it will be cleared, so it's wrong
anyway.

The whole reason for the paranoia is that we're going to start calling
the IRQ preinstall/postinstall/uninstall from the runtime PM
callbacks, so we need to make sure everything is behaving as expected.

v2: - Change the original DRM_ERROR to WARN and clear IIR in case it's
      not zero (Ben).
    - Improve commit message (Daniel).

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ef4adf6..65e901e 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -101,13 +101,30 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	POSTING_READ(type##IIR); \
 } 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) { \
+		WARN(1, "Interrupt register 0x%x is not zero: 0x%08x\n", \
+		     (reg), val); \
+		I915_WRITE((reg), 0xffffffff); \
+		POSTING_READ(reg); \
+		I915_WRITE((reg), 0xffffffff); \
+		POSTING_READ(reg); \
+	} \
+} 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)); \
 	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); \
@@ -2993,7 +3010,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);
 }
 
@@ -3019,7 +3036,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) {
@@ -3029,7 +3045,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);
 	}
 }
@@ -3061,8 +3076,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);
@@ -3223,13 +3236,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]);
-	}
 }
 
 static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
@@ -3245,14 +3253,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);
-	}
 
 	GEN5_IRQ_INIT(GEN8_DE_PORT_, ~GEN8_AUX_CHANNEL_A, GEN8_AUX_CHANNEL_A);
 }
-- 
1.8.5.3

[PATCH 09/19] drm/i915: fix SERR_INT init/reset code

[Paulo Zanoni]

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

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.

v2: - Remove the is_zero assertion (Ben).

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 65e901e..bfb7e14 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2872,6 +2872,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
@@ -3002,14 +3006,11 @@ static void ibx_irq_postinstall(struct drm_device *dev)
 	if (HAS_PCH_NOP(dev))
 		return;
 
-	if (HAS_PCH_IBX(dev)) {
+	if (HAS_PCH_IBX(dev))
 		mask = SDE_GMBUS | SDE_AUX_MASK | SDE_POISON;
-	} else {
+	else
 		mask = SDE_GMBUS_CPT | SDE_AUX_MASK_CPT;
 
-		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
-	}
-
 	GEN5_ASSERT_IIR_IS_ZERO(SDEIIR);
 	I915_WRITE(SDEIMR, ~mask);
 }
@@ -3353,7 +3354,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)
-- 
1.8.5.3

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

[Paulo Zanoni]

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

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.

v2: - Remove the is_zero assertion (Ben).

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bfb7e14..ace3703 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2905,6 +2905,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);
@@ -3063,8 +3066,6 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 				DE_PLANEA_FLIP_DONE_IVB | DE_AUX_CHANNEL_A_IVB);
 		extra_mask = (DE_PIPEC_VBLANK_IVB | DE_PIPEB_VBLANK_IVB |
 			      DE_PIPEA_VBLANK_IVB | DE_ERR_INT_IVB);
-
-		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
 	} else {
 		display_mask = (DE_MASTER_IRQ_CONTROL | DE_GSE | DE_PCH_EVENT |
 				DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE |
@@ -3343,7 +3344,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)
-- 
1.8.5.3

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

[Paulo Zanoni]

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

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

v2: - Rebase.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ace3703..5e1a6d3 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2886,7 +2886,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;
 
@@ -2908,7 +2908,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);
 }
@@ -2928,7 +2928,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);
 
@@ -3346,9 +3346,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);
+	gen5_gt_irq_reset(dev);
 
 	if (HAS_PCH_NOP(dev))
 		return;
-- 
1.8.5.3

[PATCH 12/19] drm/i915: extract ibx_irq_uninstall

[Paulo Zanoni]

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

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

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 5e1a6d3..ad2c3de 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2904,7 +2904,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);
 
@@ -3277,6 +3276,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;
@@ -3348,12 +3360,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)
-- 
1.8.5.3

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

[Paulo Zanoni]

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

After all, we call ibx_irq_preinstall from gen8_irq_preinstall.

v2: - Rebase.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ad2c3de..1f601ef 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3310,6 +3310,8 @@ static void gen8_irq_uninstall(struct drm_device *dev)
 	GEN5_IRQ_RESET(GEN8_DE_PORT_);
 	GEN5_IRQ_RESET(GEN8_DE_MISC_);
 	GEN5_IRQ_RESET(GEN8_PCU_);
+
+	ibx_irq_uninstall(dev);
 }
 
 static void valleyview_irq_uninstall(struct drm_device *dev)
-- 
1.8.5.3

[PATCH 14/19] drm/i915: enable SDEIER later

[Paulo Zanoni]

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

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 <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1f601ef..0b03c55 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2875,13 +2875,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);
 }
@@ -3077,6 +3088,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);
@@ -3265,6 +3278,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

[PATCH 15/19] drm/i915: remove ibx_irq_uninstall

[Paulo Zanoni]

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

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

v2: - Rebase.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 0b03c55..b69b7b2 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2864,7 +2864,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;
 
@@ -2920,7 +2920,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)
@@ -2973,7 +2973,7 @@ static void gen8_irq_preinstall(struct drm_device *dev)
 	GEN5_IRQ_RESET(GEN8_DE_MISC_);
 	GEN5_IRQ_RESET(GEN8_PCU_);
 
-	ibx_irq_preinstall(dev);
+	ibx_irq_reset(dev);
 }
 
 static void ibx_hpd_irq_setup(struct drm_device *dev)
@@ -3291,19 +3291,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;
@@ -3326,7 +3313,7 @@ static void gen8_irq_uninstall(struct drm_device *dev)
 	GEN5_IRQ_RESET(GEN8_DE_MISC_);
 	GEN5_IRQ_RESET(GEN8_PCU_);
 
-	ibx_irq_uninstall(dev);
+	ibx_irq_reset(dev);
 }
 
 static void valleyview_irq_uninstall(struct drm_device *dev)
@@ -3377,7 +3364,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)
-- 
1.8.5.3

[PATCH 16/19] drm/i915: add missing intel_hpd_irq_uninstall

[Paulo Zanoni]

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

Missing from gen8_irq_uninstall.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b69b7b2..ee89a25 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3299,6 +3299,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);
-- 
1.8.5.3

[PATCH 17/19] drm/i915: add ironlake_irq_reset

[Paulo Zanoni]

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

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 <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ee89a25..373620c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2908,12 +2908,10 @@ 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)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	I915_WRITE(HWSTAM, 0xeffe);
-
 	GEN5_IRQ_RESET(DE);
 	if (IS_GEN7(dev))
 		I915_WRITE(GEN7_ERR_INT, 0xffffffff);
@@ -2923,6 +2921,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)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3360,13 +3367,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)
-- 
1.8.5.3

[PATCH 18/19] drm/i915: add gen8_irq_reset

[Paulo Zanoni]

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

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

v2: - Rebase.
    - While at it, remove useless { and }.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 373620c..fd1ef09 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2959,7 +2959,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;
@@ -2972,9 +2972,8 @@ static void gen8_irq_preinstall(struct drm_device *dev)
 	GEN8_IRQ_RESET_NDX(GT, 2);
 	GEN8_IRQ_RESET_NDX(GT, 3);
 
-	for_each_pipe(pipe) {
+	for_each_pipe(pipe)
 		GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
-	}
 
 	GEN5_IRQ_RESET(GEN8_DE_PORT_);
 	GEN5_IRQ_RESET(GEN8_DE_MISC_);
@@ -2983,6 +2982,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)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3301,28 +3305,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_);
-
-	ibx_irq_reset(dev);
+	gen8_irq_reset(dev);
 }
 
 static void valleyview_irq_uninstall(struct drm_device *dev)
-- 
1.8.5.3

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

[Paulo Zanoni]

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

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 <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index fd1ef09..75f1997 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2912,6 +2912,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);
@@ -2923,10 +2925,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);
 }
 
@@ -3099,6 +3097,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);
@@ -3354,8 +3354,6 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	intel_hpd_irq_uninstall(dev_priv);
 
-	I915_WRITE(HWSTAM, 0xffffffff);
-
 	ironlake_irq_reset(dev);
 }
 
-- 
1.8.5.3

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

[Daniel Vetter]

On Tue, Apr 01, 2014 at 03:37:27PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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 <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index fd1ef09..75f1997 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2912,6 +2912,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);
> @@ -2923,10 +2925,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);
>  }

Follow-up patch to remove this wrapper functions and just put
foo_irq_reset into the preinstall hooks?

In any case entire series merged (with one tiny rebase conflict since
drm_i915_private_t is now gone).
-Daniel

>  
> @@ -3099,6 +3097,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);
> @@ -3354,8 +3354,6 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>  
>  	intel_hpd_irq_uninstall(dev_priv);
>  
> -	I915_WRITE(HWSTAM, 0xffffffff);
> -
>  	ironlake_irq_reset(dev);
>  }
>  
> -- 
> 1.8.5.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Damien Lespiau]

On Tue, Apr 01, 2014 at 03:37:27PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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 <paulo.r.zanoni@intel.com>

Orthogonal note to this patch, I'm wondering why we enable any interrupt
reporting in the HW status page, I don't see anywhere we read that
information back? Any idea?

-- 
Damien

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index fd1ef09..75f1997 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2912,6 +2912,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);
> @@ -2923,10 +2925,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);
>  }
>  
> @@ -3099,6 +3097,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);
> @@ -3354,8 +3354,6 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>  
>  	intel_hpd_irq_uninstall(dev_priv);
>  
> -	I915_WRITE(HWSTAM, 0xffffffff);
> -
>  	ironlake_irq_reset(dev);
>  }
>  
> -- 
> 1.8.5.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Wed, Apr 02, 2014 at 12:21:45PM +0100, Damien Lespiau wrote:
> On Tue, Apr 01, 2014 at 03:37:27PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > 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 <paulo.r.zanoni@intel.com>
> 
> Orthogonal note to this patch, I'm wondering why we enable any interrupt
> reporting in the HW status page, I don't see anywhere we read that
> information back? Any idea?

Back in the SNB days, interrupts broke without this piece of magic.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Wed, Apr 02, 2014 at 12:23:51PM +0100, Chris Wilson wrote:
> On Wed, Apr 02, 2014 at 12:21:45PM +0100, Damien Lespiau wrote:
> > On Tue, Apr 01, 2014 at 03:37:27PM -0300, Paulo Zanoni wrote:
> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > 
> > > 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 <paulo.r.zanoni@intel.com>
> > 
> > Orthogonal note to this patch, I'm wondering why we enable any interrupt
> > reporting in the HW status page, I don't see anywhere we read that
> > information back? Any idea?
> 
> Back in the SNB days, interrupts broke without this piece of magic.

To be more precise, iirc, it was a step towards getting coherent
breadcrumb writes into the HWS. As it turns out, there were more steps
required. Considering that it can now probably be dropped again? Any
takers?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Paulo Zanoni]

2014-04-02 8:27 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Wed, Apr 02, 2014 at 12:23:51PM +0100, Chris Wilson wrote:
>> On Wed, Apr 02, 2014 at 12:21:45PM +0100, Damien Lespiau wrote:
>> > On Tue, Apr 01, 2014 at 03:37:27PM -0300, Paulo Zanoni wrote:
>> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > >
>> > > 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 <paulo.r.zanoni@intel.com>
>> >
>> > Orthogonal note to this patch, I'm wondering why we enable any interrupt
>> > reporting in the HW status page, I don't see anywhere we read that
>> > information back? Any idea?
>>
>> Back in the SNB days, interrupts broke without this piece of magic.
>
> To be more precise, iirc, it was a step towards getting coherent
> breadcrumb writes into the HWS. As it turns out, there were more steps
> required. Considering that it can now probably be dropped again? Any
> takers?

I noticed the HWSTAM code looks weird, but I don't really know much
about why it's there, so I decided to not add any regressions. I wrote
this patch a long time ago, and gave up on upstreaming it, but in case
anybody wants, I can send it to the list:
http://cgit.freedesktop.org/~pzanoni/linux/commit/?h=c8-wip&id=7ebe245a2c55379ddc7b36f1fb440215c23f1570
. Look at the commit message, it may be an interesting starting point
if anybody wants to dig on the issue...

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni

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

[Chris Wilson]

On Wed, Apr 02, 2014 at 11:14:58AM -0300, Paulo Zanoni wrote:
> 2014-04-02 8:27 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Wed, Apr 02, 2014 at 12:23:51PM +0100, Chris Wilson wrote:
> >> On Wed, Apr 02, 2014 at 12:21:45PM +0100, Damien Lespiau wrote:
> >> > On Tue, Apr 01, 2014 at 03:37:27PM -0300, Paulo Zanoni wrote:
> >> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> > >
> >> > > 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 <paulo.r.zanoni@intel.com>
> >> >
> >> > Orthogonal note to this patch, I'm wondering why we enable any interrupt
> >> > reporting in the HW status page, I don't see anywhere we read that
> >> > information back? Any idea?
> >>
> >> Back in the SNB days, interrupts broke without this piece of magic.
> >
> > To be more precise, iirc, it was a step towards getting coherent
> > breadcrumb writes into the HWS. As it turns out, there were more steps
> > required. Considering that it can now probably be dropped again? Any
> > takers?
> 
> I noticed the HWSTAM code looks weird, but I don't really know much
> about why it's there, so I decided to not add any regressions. I wrote
> this patch a long time ago, and gave up on upstreaming it, but in case
> anybody wants, I can send it to the list:
> http://cgit.freedesktop.org/~pzanoni/linux/commit/?h=c8-wip&id=7ebe245a2c55379ddc7b36f1fb440215c23f1570
> . Look at the commit message, it may be an interesting starting point
> if anybody wants to dig on the issue...

Yup, that is very interesting. That may be the minimal piece of HWSTAM
magic we need, and be an interim step to removing HWSTAM entirely.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Damien Lespiau]

On Wed, Apr 02, 2014 at 04:28:06PM +0100, Chris Wilson wrote:
> On Wed, Apr 02, 2014 at 11:14:58AM -0300, Paulo Zanoni wrote:
> > 2014-04-02 8:27 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > > On Wed, Apr 02, 2014 at 12:23:51PM +0100, Chris Wilson wrote:
> > >> On Wed, Apr 02, 2014 at 12:21:45PM +0100, Damien Lespiau wrote:
> > >> > On Tue, Apr 01, 2014 at 03:37:27PM -0300, Paulo Zanoni wrote:
> > >> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >> > >
> > >> > > 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 <paulo.r.zanoni@intel.com>
> > >> >
> > >> > Orthogonal note to this patch, I'm wondering why we enable any interrupt
> > >> > reporting in the HW status page, I don't see anywhere we read that
> > >> > information back? Any idea?
> > >>
> > >> Back in the SNB days, interrupts broke without this piece of magic.
> > >
> > > To be more precise, iirc, it was a step towards getting coherent
> > > breadcrumb writes into the HWS. As it turns out, there were more steps
> > > required. Considering that it can now probably be dropped again? Any
> > > takers?
> > 
> > I noticed the HWSTAM code looks weird, but I don't really know much
> > about why it's there, so I decided to not add any regressions. I wrote
> > this patch a long time ago, and gave up on upstreaming it, but in case
> > anybody wants, I can send it to the list:
> > http://cgit.freedesktop.org/~pzanoni/linux/commit/?h=c8-wip&id=7ebe245a2c55379ddc7b36f1fb440215c23f1570
> > . Look at the commit message, it may be an interesting starting point
> > if anybody wants to dig on the issue...
> 
> Yup, that is very interesting. That may be the minimal piece of HWSTAM
> magic we need, and be an interim step to removing HWSTAM entirely.

Would you mind resending:
  - a patch with Chris' comment above, that's alone is useful for the
    next guy looking at the code,
  - a patch with the change (Maybe reusing GT_RENDER_USER_INTERRUPT as
    HWSTAM uses the same definitions as the other interrupts registers)?

I'm sure we can get some reviews on the first, hopefully some testing on
the second.

-- 
Damien

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

[Daniel Vetter]

On Wed, Apr 02, 2014 at 05:08:34PM +0100, Damien Lespiau wrote:
> On Wed, Apr 02, 2014 at 04:28:06PM +0100, Chris Wilson wrote:
> > On Wed, Apr 02, 2014 at 11:14:58AM -0300, Paulo Zanoni wrote:
> > > 2014-04-02 8:27 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > > > On Wed, Apr 02, 2014 at 12:23:51PM +0100, Chris Wilson wrote:
> > > >> On Wed, Apr 02, 2014 at 12:21:45PM +0100, Damien Lespiau wrote:
> > > >> > On Tue, Apr 01, 2014 at 03:37:27PM -0300, Paulo Zanoni wrote:
> > > >> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > >> > >
> > > >> > > 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 <paulo.r.zanoni@intel.com>
> > > >> >
> > > >> > Orthogonal note to this patch, I'm wondering why we enable any interrupt
> > > >> > reporting in the HW status page, I don't see anywhere we read that
> > > >> > information back? Any idea?
> > > >>
> > > >> Back in the SNB days, interrupts broke without this piece of magic.
> > > >
> > > > To be more precise, iirc, it was a step towards getting coherent
> > > > breadcrumb writes into the HWS. As it turns out, there were more steps
> > > > required. Considering that it can now probably be dropped again? Any
> > > > takers?
> > > 
> > > I noticed the HWSTAM code looks weird, but I don't really know much
> > > about why it's there, so I decided to not add any regressions. I wrote
> > > this patch a long time ago, and gave up on upstreaming it, but in case
> > > anybody wants, I can send it to the list:
> > > http://cgit.freedesktop.org/~pzanoni/linux/commit/?h=c8-wip&id=7ebe245a2c55379ddc7b36f1fb440215c23f1570
> > > . Look at the commit message, it may be an interesting starting point
> > > if anybody wants to dig on the issue...
> > 
> > Yup, that is very interesting. That may be the minimal piece of HWSTAM
> > magic we need, and be an interim step to removing HWSTAM entirely.
> 
> Would you mind resending:
>   - a patch with Chris' comment above, that's alone is useful for the
>     next guy looking at the code,
>   - a patch with the change (Maybe reusing GT_RENDER_USER_INTERRUPT as
>     HWSTAM uses the same definitions as the other interrupts registers)?
> 
> I'm sure we can get some reviews on the first, hopefully some testing on
> the second.

Seconded. Documenting what we're doing is always good ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch