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

[Lukasz Luba <lukasz.luba@arm.com>]

Hi Douglas

On 9/30/20 6:24 PM, Doug Anderson wrote:
> Hi,
> 
> On Wed, Sep 30, 2020 at 8:48 AM Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>
>>
>> On 9/30/2020 7:34 PM, Lukasz Luba wrote:
>>>
>>>
>>> On 9/30/20 11:55 AM, Rajendra Nayak wrote:
>>>>
>>>> On 9/30/2020 1:55 PM, Lukasz Luba wrote:
>>>>> Hi Douglas,
>>>>>
>>>>> On 9/30/20 12:53 AM, Doug Anderson wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Tue, Sep 29, 2020 at 5:16 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>>>>
>>>>>>> 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          |  8 ++++++++
>>>>>>>    Documentation/power/energy-model.rst                | 13 +++++++++++++
>>>>>>>    Documentation/scheduler/sched-energy.rst            |  5 +++++
>>>>>>>    3 files changed, 26 insertions(+)
>>>>>>
>>>>>> I haven't read through these files in massive detail, but the quick
>>>>>> skim makes me believe that your additions seem sane.  In general, I'm
>>>>>> happy with documenting reality, thus:
>>>>>>
>>>>>> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>>>>>
>>>>> Thank you for the review.
>>>>>
>>>>>>
>>>>>> I will note: you haven't actually updated the device tree bindings.
>>>>>> Thus, presumably, anyone who is specifying these numbers in the device
>>>>>> tree is still supposed to specify them in a way that mW can be
>>>>>> recovered, right?  Said another way: nothing about your patches makes
>>>>>> it OK to specify numbers in device trees using an "abstract scale",
>>>>>> right?
>>>>>
>>>>> For completeness, we are talking here about the binding from:
>>>>> Documentation/devicetree/bindings/arm/cpus.yaml
>>>>> which is 'dynamic-power-coefficient'. Yes, it stays untouched, also the
>>>>> unit (uW/MHz/V^2) which then allows to have mW in the power
>>>>> values in the EM.
>>>>
>>>> So for platforms where 'dynamic-power-coefficient' is specified in device tree,
>>>> its always expected to be derived from 'real' power numbers on these platforms in
>>>> 'real' mW?
>>>
>>> Yes, the purpose and the name of that binding was only for 'real'
>>> power in mW.
>>>
>>>>
>>>> Atleast on Qualcomm platforms we have these numbers scaled, so in essence it
>>>> can't be used to derive 'real' mW values. That said we also do not have any of
>>>> the 'platform might face potential issue of mixing devices in one thermal zone
>>>> of two scales' problem.
>>>
>>> If you have these numbers scaled, then it's probably documented
>>> somewhere in your docs for your OEMs, because they might assume it's in
>>> uW/MHz/V^2 (according to the bindings doc). If not, they probably
>>> realized it during the measurements and comparison (that the power in
>>> EM is not what they see on the power meter).
>>> This binding actually helps those developers who take the experiments
>>> and based on measured power values, store derived coefficient.
>>> Everyone can just measure in local setup and compare the results
>>> easily, speaking the same language (proposing maybe a patch adjusting
>>> the value in DT).
>>>
>>>>
>>>> So the question is, can such platforms still use 'dynamic-power-coefficient'
>>>> in device tree and create an abstract scale? The other way of doing this would
>>>> be to *not* specify this value in device tree and have these values stored in the
>>>> cpufreq driver and register a custom callback to do the math.
>>>
>>> But then we would also have to change the name of that binding.
>>>
>>> I'd recommend you the second way that you've described. It will avoid
>>> your OEMs confusion. In your cpufreq driver you can simply register
>>> to EM using the em_dev_register_perf_domain(). In your local
>>> callback you can do whatever you need (read driver array, firmware,
>>> DT, scale or not, etc).
>>> The helper code in dev_pm_opp_of_register_em() is probably not suited
>>> for your use case (when you don't want to share the real power of the
>>> SoC).
>>
>> Got it, thanks for the clarification. I will get the cpufreq driver updated
>> to use em_dev_register_perf_domain() with a custom callback and get rid of these
>> values from device tree.
> 
> This sounds good.  ...except...
> 
> How exactly are boards supposed to provide their "sustainable-power"
> number in this model?  As far as I'm aware, there's no place to
> specify this board-specific file other than in device tree, and the
> bindings [1] say that this value has to be in mW.  Lukasz: how do you
> envision boards can provide "sustainable-power" in cases where the
> energy model is in "abstract scale"?
> 
> [1] Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> 


I am currently investigating this issue. I will keep you in CC list
when I send some patches.

Regards,
Lukasz