[PATCH 0/3] Add upper and lower limits in IPA power budget calculation

[PATCH 0/3] Add upper and lower limits in IPA power budget calculation

[Lukasz Luba]

Hi all,

This patch set makes thermal governor Intelligent Power Allocation (IPA)
aware of cooling device limits for upper and lower bounds and respects them
in the internal power budget calculation.
The patch set should be applied on top of some already posted IPA changes [1][2].

Regards,
Lukasz

[1] https://lore.kernel.org/linux-pm/20201002122416.13659-1-lukasz.luba@arm.com/
[2] https://lore.kernel.org/linux-pm/9ecedd8a-fbc3-895c-d79c-f05af5c90ae5@arm.com/T/#t

Lukasz Luba (3):
  thermal: power_allocator: respect upper and lower bounds for cooling
    device
  thermal: core: remove unused functions in power actor section
  thermal: move power_actor_set_power into IPA

 drivers/thermal/gov_power_allocator.c | 38 ++++++++++-
 drivers/thermal/thermal_core.c        | 90 ---------------------------
 drivers/thermal/thermal_core.h        |  6 --
 3 files changed, 36 insertions(+), 98 deletions(-)

-- 
2.17.1

[PATCH 1/3] thermal: power_allocator: respect upper and lower bounds for cooling device

[Lukasz Luba]

The thermal cooling device specified in DT might be instantiated for
a thermal zone trip point with a limited set of OPPs to operate on. This
configuration should be supported by Intelligent Power Allocation (IPA),
since it is a standard for other governors. Change the code and allow IPA
to get power value of lower and upper bound set for a given cooling
device.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/thermal/gov_power_allocator.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index dd59085f38f5..f9ee7787b325 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -96,7 +96,8 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
 		if (instance->trip != params->trip_max_desired_temperature)
 			continue;
 
-		if (power_actor_get_min_power(cdev, tz, &min_power))
+		if (cdev->ops->state2power(cdev, tz, instance->upper,
+					   &min_power))
 			continue;
 
 		sustainable_power += min_power;
@@ -404,7 +405,8 @@ static int allocate_power(struct thermal_zone_device *tz,
 
 		weighted_req_power[i] = frac_to_int(weight * req_power[i]);
 
-		if (power_actor_get_max_power(cdev, tz, &max_power[i]))
+		if (cdev->ops->state2power(cdev, tz, instance->lower,
+					   &max_power[i]))
 			continue;
 
 		total_req_power += req_power[i];
-- 
2.17.1

[PATCH 2/3] thermal: core: remove unused functions in power actor section

[Lukasz Luba]

Since the Intelligent Power Allocation (IPA) uses different way to get
minimum and maximum power for a given cooling device, the helper functions
are not needed. There is no other code which uses them, so remove the
helper functions.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/thermal/thermal_core.c | 49 ----------------------------------
 drivers/thermal/thermal_core.h |  4 ---
 2 files changed, 53 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 1cd8721327fb..6cba54929ef8 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -600,55 +600,6 @@ static void thermal_zone_device_check(struct work_struct *work)
  * how to estimate their devices power consumption.
  */
 
-/**
- * power_actor_get_max_power() - get the maximum power that a cdev can consume
- * @cdev:	pointer to &thermal_cooling_device
- * @tz:		a valid thermal zone device pointer
- * @max_power:	pointer in which to store the maximum power
- *
- * Calculate the maximum power consumption in milliwats that the
- * cooling device can currently consume and store it in @max_power.
- *
- * Return: 0 on success, -EINVAL if @cdev doesn't support the
- * power_actor API or -E* on other error.
- */
-int power_actor_get_max_power(struct thermal_cooling_device *cdev,
-			      struct thermal_zone_device *tz, u32 *max_power)
-{
-	if (!cdev_is_power_actor(cdev))
-		return -EINVAL;
-
-	return cdev->ops->state2power(cdev, tz, 0, max_power);
-}
-
-/**
- * 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 a cooling device consumes
  * @cdev:	pointer to &thermal_cooling_device
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index e00fc5585ea8..14f8a829a84a 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -65,10 +65,6 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev)
 		cdev->ops->power2state;
 }
 
-int power_actor_get_max_power(struct thermal_cooling_device *cdev,
-			      struct thermal_zone_device *tz, u32 *max_power);
-int power_actor_get_min_power(struct thermal_cooling_device *cdev,
-			      struct thermal_zone_device *tz, u32 *min_power);
 int power_actor_set_power(struct thermal_cooling_device *cdev,
 			  struct thermal_instance *ti, u32 power);
 /**
-- 
2.17.1

[PATCH 3/3] thermal: move power_actor_set_power into IPA

[Lukasz Luba]

Since the power actor section has one function power_actor_set_power()
move it into Intelligent Power Allocation (IPA). There is no other user
of that helper function. It would also allow to remove the check of
cdev_is_power_actor() because the code which calls it in IPA already does
the needed check. Make the function static since only IPA use it.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/thermal/gov_power_allocator.c | 32 +++++++++++++++++++++
 drivers/thermal/thermal_core.c        | 41 ---------------------------
 drivers/thermal/thermal_core.h        |  2 --
 3 files changed, 32 insertions(+), 43 deletions(-)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index f9ee7787b325..5009eb012dfe 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -258,6 +258,38 @@ static u32 pid_controller(struct thermal_zone_device *tz,
 	return power_range;
 }
 
+/**
+ * power_actor_set_power() - limit the maximum power a cooling device consumes
+ * @cdev:	pointer to &thermal_cooling_device
+ * @instance:	thermal instance to update
+ * @power:	the power in milliwatts
+ *
+ * Set the cooling device to consume at most @power milliwatts. The limit is
+ * expected to be a cap at the maximum power consumption.
+ *
+ * Return: 0 on success, -EINVAL if the cooling device does not
+ * implement the power actor API or -E* for other failures.
+ */
+static int
+power_actor_set_power(struct thermal_cooling_device *cdev,
+		      struct thermal_instance *instance, u32 power)
+{
+	unsigned long state;
+	int ret;
+
+	ret = cdev->ops->power2state(cdev, instance->tz, power, &state);
+	if (ret)
+		return ret;
+
+	instance->target = clamp_val(state, instance->lower, instance->upper);
+	mutex_lock(&cdev->lock);
+	cdev->updated = false;
+	mutex_unlock(&cdev->lock);
+	thermal_cdev_update(cdev);
+
+	return 0;
+}
+
 /**
  * divvy_up_power() - divvy the allocated power between the actors
  * @req_power:	each actor's requested power
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 6cba54929ef8..fd14e4b949f3 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -593,47 +593,6 @@ static void thermal_zone_device_check(struct work_struct *work)
 	thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
 }
 
-/*
- * Power actor section: interface to power actors to estimate power
- *
- * Set of functions used to interact to cooling devices that know
- * how to estimate their devices power consumption.
- */
-
-/**
- * power_actor_set_power() - limit the maximum power a cooling device consumes
- * @cdev:	pointer to &thermal_cooling_device
- * @instance:	thermal instance to update
- * @power:	the power in milliwatts
- *
- * Set the cooling device to consume at most @power milliwatts. The limit is
- * expected to be a cap at the maximum power consumption.
- *
- * Return: 0 on success, -EINVAL if the cooling device does not
- * implement the power actor API or -E* for other failures.
- */
-int power_actor_set_power(struct thermal_cooling_device *cdev,
-			  struct thermal_instance *instance, u32 power)
-{
-	unsigned long state;
-	int ret;
-
-	if (!cdev_is_power_actor(cdev))
-		return -EINVAL;
-
-	ret = cdev->ops->power2state(cdev, instance->tz, power, &state);
-	if (ret)
-		return ret;
-
-	instance->target = clamp_val(state, instance->lower, instance->upper);
-	mutex_lock(&cdev->lock);
-	cdev->updated = false;
-	mutex_unlock(&cdev->lock);
-	thermal_cdev_update(cdev);
-
-	return 0;
-}
-
 void thermal_zone_device_rebind_exception(struct thermal_zone_device *tz,
 					  const char *cdev_type, size_t size)
 {
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 14f8a829a84a..fc887d6f23ff 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -65,8 +65,6 @@ static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev)
 		cdev->ops->power2state;
 }
 
-int power_actor_set_power(struct thermal_cooling_device *cdev,
-			  struct thermal_instance *ti, u32 power);
 /**
  * struct thermal_trip - representation of a point in temperature domain
  * @np: pointer to struct device_node that this trip point was created from
-- 
2.17.1

Re: [PATCH 1/3] thermal: power_allocator: respect upper and lower bounds for cooling device

[Daniel Lezcano]

On 07/10/2020 14:22, Lukasz Luba wrote:
> The thermal cooling device specified in DT might be instantiated for
> a thermal zone trip point with a limited set of OPPs to operate on. This
> configuration should be supported by Intelligent Power Allocation (IPA),
> since it is a standard for other governors. Change the code and allow IPA
> to get power value of lower and upper bound set for a given cooling
> device.
> 
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/thermal/gov_power_allocator.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index dd59085f38f5..f9ee7787b325 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -96,7 +96,8 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
>  		if (instance->trip != params->trip_max_desired_temperature)
>  			continue;
>  
> -		if (power_actor_get_min_power(cdev, tz, &min_power))
> +		if (cdev->ops->state2power(cdev, tz, instance->upper,
> +					   &min_power))

	if (cdev->ops->state2power && cdev->ops->state2power(cdev, tz,
							instance->upper,
							&min_power))

?

>  			continue;
>  
>  		sustainable_power += min_power;
> @@ -404,7 +405,8 @@ static int allocate_power(struct thermal_zone_device *tz,
>  
>  		weighted_req_power[i] = frac_to_int(weight * req_power[i]);
>  
> -		if (power_actor_get_max_power(cdev, tz, &max_power[i]))
> +		if (cdev->ops->state2power(cdev, tz, instance->lower,
> +					   &max_power[i]))
>  			continue;

Same here ?

>  		total_req_power += req_power[i];
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 0/3] Add upper and lower limits in IPA power budget calculation

[Daniel Lezcano]

On 07/10/2020 14:22, Lukasz Luba wrote:
> Hi all,
> 
> This patch set makes thermal governor Intelligent Power Allocation (IPA)
> aware of cooling device limits for upper and lower bounds and respects them
> in the internal power budget calculation.
> The patch set should be applied on top of some already posted IPA changes [1][2].
> 
> Regards,
> Lukasz
> 
> [1] https://lore.kernel.org/linux-pm/20201002122416.13659-1-lukasz.luba@arm.com/
> [2] https://lore.kernel.org/linux-pm/9ecedd8a-fbc3-895c-d79c-f05af5c90ae5@arm.com/T/#t
> 
> Lukasz Luba (3):
>   thermal: power_allocator: respect upper and lower bounds for cooling
>     device
>   thermal: core: remove unused functions in power actor section
>   thermal: move power_actor_set_power into IPA
> 
>  drivers/thermal/gov_power_allocator.c | 38 ++++++++++-
>  drivers/thermal/thermal_core.c        | 90 ---------------------------
>  drivers/thermal/thermal_core.h        |  6 --
>  3 files changed, 36 insertions(+), 98 deletions(-)

Thanks for this series. Except a comment in patch 1/3, it looks good to me.


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/3] thermal: power_allocator: respect upper and lower bounds for cooling device

[Lukasz Luba]

On 10/14/20 1:31 PM, Daniel Lezcano wrote:
> On 07/10/2020 14:22, Lukasz Luba wrote:
>> The thermal cooling device specified in DT might be instantiated for
>> a thermal zone trip point with a limited set of OPPs to operate on. This
>> configuration should be supported by Intelligent Power Allocation (IPA),
>> since it is a standard for other governors. Change the code and allow IPA
>> to get power value of lower and upper bound set for a given cooling
>> device.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   drivers/thermal/gov_power_allocator.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
>> index dd59085f38f5..f9ee7787b325 100644
>> --- a/drivers/thermal/gov_power_allocator.c
>> +++ b/drivers/thermal/gov_power_allocator.c
>> @@ -96,7 +96,8 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
>>   		if (instance->trip != params->trip_max_desired_temperature)
>>   			continue;
>>   
>> -		if (power_actor_get_min_power(cdev, tz, &min_power))
>> +		if (cdev->ops->state2power(cdev, tz, instance->upper,
>> +					   &min_power))
> 
> 	if (cdev->ops->state2power && cdev->ops->state2power(cdev, tz,
> 							instance->upper,
> 							&min_power))
> 
> ?


Yes, worth to check. I had this in [1] and missed it here while playing
with re-base of these patch series and other test branches.

I will send v2 with the needed cdev_is_power_actor() check.

> 
>>   			continue;
>>   
>>   		sustainable_power += min_power;
>> @@ -404,7 +405,8 @@ static int allocate_power(struct thermal_zone_device *tz,
>>   
>>   		weighted_req_power[i] = frac_to_int(weight * req_power[i]);
>>   
>> -		if (power_actor_get_max_power(cdev, tz, &max_power[i]))
>> +		if (cdev->ops->state2power(cdev, tz, instance->lower,
>> +					   &max_power[i]))
>>   			continue;
> 
> Same here ?

Inside that loop we check (just a few lines above):

		if (!cdev_is_power_actor(cdev))
			continue;

then we call this:

		if (cdev->ops->state2power(cdev, tz, instance->lower,
					&max_power[i]))

So it should be safe.

> 
>>   		total_req_power += req_power[i];
>>
> 
> 

Thank you Daniel for reviewing this.

Regards,
Lukasz

[1] 
https://lore.kernel.org/linux-pm/20201008170426.465-3-lukasz.luba@arm.com/