Re: [PATCH v2 i-g-t 1/1] tests/intel/xe_pmu: Provision VFs inside dynamic subtests

[Gustavo Sousa <gustavo.sousa@intel.com>]

Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com> writes:

> VF setup helpers can call igt_assert(), which is not allowed in the outer
> igt_subtest_with_dynamic() scope. Set up VFs in the first dynamic case,
> reuse them across engine cases, and clean up after the group. This keeps
> VF setup out of unrelated subtests.
>
> Assisted-by: Copilot:Claude-Opus-5
> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
> Cc: Gustavo Sousa <gustavo.sousa@intel.com>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> ---
>  tests/intel/xe_pmu.c | 48 +++++++++++++++++++++++++++++++++-----------
>  1 file changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/tests/intel/xe_pmu.c b/tests/intel/xe_pmu.c
> index d41ebba1f..159a7f8a2 100644
> --- a/tests/intel/xe_pmu.c
> +++ b/tests/intel/xe_pmu.c
> @@ -117,6 +117,8 @@ static char xe_device[NAME_MAX];
>  static bool autoprobe;
>  static int total_exec_quantum;
>  static bool has_engine_active_ticks;
> +static unsigned int num_functions;

I feel like num_functions as a name for a global variable is a bit too
generic for what its purpose is.

Maybe sriov_num_fns (or the longer sriov_num_functions).

And then, for consistency, sriov_vfs_setup_started?

Anyway, the patch looks good to me. Thanks for this!

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

> +static bool vfs_setup_started;
>  
>  #define test_each_engine(test, fd, hwe) \
>  	igt_subtest_with_dynamic(test) \
> @@ -999,6 +1001,7 @@ static unsigned int enable_and_provision_vfs(int fd)
>  	num_vfs = total_vfs > 2 ? 2 : total_vfs;
>  
>  	/* Enable VF's */
> +	vfs_setup_started = true;
>  	igt_sriov_disable_driver_autoprobe(fd);
>  	igt_sriov_enable_vfs(fd, num_vfs);
>  	igt_require(igt_sriov_get_enabled_vfs(fd) == num_vfs);
> @@ -1021,6 +1024,16 @@ static unsigned int enable_and_provision_vfs(int fd)
>  	return num_vfs;
>  }
>  
> +static void ensure_vfs_enabled(int fd)
> +{
> +	if (num_functions)
> +		return;
> +
> +	igt_require_f(!vfs_setup_started,
> +		      "VF setup failed in an earlier dynamic subtest; see its log\n");
> +	num_functions = enable_and_provision_vfs(fd) + 1;
> +}
> +
>  static void unprovision_and_disable_vfs(int fd)
>  {
>  	int ret;
> @@ -1039,6 +1052,16 @@ static void unprovision_and_disable_vfs(int fd)
>  	igt_sriov_clear_exit_handler();
>  }
>  
> +static void cleanup_vfs_setup(int fd)
> +{
> +	if (!vfs_setup_started)
> +		return;
> +
> +	unprovision_and_disable_vfs(fd);
> +	vfs_setup_started = false;
> +	num_functions = 0;
> +}
> +
>  static void stash_gt_freq(int fd, uint32_t **stash_min, uint32_t **stash_max)
>  {
>  	int max_gt, gt;
> @@ -1190,28 +1213,29 @@ int igt_main()
>  	}
>  
>  	igt_subtest_group() {
> -		unsigned int num_fns;
> -
> -		igt_fixture()
> -			num_fns = enable_and_provision_vfs(fd) + 1;
> -
>  		igt_describe("Validate engine activity on all functions");
> -		test_each_engine("all-fn-engine-activity-load", fd, eci)
> -			engine_activity_all_fn(fd, eci, num_fns);
> +		test_each_engine("all-fn-engine-activity-load", fd, eci) {
> +			ensure_vfs_enabled(fd);
> +			engine_activity_all_fn(fd, eci, num_functions);
> +		}
>  
>  		igt_describe("Validate per-function engine activity");
> -		test_each_engine("fn-engine-activity-load", fd, eci)
> -			for (int fn = 0; fn < num_fns; fn++)
> -				engine_activity_fn(fd, eci, fn, XE_SRIOV_SCHED_PRIORITY_LOW);
> +		test_each_engine("fn-engine-activity-load", fd, eci) {
> +			ensure_vfs_enabled(fd);
> +			for (int fn = 0; fn < num_functions; fn++)
> +				engine_activity_fn(fd, eci, fn,
> +						   XE_SRIOV_SCHED_PRIORITY_LOW);
> +		}
>  
>  		igt_describe("Validate per-function engine activity when sched-if-idle is set");
>  		test_each_engine("fn-engine-activity-sched-if-idle", fd, eci) {
> -			for (int fn = 0; fn < num_fns; fn++)
> +			ensure_vfs_enabled(fd);
> +			for (int fn = 0; fn < num_functions; fn++)
>  				engine_activity_fn(fd, eci, fn, XE_SRIOV_SCHED_PRIORITY_NORMAL);
>  		}
>  
>  		igt_fixture()
> -			unprovision_and_disable_vfs(fd);
> +			cleanup_vfs_setup(fd);
>  	}
>  
>  	igt_subtest_group() {
> -- 
> 2.43.0