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

[PATCH v2 0/3] 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. There is also a need to update the DT binding for the
'sustainable-power' and allow it to have abstract scale as well.

Changes:
v2:
- updated sustainable power section in IPA documentation
- updated DT binding for the 'sustainable-power'

The v1 of the patch set and related discussion can be found in [1].

Regards,
Lukasz Luba

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

Lukasz Luba (3):
  docs: Clarify abstract scale usage for power values in Energy Model
  PM / EM: update the comments related to power scale
  dt-bindings: thermal: update sustainable-power with abstract scale

 .../devicetree/bindings/thermal/thermal-zones.yaml  | 13 +++++++++----
 .../driver-api/thermal/power_allocator.rst          | 13 ++++++++++++-
 Documentation/power/energy-model.rst                | 13 +++++++++++++
 Documentation/scheduler/sched-energy.rst            |  5 +++++
 include/linux/energy_model.h                        | 11 +++++------
 kernel/power/energy_model.c                         |  2 +-
 6 files changed, 45 insertions(+), 12 deletions(-)

-- 
2.17.1

[PATCH v2 1/3] 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 v2 2/3] 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 b67a51c574b9..b19146b31760 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
  */
@@ -55,7 +54,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)
@@ -66,8 +65,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 c1ff7fa030ab..2bd2afbb83f5 100644
--- a/kernel/power/energy_model.c
+++ b/kernel/power/energy_model.c
@@ -130,7 +130,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 v2 3/3] dt-bindings: thermal: update sustainable-power with abstract scale

[Lukasz Luba]

Update the documentation for the binding 'sustainable-power' and allow
to provide values in an abstract scale. It is required when the cooling
devices use an abstract scale for their power values.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 .../devicetree/bindings/thermal/thermal-zones.yaml  | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
index 3ec9cc87ec50..4d8f2e37d1e6 100644
--- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
+++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
@@ -99,10 +99,15 @@ patternProperties:
       sustainable-power:
         $ref: /schemas/types.yaml#/definitions/uint32
         description:
-          An estimate of the sustainable power (in mW) that this thermal zone
-          can dissipate at the desired control temperature. For reference, the
-          sustainable power of a 4-inch phone is typically 2000mW, while on a
-          10-inch tablet is around 4500mW.
+          An estimate of the sustainable power (in mW or in an abstract scale)
+	  that this thermal zone can dissipate at the desired control
+	  temperature. For reference, the sustainable power of a 4-inch phone
+	  is typically 2000mW, while on a 10-inch tablet is around 4500mW.
+
+	  It is possible to express the sustainable power in an abstract
+	  scale. This is the case when the related cooling devices use also
+	  abstract scale to express their power usage. The scale must be
+	  consistent.
 
       trips:
         type: object
-- 
2.17.1

Re: [PATCH v2 3/3] dt-bindings: thermal: update sustainable-power with abstract scale

[Doug Anderson]

Hi,

On Fri, Oct 2, 2020 at 4:45 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Update the documentation for the binding 'sustainable-power' and allow
> to provide values in an abstract scale. It is required when the cooling
> devices use an abstract scale for their power values.
>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  .../devicetree/bindings/thermal/thermal-zones.yaml  | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> index 3ec9cc87ec50..4d8f2e37d1e6 100644
> --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> @@ -99,10 +99,15 @@ patternProperties:
>        sustainable-power:
>          $ref: /schemas/types.yaml#/definitions/uint32
>          description:
> -          An estimate of the sustainable power (in mW) that this thermal zone
> -          can dissipate at the desired control temperature. For reference, the
> -          sustainable power of a 4-inch phone is typically 2000mW, while on a
> -          10-inch tablet is around 4500mW.
> +          An estimate of the sustainable power (in mW or in an abstract scale)
> +         that this thermal zone can dissipate at the desired control
> +         temperature. For reference, the sustainable power of a 4-inch phone
> +         is typically 2000mW, while on a 10-inch tablet is around 4500mW.
> +
> +         It is possible to express the sustainable power in an abstract
> +         scale. This is the case when the related cooling devices use also
> +         abstract scale to express their power usage. The scale must be
> +         consistent.

Two thoughts:

1. If we're going to allow "sustainable-power" to be in abstract
scale, why not allow "dynamic-power-coefficient" to be in abstract
scale too?  I assume that the whole reason against that originally was
the idea of device tree purity, but if we're allowing the abstract
scale here then there seems no reason not to allow it for
"dynamic-power-coefficient".

2. Is it worth adding some type of indication of what type of units
"sustainable-power" is represented in?  Maybe even a made up unit so
that you could tell the difference between made up units in the same
system?  I'd envision something like:

sustainable-power-units = "qualcomm,sc7180-bogoWatts"

...and on the dynamic-power-coefficient side, the same:

dynamic-power-coefficient-units = "qualcomm,sc7180-bogoWatts"

One could imagine someone even later (after devices are widely
distributed) figuring out translations between these bogoWatts numbers
and real Watts if someone could come up with a case where it matters.


-Doug

Re: [PATCH v2 3/3] dt-bindings: thermal: update sustainable-power with abstract scale

[Lukasz Luba]

Hi Doug,

On 10/2/20 3:31 PM, Doug Anderson wrote:
> Hi,
> 
> On Fri, Oct 2, 2020 at 4:45 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Update the documentation for the binding 'sustainable-power' and allow
>> to provide values in an abstract scale. It is required when the cooling
>> devices use an abstract scale for their power values.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   .../devicetree/bindings/thermal/thermal-zones.yaml  | 13 +++++++++----
>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
>> index 3ec9cc87ec50..4d8f2e37d1e6 100644
>> --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
>> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
>> @@ -99,10 +99,15 @@ patternProperties:
>>         sustainable-power:
>>           $ref: /schemas/types.yaml#/definitions/uint32
>>           description:
>> -          An estimate of the sustainable power (in mW) that this thermal zone
>> -          can dissipate at the desired control temperature. For reference, the
>> -          sustainable power of a 4-inch phone is typically 2000mW, while on a
>> -          10-inch tablet is around 4500mW.
>> +          An estimate of the sustainable power (in mW or in an abstract scale)
>> +         that this thermal zone can dissipate at the desired control
>> +         temperature. For reference, the sustainable power of a 4-inch phone
>> +         is typically 2000mW, while on a 10-inch tablet is around 4500mW.
>> +
>> +         It is possible to express the sustainable power in an abstract
>> +         scale. This is the case when the related cooling devices use also
>> +         abstract scale to express their power usage. The scale must be
>> +         consistent.
> 
> Two thoughts:
> 
> 1. If we're going to allow "sustainable-power" to be in abstract
> scale, why not allow "dynamic-power-coefficient" to be in abstract
> scale too?  I assume that the whole reason against that originally was
> the idea of device tree purity, but if we're allowing the abstract
> scale here then there seems no reason not to allow it for
> "dynamic-power-coefficient".

With this binding it's a bit more tricky.
I also have to discuss a few things internally. This requirement of
uW/MHz/V^2 makes the code easier also for potential drivers
like GPU (which are going to register the devfreq cooling with EM).

Let me think about it, but for now I would just update these bits.
These are required to proper IPA operation, the dyn.-pow.-coef. is a
nice to have and possible next step.

> 
> 2. Is it worth adding some type of indication of what type of units
> "sustainable-power" is represented in?  Maybe even a made up unit so
> that you could tell the difference between made up units in the same
> system?  I'd envision something like:
> 
> sustainable-power-units = "qualcomm,sc7180-bogoWatts"
> 
> ...and on the dynamic-power-coefficient side, the same:
> 
> dynamic-power-coefficient-units = "qualcomm,sc7180-bogoWatts"
> 
> One could imagine someone even later (after devices are widely
> distributed) figuring out translations between these bogoWatts numbers
> and real Watts if someone could come up with a case where it matters.

To figure this out we don't need a new binding.
I think a simple comment in the DT would be enough for this, even e.g.:

sustainable-power = <100> /* bogoWatts */


Thank you for your comments.
BTW, I haven't put your 'Reviewed-by' because I have added this
sustainable-power new stuff in patch 1/3. I will grateful if you
have a look on that.

Regards,
Lukasz

Re: [PATCH v2 3/3] dt-bindings: thermal: update sustainable-power with abstract scale

[Doug Anderson]

Hi,

On Fri, Oct 2, 2020 at 8:13 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Doug,
>
> On 10/2/20 3:31 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Oct 2, 2020 at 4:45 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>
> >> Update the documentation for the binding 'sustainable-power' and allow
> >> to provide values in an abstract scale. It is required when the cooling
> >> devices use an abstract scale for their power values.
> >>
> >> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> >> ---
> >>   .../devicetree/bindings/thermal/thermal-zones.yaml  | 13 +++++++++----
> >>   1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> >> index 3ec9cc87ec50..4d8f2e37d1e6 100644
> >> --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> >> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> >> @@ -99,10 +99,15 @@ patternProperties:
> >>         sustainable-power:
> >>           $ref: /schemas/types.yaml#/definitions/uint32
> >>           description:
> >> -          An estimate of the sustainable power (in mW) that this thermal zone
> >> -          can dissipate at the desired control temperature. For reference, the
> >> -          sustainable power of a 4-inch phone is typically 2000mW, while on a
> >> -          10-inch tablet is around 4500mW.
> >> +          An estimate of the sustainable power (in mW or in an abstract scale)
> >> +         that this thermal zone can dissipate at the desired control
> >> +         temperature. For reference, the sustainable power of a 4-inch phone
> >> +         is typically 2000mW, while on a 10-inch tablet is around 4500mW.
> >> +
> >> +         It is possible to express the sustainable power in an abstract
> >> +         scale. This is the case when the related cooling devices use also
> >> +         abstract scale to express their power usage. The scale must be
> >> +         consistent.
> >
> > Two thoughts:
> >
> > 1. If we're going to allow "sustainable-power" to be in abstract
> > scale, why not allow "dynamic-power-coefficient" to be in abstract
> > scale too?  I assume that the whole reason against that originally was
> > the idea of device tree purity, but if we're allowing the abstract
> > scale here then there seems no reason not to allow it for
> > "dynamic-power-coefficient".
>
> With this binding it's a bit more tricky.
> I also have to discuss a few things internally. This requirement of
> uW/MHz/V^2 makes the code easier also for potential drivers
> like GPU (which are going to register the devfreq cooling with EM).
>
> Let me think about it, but for now I would just update these bits.
> These are required to proper IPA operation, the dyn.-pow.-coef. is a
> nice to have and possible next step.

I guess the problem is that Rajendra is currently planning to remove
all the "dynamic-power-coefficient" values from device tree right now
and move them to the source code because the numbers we currently have
in the device tree _are_ in abstract scale and thus violate the
bindings.  Moving this to source code won't help us get to more real
power numbers (since it'll still be abstract scale), it'll just be
pure churn.  If we're OK with the abstract scale in general then we
should allow it everywhere and not add churn for no reason.


> > 2. Is it worth adding some type of indication of what type of units
> > "sustainable-power" is represented in?  Maybe even a made up unit so
> > that you could tell the difference between made up units in the same
> > system?  I'd envision something like:
> >
> > sustainable-power-units = "qualcomm,sc7180-bogoWatts"
> >
> > ...and on the dynamic-power-coefficient side, the same:
> >
> > dynamic-power-coefficient-units = "qualcomm,sc7180-bogoWatts"
> >
> > One could imagine someone even later (after devices are widely
> > distributed) figuring out translations between these bogoWatts numbers
> > and real Watts if someone could come up with a case where it matters.
>
> To figure this out we don't need a new binding.
> I think a simple comment in the DT would be enough for this, even e.g.:
>
> sustainable-power = <100> /* bogoWatts */

There are some important differences:

a) Your comment is gone when the device tree is compiled.  If we
actually add a string to the device tree then, in theory, we can add
conversions in code (without touching the device tree) down the road.

b) I believe there can be more than one abstract scale present in a
single device tree, at least in theory.  Adding a string allows you to
know if you're comparing apples to apples or apples to organges.


> Thank you for your comments.
> BTW, I haven't put your 'Reviewed-by' because I have added this
> sustainable-power new stuff in patch 1/3. I will grateful if you
> have a look on that.

I can if needed, but I'd kinda like to get the above resolved first
since it feels like it could have an effect on the other patches?


-Doug

Re: [PATCH v2 3/3] dt-bindings: thermal: update sustainable-power with abstract scale

[Lukasz Luba]

On 10/2/20 4:47 PM, Doug Anderson wrote:
> Hi,
> 
> On Fri, Oct 2, 2020 at 8:13 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Hi Doug,
>>
>> On 10/2/20 3:31 PM, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Fri, Oct 2, 2020 at 4:45 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>
>>>> Update the documentation for the binding 'sustainable-power' and allow
>>>> to provide values in an abstract scale. It is required when the cooling
>>>> devices use an abstract scale for their power values.
>>>>
>>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>>> ---
>>>>    .../devicetree/bindings/thermal/thermal-zones.yaml  | 13 +++++++++----
>>>>    1 file changed, 9 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
>>>> index 3ec9cc87ec50..4d8f2e37d1e6 100644
>>>> --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
>>>> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
>>>> @@ -99,10 +99,15 @@ patternProperties:
>>>>          sustainable-power:
>>>>            $ref: /schemas/types.yaml#/definitions/uint32
>>>>            description:
>>>> -          An estimate of the sustainable power (in mW) that this thermal zone
>>>> -          can dissipate at the desired control temperature. For reference, the
>>>> -          sustainable power of a 4-inch phone is typically 2000mW, while on a
>>>> -          10-inch tablet is around 4500mW.
>>>> +          An estimate of the sustainable power (in mW or in an abstract scale)
>>>> +         that this thermal zone can dissipate at the desired control
>>>> +         temperature. For reference, the sustainable power of a 4-inch phone
>>>> +         is typically 2000mW, while on a 10-inch tablet is around 4500mW.
>>>> +
>>>> +         It is possible to express the sustainable power in an abstract
>>>> +         scale. This is the case when the related cooling devices use also
>>>> +         abstract scale to express their power usage. The scale must be
>>>> +         consistent.
>>>
>>> Two thoughts:
>>>
>>> 1. If we're going to allow "sustainable-power" to be in abstract
>>> scale, why not allow "dynamic-power-coefficient" to be in abstract
>>> scale too?  I assume that the whole reason against that originally was
>>> the idea of device tree purity, but if we're allowing the abstract
>>> scale here then there seems no reason not to allow it for
>>> "dynamic-power-coefficient".
>>
>> With this binding it's a bit more tricky.
>> I also have to discuss a few things internally. This requirement of
>> uW/MHz/V^2 makes the code easier also for potential drivers
>> like GPU (which are going to register the devfreq cooling with EM).
>>
>> Let me think about it, but for now I would just update these bits.
>> These are required to proper IPA operation, the dyn.-pow.-coef. is a
>> nice to have and possible next step.
> 
> I guess the problem is that Rajendra is currently planning to remove
> all the "dynamic-power-coefficient" values from device tree right now
> and move them to the source code because the numbers we currently have
> in the device tree _are_ in abstract scale and thus violate the
> bindings.  Moving this to source code won't help us get to more real
> power numbers (since it'll still be abstract scale), it'll just be
> pure churn.  If we're OK with the abstract scale in general then we
> should allow it everywhere and not add churn for no reason.

IIUC he is still going to use the Energy Model, but with different
registration function. We have such a driver: scmi-cpufreq.c, which
uses em_dev_register_perf_domain(). He can still use EM, EAS, IPA
not violating anything.

The real problem that we want to address is with sustainable-power in
IPA. It is used in power budget calculation and if the devices operate
in abstract scale, then there is an issue.
There are two options to get that value:
1. from DT, which can have optimized value, stored by OEM engineer
2. from IPA estimation code, which just calculates it as a sum of
minimum OPP power for each cooling device.

The 2nd option might not be the best for a platform, so vendor/OEM
engineer might want to provide a better value in DT -> 1st option.
This is currently against the binding description and I have to fix it.

> 
> 
>>> 2. Is it worth adding some type of indication of what type of units
>>> "sustainable-power" is represented in?  Maybe even a made up unit so
>>> that you could tell the difference between made up units in the same
>>> system?  I'd envision something like:
>>>
>>> sustainable-power-units = "qualcomm,sc7180-bogoWatts"
>>>
>>> ...and on the dynamic-power-coefficient side, the same:
>>>
>>> dynamic-power-coefficient-units = "qualcomm,sc7180-bogoWatts"
>>>
>>> One could imagine someone even later (after devices are widely
>>> distributed) figuring out translations between these bogoWatts numbers
>>> and real Watts if someone could come up with a case where it matters.
>>
>> To figure this out we don't need a new binding.
>> I think a simple comment in the DT would be enough for this, even e.g.:
>>
>> sustainable-power = <100> /* bogoWatts */
> 
> There are some important differences:
> 
> a) Your comment is gone when the device tree is compiled.  If we
> actually add a string to the device tree then, in theory, we can add
> conversions in code (without touching the device tree) down the road.

We don't need code and binding with a bogoscale. It is up to the
platform integrator to make sure the scale in consistent in all devices.
Comment in DT is good enough.

> 
> b) I believe there can be more than one abstract scale present in a
> single device tree, at least in theory.  Adding a string allows you to
> know if you're comparing apples to apples or apples to organges.

IMHO DT is not the place for such abstractions, but Rob might correct me
here.

Regards,
Lukasz

Re: [PATCH v2 3/3] dt-bindings: thermal: update sustainable-power with abstract scale

[Doug Anderson]

Hi,

On Fri, Oct 2, 2020 at 9:40 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> On 10/2/20 4:47 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Oct 2, 2020 at 8:13 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>
> >> Hi Doug,
> >>
> >> On 10/2/20 3:31 PM, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Fri, Oct 2, 2020 at 4:45 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>>>
> >>>> Update the documentation for the binding 'sustainable-power' and allow
> >>>> to provide values in an abstract scale. It is required when the cooling
> >>>> devices use an abstract scale for their power values.
> >>>>
> >>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> >>>> ---
> >>>>    .../devicetree/bindings/thermal/thermal-zones.yaml  | 13 +++++++++----
> >>>>    1 file changed, 9 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> >>>> index 3ec9cc87ec50..4d8f2e37d1e6 100644
> >>>> --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> >>>> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> >>>> @@ -99,10 +99,15 @@ patternProperties:
> >>>>          sustainable-power:
> >>>>            $ref: /schemas/types.yaml#/definitions/uint32
> >>>>            description:
> >>>> -          An estimate of the sustainable power (in mW) that this thermal zone
> >>>> -          can dissipate at the desired control temperature. For reference, the
> >>>> -          sustainable power of a 4-inch phone is typically 2000mW, while on a
> >>>> -          10-inch tablet is around 4500mW.
> >>>> +          An estimate of the sustainable power (in mW or in an abstract scale)
> >>>> +         that this thermal zone can dissipate at the desired control
> >>>> +         temperature. For reference, the sustainable power of a 4-inch phone
> >>>> +         is typically 2000mW, while on a 10-inch tablet is around 4500mW.
> >>>> +
> >>>> +         It is possible to express the sustainable power in an abstract
> >>>> +         scale. This is the case when the related cooling devices use also
> >>>> +         abstract scale to express their power usage. The scale must be
> >>>> +         consistent.
> >>>
> >>> Two thoughts:
> >>>
> >>> 1. If we're going to allow "sustainable-power" to be in abstract
> >>> scale, why not allow "dynamic-power-coefficient" to be in abstract
> >>> scale too?  I assume that the whole reason against that originally was
> >>> the idea of device tree purity, but if we're allowing the abstract
> >>> scale here then there seems no reason not to allow it for
> >>> "dynamic-power-coefficient".
> >>
> >> With this binding it's a bit more tricky.
> >> I also have to discuss a few things internally. This requirement of
> >> uW/MHz/V^2 makes the code easier also for potential drivers
> >> like GPU (which are going to register the devfreq cooling with EM).
> >>
> >> Let me think about it, but for now I would just update these bits.
> >> These are required to proper IPA operation, the dyn.-pow.-coef. is a
> >> nice to have and possible next step.
> >
> > I guess the problem is that Rajendra is currently planning to remove
> > all the "dynamic-power-coefficient" values from device tree right now
> > and move them to the source code because the numbers we currently have
> > in the device tree _are_ in abstract scale and thus violate the
> > bindings.  Moving this to source code won't help us get to more real
> > power numbers (since it'll still be abstract scale), it'll just be
> > pure churn.  If we're OK with the abstract scale in general then we
> > should allow it everywhere and not add churn for no reason.
>
> IIUC he is still going to use the Energy Model, but with different
> registration function. We have such a driver: scmi-cpufreq.c, which
> uses em_dev_register_perf_domain(). He can still use EM, EAS, IPA
> not violating anything.

Right.  He's going to take the exact same "abstract scale" numbers
that he has today and take them out of device tree and put them in the
cpufreq driver.  Doing so magically makes it so that he's not
violating anything since "abstract scale" is not currently allowed in
device tree but is allowed in the cpufreq driver.  I'm not saying that
he's doing anything wrong, I'm just saying that it's pointless churn.
If we're OK with "abstract scale" in one place in the device tree we
should be OK with it everywhere in the device tree.  Then Rajendra
wouldn't need his patch at all and he could leave his numbers in the
device tree.


> The real problem that we want to address is with sustainable-power in
> IPA. It is used in power budget calculation and if the devices operate
> in abstract scale, then there is an issue.
> There are two options to get that value:
> 1. from DT, which can have optimized value, stored by OEM engineer
> 2. from IPA estimation code, which just calculates it as a sum of
> minimum OPP power for each cooling device.
>
> The 2nd option might not be the best for a platform, so vendor/OEM
> engineer might want to provide a better value in DT -> 1st option.
> This is currently against the binding description and I have to fix it.

Right, things are already broken today because a SoC vendor could
(without violating any rules) provide their SoC core
"dynamic-power-coefficient" in "abstract scale" in code and there
would be no way to for a board to (without violating DT bindings)
specify a "sustainable-power".  ...so, in that sense, your patch does
provide a benefit even if we don't make any changes to the rules for
"sustainable-power".  All I'm saying is that if these new rules for
allowing an abstract scale for "sustainable-power" in the device tree
are OK that it should _also_ be OK to add new rules to allow an
abstract scale for "dynamic-power-coefficient".


> >>> 2. Is it worth adding some type of indication of what type of units
> >>> "sustainable-power" is represented in?  Maybe even a made up unit so
> >>> that you could tell the difference between made up units in the same
> >>> system?  I'd envision something like:
> >>>
> >>> sustainable-power-units = "qualcomm,sc7180-bogoWatts"
> >>>
> >>> ...and on the dynamic-power-coefficient side, the same:
> >>>
> >>> dynamic-power-coefficient-units = "qualcomm,sc7180-bogoWatts"
> >>>
> >>> One could imagine someone even later (after devices are widely
> >>> distributed) figuring out translations between these bogoWatts numbers
> >>> and real Watts if someone could come up with a case where it matters.
> >>
> >> To figure this out we don't need a new binding.
> >> I think a simple comment in the DT would be enough for this, even e.g.:
> >>
> >> sustainable-power = <100> /* bogoWatts */
> >
> > There are some important differences:
> >
> > a) Your comment is gone when the device tree is compiled.  If we
> > actually add a string to the device tree then, in theory, we can add
> > conversions in code (without touching the device tree) down the road.
>
> We don't need code and binding with a bogoscale. It is up to the
> platform integrator to make sure the scale in consistent in all devices.
> Comment in DT is good enough.

One other nice thing about having the units is that the device tree is
supposed to be more of a "pure" thing, less sullied about what's
convenient and more about a real description of a thing.  Presumably
that's why "abstract scale" wasn't allowed originally?  In any case,
giving quantifiable units to the number somehow makes it feel less
made up because it's possible to come up with a way to convert it back
to real units.


> > b) I believe there can be more than one abstract scale present in a
> > single device tree, at least in theory.  Adding a string allows you to
> > know if you're comparing apples to apples or apples to organges.
>
> IMHO DT is not the place for such abstractions, but Rob might correct me
> here.

Yup, seems like we're blocked waiting for Rob to chime in unless
someone else has the authority to make the call about how to deal with
"abstract scale" numbers in the device tree.

-Doug

Re: [PATCH v2 3/3] dt-bindings: thermal: update sustainable-power with abstract scale

[Rob Herring]

On Fri, Oct 2, 2020 at 12:39 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Fri, Oct 2, 2020 at 9:40 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >
> > On 10/2/20 4:47 PM, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Fri, Oct 2, 2020 at 8:13 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> > >>
> > >> Hi Doug,
> > >>
> > >> On 10/2/20 3:31 PM, Doug Anderson wrote:
> > >>> Hi,
> > >>>
> > >>> On Fri, Oct 2, 2020 at 4:45 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> > >>>>
> > >>>> Update the documentation for the binding 'sustainable-power' and allow
> > >>>> to provide values in an abstract scale. It is required when the cooling
> > >>>> devices use an abstract scale for their power values.
> > >>>>
> > >>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> > >>>> ---
> > >>>>    .../devicetree/bindings/thermal/thermal-zones.yaml  | 13 +++++++++----
> > >>>>    1 file changed, 9 insertions(+), 4 deletions(-)
> > >>>>
> > >>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> > >>>> index 3ec9cc87ec50..4d8f2e37d1e6 100644
> > >>>> --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> > >>>> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> > >>>> @@ -99,10 +99,15 @@ patternProperties:
> > >>>>          sustainable-power:
> > >>>>            $ref: /schemas/types.yaml#/definitions/uint32
> > >>>>            description:
> > >>>> -          An estimate of the sustainable power (in mW) that this thermal zone
> > >>>> -          can dissipate at the desired control temperature. For reference, the
> > >>>> -          sustainable power of a 4-inch phone is typically 2000mW, while on a
> > >>>> -          10-inch tablet is around 4500mW.
> > >>>> +          An estimate of the sustainable power (in mW or in an abstract scale)
> > >>>> +         that this thermal zone can dissipate at the desired control
> > >>>> +         temperature. For reference, the sustainable power of a 4-inch phone
> > >>>> +         is typically 2000mW, while on a 10-inch tablet is around 4500mW.
> > >>>> +
> > >>>> +         It is possible to express the sustainable power in an abstract
> > >>>> +         scale. This is the case when the related cooling devices use also
> > >>>> +         abstract scale to express their power usage. The scale must be
> > >>>> +         consistent.
> > >>>
> > >>> Two thoughts:
> > >>>
> > >>> 1. If we're going to allow "sustainable-power" to be in abstract
> > >>> scale, why not allow "dynamic-power-coefficient" to be in abstract
> > >>> scale too?  I assume that the whole reason against that originally was
> > >>> the idea of device tree purity, but if we're allowing the abstract
> > >>> scale here then there seems no reason not to allow it for
> > >>> "dynamic-power-coefficient".
> > >>
> > >> With this binding it's a bit more tricky.
> > >> I also have to discuss a few things internally. This requirement of
> > >> uW/MHz/V^2 makes the code easier also for potential drivers
> > >> like GPU (which are going to register the devfreq cooling with EM).
> > >>
> > >> Let me think about it, but for now I would just update these bits.
> > >> These are required to proper IPA operation, the dyn.-pow.-coef. is a
> > >> nice to have and possible next step.
> > >
> > > I guess the problem is that Rajendra is currently planning to remove
> > > all the "dynamic-power-coefficient" values from device tree right now
> > > and move them to the source code because the numbers we currently have
> > > in the device tree _are_ in abstract scale and thus violate the
> > > bindings.  Moving this to source code won't help us get to more real
> > > power numbers (since it'll still be abstract scale), it'll just be
> > > pure churn.  If we're OK with the abstract scale in general then we
> > > should allow it everywhere and not add churn for no reason.
> >
> > IIUC he is still going to use the Energy Model, but with different
> > registration function. We have such a driver: scmi-cpufreq.c, which
> > uses em_dev_register_perf_domain(). He can still use EM, EAS, IPA
> > not violating anything.
>
> Right.  He's going to take the exact same "abstract scale" numbers
> that he has today and take them out of device tree and put them in the
> cpufreq driver.  Doing so magically makes it so that he's not
> violating anything since "abstract scale" is not currently allowed in
> device tree but is allowed in the cpufreq driver.  I'm not saying that
> he's doing anything wrong, I'm just saying that it's pointless churn.
> If we're OK with "abstract scale" in one place in the device tree we
> should be OK with it everywhere in the device tree.  Then Rajendra
> wouldn't need his patch at all and he could leave his numbers in the
> device tree.
>
>
> > The real problem that we want to address is with sustainable-power in
> > IPA. It is used in power budget calculation and if the devices operate
> > in abstract scale, then there is an issue.
> > There are two options to get that value:
> > 1. from DT, which can have optimized value, stored by OEM engineer
> > 2. from IPA estimation code, which just calculates it as a sum of
> > minimum OPP power for each cooling device.
> >
> > The 2nd option might not be the best for a platform, so vendor/OEM
> > engineer might want to provide a better value in DT -> 1st option.
> > This is currently against the binding description and I have to fix it.
>
> Right, things are already broken today because a SoC vendor could
> (without violating any rules) provide their SoC core
> "dynamic-power-coefficient" in "abstract scale" in code and there
> would be no way to for a board to (without violating DT bindings)
> specify a "sustainable-power".  ...so, in that sense, your patch does
> provide a benefit even if we don't make any changes to the rules for
> "sustainable-power".  All I'm saying is that if these new rules for
> allowing an abstract scale for "sustainable-power" in the device tree
> are OK that it should _also_ be OK to add new rules to allow an
> abstract scale for "dynamic-power-coefficient".

Didn't we beat this one to death with "dynamic-power-coefficient"?
That is the abstract scale because I don't think you can really ever
measure it and because vendors don't want to advertise their absolute
power.

> > >>> 2. Is it worth adding some type of indication of what type of units
> > >>> "sustainable-power" is represented in?  Maybe even a made up unit so
> > >>> that you could tell the difference between made up units in the same
> > >>> system?  I'd envision something like:
> > >>>
> > >>> sustainable-power-units = "qualcomm,sc7180-bogoWatts"
> > >>>
> > >>> ...and on the dynamic-power-coefficient side, the same:
> > >>>
> > >>> dynamic-power-coefficient-units = "qualcomm,sc7180-bogoWatts"
> > >>>
> > >>> One could imagine someone even later (after devices are widely
> > >>> distributed) figuring out translations between these bogoWatts numbers
> > >>> and real Watts if someone could come up with a case where it matters.
> > >>
> > >> To figure this out we don't need a new binding.
> > >> I think a simple comment in the DT would be enough for this, even e.g.:
> > >>
> > >> sustainable-power = <100> /* bogoWatts */
> > >
> > > There are some important differences:
> > >
> > > a) Your comment is gone when the device tree is compiled.  If we
> > > actually add a string to the device tree then, in theory, we can add
> > > conversions in code (without touching the device tree) down the road.
> >
> > We don't need code and binding with a bogoscale. It is up to the
> > platform integrator to make sure the scale in consistent in all devices.
> > Comment in DT is good enough.
>
> One other nice thing about having the units is that the device tree is
> supposed to be more of a "pure" thing, less sullied about what's
> convenient and more about a real description of a thing.  Presumably
> that's why "abstract scale" wasn't allowed originally?  In any case,
> giving quantifiable units to the number somehow makes it feel less
> made up because it's possible to come up with a way to convert it back
> to real units.
>
>
> > > b) I believe there can be more than one abstract scale present in a
> > > single device tree, at least in theory.  Adding a string allows you to
> > > know if you're comparing apples to apples or apples to organges.
> >
> > IMHO DT is not the place for such abstractions, but Rob might correct me
> > here.
>
> Yup, seems like we're blocked waiting for Rob to chime in unless
> someone else has the authority to make the call about how to deal with
> "abstract scale" numbers in the device tree.

I don't really know nor completely follow the issues. I just get all
these PM related bindings piece by piece with everyone solving their
own single issue. It's death by 1000 cuts. So my default position is
NAK. All the missing pieces and deficiencies can build up until
there's a coherent picture (maybe?).

Rob

Re: [PATCH v2 3/3] dt-bindings: thermal: update sustainable-power with abstract scale

[Doug Anderson]

Hi,

On Tue, Oct 6, 2020 at 3:24 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Fri, Oct 2, 2020 at 12:39 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Fri, Oct 2, 2020 at 9:40 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> > >
> > > On 10/2/20 4:47 PM, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Fri, Oct 2, 2020 at 8:13 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> > > >>
> > > >> Hi Doug,
> > > >>
> > > >> On 10/2/20 3:31 PM, Doug Anderson wrote:
> > > >>> Hi,
> > > >>>
> > > >>> On Fri, Oct 2, 2020 at 4:45 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> > > >>>>
> > > >>>> Update the documentation for the binding 'sustainable-power' and allow
> > > >>>> to provide values in an abstract scale. It is required when the cooling
> > > >>>> devices use an abstract scale for their power values.
> > > >>>>
> > > >>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> > > >>>> ---
> > > >>>>    .../devicetree/bindings/thermal/thermal-zones.yaml  | 13 +++++++++----
> > > >>>>    1 file changed, 9 insertions(+), 4 deletions(-)
> > > >>>>
> > > >>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> > > >>>> index 3ec9cc87ec50..4d8f2e37d1e6 100644
> > > >>>> --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> > > >>>> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> > > >>>> @@ -99,10 +99,15 @@ patternProperties:
> > > >>>>          sustainable-power:
> > > >>>>            $ref: /schemas/types.yaml#/definitions/uint32
> > > >>>>            description:
> > > >>>> -          An estimate of the sustainable power (in mW) that this thermal zone
> > > >>>> -          can dissipate at the desired control temperature. For reference, the
> > > >>>> -          sustainable power of a 4-inch phone is typically 2000mW, while on a
> > > >>>> -          10-inch tablet is around 4500mW.
> > > >>>> +          An estimate of the sustainable power (in mW or in an abstract scale)
> > > >>>> +         that this thermal zone can dissipate at the desired control
> > > >>>> +         temperature. For reference, the sustainable power of a 4-inch phone
> > > >>>> +         is typically 2000mW, while on a 10-inch tablet is around 4500mW.
> > > >>>> +
> > > >>>> +         It is possible to express the sustainable power in an abstract
> > > >>>> +         scale. This is the case when the related cooling devices use also
> > > >>>> +         abstract scale to express their power usage. The scale must be
> > > >>>> +         consistent.
> > > >>>
> > > >>> Two thoughts:
> > > >>>
> > > >>> 1. If we're going to allow "sustainable-power" to be in abstract
> > > >>> scale, why not allow "dynamic-power-coefficient" to be in abstract
> > > >>> scale too?  I assume that the whole reason against that originally was
> > > >>> the idea of device tree purity, but if we're allowing the abstract
> > > >>> scale here then there seems no reason not to allow it for
> > > >>> "dynamic-power-coefficient".
> > > >>
> > > >> With this binding it's a bit more tricky.
> > > >> I also have to discuss a few things internally. This requirement of
> > > >> uW/MHz/V^2 makes the code easier also for potential drivers
> > > >> like GPU (which are going to register the devfreq cooling with EM).
> > > >>
> > > >> Let me think about it, but for now I would just update these bits.
> > > >> These are required to proper IPA operation, the dyn.-pow.-coef. is a
> > > >> nice to have and possible next step.
> > > >
> > > > I guess the problem is that Rajendra is currently planning to remove
> > > > all the "dynamic-power-coefficient" values from device tree right now
> > > > and move them to the source code because the numbers we currently have
> > > > in the device tree _are_ in abstract scale and thus violate the
> > > > bindings.  Moving this to source code won't help us get to more real
> > > > power numbers (since it'll still be abstract scale), it'll just be
> > > > pure churn.  If we're OK with the abstract scale in general then we
> > > > should allow it everywhere and not add churn for no reason.
> > >
> > > IIUC he is still going to use the Energy Model, but with different
> > > registration function. We have such a driver: scmi-cpufreq.c, which
> > > uses em_dev_register_perf_domain(). He can still use EM, EAS, IPA
> > > not violating anything.
> >
> > Right.  He's going to take the exact same "abstract scale" numbers
> > that he has today and take them out of device tree and put them in the
> > cpufreq driver.  Doing so magically makes it so that he's not
> > violating anything since "abstract scale" is not currently allowed in
> > device tree but is allowed in the cpufreq driver.  I'm not saying that
> > he's doing anything wrong, I'm just saying that it's pointless churn.
> > If we're OK with "abstract scale" in one place in the device tree we
> > should be OK with it everywhere in the device tree.  Then Rajendra
> > wouldn't need his patch at all and he could leave his numbers in the
> > device tree.
> >
> >
> > > The real problem that we want to address is with sustainable-power in
> > > IPA. It is used in power budget calculation and if the devices operate
> > > in abstract scale, then there is an issue.
> > > There are two options to get that value:
> > > 1. from DT, which can have optimized value, stored by OEM engineer
> > > 2. from IPA estimation code, which just calculates it as a sum of
> > > minimum OPP power for each cooling device.
> > >
> > > The 2nd option might not be the best for a platform, so vendor/OEM
> > > engineer might want to provide a better value in DT -> 1st option.
> > > This is currently against the binding description and I have to fix it.
> >
> > Right, things are already broken today because a SoC vendor could
> > (without violating any rules) provide their SoC core
> > "dynamic-power-coefficient" in "abstract scale" in code and there
> > would be no way to for a board to (without violating DT bindings)
> > specify a "sustainable-power".  ...so, in that sense, your patch does
> > provide a benefit even if we don't make any changes to the rules for
> > "sustainable-power".  All I'm saying is that if these new rules for
> > allowing an abstract scale for "sustainable-power" in the device tree
> > are OK that it should _also_ be OK to add new rules to allow an
> > abstract scale for "dynamic-power-coefficient".
>
> Didn't we beat this one to death with "dynamic-power-coefficient"?

We did?  Where / when?  I'm not sure I was involved, but right now
both "sustainable-power" and "dynamic-power-coefficient" are still
defined in the device tree to be in real units, not abstract scale.
Are you saying that we beat it to death and decided that it needed to
be in real units, or we beat it to death and decided that abstract
scale was OK and we just didn't put it in the bindings?


> That is the abstract scale because I don't think you can really ever
> measure it

That's debatable.  it's not very hard to get reasonable measurements.
Matthias provided a recipe earlier in the thread.  See commit
ac60c5e33df4 ("ARM: dts: rockchip: Add dynamic-power-coefficient for
rk3288").  In that case he used a machine that could easily measure
power on the CPU rail, but if you simply keep all other rails in the
system constant (and/or run a long enough test), you can easily
accomplish this by just querying the smart battery in systems.


> and because vendors don't want to advertise their absolute
> power.

That is certainly true, though after a device has shipped it's not
that hard to measure.


> > > >>> 2. Is it worth adding some type of indication of what type of units
> > > >>> "sustainable-power" is represented in?  Maybe even a made up unit so
> > > >>> that you could tell the difference between made up units in the same
> > > >>> system?  I'd envision something like:
> > > >>>
> > > >>> sustainable-power-units = "qualcomm,sc7180-bogoWatts"
> > > >>>
> > > >>> ...and on the dynamic-power-coefficient side, the same:
> > > >>>
> > > >>> dynamic-power-coefficient-units = "qualcomm,sc7180-bogoWatts"
> > > >>>
> > > >>> One could imagine someone even later (after devices are widely
> > > >>> distributed) figuring out translations between these bogoWatts numbers
> > > >>> and real Watts if someone could come up with a case where it matters.
> > > >>
> > > >> To figure this out we don't need a new binding.
> > > >> I think a simple comment in the DT would be enough for this, even e.g.:
> > > >>
> > > >> sustainable-power = <100> /* bogoWatts */
> > > >
> > > > There are some important differences:
> > > >
> > > > a) Your comment is gone when the device tree is compiled.  If we
> > > > actually add a string to the device tree then, in theory, we can add
> > > > conversions in code (without touching the device tree) down the road.
> > >
> > > We don't need code and binding with a bogoscale. It is up to the
> > > platform integrator to make sure the scale in consistent in all devices.
> > > Comment in DT is good enough.
> >
> > One other nice thing about having the units is that the device tree is
> > supposed to be more of a "pure" thing, less sullied about what's
> > convenient and more about a real description of a thing.  Presumably
> > that's why "abstract scale" wasn't allowed originally?  In any case,
> > giving quantifiable units to the number somehow makes it feel less
> > made up because it's possible to come up with a way to convert it back
> > to real units.
> >
> >
> > > > b) I believe there can be more than one abstract scale present in a
> > > > single device tree, at least in theory.  Adding a string allows you to
> > > > know if you're comparing apples to apples or apples to organges.
> > >
> > > IMHO DT is not the place for such abstractions, but Rob might correct me
> > > here.
> >
> > Yup, seems like we're blocked waiting for Rob to chime in unless
> > someone else has the authority to make the call about how to deal with
> > "abstract scale" numbers in the device tree.
>
> I don't really know nor completely follow the issues. I just get all
> these PM related bindings piece by piece with everyone solving their
> own single issue. It's death by 1000 cuts. So my default position is
> NAK. All the missing pieces and deficiencies can build up until
> there's a coherent picture (maybe?).

I'm totally confused.  NAK on what?  NAK on Lukasz's patch?  ...or
Lukasz's patch is totally fine but NAK on also allowing abstract scale
for 'dynamic-power-coefficient".  Or NAK on adding units?  NAK on
something else?

-Doug

Re: [PATCH v2 3/3] dt-bindings: thermal: update sustainable-power with abstract scale

[Rob Herring]

On Tue, Oct 6, 2020 at 8:17 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Oct 6, 2020 at 3:24 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Fri, Oct 2, 2020 at 12:39 PM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Fri, Oct 2, 2020 at 9:40 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> > > >
> > > > On 10/2/20 4:47 PM, Doug Anderson wrote:
> > > > > Hi,
> > > > >
> > > > > On Fri, Oct 2, 2020 at 8:13 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> > > > >>
> > > > >> Hi Doug,
> > > > >>
> > > > >> On 10/2/20 3:31 PM, Doug Anderson wrote:
> > > > >>> Hi,
> > > > >>>
> > > > >>> On Fri, Oct 2, 2020 at 4:45 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> > > > >>>>
> > > > >>>> Update the documentation for the binding 'sustainable-power' and allow
> > > > >>>> to provide values in an abstract scale. It is required when the cooling
> > > > >>>> devices use an abstract scale for their power values.
> > > > >>>>
> > > > >>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> > > > >>>> ---
> > > > >>>>    .../devicetree/bindings/thermal/thermal-zones.yaml  | 13 +++++++++----
> > > > >>>>    1 file changed, 9 insertions(+), 4 deletions(-)
> > > > >>>>
> > > > >>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> > > > >>>> index 3ec9cc87ec50..4d8f2e37d1e6 100644
> > > > >>>> --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> > > > >>>> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> > > > >>>> @@ -99,10 +99,15 @@ patternProperties:
> > > > >>>>          sustainable-power:
> > > > >>>>            $ref: /schemas/types.yaml#/definitions/uint32
> > > > >>>>            description:
> > > > >>>> -          An estimate of the sustainable power (in mW) that this thermal zone
> > > > >>>> -          can dissipate at the desired control temperature. For reference, the
> > > > >>>> -          sustainable power of a 4-inch phone is typically 2000mW, while on a
> > > > >>>> -          10-inch tablet is around 4500mW.
> > > > >>>> +          An estimate of the sustainable power (in mW or in an abstract scale)
> > > > >>>> +         that this thermal zone can dissipate at the desired control
> > > > >>>> +         temperature. For reference, the sustainable power of a 4-inch phone
> > > > >>>> +         is typically 2000mW, while on a 10-inch tablet is around 4500mW.
> > > > >>>> +
> > > > >>>> +         It is possible to express the sustainable power in an abstract
> > > > >>>> +         scale. This is the case when the related cooling devices use also
> > > > >>>> +         abstract scale to express their power usage. The scale must be
> > > > >>>> +         consistent.
> > > > >>>
> > > > >>> Two thoughts:
> > > > >>>
> > > > >>> 1. If we're going to allow "sustainable-power" to be in abstract
> > > > >>> scale, why not allow "dynamic-power-coefficient" to be in abstract
> > > > >>> scale too?  I assume that the whole reason against that originally was
> > > > >>> the idea of device tree purity, but if we're allowing the abstract
> > > > >>> scale here then there seems no reason not to allow it for
> > > > >>> "dynamic-power-coefficient".
> > > > >>
> > > > >> With this binding it's a bit more tricky.
> > > > >> I also have to discuss a few things internally. This requirement of
> > > > >> uW/MHz/V^2 makes the code easier also for potential drivers
> > > > >> like GPU (which are going to register the devfreq cooling with EM).
> > > > >>
> > > > >> Let me think about it, but for now I would just update these bits.
> > > > >> These are required to proper IPA operation, the dyn.-pow.-coef. is a
> > > > >> nice to have and possible next step.
> > > > >
> > > > > I guess the problem is that Rajendra is currently planning to remove
> > > > > all the "dynamic-power-coefficient" values from device tree right now
> > > > > and move them to the source code because the numbers we currently have
> > > > > in the device tree _are_ in abstract scale and thus violate the
> > > > > bindings.  Moving this to source code won't help us get to more real
> > > > > power numbers (since it'll still be abstract scale), it'll just be
> > > > > pure churn.  If we're OK with the abstract scale in general then we
> > > > > should allow it everywhere and not add churn for no reason.
> > > >
> > > > IIUC he is still going to use the Energy Model, but with different
> > > > registration function. We have such a driver: scmi-cpufreq.c, which
> > > > uses em_dev_register_perf_domain(). He can still use EM, EAS, IPA
> > > > not violating anything.
> > >
> > > Right.  He's going to take the exact same "abstract scale" numbers
> > > that he has today and take them out of device tree and put them in the
> > > cpufreq driver.  Doing so magically makes it so that he's not
> > > violating anything since "abstract scale" is not currently allowed in
> > > device tree but is allowed in the cpufreq driver.  I'm not saying that
> > > he's doing anything wrong, I'm just saying that it's pointless churn.
> > > If we're OK with "abstract scale" in one place in the device tree we
> > > should be OK with it everywhere in the device tree.  Then Rajendra
> > > wouldn't need his patch at all and he could leave his numbers in the
> > > device tree.
> > >
> > >
> > > > The real problem that we want to address is with sustainable-power in
> > > > IPA. It is used in power budget calculation and if the devices operate
> > > > in abstract scale, then there is an issue.
> > > > There are two options to get that value:
> > > > 1. from DT, which can have optimized value, stored by OEM engineer
> > > > 2. from IPA estimation code, which just calculates it as a sum of
> > > > minimum OPP power for each cooling device.
> > > >
> > > > The 2nd option might not be the best for a platform, so vendor/OEM
> > > > engineer might want to provide a better value in DT -> 1st option.
> > > > This is currently against the binding description and I have to fix it.
> > >
> > > Right, things are already broken today because a SoC vendor could
> > > (without violating any rules) provide their SoC core
> > > "dynamic-power-coefficient" in "abstract scale" in code and there
> > > would be no way to for a board to (without violating DT bindings)
> > > specify a "sustainable-power".  ...so, in that sense, your patch does
> > > provide a benefit even if we don't make any changes to the rules for
> > > "sustainable-power".  All I'm saying is that if these new rules for
> > > allowing an abstract scale for "sustainable-power" in the device tree
> > > are OK that it should _also_ be OK to add new rules to allow an
> > > abstract scale for "dynamic-power-coefficient".
> >
> > Didn't we beat this one to death with "dynamic-power-coefficient"?
>
> We did?  Where / when?

https://lore.kernel.org/r/1448288921-30307-1-git-send-email-juri.lelli@arm.com/

> I'm not sure I was involved, but right now
> both "sustainable-power" and "dynamic-power-coefficient" are still
> defined in the device tree to be in real units, not abstract scale.
> Are you saying that we beat it to death and decided that it needed to
> be in real units, or we beat it to death and decided that abstract
> scale was OK and we just didn't put it in the bindings?

The former.

> > That is the abstract scale because I don't think you can really ever
> > measure it
>
> That's debatable.  it's not very hard to get reasonable measurements.
> Matthias provided a recipe earlier in the thread.  See commit
> ac60c5e33df4 ("ARM: dts: rockchip: Add dynamic-power-coefficient for
> rk3288").  In that case he used a machine that could easily measure
> power on the CPU rail, but if you simply keep all other rails in the
> system constant (and/or run a long enough test), you can easily
> accomplish this by just querying the smart battery in systems.

Okay, yes, you can measure and then calculate something. But the value
is only meaningful within that platform. There's no standardized test
to run. What the power rails are could be different (e.g. CPU RAMs on
a separate rail and shared).

> > and because vendors don't want to advertise their absolute
> > power.
>
> That is certainly true, though after a device has shipped it's not
> that hard to measure.

Can you tell me how to measure the CPU rail on my Pixel3?

> > > > >>> 2. Is it worth adding some type of indication of what type of units
> > > > >>> "sustainable-power" is represented in?  Maybe even a made up unit so
> > > > >>> that you could tell the difference between made up units in the same
> > > > >>> system?  I'd envision something like:
> > > > >>>
> > > > >>> sustainable-power-units = "qualcomm,sc7180-bogoWatts"
> > > > >>>
> > > > >>> ...and on the dynamic-power-coefficient side, the same:
> > > > >>>
> > > > >>> dynamic-power-coefficient-units = "qualcomm,sc7180-bogoWatts"
> > > > >>>
> > > > >>> One could imagine someone even later (after devices are widely
> > > > >>> distributed) figuring out translations between these bogoWatts numbers
> > > > >>> and real Watts if someone could come up with a case where it matters.
> > > > >>
> > > > >> To figure this out we don't need a new binding.
> > > > >> I think a simple comment in the DT would be enough for this, even e.g.:
> > > > >>
> > > > >> sustainable-power = <100> /* bogoWatts */
> > > > >
> > > > > There are some important differences:
> > > > >
> > > > > a) Your comment is gone when the device tree is compiled.  If we
> > > > > actually add a string to the device tree then, in theory, we can add
> > > > > conversions in code (without touching the device tree) down the road.
> > > >
> > > > We don't need code and binding with a bogoscale. It is up to the
> > > > platform integrator to make sure the scale in consistent in all devices.
> > > > Comment in DT is good enough.
> > >
> > > One other nice thing about having the units is that the device tree is
> > > supposed to be more of a "pure" thing, less sullied about what's
> > > convenient and more about a real description of a thing.  Presumably
> > > that's why "abstract scale" wasn't allowed originally?  In any case,
> > > giving quantifiable units to the number somehow makes it feel less
> > > made up because it's possible to come up with a way to convert it back
> > > to real units.
> > >
> > >
> > > > > b) I believe there can be more than one abstract scale present in a
> > > > > single device tree, at least in theory.  Adding a string allows you to
> > > > > know if you're comparing apples to apples or apples to organges.
> > > >
> > > > IMHO DT is not the place for such abstractions, but Rob might correct me
> > > > here.
> > >
> > > Yup, seems like we're blocked waiting for Rob to chime in unless
> > > someone else has the authority to make the call about how to deal with
> > > "abstract scale" numbers in the device tree.
> >
> > I don't really know nor completely follow the issues. I just get all
> > these PM related bindings piece by piece with everyone solving their
> > own single issue. It's death by 1000 cuts. So my default position is
> > NAK. All the missing pieces and deficiencies can build up until
> > there's a coherent picture (maybe?).
>
> I'm totally confused.  NAK on what?  NAK on Lukasz's patch?  ...or
> Lukasz's patch is totally fine but NAK on also allowing abstract scale
> for 'dynamic-power-coefficient".  Or NAK on adding units?  NAK on
> something else?

That's just my rant on PM bindings in general.
'cpu-performance-dependencies' is another one currently.

Rob

Re: [PATCH v2 3/3] dt-bindings: thermal: update sustainable-power with abstract scale

[Doug Anderson]

Hi,

On Wed, Oct 7, 2020 at 6:26 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Tue, Oct 6, 2020 at 8:17 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Tue, Oct 6, 2020 at 3:24 PM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Fri, Oct 2, 2020 at 12:39 PM Doug Anderson <dianders@chromium.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Fri, Oct 2, 2020 at 9:40 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> > > > >
> > > > > On 10/2/20 4:47 PM, Doug Anderson wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Fri, Oct 2, 2020 at 8:13 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> > > > > >>
> > > > > >> Hi Doug,
> > > > > >>
> > > > > >> On 10/2/20 3:31 PM, Doug Anderson wrote:
> > > > > >>> Hi,
> > > > > >>>
> > > > > >>> On Fri, Oct 2, 2020 at 4:45 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> > > > > >>>>
> > > > > >>>> Update the documentation for the binding 'sustainable-power' and allow
> > > > > >>>> to provide values in an abstract scale. It is required when the cooling
> > > > > >>>> devices use an abstract scale for their power values.
> > > > > >>>>
> > > > > >>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> > > > > >>>> ---
> > > > > >>>>    .../devicetree/bindings/thermal/thermal-zones.yaml  | 13 +++++++++----
> > > > > >>>>    1 file changed, 9 insertions(+), 4 deletions(-)
> > > > > >>>>
> > > > > >>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> > > > > >>>> index 3ec9cc87ec50..4d8f2e37d1e6 100644
> > > > > >>>> --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> > > > > >>>> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> > > > > >>>> @@ -99,10 +99,15 @@ patternProperties:
> > > > > >>>>          sustainable-power:
> > > > > >>>>            $ref: /schemas/types.yaml#/definitions/uint32
> > > > > >>>>            description:
> > > > > >>>> -          An estimate of the sustainable power (in mW) that this thermal zone
> > > > > >>>> -          can dissipate at the desired control temperature. For reference, the
> > > > > >>>> -          sustainable power of a 4-inch phone is typically 2000mW, while on a
> > > > > >>>> -          10-inch tablet is around 4500mW.
> > > > > >>>> +          An estimate of the sustainable power (in mW or in an abstract scale)
> > > > > >>>> +         that this thermal zone can dissipate at the desired control
> > > > > >>>> +         temperature. For reference, the sustainable power of a 4-inch phone
> > > > > >>>> +         is typically 2000mW, while on a 10-inch tablet is around 4500mW.
> > > > > >>>> +
> > > > > >>>> +         It is possible to express the sustainable power in an abstract
> > > > > >>>> +         scale. This is the case when the related cooling devices use also
> > > > > >>>> +         abstract scale to express their power usage. The scale must be
> > > > > >>>> +         consistent.
> > > > > >>>
> > > > > >>> Two thoughts:
> > > > > >>>
> > > > > >>> 1. If we're going to allow "sustainable-power" to be in abstract
> > > > > >>> scale, why not allow "dynamic-power-coefficient" to be in abstract
> > > > > >>> scale too?  I assume that the whole reason against that originally was
> > > > > >>> the idea of device tree purity, but if we're allowing the abstract
> > > > > >>> scale here then there seems no reason not to allow it for
> > > > > >>> "dynamic-power-coefficient".
> > > > > >>
> > > > > >> With this binding it's a bit more tricky.
> > > > > >> I also have to discuss a few things internally. This requirement of
> > > > > >> uW/MHz/V^2 makes the code easier also for potential drivers
> > > > > >> like GPU (which are going to register the devfreq cooling with EM).
> > > > > >>
> > > > > >> Let me think about it, but for now I would just update these bits.
> > > > > >> These are required to proper IPA operation, the dyn.-pow.-coef. is a
> > > > > >> nice to have and possible next step.
> > > > > >
> > > > > > I guess the problem is that Rajendra is currently planning to remove
> > > > > > all the "dynamic-power-coefficient" values from device tree right now
> > > > > > and move them to the source code because the numbers we currently have
> > > > > > in the device tree _are_ in abstract scale and thus violate the
> > > > > > bindings.  Moving this to source code won't help us get to more real
> > > > > > power numbers (since it'll still be abstract scale), it'll just be
> > > > > > pure churn.  If we're OK with the abstract scale in general then we
> > > > > > should allow it everywhere and not add churn for no reason.
> > > > >
> > > > > IIUC he is still going to use the Energy Model, but with different
> > > > > registration function. We have such a driver: scmi-cpufreq.c, which
> > > > > uses em_dev_register_perf_domain(). He can still use EM, EAS, IPA
> > > > > not violating anything.
> > > >
> > > > Right.  He's going to take the exact same "abstract scale" numbers
> > > > that he has today and take them out of device tree and put them in the
> > > > cpufreq driver.  Doing so magically makes it so that he's not
> > > > violating anything since "abstract scale" is not currently allowed in
> > > > device tree but is allowed in the cpufreq driver.  I'm not saying that
> > > > he's doing anything wrong, I'm just saying that it's pointless churn.
> > > > If we're OK with "abstract scale" in one place in the device tree we
> > > > should be OK with it everywhere in the device tree.  Then Rajendra
> > > > wouldn't need his patch at all and he could leave his numbers in the
> > > > device tree.
> > > >
> > > >
> > > > > The real problem that we want to address is with sustainable-power in
> > > > > IPA. It is used in power budget calculation and if the devices operate
> > > > > in abstract scale, then there is an issue.
> > > > > There are two options to get that value:
> > > > > 1. from DT, which can have optimized value, stored by OEM engineer
> > > > > 2. from IPA estimation code, which just calculates it as a sum of
> > > > > minimum OPP power for each cooling device.
> > > > >
> > > > > The 2nd option might not be the best for a platform, so vendor/OEM
> > > > > engineer might want to provide a better value in DT -> 1st option.
> > > > > This is currently against the binding description and I have to fix it.
> > > >
> > > > Right, things are already broken today because a SoC vendor could
> > > > (without violating any rules) provide their SoC core
> > > > "dynamic-power-coefficient" in "abstract scale" in code and there
> > > > would be no way to for a board to (without violating DT bindings)
> > > > specify a "sustainable-power".  ...so, in that sense, your patch does
> > > > provide a benefit even if we don't make any changes to the rules for
> > > > "sustainable-power".  All I'm saying is that if these new rules for
> > > > allowing an abstract scale for "sustainable-power" in the device tree
> > > > are OK that it should _also_ be OK to add new rules to allow an
> > > > abstract scale for "dynamic-power-coefficient".
> > >
> > > Didn't we beat this one to death with "dynamic-power-coefficient"?
> >
> > We did?  Where / when?
>
> https://lore.kernel.org/r/1448288921-30307-1-git-send-email-juri.lelli@arm.com/

Thanks for the reference.


> > I'm not sure I was involved, but right now
> > both "sustainable-power" and "dynamic-power-coefficient" are still
> > defined in the device tree to be in real units, not abstract scale.
> > Are you saying that we beat it to death and decided that it needed to
> > be in real units, or we beat it to death and decided that abstract
> > scale was OK and we just didn't put it in the bindings?
>
> The former.

OK.  So I suppose this is a NAK to Lukasz's patch.  It also means that:

* The power numbers that landed in the sc7180 devicetree violate
what's documented in the bindings.

* While Rajendra can fix this by moving the numbers out of devicetree
and into code, it doesn't really help us because there will be no way
to allow boards to specify their "sustainable-power" in code.

* Anyone who is using the "abstract scale" provided by firmware or by
code is in the same boat.  There's no way for a board to specify
"sustainable-power" that will match this "abstract scale" without
violating the devicetree bindings.

Obviously the easiest way to fix this is to just move everyone off of
"abstract scale".

If someone else has other bright ideas I'm all ears.


> > > That is the abstract scale because I don't think you can really ever
> > > measure it
> >
> > That's debatable.  it's not very hard to get reasonable measurements.
> > Matthias provided a recipe earlier in the thread.  See commit
> > ac60c5e33df4 ("ARM: dts: rockchip: Add dynamic-power-coefficient for
> > rk3288").  In that case he used a machine that could easily measure
> > power on the CPU rail, but if you simply keep all other rails in the
> > system constant (and/or run a long enough test), you can easily
> > accomplish this by just querying the smart battery in systems.
>
> Okay, yes, you can measure and then calculate something. But the value
> is only meaningful within that platform. There's no standardized test
> to run. What the power rails are could be different (e.g. CPU RAMs on
> a separate rail and shared).

I think in this case the dynamic-power-coefficient is supposed to
describe the CPU cores only.  Presumably for a given SoC the cores
will behave (to a rough approximation) the same from board to board?


> > > and because vendors don't want to advertise their absolute
> > > power.
> >
> > That is certainly true, though after a device has shipped it's not
> > that hard to measure.
>
> Can you tell me how to measure the CPU rail on my Pixel3?

I've written a script to do this and I'll share it soon.
Unfortunately the power consumption of little cores when running at
very low frequencies is pretty miniscule and sbs_battery doesn't have
that fine of a granularity.  I tried running where I spent 10 minutes
at each frequency and it was still slightly too noisy.  I'll run it
overnight where I spend 30 minutes at each frequency and see if my
numbers are accurate / consistent.  Since I'm running on hardware that
hasn't been released to the general public I won't publish my numbers,
but I'm hoping my script will be good enough that you should be able
to run it on your hardware and get real / consistent numbers.


> > > > > >>> 2. Is it worth adding some type of indication of what type of units
> > > > > >>> "sustainable-power" is represented in?  Maybe even a made up unit so
> > > > > >>> that you could tell the difference between made up units in the same
> > > > > >>> system?  I'd envision something like:
> > > > > >>>
> > > > > >>> sustainable-power-units = "qualcomm,sc7180-bogoWatts"
> > > > > >>>
> > > > > >>> ...and on the dynamic-power-coefficient side, the same:
> > > > > >>>
> > > > > >>> dynamic-power-coefficient-units = "qualcomm,sc7180-bogoWatts"
> > > > > >>>
> > > > > >>> One could imagine someone even later (after devices are widely
> > > > > >>> distributed) figuring out translations between these bogoWatts numbers
> > > > > >>> and real Watts if someone could come up with a case where it matters.
> > > > > >>
> > > > > >> To figure this out we don't need a new binding.
> > > > > >> I think a simple comment in the DT would be enough for this, even e.g.:
> > > > > >>
> > > > > >> sustainable-power = <100> /* bogoWatts */
> > > > > >
> > > > > > There are some important differences:
> > > > > >
> > > > > > a) Your comment is gone when the device tree is compiled.  If we
> > > > > > actually add a string to the device tree then, in theory, we can add
> > > > > > conversions in code (without touching the device tree) down the road.
> > > > >
> > > > > We don't need code and binding with a bogoscale. It is up to the
> > > > > platform integrator to make sure the scale in consistent in all devices.
> > > > > Comment in DT is good enough.
> > > >
> > > > One other nice thing about having the units is that the device tree is
> > > > supposed to be more of a "pure" thing, less sullied about what's
> > > > convenient and more about a real description of a thing.  Presumably
> > > > that's why "abstract scale" wasn't allowed originally?  In any case,
> > > > giving quantifiable units to the number somehow makes it feel less
> > > > made up because it's possible to come up with a way to convert it back
> > > > to real units.
> > > >
> > > >
> > > > > > b) I believe there can be more than one abstract scale present in a
> > > > > > single device tree, at least in theory.  Adding a string allows you to
> > > > > > know if you're comparing apples to apples or apples to organges.
> > > > >
> > > > > IMHO DT is not the place for such abstractions, but Rob might correct me
> > > > > here.
> > > >
> > > > Yup, seems like we're blocked waiting for Rob to chime in unless
> > > > someone else has the authority to make the call about how to deal with
> > > > "abstract scale" numbers in the device tree.
> > >
> > > I don't really know nor completely follow the issues. I just get all
> > > these PM related bindings piece by piece with everyone solving their
> > > own single issue. It's death by 1000 cuts. So my default position is
> > > NAK. All the missing pieces and deficiencies can build up until
> > > there's a coherent picture (maybe?).
> >
> > I'm totally confused.  NAK on what?  NAK on Lukasz's patch?  ...or
> > Lukasz's patch is totally fine but NAK on also allowing abstract scale
> > for 'dynamic-power-coefficient".  Or NAK on adding units?  NAK on
> > something else?
>
> That's just my rant on PM bindings in general.
> 'cpu-performance-dependencies' is another one currently.
>
> Rob

Re: [PATCH v2 3/3] dt-bindings: thermal: update sustainable-power with abstract scale

[Lukasz Luba]

On 10/7/20 10:40 PM, Doug Anderson wrote:
> Hi,
> 
> On Wed, Oct 7, 2020 at 6:26 AM Rob Herring <robh+dt@kernel.org> wrote:
>>
>> On Tue, Oct 6, 2020 at 8:17 PM Doug Anderson <dianders@chromium.org> wrote:
>>>
>>> Hi,
>>>
>>> On Tue, Oct 6, 2020 at 3:24 PM Rob Herring <robh+dt@kernel.org> wrote:
>>>>
>>>> On Fri, Oct 2, 2020 at 12:39 PM Doug Anderson <dianders@chromium.org> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On Fri, Oct 2, 2020 at 9:40 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>>>
>>>>>> On 10/2/20 4:47 PM, Doug Anderson wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Fri, Oct 2, 2020 at 8:13 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>>>>>
>>>>>>>> Hi Doug,
>>>>>>>>
>>>>>>>> On 10/2/20 3:31 PM, Doug Anderson wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On Fri, Oct 2, 2020 at 4:45 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>>>>>>>
>>>>>>>>>> Update the documentation for the binding 'sustainable-power' and allow
>>>>>>>>>> to provide values in an abstract scale. It is required when the cooling
>>>>>>>>>> devices use an abstract scale for their power values.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>>>>>>>>> ---
>>>>>>>>>>     .../devicetree/bindings/thermal/thermal-zones.yaml  | 13 +++++++++----
>>>>>>>>>>     1 file changed, 9 insertions(+), 4 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
>>>>>>>>>> index 3ec9cc87ec50..4d8f2e37d1e6 100644
>>>>>>>>>> --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
>>>>>>>>>> @@ -99,10 +99,15 @@ patternProperties:
>>>>>>>>>>           sustainable-power:
>>>>>>>>>>             $ref: /schemas/types.yaml#/definitions/uint32
>>>>>>>>>>             description:
>>>>>>>>>> -          An estimate of the sustainable power (in mW) that this thermal zone
>>>>>>>>>> -          can dissipate at the desired control temperature. For reference, the
>>>>>>>>>> -          sustainable power of a 4-inch phone is typically 2000mW, while on a
>>>>>>>>>> -          10-inch tablet is around 4500mW.
>>>>>>>>>> +          An estimate of the sustainable power (in mW or in an abstract scale)
>>>>>>>>>> +         that this thermal zone can dissipate at the desired control
>>>>>>>>>> +         temperature. For reference, the sustainable power of a 4-inch phone
>>>>>>>>>> +         is typically 2000mW, while on a 10-inch tablet is around 4500mW.
>>>>>>>>>> +
>>>>>>>>>> +         It is possible to express the sustainable power in an abstract
>>>>>>>>>> +         scale. This is the case when the related cooling devices use also
>>>>>>>>>> +         abstract scale to express their power usage. The scale must be
>>>>>>>>>> +         consistent.
>>>>>>>>>
>>>>>>>>> Two thoughts:
>>>>>>>>>
>>>>>>>>> 1. If we're going to allow "sustainable-power" to be in abstract
>>>>>>>>> scale, why not allow "dynamic-power-coefficient" to be in abstract
>>>>>>>>> scale too?  I assume that the whole reason against that originally was
>>>>>>>>> the idea of device tree purity, but if we're allowing the abstract
>>>>>>>>> scale here then there seems no reason not to allow it for
>>>>>>>>> "dynamic-power-coefficient".
>>>>>>>>
>>>>>>>> With this binding it's a bit more tricky.
>>>>>>>> I also have to discuss a few things internally. This requirement of
>>>>>>>> uW/MHz/V^2 makes the code easier also for potential drivers
>>>>>>>> like GPU (which are going to register the devfreq cooling with EM).
>>>>>>>>
>>>>>>>> Let me think about it, but for now I would just update these bits.
>>>>>>>> These are required to proper IPA operation, the dyn.-pow.-coef. is a
>>>>>>>> nice to have and possible next step.
>>>>>>>
>>>>>>> I guess the problem is that Rajendra is currently planning to remove
>>>>>>> all the "dynamic-power-coefficient" values from device tree right now
>>>>>>> and move them to the source code because the numbers we currently have
>>>>>>> in the device tree _are_ in abstract scale and thus violate the
>>>>>>> bindings.  Moving this to source code won't help us get to more real
>>>>>>> power numbers (since it'll still be abstract scale), it'll just be
>>>>>>> pure churn.  If we're OK with the abstract scale in general then we
>>>>>>> should allow it everywhere and not add churn for no reason.
>>>>>>
>>>>>> IIUC he is still going to use the Energy Model, but with different
>>>>>> registration function. We have such a driver: scmi-cpufreq.c, which
>>>>>> uses em_dev_register_perf_domain(). He can still use EM, EAS, IPA
>>>>>> not violating anything.
>>>>>
>>>>> Right.  He's going to take the exact same "abstract scale" numbers
>>>>> that he has today and take them out of device tree and put them in the
>>>>> cpufreq driver.  Doing so magically makes it so that he's not
>>>>> violating anything since "abstract scale" is not currently allowed in
>>>>> device tree but is allowed in the cpufreq driver.  I'm not saying that
>>>>> he's doing anything wrong, I'm just saying that it's pointless churn.
>>>>> If we're OK with "abstract scale" in one place in the device tree we
>>>>> should be OK with it everywhere in the device tree.  Then Rajendra
>>>>> wouldn't need his patch at all and he could leave his numbers in the
>>>>> device tree.
>>>>>
>>>>>
>>>>>> The real problem that we want to address is with sustainable-power in
>>>>>> IPA. It is used in power budget calculation and if the devices operate
>>>>>> in abstract scale, then there is an issue.
>>>>>> There are two options to get that value:
>>>>>> 1. from DT, which can have optimized value, stored by OEM engineer
>>>>>> 2. from IPA estimation code, which just calculates it as a sum of
>>>>>> minimum OPP power for each cooling device.
>>>>>>
>>>>>> The 2nd option might not be the best for a platform, so vendor/OEM
>>>>>> engineer might want to provide a better value in DT -> 1st option.
>>>>>> This is currently against the binding description and I have to fix it.
>>>>>
>>>>> Right, things are already broken today because a SoC vendor could
>>>>> (without violating any rules) provide their SoC core
>>>>> "dynamic-power-coefficient" in "abstract scale" in code and there
>>>>> would be no way to for a board to (without violating DT bindings)
>>>>> specify a "sustainable-power".  ...so, in that sense, your patch does
>>>>> provide a benefit even if we don't make any changes to the rules for
>>>>> "sustainable-power".  All I'm saying is that if these new rules for
>>>>> allowing an abstract scale for "sustainable-power" in the device tree
>>>>> are OK that it should _also_ be OK to add new rules to allow an
>>>>> abstract scale for "dynamic-power-coefficient".
>>>>
>>>> Didn't we beat this one to death with "dynamic-power-coefficient"?
>>>
>>> We did?  Where / when?
>>
>> https://lore.kernel.org/r/1448288921-30307-1-git-send-email-juri.lelli@arm.com/
> 
> Thanks for the reference.
> 
> 
>>> I'm not sure I was involved, but right now
>>> both "sustainable-power" and "dynamic-power-coefficient" are still
>>> defined in the device tree to be in real units, not abstract scale.
>>> Are you saying that we beat it to death and decided that it needed to
>>> be in real units, or we beat it to death and decided that abstract
>>> scale was OK and we just didn't put it in the bindings?
>>
>> The former.
> 
> OK.  So I suppose this is a NAK to Lukasz's patch.  It also means that:

I also consider this as a NAK for this patch 3/3, but other two can go.
It will be also NAK for dt-binding change adding 'abstract scale'
description to "dynamic-power-coefficient", so I won't post it.

> 
> * The power numbers that landed in the sc7180 devicetree violate
> what's documented in the bindings.
> 
> * While Rajendra can fix this by moving the numbers out of devicetree
> and into code, it doesn't really help us because there will be no way
> to allow boards to specify their "sustainable-power" in code.
> 
> * Anyone who is using the "abstract scale" provided by firmware or by
> code is in the same boat.  There's no way for a board to specify
> "sustainable-power" that will match this "abstract scale" without
> violating the devicetree bindings.
> 
> Obviously the easiest way to fix this is to just move everyone off of
> "abstract scale".
> 
> If someone else has other bright ideas I'm all ears.

To summarize, we allow abstract scale to be in EM, EAS and IPA.
For EM/EAS it is possible have this via
em_dev_register_perf_domain()
IPA would also get these devices with abstract scale, but the DT
"sustainable-power" would not be aligned, so might be not set in DT.
For that, what would help:
- IPA internal code for sustainable power estimation
- thermal sysfs interface for IPA 'sustainable_power'

In this case I think patch 1/3 and 2/3 can go into upstream.
This one (patch 3/3) can be dropped.

Thank you Rob and Doug for this discussion.

Regards,
Lukasz

Re: [PATCH v2 3/3] dt-bindings: thermal: update sustainable-power with abstract scale

[Doug Anderson]

Hi,

On Wed, Oct 7, 2020 at 2:40 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Oct 7, 2020 at 6:26 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Tue, Oct 6, 2020 at 8:17 PM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Oct 6, 2020 at 3:24 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > On Fri, Oct 2, 2020 at 12:39 PM Doug Anderson <dianders@chromium.org> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Fri, Oct 2, 2020 at 9:40 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> > > > > >
> > > > > > On 10/2/20 4:47 PM, Doug Anderson wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Fri, Oct 2, 2020 at 8:13 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> > > > > > >>
> > > > > > >> Hi Doug,
> > > > > > >>
> > > > > > >> On 10/2/20 3:31 PM, Doug Anderson wrote:
> > > > > > >>> Hi,
> > > > > > >>>
> > > > > > >>> On Fri, Oct 2, 2020 at 4:45 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
> > > > > > >>>>
> > > > > > >>>> Update the documentation for the binding 'sustainable-power' and allow
> > > > > > >>>> to provide values in an abstract scale. It is required when the cooling
> > > > > > >>>> devices use an abstract scale for their power values.
> > > > > > >>>>
> > > > > > >>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> > > > > > >>>> ---
> > > > > > >>>>    .../devicetree/bindings/thermal/thermal-zones.yaml  | 13 +++++++++----
> > > > > > >>>>    1 file changed, 9 insertions(+), 4 deletions(-)
> > > > > > >>>>
> > > > > > >>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> > > > > > >>>> index 3ec9cc87ec50..4d8f2e37d1e6 100644
> > > > > > >>>> --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> > > > > > >>>> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> > > > > > >>>> @@ -99,10 +99,15 @@ patternProperties:
> > > > > > >>>>          sustainable-power:
> > > > > > >>>>            $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > >>>>            description:
> > > > > > >>>> -          An estimate of the sustainable power (in mW) that this thermal zone
> > > > > > >>>> -          can dissipate at the desired control temperature. For reference, the
> > > > > > >>>> -          sustainable power of a 4-inch phone is typically 2000mW, while on a
> > > > > > >>>> -          10-inch tablet is around 4500mW.
> > > > > > >>>> +          An estimate of the sustainable power (in mW or in an abstract scale)
> > > > > > >>>> +         that this thermal zone can dissipate at the desired control
> > > > > > >>>> +         temperature. For reference, the sustainable power of a 4-inch phone
> > > > > > >>>> +         is typically 2000mW, while on a 10-inch tablet is around 4500mW.
> > > > > > >>>> +
> > > > > > >>>> +         It is possible to express the sustainable power in an abstract
> > > > > > >>>> +         scale. This is the case when the related cooling devices use also
> > > > > > >>>> +         abstract scale to express their power usage. The scale must be
> > > > > > >>>> +         consistent.
> > > > > > >>>
> > > > > > >>> Two thoughts:
> > > > > > >>>
> > > > > > >>> 1. If we're going to allow "sustainable-power" to be in abstract
> > > > > > >>> scale, why not allow "dynamic-power-coefficient" to be in abstract
> > > > > > >>> scale too?  I assume that the whole reason against that originally was
> > > > > > >>> the idea of device tree purity, but if we're allowing the abstract
> > > > > > >>> scale here then there seems no reason not to allow it for
> > > > > > >>> "dynamic-power-coefficient".
> > > > > > >>
> > > > > > >> With this binding it's a bit more tricky.
> > > > > > >> I also have to discuss a few things internally. This requirement of
> > > > > > >> uW/MHz/V^2 makes the code easier also for potential drivers
> > > > > > >> like GPU (which are going to register the devfreq cooling with EM).
> > > > > > >>
> > > > > > >> Let me think about it, but for now I would just update these bits.
> > > > > > >> These are required to proper IPA operation, the dyn.-pow.-coef. is a
> > > > > > >> nice to have and possible next step.
> > > > > > >
> > > > > > > I guess the problem is that Rajendra is currently planning to remove
> > > > > > > all the "dynamic-power-coefficient" values from device tree right now
> > > > > > > and move them to the source code because the numbers we currently have
> > > > > > > in the device tree _are_ in abstract scale and thus violate the
> > > > > > > bindings.  Moving this to source code won't help us get to more real
> > > > > > > power numbers (since it'll still be abstract scale), it'll just be
> > > > > > > pure churn.  If we're OK with the abstract scale in general then we
> > > > > > > should allow it everywhere and not add churn for no reason.
> > > > > >
> > > > > > IIUC he is still going to use the Energy Model, but with different
> > > > > > registration function. We have such a driver: scmi-cpufreq.c, which
> > > > > > uses em_dev_register_perf_domain(). He can still use EM, EAS, IPA
> > > > > > not violating anything.
> > > > >
> > > > > Right.  He's going to take the exact same "abstract scale" numbers
> > > > > that he has today and take them out of device tree and put them in the
> > > > > cpufreq driver.  Doing so magically makes it so that he's not
> > > > > violating anything since "abstract scale" is not currently allowed in
> > > > > device tree but is allowed in the cpufreq driver.  I'm not saying that
> > > > > he's doing anything wrong, I'm just saying that it's pointless churn.
> > > > > If we're OK with "abstract scale" in one place in the device tree we
> > > > > should be OK with it everywhere in the device tree.  Then Rajendra
> > > > > wouldn't need his patch at all and he could leave his numbers in the
> > > > > device tree.
> > > > >
> > > > >
> > > > > > The real problem that we want to address is with sustainable-power in
> > > > > > IPA. It is used in power budget calculation and if the devices operate
> > > > > > in abstract scale, then there is an issue.
> > > > > > There are two options to get that value:
> > > > > > 1. from DT, which can have optimized value, stored by OEM engineer
> > > > > > 2. from IPA estimation code, which just calculates it as a sum of
> > > > > > minimum OPP power for each cooling device.
> > > > > >
> > > > > > The 2nd option might not be the best for a platform, so vendor/OEM
> > > > > > engineer might want to provide a better value in DT -> 1st option.
> > > > > > This is currently against the binding description and I have to fix it.
> > > > >
> > > > > Right, things are already broken today because a SoC vendor could
> > > > > (without violating any rules) provide their SoC core
> > > > > "dynamic-power-coefficient" in "abstract scale" in code and there
> > > > > would be no way to for a board to (without violating DT bindings)
> > > > > specify a "sustainable-power".  ...so, in that sense, your patch does
> > > > > provide a benefit even if we don't make any changes to the rules for
> > > > > "sustainable-power".  All I'm saying is that if these new rules for
> > > > > allowing an abstract scale for "sustainable-power" in the device tree
> > > > > are OK that it should _also_ be OK to add new rules to allow an
> > > > > abstract scale for "dynamic-power-coefficient".
> > > >
> > > > Didn't we beat this one to death with "dynamic-power-coefficient"?
> > >
> > > We did?  Where / when?
> >
> > https://lore.kernel.org/r/1448288921-30307-1-git-send-email-juri.lelli@arm.com/
>
> Thanks for the reference.
>
>
> > > I'm not sure I was involved, but right now
> > > both "sustainable-power" and "dynamic-power-coefficient" are still
> > > defined in the device tree to be in real units, not abstract scale.
> > > Are you saying that we beat it to death and decided that it needed to
> > > be in real units, or we beat it to death and decided that abstract
> > > scale was OK and we just didn't put it in the bindings?
> >
> > The former.
>
> OK.  So I suppose this is a NAK to Lukasz's patch.  It also means that:
>
> * The power numbers that landed in the sc7180 devicetree violate
> what's documented in the bindings.
>
> * While Rajendra can fix this by moving the numbers out of devicetree
> and into code, it doesn't really help us because there will be no way
> to allow boards to specify their "sustainable-power" in code.
>
> * Anyone who is using the "abstract scale" provided by firmware or by
> code is in the same boat.  There's no way for a board to specify
> "sustainable-power" that will match this "abstract scale" without
> violating the devicetree bindings.
>
> Obviously the easiest way to fix this is to just move everyone off of
> "abstract scale".
>
> If someone else has other bright ideas I'm all ears.
>
>
> > > > That is the abstract scale because I don't think you can really ever
> > > > measure it
> > >
> > > That's debatable.  it's not very hard to get reasonable measurements.
> > > Matthias provided a recipe earlier in the thread.  See commit
> > > ac60c5e33df4 ("ARM: dts: rockchip: Add dynamic-power-coefficient for
> > > rk3288").  In that case he used a machine that could easily measure
> > > power on the CPU rail, but if you simply keep all other rails in the
> > > system constant (and/or run a long enough test), you can easily
> > > accomplish this by just querying the smart battery in systems.
> >
> > Okay, yes, you can measure and then calculate something. But the value
> > is only meaningful within that platform. There's no standardized test
> > to run. What the power rails are could be different (e.g. CPU RAMs on
> > a separate rail and shared).
>
> I think in this case the dynamic-power-coefficient is supposed to
> describe the CPU cores only.  Presumably for a given SoC the cores
> will behave (to a rough approximation) the same from board to board?
>
>
> > > > and because vendors don't want to advertise their absolute
> > > > power.
> > >
> > > That is certainly true, though after a device has shipped it's not
> > > that hard to measure.
> >
> > Can you tell me how to measure the CPU rail on my Pixel3?
>
> I've written a script to do this and I'll share it soon.
> Unfortunately the power consumption of little cores when running at
> very low frequencies is pretty miniscule and sbs_battery doesn't have
> that fine of a granularity.  I tried running where I spent 10 minutes
> at each frequency and it was still slightly too noisy.  I'll run it
> overnight where I spend 30 minutes at each frequency and see if my
> numbers are accurate / consistent.  Since I'm running on hardware that
> hasn't been released to the general public I won't publish my numbers,
> but I'm hoping my script will be good enough that you should be able
> to run it on your hardware and get real / consistent numbers.

OK, I finished writing my python script for this.  It's not massively
pretty, but I at least tried to comment all of it.  This should make
it easy to get "real" power numbers on any hardware that has a smart
battery without too much trouble.

I ran this on my board and got reasonable-looking results.  My smart
battery wasn't quite as smart as I hoped (reported wattage for the
same thing increased a little), but given that the numbers made sense
and were reasonable I'm willing to believe they're at least in the
ballpark.  To test, I charged my battery up to ~95% and then ran:

powernumbers.py 6 120; \
powernumbers.py 0 120; \
powernumbers.py 0 120; \
powernumbers.py 6 120

Running twice and 2 orders helped me account for the smart battery
seeming to report larger numbers as it drained.  If you wanted even
more accurate numbers you could run it until the battery drained and
take the average all the way down.  ...or find a better smart
battery...  ;-)

I'd be super curious if anyone else wants to run this on "released" hardware...

My hope in writing this is to prove that these numbers are _not_
secret nor hard to obtain on any hardware that's in the general
public's hands.

One note: in measuring on my board, I found that the ratio between the
dynamic-power-coefficient for the big and little cores didn't match
what Qualcomm provided.  I'll kick off a task to ask about this.
Specifically their numbers (in the public DTS) show bigs have a
coefficient of 405 and littles of 100.  If I understand correctly this
means that Qualcomm is claiming that bigs draw roughly 4x more power
if run at the same voltage / frequency.  My numbers showed closer to
3x (if littles were 100 bigs would be 313)

In any case, without further ado, here's my script.  Hopefully gmail
doesn't mangle it too badly (I tried to keep away from long lines and
it just uses spaces for indent):

===

#!python3

# NOTES:
# - You have to muck with the config (in code) below a bit.
#   It shouldn't be too hard, I hope.
# - You have to manually get the frequency / voltage mapping
#   and enter it below.  This is _not_ secret if you have
#   access to a device because the kernel needs to know it.
#   See below for at least one way to obtain.
# - This assumes you've got a battery that can measure current
#   that is reasonably accurate.  The one on the Chromebook
#   I tested seemed OK.
# - This doesn't cleanup after itself (leaves CPUs offline
#   at whatever freq it last tested).  Reboot after using.
# - You need to have "dry2" in your path (dhrystone).  This is
#   the canonical program used to benchmark.  There might
#   be some variance between the 32-bit version and 64-bit
#   version, so the 64-bit version is preferred if you want
#   to compare your numbers to others.
#   - I git cloned drystone and ran 2.2
#     https://github.com/Keith-S-Thompson/dhrystone.git
#   - I think I had to make a small change to includes
#     to fix compiler complaints?
#   - I compiled in Chrome OS chroot with:
#     LFLAGS="-static" \
#     CFLAGS="-O3" \
#     CC=aarch64-cros-linux-gnu-gcc sh dry.c
# - This is whipped together code.  It won't win beauty
#   contests.  Sorry.

# Before running, quiesce power and disconnect charger.
# You don't have to try too hard.  On Chrome OS, I did this:
#
# echo 0 > /sys/class/backlight/backlight/brightness
# stop ui
# stop powerd
# stop anomaly-detector
# stop metrics_daemon
# stop auditd
# killall timberslide

import glob
import os
import pprint
import subprocess
import sys
import time

### Config starts here ###

# Put a CPU number you want to measure here, or pass it in
# as the first argument.
if len(sys.argv) >= 2:
  measure_cpu = int(sys.argv[1])
else:
  measure_cpu = 0

# We'll spend this much time at each frequency.
# 2nd argument
# It seems that (at least my battery) slowly reports
# more power as it drains, so don't make this too long
# or it might skew your results.
if len(sys.argv) >= 3:
  min_time_per_freq = int(sys.argv[2])
else:
  min_time_per_freq = 120

# Path to your smart battery on your system.
sbs_path = "/sys/class/power_supply/sbs-12-000b"

# You need to fill in this voltage table for your system.
# Maybe there's a better way to get this, but on Qualcomm CPU
# frequencies you can simply instrument qcom_cpufreq_hw_read_lut()
# to print out a frequency and voltage.
#
# This maps Hz to uW
if measure_cpu == 0:
  voltage_table = {
    # FILL THIS IN.  See comment above.
    freq: uW,
  }
elif measure_cpu == 6:
  voltage_table = {
    # FILL THIS IN.  See comment above.
    freq: uW,
  }

### Program starts here ###

cpufreq_dir = "/sys/devices/system/cpu/cpu%d/cpufreq/" % measure_cpu
freqs = sorted(voltage_table.keys())

# Kick off a background process to log current/voltage every second
def start_power_measure():
  p = subprocess.Popen("""
    rm -f /tmp/times.txt;
    for i in $(seq %d); do
      echo $(cat %s/current_now %s/voltage_now) >> /tmp/times.txt;
      sleep 1;
    done"""  % (min_time_per_freq, sbs_path, sbs_path), shell=True)
  return p

# Look at the times the background process logged and return mW.
def stop_power_measure(p):
  # Finish the program.  It shouldn't have output anything.
  p.communicate()

  # Read and covert to watts.
  lines = open("/tmp/times.txt", "r").readlines()
  pairs = [line.split() for line in lines]
  watts = [(-float(uA) / 1000000) * (float(uV) / 1000000)
           for (uA, uV) in pairs]
  watts = list(sorted(watts))

  # Take only the middle 3rd (throw out the outliers)
  num = len(watts)
  watts = watts[num // 3:-num // 3]

  # Return mW
  return sum(watts) / len(watts) * 1000.

# Make sure only the CPU being measured is online
open("/sys/devices/system/cpu/cpu%d/online" %
     measure_cpu, "w").write("1")
for dir in glob.glob("/sys/devices/system/cpu/cpu[0-9]*"):
  if dir == "/sys/devices/system/cpu/cpu%d" % measure_cpu:
    continue
  open("%s/online" % dir, "w").write("0")

results = {}

# We need userspace governor so we can pick the frequency.
open(cpufreq_dir + "scaling_governor", "w").write("userspace")

# We'll measure quiescent power first at the lowest CPU freq
print("Reading quiescent power")
open(cpufreq_dir + "scaling_setspeed", "w").write(str(freqs[0]))
p = start_power_measure()
time.sleep(min_time_per_freq)
baseline_mW = stop_power_measure(p)
print("Baseline power is %.2f mW" % baseline_mW)

# Now we go through each
for freq in freqs:
  open(cpufreq_dir + "scaling_setspeed", "w").write(str(freq))

  print("========== Measuring power at %d Hz ==========" % freq)

  p = start_power_measure()
  start_time = time.time()

  # Keep running the benchmark to keep the CPU busy while we're
  # measuring power.  Arbitrarily picked a loops count so that
  # it was busy right away at all tested freqs.
  while time.time() - start_time < min_time_per_freq:
    subprocess.check_output(["dry2 100000000; true"],
                            stderr=subprocess.STDOUT, shell=True)

  mW = stop_power_measure(p)

  results[freq] = mW - baseline_mW
  print("Took %d seconds, used %.2f mW" %
        (time.time() - start_time, results[freq]))

# We now have total system power for each frequency.
pprint.pprint(repr(results))

# Use the magic math, as documented in:
# ac60c5e33df4 ARM: dts: rockchip: Add dynamic-power-coefficient...

P1 = results[freqs[0]]
V1 = voltage_table[freqs[0]] / 1000000.
f1 = freqs[0] / 1000000.

all_Cx = []
for freq in freqs[1:]:
  Px = results[freq]
  Vx = voltage_table[freq] / 1000000.
  fx = freq / 1000000.
  Cx = (Px - P1) / (Vx * Vx * fx - V1 * V1 * f1)
  all_Cx.append(Cx)
  print("%d kHz, %d mV, %d mW, %d Cx" %
        (freq / 1000, Vx * 1000, Px, Cx))

print("Your dynamic-power-coefficient for cpu %d: %d" %
      (measure_cpu, round(sum(all_Cx) / len(all_Cx))))

Re: [PATCH v2 3/3] dt-bindings: thermal: update sustainable-power with abstract scale

[Lukasz Luba]

Hi Doug,

On 10/2/20 4:47 PM, Doug Anderson wrote:
> Hi,
> 
> On Fri, Oct 2, 2020 at 8:13 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Hi Doug,
>>
>> On 10/2/20 3:31 PM, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Fri, Oct 2, 2020 at 4:45 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>
>>>> Update the documentation for the binding 'sustainable-power' and allow
>>>> to provide values in an abstract scale. It is required when the cooling
>>>> devices use an abstract scale for their power values.
>>>>
>>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>>> ---
>>>>    .../devicetree/bindings/thermal/thermal-zones.yaml  | 13 +++++++++----
>>>>    1 file changed, 9 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
>>>> index 3ec9cc87ec50..4d8f2e37d1e6 100644
>>>> --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
>>>> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
>>>> @@ -99,10 +99,15 @@ patternProperties:
>>>>          sustainable-power:
>>>>            $ref: /schemas/types.yaml#/definitions/uint32
>>>>            description:
>>>> -          An estimate of the sustainable power (in mW) that this thermal zone
>>>> -          can dissipate at the desired control temperature. For reference, the
>>>> -          sustainable power of a 4-inch phone is typically 2000mW, while on a
>>>> -          10-inch tablet is around 4500mW.
>>>> +          An estimate of the sustainable power (in mW or in an abstract scale)
>>>> +         that this thermal zone can dissipate at the desired control
>>>> +         temperature. For reference, the sustainable power of a 4-inch phone
>>>> +         is typically 2000mW, while on a 10-inch tablet is around 4500mW.
>>>> +
>>>> +         It is possible to express the sustainable power in an abstract
>>>> +         scale. This is the case when the related cooling devices use also
>>>> +         abstract scale to express their power usage. The scale must be
>>>> +         consistent.
>>>
>>> Two thoughts:
>>>
>>> 1. If we're going to allow "sustainable-power" to be in abstract
>>> scale, why not allow "dynamic-power-coefficient" to be in abstract
>>> scale too?  I assume that the whole reason against that originally was
>>> the idea of device tree purity, but if we're allowing the abstract
>>> scale here then there seems no reason not to allow it for
>>> "dynamic-power-coefficient".
>>
>> With this binding it's a bit more tricky.
>> I also have to discuss a few things internally. This requirement of
>> uW/MHz/V^2 makes the code easier also for potential drivers
>> like GPU (which are going to register the devfreq cooling with EM).
>>
>> Let me think about it, but for now I would just update these bits.
>> These are required to proper IPA operation, the dyn.-pow.-coef. is a
>> nice to have and possible next step.
> 
> I guess the problem is that Rajendra is currently planning to remove
> all the "dynamic-power-coefficient" values from device tree right now
> and move them to the source code because the numbers we currently have
> in the device tree _are_ in abstract scale and thus violate the
> bindings.  Moving this to source code won't help us get to more real
> power numbers (since it'll still be abstract scale), it'll just be
> pure churn.  If we're OK with the abstract scale in general then we
> should allow it everywhere and not add churn for no reason.
> 
> 

I just want to notify you that I had internal conversation about this
'dynamic-power-coefficient' binding and abstract scale. We would
change it as well, similarly to 'sustainable-power'. It must pass
internal review and I will send the v3 of this series.

Regards,
Lukasz

Re: [PATCH v2 3/3] dt-bindings: thermal: update sustainable-power with abstract scale

[Rob Herring]

On Fri, 02 Oct 2020 12:44:26 +0100, Lukasz Luba wrote:
> Update the documentation for the binding 'sustainable-power' and allow
> to provide values in an abstract scale. It is required when the cooling
> devices use an abstract scale for their power values.
> 
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  .../devicetree/bindings/thermal/thermal-zones.yaml  | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 


My bot found errors running 'make dt_binding_check' on your patch:

Traceback (most recent call last):
  File "/usr/local/bin/dt-extract-example", line 45, in <module>
    binding = yaml.load(open(args.yamlfile, encoding='utf-8').read())
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 343, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 111, in get_single_data
    node = self.composer.get_single_node()
  File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node
  File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 731, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event
ruamel.yaml.scanner.ScannerError: while scanning a plain scalar
  in "<unicode string>", line 102, column 11
found a tab character that violates indentation
  in "<unicode string>", line 103, column 1
make[1]: *** [Documentation/devicetree/bindings/Makefile:18: Documentation/devicetree/bindings/thermal/thermal-zones.example.dts] Error 1
make[1]: *** Deleting file 'Documentation/devicetree/bindings/thermal/thermal-zones.example.dts'
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/thermal/thermal-zones.yaml:  while scanning a plain scalar
  in "<unicode string>", line 102, column 11
found a tab character that violates indentation
  in "<unicode string>", line 103, column 1
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/thermal/thermal-zones.yaml: ignoring, error parsing file
warning: no schema found in file: ./Documentation/devicetree/bindings/thermal/thermal-zones.yaml
make: *** [Makefile:1366: dt_binding_check] Error 2


See https://patchwork.ozlabs.org/patch/1375670

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.

Re: [PATCH v2 3/3] dt-bindings: thermal: update sustainable-power with abstract scale

[Lukasz Luba]

Hi Rob,

On 10/5/20 2:58 PM, Rob Herring wrote:
> On Fri, 02 Oct 2020 12:44:26 +0100, Lukasz Luba wrote:
>> Update the documentation for the binding 'sustainable-power' and allow
>> to provide values in an abstract scale. It is required when the cooling
>> devices use an abstract scale for their power values.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   .../devicetree/bindings/thermal/thermal-zones.yaml  | 13 +++++++++----
>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>
> 
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> Traceback (most recent call last):
>    File "/usr/local/bin/dt-extract-example", line 45, in <module>
>      binding = yaml.load(open(args.yamlfile, encoding='utf-8').read())
>    File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 343, in load
>      return constructor.get_single_data()
>    File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 111, in get_single_data
>      node = self.composer.get_single_node()
>    File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node
>    File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document
>    File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
>    File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node
>    File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
>    File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node
>    File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
>    File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node
>    File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
>    File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node
>    File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
>    File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node
>    File "_ruamel_yaml.pyx", line 731, in _ruamel_yaml.CParser._compose_node
>    File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event
> ruamel.yaml.scanner.ScannerError: while scanning a plain scalar
>    in "<unicode string>", line 102, column 11
> found a tab character that violates indentation
>    in "<unicode string>", line 103, column 1
> make[1]: *** [Documentation/devicetree/bindings/Makefile:18: Documentation/devicetree/bindings/thermal/thermal-zones.example.dts] Error 1
> make[1]: *** Deleting file 'Documentation/devicetree/bindings/thermal/thermal-zones.example.dts'
> make[1]: *** Waiting for unfinished jobs....
> ./Documentation/devicetree/bindings/thermal/thermal-zones.yaml:  while scanning a plain scalar
>    in "<unicode string>", line 102, column 11
> found a tab character that violates indentation
>    in "<unicode string>", line 103, column 1
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/thermal/thermal-zones.yaml: ignoring, error parsing file
> warning: no schema found in file: ./Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> make: *** [Makefile:1366: dt_binding_check] Error 2
> 
> 
> See https://patchwork.ozlabs.org/patch/1375670
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure dt-schema is up to date:
> 
> pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade
> 
> Please check and re-submit.
> 

My apologies, I have put tabs instead of spaces in there.
I have run this command and now it passed.

I will resend the patch 3/3.

Regards,
Lukasz

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

[Lukasz Luba]

Hi Rafael,

On 10/2/20 12:44 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. There is also a need to update the DT binding for the
> 'sustainable-power' and allow it to have abstract scale as well.
> 
> Changes:
> v2:
> - updated sustainable power section in IPA documentation
> - updated DT binding for the 'sustainable-power'
> 
> The v1 of the patch set and related discussion can be found in [1].
> 
> Regards,
> Lukasz Luba
> 
> [1] https://lore.kernel.org/linux-doc/20200929121610.16060-1-lukasz.luba@arm.com/
> 
> Lukasz Luba (3):
>    docs: Clarify abstract scale usage for power values in Energy Model
>    PM / EM: update the comments related to power scale
>    dt-bindings: thermal: update sustainable-power with abstract scale
> 
>   .../devicetree/bindings/thermal/thermal-zones.yaml  | 13 +++++++++----
>   .../driver-api/thermal/power_allocator.rst          | 13 ++++++++++++-
>   Documentation/power/energy-model.rst                | 13 +++++++++++++
>   Documentation/scheduler/sched-energy.rst            |  5 +++++
>   include/linux/energy_model.h                        | 11 +++++------
>   kernel/power/energy_model.c                         |  2 +-
>   6 files changed, 45 insertions(+), 12 deletions(-)
> 

Could you take patch 1/3 and patch 2/3 via your PM tree,
please? I will be very grateful.

These patches just update the documentation and comments regarding
an issue that we can have: bogoWatts in the Energy Model (and we
already have). One of the drawbacks is that we cannot derive real energy
from these numbers. Will see how this would evolve.

The patch 3/3 with dt-binding is not going to fly upstream [1].
I hope developers will put a real number in DT so we could
have real milliWatts from that source (wishful thinking).
Doug even made a script, which might be helpful for that [2].

Regards,
Lukasz

[1] 
https://lore.kernel.org/linux-pm/45fae8cd-0635-41dc-c744-3c9833bf6492@arm.com/
[2] 
https://lore.kernel.org/linux-pm/CAD=FV=U1FP0e3_AVHpauUUZtD-5X3XCwh5aT9fH_8S_FFML2Uw@mail.gmail.com/

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

[Daniel Lezcano]

Hi Lukasz,

On 09/10/2020 11:16, Lukasz Luba wrote:
> Hi Rafael,
> 
> On 10/2/20 12:44 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. There is also a need to update the DT binding for
>> the
>> 'sustainable-power' and allow it to have abstract scale as well.
>>
>> Changes:
>> v2:
>> - updated sustainable power section in IPA documentation
>> - updated DT binding for the 'sustainable-power'
>>
>> The v1 of the patch set and related discussion can be found in [1].
>>
>> Regards,
>> Lukasz Luba
>>
>> [1]
>> https://lore.kernel.org/linux-doc/20200929121610.16060-1-lukasz.luba@arm.com/
>>
>>
>> Lukasz Luba (3):
>>    docs: Clarify abstract scale usage for power values in Energy Model
>>    PM / EM: update the comments related to power scale
>>    dt-bindings: thermal: update sustainable-power with abstract scale
>>
>>   .../devicetree/bindings/thermal/thermal-zones.yaml  | 13 +++++++++----
>>   .../driver-api/thermal/power_allocator.rst          | 13 ++++++++++++-
>>   Documentation/power/energy-model.rst                | 13 +++++++++++++
>>   Documentation/scheduler/sched-energy.rst            |  5 +++++
>>   include/linux/energy_model.h                        | 11 +++++------
>>   kernel/power/energy_model.c                         |  2 +-
>>   6 files changed, 45 insertions(+), 12 deletions(-)
>>
> 
> Could you take patch 1/3 and patch 2/3 via your PM tree,
> please? I will be very grateful.
> 
> These patches just update the documentation and comments regarding
> an issue that we can have: bogoWatts in the Energy Model (and we
> already have). One of the drawbacks is that we cannot derive real energy
> from these numbers. Will see how this would evolve.

The purpose of the energy model is to provide these power numbers.

If the SoC vendors do not want to share those numbers, then better to
not use the energy model at all.

If they want to use the EAS and the IPA at all costs without sharing the
power numbers, then it is up to them to take responsibility of providing
consistent numbers, not the community to document how to hack the energy
model.

And that is even more true as mentioned by Doug: the power numbers are
not impossible to measure.

Documenting the scale values give the opportunity to the SoC vendor to
never share the power numbers, and even worst, that implies all the
existing and future frameworks based on the energy model (and its
evolution) *must* comply with these dummy values. That is the promise of
a real pain.

IMO, we must keep a strong constraint on the power values for the energy
model.

However, nothing prevents to write a recipe on a website explaining how
to use the energy model without the power numbers with a big warning
that could not work in the future if the energy model evolves or it
could be incompatible with the IPA.

I suggest to solve the energy model main issue: the SoC vendor do not
want to share the power numbers. Why not give the opportunity to load a
firmware where the power numbers will be ? The firmware could be in a
vendor partition for example.


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

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

[Lukasz Luba]

Hi Daniel,

On 10/14/20 9:22 AM, Daniel Lezcano wrote:
> 
> Hi Lukasz,
> 
> On 09/10/2020 11:16, Lukasz Luba wrote:
>> Hi Rafael,
>>
>> On 10/2/20 12:44 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. There is also a need to update the DT binding for
>>> the
>>> 'sustainable-power' and allow it to have abstract scale as well.
>>>
>>> Changes:
>>> v2:
>>> - updated sustainable power section in IPA documentation
>>> - updated DT binding for the 'sustainable-power'
>>>
>>> The v1 of the patch set and related discussion can be found in [1].
>>>
>>> Regards,
>>> Lukasz Luba
>>>
>>> [1]
>>> https://lore.kernel.org/linux-doc/20200929121610.16060-1-lukasz.luba@arm.com/
>>>
>>>
>>> Lukasz Luba (3):
>>>     docs: Clarify abstract scale usage for power values in Energy Model
>>>     PM / EM: update the comments related to power scale
>>>     dt-bindings: thermal: update sustainable-power with abstract scale
>>>
>>>    .../devicetree/bindings/thermal/thermal-zones.yaml  | 13 +++++++++----
>>>    .../driver-api/thermal/power_allocator.rst          | 13 ++++++++++++-
>>>    Documentation/power/energy-model.rst                | 13 +++++++++++++
>>>    Documentation/scheduler/sched-energy.rst            |  5 +++++
>>>    include/linux/energy_model.h                        | 11 +++++------
>>>    kernel/power/energy_model.c                         |  2 +-
>>>    6 files changed, 45 insertions(+), 12 deletions(-)
>>>
>>
>> Could you take patch 1/3 and patch 2/3 via your PM tree,
>> please? I will be very grateful.
>>
>> These patches just update the documentation and comments regarding
>> an issue that we can have: bogoWatts in the Energy Model (and we
>> already have). One of the drawbacks is that we cannot derive real energy
>> from these numbers. Will see how this would evolve.
> 
> The purpose of the energy model is to provide these power numbers.
> 
> If the SoC vendors do not want to share those numbers, then better to
> not use the energy model at all.
> 
> If they want to use the EAS and the IPA at all costs without sharing the
> power numbers, then it is up to them to take responsibility of providing
> consistent numbers, not the community to document how to hack the energy
> model.
> 
> And that is even more true as mentioned by Doug: the power numbers are
> not impossible to measure.
> 
> Documenting the scale values give the opportunity to the SoC vendor to
> never share the power numbers, and even worst, that implies all the
> existing and future frameworks based on the energy model (and its
> evolution) *must* comply with these dummy values. That is the promise of
> a real pain.
> 
> IMO, we must keep a strong constraint on the power values for the energy
> model.
> 
> However, nothing prevents to write a recipe on a website explaining how
> to use the energy model without the power numbers with a big warning
> that could not work in the future if the energy model evolves or it
> could be incompatible with the IPA.
> 
> I suggest to solve the energy model main issue: the SoC vendor do not
> want to share the power numbers. Why not give the opportunity to load a
> firmware where the power numbers will be ? The firmware could be in a
> vendor partition for example.
> 
> 

I understand your concerns. Unfortunately, the reality is that the
bogoWatts are there. I had discussion about it a few days ago with
Rajendra and Doug [1], where I was also opposed to allow bogoValue
coming from DT 'dynamic-power-coefficient'. But I have discussed it
internally and we allow, because developers would do it anyway.

Regarding your question with firmware where the power numbers can be
stored. Unfortunately, it is quite opposite, FW might want to hide it.
We even allow bogoWatts to come from firmware, the SCMI spec:
(4.5.1 Performance domain management protocol background)
'The power can be expressed in mW or in an abstract scale. Vendors are
not obliged to reveal power costs if it is undesirable, but a linear
scale is required.'
The callback which does this is not able to check if the value is a
bogoWatt [2].

EAS can handle EM with bogoWatts, as I described in the patch.
IPA has some issues: 'sustainable-power' in DT (which shouldn't be used
when EM devices use abstract scale) but sysfs interface can be used.

This patch set just align the SCMI spec with EM, EAS, IPA
documentation and already present platforms which use it.

I hope that the real milliWatts would come to EM via the DT
'dynamic-power-coefficient' and function dev_pm_opp_of_register_em().
But no guaranties as you can see in [1].

Regards,
Lukasz

[1] 
https://lore.kernel.org/linux-pm/62540312-65a2-b6d9-86ce-b4deaaa913c1@codeaurora.org/
[2] 
https://elixir.bootlin.com/linux/v5.9/source/drivers/cpufreq/scmi-cpufreq.c#L118

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

[Daniel Lezcano]

On 14/10/2020 11:08, Lukasz Luba wrote:
> Hi Daniel,
> 
> On 10/14/20 9:22 AM, Daniel Lezcano wrote:
>>
>> Hi Lukasz,
>>
>> On 09/10/2020 11:16, Lukasz Luba wrote:
>>> Hi Rafael,
>>>
>>> On 10/2/20 12:44 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. There is also a need to update the DT binding for
>>>> the
>>>> 'sustainable-power' and allow it to have abstract scale as well.
>>>>
>>>> Changes:
>>>> v2:
>>>> - updated sustainable power section in IPA documentation
>>>> - updated DT binding for the 'sustainable-power'
>>>>
>>>> The v1 of the patch set and related discussion can be found in [1].
>>>>
>>>> Regards,
>>>> Lukasz Luba
>>>>
>>>> [1]
>>>> https://lore.kernel.org/linux-doc/20200929121610.16060-1-lukasz.luba@arm.com/
>>>>
>>>>
>>>>
>>>> Lukasz Luba (3):
>>>>     docs: Clarify abstract scale usage for power values in Energy Model
>>>>     PM / EM: update the comments related to power scale
>>>>     dt-bindings: thermal: update sustainable-power with abstract scale
>>>>
>>>>    .../devicetree/bindings/thermal/thermal-zones.yaml  | 13
>>>> +++++++++----
>>>>    .../driver-api/thermal/power_allocator.rst          | 13
>>>> ++++++++++++-
>>>>    Documentation/power/energy-model.rst                | 13
>>>> +++++++++++++
>>>>    Documentation/scheduler/sched-energy.rst            |  5 +++++
>>>>    include/linux/energy_model.h                        | 11 +++++------
>>>>    kernel/power/energy_model.c                         |  2 +-
>>>>    6 files changed, 45 insertions(+), 12 deletions(-)
>>>>
>>>
>>> Could you take patch 1/3 and patch 2/3 via your PM tree,
>>> please? I will be very grateful.
>>>
>>> These patches just update the documentation and comments regarding
>>> an issue that we can have: bogoWatts in the Energy Model (and we
>>> already have). One of the drawbacks is that we cannot derive real energy
>>> from these numbers. Will see how this would evolve.
>>
>> The purpose of the energy model is to provide these power numbers.
>>
>> If the SoC vendors do not want to share those numbers, then better to
>> not use the energy model at all.
>>
>> If they want to use the EAS and the IPA at all costs without sharing the
>> power numbers, then it is up to them to take responsibility of providing
>> consistent numbers, not the community to document how to hack the energy
>> model.
>>
>> And that is even more true as mentioned by Doug: the power numbers are
>> not impossible to measure.
>>
>> Documenting the scale values give the opportunity to the SoC vendor to
>> never share the power numbers, and even worst, that implies all the
>> existing and future frameworks based on the energy model (and its
>> evolution) *must* comply with these dummy values. That is the promise of
>> a real pain.
>>
>> IMO, we must keep a strong constraint on the power values for the energy
>> model.
>>
>> However, nothing prevents to write a recipe on a website explaining how
>> to use the energy model without the power numbers with a big warning
>> that could not work in the future if the energy model evolves or it
>> could be incompatible with the IPA.
>>
>> I suggest to solve the energy model main issue: the SoC vendor do not
>> want to share the power numbers. Why not give the opportunity to load a
>> firmware where the power numbers will be ? The firmware could be in a
>> vendor partition for example.
>>
>>
> 
> I understand your concerns. Unfortunately, the reality is that the
> bogoWatts are there. I had discussion about it a few days ago with
> Rajendra and Doug [1], where I was also opposed to allow bogoValue
> coming from DT 'dynamic-power-coefficient'. But I have discussed it
> internally and we allow, because developers would do it anyway.

With all respects, 'internal discussions' is like out of tree kernels:
for the community, they don't exist :)

The development process in opensource is, by essence, public.

That said, if the developers want to use abstract values, up to them to
make sure it is consistent with the existing framework.

Why do you need to document that and involve the community
responsibility by adding these information in the Documentation even if
you know different frameworks could be incompatible ?


> Regarding your question with firmware where the power numbers can be
> stored. Unfortunately, it is quite opposite, FW might want to hide it.

No, I meant a firmware file, called by request_firmware(). So instead of
having the power numbers in the DT, so published with the kernel, they
are in the SoC vendor's partition in the firmware file, like
'energy_model.bin'.

Then when the energy model is initialized, it will try to request an
energy model firmware file.

That gives the opportunity to the SoC vendor to put the power numbers in
the file and distribute it with the product.

> We even allow bogoWatts to come from firmware, the SCMI spec:
> (4.5.1 Performance domain management protocol background)
> 'The power can be expressed in mW or in an abstract scale. Vendors are
> not obliged to reveal power costs if it is undesirable, but a linear
> scale is required.'
> The callback which does this is not able to check if the value is a
> bogoWatt [2].

So the definition is clear: '... linear scale is required'. So that
implies *all* power numbers for all devices defined in the SCMI. It is
up to the SoC vendor to provide the right numbers.

The EM / IPA / EAS do not have to care about the values.

> EAS can handle EM with bogoWatts, as I described in the patch.
> IPA has some issues: 'sustainable-power' in DT (which shouldn't be used
> when EM devices use abstract scale) but sysfs interface can be used.

Here the platform is mixing the numbers from different firmwares with
different units.

Why not make things consistent ? If the power numbers are coming from
the SCMI, then ignore the ones coming from the DT, no? That should be
simpler now that we have the energy model used for devfreq and cpufreq.

May be add a flag in the energy model giving the origin of the data?

> This patch set just align the SCMI spec with EM, EAS, IPA
> documentation and already present platforms which use it.

Actually, it is the opposite, the patch aligns EAS and EM to the SCMI
spec, but we end up with IPA based on the EM/SCMI & DT and EAS based on
EM/SCMI, right ?

That is the root cause of the incompatibility.

> I hope that the real milliWatts would come to EM via the DT
> 'dynamic-power-coefficient' and function dev_pm_opp_of_register_em().
> But no guaranties as you can see in [1].

It is not a kernel problem if inconsistent values are specified in the DT.

May be make developer life easier by submitting a script which will take
a device tree, check all power numbers, and consistently abstract them.
The developer will write the real values in the DT, test everything is
working fine, then run the script which will make the 'linear scale' of
all the power numbers and convert them to bogoWatt (with different
properties name, so watt and bogowatt mix can be detected).

In any case, if the DT is specifying real numbers, and SCMI abstract
numbers or the opposite, obviously there is a conflict if we are using both.

I suggest to fix the conflict first and provide the features to make the
numbers more easy to share (like the script described above and/or the
firmware file).

Then with the right tools, everything can be documented.



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

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

[Lukasz Luba]

On 10/14/20 12:23 PM, Daniel Lezcano wrote:
> On 14/10/2020 11:08, Lukasz Luba wrote:
>> Hi Daniel,
>>
>> On 10/14/20 9:22 AM, Daniel Lezcano wrote:
>>>
>>> Hi Lukasz,
>>>
>>> On 09/10/2020 11:16, Lukasz Luba wrote:
>>>> Hi Rafael,
>>>>
>>>> On 10/2/20 12:44 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. There is also a need to update the DT binding for
>>>>> the
>>>>> 'sustainable-power' and allow it to have abstract scale as well.
>>>>>
>>>>> Changes:
>>>>> v2:
>>>>> - updated sustainable power section in IPA documentation
>>>>> - updated DT binding for the 'sustainable-power'
>>>>>
>>>>> The v1 of the patch set and related discussion can be found in [1].
>>>>>
>>>>> Regards,
>>>>> Lukasz Luba
>>>>>
>>>>> [1]
>>>>> https://lore.kernel.org/linux-doc/20200929121610.16060-1-lukasz.luba@arm.com/
>>>>>
>>>>>
>>>>>
>>>>> Lukasz Luba (3):
>>>>>      docs: Clarify abstract scale usage for power values in Energy Model
>>>>>      PM / EM: update the comments related to power scale
>>>>>      dt-bindings: thermal: update sustainable-power with abstract scale
>>>>>
>>>>>     .../devicetree/bindings/thermal/thermal-zones.yaml  | 13
>>>>> +++++++++----
>>>>>     .../driver-api/thermal/power_allocator.rst          | 13
>>>>> ++++++++++++-
>>>>>     Documentation/power/energy-model.rst                | 13
>>>>> +++++++++++++
>>>>>     Documentation/scheduler/sched-energy.rst            |  5 +++++
>>>>>     include/linux/energy_model.h                        | 11 +++++------
>>>>>     kernel/power/energy_model.c                         |  2 +-
>>>>>     6 files changed, 45 insertions(+), 12 deletions(-)
>>>>>
>>>>
>>>> Could you take patch 1/3 and patch 2/3 via your PM tree,
>>>> please? I will be very grateful.
>>>>
>>>> These patches just update the documentation and comments regarding
>>>> an issue that we can have: bogoWatts in the Energy Model (and we
>>>> already have). One of the drawbacks is that we cannot derive real energy
>>>> from these numbers. Will see how this would evolve.
>>>
>>> The purpose of the energy model is to provide these power numbers.
>>>
>>> If the SoC vendors do not want to share those numbers, then better to
>>> not use the energy model at all.
>>>
>>> If they want to use the EAS and the IPA at all costs without sharing the
>>> power numbers, then it is up to them to take responsibility of providing
>>> consistent numbers, not the community to document how to hack the energy
>>> model.
>>>
>>> And that is even more true as mentioned by Doug: the power numbers are
>>> not impossible to measure.
>>>
>>> Documenting the scale values give the opportunity to the SoC vendor to
>>> never share the power numbers, and even worst, that implies all the
>>> existing and future frameworks based on the energy model (and its
>>> evolution) *must* comply with these dummy values. That is the promise of
>>> a real pain.
>>>
>>> IMO, we must keep a strong constraint on the power values for the energy
>>> model.
>>>
>>> However, nothing prevents to write a recipe on a website explaining how
>>> to use the energy model without the power numbers with a big warning
>>> that could not work in the future if the energy model evolves or it
>>> could be incompatible with the IPA.
>>>
>>> I suggest to solve the energy model main issue: the SoC vendor do not
>>> want to share the power numbers. Why not give the opportunity to load a
>>> firmware where the power numbers will be ? The firmware could be in a
>>> vendor partition for example.
>>>
>>>
>>
>> I understand your concerns. Unfortunately, the reality is that the
>> bogoWatts are there. I had discussion about it a few days ago with
>> Rajendra and Doug [1], where I was also opposed to allow bogoValue
>> coming from DT 'dynamic-power-coefficient'. But I have discussed it
>> internally and we allow, because developers would do it anyway.
> 
> With all respects, 'internal discussions' is like out of tree kernels:
> for the community, they don't exist :)
> 
> The development process in opensource is, by essence, public.

Please don't get me wrong. I just wanted to say that this issue has been
discussed in Arm Power team and presented here in public (and it's not
my private view - but the team) responding to community questions.

> 
> That said, if the developers want to use abstract values, up to them to
> make sure it is consistent with the existing framework.
> 
> Why do you need to document that and involve the community
> responsibility by adding these information in the Documentation even if
> you know different frameworks could be incompatible ?

The community asked for better documentation [1].
'I believe relative values work perfectly fine for scheduling decisions'
	Rajendra
(who was right, EAS works fine, but it is not documented)

It started from the discussion there, then discussion on v1 of this
patch set.

So it's the Community who wanted to know more information about 'real'
vs 'abstract' scale for power and the EM registration process.
They made a point and I had to consult it with my team.

> 
> 
>> Regarding your question with firmware where the power numbers can be
>> stored. Unfortunately, it is quite opposite, FW might want to hide it.
> 
> No, I meant a firmware file, called by request_firmware(). So instead of
> having the power numbers in the DT, so published with the kernel, they
> are in the SoC vendor's partition in the firmware file, like
> 'energy_model.bin'.
> 
> Then when the energy model is initialized, it will try to request an
> energy model firmware file.
> 
> That gives the opportunity to the SoC vendor to put the power numbers in
> the file and distribute it with the product.

Yes, but it will not protect EM from vendors writing bogoWatts in there.

> 
>> We even allow bogoWatts to come from firmware, the SCMI spec:
>> (4.5.1 Performance domain management protocol background)
>> 'The power can be expressed in mW or in an abstract scale. Vendors are
>> not obliged to reveal power costs if it is undesirable, but a linear
>> scale is required.'
>> The callback which does this is not able to check if the value is a
>> bogoWatt [2].
> 
> So the definition is clear: '... linear scale is required'. So that
> implies *all* power numbers for all devices defined in the SCMI. It is
> up to the SoC vendor to provide the right numbers.

Yes, we wanted to add this to the documentation to make it clear.

> 
> The EM / IPA / EAS do not have to care about the values.

Community developers are asking about it: if IPA or EAS can handle it.
Thus this documentation update.

> 
>> EAS can handle EM with bogoWatts, as I described in the patch.
>> IPA has some issues: 'sustainable-power' in DT (which shouldn't be used
>> when EM devices use abstract scale) but sysfs interface can be used.
> 
> Here the platform is mixing the numbers from different firmwares with
> different units.
> 
> Why not make things consistent ? If the power numbers are coming from
> the SCMI, then ignore the ones coming from the DT, no? That should be
> simpler now that we have the energy model used for devfreq and cpufreq.

The numbers coming from SCMI might be also real mW, the spec allows to
be either milliWatts or bogoWatts. So we cannot block SCMI in your
example.

> 
> May be add a flag in the energy model giving the origin of the data?

I think the flag could cause wrong assumptions.

> 
>> This patch set just align the SCMI spec with EM, EAS, IPA
>> documentation and already present platforms which use it.
> 
> Actually, it is the opposite, the patch aligns EAS and EM to the SCMI
> spec, but we end up with IPA based on the EM/SCMI & DT and EAS based on
> EM/SCMI, right ?

Yes, correct, it aligns EAS, EM with SCMI spec and also community
driver. It makes it clear that EM, EAS allows abstract scale for all
drivers not only SCMI (like Qcom with values not exposing sensitive
information (milliWatts)).

> 
> That is the root cause of the incompatibility.

The patch 3/3 from this series is dropped. Just the IPA documentation is
going to be updated (patch 1/3). It should be, because in EM abstract
scale is allowed and we use EM in IPA.
DT binding for 'sustainable-power' stays as is.

> 
>> I hope that the real milliWatts would come to EM via the DT
>> 'dynamic-power-coefficient' and function dev_pm_opp_of_register_em().
>> But no guaranties as you can see in [1].
> 
> It is not a kernel problem if inconsistent values are specified in the DT.
> 
> May be make developer life easier by submitting a script which will take
> a device tree, check all power numbers, and consistently abstract them.
> The developer will write the real values in the DT, test everything is
> working fine, then run the script which will make the 'linear scale' of
> all the power numbers and convert them to bogoWatt (with different
> properties name, so watt and bogowatt mix can be detected).

Do you mean a new property in DT? Which would handle the bogoWatts?

IMHO DT describes only the HW, using it as a helper for SW abstractions
abuses it. It already has these 'sust. power', 'dynamic-power-coeff.'
and 'capacity-dmips-mhz'. I think Rob will also be against another
stuff fixing our 'generic' software.

We have to update the EM doc about allowed abstract scale, which
implies EAS, IPA doc update with some information to the community that
these components can handle it.

The script will just make developers life easier, but the current
documentation does not say anything about abstract scale.

> 
> In any case, if the DT is specifying real numbers, and SCMI abstract
> numbers or the opposite, obviously there is a conflict if we are using both.

True, DT only allows real numbers (I have Rob's opinion regarding
patch 3/3).

It's not that there is only SCMI which might use abstract scale. Qcom
already has it and other vendors will follow (not exposing real
numbers). They would register bogoWatts to EM because they know that EAS
can deal with both.

> 
> I suggest to fix the conflict first and provide the features to make the
> numbers more easy to share (like the script described above and/or the
> firmware file).
> 
> Then with the right tools, everything can be documented.
> 

We cannot block one way of registration to EM when the other was used.
They might have correct and consistent numbers.

It's up to the platform developers to choose the path:
- go with bogoWatts - if they are not allowed to expose sensitive
   information, use em_dev_register_perf_domain() in drivers, not DT;
   make sure everything that is needed works; check the doc, which
   sub-systems can handle it or needs some tuning (patches 1/3 and 2/3
   try to help here);
- use milliWatts - easier; DT is allowed; help from the community in
   reviews, possible results comparisons; both EM registration ways
   might be used;

We cannot force vendors/OEM engineers to store milliWatts in the
Energy Model if these values are protected by some NDA. Your proposed
way of providing data into EM from user-space firmware.bin IMHO also
falls into the same bucket. That information would be accessible in EM
debugfs and they would avoid it.

Regards,
Lukasz

[1] 
https://lore.kernel.org/linux-devicetree/248bb01e-1746-c84c-78c4-3cf7d2541a70@codeaurora.org/

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

[Daniel Lezcano]

On 14/10/2020 17:24, Lukasz Luba wrote:

[ ... ]

> We have to update the EM doc about allowed abstract scale, which
> implies EAS, IPA doc update with some information to the community that
> these components can handle it.
> 
> The script will just make developers life easier, but the current
> documentation does not say anything about abstract scale.

... yes, because there is no consistency across the source of power
numbers and no tools to ensure DT power numbers consistency, yet.

>> In any case, if the DT is specifying real numbers, and SCMI abstract
>> numbers or the opposite, obviously there is a conflict if we are using
>> both.
> 
> True, DT only allows real numbers (I have Rob's opinion regarding
> patch 3/3).
> 
> It's not that there is only SCMI which might use abstract scale. Qcom
> already has it and other vendors will follow (not exposing real
> numbers). They would register bogoWatts to EM because they know that EAS
> can deal with both.

So vendors are using bogoWatts, despite the documentation.

By updating the documentation saying it supports the abstract values,
that means every new framework, device with power values, will have to
comply with that. How is it possible to add a device with power numbers
if the existing ones are obfuscated ?

With two subsystems using the energy model, evolving independently we
can see there are conflicts. With more subsystems, that may become a
source of confusion, especially with different contributors.

I think the energy model should stick to milliwatts and keep the
documentation unchanged regarding this. And vendors should take the
responsibility of not sticking to the documentation.

>> I suggest to fix the conflict first and provide the features to make the
>> numbers more easy to share (like the script described above and/or the
>> firmware file).
>>
>> Then with the right tools, everything can be documented.
>>
> 
> We cannot block one way of registration to EM when the other was used.
> They might have correct and consistent numbers.

What is the rational of using two firmware power information ?

> It's up to the platform developers to choose the path:
> - go with bogoWatts - if they are not allowed to expose sensitive
>   information, use em_dev_register_perf_domain() in drivers, not DT;
>   make sure everything that is needed works; check the doc, which
>   sub-systems can handle it or needs some tuning (patches 1/3 and 2/3
>   try to help here);
> - use milliWatts - easier; DT is allowed; help from the community in
>   reviews, possible results comparisons; both EM registration ways
>   might be used;
> 
> We cannot force vendors/OEM engineers to store milliWatts in the
> Energy Model if these values are protected by some NDA. 

If I am able to measure one real power value, (and I'm pretty sure it is
quite possible), whatever which one, it is possible to deduce all the
numbers with the linear scale. IMO that is a false debate. Anyway ...

> Your proposed
> way of providing data into EM from user-space firmware.bin IMHO also
> falls into the same bucket. That information would be accessible in EM
> debugfs and they would avoid it.

I think you misunderstood my point.

There is the SCMI and the DT. Because there are two sources where it is
impossible to know if they are using the same units, we are stuck to
ensure a consistency for the kernel.

The platform should use:
 - the SCMI only (scaled or real)
 - the DT only (real)
 [ - the firmware file only (scaled or real) ]


As it is not possible to know if they are scaled or real, there is no
choice except making them mutually exclusive.

From my POV, it is not adequate to let SCMI power information co-exists
with the DT power information if we know they can be with different units.

I've just expressed my opinions:

 - vendors take responsibility of putting different units for the EM

 - Power numbers should come from the same source


Up to Rafael to decide what to do with this documentation update.

Thanks
  -- Daniel


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

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

[Lukasz Luba]

On 10/14/20 6:10 PM, Daniel Lezcano wrote:
> On 14/10/2020 17:24, Lukasz Luba wrote:
> 
> [ ... ]
> 
>> We have to update the EM doc about allowed abstract scale, which
>> implies EAS, IPA doc update with some information to the community that
>> these components can handle it.
>>
>> The script will just make developers life easier, but the current
>> documentation does not say anything about abstract scale.
> 
> ... yes, because there is no consistency across the source of power
> numbers and no tools to ensure DT power numbers consistency, yet.
> 
>>> In any case, if the DT is specifying real numbers, and SCMI abstract
>>> numbers or the opposite, obviously there is a conflict if we are using
>>> both.
>>
>> True, DT only allows real numbers (I have Rob's opinion regarding
>> patch 3/3).
>>
>> It's not that there is only SCMI which might use abstract scale. Qcom
>> already has it and other vendors will follow (not exposing real
>> numbers). They would register bogoWatts to EM because they know that EAS
>> can deal with both.
> 
> So vendors are using bogoWatts, despite the documentation.
> 
> By updating the documentation saying it supports the abstract values,
> that means every new framework, device with power values, will have to
> comply with that. How is it possible to add a device with power numbers
> if the existing ones are obfuscated ?

Good question. I've been thinking about this Qcom platform. If
someone after a while would like to add new device to EM e.g DSP
of that SoC, then it has to measure the real power and decode the
existing numbers for the CPUs. Then encode the new device power
numbers in the same way and register to EM.

> 
> With two subsystems using the energy model, evolving independently we
> can see there are conflicts. With more subsystems, that may become a
> source of confusion, especially with different contributors.
> 
> I think the energy model should stick to milliwatts and keep the
> documentation unchanged regarding this. And vendors should take the
> responsibility of not sticking to the documentation.
> 
>>> I suggest to fix the conflict first and provide the features to make the
>>> numbers more easy to share (like the script described above and/or the
>>> firmware file).
>>>
>>> Then with the right tools, everything can be documented.
>>>
>>
>> We cannot block one way of registration to EM when the other was used.
>> They might have correct and consistent numbers.
> 
> What is the rational of using two firmware power information ?

I mean, there is two ways to register into EM:
1) em_dev_register_perf_domain() - can bring milliWatts or bogoWatts
2) dev_pm_opp_of_register_em() - should bring only milliWatts


> 
>> It's up to the platform developers to choose the path:
>> - go with bogoWatts - if they are not allowed to expose sensitive
>>    information, use em_dev_register_perf_domain() in drivers, not DT;
>>    make sure everything that is needed works; check the doc, which
>>    sub-systems can handle it or needs some tuning (patches 1/3 and 2/3
>>    try to help here);
>> - use milliWatts - easier; DT is allowed; help from the community in
>>    reviews, possible results comparisons; both EM registration ways
>>    might be used;
>>
>> We cannot force vendors/OEM engineers to store milliWatts in the
>> Energy Model if these values are protected by some NDA.
> 
> If I am able to measure one real power value, (and I'm pretty sure it is
> quite possible), whatever which one, it is possible to deduce all the
> numbers with the linear scale. IMO that is a false debate. Anyway ...
> 
>> Your proposed
>> way of providing data into EM from user-space firmware.bin IMHO also
>> falls into the same bucket. That information would be accessible in EM
>> debugfs and they would avoid it.
> 
> I think you misunderstood my point.
> 
> There is the SCMI and the DT. Because there are two sources where it is
> impossible to know if they are using the same units, we are stuck to
> ensure a consistency for the kernel.
> 
> The platform should use:
>   - the SCMI only (scaled or real)
>   - the DT only (real)
>   [ - the firmware file only (scaled or real) ]
> 

Do you mean by SCMI - registration using em_dev_register_perf_domain() ?

> 
> As it is not possible to know if they are scaled or real, there is no
> choice except making them mutually exclusive.

So you propose a bit more restriction in registration EM, to not get
lost in the future. I also have these doubts. Let's consider it and
maybe agree.

I've recommended Qcom to use em_dev_register_perf_domain() when they
have this obfuscated power values. Then any developer in the future
who wants to add EM for a new device on that platform, should use the
em_dev_register_perf_domain().

In this case the flag in EM that you have proposed makes sense.
We probably need an argument 'bool abstract_scale' in the
em_dev_register_perf_domain(..., bool abstract_scale)
as a source of information.

We would allow to co-exist em_dev_register_perf_domain(..., false)
with dev_pm_opp_of_register_em() EM devices.

Is it make sense?

> 
>  From my POV, it is not adequate to let SCMI power information co-exists
> with the DT power information if we know they can be with different units.
> 
> I've just expressed my opinions:
> 
>   - vendors take responsibility of putting different units for the EM
> 
>   - Power numbers should come from the same source
> 
> 
> Up to Rafael to decide what to do with this documentation update.
> 
> Thanks
>    -- Daniel
> 
> 

Thank you for your valuable opinion.

Regards,
Lukasz

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

[Daniel Lezcano]

On 15/10/2020 11:00, Lukasz Luba wrote:

[ ... ]

>> There is the SCMI and the DT. Because there are two sources where it is
>> impossible to know if they are using the same units, we are stuck to
>> ensure a consistency for the kernel.
>>
>> The platform should use:
>>   - the SCMI only (scaled or real)
>>   - the DT only (real)
>>   [ - the firmware file only (scaled or real) ]
>>
> 
> Do you mean by SCMI - registration using em_dev_register_perf_domain() ?

It was high level description, but yes, I guess it is the case.

>> As it is not possible to know if they are scaled or real, there is no
>> choice except making them mutually exclusive.
> 
> So you propose a bit more restriction in registration EM, to not get
> lost in the future. I also have these doubts. Let's consider it and
> maybe agree.
> 
> I've recommended Qcom to use em_dev_register_perf_domain() when they
> have this obfuscated power values. Then any developer in the future
> who wants to add EM for a new device on that platform, should use the
> em_dev_register_perf_domain().
> 
> In this case the flag in EM that you have proposed makes sense.
> We probably need an argument 'bool abstract_scale' in the
> em_dev_register_perf_domain(..., bool abstract_scale)
> as a source of information.

I was suggesting to add a flag to the em_perf_domain structure giving
the source of the power numbers.

So if the IPA is having the 'sustainable-power' set in DT but the
em_perf_domain is flagged with power number coming from SCMI, then they
will be incompatible, the thermal zone will fail to register.


> We would allow to co-exist em_dev_register_perf_domain(..., false)
> with dev_pm_opp_of_register_em() EM devices.
> 
> Is it make sense?

Well, it does not change my opinion. We should assume the energy model
is always milliwatts. If the SoC vendors find a way to get around with
bogoWatts, then good to them and up to them to deal with in the future.


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

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

[Rafael J. Wysocki]

On Thu, Oct 15, 2020 at 12:22 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 15/10/2020 11:00, Lukasz Luba wrote:
>
> [ ... ]
>
> >> There is the SCMI and the DT. Because there are two sources where it is
> >> impossible to know if they are using the same units, we are stuck to
> >> ensure a consistency for the kernel.
> >>
> >> The platform should use:
> >>   - the SCMI only (scaled or real)
> >>   - the DT only (real)
> >>   [ - the firmware file only (scaled or real) ]
> >>
> >
> > Do you mean by SCMI - registration using em_dev_register_perf_domain() ?
>
> It was high level description, but yes, I guess it is the case.
>
> >> As it is not possible to know if they are scaled or real, there is no
> >> choice except making them mutually exclusive.
> >
> > So you propose a bit more restriction in registration EM, to not get
> > lost in the future. I also have these doubts. Let's consider it and
> > maybe agree.
> >
> > I've recommended Qcom to use em_dev_register_perf_domain() when they
> > have this obfuscated power values. Then any developer in the future
> > who wants to add EM for a new device on that platform, should use the
> > em_dev_register_perf_domain().
> >
> > In this case the flag in EM that you have proposed makes sense.
> > We probably need an argument 'bool abstract_scale' in the
> > em_dev_register_perf_domain(..., bool abstract_scale)
> > as a source of information.
>
> I was suggesting to add a flag to the em_perf_domain structure giving
> the source of the power numbers.
>
> So if the IPA is having the 'sustainable-power' set in DT but the
> em_perf_domain is flagged with power number coming from SCMI, then they
> will be incompatible, the thermal zone will fail to register.
>
>
> > We would allow to co-exist em_dev_register_perf_domain(..., false)
> > with dev_pm_opp_of_register_em() EM devices.
> >
> > Is it make sense?
>
> Well, it does not change my opinion. We should assume the energy model
> is always milliwatts. If the SoC vendors find a way to get around with
> bogoWatts, then good to them and up to them to deal with in the future.

That sounds fair enough, but it also means that any kernel patches
using power units different from milliwatts for the EM should be
rejected in the future, doesn't it?

And the existing code using different power units for the EM (if any)
should be updated/fixed accordingly, shouldn't it?

Otherwise I don't see now this can be regarded as a hard rule.

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

[Quentin Perret]

On Thursday 15 Oct 2020 at 15:40:16 (+0200), Rafael J. Wysocki wrote:
> On Thu, Oct 15, 2020 at 12:22 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> >
> > On 15/10/2020 11:00, Lukasz Luba wrote:
> >
> > [ ... ]
> >
> > >> There is the SCMI and the DT. Because there are two sources where it is
> > >> impossible to know if they are using the same units, we are stuck to
> > >> ensure a consistency for the kernel.
> > >>
> > >> The platform should use:
> > >>   - the SCMI only (scaled or real)
> > >>   - the DT only (real)
> > >>   [ - the firmware file only (scaled or real) ]
> > >>
> > >
> > > Do you mean by SCMI - registration using em_dev_register_perf_domain() ?
> >
> > It was high level description, but yes, I guess it is the case.
> >
> > >> As it is not possible to know if they are scaled or real, there is no
> > >> choice except making them mutually exclusive.
> > >
> > > So you propose a bit more restriction in registration EM, to not get
> > > lost in the future. I also have these doubts. Let's consider it and
> > > maybe agree.
> > >
> > > I've recommended Qcom to use em_dev_register_perf_domain() when they
> > > have this obfuscated power values. Then any developer in the future
> > > who wants to add EM for a new device on that platform, should use the
> > > em_dev_register_perf_domain().
> > >
> > > In this case the flag in EM that you have proposed makes sense.
> > > We probably need an argument 'bool abstract_scale' in the
> > > em_dev_register_perf_domain(..., bool abstract_scale)
> > > as a source of information.
> >
> > I was suggesting to add a flag to the em_perf_domain structure giving
> > the source of the power numbers.
> >
> > So if the IPA is having the 'sustainable-power' set in DT but the
> > em_perf_domain is flagged with power number coming from SCMI, then they
> > will be incompatible, the thermal zone will fail to register.
> >
> >
> > > We would allow to co-exist em_dev_register_perf_domain(..., false)
> > > with dev_pm_opp_of_register_em() EM devices.
> > >
> > > Is it make sense?
> >
> > Well, it does not change my opinion. We should assume the energy model
> > is always milliwatts. If the SoC vendors find a way to get around with
> > bogoWatts, then good to them and up to them to deal with in the future.
> 
> That sounds fair enough, but it also means that any kernel patches
> using power units different from milliwatts for the EM should be
> rejected in the future, doesn't it?
> 
> And the existing code using different power units for the EM (if any)
> should be updated/fixed accordingly, shouldn't it?
> 
> Otherwise I don't see now this can be regarded as a hard rule.

Sorry, jumping late in the discussion :)

To add a bit of background to this, it's been the plan from the very
beginning to make PM_EM use an abstract scale. The only reason it was
not merged like that is because the first version only worked for CPUs,
and IPA was using a totally different source for other devices. So we
had no choice but to specify PM_EM in mW to keep things compatible and
allow to transition IPA. But that is no longer true, so I'm in favor of
evolving PM_EM where it was supposed to be to begin with.

IMO, the only thing the kernel cares about is consistency across power
numbers, but not about the exact unit. And I agree with Rafael, we have
code paths in the kernel that feed data in PM_EM but _cannot_ guarantee
mW, SCMI being a prime example, so I don't think it is reasonable to
mandate that.

Having that properly documented + an 'abstract_scale' parameter in
dev_pm_opp_of_register_em() (or even a unit, which could be bogo-watts)
should work IMO. What is the concern with this approach?

Thanks,
Quentin

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

[Daniel Lezcano]

On 15/10/2020 15:40, Rafael J. Wysocki wrote:
> On Thu, Oct 15, 2020 at 12:22 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:

[ ... ]

>>> We would allow to co-exist em_dev_register_perf_domain(..., false)
>>> with dev_pm_opp_of_register_em() EM devices.
>>>
>>> Is it make sense?
>>
>> Well, it does not change my opinion. We should assume the energy model
>> is always milliwatts. If the SoC vendors find a way to get around with
>> bogoWatts, then good to them and up to them to deal with in the future.
> 
> That sounds fair enough, but it also means that any kernel patches
> using power units different from milliwatts for the EM should be
> rejected in the future, doesn't it?

Actually there are two things: the units and the numbers.

The energy model is expressed in mW.

All the frameworks (EAS, IPA, hopefully DTPM) using the energy model
should stick to the same unit, which I believe makes sense.

The numbers are provided by the SoC vendor or any contributors [1][2].

The different frameworks depends on those numbers.

If we specify in the documentation we support abstract numbers for the
EM, then that will imply any framework using it will have to comply with
that.

My point is we use milliwatts as a reference.

If we want to support abstract values, then the code should be changed
by *explicitly* use with these values, so if the other frameworks are
expecting real watts, they can detect they are not available and take
another action, like the scmi scaled power numbers and the
sustainable-power of the thermal which are incompatible.

If the consistency across the frameworks is guarantee by identifying the
kind of values (abstract or real), then we can put in the documentation
we support abstract value.

Unfortunately, IIUC, scmi does not tell us if the power numbers are real
or abstract ... :/

I don't see how we can ensure a consistency across the framework without
enforcing a strong policy.

> And the existing code using different power units for the EM (if any)
> should be updated/fixed accordingly, shouldn't it?

Currently, the power units are expressed in mwatts for the energy model
and the frameworks using it.  AFAICT, no change is needed if we keep mW.

If we use scaled numbers, the EAS will work correctly (but the energy
values will be incorrect), but other frameworks won't.

The power numbers are provided by the DT (as supposed real), or by SCMI
(real or abstract).

If the SCMI is returning abstract numbers, the thermal IPA governor will
use these numbers as a reference to mitigate the temperature at the
specified sustainable power which is expressed in mW in the DT. So it
does not work and we can not detect such conflict.

That is why I'm advocating to keep mW for the energy model and make the
SCMI and DT power numbers incompatible.

  -- Daniel


[1]
https://patchwork.kernel.org/project/linux-arm-kernel/patch/1500974575-2244-1-git-send-email-wxt@rock-chips.com/

[2]
https://patchwork.kernel.org/project/linux-arm-kernel/patch/20190604165802.7338-2-daniel.lezcano@linaro.org/#22686211

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

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

[Quentin Perret]

On Friday 16 Oct 2020 at 13:48:33 (+0200), Daniel Lezcano wrote:
> If the SCMI is returning abstract numbers, the thermal IPA governor will
> use these numbers as a reference to mitigate the temperature at the
> specified sustainable power which is expressed in mW in the DT. So it
> does not work and we can not detect such conflict.
> 
> That is why I'm advocating to keep mW for the energy model and make the
> SCMI and DT power numbers incompatible.

I think it's fair to say SCMI-provided number should only be compared to
other SCMI-provided numbers, so +1 on that. But what I don't understand
is why specifying the EM in mW helps with that? Can we not let the
providers specify the unit? And then it's up to the clients to decide
what they want to do. The scheduler wouldn't care, and IPA would have to
check things are comparable, but all in all that should work out fine
without a strong requirement on the actual unit.

Also, I thought SCMI had a notion of sustained performance too, why
can't we use that for IPA? Lukasz?

Thanks.
Quentin

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

[Daniel Lezcano]

On 16/10/2020 14:18, Quentin Perret wrote:
> On Friday 16 Oct 2020 at 13:48:33 (+0200), Daniel Lezcano wrote:
>> If the SCMI is returning abstract numbers, the thermal IPA governor will
>> use these numbers as a reference to mitigate the temperature at the
>> specified sustainable power which is expressed in mW in the DT. So it
>> does not work and we can not detect such conflict.
>>
>> That is why I'm advocating to keep mW for the energy model and make the
>> SCMI and DT power numbers incompatible.
> 
> I think it's fair to say SCMI-provided number should only be compared to
> other SCMI-provided numbers, so +1 on that. But what I don't understand
> is why specifying the EM in mW helps with that?

It is already specified in mW. I'm just saying to not add the
'scale'/'abstract'/'bogoWatt' in the documentation.

> Can we not let the providers specify the unit? 

Yes, it is possible but the provider must give the 'unit' and the energy
model must store this information along with the "power" numbers, so we
can compare apple with apple.

Today, the energy model is using the mW unit only and the providers are
not telling the 'unit', so both are missing.

Because both are missing, it does not make sense to talk about
'abstract' values in the energy model documentation until the above is
fixed.



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

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

[Quentin Perret]

On Friday 16 Oct 2020 at 14:50:29 (+0200), Daniel Lezcano wrote:
> On 16/10/2020 14:18, Quentin Perret wrote:
> > On Friday 16 Oct 2020 at 13:48:33 (+0200), Daniel Lezcano wrote:
> >> If the SCMI is returning abstract numbers, the thermal IPA governor will
> >> use these numbers as a reference to mitigate the temperature at the
> >> specified sustainable power which is expressed in mW in the DT. So it
> >> does not work and we can not detect such conflict.
> >>
> >> That is why I'm advocating to keep mW for the energy model and make the
> >> SCMI and DT power numbers incompatible.
> > 
> > I think it's fair to say SCMI-provided number should only be compared to
> > other SCMI-provided numbers, so +1 on that. But what I don't understand
> > is why specifying the EM in mW helps with that?
> 
> It is already specified in mW. I'm just saying to not add the
> 'scale'/'abstract'/'bogoWatt' in the documentation.
> 
> > Can we not let the providers specify the unit? 
> 
> Yes, it is possible but the provider must give the 'unit' and the energy
> model must store this information along with the "power" numbers, so we
> can compare apple with apple.
> 
> Today, the energy model is using the mW unit only and the providers are
> not telling the 'unit', so both are missing.
> 
> Because both are missing, it does not make sense to talk about
> 'abstract' values in the energy model documentation until the above is
> fixed.

Right, so that sounds like a reasonable way forward with this series.

Lukasz would you be able to re-spin this with a first patch that allows
the EM provider to specify a unit? And perhaps we could use Doug's idea
for the sustained power DT binding and allow specifying a unit
explicitly there too, so we're sure to compare apples with apples.

Thanks,
Quentin

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

[Doug Anderson]

Hi,

On Fri, Oct 16, 2020 at 6:09 AM Quentin Perret <qperret@google.com> wrote:
>
> On Friday 16 Oct 2020 at 14:50:29 (+0200), Daniel Lezcano wrote:
> > On 16/10/2020 14:18, Quentin Perret wrote:
> > > On Friday 16 Oct 2020 at 13:48:33 (+0200), Daniel Lezcano wrote:
> > >> If the SCMI is returning abstract numbers, the thermal IPA governor will
> > >> use these numbers as a reference to mitigate the temperature at the
> > >> specified sustainable power which is expressed in mW in the DT. So it
> > >> does not work and we can not detect such conflict.
> > >>
> > >> That is why I'm advocating to keep mW for the energy model and make the
> > >> SCMI and DT power numbers incompatible.
> > >
> > > I think it's fair to say SCMI-provided number should only be compared to
> > > other SCMI-provided numbers, so +1 on that. But what I don't understand
> > > is why specifying the EM in mW helps with that?
> >
> > It is already specified in mW. I'm just saying to not add the
> > 'scale'/'abstract'/'bogoWatt' in the documentation.
> >
> > > Can we not let the providers specify the unit?
> >
> > Yes, it is possible but the provider must give the 'unit' and the energy
> > model must store this information along with the "power" numbers, so we
> > can compare apple with apple.
> >
> > Today, the energy model is using the mW unit only and the providers are
> > not telling the 'unit', so both are missing.
> >
> > Because both are missing, it does not make sense to talk about
> > 'abstract' values in the energy model documentation until the above is
> > fixed.
>
> Right, so that sounds like a reasonable way forward with this series.
>
> Lukasz would you be able to re-spin this with a first patch that allows
> the EM provider to specify a unit? And perhaps we could use Doug's idea
> for the sustained power DT binding and allow specifying a unit
> explicitly there too, so we're sure to compare apples with apples.

The one issue that I started with, though, is that I wanted to be able
to specify "sustainable-power" for a board in the device tree.  Unless
you think you'll convince Rob that it's OK to provide a "units"
property in the device tree then just adding a "units" to the API
won't help us because you'll still be stuck mixing/matching with a
value based in mW, right?  ...or are you suggesting that the
board-specific value "sustainable-power" would also have to come from
SCMI?  That would be pretty annoying.

-Doug

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

[Quentin Perret]

On Friday 16 Oct 2020 at 07:36:03 (-0700), Doug Anderson wrote:
> The one issue that I started with, though, is that I wanted to be able
> to specify "sustainable-power" for a board in the device tree.  Unless
> you think you'll convince Rob that it's OK to provide a "units"
> property in the device tree then just adding a "units" to the API
> won't help us because you'll still be stuck mixing/matching with a
> value based in mW, right?  ...or are you suggesting that the
> board-specific value "sustainable-power" would also have to come from
> SCMI?  That would be pretty annoying.

Hmm, maybe, but that's the sanest option IMO.

We should fix the PM_EM API regardless of the DT stuff because
pretending SCMI values are mW is kinda dodgy and confusing. And for the
sustained power stuff, then yes you need this in a comparable unit. If
SCMI gives it to you then it sounds like should just use that. And if we
can make that change to the DT binding then you'll be able to specify it
there as well. But if we can't, then we just won't support mixing and
matching DT and SCMI values. So, yeah, either the EM or the sustained
power value will have to be provided some other way, to keep thing
consistent ...

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

[Lukasz Luba]

On 10/16/20 2:09 PM, Quentin Perret wrote:
> On Friday 16 Oct 2020 at 14:50:29 (+0200), Daniel Lezcano wrote:
>> On 16/10/2020 14:18, Quentin Perret wrote:
>>> On Friday 16 Oct 2020 at 13:48:33 (+0200), Daniel Lezcano wrote:
>>>> If the SCMI is returning abstract numbers, the thermal IPA governor will
>>>> use these numbers as a reference to mitigate the temperature at the
>>>> specified sustainable power which is expressed in mW in the DT. So it
>>>> does not work and we can not detect such conflict.
>>>>
>>>> That is why I'm advocating to keep mW for the energy model and make the
>>>> SCMI and DT power numbers incompatible.
>>>
>>> I think it's fair to say SCMI-provided number should only be compared to
>>> other SCMI-provided numbers, so +1 on that. But what I don't understand
>>> is why specifying the EM in mW helps with that?
>>
>> It is already specified in mW. I'm just saying to not add the
>> 'scale'/'abstract'/'bogoWatt' in the documentation.
>>
>>> Can we not let the providers specify the unit?
>>
>> Yes, it is possible but the provider must give the 'unit' and the energy
>> model must store this information along with the "power" numbers, so we
>> can compare apple with apple.
>>
>> Today, the energy model is using the mW unit only and the providers are
>> not telling the 'unit', so both are missing.
>>
>> Because both are missing, it does not make sense to talk about
>> 'abstract' values in the energy model documentation until the above is
>> fixed.
> 
> Right, so that sounds like a reasonable way forward with this series.
> 
> Lukasz would you be able to re-spin this with a first patch that allows
> the EM provider to specify a unit? And perhaps we could use Doug's idea
> for the sustained power DT binding and allow specifying a unit
> explicitly there too, so we're sure to compare apples with apples.

Do you mean a new entry in DT which will be always below
'dynamic-power-coefficient' and/or 'sustainable-power' saying the unit
of above value?

There was discussion with Rob (and Doug) about this. I got the
impression he was against any new DT stuff [1].
We don't have to, I think we all agree that DT will only support mW.

I have agreed to this idea having a 'flag' inside EM [2], which
indicates the mW or bogoWatts. It could be set via API:
em_dev_register_perf_domain() and this new last argument.

I can write that patch. There is only two usage (3rd is on LKML) of
that function. The DT way, which is via:
dev_pm_opp_of_register_em() will always set 'true';
Driver direct calls of em_dev_register_perf_domain(), will have to
set appropriate value ('true' or 'false'). The EM struct em_perf_domain
will have the new bool field set based on that.
Is it make sense?

Regards,
Lukasz

[1] 
https://lore.kernel.org/lkml/CAL_JsqJ=brfbLiTm9D+p2N0Az-gcStbYj=RS2EaG50dHo0-5WA@mail.gmail.com/
[2] 
https://lore.kernel.org/lkml/3e3dd42c-48ac-7267-45c5-ca88205611bd@arm.com/

> 
> Thanks,
> Quentin
> 

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

[Quentin Perret]

On Friday 16 Oct 2020 at 15:42:57 (+0100), Lukasz Luba wrote:
> Do you mean a new entry in DT which will be always below
> 'dynamic-power-coefficient' and/or 'sustainable-power' saying the unit
> of above value?

Yes, something like that.

> There was discussion with Rob (and Doug) about this. I got the
> impression he was against any new DT stuff [1].
> We don't have to, I think we all agree that DT will only support mW.

Right, I agree this is a 'nice-to-have'.

> I have agreed to this idea having a 'flag' inside EM [2], which
> indicates the mW or bogoWatts. It could be set via API:
> em_dev_register_perf_domain() and this new last argument.
> 
> I can write that patch. There is only two usage (3rd is on LKML) of
> that function. The DT way, which is via:
> dev_pm_opp_of_register_em() will always set 'true';
> Driver direct calls of em_dev_register_perf_domain(), will have to
> set appropriate value ('true' or 'false'). The EM struct em_perf_domain
> will have the new bool field set based on that.
> Is it make sense?

I had something more complicated in mind, where units are arbitrary
('milliwats', 'scmi-bogowatts', ...) as that would help if units can be
specified in the DT too, but if we don't care about that then yes I
suppose a boolean flag should do.

Thanks!
Quentin

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

[Lukasz Luba]

On 10/16/20 5:02 PM, Quentin Perret wrote:
> On Friday 16 Oct 2020 at 15:42:57 (+0100), Lukasz Luba wrote:
>> Do you mean a new entry in DT which will be always below
>> 'dynamic-power-coefficient' and/or 'sustainable-power' saying the unit
>> of above value?
> 
> Yes, something like that.
> 
>> There was discussion with Rob (and Doug) about this. I got the
>> impression he was against any new DT stuff [1].
>> We don't have to, I think we all agree that DT will only support mW.
> 
> Right, I agree this is a 'nice-to-have'.
> 
>> I have agreed to this idea having a 'flag' inside EM [2], which
>> indicates the mW or bogoWatts. It could be set via API:
>> em_dev_register_perf_domain() and this new last argument.
>>
>> I can write that patch. There is only two usage (3rd is on LKML) of
>> that function. The DT way, which is via:
>> dev_pm_opp_of_register_em() will always set 'true';
>> Driver direct calls of em_dev_register_perf_domain(), will have to
>> set appropriate value ('true' or 'false'). The EM struct em_perf_domain
>> will have the new bool field set based on that.
>> Is it make sense?
> 
> I had something more complicated in mind, where units are arbitrary
> ('milliwats', 'scmi-bogowatts', ...) as that would help if units can be
> specified in the DT too, but if we don't care about that then yes I
> suppose a boolean flag should do.

Thank you Quentin for help in sorting this out.
I'll send the v3.

Regards,
Lukasz

> 
> Thanks!
> Quentin
> 

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

[Rafael J. Wysocki]

On Wed, Oct 14, 2020 at 7:10 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 14/10/2020 17:24, Lukasz Luba wrote:
>
> [ ... ]
>
> > We have to update the EM doc about allowed abstract scale, which
> > implies EAS, IPA doc update with some information to the community that
> > these components can handle it.
> >
> > The script will just make developers life easier, but the current
> > documentation does not say anything about abstract scale.
>
> ... yes, because there is no consistency across the source of power
> numbers and no tools to ensure DT power numbers consistency, yet.
>
> >> In any case, if the DT is specifying real numbers, and SCMI abstract
> >> numbers or the opposite, obviously there is a conflict if we are using
> >> both.
> >
> > True, DT only allows real numbers (I have Rob's opinion regarding
> > patch 3/3).
> >
> > It's not that there is only SCMI which might use abstract scale. Qcom
> > already has it and other vendors will follow (not exposing real
> > numbers). They would register bogoWatts to EM because they know that EAS
> > can deal with both.
>
> So vendors are using bogoWatts, despite the documentation.
>
> By updating the documentation saying it supports the abstract values,
> that means every new framework, device with power values, will have to
> comply with that. How is it possible to add a device with power numbers
> if the existing ones are obfuscated ?
>
> With two subsystems using the energy model, evolving independently we
> can see there are conflicts. With more subsystems, that may become a
> source of confusion, especially with different contributors.
>
> I think the energy model should stick to milliwatts and keep the
> documentation unchanged regarding this. And vendors should take the
> responsibility of not sticking to the documentation.
>
> >> I suggest to fix the conflict first and provide the features to make the
> >> numbers more easy to share (like the script described above and/or the
> >> firmware file).
> >>
> >> Then with the right tools, everything can be documented.
> >>
> >
> > We cannot block one way of registration to EM when the other was used.
> > They might have correct and consistent numbers.
>
> What is the rational of using two firmware power information ?
>
> > It's up to the platform developers to choose the path:
> > - go with bogoWatts - if they are not allowed to expose sensitive
> >   information, use em_dev_register_perf_domain() in drivers, not DT;
> >   make sure everything that is needed works; check the doc, which
> >   sub-systems can handle it or needs some tuning (patches 1/3 and 2/3
> >   try to help here);
> > - use milliWatts - easier; DT is allowed; help from the community in
> >   reviews, possible results comparisons; both EM registration ways
> >   might be used;
> >
> > We cannot force vendors/OEM engineers to store milliWatts in the
> > Energy Model if these values are protected by some NDA.
>
> If I am able to measure one real power value, (and I'm pretty sure it is
> quite possible), whatever which one, it is possible to deduce all the
> numbers with the linear scale. IMO that is a false debate. Anyway ...
>
> > Your proposed
> > way of providing data into EM from user-space firmware.bin IMHO also
> > falls into the same bucket. That information would be accessible in EM
> > debugfs and they would avoid it.
>
> I think you misunderstood my point.
>
> There is the SCMI and the DT. Because there are two sources where it is
> impossible to know if they are using the same units, we are stuck to
> ensure a consistency for the kernel.
>
> The platform should use:
>  - the SCMI only (scaled or real)
>  - the DT only (real)
>  [ - the firmware file only (scaled or real) ]
>
>
> As it is not possible to know if they are scaled or real, there is no
> choice except making them mutually exclusive.
>
> From my POV, it is not adequate to let SCMI power information co-exists
> with the DT power information if we know they can be with different units.
>
> I've just expressed my opinions:
>
>  - vendors take responsibility of putting different units for the EM
>
>  - Power numbers should come from the same source
>
>
> Up to Rafael to decide what to do with this documentation update.

Well, I was hoping that you both would reach some kind of agreement.

I don't feel like the decision is mine here to be honest.

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

[Daniel Lezcano]

On 15/10/2020 15:33, Rafael J. Wysocki wrote:

[ ... ]

>> Up to Rafael to decide what to do with this documentation update.
> 
> Well, I was hoping that you both would reach some kind of agreement.
> 
> I don't feel like the decision is mine here to be honest.

No problem, probably we have to think about that a bit more before
reaching the agreement.


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