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

[Di Shen]

On Tue, Jun 20, 2023 at 6:39 PM Lukasz Luba <lukasz.luba@xxxxxxx> wrote:
>
> Hi Di,
>
> I have missed your v4 because it landed below your v3 thread.
>
> On 6/19/23 07:35, Di Shen wrote:
> > When the thermal trip point is changed, the governor should
> > be reset so that the policy algorithm be updated to adapt to the
> > new trip point.
> >
> > This patch adds an ops for thermal the governor structure to reset
>
> s/ops/callback
>
> > the governor. The ops is called when the trip point is changed.
> > For power allocator, the parameters of pid controller and the states
> > of power cooling devices can be reset when the passive trip point
> > is changed.
> >
> > Signed-off-by: Di Shen <di.shen@xxxxxxxxxx>
> >
> > ---
> > V4:
> > - 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:
> > - Add fix tag.
> >
> > V2:
> > - 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:
> > - Revert commit 0952177f2a1f.
> > ---
> > ---
> >   drivers/thermal/gov_power_allocator.c | 21 +++++++++++++++++++++
> >   drivers/thermal/thermal_trip.c        |  6 ++++++
> >   include/linux/thermal.h               |  1 +
> >   3 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> > index 8642f1096b91..41d155adc616 100644
> > --- a/drivers/thermal/gov_power_allocator.c
> > +++ b/drivers/thermal/gov_power_allocator.c
> > @@ -729,10 +729,31 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
> >       return allocate_power(tz, trip.temperature);
> >   }
> >
> > +static int power_allocator_reset(struct thermal_zone_device *tz, int trip_id)
> > +{
> > +     int ret = 0;
> > +     struct thermal_trip trip;
> > +     struct power_allocator_params *params = tz->governor_data;
> > +
> > +     ret = __thermal_zone_get_trip(tz, trip_id, &trip);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Only need reset for passive trips */
> > +     if (trip.type != THERMAL_TRIP_PASSIVE)
> > +             return -EINVAL;
> > +
> > +     reset_pid_controller(params);
> > +     allow_maximum_power(tz, true);
> > +
> > +     return ret;
> > +}
> > +
> >   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..52eb768fada8 100644
> > --- a/drivers/thermal/thermal_trip.c
> > +++ b/drivers/thermal/thermal_trip.c
> > @@ -173,6 +173,12 @@ 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;
> >
> > +     if (t.temperature != trip->temperature && tz->governor && tz->governor->reset) {
> > +             ret = tz->governor->reset(tz, trip_id);
> > +             if (ret)
> > +                     pr_warn_once("Failed to reset thermal governor\n");
> > +     }
>
> I agree with Rafael. Maybe change that to debug print, so that can be
> checked during the product testing. We cannot do much if that happens.
>
Right.

> > +
> >       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..155ce2291fa5 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -204,6 +204,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);
> > +     int (*reset)(struct thermal_zone_device *tz, int trip);
> >       struct list_head        governor_list;
> >   };
> >
>
> That thermal_governor::reset() callback is what I had im mind while
> giving you the feedback for the v1. Now it's much cleaner what is going
> on and why.
>

Yes, it is necessary to do something for the governor if the trip point
is changed, especially for the governors that their trips are strongly
related to
the policy.

> Apart from some small bits, LGTM. Please adjust the comment in the patch
> header and this debug print and you can add:
>
> Reviewed-by: Lukasz Luba <lukasz.luba@xxxxxxx>
>
> Please send the next version as separate new thread.
>
> Regards,
> Lukasz

Thank you Lukasz !
I couldn't agree with you more about your comments. What you have said is what
I want to express.

I'd love to send the next version. Thanks again.

Best regards,
Di