* [RFC PATCH 11/12] thermal: introduce cooling state arbitrator
@ 2012-06-11 3:20 Zhang Rui
2012-06-13 14:23 ` Eduardo Valentin
0 siblings, 1 reply; 6+ messages in thread
From: Zhang Rui @ 2012-06-11 3:20 UTC (permalink / raw)
To: linux-pm, linux-acpi
Cc: Amit Kachhap, eduardo, Rafael J. Wysocki, Len, Brown, R,
Durgadoss, Matthew Garrett
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@intel.com>
---
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;
+ }
+}
+
/*
* 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;
};
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 11/12] thermal: introduce cooling state arbitrator
2012-06-11 3:20 [RFC PATCH 11/12] thermal: introduce cooling state arbitrator Zhang Rui
@ 2012-06-13 14:23 ` Eduardo Valentin
2012-06-20 6:47 ` Zhang Rui
0 siblings, 1 reply; 6+ messages in thread
From: Eduardo Valentin @ 2012-06-13 14:23 UTC (permalink / raw)
To: Zhang Rui; +Cc: linux-acpi, linux-pm
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@intel.com>
> ---
> 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.
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.
> +
> /*
> * 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;
> };
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 11/12] thermal: introduce cooling state arbitrator
2012-06-13 14:23 ` Eduardo Valentin
@ 2012-06-20 6:47 ` Zhang Rui
2012-06-24 15:19 ` Valentin, Eduardo
0 siblings, 1 reply; 6+ messages in thread
From: Zhang Rui @ 2012-06-20 6:47 UTC (permalink / raw)
To: eduardo.valentin; +Cc: linux-acpi, linux-pm
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@intel.com>
> > ---
> > 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.
> 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?
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;
> > };
> >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 11/12] thermal: introduce cooling state arbitrator
2012-06-20 6:47 ` Zhang Rui
@ 2012-06-24 15:19 ` Valentin, Eduardo
2012-07-02 5:52 ` Zhang Rui
0 siblings, 1 reply; 6+ messages in thread
From: Valentin, Eduardo @ 2012-06-24 15:19 UTC (permalink / raw)
To: Zhang Rui; +Cc: linux-acpi, linux-pm
Hello Rui,
On Wed, Jun 20, 2012 at 9:47 AM, Zhang Rui <rui.zhang@intel.com> 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@intel.com>
>> > ---
>> > 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.
>
>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 11/12] thermal: introduce cooling state arbitrator
2012-06-24 15:19 ` Valentin, Eduardo
@ 2012-07-02 5:52 ` Zhang Rui
2012-07-02 7:22 ` Valentin, Eduardo
0 siblings, 1 reply; 6+ messages in thread
From: Zhang Rui @ 2012-07-02 5:52 UTC (permalink / raw)
To: Valentin, Eduardo; +Cc: linux-acpi, linux-pm
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@intel.com> 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@intel.com>
> >> > ---
> >> > 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.
First, we should use plist_node->prio as the thermal_instance->target
instead, 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?
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.
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;
> >> > };
> >> >
> >> >
> >
> >
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 11/12] thermal: introduce cooling state arbitrator
2012-07-02 5:52 ` Zhang Rui
@ 2012-07-02 7:22 ` Valentin, Eduardo
0 siblings, 0 replies; 6+ messages in thread
From: Valentin, Eduardo @ 2012-07-02 7:22 UTC (permalink / raw)
To: Zhang Rui
Cc: linux-pm, linux-acpi, Amit Kachhap, Rafael J. Wysocki, Len,
Brown, R, Durgadoss, Matthew Garrett
Hello Rui,
On Mon, Jul 2, 2012 at 8:52 AM, Zhang Rui <rui.zhang@intel.com> 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@intel.com> 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@intel.com>
>> >> > ---
>> >> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-07-02 7:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-06-11 3:20 [RFC PATCH 11/12] thermal: introduce cooling state arbitrator Zhang Rui
2012-06-13 14:23 ` Eduardo Valentin
2012-06-20 6:47 ` Zhang Rui
2012-06-24 15:19 ` Valentin, Eduardo
2012-07-02 5:52 ` Zhang Rui
2012-07-02 7:22 ` Valentin, Eduardo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.