| Message ID | 20220709141739.834950-1-Perry.Yuan@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,01/14] x86/msr: Add the MSR definition for AMD CPPC hardware control. | expand |
On 7/9/22 09:17, Perry Yuan wrote: > Add support to switch AMD precision boost state to scale cpu max > frequency that will help to improve the processor throughput. > > when set boost state to be enabled, user will need to execute below commands, > the CPU will reach absolute maximum performance level or the highest perf which > CPU physical support. This performance level may not be sustainable for > long durations, it will help to improve the IO workload tasks. > > * turn on CPU boost state under root > echo 1 > /sys/devices/system/cpu/cpufreq/boost > > If user set boost off,the CPU can reach to the maximum sustained > performance level of the process, that level is the process can maintain > continously working and definitely it can save some power compared to > boost on mode. > > * turn off CPU boost state under root > echo 0 > /sys/devices/system/cpu/cpufreq/boost > > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com> > --- > arch/x86/include/asm/msr-index.h | 2 ++ > drivers/cpufreq/amd-pstate.c | 22 +++++++++++++++++++--- > 2 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h > index 869508de8269..b952fd6d6916 100644 > --- a/arch/x86/include/asm/msr-index.h > +++ b/arch/x86/include/asm/msr-index.h > @@ -559,6 +559,8 @@ > #define AMD_CPPC_MIN_PERF(x) (((x) & 0xff) << 8) > #define AMD_CPPC_DES_PERF(x) (((x) & 0xff) << 16) > #define AMD_CPPC_ENERGY_PERF_PREF(x) (((x) & 0xff) << 24) > +#define AMD_CPPC_PRECISION_BOOST_BIT 25 > +#define AMD_CPPC_PRECISION_BOOST_ENABLED BIT_ULL(AMD_CPPC_PRECISION_BOOST_BIT) > > /* AMD Performance Counter Global Status and Control MSRs */ > #define MSR_AMD64_PERF_CNTR_GLOBAL_STATUS 0xc0000300 > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index 9ac75c1cde9c..188e055e24a2 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -122,6 +122,7 @@ struct amd_cpudata { > > u64 freq; > bool boost_supported; > + u64 cppc_hw_conf_cached; > }; > > static inline int pstate_enable(bool enable) > @@ -438,18 +439,27 @@ static int amd_pstate_set_boost(struct cpufreq_policy *policy, int state) > { > struct amd_cpudata *cpudata = policy->driver_data; > int ret; > + u64 value; > > if (!cpudata->boost_supported) { > pr_err("Boost mode is not supported by this processor or SBIOS\n"); > return -EINVAL; > } > > - if (state) > + ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_HW_CTL, &value); > + if (ret) > + return ret; > + > + if (state) { > + value |= AMD_CPPC_PRECISION_BOOST_ENABLED; > policy->cpuinfo.max_freq = cpudata->max_freq; > - else > + } else { > + value &= ~AMD_CPPC_PRECISION_BOOST_ENABLED; > policy->cpuinfo.max_freq = cpudata->nominal_freq; > - > + } > policy->max = policy->cpuinfo.max_freq; > + WRITE_ONCE(cpudata->cppc_hw_conf_cached, value); Does the entire MSR value need to be cached? We only care about the boost enabled bit so it may be better to just cache that. -Nathan > + wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_HW_CTL, value); > > ret = freq_qos_update_request(&cpudata->req[1], > policy->cpuinfo.max_freq); > @@ -478,6 +488,7 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy) > int min_freq, max_freq, nominal_freq, lowest_nonlinear_freq, ret; > struct device *dev; > struct amd_cpudata *cpudata; > + u64 value; > > dev = get_cpu_device(policy->cpu); > if (!dev) > @@ -542,6 +553,11 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy) > > policy->driver_data = cpudata; > > + ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_HW_CTL, &value); > + if (ret) > + return ret; > + WRITE_ONCE(cpudata->cppc_hw_conf_cached, value); > + > amd_pstate_boost_init(cpudata); > > return 0;
[AMD Official Use Only - General] Hi Nathan: > -----Original Message----- > From: Fontenot, Nathan <Nathan.Fontenot@amd.com> > Sent: Tuesday, July 12, 2022 3:27 AM > To: Yuan, Perry <Perry.Yuan@amd.com>; rafael.j.wysocki@intel.com; > viresh.kumar@linaro.org; Huang, Ray <Ray.Huang@amd.com> > Cc: Sharma, Deepak <Deepak.Sharma@amd.com>; Limonciello, Mario > <Mario.Limonciello@amd.com>; Fontenot, Nathan > <Nathan.Fontenot@amd.com>; Deucher, Alexander > <Alexander.Deucher@amd.com>; Su, Jinzhou (Joe) <Jinzhou.Su@amd.com>; > Huang, Shimmer <Shimmer.Huang@amd.com>; Du, Xiaojian > <Xiaojian.Du@amd.com>; Meng, Li (Jassmine) <Li.Meng@amd.com>; linux- > pm@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v2 02/14] cpufreq: amd-pstate: enable AMD Precision > Boost mode switch > > On 7/9/22 09:17, Perry Yuan wrote: > > Add support to switch AMD precision boost state to scale cpu max > > frequency that will help to improve the processor throughput. > > > > when set boost state to be enabled, user will need to execute below > > commands, the CPU will reach absolute maximum performance level or > the > > highest perf which CPU physical support. This performance level may > > not be sustainable for long durations, it will help to improve the IO > workload tasks. > > > > * turn on CPU boost state under root > > echo 1 > /sys/devices/system/cpu/cpufreq/boost > > > > If user set boost off,the CPU can reach to the maximum sustained > > performance level of the process, that level is the process can > > maintain continously working and definitely it can save some power > > compared to boost on mode. > > > > * turn off CPU boost state under root > > echo 0 > /sys/devices/system/cpu/cpufreq/boost > > > > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com> > > --- > > arch/x86/include/asm/msr-index.h | 2 ++ > > drivers/cpufreq/amd-pstate.c | 22 +++++++++++++++++++--- > > 2 files changed, 21 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/include/asm/msr-index.h > > b/arch/x86/include/asm/msr-index.h > > index 869508de8269..b952fd6d6916 100644 > > --- a/arch/x86/include/asm/msr-index.h > > +++ b/arch/x86/include/asm/msr-index.h > > @@ -559,6 +559,8 @@ > > #define AMD_CPPC_MIN_PERF(x) (((x) & 0xff) << 8) > > #define AMD_CPPC_DES_PERF(x) (((x) & 0xff) << 16) > > #define AMD_CPPC_ENERGY_PERF_PREF(x) (((x) & 0xff) << 24) > > +#define AMD_CPPC_PRECISION_BOOST_BIT 25 > > +#define AMD_CPPC_PRECISION_BOOST_ENABLED > BIT_ULL(AMD_CPPC_PRECISION_BOOST_BIT) > > > > /* AMD Performance Counter Global Status and Control MSRs */ > > #define MSR_AMD64_PERF_CNTR_GLOBAL_STATUS 0xc0000300 > > diff --git a/drivers/cpufreq/amd-pstate.c > > b/drivers/cpufreq/amd-pstate.c index 9ac75c1cde9c..188e055e24a2 > 100644 > > --- a/drivers/cpufreq/amd-pstate.c > > +++ b/drivers/cpufreq/amd-pstate.c > > @@ -122,6 +122,7 @@ struct amd_cpudata { > > > > u64 freq; > > bool boost_supported; > > + u64 cppc_hw_conf_cached; > > }; > > > > static inline int pstate_enable(bool enable) @@ -438,18 +439,27 @@ > > static int amd_pstate_set_boost(struct cpufreq_policy *policy, int > > state) { > > struct amd_cpudata *cpudata = policy->driver_data; > > int ret; > > + u64 value; > > > > if (!cpudata->boost_supported) { > > pr_err("Boost mode is not supported by this processor or > SBIOS\n"); > > return -EINVAL; > > } > > > > - if (state) > > + ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_HW_CTL, > &value); > > + if (ret) > > + return ret; > > + > > + if (state) { > > + value |= AMD_CPPC_PRECISION_BOOST_ENABLED; > > policy->cpuinfo.max_freq = cpudata->max_freq; > > - else > > + } else { > > + value &= ~AMD_CPPC_PRECISION_BOOST_ENABLED; > > policy->cpuinfo.max_freq = cpudata->nominal_freq; > > - > > + } > > policy->max = policy->cpuinfo.max_freq; > > + WRITE_ONCE(cpudata->cppc_hw_conf_cached, value); > > Does the entire MSR value need to be cached? We only care about the > boost enabled bit so it may be better to just cache that. > > -Nathan I think the whole MSR value should be cached, because it has no bad impact to the hardware or driver . And it is simple to do that, when we need to check other bits in the hardware configuration MSR in future, we can still use this cached value as well. Dose it make sense ? Perry. > > > + wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_HW_CTL, value); > > > > ret = freq_qos_update_request(&cpudata->req[1], > > policy->cpuinfo.max_freq); > > @@ -478,6 +488,7 @@ static int amd_pstate_cpu_init(struct > cpufreq_policy *policy) > > int min_freq, max_freq, nominal_freq, lowest_nonlinear_freq, ret; > > struct device *dev; > > struct amd_cpudata *cpudata; > > + u64 value; > > > > dev = get_cpu_device(policy->cpu); > > if (!dev) > > @@ -542,6 +553,11 @@ static int amd_pstate_cpu_init(struct > > cpufreq_policy *policy) > > > > policy->driver_data = cpudata; > > > > + ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_HW_CTL, > &value); > > + if (ret) > > + return ret; > > + WRITE_ONCE(cpudata->cppc_hw_conf_cached, value); > > + > > amd_pstate_boost_init(cpudata); > > > > return 0;
On 7/11/22 23:15, Yuan, Perry wrote:>>> --- a/drivers/cpufreq/amd-pstate.c >>> +++ b/drivers/cpufreq/amd-pstate.c >>> @@ -122,6 +122,7 @@ struct amd_cpudata { >>> >>> u64 freq; >>> bool boost_supported; >>> + u64 cppc_hw_conf_cached; >>> }; >>> >>> static inline int pstate_enable(bool enable) @@ -438,18 +439,27 @@ >>> static int amd_pstate_set_boost(struct cpufreq_policy *policy, int >>> state) { >>> struct amd_cpudata *cpudata = policy->driver_data; >>> int ret; >>> + u64 value; >>> >>> if (!cpudata->boost_supported) { >>> pr_err("Boost mode is not supported by this processor or >> SBIOS\n"); >>> return -EINVAL; >>> } >>> >>> - if (state) >>> + ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_HW_CTL, >> &value); >>> + if (ret) >>> + return ret; >>> + >>> + if (state) { >>> + value |= AMD_CPPC_PRECISION_BOOST_ENABLED; >>> policy->cpuinfo.max_freq = cpudata->max_freq; >>> - else >>> + } else { >>> + value &= ~AMD_CPPC_PRECISION_BOOST_ENABLED; >>> policy->cpuinfo.max_freq = cpudata->nominal_freq; >>> - >>> + } >>> policy->max = policy->cpuinfo.max_freq; >>> + WRITE_ONCE(cpudata->cppc_hw_conf_cached, value); >> >> Does the entire MSR value need to be cached? We only care about the >> boost enabled bit so it may be better to just cache that. >> >> -Nathan > > I think the whole MSR value should be cached, because it has no bad impact to the hardware or driver . > And it is simple to do that, when we need to check other bits in the hardware configuration MSR in future, we can still use this cached value as well. > Dose it make sense ? > > Perry. > The MSR controls much more than boost enabling, my concern is someone else starting to update bits in the MSR and now we have a stale MSR value. Just caching the boost enabled bit is really aimed at possible future bug prevention. -Nathan
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h index 869508de8269..b952fd6d6916 100644 --- a/arch/x86/include/asm/msr-index.h +++ b/arch/x86/include/asm/msr-index.h @@ -559,6 +559,8 @@ #define AMD_CPPC_MIN_PERF(x) (((x) & 0xff) << 8) #define AMD_CPPC_DES_PERF(x) (((x) & 0xff) << 16) #define AMD_CPPC_ENERGY_PERF_PREF(x) (((x) & 0xff) << 24) +#define AMD_CPPC_PRECISION_BOOST_BIT 25 +#define AMD_CPPC_PRECISION_BOOST_ENABLED BIT_ULL(AMD_CPPC_PRECISION_BOOST_BIT) /* AMD Performance Counter Global Status and Control MSRs */ #define MSR_AMD64_PERF_CNTR_GLOBAL_STATUS 0xc0000300 diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index 9ac75c1cde9c..188e055e24a2 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -122,6 +122,7 @@ struct amd_cpudata { u64 freq; bool boost_supported; + u64 cppc_hw_conf_cached; }; static inline int pstate_enable(bool enable) @@ -438,18 +439,27 @@ static int amd_pstate_set_boost(struct cpufreq_policy *policy, int state) { struct amd_cpudata *cpudata = policy->driver_data; int ret; + u64 value; if (!cpudata->boost_supported) { pr_err("Boost mode is not supported by this processor or SBIOS\n"); return -EINVAL; } - if (state) + ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_HW_CTL, &value); + if (ret) + return ret; + + if (state) { + value |= AMD_CPPC_PRECISION_BOOST_ENABLED; policy->cpuinfo.max_freq = cpudata->max_freq; - else + } else { + value &= ~AMD_CPPC_PRECISION_BOOST_ENABLED; policy->cpuinfo.max_freq = cpudata->nominal_freq; - + } policy->max = policy->cpuinfo.max_freq; + WRITE_ONCE(cpudata->cppc_hw_conf_cached, value); + wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_HW_CTL, value); ret = freq_qos_update_request(&cpudata->req[1], policy->cpuinfo.max_freq); @@ -478,6 +488,7 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy) int min_freq, max_freq, nominal_freq, lowest_nonlinear_freq, ret; struct device *dev; struct amd_cpudata *cpudata; + u64 value; dev = get_cpu_device(policy->cpu); if (!dev) @@ -542,6 +553,11 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy) policy->driver_data = cpudata; + ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_HW_CTL, &value); + if (ret) + return ret; + WRITE_ONCE(cpudata->cppc_hw_conf_cached, value); + amd_pstate_boost_init(cpudata); return 0;
Add support to switch AMD precision boost state to scale cpu max frequency that will help to improve the processor throughput. when set boost state to be enabled, user will need to execute below commands, the CPU will reach absolute maximum performance level or the highest perf which CPU physical support. This performance level may not be sustainable for long durations, it will help to improve the IO workload tasks. * turn on CPU boost state under root echo 1 > /sys/devices/system/cpu/cpufreq/boost If user set boost off,the CPU can reach to the maximum sustained performance level of the process, that level is the process can maintain continously working and definitely it can save some power compared to boost on mode. * turn off CPU boost state under root echo 0 > /sys/devices/system/cpu/cpufreq/boost Signed-off-by: Perry Yuan <Perry.Yuan@amd.com> --- arch/x86/include/asm/msr-index.h | 2 ++ drivers/cpufreq/amd-pstate.c | 22 +++++++++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-)