[Intel-gfx] [PATCH] drm/i915: Decouple execbuf uAPI from internal implementation

[Tvrtko Ursulin]

On 14/01/16 22:09, Chris Wilson wrote:
> On Thu, Jan 14, 2016 at 03:02:59PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> At the moment execbuf ring selection is fully coupled to
>> internal ring ids which is not a good thing on its own.
>>
>> This dependency is also spread between two source files and
>> not spelled out at either side which makes it hidden and
>> fragile.
>>
>> This patch decouples this dependency by introducing an explicit
>> translation table of execbuf uAPI to ring id close to the only
>> call site (i915_gem_do_execbuffer).
>>
>> This way we are free to change driver internal implementation
>> details without breaking userspace. All state relating to the
>> uAPI is now contained in, or next to, i915_gem_do_execbuffer.
>
> I was hesistant at first, since "why?" isn't fully developed, but the
> code won me over.
>
>> +#define I915_USER_RINGS (4)
>> +
>> +static const enum intel_ring_id user_ring_map[I915_USER_RINGS + 1] = {
>> +	[I915_EXEC_DEFAULT]	= RCS,
>> +	[I915_EXEC_RENDER]	= RCS,
>> +	[I915_EXEC_BLT]		= BCS,
>> +	[I915_EXEC_BSD]		= VCS,
>> +	[I915_EXEC_VEBOX]	= VECS
>> +};
>
> I was wondering whether packing here mattered at all, but we're under a
> cacheline so highly unlikely.
>
>>   static int
>>   i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>>   		       struct drm_file *file,
>> @@ -1393,6 +1393,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>>   	struct i915_execbuffer_params params_master; /* XXX: will be removed later */
>>   	struct i915_execbuffer_params *params = &params_master;
>>   	const u32 ctx_id = i915_execbuffer2_get_context_id(*args);
>> +	unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK;
>>   	u32 dispatch_flags;
>>   	int ret;
>>   	bool need_relocs;
>> @@ -1414,49 +1415,39 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>>   	if (args->flags & I915_EXEC_IS_PINNED)
>>   		dispatch_flags |= I915_DISPATCH_PINNED;
>>
>> -	if ((args->flags & I915_EXEC_RING_MASK) > LAST_USER_RING) {
>> -		DRM_DEBUG("execbuf with unknown ring: %d\n",
>> -			  (int)(args->flags & I915_EXEC_RING_MASK));
>> +	if (user_ring_id > I915_USER_RINGS) {
>> +		DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id);
>>   		return -EINVAL;
>>   	}
>>
>> -	if (((args->flags & I915_EXEC_RING_MASK) != I915_EXEC_BSD) &&
>> +	if ((user_ring_id != I915_EXEC_BSD) &&
>>   	    ((args->flags & I915_EXEC_BSD_MASK) != 0)) {
>>   		DRM_DEBUG("execbuf with non bsd ring but with invalid "
>>   			"bsd dispatch flags: %d\n", (int)(args->flags));
>>   		return -EINVAL;
>> -	}
>> -
>> -	if ((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_DEFAULT)
>> -		ring = &dev_priv->ring[RCS];
>> -	else if ((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_BSD) {
>> -		if (HAS_BSD2(dev)) {
>> -			int ring_id;
>> -
>> -			switch (args->flags & I915_EXEC_BSD_MASK) {
>> -			case I915_EXEC_BSD_DEFAULT:
>> -				ring_id = gen8_dispatch_bsd_ring(dev, file);
>> -				ring = &dev_priv->ring[ring_id];
>> -				break;
>> -			case I915_EXEC_BSD_RING1:
>> -				ring = &dev_priv->ring[VCS];
>> -				break;
>> -			case I915_EXEC_BSD_RING2:
>> -				ring = &dev_priv->ring[VCS2];
>> -				break;
>> -			default:
>> -				DRM_DEBUG("execbuf with unknown bsd ring: %d\n",
>> -					  (int)(args->flags & I915_EXEC_BSD_MASK));
>> -				return -EINVAL;
>> -			}
>> -		} else
>> -			ring = &dev_priv->ring[VCS];
>> -	} else
>> -		ring = &dev_priv->ring[(args->flags & I915_EXEC_RING_MASK) - 1];
>> +	}
>> +
>> +	if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev)) {
>
> HAS_BSD2(dev_priv)
>
>> +		unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK;
>> +
>> +		if (bsd_idx == I915_EXEC_BSD_DEFAULT) {
>> +			bsd_idx = gen8_dispatch_bsd_ring(dev_priv, file);
>> +		} else if (bsd_idx >= I915_EXEC_BSD_RING1 &&
>> +			   bsd_idx <= I915_EXEC_BSD_RING2) {
>> +			bsd_idx--;
>> +		} else {
>> +			DRM_DEBUG("execbuf with unknown bsd ring: %u\n",
>> +				  bsd_idx);
>> +			return -EINVAL;
>> +		}
>> +
>> +		ring = &dev_priv->ring[_VCS(bsd_idx)];
>> +	} else {
>> +		ring = &dev_priv->ring[user_ring_map[user_ring_id]];
>> +	}
>>
>>   	if (!intel_ring_initialized(ring)) {
>> -		DRM_DEBUG("execbuf with invalid ring: %d\n",
>> -			  (int)(args->flags & I915_EXEC_RING_MASK));
>> +		DRM_DEBUG("execbuf with invalid ring: %u\n", user_ring_id);
>>   		return -EINVAL;
>>   	}
>
> One question, how does this look if we move this section to its own
> function:
>
> ring = eb_select_ring(dev_priv, file, args);
> if (IS_ERR(ring))
> 	return PTR_ERR(ring);
>
> Do you keep your code size reduction?

No, perhaps because of all the ERR_PTR business. But decoupling the ring 
and return value keeps it:

static int
eb_select_ring(struct drm_i915_private *dev_priv,
	       struct drm_file *file,
	       struct drm_i915_gem_execbuffer2 *args,
	       struct intel_engine_cs **ring)

Saves ~100 bytes out of ~4600 on my build.

>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index 7349d9258191..fdc220fc2b18 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -148,14 +148,14 @@ struct  i915_ctx_workarounds {
>>   struct  intel_engine_cs {
>>   	const char	*name;
>>   	enum intel_ring_id {
>> -		RCS = 0x0,
>> -		VCS,
>> +		RCS = 0,
>>   		BCS,
>> -		VECS,
>> -		VCS2
>> +		VCS,
>> +		VCS2,	/* Keep instances of the same type engine together. */
>> +		VECS
>
> Technically, this breaks userspace ABI elsewhere. :| Though the only
> user of the id mask is only looking for RCS==0 vs the reset.
>
> So I think we would cope, but to be extra safe we could just avoid
> reshuffling the ids. Let me sleep on the implications. We may say just
> break that ABI whilst we still can and do a reverse-map to EXEC bit.
> Hmm.

What are the other places it breaks the ABI? I'd like to understand it.

Regards,

Tvrtko