Re: [PATCH V5] thermal/core/power_allocator: reset thermal governor when trip point is changed

[Di Shen]

On Mon, Jul 10, 2023 at 11:38 AM Di Shen <di.shen@xxxxxxxxxx> 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);
> +}
> +
>  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);
> +       if (ret && t.temperature != trip->temperature && tz->governor && tz->governor->reset)
> +               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;
>  };
>
> --
> 2.17.1
>

Sorry, the change log was not successfully saved to the commit message.
Add it as following here:
---
V5:
- Simplify the reset ops, make it no return value and no specific
  trip ID as argument.
- Extend the commit message.

V4: [4]
- Compared to V3, handle it in thermal core instead of in governor.
- Add an ops to the governor structure, and call it when a trip
  point is changed
- Define reset ops for power allocator.

V3: [3]
- Add fix tag.

V2: [2]
- Compared to v1, do not revert.
- Add a variable(last_switch_on_temp) in power_allocator_params
  to record the last switch_on_temp value.
- Adds a function to renew the update flag and update the
  last_switch_on_temp when thermal trips are writable.

V1: [1]
- Revert commit 0952177f2a1f.

[1] https://lore.kernel.org/all/20230309135515.1232-1-di.shen@xxxxxxxxxx/
[2] https://lore.kernel.org/all/20230315093008.17489-1-di.shen@xxxxxxxxxx/
[3] https://lore.kernel.org/all/20230320095620.7480-1-di.shen@xxxxxxxxxx/
[4] https://lore.kernel.org/all/20230619063534.12831-1-di.shen@xxxxxxxxxx/
---