Re: [PATCH V3 resend] cpufreq: Cap the default transition delay value to 10 ms

[Rafael J. Wysocki]

On Thursday, August 17, 2017 5:42:27 AM CEST Viresh Kumar wrote:
> If transition_delay_us isn't defined by the cpufreq driver, the default
> value of transition delay (time after which the cpufreq governor will
> try updating the frequency again) is currently calculated by multiplying
> transition_latency (nsec) with LATENCY_MULTIPLIER (1000) and then
> converting this time to usec. That gives the exact same value as
> transition_latency, just that the time unit is usec instead of nsec.
> 
> With acpi-cpufreq for example, transition_latency is set to around 10
> usec and we get transition delay as 10 ms. Which seems to be a
> reasonable amount of time to reevaluate the frequency again.
> 
> But for platforms where frequency switching isn't that fast (like ARM),
> the transition_latency varies from 500 usec to 3 ms, and the transition
> delay becomes 500 ms to 3 seconds. Of course, that is a pretty bad
> default value to start with.
> 
> We can try to come across a better formula (instead of multiplying with
> LATENCY_MULTIPLIER) to solve this problem, but will that be worth it ?
> 
> This patch tries a simple approach and caps the maximum value of default
> transition delay to 10 ms. Of course, userspace can still come in and
> change this value anytime or individual drivers can rather provide
> transition_delay_us instead.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
> One of the IMX shows odd behavior after this patch is applied and stays
> at max freq for little longer. More detailed debugging is required to be
> done by some IMX guys (who haven't replied for the last 3 weeks on that
> email thread, apart from Leonard).
> 
> As per Leonard as well, we shouldn't stop this patch from getting merged
> and so sending it again.
> 
>  drivers/cpufreq/cpufreq.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index b2cc98551fc3..c7ae67d6886d 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -532,8 +532,19 @@ unsigned int cpufreq_policy_transition_delay_us(struct cpufreq_policy *policy)
>  		return policy->transition_delay_us;
>  
>  	latency = policy->cpuinfo.transition_latency / NSEC_PER_USEC;
> -	if (latency)
> -		return latency * LATENCY_MULTIPLIER;
> +	if (latency) {
> +		/*
> +		 * For platforms that can change the frequency very fast (< 10
> +		 * us), the above formula gives a decent transition delay. But
> +		 * for platforms where transition_latency is in milliseconds, it
> +		 * ends up giving unrealistic values.
> +		 *
> +		 * Cap the default transition delay to 10 ms, which seems to be
> +		 * a reasonable amount of time after which we should reevaluate
> +		 * the frequency.
> +		 */
> +		return min(latency * LATENCY_MULTIPLIER, (unsigned int)10000);
> +	}
>  
>  	return LATENCY_MULTIPLIER;
>  }
> 

Applied, thanks!