Re: [igt-dev] [PATCH i-g-t 1/1] tests/i915/gem_spin_batch: Removing context persistence

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Mon, 14 Feb 2022 09:22:08 -0800, Jasmine Newsome wrote:
>
> The spin all test relied on context persistence unecessarily
> by trying to destroy contexts while keeping spinners active.
> The current implementation of context persistence in i915
> can cause failures, and persistence is not needed for this
> test.

IGT follows the kernel coding style so 8 wide tabs and no spaces, you can
run kernel checkpatch to check IGT patches "for the most part" (ignoring
kernel specific issues).

> Moving intel_ctx_destroy after igt_spin_end/free.
>
> Signed-off-by: Jasmine Newsome <jasmine.newsome@intel.com>
> ---
>  tests/i915/gem_spin_batch.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/tests/i915/gem_spin_batch.c b/tests/i915/gem_spin_batch.c
> index 653812c7..65dccd83 100644
> --- a/tests/i915/gem_spin_batch.c
> +++ b/tests/i915/gem_spin_batch.c
> @@ -145,28 +145,31 @@ static void spin_all(int i915, const intel_ctx_t *ctx, unsigned int flags)
>	struct igt_spin *spin, *n;
>	uint64_t ahnd;
>	IGT_LIST_HEAD(list);
> +        const intel_ctx_t *local_ctx[GEM_MAX_ENGINES];
> +        int num_ctx;
>
> -	for_each_ctx_cfg_engine(i915, &cfg, e) {
> +	num_ctx = 0;
> +        for_each_ctx_cfg_engine(i915, &cfg, e) {
>		if (!gem_class_can_store_dword(i915, e->class))
>			continue;
>
>		if (flags & PARALLEL_SPIN_NEW_CTX)
> -			ctx = intel_ctx_create(i915, &cfg);
> +			local_ctx[num_ctx] = intel_ctx_create(i915, &cfg);
>		ahnd = get_reloc_ahnd(i915, ctx->id);
>
>		/* Prevent preemption so only one is allowed on each engine */
>		spin = igt_spin_new(i915,
>				    .ahnd = ahnd,
> -				    .ctx = ctx,
> +				    .ctx = local_ctx[num_ctx],

This is not initialized if !PARALLEL_SPIN_NEW_CTX.

I would say local_ctx[] array is not needed. We could do something like:

igt_spin_new(.ctx = flags & PARALLEL_SPIN_NEW_CTX ? intel_ctx_create() : ctx);

And then use intel_ctx_destroy(spin->execbuf.rsvd1) before freeing the spin
in the igt_list_for_each_entry_safe() loop below.

>				    .engine = e->flags,
>				    .flags = (IGT_SPIN_POLL_RUN |
>					      IGT_SPIN_NO_PREEMPTION));
> -		if (flags & PARALLEL_SPIN_NEW_CTX)
> -			intel_ctx_destroy(i915, ctx);
>
> -		igt_spin_busywait_until_started(spin);
> -		igt_list_move(&spin->link, &list);
> -	}
> +                igt_assert_eq(spin->execbuf.rsvd1, local_ctx[num_ctx]->id);
> +                igt_spin_busywait_until_started(spin);
> +                igt_list_move(&spin->link, &list);
> +                num_ctx++;
> +        }
>
>	igt_list_for_each_entry_safe(spin, n, &list, link) {
>		igt_assert(gem_bo_busy(i915, spin->handle));
> @@ -176,6 +179,11 @@ static void spin_all(int i915, const intel_ctx_t *ctx, unsigned int flags)
>		igt_spin_free(i915, spin);
>		put_ahnd(ahnd);
>	}
> +
> +       if (flags & PARALLEL_SPIN_NEW_CTX){
> +               while (num_ctx)
> +                       intel_ctx_destroy(i915, local_ctx[--num_ctx]);
> +       }
>  }
>
>  static bool has_userptr(int fd)
> --
> 2.25.1
>