Hello,
2011/4/26 Rafael J. Wysocki <rjw@xxxxxxx>:
> Hi,
>
> To start with let me say I don't have any fundamental objections at this point,
> although I'm not 100% sure I understand correctly how this feature is supposed
> to be used. ÂMy understanding is that if a device is known to have multiple
> OPPs, its subsystem and driver may call dvfs_add_device() for it, which will
> cause it to be periodically monitored and put into the "optimum" OPP using
> the infrastructure introduced by your patch (on the basis of the given
> usage profile and with the help of the given governor). ÂIs that correct?
Yes, that is correct.
>
> That said I have some specific comments below.
>
> On Tuesday, April 26, 2011, MyungJoo Ham wrote:
>> With OPPs, a device may have multiple operable frequency and voltage
>> sets. However, there can be multiple possible operable sets and a system
>> will need to choose one from them. In order to reduce the power
>> consumption (by reducing frequency and voltage) without affecting the
>> performance too much, a DVFS scheme may be used.
>>
>> This patch introduces the DVFS capability to non-CPU devices with OPPs.
>
> Well, I'd like the DVFS acronym to be expanded at least once somewhere,
> possibly in the comments describing the new files and necessarily in the
> Kconfig option description.
Sure, I will do so.
>
>> In this RTC version, we do not have specific gorvernors or device
>
> RFC I guess?
Uh oh.. yes, it should've been typed "RFC".
>
[]
>> +
>> +#define DVFS_INTERVAL Â Â Â Â20
>
> You should state in a comment what's the unit of this and explain the choice
> of the value.
Ok. I'll add a comment noting that it's in msec.
>
>> +/**
>> + * struct dvfs - Device DVFS structure
>> + * @node   list node - contains the devices with DVFS that have been
>> + * Â Â Â Â Â registered.
>> + * @dev       Âdevice pointer
>> + * @profile Âdevice-specific dvfs profile
>> + * @governor method how to choose frequency based on the usage.
>> + * @previous_freq  Âpreviously configured frequency value.
>> + * @next_polling   the number of remaining "dvfs_monitor" executions to
>> + * Â Â Â Â Â Â Â Â Â reevaluate frequency/voltage of the device. Set by
>> + * Â Â Â Â Â Â Â Â Â profile's polling_ms interval.
>> + * @tickle  positive if DVFS-tickling is activated for the device.
>> + * Â Â Â Â Â at each executino of dvfs_monitor, tickle is decremented.
>> + * Â Â Â Â Â User may tickle a device-dvfs in order to set maximum
>> + * Â Â Â Â Â frequency instaneously with some guaranteed duration.
>> + *
>> + * This structure stores the DVFS information for a give device.
>> + */
>> +struct dvfs {
>> + Â Â struct list_head node;
>> +
>> + Â Â struct device *dev;
>> + Â Â struct dvfs_device_profile *profile;
>> + Â Â struct dvfs_governor *governor;
>> +
>> + Â Â unsigned long previous_freq;
>> + Â Â unsigned int next_polling;
>> + Â Â unsigned int tickle;
>> +};
>> +
>
> I'd move the structure definition to the header file you introduce below.
> Even though it's supposed to be internal, it's better to have it defined
> next to the other structures for the benefit of the people who will
> try to understand your code in the future.
Ok. I'll do that.
>
[]
>> +static unsigned int dvfs_next_polling(int ms)
>> +{
>> + Â Â unsigned int ret;
>> +
>> + Â Â if (ms <= 0)
>> + Â Â Â Â Â Â return 0;
>> +
>> + Â Â ret = ms / DVFS_INTERVAL;
>> + Â Â if (ms % DVFS_INTERVAL)
>> + Â Â Â Â Â Â ret++;
>> +
>> + Â Â return ret;
>> +}
>
> It seems you could use DIV_ROUND_UP instead of this whole function.
Thanks.
>
>> +
>> +/**
>> + * dvfs_do() - Check the usage profile of a given device and configure
>> + * Â Â Â Â Â frequency and voltage accordingly
>> + * @dvfs: Â ÂDVFS info of the given device
>> + */
>> +static int dvfs_do(struct dvfs *dvfs)
>> +{
>> + Â Â struct dvfs_usage_profile profile;
>> + Â Â struct opp *opp;
>> + Â Â unsigned long freq;
>> + Â Â int err;
>> +
>> + Â Â err = dvfs->profile->get_usage_profile(dvfs->dev, &profile);
>> + Â Â if (err) {
>> + Â Â Â Â Â Â dev_err(dvfs->dev, "%s: Cannot get usage profile (%d)\n",
>> + Â Â Â Â Â Â Â Â Â Â __func__, err);
>> + Â Â Â Â Â Â return err;
>> + Â Â }
>> +
>> + Â Â err = dvfs->governor->get_target_freq(&profile, dvfs->governor, &freq);
>
> It may be better to define ->get_target_freq() so that it takes devfs as an arg.
> In that case you'd need to pass fewer pointers and the governor would be
> able to access all of its data anyway.
Oh Yes, I need to revise governor design so that the governor can take
more information about the device and dvfs options. I've been testing
with some test-case governors and it made some issues. :)
>
[]
>
> I'd generally consider using fewer error messages. ÂWhile they may be good
> for debugging, they have a potential of spamming the kernel's message buffer
> in case of a problem on a production system. ÂAlso they don't really make
> the code cleaner, so to speak. :-)
Ok, I'll keep that in mind in the next revision.
>
[]
>> + Â Â err = opp_init_cpufreq_table(dev, &table);
>
> I'd change the name opp_init_cpufreq_table() to something more suitable,
> since it's no longer used for the CPU frequencies alone. ÂSomething like
> opp_init_frequency_table() might work.
Got it. Anyway, I'm going to remove the dependency on
cpufreq_frequency_table or anything similar as well.
>
[]
>> +
>> + Â Â dvfs = find_device_dvfs(dev);
>> + Â Â if (!IS_ERR(dvfs)) {
>> + Â Â Â Â Â Â dev_err(dev, "%s: Unable to create DVFS for the device. "
>> + Â Â Â Â Â Â Â Â Â Â "It already has one.\n", __func__);
>> + Â Â Â Â Â Â mutex_unlock(&dvfs_list_lock);
>> + Â Â Â Â Â Â kfree(new_dvfs);
>> + Â Â Â Â Â Â return -EINVAL;
>> + Â Â }
>
> It seems that this should be checked before allocating the new object
> (you can use kzalloc() under the mutex). ÂI'd even run it before initializing
> the device's frequency table.
Fine. That seems more economic.
>
[]
>> +
>> +/**
>> + * dvfs_tickle_device() - Guarantee maximum operation speed for a while
>> + * Â Â Â Â Â Â Â Â Â instaneously.
>> + * @dev: Â Â the device to be tickled.
>> + * @duration_ms: Â Â the duration of tickle effect.
>> + *
>> + * Tickle sets the device at the maximum frequency instaneously and
>> + * the maximul frequency is guaranteed to be used for the given duration.
> Â Â Â Â Â^^^^^^^
> I guess that should be "maximum".
Yes. :)
>
>> + * For faster user reponse time, an input event may tickle a related device
>> + * so that the input event does not need to wait for the DVFS to react with
>> + * normal interval.
>> + */
>
> Can you explain how this function is supposed to be used, please?
Ok, I'll show you a usage example
The system: a touchscreen based computer with DVFS'able GPU.
Use case: a user scrolls the screen with fingers suddenly. (think of
an iPhone or Android device's home screen filled wit icons)
While there is no touchscreen input (thus, the screen does not move
fast), we do not use GPU too much; thus it is scaled to run at slower
rate (let's say 100MHz).
If a user suddenly touches screen and starts moving, the screen should
follow it; thus, requires much output from GPU.
With normal DVFS-GPU, it would take some time to speed it up because
it should monitor the GPU usage and react afterwards.
With the API given, "tickle" may be included in the user input event.
So that GPU can be scaled to maximum frequency immediately.
If the monitoring frequency is 50ms, DVFS will likely to react with
about 75ms of delay.
For example, if the touch event has occurred at 25ms after the last
DVFS monitoring event, at the next DVFS monitoring event, the measured
load will be about 50%, which usually won't require DVFS to speed up.
Then, DVFS will react at the next monitoring event; thus, it took 50ms
+ 25ms.
75ms is enough to give some impression to users. These days, many
mobile devices require 60Hz-based UI, which is about 16ms.
Anyway, this happens with drivers/cpufreq also. We have been testing
"tickling" associated with drivers/cpufreq/cpufreq.c. This has been
reduced user response time significantly removing the need for tuning
the threshold values.
>
>> +int dvfs_tickle_device(struct device *dev, unsigned long duration_ms)
>> +{
>> + Â Â struct dvfs *dvfs;
>> + Â Â struct opp *opp;
>> + Â Â unsigned long freq;
>> + Â Â int err = 0;
>> +
>> + Â Â mutex_lock(&dvfs_list_lock);
>> + Â Â dvfs = find_device_dvfs(dev);
>> + Â Â if (!IS_ERR(dvfs)) {
>> + Â Â Â Â Â Â freq = dvfs->profile->max_freq;
>> + Â Â Â Â Â Â opp = opp_find_freq_floor(dvfs->dev, &freq);
>> + Â Â Â Â Â Â freq = opp_get_freq(opp);
>> + Â Â Â Â Â Â if (dvfs->previous_freq != freq) {
>> + Â Â Â Â Â Â Â Â Â Â err = dvfs->profile->target(dvfs->dev, freq,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â opp_get_voltage(opp));
>> + Â Â Â Â Â Â Â Â Â Â dvfs->previous_freq = freq;
>> + Â Â Â Â Â Â }
>> + Â Â Â Â Â Â if (err)
>> + Â Â Â Â Â Â Â Â Â Â dev_err(dev, "%s: Cannot set frequency.\n", __func__);
>> + Â Â Â Â Â Â else
>> + Â Â Â Â Â Â Â Â Â Â dvfs->tickle = dvfs_next_polling(duration_ms);
>> + Â Â }
>> + Â Â mutex_unlock(&dvfs_list_lock);
>> +
>> + Â Â if (IS_ERR(dvfs)) {
>> + Â Â Â Â Â Â dev_err(dev, "%s: Cannot find dvfs.\n", __func__);
>> + Â Â Â Â Â Â err = PTR_ERR(dvfs);
>> + Â Â }
>> +
>> + Â Â return err;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int dvfs_pause(struct device *dev)
>> +{
>> + Â Â cancel_delayed_work_sync(&dvfs_work);
>> + Â Â return 0;
>> +}
>> +
>> +static void dvfs_continue(struct device *dev)
>> +{
>> + Â Â schedule_delayed_work(&dvfs_work, msecs_to_jiffies(DVFS_INTERVAL));
>> +}
>> +#else
>> +#define dvfs_pause  NULL
>> +#define dvfs_continue    ÂNULL
>> +#endif
>> +static const struct dev_pm_ops dvfs_pm = {
>> +   .prepare    Â= dvfs_pause,
>> +   .complete    = dvfs_continue,
>> +};
>> +static struct platform_driver dvfs_pm_drv = {
>> + Â Â .driver = {
>> +       .name  = "generic_dvfs_pm",
>> +       .pm   = &dvfs_pm,
>> + Â Â },
>> +};
>> +static struct platform_device dvfs_pm_dev = {
>> + Â Â .name = "generic_dvfs_pm",
>> +};
>
> Please don't do that. ÂYou can simply use the freezable workqueue introduced
> by commit 4149efb (workqueue: add system_freezeable_wq) and then you won't
> have to worry about system suspend.
Cool! Thanks.
>
>
> Thanks,
> Rafael
>
Thank you for your comments.
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
