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

[Peter Senna Tschudin]

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.

This patch fixes the issue in two steps:
 1. Moves the call to configure_hangs() to inside the for loop, where
    '*e' is valid because there are no issues with scope and lifetime of
    'ed'.
 2. Adds `e = &saved.engine;` to the end of
    find_first_available_engine(). The reason why this works is twofold:
    First 'saved' has the same content as e had when there were no
    variable scope and lifetime issues. Second both '*e' and 'saved' are
    defined in many() meaning that they both have the same scope and
    lifetime.

 v6: new commit message; moves igt_assert(e); to before the call to
     configure_hangs(). This check is there because '*e' is set by
     intel_get_current_engine() which will return NULL if ed->n >=
     ed->nengines.

Signed-off-by: Peter Senna Tschudin <me at petersenna.com>
---
 tests/intel/gem_exec_capture.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
index 57b178f3e..a8348f21b 100644
--- a/tests/intel/gem_exec_capture.c
+++ b/tests/intel/gem_exec_capture.c
@@ -665,10 +665,12 @@ static bool needs_recoverable_ctx(int fd)
 		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_if(gem_class_can_store_dword(fd, e->class)) { \
+				igt_assert(e); \
+				saved = configure_hangs(fd, e, ctx->id); \
 				break; \
-		igt_assert(e); \
-		saved = configure_hangs(fd, e, ctx->id); \
+			} \
+		e = &saved.engine; \
 	} while(0)
 
 static void many(int fd, int dir, uint64_t size, unsigned int flags)
-- 
2.44.0