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

["Manszewski, Christoph" <christoph.manszewski@intel.com>]

Hi Jan,

On 17.01.2025 09:20, 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.
> 
> Refactor exec-queue-placements test so that all possible
> failure paths are serviced, and add a proper cleanup method,
> ccs_mode_restore.
> 
> Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com>
> ---
> 
> v2: Forgot proper path in title
> v3: More changes. Moved test back to where it originally was

Sorry I missed that version and replied to v2 earlier, though some of my 
comments still stand.

> 
> ---
> 
>   tests/intel/xe_eudebug.c | 41 ++++++++++++++++++++++++++++++++++------
>   1 file changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/intel/xe_eudebug.c b/tests/intel/xe_eudebug.c
> index 91e9ae885..9787183ed 100644
> --- a/tests/intel/xe_eudebug.c
> +++ b/tests/intel/xe_eudebug.c
> @@ -2797,7 +2797,7 @@ static void ccs_mode_all_engines(int num_gt)
>   
>   		igt_assert(igt_sysfs_printf(gt_fd, "ccs_mode", "%u", num_slices) > 0);
>   		igt_assert(igt_sysfs_scanf(gt_fd, "ccs_mode", "%u", &ccs_mode) > 0);
> -		igt_assert(num_slices == ccs_mode);
> +		igt_require(num_slices == ccs_mode);

Can you explain this change? We successfully write ccs_mode but we 
expect to don't read back the written value? I admit that I don't know 
how this setting works, but that looks suspicious at first glance.

>   		close(gt_fd);
>   	}
>   
> @@ -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);

See my comment from v2.

> +		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;
> @@ -2920,16 +2939,26 @@ igt_main
>   		test_empty_discovery(fd, DISCOVERY_DESTROY_RESOURCES, 16);
>   
>   	igt_subtest_group {
> -		igt_fixture {
> +		bool restore_ccs = false;
> +
> +		igt_subtest("exec-queue-placements") {
>   			drm_close_driver(fd);
> +			fd = -1;
>   			ccs_mode_all_engines(gt_count);
> +			restore_ccs = true;

Do we need this flag? I would assume, that we always want to restore the 
previous state. It would just have no effect at worst.


>   			fd = drm_open_driver(DRIVER_XE);
> -		}
> -
> -		igt_subtest("exec-queue-placements")
>   			test_basic_sessions(fd, EXEC_QUEUES_PLACEMENTS, 1, true);
> +		}
> +		igt_fixture {
> +			if (restore_ccs) {
> +				drm_close_driver(fd);
> +				fd = -1;
> +				ccs_mode_restore(gt_count);
> +			}
> +			if (fd == -1)
> +				fd = drm_open_driver(DRIVER_XE);

Like in my comment in v2 - that looks like a weird workaround for 
skipping too early in the fixture.

Thanks,
Christoph
> +		}
>   	}
> -
>   	igt_fixture {
>   		xe_eudebug_enable(fd, was_enabled);
>   		drm_close_driver(fd);