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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Mon, 07 Nov 2022 02:27:16 -0800, Tvrtko Ursulin wrote:
>

Hi Tvrtko,

> On 07/11/2022 06:23, Ashutosh Dixit wrote:
> > In 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. If requested freq differs from the set freq (even when min == max
> > freq) for whatever reason, the sysfs requested freq should be closer to the
> > PMU measured requested freq which should minimize sporadic failures.
> >
> > Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6806
>
> So this bug describes the failure looks like this:
>
> Starting subtest: frequency
> (perf_pmu:1798) CRITICAL: Test assertion failure function test_frequency, file ../tests/i915/perf_pmu.c:1636:
> (perf_pmu:1798) CRITICAL: Failed assertion: (double)(min[0]) <= (1.0 + (tolerance)) * (double)(min_freq) && (double)(min[0]) >= (1.0 - (tolerance)) * (double)(min_freq)
> (perf_pmu:1798) CRITICAL: 'min[0]' != 'min_freq' (347.914580 not within +5.0%/-5.0% tolerance of 300.000000)
> Subtest frequency failed.
>
> Which is this in code:
>
>
>	min_freq = igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
> ...
>	/*
>	 * Set GPU to min frequency and read PMU counters.
>	 */
>	igt_require(igt_sysfs_set_u32(sysfs, "gt_min_freq_mhz", min_freq));
>	igt_require(igt_sysfs_get_u32(sysfs, "gt_min_freq_mhz") == min_freq);
>	igt_require(igt_sysfs_set_u32(sysfs, "gt_max_freq_mhz", min_freq));
>	igt_require(igt_sysfs_get_u32(sysfs, "gt_max_freq_mhz") == min_freq);
>	igt_require(igt_sysfs_set_u32(sysfs, "gt_boost_freq_mhz", min_freq));
>	igt_require(igt_sysfs_get_u32(sysfs, "gt_boost_freq_mhz") == min_freq);
> ...
>	gem_quiescent_gpu(gem_fd); /* Idle to be sure the change takes effect */
>	spin = spin_sync_flags(gem_fd, ahnd, 0, I915_EXEC_DEFAULT);
>
>	slept = pmu_read_multi(fd[0], 2, start);
>	measured_usleep(batch_duration_ns / 1000);
>	slept = pmu_read_multi(fd[0], 2, val) - slept;
>
>	min[0] = 1e9*(val[0] - start[0]) / slept;
>	min[1] = 1e9*(val[1] - start[1]) / slept;
> ...
>	/*
>	 * Restore min/max.
>	 */
>	igt_sysfs_set_u32(sysfs, "gt_min_freq_mhz", min_freq);
>	if (igt_sysfs_get_u32(sysfs, "gt_min_freq_mhz") != min_freq)
>		igt_warn("Unable to restore min frequency to saved value [%u MHz], now %u MHz\n",
>			 min_freq, igt_sysfs_get_u32(sysfs, "gt_min_freq_mhz"));
>	close(fd[0]);
>	close(fd[1]);
>	put_ahnd(ahnd);
>
>	igt_info("Min frequency: requested %.1f, actual %.1f\n",
>		 min[0], min[1]);
>	igt_info("Max frequency: requested %.1f, actual %.1f\n",
>		 max[0], max[1]);
>
>	assert_within_epsilon(min[0], min_freq, tolerance);
>
> Which means sysfs told the test which is the minimum frequency. But it
> really isn't - it turns out 350 MHz is the real minimum? Or setting of
> gt_max_freq_mhz/gt_boost_freq_mhz is not actually respected by SLPC? It
> sounds trivial to respect the minimum frequency so I don't see what is
> the excuse.

So I submitted this patch to repro the issue and to print out the requested
freq from sysfs:

https://patchwork.freedesktop.org/series/110630/

And we can see the output here:

https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8061/bat-dg2-11/igt@perf_pmu@frequency.html

```
IGT-Version: 1.26-g1bef4d081 (x86_64) (Linux: 6.1.0-rc4-CI_DRM_12352-gc55ac6a74bd1+ x86_64)
Starting subtest: frequency
Frequency: min=300, max=2050, boost=2050 MHz
Min frequency: requested 349.7, actual 349.7
Max frequency: requested 2048.0, actual 2048.0
Sysfs requested: min 350, max 2050
Stack trace:
  #0 ../../../usr/src/igt-gpu-tools/lib/igt_core.c:1908 __igt_fail_assert()
  #1 ../../../usr/src/igt-gpu-tools/tests/i915/perf_pmu.c:1656 __igt_unique____real_main2147()
  #2 ../../../usr/src/igt-gpu-tools/tests/i915/perf_pmu.c:2147 main()
  #3 [__libc_start_main+0xf3]
  #4 [_start+0x2e]
Subtest frequency: FAIL (2.212s)
```

So we clearly see the requested freq from sysfs is indeed 350 MHz so
SLPC/PCODE is not honoring the set min == max == boost freq (and PMU is
measuring what sysfs is showing). In general PCODE is the final arbiter in
such cases and we do occasionally see instances where set freq limits are
not honored.

I would say if igt@perf_pmu@frequency is testing freq measured by PMU then
the patch below is correct. Whether SLPC/PCODE is honoring the set freq
limits should be tested in a SLPC test (which we also have).

Thanks.
--
Ashutosh


>
> Regards,
>
> Tvrtko
>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> >   tests/i915/perf_pmu.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
> > index f363db2ba13..42b5b7bd6f2 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, max_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 */
> > @@ -1613,6 +1614,7 @@ test_frequency(int gem_fd)
> >		max[0] = 1e9*(val[0] - start[0]) / slept;
> >	max[1] = 1e9*(val[1] - start[1]) / slept;
> > +	max_req = igt_sysfs_get_u32(sysfs, "gt_cur_freq_mhz");
> >		igt_spin_free(gem_fd, spin);
> >	gem_quiescent_gpu(gem_fd);
> > @@ -1633,12 +1635,12 @@ 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.
> >	 */
> > -	__assert_within_epsilon(max[0], max_freq, tolerance, 0.15f);
> > +	__assert_within_epsilon(max[0], max_req, tolerance, 0.15f);
> >   }
> >     static void