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

[Tvrtko Ursulin]

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.

> +	{ "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?

> +
>   extern const struct intel_execution_engine {
>   	const char *name;
>   	const char *full_name;
>   	unsigned exec_id;
> +	unsigned class_id;

And then use the enum here as well.

>   	unsigned flags;
>   } intel_execution_engines[];
>   
> 

Regards,

Tvrtko