On 15/11/2017 13:32, Chris Wilson wrote:
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@xxxxxxxxxxxxxxxxxx> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> --- 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 3Why 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 ;)
enum would have to be temporary and these ones can be left in "forever" you mean? I actually have no idea at the moment on how the enum namespace is handled in C. But anyway, none of this matters hugely.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
