Re: [PATCH v5 1/5] PM / EM: add devices to Energy Model

From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Lukasz Luba <lukasz.luba@arm.com>,
	linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	dri-devel@lists.freedesktop.org, linux-omap@vger.kernel.org,
	linux-mediatek@lists.infradead.org,
	linux-arm-msm@vger.kernel.org, linux-imx@nxp.com
Cc: nm@ti.com, juri.lelli@redhat.com, peterz@infradead.org,
	viresh.kumar@linaro.org, liviu.dudau@arm.com,
	bjorn.andersson@linaro.org, bsegall@google.com,
	festevam@gmail.com, Morten.Rasmussen@arm.com, robh@kernel.org,
	amit.kucheria@verdurent.com, lorenzo.pieralisi@arm.com,
	vincent.guittot@linaro.org, khilman@kernel.org,
	steven.price@arm.com, cw00.choi@samsung.com, mingo@redhat.com,
	mgorman@suse.de, rui.zhang@intel.com,
	alyssa.rosenzweig@collabora.com, orjan.eide@arm.com,
	daniel@ffwll.ch, b.zolnierkie@samsung.com,
	s.hauer@pengutronix.de, rostedt@goodmis.org,
	matthias.bgg@gmail.com, Dietmar.Eggemann@arm.com,
	airlied@linux.ie, javi.merino@arm.com,
	tomeu.vizoso@collabora.com, qperret@google.com, sboyd@kernel.org,
	mka@chromium.org, rdunlap@infradead.org, rjw@rjwysocki.net,
	agross@kernel.org, kernel@pengutronix.de, sudeep.holla@arm.com,
	patrick.bellasi@matbug.net, shawnguo@kernel.org
Subject: Re: [PATCH v5 1/5] PM / EM: add devices to Energy Model
Date: Mon, 6 Apr 2020 16:58:52 +0200	[thread overview]
Message-ID: <6b980e2a-c15c-0718-14b8-e8aa7510c832@linaro.org> (raw)
In-Reply-To: <cb7f670a-a04f-ba6f-1486-0421f3cce2e9@arm.com>

Hi Lukasz,

On 06/04/2020 15:29, Lukasz Luba wrote:
> 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.

Yeah, I suspected you already think about that :)

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

What is the problem of registering at the end of the cpufreq_online ?

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

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek