On Wed,  7 Sep 2011 16:12:41 -0700, Ben Widawsky <ben@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@intel.com