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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > > ---
> > > 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
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 <[email protected]>
> > > > ---
> > > > 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
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
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 <[email protected]>
Cc: Eduardo Valentin <[email protected]>
Signed-off-by: Javi Merino <[email protected]>
---
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
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 <[email protected]>
Cc: Eduardo Valentin <[email protected]>
Signed-off-by: Javi Merino <[email protected]>
---
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
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
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
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 <[email protected]>
Cc: Eduardo Valentin <[email protected]>
Signed-off-by: Javi Merino <[email protected]>
---
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
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 <[email protected]>
Cc: Eduardo Valentin <[email protected]>
Signed-off-by: Javi Merino <[email protected]>
---
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
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
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
Hi Javi,
A few nitpicks and a comment below.
Javi Merino <[email protected]> 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 <[email protected]>
> Cc: Eduardo Valentin <[email protected]>
> Signed-off-by: Javi Merino <[email protected]>
> ---
> 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; }
On Tue, Aug 11, 2015 at 02:42:20PM +0100, Punit Agrawal wrote:
> Hi Javi,
>
> A few nitpicks and a comment below.
>
> Javi Merino <[email protected]> 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 <[email protected]>
> > Cc: Eduardo Valentin <[email protected]>
> > Signed-off-by: Javi Merino <[email protected]>
> > ---
> > 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
Hi Javi,
One Tiny nit below...
On Tue, Aug 11, 2015 at 6:21 PM, Javi Merino <[email protected]> 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 <[email protected]>
> Cc: Eduardo Valentin <[email protected]>
> Signed-off-by: Javi Merino <[email protected]>
> ---
[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
Hi Javi,
tiny nits again...
On Tue, Aug 11, 2015 at 6:21 PM, Javi Merino <[email protected]> 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 <[email protected]>
> Cc: Eduardo Valentin <[email protected]>
> Signed-off-by: Javi Merino <[email protected]>
> ---
> 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
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 <[email protected]> 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 <[email protected]>
> > Cc: Eduardo Valentin <[email protected]>
> > Signed-off-by: Javi Merino <[email protected]>
> > ---
> > 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
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 v2:
- Typos suggested by Daniel Kurtz
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
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 <[email protected]>
Cc: Eduardo Valentin <[email protected]>
Signed-off-by: Javi Merino <[email protected]>
---
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..7ec459780dff 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 not estimate sustainable_power
+ * at this point because no cooling devices have been
+ * registered yet. By providing an arbitrary value we get
+ * better defaults than 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..760204f0b63c 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 milliwatts 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
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 <[email protected]>
Cc: Eduardo Valentin <[email protected]>
Signed-off-by: Javi Merino <[email protected]>
---
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 7ec459780dff..b7c006fe20bd 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 to 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
+ * won'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
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.
Cc: Zhang Rui <[email protected]>
Cc: Eduardo Valentin <[email protected]>
Signed-off-by: Javi Merino <[email protected]>
---
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 b7c006fe20bd..6dcc4fedd4f2 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
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.
Cc: Zhang Rui <[email protected]>
Cc: Eduardo Valentin <[email protected]>
Signed-off-by: Javi Merino <[email protected]>
---
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 6dcc4fedd4f2..6b536ffd5ef6 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
On Mon, Aug 17, 2015 at 06:36:45PM +0100, Javi Merino 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 <[email protected]>
> Cc: Eduardo Valentin <[email protected]>
> Signed-off-by: Javi Merino <[email protected]>
> ---
> 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..7ec459780dff 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 not estimate sustainable_power
> + * at this point because no cooling devices have been
> + * registered yet. By providing an arbitrary value we get
> + * better defaults than setting k_po and k_pu to 0.
> + */
> + sustainable_power = tz->tzp->sustainable_power ?: 2500;
I think having 2500 here may produce constants that are not sane for
most thermal zones.
> 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;
I would prefer you move the constants estimations to where you have a
sane sustainable_power.
> /*
> * 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..760204f0b63c 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 milliwatts 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
>
On Thu, Aug 20, 2015 at 11:16:53PM +0100, Eduardo Valentin wrote:
> On Mon, Aug 17, 2015 at 06:36:45PM +0100, Javi Merino 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 <[email protected]>
> > Cc: Eduardo Valentin <[email protected]>
> > Signed-off-by: Javi Merino <[email protected]>
> > ---
> > 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..7ec459780dff 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 not estimate sustainable_power
> > + * at this point because no cooling devices have been
> > + * registered yet. By providing an arbitrary value we get
> > + * better defaults than setting k_po and k_pu to 0.
> > + */
> > + sustainable_power = tz->tzp->sustainable_power ?: 2500;
>
> I think having 2500 here may produce constants that are not sane for
> most thermal zones.
>
> > 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;
>
> I would prefer you move the constants estimations to where you have a
> sane sustainable_power.
Ok, I'll factor it to a function so that they can be estimated here if
you provide sustainable_power or they can be re-estimated in
pid_controller() if there is no sustainable_power.
Cheers,
Javi
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 v3:
- Don't hardcode a value for sustainable power and re-estimate
the PID controllers every time if no sustainable power is given
as suggested by Eduardo Valentin.
- power_actor_get_min_power() moved to a patch of its own.
Changes since v2:
- Typos suggested by Daniel Kurtz
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 (5):
thermal: Add a function to get the minimum power
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 | 241 ++++++++++++++++++++++--------
drivers/thermal/thermal_core.c | 28 ++++
include/linux/thermal.h | 6 +
4 files changed, 212 insertions(+), 65 deletions(-)
--
1.9.1
The thermal core already has a function to get the maximum power of a
cooling device: power_actor_get_max_power(). Add a function to get the
minimum power of a cooling device.
Cc: Zhang Rui <[email protected]>
Cc: Eduardo Valentin <[email protected]>
Signed-off-by: Javi Merino <[email protected]>
---
drivers/thermal/thermal_core.c | 28 ++++++++++++++++++++++++++++
include/linux/thermal.h | 6 ++++++
2 files changed, 34 insertions(+)
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 4ca211be4c0f..760204f0b63c 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 milliwatts 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
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 <[email protected]>
Cc: Eduardo Valentin <[email protected]>
Signed-off-by: Javi Merino <[email protected]>
---
drivers/thermal/power_allocator.c | 128 ++++++++++++++++++++++++++++++--------
1 file changed, 103 insertions(+), 25 deletions(-)
diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
index 7006860f2f36..eae8a5ae794a 100644
--- a/drivers/thermal/power_allocator.c
+++ b/drivers/thermal/power_allocator.c
@@ -73,6 +73,90 @@ 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;
+}
+
+/**
+ * estimate_controller_constants() - Estimate the constants for the PID controller
+ * @tz: thermal zone for which to estimate the constants
+ * @sustainable_power: sustainable power for the thermal zone
+ * @trip_switch_on: trip point number for the switch on temperature
+ * @control_temp: target temperature for the power allocator governor
+ * @force: whether to force the update of the constants
+ *
+ * This function is used to update the estimation of the PID
+ * controller constants in struct thermal_zone_parameters.
+ * Sustainable power is provided in case it was estimated. The
+ * estimated sustainable_power should not be stored in the
+ * thermal_zone_parameters so it has to be passed explicitly to this
+ * function.
+ *
+ * If @force is not set, the values in the thermal zone's parameters
+ * are preserved if they are not zero. If @force is set, the values
+ * in thermal zone's parameters are overwritten.
+ */
+static void estimate_controller_constants(struct thermal_zone_device *tz,
+ u32 sustainable_power,
+ int trip_switch_on,
+ unsigned long control_temp,
+ bool force)
+{
+ int ret;
+ unsigned long switch_on_temp;
+ u32 temperature_threshold;
+
+ ret = tz->ops->get_trip_temp(tz, trip_switch_on, &switch_on_temp);
+ if (ret)
+ switch_on_temp = 0;
+
+ temperature_threshold = control_temp - switch_on_temp;
+
+ if (!tz->tzp->k_po || force)
+ tz->tzp->k_po = int_to_frac(sustainable_power) /
+ temperature_threshold;
+
+ if (!tz->tzp->k_pu || force)
+ tz->tzp->k_pu = int_to_frac(2 * sustainable_power) /
+ temperature_threshold;
+
+ if (!tz->tzp->k_i || force)
+ tz->tzp->k_i = int_to_frac(10) / 1000;
+ /*
+ * The default for k_d and integral_cutoff is 0, so we can
+ * leave them as they are.
+ */
+}
+
+/**
* pid_controller() - PID controller
* @tz: thermal zone we are operating in
* @current_temp: the current temperature in millicelsius
@@ -98,10 +182,20 @@ 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);
+ if (tz->tzp->sustainable_power) {
+ sustainable_power = tz->tzp->sustainable_power;
+ } else {
+ sustainable_power = estimate_sustainable_power(tz);
+ estimate_controller_constants(tz, sustainable_power,
+ params->trip_switch_on,
+ control_temp, true);
+ }
+
err = ((s32)control_temp - (s32)current_temp);
err = int_to_frac(err);
@@ -139,7 +233,7 @@ static u32 pid_controller(struct thermal_zone_device *tz,
power_range = p + i + d;
/* 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);
@@ -417,19 +511,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;
+ unsigned long control_temp;
- if (!tz->tzp || !tz->tzp->sustainable_power) {
- dev_err(&tz->device,
- "power_allocator: missing sustainable_power\n");
+ if (!tz->tzp)
return -EINVAL;
- }
params = kzalloc(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,
@@ -438,29 +531,14 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
goto free;
}
- ret = tz->ops->get_trip_temp(tz, params->trip_switch_on,
- &switch_on_temp);
- if (ret)
- goto free;
-
ret = tz->ops->get_trip_temp(tz, params->trip_max_desired_temperature,
&control_temp);
if (ret)
goto free;
- temperature_threshold = control_temp - switch_on_temp;
-
- tz->tzp->k_po = tz->tzp->k_po ?:
- int_to_frac(tz->tzp->sustainable_power) / temperature_threshold;
- tz->tzp->k_pu = tz->tzp->k_pu ?:
- int_to_frac(2 * tz->tzp->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
- * leave them as they are.
- */
-
+ estimate_controller_constants(tz, tz->tzp->sustainable_power,
+ params->trip_switch_on, control_temp,
+ false);
reset_pid_controller(params);
tz->governor_data = params;
--
1.9.1
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 <[email protected]>
Cc: Eduardo Valentin <[email protected]>
Signed-off-by: Javi Merino <[email protected]>
---
Documentation/thermal/power_allocator.txt | 2 +-
drivers/thermal/power_allocator.c | 96 +++++++++++++++++--------------
2 files changed, 53 insertions(+), 45 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 eae8a5ae794a..2dfb8ade4d1b 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.
@@ -435,43 +439,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 to 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
+ * won'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)
@@ -500,11 +527,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)
@@ -523,31 +549,19 @@ 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_max_desired_temperature,
&control_temp);
- if (ret)
- goto free;
-
- estimate_controller_constants(tz, tz->tzp->sustainable_power,
- params->trip_switch_on, control_temp,
- false);
+ if (!ret)
+ estimate_controller_constants(tz, tz->tzp->sustainable_power,
+ params->trip_switch_on,
+ control_temp, false);
reset_pid_controller(params);
tz->governor_data = params;
return 0;
-
-free:
- kfree(params);
- return ret;
}
static void power_allocator_unbind(struct thermal_zone_device *tz)
@@ -578,13 +592,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
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.
Cc: Zhang Rui <[email protected]>
Cc: Eduardo Valentin <[email protected]>
Signed-off-by: Javi Merino <[email protected]>
---
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 2dfb8ade4d1b..85ce0aac9a41 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;
@@ -530,8 +533,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)
{
@@ -539,13 +541,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
struct power_allocator_params *params;
unsigned long control_temp;
- if (!tz->tzp)
- return -EINVAL;
-
params = kzalloc(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");
@@ -562,11 +571,24 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
tz->governor_data = params;
return 0;
+
+free_params:
+ kfree(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;
+ }
+
kfree(tz->governor_data);
tz->governor_data = NULL;
}
--
1.9.1
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.
Cc: Zhang Rui <[email protected]>
Cc: Eduardo Valentin <[email protected]>
Signed-off-by: Javi Merino <[email protected]>
---
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 85ce0aac9a41..e6fdcf24ba88 100644
--- a/drivers/thermal/power_allocator.c
+++ b/drivers/thermal/power_allocator.c
@@ -348,6 +348,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
On Wed, Aug 26, 2015 at 9:26 PM, Javi Merino <[email protected]> wrote:
> 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.
>
> Cc: Zhang Rui <[email protected]>
> Cc: Eduardo Valentin <[email protected]>
> Signed-off-by: Javi Merino <[email protected]>
> ---
>
> 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.
Why?
Just ignoring the thermal zone seems reasonable and simpler.
>
> 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 2dfb8ade4d1b..85ce0aac9a41 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;
> @@ -530,8 +533,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)
> {
> @@ -539,13 +541,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
> struct power_allocator_params *params;
> unsigned long control_temp;
>
> - if (!tz->tzp)
> - return -EINVAL;
> -
> params = kzalloc(sizeof(*params), GFP_KERNEL);
> if (!params)
> return -ENOMEM;
>
> + if (!tz->tzp) {
> + tz->tzp = kzalloc(sizeof(*tz->tzp), GFP_KERNEL);
Why bother to allocate this dummy struct?
Can't we just leave tz->tzp as NULL, and do a NULL check where needed?
> + 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");
>
> @@ -562,11 +571,24 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
> tz->governor_data = params;
>
> return 0;
> +
> +free_params:
> + kfree(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;
> + }
> +
> kfree(tz->governor_data);
> tz->governor_data = NULL;
> }
> --
> 1.9.1
>
On Fri, Aug 28, 2015 at 03:18:20AM +0100, Daniel Kurtz wrote:
> On Wed, Aug 26, 2015 at 9:26 PM, Javi Merino <[email protected]> wrote:
> > 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.
> >
> > Cc: Zhang Rui <[email protected]>
> > Cc: Eduardo Valentin <[email protected]>
> > Signed-off-by: Javi Merino <[email protected]>
> > ---
> >
> > 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.
>
> Why?
> Just ignoring the thermal zone seems reasonable and simpler.
>From the developer point of view, I agree that it's simpler. What I
want to avoid is the system integrator getting different behaviors
based on the presence of tzp when the thermal zone was created. If
the integrator was to configure this from userspace, they would only
be able to do so if the thermal zone was created with tzp. I don't
like this distinction, I prefer the consistency from the user point of
view that this patch gives.
Cheers,
Javi
> > 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 2dfb8ade4d1b..85ce0aac9a41 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;
> > @@ -530,8 +533,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)
> > {
> > @@ -539,13 +541,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
> > struct power_allocator_params *params;
> > unsigned long control_temp;
> >
> > - if (!tz->tzp)
> > - return -EINVAL;
> > -
> > params = kzalloc(sizeof(*params), GFP_KERNEL);
> > if (!params)
> > return -ENOMEM;
> >
> > + if (!tz->tzp) {
> > + tz->tzp = kzalloc(sizeof(*tz->tzp), GFP_KERNEL);
>
> Why bother to allocate this dummy struct?
> Can't we just leave tz->tzp as NULL, and do a NULL check where needed?
>
> > + 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");
> >
> > @@ -562,11 +571,24 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
> > tz->governor_data = params;
> >
> > return 0;
> > +
> > +free_params:
> > + kfree(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;
> > + }
> > +
> > kfree(tz->governor_data);
> > tz->governor_data = NULL;
> > }
> > --
> > 1.9.1
> >
>
Hi Javi,
On Sat, Aug 29, 2015 at 12:28 AM, Javi Merino <[email protected]> wrote:
> On Fri, Aug 28, 2015 at 03:18:20AM +0100, Daniel Kurtz wrote:
>> On Wed, Aug 26, 2015 at 9:26 PM, Javi Merino <[email protected]> wrote:
>> > 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.
>> >
>> > Cc: Zhang Rui <[email protected]>
>> > Cc: Eduardo Valentin <[email protected]>
>> > Signed-off-by: Javi Merino <[email protected]>
>> > ---
>> >
>> > 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.
>>
>> Why?
>> Just ignoring the thermal zone seems reasonable and simpler.
>
> From the developer point of view, I agree that it's simpler. What I
> want to avoid is the system integrator getting different behaviors
> based on the presence of tzp when the thermal zone was created. If
> the integrator was to configure this from userspace, they would only
> be able to do so if the thermal zone was created with tzp. I don't
> like this distinction, I prefer the consistency from the user point of
> view that this patch gives.
Ok, thanks for the answer.
Reviewed-by: Daniel Kurtz <[email protected]>
Thanks!
-Dan
>
> Cheers,
> Javi
>
>> > 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 2dfb8ade4d1b..85ce0aac9a41 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;
>> > @@ -530,8 +533,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)
>> > {
>> > @@ -539,13 +541,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
>> > struct power_allocator_params *params;
>> > unsigned long control_temp;
>> >
>> > - if (!tz->tzp)
>> > - return -EINVAL;
>> > -
>> > params = kzalloc(sizeof(*params), GFP_KERNEL);
>> > if (!params)
>> > return -ENOMEM;
>> >
>> > + if (!tz->tzp) {
>> > + tz->tzp = kzalloc(sizeof(*tz->tzp), GFP_KERNEL);
>>
>> Why bother to allocate this dummy struct?
>> Can't we just leave tz->tzp as NULL, and do a NULL check where needed?
>>
>> > + 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");
>> >
>> > @@ -562,11 +571,24 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
>> > tz->governor_data = params;
>> >
>> > return 0;
>> > +
>> > +free_params:
>> > + kfree(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;
>> > + }
>> > +
>> > kfree(tz->governor_data);
>> > tz->governor_data = NULL;
>> > }
>> > --
>> > 1.9.1
>> >
>>