| Message ID | 1468441527-23534-2-git-send-email-smuckle@linaro.org |
|---|---|
| State | Accepted |
| Commit | e3c06236087051d5c62d60d0668588c370fda887 |
| Headers | show |
On 13-07-16, 13:25, 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(). This API returns the lowest driver > frequency equal or greater than the target frequency > (CPUFREQ_RELATION_L), subject to any policy (min/max) or driver > limitations. The mapping is also cached in the policy so that a > subsequent fast_switch operation can avoid repeating the same lookup. > > The API will call a new cpufreq driver callback, resolve_freq(), if it > has been registered by the driver. Otherwise the frequency is resolved > via cpufreq_frequency_table_target(). Rather than require ->target() > style drivers to provide a resolve_freq() callback it is left to the > caller to ensure that the driver implements this callback if necessary > to use cpufreq_driver_resolve_freq(). > > 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 | 16 ++++++++++++++++ > 2 files changed, 41 insertions(+) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 118b4f30a406..b696baeb249d 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -492,6 +492,29 @@ void cpufreq_disable_fast_switch(struct cpufreq_policy *policy) > } > EXPORT_SYMBOL_GPL(cpufreq_disable_fast_switch); > > +/** > + * cpufreq_driver_resolve_freq - Map a target frequency to a driver-supported > + * one. > + * @target_freq: target frequency to resolve. > + * > + * The target to driver frequency mapping is cached in the policy. > + * > + * Return: Lowest driver-supported frequency greater than or equal to the > + * given target_freq, subject to policy (min/max) and driver limitations. > + */ > +unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, > + unsigned int target_freq) > +{ > + target_freq = clamp_val(target_freq, policy->min, policy->max); > + policy->cached_target_freq = target_freq; > + if (cpufreq_driver->resolve_freq) > + return cpufreq_driver->resolve_freq(policy, target_freq); Any reason why we still have this call around ? I thought the whole attempt I made was to get rid of this :) The core can do this pretty much now by itself, why do we still want this call? Also, your series doesn't add a user for it yet, so better drop it for now. -- viresh -- 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 Thu, Jul 21, 2016 at 01:30:41PM -0700, Viresh Kumar wrote: > Okay, but in that case shouldn't we do something like this: > > unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, > unsigned int target_freq) > { > target_freq = clamp_val(target_freq, policy->min, policy->max); > policy->cached_target_freq = target_freq; > > if (cpufreq_driver->target_index) { > policy->cached_resolved_idx = > cpufreq_frequency_table_target(policy, target_freq, > CPUFREQ_RELATION_L); > return policy->freq_table[policy->cached_resolved_idx].frequency; > } > > if (cpufreq_driver->resolve_freq) > return cpufreq_driver->resolve_freq(policy, target_freq); > } Thanks for the review. My thinking (noted in the commit text) was that the caller of cpufreq_driver_resolve_freq() would verify that the driver supported the proper calls before using this API. This way it can be checked once, presumably in a governor's init routine. Checking the pointer over and over again in a fast path is wasteful. -- 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 Thu, Jul 21, 2016 at 04:21:31PM -0700, Steve Muckle wrote: > On Thu, Jul 21, 2016 at 01:30:41PM -0700, Viresh Kumar wrote: > > Okay, but in that case shouldn't we do something like this: > > > > unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, > > unsigned int target_freq) > > { > > target_freq = clamp_val(target_freq, policy->min, policy->max); > > policy->cached_target_freq = target_freq; > > > > if (cpufreq_driver->target_index) { > > policy->cached_resolved_idx = > > cpufreq_frequency_table_target(policy, target_freq, > > CPUFREQ_RELATION_L); > > return policy->freq_table[policy->cached_resolved_idx].frequency; > > } > > > > if (cpufreq_driver->resolve_freq) > > return cpufreq_driver->resolve_freq(policy, target_freq); > > } > > Thanks for the review. > > My thinking (noted in the commit text) was that the caller of > cpufreq_driver_resolve_freq() would verify that the driver supported the > proper calls before using this API. This way it can be checked once, > presumably in a governor's init routine. Checking the pointer over and > over again in a fast path is wasteful. I guess this isn't immediately possible as the governor can't see cpufreq_driver. I was hoping to change that however to allow cpufreq_driver_resolve_freq() to be inlined in schedutil to get rid of another function call... -- 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 21-07-16, 16:29, Steve Muckle wrote: > On Thu, Jul 21, 2016 at 04:21:31PM -0700, Steve Muckle wrote: > > On Thu, Jul 21, 2016 at 01:30:41PM -0700, Viresh Kumar wrote: > > > Okay, but in that case shouldn't we do something like this: > > > > > > unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, > > > unsigned int target_freq) > > > { > > > target_freq = clamp_val(target_freq, policy->min, policy->max); > > > policy->cached_target_freq = target_freq; > > > > > > if (cpufreq_driver->target_index) { > > > policy->cached_resolved_idx = > > > cpufreq_frequency_table_target(policy, target_freq, > > > CPUFREQ_RELATION_L); > > > return policy->freq_table[policy->cached_resolved_idx].frequency; > > > } > > > > > > if (cpufreq_driver->resolve_freq) > > > return cpufreq_driver->resolve_freq(policy, target_freq); > > > } > > > > Thanks for the review. > > > > My thinking (noted in the commit text) was that the caller of > > cpufreq_driver_resolve_freq() would verify that the driver supported the > > proper calls before using this API. This way it can be checked once, > > presumably in a governor's init routine. Checking the pointer over and > > over again in a fast path is wasteful. > > I guess this isn't immediately possible as the governor can't see > cpufreq_driver. I was hoping to change that however to allow > cpufreq_driver_resolve_freq() to be inlined in schedutil to get rid of > another function call... Well, you can do that by moving the newly created routine to cpufreq.h. -- viresh -- 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 Thu, Jul 21, 2016 at 04:30:03PM -0700, Viresh Kumar wrote: > On 21-07-16, 16:21, Steve Muckle wrote: > > On Thu, Jul 21, 2016 at 01:30:41PM -0700, Viresh Kumar wrote: > > > Okay, but in that case shouldn't we do something like this: > > > > > > unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, > > > unsigned int target_freq) > > > { > > > target_freq = clamp_val(target_freq, policy->min, policy->max); > > > policy->cached_target_freq = target_freq; > > > > > > if (cpufreq_driver->target_index) { > > > policy->cached_resolved_idx = > > > cpufreq_frequency_table_target(policy, target_freq, > > > CPUFREQ_RELATION_L); > > > return policy->freq_table[policy->cached_resolved_idx].frequency; > > > } > > > > > > if (cpufreq_driver->resolve_freq) > > > return cpufreq_driver->resolve_freq(policy, target_freq); > > > } > > > > Thanks for the review. > > > > My thinking (noted in the commit text) was that the caller of > > cpufreq_driver_resolve_freq() would verify that the driver supported the > > proper calls before using this API. > > Okay, but the caller isn't doing that today. Right? There is no caller yet. > > This way it can be checked once, > > presumably in a governor's init routine. Checking the pointer over and > > over again in a fast path is wasteful. > > But we just can not assume the callers to always check that the driver > has a ->target() and no ->resolve_freq(), and in that case not to call > this routine. We would be forced to add a WARN_ON() in that case here > to make sure we aren't trying to access a NULL ->resolve_freq. Why not? Can we not catch that in code review? If somehow this slips past and someone tries to use a driver with schedutil that doesn't provide either target_index or resolve_freq, it's not like it'll be a rare crash. It'll die immediately and in a very obvious way. > Over that, it will be used for a very small number of drivers which > still use the ->target() callback and anyway we are going to do a > function call for them. We can add a likely() here if that helps, but > some sort of checking is surely required IMO. > > And, this is a core API, which can be used for other governor's > tomorrow :) As another alternative, this could be caught in cpufreq driver initialization? I believe you suggested that originally, but I avoided it as I didn't want to have to implement resolve_freq() for every target() style driver. It sounds like there aren't many though. -- 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 Thu, Jul 21, 2016 at 04:36:48PM -0700, Steve Muckle wrote: > On Thu, Jul 21, 2016 at 04:30:03PM -0700, Viresh Kumar wrote: > > On 21-07-16, 16:21, Steve Muckle wrote: > > > On Thu, Jul 21, 2016 at 01:30:41PM -0700, Viresh Kumar wrote: > > > > Okay, but in that case shouldn't we do something like this: > > > > > > > > unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, > > > > unsigned int target_freq) > > > > { > > > > target_freq = clamp_val(target_freq, policy->min, policy->max); > > > > policy->cached_target_freq = target_freq; > > > > > > > > if (cpufreq_driver->target_index) { > > > > policy->cached_resolved_idx = > > > > cpufreq_frequency_table_target(policy, target_freq, > > > > CPUFREQ_RELATION_L); > > > > return policy->freq_table[policy->cached_resolved_idx].frequency; > > > > } > > > > > > > > if (cpufreq_driver->resolve_freq) > > > > return cpufreq_driver->resolve_freq(policy, target_freq); > > > > } > > > > > > Thanks for the review. > > > > > > My thinking (noted in the commit text) was that the caller of > > > cpufreq_driver_resolve_freq() would verify that the driver supported the > > > proper calls before using this API. > > > > Okay, but the caller isn't doing that today. Right? > > There is no caller yet. Sorry, of course this is not true. I'm still of the opinion that modifying the governor (I could fix up schedutil now) or adding a check in driver init would be better than any unnecessary logic in the fast path. -- 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 Thu, Jul 21, 2016 at 04:36:48PM -0700, Steve Muckle wrote: > As another alternative, this could be caught in cpufreq driver > initialization? I believe you suggested that originally, but I avoided > it as I didn't want to have to implement resolve_freq() for every > target() style driver. It sounds like there aren't many though. Going back and checking I see I was thinking of your suggestion that cpufreq_register_driver() check that only target() drivers offer a resolve_freq() callback. I put a comment for this in cpufreq.h but not a check - I could add a check in another patch if you like. Long term as I was mentioning in the other thread I think it'd be good if the current target() drivers were modified to supply resolve_freq(), and that cpufreq_register_driver() were again changed to require it for those drivers. -- 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 21-07-16, 17:44, Steve Muckle wrote: > Going back and checking I see I was thinking of your suggestion that > cpufreq_register_driver() check that only target() drivers offer a > resolve_freq() callback. I put a comment for this in cpufreq.h but not a > check - I could add a check in another patch if you like. That can be done as we aren't supporting the ->resolve_freq() callback for ->target_index() drivers. > Long term as I was mentioning in the other thread I think it'd be good > if the current target() drivers were modified to supply resolve_freq(), > and that cpufreq_register_driver() were again changed to require it for > those drivers. There is no need for us to force this, its really optional for such platforms. Worst case, schedutil wouldn't work at the best, so what? Its a platform driver's choice, isn't it ? -- viresh -- 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 Fri, Jul 22, 2016 at 08:16:42AM -0700, Viresh Kumar wrote: > > Long term as I was mentioning in the other thread I think it'd be good > > if the current target() drivers were modified to supply resolve_freq(), > > and that cpufreq_register_driver() were again changed to require it for > > those drivers. > > There is no need for us to force this, its really optional for such > platforms. Worst case, schedutil wouldn't work at the best, so what? > Its a platform driver's choice, isn't it ? This would be in the context of then being able to remove the additional if statement from the hot path. To reply to the suggestion of using likely() here, I'm partial to solving it without that because I'm guessing likely() has to be an architecture-dependent thing? It seems cleaner to me if the existing few target() drivers were augmented to provide the resolve_freq() calback and its presence required. But it's certainly not a big deal and won't affect any platforms I'm involved with, at least for now. Maybe I could work on those target() drivers if things change. -- 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 118b4f30a406..b696baeb249d 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -492,6 +492,29 @@ void cpufreq_disable_fast_switch(struct cpufreq_policy *policy) } EXPORT_SYMBOL_GPL(cpufreq_disable_fast_switch); +/** + * cpufreq_driver_resolve_freq - Map a target frequency to a driver-supported + * one. + * @target_freq: target frequency to resolve. + * + * The target to driver frequency mapping is cached in the policy. + * + * Return: Lowest driver-supported frequency greater than or equal to the + * given target_freq, subject to policy (min/max) and driver limitations. + */ +unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, + unsigned int target_freq) +{ + target_freq = clamp_val(target_freq, policy->min, policy->max); + policy->cached_target_freq = target_freq; + if (cpufreq_driver->resolve_freq) + return cpufreq_driver->resolve_freq(policy, target_freq); + policy->cached_resolved_idx = + cpufreq_frequency_table_target(policy, target_freq, + CPUFREQ_RELATION_L); + return policy->freq_table[policy->cached_resolved_idx].frequency; +} + /********************************************************************* * SYSFS INTERFACE * *********************************************************************/ @@ -2199,6 +2222,8 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, policy->min = new_policy->min; policy->max = new_policy->max; + policy->cached_target_freq = UINT_MAX; + pr_debug("new min and max freqs are %u - %u kHz\n", policy->min, policy->max); diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index c6410b1b2490..631ba33bbe9f 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -120,6 +120,10 @@ struct cpufreq_policy { bool fast_switch_possible; bool fast_switch_enabled; + /* Cached frequency lookup from cpufreq_driver_resolve_freq. */ + unsigned int cached_target_freq; + int cached_resolved_idx; + /* Synchronization for frequency transitions */ bool transition_ongoing; /* Tracks transition status */ spinlock_t transition_lock; @@ -270,6 +274,16 @@ struct cpufreq_driver { unsigned int index); unsigned int (*fast_switch)(struct cpufreq_policy *policy, unsigned int target_freq); + + /* + * Caches and returns the lowest driver-supported frequency greater than + * or equal to the target frequency, subject to any driver limitations. + * Does not set the frequency. Only to be implemented for drivers with + * target(). + */ + unsigned int (*resolve_freq)(struct cpufreq_policy *policy, + unsigned int target_freq); + /* * Only for drivers with target_index() and CPUFREQ_ASYNC_NOTIFICATION * unset. @@ -501,6 +515,8 @@ 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(). This API returns the lowest driver frequency equal or greater than the target frequency (CPUFREQ_RELATION_L), subject to any policy (min/max) or driver limitations. The mapping is also cached in the policy so that a subsequent fast_switch operation can avoid repeating the same lookup. The API will call a new cpufreq driver callback, resolve_freq(), if it has been registered by the driver. Otherwise the frequency is resolved via cpufreq_frequency_table_target(). Rather than require ->target() style drivers to provide a resolve_freq() callback it is left to the caller to ensure that the driver implements this callback if necessary to use cpufreq_driver_resolve_freq(). 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 | 16 ++++++++++++++++ 2 files changed, 41 insertions(+) -- 2.7.3 -- 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