Received-SPF: pass (google.com: domain of linux-kernel+bounces-151821-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Content-Type: text/plain;
	charset=us-ascii
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.500.171.1.1\))
Subject: Re: [PATCH v6 2/2] arm64: dts: rockchip: Add Hantro G1 VPU support
 for RK3588
From: Hugh Cole-Baker <sigmaris@gmail.com>
In-Reply-To: <20240418111002.83015-3-liujianfeng1994@gmail.com>
Date: Fri, 19 Apr 2024 18:28:01 +0100
Cc: linux-media@vger.kernel.org,
 linux-rockchip@lists.infradead.org,
 devicetree@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org,
 linux-kernel@vger.kernel.org,
 ezequiel@vanguardiasur.com.ar,
 p.zabel@pengutronix.de,
 mchehab@kernel.org,
 robh@kernel.org,
 krzk+dt@kernel.org,
 Conor Dooley <conor+dt@kernel.org>,
 Heiko Stuebner <heiko@sntech.de>,
 sfr@canb.auug.org.au,
 sebastian.reichel@collabora.com,
 didi.debian@cknow.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <B9F108CF-4BC5-41A0-A28A-1CA1F4D2CD3C@gmail.com>
References: <20240418111002.83015-1-liujianfeng1994@gmail.com>
 <20240418111002.83015-3-liujianfeng1994@gmail.com>
To: Jianfeng Liu <liujianfeng1994@gmail.com>

Hi Jianfeng,

> On 18 Apr 2024, at 12:10, Jianfeng Liu <liujianfeng1994@gmail.com> =
wrote:
>=20
> Enable Hantro G1 video decoder in RK3588's devicetree.
>=20
> Tested with FFmpeg v4l2_request code taken from [1]
> with MPEG2, H.264 and VP8 samples.
>=20
> [1] =
https://github.com/LibreELEC/LibreELEC.tv/blob/master/packages/multimedia/=
ffmpeg/patches/v4l2-request/ffmpeg-001-v4l2-request.patch
>=20
> Signed-off-by: Jianfeng Liu <liujianfeng1994@gmail.com>
> Tested-by: Hugh Cole-Baker <sigmaris@gmail.com>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
> arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>=20
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi =
b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> index b0a59ec51..b0817382f 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> @@ -1135,6 +1135,27 @@ power-domain@RK3588_PD_SDMMC {
> };
> };
>=20
> + vpu: video-codec@fdb50000 {
> + compatible =3D "rockchip,rk3588-vdpu121", "rockchip,rk3568-vpu";
> + reg =3D <0x0 0xfdb50000 0x0 0x800>;

The register range at 0xfdb50000 length 0x800 includes "VEPU121 core0" =
encoder
regs at offset 0 and "VDPU121" decoder regs at offset 0x400 (referring =
to the
TRM v1.0 Part 1, section 5.5.1). So I think the =
"rockchip,rk3588-vdpu121"
compatible isn't exactly correct to use for this entire device.

IMO "rockchip,rk3588-vpu121" would be more appropriate if including both =
the
decoder and encoder. It also raises the question of whether the decoder =
and
encoder should be modeled in DT as one device like on RK3399, or =
separate
devices. In the vendor DT [0] they are modeled as two devices but they =
share
clocks, resets, IOMMU, and a "rockchip,taskqueue-node" value.

I've tested the JPEG encoding functionality of this encoder with [1], =
and it
seems to work, gstreamer produces a MJPEG video of the test pattern as
expected.

> + interrupts =3D <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH 0>;
> + interrupt-names =3D "vdpu";
> + clocks =3D <&cru ACLK_VPU>, <&cru HCLK_VPU>;
> + clock-names =3D "aclk", "hclk";
> + iommus =3D <&vdpu_mmu>;
> + power-domains =3D <&power RK3588_PD_VDPU>;
> + };
> +
> + vdpu_mmu: iommu@fdb50800 {
> + compatible =3D "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";
> + reg =3D <0x0 0xfdb50800 0x0 0x40>;
> + interrupts =3D <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH 0>;
> + clock-names =3D "aclk", "iface";
> + clocks =3D <&cru ACLK_VPU>, <&cru HCLK_VPU>;
> + power-domains =3D <&power RK3588_PD_VDPU>;
> + #iommu-cells =3D <0>;
> + };
> +
> av1d: video-codec@fdc70000 {
> compatible =3D "rockchip,rk3588-av1-vpu";
> reg =3D <0x0 0xfdc70000 0x0 0x800>;
> --
> 2.34.1
>=20

[0]: =
https://github.com/friendlyarm/kernel-rockchip/blob/18fd1215fee01daef16b6c=
ed1c0c3c3b83a4d8df/arch/arm64/boot/dts/rockchip/rk3588s.dtsi#L3630-L3683
[1]: https://github.com/sigmaris/linux/tree/rk3588-hantro-vpus
     with: gst-launch-1.0 videotestsrc pattern=3Dball flip=3Dtrue ! =
v4l2jpegenc \
      ! matroskamux ! filesink location=3Djpegtest.mkv