[igt-dev] [PATCH i-g-t] tests/i915/gem_ctx_persistence: Fixed assert issue seen for heartbeat-* tests

[Dixit, Ashutosh]

On Fri, 08 Apr 2022 09:56:43 -0700, Arjun Melkaveri wrote:
>
> heartbeat-* tests were failing with assert mentioned below.

Failing where? GuC or non-GuC? If it's GuC maybe tests are supposed to fail
since GuC doesn't support persistence properly?

> Issue got fixed by moving for_each_physical_ring to
> for_each_ctx_cfg_engine, and creating right ctx .
>
> Issue :-
> Stack trace:
>   #0 ../lib/igt_core.c:1756 __igt_fail_assert()
>   #1 ../tests/i915/gem_ctx_persistence.c:589 test_noheartbeat_close()

There is no assert on this line.

> diff --git a/tests/i915/gem_ctx_persistence.c b/tests/i915/gem_ctx_persistence.c
> index 00dda3a8..08cd2a89 100644
> --- a/tests/i915/gem_ctx_persistence.c
> +++ b/tests/i915/gem_ctx_persistence.c
> @@ -476,8 +476,10 @@ static bool set_preempt_timeout(int i915, const char *name, unsigned int value)
>	return true;
>  }
>
> -static void test_noheartbeat_many(int i915, int count, unsigned int flags)
> +static void test_noheartbeat_many(int i915, const intel_ctx_cfg_t *cfg,
> +				  int count, unsigned int flags)
>  {
> +	const struct intel_execution_engine2 *e;
>	unsigned long checked = 0;
>
>	cleanup(i915);
> @@ -489,25 +491,25 @@ static void test_noheartbeat_many(int i915, int count, unsigned int flags)
>	 * cleaned up.
>	 */
>
> -	for_each_physical_ring(e, i915) {
> +	for_each_ctx_cfg_engine(i915, cfg, e) {

But why? With an empty context these two lines are close to equivalent, so
why change it?

>		igt_spin_t *spin[count];
>		uint64_t ahnd;
>
> -		if (!set_preempt_timeout(i915, e->full_name, 250))
> +		if (!set_preempt_timeout(i915, e->name, 250))

Whey s/full_name/name/ ?

>			continue;
>
> -		if (!set_heartbeat(i915, e->full_name, 0))
> +		if (!set_heartbeat(i915, e->name, 0))
>			continue;
>
> -		igt_assert(set_heartbeat(i915, e->full_name, 500));
> +		igt_assert(set_heartbeat(i915, e->name, 500));
>
>		for (int n = 0; n < ARRAY_SIZE(spin); n++) {
>			const intel_ctx_t *ctx;
>
> -			ctx = intel_ctx_create(i915, NULL);
> +			ctx = ctx_create_persistence(i915, cfg, false);

Just above the comment says this:

        /*
         * If the user disables the heartbeat, after leaving behind
         * a number of long running *persistent* contexts, check they get
         * cleaned up.
         */

So now we are changing the context to be non-persistent, thereby changing
the test to make it pass?

So let's try to first figure out why the tests are failing and what we are
trying to fix.