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

[Di Shen]

Hi Daniel,
Thank you for your reply.
On Mon, Jul 10, 2023 at 4:59 PM Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx> wrote:
>
> On 10/07/2023 05:32, Di Shen 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);
>
> Do you really want to allow the maximum power? What about if the trip
> temperature is decreased ?
>
If the trip temperature is decreased, allow_maximum_power will only
be executed once, and then the ipa governor will adapt to the lower trip
temperature and calculate the allocated power for cooling actors again.
Right?

But if the trip temperature is increased, it must allow the maximum power,
otherwise, the Line 723 update flag might be false(see the false example
in patch-v3), the power of cooling devices would be not allowed maximum
for a while unless the current temperature is higher than the new
switch_on_temp. In this situation, for cpufreq cooling devices, it means the
cpu max_freq is limited, which is bad for device performance. The same for
other cooling devices, if not reset the cooling state, it may introduce
unexpected issues.

705 static int power_allocator_throttle(struct thermal_zone_device
*tz, int trip_id)
706 {
707         struct power_allocator_params *params = tz->governor_data;
708         struct thermal_trip trip;
709         int ret;
710         bool update;
711
712        //......
721         ret = __thermal_zone_get_trip(tz, params->trip_switch_on,
&trip);
722         if (!ret && (tz->temperature < trip.temperature)) {
723                 update = (tz->last_temperature >=
trip.temperature);
724                 tz->passive = 0;
725                 reset_pid_controller(params);
726                 allow_maximum_power(tz, update);
727                 return 0;
728         }
729
730         tz->passive = 1;
731          //.......
740 }

> You want maximum power only if the mitigation ends.
>
Yes, you're right, I agree. It's better to do like that, actually,
patch-v3 is in line
with this point I think, but it is not in the thermal core.

Oh, I see!
How about modified like that:
static void power_allocator_reset(struct thermal_zone_device *tz)
{
     struct power_allocator_params *params = tz->governor_data;
+   struct thermal_trip trip;
+   int ret;

     reset_pid_controller(params);
+   ret = __thermal_zone_get_trip(tz, params->trip_switch_on, &trip);
+   if (!ret && (tz->temperature < trip.temperature))
             allow_maximum_power(tz, true);
}
It seems clearer.

> > +}
> > +
> >   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);
>
> Actually we have the trip points:
>
> ACTIVE, PASSIVE, HOT and CRITICAL
>
Correct.

> The last two ones should not be writable.
>
> Instead of this test, it would be cleaner to only make the ACTIVE and
> PASSIVE trip point writable when the CONFIG_THERMAL_WRITABLE_TRIPS
> option is set. Consequently, other trip points won't be writable and
> this test can go away as set_trip will be protected by a RO sysfs file
> property.
>
> > +     if (ret && t.temperature != trip->temperature && tz->governor && tz->governor->reset)
>
> The temperature test is duplicated because it is already done in the
> block before.
>
So here you mean two points:
1. Make the HOT and CRITICAL point RO so that it can no trip type test
when gov->reset
2. not do the temperature test twice
Right?

For the first point, it's OK for me. But what if someone wants to
power off the device earlier
or later? I think it should have the access to update the critical
temperature. I'm not really
know about the hot trip temperature. So I'm not sure the real reason
to make them RO
when the trip points are writable. I can't make it RO just because of
the code simplification.

For the second point, that's OK. I will simplify in the next patch
version. Thank you, Daniel.

> > +             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;
> >   };
> >
>
> --
> <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
>

Best regards,
Di