Re: [igt-dev] [RFC PATCH v6 2/4] lib: implement new engine discovery interface

[Antonio Argenziano <antonio.argenziano@intel.com>]

On 06/02/19 01:18, Andi Shyti wrote:
> Kernel commits:
> 
> [1] ae8f4544dd8f ("drm/i915: Engine discovery query")
> [2] 31e7d35667a0 ("drm/i915: Allow a context to define its set of engines")
> 
> implement a new uapi for engine discovery that consist in first
> querying the driver about the engines in the gpu [1] and then
> binding a context to the set of engines that it can access [2].
> 
> In igt the classic way for discovering engines is done through
> the for_each_physical_engine() macro, that would be replaced by
> the new for_each_engine_ctx().
> 
> A new function gem_init_engine_list() is addedthat is called

nit: missing white space ^^^^^^^^^^^^^^^^^^^^^

> in igt_require_gem() to set the i915 requirement. A list of
> existing engines is created and stored in the
> intel_execution_engines2 that replaces the current array which
> has more a reference meaning. Now the intel_execution_engines2
> stores the engines currently present in the GPU.
> 
> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
> ---
>   lib/igt_gt.c         | 110 ++++++++++++++++++++++++++++++++++++++++++-
>   lib/igt_gt.h         |  14 +++++-
>   lib/ioctl_wrappers.c |   3 ++
>   3 files changed, 124 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index 646696727ee4..7d933e64ec59 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -577,7 +577,7 @@ bool gem_can_store_dword(int fd, unsigned int engine)
>   	return true;
>   }
>   
> -const struct intel_execution_engine2 intel_execution_engines2[] = {
> +static struct intel_execution_engine2 intel_exec_engines2_reflist[] = {
>   	{ "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
>   	{ "bcs0", I915_ENGINE_CLASS_COPY, 0 },
>   	{ "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
> @@ -586,6 +586,8 @@ const struct intel_execution_engine2 intel_execution_engines2[] = {
>   	{ }
>   };
>   
> +struct intel_execution_engine2 *intel_execution_engines2;
> +
>   unsigned int
>   gem_class_instance_to_eb_flags(int gem_fd,
>   			       enum drm_i915_gem_engine_class class,
> @@ -650,3 +652,109 @@ bool gem_ring_has_physical_engine(int fd, unsigned ring)
>   
>   	return gem_has_ring(fd, ring);
>   }
> +
> +static struct drm_i915_query_engine_info *query_engines(int fd)
> +{
> +	struct drm_i915_query query = { };
> +	struct drm_i915_query_item item = { };
> +	struct drm_i915_query_engine_info *query_engines;
> +
> +	item.query_id = DRM_I915_QUERY_ENGINE_INFO;
> +	query.items_ptr = to_user_pointer(&item);
> +	query.num_items = 1;
> +	item.length = sizeof(*query_engines) +
> +		      64 * sizeof(struct drm_i915_engine_info);
> +
> +	igt_assert((query_engines = calloc(1, item.length)));
> +	item.data_ptr = to_user_pointer(query_engines);
> +
> +	igt_assert(!igt_ioctl(fd, DRM_IOCTL_I915_QUERY, &query));
> +
> +	return query_engines;
> +}
> +
> +bool gem_is_new_get_set_param(void)
> +{
> +	return intel_execution_engines2 != intel_exec_engines2_reflist;
> +}
> +
> +void __set_ctx_engine_map(int fd, uint32_t ctx_id)
> +{
> +	int i;
> +	struct intel_execution_engine2 *e2;
> +	struct drm_i915_gem_context_param ctx_param;
> +	struct i915_context_param_engines *ctx_engine;
> +
> +	if (!gem_is_new_get_set_param())
> +		return;
> +
> +	ctx_param.ctx_id = ctx_id;
> +	ctx_param.param = I915_CONTEXT_PARAM_ENGINES;
> +	ctx_param.size = sizeof(*ctx_engine) +
> +			 (I915_EXEC_RING_MASK - 1) *
> +			 sizeof(*ctx_engine->class_instance);
> +
> +	igt_assert((ctx_engine = calloc(1, ctx_param.size)));
> +
> +	ctx_engine->extensions = 0;
> +	for (i = 0, e2 = intel_execution_engines2; e2->name; i++, e2++) {
> +		ctx_engine->class_instance[i].engine_class = e2->class;
> +		ctx_engine->class_instance[i].engine_instance = e2->instance;
> +	}
> +
> +	ctx_param.value = to_user_pointer(ctx_engine);
> +
> +	igt_assert(!igt_ioctl(fd,
> +			      DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &ctx_param));

It would be good to have a wrapper for assigning the context to an 
engine instance and then another wrapper (or maybe a macro) for 
assigning the context to all engines, so not to hide the intent of the 
IOCLT underneath. I assume tests will want to have contexts assigned 
only to some engines and not others.

> +
> +	free(ctx_engine);
> +}
> +
> +void gem_init_engine_list(int fd)
> +{
> +	int i, ret;
> +	struct drm_i915_query_engine_info *query_engine = query_engines(fd);
> +	const char *engine_names[] = { "rcs", "bcs", "vcs", "vecs" };
> +	struct drm_i915_gem_context_param ctx_param = {
> +		.param = I915_CONTEXT_PARAM_ENGINES,
> +	};
> +
> +	if (intel_execution_engines2)
> +		return;
> +
> +	/*
> +	 * we check first whether the new engine discovery uapi
> +	 * is in the current kernel, if not, the
> +	 * DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM will fail with
> +	 * errno = EINVAL. In this case, we fall back to using
> +	 * the previous engine discovery way
> +	 */
> +	ret = igt_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM, &ctx_param);
> +	if (errno == EINVAL) {
> +		intel_execution_engines2 = intel_exec_engines2_reflist;
> +		return;
> +	}
> +
> +	igt_require_f(!ret, "failed DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM ioctl");
> +
> +	igt_assert((intel_execution_engines2 =
> +		    calloc(64, sizeof(*intel_execution_engines2))));

If you require !ret, I think you should require ^. Also, don't you want 
to request 'query_engine->num_engines' elements?

Antonio

> +
> +	for (i = 0; i < query_engine->num_engines; i++) {
> +		char *name;
> +		int class = query_engine->engines[i].class;
> +		int instance = query_engine->engines[i].instance;
> +
> +		igt_require(class < ARRAY_SIZE(engine_names) && class >= 0);
> +		igt_require(engine_names[class]);
> +
> +		intel_execution_engines2[i].class = class;
> +		intel_execution_engines2[i].instance = instance;
> +
> +		igt_assert(asprintf(&name, "%s%d",
> +				    engine_names[class], instance) > 0);
> +		intel_execution_engines2[i].name = name;
> +	}
> +
> +	free(query_engine);
> +}
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index 54e95da98084..fde67a6eb0ee 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -86,16 +86,26 @@ extern const struct intel_execution_engine {
>   	     e__++) \
>   		for_if (gem_ring_has_physical_engine(fd__, flags__ = e__->exec_id | e__->flags))
>   
> +#define for_each_engine_ctx(fd, ctx, e) \
> +		for (__set_ctx_engine_map(fd, ctx_id), \
> +		     e = intel_execution_engines2; e->name; e++) \
> +			for_if (gem_is_new_get_set_param() || \
> +				gem_has_engine(fd, e->class, e->instance))
> +
>   bool gem_ring_is_physical_engine(int fd, unsigned int ring);
>   bool gem_ring_has_physical_engine(int fd, unsigned int ring);
>   
>   bool gem_can_store_dword(int fd, unsigned int engine);
>   
> -extern const struct intel_execution_engine2 {
> +extern struct intel_execution_engine2 {
>   	const char *name;
>   	int class;
>   	int instance;
> -} intel_execution_engines2[];
> +} *intel_execution_engines2;
> +
> +bool gem_is_new_get_set_param(void);
> +void gem_init_engine_list(int fd);
> +void __set_ctx_engine_map(int fd, uint32_t ctx_id);
>   
>   unsigned int
>   gem_class_instance_to_eb_flags(int gem_fd,
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 404c2fbf9355..57ed8c51bea5 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -55,6 +55,7 @@
>   #include "igt_debugfs.h"
>   #include "igt_sysfs.h"
>   #include "config.h"
> +#include "igt_gt.h"
>   
>   #ifdef HAVE_VALGRIND
>   #include <valgrind/valgrind.h>
> @@ -1459,6 +1460,8 @@ void igt_require_gem(int fd)
>   	err = 0;
>   	if (ioctl(fd, DRM_IOCTL_I915_GEM_THROTTLE))
>   		err = -errno;
> +	else
> +		gem_init_engine_list(fd);
>   
>   	close(fd);
>   
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev