[Intel-gfx] [RFC v2 2/2] drm/i915: Select engines via class and instance in execbuffer2

[Tvrtko Ursulin]

On 27/04/2017 10:25, Chris Wilson wrote:
> On Thu, Apr 27, 2017 at 10:10:34AM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> Building on top of the previous patch which exported the concept
>> of engine classes and instances, we can also use this instead of
>> the current awkward engine selection uAPI.
>>
>> This is primarily interesting for the VCS engine selection which
>> is a) currently done via disjoint set of flags, and b) the
>> current I915_EXEC_BSD flags has different semantics depending on
>> the underlying hardware which is bad.
>>
>> Proposed idea here is to reserve 16-bits of flags, to pass in
>> the engine class and instance (8 bits each), and a new flag
>> named I915_EXEC_CLASS_INSTACE to tell the kernel this new engine
>> selection API is in use.
>>
>> The new uAPI also removes access to the weak VCS engine
>> balancing as currently existing in the driver.
>>
>> Example usage to send a command to VCS0:
>>
>>   eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 0);
>>
>> Or to send a command to VCS1:
>>
>>   eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 1);
>>
>> v2:
>>  * Fix unknown flags mask.
>>  * Use I915_EXEC_RING_MASK for class. (Chris Wilson)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Cc: Ben Widawsky <ben at bwidawsk.net>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Daniel Vetter <daniel.vetter at intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Cc: Jon Bloomfield <jon.bloomfield at intel.com>
>> Cc: Daniel Charles <daniel.charles at intel.com>
>> Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin at intel.com>
>> Cc: Oscar Mateo <oscar.mateo at intel.com>
>> Cc: "Gong, Zhipeng" <zhipeng.gong at intel.com>
>> Cc: intel-vaapi-media at lists.01.org
>> Cc: mesa-dev at lists.freedesktop.org
>> ---
>>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 29 +++++++++++++++++++++++++++++
>>  include/uapi/drm/i915_drm.h                | 11 ++++++++++-
>>  2 files changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> index af1965774e7b..ecd1486642a7 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> @@ -1492,6 +1492,32 @@ gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv,
>>  	return file_priv->bsd_engine;
>>  }
>>
>> +extern u8 user_class_map[DRM_I915_ENGINE_CLASS_MAX];
>> +
>> +static struct intel_engine_cs *
>> +eb_select_engine_class_instance(struct drm_i915_private *i915,
>> +				struct drm_i915_gem_execbuffer2 *args)
>> +{
>> +	struct intel_engine_cs *engine;
>> +	enum intel_engine_id id;
>> +	u8 class, instance;
>> +
>> +	class = args->flags & I915_EXEC_RING_MASK;
>> +	if (class >= DRM_I915_ENGINE_CLASS_MAX)
>> +		return NULL;
>> +	class = user_class_map[class];
>> +
>> +	instance = (args->flags >> I915_EXEC_INSTANCE_SHIFT) &&
>> +		   I915_EXEC_INSTANCE_MASK;
>> +
>> +	for_each_engine(engine, i915, id) {
>> +		if (engine->class == class && engine->instance == instance)
>> +			return engine;
>> +	}
>
> I am underwhelmed. No, i915->class_engine[class][instance] ?

Hey it's just an RFC for the uAPI proposal! Implementation efficiency 
only comes later! :)

> Still, at what point do we kill busy-ioctl per-engine reporting? Should

It's the one we already broke before without no one noticing, where it 
userspace only effectively cares about a boolean value?

If so you recommend we make it a real boolean?

> we update all tracepoints to use class:instance (I think that's a better
> abi going forward).

I can't think of any big problems doing so. Could rename ring= to 
engine= there as well. engine=<class>.<instance> for example?

Regards,

Tvrtko