[Intel-gfx] [PATCH 1/2] drm/i915: Dumb down the semaphore logic
Keith Packard
keithp at keithp.com
Thu Sep 8 06:31:05 CEST 2011
On Wed, 7 Sep 2011 16:12:41 -0700, Ben Widawsky <ben at bwidawsk.net> wrote:
> -update_semaphore(struct intel_ring_buffer *ring, int i, u32 seqno)
> +update_mboxes(struct intel_ring_buffer *ring,
> + u32 seqno,
> + u32 mmio_offset)
Yeah, definitely like this change; lots less magic here.
> -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.
> + * This acts like a signal in the canonical semaphore.
> + */
> +static u32
> gen6_add_request(struct intel_ring_buffer *ring,
> - u32 *result)
> + u32 mbox1_reg,
> + u32 mbox2_reg)
I think you're losing the ability to return errors from here.
> 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);
Why change the ordering of get_seqno relative to ring_begin here?
> +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;
Why the magic constants? Can we have named values? And, note that this
function never returns an error value, which is definitely not a good plan.
> + temp |= MI_SEMAPHORE_REGISTER;
temp is a constant, why is it being |='d here?
> +/* VCS->RCS (RVSYNC) or BCS->RCS (RBSYNC) */
> +int
> +render_ring_sync_to(struct intel_ring_buffer *waiter,
> + struct intel_ring_buffer *signaller,
> + u32 seqno)
> +{
> + WARN_ON(signaller->semaphore_register[RCS] == MI_SEMAPHORE_SYNC_INVALID);
> + return intel_ring_sync(waiter,
> + signaller,
> + signaller->semaphore_register[RCS],
Should you just pass the index instead of the register value itself?
Otherwise, this seems like a reasonable change to me.
--
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20110907/84f606ca/attachment.sig>
More information about the Intel-gfx
mailing list