Received-SPF: pass (google.com: domain of linux-kernel+bounces-83144-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240227034539.193573-1-aford173@gmail.com> <20240227034539.193573-3-aford173@gmail.com>
 <39aead3b-b809-4c9c-8a5d-c0be2b36ea47@imgtec.com>
In-Reply-To: <39aead3b-b809-4c9c-8a5d-c0be2b36ea47@imgtec.com>
From: Adam Ford <aford173@gmail.com>
Date: Tue, 27 Feb 2024 05:50:42 -0600
Message-ID: <CAHCN7xJnKNdsrs+UMvPqdkN+j8v+8UaoH=zargcKRi7dw0GLNA@mail.gmail.com>
Subject: Re: [PATCH 2/6] arm64: dts: renesas: r8a774a1: Enable GPU
To: Matt Coster <Matt.Coster@imgtec.com>
Cc: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, 
	"linux-renesas-soc@vger.kernel.org" <linux-renesas-soc@vger.kernel.org>, Adam Ford <aford@beaconembedded.com>, 
	Frank Binns <Frank.Binns@imgtec.com>, David Airlie <airlied@gmail.com>, 
	Daniel Vetter <daniel@ffwll.ch>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, 
	Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, Rob Herring <robh@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Geert Uytterhoeven <geert+renesas@glider.be>, Magnus Damm <magnus.damm@gmail.com>, 
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Feb 27, 2024 at 3:31=E2=80=AFAM Matt Coster <Matt.Coster@imgtec.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_v=
1.fw
> >   powervr fd000000.gpu: [drm] FW version v1.0 (build 6513336 OS)
>
> These messages are printed after verifying the firmware blob=E2=80=99s he=
aders,
> *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=E2=80=99s 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=E2=
=80=99s
> 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=E2=80=99re needed to actually make thes=
e 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.
>
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> > diff --git a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi b/arch/arm64/boo=
t/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@f1010000 {
> >                       resets =3D <&cpg 408>;
> >               };
> >
> > +             gpu: gpu@fd000000 {
> > +                     compatible =3D "renesas,r8a774a1-gpu", "img,img-a=
xe";
>
> The GX6250 is *not* an AXE core - it shouldn=E2=80=99t be listed as compa=
tible
> with one. For prior art, see [1] where we added support for the MT8173
> found in Elm Chromebooks R13 (also a Series6XT GPU).
>
> > +                     reg =3D <0 0xfd000000 0 0x20000>;
> > +                     clocks =3D <&cpg CPG_MOD 112>;
> > +                     clock-names =3D "core";
>
> Series6XT cores have three clocks (see [1] again). I don=E2=80=99t have a
> Renesas TRM to hand =E2=80=93 do you know if their docs go into detail on=
 the
> GPU integration?
>
> > +                     interrupts =3D <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>;
> > +                     power-domains =3D <&sysc R8A774A1_PD_3DG_B>;
> > +                     resets =3D <&cpg 112>;
> > +             };
> > +
> >               pciec0: pcie@fe000000 {
> >                       compatible =3D "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=E2=80=99ll 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.
>
> Cheers,
> Matt
>
> [1]: https://gitlab.freedesktop.org/imagination/linux/-/blob/b3506b8bc45e=
d6d4005eb32a994df0e33d6613f1/arch/arm64/boot/dts/mediatek/mt8173.dtsi#L993-=
1006