Hi,
On Tuesday, August 02, 2011, Jean Pihet wrote:
...
> >> -static inline void pm_qos_set_value(struct pm_qos_object *o, s32 value)
> >> +static inline void pm_qos_set_value(struct pm_qos_constraints *c, s32 value)
> >> {
> >> - o->target_value = value;
> >> + c->target_value = value;
> >> }
> >
> > Well, I'm not sure that this function is necessary at all. You might as well
> > simply remove it as far as I'm concerned.
> The idea is to provide an efficient and lockless way to get the
> aggregated constraint class value. When the constraints a re changing
> the new value is calculated and stored using pm_qos_get_value and
> pm_qos_set_value. Then pm_qos_read_value is used to retrieve the
> value. For example cpuidle calls pm_qos_request which uses
> pm_qos_read_value to get the CPU/DMA minimum latency.
Still, pm_qos_set_value() is static in this file and doesn't really serve
any specific purpose except for symmetry with pm_qos_read_value().
Anyway, as I said I'm not sure, so it's OK to leave it as is to me too.
>
> >
> >> -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 *req,
> >> + enum pm_qos_req_action action, int value)
> >> {
> >> unsigned long flags;
> >> - int prev_value, curr_value;
> >> + int prev_value, curr_value, new_value;
> >> + struct pm_qos_object *o = pm_qos_array[req->class];
> >> + struct pm_qos_constraints *c;
> >> +
> >> + switch (req->class) {
> >> + case PM_QOS_DEV_LATENCY:
> >> + if (!req->dev) {
> >> + WARN(1, KERN_ERR "PM QoS API called with NULL dev\n");
> >> + return;
> >> + }
> >> + c = &req->dev->power.latency_constraints;
> >> + break;
> >> + case PM_QOS_CPU_DMA_LATENCY:
> >> + case PM_QOS_NETWORK_LATENCY:
> >> + case PM_QOS_NETWORK_THROUGHPUT:
> >> + c = o->constraints;
> >> + break;
> >> + case PM_QOS_RESERVED:
> >> + default:
> >> + WARN(1, KERN_ERR "PM QoS API called with wrong class %d, "
> >> + "req 0x%p\n", req->class, req);
> >> + return;
> >> + }
> >
> > Do we _really_ need that switch()?
> >
> > What about introducing dev_pm_qos_add_request() and friends specifically
> > for devices, such that they will take the target device object (dev) as
> > their first argument? Then, you could keep pm_qos_add_request() pretty
> > much as is, right?
> Yes but in that case I need to duplicate the API functions for devices
> (add, update, remove). Those functions will call update_target.
> I prefer the way this patch does it: simplify the API functions and
> have only 1 place with the constraints management and notification
> logic (in update_target).
The device functions would still call update_target(), but directly on the
device's struct struct pm_qos_constraints, so they wouldn't really duplicate
the existing pm_qos_*_request().
>
> >
> >>
> >> spin_lock_irqsave(&pm_qos_lock, flags);
> >> - prev_value = pm_qos_get_value(o);
> >> - /* PM_QOS_DEFAULT_VALUE is a signal that the value is unchanged */
> >> - if (value != PM_QOS_DEFAULT_VALUE) {
> >> +
> >> + prev_value = pm_qos_get_value(c);
> >> + if (value == PM_QOS_DEFAULT_VALUE)
> >> + new_value = c->default_value;
> >> + else
> >> + new_value = value;
> >
> > What about writing that as
> >
> > new_value = value != PM_QOS_DEFAULT_VALUE ? value : c->default_value;
> This is shorter but more difficult to read ;8
That depends on who you ask. :-)
> >
> >> +
> >> + switch (action) {
> >> + case PM_QOS_REMOVE_REQ:
> >> + plist_del(&req->node, &c->list);
> >> + break;
> >> + case PM_QOS_UPDATE_REQ:
> >> /*
> >> * to change the list, we atomically remove, reinit
> >> * with new value and add, then see if the extremal
> >> * changed
> >> */
> >> - plist_del(node, &o->requests);
> >> - plist_node_init(node, value);
> >> - plist_add(node, &o->requests);
> >> - } else if (del) {
> >> - plist_del(node, &o->requests);
> >> - } else {
> >> - plist_add(node, &o->requests);
> >> + plist_del(&req->node, &c->list);
> >> + case PM_QOS_ADD_REQ:
> >> + plist_node_init(&req->node, new_value);
> >> + plist_add(&req->node, &c->list);
> >> + break;
> >> + default:
> >> + /* no action */
> >> + ;
> >> }
> >> - curr_value = pm_qos_get_value(o);
> >> - pm_qos_set_value(o, curr_value);
> >> +
> >> + curr_value = pm_qos_get_value(c);
> >> + pm_qos_set_value(c, curr_value);
> >> spin_unlock_irqrestore(&pm_qos_lock, flags);
> >>
> >> if (prev_value != curr_value)
> >> blocking_notifier_call_chain(o->notifiers,
> >
> > That's why I'm thinking that it would be helpful to have a pointer
> > to the notifier list from struct pm_qos_constraints .
> I think having a per-device notifier list is complicating things. If a
> subsystem needs te be notified it needs to register a notifier
> callback for _every_ device in the system.
I'm not really sure. For example, why would a video subsystem want to be
notified of a PM QoS change in a keyboard?
> As a first implementation for OMAP the low-level PM code is using a
> single notifier to retrieve all the devices latency constraints and
> apply them to the pwer domains. I do not see how this could work with
> a per-device notifier list.
If you need a global notifier chain, it can be implemented as a separate
static variable as I said in my last reply in the [03/13] thread.
>
> >
> > Besides, you can use "pm_qos_array[req->class]->notifiers" instead of
> > "o->notifiers".
> Ok
>
> ...
> >> +static int find_pm_qos_object_by_minor(int minor)
> >> +{
> >> + int pm_qos_class;
> >> +
> >> + for (pm_qos_class = 0;
> >> + pm_qos_class < PM_QOS_NUM_CLASSES; pm_qos_class++) {
> >> + if (minor ==
> >> + pm_qos_array[pm_qos_class]->pm_qos_power_miscdev.minor)
> >> + return pm_qos_class;
> >> + }
> >> + return -1;
> >> +}
> >
> > This function doesn't seem to be used anywhere, what's the purpose of it?
> It is used by pm_qos_power_open in order to retrieve the class
> associated with the MISC device.
> BTW this patch is moving the code so that all the MISC related
> functions are grouped together.
OK, one more thing that needs to be done in a separate patch. :-)
Thanks,
Rafael
--
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
