Re: [PATCH 1/2] thermal: cooling: Check Energy Model type in cpufreq_cooling and devfreq_cooling

[Matthias Kaehlcke]

On Thu, Feb 17, 2022 at 12:11:27PM +0000, Lukasz Luba wrote:
> 
> 
> On 2/17/22 11:28 AM, Daniel Lezcano wrote:
> > On 17/02/2022 11:47, Lukasz Luba wrote:
> > > Hi Daniel,
> > > 
> > > On 2/17/22 10:10 AM, Daniel Lezcano wrote:
> > > > On 16/02/2022 18:33, Doug Anderson wrote:
> > > > > Hi,
> > > > > 
> > > > > On Wed, Feb 16, 2022 at 7:35 AM Lukasz Luba
> > > > > <lukasz.luba@xxxxxxx> wrote:
> > > > > > 
> > > > > > Hi Matthias,
> > > > > > 
> > > > > > On 2/9/22 10:17 PM, Matthias Kaehlcke wrote:
> > > > > > > On Wed, Feb 09, 2022 at 11:16:36AM +0000, Lukasz Luba wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On 2/8/22 5:25 PM, Matthias Kaehlcke wrote:
> > > > > > > > > On Tue, Feb 08, 2022 at 09:32:28AM +0000, Lukasz Luba wrote:
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > 
> > > > > > [snip]
> > > > > > 
> > > > > > > > > > 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]
> > > > > > > > 
> > > > > > > > Thank you Matthias. I will investigate your setup to get better
> > > > > > > > understanding.
> > > > > > > 
> > > > > > > Thanks!
> > > > > > > 
> > > > > > 
> > > > > > I've checked those DT files and related code.
> > > > > > As you already said, this patch is safe for them.
> > > > > > So we can apply it IMO.
> > > > > > 
> > > > > > 
> > > > > > -------------Off-topic------------------
> > > > > > Not in $subject comments:
> > > > > > 
> > > > > > AFAICS based on two files which define thermal zones:
> > > > > > sc7180-trogdor-homestar.dtsi
> > > > > > sc7180-trogdor-coachz.dtsi
> > > > > > 
> > > > > > only the 'big' cores are used as cooling devices in the
> > > > > > 'skin_temp_thermal' - the CPU6 and CPU7.
> > > > > > 
> > > > > > I assume you don't want to model at all the power usage
> > > > > > from the Little cluster (which is quite big: 6 CPUs), do you?
> > > > > > I can see that the Little CPUs have small dyn-power-coeff
> > > > > > ~30% of the big and lower max freq, but still might be worth
> > > > > > to add them to IPA. You might give them more 'weight', to
> > > > > > make sure they receive more power during power split.
> > > > > > 
> > > > > > You also don't have GPU cooling device in that thermal zone.
> > > > > > Based on my experience if your GPU is a power hungry one,
> > > > > > e.g. 2-4Watts, you might get better results when you model
> > > > > > this 'hot' device (which impacts your temp sensor reported value).
> > > > > 
> > > > > I think the two boards you point at (homestar and coachz) are just the
> > > > > two that override the default defined in the SoC dtsi file. If you
> > > > > look in sc7180.dtsi you'll see 'gpuss1-thermal' which has a cooling
> > > > > map. You can also see the cooling maps for the littles.
> > > > > 
> > > > > I guess we don't have a `dynamic-power-coefficient` for the GPU,
> > > > > though? Seems like we should, but I haven't dug through all the code
> > > > > here...
> > > > 
> > > > The dynamic-power-coefficient is available for OPPs which
> > > > includes CPUfreq and devfreq. As the GPU is managed by devfreq,
> > > > setting the dynamic-power-coefficient makes the energy model
> > > > available for it.
> > > > 
> > > > However, the OPPs must define the frequency and the voltage.
> > > > That is the case for most platforms except on QCom platform.
> > > > 
> > > > That may not be specified as it uses a frequency index and the
> > > > hardware does the voltage change in our back. The QCom cpufreq
> > > > backend get the voltage table from a register (or whatever) and
> > > > completes the voltage values for the OPPs, thus adding the
> > > > information which is missing in the device tree. The energy
> > > > model can then initializes itself and allows the usage of the
> > > > Energy Aware Scheduler.
> > > > 
> > > > However this piece of code is missing for the GPU part.
> > > > 
> > > 
> > > Thank you for joining the discussion. I don't know about that Qcom
> > > GPU voltage information is missing.
> > > 
> > > If the voltage is not available (only the frequencies), there is
> > > another way. There is an 'advanced' EM which uses registration function:
> > > em_dev_register_perf_domain(). It uses a local driver callback to get
> > > power for each found frequency. It has benefit because there is no
> > > restriction to 'fit' into the math formula, instead just avg power
> > > values can be feed into EM. It's called 'advanced' EM [1].
> > > 
> > > Now we hit (again) the DT & EM issue (it's an old one, IIRC Morten
> > > was proposing from ~2014 this upstream, but EAS wasn't merged back
> > > then):
> > > where to store these power-freq values, which are then used by the
> > > callback.
> > 
> > Why not make it more generic and replace the frequency by a performance
> > index, so it can be used by any kind of perf limiter?
> 
> For that DT array, yes, it can be an index, so effectively it could be
> a simple 1d array.
> 
> something like:
> 
> msm_gpu_energy_model: msm-gpu-energy-model {
> 	compatible = "energy-model"
> 	/* Values are sorted micro-Watts which correspond to each OPP
> 	   or performance state. The total amount of them must match
> 	   number of OPPs. */
> 	power-microwatt = <100000>,
> 			<230000>,
> 			<380000>,
> 			<600000>;
> };

IIUC for the QCOM GPU the voltages/power consumption per OPP aren't fixed
but can vary between different SoCs of the same model. If the ranges aren't
too wide it might still be suitable to have a table with the average power
consumption.

Another question is whether QCOM would be willing to provide information
about the GPU power consumption. For the SC7180 CPUs they only provided
bogoWatt numbers. Once boards with a given SoC/GPU are available to the
public someone could come up with such a table based on measurements,
similar to what Doug did for the SC7180 CPUs (commit 82ea7d411d43f).

> then in gpu node instead of having 'dynamic-power-coefficient',
> which is useless because voltage is missing, we would have
> 'energy-model', like:
> 
> 	energy-model = <&msm_gpu_energy_model>;
> 
> 
> If you agree to continue this topic. I will send an RFC so we could
> further discuss this idea. This $subject doesn't fit well.
> 
> Thank you again for your feedback Daniel!

Thanks Lukasz and Daniel for looking into this!

m.