[Intel-gfx] [PATCH v5 2/9] drm/i915: Define an engine class enum for the uABI

[Chris Wilson]

Quoting Lionel Landwerlin (2017-11-09 22:16:26)
> On 09/11/17 21:29, Chris Wilson wrote:
> > Quoting Lionel Landwerlin (2017-11-09 09:13:03)
> >> On 08/11/17 19:14, Chris Wilson wrote:
> >>>    
> >>> +/*
> >>> + * Different engines serve different roles, and there may be more than one
> >>> + * engine serving each role. enum drm_i915_gem_engine_class provides a
> >>> + * classification of the role of the engine, which may be used when requesting
> >>> + * operations to be performed on a certain subset of engines, or for providing
> >>> + * information about that group.
> >>> + */
> >>> +enum drm_i915_gem_engine_class {
> >>> +     I915_ENGINE_CLASS_OTHER = 0,
> >>> +     I915_ENGINE_CLASS_RENDER = 1,
> >>> +     I915_ENGINE_CLASS_COPY = 2,
> >>> +     I915_ENGINE_CLASS_VIDEO = 3,
> >>> +     I915_ENGINE_CLASS_VIDEO_ENHANCE = 4,
> >>> +};
> >>> +
> >> I've tried to build a bit UI in GPUTop to show this.
> >> I'm a bit skeptical about the OTHER type because if this enum is meant
> >> to be extended, then why do we need an OTHER class? We should create new
> >> classes instead.
> > Whilst OTHER is certainly a question of "why not add a define for the
> > new class", my 2c is that we do want an explicit INVALID enum. It
> > doesn't have to 0, -1 will work just fine as well, but there will be a
> > time either in userspace or in reporting from the kernel where we will
> > need to store an invalid value that is guaranteed not to map onto a real
> > class.
> >
> > So I915_ENGINE_CLASS_INVALID = -1 ?
> > -Chris
> >
> Okay, so you want to use that value for userspace to say "execute this 
> batch buffer on whatever engine".

No, I don't expect it to ever be a part of a request from userspace.
(Though who knows!)

> And in that case that's fine with me, I didn't get that would be used in 
> the userspace->kernel direction.
> 
> But I'm still struggling to understand why the kernel would report that 
> there is an "invalid" class of engine on this device, rather than 
> something more descriptive.

There's the discussion of exactly what class gen2-5 CS comprises; it has
dual functionality of both CLASS_COPY and CLASS_RENDER. (And the
unspoken CLASS_GPGPU.)

The usecase for invalid is in userspace in being able to say that its
struct engine doesn't map to any exact HW class. And at some point, in
some interface the same will be required from the kernel. What I am
saying is that history says that having an explicit invalid value from
the beginning is useful for futureproofing the API, as well as being
convenient for writing generic code to map between the different
interfaces.
-Chris