* [PATCH v3 0/2] Improve the estimations in Intelligent Power Allocation
@ 2020-10-09 13:58 Lukasz Luba
2020-10-09 13:58 ` [PATCH v3 1/2] thermal: power allocator: change the 'k_i' coefficient estimation Lukasz Luba
2020-10-09 13:58 ` [PATCH v3 2/2] thermal: power allocator: change how estimation code is called Lukasz Luba
0 siblings, 2 replies; 6+ messages in thread
From: Lukasz Luba @ 2020-10-09 13:58 UTC (permalink / raw)
To: linux-kernel, linux-pm
Cc: daniel.lezcano, amitk, Dietmar.Eggemann, lukasz.luba, ionela.voinescu
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 only once and avoid expensive calculation every time
the IPA is called.
The patch set should apply on top of patches adding upper and lower bound for
power actors [1].
Regards,
Lukasz Luba
[1] https://lore.kernel.org/linux-pm/20201007122256.28080-1-lukasz.luba@arm.com/
Lukasz Luba (2):
thermal: power allocator: change the 'k_i' coefficient estimation
thermal: power allocator: change how estimation code is called
drivers/thermal/gov_power_allocator.c | 73 +++++++++++++--------------
1 file changed, 35 insertions(+), 38 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/2] thermal: power allocator: change the 'k_i' coefficient estimation
2020-10-09 13:58 [PATCH v3 0/2] Improve the estimations in Intelligent Power Allocation Lukasz Luba
@ 2020-10-09 13:58 ` Lukasz Luba
2020-10-09 13:58 ` [PATCH v3 2/2] thermal: power allocator: change how estimation code is called Lukasz Luba
1 sibling, 0 replies; 6+ messages in thread
From: Lukasz Luba @ 2020-10-09 13:58 UTC (permalink / raw)
To: linux-kernel, linux-pm
Cc: daniel.lezcano, amitk, Dietmar.Eggemann, lukasz.luba, ionela.voinescu
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.
Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>
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 e566806f1550..aa35aa6c561c 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -132,6 +132,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)
@@ -157,8 +158,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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] thermal: power allocator: change how estimation code is called
2020-10-09 13:58 [PATCH v3 0/2] Improve the estimations in Intelligent Power Allocation Lukasz Luba
2020-10-09 13:58 ` [PATCH v3 1/2] thermal: power allocator: change the 'k_i' coefficient estimation Lukasz Luba
@ 2020-10-09 13:58 ` Lukasz Luba
2020-10-09 14:23 ` Ionela Voinescu
2020-10-13 16:41 ` Daniel Lezcano
1 sibling, 2 replies; 6+ messages in thread
From: Lukasz Luba @ 2020-10-09 13:58 UTC (permalink / raw)
To: linux-kernel, linux-pm
Cc: daniel.lezcano, amitk, Dietmar.Eggemann, lukasz.luba, ionela.voinescu
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. Re-invoke the estimation only in case the
sustainable power was set to 0. Apart from that the PID coefficients
are not going to be force updated thus can better handle sysfs settings.
Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
v3:
- 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'
drivers/thermal/gov_power_allocator.c | 65 ++++++++++++---------------
1 file changed, 29 insertions(+), 36 deletions(-)
diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index aa35aa6c561c..e92a8d3ca5d4 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -96,6 +96,9 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
if (instance->trip != params->trip_max_desired_temperature)
continue;
+ if (!cdev_is_power_actor(cdev))
+ continue;
+
if (cdev->ops->state2power(cdev, tz, instance->upper,
&min_power))
continue;
@@ -107,40 +110,37 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
}
/**
- * estimate_pid_constants() - Estimate the constants for the PID controller
+ * estimate_tzp_constants() - Estimate sustainable power and PID constants
* @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.
+ * This function is used to estimate the sustainable power and PID controller
+ * constants in struct thermal_zone_parameters. These estimations will then be
+ * available in the sysfs.
*/
-static void estimate_pid_constants(struct thermal_zone_device *tz,
- u32 sustainable_power, int trip_switch_on,
- int control_temp, bool force)
+static void estimate_tzp_constants(struct thermal_zone_device *tz,
+ int trip_switch_on, int control_temp)
{
- int ret;
- int switch_on_temp;
u32 temperature_threshold;
+ int switch_on_temp;
+ bool force = false;
+ int ret;
s32 k_i;
+ if (!tz->tzp->sustainable_power) {
+ tz->tzp->sustainable_power = estimate_sustainable_power(tz);
+ force = true;
+ dev_info(&tz->device, "power_allocator: estimating sust. power and PID constants\n");
+ }
+
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;
/*
- * estimate_pid_constants() tries to find appropriate default
+ * estimate_tzp_constants() tries to find appropriate default
* values for thermal zones that don't provide them. If a
* system integrator has configured a thermal zone with two
* passive trip points at the same temperature, that person
@@ -151,11 +151,11 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
return;
if (!tz->tzp->k_po || force)
- tz->tzp->k_po = int_to_frac(sustainable_power) /
+ tz->tzp->k_po = int_to_frac(tz->tzp->sustainable_power) /
temperature_threshold;
if (!tz->tzp->k_pu || force)
- tz->tzp->k_pu = int_to_frac(2 * sustainable_power) /
+ tz->tzp->k_pu = int_to_frac(2 * tz->tzp->sustainable_power) /
temperature_threshold;
if (!tz->tzp->k_i || force) {
@@ -193,19 +193,13 @@ 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);
- }
+ if (!tz->tzp->sustainable_power)
+ estimate_tzp_constants(tz, params->trip_switch_on,
+ control_temp);
err = control_temp - tz->temperature;
err = int_to_frac(err);
@@ -244,7 +238,7 @@ static u32 pid_controller(struct thermal_zone_device *tz,
power_range = p + i + d;
/* feed-forward the known sustainable dissipatable power */
- power_range = sustainable_power + frac_to_int(power_range);
+ power_range = tz->tzp->sustainable_power + frac_to_int(power_range);
power_range = clamp(power_range, (s64)0, (s64)max_allocatable_power);
@@ -603,20 +597,19 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
get_governor_trips(tz, params);
+ tz->governor_data = params;
+
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_tzp_constants(tz, params->trip_switch_on,
+ control_temp);
}
reset_pid_controller(params);
- tz->governor_data = params;
-
return 0;
free_params:
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] thermal: power allocator: change how estimation code is called
2020-10-09 13:58 ` [PATCH v3 2/2] thermal: power allocator: change how estimation code is called Lukasz Luba
@ 2020-10-09 14:23 ` Ionela Voinescu
2020-10-13 16:41 ` Daniel Lezcano
1 sibling, 0 replies; 6+ messages in thread
From: Ionela Voinescu @ 2020-10-09 14:23 UTC (permalink / raw)
To: Lukasz Luba
Cc: linux-kernel, linux-pm, daniel.lezcano, amitk, Dietmar.Eggemann
On Friday 09 Oct 2020 at 14:58:50 (+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. Re-invoke the estimation only in case the
> sustainable power was set to 0. Apart from that the PID coefficients
> are not going to be force updated thus can better handle sysfs settings.
>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>
> v3:
> - 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'
>
>
> drivers/thermal/gov_power_allocator.c | 65 ++++++++++++---------------
> 1 file changed, 29 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index aa35aa6c561c..e92a8d3ca5d4 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -96,6 +96,9 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
> if (instance->trip != params->trip_max_desired_temperature)
> continue;
>
> + if (!cdev_is_power_actor(cdev))
> + continue;
> +
> if (cdev->ops->state2power(cdev, tz, instance->upper,
> &min_power))
> continue;
> @@ -107,40 +110,37 @@ static u32 estimate_sustainable_power(struct thermal_zone_device *tz)
> }
>
> /**
> - * estimate_pid_constants() - Estimate the constants for the PID controller
> + * estimate_tzp_constants() - Estimate sustainable power and PID constants
> * @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.
> + * This function is used to estimate the sustainable power and PID controller
> + * constants in struct thermal_zone_parameters. These estimations will then be
> + * available in the sysfs.
> */
> -static void estimate_pid_constants(struct thermal_zone_device *tz,
> - u32 sustainable_power, int trip_switch_on,
> - int control_temp, bool force)
> +static void estimate_tzp_constants(struct thermal_zone_device *tz,
> + int trip_switch_on, int control_temp)
> {
> - int ret;
> - int switch_on_temp;
> u32 temperature_threshold;
> + int switch_on_temp;
> + bool force = false;
> + int ret;
> s32 k_i;
>
> + if (!tz->tzp->sustainable_power) {
> + tz->tzp->sustainable_power = estimate_sustainable_power(tz);
> + force = true;
> + dev_info(&tz->device, "power_allocator: estimating sust. power and PID constants\n");
> + }
> +
> 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;
> /*
> - * estimate_pid_constants() tries to find appropriate default
> + * estimate_tzp_constants() tries to find appropriate default
> * values for thermal zones that don't provide them. If a
> * system integrator has configured a thermal zone with two
> * passive trip points at the same temperature, that person
> @@ -151,11 +151,11 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
> return;
>
> if (!tz->tzp->k_po || force)
> - tz->tzp->k_po = int_to_frac(sustainable_power) /
> + tz->tzp->k_po = int_to_frac(tz->tzp->sustainable_power) /
> temperature_threshold;
>
> if (!tz->tzp->k_pu || force)
> - tz->tzp->k_pu = int_to_frac(2 * sustainable_power) /
> + tz->tzp->k_pu = int_to_frac(2 * tz->tzp->sustainable_power) /
> temperature_threshold;
>
> if (!tz->tzp->k_i || force) {
> @@ -193,19 +193,13 @@ 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);
> - }
> + if (!tz->tzp->sustainable_power)
> + estimate_tzp_constants(tz, params->trip_switch_on,
> + control_temp);
>
> err = control_temp - tz->temperature;
> err = int_to_frac(err);
> @@ -244,7 +238,7 @@ static u32 pid_controller(struct thermal_zone_device *tz,
> power_range = p + i + d;
>
> /* feed-forward the known sustainable dissipatable power */
> - power_range = sustainable_power + frac_to_int(power_range);
> + power_range = tz->tzp->sustainable_power + frac_to_int(power_range);
>
> power_range = clamp(power_range, (s64)0, (s64)max_allocatable_power);
>
> @@ -603,20 +597,19 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
>
> get_governor_trips(tz, params);
>
> + tz->governor_data = params;
> +
> 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_tzp_constants(tz, params->trip_switch_on,
> + control_temp);
> }
>
> reset_pid_controller(params);
>
> - tz->governor_data = params;
> -
> return 0;
>
> free_params:
> --
> 2.17.1
>
Awesome!
Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] thermal: power allocator: change how estimation code is called
2020-10-09 13:58 ` [PATCH v3 2/2] thermal: power allocator: change how estimation code is called Lukasz Luba
2020-10-09 14:23 ` Ionela Voinescu
@ 2020-10-13 16:41 ` Daniel Lezcano
2020-10-29 9:24 ` Lukasz Luba
1 sibling, 1 reply; 6+ messages in thread
From: Daniel Lezcano @ 2020-10-13 16:41 UTC (permalink / raw)
To: Lukasz Luba, linux-kernel, linux-pm
Cc: amitk, Dietmar.Eggemann, ionela.voinescu
On 09/10/2020 15:58, 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. Re-invoke the estimation only in case the
> sustainable power was set to 0. Apart from that the PID coefficients
> are not going to be force updated thus can better handle sysfs settings.
>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
[ ... ]
> -static void estimate_pid_constants(struct thermal_zone_device *tz,
> - u32 sustainable_power, int trip_switch_on,
> - int control_temp, bool force)
> +static void estimate_tzp_constants(struct thermal_zone_device *tz,
> + int trip_switch_on, int control_temp)
> {
> - int ret;
> - int switch_on_temp;
> u32 temperature_threshold;
> + int switch_on_temp;
> + bool force = false;
> + int ret;
> s32 k_i;
>
> + if (!tz->tzp->sustainable_power) {
> + tz->tzp->sustainable_power = estimate_sustainable_power(tz);
> + force = true;
> + dev_info(&tz->device, "power_allocator: estimating sust. power and PID constants\n");
> + }
> +
> 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;
> /*
> - * estimate_pid_constants() tries to find appropriate default
> + * estimate_tzp_constants() tries to find appropriate default
> * values for thermal zones that don't provide them. If a
> * system integrator has configured a thermal zone with two
> * passive trip points at the same temperature, that person
> @@ -151,11 +151,11 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
> return;
>
> if (!tz->tzp->k_po || force)
> - tz->tzp->k_po = int_to_frac(sustainable_power) /
> + tz->tzp->k_po = int_to_frac(tz->tzp->sustainable_power) /
> temperature_threshold;
>
> if (!tz->tzp->k_pu || force)
> - tz->tzp->k_pu = int_to_frac(2 * sustainable_power) /
> + tz->tzp->k_pu = int_to_frac(2 * tz->tzp->sustainable_power) /
> temperature_threshold;
>
> if (!tz->tzp->k_i || force) {
> @@ -193,19 +193,13 @@ 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);
> - }
> + if (!tz->tzp->sustainable_power)
> + estimate_tzp_constants(tz, params->trip_switch_on,
> + control_temp);
The changes in this patch are appropriate and make sense but they are
not done at the right place.
estimate_tzp_constants() must be called when sustainable_power is
updated via DT/init or sysfs.
Keeping a function to estimate the sustainable power and another one to
estimate the k_* separated would be more clear.
Actually the confusion is coming from when the pid constants are
computed, I suggest moving the initialization of k_* out of this
function and killing the 'force' test.
[ ... ]
--
<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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] thermal: power allocator: change how estimation code is called
2020-10-13 16:41 ` Daniel Lezcano
@ 2020-10-29 9:24 ` Lukasz Luba
0 siblings, 0 replies; 6+ messages in thread
From: Lukasz Luba @ 2020-10-29 9:24 UTC (permalink / raw)
To: Daniel Lezcano, linux-kernel, linux-pm
Cc: amitk, Dietmar.Eggemann, ionela.voinescu
Hi Daniel,
On 10/13/20 5:41 PM, Daniel Lezcano wrote:
> On 09/10/2020 15:58, 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. Re-invoke the estimation only in case the
>> sustainable power was set to 0. Apart from that the PID coefficients
>> are not going to be force updated thus can better handle sysfs settings.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>
> [ ... ]
>
>> -static void estimate_pid_constants(struct thermal_zone_device *tz,
>> - u32 sustainable_power, int trip_switch_on,
>> - int control_temp, bool force)
>> +static void estimate_tzp_constants(struct thermal_zone_device *tz,
>> + int trip_switch_on, int control_temp)
>> {
>> - int ret;
>> - int switch_on_temp;
>> u32 temperature_threshold;
>> + int switch_on_temp;
>> + bool force = false;
>> + int ret;
>> s32 k_i;
>>
>> + if (!tz->tzp->sustainable_power) {
>> + tz->tzp->sustainable_power = estimate_sustainable_power(tz);
>> + force = true;
>> + dev_info(&tz->device, "power_allocator: estimating sust. power and PID constants\n");
>> + }
>> +
>> 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;
>> /*
>> - * estimate_pid_constants() tries to find appropriate default
>> + * estimate_tzp_constants() tries to find appropriate default
>> * values for thermal zones that don't provide them. If a
>> * system integrator has configured a thermal zone with two
>> * passive trip points at the same temperature, that person
>> @@ -151,11 +151,11 @@ static void estimate_pid_constants(struct thermal_zone_device *tz,
>> return;
>>
>> if (!tz->tzp->k_po || force)
>> - tz->tzp->k_po = int_to_frac(sustainable_power) /
>> + tz->tzp->k_po = int_to_frac(tz->tzp->sustainable_power) /
>> temperature_threshold;
>>
>> if (!tz->tzp->k_pu || force)
>> - tz->tzp->k_pu = int_to_frac(2 * sustainable_power) /
>> + tz->tzp->k_pu = int_to_frac(2 * tz->tzp->sustainable_power) /
>> temperature_threshold;
>>
>> if (!tz->tzp->k_i || force) {
>> @@ -193,19 +193,13 @@ 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);
>> - }
>> + if (!tz->tzp->sustainable_power)
>> + estimate_tzp_constants(tz, params->trip_switch_on,
>> + control_temp);
>
> The changes in this patch are appropriate and make sense but they are
> not done at the right place.
>
> estimate_tzp_constants() must be called when sustainable_power is
> updated via DT/init or sysfs.
>
> Keeping a function to estimate the sustainable power and another one to
> estimate the k_* separated would be more clear.
>
> Actually the confusion is coming from when the pid constants are
> computed, I suggest moving the initialization of k_* out of this
> function and killing the 'force' test.
>
>
> [ ... ]
>
>
Thank you for the review. I will re-write the patch
and address your suggestion.
Regards,
Lukasz
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-29 9:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-09 13:58 [PATCH v3 0/2] Improve the estimations in Intelligent Power Allocation Lukasz Luba
2020-10-09 13:58 ` [PATCH v3 1/2] thermal: power allocator: change the 'k_i' coefficient estimation Lukasz Luba
2020-10-09 13:58 ` [PATCH v3 2/2] thermal: power allocator: change how estimation code is called Lukasz Luba
2020-10-09 14:23 ` Ionela Voinescu
2020-10-13 16:41 ` Daniel Lezcano
2020-10-29 9:24 ` Lukasz Luba
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).