Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 8 Feb 2022 09:25:48 -0800
From:   Matthias Kaehlcke <mka@chromium.org>
To:     Lukasz Luba <lukasz.luba@arm.com>
Cc:     linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
        amit.kachhap@gmail.com, daniel.lezcano@linaro.org,
        viresh.kumar@linaro.org, rafael@kernel.org, amitk@kernel.org,
        rui.zhang@intel.com, dietmar.eggemann@arm.com,
        Pierre.Gondois@arm.com, Douglas Anderson <dianders@chromium.org>,
        Stephen Boyd <swboyd@chromium.org>,
        Rajendra Nayak <rnayak@codeaurora.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>
Subject: Re: [PATCH 1/2] thermal: cooling: Check Energy Model type in
 cpufreq_cooling and devfreq_cooling
Message-ID: <YgKnnFl7Gp8AS30X@google.com>
References: <20220207073036.14901-1-lukasz.luba@arm.com>
 <20220207073036.14901-2-lukasz.luba@arm.com>
 <YgG+TmLrCSXX4Bvt@google.com>
 <4a7d4e94-1461-5bac-5798-29998af9793a@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <4a7d4e94-1461-5bac-5798-29998af9793a@arm.com>
Precedence: bulk

On Tue, Feb 08, 2022 at 09:32:28AM +0000, Lukasz Luba wrote:
> 
> 
> On 2/8/22 12:50 AM, Matthias Kaehlcke wrote:
> > On Mon, Feb 07, 2022 at 07:30:35AM +0000, Lukasz Luba wrote:
> > > The Energy Model supports power values either in Watts or in some abstract
> > > scale. When the 2nd option is in use, the thermal governor IPA should not
> > > be allowed to operate, since the relation between cooling devices is not
> > > properly defined. Thus, it might be possible that big GPU has lower power
> > > values in abstract scale than a Little CPU. To mitigate a misbehaviour
> > > of the thermal control algorithm, simply not register a cooling device
> > > capable of working with IPA.
> > 
> > Ugh, this would break thermal throttling for existing devices that are
> > currently supported in the upstream kernel.
> 
> Could you point me to those devices? I cannot find them in the mainline
> DT. There are no GPU devices which register Energy Model (EM) in
> upstream, neither using DT (which would be power in mW) nor explicitly
> providing EM get_power() callback. The EM is needed to have IPA.
> 
> Please clarify which existing devices are going to be broken with this
> change.

I was thinking about arch/arm64/boot/dts/qcom/sc7180-trogdor-*, and
potentially other SC7180 boards that use IPA for the CPU thermal
zones.

Initially SC7180 used an abstract scale for the CPU energy model,
however I realized your change doesn't actually impact SC7180 CPUs
for two reasons:

1. The energy model of the CPUs is registered through

  cpufreq_register_em_with_opp
    dev_pm_opp_of_register_em
      em_dev_register_perf_domain

em_dev_register_perf_domain() is called with 'milliwatts = true',
regardless of the potentially abstract scale, so IPA would not be
disabled with your change.

2. There is a patch from Doug that adjusted the dynamic power
coefficients of the CPUs to be closer to reality:

commit 82ea7d411d43f60dce878252558e926f957109f0
Author: Douglas Anderson <dianders@chromium.org>
Date:   Thu Sep 2 14:51:37 2021 -0700

    arm64: dts: qcom: sc7180: Base dynamic CPU power coefficients in reality

> > Wasn't the conclusion that it is the responsability of the device tree
> > owners to ensure that cooling devices with different scales aren't used
> > in the same thermal zone?
> 
> It's based on assumption that someone has DT and control. There was also
> implicit assumption that IPA would work properly on such platform - but
> it won't.
> 
> 1. You cannot have 2 thermal zones: one with CPUs and other with GPU
> only and both working with two instances of IPA.

It's not clear to me why such a configuration wouldn't work. Is it also a
problem to have multiple CPU thermal zones (one for each core) that use
multiple instances of IPA? SC7180 has separate thermal zones for each core
(or thermal sensor), Chrome OS uses IPA for CPU thermal throttling.

> 2. The abstract power scale doesn't guaranty anything about power values
> and IPA was simply designed with milli-Watts in mind. So even working
> on CPUs only using bogoWatts, is not what we could guaranty in IPA.

That's bad news for SoCs that are configured with bogoWatt values, from
past discussions I had the impression that this is unfortunately not
uncommon.

> It's ugly to have the abstract scales in the first place, but that's
> unfortunately what we currently have for at least some cooling devices.

> A few questions:
>
> Do you use 'we' as Chrome engineers?

I was referring to the kernel, in particular QCOM SC7180.

> Could you point me to those devices please?

arch/arm64/boot/dts/qcom/sc7180-trogdor-*

Though as per above they shouldn't be impacted by your change, since the
CPUs always pretend to use milli-Watts.

[skipped some questions/answers since sc7180 isn't actually impacted by
 the change]

Thanks

Matthias