[PATCH 0/2] Fixes for wrong performance levels in acpi-cpufreq

[PATCH 0/2] Fixes for wrong performance levels in acpi-cpufreq

[Mario Limonciello]

In testing with amd-pstate disabled, I noticed that family 0x19 model 0x70
(Phoenix) was reporting an inccorrect max frequency.  This is because the
perf levels used to look it up are wrong.

The correct values are stored in amd-pstate, but there is no reason to
store it in two places. Move the amd-pstate values over to one place
so that both drivers get the right values.

Mario Limonciello (2):
  x86/cpu/amd: Clarify amd_get_highest_perf()
  cpufreq: amd-pstate: Use amd_get_highest_perf() to lookup perf values

 arch/x86/kernel/cpu/amd.c    | 44 ++++++++++++++++++++++++++++++------
 drivers/cpufreq/amd-pstate.c | 21 ++---------------
 2 files changed, 39 insertions(+), 26 deletions(-)

-- 
2.43.0

[PATCH 1/2] x86/cpu/amd: Clarify amd_get_highest_perf()

[Mario Limonciello]

Rather than parsing through families and models as an if/else, use
cpu_feature_enabled() and switch/case.

Add definitions aligned with the amd-pstate definition for performance
levels. No intended functional changes.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 arch/x86/kernel/cpu/amd.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 44df3f11e731..8b730193d79e 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -29,6 +29,10 @@
 
 #include "cpu.h"
 
+#define CPPC_HIGHEST_PERF_MAX		255
+#define CPPC_HIGHEST_PERF_PERFORMANCE	196
+#define CPPC_HIGHEST_PERF_DEFAULT	166
+
 static inline int rdmsrl_amd_safe(unsigned msr, unsigned long long *p)
 {
 	u32 gprs[8] = { 0 };
@@ -1194,15 +1198,27 @@ u32 amd_get_highest_perf(void)
 {
 	struct cpuinfo_x86 *c = &boot_cpu_data;
 
-	if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) ||
-			       (c->x86_model >= 0x70 && c->x86_model < 0x80)))
-		return 166;
+	if (cpu_feature_enabled(X86_FEATURE_ZEN2)) {
+		switch (c->x86_model) {
+		case 0x30 ... 0x40:
+		case 0x70 ... 0x80:
+			return CPPC_HIGHEST_PERF_DEFAULT;
+		default:
+			return CPPC_HIGHEST_PERF_MAX;
+		}
+	}
 
-	if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) ||
-			       (c->x86_model >= 0x40 && c->x86_model < 0x70)))
-		return 166;
+	if (cpu_feature_enabled(X86_FEATURE_ZEN3)) {
+		switch (c->x86_model) {
+		case 0x20 ... 0x30:
+		case 0x40 ... 0x70:
+			return CPPC_HIGHEST_PERF_DEFAULT;
+		default:
+			return CPPC_HIGHEST_PERF_MAX;
+		}
+	}
 
-	return 255;
+	return CPPC_HIGHEST_PERF_MAX;
 }
 EXPORT_SYMBOL_GPL(amd_get_highest_perf);
 
-- 
2.43.0

[PATCH 2/2] cpufreq: amd-pstate: Use amd_get_highest_perf() to lookup perf values

[Mario Limonciello]

To keep consistency with amd-pstate and acpi-cpufreq behavior, use
amd_get_highest_perf() to find the highest perf value for a given
platform.

This fixes the exact same problem as commit bf202e654bfa ("cpufreq:
amd-pstate: fix the highest frequency issue which limits performance")
from happening on acpi-cpufreq too.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 arch/x86/kernel/cpu/amd.c    | 16 +++++++++++++++-
 drivers/cpufreq/amd-pstate.c | 21 ++-------------------
 2 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 8b730193d79e..e69f640cc248 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -1218,7 +1218,21 @@ u32 amd_get_highest_perf(void)
 		}
 	}
 
-	return CPPC_HIGHEST_PERF_MAX;
+	/*
+	 * 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 (c->x86_model) {
+		case 0x70 ... 0x7f:
+			return CPPC_HIGHEST_PERF_PERFORMANCE;
+		default:
+			return CPPC_HIGHEST_PERF_DEFAULT;
+		}
+	}
+
+	return CPPC_HIGHEST_PERF_DEFAULT;
 }
 EXPORT_SYMBOL_GPL(amd_get_highest_perf);
 
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 80eaa58f1405..f468d8562e17 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
@@ -349,21 +347,6 @@ 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;
@@ -380,7 +363,7 @@ static int pstate_init_perf(struct amd_cpudata *cpudata)
 	 * the default max perf.
 	 */
 	if (cpudata->hw_prefcore)
-		highest_perf = amd_pstate_highest_perf_set(cpudata);
+		highest_perf = amd_get_highest_perf();
 	else
 		highest_perf = AMD_CPPC_HIGHEST_PERF(cap1);
 
@@ -404,7 +387,7 @@ static int cppc_init_perf(struct amd_cpudata *cpudata)
 		return ret;
 
 	if (cpudata->hw_prefcore)
-		highest_perf = amd_pstate_highest_perf_set(cpudata);
+		highest_perf = amd_get_highest_perf();
 	else
 		highest_perf = cppc_perf.highest_perf;
 
-- 
2.43.0

Re: [PATCH 1/2] x86/cpu/amd: Clarify amd_get_highest_perf()

[Borislav Petkov]

On Tue, Jun 25, 2024 at 11:20:42PM -0500, Mario Limonciello wrote:
>  static inline int rdmsrl_amd_safe(unsigned msr, unsigned long long *p)
>  {
>  	u32 gprs[8] = { 0 };
> @@ -1194,15 +1198,27 @@ u32 amd_get_highest_perf(void)
>  {
>  	struct cpuinfo_x86 *c = &boot_cpu_data;
>  
> -	if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) ||
> -			       (c->x86_model >= 0x70 && c->x86_model < 0x80)))
> -		return 166;
> +	if (cpu_feature_enabled(X86_FEATURE_ZEN2)) {
> +		switch (c->x86_model) {
> +		case 0x30 ... 0x40:
> +		case 0x70 ... 0x80:

Well, it was < 0x40 and < 0x80

You're making it <=.

> +			return CPPC_HIGHEST_PERF_DEFAULT;
> +		default:
> +			return CPPC_HIGHEST_PERF_MAX;
> +		}
> +	}
>  
> -	if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) ||
> -			       (c->x86_model >= 0x40 && c->x86_model < 0x70)))
> -		return 166;
> +	if (cpu_feature_enabled(X86_FEATURE_ZEN3)) {
> +		switch (c->x86_model) {
> +		case 0x20 ... 0x30:
> +		case 0x40 ... 0x70:

Ditto.

Also, ontop:

diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 73559db78433..5d496de4e141 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -1204,7 +1204,7 @@ u32 amd_get_highest_perf(void)
 		case 0x70 ... 0x80:
 			return CPPC_HIGHEST_PERF_DEFAULT;
 		default:
-			return CPPC_HIGHEST_PERF_MAX;
+			break;
 		}
 	}
 
@@ -1214,7 +1214,7 @@ u32 amd_get_highest_perf(void)
 		case 0x40 ... 0x70:
 			return CPPC_HIGHEST_PERF_DEFAULT;
 		default:
-			return CPPC_HIGHEST_PERF_MAX;
+			break;
 		}
 	}
 
so that you don't have so many redundant returns in the function.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/2] cpufreq: amd-pstate: Use amd_get_highest_perf() to lookup perf values

[Borislav Petkov]

On Tue, Jun 25, 2024 at 11:20:43PM -0500, Mario Limonciello wrote:
> +	/*
> +	 * 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 (c->x86_model) {
> +		case 0x70 ... 0x7f:

Aha, so here it is non-inclusive - "<" and not "<=".

So you need to check the model ranges first.

> +			return CPPC_HIGHEST_PERF_PERFORMANCE;
> +		default:
> +			return CPPC_HIGHEST_PERF_DEFAULT;

As for patch 1.

> +		}
> +	}
> +
> +	return CPPC_HIGHEST_PERF_DEFAULT;
>  }
>  EXPORT_SYMBOL_GPL(amd_get_highest_perf);
>  
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 80eaa58f1405..f468d8562e17 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

This already doesn't apply:

checking file arch/x86/kernel/cpu/amd.c
checking file drivers/cpufreq/amd-pstate.c
Hunk #1 FAILED at 52.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/2] x86/cpu/amd: Clarify amd_get_highest_perf()

[Mario Limonciello]

On 6/26/2024 12:14, Borislav Petkov wrote:
> On Tue, Jun 25, 2024 at 11:20:42PM -0500, Mario Limonciello wrote:
>>   static inline int rdmsrl_amd_safe(unsigned msr, unsigned long long *p)
>>   {
>>   	u32 gprs[8] = { 0 };
>> @@ -1194,15 +1198,27 @@ u32 amd_get_highest_perf(void)
>>   {
>>   	struct cpuinfo_x86 *c = &boot_cpu_data;
>>   
>> -	if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) ||
>> -			       (c->x86_model >= 0x70 && c->x86_model < 0x80)))
>> -		return 166;
>> +	if (cpu_feature_enabled(X86_FEATURE_ZEN2)) {
>> +		switch (c->x86_model) {
>> +		case 0x30 ... 0x40:
>> +		case 0x70 ... 0x80:
> 
> Well, it was < 0x40 and < 0x80
> 
> You're making it <=.
> 

Good catch, I'll adjust to 0x3f and 0x7f.

>> +			return CPPC_HIGHEST_PERF_DEFAULT;
>> +		default:
>> +			return CPPC_HIGHEST_PERF_MAX;
>> +		}
>> +	}
>>   
>> -	if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) ||
>> -			       (c->x86_model >= 0x40 && c->x86_model < 0x70)))
>> -		return 166;
>> +	if (cpu_feature_enabled(X86_FEATURE_ZEN3)) {
>> +		switch (c->x86_model) {
>> +		case 0x20 ... 0x30:
>> +		case 0x40 ... 0x70:
> 
> Ditto.
> 
> Also, ontop:
> 
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 73559db78433..5d496de4e141 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -1204,7 +1204,7 @@ u32 amd_get_highest_perf(void)
>   		case 0x70 ... 0x80:
>   			return CPPC_HIGHEST_PERF_DEFAULT;
>   		default:
> -			return CPPC_HIGHEST_PERF_MAX;
> +			break;
>   		}
>   	}
>   
> @@ -1214,7 +1214,7 @@ u32 amd_get_highest_perf(void)
>   		case 0x40 ... 0x70:
>   			return CPPC_HIGHEST_PERF_DEFAULT;
>   		default:
> -			return CPPC_HIGHEST_PERF_MAX;
> +			break;
>   		}
>   	}
>   
> so that you don't have so many redundant returns in the function.
> 

This uncovers a case that I'm not really sure what to do about.

Right now acpi-cpufreq uses this function and if the CPU isn't special 
cased you'll get the value as 255.  This is totally wrong for newer SoCs 
though.  That's what prompted this series in the first place that I saw 
different behavior in amd-pstate and acpi-cpufreq.

So I think we need to have something that is:

switch (zen1) {
case ...
default)
	return 255;
}
switch (zen2) {
case ...
default)
	return 255;
}
switch (zen3) {
case ...
default)
	break;
}
switch (zen4) {
case ...
default)
	break;
}

return 255;

(This is no functional changes)

And then patch 2 or patch 3 change the "default" return to 166 and if 
there is functional issues then they need to be special cased.

Re: [PATCH 2/2] cpufreq: amd-pstate: Use amd_get_highest_perf() to lookup perf values

[Mario Limonciello]

On 6/26/2024 12:18, Borislav Petkov wrote:
> On Tue, Jun 25, 2024 at 11:20:43PM -0500, Mario Limonciello wrote:
>> +	/*
>> +	 * 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 (c->x86_model) {
>> +		case 0x70 ... 0x7f:
> 
> Aha, so here it is non-inclusive - "<" and not "<=".
> 
> So you need to check the model ranges first.
> 
>> +			return CPPC_HIGHEST_PERF_PERFORMANCE;
>> +		default:
>> +			return CPPC_HIGHEST_PERF_DEFAULT;
> 
> As for patch 1.
> 
>> +		}
>> +	}
>> +
>> +	return CPPC_HIGHEST_PERF_DEFAULT;
>>   }
>>   EXPORT_SYMBOL_GPL(amd_get_highest_perf);
>>   
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index 80eaa58f1405..f468d8562e17 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
> 
> This already doesn't apply:
> 
> checking file arch/x86/kernel/cpu/amd.c
> checking file drivers/cpufreq/amd-pstate.c
> Hunk #1 FAILED at 52.
> 

I was thinking we would take this patch through superm1/linux-next or 
linux-pm/linux-next as there is other amd-pstate stuff for the next 
merge window, but if you'd rather go through x86 then we can wait until 
after the merge window on this series.

Re: [PATCH 1/2] x86/cpu/amd: Clarify amd_get_highest_perf()

[Borislav Petkov]

On Wed, Jun 26, 2024 at 01:18:17PM -0500, Mario Limonciello wrote:
> And then patch 2 or patch 3 change the "default" return to 166 and if there
> is functional issues then they need to be special cased.

Sounds ok to me. Keep the whole logic in one place. Sure.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/2] cpufreq: amd-pstate: Use amd_get_highest_perf() to lookup perf values

[Borislav Petkov]

On Wed, Jun 26, 2024 at 01:19:56PM -0500, Mario Limonciello wrote:
> I was thinking we would take this patch through superm1/linux-next or
> linux-pm/linux-next as there is other amd-pstate stuff for the next merge
> window, but if you'd rather go through x86 then we can wait until after the
> merge window on this series.

I can also ACK this once it is ready and you can take it through whichever
tree you prefer.  

I don't think we'll have merge conflicts there.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/2] cpufreq: amd-pstate: Use amd_get_highest_perf() to lookup perf values

[Gautham R.Shenoy]

Mario Limonciello <mario.limonciello@amd.com> writes:

> To keep consistency with amd-pstate and acpi-cpufreq behavior, use
> amd_get_highest_perf() to find the highest perf value for a given
> platform.
>
> This fixes the exact same problem as commit bf202e654bfa ("cpufreq:
> amd-pstate: fix the highest frequency issue which limits performance")
> from happening on acpi-cpufreq too.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  arch/x86/kernel/cpu/amd.c    | 16 +++++++++++++++-
>  drivers/cpufreq/amd-pstate.c | 21 ++-------------------
>  2 files changed, 17 insertions(+), 20 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 8b730193d79e..e69f640cc248 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -1218,7 +1218,21 @@ u32 amd_get_highest_perf(void)
>  		}
>  	}
>

From Patch 1,

+#define CPPC_HIGHEST_PERF_MAX		255
+#define CPPC_HIGHEST_PERF_PERFORMANCE	196
+#define CPPC_HIGHEST_PERF_DEFAULT	166
+



> -	return CPPC_HIGHEST_PERF_MAX;
> +	/*
> +	 * 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 (c->x86_model) {
> +		case 0x70 ... 0x7f:
> +			return CPPC_HIGHEST_PERF_PERFORMANCE;
> +		default:
> +			return CPPC_HIGHEST_PERF_DEFAULT;
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Should this be CPPC_HIGHEST_PERF_MAX ?

Without this patchset, this function returns 255 on Genoa (0x10-0x1f)
and Bergamo (0xa0-0xaf) systems. This patchset changes the return value
to 166.

The acpi-cpufreq driver computes the max frequency based on the
boost-ratio, which is the ratio of the highest_perf (returned by this
function) to the nominal_perf.

So assuming a nominal_freq of 2000Mhz, nominal_perf of 159.

Previously the max_perf = (2000*255/159) ~ 3200Mhz
With this patch max_perf = (2000*166/159) ~ 2100Mhz.

Am I missing something ?


--
Thanks and Regards
gautham.

Re: [PATCH 2/2] cpufreq: amd-pstate: Use amd_get_highest_perf() to lookup perf values

[Mario Limonciello]

On 6/27/2024 00:12, Gautham R.Shenoy wrote:
> Mario Limonciello <mario.limonciello@amd.com> writes:
> 
>> To keep consistency with amd-pstate and acpi-cpufreq behavior, use
>> amd_get_highest_perf() to find the highest perf value for a given
>> platform.
>>
>> This fixes the exact same problem as commit bf202e654bfa ("cpufreq:
>> amd-pstate: fix the highest frequency issue which limits performance")
>> from happening on acpi-cpufreq too.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   arch/x86/kernel/cpu/amd.c    | 16 +++++++++++++++-
>>   drivers/cpufreq/amd-pstate.c | 21 ++-------------------
>>   2 files changed, 17 insertions(+), 20 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
>> index 8b730193d79e..e69f640cc248 100644
>> --- a/arch/x86/kernel/cpu/amd.c
>> +++ b/arch/x86/kernel/cpu/amd.c
>> @@ -1218,7 +1218,21 @@ u32 amd_get_highest_perf(void)
>>   		}
>>   	}
>>
> 
>  From Patch 1,
> 
> +#define CPPC_HIGHEST_PERF_MAX		255
> +#define CPPC_HIGHEST_PERF_PERFORMANCE	196
> +#define CPPC_HIGHEST_PERF_DEFAULT	166
> +
> 
> 
> 
>> -	return CPPC_HIGHEST_PERF_MAX;
>> +	/*
>> +	 * 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 (c->x86_model) {
>> +		case 0x70 ... 0x7f:
>> +			return CPPC_HIGHEST_PERF_PERFORMANCE;
>> +		default:
>> +			return CPPC_HIGHEST_PERF_DEFAULT;
>                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Should this be CPPC_HIGHEST_PERF_MAX ?
> 
> Without this patchset, this function returns 255 on Genoa (0x10-0x1f)
> and Bergamo (0xa0-0xaf) systems. This patchset changes the return value
> to 166.
> 
> The acpi-cpufreq driver computes the max frequency based on the
> boost-ratio, which is the ratio of the highest_perf (returned by this
> function) to the nominal_perf.
> 
> So assuming a nominal_freq of 2000Mhz, nominal_perf of 159.
> 
> Previously the max_perf = (2000*255/159) ~ 3200Mhz
> With this patch max_perf = (2000*166/159) ~ 2100Mhz.
> 
> Am I missing something ?

Yeah; this is exactly what I'm worried about.

How does Bergamo handle amd-pstate?  It should probably explode there too.

Re: [PATCH 2/2] cpufreq: amd-pstate: Use amd_get_highest_perf() to lookup perf values

[Gautham R.Shenoy]

Mario Limonciello <mario.limonciello@amd.com> writes:

> On 6/27/2024 00:12, Gautham R.Shenoy wrote:

[..snip..]
>> 
>>> -	return CPPC_HIGHEST_PERF_MAX;
>>> +	/*
>>> +	 * 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 (c->x86_model) {
>>> +		case 0x70 ... 0x7f:
>>> +			return CPPC_HIGHEST_PERF_PERFORMANCE;
>>> +		default:
>>> +			return CPPC_HIGHEST_PERF_DEFAULT;
>>                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> Should this be CPPC_HIGHEST_PERF_MAX ?
>> 
>> Without this patchset, this function returns 255 on Genoa (0x10-0x1f)
>> and Bergamo (0xa0-0xaf) systems. This patchset changes the return value
>> to 166.
>> 
>> The acpi-cpufreq driver computes the max frequency based on the
>> boost-ratio, which is the ratio of the highest_perf (returned by this
>> function) to the nominal_perf.
>> 
>> So assuming a nominal_freq of 2000Mhz, nominal_perf of 159.
>> 
>> Previously the max_perf = (2000*255/159) ~ 3200Mhz
>> With this patch max_perf = (2000*166/159) ~ 2100Mhz.
>> 
>> Am I missing something ?
>
> Yeah; this is exactly what I'm worried about.
>
> How does Bergamo handle amd-pstate?  It should probably explode there
> too.

So amd-pstate driver calls amd_pstate_highest_perf_set() only when
hw_prefcore is set.

Thus for Genoa and Bergamo, since hw_prefcore is false, the highest_perf
is extracted from the MSR_AMD_CPPC_CAP1. See this fragment in
pstate_init_perf()


	/* 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);

Hence it doesn't blow up on amd-pstate. So it looks like it would be
better if the prefcore check is in the amd_get_highest_perf() function
so that it can be invoked from both acpi-cpufreq and amd-pstate drivers.

--
Thanks and Regards
gautham.

Re: [PATCH 2/2] cpufreq: amd-pstate: Use amd_get_highest_perf() to lookup perf values

[Mario Limonciello]

On 6/27/2024 09:47, Gautham R.Shenoy wrote:
> Mario Limonciello <mario.limonciello@amd.com> writes:
> 
>> On 6/27/2024 00:12, Gautham R.Shenoy wrote:
> 
> [..snip..]
>>>
>>>> -	return CPPC_HIGHEST_PERF_MAX;
>>>> +	/*
>>>> +	 * 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 (c->x86_model) {
>>>> +		case 0x70 ... 0x7f:
>>>> +			return CPPC_HIGHEST_PERF_PERFORMANCE;
>>>> +		default:
>>>> +			return CPPC_HIGHEST_PERF_DEFAULT;
>>>                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> Should this be CPPC_HIGHEST_PERF_MAX ?
>>>
>>> Without this patchset, this function returns 255 on Genoa (0x10-0x1f)
>>> and Bergamo (0xa0-0xaf) systems. This patchset changes the return value
>>> to 166.
>>>
>>> The acpi-cpufreq driver computes the max frequency based on the
>>> boost-ratio, which is the ratio of the highest_perf (returned by this
>>> function) to the nominal_perf.
>>>
>>> So assuming a nominal_freq of 2000Mhz, nominal_perf of 159.
>>>
>>> Previously the max_perf = (2000*255/159) ~ 3200Mhz
>>> With this patch max_perf = (2000*166/159) ~ 2100Mhz.
>>>
>>> Am I missing something ?
>>
>> Yeah; this is exactly what I'm worried about.
>>
>> How does Bergamo handle amd-pstate?  It should probably explode there
>> too.
> 
> So amd-pstate driver calls amd_pstate_highest_perf_set() only when
> hw_prefcore is set.
> 
> Thus for Genoa and Bergamo, since hw_prefcore is false, the highest_perf
> is extracted from the MSR_AMD_CPPC_CAP1. See this fragment in
> pstate_init_perf()
> 
> 
> 	/* 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);
> 
> Hence it doesn't blow up on amd-pstate. So it looks like it would be
> better if the prefcore check is in the amd_get_highest_perf() function
> so that it can be invoked from both acpi-cpufreq and amd-pstate drivers.
> 

Ah; yes this makes more sense then.  I'll work on a modified series 
during next kernel cycle.