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

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

[Jan Sokolowski]

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@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);
+		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);
+			drm_close_driver(fd);
+		}
+	}
+
 }
-- 
2.34.1

✗ i915.CI.BAT: failure for series starting with [i-g-t,v2,1/1] tests/intel/xe_eudebug: refactor exec-queue-placements test

[Patchwork]

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

== Series Details ==

Series: series starting with [i-g-t,v2,1/1] tests/intel/xe_eudebug: refactor exec-queue-placements test
URL   : https://patchwork.freedesktop.org/series/143607/
State : failure

== Summary ==

CI Bug Log - changes from IGT_8195 -> IGTPW_12449
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_12449 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_12449, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

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

Participating hosts (42 -> 40)
------------------------------

  Missing    (2): bat-atsm-1 fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_12449:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@workarounds:
    - bat-adlp-9:         [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_8195/bat-adlp-9/igt@i915_selftest@live@workarounds.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_12449/bat-adlp-9/igt@i915_selftest@live@workarounds.html

  
Known issues
------------

  Here are the changes found in IGTPW_12449 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live:
    - bat-mtlp-8:         [PASS][3] -> [DMESG-FAIL][4] ([i915#12061]) +1 other test dmesg-fail
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_8195/bat-mtlp-8/igt@i915_selftest@live.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_12449/bat-mtlp-8/igt@i915_selftest@live.html
    - bat-adlp-9:         [PASS][5] -> [ABORT][6] ([i915#13399])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_8195/bat-adlp-9/igt@i915_selftest@live.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_12449/bat-adlp-9/igt@i915_selftest@live.html
    - bat-arlh-2:         [PASS][7] -> [INCOMPLETE][8] ([i915#12445])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_8195/bat-arlh-2/igt@i915_selftest@live.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_12449/bat-arlh-2/igt@i915_selftest@live.html

  * igt@i915_selftest@live@workarounds:
    - bat-arls-5:         [PASS][9] -> [DMESG-FAIL][10] ([i915#12061]) +1 other test dmesg-fail
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_8195/bat-arls-5/igt@i915_selftest@live@workarounds.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_12449/bat-arls-5/igt@i915_selftest@live@workarounds.html
    - bat-arlh-2:         [PASS][11] -> [INCOMPLETE][12] ([i915#13269])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_8195/bat-arlh-2/igt@i915_selftest@live@workarounds.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_12449/bat-arlh-2/igt@i915_selftest@live@workarounds.html

  
#### Possible fixes ####

  * igt@i915_selftest@live:
    - bat-adlp-6:         [ABORT][13] ([i915#13399]) -> [PASS][14] +1 other test pass
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_8195/bat-adlp-6/igt@i915_selftest@live.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_12449/bat-adlp-6/igt@i915_selftest@live.html

  * igt@i915_selftest@live@workarounds:
    - bat-mtlp-6:         [DMESG-FAIL][15] ([i915#12061]) -> [PASS][16] +1 other test pass
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_8195/bat-mtlp-6/igt@i915_selftest@live@workarounds.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_12449/bat-mtlp-6/igt@i915_selftest@live@workarounds.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#12061]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12061
  [i915#12445]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12445
  [i915#13269]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13269
  [i915#13399]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13399
  [i915#13494]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13494


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

  * CI: CI-20190529 -> None
  * IGT: IGT_8195 -> IGTPW_12449
  * Linux: CI_DRM_15966 -> CI_DRM_15967

  CI-20190529: 20190529
  CI_DRM_15966: 2334d1179d652eb4032fcb0d8e6f53cbc6a1011c @ git://anongit.freedesktop.org/gfx-ci/linux
  CI_DRM_15967: 50d15a728295807727a2642284d639fc71ecaa15 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_12449: 60034091ec5e0fc09c76c44fd3e34775b5ab681b @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  IGT_8195: 8195

== Logs ==

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

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

Re: [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@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);
> +		}
> +	}
> +
>   }