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

[Tvrtko Ursulin]

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 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 ;)

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 at intel.com>

Regards,

Tvrtko