[PATCH 00/15] Unify interrupt register init/reset

[PATCH 00/15] Unify interrupt register init/reset

[Paulo Zanoni]

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

Hi

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

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

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

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

Thanks,
Paulo

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

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

-- 
1.8.1.2

[PATCH 01/15] drm/i915: add INTEL_IRQ_REG_RESET

[Paulo Zanoni]

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

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

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

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index f708e4e..351e30a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -79,6 +79,15 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
+#define INTEL_IRQ_REG_RESET(type, do_iir) do { \
+	I915_WRITE(type##MR, 0xffffffff); \
+	I915_WRITE(type##ER, 0); \
+	if (do_iir) \
+		I915_WRITE(type##IR, I915_READ(type##IR)); \
+	else \
+		POSTING_READ(type##ER); \
+} while (0)
+
 /* For display hotplug interrupt */
 static void
 ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
@@ -2003,17 +2012,10 @@ static void gen5_gt_irq_preinstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	/* and GT */
-	I915_WRITE(GTIMR, 0xffffffff);
-	I915_WRITE(GTIER, 0x0);
-	POSTING_READ(GTIER);
+	INTEL_IRQ_REG_RESET(GTI, false);
 
-	if (INTEL_INFO(dev)->gen >= 6) {
-		/* and PM */
-		I915_WRITE(GEN6_PMIMR, 0xffffffff);
-		I915_WRITE(GEN6_PMIER, 0x0);
-		POSTING_READ(GEN6_PMIER);
-	}
+	if (INTEL_INFO(dev)->gen >= 6)
+		INTEL_IRQ_REG_RESET(GEN6_PMI, false);
 }
 
 /* drm_dma.h hooks
@@ -2026,9 +2028,7 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 
 	I915_WRITE(HWSTAM, 0xeffe);
 
-	I915_WRITE(DEIMR, 0xffffffff);
-	I915_WRITE(DEIER, 0x0);
-	POSTING_READ(DEIER);
+	INTEL_IRQ_REG_RESET(DEI, false);
 
 	gen5_gt_irq_preinstall(dev);
 
@@ -2061,9 +2061,7 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
 	I915_WRITE(VLV_IIR, 0xffffffff);
-	I915_WRITE(VLV_IMR, 0xffffffff);
-	I915_WRITE(VLV_IER, 0x0);
-	POSTING_READ(VLV_IER);
+	INTEL_IRQ_REG_RESET(VLV_I, false);
 }
 
 static void ibx_hpd_irq_setup(struct drm_device *dev)
@@ -2286,9 +2284,7 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
 	I915_WRITE(VLV_IIR, 0xffffffff);
-	I915_WRITE(VLV_IMR, 0xffffffff);
-	I915_WRITE(VLV_IER, 0x0);
-	POSTING_READ(VLV_IER);
+	INTEL_IRQ_REG_RESET(VLV_I, false);
 }
 
 static void ironlake_irq_uninstall(struct drm_device *dev)
@@ -2302,22 +2298,16 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	I915_WRITE(HWSTAM, 0xffffffff);
 
-	I915_WRITE(DEIMR, 0xffffffff);
-	I915_WRITE(DEIER, 0x0);
-	I915_WRITE(DEIIR, I915_READ(DEIIR));
+	INTEL_IRQ_REG_RESET(DEI, true);
 	if (IS_GEN7(dev))
 		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
 
-	I915_WRITE(GTIMR, 0xffffffff);
-	I915_WRITE(GTIER, 0x0);
-	I915_WRITE(GTIIR, I915_READ(GTIIR));
+	INTEL_IRQ_REG_RESET(GTI, true);
 
 	if (HAS_PCH_NOP(dev))
 		return;
 
-	I915_WRITE(SDEIMR, 0xffffffff);
-	I915_WRITE(SDEIER, 0x0);
-	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
+	INTEL_IRQ_REG_RESET(SDEI, true);
 	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
 		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
 }
@@ -2491,9 +2481,7 @@ static void i915_irq_preinstall(struct drm_device * dev)
 	I915_WRITE16(HWSTAM, 0xeffe);
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0);
-	I915_WRITE(IMR, 0xffffffff);
-	I915_WRITE(IER, 0x0);
-	POSTING_READ(IER);
+	INTEL_IRQ_REG_RESET(I, false);
 }
 
 static int i915_irq_postinstall(struct drm_device *dev)
@@ -2693,10 +2681,8 @@ static void i915_irq_uninstall(struct drm_device * dev)
 		I915_WRITE(PIPESTAT(pipe), 0);
 		I915_WRITE(PIPESTAT(pipe), I915_READ(PIPESTAT(pipe)));
 	}
-	I915_WRITE(IMR, 0xffffffff);
-	I915_WRITE(IER, 0x0);
 
-	I915_WRITE(IIR, I915_READ(IIR));
+	INTEL_IRQ_REG_RESET(I, true);
 }
 
 static void i965_irq_preinstall(struct drm_device * dev)
@@ -2712,9 +2698,7 @@ static void i965_irq_preinstall(struct drm_device * dev)
 	I915_WRITE(HWSTAM, 0xeffe);
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0);
-	I915_WRITE(IMR, 0xffffffff);
-	I915_WRITE(IER, 0x0);
-	POSTING_READ(IER);
+	INTEL_IRQ_REG_RESET(I, false);
 }
 
 static int i965_irq_postinstall(struct drm_device *dev)
-- 
1.8.1.2

[PATCH 02/15] drm/i915: change how VLV_IIR is reset

[Paulo Zanoni]

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 351e30a..292337b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2060,8 +2060,7 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
-	I915_WRITE(VLV_IIR, 0xffffffff);
-	INTEL_IRQ_REG_RESET(VLV_I, false);
+	INTEL_IRQ_REG_RESET(VLV_I, true);
 }
 
 static void ibx_hpd_irq_setup(struct drm_device *dev)
@@ -2283,8 +2282,7 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
-	I915_WRITE(VLV_IIR, 0xffffffff);
-	INTEL_IRQ_REG_RESET(VLV_I, false);
+	INTEL_IRQ_REG_RESET(VLV_I, true);
 }
 
 static void ironlake_irq_uninstall(struct drm_device *dev)
-- 
1.8.1.2

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

[Paulo Zanoni]

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 292337b..b1b6552 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2920,13 +2920,11 @@ static void i965_irq_uninstall(struct drm_device * dev)
 	I915_WRITE(HWSTAM, 0xffffffff);
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0);
-	I915_WRITE(IMR, 0xffffffff);
-	I915_WRITE(IER, 0x0);
+	INTEL_IRQ_REG_RESET(I, true);
 
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe),
 			   I915_READ(PIPESTAT(pipe)) & 0x8000ffff);
-	I915_WRITE(IIR, I915_READ(IIR));
 }
 
 static void i915_reenable_hotplug_timer_func(unsigned long data)
-- 
1.8.1.2

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

[Paulo Zanoni]

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

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b1b6552..29eac7a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -82,10 +82,14 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 #define INTEL_IRQ_REG_RESET(type, do_iir) do { \
 	I915_WRITE(type##MR, 0xffffffff); \
 	I915_WRITE(type##ER, 0); \
-	if (do_iir) \
-		I915_WRITE(type##IR, I915_READ(type##IR)); \
-	else \
-		POSTING_READ(type##ER); \
+	POSTING_READ(type##ER); \
+	if (do_iir) { \
+		I915_WRITE(type##IR, 0xffffffff); \
+		if (I915_READ(type##IR)) { \
+			I915_WRITE(type##IR, 0xffffffff); \
+			POSTING_READ(type##IR); \
+		} \
+	} \
 } while (0)
 
 /* For display hotplug interrupt */
-- 
1.8.1.2

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

[Paulo Zanoni]

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 29eac7a..e416848 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -92,6 +92,14 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	} \
 } while (0)
 
+#define INTEL_IRQ_REG_INIT(type, do_iir, ier_val, imr_val) do { \
+	if (do_iir) \
+		I915_WRITE(type##IR, I915_READ(type##IR)); \
+	I915_WRITE(type##MR, (imr_val)); \
+	I915_WRITE(type##ER, (ier_val)); \
+	POSTING_READ(type##ER); \
+} while (0)
+
 /* For display hotplug interrupt */
 static void
 ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
@@ -2145,10 +2153,7 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
 		gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
 	}
 
-	I915_WRITE(GTIIR, I915_READ(GTIIR));
-	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
-	I915_WRITE(GTIER, gt_irqs);
-	POSTING_READ(GTIER);
+	INTEL_IRQ_REG_INIT(GTI, true, gt_irqs, dev_priv->gt_irq_mask);
 
 	if (INTEL_INFO(dev)->gen >= 6) {
 		pm_irqs |= GEN6_PM_RPS_EVENTS;
@@ -2156,10 +2161,7 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
 		if (HAS_VEBOX(dev))
 			pm_irqs |= PM_VEBOX_USER_INTERRUPT;
 
-		I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
-		I915_WRITE(GEN6_PMIMR, 0xffffffff);
-		I915_WRITE(GEN6_PMIER, pm_irqs);
-		POSTING_READ(GEN6_PMIER);
+		INTEL_IRQ_REG_INIT(GEN6_PMI, true, pm_irqs, 0xffffffff);
 	}
 }
 
@@ -2189,11 +2191,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 
 	dev_priv->irq_mask = ~display_mask;
 
-	/* should always can generate irq */
-	I915_WRITE(DEIIR, I915_READ(DEIIR));
-	I915_WRITE(DEIMR, dev_priv->irq_mask);
-	I915_WRITE(DEIER, display_mask | extra_mask);
-	POSTING_READ(DEIER);
+	INTEL_IRQ_REG_INIT(DEI, true, display_mask | extra_mask,
+			   dev_priv->irq_mask);
 
 	gen5_gt_irq_postinstall(dev);
 
@@ -2519,9 +2518,7 @@ static int i915_irq_postinstall(struct drm_device *dev)
 		dev_priv->irq_mask &= ~I915_DISPLAY_PORT_INTERRUPT;
 	}
 
-	I915_WRITE(IMR, dev_priv->irq_mask);
-	I915_WRITE(IER, enable_mask);
-	POSTING_READ(IER);
+	INTEL_IRQ_REG_INIT(I, false, enable_mask, dev_priv->irq_mask);
 
 	i915_enable_asle_pipestat(dev);
 
@@ -2751,6 +2748,7 @@ static int i965_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(IMR, dev_priv->irq_mask);
 	I915_WRITE(IER, enable_mask);
 	POSTING_READ(IER);
+	INTEL_IRQ_REG_INIT(I, false, enable_mask, dev_priv->irq_mask);
 
 	I915_WRITE(PORT_HOTPLUG_EN, 0);
 	POSTING_READ(PORT_HOTPLUG_EN);
-- 
1.8.1.2

[PATCH 06/15] drm/i915: use INTEL_IRQ_REG_INIT on VLV too

[Paulo Zanoni]

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e416848..37420b5 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2236,12 +2236,9 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(PORT_HOTPLUG_EN, 0);
 	POSTING_READ(PORT_HOTPLUG_EN);
 
-	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
-	I915_WRITE(VLV_IER, enable_mask);
-	I915_WRITE(VLV_IIR, 0xffffffff);
 	I915_WRITE(PIPESTAT(0), 0xffff);
 	I915_WRITE(PIPESTAT(1), 0xffff);
-	POSTING_READ(VLV_IER);
+	INTEL_IRQ_REG_INIT(VLV_I, true, enable_mask, dev_priv->irq_mask);
 
 	/* Interrupt setup is already guaranteed to be single-threaded, this is
 	 * just to make the assert_spin_locked check happy. */
-- 
1.8.1.2

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

[Paulo Zanoni]

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 37420b5..400aa63 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -79,16 +79,13 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
-#define INTEL_IRQ_REG_RESET(type, do_iir) do { \
+#define INTEL_IRQ_REG_RESET(type) do { \
 	I915_WRITE(type##MR, 0xffffffff); \
 	I915_WRITE(type##ER, 0); \
-	POSTING_READ(type##ER); \
-	if (do_iir) { \
+	I915_WRITE(type##IR, 0xffffffff); \
+	if (I915_READ(type##IR)) { \
 		I915_WRITE(type##IR, 0xffffffff); \
-		if (I915_READ(type##IR)) { \
-			I915_WRITE(type##IR, 0xffffffff); \
-			POSTING_READ(type##IR); \
-		} \
+		POSTING_READ(type##IR); \
 	} \
 } while (0)
 
@@ -2024,10 +2021,10 @@ static void gen5_gt_irq_preinstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	INTEL_IRQ_REG_RESET(GTI, false);
+	INTEL_IRQ_REG_RESET(GTI);
 
 	if (INTEL_INFO(dev)->gen >= 6)
-		INTEL_IRQ_REG_RESET(GEN6_PMI, false);
+		INTEL_IRQ_REG_RESET(GEN6_PMI);
 }
 
 /* drm_dma.h hooks
@@ -2040,7 +2037,7 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 
 	I915_WRITE(HWSTAM, 0xeffe);
 
-	INTEL_IRQ_REG_RESET(DEI, false);
+	INTEL_IRQ_REG_RESET(DEI);
 
 	gen5_gt_irq_preinstall(dev);
 
@@ -2072,7 +2069,7 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
-	INTEL_IRQ_REG_RESET(VLV_I, true);
+	INTEL_IRQ_REG_RESET(VLV_I);
 }
 
 static void ibx_hpd_irq_setup(struct drm_device *dev)
@@ -2282,7 +2279,7 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
-	INTEL_IRQ_REG_RESET(VLV_I, true);
+	INTEL_IRQ_REG_RESET(VLV_I);
 }
 
 static void ironlake_irq_uninstall(struct drm_device *dev)
@@ -2296,16 +2293,16 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	I915_WRITE(HWSTAM, 0xffffffff);
 
-	INTEL_IRQ_REG_RESET(DEI, true);
+	INTEL_IRQ_REG_RESET(DEI);
 	if (IS_GEN7(dev))
 		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
 
-	INTEL_IRQ_REG_RESET(GTI, true);
+	INTEL_IRQ_REG_RESET(GTI);
 
 	if (HAS_PCH_NOP(dev))
 		return;
 
-	INTEL_IRQ_REG_RESET(SDEI, true);
+	INTEL_IRQ_REG_RESET(SDEI);
 	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
 		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
 }
@@ -2479,7 +2476,7 @@ static void i915_irq_preinstall(struct drm_device * dev)
 	I915_WRITE16(HWSTAM, 0xeffe);
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0);
-	INTEL_IRQ_REG_RESET(I, false);
+	INTEL_IRQ_REG_RESET(I);
 }
 
 static int i915_irq_postinstall(struct drm_device *dev)
@@ -2678,7 +2675,7 @@ static void i915_irq_uninstall(struct drm_device * dev)
 		I915_WRITE(PIPESTAT(pipe), I915_READ(PIPESTAT(pipe)));
 	}
 
-	INTEL_IRQ_REG_RESET(I, true);
+	INTEL_IRQ_REG_RESET(I);
 }
 
 static void i965_irq_preinstall(struct drm_device * dev)
@@ -2694,7 +2691,7 @@ static void i965_irq_preinstall(struct drm_device * dev)
 	I915_WRITE(HWSTAM, 0xeffe);
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0);
-	INTEL_IRQ_REG_RESET(I, false);
+	INTEL_IRQ_REG_RESET(I);
 }
 
 static int i965_irq_postinstall(struct drm_device *dev)
@@ -2919,7 +2916,7 @@ static void i965_irq_uninstall(struct drm_device * dev)
 	I915_WRITE(HWSTAM, 0xffffffff);
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0);
-	INTEL_IRQ_REG_RESET(I, true);
+	INTEL_IRQ_REG_RESET(I);
 
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe),
-- 
1.8.1.2

[PATCH 08/15] drm/i915: WARN if IIR is not zero at irq_postinstall

[Paulo Zanoni]

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

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

Signed-off-by: Paulo Zanoni <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 400aa63..6793b30 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -90,8 +90,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 } while (0)
 
 #define INTEL_IRQ_REG_INIT(type, do_iir, ier_val, imr_val) do { \
-	if (do_iir) \
-		I915_WRITE(type##IR, I915_READ(type##IR)); \
+	WARN(I915_READ(type##IR) != 0, "Register 0x%x is not 0\n", type##IR); \
 	I915_WRITE(type##MR, (imr_val)); \
 	I915_WRITE(type##ER, (ier_val)); \
 	POSTING_READ(type##ER); \
-- 
1.8.1.2

[PATCH 09/15] drm/i915: remove additional zerogin of VLV_IIR at postinstall

[Paulo Zanoni]

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6793b30..5ca50a0 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2244,9 +2244,6 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
 	i915_enable_pipestat(dev_priv, 1, pipestat_enable);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
-	I915_WRITE(VLV_IIR, 0xffffffff);
-	I915_WRITE(VLV_IIR, 0xffffffff);
-
 	gen5_gt_irq_postinstall(dev);
 
 	/* ack & enable invalid PTE error interrupts */
-- 
1.8.1.2

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

[Paulo Zanoni]

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 5ca50a0..9684f1e 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2056,10 +2056,6 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(RING_IMR(GEN6_BSD_RING_BASE), 0);
 	I915_WRITE(RING_IMR(BLT_RING_BASE), 0);
 
-	/* and GT */
-	I915_WRITE(GTIIR, I915_READ(GTIIR));
-	I915_WRITE(GTIIR, I915_READ(GTIIR));
-
 	gen5_gt_irq_preinstall(dev);
 
 	I915_WRITE(DPINVGTT, 0xff);
-- 
1.8.1.2

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

[Paulo Zanoni]

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9684f1e..7588ad3 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -79,6 +79,15 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
+#define INTEL_IRQ_REG_RESET16(type, do_iir) do { \
+	I915_WRITE16(type##MR, 0xffff); \
+	I915_WRITE16(type##ER, 0); \
+	if (do_iir) \
+		I915_WRITE16(type##IR, I915_READ16(type##IR)); \
+	else \
+		POSTING_READ16(type##ER); \
+} while (0)
+
 #define INTEL_IRQ_REG_RESET(type) do { \
 	I915_WRITE(type##MR, 0xffffffff); \
 	I915_WRITE(type##ER, 0); \
@@ -2308,9 +2317,7 @@ static void i8xx_irq_preinstall(struct drm_device * dev)
 
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0);
-	I915_WRITE16(IMR, 0xffff);
-	I915_WRITE16(IER, 0x0);
-	POSTING_READ16(IER);
+	INTEL_IRQ_REG_RESET16(I, false);
 }
 
 static int i8xx_irq_postinstall(struct drm_device *dev)
@@ -2448,9 +2455,7 @@ static void i8xx_irq_uninstall(struct drm_device * dev)
 		I915_WRITE(PIPESTAT(pipe), 0);
 		I915_WRITE(PIPESTAT(pipe), I915_READ(PIPESTAT(pipe)));
 	}
-	I915_WRITE16(IMR, 0xffff);
-	I915_WRITE16(IER, 0x0);
-	I915_WRITE16(IIR, I915_READ16(IIR));
+	INTEL_IRQ_REG_RESET16(I, true);
 }
 
 static void i915_irq_preinstall(struct drm_device * dev)
-- 
1.8.1.2

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

[Paulo Zanoni]

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 7588ad3..a936c59 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -82,10 +82,14 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 #define INTEL_IRQ_REG_RESET16(type, do_iir) do { \
 	I915_WRITE16(type##MR, 0xffff); \
 	I915_WRITE16(type##ER, 0); \
-	if (do_iir) \
-		I915_WRITE16(type##IR, I915_READ16(type##IR)); \
-	else \
-		POSTING_READ16(type##ER); \
+	POSTING_READ16(type##ER); \
+	if (do_iir) { \
+		I915_WRITE16(type##IR, 0xffff); \
+		if (I915_READ16(type##IR)) { \
+			I915_WRITE16(type##IR, 0xffff); \
+			POSTING_READ16(type##IR); \
+		} \
+	} \
 } while (0)
 
 #define INTEL_IRQ_REG_RESET(type) do { \
-- 
1.8.1.2

[PATCH 13/15] drm/i915: add INTEL_IRQ_REG_INIT16

[Paulo Zanoni]

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

Same thing as INTEL_IRQ_REG_INIT, but on a separate patch.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a936c59..d32042d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -102,6 +102,14 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	} \
 } while (0)
 
+#define INTEL_IRQ_REG_INIT16(type, do_iir, ier_val, imr_val) do { \
+	if (do_iir) \
+		I915_WRITE16(type##IR, I915_READ16(type##IR)); \
+	I915_WRITE16(type##MR, (imr_val)); \
+	I915_WRITE16(type##ER, (ier_val)); \
+	POSTING_READ16(type##ER); \
+} while (0)
+
 #define INTEL_IRQ_REG_INIT(type, do_iir, ier_val, imr_val) do { \
 	WARN(I915_READ(type##IR) != 0, "Register 0x%x is not 0\n", type##IR); \
 	I915_WRITE(type##MR, (imr_val)); \
@@ -2327,6 +2335,10 @@ static void i8xx_irq_preinstall(struct drm_device * dev)
 static int i8xx_irq_postinstall(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
+	uint32_t enable_mask = (I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
+				I915_DISPLAY_PIPE_B_EVENT_INTERRUPT |
+				I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT |
+				I915_USER_INTERRUPT);
 
 	I915_WRITE16(EMR,
 		     ~(I915_ERROR_PAGE_TABLE | I915_ERROR_MEMORY_REFRESH));
@@ -2338,14 +2350,8 @@ static int i8xx_irq_postinstall(struct drm_device *dev)
 		  I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
 		  I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT |
 		  I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT);
-	I915_WRITE16(IMR, dev_priv->irq_mask);
-
-	I915_WRITE16(IER,
-		     I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
-		     I915_DISPLAY_PIPE_B_EVENT_INTERRUPT |
-		     I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT |
-		     I915_USER_INTERRUPT);
-	POSTING_READ16(IER);
+
+	INTEL_IRQ_REG_INIT16(I, false, enable_mask, dev_priv->irq_mask);
 
 	return 0;
 }
-- 
1.8.1.2

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

[Paulo Zanoni]

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

Same thing as the equivalent commit that touched INTEL_IRQ_REG_RESET.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d32042d..4f0bc26 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -79,16 +79,13 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
-#define INTEL_IRQ_REG_RESET16(type, do_iir) do { \
+#define INTEL_IRQ_REG_RESET16(type) do { \
 	I915_WRITE16(type##MR, 0xffff); \
 	I915_WRITE16(type##ER, 0); \
-	POSTING_READ16(type##ER); \
-	if (do_iir) { \
+	I915_WRITE16(type##IR, 0xffff); \
+	if (I915_READ16(type##IR)) { \
 		I915_WRITE16(type##IR, 0xffff); \
-		if (I915_READ16(type##IR)) { \
-			I915_WRITE16(type##IR, 0xffff); \
-			POSTING_READ16(type##IR); \
-		} \
+		POSTING_READ16(type##IR); \
 	} \
 } while (0)
 
@@ -2329,7 +2326,7 @@ static void i8xx_irq_preinstall(struct drm_device * dev)
 
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0);
-	INTEL_IRQ_REG_RESET16(I, false);
+	INTEL_IRQ_REG_RESET16(I);
 }
 
 static int i8xx_irq_postinstall(struct drm_device *dev)
@@ -2465,7 +2462,7 @@ static void i8xx_irq_uninstall(struct drm_device * dev)
 		I915_WRITE(PIPESTAT(pipe), 0);
 		I915_WRITE(PIPESTAT(pipe), I915_READ(PIPESTAT(pipe)));
 	}
-	INTEL_IRQ_REG_RESET16(I, true);
+	INTEL_IRQ_REG_RESET16(I);
 }
 
 static void i915_irq_preinstall(struct drm_device * dev)
-- 
1.8.1.2

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

[Paulo Zanoni]

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

Same thing as the previous non-i8xx commit.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4f0bc26..8b12caa 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -100,8 +100,8 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 } while (0)
 
 #define INTEL_IRQ_REG_INIT16(type, do_iir, ier_val, imr_val) do { \
-	if (do_iir) \
-		I915_WRITE16(type##IR, I915_READ16(type##IR)); \
+	WARN(I915_READ16(type##IR) != 0, "Register 0x%x is not 0\n", \
+	     type##IR); \
 	I915_WRITE16(type##MR, (imr_val)); \
 	I915_WRITE16(type##ER, (ier_val)); \
 	POSTING_READ16(type##ER); \
-- 
1.8.1.2

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

[Chris Wilson]

On Tue, Jul 23, 2013 at 07:33:43PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The problem here is that we have the PIPESTAT registers between IER
> and IIR, so when we use intel_irq_reg_reset we flip the order used to
> reset IIR and PIPESTAT. That should be safe since after we clear
> IMR/IER we won't get any other IIR/PIPESTAT interrupts. Still, the
> change is on its own patch, so it should be easy to bisect and revert
> if needed.

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

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Tue, Jul 23, 2013 at 07:33:44PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> As written on our docs, the IIR registers are capable of storing 2
> interrupts, so if we write once to them there's no guarantee they will
> become zero. So on this patch we write to the register, read to check
> if it's zero, and then write again in case it's needed.
> 
> Also replace I915_WRITE(iir, I915_READ(iir)) with I915_WRITE(iir,
> 0xffffffff), and then move the POSTING_READs on IER because we removed
> the extra IIR read.

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

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Tue, Jul 23, 2013 at 07:33:45PM -0300, Paulo Zanoni wrote:
> @@ -2519,9 +2518,7 @@ static int i915_irq_postinstall(struct drm_device *dev)
>  		dev_priv->irq_mask &= ~I915_DISPLAY_PORT_INTERRUPT;
>  	}
>  
> -	I915_WRITE(IMR, dev_priv->irq_mask);
> -	I915_WRITE(IER, enable_mask);
> -	POSTING_READ(IER);
> +	INTEL_IRQ_REG_INIT(I, false, enable_mask, dev_priv->irq_mask);
>  
>  	i915_enable_asle_pipestat(dev);
>  
> @@ -2751,6 +2748,7 @@ static int i965_irq_postinstall(struct drm_device *dev)
>  	I915_WRITE(IMR, dev_priv->irq_mask);
>  	I915_WRITE(IER, enable_mask);
>  	POSTING_READ(IER);
> +	INTEL_IRQ_REG_INIT(I, false, enable_mask, dev_priv->irq_mask);
>  
>  	I915_WRITE(PORT_HOTPLUG_EN, 0);
>  	POSTING_READ(PORT_HOTPLUG_EN);

Change missing?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Tue, Jul 23, 2013 at 07:33:47PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The long-term goal is to make preinstall and uninstall almost the
> same thing, so we will be able to call uninstall and then postinstall
> if needed.

write(read(iir)) as before
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Tue, Jul 23, 2013 at 07:33:51PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Same thing as INTEL_IRQ_REG_RESET, but now on a separate patch, as
> requested by Ben.

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 00/15] Unify interrupt register init/reset

[Daniel Vetter]

On Tue, Jul 23, 2013 at 07:33:40PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Hi
> 
> This patch series is based on emails I sent a few days ago, with subject
> "proposals/questions about the IRQ registers". The biggest goal of the series is
> to have an unified way to initialize/reset IRQ regsiters. After this series,
> when you add new interrupts to our driver you won't need to think about doing
> the correct reads, writes and posting_reads, all you'll need to worry about is
> to call the macros everyone else calls. So now either all the registers are
> initialized correctly or all the registers are initialized wrongly.
> 
> The first 10 patches do all the rework on i915+. The changes should be
> incremental and any non-trivial changes are on separate patches, so bisecting
> regressions should be very easy. It is worth mentioning that patches 2, 6, 9 and
> 10 touch VLV-specific code, and I don't have a way to test this code.
> 
> On my intial patches the i8xx code was changed along with the i915+ code, but
> Ben suggested I shouldn't be touching i8xx code, so patches 11-15 do the i8xx
> work.  I don't know if we really want these patches, but they're here anyway, so
> feel free to give them a NACK, I won't complain. In case we decide to merge the
> patches, git bisect should easily spot regressions.
> 
> Patches tested on Haswell (constant use for 2 weeks), SandyBridge (a few hours
> of usage) and Atom (only booted and then suspend/resumed once).

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

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

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

Cheers, Daniel
> 

> Thanks,
> Paulo
> 
> Paulo Zanoni (15):
>   drm/i915: add INTEL_IRQ_REG_RESET
>   drm/i915: change how VLV_IIR is reset
>   drm/i915: port i965_irq_uninstall go INTEL_IRQ_REG_RESET
>   drm/i915: really clear the IIR registers
>   drm/i915: add INTEL_IRQ_REG_INIT
>   drm/i915: use INTEL_IRQ_REG_INIT on VLV too
>   drm/i915: reset the IIR registers at preinstall
>   drm/i915: WARN if IIR is not zero at irq_postinstall
>   drm/i915: remove additional zerogin of VLV_IIR at postinstall
>   drm/i915: remove extra clearing of GTIIR from VLV irq preinstall
>   drm/i915: add INTEL_IRQ_REG_RESET16
>   drm/i915: really clear the IIR registers on i8xx
>   drm/i915: add INTEL_IRQ_REG_INIT16
>   drm/i915: reset the i8xx IIR registers at preinstall
>   drm/i915: WARN if IIR is not zero at i8xx irq_postinstall
> 
>  drivers/gpu/drm/i915/i915_irq.c | 144 +++++++++++++++++-----------------------
>  1 file changed, 62 insertions(+), 82 deletions(-)
> 
> -- 
> 1.8.1.2
> 
> _______________________________________________
> 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 04/15] drm/i915: really clear the IIR registers

[Paulo Zanoni]

2013/7/24 Chris Wilson <chris@chris-wilson.co.uk>:
> On Tue, Jul 23, 2013 at 07:33:44PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> As written on our docs, the IIR registers are capable of storing 2
>> interrupts, so if we write once to them there's no guarantee they will
>> become zero. So on this patch we write to the register, read to check
>> if it's zero, and then write again in case it's needed.
>>
>> Also replace I915_WRITE(iir, I915_READ(iir)) with I915_WRITE(iir,
>> 0xffffffff), and then move the POSTING_READs on IER because we removed
>> the extra IIR read.
>
> Just write(read(iir)) twice and add a comment why.

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


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



-- 
Paulo Zanoni

Re: [PATCH 00/15] Unify interrupt register init/reset

[Paulo Zanoni]

2013/7/24 Daniel Vetter <daniel@ffwll.ch>:
> On Tue, Jul 23, 2013 at 07:33:40PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Hi
>>
>> This patch series is based on emails I sent a few days ago, with subject
>> "proposals/questions about the IRQ registers". The biggest goal of the series is
>> to have an unified way to initialize/reset IRQ regsiters. After this series,
>> when you add new interrupts to our driver you won't need to think about doing
>> the correct reads, writes and posting_reads, all you'll need to worry about is
>> to call the macros everyone else calls. So now either all the registers are
>> initialized correctly or all the registers are initialized wrongly.
>>
>> The first 10 patches do all the rework on i915+. The changes should be
>> incremental and any non-trivial changes are on separate patches, so bisecting
>> regressions should be very easy. It is worth mentioning that patches 2, 6, 9 and
>> 10 touch VLV-specific code, and I don't have a way to test this code.
>>
>> On my intial patches the i8xx code was changed along with the i915+ code, but
>> Ben suggested I shouldn't be touching i8xx code, so patches 11-15 do the i8xx
>> work.  I don't know if we really want these patches, but they're here anyway, so
>> feel free to give them a NACK, I won't complain. In case we decide to merge the
>> patches, git bisect should easily spot regressions.
>>
>> Patches tested on Haswell (constant use for 2 weeks), SandyBridge (a few hours
>> of usage) and Atom (only booted and then suspend/resumed once).
>
> My first question was why you didn't use name##IMR, name##IER, name##IIR
> in the macros, since that would match what we have in -internal and imo
> would look more natural. But then I've noticed that for IIR, IER & IMR we
> don't have a prefix ... I still think the I in the name looks a bit funny
> though (e.g. GTI). Maybe we should add an I915_ prefix then we could
> remove the I.

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


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

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

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

Will do.

Thanks Daniel and Chris for the fast reviews!

>
> Cheers, Daniel
>>
>
>> Thanks,
>> Paulo
>>
>> Paulo Zanoni (15):
>>   drm/i915: add INTEL_IRQ_REG_RESET
>>   drm/i915: change how VLV_IIR is reset
>>   drm/i915: port i965_irq_uninstall go INTEL_IRQ_REG_RESET
>>   drm/i915: really clear the IIR registers
>>   drm/i915: add INTEL_IRQ_REG_INIT
>>   drm/i915: use INTEL_IRQ_REG_INIT on VLV too
>>   drm/i915: reset the IIR registers at preinstall
>>   drm/i915: WARN if IIR is not zero at irq_postinstall
>>   drm/i915: remove additional zerogin of VLV_IIR at postinstall
>>   drm/i915: remove extra clearing of GTIIR from VLV irq preinstall
>>   drm/i915: add INTEL_IRQ_REG_RESET16
>>   drm/i915: really clear the IIR registers on i8xx
>>   drm/i915: add INTEL_IRQ_REG_INIT16
>>   drm/i915: reset the i8xx IIR registers at preinstall
>>   drm/i915: WARN if IIR is not zero at i8xx irq_postinstall
>>
>>  drivers/gpu/drm/i915/i915_irq.c | 144 +++++++++++++++++-----------------------
>>  1 file changed, 62 insertions(+), 82 deletions(-)
>>
>> --
>> 1.8.1.2
>>
>> _______________________________________________
>> 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



-- 
Paulo Zanoni

Re: [PATCH 00/15] Unify interrupt register init/reset

[Daniel Vetter]

On Wed, Jul 24, 2013 at 3:10 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> My first question was why you didn't use name##IMR, name##IER, name##IIR
>> in the macros, since that would match what we have in -internal and imo
>> would look more natural. But then I've noticed that for IIR, IER & IMR we
>> don't have a prefix ... I still think the I in the name looks a bit funny
>> though (e.g. GTI). Maybe we should add an I915_ prefix then we could
>> remove the I.
>
> Yeah, you guessed the reason. If we use name##IMR then we could
> replace "INTEL_IRQ_REG_INIT(I, false, enable_mask,
> dev_priv->irq_mask);" with "INTEL_IRQ_REG_INIT(, false, enable_mask,
> dev_priv->irq_mask);", but I guess your suggestion of renaming is
> probably better. The problem with renaming to I915_IIR is that the
> registers also exists on i8xx, so I'd vote for another name
> (GEN2_IIR?).

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

>> The other thing I'm unclear about is the do_iir flag, which in the end
>> seems to clear IIR bits in uninstall but not in preinstall. Iirc Ben's
>> proposal was to do the double IIR clear in preinstall, and I tend to agree
>> with him. Can you please elaborate on this?
>
> Please look at the final result of the series, we remove the flag on
> later patches (you just made me notice I forgot to remove it on the
> reg_init functions, but we don't really use it anymore). At the end of
> the series we do the double-IIR clear at both preinstall and
> uninstall. I wanted to keep one functional change at each patch.

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

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

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

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

[Chris Wilson]

On Wed, Jul 24, 2013 at 10:00:49AM -0300, Paulo Zanoni wrote:
> 2013/7/24 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Tue, Jul 23, 2013 at 07:33:44PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> As written on our docs, the IIR registers are capable of storing 2
> >> interrupts, so if we write once to them there's no guarantee they will
> >> become zero. So on this patch we write to the register, read to check
> >> if it's zero, and then write again in case it's needed.
> >>
> >> Also replace I915_WRITE(iir, I915_READ(iir)) with I915_WRITE(iir,
> >> 0xffffffff), and then move the POSTING_READs on IER because we removed
> >> the extra IIR read.
> >
> > Just write(read(iir)) twice and add a comment why.
> 
> Just to clarify: why not write(iir, 0xffffffff) twice to save the 2 reads?

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

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Paulo Zanoni]

2013/7/24 Chris Wilson <chris@chris-wilson.co.uk>:
> On Tue, Jul 23, 2013 at 07:33:43PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> The problem here is that we have the PIPESTAT registers between IER
>> and IIR, so when we use intel_irq_reg_reset we flip the order used to
>> reset IIR and PIPESTAT. That should be safe since after we clear
>> IMR/IER we won't get any other IIR/PIPESTAT interrupts. Still, the
>> change is on its own patch, so it should be easy to bisect and revert
>> if needed.
>
> This is wrong. PIPESTAT needs to be cleared before IIR.

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

Thanks for the review!

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



-- 
Paulo Zanoni

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

[Ville Syrjälä]

On Wed, Jul 24, 2013 at 11:14:57AM -0300, Paulo Zanoni wrote:
> 2013/7/24 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Tue, Jul 23, 2013 at 07:33:43PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> The problem here is that we have the PIPESTAT registers between IER
> >> and IIR, so when we use intel_irq_reg_reset we flip the order used to
> >> reset IIR and PIPESTAT. That should be safe since after we clear
> >> IMR/IER we won't get any other IIR/PIPESTAT interrupts. Still, the
> >> change is on its own patch, so it should be easy to bisect and revert
> >> if needed.
> >
> > This is wrong. PIPESTAT needs to be cleared before IIR.
> 
> Yeah, you're right. I see that if we want to make all the code
> touching pipestat consistent we could probably dedicate a full patch
> series to it. The fact that PIPESTAT needs to be disabled after
> IER/IMR but before IIR also kinda breaks my macros, I'll have to
> rethink them now.

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

-- 
Ville Syrjälä
Intel OTC