[PATCH v5 0/5] RK3588 VEPU121/VPU121 support

[PATCH v5 0/5] 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
stall 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.

The series is based on Heiko's for-next branch.

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 (1):
  media: hantro: Add RK3588 VEPU121 support

 .../bindings/media/rockchip,rk3568-vepu.yaml  |   5 +-
 .../bindings/media/rockchip-vpu.yaml          |   3 +
 arch/arm64/boot/dts/rockchip/rk3588s.dtsi     | 101 ++++++++++++++++++
 .../media/platform/verisilicon/hantro_drv.c   |  38 +++++++
 4 files changed, 145 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH v5 1/5] 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>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 .../devicetree/bindings/media/rockchip,rk3568-vepu.yaml      | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml b/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
index 9d90d8d0565a..8b9496e6a2bb 100644
--- a/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
+++ b/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
@@ -15,8 +15,9 @@ description:
 
 properties:
   compatible:
-    enum:
-      - rockchip,rk3568-vepu
+    oneOf:
+      - const: rockchip,rk3568-vepu
+      - const: rockchip,rk3588-vepu121
 
   reg:
     maxItems: 1
-- 
2.43.0

[PATCH v5 2/5] 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

[PATCH v5 3/5] media: hantro: Add RK3588 VEPU121 support

[Sebastian Reichel]

Avoid exposing each of the 4 Hantro H1 cores separately to userspace.
For now just expose the first one.

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

diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index 34b123dafd89..b722a20c5fe3 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_vpu_variant, },
 	{ .compatible = "rockchip,rk3588-av1-vpu", .data = &rk3588_vpu981_variant, },
 #endif
 #ifdef CONFIG_VIDEO_HANTRO_IMX8M
@@ -992,6 +993,39 @@ 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)
+{
+	const char *compatible;
+	struct device_node *node;
+	int ret;
+
+	/* Intentionally ignores the fallback strings */
+	ret = of_property_read_string(vpu->dev->of_node, "compatible", &compatible);
+	if (ret)
+		return ret;
+
+	/* first compatible node found from the root node is considered the main core */
+	node = of_find_compatible_node(NULL, NULL, compatible);
+	if (!node)
+		return -EINVAL; /* broken DT? */
+
+	if (vpu->dev->of_node != node) {
+		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 +1045,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

[PATCH v5 4/5] 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..9edbcfe778ca 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 {
 		};
 	};
 
+	jpeg_enc0: 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 = <&jpeg_enc0_mmu>;
+		power-domains = <&power RK3588_PD_VDPU>;
+	};
+
+	jpeg_enc0_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>;
+	};
+
+	jpeg_enc1: 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 = <&jpeg_enc1_mmu>;
+		power-domains = <&power RK3588_PD_VDPU>;
+	};
+
+	jpeg_enc1_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>;
+	};
+
+	jpeg_enc2: 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 = <&jpeg_enc2_mmu>;
+		power-domains = <&power RK3588_PD_VDPU>;
+	};
+
+	jpeg_enc2_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>;
+	};
+
+	jpeg_enc3: 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 = <&jpeg_enc3_mmu>;
+		power-domains = <&power RK3588_PD_VDPU>;
+	};
+
+	jpeg_enc3_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

[PATCH v5 5/5] 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 9edbcfe778ca..e7e1b456b9b9 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
@@ -1239,6 +1239,27 @@ jpeg_enc3_mmu: iommu@fdbac800 {
 		#iommu-cells = <0>;
 	};
 
+	vpu: 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 = <&vpu_mmu>;
+		power-domains = <&power RK3588_PD_VDPU>;
+	};
+
+	vpu_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>;
+	};
+
 	av1d: video-codec@fdc70000 {
 		compatible = "rockchip,rk3588-av1-vpu";
 		reg = <0x0 0xfdc70000 0x0 0x800>;
-- 
2.43.0

Re: [PATCH v5 3/5] media: hantro: Add RK3588 VEPU121 support

[Heiko Stübner]

Am Mittwoch, 12. Juni 2024, 19:15:43 CEST schrieb Sebastian Reichel:
> Avoid exposing each of the 4 Hantro H1 cores separately to userspace.
> For now just expose the first one.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  .../media/platform/verisilicon/hantro_drv.c   | 38 +++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index 34b123dafd89..b722a20c5fe3 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_vpu_variant, },
>  	{ .compatible = "rockchip,rk3588-av1-vpu", .data = &rk3588_vpu981_variant, },
>  #endif
>  #ifdef CONFIG_VIDEO_HANTRO_IMX8M
> @@ -992,6 +993,39 @@ 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)
> +{
> +	const char *compatible;
> +	struct device_node *node;
> +	int ret;
> +
> +	/* Intentionally ignores the fallback strings */
> +	ret = of_property_read_string(vpu->dev->of_node, "compatible", &compatible);
> +	if (ret)
> +		return ret;
> +
> +	/* first compatible node found from the root node is considered the main core */
> +	node = of_find_compatible_node(NULL, NULL, compatible);
> +	if (!node)
> +		return -EINVAL; /* broken DT? */
> +
> +	if (vpu->dev->of_node != node) {
> +		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 +1045,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;
> +

I think this might be better as two patches?

As this patch stands, the disable-multicore handling is done for _all_
hantro variants, so part of me wants this to be labeled as such.

The whole reasoning is completely ok, but somehow having this under
the "add rk3588" umbrella feels strange ;-)


Heiko

Re: [PATCH v5 5/5] arm64: dts: rockchip: Add VPU121 support for RK3588

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 1558 bytes --]

Hi,

On Wednesday, 12 June 2024 19:15:45 CEST Sebastian Reichel wrote:
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi index
> 9edbcfe778ca..e7e1b456b9b9 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> @@ -1239,6 +1239,27 @@ jpeg_enc3_mmu: iommu@fdbac800 {
>                 #iommu-cells = <0>;
>         };
> 
> +       vpu: 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 = <&vpu_mmu>;
> +               power-domains = <&power RK3588_PD_VDPU>;
> +       };
> +
> +       vpu_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>;
> +       };
> +
>         av1d: video-codec@fdc70000 {

Shouldn't these nodes come *before* 
jpeg_enc0: video-codec@fdba0000 
As fdb50000 is lower then fdba0000?

Cheers,
  Diederik

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 1/5] media: dt-bindings: rk3568-vepu: Add RK3588 VEPU121

[Rob Herring (Arm)]

On Wed, 12 Jun 2024 19:15:41 +0200, Sebastian Reichel wrote:
> 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>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  .../devicetree/bindings/media/rockchip,rk3568-vepu.yaml      | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml: properties:compatible:oneOf: [{'const': 'rockchip,rk3568-vepu'}, {'const': 'rockchip,rk3588-vepu121'}] should not be valid under {'items': {'propertyNames': {'const': 'const'}, 'required': ['const']}}
	hint: Use 'enum' rather than 'oneOf' + 'const' entries
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240612173213.42827-2-sebastian.reichel@collabora.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v5 4/5] arm64: dts: rockchip: Add VEPU121 to RK3588

[Nicolas Dufresne]

Hi Sebastian,

Le mercredi 12 juin 2024 à 19:15 +0200, Sebastian Reichel a écrit :
> 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..9edbcfe778ca 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 {
>  		};
>  	};
>  
> +	jpeg_enc0: video-codec@fdba0000 {
> +		compatible = "rockchip,rk3588-vepu121";

As discussed earlier, VEPU121 is an modifier Hantro H1 encoder core that also
supports VP8 and H.264 encoding (even though RK vendor kernel only expose them
for jpeg encoding). The compatible follow this idea, shall we change the alias
now ?

Nicolas

> +		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 = <&jpeg_enc0_mmu>;
> +		power-domains = <&power RK3588_PD_VDPU>;
> +	};
> +
> +	jpeg_enc0_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>;
> +	};
> +
> +	jpeg_enc1: 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 = <&jpeg_enc1_mmu>;
> +		power-domains = <&power RK3588_PD_VDPU>;
> +	};
> +
> +	jpeg_enc1_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>;
> +	};
> +
> +	jpeg_enc2: 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 = <&jpeg_enc2_mmu>;
> +		power-domains = <&power RK3588_PD_VDPU>;
> +	};
> +
> +	jpeg_enc2_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>;
> +	};
> +
> +	jpeg_enc3: 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 = <&jpeg_enc3_mmu>;
> +		power-domains = <&power RK3588_PD_VDPU>;
> +	};
> +
> +	jpeg_enc3_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>;

Re: [PATCH v5 1/5] media: dt-bindings: rk3568-vepu: Add RK3588 VEPU121

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2497 bytes --]

Hi,

On Wed, Jun 12, 2024 at 12:26:32PM GMT, Rob Herring (Arm) wrote:
> On Wed, 12 Jun 2024 19:15:41 +0200, Sebastian Reichel wrote:
> > 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>
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> >  .../devicetree/bindings/media/rockchip,rk3568-vepu.yaml      | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml: properties:compatible:oneOf: [{'const': 'rockchip,rk3568-vepu'}, {'const': 'rockchip,rk3588-vepu121'}] should not be valid under {'items': {'propertyNames': {'const': 'const'}, 'required': ['const']}}
> 	hint: Use 'enum' rather than 'oneOf' + 'const' entries
> 	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240612173213.42827-2-sebastian.reichel@collabora.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.

oops. That's on me for not doing another test and doing something
stupid. I obviously wanted this and didn't recheck the bindings
after dropping the fallback compatible.

enum:
  - rockchip,rk3568-vepu
  - rockchip,rk3588-vepu121

I will change it in v6 if people are fine with this solution.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 3/5] media: hantro: Add RK3588 VEPU121 support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3740 bytes --]

Hi,

On Wed, Jun 12, 2024 at 08:08:51PM GMT, Heiko Stübner wrote:
> Am Mittwoch, 12. Juni 2024, 19:15:43 CEST schrieb Sebastian Reichel:
> > Avoid exposing each of the 4 Hantro H1 cores separately to userspace.
> > For now just expose the first one.
> > 
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> >  .../media/platform/verisilicon/hantro_drv.c   | 38 +++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> > 
> > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> > index 34b123dafd89..b722a20c5fe3 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_vpu_variant, },
> >  	{ .compatible = "rockchip,rk3588-av1-vpu", .data = &rk3588_vpu981_variant, },
> >  #endif
> >  #ifdef CONFIG_VIDEO_HANTRO_IMX8M
> > @@ -992,6 +993,39 @@ 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)
> > +{
> > +	const char *compatible;
> > +	struct device_node *node;
> > +	int ret;
> > +
> > +	/* Intentionally ignores the fallback strings */
> > +	ret = of_property_read_string(vpu->dev->of_node, "compatible", &compatible);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* first compatible node found from the root node is considered the main core */
> > +	node = of_find_compatible_node(NULL, NULL, compatible);
> > +	if (!node)
> > +		return -EINVAL; /* broken DT? */
> > +
> > +	if (vpu->dev->of_node != node) {
> > +		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 +1045,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;
> > +
> 
> I think this might be better as two patches?
> 
> As this patch stands, the disable-multicore handling is done for _all_
> hantro variants, so part of me wants this to be labeled as such.
> 
> The whole reasoning is completely ok, but somehow having this under
> the "add rk3588" umbrella feels strange ;-)

I can do that, but the 'rockchip,rk3588-vepu121' part is only needed
because of the multicore handling. If the kernel already had this bit
in the past, the RK3568 compatible could be used for RK3588 (as a
fallback compatible), just like for VPU121.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 4/5] arm64: dts: rockchip: Add VEPU121 to RK3588

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 4951 bytes --]

Hi,

On Wed, Jun 12, 2024 at 04:20:57PM GMT, Nicolas Dufresne wrote:
> Hi Sebastian,
> 
> Le mercredi 12 juin 2024 à 19:15 +0200, Sebastian Reichel a écrit :
> > 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..9edbcfe778ca 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 {
> >  		};
> >  	};
> >  
> > +	jpeg_enc0: video-codec@fdba0000 {
> > +		compatible = "rockchip,rk3588-vepu121";
> 
> As discussed earlier, VEPU121 is an modifier Hantro H1 encoder core that also
> supports VP8 and H.264 encoding (even though RK vendor kernel only expose them
> for jpeg encoding). The compatible follow this idea, shall we change the alias
> now?

Makes sense. Do you have any preference for the alias Heiko?
Maybe vepu121_0 / vepu121_0_mmu?

-- Sebastian

> 
> Nicolas
> 
> > +		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 = <&jpeg_enc0_mmu>;
> > +		power-domains = <&power RK3588_PD_VDPU>;
> > +	};
> > +
> > +	jpeg_enc0_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>;
> > +	};
> > +
> > +	jpeg_enc1: 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 = <&jpeg_enc1_mmu>;
> > +		power-domains = <&power RK3588_PD_VDPU>;
> > +	};
> > +
> > +	jpeg_enc1_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>;
> > +	};
> > +
> > +	jpeg_enc2: 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 = <&jpeg_enc2_mmu>;
> > +		power-domains = <&power RK3588_PD_VDPU>;
> > +	};
> > +
> > +	jpeg_enc2_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>;
> > +	};
> > +
> > +	jpeg_enc3: 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 = <&jpeg_enc3_mmu>;
> > +		power-domains = <&power RK3588_PD_VDPU>;
> > +	};
> > +
> > +	jpeg_enc3_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>;
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 5/5] arm64: dts: rockchip: Add VPU121 support for RK3588

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1767 bytes --]

Hi,

On Wed, Jun 12, 2024 at 08:10:30PM GMT, Diederik de Haas wrote:
> On Wednesday, 12 June 2024 19:15:45 CEST Sebastian Reichel wrote:
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi index
> > 9edbcfe778ca..e7e1b456b9b9 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > @@ -1239,6 +1239,27 @@ jpeg_enc3_mmu: iommu@fdbac800 {
> >                 #iommu-cells = <0>;
> >         };
> > 
> > +       vpu: 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 = <&vpu_mmu>;
> > +               power-domains = <&power RK3588_PD_VDPU>;
> > +       };
> > +
> > +       vpu_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>;
> > +       };
> > +
> >         av1d: video-codec@fdc70000 {
> 
> Shouldn't these nodes come *before* 
> jpeg_enc0: video-codec@fdba0000 
> As fdb50000 is lower then fdba0000?

Of course. I will fix that in v6.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 1/5] media: dt-bindings: rk3568-vepu: Add RK3588 VEPU121

[Krzysztof Kozlowski]

On 13/06/2024 00:20, Sebastian Reichel wrote:
> oops. That's on me for not doing another test and doing something
> stupid. I obviously wanted this and didn't recheck the bindings
> after dropping the fallback compatible.
> 
> enum:
>   - rockchip,rk3568-vepu
>   - rockchip,rk3588-vepu121
> 
> I will change it in v6 if people are fine with this solution.

Ack

Best regards,
Krzysztof

Re: [PATCH v5 3/5] media: hantro: Add RK3588 VEPU121 support

[Heiko Stübner]

Am Donnerstag, 13. Juni 2024, 00:44:38 CEST schrieb Sebastian Reichel:
> Hi,
> 
> On Wed, Jun 12, 2024 at 08:08:51PM GMT, Heiko Stübner wrote:
> > Am Mittwoch, 12. Juni 2024, 19:15:43 CEST schrieb Sebastian Reichel:
> > > Avoid exposing each of the 4 Hantro H1 cores separately to userspace.
> > > For now just expose the first one.
> > > 
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > ---
> > >  .../media/platform/verisilicon/hantro_drv.c   | 38 +++++++++++++++++++
> > >  1 file changed, 38 insertions(+)
> > > 
> > > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> > > index 34b123dafd89..b722a20c5fe3 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_vpu_variant, },
> > >  	{ .compatible = "rockchip,rk3588-av1-vpu", .data = &rk3588_vpu981_variant, },
> > >  #endif
> > >  #ifdef CONFIG_VIDEO_HANTRO_IMX8M
> > > @@ -992,6 +993,39 @@ 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)
> > > +{
> > > +	const char *compatible;
> > > +	struct device_node *node;
> > > +	int ret;
> > > +
> > > +	/* Intentionally ignores the fallback strings */
> > > +	ret = of_property_read_string(vpu->dev->of_node, "compatible", &compatible);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* first compatible node found from the root node is considered the main core */
> > > +	node = of_find_compatible_node(NULL, NULL, compatible);
> > > +	if (!node)
> > > +		return -EINVAL; /* broken DT? */
> > > +
> > > +	if (vpu->dev->of_node != node) {
> > > +		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 +1045,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;
> > > +
> > 
> > I think this might be better as two patches?
> > 
> > As this patch stands, the disable-multicore handling is done for _all_
> > hantro variants, so part of me wants this to be labeled as such.
> > 
> > The whole reasoning is completely ok, but somehow having this under
> > the "add rk3588" umbrella feels strange ;-)
> 
> I can do that, but the 'rockchip,rk3588-vepu121' part is only needed
> because of the multicore handling. If the kernel already had this bit
> in the past, the RK3568 compatible could be used for RK3588 (as a
> fallback compatible), just like for VPU121.

I meant, you're doing hantro_disable_multicore() here also for everyone
else (i.MX etc), hence I'd like that to be a separate commit in this
series like:

----- 8< ------
media: hantro: Disable multi-core handling for the time being

The VSI doc for the Hantro codec describes the grouping of up to 4 instances.
The kernel currently doesn't handle multi-core processing .... foo bar ....
----- 8< ------

And then add rk3588 support on top of that.


Heiko