[igt-dev] [PATCH i-g-t] tests/perf_pmu: Compare against requested freq in frequency subtest
Rodrigo Vivi
rodrigo.vivi at intel.com
Fri Dec 23 15:39:29 UTC 2022
On Fri, Dec 23, 2022 at 09:22:53AM +0000, Tvrtko Ursulin wrote:
>
> On 22/12/2022 20:28, Rodrigo Vivi wrote:
> > On Mon, Dec 19, 2022 at 08:46:59AM +0000, Tvrtko Ursulin wrote:
> > >
> > > On 17/12/2022 02:49, Dixit, Ashutosh wrote:
> > > > On Fri, 16 Dec 2022 07:39:52 -0800, Rodrigo Vivi wrote:
> > > > > On Fri, Dec 16, 2022 at 09:37:31AM +0000, Tvrtko Ursulin wrote:
> > > > > >
> > > > > > On 16/12/2022 06:21, Dixit, Ashutosh wrote:
> > > > > > > On Thu, 24 Nov 2022 04:42:08 -0800, Tvrtko Ursulin wrote:
> > > > > > > >
> > > > > > > > On 23/11/2022 06:03, Dixit, Ashutosh wrote:
> > > > > > > > > On Mon, 21 Nov 2022 01:09:55 -0800, Tvrtko Ursulin wrote:
> > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > > > > Hi Tvrtko,
> > > > > > >
> > > > > > > Sorry for the delay in replying on this. We are discussing this but still
> > > > > > > no conclusion. I have replied what I can below, will update again after I
> > > > > > > know more. Wanted to send a reply before disappearing for the
> > > > > > > holidays. Please see below.
> > > > > > >
> > > > > > > > > Hi Tvrtko,
> > > > > > > > >
> > > > > > > > > I am only offering an overall clarification below first, not answering all
> > > > > > > > > of your points for now.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On 19/11/2022 02:00, Ashutosh Dixit wrote:
> > > > > > > > > > > With SLPC, even when we set the same min and max freq's, the requested and
> > > > > > > > > > > actual freq's can differ from the min/max freq set. For example "efficient
> > > > > > > > > > > freq" (when in effect) can override set min/max freq. In general FW is the
> > > > > > > > > > > final arbiter in determining freq and can override set values.
> > > > > > > > > > >
> > > > > > > > > > > Therefore in igt at perf_pmu@frequency subtest, compare the requested freq
> > > > > > > > > > > reported by PMU not against the set freq's but against the requested freq
> > > > > > > > > > > reported in sysfs. Also add a delay after setting freq's to account for
> > > > > > > > > > > messaging delays in setting freq's in GuC.
> > > > > > > > > > >
> > > > > > > > > > > v2: Introduce a 100 ms delay after setting freq
> > > > > > > > > > > v3: Update commit message, code identical to v2
> > > > > > > > > > >
> > > > > > > > > > > Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/6806
> > > > > > > > > > > Cc: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
> > > > > > > > > > > Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> > > > > > > > > > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > > > > > > > > > > ---
> > > > > > > > > > > tests/i915/perf_pmu.c | 23 +++++++++++++++--------
> > > > > > > > > > > 1 file changed, 15 insertions(+), 8 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
> > > > > > > > > > > index f363db2ba13..02f6ae989b1 100644
> > > > > > > > > > > --- a/tests/i915/perf_pmu.c
> > > > > > > > > > > +++ b/tests/i915/perf_pmu.c
> > > > > > > > > > > @@ -1543,10 +1543,13 @@ test_interrupts_sync(int gem_fd)
> > > > > > > > > > > igt_assert_lte(target, busy);
> > > > > > > > > > > }
> > > > > > > > > > > +/* Wait for GuC SLPC freq changes to take effect */
> > > > > > > > > > > +#define wait_freq_set() usleep(100000)
> > > > > > > > > >
> > > > > > > > > > A future task of maybe adding a drop_caches flag to wait for this?
> > > > > > > > > >
> > > > > > > > > > Alternatively, if we have a sysfs or debugfs file which reflects the value
> > > > > > > > > > once GuC has processed it, keep reading it until it is updated? Even if to
> > > > > > > > > > something other than it was before the write, to handle the inability to
> > > > > > > > > > set the exact requested frequency sometimes?
> > > > > > > > > > >
> > > > > > > > > > > +
> > > > > > > > > > > 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;
> > > > > > > > > > > @@ -1572,10 +1575,11 @@ test_frequency(int gem_fd)
> > > > > > > > > > > * 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));
> > > > > > > > > > > + wait_freq_set();
> > > > > > > > > > > + igt_require(igt_sysfs_get_u32(sysfs, "gt_min_freq_mhz") == min_freq);
> > > > > > > > > > > + igt_require(igt_sysfs_get_u32(sysfs, "gt_max_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 */
> > > > > > > > > > > @@ -1587,6 +1591,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");
> > > > > > > > > >
> > > > > > > > > > This really leaves a bad taste for me still, sorry. First of all, it may
> > > > > > > > > > make it more palatable if readback was immediately after wait_freq_set(),
> > > > > > > > > > or as part of it. Otherwise it seems like even more confused test and sysfs
> > > > > > > > > > ABI.
> > > > > > > > > >
> > > > > > > > > > Did we close on my question of whether we can make readback of
> > > > > > > > > > gt_max_freq_mhz actually represent the real max? Correct me please if I got
> > > > > > > > > > confused - with min freq checking here we write max=300 and get 350? If so
> > > > > > > > > > can we make gt_max_freq_mhz return 350 after the write of 300? Or return an
> > > > > > > > > > error?
> > > > > > > > >
> > > > > > > > > Did you read Vinay's email on v1 here:
> > > > > > > > >
> > > > > > > > > https://patchwork.freedesktop.org/patch/510304/?series=110574&rev=1#comment_921571
> > > > > > > > >
> > > > > > > > > As Vinay says, SLPC is requesting an entirely different freq (the effecient
> > > > > > > > > freq rather than the set 'min == max freq') and no amount of waiting will
> > > > > > > > > result in the requested freq being the same as the set 'min == max
> > > > > > > > > freq'. Also the efficient freq cannot be controlled from user space.
> > > > > > > >
> > > > > > > > Why is that not a break in our ABI? What is the point in allowing users to
> > > > > > > > write into this field if the desired value can be silently ignored?
> > > > > > >
> > > > > > > Yes, this is in discussion. FW is doing this and we need to see if FW can
> > > > > > > be made to obey the i915 ABI. IMHO what matters is performance per watt and
> > > > > > > if the ABI has to be tossed so be it. I will keep this list posted about
> > > > > > > what happens here. Merging any patches for these issues (GL #6806 and
> > > > > > > #6786) is blocked till the ABI breakage issue is resolved one way or
> > > > > > > another.
> > > > > >
> > > > > > Two thoughts here, one has been mentioned before.
> > > > > >
> > > > > > New one is that performance per watt perhaps does not matter (is not a
> > > > > > primary concern at least) for use cases where "users" (for some definition)
> > > > > > tweak the min/max in sysfs. Presumably in those cases user wants control,
> > > > > > for some reason, so as long as it is safe and within the limits why not give
> > > > > > it to them.
> > > > > >
> > > > > > Same argument from a different angle - what is the use case for allowing
> > > > > > min/max control which does not really do what it says on the tin?
> > > > > >
> > > > > > Old thought is this - if SLPC refuses to be controlled using our existing
> > > > > > controls then we could just say -ENODEV on attempts to modify them. Whether
> > > > > > for any input value, or some, depending on what SLPC can support (or wants
> > > > > > to support).
> > > > >
> > > > > The problem is not just SLPC. The freq is ultimately decided by PCODE and it
> > > > > has sudo and all the rights to disrespect what drivers (including SLPC as a
> > > > > a driver from PCODE perspective) are requesting.
> > > > >
> > > > > >
> > > > > > Important to note I am primarily focusing on the min freq here, which AFAIR
> > > > > > we concluded can behave that user writes in 300, but SLPC decides it will
> > > > > > only go as low as 350.
> > > > >
> > > > > The other caveat here with the efficient freq is that it is dynamic and PCODE
> > > > > decides it with runtime conditions. It may start with 300, then move to 350,
> > > > > then back to 300. i915 is only stashing the initial value.
> > > > >
> > > > > We could potentially update our rpe everytime any freq control is
> > > > > touched.
> > > >
> > > > We thought of doing this but two points:
> > > >
> > > > 1. i915 now reads the same register as GuC for rpe but I never see the
> > > > value in this register change.
> > > > 2. Even if we could know the real rpe, the freq's might be set before a
> > > > workload start running and rpe might change after the workload starts
> > > > running. So knowing rpe before the workload might not be very useful.
> > > >
> > > > I am of the opposite opinion that anyway the previous kernel ABI is
> > > > probably now irrecoverably bust, so we should give FW free reign and stop
> > > > disabling efficient freq the way we do now when min_freq < rpe_freq.
> > >
> > > We have to answer the question of why we need to keep exposing the sysfs
> > > controls if they don't/can't/won't do what they say on the tin. Could we
> > > make a break with DG2 for instance? Some use cases might suffer but it's
> > > probably better than having them think it does what they think it does, or
> > > file bugs when it does not.
> >
> > That's a good question. But if we have no control at all people will still
> > file requests for something our hardware and low level firmwares cannot promise.
> >
> > There are many folks who run important/critical experiments using these
> > interfaces at the same time they are monitoring throttle conditions and all.
>
> Aren't the two paragraphs a bit in contradiction? Important and critical
> experiments using controls which are not doing what they think they are.
Well, not really.
Most of the times it will work. We just don't have a warranty.
We have some indications when this were disrespected like the throttle_reasons.
Those limited times doesn't exclude the importance of the usage.
>
> I don't claim I know for what all use cases do they get used. I *think*
> there was one in the media transcoding world one where people wanted truly
> fixed frequency so they can compare like-for-like between something.
>
> So bah.. but okay for the perf_pmu changes, how about instead of this:
>
> rpN = read RPn from sysfs
> set_min_freq(rpN)
> sleep
> real_min = read actual from sysfs
> actual = sample pmu for t
> assert real_min is actual
>
> We do this:
>
> rpN = read RPn from sysfs
> set_min_freq(rpN)
> sleep
> fw_min = read actual from sysfs
> set_min_freq(fw_min)
> sleep
> actual = sample pmu for t
> assert fw_min is actual
>
> That's a tiny difference which both "documents" and verifies that even if we
> cannot set the min to RPn, firmware will somewhat consistently (stable)
> respect the minimum it prefers.
>
> Is that somewhat acceptable?
yeap, this sounds a better approach for a sane test.
>
> Regards,
>
> Tvrtko
More information about the igt-dev
mailing list