[PATCH v4 0/3] Improve the estimations in Intelligent Power Allocation

[PATCH v4 0/3] Improve the estimations in Intelligent Power Allocation

[Lukasz Luba]

Hi all,

The Intelligent Power Allocation (IPA) estimates the needed coefficients for
internal algorithm. It can also estimate the sustainable power value when the
DT has not provided one. Fix the 'k_i' coefficient which might be to big
related to the other values, when the sustainable power is in an abstract
scale. Do the estimation of sustainable power only once and avoid expensive
calculation every time the IPA is called. Do the estimation of PID constants
when there was user update via sysfs to sustainable power.

The patch set should apply on top next-20201124

Changes:
v4:
- added new function get_sustainable_power() which handles use cases
  when the value should be estimated again or simply returned
- added sustainable_power in the power_allocator_params to track if there
  was a change to sustainable_power by the user via sysfs
- addressed Daniel's comments that sustainable power set via sysfs should
  trigger PID coefficients estimation
- removed 'force' argument from estimate_pid_constants() and make it ready
  for updates due to new value for sust. power from sysfs
- abandoned the design from v3 with a single function responsible for
  estimation both sust. power and PID const. requested by Ionela 
v3 [1]:
- changed estimate_pid_constants to estimate_tzp_constants and related comments
- estimate the PID coefficients always together with sust. power
- added print indicating that we are estimating sust. power and PID const.
- don't use local variable 'sustainable_power'

Regards,
Lukasz Luba

[1] https://lore.kernel.org/lkml/20201009135850.14727-1-lukasz.luba@arm.com/

Lukasz Luba (3):
  thermal: power allocator: change the 'k_i' coefficient estimation
  thermal: power allocator: refactor sustainable power estimation
  thermal: power allocator: change the 'k_*' always in
    estimate_pid_constants()

 drivers/thermal/gov_power_allocator.c | 76 +++++++++++++++++----------
 1 file changed, 49 insertions(+), 27 deletions(-)

-- 
2.17.1

[PATCH v4 1/3] thermal: power allocator: change the 'k_i' coefficient estimation

[Lukasz Luba]

Intelligent Power Allocation (IPA) is built around the PID controller
concept. The initialization code tries to setup the environment based on
the information available in DT or estimate the value based on minimum
power reported by each of the cooling device. The estimation will have an
impact on the PID controller behaviour via the related 'k_po', 'k_pu',
'k_i' coefficients and also on the power budget calculation.

This change prevents the situation when 'k_i' is relatively big compared
to 'k_po' and 'k_pu' values. This might happen when the estimation for
'sustainable_power' returned small value, thus 'k_po' and 'k_pu' are
small.

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

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index b29e21c56a4f..2e20085ed217 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -134,6 +134,7 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
 	int ret;
 	int switch_on_temp;
 	u32 temperature_threshold;
+	s32 k_i;
 
 	ret = tz->ops->get_trip_temp(tz, trip_switch_on, &switch_on_temp);
 	if (ret)
@@ -159,8 +160,11 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
 		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;
+	if (!tz->tzp->k_i || force) {
+		k_i = tz->tzp->k_pu / 10;
+		tz->tzp->k_i = k_i > 0 ? k_i : 1;
+	}
+
 	/*
 	 * The default for k_d and integral_cutoff is 0, so we can
 	 * leave them as they are.
-- 
2.17.1

[PATCH v4 2/3] thermal: power allocator: refactor sustainable power estimation

[Lukasz Luba]

The sustainable power value might come from the Device Tree or can be
estimated in run time. The sustainable power might be updated by the user
via sysfs interface, which should trigger new estimation of PID
coefficients. There is no need to estimate it 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 | 52 ++++++++++++++++++++-------
 1 file changed, 40 insertions(+), 12 deletions(-)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 2e20085ed217..d7e4b9f6af60 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -63,6 +63,8 @@ static inline s64 div_frac(s64 x, s64 y)
  * @trip_max_desired_temperature:	last passive trip point of the thermal
  *					zone.  The temperature we are
  *					controlling for.
+ * @sustainable_power:	Sustainable power (heat) that this thermal zone can
+ *			dissipate
  */
 struct power_allocator_params {
 	bool allocated_tzp;
@@ -70,6 +72,7 @@ struct power_allocator_params {
 	s32 prev_err;
 	int trip_switch_on;
 	int trip_max_desired_temperature;
+	u32 sustainable_power;
 };
 
 /**
@@ -118,10 +121,6 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
  *
  * 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
@@ -171,6 +170,42 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
 	 */
 }
 
+/**
+ * get_sustainable_power() - Get the right sustainable power
+ * @tz:		thermal zone for which to estimate the constants
+ * @params:	parameters for the power allocator governor
+ * @control_temp:	target temperature for the power allocator governor
+ *
+ * This function is used for getting the proper sustainable power value based
+ * on variables which might be updated by the user sysfs interface. If that
+ * happen the new value is going to be estimated and updated. It is also used
+ * after thermal zone binding, where the initial values where set to 0.
+ */
+static u32 get_sustainable_power(struct thermal_zone_device *tz,
+				 struct power_allocator_params *params,
+				 int control_temp)
+{
+	u32 sustainable_power;
+
+	if (!tz->tzp->sustainable_power)
+		sustainable_power = estimate_sustainable_power(tz);
+	else
+		sustainable_power = tz->tzp->sustainable_power;
+
+	/* Check if it's init value 0 or there was update via sysfs */
+	if (sustainable_power != params->sustainable_power) {
+		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;
+		params->sustainable_power = sustainable_power;
+	}
+
+	return sustainable_power;
+}
+
 /**
  * pid_controller() - PID controller
  * @tz:	thermal zone we are operating in
@@ -200,14 +235,7 @@ static u32 pid_controller(struct thermal_zone_device *tz,
 
 	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);
-	}
+	sustainable_power = get_sustainable_power(tz, params, control_temp);
 
 	err = control_temp - tz->temperature;
 	err = int_to_frac(err);
-- 
2.17.1

[PATCH v4 3/3] thermal: power allocator: change the 'k_*' always in estimate_pid_constants()

[Lukasz Luba]

The PID coefficients should be estimated again when there was a change to
sustainable power value made by user. This change removes unused argument
'force' and makes the function ready for such updates.

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

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index d7e4b9f6af60..7a4170a0b51f 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -117,18 +117,13 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
  * @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.
- *
- * 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 control_temp)
 {
 	int ret;
 	int switch_on_temp;
@@ -151,18 +146,14 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
 	if (!temperature_threshold)
 		return;
 
-	if (!tz->tzp->k_po || force)
-		tz->tzp->k_po = int_to_frac(sustainable_power) /
-			temperature_threshold;
+	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;
+	tz->tzp->k_pu = int_to_frac(2 * sustainable_power) /
+		temperature_threshold;
 
-	if (!tz->tzp->k_i || force) {
-		k_i = tz->tzp->k_pu / 10;
-		tz->tzp->k_i = k_i > 0 ? k_i : 1;
-	}
+	k_i = tz->tzp->k_pu / 10;
+	tz->tzp->k_i = k_i > 0 ? k_i : 1;
 
 	/*
 	 * The default for k_d and integral_cutoff is 0, so we can
@@ -195,8 +186,7 @@ static u32 get_sustainable_power(struct thermal_zone_device *tz,
 	/* Check if it's init value 0 or there was update via sysfs */
 	if (sustainable_power != params->sustainable_power) {
 		estimate_pid_constants(tz, sustainable_power,
-				       params->trip_switch_on, control_temp,
-				       true);
+				       params->trip_switch_on, control_temp);
 
 		/* Do the estimation only once and make available in sysfs */
 		tz->tzp->sustainable_power = sustainable_power;
@@ -640,7 +630,7 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
 		if (!ret)
 			estimate_pid_constants(tz, tz->tzp->sustainable_power,
 					       params->trip_switch_on,
-					       control_temp, false);
+					       control_temp);
 	}
 
 	reset_pid_controller(params);
-- 
2.17.1

Re: [PATCH v4 0/3] Improve the estimations in Intelligent Power Allocation

[Lukasz Luba]

Hi Daniel,

On 11/24/20 4:10 PM, Lukasz Luba wrote:
> Hi all,
> 
> The Intelligent Power Allocation (IPA) estimates the needed coefficients for
> internal algorithm. It can also estimate the sustainable power value when the
> DT has not provided one. Fix the 'k_i' coefficient which might be to big
> related to the other values, when the sustainable power is in an abstract
> scale. Do the estimation of sustainable power only once and avoid expensive
> calculation every time the IPA is called. Do the estimation of PID constants
> when there was user update via sysfs to sustainable power.
> 
> The patch set should apply on top next-20201124
> 
> Changes:
> v4:
> - added new function get_sustainable_power() which handles use cases
>    when the value should be estimated again or simply returned
> - added sustainable_power in the power_allocator_params to track if there
>    was a change to sustainable_power by the user via sysfs
> - addressed Daniel's comments that sustainable power set via sysfs should
>    trigger PID coefficients estimation
> - removed 'force' argument from estimate_pid_constants() and make it ready
>    for updates due to new value for sust. power from sysfs
> - abandoned the design from v3 with a single function responsible for
>    estimation both sust. power and PID const. requested by Ionela
> v3 [1]:
> - changed estimate_pid_constants to estimate_tzp_constants and related comments
> - estimate the PID coefficients always together with sust. power
> - added print indicating that we are estimating sust. power and PID const.
> - don't use local variable 'sustainable_power'
> 
> Regards,
> Lukasz Luba
> 
> [1] https://lore.kernel.org/lkml/20201009135850.14727-1-lukasz.luba@arm.com/
> 
> Lukasz Luba (3):
>    thermal: power allocator: change the 'k_i' coefficient estimation
>    thermal: power allocator: refactor sustainable power estimation
>    thermal: power allocator: change the 'k_*' always in
>      estimate_pid_constants()
> 
>   drivers/thermal/gov_power_allocator.c | 76 +++++++++++++++++----------
>   1 file changed, 49 insertions(+), 27 deletions(-)
> 

Gentle ping. This is a self contained change to only power allocator
file. It addresses also your requirement regarding sustainable_power
changed via sysfs.

Could you take it please? It should apply smoothly in your tree.

Regards,
Lukasz

Re: [PATCH v4 0/3] Improve the estimations in Intelligent Power Allocation

[Daniel Lezcano]

On 26/11/2020 13:49, Lukasz Luba wrote:
> Hi Daniel,
> 
> On 11/24/20 4:10 PM, Lukasz Luba wrote:
>> Hi all,
>>
>> The Intelligent Power Allocation (IPA) estimates the needed
>> coefficients for
>> internal algorithm. It can also estimate the sustainable power value
>> when the
>> DT has not provided one. Fix the 'k_i' coefficient which might be to big
>> related to the other values, when the sustainable power is in an abstract
>> scale. Do the estimation of sustainable power only once and avoid
>> expensive
>> calculation every time the IPA is called. Do the estimation of PID
>> constants
>> when there was user update via sysfs to sustainable power.
>>
>> The patch set should apply on top next-20201124
>>
>> Changes:
>> v4:
>> - added new function get_sustainable_power() which handles use cases
>>    when the value should be estimated again or simply returned
>> - added sustainable_power in the power_allocator_params to track if there
>>    was a change to sustainable_power by the user via sysfs
>> - addressed Daniel's comments that sustainable power set via sysfs should
>>    trigger PID coefficients estimation
>> - removed 'force' argument from estimate_pid_constants() and make it
>> ready
>>    for updates due to new value for sust. power from sysfs
>> - abandoned the design from v3 with a single function responsible for
>>    estimation both sust. power and PID const. requested by Ionela
>> v3 [1]:
>> - changed estimate_pid_constants to estimate_tzp_constants and related
>> comments
>> - estimate the PID coefficients always together with sust. power
>> - added print indicating that we are estimating sust. power and PID
>> const.
>> - don't use local variable 'sustainable_power'
>>
>> Regards,
>> Lukasz Luba
>>
>> [1]
>> https://lore.kernel.org/lkml/20201009135850.14727-1-lukasz.luba@arm.com/
>>
>> Lukasz Luba (3):
>>    thermal: power allocator: change the 'k_i' coefficient estimation
>>    thermal: power allocator: refactor sustainable power estimation
>>    thermal: power allocator: change the 'k_*' always in
>>      estimate_pid_constants()
>>
>>   drivers/thermal/gov_power_allocator.c | 76 +++++++++++++++++----------
>>   1 file changed, 49 insertions(+), 27 deletions(-)
>>
> 
> Gentle ping. This is a self contained change to only power allocator
> file. It addresses also your requirement regarding sustainable_power
> changed via sysfs.
> 
> Could you take it please? It should apply smoothly in your tree.

Actually, I'm waiting for Ionela and Dietmar ack.


-- 
<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 v4 0/3] Improve the estimations in Intelligent Power Allocation

[Lukasz Luba]

On 11/26/20 1:09 PM, Daniel Lezcano wrote:
> On 26/11/2020 13:49, Lukasz Luba wrote:
>> Hi Daniel,
>>
>> On 11/24/20 4:10 PM, Lukasz Luba wrote:
>>> Hi all,
>>>
>>> The Intelligent Power Allocation (IPA) estimates the needed
>>> coefficients for
>>> internal algorithm. It can also estimate the sustainable power value
>>> when the
>>> DT has not provided one. Fix the 'k_i' coefficient which might be to big
>>> related to the other values, when the sustainable power is in an abstract
>>> scale. Do the estimation of sustainable power only once and avoid
>>> expensive
>>> calculation every time the IPA is called. Do the estimation of PID
>>> constants
>>> when there was user update via sysfs to sustainable power.
>>>
>>> The patch set should apply on top next-20201124
>>>
>>> Changes:
>>> v4:
>>> - added new function get_sustainable_power() which handles use cases
>>>     when the value should be estimated again or simply returned
>>> - added sustainable_power in the power_allocator_params to track if there
>>>     was a change to sustainable_power by the user via sysfs
>>> - addressed Daniel's comments that sustainable power set via sysfs should
>>>     trigger PID coefficients estimation
>>> - removed 'force' argument from estimate_pid_constants() and make it
>>> ready
>>>     for updates due to new value for sust. power from sysfs
>>> - abandoned the design from v3 with a single function responsible for
>>>     estimation both sust. power and PID const. requested by Ionela
>>> v3 [1]:
>>> - changed estimate_pid_constants to estimate_tzp_constants and related
>>> comments
>>> - estimate the PID coefficients always together with sust. power
>>> - added print indicating that we are estimating sust. power and PID
>>> const.
>>> - don't use local variable 'sustainable_power'
>>>
>>> Regards,
>>> Lukasz Luba
>>>
>>> [1]
>>> https://lore.kernel.org/lkml/20201009135850.14727-1-lukasz.luba@arm.com/
>>>
>>> Lukasz Luba (3):
>>>     thermal: power allocator: change the 'k_i' coefficient estimation
>>>     thermal: power allocator: refactor sustainable power estimation
>>>     thermal: power allocator: change the 'k_*' always in
>>>       estimate_pid_constants()
>>>
>>>    drivers/thermal/gov_power_allocator.c | 76 +++++++++++++++++----------
>>>    1 file changed, 49 insertions(+), 27 deletions(-)
>>>
>>
>> Gentle ping. This is a self contained change to only power allocator
>> file. It addresses also your requirement regarding sustainable_power
>> changed via sysfs.
>>
>> Could you take it please? It should apply smoothly in your tree.
> 
> Actually, I'm waiting for Ionela and Dietmar ack.
> 
> 

Are they maintainers of this file that you need their ACKs?
Maybe I should drop mine then.

Re: [PATCH v4 0/3] Improve the estimations in Intelligent Power Allocation

[Daniel Lezcano]

On 26/11/2020 15:02, Lukasz Luba wrote:

[ ... ]

>>> changed via sysfs.
>>>
>>> Could you take it please? It should apply smoothly in your tree.
>>
>> Actually, I'm waiting for Ionela and Dietmar ack.
>>
>>
> 
> Are they maintainers of this file that you need their ACKs?
> Maybe I should drop mine then.

Ok let me clarify :)

In general when someone comments on the changes, I usually wait for the
consensus before merging the patches. If the persons who commented
before are unresponsive, depending on the context and my perception, I
apply the changes or not.

I'm giving the opportunity to Ionela to review the series again as she
commented the previous version (and gave a reviewed-by). I thought also
Dietmar commented the series but apparently I was wrong.

As you stated, you are the maintainer of the subsystem, so if there are
no acked-by or reviewed-by, the series will be applied anyway soon.

Meanwhile, they are in the 'testing' branch of the tree, the antechamber
of 'linux-next' and 'next' ;)





-- 
<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 v4 0/3] Improve the estimations in Intelligent Power Allocation

[Lukasz Luba]

On 11/26/20 2:30 PM, Daniel Lezcano wrote:
> On 26/11/2020 15:02, Lukasz Luba wrote:
> 
> [ ... ]
> 
>>>> changed via sysfs.
>>>>
>>>> Could you take it please? It should apply smoothly in your tree.
>>>
>>> Actually, I'm waiting for Ionela and Dietmar ack.
>>>
>>>
>>
>> Are they maintainers of this file that you need their ACKs?
>> Maybe I should drop mine then.
> 
> Ok let me clarify :)
> 
> In general when someone comments on the changes, I usually wait for the
> consensus before merging the patches. If the persons who commented
> before are unresponsive, depending on the context and my perception, I
> apply the changes or not.
> 
> I'm giving the opportunity to Ionela to review the series again as she
> commented the previous version (and gave a reviewed-by). I thought also
> Dietmar commented the series but apparently I was wrong.
> 
> As you stated, you are the maintainer of the subsystem, so if there are
> no acked-by or reviewed-by, the series will be applied anyway soon.
> 
> Meanwhile, they are in the 'testing' branch of the tree, the antechamber
> of 'linux-next' and 'next' ;)
> 

Thank you clarifying this and taking the patches into the testing
branch.

Regards,
Lukasz

Re: [PATCH v4 2/3] thermal: power allocator: refactor sustainable power estimation

[Ionela Voinescu]

Hey,

Mostly trivial nits (added in case you want to consider them for this code
or future changes).

On Tuesday 24 Nov 2020 at 16:10:24 (+0000), Lukasz Luba wrote:
> The sustainable power value might come from the Device Tree or can be
> estimated in run time. The sustainable power might be updated by the user
> via sysfs interface, which should trigger new estimation of PID
> coefficients. There is no need to estimate it 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 | 52 ++++++++++++++++++++-------
>  1 file changed, 40 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index 2e20085ed217..d7e4b9f6af60 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -63,6 +63,8 @@ static inline s64 div_frac(s64 x, s64 y)
>   * @trip_max_desired_temperature:	last passive trip point of the thermal
>   *					zone.  The temperature we are
>   *					controlling for.
> + * @sustainable_power:	Sustainable power (heat) that this thermal zone can
> + *			dissipate
>   */
>  struct power_allocator_params {
>  	bool allocated_tzp;
> @@ -70,6 +72,7 @@ struct power_allocator_params {
>  	s32 prev_err;
>  	int trip_switch_on;
>  	int trip_max_desired_temperature;
> +	u32 sustainable_power;
>  };
>  
>  /**
> @@ -118,10 +121,6 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
>   *
>   * 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
> @@ -171,6 +170,42 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
>  	 */
>  }
>  
> +/**
> + * get_sustainable_power() - Get the right sustainable power
                                    ^^^^^^^^^
Nit: I would not say there is a right sustainable power. I would remove
this.

> + * @tz:		thermal zone for which to estimate the constants
> + * @params:	parameters for the power allocator governor
> + * @control_temp:	target temperature for the power allocator governor
> + *
> + * This function is used for getting the proper sustainable power value based
> + * on variables which might be updated by the user sysfs interface. If that
                                          ^^
					  through
> + * happen the new value is going to be estimated and updated. It is also used

Nit: "If that happens, the new.."

> + * after thermal zone binding, where the initial values where set to 0.
                                                    ^^^^^^^^^^^^^^^^^^^^^
						    value could be 0.
> + */

Nit: I think the code is self explanatory so you might not need to go
into so many details in the description.

> +static u32 get_sustainable_power(struct thermal_zone_device *tz,
> +				 struct power_allocator_params *params,
> +				 int control_temp)
> +{
> +	u32 sustainable_power;
> +

Given that we call this every time the controller kicks in, it might
help to add unlikely to both conditions. I think the most likely
scenario is for our stored params->sustainable_power and
tz->tzp->sustainable_power to match.

> +	if (!tz->tzp->sustainable_power)
> +		sustainable_power = estimate_sustainable_power(tz);
> +	else
> +		sustainable_power = tz->tzp->sustainable_power;
> +
> +	/* Check if it's init value 0 or there was update via sysfs */
> +	if (sustainable_power != params->sustainable_power) {
> +		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;
> +		params->sustainable_power = sustainable_power;
> +	}
> +
> +	return sustainable_power;
> +}
> +
>  /**
>   * pid_controller() - PID controller
>   * @tz:	thermal zone we are operating in
> @@ -200,14 +235,7 @@ static u32 pid_controller(struct thermal_zone_device *tz,
>  
>  	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);
> -	}
> +	sustainable_power = get_sustainable_power(tz, params, control_temp);
>  
>  	err = control_temp - tz->temperature;
>  	err = int_to_frac(err);
> -- 

The logic seems sane so:

Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>

Thank you,
Ionela.

> 2.17.1
> 

Re: [PATCH v4 1/3] thermal: power allocator: change the 'k_i' coefficient estimation

[Ionela Voinescu]

Hi Lukasz,

On Tuesday 24 Nov 2020 at 16:10:23 (+0000), Lukasz Luba wrote:
> Intelligent Power Allocation (IPA) is built around the PID controller
> concept. The initialization code tries to setup the environment based on
> the information available in DT or estimate the value based on minimum
> power reported by each of the cooling device. The estimation will have an
> impact on the PID controller behaviour via the related 'k_po', 'k_pu',
> 'k_i' coefficients and also on the power budget calculation.
> 
> This change prevents the situation when 'k_i' is relatively big compared
> to 'k_po' and 'k_pu' values. This might happen when the estimation for
> 'sustainable_power' returned small value, thus 'k_po' and 'k_pu' are
> small.
> 
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/thermal/gov_power_allocator.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index b29e21c56a4f..2e20085ed217 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -134,6 +134,7 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
>  	int ret;
>  	int switch_on_temp;
>  	u32 temperature_threshold;
> +	s32 k_i;
>  
>  	ret = tz->ops->get_trip_temp(tz, trip_switch_on, &switch_on_temp);
>  	if (ret)
> @@ -159,8 +160,11 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
>  		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;
> +	if (!tz->tzp->k_i || force) {
> +		k_i = tz->tzp->k_pu / 10;
> +		tz->tzp->k_i = k_i > 0 ? k_i : 1;
> +	}
> +
>  	/*
>  	 * The default for k_d and integral_cutoff is 0, so we can
>  	 * leave them as they are.
> -- 

I see this patch did not change so:

Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>

> 2.17.1
> 

Re: [PATCH v4 3/3] thermal: power allocator: change the 'k_*' always in estimate_pid_constants()

[Ionela Voinescu]

On Tuesday 24 Nov 2020 at 16:10:25 (+0000), Lukasz Luba wrote:
> The PID coefficients should be estimated again when there was a change to
> sustainable power value made by user. This change removes unused argument
> 'force' and makes the function ready for such updates.
> 
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/thermal/gov_power_allocator.c | 28 +++++++++------------------
>  1 file changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index d7e4b9f6af60..7a4170a0b51f 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -117,18 +117,13 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
>   * @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.
> - *
> - * 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 control_temp)
>  {
>  	int ret;
>  	int switch_on_temp;
> @@ -151,18 +146,14 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
>  	if (!temperature_threshold)
>  		return;
>  
> -	if (!tz->tzp->k_po || force)
> -		tz->tzp->k_po = int_to_frac(sustainable_power) /
> -			temperature_threshold;
> +	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;
> +	tz->tzp->k_pu = int_to_frac(2 * sustainable_power) /
> +		temperature_threshold;
>  
> -	if (!tz->tzp->k_i || force) {
> -		k_i = tz->tzp->k_pu / 10;
> -		tz->tzp->k_i = k_i > 0 ? k_i : 1;
> -	}
> +	k_i = tz->tzp->k_pu / 10;
> +	tz->tzp->k_i = k_i > 0 ? k_i : 1;
>  
>  	/*
>  	 * The default for k_d and integral_cutoff is 0, so we can
> @@ -195,8 +186,7 @@ static u32 get_sustainable_power(struct thermal_zone_device *tz,
>  	/* Check if it's init value 0 or there was update via sysfs */
>  	if (sustainable_power != params->sustainable_power) {
>  		estimate_pid_constants(tz, sustainable_power,
> -				       params->trip_switch_on, control_temp,
> -				       true);
> +				       params->trip_switch_on, control_temp);
>  
>  		/* Do the estimation only once and make available in sysfs */
>  		tz->tzp->sustainable_power = sustainable_power;
> @@ -640,7 +630,7 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
>  		if (!ret)
>  			estimate_pid_constants(tz, tz->tzp->sustainable_power,
>  					       params->trip_switch_on,
> -					       control_temp, false);
> +					       control_temp);
>  	}
>  
>  	reset_pid_controller(params);
> -- 

I was actually wondering why we still need force, while reading 2/3.

It looks good!

Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>

> 2.17.1
> 

Re: [PATCH v4 2/3] thermal: power allocator: refactor sustainable power estimation

[Lukasz Luba]

Hi Ionela,

On 11/26/20 3:59 PM, Ionela Voinescu wrote:
> Hey,
> 
> Mostly trivial nits (added in case you want to consider them for this code
> or future changes).
> 
> On Tuesday 24 Nov 2020 at 16:10:24 (+0000), Lukasz Luba wrote:
>> The sustainable power value might come from the Device Tree or can be
>> estimated in run time. The sustainable power might be updated by the user
>> via sysfs interface, which should trigger new estimation of PID
>> coefficients. There is no need to estimate it 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 | 52 ++++++++++++++++++++-------
>>   1 file changed, 40 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
>> index 2e20085ed217..d7e4b9f6af60 100644
>> --- a/drivers/thermal/gov_power_allocator.c
>> +++ b/drivers/thermal/gov_power_allocator.c
>> @@ -63,6 +63,8 @@ static inline s64 div_frac(s64 x, s64 y)
>>    * @trip_max_desired_temperature:	last passive trip point of the thermal
>>    *					zone.  The temperature we are
>>    *					controlling for.
>> + * @sustainable_power:	Sustainable power (heat) that this thermal zone can
>> + *			dissipate
>>    */
>>   struct power_allocator_params {
>>   	bool allocated_tzp;
>> @@ -70,6 +72,7 @@ struct power_allocator_params {
>>   	s32 prev_err;
>>   	int trip_switch_on;
>>   	int trip_max_desired_temperature;
>> +	u32 sustainable_power;
>>   };
>>   
>>   /**
>> @@ -118,10 +121,6 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
>>    *
>>    * 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
>> @@ -171,6 +170,42 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
>>   	 */
>>   }
>>   
>> +/**
>> + * get_sustainable_power() - Get the right sustainable power
>                                      ^^^^^^^^^
> Nit: I would not say there is a right sustainable power. I would remove
> this.

I meant the 'right' at that moment in time (because value can change).

> 
>> + * @tz:		thermal zone for which to estimate the constants
>> + * @params:	parameters for the power allocator governor
>> + * @control_temp:	target temperature for the power allocator governor
>> + *
>> + * This function is used for getting the proper sustainable power value based
>> + * on variables which might be updated by the user sysfs interface. If that
>                                            ^^
> 					  through
>> + * happen the new value is going to be estimated and updated. It is also used
> 
> Nit: "If that happens, the new.."
> 
>> + * after thermal zone binding, where the initial values where set to 0.
>                                                      ^^^^^^^^^^^^^^^^^^^^^
> 						    value could be 0.

I meant many variables in the struct which was kzalloc'ed

>> + */
> 
> Nit: I think the code is self explanatory so you might not need to go
> into so many details in the description.
> 
>> +static u32 get_sustainable_power(struct thermal_zone_device *tz,
>> +				 struct power_allocator_params *params,
>> +				 int control_temp)
>> +{
>> +	u32 sustainable_power;
>> +
> 
> Given that we call this every time the controller kicks in, it might
> help to add unlikely to both conditions. I think the most likely
> scenario is for our stored params->sustainable_power and
> tz->tzp->sustainable_power to match.

No, because it returned recently and Greg was explicit [1]:

'Unless you can benchmark the benifit of using likely/unlikely, do not
use it, as the compiler/CPU will do it better for you.'


> 
>> +	if (!tz->tzp->sustainable_power)
>> +		sustainable_power = estimate_sustainable_power(tz);
>> +	else
>> +		sustainable_power = tz->tzp->sustainable_power;
>> +
>> +	/* Check if it's init value 0 or there was update via sysfs */
>> +	if (sustainable_power != params->sustainable_power) {
>> +		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;
>> +		params->sustainable_power = sustainable_power;
>> +	}
>> +
>> +	return sustainable_power;
>> +}
>> +
>>   /**
>>    * pid_controller() - PID controller
>>    * @tz:	thermal zone we are operating in
>> @@ -200,14 +235,7 @@ static u32 pid_controller(struct thermal_zone_device *tz,
>>   
>>   	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);
>> -	}
>> +	sustainable_power = get_sustainable_power(tz, params, control_temp);
>>   
>>   	err = control_temp - tz->temperature;
>>   	err = int_to_frac(err);
>> -- 
> 
> The logic seems sane so:
> 
> Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>
> 
> Thank you,
> Ionela.
> 


Thank you for the review.

Regards,
Lukasz

[1] https://lore.kernel.org/lkml/X7P4lA1nITo58eFT@kroah.com/