drivers: thermal: step_wise: add support for hysteresis

Message ID 8e812065f4a76325097c5f9c17f3386736d8c1d4.1574315190.git.amit.kucheria@linaro.org
State New
Headers show
Series
  • drivers: thermal: step_wise: add support for hysteresis
Related show

Commit Message

Amit Kucheria Nov. 21, 2019, 5:50 a.m.
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(-)

-- 
2.17.1

Comments

Thara Gopinath Nov. 21, 2019, 8:57 p.m. | #1
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
Amit Kucheria Dec. 10, 2019, 6:51 a.m. | #2
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

>
Daniel Lezcano Dec. 11, 2019, 1:35 p.m. | #3
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

Patch

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);