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

["Gautham R. Shenoy" <gautham.shenoy@amd.com>]

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@9elements.com>
> 
> 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);

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