From: Lukasz Luba <lukasz.luba@arm.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>, Di Shen <di.shen@unisoc.com>
Cc: daniel.lezcano@linaro.org, rui.zhang@intel.com, amitk@kernel.org,
linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
xuewen.yan@unisoc.com, jeson.gao@unisoc.com, zhanglyra@gmail.com,
orsonzhai@gmail.com
Subject: Re: [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed
Date: Fri, 23 Jun 2023 18:34:31 +0100 [thread overview]
Message-ID: <86da9945-04d5-047a-cb2d-5fb63737839f@arm.com> (raw)
In-Reply-To: <CAJZ5v0i5V8kpaaCsH4wuU83=zXpdJgR2CdCX-Wj=PmJx3OJ2Lg@mail.gmail.com>
On 6/23/23 17:55, Rafael J. Wysocki wrote:
> On Fri, Jun 23, 2023 at 9:43 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>>
>>
[snip]
>>
>> I agree, the patch header doesn't explain that properly. Here is the
>> explanation for this Intelligent Power Allocator (IPA):
>>
>> The IPA controls temperature using PID mechanism. It's a closed
>> feedback loop. That algorithm can 'learn' from the 'observed'
>> in the past reaction for it's control decisions and accumulates that
>> information in the part called 'error integral'. Those accumulated
>> 'error' gaps are the differences between the set target value and the
>> actually achieved value. In our case the target value is the target
>> temperature which is coming from the trip point. That part is then used
>> with the 'I' (of PID) component, so we can compensate for those
>> 'learned' mistakes.
>> Now, when you change the target temperature value - all your previous
>> learned errors won't help you. That's why Intelligent Power Allocator
>> should reset previously accumulated history.
>
> Right.
>
> And every other governor using information from the past for control
> will have an analogous problem, won't it?
Not necessarily, but to play safe I would go case-by-case and make
sure other governors are aligned to this new feature.
E.g. the bang-bang governor operates only on current temperature and
current trip value + trip hysteresis. The flow graph describes it [1].
The control (state of the fan: ON or OFF) of that governor could be
simply adjusted to the new reality -> new trip point temp. That would
just mean 'toggling' the fan if needed. There are only 2 'target'
states: 0 or 1 for the fan. You can images a situation when the
temperature doesn't change, but we manipulate the trip value for that
governor. The governor would react correctly always in such situation
w/o a need of a reset IMO.
>
>>>
>>>>>
>>>>>> For the 2nd case IIUC the code, we pass the 'trip.temperature'
>>>>>> and should be ready for what you said (modification of that value).
>>>>>
>>>>> Generally speaking, it needs to be prepared for a simultaneous change
>>>>> of multiple trip points (including active), in which case it may not
>>>>> be useful to invoke the ->reset() callback for each of them
>>>>> individually.
>>>>
>>>> Although, that looks more cleaner IMO. Resetting one by one in
>>>> a temperature order would be easily maintainable, won't be?
>>>
>>> I wouldn't call it maintainable really.
>>>
>>> First of all, the trips may not be ordered. There are no guarantees
>>> whatsoever that they will be ordered, so the caller may have to
>>> determine the temperature order in the first place. This would be an
>>> extra requirement that currently is not there.
>>>
>>> Apart from this, I don't see any fundamental difference between the
>>> case when trip points are updated via sysfs and when they are updated
>>> by the driver. The governor should reset itself in any of those cases
>>> and even if one trip point changes, the temperature order of all of
>>> them may change, so the governor reset mechanism should be able to
>>> handle the case when multiple trip points are updated at the same
>>> time. While you may argue that this is theoretical, the ACPI spec
>>> clearly states that this is allowed to happen, for example.
>>>
>>> If you want a generic reset callback for governors, that's fine, but
>>> make it generic and not specific to a particular use case.
>>
>> I think we agree here, but probably having slightly different
>> implementation in mind. Based on you explanation I think you
>> want simply this API:
>> void (*reset)(struct thermal_zone_device *tz);
>>
>> 1. no return value
>> 2. no specific trip ID as argument
>>
>> Do you agree?
>
> Yes, I do.
OK, thanks.
Di could you implement that 'reset()' API according to this description,
please?
>
>> IMO such implementation and API would also work for this IPA
>> purpose. Would that work for the ACPI use case as well?
>
> It would AFAICS.
Thanks Rafael for the comments and the progress that we made :)
Regards,
Lukasz
[1]
https://elixir.bootlin.com/linux/v6.3/source/drivers/thermal/gov_bang_bang.c#L80
next prev parent reply other threads:[~2023-06-23 17:34 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-20 9:56 [PATCH V3] thermal/core/power_allocator: avoid thermal cdev can not be reset Di Shen
2023-03-20 12:24 ` Lukasz Luba
2023-04-10 2:09 ` Di Shen
2023-04-10 19:51 ` Daniel Lezcano
2023-04-11 7:17 ` Lukasz Luba
2023-04-13 4:51 ` Di Shen
2023-04-13 7:37 ` Daniel Lezcano
2023-04-13 8:40 ` Di Shen
2023-04-14 11:12 ` Daniel Lezcano
2023-04-14 14:18 ` Lukasz Luba
2023-04-14 15:06 ` Daniel Lezcano
2023-04-14 15:21 ` Lukasz Luba
2023-05-25 6:39 ` Di Shen
2023-06-19 6:35 ` [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed Di Shen
2023-06-20 9:46 ` Rafael J. Wysocki
2023-06-20 10:07 ` Rafael J. Wysocki
2023-06-20 10:20 ` Lukasz Luba
2023-06-20 10:39 ` Rafael J. Wysocki
2023-06-20 11:56 ` Lukasz Luba
2023-06-22 18:27 ` Rafael J. Wysocki
2023-06-23 7:43 ` Lukasz Luba
2023-06-23 16:55 ` Rafael J. Wysocki
2023-06-23 17:34 ` Lukasz Luba [this message]
2023-06-25 8:40 ` Di Shen
2023-06-25 8:39 ` Di Shen
2023-06-26 7:45 ` Lukasz Luba
2023-06-20 10:38 ` Lukasz Luba
2023-06-21 4:51 ` Di Shen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=86da9945-04d5-047a-cb2d-5fb63737839f@arm.com \
--to=lukasz.luba@arm.com \
--cc=amitk@kernel.org \
--cc=daniel.lezcano@linaro.org \
--cc=di.shen@unisoc.com \
--cc=jeson.gao@unisoc.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=orsonzhai@gmail.com \
--cc=rafael@kernel.org \
--cc=rui.zhang@intel.com \
--cc=xuewen.yan@unisoc.com \
--cc=zhanglyra@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).