lkml.org 
[lkml]   [2019]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] drivers: thermal: step_wise: add support for hysteresis
    From
    Date
    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

    \
     
     \ /
      Last update: 2019-11-21 21:58    [W:2.834 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site