Re: [PATCH V6 2/3] cpuidle: play_idle: Specify play_idle with an idle state

[Daniel Lezcano]

On 08/11/2019 02:20, Rafael J. Wysocki wrote:
> On Wednesday, October 30, 2019 8:51:40 AM CET Daniel Lezcano wrote:
>> Currently, the play_idle function does not allow to tell which idle
>> state we want to go. Improve this by passing the idle state as
>> parameter to the function.
>>
>> Export cpuidle_find_deepest_state() symbol as it is used from the
>> intel_powerclamp driver as a module.

[ ... ]

>>  /**
>> diff --git a/drivers/thermal/intel/intel_powerclamp.c b/drivers/thermal/intel/intel_powerclamp.c
>> index 53216dcbe173..b55786c169ae 100644
>> --- a/drivers/thermal/intel/intel_powerclamp.c
>> +++ b/drivers/thermal/intel/intel_powerclamp.c
>> @@ -29,6 +29,7 @@
>>  #include <linux/delay.h>
>>  #include <linux/kthread.h>
>>  #include <linux/cpu.h>
>> +#include <linux/cpuidle.h>
>>  #include <linux/thermal.h>
>>  #include <linux/slab.h>
>>  #include <linux/tick.h>
>> @@ -430,7 +431,8 @@ static void clamp_idle_injection_func(struct kthread_work *work)
>>  	if (should_skip)
>>  		goto balance;
>>  
>> -	play_idle(jiffies_to_usecs(w_data->duration_jiffies));
>> +	play_idle(jiffies_to_usecs(w_data->duration_jiffies),
>> +		  cpuidle_find_deepest_state());
> 
> I don't see a reason for changing the code here like this.
> 
> What you really need is to have a way to set a limit on the idle
> state exit latency for idle injection on ARM.
> 
> For that you can pass the exit latency limit to play_idle(), but then
> you need to change powerclamp to pass UNIT_MAX or similar which is
> ugly, or you can redefine cpuidle_use_deepest_state() to take the
> exit latency limit as the arg (with 0 meaning use_deepest_state == false).

I'm confused with the "... ugly, *or* ...". In any case we have to
specify a latency constraint to play_idle(), no?

> In the latter case, it would be quite straightforward to add an
> exit_latency argument to cpuidle_find_deepest_state() and note that
> find_deepest_state() takes a max_latency arg already, so that would be
> a trivial change (hint!).




-- 
 <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