[PATCH i-g-t v2] tests/intel/gem_exec_capture: Fix many-* subtests

Peter Senna Tschudin me at petersenna.com
Thu Apr 11 19:53:34 UTC 2024


Currently trying to run `gem_exec_capture --run-subtest
many-4K-incremental` or `gem_exec_capture --run-subtest many-4K-zero`
will fail with:

 (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: Test
 assertion failure function gem_engine_properties_configure, file
 ../lib/i915/gem_engine_topology.c:577:
 (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: Failed assertion: ret == 1
 (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: Last errno: 9, Bad file descriptor
 (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: error: -1 != 1

This problem happens inside the macro find_first_available_engine()
when:
 1. for_each_ctx_engine() allocates an struct intel_engine_data 'ed'
    inside a for loop. The core of the issue is that ed only exists
    inside the for loop. As soon as the for loop ends, ed is out of scope
    and after it's lifetime.
 2. intel_get_current_engine() sets '*e' to an address of ed. This is ok
    while inside the for loop, and is undefined behavior after the for
    loop ends.
 3. configure_hangs() uses '*e' after the lifetime of 'ed' has ended
    leading to undefined behavior
 4. After the call to find_first_available_engine() __captureN() will
    fail as it expects '*e' to be valid. This is also undefined
    behavior.

Additionally, this patch sets 'e' to NULL early on at the macro, and at
the end check if it is still NULL. If so it will skip the test to
prevent downstream code from crashing.

 v2: removed the 'igt_assert(tmpe)'; added the explanation about
     checking if e is still NULL by the end of the macro to decide to skip
     the test or not.

Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
Signed-off-by: Peter Senna Tschudin <me at petersenna.com>
---
 tests/intel/gem_exec_capture.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
index 57b178f3e..e6598ec42 100644
--- a/tests/intel/gem_exec_capture.c
+++ b/tests/intel/gem_exec_capture.c
@@ -662,13 +662,18 @@ static bool needs_recoverable_ctx(int fd)
 
 #define find_first_available_engine(fd, ctx, e, saved) \
 	do { \
+		struct intel_execution_engine2 *tmpe = NULL; \
+		e = NULL; \
+		\
 		ctx = intel_ctx_create_all_physical(fd); \
 		igt_assert(ctx); \
-		for_each_ctx_engine(fd, ctx, e) \
-			for_each_if(gem_class_can_store_dword(fd, e->class)) \
+		for_each_ctx_engine(fd, ctx, tmpe) \
+			if(gem_class_can_store_dword(fd, tmpe->class)) { \
+				saved = configure_hangs(fd, tmpe, ctx->id); \
+				e = &saved.engine; \
 				break; \
-		igt_assert(e); \
-		saved = configure_hangs(fd, e, ctx->id); \
+			} \
+		igt_skip_on_f(e == NULL, "no capable engine found\n"); \
 	} while(0)
 
 static void many(int fd, int dir, uint64_t size, unsigned int flags)
-- 
2.44.0



More information about the igt-dev mailing list