Hi Dietmar,
On 1/20/20 2:53 PM, Dietmar Eggemann wrote:
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()?
It was it the old implementation, I will remove 'em_dev_get()' from the doc. The em_pd_get() is OK for me, I can change it.
+---------+ | +---------+ | | | 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 ...
Right, I could modify the pd_init code to use one 'em_get_pd' API and remove the 'em_cpu_get'.
[...]
+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 ...
The EM_SIMPLE is set in the em_register_perf_domain() to distinguish CPU device which has populated 'priv' pointer and set EM_CPU. We can just rely on 'priv == NULL' to check if we are dealing with a CPU EM. Do you prefer this approach and get rid of em_type?
Then the code would look like:
if (em_pd->priv) seq_puts(s, "EM_CPU\n"); else seq_puts(s, "EM_SIMPLE\n");
Regards, Lukasz
[...]