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

[Tvrtko Ursulin]

On 18/05/2017 13:24, Chris Wilson wrote:
> On Thu, May 18, 2017 at 01:13:20PM +0100, Tvrtko Ursulin wrote:
>>
>> On 18/05/2017 12:10, Chris Wilson wrote:
>>> On Thu, May 18, 2017 at 01:55:59PM +0300, Joonas Lahtinen wrote:
>>>> On ke, 2017-05-17 at 16:40 +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.
>>>
>>> Note this text doesn't describe the interface in v3.
>>>
>>>> Would it make sense to use bitmask for future proofing? Then we could
>>>> allow passing multiple options in the future.
>>>
>>> No. The first use case has to be explicit control of engine. That's
>>> orthogonal to asking to select any of a particular class.
>>
>> Was the suggestion to allow instance=any and instance=N? Or even all
>> the way to instance=N-or-M?
>>
>> If not the latter, then I can think of two other possible approaches:
>>
>> 1. Reserve 0xff as instance=any, aka 128 instances should be enough
>> for everbody. :) Could simply enforce in the uAPI that instance ==
>> I915_EXEC_INSTANCE_MASK = -EINVAL for now or something.
>>
>> 2. Do nothing now and add say I915_EXEC_CLASS at a later point. This
>> patch adds I915_EXEC_CLASS_INSTANCE so I915_EXEC_CLASS would not
>> sound out of place.
>
> Yes, I would argue to defer it until later. One problem I have at the
> moment is that not all members of a class are equal, HEVC-capable
> engines and the reset do not belong to the same class (i.e. my hope is
> that we could just say <class> | [ <mask> | INSTANCE_MASK ] | LOAD_BALANCE)
> So I can see the sense in having instance as a mask, or at least making
> the current instance field large enough to store a mask in future. I
> just feel uneasy as that field could grow quite large, and maybe it will
> be better to set the constraint via a context param (all dependency on
> frequency and tuning of the LOAD_BALANCE). Hmm, liking having the
> instance-mask on the context atm.

I don't think per context mask would work unless you won't to mandate 
multi-contexts where they wouldn't otherwise be needed.

But this problem in general can also be solved separately from 
class-instance addressing via engine feature masking.

As the GEM_ENGINE_INFO ioctl proposal defines a set of engine flags, at 
a later point execbuf could be extended with a new flag. For example:

eb.flags = I915_EXEC_CLASS | I915_ENGINE_CLASS_VIDEO | 
I915_EXEC_ENGINE_FEATURES | I915_ENGINE_HAS_HEVC;

Only problem I see that engine flags in the current proposal are u64 so 
it would be a bit challenging to fit that in eb.flags.

Not sure if it would make sense to split the engine info flags into a 
smaller and larger set. u8 which would be flags "compatible" with 
I915_EXEC_ENGINE_FEATURES, and the remainder which would be something 
else, or unused/reserved? Like:

struct drm_i915_engine_info {
	/** Engine instance number. */
	__u32	instance;
	__u32	rsvd;

	/** Engine specific features and info. */
#define DRM_I915_ENGINE_HAS_HEVC	BIT(0)
	__u8 features;
	__u8 rsvd;

	__32 info;
};

Or at that point you bring on eb3.

Regards,

Tvrtko