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

[Kamil Konieczny]

Hi Peter,
On 2024-03-28 at 13:42:51 +0100, Peter Senna Tschudin wrote:
> This patch fixes the tests many-4k-incremental and many-4k-zero from
> gem_exec_capture that are currently failing with an invalid file
> descriptor error.
> 
> tests/intel/gem_exec_capture.c
>     many(), userptr(), capture_invisible()

Please cite error log, delete two above lines and write that problem
is with macro find_first_available_engine() (you later describe in datails).

>         find_first_available_engine()
>             for_each_ctx_engine()
>                 for_each_ctx_cfg_engine()
> 
> find_first_available_engine() expects a struct intel_execution_engine2
> *e as parameter. for_each_ctx_cfg_engine() allocates a struct
> intel_engine_data ed inside a for loop and then update e to point to an
> element of ed.
> 
> The problem is that ed has the block scope of the for loop, and trying
> to access its content through e after the for loop has ended creates
> undefined behavior.
> 
> This patch updates e to point to the same content at a different memory
> region, to avoid the block scope of ed. `e = &saved.engine;` updates e
> to point to the same content but without scope restrictions as both
> saved and e have the scope of the calling function.
> 
> v5: Update subject and commit message.
> 
> Signed-off-by: Peter Senna Tschudin <me at petersenna.com>
> ---
>  tests/intel/gem_exec_capture.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
> index 57b178f3e..f7b10bdd4 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)) { \
> +				saved = configure_hangs(fd, e, ctx->id); \
>  				break; \
> +			}  \
> +		e = &saved.engine; \

Ok, we need 'e' later on, so it is ok.

>  		igt_assert(e); \

But this assert do no work, it will always succeed. Imho here
code should check if it actually saved anything, so what about:

#define find_first_available_engine(fd, ctx, e, saved) \
    do { \
        bool no_dw_store_engine = true; \
        \
        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)) { \
                saved = configure_hangs(fd, e, ctx->id); \
                no_dw_store_engine = false; \
                break; \
            }  \
        e = &saved.engine; \
        igt_skip_on(no_dw_store_engine); \
    } while(0)


Regards,
Kamil

> -		saved = configure_hangs(fd, e, ctx->id); \
>  	} while(0)
>  
>  static void many(int fd, int dir, uint64_t size, unsigned int flags)
> -- 
> 2.44.0
>