Re: [PATCH V5] thermal/core/power_allocator: reset thermal governor when trip point is changed
From: Daniel Lezcano
Date: Mon Jul 10 2023 - 15:33:18 EST
Hi Di,
On 10/07/2023 15:36, Di Shen wrote:
Hi Daniel,
Thank you for your reply.
On Mon, Jul 10, 2023 at 4:59 PM Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx> wrote:
On 10/07/2023 05:32, Di Shen wrote:
When the thermal trip point is changed, the governor should
be reset so that the policy algorithm can be updated to adapt
to the new trip point.
1.Thermal governor is working for the passive trip point or active
trip point. Therefore, when the trip point is changed it should
reset the governor only for passic/active trip points.
2.For "power_allocator" governor reset, the parameters of pid
controller and the states of power cooling devices should be reset.
2.1
The IPA controls temperature using PID mechanism. It is a closed-
loop feedback monitoring system. It uses the gap between target
temperature and current temperature which says "error" as the
input of the PID controller:
err = desired_temperature - current_temperature
P_max =
k_p * err + k_i * err_integral + k_d * diff_err + sustainable_power
That algorithm can 'learn' from the 'observed' in the past reaction
for it's control decisions and accumulates that information in the
I(Integral) part so that it can conpensate for those 'learned'
mistakes.
Based on the above, the most important is the desired temperature
comes from the trip point. When the trip point is changed, all the
previous learned errors won't help for the IPA. So the pid parameters
should be reset for IPA governor reset.
2.2
Other wise, the cooling devices should also be reset when the trip
point is changed.
This patch adds an ops for the thermal_governor structure to reset
the governor and give the 'reset' function definition for power
allocator. The ops is called when the trip points are changed via
sysfs interface.
Signed-off-by: Di Shen <di.shen@xxxxxxxxxx>
---
drivers/thermal/gov_power_allocator.c | 9 +++++++++
drivers/thermal/thermal_trip.c | 5 +++++
include/linux/thermal.h | 3 +++
3 files changed, 17 insertions(+)
diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 8642f1096b91..8d17a10671e4 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -729,10 +729,19 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
return allocate_power(tz, trip.temperature);
}
+static void power_allocator_reset(struct thermal_zone_device *tz)
+{
+ struct power_allocator_params *params = tz->governor_data;
+
+ reset_pid_controller(params);
+ allow_maximum_power(tz, true);
Do you really want to allow the maximum power? What about if the trip
temperature is decreased ?
If the trip temperature is decreased, allow_maximum_power will only
be executed once, and then the ipa governor will adapt to the lower trip
temperature and calculate the allocated power for cooling actors again.
Right?
Sorry for jumping in this fifth version but I'm not sure about resetting
the change is the right way (and probably, changing a trip point with
the power allocator is not a good idea)
The platforms where the IPA is planned to be used are creating a dummy
trip point where the IPA begins the acquisition without cooling devices
in order to have the math building the PID schema (eg. hi3660.dtsi).
What about the sustainable power vs the trip point temperature? I mean
we can change the trip temperature but not the sustainable power which
is directly related to the target temperature. So the resulting power
computation will be wrong.
The more I think about that, the more I do believe writable trip point
and IPA are incompatible.
What about forbid that?
For instance, add a set_trip callback instead of resetting in the
governor and return -EPERM from the IPA?
Lukasz ?
But if the trip temperature is increased, it must allow the maximum power,
I would not say allow the maximum temperature but the new power head
room. But that needs an update of the sustainable power information
related to the new temperature :/
otherwise, the Line 723 update flag might be false(see the false example
in patch-v3), the power of cooling devices would be not allowed maximum
for a while unless the current temperature is higher than the new
switch_on_temp. In this situation, for cpufreq cooling devices, it means the
cpu max_freq is limited, which is bad for device performance. The same for
other cooling devices, if not reset the cooling state, it may introduce
unexpected issues.
705 static int power_allocator_throttle(struct thermal_zone_device
*tz, int trip_id)
706 {
707 struct power_allocator_params *params = tz->governor_data;
708 struct thermal_trip trip;
709 int ret;
710 bool update;
711
712 //......
721 ret = __thermal_zone_get_trip(tz, params->trip_switch_on,
&trip);
722 if (!ret && (tz->temperature < trip.temperature)) {
723 update = (tz->last_temperature >=
trip.temperature);
724 tz->passive = 0;
725 reset_pid_controller(params);
726 allow_maximum_power(tz, update);
727 return 0;
728 }
729
730 tz->passive = 1;
731 //.......
740 }
You want maximum power only if the mitigation ends.
Yes, you're right, I agree. It's better to do like that, actually,
patch-v3 is in line
with this point I think, but it is not in the thermal core.
Oh, I see!
How about modified like that:
static void power_allocator_reset(struct thermal_zone_device *tz)
{
struct power_allocator_params *params = tz->governor_data;
+ struct thermal_trip trip;
+ int ret;
reset_pid_controller(params);
+ ret = __thermal_zone_get_trip(tz, params->trip_switch_on, &trip);
+ if (!ret && (tz->temperature < trip.temperature))
allow_maximum_power(tz, true);
No, that would be wrong. It should be
(tz->temperature - hysteresis) < trip.temperature
But if we are in the hysteresis area, we should allow a bit more power
which depends on the sustainable power which was not updated with the
trip temperature change :/
}
It seems clearer.
+}
+
static struct thermal_governor thermal_gov_power_allocator = {
.name = "power_allocator",
.bind_to_tz = power_allocator_bind,
.unbind_from_tz = power_allocator_unbind,
.throttle = power_allocator_throttle,
+ .reset = power_allocator_reset,
};
THERMAL_GOVERNOR_DECLARE(thermal_gov_power_allocator);
diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
index 907f3a4d7bc8..13bbe029c6ab 100644
--- a/drivers/thermal/thermal_trip.c
+++ b/drivers/thermal/thermal_trip.c
@@ -173,6 +173,11 @@ int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis))
tz->trips[trip_id] = *trip;
+ /* Reset the governor only when the trip type is active or passive. */
+ ret = (trip->type == THERMAL_TRIP_PASSIVE || trip->type == THERMAL_TRIP_ACTIVE);
Actually we have the trip points:
ACTIVE, PASSIVE, HOT and CRITICAL
Correct.
The last two ones should not be writable.
Instead of this test, it would be cleaner to only make the ACTIVE and
PASSIVE trip point writable when the CONFIG_THERMAL_WRITABLE_TRIPS
option is set. Consequently, other trip points won't be writable and
this test can go away as set_trip will be protected by a RO sysfs file
property.
+ if (ret && t.temperature != trip->temperature && tz->governor && tz->governor->reset)
The temperature test is duplicated because it is already done in the
block before.
So here you mean two points:
1. Make the HOT and CRITICAL point RO so that it can no trip type test
when gov->reset
2. not do the temperature test twice
Right?
HOT and CRITICAL can not be changed because RO. The core code can use
this assumption by not checking they are changed.
For the first point, it's OK for me. But what if someone wants to
power off the device earlier
or later? I think it should have the access to update the critical
temperature.
>
I'm not really
know about the hot trip temperature. So I'm not sure the real reason
to make them RO
when the trip points are writable. I can't make it RO just because of
the code simplification.
That is critical firmware information and it must not be touched by the
user (even root). Imagine I can set 250°C for the critical trip point
while the silicon supports 120°C only?
So basically, we should protect these trip points by making them RO.
By fixing this, only PASSIVE and ACTIVE trip points can be changed, thus
checking CRITICAL and HOT above is pointless.
For the second point, that's OK. I will simplify in the next patch
version. Thank you, Daniel.
Before sending a new version, IMO we should think a bit about writable
trip points in general because the feature does not look mature.
+ tz->governor->reset(tz);
+
thermal_notify_tz_trip_change(tz->id, trip_id, trip->type,
trip->temperature, trip->hysteresis);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 87837094d549..d27d053319bf 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -197,6 +197,8 @@ struct thermal_zone_device {
* thermal zone.
* @throttle: callback called for every trip point even if temperature is
* below the trip point temperature
+ * @reset: callback called for governor reset
+ *
* @governor_list: node in thermal_governor_list (in thermal_core.c)
*/
struct thermal_governor {
@@ -204,6 +206,7 @@ struct thermal_governor {
int (*bind_to_tz)(struct thermal_zone_device *tz);
void (*unbind_from_tz)(struct thermal_zone_device *tz);
int (*throttle)(struct thermal_zone_device *tz, int trip);
+ void (*reset)(struct thermal_zone_device *tz);
struct list_head governor_list;
};
--
<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
Best regards,
Di
--
<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