[PATCH v2 0/2] cpufreq/amd-pstate: Set initial min_freq to lowest_nonlinear_freq

[PATCH v2 0/2] cpufreq/amd-pstate: Set initial min_freq to lowest_nonlinear_freq

[Dhananjay Ugwekar]

According to the AMD architectural programmer's manual volume 2 [1], 
in section "17.6.4.1 CPPC_CAPABILITY_1" lowest_nonlinear_perf is described 
as "Reports the most energy efficient performance level (in terms of 
performance per watt). Above this threshold, lower performance levels 
generally result in increased energy efficiency. Reducing performance 
below this threshold does not result in total energy savings for a given 
computation, although it reduces instantaneous power consumption". So 
lowest_nonlinear_perf is the most power efficient performance level, and 
going below that would lead to a worse performance/watt.

Also setting the minimum frequency to lowest_nonlinear_freq (instead of
lowest_freq) allows the CPU to idle at a higher frequency which leads
to more time being spent in a deeper idle state (as trivial idle tasks
are completed sooner). This has shown a power benefit in some systems.
In other systems, power consumption has increased but so has the
throughput/watt.

Our objective here is to update the initial lower frequency limit to 
lowest_nonlinear_freq, while allowing the user to later update the lower 
limit to anywhere between lowest_freq to highest_freq for the platform.

So, set the policy->min to lowest_nonlinear_freq in the ->verify() 
callback, only if the original value is equal to FREQ_QOS_MIN_DEFAULT_VALUE
(i.e. 0). Merge the two identical verify functions while at it.

Link: https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24593.pdf [1]

Changes from v1:
* Modify the initial min_freq from verify callback, instead of adding a
  new callback in cpufreq_driver struct. (Rafael)

v1 Link: https://lore.kernel.org/linux-pm/20241003083952.3186-1-Dhananjay.Ugwekar@amd.com/

Dhananjay Ugwekar (2):
  cpufreq/amd-pstate: Remove the redundant verify() function
  cpufreq/amd-pstate: Set the initial min_freq to lowest_nonlinear_freq

 drivers/cpufreq/amd-pstate.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] cpufreq/amd-pstate: Remove the redundant verify() function

[Dhananjay Ugwekar]

Merge the two verify() callback functions and rename the
cpufreq_policy_data argument for better readability.

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index b7a17a3ef122..fa16d72d6058 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -527,10 +527,10 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
 	cpufreq_cpu_put(policy);
 }
 
-static int amd_pstate_verify(struct cpufreq_policy_data *policy)
+static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
 {
-	cpufreq_verify_within_cpu_limits(policy);
-
+	cpufreq_verify_within_cpu_limits(policy_data);
+	pr_debug("policy_max =%d, policy_min=%d\n", policy_data->max, policy_data->min);
 	return 0;
 }
 
@@ -1661,13 +1661,6 @@ static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
 	return 0;
 }
 
-static int amd_pstate_epp_verify_policy(struct cpufreq_policy_data *policy)
-{
-	cpufreq_verify_within_cpu_limits(policy);
-	pr_debug("policy_max =%d, policy_min=%d\n", policy->max, policy->min);
-	return 0;
-}
-
 static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
@@ -1723,7 +1716,7 @@ static struct cpufreq_driver amd_pstate_driver = {
 
 static struct cpufreq_driver amd_pstate_epp_driver = {
 	.flags		= CPUFREQ_CONST_LOOPS,
-	.verify		= amd_pstate_epp_verify_policy,
+	.verify		= amd_pstate_verify,
 	.setpolicy	= amd_pstate_epp_set_policy,
 	.init		= amd_pstate_epp_cpu_init,
 	.exit		= amd_pstate_epp_cpu_exit,
-- 
2.34.1

[PATCH v2 2/2] cpufreq/amd-pstate: Set the initial min_freq to lowest_nonlinear_freq

[Dhananjay Ugwekar]

According to the AMD architectural programmer's manual volume 2 [1], in
section "17.6.4.1 CPPC_CAPABILITY_1" lowest_nonlinear_perf is described
as "Reports the most energy efficient performance level (in terms of
performance per watt). Above this threshold, lower performance levels
generally result in increased energy efficiency. Reducing performance
below this threshold does not result in total energy savings for a given
computation, although it reduces instantaneous power consumption". So
lowest_nonlinear_perf is the most power efficient performance level, and
going below that would lead to a worse performance/watt.

Also, setting the minimum frequency to lowest_nonlinear_freq (instead of
lowest_freq) allows the CPU to idle at a higher frequency which leads
to more time being spent in a deeper idle state (as trivial idle tasks
are completed sooner). This has shown a power benefit in some systems,
in other systems, power consumption has increased but so has the
throughput/watt.

Modify the initial policy_data->min passed by cpufreq core to
lowest_nonlinear_freq, in the ->verify() callback. Also set the
qos_request cpudata->req[0] to FREQ_QOS_MIN_DEFAULT_VALUE (i.e. 0), 
so that it also gets overridden by the check in verify function.

Link: https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24593.pdf [1]

Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index fa16d72d6058..117ad5988e8e 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -529,8 +529,20 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
 
 static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
 {
+	struct cpufreq_policy *policy = cpufreq_cpu_get(policy_data->cpu);
+	struct amd_cpudata *cpudata = policy->driver_data;
+
+	if (!policy)
+		return -EINVAL;
+
+	if (policy_data->min == FREQ_QOS_MIN_DEFAULT_VALUE)
+		policy_data->min = cpudata->lowest_nonlinear_freq;
+
 	cpufreq_verify_within_cpu_limits(policy_data);
 	pr_debug("policy_max =%d, policy_min=%d\n", policy_data->max, policy_data->min);
+
+	cpufreq_cpu_put(policy);
+
 	return 0;
 }
 
@@ -996,7 +1008,7 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
 		policy->fast_switch_possible = true;
 
 	ret = freq_qos_add_request(&policy->constraints, &cpudata->req[0],
-				   FREQ_QOS_MIN, policy->cpuinfo.min_freq);
+				   FREQ_QOS_MIN, FREQ_QOS_MIN_DEFAULT_VALUE);
 	if (ret < 0) {
 		dev_err(dev, "Failed to add min-freq constraint (%d)\n", ret);
 		goto free_cpudata1;
-- 
2.34.1

Re: [PATCH v2 2/2] cpufreq/amd-pstate: Set the initial min_freq to lowest_nonlinear_freq

[Mario Limonciello]

On 10/16/2024 09:46, Dhananjay Ugwekar wrote:
> According to the AMD architectural programmer's manual volume 2 [1], in
> section "17.6.4.1 CPPC_CAPABILITY_1" lowest_nonlinear_perf is described
> as "Reports the most energy efficient performance level (in terms of
> performance per watt). Above this threshold, lower performance levels
> generally result in increased energy efficiency. Reducing performance
> below this threshold does not result in total energy savings for a given
> computation, although it reduces instantaneous power consumption". So
> lowest_nonlinear_perf is the most power efficient performance level, and
> going below that would lead to a worse performance/watt.
> 
> Also, setting the minimum frequency to lowest_nonlinear_freq (instead of
> lowest_freq) allows the CPU to idle at a higher frequency which leads
> to more time being spent in a deeper idle state (as trivial idle tasks
> are completed sooner). This has shown a power benefit in some systems,
> in other systems, power consumption has increased but so has the
> throughput/watt.
> 
> Modify the initial policy_data->min passed by cpufreq core to
> lowest_nonlinear_freq, in the ->verify() callback. Also set the
> qos_request cpudata->req[0] to FREQ_QOS_MIN_DEFAULT_VALUE (i.e. 0),
> so that it also gets overridden by the check in verify function.
> 
> Link: https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24593.pdf [1]
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
> ---
>   drivers/cpufreq/amd-pstate.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index fa16d72d6058..117ad5988e8e 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -529,8 +529,20 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
>   
>   static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
>   {
> +	struct cpufreq_policy *policy = cpufreq_cpu_get(policy_data->cpu);
> +	struct amd_cpudata *cpudata = policy->driver_data;

This /could/ be a NULL pointer de-reference.  It should have been 
initialized after the "if (!policy)" check.

It's a one line change though to initialize at the right place so I'll 
do some testing on the series though with that manually fixed up and if 
there are no other problems I'll take it.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> +
> +	if (!policy)
> +		return -EINVAL;
> +
> +	if (policy_data->min == FREQ_QOS_MIN_DEFAULT_VALUE)
> +		policy_data->min = cpudata->lowest_nonlinear_freq;
> +
>   	cpufreq_verify_within_cpu_limits(policy_data);
>   	pr_debug("policy_max =%d, policy_min=%d\n", policy_data->max, policy_data->min);
> +
> +	cpufreq_cpu_put(policy);
> +
>   	return 0;
>   }
>   
> @@ -996,7 +1008,7 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>   		policy->fast_switch_possible = true;
>   
>   	ret = freq_qos_add_request(&policy->constraints, &cpudata->req[0],
> -				   FREQ_QOS_MIN, policy->cpuinfo.min_freq);
> +				   FREQ_QOS_MIN, FREQ_QOS_MIN_DEFAULT_VALUE);
>   	if (ret < 0) {
>   		dev_err(dev, "Failed to add min-freq constraint (%d)\n", ret);
>   		goto free_cpudata1;

Re: [PATCH v2 1/2] cpufreq/amd-pstate: Remove the redundant verify() function

[Mario Limonciello]

On 10/16/2024 09:46, Dhananjay Ugwekar wrote:
> Merge the two verify() callback functions and rename the
> cpufreq_policy_data argument for better readability.
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/cpufreq/amd-pstate.c | 15 ++++-----------
>   1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index b7a17a3ef122..fa16d72d6058 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -527,10 +527,10 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
>   	cpufreq_cpu_put(policy);
>   }
>   
> -static int amd_pstate_verify(struct cpufreq_policy_data *policy)
> +static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
>   {
> -	cpufreq_verify_within_cpu_limits(policy);
> -
> +	cpufreq_verify_within_cpu_limits(policy_data);
> +	pr_debug("policy_max =%d, policy_min=%d\n", policy_data->max, policy_data->min);
>   	return 0;
>   }
>   
> @@ -1661,13 +1661,6 @@ static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
>   	return 0;
>   }
>   
> -static int amd_pstate_epp_verify_policy(struct cpufreq_policy_data *policy)
> -{
> -	cpufreq_verify_within_cpu_limits(policy);
> -	pr_debug("policy_max =%d, policy_min=%d\n", policy->max, policy->min);
> -	return 0;
> -}
> -
>   static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
>   {
>   	struct amd_cpudata *cpudata = policy->driver_data;
> @@ -1723,7 +1716,7 @@ static struct cpufreq_driver amd_pstate_driver = {
>   
>   static struct cpufreq_driver amd_pstate_epp_driver = {
>   	.flags		= CPUFREQ_CONST_LOOPS,
> -	.verify		= amd_pstate_epp_verify_policy,
> +	.verify		= amd_pstate_verify,
>   	.setpolicy	= amd_pstate_epp_set_policy,
>   	.init		= amd_pstate_epp_cpu_init,
>   	.exit		= amd_pstate_epp_cpu_exit,

Re: [PATCH v2 1/2] cpufreq/amd-pstate: Remove the redundant verify() function

[Gautham R. Shenoy]

Hello Dhananjay,

On Wed, Oct 16, 2024 at 02:46:40PM +0000, Dhananjay Ugwekar wrote:
> Merge the two verify() callback functions and rename the
> cpufreq_policy_data argument for better readability.

Thanks for cleaning this up.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.

> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index b7a17a3ef122..fa16d72d6058 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -527,10 +527,10 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
>  	cpufreq_cpu_put(policy);
>  }
>  
> -static int amd_pstate_verify(struct cpufreq_policy_data *policy)
> +static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
>  {
> -	cpufreq_verify_within_cpu_limits(policy);
> -
> +	cpufreq_verify_within_cpu_limits(policy_data);
> +	pr_debug("policy_max =%d, policy_min=%d\n", policy_data->max, policy_data->min);
>  	return 0;
>  }
>  
> @@ -1661,13 +1661,6 @@ static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
>  	return 0;
>  }
>  
> -static int amd_pstate_epp_verify_policy(struct cpufreq_policy_data *policy)
> -{
> -	cpufreq_verify_within_cpu_limits(policy);
> -	pr_debug("policy_max =%d, policy_min=%d\n", policy->max, policy->min);
> -	return 0;
> -}
> -
>  static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
>  {
>  	struct amd_cpudata *cpudata = policy->driver_data;
> @@ -1723,7 +1716,7 @@ static struct cpufreq_driver amd_pstate_driver = {
>  
>  static struct cpufreq_driver amd_pstate_epp_driver = {
>  	.flags		= CPUFREQ_CONST_LOOPS,
> -	.verify		= amd_pstate_epp_verify_policy,
> +	.verify		= amd_pstate_verify,
>  	.setpolicy	= amd_pstate_epp_set_policy,
>  	.init		= amd_pstate_epp_cpu_init,
>  	.exit		= amd_pstate_epp_cpu_exit,
> -- 
> 2.34.1
> 

Re: [PATCH v2 2/2] cpufreq/amd-pstate: Set the initial min_freq to lowest_nonlinear_freq

[Gautham R. Shenoy]

Hello Dhananjay,

On Wed, Oct 16, 2024 at 02:46:42PM +0000, Dhananjay Ugwekar wrote:
> According to the AMD architectural programmer's manual volume 2 [1], in
> section "17.6.4.1 CPPC_CAPABILITY_1" lowest_nonlinear_perf is described
> as "Reports the most energy efficient performance level (in terms of
> performance per watt). Above this threshold, lower performance levels
> generally result in increased energy efficiency. Reducing performance
> below this threshold does not result in total energy savings for a given
> computation, although it reduces instantaneous power consumption". So
> lowest_nonlinear_perf is the most power efficient performance level, and
> going below that would lead to a worse performance/watt.
> 
> Also, setting the minimum frequency to lowest_nonlinear_freq (instead of
> lowest_freq) allows the CPU to idle at a higher frequency which leads
> to more time being spent in a deeper idle state (as trivial idle tasks
> are completed sooner). This has shown a power benefit in some systems,
> in other systems, power consumption has increased but so has the
> throughput/watt.
> 
> Modify the initial policy_data->min passed by cpufreq core to
> lowest_nonlinear_freq, in the ->verify() callback. Also set the
> qos_request cpudata->req[0] to FREQ_QOS_MIN_DEFAULT_VALUE (i.e. 0), 
> so that it also gets overridden by the check in verify function.
> 
> Link: https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24593.pdf [1]
> 
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index fa16d72d6058..117ad5988e8e 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -529,8 +529,20 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
>  
>  static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
>  {
> +	struct cpufreq_policy *policy = cpufreq_cpu_get(policy_data->cpu);
> +	struct amd_cpudata *cpudata = policy->driver_data;
> +
> +	if (!policy)
> +		return -EINVAL;
> +
> +	if (policy_data->min == FREQ_QOS_MIN_DEFAULT_VALUE)
> +		policy_data->min = cpudata->lowest_nonlinear_freq;

Why not unconditionally set policy->min to lowest_nonlinear_freq ?


> +
>  	cpufreq_verify_within_cpu_limits(policy_data);
>  	pr_debug("policy_max =%d, policy_min=%d\n", policy_data->max, policy_data->min);
> +
> +	cpufreq_cpu_put(policy);
> +
>  	return 0;
>  }
>  
> @@ -996,7 +1008,7 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>  		policy->fast_switch_possible = true;
>  
>  	ret = freq_qos_add_request(&policy->constraints, &cpudata->req[0],
> -				   FREQ_QOS_MIN, policy->cpuinfo.min_freq);
> +				   FREQ_QOS_MIN, FREQ_QOS_MIN_DEFAULT_VALUE);


This qos request can still be set to cpuinfo.min_freq, no ? Especially
if you unconditionally initialize policy->min to lowest_nonlinear_freq
in amd_pstate_policy, no?

--
Thanks and Regards
gautham.

Re: [PATCH v2 2/2] cpufreq/amd-pstate: Set the initial min_freq to lowest_nonlinear_freq

[Dhananjay Ugwekar]

Hello Gautham,

On 10/17/2024 9:26 AM, Gautham R. Shenoy wrote:
> Hello Dhananjay,
> 
> On Wed, Oct 16, 2024 at 02:46:42PM +0000, Dhananjay Ugwekar wrote:
>> According to the AMD architectural programmer's manual volume 2 [1], in
>> section "17.6.4.1 CPPC_CAPABILITY_1" lowest_nonlinear_perf is described
>> as "Reports the most energy efficient performance level (in terms of
>> performance per watt). Above this threshold, lower performance levels
>> generally result in increased energy efficiency. Reducing performance
>> below this threshold does not result in total energy savings for a given
>> computation, although it reduces instantaneous power consumption". So
>> lowest_nonlinear_perf is the most power efficient performance level, and
>> going below that would lead to a worse performance/watt.
>>
>> Also, setting the minimum frequency to lowest_nonlinear_freq (instead of
>> lowest_freq) allows the CPU to idle at a higher frequency which leads
>> to more time being spent in a deeper idle state (as trivial idle tasks
>> are completed sooner). This has shown a power benefit in some systems,
>> in other systems, power consumption has increased but so has the
>> throughput/watt.
>>
>> Modify the initial policy_data->min passed by cpufreq core to
>> lowest_nonlinear_freq, in the ->verify() callback. Also set the
>> qos_request cpudata->req[0] to FREQ_QOS_MIN_DEFAULT_VALUE (i.e. 0), 
>> so that it also gets overridden by the check in verify function.
>>
>> Link: https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24593.pdf [1]
>>
>> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
>> ---
>>  drivers/cpufreq/amd-pstate.c | 14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index fa16d72d6058..117ad5988e8e 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -529,8 +529,20 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
>>  
>>  static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
>>  {
>> +	struct cpufreq_policy *policy = cpufreq_cpu_get(policy_data->cpu);
>> +	struct amd_cpudata *cpudata = policy->driver_data;
>> +
>> +	if (!policy)
>> +		return -EINVAL;
>> +
>> +	if (policy_data->min == FREQ_QOS_MIN_DEFAULT_VALUE)
>> +		policy_data->min = cpudata->lowest_nonlinear_freq;
> 
> Why not unconditionally set policy->min to lowest_nonlinear_freq ?

That will lead to discarding all of the user's writes to scaling_min_freq and the lowest_nonlinear_freq 
will become the permanent policy->min value. Because, verify() is called in the scaling_min_freq write 
path as well. refresh_frequency_limits() --> cpufreq_set_policy() --> driver->verify().

> 
> 
>> +
>>  	cpufreq_verify_within_cpu_limits(policy_data);
>>  	pr_debug("policy_max =%d, policy_min=%d\n", policy_data->max, policy_data->min);
>> +
>> +	cpufreq_cpu_put(policy);
>> +
>>  	return 0;
>>  }
>>  
>> @@ -996,7 +1008,7 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>>  		policy->fast_switch_possible = true;
>>  
>>  	ret = freq_qos_add_request(&policy->constraints, &cpudata->req[0],
>> -				   FREQ_QOS_MIN, policy->cpuinfo.min_freq);
>> +				   FREQ_QOS_MIN, FREQ_QOS_MIN_DEFAULT_VALUE);
> 
> 
> This qos request can still be set to cpuinfo.min_freq, no ? Especially
> if you unconditionally initialize policy->min to lowest_nonlinear_freq
> in amd_pstate_policy, no?

As we cant unconditionally init the policy->min above, this needs to be set accordingly for the 
above if condition to be true (i.e. FREQ_QOS_MIN_DEFAULT_VALUE).

Thanks,
Dhananjay

> 
> --
> Thanks and Regards
> gautham.

Re: [PATCH v2 2/2] cpufreq/amd-pstate: Set the initial min_freq to lowest_nonlinear_freq

[Dhananjay Ugwekar]

Hello Mario,

On 10/16/2024 8:27 PM, Mario Limonciello wrote:
> On 10/16/2024 09:46, Dhananjay Ugwekar wrote:
>> According to the AMD architectural programmer's manual volume 2 [1], in
>> section "17.6.4.1 CPPC_CAPABILITY_1" lowest_nonlinear_perf is described
>> as "Reports the most energy efficient performance level (in terms of
>> performance per watt). Above this threshold, lower performance levels
>> generally result in increased energy efficiency. Reducing performance
>> below this threshold does not result in total energy savings for a given
>> computation, although it reduces instantaneous power consumption". So
>> lowest_nonlinear_perf is the most power efficient performance level, and
>> going below that would lead to a worse performance/watt.
>>
>> Also, setting the minimum frequency to lowest_nonlinear_freq (instead of
>> lowest_freq) allows the CPU to idle at a higher frequency which leads
>> to more time being spent in a deeper idle state (as trivial idle tasks
>> are completed sooner). This has shown a power benefit in some systems,
>> in other systems, power consumption has increased but so has the
>> throughput/watt.
>>
>> Modify the initial policy_data->min passed by cpufreq core to
>> lowest_nonlinear_freq, in the ->verify() callback. Also set the
>> qos_request cpudata->req[0] to FREQ_QOS_MIN_DEFAULT_VALUE (i.e. 0),
>> so that it also gets overridden by the check in verify function.
>>
>> Link: https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24593.pdf [1]
>>
>> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
>> ---
>>   drivers/cpufreq/amd-pstate.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index fa16d72d6058..117ad5988e8e 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -529,8 +529,20 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
>>     static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
>>   {
>> +    struct cpufreq_policy *policy = cpufreq_cpu_get(policy_data->cpu);
>> +    struct amd_cpudata *cpudata = policy->driver_data;
> 
> This /could/ be a NULL pointer de-reference.  It should have been initialized after the "if (!policy)" check.
> 
> It's a one line change though to initialize at the right place so I'll do some testing on the series though with that manually fixed up and if there are no other problems I'll take it.

Thanks for catching it!, last minute changes led to this oversight. As we 
discussed will put out a v3 with this fixed and a comment to explain the rationale.

Thanks,
Dhananjay

> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>> +
>> +    if (!policy)
>> +        return -EINVAL;
>> +
>> +    if (policy_data->min == FREQ_QOS_MIN_DEFAULT_VALUE)
>> +        policy_data->min = cpudata->lowest_nonlinear_freq;
>> +
>>       cpufreq_verify_within_cpu_limits(policy_data);
>>       pr_debug("policy_max =%d, policy_min=%d\n", policy_data->max, policy_data->min);
>> +
>> +    cpufreq_cpu_put(policy);
>> +
>>       return 0;
>>   }
>>   @@ -996,7 +1008,7 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>>           policy->fast_switch_possible = true;
>>         ret = freq_qos_add_request(&policy->constraints, &cpudata->req[0],
>> -                   FREQ_QOS_MIN, policy->cpuinfo.min_freq);
>> +                   FREQ_QOS_MIN, FREQ_QOS_MIN_DEFAULT_VALUE);
>>       if (ret < 0) {
>>           dev_err(dev, "Failed to add min-freq constraint (%d)\n", ret);
>>           goto free_cpudata1;
>