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(a)intel.com&gt; >>>> >>>> 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.