[PATCH 1/2] drm/i915: ring irq cleanups

[PATCH 1/2] drm/i915: ring irq cleanups

[Ben Widawsky]

- gen6 put/get only need one argument
    rflags and gflags are always the same (see above explanation)
- remove a couple redundantly defined IRQs
- reordered some lines to make things go in descending order

Every ring has its own interrupts, enables, masks, and status bits that
are fed into the main interrupt enable/mask/status registers. At one
point in time it seemed like a good idea to make our functions support
the notion that each interrupt may have a different bit position in the
corresponding register (blitter parser error may be bit n in IMR, but
bit m in blitter IMR). It turned out though that the HW designers did us
a solid on Gen6+ and this unfortunate situation has been avoided. This
allows our interrupt code to be cleaned up a bit.

I jammed this into one commit because there should be no functional
change with this commit, and staging it into multiple commits was
unnecessarily artificial IMO.

CC: Chris Wilson <chris@chris-wilson.co.uk>
CC: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c         |   16 +++++++-------
 drivers/gpu/drm/i915/i915_reg.h         |    6 ++----
 drivers/gpu/drm/i915/intel_ringbuffer.c |   36 +++++++++++--------------------
 3 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 998116e..c550901 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -503,9 +503,9 @@ static irqreturn_t ivybridge_irq_handler(DRM_IRQ_ARGS)
 
 	if (gt_iir & (GT_USER_INTERRUPT | GT_PIPE_NOTIFY))
 		notify_ring(dev, &dev_priv->ring[RCS]);
-	if (gt_iir & GT_GEN6_BSD_USER_INTERRUPT)
+	if (gt_iir & GEN6_BSD_USER_INTERRUPT)
 		notify_ring(dev, &dev_priv->ring[VCS]);
-	if (gt_iir & GT_BLT_USER_INTERRUPT)
+	if (gt_iir & GEN6_BLITTER_USER_INTERRUPT)
 		notify_ring(dev, &dev_priv->ring[BCS]);
 
 	if (de_iir & DE_GSE_IVB)
@@ -571,7 +571,7 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
 	atomic_inc(&dev_priv->irq_received);
 
 	if (IS_GEN6(dev))
-		bsd_usr_interrupt = GT_GEN6_BSD_USER_INTERRUPT;
+		bsd_usr_interrupt = GEN6_BSD_USER_INTERRUPT;
 
 	/* disable master interrupt before clearing iir  */
 	de_ier = I915_READ(DEIER);
@@ -605,7 +605,7 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
 		notify_ring(dev, &dev_priv->ring[RCS]);
 	if (gt_iir & bsd_usr_interrupt)
 		notify_ring(dev, &dev_priv->ring[VCS]);
-	if (gt_iir & GT_BLT_USER_INTERRUPT)
+	if (gt_iir & GEN6_BLITTER_USER_INTERRUPT)
 		notify_ring(dev, &dev_priv->ring[BCS]);
 
 	if (de_iir & DE_GSE)
@@ -1869,8 +1869,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 	if (IS_GEN6(dev))
 		render_irqs =
 			GT_USER_INTERRUPT |
-			GT_GEN6_BSD_USER_INTERRUPT |
-			GT_BLT_USER_INTERRUPT;
+			GEN6_BSD_USER_INTERRUPT |
+			GEN6_BLITTER_USER_INTERRUPT;
 	else
 		render_irqs =
 			GT_USER_INTERRUPT |
@@ -1942,8 +1942,8 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(GTIIR, I915_READ(GTIIR));
 	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
 
-	render_irqs = GT_USER_INTERRUPT | GT_GEN6_BSD_USER_INTERRUPT |
-		GT_BLT_USER_INTERRUPT;
+	render_irqs = GT_USER_INTERRUPT | GEN6_BSD_USER_INTERRUPT |
+		GEN6_BLITTER_USER_INTERRUPT;
 	I915_WRITE(GTIER, render_irqs);
 	POSTING_READ(GTIER);
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 52a06be..e906885 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -589,7 +589,7 @@
 #define   GEN6_RENDER_COMMAND_PARSER_MASTER_ERROR	(1 << 3)
 #define   GEN6_RENDER_SYNC_STATUS			(1 << 2)
 #define   GEN6_RENDER_DEBUG_INTERRUPT			(1 << 1)
-#define   GEN6_RENDER_USER_INTERRUPT			(1 << 0)
+#define   GEN6_RENDER_USER_INTERRUPT			(1 << 0) /* Alias for GT_USER_INTERRUPT */
 
 #define GEN6_BLITTER_HWSTAM	0x22098
 #define GEN6_BLITTER_IMR	0x220a8
@@ -3069,12 +3069,10 @@
 #define DEIER   0x4400c
 
 /* GT interrupt */
+#define GT_BSD_USER_INTERRUPT   (1 << 5)
 #define GT_PIPE_NOTIFY		(1 << 4)
 #define GT_SYNC_STATUS          (1 << 2)
 #define GT_USER_INTERRUPT       (1 << 0)
-#define GT_BSD_USER_INTERRUPT   (1 << 5)
-#define GT_GEN6_BSD_USER_INTERRUPT	(1 << 12)
-#define GT_BLT_USER_INTERRUPT	(1 << 22)
 
 #define GTISR   0x44010
 #define GTIMR   0x44014
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index b7b50a5..25b9b9c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -788,7 +788,7 @@ ring_add_request(struct intel_ring_buffer *ring,
 }
 
 static bool
-gen6_ring_get_irq(struct intel_ring_buffer *ring, u32 gflag, u32 rflag)
+gen6_ring_get_irq(struct intel_ring_buffer *ring, u32 mask)
 {
 	struct drm_device *dev = ring->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
@@ -803,9 +803,9 @@ gen6_ring_get_irq(struct intel_ring_buffer *ring, u32 gflag, u32 rflag)
 
 	spin_lock(&ring->irq_lock);
 	if (ring->irq_refcount++ == 0) {
-		ring->irq_mask &= ~rflag;
+		ring->irq_mask &= ~mask;
 		I915_WRITE_IMR(ring, ring->irq_mask);
-		ironlake_enable_irq(dev_priv, gflag);
+		ironlake_enable_irq(dev_priv, mask);
 	}
 	spin_unlock(&ring->irq_lock);
 
@@ -813,16 +813,16 @@ gen6_ring_get_irq(struct intel_ring_buffer *ring, u32 gflag, u32 rflag)
 }
 
 static void
-gen6_ring_put_irq(struct intel_ring_buffer *ring, u32 gflag, u32 rflag)
+gen6_ring_put_irq(struct intel_ring_buffer *ring, u32 mask)
 {
 	struct drm_device *dev = ring->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
 	spin_lock(&ring->irq_lock);
 	if (--ring->irq_refcount == 0) {
-		ring->irq_mask |= rflag;
+		ring->irq_mask |= mask;
 		I915_WRITE_IMR(ring, ring->irq_mask);
-		ironlake_disable_irq(dev_priv, gflag);
+		ironlake_disable_irq(dev_priv, mask);
 	}
 	spin_unlock(&ring->irq_lock);
 
@@ -1376,33 +1376,25 @@ gen6_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
 static bool
 gen6_render_ring_get_irq(struct intel_ring_buffer *ring)
 {
-	return gen6_ring_get_irq(ring,
-				 GT_USER_INTERRUPT,
-				 GEN6_RENDER_USER_INTERRUPT);
+	return gen6_ring_get_irq(ring, GT_USER_INTERRUPT);
 }
 
 static void
 gen6_render_ring_put_irq(struct intel_ring_buffer *ring)
 {
-	return gen6_ring_put_irq(ring,
-				 GT_USER_INTERRUPT,
-				 GEN6_RENDER_USER_INTERRUPT);
+	return gen6_ring_put_irq(ring, GT_USER_INTERRUPT);
 }
 
 static bool
 gen6_bsd_ring_get_irq(struct intel_ring_buffer *ring)
 {
-	return gen6_ring_get_irq(ring,
-				 GT_GEN6_BSD_USER_INTERRUPT,
-				 GEN6_BSD_USER_INTERRUPT);
+	return gen6_ring_get_irq(ring, GEN6_BSD_USER_INTERRUPT);
 }
 
 static void
 gen6_bsd_ring_put_irq(struct intel_ring_buffer *ring)
 {
-	return gen6_ring_put_irq(ring,
-				 GT_GEN6_BSD_USER_INTERRUPT,
-				 GEN6_BSD_USER_INTERRUPT);
+	return gen6_ring_put_irq(ring, GEN6_BSD_USER_INTERRUPT);
 }
 
 /* ring buffer for Video Codec for Gen6+ */
@@ -1431,17 +1423,13 @@ static const struct intel_ring_buffer gen6_bsd_ring = {
 static bool
 blt_ring_get_irq(struct intel_ring_buffer *ring)
 {
-	return gen6_ring_get_irq(ring,
-				 GT_BLT_USER_INTERRUPT,
-				 GEN6_BLITTER_USER_INTERRUPT);
+	return gen6_ring_get_irq(ring, GEN6_BLITTER_USER_INTERRUPT);
 }
 
 static void
 blt_ring_put_irq(struct intel_ring_buffer *ring)
 {
-	gen6_ring_put_irq(ring,
-			  GT_BLT_USER_INTERRUPT,
-			  GEN6_BLITTER_USER_INTERRUPT);
+	gen6_ring_put_irq(ring, GEN6_BLITTER_USER_INTERRUPT);
 }
 
 static int blt_ring_flush(struct intel_ring_buffer *ring,
-- 
1.7.9.5

[PATCH 2/2] drm/i915: open code gen6+ ring irqs

[Ben Widawsky]

We can now open-code the get/put irq functions as they were just
abstracting single register definitions.

It would be nice to merge this in with the IRQ handling code... but that
is too much work for me at present. In addition I could probably
collapse this in to a lot of the Ironlake stuff, but I don't think it's
worth the potential regressions.

This patch itself should not effect functionality.

CC: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   62 ++++++++-----------------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |    1 +
 2 files changed, 17 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 25b9b9c..6b12ede 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -788,10 +788,11 @@ ring_add_request(struct intel_ring_buffer *ring,
 }
 
 static bool
-gen6_ring_get_irq(struct intel_ring_buffer *ring, u32 mask)
+gen6_ring_get_irq(struct intel_ring_buffer *ring)
 {
 	struct drm_device *dev = ring->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	u32 mask = ring->irq_enable;
 
 	if (!dev->irq_enabled)
 	       return false;
@@ -813,10 +814,11 @@ gen6_ring_get_irq(struct intel_ring_buffer *ring, u32 mask)
 }
 
 static void
-gen6_ring_put_irq(struct intel_ring_buffer *ring, u32 mask)
+gen6_ring_put_irq(struct intel_ring_buffer *ring)
 {
 	struct drm_device *dev = ring->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	u32 mask = ring->irq_enable;
 
 	spin_lock(&ring->irq_lock);
 	if (--ring->irq_refcount == 0) {
@@ -1373,30 +1375,6 @@ gen6_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
 	return 0;
 }
 
-static bool
-gen6_render_ring_get_irq(struct intel_ring_buffer *ring)
-{
-	return gen6_ring_get_irq(ring, GT_USER_INTERRUPT);
-}
-
-static void
-gen6_render_ring_put_irq(struct intel_ring_buffer *ring)
-{
-	return gen6_ring_put_irq(ring, GT_USER_INTERRUPT);
-}
-
-static bool
-gen6_bsd_ring_get_irq(struct intel_ring_buffer *ring)
-{
-	return gen6_ring_get_irq(ring, GEN6_BSD_USER_INTERRUPT);
-}
-
-static void
-gen6_bsd_ring_put_irq(struct intel_ring_buffer *ring)
-{
-	return gen6_ring_put_irq(ring, GEN6_BSD_USER_INTERRUPT);
-}
-
 /* ring buffer for Video Codec for Gen6+ */
 static const struct intel_ring_buffer gen6_bsd_ring = {
 	.name			= "gen6 bsd ring",
@@ -1408,8 +1386,9 @@ static const struct intel_ring_buffer gen6_bsd_ring = {
 	.flush			= gen6_ring_flush,
 	.add_request		= gen6_add_request,
 	.get_seqno		= gen6_ring_get_seqno,
-	.irq_get		= gen6_bsd_ring_get_irq,
-	.irq_put		= gen6_bsd_ring_put_irq,
+	.irq_enable		= GEN6_BSD_USER_INTERRUPT,
+	.irq_get		= gen6_ring_get_irq,
+	.irq_put		= gen6_ring_put_irq,
 	.dispatch_execbuffer	= gen6_ring_dispatch_execbuffer,
 	.sync_to		= gen6_bsd_ring_sync_to,
 	.semaphore_register	= {MI_SEMAPHORE_SYNC_VR,
@@ -1420,18 +1399,6 @@ static const struct intel_ring_buffer gen6_bsd_ring = {
 
 /* Blitter support (SandyBridge+) */
 
-static bool
-blt_ring_get_irq(struct intel_ring_buffer *ring)
-{
-	return gen6_ring_get_irq(ring, GEN6_BLITTER_USER_INTERRUPT);
-}
-
-static void
-blt_ring_put_irq(struct intel_ring_buffer *ring)
-{
-	gen6_ring_put_irq(ring, GEN6_BLITTER_USER_INTERRUPT);
-}
-
 static int blt_ring_flush(struct intel_ring_buffer *ring,
 			  u32 invalidate, u32 flush)
 {
@@ -1463,8 +1430,9 @@ static const struct intel_ring_buffer gen6_blt_ring = {
 	.flush			= blt_ring_flush,
 	.add_request		= gen6_add_request,
 	.get_seqno		= gen6_ring_get_seqno,
-	.irq_get		= blt_ring_get_irq,
-	.irq_put		= blt_ring_put_irq,
+	.irq_get		= gen6_ring_get_irq,
+	.irq_put		= gen6_ring_put_irq,
+	.irq_enable		= GEN6_BLITTER_USER_INTERRUPT,
 	.dispatch_execbuffer	= gen6_ring_dispatch_execbuffer,
 	.sync_to		= gen6_blt_ring_sync_to,
 	.semaphore_register	= {MI_SEMAPHORE_SYNC_BR,
@@ -1482,8 +1450,9 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 	if (INTEL_INFO(dev)->gen >= 6) {
 		ring->add_request = gen6_add_request;
 		ring->flush = gen6_render_ring_flush;
-		ring->irq_get = gen6_render_ring_get_irq;
-		ring->irq_put = gen6_render_ring_put_irq;
+		ring->irq_get = gen6_ring_get_irq;
+		ring->irq_put = gen6_ring_put_irq;
+		ring->irq_enable = GT_USER_INTERRUPT;
 		ring->get_seqno = gen6_ring_get_seqno;
 	} else if (IS_GEN5(dev)) {
 		ring->add_request = pc_render_add_request;
@@ -1506,8 +1475,9 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
 	*ring = render_ring;
 	if (INTEL_INFO(dev)->gen >= 6) {
 		ring->add_request = gen6_add_request;
-		ring->irq_get = gen6_render_ring_get_irq;
-		ring->irq_put = gen6_render_ring_put_irq;
+		ring->irq_get = gen6_ring_get_irq;
+		ring->irq_put = gen6_ring_put_irq;
+		ring->irq_enable = GT_USER_INTERRUPT;
 	} else if (IS_GEN5(dev)) {
 		ring->add_request = pc_render_add_request;
 		ring->get_seqno = pc_render_get_seqno;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index bc0365b..3488a5a 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -59,6 +59,7 @@ struct  intel_ring_buffer {
 	spinlock_t	irq_lock;
 	u32		irq_refcount;
 	u32		irq_mask;
+	u32		irq_enable;		/* IRQs enabled for this ring */
 	u32		irq_seqno;		/* last seq seem at irq time */
 	u32		trace_irq_seqno;
 	u32		waiting_seqno;
-- 
1.7.9.5

Re: [PATCH 1/2] drm/i915: ring irq cleanups

[Chris Wilson]

On Thu, 29 Mar 2012 19:11:26 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> - gen6 put/get only need one argument
>     rflags and gflags are always the same (see above explanation)
> - remove a couple redundantly defined IRQs
> - reordered some lines to make things go in descending order
> 
> Every ring has its own interrupts, enables, masks, and status bits that
> are fed into the main interrupt enable/mask/status registers. At one
> point in time it seemed like a good idea to make our functions support
> the notion that each interrupt may have a different bit position in the
> corresponding register (blitter parser error may be bit n in IMR, but
> bit m in blitter IMR). It turned out though that the HW designers did us
> a solid on Gen6+ and this unfortunate situation has been avoided. This
> allows our interrupt code to be cleaned up a bit.
> 
> I jammed this into one commit because there should be no functional
> change with this commit, and staging it into multiple commits was
> unnecessarily artificial IMO.
> 
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> CC: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>

Those two patches are
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

We can go further in simplifying get/put irq. First requires a
dev_priv->gt.enable_irq() and then refactor the common (pro|epi)logue.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: ring irq cleanups

[Daniel Vetter]

On Fri, Mar 30, 2012 at 09:33:22AM +0100, Chris Wilson wrote:
> On Thu, 29 Mar 2012 19:11:26 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > - gen6 put/get only need one argument
> >     rflags and gflags are always the same (see above explanation)
> > - remove a couple redundantly defined IRQs
> > - reordered some lines to make things go in descending order
> > 
> > Every ring has its own interrupts, enables, masks, and status bits that
> > are fed into the main interrupt enable/mask/status registers. At one
> > point in time it seemed like a good idea to make our functions support
> > the notion that each interrupt may have a different bit position in the
> > corresponding register (blitter parser error may be bit n in IMR, but
> > bit m in blitter IMR). It turned out though that the HW designers did us
> > a solid on Gen6+ and this unfortunate situation has been avoided. This
> > allows our interrupt code to be cleaned up a bit.
> > 
> > I jammed this into one commit because there should be no functional
> > change with this commit, and staging it into multiple commits was
> > unnecessarily artificial IMO.
> > 
> > CC: Chris Wilson <chris@chris-wilson.co.uk>
> > CC: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> 
> Those two patches are
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Both patches applied, with a few things added to i915_reg.h for the first
one - I got confused about this stuff too much. Thanks for wrestling that
red dragon.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48