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

[Lukasz Luba <lukasz.luba@arm.com>]

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