[PATCH v2 0/3] Enable JPEG Encoder on RK3566/RK3568

[PATCH v2 0/3] Enable JPEG Encoder on RK3566/RK3568

[Nicolas Frattaroli]

Hello,

the following series adds support for and enables one of the hardware
video encoders on the RK3566 and RK3568 line of SoCs by Rockchip,
initially just for the JPEG format in line with what the kernel supports.

The encoder block is separate from the Hantro decoder instance, as they
are in different power domains and have wildly different memory addresses
as well.

The encoder hardware seemingly also supports VP8 and H.264 in addition
to just JPEG, as is evident from both the downstream vendor stack and the
register listing in the TRM. The hantro driver in Linux, however, does
not yet support encoding these formats.

The first patch modifies the bindings with a new compatible, and adds
the ability to just have a vepu interrupt without a vdpu interrupt.

The second patch makes the actual driver changes to support this variant.

The third and final patch makes the necessary device tree changes for
the rk356x device tree file to add both the node for the encoder and
its MMU.

The series has been tested on a PINE64 Quartz64 Model A with an RK3566
SoC using GStreamer.

Below you'll also find an interdiff against V1, which falsely assumed
this encoder instance only did JPEG.

Regards,
Nicolas Frattaroli

Changes in v2:
 - rename compatible as it's not JPEG only
 - rename device tree nodes as it's not JPEG only
 - reword commits as it's not JPEG only
 - get rid of a whole bunch of redundant struct definitions, as, you
   guessed it, it's not JPEG only

Nicolas Frattaroli (3):
  dt-bindings: media: rockchip-vpu: Add RK3568 VEPU compatible
  media: hantro: Add support for RK356x encoder
  arm64: dts: rockchip: Add Hantro encoder node to rk356x

 .../bindings/media/rockchip-vpu.yaml          |  2 ++
 arch/arm64/boot/dts/rockchip/rk356x.dtsi      | 21 ++++++++++++++++
 drivers/staging/media/hantro/hantro_drv.c     |  1 +
 drivers/staging/media/hantro/hantro_hw.h      |  1 +
 .../staging/media/hantro/rockchip_vpu_hw.c    | 25 +++++++++++++++++++
 5 files changed, 50 insertions(+)

Interdiff against v1:
diff --git a/Documentation/devicetree/bindings/media/rockchip-vpu.yaml b/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
index cd62b44c34c3..4045f107ca4e 100644
--- a/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
+++ b/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
@@ -22,7 +22,7 @@ properties:
           - rockchip,rk3288-vpu
           - rockchip,rk3328-vpu
           - rockchip,rk3399-vpu
-          - rockchip,rk3568-jpeg-vepu
+          - rockchip,rk3568-vepu
           - rockchip,px30-vpu
       - items:
           - const: rockchip,rk3188-vpu
diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
index 276b76d5f3fb..2e3c9e1887e3 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -508,18 +508,18 @@ gpu: gpu@fde60000 {
 		status = "disabled";
 	};
 
-	vepu_jpeg: video-codec@fdee0000 {
-		compatible = "rockchip,rk3568-jpeg-vepu";
+	vepu: video-codec@fdee0000 {
+		compatible = "rockchip,rk3568-vepu";
 		reg = <0x0 0xfdee0000 0x0 0x800>;
 		interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
 		interrupt-names = "vepu";
 		clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
 		clock-names = "aclk", "hclk";
-		iommus = <&vepu_jpeg_mmu>;
+		iommus = <&vepu_mmu>;
 		power-domains = <&power RK3568_PD_RGA>;
 	};
 
-	vepu_jpeg_mmu: iommu@fdee0800 {
+	vepu_mmu: iommu@fdee0800 {
 		compatible = "rockchip,rk3568-iommu";
 		reg = <0x0 0xfdee0800 0x0 0x40>;
 		interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index 3add9babd7bb..0b38b41136e2 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -628,7 +628,7 @@ static const struct of_device_id of_hantro_match[] = {
 	{ .compatible = "rockchip,rk3288-vpu", .data = &rk3288_vpu_variant, },
 	{ .compatible = "rockchip,rk3328-vpu", .data = &rk3328_vpu_variant, },
 	{ .compatible = "rockchip,rk3399-vpu", .data = &rk3399_vpu_variant, },
-	{ .compatible = "rockchip,rk3568-jpeg-vepu", .data = &rk3568_jpeg_vepu_variant, },
+	{ .compatible = "rockchip,rk3568-vepu", .data = &rk3568_vepu_variant, },
 #endif
 #ifdef CONFIG_VIDEO_HANTRO_IMX8M
 	{ .compatible = "nxp,imx8mm-vpu-g1", .data = &imx8mm_vpu_g1_variant, },
diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
index dd7f1edfacf2..b312da654d38 100644
--- a/drivers/staging/media/hantro/hantro_hw.h
+++ b/drivers/staging/media/hantro/hantro_hw.h
@@ -300,7 +300,7 @@ extern const struct hantro_variant rk3066_vpu_variant;
 extern const struct hantro_variant rk3288_vpu_variant;
 extern const struct hantro_variant rk3328_vpu_variant;
 extern const struct hantro_variant rk3399_vpu_variant;
-extern const struct hantro_variant rk3568_jpeg_vepu_variant;
+extern const struct hantro_variant rk3568_vepu_variant;
 extern const struct hantro_variant sama5d4_vdec_variant;
 extern const struct hantro_variant sunxi_vpu_variant;
 
diff --git a/drivers/staging/media/hantro/rockchip_vpu_hw.c b/drivers/staging/media/hantro/rockchip_vpu_hw.c
index 10d3ea92a954..a97a4ea8ede4 100644
--- a/drivers/staging/media/hantro/rockchip_vpu_hw.c
+++ b/drivers/staging/media/hantro/rockchip_vpu_hw.c
@@ -204,43 +204,6 @@ static const struct hantro_fmt rk3399_vpu_dec_fmts[] = {
 	},
 };
 
-static const struct hantro_fmt rk3568_jpeg_vepu_enc_fmts[] = {
-	{
-		.fourcc = V4L2_PIX_FMT_YUV420M,
-		.codec_mode = HANTRO_MODE_NONE,
-		.enc_fmt = ROCKCHIP_VPU_ENC_FMT_YUV420P,
-	},
-	{
-		.fourcc = V4L2_PIX_FMT_NV12M,
-		.codec_mode = HANTRO_MODE_NONE,
-		.enc_fmt = ROCKCHIP_VPU_ENC_FMT_YUV420SP,
-	},
-	{
-		.fourcc = V4L2_PIX_FMT_YUYV,
-		.codec_mode = HANTRO_MODE_NONE,
-		.enc_fmt = ROCKCHIP_VPU_ENC_FMT_YUYV422,
-	},
-	{
-		.fourcc = V4L2_PIX_FMT_UYVY,
-		.codec_mode = HANTRO_MODE_NONE,
-		.enc_fmt = ROCKCHIP_VPU_ENC_FMT_UYVY422,
-	},
-	{
-		.fourcc = V4L2_PIX_FMT_JPEG,
-		.codec_mode = HANTRO_MODE_JPEG_ENC,
-		.max_depth = 2,
-		.header_size = JPEG_HEADER_SIZE,
-		.frmsize = {
-			.min_width = 96,
-			.max_width = 8192,
-			.step_width = MB_DIM,
-			.min_height = 32,
-			.max_height = 8192,
-			.step_height = MB_DIM,
-		},
-	},
-};
-
 static irqreturn_t rockchip_vpu1_vepu_irq(int irq, void *dev_id)
 {
 	struct hantro_dev *vpu = dev_id;
@@ -484,7 +447,7 @@ static const struct hantro_irq rockchip_vpu2_irqs[] = {
 	{ "vdpu", rockchip_vpu2_vdpu_irq },
 };
 
-static const struct hantro_irq rk3568_jpeg_vepu_irqs[] = {
+static const struct hantro_irq rk3568_vepu_irqs[] = {
 	{ "vepu", rockchip_vpu2_vepu_irq },
 };
 
@@ -594,14 +557,14 @@ const struct hantro_variant rk3399_vpu_variant = {
 	.num_clocks = ARRAY_SIZE(rockchip_vpu_clk_names)
 };
 
-const struct hantro_variant rk3568_jpeg_vepu_variant = {
+const struct hantro_variant rk3568_vepu_variant = {
 	.enc_offset = 0x0,
-	.enc_fmts = rk3568_jpeg_vepu_enc_fmts,
-	.num_enc_fmts = ARRAY_SIZE(rk3568_jpeg_vepu_enc_fmts),
+	.enc_fmts = rockchip_vpu_enc_fmts,
+	.num_enc_fmts = ARRAY_SIZE(rockchip_vpu_enc_fmts),
 	.codec = HANTRO_JPEG_ENCODER,
 	.codec_ops = rk3568_jpeg_enc_codec_ops,
-	.irqs = rk3568_jpeg_vepu_irqs,
-	.num_irqs = ARRAY_SIZE(rk3568_jpeg_vepu_irqs),
+	.irqs = rk3568_vepu_irqs,
+	.num_irqs = ARRAY_SIZE(rk3568_vepu_irqs),
 	.init = rockchip_vpu_hw_init,
 	.clk_names = rockchip_vpu_clk_names,
 	.num_clocks = ARRAY_SIZE(rockchip_vpu_clk_names)
-- 
2.36.0

[PATCH v2 1/3] dt-bindings: media: rockchip-vpu: Add RK3568 VEPU compatible

[Nicolas Frattaroli]

The RK3568 and RK3566 have a Hantro VPU node solely dedicated to
encoding. This patch adds a compatible for it, and also allows
the bindings to only come with a vepu interrupt.

Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
---
 Documentation/devicetree/bindings/media/rockchip-vpu.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/rockchip-vpu.yaml b/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
index bacb60a34989..4045f107ca4e 100644
--- a/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
+++ b/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
@@ -22,6 +22,7 @@ properties:
           - rockchip,rk3288-vpu
           - rockchip,rk3328-vpu
           - rockchip,rk3399-vpu
+          - rockchip,rk3568-vepu
           - rockchip,px30-vpu
       - items:
           - const: rockchip,rk3188-vpu
@@ -40,6 +41,7 @@ properties:
   interrupt-names:
     oneOf:
       - const: vdpu
+      - const: vepu
       - items:
           - const: vepu
           - const: vdpu
-- 
2.36.0

[PATCH v2 2/3] media: hantro: Add support for RK356x encoder

[Nicolas Frattaroli]

The RK3566 and RK3568 SoCs come with a small Hantro instance which is
solely dedicated to encoding. This patch adds the necessary structs to
the Hantro driver to allow the JPEG encoder of it to function.

Through some sleuthing through the vendor's MPP source code and after
closer inspection of the TRM, it was determined that the hardware likely
supports VP8 and H.264 as well.

Tested with the following GStreamer command:

gst-launch-1.0 videotestsrc ! v4l2jpegenc ! matroskamux ! \
               filesink location=foo.mkv

Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
---
 drivers/staging/media/hantro/hantro_drv.c     |  1 +
 drivers/staging/media/hantro/hantro_hw.h      |  1 +
 .../staging/media/hantro/rockchip_vpu_hw.c    | 25 +++++++++++++++++++
 3 files changed, 27 insertions(+)

diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index dc768884cb79..0b38b41136e2 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -628,6 +628,7 @@ static const struct of_device_id of_hantro_match[] = {
 	{ .compatible = "rockchip,rk3288-vpu", .data = &rk3288_vpu_variant, },
 	{ .compatible = "rockchip,rk3328-vpu", .data = &rk3328_vpu_variant, },
 	{ .compatible = "rockchip,rk3399-vpu", .data = &rk3399_vpu_variant, },
+	{ .compatible = "rockchip,rk3568-vepu", .data = &rk3568_vepu_variant, },
 #endif
 #ifdef CONFIG_VIDEO_HANTRO_IMX8M
 	{ .compatible = "nxp,imx8mm-vpu-g1", .data = &imx8mm_vpu_g1_variant, },
diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
index ed018e293ba0..b312da654d38 100644
--- a/drivers/staging/media/hantro/hantro_hw.h
+++ b/drivers/staging/media/hantro/hantro_hw.h
@@ -300,6 +300,7 @@ extern const struct hantro_variant rk3066_vpu_variant;
 extern const struct hantro_variant rk3288_vpu_variant;
 extern const struct hantro_variant rk3328_vpu_variant;
 extern const struct hantro_variant rk3399_vpu_variant;
+extern const struct hantro_variant rk3568_vepu_variant;
 extern const struct hantro_variant sama5d4_vdec_variant;
 extern const struct hantro_variant sunxi_vpu_variant;
 
diff --git a/drivers/staging/media/hantro/rockchip_vpu_hw.c b/drivers/staging/media/hantro/rockchip_vpu_hw.c
index 163cf92eafca..a97a4ea8ede4 100644
--- a/drivers/staging/media/hantro/rockchip_vpu_hw.c
+++ b/drivers/staging/media/hantro/rockchip_vpu_hw.c
@@ -417,6 +417,14 @@ static const struct hantro_codec_ops rk3399_vpu_codec_ops[] = {
 	},
 };
 
+static const struct hantro_codec_ops rk3568_jpeg_enc_codec_ops[] = {
+	[HANTRO_MODE_JPEG_ENC] = {
+		.run = rockchip_vpu2_jpeg_enc_run,
+		.reset = rockchip_vpu2_enc_reset,
+		.done = rockchip_vpu2_jpeg_enc_done,
+	},
+};
+
 /*
  * VPU variant.
  */
@@ -439,6 +447,10 @@ static const struct hantro_irq rockchip_vpu2_irqs[] = {
 	{ "vdpu", rockchip_vpu2_vdpu_irq },
 };
 
+static const struct hantro_irq rk3568_vepu_irqs[] = {
+	{ "vepu", rockchip_vpu2_vepu_irq },
+};
+
 static const char * const rk3066_vpu_clk_names[] = {
 	"aclk_vdpu", "hclk_vdpu",
 	"aclk_vepu", "hclk_vepu"
@@ -545,6 +557,19 @@ const struct hantro_variant rk3399_vpu_variant = {
 	.num_clocks = ARRAY_SIZE(rockchip_vpu_clk_names)
 };
 
+const struct hantro_variant rk3568_vepu_variant = {
+	.enc_offset = 0x0,
+	.enc_fmts = rockchip_vpu_enc_fmts,
+	.num_enc_fmts = ARRAY_SIZE(rockchip_vpu_enc_fmts),
+	.codec = HANTRO_JPEG_ENCODER,
+	.codec_ops = rk3568_jpeg_enc_codec_ops,
+	.irqs = rk3568_vepu_irqs,
+	.num_irqs = ARRAY_SIZE(rk3568_vepu_irqs),
+	.init = rockchip_vpu_hw_init,
+	.clk_names = rockchip_vpu_clk_names,
+	.num_clocks = ARRAY_SIZE(rockchip_vpu_clk_names)
+};
+
 const struct hantro_variant px30_vpu_variant = {
 	.enc_offset = 0x0,
 	.enc_fmts = rockchip_vpu_enc_fmts,
-- 
2.36.0

[PATCH v2 3/3] arm64: dts: rockchip: Add Hantro encoder node to rk356x

[Nicolas Frattaroli]

The RK3566 and RK3568 come with a dedicated Hantro instance solely for
encoding. This patch adds a node for this to the device tree, along with
a node for its MMU.

Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
---
 arch/arm64/boot/dts/rockchip/rk356x.dtsi | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
index 7cdef800cb3c..2e3c9e1887e3 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -508,6 +508,27 @@ gpu: gpu@fde60000 {
 		status = "disabled";
 	};
 
+	vepu: video-codec@fdee0000 {
+		compatible = "rockchip,rk3568-vepu";
+		reg = <0x0 0xfdee0000 0x0 0x800>;
+		interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-names = "vepu";
+		clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
+		clock-names = "aclk", "hclk";
+		iommus = <&vepu_mmu>;
+		power-domains = <&power RK3568_PD_RGA>;
+	};
+
+	vepu_mmu: iommu@fdee0800 {
+		compatible = "rockchip,rk3568-iommu";
+		reg = <0x0 0xfdee0800 0x0 0x40>;
+		interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
+		clock-names = "aclk", "iface";
+		power-domains = <&power RK3568_PD_RGA>;
+		#iommu-cells = <0>;
+	};
+
 	sdmmc2: mmc@fe000000 {
 		compatible = "rockchip,rk3568-dw-mshc", "rockchip,rk3288-dw-mshc";
 		reg = <0x0 0xfe000000 0x0 0x4000>;
-- 
2.36.0

Re: [PATCH v2 1/3] dt-bindings: media: rockchip-vpu: Add RK3568 VEPU compatible

[Krzysztof Kozlowski]

On 08/05/2022 22:25, Nicolas Frattaroli wrote:
> The RK3568 and RK3566 have a Hantro VPU node solely dedicated to
> encoding. This patch adds a compatible for it, and also allows
> the bindings to only come with a vepu interrupt.
> 
> Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> ---
>  Documentation/devicetree/bindings/media/rockchip-vpu.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/rockchip-vpu.yaml b/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
> index bacb60a34989..4045f107ca4e 100644
> --- a/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
> +++ b/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
> @@ -22,6 +22,7 @@ properties:
>            - rockchip,rk3288-vpu
>            - rockchip,rk3328-vpu
>            - rockchip,rk3399-vpu
> +          - rockchip,rk3568-vepu
>            - rockchip,px30-vpu
>        - items:
>            - const: rockchip,rk3188-vpu
> @@ -40,6 +41,7 @@ properties:
>    interrupt-names:
>      oneOf:
>        - const: vdpu
> +      - const: vepu

This should be enum (for both lines above) and you should add
allOf:if:then with a constraints which variant can have which interrupts.


Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: media: rockchip-vpu: Add RK3568 VEPU compatible

[Nicolas Frattaroli]

On Montag, 9. Mai 2022 09:25:23 CEST Krzysztof Kozlowski wrote:
> On 08/05/2022 22:25, Nicolas Frattaroli wrote:
> > The RK3568 and RK3566 have a Hantro VPU node solely dedicated to
> > encoding. This patch adds a compatible for it, and also allows
> > the bindings to only come with a vepu interrupt.
> > 
> > Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/media/rockchip-vpu.yaml | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/rockchip-vpu.yaml b/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
> > index bacb60a34989..4045f107ca4e 100644
> > --- a/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
> > +++ b/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
> > @@ -22,6 +22,7 @@ properties:
> >            - rockchip,rk3288-vpu
> >            - rockchip,rk3328-vpu
> >            - rockchip,rk3399-vpu
> > +          - rockchip,rk3568-vepu
> >            - rockchip,px30-vpu
> >        - items:
> >            - const: rockchip,rk3188-vpu
> > @@ -40,6 +41,7 @@ properties:
> >    interrupt-names:
> >      oneOf:
> >        - const: vdpu
> > +      - const: vepu
> 
> This should be enum (for both lines above) and you should add
> allOf:if:then with a constraints which variant can have which interrupts.
> 
> 
> Best regards,
> Krzysztof
> 

So something like this?

  interrupt-names:
    oneOf:
      - enum:
         - vdpu
         - vepu
      - items:
          - const: vepu
          - const: vdpu

What's the difference between a list of consts and an enum here?
I'm not very familiar with dt-schema, my apologies.

Also, since I don't know which of the other variants can have
the encoding interrupt and this wasn't brought up until now, I think
my solution will be to have a check for -vepu in the compatible and in
that case require that only the vepu interrupt is present, if that's
alright with you.

Regards,
Nicolas Frattaroli

Re: [PATCH v2 1/3] dt-bindings: media: rockchip-vpu: Add RK3568 VEPU compatible

[Krzysztof Kozlowski]

On 09/05/2022 11:24, Nicolas Frattaroli wrote:
> On Montag, 9. Mai 2022 09:25:23 CEST Krzysztof Kozlowski wrote:
>> On 08/05/2022 22:25, Nicolas Frattaroli wrote:
>>> The RK3568 and RK3566 have a Hantro VPU node solely dedicated to
>>> encoding. This patch adds a compatible for it, and also allows
>>> the bindings to only come with a vepu interrupt.
>>>
>>> Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
>>> ---
>>>  Documentation/devicetree/bindings/media/rockchip-vpu.yaml | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/rockchip-vpu.yaml b/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
>>> index bacb60a34989..4045f107ca4e 100644
>>> --- a/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
>>> +++ b/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
>>> @@ -22,6 +22,7 @@ properties:
>>>            - rockchip,rk3288-vpu
>>>            - rockchip,rk3328-vpu
>>>            - rockchip,rk3399-vpu
>>> +          - rockchip,rk3568-vepu
>>>            - rockchip,px30-vpu
>>>        - items:
>>>            - const: rockchip,rk3188-vpu
>>> @@ -40,6 +41,7 @@ properties:
>>>    interrupt-names:
>>>      oneOf:
>>>        - const: vdpu
>>> +      - const: vepu
>>
>> This should be enum (for both lines above) and you should add
>> allOf:if:then with a constraints which variant can have which interrupts.
>>
>>
>> Best regards,
>> Krzysztof
>>
> 
> So something like this?
> 
>   interrupt-names:
>     oneOf:
>       - enum:
>          - vdpu
>          - vepu
>       - items:
>           - const: vepu
>           - const: vdpu

Yes

> 
> What's the difference between a list of consts and an enum here?
> I'm not very familiar with dt-schema, my apologies.

The effect is the same, just oneOf is a bit more complicated way to
describe it.

> 
> Also, since I don't know which of the other variants can have
> the encoding interrupt and this wasn't brought up until now, I think
> my solution will be to have a check for -vepu in the compatible and in
> that case require that only the vepu interrupt is present, if that's
> alright with you.

If you meant by adding a "if" case for only rockchip,rk3568-vepu, it's ok.

Best regards,
Krzysztof

Re: [PATCH v2 3/3] arm64: dts: rockchip: Add Hantro encoder node to rk356x

[Ezequiel Garcia]

Hi Nicolas,

On Sun, May 8, 2022 at 5:26 PM Nicolas Frattaroli
<frattaroli.nicolas@gmail.com> wrote:
>
> The RK3566 and RK3568 come with a dedicated Hantro instance solely for
> encoding. This patch adds a node for this to the device tree, along with
> a node for its MMU.
>
> Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> index 7cdef800cb3c..2e3c9e1887e3 100644
> --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> @@ -508,6 +508,27 @@ gpu: gpu@fde60000 {
>                 status = "disabled";
>         };
>
> +       vepu: video-codec@fdee0000 {
> +               compatible = "rockchip,rk3568-vepu";
> +               reg = <0x0 0xfdee0000 0x0 0x800>;
> +               interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> +               interrupt-names = "vepu";

It this block "encoder only" and if so, maybe we should remove the
"interrupt-names" [1]?

The driver is able to handle it. See:

https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/hantro/hantro_drv.c#L962

You might have to adjust the dt-bindings for this.

[1] https://lore.kernel.org/linux-media/20210324151715.GA3070006@robh.at.kernel.org/

Thanks,
Ezequiel

> +               clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> +               clock-names = "aclk", "hclk";
> +               iommus = <&vepu_mmu>;
> +               power-domains = <&power RK3568_PD_RGA>;
> +       };
> +
> +       vepu_mmu: iommu@fdee0800 {
> +               compatible = "rockchip,rk3568-iommu";
> +               reg = <0x0 0xfdee0800 0x0 0x40>;
> +               interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
> +               clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> +               clock-names = "aclk", "iface";
> +               power-domains = <&power RK3568_PD_RGA>;
> +               #iommu-cells = <0>;
> +       };
> +
>         sdmmc2: mmc@fe000000 {
>                 compatible = "rockchip,rk3568-dw-mshc", "rockchip,rk3288-dw-mshc";
>                 reg = <0x0 0xfe000000 0x0 0x4000>;
> --
> 2.36.0
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 3/3] arm64: dts: rockchip: Add Hantro encoder node to rk356x

[Nicolas Frattaroli]

Hi Ezequiel,

On Montag, 9. Mai 2022 16:17:03 CEST Ezequiel Garcia wrote:
> Hi Nicolas,
> 
> On Sun, May 8, 2022 at 5:26 PM Nicolas Frattaroli
> <frattaroli.nicolas@gmail.com> wrote:
> >
> > The RK3566 and RK3568 come with a dedicated Hantro instance solely for
> > encoding. This patch adds a node for this to the device tree, along with
> > a node for its MMU.
> >
> > Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > index 7cdef800cb3c..2e3c9e1887e3 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > @@ -508,6 +508,27 @@ gpu: gpu@fde60000 {
> >                 status = "disabled";
> >         };
> >
> > +       vepu: video-codec@fdee0000 {
> > +               compatible = "rockchip,rk3568-vepu";
> > +               reg = <0x0 0xfdee0000 0x0 0x800>;
> > +               interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> > +               interrupt-names = "vepu";
> 
> It this block "encoder only" and if so, maybe we should remove the
> "interrupt-names" [1]?
> 
> The driver is able to handle it. See:
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/hantro/hantro_drv.c#L962
> 
> You might have to adjust the dt-bindings for this.
> 
> [1] https://lore.kernel.org/linux-media/20210324151715.GA3070006@robh.at.kernel.org/

What the Linux driver can handle should not matter to the device tree;
device trees are independent of drivers and kernels.

What does matter though is to be consistent in the bindings.
interrupt-names is a required property even if there's only a vdpu
interrupt. I modelled my vepu-only binding after this case.

If robh thinks there is no value to having the interrupt show up
as anything other than "default" in /proc/interrupts, then I respectfully
disagree with that opinion and point out that this should have been brought
up when the vdpu-only case in the bindings was made to require
interrupt-names also.

Changing the binding now that there theoretically could be drivers out
in the wild (though I doubt it) that do require interrupt-names, because
the binding told them that this is okay to do, seems unwise to me.

Regards,
Nicolas Frattaroli

> 
> Thanks,
> Ezequiel
> 
> > +               clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> > +               clock-names = "aclk", "hclk";
> > +               iommus = <&vepu_mmu>;
> > +               power-domains = <&power RK3568_PD_RGA>;
> > +       };
> > +
> > +       vepu_mmu: iommu@fdee0800 {
> > +               compatible = "rockchip,rk3568-iommu";
> > +               reg = <0x0 0xfdee0800 0x0 0x40>;
> > +               interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
> > +               clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> > +               clock-names = "aclk", "iface";
> > +               power-domains = <&power RK3568_PD_RGA>;
> > +               #iommu-cells = <0>;
> > +       };
> > +
> >         sdmmc2: mmc@fe000000 {
> >                 compatible = "rockchip,rk3568-dw-mshc", "rockchip,rk3288-dw-mshc";
> >                 reg = <0x0 0xfe000000 0x0 0x4000>;
> > --
> > 2.36.0
> >
> >
> > _______________________________________________
> > Linux-rockchip mailing list
> > Linux-rockchip@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 

Re: [PATCH v2 3/3] arm64: dts: rockchip: Add Hantro encoder node to rk356x

[Ezequiel Garcia]

On Tue, May 10, 2022 at 12:28 PM Nicolas Frattaroli
<frattaroli.nicolas@gmail.com> wrote:
>
> Hi Ezequiel,
>
> On Montag, 9. Mai 2022 16:17:03 CEST Ezequiel Garcia wrote:
> > Hi Nicolas,
> >
> > On Sun, May 8, 2022 at 5:26 PM Nicolas Frattaroli
> > <frattaroli.nicolas@gmail.com> wrote:
> > >
> > > The RK3566 and RK3568 come with a dedicated Hantro instance solely for
> > > encoding. This patch adds a node for this to the device tree, along with
> > > a node for its MMU.
> > >
> > > Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> > > ---
> > >  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 21 +++++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > index 7cdef800cb3c..2e3c9e1887e3 100644
> > > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > @@ -508,6 +508,27 @@ gpu: gpu@fde60000 {
> > >                 status = "disabled";
> > >         };
> > >
> > > +       vepu: video-codec@fdee0000 {
> > > +               compatible = "rockchip,rk3568-vepu";
> > > +               reg = <0x0 0xfdee0000 0x0 0x800>;
> > > +               interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> > > +               interrupt-names = "vepu";
> >
> > It this block "encoder only" and if so, maybe we should remove the
> > "interrupt-names" [1]?
> >
> > The driver is able to handle it. See:
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/hantro/hantro_drv.c#L962
> >
> > You might have to adjust the dt-bindings for this.
> >
> > [1] https://lore.kernel.org/linux-media/20210324151715.GA3070006@robh.at.kernel.org/
>
> What the Linux driver can handle should not matter to the device tree;
> device trees are independent of drivers and kernels.
>

I guess my message wasn't clear, no need to lecture me on Device
Trees, although I appreciate
your friendly reminder of what a Device Tree is.

Having said that, the binding is designed to support both decoders and encoders
for instance:

        vpu: video-codec@ff9a0000 {
                compatible = "rockchip,rk3288-vpu";
                reg = <0x0 0xff9a0000 0x0 0x800>;
                interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
                             <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
                interrupt-names = "vepu", "vdpu";
                clocks = <&cru ACLK_VCODEC>, <&cru HCLK_VCODEC>;
                clock-names = "aclk", "hclk";
                iommus = <&vpu_mmu>;
                power-domains = <&power RK3288_PD_VIDEO>;
        };

Hence the question is why do you splitted the encoder to its own node?

If we have good reasons to have separated Device Tree nodes,
then having interrupt-names = "vepu" for its only interrupt line
doesn't make sense.

> What does matter though is to be consistent in the bindings.
> interrupt-names is a required property even if there's only a vdpu
> interrupt. I modelled my vepu-only binding after this case.
>

The current binding models the idea of decoder and encoder
being the same device. This has never been really really accurate,
as the encoder and decoders have always been more or less independent.

The reason for having them on a single device are mostly historical,
some old devices shared some resource. I don't think this is the case anymore,
but the binding was still modeled to support that.

Hopefully this makes sense!
Thanks,
Ezequiel


> If robh thinks there is no value to having the interrupt show up
> as anything other than "default" in /proc/interrupts, then I respectfully
> disagree with that opinion and point out that this should have been brought
> up when the vdpu-only case in the bindings was made to require
> interrupt-names also.
>
> Changing the binding now that there theoretically could be drivers out
> in the wild (though I doubt it) that do require interrupt-names, because
> the binding told them that this is okay to do, seems unwise to me.
>
> Regards,
> Nicolas Frattaroli
>
> >
> > Thanks,
> > Ezequiel
> >
> > > +               clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> > > +               clock-names = "aclk", "hclk";
> > > +               iommus = <&vepu_mmu>;
> > > +               power-domains = <&power RK3568_PD_RGA>;
> > > +       };
> > > +
> > > +       vepu_mmu: iommu@fdee0800 {
> > > +               compatible = "rockchip,rk3568-iommu";
> > > +               reg = <0x0 0xfdee0800 0x0 0x40>;
> > > +               interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
> > > +               clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> > > +               clock-names = "aclk", "iface";
> > > +               power-domains = <&power RK3568_PD_RGA>;
> > > +               #iommu-cells = <0>;
> > > +       };
> > > +
> > >         sdmmc2: mmc@fe000000 {
> > >                 compatible = "rockchip,rk3568-dw-mshc", "rockchip,rk3288-dw-mshc";
> > >                 reg = <0x0 0xfe000000 0x0 0x4000>;
> > > --
> > > 2.36.0
> > >
> > >
> > > _______________________________________________
> > > Linux-rockchip mailing list
> > > Linux-rockchip@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-rockchip
> >
>
>
>
>

Re: [PATCH v2 3/3] arm64: dts: rockchip: Add Hantro encoder node to rk356x

[Nicolas Frattaroli]

On Donnerstag, 12. Mai 2022 16:16:52 CEST Ezequiel Garcia wrote:
> On Tue, May 10, 2022 at 12:28 PM Nicolas Frattaroli
> <frattaroli.nicolas@gmail.com> wrote:
> >
> > Hi Ezequiel,
> >
> > On Montag, 9. Mai 2022 16:17:03 CEST Ezequiel Garcia wrote:
> > > Hi Nicolas,
> > >
> > > On Sun, May 8, 2022 at 5:26 PM Nicolas Frattaroli
> > > <frattaroli.nicolas@gmail.com> wrote:
> > > >
> > > > The RK3566 and RK3568 come with a dedicated Hantro instance solely for
> > > > encoding. This patch adds a node for this to the device tree, along with
> > > > a node for its MMU.
> > > >
> > > > Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> > > > ---
> > > >  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 21 +++++++++++++++++++++
> > > >  1 file changed, 21 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > index 7cdef800cb3c..2e3c9e1887e3 100644
> > > > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > @@ -508,6 +508,27 @@ gpu: gpu@fde60000 {
> > > >                 status = "disabled";
> > > >         };
> > > >
> > > > +       vepu: video-codec@fdee0000 {
> > > > +               compatible = "rockchip,rk3568-vepu";
> > > > +               reg = <0x0 0xfdee0000 0x0 0x800>;
> > > > +               interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> > > > +               interrupt-names = "vepu";
> > >
> > > It this block "encoder only" and if so, maybe we should remove the
> > > "interrupt-names" [1]?
> > >
> > > The driver is able to handle it. See:
> > >
> > > https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/hantro/hantro_drv.c#L962
> > >
> > > You might have to adjust the dt-bindings for this.
> > >
> > > [1] https://lore.kernel.org/linux-media/20210324151715.GA3070006@robh.at.kernel.org/
> >
> > What the Linux driver can handle should not matter to the device tree;
> > device trees are independent of drivers and kernels.
> >
> 
> I guess my message wasn't clear, no need to lecture me on Device
> Trees, although I appreciate
> your friendly reminder of what a Device Tree is.
> 
> Having said that, the binding is designed to support both decoders and encoders
> for instance:
> 
>         vpu: video-codec@ff9a0000 {
>                 compatible = "rockchip,rk3288-vpu";
>                 reg = <0x0 0xff9a0000 0x0 0x800>;
>                 interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
>                              <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
>                 interrupt-names = "vepu", "vdpu";
>                 clocks = <&cru ACLK_VCODEC>, <&cru HCLK_VCODEC>;
>                 clock-names = "aclk", "hclk";
>                 iommus = <&vpu_mmu>;
>                 power-domains = <&power RK3288_PD_VIDEO>;
>         };
> 
> Hence the question is why do you splitted the encoder to its own node?

It has its own IOMMU and is in a different power domain than the decoder.
I think I have mentioned this multiple times before, including in the
cover letter.

Assuming you do not believe me, feel free to check the TRM, of which I
am sure you also have a copy: page 475 of Part 1 shows the VPU being in
PD_VPU while the JPEG encoder is in PD_RGA. Pages 478 and 479 of Part 2,
Section 10.5, shows that the JPEG encoder (VEPU121)'s base is not the
same as the Hantro decoder (VDPU121)'s base, and their IOMMUs which are
based relative to their base offset are therefore also not at the same
address. If you think the TRM must be wrong then, consider the fact that
I have actually run this patch set, presumably being the only person to
do so, and found that it works, so no, the addresses and power domains
are correct.

I do not see any way in which it would make sense to put this into the
same node as the decoder. It would not even be possible to do this in
your bindings, as they specify a maxItems for power-domains and iommus
of 1. Even if I modified them the driver wouldn't know which PD and
IOMMU belongs to decoder and encoder.

I think if we put this encoder in the same node as the decoder, we
might as well take this to its natural conclusion and put the entire
device tree into a single very large node. It's not the same hardware,
it cannot be modelled as being the same hardware, just because the
bindings lets people model some separate hardware as the same hardware
doesn't mean this applies to this hardware.

Long story short, why did I split the encoder to its own node? The
answer is that I didn't. I simply refused to combine it into a node
that it has nothing to do with.
 
> If we have good reasons to have separated Device Tree nodes,
> then having interrupt-names = "vepu" for its only interrupt line
> doesn't make sense.

How does it not make sense? The bindings allow for a vdpu only
interrupt-names, which in my understanding makes the same amount
of sense.

Regards,
Nicolas Frattaroli

> 
> > What does matter though is to be consistent in the bindings.
> > interrupt-names is a required property even if there's only a vdpu
> > interrupt. I modelled my vepu-only binding after this case.
> >
> 
> The current binding models the idea of decoder and encoder
> being the same device. This has never been really really accurate,
> as the encoder and decoders have always been more or less independent.
> 
> The reason for having them on a single device are mostly historical,
> some old devices shared some resource. I don't think this is the case anymore,
> but the binding was still modeled to support that.
> 
> Hopefully this makes sense!
> Thanks,
> Ezequiel
> 
> 
> > If robh thinks there is no value to having the interrupt show up
> > as anything other than "default" in /proc/interrupts, then I respectfully
> > disagree with that opinion and point out that this should have been brought
> > up when the vdpu-only case in the bindings was made to require
> > interrupt-names also.
> >
> > Changing the binding now that there theoretically could be drivers out
> > in the wild (though I doubt it) that do require interrupt-names, because
> > the binding told them that this is okay to do, seems unwise to me.
> >
> > Regards,
> > Nicolas Frattaroli
> >
> > >
> > > Thanks,
> > > Ezequiel
> > >
> > > > +               clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> > > > +               clock-names = "aclk", "hclk";
> > > > +               iommus = <&vepu_mmu>;
> > > > +               power-domains = <&power RK3568_PD_RGA>;
> > > > +       };
> > > > +
> > > > +       vepu_mmu: iommu@fdee0800 {
> > > > +               compatible = "rockchip,rk3568-iommu";
> > > > +               reg = <0x0 0xfdee0800 0x0 0x40>;
> > > > +               interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
> > > > +               clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> > > > +               clock-names = "aclk", "iface";
> > > > +               power-domains = <&power RK3568_PD_RGA>;
> > > > +               #iommu-cells = <0>;
> > > > +       };
> > > > +
> > > >         sdmmc2: mmc@fe000000 {
> > > >                 compatible = "rockchip,rk3568-dw-mshc", "rockchip,rk3288-dw-mshc";
> > > >                 reg = <0x0 0xfe000000 0x0 0x4000>;
> > > > --
> > > > 2.36.0
> > > >
> > > >
> > > > _______________________________________________
> > > > Linux-rockchip mailing list
> > > > Linux-rockchip@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-rockchip
> > >
> >
> >
> >
> >
> 

Re: [PATCH v2 3/3] arm64: dts: rockchip: Add Hantro encoder node to rk356x

[Ezequiel Garcia]

On Thu, May 12, 2022 at 5:00 PM Nicolas Frattaroli
<frattaroli.nicolas@gmail.com> wrote:
>
> On Donnerstag, 12. Mai 2022 16:16:52 CEST Ezequiel Garcia wrote:
> > On Tue, May 10, 2022 at 12:28 PM Nicolas Frattaroli
> > <frattaroli.nicolas@gmail.com> wrote:
> > >
> > > Hi Ezequiel,
> > >
> > > On Montag, 9. Mai 2022 16:17:03 CEST Ezequiel Garcia wrote:
> > > > Hi Nicolas,
> > > >
> > > > On Sun, May 8, 2022 at 5:26 PM Nicolas Frattaroli
> > > > <frattaroli.nicolas@gmail.com> wrote:
> > > > >
> > > > > The RK3566 and RK3568 come with a dedicated Hantro instance solely for
> > > > > encoding. This patch adds a node for this to the device tree, along with
> > > > > a node for its MMU.
> > > > >
> > > > > Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> > > > > ---
> > > > >  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 21 +++++++++++++++++++++
> > > > >  1 file changed, 21 insertions(+)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > > index 7cdef800cb3c..2e3c9e1887e3 100644
> > > > > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > > @@ -508,6 +508,27 @@ gpu: gpu@fde60000 {
> > > > >                 status = "disabled";
> > > > >         };
> > > > >
> > > > > +       vepu: video-codec@fdee0000 {
> > > > > +               compatible = "rockchip,rk3568-vepu";
> > > > > +               reg = <0x0 0xfdee0000 0x0 0x800>;
> > > > > +               interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> > > > > +               interrupt-names = "vepu";
> > > >
> > > > It this block "encoder only" and if so, maybe we should remove the
> > > > "interrupt-names" [1]?
> > > >
> > > > The driver is able to handle it. See:
> > > >
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/hantro/hantro_drv.c#L962
> > > >
> > > > You might have to adjust the dt-bindings for this.
> > > >
> > > > [1] https://lore.kernel.org/linux-media/20210324151715.GA3070006@robh.at.kernel.org/
> > >
> > > What the Linux driver can handle should not matter to the device tree;
> > > device trees are independent of drivers and kernels.
> > >
> >
> > I guess my message wasn't clear, no need to lecture me on Device
> > Trees, although I appreciate
> > your friendly reminder of what a Device Tree is.
> >
> > Having said that, the binding is designed to support both decoders and encoders
> > for instance:
> >
> >         vpu: video-codec@ff9a0000 {
> >                 compatible = "rockchip,rk3288-vpu";
> >                 reg = <0x0 0xff9a0000 0x0 0x800>;
> >                 interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> >                              <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> >                 interrupt-names = "vepu", "vdpu";
> >                 clocks = <&cru ACLK_VCODEC>, <&cru HCLK_VCODEC>;
> >                 clock-names = "aclk", "hclk";
> >                 iommus = <&vpu_mmu>;
> >                 power-domains = <&power RK3288_PD_VIDEO>;
> >         };
> >
> > Hence the question is why do you splitted the encoder to its own node?
>
> It has its own IOMMU and is in a different power domain than the decoder.
> I think I have mentioned this multiple times before, including in the
> cover letter.
>
> Assuming you do not believe me, feel free to check the TRM, of which I
> am sure you also have a copy: page 475 of Part 1 shows the VPU being in
> PD_VPU while the JPEG encoder is in PD_RGA. Pages 478 and 479 of Part 2,
> Section 10.5, shows that the JPEG encoder (VEPU121)'s base is not the
> same as the Hantro decoder (VDPU121)'s base, and their IOMMUs which are
> based relative to their base offset are therefore also not at the same
> address. If you think the TRM must be wrong then, consider the fact that
> I have actually run this patch set, presumably being the only person to
> do so, and found that it works, so no, the addresses and power domains
> are correct.
>
> I do not see any way in which it would make sense to put this into the
> same node as the decoder. It would not even be possible to do this in
> your bindings, as they specify a maxItems for power-domains and iommus
> of 1. Even if I modified them the driver wouldn't know which PD and
> IOMMU belongs to decoder and encoder.
>
> I think if we put this encoder in the same node as the decoder, we
> might as well take this to its natural conclusion and put the entire
> device tree into a single very large node. It's not the same hardware,
> it cannot be modelled as being the same hardware, just because the
> bindings lets people model some separate hardware as the same hardware
> doesn't mean this applies to this hardware.
>
> Long story short, why did I split the encoder to its own node? The
> answer is that I didn't. I simply refused to combine it into a node
> that it has nothing to do with.
>

As I've mentioned:

"""
the current binding models the idea of decoder and encoder
being the same device. This has never been really really accurate,
as the encoder and decoders have always been more or less independent.

The reason for having them on a single device are mostly historical,
some old devices shared some resource. I don't think this is the case anymore,
but the binding was still modeled to support that.
"""

The PX30 and RK3399 VPUs are probably pretty independent as well,
and in retrospective, we should have done separated Device Tree nodes.
For historical reasons, we didn't, and we introduced those weird "enc_offset"
and "dec_offset" fields:

const struct hantro_variant px30_vpu_variant = {
        .enc_offset = 0x0,
        .enc_fmts = rockchip_vpu_enc_fmts,
        .num_enc_fmts = ARRAY_SIZE(rockchip_vpu_enc_fmts),
        .dec_offset = 0x400,
        .dec_fmts = rk3399_vpu_dec_fmts,


> > If we have good reasons to have separated Device Tree nodes,
> > then having interrupt-names = "vepu" for its only interrupt line
> > doesn't make sense.
>
> How does it not make sense? The bindings allow for a vdpu only
> interrupt-names, which in my understanding makes the same amount
> of sense.
>

That applies for the binding for the previous existing compatible strings.

You are adding a new compatible string, so just change the binding
so it no longer requires "interrupt-names", for its single interrupt line.

Quoting devicetree maintainer [1]:

"""
 *-names are used to distinguish multiple entries
and don't add anything if only a single entry.
"""

[1] https://lore.kernel.org/linux-media/20210324151715.GA3070006@robh.at.kernel.org/

Thanks!
Ezequiel

> Regards,
> Nicolas Frattaroli
>
> >
> > > What does matter though is to be consistent in the bindings.
> > > interrupt-names is a required property even if there's only a vdpu
> > > interrupt. I modelled my vepu-only binding after this case.
> > >
> >
> > The current binding models the idea of decoder and encoder
> > being the same device. This has never been really really accurate,
> > as the encoder and decoders have always been more or less independent.
> >
> > The reason for having them on a single device are mostly historical,
> > some old devices shared some resource. I don't think this is the case anymore,
> > but the binding was still modeled to support that.
> >
> > Hopefully this makes sense!
> > Thanks,
> > Ezequiel
> >
> >
> > > If robh thinks there is no value to having the interrupt show up
> > > as anything other than "default" in /proc/interrupts, then I respectfully
> > > disagree with that opinion and point out that this should have been brought
> > > up when the vdpu-only case in the bindings was made to require
> > > interrupt-names also.
> > >
> > > Changing the binding now that there theoretically could be drivers out
> > > in the wild (though I doubt it) that do require interrupt-names, because
> > > the binding told them that this is okay to do, seems unwise to me.
> > >
> > > Regards,
> > > Nicolas Frattaroli
> > >
> > > >
> > > > Thanks,
> > > > Ezequiel
> > > >
> > > > > +               clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> > > > > +               clock-names = "aclk", "hclk";
> > > > > +               iommus = <&vepu_mmu>;
> > > > > +               power-domains = <&power RK3568_PD_RGA>;
> > > > > +       };
> > > > > +
> > > > > +       vepu_mmu: iommu@fdee0800 {
> > > > > +               compatible = "rockchip,rk3568-iommu";
> > > > > +               reg = <0x0 0xfdee0800 0x0 0x40>;
> > > > > +               interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
> > > > > +               clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> > > > > +               clock-names = "aclk", "iface";
> > > > > +               power-domains = <&power RK3568_PD_RGA>;
> > > > > +               #iommu-cells = <0>;
> > > > > +       };
> > > > > +
> > > > >         sdmmc2: mmc@fe000000 {
> > > > >                 compatible = "rockchip,rk3568-dw-mshc", "rockchip,rk3288-dw-mshc";
> > > > >                 reg = <0x0 0xfe000000 0x0 0x4000>;
> > > > > --
> > > > > 2.36.0
> > > > >
> > > > >
> > > > > _______________________________________________
> > > > > Linux-rockchip mailing list
> > > > > Linux-rockchip@lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/linux-rockchip
> > > >
> > >
> > >
> > >
> > >
> >
>
>
>
>

Re: [PATCH v2 3/3] arm64: dts: rockchip: Add Hantro encoder node to rk356x

[Nicolas Frattaroli]

On Donnerstag, 12. Mai 2022 23:33:03 CEST Ezequiel Garcia wrote:
> On Thu, May 12, 2022 at 5:00 PM Nicolas Frattaroli
> <frattaroli.nicolas@gmail.com> wrote:
> >
> > On Donnerstag, 12. Mai 2022 16:16:52 CEST Ezequiel Garcia wrote:
> > > On Tue, May 10, 2022 at 12:28 PM Nicolas Frattaroli
> > > <frattaroli.nicolas@gmail.com> wrote:
> > > >
> > > > Hi Ezequiel,
> > > >
> > > > On Montag, 9. Mai 2022 16:17:03 CEST Ezequiel Garcia wrote:
> > > > > Hi Nicolas,
> > > > >
> > > > > On Sun, May 8, 2022 at 5:26 PM Nicolas Frattaroli
> > > > > <frattaroli.nicolas@gmail.com> wrote:
> > > > > >
> > > > > > The RK3566 and RK3568 come with a dedicated Hantro instance solely for
> > > > > > encoding. This patch adds a node for this to the device tree, along with
> > > > > > a node for its MMU.
> > > > > >
> > > > > > Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> > > > > > ---
> > > > > >  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 21 +++++++++++++++++++++
> > > > > >  1 file changed, 21 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > > > index 7cdef800cb3c..2e3c9e1887e3 100644
> > > > > > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > > > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > > > @@ -508,6 +508,27 @@ gpu: gpu@fde60000 {
> > > > > >                 status = "disabled";
> > > > > >         };
> > > > > >
> > > > > > +       vepu: video-codec@fdee0000 {
> > > > > > +               compatible = "rockchip,rk3568-vepu";
> > > > > > +               reg = <0x0 0xfdee0000 0x0 0x800>;
> > > > > > +               interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > +               interrupt-names = "vepu";
> > > > >
> > > > > It this block "encoder only" and if so, maybe we should remove the
> > > > > "interrupt-names" [1]?
> > > > >
> > > > > The driver is able to handle it. See:
> > > > >
> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/hantro/hantro_drv.c#L962
> > > > >
> > > > > You might have to adjust the dt-bindings for this.
> > > > >
> > > > > [1] https://lore.kernel.org/linux-media/20210324151715.GA3070006@robh.at.kernel.org/
> > > >
> > > > What the Linux driver can handle should not matter to the device tree;
> > > > device trees are independent of drivers and kernels.
> > > >
> > >
> > > I guess my message wasn't clear, no need to lecture me on Device
> > > Trees, although I appreciate
> > > your friendly reminder of what a Device Tree is.
> > >
> > > Having said that, the binding is designed to support both decoders and encoders
> > > for instance:
> > >
> > >         vpu: video-codec@ff9a0000 {
> > >                 compatible = "rockchip,rk3288-vpu";
> > >                 reg = <0x0 0xff9a0000 0x0 0x800>;
> > >                 interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> > >                              <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> > >                 interrupt-names = "vepu", "vdpu";
> > >                 clocks = <&cru ACLK_VCODEC>, <&cru HCLK_VCODEC>;
> > >                 clock-names = "aclk", "hclk";
> > >                 iommus = <&vpu_mmu>;
> > >                 power-domains = <&power RK3288_PD_VIDEO>;
> > >         };
> > >
> > > Hence the question is why do you splitted the encoder to its own node?
> >
> > It has its own IOMMU and is in a different power domain than the decoder.
> > I think I have mentioned this multiple times before, including in the
> > cover letter.
> >
> > Assuming you do not believe me, feel free to check the TRM, of which I
> > am sure you also have a copy: page 475 of Part 1 shows the VPU being in
> > PD_VPU while the JPEG encoder is in PD_RGA. Pages 478 and 479 of Part 2,
> > Section 10.5, shows that the JPEG encoder (VEPU121)'s base is not the
> > same as the Hantro decoder (VDPU121)'s base, and their IOMMUs which are
> > based relative to their base offset are therefore also not at the same
> > address. If you think the TRM must be wrong then, consider the fact that
> > I have actually run this patch set, presumably being the only person to
> > do so, and found that it works, so no, the addresses and power domains
> > are correct.
> >
> > I do not see any way in which it would make sense to put this into the
> > same node as the decoder. It would not even be possible to do this in
> > your bindings, as they specify a maxItems for power-domains and iommus
> > of 1. Even if I modified them the driver wouldn't know which PD and
> > IOMMU belongs to decoder and encoder.
> >
> > I think if we put this encoder in the same node as the decoder, we
> > might as well take this to its natural conclusion and put the entire
> > device tree into a single very large node. It's not the same hardware,
> > it cannot be modelled as being the same hardware, just because the
> > bindings lets people model some separate hardware as the same hardware
> > doesn't mean this applies to this hardware.
> >
> > Long story short, why did I split the encoder to its own node? The
> > answer is that I didn't. I simply refused to combine it into a node
> > that it has nothing to do with.
> >
> 
> As I've mentioned:
> 
> """
> the current binding models the idea of decoder and encoder
> being the same device. This has never been really really accurate,
> as the encoder and decoders have always been more or less independent.
> 
> The reason for having them on a single device are mostly historical,
> some old devices shared some resource. I don't think this is the case anymore,
> but the binding was still modeled to support that.
> """
> 
> The PX30 and RK3399 VPUs are probably pretty independent as well,
> and in retrospective, we should have done separated Device Tree nodes.
> For historical reasons, we didn't, and we introduced those weird "enc_offset"
> and "dec_offset" fields:
> 
> const struct hantro_variant px30_vpu_variant = {
>         .enc_offset = 0x0,
>         .enc_fmts = rockchip_vpu_enc_fmts,
>         .num_enc_fmts = ARRAY_SIZE(rockchip_vpu_enc_fmts),
>         .dec_offset = 0x400,
>         .dec_fmts = rk3399_vpu_dec_fmts,
> 

As I've mentioned: that doesn't work for this hardware. It's not just the
memory addresses. You literally quoted the part where I explain this, and
then decided to completely ignore it. I will not explain it again, you
have the explanation once more right in this e-mail. Read it.

Not to mention that you've also ignored that I disagree with rob's
assessment about interrupt-names.

I'm actually done arguing with you, this is going in circles. v4 will not
address any of your concerns, because it's either literally impossible or
because I disagree with your concern and you did not actually address my
disagreement.

> 
> > > If we have good reasons to have separated Device Tree nodes,
> > > then having interrupt-names = "vepu" for its only interrupt line
> > > doesn't make sense.
> >
> > How does it not make sense? The bindings allow for a vdpu only
> > interrupt-names, which in my understanding makes the same amount
> > of sense.
> >
> 
> That applies for the binding for the previous existing compatible strings.
> 
> You are adding a new compatible string, so just change the binding
> so it no longer requires "interrupt-names", for its single interrupt line.
> 
> Quoting devicetree maintainer [1]:
> 
> """
>  *-names are used to distinguish multiple entries
> and don't add anything if only a single entry.
> """
> 
> [1] https://lore.kernel.org/linux-media/20210324151715.GA3070006@robh.at.kernel.org/
> 
> Thanks!
> Ezequiel
> 
> > Regards,
> > Nicolas Frattaroli
> >
> > >
> > > > What does matter though is to be consistent in the bindings.
> > > > interrupt-names is a required property even if there's only a vdpu
> > > > interrupt. I modelled my vepu-only binding after this case.
> > > >
> > >
> > > The current binding models the idea of decoder and encoder
> > > being the same device. This has never been really really accurate,
> > > as the encoder and decoders have always been more or less independent.
> > >
> > > The reason for having them on a single device are mostly historical,
> > > some old devices shared some resource. I don't think this is the case anymore,
> > > but the binding was still modeled to support that.
> > >
> > > Hopefully this makes sense!
> > > Thanks,
> > > Ezequiel
> > >
> > >
> > > > If robh thinks there is no value to having the interrupt show up
> > > > as anything other than "default" in /proc/interrupts, then I respectfully
> > > > disagree with that opinion and point out that this should have been brought
> > > > up when the vdpu-only case in the bindings was made to require
> > > > interrupt-names also.
> > > >
> > > > Changing the binding now that there theoretically could be drivers out
> > > > in the wild (though I doubt it) that do require interrupt-names, because
> > > > the binding told them that this is okay to do, seems unwise to me.
> > > >
> > > > Regards,
> > > > Nicolas Frattaroli
> > > >
> > > > >
> > > > > Thanks,
> > > > > Ezequiel
> > > > >
> > > > > > +               clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> > > > > > +               clock-names = "aclk", "hclk";
> > > > > > +               iommus = <&vepu_mmu>;
> > > > > > +               power-domains = <&power RK3568_PD_RGA>;
> > > > > > +       };
> > > > > > +
> > > > > > +       vepu_mmu: iommu@fdee0800 {
> > > > > > +               compatible = "rockchip,rk3568-iommu";
> > > > > > +               reg = <0x0 0xfdee0800 0x0 0x40>;
> > > > > > +               interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > +               clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> > > > > > +               clock-names = "aclk", "iface";
> > > > > > +               power-domains = <&power RK3568_PD_RGA>;
> > > > > > +               #iommu-cells = <0>;
> > > > > > +       };
> > > > > > +
> > > > > >         sdmmc2: mmc@fe000000 {
> > > > > >                 compatible = "rockchip,rk3568-dw-mshc", "rockchip,rk3288-dw-mshc";
> > > > > >                 reg = <0x0 0xfe000000 0x0 0x4000>;
> > > > > > --
> > > > > > 2.36.0
> > > > > >
> > > > > >
> > > > > > _______________________________________________
> > > > > > Linux-rockchip mailing list
> > > > > > Linux-rockchip@lists.infradead.org
> > > > > > http://lists.infradead.org/mailman/listinfo/linux-rockchip
> > > > >
> > > >
> > > >
> > > >
> > > >
> > >
> >
> >
> >
> >
> 

Re: [PATCH v2 3/3] arm64: dts: rockchip: Add Hantro encoder node to rk356x

[Ezequiel Garcia]

Hi Nicolas,

On Fri, May 13, 2022 at 3:23 AM Nicolas Frattaroli
<frattaroli.nicolas@gmail.com> wrote:
>
> On Donnerstag, 12. Mai 2022 23:33:03 CEST Ezequiel Garcia wrote:
> > On Thu, May 12, 2022 at 5:00 PM Nicolas Frattaroli
> > <frattaroli.nicolas@gmail.com> wrote:
> > >
> > > On Donnerstag, 12. Mai 2022 16:16:52 CEST Ezequiel Garcia wrote:
> > > > On Tue, May 10, 2022 at 12:28 PM Nicolas Frattaroli
> > > > <frattaroli.nicolas@gmail.com> wrote:
> > > > >
> > > > > Hi Ezequiel,
> > > > >
> > > > > On Montag, 9. Mai 2022 16:17:03 CEST Ezequiel Garcia wrote:
> > > > > > Hi Nicolas,
> > > > > >
> > > > > > On Sun, May 8, 2022 at 5:26 PM Nicolas Frattaroli
> > > > > > <frattaroli.nicolas@gmail.com> wrote:
> > > > > > >
> > > > > > > The RK3566 and RK3568 come with a dedicated Hantro instance solely for
> > > > > > > encoding. This patch adds a node for this to the device tree, along with
> > > > > > > a node for its MMU.
> > > > > > >
> > > > > > > Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> > > > > > > ---
> > > > > > >  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 21 +++++++++++++++++++++
> > > > > > >  1 file changed, 21 insertions(+)
> > > > > > >
> > > > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > > > > index 7cdef800cb3c..2e3c9e1887e3 100644
> > > > > > > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > > > > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > > > > @@ -508,6 +508,27 @@ gpu: gpu@fde60000 {
> > > > > > >                 status = "disabled";
> > > > > > >         };
> > > > > > >
> > > > > > > +       vepu: video-codec@fdee0000 {
> > > > > > > +               compatible = "rockchip,rk3568-vepu";
> > > > > > > +               reg = <0x0 0xfdee0000 0x0 0x800>;
> > > > > > > +               interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > +               interrupt-names = "vepu";
> > > > > >
> > > > > > It this block "encoder only" and if so, maybe we should remove the
> > > > > > "interrupt-names" [1]?
> > > > > >
> > > > > > The driver is able to handle it. See:
> > > > > >
> > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/hantro/hantro_drv.c#L962
> > > > > >
> > > > > > You might have to adjust the dt-bindings for this.
> > > > > >
> > > > > > [1] https://lore.kernel.org/linux-media/20210324151715.GA3070006@robh.at.kernel.org/
> > > > >
> > > > > What the Linux driver can handle should not matter to the device tree;
> > > > > device trees are independent of drivers and kernels.
> > > > >
> > > >
> > > > I guess my message wasn't clear, no need to lecture me on Device
> > > > Trees, although I appreciate
> > > > your friendly reminder of what a Device Tree is.
> > > >
> > > > Having said that, the binding is designed to support both decoders and encoders
> > > > for instance:
> > > >
> > > >         vpu: video-codec@ff9a0000 {
> > > >                 compatible = "rockchip,rk3288-vpu";
> > > >                 reg = <0x0 0xff9a0000 0x0 0x800>;
> > > >                 interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> > > >                              <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> > > >                 interrupt-names = "vepu", "vdpu";
> > > >                 clocks = <&cru ACLK_VCODEC>, <&cru HCLK_VCODEC>;
> > > >                 clock-names = "aclk", "hclk";
> > > >                 iommus = <&vpu_mmu>;
> > > >                 power-domains = <&power RK3288_PD_VIDEO>;
> > > >         };
> > > >
> > > > Hence the question is why do you splitted the encoder to its own node?
> > >
> > > It has its own IOMMU and is in a different power domain than the decoder.
> > > I think I have mentioned this multiple times before, including in the
> > > cover letter.
> > >
> > > Assuming you do not believe me, feel free to check the TRM, of which I
> > > am sure you also have a copy: page 475 of Part 1 shows the VPU being in
> > > PD_VPU while the JPEG encoder is in PD_RGA. Pages 478 and 479 of Part 2,
> > > Section 10.5, shows that the JPEG encoder (VEPU121)'s base is not the
> > > same as the Hantro decoder (VDPU121)'s base, and their IOMMUs which are
> > > based relative to their base offset are therefore also not at the same
> > > address. If you think the TRM must be wrong then, consider the fact that
> > > I have actually run this patch set, presumably being the only person to
> > > do so, and found that it works, so no, the addresses and power domains
> > > are correct.
> > >
> > > I do not see any way in which it would make sense to put this into the
> > > same node as the decoder. It would not even be possible to do this in
> > > your bindings, as they specify a maxItems for power-domains and iommus
> > > of 1. Even if I modified them the driver wouldn't know which PD and
> > > IOMMU belongs to decoder and encoder.
> > >
> > > I think if we put this encoder in the same node as the decoder, we
> > > might as well take this to its natural conclusion and put the entire
> > > device tree into a single very large node. It's not the same hardware,
> > > it cannot be modelled as being the same hardware, just because the
> > > bindings lets people model some separate hardware as the same hardware
> > > doesn't mean this applies to this hardware.
> > >
> > > Long story short, why did I split the encoder to its own node? The
> > > answer is that I didn't. I simply refused to combine it into a node
> > > that it has nothing to do with.
> > >
> >
> > As I've mentioned:
> >
> > """
> > the current binding models the idea of decoder and encoder
> > being the same device. This has never been really really accurate,
> > as the encoder and decoders have always been more or less independent.
> >
> > The reason for having them on a single device are mostly historical,
> > some old devices shared some resource. I don't think this is the case anymore,
> > but the binding was still modeled to support that.
> > """
> >
> > The PX30 and RK3399 VPUs are probably pretty independent as well,
> > and in retrospective, we should have done separated Device Tree nodes.
> > For historical reasons, we didn't, and we introduced those weird "enc_offset"
> > and "dec_offset" fields:
> >
> > const struct hantro_variant px30_vpu_variant = {
> >         .enc_offset = 0x0,
> >         .enc_fmts = rockchip_vpu_enc_fmts,
> >         .num_enc_fmts = ARRAY_SIZE(rockchip_vpu_enc_fmts),
> >         .dec_offset = 0x400,
> >         .dec_fmts = rk3399_vpu_dec_fmts,
> >
>
> As I've mentioned: that doesn't work for this hardware. It's not just the
> memory addresses. You literally quoted the part where I explain this, and
> then decided to completely ignore it.

I didn't ignore anything. I was just trying to explain you,
how the decoder and the encoder could have been separated for almost
all the other Rockchip devices, just like you are doing here.

> I will not explain it again, you
> have the explanation once more right in this e-mail. Read it.
>
> Not to mention that you've also ignored that I disagree with rob's
> assessment about interrupt-names.
>

I didn't ignore it, I just didn't reply to it. You think this is about
changing a dt-binding, but you are actually introducing a new dt-binding
since you are adding a new compatible string.

You are doing so by extending an existing dt-binding.

I am explaining you the _existing_ dt-binding models the (incorrect) idea
of a combined decoder and encoder. Since your device is encoder-only
and has a single interrupt line, you should omit the interrupt-names,
because it doesn't not add anything.

(About your dislike for the "default" string in /proc,  that is a
driver thing, which can be changed. It is not related to the
dt-binding).

> I'm actually done arguing with you, this is going in circles. v4 will not
> address any of your concerns, because it's either literally impossible or
> because I disagree with your concern and you did not actually address my
> disagreement.
>

Let's just wait for a Device Tree maintainer then. If you get a +1
from a DT maintainer for your dt-binding change, then I'll review and
consider how the rest of the patches look like.

However, it is very important that you moderate your communication,
you have been very pedantic and rude since your first reply.

Hope you can do that!
Thanks,
Ezequiel


> >
> > > > If we have good reasons to have separated Device Tree nodes,
> > > > then having interrupt-names = "vepu" for its only interrupt line
> > > > doesn't make sense.
> > >
> > > How does it not make sense? The bindings allow for a vdpu only
> > > interrupt-names, which in my understanding makes the same amount
> > > of sense.
> > >
> >
> > That applies for the binding for the previous existing compatible strings.
> >
> > You are adding a new compatible string, so just change the binding
> > so it no longer requires "interrupt-names", for its single interrupt line.
> >
> > Quoting devicetree maintainer [1]:
> >
> > """
> >  *-names are used to distinguish multiple entries
> > and don't add anything if only a single entry.
> > """
> >
> > [1] https://lore.kernel.org/linux-media/20210324151715.GA3070006@robh.at.kernel.org/
> >
> > Thanks!
> > Ezequiel
> >
> > > Regards,
> > > Nicolas Frattaroli
> > >
> > > >
> > > > > What does matter though is to be consistent in the bindings.
> > > > > interrupt-names is a required property even if there's only a vdpu
> > > > > interrupt. I modelled my vepu-only binding after this case.
> > > > >
> > > >
> > > > The current binding models the idea of decoder and encoder
> > > > being the same device. This has never been really really accurate,
> > > > as the encoder and decoders have always been more or less independent.
> > > >
> > > > The reason for having them on a single device are mostly historical,
> > > > some old devices shared some resource. I don't think this is the case anymore,
> > > > but the binding was still modeled to support that.
> > > >
> > > > Hopefully this makes sense!
> > > > Thanks,
> > > > Ezequiel
> > > >
> > > >
> > > > > If robh thinks there is no value to having the interrupt show up
> > > > > as anything other than "default" in /proc/interrupts, then I respectfully
> > > > > disagree with that opinion and point out that this should have been brought
> > > > > up when the vdpu-only case in the bindings was made to require
> > > > > interrupt-names also.
> > > > >
> > > > > Changing the binding now that there theoretically could be drivers out
> > > > > in the wild (though I doubt it) that do require interrupt-names, because
> > > > > the binding told them that this is okay to do, seems unwise to me.
> > > > >
> > > > > Regards,
> > > > > Nicolas Frattaroli
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Ezequiel
> > > > > >
> > > > > > > +               clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> > > > > > > +               clock-names = "aclk", "hclk";
> > > > > > > +               iommus = <&vepu_mmu>;
> > > > > > > +               power-domains = <&power RK3568_PD_RGA>;
> > > > > > > +       };
> > > > > > > +
> > > > > > > +       vepu_mmu: iommu@fdee0800 {
> > > > > > > +               compatible = "rockchip,rk3568-iommu";
> > > > > > > +               reg = <0x0 0xfdee0800 0x0 0x40>;
> > > > > > > +               interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > +               clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> > > > > > > +               clock-names = "aclk", "iface";
> > > > > > > +               power-domains = <&power RK3568_PD_RGA>;
> > > > > > > +               #iommu-cells = <0>;
> > > > > > > +       };
> > > > > > > +
> > > > > > >         sdmmc2: mmc@fe000000 {
> > > > > > >                 compatible = "rockchip,rk3568-dw-mshc", "rockchip,rk3288-dw-mshc";
> > > > > > >                 reg = <0x0 0xfe000000 0x0 0x4000>;
> > > > > > > --
> > > > > > > 2.36.0
> > > > > > >
> > > > > > >
> > > > > > > _______________________________________________
> > > > > > > Linux-rockchip mailing list
> > > > > > > Linux-rockchip@lists.infradead.org
> > > > > > > http://lists.infradead.org/mailman/listinfo/linux-rockchip
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > >
> > >
> > >
> > >
> >
>
>
>
>

Re: [PATCH v2 3/3] arm64: dts: rockchip: Add Hantro encoder node to rk356x

[Nicolas Frattaroli]

On Freitag, 13. Mai 2022 15:07:51 CEST Ezequiel Garcia wrote:
> Hi Nicolas,
> 
> On Fri, May 13, 2022 at 3:23 AM Nicolas Frattaroli
> <frattaroli.nicolas@gmail.com> wrote:
> >
> > On Donnerstag, 12. Mai 2022 23:33:03 CEST Ezequiel Garcia wrote:
> > > On Thu, May 12, 2022 at 5:00 PM Nicolas Frattaroli
> > > <frattaroli.nicolas@gmail.com> wrote:
> > > >
> > > > On Donnerstag, 12. Mai 2022 16:16:52 CEST Ezequiel Garcia wrote:
> > > > > On Tue, May 10, 2022 at 12:28 PM Nicolas Frattaroli
> > > > > <frattaroli.nicolas@gmail.com> wrote:
> > > > > >
> > > > > > Hi Ezequiel,
> > > > > >
> > > > > > On Montag, 9. Mai 2022 16:17:03 CEST Ezequiel Garcia wrote:
> > > > > > > Hi Nicolas,
> > > > > > >
> > > > > > > On Sun, May 8, 2022 at 5:26 PM Nicolas Frattaroli
> > > > > > > <frattaroli.nicolas@gmail.com> wrote:
> > > > > > > >
> > > > > > > > The RK3566 and RK3568 come with a dedicated Hantro instance solely for
> > > > > > > > encoding. This patch adds a node for this to the device tree, along with
> > > > > > > > a node for its MMU.
> > > > > > > >
> > > > > > > > Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> > > > > > > > ---
> > > > > > > >  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 21 +++++++++++++++++++++
> > > > > > > >  1 file changed, 21 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > > > > > index 7cdef800cb3c..2e3c9e1887e3 100644
> > > > > > > > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > > > > > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > > > > > @@ -508,6 +508,27 @@ gpu: gpu@fde60000 {
> > > > > > > >                 status = "disabled";
> > > > > > > >         };
> > > > > > > >
> > > > > > > > +       vepu: video-codec@fdee0000 {
> > > > > > > > +               compatible = "rockchip,rk3568-vepu";
> > > > > > > > +               reg = <0x0 0xfdee0000 0x0 0x800>;
> > > > > > > > +               interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > > +               interrupt-names = "vepu";
> > > > > > >
> > > > > > > It this block "encoder only" and if so, maybe we should remove the
> > > > > > > "interrupt-names" [1]?
> > > > > > >
> > > > > > > The driver is able to handle it. See:
> > > > > > >
> > > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/hantro/hantro_drv.c#L962
> > > > > > >
> > > > > > > You might have to adjust the dt-bindings for this.
> > > > > > >
> > > > > > > [1] https://lore.kernel.org/linux-media/20210324151715.GA3070006@robh.at.kernel.org/
> > > > > >
> > > > > > What the Linux driver can handle should not matter to the device tree;
> > > > > > device trees are independent of drivers and kernels.
> > > > > >
> > > > >
> > > > > I guess my message wasn't clear, no need to lecture me on Device
> > > > > Trees, although I appreciate
> > > > > your friendly reminder of what a Device Tree is.
> > > > >
> > > > > Having said that, the binding is designed to support both decoders and encoders
> > > > > for instance:
> > > > >
> > > > >         vpu: video-codec@ff9a0000 {
> > > > >                 compatible = "rockchip,rk3288-vpu";
> > > > >                 reg = <0x0 0xff9a0000 0x0 0x800>;
> > > > >                 interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> > > > >                              <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> > > > >                 interrupt-names = "vepu", "vdpu";
> > > > >                 clocks = <&cru ACLK_VCODEC>, <&cru HCLK_VCODEC>;
> > > > >                 clock-names = "aclk", "hclk";
> > > > >                 iommus = <&vpu_mmu>;
> > > > >                 power-domains = <&power RK3288_PD_VIDEO>;
> > > > >         };
> > > > >
> > > > > Hence the question is why do you splitted the encoder to its own node?
> > > >
> > > > It has its own IOMMU and is in a different power domain than the decoder.
> > > > I think I have mentioned this multiple times before, including in the
> > > > cover letter.
> > > >
> > > > Assuming you do not believe me, feel free to check the TRM, of which I
> > > > am sure you also have a copy: page 475 of Part 1 shows the VPU being in
> > > > PD_VPU while the JPEG encoder is in PD_RGA. Pages 478 and 479 of Part 2,
> > > > Section 10.5, shows that the JPEG encoder (VEPU121)'s base is not the
> > > > same as the Hantro decoder (VDPU121)'s base, and their IOMMUs which are
> > > > based relative to their base offset are therefore also not at the same
> > > > address. If you think the TRM must be wrong then, consider the fact that
> > > > I have actually run this patch set, presumably being the only person to
> > > > do so, and found that it works, so no, the addresses and power domains
> > > > are correct.
> > > >
> > > > I do not see any way in which it would make sense to put this into the
> > > > same node as the decoder. It would not even be possible to do this in
> > > > your bindings, as they specify a maxItems for power-domains and iommus
> > > > of 1. Even if I modified them the driver wouldn't know which PD and
> > > > IOMMU belongs to decoder and encoder.
> > > >
> > > > I think if we put this encoder in the same node as the decoder, we
> > > > might as well take this to its natural conclusion and put the entire
> > > > device tree into a single very large node. It's not the same hardware,
> > > > it cannot be modelled as being the same hardware, just because the
> > > > bindings lets people model some separate hardware as the same hardware
> > > > doesn't mean this applies to this hardware.
> > > >
> > > > Long story short, why did I split the encoder to its own node? The
> > > > answer is that I didn't. I simply refused to combine it into a node
> > > > that it has nothing to do with.
> > > >
> > >
> > > As I've mentioned:
> > >
> > > """
> > > the current binding models the idea of decoder and encoder
> > > being the same device. This has never been really really accurate,
> > > as the encoder and decoders have always been more or less independent.
> > >
> > > The reason for having them on a single device are mostly historical,
> > > some old devices shared some resource. I don't think this is the case anymore,
> > > but the binding was still modeled to support that.
> > > """
> > >
> > > The PX30 and RK3399 VPUs are probably pretty independent as well,
> > > and in retrospective, we should have done separated Device Tree nodes.
> > > For historical reasons, we didn't, and we introduced those weird "enc_offset"
> > > and "dec_offset" fields:
> > >
> > > const struct hantro_variant px30_vpu_variant = {
> > >         .enc_offset = 0x0,
> > >         .enc_fmts = rockchip_vpu_enc_fmts,
> > >         .num_enc_fmts = ARRAY_SIZE(rockchip_vpu_enc_fmts),
> > >         .dec_offset = 0x400,
> > >         .dec_fmts = rk3399_vpu_dec_fmts,
> > >
> >
> > As I've mentioned: that doesn't work for this hardware. It's not just the
> > memory addresses. You literally quoted the part where I explain this, and
> > then decided to completely ignore it.
> 
> I didn't ignore anything. I was just trying to explain you,
> how the decoder and the encoder could have been separated for almost
> all the other Rockchip devices, just like you are doing here.
> 
> > I will not explain it again, you
> > have the explanation once more right in this e-mail. Read it.
> >
> > Not to mention that you've also ignored that I disagree with rob's
> > assessment about interrupt-names.
> >
> 
> I didn't ignore it, I just didn't reply to it. You think this is about
> changing a dt-binding, but you are actually introducing a new dt-binding
> since you are adding a new compatible string.
> 
> You are doing so by extending an existing dt-binding.
> 
> I am explaining you the _existing_ dt-binding models the (incorrect) idea
> of a combined decoder and encoder. Since your device is encoder-only
> and has a single interrupt line, you should omit the interrupt-names,
> because it doesn't not add anything.
> 
> (About your dislike for the "default" string in /proc,  that is a
> driver thing, which can be changed. It is not related to the
> dt-binding).
> 
> > I'm actually done arguing with you, this is going in circles. v4 will not
> > address any of your concerns, because it's either literally impossible or
> > because I disagree with your concern and you did not actually address my
> > disagreement.
> >
> 
> Let's just wait for a Device Tree maintainer then. If you get a +1
> from a DT maintainer for your dt-binding change, then I'll review and
> consider how the rest of the patches look like.
> 
> However, it is very important that you moderate your communication,
> you have been very pedantic and rude since your first reply.
> 
> Hope you can do that!
> Thanks,
> Ezequiel

I apologise for my rude and pedantic communication, I misread your
replies as you repeatedly suggesting I merge the nodes into one. Had
I understood that this wasn't your intention, and you were trying to
explain to me how the old model worked, I would have been less grumpy
about this. However, I already did know how the old model worked, and
your explanation registered as you trying to make me use it to me as
I did not realise you generally agreed with me separating the encoder
into its own node.

Regards,
Nicolas Frattaroli

> 
> 
> > >
> > > > > If we have good reasons to have separated Device Tree nodes,
> > > > > then having interrupt-names = "vepu" for its only interrupt line
> > > > > doesn't make sense.
> > > >
> > > > How does it not make sense? The bindings allow for a vdpu only
> > > > interrupt-names, which in my understanding makes the same amount
> > > > of sense.
> > > >
> > >
> > > That applies for the binding for the previous existing compatible strings.
> > >
> > > You are adding a new compatible string, so just change the binding
> > > so it no longer requires "interrupt-names", for its single interrupt line.
> > >
> > > Quoting devicetree maintainer [1]:
> > >
> > > """
> > >  *-names are used to distinguish multiple entries
> > > and don't add anything if only a single entry.
> > > """
> > >
> > > [1] https://lore.kernel.org/linux-media/20210324151715.GA3070006@robh.at.kernel.org/
> > >
> > > Thanks!
> > > Ezequiel
> > >
> > > > Regards,
> > > > Nicolas Frattaroli
> > > >
> > > > >
> > > > > > What does matter though is to be consistent in the bindings.
> > > > > > interrupt-names is a required property even if there's only a vdpu
> > > > > > interrupt. I modelled my vepu-only binding after this case.
> > > > > >
> > > > >
> > > > > The current binding models the idea of decoder and encoder
> > > > > being the same device. This has never been really really accurate,
> > > > > as the encoder and decoders have always been more or less independent.
> > > > >
> > > > > The reason for having them on a single device are mostly historical,
> > > > > some old devices shared some resource. I don't think this is the case anymore,
> > > > > but the binding was still modeled to support that.
> > > > >
> > > > > Hopefully this makes sense!
> > > > > Thanks,
> > > > > Ezequiel
> > > > >
> > > > >
> > > > > > If robh thinks there is no value to having the interrupt show up
> > > > > > as anything other than "default" in /proc/interrupts, then I respectfully
> > > > > > disagree with that opinion and point out that this should have been brought
> > > > > > up when the vdpu-only case in the bindings was made to require
> > > > > > interrupt-names also.
> > > > > >
> > > > > > Changing the binding now that there theoretically could be drivers out
> > > > > > in the wild (though I doubt it) that do require interrupt-names, because
> > > > > > the binding told them that this is okay to do, seems unwise to me.
> > > > > >
> > > > > > Regards,
> > > > > > Nicolas Frattaroli
> > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Ezequiel
> > > > > > >
> > > > > > > > +               clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> > > > > > > > +               clock-names = "aclk", "hclk";
> > > > > > > > +               iommus = <&vepu_mmu>;
> > > > > > > > +               power-domains = <&power RK3568_PD_RGA>;
> > > > > > > > +       };
> > > > > > > > +
> > > > > > > > +       vepu_mmu: iommu@fdee0800 {
> > > > > > > > +               compatible = "rockchip,rk3568-iommu";
> > > > > > > > +               reg = <0x0 0xfdee0800 0x0 0x40>;
> > > > > > > > +               interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > > +               clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> > > > > > > > +               clock-names = "aclk", "iface";
> > > > > > > > +               power-domains = <&power RK3568_PD_RGA>;
> > > > > > > > +               #iommu-cells = <0>;
> > > > > > > > +       };
> > > > > > > > +
> > > > > > > >         sdmmc2: mmc@fe000000 {
> > > > > > > >                 compatible = "rockchip,rk3568-dw-mshc", "rockchip,rk3288-dw-mshc";
> > > > > > > >                 reg = <0x0 0xfe000000 0x0 0x4000>;
> > > > > > > > --
> > > > > > > > 2.36.0
> > > > > > > >
> > > > > > > >
> > > > > > > > _______________________________________________
> > > > > > > > Linux-rockchip mailing list
> > > > > > > > Linux-rockchip@lists.infradead.org
> > > > > > > > http://lists.infradead.org/mailman/listinfo/linux-rockchip
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > >
> > > >
> > > >
> > > >
> > >
> >
> >
> >
> >
> 

Re: [PATCH v2 3/3] arm64: dts: rockchip: Add Hantro encoder node to rk356x

[Rob Herring]

On Fri, May 13, 2022 at 10:07:51AM -0300, Ezequiel Garcia wrote:
> Hi Nicolas,
> 
> On Fri, May 13, 2022 at 3:23 AM Nicolas Frattaroli
> <frattaroli.nicolas@gmail.com> wrote:
> >
> > On Donnerstag, 12. Mai 2022 23:33:03 CEST Ezequiel Garcia wrote:
> > > On Thu, May 12, 2022 at 5:00 PM Nicolas Frattaroli
> > > <frattaroli.nicolas@gmail.com> wrote:
> > > >
> > > > On Donnerstag, 12. Mai 2022 16:16:52 CEST Ezequiel Garcia wrote:
> > > > > On Tue, May 10, 2022 at 12:28 PM Nicolas Frattaroli
> > > > > <frattaroli.nicolas@gmail.com> wrote:
> > > > > >
> > > > > > Hi Ezequiel,
> > > > > >
> > > > > > On Montag, 9. Mai 2022 16:17:03 CEST Ezequiel Garcia wrote:
> > > > > > > Hi Nicolas,
> > > > > > >
> > > > > > > On Sun, May 8, 2022 at 5:26 PM Nicolas Frattaroli
> > > > > > > <frattaroli.nicolas@gmail.com> wrote:
> > > > > > > >
> > > > > > > > The RK3566 and RK3568 come with a dedicated Hantro instance solely for
> > > > > > > > encoding. This patch adds a node for this to the device tree, along with
> > > > > > > > a node for its MMU.
> > > > > > > >
> > > > > > > > Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> > > > > > > > ---
> > > > > > > >  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 21 +++++++++++++++++++++
> > > > > > > >  1 file changed, 21 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > > > > > index 7cdef800cb3c..2e3c9e1887e3 100644
> > > > > > > > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > > > > > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > > > > > @@ -508,6 +508,27 @@ gpu: gpu@fde60000 {
> > > > > > > >                 status = "disabled";
> > > > > > > >         };
> > > > > > > >
> > > > > > > > +       vepu: video-codec@fdee0000 {
> > > > > > > > +               compatible = "rockchip,rk3568-vepu";
> > > > > > > > +               reg = <0x0 0xfdee0000 0x0 0x800>;
> > > > > > > > +               interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > > +               interrupt-names = "vepu";
> > > > > > >
> > > > > > > It this block "encoder only" and if so, maybe we should remove the
> > > > > > > "interrupt-names" [1]?

Please raise binding design questions on binding patches. I don't 
regularly review dts files.

> > > > > > >
> > > > > > > The driver is able to handle it. See:
> > > > > > >
> > > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/hantro/hantro_drv.c#L962
> > > > > > >
> > > > > > > You might have to adjust the dt-bindings for this.
> > > > > > >
> > > > > > > [1] https://lore.kernel.org/linux-media/20210324151715.GA3070006@robh.at.kernel.org/
> > > > > >
> > > > > > What the Linux driver can handle should not matter to the device tree;
> > > > > > device trees are independent of drivers and kernels.
> > > > > >
> > > > >
> > > > > I guess my message wasn't clear, no need to lecture me on Device
> > > > > Trees, although I appreciate
> > > > > your friendly reminder of what a Device Tree is.

The independence is not universally known, understood, nor liked, so not 
unreasonable to point out. Your tone here is not any better.


> > > > > Having said that, the binding is designed to support both decoders and encoders
> > > > > for instance:
> > > > >
> > > > >         vpu: video-codec@ff9a0000 {
> > > > >                 compatible = "rockchip,rk3288-vpu";
> > > > >                 reg = <0x0 0xff9a0000 0x0 0x800>;
> > > > >                 interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> > > > >                              <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> > > > >                 interrupt-names = "vepu", "vdpu";
> > > > >                 clocks = <&cru ACLK_VCODEC>, <&cru HCLK_VCODEC>;
> > > > >                 clock-names = "aclk", "hclk";
> > > > >                 iommus = <&vpu_mmu>;
> > > > >                 power-domains = <&power RK3288_PD_VIDEO>;
> > > > >         };
> > > > >
> > > > > Hence the question is why do you splitted the encoder to its own node?
> > > >
> > > > It has its own IOMMU and is in a different power domain than the decoder.
> > > > I think I have mentioned this multiple times before, including in the
> > > > cover letter.
> > > >
> > > > Assuming you do not believe me, feel free to check the TRM, of which I
> > > > am sure you also have a copy: page 475 of Part 1 shows the VPU being in
> > > > PD_VPU while the JPEG encoder is in PD_RGA. Pages 478 and 479 of Part 2,
> > > > Section 10.5, shows that the JPEG encoder (VEPU121)'s base is not the
> > > > same as the Hantro decoder (VDPU121)'s base, and their IOMMUs which are
> > > > based relative to their base offset are therefore also not at the same
> > > > address. If you think the TRM must be wrong then, consider the fact that
> > > > I have actually run this patch set, presumably being the only person to
> > > > do so, and found that it works, so no, the addresses and power domains
> > > > are correct.
> > > >
> > > > I do not see any way in which it would make sense to put this into the
> > > > same node as the decoder. It would not even be possible to do this in
> > > > your bindings, as they specify a maxItems for power-domains and iommus
> > > > of 1. Even if I modified them the driver wouldn't know which PD and
> > > > IOMMU belongs to decoder and encoder.
> > > >
> > > > I think if we put this encoder in the same node as the decoder, we
> > > > might as well take this to its natural conclusion and put the entire
> > > > device tree into a single very large node. It's not the same hardware,
> > > > it cannot be modelled as being the same hardware, just because the
> > > > bindings lets people model some separate hardware as the same hardware
> > > > doesn't mean this applies to this hardware.
> > > >
> > > > Long story short, why did I split the encoder to its own node? The
> > > > answer is that I didn't. I simply refused to combine it into a node
> > > > that it has nothing to do with.
> > > >
> > >
> > > As I've mentioned:
> > >
> > > """
> > > the current binding models the idea of decoder and encoder
> > > being the same device. This has never been really really accurate,
> > > as the encoder and decoders have always been more or less independent.
> > >
> > > The reason for having them on a single device are mostly historical,
> > > some old devices shared some resource. I don't think this is the case anymore,
> > > but the binding was still modeled to support that.
> > > """
> > >
> > > The PX30 and RK3399 VPUs are probably pretty independent as well,
> > > and in retrospective, we should have done separated Device Tree nodes.
> > > For historical reasons, we didn't, and we introduced those weird "enc_offset"
> > > and "dec_offset" fields:
> > >
> > > const struct hantro_variant px30_vpu_variant = {
> > >         .enc_offset = 0x0,
> > >         .enc_fmts = rockchip_vpu_enc_fmts,
> > >         .num_enc_fmts = ARRAY_SIZE(rockchip_vpu_enc_fmts),
> > >         .dec_offset = 0x400,
> > >         .dec_fmts = rk3399_vpu_dec_fmts,
> > >
> >
> > As I've mentioned: that doesn't work for this hardware. It's not just the
> > memory addresses. You literally quoted the part where I explain this, and
> > then decided to completely ignore it.
> 
> I didn't ignore anything. I was just trying to explain you,
> how the decoder and the encoder could have been separated for almost
> all the other Rockchip devices, just like you are doing here.
> 
> > I will not explain it again, you
> > have the explanation once more right in this e-mail. Read it.
> >
> > Not to mention that you've also ignored that I disagree with rob's
> > assessment about interrupt-names.
> >
> 
> I didn't ignore it, I just didn't reply to it. You think this is about
> changing a dt-binding, but you are actually introducing a new dt-binding
> since you are adding a new compatible string.
> 
> You are doing so by extending an existing dt-binding.
> 
> I am explaining you the _existing_ dt-binding models the (incorrect) idea
> of a combined decoder and encoder. Since your device is encoder-only
> and has a single interrupt line, you should omit the interrupt-names,
> because it doesn't not add anything.

While in general I agree on single entries don't need -names, given just 
'vdpu' is already allowed I would go with symmetry here and allow it for 
the encoder.

If you wanted to mark 'vdpu' alone as deprecated, then that would be 
fine too. No need for an if/then schema to disallow interrupt-names 
either. Eventually, I plan to (optionally) remove deprecated schemas 
from validation and that would have the effect of requiring 
interrupt-names to have 2 entries here.

Rob

Re: [PATCH v2 3/3] arm64: dts: rockchip: Add Hantro encoder node to rk356x

[Nicolas Frattaroli]

Hi Rob,

On Freitag, 13. Mai 2022 17:23:58 CEST Rob Herring wrote:
> On Fri, May 13, 2022 at 10:07:51AM -0300, Ezequiel Garcia wrote:
> > [...]
> > 
> > I didn't ignore it, I just didn't reply to it. You think this is about
> > changing a dt-binding, but you are actually introducing a new dt-binding
> > since you are adding a new compatible string.
> > 
> > You are doing so by extending an existing dt-binding.
> > 
> > I am explaining you the _existing_ dt-binding models the (incorrect) idea
> > of a combined decoder and encoder. Since your device is encoder-only
> > and has a single interrupt line, you should omit the interrupt-names,
> > because it doesn't not add anything.
> 
> While in general I agree on single entries don't need -names, given just 
> 'vdpu' is already allowed I would go with symmetry here and allow it for 
> the encoder.
> 
> If you wanted to mark 'vdpu' alone as deprecated, then that would be 
> fine too. No need for an if/then schema to disallow interrupt-names 
> either. Eventually, I plan to (optionally) remove deprecated schemas 
> from validation and that would have the effect of requiring 
> interrupt-names to have 2 entries here.
> 
> Rob

After some discussion in the #linux-media IRC channel on OFTC, we've
come to the conclusion that moving forward with a separate binding for
encoder-only instances would probably be a wise move, as the VPU binding
assumes we'll always have a decoder, and breaking this assumption will
just make it more complex the longer we go on not separating these.

While I was writing such a separate binding for v4 of my series, I
discovered that this makes things quite a bit simpler. Especially
looking into the near future, there will be between one to two
additional compatibles that will be added to this binding for the
RK3588, as it has no less than 5 instances capable of encoding
JPEG, and I believe at least one of them is capable of other
formats as well.

Regards,
Nicolas Frattaroli