[PATCH] drm/i915: Dumb down the semaphore logic

[PATCH] drm/i915: Dumb down the semaphore logic

[Ben Widawsky]

While I think the previous code is correct, it was hard to follow and
hard to debug. Since we already have a ring abstraction, might as well
use it to handle the semaphore updates and compares.

I don't expect this code to make semaphores better or worse, but you
never know...

Cc: Andrew Lutomirski <luto@mit.edu>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    3 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c    |  164 +++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_ringbuffer.h    |    7 +-
 3 files changed, 119 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 4934cf8..3693e83 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -784,7 +784,8 @@ i915_gem_execbuffer_sync_rings(struct drm_i915_gem_object *obj,
 	}
 
 	from->sync_seqno[idx] = seqno;
-	return intel_ring_sync(to, from, seqno - 1);
+
+	return to->sync_to(to, from, seqno - 1);
 }
 
 static int
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index c30626e..c3d3906 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -313,81 +313,137 @@ static void render_ring_cleanup(struct intel_ring_buffer *ring)
 
 	cleanup_pipe_control(ring);
 }
+#define MBOX_UPDATE(ring, seqno) \
+		intel_ring_emit(ring, \
+				MI_SEMAPHORE_MBOX | \
+				MI_SEMAPHORE_GLOBAL_GTT | /* Should be ignored */ \
+				MI_SEMAPHORE_REGISTER | \
+				MI_SEMAPHORE_UPDATE); \
+		intel_ring_emit(ring, seqno)
 
-static void
-update_semaphore(struct intel_ring_buffer *ring, int i, u32 seqno)
-{
-	struct drm_device *dev = ring->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int id;
-
-	/*
-	 * cs -> 1 = vcs, 0 = bcs
-	 * vcs -> 1 = bcs, 0 = cs,
-	 * bcs -> 1 = cs, 0 = vcs.
-	 */
-	id = ring - dev_priv->ring;
-	id += 2 - i;
-	id %= 3;
-
-	intel_ring_emit(ring,
-			MI_SEMAPHORE_MBOX |
-			MI_SEMAPHORE_REGISTER |
-			MI_SEMAPHORE_UPDATE);
-	intel_ring_emit(ring, seqno);
-	intel_ring_emit(ring,
-			RING_SYNC_0(dev_priv->ring[id].mmio_base) + 4*i);
-}
-
-static int
-gen6_add_request(struct intel_ring_buffer *ring,
-		 u32 *result)
+static u32
+update_semaphore(struct intel_ring_buffer *ring,
+		 u32 first,
+		 u32 second)
 {
 	u32 seqno;
 	int ret;
+	seqno = i915_gem_get_seqno(ring->dev);
 
 	ret = intel_ring_begin(ring, 10);
 	if (ret)
 		return ret;
 
-	seqno = i915_gem_get_seqno(ring->dev);
-	update_semaphore(ring, 0, seqno);
-	update_semaphore(ring, 1, seqno);
-
+	MBOX_UPDATE(ring, seqno);
+	intel_ring_emit(ring, first);
+	MBOX_UPDATE(ring, seqno);
+	intel_ring_emit(ring, second);
 	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 seqno;
+}
+
+static int
+gen6_blt_add_request(struct intel_ring_buffer *ring,
+		 u32 *result)
+{
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	*result = update_semaphore(ring,
+				   dev_priv->ring[RCS].mmio_base + 0x44,
+				   dev_priv->ring[VCS].mmio_base + 0x40);
 	return 0;
 }
 
-int
-intel_ring_sync(struct intel_ring_buffer *ring,
-		struct intel_ring_buffer *to,
-		u32 seqno)
+static int
+gen6_bsd_add_request(struct intel_ring_buffer *ring,
+		 u32 *result)
+{
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	*result = update_semaphore(ring,
+				   dev_priv->ring[RCS].mmio_base + 0x40,
+				   dev_priv->ring[BCS].mmio_base + 0x44);
+	return 0;
+}
+
+static int
+gen6_render_add_request(struct intel_ring_buffer *ring,
+		 u32 *result)
+{
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	*result = update_semaphore(ring,
+				   dev_priv->ring[VCS].mmio_base + 0x44,
+				   dev_priv->ring[BCS].mmio_base + 0x40);
+	return 0;
+}
+
+static int
+intel_ring_sync(struct intel_ring_buffer *comparer,
+		struct intel_ring_buffer *updater,
+		u32 seqno,
+		u32 semaphore_register)
 {
 	int ret;
+	u32 temp = MI_SEMAPHORE_MBOX |
+		   MI_SEMAPHORE_GLOBAL_GTT | /* Not needed */
+		   MI_SEMAPHORE_COMPARE;
 
-	ret = intel_ring_begin(ring, 4);
+	ret = intel_ring_begin(comparer, 4);
 	if (ret)
 		return ret;
 
-	intel_ring_emit(ring,
-			MI_SEMAPHORE_MBOX |
-			MI_SEMAPHORE_REGISTER |
-			intel_ring_sync_index(ring, to) << 17 |
-			MI_SEMAPHORE_COMPARE);
-	intel_ring_emit(ring, seqno);
-	intel_ring_emit(ring, 0);
-	intel_ring_emit(ring, MI_NOOP);
-	intel_ring_advance(ring);
+	temp |= MI_SEMAPHORE_REGISTER;
+
+	intel_ring_emit(comparer, temp | semaphore_register);
+	intel_ring_emit(comparer, seqno);
+	intel_ring_emit(comparer, 0);
+	intel_ring_emit(comparer, MI_NOOP);
+	intel_ring_advance(comparer);
 
 	return 0;
 }
 
+/* VCS->RCS (RVSYNC) or BCS->RCS (RBSYNC) */
+int
+render_ring_sync_to(struct intel_ring_buffer *comparer,
+		struct intel_ring_buffer *updater,
+		u32 seqno)
+{
+	WARN_ON(updater->semaphore_register[RCS] == 1);
+	return intel_ring_sync(comparer, updater, seqno,
+			       updater->semaphore_register[RCS]);
+}
+
+/* RCS->VCS (VRSYNC) or BCS->VCS (VBSYNC) */
+int
+gen6_bsd_ring_sync_to(struct intel_ring_buffer *comparer,
+		struct intel_ring_buffer *updater,
+		u32 seqno)
+{
+	WARN_ON(updater->semaphore_register[VCS] == 1);
+	return intel_ring_sync(comparer, updater, seqno,
+			       updater->semaphore_register[VCS]);
+}
+
+/* RCS->BCS (BRSYNC) or VCS->BCS (BVSYNC) */
+int
+gen6_blt_ring_sync_to(struct intel_ring_buffer *comparer,
+		struct intel_ring_buffer *updater,
+		u32 seqno)
+{
+	WARN_ON(updater->semaphore_register[BCS] == 1);
+	return intel_ring_sync(comparer, updater, seqno,
+			       updater->semaphore_register[BCS]);
+}
+
+
+
 #define PIPE_CONTROL_FLUSH(ring__, addr__)					\
 do {									\
 	intel_ring_emit(ring__, GFX_OP_PIPE_CONTROL | PIPE_CONTROL_QW_WRITE |		\
@@ -1027,6 +1083,8 @@ static const struct intel_ring_buffer render_ring = {
 	.irq_put		= render_ring_put_irq,
 	.dispatch_execbuffer	= render_ring_dispatch_execbuffer,
        .cleanup			= render_ring_cleanup,
+	.sync_to		= render_ring_sync_to,
+	.semaphore_register	= {1, 2 << 16, 0 << 16}, /* invalid, RVSYNC, RBSYNC */
 };
 
 /* ring buffer for bit-stream decoder */
@@ -1149,11 +1207,13 @@ static const struct intel_ring_buffer gen6_bsd_ring = {
 	.init			= init_ring_common,
 	.write_tail		= gen6_bsd_ring_write_tail,
 	.flush			= gen6_ring_flush,
-	.add_request		= gen6_add_request,
+	.add_request		= gen6_bsd_add_request,
 	.get_seqno		= ring_get_seqno,
 	.irq_get		= gen6_bsd_ring_get_irq,
 	.irq_put		= gen6_bsd_ring_put_irq,
 	.dispatch_execbuffer	= gen6_ring_dispatch_execbuffer,
+	.sync_to		= gen6_bsd_ring_sync_to,
+	.semaphore_register	= {0 << 16, 1, 2 << 16}, /* VRSYNC, 0, VBSYNC */
 };
 
 /* Blitter support (SandyBridge+) */
@@ -1279,12 +1339,14 @@ static const struct intel_ring_buffer gen6_blt_ring = {
        .init			= blt_ring_init,
        .write_tail		= ring_write_tail,
        .flush			= blt_ring_flush,
-       .add_request		= gen6_add_request,
+       .add_request		= gen6_blt_add_request,
        .get_seqno		= ring_get_seqno,
        .irq_get			= blt_ring_get_irq,
        .irq_put			= blt_ring_put_irq,
        .dispatch_execbuffer	= gen6_ring_dispatch_execbuffer,
        .cleanup			= blt_ring_cleanup,
+	.sync_to		= gen6_blt_ring_sync_to,
+	.semaphore_register	= {2 << 16, 0 << 16, 1}, /* BRSYNC, BVSYNC, 0 */
 };
 
 int intel_init_render_ring_buffer(struct drm_device *dev)
@@ -1294,7 +1356,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 
 	*ring = render_ring;
 	if (INTEL_INFO(dev)->gen >= 6) {
-		ring->add_request = gen6_add_request;
+		ring->add_request = gen6_render_add_request;
 		ring->irq_get = gen6_render_ring_get_irq;
 		ring->irq_put = gen6_render_ring_put_irq;
 	} else if (IS_GEN5(dev)) {
@@ -1317,7 +1379,7 @@ 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->add_request = gen6_render_add_request;
 		ring->irq_get = gen6_render_ring_get_irq;
 		ring->irq_put = gen6_render_ring_put_irq;
 	} else if (IS_GEN5(dev)) {
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 39ac2b6..98052fd 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -75,7 +75,11 @@ struct  intel_ring_buffer {
 	int		(*dispatch_execbuffer)(struct intel_ring_buffer *ring,
 					       u32 offset, u32 length);
 	void		(*cleanup)(struct intel_ring_buffer *ring);
+	int		(*sync_to)(struct intel_ring_buffer *ring,
+				   struct intel_ring_buffer *to,
+				   u32 seqno);
 
+	u32		semaphore_register[3];
 	/**
 	 * List of objects currently involved in rendering from the
 	 * ringbuffer.
@@ -180,9 +184,6 @@ static inline void intel_ring_emit(struct intel_ring_buffer *ring,
 void intel_ring_advance(struct intel_ring_buffer *ring);
 
 u32 intel_ring_get_seqno(struct intel_ring_buffer *ring);
-int intel_ring_sync(struct intel_ring_buffer *ring,
-		    struct intel_ring_buffer *to,
-		    u32 seqno);
 
 int intel_init_render_ring_buffer(struct drm_device *dev);
 int intel_init_bsd_ring_buffer(struct drm_device *dev);
-- 
1.7.6.1

Re: [PATCH] drm/i915: Dumb down the semaphore logic

[Chris Wilson]

On Thu,  1 Sep 2011 20:55:35 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> While I think the previous code is correct, it was hard to follow and
> hard to debug. Since we already have a ring abstraction, might as well
> use it to handle the semaphore updates and compares.
> 
> I don't expect this code to make semaphores better or worse, but you
> never know...

So "dumbing it down" means using macros instead of small functions and
putting magic values into the ring structure. I'm not sure if that's an
improvement to readability at all.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Dumb down the semaphore logic

[Ben Widawsky]

On Fri, Sep 02, 2011 at 09:54:32AM +0100, Chris Wilson wrote:
> On Thu,  1 Sep 2011 20:55:35 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > While I think the previous code is correct, it was hard to follow and
> > hard to debug. Since we already have a ring abstraction, might as well
> > use it to handle the semaphore updates and compares.
> > 
> > I don't expect this code to make semaphores better or worse, but you
> > never know...
> 
> So "dumbing it down" means using macros instead of small functions and
> putting magic values into the ring structure. I'm not sure if that's an
> improvement to readability at all.
> -Chris

The previous code for doing an update was non obvious to me. Obviously I
will be biased in thinking my code is easier to follow, but for the
existing code I had to map out exactly what it's doing every single time
I went through it to convince myself it is correct.

Using pointer arithmetic and then RING_SYNC_0 with math did not seem the
correct way to do it. As for the magic values, they started out as
non-magic, but got changed at some point during developing the patch. At
the end I went back to magic to reduce the LOC.  You'll notice the
comments there make them slightly less magic.

And although a lame argument... if a new ring comes along the previous code was
not very well equiped to handle it.

Ben

Re: [PATCH] drm/i915: Dumb down the semaphore logic

[Eric Anholt]

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

On Thu,  1 Sep 2011 20:55:35 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> While I think the previous code is correct, it was hard to follow and
> hard to debug. Since we already have a ring abstraction, might as well
> use it to handle the semaphore updates and compares.
> 
> I don't expect this code to make semaphores better or worse, but you
> never know...

This code is generally more legible, and I think I could review it
compared to the specs in a few minutes instead of the awful I experience
I had reviewing what was there before (particularly the awful %
tricks).  Still, some review inline:

> Cc: Andrew Lutomirski <luto@mit.edu>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    3 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c    |  164 +++++++++++++++++++---------
>  drivers/gpu/drm/i915/intel_ringbuffer.h    |    7 +-
>  3 files changed, 119 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 4934cf8..3693e83 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -784,7 +784,8 @@ i915_gem_execbuffer_sync_rings(struct drm_i915_gem_object *obj,
>  	}
>  
>  	from->sync_seqno[idx] = seqno;
> -	return intel_ring_sync(to, from, seqno - 1);
> +
> +	return to->sync_to(to, from, seqno - 1);
>  }
>  
>  static int
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index c30626e..c3d3906 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -313,81 +313,137 @@ static void render_ring_cleanup(struct intel_ring_buffer *ring)
>  
>  	cleanup_pipe_control(ring);
>  }
> +#define MBOX_UPDATE(ring, seqno) \
> +		intel_ring_emit(ring, \
> +				MI_SEMAPHORE_MBOX | \
> +				MI_SEMAPHORE_GLOBAL_GTT | /* Should be ignored */ \
> +				MI_SEMAPHORE_REGISTER | \
> +				MI_SEMAPHORE_UPDATE); \
> +		intel_ring_emit(ring, seqno)

I do also find the macroing unnecessary, when there could have just been
a little helper function for "update this register with this seqno".

> -static void
> -update_semaphore(struct intel_ring_buffer *ring, int i, u32 seqno)
> -{
> -	struct drm_device *dev = ring->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int id;
> -
> -	/*
> -	 * cs -> 1 = vcs, 0 = bcs
> -	 * vcs -> 1 = bcs, 0 = cs,
> -	 * bcs -> 1 = cs, 0 = vcs.
> -	 */
> -	id = ring - dev_priv->ring;
> -	id += 2 - i;
> -	id %= 3;
> -
> -	intel_ring_emit(ring,
> -			MI_SEMAPHORE_MBOX |
> -			MI_SEMAPHORE_REGISTER |
> -			MI_SEMAPHORE_UPDATE);
> -	intel_ring_emit(ring, seqno);
> -	intel_ring_emit(ring,
> -			RING_SYNC_0(dev_priv->ring[id].mmio_base) + 4*i);
> -}
> -
> -static int
> -gen6_add_request(struct intel_ring_buffer *ring,
> -		 u32 *result)
> +static u32
> +update_semaphore(struct intel_ring_buffer *ring,
> +		 u32 first,
> +		 u32 second)
>  {
>  	u32 seqno;
>  	int ret;
> +	seqno = i915_gem_get_seqno(ring->dev);
>  
>  	ret = intel_ring_begin(ring, 10);
>  	if (ret)
>  		return ret;
>  
> -	seqno = i915_gem_get_seqno(ring->dev);
> -	update_semaphore(ring, 0, seqno);
> -	update_semaphore(ring, 1, seqno);
> -
> +	MBOX_UPDATE(ring, seqno);
> +	intel_ring_emit(ring, first);
> +	MBOX_UPDATE(ring, seqno);
> +	intel_ring_emit(ring, second);
>  	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 seqno;
> +}

So this function isn't "update_semaphore", it's "add_request" still --
notably, it's doing the interrupt emit.  Also, "first" and "second"
should probably have some naming reflecting that they're register
numbers.

> +static int
> +gen6_blt_add_request(struct intel_ring_buffer *ring,
> +		 u32 *result)
> +{
> +	struct drm_device *dev = ring->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	*result = update_semaphore(ring,
> +				   dev_priv->ring[RCS].mmio_base + 0x44,
> +				   dev_priv->ring[VCS].mmio_base + 0x40);
>  	return 0;
>  }
>  
> -int
> -intel_ring_sync(struct intel_ring_buffer *ring,
> -		struct intel_ring_buffer *to,
> -		u32 seqno)
> +static int
> +gen6_bsd_add_request(struct intel_ring_buffer *ring,
> +		 u32 *result)
> +{
> +	struct drm_device *dev = ring->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	*result = update_semaphore(ring,
> +				   dev_priv->ring[RCS].mmio_base + 0x40,
> +				   dev_priv->ring[BCS].mmio_base + 0x44);
> +	return 0;
> +}
> +
> +static int
> +gen6_render_add_request(struct intel_ring_buffer *ring,
> +		 u32 *result)
> +{
> +	struct drm_device *dev = ring->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	*result = update_semaphore(ring,
> +				   dev_priv->ring[VCS].mmio_base + 0x44,
> +				   dev_priv->ring[BCS].mmio_base + 0x40);
> +	return 0;
> +}
> +
> +static int
> +intel_ring_sync(struct intel_ring_buffer *comparer,
> +		struct intel_ring_buffer *updater,
> +		u32 seqno,
> +		u32 semaphore_register)
>  {
>  	int ret;
> +	u32 temp = MI_SEMAPHORE_MBOX |
> +		   MI_SEMAPHORE_GLOBAL_GTT | /* Not needed */
> +		   MI_SEMAPHORE_COMPARE;
>  
> -	ret = intel_ring_begin(ring, 4);
> +	ret = intel_ring_begin(comparer, 4);
>  	if (ret)
>  		return ret;
>  
> -	intel_ring_emit(ring,
> -			MI_SEMAPHORE_MBOX |
> -			MI_SEMAPHORE_REGISTER |
> -			intel_ring_sync_index(ring, to) << 17 |
> -			MI_SEMAPHORE_COMPARE);
> -	intel_ring_emit(ring, seqno);
> -	intel_ring_emit(ring, 0);
> -	intel_ring_emit(ring, MI_NOOP);
> -	intel_ring_advance(ring);
> +	temp |= MI_SEMAPHORE_REGISTER;
> +
> +	intel_ring_emit(comparer, temp | semaphore_register);
> +	intel_ring_emit(comparer, seqno);
> +	intel_ring_emit(comparer, 0);
> +	intel_ring_emit(comparer, MI_NOOP);
> +	intel_ring_advance(comparer);
>  
>  	return 0;
>  }
>  
> +/* VCS->RCS (RVSYNC) or BCS->RCS (RBSYNC) */
> +int
> +render_ring_sync_to(struct intel_ring_buffer *comparer,
> +		struct intel_ring_buffer *updater,
> +		u32 seqno)
> +{
> +	WARN_ON(updater->semaphore_register[RCS] == 1);
> +	return intel_ring_sync(comparer, updater, seqno,
> +			       updater->semaphore_register[RCS]);
> +}
> +
> +/* RCS->VCS (VRSYNC) or BCS->VCS (VBSYNC) */
> +int
> +gen6_bsd_ring_sync_to(struct intel_ring_buffer *comparer,
> +		struct intel_ring_buffer *updater,
> +		u32 seqno)
> +{
> +	WARN_ON(updater->semaphore_register[VCS] == 1);
> +	return intel_ring_sync(comparer, updater, seqno,
> +			       updater->semaphore_register[VCS]);
> +}
> +
> +/* RCS->BCS (BRSYNC) or VCS->BCS (BVSYNC) */
> +int
> +gen6_blt_ring_sync_to(struct intel_ring_buffer *comparer,
> +		struct intel_ring_buffer *updater,
> +		u32 seqno)
> +{
> +	WARN_ON(updater->semaphore_register[BCS] == 1);
> +	return intel_ring_sync(comparer, updater, seqno,
> +			       updater->semaphore_register[BCS]);
> +}
> +
> +
> +
>  #define PIPE_CONTROL_FLUSH(ring__, addr__)					\
>  do {									\
>  	intel_ring_emit(ring__, GFX_OP_PIPE_CONTROL | PIPE_CONTROL_QW_WRITE |		\
> @@ -1027,6 +1083,8 @@ static const struct intel_ring_buffer render_ring = {
>  	.irq_put		= render_ring_put_irq,
>  	.dispatch_execbuffer	= render_ring_dispatch_execbuffer,
>         .cleanup			= render_ring_cleanup,
> +	.sync_to		= render_ring_sync_to,
> +	.semaphore_register	= {1, 2 << 16, 0 << 16}, /* invalid, RVSYNC, RBSYNC */
>  };
>  
>  /* ring buffer for bit-stream decoder */
> @@ -1149,11 +1207,13 @@ static const struct intel_ring_buffer gen6_bsd_ring = {
>  	.init			= init_ring_common,
>  	.write_tail		= gen6_bsd_ring_write_tail,
>  	.flush			= gen6_ring_flush,
> -	.add_request		= gen6_add_request,
> +	.add_request		= gen6_bsd_add_request,
>  	.get_seqno		= ring_get_seqno,
>  	.irq_get		= gen6_bsd_ring_get_irq,
>  	.irq_put		= gen6_bsd_ring_put_irq,
>  	.dispatch_execbuffer	= gen6_ring_dispatch_execbuffer,
> +	.sync_to		= gen6_bsd_ring_sync_to,
> +	.semaphore_register	= {0 << 16, 1, 2 << 16}, /* VRSYNC, 0, VBSYNC */
>  };

above, you said "invalid" instead of "0".

>  
>  /* Blitter support (SandyBridge+) */
> @@ -1279,12 +1339,14 @@ static const struct intel_ring_buffer gen6_blt_ring = {
>         .init			= blt_ring_init,
>         .write_tail		= ring_write_tail,
>         .flush			= blt_ring_flush,
> -       .add_request		= gen6_add_request,
> +       .add_request		= gen6_blt_add_request,
>         .get_seqno		= ring_get_seqno,
>         .irq_get			= blt_ring_get_irq,
>         .irq_put			= blt_ring_put_irq,
>         .dispatch_execbuffer	= gen6_ring_dispatch_execbuffer,
>         .cleanup			= blt_ring_cleanup,
> +	.sync_to		= gen6_blt_ring_sync_to,
> +	.semaphore_register	= {2 << 16, 0 << 16, 1}, /* BRSYNC, BVSYNC, 0 */
>  };

[-- 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] drm/i915: Dumb down the semaphore logic

[Ben Widawsky]

On Fri, 02 Sep 2011 12:10:28 -0700
Eric Anholt <eric@anholt.net> wrote:

> On Thu,  1 Sep 2011 20:55:35 -0700, Ben Widawsky <ben@bwidawsk.net>
> wrote:
> > While I think the previous code is correct, it was hard to follow
> > and hard to debug. Since we already have a ring abstraction, might
> > as well use it to handle the semaphore updates and compares.
> > 
> > I don't expect this code to make semaphores better or worse, but you
> > never know...
> 
> This code is generally more legible, and I think I could review it
> compared to the specs in a few minutes instead of the awful I
> experience I had reviewing what was there before (particularly the
> awful % tricks).  Still, some review inline:

Keith, worth cleaning this one up?

Re: [PATCH] drm/i915: Dumb down the semaphore logic

[Keith Packard]

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

On Sat, 3 Sep 2011 13:09:47 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:

> Keith, worth cleaning this one up?

Yes, I think so. If nothing else, we'll have more people who actually
understand how the code is supposed to work, which should help with
future maintenance.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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] drm/i915: Dumb down the semaphore logic

[Ben Widawsky]

On Sun,  4 Sep 2011 20:52:42 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> While I think the previous code is correct, it was hard to follow and
> hard to debug. Since we already have a ring abstraction, might as well
> use it to handle the semaphore updates and compares.
> 
> I don't expect this code to make semaphores better or worse, but you
> never know...
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Eric Anholt <eric@anholt.net>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

I just realized I forgot to use the new invalid #define in the code.
I'll resubmit that tomorrow.

Ben

[PATCH] drm/i915: Dumb down the semaphore logic

[Ben Widawsky]

While I think the previous code is correct, it was hard to follow and
hard to debug. Since we already have a ring abstraction, might as well
use it to handle the semaphore updates and compares.

I don't expect this code to make semaphores better or worse, but you
never know...

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Eric Anholt <eric@anholt.net>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    3 +-
 drivers/gpu/drm/i915/i915_reg.h            |    7 +
 drivers/gpu/drm/i915/intel_ringbuffer.c    |  171 ++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_ringbuffer.h    |    7 +-
 4 files changed, 139 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 4934cf8..3693e83 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -784,7 +784,8 @@ i915_gem_execbuffer_sync_rings(struct drm_i915_gem_object *obj,
 	}
 
 	from->sync_seqno[idx] = seqno;
-	return intel_ring_sync(to, from, seqno - 1);
+
+	return to->sync_to(to, from, seqno - 1);
 }
 
 static int
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 542453f..f0b5287 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -194,6 +194,13 @@
 #define  MI_SEMAPHORE_UPDATE	    (1<<21)
 #define  MI_SEMAPHORE_COMPARE	    (1<<20)
 #define  MI_SEMAPHORE_REGISTER	    (1<<18)
+#define  MI_SEMAPHORE_SYNC_RV	    (2<<16)
+#define  MI_SEMAPHORE_SYNC_RB	    (0<<16)
+#define  MI_SEMAPHORE_SYNC_VR	    (0<<16)
+#define  MI_SEMAPHORE_SYNC_VB	    (2<<16)
+#define  MI_SEMAPHORE_SYNC_BR	    (2<<16)
+#define  MI_SEMAPHORE_SYNC_BV	    (0<<16)
+#define  MI_SEMAPHORE_SYNC_INVALID  (1<<0)
 /*
  * 3D instructions used by the kernel
  */
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index c30626e..ecb33bd 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -315,79 +315,148 @@ static void render_ring_cleanup(struct intel_ring_buffer *ring)
 }
 
 static void
-update_semaphore(struct intel_ring_buffer *ring, int i, u32 seqno)
+update_mboxes(struct intel_ring_buffer *ring,
+	    u32 seqno,
+	    u32 mmio_offset)
 {
-	struct drm_device *dev = ring->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int id;
-
-	/*
-	 * cs -> 1 = vcs, 0 = bcs
-	 * vcs -> 1 = bcs, 0 = cs,
-	 * bcs -> 1 = cs, 0 = vcs.
-	 */
-	id = ring - dev_priv->ring;
-	id += 2 - i;
-	id %= 3;
-
-	intel_ring_emit(ring,
-			MI_SEMAPHORE_MBOX |
-			MI_SEMAPHORE_REGISTER |
-			MI_SEMAPHORE_UPDATE);
+	intel_ring_emit(ring, MI_SEMAPHORE_MBOX |
+			      MI_SEMAPHORE_GLOBAL_GTT | /* Should be ignored */
+			      MI_SEMAPHORE_REGISTER |
+			      MI_SEMAPHORE_UPDATE);
 	intel_ring_emit(ring, seqno);
-	intel_ring_emit(ring,
-			RING_SYNC_0(dev_priv->ring[id].mmio_base) + 4*i);
+	intel_ring_emit(ring, mmio_offset);
 }
 
-static int
+/**
+ * gen6_add_request - Update the semaphore mailbox registers
+ * 
+ * @ring - ring that is adding a request
+ * @mbox1_reg - mailbox address for RCS or VCS ring
+ * @mbox2_reg - mailbox address for VCS or BCS ring
+ *
+ * Update the mailbox registers in the *other* rings with the current seqno.
+ */
+static u32
 gen6_add_request(struct intel_ring_buffer *ring,
-		 u32 *result)
+		 u32 mbox1_reg,
+		 u32 mbox2_reg)
 {
 	u32 seqno;
 	int ret;
+	seqno = i915_gem_get_seqno(ring->dev);
 
 	ret = intel_ring_begin(ring, 10);
 	if (ret)
 		return ret;
 
-	seqno = i915_gem_get_seqno(ring->dev);
-	update_semaphore(ring, 0, seqno);
-	update_semaphore(ring, 1, seqno);
-
+	update_mboxes(ring, seqno, mbox1_reg);
+	update_mboxes(ring, seqno, mbox2_reg);
 	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 seqno;
+}
+
+static int
+gen6_blt_add_request(struct intel_ring_buffer *ring,
+		 u32 *result)
+{
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	*result = gen6_add_request(ring,
+				   dev_priv->ring[RCS].mmio_base + 0x44,
+				   dev_priv->ring[VCS].mmio_base + 0x40);
 	return 0;
 }
 
-int
-intel_ring_sync(struct intel_ring_buffer *ring,
-		struct intel_ring_buffer *to,
-		u32 seqno)
+static int
+gen6_bsd_add_request(struct intel_ring_buffer *ring,
+		 u32 *result)
+{
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	*result = gen6_add_request(ring,
+				   dev_priv->ring[RCS].mmio_base + 0x40,
+				   dev_priv->ring[BCS].mmio_base + 0x44);
+	return 0;
+}
+
+static int
+gen6_render_add_request(struct intel_ring_buffer *ring,
+		 u32 *result)
+{
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	*result = gen6_add_request(ring,
+				   dev_priv->ring[VCS].mmio_base + 0x44,
+				   dev_priv->ring[BCS].mmio_base + 0x40);
+	return 0;
+}
+
+static int
+intel_ring_sync(struct intel_ring_buffer *comparer,
+		struct intel_ring_buffer *updater,
+		u32 seqno,
+		u32 semaphore_register)
 {
 	int ret;
+	u32 temp = MI_SEMAPHORE_MBOX |
+		   MI_SEMAPHORE_GLOBAL_GTT | /* Not needed */
+		   MI_SEMAPHORE_COMPARE;
 
-	ret = intel_ring_begin(ring, 4);
+	ret = intel_ring_begin(comparer, 4);
 	if (ret)
 		return ret;
 
-	intel_ring_emit(ring,
-			MI_SEMAPHORE_MBOX |
-			MI_SEMAPHORE_REGISTER |
-			intel_ring_sync_index(ring, to) << 17 |
-			MI_SEMAPHORE_COMPARE);
-	intel_ring_emit(ring, seqno);
-	intel_ring_emit(ring, 0);
-	intel_ring_emit(ring, MI_NOOP);
-	intel_ring_advance(ring);
+	temp |= MI_SEMAPHORE_REGISTER;
+
+	intel_ring_emit(comparer, temp | semaphore_register);
+	intel_ring_emit(comparer, seqno);
+	intel_ring_emit(comparer, 0);
+	intel_ring_emit(comparer, MI_NOOP);
+	intel_ring_advance(comparer);
 
 	return 0;
 }
 
+/* VCS->RCS (RVSYNC) or BCS->RCS (RBSYNC) */
+int
+render_ring_sync_to(struct intel_ring_buffer *comparer,
+		struct intel_ring_buffer *updater,
+		u32 seqno)
+{
+	WARN_ON(updater->semaphore_register[RCS] == 1);
+	return intel_ring_sync(comparer, updater, seqno,
+			       updater->semaphore_register[RCS]);
+}
+
+/* RCS->VCS (VRSYNC) or BCS->VCS (VBSYNC) */
+int
+gen6_bsd_ring_sync_to(struct intel_ring_buffer *comparer,
+		struct intel_ring_buffer *updater,
+		u32 seqno)
+{
+	WARN_ON(updater->semaphore_register[VCS] == 1);
+	return intel_ring_sync(comparer, updater, seqno,
+			       updater->semaphore_register[VCS]);
+}
+
+/* RCS->BCS (BRSYNC) or VCS->BCS (BVSYNC) */
+int
+gen6_blt_ring_sync_to(struct intel_ring_buffer *comparer,
+		struct intel_ring_buffer *updater,
+		u32 seqno)
+{
+	WARN_ON(updater->semaphore_register[BCS] == 1);
+	return intel_ring_sync(comparer, updater, seqno,
+			       updater->semaphore_register[BCS]);
+}
+
+
+
 #define PIPE_CONTROL_FLUSH(ring__, addr__)					\
 do {									\
 	intel_ring_emit(ring__, GFX_OP_PIPE_CONTROL | PIPE_CONTROL_QW_WRITE |		\
@@ -1027,6 +1096,10 @@ static const struct intel_ring_buffer render_ring = {
 	.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},
 };
 
 /* ring buffer for bit-stream decoder */
@@ -1149,11 +1222,15 @@ static const struct intel_ring_buffer gen6_bsd_ring = {
 	.init			= init_ring_common,
 	.write_tail		= gen6_bsd_ring_write_tail,
 	.flush			= gen6_ring_flush,
-	.add_request		= gen6_add_request,
+	.add_request		= gen6_bsd_add_request,
 	.get_seqno		= ring_get_seqno,
 	.irq_get		= gen6_bsd_ring_get_irq,
 	.irq_put		= gen6_bsd_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},
 };
 
 /* Blitter support (SandyBridge+) */
@@ -1279,12 +1356,16 @@ static const struct intel_ring_buffer gen6_blt_ring = {
        .init			= blt_ring_init,
        .write_tail		= ring_write_tail,
        .flush			= blt_ring_flush,
-       .add_request		= gen6_add_request,
+       .add_request		= gen6_blt_add_request,
        .get_seqno		= ring_get_seqno,
        .irq_get			= blt_ring_get_irq,
        .irq_put			= blt_ring_put_irq,
        .dispatch_execbuffer	= gen6_ring_dispatch_execbuffer,
        .cleanup			= blt_ring_cleanup,
+	.sync_to		= gen6_blt_ring_sync_to,
+	.semaphore_register	= {MI_SEMAPHORE_SYNC_BR,
+				   MI_SEMAPHORE_SYNC_BV,
+				   MI_SEMAPHORE_SYNC_INVALID},
 };
 
 int intel_init_render_ring_buffer(struct drm_device *dev)
@@ -1294,7 +1375,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 
 	*ring = render_ring;
 	if (INTEL_INFO(dev)->gen >= 6) {
-		ring->add_request = gen6_add_request;
+		ring->add_request = gen6_render_add_request;
 		ring->irq_get = gen6_render_ring_get_irq;
 		ring->irq_put = gen6_render_ring_put_irq;
 	} else if (IS_GEN5(dev)) {
@@ -1317,7 +1398,7 @@ 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->add_request = gen6_render_add_request;
 		ring->irq_get = gen6_render_ring_get_irq;
 		ring->irq_put = gen6_render_ring_put_irq;
 	} else if (IS_GEN5(dev)) {
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 39ac2b6..98052fd 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -75,7 +75,11 @@ struct  intel_ring_buffer {
 	int		(*dispatch_execbuffer)(struct intel_ring_buffer *ring,
 					       u32 offset, u32 length);
 	void		(*cleanup)(struct intel_ring_buffer *ring);
+	int		(*sync_to)(struct intel_ring_buffer *ring,
+				   struct intel_ring_buffer *to,
+				   u32 seqno);
 
+	u32		semaphore_register[3];
 	/**
 	 * List of objects currently involved in rendering from the
 	 * ringbuffer.
@@ -180,9 +184,6 @@ static inline void intel_ring_emit(struct intel_ring_buffer *ring,
 void intel_ring_advance(struct intel_ring_buffer *ring);
 
 u32 intel_ring_get_seqno(struct intel_ring_buffer *ring);
-int intel_ring_sync(struct intel_ring_buffer *ring,
-		    struct intel_ring_buffer *to,
-		    u32 seqno);
 
 int intel_init_render_ring_buffer(struct drm_device *dev);
 int intel_init_bsd_ring_buffer(struct drm_device *dev);
-- 
1.7.6.1