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

From: Daniel Lezcano
Date: Tue Jul 11 2023 - 04:23:42 EST



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?


--
<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