| Message ID | 20200722093732.14297-3-ionela.voinescu@arm.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | cpufreq: improve frequency invariance support | expand |
On Wed, Jul 22, 2020 at 11:38 AM Ionela Voinescu <ionela.voinescu@arm.com> wrote: > > While the move of the invariance setter calls (arch_set_freq_scale()) > from cpufreq drivers to cpufreq core maintained the previous > functionality for existing drivers that use target_index() and > fast_switch() for frequency switching, it also gives the possibility > of adding support for users of the target() callback, which is exploited > here. > > To be noted that the target() callback has been flagged as deprecated > since: > > commit 9c0ebcf78fde ("cpufreq: Implement light weight ->target_index() routine") > > It also doesn't have that many users: > > cpufreq-nforce2.c:371:2: .target = nforce2_target, > cppc_cpufreq.c:416:2: .target = cppc_cpufreq_set_target, > gx-suspmod.c:439:2: .target = cpufreq_gx_target, > pcc-cpufreq.c:573:2: .target = pcc_cpufreq_target, > > Similarly to the path taken for target_index() calls in the cpufreq core > during a frequency change, all of the drivers above will mark the end of a > frequency change by a call to cpufreq_freq_transition_end(). > > Therefore, cpufreq_freq_transition_end() can be used as the location for > the arch_set_freq_scale() call to potentially inform the scheduler of the > frequency change. > > This change maintains the previous functionality for the drivers that > implement the target_index() callback, while also adding support for the > few drivers that implement the deprecated target() callback. > > Two notes are worthwhile here: > - In __target_index(), cpufreq_freq_transition_end() is called only for > drivers that have synchronous notifications enabled. There is only one > driver that disables them, > > drivers/cpufreq/powernow-k8.c:1142: .flags = CPUFREQ_ASYNC_NOTIFICATION, > > which is deprecated. > > - Despite marking a successful frequency change, many cpufreq drivers > will populate the new policy->cur with the new requested frequency, > although this might not be the one granted by the hardware. > > Therefore, the call to arch_set_freq_scale() is a "best effort" one, > and it is up to the architecture if the new frequency is used in the > new frequency scale factor setting or eventually used by the scheduler. > The architecture is in a better position to decide if it has better > methods to obtain more accurate information regarding the current > frequency (for example the use of counters). > > Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/cpufreq.c | 15 ++++++--------- > 1 file changed, 6 insertions(+), 9 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index bac4101546db..3497c1cd6818 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -448,6 +448,10 @@ void cpufreq_freq_transition_end(struct cpufreq_policy *policy, > > cpufreq_notify_post_transition(policy, freqs, transition_failed); > > + arch_set_freq_scale(policy->related_cpus, > + policy->cur, > + policy->cpuinfo.max_freq); > + > policy->transition_ongoing = false; > policy->transition_task = NULL; > > @@ -2159,7 +2163,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, > unsigned int relation) > { > unsigned int old_target_freq = target_freq; > - int index, retval; > + int index; > > if (cpufreq_disabled()) > return -ENODEV; > @@ -2190,14 +2194,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, > > index = cpufreq_frequency_table_target(policy, target_freq, relation); > > - retval = __target_index(policy, index); > - > - if (!retval) > - arch_set_freq_scale(policy->related_cpus, > - policy->freq_table[index].frequency, > - policy->cpuinfo.max_freq); > - > - return retval; > + return __target_index(policy, index); > } > EXPORT_SYMBOL_GPL(__cpufreq_driver_target); > > -- I would fold this patch into the previous one. I don't see much reason for it to be separate and it looks like folding it in would cause the previous patch to be simpler.
On Monday 27 Jul 2020 at 15:52:41 (+0200), Rafael J. Wysocki wrote: > On Wed, Jul 22, 2020 at 11:38 AM Ionela Voinescu > <ionela.voinescu@arm.com> wrote: > > > > While the move of the invariance setter calls (arch_set_freq_scale()) > > from cpufreq drivers to cpufreq core maintained the previous > > functionality for existing drivers that use target_index() and > > fast_switch() for frequency switching, it also gives the possibility > > of adding support for users of the target() callback, which is exploited > > here. > > > > To be noted that the target() callback has been flagged as deprecated > > since: > > > > commit 9c0ebcf78fde ("cpufreq: Implement light weight ->target_index() routine") > > > > It also doesn't have that many users: > > > > cpufreq-nforce2.c:371:2: .target = nforce2_target, > > cppc_cpufreq.c:416:2: .target = cppc_cpufreq_set_target, > > gx-suspmod.c:439:2: .target = cpufreq_gx_target, > > pcc-cpufreq.c:573:2: .target = pcc_cpufreq_target, > > > > Similarly to the path taken for target_index() calls in the cpufreq core > > during a frequency change, all of the drivers above will mark the end of a > > frequency change by a call to cpufreq_freq_transition_end(). > > > > Therefore, cpufreq_freq_transition_end() can be used as the location for > > the arch_set_freq_scale() call to potentially inform the scheduler of the > > frequency change. > > > > This change maintains the previous functionality for the drivers that > > implement the target_index() callback, while also adding support for the > > few drivers that implement the deprecated target() callback. > > > > Two notes are worthwhile here: > > - In __target_index(), cpufreq_freq_transition_end() is called only for > > drivers that have synchronous notifications enabled. There is only one > > driver that disables them, > > > > drivers/cpufreq/powernow-k8.c:1142: .flags = CPUFREQ_ASYNC_NOTIFICATION, > > > > which is deprecated. > > > > - Despite marking a successful frequency change, many cpufreq drivers > > will populate the new policy->cur with the new requested frequency, > > although this might not be the one granted by the hardware. > > > > Therefore, the call to arch_set_freq_scale() is a "best effort" one, > > and it is up to the architecture if the new frequency is used in the > > new frequency scale factor setting or eventually used by the scheduler. > > The architecture is in a better position to decide if it has better > > methods to obtain more accurate information regarding the current > > frequency (for example the use of counters). > > [..] > I would fold this patch into the previous one. > > I don't see much reason for it to be separate and it looks like > folding it in would cause the previous patch to be simpler. I kept it separate in this version as a proposal to move the call to cpufreq_freq_transition_end() and properly justify it in the commit message. I'll squash it into the previous one, as recommended. Thanks, Ionela.
On 22-07-20, 10:37, Ionela Voinescu wrote: > While the move of the invariance setter calls (arch_set_freq_scale()) > from cpufreq drivers to cpufreq core maintained the previous > functionality for existing drivers that use target_index() and > fast_switch() for frequency switching, it also gives the possibility > of adding support for users of the target() callback, which is exploited > here. > > To be noted that the target() callback has been flagged as deprecated > since: > > commit 9c0ebcf78fde ("cpufreq: Implement light weight ->target_index() routine") > > It also doesn't have that many users: > > cpufreq-nforce2.c:371:2: .target = nforce2_target, > cppc_cpufreq.c:416:2: .target = cppc_cpufreq_set_target, > gx-suspmod.c:439:2: .target = cpufreq_gx_target, > pcc-cpufreq.c:573:2: .target = pcc_cpufreq_target, > > Similarly to the path taken for target_index() calls in the cpufreq core > during a frequency change, all of the drivers above will mark the end of a > frequency change by a call to cpufreq_freq_transition_end(). > > Therefore, cpufreq_freq_transition_end() can be used as the location for > the arch_set_freq_scale() call to potentially inform the scheduler of the > frequency change. > > This change maintains the previous functionality for the drivers that > implement the target_index() callback, while also adding support for the > few drivers that implement the deprecated target() callback. > > Two notes are worthwhile here: > - In __target_index(), cpufreq_freq_transition_end() is called only for > drivers that have synchronous notifications enabled. There is only one > driver that disables them, > > drivers/cpufreq/powernow-k8.c:1142: .flags = CPUFREQ_ASYNC_NOTIFICATION, > > which is deprecated. I don't think this is deprecated.
Hi Viresh, On Thursday 30 Jul 2020 at 09:43:34 (+0530), Viresh Kumar wrote: > On 22-07-20, 10:37, Ionela Voinescu wrote: > > While the move of the invariance setter calls (arch_set_freq_scale()) > > from cpufreq drivers to cpufreq core maintained the previous > > functionality for existing drivers that use target_index() and > > fast_switch() for frequency switching, it also gives the possibility > > of adding support for users of the target() callback, which is exploited > > here. > > > > To be noted that the target() callback has been flagged as deprecated > > since: > > > > commit 9c0ebcf78fde ("cpufreq: Implement light weight ->target_index() routine") > > > > It also doesn't have that many users: > > > > cpufreq-nforce2.c:371:2: .target = nforce2_target, > > cppc_cpufreq.c:416:2: .target = cppc_cpufreq_set_target, > > gx-suspmod.c:439:2: .target = cpufreq_gx_target, > > pcc-cpufreq.c:573:2: .target = pcc_cpufreq_target, > > > > Similarly to the path taken for target_index() calls in the cpufreq core > > during a frequency change, all of the drivers above will mark the end of a > > frequency change by a call to cpufreq_freq_transition_end(). > > > > Therefore, cpufreq_freq_transition_end() can be used as the location for > > the arch_set_freq_scale() call to potentially inform the scheduler of the > > frequency change. > > > > This change maintains the previous functionality for the drivers that > > implement the target_index() callback, while also adding support for the > > few drivers that implement the deprecated target() callback. > > > > Two notes are worthwhile here: > > - In __target_index(), cpufreq_freq_transition_end() is called only for > > drivers that have synchronous notifications enabled. There is only one > > driver that disables them, > > > > drivers/cpufreq/powernow-k8.c:1142: .flags = CPUFREQ_ASYNC_NOTIFICATION, > > > > which is deprecated. > > I don't think this is deprecated. Sorry, possibly 'deprecated' is a strong word. As far as I knew acpi_cpufreq was recommended more recently for K8/K10 CPUs so that's why I decided not to create a special case for it, also considering that it was not supporting cpufreq-based frequency invariance to begin with. We could support this as well by having a call to arch_set_freq_scale() on the else path in __target_index(). But given that there was only this one user of CPUFREQ_ASYNC_NOTIFICATION, I thought I'd propose this simpler version first. Let me know if my reasoning is wrong. Thank you, Ionela. > > -- > viresh
On 03-08-20, 14:58, Ionela Voinescu wrote: > Hi Viresh, > > On Thursday 30 Jul 2020 at 09:43:34 (+0530), Viresh Kumar wrote: > > On 22-07-20, 10:37, Ionela Voinescu wrote: > > > While the move of the invariance setter calls (arch_set_freq_scale()) > > > from cpufreq drivers to cpufreq core maintained the previous > > > functionality for existing drivers that use target_index() and > > > fast_switch() for frequency switching, it also gives the possibility > > > of adding support for users of the target() callback, which is exploited > > > here. > > > > > > To be noted that the target() callback has been flagged as deprecated > > > since: > > > > > > commit 9c0ebcf78fde ("cpufreq: Implement light weight ->target_index() routine") > > > > > > It also doesn't have that many users: > > > > > > cpufreq-nforce2.c:371:2: .target = nforce2_target, > > > cppc_cpufreq.c:416:2: .target = cppc_cpufreq_set_target, > > > gx-suspmod.c:439:2: .target = cpufreq_gx_target, > > > pcc-cpufreq.c:573:2: .target = pcc_cpufreq_target, > > > > > > Similarly to the path taken for target_index() calls in the cpufreq core > > > during a frequency change, all of the drivers above will mark the end of a > > > frequency change by a call to cpufreq_freq_transition_end(). > > > > > > Therefore, cpufreq_freq_transition_end() can be used as the location for > > > the arch_set_freq_scale() call to potentially inform the scheduler of the > > > frequency change. > > > > > > This change maintains the previous functionality for the drivers that > > > implement the target_index() callback, while also adding support for the > > > few drivers that implement the deprecated target() callback. > > > > > > Two notes are worthwhile here: > > > - In __target_index(), cpufreq_freq_transition_end() is called only for > > > drivers that have synchronous notifications enabled. There is only one > > > driver that disables them, > > > > > > drivers/cpufreq/powernow-k8.c:1142: .flags = CPUFREQ_ASYNC_NOTIFICATION, > > > > > > which is deprecated. > > > > I don't think this is deprecated. Heh, maybe I misunderstood. I thought you are talking about the flag, while you were talking about the driver. > Sorry, possibly 'deprecated' is a strong word. > > As far as I knew acpi_cpufreq was recommended more recently for K8/K10 > CPUs so that's why I decided not to create a special case for it, also > considering that it was not supporting cpufreq-based frequency > invariance to begin with. > > We could support this as well by having a call to arch_set_freq_scale() > on the else path in __target_index(). But given that there was only this > one user of CPUFREQ_ASYNC_NOTIFICATION, I thought I'd propose this simpler > version first. > > Let me know if my reasoning is wrong. Nevertheless, I don't think you need to mention this detail in changelog for powernow-k8 as cpufreq_freq_transition_end() does get called for it as well, by the driver instead of the core.
On Tuesday 04 Aug 2020 at 11:56:11 (+0530), Viresh Kumar wrote: [..] > > > > - In __target_index(), cpufreq_freq_transition_end() is called only for > > > > drivers that have synchronous notifications enabled. There is only one > > > > driver that disables them, > > > > > > > > drivers/cpufreq/powernow-k8.c:1142: .flags = CPUFREQ_ASYNC_NOTIFICATION, > > > > > > > > which is deprecated. > > > > > > I don't think this is deprecated. > > Heh, maybe I misunderstood. I thought you are talking about the flag, > while you were talking about the driver. > > > Sorry, possibly 'deprecated' is a strong word. > > > > As far as I knew acpi_cpufreq was recommended more recently for K8/K10 > > CPUs so that's why I decided not to create a special case for it, also > > considering that it was not supporting cpufreq-based frequency > > invariance to begin with. > > > > We could support this as well by having a call to arch_set_freq_scale() > > on the else path in __target_index(). But given that there was only this > > one user of CPUFREQ_ASYNC_NOTIFICATION, I thought I'd propose this simpler > > version first. > > > > Let me know if my reasoning is wrong. > > Nevertheless, I don't think you need to mention this detail in > changelog for powernow-k8 as cpufreq_freq_transition_end() does get > called for it as well, by the driver instead of the core. > Agreed! Many thanks, Ionela. > -- > viresh
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index bac4101546db..3497c1cd6818 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -448,6 +448,10 @@ void cpufreq_freq_transition_end(struct cpufreq_policy *policy, cpufreq_notify_post_transition(policy, freqs, transition_failed); + arch_set_freq_scale(policy->related_cpus, + policy->cur, + policy->cpuinfo.max_freq); + policy->transition_ongoing = false; policy->transition_task = NULL; @@ -2159,7 +2163,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, unsigned int relation) { unsigned int old_target_freq = target_freq; - int index, retval; + int index; if (cpufreq_disabled()) return -ENODEV; @@ -2190,14 +2194,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, index = cpufreq_frequency_table_target(policy, target_freq, relation); - retval = __target_index(policy, index); - - if (!retval) - arch_set_freq_scale(policy->related_cpus, - policy->freq_table[index].frequency, - policy->cpuinfo.max_freq); - - return retval; + return __target_index(policy, index); } EXPORT_SYMBOL_GPL(__cpufreq_driver_target);
While the move of the invariance setter calls (arch_set_freq_scale()) from cpufreq drivers to cpufreq core maintained the previous functionality for existing drivers that use target_index() and fast_switch() for frequency switching, it also gives the possibility of adding support for users of the target() callback, which is exploited here. To be noted that the target() callback has been flagged as deprecated since: commit 9c0ebcf78fde ("cpufreq: Implement light weight ->target_index() routine") It also doesn't have that many users: cpufreq-nforce2.c:371:2: .target = nforce2_target, cppc_cpufreq.c:416:2: .target = cppc_cpufreq_set_target, gx-suspmod.c:439:2: .target = cpufreq_gx_target, pcc-cpufreq.c:573:2: .target = pcc_cpufreq_target, Similarly to the path taken for target_index() calls in the cpufreq core during a frequency change, all of the drivers above will mark the end of a frequency change by a call to cpufreq_freq_transition_end(). Therefore, cpufreq_freq_transition_end() can be used as the location for the arch_set_freq_scale() call to potentially inform the scheduler of the frequency change. This change maintains the previous functionality for the drivers that implement the target_index() callback, while also adding support for the few drivers that implement the deprecated target() callback. Two notes are worthwhile here: - In __target_index(), cpufreq_freq_transition_end() is called only for drivers that have synchronous notifications enabled. There is only one driver that disables them, drivers/cpufreq/powernow-k8.c:1142: .flags = CPUFREQ_ASYNC_NOTIFICATION, which is deprecated. - Despite marking a successful frequency change, many cpufreq drivers will populate the new policy->cur with the new requested frequency, although this might not be the one granted by the hardware. Therefore, the call to arch_set_freq_scale() is a "best effort" one, and it is up to the architecture if the new frequency is used in the new frequency scale factor setting or eventually used by the scheduler. The architecture is in a better position to decide if it has better methods to obtain more accurate information regarding the current frequency (for example the use of counters). Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/cpufreq.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)