| Message ID | 1464231181-30741-3-git-send-email-smuckle@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 25-05-16, 19:53, Steve Muckle wrote: > Support the new resolve_freq cpufreq callback which resolves a target > frequency to a driver-supported frequency without actually setting it. And here is the first abuser of this API as I was talking about in the earlier patch :) But, I know why you are doing it and I think we can do it differently. So, lets assume that the ->resolve_freq() callback will ONLY be provided by the drivers which also provide a ->target() callback. i.e. not by acpi-cpufreq at least. > The target frequency and resolved frequency table entry are cached so > that a subsequent fast_switch operation may avoid the frequency table > walk assuming the requested target frequency is the same. > > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Signed-off-by: Steve Muckle <smuckle@linaro.org> > --- > drivers/cpufreq/acpi-cpufreq.c | 56 ++++++++++++++++++++++++++++++++---------- > 1 file changed, 43 insertions(+), 13 deletions(-) > > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c > index 7f38fb55f223..d87962eda1ed 100644 > --- a/drivers/cpufreq/acpi-cpufreq.c > +++ b/drivers/cpufreq/acpi-cpufreq.c > @@ -66,6 +66,8 @@ enum { > > struct acpi_cpufreq_data { > struct cpufreq_frequency_table *freq_table; > + unsigned int cached_lookup_freq; > + struct cpufreq_frequency_table *cached_lookup_entry; This could have been an integer value 'Index', which could have been used together with the freq-table to get the freq we wanted. And, then we can move these two fields into the cpufreq_policy structure and make them part of the first patch itself. > unsigned int resume; > unsigned int cpu_feature; > unsigned int acpi_perf_cpu; > @@ -458,26 +460,53 @@ static int acpi_cpufreq_target(struct cpufreq_policy *policy, > return result; > } > > -unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy, > - unsigned int target_freq) > +/* > + * Find the closest frequency above target_freq. > + * > + * The table is sorted in the reverse order with respect to the > + * frequency and all of the entries are valid (see the initialization). > + */ > +static inline struct cpufreq_frequency_table > +*lookup_freq(struct cpufreq_frequency_table *table, unsigned int target_freq) > { > - struct acpi_cpufreq_data *data = policy->driver_data; > - struct acpi_processor_performance *perf; > - struct cpufreq_frequency_table *entry; > - unsigned int next_perf_state, next_freq, freq; > + struct cpufreq_frequency_table *entry = table; > + unsigned int freq; > > - /* > - * Find the closest frequency above target_freq. > - * > - * The table is sorted in the reverse order with respect to the > - * frequency and all of the entries are valid (see the initialization). > - */ > - entry = data->freq_table; > do { > entry++; > freq = entry->frequency; > } while (freq >= target_freq && freq != CPUFREQ_TABLE_END); > entry--; > + > + return entry; > +} > + > +unsigned int acpi_cpufreq_resolve_freq(struct cpufreq_policy *policy, > + unsigned int target_freq) > +{ > + struct acpi_cpufreq_data *data = policy->driver_data; > + struct cpufreq_frequency_table *entry; > + > + data->cached_lookup_freq = target_freq; > + entry = lookup_freq(data->freq_table, target_freq); > + data->cached_lookup_entry = entry; > + > + return entry->frequency; > +} > + And then we could remove this callback from acpi-cpufreq. > +unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy, > + unsigned int target_freq) > +{ > + struct acpi_cpufreq_data *data = policy->driver_data; > + struct acpi_processor_performance *perf; > + struct cpufreq_frequency_table *entry; > + unsigned int next_perf_state, next_freq; > + > + if (data->cached_lookup_entry && > + data->cached_lookup_freq == target_freq) > + entry = data->cached_lookup_entry; > + else > + entry = lookup_freq(data->freq_table, target_freq); And a static inline callback in cpufreq.h to get this entry. > next_freq = entry->frequency; > next_perf_state = entry->driver_data; > > @@ -918,6 +947,7 @@ static struct cpufreq_driver acpi_cpufreq_driver = { > .verify = cpufreq_generic_frequency_table_verify, > .target_index = acpi_cpufreq_target, > .fast_switch = acpi_cpufreq_fast_switch, > + .resolve_freq = acpi_cpufreq_resolve_freq, > .bios_limit = acpi_processor_get_bios_limit, > .init = acpi_cpufreq_cpu_init, > .exit = acpi_cpufreq_cpu_exit, Sounds reasonable ?
On Thu, May 26, 2016 at 12:13:41PM +0530, Viresh Kumar wrote: > On 25-05-16, 19:53, Steve Muckle wrote: > > Support the new resolve_freq cpufreq callback which resolves a target > > frequency to a driver-supported frequency without actually setting it. > > And here is the first abuser of this API as I was talking about in the > earlier patch :) > > But, I know why you are doing it and I think we can do it differently. > > So, lets assume that the ->resolve_freq() callback will ONLY be > provided by the drivers which also provide a ->target() callback. > > i.e. not by acpi-cpufreq at least. > > > The target frequency and resolved frequency table entry are cached so > > that a subsequent fast_switch operation may avoid the frequency table > > walk assuming the requested target frequency is the same. > > > > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Signed-off-by: Steve Muckle <smuckle@linaro.org> > > --- > > drivers/cpufreq/acpi-cpufreq.c | 56 ++++++++++++++++++++++++++++++++---------- > > 1 file changed, 43 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c > > index 7f38fb55f223..d87962eda1ed 100644 > > --- a/drivers/cpufreq/acpi-cpufreq.c > > +++ b/drivers/cpufreq/acpi-cpufreq.c > > @@ -66,6 +66,8 @@ enum { > > > > struct acpi_cpufreq_data { > > struct cpufreq_frequency_table *freq_table; > > + unsigned int cached_lookup_freq; > > + struct cpufreq_frequency_table *cached_lookup_entry; > > This could have been an integer value 'Index', which could have been > used together with the freq-table to get the freq we wanted. > > And, then we can move these two fields into the cpufreq_policy > structure and make them part of the first patch itself. > > > unsigned int resume; > > unsigned int cpu_feature; > > unsigned int acpi_perf_cpu; > > @@ -458,26 +460,53 @@ static int acpi_cpufreq_target(struct cpufreq_policy *policy, > > return result; > > } > > > > -unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy, > > - unsigned int target_freq) > > +/* > > + * Find the closest frequency above target_freq. > > + * > > + * The table is sorted in the reverse order with respect to the > > + * frequency and all of the entries are valid (see the initialization). > > + */ > > +static inline struct cpufreq_frequency_table > > +*lookup_freq(struct cpufreq_frequency_table *table, unsigned int target_freq) > > { > > - struct acpi_cpufreq_data *data = policy->driver_data; > > - struct acpi_processor_performance *perf; > > - struct cpufreq_frequency_table *entry; > > - unsigned int next_perf_state, next_freq, freq; > > + struct cpufreq_frequency_table *entry = table; > > + unsigned int freq; > > > > - /* > > - * Find the closest frequency above target_freq. > > - * > > - * The table is sorted in the reverse order with respect to the > > - * frequency and all of the entries are valid (see the initialization). > > - */ > > - entry = data->freq_table; > > do { > > entry++; > > freq = entry->frequency; > > } while (freq >= target_freq && freq != CPUFREQ_TABLE_END); > > entry--; > > + > > + return entry; > > +} > > + > > +unsigned int acpi_cpufreq_resolve_freq(struct cpufreq_policy *policy, > > + unsigned int target_freq) > > +{ > > + struct acpi_cpufreq_data *data = policy->driver_data; > > + struct cpufreq_frequency_table *entry; > > + > > + data->cached_lookup_freq = target_freq; > > + entry = lookup_freq(data->freq_table, target_freq); > > + data->cached_lookup_entry = entry; > > + > > + return entry->frequency; > > +} > > + > > And then we could remove this callback from acpi-cpufreq. > > > +unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy, > > + unsigned int target_freq) > > +{ > > + struct acpi_cpufreq_data *data = policy->driver_data; > > + struct acpi_processor_performance *perf; > > + struct cpufreq_frequency_table *entry; > > + unsigned int next_perf_state, next_freq; > > + > > + if (data->cached_lookup_entry && > > + data->cached_lookup_freq == target_freq) > > + entry = data->cached_lookup_entry; > > + else > > + entry = lookup_freq(data->freq_table, target_freq); > > And a static inline callback in cpufreq.h to get this entry. > > > next_freq = entry->frequency; > > next_perf_state = entry->driver_data; > > > > @@ -918,6 +947,7 @@ static struct cpufreq_driver acpi_cpufreq_driver = { > > .verify = cpufreq_generic_frequency_table_verify, > > .target_index = acpi_cpufreq_target, > > .fast_switch = acpi_cpufreq_fast_switch, > > + .resolve_freq = acpi_cpufreq_resolve_freq, > > .bios_limit = acpi_processor_get_bios_limit, > > .init = acpi_cpufreq_cpu_init, > > .exit = acpi_cpufreq_cpu_exit, > > Sounds reasonable ? A couple concerns... One is that if we do the lookup in cpufreq_driver_resolve_freq() for drivers which implement target_index() then it means using cpufreq_frequency_table_target() there. This is a heavier weight function that can't take advantage of driver-specific knowledge that the freq table is sorted a particular way. So for acpi-cpufreq we'd now be having to walk the whole table for every fast_switch. Another is that it'll be a a bit odd that the logic used to lookup the driver frequency will be different in the cached and uncached fast_switch cases. In the cached case it will have been determined by code in cpufreq_driver_resolve_freq() whereas in the uncached case it will be logic in the driver, in its fast_switch routine. I think at least the first issue would need to be solved to use this approach as it would impact performance for every fast_switch call. (Thanks for the review btw!) 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, 09:20, Steve Muckle wrote: > A couple concerns... One is that if we do the lookup in > cpufreq_driver_resolve_freq() for drivers which implement target_index() > then it means using cpufreq_frequency_table_target() there. This is a > heavier weight function that can't take advantage of driver-specific > knowledge that the freq table is sorted a particular way. I completely agree. > So for > acpi-cpufreq we'd now be having to walk the whole table for every > fast_switch. I have just tried to address that with following set: [PATCH 0/2] cpufreq: Use sorted frequency tables Lets see what Rafael has to say about that. > Another is that it'll be a a bit odd that the logic used to lookup the > driver frequency will be different in the cached and uncached > fast_switch cases. In the cached case it will have been determined by > code in cpufreq_driver_resolve_freq() whereas in the uncached case it > will be logic in the driver, in its fast_switch routine. We can make both of them refer the above code then. Lets see.
diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index 7f38fb55f223..d87962eda1ed 100644 --- a/drivers/cpufreq/acpi-cpufreq.c +++ b/drivers/cpufreq/acpi-cpufreq.c @@ -66,6 +66,8 @@ enum { struct acpi_cpufreq_data { struct cpufreq_frequency_table *freq_table; + unsigned int cached_lookup_freq; + struct cpufreq_frequency_table *cached_lookup_entry; unsigned int resume; unsigned int cpu_feature; unsigned int acpi_perf_cpu; @@ -458,26 +460,53 @@ static int acpi_cpufreq_target(struct cpufreq_policy *policy, return result; } -unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy, - unsigned int target_freq) +/* + * Find the closest frequency above target_freq. + * + * The table is sorted in the reverse order with respect to the + * frequency and all of the entries are valid (see the initialization). + */ +static inline struct cpufreq_frequency_table +*lookup_freq(struct cpufreq_frequency_table *table, unsigned int target_freq) { - struct acpi_cpufreq_data *data = policy->driver_data; - struct acpi_processor_performance *perf; - struct cpufreq_frequency_table *entry; - unsigned int next_perf_state, next_freq, freq; + struct cpufreq_frequency_table *entry = table; + unsigned int freq; - /* - * Find the closest frequency above target_freq. - * - * The table is sorted in the reverse order with respect to the - * frequency and all of the entries are valid (see the initialization). - */ - entry = data->freq_table; do { entry++; freq = entry->frequency; } while (freq >= target_freq && freq != CPUFREQ_TABLE_END); entry--; + + return entry; +} + +unsigned int acpi_cpufreq_resolve_freq(struct cpufreq_policy *policy, + unsigned int target_freq) +{ + struct acpi_cpufreq_data *data = policy->driver_data; + struct cpufreq_frequency_table *entry; + + data->cached_lookup_freq = target_freq; + entry = lookup_freq(data->freq_table, target_freq); + data->cached_lookup_entry = entry; + + return entry->frequency; +} + +unsigned int acpi_cpufreq_fast_switch(struct cpufreq_policy *policy, + unsigned int target_freq) +{ + struct acpi_cpufreq_data *data = policy->driver_data; + struct acpi_processor_performance *perf; + struct cpufreq_frequency_table *entry; + unsigned int next_perf_state, next_freq; + + if (data->cached_lookup_entry && + data->cached_lookup_freq == target_freq) + entry = data->cached_lookup_entry; + else + entry = lookup_freq(data->freq_table, target_freq); next_freq = entry->frequency; next_perf_state = entry->driver_data; @@ -918,6 +947,7 @@ static struct cpufreq_driver acpi_cpufreq_driver = { .verify = cpufreq_generic_frequency_table_verify, .target_index = acpi_cpufreq_target, .fast_switch = acpi_cpufreq_fast_switch, + .resolve_freq = acpi_cpufreq_resolve_freq, .bios_limit = acpi_processor_get_bios_limit, .init = acpi_cpufreq_cpu_init, .exit = acpi_cpufreq_cpu_exit,
Support the new resolve_freq cpufreq callback which resolves a target frequency to a driver-supported frequency without actually setting it. The target frequency and resolved frequency table entry are cached so that a subsequent fast_switch operation may avoid the frequency table walk assuming the requested target frequency is the same. Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Signed-off-by: Steve Muckle <smuckle@linaro.org> --- drivers/cpufreq/acpi-cpufreq.c | 56 ++++++++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 13 deletions(-)