On Fri, May 20, 2016 at 1:54 PM, Rafael J. Wysocki <raf...@kernel.org> wrote: > On Fri, May 20, 2016 at 1:39 PM, Rafael J. Wysocki <raf...@kernel.org> wrote: >> On Fri, May 20, 2016 at 2:46 AM, Rafael J. Wysocki <raf...@kernel.org> wrote: >>> On Fri, May 20, 2016 at 2:40 AM, Steve Muckle <steve.muc...@linaro.org> >>> wrote: >>>> On Fri, May 20, 2016 at 02:37:17AM +0200, Rafael J. Wysocki wrote: >>>>> Also I think that it would be good to avoid walking the frequency >>>>> table twice in case we end up wanting to update the frequency after >>>>> all. With the [4/5] we'd do it once in get_next_freq() and then once >>>>> more in cpufreq_driver_fast_switch(), for example, and walking the >>>>> frequency table may be more expensive that doing the switch in the >>>>> first place. >>>> >>>> If a driver API is added to return the platform frequency associated >>>> with a target frequency, what do you think about requiring the >>>> fast_switch API to take a target-supported frequency? >>> >>> That doesn't help much, because it generally would need to find a >>> table entry corresponding to it anyway, to find the actual command >>> value to write to a register, for example. >>> >>> But the driver could be smart and cache the value returned from the >>> new callback along with the command value associated with it. If >>> invoked with that particular frequency, it would use the cached >>> command. Otherwise, it would walk the table. >> >> It also makes sense to save both the "raw" value computed by >> get_next_freq() and the corresponding "driver" value, because if the >> current "raw" value is equal to the previous "raw" value, it shouldn't >> be necessary to walk the frequency table at all (as the previous >> "driver" value would then be equal to the current "driver" value too). >> >> So maybe the "driver" value should only be checked after the "raw" >> value check in sugov_update_commit() or equivalent? > > Moreover, you need to be careful about policy->min/max changes, > because both cpufreq_driver_fast_switch() and > __cpufreq_driver_target() clamp the target frequency between those and > if they change in the meantime, you may end up having to use a > different frequency at the driver level even if you get the same "raw" > value as last time. > > It looks like we don't do the right thing here in the current code even ...
Scratch that, sorry. We'll get the "limits" notification and the need_freq_update thing will cause next_freq to become zero then.