Re: [PATCH v12 4/9] cpufreq: amd-pstate: initialize new core precision boost state

[Gautham R.Shenoy <gautham.shenoy@amd.com>]

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.