Re: [PATCH 1/2] drm/i915: Dumb down the semaphore logic

From: Keith Packard <keithp@keithp.com>
To: intel-gfx@lists.freedesktop.org
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, Ben Widawsky <ben@bwidawsk.net>
Subject: Re: [PATCH 1/2] drm/i915: Dumb down the semaphore logic
Date: Wed, 07 Sep 2011 21:31:05 -0700	[thread overview]
Message-ID: <yund3fbd59y.fsf@aiko.keithp.com> (raw)
In-Reply-To: <1315437162-14312-1-git-send-email-ben@bwidawsk.net>

[-- Attachment #1.1: Type: text/plain, Size: 2321 bytes --]

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

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx