Re: [PATCH v2 12/16] cpufreq/amd-pstate: Always write EPP value when updating perf

[Gautham R. Shenoy]

On Mon, Dec 09, 2024 at 11:15:49AM -0600, Mario Limonciello wrote:
> On 12/9/2024 10:49, Mario Limonciello wrote:
> > On 12/9/2024 02:42, Gautham R. Shenoy wrote:
> > > Hello Mario,
> > > 
> > > On Sun, Dec 08, 2024 at 12:30:27AM -0600, Mario Limonciello wrote:
> > > > For MSR systems the EPP value is in the same register as perf targets
> > > > and so divding them into two separate MSR writes is wasteful.
> > > > 
> > > > In msr_update_perf(), update both EPP and perf values in one write to
> > > > MSR_AMD_CPPC_REQ, and cache them if successful.
> > > > 
> > > > To accomplish this plumb the EPP value into the update_perf call
> > > > and modify
> > > > all its callers to check the return value.
> > > > 
> > > > Reviewed-and-tested-by: Dhananjay Ugwekar <dhananjay.ugwekar@xxxxxxx>
> > > > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > > > ---
> > > >   drivers/cpufreq/amd-pstate.c | 71 ++++++++++++++++++++++--------------
> > > >   1 file changed, 43 insertions(+), 28 deletions(-)
> > > > 
> > > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> > > > index d21acd961edcd..dd11ba6c00cc3 100644
> > > > --- a/drivers/cpufreq/amd-pstate.c
> > > > +++ b/drivers/cpufreq/amd-pstate.c
> > > > @@ -222,25 +222,36 @@ static s16 shmem_get_epp(struct
> > > > amd_cpudata *cpudata)
> > > >   }
> > > >   static int msr_update_perf(struct amd_cpudata *cpudata, u32 min_perf,
> > > > -                   u32 des_perf, u32 max_perf, bool fast_switch)
> > > > +               u32 des_perf, u32 max_perf, u32 epp, bool fast_switch)
> > > >   {
> > > > +    u64 value;
> > > > +
> > > > +    value = READ_ONCE(cpudata->cppc_req_cached);
> > > 
> > > 
> > > There seems to be a mismatch here between what the API is passing and
> > > parameters and how this function is *not* using them, and instead
> > > using cpudata->cppc_req_cached.
> > > 
> > > The expectation seems to be that the max_perf, min_perf, des_perf and
> > > epp fields in cpudata->cppc_req_cached would be the same as @des_perf,
> > > @max_perf, @min_perf and @ep, no ?
> > > 
> > > Or is it that for the MSR update, the value in
> > > cpudata->cppc_req_cached take precedence over the arguments passed ?
> > > 
> > > Ideally, the "value" should be recomputed here using (@min_perf |
> > > @max_perf | @des_perf | @epp) and that value should be cached as you
> > > are doing below.
> > > 
> > 
> > Yeah - that's what the next patch does (which I think you probably saw
> > after you reviewed it).
> > 
> > Do you think maybe I should just squash the two?  Or would you be
> > happier if I re-ordered the two?
> 
> FYI - I looked into re-ordering and it's not feasible because you need EPP
> plumbed in order to validate the result.
> 
> So I'm going to squash the two patches, and I'll do another one that adjusts
> tracing locations for your other feedback.

Yeah, it is not possible to reorder the two. I like the newer version
better.


--
Thanks and Regards
gautham.