| Message ID | 39b55abeb278d9ae1688c0b87cb7ec8a3e37932a.1718095377.git.perry.yuan@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Mario Limonciello |
| Headers | show |
| Series | AMD Pstate Driver Fixes and Improvements | expand |
On 6/11/2024 03:52, Perry Yuan wrote: > If the `amd-pstate` driver is not loaded automatically by default, > it is because the kernel command line parameter has not been added. > To resolve this issue, it is necessary to call the `amd_pstate_set_driver()` > function to enable the desired mode (passive/active/guided) before registering > the driver instance. > This ensures that the driver is loaded correctly without relying on the kernel > command line parameter. > > Meanwhle, user can add driver mode in command line which will override > the kernel config default option. > > [ 0.917789] usb usb6: Manufacturer: Linux 6.9.0-rc6-amd-pstate-new-fix-v1 xhci-hcd > [ 0.982579] amd_pstate: failed to register with return -22 This is currently "intended" behavior. There is a comment remaining in the driver at the moment not part of this patch: /* * TODO: We need more time to fine tune processors with shared memory solution * with community together. * * There are some performance drops on the CPU benchmarks which reports from * Suse. We are co-working with them to fine tune the shared memory solution. So * 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. */ Would you say that the performance drops are worked out on the shared memory designs? * If so; this comment should be dropped too in this patch. * If not; this patch really shouldn't be done as is. > > Reported-by: Andrei Amuraritei <andamu@posteo.net> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218705 > Signed-off-by: Perry Yuan <perry.yuan@amd.com> > --- > drivers/cpufreq/amd-pstate.c | 27 ++++++++++++++++++--------- > 1 file changed, 18 insertions(+), 9 deletions(-) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index fa486dfaa7e8..6e5c398810bf 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -1841,28 +1841,37 @@ static int __init amd_pstate_init(void) > /* check if this machine need CPPC quirks */ > dmi_check_system(amd_pstate_quirks_table); > > - switch (cppc_state) { > - case AMD_PSTATE_UNDEFINED: > + /* > + * get driver mode for loading from command line choice or kernel config > + * cppc_state will be AMD_PSTATE_UNDEFINED if no command line input > + * command line choice will override the kconfig option > + */ > + if (cppc_state == AMD_PSTATE_UNDEFINED) { > + pr_err("pyuan cppc_state == AMD_PSTATE_UNDEFINED \n"); Looks like a debug line escaped into the patch. > /* Disable on the following configs by default: > * 1. Undefined platforms > * 2. Server platforms > * 3. Shared memory designs > */ > if (amd_pstate_acpi_pm_profile_undefined() || > - amd_pstate_acpi_pm_profile_server() || > - !cpu_feature_enabled(X86_FEATURE_CPPC)) { > + amd_pstate_acpi_pm_profile_server()) { > pr_info("driver load is disabled, boot with specific mode to enable this\n"); > return -ENODEV; > } > - ret = amd_pstate_set_driver(CONFIG_X86_AMD_PSTATE_DEFAULT_MODE); > - if (ret) > - return ret; > - break; > + /* get driver mode from kernel config option [1:4] */ > + cppc_state = CONFIG_X86_AMD_PSTATE_DEFAULT_MODE; > + } > + > + switch (cppc_state) { > case AMD_PSTATE_DISABLE: > + pr_info("driver load is disabled, boot with specific mode to enable this\n"); > return -ENODEV; > case AMD_PSTATE_PASSIVE: > case AMD_PSTATE_ACTIVE: > case AMD_PSTATE_GUIDED: > + ret = amd_pstate_set_driver(cppc_state); > + if (ret) > + return ret; > break; > default: > return -EINVAL; > @@ -1883,7 +1892,7 @@ static int __init amd_pstate_init(void) > /* enable amd pstate feature */ > ret = amd_pstate_enable(true); > if (ret) { > - pr_err("failed to enable with return %d\n", ret); > + pr_err("failed to enable driver mode(%d)\n", cppc_state); > return ret; > } >
Hello Perry, Perry Yuan <perry.yuan@amd.com> writes: > If the `amd-pstate` driver is not loaded automatically by default, > it is because the kernel command line parameter has not been added. > To resolve this issue, it is necessary to call the `amd_pstate_set_driver()` > function to enable the desired mode (passive/active/guided) before registering > the driver instance. > This ensures that the driver is loaded correctly without relying on the kernel > command line parameter. > > Meanwhle, user can add driver mode in command line which will override > the kernel config default option. > > [ 0.917789] usb usb6: Manufacturer: Linux 6.9.0-rc6-amd-pstate-new-fix-v1 xhci-hcd > [ 0.982579] amd_pstate: failed to register with return -22 > > Reported-by: Andrei Amuraritei <andamu@posteo.net> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218705 > Signed-off-by: Perry Yuan <perry.yuan@amd.com> > --- > drivers/cpufreq/amd-pstate.c | 27 ++++++++++++++++++--------- > 1 file changed, 18 insertions(+), 9 deletions(-) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index fa486dfaa7e8..6e5c398810bf 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -1841,28 +1841,37 @@ static int __init amd_pstate_init(void) > /* check if this machine need CPPC quirks */ > dmi_check_system(amd_pstate_quirks_table); > > - switch (cppc_state) { > - case AMD_PSTATE_UNDEFINED: > + /* > + * get driver mode for loading from command line choice or kernel config > + * cppc_state will be AMD_PSTATE_UNDEFINED if no command line input > + * command line choice will override the kconfig option > + */ > + if (cppc_state == AMD_PSTATE_UNDEFINED) { > + pr_err("pyuan cppc_state == AMD_PSTATE_UNDEFINED \n"); As Mario pointed out, this needs to be removed :-) The following review comments are assuming that you want this patch so that amd-pstate is the default driver on shared-memory non-server platforms. > /* Disable on the following configs by default: > * 1. Undefined platforms > * 2. Server platforms > * 3. Shared memory designs The comment says the driver needs to be disabled on the shared memory designs by default. But... > */ > if (amd_pstate_acpi_pm_profile_undefined() || > - amd_pstate_acpi_pm_profile_server() || > - !cpu_feature_enabled(X86_FEATURE_CPPC)) { ...the check for shared-memory design has been removed. Is this intentional ? So do you want the comment to be fixed so that it is clear that we want amd-pstate to be the default driver on non-server platforms ? > + amd_pstate_acpi_pm_profile_server()) { > pr_info("driver load is disabled, boot with specific mode to enable this\n"); > return -ENODEV; > } > - ret = amd_pstate_set_driver(CONFIG_X86_AMD_PSTATE_DEFAULT_MODE); > - if (ret) > - return ret; > - break; > + /* get driver mode from kernel config option [1:4] */ > + cppc_state = CONFIG_X86_AMD_PSTATE_DEFAULT_MODE; If someone booted the system with "amd_pstate=disable", the above will overwrite that preference, no ? > + } > + > + switch (cppc_state) { > case AMD_PSTATE_DISABLE: > + pr_info("driver load is disabled, boot with specific mode to enable this\n"); > return -ENODEV; ....and this "case" statement will never be reachable in the aforementioned case. > case AMD_PSTATE_PASSIVE: > case AMD_PSTATE_ACTIVE: > case AMD_PSTATE_GUIDED: > + ret = amd_pstate_set_driver(cppc_state); > + if (ret) > + return ret; > break; > default: > return -EINVAL; > @@ -1883,7 +1892,7 @@ static int __init amd_pstate_init(void) > /* enable amd pstate feature */ > ret = amd_pstate_enable(true); > if (ret) { > - pr_err("failed to enable with return %d\n", ret); > + pr_err("failed to enable driver mode(%d)\n", cppc_state); > return ret; > } > > -- > 2.34.1
[AMD Official Use Only - AMD Internal Distribution Only] Hi Gautham > -----Original Message----- > From: Shenoy, Gautham Ranjal <gautham.shenoy@amd.com> > Sent: Thursday, June 13, 2024 4:19 PM > To: Yuan, Perry <Perry.Yuan@amd.com>; Limonciello, Mario > <Mario.Limonciello@amd.com> > Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Huang, Ray > <Ray.Huang@amd.com>; Petkov, Borislav <Borislav.Petkov@amd.com>; > Deucher, Alexander <Alexander.Deucher@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 v3 10/10] cpufreq: amd-pstate: automatically load pstate > driver by default > > Hello Perry, > > > Perry Yuan <perry.yuan@amd.com> writes: > > > If the `amd-pstate` driver is not loaded automatically by default, it > > is because the kernel command line parameter has not been added. > > To resolve this issue, it is necessary to call the > > `amd_pstate_set_driver()` function to enable the desired mode > > (passive/active/guided) before registering the driver instance. > > This ensures that the driver is loaded correctly without relying on > > the kernel command line parameter. > > > > Meanwhle, user can add driver mode in command line which will override > > the kernel config default option. > > > > [ 0.917789] usb usb6: Manufacturer: Linux 6.9.0-rc6-amd-pstate-new-fix- > v1 xhci-hcd > > [ 0.982579] amd_pstate: failed to register with return -22 > > > > Reported-by: Andrei Amuraritei <andamu@posteo.net> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218705 > > Signed-off-by: Perry Yuan <perry.yuan@amd.com> > > --- > > drivers/cpufreq/amd-pstate.c | 27 ++++++++++++++++++--------- > > 1 file changed, 18 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/cpufreq/amd-pstate.c > > b/drivers/cpufreq/amd-pstate.c index fa486dfaa7e8..6e5c398810bf 100644 > > --- a/drivers/cpufreq/amd-pstate.c > > +++ b/drivers/cpufreq/amd-pstate.c > > @@ -1841,28 +1841,37 @@ static int __init amd_pstate_init(void) > > /* check if this machine need CPPC quirks */ > > dmi_check_system(amd_pstate_quirks_table); > > > > - switch (cppc_state) { > > - case AMD_PSTATE_UNDEFINED: > > + /* > > + * get driver mode for loading from command line choice or kernel > config > > + * cppc_state will be AMD_PSTATE_UNDEFINED if no command line > input > > + * command line choice will override the kconfig option > > + */ > > + if (cppc_state == AMD_PSTATE_UNDEFINED) { > > + pr_err("pyuan cppc_state == AMD_PSTATE_UNDEFINED \n"); > > As Mario pointed out, this needs to be removed :-) Yep , I forget to delete it when sending the patches ☹ > > The following review comments are assuming that you want this patch so > that amd-pstate is the default driver on shared-memory non-server > platforms. > > > > /* Disable on the following configs by default: > > * 1. Undefined platforms > > * 2. Server platforms > > * 3. Shared memory designs > > The comment says the driver needs to be disabled on the shared memory > designs by default. But... The comments need to be updated looks like. > > > > > */ > > if (amd_pstate_acpi_pm_profile_undefined() || > > - amd_pstate_acpi_pm_profile_server() || > > - !cpu_feature_enabled(X86_FEATURE_CPPC)) { > > ...the check for shared-memory design has been removed. Is this > intentional ? So do you want the comment to be fixed so that it is clear that > we want amd-pstate to be the default driver on non-server platforms ? We are going to make the pstate driver as default driver on client CPUs, server CPU will not load pstate drivers when no driver mode command line added. I believe it will return TRUE on all server CPUs with " amd_pstate_acpi_pm_profile_server() ", so server CPU is not impacted. Even the shared memory type client system, I guess it is OK to load pstate driver since this change.(Just a few generations CPU using shared memory) Some customers reported that TR40 system(shared memory) has better performance than acpi_cpufreq. Users can disable the pstate driver from kernel config or command line, from the initial kernel config patches made by Mario, the purpose is load EPP driver by default. It just had a few unexpected loading issues fixed by this patch. > > > > > + amd_pstate_acpi_pm_profile_server()) { > > pr_info("driver load is disabled, boot with specific > mode to enable this\n"); > > return -ENODEV; > > } > > - ret = > amd_pstate_set_driver(CONFIG_X86_AMD_PSTATE_DEFAULT_MODE); > > - if (ret) > > - return ret; > > - break; > > + /* get driver mode from kernel config option [1:4] */ > > + cppc_state = CONFIG_X86_AMD_PSTATE_DEFAULT_MODE; > > If someone booted the system with "amd_pstate=disable", the above will > overwrite that preference, no ? Command line will override kernel config anyway. amd_pstate=disable will work or disable from Kconfig. > > > + } > > + > > + switch (cppc_state) { > > case AMD_PSTATE_DISABLE: > > + pr_info("driver load is disabled, boot with specific mode to > enable > > +this\n"); > > return -ENODEV; > > ....and this "case" statement will never be reachable in the aforementioned > case. It will be called when kernel config set "disable" state from kernel booting. > > > > case AMD_PSTATE_PASSIVE: > > case AMD_PSTATE_ACTIVE: > > case AMD_PSTATE_GUIDED: > > + ret = amd_pstate_set_driver(cppc_state); > > + if (ret) > > + return ret; > > break; > > default: > > return -EINVAL; > > @@ -1883,7 +1892,7 @@ static int __init amd_pstate_init(void) > > /* enable amd pstate feature */ > > ret = amd_pstate_enable(true); > > if (ret) { > > - pr_err("failed to enable with return %d\n", ret); > > + pr_err("failed to enable driver mode(%d)\n", cppc_state); > > return ret; > > } > > > > -- > > 2.34.1
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index fa486dfaa7e8..6e5c398810bf 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -1841,28 +1841,37 @@ static int __init amd_pstate_init(void) /* check if this machine need CPPC quirks */ dmi_check_system(amd_pstate_quirks_table); - switch (cppc_state) { - case AMD_PSTATE_UNDEFINED: + /* + * get driver mode for loading from command line choice or kernel config + * cppc_state will be AMD_PSTATE_UNDEFINED if no command line input + * command line choice will override the kconfig option + */ + if (cppc_state == AMD_PSTATE_UNDEFINED) { + pr_err("pyuan cppc_state == AMD_PSTATE_UNDEFINED \n"); /* Disable on the following configs by default: * 1. Undefined platforms * 2. Server platforms * 3. Shared memory designs */ if (amd_pstate_acpi_pm_profile_undefined() || - amd_pstate_acpi_pm_profile_server() || - !cpu_feature_enabled(X86_FEATURE_CPPC)) { + amd_pstate_acpi_pm_profile_server()) { pr_info("driver load is disabled, boot with specific mode to enable this\n"); return -ENODEV; } - ret = amd_pstate_set_driver(CONFIG_X86_AMD_PSTATE_DEFAULT_MODE); - if (ret) - return ret; - break; + /* get driver mode from kernel config option [1:4] */ + cppc_state = CONFIG_X86_AMD_PSTATE_DEFAULT_MODE; + } + + switch (cppc_state) { case AMD_PSTATE_DISABLE: + pr_info("driver load is disabled, boot with specific mode to enable this\n"); return -ENODEV; case AMD_PSTATE_PASSIVE: case AMD_PSTATE_ACTIVE: case AMD_PSTATE_GUIDED: + ret = amd_pstate_set_driver(cppc_state); + if (ret) + return ret; break; default: return -EINVAL; @@ -1883,7 +1892,7 @@ static int __init amd_pstate_init(void) /* enable amd pstate feature */ ret = amd_pstate_enable(true); if (ret) { - pr_err("failed to enable with return %d\n", ret); + pr_err("failed to enable driver mode(%d)\n", cppc_state); return ret; }
If the `amd-pstate` driver is not loaded automatically by default, it is because the kernel command line parameter has not been added. To resolve this issue, it is necessary to call the `amd_pstate_set_driver()` function to enable the desired mode (passive/active/guided) before registering the driver instance. This ensures that the driver is loaded correctly without relying on the kernel command line parameter. Meanwhle, user can add driver mode in command line which will override the kernel config default option. [ 0.917789] usb usb6: Manufacturer: Linux 6.9.0-rc6-amd-pstate-new-fix-v1 xhci-hcd [ 0.982579] amd_pstate: failed to register with return -22 Reported-by: Andrei Amuraritei <andamu@posteo.net> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218705 Signed-off-by: Perry Yuan <perry.yuan@amd.com> --- drivers/cpufreq/amd-pstate.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-)