| Message ID | 20230710033234.28641-1-di.shen@unisoc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V5] thermal/core/power_allocator: reset thermal governor when trip point is changed | expand |
Hi Daniel, On 7/11/23 09:23, Daniel Lezcano wrote: > > Hi Di, > > On 11/07/2023 05:40, Di Shen wrote: > > [ ... ] > >>>>>> +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? >>> >>> Sorry for jumping in this fifth version but I'm not sure about resetting >>> the change is the right way (and probably, changing a trip point with >>> the power allocator is not a good idea) >>> >>> The platforms where the IPA is planned to be used are creating a dummy >>> trip point where the IPA begins the acquisition without cooling devices >>> in order to have the math building the PID schema (eg. hi3660.dtsi). >>> >>> What about the sustainable power vs the trip point temperature? I mean >>> we can change the trip temperature but not the sustainable power which >>> is directly related to the target temperature. So the resulting power >>> computation will be wrong. >>> >> I totally agree, thanks for reminding me. Sustainable power is the >> maximum >> power available at the target temperature, so it must be updated when >> the trip >> point is changed. Sorry for missing this point. How about calling >> get_sustainable_power() to update the sustainable_power? Furthermore, >> when >> the sustainble_power() is changed, the pid constants tzp->k_* must be >> estimated >> again. In get_sustainble_power, it checks that the sustainable_power >> is updated, >> it will call the estimate_pid_constants() to renew the tzp->k_*. > > Yes and the sustainable power can be set from userspace too. > > So here we have to distinguish what is related to the thermal setup and > the thermal usage. > > Actually the thermal framework should protect the information from the > firmware. It is not acceptable to have an user being able to change the > trip points provided by the firmware. > > The writable trip point should allow only temperature changes below the > ones given in the firmware. > >>> The more I think about that, the more I do believe writable trip point >>> and IPA are incompatible. >>> >>> What about forbid that? >>> >>> For instance, add a set_trip callback instead of resetting in the >>> governor and return -EPERM from the IPA? >>> >> I've seen that you have sent a patch recently which adds the callback >> thermal_zone_trips_update(), is that what you said set_trip callback? > > Not exactly. > > Instead of adding a 'reset' callback, add a 'trip_update' (or whatever > the name) callback. > > Then pass the trip point to the callback along with the thermal zone. > > int ipa_trip_update(struct thermal_zone_device *tz, > struct thermal_trip *trip) > { > // Do more IPA crazy stuff or return -EPERM > } > > >>> Lukasz ? > > Lukasz? what do you think? > > My apologies for delay, I was on 2-weeks vacation. I'll catch up and respond to those questions. Regards, Lukasz
Hi Lukasz, On 17/07/2023 12:07, Lukasz Luba wrote: [ ... ] >> int ipa_trip_update(struct thermal_zone_device *tz, >> struct thermal_trip *trip) >> { >> // Do more IPA crazy stuff or return -EPERM >> } >> >> >>>> Lukasz ? >> >> Lukasz? what do you think? >> >> > > My apologies for delay, I was on 2-weeks vacation. I'll catch up and > respond to those questions. No worries, I hope you enjoyed your vacations The main thing is to know you got the info ;) Thanks
Hi Lukasz, On Mon, Jul 17, 2023 at 6:06 PM Lukasz Luba <lukasz.luba@arm.com> wrote: > > Hi Daniel, > > > On 7/11/23 09:23, Daniel Lezcano wrote: > > > > Hi Di, > > > > On 11/07/2023 05:40, Di Shen wrote: > > > > [ ... ] > > > >>>>>> +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? > >>> > >>> Sorry for jumping in this fifth version but I'm not sure about resetting > >>> the change is the right way (and probably, changing a trip point with > >>> the power allocator is not a good idea) > >>> > >>> The platforms where the IPA is planned to be used are creating a dummy > >>> trip point where the IPA begins the acquisition without cooling devices > >>> in order to have the math building the PID schema (eg. hi3660.dtsi). > >>> > >>> What about the sustainable power vs the trip point temperature? I mean > >>> we can change the trip temperature but not the sustainable power which > >>> is directly related to the target temperature. So the resulting power > >>> computation will be wrong. > >>> > >> I totally agree, thanks for reminding me. Sustainable power is the > >> maximum > >> power available at the target temperature, so it must be updated when > >> the trip > >> point is changed. Sorry for missing this point. How about calling > >> get_sustainable_power() to update the sustainable_power? Furthermore, > >> when > >> the sustainble_power() is changed, the pid constants tzp->k_* must be > >> estimated > >> again. In get_sustainble_power, it checks that the sustainable_power > >> is updated, > >> it will call the estimate_pid_constants() to renew the tzp->k_*. > > > > Yes and the sustainable power can be set from userspace too. > > > > So here we have to distinguish what is related to the thermal setup and > > the thermal usage. > > > > Actually the thermal framework should protect the information from the > > firmware. It is not acceptable to have an user being able to change the > > trip points provided by the firmware. > > > > The writable trip point should allow only temperature changes below the > > ones given in the firmware. > > > >>> The more I think about that, the more I do believe writable trip point > >>> and IPA are incompatible. > >>> > >>> What about forbid that? > >>> > >>> For instance, add a set_trip callback instead of resetting in the > >>> governor and return -EPERM from the IPA? > >>> > >> I've seen that you have sent a patch recently which adds the callback > >> thermal_zone_trips_update(), is that what you said set_trip callback? > > > > Not exactly. > > > > Instead of adding a 'reset' callback, add a 'trip_update' (or whatever > > the name) callback. > > > > Then pass the trip point to the callback along with the thermal zone. > > > > int ipa_trip_update(struct thermal_zone_device *tz, > > struct thermal_trip *trip) > > { > > // Do more IPA crazy stuff or return -EPERM > > } > > > > > >>> Lukasz ? > > > > Lukasz? what do you think? > > > > > > My apologies for delay, I was on 2-weeks vacation. I'll catch up and > respond to those questions. > > Regards, > Lukasz What do you think about the points that Daniel has mentioned? Any comments would be very appreciated, hope to hear from you, thank you. Best regards, Di
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; };
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@unisoc.com> --- drivers/thermal/gov_power_allocator.c | 9 +++++++++ drivers/thermal/thermal_trip.c | 5 +++++ include/linux/thermal.h | 3 +++ 3 files changed, 17 insertions(+)