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

[Lukasz Luba]

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