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

[PATCH v3 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.
v3: Improve the source comments.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 5522cbf..a68f68c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2136,6 +2136,14 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
 	}
 }
 
+/*
+ * To handle irqs with the minimum 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.
+ */
 static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 {
 	struct drm_device *dev = arg;
@@ -2163,32 +2171,34 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 		POSTING_READ(SDEIER);
 	}
 
+	/* Find, clear, then process each source of interrupt */
+
 	gt_iir = I915_READ(GTIIR);
 	if (gt_iir) {
+		I915_WRITE(GTIIR, gt_iir);
+		ret = IRQ_HANDLED;
 		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);
 		}
 	}
 
-- 
1.9.0

[PATCH v3 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.

v3: Improve the source code comment.

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 a68f68c..20f7c05 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, clear, then process each source of interrupt */
+
 		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 v3 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.

v3: Improve the source code comment.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 20f7c05..46fe3ad 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");
 	}
@@ -2238,36 +2238,36 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 	I915_WRITE(GEN8_MASTER_IRQ, 0);
 	POSTING_READ(GEN8_MASTER_IRQ);
 
+	/* Find, clear, then process each source of interrupt */
+
 	ret = gen8_gt_irq_handler(dev, dev_priv, master_ctl);
 
 	if (master_ctl & GEN8_DE_MISC_IRQ) {
 		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;
+			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");
 	}
 
 	for_each_pipe(pipe) {
@@ -2277,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");
 	}
@@ -2315,13 +2314,13 @@ 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");
+
 	}
 
 	I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
-- 
1.9.0

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

[Daniel Vetter]

On Mon, Jun 16, 2014 at 04:10:59PM +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.
> 
> 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.
> 
> v3: Improve the source code comment.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

This one had a small conflict with Chris' stall detection patch that I
fixed up while applying. Entire series merged, thanks.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 91 ++++++++++++++++++++---------------------
>  1 file changed, 45 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 20f7c05..46fe3ad 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");
>  	}
> @@ -2238,36 +2238,36 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
>  	I915_WRITE(GEN8_MASTER_IRQ, 0);
>  	POSTING_READ(GEN8_MASTER_IRQ);
>  
> +	/* Find, clear, then process each source of interrupt */
> +
>  	ret = gen8_gt_irq_handler(dev, dev_priv, master_ctl);
>  
>  	if (master_ctl & GEN8_DE_MISC_IRQ) {
>  		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;
> +			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");
>  	}
>  
>  	for_each_pipe(pipe) {
> @@ -2277,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");
>  	}
> @@ -2315,13 +2314,13 @@ 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");
> +
>  	}
>  
>  	I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> -- 
> 1.9.0
> 
> _______________________________________________
> 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

[PATCH v3 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.

v3: Improve the source code comment.

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 46fe3ad..aa155b5 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1895,21 +1895,27 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
 		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);
+		/* Find, clear, then process each source of interrupt */
 
-		valleyview_pipestat_irq_handler(dev, 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);
+		}
 
-		/* Consume port.  Then clear IIR or we'll miss events */
-		i9xx_hpd_irq_handler(dev);
+		gen8_gt_irq_handler(dev, dev_priv, master_ctl);
 
-		I915_WRITE(VLV_IIR, iir);
+		/* Call regardless, as some status bits might not be
+		 * signalled in iir */
+		valleyview_pipestat_irq_handler(dev, iir);
 
 		I915_WRITE(GEN8_MASTER_IRQ, DE_MASTER_IRQ_CONTROL);
 		POSTING_READ(GEN8_MASTER_IRQ);
-
-		ret = IRQ_HANDLED;
 	}
 
 	return ret;
-- 
1.9.0

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

[Chris Wilson]

On Mon, Jun 16, 2014 at 04:11:00PM +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.
> 
> 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.
> 
> v3: Improve the source code comment.

The code still talks about the necessity of clearing the PIPESTAT
interrupt generators before resetting IIR, and since it is clearly doing
so for the hpd interrupt, it probably should do so for the others. This
patch introduces a regression in how we handle PIPESTAT on CHV (+VLV
earlier).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 3486 bytes --]

On Mon, 2014-06-16 at 16:10 +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).

I can see how we can miss a second, third etc. back-to-back interrupt,
but that's always a problem with edge triggered interrupts. But the
rearranging done in this patchset closes the race where we are left with
a pending interrupt flag without ever calling its handler.

> 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.
> v3: Improve the source comments.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

I couldn't spot any problems, so on all 4 patches:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 5522cbf..a68f68c 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2136,6 +2136,14 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
>  	}
>  }
>  
> +/*
> + * To handle irqs with the minimum 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.
> + */
>  static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  {
>  	struct drm_device *dev = arg;
> @@ -2163,32 +2171,34 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  		POSTING_READ(SDEIER);
>  	}
>  
> +	/* Find, clear, then process each source of interrupt */
> +
>  	gt_iir = I915_READ(GTIIR);
>  	if (gt_iir) {
> +		I915_WRITE(GTIIR, gt_iir);
> +		ret = IRQ_HANDLED;
>  		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);
>  		}
>  	}
>  


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx