[PATCH v7 0/6] RK3588 VEPU121/VPU121 support

[PATCH v7 0/6] RK3588 VEPU121/VPU121 support

[Sebastian Reichel]

Hi,

This series enables Hantro support for RK3588. It is based on these two
previous series from Emmanuel Gil Peyrot and Jianfeng Liu, which looked
stalled to me. Considering the full driver is already upstream, I think
this low hanging fruit should be enabled in 6.11:

 * https://lore.kernel.org/all/20240316071100.2419369-1-liujianfeng1994@gmail.com/
 * https://lore.kernel.org/linux-rockchip/20240412151515.837824-1-linkmauve@linkmauve.fr/

Their series got some feedback from Nicolas Dufresne, that there should be a
plan how multi-core processing will be handled once it is supported in the
kernel. I had a look (and internal discussion with Nicolas) and came up with a
patch, which allows describing all the Hantro IP in DT. The driver will only
probe for the first instance. This involves dropping the RK3568 compatible
for the VEPU121, so that only kernels with the driver change will try to
handle these IP. Once the kernel is capable of multi-core support, the same
technique to disable cores 1-3 can be used to combine them all into one
cluster.

We also discussed, if they should be described as a cluster (e.g. by creating
some kind of virtual bus for the 4 encoders in DT). Apparently the VSI doc
describes the grouping of up to 4 instances. But there is no obvious reason
why only these groups can be used as a cluster. It seems that even the 5th
encoder from the combo VPU121 could be used together with the other clustered
cores in theory. In practice this is probably a bad idea because of the shared
cache of that encoder. Since that is handled with a different compatible, this
can be thought about at a later point of time and handled in the kernel. Thus
no special cluster description is needed in DT.

Changes since PATCHv6:
 * Collected Acked-by for RK3588 VEPU121 DT binding from Conor Dooley
 * Fix resource leak of DT node in hantro_disable_multicore()
 * Support disabled nodes in hantro_disable_multicore()
 * Use correct match data (RK3568 VEPU instead of VPU) for RK3588 VEPU121

Changes since PATCHv5:
 * Fix binding for vepu121 (use enum)
 * split hantro driver patch (multicore / vepu121 compatible)
 * move video-codec@fdb50000 node to correct position
 * change "jpeg_enc*" alias to "vepu121_*"
 * change "vpu_*" alias to "vpu121_*" (to be consistent)

Changes since PATCHv3 (VEPU121) / PATCHv4 (VPU121)
 * combine both patchsets, since there is some overleap
 * add patch to disable multi-core handling in the hantro driver
 * drop the RK3568 fallback compatible for VEPU (see above for the reason)
 * describe all RK3588 VEPU cores (possible because of driver change)

Greetings,

-- Sebastian

Emmanuel Gil Peyrot (2):
  media: dt-bindings: rk3568-vepu: Add RK3588 VEPU121
  arm64: dts: rockchip: Add VEPU121 to RK3588

Jianfeng Liu (2):
  media: dt-bindings: rockchip-vpu: Add RK3588 VPU121
  arm64: dts: rockchip: Add VPU121 support for RK3588

Sebastian Reichel (2):
  media: hantro: Disable multicore support
  media: hantro: Add RK3588 VEPU121

 .../bindings/media/rockchip,rk3568-vepu.yaml  |   1 +
 .../bindings/media/rockchip-vpu.yaml          |   3 +
 arch/arm64/boot/dts/rockchip/rk3588s.dtsi     | 101 ++++++++++++++++++
 .../media/platform/verisilicon/hantro_drv.c   |  48 +++++++++
 4 files changed, 153 insertions(+)

-- 
2.43.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v7 1/6] media: dt-bindings: rk3568-vepu: Add RK3588 VEPU121

[Sebastian Reichel]

From: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>

This encoder-only device is present four times on this SoC, and should
support everything the rk3568 vepu supports (so JPEG, H.264 and VP8
encoding). No fallback compatible has been added, since the operating
systems might already support RK3568 VEPU and want to avoid registering
four of them separately considering they can be used as a cluster.

Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 .../devicetree/bindings/media/rockchip,rk3568-vepu.yaml          | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml b/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
index 9d90d8d0565a..947ad699cc5e 100644
--- a/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
+++ b/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
@@ -17,6 +17,7 @@ properties:
   compatible:
     enum:
       - rockchip,rk3568-vepu
+      - rockchip,rk3588-vepu121
 
   reg:
     maxItems: 1
-- 
2.43.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v7 2/6] media: dt-bindings: rockchip-vpu: Add RK3588 VPU121

[Sebastian Reichel]

From: Jianfeng Liu <liujianfeng1994@gmail.com>

RK3588 has four Hantro H1 VEPUs (encoder-only) modules and one combined
Hantro H1/G1 VPU (decoder and encoder). These are not described as
separate IP, since they are sharing an internal cache. This adds the
RK3588 specific compatible string for the combined VPU, which seems to
be identical to the version found in the RK3568.

Signed-off-by: Jianfeng Liu <liujianfeng1994@gmail.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 Documentation/devicetree/bindings/media/rockchip-vpu.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/rockchip-vpu.yaml b/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
index c57e1f488895..2710bb2fb0d1 100644
--- a/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
+++ b/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
@@ -31,6 +31,9 @@ properties:
       - items:
           - const: rockchip,rk3228-vpu
           - const: rockchip,rk3399-vpu
+      - items:
+          - const: rockchip,rk3588-vpu121
+          - const: rockchip,rk3568-vpu
 
   reg:
     maxItems: 1
-- 
2.43.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v7 3/6] media: hantro: Disable multicore support

[Sebastian Reichel]

Avoid exposing equal Hantro video codecs to userspace. Equal video
codecs allow scheduling work between the cores. For that kernel support
is required, which does not yet exist. Until that is implemented avoid
exposing each core separately to userspace so that multicore can be
added in the future without breaking userspace ABI.

This was written with Rockchip RK3588 in mind (which has 4 Hantro H1
cores), but applies to all SoCs.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 .../media/platform/verisilicon/hantro_drv.c   | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index 34b123dafd89..748187623990 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -992,6 +992,49 @@ static const struct media_device_ops hantro_m2m_media_ops = {
 	.req_queue = v4l2_m2m_request_queue,
 };
 
+/*
+ * Some SoCs, like RK3588 have multiple identical Hantro cores, but the
+ * kernel is currently missing support for multi-core handling. Exposing
+ * separate devices for each core to userspace is bad, since that does
+ * not allow scheduling tasks properly (and creates ABI). With this workaround
+ * the driver will only probe for the first core and early exit for the other
+ * cores. Once the driver gains multi-core support, the same technique
+ * for detecting the main core can be used to cluster all cores together.
+ */
+static int hantro_disable_multicore(struct hantro_dev *vpu)
+{
+	struct device_node *node = NULL;
+	const char *compatible;
+	bool is_main_core;
+	int ret;
+
+	/* Intentionally ignores the fallback strings */
+	ret = of_property_read_string(vpu->dev->of_node, "compatible", &compatible);
+	if (ret)
+		return ret;
+
+	/* The first compatible and available node found is considered the main core */
+	do {
+		node = of_find_compatible_node(node, NULL, compatible);
+		if (of_device_is_available(node))
+			break;
+	} while (node);
+
+	if (!node)
+		return -EINVAL;
+
+	is_main_core = (vpu->dev->of_node == node);
+
+	of_node_put(node);
+
+	if (!is_main_core) {
+		dev_info(vpu->dev, "missing multi-core support, ignoring this instance\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
 static int hantro_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
@@ -1011,6 +1054,10 @@ static int hantro_probe(struct platform_device *pdev)
 	match = of_match_node(of_hantro_match, pdev->dev.of_node);
 	vpu->variant = match->data;
 
+	ret = hantro_disable_multicore(vpu);
+	if (ret)
+		return ret;
+
 	/*
 	 * Support for nxp,imx8mq-vpu is kept for backwards compatibility
 	 * but it's deprecated. Please update your DTS file to use
-- 
2.43.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v7 4/6] media: hantro: Add RK3588 VEPU121

[Sebastian Reichel]

RK3588 handling is exactly the same as RK3568. This is not
handled using fallback compatibles to avoid exposing multiple
video devices on kernels not having the multicore disable
patch.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/media/platform/verisilicon/hantro_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index 748187623990..05bbac853c4f 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -722,6 +722,7 @@ static const struct of_device_id of_hantro_match[] = {
 	{ .compatible = "rockchip,rk3399-vpu", .data = &rk3399_vpu_variant, },
 	{ .compatible = "rockchip,rk3568-vepu", .data = &rk3568_vepu_variant, },
 	{ .compatible = "rockchip,rk3568-vpu", .data = &rk3568_vpu_variant, },
+	{ .compatible = "rockchip,rk3588-vepu121", .data = &rk3568_vepu_variant, },
 	{ .compatible = "rockchip,rk3588-av1-vpu", .data = &rk3588_vpu981_variant, },
 #endif
 #ifdef CONFIG_VIDEO_HANTRO_IMX8M
-- 
2.43.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v7 5/6] arm64: dts: rockchip: Add VEPU121 to RK3588

[Sebastian Reichel]

From: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>

RK3588 has 4 Hantro G1 encoder-only cores. They are all independent IP,
but can be used as a cluster (i.e. sharing work between the cores).
These cores are called VEPU121 in the TRM. The TRM describes one more
VEPU121, but that is combined with a Hantro H1. That one will be handled
using the VPU binding instead.

Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 80 +++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
index 6ac5ac8b48ab..dd85d4e55922 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
@@ -1159,6 +1159,86 @@ power-domain@RK3588_PD_SDMMC {
 		};
 	};
 
+	vepu121_0: video-codec@fdba0000 {
+		compatible = "rockchip,rk3588-vepu121";
+		reg = <0x0 0xfdba0000 0x0 0x800>;
+		interrupts = <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru ACLK_JPEG_ENCODER0>, <&cru HCLK_JPEG_ENCODER0>;
+		clock-names = "aclk", "hclk";
+		iommus = <&vepu121_0_mmu>;
+		power-domains = <&power RK3588_PD_VDPU>;
+	};
+
+	vepu121_0_mmu: iommu@fdba0800 {
+		compatible = "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";
+		reg = <0x0 0xfdba0800 0x0 0x40>;
+		interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru ACLK_JPEG_ENCODER0>, <&cru HCLK_JPEG_ENCODER0>;
+		clock-names = "aclk", "iface";
+		power-domains = <&power RK3588_PD_VDPU>;
+		#iommu-cells = <0>;
+	};
+
+	vepu121_1: video-codec@fdba4000 {
+		compatible = "rockchip,rk3588-vepu121";
+		reg = <0x0 0xfdba4000 0x0 0x800>;
+		interrupts = <GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru ACLK_JPEG_ENCODER1>, <&cru HCLK_JPEG_ENCODER1>;
+		clock-names = "aclk", "hclk";
+		iommus = <&vepu121_1_mmu>;
+		power-domains = <&power RK3588_PD_VDPU>;
+	};
+
+	vepu121_1_mmu: iommu@fdba4800 {
+		compatible = "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";
+		reg = <0x0 0xfdba4800 0x0 0x40>;
+		interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru ACLK_JPEG_ENCODER1>, <&cru HCLK_JPEG_ENCODER1>;
+		clock-names = "aclk", "iface";
+		power-domains = <&power RK3588_PD_VDPU>;
+		#iommu-cells = <0>;
+	};
+
+	vepu121_2: video-codec@fdba8000 {
+		compatible = "rockchip,rk3588-vepu121";
+		reg = <0x0 0xfdba8000 0x0 0x800>;
+		interrupts = <GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru ACLK_JPEG_ENCODER2>, <&cru HCLK_JPEG_ENCODER2>;
+		clock-names = "aclk", "hclk";
+		iommus = <&vepu121_2_mmu>;
+		power-domains = <&power RK3588_PD_VDPU>;
+	};
+
+	vepu121_2_mmu: iommu@fdba8800 {
+		compatible = "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";
+		reg = <0x0 0xfdba8800 0x0 0x40>;
+		interrupts = <GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru ACLK_JPEG_ENCODER2>, <&cru HCLK_JPEG_ENCODER2>;
+		clock-names = "aclk", "iface";
+		power-domains = <&power RK3588_PD_VDPU>;
+		#iommu-cells = <0>;
+	};
+
+	vepu121_3: video-codec@fdbac000 {
+		compatible = "rockchip,rk3588-vepu121";
+		reg = <0x0 0xfdbac000 0x0 0x800>;
+		interrupts = <GIC_SPI 128 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru ACLK_JPEG_ENCODER3>, <&cru HCLK_JPEG_ENCODER3>;
+		clock-names = "aclk", "hclk";
+		iommus = <&vepu121_3_mmu>;
+		power-domains = <&power RK3588_PD_VDPU>;
+	};
+
+	vepu121_3_mmu: iommu@fdbac800 {
+		compatible = "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";
+		reg = <0x0 0xfdbac800 0x0 0x40>;
+		interrupts = <GIC_SPI 127 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru ACLK_JPEG_ENCODER3>, <&cru HCLK_JPEG_ENCODER3>;
+		clock-names = "aclk", "iface";
+		power-domains = <&power RK3588_PD_VDPU>;
+		#iommu-cells = <0>;
+	};
+
 	av1d: video-codec@fdc70000 {
 		compatible = "rockchip,rk3588-av1-vpu";
 		reg = <0x0 0xfdc70000 0x0 0x800>;
-- 
2.43.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Sebastian Reichel]

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(+)

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>;
-- 
2.43.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Jianfeng Liu]

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Nicolas Dufresne]

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Jianfeng Liu]

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Nicolas Dufresne]

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip