On Tue, Aug 16, 2011 at 1:52 AM, MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx> wrote: > On Thu, Aug 11, 2011 at 10:35 AM, Turquette, Mike <mturquette@xxxxxx> wrote: > [] >> >> Polling is the only practical use for devfreq, assuming a QoS API >> exists for DVFS. As such powersave and performance governors should >> be removed. > > Although powersave/performance governors may seem useless, they are > used as basis on measuring the usefulness of DVFS mechanism of > specific devices. If a device is going to use DVFS, we can test the > device with them to find out the potential power save (compare > powersave to performance) and the performance deterioration (compared > to performance). Often, in testing phase, QA teams use performance to > find out any issues with DVFS features (in CPUFREQ). Users may simply > want to use performance governor in some cases (power is not an issue > sometimes). Fair enough. Keeping them around for testing is sensible. > Using QoS APIs simply to set "minimum" or "maximum" is possible. > However, they are not that straightforward; e.g., how should we set > "DMA latency" to be fixed at the minimum frequency regardless of > others, or how should we set "Network latency" to be fixed at the > maximum frequency? especially without knowing the specifications of > each DVFS-capable device (such as available frequencies, valid latency > values, ...). > >> >>> userspace: use the user specified frequency stored at >>> devfreq.user_set_freq. With sysfs support in the following patch, a user >>> may set the value with the sysfs interface. >>> >>> simple_ondemand: simplified version of CPUFREQ's ONDEMAND governor. >> >> I won't repeate everything from patch 1 of this series, but the >> governors should implement the queue/loop logic in the same way that >> CPUfreq does, and the individual devices should have their own >> delayed_work. > > First, in case where we want to let each DVFS-capable device have > exact polling frequency (up to jiffy resolution), we only need to set > polling_interval = jiffies_to_msecs(1);. That requires a source code change for anyone that wants to do it. My main complaint with this method is that it is restrictive to begin with and the whole method for determining the next_polling time reproduces what workqueues already give us. > In case of CPUFREQ, there would be only one polling loop at most for > each core. However, in case of DEVFREQ, there could be multiple > polling loops at a core if CPUFREQ-like looping logic is introduced. > Why don't we reduce that overhead while their function is same, it is > easily doable, and it reduces redundancy? I'm afraid I don't follow. I was thinking of having a single wq loop for each device. Under what conditions would a single device have multiple wq loops operating against it? >>> When a user updates OPP entries (enable/disable/add), OPP framework >>> automatically notifies DEVFREQ to update operating frequency >>> accordingly. Thus, DEVFREQ users (device drivers) do not need to update >> >> It would be nice if OPP library "notified" devfreq but it does not >> today. OPP library needs notifiers and devfreq can provide handlers >> for them. > > That's why devfreq_update() is added in the patch. While DEVFREQ is > the only one requiring notifications from OPP, do you think we may > incur the overhead of notifier at OPP by replacing devfreq_update with > notifier? If we somehow add another module that requires notifications > from OPP for frequency availability changes, we will need to implement > notifier at OPP side, but not just yet, I guess: (discussed before at > https://lists.linux-foundation.org/pipermail/linux-pm/2011-July/032053.html > ) Reading that thread makes me think that we really should implement notifiers in the OPP library. An obvious user of OPP notifiers would be CPUfreq. I think it is safe to say that there may be implementations of devfreq and CPUfreq that live side-by-side in the near future; OPPs might be enabled/disabled dynamically, which means both of them need callbacks. Better to abstract it out early, I think. >> > [] >>> --- >>> drivers/base/power/devfreq.c | 100 ++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/devfreq.h | 8 +++ >>> 2 files changed, 108 insertions(+), 0 deletions(-) >> >> Governors should be split out into their own file, especially since >> they need to grow to include polling/queueing logic. > > We will need to decide where to settle devfreq core, drivers, and > governors first. Would /drivers/devfreq/ be appropriate? I think GKH already ACK'd drivers/devfreq in a previous thread: https://lists.linux-foundation.org/pipermail/linux-pm/2011-August/032537.html > [] >>> + >>> + /* Set MAX if it's busy enough */ >>> + if (stat.busy_time * 100 > >>> + stat.total_time * DFSO_UPTHRESHOLD) { >> >> Thresholds should not be constants, but should be tuneable parameters, >> per-device. This is yet another reason for revising the existing >> relationship between devfreq core code, governors and devices. >> > > I agree. I think I should add governor specific "setup" value at > devfreq_add_device(); modifying the interface from > > extern int devfreq_add_device(struct device *dev, struct > devfreq_dev_profile *profile, struct devfreq_governor *governor); > ==> > extern int devfreq_add_device(struct device *dev, struct > devfreq_dev_profile *profile, struct devfreq_governor *governor, void > *gov_data); > > where gov_data is fed to struct devfreq's data field. It would be nice for the threshold values to be run-time tunable via sysfs. CPUfreq does this well today for ondemand/conservative governors and it really helps when doing power/performance tuning. Regards, Mike >>> + *freq = UINT_MAX; >>> + return 0; >>> + } >>> + >>> + /* Set MAX if we do not know the initial frequency */ >>> + if (stat.current_frequency == 0) { >>> + *freq = UINT_MAX; >>> + return 0; >>> + } >>> + >>> + /* Keep the current frequency */ >>> + if (stat.busy_time * 100 > >>> + stat.total_time * (DFSO_UPTHRESHOLD - DFSO_DOWNDIFFERENCTIAL)) { >> >> Same as above. > Yes. > >> >>> + *freq = stat.current_frequency; >>> + return 0; >>> + } >>> + >>> + /* Set the desired frequency based on the load */ >>> + a = stat.busy_time; >>> + a *= stat.current_frequency; >>> + b = div_u64(a, stat.total_time); >>> + b *= 100; >>> + b = div_u64(b, (DFSO_UPTHRESHOLD - DFSO_DOWNDIFFERENCTIAL / 2)); >> >> Same as above. > Yes. > >> >> Regards, >> Mike >> > > > Cheers! > MyungJoo > > -- > MyungJoo Ham (함명주), Ph.D. > Mobile Software Platform Lab, > Digital Media and Communications (DMC) Business > Samsung Electronics > cell: 82-10-6714-2858 > _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
