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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 19/02/2022 01:32, 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.

Could you please expand a bit on "current implementation" and "can cause 
failures"?

Also from the subject of "Removing context persistence" I was expecting 
to see usage of I915_CONTEXT_PARAM_PERSISTENCE to actually change the mode.

My concern is that the pattern of destroying contexts while keeping 
things active on the GPU is very wide spread in IGT and possibly exists 
in userspace as well.

Has the wider story been analysed by the architects here and what is the 
plan? Do we actually know no userspace actually depends on it?

[Comes back later, after spotting the cover letter.]

So it's only GuC, not i915, so please say that in this commit message 
since cover letters are not saved in git history.

Regards,

Tvrtko

> 
> Moving intel_ctx_destroy after igt_spin_end.
> 
> Signed-off-by: Jasmine Newsome <jasmine.newsome@intel.com>
> ---
>   tests/i915/gem_spin_batch.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/i915/gem_spin_batch.c b/tests/i915/gem_spin_batch.c
> index 653812c7..707d69b6 100644
> --- a/tests/i915/gem_spin_batch.c
> +++ b/tests/i915/gem_spin_batch.c
> @@ -161,8 +161,6 @@ static void spin_all(int i915, const intel_ctx_t *ctx, unsigned int flags)
>   				    .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);
> @@ -172,6 +170,8 @@ static void spin_all(int i915, const intel_ctx_t *ctx, unsigned int flags)
>   		igt_assert(gem_bo_busy(i915, spin->handle));
>   		ahnd = spin->ahnd;
>   		igt_spin_end(spin);
> +		if (flags & PARALLEL_SPIN_NEW_CTX)
> +			intel_ctx_destroy(i915, spin->opts.ctx);
>   		gem_sync(i915, spin->handle);
>   		igt_spin_free(i915, spin);
>   		put_ahnd(ahnd);