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

[Di Shen]

Hi Lukasz,

On Mon, Jul 17, 2023 at 6:06 PM Lukasz Luba <lukasz.luba@xxxxxxx> 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