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