[PATCH i-g-t v2 1/1] tests/intel/xe_eudebug: refactor exec-queue-placements test

[Manszewski, Christoph]

Hi Jan,

On 16.01.2025 13:57, Jan Sokolowski wrote:
> In some cases, ccs_mode_all_engines can fail,
> which will cause test fixture to not execute properly
> and put the rest of the tests in an unstable state. Also,
> ccs_mode_all_engines changes the state of the card for
> other tests as well, thus it should clean after itself too,
> which until now it didn't do.
> 
> Move the test to the end in the execution list and add
> a proper cleanup method, ccs_mode_restore;
> 
> Signed-off-by: Jan Sokolowski <jan.sokolowski at intel.com>
> ---
> 
> v2: Forgot proper path in title
> 
> ---
>   tests/intel/xe_eudebug.c | 46 ++++++++++++++++++++++++++++++----------
>   1 file changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/intel/xe_eudebug.c b/tests/intel/xe_eudebug.c
> index 91e9ae885..c93b55857 100644
> --- a/tests/intel/xe_eudebug.c
> +++ b/tests/intel/xe_eudebug.c
> @@ -2805,6 +2805,25 @@ static void ccs_mode_all_engines(int num_gt)
>   	igt_require(num_gts_with_ccs_mode > 0);
>   }
>   
> +static void ccs_mode_restore(int num_gt)
> +{
> +	int fd, gt, gt_fd, ccs_mode, num_slices;
> +
> +	for (gt = 0; gt < num_gt; gt++) {
> +		fd = drm_open_driver(DRIVER_XE);
> +		gt_fd = xe_sysfs_gt_open(fd, gt);
> +		close(fd);
> +
> +		if (igt_sysfs_scanf(gt_fd, "num_cslices", "%u", &num_slices) <= 0)
> +			continue;
> +
> +		igt_assert(igt_sysfs_printf(gt_fd, "ccs_mode", "%u", 1) > 0);

Well - this doesn't really restore the previous value, rather sets it to 1.

> +		igt_assert(igt_sysfs_scanf(gt_fd, "ccs_mode", "%u", &ccs_mode) > 0);
> +		igt_assert(ccs_mode == 1);
> +		close(gt_fd);
> +	}
> +}
> +
>   igt_main
>   {
>   	bool was_enabled;
> @@ -2919,17 +2938,6 @@ igt_main
>   	igt_subtest("discovery-empty-clients")
>   		test_empty_discovery(fd, DISCOVERY_DESTROY_RESOURCES, 16);
>   
> -	igt_subtest_group {
> -		igt_fixture {
> -			drm_close_driver(fd);
> -			ccs_mode_all_engines(gt_count);
> -			fd = drm_open_driver(DRIVER_XE);
> -		}
> -
> -		igt_subtest("exec-queue-placements")
> -			test_basic_sessions(fd, EXEC_QUEUES_PLACEMENTS, 1, true);
> -	}
> -
>   	igt_fixture {
>   		xe_eudebug_enable(fd, was_enabled);
>   		drm_close_driver(fd);
> @@ -2984,4 +2992,20 @@ igt_main
>   			free(multigpu_was_enabled);
>   		}
>   	}
> +
> +	igt_subtest_group {
> +		igt_fixture {
> +			ccs_mode_all_engines(gt_count);
> +		}
> +
> +		igt_subtest("exec-queue-placements") {
> +			fd = drm_open_driver(DRIVER_XE);
> +
> +			test_basic_sessions(fd, EXEC_QUEUES_PLACEMENTS, 1, true);
> +
> +			ccs_mode_restore(gt_count);

This looks weird - the fixture sets something, that the test unsets. I 
think this should be symmetric - either we want the setting to apply for 
a subtest_group and we set/unset it in a fixture, or we want it to be 
subtest specific so we set/unset it in a subtest. The original placement 
in the fixture indicates the former, though for now we only have a 
single subtest and I am not sure what the plans for future tests are.

I am also not convinced about moving the subtest group to work around 
the fact, that the skip from 'ccs_mode_all_engines' could leave the 
device closed. You could just fix it by either returning an appropriate 
value based on which you skip after reopening the driver in the fixture, 
or let the function handle the closing and opening by passing in the fd, 
and returning a new one. In the end I am also not sure why we need that 
close/open in the first place though I assume it is there for a reason.

Thanks,
Christoph

> +			drm_close_driver(fd);
> +		}
> +	}
> +
>   }