Received-SPF: pass (google.com: domain of linux-kernel+bounces-159997-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <bb59b9582e100420ef5ab9fc9a089f91a3d49883.camel@ndufresne.ca>
Subject: Re: Re: [PATCH v6 2/2] arm64: dts: rockchip: Add Hantro G1 VPU
 support for RK3588
From: Nicolas Dufresne <nicolas@ndufresne.ca>
To: Jianfeng Liu <liujianfeng1994@gmail.com>, 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, 
 mchehab@kernel.org, p.zabel@pengutronix.de, robh@kernel.org, 
 sebastian.reichel@collabora.com, sfr@canb.auug.org.au,
 linkmauve@linkmauve.fr
Date: Fri, 26 Apr 2024 08:49:53 -0400
In-Reply-To: <20240420050913.182225-1-liujianfeng1994@gmail.com>
References: <B9F108CF-4BC5-41A0-A28A-1CA1F4D2CD3C@gmail.com>
	 <20240420050913.182225-1-liujianfeng1994@gmail.com>
Autocrypt: addr=nicolas@ndufresne.ca; prefer-encrypt=mutual; keydata=mQGiBEUQN0MRBACQYceNSezSdMjx7sx6gwKkMghrrODgl3B0eXBTgNp6c431IfOOEsdvkoOh1kwoYcQgbg4MXw6beOltysX4e8fFWsiRkc2nvvRW9ir9kHDm49MkBLqaDjTqOkYKNMiurFW+gozpr/lUW15QqT6v68RYe0zRdtwGZqeLzX2LVuukGwCg4AISzswrrYHNV7vQLcbaUhPgIl0D+gILYT9TJgAEK4YHW+bFRcY+cgUFoLQqQayECMlctKoLOE69nIYOc/hDr9uih1wxrQ/yL0NJvQCohSPyoyLF9b2EuIGhQVp05XP7FzlTxhYvGO/DtO08ec85+bTfVBMV6eeY4MS3ZU+1z7ObD7Pf29YjyTehN2Dan6w1g2rBk5MoA/9nDocSlk4pbFpsYSFmVHsDiAOFje3+iY4ftVDKunKYWMhwRVBjAREOByBagmRau0cLEcElpf4hX5f978GoxSGIsiKoDAlXX+ICDOWC1/EXhEEmBR1gL0QJgiVviNyLfGJlZWnPjw6xhhmtHYWTDxBOP5peztyc2PqeKsLsLWzAr7RDTmljb2xhcyBEdWZyZXNuZSAoQi4gU2MuIEluZm9ybWF0aXF1ZSkgPG5pY29sYXMuZHVmcmVzbmVAZ21haWwuY29tPohgBBMRAgAgBQJFlCyOAhsDBgsJCAcDAgQVAggDBBYCAwECHgECF4AACgkQcVMCLawGqBwhLQCgzYlrLBj6KIAZ4gmsfjXD6ZtddT8AoIeGDicVq5WvMHNWign6ApQcZUihtElOaWNvbGFzIER1ZnJlc25lIChCLiBTYy4gSW5mb3JtYXRpcXVlKSA8bmljb2xhcy5kdWZyZXNuZUBjb2xsYWJvcmEuY28udWs+iGIEExECACIFAkuzca8CGwMGCwkIBwMCBhUIAgkKCwQWA
 gMBAh4BAheAAAoJEHFTAi2sBqgcQX8An2By6LDEeMxi4B9hUbpvRnzaaeNqA J9Rox8rfqHZnSErw9bCHiBwvwJZ77QxTmljb2xhcyBEdWZyZXNuZSA8bmljb2xhcy5kdWZyZXNuZUBjb2xsYWJvcmEuY29tPohiBBMRAgAiBQJNzZzPAhsDBgsJCAcDAgYVCAIJCgsEFgIDAQIeAQIXgAAKCRBxUwItrAaoHLlxAKCYAGf4JL7DYDLs/188CPMGuwLypwCfWKc9DorA9f5pyYlD5pQo6SgSoiC0J05pY29sYXMgRHVmcmVzbmUgPG5pY29sYXNAbmR1ZnJlc25lLmNhPohiBBMRAgAiBQJVwNwgAhsDBgsJCAcDAgYVCAIJCgsEFgIDAQIeAQIXgAAKCRBxUwItrAaoHCZ4AJ0QwU6/G4c7h9CkMBT9ZxGLX4KSnQCgq0P7CX7hv/M7HeyfMFZe8t3vAEW0RE5pY29sYXMgRHVmcmVzbmUgKEIuIFNjLiBJbmZvcm1hdGlxdWUpIDxuaWNvbGFzZEBibHVlc3RyZWFrdGVjaC5jb20+iGAEExECACAFAkZjGzoCGwMGCwkIBwMCBBUCCAMEFgIDAQIeAQIXgAAKCRBxUwItrAaoHBl7AJ0d2lrzshMmJaik/EaDEakzEwqgxQCg0JVZMZm9gRfEou1FvinuZxwf/mu0R05pY29sYXMgRHVmcmVzbmUgKEIgU2MuIEluZm9ybWF0aXF1ZSkgPG5pY29sYXMuZHVmcmVzbmVAdXNoZXJicm9va2UuY2E+iGAEExECACAFAkUQN0MCGwMGCwkIBwMCBBUCCAMEFgIDAQIeAQIXgAAKCRBxUwItrAaoHPTnAJ0WGgJJVspoctAvEcI00mtp5WAFGgCgr+E7ItOqZEHAs+xabBgknYZIFPW5Ag0ERRA3UhAIAJ0rxl2HsVg/nSOAUt7U/T/W+RKzVAlD9orCB0pRVvyWNxSr8MHcH
 mWCxykLuB34ouM4GuDVRKfGnqLzJRBfjs7Ax9K2FI3Odund9xpviLCt1jFC0K XL04RebrFT7xjDfocDaSLFvgxMVs/Jr2/ckKPId1oKvgYgt/o+MzUabKyFB8wIvq4GMtj3LoBKLCie2nCaSt7uVUt6q2t5bNWrd3lO6/mWn7YMc5Hsn33H9pS0+9szw6m3dG08eMKNueDlt72QxiYl2rhjzkT4ltKEkFgYBdyrtIj1UO6eX+YXb4E1rCMJrdjBSgqDPK1sWHC7gliy+izr+XTHuFwlfy8gBpsAAwUIAJJNus64gri4HAL632eqVpza83EphX1IuHzLi1LlMnQ9Tm7XKag46NhmJbOByMG33LwBsBdLjjHQSVkYZFWUifq+NWSFC/kqlb72vW8rBAv64+i3QdfxK9FWbweiRsPpvuHjJQuecbPDJpubLaxKbu2aqLCN5LuHXvdQr6KiXwabT+OJ9AJAqHG7q4IEzg4RNUVn9AS6L8bxqMSocjqpWNBCY2efCVd/c6k4Acv6jXu+wDAZEbWXK+71uaUHExhigBYBpiHGrobe32YlTVE/XEIzKKywhm/Hkn5YKWzumLte6xiD9JhKabmD7uqIvLt2twUpz4BdPzj0dvGlSmvFcaaISQQYEQIACQUCRRA3UgIbDAAKCRBxUwItrAaoHJLyAKDeS3AFowM3f1Y3OFU6XRCTKK2ZhwCfT/7P9WDjkkmiq5AfeOiwVlpuHtM=
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.4 (3.50.4-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

Le samedi 20 avril 2024 =C3=A0 13:09 +0800, Jianfeng Liu a =C3=A9crit=C2=A0=
:
> Hi Hugh,
>=20
> 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-vdpu12=
1"
> > compatible isn't exactly correct to use for this entire device.
>=20
> There are five vepu121 cores for jpeg encoding. And Emmanuel is doing wor=
k on
> them[1]. And at the moment the driver doesn=E2=80=99t yet support exposin=
g these cores
> all as a single video node to userspace, so Emmanuel only exposes one sin=
gle
> core.
>=20
> > IMO "rockchip,rk3588-vpu121" would be more appropriate if including bot=
h 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 separa=
te
> > devices. In the vendor DT [0] they are modeled as two devices but they =
share
> > clocks, resets, IOMMU, and a "rockchip,taskqueue-node" value.
>=20
> 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 enc=
oder
> and decoder like rk3399.
>=20
> This vpu121 should be exactly the same as the one in rk3399 which support=
s both
> encoding and decoding. But the current hantro driver has disabled h264 de=
coding

If its exactly the same combo as on rk3399, it have to be combined as they =
share
the same internal memory. You'll notice this strange thing about both being
60fps FHD seperately and 30fps FHD concurrently, this is why.

This leaves me with a feeling our understanding the of HW is far from perfe=
ct,
we should be extra careful and circle back to Rockchip (ping Kever, he'll
translate and CC the right person I suppose). Though, just exposing the dec=
oder,
and ignoring the encoder seems fine in the short term. Userspace will miss-
behave though when we introduce the rkvdec2 decoder.

As we'd like to change from trivial time multiplexing to proper core schedu=
ling
in the long term (for identical cores only), we have to keep in mind of the
possible grouping, which will need to be something deducible from DT. Pleas=
e
keep that in mind when designing DT for this chip.

> 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 rk=
vdec2
> on rk3588 is supported by mainline kernel.
>=20
> At the moment we can keep the compatible string same as the one from rk35=
6x.
> Since there are already jpeg enc cores at 0xfdba0000, we can ignore the o=
ne at
> 0xfdb50000. When rkvdec2 is supported, I will change "rockchip,rk3588-vpu=
121"
> same as "rockchip,rk3399-vpu".
>=20
> And I think changing "rockchip,rk3588-vdpu121" to "rockchip,rk3588-vpu121=
"
> should match the hardware correctly.
>=20
> [1] https://lore.kernel.org/all/20240418141509.2485053-1-linkmauve@linkma=
uve.fr/
>=20
> Best regards,
> Jianfeng