Re: [PATCH i-g-t] lib|tests: Don't dereference engine outside for_each_ctx_engine macro

[Peter Senna Tschudin <peter.senna@linux.intel.com>]

Please ignore my previous emails on this topic. I underestimated the effort and the potential consequences of the changes I want to make. I have reviewed the code and compile tested it.

On 13.08.2024 14:45, Zbigniew Kempczyński wrote:
> Engine used inside for_each_ctx_engine() macro is valid only inside its
> block as it is accessing local stack memory. Using outside the block works
> by an accident if luckily nothing will overwrite this stack before use.
> Fix found risky usages by using engine only inside the block or do its
> copy.
> 
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>
Reviewed-by: Peter Senna Tschudin <peter.senna@linux.intel.com>

> ---
>  lib/igt_fb.c                    |  8 ++++----
>  tests/intel/gem_lmem_swapping.c | 14 ++++++++++----
>  2 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index ab162a5b74..addff2cfba 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -3050,13 +3050,13 @@ static void blitcopy(const struct igt_fb *dst_fb,
>  						   dst_fb->size);
>  		} else if (ahnd && block_copy_ok(src_fb) && block_copy_ok(dst_fb)) {
>  			for_each_ctx_engine(src_fb->fd, ictx, e) {
> -				if (gem_engine_can_block_copy(src_fb->fd, e))
> +				if (gem_engine_can_block_copy(src_fb->fd, e)) {
> +					do_block_copy(src_fb, dst_fb, mem_region, i, ahnd,
> +						      bb, bb_size, ictx, e);
>  					break;
> +				}
>  			}
>  			igt_assert_f(e, "No block copy capable engine found!\n");
> -
> -			do_block_copy(src_fb, dst_fb, mem_region, i, ahnd,
> -				      bb, bb_size, ictx, e);
>  		} else {
>  			igt_blitter_src_copy(dst_fb->fd,
>  					     ahnd, ctx, NULL,
> diff --git a/tests/intel/gem_lmem_swapping.c b/tests/intel/gem_lmem_swapping.c
> index b8f24742b8..b125261519 100644
> --- a/tests/intel/gem_lmem_swapping.c
> +++ b/tests/intel/gem_lmem_swapping.c
> @@ -189,6 +189,7 @@ init_object_ccs(int i915, struct object *obj, struct blt_copy_object *tmp,
>  {
>  	struct blt_block_copy_data_ext ext = {}, *pext = &ext;
>  	const struct intel_execution_engine2 *e;
> +	struct intel_execution_engine2 ec;
>  	struct blt_copy_data blt = {};
>  	struct blt_copy_batch *cmd;
>  	uint64_t size = 4096;
> @@ -196,8 +197,10 @@ init_object_ccs(int i915, struct object *obj, struct blt_copy_object *tmp,
>  
>  	obj->seed = seed;
>  	for_each_ctx_engine(i915, ctx, e) {
> -		if (gem_engine_can_block_copy(i915, e))
> +		if (gem_engine_can_block_copy(i915, e)) {
> +			ec = *e;
>  			break;
> +		}
>  	}
>  	igt_assert_f(e, "Ctx don't have blt engine\n");
>  
> @@ -224,7 +227,7 @@ init_object_ccs(int i915, struct object *obj, struct blt_copy_object *tmp,
>  	blt_set_object_ext(&ext.dst, 0, obj->blt_obj->x2, obj->blt_obj->y2,
>  			   SURFACE_TYPE_2D);
>  
> -	blt_block_copy(i915, ctx, e, ahnd, &blt, pext);
> +	blt_block_copy(i915, ctx, &ec, ahnd, &blt, pext);
>  	free(cmd);
>  }
>  
> @@ -255,14 +258,17 @@ verify_object_ccs(int i915, const struct object *obj,
>  {
>  	struct blt_block_copy_data_ext ext = {}, *pext = &ext;
>  	const struct intel_execution_engine2 *e;
> +	struct intel_execution_engine2 ec;
>  	struct blt_copy_data blt = {};
>  	struct blt_copy_batch *cmd;
>  	uint64_t size = 4096;
>  	unsigned long j, val, *buf;
>  
>  	for_each_ctx_engine(i915, ctx, e) {
> -		if (gem_engine_can_block_copy(i915, e))
> +		if (gem_engine_can_block_copy(i915, e)) {
> +			ec = *e;
>  			break;
> +		}
>  	}
>  	igt_assert_f(e, "Ctx don't have blt engine\n");
>  
> @@ -284,7 +290,7 @@ verify_object_ccs(int i915, const struct object *obj,
>  	blt_set_object_ext(&ext.src, 0, obj->blt_obj->x2, obj->blt_obj->y2,
>  			   SURFACE_TYPE_2D);
>  	blt_set_object_ext(&ext.dst, 0, tmp->x2, tmp->y2, SURFACE_TYPE_2D);
> -	blt_block_copy(i915, ctx, e, ahnd, &blt, pext);
> +	blt_block_copy(i915, ctx, &ec, ahnd, &blt, pext);
>  
>  	buf = gem_mmap__device_coherent(i915, tmp->handle, 0,
>  					obj->size, PROT_READ);