new engine discovery interface

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 <[email protected]>

---
 lib/igt_gt.c         | 110 ++++++++++++++++++++++++++++++++++++++++++-
 lib/igt_gt.h         |  14 +++++-
 lib/ioctl_wrappers.c |   3 ++
 3 files changed, 124 insertions(+), 3 deletions(-)

lib/igt_gt.c +10 -1

index 646696727ee4..7d933e64ec59 100644
@@ -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;
+

lib/igt_gt.c +57 -0

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

lib/igt_gt.c +10 -0

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

lib/igt_gt.h +12 -2

index 54e95da98084..fde67a6eb0ee 100644
@@ -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,

lib/ioctl_wrappers.c +1 -0

index 404c2fbf9355..57ed8c51bea5 100644
@@ -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>

lib/ioctl_wrappers.c void igt_require_gem(int fd) · line 1464

Quoted 6 lines

lib/ioctl_wrappers.c +2 -0

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

Hi Chris,

Thanks for the review!

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

Quoted 8 lines

lib/ioctl_wrappers.c void igt_require_gem(int fd) · line 1464

Quoted 6 lines

lib/ioctl_wrappers.c +2 -0

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

Quoted 3 lines

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

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 <[email protected]>

---
  lib/igt_gt.c         | 110 ++++++++++++++++++++++++++++++++++++++++++-
  lib/igt_gt.h         |  14 +++++-
  lib/ioctl_wrappers.c |   3 ++
  3 files changed, 124 insertions(+), 3 deletions(-)

lib/igt_gt.c +47 -1

index 646696727ee4..7d933e64ec59 100644
@@ -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)));
+

lib/igt_gt.c +23 -0

+
+	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
+	 */

lib/igt_gt.c +19 -0

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

lib/igt_gt.h +12 -2

index 54e95da98084..fde67a6eb0ee 100644
@@ -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,

lib/ioctl_wrappers.c +1 -0

index 404c2fbf9355..57ed8c51bea5 100644
@@ -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>

Quoted 1 line

A new function gem_init_engine_list() is addedthat is called

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

lib/igt_gt.c bool gem_ring_has_physical_engine(int fd, unsigned ring) · line 708

Quoted 2 lines

lib/igt_gt.c +2 -0

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

lib/igt_gt.c bool gem_ring_has_physical_engine(int fd, unsigned ring) · line 741

Quoted 2 lines

lib/igt_gt.c +2 -0

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

Hi Antonio,

Quoted 3 lines

Quoted 1 line

A new function gem_init_engine_list() is addedthat is called

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

Thanks :)

Quoted 3 lines

Quoted 2 lines

lib/igt_gt.c +2 -0

+   igt_assert(!igt_ioctl(fd,
+                         DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &ctx_param));

Quoted 5 lines

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.

With the new engine query method engines are reachable through an index and context they are combined with.

The 'gem_has_ring()' becomes 'gem_has_ring_by_idx()' that requires the index that the engine is mapped with in the driver.

The previous function becomes a wrapper to the new 'gem_has_ring_by_idx()'.

---
 lib/ioctl_wrappers.c | 10 ++++++----
 lib/ioctl_wrappers.h |  4 +++-
 2 files changed, 9 insertions(+), 5 deletions(-)

lib/ioctl_wrappers.c +0 -0

index 57ed8c51bea5..5c908f4a35a5 100644

== Series Details ==

Series: new engine discovery interface (rev7)
URL : https://patchwork.freedesktop.org/series/52699/
State : failure

== Summary ==

IGT patchset build failed on latest successful build 592b854fead32c2b0dac7198edfb9a6bffd66932 tools/intel_watermark: Clean up the platform checks in the ilk+ code

The output from the failed tests:

138/266 testcase check: gem_ctx_isolation FAIL 0.36 s

--- command --- /home/cidrm/igt-gpu-tools/tests/igt_command_line.sh gem_ctx_isolation --- stdout ---
tests/gem_ctx_isolation:
Checking invalid option handling...
Checking valid option handling...
Checking subtest enumeration...
FAIL: tests/gem_ctx_isolation
--- stderr ---
Received signal SIGSEGV.
Stack trace:
#0 [fatal_sig_handler+0xd5]
#1 [killpg+0x40]
#2 [__real_main687+0x810]
#3 [main+0x44]
#4 [__libc_start_main+0xe7]
#5 [_start+0x2a]
-------

248/266 testcase check: perf_pmu FAIL 0.21 s

--- command --- /home/cidrm/igt-gpu-tools/tests/igt_command_line.sh perf_pmu --- stdout ---
tests/perf_pmu:
Checking invalid option handling...
Checking valid option handling...
Checking subtest enumeration...
FAIL: tests/perf_pmu
--- stderr ---
Received signal SIGSEGV.

Stack trace:
#0 [fatal_sig_handler+0xd5]
#1 [killpg+0x40]
#2 [__real_main1671+0xf1f]
#3 [main+0x44]
#4 [__libc_start_main+0xe7]
#5 [_start+0x2a]
-------

Quoted 33 lines

138/266 testcase check: gem_ctx_isolation FAIL 0.36 s

--- command --- /home/cidrm/igt-gpu-tools/tests/igt_command_line.sh gem_ctx_isolation --- stdout ---
tests/gem_ctx_isolation:
Checking invalid option handling...
Checking valid option handling...
Checking subtest enumeration...
FAIL: tests/gem_ctx_isolation
--- stderr ---
Received signal SIGSEGV.
Stack trace:
#0 [fatal_sig_handler+0xd5]
#1 [killpg+0x40]
#2 [__real_main687+0x810]
#3 [main+0x44]
#4 [__libc_start_main+0xe7]
#5 [_start+0x2a]
-------

248/266 testcase check: perf_pmu FAIL 0.21 s

--- command --- /home/cidrm/igt-gpu-tools/tests/igt_command_line.sh perf_pmu --- stdout ---
tests/perf_pmu:
Checking invalid option handling...
Checking valid option handling...
Checking subtest enumeration...
FAIL: tests/perf_pmu
--- stderr ---
Received signal SIGSEGV.

Quoted 9 lines

Stack trace:
#0 [fatal_sig_handler+0xd5]
#1 [killpg+0x40]
#2 [__real_main1671+0xf1f]
#3 [main+0x44]
#4 [__libc_start_main+0xe7]
#5 [_start+0x2a]
-------

These tests are constructing subtests for each known engine. Your engine loop changes make it not possible to enumerate any names statically, without accessing the driver.

Quoted 48 lines · Patchwork

== Series Details ==

Series: new engine discovery interface (rev7)
URL : https://patchwork.freedesktop.org/series/52699/
State : failure

== Summary ==

IGT patchset build failed on latest successful build 592b854fead32c2b0dac7198edfb9a6bffd66932 tools/intel_watermark: Clean up the platform checks in the ilk+ code

The output from the failed tests:

138/266 testcase check: gem_ctx_isolation FAIL 0.36 s

--- command --- /home/cidrm/igt-gpu-tools/tests/igt_command_line.sh gem_ctx_isolation --- stdout ---
tests/gem_ctx_isolation:
Checking invalid option handling...
Checking valid option handling...
Checking subtest enumeration...
FAIL: tests/gem_ctx_isolation
--- stderr ---
Received signal SIGSEGV.
Stack trace:
#0 [fatal_sig_handler+0xd5]
#1 [killpg+0x40]
#2 [__real_main687+0x810]
#3 [main+0x44]
#4 [__libc_start_main+0xe7]
#5 [_start+0x2a]
-------

248/266 testcase check: perf_pmu FAIL 0.21 s

--- command --- /home/cidrm/igt-gpu-tools/tests/igt_command_line.sh perf_pmu --- stdout ---
tests/perf_pmu:
Checking invalid option handling...
Checking valid option handling...
Checking subtest enumeration...
FAIL: tests/perf_pmu
--- stderr ---
Received signal SIGSEGV.

Quoted 9 lines

Stack trace:
#0 [fatal_sig_handler+0xd5]
#1 [killpg+0x40]
#2 [__real_main1671+0xf1f]
#3 [main+0x44]
#4 [__libc_start_main+0xe7]
#5 [_start+0x2a]
-------

These tests are constructing subtests for each known engine. Your engine loop changes make it not possible to enumerate any names statically, without accessing the driver.

Quoted 60 lines · Petri Latvala

Quoted 48 lines · Patchwork

== Series Details ==

Series: new engine discovery interface (rev7)
URL : https://patchwork.freedesktop.org/series/52699/
State : failure

== Summary ==

IGT patchset build failed on latest successful build 592b854fead32c2b0dac7198edfb9a6bffd66932 tools/intel_watermark: Clean up the platform checks in the ilk+ code

The output from the failed tests:

138/266 testcase check: gem_ctx_isolation FAIL 0.36 s

--- command --- /home/cidrm/igt-gpu-tools/tests/igt_command_line.sh gem_ctx_isolation --- stdout ---
tests/gem_ctx_isolation:
Checking invalid option handling...
Checking valid option handling...
Checking subtest enumeration...
FAIL: tests/gem_ctx_isolation
--- stderr ---
Received signal SIGSEGV.
Stack trace:
#0 [fatal_sig_handler+0xd5]
#1 [killpg+0x40]
#2 [__real_main687+0x810]
#3 [main+0x44]
#4 [__libc_start_main+0xe7]
#5 [_start+0x2a]
-------

248/266 testcase check: perf_pmu FAIL 0.21 s

--- command --- /home/cidrm/igt-gpu-tools/tests/igt_command_line.sh perf_pmu --- stdout ---
tests/perf_pmu:
Checking invalid option handling...
Checking valid option handling...
Checking subtest enumeration...
FAIL: tests/perf_pmu
--- stderr ---
Received signal SIGSEGV.

Quoted 9 lines

Stack trace:
#0 [fatal_sig_handler+0xd5]
#1 [killpg+0x40]
#2 [__real_main1671+0xf1f]
#3 [main+0x44]
#4 [__libc_start_main+0xe7]
#5 [_start+0x2a]
-------

Quoted 3 lines

These tests are constructing subtests for each known engine. Your engine loop changes make it not possible to enumerate any names statically, without accessing the driver.

Is generating subtest names without accessing the driver an absolute requirement?

This particular failure would be fixable if the engine list was initialized on the first call to __for_each_engine_class_instance, as called by the perf_pmu above. And the fixture block is already ran in test enumeration, where the driver is opened. I think a lot of tests are like this. So in this context a simple query ioctl on top doesn't sound bad.

But I also remember, and I think it was due bug tracking limitations, that in the past the desire was test enumeration is constant regardless of the execution platform.

I think it would be simpler if we didn't have to maintain a separate static list of all possible engines. To make that work we would need to detect the mode of execution (list vs execute) in the engine iterator as well. So gain, to me it sounds preferable that tests would be allowed to enumerate dynamically.

So question is can CI/bugtracking cope with that, and are we okay with doing a trivial ioctl from the for_each_engine_<something>.

Quoted 79 lines · Tvrtko Ursulin via igt-dev

Quoted 60 lines · Petri Latvala

Quoted 49 lines · Patchwork

== Series Details ==

Series: new engine discovery interface (rev7)
URL : https://patchwork.freedesktop.org/series/52699/
State : failure

== Summary ==

IGT patchset build failed on latest successful build 592b854fead32c2b0dac7198edfb9a6bffd66932 tools/intel_watermark: Clean up the platform checks in the ilk+ code

The output from the failed tests:

138/266 testcase check: gem_ctx_isolation FAIL 0.36 s

--- command --- /home/cidrm/igt-gpu-tools/tests/igt_command_line.sh gem_ctx_isolation --- stdout ---
tests/gem_ctx_isolation:
Checking invalid option handling...
Checking valid option handling...
Checking subtest enumeration...
FAIL: tests/gem_ctx_isolation
--- stderr ---
Received signal SIGSEGV.
Stack trace:

   #0 [fatal_sig_handler+0xd5]
   #1 [killpg+0x40]
   #2 [__real_main687+0x810]
   #3 [main+0x44]
   #4 [__libc_start_main+0xe7]
   #5 [_start+0x2a]

-------

248/266 testcase check: perf_pmu FAIL 0.21 s

--- command --- /home/cidrm/igt-gpu-tools/tests/igt_command_line.sh perf_pmu --- stdout ---
tests/perf_pmu:
Checking invalid option handling...
Checking valid option handling...
Checking subtest enumeration...
FAIL: tests/perf_pmu
--- stderr ---
Received signal SIGSEGV.
Stack trace:

Quoted 8 lines

   #0 [fatal_sig_handler+0xd5]
   #1 [killpg+0x40]
   #2 [__real_main1671+0xf1f]
   #3 [main+0x44]
   #4 [__libc_start_main+0xe7]
   #5 [_start+0x2a]

-------

Quoted 3 lines

These tests are constructing subtests for each known engine. Your engine loop changes make it not possible to enumerate any names statically, without accessing the driver.

Is generating subtest names without accessing the driver an absolute requirement?

This particular failure would be fixable if the engine list was initialized on the first call to __for_each_engine_class_instance, as called by the perf_pmu above. And the fixture block is already ran in test enumeration, where the driver is opened. I think a lot of tests are like this. So in this context a simple query ioctl on top doesn't sound bad.

But I also remember, and I think it was due bug tracking limitations, that in the past the desire was test enumeration is constant regardless of the execution platform.

Quoted 8 lines

I think it would be simpler if we didn't have to maintain a separate static list of all possible engines. To make that work we would need to detect the mode of execution (list vs execute) in the engine iterator as well. So gain, to me it sounds preferable that tests would be allowed to enumerate dynamically.

So question is can CI/bugtracking cope with that, and are we okay with doing a trivial ioctl from the for_each_engine_<something>.

For quite some time it is enforced that, in IGT, listing tests is exhaustive and independent of the machine. CI heavily relies on this assumption. We generate and manage the testlists on macines that are GT-less.

Especially for 'shards':

1. list all test,
split it in 35 'randomized' chunks that have roughly the same execution time

2. currnet_chunk=35;
while (current_chunk)
if(any(machines, is_free))
schedule(current_chunk--, first_free(machines))

I don't really see how we can improve upon that.

Overall keeping static list of all the possible engines for listing and then "skipping" the actual exectuion if it's not there seems like a simpler issue to tackle.

Is generating subtest names without accessing the driver an absolute requirement?

This particular failure would be fixable if the engine list was initialized on the first call to __for_each_engine_class_instance, as called by the perf_pmu above. And the fixture block is already ran in test enumeration, where the driver is opened. I think a lot of tests are like this. So in this context a simple query ioctl on top doesn't sound bad.

I think it would be simpler if we didn't have to maintain a separate static list of all possible engines. To make that work we would need to detect the mode of execution (list vs execute) in the engine iterator as well. So gain, to me it sounds preferable that tests would be allowed to enumerate dynamically.

Quoted 2 lines · Tvrtko Ursulin

Is generating subtest names without accessing the driver an absolute requirement?

Yes.

Quoted 6 lines

This particular failure would be fixable if the engine list was initialized on the first call to __for_each_engine_class_instance, as called by the perf_pmu above. And the fixture block is already ran in test enumeration, where the driver is opened. I think a lot of tests are like this. So in this context a simple query ioctl on top doesn't sound bad.

Fixtures are not executed when enumerating tests.

Quoted 6 lines

I think it would be simpler if we didn't have to maintain a separate static list of all possible engines. To make that work we would need to detect the mode of execution (list vs execute) in the engine iterator as well. So gain, to me it sounds preferable that tests would be allowed to enumerate dynamically.

Can the tests be refactored to have a subtest per engine _class_? That's a static list, isn't it? Within the subtest they would loop over instances of the class. Kind of like tests that loop over connectors of $type.

Quoted 5 lines · Petri Latvala

Quoted 2 lines · Tvrtko Ursulin

Is generating subtest names without accessing the driver an absolute requirement?

Yes.

I don't like how this creates a coupling between CI _implementation_ and IGT codebase and think can be improved.

Quoted 9 lines

Quoted 7 lines

This particular failure would be fixable if the engine list was initialized on the first call to __for_each_engine_class_instance, as called by the perf_pmu above. And the fixture block is already ran in test enumeration, where the driver is opened. I think a lot of tests are like this. So in this context a simple query ioctl on top doesn't sound bad.

Fixtures are not executed when enumerating tests.

Okay.

Quoted 7 lines

Quoted 6 lines

I think it would be simpler if we didn't have to maintain a separate static list of all possible engines. To make that work we would need to detect the mode of execution (list vs execute) in the engine iterator as well. So gain, to me it sounds preferable that tests would be allowed to enumerate dynamically.

Quoted 4 lines

Can the tests be refactored to have a subtest per engine _class_? That's a static list, isn't it? Within the subtest they would loop over instances of the class. Kind of like tests that loop over connectors of $type.

Sounds quite invasive, fundamental and limiting (how to run a test against a single engine in this world?) change.

How about instead, which is more or
less copy & paste from my reply to Arek:

Would it be feasible to generate the test list on sharded platforms?

It could be a "list tests" job which you could send to each of the shard platforms (one machine of each platforms), executed before randomizing and chunking.

Advantage there would be that you don't send chunks containing tests which a particular platform cannot run.

And of course another advantage is that we remove unnecessary coupling between two components so IGT maintenance becomes easier. (No two sets of engines, no two ways to iterate those sets, and no need to remember when to use each.)