On Thu, 1 Sep 2011 20:55:35 -0700, Ben Widawsky <b...@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 <l...@mit.edu>
> Signed-off-by: Ben Widawsky <b...@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 */
> };
pgp8LlUaZ8gbW.pgp
Description: PGP signature
_______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
