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

[Mario Limonciello <mario.limonciello@amd.com>]

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@amd.com>
>>
>> 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@amd.com>
>> ---
>>   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.
>