Message-ID: <40320c0d-1bea-568e-f668-119e1c7df204@linaro.org>
Date:   Wed, 16 Feb 2022 19:51:25 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Lukasz Luba <lukasz.luba@....com>,
        Manaf Meethalavalappu Pallikunhi <quic_manafm@...cinc.com>
Cc:     linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Zhang Rui <rui.zhang@...el.com>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Amit Kucheria <amitk@...nel.org>
Subject: Re: [PATCH v5] drivers: thermal: clear all mitigation when thermal
 zone is disabled

On 15/02/2022 10:57, Lukasz Luba wrote:
> 
> 
> On 2/14/22 8:00 PM, Manaf Meethalavalappu Pallikunhi wrote:
>>
>> On 1/31/2022 12:55 PM, Lukasz Luba wrote:
>>> Hi Manaf,
>>>
>>> On 1/27/22 6:11 PM, Manaf Meethalavalappu Pallikunhi wrote:
>>>> Whenever a thermal zone is in trip violated state, there is a chance
>>>> that the same thermal zone mode can be disabled either via
>>>> thermal core API or via thermal zone sysfs. Once it is disabled,
>>>> the framework bails out any re-evaluation of thermal zone. It leads
>>>> to a case where if it is already in mitigation state, it will stay
>>>> the same state forever.
>>>>
>>>> To avoid above mentioned issue, add support to bind/unbind
>>>> governor from thermal zone during thermal zone mode change request
>>>> and clear all existing throttling in governor unbind_from_tz()
>>>> callback.
>>>
>>> I have one use case:
>>> This would be a bit dangerous, e.g. to switch governors while there is a
>>> high temperature. Although, sounds reasonable to left a 'default' state
>>> for a next governor.
>>>
>> I believe only way to change the governror via userspace at runtime.
>>
>> Just re-evaluate thermal zone  (thermal_zone_device_update) 
>> immediately after
>>
>> thermal_zone_device_set_policy()  in same policy_store() context, 
>> isn't it good enough ?
> 
> It depends. The code would switch the governors very fast, in the
> meantime notifying about possible full speed of CPU (cooling state = 0).
> If the task scheduler goes via schedutil (cpufreq governor) at that
> moment and decides to set this max frequency, it will be set.
> This is situation with your patch, since you added in IPA unbind
> 'allow_maximum_power()'.
> Then the new governor is bind, evaluates the max cooling state, the
> notification about reduced max freq is sent to schedutil (a workqueue
> will call .sugov_limits() callback) and lower freq would be set.
> 
> Now there are things which are not greatly covered by these 4
> involved sub-systems (thermal fwk, schedutil, scheduler, HW).
> It takes time. It also depends when the actual HW freq is possible to be
> set. It might take a few milli-seconds or even a dozes of milli-seconds
> (depends on HW).
> 
> Without your change, we avoid such situation while switching the
> thermal governors.
> 
> For your requirement, which is 'mode' enable/disable it OK to
> un-throttle.
> 
> It's probably something to Rafael and Daniel to judge if we want to
> pay that cost and introduce this racy time slot.
> 
> Maybe there is a way to implement your needed feature differently.
> Unfortunately, I'm super busy with other stuff this month so I cannot
> spent much time investigating this.

IMO, we should be able to disable a thermal zone, no mitigation will 
happen but somehow we can keep the hot / critical trip points enabled, 
so if the temperature crosses these that would trigger an action for safety.

However, for me it is still unclear what means "disabling a thermal 
zone" exactly ?

Maybe we should clarify that before going further


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Powered by blists - more mailing lists