diff mbox series

[2/2] thermal: power allocator: estimate sustainable power only once

Message ID 20201002122416.13659-3-lukasz.luba@arm.com (mailing list archive)
State New, archived
Delegated to: Daniel Lezcano
Headers show
Series Improve the estimations in Intelligent Power Allocation | expand

Commit Message

Lukasz Luba Oct. 2, 2020, 12:24 p.m. UTC
The sustainable power value might come from the Device Tree or can be
estimated in run time. There is no need to estimate every time when the
governor is called and temperature is high. Instead, store the estimated
value and make it available via standard sysfs interface so it can be
checked from the user-space.

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

Comments

Ionela Voinescu Oct. 8, 2020, 10:14 a.m. UTC | #1
Hi Lukasz,

On Friday 02 Oct 2020 at 13:24:16 (+0100), Lukasz Luba wrote:
> The sustainable power value might come from the Device Tree or can be
> estimated in run time. There is no need to estimate every time when the
> governor is called and temperature is high. Instead, store the estimated
> value and make it available via standard sysfs interface so it can be
> checked from the user-space.
> 
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/thermal/gov_power_allocator.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index f69fafe486a5..dd59085f38f5 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -204,6 +204,8 @@ static u32 pid_controller(struct thermal_zone_device *tz,
>  		estimate_pid_constants(tz, sustainable_power,
>  				       params->trip_switch_on, control_temp,
>  				       true);
> +		/* Do the estimation only once and make available in sysfs */
> +		tz->tzp->sustainable_power = sustainable_power;

After looking over the code, it does seems mostly useless to do the
estimation every time the controller kicks in.

But I have two comments in this regard:

 - The estimation is dependent on the temperature we control for which
   can be changed from sysfs. While I don't see that as a big worry,
   (sustainable power is an estimation anyway), it might be worth a
   more detailed comment on why we don't expect this to be a problem,
   or what we expect the consequences of computing sustainable power
   only once could be.

 - In the function comment for estimate_pid_constants() there is a
   mention of sustainable power:
   """
    * 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 we are going to compute the sustainable power estimation only once,
   this comment should be removed, the estimated value should be added to
   the trip point parameters before estimate_pid_constants(), and the
   sustainable_power argument should be removed.
   Otherwise we end up with conflicting information in the code.

Regards,
Ionela.

>  	}
>  
>  	err = control_temp - tz->temperature;
> -- 
> 2.17.1
>
Lukasz Luba Oct. 8, 2020, 12:34 p.m. UTC | #2
Hi Ionela,

On 10/8/20 11:14 AM, Ionela Voinescu wrote:
> Hi Lukasz,
> 
> On Friday 02 Oct 2020 at 13:24:16 (+0100), Lukasz Luba wrote:
>> The sustainable power value might come from the Device Tree or can be
>> estimated in run time. There is no need to estimate every time when the
>> governor is called and temperature is high. Instead, store the estimated
>> value and make it available via standard sysfs interface so it can be
>> checked from the user-space.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   drivers/thermal/gov_power_allocator.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
>> index f69fafe486a5..dd59085f38f5 100644
>> --- a/drivers/thermal/gov_power_allocator.c
>> +++ b/drivers/thermal/gov_power_allocator.c
>> @@ -204,6 +204,8 @@ static u32 pid_controller(struct thermal_zone_device *tz,
>>   		estimate_pid_constants(tz, sustainable_power,
>>   				       params->trip_switch_on, control_temp,
>>   				       true);
>> +		/* Do the estimation only once and make available in sysfs */
>> +		tz->tzp->sustainable_power = sustainable_power;
> 
> After looking over the code, it does seems mostly useless to do the
> estimation every time the controller kicks in.
> 
> But I have two comments in this regard:
> 
>   - The estimation is dependent on the temperature we control for which
>     can be changed from sysfs. While I don't see that as a big worry,
>     (sustainable power is an estimation anyway), it might be worth a
>     more detailed comment on why we don't expect this to be a problem,
>     or what we expect the consequences of computing sustainable power
>     only once could be.

The problem is that we don't expose the estimated value in the sysfs.
This is the case when there was no DT entry with 'sustainable-power'.
If someone is going to write the values via sysfs, we assume he/she
knows the consequences and also what other values to write and where,
to make it working optimally.

> 
>   - In the function comment for estimate_pid_constants() there is a
>     mention of sustainable power:
>     """
>      * 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.
>     """

Good catch, that comment left. I will remove it.

>     If we are going to compute the sustainable power estimation only once,
>     this comment should be removed, the estimated value should be added to
>     the trip point parameters before estimate_pid_constants(), and the
>     sustainable_power argument should be removed.
>     Otherwise we end up with conflicting information in the code.

We can also call estimate_sustainable_power() inside the
estimate_pid_constants() if sust. power was 0, then set the
tz->tzp->sustainable_power = sustainable_power

Thank you for your comments.

Regards,
Lukasz

> 
> Regards,
> Ionela.
>
diff mbox series

Patch

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index f69fafe486a5..dd59085f38f5 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -204,6 +204,8 @@  static u32 pid_controller(struct thermal_zone_device *tz,
 		estimate_pid_constants(tz, sustainable_power,
 				       params->trip_switch_on, control_temp,
 				       true);
+		/* Do the estimation only once and make available in sysfs */
+		tz->tzp->sustainable_power = sustainable_power;
 	}
 
 	err = control_temp - tz->temperature;