Re: [PATCH 02/11] PM: extend PM QoS with per-device wake-up constraints

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux