[PATCH 00/14] intel_ringbuffer.c reorg + cleanups

[PATCH 00/14] intel_ringbuffer.c reorg + cleanups

[Daniel Vetter]

Hi all,

This patch series is inspired by Ben's ring->get|put_irq cleanup for gen6+ and
my perpetual hatred for intel_ringbuffer.c.

It's a lot of churn, but the end result is imho worth it - I almost started to
like what the ringbuffer abstraction looks like now. There are some follow-up
cleanups possible, but I think that can wait until we've cleanup up our domain
tracking and ripped out the flushing_list (if that ever happens).

Commments, flames and review highly welcome.

Yours, Daniel

Daniel Vetter (14):
  drm/i915: rip out ring->irq_mask
  drm/i915: set ring->size in common ring setup code
  drm/i915: dynamically set up the render ring functions and params
  drm/i915: dynamically set up bsd ring functions and params
  drm/i915: dynamically set up blt ring functions and parameters
  drm/i915: don't set up rings on gen6+ for non-kms
  drm/i915: consolidate ring->sync-to functions
  drm/i915: abstract away ring-specific irq_get/put
  drm/i915: split out the gen5 ring irq get/put functions
  drm/i915: don't enable the gen6 bsd ring tail write enable on gen7
  drm/i915: split up ring->dispatch_execbuffer functions
  drm/i915: consolidate ring->flush a bit
  drm/i915: don't set up gem ring functions on gen5 for !kms
  drm/i915: inline enable/disable_irq into ring->get/put_irq

 drivers/gpu/drm/i915/intel_ringbuffer.c |  466 +++++++++++++------------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |    3 +-
 2 files changed, 196 insertions(+), 273 deletions(-)

-- 
1.7.7.5

[PATCH 01/14] drm/i915: rip out ring->irq_mask

[Daniel Vetter]

We only ever enable/disable one interrupt (namely user_interrupts and
pipe_notify), so we don't need to track the interrupt masking state.

Also rename irq_enable to irq_enable_mask, now that it won't collide -
beforehand both a irq_mask and irq_enable_mask would have looked a bit
strange.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   21 ++++++++-------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |    3 +--
 2 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index dfdb613..54595cd 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -792,7 +792,6 @@ 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;
@@ -804,9 +803,8 @@ gen6_ring_get_irq(struct intel_ring_buffer *ring)
 
 	spin_lock(&ring->irq_lock);
 	if (ring->irq_refcount++ == 0) {
-		ring->irq_mask &= ~mask;
-		I915_WRITE_IMR(ring, ring->irq_mask);
-		ironlake_enable_irq(dev_priv, mask);
+		I915_WRITE_IMR(ring, ~ring->irq_enable_mask);
+		ironlake_enable_irq(dev_priv, ring->irq_enable_mask);
 	}
 	spin_unlock(&ring->irq_lock);
 
@@ -818,13 +816,11 @@ 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) {
-		ring->irq_mask |= mask;
-		I915_WRITE_IMR(ring, ring->irq_mask);
-		ironlake_disable_irq(dev_priv, mask);
+		I915_WRITE_IMR(ring, ~0);
+		ironlake_disable_irq(dev_priv, ring->irq_enable_mask);
 	}
 	spin_unlock(&ring->irq_lock);
 
@@ -996,7 +992,6 @@ int intel_init_ring_buffer(struct drm_device *dev,
 
 	init_waitqueue_head(&ring->irq_queue);
 	spin_lock_init(&ring->irq_lock);
-	ring->irq_mask = ~0;
 
 	if (I915_NEED_GFX_HWS(dev)) {
 		ret = init_status_page(ring);
@@ -1374,7 +1369,7 @@ 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_enable		= GEN6_BSD_USER_INTERRUPT,
+	.irq_enable_mask	= GEN6_BSD_USER_INTERRUPT,
 	.irq_get		= gen6_ring_get_irq,
 	.irq_put		= gen6_ring_put_irq,
 	.dispatch_execbuffer	= gen6_ring_dispatch_execbuffer,
@@ -1420,7 +1415,7 @@ static const struct intel_ring_buffer gen6_blt_ring = {
 	.get_seqno		= gen6_ring_get_seqno,
 	.irq_get		= gen6_ring_get_irq,
 	.irq_put		= gen6_ring_put_irq,
-	.irq_enable		= GEN6_BLITTER_USER_INTERRUPT,
+	.irq_enable_mask	= GEN6_BLITTER_USER_INTERRUPT,
 	.dispatch_execbuffer	= gen6_ring_dispatch_execbuffer,
 	.sync_to		= gen6_blt_ring_sync_to,
 	.semaphore_register	= {MI_SEMAPHORE_SYNC_BR,
@@ -1440,7 +1435,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 		ring->flush = gen6_render_ring_flush;
 		ring->irq_get = gen6_ring_get_irq;
 		ring->irq_put = gen6_ring_put_irq;
-		ring->irq_enable = GT_USER_INTERRUPT;
+		ring->irq_enable_mask = GT_USER_INTERRUPT;
 		ring->get_seqno = gen6_ring_get_seqno;
 	} else if (IS_GEN5(dev)) {
 		ring->add_request = pc_render_add_request;
@@ -1465,7 +1460,7 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
 		ring->add_request = gen6_add_request;
 		ring->irq_get = gen6_ring_get_irq;
 		ring->irq_put = gen6_ring_put_irq;
-		ring->irq_enable = GT_USER_INTERRUPT;
+		ring->irq_enable_mask = 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 3488a5a..06a66ad 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -58,8 +58,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_enable_mask;	/* bitmask to enable ring interrupt */
 	u32		irq_seqno;		/* last seq seem at irq time */
 	u32		trace_irq_seqno;
 	u32		waiting_seqno;
-- 
1.7.7.5

[PATCH 02/14] drm/i915: set ring->size in common ring setup code

[Daniel Vetter]

Eventually we want to scale the ring size depending upon available
gtt space. For now just consolidate this instead of replicating it
over all ringbuffer templates.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 54595cd..8131c40 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -989,6 +989,7 @@ int intel_init_ring_buffer(struct drm_device *dev,
 	INIT_LIST_HEAD(&ring->active_list);
 	INIT_LIST_HEAD(&ring->request_list);
 	INIT_LIST_HEAD(&ring->gpu_write_list);
+	ring->size = 32 * PAGE_SIZE;
 
 	init_waitqueue_head(&ring->irq_queue);
 	spin_lock_init(&ring->irq_lock);
@@ -1262,7 +1263,6 @@ static const struct intel_ring_buffer render_ring = {
 	.name			= "render ring",
 	.id			= RCS,
 	.mmio_base		= RENDER_RING_BASE,
-	.size			= 32 * PAGE_SIZE,
 	.init			= init_render_ring,
 	.write_tail		= ring_write_tail,
 	.flush			= render_ring_flush,
@@ -1285,7 +1285,6 @@ static const struct intel_ring_buffer bsd_ring = {
 	.name                   = "bsd ring",
 	.id			= VCS,
 	.mmio_base		= BSD_RING_BASE,
-	.size			= 32 * PAGE_SIZE,
 	.init			= init_ring_common,
 	.write_tail		= ring_write_tail,
 	.flush			= bsd_ring_flush,
@@ -1363,7 +1362,6 @@ static const struct intel_ring_buffer gen6_bsd_ring = {
 	.name			= "gen6 bsd ring",
 	.id			= VCS,
 	.mmio_base		= GEN6_BSD_RING_BASE,
-	.size			= 32 * PAGE_SIZE,
 	.init			= init_ring_common,
 	.write_tail		= gen6_bsd_ring_write_tail,
 	.flush			= gen6_ring_flush,
@@ -1407,7 +1405,6 @@ static const struct intel_ring_buffer gen6_blt_ring = {
 	.name			= "blt ring",
 	.id			= BCS,
 	.mmio_base		= BLT_RING_BASE,
-	.size			= 32 * PAGE_SIZE,
 	.init			= init_ring_common,
 	.write_tail		= ring_write_tail,
 	.flush			= blt_ring_flush,
-- 
1.7.7.5

[PATCH 03/14] drm/i915: dynamically set up the render ring functions and params

[Daniel Vetter]

Our hw is simply not well-designed enough that it neatly fits into
boxes. Everywhere else we set up vtables and similar things
dynamically using switch statements - it's simply much more flexible.

This is prep work to rework the pre-gen6 ring irq stuff - it'll add a
few more differences. With the current const struct templates, that
would be a mess.

This leads to some unfortunate duplication with the old dri1 code, but
we can reap that again because gen6 isn't actually supported there.
But that's for a separate patch.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   71 +++++++++++++++++++++----------
 1 files changed, 49 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 8131c40..03ead75 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1259,26 +1259,6 @@ void intel_ring_advance(struct intel_ring_buffer *ring)
 	ring->write_tail(ring, ring->tail);
 }
 
-static const struct intel_ring_buffer render_ring = {
-	.name			= "render ring",
-	.id			= RCS,
-	.mmio_base		= RENDER_RING_BASE,
-	.init			= init_render_ring,
-	.write_tail		= ring_write_tail,
-	.flush			= render_ring_flush,
-	.add_request		= render_ring_add_request,
-	.get_seqno		= ring_get_seqno,
-	.irq_get		= render_ring_get_irq,
-	.irq_put		= render_ring_put_irq,
-	.dispatch_execbuffer	= render_ring_dispatch_execbuffer,
-	.cleanup		= render_ring_cleanup,
-	.sync_to		= render_ring_sync_to,
-	.semaphore_register	= {MI_SEMAPHORE_SYNC_INVALID,
-				   MI_SEMAPHORE_SYNC_RV,
-				   MI_SEMAPHORE_SYNC_RB},
-	.signal_mbox		= {GEN6_VRSYNC, GEN6_BRSYNC},
-};
-
 /* ring buffer for bit-stream decoder */
 
 static const struct intel_ring_buffer bsd_ring = {
@@ -1426,7 +1406,10 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
 
-	*ring = render_ring;
+	ring->name = "render ring";
+	ring->id = RCS;
+	ring->mmio_base = RENDER_RING_BASE;
+
 	if (INTEL_INFO(dev)->gen >= 6) {
 		ring->add_request = gen6_add_request;
 		ring->flush = gen6_render_ring_flush;
@@ -1434,10 +1417,30 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 		ring->irq_put = gen6_ring_put_irq;
 		ring->irq_enable_mask = GT_USER_INTERRUPT;
 		ring->get_seqno = gen6_ring_get_seqno;
+		ring->sync_to = render_ring_sync_to;
+		ring->semaphore_register[0] = MI_SEMAPHORE_SYNC_INVALID;
+		ring->semaphore_register[1] = MI_SEMAPHORE_SYNC_RV;
+		ring->semaphore_register[2] = MI_SEMAPHORE_SYNC_RB;
+		ring->signal_mbox[0] = GEN6_VRSYNC;
+		ring->signal_mbox[1] = GEN6_BRSYNC;
 	} else if (IS_GEN5(dev)) {
 		ring->add_request = pc_render_add_request;
+		ring->flush = render_ring_flush;
 		ring->get_seqno = pc_render_get_seqno;
+		ring->irq_get = render_ring_get_irq;
+		ring->irq_put = render_ring_put_irq;
+	} else {
+		ring->add_request = render_ring_add_request;
+		ring->flush = render_ring_flush;
+		ring->get_seqno = ring_get_seqno;
+		ring->irq_get = render_ring_get_irq;
+		ring->irq_put = render_ring_put_irq;
 	}
+	ring->write_tail = ring_write_tail;
+	ring->dispatch_execbuffer = render_ring_dispatch_execbuffer;
+	ring->init = init_render_ring;
+	ring->cleanup = render_ring_cleanup;
+
 
 	if (!I915_NEED_GFX_HWS(dev)) {
 		ring->status_page.page_addr = dev_priv->status_page_dmah->vaddr;
@@ -1452,16 +1455,40 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
 
-	*ring = render_ring;
+	ring->name = "render ring";
+	ring->id = RCS;
+	ring->mmio_base = RENDER_RING_BASE;
+
 	if (INTEL_INFO(dev)->gen >= 6) {
 		ring->add_request = gen6_add_request;
+		ring->flush = gen6_render_ring_flush;
 		ring->irq_get = gen6_ring_get_irq;
 		ring->irq_put = gen6_ring_put_irq;
 		ring->irq_enable_mask = GT_USER_INTERRUPT;
+		ring->get_seqno = gen6_ring_get_seqno;
+		ring->sync_to = render_ring_sync_to;
+		ring->semaphore_register[0] = MI_SEMAPHORE_SYNC_INVALID;
+		ring->semaphore_register[1] = MI_SEMAPHORE_SYNC_RV;
+		ring->semaphore_register[2] = MI_SEMAPHORE_SYNC_RB;
+		ring->signal_mbox[0] = GEN6_VRSYNC;
+		ring->signal_mbox[1] = GEN6_BRSYNC;
 	} else if (IS_GEN5(dev)) {
 		ring->add_request = pc_render_add_request;
+		ring->flush = render_ring_flush;
 		ring->get_seqno = pc_render_get_seqno;
+		ring->irq_get = render_ring_get_irq;
+		ring->irq_put = render_ring_put_irq;
+	} else {
+		ring->add_request = render_ring_add_request;
+		ring->flush = render_ring_flush;
+		ring->get_seqno = ring_get_seqno;
+		ring->irq_get = render_ring_get_irq;
+		ring->irq_put = render_ring_put_irq;
 	}
+	ring->write_tail = ring_write_tail;
+	ring->dispatch_execbuffer = render_ring_dispatch_execbuffer;
+	ring->init = init_render_ring;
+	ring->cleanup = render_ring_cleanup;
 
 	if (!I915_NEED_GFX_HWS(dev))
 		ring->status_page.page_addr = dev_priv->status_page_dmah->vaddr;
-- 
1.7.7.5

[PATCH 04/14] drm/i915: dynamically set up bsd ring functions and params

[Daniel Vetter]

The same treatment for the bds ring. Again, this will be split up
further by the irq rework.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   72 +++++++++++++-----------------
 1 files changed, 31 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 03ead75..32ff8c7 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1259,22 +1259,6 @@ void intel_ring_advance(struct intel_ring_buffer *ring)
 	ring->write_tail(ring, ring->tail);
 }
 
-/* ring buffer for bit-stream decoder */
-
-static const struct intel_ring_buffer bsd_ring = {
-	.name                   = "bsd ring",
-	.id			= VCS,
-	.mmio_base		= BSD_RING_BASE,
-	.init			= init_ring_common,
-	.write_tail		= ring_write_tail,
-	.flush			= bsd_ring_flush,
-	.add_request		= ring_add_request,
-	.get_seqno		= ring_get_seqno,
-	.irq_get		= bsd_ring_get_irq,
-	.irq_put		= bsd_ring_put_irq,
-	.dispatch_execbuffer	= ring_dispatch_execbuffer,
-};
-
 
 static void gen6_bsd_ring_write_tail(struct intel_ring_buffer *ring,
 				     u32 value)
@@ -1337,27 +1321,6 @@ gen6_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
 	return 0;
 }
 
-/* ring buffer for Video Codec for Gen6+ */
-static const struct intel_ring_buffer gen6_bsd_ring = {
-	.name			= "gen6 bsd ring",
-	.id			= VCS,
-	.mmio_base		= GEN6_BSD_RING_BASE,
-	.init			= init_ring_common,
-	.write_tail		= gen6_bsd_ring_write_tail,
-	.flush			= gen6_ring_flush,
-	.add_request		= gen6_add_request,
-	.get_seqno		= gen6_ring_get_seqno,
-	.irq_enable_mask	= 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,
-				   MI_SEMAPHORE_SYNC_INVALID,
-				   MI_SEMAPHORE_SYNC_VB},
-	.signal_mbox		= {GEN6_RVSYNC, GEN6_BVSYNC},
-};
-
 /* Blitter support (SandyBridge+) */
 
 static int blt_ring_flush(struct intel_ring_buffer *ring,
@@ -1525,10 +1488,37 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct intel_ring_buffer *ring = &dev_priv->ring[VCS];
 
-	if (IS_GEN6(dev) || IS_GEN7(dev))
-		*ring = gen6_bsd_ring;
-	else
-		*ring = bsd_ring;
+	ring->name = "bsd ring";
+	ring->id = VCS;
+
+	if (IS_GEN6(dev) || IS_GEN7(dev)) {
+		ring->mmio_base = GEN6_BSD_RING_BASE;
+		ring->write_tail = gen6_bsd_ring_write_tail;
+		ring->flush = gen6_ring_flush;
+		ring->add_request = gen6_add_request;
+		ring->get_seqno = gen6_ring_get_seqno;
+		ring->irq_enable_mask = GEN6_BSD_USER_INTERRUPT;
+		ring->irq_get = gen6_ring_get_irq;
+		ring->irq_put = gen6_ring_put_irq;
+		ring->dispatch_execbuffer = gen6_ring_dispatch_execbuffer;
+		ring->sync_to = gen6_bsd_ring_sync_to;
+		ring->semaphore_register[0] = MI_SEMAPHORE_SYNC_VR;
+		ring->semaphore_register[1] = MI_SEMAPHORE_SYNC_INVALID;
+		ring->semaphore_register[2] = MI_SEMAPHORE_SYNC_VB;
+		ring->signal_mbox[0] = GEN6_RVSYNC;
+		ring->signal_mbox[1] = GEN6_BVSYNC;
+	} else {
+		ring->mmio_base = BSD_RING_BASE;
+		ring->write_tail = ring_write_tail;
+		ring->flush = bsd_ring_flush;
+		ring->add_request = ring_add_request;
+		ring->get_seqno = ring_get_seqno;
+		ring->irq_get = bsd_ring_get_irq;
+		ring->irq_put = bsd_ring_put_irq;
+		ring->dispatch_execbuffer = ring_dispatch_execbuffer;
+	}
+	ring->init = init_ring_common;
+
 
 	return intel_init_ring_buffer(dev, ring);
 }
-- 
1.7.7.5

[PATCH 05/14] drm/i915: dynamically set up blt ring functions and parameters

[Daniel Vetter]

Just for consistency.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   40 ++++++++++++++----------------
 1 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 32ff8c7..ac2d10b 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1344,26 +1344,6 @@ static int blt_ring_flush(struct intel_ring_buffer *ring,
 	return 0;
 }
 
-static const struct intel_ring_buffer gen6_blt_ring = {
-	.name			= "blt ring",
-	.id			= BCS,
-	.mmio_base		= BLT_RING_BASE,
-	.init			= init_ring_common,
-	.write_tail		= ring_write_tail,
-	.flush			= blt_ring_flush,
-	.add_request		= gen6_add_request,
-	.get_seqno		= gen6_ring_get_seqno,
-	.irq_get		= gen6_ring_get_irq,
-	.irq_put		= gen6_ring_put_irq,
-	.irq_enable_mask	= GEN6_BLITTER_USER_INTERRUPT,
-	.dispatch_execbuffer	= gen6_ring_dispatch_execbuffer,
-	.sync_to		= gen6_blt_ring_sync_to,
-	.semaphore_register	= {MI_SEMAPHORE_SYNC_BR,
-				   MI_SEMAPHORE_SYNC_BV,
-				   MI_SEMAPHORE_SYNC_INVALID},
-	.signal_mbox		= {GEN6_RBSYNC, GEN6_VBSYNC},
-};
-
 int intel_init_render_ring_buffer(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
@@ -1528,7 +1508,25 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct intel_ring_buffer *ring = &dev_priv->ring[BCS];
 
-	*ring = gen6_blt_ring;
+	ring->name = "blitter ring";
+	ring->id = BCS;
+
+	ring->mmio_base = BLT_RING_BASE;
+	ring->write_tail = ring_write_tail;
+	ring->flush = blt_ring_flush;
+	ring->add_request = gen6_add_request;
+	ring->get_seqno = gen6_ring_get_seqno;
+	ring->irq_enable_mask = GEN6_BLITTER_USER_INTERRUPT;
+	ring->irq_get = gen6_ring_get_irq;
+	ring->irq_put = gen6_ring_put_irq;
+	ring->dispatch_execbuffer = gen6_ring_dispatch_execbuffer;
+	ring->sync_to = gen6_blt_ring_sync_to;
+	ring->semaphore_register[0] = MI_SEMAPHORE_SYNC_BR;
+	ring->semaphore_register[1] = MI_SEMAPHORE_SYNC_BV;
+	ring->semaphore_register[2] = MI_SEMAPHORE_SYNC_INVALID;
+	ring->signal_mbox[0] = GEN6_RBSYNC;
+	ring->signal_mbox[1] = GEN6_VBSYNC;
+	ring->init = init_ring_common;
 
 	return intel_init_ring_buffer(dev, ring);
 }
-- 
1.7.7.5

[PATCH 06/14] drm/i915: don't set up rings on gen6+ for non-kms

[Daniel Vetter]

It's not supported, and with the patch to refuse loading on gen6+
without kms enabled, there's also no way we can hit this.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   14 ++------------
 1 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index ac2d10b..7d0a5bc 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1403,18 +1403,8 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
 	ring->mmio_base = RENDER_RING_BASE;
 
 	if (INTEL_INFO(dev)->gen >= 6) {
-		ring->add_request = gen6_add_request;
-		ring->flush = gen6_render_ring_flush;
-		ring->irq_get = gen6_ring_get_irq;
-		ring->irq_put = gen6_ring_put_irq;
-		ring->irq_enable_mask = GT_USER_INTERRUPT;
-		ring->get_seqno = gen6_ring_get_seqno;
-		ring->sync_to = render_ring_sync_to;
-		ring->semaphore_register[0] = MI_SEMAPHORE_SYNC_INVALID;
-		ring->semaphore_register[1] = MI_SEMAPHORE_SYNC_RV;
-		ring->semaphore_register[2] = MI_SEMAPHORE_SYNC_RB;
-		ring->signal_mbox[0] = GEN6_VRSYNC;
-		ring->signal_mbox[1] = GEN6_BRSYNC;
+		/* non-kms not supported on gen6+ */
+		return -ENODEV;
 	} else if (IS_GEN5(dev)) {
 		ring->add_request = pc_render_add_request;
 		ring->flush = render_ring_flush;
-- 
1.7.7.5

[PATCH 07/14] drm/i915: consolidate ring->sync-to functions

[Daniel Vetter]

The waiter is always the ring itself (otherwise we'd have a decent
snafu in a callsite), so we can unify this easily.

Also give it the usual gen6_ prefix, in case anyone is foolish enough to
implement hw semaphores for gen5.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   60 ++++++-------------------------
 1 files changed, 11 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 7d0a5bc..12f9304 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -472,21 +472,24 @@ gen6_add_request(struct intel_ring_buffer *ring,
  * @seqno - seqno which the waiter will block on
  */
 static int
-intel_ring_sync(struct intel_ring_buffer *waiter,
-		struct intel_ring_buffer *signaller,
-		int ring,
-		u32 seqno)
+gen6_ring_sync(struct intel_ring_buffer *waiter,
+	       struct intel_ring_buffer *signaller,
+	       u32 seqno)
 {
 	int ret;
 	u32 dw1 = MI_SEMAPHORE_MBOX |
 		  MI_SEMAPHORE_COMPARE |
 		  MI_SEMAPHORE_REGISTER;
 
+	WARN_ON(signaller->semaphore_register[waiter->id] ==
+		MI_SEMAPHORE_SYNC_INVALID);
+
 	ret = intel_ring_begin(waiter, 4);
 	if (ret)
 		return ret;
 
-	intel_ring_emit(waiter, dw1 | signaller->semaphore_register[ring]);
+	intel_ring_emit(waiter,
+			dw1 | signaller->semaphore_register[waiter->id]);
 	intel_ring_emit(waiter, seqno);
 	intel_ring_emit(waiter, 0);
 	intel_ring_emit(waiter, MI_NOOP);
@@ -495,47 +498,6 @@ intel_ring_sync(struct intel_ring_buffer *waiter,
 	return 0;
 }
 
-/* VCS->RCS (RVSYNC) or BCS->RCS (RBSYNC) */
-int
-render_ring_sync_to(struct intel_ring_buffer *waiter,
-		    struct intel_ring_buffer *signaller,
-		    u32 seqno)
-{
-	WARN_ON(signaller->semaphore_register[RCS] == MI_SEMAPHORE_SYNC_INVALID);
-	return intel_ring_sync(waiter,
-			       signaller,
-			       RCS,
-			       seqno);
-}
-
-/* RCS->VCS (VRSYNC) or BCS->VCS (VBSYNC) */
-int
-gen6_bsd_ring_sync_to(struct intel_ring_buffer *waiter,
-		      struct intel_ring_buffer *signaller,
-		      u32 seqno)
-{
-	WARN_ON(signaller->semaphore_register[VCS] == MI_SEMAPHORE_SYNC_INVALID);
-	return intel_ring_sync(waiter,
-			       signaller,
-			       VCS,
-			       seqno);
-}
-
-/* RCS->BCS (BRSYNC) or VCS->BCS (BVSYNC) */
-int
-gen6_blt_ring_sync_to(struct intel_ring_buffer *waiter,
-		      struct intel_ring_buffer *signaller,
-		      u32 seqno)
-{
-	WARN_ON(signaller->semaphore_register[BCS] == MI_SEMAPHORE_SYNC_INVALID);
-	return intel_ring_sync(waiter,
-			       signaller,
-			       BCS,
-			       seqno);
-}
-
-
-
 #define PIPE_CONTROL_FLUSH(ring__, addr__)					\
 do {									\
 	intel_ring_emit(ring__, GFX_OP_PIPE_CONTROL(4) | PIPE_CONTROL_QW_WRITE |		\
@@ -1360,7 +1322,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 		ring->irq_put = gen6_ring_put_irq;
 		ring->irq_enable_mask = GT_USER_INTERRUPT;
 		ring->get_seqno = gen6_ring_get_seqno;
-		ring->sync_to = render_ring_sync_to;
+		ring->sync_to = gen6_ring_sync;
 		ring->semaphore_register[0] = MI_SEMAPHORE_SYNC_INVALID;
 		ring->semaphore_register[1] = MI_SEMAPHORE_SYNC_RV;
 		ring->semaphore_register[2] = MI_SEMAPHORE_SYNC_RB;
@@ -1471,7 +1433,7 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
 		ring->irq_get = gen6_ring_get_irq;
 		ring->irq_put = gen6_ring_put_irq;
 		ring->dispatch_execbuffer = gen6_ring_dispatch_execbuffer;
-		ring->sync_to = gen6_bsd_ring_sync_to;
+		ring->sync_to = gen6_ring_sync;
 		ring->semaphore_register[0] = MI_SEMAPHORE_SYNC_VR;
 		ring->semaphore_register[1] = MI_SEMAPHORE_SYNC_INVALID;
 		ring->semaphore_register[2] = MI_SEMAPHORE_SYNC_VB;
@@ -1510,7 +1472,7 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
 	ring->irq_get = gen6_ring_get_irq;
 	ring->irq_put = gen6_ring_put_irq;
 	ring->dispatch_execbuffer = gen6_ring_dispatch_execbuffer;
-	ring->sync_to = gen6_blt_ring_sync_to;
+	ring->sync_to = gen6_ring_sync;
 	ring->semaphore_register[0] = MI_SEMAPHORE_SYNC_BR;
 	ring->semaphore_register[1] = MI_SEMAPHORE_SYNC_BV;
 	ring->semaphore_register[2] = MI_SEMAPHORE_SYNC_INVALID;
-- 
1.7.7.5

[PATCH 08/14] drm/i915: abstract away ring-specific irq_get/put

[Daniel Vetter]

Inspired by Ben Widawsky's patch for gen6+. Now after restructuring
how we set up the ring vtables and parameters, we can do this right.

This kills the bsd specific get/put_irq functions, they're now the
same.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   77 ++++++++++---------------------
 1 files changed, 24 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 12f9304..6624a22 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -639,7 +639,7 @@ i915_disable_irq(drm_i915_private_t *dev_priv, u32 mask)
 }
 
 static bool
-render_ring_get_irq(struct intel_ring_buffer *ring)
+i9xx_ring_get_irq(struct intel_ring_buffer *ring)
 {
 	struct drm_device *dev = ring->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
@@ -651,9 +651,9 @@ render_ring_get_irq(struct intel_ring_buffer *ring)
 	if (ring->irq_refcount++ == 0) {
 		if (INTEL_INFO(dev)->gen >= 5)
 			ironlake_enable_irq(dev_priv,
-					    GT_PIPE_NOTIFY | GT_USER_INTERRUPT);
+					    ring->irq_enable_mask);
 		else
-			i915_enable_irq(dev_priv, I915_USER_INTERRUPT);
+			i915_enable_irq(dev_priv, ring->irq_enable_mask);
 	}
 	spin_unlock(&ring->irq_lock);
 
@@ -661,7 +661,7 @@ render_ring_get_irq(struct intel_ring_buffer *ring)
 }
 
 static void
-render_ring_put_irq(struct intel_ring_buffer *ring)
+i9xx_ring_put_irq(struct intel_ring_buffer *ring)
 {
 	struct drm_device *dev = ring->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
@@ -670,10 +670,9 @@ render_ring_put_irq(struct intel_ring_buffer *ring)
 	if (--ring->irq_refcount == 0) {
 		if (INTEL_INFO(dev)->gen >= 5)
 			ironlake_disable_irq(dev_priv,
-					     GT_USER_INTERRUPT |
-					     GT_PIPE_NOTIFY);
+					     ring->irq_enable_mask);
 		else
-			i915_disable_irq(dev_priv, I915_USER_INTERRUPT);
+			i915_disable_irq(dev_priv, ring->irq_enable_mask);
 	}
 	spin_unlock(&ring->irq_lock);
 }
@@ -789,42 +788,6 @@ gen6_ring_put_irq(struct intel_ring_buffer *ring)
 	gen6_gt_force_wake_put(dev_priv);
 }
 
-static bool
-bsd_ring_get_irq(struct intel_ring_buffer *ring)
-{
-	struct drm_device *dev = ring->dev;
-	drm_i915_private_t *dev_priv = dev->dev_private;
-
-	if (!dev->irq_enabled)
-		return false;
-
-	spin_lock(&ring->irq_lock);
-	if (ring->irq_refcount++ == 0) {
-		if (IS_G4X(dev))
-			i915_enable_irq(dev_priv, I915_BSD_USER_INTERRUPT);
-		else
-			ironlake_enable_irq(dev_priv, GT_BSD_USER_INTERRUPT);
-	}
-	spin_unlock(&ring->irq_lock);
-
-	return true;
-}
-static void
-bsd_ring_put_irq(struct intel_ring_buffer *ring)
-{
-	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) {
-		if (IS_G4X(dev))
-			i915_disable_irq(dev_priv, I915_BSD_USER_INTERRUPT);
-		else
-			ironlake_disable_irq(dev_priv, GT_BSD_USER_INTERRUPT);
-	}
-	spin_unlock(&ring->irq_lock);
-}
-
 static int
 ring_dispatch_execbuffer(struct intel_ring_buffer *ring, u32 offset, u32 length)
 {
@@ -1332,14 +1295,16 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 		ring->add_request = pc_render_add_request;
 		ring->flush = render_ring_flush;
 		ring->get_seqno = pc_render_get_seqno;
-		ring->irq_get = render_ring_get_irq;
-		ring->irq_put = render_ring_put_irq;
+		ring->irq_get = i9xx_ring_get_irq;
+		ring->irq_put = i9xx_ring_put_irq;
+		ring->irq_enable_mask = GT_USER_INTERRUPT | GT_PIPE_NOTIFY;
 	} else {
 		ring->add_request = render_ring_add_request;
 		ring->flush = render_ring_flush;
 		ring->get_seqno = ring_get_seqno;
-		ring->irq_get = render_ring_get_irq;
-		ring->irq_put = render_ring_put_irq;
+		ring->irq_get = i9xx_ring_get_irq;
+		ring->irq_put = i9xx_ring_put_irq;
+		ring->irq_enable_mask = I915_USER_INTERRUPT;
 	}
 	ring->write_tail = ring_write_tail;
 	ring->dispatch_execbuffer = render_ring_dispatch_execbuffer;
@@ -1371,14 +1336,16 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
 		ring->add_request = pc_render_add_request;
 		ring->flush = render_ring_flush;
 		ring->get_seqno = pc_render_get_seqno;
-		ring->irq_get = render_ring_get_irq;
-		ring->irq_put = render_ring_put_irq;
+		ring->irq_get = i9xx_ring_get_irq;
+		ring->irq_put = i9xx_ring_put_irq;
+		ring->irq_enable_mask = GT_USER_INTERRUPT | GT_PIPE_NOTIFY;
 	} else {
 		ring->add_request = render_ring_add_request;
 		ring->flush = render_ring_flush;
 		ring->get_seqno = ring_get_seqno;
-		ring->irq_get = render_ring_get_irq;
-		ring->irq_put = render_ring_put_irq;
+		ring->irq_get = i9xx_ring_get_irq;
+		ring->irq_put = i9xx_ring_put_irq;
+		ring->irq_enable_mask = I915_USER_INTERRUPT;
 	}
 	ring->write_tail = ring_write_tail;
 	ring->dispatch_execbuffer = render_ring_dispatch_execbuffer;
@@ -1445,8 +1412,12 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
 		ring->flush = bsd_ring_flush;
 		ring->add_request = ring_add_request;
 		ring->get_seqno = ring_get_seqno;
-		ring->irq_get = bsd_ring_get_irq;
-		ring->irq_put = bsd_ring_put_irq;
+		ring->irq_get = i9xx_ring_get_irq;
+		ring->irq_put = i9xx_ring_put_irq;
+		if (IS_GEN5(dev))
+			ring->irq_enable_mask = GT_BSD_USER_INTERRUPT;
+		else
+			ring->irq_enable_mask = I915_BSD_USER_INTERRUPT;
 		ring->dispatch_execbuffer = ring_dispatch_execbuffer;
 	}
 	ring->init = init_ring_common;
-- 
1.7.7.5

[PATCH 09/14] drm/i915: split out the gen5 ring irq get/put functions

[Daniel Vetter]

Now that we have sensibly split up, we can nicely get rid of that ugly
is_gen5 check.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   66 ++++++++++++++++++++----------
 1 files changed, 44 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 6624a22..36dd660 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -639,6 +639,35 @@ i915_disable_irq(drm_i915_private_t *dev_priv, u32 mask)
 }
 
 static bool
+gen5_ring_get_irq(struct intel_ring_buffer *ring)
+{
+	struct drm_device *dev = ring->dev;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+
+	if (!dev->irq_enabled)
+		return false;
+
+	spin_lock(&ring->irq_lock);
+	if (ring->irq_refcount++ == 0)
+		ironlake_enable_irq(dev_priv, ring->irq_enable_mask);
+	spin_unlock(&ring->irq_lock);
+
+	return true;
+}
+
+static void
+gen5_ring_put_irq(struct intel_ring_buffer *ring)
+{
+	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)
+		ironlake_disable_irq(dev_priv, ring->irq_enable_mask);
+	spin_unlock(&ring->irq_lock);
+}
+
+static bool
 i9xx_ring_get_irq(struct intel_ring_buffer *ring)
 {
 	struct drm_device *dev = ring->dev;
@@ -648,13 +677,8 @@ i9xx_ring_get_irq(struct intel_ring_buffer *ring)
 		return false;
 
 	spin_lock(&ring->irq_lock);
-	if (ring->irq_refcount++ == 0) {
-		if (INTEL_INFO(dev)->gen >= 5)
-			ironlake_enable_irq(dev_priv,
-					    ring->irq_enable_mask);
-		else
-			i915_enable_irq(dev_priv, ring->irq_enable_mask);
-	}
+	if (ring->irq_refcount++ == 0)
+		i915_enable_irq(dev_priv, ring->irq_enable_mask);
 	spin_unlock(&ring->irq_lock);
 
 	return true;
@@ -667,13 +691,8 @@ i9xx_ring_put_irq(struct intel_ring_buffer *ring)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
 	spin_lock(&ring->irq_lock);
-	if (--ring->irq_refcount == 0) {
-		if (INTEL_INFO(dev)->gen >= 5)
-			ironlake_disable_irq(dev_priv,
-					     ring->irq_enable_mask);
-		else
-			i915_disable_irq(dev_priv, ring->irq_enable_mask);
-	}
+	if (--ring->irq_refcount == 0)
+		i915_disable_irq(dev_priv, ring->irq_enable_mask);
 	spin_unlock(&ring->irq_lock);
 }
 
@@ -1295,8 +1314,8 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 		ring->add_request = pc_render_add_request;
 		ring->flush = render_ring_flush;
 		ring->get_seqno = pc_render_get_seqno;
-		ring->irq_get = i9xx_ring_get_irq;
-		ring->irq_put = i9xx_ring_put_irq;
+		ring->irq_get = gen5_ring_get_irq;
+		ring->irq_put = gen5_ring_put_irq;
 		ring->irq_enable_mask = GT_USER_INTERRUPT | GT_PIPE_NOTIFY;
 	} else {
 		ring->add_request = render_ring_add_request;
@@ -1336,8 +1355,8 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
 		ring->add_request = pc_render_add_request;
 		ring->flush = render_ring_flush;
 		ring->get_seqno = pc_render_get_seqno;
-		ring->irq_get = i9xx_ring_get_irq;
-		ring->irq_put = i9xx_ring_put_irq;
+		ring->irq_get = gen5_ring_get_irq;
+		ring->irq_put = gen5_ring_put_irq;
 		ring->irq_enable_mask = GT_USER_INTERRUPT | GT_PIPE_NOTIFY;
 	} else {
 		ring->add_request = render_ring_add_request;
@@ -1412,12 +1431,15 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
 		ring->flush = bsd_ring_flush;
 		ring->add_request = ring_add_request;
 		ring->get_seqno = ring_get_seqno;
-		ring->irq_get = i9xx_ring_get_irq;
-		ring->irq_put = i9xx_ring_put_irq;
-		if (IS_GEN5(dev))
+		if (IS_GEN5(dev)) {
 			ring->irq_enable_mask = GT_BSD_USER_INTERRUPT;
-		else
+			ring->irq_get = gen5_ring_get_irq;
+			ring->irq_put = gen5_ring_put_irq;
+		} else {
 			ring->irq_enable_mask = I915_BSD_USER_INTERRUPT;
+			ring->irq_get = i9xx_ring_get_irq;
+			ring->irq_put = i9xx_ring_put_irq;
+		}
 		ring->dispatch_execbuffer = ring_dispatch_execbuffer;
 	}
 	ring->init = init_ring_common;
-- 
1.7.7.5

[PATCH 10/14] drm/i915: don't enable the gen6 bsd ring tail write enable on gen7

[Daniel Vetter]

HW engineers have fixed this issue for ivb. Again, a nice cleanup
possible thanks to the more flexible ring initialization.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 36dd660..2cb1c8f 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1409,9 +1409,12 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
 	ring->name = "bsd ring";
 	ring->id = VCS;
 
+	ring->write_tail = ring_write_tail;
 	if (IS_GEN6(dev) || IS_GEN7(dev)) {
 		ring->mmio_base = GEN6_BSD_RING_BASE;
-		ring->write_tail = gen6_bsd_ring_write_tail;
+		/* gen6 bsd needs a special wa for tail updates */
+		if (IS_GEN6(dev))
+			ring->write_tail = gen6_bsd_ring_write_tail;
 		ring->flush = gen6_ring_flush;
 		ring->add_request = gen6_add_request;
 		ring->get_seqno = gen6_ring_get_seqno;
@@ -1427,7 +1430,6 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
 		ring->signal_mbox[1] = GEN6_BVSYNC;
 	} else {
 		ring->mmio_base = BSD_RING_BASE;
-		ring->write_tail = ring_write_tail;
 		ring->flush = bsd_ring_flush;
 		ring->add_request = ring_add_request;
 		ring->get_seqno = ring_get_seqno;
-- 
1.7.7.5

[PATCH 11/14] drm/i915: split up ring->dispatch_execbuffer functions

[Daniel Vetter]

Now that we can, we should split them up in a way that makes some
sense and banishes the IS_ checks into init code.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   69 ++++++++++++++++++-------------
 1 files changed, 40 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 2cb1c8f..3d32c51 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -808,7 +808,7 @@ gen6_ring_put_irq(struct intel_ring_buffer *ring)
 }
 
 static int
-ring_dispatch_execbuffer(struct intel_ring_buffer *ring, u32 offset, u32 length)
+i965_dispatch_execbuffer(struct intel_ring_buffer *ring, u32 offset, u32 length)
 {
 	int ret;
 
@@ -826,37 +826,36 @@ ring_dispatch_execbuffer(struct intel_ring_buffer *ring, u32 offset, u32 length)
 }
 
 static int
-render_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
+i830_dispatch_execbuffer(struct intel_ring_buffer *ring,
 				u32 offset, u32 len)
 {
-	struct drm_device *dev = ring->dev;
 	int ret;
 
-	if (IS_I830(dev) || IS_845G(dev)) {
-		ret = intel_ring_begin(ring, 4);
-		if (ret)
-			return ret;
+	ret = intel_ring_begin(ring, 4);
+	if (ret)
+		return ret;
 
-		intel_ring_emit(ring, MI_BATCH_BUFFER);
-		intel_ring_emit(ring, offset | MI_BATCH_NON_SECURE);
-		intel_ring_emit(ring, offset + len - 8);
-		intel_ring_emit(ring, 0);
-	} else {
-		ret = intel_ring_begin(ring, 2);
-		if (ret)
-			return ret;
+	intel_ring_emit(ring, MI_BATCH_BUFFER);
+	intel_ring_emit(ring, offset | MI_BATCH_NON_SECURE);
+	intel_ring_emit(ring, offset + len - 8);
+	intel_ring_emit(ring, 0);
+	intel_ring_advance(ring);
 
-		if (INTEL_INFO(dev)->gen >= 4) {
-			intel_ring_emit(ring,
-					MI_BATCH_BUFFER_START | (2 << 6) |
-					MI_BATCH_NON_SECURE_I965);
-			intel_ring_emit(ring, offset);
-		} else {
-			intel_ring_emit(ring,
-					MI_BATCH_BUFFER_START | (2 << 6));
-			intel_ring_emit(ring, offset | MI_BATCH_NON_SECURE);
-		}
-	}
+	return 0;
+}
+
+static int
+i915_dispatch_execbuffer(struct intel_ring_buffer *ring,
+				u32 offset, u32 len)
+{
+	int ret;
+
+	ret = intel_ring_begin(ring, 2);
+	if (ret)
+		return ret;
+
+	intel_ring_emit(ring, MI_BATCH_BUFFER_START | (2 << 6));
+	intel_ring_emit(ring, offset | MI_BATCH_NON_SECURE);
 	intel_ring_advance(ring);
 
 	return 0;
@@ -1326,7 +1325,14 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 		ring->irq_enable_mask = I915_USER_INTERRUPT;
 	}
 	ring->write_tail = ring_write_tail;
-	ring->dispatch_execbuffer = render_ring_dispatch_execbuffer;
+	if (INTEL_INFO(dev)->gen >= 6)
+		ring->dispatch_execbuffer = gen6_ring_dispatch_execbuffer;
+	else if (INTEL_INFO(dev)->gen >= 4)
+		ring->dispatch_execbuffer = i965_dispatch_execbuffer;
+	else if (IS_I830(dev) || IS_845G(dev))
+		ring->dispatch_execbuffer = i830_dispatch_execbuffer;
+	else
+		ring->dispatch_execbuffer = i915_dispatch_execbuffer;
 	ring->init = init_render_ring;
 	ring->cleanup = render_ring_cleanup;
 
@@ -1367,7 +1373,12 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
 		ring->irq_enable_mask = I915_USER_INTERRUPT;
 	}
 	ring->write_tail = ring_write_tail;
-	ring->dispatch_execbuffer = render_ring_dispatch_execbuffer;
+	if (INTEL_INFO(dev)->gen >= 4)
+		ring->dispatch_execbuffer = i965_dispatch_execbuffer;
+	else if (IS_I830(dev) || IS_845G(dev))
+		ring->dispatch_execbuffer = i830_dispatch_execbuffer;
+	else
+		ring->dispatch_execbuffer = i915_dispatch_execbuffer;
 	ring->init = init_render_ring;
 	ring->cleanup = render_ring_cleanup;
 
@@ -1442,7 +1453,7 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
 			ring->irq_get = i9xx_ring_get_irq;
 			ring->irq_put = i9xx_ring_put_irq;
 		}
-		ring->dispatch_execbuffer = ring_dispatch_execbuffer;
+		ring->dispatch_execbuffer = i965_dispatch_execbuffer;
 	}
 	ring->init = init_ring_common;
 
-- 
1.7.7.5

[PATCH 12/14] drm/i915: consolidate ring->flush a bit

[Daniel Vetter]

They're indentical, so just kill one. Also give the other a prefix to
distinguish it from the gen6+ functions - this add_request function is
not really generic code.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   29 ++++-------------------------
 1 files changed, 4 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 3d32c51..111981a 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -559,27 +559,6 @@ pc_render_add_request(struct intel_ring_buffer *ring,
 	return 0;
 }
 
-static int
-render_ring_add_request(struct intel_ring_buffer *ring,
-			u32 *result)
-{
-	u32 seqno = i915_gem_next_request_seqno(ring);
-	int ret;
-
-	ret = intel_ring_begin(ring, 4);
-	if (ret)
-		return ret;
-
-	intel_ring_emit(ring, MI_STORE_DWORD_INDEX);
-	intel_ring_emit(ring, I915_GEM_HWS_INDEX << MI_STORE_DWORD_INDEX_SHIFT);
-	intel_ring_emit(ring, seqno);
-	intel_ring_emit(ring, MI_USER_INTERRUPT);
-	intel_ring_advance(ring);
-
-	*result = seqno;
-	return 0;
-}
-
 static u32
 gen6_ring_get_seqno(struct intel_ring_buffer *ring)
 {
@@ -745,7 +724,7 @@ bsd_ring_flush(struct intel_ring_buffer *ring,
 }
 
 static int
-ring_add_request(struct intel_ring_buffer *ring,
+i9xx_add_request(struct intel_ring_buffer *ring,
 		 u32 *result)
 {
 	u32 seqno;
@@ -1317,7 +1296,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 		ring->irq_put = gen5_ring_put_irq;
 		ring->irq_enable_mask = GT_USER_INTERRUPT | GT_PIPE_NOTIFY;
 	} else {
-		ring->add_request = render_ring_add_request;
+		ring->add_request = i9xx_add_request;
 		ring->flush = render_ring_flush;
 		ring->get_seqno = ring_get_seqno;
 		ring->irq_get = i9xx_ring_get_irq;
@@ -1365,7 +1344,7 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
 		ring->irq_put = gen5_ring_put_irq;
 		ring->irq_enable_mask = GT_USER_INTERRUPT | GT_PIPE_NOTIFY;
 	} else {
-		ring->add_request = render_ring_add_request;
+		ring->add_request = i9xx_add_request;
 		ring->flush = render_ring_flush;
 		ring->get_seqno = ring_get_seqno;
 		ring->irq_get = i9xx_ring_get_irq;
@@ -1442,7 +1421,7 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
 	} else {
 		ring->mmio_base = BSD_RING_BASE;
 		ring->flush = bsd_ring_flush;
-		ring->add_request = ring_add_request;
+		ring->add_request = i9xx_add_request;
 		ring->get_seqno = ring_get_seqno;
 		if (IS_GEN5(dev)) {
 			ring->irq_enable_mask = GT_BSD_USER_INTERRUPT;
-- 
1.7.7.5

[PATCH 13/14] drm/i915: don't set up gem ring functions on gen5 for !kms

[Daniel Vetter]

We already disallow initialition of gem in this case in the
corresponding ioctl, so don't bother setting up the gem support ring
functions in the legacy dri render ring init.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   24 ++++++++++--------------
 1 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 111981a..108bfd6 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1336,21 +1336,17 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
 	if (INTEL_INFO(dev)->gen >= 6) {
 		/* non-kms not supported on gen6+ */
 		return -ENODEV;
-	} else if (IS_GEN5(dev)) {
-		ring->add_request = pc_render_add_request;
-		ring->flush = render_ring_flush;
-		ring->get_seqno = pc_render_get_seqno;
-		ring->irq_get = gen5_ring_get_irq;
-		ring->irq_put = gen5_ring_put_irq;
-		ring->irq_enable_mask = GT_USER_INTERRUPT | GT_PIPE_NOTIFY;
-	} else {
-		ring->add_request = i9xx_add_request;
-		ring->flush = render_ring_flush;
-		ring->get_seqno = ring_get_seqno;
-		ring->irq_get = i9xx_ring_get_irq;
-		ring->irq_put = i9xx_ring_put_irq;
-		ring->irq_enable_mask = I915_USER_INTERRUPT;
 	}
+
+	/* Note: gem is not supported on gen5/ilk without kms (the corresponding
+	 * gem_init ioctl returns with -ENODEV). Hence we do not need to set up
+	 * the special gen5 functions. */
+	ring->add_request = i9xx_add_request;
+	ring->flush = render_ring_flush;
+	ring->get_seqno = ring_get_seqno;
+	ring->irq_get = i9xx_ring_get_irq;
+	ring->irq_put = i9xx_ring_put_irq;
+	ring->irq_enable_mask = I915_USER_INTERRUPT;
 	ring->write_tail = ring_write_tail;
 	if (INTEL_INFO(dev)->gen >= 4)
 		ring->dispatch_execbuffer = i965_dispatch_execbuffer;
-- 
1.7.7.5

[PATCH 14/14] drm/i915: inline enable/disable_irq into ring->get/put_irq

[Daniel Vetter]

Now that these are properly refactored this additional indirection
doesn't really buy us anything but confusion. Hence inline them.

This duplicates the ironlake gt enable/disable code snippet, but we've
already separate ilk from gen6+ gt irq in i915_irq.c, so I think this
makes more sense.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   68 ++++++++++++-------------------
 1 files changed, 26 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 108bfd6..2ffde00 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -585,38 +585,6 @@ pc_render_get_seqno(struct intel_ring_buffer *ring)
 	return pc->cpu_page[0];
 }
 
-static void
-ironlake_enable_irq(drm_i915_private_t *dev_priv, u32 mask)
-{
-	dev_priv->gt_irq_mask &= ~mask;
-	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
-	POSTING_READ(GTIMR);
-}
-
-static void
-ironlake_disable_irq(drm_i915_private_t *dev_priv, u32 mask)
-{
-	dev_priv->gt_irq_mask |= mask;
-	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
-	POSTING_READ(GTIMR);
-}
-
-static void
-i915_enable_irq(drm_i915_private_t *dev_priv, u32 mask)
-{
-	dev_priv->irq_mask &= ~mask;
-	I915_WRITE(IMR, dev_priv->irq_mask);
-	POSTING_READ(IMR);
-}
-
-static void
-i915_disable_irq(drm_i915_private_t *dev_priv, u32 mask)
-{
-	dev_priv->irq_mask |= mask;
-	I915_WRITE(IMR, dev_priv->irq_mask);
-	POSTING_READ(IMR);
-}
-
 static bool
 gen5_ring_get_irq(struct intel_ring_buffer *ring)
 {
@@ -627,8 +595,11 @@ gen5_ring_get_irq(struct intel_ring_buffer *ring)
 		return false;
 
 	spin_lock(&ring->irq_lock);
-	if (ring->irq_refcount++ == 0)
-		ironlake_enable_irq(dev_priv, ring->irq_enable_mask);
+	if (ring->irq_refcount++ == 0) {
+		dev_priv->gt_irq_mask &= ~ring->irq_enable_mask;
+		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
+		POSTING_READ(GTIMR);
+	}
 	spin_unlock(&ring->irq_lock);
 
 	return true;
@@ -641,8 +612,11 @@ gen5_ring_put_irq(struct intel_ring_buffer *ring)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
 	spin_lock(&ring->irq_lock);
-	if (--ring->irq_refcount == 0)
-		ironlake_disable_irq(dev_priv, ring->irq_enable_mask);
+	if (--ring->irq_refcount == 0) {
+		dev_priv->gt_irq_mask |= ring->irq_enable_mask;
+		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
+		POSTING_READ(GTIMR);
+	}
 	spin_unlock(&ring->irq_lock);
 }
 
@@ -656,8 +630,11 @@ i9xx_ring_get_irq(struct intel_ring_buffer *ring)
 		return false;
 
 	spin_lock(&ring->irq_lock);
-	if (ring->irq_refcount++ == 0)
-		i915_enable_irq(dev_priv, ring->irq_enable_mask);
+	if (ring->irq_refcount++ == 0) {
+		dev_priv->irq_mask &= ~ring->irq_enable_mask;
+		I915_WRITE(IMR, dev_priv->irq_mask);
+		POSTING_READ(IMR);
+	}
 	spin_unlock(&ring->irq_lock);
 
 	return true;
@@ -670,8 +647,11 @@ i9xx_ring_put_irq(struct intel_ring_buffer *ring)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
 	spin_lock(&ring->irq_lock);
-	if (--ring->irq_refcount == 0)
-		i915_disable_irq(dev_priv, ring->irq_enable_mask);
+	if (--ring->irq_refcount == 0) {
+		dev_priv->irq_mask |= ring->irq_enable_mask;
+		I915_WRITE(IMR, dev_priv->irq_mask);
+		POSTING_READ(IMR);
+	}
 	spin_unlock(&ring->irq_lock);
 }
 
@@ -763,7 +743,9 @@ gen6_ring_get_irq(struct intel_ring_buffer *ring)
 	spin_lock(&ring->irq_lock);
 	if (ring->irq_refcount++ == 0) {
 		I915_WRITE_IMR(ring, ~ring->irq_enable_mask);
-		ironlake_enable_irq(dev_priv, ring->irq_enable_mask);
+		dev_priv->gt_irq_mask &= ~ring->irq_enable_mask;
+		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
+		POSTING_READ(GTIMR);
 	}
 	spin_unlock(&ring->irq_lock);
 
@@ -779,7 +761,9 @@ gen6_ring_put_irq(struct intel_ring_buffer *ring)
 	spin_lock(&ring->irq_lock);
 	if (--ring->irq_refcount == 0) {
 		I915_WRITE_IMR(ring, ~0);
-		ironlake_disable_irq(dev_priv, ring->irq_enable_mask);
+		dev_priv->gt_irq_mask |= ring->irq_enable_mask;
+		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
+		POSTING_READ(GTIMR);
 	}
 	spin_unlock(&ring->irq_lock);
 
-- 
1.7.7.5

Re: [PATCH 00/14] intel_ringbuffer.c reorg + cleanups

[Chris Wilson]

On Wed, 11 Apr 2012 22:12:45 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Hi all,
> 
> This patch series is inspired by Ben's ring->get|put_irq cleanup for gen6+ and
> my perpetual hatred for intel_ringbuffer.c.
> 
> It's a lot of churn, but the end result is imho worth it - I almost started to
> like what the ringbuffer abstraction looks like now. There are some follow-up
> cleanups possible, but I think that can wait until we've cleanup up our domain
> tracking and ripped out the flushing_list (if that ever happens).
> 
> Commments, flames and review highly welcome.

It's not offensive. Cleaning up the function names and the
initialisation is certainly worth it. Maybe I was just expecting more.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 00/14] intel_ringbuffer.c reorg + cleanups

[Eric Anholt]

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

On Wed, 11 Apr 2012 22:12:45 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Hi all,
> 
> This patch series is inspired by Ben's ring->get|put_irq cleanup for gen6+ and
> my perpetual hatred for intel_ringbuffer.c.
> 
> It's a lot of churn, but the end result is imho worth it - I almost started to
> like what the ringbuffer abstraction looks like now. There are some follow-up
> cleanups possible, but I think that can wait until we've cleanup up our domain
> tracking and ripped out the flushing_list (if that ever happens).
> 
> Commments, flames and review highly welcome.

This is so nice.  It's way better than the series I started with when
working on domain tracking lobotomy.

Except for a s/bds/bsd/ in patch 4's commit message,

Reviewed-by: Eric Anholt <eric@anholt.net>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 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 02/14] drm/i915: set ring->size in common ring setup code

[Ben Widawsky]

On Wed, 11 Apr 2012 22:12:47 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Eventually we want to scale the ring size depending upon available
> gtt space. For now just consolidate this instead of replicating it
> over all ringbuffer templates.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---

Definitely in the bikeshed category:
Why not just make it a macro and scrap the member in struct
intel_ring_buffer?

Re: [PATCH 02/14] drm/i915: set ring->size in common ring setup code

[Daniel Vetter]

On Thu, Apr 12, 2012 at 12:23:47PM -0700, Ben Widawsky wrote:
> On Wed, 11 Apr 2012 22:12:47 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > Eventually we want to scale the ring size depending upon available
> > gtt space. For now just consolidate this instead of replicating it
> > over all ringbuffer templates.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> 
> Definitely in the bikeshed category:
> Why not just make it a macro and scrap the member in struct
> intel_ring_buffer?

830/845 need a workaround to reduce the effective ringbuffer size by 2
cachelines. And as alluded in the commit-message, we want to make this
slightly more dynamic. All this pipe_control workarounds eat away way too
much ringbuffer space :(
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 12/14] drm/i915: consolidate ring->flush a bit

[Ben Widawsky]

On Wed, 11 Apr 2012 22:12:57 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> They're indentical, so just kill one. Also give the other a prefix to
> distinguish it from the gen6+ functions - this add_request function is
> not really generic code.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

The subject appears to indicate that you're consolidating ring->flush,
but I think you meant add_request.

> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c |   29 ++++-------------------------
>  1 files changed, 4 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 3d32c51..111981a 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -559,27 +559,6 @@ pc_render_add_request(struct intel_ring_buffer *ring,
>  	return 0;
>  }
>  
> -static int
> -render_ring_add_request(struct intel_ring_buffer *ring,
> -			u32 *result)
> -{
> -	u32 seqno = i915_gem_next_request_seqno(ring);
> -	int ret;
> -
> -	ret = intel_ring_begin(ring, 4);
> -	if (ret)
> -		return ret;
> -
> -	intel_ring_emit(ring, MI_STORE_DWORD_INDEX);
> -	intel_ring_emit(ring, I915_GEM_HWS_INDEX << MI_STORE_DWORD_INDEX_SHIFT);
> -	intel_ring_emit(ring, seqno);
> -	intel_ring_emit(ring, MI_USER_INTERRUPT);
> -	intel_ring_advance(ring);
> -
> -	*result = seqno;
> -	return 0;
> -}
> -
>  static u32
>  gen6_ring_get_seqno(struct intel_ring_buffer *ring)
>  {
> @@ -745,7 +724,7 @@ bsd_ring_flush(struct intel_ring_buffer *ring,
>  }
>  
>  static int
> -ring_add_request(struct intel_ring_buffer *ring,
> +i9xx_add_request(struct intel_ring_buffer *ring,
>  		 u32 *result)
>  {
>  	u32 seqno;
> @@ -1317,7 +1296,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
>  		ring->irq_put = gen5_ring_put_irq;
>  		ring->irq_enable_mask = GT_USER_INTERRUPT | GT_PIPE_NOTIFY;
>  	} else {
> -		ring->add_request = render_ring_add_request;
> +		ring->add_request = i9xx_add_request;
>  		ring->flush = render_ring_flush;
>  		ring->get_seqno = ring_get_seqno;
>  		ring->irq_get = i9xx_ring_get_irq;
> @@ -1365,7 +1344,7 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
>  		ring->irq_put = gen5_ring_put_irq;
>  		ring->irq_enable_mask = GT_USER_INTERRUPT | GT_PIPE_NOTIFY;
>  	} else {
> -		ring->add_request = render_ring_add_request;
> +		ring->add_request = i9xx_add_request;
>  		ring->flush = render_ring_flush;
>  		ring->get_seqno = ring_get_seqno;
>  		ring->irq_get = i9xx_ring_get_irq;
> @@ -1442,7 +1421,7 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
>  	} else {
>  		ring->mmio_base = BSD_RING_BASE;
>  		ring->flush = bsd_ring_flush;
> -		ring->add_request = ring_add_request;
> +		ring->add_request = i9xx_add_request;
>  		ring->get_seqno = ring_get_seqno;
>  		if (IS_GEN5(dev)) {
>  			ring->irq_enable_mask = GT_BSD_USER_INTERRUPT;

Re: [PATCH 14/14] drm/i915: inline enable/disable_irq into ring->get/put_irq

[Ben Widawsky]

On Wed, 11 Apr 2012 22:12:59 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Now that these are properly refactored this additional indirection
> doesn't really buy us anything but confusion. Hence inline them.
> 
> This duplicates the ironlake gt enable/disable code snippet, but we've
> already separate ilk from gen6+ gt irq in i915_irq.c, so I think this
> makes more sense.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Bikeshed:
While doing all this, I think put/get irq is really terribly named. I
was a much bigger fan of the enable disable.

Also, you could use a bit of flow control to write to the correct IMR
register and not duplicate functions at all. You already do the
POSTING_READ so performance shouldn't matter.

Something like...

uint32_t imr = GEN(dev) >= 5 ? GTIMR: IMR;

Re: [PATCH 13/14] drm/i915: don't set up gem ring functions on gen5 for !kms

[Ben Widawsky]

On Wed, 11 Apr 2012 22:12:58 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> We already disallow initialition of gem in this case in the
> corresponding ioctl, so don't bother setting up the gem support ring
> functions in the legacy dri render ring init.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c |   24 ++++++++++--------------
>  1 files changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 111981a..108bfd6 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1336,21 +1336,17 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
>  	if (INTEL_INFO(dev)->gen >= 6) {
>  		/* non-kms not supported on gen6+ */
>  		return -ENODEV;
> -	} else if (IS_GEN5(dev)) {
> -		ring->add_request = pc_render_add_request;
> -		ring->flush = render_ring_flush;
> -		ring->get_seqno = pc_render_get_seqno;
> -		ring->irq_get = gen5_ring_get_irq;
> -		ring->irq_put = gen5_ring_put_irq;
> -		ring->irq_enable_mask = GT_USER_INTERRUPT | GT_PIPE_NOTIFY;
> -	} else {
> -		ring->add_request = i9xx_add_request;
> -		ring->flush = render_ring_flush;
> -		ring->get_seqno = ring_get_seqno;
> -		ring->irq_get = i9xx_ring_get_irq;
> -		ring->irq_put = i9xx_ring_put_irq;
> -		ring->irq_enable_mask = I915_USER_INTERRUPT;
>  	}
> +
> +	/* Note: gem is not supported on gen5/ilk without kms (the corresponding
> +	 * gem_init ioctl returns with -ENODEV). Hence we do not need to set up
> +	 * the special gen5 functions. */
Can we make the check >= 5 above? I think doing so would remove the need
for this comment.
> +	ring->add_request = i9xx_add_request;
> +	ring->flush = render_ring_flush;
> +	ring->get_seqno = ring_get_seqno;
> +	ring->irq_get = i9xx_ring_get_irq;
> +	ring->irq_put = i9xx_ring_put_irq;
> +	ring->irq_enable_mask = I915_USER_INTERRUPT;
>  	ring->write_tail = ring_write_tail;
>  	if (INTEL_INFO(dev)->gen >= 4)
>  		ring->dispatch_execbuffer = i965_dispatch_execbuffer;

Re: [PATCH 12/14] drm/i915: consolidate ring->flush a bit

[Daniel Vetter]

On Thu, Apr 12, 2012 at 05:54:45PM -0700, Ben Widawsky wrote:
> On Wed, 11 Apr 2012 22:12:57 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > They're indentical, so just kill one. Also give the other a prefix to
> > distinguish it from the gen6+ functions - this add_request function is
> > not really generic code.
> > 
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> The subject appears to indicate that you're consolidating ring->flush,
> but I think you meant add_request.

Oops, I'll fix this up when applying, thanks for catching this.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 13/14] drm/i915: don't set up gem ring functions on gen5 for !kms

[Daniel Vetter]

On Thu, Apr 12, 2012 at 06:05:14PM -0700, Ben Widawsky wrote:
> On Wed, 11 Apr 2012 22:12:58 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > We already disallow initialition of gem in this case in the
> > corresponding ioctl, so don't bother setting up the gem support ring
> > functions in the legacy dri render ring init.
> > 
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_ringbuffer.c |   24 ++++++++++--------------
> >  1 files changed, 10 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 111981a..108bfd6 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -1336,21 +1336,17 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
> >  	if (INTEL_INFO(dev)->gen >= 6) {
> >  		/* non-kms not supported on gen6+ */
> >  		return -ENODEV;
> > -	} else if (IS_GEN5(dev)) {
> > -		ring->add_request = pc_render_add_request;
> > -		ring->flush = render_ring_flush;
> > -		ring->get_seqno = pc_render_get_seqno;
> > -		ring->irq_get = gen5_ring_get_irq;
> > -		ring->irq_put = gen5_ring_put_irq;
> > -		ring->irq_enable_mask = GT_USER_INTERRUPT | GT_PIPE_NOTIFY;
> > -	} else {
> > -		ring->add_request = i9xx_add_request;
> > -		ring->flush = render_ring_flush;
> > -		ring->get_seqno = ring_get_seqno;
> > -		ring->irq_get = i9xx_ring_get_irq;
> > -		ring->irq_put = i9xx_ring_put_irq;
> > -		ring->irq_enable_mask = I915_USER_INTERRUPT;
> >  	}
> > +
> > +	/* Note: gem is not supported on gen5/ilk without kms (the corresponding
> > +	 * gem_init ioctl returns with -ENODEV). Hence we do not need to set up
> > +	 * the special gen5 functions. */
> Can we make the check >= 5 above? I think doing so would remove the need
> for this comment.

We can bail out for gen6+ because the driver will flat-out refuse to load,
so we can never actually hit this path. For gen5 we cannot refuse to load
and hence can't fail ring init (otoh it doesn't matter how broken the ring
is). Imo we have two options: Either try not to initialize any of the ring
stuff for gen5 and carefully auditing that this does not have any bad side
effects for vt-switching, suspend/resume where we might fall over a NULL
render ring. Or just initialize it half-assed and add this comment. I've
figured the latter is the less risky option and went with it.
-Daniel

> > +	ring->add_request = i9xx_add_request;
> > +	ring->flush = render_ring_flush;
> > +	ring->get_seqno = ring_get_seqno;
> > +	ring->irq_get = i9xx_ring_get_irq;
> > +	ring->irq_put = i9xx_ring_put_irq;
> > +	ring->irq_enable_mask = I915_USER_INTERRUPT;
> >  	ring->write_tail = ring_write_tail;
> >  	if (INTEL_INFO(dev)->gen >= 4)
> >  		ring->dispatch_execbuffer = i965_dispatch_execbuffer;
> 
> _______________________________________________
> 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 14/14] drm/i915: inline enable/disable_irq into ring->get/put_irq

[Daniel Vetter]

On Thu, Apr 12, 2012 at 06:03:30PM -0700, Ben Widawsky wrote:
> On Wed, 11 Apr 2012 22:12:59 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > Now that these are properly refactored this additional indirection
> > doesn't really buy us anything but confusion. Hence inline them.
> > 
> > This duplicates the ironlake gt enable/disable code snippet, but we've
> > already separate ilk from gen6+ gt irq in i915_irq.c, so I think this
> > makes more sense.
> > 
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Bikeshed:
> While doing all this, I think put/get irq is really terribly named. I
> was a much bigger fan of the enable disable.

Actually that can be combined with Chris' bikeshed to move the
ring->put|get_irg to dev_priv->core.ring_enable/disable_irq. But I've
figured that the same patch series should also move the forcewake function
pointers from dev_priv->display to dev_priv->core, so this is imo a
different patch series.

> Also, you could use a bit of flow control to write to the correct IMR
> register and not duplicate functions at all. You already do the
> POSTING_READ so performance shouldn't matter.
> 
> Something like...
> 
> uint32_t imr = GEN(dev) >= 5 ? GTIMR: IMR;

I've  figured I want to ditch all IS_GEN checks from these functions. But
for this case it makes some sense. Imo could be done with the follow-up
though.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 14/14] drm/i915: inline enable/disable_irq into ring->get/put_irq

[Chris Wilson]

On Fri, 13 Apr 2012 11:13:53 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Apr 12, 2012 at 06:03:30PM -0700, Ben Widawsky wrote:
> > On Wed, 11 Apr 2012 22:12:59 +0200
> > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > 
> > > Now that these are properly refactored this additional indirection
> > > doesn't really buy us anything but confusion. Hence inline them.
> > > 
> > > This duplicates the ironlake gt enable/disable code snippet, but we've
> > > already separate ilk from gen6+ gt irq in i915_irq.c, so I think this
> > > makes more sense.
> > > 
> > > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > Bikeshed:
> > While doing all this, I think put/get irq is really terribly named. I
> > was a much bigger fan of the enable disable.
> 
> Actually that can be combined with Chris' bikeshed to move the
> ring->put|get_irg to dev_priv->core.ring_enable/disable_irq. But I've
> figured that the same patch series should also move the forcewake function
> pointers from dev_priv->display to dev_priv->core, so this is imo a
> different patch series.

Not quite, they are get/put functions, as we do have multiple waiters
sharing the irq.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 00/14] intel_ringbuffer.c reorg + cleanups

[Daniel Vetter]

On Wed, Apr 11, 2012 at 04:42:13PM -0700, Eric Anholt wrote:
> On Wed, 11 Apr 2012 22:12:45 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Hi all,
> > 
> > This patch series is inspired by Ben's ring->get|put_irq cleanup for gen6+ and
> > my perpetual hatred for intel_ringbuffer.c.
> > 
> > It's a lot of churn, but the end result is imho worth it - I almost started to
> > like what the ringbuffer abstraction looks like now. There are some follow-up
> > cleanups possible, but I think that can wait until we've cleanup up our domain
> > tracking and ripped out the flushing_list (if that ever happens).
> > 
> > Commments, flames and review highly welcome.
> 
> This is so nice.  It's way better than the series I started with when
> working on domain tracking lobotomy.

tbh it's far from my first attempt at this, and I've dodged the domain
tracking things mostly by leaving the ring->flush code as-is.

> Except for a s/bds/bsd/ in patch 4's commit message,

Fixed, same for the s/ring->flush/ring->add_request/ noticed by Ben.

> Reviewed-by: Eric Anholt <eric@anholt.net>

Thanks for the review, all queued for -next. Imo Ben's concerns are valid,
but better tackled in follow up patches.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48