diff mbox series

[v7,6/6] arm64: dts: rockchip: Add VPU121 support for RK3588

Message ID 20240618183816.77597-7-sebastian.reichel@collabora.com (mailing list archive)
State New
Headers show
Series RK3588 VEPU121/VPU121 support | expand

Commit Message

Sebastian Reichel June 18, 2024, 6:18 p.m. UTC
From: Jianfeng Liu <liujianfeng1994@gmail.com>

Enable Hantro G1 video decoder in RK3588's devicetree.

Tested with FFmpeg v4l2_request code taken from [1]
with MPEG2, H.264 and VP8 samples.

[1] https://github.com/LibreELEC/LibreELEC.tv/blob/master/packages/multimedia/ffmpeg/patches/v4l2-request/ffmpeg-001-v4l2-request.patch

Signed-off-by: Jianfeng Liu <liujianfeng1994@gmail.com>
Tested-by: Hugh Cole-Baker <sigmaris@gmail.com>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Jianfeng Liu June 21, 2024, 9:22 a.m. UTC | #1
Hi Sebastian,

Detlev is working on rkvdec2 and gstreamer can't deal with two h264
stateless decoders. So it's better to disable h264 decoding feature of
this vpu121, just like what we have done for rk3399. If your multicore
patch can handle the jpeg enc node at fdb50000 with other VEPU121 nodes
properly, we can just use compatible string "rockchip,rk3399-vpu" instead
of "rockchip,rk3568-vpu".

Best regards,
Jianfeng
Nicolas Dufresne June 26, 2024, 5:46 p.m. UTC | #2
Hi Jianfeng,

Le vendredi 21 juin 2024 à 17:22 +0800, Jianfeng Liu a écrit :
> Hi Sebastian,
> 
> Detlev is working on rkvdec2 and gstreamer can't deal with two h264

Just to clarify, since you are right that it won't work well with GStreamer. It
does work with multiple decoders (it exposes them all), it is simply that it
will randomly pick one when decoding, and it may not pick the best one.

> stateless decoders. So it's better to disable h264 decoding feature of
> this vpu121, just like what we have done for rk3399. If your multicore
> patch can handle the jpeg enc node at fdb50000 with other VEPU121 nodes
> properly, we can just use compatible string "rockchip,rk3399-vpu" instead
> of "rockchip,rk3568-vpu".

In the long term, I'd like to stop having to do "like downstream" and expose
them all. I believe the fix is fairly straightforward in GStreamer. We need to
expose in the generated element the width/height ranges, and for H.264 the
supported profiles and level. With that, we at least won't randomly fail at
decoding 4K, and it should be good enough.

For RK3588, which is a new SoC, its not a problem to upstream something that
does not work with existing userspace. It would only be a regression if we where
to enable VDPU121 on RK3399, as now updating linux would cause bugs with
existing userspace.

For users, it would be best if we get this sorted out in GStreamer by the time
we have a second decoder. Note that I have some vacation coming up this month,
so there might be extra delays. Yet, its logical to merge this (the "worst"
decoder) first, since then randomly picking a better one won't be a regression.

Nicolas
Jianfeng Liu June 27, 2024, 8:13 a.m. UTC | #3
Hi Nicolas,

On Wed, 26 Jun 2024 13:46:03 -0400, Nicolas Dufresne wrote:
>Just to clarify, since you are right that it won't work well with GStreamer. It
>does work with multiple decoders (it exposes them all), it is simply that it
>will randomly pick one when decoding, and it may not pick the best one.

I have tested rkvdec2 and vpu121 with gstreamer 1.24.2 on rk356x to decode
a 4K video, and gstreamer always fall with error:
"v4l2slh264dec0: Failed to configure H264 decoder".
I guess that's because 1080p vpu is at fdea0000 which is always
initialized earlier than rkvdec2 at fdf80200, so gstreamer will always
choose the 1080p decoder.

>In the long term, I'd like to stop having to do "like downstream" and expose
>them all. I believe the fix is fairly straightforward in GStreamer. We need to
>expose in the generated element the width/height ranges, and for H.264 the
>supported profiles and level. With that, we at least won't randomly fail at
>decoding 4K, and it should be good enough.

Not only gstreamer, chromium also has similar issue. Chromium will only
check video resolution globally before starting to use one decoder: if
there is a 4K decoder detected before, it will mark 4K resolution as
supported. But when decoding videos, it will choose the first decoder
supporting profile like H264. So chromium may use a 1080p decoder to
decode a 4K video.

Chromium's code about v4l2 is complicated for me. I may create a bug about
it. But chrome os doesn't support devices with multi v4l2 decoders like
rockchip's socs, I don't know if they have the motion to fix it quickly.

>For RK3588, which is a new SoC, its not a problem to upstream something that
>does not work with existing userspace. It would only be a regression if we where
>to enable VDPU121 on RK3399, as now updating linux would cause bugs with
>existing userspace.

There is an old soc just like RK3399: RK3328, which also has a 1080p
hantro h264 decoder and a 4K rkvdec h264 decoder. I guess less people care
about its mainline decoding with gstreamer/chromium so it still has 1080p
decoder enabled.

>For users, it would be best if we get this sorted out in GStreamer by the time
>we have a second decoder. Note that I have some vacation coming up this month,
>so there might be extra delays. Yet, its logical to merge this (the "worst"
>decoder) first, since then randomly picking a better one won't be a regression.

Happy vacation days! I will also take a look at chromium's code to see if
I can fix it.

Best regards,
Jianfeng
Nicolas Dufresne June 27, 2024, 1:53 p.m. UTC | #4
Le jeudi 27 juin 2024 à 16:13 +0800, Jianfeng Liu a écrit :
> Hi Nicolas,
> 
> On Wed, 26 Jun 2024 13:46:03 -0400, Nicolas Dufresne wrote:
> > Just to clarify, since you are right that it won't work well with GStreamer. It
> > does work with multiple decoders (it exposes them all), it is simply that it
> > will randomly pick one when decoding, and it may not pick the best one.
> 
> I have tested rkvdec2 and vpu121 with gstreamer 1.24.2 on rk356x to decode
> a 4K video, and gstreamer always fall with error:
> "v4l2slh264dec0: Failed to configure H264 decoder".
> I guess that's because 1080p vpu is at fdea0000 which is always
> initialized earlier than rkvdec2 at fdf80200, so gstreamer will always
> choose the 1080p decoder.

I've never done any research, but that is plausible.

- Probe happen in address order, since DT are in address order
- Media notes are assigned in probe order
- GStreamer register the element in the same order in its registry
- In adsence of a rank or capabilities to differentiate, the probe order is
maintained.

> 
> > In the long term, I'd like to stop having to do "like downstream" and expose
> > them all. I believe the fix is fairly straightforward in GStreamer. We need to
> > expose in the generated element the width/height ranges, and for H.264 the
> > supported profiles and level. With that, we at least won't randomly fail at
> > decoding 4K, and it should be good enough.
> 
> Not only gstreamer, chromium also has similar issue. Chromium will only
> check video resolution globally before starting to use one decoder: if
> there is a 4K decoder detected before, it will mark 4K resolution as
> supported. But when decoding videos, it will choose the first decoder
> supporting profile like H264. So chromium may use a 1080p decoder to
> decode a 4K video.
> 
> Chromium's code about v4l2 is complicated for me. I may create a bug about
> it. But chrome os doesn't support devices with multi v4l2 decoders like
> rockchip's socs, I don't know if they have the motion to fix it quickly.

That's an interesting bug, which makes its more of less equal to GStreamer
"unimplemented behaviour". Filing a bug is best indeed, ChromeOS team, who
maintains this, is probably unaware as they don't have any SoC with multiple
decoders. Even on PC side, their Chromebooks only ever have a single GPU, I
haven't heard about any eGPU support either.

> 
> > For RK3588, which is a new SoC, its not a problem to upstream something that
> > does not work with existing userspace. It would only be a regression if we where
> > to enable VDPU121 on RK3399, as now updating linux would cause bugs with
> > existing userspace.
> 
> There is an old soc just like RK3399: RK3328, which also has a 1080p
> hantro h264 decoder and a 4K rkvdec h264 decoder. I guess less people care
> about its mainline decoding with gstreamer/chromium so it still has 1080p
> decoder enabled.

What I meant by new/old, is supported mainline or not. But yes, on timeline,
there is many older SoC with dual decoders in the Rockchip line.

> 
> > For users, it would be best if we get this sorted out in GStreamer by the time
> > we have a second decoder. Note that I have some vacation coming up this month,
> > so there might be extra delays. Yet, its logical to merge this (the "worst"
> > decoder) first, since then randomly picking a better one won't be a regression.
> 
> Happy vacation days! I will also take a look at chromium's code to see if
> I can fix it.

Great, let's keep everyone on sync, I'm sure we can come up with something
better then disabling the possibly useful hardware.

Nicolas
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
index dd85d4e55922..c0466982646f 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
@@ -1159,6 +1159,27 @@  power-domain@RK3588_PD_SDMMC {
 		};
 	};
 
+	vpu121: video-codec@fdb50000 {
+		compatible = "rockchip,rk3588-vpu121", "rockchip,rk3568-vpu";
+		reg = <0x0 0xfdb50000 0x0 0x800>;
+		interrupts = <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH 0>;
+		interrupt-names = "vdpu";
+		clocks = <&cru ACLK_VPU>, <&cru HCLK_VPU>;
+		clock-names = "aclk", "hclk";
+		iommus = <&vpu121_mmu>;
+		power-domains = <&power RK3588_PD_VDPU>;
+	};
+
+	vpu121_mmu: iommu@fdb50800 {
+		compatible = "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";
+		reg = <0x0 0xfdb50800 0x0 0x40>;
+		interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH 0>;
+		clock-names = "aclk", "iface";
+		clocks = <&cru ACLK_VPU>, <&cru HCLK_VPU>;
+		power-domains = <&power RK3588_PD_VDPU>;
+		#iommu-cells = <0>;
+	};
+
 	vepu121_0: video-codec@fdba0000 {
 		compatible = "rockchip,rk3588-vepu121";
 		reg = <0x0 0xfdba0000 0x0 0x800>;