Re: [PATCH 3/3] thermal: cpu_cooling: Migrate to using the EM framework

[Daniel Lezcano]

On 29/03/2019 10:16, Quentin Perret wrote:
> Hi Daniel,
> 
> On Thursday 28 Mar 2019 at 21:23:35 (+0100), Daniel Lezcano wrote:
>>>  /**
>>>   * struct time_in_idle - Idle time stats
>>>   * @time: previous reading of the absolute time that this cpu was idle
>>> @@ -82,7 +70,7 @@ struct time_in_idle {
>>>   *	frequency.
>>>   * @max_level: maximum cooling level. One less than total number of valid
>>>   *	cpufreq frequencies.
>>> - * @freq_table: Freq table in descending order of frequencies
>>> + * @em: Reference on the Energy Model of the device
>>>   * @cdev: thermal_cooling_device pointer to keep track of the
>>>   *	registered cooling device.
>>>   * @policy: cpufreq policy.
>>> @@ -98,7 +86,7 @@ struct cpufreq_cooling_device {
>>>  	unsigned int cpufreq_state;
>>>  	unsigned int clipped_freq;
>>>  	unsigned int max_level;
>>> -	struct freq_table *freq_table;	/* In descending order */
>>> +	struct em_perf_domain *em;
>>
>> Why do you need to add this field? it will be accessible via policy->em, no?
> 
> You mean via the CPUFreq policy ? Then no, the EM isn't attached to the
> CPUFreq policy. And we can't attach it directly to the CPUFreq policy
> since in *theory* it is not required to map 1:1 to CPUFreq policies
> (even though that _is_ true for all existing platforms). That's one of
> the things this patch checks in that em_is_sane() function below.
> 
> FWIW, the idea of the design is, the EM framework is 'independent' and
> it's up to the client subsystems (scheduler, IPA) to check if it actually
> works for them. In the case of the scheduler, for example, we can't use
> an EM that's too complex because that would cause too much overhead, so
> we don't start EAS if that's not the case. See:
> 
>   https://elixir.bootlin.com/linux/latest/source/kernel/sched/topology.c#L367
> 
> In the case of IPA, we need to do something similar. We can't use an EM
> that doesn't map 1:1 to CPUFreq policies, so we bail out if that's not
> true, etc, ... This isn't supposed to trigger any time soon, but it's
> good to have a check just to be on the safe side I think.

Ok, makes sense. Thanks for the clarification.



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