diff mbox series

thermal/governors: Remove integral_cutoff parameter, IPA is more regulated

Message ID 20221013090733.28462-1-huangqibo.tech@gmail.com
State New
Headers
Series thermal/governors: Remove integral_cutoff parameter, IPA is more regulated |

Commit Message

Qibo Huang Oct. 13, 2022, 9:07 a.m. UTC 
  Reason 1: If the integral_cutoff parameter is 0,
the current temperature is greater than the control
temperature, and err_integral continues to increase.
If an abnormal situation occurs suddenly, the err_integral
value will become very large. Even if the current
temperature is lower than the control temperature,
err_integral will always exist, which will cause the
IPA to run out of control and cannot return to normal.
This is the problem I'm actually having.

Reason 2: The integral_cutoff parameter is difficult to
confirm the optimal, and can not bring good results.

Signed-off-by: Qibo Huang <huangqibo.tech@gmail.com>
---
 drivers/thermal/gov_power_allocator.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Comments

Lukasz Luba Oct. 13, 2022, 3:58 p.m. UTC | #1 
Hi Qibo,

On 10/13/22 10:07, Qibo Huang wrote:
> Reason 1: If the integral_cutoff parameter is 0,
> the current temperature is greater than the control
> temperature, and err_integral continues to increase.
> If an abnormal situation occurs suddenly, the err_integral
> value will become very large. Even if the current
> temperature is lower than the control temperature,
> err_integral will always exist, which will cause the
> IPA to run out of control and cannot return to normal.
> This is the problem I'm actually having.
> 
> Reason 2: The integral_cutoff parameter is difficult to
> confirm the optimal, and can not bring good results.

Have you tired different 'k_i', 'k_po', 'k_pu' settings,
not the default from binding estimation?

These coefficients help to control the behavior of IPA
and how good the control loop could work.

The integral cutoff in default is set to 0, which means
we account the error when we overshoot the temperature
vs our desired control temperature value.

Please also pay attention to the type for those variables:
err, i, p and how they are treated (and when).
In current implementation we account 'err' negative, so we
reduce the power budget. Next time, even when we are below
control temperature, we still 'remember' those
overshoot mistakes, so we upfront shrink the possible
budget (to avoid overshooting). We clean the accumulated
error when the temperature drops below 'enable' trip point.

Could you describe a bit more what kind of problems you
observe, please?

Regards,
Lukasz
diff mbox series

Patch

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 2bdf8d797e3c..87e87ce71ff3 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -221,6 +221,7 @@  static u32 pid_controller(struct thermal_zone_device *tz,
 	s64 p, i, d, power_range;
 	s32 err, max_power_frac;
 	u32 sustainable_power;
+	s64 i_next;
 	struct power_allocator_params *params = tz->governor_data;
 
 	max_power_frac = int_to_frac(max_allocatable_power);
@@ -241,13 +242,11 @@  static u32 pid_controller(struct thermal_zone_device *tz,
 	 */
 	i = mul_frac(tz->tzp->k_i, params->err_integral);
 
-	if (err < int_to_frac(tz->tzp->integral_cutoff)) {
-		s64 i_next = i + mul_frac(tz->tzp->k_i, err);
+	i_next = i + mul_frac(tz->tzp->k_i, err);
 
-		if (abs(i_next) < max_power_frac) {
-			i = i_next;
-			params->err_integral += err;
-		}
+	if (abs(i_next) < max_power_frac) {
+		i = i_next;
+		params->err_integral += err;
 	}
 
 	/*