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

Wyes Karny
2022-11-17T05:06:36Z

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@amd.com>

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

> ---
> 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@amd.com>");
> MODULE_DESCRIPTION("AMD Processor P-state Frequency Driver");
> MODULE_LICENSE("GPL");

--
Thanks & Regards,
Wyes