| Message ID | 20200116152032.11301-1-lukasz.luba@arm.com |
|---|---|
| Headers | show |
| Series | Add support for devices in the Energy Model | expand |
Hey Lukasz, Still reading through this, but with small changes, this looks pretty good to me. On Thursday 16 Jan 2020 at 15:20:29 (+0000), lukasz.luba@arm.com wrote: > +int em_register_perf_domain(struct device *dev, unsigned int nr_states, > + struct em_data_callback *cb) > { > unsigned long cap, prev_cap = 0; > struct em_perf_domain *pd; > - int cpu, ret = 0; > + struct em_device *em_dev; > + cpumask_t *span = NULL; > + int cpu, ret; > > - if (!span || !nr_states || !cb) > + if (!dev || !nr_states || !cb || !cb->active_power) Nit: you check !cb->active_power in em_create_pd() too I think, so only one of the two is needed. > return -EINVAL; > > - /* > - * Use a mutex to serialize the registration of performance domains and > - * let the driver-defined callback functions sleep. > - */ > mutex_lock(&em_pd_mutex); > > - for_each_cpu(cpu, span) { > - /* Make sure we don't register again an existing domain. */ > - if (READ_ONCE(per_cpu(em_data, cpu))) { > + if (_is_cpu_device(dev)) { > + span = kzalloc(cpumask_size(), GFP_KERNEL); > + if (!span) { > + mutex_unlock(&em_pd_mutex); > + return -ENOMEM; > + } > + > + ret = dev_pm_opp_get_sharing_cpus(dev, span); > + if (ret) > + goto free_cpumask; That I think should be changed. This creates some dependency on PM_OPP for the EM framework. And in fact, the reason we came up with PM_EM was precisely to not depend on PM_OPP which was deemed too Arm-specific. Suggested alternative: have two registration functions like so: int em_register_dev_pd(struct device *dev, unsigned int nr_states, struct em_data_callback *cb); int em_register_cpu_pd(cpumask_t *span, unsigned int nr_states, struct em_data_callback *cb); where em_register_cpu_pd() does the CPU-specific work and then calls em_register_dev_pd() (instead of having that big if (_is_cpu_device(dev)) as you currently have). Would that work ? Another possibility would be to query CPUFreq instead of PM_OPP to get the mask, but I'd need to look again at the driver registration path in CPUFreq to see if the policy masks have been populated when we enter PM_EM ... I am not sure if this is the case, but it's worth having a look too. Thanks, Quentin
On 16/01/2020 16:20, lukasz.luba@arm.com wrote: > From: Lukasz Luba <lukasz.luba@arm.com> > > 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. [...] > -The source of the information about the power consumed by CPUs can vary greatly > +The source of the information about the power consumed by devices can vary greatly > from one platform to another. These power costs can be estimated using > devicetree data in some cases. In others, the firmware will know better. > Alternatively, userspace might be best positioned. And so on. In order to avoid > @@ -26,7 +28,7 @@ framework, and interested clients reading the data from it:: > | Thermal (IPA) | | Scheduler (EAS) | | Other | > +---------------+ +-----------------+ +---------------+ > | | em_pd_energy() | > - | | em_cpu_get() | > + | em_dev_get() | em_cpu_get() | Looked really hard but can't find a em_dev_get() in the code? You mean em_get_pd() ? And why em_get_pd() and not em_pd_get()? > +---------+ | +---------+ > | | | > v v v > @@ -47,12 +49,12 @@ framework, and interested clients reading the data from it:: > | Device Tree | | Firmware | | ? | > +--------------+ +---------------+ +--------------+ [...] > +There is two API functions which provide the access to the energy model: > +em_cpu_get() which takes CPU id as an argument and em_dev_get() with device > +pointer as an argument. It depends on the subsystem which interface it is > +going to use. Would be really nice if this wouldn't be required. We should really aim for 1 framework == 1 set of interfaces. What happens if someone calls em_get_pd() on a CPU EM? E.g: static struct perf_domain *pd_init(int cpu) { - struct em_perf_domain *obj = em_cpu_get(cpu); + struct device *dev = get_cpu_device(cpu); + struct em_perf_domain *obj = em_pd_get(dev); struct perf_domain *pd; Two versions of one functionality will confuse API user from the beginning ... [...] > +enum em_type { > + EM_SIMPLE, > + EM_CPU, > +}; s/EM_SIMPLE/EM_DEV ? Right now I only see energy models and _one_ specific type (the CPU EM). So a tag 'is a CPU EM' would suffice. No need for EM_SIMPE ... [...]
On 20/01/2020 15:53, Dietmar Eggemann wrote: > On 16/01/2020 16:20, lukasz.luba@arm.com wrote: >> From: Lukasz Luba <lukasz.luba@arm.com> [...] >> +enum em_type { >> + EM_SIMPLE, >> + EM_CPU, >> +}; > > s/EM_SIMPLE/EM_DEV ? > > Right now I only see energy models and _one_ specific type (the CPU EM). > So a tag 'is a CPU EM' would suffice. No need for EM_SIMPE ... Wait, you even have struct em_device { struct em_perf_domain *em_pd; struct device *dev; ... } static bool _is_cpu_device(struct device *dev) Shouldn't this be enough to distinguish between EM and special CPU EM under the API? Even when required to use container_of() to get from em_perf_domain to device.
On Monday 20 Jan 2020 at 15:53:35 (+0100), Dietmar Eggemann wrote: > Would be really nice if this wouldn't be required. We should really aim > for 1 framework == 1 set of interfaces. > > What happens if someone calls em_get_pd() on a CPU EM? > > E.g: > > static struct perf_domain *pd_init(int cpu) > { > - struct em_perf_domain *obj = em_cpu_get(cpu); > + struct device *dev = get_cpu_device(cpu); > + struct em_perf_domain *obj = em_pd_get(dev); > struct perf_domain *pd; > > Two versions of one functionality will confuse API user from the > beginning ... Agreed, this looks a bit confusing. It should be trivial to make em_dev_get() (or whatever we end up calling it) work for CPUs too, though. And we could always have a em_cpu_get(int cpu) API that is a basically a wrapper around em_dev_get() for convenience. Thanks, Quentin
On 1/20/20 3:28 PM, Quentin Perret wrote: > On Monday 20 Jan 2020 at 15:53:35 (+0100), Dietmar Eggemann wrote: >> Would be really nice if this wouldn't be required. We should really aim >> for 1 framework == 1 set of interfaces. >> >> What happens if someone calls em_get_pd() on a CPU EM? >> >> E.g: >> >> static struct perf_domain *pd_init(int cpu) >> { >> - struct em_perf_domain *obj = em_cpu_get(cpu); >> + struct device *dev = get_cpu_device(cpu); >> + struct em_perf_domain *obj = em_pd_get(dev); >> struct perf_domain *pd; >> >> Two versions of one functionality will confuse API user from the >> beginning ... > > Agreed, this looks a bit confusing. It should be trivial to make > em_dev_get() (or whatever we end up calling it) work for CPUs too, > though. And we could always have a em_cpu_get(int cpu) API that is a > basically a wrapper around em_dev_get() for convenience. The problem not only here is that we have a CPU index 'int cpu' and if we ask for device like: struct device *dev = get_cpu_device(cpu); It might be not the same device that was used during the registration, when we had i.e. 4 CPUs for the same policy: int cpu_id = cpumask_first(policy->cpus); struct device *cpu_dev = get_cpu_device(cpu_id); em_register_perf_domain(cpu_dev, nr_opp, &em_cb); That's why the em_cpu_get() is different than em_get_pd(), mainly by: if (cpumask_test_cpu(cpu, em_span_cpus(em_pd))) It won't be simple wrapper, let me think how it could be handled differently than it is now. Regards, Lukasz > > Thanks, > Quentin >
On 1/20/20 6:27 PM, Dietmar Eggemann wrote: > On 20/01/2020 16:09, Quentin Perret wrote: >> Hey Lukasz, >> >> On Monday 20 Jan 2020 at 14:52:07 (+0000), Lukasz Luba wrote: >>> On 1/17/20 10:54 AM, Quentin Perret wrote: >>>> Suggested alternative: have two registration functions like so: >>>> >>>> int em_register_dev_pd(struct device *dev, unsigned int nr_states, >>>> struct em_data_callback *cb); >>>> int em_register_cpu_pd(cpumask_t *span, unsigned int nr_states, >>>> struct em_data_callback *cb); >>> >>> Interesting, in the internal review Dietmar asked me to remove these two >>> functions. I had the same idea, which would simplify a bit the >>> registration and it does not need to check the dev->bus if it is CPU. >>> >>> Unfortunately, we would need also two function in drivers/opp/of.c: >>> dev_pm_opp_of_register_cpu_em(policy->cpus); >>> and >>> dev_pm_opp_of_register_dev_em(dev); >>> >>> Thus, I have created only one registration function, which you can see >>> in this patch set. >> >> Right, I can see how having a unified API would be appealing, but the >> OPP dependency is a nono, so we'll need to work around one way or >> another. >> >> FWIW, I don't think having separate APIs for CPUs and other devices is >> that bad given that we already have entirely different frameworks to >> drive their respective frequencies. And the _cpu variants are basically >> just wrappers around the _dev ones, so not too bad either IMO :). > > It's true that we need the policy->cpus cpumask only for cpu devices and > we have it available when we call em_register_perf_domain() > [scmi-cpufreq.c driver] or the OPP wrapper dev_pm_opp_of_register_em() > [e.g. cpufreq-dt.c driver]. > > And we shouldn't make EM code dependent on OPP. > > But can't we add 'struct cpumask *mask' as an additional argument to > both which can be set to NULL for (devfreq) devices? > > We can check in em_register_perf_domain() that we got a valid cpumask > for a cpu device and ignore it for (devfreq) devices. > I think we could avoid this additional argument 'cpumask'. I have checked the cpufreq_cpu_get function, which should do be good for this: ---------->8------------------------- static int _get_sharing_cpus(struct device *cpu_dev, struct cpumask *span) { struct cpufreq_policy *policy; policy = cpufreq_cpu_get(cpu_dev->id); if (policy) { cpumask_copy(span, policy->cpus); cpufreq_cpu_put(policy); return 0; } else { return -EINVAL; } } --------------------------8<------------------------------- It would be a replacement for: ret = dev_pm_opp_get_sharing_cpus(dev, span); Regards, Lukasz
On Monday 20 Jan 2020 at 18:38:41 (+0000), Lukasz Luba wrote: > I think we could avoid this additional argument 'cpumask'. I have > checked the cpufreq_cpu_get function, which should do be good for this: > > ---------->8------------------------- > static int _get_sharing_cpus(struct device *cpu_dev, struct cpumask *span) > { > struct cpufreq_policy *policy; > > policy = cpufreq_cpu_get(cpu_dev->id); > if (policy) { > cpumask_copy(span, policy->cpus); That should be with 'policy->related_cpus', but yes if the policy cpumasks have been populated this approach is OK I think. > cpufreq_cpu_put(policy); > return 0; > } else { > return -EINVAL; > } > } > --------------------------8<------------------------------- Thanks, Quentin
From: Lukasz Luba <lukasz.luba@arm.com> Hi all, This patch set introduces support for devices in the Energy Model (EM) framework. It will unify the power model for thermal subsystem and make it simpler. The 1st patch refactors EM framework and adds support for devices. The 2nd patch changes dev_pm_opp_of_register_em() in OPP/OF which now should take as an argument struct device pointer. It touches a few trees (OMAP, NXP, mediatek) updating their CPUfreq drivers to the new interface. Patch 3 changes thermal devfreq cooling removing old code for calculating local power table. It simplifies the code and uses EM for requested power calculation. Last patch 4 is based on 'drm-misc-next' and adds EM to Panfrost driver. The patch set is based on v5.5-rc6 tag (excluding last patch). Regards, Lukasz Luba Lukasz Luba (4): PM / EM: and devices to Energy Model OPP: change parameter to device pointer in dev_pm_opp_of_register_em() thermal: devfreq_cooling: Refactor code and switch to use Energy Model drm/panfrost: Register to the Energy Model with devfreq device Documentation/power/energy-model.rst | 67 ++-- drivers/cpufreq/cpufreq-dt.c | 2 +- drivers/cpufreq/imx6q-cpufreq.c | 2 +- drivers/cpufreq/mediatek-cpufreq.c | 2 +- drivers/cpufreq/omap-cpufreq.c | 2 +- drivers/cpufreq/qcom-cpufreq-hw.c | 2 +- drivers/cpufreq/scmi-cpufreq.c | 11 +- drivers/cpufreq/scpi-cpufreq.c | 2 +- drivers/gpu/drm/panfrost/panfrost_devfreq.c | 3 + drivers/opp/of.c | 43 +-- drivers/thermal/cpu_cooling.c | 10 +- drivers/thermal/devfreq_cooling.c | 397 +++++++------------ include/linux/devfreq_cooling.h | 17 - include/linux/energy_model.h | 108 ++++-- include/linux/pm_opp.h | 13 +- include/trace/events/thermal.h | 19 +- kernel/power/energy_model.c | 406 ++++++++++++++++---- kernel/sched/sched.h | 2 +- kernel/sched/topology.c | 4 +- 19 files changed, 631 insertions(+), 481 deletions(-)