| Message ID | 1464231181-30741-2-git-send-email-smuckle@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 25-05-16, 19:52, Steve Muckle wrote: > Cpufreq governors may need to know what a particular target frequency > maps to in the driver without necessarily wanting to set the frequency. > Support this operation via a new cpufreq API, > cpufreq_driver_resolve_freq(). > > The above API will call a new cpufreq driver callback, resolve_freq(), > if it has been registered by the driver. If that callback has not been > registered and a frequency table is available then the frequency table > is walked using cpufreq_frequency_table_target(). > > UINT_MAX is returned if no driver callback or frequency table is > available. Why should we return UINT_MAX here? We should return target_freq, no ? > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Signed-off-by: Steve Muckle <smuckle@linaro.org> > --- > drivers/cpufreq/cpufreq.c | 25 +++++++++++++++++++++++++ > include/linux/cpufreq.h | 11 +++++++++++ > 2 files changed, 36 insertions(+) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 77d77a4e3b74..3b44f4bdc071 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1849,6 +1849,31 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy, > } > EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch); > > +unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, > + unsigned int target_freq) > +{ > + struct cpufreq_frequency_table *freq_table; > + int index, retval; > + > + clamp_val(target_freq, policy->min, policy->max); > + > + if (cpufreq_driver->resolve_freq) > + return cpufreq_driver->resolve_freq(policy, target_freq); > + > + freq_table = cpufreq_frequency_get_table(policy->cpu); I have sent a separate patch to provide a light weight alternative to this. If that gets accepted, we can switch over to using it. > + if (!freq_table) > + return UINT_MAX; > + > + retval = cpufreq_frequency_table_target(policy, freq_table, > + target_freq, CPUFREQ_RELATION_L, > + &index); > + if (retval) > + return UINT_MAX; > + > + return freq_table[index].frequency; > +} > +EXPORT_SYMBOL_GPL(cpufreq_driver_resolve_freq); > + > /* Must set freqs->new to intermediate frequency */ > static int __target_intermediate(struct cpufreq_policy *policy, > struct cpufreq_freqs *freqs, int index) > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > index 4e81e08db752..675f17f98e75 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -271,6 +271,13 @@ struct cpufreq_driver { > int (*target_intermediate)(struct cpufreq_policy *policy, > unsigned int index); > > + /* > + * Return the driver-supported frequency that a particular target > + * frequency maps to (does not set the new frequency). > + */ > + unsigned int (*resolve_freq)(struct cpufreq_policy *policy, > + unsigned int target_freq); We have 3 categories of cpufreq-drivers today: 1. setpolicy drivers: They don't use the cpufreq governors we are working on. 2. non-setpolicy drivers: A. with ->target_index() callback, these will always provide a freq-table. B. with ->target() callback, ONLY these should be allowed to provide the ->resolve_freq() callback and no one else. And so I would suggest adding an additional check in cpufreq_register_driver() to catch incorrect usage of this callback.
On Thu, May 26, 2016 at 11:55:14AM +0530, Viresh Kumar wrote: > On 25-05-16, 19:52, Steve Muckle wrote: > > Cpufreq governors may need to know what a particular target frequency > > maps to in the driver without necessarily wanting to set the frequency. > > Support this operation via a new cpufreq API, > > cpufreq_driver_resolve_freq(). > > > > The above API will call a new cpufreq driver callback, resolve_freq(), > > if it has been registered by the driver. If that callback has not been > > registered and a frequency table is available then the frequency table > > is walked using cpufreq_frequency_table_target(). > > > > UINT_MAX is returned if no driver callback or frequency table is > > available. > > Why should we return UINT_MAX here? We should return target_freq, no ? My goal here was to have the system operate in this case in a manner that is obviously not optimized (running at fmax), so the platform owner realizes that the cpufreq driver doesn't fully support the schedutil governor. I was originally going to just return an error code but that also means having to check for it which would be nice to avoid if possible on this fast path. > > > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Signed-off-by: Steve Muckle <smuckle@linaro.org> > > --- > > drivers/cpufreq/cpufreq.c | 25 +++++++++++++++++++++++++ > > include/linux/cpufreq.h | 11 +++++++++++ > > 2 files changed, 36 insertions(+) > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index 77d77a4e3b74..3b44f4bdc071 100644 > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -1849,6 +1849,31 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy, > > } > > EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch); > > > > +unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, > > + unsigned int target_freq) > > +{ > > + struct cpufreq_frequency_table *freq_table; > > + int index, retval; > > + > > + clamp_val(target_freq, policy->min, policy->max); > > + > > + if (cpufreq_driver->resolve_freq) > > + return cpufreq_driver->resolve_freq(policy, target_freq); > > + > > + freq_table = cpufreq_frequency_get_table(policy->cpu); > > I have sent a separate patch to provide a light weight alternative to > this. If that gets accepted, we can switch over to using it. Sure. > > > + if (!freq_table) > > + return UINT_MAX; > > + > > + retval = cpufreq_frequency_table_target(policy, freq_table, > > + target_freq, CPUFREQ_RELATION_L, > > + &index); > > + if (retval) > > + return UINT_MAX; > > + > > + return freq_table[index].frequency; > > +} > > +EXPORT_SYMBOL_GPL(cpufreq_driver_resolve_freq); > > + > > /* Must set freqs->new to intermediate frequency */ > > static int __target_intermediate(struct cpufreq_policy *policy, > > struct cpufreq_freqs *freqs, int index) > > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > > index 4e81e08db752..675f17f98e75 100644 > > --- a/include/linux/cpufreq.h > > +++ b/include/linux/cpufreq.h > > @@ -271,6 +271,13 @@ struct cpufreq_driver { > > int (*target_intermediate)(struct cpufreq_policy *policy, > > unsigned int index); > > > > + /* > > + * Return the driver-supported frequency that a particular target > > + * frequency maps to (does not set the new frequency). > > + */ > > + unsigned int (*resolve_freq)(struct cpufreq_policy *policy, > > + unsigned int target_freq); > > We have 3 categories of cpufreq-drivers today: > 1. setpolicy drivers: They don't use the cpufreq governors we are > working on. > 2. non-setpolicy drivers: > A. with ->target_index() callback, these will always provide a > freq-table. > B. with ->target() callback, ONLY these should be allowed to provide > the ->resolve_freq() callback and no one else. > > And so I would suggest adding an additional check in > cpufreq_register_driver() to catch incorrect usage of this callback. I'll reply to this in the next email on patch 2... thanks, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30-05-16, 08:31, Steve Muckle wrote: > My goal here was to have the system operate in this case in a manner > that is obviously not optimized (running at fmax), so the platform owner > realizes that the cpufreq driver doesn't fully support the schedutil > governor. > > I was originally going to just return an error code but that also means > having to check for it which would be nice to avoid if possible on this > fast path. Okay, I get what you are saying. But all we are doing here is to make things fast by not sending IPIs, etc. That should *not* lead to a behavior where the frequency stays at MAX all the time even if the driver doesn't provide this callback or the freq-table. If we just return the target_freq in this case instead of UINT_MAX, the platform may eventually have some unnecessary IPIs, wakeups, etc, but its frequency will still be switched properly. Wouldn't that be a better choice ?
On 25-05-16, 19:52, Steve Muckle wrote: > +unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, > + unsigned int target_freq) > +{ > + struct cpufreq_frequency_table *freq_table; > + int index, retval; > + > + clamp_val(target_freq, policy->min, policy->max); Rafael will kill me for this, as I have fallen into the same trap and copied your *incorrect* code :( This is a useless statement unless you do: target_freq = clamp_val(target_freq, policy->min, policy->max);
On Tue, May 31, 2016 at 04:44:51PM +0530, Viresh Kumar wrote: > On 25-05-16, 19:52, Steve Muckle wrote: > > +unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, > > + unsigned int target_freq) > > +{ > > + struct cpufreq_frequency_table *freq_table; > > + int index, retval; > > + > > + clamp_val(target_freq, policy->min, policy->max); > > Rafael will kill me for this, as I have fallen into the same trap and > copied your *incorrect* code :( > > This is a useless statement unless you do: > > target_freq = clamp_val(target_freq, policy->min, policy->max); Well I wouldn't worry too much, I copied it from Rafael's code in cpufreq_driver_fast_switch() which also has this problem and needs to be fixed. Perhaps clamp_val could be changed to force the compiler to check that its return value is being used. thanks, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 31, 2016 at 11:00:11AM +0530, Viresh Kumar wrote: > On 30-05-16, 08:31, Steve Muckle wrote: > > My goal here was to have the system operate in this case in a manner > > that is obviously not optimized (running at fmax), so the platform owner > > realizes that the cpufreq driver doesn't fully support the schedutil > > governor. > > > > I was originally going to just return an error code but that also means > > having to check for it which would be nice to avoid if possible on this > > fast path. > > Okay, I get what you are saying. > > But all we are doing here is to make things fast by not sending IPIs, > etc. That should *not* lead to a behavior where the frequency stays at > MAX all the time even if the driver doesn't provide this callback or > the freq-table. > > If we just return the target_freq in this case instead of UINT_MAX, > the platform may eventually have some unnecessary IPIs, wakeups, etc, > but its frequency will still be switched properly. > > Wouldn't that be a better choice ? I'm still concerned that a platform owner may use this and accept suboptimal perf/power because they aren't aware their cpufreq driver is not fully compliant. But I agree it'd be better to have it work as well as it can. I will make the change. Maybe a warning message can be added when schedutil initializes if resolve_freq is not supported. thanks, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 77d77a4e3b74..3b44f4bdc071 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1849,6 +1849,31 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy, } EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch); +unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, + unsigned int target_freq) +{ + struct cpufreq_frequency_table *freq_table; + int index, retval; + + clamp_val(target_freq, policy->min, policy->max); + + if (cpufreq_driver->resolve_freq) + return cpufreq_driver->resolve_freq(policy, target_freq); + + freq_table = cpufreq_frequency_get_table(policy->cpu); + if (!freq_table) + return UINT_MAX; + + retval = cpufreq_frequency_table_target(policy, freq_table, + target_freq, CPUFREQ_RELATION_L, + &index); + if (retval) + return UINT_MAX; + + return freq_table[index].frequency; +} +EXPORT_SYMBOL_GPL(cpufreq_driver_resolve_freq); + /* Must set freqs->new to intermediate frequency */ static int __target_intermediate(struct cpufreq_policy *policy, struct cpufreq_freqs *freqs, int index) diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 4e81e08db752..675f17f98e75 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -271,6 +271,13 @@ struct cpufreq_driver { int (*target_intermediate)(struct cpufreq_policy *policy, unsigned int index); + /* + * Return the driver-supported frequency that a particular target + * frequency maps to (does not set the new frequency). + */ + unsigned int (*resolve_freq)(struct cpufreq_policy *policy, + unsigned int target_freq); + /* should be defined, if possible */ unsigned int (*get)(unsigned int cpu); @@ -487,6 +494,10 @@ int cpufreq_driver_target(struct cpufreq_policy *policy, int __cpufreq_driver_target(struct cpufreq_policy *policy, unsigned int target_freq, unsigned int relation); + +unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, + unsigned int target_freq); + int cpufreq_register_governor(struct cpufreq_governor *governor); void cpufreq_unregister_governor(struct cpufreq_governor *governor);
Cpufreq governors may need to know what a particular target frequency maps to in the driver without necessarily wanting to set the frequency. Support this operation via a new cpufreq API, cpufreq_driver_resolve_freq(). The above API will call a new cpufreq driver callback, resolve_freq(), if it has been registered by the driver. If that callback has not been registered and a frequency table is available then the frequency table is walked using cpufreq_frequency_table_target(). UINT_MAX is returned if no driver callback or frequency table is available. Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Signed-off-by: Steve Muckle <smuckle@linaro.org> --- drivers/cpufreq/cpufreq.c | 25 +++++++++++++++++++++++++ include/linux/cpufreq.h | 11 +++++++++++ 2 files changed, 36 insertions(+)