Hello Rui,
On Mon, Jul 2, 2012 at 8:52 AM, Zhang Rui <rui.zhang@xxxxxxxxx> wrote:
> Hi, Eduardo,
>
> On 日, 2012-06-24 at 18:19 +0300, Valentin, Eduardo wrote:
>> Hello Rui,
>>
>> On Wed, Jun 20, 2012 at 9:47 AM, Zhang Rui <rui.zhang@xxxxxxxxx> wrote:
>> > On 三, 2012-06-13 at 17:23 +0300, Eduardo Valentin wrote:
>> >> Hello Rui,
>> >>
>> >> On Mon, Jun 11, 2012 at 11:20:39AM +0800, Zhang Rui wrote:
>> >> >
>> >> > Introduce simple arbitrator for setting device cooling state,
>> >> > to fix the problem that a cooling device may be referenced by
>> >> > by multiple trip points in multiple thermal zones.
>> >> >
>> >> >
>> >> > With this patch, we have two stages for updating a thermal zone,
>> >> > 1. check if a thermal_instance needs to be updated or not
>> >> > 2. update the cooling device, based on the target cooling state
>> >> > of all its instances.
>> >> >
>> >> > Note that, currently, the cooling device is set to the deepest
>> >> > cooling state required.
>> >> >
>> >> > Signed-off-by: Zhang Rui <rui.zhang@xxxxxxxxx>
>> >> > ---
>> >> > drivers/thermal/thermal_sys.c | 41 ++++++++++++++++++++++++++++++++++++++---
>> >> > include/linux/thermal.h | 1 +
>> >> > 2 files changed, 39 insertions(+), 3 deletions(-)
>> >> >
>> >> > Index: rtd3/drivers/thermal/thermal_sys.c
>> >> > ===================================================================
>> >> > --- rtd3.orig/drivers/thermal/thermal_sys.c
>> >> > +++ rtd3/drivers/thermal/thermal_sys.c
>> >> > @@ -54,6 +54,7 @@ struct thermal_instance {
>> >> > int trip;
>> >> > unsigned long upper; /* Highest cooling state for this trip point */
>> >> > unsigned long lower; /* Lowest cooling state for this trip point */
>> >> > + unsigned long target; /* expected cooling state */
>> >> > char attr_name[THERMAL_NAME_LENGTH];
>> >> > struct device_attribute attr;
>> >> > struct list_head tz_node; /* node in tz->instances */
>> >> > @@ -812,6 +813,7 @@ int thermal_zone_bind_cooling_device(str
>> >> > dev->trip = trip;
>> >> > dev->upper = upper;
>> >> > dev->lower = lower;
>> >> > + dev->target = -1;
>> >> >
>> >> > result = get_idr(&tz->idr, &tz->lock, &dev->id);
>> >> > if (result)
>> >> > @@ -949,6 +951,7 @@ thermal_cooling_device_register(char *ty
>> >> > strcpy(cdev->type, type);
>> >> > INIT_LIST_HEAD(&cdev->instances);
>> >> > cdev->ops = ops;
>> >> > + cdev->updated = 1;
>> >> > cdev->device.class = &thermal_class;
>> >> > cdev->devdata = devdata;
>> >> > dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
>> >> > @@ -1040,6 +1043,32 @@ void thermal_cooling_device_unregister(s
>> >> > }
>> >> > EXPORT_SYMBOL(thermal_cooling_device_unregister);
>> >> >
>> >> > +static void thermal_zone_do_update(struct thermal_zone_device *tz)
>> >> > +{
>> >> > + struct thermal_instance *instance1, *instance2;
>> >> > + struct thermal_cooling_device *cdev;
>> >> > + int target;
>> >> > +
>> >> > + list_for_each_entry(instance1, &tz->instances, tz_node) {
>> >> > + cdev = instance1->cdev;
>> >> > +
>> >> > + /* cooling device has already been updated*/
>> >> > + if (cdev->updated)
>> >> > + continue;
>> >> > +
>> >> > + target = 0;
>> >> > + /* Make sure cdev enters the deepest cooling state */
>> >> > + list_for_each_entry(instance2, &cdev->instances, cdev_node) {
>> >> > + if (instance2->target == -1)
>> >> > + continue;
>> >> > + if (instance2->target > target)
>> >> > + target = instance2->target;
>> >> > + }
>> >> > + cdev->ops->set_cur_state(cdev, target);
>> >> > + cdev->updated = 1;
>> >> > + }
>> >> > +}
>> >>
>> >> I think the above arbitrator solution does not escalate. As I can see,
>> >> the arbitrator takes care of cooling device instances in the same thermal zone.
>> >>
>> > why?
>> > It parses the thermal instances in one thermal zone to find the cooling
>> > devices, and then parse all the thermal instances for the cooling
>> > devices, no matter which thermal zone it belongs to.
>>
>> I think I got confused with the nomenclature change. So, yes, it does
>> go through all zones and cooling devices. But still, ideally it should
>> be a plist per device, don't you think? This way your search space is
>> reduced.
>>
> I'm not aware of this before, but here is a problem when I tried to use
> plist.
OK.
> First, we should use plist_node->prio as the thermal_instance->target
> instead, right?
Right.
> But the problem is that thermal_instance->target needs to be updated
> when temperature is changed. But plist does not support dynamic updating
> the priority, right?
No AFAIK,
> So when changing the cooling state target of a thermal instance, I need
> to delete and re-insert the plist_node? This does not seems efficient to
> me.
Right. That would mean you want to do remove the current zone
constraint and add the next entry zone constraint. Another way would
be to add current target if you are increasing target and remove if
you are reducing target.
Then if there is a device shared among existing zones, the highest
target set would be prevalent.
In fact it is not that efficient, but what do you think is less
efficient? Going through all zones or removing and re-adding 1
constraint ?
If you look kernel/power/qos.c you will see that it is a very similar situation.
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, &c->list);
case PM_QOS_ADD_REQ:
plist_node_init(node, new_value);
plist_add(node, &c->list);
break;
In fact, if you review that file you will understand why I was talking
about improving/reusing pm-qos to address this specific thermal
constraint management topic.
>
> thanks,
> rui
>
>> >
>> >> What if you have a device which expose cooling device instances in different
>> >> thermal zones?
>> >>
>> >> Besides, the thermal constraint might collide with pm QoS or with settings
>> >> coming from user space interfaces.
>> >>
>> >
>> >> That's actually why I was suggesting to have this 'arbitrator' or constraint
>> >> management outside the thermal framework. And managed per device at some
>> >> other abstraction layer.
>> >>
>> > Yeah, but I think we can have an incremental patch to handle the pm Qos
>> > constrains. Especially Durga already has the governor patches to move
>> > this out of the thermal framework.
>> > what do you think?
>>
>> I think it sounds like a plan. I also like incremental approach.
>>
>> >
>> > thanks,
>> > rui
>> >
>> >> > +
>> >> > /*
>> >> > * Cooling algorithm for active trip points
>> >> > *
>> >> > @@ -1086,19 +1115,24 @@ static void thermal_zone_trip_update(str
>> >> > cur_state = cur_state > instance->lower ?
>> >> > (cur_state - 1) : instance->lower;
>> >> > }
>> >> > - cdev->ops->set_cur_state(cdev, cur_state);
>> >> > + instance->target = cur_state;
>> >> > + cdev->updated = 0; /* cooling device needs update */
>> >> > }
>> >> > } else { /* below trip */
>> >> > list_for_each_entry(instance, &tz->instances, tz_node) {
>> >> > if (instance->trip != trip)
>> >> > continue;
>> >> >
>> >> > + /* Do not use the deacitve thermal instance */
>> >> > + if (instance->target == -1)
>> >> > + continue;
>> >> > cdev = instance->cdev;
>> >> > cdev->ops->get_cur_state(cdev, &cur_state);
>> >> >
>> >> > cur_state = cur_state > instance->lower ?
>> >> > - (cur_state - 1) : instance->lower;
>> >> > - cdev->ops->set_cur_state(cdev, cur_state);
>> >> > + (cur_state - 1) : -1;
>> >> > + instance->target = cur_state;
>> >> > + cdev->updated = 0; /* cooling device needs update */
>> >> > }
>> >> > }
>> >> >
>> >> > @@ -1159,6 +1193,7 @@ void thermal_zone_device_update(struct t
>> >> > }
>> >> > }
>> >> >
>> >> > + thermal_zone_do_update(tz);
>> >> > if (tz->forced_passive)
>> >> > thermal_zone_device_passive(tz, temp, tz->forced_passive,
>> >> > THERMAL_TRIPS_NONE);
>> >> > Index: rtd3/include/linux/thermal.h
>> >> > ===================================================================
>> >> > --- rtd3.orig/include/linux/thermal.h
>> >> > +++ rtd3/include/linux/thermal.h
>> >> > @@ -86,6 +86,7 @@ struct thermal_cooling_device {
>> >> > struct device device;
>> >> > void *devdata;
>> >> > const struct thermal_cooling_device_ops *ops;
>> >> > + int updated; /* 1 if the cooling device does not need update */
>> >> > struct list_head instances;
>> >> > struct list_head node;
>> >> > };
>> >> >
>> >> >
>> >
>> >
>>
>>
>>
>
>
--
Eduardo Valentin
