cpufreq: transition-latency cleanups

There is no limitation in the ondemand or conservative governors which disallow the transition_latency to be greater than 10 ms.

The max_transition_latency field is rather used to disallow automatic dynamic frequency switching for platforms which didn't wanted these governors to run.

Replace max_transition_latency with a boolean (dynamic_switching) and check for transition_latency == CPUFREQ_ETERNAL along with that. This makes it pretty straight forward to read/understand now.

Quoted 7 lines · Viresh Kumar

There is no limitation in the ondemand or conservative governors which disallow the transition_latency to be greater than 10 ms.

The max_transition_latency field is rather used to disallow automatic dynamic frequency switching for platforms which didn't wanted these governors to run.

Quoted 3 lines

Replace max_transition_latency with a boolean (dynamic_switching) and check for transition_latency == CPUFREQ_ETERNAL along with that. This makes it pretty straight forward to read/understand now.

Well, using CPUFREQ_ETERNAL for that on the driver side is still not particularly straightforward IMO, so maybe add a "no_dynamic_switching" to the driver structure and set it to "true" for the one driver in question?

Quoted 12 lines · Rafael J. Wysocki

Quoted 10 lines · Viresh Kumar

There is no limitation in the ondemand or conservative governors which disallow the transition_latency to be greater than 10 ms.

The max_transition_latency field is rather used to disallow automatic dynamic frequency switching for platforms which didn't wanted these governors to run.

Replace max_transition_latency with a boolean (dynamic_switching) and check for transition_latency == CPUFREQ_ETERNAL along with that. This makes it pretty straight forward to read/understand now.

Quoted 4 lines

Well, using CPUFREQ_ETERNAL for that on the driver side is still not particularly straightforward IMO, so maybe add a "no_dynamic_switching" to the driver structure and set it to "true" for the one driver in question?

IIRC it's not just one driver which sets the latency to CPUFREQ_ETERNAL, and where dynamic switching might be harmful or at least lead to undefined behavior.

Quoted 13 lines · Dominik Brodowski

Quoted 12 lines · Rafael J. Wysocki

Quoted 10 lines · Viresh Kumar

There is no limitation in the ondemand or conservative governors which disallow the transition_latency to be greater than 10 ms.

The max_transition_latency field is rather used to disallow automatic dynamic frequency switching for platforms which didn't wanted these governors to run.

Replace max_transition_latency with a boolean (dynamic_switching) and check for transition_latency == CPUFREQ_ETERNAL along with that. This makes it pretty straight forward to read/understand now.

Quoted 8 lines

Quoted 4 lines

Well, using CPUFREQ_ETERNAL for that on the driver side is still not particularly straightforward IMO, so maybe add a "no_dynamic_switching" to the driver structure and set it to "true" for the one driver in question?

IIRC it's not just one driver which sets the latency to CPUFREQ_ETERNAL, and where dynamic switching might be harmful or at least lead to undefined behavior.

OK

Still, though, using CPUFREQ_ETERNAL to indicate the "no dynamic switching" condition is somewhat convoluted, so why don't we have a flag to *explicitly* say that instead?

Do you know which drivers they are or is it just all drivers that use CPUFREQ_ETERNAL?

Quoted 30 lines · Rafael J. Wysocki

Quoted 13 lines · Dominik Brodowski

Quoted 12 lines · Rafael J. Wysocki

Quoted 10 lines · Viresh Kumar

There is no limitation in the ondemand or conservative governors which disallow the transition_latency to be greater than 10 ms.

The max_transition_latency field is rather used to disallow automatic dynamic frequency switching for platforms which didn't wanted these governors to run.

Replace max_transition_latency with a boolean (dynamic_switching) and check for transition_latency == CPUFREQ_ETERNAL along with that. This makes it pretty straight forward to read/understand now.

Quoted 8 lines

Quoted 4 lines

Well, using CPUFREQ_ETERNAL for that on the driver side is still not particularly straightforward IMO, so maybe add a "no_dynamic_switching" to the driver structure and set it to "true" for the one driver in question?

IIRC it's not just one driver which sets the latency to CPUFREQ_ETERNAL, and where dynamic switching might be harmful or at least lead to undefined behavior.

OK

Still, though, using CPUFREQ_ETERNAL to indicate the "no dynamic switching" condition is somewhat convoluted, so why don't we have a flag to *explicitly* say that instead?

Quoted 2 lines

Do you know which drivers they are or is it just all drivers that use CPUFREQ_ETERNAL?

Well, after the $subject patch it effectively is all drivers that use CPUFREQ_ETERNAL anyway, so it looks like we actually can do a complete switch-over.

Quoted 31 lines · Rafael J. Wysocki

Quoted 30 lines · Rafael J. Wysocki

Quoted 13 lines · Dominik Brodowski

Quoted 12 lines · Rafael J. Wysocki

Quoted 10 lines · Viresh Kumar

There is no limitation in the ondemand or conservative governors which disallow the transition_latency to be greater than 10 ms.

The max_transition_latency field is rather used to disallow automatic dynamic frequency switching for platforms which didn't wanted these governors to run.

Replace max_transition_latency with a boolean (dynamic_switching) and check for transition_latency == CPUFREQ_ETERNAL along with that. This makes it pretty straight forward to read/understand now.

Quoted 8 lines

Quoted 4 lines

Well, using CPUFREQ_ETERNAL for that on the driver side is still not particularly straightforward IMO, so maybe add a "no_dynamic_switching" to the driver structure and set it to "true" for the one driver in question?

IIRC it's not just one driver which sets the latency to CPUFREQ_ETERNAL, and where dynamic switching might be harmful or at least lead to undefined behavior.

OK

Still, though, using CPUFREQ_ETERNAL to indicate the "no dynamic switching" condition is somewhat convoluted, so why don't we have a flag to *explicitly* say that instead?

Quoted 6 lines

Quoted 2 lines

Do you know which drivers they are or is it just all drivers that use CPUFREQ_ETERNAL?

Well, after the $subject patch it effectively is all drivers that use CPUFREQ_ETERNAL anyway, so it looks like we actually can do a complete switch-over.

Exactly. But lets take a quick look at the drivers ussing CPUFREQ_ETERNAL:

Using CPUFREQ_ETERNAL, as policy-setting drivers:

- intel_pstate.c - for the intel_pstate driver, which defers to the hardware to do frequency selection.

- longrun.c - hardware-based frequency selection.

=> Those drivers are not interested in kernel-based dynamic frequency selection anyway.

Using CPUFREQ_ETERNAL as a fallback if transition_latency is unknown:
- arm_big_little.c
- arm_big_little_dt.c
- cpufreq-dt.c
- imx6q-cpufreq.c
- spear_cpufreq.c

=> As it seems to be an error case, it seems best to bail out on the safe side and disallow dynamic frequency scaling. Platform experts might know better, though.

Using CPUFREQ_ETERNAL unconditionally:
- cpufreq-nforce2.c - over a decade old driver; has a commented-out hack to mdelay(10ms) after each frequency transition. This smells like it might be unsafe to do dynamic switching more often than that.

- elanfreq.c - Has udelay(1ms+10ms) in transition path, so the same terms and conditions seem to apply.

- gx-suspmod.c - works by a mechanism which reminds me of CPU frequency throttling, but chipset- and not CPU-based.

- pmac32-cpufreq.c - for some models, it sets latency to ETERNAL. In some frequency switchign code, it has mdelay(10ms) calls.

- speedsstep-smi.c - this case was discussed previously.

=> For those drivers, dynamic frequency scaling should not be enabled IMO.

- sa1100-cpufreq.c and
- sa1110-cpufreq.c - If I remember correctly, those drivers were used for fast user-space based frequency scaling in the past.

=> For these two drivers, enabling DFVS might be an option.

- sh-cpufreq.c - looks fast, but I have no clue.

- unicore2-cpufreq.c - same.

=> For those drivers, I have no clue. So to be on the safe side, I'd opt for dynamic frequency scaling to be set to off.

To summarize: At first, I'd propose a *complete* switch-over from CPUFREQ_ETERNAL to setting the flag "no DVFS" you have proposed.

Then, one might discuss with the maintainers of individual drivers/platforms on whether to relax this rule for a few of those drivers (sa11x0, sh-cpufreq, unicore2-cpufreq and the drivers using CPUFREQ_ETERNAL as a fallback).

Quoted 4 lines · Dominik Brodowski

Exactly. But lets take a quick look at the drivers ussing CPUFREQ_ETERNAL:

Using CPUFREQ_ETERNAL, as policy-setting drivers:

Quoted 4 lines

- intel_pstate.c - for the intel_pstate driver, which defers to the hardware to do frequency selection.

- longrun.c - hardware-based frequency selection.

That may or may not be hardware-based, but if the ->setpolicy callback is present, transition_latency doesn't matter anyway.

Quoted 2 lines

=> Those drivers are not interested in kernel-based dynamic frequency selection anyway.

Right.

Quoted 8 lines

Using CPUFREQ_ETERNAL as a fallback if transition_latency is unknown:
- arm_big_little.c
- arm_big_little_dt.c
- cpufreq-dt.c
- imx6q-cpufreq.c
- spear_cpufreq.c

Quoted 4 lines

=> As it seems to be an error case, it seems best to bail out on the safe side and disallow dynamic frequency scaling. Platform experts might know better, though.

Well, Viresh should know what to do for some of them at least. :-)

Quoted 11 lines

Using CPUFREQ_ETERNAL unconditionally:
- cpufreq-nforce2.c - over a decade old driver; has a commented-out hack to mdelay(10ms) after each frequency transition. This smells like it might be unsafe to do dynamic switching more often than that.

- elanfreq.c - Has udelay(1ms+10ms) in transition path, so the same terms and conditions seem to apply.

- gx-suspmod.c - works by a mechanism which reminds me of CPU frequency throttling, but chipset- and not CPU-based.

Quoted 6 lines

- pmac32-cpufreq.c - for some models, it sets latency to ETERNAL. In some frequency switchign code, it has mdelay(10ms) calls.

- speedsstep-smi.c - this case was discussed previously.

=> For those drivers, dynamic frequency scaling should not be enabled IMO.

Agreed.

Quoted 2 lines

To summarize: At first, I'd propose a *complete* switch-over from CPUFREQ_ETERNAL to setting the flag "no DVFS" you have proposed.

So we seem to be in agreement over this.

Quoted 6 lines · Rafael J. Wysocki

Quoted 4 lines · Dominik Brodowski

Exactly. But lets take a quick look at the drivers ussing CPUFREQ_ETERNAL:

Using CPUFREQ_ETERNAL, as policy-setting drivers:

Quoted 7 lines

Quoted 4 lines

- intel_pstate.c - for the intel_pstate driver, which defers to the hardware to do frequency selection.

- longrun.c - hardware-based frequency selection.

That may or may not be hardware-based, but if the ->setpolicy callback is present, transition_latency doesn't matter anyway.

Right and to avoid confusion its probably better to avoid setting transition_latency completely from them. I will try to include that in my series.

Quoted 13 lines

Quoted 2 lines

=> Those drivers are not interested in kernel-based dynamic frequency selection anyway.

Right.

Quoted 8 lines

Using CPUFREQ_ETERNAL as a fallback if transition_latency is unknown:
- arm_big_little.c
- arm_big_little_dt.c
- cpufreq-dt.c
- imx6q-cpufreq.c
- spear_cpufreq.c

Quoted 6 lines

Quoted 4 lines

=> As it seems to be an error case, it seems best to bail out on the safe side and disallow dynamic frequency scaling. Platform experts might know better, though.

Well, Viresh should know what to do for some of them at least. :-)

Yeah, they just don't know how much time it takes to change the frequency. We shouldn't disallow DVFS for them.

Quoted 11 lines

Quoted 11 lines

Using CPUFREQ_ETERNAL unconditionally:
- cpufreq-nforce2.c - over a decade old driver; has a commented-out hack to mdelay(10ms) after each frequency transition. This smells like it might be unsafe to do dynamic switching more often than that.

- elanfreq.c - Has udelay(1ms+10ms) in transition path, so the same terms and conditions seem to apply.

- gx-suspmod.c - works by a mechanism which reminds me of CPU frequency throttling, but chipset- and not CPU-based.

Quoted 8 lines

Quoted 6 lines

- pmac32-cpufreq.c - for some models, it sets latency to ETERNAL. In some frequency switchign code, it has mdelay(10ms) calls.

- speedsstep-smi.c - this case was discussed previously.

=> For those drivers, dynamic frequency scaling should not be enabled IMO.

Agreed.

+1 and these are the drivers which should have this new variable set to avoid DVFS.

Anyone who is setting CPUFREQ_ETERNAL unconditionally should be setting the new flag.

Quoted 4 lines

Quoted 2 lines

To summarize: At first, I'd propose a *complete* switch-over from CPUFREQ_ETERNAL to setting the flag "no DVFS" you have proposed.

So we seem to be in agreement over this.

The usage of CPUFREQ_ETERNAL had been confusing over the years, i.e. some use it to not allow ondemand/conservative, while others use it as they don't know their transition latencies.

A complete switch over may not be good for the later.

I would suggest we only move the platforms which set latency to CPUFREQ_ETERNAL unconditionally to the "no DVFS" list. And everyone else can still continue with CPUFREQ_ETERNAL. I have earlier proposed finding their latencies dynamically and will try to include that for them.

The policy->transition_delay_us field is used only by the schedutil governor currently, and this field describes how fast the driver wants the cpufreq governor to change CPUs frequency. It should rather be a common thing across all governors, as it doesn't have any schedutil dependency here.

Create a new helper cpufreq_policy_transition_delay_us() to get the transition delay across all governors.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/cpufreq/cpufreq_governor.c |  9 +--------
 include/linux/cpufreq.h            | 15 +++++++++++++++
 kernel/sched/cpufreq_schedutil.c   | 11 +----------
 3 files changed, 17 insertions(+), 18 deletions(-)

drivers/cpufreq/cpufreq_governor.c +1 -8

index 858081f9c3d7..eed069ecfd5e 100644
@@ -389,7 +389,6 @@ int cpufreq_dbs_governor_init(struct cpufreq_policy *policy)
        struct dbs_governor *gov = dbs_governor_of(policy);
        struct dbs_data *dbs_data;
        struct policy_dbs_info *policy_dbs;
-       unsigned int latency;
        int ret = 0;
 
        /* State should be equivalent to EXIT */

@@ -428,13 +427,7 @@ int cpufreq_dbs_governor_init(struct cpufreq_policy *policy)
        if (ret)
                goto free_policy_dbs_info;
 
-       /* policy latency is in ns. Convert it to us first */
-       latency = policy->cpuinfo.transition_latency / 1000;
-       if (latency == 0)
-               latency = 1;
-
-       /* Bring kernel and HW constraints together */
-       dbs_data->sampling_rate = LATENCY_MULTIPLIER * latency;
+       dbs_data->sampling_rate = cpufreq_policy_transition_delay_us(policy);
 
        if (!have_governor_per_policy())
                gov->gdbs_data = dbs_data;

include/linux/cpufreq.h +4 -0

index 00e4c40a3249..14f0ab61ed17 100644
@@ -532,6 +532,21 @@ static inline void cpufreq_policy_apply_limits(struct cpufreq_policy *policy)
                __cpufreq_driver_target(policy, policy->min, CPUFREQ_RELATION_L);
 }
 
+static inline unsigned int
+cpufreq_policy_transition_delay_us(struct cpufreq_policy *policy)
+{
+       unsigned int delay_us = LATENCY_MULTIPLIER, latency;