[igt-dev] [PATCH i-g-t 1/3] tests/perf_pmu: Compare against requested freq in frequency subtest

[igt-dev] [PATCH i-g-t 1/3] tests/perf_pmu: Compare against requested freq in frequency subtest

[Ashutosh Dixit]

After the i915 commit 95ccf312a1e4f ("drm/i915/guc/slpc: Allow SLPC to use
efficient frequency"), FW uses the requested freq as the efficient freq
which can exceed the max freq set. Therefore, in the "min freq" part of the
igt@perf_pmu@frequency subtest, compare the requested freq reported by PMU
not against the set freq but against the requested freq reported in sysfs.

Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6806
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 tests/i915/perf_pmu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
index f363db2ba13..d09eec8f347 100644
--- a/tests/i915/perf_pmu.c
+++ b/tests/i915/perf_pmu.c
@@ -1546,7 +1546,7 @@ test_interrupts_sync(int gem_fd)
 static void
 test_frequency(int gem_fd)
 {
-	uint32_t min_freq, max_freq, boost_freq;
+	uint32_t min_freq, max_freq, boost_freq, min_req;
 	uint64_t val[2], start[2], slept;
 	double min[2], max[2];
 	igt_spin_t *spin;
@@ -1587,6 +1587,7 @@ test_frequency(int gem_fd)
 
 	min[0] = 1e9*(val[0] - start[0]) / slept;
 	min[1] = 1e9*(val[1] - start[1]) / slept;
+	min_req = igt_sysfs_get_u32(sysfs, "gt_cur_freq_mhz");
 
 	igt_spin_free(gem_fd, spin);
 	gem_quiescent_gpu(gem_fd); /* Don't leak busy bo into the next phase */
@@ -1633,7 +1634,7 @@ test_frequency(int gem_fd)
 	igt_info("Max frequency: requested %.1f, actual %.1f\n",
 		 max[0], max[1]);
 
-	assert_within_epsilon(min[0], min_freq, tolerance);
+	assert_within_epsilon(min[0], min_req, tolerance);
 	/*
 	 * On thermally throttled devices we cannot be sure maximum frequency
 	 * can be reached so use larger tolerance downards.
-- 
2.38.0

[igt-dev] [PATCH i-g-t 1/3] tests/perf_pmu: Compare against requested freq in frequency subtest

[Ashutosh Dixit]

After the i915 commit 95ccf312a1e4f ("drm/i915/guc/slpc: Allow SLPC to use
efficient frequency"), FW uses the requested freq as the efficient freq
which can exceed the max freq set. Therefore, in the "min freq" part of the
igt@perf_pmu@frequency subtest, compare the requested freq reported by PMU
not against the set freq but against the requested freq reported in sysfs.

v2: Remove previously added delays. GuC FW is now updated to set min/max
    freq in top half so delays are not needed
v3: Increase tolerance between measured and requested freq to 10% to
    account for sporadic failures due to dynamically changing efficient
    freq. Also document the changes in code.

Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6806
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 tests/i915/perf_pmu.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
index f363db2ba13..f9ef89fb0b3 100644
--- a/tests/i915/perf_pmu.c
+++ b/tests/i915/perf_pmu.c
@@ -1546,7 +1546,7 @@ test_interrupts_sync(int gem_fd)
 static void
 test_frequency(int gem_fd)
 {
-	uint32_t min_freq, max_freq, boost_freq;
+	uint32_t min_freq, max_freq, boost_freq, min_req;
 	uint64_t val[2], start[2], slept;
 	double min[2], max[2];
 	igt_spin_t *spin;
@@ -1587,6 +1587,7 @@ test_frequency(int gem_fd)
 
 	min[0] = 1e9*(val[0] - start[0]) / slept;
 	min[1] = 1e9*(val[1] - start[1]) / slept;
+	min_req = igt_sysfs_get_u32(sysfs, "gt_cur_freq_mhz");
 
 	igt_spin_free(gem_fd, spin);
 	gem_quiescent_gpu(gem_fd); /* Don't leak busy bo into the next phase */
@@ -1633,7 +1634,14 @@ test_frequency(int gem_fd)
 	igt_info("Max frequency: requested %.1f, actual %.1f\n",
 		 max[0], max[1]);
 
-	assert_within_epsilon(min[0], min_freq, tolerance);
+	/*
+	 * With GuC SLPC, FW uses requested freq as the efficient freq which can
+	 * exceed the max freq. Therefore compare requested freq measured by the
+	 * PMU not against the set freq's but against the requested freq
+	 * reported in sysfs. Also increase the tolerance a bit to account for
+	 * dynamically changing efficient/requested freq
+	 */
+	assert_within_epsilon(min[0], min_req, 0.1f);
 	/*
 	 * On thermally throttled devices we cannot be sure maximum frequency
 	 * can be reached so use larger tolerance downards.
-- 
2.38.0

[igt-dev] [PATCH i-g-t 0/3] Fix PMU freq verification with SLPC

[Ashutosh Dixit]

After the i915 commit 95ccf312a1e4f ("drm/i915/guc/slpc: Allow SLPC to use
efficient frequency"), FW uses the requested freq as the efficient freq
which can exceed the max freq set. Therefore, in the "min freq" part of the
igt@perf_pmu@frequency subtest, compare the requested freq reported by PMU
not against the set freq but against the requested freq reported in sysfs.

v2: Remove v1 patches which need to be redone. In v2 just add failing tests
    to BAT testlist to try to repro the failures
v3: Add modified v1 patches with minimal changes which are expected to fix
    the two issues
v4: Retest
v5: Increase comparison tolerance and add comments to code explaining
    reason for the changes
v6: Retest
v7: Retest
v8: Repost and Cc reviewers, patches identical to v5

Ashutosh Dixit (3):
  tests/perf_pmu: Compare against requested freq in frequency subtest
  tests/gem_ctx_freq: Compare against requested freq
  HAX: Add gem_ctx_freq@sysfs and perf_pmu@frequency to
    fast-feedback.testlist

 tests/i915/gem_ctx_freq.c             | 24 ++++++++++++++++--------
 tests/i915/perf_pmu.c                 | 12 ++++++++++--
 tests/intel-ci/fast-feedback.testlist |  2 ++
 3 files changed, 28 insertions(+), 10 deletions(-)

-- 
2.38.0

[igt-dev] [PATCH i-g-t 1/3] tests/perf_pmu: Compare against requested freq in frequency subtest

[Ashutosh Dixit]

After the i915 commit 95ccf312a1e4f ("drm/i915/guc/slpc: Allow SLPC to use
efficient frequency"), FW uses the requested freq as the efficient freq
which can exceed the max freq set. Therefore, in the "min freq" part of the
igt@perf_pmu@frequency subtest, compare the requested freq reported by PMU
not against the set freq but against the requested freq reported in sysfs.

v2: Remove previously added delays. GuC FW is now updated to set min/max
    freq in top half so delays are not needed
v3: Increase tolerance between measured and requested freq to 10% to
    account for sporadic failures due to dynamically changing efficient
    freq. Also document the changes in code.

Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6806
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 tests/i915/perf_pmu.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
index f363db2ba13..f9ef89fb0b3 100644
--- a/tests/i915/perf_pmu.c
+++ b/tests/i915/perf_pmu.c
@@ -1546,7 +1546,7 @@ test_interrupts_sync(int gem_fd)
 static void
 test_frequency(int gem_fd)
 {
-	uint32_t min_freq, max_freq, boost_freq;
+	uint32_t min_freq, max_freq, boost_freq, min_req;
 	uint64_t val[2], start[2], slept;
 	double min[2], max[2];
 	igt_spin_t *spin;
@@ -1587,6 +1587,7 @@ test_frequency(int gem_fd)
 
 	min[0] = 1e9*(val[0] - start[0]) / slept;
 	min[1] = 1e9*(val[1] - start[1]) / slept;
+	min_req = igt_sysfs_get_u32(sysfs, "gt_cur_freq_mhz");
 
 	igt_spin_free(gem_fd, spin);
 	gem_quiescent_gpu(gem_fd); /* Don't leak busy bo into the next phase */
@@ -1633,7 +1634,14 @@ test_frequency(int gem_fd)
 	igt_info("Max frequency: requested %.1f, actual %.1f\n",
 		 max[0], max[1]);
 
-	assert_within_epsilon(min[0], min_freq, tolerance);
+	/*
+	 * With GuC SLPC, FW uses requested freq as the efficient freq which can
+	 * exceed the max freq. Therefore compare requested freq measured by the
+	 * PMU not against the set freq's but against the requested freq
+	 * reported in sysfs. Also increase the tolerance a bit to account for
+	 * dynamically changing efficient/requested freq
+	 */
+	assert_within_epsilon(min[0], min_req, 0.1f);
 	/*
 	 * On thermally throttled devices we cannot be sure maximum frequency
 	 * can be reached so use larger tolerance downards.
-- 
2.38.0

[igt-dev] [PATCH i-g-t 2/3] tests/gem_ctx_freq: Compare against requested freq

[Ashutosh Dixit]

After the i915 commit 95ccf312a1e4f ("drm/i915/guc/slpc: Allow SLPC to use
efficient frequency"), FW uses the requested freq as the efficient freq
which can exceed the max freq set. Therefore compare the requested freq
reported by PMU not against the set freq's but against the requested freq
reported in sysfs.

v2: Remove previously added delays. GuC FW is now updated to set min/max
    freq in top half so delays are not needed
v3: Document comparison against requested freq in the code.

Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6786
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 tests/i915/gem_ctx_freq.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/tests/i915/gem_ctx_freq.c b/tests/i915/gem_ctx_freq.c
index a29fe68b72e..9f9ed8cccbf 100644
--- a/tests/i915/gem_ctx_freq.c
+++ b/tests/i915/gem_ctx_freq.c
@@ -110,17 +110,18 @@ static void set_sysfs_freq(uint32_t min, uint32_t max)
 	igt_sysfs_printf(sysfs, "gt_max_freq_mhz", "%u", max);
 }
 
-static bool get_sysfs_freq(uint32_t *min, uint32_t *max)
+static bool get_sysfs_freq(uint32_t *min, uint32_t *max, uint32_t *req)
 {
 	return (igt_sysfs_scanf(sysfs, "gt_min_freq_mhz", "%u", min) == 1 &&
-		igt_sysfs_scanf(sysfs, "gt_max_freq_mhz", "%u", max) == 1);
+		igt_sysfs_scanf(sysfs, "gt_max_freq_mhz", "%u", max) == 1 &&
+		igt_sysfs_scanf(sysfs, "gt_cur_freq_mhz", "%u", req) == 1);
 }
 
 static void sysfs_range(int i915)
 {
 #define N_STEPS 10
 	uint32_t frequencies[TRIANGLE_SIZE(N_STEPS)];
-	uint32_t sys_min, sys_max;
+	uint32_t sys_min, sys_max, req;
 	igt_spin_t *spin;
 	double measured;
 	int pmu;
@@ -133,7 +134,7 @@ static void sysfs_range(int i915)
 	 * constriained sysfs range.
 	 */
 
-	igt_require(get_sysfs_freq(&sys_min, &sys_max));
+	igt_require(get_sysfs_freq(&sys_min, &sys_max, &req));
 	igt_info("System min freq: %dMHz; max freq: %dMHz\n", sys_min, sys_max);
 
 	triangle_fill(frequencies, N_STEPS, sys_min, sys_max);
@@ -150,7 +151,8 @@ static void sysfs_range(int i915)
 		usleep(10000);
 
 		set_sysfs_freq(sys_freq, sys_freq);
-		get_sysfs_freq(&cur, &discard);
+		usleep(10000);
+		get_sysfs_freq(&cur, &discard, &req);
 
 		measured = measure_frequency(pmu, SAMPLE_PERIOD);
 		igt_debugfs_dump(i915, "i915_rps_boost_info");
@@ -158,9 +160,15 @@ static void sysfs_range(int i915)
 		set_sysfs_freq(sys_min, sys_max);
 		__igt_spin_free_idle(i915, spin);
 
-		igt_info("sysfs: Measured %.1fMHz, expected %dMhz\n",
-			 measured, cur);
-		pmu_assert(measured, cur);
+		igt_info("sysfs: Set %dMhz, measured %.1fMHz, expected %dMhz\n",
+			 sys_freq, measured, req);
+		/*
+		 * With GuC SLPC, FW uses requested freq as the efficient freq
+		 * which can exceed the max freq. Therefore compare requested
+		 * freq measured by the PMU not against the set freq's but
+		 * against the requested freq reported in sysfs
+		 */
+		pmu_assert(measured, req);
 	}
 	gem_quiescent_gpu(i915);
 
-- 
2.38.0

[igt-dev] [PATCH i-g-t 3/3] HAX: Add gem_ctx_freq@sysfs and perf_pmu@frequency to fast-feedback.testlist

[Ashutosh Dixit]

CI ONLY, PLEASE DON'T REVIEW

Try to repro GL #6806 nad #6786 because these could not be reproduced
locally. Also, the only DG2 machines these are reproducing on are in
drm-tip CI and not in trybot so sorry, not sending this patch to trybot.

Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6806
Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6786
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 tests/intel-ci/fast-feedback.testlist | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/intel-ci/fast-feedback.testlist b/tests/intel-ci/fast-feedback.testlist
index 4188e97375d..4949f76de77 100644
--- a/tests/intel-ci/fast-feedback.testlist
+++ b/tests/intel-ci/fast-feedback.testlist
@@ -19,6 +19,7 @@ igt@gem_close_race@basic-threads
 igt@gem_ctx_create@basic
 igt@gem_ctx_create@basic-files
 igt@gem_ctx_exec@basic
+igt@gem_ctx_freq@sysfs
 igt@gem_exec_basic@basic
 igt@gem_exec_create@basic
 igt@gem_exec_fence@basic-busy
@@ -128,6 +129,7 @@ igt@i915_pm_backlight@basic-brightness
 igt@i915_pm_rpm@basic-pci-d3-state
 igt@i915_pm_rpm@basic-rte
 igt@i915_pm_rps@basic-api
+igt@perf_pmu@frequency
 igt@prime_self_import@basic-llseek-bad
 igt@prime_self_import@basic-llseek-size
 igt@prime_self_import@basic-with_fd_dup
-- 
2.38.0

[igt-dev] ✗ Fi.CI.BAT: failure for Fix PMU freq verification with SLPC (rev8)

[Patchwork]

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

== Series Details ==

Series: Fix PMU freq verification with SLPC (rev8)
URL   : https://patchwork.freedesktop.org/series/111282/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12569 -> IGTPW_8322
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_8322 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_8322, please notify your bug team 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_8322/index.html

Participating hosts (41 -> 40)
------------------------------

  Additional (1): fi-rkl-11600 
  Missing    (2): fi-kbl-soraka fi-snb-2520m 

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@dmabuf:
    - fi-glk-j4005:       [PASS][1] -> [DMESG-FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12569/fi-glk-j4005/igt@i915_selftest@live@dmabuf.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-glk-j4005/igt@i915_selftest@live@dmabuf.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@basic-hwmon:
    - fi-rkl-11600:       NOTRUN -> [SKIP][3] ([i915#7456])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-rkl-11600/igt@debugfs_test@basic-hwmon.html

  * igt@gem_ctx_freq@sysfs:
    - fi-ilk-650:         NOTRUN -> [SKIP][4] ([fdo#109271]) +1 similar issue
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-ilk-650/igt@gem_ctx_freq@sysfs.html
    - fi-blb-e6850:       NOTRUN -> [SKIP][5] ([fdo#109271]) +1 similar issue
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-blb-e6850/igt@gem_ctx_freq@sysfs.html
    - fi-elk-e7500:       NOTRUN -> [SKIP][6] ([fdo#109271]) +1 similar issue
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-elk-e7500/igt@gem_ctx_freq@sysfs.html
    - fi-pnv-d510:        NOTRUN -> [SKIP][7] ([fdo#109271]) +1 similar issue
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-pnv-d510/igt@gem_ctx_freq@sysfs.html

  * igt@gem_huc_copy@huc-copy:
    - fi-rkl-11600:       NOTRUN -> [SKIP][8] ([i915#2190])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-rkl-11600/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@basic:
    - fi-rkl-11600:       NOTRUN -> [SKIP][9] ([i915#4613]) +3 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-rkl-11600/igt@gem_lmem_swapping@basic.html

  * igt@gem_tiled_pread_basic:
    - fi-rkl-11600:       NOTRUN -> [SKIP][10] ([i915#3282])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-rkl-11600/igt@gem_tiled_pread_basic.html

  * igt@i915_pm_backlight@basic-brightness:
    - fi-rkl-11600:       NOTRUN -> [SKIP][11] ([i915#7561])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-rkl-11600/igt@i915_pm_backlight@basic-brightness.html

  * igt@i915_suspend@basic-s3-without-i915:
    - fi-rkl-11600:       NOTRUN -> [INCOMPLETE][12] ([i915#4817])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_chamelium_hpd@dp-hpd-fast:
    - fi-rkl-11600:       NOTRUN -> [SKIP][13] ([i915#7828]) +7 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-rkl-11600/igt@kms_chamelium_hpd@dp-hpd-fast.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor:
    - fi-rkl-11600:       NOTRUN -> [SKIP][14] ([i915#4103])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-rkl-11600/igt@kms_cursor_legacy@basic-busy-flip-before-cursor.html

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-rkl-11600:       NOTRUN -> [SKIP][15] ([fdo#109285] / [i915#4098])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-rkl-11600/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_psr@primary_page_flip:
    - fi-rkl-11600:       NOTRUN -> [SKIP][16] ([i915#1072]) +3 similar issues
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-rkl-11600/igt@kms_psr@primary_page_flip.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - fi-rkl-11600:       NOTRUN -> [SKIP][17] ([i915#3555] / [i915#4098])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-rkl-11600/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@perf_pmu@frequency:
    - fi-bsw-nick:        NOTRUN -> [SKIP][18] ([fdo#109271])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-bsw-nick/igt@perf_pmu@frequency.html
    - fi-bsw-kefka:       NOTRUN -> [SKIP][19] ([fdo#109271])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-bsw-kefka/igt@perf_pmu@frequency.html
    - fi-bsw-n3050:       NOTRUN -> [SKIP][20] ([fdo#109271])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-bsw-n3050/igt@perf_pmu@frequency.html

  * igt@prime_vgem@basic-read:
    - fi-rkl-11600:       NOTRUN -> [SKIP][21] ([fdo#109295] / [i915#3291] / [i915#3708]) +2 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-rkl-11600/igt@prime_vgem@basic-read.html

  * igt@prime_vgem@basic-userptr:
    - fi-rkl-11600:       NOTRUN -> [SKIP][22] ([fdo#109295] / [i915#3301] / [i915#3708])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-rkl-11600/igt@prime_vgem@basic-userptr.html

  * igt@runner@aborted:
    - fi-glk-j4005:       NOTRUN -> [FAIL][23] ([i915#4312])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/fi-glk-j4005/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_lrc:
    - {bat-adlp-9}:       [INCOMPLETE][24] ([i915#4983]) -> [PASS][25]
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12569/bat-adlp-9/igt@i915_selftest@live@gt_lrc.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/bat-adlp-9/igt@i915_selftest@live@gt_lrc.html

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

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4817]: https://gitlab.freedesktop.org/drm/intel/issues/4817
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#6257]: https://gitlab.freedesktop.org/drm/intel/issues/6257
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6997]: https://gitlab.freedesktop.org/drm/intel/issues/6997
  [i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828


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

  * CI: CI-20190529 -> None
  * IGT: IGT_7115 -> IGTPW_8322

  CI-20190529: 20190529
  CI_DRM_12569: 739b2ee4e76d9cd64e5fbe834b682e550e496cf4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_8322: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8322/index.html
  IGT_7115: c162d70b00c6f4cf6a0ba1ca7a7e2ad8f7190646 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git

== Logs ==

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

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

Re: [igt-dev] [PATCH i-g-t 1/3] tests/perf_pmu: Compare against requested freq in frequency subtest

[Tvrtko Ursulin]

On 10/01/2023 19:47, Ashutosh Dixit wrote:
> After the i915 commit 95ccf312a1e4f ("drm/i915/guc/slpc: Allow SLPC to use
> efficient frequency"), FW uses the requested freq as the efficient freq
> which can exceed the max freq set. Therefore, in the "min freq" part of the
> igt@perf_pmu@frequency subtest, compare the requested freq reported by PMU
> not against the set freq but against the requested freq reported in sysfs.
> 
> v2: Remove previously added delays. GuC FW is now updated to set min/max
>      freq in top half so delays are not needed
> v3: Increase tolerance between measured and requested freq to 10% to
>      account for sporadic failures due to dynamically changing efficient
>      freq. Also document the changes in code.
> 
> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6806
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>   tests/i915/perf_pmu.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
> index f363db2ba13..f9ef89fb0b3 100644
> --- a/tests/i915/perf_pmu.c
> +++ b/tests/i915/perf_pmu.c
> @@ -1546,7 +1546,7 @@ test_interrupts_sync(int gem_fd)
>   static void
>   test_frequency(int gem_fd)
>   {
> -	uint32_t min_freq, max_freq, boost_freq;
> +	uint32_t min_freq, max_freq, boost_freq, min_req;
>   	uint64_t val[2], start[2], slept;
>   	double min[2], max[2];
>   	igt_spin_t *spin;
> @@ -1587,6 +1587,7 @@ test_frequency(int gem_fd)
>   
>   	min[0] = 1e9*(val[0] - start[0]) / slept;
>   	min[1] = 1e9*(val[1] - start[1]) / slept;
> +	min_req = igt_sysfs_get_u32(sysfs, "gt_cur_freq_mhz");

So remove all of the above three igt_sysfs_set_u32 and test still passes 
right? What it is testing then?

Regards,

Tvrtko

>   
>   	igt_spin_free(gem_fd, spin);
>   	gem_quiescent_gpu(gem_fd); /* Don't leak busy bo into the next phase */
> @@ -1633,7 +1634,14 @@ test_frequency(int gem_fd)
>   	igt_info("Max frequency: requested %.1f, actual %.1f\n",
>   		 max[0], max[1]);
>   
> -	assert_within_epsilon(min[0], min_freq, tolerance);
> +	/*
> +	 * With GuC SLPC, FW uses requested freq as the efficient freq which can
> +	 * exceed the max freq. Therefore compare requested freq measured by the
> +	 * PMU not against the set freq's but against the requested freq
> +	 * reported in sysfs. Also increase the tolerance a bit to account for
> +	 * dynamically changing efficient/requested freq
> +	 */
> +	assert_within_epsilon(min[0], min_req, 0.1f);
>   	/*
>   	 * On thermally throttled devices we cannot be sure maximum frequency
>   	 * can be reached so use larger tolerance downards.

Re: [igt-dev] [PATCH i-g-t 1/3] tests/perf_pmu: Compare against requested freq in frequency subtest

[Dixit, Ashutosh]

On Wed, 11 Jan 2023 01:54:59 -0800, Tvrtko Ursulin wrote:
>

Hi Tvrtko,

Sorry I completely missed your reply and only just saw it again. People
needing a recap of the previous discussion can see it here:

https://patchwork.freedesktop.org/patch/512274/?series=110574&rev=3#comment_933447

> On 10/01/2023 19:47, Ashutosh Dixit wrote:
> > After the i915 commit 95ccf312a1e4f ("drm/i915/guc/slpc: Allow SLPC to use
> > efficient frequency"), FW uses the requested freq as the efficient freq
> > which can exceed the max freq set. Therefore, in the "min freq" part of the
> > igt@perf_pmu@frequency subtest, compare the requested freq reported by PMU
> > not against the set freq but against the requested freq reported in sysfs.
> >
> > v2: Remove previously added delays. GuC FW is now updated to set min/max
> >      freq in top half so delays are not needed
> > v3: Increase tolerance between measured and requested freq to 10% to
> >      account for sporadic failures due to dynamically changing efficient
> >      freq. Also document the changes in code.
> >
> > Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6806
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> >   tests/i915/perf_pmu.c | 12 ++++++++++--
> >   1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
> > index f363db2ba13..f9ef89fb0b3 100644
> > --- a/tests/i915/perf_pmu.c
> > +++ b/tests/i915/perf_pmu.c
> > @@ -1546,7 +1546,7 @@ test_interrupts_sync(int gem_fd)
> >   static void
> >   test_frequency(int gem_fd)
> >   {
> > -	uint32_t min_freq, max_freq, boost_freq;
> > +	uint32_t min_freq, max_freq, boost_freq, min_req;
> >	uint64_t val[2], start[2], slept;
> >	double min[2], max[2];
> >	igt_spin_t *spin;
> > @@ -1587,6 +1587,7 @@ test_frequency(int gem_fd)
> >		min[0] = 1e9*(val[0] - start[0]) / slept;
> >	min[1] = 1e9*(val[1] - start[1]) / slept;
> > +	min_req = igt_sysfs_get_u32(sysfs, "gt_cur_freq_mhz");
>
> So remove all of the above three igt_sysfs_set_u32 and test still passes
> right? What it is testing then?

Yes, so since enabling efficient freq (RPe) has broken the kernel ABI was
cannot test that the PMU measured freq is min_freq. All we can do, fwiw, is
test that the PMU measured freq matches the freq exposed via the sysfs
interface (min_req) at this "min point".

I believe what I was saying when we last discussed this was that we can
have two sets of tests:

1. Current tests with RPe enabled
2. Expose a sysfs from i915 to disable RPe and then use that to go to the
   previous versions of the tests here

So these patches are for case 1.

Now about 2., considering that we are moving to the xe driver soon, I am
wondering if there is much ROI in exposing the RPe disabling sysfs from
i915. We might as well do something like that in xe? Or should this still
be done in i915?

In any case, there is interest in closing out these two bugs if possible:

Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6806
Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6786

If we are not going to merge these patches (and assuming we won't change
i915), how about just saying that due to change in the kernel ABI these
tests are no longer valid and therefore blocklisting these tests and
closing the bugs as 'will not fix'?

Thanks.
--
Ashutosh

> >		igt_spin_free(gem_fd, spin);
> >	gem_quiescent_gpu(gem_fd); /* Don't leak busy bo into the next phase */
> > @@ -1633,7 +1634,14 @@ test_frequency(int gem_fd)
> >	igt_info("Max frequency: requested %.1f, actual %.1f\n",
> >		 max[0], max[1]);
> >   -	assert_within_epsilon(min[0], min_freq, tolerance);
> > +	/*
> > +	 * With GuC SLPC, FW uses requested freq as the efficient freq which can
> > +	 * exceed the max freq. Therefore compare requested freq measured by the
> > +	 * PMU not against the set freq's but against the requested freq
> > +	 * reported in sysfs. Also increase the tolerance a bit to account for
> > +	 * dynamically changing efficient/requested freq
> > +	 */
> > +	assert_within_epsilon(min[0], min_req, 0.1f);
> >	/*
> >	 * On thermally throttled devices we cannot be sure maximum frequency
> >	 * can be reached so use larger tolerance downards.

Re: [igt-dev] [PATCH i-g-t 1/3] tests/perf_pmu: Compare against requested freq in frequency subtest

[Tvrtko Ursulin]

On 15/02/2023 04:02, Dixit, Ashutosh wrote:
> On Wed, 11 Jan 2023 01:54:59 -0800, Tvrtko Ursulin wrote:
>>
> 
> Hi Tvrtko,
> 
> Sorry I completely missed your reply and only just saw it again. People
> needing a recap of the previous discussion can see it here:
> 
> https://patchwork.freedesktop.org/patch/512274/?series=110574&rev=3#comment_933447
> 
>> On 10/01/2023 19:47, Ashutosh Dixit wrote:
>>> After the i915 commit 95ccf312a1e4f ("drm/i915/guc/slpc: Allow SLPC to use
>>> efficient frequency"), FW uses the requested freq as the efficient freq
>>> which can exceed the max freq set. Therefore, in the "min freq" part of the
>>> igt@perf_pmu@frequency subtest, compare the requested freq reported by PMU
>>> not against the set freq but against the requested freq reported in sysfs.
>>>
>>> v2: Remove previously added delays. GuC FW is now updated to set min/max
>>>       freq in top half so delays are not needed
>>> v3: Increase tolerance between measured and requested freq to 10% to
>>>       account for sporadic failures due to dynamically changing efficient
>>>       freq. Also document the changes in code.
>>>
>>> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6806
>>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>>> ---
>>>    tests/i915/perf_pmu.c | 12 ++++++++++--
>>>    1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
>>> index f363db2ba13..f9ef89fb0b3 100644
>>> --- a/tests/i915/perf_pmu.c
>>> +++ b/tests/i915/perf_pmu.c
>>> @@ -1546,7 +1546,7 @@ test_interrupts_sync(int gem_fd)
>>>    static void
>>>    test_frequency(int gem_fd)
>>>    {
>>> -	uint32_t min_freq, max_freq, boost_freq;
>>> +	uint32_t min_freq, max_freq, boost_freq, min_req;
>>> 	uint64_t val[2], start[2], slept;
>>> 	double min[2], max[2];
>>> 	igt_spin_t *spin;
>>> @@ -1587,6 +1587,7 @@ test_frequency(int gem_fd)
>>> 		min[0] = 1e9*(val[0] - start[0]) / slept;
>>> 	min[1] = 1e9*(val[1] - start[1]) / slept;
>>> +	min_req = igt_sysfs_get_u32(sysfs, "gt_cur_freq_mhz");
>>
>> So remove all of the above three igt_sysfs_set_u32 and test still passes
>> right? What it is testing then?
> 
> Yes, so since enabling efficient freq (RPe) has broken the kernel ABI was
> cannot test that the PMU measured freq is min_freq. All we can do, fwiw, is
> test that the PMU measured freq matches the freq exposed via the sysfs
> interface (min_req) at this "min point".
> 
> I believe what I was saying when we last discussed this was that we can
> have two sets of tests:
> 
> 1. Current tests with RPe enabled
> 2. Expose a sysfs from i915 to disable RPe and then use that to go to the
>     previous versions of the tests here
> 
> So these patches are for case 1.
> 
> Now about 2., considering that we are moving to the xe driver soon, I am
> wondering if there is much ROI in exposing the RPe disabling sysfs from
> i915. We might as well do something like that in xe? Or should this still
> be done in i915?
> 
> In any case, there is interest in closing out these two bugs if possible:
> 
> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6806
> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6786
> 
> If we are not going to merge these patches (and assuming we won't change
> i915), how about just saying that due to change in the kernel ABI these
> tests are no longer valid and therefore blocklisting these tests and
> closing the bugs as 'will not fix'?

How about we drop any notion of min/max from the test and just check 
that the PMU sees what sysfs sees? Once with idle, once with busy 
(frequency-idle, frequency-busy; via TEST_BUSY/!TEST_BUSY). Would that 
work and be acceptable?

Regards,

Tvrtko

Re: [igt-dev] [PATCH i-g-t 1/3] tests/perf_pmu: Compare against requested freq in frequency subtest

[Tvrtko Ursulin]

On 02/03/2023 13:37, Tvrtko Ursulin wrote:
> 
> On 15/02/2023 04:02, Dixit, Ashutosh wrote:
>> On Wed, 11 Jan 2023 01:54:59 -0800, Tvrtko Ursulin wrote:
>>>
>>
>> Hi Tvrtko,
>>
>> Sorry I completely missed your reply and only just saw it again. People
>> needing a recap of the previous discussion can see it here:
>>
>> https://patchwork.freedesktop.org/patch/512274/?series=110574&rev=3#comment_933447
>>
>>> On 10/01/2023 19:47, Ashutosh Dixit wrote:
>>>> After the i915 commit 95ccf312a1e4f ("drm/i915/guc/slpc: Allow SLPC 
>>>> to use
>>>> efficient frequency"), FW uses the requested freq as the efficient freq
>>>> which can exceed the max freq set. Therefore, in the "min freq" part 
>>>> of the
>>>> igt@perf_pmu@frequency subtest, compare the requested freq reported 
>>>> by PMU
>>>> not against the set freq but against the requested freq reported in 
>>>> sysfs.
>>>>
>>>> v2: Remove previously added delays. GuC FW is now updated to set 
>>>> min/max
>>>>       freq in top half so delays are not needed
>>>> v3: Increase tolerance between measured and requested freq to 10% to
>>>>       account for sporadic failures due to dynamically changing 
>>>> efficient
>>>>       freq. Also document the changes in code.
>>>>
>>>> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6806
>>>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>>>> ---
>>>>    tests/i915/perf_pmu.c | 12 ++++++++++--
>>>>    1 file changed, 10 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
>>>> index f363db2ba13..f9ef89fb0b3 100644
>>>> --- a/tests/i915/perf_pmu.c
>>>> +++ b/tests/i915/perf_pmu.c
>>>> @@ -1546,7 +1546,7 @@ test_interrupts_sync(int gem_fd)
>>>>    static void
>>>>    test_frequency(int gem_fd)
>>>>    {
>>>> -    uint32_t min_freq, max_freq, boost_freq;
>>>> +    uint32_t min_freq, max_freq, boost_freq, min_req;
>>>>     uint64_t val[2], start[2], slept;
>>>>     double min[2], max[2];
>>>>     igt_spin_t *spin;
>>>> @@ -1587,6 +1587,7 @@ test_frequency(int gem_fd)
>>>>         min[0] = 1e9*(val[0] - start[0]) / slept;
>>>>     min[1] = 1e9*(val[1] - start[1]) / slept;
>>>> +    min_req = igt_sysfs_get_u32(sysfs, "gt_cur_freq_mhz");
>>>
>>> So remove all of the above three igt_sysfs_set_u32 and test still passes
>>> right? What it is testing then?
>>
>> Yes, so since enabling efficient freq (RPe) has broken the kernel ABI was
>> cannot test that the PMU measured freq is min_freq. All we can do, 
>> fwiw, is
>> test that the PMU measured freq matches the freq exposed via the sysfs
>> interface (min_req) at this "min point".
>>
>> I believe what I was saying when we last discussed this was that we can
>> have two sets of tests:
>>
>> 1. Current tests with RPe enabled
>> 2. Expose a sysfs from i915 to disable RPe and then use that to go to the
>>     previous versions of the tests here
>>
>> So these patches are for case 1.
>>
>> Now about 2., considering that we are moving to the xe driver soon, I am
>> wondering if there is much ROI in exposing the RPe disabling sysfs from
>> i915. We might as well do something like that in xe? Or should this still
>> be done in i915?
>>
>> In any case, there is interest in closing out these two bugs if possible:
>>
>> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6806
>> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6786
>>
>> If we are not going to merge these patches (and assuming we won't change
>> i915), how about just saying that due to change in the kernel ABI these
>> tests are no longer valid and therefore blocklisting these tests and
>> closing the bugs as 'will not fix'?
> 
> How about we drop any notion of min/max from the test and just check 
> that the PMU sees what sysfs sees? Once with idle, once with busy 
> (frequency-idle, frequency-busy; via TEST_BUSY/!TEST_BUSY). Would that 
> work and be acceptable?

To clarify, my angle here is that perf_pmu is testing PMU and not the 
sysfs frequency control. In a sense any ABI breakage gets swept under 
the carpet which sucks but I see zero willingness to unbreak it. 
Certainly adding more sysfs knobs to work around it shouldn't be the way.

So either remove the test, with a clear admittance of why, or blacklist 
it on GuC platforms in the same way.

Regards,

Tvrtko

Re: [igt-dev] [PATCH i-g-t 1/3] tests/perf_pmu: Compare against requested freq in frequency subtest

[Dixit, Ashutosh]

On Thu, 02 Mar 2023 05:50:36 -0800, Tvrtko Ursulin wrote:
>
> On 02/03/2023 13:37, Tvrtko Ursulin wrote:
> >

Hi Tvrtko,

> > On 15/02/2023 04:02, Dixit, Ashutosh wrote:
> >> On Wed, 11 Jan 2023 01:54:59 -0800, Tvrtko Ursulin wrote:
> >>>
> >> Sorry I completely missed your reply and only just saw it again. People
> >> needing a recap of the previous discussion can see it here:
> >>
> >> https://patchwork.freedesktop.org/patch/512274/?series=110574&rev=3#comment_933447
> >>
> >>> On 10/01/2023 19:47, Ashutosh Dixit wrote:
> >>>> After the i915 commit 95ccf312a1e4f ("drm/i915/guc/slpc: Allow SLPC to
> >>>> use
> >>>> efficient frequency"), FW uses the requested freq as the efficient freq
> >>>> which can exceed the max freq set. Therefore, in the "min freq" part
> >>>> of the
> >>>> igt@perf_pmu@frequency subtest, compare the requested freq reported by
> >>>> PMU
> >>>> not against the set freq but against the requested freq reported in
> >>>> sysfs.
> >>>>
> >>>> v2: Remove previously added delays. GuC FW is now updated to set
> >>>> min/max
> >>>>       freq in top half so delays are not needed
> >>>> v3: Increase tolerance between measured and requested freq to 10% to
> >>>>       account for sporadic failures due to dynamically changing
> >>>> efficient
> >>>>       freq. Also document the changes in code.
> >>>>
> >>>> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6806
> >>>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> >>>> ---
> >>>>    tests/i915/perf_pmu.c | 12 ++++++++++--
> >>>>    1 file changed, 10 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
> >>>> index f363db2ba13..f9ef89fb0b3 100644
> >>>> --- a/tests/i915/perf_pmu.c
> >>>> +++ b/tests/i915/perf_pmu.c
> >>>> @@ -1546,7 +1546,7 @@ test_interrupts_sync(int gem_fd)
> >>>>    static void
> >>>>    test_frequency(int gem_fd)
> >>>>    {
> >>>> -    uint32_t min_freq, max_freq, boost_freq;
> >>>> +    uint32_t min_freq, max_freq, boost_freq, min_req;
> >>>>     uint64_t val[2], start[2], slept;
> >>>>     double min[2], max[2];
> >>>>     igt_spin_t *spin;
> >>>> @@ -1587,6 +1587,7 @@ test_frequency(int gem_fd)
> >>>>         min[0] = 1e9*(val[0] - start[0]) / slept;
> >>>>     min[1] = 1e9*(val[1] - start[1]) / slept;
> >>>> +    min_req = igt_sysfs_get_u32(sysfs, "gt_cur_freq_mhz");
> >>>
> >>> So remove all of the above three igt_sysfs_set_u32 and test still passes
> >>> right? What it is testing then?
> >>
> >> Yes, so since enabling efficient freq (RPe) has broken the kernel ABI was
> >> cannot test that the PMU measured freq is min_freq. All we can do, fwiw,
> >> is
> >> test that the PMU measured freq matches the freq exposed via the sysfs
> >> interface (min_req) at this "min point".
> >>
> >> I believe what I was saying when we last discussed this was that we can
> >> have two sets of tests:
> >>
> >> 1. Current tests with RPe enabled
> >> 2. Expose a sysfs from i915 to disable RPe and then use that to go to the
> >>     previous versions of the tests here
> >>
> >> So these patches are for case 1.
> >>
> >> Now about 2., considering that we are moving to the xe driver soon, I am
> >> wondering if there is much ROI in exposing the RPe disabling sysfs from
> >> i915. We might as well do something like that in xe? Or should this still
> >> be done in i915?
> >>
> >> In any case, there is interest in closing out these two bugs if possible:
> >>
> >> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6806
> >> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6786
> >>
> >> If we are not going to merge these patches (and assuming we won't change
> >> i915), how about just saying that due to change in the kernel ABI these
> >> tests are no longer valid and therefore blocklisting these tests and
> >> closing the bugs as 'will not fix'?
> >
> > How about we drop any notion of min/max from the test and just check that
> > the PMU sees what sysfs sees?

Yes if this is acceptable this would be great.

> > Once with idle, once with busy (frequency-idle, frequency-busy; via
> > TEST_BUSY/!TEST_BUSY). Would that work and be acceptable?

"frequency-idle" is already there, so I can try coming up with a
"frequency-busy" without a notion of min/max.

> To clarify, my angle here is that perf_pmu is testing PMU and not the sysfs
> frequency control.

Hmm, but if there is no kernel ABI they would have to be compared against
each other.

> In a sense any ABI breakage gets swept under the carpet
> which sucks but I see zero willingness to unbreak it. Certainly adding more
> sysfs knobs to work around it shouldn't be the way.

I was tending this way but won't if you think it's not fruitful. Will save
some work.

> So either remove the test, with a clear admittance of why, or blacklist it
> on GuC platforms in the same way.

So most thinking of:

* Skip the "frequency" PMU subtest on GuC platforms
* Add a "frequency-busy"

Thanks.
--
Ashutosh

Re: [igt-dev] [PATCH i-g-t 1/3] tests/perf_pmu: Compare against requested freq in frequency subtest

[Tvrtko Ursulin]

On 03/03/2023 03:04, Dixit, Ashutosh wrote:
> On Thu, 02 Mar 2023 05:50:36 -0800, Tvrtko Ursulin wrote:
>>
>> On 02/03/2023 13:37, Tvrtko Ursulin wrote:
>>>
> 
> Hi Tvrtko,
> 
>>> On 15/02/2023 04:02, Dixit, Ashutosh wrote:
>>>> On Wed, 11 Jan 2023 01:54:59 -0800, Tvrtko Ursulin wrote:
>>>>>
>>>> Sorry I completely missed your reply and only just saw it again. People
>>>> needing a recap of the previous discussion can see it here:
>>>>
>>>> https://patchwork.freedesktop.org/patch/512274/?series=110574&rev=3#comment_933447
>>>>
>>>>> On 10/01/2023 19:47, Ashutosh Dixit wrote:
>>>>>> After the i915 commit 95ccf312a1e4f ("drm/i915/guc/slpc: Allow SLPC to
>>>>>> use
>>>>>> efficient frequency"), FW uses the requested freq as the efficient freq
>>>>>> which can exceed the max freq set. Therefore, in the "min freq" part
>>>>>> of the
>>>>>> igt@perf_pmu@frequency subtest, compare the requested freq reported by
>>>>>> PMU
>>>>>> not against the set freq but against the requested freq reported in
>>>>>> sysfs.
>>>>>>
>>>>>> v2: Remove previously added delays. GuC FW is now updated to set
>>>>>> min/max
>>>>>>        freq in top half so delays are not needed
>>>>>> v3: Increase tolerance between measured and requested freq to 10% to
>>>>>>        account for sporadic failures due to dynamically changing
>>>>>> efficient
>>>>>>        freq. Also document the changes in code.
>>>>>>
>>>>>> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6806
>>>>>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>>>>>> ---
>>>>>>     tests/i915/perf_pmu.c | 12 ++++++++++--
>>>>>>     1 file changed, 10 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
>>>>>> index f363db2ba13..f9ef89fb0b3 100644
>>>>>> --- a/tests/i915/perf_pmu.c
>>>>>> +++ b/tests/i915/perf_pmu.c
>>>>>> @@ -1546,7 +1546,7 @@ test_interrupts_sync(int gem_fd)
>>>>>>     static void
>>>>>>     test_frequency(int gem_fd)
>>>>>>     {
>>>>>> -    uint32_t min_freq, max_freq, boost_freq;
>>>>>> +    uint32_t min_freq, max_freq, boost_freq, min_req;
>>>>>>      uint64_t val[2], start[2], slept;
>>>>>>      double min[2], max[2];
>>>>>>      igt_spin_t *spin;
>>>>>> @@ -1587,6 +1587,7 @@ test_frequency(int gem_fd)
>>>>>>          min[0] = 1e9*(val[0] - start[0]) / slept;
>>>>>>      min[1] = 1e9*(val[1] - start[1]) / slept;
>>>>>> +    min_req = igt_sysfs_get_u32(sysfs, "gt_cur_freq_mhz");
>>>>>
>>>>> So remove all of the above three igt_sysfs_set_u32 and test still passes
>>>>> right? What it is testing then?
>>>>
>>>> Yes, so since enabling efficient freq (RPe) has broken the kernel ABI was
>>>> cannot test that the PMU measured freq is min_freq. All we can do, fwiw,
>>>> is
>>>> test that the PMU measured freq matches the freq exposed via the sysfs
>>>> interface (min_req) at this "min point".
>>>>
>>>> I believe what I was saying when we last discussed this was that we can
>>>> have two sets of tests:
>>>>
>>>> 1. Current tests with RPe enabled
>>>> 2. Expose a sysfs from i915 to disable RPe and then use that to go to the
>>>>      previous versions of the tests here
>>>>
>>>> So these patches are for case 1.
>>>>
>>>> Now about 2., considering that we are moving to the xe driver soon, I am
>>>> wondering if there is much ROI in exposing the RPe disabling sysfs from
>>>> i915. We might as well do something like that in xe? Or should this still
>>>> be done in i915?
>>>>
>>>> In any case, there is interest in closing out these two bugs if possible:
>>>>
>>>> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6806
>>>> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6786
>>>>
>>>> If we are not going to merge these patches (and assuming we won't change
>>>> i915), how about just saying that due to change in the kernel ABI these
>>>> tests are no longer valid and therefore blocklisting these tests and
>>>> closing the bugs as 'will not fix'?
>>>
>>> How about we drop any notion of min/max from the test and just check that
>>> the PMU sees what sysfs sees?
> 
> Yes if this is acceptable this would be great.

You can decide, I resigned due what I wrote - no apparent desire to make 
sysfs min/max actually be respected. IMO better than leaving effectively 
dead code in the test.

>>> Once with idle, once with busy (frequency-idle, frequency-busy; via
>>> TEST_BUSY/!TEST_BUSY). Would that work and be acceptable?
> 
> "frequency-idle" is already there, so I can try coming up with a
> "frequency-busy" without a notion of min/max.
> 
>> To clarify, my angle here is that perf_pmu is testing PMU and not the sysfs
>> frequency control.
> 
> Hmm, but if there is no kernel ABI they would have to be compared against
> each other.

Yes that's what I meant. Don't set min/max, don't even read them, just 
check PMU reports the same as the sysfs actual frequency.

>> In a sense any ABI breakage gets swept under the carpet
>> which sucks but I see zero willingness to unbreak it. Certainly adding more
>> sysfs knobs to work around it shouldn't be the way.
> 
> I was tending this way but won't if you think it's not fruitful. Will save
> some work.

I don't see how a new sysfs control is justified just for the test. 
Also, if it is technically possible to disable RPe then why not disable 
it if user touched the existing control. That would preserve the ABI of 
existing controls, no?

>> So either remove the test, with a clear admittance of why, or blacklist it
>> on GuC platforms in the same way.
> 
> So most thinking of:
> 
> * Skip the "frequency" PMU subtest on GuC platforms
> * Add a "frequency-busy"

And maybe share the body of frequency-idle with frequency-busy if going 
this route, if it makes sense.

Regards,

Tvrtko