[PATCH v2] cpufreq/cppc: Take policy->cur into judge when set target

[PATCH v2] cpufreq/cppc: Take policy->cur into judge when set target

[Riwen Lu]

From: Riwen Lu <luriwen@kylinos.cn>

There is a case that desired_perf is exactly the same with the old perf,
but the actual current freq is not. Add a judgment condition to return
only when the three values are exactly the same.

This happened in S3 while the cpufreq governor is set to powersave.
During resume process, the CPU frequency is adjusted to the highest
perf. For the booting CPU, there's a warning that "CPU frequency out of
sync:", because the policy->cur is the lowest_perf while the actual
current frequency is the highest_perf that obtained via
cppc_cpufreq_get_rate(), then set policy->cur to highest_perf. The
governor->limits() intent to configure the CPU frequency to lowest_perf
and the governor->target() returned because the desired_perf is equal to
cpu->perf_ctrls.desired_perf leaving the actual current frequency and
policy->cur are remain the highest_perf. Add a judgement that if
policy->cur is the same with desired_perf to decide whther to return.

Signed-off-by: Riwen Lu <luriwen@kylinos.cn>

---
v1 -> v2:
 - Update commit message and email.
---
 drivers/cpufreq/cppc_cpufreq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 15f1d41920a3..802f7c7c0ad8 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -296,7 +296,8 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
 
 	desired_perf = cppc_khz_to_perf(&cpu_data->perf_caps, target_freq);
 	/* Return if it is exactly the same perf */
-	if (desired_perf == cpu_data->perf_ctrls.desired_perf)
+	if (desired_perf == cpu_data->perf_ctrls.desired_perf &&
+	    desired_perf == policy->cur)
 		return ret;
 
 	cpu_data->perf_ctrls.desired_perf = desired_perf;
-- 
2.25.1

Re: [PATCH v2] cpufreq/cppc: Take policy->cur into judge when set target

[Viresh Kumar]

On 29-05-24, 11:22, Riwen Lu wrote:
> From: Riwen Lu <luriwen@kylinos.cn>
> 
> There is a case that desired_perf is exactly the same with the old perf,
> but the actual current freq is not. Add a judgment condition to return
> only when the three values are exactly the same.
> 
> This happened in S3 while the cpufreq governor is set to powersave.
> During resume process, the CPU frequency is adjusted to the highest
> perf. For the booting CPU, there's a warning that "CPU frequency out of
> sync:", because the policy->cur is the lowest_perf while the actual
> current frequency is the highest_perf that obtained via
> cppc_cpufreq_get_rate(), then set policy->cur to highest_perf. The
> governor->limits() intent to configure the CPU frequency to lowest_perf
> and the governor->target() returned because the desired_perf is equal to
> cpu->perf_ctrls.desired_perf leaving the actual current frequency and
> policy->cur are remain the highest_perf. Add a judgement that if
> policy->cur is the same with desired_perf to decide whther to return.
> 
> Signed-off-by: Riwen Lu <luriwen@kylinos.cn>
> 
> ---
> v1 -> v2:
>  - Update commit message and email.
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 15f1d41920a3..802f7c7c0ad8 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -296,7 +296,8 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
>  
>  	desired_perf = cppc_khz_to_perf(&cpu_data->perf_caps, target_freq);
>  	/* Return if it is exactly the same perf */
> -	if (desired_perf == cpu_data->perf_ctrls.desired_perf)
> +	if (desired_perf == cpu_data->perf_ctrls.desired_perf &&
> +	    desired_perf == policy->cur)

From my earlier understanding, desired_perf is a derived interpretation of the
frequency and isn't an actual frequency value which can be compared with
policy->cur.

Shouldn't we compare policy->cur with target_freq instead ? If yes, than the
core must already be doing that somewhere I guess.

-- 
viresh

Re: [PATCH v2] cpufreq/cppc: Take policy->cur into judge when set target

[Riwen Lu]

在 2024/5/29 13:36, Viresh Kumar 写道:
> On 29-05-24, 11:22, Riwen Lu wrote:
>> From: Riwen Lu <luriwen@kylinos.cn>
>>
>> There is a case that desired_perf is exactly the same with the old perf,
>> but the actual current freq is not. Add a judgment condition to return
>> only when the three values are exactly the same.
>>
>> This happened in S3 while the cpufreq governor is set to powersave.
>> During resume process, the CPU frequency is adjusted to the highest
>> perf. For the booting CPU, there's a warning that "CPU frequency out of
>> sync:", because the policy->cur is the lowest_perf while the actual
>> current frequency is the highest_perf that obtained via
>> cppc_cpufreq_get_rate(), then set policy->cur to highest_perf. The
>> governor->limits() intent to configure the CPU frequency to lowest_perf
>> and the governor->target() returned because the desired_perf is equal to
>> cpu->perf_ctrls.desired_perf leaving the actual current frequency and
>> policy->cur are remain the highest_perf. Add a judgement that if
>> policy->cur is the same with desired_perf to decide whther to return.
>>
>> Signed-off-by: Riwen Lu <luriwen@kylinos.cn>
>>
>> ---
>> v1 -> v2:
>>   - Update commit message and email.
>> ---
>>   drivers/cpufreq/cppc_cpufreq.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index 15f1d41920a3..802f7c7c0ad8 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -296,7 +296,8 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
>>   
>>   	desired_perf = cppc_khz_to_perf(&cpu_data->perf_caps, target_freq);
>>   	/* Return if it is exactly the same perf */
>> -	if (desired_perf == cpu_data->perf_ctrls.desired_perf)
>> +	if (desired_perf == cpu_data->perf_ctrls.desired_perf &&
>> +	    desired_perf == policy->cur)
> 
>  From my earlier understanding, desired_perf is a derived interpretation of the
> frequency and isn't an actual frequency value which can be compared with
> policy->cur.
> 
> Shouldn't we compare policy->cur with target_freq instead ? If yes, than the
> core must already be doing that somewhere I guess.
> 
Yes, you are right， I didn't think it through. In this circumstance, the 
policy->cur is the highest frequency, desired_perf converted from 
target_freq is the same with cpu_data->perf_ctrls.desired_perf which 
shouldn't.

Re: [PATCH v2] cpufreq/cppc: Take policy->cur into judge when set target

[Viresh Kumar]

On 29-05-24, 14:53, Riwen Lu wrote:
> Yes, you are right， I didn't think it through. In this circumstance, the
> policy->cur is the highest frequency, desired_perf converted from
> target_freq is the same with cpu_data->perf_ctrls.desired_perf which
> shouldn't.

Please investigate more and see where the real problem is.

-- 
viresh

Re: [PATCH v2] cpufreq/cppc: Take policy->cur into judge when set target

[Riwen Lu]

在 2024/5/29 15:12, Viresh Kumar 写道:
> On 29-05-24, 14:53, Riwen Lu wrote:
>> Yes, you are right， I didn't think it through. In this circumstance, the
>> policy->cur is the highest frequency, desired_perf converted from
>> target_freq is the same with cpu_data->perf_ctrls.desired_perf which
>> shouldn't.
> 
> Please investigate more and see where the real problem is.
> 
The boot CPU's frequency would be configured to the highest perf when 
powered on from S3 even though the policy governor is powersave.

In cpufreq resume process, the booting CPU's new_freq obtained via 
.get() is the highest frequency, while the policy->cur and 
cpu->perf_ctrls.desired_perf are in the lowest level(powersave 
governor). Causing the warning: "CPU frequency out of sync:", and set 
policy->cur to new_freq. Then the governor->limits() calls 
cppc_cpufreq_set_target() to configures the CPU frequency and returns 
directly because the desired_perf converted from target_freq and 
cpu->perf_ctrls.desired_perf are the same and both are the lowest_perf.

The problem is that the cpu->perf_ctrls.desired_perf is the lowest_perf 
but it should be the highest_perf.

In my opinion, desired_perf and cpu->perf_ctrls.desired_perf represent 
the target_freq and policy->cur respectively. Since target_freq and 
policy->cur have been compared in __cpufreq_driver_target(), there's no 
need to compare desired_perf and cpu->perf_ctrls.desired_perf again in 
cppc_cpufreq_set_target().
So, maybe we can remove the following logic in cppc_cpufreq_set_target().
/* Return if it is exactly the same perf */
if (desired_perf == cpu_data->perf_ctrls.desired_perf)
	return ret;

Re: [PATCH v2] cpufreq/cppc: Take policy->cur into judge when set target

[Viresh Kumar]

Cc'ing few more people.

On 30-05-24, 09:06, Riwen Lu wrote:
> 在 2024/5/29 15:12, Viresh Kumar 写道:
> > On 29-05-24, 14:53, Riwen Lu wrote:
> > > Yes, you are right， I didn't think it through. In this circumstance, the
> > > policy->cur is the highest frequency, desired_perf converted from
> > > target_freq is the same with cpu_data->perf_ctrls.desired_perf which
> > > shouldn't.
> > 
> > Please investigate more and see where the real problem is.
> > 
> The boot CPU's frequency would be configured to the highest perf when
> powered on from S3 even though the policy governor is powersave.
> 
> In cpufreq resume process, the booting CPU's new_freq obtained via .get() is
> the highest frequency, while the policy->cur and
> cpu->perf_ctrls.desired_perf are in the lowest level(powersave governor).
> Causing the warning: "CPU frequency out of sync:", and set policy->cur to
> new_freq. Then the governor->limits() calls cppc_cpufreq_set_target() to
> configures the CPU frequency and returns directly because the desired_perf
> converted from target_freq and cpu->perf_ctrls.desired_perf are the same and
> both are the lowest_perf.
> 
> The problem is that the cpu->perf_ctrls.desired_perf is the lowest_perf but
> it should be the highest_perf.
> 
> In my opinion, desired_perf and cpu->perf_ctrls.desired_perf represent the
> target_freq and policy->cur respectively. Since target_freq and policy->cur
> have been compared in __cpufreq_driver_target(), there's no need to compare
> desired_perf and cpu->perf_ctrls.desired_perf again in
> cppc_cpufreq_set_target().
> So, maybe we can remove the following logic in cppc_cpufreq_set_target().
> /* Return if it is exactly the same perf */
> if (desired_perf == cpu_data->perf_ctrls.desired_perf)
> 	return ret;

This is what I was thinking as well yesterday.

-- 
viresh

Re: [PATCH v2] cpufreq/cppc: Take policy->cur into judge when set target

[Riwen Lu]

在 2024/5/30 13:56, Viresh Kumar 写道:
> Cc'ing few more people.
> 
> On 30-05-24, 09:06, Riwen Lu wrote:
>> 在 2024/5/29 15:12, Viresh Kumar 写道:
>>> On 29-05-24, 14:53, Riwen Lu wrote:
>>>> Yes, you are right， I didn't think it through. In this circumstance, the
>>>> policy->cur is the highest frequency, desired_perf converted from
>>>> target_freq is the same with cpu_data->perf_ctrls.desired_perf which
>>>> shouldn't.
>>>
>>> Please investigate more and see where the real problem is.
>>>
>> The boot CPU's frequency would be configured to the highest perf when
>> powered on from S3 even though the policy governor is powersave.
>>
>> In cpufreq resume process, the booting CPU's new_freq obtained via .get() is
>> the highest frequency, while the policy->cur and
>> cpu->perf_ctrls.desired_perf are in the lowest level(powersave governor).
>> Causing the warning: "CPU frequency out of sync:", and set policy->cur to
>> new_freq. Then the governor->limits() calls cppc_cpufreq_set_target() to
>> configures the CPU frequency and returns directly because the desired_perf
>> converted from target_freq and cpu->perf_ctrls.desired_perf are the same and
>> both are the lowest_perf.
>>
>> The problem is that the cpu->perf_ctrls.desired_perf is the lowest_perf but
>> it should be the highest_perf.
>>
>> In my opinion, desired_perf and cpu->perf_ctrls.desired_perf represent the
>> target_freq and policy->cur respectively. Since target_freq and policy->cur
>> have been compared in __cpufreq_driver_target(), there's no need to compare
>> desired_perf and cpu->perf_ctrls.desired_perf again in
>> cppc_cpufreq_set_target().
>> So, maybe we can remove the following logic in cppc_cpufreq_set_target().
>> /* Return if it is exactly the same perf */
>> if (desired_perf == cpu_data->perf_ctrls.desired_perf)
>> 	return ret;
> 
> This is what I was thinking as well yesterday.
> 
OK, I'll push a V3 patch.

Re: [PATCH v2] cpufreq/cppc: Take policy->cur into judge when set target

[Viresh Kumar]

On 30-05-24, 14:02, Riwen Lu wrote:
> 在 2024/5/30 13:56, Viresh Kumar 写道:
> > Cc'ing few more people.
> > 
> > On 30-05-24, 09:06, Riwen Lu wrote:
> > > 在 2024/5/29 15:12, Viresh Kumar 写道:
> > > > On 29-05-24, 14:53, Riwen Lu wrote:
> > > > > Yes, you are right， I didn't think it through. In this circumstance, the
> > > > > policy->cur is the highest frequency, desired_perf converted from
> > > > > target_freq is the same with cpu_data->perf_ctrls.desired_perf which
> > > > > shouldn't.
> > > > 
> > > > Please investigate more and see where the real problem is.
> > > > 
> > > The boot CPU's frequency would be configured to the highest perf when
> > > powered on from S3 even though the policy governor is powersave.
> > > 
> > > In cpufreq resume process, the booting CPU's new_freq obtained via .get() is
> > > the highest frequency, while the policy->cur and
> > > cpu->perf_ctrls.desired_perf are in the lowest level(powersave governor).
> > > Causing the warning: "CPU frequency out of sync:", and set policy->cur to
> > > new_freq. Then the governor->limits() calls cppc_cpufreq_set_target() to
> > > configures the CPU frequency and returns directly because the desired_perf
> > > converted from target_freq and cpu->perf_ctrls.desired_perf are the same and
> > > both are the lowest_perf.
> > > 
> > > The problem is that the cpu->perf_ctrls.desired_perf is the lowest_perf but
> > > it should be the highest_perf.
> > > 
> > > In my opinion, desired_perf and cpu->perf_ctrls.desired_perf represent the
> > > target_freq and policy->cur respectively. Since target_freq and policy->cur
> > > have been compared in __cpufreq_driver_target(), there's no need to compare
> > > desired_perf and cpu->perf_ctrls.desired_perf again in
> > > cppc_cpufreq_set_target().
> > > So, maybe we can remove the following logic in cppc_cpufreq_set_target().
> > > /* Return if it is exactly the same perf */
> > > if (desired_perf == cpu_data->perf_ctrls.desired_perf)
> > > 	return ret;
> > 
> > This is what I was thinking as well yesterday.
> > 
> OK, I'll push a V3 patch.

Please CC everyone from this email.

-- 
viresh

[PATCH v3] cpufreq/cppc: Remove the desired_perf compare when set target

[Riwen Lu]

From: Riwen Lu <luriwen@kylinos.cn>

There is a case that desired_perf is exactly the same with the old perf,
but the actual current freq is not.

This happened in S3 while the cpufreq governor is set to powersave.
During cpufreq resume process, the booting CPU's new_freq obtained via
.get() is the highest frequency, while the policy->cur and
cpu->perf_ctrls.desired_perf are in the lowest level(powersave
governor). Causing the warning: "CPU frequency out of sync:", and set
policy->cur to new_freq. Then the governor->limits() calls
cppc_cpufreq_set_target() to configures the CPU frequency and returns
directly because the desired_perf converted from target_freq is the
same with cpu->perf_ctrls.desired_perf and both are the lowest_perf.
Since target_freq and policy->cur have been compared in
__cpufreq_driver_target(), there's no need to compare desired_perf
and cpu->perf_ctrls.desired_perf again in cppc_cpufreq_set_target()
to ensure that the CPU frequency is properly configured.

Signed-off-by: Riwen Lu <luriwen@kylinos.cn>

---
v1 -> v2:
 - Update commit message and email.
v2 -> v3:
 - Update patch subject and commit message.
 - Remove the desired_perf compare logic.
---
 drivers/cpufreq/cppc_cpufreq.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 15f1d41920a3..337cece61ab5 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -295,9 +295,6 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
 	int ret = 0;
 
 	desired_perf = cppc_khz_to_perf(&cpu_data->perf_caps, target_freq);
-	/* Return if it is exactly the same perf */
-	if (desired_perf == cpu_data->perf_ctrls.desired_perf)
-		return ret;
 
 	cpu_data->perf_ctrls.desired_perf = desired_perf;
 	freqs.old = policy->cur;
-- 
2.25.1

Re: [PATCH v3] cpufreq/cppc: Remove the desired_perf compare when set target

[Viresh Kumar]

Ionela, Beata,

On 30-05-24, 19:08, Riwen Lu wrote:
> From: Riwen Lu <luriwen@kylinos.cn>
> 
> There is a case that desired_perf is exactly the same with the old perf,
> but the actual current freq is not.
> 
> This happened in S3 while the cpufreq governor is set to powersave.
> During cpufreq resume process, the booting CPU's new_freq obtained via
> .get() is the highest frequency, while the policy->cur and
> cpu->perf_ctrls.desired_perf are in the lowest level(powersave
> governor). Causing the warning: "CPU frequency out of sync:", and set
> policy->cur to new_freq. Then the governor->limits() calls
> cppc_cpufreq_set_target() to configures the CPU frequency and returns
> directly because the desired_perf converted from target_freq is the
> same with cpu->perf_ctrls.desired_perf and both are the lowest_perf.
> Since target_freq and policy->cur have been compared in
> __cpufreq_driver_target(), there's no need to compare desired_perf
> and cpu->perf_ctrls.desired_perf again in cppc_cpufreq_set_target()
> to ensure that the CPU frequency is properly configured.
> 
> Signed-off-by: Riwen Lu <luriwen@kylinos.cn>
> 
> ---
> v1 -> v2:
>  - Update commit message and email.
> v2 -> v3:
>  - Update patch subject and commit message.
>  - Remove the desired_perf compare logic.
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 15f1d41920a3..337cece61ab5 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -295,9 +295,6 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
>  	int ret = 0;
>  
>  	desired_perf = cppc_khz_to_perf(&cpu_data->perf_caps, target_freq);
> -	/* Return if it is exactly the same perf */
> -	if (desired_perf == cpu_data->perf_ctrls.desired_perf)
> -		return ret;
>  
>  	cpu_data->perf_ctrls.desired_perf = desired_perf;
>  	freqs.old = policy->cur;

Any objections to this change ?

-- 
viresh

Re: [PATCH v3] cpufreq/cppc: Remove the desired_perf compare when set target

[Ionela Voinescu]

Hey,

On Thursday 06 Jun 2024 at 14:37:37 (+0530), Viresh Kumar wrote:
> Ionela, Beata,
> 
> On 30-05-24, 19:08, Riwen Lu wrote:
> > From: Riwen Lu <luriwen@kylinos.cn>
> > 
> > There is a case that desired_perf is exactly the same with the old perf,
> > but the actual current freq is not.
> > 
> > This happened in S3 while the cpufreq governor is set to powersave.
> > During cpufreq resume process, the booting CPU's new_freq obtained via
> > .get() is the highest frequency, while the policy->cur and
> > cpu->perf_ctrls.desired_perf are in the lowest level(powersave
> > governor). Causing the warning: "CPU frequency out of sync:", and set
> > policy->cur to new_freq. Then the governor->limits() calls
> > cppc_cpufreq_set_target() to configures the CPU frequency and returns
> > directly because the desired_perf converted from target_freq is the
> > same with cpu->perf_ctrls.desired_perf and both are the lowest_perf.
> > Since target_freq and policy->cur have been compared in
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	  [note] below

> > __cpufreq_driver_target(), there's no need to compare desired_perf
> > and cpu->perf_ctrls.desired_perf again in cppc_cpufreq_set_target()
> > to ensure that the CPU frequency is properly configured.
> > 
> > Signed-off-by: Riwen Lu <luriwen@kylinos.cn>
> > 
> > ---
> > v1 -> v2:
> >  - Update commit message and email.
> > v2 -> v3:
> >  - Update patch subject and commit message.
> >  - Remove the desired_perf compare logic.
> > ---
> >  drivers/cpufreq/cppc_cpufreq.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> > index 15f1d41920a3..337cece61ab5 100644
> > --- a/drivers/cpufreq/cppc_cpufreq.c
> > +++ b/drivers/cpufreq/cppc_cpufreq.c
> > @@ -295,9 +295,6 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
> >  	int ret = 0;
> >  
> >  	desired_perf = cppc_khz_to_perf(&cpu_data->perf_caps, target_freq);
> > -	/* Return if it is exactly the same perf */
> > -	if (desired_perf == cpu_data->perf_ctrls.desired_perf)
> > -		return ret;
> >  
> >  	cpu_data->perf_ctrls.desired_perf = desired_perf;
> >  	freqs.old = policy->cur;
> 
> Any objections to this change ?

It's alright with me.

Some "nits":
 - the "desired_perf" local variable could be removed in this case.

 - [note] while this change helps, we'd still need policy->cur to always
   have the latest request value (see details at [1]) for this check to
   be made obsolete by the comparison between target_freq and policy->cur,
   as mentioned in the commit message. But this is/can be a separate
   matter.

   [1] https://lore.kernel.org/lkml/ZmB1qKucR5fXk100@arm.com/

Thanks,
Ionela.

> 
> -- 
> viresh

Re: [PATCH v3] cpufreq/cppc: Remove the desired_perf compare when set target

[Viresh Kumar]

On 11-06-24, 09:54, Ionela Voinescu wrote:
> It's alright with me.

Great.

> Some "nits":
>  - the "desired_perf" local variable could be removed in this case.

Riwen, please fix this and resend.

>  - [note] while this change helps, we'd still need policy->cur to always
>    have the latest request value (see details at [1]) for this check to
>    be made obsolete by the comparison between target_freq and policy->cur,
>    as mentioned in the commit message. But this is/can be a separate
>    matter.
> 
>    [1] https://lore.kernel.org/lkml/ZmB1qKucR5fXk100@arm.com/

Yeah, lets discuss that in the other thread only.

-- 
viresh

Re: [PATCH v3] cpufreq/cppc: Remove the desired_perf compare when set target

[Riwen Lu]

在 2024/6/11 17:10, Viresh Kumar 写道:
> On 11-06-24, 09:54, Ionela Voinescu wrote:
>> It's alright with me.
> 
> Great.
> 
>> Some "nits":
>>   - the "desired_perf" local variable could be removed in this case.
> 
> Riwen, please fix this and resend.

I think it's no need to remove the "desired_perf" local variable here, 
the code is seems more readable.
> 
>>   - [note] while this change helps, we'd still need policy->cur to always
>>     have the latest request value (see details at [1]) for this check to
>>     be made obsolete by the comparison between target_freq and policy->cur,
>>     as mentioned in the commit message. But this is/can be a separate
>>     matter.
>>
>>     [1] https://lore.kernel.org/lkml/ZmB1qKucR5fXk100@arm.com/
> 
> Yeah, lets discuss that in the other thread only.
> 

Re: [PATCH v3] cpufreq/cppc: Remove the desired_perf compare when set target

[Viresh Kumar]

On 12-06-24, 10:52, Riwen Lu wrote:
> I think it's no need to remove the "desired_perf" local variable here, the
> code is seems more readable.

There is no need of a temporary variable and I don't think it increases
readability at all. Moreover, same needs to be done in
cppc_cpufreq_fast_switch() too.

-- 
viresh

Re: [PATCH v3] cpufreq/cppc: Remove the desired_perf compare when set target

[Viresh Kumar]

On 12-06-24, 11:54, Viresh Kumar wrote:
> On 12-06-24, 10:52, Riwen Lu wrote:
> > I think it's no need to remove the "desired_perf" local variable here, the
> > code is seems more readable.
> 
> There is no need of a temporary variable and I don't think it increases
> readability at all. Moreover, same needs to be done in
> cppc_cpufreq_fast_switch() too.

Also please don't send the new patch in-reply-to the earlier one. Send
a fresh thread.

Thanks.

-- 
viresh

Re: [PATCH v3] cpufreq/cppc: Remove the desired_perf compare when set target

[Pierre Gondois]

Hello Riwen,

This function seems to be the only cpufreq function saving and comparing the
requested frequency with the last requested frequency. This seems to be more the
task of the cpufreq framework than the cpufreq driver.

So FYIW, the patch looks good to me.

On 5/30/24 13:08, Riwen Lu wrote:
> From: Riwen Lu <luriwen@kylinos.cn>
> 
> There is a case that desired_perf is exactly the same with the old perf,
> but the actual current freq is not.
> 
> This happened in S3 while the cpufreq governor is set to powersave.
> During cpufreq resume process, the booting CPU's new_freq obtained via
> .get() is the highest frequency, while the policy->cur and
> cpu->perf_ctrls.desired_perf are in the lowest level(powersave
> governor). Causing the warning: "CPU frequency out of sync:", and set
> policy->cur to new_freq.

(new paragraph)

Then the governor->limits() calls
> cppc_cpufreq_set_target() to configures the CPU frequency and returns
> directly because the desired_perf converted from target_freq is the
> same with cpu->perf_ctrls.desired_perf and both are the lowest_perf.

(new paragraph)

> Since target_freq and policy->cur have been compared in
> __cpufreq_driver_target(), there's no need to compare desired_perf
> and cpu->perf_ctrls.desired_perf again in cppc_cpufreq_set_target()
> to ensure that the CPU frequency is properly configured.

NIT:
Would it be possible to make distinct paragraphs ?

> 
> Signed-off-by: Riwen Lu <luriwen@kylinos.cn>
> 
> ---
> v1 -> v2:
>   - Update commit message and email.
> v2 -> v3:
>   - Update patch subject and commit message.
>   - Remove the desired_perf compare logic.
> ---
>   drivers/cpufreq/cppc_cpufreq.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 15f1d41920a3..337cece61ab5 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -295,9 +295,6 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
>   	int ret = 0;
>   
>   	desired_perf = cppc_khz_to_perf(&cpu_data->perf_caps, target_freq);
> -	/* Return if it is exactly the same perf */
> -	if (desired_perf == cpu_data->perf_ctrls.desired_perf)
> -		return ret;
>   
>   	cpu_data->perf_ctrls.desired_perf = desired_perf;
>   	freqs.old = policy->cur;

Re: [PATCH v3] cpufreq/cppc: Remove the desired_perf compare when set target

[Riwen Lu]

在 2024/6/12 14:24, Viresh Kumar 写道:
> On 12-06-24, 10:52, Riwen Lu wrote:
>> I think it's no need to remove the "desired_perf" local variable here, the
>> code is seems more readable.
> 
> There is no need of a temporary variable and I don't think it increases
> readability at all. Moreover, same needs to be done in
> cppc_cpufreq_fast_switch() too.
> 
OK. I'll push a patch that remove the desired_perf temporary in 
cppc_cpufreq_fast_switch().