Message-ID: <d959159dac0effce1a80986bc87e18ffd24773b9.camel@imgtec.com>
Date: Thu, 7 Mar 2024 12:26:19 +0000
From: Frank Binns <Frank.Binns@...tec.com>
To: "aford173@...il.com" <aford173@...il.com>,
        Matt Coster
	<Matt.Coster@...tec.com>
CC: "magnus.damm@...il.com" <magnus.damm@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "tzimmermann@...e.de" <tzimmermann@...e.de>,
        "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>,
        "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>,
        "robh@...nel.org" <robh@...nel.org>,
        "linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>,
        "airlied@...il.com" <airlied@...il.com>,
        "geert+renesas@...der.be"
	<geert+renesas@...der.be>,
        "maarten.lankhorst@...ux.intel.com"
	<maarten.lankhorst@...ux.intel.com>,
        "krzysztof.kozlowski+dt@...aro.org"
	<krzysztof.kozlowski+dt@...aro.org>,
        "aford@...conembedded.com"
	<aford@...conembedded.com>,
        "daniel@...ll.ch" <daniel@...ll.ch>,
        "conor+dt@...nel.org" <conor+dt@...nel.org>,
        "mripard@...nel.org"
	<mripard@...nel.org>
Subject: Re: [PATCH 2/6] arm64: dts: renesas: r8a774a1: Enable GPU

On Tue, 2024-02-27 at 05:50 -0600, Adam Ford wrote:
> On Tue, Feb 27, 2024 at 3:31 AM Matt Coster <Matt.Coster@...tec.com> wrote:
> > Hi Adam,
> > 
> > Thanks for these patches! I'll just reply to this one patch, but my
> > comments apply to them all.
> > 
> > On 27/02/2024 03:45, Adam Ford wrote:
> > > The GPU on the RZ/G2M is a Rogue GX6250 which uses firmware
> > > rogue_4.45.2.58_v1.fw available from Imagination.
> > > 
> > > When enumerated, it appears as:
> > >   powervr fd000000.gpu: [drm] loaded firmware powervr/rogue_4.45.2.58_v1.fw
> > >   powervr fd000000.gpu: [drm] FW version v1.0 (build 6513336 OS)
> > 
> > These messages are printed after verifying the firmware blob’s headers,
> > *before* attempting to upload it to the device. Just because they appear
> > in dmesg does *not* imply the device is functional beyond the handful of
> > register reads in pvr_load_gpu_id().
> > 
> > Since Mesa does not yet have support for this GPU, there’s not a lot
> > that can be done to actually test these bindings.
> > 
> > When we added upstream support for the first GPU (the AXE core in TI’s
> > AM62), we opted to wait until userspace was sufficiently progressed to
> > the point it could be used for testing. This thought process still
> > applies when adding new GPUs.
> > 
> > Our main concern is that adding bindings for GPUs implies a level of
> > support that cannot be tested. That in turn may make it challenging to
> > justify UAPI changes if/when they’re needed to actually make these GPUs
> > functional.
> 
> I wrongly assumed that when the firmware was ready, there was some
> preliminary functionality, but it sounds like we need to work for
> Series6XT support to be added to the driver.  I only used the AXE
> compatible since it appeared to the be the only one and the existing
> binding document stated "model/revision is fully discoverable" which I
> interpreted to mean that the AXE compatible was sufficient.

The comment is related to there being a few models/revisions of AXE [1][2][3],
which we can distinguish between by reading a register.

> > > Signed-off-by: Adam Ford <aford173@...il.com>
> > > 
> > > diff --git a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi
> > > index a8a44fe5e83b..8923d9624b39 100644
> > > --- a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi
> > > +++ b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi
> > > @@ -2352,6 +2352,16 @@ gic: interrupt-controller@...10000 {
> > >                       resets = <&cpg 408>;
> > >               };
> > > 
> > > +             gpu: gpu@...00000 {
> > > +                     compatible = "renesas,r8a774a1-gpu", "img,img-axe";
> > 
> > The GX6250 is *not* an AXE core - it shouldn’t be listed as compatible
> > with one. For prior art, see [1] where we added support for the MT8173
> > found in Elm Chromebooks R13 (also a Series6XT GPU).
> > 
> > > +                     reg = <0 0xfd000000 0 0x20000>;
> > > +                     clocks = <&cpg CPG_MOD 112>;
> > > +                     clock-names = "core";
> > 
> > Series6XT cores have three clocks (see [1] again). I don’t have a
> > Renesas TRM to hand – do you know if their docs go into detail on the
> > GPU integration?
> > 
> > > +                     interrupts = <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>;
> > > +                     power-domains = <&sysc R8A774A1_PD_3DG_B>;
> > > +                     resets = <&cpg 112>;
> > > +             };
> > > +
> > >               pciec0: pcie@...00000 {
> > >                       compatible = "renesas,pcie-r8a774a1",
> > >                                    "renesas,pcie-rcar-gen3";
> > 
> > As you probably expect by this point, I have to nack this series for
> > now. I appreciate your effort here and I’ll be happy to help you land
> 
> I get that.  I wasn't sure if I should have even pushed this, but I
> wanted to get a little traction, because I know there are people like
> myself who want to use the 3D in the Renesas boards, but don't want to
> use the closed-source blobs tied to EULA and NDA documents.
> 
> > these once Mesa gains some form of usable support to allow testing.
> 
> Is there a way for your group to add me to the CC list when future
> updates are submitted?  I'd like to follow this and resubmit when it's
> ready.

Sure, we'll keep you updated as things progress.

Thanks
Frank

[1] https://www.imaginationtech.com/product/img-axe-1-16m/
[2] https://www.imaginationtech.com/product/img-axe-1-16/
[3] https://www.imaginationtech.com/product/img-axe-2-16/

> > Cheers,
> > Matt
> > 
> > [1]: https://gitlab.freedesktop.org/imagination/linux/-/blob/b3506b8bc45ed6d4005eb32a994df0e33d6613f1/arch/arm64/boot/dts/mediatek/mt8173.dtsi#L993-1006

Powered by blists - more mailing lists