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

[Lukasz Luba]

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@xxxxxxxxxxxxxx> 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@xxxxxxx> 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@xxxxxxx>
> > > > > > > ---
> > > > > > >    .../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@xxxxxxxxxxxx>
> > > > >
> > > > > 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