[RFC,v6,2/4] lib: implement new engine discovery interface

Submitted by Andi Shyti on Feb. 6, 2019, 9:18 a.m.

Details

Message ID 20190206091850.2078-3-andi.shyti@intel.com
State New
Headers show
Series "new engine discovery interface" ( rev: 7 ) in IGT

Not browsing as part of any series.

Commit Message

Andi Shyti Feb. 6, 2019, 9:18 a.m.
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
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(-)

Patch hide | download patch | download mbox

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

Comments

Quoting Andi Shyti (2019-02-06 09:18:48)
> 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
> 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));

These ioctls should all be wrapped, in gem_query(),
gem_context_set_param() etc.

You never ever want to have to debug an error from
igt_assert(!igt_ioctl(foo)), it is painful to the eyes and utterly
opaque.

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

Please use English. This is worse than
igt_require(__gem_context_set_param() == 0).

> +
> +       igt_assert((intel_execution_engines2 =
> +                   calloc(64, sizeof(*intel_execution_engines2))));
> +
> +       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]);

require here?

You can't just use "unknown[%d]%d" for future proofing?

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

igt_require_gem() maybe called multiple times per subtest.

Just gem_require_engine_list() ? Gives a good indicator which tests are
converted and which not, and a reminder that we still need to test
legacy ABIs in conjunction with the new era.
-Chris
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);
>   
>
Hi Antonio,

> > A new function gem_init_engine_list() is addedthat is called
> 
> nit: missing white space ^^^^^^^^^^^^^^^^^^^^^

Thanks :)

> > +	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.

yes, that's the same thing Chris pointed out, I'll put everything
into a macro.

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

Yes, the design of this changed form the previous iterations and
this is a leftover. It can, indeed, be safely num_engines. Thanks.

Thank you,
Andi
Quoting Andi Shyti (2019-02-08 11:03:21)
> Hi Antonio,
> 
> > > A new function gem_init_engine_list() is addedthat is called
> > 
> > nit: missing white space ^^^^^^^^^^^^^^^^^^^^^
> 
> Thanks :)
> 
> > > +   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.
> 
> yes, that's the same thing Chris pointed out, I'll put everything
> into a macro.

Not macros! Functions. The macros get stringified in all their
glory making the failure message hideous and unreadable.

The functions should already exist, for the most part at least.
-Chris
Hi Chris,

Thanks for the review!

I think fixed everything, just a few questions on the following

> > @@ -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);
> 
> igt_require_gem() maybe called multiple times per subtest.

I'm actually checking that with:

        if (intel_execution_engines2)
                return;

inside the gem_init_engine_list(). Shouldn't that be enough to
prevent multiple calls?

> Just gem_require_engine_list() ? Gives a good indicator which tests are
> converted and which not, and a reminder that we still need to test
> legacy ABIs in conjunction with the new era.

Can do that, but I thought that in the new era, when your and
Tvrtko's patch will get in, we will convert everything to the new
ABI's and this would remain an extra step (which BTW, wouldn't
prevent multiple calls either, as above).

Thanks,
Andi
Quoting Andi Shyti (2019-02-08 12:55:42)
> Hi Chris,
> 
> Thanks for the review!
> 
> I think fixed everything, just a few questions on the following
> 
> > > @@ -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);
> > 
> > igt_require_gem() maybe called multiple times per subtest.
> 
> I'm actually checking that with:
> 
>         if (intel_execution_engines2)
>                 return;
> 
> inside the gem_init_engine_list(). Shouldn't that be enough to
> prevent multiple calls?

Still not keen on having it here. It doesn't smell right; the purpose of
igt_require_gem() is to check the gpu is functional. Having a
side-effect of initialising an obscure struct used by a few tests?

> > Just gem_require_engine_list() ? Gives a good indicator which tests are
> > converted and which not, and a reminder that we still need to test
> > legacy ABIs in conjunction with the new era.
> 
> Can do that, but I thought that in the new era, when your and
> Tvrtko's patch will get in, we will convert everything to the new
> ABI's and this would remain an extra step (which BTW, wouldn't
> prevent multiple calls either, as above).

NO! The ABI doesn't simply no longer exist because we wish it so. The
tests for the old ABI will have to remain for as long as it is
accessible; which means that we need both sets of ABI for anything that
looks suspiciously like an ABI test.

It only bits and pieces that merely want to run over a set of HW that we
can simplify.
-Chris