[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fdd82ddb-82bc-4c8c-86ef-c80505881013@arm.com>
Date: Fri, 23 Feb 2024 10:48:48 +0100
From: Pierre Gondois <pierre.gondois@....com>
To: Qais Yousef <qyousef@...alina.io>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
Viresh Kumar <viresh.kumar@...aro.org>, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Vincent Guittot <vincent.guittot@...aro.org>,
Dietmar Eggemann <dietmar.eggemann@....com>, Christian.Loehle@....com
Subject: Re: [PATCH] cpufreq: Change default transition delay to 2ms
On 2/23/24 00:39, Qais Yousef wrote:
> On 02/22/24 16:15, Pierre Gondois wrote:
>
>>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>>> index 66cef33c4ec7..68a5ba24a5e0 100644
>>> --- a/drivers/cpufreq/cpufreq.c
>>> +++ b/drivers/cpufreq/cpufreq.c
>>> @@ -576,6 +576,15 @@ unsigned int cpufreq_policy_transition_delay_us(struct cpufreq_policy *policy)
>>>
>>> latency = policy->cpuinfo.transition_latency / NSEC_PER_USEC;
>>> if (latency) {
>>> + unsigned int max_delay_us = 2 * MSEC_PER_SEC;;
>>> +
>>> + /*
>>> + * If the platform already has high transition_latency, use it
>>> + * as-is.
>>> + */
>>> + if (latency > max_delay_us)
>> [1] return min(latency, 10ms);
>>> + return latency;
>>> +
>>> /*
>>> * For platforms that can change the frequency very fast (< 10
>>> * us), the above formula gives a decent transition delay. But
>>> @@ -586,7 +595,7 @@ unsigned int cpufreq_policy_transition_delay_us(struct cpufreq_policy *policy)
>>> * a reasonable amount of time after which we should reevaluate
>>> * the frequency.
>>> */
>>> - return min(latency * LATENCY_MULTIPLIER, (unsigned int)(2 * MSEC_PER_SEC));
>>> + return min(latency * LATENCY_MULTIPLIER, max_delay_us);
>>> }
>>>
>>> return LATENCY_MULTIPLIER;
>>>
>>
>> A policy with these values:
>> - transition_delay_us = 0
>> - transition_latency = 30ms
>> would get a transition_delay of 30ms I think.
>>
>> Maybe it would be better to default to the old value in this case [1].
>
> Hmm. I think it wouldn't make sense to have 2 levels of capping. It's either we
> cap to 2ms, or honour the transition latency from the driver if it is higher
> and let it lower it if it can truly handle smaller values?
>
> Rafael, should I send a new version of the patch, a new patch on top or would
> you like to take a fixup if you can amend the commit? If you and Viresh think
> the two levels of capping make sense as suggested above let me know. I think
> better to delegate to the drivers if they report transition_latency higher than
> 2ms.
The latency can be computed by dev_pm_opp_get_max_transition_latency() and
one of its component comes from `clock-latency-ns` DT binding. The maximum value
I saw is 10ms, so it seems it should be ok not to add: `min(latency, 10ms)`
>
> -->8--
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 66cef33c4ec7..668263c53810 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -576,8 +576,17 @@ unsigned int cpufreq_policy_transition_delay_us(struct cpufreq_policy *policy)
>
> latency = policy->cpuinfo.transition_latency / NSEC_PER_USEC;
> if (latency) {
> + unsigned int max_delay_us = 2 * MSEC_PER_SEC;;
> +
> + /*
> + * If the platform already has high transition_latency, use it
> + * as-is.
> + */
> + if (latency > max_delay_us)
> + return latency;
> +
> /*
> - * For platforms that can change the frequency very fast (< 10
> + * For platforms that can change the frequency very fast (< 20
I think it should be 10->2us as we do:
min(latency * 1000, 2ms)
> * us), the above formula gives a decent transition delay. But
> * for platforms where transition_latency is in milliseconds, it
> * ends up giving unrealistic values.
> @@ -586,7 +595,7 @@ unsigned int cpufreq_policy_transition_delay_us(struct cpufreq_policy *policy)
> * a reasonable amount of time after which we should reevaluate
> * the frequency.
> */
> - return min(latency * LATENCY_MULTIPLIER, (unsigned int)(2 * MSEC_PER_SEC));
> + return min(latency * LATENCY_MULTIPLIER, max_delay_us);
> }
>
> return LATENCY_MULTIPLIER;
>
> -->8--
>
>>
>> ---
>>
>> Also a note that on the Pixel6 I have, transition_latency=5ms,
>> so the platform's policies would end up with transition_delay=5ms
>
> Yes I know. But at this stage it's a driver issue. I think this value is not
> correct and there's a typo.
>
>>
>>
>>>>
>>>>
>>>> 2.
>>>> There are references to the 10ms values at other places in the code:
>>>>
>>>> include/linux/cpufreq.h
>>>> * ondemand governor will work on any processor with transition latency <= 10ms,
>>>
>>> Not sure this one needs updating. Especially with the change above which means
>>> 10ms could theoretically happen. But if there are suggestions happy to take
>>> them.
>>
>> a.
>> LATENCY_MULTIPLIER introduction:
>> 112124ab0a9f ("[CPUFREQ] ondemand/conservative: sanitize sampling_rate restrictions")
>>
>> b.
>> max_transition_latency removal:
>> ed4676e25463 ("cpufreq: Replace "max_transition_latency" with "dynamic_switching"")
>>
>> c.
>> dynamic_switching removal:
>> 9a2a9ebc0a75 ("cpufreq: Introduce governor flags")
>>
>> The value could be removed independently from this patch indeed, as this is not
>> related to cpufreq_policy_transition_delay_us() since b.
>
> Thanks for sending the patch.
>
>>
>>
>>>
>>>>
>>>> drivers/cpufreq/cpufreq.c
>>>> * For platforms that can change the frequency very fast (< 10
>>>> * us), the above formula gives a decent transition delay. But
>>>> -> the 10us value matches 10ms = 10us * LATENCY_MULTIPLIER
>>>
>>> I can't find this one.
>>
>> It's in cpufreq_policy_transition_delay_us(),
>> "the 10us value matches 10ms = 10us * LATENCY_MULTIPLIER"
>> is a sentence I wrote, the comment to modify would be:
>> """
>> * For platforms that can change the frequency very fast (< 10
>> * us), the above formula gives a decent transition delay. But
>> """
>
> Ah you were referring to s/10/20/. Done.
>
>>
>>>
>>>>
>>>> Documentation/admin-guide/pm/cpufreq.rst
>>>> Typically, it is set to values of the order of 10000 (10 ms). Its
>>>> default value is equal to the value of ``cpuinfo_transition_latency``
>>>
>>> I am not sure about this one. It refers to cpuinfo_transition_latency not the
>>> delay and uses a formula to calculate it based on that.
>>>
>>> Seems the paragraph needs updating in general to reflect other changes?
>>
>> aa7519af450d ("cpufreq: Use transition_delay_us for legacy governors as well")
>>
>> The cpufreq_policy_transition_delay_us() was introduced there and integrates the
>> 10ms value related to this paragraph.
>>
>> ---
>>
>> I assume that if we keep the 10ms value in the code, it should be ok to let
>> the comment as is. I'll send a patch to remove the first one as it can be
>> done independently.
>
> Thanks!
>
> --
> Qais Yousef
Powered by blists - more mailing lists