Re: [PATCH] cpufreq/amd-pstate: Refactor max frequency calculation

[Mario Limonciello]

On 12/19/2024 03:21, Naresh Solanki wrote:
> Hi ,
>
> On Thu, 19 Dec 2024 at 00:50, Mario Limonciello
> <mario.limonciello@xxxxxxx> wrote:
> > On 12/18/2024 13:00, Naresh Solanki wrote:
> > > Refactor to calculate max-freq more accurately.
> >
> > Can you add some more detail about what you're finding?
> > What was it before, what is it now, why is it more accurate?
> Sure.
> The previous approach had some roundoff error due to division compute
> when calculating boost ratio.
> Which intern affected max-freq calculation resulting in reporting lower value
> In my Glinda SoC board,
> See below calculations with below values:
> max_perf = 208
> nominal_perf = 100
> normal_freq = 2600
>
> How linux kernel calculates:
> freq = ( (max_perf * 1024 / nominal_perf) * normal_freq) / 1024
> freq = 5405 // Integer arithmatic.
>
> With the changes
> freq = (max_perf * normal_freq) / nominal_perf
> freq = 5408

Ah, thanks!  Please include some of this info in the commit message for 
the next version.

> > > Signed-off-by: Naresh Solanki <naresh.solanki@xxxxxxxxxxxxx>
> > > ---
> > >    drivers/cpufreq/amd-pstate.c | 5 ++---
> > >    1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> > > index d7630bab2516..78a2cbd14952 100644
> > > --- a/drivers/cpufreq/amd-pstate.c
> > > +++ b/drivers/cpufreq/amd-pstate.c
> > > @@ -892,7 +892,7 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
> > >        u64 numerator;
> > >        u32 nominal_perf, nominal_freq;
> > >        u32 lowest_nonlinear_perf, lowest_nonlinear_freq;
> > > -     u32 boost_ratio, lowest_nonlinear_ratio;
> > > +     u32 lowest_nonlinear_ratio;
> > >        struct cppc_perf_caps cppc_perf;
> > >
> > >        ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> > > @@ -914,8 +914,7 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
> > >        ret = amd_get_boost_ratio_numerator(cpudata->cpu, &numerator);
> > >        if (ret)
> > >                return ret;
> > > -     boost_ratio = div_u64(numerator << SCHED_CAPACITY_SHIFT, nominal_perf);
> > > -     max_freq = (nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT) * 1000;
> > > +     max_freq = div_u64(numerator * nominal_freq * 1000, nominal_perf);
> >
> > This doesn't apply currently, because of some changes in the
> > superm1.git/linux-next branch; specifically:
> >
> > https://git.kernel.org/superm1/c/68cb0e77b6439
> >
> > I haven't sent this out to linux-pm yet so it could be in linux-next,
> > but will be doing that soon.  So can you please rebase on that branch if
> > this change still makes sense?
> Sure. Will do

Thanks!

> Regards,
> Naresh Solanki
> > >        lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
> > >        lowest_nonlinear_ratio = div_u64(lowest_nonlinear_perf << SCHED_CAPACITY_SHIFT,