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

Eric Anholt eric at anholt.net
Fri Sep 2 21:10:28 CEST 2011


On Thu,  1 Sep 2011 20:55:35 -0700, Ben Widawsky <ben at 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 at mit.edu>
> Signed-off-by: Ben Widawsky <ben at 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 */
>  };
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20110902/acbab2cd/attachment.sig>


More information about the Intel-gfx mailing list