| Message ID | a9e6d487f7405e1c5b4affbd5ebeb4098f0e70e4.1718787627.git.perry.yuan@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Mario Limonciello |
| Headers | show |
| Series | AMD Pstate Driver Core Performance Boost | expand |
On 6/19/2024 04:16, Perry Yuan wrote: > From: Perry Yuan <Perry.Yuan@amd.com> > > Add one global `global_params` to represent CPU Performance Boost(cpb) > state for cpu frequency scaling, both active and passive modes all can > support CPU cores frequency boosting control which is based on the BIOS > setting, while BIOS turn on the "Core Performance Boost", it will > allow OS control each core highest perf limitation from OS side. > > The active, guided and passive modes of the amd-pstate driver can > support frequency boost control when the "Core Performance Boost" > (CPB) feature is enabled in the BIOS. When enabled in BIOS, the user > has an option at runtime to allow/disallow the cores from operating in > the boost frequency range. > > Add an amd_pstate_global_params object to record whether CPB is > enabled in BIOS, and if it has been activated by the user > > Reported-by: Artem S. Tashkinov" <aros@gmx.com> > Cc: Oleksandr Natalenko <oleksandr@natalenko.name> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217931 > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com> > --- > drivers/cpufreq/amd-pstate.c | 59 +++++++++++++++++++++++++++++------- > drivers/cpufreq/amd-pstate.h | 13 ++++++++ > 2 files changed, 61 insertions(+), 11 deletions(-) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index 5bdcdd3ea163..0c50b8ba16b6 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -102,6 +102,8 @@ static int cppc_state = AMD_PSTATE_UNDEFINED; > static bool cppc_enabled; > static bool amd_pstate_prefcore = true; > static struct quirk_entry *quirks; > +struct amd_pstate_global_params amd_pstate_global_params; > +EXPORT_SYMBOL_GPL(amd_pstate_global_params); > > /* > * AMD Energy Preference Performance (EPP) > @@ -694,7 +696,7 @@ static int amd_pstate_set_boost(struct cpufreq_policy *policy, int state) > > if (!cpudata->boost_supported) { > pr_err("Boost mode is not supported by this processor or SBIOS\n"); > - return -EINVAL; > + return -ENOTSUPP; You're using -ENOTSUPP here and -EOPNOTSUPP in amd_pstate_boost_set(). You should be consistent with these error codes. That being said I think the other error flow needs to go. More on that below. > } > > if (state) > @@ -712,18 +714,38 @@ static int amd_pstate_set_boost(struct cpufreq_policy *policy, int state) > return 0; > } > > -static void amd_pstate_boost_init(struct amd_cpudata *cpudata) > +static int amd_pstate_boost_set(struct amd_cpudata *cpudata) > { > - u32 highest_perf, nominal_perf; > + u64 boost_val; > + int ret = -1; > > - highest_perf = READ_ONCE(cpudata->highest_perf); > - nominal_perf = READ_ONCE(cpudata->nominal_perf); > + if (!cpu_feature_enabled(X86_FEATURE_CPB)) { > + pr_debug_once("Boost CPB capabilities not present in the processor\n"); > + ret = -EOPNOTSUPP; > + goto exit_err; > + } This needs a little bit of unwinding to think about whether it's correct return code. This is the call path: cpufreq_driver->init_boost(); -> amd_pstate_init_boost() ->-> amd_pstate_boost_set() So if we end up with a platform that supports CPPC but doesn't support boost we might end up nuking the amd-pstate support "because" of the lack of boost. For this reason I think it's better to set the return code to 0 in this specific place. The others in this function look correct to me and if fail should cause a critical problem with setting up CPPC. > > - if (highest_perf <= nominal_perf) > - return; > + ret = rdmsrl_on_cpu(cpudata->cpu, MSR_K7_HWCR, &boost_val); > + if (ret) { > + pr_err_once("failed to read initial CPU boost state!\n"); > + ret = -EIO; > + goto exit_err; > + } > + > + amd_pstate_global_params.cpb_supported = !(boost_val & MSR_K7_HWCR_CPB_DIS); > + if (amd_pstate_global_params.cpb_supported) { > + current_pstate_driver->boost_enabled = true; > + WRITE_ONCE(cpudata->boost_supported, true); > + } > > - cpudata->boost_supported = true; > - current_pstate_driver->boost_enabled = true; > + amd_pstate_global_params.cpb_boost = amd_pstate_global_params.cpb_supported; > + return 0; > + > +exit_err: > + WRITE_ONCE(cpudata->boost_supported, false); > + current_pstate_driver->boost_enabled = false; > + amd_pstate_global_params.cpb_boost = false; > + return ret; > } > > static void amd_perf_ctl_reset(unsigned int cpu) > @@ -1005,7 +1027,6 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy) > > policy->driver_data = cpudata; > > - amd_pstate_boost_init(cpudata); > if (!current_pstate_driver->adjust_perf) > current_pstate_driver->adjust_perf = amd_pstate_adjust_perf; > > @@ -1387,6 +1408,21 @@ static bool amd_pstate_acpi_pm_profile_undefined(void) > return false; > } > > +static int amd_pstate_init_boost(struct cpufreq_policy *policy) > +{ > + struct amd_cpudata *cpudata = policy->driver_data; > + int ret; > + > + /* initialize cpu cores boot state */ > + ret = amd_pstate_boost_set(cpudata); > + if (ret) > + return ret; > + > + policy->boost_enabled = READ_ONCE(cpudata->boost_supported); > + > + return 0; > +} > + > static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) > { > int min_freq, max_freq, nominal_freq, ret; > @@ -1465,7 +1501,6 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) > return ret; > WRITE_ONCE(cpudata->cppc_cap1_cached, value); > } > - amd_pstate_boost_init(cpudata); > > return 0; > > @@ -1704,6 +1739,7 @@ static struct cpufreq_driver amd_pstate_driver = { > .exit = amd_pstate_cpu_exit, > .suspend = amd_pstate_cpu_suspend, > .resume = amd_pstate_cpu_resume, > + .init_boost = amd_pstate_init_boost, > .set_boost = amd_pstate_set_boost, > .update_limits = amd_pstate_update_limits, > .name = "amd-pstate", > @@ -1721,6 +1757,7 @@ static struct cpufreq_driver amd_pstate_epp_driver = { > .suspend = amd_pstate_epp_suspend, > .resume = amd_pstate_epp_resume, > .update_limits = amd_pstate_update_limits, > + .init_boost = amd_pstate_init_boost, > .name = "amd-pstate-epp", > .attr = amd_pstate_epp_attr, > }; > diff --git a/drivers/cpufreq/amd-pstate.h b/drivers/cpufreq/amd-pstate.h > index f80b33fa5d43..133042370a8f 100644 > --- a/drivers/cpufreq/amd-pstate.h > +++ b/drivers/cpufreq/amd-pstate.h > @@ -102,4 +102,17 @@ struct amd_cpudata { > s16 epp_default; > }; > > +/** > + * struct amd_pstate_global_params - Global parameters, mostly tunable via sysfs. > + * @cpb_boost: Whether or not to use boost CPU P-states. > + * @cpb_supported: Whether or not CPU boost P-states are available > + * based on the MSR_K7_HWCR bit[25] state > + */ > +struct amd_pstate_global_params { > + bool cpb_boost; > + bool cpb_supported; > +}; > + > +extern struct amd_pstate_global_params amd_pstate_global_params; > + > #endif /* _LINUX_AMD_PSTATE_H */
Perry Yuan <perry.yuan@amd.com> writes: > From: Perry Yuan <Perry.Yuan@amd.com> > > Add one global `global_params` to represent CPU Performance Boost(cpb) > state for cpu frequency scaling, both active and passive modes all can > support CPU cores frequency boosting control which is based on the BIOS > setting, while BIOS turn on the "Core Performance Boost", it will > allow OS control each core highest perf limitation from OS side. > > The active, guided and passive modes of the amd-pstate driver can > support frequency boost control when the "Core Performance Boost" > (CPB) feature is enabled in the BIOS. When enabled in BIOS, the user > has an option at runtime to allow/disallow the cores from operating in > the boost frequency range. > > Add an amd_pstate_global_params object to record whether CPB is > enabled in BIOS, and if it has been activated by the user Can we rephrase this as : "The "Core Performance Boost (CPB) feature, when enabled in the BIOS, allows the OS to control the highest performance for each individual core. The active, passive and the guided modes of the amd-pstate driver do support controlling the core frequency boost when this BIOS feature is enabled. Additionally, the amd-pstate driver provides a sysfs interface allowing the user to activate/deactive this core performance boost featur at runtime. Add an amd_pstate_global_params object to record whether CPB is enabled in the BIOS, and if it has been activated by the user." > > Reported-by: Artem S. Tashkinov" <aros@gmx.com> > Cc: Oleksandr Natalenko <oleksandr@natalenko.name> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217931 > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com> > --- > drivers/cpufreq/amd-pstate.c | 59 +++++++++++++++++++++++++++++------- > drivers/cpufreq/amd-pstate.h | 13 ++++++++ > 2 files changed, 61 insertions(+), 11 deletions(-) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index 5bdcdd3ea163..0c50b8ba16b6 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -102,6 +102,8 @@ static int cppc_state = AMD_PSTATE_UNDEFINED; > static bool cppc_enabled; > static bool amd_pstate_prefcore = true; > static struct quirk_entry *quirks; > +struct amd_pstate_global_params amd_pstate_global_params; > +EXPORT_SYMBOL_GPL(amd_pstate_global_params); > > /* > * AMD Energy Preference Performance (EPP) > @@ -694,7 +696,7 @@ static int amd_pstate_set_boost(struct cpufreq_policy *policy, int state) > > if (!cpudata->boost_supported) { > pr_err("Boost mode is not supported by this processor or SBIOS\n"); > - return -EINVAL; > + return -ENOTSUPP; > } > > if (state) > @@ -712,18 +714,38 @@ static int amd_pstate_set_boost(struct cpufreq_policy *policy, int state) > return 0; > } > > -static void amd_pstate_boost_init(struct amd_cpudata *cpudata) > +static int amd_pstate_boost_set(struct amd_cpudata *cpudata) There is already an amd_pstate_set_boost(). The new name you are providing is amd_pstate_boost_set(). Makes it hard to read the code. Can we rename the existing amd_pstate_set_boost() to amd_pstate_update_boost() and keep amd_pstate_boost_set() for this function ? > { > - u32 highest_perf, nominal_perf; > + u64 boost_val; > + int ret = -1; > > - highest_perf = READ_ONCE(cpudata->highest_perf); > - nominal_perf = READ_ONCE(cpudata->nominal_perf); > + if (!cpu_feature_enabled(X86_FEATURE_CPB)) { > + pr_debug_once("Boost CPB capabilities not present in the processor\n"); > + ret = -EOPNOTSUPP; > + goto exit_err; > + } > > - if (highest_perf <= nominal_perf) > - return; > + ret = rdmsrl_on_cpu(cpudata->cpu, MSR_K7_HWCR, &boost_val); > + if (ret) { > + pr_err_once("failed to read initial CPU boost state!\n"); > + ret = -EIO; > + goto exit_err; > + } > + > + amd_pstate_global_params.cpb_supported = !(boost_val & MSR_K7_HWCR_CPB_DIS); "amd_pstate_global_params.cpb_supported" will always contain the MSR_K7_HWCR[CPB_DIS] of the last CPU that calls amd_pstate_boost_set() since the code overwrites the value each time amd_pstate_boost_set() is called for cpudata. So would it be correct to assume the MSR_K7_HWCR[CPB_DIS] is going to be the same for every CPU ? -- Thanks and Regards gautham.
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index 5bdcdd3ea163..0c50b8ba16b6 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -102,6 +102,8 @@ static int cppc_state = AMD_PSTATE_UNDEFINED; static bool cppc_enabled; static bool amd_pstate_prefcore = true; static struct quirk_entry *quirks; +struct amd_pstate_global_params amd_pstate_global_params; +EXPORT_SYMBOL_GPL(amd_pstate_global_params); /* * AMD Energy Preference Performance (EPP) @@ -694,7 +696,7 @@ static int amd_pstate_set_boost(struct cpufreq_policy *policy, int state) if (!cpudata->boost_supported) { pr_err("Boost mode is not supported by this processor or SBIOS\n"); - return -EINVAL; + return -ENOTSUPP; } if (state) @@ -712,18 +714,38 @@ static int amd_pstate_set_boost(struct cpufreq_policy *policy, int state) return 0; } -static void amd_pstate_boost_init(struct amd_cpudata *cpudata) +static int amd_pstate_boost_set(struct amd_cpudata *cpudata) { - u32 highest_perf, nominal_perf; + u64 boost_val; + int ret = -1; - highest_perf = READ_ONCE(cpudata->highest_perf); - nominal_perf = READ_ONCE(cpudata->nominal_perf); + if (!cpu_feature_enabled(X86_FEATURE_CPB)) { + pr_debug_once("Boost CPB capabilities not present in the processor\n"); + ret = -EOPNOTSUPP; + goto exit_err; + } - if (highest_perf <= nominal_perf) - return; + ret = rdmsrl_on_cpu(cpudata->cpu, MSR_K7_HWCR, &boost_val); + if (ret) { + pr_err_once("failed to read initial CPU boost state!\n"); + ret = -EIO; + goto exit_err; + } + + amd_pstate_global_params.cpb_supported = !(boost_val & MSR_K7_HWCR_CPB_DIS); + if (amd_pstate_global_params.cpb_supported) { + current_pstate_driver->boost_enabled = true; + WRITE_ONCE(cpudata->boost_supported, true); + } - cpudata->boost_supported = true; - current_pstate_driver->boost_enabled = true; + amd_pstate_global_params.cpb_boost = amd_pstate_global_params.cpb_supported; + return 0; + +exit_err: + WRITE_ONCE(cpudata->boost_supported, false); + current_pstate_driver->boost_enabled = false; + amd_pstate_global_params.cpb_boost = false; + return ret; } static void amd_perf_ctl_reset(unsigned int cpu) @@ -1005,7 +1027,6 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy) policy->driver_data = cpudata; - amd_pstate_boost_init(cpudata); if (!current_pstate_driver->adjust_perf) current_pstate_driver->adjust_perf = amd_pstate_adjust_perf; @@ -1387,6 +1408,21 @@ static bool amd_pstate_acpi_pm_profile_undefined(void) return false; } +static int amd_pstate_init_boost(struct cpufreq_policy *policy) +{ + struct amd_cpudata *cpudata = policy->driver_data; + int ret; + + /* initialize cpu cores boot state */ + ret = amd_pstate_boost_set(cpudata); + if (ret) + return ret; + + policy->boost_enabled = READ_ONCE(cpudata->boost_supported); + + return 0; +} + static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) { int min_freq, max_freq, nominal_freq, ret; @@ -1465,7 +1501,6 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) return ret; WRITE_ONCE(cpudata->cppc_cap1_cached, value); } - amd_pstate_boost_init(cpudata); return 0; @@ -1704,6 +1739,7 @@ static struct cpufreq_driver amd_pstate_driver = { .exit = amd_pstate_cpu_exit, .suspend = amd_pstate_cpu_suspend, .resume = amd_pstate_cpu_resume, + .init_boost = amd_pstate_init_boost, .set_boost = amd_pstate_set_boost, .update_limits = amd_pstate_update_limits, .name = "amd-pstate", @@ -1721,6 +1757,7 @@ static struct cpufreq_driver amd_pstate_epp_driver = { .suspend = amd_pstate_epp_suspend, .resume = amd_pstate_epp_resume, .update_limits = amd_pstate_update_limits, + .init_boost = amd_pstate_init_boost, .name = "amd-pstate-epp", .attr = amd_pstate_epp_attr, }; diff --git a/drivers/cpufreq/amd-pstate.h b/drivers/cpufreq/amd-pstate.h index f80b33fa5d43..133042370a8f 100644 --- a/drivers/cpufreq/amd-pstate.h +++ b/drivers/cpufreq/amd-pstate.h @@ -102,4 +102,17 @@ struct amd_cpudata { s16 epp_default; }; +/** + * struct amd_pstate_global_params - Global parameters, mostly tunable via sysfs. + * @cpb_boost: Whether or not to use boost CPU P-states. + * @cpb_supported: Whether or not CPU boost P-states are available + * based on the MSR_K7_HWCR bit[25] state + */ +struct amd_pstate_global_params { + bool cpb_boost; + bool cpb_supported; +}; + +extern struct amd_pstate_global_params amd_pstate_global_params; + #endif /* _LINUX_AMD_PSTATE_H */