[PATCH v3 0/4] Clarify abstract scale usage for power values in Energy Model, EAS and IPA

[PATCH v3 0/4] Clarify abstract scale usage for power values in Energy Model, EAS and IPA

[Lukasz Luba]

Hi all,

The Energy Model supports power values expressed in an abstract scale.
This has an impact on Intelligent Power Allocation (IPA) and should be
documented properly. Kernel sub-systems like EAS, IPA and DTPM
(new comming PowerCap framework) would use the new flag to capture
potential miss-configuration where the devices have registered different
power scales, thus cannot operate together.

There was a discussion below v2 of this patch series, which might help
you to get context of these changes [2].

The agreed approach is to have the DT as a source of power values expressed
always in milli-Watts and the only way to submit with abstract scale values
is via the em_dev_register_perf_domain() API.

Changes:
v3:
- added boolean flag to struct em_perf_domain and registration function
  indicating if EM holds real power values in milli-Watts (suggested by
  Daniel and aggreed with Quentin)
- updated documentation regarding this new flag
- dropped DT binding change for 'sustainable-power'
- added more maintainers on CC (due to patch 1/4 touching different things)
v2 [2]:
- updated sustainable power section in IPA documentation
- updated DT binding for the 'sustainable-power'
v1 [1]:
- simple documenation update with new 'abstract scale' in EAS, EM, IPA

Regards,
Lukasz Luba

[1] https://lore.kernel.org/linux-doc/20200929121610.16060-1-lukasz.luba@arm.com/
[2] https://lore.kernel.org/lkml/20201002114426.31277-1-lukasz.luba@arm.com/

Lukasz Luba (4):
  PM / EM: Add a flag indicating units of power values in Energy Model
  docs: Clarify abstract scale usage for power values in Energy Model
  PM / EM: update the comments related to power scale
  docs: power: Update Energy Model with new flag indicating power scale

 .../driver-api/thermal/power_allocator.rst    | 13 +++++++-
 Documentation/power/energy-model.rst          | 30 +++++++++++++++----
 Documentation/scheduler/sched-energy.rst      |  5 ++++
 drivers/cpufreq/scmi-cpufreq.c                |  3 +-
 drivers/opp/of.c                              |  2 +-
 include/linux/energy_model.h                  | 20 ++++++++-----
 kernel/power/energy_model.c                   | 26 ++++++++++++++--
 7 files changed, 81 insertions(+), 18 deletions(-)

-- 
2.17.1

[PATCH v3 1/4] PM / EM: Add a flag indicating units of power values in Energy Model

[Lukasz Luba]

There are different platforms and devices which might use different scale
for the power values. Kernel sub-systems might need to check if all
Energy Model (EM) devices are using the same scale. Address that issue and
store the information inside EM for each device. Thanks to that they can
be easily compared and proper action triggered.

Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/cpufreq/scmi-cpufreq.c |  3 ++-
 drivers/opp/of.c               |  2 +-
 include/linux/energy_model.h   |  9 +++++++--
 kernel/power/energy_model.c    | 24 +++++++++++++++++++++++-
 4 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
index e855e8612a67..3714a4cd07fa 100644
--- a/drivers/cpufreq/scmi-cpufreq.c
+++ b/drivers/cpufreq/scmi-cpufreq.c
@@ -188,7 +188,8 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 	policy->fast_switch_possible =
 		handle->perf_ops->fast_switch_possible(handle, cpu_dev);
 
-	em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, policy->cpus);
+	em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, policy->cpus,
+				    false);
 
 	return 0;
 
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 874b58756220..9e1307061de5 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -1333,7 +1333,7 @@ int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus)
 		goto failed;
 	}
 
-	ret = em_dev_register_perf_domain(dev, nr_opp, &em_cb, cpus);
+	ret = em_dev_register_perf_domain(dev, nr_opp, &em_cb, cpus, true);
 	if (ret)
 		goto failed;
 
diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
index b67a51c574b9..2c31d79bb922 100644
--- a/include/linux/energy_model.h
+++ b/include/linux/energy_model.h
@@ -29,6 +29,8 @@ struct em_perf_state {
  * em_perf_domain - Performance domain
  * @table:		List of performance states, in ascending order
  * @nr_perf_states:	Number of performance states
+ * @milliwatts:		Flag indicating the power values are in milli-Watts
+ *			or some other scale.
  * @cpus:		Cpumask covering the CPUs of the domain. It's here
  *			for performance reasons to avoid potential cache
  *			misses during energy calculations in the scheduler
@@ -43,6 +45,7 @@ struct em_perf_state {
 struct em_perf_domain {
 	struct em_perf_state *table;
 	int nr_perf_states;
+	bool milliwatts;
 	unsigned long cpus[];
 };
 
@@ -79,7 +82,8 @@ struct em_data_callback {
 struct em_perf_domain *em_cpu_get(int cpu);
 struct em_perf_domain *em_pd_get(struct device *dev);
 int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
-				struct em_data_callback *cb, cpumask_t *span);
+				struct em_data_callback *cb, cpumask_t *spani,
+				bool milliwatts);
 void em_dev_unregister_perf_domain(struct device *dev);
 
 /**
@@ -186,7 +190,8 @@ struct em_data_callback {};
 
 static inline
 int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
-				struct em_data_callback *cb, cpumask_t *span)
+				struct em_data_callback *cb, cpumask_t *span,
+				bool milliwatts)
 {
 	return -EINVAL;
 }
diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
index c1ff7fa030ab..efe2a595988e 100644
--- a/kernel/power/energy_model.c
+++ b/kernel/power/energy_model.c
@@ -52,6 +52,17 @@ static int em_debug_cpus_show(struct seq_file *s, void *unused)
 }
 DEFINE_SHOW_ATTRIBUTE(em_debug_cpus);
 
+static int em_debug_units_show(struct seq_file *s, void *unused)
+{
+	struct em_perf_domain *pd = s->private;
+	char *units = pd->milliwatts ? "milliWatts" : "bogoWatts";
+
+	seq_printf(s, "%s\n", units);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(em_debug_units);
+
 static void em_debug_create_pd(struct device *dev)
 {
 	struct dentry *d;
@@ -64,6 +75,8 @@ static void em_debug_create_pd(struct device *dev)
 		debugfs_create_file("cpus", 0444, d, dev->em_pd->cpus,
 				    &em_debug_cpus_fops);
 
+	debugfs_create_file("units", 0444, d, dev->em_pd, &em_debug_units_fops);
+
 	/* Create a sub-directory for each performance state */
 	for (i = 0; i < dev->em_pd->nr_perf_states; i++)
 		em_debug_create_ps(&dev->em_pd->table[i], d);
@@ -250,17 +263,24 @@ EXPORT_SYMBOL_GPL(em_cpu_get);
  * @cpus	: Pointer to cpumask_t, which in case of a CPU device is
  *		obligatory. It can be taken from i.e. 'policy->cpus'. For other
  *		type of devices this should be set to NULL.
+ * @milliwatts	: Flag indicating that the power values are in milliWatts or
+ *		in some other scale. It must be set properly.
  *
  * Create Energy Model tables for a performance domain using the callbacks
  * defined in cb.
  *
+ * The @milliwatts is important to set with correct value. Some kernel
+ * sub-systems might rely on this flag and check if all devices in the EM are
+ * using the same scale.
+ *
  * If multiple clients register the same performance domain, all but the first
  * registration will be ignored.
  *
  * Return 0 on success
  */
 int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
-				struct em_data_callback *cb, cpumask_t *cpus)
+				struct em_data_callback *cb, cpumask_t *cpus,
+				bool milliwatts)
 {
 	unsigned long cap, prev_cap = 0;
 	int cpu, ret;
@@ -313,6 +333,8 @@ int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
 	if (ret)
 		goto unlock;
 
+	dev->em_pd->milliwatts = milliwatts;
+
 	em_debug_create_pd(dev);
 	dev_info(dev, "EM: created perf domain\n");
 
-- 
2.17.1

[PATCH v3 2/4] docs: Clarify abstract scale usage for power values in Energy Model

[Lukasz Luba]

The Energy Model (EM) can store power values in milli-Watts or in abstract
scale. This might cause issues in the subsystems which use the EM for
estimating the device power, such as:
- mixing of different scales in a subsystem which uses multiple
  (cooling) devices (e.g. thermal Intelligent Power Allocation (IPA))
- assuming that energy [milli-Joules] can be derived from the EM power
  values which might not be possible since the power scale doesn't have to
  be in milli-Watts

To avoid misconfiguration add the needed documentation to the EM and
related subsystems: EAS and IPA.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 .../driver-api/thermal/power_allocator.rst          | 13 ++++++++++++-
 Documentation/power/energy-model.rst                | 13 +++++++++++++
 Documentation/scheduler/sched-energy.rst            |  5 +++++
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/Documentation/driver-api/thermal/power_allocator.rst b/Documentation/driver-api/thermal/power_allocator.rst
index 67b6a3297238..b7992ae84fef 100644
--- a/Documentation/driver-api/thermal/power_allocator.rst
+++ b/Documentation/driver-api/thermal/power_allocator.rst
@@ -71,7 +71,10 @@ to the speed-grade of the silicon.  `sustainable_power` is therefore
 simply an estimate, and may be tuned to affect the aggressiveness of
 the thermal ramp. For reference, the sustainable power of a 4" phone
 is typically 2000mW, while on a 10" tablet is around 4500mW (may vary
-depending on screen size).
+depending on screen size). It is possible to have the power value
+expressed in an abstract scale. This is the case when the Energy Model
+provides the power values in an abstract scale. The sustained power
+should be aligned to the scale used by the related cooling devices.
 
 If you are using device tree, do add it as a property of the
 thermal-zone.  For example::
@@ -269,3 +272,11 @@ won't be very good.  Note that this is not particular to this
 governor, step-wise will also misbehave if you call its throttle()
 faster than the normal thermal framework tick (due to interrupts for
 example) as it will overreact.
+
+Energy Model requirements
+=========================
+
+Another important thing is the consistent scale of the power values
+provided by the cooling devices. All of the cooling devices in a single
+thermal zone should have power values reported either in milli-Watts
+or scaled to the same 'abstract scale'.
diff --git a/Documentation/power/energy-model.rst b/Documentation/power/energy-model.rst
index a6fb986abe3c..ba7aa581b307 100644
--- a/Documentation/power/energy-model.rst
+++ b/Documentation/power/energy-model.rst
@@ -20,6 +20,19 @@ possible source of information on its own, the EM framework intervenes as an
 abstraction layer which standardizes the format of power cost tables in the
 kernel, hence enabling to avoid redundant work.
 
+The power values might be expressed in milli-Watts or in an 'abstract scale'.
+Multiple subsystems might use the EM and it is up to the system integrator to
+check that the requirements for the power value scale types are met. An example
+can be found in the Energy-Aware Scheduler documentation
+Documentation/scheduler/sched-energy.rst. For some subsystems like thermal or
+powercap power values expressed in an 'abstract scale' might cause issues.
+These subsystems are more interested in estimation of power used in the past,
+thus the real milli-Watts might be needed. An example of these requirements can
+be found in the Intelligent Power Allocation in
+Documentation/driver-api/thermal/power_allocator.rst.
+Important thing to keep in mind is that when the power values are expressed in
+an 'abstract scale' deriving real energy in milli-Joules would not be possible.
+
 The figure below depicts an example of drivers (Arm-specific here, but the
 approach is applicable to any architecture) providing power costs to the EM
 framework, and interested clients reading the data from it::
diff --git a/Documentation/scheduler/sched-energy.rst b/Documentation/scheduler/sched-energy.rst
index 001e09c95e1d..afe02d394402 100644
--- a/Documentation/scheduler/sched-energy.rst
+++ b/Documentation/scheduler/sched-energy.rst
@@ -350,6 +350,11 @@ independent EM framework in Documentation/power/energy-model.rst.
 Please also note that the scheduling domains need to be re-built after the
 EM has been registered in order to start EAS.
 
+EAS uses the EM to make a forecasting decision on energy usage and thus it is
+more focused on the difference when checking possible options for task
+placement. For EAS it doesn't matter whether the EM power values are expressed
+in milli-Watts or in an 'abstract scale'.
+
 
 6.3 - Energy Model complexity
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-- 
2.17.1

[PATCH v3 3/4] PM / EM: update the comments related to power scale

[Lukasz Luba]

The Energy Model supports power values expressed in milli-Watts or in an
'abstract scale'. Update the related comments is the code to reflect that
state.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 include/linux/energy_model.h | 11 +++++------
 kernel/power/energy_model.c  |  2 +-
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
index 2c31d79bb922..a900b335dd61 100644
--- a/include/linux/energy_model.h
+++ b/include/linux/energy_model.h
@@ -13,9 +13,8 @@
 /**
  * em_perf_state - Performance state of a performance domain
  * @frequency:	The frequency in KHz, for consistency with CPUFreq
- * @power:	The power consumed at this level, in milli-watts (by 1 CPU or
-		by a registered device). It can be a total power: static and
-		dynamic.
+ * @power:	The power consumed at this level (by 1 CPU or by a registered
+ *		device). It can be a total power: static and dynamic.
  * @cost:	The cost coefficient associated with this level, used during
  *		energy calculation. Equal to: power * max_frequency / frequency
  */
@@ -58,7 +57,7 @@ struct em_data_callback {
 	/**
 	 * active_power() - Provide power at the next performance state of
 	 *		a device
-	 * @power	: Active power at the performance state in mW
+	 * @power	: Active power at the performance state
 	 *		(modified)
 	 * @freq	: Frequency at the performance state in kHz
 	 *		(modified)
@@ -69,8 +68,8 @@ struct em_data_callback {
 	 * and frequency.
 	 *
 	 * In case of CPUs, the power is the one of a single CPU in the domain,
-	 * expressed in milli-watts. It is expected to fit in the
-	 * [0, EM_MAX_POWER] range.
+	 * expressed in milli-Watts or an abstract scale. It is expected to
+	 * fit in the [0, EM_MAX_POWER] range.
 	 *
 	 * Return 0 on success.
 	 */
diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
index efe2a595988e..1358fa4abfa8 100644
--- a/kernel/power/energy_model.c
+++ b/kernel/power/energy_model.c
@@ -143,7 +143,7 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd,
 
 		/*
 		 * The power returned by active_state() is expected to be
-		 * positive, in milli-watts and to fit into 16 bits.
+		 * positive and to fit into 16 bits.
 		 */
 		if (!power || power > EM_MAX_POWER) {
 			dev_err(dev, "EM: invalid power: %lu\n",
-- 
2.17.1

[PATCH v3 4/4] docs: power: Update Energy Model with new flag indicating power scale

[Lukasz Luba]

Update description and meaning of a new flag, which indicates the type of
power scale used for a registered Energy Model (EM) device.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 Documentation/power/energy-model.rst | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/Documentation/power/energy-model.rst b/Documentation/power/energy-model.rst
index ba7aa581b307..60ac091d3b0d 100644
--- a/Documentation/power/energy-model.rst
+++ b/Documentation/power/energy-model.rst
@@ -30,6 +30,8 @@ These subsystems are more interested in estimation of power used in the past,
 thus the real milli-Watts might be needed. An example of these requirements can
 be found in the Intelligent Power Allocation in
 Documentation/driver-api/thermal/power_allocator.rst.
+Kernel subsystems might implement automatic detection to check whether EM
+registered devices have inconsistent scale (based on EM internal flag).
 Important thing to keep in mind is that when the power values are expressed in
 an 'abstract scale' deriving real energy in milli-Joules would not be possible.
 
@@ -86,7 +88,7 @@ Drivers are expected to register performance domains into the EM framework by
 calling the following API::
 
   int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
-		struct em_data_callback *cb, cpumask_t *cpus);
+		struct em_data_callback *cb, cpumask_t *cpus, bool milliwatts);
 
 Drivers must provide a callback function returning <frequency, power> tuples
 for each performance state. The callback function provided by the driver is free
@@ -94,6 +96,10 @@ to fetch data from any relevant location (DT, firmware, ...), and by any mean
 deemed necessary. Only for CPU devices, drivers must specify the CPUs of the
 performance domains using cpumask. For other devices than CPUs the last
 argument must be set to NULL.
+The last argument 'milliwatts' is important to set with correct value. Kernel
+subsystems which use EM might rely on this flag to check if all EM devices use
+the same scale. If there are different scales, these subsystems might decide
+to: return warning/error, stop working or panic.
 See Section 3. for an example of driver implementing this
 callback, and kernel/power/energy_model.c for further documentation on this
 API.
@@ -169,7 +175,8 @@ EM framework::
   37     	nr_opp = foo_get_nr_opp(policy);
   38
   39     	/* And register the new performance domain */
-  40     	em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, policy->cpus);
-  41
-  42	        return 0;
-  43	}
+  40     	em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, policy->cpus,
+  41					    true);
+  42
+  43	        return 0;
+  44	}
-- 
2.17.1

Re: [PATCH v3 0/4] Clarify abstract scale usage for power values in Energy Model, EAS and IPA

[Doug Anderson]

Hi,

On Mon, Oct 19, 2020 at 7:06 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi all,
>
> The Energy Model supports power values expressed in an abstract scale.
> This has an impact on Intelligent Power Allocation (IPA) and should be
> documented properly. Kernel sub-systems like EAS, IPA and DTPM
> (new comming PowerCap framework) would use the new flag to capture
> potential miss-configuration where the devices have registered different
> power scales, thus cannot operate together.
>
> There was a discussion below v2 of this patch series, which might help
> you to get context of these changes [2].
>
> The agreed approach is to have the DT as a source of power values expressed
> always in milli-Watts and the only way to submit with abstract scale values
> is via the em_dev_register_perf_domain() API.
>
> Changes:
> v3:
> - added boolean flag to struct em_perf_domain and registration function
>   indicating if EM holds real power values in milli-Watts (suggested by
>   Daniel and aggreed with Quentin)
> - updated documentation regarding this new flag
> - dropped DT binding change for 'sustainable-power'
> - added more maintainers on CC (due to patch 1/4 touching different things)
> v2 [2]:
> - updated sustainable power section in IPA documentation
> - updated DT binding for the 'sustainable-power'
> v1 [1]:
> - simple documenation update with new 'abstract scale' in EAS, EM, IPA
>
> Regards,
> Lukasz Luba
>
> [1] https://lore.kernel.org/linux-doc/20200929121610.16060-1-lukasz.luba@arm.com/
> [2] https://lore.kernel.org/lkml/20201002114426.31277-1-lukasz.luba@arm.com/
>
> Lukasz Luba (4):
>   PM / EM: Add a flag indicating units of power values in Energy Model
>   docs: Clarify abstract scale usage for power values in Energy Model
>   PM / EM: update the comments related to power scale
>   docs: power: Update Energy Model with new flag indicating power scale
>
>  .../driver-api/thermal/power_allocator.rst    | 13 +++++++-
>  Documentation/power/energy-model.rst          | 30 +++++++++++++++----
>  Documentation/scheduler/sched-energy.rst      |  5 ++++
>  drivers/cpufreq/scmi-cpufreq.c                |  3 +-
>  drivers/opp/of.c                              |  2 +-
>  include/linux/energy_model.h                  | 20 ++++++++-----
>  kernel/power/energy_model.c                   | 26 ++++++++++++++--
>  7 files changed, 81 insertions(+), 18 deletions(-)

While I don't feel like I have enough skin in the game to make any
demands, I'm definitely not a huge fan of this series still.  I am a
fan of documenting reality, but (to me) trying to mix stuff like this
is just going to be adding needless complexity.  From where I'm
standing, it's a lot more of a pain to specify these types of numbers
in the firmware than it is to specify them in the device tree.  They
are harder to customize per board, harder to spin, and harder to
specify constraints for everything in the system (all heat generators,
all cooling devices, etc).  ...and since we already have a way to
specify this type of thing in the device tree and that's super easy
for people to do, we're going to end up with weird mixes / matches of
numbers coming from different locations and now we've got to figure
out which numbers we can use when and which to ignore.  Ick.

In my opinion the only way to allow for mixing and matching the
bogoWatts and real Watts would be to actually have units and the
ability to provide a conversion factor somewhere.  Presumably that
might give you a chance of mixing and matching if someone wants to
provide some stuff in device tree and get other stuff from the
firmware.  Heck, I guess you could even magically figure out a
conversion factor if someone provides device tree numbers for
something that was already registered in SCMI, assuming all the SCMI
numbers are consistent with each other...

-Doug



-Doug

Re: [PATCH v3 1/4] PM / EM: Add a flag indicating units of power values in Energy Model

[Doug Anderson]

Hi,

On Mon, Oct 19, 2020 at 7:06 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> There are different platforms and devices which might use different scale
> for the power values. Kernel sub-systems might need to check if all
> Energy Model (EM) devices are using the same scale. Address that issue and
> store the information inside EM for each device. Thanks to that they can
> be easily compared and proper action triggered.
>
> Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/cpufreq/scmi-cpufreq.c |  3 ++-
>  drivers/opp/of.c               |  2 +-
>  include/linux/energy_model.h   |  9 +++++++--
>  kernel/power/energy_model.c    | 24 +++++++++++++++++++++++-
>  4 files changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> index e855e8612a67..3714a4cd07fa 100644
> --- a/drivers/cpufreq/scmi-cpufreq.c
> +++ b/drivers/cpufreq/scmi-cpufreq.c
> @@ -188,7 +188,8 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>         policy->fast_switch_possible =
>                 handle->perf_ops->fast_switch_possible(handle, cpu_dev);
>
> -       em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, policy->cpus);
> +       em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, policy->cpus,
> +                                   false);
>
>         return 0;
>
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 874b58756220..9e1307061de5 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -1333,7 +1333,7 @@ int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus)
>                 goto failed;
>         }
>
> -       ret = em_dev_register_perf_domain(dev, nr_opp, &em_cb, cpus);
> +       ret = em_dev_register_perf_domain(dev, nr_opp, &em_cb, cpus, true);
>         if (ret)
>                 goto failed;
>
> diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
> index b67a51c574b9..2c31d79bb922 100644
> --- a/include/linux/energy_model.h
> +++ b/include/linux/energy_model.h
> @@ -29,6 +29,8 @@ struct em_perf_state {
>   * em_perf_domain - Performance domain
>   * @table:             List of performance states, in ascending order
>   * @nr_perf_states:    Number of performance states
> + * @milliwatts:                Flag indicating the power values are in milli-Watts
> + *                     or some other scale.
>   * @cpus:              Cpumask covering the CPUs of the domain. It's here
>   *                     for performance reasons to avoid potential cache
>   *                     misses during energy calculations in the scheduler
> @@ -43,6 +45,7 @@ struct em_perf_state {
>  struct em_perf_domain {
>         struct em_perf_state *table;
>         int nr_perf_states;
> +       bool milliwatts;
>         unsigned long cpus[];
>  };
>
> @@ -79,7 +82,8 @@ struct em_data_callback {
>  struct em_perf_domain *em_cpu_get(int cpu);
>  struct em_perf_domain *em_pd_get(struct device *dev);
>  int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
> -                               struct em_data_callback *cb, cpumask_t *span);
> +                               struct em_data_callback *cb, cpumask_t *spani,
> +                               bool milliwatts);
>  void em_dev_unregister_perf_domain(struct device *dev);
>
>  /**
> @@ -186,7 +190,8 @@ struct em_data_callback {};
>
>  static inline
>  int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
> -                               struct em_data_callback *cb, cpumask_t *span)
> +                               struct em_data_callback *cb, cpumask_t *span,
> +                               bool milliwatts)
>  {
>         return -EINVAL;
>  }
> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> index c1ff7fa030ab..efe2a595988e 100644
> --- a/kernel/power/energy_model.c
> +++ b/kernel/power/energy_model.c
> @@ -52,6 +52,17 @@ static int em_debug_cpus_show(struct seq_file *s, void *unused)
>  }
>  DEFINE_SHOW_ATTRIBUTE(em_debug_cpus);
>
> +static int em_debug_units_show(struct seq_file *s, void *unused)
> +{
> +       struct em_perf_domain *pd = s->private;
> +       char *units = pd->milliwatts ? "milliWatts" : "bogoWatts";
> +
> +       seq_printf(s, "%s\n", units);
> +
> +       return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(em_debug_units);
> +
>  static void em_debug_create_pd(struct device *dev)
>  {
>         struct dentry *d;
> @@ -64,6 +75,8 @@ static void em_debug_create_pd(struct device *dev)
>                 debugfs_create_file("cpus", 0444, d, dev->em_pd->cpus,
>                                     &em_debug_cpus_fops);
>
> +       debugfs_create_file("units", 0444, d, dev->em_pd, &em_debug_units_fops);
> +
>         /* Create a sub-directory for each performance state */
>         for (i = 0; i < dev->em_pd->nr_perf_states; i++)
>                 em_debug_create_ps(&dev->em_pd->table[i], d);
> @@ -250,17 +263,24 @@ EXPORT_SYMBOL_GPL(em_cpu_get);
>   * @cpus       : Pointer to cpumask_t, which in case of a CPU device is
>   *             obligatory. It can be taken from i.e. 'policy->cpus'. For other
>   *             type of devices this should be set to NULL.
> + * @milliwatts : Flag indicating that the power values are in milliWatts or
> + *             in some other scale. It must be set properly.
>   *
>   * Create Energy Model tables for a performance domain using the callbacks
>   * defined in cb.
>   *
> + * The @milliwatts is important to set with correct value. Some kernel
> + * sub-systems might rely on this flag and check if all devices in the EM are
> + * using the same scale.
> + *
>   * If multiple clients register the same performance domain, all but the first
>   * registration will be ignored.

Should the bullet point above be changed?

I haven't dug through all the code so I may be being naive, but it
seems like if someone registers with "milliWatts" set to true then it
should ignore the old values where milliWatts were false?  Otherwise,
I think, if SCMI registers some numbers first does that mean we can't
later register updated numbers in the device tree?  Also, what happens
when some devices register with milliWatts, some with bogoWatts, and
some with both?  How do we decide what we should be using and what we
should be throwing away?

As per my response in the cover letter, I guess one option would be to
try to figure out a bogoWatts to milliWatts conversion factor the
first time someone tried to register once one way and once the other
way?


-Doug

Re: [PATCH v3 1/4] PM / EM: Add a flag indicating units of power values in Energy Model

[Lukasz Luba]

On 10/20/20 1:17 AM, Doug Anderson wrote:
> Hi,
> 
> On Mon, Oct 19, 2020 at 7:06 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> There are different platforms and devices which might use different scale
>> for the power values. Kernel sub-systems might need to check if all
>> Energy Model (EM) devices are using the same scale. Address that issue and
>> store the information inside EM for each device. Thanks to that they can
>> be easily compared and proper action triggered.
>>
>> Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   drivers/cpufreq/scmi-cpufreq.c |  3 ++-
>>   drivers/opp/of.c               |  2 +-
>>   include/linux/energy_model.h   |  9 +++++++--
>>   kernel/power/energy_model.c    | 24 +++++++++++++++++++++++-
>>   4 files changed, 33 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
>> index e855e8612a67..3714a4cd07fa 100644
>> --- a/drivers/cpufreq/scmi-cpufreq.c
>> +++ b/drivers/cpufreq/scmi-cpufreq.c
>> @@ -188,7 +188,8 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>>          policy->fast_switch_possible =
>>                  handle->perf_ops->fast_switch_possible(handle, cpu_dev);
>>
>> -       em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, policy->cpus);
>> +       em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, policy->cpus,
>> +                                   false);
>>
>>          return 0;
>>
>> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
>> index 874b58756220..9e1307061de5 100644
>> --- a/drivers/opp/of.c
>> +++ b/drivers/opp/of.c
>> @@ -1333,7 +1333,7 @@ int dev_pm_opp_of_register_em(struct device *dev, struct cpumask *cpus)
>>                  goto failed;
>>          }
>>
>> -       ret = em_dev_register_perf_domain(dev, nr_opp, &em_cb, cpus);
>> +       ret = em_dev_register_perf_domain(dev, nr_opp, &em_cb, cpus, true);
>>          if (ret)
>>                  goto failed;
>>
>> diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
>> index b67a51c574b9..2c31d79bb922 100644
>> --- a/include/linux/energy_model.h
>> +++ b/include/linux/energy_model.h
>> @@ -29,6 +29,8 @@ struct em_perf_state {
>>    * em_perf_domain - Performance domain
>>    * @table:             List of performance states, in ascending order
>>    * @nr_perf_states:    Number of performance states
>> + * @milliwatts:                Flag indicating the power values are in milli-Watts
>> + *                     or some other scale.
>>    * @cpus:              Cpumask covering the CPUs of the domain. It's here
>>    *                     for performance reasons to avoid potential cache
>>    *                     misses during energy calculations in the scheduler
>> @@ -43,6 +45,7 @@ struct em_perf_state {
>>   struct em_perf_domain {
>>          struct em_perf_state *table;
>>          int nr_perf_states;
>> +       bool milliwatts;
>>          unsigned long cpus[];
>>   };
>>
>> @@ -79,7 +82,8 @@ struct em_data_callback {
>>   struct em_perf_domain *em_cpu_get(int cpu);
>>   struct em_perf_domain *em_pd_get(struct device *dev);
>>   int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
>> -                               struct em_data_callback *cb, cpumask_t *span);
>> +                               struct em_data_callback *cb, cpumask_t *spani,
>> +                               bool milliwatts);
>>   void em_dev_unregister_perf_domain(struct device *dev);
>>
>>   /**
>> @@ -186,7 +190,8 @@ struct em_data_callback {};
>>
>>   static inline
>>   int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
>> -                               struct em_data_callback *cb, cpumask_t *span)
>> +                               struct em_data_callback *cb, cpumask_t *span,
>> +                               bool milliwatts)
>>   {
>>          return -EINVAL;
>>   }
>> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
>> index c1ff7fa030ab..efe2a595988e 100644
>> --- a/kernel/power/energy_model.c
>> +++ b/kernel/power/energy_model.c
>> @@ -52,6 +52,17 @@ static int em_debug_cpus_show(struct seq_file *s, void *unused)
>>   }
>>   DEFINE_SHOW_ATTRIBUTE(em_debug_cpus);
>>
>> +static int em_debug_units_show(struct seq_file *s, void *unused)
>> +{
>> +       struct em_perf_domain *pd = s->private;
>> +       char *units = pd->milliwatts ? "milliWatts" : "bogoWatts";
>> +
>> +       seq_printf(s, "%s\n", units);
>> +
>> +       return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(em_debug_units);
>> +
>>   static void em_debug_create_pd(struct device *dev)
>>   {
>>          struct dentry *d;
>> @@ -64,6 +75,8 @@ static void em_debug_create_pd(struct device *dev)
>>                  debugfs_create_file("cpus", 0444, d, dev->em_pd->cpus,
>>                                      &em_debug_cpus_fops);
>>
>> +       debugfs_create_file("units", 0444, d, dev->em_pd, &em_debug_units_fops);
>> +
>>          /* Create a sub-directory for each performance state */
>>          for (i = 0; i < dev->em_pd->nr_perf_states; i++)
>>                  em_debug_create_ps(&dev->em_pd->table[i], d);
>> @@ -250,17 +263,24 @@ EXPORT_SYMBOL_GPL(em_cpu_get);
>>    * @cpus       : Pointer to cpumask_t, which in case of a CPU device is
>>    *             obligatory. It can be taken from i.e. 'policy->cpus'. For other
>>    *             type of devices this should be set to NULL.
>> + * @milliwatts : Flag indicating that the power values are in milliWatts or
>> + *             in some other scale. It must be set properly.
>>    *
>>    * Create Energy Model tables for a performance domain using the callbacks
>>    * defined in cb.
>>    *
>> + * The @milliwatts is important to set with correct value. Some kernel
>> + * sub-systems might rely on this flag and check if all devices in the EM are
>> + * using the same scale.
>> + *
>>    * If multiple clients register the same performance domain, all but the first
>>    * registration will be ignored.
> 
> Should the bullet point above be changed?
> 
> I haven't dug through all the code so I may be being naive, but it
> seems like if someone registers with "milliWatts" set to true then it
> should ignore the old values where milliWatts were false?  Otherwise,
> I think, if SCMI registers some numbers first does that mean we can't
> later register updated numbers in the device tree?  Also, what happens
> when some devices register with milliWatts, some with bogoWatts, and
> some with both?  How do we decide what we should be using and what we
> should be throwing away?

When the SCMI is used, there is no DT involved. The SCMI provides
get_power() callback and protocol supports this. The values are coming
from FW. SCMI also requires that the scale is consistent across perf
domains, so no need to 'align' these devices to the same scale.

You cannot register EM twice for the same device, the existing code
already does not allow for that.
The EM internally does not track devices, the device struct has em_pd
field. It is up the the client framework or governor e.g. IPA to
check all power actors if some of them use abstract scale and other
milliWatts.

> 
> As per my response in the cover letter, I guess one option would be to
> try to figure out a bogoWatts to milliWatts conversion factor the
> first time someone tried to register once one way and once the other
> way?

It is not possible.

Regards,
Lukasz

> 
> 
> -Doug
> 

Re: [PATCH v3 0/4] Clarify abstract scale usage for power values in Energy Model, EAS and IPA

[Lukasz Luba]

On 10/20/20 1:15 AM, Doug Anderson wrote:
> Hi,
> 
> On Mon, Oct 19, 2020 at 7:06 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Hi all,
>>
>> The Energy Model supports power values expressed in an abstract scale.
>> This has an impact on Intelligent Power Allocation (IPA) and should be
>> documented properly. Kernel sub-systems like EAS, IPA and DTPM
>> (new comming PowerCap framework) would use the new flag to capture
>> potential miss-configuration where the devices have registered different
>> power scales, thus cannot operate together.
>>
>> There was a discussion below v2 of this patch series, which might help
>> you to get context of these changes [2].
>>
>> The agreed approach is to have the DT as a source of power values expressed
>> always in milli-Watts and the only way to submit with abstract scale values
>> is via the em_dev_register_perf_domain() API.
>>
>> Changes:
>> v3:
>> - added boolean flag to struct em_perf_domain and registration function
>>    indicating if EM holds real power values in milli-Watts (suggested by
>>    Daniel and aggreed with Quentin)
>> - updated documentation regarding this new flag
>> - dropped DT binding change for 'sustainable-power'
>> - added more maintainers on CC (due to patch 1/4 touching different things)
>> v2 [2]:
>> - updated sustainable power section in IPA documentation
>> - updated DT binding for the 'sustainable-power'
>> v1 [1]:
>> - simple documenation update with new 'abstract scale' in EAS, EM, IPA
>>
>> Regards,
>> Lukasz Luba
>>
>> [1] https://lore.kernel.org/linux-doc/20200929121610.16060-1-lukasz.luba@arm.com/
>> [2] https://lore.kernel.org/lkml/20201002114426.31277-1-lukasz.luba@arm.com/
>>
>> Lukasz Luba (4):
>>    PM / EM: Add a flag indicating units of power values in Energy Model
>>    docs: Clarify abstract scale usage for power values in Energy Model
>>    PM / EM: update the comments related to power scale
>>    docs: power: Update Energy Model with new flag indicating power scale
>>
>>   .../driver-api/thermal/power_allocator.rst    | 13 +++++++-
>>   Documentation/power/energy-model.rst          | 30 +++++++++++++++----
>>   Documentation/scheduler/sched-energy.rst      |  5 ++++
>>   drivers/cpufreq/scmi-cpufreq.c                |  3 +-
>>   drivers/opp/of.c                              |  2 +-
>>   include/linux/energy_model.h                  | 20 ++++++++-----
>>   kernel/power/energy_model.c                   | 26 ++++++++++++++--
>>   7 files changed, 81 insertions(+), 18 deletions(-)
> 
> While I don't feel like I have enough skin in the game to make any
> demands, I'm definitely not a huge fan of this series still.  I am a
> fan of documenting reality, but (to me) trying to mix stuff like this
> is just going to be adding needless complexity.  From where I'm
> standing, it's a lot more of a pain to specify these types of numbers
> in the firmware than it is to specify them in the device tree.  They

When you have SCMI, you receive power values from FW directly, not using
DT.

> are harder to customize per board, harder to spin, and harder to
> specify constraints for everything in the system (all heat generators,
> all cooling devices, etc).  ...and since we already have a way to
> specify this type of thing in the device tree and that's super easy
> for people to do, we're going to end up with weird mixes / matches of
> numbers coming from different locations and now we've got to figure
> out which numbers we can use when and which to ignore.  Ick.

This is not that bad as you described. When you have SCMI and FW
all your perf domains should be aligned to the same scale.
In example, you have 4 little CPU, 3 big CPUs, 1 super big CPU,
1 GPU, 1 DSP. For all of them the SCMI get_power callback should return
consistent values. You don't have to specify anything else or rev-eng.
Then a client like EAS would use those values from CPUs to estimate
energy and this works fine. Another client: IPA, which would use
all of them and also works fine.

> 
> In my opinion the only way to allow for mixing and matching the
> bogoWatts and real Watts would be to actually have units and the
> ability to provide a conversion factor somewhere.  Presumably that
> might give you a chance of mixing and matching if someone wants to
> provide some stuff in device tree and get other stuff from the
> firmware.  Heck, I guess you could even magically figure out a
> conversion factor if someone provides device tree numbers for
> something that was already registered in SCMI, assuming all the SCMI
> numbers are consistent with each other...

What you demand here is another code path, just to support revers
engineered power values for SCMI devices, which are stored in DT.
Then the SCMI protocol code and drivers should take them into account
and abandon standard implementation and use these values to provide
'hacked' power numbers to EM. Am I right?
It is not going to happen.

Regards,
Lukasz


> 
> -Doug
> 
> 
> 
> -Doug
> 

Re: [PATCH v3 0/4] Clarify abstract scale usage for power values in Energy Model, EAS and IPA

[Doug Anderson]

Hi,

On Thu, Oct 29, 2020 at 5:37 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> On 10/20/20 1:15 AM, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Oct 19, 2020 at 7:06 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>
> >> Hi all,
> >>
> >> The Energy Model supports power values expressed in an abstract scale.
> >> This has an impact on Intelligent Power Allocation (IPA) and should be
> >> documented properly. Kernel sub-systems like EAS, IPA and DTPM
> >> (new comming PowerCap framework) would use the new flag to capture
> >> potential miss-configuration where the devices have registered different
> >> power scales, thus cannot operate together.
> >>
> >> There was a discussion below v2 of this patch series, which might help
> >> you to get context of these changes [2].
> >>
> >> The agreed approach is to have the DT as a source of power values expressed
> >> always in milli-Watts and the only way to submit with abstract scale values
> >> is via the em_dev_register_perf_domain() API.
> >>
> >> Changes:
> >> v3:
> >> - added boolean flag to struct em_perf_domain and registration function
> >>    indicating if EM holds real power values in milli-Watts (suggested by
> >>    Daniel and aggreed with Quentin)
> >> - updated documentation regarding this new flag
> >> - dropped DT binding change for 'sustainable-power'
> >> - added more maintainers on CC (due to patch 1/4 touching different things)
> >> v2 [2]:
> >> - updated sustainable power section in IPA documentation
> >> - updated DT binding for the 'sustainable-power'
> >> v1 [1]:
> >> - simple documenation update with new 'abstract scale' in EAS, EM, IPA
> >>
> >> Regards,
> >> Lukasz Luba
> >>
> >> [1] https://lore.kernel.org/linux-doc/20200929121610.16060-1-lukasz.luba@arm.com/
> >> [2] https://lore.kernel.org/lkml/20201002114426.31277-1-lukasz.luba@arm.com/
> >>
> >> Lukasz Luba (4):
> >>    PM / EM: Add a flag indicating units of power values in Energy Model
> >>    docs: Clarify abstract scale usage for power values in Energy Model
> >>    PM / EM: update the comments related to power scale
> >>    docs: power: Update Energy Model with new flag indicating power scale
> >>
> >>   .../driver-api/thermal/power_allocator.rst    | 13 +++++++-
> >>   Documentation/power/energy-model.rst          | 30 +++++++++++++++----
> >>   Documentation/scheduler/sched-energy.rst      |  5 ++++
> >>   drivers/cpufreq/scmi-cpufreq.c                |  3 +-
> >>   drivers/opp/of.c                              |  2 +-
> >>   include/linux/energy_model.h                  | 20 ++++++++-----
> >>   kernel/power/energy_model.c                   | 26 ++++++++++++++--
> >>   7 files changed, 81 insertions(+), 18 deletions(-)
> >
> > While I don't feel like I have enough skin in the game to make any
> > demands, I'm definitely not a huge fan of this series still.  I am a
> > fan of documenting reality, but (to me) trying to mix stuff like this
> > is just going to be adding needless complexity.  From where I'm
> > standing, it's a lot more of a pain to specify these types of numbers
> > in the firmware than it is to specify them in the device tree.  They
>
> When you have SCMI, you receive power values from FW directly, not using
> DT.
>
> > are harder to customize per board, harder to spin, and harder to
> > specify constraints for everything in the system (all heat generators,
> > all cooling devices, etc).  ...and since we already have a way to
> > specify this type of thing in the device tree and that's super easy
> > for people to do, we're going to end up with weird mixes / matches of
> > numbers coming from different locations and now we've got to figure
> > out which numbers we can use when and which to ignore.  Ick.
>
> This is not that bad as you described. When you have SCMI and FW
> all your perf domains should be aligned to the same scale.
> In example, you have 4 little CPU, 3 big CPUs, 1 super big CPU,
> 1 GPU, 1 DSP. For all of them the SCMI get_power callback should return
> consistent values. You don't have to specify anything else or rev-eng.
> Then a client like EAS would use those values from CPUs to estimate
> energy and this works fine. Another client: IPA, which would use
> all of them and also works fine.

I guess I'm confused.  When using SCMI and FW, are there already code
paths to get the board-specific "sustainable-power" from SCMI and FW?

I know that "sustainable-power" is not truly necessary.  IIRC some of
the code assumes that the lowest power state of all components must be
sustainable and uses that.  However, though this makes the code work,
it's far from ideal.  I don't want to accept a mediocre solution here.

In any case, I'm saying that even if "sustainable-power" can come from
firmware, it's not as ideal of a place for it to live.  Maybe my
experience on Chromebooks is different from the rest of upstream, but
it's generally quite easy to adjust the device tree for a board and
much harder to convince firmware folks to put a board-specific table
of values.


> > In my opinion the only way to allow for mixing and matching the
> > bogoWatts and real Watts would be to actually have units and the
> > ability to provide a conversion factor somewhere.  Presumably that
> > might give you a chance of mixing and matching if someone wants to
> > provide some stuff in device tree and get other stuff from the
> > firmware.  Heck, I guess you could even magically figure out a
> > conversion factor if someone provides device tree numbers for
> > something that was already registered in SCMI, assuming all the SCMI
> > numbers are consistent with each other...
>
> What you demand here is another code path, just to support revers
> engineered power values for SCMI devices, which are stored in DT.
> Then the SCMI protocol code and drivers should take them into account
> and abandon standard implementation and use these values to provide
> 'hacked' power numbers to EM. Am I right?
> It is not going to happen.

Quite honestly, all I want to be able to do is to provide a
board-specific "sustainable-power" and have it match with the
power-coefficients.  Thus:

* If device tree accepted abstract scale, we'd be done and I'd shut
up.  ...but Rob has made it quite clear that this is a no-go.

* If it was super easy to add all these values into firmware for a
board and we could totally remove these from the device tree, I'd
grumble a bit about firmware being a terrible place for this but at
least we'd have a solution and we'd be done and I'd shut up.  NOTE: I
don't know ATF terribly well, but I'd guess that this needs to go
there?  Presumably part of this is convincing firmware folks to add
this board-specific value there...

-Doug

Re: [PATCH v3 0/4] Clarify abstract scale usage for power values in Energy Model, EAS and IPA

[Lukasz Luba]

On 10/29/20 3:39 PM, Doug Anderson wrote:
> Hi,
> 
> On Thu, Oct 29, 2020 at 5:37 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> On 10/20/20 1:15 AM, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Mon, Oct 19, 2020 at 7:06 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> The Energy Model supports power values expressed in an abstract scale.
>>>> This has an impact on Intelligent Power Allocation (IPA) and should be
>>>> documented properly. Kernel sub-systems like EAS, IPA and DTPM
>>>> (new comming PowerCap framework) would use the new flag to capture
>>>> potential miss-configuration where the devices have registered different
>>>> power scales, thus cannot operate together.
>>>>
>>>> There was a discussion below v2 of this patch series, which might help
>>>> you to get context of these changes [2].
>>>>
>>>> The agreed approach is to have the DT as a source of power values expressed
>>>> always in milli-Watts and the only way to submit with abstract scale values
>>>> is via the em_dev_register_perf_domain() API.
>>>>
>>>> Changes:
>>>> v3:
>>>> - added boolean flag to struct em_perf_domain and registration function
>>>>     indicating if EM holds real power values in milli-Watts (suggested by
>>>>     Daniel and aggreed with Quentin)
>>>> - updated documentation regarding this new flag
>>>> - dropped DT binding change for 'sustainable-power'
>>>> - added more maintainers on CC (due to patch 1/4 touching different things)
>>>> v2 [2]:
>>>> - updated sustainable power section in IPA documentation
>>>> - updated DT binding for the 'sustainable-power'
>>>> v1 [1]:
>>>> - simple documenation update with new 'abstract scale' in EAS, EM, IPA
>>>>
>>>> Regards,
>>>> Lukasz Luba
>>>>
>>>> [1] https://lore.kernel.org/linux-doc/20200929121610.16060-1-lukasz.luba@arm.com/
>>>> [2] https://lore.kernel.org/lkml/20201002114426.31277-1-lukasz.luba@arm.com/
>>>>
>>>> Lukasz Luba (4):
>>>>     PM / EM: Add a flag indicating units of power values in Energy Model
>>>>     docs: Clarify abstract scale usage for power values in Energy Model
>>>>     PM / EM: update the comments related to power scale
>>>>     docs: power: Update Energy Model with new flag indicating power scale
>>>>
>>>>    .../driver-api/thermal/power_allocator.rst    | 13 +++++++-
>>>>    Documentation/power/energy-model.rst          | 30 +++++++++++++++----
>>>>    Documentation/scheduler/sched-energy.rst      |  5 ++++
>>>>    drivers/cpufreq/scmi-cpufreq.c                |  3 +-
>>>>    drivers/opp/of.c                              |  2 +-
>>>>    include/linux/energy_model.h                  | 20 ++++++++-----
>>>>    kernel/power/energy_model.c                   | 26 ++++++++++++++--
>>>>    7 files changed, 81 insertions(+), 18 deletions(-)
>>>
>>> While I don't feel like I have enough skin in the game to make any
>>> demands, I'm definitely not a huge fan of this series still.  I am a
>>> fan of documenting reality, but (to me) trying to mix stuff like this
>>> is just going to be adding needless complexity.  From where I'm
>>> standing, it's a lot more of a pain to specify these types of numbers
>>> in the firmware than it is to specify them in the device tree.  They
>>
>> When you have SCMI, you receive power values from FW directly, not using
>> DT.
>>
>>> are harder to customize per board, harder to spin, and harder to
>>> specify constraints for everything in the system (all heat generators,
>>> all cooling devices, etc).  ...and since we already have a way to
>>> specify this type of thing in the device tree and that's super easy
>>> for people to do, we're going to end up with weird mixes / matches of
>>> numbers coming from different locations and now we've got to figure
>>> out which numbers we can use when and which to ignore.  Ick.
>>
>> This is not that bad as you described. When you have SCMI and FW
>> all your perf domains should be aligned to the same scale.
>> In example, you have 4 little CPU, 3 big CPUs, 1 super big CPU,
>> 1 GPU, 1 DSP. For all of them the SCMI get_power callback should return
>> consistent values. You don't have to specify anything else or rev-eng.
>> Then a client like EAS would use those values from CPUs to estimate
>> energy and this works fine. Another client: IPA, which would use
>> all of them and also works fine.
> 
> I guess I'm confused.  When using SCMI and FW, are there already code
> paths to get the board-specific "sustainable-power" from SCMI and FW?
> 
> I know that "sustainable-power" is not truly necessary.  IIRC some of
> the code assumes that the lowest power state of all components must be
> sustainable and uses that.  However, though this makes the code work,
> it's far from ideal.  I don't want to accept a mediocre solution here.

As you said, sustainable power would be estimated when it is not coming
from DT. Currently it would be done based on lowest allowed OPPs. I am
trying to address this by marking OPP as sustainable [1]. The estimation 
would be more accurate (and also the derived coefficients).

> 
> In any case, I'm saying that even if "sustainable-power" can come from
> firmware, it's not as ideal of a place for it to live.  Maybe my
> experience on Chromebooks is different from the rest of upstream, but
> it's generally quite easy to adjust the device tree for a board and
> much harder to convince firmware folks to put a board-specific table
> of values.

The sysfs (which is there) is even easier for this adjustment than DT.

> 
> 
>>> In my opinion the only way to allow for mixing and matching the
>>> bogoWatts and real Watts would be to actually have units and the
>>> ability to provide a conversion factor somewhere.  Presumably that
>>> might give you a chance of mixing and matching if someone wants to
>>> provide some stuff in device tree and get other stuff from the
>>> firmware.  Heck, I guess you could even magically figure out a
>>> conversion factor if someone provides device tree numbers for
>>> something that was already registered in SCMI, assuming all the SCMI
>>> numbers are consistent with each other...
>>
>> What you demand here is another code path, just to support revers
>> engineered power values for SCMI devices, which are stored in DT.
>> Then the SCMI protocol code and drivers should take them into account
>> and abandon standard implementation and use these values to provide
>> 'hacked' power numbers to EM. Am I right?
>> It is not going to happen.
> 
> Quite honestly, all I want to be able to do is to provide a
> board-specific "sustainable-power" and have it match with the
> power-coefficients.  Thus:
> 
> * If device tree accepted abstract scale, we'd be done and I'd shut
> up.  ...but Rob has made it quite clear that this is a no-go.
> 
> * If it was super easy to add all these values into firmware for a
> board and we could totally remove these from the device tree, I'd
> grumble a bit about firmware being a terrible place for this but at
> least we'd have a solution and we'd be done and I'd shut up.  NOTE: I
> don't know ATF terribly well, but I'd guess that this needs to go
> there?  Presumably part of this is convincing firmware folks to add
> this board-specific value there...

The SCMI spec that we are talking supports 'sustained performance'
level for each performance domain. You can check doc [2] table 11
for the definition. In SCMI there is no concept of 'sustainable-power'
which would substitute the missing DT value. But we can estimate it
more accurately based on sustainable OPP.
You can check how I am going to feed that FW value into the OPP in patch
4/4 of [3]. I am also working on improved estimation patch set v4 for
IPA (some description of an issue in v2 [4], latest v3 is here [5]),
which is using the proposed sustainable OPP concept (Viresh mentioned
he would like to see the user of that).

As you can see, I am not going to leave you with this issue ;)

Regards,
Lukasz


[1] 
https://lore.kernel.org/linux-pm/20201028140847.1018-1-lukasz.luba@arm.com/
[2] https://developer.arm.com/documentation/den0056/b
[3] 
https://lore.kernel.org/linux-pm/20201028140847.1018-5-lukasz.luba@arm.com/
[4] 
https://lore.kernel.org/linux-pm/5f682bbb-b250-49e6-dbb7-aea522a58595@arm.com/
[5] https://lore.kernel.org/lkml/20201009135850.14727-1-lukasz.luba@arm.com/

> 
> -Doug
> 

Re: [PATCH v3 0/4] Clarify abstract scale usage for power values in Energy Model, EAS and IPA

[Lukasz Luba]

On 10/19/20 3:05 PM, Lukasz Luba wrote:
> Hi all,
> 
> The Energy Model supports power values expressed in an abstract scale.
> This has an impact on Intelligent Power Allocation (IPA) and should be
> documented properly. Kernel sub-systems like EAS, IPA and DTPM
> (new comming PowerCap framework) would use the new flag to capture
> potential miss-configuration where the devices have registered different
> power scales, thus cannot operate together.
> 
> There was a discussion below v2 of this patch series, which might help
> you to get context of these changes [2].
> 
> The agreed approach is to have the DT as a source of power values expressed
> always in milli-Watts and the only way to submit with abstract scale values
> is via the em_dev_register_perf_domain() API.
> 
> Changes:
> v3:
> - added boolean flag to struct em_perf_domain and registration function
>    indicating if EM holds real power values in milli-Watts (suggested by
>    Daniel and aggreed with Quentin)
> - updated documentation regarding this new flag
> - dropped DT binding change for 'sustainable-power'
> - added more maintainers on CC (due to patch 1/4 touching different things)
> v2 [2]:
> - updated sustainable power section in IPA documentation
> - updated DT binding for the 'sustainable-power'
> v1 [1]:
> - simple documenation update with new 'abstract scale' in EAS, EM, IPA
> 
> Regards,
> Lukasz Luba
> 
> [1] https://lore.kernel.org/linux-doc/20200929121610.16060-1-lukasz.luba@arm.com/
> [2] https://lore.kernel.org/lkml/20201002114426.31277-1-lukasz.luba@arm.com/
> 
> Lukasz Luba (4):
>    PM / EM: Add a flag indicating units of power values in Energy Model
>    docs: Clarify abstract scale usage for power values in Energy Model
>    PM / EM: update the comments related to power scale
>    docs: power: Update Energy Model with new flag indicating power scale
> 
>   .../driver-api/thermal/power_allocator.rst    | 13 +++++++-
>   Documentation/power/energy-model.rst          | 30 +++++++++++++++----
>   Documentation/scheduler/sched-energy.rst      |  5 ++++
>   drivers/cpufreq/scmi-cpufreq.c                |  3 +-
>   drivers/opp/of.c                              |  2 +-
>   include/linux/energy_model.h                  | 20 ++++++++-----
>   kernel/power/energy_model.c                   | 26 ++++++++++++++--
>   7 files changed, 81 insertions(+), 18 deletions(-)
> 


Gentle ping to Quentin and Daniel for sharing opinion on this patch set.
If you are OK, then I could use this as a base for next work.

As you probably know I am working also on 'sustainable power' estimation
which could be used when there is no DT value but it comes from FW.
That would meet requirement from Doug, when the DT cannot be used,
but we have sustainable levels from FW [1].

Regards,
Lukasz

[1] https://lore.kernel.org/lkml/20201028140847.1018-5-lukasz.luba@arm.com/

Re: [PATCH v3 1/4] PM / EM: Add a flag indicating units of power values in Energy Model

[Quentin Perret]

On Monday 19 Oct 2020 at 15:05:58 (+0100), Lukasz Luba wrote:
> diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
> index b67a51c574b9..2c31d79bb922 100644
> --- a/include/linux/energy_model.h
> +++ b/include/linux/energy_model.h
> @@ -29,6 +29,8 @@ struct em_perf_state {
>   * em_perf_domain - Performance domain
>   * @table:		List of performance states, in ascending order
>   * @nr_perf_states:	Number of performance states
> + * @milliwatts:		Flag indicating the power values are in milli-Watts
> + *			or some other scale.
>   * @cpus:		Cpumask covering the CPUs of the domain. It's here
>   *			for performance reasons to avoid potential cache
>   *			misses during energy calculations in the scheduler
> @@ -43,6 +45,7 @@ struct em_perf_state {
>  struct em_perf_domain {
>  	struct em_perf_state *table;
>  	int nr_perf_states;
> +	bool milliwatts;
>  	unsigned long cpus[];
>  };

Make that an int please, sizeof(bool) is impdef.

With that:

Reviewed-by: Quentin Perret <qperret@google.com>

Re: [PATCH v3 2/4] docs: Clarify abstract scale usage for power values in Energy Model

[Quentin Perret]

On Monday 19 Oct 2020 at 15:05:59 (+0100), Lukasz Luba wrote:
> diff --git a/Documentation/driver-api/thermal/power_allocator.rst b/Documentation/driver-api/thermal/power_allocator.rst
> index 67b6a3297238..b7992ae84fef 100644
> --- a/Documentation/driver-api/thermal/power_allocator.rst
> +++ b/Documentation/driver-api/thermal/power_allocator.rst
> @@ -71,7 +71,10 @@ to the speed-grade of the silicon.  `sustainable_power` is therefore
>  simply an estimate, and may be tuned to affect the aggressiveness of
>  the thermal ramp. For reference, the sustainable power of a 4" phone
>  is typically 2000mW, while on a 10" tablet is around 4500mW (may vary
> -depending on screen size).
> +depending on screen size). It is possible to have the power value
> +expressed in an abstract scale. This is the case when the Energy Model
> +provides the power values in an abstract scale.

Maybe remove one of the 2 sentences?

Thanks,
Quentin

Re: [PATCH v3 3/4] PM / EM: update the comments related to power scale

[Quentin Perret]

On Monday 19 Oct 2020 at 15:06:00 (+0100), Lukasz Luba wrote:
> The Energy Model supports power values expressed in milli-Watts or in an
> 'abstract scale'. Update the related comments is the code to reflect that
> state.

Reviewed-by: Quentin Perret <qperret@google.com>

> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>

Thanks,
Quentin

Re: [PATCH v3 4/4] docs: power: Update Energy Model with new flag indicating power scale

[Quentin Perret]

On Monday 19 Oct 2020 at 15:06:01 (+0100), Lukasz Luba wrote:
> Update description and meaning of a new flag, which indicates the type of
> power scale used for a registered Energy Model (EM) device.

Reviewed-by: Quentin Perret <qperret@google.com>

> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>

Thanks,
Quentin

Re: [PATCH v3 0/4] Clarify abstract scale usage for power values in Energy Model, EAS and IPA

[Quentin Perret]

On Monday 02 Nov 2020 at 08:54:38 (+0000), Lukasz Luba wrote:
> Gentle ping to Quentin and Daniel for sharing opinion on this patch set.
> If you are OK, then I could use this as a base for next work.

One or two small nits, but overall this LGTM. Thanks Lukasz.

> As you probably know I am working also on 'sustainable power' estimation
> which could be used when there is no DT value but it comes from FW.
> That would meet requirement from Doug, when the DT cannot be used,
> but we have sustainable levels from FW [1].

Cool, and also, I'd be happy to hear from Doug if passing the sustained
power via sysfs is good enough for his use-case in the meantime?

Thanks,
Quentin

Re: [PATCH v3 0/4] Clarify abstract scale usage for power values in Energy Model, EAS and IPA

[Doug Anderson]

Hi,

On Mon, Nov 2, 2020 at 5:54 AM Quentin Perret <qperret@google.com> wrote:
>
> On Monday 02 Nov 2020 at 08:54:38 (+0000), Lukasz Luba wrote:
> > Gentle ping to Quentin and Daniel for sharing opinion on this patch set.
> > If you are OK, then I could use this as a base for next work.
>
> One or two small nits, but overall this LGTM. Thanks Lukasz.
>
> > As you probably know I am working also on 'sustainable power' estimation
> > which could be used when there is no DT value but it comes from FW.
> > That would meet requirement from Doug, when the DT cannot be used,
> > but we have sustainable levels from FW [1].
>
> Cool, and also, I'd be happy to hear from Doug if passing the sustained
> power via sysfs is good enough for his use-case in the meantime?

It does sound like sysfs could be made to work for us, but it's
definitely a workaround.  If the normal way to set these values was
through sysfs then it would be fine, but I think most people expect
that these values are just setup properly by the kernel.  That means
anyone using our board with a different userspace (someone running
upstream on it) would need to figure out what mechanism they were
going to use to program them.  There's very little advantage here
compared to a downstream patch that just violates official upstream
policy by putting something bogoWatts based in the device tree.

My current plan of record (which I don't love) is basically:

1. Before devices are in consumer's hands, accept bogoWatts numbers in
our downstream kernel.

2. Once devices are in consumers hands, run the script I sent out to
generate some numbers and post them upstream.

If, at some point, there's a better solution then I'll switch to it,
but until then that seems workable even if it makes me grumpy.


-Doug

Re: [PATCH v3 1/4] PM / EM: Add a flag indicating units of power values in Energy Model

[Lukasz Luba]

On 11/2/20 1:43 PM, Quentin Perret wrote:
> On Monday 19 Oct 2020 at 15:05:58 (+0100), Lukasz Luba wrote:
>> diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
>> index b67a51c574b9..2c31d79bb922 100644
>> --- a/include/linux/energy_model.h
>> +++ b/include/linux/energy_model.h
>> @@ -29,6 +29,8 @@ struct em_perf_state {
>>    * em_perf_domain - Performance domain
>>    * @table:		List of performance states, in ascending order
>>    * @nr_perf_states:	Number of performance states
>> + * @milliwatts:		Flag indicating the power values are in milli-Watts
>> + *			or some other scale.
>>    * @cpus:		Cpumask covering the CPUs of the domain. It's here
>>    *			for performance reasons to avoid potential cache
>>    *			misses during energy calculations in the scheduler
>> @@ -43,6 +45,7 @@ struct em_perf_state {
>>   struct em_perf_domain {
>>   	struct em_perf_state *table;
>>   	int nr_perf_states;
>> +	bool milliwatts;
>>   	unsigned long cpus[];
>>   };
> 
> Make that an int please, sizeof(bool) is impdef.

OK, I will change it.

> 
> With that:
> 
> Reviewed-by: Quentin Perret <qperret@google.com>
> 

Thank you for the review.

Re: [PATCH v3 2/4] docs: Clarify abstract scale usage for power values in Energy Model

[Lukasz Luba]

On 11/2/20 1:45 PM, Quentin Perret wrote:
> On Monday 19 Oct 2020 at 15:05:59 (+0100), Lukasz Luba wrote:
>> diff --git a/Documentation/driver-api/thermal/power_allocator.rst b/Documentation/driver-api/thermal/power_allocator.rst
>> index 67b6a3297238..b7992ae84fef 100644
>> --- a/Documentation/driver-api/thermal/power_allocator.rst
>> +++ b/Documentation/driver-api/thermal/power_allocator.rst
>> @@ -71,7 +71,10 @@ to the speed-grade of the silicon.  `sustainable_power` is therefore
>>   simply an estimate, and may be tuned to affect the aggressiveness of
>>   the thermal ramp. For reference, the sustainable power of a 4" phone
>>   is typically 2000mW, while on a 10" tablet is around 4500mW (may vary
>> -depending on screen size).
>> +depending on screen size). It is possible to have the power value
>> +expressed in an abstract scale. This is the case when the Energy Model
>> +provides the power values in an abstract scale.
> 
> Maybe remove one of the 2 sentences?

I will remove the 2nd sentence.

> 
> Thanks,
> Quentin
> 

Re: [PATCH v3 0/4] Clarify abstract scale usage for power values in Energy Model, EAS and IPA

[Lukasz Luba]

On 11/2/20 1:54 PM, Quentin Perret wrote:
> On Monday 02 Nov 2020 at 08:54:38 (+0000), Lukasz Luba wrote:
>> Gentle ping to Quentin and Daniel for sharing opinion on this patch set.
>> If you are OK, then I could use this as a base for next work.
> 
> One or two small nits, but overall this LGTM. Thanks Lukasz.

Thank you Quentin for the review. I am going to send v4 with these small
changes.

Regards,
Lukasz

> 
>> As you probably know I am working also on 'sustainable power' estimation
>> which could be used when there is no DT value but it comes from FW.
>> That would meet requirement from Doug, when the DT cannot be used,
>> but we have sustainable levels from FW [1].
> 
> Cool, and also, I'd be happy to hear from Doug if passing the sustained
> power via sysfs is good enough for his use-case in the meantime?
> 
> Thanks,
> Quentin
>