Messages in this thread | | | From | Di Shen <> | Date | Wed, 21 Jun 2023 12:51:00 +0800 | Subject | Re: [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed |
| |
On Tue, Jun 20, 2023 at 6:39 PM Lukasz Luba <lukasz.luba@arm.com> 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@unisoc.com> > > > > --- > > 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@arm.com> > > 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
| |