lkml.org 
[lkml]   [2020]   [Apr]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v5 1/5] PM / EM: add devices to Energy Model
From
Date
Hi Daniel,

Thank you for the review.

On 4/3/20 5:05 PM, Daniel Lezcano wrote:
>
> Hi Lukasz,
>
>
> On 18/03/2020 12:45, Lukasz Luba wrote:
>> Add support of other devices into the Energy Model framework not only the
>> CPUs. Change the interface to be more unified which can handle other
>> devices as well.
>
> thanks for taking care of that. Overall I like the changes in this patch
> but it hard to review in details because the patch is too big :/
>
> Could you split this patch into smaller ones?
>
> eg. (at your convenience)
>
> - One patch renaming s/cap/perf/
>
> - One patch adding a new function:
>
> em_dev_register_perf_domain(struct device *dev,
> unsigned int nr_states,
> struct em_data_callback *cb);
>
> (+ EXPORT_SYMBOL_GPL)
>
> And em_register_perf_domain() using it.
>
> - One converting the em_register_perf_domain() user to
> em_dev_register_perf_domain
>
> - One adding the different new 'em' functions
>
> - And finally one removing em_register_perf_domain().

I agree and will do the split. I could also break the dependencies
for future easier merge.

>
>
>> Acked-by: Quentin Perret <qperret@google.com>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>
> [ ... ]
>
>> 2. Core APIs
>> @@ -70,14 +72,16 @@ CONFIG_ENERGY_MODEL must be enabled to use the EM framework.
>> Drivers are expected to register performance domains into the EM framework by
>> calling the following API::
>>
>> - int em_register_perf_domain(cpumask_t *span, unsigned int nr_states,
>> - struct em_data_callback *cb);
>> + int em_register_perf_domain(struct device *dev, unsigned int nr_states,
>> + struct em_data_callback *cb, cpumask_t *cpus);
>
> Isn't possible to get rid of this cpumask by using
> cpufreq_cpu_get() which returns the cpufreq's policy and from their get
> the related cpus ?

We had similar thoughts with Quentin and I've checked this.
Unfortunately, if the policy is a 'new policy' [1] it gets
allocated and passed into cpufreq driver ->init(policy) [2].
Then that policy is set into per_cpu pointer for each related_cpu [3]:

for_each_cpu(j, policy->related_cpus)
per_cpu(cpufreq_cpu_data, j) = policy;


Thus, any calls of functions (i.e. cpufreq_cpu_get()) which try to
take this ptr before [3] won't work.

We are trying to register EM from cpufreq_driver->init(policy) and the
per_cpu policy is likely to be not populated at that phase.

Regards,
Lukasz

[1]
https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1328
[2]
https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1350
[3]
https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1374


\
 
 \ /
  Last update: 2020-04-06 15:30    [W:0.330 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site