[PATCH 1/2] drm/i915: Support pageflipping interrupts for all 3-pipes on IVB

[PATCH 1/2] drm/i915: Support pageflipping interrupts for all 3-pipes on IVB

[Chris Wilson]

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_irq.c |   31 ++++++++++++++++++++++---------
 drivers/gpu/drm/i915/i915_reg.h |    7 +++++--
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 39c4e68..7d1fc62 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -623,12 +623,20 @@ static irqreturn_t ivybridge_irq_handler(DRM_IRQ_ARGS)
 		intel_finish_page_flip_plane(dev, 1);
 	}
 
+	if (de_iir & DE_PLANEC_FLIP_DONE_IVB) {
+		intel_prepare_page_flip(dev, 2);
+		intel_finish_page_flip_plane(dev, 2);
+	}
+
 	if (de_iir & DE_PIPEA_VBLANK_IVB)
 		drm_handle_vblank(dev, 0);
 
 	if (de_iir & DE_PIPEB_VBLANK_IVB)
 		drm_handle_vblank(dev, 1);
 
+	if (de_iir & DE_PIPEC_VBLANK_IVB)
+		drm_handle_vblank(dev, 2);
+
 	/* check event from PCH */
 	if (de_iir & DE_PCH_EVENT_IVB) {
 		if (pch_iir & SDE_HOTPLUG_MASK_CPT)
@@ -1542,8 +1550,8 @@ static int ivybridge_enable_vblank(struct drm_device *dev, int pipe)
 		return -EINVAL;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	ironlake_enable_display_irq(dev_priv, (pipe == 0) ?
-				    DE_PIPEA_VBLANK_IVB : DE_PIPEB_VBLANK_IVB);
+	ironlake_enable_display_irq(dev_priv,
+				    DE_PIPEA_VBLANK_IVB << (5 * pipe));
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 	return 0;
@@ -1610,8 +1618,8 @@ static void ivybridge_disable_vblank(struct drm_device *dev, int pipe)
 	unsigned long irqflags;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	ironlake_disable_display_irq(dev_priv, (pipe == 0) ?
-				     DE_PIPEA_VBLANK_IVB : DE_PIPEB_VBLANK_IVB);
+	ironlake_disable_display_irq(dev_priv,
+				     DE_PIPEA_VBLANK_IVB << (pipe * 5));
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
@@ -1987,9 +1995,11 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	/* enable kind of interrupts always enabled */
-	u32 display_mask = DE_MASTER_IRQ_CONTROL | DE_GSE_IVB |
-		DE_PCH_EVENT_IVB | DE_PLANEA_FLIP_DONE_IVB |
-		DE_PLANEB_FLIP_DONE_IVB;
+	u32 display_mask =
+		DE_MASTER_IRQ_CONTROL | DE_GSE_IVB | DE_PCH_EVENT_IVB |
+		DE_PLANEC_FLIP_DONE_IVB |
+		DE_PLANEB_FLIP_DONE_IVB |
+		DE_PLANEA_FLIP_DONE_IVB;
 	u32 render_irqs;
 	u32 hotplug_mask;
 
@@ -1998,8 +2008,11 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 	/* should always can generate irq */
 	I915_WRITE(DEIIR, I915_READ(DEIIR));
 	I915_WRITE(DEIMR, dev_priv->irq_mask);
-	I915_WRITE(DEIER, display_mask | DE_PIPEA_VBLANK_IVB |
-		   DE_PIPEB_VBLANK_IVB);
+	I915_WRITE(DEIER,
+		   display_mask |
+		   DE_PIPEC_VBLANK_IVB |
+		   DE_PIPEB_VBLANK_IVB |
+		   DE_PIPEA_VBLANK_IVB);
 	POSTING_READ(DEIER);
 
 	dev_priv->gt_irq_mask = ~0;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 7bc407a..159d732 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3220,11 +3220,14 @@
 #define DE_PCH_EVENT_IVB		(1<<28)
 #define DE_DP_A_HOTPLUG_IVB		(1<<27)
 #define DE_AUX_CHANNEL_A_IVB		(1<<26)
+#define DE_SPRITEC_FLIP_DONE_IVB	(1<<14)
+#define DE_PLANEC_FLIP_DONE_IVB		(1<<13)
+#define DE_PIPEC_VBLANK_IVB		(1<<10)
 #define DE_SPRITEB_FLIP_DONE_IVB	(1<<9)
-#define DE_SPRITEA_FLIP_DONE_IVB	(1<<4)
 #define DE_PLANEB_FLIP_DONE_IVB		(1<<8)
-#define DE_PLANEA_FLIP_DONE_IVB		(1<<3)
 #define DE_PIPEB_VBLANK_IVB		(1<<5)
+#define DE_SPRITEA_FLIP_DONE_IVB	(1<<4)
+#define DE_PLANEA_FLIP_DONE_IVB		(1<<3)
 #define DE_PIPEA_VBLANK_IVB		(1<<0)
 
 #define VLV_MASTER_IER			0x4400c /* Gunit master IER */
-- 
1.7.10

[PATCH 2/2] drm/i915: Simplify the IVB interrupt handler

[Chris Wilson]

Reduce the number of reads and writes required for handling a single
interrupt.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_irq.c |   94 ++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 7d1fc62..72c85f0 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -533,14 +533,11 @@ out:
 	return ret;
 }
 
-static void pch_irq_handler(struct drm_device *dev)
+static void pch_irq_handler(struct drm_device *dev, u32 pch_iir)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
-	u32 pch_iir;
 	int pipe;
 
-	pch_iir = I915_READ(SDEIIR);
-
 	if (pch_iir & SDE_AUDIO_POWER_MASK)
 		DRM_DEBUG_DRIVER("PCH audio power change on port %d\n",
 				 (pch_iir & SDE_AUDIO_POWER_MASK) >>
@@ -580,26 +577,15 @@ static irqreturn_t ivybridge_irq_handler(DRM_IRQ_ARGS)
 {
 	struct drm_device *dev = (struct drm_device *) arg;
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
-	int ret = IRQ_NONE;
-	u32 de_iir, gt_iir, de_ier, pch_iir, pm_iir;
+	u32 de_iir, gt_iir, de_ier, pm_iir;
 	struct drm_i915_master_private *master_priv;
+	int i;
 
 	atomic_inc(&dev_priv->irq_received);
 
 	/* disable master interrupt before clearing iir  */
 	de_ier = I915_READ(DEIER);
 	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
-	POSTING_READ(DEIER);
-
-	de_iir = I915_READ(DEIIR);
-	gt_iir = I915_READ(GTIIR);
-	pch_iir = I915_READ(SDEIIR);
-	pm_iir = I915_READ(GEN6_PMIIR);
-
-	if (de_iir == 0 && gt_iir == 0 && pch_iir == 0 && pm_iir == 0)
-		goto done;
-
-	ret = IRQ_HANDLED;
 
 	if (dev->primary->master) {
 		master_priv = dev->primary->master->driver_priv;
@@ -608,56 +594,52 @@ static irqreturn_t ivybridge_irq_handler(DRM_IRQ_ARGS)
 				READ_BREADCRUMB(dev_priv);
 	}
 
-	snb_gt_irq_handler(dev, dev_priv, gt_iir);
-
-	if (de_iir & DE_GSE_IVB)
-		intel_opregion_gse_intr(dev);
-
-	if (de_iir & DE_PLANEA_FLIP_DONE_IVB) {
-		intel_prepare_page_flip(dev, 0);
-		intel_finish_page_flip_plane(dev, 0);
-	}
-
-	if (de_iir & DE_PLANEB_FLIP_DONE_IVB) {
-		intel_prepare_page_flip(dev, 1);
-		intel_finish_page_flip_plane(dev, 1);
+	gt_iir = I915_READ(GTIIR);
+	if (gt_iir) {
+		snb_gt_irq_handler(dev, dev_priv, gt_iir);
+		I915_WRITE(GTIIR, gt_iir);
 	}
 
-	if (de_iir & DE_PLANEC_FLIP_DONE_IVB) {
-		intel_prepare_page_flip(dev, 2);
-		intel_finish_page_flip_plane(dev, 2);
-	}
+	de_iir = I915_READ(DEIIR);
+	if (de_iir) {
+		if (de_iir & DE_GSE_IVB)
+			intel_opregion_gse_intr(dev);
+
+		for (i = 0; i < 3; i++) {
+			if (de_iir & (DE_PLANEA_FLIP_DONE_IVB << (5 * i))) {
+				intel_prepare_page_flip(dev, i);
+				intel_finish_page_flip_plane(dev, i);
+			}
+			if (de_iir & (DE_PIPEA_VBLANK_IVB << (5 * i)))
+				drm_handle_vblank(dev, i);
+		}
 
-	if (de_iir & DE_PIPEA_VBLANK_IVB)
-		drm_handle_vblank(dev, 0);
+		/* check event from PCH */
+		if (de_iir & DE_PCH_EVENT_IVB) {
+			u32 pch_iir = I915_READ(SDEIIR);
 
-	if (de_iir & DE_PIPEB_VBLANK_IVB)
-		drm_handle_vblank(dev, 1);
+			if (pch_iir & SDE_HOTPLUG_MASK_CPT)
+				queue_work(dev_priv->wq, &dev_priv->hotplug_work);
+			pch_irq_handler(dev, pch_iir);
 
-	if (de_iir & DE_PIPEC_VBLANK_IVB)
-		drm_handle_vblank(dev, 2);
+			/* clear PCH hotplug event before clear CPU irq */
+			I915_WRITE(SDEIIR, pch_iir);
+		}
 
-	/* check event from PCH */
-	if (de_iir & DE_PCH_EVENT_IVB) {
-		if (pch_iir & SDE_HOTPLUG_MASK_CPT)
-			queue_work(dev_priv->wq, &dev_priv->hotplug_work);
-		pch_irq_handler(dev);
+		I915_WRITE(DEIIR, de_iir);
 	}
 
-	if (pm_iir & GEN6_PM_DEFERRED_EVENTS)
-		gen6_queue_rps_work(dev_priv, pm_iir);
-
-	/* should clear PCH hotplug event before clear CPU irq */
-	I915_WRITE(SDEIIR, pch_iir);
-	I915_WRITE(GTIIR, gt_iir);
-	I915_WRITE(DEIIR, de_iir);
-	I915_WRITE(GEN6_PMIIR, pm_iir);
+	pm_iir = I915_READ(GEN6_PMIIR);
+	if (pm_iir) {
+		if (pm_iir & GEN6_PM_DEFERRED_EVENTS)
+			gen6_queue_rps_work(dev_priv, pm_iir);
+		I915_WRITE(GEN6_PMIIR, pm_iir);
+	}
 
-done:
 	I915_WRITE(DEIER, de_ier);
 	POSTING_READ(DEIER);
 
-	return ret;
+	return IRQ_HANDLED;
 }
 
 static void ilk_gt_irq_handler(struct drm_device *dev,
@@ -737,7 +719,7 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
 	if (de_iir & DE_PCH_EVENT) {
 		if (pch_iir & hotplug_mask)
 			queue_work(dev_priv->wq, &dev_priv->hotplug_work);
-		pch_irq_handler(dev);
+		pch_irq_handler(dev, pch_iir);
 	}
 
 	if (de_iir & DE_PCU_EVENT) {
-- 
1.7.10

Re: [PATCH 1/2] drm/i915: Support pageflipping interrupts for all 3-pipes on IVB

[Jesse Barnes]

On Wed,  2 May 2012 09:52:12 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_irq.c |   31 ++++++++++++++++++++++---------
>  drivers/gpu/drm/i915/i915_reg.h |    7 +++++--
>  2 files changed, 27 insertions(+), 11 deletions(-)
> 

Highly embarrassing.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Gordon, this is a QA failure too.  Where are the vblank and flip tests
for 3 pipe?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/2] drm/i915: Support pageflipping interrupts for all 3-pipes on IVB

[Daniel Vetter]

On Wed, May 02, 2012 at 12:46:42PM -0700, Jesse Barnes wrote:
> On Wed,  2 May 2012 09:52:12 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c |   31 ++++++++++++++++++++++---------
> >  drivers/gpu/drm/i915/i915_reg.h |    7 +++++--
> >  2 files changed, 27 insertions(+), 11 deletions(-)
> > 
> 
> Highly embarrassing.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Even more embarrassing that I've failed to pick this up until I've noticed
that lack of a merge conflict that should be there when mergin in -fixes.
We really need some automated tests to check whether pageflipping actually
works on all crtcs ...
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/2] drm/i915: Simplify the IVB interrupt handler

[Daniel Vetter]

On Wed, May 02, 2012 at 09:52:13AM +0100, Chris Wilson wrote:
> Reduce the number of reads and writes required for handling a single
> interrupt.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Ok, I've looked at this again and imo it does too much. On a quick look we
have adding the pch_iir argument to pch_irq_handler and restructuring the
register read/write sequence in the actual handler. The latter seems to
require that we unconditionally return IRQ_HANDLED. And the diff looks a
bit horrible ...
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c |   94 ++++++++++++++++-----------------------
>  1 file changed, 38 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 7d1fc62..72c85f0 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -533,14 +533,11 @@ out:
>  	return ret;
>  }
>  
> -static void pch_irq_handler(struct drm_device *dev)
> +static void pch_irq_handler(struct drm_device *dev, u32 pch_iir)
>  {
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> -	u32 pch_iir;
>  	int pipe;
>  
> -	pch_iir = I915_READ(SDEIIR);
> -
>  	if (pch_iir & SDE_AUDIO_POWER_MASK)
>  		DRM_DEBUG_DRIVER("PCH audio power change on port %d\n",
>  				 (pch_iir & SDE_AUDIO_POWER_MASK) >>
> @@ -580,26 +577,15 @@ static irqreturn_t ivybridge_irq_handler(DRM_IRQ_ARGS)
>  {
>  	struct drm_device *dev = (struct drm_device *) arg;
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> -	int ret = IRQ_NONE;
> -	u32 de_iir, gt_iir, de_ier, pch_iir, pm_iir;
> +	u32 de_iir, gt_iir, de_ier, pm_iir;
>  	struct drm_i915_master_private *master_priv;
> +	int i;
>  
>  	atomic_inc(&dev_priv->irq_received);
>  
>  	/* disable master interrupt before clearing iir  */
>  	de_ier = I915_READ(DEIER);
>  	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
> -	POSTING_READ(DEIER);
> -
> -	de_iir = I915_READ(DEIIR);
> -	gt_iir = I915_READ(GTIIR);
> -	pch_iir = I915_READ(SDEIIR);
> -	pm_iir = I915_READ(GEN6_PMIIR);
> -
> -	if (de_iir == 0 && gt_iir == 0 && pch_iir == 0 && pm_iir == 0)
> -		goto done;
> -
> -	ret = IRQ_HANDLED;
>  
>  	if (dev->primary->master) {
>  		master_priv = dev->primary->master->driver_priv;
> @@ -608,56 +594,52 @@ static irqreturn_t ivybridge_irq_handler(DRM_IRQ_ARGS)
>  				READ_BREADCRUMB(dev_priv);
>  	}
>  
> -	snb_gt_irq_handler(dev, dev_priv, gt_iir);
> -
> -	if (de_iir & DE_GSE_IVB)
> -		intel_opregion_gse_intr(dev);
> -
> -	if (de_iir & DE_PLANEA_FLIP_DONE_IVB) {
> -		intel_prepare_page_flip(dev, 0);
> -		intel_finish_page_flip_plane(dev, 0);
> -	}
> -
> -	if (de_iir & DE_PLANEB_FLIP_DONE_IVB) {
> -		intel_prepare_page_flip(dev, 1);
> -		intel_finish_page_flip_plane(dev, 1);
> +	gt_iir = I915_READ(GTIIR);
> +	if (gt_iir) {
> +		snb_gt_irq_handler(dev, dev_priv, gt_iir);
> +		I915_WRITE(GTIIR, gt_iir);
>  	}
>  
> -	if (de_iir & DE_PLANEC_FLIP_DONE_IVB) {
> -		intel_prepare_page_flip(dev, 2);
> -		intel_finish_page_flip_plane(dev, 2);
> -	}
> +	de_iir = I915_READ(DEIIR);
> +	if (de_iir) {
> +		if (de_iir & DE_GSE_IVB)
> +			intel_opregion_gse_intr(dev);
> +
> +		for (i = 0; i < 3; i++) {
> +			if (de_iir & (DE_PLANEA_FLIP_DONE_IVB << (5 * i))) {
> +				intel_prepare_page_flip(dev, i);
> +				intel_finish_page_flip_plane(dev, i);
> +			}
> +			if (de_iir & (DE_PIPEA_VBLANK_IVB << (5 * i)))
> +				drm_handle_vblank(dev, i);
> +		}
>  
> -	if (de_iir & DE_PIPEA_VBLANK_IVB)
> -		drm_handle_vblank(dev, 0);
> +		/* check event from PCH */
> +		if (de_iir & DE_PCH_EVENT_IVB) {
> +			u32 pch_iir = I915_READ(SDEIIR);
>  
> -	if (de_iir & DE_PIPEB_VBLANK_IVB)
> -		drm_handle_vblank(dev, 1);
> +			if (pch_iir & SDE_HOTPLUG_MASK_CPT)
> +				queue_work(dev_priv->wq, &dev_priv->hotplug_work);
> +			pch_irq_handler(dev, pch_iir);
>  
> -	if (de_iir & DE_PIPEC_VBLANK_IVB)
> -		drm_handle_vblank(dev, 2);
> +			/* clear PCH hotplug event before clear CPU irq */
> +			I915_WRITE(SDEIIR, pch_iir);
> +		}
>  
> -	/* check event from PCH */
> -	if (de_iir & DE_PCH_EVENT_IVB) {
> -		if (pch_iir & SDE_HOTPLUG_MASK_CPT)
> -			queue_work(dev_priv->wq, &dev_priv->hotplug_work);
> -		pch_irq_handler(dev);
> +		I915_WRITE(DEIIR, de_iir);
>  	}
>  
> -	if (pm_iir & GEN6_PM_DEFERRED_EVENTS)
> -		gen6_queue_rps_work(dev_priv, pm_iir);
> -
> -	/* should clear PCH hotplug event before clear CPU irq */
> -	I915_WRITE(SDEIIR, pch_iir);
> -	I915_WRITE(GTIIR, gt_iir);
> -	I915_WRITE(DEIIR, de_iir);
> -	I915_WRITE(GEN6_PMIIR, pm_iir);
> +	pm_iir = I915_READ(GEN6_PMIIR);
> +	if (pm_iir) {
> +		if (pm_iir & GEN6_PM_DEFERRED_EVENTS)
> +			gen6_queue_rps_work(dev_priv, pm_iir);
> +		I915_WRITE(GEN6_PMIIR, pm_iir);
> +	}
>  
> -done:
>  	I915_WRITE(DEIER, de_ier);
>  	POSTING_READ(DEIER);
>  
> -	return ret;
> +	return IRQ_HANDLED;
>  }
>  
>  static void ilk_gt_irq_handler(struct drm_device *dev,
> @@ -737,7 +719,7 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
>  	if (de_iir & DE_PCH_EVENT) {
>  		if (pch_iir & hotplug_mask)
>  			queue_work(dev_priv->wq, &dev_priv->hotplug_work);
> -		pch_irq_handler(dev);
> +		pch_irq_handler(dev, pch_iir);
>  	}
>  
>  	if (de_iir & DE_PCU_EVENT) {
> -- 
> 1.7.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/2] drm/i915: Simplify the IVB interrupt handler

[Chris Wilson]

On Sun, 6 May 2012 17:37:50 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, May 02, 2012 at 09:52:13AM +0100, Chris Wilson wrote:
> > Reduce the number of reads and writes required for handling a single
> > interrupt.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Ok, I've looked at this again and imo it does too much. On a quick look we
> have adding the pch_iir argument to pch_irq_handler and restructuring the
> register read/write sequence in the actual handler. The latter seems to
> require that we unconditionally return IRQ_HANDLED.

This being an MS| directed, and never shared, interrupt, this handler
should not be called unless we have something to do. A IRQ_NONE return
here is a serious bug. Another bug is that we read the PCH IIR twice and
only write back the original value, opening the possiblity of handling
the same interrupt twice. All of which is sumised by reducing the number
of reads and writes required for handing a single interrupt.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: Support pageflipping interrupts for all 3-pipes on IVB

[Jesse Barnes]

On Sun, 6 May 2012 17:34:53 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, May 02, 2012 at 12:46:42PM -0700, Jesse Barnes wrote:
> > On Wed,  2 May 2012 09:52:12 +0100
> > Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/i915_irq.c |   31
> > > ++++++++++++++++++++++--------- drivers/gpu/drm/i915/i915_reg.h
> > > |    7 +++++-- 2 files changed, 27 insertions(+), 11 deletions(-)
> > > 
> > 
> > Highly embarrassing.
> > 
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Even more embarrassing that I've failed to pick this up until I've
> noticed that lack of a merge conflict that should be there when
> mergin in -fixes. We really need some automated tests to check
> whether pageflipping actually works on all crtcs ...

Yeah definitely.  We have modetest but I'm not sure if that's part of
the QA suite.  I'll add something small and targeted to i-g-t for both
vblank and flipping.  I think Shuang was supposed to do this, but he's
busy with other things...

Jesse

[PATCH 1/2] drm/i915: Avoid a double-read of PCH_IIR during interrupt handling

[Chris Wilson]

Currently the code re-reads PCH_IIR during the hotplug interrupt
processing. Not only is this a wasted read, but introduces a potential
for handling a spurious interrupt as we then may not clear all the
interrupts processed (since the re-read IIR may contains more interrupts
asserted than we clear using the result of the original read).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: stable@kernel.org
---
 drivers/gpu/drm/i915/i915_irq.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b4999b5..14943ba 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -533,14 +533,11 @@ out:
 	return ret;
 }
 
-static void pch_irq_handler(struct drm_device *dev)
+static void pch_irq_handler(struct drm_device *dev, u32 pch_iir)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
-	u32 pch_iir;
 	int pipe;
 
-	pch_iir = I915_READ(SDEIIR);
-
 	if (pch_iir & SDE_AUDIO_POWER_MASK)
 		DRM_DEBUG_DRIVER("PCH audio power change on port %d\n",
 				 (pch_iir & SDE_AUDIO_POWER_MASK) >>
@@ -633,7 +630,7 @@ static irqreturn_t ivybridge_irq_handler(DRM_IRQ_ARGS)
 	if (de_iir & DE_PCH_EVENT_IVB) {
 		if (pch_iir & SDE_HOTPLUG_MASK_CPT)
 			queue_work(dev_priv->wq, &dev_priv->hotplug_work);
-		pch_irq_handler(dev);
+		pch_irq_handler(dev, pch_iir);
 	}
 
 	if (pm_iir & GEN6_PM_DEFERRED_EVENTS)
@@ -721,7 +718,7 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
 	if (de_iir & DE_PCH_EVENT) {
 		if (pch_iir & hotplug_mask)
 			queue_work(dev_priv->wq, &dev_priv->hotplug_work);
-		pch_irq_handler(dev);
+		pch_irq_handler(dev, pch_iir);
 	}
 
 	if (de_iir & DE_PCU_EVENT) {
-- 
1.7.10

[PATCH 2/2] drm/i915: Simplify interrupt processing for IvyBridge

[Chris Wilson]

We can take advantage that the PCH_IIR is a subordinate register to
reduce one of the required IIR reads, and that we only need to clear
interrupts handled to reduce the writes. And by simply tidying the code
we can reduce the line count and hopefully make it more readable.

v2: Split out the bugfix from the refactoring.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_irq.c |   87 +++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 14943ba..324431e 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -577,72 +577,61 @@ static irqreturn_t ivybridge_irq_handler(DRM_IRQ_ARGS)
 {
 	struct drm_device *dev = (struct drm_device *) arg;
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
-	int ret = IRQ_NONE;
-	u32 de_iir, gt_iir, de_ier, pch_iir, pm_iir;
+	u32 de_iir, gt_iir, de_ier, pm_iir;
+	irqreturn_t ret = IRQ_NONE;
+	int i;
 
 	atomic_inc(&dev_priv->irq_received);
 
 	/* disable master interrupt before clearing iir  */
 	de_ier = I915_READ(DEIER);
 	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
-	POSTING_READ(DEIER);
 
-	de_iir = I915_READ(DEIIR);
 	gt_iir = I915_READ(GTIIR);
-	pch_iir = I915_READ(SDEIIR);
-	pm_iir = I915_READ(GEN6_PMIIR);
-
-	if (de_iir == 0 && gt_iir == 0 && pch_iir == 0 && pm_iir == 0)
-		goto done;
-
-	ret = IRQ_HANDLED;
-
-	snb_gt_irq_handler(dev, dev_priv, gt_iir);
-
-	if (de_iir & DE_GSE_IVB)
-		intel_opregion_gse_intr(dev);
-
-	if (de_iir & DE_PLANEA_FLIP_DONE_IVB) {
-		intel_prepare_page_flip(dev, 0);
-		intel_finish_page_flip_plane(dev, 0);
-	}
-
-	if (de_iir & DE_PLANEB_FLIP_DONE_IVB) {
-		intel_prepare_page_flip(dev, 1);
-		intel_finish_page_flip_plane(dev, 1);
+	if (gt_iir) {
+		snb_gt_irq_handler(dev, dev_priv, gt_iir);
+		I915_WRITE(GTIIR, gt_iir);
+		ret = IRQ_HANDLED;
 	}
 
-	if (de_iir & DE_PLANEC_FLIP_DONE_IVB) {
-		intel_prepare_page_flip(dev, 2);
-		intel_finish_page_flip_plane(dev, 2);
-	}
+	de_iir = I915_READ(DEIIR);
+	if (de_iir) {
+		if (de_iir & DE_GSE_IVB)
+			intel_opregion_gse_intr(dev);
+
+		for (i = 0; i < 3; i++) {
+			if (de_iir & (DE_PLANEA_FLIP_DONE_IVB << (5 * i))) {
+				intel_prepare_page_flip(dev, i);
+				intel_finish_page_flip_plane(dev, i);
+			}
+			if (de_iir & (DE_PIPEA_VBLANK_IVB << (5 * i)))
+				drm_handle_vblank(dev, i);
+		}
 
-	if (de_iir & DE_PIPEA_VBLANK_IVB)
-		drm_handle_vblank(dev, 0);
+		/* check event from PCH */
+		if (de_iir & DE_PCH_EVENT_IVB) {
+			u32 pch_iir = I915_READ(SDEIIR);
 
-	if (de_iir & DE_PIPEB_VBLANK_IVB)
-		drm_handle_vblank(dev, 1);
+			if (pch_iir & SDE_HOTPLUG_MASK_CPT)
+				queue_work(dev_priv->wq, &dev_priv->hotplug_work);
+			pch_irq_handler(dev, pch_iir);
 
-	if (de_iir & DE_PIPEC_VBLANK_IVB)
-		drm_handle_vblank(dev, 2);
+			/* clear PCH hotplug event before clear CPU irq */
+			I915_WRITE(SDEIIR, pch_iir);
+		}
 
-	/* check event from PCH */
-	if (de_iir & DE_PCH_EVENT_IVB) {
-		if (pch_iir & SDE_HOTPLUG_MASK_CPT)
-			queue_work(dev_priv->wq, &dev_priv->hotplug_work);
-		pch_irq_handler(dev, pch_iir);
+		I915_WRITE(DEIIR, de_iir);
+		ret = IRQ_HANDLED;
 	}
 
-	if (pm_iir & GEN6_PM_DEFERRED_EVENTS)
-		gen6_queue_rps_work(dev_priv, pm_iir);
-
-	/* should clear PCH hotplug event before clear CPU irq */
-	I915_WRITE(SDEIIR, pch_iir);
-	I915_WRITE(GTIIR, gt_iir);
-	I915_WRITE(DEIIR, de_iir);
-	I915_WRITE(GEN6_PMIIR, pm_iir);
+	pm_iir = I915_READ(GEN6_PMIIR);
+	if (pm_iir) {
+		if (pm_iir & GEN6_PM_DEFERRED_EVENTS)
+			gen6_queue_rps_work(dev_priv, pm_iir);
+		I915_WRITE(GEN6_PMIIR, pm_iir);
+		ret = IRQ_HANDLED;
+	}
 
-done:
 	I915_WRITE(DEIER, de_ier);
 	POSTING_READ(DEIER);
 
-- 
1.7.10

Re: [PATCH 2/2] drm/i915: Simplify interrupt processing for IvyBridge

[Daniel Vetter]

On Wed, May 09, 2012 at 09:45:44PM +0100, Chris Wilson wrote:
> We can take advantage that the PCH_IIR is a subordinate register to
> reduce one of the required IIR reads, and that we only need to clear
> interrupts handled to reduce the writes. And by simply tidying the code
> we can reduce the line count and hopefully make it more readable.
> 
> v2: Split out the bugfix from the refactoring.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Both patches queued for -next, thanks.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48