NVIDIA Tegra20 video decoder driver

drivers/staging/tegra-vde/TODO line 4

Quoted 12 lines · Dmitry Osipenko

drivers/staging/tegra- renamed from drivers/staging/tegra-vde/TODO +0 -0

vde/TODO
new file mode 100644
index 000000000000..e98bbc7b3c19

drivers/staging/tegra-vde/TODO new file +2 -0

@@ -0,0 +1,5 @@
+TODO:
+       - Figure out how generic V4L2 API could be utilized by this

driver,

drivers/staging/tegra-vde/TODO new file +2 -0

+         implement it.
+

That is a very interesting effort, I think it's the first time someone is proposing an upstream driver for a Tegra platform.

tegra_vde_h264_decoder_ctx, it looks like the only thing that the HW is not parsing is the media header (pps/sps). Is that correct ?

I wonder how acceptable it would be to parse this inside the driver. It is no more complex then parsing an EDID. If that was possible, wrapping this driver as a v4l2 mem2mem should be rather simple. As a side effect, you'll automatically get some userspace working, notably GStreamer and FFmpeg.

For the case even parsing the headers is too much from a kernel point of view, then I think you should have a look at the following effort. It's a proposal base on yet to be merged Request API. Hugues is also propose a libv4l2 adapter that makes the driver looks like a normal v4l2 m2m, hiding all the userspace parsing and table filling. This though, is long term plan to integrate state-less or parser-less encoders into linux-media. It seems rather overkill for state-full driver that requires parsed headers like PPS/SPS.

https://lwn.net/Articles/720797/

Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports decoding of CAVLC H.264 only.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 .../bindings/arm/tegra/nvidia,tegra20-vde.txt      |   44 +

 drivers/staging/Kconfig                            |    2 +
 drivers/staging/Makefile                           |    1 +
 drivers/staging/tegra-vde/Kconfig                  |    6 +
 drivers/staging/tegra-vde/Makefile                 |    1 +
 drivers/staging/tegra-vde/TODO                     |    5 +
 drivers/staging/tegra-vde/uapi.h                   |  101 ++
 drivers/staging/tegra-vde/vde.c                    | 1109 ++++++++++++++++++++
 8 files changed, 1269 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt
 create mode 100644 drivers/staging/tegra-vde/Kconfig
 create mode 100644 drivers/staging/tegra-vde/Makefile
 create mode 100644 drivers/staging/tegra-vde/TODO
 create mode 100644 drivers/staging/tegra-vde/uapi.h
 create mode 100644 drivers/staging/tegra-vde/vde.c

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt new file +0 -0

new file mode 100644
index 000000000000..c3f847db8167

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt new file +8 -0

+- reg-names : Must include the following entries:
+  - regs
+  - iram
+- interrupts : Must contain an entry for each entry in interrupt-names.
+- interrupt-names : Must include the following entries:
+  - ucq-error
+  - sync-token
+  - bsev

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt new file +12 -0

+	reg-names = "regs", "iram";
+	interrupts = <GIC_SPI  8 IRQ_TYPE_LEVEL_HIGH>, /* UCQ error interrupt */
+			<GIC_SPI  9 IRQ_TYPE_LEVEL_HIGH>, /* Sync token interrupt */
+			<GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>, /* BSE-V interrupt */
+			<GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>, /* BSE-A interrupt */
+			<GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>; /* SXE interrupt */
+	interrupt-names = "ucq-error", "sync-token", "bsev", "bsea", "sxe";
+	clocks = <&tegra_car TEGRA20_CLK_VDE>;
+	clock-names = "vde";
+	resets = <&tegra_car 61>;
+	reset-names = "vde";
+};

Quoted 1 line

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt +0 -0

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt line 25

Quoted 4 lines

+- resets : Must contain an entry for each entry in reset-names.

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt +3 -0

+  See ../reset/reset.txt for details.
+- reset-names : Must include the following entries:
+  - vde

-names is pointless when there is only one.

On Tue, Oct 17, 2017 at 03:13:54PM -0500, Rob Herring wrote:
[...]

Quoted 1 line

Quoted 1 line

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt +0 -0

[...]

Quoted 5 lines

Quoted 4 lines

+- resets : Must contain an entry for each entry in reset-names.

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt +3 -0

+  See ../reset/reset.txt for details.
+- reset-names : Must include the following entries:
+  - vde

Quoted 1 line

-names is pointless when there is only one.

I'd prefer to keep it. In the past we occasionally had to add clocks or resets to a device tree node where only one had been present (and hence no -names property) and that caused some awkwardness because verbiage had to be added to the bindings that clarified that one particular entry (the original one) always had to come first.

Quoted 9 lines · Thierry Reding

On Tue, Oct 17, 2017 at 03:13:54PM -0500, Rob Herring wrote:
[...]

Quoted 1 line

Quoted 1 line

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt +0 -0

[...]

Quoted 5 lines

Quoted 4 lines

+- resets : Must contain an entry for each entry in reset-names.

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt +3 -0

+  See ../reset/reset.txt for details.
+- reset-names : Must include the following entries:
+  - vde

Quoted 7 lines

Quoted 1 line

-names is pointless when there is only one.

I'd prefer to keep it. In the past we occasionally had to add clocks or resets to a device tree node where only one had been present (and hence no -names property) and that caused some awkwardness because verbiage had to be added to the bindings that clarified that one particular entry (the original one) always had to come first.

The order should be specified regardless of -names and the original one has to come first if you add any. That's not awkwardness, but how bindings work.

Quoted 19 lines · Rob Herring

Quoted 16 lines · Thierry Reding

On Tue, Oct 17, 2017 at 03:13:54PM -0500, Rob Herring wrote:
[...]

Quoted 1 line

Quoted 1 line

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt +0 -0

[...]

Quoted 6 lines

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt line 25

Quoted 4 lines

+- resets : Must contain an entry for each entry in reset-names.

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt +3 -0

+  See ../reset/reset.txt for details.
+- reset-names : Must include the following entries:
+  - vde

-names is pointless when there is only one.

I'd prefer to keep it. In the past we occasionally had to add clocks or resets to a device tree node where only one had been present (and hence no -names property) and that caused some awkwardness because verbiage had to be added to the bindings that clarified that one particular entry (the original one) always had to come first.

Quoted 3 lines

The order should be specified regardless of -names and the original one has to come first if you add any. That's not awkwardness, but how bindings work.

Probably it would be okay to remove '-names' from the binding doc, but keep them in the actual DT, wouldn't it?

Add a device node for the video decoder engine found on Tegra20.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 arch/arm/boot/dts/tegra20.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

arch/arm/boot/dts/tegra20.dtsi +0 -0

index 7c85f97f72ea..1b5d54b6c0cb 100644

Hello Dmitry,

Quoted 12 lines · Dmitry Osipenko

Add a device node for the video decoder engine found on Tegra20.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 arch/arm/boot/dts/tegra20.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

arch/arm/boot/dts/tegra20.dtsi +0 -0

index 7c85f97f72ea..1b5d54b6c0cb 100644

arch/arm/boot/dts/tegra20.dtsi line 255

Quoted 7 lines

arch/arm/boot/dts/tegra20.dtsi +4 -0

@@ -249,6 +249,23 @@
 		*/
 	};
 
+	vde@6001a000 {
+		compatible = "nvidia,tegra20-vde";
+		reg = <0x6001a000 0x3D00    /* VDE registers */
+		       0x40000400 0x3FC00>; /* IRAM region */

this notation of a used region in IRAM is non-standard and potentially it may lead to conflicts for IRAM resource between users.

My proposal is to add a valid device tree node to describe an IRAM region firstly, then reserve a subregion in it by using a new "iram" property.

----8<----
From: Vladimir Zapolskiy <[email protected]>
Date: Thu, 12 Oct 2017 10:25:45 +0300
Subject: [PATCH] ARM: tegra: add device tree node to describe IRAM on Tegra20

All Tegra20 SoCs contain 256KiB IRAM, which is used to store resume code and by a video decoder engine.

Signed-off-by Vladimir Zapolskiy <[email protected]>

---
 arch/arm/boot/dts/tegra20.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

arch/arm/boot/dts/tegra20.dtsi +8 -1

index 7c85f97f72ea..fd2843c90920 100644
@@ -9,6 +9,14 @@
 	compatible = "nvidia,tegra20";
 	interrupt-parent = <&lic>;
 
+	iram@40000000 {
+		compatible = "mmio-sram";
+		reg = <0x40000000 0x40000>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0 0x40000000 0x40000>;
+	};
+
 	host1x@50000000 {
 		compatible = "nvidia,tegra20-host1x", "simple-bus";
 		reg = <0x50000000 0x00024000>;
----8<----

Please add the change above to your next version of the series, or if you wish I can send it separately for review by Thierry.

After applying that change you do define a region in IRAM for the exclusive usage by a video decoder engine and add an 'iram' property:

----8<----

arch/arm/boot/dts/tegra20.dtsi +9 -1

index fd2843c90920..5133fbac2185 100644
@@ -15,6 +15,11 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 		ranges = <0 0x40000000 0x40000>;
+
+		vde_pool: vde {
+			reg = <0x400 0x3fc00>;
+			pool;
+		};
 	};
 
 	host1x@50000000 {
[snip]
 
+	vde@6001a000 {
+		compatible = "nvidia,tegra20-vde";
+		reg = <0x6001a000 0x3d00>;	/* VDE registers */
+		iram = <&vde_pool>;		/* IRAM region */
[snip]
----8<----

And finally in the driver you'll use genalloc API to access the IRAM region, for that you can find ready examples in the kernel source code.

Hello Vladimir,

Quoted 26 lines · Vladimir Zapolskiy

Hello Dmitry,

Quoted 12 lines · Dmitry Osipenko

Add a device node for the video decoder engine found on Tegra20.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 arch/arm/boot/dts/tegra20.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

arch/arm/boot/dts/tegra20.dtsi +0 -0

index 7c85f97f72ea..1b5d54b6c0cb 100644

arch/arm/boot/dts/tegra20.dtsi line 255

Quoted 7 lines

arch/arm/boot/dts/tegra20.dtsi +4 -0

@@ -249,6 +249,23 @@
 		*/
 	};
 
+	vde@6001a000 {
+		compatible = "nvidia,tegra20-vde";
+		reg = <0x6001a000 0x3D00    /* VDE registers */
+		       0x40000400 0x3FC00>; /* IRAM region */

this notation of a used region in IRAM is non-standard and potentially it may lead to conflicts for IRAM resource between users.

Quoted 3 lines

My proposal is to add a valid device tree node to describe an IRAM region firstly, then reserve a subregion in it by using a new "iram" property.

The defined in DT IRAM region used by VDE isn't exactly correct, actually it should be much smaller. I don't know exactly what parts of IRAM VDE uses, for now it is just safer to assign the rest of the IRAM region to VDE.

I'm not sure whether it really worthy to use a dynamic allocator for a single static allocation, but maybe it would come handy later.. Stephen / Jon / Thierry, what do you think?

* PGP Signed by an unknown key

Quoted 33 lines · Dmitry Osipenko

Hello Vladimir,

Quoted 26 lines · Vladimir Zapolskiy

Hello Dmitry,

Quoted 12 lines · Dmitry Osipenko

Add a device node for the video decoder engine found on Tegra20.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 arch/arm/boot/dts/tegra20.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

arch/arm/boot/dts/tegra20.dtsi +0 -0

index 7c85f97f72ea..1b5d54b6c0cb 100644

arch/arm/boot/dts/tegra20.dtsi line 255

Quoted 7 lines

arch/arm/boot/dts/tegra20.dtsi +4 -0

@@ -249,6 +249,23 @@
 		*/
 	};
 
+	vde@6001a000 {
+		compatible = "nvidia,tegra20-vde";
+		reg = <0x6001a000 0x3D00    /* VDE registers */
+		       0x40000400 0x3FC00>; /* IRAM region */

this notation of a used region in IRAM is non-standard and potentially it may lead to conflicts for IRAM resource between users.

Quoted 3 lines

My proposal is to add a valid device tree node to describe an IRAM region firstly, then reserve a subregion in it by using a new "iram" property.

Quoted 7 lines

The defined in DT IRAM region used by VDE isn't exactly correct, actually it should be much smaller. I don't know exactly what parts of IRAM VDE uses, for now it is just safer to assign the rest of the IRAM region to VDE.

I'm not sure whether it really worthy to use a dynamic allocator for a single static allocation, but maybe it would come handy later.. Stephen / Jon / Thierry, what do you think?

This sounds like a good idea. I agree that this currently doesn't seem to be warranted, but consider what would happen if at some point we have more devices requiring access to the IRAM. Spreading individual reg properties all across the DT will make it very difficult to ensure they don't overlap.

Presumably the mmio-sram driver will check that pool don't overlap. Or even if it doesn't it will make it a lot easier to verify because it's all in the same DT node and then consumers only reference it.

I like Vladimir's proposal. I also suspect that Rob may want us to stick to a standardized way referencing such external memory.

Quoted 50 lines · Thierry Reding

* PGP Signed by an unknown key

Quoted 33 lines · Dmitry Osipenko

Hello Vladimir,

Quoted 26 lines · Vladimir Zapolskiy

Hello Dmitry,

Quoted 12 lines · Dmitry Osipenko

Add a device node for the video decoder engine found on Tegra20.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 arch/arm/boot/dts/tegra20.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

arch/arm/boot/dts/tegra20.dtsi +0 -0

index 7c85f97f72ea..1b5d54b6c0cb 100644

arch/arm/boot/dts/tegra20.dtsi line 255

Quoted 7 lines

arch/arm/boot/dts/tegra20.dtsi +4 -0

@@ -249,6 +249,23 @@
 		*/
 	};
 
+	vde@6001a000 {
+		compatible = "nvidia,tegra20-vde";
+		reg = <0x6001a000 0x3D00    /* VDE registers */
+		       0x40000400 0x3FC00>; /* IRAM region */

this notation of a used region in IRAM is non-standard and potentially it may lead to conflicts for IRAM resource between users.

Quoted 3 lines

My proposal is to add a valid device tree node to describe an IRAM region firstly, then reserve a subregion in it by using a new "iram" property.

Quoted 7 lines

The defined in DT IRAM region used by VDE isn't exactly correct, actually it should be much smaller. I don't know exactly what parts of IRAM VDE uses, for now it is just safer to assign the rest of the IRAM region to VDE.

I'm not sure whether it really worthy to use a dynamic allocator for a single static allocation, but maybe it would come handy later.. Stephen / Jon / Thierry, what do you think?

This sounds like a good idea. I agree that this currently doesn't seem to be warranted, but consider what would happen if at some point we have more devices requiring access to the IRAM. Spreading individual reg properties all across the DT will make it very difficult to ensure they don't overlap.

Quoted 6 lines

Presumably the mmio-sram driver will check that pool don't overlap. Or even if it doesn't it will make it a lot easier to verify because it's all in the same DT node and then consumers only reference it.

I like Vladimir's proposal. I also suspect that Rob may want us to stick to a standardized way referencing such external memory.

FWIW I agree. Seems like a nice approach and describes the h/w accurately.

Add a device node for the video decoder engine found on Tegra20.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 arch/arm/boot/dts/tegra20.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

arch/arm/boot/dts/tegra20.dtsi +7 -0

index 7c85f97f72ea..1b5d54b6c0cb 100644
@@ -249,6 +249,23 @@
 		*/
 	};
 
+	vde@6001a000 {
+		compatible = "nvidia,tegra20-vde";
+		reg = <0x6001a000 0x3D00    /* VDE registers */
+		       0x40000400 0x3FC00>; /* IRAM region */
+		reg-names = "regs", "iram";
+		interrupts = <GIC_SPI  8 IRQ_TYPE_LEVEL_HIGH>, /* UCQ error interrupt */
+			     <GIC_SPI  9 IRQ_TYPE_LEVEL_HIGH>, /* Sync token interrupt */

Quoted 27 lines · Dmitry Osipenko

Add a device node for the video decoder engine found on Tegra20.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 arch/arm/boot/dts/tegra20.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

arch/arm/boot/dts/tegra20.dtsi +12 -0

index 7c85f97f72ea..1b5d54b6c0cb 100644
@@ -249,6 +249,23 @@
 		*/
 	};
 
+	vde@6001a000 {
+		compatible = "nvidia,tegra20-vde";
+		reg = <0x6001a000 0x3D00    /* VDE registers */
+		       0x40000400 0x3FC00>; /* IRAM region */
+		reg-names = "regs", "iram";
+		interrupts = <GIC_SPI  8 IRQ_TYPE_LEVEL_HIGH>, /* UCQ error interrupt */
+			     <GIC_SPI  9 IRQ_TYPE_LEVEL_HIGH>, /* Sync token interrupt */
+			     <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>, /* BSE-V interrupt */
+			     <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>, /* BSE-A interrupt */
+			     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>; /* SXE interrupt */
+		interrupt-names = "ucq-error", "sync-token", "bsev", "bsea", "sxe";
+		clocks = <&tegra_car TEGRA20_CLK_VDE>;

arch/arm/boot/dts/tegra20.dtsi line 268

Quoted 5 lines

arch/arm/boot/dts/tegra20.dtsi +5 -0

+		clock-names = "vde";
+		resets = <&tegra_car 61>;
+		reset-names = "vde";
+	};
+

I don't see any binding documentation for this node. We need to make sure we add this.

Quoted 29 lines · Jon Hunter

Quoted 27 lines · Dmitry Osipenko

Add a device node for the video decoder engine found on Tegra20.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 arch/arm/boot/dts/tegra20.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

arch/arm/boot/dts/tegra20.dtsi +12 -0

index 7c85f97f72ea..1b5d54b6c0cb 100644
@@ -249,6 +249,23 @@
 		*/
 	};
 
+	vde@6001a000 {
+		compatible = "nvidia,tegra20-vde";
+		reg = <0x6001a000 0x3D00    /* VDE registers */
+		       0x40000400 0x3FC00>; /* IRAM region */
+		reg-names = "regs", "iram";
+		interrupts = <GIC_SPI  8 IRQ_TYPE_LEVEL_HIGH>, /* UCQ error interrupt */
+			     <GIC_SPI  9 IRQ_TYPE_LEVEL_HIGH>, /* Sync token interrupt */
+			     <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>, /* BSE-V interrupt */
+			     <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>, /* BSE-A interrupt */
+			     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>; /* SXE interrupt */
+		interrupt-names = "ucq-error", "sync-token", "bsev", "bsea", "sxe";
+		clocks = <&tegra_car TEGRA20_CLK_VDE>;

Quoted 9 lines

arch/arm/boot/dts/tegra20.dtsi line 268

Quoted 5 lines

arch/arm/boot/dts/tegra20.dtsi +5 -0

+		clock-names = "vde";
+		resets = <&tegra_car 61>;
+		reset-names = "vde";
+	};
+

I don't see any binding documentation for this node. We need to make sure we add this.

It's in the first patch.

+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt

Quoted 40 lines · Dmitry Osipenko

Quoted 38 lines · Jon Hunter

Quoted 22 lines · Dmitry Osipenko

Add a device node for the video decoder engine found on Tegra20.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 arch/arm/boot/dts/tegra20.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

arch/arm/boot/dts/tegra20.dtsi +7 -0

index 7c85f97f72ea..1b5d54b6c0cb 100644
@@ -249,6 +249,23 @@
 		*/
 	};
 
+	vde@6001a000 {
+		compatible = "nvidia,tegra20-vde";
+		reg = <0x6001a000 0x3D00    /* VDE registers */
+		       0x40000400 0x3FC00>; /* IRAM region */
+		reg-names = "regs", "iram";
+		interrupts = <GIC_SPI  8 IRQ_TYPE_LEVEL_HIGH>, /* UCQ error interrupt */
+			     <GIC_SPI  9 IRQ_TYPE_LEVEL_HIGH>, /* Sync token interrupt */

arch/arm/boot/dts/tegra20.dtsi line 268

Quoted 10 lines

arch/arm/boot/dts/tegra20.dtsi +10 -0

+			     <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>, /* BSE-V interrupt */
+			     <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>, /* BSE-A interrupt */
+			     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>; /* SXE interrupt */
+		interrupt-names = "ucq-error", "sync-token", "bsev", "bsea", "sxe";
+		clocks = <&tegra_car TEGRA20_CLK_VDE>;
+		clock-names = "vde";
+		resets = <&tegra_car 61>;
+		reset-names = "vde";
+	};
+

I don't see any binding documentation for this node. We need to make sure we add this.

Quoted 4 lines

It's in the first patch.

+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt

Ah yes indeed, then that needs to be a separate patch.