Hello,
this is v4 of my RK356x JPEG encoder patch set. It enables the Hantro
hardware encoder of the RK3566 and RK3568 line of SoCs, with JPEG being
the only format the driver currently supports encoding for.
The first patch adds a new binding, the rockchip-vepu binding, to
describe this kind of hardware. The reason for going with a new binding
instead of modifying the vpu binding as the previous versions did is
that the vpu binding is getting quite long, and Ezequiel Garcia suggested
(on IRC) that we could document encoder only instances in their own
binding. This makes sense to me, especially considering that RK3588 will
have more Hantro instances like this to document.
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.
No interdiff this time around, I discovered that it breaks patchwork.
Regards,
Nicolas Frattaroli
Changes in v4:
- bindings: move vepu to its own binding, also add it to MAINTAINERS
- driver: rename a stray rk3568_jpeg_enc_codec_ops to
rk3568_vepu_codec_ops
- devicetree: remove interrupt-names property
- rebase onto linux-next
Changes in v3:
- bindings: change consts to an enum
- bindings: add check to make sure devices with a -vepu compatible only
have the vepu interrupt
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):
media: dt-binding: media: Add rockchip-vepu binding
media: hantro: Add support for RK356x encoder
arm64: dts: rockchip: Add Hantro encoder node to rk356x
.../bindings/media/rockchip-vepu.yaml | 64 +++++++++++++++++++
MAINTAINERS | 1 +
arch/arm64/boot/dts/rockchip/rk356x.dtsi | 20 ++++++
drivers/staging/media/hantro/hantro_drv.c | 1 +
drivers/staging/media/hantro/hantro_hw.h | 1 +
.../staging/media/hantro/rockchip_vpu_hw.c | 25 ++++++++
6 files changed, 112 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/rockchip-vepu.yaml
--
2.36.1
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 <[email protected]>
---
arch/arm64/boot/dts/rockchip/rk356x.dtsi | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
index 1042e68602de..9b4bed7a8a44 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -569,6 +569,26 @@ 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>;
+ 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.1
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 <[email protected]>
---
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 ac232b5f7825..1112e8d0c821 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -638,6 +638,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, },
{ .compatible = "rockchip,rk3568-vpu", .data = &rk3568_vpu_variant, },
#endif
#ifdef CONFIG_VIDEO_HANTRO_IMX8M
diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
index c5dc77125fb7..38988be04d39 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 rk3568_vpu_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 fc96501f3bc8..b39813d53e57 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_vepu_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_vepu_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 rk3568_vpu_variant = {
.dec_offset = 0x400,
.dec_fmts = rk3399_vpu_dec_fmts,
--
2.36.1
The RK3568 and RK3566 have a Hantro VPU node solely dedicated to
encoding. This patch adds a new binding to describe it, as it
does not really fit the rockchip-vpu binding, since there is no
decoder.
Signed-off-by: Nicolas Frattaroli <[email protected]>
---
.../bindings/media/rockchip-vepu.yaml | 64 +++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 65 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/rockchip-vepu.yaml
diff --git a/Documentation/devicetree/bindings/media/rockchip-vepu.yaml b/Documentation/devicetree/bindings/media/rockchip-vepu.yaml
new file mode 100644
index 000000000000..b7ba5bf3517a
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/rockchip-vepu.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/media/rockchip-vepu.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Hantro G1 VPU encoders implemented on Rockchip SoCs
+
+maintainers:
+ - Nicolas Frattaroli <[email protected]>
+
+description:
+ Hantro G1 video encode-only accelerators present on Rockchip SoCs.
+
+properties:
+ compatible:
+ enum:
+ - rockchip,rk3568-vepu
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ maxItems: 2
+
+ clock-names:
+ items:
+ - const: aclk
+ - const: hclk
+
+ power-domains:
+ maxItems: 1
+
+ iommus:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - clock-names
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/rk3568-cru.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/power/rk3568-power.h>
+
+ vepu: video-codec@fdee0000 {
+ compatible = "rockchip,rk3568-vepu";
+ reg = <0x0 0xfdee0000 0x0 0x800>;
+ interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
+ clock-names = "aclk", "hclk";
+ iommus = <&vepu_mmu>;
+ power-domains = <&power RK3568_PD_RGA>;
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 9ce78f2275dc..f901a42e5d0f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8637,6 +8637,7 @@ L: [email protected]
L: [email protected]
S: Maintained
F: Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
+F: Documentation/devicetree/bindings/media/rockchip-vepu.yaml
F: Documentation/devicetree/bindings/media/rockchip-vpu.yaml
F: drivers/staging/media/hantro/
--
2.36.1
On 14/05/2022 15:36, Nicolas Frattaroli wrote:
> The RK3568 and RK3566 have a Hantro VPU node solely dedicated to
> encoding. This patch adds a new binding to describe it, as it
> does not really fit the rockchip-vpu binding, since there is no
> decoder.
>
> Signed-off-by: Nicolas Frattaroli <[email protected]>
> ---
> .../bindings/media/rockchip-vepu.yaml | 64 +++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 65 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/rockchip-vepu.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/rockchip-vepu.yaml b/Documentation/devicetree/bindings/media/rockchip-vepu.yaml
> new file mode 100644
> index 000000000000..b7ba5bf3517a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/rockchip-vepu.yaml
Filename: vendor,device (not hyphen)
It would be actually better if it followed the first compatible, so
"rockchip,rk3568-vepu.yaml"
> @@ -0,0 +1,64 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/media/rockchip-vepu.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Hantro G1 VPU encoders implemented on Rockchip SoCs
> +
> +maintainers:
> + - Nicolas Frattaroli <[email protected]>
> +
> +description:
> + Hantro G1 video encode-only accelerators present on Rockchip SoCs.
> +
> +properties:
> + compatible:
> + enum:
> + - rockchip,rk3568-vepu
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + clocks:
> + maxItems: 2
> +
> + clock-names:
> + items:
> + - const: aclk
> + - const: hclk
Since these are new bindings, it would be good to follow DT convention
and not add common "clk" prefix to clocks. Just like DMA is "tx" not
"txdma". However clock names "a" and "h" are also not good and maybe
this is already shared implementation?
> +
> + power-domains:
> + maxItems: 1
> +
> + iommus:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> + - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/rk3568-cru.h>
Indentation starts at "|" (so four spaces)
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/power/rk3568-power.h>
> +
> + vepu: video-codec@fdee0000 {
four spaces.
> + compatible = "rockchip,rk3568-vepu";
> + reg = <0x0 0xfdee0000 0x0 0x800>;
> + interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> + clock-names = "aclk", "hclk";
> + iommus = <&vepu_mmu>;
> + power-domains = <&power RK3568_PD_RGA>;
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9ce78f2275dc..f901a42e5d0f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8637,6 +8637,7 @@ L: [email protected]
> L: [email protected]
> S: Maintained
> F: Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> +F: Documentation/devicetree/bindings/media/rockchip-vepu.yaml
> F: Documentation/devicetree/bindings/media/rockchip-vpu.yaml
> F: drivers/staging/media/hantro/
>
Best regards,
Krzysztof
Hello
On Samstag, 14. Mai 2022 22:41:29 CEST Krzysztof Kozlowski wrote:
> On 14/05/2022 15:36, Nicolas Frattaroli wrote:
> > The RK3568 and RK3566 have a Hantro VPU node solely dedicated to
> > encoding. This patch adds a new binding to describe it, as it
> > does not really fit the rockchip-vpu binding, since there is no
> > decoder.
> >
> > Signed-off-by: Nicolas Frattaroli <[email protected]>
> > ---
> > .../bindings/media/rockchip-vepu.yaml | 64 +++++++++++++++++++
> > MAINTAINERS | 1 +
> > 2 files changed, 65 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/rockchip-vepu.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/rockchip-vepu.yaml b/Documentation/devicetree/bindings/media/rockchip-vepu.yaml
> > new file mode 100644
> > index 000000000000..b7ba5bf3517a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/rockchip-vepu.yaml
>
> Filename: vendor,device (not hyphen)
> It would be actually better if it followed the first compatible, so
> "rockchip,rk3568-vepu.yaml"
Thanks, will do.
>
> > @@ -0,0 +1,64 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/media/rockchip-vepu.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Hantro G1 VPU encoders implemented on Rockchip SoCs
> > +
> > +maintainers:
> > + - Nicolas Frattaroli <[email protected]>
> > +
> > +description:
> > + Hantro G1 video encode-only accelerators present on Rockchip SoCs.
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - rockchip,rk3568-vepu
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + clocks:
> > + maxItems: 2
> > +
> > + clock-names:
> > + items:
> > + - const: aclk
> > + - const: hclk
>
> Since these are new bindings, it would be good to follow DT convention
> and not add common "clk" prefix to clocks. Just like DMA is "tx" not
> "txdma". However clock names "a" and "h" are also not good and maybe
> this is already shared implementation?
This is indeed a shared implementation. Theoretically I could change
the driver for this one case but that seems pointless, especially
since "aclk" and "hclk" are the usual clk names for AXI and AHB on
ARM as far as I understand. I think I've been told before that those
two clocks should always be called aclk and hclk.
>
> > +
> > + power-domains:
> > + maxItems: 1
> > +
> > + iommus:
> > + maxItems: 1
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - interrupts
> > + - clocks
> > + - clock-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/clock/rk3568-cru.h>
>
> Indentation starts at "|" (so four spaces)
>
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > + #include <dt-bindings/power/rk3568-power.h>
> > +
> > + vepu: video-codec@fdee0000 {
>
> four spaces.
>
> > + compatible = "rockchip,rk3568-vepu";
> > + reg = <0x0 0xfdee0000 0x0 0x800>;
> > + interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&cru ACLK_JENC>, <&cru HCLK_JENC>;
> > + clock-names = "aclk", "hclk";
> > + iommus = <&vepu_mmu>;
> > + power-domains = <&power RK3568_PD_RGA>;
> > + };
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 9ce78f2275dc..f901a42e5d0f 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -8637,6 +8637,7 @@ L: [email protected]
> > L: [email protected]
> > S: Maintained
> > F: Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > +F: Documentation/devicetree/bindings/media/rockchip-vepu.yaml
> > F: Documentation/devicetree/bindings/media/rockchip-vpu.yaml
> > F: drivers/staging/media/hantro/
> >
>
>
> Best regards,
> Krzysztof
>
Thank you for your feedback,
Nicolas Frattaroli
On 12/06/2022 17:05, Nicolas Frattaroli wrote:
>>> +
>>> + clock-names:
>>> + items:
>>> + - const: aclk
>>> + - const: hclk
>>
>> Since these are new bindings, it would be good to follow DT convention
>> and not add common "clk" prefix to clocks. Just like DMA is "tx" not
>> "txdma". However clock names "a" and "h" are also not good and maybe
>> this is already shared implementation?
>
> This is indeed a shared implementation. Theoretically I could change
> the driver for this one case but that seems pointless, especially
> since "aclk" and "hclk" are the usual clk names for AXI and AHB on
> ARM as far as I understand. I think I've been told before that those
> two clocks should always be called aclk and hclk.
>
ok
Best regards,
Krzysztof