Re: [PATCH 04/13] drm/i915: Make semaphore updates more precise

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jan 29, 2014 at 11:55:24AM -0800, Ben Widawsky wrote:
> With the ring mask we now have an easy way to know the number of rings
> in the system, and therefore can accurately predict the number of dwords
> to emit for semaphore signalling. This was not possible (easily)
> previously.
> 
> There should be no functional impact, simply fewer instructions emitted.
> 
> While we're here, simply do the round up to 2 instead of the fancier
> rounding we did before, which rounding up per mbox, ie 4.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 43 +++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 70f7190..97789ff 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -635,24 +635,20 @@ static void render_ring_cleanup(struct intel_ring_buffer *ring)
>  static int gen6_signal(struct intel_ring_buffer *signaller,
>  		       unsigned int num_dwords)
>  {
> +#define MBOX_UPDATE_DWORDS 4
>  	struct drm_device *dev = signaller->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_ring_buffer *useless;
> -	int i, ret;
> +	int i, ret, num_rings;
>  
> -	/* NB: In order to be able to do semaphore MBOX updates for varying
> -	 * number of rings, it's easiest if we round up each individual update
> -	 * to a multiple of 2 (since ring updates must always be a multiple of
> -	 * 2) even though the actual update only requires 3 dwords.
> -	 */
> -#define MBOX_UPDATE_DWORDS 4
> -	if (i915_semaphore_is_enabled(dev))
> -		num_dwords += ((I915_NUM_RINGS-1) * MBOX_UPDATE_DWORDS);
> +	num_rings = hweight_long(INTEL_INFO(dev)->ring_mask);
> +	num_dwords = round_up((num_rings-1) * MBOX_UPDATE_DWORDS, 2);

num_dwords +=

Also round_up() is useless since it's already a multiple of 4. Or did
you mean to change it to emit only 3 dwords per mbox?

> +#undef MBOX_UPDATE_DWORDS
>  
> -	ret = intel_ring_begin(signaller, num_dwords);
> +	/* XXX: + 4 for the caller */
> +	ret = intel_ring_begin(signaller, num_dwords + 4);

The += earlier gets rid of the +4 here.

>  	if (ret)
>  		return ret;
> -#undef MBOX_UPDATE_DWORDS
>  
>  	for_each_ring(useless, dev_priv, i) {
>  		u32 mbox_reg = signaller->semaphore.signal_mbox[i];
> @@ -661,14 +657,11 @@ static int gen6_signal(struct intel_ring_buffer *signaller,
>  			intel_ring_emit(signaller, mbox_reg);
>  			intel_ring_emit(signaller, signaller->outstanding_lazy_seqno);
>  			intel_ring_emit(signaller, MI_NOOP);
> -		} else {
> -			intel_ring_emit(signaller, MI_NOOP);
> -			intel_ring_emit(signaller, MI_NOOP);
> -			intel_ring_emit(signaller, MI_NOOP);
> -			intel_ring_emit(signaller, MI_NOOP);
>  		}
>  	}
>  
> +	WARN_ON(i != num_rings);

So we're not expecting dev_priv->ring[] to be sparsely populated ever?

> +
>  	return 0;
>  }
>  
> @@ -686,7 +679,11 @@ gen6_add_request(struct intel_ring_buffer *ring)
>  {
>  	int ret;
>  
> -	ret = ring->semaphore.signal(ring, 4);
> +	if (ring->semaphore.signal)
> +		ret = ring->semaphore.signal(ring, 4);
> +	else
> +		ret = intel_ring_begin(ring, 4);
> +
>  	if (ret)
>  		return ret;
>  
> @@ -1881,7 +1878,8 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
>  		ring->get_seqno = gen6_ring_get_seqno;
>  		ring->set_seqno = ring_set_seqno;
>  		ring->semaphore.sync_to = gen6_ring_sync;
> -		ring->semaphore.signal = gen6_signal;
> +		if (i915_semaphore_is_enabled(dev))
> +			ring->semaphore.signal = gen6_signal;

I guess we could also set .sync_to conditionally, but doesn't really
matter since we won't call it anyway w/o semaphores enabled.

>  		ring->semaphore.mbox[RCS] = MI_SEMAPHORE_SYNC_INVALID;
>  		ring->semaphore.mbox[VCS] = MI_SEMAPHORE_SYNC_RV;
>  		ring->semaphore.mbox[BCS] = MI_SEMAPHORE_SYNC_RB;
> @@ -2058,7 +2056,8 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
>  				gen6_ring_dispatch_execbuffer;
>  		}
>  		ring->semaphore.sync_to = gen6_ring_sync;
> -		ring->semaphore.signal = gen6_signal;
> +		if (i915_semaphore_is_enabled(dev))
> +			ring->semaphore.signal = gen6_signal;
>  		ring->semaphore.mbox[RCS] = MI_SEMAPHORE_SYNC_VR;
>  		ring->semaphore.mbox[VCS] = MI_SEMAPHORE_SYNC_INVALID;
>  		ring->semaphore.mbox[BCS] = MI_SEMAPHORE_SYNC_VB;
> @@ -2116,7 +2115,8 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
>  		ring->dispatch_execbuffer = gen6_ring_dispatch_execbuffer;
>  	}
>  	ring->semaphore.sync_to = gen6_ring_sync;
> -	ring->semaphore.signal = gen6_signal;
> +	if (i915_semaphore_is_enabled(dev))
> +		ring->semaphore.signal = gen6_signal;
>  	ring->semaphore.mbox[RCS] = MI_SEMAPHORE_SYNC_BR;
>  	ring->semaphore.mbox[VCS] = MI_SEMAPHORE_SYNC_BV;
>  	ring->semaphore.mbox[BCS] = MI_SEMAPHORE_SYNC_INVALID;
> @@ -2158,7 +2158,8 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
>  		ring->dispatch_execbuffer = gen6_ring_dispatch_execbuffer;
>  	}
>  	ring->semaphore.sync_to = gen6_ring_sync;
> -	ring->semaphore.signal = gen6_signal;
> +	if (i915_semaphore_is_enabled(dev))
> +		ring->semaphore.signal = gen6_signal;
>  	ring->semaphore.mbox[RCS] = MI_SEMAPHORE_SYNC_VER;
>  	ring->semaphore.mbox[VCS] = MI_SEMAPHORE_SYNC_VEV;
>  	ring->semaphore.mbox[BCS] = MI_SEMAPHORE_SYNC_VEB;
> -- 
> 1.8.5.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC