Hi Rafael, Mark,
- Looping in Mark for the user space API
2011/7/2 Rafael J. Wysocki <rjw@xxxxxxx>:
> Hi,
>
> On Thursday, June 30, 2011, jean.pihet@xxxxxxxxxxxxxx wrote:
>> From: Jean Pihet <j-pihet@xxxxxx>
>>
>> - add a new PM QoS class PM_QOS_DEV_WAKEUP_LATENCY for device wake-up
>> constraints. Due to the per-device nature of the new class the constraints
>> list is stored inside the device dev_pm_info struct instead of the internal
>> per-class constraints lists.
>
> I think PM_QOS_DEV_LATENCY might be a better name.
Ok, will update.
>
>> The new class is only available from kernel drivers and so is not exported
>> to user space.
>
> It should be available to user space, however, because in many cases drivers
> simply have no idea what values to use (after all, the use decides if he
> wants to trade worse video playback quality for better battery life, for
> example).
Ok. A per-device sysfs entry can be used as the user space interface.
Multiple users could access the same device constraints entry. Cf. the
existing implementation in kernel/pm_qos_params.c as an example.
Mark,
In a previous e-mail discussion you mentioned some concerns about the
user space API. Can you please give more details about it? Is the
proposed user space API suited?
>
>> The new class is used to put constraints on given devices in the system
>> while the existing PM_QOS_CPU_DMA_LATENCY class is used by cpuidle to
>> determine the next MPU subsystem state.
>>
>> - make the pm_qos_add_request API more generic by using a struct
>> pm_qos_parameters parameter
>>
>> - the notification mechanism now passes the constraint request struct ptr
>> in order for the notifier callback to have access to the full set of
>> constraint data, e.g. the struct device ptr
>>
>> - update the pm_qos_add_request callers to the generic API
>>
>> - minor clean-ups and rename of struct fields
>>
>> Signed-off-by: Jean Pihet <j-pihet@xxxxxx>
>
> Well, I think this patch attempts to do too many things at a time, which
> makes it somewhat hard to comprehend at first glance. I'd stronly suggest
> splitting it into a series of patches that first modify the existing API
> to make it suitable for the "real" changes and second introduce those
> "real" chages in the most straightforward way possible.
Ok will split it up in a patch for 'hollow' API changes, then the
implementation itself in a separate patch.
>
>> ---
>> arch/arm/plat-omap/i2c.c | 20 ----
>> drivers/i2c/busses/i2c-omap.c | 35 ++++---
>> drivers/media/video/via-camera.c | 5 +-
>> drivers/net/e1000e/netdev.c | 9 +-
>> drivers/net/wireless/ipw2x00/ipw2100.c | 6 +-
>> include/linux/pm_qos_params.h | 40 +++++---
>> kernel/pm_qos_params.c | 185 +++++++++++++++++++-------------
>
> Hmm. If you're at it, what about moving pm_qos_params.c to kernel/power
> and changing its name to pm_qos.c beforehand?
>
> The header might be called pm_qos.h too, BTW.
Ok, will update it.
>
>> sound/core/pcm_native.c | 8 +-
>> 8 files changed, 177 insertions(+), 131 deletions(-)
>>
> ...
>> diff --git a/include/linux/pm_qos_params.h b/include/linux/pm_qos_params.h
>> index a7d87f9..e6e16cb 100644
>> --- a/include/linux/pm_qos_params.h
>> +++ b/include/linux/pm_qos_params.h
>> @@ -8,31 +8,41 @@
>> #include <linux/notifier.h>
>> #include <linux/miscdevice.h>
>>
>> -#define PM_QOS_RESERVED 0
>> -#define PM_QOS_CPU_DMA_LATENCY 1
>> -#define PM_QOS_NETWORK_LATENCY 2
>> -#define PM_QOS_NETWORK_THROUGHPUT 3
>> +#define PM_QOS_RESERVED 0
>> +#define PM_QOS_CPU_DMA_LATENCY 1
>> +#define PM_QOS_DEV_WAKEUP_LATENCY 2
>> +#define PM_QOS_NETWORK_LATENCY 3
>> +#define PM_QOS_NETWORK_THROUGHPUT 4
>>
>> -#define PM_QOS_NUM_CLASSES 4
>> -#define PM_QOS_DEFAULT_VALUE -1
>> +#define PM_QOS_NUM_CLASSES 5
>> +#define PM_QOS_DEFAULT_VALUE -1
>>
>> -#define PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
>> -#define PM_QOS_NETWORK_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
>> -#define PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE 0
>> +#define PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
>> +#define PM_QOS_DEV_WAKEUP_LAT_DEFAULT_VALUE 0
>> +#define PM_QOS_NETWORK_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
>> +#define PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE 0
>>
>> struct pm_qos_request_list {
>> struct plist_node list;
>> - int pm_qos_class;
>> + int class;
>> + struct device *dev;
>> };
>>
>> -void pm_qos_add_request(struct pm_qos_request_list *l, int pm_qos_class, s32 value);
>> +struct pm_qos_parameters {
>> + int class;
>> + struct device *dev;
>> + s32 value;
>> +};
>> +
>> +void pm_qos_add_request(struct pm_qos_request_list *l,
>> + struct pm_qos_parameters *params);
>> void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
>> - s32 new_value);
>> + s32 new_value);
>> void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req);
>>
>> -int pm_qos_request(int pm_qos_class);
>> -int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier);
>> -int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier);
>> +int pm_qos_request(int class);
>> +int pm_qos_add_notifier(int class, struct notifier_block *notifier);
>> +int pm_qos_remove_notifier(int class, struct notifier_block *notifier);
>> int pm_qos_request_active(struct pm_qos_request_list *req);
>>
>> #endif
>> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
>> index 6824ca7..d61c8e5 100644
>> --- a/kernel/pm_qos_params.c
>> +++ b/kernel/pm_qos_params.c
>> @@ -60,7 +60,7 @@ enum pm_qos_type {
>> * types linux supports for 32 bit quantites
>> */
>> struct pm_qos_object {
>> - struct plist_head requests;
>> + struct plist_head *requests;
>> struct blocking_notifier_head *notifiers;
>> struct miscdevice pm_qos_power_miscdev;
>> char *name;
>
> So, I gather the idea is to have "requests" point to the device's private
> list of latency constraints. [BTW, there seems to be a naming confusion,
> because you tend to call those things "constraints" rather that "requests".]
Ok will change the naming.
> If so, doesn't that mean we'll need a per-device struct pm_qos_object too?
> In which case why not to make the device object point to that pm_qos_object
> object instead?
>
>> @@ -72,9 +72,12 @@ struct pm_qos_object {
>> static DEFINE_SPINLOCK(pm_qos_lock);
>>
>> static struct pm_qos_object null_pm_qos;
>> +
>> static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
>> +static struct plist_head _cpu_dma_reqs =
>> + PLIST_HEAD_INIT(_cpu_dma_reqs, pm_qos_lock);
>> static struct pm_qos_object cpu_dma_pm_qos = {
>> - .requests = PLIST_HEAD_INIT(cpu_dma_pm_qos.requests, pm_qos_lock),
>> + .requests = &_cpu_dma_reqs,
>> .notifiers = &cpu_dma_lat_notifier,
>> .name = "cpu_dma_latency",
>> .target_value = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE,
>> @@ -82,9 +85,20 @@ static struct pm_qos_object cpu_dma_pm_qos = {
>> .type = PM_QOS_MIN,
>> };
>>
>> +static BLOCKING_NOTIFIER_HEAD(dev_wakeup_lat_notifier);
>> +static struct pm_qos_object dev_wakeup_lat_pm_qos = {
>> + .notifiers = &dev_wakeup_lat_notifier,
>> + .name = "dev_wakeup_latency",
>> + .target_value = PM_QOS_DEV_WAKEUP_LAT_DEFAULT_VALUE,
>> + .default_value = PM_QOS_DEV_WAKEUP_LAT_DEFAULT_VALUE,
>> + .type = PM_QOS_MIN,
>> +};
>> +
>> static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
>> +static struct plist_head _network_lat_reqs =
>> + PLIST_HEAD_INIT(_network_lat_reqs, pm_qos_lock);
>> static struct pm_qos_object network_lat_pm_qos = {
>> - .requests = PLIST_HEAD_INIT(network_lat_pm_qos.requests, pm_qos_lock),
>> + .requests = &_network_lat_reqs,
>> .notifiers = &network_lat_notifier,
>> .name = "network_latency",
>> .target_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
>> @@ -94,8 +108,10 @@ static struct pm_qos_object network_lat_pm_qos = {
>>
>>
>> static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
>> +static struct plist_head _network_throughput_reqs =
>> + PLIST_HEAD_INIT(_network_throughput_reqs, pm_qos_lock);
>> static struct pm_qos_object network_throughput_pm_qos = {
>> - .requests = PLIST_HEAD_INIT(network_throughput_pm_qos.requests, pm_qos_lock),
>> + .requests = &_network_throughput_reqs,
>> .notifiers = &network_throughput_notifier,
>> .name = "network_throughput",
>> .target_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
>> @@ -107,6 +123,7 @@ static struct pm_qos_object network_throughput_pm_qos = {
>> static struct pm_qos_object *pm_qos_array[] = {
>> &null_pm_qos,
>> &cpu_dma_pm_qos,
>> + &dev_wakeup_lat_pm_qos,
>> &network_lat_pm_qos,
>> &network_throughput_pm_qos
>> };
>> @@ -129,19 +146,19 @@ static const struct file_operations pm_qos_power_fops = {
>> /* unlocked internal variant */
>> static inline int pm_qos_get_value(struct pm_qos_object *o)
>> {
>> - if (plist_head_empty(&o->requests))
>> + if (plist_head_empty(o->requests))
>> return o->default_value;
>>
>> switch (o->type) {
>> - case PM_QOS_MIN:
>> - return plist_first(&o->requests)->prio;
>> + case PM_QOS_MIN:
>> + return plist_first(o->requests)->prio;
>>
>> - case PM_QOS_MAX:
>> - return plist_last(&o->requests)->prio;
>> + case PM_QOS_MAX:
>> + return plist_last(o->requests)->prio;
>>
>> - default:
>> - /* runtime check for not using enum */
>> - BUG();
>> + default:
>> + /* runtime check for not using enum */
>> + BUG();
>> }
>> }
>>
>> @@ -155,13 +172,22 @@ static inline void pm_qos_set_value(struct pm_qos_object *o, s32 value)
>> o->target_value = value;
>> }
>>
>> -static void update_target(struct pm_qos_object *o, struct plist_node *node,
>> - int del, int value)
>> +static void update_target(struct pm_qos_request_list *req, int del, int value)
>> {
>> unsigned long flags;
>> int prev_value, curr_value;
>> + struct pm_qos_object *o = pm_qos_array[req->class];
>> + struct plist_node *node = &req->list;
>>
>> spin_lock_irqsave(&pm_qos_lock, flags);
>> + /*
>> + * PM_QOS_DEV_WAKEUP_LATENCY:
>> + * Use the per-device wake-up latency constraints list for
>> + * constraints storage
>> + */
>> + if (req->class == PM_QOS_DEV_WAKEUP_LATENCY)
>> + o->requests = &req->dev->power.wakeup_lat_plist_head;
>
> Ah, I see what the idea is and quite frankly I don't like it.
>
> Changing the requests field on the fly in every second (or so) invocation of
> update_target() for PM_QOS_DEV_WAKEUP_LATENCY class is beyond ugly. Moreover,
> it makes all devices share the default_value field which I don't think is
> correct (the target_value field is shared too, which may lead to all sorts of
> problems).
Indeed I had the same concern about the code not being generic enough.
>
> I wouldn't even try to use struct pm_qos_object as it is. Instead, I'd
> introduce something like
>
> struct pm_qos_constraits {
> struct plist_head requests;
> unsigned int current_val;
> unsigned int default_val;
> unsigned int floor_val; /* for adding new requests */
> unsigned int ceiling_val; /* ditto */
> enum pm_qos_type type;
> };
>
> Where I'm not sure if the "type" field is really necessary, but that should
> work out at one point and "unsigned int" is on purpose (it should have been
> that way from day one in the first place). That's what is needed for device
> IMO, the other fields in struct pm_qos_object are simply redundant for this
> use case.
>
> Now, for the compatibility with the existing code I'd redefine struct
> pm_qos_object as:
>
> struct pm_qos_object {
> struct pm_qos_constraints constraints;
> struct blocking_notifier *notifiers;
> struct miscdevice pm_qos_power_miscdev;
> char *name;
> };
>
> and I'd change the code to operate on struct pm_qos_constraits in the first
> place, with a compatibility layer using struct pm_qos_object on top of that.
>
> Then, each device may have its own struct pm_qos_constraits object that will
> be operated on by the PM QoS functions.
That makes sense, I am working on an RFC implementation.
> [There is the problem of whether or
> not those object should be embedded in struct dev_pm_info, it may be better
> to use pointers to them.
Should the object be embedded in the device struct instead? If only a
pointer is used when should the struct be allocated (at first use
...)?
> That may be used for sharing of constraints, for
> example, in which case it will be necessary to add a refcount to struct
> pm_qos_constraits.]
More on sharing in the comments for 0/11.
>
> Thanks,
> Rafael
>
Thanks,
Jean
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
