* [PATCH] thermal: remove power allocator from list of default governors @ 2015-08-04 16:39 Dmitry Torokhov 2015-08-05 8:37 ` Javi Merino 0 siblings, 1 reply; 52+ messages in thread From: Dmitry Torokhov @ 2015-08-04 16:39 UTC (permalink / raw) To: Eduardo Valentin Cc: Zhang Rui, Srinivas Pandruvada, Javi Merino, Tushar Dave, Lan Tianyu, linux-pm, linux-kernel As it currently stands the power allocator governor can not handle thermal zones that are not specifically crafted and therefore can not be used as a default governor. Users need to explicitly enable this governor for thermal zones that do have enough information for its operation. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/thermal/Kconfig | 8 -------- 1 file changed, 8 deletions(-) diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index 0390044..34d05d3 100644 --- a/drivers/thermal/Kconfig +++ b/drivers/thermal/Kconfig @@ -82,14 +82,6 @@ config THERMAL_DEFAULT_GOV_USER_SPACE Select this if you want to let the user space manage the platform thermals. -config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR - bool "power_allocator" - select THERMAL_GOV_POWER_ALLOCATOR - help - Select this if you want to control temperature based on - system and device power allocation. This governor can only - operate on cooling devices that implement the power API. - endchoice config THERMAL_GOV_FAIR_SHARE -- 2.5.0.rc2.392.g76e840b -- Dmitry ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH] thermal: remove power allocator from list of default governors 2015-08-04 16:39 [PATCH] thermal: remove power allocator from list of default governors Dmitry Torokhov @ 2015-08-05 8:37 ` Javi Merino 2015-08-05 16:18 ` Srinivas Pandruvada 2015-08-05 16:35 ` Dmitry Torokhov 0 siblings, 2 replies; 52+ messages in thread From: Javi Merino @ 2015-08-05 8:37 UTC (permalink / raw) To: Dmitry Torokhov Cc: Eduardo Valentin, Zhang Rui, Srinivas Pandruvada, Tushar Dave, Lan Tianyu, Punit Agrawal, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org On Tue, Aug 04, 2015 at 05:39:21PM +0100, Dmitry Torokhov wrote: > As it currently stands the power allocator governor can not handle > thermal zones that are not specifically crafted and therefore can not be > used as a default governor. > > Users need to explicitly enable this governor for thermal zones that do > have enough information for its operation. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/thermal/Kconfig | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > index 0390044..34d05d3 100644 > --- a/drivers/thermal/Kconfig > +++ b/drivers/thermal/Kconfig > @@ -82,14 +82,6 @@ config THERMAL_DEFAULT_GOV_USER_SPACE > Select this if you want to let the user space manage the > platform thermals. > > -config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR > - bool "power_allocator" > - select THERMAL_GOV_POWER_ALLOCATOR > - help > - Select this if you want to control temperature based on > - system and device power allocation. This governor can only > - operate on cooling devices that implement the power API. > - Currently the only way we have for a thermal zone configured from device tree to use a governor from the kernel boot is by using THERMAL_DEFAULT_GOV_*. If we remove this option some devices won't have a workable thermal framework until userspace is up and running. Would you rather have the power allocator governor accept every thermal zone? Cheers, Javi ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] thermal: remove power allocator from list of default governors 2015-08-05 8:37 ` Javi Merino @ 2015-08-05 16:18 ` Srinivas Pandruvada 2015-08-05 16:35 ` Dmitry Torokhov 1 sibling, 0 replies; 52+ messages in thread From: Srinivas Pandruvada @ 2015-08-05 16:18 UTC (permalink / raw) To: Javi Merino Cc: Dmitry Torokhov, Eduardo Valentin, Zhang Rui, Tushar Dave, Lan Tianyu, Punit Agrawal, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org On Wed, 2015-08-05 at 09:37 +0100, Javi Merino wrote: > On Tue, Aug 04, 2015 at 05:39:21PM +0100, Dmitry Torokhov wrote: > > As it currently stands the power allocator governor can not handle > > thermal zones that are not specifically crafted and therefore can not be > > used as a default governor. > > > > Users need to explicitly enable this governor for thermal zones that do > > have enough information for its operation. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > drivers/thermal/Kconfig | 8 -------- > > 1 file changed, 8 deletions(-) > > > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > > index 0390044..34d05d3 100644 > > --- a/drivers/thermal/Kconfig > > +++ b/drivers/thermal/Kconfig > > @@ -82,14 +82,6 @@ config THERMAL_DEFAULT_GOV_USER_SPACE > > Select this if you want to let the user space manage the > > platform thermals. > > > > -config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR > > - bool "power_allocator" > > - select THERMAL_GOV_POWER_ALLOCATOR > > - help > > - Select this if you want to control temperature based on > > - system and device power allocation. This governor can only > > - operate on cooling devices that implement the power API. > > - > > Currently the only way we have for a thermal zone configured from > device tree to use a governor from the kernel boot is by using > THERMAL_DEFAULT_GOV_*. If we remove this option some devices won't > have a workable thermal framework until userspace is up and running. > > Would you rather have the power allocator governor accept every > thermal zone? May be allow to select when the binded cooling device can support this governor. Thanks, Srinivas > Cheers, > Javi > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] thermal: remove power allocator from list of default governors 2015-08-05 8:37 ` Javi Merino 2015-08-05 16:18 ` Srinivas Pandruvada @ 2015-08-05 16:35 ` Dmitry Torokhov 2015-08-05 18:49 ` Eduardo Valentin 1 sibling, 1 reply; 52+ messages in thread From: Dmitry Torokhov @ 2015-08-05 16:35 UTC (permalink / raw) To: Javi Merino Cc: Eduardo Valentin, Zhang Rui, Srinivas Pandruvada, Tushar Dave, Lan Tianyu, Punit Agrawal, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org On Wed, Aug 05, 2015 at 09:37:55AM +0100, Javi Merino wrote: > On Tue, Aug 04, 2015 at 05:39:21PM +0100, Dmitry Torokhov wrote: > > As it currently stands the power allocator governor can not handle > > thermal zones that are not specifically crafted and therefore can not be > > used as a default governor. > > > > Users need to explicitly enable this governor for thermal zones that do > > have enough information for its operation. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > drivers/thermal/Kconfig | 8 -------- > > 1 file changed, 8 deletions(-) > > > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > > index 0390044..34d05d3 100644 > > --- a/drivers/thermal/Kconfig > > +++ b/drivers/thermal/Kconfig > > @@ -82,14 +82,6 @@ config THERMAL_DEFAULT_GOV_USER_SPACE > > Select this if you want to let the user space manage the > > platform thermals. > > > > -config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR > > - bool "power_allocator" > > - select THERMAL_GOV_POWER_ALLOCATOR > > - help > > - Select this if you want to control temperature based on > > - system and device power allocation. This governor can only > > - operate on cooling devices that implement the power API. > > - > > Currently the only way we have for a thermal zone configured from > device tree to use a governor from the kernel boot is by using > THERMAL_DEFAULT_GOV_*. If we remove this option some devices won't > have a workable thermal framework until userspace is up and running. Why would step wise, or fair share be not workable (even if not optimal) thermal frameworks? It doe snot take that long to get to [early] userspace. Half of the boot time the thermal framework is not working anyway because half of the devices that can act as colling devices are not yet logically there. > > Would you rather have the power allocator governor accept every > thermal zone? If it is to be one of default governors then yes, it needs to be able to manage all thermal zones, the same way as the other 3 governors can, as far as I know. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] thermal: remove power allocator from list of default governors 2015-08-05 16:35 ` Dmitry Torokhov @ 2015-08-05 18:49 ` Eduardo Valentin 2015-08-06 8:30 ` Javi Merino 0 siblings, 1 reply; 52+ messages in thread From: Eduardo Valentin @ 2015-08-05 18:49 UTC (permalink / raw) To: Dmitry Torokhov Cc: Javi Merino, Zhang Rui, Srinivas Pandruvada, Tushar Dave, Lan Tianyu, Punit Agrawal, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org [-- Attachment #1: Type: text/plain, Size: 2777 bytes --] On Wed, Aug 05, 2015 at 09:35:39AM -0700, Dmitry Torokhov wrote: > On Wed, Aug 05, 2015 at 09:37:55AM +0100, Javi Merino wrote: > > On Tue, Aug 04, 2015 at 05:39:21PM +0100, Dmitry Torokhov wrote: > > > As it currently stands the power allocator governor can not handle > > > thermal zones that are not specifically crafted and therefore can not be > > > used as a default governor. > > > > > > Users need to explicitly enable this governor for thermal zones that do > > > have enough information for its operation. > > > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > --- > > > drivers/thermal/Kconfig | 8 -------- > > > 1 file changed, 8 deletions(-) > > > > > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > > > index 0390044..34d05d3 100644 > > > --- a/drivers/thermal/Kconfig > > > +++ b/drivers/thermal/Kconfig > > > @@ -82,14 +82,6 @@ config THERMAL_DEFAULT_GOV_USER_SPACE > > > Select this if you want to let the user space manage the > > > platform thermals. > > > > > > -config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR > > > - bool "power_allocator" > > > - select THERMAL_GOV_POWER_ALLOCATOR > > > - help > > > - Select this if you want to control temperature based on > > > - system and device power allocation. This governor can only > > > - operate on cooling devices that implement the power API. > > > - > > > > Currently the only way we have for a thermal zone configured from > > device tree to use a governor from the kernel boot is by using > > THERMAL_DEFAULT_GOV_*. If we remove this option some devices won't > > have a workable thermal framework until userspace is up and running. > > Why would step wise, or fair share be not workable (even if not optimal) > thermal frameworks? It doe snot take that long to get to [early] > userspace. Half of the boot time the thermal framework is not working > anyway because half of the devices that can act as colling devices are > not yet logically there. > > > > > Would you rather have the power allocator governor accept every > > thermal zone? > > If it is to be one of default governors then yes, it needs to be able to > manage all thermal zones, the same way as the other 3 governors can, as > far as I know. I believe fairshare also relies on platform data to be properly set, otherwise, it would not be functional. That's probably why I overlooked this point. That said, I would say, if we follow this logic, a similar patch is needed for fairshare. Javi, ideally, the governor would need to have a workable default settings for any thermal zone that miss the platform settings. Do you think it would be doable for power allocator? BR, > > Thanks. > > -- > Dmitry [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] thermal: remove power allocator from list of default governors 2015-08-05 18:49 ` Eduardo Valentin @ 2015-08-06 8:30 ` Javi Merino 2015-08-10 16:04 ` [PATCH 0/3] Let the power allocator thermal governor run on any thermal zone Javi Merino 0 siblings, 1 reply; 52+ messages in thread From: Javi Merino @ 2015-08-06 8:30 UTC (permalink / raw) To: Eduardo Valentin Cc: Dmitry Torokhov, Zhang Rui, Srinivas Pandruvada, Tushar Dave, Lan Tianyu, Punit Agrawal, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org On Wed, Aug 05, 2015 at 07:49:41PM +0100, Eduardo Valentin wrote: > On Wed, Aug 05, 2015 at 09:35:39AM -0700, Dmitry Torokhov wrote: > > On Wed, Aug 05, 2015 at 09:37:55AM +0100, Javi Merino wrote: > > > On Tue, Aug 04, 2015 at 05:39:21PM +0100, Dmitry Torokhov wrote: > > > > As it currently stands the power allocator governor can not handle > > > > thermal zones that are not specifically crafted and therefore can not be > > > > used as a default governor. > > > > > > > > Users need to explicitly enable this governor for thermal zones that do > > > > have enough information for its operation. > > > > > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > > --- > > > > drivers/thermal/Kconfig | 8 -------- > > > > 1 file changed, 8 deletions(-) > > > > > > > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > > > > index 0390044..34d05d3 100644 > > > > --- a/drivers/thermal/Kconfig > > > > +++ b/drivers/thermal/Kconfig > > > > @@ -82,14 +82,6 @@ config THERMAL_DEFAULT_GOV_USER_SPACE > > > > Select this if you want to let the user space manage the > > > > platform thermals. > > > > > > > > -config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR > > > > - bool "power_allocator" > > > > - select THERMAL_GOV_POWER_ALLOCATOR > > > > - help > > > > - Select this if you want to control temperature based on > > > > - system and device power allocation. This governor can only > > > > - operate on cooling devices that implement the power API. > > > > - > > > > > > Currently the only way we have for a thermal zone configured from > > > device tree to use a governor from the kernel boot is by using > > > THERMAL_DEFAULT_GOV_*. If we remove this option some devices won't > > > have a workable thermal framework until userspace is up and running. > > > > Why would step wise, or fair share be not workable (even if not optimal) > > thermal frameworks? It doe snot take that long to get to [early] > > userspace. Half of the boot time the thermal framework is not working > > anyway because half of the devices that can act as colling devices are > > not yet logically there. > > > > > > > > Would you rather have the power allocator governor accept every > > > thermal zone? > > > > If it is to be one of default governors then yes, it needs to be able to > > manage all thermal zones, the same way as the other 3 governors can, as > > far as I know. > > I believe fairshare also relies on platform data to be properly set, > otherwise, it would not be functional. That's probably why I overlooked > this point. That said, I would say, if we follow this logic, a similar > patch is needed for fairshare. > > Javi, ideally, the governor would need to have a workable > default settings for any thermal zone that miss the platform settings. > Do you think it would be doable for power allocator? Yes, we can come up with something that accepts any thermal zone. I'll send a patch soon. Cheers, Javi ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 0/3] Let the power allocator thermal governor run on any thermal zone 2015-08-06 8:30 ` Javi Merino @ 2015-08-10 16:04 ` Javi Merino 2015-08-10 16:04 ` [PATCH 1/3] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino ` (3 more replies) 0 siblings, 4 replies; 52+ messages in thread From: Javi Merino @ 2015-08-10 16:04 UTC (permalink / raw) To: linux-pm; +Cc: dmitry.torokhov, linux-kernel, punit.agrawal, Javi Merino 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 1/3] thermal: power_allocator: relax the requirement of a sustainable_power in tzp 2015-08-10 16:04 ` [PATCH 0/3] Let the power allocator thermal governor run on any thermal zone Javi Merino @ 2015-08-10 16:04 ` Javi Merino 2015-08-10 16:04 ` [PATCH 2/3] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino ` (2 subsequent siblings) 3 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-08-10 16:04 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, linux-kernel, punit.agrawal, Javi Merino, Zhang Rui, Eduardo Valentin The power allocator governor currently requires that a sustainable power is passed as part of the thermal zone's thermal zone parameters. If that parameter is not provided, it doesn't register with the thermal zone. While this parameter is strongly recommended for optimal performance, it doesn't need to be mandatory. Relax the requirement and allow the governor to bind to thermal zones that don't provide it by estimating it from the cooling devices' power model. Cc: Zhang Rui <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Signed-off-by: Javi Merino <javi.merino@arm.com> --- drivers/thermal/power_allocator.c | 62 +++++++++++++++++++++++++++++++++------ drivers/thermal/thermal_core.c | 28 ++++++++++++++++++ include/linux/thermal.h | 6 ++++ 3 files changed, 87 insertions(+), 9 deletions(-) diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c index 4672250b329f..cdcf5c6acc3c 100644 --- a/drivers/thermal/power_allocator.c +++ b/drivers/thermal/power_allocator.c @@ -73,6 +73,39 @@ struct power_allocator_params { }; /** + * estimate_sustainable_power() - Estimate the sustainable power of a thermal zone + * @tz: thermal zone we are operating in + * + * For thermal zones that don't provide a sustainable_power in their + * thermal_zone_params, estimate one. Calculate it using the minimum + * power of all the cooling devices as that gives a valid value that + * can give some degree of functionality. For optimal performance of + * this governor, provide a sustainable_power in the thermal zone's + * thermal_zone_params. + */ +static u32 estimate_sustainable_power(struct thermal_zone_device *tz) +{ + u32 sustainable_power = 0; + struct thermal_instance *instance; + struct power_allocator_params *params = tz->governor_data; + + list_for_each_entry(instance, &tz->thermal_instances, tz_node) { + struct thermal_cooling_device *cdev = instance->cdev; + u32 min_power; + + if (instance->trip != params->trip_max_desired_temperature) + continue; + + if (power_actor_get_min_power(cdev, tz, &min_power)) + continue; + + sustainable_power += min_power; + } + + return sustainable_power; +} + +/** * pid_controller() - PID controller * @tz: thermal zone we are operating in * @current_temp: the current temperature in millicelsius @@ -98,6 +131,7 @@ static u32 pid_controller(struct thermal_zone_device *tz, { s64 p, i, d, power_range; s32 err, max_power_frac; + u32 sustainable_power; struct power_allocator_params *params = tz->governor_data; max_power_frac = int_to_frac(max_allocatable_power); @@ -138,8 +172,11 @@ static u32 pid_controller(struct thermal_zone_device *tz, power_range = p + i + d; + sustainable_power = tz->tzp->sustainable_power ?: + estimate_sustainable_power(tz); + /* feed-forward the known sustainable dissipatable power */ - power_range = tz->tzp->sustainable_power + frac_to_int(power_range); + power_range = sustainable_power + frac_to_int(power_range); power_range = clamp(power_range, (s64)0, (s64)max_allocatable_power); @@ -412,18 +449,18 @@ static int power_allocator_bind(struct thermal_zone_device *tz) int ret; struct power_allocator_params *params; unsigned long switch_on_temp, control_temp; - u32 temperature_threshold; + u32 sustainable_power, temperature_threshold; - if (!tz->tzp || !tz->tzp->sustainable_power) { - dev_err(&tz->device, - "power_allocator: missing sustainable_power\n"); + if (!tz->tzp) return -EINVAL; - } params = devm_kzalloc(&tz->device, sizeof(*params), GFP_KERNEL); if (!params) return -ENOMEM; + if (!tz->tzp->sustainable_power) + dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n"); + ret = get_governor_trips(tz, params); if (ret) { dev_err(&tz->device, @@ -442,13 +479,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz) if (ret) goto free; + /* + * Provide an arbitrary sustainable_power to set the default + * values of k_po and k_pu. We can estimate sustainable_power + * at this point because no cooling devices have been + * registered yet. By providing an arbitrary value we get + * better defaults that setting k_po and k_pu to 0. + */ + sustainable_power = tz->tzp->sustainable_power ?: 2500; temperature_threshold = control_temp - switch_on_temp; tz->tzp->k_po = tz->tzp->k_po ?: - int_to_frac(tz->tzp->sustainable_power) / temperature_threshold; + int_to_frac(sustainable_power) / temperature_threshold; tz->tzp->k_pu = tz->tzp->k_pu ?: - int_to_frac(2 * tz->tzp->sustainable_power) / - temperature_threshold; + int_to_frac(2 * sustainable_power) / temperature_threshold; tz->tzp->k_i = tz->tzp->k_i ?: int_to_frac(10) / 1000; /* * The default for k_d and integral_cutoff is 0, so we can diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index 04659bfb888b..4040dd95bb19 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -997,6 +997,34 @@ int power_actor_get_max_power(struct thermal_cooling_device *cdev, } /** + * power_actor_get_min_power() - get the mainimum power that a cdev can consume + * @cdev: pointer to &thermal_cooling_device + * @tz: a valid thermal zone device pointer + * @min_power: pointer in which to store the minimum power + * + * Calculate the minimum power consumption in milliwats that the + * cooling device can currently consume and store it in @min_power. + * + * Return: 0 on success, -EINVAL if @cdev doesn't support the + * power_actor API or -E* on other error. + */ +int power_actor_get_min_power(struct thermal_cooling_device *cdev, + struct thermal_zone_device *tz, u32 *min_power) +{ + unsigned long max_state; + int ret; + + if (!cdev_is_power_actor(cdev)) + return -EINVAL; + + ret = cdev->ops->get_max_state(cdev, &max_state); + if (ret) + return ret; + + return cdev->ops->state2power(cdev, tz, max_state, min_power); +} + +/** * power_actor_set_power() - limit the maximum power that a cooling device can consume * @cdev: pointer to &thermal_cooling_device * @instance: thermal instance to update diff --git a/include/linux/thermal.h b/include/linux/thermal.h index 037e9df2f610..f99d934d373a 100644 --- a/include/linux/thermal.h +++ b/include/linux/thermal.h @@ -384,6 +384,8 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev) int power_actor_get_max_power(struct thermal_cooling_device *, struct thermal_zone_device *tz, u32 *max_power); +int power_actor_get_min_power(struct thermal_cooling_device *, + struct thermal_zone_device *tz, u32 *min_power); int power_actor_set_power(struct thermal_cooling_device *, struct thermal_instance *, u32); struct thermal_zone_device *thermal_zone_device_register(const char *, int, int, @@ -419,6 +421,10 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev) static inline int power_actor_get_max_power(struct thermal_cooling_device *cdev, struct thermal_zone_device *tz, u32 *max_power) { return 0; } +static inline int power_actor_get_min_power(struct thermal_cooling_device *cdev, + struct thermal_zone_device *tz, + u32 *min_power) +{ return -ENODEV; } static inline int power_actor_set_power(struct thermal_cooling_device *cdev, struct thermal_instance *tz, u32 power) { return 0; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH 2/3] thermal: power_allocator: relax the requirement of two passive trip points 2015-08-10 16:04 ` [PATCH 0/3] Let the power allocator thermal governor run on any thermal zone Javi Merino 2015-08-10 16:04 ` [PATCH 1/3] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino @ 2015-08-10 16:04 ` Javi Merino 2015-08-10 16:04 ` [PATCH 3/3] thermal: power_allocator: exit early if there are no cooling devices Javi Merino 2015-08-11 10:21 ` [PATCH v2 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino 3 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-08-10 16:04 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, linux-kernel, punit.agrawal, Javi Merino, Zhang Rui, Eduardo Valentin The power allocator governor currently requires that the thermal zone has at least two passive trip points. If there aren't, the governor refuses to bind to the thermal zone. This commit relaxes that requirement. Now the governor will bind to all thermal zones regardless of how many trip points they have. Cc: Zhang Rui <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Signed-off-by: Javi Merino <javi.merino@arm.com> --- Documentation/thermal/power_allocator.txt | 2 +- drivers/thermal/power_allocator.c | 91 +++++++++++++++++-------------- 2 files changed, 52 insertions(+), 41 deletions(-) diff --git a/Documentation/thermal/power_allocator.txt b/Documentation/thermal/power_allocator.txt index c3797b529991..a1ce2235f121 100644 --- a/Documentation/thermal/power_allocator.txt +++ b/Documentation/thermal/power_allocator.txt @@ -4,7 +4,7 @@ Power allocator governor tunables Trip points ----------- -The governor requires the following two passive trip points: +The governor works optimally with the following two passive trip points: 1. "switch on" trip point: temperature above which the governor control loop starts operating. This is the first passive trip diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c index cdcf5c6acc3c..cb3183ec364f 100644 --- a/drivers/thermal/power_allocator.c +++ b/drivers/thermal/power_allocator.c @@ -24,6 +24,8 @@ #include "thermal_core.h" +#define INVALID_TRIP -1 + #define FRAC_BITS 10 #define int_to_frac(x) ((x) << FRAC_BITS) #define frac_to_int(x) ((x) >> FRAC_BITS) @@ -61,6 +63,8 @@ static inline s64 div_frac(s64 x, s64 y) * Used to calculate the derivative term. * @trip_switch_on: first passive trip point of the thermal zone. The * governor switches on when this trip point is crossed. + * If the thermal zone only has one passive trip point, + * @trip_switch_on should be INVALID_TRIP. * @trip_max_desired_temperature: last passive trip point of the thermal * zone. The temperature we are * controlling for. @@ -372,43 +376,66 @@ unlock: return ret; } -static int get_governor_trips(struct thermal_zone_device *tz, - struct power_allocator_params *params) +/** + * get_governor_trips() - get the number of the two trip points that are key for this governor + * @tz: thermal zone to operate on + * @params: pointer the private data for this governor + * + * The power allocator governor works optimally with two trips points: + * a "switch on" trip point and a "maximum desired temperature". These + * are defined as the first and last passive trip points. + * + * If there is only one trip point, then that's considered to be the + * "maximum desired temperature" trip point and the governor is always + * on. If there are no passive or active trip points, then the + * governor won't do anything. In fact, its throttle function + * shouldn't be called at all. + */ +static void get_governor_trips(struct thermal_zone_device *tz, + struct power_allocator_params *params) { - int i, ret, last_passive; + int i, last_active, last_passive; bool found_first_passive; found_first_passive = false; - last_passive = -1; - ret = -EINVAL; + last_active = INVALID_TRIP; + last_passive = INVALID_TRIP; for (i = 0; i < tz->trips; i++) { enum thermal_trip_type type; + int ret; ret = tz->ops->get_trip_type(tz, i, &type); - if (ret) - return ret; + if (ret) { + dev_warn(&tz->device, + "Failed to get trip point %d type: %d\n", i, + ret); + continue; + } - if (!found_first_passive) { - if (type == THERMAL_TRIP_PASSIVE) { + if (type == THERMAL_TRIP_PASSIVE) { + if (!found_first_passive) { params->trip_switch_on = i; found_first_passive = true; + } else { + last_passive = i; } - } else if (type == THERMAL_TRIP_PASSIVE) { - last_passive = i; + } else if (type == THERMAL_TRIP_ACTIVE) { + last_active = i; } else { break; } } - if (last_passive != -1) { + if (last_passive != INVALID_TRIP) { params->trip_max_desired_temperature = last_passive; - ret = 0; + } else if (found_first_passive) { + params->trip_max_desired_temperature = params->trip_switch_on; + params->trip_switch_on = INVALID_TRIP; } else { - ret = -EINVAL; + params->trip_switch_on = INVALID_TRIP; + params->trip_max_desired_temperature = last_active; } - - return ret; } static void reset_pid_controller(struct power_allocator_params *params) @@ -437,11 +464,10 @@ static void allow_maximum_power(struct thermal_zone_device *tz) * power_allocator_bind() - bind the power_allocator governor to a thermal zone * @tz: thermal zone to bind it to * - * Check that the thermal zone is valid for this governor, that is, it - * has two thermal trips. If so, initialize the PID controller - * parameters and bind it to the thermal zone. + * Initialize the PID controller parameters and bind it to the thermal + * zone. * - * Return: 0 on success, -EINVAL if the trips were invalid or -ENOMEM + * Return: 0 on success, -EINVAL if the thermal zone doesn't have tzp or -ENOMEM * if we ran out of memory. */ static int power_allocator_bind(struct thermal_zone_device *tz) @@ -461,23 +487,18 @@ static int power_allocator_bind(struct thermal_zone_device *tz) if (!tz->tzp->sustainable_power) dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n"); - ret = get_governor_trips(tz, params); - if (ret) { - dev_err(&tz->device, - "thermal zone %s has wrong trip setup for power allocator\n", - tz->type); - goto free; - } + get_governor_trips(tz, params); ret = tz->ops->get_trip_temp(tz, params->trip_switch_on, &switch_on_temp); if (ret) - goto free; + switch_on_temp = 0; ret = tz->ops->get_trip_temp(tz, params->trip_max_desired_temperature, &control_temp); if (ret) - goto free; + /* Set some valid value to avoid division by zero below */ + control_temp = 70000; /* * Provide an arbitrary sustainable_power to set the default @@ -504,10 +525,6 @@ static int power_allocator_bind(struct thermal_zone_device *tz) tz->governor_data = params; return 0; - -free: - devm_kfree(&tz->device, params); - return ret; } static void power_allocator_unbind(struct thermal_zone_device *tz) @@ -538,13 +555,7 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip) ret = tz->ops->get_trip_temp(tz, params->trip_switch_on, &switch_on_temp); - if (ret) { - dev_warn(&tz->device, - "Failed to get switch on temperature: %d\n", ret); - return ret; - } - - if (current_temp < switch_on_temp) { + if ((!ret) && (current_temp < switch_on_temp)) { tz->passive = 0; reset_pid_controller(params); allow_maximum_power(tz); -- 1.9.1 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH 3/3] thermal: power_allocator: exit early if there are no cooling devices 2015-08-10 16:04 ` [PATCH 0/3] Let the power allocator thermal governor run on any thermal zone Javi Merino 2015-08-10 16:04 ` [PATCH 1/3] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino 2015-08-10 16:04 ` [PATCH 2/3] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino @ 2015-08-10 16:04 ` Javi Merino 2015-08-11 10:21 ` [PATCH v2 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino 3 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-08-10 16:04 UTC (permalink / raw) To: linux-pm; +Cc: dmitry.torokhov, linux-kernel, punit.agrawal, Javi Merino 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v2 0/4] Let the power allocator thermal governor run on any thermal zone 2015-08-10 16:04 ` [PATCH 0/3] Let the power allocator thermal governor run on any thermal zone Javi Merino ` (2 preceding siblings ...) 2015-08-10 16:04 ` [PATCH 3/3] thermal: power_allocator: exit early if there are no cooling devices Javi Merino @ 2015-08-11 10:21 ` Javi Merino 2015-08-11 10:21 ` [PATCH v2 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino ` (4 more replies) 3 siblings, 5 replies; 52+ messages in thread From: Javi Merino @ 2015-08-11 10:21 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, Javi Merino 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp 2015-08-11 10:21 ` [PATCH v2 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino @ 2015-08-11 10:21 ` Javi Merino 2015-08-11 13:42 ` Punit Agrawal 2015-08-12 11:05 ` Daniel Kurtz 2015-08-11 10:21 ` [PATCH v2 2/4] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino ` (3 subsequent siblings) 4 siblings, 2 replies; 52+ messages in thread From: Javi Merino @ 2015-08-11 10:21 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, Javi Merino, Zhang Rui, Eduardo Valentin The power allocator governor currently requires that a sustainable power is passed as part of the thermal zone's thermal zone parameters. If that parameter is not provided, it doesn't register with the thermal zone. While this parameter is strongly recommended for optimal performance, it doesn't need to be mandatory. Relax the requirement and allow the governor to bind to thermal zones that don't provide it by estimating it from the cooling devices' power model. Cc: Zhang Rui <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Signed-off-by: Javi Merino <javi.merino@arm.com> --- drivers/thermal/power_allocator.c | 62 +++++++++++++++++++++++++++++++++------ drivers/thermal/thermal_core.c | 28 ++++++++++++++++++ include/linux/thermal.h | 6 ++++ 3 files changed, 87 insertions(+), 9 deletions(-) diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c index 63a448f9d93b..f78836c2da26 100644 --- a/drivers/thermal/power_allocator.c +++ b/drivers/thermal/power_allocator.c @@ -73,6 +73,39 @@ struct power_allocator_params { }; /** + * estimate_sustainable_power() - Estimate the sustainable power of a thermal zone + * @tz: thermal zone we are operating in + * + * For thermal zones that don't provide a sustainable_power in their + * thermal_zone_params, estimate one. Calculate it using the minimum + * power of all the cooling devices as that gives a valid value that + * can give some degree of functionality. For optimal performance of + * this governor, provide a sustainable_power in the thermal zone's + * thermal_zone_params. + */ +static u32 estimate_sustainable_power(struct thermal_zone_device *tz) +{ + u32 sustainable_power = 0; + struct thermal_instance *instance; + struct power_allocator_params *params = tz->governor_data; + + list_for_each_entry(instance, &tz->thermal_instances, tz_node) { + struct thermal_cooling_device *cdev = instance->cdev; + u32 min_power; + + if (instance->trip != params->trip_max_desired_temperature) + continue; + + if (power_actor_get_min_power(cdev, tz, &min_power)) + continue; + + sustainable_power += min_power; + } + + return sustainable_power; +} + +/** * pid_controller() - PID controller * @tz: thermal zone we are operating in * @current_temp: the current temperature in millicelsius @@ -98,6 +131,7 @@ static u32 pid_controller(struct thermal_zone_device *tz, { s64 p, i, d, power_range; s32 err, max_power_frac; + u32 sustainable_power; struct power_allocator_params *params = tz->governor_data; max_power_frac = int_to_frac(max_allocatable_power); @@ -138,8 +172,11 @@ static u32 pid_controller(struct thermal_zone_device *tz, power_range = p + i + d; + sustainable_power = tz->tzp->sustainable_power ?: + estimate_sustainable_power(tz); + /* feed-forward the known sustainable dissipatable power */ - power_range = tz->tzp->sustainable_power + frac_to_int(power_range); + power_range = sustainable_power + frac_to_int(power_range); power_range = clamp(power_range, (s64)0, (s64)max_allocatable_power); @@ -418,18 +455,18 @@ static int power_allocator_bind(struct thermal_zone_device *tz) int ret; struct power_allocator_params *params; unsigned long switch_on_temp, control_temp; - u32 temperature_threshold; + u32 sustainable_power, temperature_threshold; - if (!tz->tzp || !tz->tzp->sustainable_power) { - dev_err(&tz->device, - "power_allocator: missing sustainable_power\n"); + if (!tz->tzp) return -EINVAL; - } params = devm_kzalloc(&tz->device, sizeof(*params), GFP_KERNEL); if (!params) return -ENOMEM; + if (!tz->tzp->sustainable_power) + dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n"); + ret = get_governor_trips(tz, params); if (ret) { dev_err(&tz->device, @@ -448,13 +485,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz) if (ret) goto free; + /* + * Provide an arbitrary sustainable_power to set the default + * values of k_po and k_pu. We can estimate sustainable_power + * at this point because no cooling devices have been + * registered yet. By providing an arbitrary value we get + * better defaults that setting k_po and k_pu to 0. + */ + sustainable_power = tz->tzp->sustainable_power ?: 2500; temperature_threshold = control_temp - switch_on_temp; tz->tzp->k_po = tz->tzp->k_po ?: - int_to_frac(tz->tzp->sustainable_power) / temperature_threshold; + int_to_frac(sustainable_power) / temperature_threshold; tz->tzp->k_pu = tz->tzp->k_pu ?: - int_to_frac(2 * tz->tzp->sustainable_power) / - temperature_threshold; + int_to_frac(2 * sustainable_power) / temperature_threshold; tz->tzp->k_i = tz->tzp->k_i ?: int_to_frac(10) / 1000; /* * The default for k_d and integral_cutoff is 0, so we can diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index 4ca211be4c0f..d26bc9e6f936 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -997,6 +997,34 @@ int power_actor_get_max_power(struct thermal_cooling_device *cdev, } /** + * power_actor_get_min_power() - get the mainimum power that a cdev can consume + * @cdev: pointer to &thermal_cooling_device + * @tz: a valid thermal zone device pointer + * @min_power: pointer in which to store the minimum power + * + * Calculate the minimum power consumption in milliwats that the + * cooling device can currently consume and store it in @min_power. + * + * Return: 0 on success, -EINVAL if @cdev doesn't support the + * power_actor API or -E* on other error. + */ +int power_actor_get_min_power(struct thermal_cooling_device *cdev, + struct thermal_zone_device *tz, u32 *min_power) +{ + unsigned long max_state; + int ret; + + if (!cdev_is_power_actor(cdev)) + return -EINVAL; + + ret = cdev->ops->get_max_state(cdev, &max_state); + if (ret) + return ret; + + return cdev->ops->state2power(cdev, tz, max_state, min_power); +} + +/** * power_actor_set_power() - limit the maximum power that a cooling device can consume * @cdev: pointer to &thermal_cooling_device * @instance: thermal instance to update diff --git a/include/linux/thermal.h b/include/linux/thermal.h index 037e9df2f610..f99d934d373a 100644 --- a/include/linux/thermal.h +++ b/include/linux/thermal.h @@ -384,6 +384,8 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev) int power_actor_get_max_power(struct thermal_cooling_device *, struct thermal_zone_device *tz, u32 *max_power); +int power_actor_get_min_power(struct thermal_cooling_device *, + struct thermal_zone_device *tz, u32 *min_power); int power_actor_set_power(struct thermal_cooling_device *, struct thermal_instance *, u32); struct thermal_zone_device *thermal_zone_device_register(const char *, int, int, @@ -419,6 +421,10 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev) static inline int power_actor_get_max_power(struct thermal_cooling_device *cdev, struct thermal_zone_device *tz, u32 *max_power) { return 0; } +static inline int power_actor_get_min_power(struct thermal_cooling_device *cdev, + struct thermal_zone_device *tz, + u32 *min_power) +{ return -ENODEV; } static inline int power_actor_set_power(struct thermal_cooling_device *cdev, struct thermal_instance *tz, u32 power) { return 0; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v2 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp 2015-08-11 10:21 ` [PATCH v2 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino @ 2015-08-11 13:42 ` Punit Agrawal 2015-08-11 17:37 ` Javi Merino 2015-08-12 11:05 ` Daniel Kurtz 1 sibling, 1 reply; 52+ messages in thread From: Punit Agrawal @ 2015-08-11 13:42 UTC (permalink / raw) To: Javi Merino Cc: linux-pm, dmitry.torokhov, cywang, linux-kernel, Zhang Rui, Eduardo Valentin Hi Javi, A few nitpicks and a comment below. Javi Merino <javi.merino@arm.com> writes: > The power allocator governor currently requires that a sustainable power > is passed as part of the thermal zone's thermal zone parameters. If > that parameter is not provided, it doesn't register with the thermal > zone. > > While this parameter is strongly recommended for optimal performance, it > doesn't need to be mandatory. Relax the requirement and allow the > governor to bind to thermal zones that don't provide it by estimating it > from the cooling devices' power model. > > Cc: Zhang Rui <rui.zhang@intel.com> > Cc: Eduardo Valentin <edubezval@gmail.com> > Signed-off-by: Javi Merino <javi.merino@arm.com> > --- > drivers/thermal/power_allocator.c | 62 +++++++++++++++++++++++++++++++++------ > drivers/thermal/thermal_core.c | 28 ++++++++++++++++++ > include/linux/thermal.h | 6 ++++ > 3 files changed, 87 insertions(+), 9 deletions(-) > > diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c > index 63a448f9d93b..f78836c2da26 100644 > --- a/drivers/thermal/power_allocator.c > +++ b/drivers/thermal/power_allocator.c > @@ -73,6 +73,39 @@ struct power_allocator_params { > }; > > /** > + * estimate_sustainable_power() - Estimate the sustainable power of a thermal zone > + * @tz: thermal zone we are operating in > + * > + * For thermal zones that don't provide a sustainable_power in their > + * thermal_zone_params, estimate one. Calculate it using the minimum > + * power of all the cooling devices as that gives a valid value that > + * can give some degree of functionality. For optimal performance of > + * this governor, provide a sustainable_power in the thermal zone's > + * thermal_zone_params. > + */ > +static u32 estimate_sustainable_power(struct thermal_zone_device *tz) > +{ > + u32 sustainable_power = 0; > + struct thermal_instance *instance; > + struct power_allocator_params *params = tz->governor_data; > + > + list_for_each_entry(instance, &tz->thermal_instances, tz_node) { > + struct thermal_cooling_device *cdev = instance->cdev; > + u32 min_power; > + > + if (instance->trip != params->trip_max_desired_temperature) > + continue; > + > + if (power_actor_get_min_power(cdev, tz, &min_power)) > + continue; > + > + sustainable_power += min_power; > + } > + > + return sustainable_power; > +} > + > +/** > * pid_controller() - PID controller > * @tz: thermal zone we are operating in > * @current_temp: the current temperature in millicelsius > @@ -98,6 +131,7 @@ static u32 pid_controller(struct thermal_zone_device *tz, > { > s64 p, i, d, power_range; > s32 err, max_power_frac; > + u32 sustainable_power; > struct power_allocator_params *params = tz->governor_data; > > max_power_frac = int_to_frac(max_allocatable_power); > @@ -138,8 +172,11 @@ static u32 pid_controller(struct thermal_zone_device *tz, > > power_range = p + i + d; > > + sustainable_power = tz->tzp->sustainable_power ?: > + estimate_sustainable_power(tz); > + > /* feed-forward the known sustainable dissipatable power */ > - power_range = tz->tzp->sustainable_power + frac_to_int(power_range); > + power_range = sustainable_power + frac_to_int(power_range); > > power_range = clamp(power_range, (s64)0, (s64)max_allocatable_power); > > @@ -418,18 +455,18 @@ static int power_allocator_bind(struct thermal_zone_device *tz) > int ret; > struct power_allocator_params *params; > unsigned long switch_on_temp, control_temp; > - u32 temperature_threshold; > + u32 sustainable_power, temperature_threshold; > > - if (!tz->tzp || !tz->tzp->sustainable_power) { > - dev_err(&tz->device, > - "power_allocator: missing sustainable_power\n"); > + if (!tz->tzp) > return -EINVAL; > - } > > params = devm_kzalloc(&tz->device, sizeof(*params), GFP_KERNEL); > if (!params) > return -ENOMEM; > > + if (!tz->tzp->sustainable_power) > + dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n"); > + > ret = get_governor_trips(tz, params); > if (ret) { > dev_err(&tz->device, > @@ -448,13 +485,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz) > if (ret) > goto free; > > + /* > + * Provide an arbitrary sustainable_power to set the default > + * values of k_po and k_pu. We can estimate sustainable_power ^ can not > + * at this point because no cooling devices have been > + * registered yet. By providing an arbitrary value we get > + * better defaults that setting k_po and k_pu to 0. ^ than > + */ > + sustainable_power = tz->tzp->sustainable_power ?: 2500; > temperature_threshold = control_temp - switch_on_temp; > > tz->tzp->k_po = tz->tzp->k_po ?: > - int_to_frac(tz->tzp->sustainable_power) / temperature_threshold; > + int_to_frac(sustainable_power) / temperature_threshold; > tz->tzp->k_pu = tz->tzp->k_pu ?: > - int_to_frac(2 * tz->tzp->sustainable_power) / > - temperature_threshold; > + int_to_frac(2 * sustainable_power) / temperature_threshold; As we are being conservative with our estimation of sustainable power (sum of mins) when it is not explicitly specified, should we be conservative here and let the proportional terms, k_po and k_pu be zero as well? > tz->tzp->k_i = tz->tzp->k_i ?: int_to_frac(10) / 1000; > /* > * The default for k_d and integral_cutoff is 0, so we can > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index 4ca211be4c0f..d26bc9e6f936 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -997,6 +997,34 @@ int power_actor_get_max_power(struct thermal_cooling_device *cdev, > } > > /** > + * power_actor_get_min_power() - get the mainimum power that a cdev can consume > + * @cdev: pointer to &thermal_cooling_device > + * @tz: a valid thermal zone device pointer > + * @min_power: pointer in which to store the minimum power > + * > + * Calculate the minimum power consumption in milliwats that the > + * cooling device can currently consume and store it in @min_power. > + * > + * Return: 0 on success, -EINVAL if @cdev doesn't support the > + * power_actor API or -E* on other error. > + */ > +int power_actor_get_min_power(struct thermal_cooling_device *cdev, > + struct thermal_zone_device *tz, u32 *min_power) > +{ > + unsigned long max_state; > + int ret; > + > + if (!cdev_is_power_actor(cdev)) > + return -EINVAL; > + > + ret = cdev->ops->get_max_state(cdev, &max_state); > + if (ret) > + return ret; > + > + return cdev->ops->state2power(cdev, tz, max_state, min_power); > +} > + > +/** > * power_actor_set_power() - limit the maximum power that a cooling device can consume > * @cdev: pointer to &thermal_cooling_device > * @instance: thermal instance to update > diff --git a/include/linux/thermal.h b/include/linux/thermal.h > index 037e9df2f610..f99d934d373a 100644 > --- a/include/linux/thermal.h > +++ b/include/linux/thermal.h > @@ -384,6 +384,8 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev) > > int power_actor_get_max_power(struct thermal_cooling_device *, > struct thermal_zone_device *tz, u32 *max_power); > +int power_actor_get_min_power(struct thermal_cooling_device *, > + struct thermal_zone_device *tz, u32 *min_power); > int power_actor_set_power(struct thermal_cooling_device *, > struct thermal_instance *, u32); > struct thermal_zone_device *thermal_zone_device_register(const char *, int, int, > @@ -419,6 +421,10 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev) > static inline int power_actor_get_max_power(struct thermal_cooling_device *cdev, > struct thermal_zone_device *tz, u32 *max_power) > { return 0; } > +static inline int power_actor_get_min_power(struct thermal_cooling_device *cdev, > + struct thermal_zone_device *tz, > + u32 *min_power) > +{ return -ENODEV; } Perhaps return 0 like power_actor_get_max_power just above for consistency. > static inline int power_actor_set_power(struct thermal_cooling_device *cdev, > struct thermal_instance *tz, u32 power) > { return 0; } ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp 2015-08-11 13:42 ` Punit Agrawal @ 2015-08-11 17:37 ` Javi Merino 0 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-08-11 17:37 UTC (permalink / raw) To: Punit Agrawal Cc: linux-pm@vger.kernel.org, dmitry.torokhov@gmail.com, cywang@chromium.org, linux-kernel@vger.kernel.org, Zhang Rui, Eduardo Valentin On Tue, Aug 11, 2015 at 02:42:20PM +0100, Punit Agrawal wrote: > Hi Javi, > > A few nitpicks and a comment below. > > Javi Merino <javi.merino@arm.com> writes: > > > The power allocator governor currently requires that a sustainable power > > is passed as part of the thermal zone's thermal zone parameters. If > > that parameter is not provided, it doesn't register with the thermal > > zone. > > > > While this parameter is strongly recommended for optimal performance, it > > doesn't need to be mandatory. Relax the requirement and allow the > > governor to bind to thermal zones that don't provide it by estimating it > > from the cooling devices' power model. > > > > Cc: Zhang Rui <rui.zhang@intel.com> > > Cc: Eduardo Valentin <edubezval@gmail.com> > > Signed-off-by: Javi Merino <javi.merino@arm.com> > > --- > > drivers/thermal/power_allocator.c | 62 +++++++++++++++++++++++++++++++++------ > > drivers/thermal/thermal_core.c | 28 ++++++++++++++++++ > > include/linux/thermal.h | 6 ++++ > > 3 files changed, 87 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c > > index 63a448f9d93b..f78836c2da26 100644 > > --- a/drivers/thermal/power_allocator.c > > +++ b/drivers/thermal/power_allocator.c > > @@ -73,6 +73,39 @@ struct power_allocator_params { > > }; > > > > /** > > + * estimate_sustainable_power() - Estimate the sustainable power of a thermal zone > > + * @tz: thermal zone we are operating in > > + * > > + * For thermal zones that don't provide a sustainable_power in their > > + * thermal_zone_params, estimate one. Calculate it using the minimum > > + * power of all the cooling devices as that gives a valid value that > > + * can give some degree of functionality. For optimal performance of > > + * this governor, provide a sustainable_power in the thermal zone's > > + * thermal_zone_params. > > + */ > > +static u32 estimate_sustainable_power(struct thermal_zone_device *tz) > > +{ > > + u32 sustainable_power = 0; > > + struct thermal_instance *instance; > > + struct power_allocator_params *params = tz->governor_data; > > + > > + list_for_each_entry(instance, &tz->thermal_instances, tz_node) { > > + struct thermal_cooling_device *cdev = instance->cdev; > > + u32 min_power; > > + > > + if (instance->trip != params->trip_max_desired_temperature) > > + continue; > > + > > + if (power_actor_get_min_power(cdev, tz, &min_power)) > > + continue; > > + > > + sustainable_power += min_power; > > + } > > + > > + return sustainable_power; > > +} > > + > > +/** > > * pid_controller() - PID controller > > * @tz: thermal zone we are operating in > > * @current_temp: the current temperature in millicelsius > > @@ -98,6 +131,7 @@ static u32 pid_controller(struct thermal_zone_device *tz, > > { > > s64 p, i, d, power_range; > > s32 err, max_power_frac; > > + u32 sustainable_power; > > struct power_allocator_params *params = tz->governor_data; > > > > max_power_frac = int_to_frac(max_allocatable_power); > > @@ -138,8 +172,11 @@ static u32 pid_controller(struct thermal_zone_device *tz, > > > > power_range = p + i + d; > > > > + sustainable_power = tz->tzp->sustainable_power ?: > > + estimate_sustainable_power(tz); > > + > > /* feed-forward the known sustainable dissipatable power */ > > - power_range = tz->tzp->sustainable_power + frac_to_int(power_range); > > + power_range = sustainable_power + frac_to_int(power_range); > > > > power_range = clamp(power_range, (s64)0, (s64)max_allocatable_power); > > > > @@ -418,18 +455,18 @@ static int power_allocator_bind(struct thermal_zone_device *tz) > > int ret; > > struct power_allocator_params *params; > > unsigned long switch_on_temp, control_temp; > > - u32 temperature_threshold; > > + u32 sustainable_power, temperature_threshold; > > > > - if (!tz->tzp || !tz->tzp->sustainable_power) { > > - dev_err(&tz->device, > > - "power_allocator: missing sustainable_power\n"); > > + if (!tz->tzp) > > return -EINVAL; > > - } > > > > params = devm_kzalloc(&tz->device, sizeof(*params), GFP_KERNEL); > > if (!params) > > return -ENOMEM; > > > > + if (!tz->tzp->sustainable_power) > > + dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n"); > > + > > ret = get_governor_trips(tz, params); > > if (ret) { > > dev_err(&tz->device, > > @@ -448,13 +485,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz) > > if (ret) > > goto free; > > > > + /* > > + * Provide an arbitrary sustainable_power to set the default > > + * values of k_po and k_pu. We can estimate sustainable_power > ^ > can not > > + * at this point because no cooling devices have been > > + * registered yet. By providing an arbitrary value we get > > + * better defaults that setting k_po and k_pu to 0. > ^ > than Fixed both. > > + */ > > + sustainable_power = tz->tzp->sustainable_power ?: 2500; > > temperature_threshold = control_temp - switch_on_temp; > > > > tz->tzp->k_po = tz->tzp->k_po ?: > > - int_to_frac(tz->tzp->sustainable_power) / temperature_threshold; > > + int_to_frac(sustainable_power) / temperature_threshold; > > tz->tzp->k_pu = tz->tzp->k_pu ?: > > - int_to_frac(2 * tz->tzp->sustainable_power) / > > - temperature_threshold; > > + int_to_frac(2 * sustainable_power) / temperature_threshold; > > As we are being conservative with our estimation of sustainable power > (sum of mins) when it is not explicitly specified, should we be > conservative here and let the proportional terms, k_po and k_pu be zero > as well? Yes, we could do. If we set it to zero, all cooling devices will be set to the maximum cooling state when the governor is active. If we set it to a valid(ish) value, it will do a bit of thermal control. It's not perfect but I think it's better to have a bad estimate than no estimate. > > tz->tzp->k_i = tz->tzp->k_i ?: int_to_frac(10) / 1000; > > /* > > * The default for k_d and integral_cutoff is 0, so we can > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > > index 4ca211be4c0f..d26bc9e6f936 100644 > > --- a/drivers/thermal/thermal_core.c > > +++ b/drivers/thermal/thermal_core.c > > @@ -997,6 +997,34 @@ int power_actor_get_max_power(struct thermal_cooling_device *cdev, > > } > > > > /** > > + * power_actor_get_min_power() - get the mainimum power that a cdev can consume > > + * @cdev: pointer to &thermal_cooling_device > > + * @tz: a valid thermal zone device pointer > > + * @min_power: pointer in which to store the minimum power > > + * > > + * Calculate the minimum power consumption in milliwats that the > > + * cooling device can currently consume and store it in @min_power. > > + * > > + * Return: 0 on success, -EINVAL if @cdev doesn't support the > > + * power_actor API or -E* on other error. > > + */ > > +int power_actor_get_min_power(struct thermal_cooling_device *cdev, > > + struct thermal_zone_device *tz, u32 *min_power) > > +{ > > + unsigned long max_state; > > + int ret; > > + > > + if (!cdev_is_power_actor(cdev)) > > + return -EINVAL; > > + > > + ret = cdev->ops->get_max_state(cdev, &max_state); > > + if (ret) > > + return ret; > > + > > + return cdev->ops->state2power(cdev, tz, max_state, min_power); > > +} > > + > > +/** > > * power_actor_set_power() - limit the maximum power that a cooling device can consume > > * @cdev: pointer to &thermal_cooling_device > > * @instance: thermal instance to update > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h > > index 037e9df2f610..f99d934d373a 100644 > > --- a/include/linux/thermal.h > > +++ b/include/linux/thermal.h > > @@ -384,6 +384,8 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev) > > > > int power_actor_get_max_power(struct thermal_cooling_device *, > > struct thermal_zone_device *tz, u32 *max_power); > > +int power_actor_get_min_power(struct thermal_cooling_device *, > > + struct thermal_zone_device *tz, u32 *min_power); > > int power_actor_set_power(struct thermal_cooling_device *, > > struct thermal_instance *, u32); > > struct thermal_zone_device *thermal_zone_device_register(const char *, int, int, > > @@ -419,6 +421,10 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev) > > static inline int power_actor_get_max_power(struct thermal_cooling_device *cdev, > > struct thermal_zone_device *tz, u32 *max_power) > > { return 0; } > > +static inline int power_actor_get_min_power(struct thermal_cooling_device *cdev, > > + struct thermal_zone_device *tz, > > + u32 *min_power) > > +{ return -ENODEV; } > > Perhaps return 0 like power_actor_get_max_power just above for > consistency. No, return 0 is not a good idea. If you return 0, you're telling the calling function that everything went all right and you've put the minimum power in @min_power. What we should do is fix power_actor_get_max_power() and power_actor_set_power() to return an error value if !CONFIG_THERMAL. Cheers, Javi ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp 2015-08-11 10:21 ` [PATCH v2 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino 2015-08-11 13:42 ` Punit Agrawal @ 2015-08-12 11:05 ` Daniel Kurtz 1 sibling, 0 replies; 52+ messages in thread From: Daniel Kurtz @ 2015-08-12 11:05 UTC (permalink / raw) To: Javi Merino Cc: linux-pm, Dmitry Torokhov, Chung-yih Wang, linux-kernel@vger.kernel.org, Punit Agrawal, Zhang Rui, Eduardo Valentin Hi Javi, One Tiny nit below... On Tue, Aug 11, 2015 at 6:21 PM, Javi Merino <javi.merino@arm.com> wrote: > The power allocator governor currently requires that a sustainable power > is passed as part of the thermal zone's thermal zone parameters. If > that parameter is not provided, it doesn't register with the thermal > zone. > > While this parameter is strongly recommended for optimal performance, it > doesn't need to be mandatory. Relax the requirement and allow the > governor to bind to thermal zones that don't provide it by estimating it > from the cooling devices' power model. > > Cc: Zhang Rui <rui.zhang@intel.com> > Cc: Eduardo Valentin <edubezval@gmail.com> > Signed-off-by: Javi Merino <javi.merino@arm.com> > --- [snip] > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -997,6 +997,34 @@ int power_actor_get_max_power(struct thermal_cooling_device *cdev, > } > > /** > + * power_actor_get_min_power() - get the mainimum power that a cdev can consume > + * @cdev: pointer to &thermal_cooling_device > + * @tz: a valid thermal zone device pointer > + * @min_power: pointer in which to store the minimum power > + * > + * Calculate the minimum power consumption in milliwats that the ^ milliwatts Thanks, -Dan ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 2/4] thermal: power_allocator: relax the requirement of two passive trip points 2015-08-11 10:21 ` [PATCH v2 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino 2015-08-11 10:21 ` [PATCH v2 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino @ 2015-08-11 10:21 ` Javi Merino 2015-08-12 11:13 ` Daniel Kurtz 2015-08-11 10:21 ` [PATCH v2 3/4] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino ` (2 subsequent siblings) 4 siblings, 1 reply; 52+ messages in thread From: Javi Merino @ 2015-08-11 10:21 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, Javi Merino, Zhang Rui, Eduardo Valentin The power allocator governor currently requires that the thermal zone has at least two passive trip points. If there aren't, the governor refuses to bind to the thermal zone. This commit relaxes that requirement. Now the governor will bind to all thermal zones regardless of how many trip points they have. Cc: Zhang Rui <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Signed-off-by: Javi Merino <javi.merino@arm.com> --- Documentation/thermal/power_allocator.txt | 2 +- drivers/thermal/power_allocator.c | 91 +++++++++++++++++-------------- 2 files changed, 52 insertions(+), 41 deletions(-) diff --git a/Documentation/thermal/power_allocator.txt b/Documentation/thermal/power_allocator.txt index c3797b529991..a1ce2235f121 100644 --- a/Documentation/thermal/power_allocator.txt +++ b/Documentation/thermal/power_allocator.txt @@ -4,7 +4,7 @@ Power allocator governor tunables Trip points ----------- -The governor requires the following two passive trip points: +The governor works optimally with the following two passive trip points: 1. "switch on" trip point: temperature above which the governor control loop starts operating. This is the first passive trip diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c index f78836c2da26..257c9af20f22 100644 --- a/drivers/thermal/power_allocator.c +++ b/drivers/thermal/power_allocator.c @@ -24,6 +24,8 @@ #include "thermal_core.h" +#define INVALID_TRIP -1 + #define FRAC_BITS 10 #define int_to_frac(x) ((x) << FRAC_BITS) #define frac_to_int(x) ((x) >> FRAC_BITS) @@ -61,6 +63,8 @@ static inline s64 div_frac(s64 x, s64 y) * Used to calculate the derivative term. * @trip_switch_on: first passive trip point of the thermal zone. The * governor switches on when this trip point is crossed. + * If the thermal zone only has one passive trip point, + * @trip_switch_on should be INVALID_TRIP. * @trip_max_desired_temperature: last passive trip point of the thermal * zone. The temperature we are * controlling for. @@ -378,43 +382,66 @@ unlock: return ret; } -static int get_governor_trips(struct thermal_zone_device *tz, - struct power_allocator_params *params) +/** + * get_governor_trips() - get the number of the two trip points that are key for this governor + * @tz: thermal zone to operate on + * @params: pointer the private data for this governor + * + * The power allocator governor works optimally with two trips points: + * a "switch on" trip point and a "maximum desired temperature". These + * are defined as the first and last passive trip points. + * + * If there is only one trip point, then that's considered to be the + * "maximum desired temperature" trip point and the governor is always + * on. If there are no passive or active trip points, then the + * governor won't do anything. In fact, its throttle function + * shouldn't be called at all. + */ +static void get_governor_trips(struct thermal_zone_device *tz, + struct power_allocator_params *params) { - int i, ret, last_passive; + int i, last_active, last_passive; bool found_first_passive; found_first_passive = false; - last_passive = -1; - ret = -EINVAL; + last_active = INVALID_TRIP; + last_passive = INVALID_TRIP; for (i = 0; i < tz->trips; i++) { enum thermal_trip_type type; + int ret; ret = tz->ops->get_trip_type(tz, i, &type); - if (ret) - return ret; + if (ret) { + dev_warn(&tz->device, + "Failed to get trip point %d type: %d\n", i, + ret); + continue; + } - if (!found_first_passive) { - if (type == THERMAL_TRIP_PASSIVE) { + if (type == THERMAL_TRIP_PASSIVE) { + if (!found_first_passive) { params->trip_switch_on = i; found_first_passive = true; + } else { + last_passive = i; } - } else if (type == THERMAL_TRIP_PASSIVE) { - last_passive = i; + } else if (type == THERMAL_TRIP_ACTIVE) { + last_active = i; } else { break; } } - if (last_passive != -1) { + if (last_passive != INVALID_TRIP) { params->trip_max_desired_temperature = last_passive; - ret = 0; + } else if (found_first_passive) { + params->trip_max_desired_temperature = params->trip_switch_on; + params->trip_switch_on = INVALID_TRIP; } else { - ret = -EINVAL; + params->trip_switch_on = INVALID_TRIP; + params->trip_max_desired_temperature = last_active; } - - return ret; } static void reset_pid_controller(struct power_allocator_params *params) @@ -443,11 +470,10 @@ static void allow_maximum_power(struct thermal_zone_device *tz) * power_allocator_bind() - bind the power_allocator governor to a thermal zone * @tz: thermal zone to bind it to * - * Check that the thermal zone is valid for this governor, that is, it - * has two thermal trips. If so, initialize the PID controller - * parameters and bind it to the thermal zone. + * Initialize the PID controller parameters and bind it to the thermal + * zone. * - * Return: 0 on success, -EINVAL if the trips were invalid or -ENOMEM + * Return: 0 on success, -EINVAL if the thermal zone doesn't have tzp or -ENOMEM * if we ran out of memory. */ static int power_allocator_bind(struct thermal_zone_device *tz) @@ -467,23 +493,18 @@ static int power_allocator_bind(struct thermal_zone_device *tz) if (!tz->tzp->sustainable_power) dev_warn(&tz->device, "power_allocator: sustainable_power will be estimated\n"); - ret = get_governor_trips(tz, params); - if (ret) { - dev_err(&tz->device, - "thermal zone %s has wrong trip setup for power allocator\n", - tz->type); - goto free; - } + get_governor_trips(tz, params); ret = tz->ops->get_trip_temp(tz, params->trip_switch_on, &switch_on_temp); if (ret) - goto free; + switch_on_temp = 0; ret = tz->ops->get_trip_temp(tz, params->trip_max_desired_temperature, &control_temp); if (ret) - goto free; + /* Set some valid value to avoid division by zero below */ + control_temp = 70000; /* * Provide an arbitrary sustainable_power to set the default @@ -510,10 +531,6 @@ static int power_allocator_bind(struct thermal_zone_device *tz) tz->governor_data = params; return 0; - -free: - devm_kfree(&tz->device, params); - return ret; } static void power_allocator_unbind(struct thermal_zone_device *tz) @@ -544,13 +561,7 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip) ret = tz->ops->get_trip_temp(tz, params->trip_switch_on, &switch_on_temp); - if (ret) { - dev_warn(&tz->device, - "Failed to get switch on temperature: %d\n", ret); - return ret; - } - - if (current_temp < switch_on_temp) { + if ((!ret) && (current_temp < switch_on_temp)) { tz->passive = 0; reset_pid_controller(params); allow_maximum_power(tz); -- 1.9.1 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/4] thermal: power_allocator: relax the requirement of two passive trip points 2015-08-11 10:21 ` [PATCH v2 2/4] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino @ 2015-08-12 11:13 ` Daniel Kurtz 2015-08-12 16:46 ` Javi Merino 0 siblings, 1 reply; 52+ messages in thread From: Daniel Kurtz @ 2015-08-12 11:13 UTC (permalink / raw) To: Javi Merino Cc: linux-pm, Dmitry Torokhov, Chung-yih Wang, linux-kernel@vger.kernel.org, Punit Agrawal, Zhang Rui, Eduardo Valentin Hi Javi, tiny nits again... On Tue, Aug 11, 2015 at 6:21 PM, Javi Merino <javi.merino@arm.com> wrote: > The power allocator governor currently requires that the thermal zone > has at least two passive trip points. If there aren't, the governor > refuses to bind to the thermal zone. > > This commit relaxes that requirement. Now the governor will bind to all > thermal zones regardless of how many trip points they have. > > Cc: Zhang Rui <rui.zhang@intel.com> > Cc: Eduardo Valentin <edubezval@gmail.com> > Signed-off-by: Javi Merino <javi.merino@arm.com> > --- > Documentation/thermal/power_allocator.txt | 2 +- > drivers/thermal/power_allocator.c | 91 +++++++++++++++++-------------- > 2 files changed, 52 insertions(+), 41 deletions(-) > > diff --git a/Documentation/thermal/power_allocator.txt b/Documentation/thermal/power_allocator.txt > index c3797b529991..a1ce2235f121 100644 > --- a/Documentation/thermal/power_allocator.txt > +++ b/Documentation/thermal/power_allocator.txt > @@ -4,7 +4,7 @@ Power allocator governor tunables > Trip points > ----------- > > -The governor requires the following two passive trip points: > +The governor works optimally with the following two passive trip points: > > 1. "switch on" trip point: temperature above which the governor > control loop starts operating. This is the first passive trip > diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c > index f78836c2da26..257c9af20f22 100644 > --- a/drivers/thermal/power_allocator.c > +++ b/drivers/thermal/power_allocator.c > @@ -24,6 +24,8 @@ > > #include "thermal_core.h" > > +#define INVALID_TRIP -1 > + > #define FRAC_BITS 10 > #define int_to_frac(x) ((x) << FRAC_BITS) > #define frac_to_int(x) ((x) >> FRAC_BITS) > @@ -61,6 +63,8 @@ static inline s64 div_frac(s64 x, s64 y) > * Used to calculate the derivative term. > * @trip_switch_on: first passive trip point of the thermal zone. The > * governor switches on when this trip point is crossed. > + * If the thermal zone only has one passive trip point, > + * @trip_switch_on should be INVALID_TRIP. > * @trip_max_desired_temperature: last passive trip point of the thermal > * zone. The temperature we are > * controlling for. > @@ -378,43 +382,66 @@ unlock: > return ret; > } > > -static int get_governor_trips(struct thermal_zone_device *tz, > - struct power_allocator_params *params) > +/** > + * get_governor_trips() - get the number of the two trip points that are key for this governor > + * @tz: thermal zone to operate on > + * @params: pointer the private data for this governor ^ pointer to private data > + * > + * The power allocator governor works optimally with two trips points: > + * a "switch on" trip point and a "maximum desired temperature". These > + * are defined as the first and last passive trip points. > + * > + * If there is only one trip point, then that's considered to be the > + * "maximum desired temperature" trip point and the governor is always > + * on. If there are no passive or active trip points, then the > + * governor won't do anything. In fact, its throttle function > + * shouldn't be called at all. ^ "shouldn't" or "won't" ? [snip] > static void power_allocator_unbind(struct thermal_zone_device *tz) > @@ -544,13 +561,7 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip) > > ret = tz->ops->get_trip_temp(tz, params->trip_switch_on, > &switch_on_temp); > - if (ret) { > - dev_warn(&tz->device, > - "Failed to get switch on temperature: %d\n", ret); > - return ret; > - } > - > - if (current_temp < switch_on_temp) { > + if ((!ret) && (current_temp < switch_on_temp)) { nit: I think the inner pair of () are not necessary. Thanks, -Dan ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/4] thermal: power_allocator: relax the requirement of two passive trip points 2015-08-12 11:13 ` Daniel Kurtz @ 2015-08-12 16:46 ` Javi Merino 0 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-08-12 16:46 UTC (permalink / raw) To: Daniel Kurtz Cc: linux-pm@vger.kernel.org, Dmitry Torokhov, Chung-yih Wang, linux-kernel@vger.kernel.org, Punit Agrawal, Zhang Rui, Eduardo Valentin On Wed, Aug 12, 2015 at 12:13:09PM +0100, Daniel Kurtz wrote: > Hi Javi, Hi Daniel, > On Tue, Aug 11, 2015 at 6:21 PM, Javi Merino <javi.merino@arm.com> wrote: > > The power allocator governor currently requires that the thermal zone > > has at least two passive trip points. If there aren't, the governor > > refuses to bind to the thermal zone. > > > > This commit relaxes that requirement. Now the governor will bind to all > > thermal zones regardless of how many trip points they have. > > > > Cc: Zhang Rui <rui.zhang@intel.com> > > Cc: Eduardo Valentin <edubezval@gmail.com> > > Signed-off-by: Javi Merino <javi.merino@arm.com> > > --- > > Documentation/thermal/power_allocator.txt | 2 +- > > drivers/thermal/power_allocator.c | 91 +++++++++++++++++-------------- > > 2 files changed, 52 insertions(+), 41 deletions(-) > > > > diff --git a/Documentation/thermal/power_allocator.txt b/Documentation/thermal/power_allocator.txt > > index c3797b529991..a1ce2235f121 100644 > > --- a/Documentation/thermal/power_allocator.txt > > +++ b/Documentation/thermal/power_allocator.txt > > @@ -4,7 +4,7 @@ Power allocator governor tunables > > Trip points > > ----------- > > > > -The governor requires the following two passive trip points: > > +The governor works optimally with the following two passive trip points: > > > > 1. "switch on" trip point: temperature above which the governor > > control loop starts operating. This is the first passive trip > > diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c > > index f78836c2da26..257c9af20f22 100644 > > --- a/drivers/thermal/power_allocator.c > > +++ b/drivers/thermal/power_allocator.c > > @@ -24,6 +24,8 @@ > > > > #include "thermal_core.h" > > > > +#define INVALID_TRIP -1 > > + > > #define FRAC_BITS 10 > > #define int_to_frac(x) ((x) << FRAC_BITS) > > #define frac_to_int(x) ((x) >> FRAC_BITS) > > @@ -61,6 +63,8 @@ static inline s64 div_frac(s64 x, s64 y) > > * Used to calculate the derivative term. > > * @trip_switch_on: first passive trip point of the thermal zone. The > > * governor switches on when this trip point is crossed. > > + * If the thermal zone only has one passive trip point, > > + * @trip_switch_on should be INVALID_TRIP. > > * @trip_max_desired_temperature: last passive trip point of the thermal > > * zone. The temperature we are > > * controlling for. > > @@ -378,43 +382,66 @@ unlock: > > return ret; > > } > > > > -static int get_governor_trips(struct thermal_zone_device *tz, > > - struct power_allocator_params *params) > > +/** > > + * get_governor_trips() - get the number of the two trip points that are key for this governor > > + * @tz: thermal zone to operate on > > + * @params: pointer the private data for this governor > ^ > pointer to private data Fixed > > + * > > + * The power allocator governor works optimally with two trips points: > > + * a "switch on" trip point and a "maximum desired temperature". These > > + * are defined as the first and last passive trip points. > > + * > > + * If there is only one trip point, then that's considered to be the > > + * "maximum desired temperature" trip point and the governor is always > > + * on. If there are no passive or active trip points, then the > > + * governor won't do anything. In fact, its throttle function > > + * shouldn't be called at all. > ^ > "shouldn't" or "won't" ? "won't". Sometimes I'm overly cautious. You know "never say never" ;-) > [snip] > > > static void power_allocator_unbind(struct thermal_zone_device *tz) > > @@ -544,13 +561,7 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip) > > > > ret = tz->ops->get_trip_temp(tz, params->trip_switch_on, > > &switch_on_temp); > > - if (ret) { > > - dev_warn(&tz->device, > > - "Failed to get switch on temperature: %d\n", ret); > > - return ret; > > - } > > - > > - if (current_temp < switch_on_temp) { > > + if ((!ret) && (current_temp < switch_on_temp)) { > > nit: I think the inner pair of () are not necessary. They are not necessary, but I prefer the parenthesis around the comparison. It makes it clearer to me. I've dropped the () around !ret. Thanks for the comments here and in the first patch. Javi ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 3/4] thermal: power_allocator: don't require tzp to be present for the thermal zone 2015-08-11 10:21 ` [PATCH v2 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino 2015-08-11 10:21 ` [PATCH v2 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino 2015-08-11 10:21 ` [PATCH v2 2/4] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino @ 2015-08-11 10:21 ` Javi Merino 2015-08-11 10:21 ` [PATCH v2 4/4] thermal: power_allocator: exit early if there are no cooling devices Javi Merino 2015-08-17 17:36 ` [PATCH v3 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino 4 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-08-11 10:21 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, Javi Merino 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v2 4/4] thermal: power_allocator: exit early if there are no cooling devices 2015-08-11 10:21 ` [PATCH v2 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino ` (2 preceding siblings ...) 2015-08-11 10:21 ` [PATCH v2 3/4] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino @ 2015-08-11 10:21 ` Javi Merino 2015-08-17 17:36 ` [PATCH v3 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino 4 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-08-11 10:21 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, Javi Merino 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v3 0/4] Let the power allocator thermal governor run on any thermal zone 2015-08-11 10:21 ` [PATCH v2 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino ` (3 preceding siblings ...) 2015-08-11 10:21 ` [PATCH v2 4/4] thermal: power_allocator: exit early if there are no cooling devices Javi Merino @ 2015-08-17 17:36 ` Javi Merino 2015-08-17 17:36 ` [PATCH v3 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino ` (4 more replies) 4 siblings, 5 replies; 52+ messages in thread From: Javi Merino @ 2015-08-17 17:36 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, Javi Merino 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v3 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp 2015-08-17 17:36 ` [PATCH v3 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino @ 2015-08-17 17:36 ` Javi Merino 2015-08-20 22:16 ` Eduardo Valentin 2015-08-17 17:36 ` [PATCH v3 2/4] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino ` (3 subsequent siblings) 4 siblings, 1 reply; 52+ messages in thread From: Javi Merino @ 2015-08-17 17:36 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, Javi Merino, Zhang Rui, Eduardo Valentin The power allocator governor currently requires that a sustainable power is passed as part of the thermal zone's thermal zone parameters. If that parameter is not provided, it doesn't register with the thermal zone. While this parameter is strongly recommended for optimal performance, it doesn't need to be mandatory. Relax the requirement and allow the governor to bind to thermal zones that don't provide it by estimating it from the cooling devices' power model. Cc: Zhang Rui <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Signed-off-by: Javi Merino <javi.merino@arm.com> --- drivers/thermal/power_allocator.c | 62 +++++++++++++++++++++++++++++++++------ drivers/thermal/thermal_core.c | 28 ++++++++++++++++++ include/linux/thermal.h | 6 ++++ 3 files changed, 87 insertions(+), 9 deletions(-) diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c index 63a448f9d93b..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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v3 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp 2015-08-17 17:36 ` [PATCH v3 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino @ 2015-08-20 22:16 ` Eduardo Valentin 2015-08-24 18:37 ` Javi Merino 0 siblings, 1 reply; 52+ messages in thread From: Eduardo Valentin @ 2015-08-20 22:16 UTC (permalink / raw) To: Javi Merino Cc: linux-pm, dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, Zhang Rui 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 <rui.zhang@intel.com> > Cc: Eduardo Valentin <edubezval@gmail.com> > Signed-off-by: Javi Merino <javi.merino@arm.com> > --- > drivers/thermal/power_allocator.c | 62 +++++++++++++++++++++++++++++++++------ > drivers/thermal/thermal_core.c | 28 ++++++++++++++++++ > include/linux/thermal.h | 6 ++++ > 3 files changed, 87 insertions(+), 9 deletions(-) > > diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c > index 63a448f9d93b..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 > ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp 2015-08-20 22:16 ` Eduardo Valentin @ 2015-08-24 18:37 ` Javi Merino 0 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-08-24 18:37 UTC (permalink / raw) To: Eduardo Valentin Cc: linux-pm@vger.kernel.org, dmitry.torokhov@gmail.com, cywang@chromium.org, linux-kernel@vger.kernel.org, Punit Agrawal, djkurtz@chromium.org, Zhang Rui 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 <rui.zhang@intel.com> > > Cc: Eduardo Valentin <edubezval@gmail.com> > > Signed-off-by: Javi Merino <javi.merino@arm.com> > > --- > > drivers/thermal/power_allocator.c | 62 +++++++++++++++++++++++++++++++++------ > > drivers/thermal/thermal_core.c | 28 ++++++++++++++++++ > > include/linux/thermal.h | 6 ++++ > > 3 files changed, 87 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c > > index 63a448f9d93b..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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v3 2/4] thermal: power_allocator: relax the requirement of two passive trip points 2015-08-17 17:36 ` [PATCH v3 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino 2015-08-17 17:36 ` [PATCH v3 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino @ 2015-08-17 17:36 ` Javi Merino 2015-08-17 17:36 ` [PATCH v3 3/4] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino ` (2 subsequent siblings) 4 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-08-17 17:36 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, Javi Merino, Zhang Rui, Eduardo Valentin The power allocator governor currently requires that the thermal zone has at least two passive trip points. If there aren't, the governor refuses to bind to the thermal zone. This commit relaxes that requirement. Now the governor will bind to all thermal zones regardless of how many trip points they have. Cc: Zhang Rui <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Signed-off-by: Javi Merino <javi.merino@arm.com> --- Documentation/thermal/power_allocator.txt | 2 +- drivers/thermal/power_allocator.c | 91 +++++++++++++++++-------------- 2 files changed, 52 insertions(+), 41 deletions(-) diff --git a/Documentation/thermal/power_allocator.txt b/Documentation/thermal/power_allocator.txt index c3797b529991..a1ce2235f121 100644 --- a/Documentation/thermal/power_allocator.txt +++ b/Documentation/thermal/power_allocator.txt @@ -4,7 +4,7 @@ Power allocator governor tunables Trip points ----------- -The governor requires the following two passive trip points: +The governor works optimally with the following two passive trip points: 1. "switch on" trip point: temperature above which the governor control loop starts operating. This is the first passive trip diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c index 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v3 3/4] thermal: power_allocator: don't require tzp to be present for the thermal zone 2015-08-17 17:36 ` [PATCH v3 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino 2015-08-17 17:36 ` [PATCH v3 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino 2015-08-17 17:36 ` [PATCH v3 2/4] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino @ 2015-08-17 17:36 ` Javi Merino 2015-08-17 17:36 ` [PATCH v3 4/4] thermal: power_allocator: exit early if there are no cooling devices Javi Merino 2015-08-26 13:26 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Javi Merino 4 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-08-17 17:36 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, Javi Merino, Zhang Rui, Eduardo Valentin 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 <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Signed-off-by: Javi Merino <javi.merino@arm.com> --- 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v3 4/4] thermal: power_allocator: exit early if there are no cooling devices 2015-08-17 17:36 ` [PATCH v3 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino ` (2 preceding siblings ...) 2015-08-17 17:36 ` [PATCH v3 3/4] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino @ 2015-08-17 17:36 ` Javi Merino 2015-08-26 13:26 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Javi Merino 4 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-08-17 17:36 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, Javi Merino, Zhang Rui, Eduardo Valentin 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 <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Signed-off-by: Javi Merino <javi.merino@arm.com> --- 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone 2015-08-17 17:36 ` [PATCH v3 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino ` (3 preceding siblings ...) 2015-08-17 17:36 ` [PATCH v3 4/4] thermal: power_allocator: exit early if there are no cooling devices Javi Merino @ 2015-08-26 13:26 ` Javi Merino 2015-08-26 13:26 ` [PATCH v4 1/5] thermal: Add a function to get the minimum power Javi Merino ` (7 more replies) 4 siblings, 8 replies; 52+ messages in thread From: Javi Merino @ 2015-08-26 13:26 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, edubezval, Javi Merino 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v4 1/5] thermal: Add a function to get the minimum power 2015-08-26 13:26 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Javi Merino @ 2015-08-26 13:26 ` Javi Merino 2015-08-26 13:26 ` [PATCH v4 2/5] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino ` (6 subsequent siblings) 7 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-08-26 13:26 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, edubezval, Javi Merino, Zhang Rui 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 <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Signed-off-by: Javi Merino <javi.merino@arm.com> --- 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v4 2/5] thermal: power_allocator: relax the requirement of a sustainable_power in tzp 2015-08-26 13:26 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Javi Merino 2015-08-26 13:26 ` [PATCH v4 1/5] thermal: Add a function to get the minimum power Javi Merino @ 2015-08-26 13:26 ` Javi Merino 2015-08-26 13:26 ` [PATCH v4 3/5] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino ` (5 subsequent siblings) 7 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-08-26 13:26 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, edubezval, Javi Merino, Zhang Rui The power allocator governor currently requires that a sustainable power is passed as part of the thermal zone's thermal zone parameters. If that parameter is not provided, it doesn't register with the thermal zone. While this parameter is strongly recommended for optimal performance, it doesn't need to be mandatory. Relax the requirement and allow the governor to bind to thermal zones that don't provide it by estimating it from the cooling devices' power model. Cc: Zhang Rui <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Signed-off-by: Javi Merino <javi.merino@arm.com> --- drivers/thermal/power_allocator.c | 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v4 3/5] thermal: power_allocator: relax the requirement of two passive trip points 2015-08-26 13:26 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Javi Merino 2015-08-26 13:26 ` [PATCH v4 1/5] thermal: Add a function to get the minimum power Javi Merino 2015-08-26 13:26 ` [PATCH v4 2/5] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino @ 2015-08-26 13:26 ` Javi Merino 2015-08-26 13:26 ` [PATCH v4 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino ` (4 subsequent siblings) 7 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-08-26 13:26 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, edubezval, Javi Merino, Zhang Rui The power allocator governor currently requires that the thermal zone has at least two passive trip points. If there aren't, the governor refuses to bind to the thermal zone. This commit relaxes that requirement. Now the governor will bind to all thermal zones regardless of how many trip points they have. Cc: Zhang Rui <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Signed-off-by: Javi Merino <javi.merino@arm.com> --- Documentation/thermal/power_allocator.txt | 2 +- drivers/thermal/power_allocator.c | 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v4 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone 2015-08-26 13:26 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Javi Merino ` (2 preceding siblings ...) 2015-08-26 13:26 ` [PATCH v4 3/5] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino @ 2015-08-26 13:26 ` Javi Merino 2015-08-28 2:18 ` Daniel Kurtz 2015-08-26 13:26 ` [PATCH v4 5/5] thermal: power_allocator: exit early if there are no cooling devices Javi Merino ` (3 subsequent siblings) 7 siblings, 1 reply; 52+ messages in thread From: Javi Merino @ 2015-08-26 13:26 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, edubezval, Javi Merino, Zhang Rui 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 <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Signed-off-by: Javi Merino <javi.merino@arm.com> --- 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v4 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone 2015-08-26 13:26 ` [PATCH v4 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino @ 2015-08-28 2:18 ` Daniel Kurtz 2015-08-28 16:28 ` Javi Merino 0 siblings, 1 reply; 52+ messages in thread From: Daniel Kurtz @ 2015-08-28 2:18 UTC (permalink / raw) To: Javi Merino Cc: linux-pm, Dmitry Torokhov, Chung-yih Wang, linux-kernel@vger.kernel.org, Punit Agrawal, Eduardo Valentin, Zhang Rui On Wed, Aug 26, 2015 at 9:26 PM, Javi Merino <javi.merino@arm.com> 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 <rui.zhang@intel.com> > Cc: Eduardo Valentin <edubezval@gmail.com> > Signed-off-by: Javi Merino <javi.merino@arm.com> > --- > > 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 > ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v4 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone 2015-08-28 2:18 ` Daniel Kurtz @ 2015-08-28 16:28 ` Javi Merino 2015-08-31 13:14 ` Daniel Kurtz 0 siblings, 1 reply; 52+ messages in thread From: Javi Merino @ 2015-08-28 16:28 UTC (permalink / raw) To: Daniel Kurtz Cc: linux-pm@vger.kernel.org, Dmitry Torokhov, Chung-yih Wang, linux-kernel@vger.kernel.org, Punit Agrawal, Eduardo Valentin, Zhang Rui On Fri, Aug 28, 2015 at 03:18:20AM +0100, Daniel Kurtz wrote: > On Wed, Aug 26, 2015 at 9:26 PM, Javi Merino <javi.merino@arm.com> 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 <rui.zhang@intel.com> > > Cc: Eduardo Valentin <edubezval@gmail.com> > > Signed-off-by: Javi Merino <javi.merino@arm.com> > > --- > > > > 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 > > > ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v4 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone 2015-08-28 16:28 ` Javi Merino @ 2015-08-31 13:14 ` Daniel Kurtz 0 siblings, 0 replies; 52+ messages in thread From: Daniel Kurtz @ 2015-08-31 13:14 UTC (permalink / raw) To: Javi Merino Cc: linux-pm@vger.kernel.org, Dmitry Torokhov, Chung-yih Wang, linux-kernel@vger.kernel.org, Punit Agrawal, Eduardo Valentin, Zhang Rui Hi Javi, On Sat, Aug 29, 2015 at 12:28 AM, Javi Merino <javi.merino@arm.com> 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 <javi.merino@arm.com> 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 <rui.zhang@intel.com> >> > Cc: Eduardo Valentin <edubezval@gmail.com> >> > Signed-off-by: Javi Merino <javi.merino@arm.com> >> > --- >> > >> > 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 <djkurtz@chromium.org> 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 >> > >> ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v4 5/5] thermal: power_allocator: exit early if there are no cooling devices 2015-08-26 13:26 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Javi Merino ` (3 preceding siblings ...) 2015-08-26 13:26 ` [PATCH v4 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino @ 2015-08-26 13:26 ` Javi Merino 2015-09-02 15:11 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Javi Merino ` (2 subsequent siblings) 7 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-08-26 13:26 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, edubezval, Javi Merino, Zhang Rui 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 <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Signed-off-by: Javi Merino <javi.merino@arm.com> --- 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone 2015-08-26 13:26 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Javi Merino ` (4 preceding siblings ...) 2015-08-26 13:26 ` [PATCH v4 5/5] thermal: power_allocator: exit early if there are no cooling devices Javi Merino @ 2015-09-02 15:11 ` Javi Merino 2015-09-07 13:19 ` [PATCH v5 " Javi Merino 2015-09-14 3:04 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Eduardo Valentin 7 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-09-02 15:11 UTC (permalink / raw) To: edubezval@gmail.com Cc: linux-pm@vger.kernel.org, dmitry.torokhov@gmail.com, cywang@chromium.org, linux-kernel@vger.kernel.org, Punit Agrawal, djkurtz@chromium.org On Wed, Aug 26, 2015 at 02:26:39PM +0100, Javi Merino wrote: > 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(-) Gentle ping. ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v5 0/5] Let the power allocator thermal governor run on any thermal zone 2015-08-26 13:26 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Javi Merino ` (5 preceding siblings ...) 2015-09-02 15:11 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Javi Merino @ 2015-09-07 13:19 ` Javi Merino 2015-09-07 13:19 ` [PATCH v5 1/5] thermal: Add a function to get the minimum power Javi Merino ` (6 more replies) 2015-09-14 3:04 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Eduardo Valentin 7 siblings, 7 replies; 52+ messages in thread From: Javi Merino @ 2015-09-07 13:19 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, edubezval, Javi Merino 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 possible with the data provided. Changes since v4: - Fix crash when a thermal zone with no trip points has no get_trip_point_temp(). Reported by Daniel Kurtz. - s/estimate_controller_constants()/estimate_pid_constants()/g 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 | 243 ++++++++++++++++++++++-------- drivers/thermal/thermal_core.c | 28 ++++ include/linux/thermal.h | 6 + 4 files changed, 214 insertions(+), 65 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v5 1/5] thermal: Add a function to get the minimum power 2015-09-07 13:19 ` [PATCH v5 " Javi Merino @ 2015-09-07 13:19 ` Javi Merino 2015-09-07 13:19 ` [PATCH v5 2/5] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino ` (5 subsequent siblings) 6 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-09-07 13:19 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, edubezval, Javi Merino, Zhang Rui 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 <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Signed-off-by: Javi Merino <javi.merino@arm.com> --- 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v5 2/5] thermal: power_allocator: relax the requirement of a sustainable_power in tzp 2015-09-07 13:19 ` [PATCH v5 " Javi Merino 2015-09-07 13:19 ` [PATCH v5 1/5] thermal: Add a function to get the minimum power Javi Merino @ 2015-09-07 13:19 ` Javi Merino 2015-09-07 13:19 ` [PATCH v5 3/5] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino ` (4 subsequent siblings) 6 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-09-07 13:19 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, edubezval, Javi Merino, Zhang Rui The power allocator governor currently requires that a sustainable power is passed as part of the thermal zone's thermal zone parameters. If that parameter is not provided, it doesn't register with the thermal zone. While this parameter is strongly recommended for optimal performance, it doesn't need to be mandatory. Relax the requirement and allow the governor to bind to thermal zones that don't provide it by estimating it from the cooling devices' power model. Cc: Zhang Rui <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Signed-off-by: Javi Merino <javi.merino@arm.com> --- drivers/thermal/power_allocator.c | 125 ++++++++++++++++++++++++++++++-------- 1 file changed, 100 insertions(+), 25 deletions(-) diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c index 251676902869..7fa6685f9c5b 100644 --- a/drivers/thermal/power_allocator.c +++ b/drivers/thermal/power_allocator.c @@ -73,6 +73,88 @@ 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_pid_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_pid_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 +180,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_pid_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 +231,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); @@ -416,19 +508,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, @@ -437,29 +528,13 @@ 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_pid_constants(tz, tz->tzp->sustainable_power, + params->trip_switch_on, control_temp, false); reset_pid_controller(params); tz->governor_data = params; -- 1.9.1 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v5 3/5] thermal: power_allocator: relax the requirement of two passive trip points 2015-09-07 13:19 ` [PATCH v5 " Javi Merino 2015-09-07 13:19 ` [PATCH v5 1/5] thermal: Add a function to get the minimum power Javi Merino 2015-09-07 13:19 ` [PATCH v5 2/5] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino @ 2015-09-07 13:19 ` Javi Merino 2015-09-07 13:19 ` [PATCH v5 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino ` (3 subsequent siblings) 6 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-09-07 13:19 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, edubezval, Javi Merino, Zhang Rui The power allocator governor currently requires that the thermal zone has at least two passive trip points. If there aren't, the governor refuses to bind to the thermal zone. This commit relaxes that requirement. Now the governor will bind to all thermal zones regardless of how many trip points they have. Cc: Zhang Rui <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Signed-off-by: Javi Merino <javi.merino@arm.com> --- Documentation/thermal/power_allocator.txt | 2 +- drivers/thermal/power_allocator.c | 101 +++++++++++++++++------------- 2 files changed, 58 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 7fa6685f9c5b..06e954cd81cc 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. @@ -432,43 +436,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) @@ -497,11 +524,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) @@ -520,30 +546,23 @@ 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; + if (tz->trips > 0) { + ret = tz->ops->get_trip_temp(tz, + params->trip_max_desired_temperature, + &control_temp); + if (!ret) + estimate_pid_constants(tz, tz->tzp->sustainable_power, + params->trip_switch_on, + control_temp, false); + } - estimate_pid_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) @@ -574,13 +593,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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v5 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone 2015-09-07 13:19 ` [PATCH v5 " Javi Merino ` (2 preceding siblings ...) 2015-09-07 13:19 ` [PATCH v5 3/5] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino @ 2015-09-07 13:19 ` Javi Merino 2015-09-07 13:19 ` [PATCH v5 5/5] thermal: power_allocator: exit early if there are no cooling devices Javi Merino ` (2 subsequent siblings) 6 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-09-07 13:19 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, edubezval, Javi Merino, Zhang Rui 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 <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org> Signed-off-by: Javi Merino <javi.merino@arm.com> --- 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 more consistent behavior for the system integrator as it doesn't make a distinction between thermal zones created with tzp and those that don't. 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 06e954cd81cc..78d589e7e65f 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; @@ -527,8 +530,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) { @@ -536,13 +538,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"); @@ -563,11 +572,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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v5 5/5] thermal: power_allocator: exit early if there are no cooling devices 2015-09-07 13:19 ` [PATCH v5 " Javi Merino ` (3 preceding siblings ...) 2015-09-07 13:19 ` [PATCH v5 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino @ 2015-09-07 13:19 ` Javi Merino 2015-09-08 1:46 ` [PATCH v5 0/5] Let the power allocator thermal governor run on any thermal zone Daniel Kurtz 2015-09-14 13:23 ` [PATCH v6 " Javi Merino 6 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-09-07 13:19 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, edubezval, Javi Merino, Zhang Rui 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 <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Signed-off-by: Javi Merino <javi.merino@arm.com> --- 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 78d589e7e65f..8a0d801ed29b 100644 --- a/drivers/thermal/power_allocator.c +++ b/drivers/thermal/power_allocator.c @@ -346,6 +346,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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v5 0/5] Let the power allocator thermal governor run on any thermal zone 2015-09-07 13:19 ` [PATCH v5 " Javi Merino ` (4 preceding siblings ...) 2015-09-07 13:19 ` [PATCH v5 5/5] thermal: power_allocator: exit early if there are no cooling devices Javi Merino @ 2015-09-08 1:46 ` Daniel Kurtz 2015-09-14 13:23 ` [PATCH v6 " Javi Merino 6 siblings, 0 replies; 52+ messages in thread From: Daniel Kurtz @ 2015-09-08 1:46 UTC (permalink / raw) To: Javi Merino Cc: linux-pm, Dmitry Torokhov, Chung-yih Wang, linux-kernel@vger.kernel.org, Punit Agrawal, Eduardo Valentin Hi Javi, On Mon, Sep 7, 2015 at 9:19 PM, Javi Merino <javi.merino@arm.com> wrote: > 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 possible with the > data provided. I tested this whole series on my MT8173 board with a battery thermal zone with no trip points, so, for this whole series (not sure if both are needed, so take your pick): Reviewed-by: Daniel Kurtz <djkurtz@chromium.org> Tested-by: Daniel Kurtz <djkurtz@chromium.org> I am a bit confused by MAINTAINERS for drivers/thermal. Will these patches be picked up by Eduardo or Zhang? Thanks! -Dan > Changes since v4: > - Fix crash when a thermal zone with no trip points has no > get_trip_point_temp(). Reported by Daniel Kurtz. > - s/estimate_controller_constants()/estimate_pid_constants()/g > > 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 | 243 ++++++++++++++++++++++-------- > drivers/thermal/thermal_core.c | 28 ++++ > include/linux/thermal.h | 6 + > 4 files changed, 214 insertions(+), 65 deletions(-) > > -- > 1.9.1 > ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v6 0/5] Let the power allocator thermal governor run on any thermal zone 2015-09-07 13:19 ` [PATCH v5 " Javi Merino ` (5 preceding siblings ...) 2015-09-08 1:46 ` [PATCH v5 0/5] Let the power allocator thermal governor run on any thermal zone Daniel Kurtz @ 2015-09-14 13:23 ` Javi Merino 2015-09-14 13:23 ` [PATCH v6 1/5] thermal: Add a function to get the minimum power Javi Merino ` (4 more replies) 6 siblings, 5 replies; 52+ messages in thread From: Javi Merino @ 2015-09-14 13:23 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, edubezval, Javi Merino 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 possible with the data provided. Changes since v5: - Rebased on top of v4.3-rc1 Changes since v4: - Fix crash when a thermal zone with no trip points has no get_trip_point_temp(). Reported by Daniel Kurtz. - s/estimate_controller_constants()/estimate_pid_constants()/g 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 | 243 ++++++++++++++++++++++-------- drivers/thermal/thermal_core.c | 28 ++++ include/linux/thermal.h | 6 + 4 files changed, 214 insertions(+), 65 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v6 1/5] thermal: Add a function to get the minimum power 2015-09-14 13:23 ` [PATCH v6 " Javi Merino @ 2015-09-14 13:23 ` Javi Merino 2015-09-14 13:23 ` [PATCH v6 2/5] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino ` (3 subsequent siblings) 4 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-09-14 13:23 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, edubezval, Javi Merino, Zhang Rui 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 <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org> Signed-off-by: Javi Merino <javi.merino@arm.com> --- 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 5e5fc7015c7f..d9e525cc9c1c 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -1013,6 +1013,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 17292fee8686..98baf2f4ca2f 100644 --- a/include/linux/thermal.h +++ b/include/linux/thermal.h @@ -380,6 +380,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, @@ -415,6 +417,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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v6 2/5] thermal: power_allocator: relax the requirement of a sustainable_power in tzp 2015-09-14 13:23 ` [PATCH v6 " Javi Merino 2015-09-14 13:23 ` [PATCH v6 1/5] thermal: Add a function to get the minimum power Javi Merino @ 2015-09-14 13:23 ` Javi Merino 2015-09-14 13:23 ` [PATCH v6 3/5] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino ` (2 subsequent siblings) 4 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-09-14 13:23 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, edubezval, Javi Merino, Zhang Rui The power allocator governor currently requires that a sustainable power is passed as part of the thermal zone's thermal zone parameters. If that parameter is not provided, it doesn't register with the thermal zone. While this parameter is strongly recommended for optimal performance, it doesn't need to be mandatory. Relax the requirement and allow the governor to bind to thermal zones that don't provide it by estimating it from the cooling devices' power model. Cc: Zhang Rui <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org> Signed-off-by: Javi Merino <javi.merino@arm.com> --- drivers/thermal/power_allocator.c | 125 ++++++++++++++++++++++++++++++-------- 1 file changed, 100 insertions(+), 25 deletions(-) diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c index 9c8a7aad0252..51473473f154 100644 --- a/drivers/thermal/power_allocator.c +++ b/drivers/thermal/power_allocator.c @@ -73,6 +73,88 @@ 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_pid_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_pid_constants(struct thermal_zone_device *tz, + u32 sustainable_power, int trip_switch_on, + int control_temp, bool force) +{ + int ret; + int 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 +180,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_pid_constants(tz, sustainable_power, + params->trip_switch_on, control_temp, + true); + } + err = control_temp - current_temp; err = int_to_frac(err); @@ -139,7 +231,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); @@ -416,19 +508,18 @@ static int power_allocator_bind(struct thermal_zone_device *tz) { int ret; struct power_allocator_params *params; - int switch_on_temp, control_temp; - u32 temperature_threshold; + int 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, @@ -437,29 +528,13 @@ 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_pid_constants(tz, tz->tzp->sustainable_power, + params->trip_switch_on, control_temp, false); reset_pid_controller(params); tz->governor_data = params; -- 1.9.1 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v6 3/5] thermal: power_allocator: relax the requirement of two passive trip points 2015-09-14 13:23 ` [PATCH v6 " Javi Merino 2015-09-14 13:23 ` [PATCH v6 1/5] thermal: Add a function to get the minimum power Javi Merino 2015-09-14 13:23 ` [PATCH v6 2/5] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino @ 2015-09-14 13:23 ` Javi Merino 2015-09-14 13:23 ` [PATCH v6 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino 2015-09-14 13:23 ` [PATCH v6 5/5] thermal: power_allocator: exit early if there are no cooling devices Javi Merino 4 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-09-14 13:23 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, edubezval, Javi Merino, Zhang Rui The power allocator governor currently requires that the thermal zone has at least two passive trip points. If there aren't, the governor refuses to bind to the thermal zone. This commit relaxes that requirement. Now the governor will bind to all thermal zones regardless of how many trip points they have. Cc: Zhang Rui <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org> Signed-off-by: Javi Merino <javi.merino@arm.com> --- Documentation/thermal/power_allocator.txt | 2 +- drivers/thermal/power_allocator.c | 101 +++++++++++++++++------------- 2 files changed, 58 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 51473473f154..ea57759eb095 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. @@ -432,43 +436,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) @@ -497,11 +524,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) @@ -520,30 +546,23 @@ 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; + if (tz->trips > 0) { + ret = tz->ops->get_trip_temp(tz, + params->trip_max_desired_temperature, + &control_temp); + if (!ret) + estimate_pid_constants(tz, tz->tzp->sustainable_power, + params->trip_switch_on, + control_temp, false); + } - estimate_pid_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) @@ -574,13 +593,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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v6 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone 2015-09-14 13:23 ` [PATCH v6 " Javi Merino ` (2 preceding siblings ...) 2015-09-14 13:23 ` [PATCH v6 3/5] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino @ 2015-09-14 13:23 ` Javi Merino 2015-09-14 13:23 ` [PATCH v6 5/5] thermal: power_allocator: exit early if there are no cooling devices Javi Merino 4 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-09-14 13:23 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, edubezval, Javi Merino, Zhang Rui 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 <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org> Signed-off-by: Javi Merino <javi.merino@arm.com> --- 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 more consistent behavior for the system integrator as it doesn't make a distinction between thermal zones created with tzp and those that don't. 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 ea57759eb095..718363f5be40 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; @@ -527,8 +530,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) { @@ -536,13 +538,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz) struct power_allocator_params *params; int 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"); @@ -563,11 +572,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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v6 5/5] thermal: power_allocator: exit early if there are no cooling devices 2015-09-14 13:23 ` [PATCH v6 " Javi Merino ` (3 preceding siblings ...) 2015-09-14 13:23 ` [PATCH v6 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino @ 2015-09-14 13:23 ` Javi Merino 4 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-09-14 13:23 UTC (permalink / raw) To: linux-pm Cc: dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz, edubezval, Javi Merino, Zhang Rui 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 <rui.zhang@intel.com> Cc: Eduardo Valentin <edubezval@gmail.com> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org> Signed-off-by: Javi Merino <javi.merino@arm.com> --- 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 718363f5be40..7ff96270c933 100644 --- a/drivers/thermal/power_allocator.c +++ b/drivers/thermal/power_allocator.c @@ -346,6 +346,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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone 2015-08-26 13:26 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Javi Merino ` (6 preceding siblings ...) 2015-09-07 13:19 ` [PATCH v5 " Javi Merino @ 2015-09-14 3:04 ` Eduardo Valentin 2015-09-14 13:32 ` Javi Merino 7 siblings, 1 reply; 52+ messages in thread From: Eduardo Valentin @ 2015-09-14 3:04 UTC (permalink / raw) To: Javi Merino Cc: linux-pm, dmitry.torokhov, cywang, linux-kernel, punit.agrawal, djkurtz [-- Attachment #1: Type: text/plain, Size: 1614 bytes --] Javi, On Wed, Aug 26, 2015 at 02:26:39PM +0100, Javi Merino wrote: > 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 This patch set does not apply clean. Could you please refresh it? > > 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 > [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone 2015-09-14 3:04 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Eduardo Valentin @ 2015-09-14 13:32 ` Javi Merino 0 siblings, 0 replies; 52+ messages in thread From: Javi Merino @ 2015-09-14 13:32 UTC (permalink / raw) To: Eduardo Valentin Cc: linux-pm@vger.kernel.org, dmitry.torokhov@gmail.com, cywang@chromium.org, linux-kernel@vger.kernel.org, Punit Agrawal, djkurtz@chromium.org On Mon, Sep 14, 2015 at 04:04:16AM +0100, Eduardo Valentin wrote: > Javi, > > On Wed, Aug 26, 2015 at 02:26:39PM +0100, Javi Merino wrote: > > 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 > > This patch set does not apply clean. Could you please refresh it? I've sent a v6 that's rebased on top of v4.3-rc1 Cheers, Javi ^ permalink raw reply [flat|nested] 52+ messages in thread
end of thread, other threads:[~2015-09-14 13:32 UTC | newest] Thread overview: 52+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-08-04 16:39 [PATCH] thermal: remove power allocator from list of default governors Dmitry Torokhov 2015-08-05 8:37 ` Javi Merino 2015-08-05 16:18 ` Srinivas Pandruvada 2015-08-05 16:35 ` Dmitry Torokhov 2015-08-05 18:49 ` Eduardo Valentin 2015-08-06 8:30 ` Javi Merino 2015-08-10 16:04 ` [PATCH 0/3] Let the power allocator thermal governor run on any thermal zone Javi Merino 2015-08-10 16:04 ` [PATCH 1/3] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino 2015-08-10 16:04 ` [PATCH 2/3] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino 2015-08-10 16:04 ` [PATCH 3/3] thermal: power_allocator: exit early if there are no cooling devices Javi Merino 2015-08-11 10:21 ` [PATCH v2 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino 2015-08-11 10:21 ` [PATCH v2 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino 2015-08-11 13:42 ` Punit Agrawal 2015-08-11 17:37 ` Javi Merino 2015-08-12 11:05 ` Daniel Kurtz 2015-08-11 10:21 ` [PATCH v2 2/4] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino 2015-08-12 11:13 ` Daniel Kurtz 2015-08-12 16:46 ` Javi Merino 2015-08-11 10:21 ` [PATCH v2 3/4] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino 2015-08-11 10:21 ` [PATCH v2 4/4] thermal: power_allocator: exit early if there are no cooling devices Javi Merino 2015-08-17 17:36 ` [PATCH v3 0/4] Let the power allocator thermal governor run on any thermal zone Javi Merino 2015-08-17 17:36 ` [PATCH v3 1/4] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino 2015-08-20 22:16 ` Eduardo Valentin 2015-08-24 18:37 ` Javi Merino 2015-08-17 17:36 ` [PATCH v3 2/4] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino 2015-08-17 17:36 ` [PATCH v3 3/4] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino 2015-08-17 17:36 ` [PATCH v3 4/4] thermal: power_allocator: exit early if there are no cooling devices Javi Merino 2015-08-26 13:26 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Javi Merino 2015-08-26 13:26 ` [PATCH v4 1/5] thermal: Add a function to get the minimum power Javi Merino 2015-08-26 13:26 ` [PATCH v4 2/5] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino 2015-08-26 13:26 ` [PATCH v4 3/5] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino 2015-08-26 13:26 ` [PATCH v4 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino 2015-08-28 2:18 ` Daniel Kurtz 2015-08-28 16:28 ` Javi Merino 2015-08-31 13:14 ` Daniel Kurtz 2015-08-26 13:26 ` [PATCH v4 5/5] thermal: power_allocator: exit early if there are no cooling devices Javi Merino 2015-09-02 15:11 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Javi Merino 2015-09-07 13:19 ` [PATCH v5 " Javi Merino 2015-09-07 13:19 ` [PATCH v5 1/5] thermal: Add a function to get the minimum power Javi Merino 2015-09-07 13:19 ` [PATCH v5 2/5] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino 2015-09-07 13:19 ` [PATCH v5 3/5] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino 2015-09-07 13:19 ` [PATCH v5 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino 2015-09-07 13:19 ` [PATCH v5 5/5] thermal: power_allocator: exit early if there are no cooling devices Javi Merino 2015-09-08 1:46 ` [PATCH v5 0/5] Let the power allocator thermal governor run on any thermal zone Daniel Kurtz 2015-09-14 13:23 ` [PATCH v6 " Javi Merino 2015-09-14 13:23 ` [PATCH v6 1/5] thermal: Add a function to get the minimum power Javi Merino 2015-09-14 13:23 ` [PATCH v6 2/5] thermal: power_allocator: relax the requirement of a sustainable_power in tzp Javi Merino 2015-09-14 13:23 ` [PATCH v6 3/5] thermal: power_allocator: relax the requirement of two passive trip points Javi Merino 2015-09-14 13:23 ` [PATCH v6 4/5] thermal: power_allocator: don't require tzp to be present for the thermal zone Javi Merino 2015-09-14 13:23 ` [PATCH v6 5/5] thermal: power_allocator: exit early if there are no cooling devices Javi Merino 2015-09-14 3:04 ` [PATCH v4 0/5] Let the power allocator thermal governor run on any thermal zone Eduardo Valentin 2015-09-14 13:32 ` Javi Merino
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.