From: Mario Limonciello <[email protected]>
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
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 <[email protected]>
Reviewed-by: Gautham R. Shenoy <[email protected]>
Signed-off-by: Mario Limonciello <[email protected]>
---
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 Thu, Jun 15, 2023 at 02:32:25PM +0800, Yuan, Perry wrote:
> From: Mario Limonciello <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Gautham R. Shenoy <[email protected]>
> Signed-off-by: Mario Limonciello <[email protected]>
> ---
> 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 <[email protected]>
> >>
> >> 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 <[email protected]>
> >> Reviewed-by: Gautham R. Shenoy <[email protected]>
> >> Signed-off-by: Mario Limonciello <[email protected]>
> >> ---
> >> 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 <[email protected]>
>>
>> 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 <[email protected]>
>> Reviewed-by: Gautham R. Shenoy <[email protected]>
>> Signed-off-by: Mario Limonciello <[email protected]>
>> ---
>> 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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Reviewed-by: Gautham R. Shenoy <[email protected]>
>>>> Signed-off-by: Mario Limonciello <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
> >>>>
> >>>> 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 <[email protected]>
> >
> > 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 <[email protected]>
> >>>> Reviewed-by: Gautham R. Shenoy <[email protected]>
> >>>> Signed-off-by: Mario Limonciello <[email protected]>
> >>>> ---
> >>>> 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
> >>>>