Date: Mon, 24 Jul 2017 11:39:37 -0500
From: Rob Herring <robh@kernel.org>
To: Caesar Wang <wxt@rock-chips.com>
Cc: Heiko Stuebner <heiko@sntech.de>, linux-rockchip@lists.infradead.org,
        rocky.hao@rock-chips.com, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
        David Airlie <airlied@linux.ie>, Mark Rutland <mark.rutland@arm.com>
Subject: Re: [PATCH v2 2/5] dt-bindings: gpu: add a power_model optional
 properties for MALI
Message-ID: <20170724163937.ppz24xg6ltkmal7f@rob-hp-laptop>
References: <1500279271-15249-1-git-send-email-wxt@rock-chips.com>
 <1500279271-15249-3-git-send-email-wxt@rock-chips.com>
 <20170717200733.nax6jfbvs3rjd7t4@rob-hp-laptop>
 <3b464bab-2d72-717c-3aa0-410b1e1bd930@rock-chips.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <3b464bab-2d72-717c-3aa0-410b1e1bd930@rock-chips.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3430
Lines: 75

On Tue, Jul 18, 2017 at 08:58:50AM +0800, Caesar Wang wrote:
> Rob,
> 
> 在 2017年07月18日 04:07, Rob Herring 写道:
> > On Mon, Jul 17, 2017 at 04:14:28PM +0800, Caesar Wang wrote:
> > > This patch adds the MALI's power-model to set the IPA model to be used
> > > for power management.
> > What's IPA? India Pale Ale or Intermediate Physical Address?
> 
> IPA is intelligent Power Allocator.  (As the ARM introduced on
> https://developer.arm.com/open-source/intelligent-power-allocation)
> 
> > 
> > > Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> > > ---
> > > 
> > > Changes in v2: None
> > > 
> > >   Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt | 12 ++++++++++++
> > >   1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt b/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
> > > index a461e47..b616e6b 100644
> > > --- a/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
> > > +++ b/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
> > > @@ -37,6 +37,18 @@ Optional properties:
> > >   - operating-points-v2 : Refer to Documentation/devicetree/bindings/power/opp.txt
> > >     for details.
> > > +- power_model : Sets power model parameters. Note that this model was designed for the Juno
> > > +	        platform, and may not be suitable for other platforms. A structure containing :
> > > +	- compatible: Should be arm,mali-simple-power-model
> > > +	- dynamic-coefficient: Coefficient, in pW/(Hz V^2), which is multiplied
> > > +	  by v^2*f to calculate the dynamic power consumption.
> > > +	- static-coefficient: Coefficient, in uW/V^3, which is multiplied by
> > > +	  v^3 to calculate the static power consumption.
> > > +	- ts: An array containing coefficients for the temperature scaling
> > > +	  factor. This is used to scale the static power by a factor of
> > > +	  tsf/1000000, where tsf = ts[3]*T^3 + ts[2]*T^2 + ts[1]*T + ts[0],
> > > +	  and T = temperature in degrees.
> > > +	- thermal-zone: A string identifying the thermal zone used for the GPU
> > This can all easily be implied by the compatible string. I'm not
> > inclined to accept something Mali specific here.
> 
> Isn't  arm,mali-midgard.txt document suit for Mali specific? :-)

It is, but I'm saying we shouldn't have something Mali specific here. It 
should be something that works across different GPUs at least. IOW, get 
some agreement with say adreno folks that these properties are useful and 
I'll be more receptive. 

> > 
> > This looks *very* precise, but I'd be surprised if these values are any
> > more than magic values (at least the dynamic coef) adjusted until the
> > desired power/performance requirements are achieved. To put it another
> > way, why don't we have similar values for CPUs?
> 
> These value was calculated by running full GPU process.
> 
> CPU had the similar value for dtsi.
> 
> Say: arch/arm64/boot/dts/rockchip/rk3399.dtsi
>         cpu_b0: cpu@100 {
>             ...
>             dynamic-power-coefficient = <436>;
>             ...
>         };

Indeed. While it is documented for ARM CPUs, I don't see that it 
is widely used as only the hi6220 dts defines it. So either support for 
platforms is just missing, or upstream is not really using this property.

And if we are going to use this, then it needs to be documented in a 
common location and moved out of arm/cpus.txt.

Rob