| Message ID | 20240626042043.2410-3-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Mario Limonciello |
| Headers | show |
| Series | Fixes for wrong performance levels in acpi-cpufreq | expand |
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.
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.
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.
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.
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.
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.
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(-)