| Message ID | cover.1737707712.git.viresh.kumar@linaro.org |
|---|---|
| Headers | show |
| Series | cpufreq: simplify boost handling | expand |
On 24-01-25, 12:05, Rafael J. Wysocki wrote: > On Fri, Jan 24, 2025 at 9:58 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > Hello, > > > > The boost feature can be controlled at two levels currently, driver > > level (applies to all policies) and per-policy. > > > > Currently most of the drivers enables driver level boost support from the > > per-policy ->init() callback, which isn't really efficient as that gets called > > for each policy and then there is online/offline path too where this gets done > > unnecessarily. > > > > Also it is possible to have a scenario where not all cpufreq policies support > > boost frequencies. And letting sysfs (or other parts of the kernel) enable boost > > feature for that policy isn't correct. > > > > Simplify and cleanup handling of boost to solve these issues. > > I guess this depends on the previous series? Yes my series and the boost related patches in your tree.
On 2025/1/24 16:58, Viresh Kumar wrote: > With a later commit, the cpufreq core will call the ->set_boost() > callback only if the policy supports boost frequency. The > boost_supported flag is set by the cpufreq core if policy->freq_table is > set and one or more boost frequencies are present. > > For other drivers, the flag must be set explicitly. > > With this, the local variable boost_supported isn't required anymore. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/cppc_cpufreq.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c > index 7fa89b601d2a..08117fb9c1eb 100644 > --- a/drivers/cpufreq/cppc_cpufreq.c > +++ b/drivers/cpufreq/cppc_cpufreq.c > @@ -34,8 +34,6 @@ > */ > static LIST_HEAD(cpu_data_list); > > -static bool boost_supported; > - > static struct cpufreq_driver cppc_cpufreq_driver; > > #ifdef CONFIG_ACPI_CPPC_CPUFREQ_FIE > @@ -653,7 +651,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy) > * is supported. > */ > if (caps->highest_perf > caps->nominal_perf) > - boost_supported = true; > + policy->boost_supported = true; > > /* Set policy->cur to max now. The governors will adjust later. */ > policy->cur = cppc_perf_to_khz(caps, caps->highest_perf); > @@ -791,11 +789,6 @@ static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state) > struct cppc_perf_caps *caps = &cpu_data->perf_caps; > int ret; > > - if (!boost_supported) { > - pr_err("BOOST not supported by CPU or firmware\n"); > - return -EINVAL; > - } > - > if (state) > policy->max = cppc_perf_to_khz(caps, caps->highest_perf); > else I have a little question. With the old boost_supported flag as false, it will fail when you operate the global boost flag. But if you replace it with the per-policy boost_supported flag, the global boost_enabled flag can be set to true without any error, even no policy's boost_enabled flag changed. Is this interface behavior change OK?
On 06-02-25, 11:58, zhenglifeng (A) wrote: > On 2025/1/24 16:58, Viresh Kumar wrote: > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c > > index 7fa89b601d2a..08117fb9c1eb 100644 > > --- a/drivers/cpufreq/cppc_cpufreq.c > > +++ b/drivers/cpufreq/cppc_cpufreq.c > > @@ -34,8 +34,6 @@ > > */ > > static LIST_HEAD(cpu_data_list); > > > > -static bool boost_supported; > > - > > static struct cpufreq_driver cppc_cpufreq_driver; > > > > #ifdef CONFIG_ACPI_CPPC_CPUFREQ_FIE > > @@ -653,7 +651,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy) > > * is supported. > > */ > > if (caps->highest_perf > caps->nominal_perf) > > - boost_supported = true; > > + policy->boost_supported = true; > > > > /* Set policy->cur to max now. The governors will adjust later. */ > > policy->cur = cppc_perf_to_khz(caps, caps->highest_perf); > > @@ -791,11 +789,6 @@ static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state) > > struct cppc_perf_caps *caps = &cpu_data->perf_caps; > > int ret; > > > > - if (!boost_supported) { > > - pr_err("BOOST not supported by CPU or firmware\n"); > > - return -EINVAL; > > - } > > - > > if (state) > > policy->max = cppc_perf_to_khz(caps, caps->highest_perf); > > else > > I have a little question. With the old boost_supported flag as false, it > will fail when you operate the global boost flag. But if you replace it > with the per-policy boost_supported flag, the global boost_enabled flag can > be set to true without any error, even no policy's boost_enabled flag > changed. Is this interface behavior change OK? Right, so earlier even if a single policy didn't support boost, the code disabled boost for all of them. Or it was rather racy, as the last policy to be initialized will decide if boost will be supported or not. This is surely incorrect. The global boost flag can be enabled disabled without worrying about any individual policy. If the policy supports boost, it will follow the global boost here, else it shouldn't take part in the change. So yes, the new interface does the right thing here.
On 2025/1/24 16:58, Viresh Kumar wrote: > With a later commit, the cpufreq core will call the ->set_boost() > callback only if the policy supports boost frequency. The > boost_supported flag is set by the cpufreq core if policy->freq_table is > set and one or more boost frequencies are present. > > For other drivers, the flag must be set explicitly. > > With this, the local variable boost_supported isn't required anymore. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/cppc_cpufreq.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c > index 7fa89b601d2a..08117fb9c1eb 100644 > --- a/drivers/cpufreq/cppc_cpufreq.c > +++ b/drivers/cpufreq/cppc_cpufreq.c > @@ -34,8 +34,6 @@ > */ > static LIST_HEAD(cpu_data_list); > > -static bool boost_supported; > - > static struct cpufreq_driver cppc_cpufreq_driver; > > #ifdef CONFIG_ACPI_CPPC_CPUFREQ_FIE > @@ -653,7 +651,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy) > * is supported. > */ > if (caps->highest_perf > caps->nominal_perf) > - boost_supported = true; > + policy->boost_supported = true; > > /* Set policy->cur to max now. The governors will adjust later. */ > policy->cur = cppc_perf_to_khz(caps, caps->highest_perf); > @@ -791,11 +789,6 @@ static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state) > struct cppc_perf_caps *caps = &cpu_data->perf_caps; > int ret; > > - if (!boost_supported) { > - pr_err("BOOST not supported by CPU or firmware\n"); > - return -EINVAL; > - } > - > if (state) > policy->max = cppc_perf_to_khz(caps, caps->highest_perf); > else Reviewed-by: Lifeng Zheng <zhenglifeng1@huawei.com>