[PATCH v2 i-g-t 0/1] xe_pmu: Do not enable VFs in unrelated subtests

[PATCH v2 i-g-t 0/1] xe_pmu: Do not enable VFs in unrelated subtests

[Marcin Bernatowicz]

Gustavo's v2 proposal avoids enabling VFs for unrelated xe_pmu subtests.
This update keeps that goal, but moves setup into the first selected
dynamic case because SR-IOV helpers can call igt_assert(), which is not
allowed in the outer dynamic container.

Later engine cases reuse the VFs, and cleanup runs after the group. If
setup fails partway through, later cases skip and point to the case with
the detailed failure log.

Previous proposal:
Gustavo's v2: https://lore.kernel.org/igt-dev/20261006-xe_pmu-do-not-enable-vfs-in-unrelated-subtests-v2-1-38f204d54553@intel.com/

Marcin Bernatowicz (1):
  tests/intel/xe_pmu: Provision VFs inside dynamic subtests

 tests/intel/xe_pmu.c | 48 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 12 deletions(-)

-- 
2.43.0

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

[Marcin Bernatowicz]

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;
+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

✓ i915.CI.BAT: success for xe_pmu: Do not enable VFs in unrelated subtests

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]

== Series Details ==

Series: xe_pmu: Do not enable VFs in unrelated subtests
URL   : https://patchwork.freedesktop.org/series/176050/
State : success

== Summary ==

CI Bug Log - changes from IGT_9143 -> IGTPW_15977
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_15977/index.html

Participating hosts (39 -> 38)
------------------------------

  Missing    (1): bat-dg2-13 


Changes
-------

  No changes found


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_9143 -> IGTPW_15977

  CI-20190529: 20190529
  CI_DRM_19331: 3bb5625e4dccd61506ad2815da07ac082e1df614 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_15977: b2cc4af5ae912534172ac5e4d6e6149dcab1500e @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  IGT_9143: 992e2bc5d5fcaef292ea89e3db5cb7a50b6edfc2 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_15977/index.html

[-- Attachment #2: Type: text/html, Size: 1643 bytes --]

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

[Gustavo Sousa]

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