Re: [PATCH 6/8] cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() into amd_get_boost_ratio_numerator()

[Mario Limonciello <mario.limonciello@xxxxxxx>]

On 8/27/2024 11:52, Gautham R. Shenoy wrote:
> Hello Mario,
>
>
> On Mon, Aug 26, 2024 at 04:13:56PM -0500, Mario Limonciello wrote:
> > From: Mario Limonciello <mario.limonciello@xxxxxxx>
> >
> > The special case in amd_pstate_highest_perf_set() is the value used
> > for calculating the boost numerator.  Merge this into
> > amd_get_boost_ratio_numerator() and then use that to calculate boost
> > ratio.
> >
> > This allows dropping more special casing of the highest perf value.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > ---
> >   arch/x86/kernel/acpi/cppc.c  | 16 ++++++++++++
> >   drivers/cpufreq/amd-pstate.c | 49 +++++++-----------------------------
> >   2 files changed, 25 insertions(+), 40 deletions(-)
> >
> > diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
> > index 729b35e84f5eb..44b13a4e28740 100644
> > --- a/arch/x86/kernel/acpi/cppc.c
> > +++ b/arch/x86/kernel/acpi/cppc.c
> > @@ -9,6 +9,7 @@
> >   #include <asm/processor.h>
> >   #include <asm/topology.h>
> >   
> > +#define CPPC_HIGHEST_PERF_PERFORMANCE	196
> >   #define CPPC_HIGHEST_PERF_PREFCORE	166
> >   
> >   enum amd_pref_core {
> > @@ -244,6 +245,21 @@ int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)
> >   		*numerator = boost_numerator;
> >   		return 0;
> >   	}
> > +
> > +	/*
> > +	 * For AMD CPUs with Family ID 19H and Model ID range 0x70 to 0x7f,
> > +	 * the highest performance level is set to 196.
> > +	 * https://bugzilla.kernel.org/show_bug.cgi?id=218759
> > +	 */
> > +	if (cpu_feature_enabled(X86_FEATURE_ZEN4)) {
> > +		switch (boot_cpu_data.x86_model) {
> > +		case 0x70 ... 0x7f:
> > +			*numerator = CPPC_HIGHEST_PERF_PERFORMANCE;
> > +			return 0;
> > +		default:
> > +			break;
> > +		}
> > +	}
> >   	*numerator = CPPC_HIGHEST_PERF_PREFCORE;
> >   
> >   	return 0;
> > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> > index ec32c830abc1d..75568d0f84623 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -52,8 +52,6 @@
> >   #define AMD_PSTATE_TRANSITION_LATENCY	20000
> >   #define AMD_PSTATE_TRANSITION_DELAY	1000
> >   #define AMD_PSTATE_FAST_CPPC_TRANSITION_DELAY 600
> > -#define CPPC_HIGHEST_PERF_PERFORMANCE	196
> > -#define CPPC_HIGHEST_PERF_DEFAULT	166
> >   
> >   #define AMD_CPPC_EPP_PERFORMANCE		0x00
> >   #define AMD_CPPC_EPP_BALANCE_PERFORMANCE	0x80
> > @@ -372,43 +370,17 @@ static inline int amd_pstate_enable(bool enable)
> >   	return static_call(amd_pstate_enable)(enable);
> >   }
> >   
> > -static u32 amd_pstate_highest_perf_set(struct amd_cpudata *cpudata)
> > -{
> > -	struct cpuinfo_x86 *c = &cpu_data(0);
> > -
> > -	/*
> > -	 * For AMD CPUs with Family ID 19H and Model ID range 0x70 to 0x7f,
> > -	 * the highest performance level is set to 196.
> > -	 * https://bugzilla.kernel.org/show_bug.cgi?id=218759
> > -	 */
> > -	if (c->x86 == 0x19 && (c->x86_model >= 0x70 && c->x86_model <= 0x7f))
> > -		return CPPC_HIGHEST_PERF_PERFORMANCE;
> > -
> > -	return CPPC_HIGHEST_PERF_DEFAULT;
> > -}
> > -
> >   static int pstate_init_perf(struct amd_cpudata *cpudata)
> >   {
> >   	u64 cap1;
> > -	u32 highest_perf;
> >   
> >   	int ret = rdmsrl_safe_on_cpu(cpudata->cpu, MSR_AMD_CPPC_CAP1,
> >   				     &cap1);
> >   	if (ret)
> >   		return ret;
> >   
> > -	/* For platforms that do not support the preferred core feature, the
> > -	 * highest_pef may be configured with 166 or 255, to avoid max frequency
> > -	 * calculated wrongly. we take the AMD_CPPC_HIGHEST_PERF(cap1) value as
> > -	 * the default max perf.
> > -	 */
> > -	if (cpudata->hw_prefcore)
> > -		highest_perf = amd_pstate_highest_perf_set(cpudata);
> > -	else
> > -		highest_perf = AMD_CPPC_HIGHEST_PERF(cap1);
> > -
> > -	WRITE_ONCE(cpudata->highest_perf, highest_perf);
> > -	WRITE_ONCE(cpudata->max_limit_perf, highest_perf);
> > +	WRITE_ONCE(cpudata->highest_perf, AMD_CPPC_HIGHEST_PERF(cap1));
> > +	WRITE_ONCE(cpudata->max_limit_perf, AMD_CPPC_HIGHEST_PERF(cap1));
>
>
> So henceforth, cpudata->highest_perf is expected to cache the value of
> CPPC.highest_perf and not the boost_ratio_numerator. There are couple
> of user-visible changes due to this.
>
>
> 1.  On platforms where preferred-core is supported, previously the
>      sysfs file
>      /sys/devices/system/cpu/cpuX/cpufreq/amd_pstate_highest_perf would
>      report the boost_ratio_numerator. Henceforth it will report
>      CPPC.highest_perf.
>
>      I hope there are no userspace tools that compute the boost_ratio
>      using the syfs amd_pstate_highest_perf/amd_pstate_nominal_perf.
>
> 2. The amd_pstate_prefcore_ranking and amd_pstate_highest_perf will
>     show the same values on all platforms, and henceforth are
>     redundant.

Good observations here.  I'm not aware of any tools trying to replicate 
this calculation.
With the redundancy I would actually argue we should just drop the sysfs 
file 'amd_pstate_prefcore_ranking'.

Thoughts?

> Shouldn't this be documented?

I noticed amd_pstate_prefcore_ranking wasn't properly documented in 
amd-pstate.rst in the first place.  If the decision is not to drop the 
sysfs file, then I'll add a section for it.

> The rest of the patch looks good to me.
>
>
>
> --
> Thanks and Regards
> gautham.