| Message ID | TYCP286MB24861BA890594C119892FB3DB1F22@TYCP286MB2486.JPNP286.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] cpufreq/cppc: Take policy->cur into judge when set target | expand |
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.
在 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.
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.
在 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;
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.
在 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.
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.
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;