| Message ID | 20240626042043.2410-3-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes for wrong performance levels in acpi-cpufreq | expand |
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.
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;
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(-)