| Message ID | 20230615063225.4029929-1-perry.yuan@amd.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Enable amd-pstate active mode by default | expand |
On Thu, Jun 15, 2023 at 02:32:25PM +0800, Yuan, Perry wrote: > From: Mario Limonciello <mario.limonciello@amd.com> > > If a user's configuration doesn't explicitly specify the cpufreq > scaling governor then the code currently explicitly falls back to > 'powersave'. This default is fine for notebooks and desktops, but May I know if the processor is powerful desktop such as threadripper, whether it will be default to 'performance' or 'powersave'? Thanks, Ray > servers and undefined machines should default to 'performance'. > > Look at the 'preferred_profile' field from the FADT to set this > policy accordingly. > > Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/05_ACPI_Software_Programming_Model/ACPI_Software_Programming_Model.html#fixed-acpi-description-table-fadt > Suggested-by: Wyes Karny <Wyes.Karny@amd.com> > Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > drivers/cpufreq/amd-pstate.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index ddd346a239e0..c9d296ebf81e 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -1102,10 +1102,13 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) > policy->max = policy->cpuinfo.max_freq; > > /* > - * Set the policy to powersave to provide a valid fallback value in case > + * Set the policy to provide a valid fallback value in case > * the default cpufreq governor is neither powersave nor performance. > */ > - policy->policy = CPUFREQ_POLICY_POWERSAVE; > + if (acpi_pm_profile_server() || acpi_pm_profile_undefined()) > + policy->policy = CPUFREQ_POLICY_PERFORMANCE; > + else > + policy->policy = CPUFREQ_POLICY_POWERSAVE; > > if (boot_cpu_has(X86_FEATURE_CPPC)) { > ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, &value); > -- > 2.34.1 >
On 6/20/2023 9:58 AM, Huang Rui wrote: > On Thu, Jun 15, 2023 at 02:32:25PM +0800, Yuan, Perry wrote: >> From: Mario Limonciello <mario.limonciello@amd.com> >> >> If a user's configuration doesn't explicitly specify the cpufreq >> scaling governor then the code currently explicitly falls back to >> 'powersave'. This default is fine for notebooks and desktops, but > May I know if the processor is powerful desktop such as threadripper, > whether it will be default to 'performance' or 'powersave'? It's currently defaulting to 'powersave' for desktops and workstations. Do you think we should adopt performance for these? > > Thanks, > Ray > >> servers and undefined machines should default to 'performance'. >> >> Look at the 'preferred_profile' field from the FADT to set this >> policy accordingly. >> >> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/05_ACPI_Software_Programming_Model/ACPI_Software_Programming_Model.html#fixed-acpi-description-table-fadt >> Suggested-by: Wyes Karny <Wyes.Karny@amd.com> >> Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> drivers/cpufreq/amd-pstate.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c >> index ddd346a239e0..c9d296ebf81e 100644 >> --- a/drivers/cpufreq/amd-pstate.c >> +++ b/drivers/cpufreq/amd-pstate.c >> @@ -1102,10 +1102,13 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) >> policy->max = policy->cpuinfo.max_freq; >> >> /* >> - * Set the policy to powersave to provide a valid fallback value in case >> + * Set the policy to provide a valid fallback value in case >> * the default cpufreq governor is neither powersave nor performance. >> */ >> - policy->policy = CPUFREQ_POLICY_POWERSAVE; >> + if (acpi_pm_profile_server() || acpi_pm_profile_undefined()) >> + policy->policy = CPUFREQ_POLICY_PERFORMANCE; >> + else >> + policy->policy = CPUFREQ_POLICY_POWERSAVE; >> >> if (boot_cpu_has(X86_FEATURE_CPPC)) { >> ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, &value); >> -- >> 2.34.1 >>
On Tue, Jun 20, 2023 at 11:02:00PM +0800, Limonciello, Mario wrote: > > On 6/20/2023 9:58 AM, Huang Rui wrote: > > On Thu, Jun 15, 2023 at 02:32:25PM +0800, Yuan, Perry wrote: > >> From: Mario Limonciello <mario.limonciello@amd.com> > >> > >> If a user's configuration doesn't explicitly specify the cpufreq > >> scaling governor then the code currently explicitly falls back to > >> 'powersave'. This default is fine for notebooks and desktops, but > > May I know if the processor is powerful desktop such as threadripper, > > whether it will be default to 'performance' or 'powersave'? > It's currently defaulting to 'powersave' for desktops and > workstations. > > Do you think we should adopt performance for these? Yes, I didn't see any different use cases here between server and threadripper here. Or I missed anything? Do we have a way to separate them? Thanks, Ray > > > > > Thanks, > > Ray > > > >> servers and undefined machines should default to 'performance'. > >> > >> Look at the 'preferred_profile' field from the FADT to set this > >> policy accordingly. > >> > >> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/05_ACPI_Software_Programming_Model/ACPI_Software_Programming_Model.html#fixed-acpi-description-table-fadt > >> Suggested-by: Wyes Karny <Wyes.Karny@amd.com> > >> Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com> > >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > >> --- > >> drivers/cpufreq/amd-pstate.c | 7 +++++-- > >> 1 file changed, 5 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > >> index ddd346a239e0..c9d296ebf81e 100644 > >> --- a/drivers/cpufreq/amd-pstate.c > >> +++ b/drivers/cpufreq/amd-pstate.c > >> @@ -1102,10 +1102,13 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) > >> policy->max = policy->cpuinfo.max_freq; > >> > >> /* > >> - * Set the policy to powersave to provide a valid fallback value in case > >> + * Set the policy to provide a valid fallback value in case > >> * the default cpufreq governor is neither powersave nor performance. > >> */ > >> - policy->policy = CPUFREQ_POLICY_POWERSAVE; > >> + if (acpi_pm_profile_server() || acpi_pm_profile_undefined()) > >> + policy->policy = CPUFREQ_POLICY_PERFORMANCE; > >> + else > >> + policy->policy = CPUFREQ_POLICY_POWERSAVE; > >> > >> if (boot_cpu_has(X86_FEATURE_CPPC)) { > >> ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, &value); > >> -- > >> 2.34.1 > >>
On 6/20/2023 10:06 AM, Huang Rui wrote: > On Tue, Jun 20, 2023 at 11:02:00PM +0800, Limonciello, Mario wrote: >> On 6/20/2023 9:58 AM, Huang Rui wrote: >>> On Thu, Jun 15, 2023 at 02:32:25PM +0800, Yuan, Perry wrote: >>>> From: Mario Limonciello <mario.limonciello@amd.com> >>>> >>>> If a user's configuration doesn't explicitly specify the cpufreq >>>> scaling governor then the code currently explicitly falls back to >>>> 'powersave'. This default is fine for notebooks and desktops, but >>> May I know if the processor is powerful desktop such as threadripper, >>> whether it will be default to 'performance' or 'powersave'? >> It's currently defaulting to 'powersave' for desktops and >> workstations. >> >> Do you think we should adopt performance for these? > Yes, I didn't see any different use cases here between server and > threadripper here. Or I missed anything? Workstations and Desktops usually have to go through energy consumption certifications. Couldn't setting it to performance be inappropriate for those? > Do we have a way to separate them? If Threadripper identified as 3 Workstation I'd agree; but I'd think we're going to lump AM4/AM5 desktops along with Threadripper. So should we still set all those to performance? > > Thanks, > Ray > >>> Thanks, >>> Ray >>> >>>> servers and undefined machines should default to 'performance'. >>>> >>>> Look at the 'preferred_profile' field from the FADT to set this >>>> policy accordingly. >>>> >>>> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/05_ACPI_Software_Programming_Model/ACPI_Software_Programming_Model.html#fixed-acpi-description-table-fadt >>>> Suggested-by: Wyes Karny <Wyes.Karny@amd.com> >>>> Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com> >>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>>> --- >>>> drivers/cpufreq/amd-pstate.c | 7 +++++-- >>>> 1 file changed, 5 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c >>>> index ddd346a239e0..c9d296ebf81e 100644 >>>> --- a/drivers/cpufreq/amd-pstate.c >>>> +++ b/drivers/cpufreq/amd-pstate.c >>>> @@ -1102,10 +1102,13 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) >>>> policy->max = policy->cpuinfo.max_freq; >>>> >>>> /* >>>> - * Set the policy to powersave to provide a valid fallback value in case >>>> + * Set the policy to provide a valid fallback value in case >>>> * the default cpufreq governor is neither powersave nor performance. >>>> */ >>>> - policy->policy = CPUFREQ_POLICY_POWERSAVE; >>>> + if (acpi_pm_profile_server() || acpi_pm_profile_undefined()) >>>> + policy->policy = CPUFREQ_POLICY_PERFORMANCE; >>>> + else >>>> + policy->policy = CPUFREQ_POLICY_POWERSAVE; >>>> >>>> if (boot_cpu_has(X86_FEATURE_CPPC)) { >>>> ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, &value); >>>> -- >>>> 2.34.1 >>>>
On Tue, Jun 20, 2023 at 11:18:30PM +0800, Limonciello, Mario wrote: > > On 6/20/2023 10:06 AM, Huang Rui wrote: > > On Tue, Jun 20, 2023 at 11:02:00PM +0800, Limonciello, Mario wrote: > >> On 6/20/2023 9:58 AM, Huang Rui wrote: > >>> On Thu, Jun 15, 2023 at 02:32:25PM +0800, Yuan, Perry wrote: > >>>> From: Mario Limonciello <mario.limonciello@amd.com> > >>>> > >>>> If a user's configuration doesn't explicitly specify the cpufreq > >>>> scaling governor then the code currently explicitly falls back to > >>>> 'powersave'. This default is fine for notebooks and desktops, but > >>> May I know if the processor is powerful desktop such as threadripper, > >>> whether it will be default to 'performance' or 'powersave'? > >> It's currently defaulting to 'powersave' for desktops and > >> workstations. > >> > >> Do you think we should adopt performance for these? > > Yes, I didn't see any different use cases here between server and > > threadripper here. Or I missed anything? > Workstations and Desktops usually have to go through energy > consumption certifications. Couldn't setting it to performance be > inappropriate for those? Hmm, that makes sense. Energy consumption certification is sufficient reason. > > Do we have a way to separate them? > > If Threadripper identified as > > 3 Workstation > > I'd agree; but I'd think we're going to lump AM4/AM5 desktops > along with Threadripper. So should we still set all those to performance? > If we don't have good way to separate them, we can set them to powersave at this moment with your original patches. But I think we would better dig out a method in future. Please add my Acks for these series in next version. Acked-by: Huang Rui <ray.huang@amd.com> > > > > Thanks, > > Ray > > > >>> Thanks, > >>> Ray > >>> > >>>> servers and undefined machines should default to 'performance'. > >>>> > >>>> Look at the 'preferred_profile' field from the FADT to set this > >>>> policy accordingly. > >>>> > >>>> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/05_ACPI_Software_Programming_Model/ACPI_Software_Programming_Model.html#fixed-acpi-description-table-fadt > >>>> Suggested-by: Wyes Karny <Wyes.Karny@amd.com> > >>>> Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com> > >>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > >>>> --- > >>>> drivers/cpufreq/amd-pstate.c | 7 +++++-- > >>>> 1 file changed, 5 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > >>>> index ddd346a239e0..c9d296ebf81e 100644 > >>>> --- a/drivers/cpufreq/amd-pstate.c > >>>> +++ b/drivers/cpufreq/amd-pstate.c > >>>> @@ -1102,10 +1102,13 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) > >>>> policy->max = policy->cpuinfo.max_freq; > >>>> > >>>> /* > >>>> - * Set the policy to powersave to provide a valid fallback value in case > >>>> + * Set the policy to provide a valid fallback value in case > >>>> * the default cpufreq governor is neither powersave nor performance. > >>>> */ > >>>> - policy->policy = CPUFREQ_POLICY_POWERSAVE; > >>>> + if (acpi_pm_profile_server() || acpi_pm_profile_undefined()) > >>>> + policy->policy = CPUFREQ_POLICY_PERFORMANCE; > >>>> + else > >>>> + policy->policy = CPUFREQ_POLICY_POWERSAVE; > >>>> > >>>> if (boot_cpu_has(X86_FEATURE_CPPC)) { > >>>> ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, &value); > >>>> -- > >>>> 2.34.1 > >>>>
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index ddd346a239e0..c9d296ebf81e 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -1102,10 +1102,13 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) policy->max = policy->cpuinfo.max_freq; /* - * Set the policy to powersave to provide a valid fallback value in case + * Set the policy to provide a valid fallback value in case * the default cpufreq governor is neither powersave nor performance. */ - policy->policy = CPUFREQ_POLICY_POWERSAVE; + if (acpi_pm_profile_server() || acpi_pm_profile_undefined()) + policy->policy = CPUFREQ_POLICY_PERFORMANCE; + else + policy->policy = CPUFREQ_POLICY_POWERSAVE; if (boot_cpu_has(X86_FEATURE_CPPC)) { ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, &value);