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@xxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
