Re: [PATCH 03/13] PM: QoS: extend the in-kernel API with per-device latency constraints

["Rafael J. Wysocki" <rjw@sisk.pl>]

Hi,

On Tuesday, August 02, 2011, Jean Pihet wrote:
...
> I will keep pm_qos_class if the rename brings in some confusion. The
> intention was to simplify the names of the fields in structs with
> explicit names.

Please keep it for now.  You can rename it later if there's a clear
benefit, but I'm not sure still.

> >
> >> +     struct device *dev;
> >>  };
> >>
> >> -void pm_qos_add_request(struct pm_qos_request_list *l, int pm_qos_class, s32 value);
> >> -void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
> >> -             s32 new_value);
> >> -void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req);
> >> +struct pm_qos_parameters {
> >> +     int class;
> >> +     struct device *dev;
> >> +     s32 value;
> >> +};
> >
> > What exactly is the "dev" member needed for?
> This is the target device that is passed as parameter to the API. It
> is used for constraints of the devices latency class.

Well, I don't like this change.

In fact, I'd prefer it if the old interface were kept unmodified.

> ...
> >> +static BLOCKING_NOTIFIER_HEAD(dev_lat_notifier);
> >> +static struct pm_qos_object dev_pm_qos = {
> >> +     .requests = PLIST_HEAD_INIT(dev_pm_qos.requests, pm_qos_lock),
> >> +     .notifiers = &dev_lat_notifier,
> >> +     .name = "dev_latency",
> >> +     .target_value = PM_QOS_DEV_LAT_DEFAULT_VALUE,
> >> +     .default_value = PM_QOS_DEV_LAT_DEFAULT_VALUE,
> >> +     .type = PM_QOS_MIN,
> >> +};
> >> +
> >
> > You seem to be confusing things here.  Since devices will have their own lists
> > of requests now (as per the previous patch), the .requests member above is not
> > necessary.  Moreover, it seems to be used incorrectly below.
> The idea was to split the patches as requested previously: first the
> API changes (this very patch [03/13]) and then the actual
> implementation ([04/13]). Is this correct?

The idea is right in general, but so to speak it didn't work out too well.

The most important change is the introduction of struct pm_qos_constraints,
which doesn't need any preparation IMO.  I'd do this possibly early in the
series and I'd do it like this:

+struct pm_qos_constraints {
+       struct plist_head list;
+       s32 target_value;
+       s32 default_value;
+       struct blocking_notifier_head *notifiers;
+};

(more on the notifiers later).  Please note the lack of the type field.

At the same time I'd redefine struct pm_qos_object in the following way:

 struct pm_qos_object {
-       struct plist_head requests;
+       struct pm_qos_constraints *constraints;
-       struct blocking_notifier_head *notifiers;
        struct miscdevice pm_qos_power_miscdev;
        char *name;
-       s32 target_value;       /* Do not change to 64 bit */
-       s32 default_value;
        enum pm_qos_type type;
 };

so for the _existing_ PM QoS classes you can simply use
constraints->list instead of requests, constraints->target_value
instead of target_value, constraints->defaul_value instead of default_value
and constraints->notifiers instead of notifiers.

I wouldn't introduce a "global" PM QoS class for devices.

That should be a relatively simple patch that doesn't change the
existing interface and functionality.

The next patch would be to modify update_target() so that it takes
a pointer to struct pm_qos_constraints instead of the pointers to
struct pm_qos_object and struct plist_node (possibly also to take
an "operation" argument instead of the "del" one).  All it needs is
in struct pm_qos_constraints, so that should be simple too.
The modified update_target() should return a value indicating
whether or not prev_value was different from curr_value, so that
the device PM QoS functions (introduced by the next patch) can use it
to trigger system-wide notifications.

The next patch would introduce the per-device PM QoS by (1) adding
a struct pm_qos_constraints _pointer_ to struct dev_pm_info (assuming
that at least _some_ devices won't use PM QoS) and (2) adding
dev_pm_qos_add/update/remove_request() in drivers/base/power/qos.c,
all of them using the modified update_target().

Now if system-wide notifier chain for devices is necessary, you can
simply define it as a static variable in drivers/base/power/qos.c
without actually adding any "PM QoS object" for this purpose.  Now,
you can use the value returned by the modified update_target() to
decide whether or not to run notifiers from dev_pm_qos_*_request().

Does it make sense to you?

Rafael