Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1200149 > unrolled thread
| Started by | Dmitry Torokhov <dmitry.torokhov@gmail.com> |
|---|---|
| First post | 2015-08-04 18:40 +0200 |
| Last post | 2015-08-17 19:40 +0200 |
| Articles | 20 on this page of 25 — 6 participants |
Back to article view | Back to linux.kernel
[PATCH] thermal: remove power allocator from list of default governors Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2015-08-04 18:40 +0200
Re: [PATCH] thermal: remove power allocator from list of default governors Javi Merino <javi.merino@arm.com> - 2015-08-05 10:40 +0200
Re: [PATCH] thermal: remove power allocator from list of default governors Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> - 2015-08-05 18:30 +0200
Re: [PATCH] thermal: remove power allocator from list of default governors Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2015-08-05 18:40 +0200
Re: [PATCH] thermal: remove power allocator from list of default governors Eduardo Valentin <edubezval@gmail.com> - 2015-08-05 21:00 +0200
Re: [PATCH] thermal: remove power allocator from list of default governors Javi Merino <javi.merino@arm.com> - 2015-08-06 10:40 +0200
[PATCH 3/3] thermal: power_allocator: exit early if there are no cooling devices Javi Merino <javi.merino@arm.com> - 2015-08-10 18:10 +0200
[PATCH 2/3] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino <javi.merino@arm.com> - 2015-08-10 18:10 +0200
[PATCH 0/3] Let the power allocator thermal governor run on any thermal zone Javi Merino <javi.merino@arm.com> - 2015-08-10 18:10 +0200
[PATCH 1/3] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino <javi.merino@arm.com> - 2015-08-10 18:10 +0200
[PATCH v2 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino <javi.merino@arm.com> - 2015-08-11 12:30 +0200
[PATCH v2 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino <javi.merino@arm.com> - 2015-08-11 12:30 +0200
Re: [PATCH v2 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Punit Agrawal <punit.agrawal@arm.com> - 2015-08-11 15:50 +0200
Re: [PATCH v2 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino <javi.merino@arm.com> - 2015-08-11 19:40 +0200
Re: [PATCH v2 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Daniel Kurtz <djkurtz@chromium.org> - 2015-08-12 13:10 +0200
[PATCH v2 3/4] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino <javi.merino@arm.com> - 2015-08-11 12:30 +0200
[PATCH v2 2/4] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino <javi.merino@arm.com> - 2015-08-11 12:30 +0200
Re: [PATCH v2 2/4] thermal: power_allocator: relax the requirement of two passive trip points Daniel Kurtz <djkurtz@chromium.org> - 2015-08-12 13:20 +0200
Re: [PATCH v2 2/4] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino <javi.merino@arm.com> - 2015-08-12 18:50 +0200
[PATCH v2 4/4] thermal: power_allocator: exit early if there are no cooling devices Javi Merino <javi.merino@arm.com> - 2015-08-11 12:30 +0200
[PATCH v3 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino <javi.merino@arm.com> - 2015-08-17 19:40 +0200
[PATCH v3 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino <javi.merino@arm.com> - 2015-08-17 19:40 +0200
[PATCH v3 4/4] thermal: power_allocator: exit early if there are no cooling devices Javi Merino <javi.merino@arm.com> - 2015-08-17 19:40 +0200
[PATCH v3 2/4] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino <javi.merino@arm.com> - 2015-08-17 19:40 +0200
[PATCH v3 3/4] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino <javi.merino@arm.com> - 2015-08-17 19:40 +0200
Page 1 of 2 [1] 2 Next page →
| From | Dmitry Torokhov <dmitry.torokhov@gmail.com> |
|---|---|
| Date | 2015-08-04 18:40 +0200 |
| Subject | [PATCH] thermal: remove power allocator from list of default governors |
| Message-ID | <pTNpV-7C4-23@gated-at.bofh.it> |
As it currently stands the power allocator governor can not handle thermal zones that are not specifically crafted and therefore can not be used as a default governor. Users need to explicitly enable this governor for thermal zones that do have enough information for its operation. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/thermal/Kconfig | 8 -------- 1 file changed, 8 deletions(-) diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index 0390044..34d05d3 100644 --- a/drivers/thermal/Kconfig +++ b/drivers/thermal/Kconfig @@ -82,14 +82,6 @@ config THERMAL_DEFAULT_GOV_USER_SPACE Select this if you want to let the user space manage the platform thermals. -config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR - bool "power_allocator" - select THERMAL_GOV_POWER_ALLOCATOR - help - Select this if you want to control temperature based on - system and device power allocation. This governor can only - operate on cooling devices that implement the power API. - endchoice config THERMAL_GOV_FAIR_SHARE -- 2.5.0.rc2.392.g76e840b -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Javi Merino <javi.merino@arm.com> |
|---|---|
| Date | 2015-08-05 10:40 +0200 |
| Message-ID | <pU2oV-4nL-11@gated-at.bofh.it> |
| In reply to | #1200149 |
On Tue, Aug 04, 2015 at 05:39:21PM +0100, Dmitry Torokhov wrote: > As it currently stands the power allocator governor can not handle > thermal zones that are not specifically crafted and therefore can not be > used as a default governor. > > Users need to explicitly enable this governor for thermal zones that do > have enough information for its operation. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/thermal/Kconfig | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > index 0390044..34d05d3 100644 > --- a/drivers/thermal/Kconfig > +++ b/drivers/thermal/Kconfig > @@ -82,14 +82,6 @@ config THERMAL_DEFAULT_GOV_USER_SPACE > Select this if you want to let the user space manage the > platform thermals. > > -config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR > - bool "power_allocator" > - select THERMAL_GOV_POWER_ALLOCATOR > - help > - Select this if you want to control temperature based on > - system and device power allocation. This governor can only > - operate on cooling devices that implement the power API. > - Currently the only way we have for a thermal zone configured from device tree to use a governor from the kernel boot is by using THERMAL_DEFAULT_GOV_*. If we remove this option some devices won't have a workable thermal framework until userspace is up and running. Would you rather have the power allocator governor accept every thermal zone? Cheers, Javi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> |
|---|---|
| Date | 2015-08-05 18:30 +0200 |
| Message-ID | <pU9JN-6FB-29@gated-at.bofh.it> |
| In reply to | #1200530 |
On Wed, 2015-08-05 at 09:37 +0100, Javi Merino wrote: > On Tue, Aug 04, 2015 at 05:39:21PM +0100, Dmitry Torokhov wrote: > > As it currently stands the power allocator governor can not handle > > thermal zones that are not specifically crafted and therefore can not be > > used as a default governor. > > > > Users need to explicitly enable this governor for thermal zones that do > > have enough information for its operation. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > drivers/thermal/Kconfig | 8 -------- > > 1 file changed, 8 deletions(-) > > > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > > index 0390044..34d05d3 100644 > > --- a/drivers/thermal/Kconfig > > +++ b/drivers/thermal/Kconfig > > @@ -82,14 +82,6 @@ config THERMAL_DEFAULT_GOV_USER_SPACE > > Select this if you want to let the user space manage the > > platform thermals. > > > > -config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR > > - bool "power_allocator" > > - select THERMAL_GOV_POWER_ALLOCATOR > > - help > > - Select this if you want to control temperature based on > > - system and device power allocation. This governor can only > > - operate on cooling devices that implement the power API. > > - > > Currently the only way we have for a thermal zone configured from > device tree to use a governor from the kernel boot is by using > THERMAL_DEFAULT_GOV_*. If we remove this option some devices won't > have a workable thermal framework until userspace is up and running. > > Would you rather have the power allocator governor accept every > thermal zone? May be allow to select when the binded cooling device can support this governor. Thanks, Srinivas > Cheers, > Javi > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Torokhov <dmitry.torokhov@gmail.com> |
|---|---|
| Date | 2015-08-05 18:40 +0200 |
| Message-ID | <pU9Ts-6QK-25@gated-at.bofh.it> |
| In reply to | #1200530 |
On Wed, Aug 05, 2015 at 09:37:55AM +0100, Javi Merino wrote: > On Tue, Aug 04, 2015 at 05:39:21PM +0100, Dmitry Torokhov wrote: > > As it currently stands the power allocator governor can not handle > > thermal zones that are not specifically crafted and therefore can not be > > used as a default governor. > > > > Users need to explicitly enable this governor for thermal zones that do > > have enough information for its operation. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > drivers/thermal/Kconfig | 8 -------- > > 1 file changed, 8 deletions(-) > > > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > > index 0390044..34d05d3 100644 > > --- a/drivers/thermal/Kconfig > > +++ b/drivers/thermal/Kconfig > > @@ -82,14 +82,6 @@ config THERMAL_DEFAULT_GOV_USER_SPACE > > Select this if you want to let the user space manage the > > platform thermals. > > > > -config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR > > - bool "power_allocator" > > - select THERMAL_GOV_POWER_ALLOCATOR > > - help > > - Select this if you want to control temperature based on > > - system and device power allocation. This governor can only > > - operate on cooling devices that implement the power API. > > - > > Currently the only way we have for a thermal zone configured from > device tree to use a governor from the kernel boot is by using > THERMAL_DEFAULT_GOV_*. If we remove this option some devices won't > have a workable thermal framework until userspace is up and running. Why would step wise, or fair share be not workable (even if not optimal) thermal frameworks? It doe snot take that long to get to [early] userspace. Half of the boot time the thermal framework is not working anyway because half of the devices that can act as colling devices are not yet logically there. > > Would you rather have the power allocator governor accept every > thermal zone? If it is to be one of default governors then yes, it needs to be able to manage all thermal zones, the same way as the other 3 governors can, as far as I know. Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Eduardo Valentin <edubezval@gmail.com> |
|---|---|
| Date | 2015-08-05 21:00 +0200 |
| Message-ID | <pUc4W-1s6-17@gated-at.bofh.it> |
| In reply to | #1200974 |
[Multipart message — attachments visible in raw view] — view raw
On Wed, Aug 05, 2015 at 09:35:39AM -0700, Dmitry Torokhov wrote: > On Wed, Aug 05, 2015 at 09:37:55AM +0100, Javi Merino wrote: > > On Tue, Aug 04, 2015 at 05:39:21PM +0100, Dmitry Torokhov wrote: > > > As it currently stands the power allocator governor can not handle > > > thermal zones that are not specifically crafted and therefore can not be > > > used as a default governor. > > > > > > Users need to explicitly enable this governor for thermal zones that do > > > have enough information for its operation. > > > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > --- > > > drivers/thermal/Kconfig | 8 -------- > > > 1 file changed, 8 deletions(-) > > > > > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > > > index 0390044..34d05d3 100644 > > > --- a/drivers/thermal/Kconfig > > > +++ b/drivers/thermal/Kconfig > > > @@ -82,14 +82,6 @@ config THERMAL_DEFAULT_GOV_USER_SPACE > > > Select this if you want to let the user space manage the > > > platform thermals. > > > > > > -config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR > > > - bool "power_allocator" > > > - select THERMAL_GOV_POWER_ALLOCATOR > > > - help > > > - Select this if you want to control temperature based on > > > - system and device power allocation. This governor can only > > > - operate on cooling devices that implement the power API. > > > - > > > > Currently the only way we have for a thermal zone configured from > > device tree to use a governor from the kernel boot is by using > > THERMAL_DEFAULT_GOV_*. If we remove this option some devices won't > > have a workable thermal framework until userspace is up and running. > > Why would step wise, or fair share be not workable (even if not optimal) > thermal frameworks? It doe snot take that long to get to [early] > userspace. Half of the boot time the thermal framework is not working > anyway because half of the devices that can act as colling devices are > not yet logically there. > > > > > Would you rather have the power allocator governor accept every > > thermal zone? > > If it is to be one of default governors then yes, it needs to be able to > manage all thermal zones, the same way as the other 3 governors can, as > far as I know. I believe fairshare also relies on platform data to be properly set, otherwise, it would not be functional. That's probably why I overlooked this point. That said, I would say, if we follow this logic, a similar patch is needed for fairshare. Javi, ideally, the governor would need to have a workable default settings for any thermal zone that miss the platform settings. Do you think it would be doable for power allocator? BR, > > Thanks. > > -- > Dmitry
[toc] | [prev] | [next] | [standalone]
| From | Javi Merino <javi.merino@arm.com> |
|---|---|
| Date | 2015-08-06 10:40 +0200 |
| Message-ID | <pUoSv-3wg-27@gated-at.bofh.it> |
| In reply to | #1201062 |
On Wed, Aug 05, 2015 at 07:49:41PM +0100, Eduardo Valentin wrote: > On Wed, Aug 05, 2015 at 09:35:39AM -0700, Dmitry Torokhov wrote: > > On Wed, Aug 05, 2015 at 09:37:55AM +0100, Javi Merino wrote: > > > On Tue, Aug 04, 2015 at 05:39:21PM +0100, Dmitry Torokhov wrote: > > > > As it currently stands the power allocator governor can not handle > > > > thermal zones that are not specifically crafted and therefore can not be > > > > used as a default governor. > > > > > > > > Users need to explicitly enable this governor for thermal zones that do > > > > have enough information for its operation. > > > > > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > > --- > > > > drivers/thermal/Kconfig | 8 -------- > > > > 1 file changed, 8 deletions(-) > > > > > > > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > > > > index 0390044..34d05d3 100644 > > > > --- a/drivers/thermal/Kconfig > > > > +++ b/drivers/thermal/Kconfig > > > > @@ -82,14 +82,6 @@ config THERMAL_DEFAULT_GOV_USER_SPACE > > > > Select this if you want to let the user space manage the > > > > platform thermals. > > > > > > > > -config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR > > > > - bool "power_allocator" > > > > - select THERMAL_GOV_POWER_ALLOCATOR > > > > - help > > > > - Select this if you want to control temperature based on > > > > - system and device power allocation. This governor can only > > > > - operate on cooling devices that implement the power API. > > > > - > > > > > > Currently the only way we have for a thermal zone configured from > > > device tree to use a governor from the kernel boot is by using > > > THERMAL_DEFAULT_GOV_*. If we remove this option some devices won't > > > have a workable thermal framework until userspace is up and running. > > > > Why would step wise, or fair share be not workable (even if not optimal) > > thermal frameworks? It doe snot take that long to get to [early] > > userspace. Half of the boot time the thermal framework is not working > > anyway because half of the devices that can act as colling devices are > > not yet logically there. > > > > > > > > Would you rather have the power allocator governor accept every > > > thermal zone? > > > > If it is to be one of default governors then yes, it needs to be able to > > manage all thermal zones, the same way as the other 3 governors can, as > > far as I know. > > I believe fairshare also relies on platform data to be properly set, > otherwise, it would not be functional. That's probably why I overlooked > this point. That said, I would say, if we follow this logic, a similar > patch is needed for fairshare. > > Javi, ideally, the governor would need to have a workable > default settings for any thermal zone that miss the platform settings. > Do you think it would be doable for power allocator? Yes, we can come up with something that accepts any thermal zone. I'll send a patch soon. Cheers, Javi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Javi Merino <javi.merino@arm.com> |
|---|---|
| Date | 2015-08-10 18:10 +0200 |
| Subject | [PATCH 3/3] thermal: power_allocator: exit early if there are no cooling devices |
| Message-ID | <pVXO9-2OQ-3@gated-at.bofh.it> |
| In reply to | #1201574 |
Don't waste cycles in the power allocator governor's throttle function
if there are no cooling devices and exit early.
This commit doesn't change any functionality, but should provide better
performance for the odd case of a thermal zone with trip points but
without cooling devices.
---
drivers/thermal/power_allocator.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
index cb3183ec364f..58128ce11ec9 100644
--- a/drivers/thermal/power_allocator.c
+++ b/drivers/thermal/power_allocator.c
@@ -287,6 +287,11 @@ static int allocate_power(struct thermal_zone_device *tz,
}
}
+ if (!num_actors) {
+ ret = -ENODEV;
+ goto unlock;
+ }
+
/*
* We need to allocate three arrays of the same size:
* req_power, max_power and granted_power. They are going to
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Javi Merino <javi.merino@arm.com> |
|---|---|
| Date | 2015-08-10 18:10 +0200 |
| Subject | [PATCH 2/3] thermal: power_allocator: relax the requirement of two passive trip points |
| Message-ID | <pVXO9-2OQ-7@gated-at.bofh.it> |
| In reply to | #1201574 |
The power allocator governor currently requires that the thermal zone
has at least two passive trip points. If there aren't, the governor
refuses to bind to the thermal zone.
This commit relaxes that requirement. Now the governor will bind to all
thermal zones regardless of how many trip points they have.
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
Documentation/thermal/power_allocator.txt | 2 +-
drivers/thermal/power_allocator.c | 91 +++++++++++++++++--------------
2 files changed, 52 insertions(+), 41 deletions(-)
diff --git a/Documentation/thermal/power_allocator.txt b/Documentation/thermal/power_allocator.txt
index c3797b529991..a1ce2235f121 100644
--- a/Documentation/thermal/power_allocator.txt
+++ b/Documentation/thermal/power_allocator.txt
@@ -4,7 +4,7 @@ Power allocator governor tunables
Trip points
-----------
-The governor requires the following two passive trip points:
+The governor works optimally with the following two passive trip points:
1. "switch on" trip point: temperature above which the governor
control loop starts operating. This is the first passive trip
diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
index cdcf5c6acc3c..cb3183ec364f 100644
--- a/drivers/thermal/power_allocator.c
+++ b/drivers/thermal/power_allocator.c
@@ -24,6 +24,8 @@
#include "thermal_core.h"
+#define INVALID_TRIP -1
+
#define FRAC_BITS 10
#define int_to_frac(x) ((x) << FRAC_BITS)
#define frac_to_int(x) ((x) >> FRAC_BITS)
@@ -61,6 +63,8 @@ static inline s64 div_frac(s64 x, s64 y)
* Used to calculate the derivative term.
* @trip_switch_on: first passive trip point of the thermal zone. The
* governor switches on when this trip point is crossed.
+ * If the thermal zone only has one passive trip point,
+ * @trip_switch_on should be INVALID_TRIP.
* @trip_max_desired_temperature: last passive trip point of the thermal
* zone. The temperature we are
* controlling for.
@@ -372,43 +376,66 @@ unlock:
return ret;
}
-static int get_governor_trips(struct thermal_zone_device *tz,
- struct power_allocator_params *params)
+/**
+ * get_governor_trips() - get the number of the two trip points that are key for this governor
+ * @tz: thermal zone to operate on
+ * @params: pointer the private data for this governor
+ *
+ * The power allocator governor works optimally with two trips points:
+ * a "switch on" trip point and a "maximum desired temperature". These
+ * are defined as the first and last passive trip points.
+ *
+ * If there is only one trip point, then that's considered to be the
+ * "maximum desired temperature" trip point and the governor is always
+ * on. If there are no passive or active trip points, then the
+ * governor won't do anything. In fact, its throttle function
+ * shouldn't be called at all.
+ */
+static void get_governor_trips(struct thermal_zone_device *tz,
+ struct power_allocator_params *params)
{
- int i, ret, last_passive;
+ int i, last_active, last_passive;
bool found_first_passive;
found_first_passive = false;
- last_passive = -1;
- ret = -EINVAL;
+ last_active = INVALID_TRIP;
+ last_passive = INVALID_TRIP;
for (i = 0; i < tz->trips; i++) {
enum thermal_trip_type type;
+ int ret;
ret = tz->ops->get_trip_type(tz, i, &type);
- if (ret)
- return ret;
+ if (ret) {
+ dev_warn(&tz->device,
+ "Failed to get trip point %d type: %d\n", i,
+ ret);
+ continue;
+ }
- if (!found_first_passive) {
- if (type == THERMAL_TRIP_PASSIVE) {
+ if (type == THERMAL_TRIP_PASSIVE) {
+ if (!found_first_passive) {
params->trip_switch_on = i;
found_first_passive = true;
+ } else {
+ last_passive = i;
}
- } else if (type == THERMAL_TRIP_PASSIVE) {
- last_passive = i;
+ } else if (type == THERMAL_TRIP_ACTIVE) {
+ last_active = i;
} else {
break;
}
}
- if (last_passive != -1) {
+ if (last_passive != INVALID_TRIP) {
params->trip_max_desired_temperature = last_passive;
- ret = 0;
+ } else if (found_first_passive) {
+ params->trip_max_desired_temperature = params->trip_switch_on;
+ params->trip_switch_on = INVALID_TRIP;
} else {
- ret = -EINVAL;
+ params->trip_switch_on = INVALID_TRIP;
+ params->trip_max_desired_temperature = last_active;
}
-
- return ret;
}
static void reset_pid_controller(struct power_allocator_params *params)
@@ -437,11 +464,10 @@ static void allow_maximum_power(struct thermal_zone_device *tz)
* power_allocator_bind() - bind the power_allocator governor to a thermal zone
* @tz: thermal zone to bind it to
*
- * Check that the thermal zone is valid for this governor, that is, it
- * has two thermal trips. If so, initialize the PID controller
- * parameters and bind it to the thermal zone.
+ * Initialize the PID controller parameters and bind it to the thermal
+ * zone.
*
- * Return: 0 on success, -EINVAL if the trips were invalid or -ENOMEM
+ * Return: 0 on success, -EINVAL if the thermal zone doesn't have tzp or -ENOMEM
* if we ran out of memory.
*/
static int power_allocator_bind(struct thermal_zone_device *tz)
@@ -461,23 +487,18 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
if (!tz->tzp->sustainable_power)
dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n");
- ret = get_governor_trips(tz, params);
- if (ret) {
- dev_err(&tz->device,
- "thermal zone %s has wrong trip setup for power allocator\n",
- tz->type);
- goto free;
- }
+ get_governor_trips(tz, params);
ret = tz->ops->get_trip_temp(tz, params->trip_switch_on,
&switch_on_temp);
if (ret)
- goto free;
+ switch_on_temp = 0;
ret = tz->ops->get_trip_temp(tz, params->trip_max_desired_temperature,
&control_temp);
if (ret)
- goto free;
+ /* Set some valid value to avoid division by zero below */
+ control_temp = 70000;
/*
* Provide an arbitrary sustainable_power to set the default
@@ -504,10 +525,6 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
tz->governor_data = params;
return 0;
-
-free:
- devm_kfree(&tz->device, params);
- return ret;
}
static void power_allocator_unbind(struct thermal_zone_device *tz)
@@ -538,13 +555,7 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip)
ret = tz->ops->get_trip_temp(tz, params->trip_switch_on,
&switch_on_temp);
- if (ret) {
- dev_warn(&tz->device,
- "Failed to get switch on temperature: %d\n", ret);
- return ret;
- }
-
- if (current_temp < switch_on_temp) {
+ if ((!ret) && (current_temp < switch_on_temp)) {
tz->passive = 0;
reset_pid_controller(params);
allow_maximum_power(tz);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Javi Merino <javi.merino@arm.com> |
|---|---|
| Date | 2015-08-10 18:10 +0200 |
| Subject | [PATCH 0/3] Let the power allocator thermal governor run on any thermal zone |
| Message-ID | <pVXO9-2OQ-5@gated-at.bofh.it> |
| In reply to | #1201574 |
Relax the thermal governor requirements of sustainable_power and at
least two trip points so that it can be bound to any thermal zone.
Its behavior won't be optimal, it would be the best it can with the
data provided.
Javi Merino (3):
thermal: power_allocator: relax the requirement of a sustainable_power
in tzp
thermal: power_allocator: relax the requirement of two passive trip
points
thermal: power_allocator: exit early if there are no cooling devices
Documentation/thermal/power_allocator.txt | 2 +-
drivers/thermal/power_allocator.c | 158 +++++++++++++++++++++---------
drivers/thermal/thermal_core.c | 28 ++++++
include/linux/thermal.h | 6 ++
4 files changed, 144 insertions(+), 50 deletions(-)
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Javi Merino <javi.merino@arm.com> |
|---|---|
| Date | 2015-08-10 18:10 +0200 |
| Subject | [PATCH 1/3] thermal: power_allocator: relax the requirement of a sustainable_power in tzp |
| Message-ID | <pVXOa-2OQ-27@gated-at.bofh.it> |
| In reply to | #1204398 |
The power allocator governor currently requires that a sustainable power
is passed as part of the thermal zone's thermal zone parameters. If
that parameter is not provided, it doesn't register with the thermal
zone.
While this parameter is strongly recommended for optimal performance, it
doesn't need to be mandatory. Relax the requirement and allow the
governor to bind to thermal zones that don't provide it by estimating it
from the cooling devices' power model.
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
drivers/thermal/power_allocator.c | 62 +++++++++++++++++++++++++++++++++------
drivers/thermal/thermal_core.c | 28 ++++++++++++++++++
include/linux/thermal.h | 6 ++++
3 files changed, 87 insertions(+), 9 deletions(-)
diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
index 4672250b329f..cdcf5c6acc3c 100644
--- a/drivers/thermal/power_allocator.c
+++ b/drivers/thermal/power_allocator.c
@@ -73,6 +73,39 @@ struct power_allocator_params {
};
/**
+ * estimate_sustainable_power() - Estimate the sustainable power of a thermal zone
+ * @tz: thermal zone we are operating in
+ *
+ * For thermal zones that don't provide a sustainable_power in their
+ * thermal_zone_params, estimate one. Calculate it using the minimum
+ * power of all the cooling devices as that gives a valid value that
+ * can give some degree of functionality. For optimal performance of
+ * this governor, provide a sustainable_power in the thermal zone's
+ * thermal_zone_params.
+ */
+static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
+{
+ u32 sustainable_power = 0;
+ struct thermal_instance *instance;
+ struct power_allocator_params *params = tz->governor_data;
+
+ list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+ struct thermal_cooling_device *cdev = instance->cdev;
+ u32 min_power;
+
+ if (instance->trip != params->trip_max_desired_temperature)
+ continue;
+
+ if (power_actor_get_min_power(cdev, tz, &min_power))
+ continue;
+
+ sustainable_power += min_power;
+ }
+
+ return sustainable_power;
+}
+
+/**
* pid_controller() - PID controller
* @tz: thermal zone we are operating in
* @current_temp: the current temperature in millicelsius
@@ -98,6 +131,7 @@ static u32 pid_controller(struct thermal_zone_device *tz,
{
s64 p, i, d, power_range;
s32 err, max_power_frac;
+ u32 sustainable_power;
struct power_allocator_params *params = tz->governor_data;
max_power_frac = int_to_frac(max_allocatable_power);
@@ -138,8 +172,11 @@ static u32 pid_controller(struct thermal_zone_device *tz,
power_range = p + i + d;
+ sustainable_power = tz->tzp->sustainable_power ?:
+ estimate_sustainable_power(tz);
+
/* feed-forward the known sustainable dissipatable power */
- power_range = tz->tzp->sustainable_power + frac_to_int(power_range);
+ power_range = sustainable_power + frac_to_int(power_range);
power_range = clamp(power_range, (s64)0, (s64)max_allocatable_power);
@@ -412,18 +449,18 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
int ret;
struct power_allocator_params *params;
unsigned long switch_on_temp, control_temp;
- u32 temperature_threshold;
+ u32 sustainable_power, temperature_threshold;
- if (!tz->tzp || !tz->tzp->sustainable_power) {
- dev_err(&tz->device,
- "power_allocator: missing sustainable_power\n");
+ if (!tz->tzp)
return -EINVAL;
- }
params = devm_kzalloc(&tz->device, sizeof(*params), GFP_KERNEL);
if (!params)
return -ENOMEM;
+ if (!tz->tzp->sustainable_power)
+ dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n");
+
ret = get_governor_trips(tz, params);
if (ret) {
dev_err(&tz->device,
@@ -442,13 +479,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
if (ret)
goto free;
+ /*
+ * Provide an arbitrary sustainable_power to set the default
+ * values of k_po and k_pu. We can estimate sustainable_power
+ * at this point because no cooling devices have been
+ * registered yet. By providing an arbitrary value we get
+ * better defaults that setting k_po and k_pu to 0.
+ */
+ sustainable_power = tz->tzp->sustainable_power ?: 2500;
temperature_threshold = control_temp - switch_on_temp;
tz->tzp->k_po = tz->tzp->k_po ?:
- int_to_frac(tz->tzp->sustainable_power) / temperature_threshold;
+ int_to_frac(sustainable_power) / temperature_threshold;
tz->tzp->k_pu = tz->tzp->k_pu ?:
- int_to_frac(2 * tz->tzp->sustainable_power) /
- temperature_threshold;
+ int_to_frac(2 * sustainable_power) / temperature_threshold;
tz->tzp->k_i = tz->tzp->k_i ?: int_to_frac(10) / 1000;
/*
* The default for k_d and integral_cutoff is 0, so we can
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 04659bfb888b..4040dd95bb19 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -997,6 +997,34 @@ int power_actor_get_max_power(struct thermal_cooling_device *cdev,
}
/**
+ * power_actor_get_min_power() - get the mainimum power that a cdev can consume
+ * @cdev: pointer to &thermal_cooling_device
+ * @tz: a valid thermal zone device pointer
+ * @min_power: pointer in which to store the minimum power
+ *
+ * Calculate the minimum power consumption in milliwats that the
+ * cooling device can currently consume and store it in @min_power.
+ *
+ * Return: 0 on success, -EINVAL if @cdev doesn't support the
+ * power_actor API or -E* on other error.
+ */
+int power_actor_get_min_power(struct thermal_cooling_device *cdev,
+ struct thermal_zone_device *tz, u32 *min_power)
+{
+ unsigned long max_state;
+ int ret;
+
+ if (!cdev_is_power_actor(cdev))
+ return -EINVAL;
+
+ ret = cdev->ops->get_max_state(cdev, &max_state);
+ if (ret)
+ return ret;
+
+ return cdev->ops->state2power(cdev, tz, max_state, min_power);
+}
+
+/**
* power_actor_set_power() - limit the maximum power that a cooling device can consume
* @cdev: pointer to &thermal_cooling_device
* @instance: thermal instance to update
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 037e9df2f610..f99d934d373a 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -384,6 +384,8 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev)
int power_actor_get_max_power(struct thermal_cooling_device *,
struct thermal_zone_device *tz, u32 *max_power);
+int power_actor_get_min_power(struct thermal_cooling_device *,
+ struct thermal_zone_device *tz, u32 *min_power);
int power_actor_set_power(struct thermal_cooling_device *,
struct thermal_instance *, u32);
struct thermal_zone_device *thermal_zone_device_register(const char *, int, int,
@@ -419,6 +421,10 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev)
static inline int power_actor_get_max_power(struct thermal_cooling_device *cdev,
struct thermal_zone_device *tz, u32 *max_power)
{ return 0; }
+static inline int power_actor_get_min_power(struct thermal_cooling_device *cdev,
+ struct thermal_zone_device *tz,
+ u32 *min_power)
+{ return -ENODEV; }
static inline int power_actor_set_power(struct thermal_cooling_device *cdev,
struct thermal_instance *tz, u32 power)
{ return 0; }
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Javi Merino <javi.merino@arm.com> |
|---|---|
| Date | 2015-08-11 12:30 +0200 |
| Subject | [PATCH v2 0/4] Let the power allocator thermal governor run on any thermal zone |
| Message-ID | <pWeYF-2jf-1@gated-at.bofh.it> |
| In reply to | #1204398 |
Relax the thermal governor requirements of sustainable_power and at
least two trip points so that it can be bound to any thermal zone.
Its behavior won't be optimal, it would be the best it can with the
data provided.
Changes since v1:
- Let the power allocator governor operate if the thermal zone
doesn't have tzp as suggested by Chung-yih Wang
Javi Merino (4):
thermal: power_allocator: relax the requirement of a sustainable_power
in tzp
thermal: power_allocator: relax the requirement of two passive trip
points
thermal: power_allocator: don't require tzp to be present for the
thermal zone
thermal: power_allocator: exit early if there are no cooling devices
Documentation/thermal/power_allocator.txt | 2 +-
drivers/thermal/power_allocator.c | 178 ++++++++++++++++++++++--------
drivers/thermal/thermal_core.c | 28 +++++
include/linux/thermal.h | 6 +
4 files changed, 165 insertions(+), 49 deletions(-)
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Javi Merino <javi.merino@arm.com> |
|---|---|
| Date | 2015-08-11 12:30 +0200 |
| Subject | [PATCH v2 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp |
| Message-ID | <pWeYF-2jf-3@gated-at.bofh.it> |
| In reply to | #1205020 |
The power allocator governor currently requires that a sustainable power
is passed as part of the thermal zone's thermal zone parameters. If
that parameter is not provided, it doesn't register with the thermal
zone.
While this parameter is strongly recommended for optimal performance, it
doesn't need to be mandatory. Relax the requirement and allow the
governor to bind to thermal zones that don't provide it by estimating it
from the cooling devices' power model.
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
drivers/thermal/power_allocator.c | 62 +++++++++++++++++++++++++++++++++------
drivers/thermal/thermal_core.c | 28 ++++++++++++++++++
include/linux/thermal.h | 6 ++++
3 files changed, 87 insertions(+), 9 deletions(-)
diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
index 63a448f9d93b..f78836c2da26 100644
--- a/drivers/thermal/power_allocator.c
+++ b/drivers/thermal/power_allocator.c
@@ -73,6 +73,39 @@ struct power_allocator_params {
};
/**
+ * estimate_sustainable_power() - Estimate the sustainable power of a thermal zone
+ * @tz: thermal zone we are operating in
+ *
+ * For thermal zones that don't provide a sustainable_power in their
+ * thermal_zone_params, estimate one. Calculate it using the minimum
+ * power of all the cooling devices as that gives a valid value that
+ * can give some degree of functionality. For optimal performance of
+ * this governor, provide a sustainable_power in the thermal zone's
+ * thermal_zone_params.
+ */
+static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
+{
+ u32 sustainable_power = 0;
+ struct thermal_instance *instance;
+ struct power_allocator_params *params = tz->governor_data;
+
+ list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+ struct thermal_cooling_device *cdev = instance->cdev;
+ u32 min_power;
+
+ if (instance->trip != params->trip_max_desired_temperature)
+ continue;
+
+ if (power_actor_get_min_power(cdev, tz, &min_power))
+ continue;
+
+ sustainable_power += min_power;
+ }
+
+ return sustainable_power;
+}
+
+/**
* pid_controller() - PID controller
* @tz: thermal zone we are operating in
* @current_temp: the current temperature in millicelsius
@@ -98,6 +131,7 @@ static u32 pid_controller(struct thermal_zone_device *tz,
{
s64 p, i, d, power_range;
s32 err, max_power_frac;
+ u32 sustainable_power;
struct power_allocator_params *params = tz->governor_data;
max_power_frac = int_to_frac(max_allocatable_power);
@@ -138,8 +172,11 @@ static u32 pid_controller(struct thermal_zone_device *tz,
power_range = p + i + d;
+ sustainable_power = tz->tzp->sustainable_power ?:
+ estimate_sustainable_power(tz);
+
/* feed-forward the known sustainable dissipatable power */
- power_range = tz->tzp->sustainable_power + frac_to_int(power_range);
+ power_range = sustainable_power + frac_to_int(power_range);
power_range = clamp(power_range, (s64)0, (s64)max_allocatable_power);
@@ -418,18 +455,18 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
int ret;
struct power_allocator_params *params;
unsigned long switch_on_temp, control_temp;
- u32 temperature_threshold;
+ u32 sustainable_power, temperature_threshold;
- if (!tz->tzp || !tz->tzp->sustainable_power) {
- dev_err(&tz->device,
- "power_allocator: missing sustainable_power\n");
+ if (!tz->tzp)
return -EINVAL;
- }
params = devm_kzalloc(&tz->device, sizeof(*params), GFP_KERNEL);
if (!params)
return -ENOMEM;
+ if (!tz->tzp->sustainable_power)
+ dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n");
+
ret = get_governor_trips(tz, params);
if (ret) {
dev_err(&tz->device,
@@ -448,13 +485,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
if (ret)
goto free;
+ /*
+ * Provide an arbitrary sustainable_power to set the default
+ * values of k_po and k_pu. We can estimate sustainable_power
+ * at this point because no cooling devices have been
+ * registered yet. By providing an arbitrary value we get
+ * better defaults that setting k_po and k_pu to 0.
+ */
+ sustainable_power = tz->tzp->sustainable_power ?: 2500;
temperature_threshold = control_temp - switch_on_temp;
tz->tzp->k_po = tz->tzp->k_po ?:
- int_to_frac(tz->tzp->sustainable_power) / temperature_threshold;
+ int_to_frac(sustainable_power) / temperature_threshold;
tz->tzp->k_pu = tz->tzp->k_pu ?:
- int_to_frac(2 * tz->tzp->sustainable_power) /
- temperature_threshold;
+ int_to_frac(2 * sustainable_power) / temperature_threshold;
tz->tzp->k_i = tz->tzp->k_i ?: int_to_frac(10) / 1000;
/*
* The default for k_d and integral_cutoff is 0, so we can
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 4ca211be4c0f..d26bc9e6f936 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -997,6 +997,34 @@ int power_actor_get_max_power(struct thermal_cooling_device *cdev,
}
/**
+ * power_actor_get_min_power() - get the mainimum power that a cdev can consume
+ * @cdev: pointer to &thermal_cooling_device
+ * @tz: a valid thermal zone device pointer
+ * @min_power: pointer in which to store the minimum power
+ *
+ * Calculate the minimum power consumption in milliwats that the
+ * cooling device can currently consume and store it in @min_power.
+ *
+ * Return: 0 on success, -EINVAL if @cdev doesn't support the
+ * power_actor API or -E* on other error.
+ */
+int power_actor_get_min_power(struct thermal_cooling_device *cdev,
+ struct thermal_zone_device *tz, u32 *min_power)
+{
+ unsigned long max_state;
+ int ret;
+
+ if (!cdev_is_power_actor(cdev))
+ return -EINVAL;
+
+ ret = cdev->ops->get_max_state(cdev, &max_state);
+ if (ret)
+ return ret;
+
+ return cdev->ops->state2power(cdev, tz, max_state, min_power);
+}
+
+/**
* power_actor_set_power() - limit the maximum power that a cooling device can consume
* @cdev: pointer to &thermal_cooling_device
* @instance: thermal instance to update
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 037e9df2f610..f99d934d373a 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -384,6 +384,8 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev)
int power_actor_get_max_power(struct thermal_cooling_device *,
struct thermal_zone_device *tz, u32 *max_power);
+int power_actor_get_min_power(struct thermal_cooling_device *,
+ struct thermal_zone_device *tz, u32 *min_power);
int power_actor_set_power(struct thermal_cooling_device *,
struct thermal_instance *, u32);
struct thermal_zone_device *thermal_zone_device_register(const char *, int, int,
@@ -419,6 +421,10 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev)
static inline int power_actor_get_max_power(struct thermal_cooling_device *cdev,
struct thermal_zone_device *tz, u32 *max_power)
{ return 0; }
+static inline int power_actor_get_min_power(struct thermal_cooling_device *cdev,
+ struct thermal_zone_device *tz,
+ u32 *min_power)
+{ return -ENODEV; }
static inline int power_actor_set_power(struct thermal_cooling_device *cdev,
struct thermal_instance *tz, u32 power)
{ return 0; }
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Punit Agrawal <punit.agrawal@arm.com> |
|---|---|
| Date | 2015-08-11 15:50 +0200 |
| Subject | Re: [PATCH v2 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp |
| Message-ID | <pWi6d-6Jq-15@gated-at.bofh.it> |
| In reply to | #1205021 |
Hi Javi,
A few nitpicks and a comment below.
Javi Merino <javi.merino@arm.com> writes:
> The power allocator governor currently requires that a sustainable power
> is passed as part of the thermal zone's thermal zone parameters. If
> that parameter is not provided, it doesn't register with the thermal
> zone.
>
> While this parameter is strongly recommended for optimal performance, it
> doesn't need to be mandatory. Relax the requirement and allow the
> governor to bind to thermal zones that don't provide it by estimating it
> from the cooling devices' power model.
>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> ---
> drivers/thermal/power_allocator.c | 62 +++++++++++++++++++++++++++++++++------
> drivers/thermal/thermal_core.c | 28 ++++++++++++++++++
> include/linux/thermal.h | 6 ++++
> 3 files changed, 87 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
> index 63a448f9d93b..f78836c2da26 100644
> --- a/drivers/thermal/power_allocator.c
> +++ b/drivers/thermal/power_allocator.c
> @@ -73,6 +73,39 @@ struct power_allocator_params {
> };
>
> /**
> + * estimate_sustainable_power() - Estimate the sustainable power of a thermal zone
> + * @tz: thermal zone we are operating in
> + *
> + * For thermal zones that don't provide a sustainable_power in their
> + * thermal_zone_params, estimate one. Calculate it using the minimum
> + * power of all the cooling devices as that gives a valid value that
> + * can give some degree of functionality. For optimal performance of
> + * this governor, provide a sustainable_power in the thermal zone's
> + * thermal_zone_params.
> + */
> +static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
> +{
> + u32 sustainable_power = 0;
> + struct thermal_instance *instance;
> + struct power_allocator_params *params = tz->governor_data;
> +
> + list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> + struct thermal_cooling_device *cdev = instance->cdev;
> + u32 min_power;
> +
> + if (instance->trip != params->trip_max_desired_temperature)
> + continue;
> +
> + if (power_actor_get_min_power(cdev, tz, &min_power))
> + continue;
> +
> + sustainable_power += min_power;
> + }
> +
> + return sustainable_power;
> +}
> +
> +/**
> * pid_controller() - PID controller
> * @tz: thermal zone we are operating in
> * @current_temp: the current temperature in millicelsius
> @@ -98,6 +131,7 @@ static u32 pid_controller(struct thermal_zone_device *tz,
> {
> s64 p, i, d, power_range;
> s32 err, max_power_frac;
> + u32 sustainable_power;
> struct power_allocator_params *params = tz->governor_data;
>
> max_power_frac = int_to_frac(max_allocatable_power);
> @@ -138,8 +172,11 @@ static u32 pid_controller(struct thermal_zone_device *tz,
>
> power_range = p + i + d;
>
> + sustainable_power = tz->tzp->sustainable_power ?:
> + estimate_sustainable_power(tz);
> +
> /* feed-forward the known sustainable dissipatable power */
> - power_range = tz->tzp->sustainable_power + frac_to_int(power_range);
> + power_range = sustainable_power + frac_to_int(power_range);
>
> power_range = clamp(power_range, (s64)0, (s64)max_allocatable_power);
>
> @@ -418,18 +455,18 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
> int ret;
> struct power_allocator_params *params;
> unsigned long switch_on_temp, control_temp;
> - u32 temperature_threshold;
> + u32 sustainable_power, temperature_threshold;
>
> - if (!tz->tzp || !tz->tzp->sustainable_power) {
> - dev_err(&tz->device,
> - "power_allocator: missing sustainable_power\n");
> + if (!tz->tzp)
> return -EINVAL;
> - }
>
> params = devm_kzalloc(&tz->device, sizeof(*params), GFP_KERNEL);
> if (!params)
> return -ENOMEM;
>
> + if (!tz->tzp->sustainable_power)
> + dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n");
> +
> ret = get_governor_trips(tz, params);
> if (ret) {
> dev_err(&tz->device,
> @@ -448,13 +485,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
> if (ret)
> goto free;
>
> + /*
> + * Provide an arbitrary sustainable_power to set the default
> + * values of k_po and k_pu. We can estimate sustainable_power
^
can not
> + * at this point because no cooling devices have been
> + * registered yet. By providing an arbitrary value we get
> + * better defaults that setting k_po and k_pu to 0.
^
than
> + */
> + sustainable_power = tz->tzp->sustainable_power ?: 2500;
> temperature_threshold = control_temp - switch_on_temp;
>
> tz->tzp->k_po = tz->tzp->k_po ?:
> - int_to_frac(tz->tzp->sustainable_power) / temperature_threshold;
> + int_to_frac(sustainable_power) / temperature_threshold;
> tz->tzp->k_pu = tz->tzp->k_pu ?:
> - int_to_frac(2 * tz->tzp->sustainable_power) /
> - temperature_threshold;
> + int_to_frac(2 * sustainable_power) / temperature_threshold;
As we are being conservative with our estimation of sustainable power
(sum of mins) when it is not explicitly specified, should we be
conservative here and let the proportional terms, k_po and k_pu be zero
as well?
> tz->tzp->k_i = tz->tzp->k_i ?: int_to_frac(10) / 1000;
> /*
> * The default for k_d and integral_cutoff is 0, so we can
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 4ca211be4c0f..d26bc9e6f936 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -997,6 +997,34 @@ int power_actor_get_max_power(struct thermal_cooling_device *cdev,
> }
>
> /**
> + * power_actor_get_min_power() - get the mainimum power that a cdev can consume
> + * @cdev: pointer to &thermal_cooling_device
> + * @tz: a valid thermal zone device pointer
> + * @min_power: pointer in which to store the minimum power
> + *
> + * Calculate the minimum power consumption in milliwats that the
> + * cooling device can currently consume and store it in @min_power.
> + *
> + * Return: 0 on success, -EINVAL if @cdev doesn't support the
> + * power_actor API or -E* on other error.
> + */
> +int power_actor_get_min_power(struct thermal_cooling_device *cdev,
> + struct thermal_zone_device *tz, u32 *min_power)
> +{
> + unsigned long max_state;
> + int ret;
> +
> + if (!cdev_is_power_actor(cdev))
> + return -EINVAL;
> +
> + ret = cdev->ops->get_max_state(cdev, &max_state);
> + if (ret)
> + return ret;
> +
> + return cdev->ops->state2power(cdev, tz, max_state, min_power);
> +}
> +
> +/**
> * power_actor_set_power() - limit the maximum power that a cooling device can consume
> * @cdev: pointer to &thermal_cooling_device
> * @instance: thermal instance to update
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 037e9df2f610..f99d934d373a 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -384,6 +384,8 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev)
>
> int power_actor_get_max_power(struct thermal_cooling_device *,
> struct thermal_zone_device *tz, u32 *max_power);
> +int power_actor_get_min_power(struct thermal_cooling_device *,
> + struct thermal_zone_device *tz, u32 *min_power);
> int power_actor_set_power(struct thermal_cooling_device *,
> struct thermal_instance *, u32);
> struct thermal_zone_device *thermal_zone_device_register(const char *, int, int,
> @@ -419,6 +421,10 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev)
> static inline int power_actor_get_max_power(struct thermal_cooling_device *cdev,
> struct thermal_zone_device *tz, u32 *max_power)
> { return 0; }
> +static inline int power_actor_get_min_power(struct thermal_cooling_device *cdev,
> + struct thermal_zone_device *tz,
> + u32 *min_power)
> +{ return -ENODEV; }
Perhaps return 0 like power_actor_get_max_power just above for
consistency.
> static inline int power_actor_set_power(struct thermal_cooling_device *cdev,
> struct thermal_instance *tz, u32 power)
> { return 0; }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Javi Merino <javi.merino@arm.com> |
|---|---|
| Date | 2015-08-11 19:40 +0200 |
| Subject | Re: [PATCH v2 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp |
| Message-ID | <pWlGO-3z2-21@gated-at.bofh.it> |
| In reply to | #1205148 |
On Tue, Aug 11, 2015 at 02:42:20PM +0100, Punit Agrawal wrote:
> Hi Javi,
>
> A few nitpicks and a comment below.
>
> Javi Merino <javi.merino@arm.com> writes:
>
> > The power allocator governor currently requires that a sustainable power
> > is passed as part of the thermal zone's thermal zone parameters. If
> > that parameter is not provided, it doesn't register with the thermal
> > zone.
> >
> > While this parameter is strongly recommended for optimal performance, it
> > doesn't need to be mandatory. Relax the requirement and allow the
> > governor to bind to thermal zones that don't provide it by estimating it
> > from the cooling devices' power model.
> >
> > Cc: Zhang Rui <rui.zhang@intel.com>
> > Cc: Eduardo Valentin <edubezval@gmail.com>
> > Signed-off-by: Javi Merino <javi.merino@arm.com>
> > ---
> > drivers/thermal/power_allocator.c | 62 +++++++++++++++++++++++++++++++++------
> > drivers/thermal/thermal_core.c | 28 ++++++++++++++++++
> > include/linux/thermal.h | 6 ++++
> > 3 files changed, 87 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
> > index 63a448f9d93b..f78836c2da26 100644
> > --- a/drivers/thermal/power_allocator.c
> > +++ b/drivers/thermal/power_allocator.c
> > @@ -73,6 +73,39 @@ struct power_allocator_params {
> > };
> >
> > /**
> > + * estimate_sustainable_power() - Estimate the sustainable power of a thermal zone
> > + * @tz: thermal zone we are operating in
> > + *
> > + * For thermal zones that don't provide a sustainable_power in their
> > + * thermal_zone_params, estimate one. Calculate it using the minimum
> > + * power of all the cooling devices as that gives a valid value that
> > + * can give some degree of functionality. For optimal performance of
> > + * this governor, provide a sustainable_power in the thermal zone's
> > + * thermal_zone_params.
> > + */
> > +static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
> > +{
> > + u32 sustainable_power = 0;
> > + struct thermal_instance *instance;
> > + struct power_allocator_params *params = tz->governor_data;
> > +
> > + list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> > + struct thermal_cooling_device *cdev = instance->cdev;
> > + u32 min_power;
> > +
> > + if (instance->trip != params->trip_max_desired_temperature)
> > + continue;
> > +
> > + if (power_actor_get_min_power(cdev, tz, &min_power))
> > + continue;
> > +
> > + sustainable_power += min_power;
> > + }
> > +
> > + return sustainable_power;
> > +}
> > +
> > +/**
> > * pid_controller() - PID controller
> > * @tz: thermal zone we are operating in
> > * @current_temp: the current temperature in millicelsius
> > @@ -98,6 +131,7 @@ static u32 pid_controller(struct thermal_zone_device *tz,
> > {
> > s64 p, i, d, power_range;
> > s32 err, max_power_frac;
> > + u32 sustainable_power;
> > struct power_allocator_params *params = tz->governor_data;
> >
> > max_power_frac = int_to_frac(max_allocatable_power);
> > @@ -138,8 +172,11 @@ static u32 pid_controller(struct thermal_zone_device *tz,
> >
> > power_range = p + i + d;
> >
> > + sustainable_power = tz->tzp->sustainable_power ?:
> > + estimate_sustainable_power(tz);
> > +
> > /* feed-forward the known sustainable dissipatable power */
> > - power_range = tz->tzp->sustainable_power + frac_to_int(power_range);
> > + power_range = sustainable_power + frac_to_int(power_range);
> >
> > power_range = clamp(power_range, (s64)0, (s64)max_allocatable_power);
> >
> > @@ -418,18 +455,18 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
> > int ret;
> > struct power_allocator_params *params;
> > unsigned long switch_on_temp, control_temp;
> > - u32 temperature_threshold;
> > + u32 sustainable_power, temperature_threshold;
> >
> > - if (!tz->tzp || !tz->tzp->sustainable_power) {
> > - dev_err(&tz->device,
> > - "power_allocator: missing sustainable_power\n");
> > + if (!tz->tzp)
> > return -EINVAL;
> > - }
> >
> > params = devm_kzalloc(&tz->device, sizeof(*params), GFP_KERNEL);
> > if (!params)
> > return -ENOMEM;
> >
> > + if (!tz->tzp->sustainable_power)
> > + dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n");
> > +
> > ret = get_governor_trips(tz, params);
> > if (ret) {
> > dev_err(&tz->device,
> > @@ -448,13 +485,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
> > if (ret)
> > goto free;
> >
> > + /*
> > + * Provide an arbitrary sustainable_power to set the default
> > + * values of k_po and k_pu. We can estimate sustainable_power
> ^
> can not
> > + * at this point because no cooling devices have been
> > + * registered yet. By providing an arbitrary value we get
> > + * better defaults that setting k_po and k_pu to 0.
> ^
> than
Fixed both.
> > + */
> > + sustainable_power = tz->tzp->sustainable_power ?: 2500;
> > temperature_threshold = control_temp - switch_on_temp;
> >
> > tz->tzp->k_po = tz->tzp->k_po ?:
> > - int_to_frac(tz->tzp->sustainable_power) / temperature_threshold;
> > + int_to_frac(sustainable_power) / temperature_threshold;
> > tz->tzp->k_pu = tz->tzp->k_pu ?:
> > - int_to_frac(2 * tz->tzp->sustainable_power) /
> > - temperature_threshold;
> > + int_to_frac(2 * sustainable_power) / temperature_threshold;
>
> As we are being conservative with our estimation of sustainable power
> (sum of mins) when it is not explicitly specified, should we be
> conservative here and let the proportional terms, k_po and k_pu be zero
> as well?
Yes, we could do. If we set it to zero, all cooling devices will be
set to the maximum cooling state when the governor is active. If we
set it to a valid(ish) value, it will do a bit of thermal control.
It's not perfect but I think it's better to have a bad estimate than
no estimate.
> > tz->tzp->k_i = tz->tzp->k_i ?: int_to_frac(10) / 1000;
> > /*
> > * The default for k_d and integral_cutoff is 0, so we can
> > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> > index 4ca211be4c0f..d26bc9e6f936 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -997,6 +997,34 @@ int power_actor_get_max_power(struct thermal_cooling_device *cdev,
> > }
> >
> > /**
> > + * power_actor_get_min_power() - get the mainimum power that a cdev can consume
> > + * @cdev: pointer to &thermal_cooling_device
> > + * @tz: a valid thermal zone device pointer
> > + * @min_power: pointer in which to store the minimum power
> > + *
> > + * Calculate the minimum power consumption in milliwats that the
> > + * cooling device can currently consume and store it in @min_power.
> > + *
> > + * Return: 0 on success, -EINVAL if @cdev doesn't support the
> > + * power_actor API or -E* on other error.
> > + */
> > +int power_actor_get_min_power(struct thermal_cooling_device *cdev,
> > + struct thermal_zone_device *tz, u32 *min_power)
> > +{
> > + unsigned long max_state;
> > + int ret;
> > +
> > + if (!cdev_is_power_actor(cdev))
> > + return -EINVAL;
> > +
> > + ret = cdev->ops->get_max_state(cdev, &max_state);
> > + if (ret)
> > + return ret;
> > +
> > + return cdev->ops->state2power(cdev, tz, max_state, min_power);
> > +}
> > +
> > +/**
> > * power_actor_set_power() - limit the maximum power that a cooling device can consume
> > * @cdev: pointer to &thermal_cooling_device
> > * @instance: thermal instance to update
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index 037e9df2f610..f99d934d373a 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -384,6 +384,8 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev)
> >
> > int power_actor_get_max_power(struct thermal_cooling_device *,
> > struct thermal_zone_device *tz, u32 *max_power);
> > +int power_actor_get_min_power(struct thermal_cooling_device *,
> > + struct thermal_zone_device *tz, u32 *min_power);
> > int power_actor_set_power(struct thermal_cooling_device *,
> > struct thermal_instance *, u32);
> > struct thermal_zone_device *thermal_zone_device_register(const char *, int, int,
> > @@ -419,6 +421,10 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev)
> > static inline int power_actor_get_max_power(struct thermal_cooling_device *cdev,
> > struct thermal_zone_device *tz, u32 *max_power)
> > { return 0; }
> > +static inline int power_actor_get_min_power(struct thermal_cooling_device *cdev,
> > + struct thermal_zone_device *tz,
> > + u32 *min_power)
> > +{ return -ENODEV; }
>
> Perhaps return 0 like power_actor_get_max_power just above for
> consistency.
No, return 0 is not a good idea. If you return 0, you're telling the
calling function that everything went all right and you've put the
minimum power in @min_power.
What we should do is fix power_actor_get_max_power() and
power_actor_set_power() to return an error value if !CONFIG_THERMAL.
Cheers,
Javi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Daniel Kurtz <djkurtz@chromium.org> |
|---|---|
| Date | 2015-08-12 13:10 +0200 |
| Subject | Re: [PATCH v2 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp |
| Message-ID | <pWC4V-2z5-5@gated-at.bofh.it> |
| In reply to | #1205021 |
Hi Javi,
One Tiny nit below...
On Tue, Aug 11, 2015 at 6:21 PM, Javi Merino <javi.merino@arm.com> wrote:
> The power allocator governor currently requires that a sustainable power
> is passed as part of the thermal zone's thermal zone parameters. If
> that parameter is not provided, it doesn't register with the thermal
> zone.
>
> While this parameter is strongly recommended for optimal performance, it
> doesn't need to be mandatory. Relax the requirement and allow the
> governor to bind to thermal zones that don't provide it by estimating it
> from the cooling devices' power model.
>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> ---
[snip]
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -997,6 +997,34 @@ int power_actor_get_max_power(struct thermal_cooling_device *cdev,
> }
>
> /**
> + * power_actor_get_min_power() - get the mainimum power that a cdev can consume
> + * @cdev: pointer to &thermal_cooling_device
> + * @tz: a valid thermal zone device pointer
> + * @min_power: pointer in which to store the minimum power
> + *
> + * Calculate the minimum power consumption in milliwats that the
^
milliwatts
Thanks,
-Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Javi Merino <javi.merino@arm.com> |
|---|---|
| Date | 2015-08-11 12:30 +0200 |
| Subject | [PATCH v2 3/4] thermal: power_allocator: don't require tzp to be present for the thermal zone |
| Message-ID | <pWeYF-2jf-5@gated-at.bofh.it> |
| In reply to | #1205020 |
Thermal zones created using thermal_zone_device_create() may not have
tzp. As the governor gets its parameters from there, allocate it while
the governor is bound to the thermal zone so that it can operate in it.
In this case, tzp is freed when the thermal zone switches to another
governor.
---
While this would be easier to do by just ignoring the thermal zone if
there was no tzp, I think the approach in this patch provides a better
behavior.
drivers/thermal/power_allocator.c | 32 +++++++++++++++++++++++++++-----
1 file changed, 27 insertions(+), 5 deletions(-)
diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
index 257c9af20f22..38beea721e18 100644
--- a/drivers/thermal/power_allocator.c
+++ b/drivers/thermal/power_allocator.c
@@ -58,6 +58,8 @@ static inline s64 div_frac(s64 x, s64 y)
/**
* struct power_allocator_params - parameters for the power allocator governor
+ * @allocated_tzp: whether we have allocated tzp for this thermal zone and
+ * it needs to be freed on unbind
* @err_integral: accumulated error in the PID controller.
* @prev_err: error in the previous iteration of the PID controller.
* Used to calculate the derivative term.
@@ -70,6 +72,7 @@ static inline s64 div_frac(s64 x, s64 y)
* controlling for.
*/
struct power_allocator_params {
+ bool allocated_tzp;
s64 err_integral;
s32 prev_err;
int trip_switch_on;
@@ -473,8 +476,7 @@ static void allow_maximum_power(struct thermal_zone_device *tz)
* Initialize the PID controller parameters and bind it to the thermal
* zone.
*
- * Return: 0 on success, -EINVAL if the thermal zone doesn't have tzp or -ENOMEM
- * if we ran out of memory.
+ * Return: 0 on success, or -ENOMEM if we ran out of memory.
*/
static int power_allocator_bind(struct thermal_zone_device *tz)
{
@@ -483,13 +485,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
unsigned long switch_on_temp, control_temp;
u32 sustainable_power, temperature_threshold;
- if (!tz->tzp)
- return -EINVAL;
-
params = devm_kzalloc(&tz->device, sizeof(*params), GFP_KERNEL);
if (!params)
return -ENOMEM;
+ if (!tz->tzp) {
+ tz->tzp = kzalloc(sizeof(*tz->tzp), GFP_KERNEL);
+ if (!tz->tzp) {
+ ret = -ENOMEM;
+ goto free_params;
+ }
+
+ params->allocated_tzp = true;
+ }
+
if (!tz->tzp->sustainable_power)
dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n");
@@ -531,11 +540,24 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
tz->governor_data = params;
return 0;
+
+free_params:
+ devm_kfree(&tz->device, params);
+
+ return ret;
}
static void power_allocator_unbind(struct thermal_zone_device *tz)
{
+ struct power_allocator_params *params = tz->governor_data;
+
dev_dbg(&tz->device, "Unbinding from thermal zone %d\n", tz->id);
+
+ if (params->allocated_tzp) {
+ kfree(tz->tzp);
+ tz->tzp = NULL;
+ }
+
devm_kfree(&tz->device, tz->governor_data);
tz->governor_data = NULL;
}
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Javi Merino <javi.merino@arm.com> |
|---|---|
| Date | 2015-08-11 12:30 +0200 |
| Subject | [PATCH v2 2/4] thermal: power_allocator: relax the requirement of two passive trip points |
| Message-ID | <pWeYF-2jf-9@gated-at.bofh.it> |
| In reply to | #1205020 |
The power allocator governor currently requires that the thermal zone
has at least two passive trip points. If there aren't, the governor
refuses to bind to the thermal zone.
This commit relaxes that requirement. Now the governor will bind to all
thermal zones regardless of how many trip points they have.
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
Documentation/thermal/power_allocator.txt | 2 +-
drivers/thermal/power_allocator.c | 91 +++++++++++++++++--------------
2 files changed, 52 insertions(+), 41 deletions(-)
diff --git a/Documentation/thermal/power_allocator.txt b/Documentation/thermal/power_allocator.txt
index c3797b529991..a1ce2235f121 100644
--- a/Documentation/thermal/power_allocator.txt
+++ b/Documentation/thermal/power_allocator.txt
@@ -4,7 +4,7 @@ Power allocator governor tunables
Trip points
-----------
-The governor requires the following two passive trip points:
+The governor works optimally with the following two passive trip points:
1. "switch on" trip point: temperature above which the governor
control loop starts operating. This is the first passive trip
diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
index f78836c2da26..257c9af20f22 100644
--- a/drivers/thermal/power_allocator.c
+++ b/drivers/thermal/power_allocator.c
@@ -24,6 +24,8 @@
#include "thermal_core.h"
+#define INVALID_TRIP -1
+
#define FRAC_BITS 10
#define int_to_frac(x) ((x) << FRAC_BITS)
#define frac_to_int(x) ((x) >> FRAC_BITS)
@@ -61,6 +63,8 @@ static inline s64 div_frac(s64 x, s64 y)
* Used to calculate the derivative term.
* @trip_switch_on: first passive trip point of the thermal zone. The
* governor switches on when this trip point is crossed.
+ * If the thermal zone only has one passive trip point,
+ * @trip_switch_on should be INVALID_TRIP.
* @trip_max_desired_temperature: last passive trip point of the thermal
* zone. The temperature we are
* controlling for.
@@ -378,43 +382,66 @@ unlock:
return ret;
}
-static int get_governor_trips(struct thermal_zone_device *tz,
- struct power_allocator_params *params)
+/**
+ * get_governor_trips() - get the number of the two trip points that are key for this governor
+ * @tz: thermal zone to operate on
+ * @params: pointer the private data for this governor
+ *
+ * The power allocator governor works optimally with two trips points:
+ * a "switch on" trip point and a "maximum desired temperature". These
+ * are defined as the first and last passive trip points.
+ *
+ * If there is only one trip point, then that's considered to be the
+ * "maximum desired temperature" trip point and the governor is always
+ * on. If there are no passive or active trip points, then the
+ * governor won't do anything. In fact, its throttle function
+ * shouldn't be called at all.
+ */
+static void get_governor_trips(struct thermal_zone_device *tz,
+ struct power_allocator_params *params)
{
- int i, ret, last_passive;
+ int i, last_active, last_passive;
bool found_first_passive;
found_first_passive = false;
- last_passive = -1;
- ret = -EINVAL;
+ last_active = INVALID_TRIP;
+ last_passive = INVALID_TRIP;
for (i = 0; i < tz->trips; i++) {
enum thermal_trip_type type;
+ int ret;
ret = tz->ops->get_trip_type(tz, i, &type);
- if (ret)
- return ret;
+ if (ret) {
+ dev_warn(&tz->device,
+ "Failed to get trip point %d type: %d\n", i,
+ ret);
+ continue;
+ }
- if (!found_first_passive) {
- if (type == THERMAL_TRIP_PASSIVE) {
+ if (type == THERMAL_TRIP_PASSIVE) {
+ if (!found_first_passive) {
params->trip_switch_on = i;
found_first_passive = true;
+ } else {
+ last_passive = i;
}
- } else if (type == THERMAL_TRIP_PASSIVE) {
- last_passive = i;
+ } else if (type == THERMAL_TRIP_ACTIVE) {
+ last_active = i;
} else {
break;
}
}
- if (last_passive != -1) {
+ if (last_passive != INVALID_TRIP) {
params->trip_max_desired_temperature = last_passive;
- ret = 0;
+ } else if (found_first_passive) {
+ params->trip_max_desired_temperature = params->trip_switch_on;
+ params->trip_switch_on = INVALID_TRIP;
} else {
- ret = -EINVAL;
+ params->trip_switch_on = INVALID_TRIP;
+ params->trip_max_desired_temperature = last_active;
}
-
- return ret;
}
static void reset_pid_controller(struct power_allocator_params *params)
@@ -443,11 +470,10 @@ static void allow_maximum_power(struct thermal_zone_device *tz)
* power_allocator_bind() - bind the power_allocator governor to a thermal zone
* @tz: thermal zone to bind it to
*
- * Check that the thermal zone is valid for this governor, that is, it
- * has two thermal trips. If so, initialize the PID controller
- * parameters and bind it to the thermal zone.
+ * Initialize the PID controller parameters and bind it to the thermal
+ * zone.
*
- * Return: 0 on success, -EINVAL if the trips were invalid or -ENOMEM
+ * Return: 0 on success, -EINVAL if the thermal zone doesn't have tzp or -ENOMEM
* if we ran out of memory.
*/
static int power_allocator_bind(struct thermal_zone_device *tz)
@@ -467,23 +493,18 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
if (!tz->tzp->sustainable_power)
dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n");
- ret = get_governor_trips(tz, params);
- if (ret) {
- dev_err(&tz->device,
- "thermal zone %s has wrong trip setup for power allocator\n",
- tz->type);
- goto free;
- }
+ get_governor_trips(tz, params);
ret = tz->ops->get_trip_temp(tz, params->trip_switch_on,
&switch_on_temp);
if (ret)
- goto free;
+ switch_on_temp = 0;
ret = tz->ops->get_trip_temp(tz, params->trip_max_desired_temperature,
&control_temp);
if (ret)
- goto free;
+ /* Set some valid value to avoid division by zero below */
+ control_temp = 70000;
/*
* Provide an arbitrary sustainable_power to set the default
@@ -510,10 +531,6 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
tz->governor_data = params;
return 0;
-
-free:
- devm_kfree(&tz->device, params);
- return ret;
}
static void power_allocator_unbind(struct thermal_zone_device *tz)
@@ -544,13 +561,7 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip)
ret = tz->ops->get_trip_temp(tz, params->trip_switch_on,
&switch_on_temp);
- if (ret) {
- dev_warn(&tz->device,
- "Failed to get switch on temperature: %d\n", ret);
- return ret;
- }
-
- if (current_temp < switch_on_temp) {
+ if ((!ret) && (current_temp < switch_on_temp)) {
tz->passive = 0;
reset_pid_controller(params);
allow_maximum_power(tz);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Daniel Kurtz <djkurtz@chromium.org> |
|---|---|
| Date | 2015-08-12 13:20 +0200 |
| Subject | Re: [PATCH v2 2/4] thermal: power_allocator: relax the requirement of two passive trip points |
| Message-ID | <pWCeC-2Kj-23@gated-at.bofh.it> |
| In reply to | #1205024 |
Hi Javi,
tiny nits again...
On Tue, Aug 11, 2015 at 6:21 PM, Javi Merino <javi.merino@arm.com> wrote:
> The power allocator governor currently requires that the thermal zone
> has at least two passive trip points. If there aren't, the governor
> refuses to bind to the thermal zone.
>
> This commit relaxes that requirement. Now the governor will bind to all
> thermal zones regardless of how many trip points they have.
>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> ---
> Documentation/thermal/power_allocator.txt | 2 +-
> drivers/thermal/power_allocator.c | 91 +++++++++++++++++--------------
> 2 files changed, 52 insertions(+), 41 deletions(-)
>
> diff --git a/Documentation/thermal/power_allocator.txt b/Documentation/thermal/power_allocator.txt
> index c3797b529991..a1ce2235f121 100644
> --- a/Documentation/thermal/power_allocator.txt
> +++ b/Documentation/thermal/power_allocator.txt
> @@ -4,7 +4,7 @@ Power allocator governor tunables
> Trip points
> -----------
>
> -The governor requires the following two passive trip points:
> +The governor works optimally with the following two passive trip points:
>
> 1. "switch on" trip point: temperature above which the governor
> control loop starts operating. This is the first passive trip
> diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
> index f78836c2da26..257c9af20f22 100644
> --- a/drivers/thermal/power_allocator.c
> +++ b/drivers/thermal/power_allocator.c
> @@ -24,6 +24,8 @@
>
> #include "thermal_core.h"
>
> +#define INVALID_TRIP -1
> +
> #define FRAC_BITS 10
> #define int_to_frac(x) ((x) << FRAC_BITS)
> #define frac_to_int(x) ((x) >> FRAC_BITS)
> @@ -61,6 +63,8 @@ static inline s64 div_frac(s64 x, s64 y)
> * Used to calculate the derivative term.
> * @trip_switch_on: first passive trip point of the thermal zone. The
> * governor switches on when this trip point is crossed.
> + * If the thermal zone only has one passive trip point,
> + * @trip_switch_on should be INVALID_TRIP.
> * @trip_max_desired_temperature: last passive trip point of the thermal
> * zone. The temperature we are
> * controlling for.
> @@ -378,43 +382,66 @@ unlock:
> return ret;
> }
>
> -static int get_governor_trips(struct thermal_zone_device *tz,
> - struct power_allocator_params *params)
> +/**
> + * get_governor_trips() - get the number of the two trip points that are key for this governor
> + * @tz: thermal zone to operate on
> + * @params: pointer the private data for this governor
^
pointer to private data
> + *
> + * The power allocator governor works optimally with two trips points:
> + * a "switch on" trip point and a "maximum desired temperature". These
> + * are defined as the first and last passive trip points.
> + *
> + * If there is only one trip point, then that's considered to be the
> + * "maximum desired temperature" trip point and the governor is always
> + * on. If there are no passive or active trip points, then the
> + * governor won't do anything. In fact, its throttle function
> + * shouldn't be called at all.
^
"shouldn't" or "won't" ?
[snip]
> static void power_allocator_unbind(struct thermal_zone_device *tz)
> @@ -544,13 +561,7 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip)
>
> ret = tz->ops->get_trip_temp(tz, params->trip_switch_on,
> &switch_on_temp);
> - if (ret) {
> - dev_warn(&tz->device,
> - "Failed to get switch on temperature: %d\n", ret);
> - return ret;
> - }
> -
> - if (current_temp < switch_on_temp) {
> + if ((!ret) && (current_temp < switch_on_temp)) {
nit: I think the inner pair of () are not necessary.
Thanks,
-Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Javi Merino <javi.merino@arm.com> |
|---|---|
| Date | 2015-08-12 18:50 +0200 |
| Subject | Re: [PATCH v2 2/4] thermal: power_allocator: relax the requirement of two passive trip points |
| Message-ID | <pWHnY-1DY-19@gated-at.bofh.it> |
| In reply to | #1205970 |
On Wed, Aug 12, 2015 at 12:13:09PM +0100, Daniel Kurtz wrote:
> Hi Javi,
Hi Daniel,
> On Tue, Aug 11, 2015 at 6:21 PM, Javi Merino <javi.merino@arm.com> wrote:
> > The power allocator governor currently requires that the thermal zone
> > has at least two passive trip points. If there aren't, the governor
> > refuses to bind to the thermal zone.
> >
> > This commit relaxes that requirement. Now the governor will bind to all
> > thermal zones regardless of how many trip points they have.
> >
> > Cc: Zhang Rui <rui.zhang@intel.com>
> > Cc: Eduardo Valentin <edubezval@gmail.com>
> > Signed-off-by: Javi Merino <javi.merino@arm.com>
> > ---
> > Documentation/thermal/power_allocator.txt | 2 +-
> > drivers/thermal/power_allocator.c | 91 +++++++++++++++++--------------
> > 2 files changed, 52 insertions(+), 41 deletions(-)
> >
> > diff --git a/Documentation/thermal/power_allocator.txt b/Documentation/thermal/power_allocator.txt
> > index c3797b529991..a1ce2235f121 100644
> > --- a/Documentation/thermal/power_allocator.txt
> > +++ b/Documentation/thermal/power_allocator.txt
> > @@ -4,7 +4,7 @@ Power allocator governor tunables
> > Trip points
> > -----------
> >
> > -The governor requires the following two passive trip points:
> > +The governor works optimally with the following two passive trip points:
> >
> > 1. "switch on" trip point: temperature above which the governor
> > control loop starts operating. This is the first passive trip
> > diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
> > index f78836c2da26..257c9af20f22 100644
> > --- a/drivers/thermal/power_allocator.c
> > +++ b/drivers/thermal/power_allocator.c
> > @@ -24,6 +24,8 @@
> >
> > #include "thermal_core.h"
> >
> > +#define INVALID_TRIP -1
> > +
> > #define FRAC_BITS 10
> > #define int_to_frac(x) ((x) << FRAC_BITS)
> > #define frac_to_int(x) ((x) >> FRAC_BITS)
> > @@ -61,6 +63,8 @@ static inline s64 div_frac(s64 x, s64 y)
> > * Used to calculate the derivative term.
> > * @trip_switch_on: first passive trip point of the thermal zone. The
> > * governor switches on when this trip point is crossed.
> > + * If the thermal zone only has one passive trip point,
> > + * @trip_switch_on should be INVALID_TRIP.
> > * @trip_max_desired_temperature: last passive trip point of the thermal
> > * zone. The temperature we are
> > * controlling for.
> > @@ -378,43 +382,66 @@ unlock:
> > return ret;
> > }
> >
> > -static int get_governor_trips(struct thermal_zone_device *tz,
> > - struct power_allocator_params *params)
> > +/**
> > + * get_governor_trips() - get the number of the two trip points that are key for this governor
> > + * @tz: thermal zone to operate on
> > + * @params: pointer the private data for this governor
> ^
> pointer to private data
Fixed
> > + *
> > + * The power allocator governor works optimally with two trips points:
> > + * a "switch on" trip point and a "maximum desired temperature". These
> > + * are defined as the first and last passive trip points.
> > + *
> > + * If there is only one trip point, then that's considered to be the
> > + * "maximum desired temperature" trip point and the governor is always
> > + * on. If there are no passive or active trip points, then the
> > + * governor won't do anything. In fact, its throttle function
> > + * shouldn't be called at all.
> ^
> "shouldn't" or "won't" ?
"won't". Sometimes I'm overly cautious. You know "never say never" ;-)
> [snip]
>
> > static void power_allocator_unbind(struct thermal_zone_device *tz)
> > @@ -544,13 +561,7 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip)
> >
> > ret = tz->ops->get_trip_temp(tz, params->trip_switch_on,
> > &switch_on_temp);
> > - if (ret) {
> > - dev_warn(&tz->device,
> > - "Failed to get switch on temperature: %d\n", ret);
> > - return ret;
> > - }
> > -
> > - if (current_temp < switch_on_temp) {
> > + if ((!ret) && (current_temp < switch_on_temp)) {
>
> nit: I think the inner pair of () are not necessary.
They are not necessary, but I prefer the parenthesis around the
comparison. It makes it clearer to me. I've dropped the () around
!ret.
Thanks for the comments here and in the first patch.
Javi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Javi Merino <javi.merino@arm.com> |
|---|---|
| Date | 2015-08-11 12:30 +0200 |
| Subject | [PATCH v2 4/4] thermal: power_allocator: exit early if there are no cooling devices |
| Message-ID | <pWeYG-2jf-15@gated-at.bofh.it> |
| In reply to | #1205020 |
Don't waste cycles in the power allocator governor's throttle function
if there are no cooling devices and exit early.
This commit doesn't change any functionality, but should provide better
performance for the odd case of a thermal zone with trip points but
without cooling devices.
---
drivers/thermal/power_allocator.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
index 38beea721e18..05a3c2f21fa6 100644
--- a/drivers/thermal/power_allocator.c
+++ b/drivers/thermal/power_allocator.c
@@ -291,6 +291,11 @@ static int allocate_power(struct thermal_zone_device *tz,
}
}
+ if (!num_actors) {
+ ret = -ENODEV;
+ goto unlock;
+ }
+
/*
* We need to allocate five arrays of the same size:
* req_power, max_power, granted_power, extra_actor_power and
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web