drivers: thermal: step_wise: add support for hysteresis
Subsystems (2)
PATCH 1/1 drivers: thermal: step_wise: add support for hysteresis
From: Ram Chandrasekar <[email protected]>
From: Ram Chandrasekar <[email protected]>
Step wise governor increases the mitigation level when the temperature goes above a threshold and will decrease the mitigation when the temperature falls below the threshold. If it were a case, where the temperature hovers around a threshold, the mitigation will be applied and removed at every iteration. This reaction to the temperature is inefficient for performance.
The use of hysteresis temperature could avoid this ping-pong of mitigation by relaxing the mitigation to happen only when the temperature goes below this lower hysteresis value.
--- drivers/thermal/step_wise.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-)
drivers/thermal/step_wise.c +23 -10
@@ -36,7 +36,7 @@* for this trip point * d. if the trend is THERMAL_TREND_DROP_FULL, use lower limit * for this trip point -* If the temperature is lower than a trip point, +* If the temperature is lower than a hysteresis temperature, * a. if the trend is THERMAL_TREND_RAISING, do nothing * b. if the trend is THERMAL_TREND_DROPPING, use lower cooling * state for this trip point, if the cooling state already
@@ -127,7 +127,7 @@ static void update_passive_instance(struct thermal_zone_device *tz,static void thermal_zone_trip_update (struct thermal_zone_device *tz,int trip) { -int trip_temp; +int trip_temp, hyst_temp;enum thermal_trip_type trip_type;enum thermal_trend trend;struct thermal_instance *instance;
@@ -135,22 +135,23 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)int old_target;if (trip ==THERMAL_TRIPS_NONE ) { - trip_temp = tz->forced_passive; + hyst_temp = trip_temp = tz->forced_passive; trip_type =THERMAL_TRIPS_NONE ; }else { tz->ops->get_trip_temp (tz, trip, &trip_temp); + hyst_temp = trip_temp; +if (tz->ops->get_trip_hyst) { + tz->ops->get_trip_hyst (tz, trip, &hyst_temp); + hyst_temp = trip_temp - hyst_temp; + } tz->ops->get_trip_type (tz, trip, &trip_type); } trend =get_tz_trend (tz, trip); -if (tz->temperature >= trip_temp) { - throttle = true; -trace_thermal_zone_trip (tz, trip, trip_type); - } - -dev_dbg (&tz->device,"Trip%d[type=%d,temp=%d]:trend=%d,throttle=%d\n" , - trip, trip_type, trip_temp, trend, throttle); +dev_dbg (&tz->device, +"Trip%d[type=%d,temp=%d,hyst=%d]:trend=%d,throttle=%d\n" , + trip, trip_type, trip_temp, hyst_temp, trend, throttle);mutex_lock (&tz->lock);
@@ -159,6 +160,18 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)continue ; old_target = instance->target; + throttle = false; +/* +* Lower the mitigation only if the temperature +* goes below the hysteresis temperature. +*/ +if (tz->temperature >= trip_temp || + (tz->temperature >= hyst_temp && + old_target !=THERMAL_NO_TARGET )) { + throttle = true; +trace_thermal_zone_trip (tz, trip, trip_type); + } + instance->target =get_target_state (instance, trend, throttle);dev_dbg (&instance->cdev->device,"old_target=%d, target=%d\n" , old_target, (int )instance->target);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 11 lines · Lina Iyer
From: Ram Chandrasekar <[email protected]>
From: Ram Chandrasekar <[email protected]>
Step wise governor increases the mitigation level when the temperature goes above a threshold and will decrease the mitigation when the temperature falls below the threshold. If it were a case, where the temperature hovers around a threshold, the mitigation will be applied and removed at every iteration. This reaction to the temperature is inefficient for performance.
Quoted 3 lines
The use of hysteresis temperature could avoid this ping-pong of mitigation by relaxing the mitigation to happen only when the temperature goes below this lower hysteresis value.
I don't disagree with this but the ping-pong around a temperature is usually avoided with a P-I-D computation which is implemented with the IPA governor. Wouldn't be more interesting to add the power numbers like some other platforms, so the IPA could be used?
You will probably have better results with the IPA than changing the step-wise governor behavior (which may potentially impact other users).
Signature · 5 lines
<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
Hi Daniel,
Quoted 12 lines · Daniel Lezcano
Quoted 11 lines · Lina Iyer
From: Ram Chandrasekar <[email protected]>
From: Ram Chandrasekar <[email protected]>
Step wise governor increases the mitigation level when the temperature goes above a threshold and will decrease the mitigation when the temperature falls below the threshold. If it were a case, where the temperature hovers around a threshold, the mitigation will be applied and removed at every iteration. This reaction to the temperature is inefficient for performance.
Quoted 9 lines
Quoted 3 lines
The use of hysteresis temperature could avoid this ping-pong of mitigation by relaxing the mitigation to happen only when the temperature goes below this lower hysteresis value.
I don't disagree with this but the ping-pong around a temperature is usually avoided with a P-I-D computation which is implemented with the IPA governor. Wouldn't be more interesting to add the power numbers like some other platforms, so the IPA could be used?
Possibly. But we have had better thermal performance for our hardware, with stepwise and custom governor. Much of the mitigation happens through the firmware and hardware. The stepwise governor works well for us.
Quoted 3 lines
You will probably have better results with the IPA than changing the step-wise governor behavior (which may potentially impact other users).
This should not impact others who have not implemented the ->get_trip_hyst method.
Thanks,
Lina
Zhang Rui
reviewerHi, Lina,
Quoted 12 lines · Lina Iyer
From: Ram Chandrasekar <[email protected]>
From: Ram Chandrasekar <[email protected]>
Step wise governor increases the mitigation level when the temperature
goes above a threshold and will decrease the mitigation when the temperature falls below the threshold. If it were a case, where the temperature hovers around a threshold, the mitigation will be applied and removed at every iteration. This reaction to the temperature is inefficient for performance.
Quoted 4 lines
The use of hysteresis temperature could avoid this ping-pong of mitigation by relaxing the mitigation to happen only when the temperature goes below this lower hysteresis value.
the idea looks okay to me, just some minor comments.
Quoted 12 lines
Signed-off-by Ram Chandrasekar <[email protected]>Signed-off-by Lina Iyer <[email protected]>---
drivers/thermal/step_wise.c | 33 +++++++++++++++++++++++----------1 file changed, 23 insertions(+), 10 deletions(-)drivers/thermal/step_wise.c +0
-0
drivers/thermal/step_wise.c line 39
Quoted 5 lines
drivers/thermal/step_wise.c +1
-1@@ -36,7 +36,7 @@ * for this trip point * d. if the trend is THERMAL_TREND_DROP_FULL, use lower limit * for this trip point - * If the temperature is lower than a trip point, + * If the temperature is lower than a hysteresis temperature,
1. if you update this, you should update "if the temperature is higher than ..." as well.
2. the updated comment does not fully match the code change you made below.
Quoted 48 lines
drivers/thermal/step_wise.c +0
-0* a. if the trend is THERMAL_TREND_RAISING, do nothing * b. if the trend is THERMAL_TREND_DROPPING, use lower cooling * state for this trip point, if the cooling state already@@ -127,7 +127,7 @@ static void update_passive_instance(struct thermal_zone_device *tz,
static voidthermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
{drivers/thermal/step_wise.c +1
-1-int trip_temp; +int trip_temp, hyst_temp;enum
thermal_trip_typetrip_type;
enumthermal_trendtrend;
structthermal_instance*instance;drivers/thermal/step_wise.c +0
-0@@ -135,22 +135,23 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip) int old_target;
if (trip ==THERMAL_TRIPS_NONE) {drivers/thermal/step_wise.c +1
-1- trip_temp = tz->forced_passive; + hyst_temp = trip_temp = tz->forced_passive;
trip_type=THERMAL_TRIPS_NONE;
} else {
tz->ops->get_trip_temp(tz, trip, &trip_temp);drivers/thermal/step_wise.c +3
-0+ hyst_temp = trip_temp; +if (tz->ops->get_trip_hyst) { + tz->ops->get_trip_hyst(tz, trip,
&hyst_temp);drivers/thermal/step_wise.c +2
-0+ hyst_temp = trip_temp - hyst_temp; + }
tz->ops->get_trip_type(tz, trip, &trip_type); }
trend =get_tz_trend(tz, trip);drivers/thermal/step_wise.c +0
-6-if (tz->temperature >= trip_temp) { - throttle = true; -trace_thermal_zone_trip (tz, trip, trip_type); - } - - dev_dbg(&tz->device,"Trip%d[type=%d,temp=%d]:trend=%d,throttle=%d\n",
drivers/thermal/step_wise.c +0
-1- trip, trip_type, trip_temp, trend,throttle);
drivers/thermal/step_wise.c +2
-0+ dev_dbg(&tz->device, + "Trip%d[type=%d,temp=%d,hyst=%d]:trend=%d,throttle=%d\n",
drivers/thermal/step_wise.c +1
-0+ trip, trip_type, trip_temp, hyst_temp, trend,
Quoted 2 lines
throttle);
throttle is not set properly here, so this debug message does not make sense.
thanks,
rui
Quoted 27 lines
mutex_lock(&tz->lock);drivers/thermal/step_wise.c +0
-0@@ -159,6 +160,18 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip) continue;
old_target=instance->target;drivers/thermal/step_wise.c +10
-0+ throttle = false; +/* +* Lower the mitigation only if the temperature +* goes below the hysteresis temperature. +*/ +if (tz->temperature >= trip_temp || + (tz->temperature >= hyst_temp && + old_target != THERMAL_NO_TARGET)) { + throttle = true; + trace_thermal_zone_trip(tz, trip,
trip_type);drivers/thermal/step_wise.c +2
-0+ } +
instance->target=get_target_state(instance, trend, throttle);
dev_dbg(&instance->cdev->device, "old_target=%d, target=%d\n",
old_target, (int)instance-Quoted 1 line
target);