diff mbox series

[v2,2/4] cpufreq: amd-pstate: Set a fallback policy based on preferred_profile

Message ID 20230615063225.4029929-1-perry.yuan@amd.com
State New
Headers
Series Enable amd-pstate active mode by default |

Commit Message

Yuan, Perry June 15, 2023, 6:32 a.m. UTC 
  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
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(-)

Comments

Huang Rui June 20, 2023, 2:58 p.m. UTC | #1 
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
>
Mario Limonciello June 20, 2023, 3:02 p.m. UTC | #2 
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
>>
Huang Rui June 20, 2023, 3:06 p.m. UTC | #3 
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
> >>
Mario Limonciello June 20, 2023, 3:18 p.m. UTC | #4 
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
>>>>
Huang Rui June 20, 2023, 3:36 p.m. UTC | #5 
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 mbox series

Patch

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);