by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH 6/6] arm64: dts: renesas: r8a77961: Enable GPU

On Tue, Feb 27, 2024 at 4:46 AM Adam Ford <[email protected]> wrote:
> The GPU on the R-Car M3-W+ 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)
>
> Signed-off-by: Adam Ford <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2024-02-27 09:46:10

On Thu, Mar 7, 2024 at 6:41 AM Frank Binns <[email protected]> wrote:
>
> Hi Adam,
>
> On Mon, 2024-02-26 at 21:45 -0600, Adam Ford wrote:
> > Update the binding to add support for various Renesas SoC's with PowerVR
> > Rogue GX6250 and GX6650 GPUs. These devices only need one clock, so update
> > the table to indicate such like what was done for the ti,am62-gpu.
> >
> > Signed-off-by: Adam Ford <[email protected]>
> >
> > diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> > index 256e252f8087..7c75104df09f 100644
> > --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> > +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> > @@ -14,6 +14,11 @@ properties:
> > compatible:
> > items:
> > - enum:
> > + - renesas,r8a774a1-gpu
> > + - renesas,r8a774e1-gpu
> > + - renesas,r8a77951-gpu
> > + - renesas,r8a77960-gpu
> > + - renesas,r8a77961-gpu
> > - ti,am62-gpu
> > - const: img,img-axe # IMG AXE GPU model/revision is fully discoverable
>
> A new set of items should be added for 'img,powervr-series6xt' and the Renesas
> models along the lines of [1].

Should I rebase my binding off the one below, so it applies to your
branch or should I attempt to base it off the mainline?
>
> Thanks
> Frank
>
> [1]
> https://gitlab.freedesktop.org/imagination/linux/-/blob/powervr-next/Documentation/devicetree/bindings/gpu/img,powervr.yaml?ref_type=heads#L16-19
>
> >
> > @@ -51,7 +56,13 @@ allOf:
> > properties:
> > compatible:
> > contains:
> > - const: ti,am62-gpu
> > + enum:
> > + - ti,am62-gpu
> > + - renesas,r8a774a1-gpu
> > + - renesas,r8a774e1-gpu
> > + - renesas,r8a77951-gpu
> > + - renesas,r8a77960-gpu
> > + - renesas,r8a77961-gpu
> > then:
> > properties:
> > clocks:

2024-03-11 09:03:51

by Frank Binns

[permalink] [raw]

Subject: Re: [PATCH 2/6] arm64: dts: renesas: r8a774a1: Enable GPU

On Thu, 2024-03-07 at 07:31 -0600, Adam Ford wrote:
> On Thu, Mar 7, 2024 at 6:37 AM Frank Binns <[email protected]> wrote:
> > On Thu, 2024-03-07 at 12:26 +0000, Frank Binns wrote:
> > > On Tue, 2024-02-27 at 05:50 -0600, Adam Ford wrote:
> > > > On Tue, Feb 27, 2024 at 3:31 AM Matt Coster <[email protected]> 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 <[email protected]>
> > > > > >
> > > > > > 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@f1010000 {
> > > > > > resets = <&cpg 408>;
> > > > > > };
> > > > > >
> > > > > > + gpu: gpu@fd000000 {
> > > > > > + 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@fe000000 {
> > > > > > 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.
> > >
> >
> > Oh, I forgot to add, in the meantime, would you find it useful for us to create
> > a Series6XT branch on GitLab where we can include these patches? We can create a
> > corresponding Mesa branch that we'll update as we progress support for GX6250.
> > This should make it easier for you (and others) to test and hopefully make it
> > easier for others to contribute while we work to get support into a good state.
>
> That works for me. Do you want me to make any changes to the series?
> I know there was some discussion about the number of clocks for the
> Renesas variants.

I'd say leave it as is for now. We'll add a cleaned up version of our DT
bindings patch adding 'img,powervr-series6xt' to the branch. You can then rebase
your series on top of that and send out a GitLab merge request for inclusion in
the branch. Does that sound okay?

I'll let you know once we've created the kernel and Mesa branches.

Thanks
Frank

>
> adam
> > > 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