[PATCH v2 1/4] drm/i915: Ack interrupts before handling them (GEN5 - GEN7)

[PATCH v2 1/4] drm/i915: Ack interrupts before handling them (GEN5 - GEN7)

[oscar.mateo]

From: Oscar Mateo <oscar.mateo@intel.com>

Otherwise, we might receive a new interrupt before we have time to ack the first
one, eventually missing it.

According to BSPec, the right order should be:

1 - Disable Master Interrupt Control.
2 - Find the source(s) of the interrupt.
3 - Clear the Interrupt Identity bits (IIR).
4 - Process the interrupt(s) that had bits set in the IIRs.
5 - Re-enable Master Interrupt Control.

Without an atomic XCHG operation with mmio space, the above merely reduces the window
in which we can miss an interrupt (especially when you consider how heavyweight the
I915_READ/I915_WRITE operations are).

We maintain the "disable SDE interrupts when handling" hack since apparently it works.

Spotted by Bob Beckett <robert.beckett@intel.com>.

v2: Add warning to commit message and comments to the code as per Chris Wilson's request.

Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 5522cbf..7e9fba0 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2147,7 +2147,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 	 * do any I915_{READ,WRITE}. */
 	intel_uncore_check_errors(dev);
 
-	/* disable master interrupt before clearing iir  */
+	/* 1 - Disable master interrupt */
 	de_ier = I915_READ(DEIER);
 	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
 	POSTING_READ(DEIER);
@@ -2163,37 +2163,43 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 		POSTING_READ(SDEIER);
 	}
 
+	/* 2 - Find the source(s) of the interrupt. */
 	gt_iir = I915_READ(GTIIR);
 	if (gt_iir) {
+		/* 3 - Clear the Interrupt Identity bits (IIR) before trying to
+		 * handle the interrupt (we have the IIR safely stored) */
+		I915_WRITE(GTIIR, gt_iir);
+		ret = IRQ_HANDLED;
+		/* 4 - Process the interrupt(s) that had bits set in the IIRs. */
 		if (INTEL_INFO(dev)->gen >= 6)
 			snb_gt_irq_handler(dev, dev_priv, gt_iir);
 		else
 			ilk_gt_irq_handler(dev, dev_priv, gt_iir);
-		I915_WRITE(GTIIR, gt_iir);
-		ret = IRQ_HANDLED;
 	}
 
 	de_iir = I915_READ(DEIIR);
 	if (de_iir) {
+		I915_WRITE(DEIIR, de_iir);
+		ret = IRQ_HANDLED;
 		if (INTEL_INFO(dev)->gen >= 7)
 			ivb_display_irq_handler(dev, de_iir);
 		else
 			ilk_display_irq_handler(dev, de_iir);
-		I915_WRITE(DEIIR, de_iir);
-		ret = IRQ_HANDLED;
 	}
 
 	if (INTEL_INFO(dev)->gen >= 6) {
 		u32 pm_iir = I915_READ(GEN6_PMIIR);
 		if (pm_iir) {
-			gen6_rps_irq_handler(dev_priv, pm_iir);
 			I915_WRITE(GEN6_PMIIR, pm_iir);
 			ret = IRQ_HANDLED;
+			gen6_rps_irq_handler(dev_priv, pm_iir);
 		}
 	}
 
+	/* 5 - Re-enable Master Interrupt Control */
 	I915_WRITE(DEIER, de_ier);
 	POSTING_READ(DEIER);
+
 	if (!HAS_PCH_NOP(dev)) {
 		I915_WRITE(SDEIER, sde_ier);
 		POSTING_READ(SDEIER);
-- 
1.9.0

[PATCH v2 2/4] drm/i915/vlv: Ack interrupts before handling them (VLV)

[oscar.mateo]

From: Oscar Mateo <oscar.mateo@intel.com>

Otherwise, we might receive a new interrupt before we have time to
ack the first one, eventually missing it.

Without an atomic XCHG operation with mmio space, this patch merely
reduces the window in which we can miss an interrupt (especially when
you consider how heavyweight the I915_READ/I915_WRITE operations are).

Notice that, before clearing a port-sourced interrupt in the IIR, the
corresponding interrupt source status in the PORT_HOTPLUG_STAT must be
cleared.

Spotted by Bob Beckett <robert.beckett@intel.com>.

v2:
- Reorder the IIR clearing to reduce the window even further.
- Add warning to commit message and comments to the code as per Chris
  Wilson's request.
- Imre Deak pointed out that the pipe underrun flag might not be signaled
  in IIR, so do not make valleyview_pipestat_irq_handler depend on it.

Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 67 +++++++++++++++++++++++------------------
 1 file changed, 38 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 7e9fba0..25044e9 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1813,26 +1813,28 @@ static void i9xx_hpd_irq_handler(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 hotplug_status = I915_READ(PORT_HOTPLUG_STAT);
 
-	if (IS_G4X(dev)) {
-		u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_G4X;
+	if (hotplug_status) {
+		I915_WRITE(PORT_HOTPLUG_STAT, hotplug_status);
+		/*
+		 * Make sure hotplug status is cleared before we clear IIR, or else we
+		 * may miss hotplug events.
+		 */
+		POSTING_READ(PORT_HOTPLUG_STAT);
 
-		intel_hpd_irq_handler(dev, hotplug_trigger, hpd_status_g4x);
-	} else {
-		u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
+		if (IS_G4X(dev)) {
+			u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_G4X;
 
-		intel_hpd_irq_handler(dev, hotplug_trigger, hpd_status_i915);
-	}
+			intel_hpd_irq_handler(dev, hotplug_trigger, hpd_status_g4x);
+		} else {
+			u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
 
-	if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)) &&
-	    hotplug_status & DP_AUX_CHANNEL_MASK_INT_STATUS_G4X)
-		dp_aux_irq_handler(dev);
+			intel_hpd_irq_handler(dev, hotplug_trigger, hpd_status_i915);
+		}
 
-	I915_WRITE(PORT_HOTPLUG_STAT, hotplug_status);
-	/*
-	 * Make sure hotplug status is cleared before we clear IIR, or else we
-	 * may miss hotplug events.
-	 */
-	POSTING_READ(PORT_HOTPLUG_STAT);
+		if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)) &&
+		    hotplug_status & DP_AUX_CHANNEL_MASK_INT_STATUS_G4X)
+			dp_aux_irq_handler(dev);
+	}
 }
 
 static irqreturn_t valleyview_irq_handler(int irq, void *arg)
@@ -1843,29 +1845,36 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 	irqreturn_t ret = IRQ_NONE;
 
 	while (true) {
-		iir = I915_READ(VLV_IIR);
+		/* Find the source(s) of the interrupt and clear the IIR
+		 * before processing (the iir values are safely stored) */
 		gt_iir = I915_READ(GTIIR);
+		if (gt_iir)
+			I915_WRITE(GTIIR, gt_iir);
+
 		pm_iir = I915_READ(GEN6_PMIIR);
+		if (pm_iir)
+			I915_WRITE(GEN6_PMIIR, pm_iir);
+
+		iir = I915_READ(VLV_IIR);
+		if (iir) {
+			/* Consume port before clearing IIR or we'll miss events */
+			if (iir & I915_DISPLAY_PORT_INTERRUPT)
+				i9xx_hpd_irq_handler(dev);
+			I915_WRITE(VLV_IIR, iir);
+		}
 
 		if (gt_iir == 0 && pm_iir == 0 && iir == 0)
 			goto out;
 
 		ret = IRQ_HANDLED;
 
-		snb_gt_irq_handler(dev, dev_priv, gt_iir);
-
-		valleyview_pipestat_irq_handler(dev, iir);
-
-		/* Consume port.  Then clear IIR or we'll miss events */
-		if (iir & I915_DISPLAY_PORT_INTERRUPT)
-			i9xx_hpd_irq_handler(dev);
-
+		if (gt_iir)
+			snb_gt_irq_handler(dev, dev_priv, gt_iir);
 		if (pm_iir)
 			gen6_rps_irq_handler(dev_priv, pm_iir);
-
-		I915_WRITE(GTIIR, gt_iir);
-		I915_WRITE(GEN6_PMIIR, pm_iir);
-		I915_WRITE(VLV_IIR, iir);
+		/* Call regardless, as some status bits might not be
+		 * signalled in iir */
+		valleyview_pipestat_irq_handler(dev, iir);
 	}
 
 out:
-- 
1.9.0

[PATCH v2 3/4] drm/i915/bdw: Ack interrupts before handling them (GEN8)

[oscar.mateo]

From: Oscar Mateo <oscar.mateo@intel.com>

Otherwise, we might receive a new interrupt before we have time to
ack the first one, eventually missing it.

The right order should be:

1 - Disable Master Interrupt Control.
2 - Find the category of interrupt that is pending.
3 - Find the source(s) of the interrupt and clear the Interrupt Identity bits (IIR)
4 - Process the interrupt(s) that had bits set in the IIRs.
5 - Re-enable Master Interrupt Control.

Without an atomic XCHG operation with mmio space, the above merely reduces the window
in which we can miss an interrupt (especially when you consider how heavyweight the
I915_READ/I915_WRITE operations are).

Spotted by Bob Beckett <robert.beckett@intel.com>.

v2: Add warning to commit message and comments to the code as per Chris Wilson's request.

Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 100 +++++++++++++++++++++-------------------
 1 file changed, 52 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 25044e9..69c24b0 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1462,6 +1462,7 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
 	if (master_ctl & (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ)) {
 		tmp = I915_READ(GEN8_GT_IIR(0));
 		if (tmp) {
+			I915_WRITE(GEN8_GT_IIR(0), tmp);
 			ret = IRQ_HANDLED;
 			rcs = tmp >> GEN8_RCS_IRQ_SHIFT;
 			bcs = tmp >> GEN8_BCS_IRQ_SHIFT;
@@ -1469,7 +1470,6 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
 				notify_ring(dev, &dev_priv->ring[RCS]);
 			if (bcs & GT_RENDER_USER_INTERRUPT)
 				notify_ring(dev, &dev_priv->ring[BCS]);
-			I915_WRITE(GEN8_GT_IIR(0), tmp);
 		} else
 			DRM_ERROR("The master control interrupt lied (GT0)!\n");
 	}
@@ -1477,6 +1477,7 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
 	if (master_ctl & (GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ)) {
 		tmp = I915_READ(GEN8_GT_IIR(1));
 		if (tmp) {
+			I915_WRITE(GEN8_GT_IIR(1), tmp);
 			ret = IRQ_HANDLED;
 			vcs = tmp >> GEN8_VCS1_IRQ_SHIFT;
 			if (vcs & GT_RENDER_USER_INTERRUPT)
@@ -1484,7 +1485,6 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
 			vcs = tmp >> GEN8_VCS2_IRQ_SHIFT;
 			if (vcs & GT_RENDER_USER_INTERRUPT)
 				notify_ring(dev, &dev_priv->ring[VCS2]);
-			I915_WRITE(GEN8_GT_IIR(1), tmp);
 		} else
 			DRM_ERROR("The master control interrupt lied (GT1)!\n");
 	}
@@ -1492,10 +1492,10 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
 	if (master_ctl & GEN8_GT_PM_IRQ) {
 		tmp = I915_READ(GEN8_GT_IIR(2));
 		if (tmp & dev_priv->pm_rps_events) {
-			ret = IRQ_HANDLED;
-			gen8_rps_irq_handler(dev_priv, tmp);
 			I915_WRITE(GEN8_GT_IIR(2),
 				   tmp & dev_priv->pm_rps_events);
+			ret = IRQ_HANDLED;
+			gen8_rps_irq_handler(dev_priv, tmp);
 		} else
 			DRM_ERROR("The master control interrupt lied (PM)!\n");
 	}
@@ -1503,11 +1503,11 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
 	if (master_ctl & GEN8_GT_VECS_IRQ) {
 		tmp = I915_READ(GEN8_GT_IIR(3));
 		if (tmp) {
+			I915_WRITE(GEN8_GT_IIR(3), tmp);
 			ret = IRQ_HANDLED;
 			vcs = tmp >> GEN8_VECS_IRQ_SHIFT;
 			if (vcs & GT_RENDER_USER_INTERRUPT)
 				notify_ring(dev, &dev_priv->ring[VECS]);
-			I915_WRITE(GEN8_GT_IIR(3), tmp);
 		} else
 			DRM_ERROR("The master control interrupt lied (GT3)!\n");
 	}
@@ -2231,41 +2231,45 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 	if (!master_ctl)
 		return IRQ_NONE;
 
+	/* 1 - Disable Master Interrupt Control. */
 	I915_WRITE(GEN8_MASTER_IRQ, 0);
 	POSTING_READ(GEN8_MASTER_IRQ);
 
-	ret = gen8_gt_irq_handler(dev, dev_priv, master_ctl);
-
+	/* 2 - Find the category of interrupt that is pending. */
 	if (master_ctl & GEN8_DE_MISC_IRQ) {
+		/* 3 - Find the source(s) of the interrupt and clear the
+		 * Interrupt Identity bits (IIR) */
 		tmp = I915_READ(GEN8_DE_MISC_IIR);
-		if (tmp & GEN8_DE_MISC_GSE)
-			intel_opregion_asle_intr(dev);
-		else if (tmp)
-			DRM_ERROR("Unexpected DE Misc interrupt\n");
-		else
-			DRM_ERROR("The master control interrupt lied (DE MISC)!\n");
-
 		if (tmp) {
 			I915_WRITE(GEN8_DE_MISC_IIR, tmp);
 			ret = IRQ_HANDLED;
+			/* 4 - Process the interrupt(s) that had bits set
+			 * in the IIRs. */
+			if (tmp & GEN8_DE_MISC_GSE)
+				intel_opregion_asle_intr(dev);
+			else
+				DRM_ERROR("Unexpected DE Misc interrupt\n");
 		}
+		else
+			DRM_ERROR("The master control interrupt lied (DE MISC)!\n");
 	}
 
 	if (master_ctl & GEN8_DE_PORT_IRQ) {
 		tmp = I915_READ(GEN8_DE_PORT_IIR);
-		if (tmp & GEN8_AUX_CHANNEL_A)
-			dp_aux_irq_handler(dev);
-		else if (tmp)
-			DRM_ERROR("Unexpected DE Port interrupt\n");
-		else
-			DRM_ERROR("The master control interrupt lied (DE PORT)!\n");
-
 		if (tmp) {
 			I915_WRITE(GEN8_DE_PORT_IIR, tmp);
 			ret = IRQ_HANDLED;
+			if (tmp & GEN8_AUX_CHANNEL_A)
+				dp_aux_irq_handler(dev);
+			else
+				DRM_ERROR("Unexpected DE Port interrupt\n");
 		}
+		else
+			DRM_ERROR("The master control interrupt lied (DE PORT)!\n");
 	}
 
+	ret = gen8_gt_irq_handler(dev, dev_priv, master_ctl);
+
 	for_each_pipe(pipe) {
 		uint32_t pipe_iir;
 
@@ -2273,33 +2277,32 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 			continue;
 
 		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
-		if (pipe_iir & GEN8_PIPE_VBLANK)
-			intel_pipe_handle_vblank(dev, pipe);
-
-		if (pipe_iir & GEN8_PIPE_PRIMARY_FLIP_DONE) {
-			intel_prepare_page_flip(dev, pipe);
-			intel_finish_page_flip_plane(dev, pipe);
-		}
+		if (pipe_iir) {
+			ret = IRQ_HANDLED;
+			I915_WRITE(GEN8_DE_PIPE_IIR(pipe), pipe_iir);
+			if (pipe_iir & GEN8_PIPE_VBLANK)
+				intel_pipe_handle_vblank(dev, pipe);
 
-		if (pipe_iir & GEN8_PIPE_CDCLK_CRC_DONE)
-			hsw_pipe_crc_irq_handler(dev, pipe);
+			if (pipe_iir & GEN8_PIPE_PRIMARY_FLIP_DONE) {
+				intel_prepare_page_flip(dev, pipe);
+				intel_finish_page_flip_plane(dev, pipe);
+			}
 
-		if (pipe_iir & GEN8_PIPE_FIFO_UNDERRUN) {
-			if (intel_set_cpu_fifo_underrun_reporting(dev, pipe,
-								  false))
-				DRM_ERROR("Pipe %c FIFO underrun\n",
-					  pipe_name(pipe));
-		}
+			if (pipe_iir & GEN8_PIPE_CDCLK_CRC_DONE)
+				hsw_pipe_crc_irq_handler(dev, pipe);
 
-		if (pipe_iir & GEN8_DE_PIPE_IRQ_FAULT_ERRORS) {
-			DRM_ERROR("Fault errors on pipe %c\n: 0x%08x",
-				  pipe_name(pipe),
-				  pipe_iir & GEN8_DE_PIPE_IRQ_FAULT_ERRORS);
-		}
+			if (pipe_iir & GEN8_PIPE_FIFO_UNDERRUN) {
+				if (intel_set_cpu_fifo_underrun_reporting(dev, pipe,
+									  false))
+					DRM_ERROR("Pipe %c FIFO underrun\n",
+						  pipe_name(pipe));
+			}
 
-		if (pipe_iir) {
-			ret = IRQ_HANDLED;
-			I915_WRITE(GEN8_DE_PIPE_IIR(pipe), pipe_iir);
+			if (pipe_iir & GEN8_DE_PIPE_IRQ_FAULT_ERRORS) {
+				DRM_ERROR("Fault errors on pipe %c\n: 0x%08x",
+					  pipe_name(pipe),
+					  pipe_iir & GEN8_DE_PIPE_IRQ_FAULT_ERRORS);
+			}
 		} else
 			DRM_ERROR("The master control interrupt lied (DE PIPE)!\n");
 	}
@@ -2311,15 +2314,16 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 		 * on older pch-split platforms. But this needs testing.
 		 */
 		u32 pch_iir = I915_READ(SDEIIR);
-
-		cpt_irq_handler(dev, pch_iir);
-
 		if (pch_iir) {
 			I915_WRITE(SDEIIR, pch_iir);
 			ret = IRQ_HANDLED;
-		}
+			cpt_irq_handler(dev, pch_iir);
+		} else
+			DRM_ERROR("The master control interrupt lied (SDE)!\n");
+
 	}
 
+	/* 5 - Re-enable Master Interrupt Control. */
 	I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
 	POSTING_READ(GEN8_MASTER_IRQ);
 
-- 
1.9.0

[PATCH v2 4/4] drm/i915/chv: Ack interrupts before handling them (CHV)

[oscar.mateo]

From: Oscar Mateo <oscar.mateo@intel.com>

Otherwise, we might receive a new interrupt before we have time to
ack the first one, eventually missing it.

Without an atomic XCHG operation with mmio space, this patch merely
reduces the window in which we can miss an interrupt (especially when
you consider how heavyweight the I915_READ/I915_WRITE operations are).

Notice that, before clearing a port-sourced interrupt in the IIR, the
corresponding interrupt source status in the PORT_HOTPLUG_STAT must be
cleared.

Spotted by Bob Beckett <robert.beckett@intel.com>.

v2:
- Add warning to commit message and comments to the code as per Chris
  Wilson's request.
- Imre Deak pointed out that the pipe underrun flag might not be signaled
  in IIR, so do not make valleyview_pipestat_irq_handler depend on it.

Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 69c24b0..630b7fc 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1889,27 +1889,33 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
 	irqreturn_t ret = IRQ_NONE;
 
 	for (;;) {
+		/* Find the source(s) of the interrupt and clear the IIR
+		 * before processing */
 		master_ctl = I915_READ(GEN8_MASTER_IRQ) & ~GEN8_MASTER_IRQ_CONTROL;
 		iir = I915_READ(VLV_IIR);
 
 		if (master_ctl == 0 && iir == 0)
 			break;
 
+		ret = IRQ_HANDLED;
+
 		I915_WRITE(GEN8_MASTER_IRQ, 0);
 
-		gen8_gt_irq_handler(dev, dev_priv, master_ctl);
+		if (iir) {
+			/* Consume port before clearing IIR or we'll miss events */
+			if (iir & I915_DISPLAY_PORT_INTERRUPT)
+				i9xx_hpd_irq_handler(dev);
+			I915_WRITE(VLV_IIR, iir);
+		}
 
+		/* Call regardless, as some status bits might not be
+		 * signalled in iir */
 		valleyview_pipestat_irq_handler(dev, iir);
 
-		/* Consume port.  Then clear IIR or we'll miss events */
-		i9xx_hpd_irq_handler(dev);
-
-		I915_WRITE(VLV_IIR, iir);
+		gen8_gt_irq_handler(dev, dev_priv, master_ctl);
 
 		I915_WRITE(GEN8_MASTER_IRQ, DE_MASTER_IRQ_CONTROL);
 		POSTING_READ(GEN8_MASTER_IRQ);
-
-		ret = IRQ_HANDLED;
 	}
 
 	return ret;
-- 
1.9.0

Re: [PATCH v2 1/4] drm/i915: Ack interrupts before handling them (GEN5 - GEN7)

[Chris Wilson]

On Mon, Jun 16, 2014 at 03:09:24PM +0100, oscar.mateo@intel.com wrote:
> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> Otherwise, we might receive a new interrupt before we have time to ack the first
> one, eventually missing it.
> 
> According to BSPec, the right order should be:
> 
> 1 - Disable Master Interrupt Control.
> 2 - Find the source(s) of the interrupt.
> 3 - Clear the Interrupt Identity bits (IIR).
> 4 - Process the interrupt(s) that had bits set in the IIRs.
> 5 - Re-enable Master Interrupt Control.
> 
> Without an atomic XCHG operation with mmio space, the above merely reduces the window
> in which we can miss an interrupt (especially when you consider how heavyweight the
> I915_READ/I915_WRITE operations are).
> 
> We maintain the "disable SDE interrupts when handling" hack since apparently it works.
> 
> Spotted by Bob Beckett <robert.beckett@intel.com>.
> 
> v2: Add warning to commit message and comments to the code as per Chris Wilson's request.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 5522cbf..7e9fba0 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2147,7 +2147,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  	 * do any I915_{READ,WRITE}. */
>  	intel_uncore_check_errors(dev);
>  
> -	/* disable master interrupt before clearing iir  */
> +	/* 1 - Disable master interrupt */

Sorry to be a nuisance, but I was thinking of more of theory of
operation block at the start of the function as well.

/* To handle irqs with the minimum of potential races with fresh
 * interrupts, we
 * 1 - Disable Master Interrupt Control.
 * 2 - Find the source(s) of the interrupt.
 * 3 - Clear the Interrupt Identity bits (IIR).
 * 4 - Process the interrupt(s) that had bits set in the IIRs.
 * 5 - Re-enable Master Interrupt Control.
 */

Since we have a number of similar irq handlers this can be in a comment
block in just the first handler. And then we leave condensed reminders
in each function. Actually it may work best with this t.o.p. repeated
each time.

>  	de_ier = I915_READ(DEIER);
>  	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
>  	POSTING_READ(DEIER);
> @@ -2163,37 +2163,43 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  		POSTING_READ(SDEIER);
>  	}
>  
> +	/* 2 - Find the source(s) of the interrupt. */

/* Find, clear, then process each source of interrupt */

Then drop 3, 4 since we have the overview block and the ordering reminder
here.
>  	gt_iir = I915_READ(GTIIR);
>  	if (gt_iir) {
> +		/* 3 - Clear the Interrupt Identity bits (IIR) before trying to
> +		 * handle the interrupt (we have the IIR safely stored) */
> +		I915_WRITE(GTIIR, gt_iir);
> +		ret = IRQ_HANDLED;
> +		/* 4 - Process the interrupt(s) that had bits set in the IIRs. */
>  		if (INTEL_INFO(dev)->gen >= 6)
>  			snb_gt_irq_handler(dev, dev_priv, gt_iir);
>  		else
>  			ilk_gt_irq_handler(dev, dev_priv, gt_iir);
> -		I915_WRITE(GTIIR, gt_iir);
> -		ret = IRQ_HANDLED;
>  	}
>  
>  	de_iir = I915_READ(DEIIR);
>  	if (de_iir) {
> +		I915_WRITE(DEIIR, de_iir);
> +		ret = IRQ_HANDLED;
>  		if (INTEL_INFO(dev)->gen >= 7)
>  			ivb_display_irq_handler(dev, de_iir);
>  		else
>  			ilk_display_irq_handler(dev, de_iir);
> -		I915_WRITE(DEIIR, de_iir);
> -		ret = IRQ_HANDLED;
>  	}
>  
>  	if (INTEL_INFO(dev)->gen >= 6) {
>  		u32 pm_iir = I915_READ(GEN6_PMIIR);
>  		if (pm_iir) {
> -			gen6_rps_irq_handler(dev_priv, pm_iir);
>  			I915_WRITE(GEN6_PMIIR, pm_iir);
>  			ret = IRQ_HANDLED;
> +			gen6_rps_irq_handler(dev_priv, pm_iir);
>  		}
>  	}
>  
> +	/* 5 - Re-enable Master Interrupt Control */
/* Finally re-enable the master interrupt */
>  	I915_WRITE(DEIER, de_ier);
>  	POSTING_READ(DEIER);
> +
>  	if (!HAS_PCH_NOP(dev)) {
>  		I915_WRITE(SDEIER, sde_ier);
>  		POSTING_READ(SDEIER);

-- 
Chris Wilson, Intel Open Source Technology Centre