[Intel-gfx] [PATCH igt 1/2] lib/gt: Mark up engine classes

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-11-15 13:26:09)
> 
> On 14/11/2017 13:19, Chris Wilson wrote:
> > We introduce the concept of classes that each engine may belong to.
> > There may be more than one engine available in each class, but each
> > engine only belongs to one class. Each class has a unique set of
> > capabilities and purpose (e.g. 3D rendering or video decode), but
> > different engines within each class may have different features (e.g.
> > only the first decoder instance may have the full set of fixed function
> > blocks, but most of the work can still be offload onto the other decoders
> > which shared the same ISA etc).
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >   lib/igt_gt.c | 16 ++++++++--------
> >   lib/igt_gt.h |  7 +++++++
> >   2 files changed, 15 insertions(+), 8 deletions(-)
> > 
> > diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> > index 89727d22..8496fe4d 100644
> > --- a/lib/igt_gt.c
> > +++ b/lib/igt_gt.c
> > @@ -578,14 +578,14 @@ unsigned intel_detect_and_clear_missed_interrupts(int fd)
> >   }
> >   
> >   const struct intel_execution_engine intel_execution_engines[] = {
> > -     { "default", NULL, 0, 0 },
> > -     { "render", "rcs0", I915_EXEC_RENDER, 0 },
> > -     { "bsd", "vcs0", I915_EXEC_BSD, 0 },
> > -     { "bsd1", "vcs0", I915_EXEC_BSD, 1<<13 /*I915_EXEC_BSD_RING1*/ },
> > -     { "bsd2", "vcs1", I915_EXEC_BSD, 2<<13 /*I915_EXEC_BSD_RING2*/ },
> > -     { "blt", "bcs0", I915_EXEC_BLT, 0 },
> > -     { "vebox", "vecs0", I915_EXEC_VEBOX, 0 },
> > -     { NULL, 0, 0 }
> > +     { "default", NULL, 0, LOCAL_ENGINE_CLASS_INVALID, 0 },
> 
> A bit strange that you have marked it with invalid here. Will cause 
> random issues for tests which will use class_id from this array. On the 
> other hand maybe it is for the better since it will remove the "default" 
> engine from the subtests ran once we (hopefully) switch to 
> class-instance execbuf. Don't know, hard to guess, but it works so OK.

Default doesn't map onto a class-instance, it is vague and may map on to
whatever the kernel considers most convenient. And yes, when we are
iterating over HW engines and not I915_EXEC_$RING, we don't care for the
ambiguously defined uabi.

> > +     { "render", "rcs0", I915_EXEC_RENDER, LOCAL_ENGINE_CLASS_RENDER, 0 },
> > +     { "bsd", "vcs0", I915_EXEC_BSD, LOCAL_ENGINE_CLASS_VIDEO, 0 },
> > +     { "bsd1", "vcs0", I915_EXEC_BSD, LOCAL_ENGINE_CLASS_VIDEO, 1<<13 /*I915_EXEC_BSD_RING1*/ },
> > +     { "bsd2", "vcs1", I915_EXEC_BSD, LOCAL_ENGINE_CLASS_VIDEO, 2<<13 /*I915_EXEC_BSD_RING2*/ },
> > +     { "blt", "bcs0", I915_EXEC_BLT, LOCAL_ENGINE_CLASS_COPY, 0 },
> > +     { "vebox", "vecs0", I915_EXEC_VEBOX, LOCAL_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
> > +     { NULL }
> >   };
> >   
> >   bool gem_can_store_dword(int fd, unsigned int engine)
> > diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> > index 2579cbd3..4f13f86f 100644
> > --- a/lib/igt_gt.h
> > +++ b/lib/igt_gt.h
> > @@ -63,10 +63,17 @@ void igt_clflush_range(void *addr, int size);
> >   
> >   unsigned intel_detect_and_clear_missed_interrupts(int fd);
> >   
> > +#define LOCAL_ENGINE_CLASS_INVALID           -1
> > +#define LOCAL_ENGINE_CLASS_RENDER            0
> > +#define LOCAL_ENGINE_CLASS_COPY                      1
> > +#define LOCAL_ENGINE_CLASS_VIDEO             2
> > +#define LOCAL_ENGINE_CLASS_VIDEO_ENHANCE     3
> 
> Why not make a local copy of the enum to be consistent with the kernel 
> uPAI headers?

Shrug. It has to be temporary. #define have the advantage that the
compiler only complains when they differ, which was why I suggesting we
didn't use enum ;)
-Chris