Messages in this thread | | | Subject | Re: [PATCH v5 1/5] PM / EM: add devices to Energy Model | From | Lukasz Luba <> | Date | Tue, 7 Apr 2020 10:32:55 +0100 |
| |
On 4/6/20 10:17 PM, Daniel Lezcano wrote: > On 06/04/2020 18:07, Lukasz Luba wrote: >> >> >> On 4/6/20 3:58 PM, Daniel Lezcano wrote: >>> >>> 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 ? >> >> We want to enable driver developers to choose one of two options for the >> registration of Energy Model: >> 1. a simple one via dev_pm_opp_of_register_em(), which uses default >> callback function calculating power based on: voltage, freq >> and DT entry 'dynamic-power-coefficient' for each OPP >> 2. a more sophisticated, when driver provides callback function, which >> will be called from EM for each OPP to ask for related power; >> This interface could also be used by devices which relay not only >> on one source of 'voltage', i.e. manipulate body bias or have >> other controlling voltage for gates in the new 3D transistors. They >> might provide custom callback function in their cpufreq driver. >> This is used i.e. in cpufreq drivers which use firmware to get power, >> like scmi-cpufreq.c; >> >> To meet this requirement the registration of EM is moved into cpufreq >> drivers, not in the framework i.e cpufreq_online(). If we could limit >> the support for only option 1. then we could move the registration >> call into cpufreq framework and clean the cpufreq drivers. > > I'm not sure to get your point but I think a series setting the scene by > moving the dev_pm_opp_of_register_em() to cpufreq_online() and remove > the cpumask may make sense.
Some of the cpufreq drivers don't use dev_pm_opp_of_register_em() but instead em_register_perf_domain() with their em_data_callback [1]. It is because of point 2. described above. The dev_pm_opp_of_register_em won't work for them, so it's not a good candidate to cover all use cases in the framework.
> > Can you send the split version of patch 1/5 as a series without the > other changes ? So we can focus on first ?
Sure, I will only split patch 1/5 as you suggested and send v6. Thank you for your time and help.
Regards, Lukasz
[1] https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/scmi-cpufreq.c#L203
| |