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

[Doug Anderson <dianders@chromium.org>]

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


-Doug