Re: [PATCH 3/5] cpufreq: amd-pstate: add amd-pstate driver parameter for mode selection

[Wyes Karny]

On 11/17/2022 8:19 AM, Perry Yuan wrote:
> When the amd_pstate driver is built-in users still need a method to be
> able enable or disable it depending upon their circumstance.
> Add support for an early parameter to do this.
> 
> There is some performance degradation on a number of ASICs in the
> passive mode. This performance issue was originally discovered in
> shared memory systems but it has been proven that certain workloads
> on MSR systems also suffer performance issues.
> Set the amd-pstate driver as disabled by default to temporarily
> mitigate the performance problem.
> 
> 1) with `amd_pstate=disable`, pstate driver will be disabled to load at
> kernel booting.
> 
> 2) with `amd_pstate=passive`, pstate driver will be enabled and loaded as
> non-autonomous working mode supported in the low-level power management
> firmware.
> 
> 3) If neither parameter is specified, the driver will be disabled by
> default to avoid triggering performance regressions in certain ASICs
> 
> Signed-off-by: Perry Yuan <Perry.Yuan@xxxxxxx>

Tested-by: Wyes Karny <wyes.karny@xxxxxxx>

> ---
>  drivers/cpufreq/amd-pstate.c | 36 +++++++++++++++++++++++++++---------
>  1 file changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 701f49d6d240..204e39006dda 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -59,12 +59,8 @@
>   * we disable it by default to go acpi-cpufreq on these processors and add a
>   * module parameter to be able to enable it manually for debugging.
>   */
> -static bool shared_mem = false;
> -module_param(shared_mem, bool, 0444);
> -MODULE_PARM_DESC(shared_mem,
> -		 "enable amd-pstate on processors with shared memory solution (false = disabled (default), true = enabled)");
> -
>  static struct cpufreq_driver amd_pstate_driver;
> +static int cppc_load __initdata;
>  
>  static inline int pstate_enable(bool enable)
>  {
> @@ -626,6 +622,15 @@ static int __init amd_pstate_init(void)
>  
>  	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
>  		return -ENODEV;
> +	/*
> +	 * by default the pstate driver is disabled to load
> +	 * enable the amd_pstate passive mode driver explicitly
> +	 * with amd_pstate=passive in kernel command line
> +	 */
> +	if (!cppc_load) {
> +		pr_debug("driver load is disabled, boot with amd_pstate=passive to enable this\n");
> +		return -ENODEV;
> +	}
>  
>  	if (!acpi_cpc_valid()) {
>  		pr_warn_once("the _CPC object is not present in SBIOS or ACPI disabled\n");
> @@ -640,13 +645,11 @@ static int __init amd_pstate_init(void)
>  	if (boot_cpu_has(X86_FEATURE_CPPC)) {
>  		pr_debug("AMD CPPC MSR based functionality is supported\n");
>  		amd_pstate_driver.adjust_perf = amd_pstate_adjust_perf;
> -	} else if (shared_mem) {
> +	} else {
> +		pr_debug("AMD CPPC shared memory based functionality is supported\n");
>  		static_call_update(amd_pstate_enable, cppc_enable);
>  		static_call_update(amd_pstate_init_perf, cppc_init_perf);
>  		static_call_update(amd_pstate_update_perf, cppc_update_perf);
> -	} else {
> -		pr_info("This processor supports shared memory solution, you can enable it with amd_pstate.shared_mem=1\n");
> -		return -ENODEV;
>  	}
>  
>  	/* enable amd pstate feature */
> @@ -665,6 +668,21 @@ static int __init amd_pstate_init(void)
>  }
>  device_initcall(amd_pstate_init);
>  
> +static int __init amd_pstate_param(char *str)
> +{
> +	if (!str)
> +		return -EINVAL;
> +
> +	if (!strcmp(str, "disable")) {
> +		cppc_load = 0;
> +		pr_info("driver is explicitly disabled\n");
> +	} else if (!strcmp(str, "passive"))
> +		cppc_load = 1;
> +
> +	return 0;
> +}
> +early_param("amd_pstate", amd_pstate_param);
> +
>  MODULE_AUTHOR("Huang Rui <ray.huang@xxxxxxx>");
>  MODULE_DESCRIPTION("AMD Processor P-state Frequency Driver");
>  MODULE_LICENSE("GPL");

-- 
Thanks & Regards,
Wyes