Received-SPF: pass (google.com: domain of linux-kernel+bounces-152141-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Jianfeng Liu <liujianfeng1994@gmail.com>
To: sigmaris@gmail.com
Cc: conor+dt@kernel.org,
	devicetree@vger.kernel.org,
	didi.debian@cknow.org,
	ezequiel@vanguardiasur.com.ar,
	heiko@sntech.de,
	krzk+dt@kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	linux-media@vger.kernel.org,
	linux-rockchip@lists.infradead.org,
	liujianfeng1994@gmail.com,
	mchehab@kernel.org,
	p.zabel@pengutronix.de,
	robh@kernel.org,
	sebastian.reichel@collabora.com,
	sfr@canb.auug.org.au,
	nicolas@ndufresne.ca,
	linkmauve@linkmauve.fr
Subject: Re: Re: [PATCH v6 2/2] arm64: dts: rockchip: Add Hantro G1 VPU support for RK3588
Date: Sat, 20 Apr 2024 13:09:13 +0800
Message-Id: <20240420050913.182225-1-liujianfeng1994@gmail.com>
In-Reply-To: <B9F108CF-4BC5-41A0-A28A-1CA1F4D2CD3C@gmail.com>
References: <B9F108CF-4BC5-41A0-A28A-1CA1F4D2CD3C@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Hi Hugh,

Fri, 19 Apr 2024 18:28:01 +0100, Hugh Cole-Baker wrote:
>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.

There are five vepu121 cores for jpeg encoding. And Emmanuel is doing work on
them[1]. And at the moment the driver doesn’t yet support exposing these cores
all as a single video node to userspace, so Emmanuel only exposes one single
core.

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

Now we have 5 jpeg enc cores, one from 0xfdb50000 and other four from
0xfdba00000. I tried to add a decoding only core 0xfb50400, but that does not
work. So the vpu should be defined as one node in devicetree for both encoder
and decoder like rk3399.

This vpu121 should be exactly the same as the one in rk3399 which supports both
encoding and decoding. But the current hantro driver has disabled h264 decoding
since there is anthoer decoder rkvdec on rk3399. This vpu121 is the only
decoder which supports h254 decoding on rk3588, so we can't just use the
vpu_variant from rk3399. Maybe we can use rk3399_vpu_variant back when rkvdec2
on rk3588 is supported by mainline kernel.

At the moment we can keep the compatible string same as the one from rk356x.
Since there are already jpeg enc cores at 0xfdba0000, we can ignore the one at
0xfdb50000. When rkvdec2 is supported, I will change "rockchip,rk3588-vpu121"
same as "rockchip,rk3399-vpu".

And I think changing "rockchip,rk3588-vdpu121" to "rockchip,rk3588-vpu121"
should match the hardware correctly.

[1] https://lore.kernel.org/all/20240418141509.2485053-1-linkmauve@linkmauve.fr/

Best regards,
Jianfeng