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

[Mario Limonciello]

On 12/26/2024 23:49, Dhananjay Ugwekar wrote:
> On 12/20/2024 11:46 AM, Gautham R. Shenoy wrote:
> > On Fri, Dec 20, 2024 at 12:51:43AM +0530, Naresh Solanki wrote:
> > > The previous approach introduced roundoff errors during division when
> > > calculating the boost ratio. This, in turn, affected the maximum
> > > frequency calculation, often resulting in reporting lower frequency
> > > values.
> > >
> > > For example, on the Glinda SoC based board with the following
> > > parameters:
> > >
> > > max_perf = 208
> > > nominal_perf = 100
> > > nominal_freq = 2600 MHz
> > >
> > > The Linux kernel previously calculated the frequency as:
> > > freq = ((max_perf * 1024 / nominal_perf) * nominal_freq) / 1024
> > > freq = 5405 MHz  // Integer arithmetic.
> > >
> > > With the updated formula:
> > > freq = (max_perf * nominal_freq) / nominal_perf
> > > freq = 5408 MHz
> > >
> > > This change ensures more accurate frequency calculations by eliminating
> > > unnecessary shifts and divisions, thereby improving precision.
> > >
> > > Signed-off-by: Naresh Solanki <naresh.solanki@xxxxxxxxxxxxx>
> > >
> > > Changes in V2:
> > > 1. Rebase on superm1.git/linux-next branch
> > > ---
> > >   drivers/cpufreq/amd-pstate.c | 9 ++++-----
> > >   1 file changed, 4 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> > > index d7b1de97727a..02a851f93fd6 100644
> > > --- a/drivers/cpufreq/amd-pstate.c
> > > +++ b/drivers/cpufreq/amd-pstate.c
> > > @@ -908,9 +908,9 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
> > >   {
> > >   	int ret;
> > >   	u32 min_freq, max_freq;
> > > -	u32 nominal_perf, nominal_freq;
> > > +	u32 highest_perf, 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);
> > > @@ -927,10 +927,9 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
> > >   	else
> > >   		nominal_freq = cppc_perf.nominal_freq;
> > >   
> > > +	highest_perf = READ_ONCE(cpudata->highest_perf);
> > >   	nominal_perf = READ_ONCE(cpudata->nominal_perf);
> > > -
> > > -	boost_ratio = div_u64(cpudata->highest_perf << SCHED_CAPACITY_SHIFT, nominal_perf);
> > > -	max_freq = (nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT);
> >
> >
> > The patch looks obviously correct to me. And the suggested method
> > would work because nominal_freq is larger than the nominal_perf and
> > thus scaling is really necessary.
> >
> > Besides, before this patch, there was another obvious issue that we
> > were computing the boost_ratio when we should have been computing the
> > ratio of nominal_freq and nominal_perf and then multiplied this with
> > max_perf without losing precision.
> >
> > This is just one instance, but it can be generalized so that any
> > freq --> perf and perf --> freq can be computed without loss of precision.
> >
> > We need two things:
> >
> > 1. The mult_factor should be computed as a ratio of nominal_freq and
> > nominal_perf (and vice versa) as they are always known.
> >
> > 2. Use DIV64_U64_ROUND_UP instead of div64() which rounds up instead of rounding down.
> >
> > So if we have the shifts defined as follows:
> >
> > #define PERF_SHIFT   12UL //shift used for freq --> perf conversion
> > #define FREQ_SHIFT   10UL //shift used for perf --> freq conversion.
> >
> > And in amd_pstate_init_freq() code, we initialize the two global variables:
> >
> > u64 freq_mult_factor = DIV64_U64_ROUND_UP(nominal_freq  << FREQ_SHIFT, nominal_perf);
> > u64 perf_mult_factor = DIV64_U64_ROUND_UP(nominal_perf  << PERF_SHIFT, nominal_freq);
>
> I like this approach, but can we assume the nominal freq/perf values to be the same for
> all CPUs, otherwise we would need to make these factors a per-CPU or per-domain(where
> all CPUs within a "domain" have the same nominal_freq/perf). At which point the benefit
> of caching these ratios might diminish.
>
> Thoughts, Gautham, Mario?

No; in this day of heterogeneous designs I don't think that you can make 
that assumption, so yes if we had helpers they would have to apply to a 
group of CPUs, and I agree at that point the caching isn't very 
beneficial anymore.

If the main argument is to make it easier to follow we could have some 
macros though?

> Thanks,
> Dhananjay
>
> > .. and have a couple of helper functions:
> >
> > /* perf to freq conversion */
> > static inline unsigned int perf_to_freq(perf)
> > {
> > 	return (perf * freq_mult_factor) >> FREQ_SHIFT;
> > }
> >
> >
> > /* freq to perf conversion */
> > static inline unsigned int freq_to_perf(freq)
> > {
> > 	return (freq * perf_mult_factor) >> PERF_SHIFT;
> > }
> >
> >
> > > +	max_freq = div_u64((u64)highest_perf * nominal_freq, nominal_perf);
> >
> > Then,
> >          max_freq = perf_to_freq(highest_perf);
> > 	min_freq = perf_to_freq(lowest_non_linear_perf);
> >
> >
> > and so on.
> >
> > This should just work.
> >
> >
> > >   
> > >   	lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
> > >   	lowest_nonlinear_ratio = div_u64(lowest_nonlinear_perf << SCHED_CAPACITY_SHIFT,
> > > --
> >
> > --
> > Thanks and Regards
> > gautham.