[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0844022b-cf2a-dadb-9340-9107cd40169b@arm.com>
Date: Mon, 17 Jul 2023 11:07:05 +0100
From: Lukasz Luba <lukasz.luba@....com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: Di Shen <di.shen@...soc.com>, rafael@...nel.org, amitk@...nel.org,
rui.zhang@...el.com, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org, xuewen.yan@...soc.com,
jeson.gao@...soc.com, orsonzhai@...il.com, zhanglyra@...il.com,
Di Shen <cindygm567@...il.com>
Subject: Re: [PATCH V5] thermal/core/power_allocator: reset thermal governor
when trip point is changed
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
Powered by blists - more mailing lists