| Message ID | 8e812065f4a76325097c5f9c17f3386736d8c1d4.1574315190.git.amit.kucheria@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | drivers: thermal: step_wise: add support for hysteresis | expand |
Hi Amit, On 11/21/2019 12:50 AM, Amit Kucheria wrote: > From: Ram Chandrasekar <rkumbako@codeaurora.org> > > Currently, step wise governor increases the mitigation when the > temperature goes above a threshold and decreases the mitigation when the > temperature goes below the threshold. If there is a case where the > temperature is wavering around the threshold, the mitigation will be > applied and removed every iteration, which is not very efficient. > > 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. > > Signed-off-by: Ram Chandrasekar <rkumbako@codeaurora.org> > Signed-off-by: Lina Iyer <ilina@codeaurora.org> > [Rebased patch from downstream] > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > --- > drivers/thermal/step_wise.c | 35 ++++++++++++++++++++++++----------- > 1 file changed, 24 insertions(+), 11 deletions(-) > > diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c > index 6e051cbd824ff..2c8a34a7cf959 100644 > --- a/drivers/thermal/step_wise.c > +++ b/drivers/thermal/step_wise.c > @@ -24,7 +24,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 > @@ -115,30 +115,31 @@ 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; > - bool throttle = false; > + bool throttle; There is no need to remove throttle = false here. You are setting it to false later down. > 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); throttle value is not final here. Why is the debug print and the setting of throttle reversed ? Idea is to print the final value of throttle. > > mutex_lock(&tz->lock); > > @@ -147,6 +148,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. > + */ I think this requires more comment here on why there is a check for old_target != THERMAL_NO_TARGET. Basically to ensure that the hysteresis is considered only when temperature is dropping. > + 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); > -- Warm Regards Thara
On Thu, Nov 21, 2019 at 11:21 AM Amit Kucheria <amit.kucheria@linaro.org> wrote: > > From: Ram Chandrasekar <rkumbako@codeaurora.org> > > Currently, step wise governor increases the mitigation when the > temperature goes above a threshold and decreases the mitigation when the > temperature goes below the threshold. If there is a case where the > temperature is wavering around the threshold, the mitigation will be > applied and removed every iteration, which is not very efficient. > > 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. > > Signed-off-by: Ram Chandrasekar <rkumbako@codeaurora.org> > Signed-off-by: Lina Iyer <ilina@codeaurora.org> > [Rebased patch from downstream] > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> Daniel, Rui: ping. Keerthy: This works for you, right? > --- > drivers/thermal/step_wise.c | 35 ++++++++++++++++++++++++----------- > 1 file changed, 24 insertions(+), 11 deletions(-) > > diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c > index 6e051cbd824ff..2c8a34a7cf959 100644 > --- a/drivers/thermal/step_wise.c > +++ b/drivers/thermal/step_wise.c > @@ -24,7 +24,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 > @@ -115,30 +115,31 @@ 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; > - bool throttle = false; > + bool throttle; > 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); > > @@ -147,6 +148,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); > -- > 2.17.1 >
On 21/11/2019 06:50, Amit Kucheria wrote: > From: Ram Chandrasekar <rkumbako@codeaurora.org> > > Currently, step wise governor increases the mitigation when the > temperature goes above a threshold and decreases the mitigation when the > temperature goes below the threshold. > > If there is a case where the > temperature is wavering around the threshold, the mitigation will be > applied and removed every iteration, which is not very efficient. > > 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. What I'm worried about is how the hysteresis is used in the current code, where the destination of this data is to set the value in the sensor hardware if it is supported. Using the hysteresis in the governor seems like abusing the initial purpose of this information. Moreover, the hysteresis creates a gray area where the above algorithm (DROPPING && !throttle) => state-- or (RAISING && throttle) => state++ may drop the performances because we will continue mitigating even below the threshold. As the governor is an open-loop controller, I'm not sure if we can do something except adding some kind of low pass filter to prevent mitigation bounces. > Signed-off-by: Ram Chandrasekar <rkumbako@codeaurora.org> > Signed-off-by: Lina Iyer <ilina@codeaurora.org> > [Rebased patch from downstream] > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > --- > drivers/thermal/step_wise.c | 35 ++++++++++++++++++++++++----------- > 1 file changed, 24 insertions(+), 11 deletions(-) > > diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c > index 6e051cbd824ff..2c8a34a7cf959 100644 > --- a/drivers/thermal/step_wise.c > +++ b/drivers/thermal/step_wise.c > @@ -24,7 +24,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 > @@ -115,30 +115,31 @@ 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; > - bool throttle = false; > + bool throttle; > 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); > > @@ -147,6 +148,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); > -- <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
diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c index 6e051cbd824ff..2c8a34a7cf959 100644 --- a/drivers/thermal/step_wise.c +++ b/drivers/thermal/step_wise.c @@ -24,7 +24,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 @@ -115,30 +115,31 @@ 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; - bool throttle = false; + bool throttle; 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); @@ -147,6 +148,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);