[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 at 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 at 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.