| Message ID | 1464231181-30741-2-git-send-email-smuckle@linaro.org |
|---|---|
| State | New |
| 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. -- viresh
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
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); -- viresh
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(+) -- 2.4.10