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

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

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 5522cbf..4439e2d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2165,30 +2165,30 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 
 	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 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.

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

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4439e2d..9d381cc 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);
 		gt_iir = I915_READ(GTIIR);
 		pm_iir = I915_READ(GEN6_PMIIR);
+		iir = I915_READ(VLV_IIR);
 
 		if (gt_iir == 0 && pm_iir == 0 && iir == 0)
 			goto out;
 
-		ret = IRQ_HANDLED;
+		if (gt_iir)
+			I915_WRITE(GTIIR, gt_iir);
 
-		snb_gt_irq_handler(dev, dev_priv, gt_iir);
+		if (pm_iir)
+			I915_WRITE(GEN6_PMIIR, pm_iir);
 
-		valleyview_pipestat_irq_handler(dev, iir);
+		if (iir) {
+			/* Consume port. Then clear 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 */
-		if (iir & I915_DISPLAY_PORT_INTERRUPT)
-			i9xx_hpd_irq_handler(dev);
+		ret = IRQ_HANDLED;
+
+		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);
+		if (iir)
+			valleyview_pipestat_irq_handler(dev, iir);
 	}
 
 out:
-- 
1.9.0

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9d381cc..59ff177 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");
 	}
@@ -2232,32 +2232,30 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 
 	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) {
@@ -2267,33 +2265,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");
 	}
@@ -2305,13 +2302,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

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 59ff177..98009b5 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1895,21 +1895,22 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
 		if (master_ctl == 0 && iir == 0)
 			break;
 
-		I915_WRITE(GEN8_MASTER_IRQ, 0);
-
-		gen8_gt_irq_handler(dev, dev_priv, master_ctl);
+		ret = IRQ_HANDLED;
 
-		valleyview_pipestat_irq_handler(dev, iir);
+		I915_WRITE(GEN8_MASTER_IRQ, 0);
 
-		/* Consume port.  Then clear IIR or we'll miss events */
-		i9xx_hpd_irq_handler(dev);
+		if (iir) {
+			/* Consume port. Then clear IIR or we'll miss events */
+			if (iir & I915_DISPLAY_PORT_INTERRUPT)
+				i9xx_hpd_irq_handler(dev);
+			I915_WRITE(VLV_IIR, iir);
+			valleyview_pipestat_irq_handler(dev, iir);
+		}
 
-		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 1/4] drm/i915: Ack interrupts before handling them (GEN5 - GEN7)

[Chris Wilson]

On Mon, Jun 16, 2014 at 12:30:26PM +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 a atomic xchg operation with mmio space, we merely reduce the
window. This is even more apparent when you consider how heavyweight
those I915_READ/I915_WRITE are.

> According to BSPec, the right order should be:
> 
> 1 - Disable Master Interrupt Control.
> 2 - 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.
> 
> We maintain the "disable SDE interrupts when handling" hack since apparently it works.
> 
> Spotted by Bob Beckett <robert.beckett@intel.com>.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

I'd like the changelog slightly clarified, and the notes here would be
useful in the code as well to describe the theory of operation in
handling IRQ.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Ville Syrjälä]

On Mon, Jun 16, 2014 at 12:30:27PM +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.
> 
> Notice that, before clearing a port-sourced interrupt in the IIR, the
> corresponding interrupt source status in the PORT_HOTPLUG_STAT must be
> cleared.

I believe PIPESTAT (and actually all multi-level interrupts) should be
handled the same way.

The way I would write interrupt handlers is something like this:

{
 iir = read(IIR);
 if (iir & X) {
 	iir_x = read(IIR_X);
 	write(IIR_X, iir_x);
 }
 if (iir & Y) {
 	iir_y = read(IIR_Y);
 	write(IIR_Y, iir_y);
 }
 ...
 write(IIR, iir);
 
 process_x(iir_x);
 process_y(iir_y);
 ...
}

And then we hope the hardware is sane enough to keep the IIR bit high as
long as the relevant sub-IIR bits are high, and also that the CPU interrupt
would be re-raised if any IIR bits are high when exiting the interrupt
handler. But I have the impression our hardware isn't quite that sane.

> 
> Spotted by Bob Beckett <robert.beckett@intel.com>.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 61 +++++++++++++++++++++++------------------
>  1 file changed, 35 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 4439e2d..9d381cc 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);
>  		gt_iir = I915_READ(GTIIR);
>  		pm_iir = I915_READ(GEN6_PMIIR);
> +		iir = I915_READ(VLV_IIR);
>  
>  		if (gt_iir == 0 && pm_iir == 0 && iir == 0)
>  			goto out;
>  
> -		ret = IRQ_HANDLED;
> +		if (gt_iir)
> +			I915_WRITE(GTIIR, gt_iir);
>  
> -		snb_gt_irq_handler(dev, dev_priv, gt_iir);
> +		if (pm_iir)
> +			I915_WRITE(GEN6_PMIIR, pm_iir);
>  
> -		valleyview_pipestat_irq_handler(dev, iir);
> +		if (iir) {
> +			/* Consume port. Then clear 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 */
> -		if (iir & I915_DISPLAY_PORT_INTERRUPT)
> -			i9xx_hpd_irq_handler(dev);
> +		ret = IRQ_HANDLED;
> +
> +		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);
> +		if (iir)
> +			valleyview_pipestat_irq_handler(dev, iir);
>  	}
>  
>  out:
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

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

[Imre Deak]

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

On Mon, 2014-06-16 at 12:30 +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.
> 
> 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>.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 61 +++++++++++++++++++++++------------------
>  1 file changed, 35 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 4439e2d..9d381cc 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);
>  		gt_iir = I915_READ(GTIIR);
>  		pm_iir = I915_READ(GEN6_PMIIR);
> +		iir = I915_READ(VLV_IIR);
>  
>  		if (gt_iir == 0 && pm_iir == 0 && iir == 0)
>  			goto out;
>  
> -		ret = IRQ_HANDLED;
> +		if (gt_iir)
> +			I915_WRITE(GTIIR, gt_iir);
>  
> -		snb_gt_irq_handler(dev, dev_priv, gt_iir);
> +		if (pm_iir)
> +			I915_WRITE(GEN6_PMIIR, pm_iir);
>  
> -		valleyview_pipestat_irq_handler(dev, iir);
> +		if (iir) {
> +			/* Consume port. Then clear 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 */
> -		if (iir & I915_DISPLAY_PORT_INTERRUPT)
> -			i9xx_hpd_irq_handler(dev);
> +		ret = IRQ_HANDLED;
> +
> +		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);
> +		if (iir)
> +			valleyview_pipestat_irq_handler(dev, iir);

Afaik the pipe underrun flag handled in
valleyview_pipestat_irq_handler() is not signaled in IIR, although bspec
is rather unclear about this.

--Imre


[-- 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

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

[Daniel Vetter]

On Mon, Jun 16, 2014 at 04:19:30PM +0300, Imre Deak wrote:
> On Mon, 2014-06-16 at 12:30 +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.
> > 
> > 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>.
> > 
> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 61 +++++++++++++++++++++++------------------
> >  1 file changed, 35 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 4439e2d..9d381cc 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);
> >  		gt_iir = I915_READ(GTIIR);
> >  		pm_iir = I915_READ(GEN6_PMIIR);
> > +		iir = I915_READ(VLV_IIR);
> >  
> >  		if (gt_iir == 0 && pm_iir == 0 && iir == 0)
> >  			goto out;
> >  
> > -		ret = IRQ_HANDLED;
> > +		if (gt_iir)
> > +			I915_WRITE(GTIIR, gt_iir);
> >  
> > -		snb_gt_irq_handler(dev, dev_priv, gt_iir);
> > +		if (pm_iir)
> > +			I915_WRITE(GEN6_PMIIR, pm_iir);
> >  
> > -		valleyview_pipestat_irq_handler(dev, iir);
> > +		if (iir) {
> > +			/* Consume port. Then clear 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 */
> > -		if (iir & I915_DISPLAY_PORT_INTERRUPT)
> > -			i9xx_hpd_irq_handler(dev);
> > +		ret = IRQ_HANDLED;
> > +
> > +		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);
> > +		if (iir)
> > +			valleyview_pipestat_irq_handler(dev, iir);
> 
> Afaik the pipe underrun flag handled in
> valleyview_pipestat_irq_handler() is not signaled in IIR, although bspec
> is rather unclear about this.

Pipe underrun isn't signalling the top-level interrupt and we also can't
mask it in any way. Hence the funny logic.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Daniel Vetter]

On Mon, Jun 16, 2014 at 01:07:37PM +0100, Chris Wilson wrote:
> On Mon, Jun 16, 2014 at 12:30:26PM +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 a atomic xchg operation with mmio space, we merely reduce the
> window. This is even more apparent when you consider how heavyweight
> those I915_READ/I915_WRITE are.

I think the correct description of the race is:

If we handle the irq source before resetting the irq signal bit then
there's a race between the handling and the resetting where a new
interrupt can get lost. This is important where interrupts can happen
quickly and have some status (like seqno or other completion number)
attached.

If we on the other hand handle the irq (i.e. read the seqno/cookie) after
clearing the interrupt we might get a spurious interrupt occasionally
(i.e. one for a cookie value we've seen already), but we'll never miss the
interrupt for a cookie update.

This fix has the potential to explain tons of the missed interrupt fun
we've seen in the past years all over the place (render, rps, ...).
-Daniel

> 
> > According to BSPec, the right order should be:
> > 
> > 1 - Disable Master Interrupt Control.
> > 2 - 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.
> > 
> > We maintain the "disable SDE interrupts when handling" hack since apparently it works.
> > 
> > Spotted by Bob Beckett <robert.beckett@intel.com>.
> > 
> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> 
> I'd like the changelog slightly clarified, and the notes here would be
> useful in the code as well to describe the theory of operation in
> handling IRQ.
> -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

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

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

[Mateo Lozano, Oscar]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Monday, June 16, 2014 7:07 PM
> To: Deak, Imre
> Cc: Mateo Lozano, Oscar; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 2/4] drm/i915/vlv: Ack interrupts before
> handling them (VLV)
> 
> On Mon, Jun 16, 2014 at 04:19:30PM +0300, Imre Deak wrote:
> > On Mon, 2014-06-16 at 12:30 +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.
> > >
> > > 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>.
> > >
> > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_irq.c | 61
> > > +++++++++++++++++++++++------------------
> > >  1 file changed, 35 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > b/drivers/gpu/drm/i915/i915_irq.c index 4439e2d..9d381cc 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);
> > >  		gt_iir = I915_READ(GTIIR);
> > >  		pm_iir = I915_READ(GEN6_PMIIR);
> > > +		iir = I915_READ(VLV_IIR);
> > >
> > >  		if (gt_iir == 0 && pm_iir == 0 && iir == 0)
> > >  			goto out;
> > >
> > > -		ret = IRQ_HANDLED;
> > > +		if (gt_iir)
> > > +			I915_WRITE(GTIIR, gt_iir);
> > >
> > > -		snb_gt_irq_handler(dev, dev_priv, gt_iir);
> > > +		if (pm_iir)
> > > +			I915_WRITE(GEN6_PMIIR, pm_iir);
> > >
> > > -		valleyview_pipestat_irq_handler(dev, iir);
> > > +		if (iir) {
> > > +			/* Consume port. Then clear 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 */
> > > -		if (iir & I915_DISPLAY_PORT_INTERRUPT)
> > > -			i9xx_hpd_irq_handler(dev);
> > > +		ret = IRQ_HANDLED;
> > > +
> > > +		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);
> > > +		if (iir)
> > > +			valleyview_pipestat_irq_handler(dev, iir);
> >
> > Afaik the pipe underrun flag handled in
> > valleyview_pipestat_irq_handler() is not signaled in IIR, although
> > bspec is rather unclear about this.
> 
> Pipe underrun isn't signalling the top-level interrupt and we also can't mask it
> in any way. Hence the funny logic.
> -Daniel

In v3 of the patch I call valleyview_pipestat_irq_handler() regardless, with a comment explaining why. AFAICT, we don´t need to do the same thing for other handlers, but a thorough review of all the patches would be greatly appreciated (this is my first encounter with the interrupt code, other than the pretty clean GEN8 gt sources handler).