[PATCH] thermal: power_allocator: initialize params->sustainable_power in the bind callback

[PATCH] thermal: power_allocator: initialize params->sustainable_power in the bind callback

[Di Shen]

If we set the PID constants k_* via sysfs before the IPA algorithm is
triggered, the constants would be changed after IPA is triggered which
means the k_* are set fail.

The process is as follow:
set k_* via sysfs
    |
set emul_temp via sysfs(temperature > switch_on_temp)
    |
throttle()
    |
temp > switch_on
    |
allocate_power
    |
pid_controller
    |
get_sustainable_power
    |
if (sustainable_power != params->sustainable_power)
because the params->sustainable_power is not initialized first,
so params->sustainable_power = 0, the condition is true, then
call the estimate_pid_constants().
		|
	estimate_pid_constants
		|
The k_* are overwritten, the k_* we set before are invalid.

For example:
unisoc:/sys/class/thermal/thermal_zone0 # cat policy
power_allocator
unisoc:/sys/class/thermal/thermal_zone0 # cat temp
32722
unisoc:/sys/class/thermal/thermal_zone0 # cat k_po
307
unisoc:/sys/class/thermal/thermal_zone0 # cat k_pu
614
unisoc:/sys/class/thermal/thermal_zone0 # cat k_i
61
unisoc:/sys/class/thermal/thermal_zone0 # cat k_*
0
61
307
614

unisoc:/sys/class/thermal/thermal_zone0 # echo 300 > k_po
unisoc:/sys/class/thermal/thermal_zone0 # echo 600 > k_pu
unisoc:/sys/class/thermal/thermal_zone0 # echo 60 >k_i
unisoc:/sys/class/thermal/thermal_zone0 # cat k_*
0
60
300
600

unisoc:/sys/class/thermal/thermal_zone0 # echo 70001 > emul_temp
unisoc:/sys/class/thermal/thermal_zone0 # cat k_*
0
61
307
614
unisoc:/sys/class/thermal/thermal_zone0 # echo 0 > emul_temp

This patch initializes params->sustainable_power when the governor
binds to thermal zone to avoid overwriting k_*.

The basic function won't be affected, as the k_* still can be estimated
if the sustainable_power is modified.

Signed-off-by: Di Shen <di.shen@unisoc.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 81e061f183ad..1b17dc4c219c 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -711,6 +711,8 @@ 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");
+	else
+		params->sustainable_power = tz->tzp->sustainable_power;
 
 	estimate_pid_constants(tz, tz->tzp->sustainable_power,
 			       params->trip_switch_on,
-- 
2.17.1

Re: [PATCH] thermal: power_allocator: initialize params->sustainable_power in the bind callback

[Lukasz Luba]

Hi Di,

Thanks for the patch. The code looks good, but let's
adjust the description.

So 1st line (subject):
thermal: power_allocator: Avoid overwriting PID coefficients from setup time

On 1/24/24 11:49, Di Shen wrote:
> If we set the PID constants k_* via sysfs before the IPA algorithm is

I would say:
'When the PID coefficients k_* are set via sysfs before the IPA
algorithm is triggered then the coefficients would be overwritten after
IPA throttle() is called. The old configuration values might be
different than the new values estimated by the IPA internal algorithm.'

I would also add something like this:
'There might be a time delay when this overwriting happens. It
depends on the thermal zone temperature value. The temperature value
needs to cross the first trip point value then IPA algorithms start
operating. Although, the PID coefficients setup time should not be
affected or linked to any later operating phase and values must not be
overwritten.'

Then we can drop this description from here...

> triggered, the constants would be changed after IPA is triggered which
> means the k_* are set fail.
> 
> The process is as follow:
> set k_* via sysfs
>      |
> set emul_temp via sysfs(temperature > switch_on_temp)
>      |
> throttle()
>      |
> temp > switch_on
>      |
> allocate_power
>      |
> pid_controller
>      |
> get_sustainable_power
>      |
> if (sustainable_power != params->sustainable_power)
> because the params->sustainable_power is not initialized first,
> so params->sustainable_power = 0, the condition is true, then
> call the estimate_pid_constants().
> 		|
> 	estimate_pid_constants
> 		|
> The k_* are overwritten, the k_* we set before are invalid.
> 
> For example:
> unisoc:/sys/class/thermal/thermal_zone0 # cat policy
> power_allocator
> unisoc:/sys/class/thermal/thermal_zone0 # cat temp
> 32722
> unisoc:/sys/class/thermal/thermal_zone0 # cat k_po
> 307
> unisoc:/sys/class/thermal/thermal_zone0 # cat k_pu
> 614
> unisoc:/sys/class/thermal/thermal_zone0 # cat k_i
> 61
> unisoc:/sys/class/thermal/thermal_zone0 # cat k_*
> 0
> 61
> 307
> 614
> 
> unisoc:/sys/class/thermal/thermal_zone0 # echo 300 > k_po
> unisoc:/sys/class/thermal/thermal_zone0 # echo 600 > k_pu
> unisoc:/sys/class/thermal/thermal_zone0 # echo 60 >k_i
> unisoc:/sys/class/thermal/thermal_zone0 # cat k_*
> 0
> 60
> 300
> 600
> 
> unisoc:/sys/class/thermal/thermal_zone0 # echo 70001 > emul_temp
> unisoc:/sys/class/thermal/thermal_zone0 # cat k_*
> 0
> 61
> 307
> 614
> unisoc:/sys/class/thermal/thermal_zone0 # echo 0 > emul_temp

... up to here.


> 
> This patch initializes params->sustainable_power when the governor
> binds to thermal zone to avoid overwriting k_*.
> 
> The basic function won't be affected, as the k_* still can be estimated
> if the sustainable_power is modified.
> 
> Signed-off-by: Di Shen <di.shen@unisoc.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 81e061f183ad..1b17dc4c219c 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -711,6 +711,8 @@ 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");
> +	else
> +		params->sustainable_power = tz->tzp->sustainable_power;
>   
>   	estimate_pid_constants(tz, tz->tzp->sustainable_power,
>   			       params->trip_switch_on,

The code LGTM. Please sent next version of the patch.

Regards,
Lukasz

[PATCH V2] thermal: power_allocator: Avoid overwriting PID coefficients from setup time

[Di Shen]

When the PID coefficients k_* are set via sysfs before the IPA
algorithm is triggered then the coefficients would be overwritten after
IPA throttle() is called. The old configuration values might be
different than the new values estimated by the IPA internal algorithm.

There might be a time delay when this overwriting happens. It
depends on the thermal zone temperature value. The temperature value
needs to cross the first trip point value then IPA algorithms start
operating. Although, the PID coefficients setup time should not be
affected or linked to any later operating phase and values must not be
overwritten.

This patch initializes params->sustainable_power when the governor
binds to thermal zone to avoid overwriting k_*. The basic function won't
be affected, as the k_* still can be estimated if the sustainable_power
is modified.

Signed-off-by: Di Shen <di.shen@unisoc.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 81e061f183ad..1b17dc4c219c 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -711,6 +711,8 @@ 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");
+	else
+		params->sustainable_power = tz->tzp->sustainable_power;
 
 	estimate_pid_constants(tz, tz->tzp->sustainable_power,
 			       params->trip_switch_on,
-- 
2.17.1

Re: [PATCH V2] thermal: power_allocator: Avoid overwriting PID coefficients from setup time

[Di Shen]

Hi Lukasz,

On Thu, Jan 25, 2024 at 2:24 PM Di Shen <di.shen@unisoc.com> wrote:
>
> When the PID coefficients k_* are set via sysfs before the IPA
> algorithm is triggered then the coefficients would be overwritten after
> IPA throttle() is called. The old configuration values might be
> different than the new values estimated by the IPA internal algorithm.
>
> There might be a time delay when this overwriting happens. It
> depends on the thermal zone temperature value. The temperature value
> needs to cross the first trip point value then IPA algorithms start
> operating. Although, the PID coefficients setup time should not be
> affected or linked to any later operating phase and values must not be
> overwritten.
>
> This patch initializes params->sustainable_power when the governor
> binds to thermal zone to avoid overwriting k_*. The basic function won't
> be affected, as the k_* still can be estimated if the sustainable_power
> is modified.
>
> Signed-off-by: Di Shen <di.shen@unisoc.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 81e061f183ad..1b17dc4c219c 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -711,6 +711,8 @@ 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");
> +       else
> +               params->sustainable_power = tz->tzp->sustainable_power;
>
>         estimate_pid_constants(tz, tz->tzp->sustainable_power,
>                                params->trip_switch_on,
> --
> 2.17.1
>

Any comments on patchv2?
Looking forward to hearing from you. Thanks!

Best regards,
Di

Re: [PATCH V2] thermal: power_allocator: Avoid overwriting PID coefficients from setup time

[Lukasz Luba]

Hi Di,

On 2/4/24 02:55, Di Shen wrote:
> Hi Lukasz,
> 
> On Thu, Jan 25, 2024 at 2:24 PM Di Shen <di.shen@unisoc.com> wrote:
>>
>> When the PID coefficients k_* are set via sysfs before the IPA
>> algorithm is triggered then the coefficients would be overwritten after
>> IPA throttle() is called. The old configuration values might be
>> different than the new values estimated by the IPA internal algorithm.
>>
>> There might be a time delay when this overwriting happens. It
>> depends on the thermal zone temperature value. The temperature value
>> needs to cross the first trip point value then IPA algorithms start
>> operating. Although, the PID coefficients setup time should not be
>> affected or linked to any later operating phase and values must not be
>> overwritten.
>>
>> This patch initializes params->sustainable_power when the governor
>> binds to thermal zone to avoid overwriting k_*. The basic function won't
>> be affected, as the k_* still can be estimated if the sustainable_power
>> is modified.
>>
>> Signed-off-by: Di Shen <di.shen@unisoc.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 81e061f183ad..1b17dc4c219c 100644
>> --- a/drivers/thermal/gov_power_allocator.c
>> +++ b/drivers/thermal/gov_power_allocator.c
>> @@ -711,6 +711,8 @@ 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");
>> +       else
>> +               params->sustainable_power = tz->tzp->sustainable_power;
>>
>>          estimate_pid_constants(tz, tz->tzp->sustainable_power,
>>                                 params->trip_switch_on,
>> --
>> 2.17.1
>>
> 
> Any comments on patchv2?
> Looking forward to hearing from you. Thanks!

My apologies I was out-of-office.

That v2 patch looks good. Although, please resend it as a new
message, not as a response to the v1. In that way it's easier to
pick it up by maintainers. You can use a tag:
[RESEND][PATCH v2] .....

You can add
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Regards,
Lukasz