[PATCH 0/7] Raspberry Pi HEVC decoder driver

[PATCH 0/7] Raspberry Pi HEVC decoder driver

[Dave Stevenson]

Hi All

This has been in the pipeline for a while, but I've finally cleaned
up our HEVC decoder driver to be in a shape to at least get a first
review.
John Cox has done almost all of the work under contract to Raspberry
Pi, and I'm largely just doing the process of patch curation and
sending.

There are a couple of questions raised in frameworks.
The main one is that the codec has 2 independent phases to the decode,
CABAC and reconstruction. To keep the decoder operating optimally
means that two requests need to be in process at once, whilst the
current frameworks don't want to allow as there is an implicit
assumption of only a single job being active at once, and
completition returns both buffers and releases the media request.

The OUTPUT queue buffer is finished with and can be returned at the
end of phase 1, but the media request is still required for phase 2.
The frameworks currently force the driver to be returning both
together via v4l2_m2m_buf_done_and_job_finish. v4l2_m2m_job_finish
would complete the job without returning the buffer as we need,
however if the driver has set VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF
then we have a WARN in v4l2_m2m_job_finish.
Dropping the WARN as this series is currently doing isn't going to be
the right answer, but it isn't obvious what the right answer is.
Discussion required.

We also have a need to hold on to the media request for phase 2. John
had discussed this with Ezequiel (and others) a couple of years back,
and hence suggested a patch that adds media_request_{pin,unpin} to
grab references on the media request. Discussion required on that
or a better way of handling it.

I will apologise in advance for sending this V1 just before I head off
on the Christmas break, but will respond to things as soon as possible.

Thanks
  Dave

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
Dave Stevenson (4):
      docs: uapi: media: Document Raspberry Pi NV12 column format
      media: ioctl: Add pixel formats NV12MT_COL128 and NV12MT_10_COL128
      media: dt-bindings: media: Add binding for the Raspberry Pi HEVC decoder
      arm: dts: bcm2711-rpi: Add HEVC decoder node

Ezequiel Garcia (1):
      RFC: media: Add media_request_{pin,unpin} API

John Cox (2):
      media: platform: Add Raspberry Pi HEVC decoder driver
      RFC: v4l2-mem2mem: Remove warning from v4l2_m2m_job_finish

 .../bindings/media/raspberrypi,hevc-dec.yaml       |   72 +
 .../userspace-api/media/v4l/pixfmt-yuv-planar.rst  |   42 +
 MAINTAINERS                                        |   10 +
 arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi        |    5 +
 arch/arm/boot/dts/broadcom/bcm2711.dtsi            |    9 +
 drivers/media/mc/mc-request.c                      |   35 +
 drivers/media/platform/raspberrypi/Kconfig         |    1 +
 drivers/media/platform/raspberrypi/Makefile        |    1 +
 .../media/platform/raspberrypi/hevc_dec/Kconfig    |   17 +
 .../media/platform/raspberrypi/hevc_dec/Makefile   |    5 +
 .../media/platform/raspberrypi/hevc_dec/hevc_d.c   |  443 ++++
 .../media/platform/raspberrypi/hevc_dec/hevc_d.h   |  190 ++
 .../platform/raspberrypi/hevc_dec/hevc_d_h265.c    | 2629 ++++++++++++++++++++
 .../platform/raspberrypi/hevc_dec/hevc_d_hw.c      |  376 +++
 .../platform/raspberrypi/hevc_dec/hevc_d_hw.h      |  303 +++
 .../platform/raspberrypi/hevc_dec/hevc_d_video.c   |  685 +++++
 .../platform/raspberrypi/hevc_dec/hevc_d_video.h   |   38 +
 drivers/media/v4l2-core/v4l2-ioctl.c               |    2 +
 drivers/media/v4l2-core/v4l2-mem2mem.c             |    7 -
 include/media/media-request.h                      |   12 +
 include/uapi/linux/videodev2.h                     |    5 +
 21 files changed, 4880 insertions(+), 7 deletions(-)
---
base-commit: e90c9612ac3969cb8206029a26bcd2b6f5d4a942
change-id: 20241212-media-rpi-hevc-dec-3b5be739f3bd

Best regards,
-- 
Dave Stevenson <dave.stevenson@raspberrypi.com>

[PATCH 1/7] RFC: media: Add media_request_{pin,unpin} API

[Dave Stevenson]

From: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

This is probably not the API we will want to add, but it
should show what semantics are needed by drivers.

The goal is to allow the OUTPUT (aka source) buffer and the
controls associated to a request to be released from the request,
and in particular return the OUTPUT buffer back to userspace,
without signalling the media request fd.

This is useful for devices that are able to pre-process
the OUTPUT buffer, therefore able to release it before
the decoding is finished. These drivers should signal
the media request fd only after the CAPTURE buffer is done.

Tested-by: John Cox <john.cox@raspberypi.com>
Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/mc/mc-request.c | 35 +++++++++++++++++++++++++++++++++++
 include/media/media-request.h | 12 ++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c
index 5edfc2791ce7..b5334389d846 100644
--- a/drivers/media/mc/mc-request.c
+++ b/drivers/media/mc/mc-request.c
@@ -499,3 +499,38 @@ void media_request_object_complete(struct media_request_object *obj)
 		media_request_put(req);
 }
 EXPORT_SYMBOL_GPL(media_request_object_complete);
+
+void media_request_pin(struct media_request *req)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&req->lock, flags);
+	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
+		goto unlock;
+	req->num_incomplete_objects++;
+unlock:
+	spin_unlock_irqrestore(&req->lock, flags);
+}
+EXPORT_SYMBOL_GPL(media_request_pin);
+
+void media_request_unpin(struct media_request *req)
+{
+	unsigned long flags;
+	bool completed = false;
+
+	spin_lock_irqsave(&req->lock, flags);
+	if (WARN_ON(!req->num_incomplete_objects) ||
+	    WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
+		goto unlock;
+
+	if (!--req->num_incomplete_objects) {
+		req->state = MEDIA_REQUEST_STATE_COMPLETE;
+		wake_up_interruptible_all(&req->poll_wait);
+		completed = true;
+	}
+unlock:
+	spin_unlock_irqrestore(&req->lock, flags);
+	if (completed)
+		media_request_put(req);
+}
+EXPORT_SYMBOL_GPL(media_request_unpin);
diff --git a/include/media/media-request.h b/include/media/media-request.h
index d4ac557678a7..c48cfb710959 100644
--- a/include/media/media-request.h
+++ b/include/media/media-request.h
@@ -189,6 +189,10 @@ static inline void media_request_get(struct media_request *req)
  */
 void media_request_put(struct media_request *req);
 
+void media_request_pin(struct media_request *req);
+
+void media_request_unpin(struct media_request *req);
+
 /**
  * media_request_get_by_fd - Get a media request by fd
  *
@@ -228,6 +232,14 @@ static inline void media_request_put(struct media_request *req)
 {
 }
 
+static inline void media_request_pin(struct media_request *req)
+{
+}
+
+static inline void media_request_unpin(struct media_request *req)
+{
+}
+
 static inline struct media_request *
 media_request_get_by_fd(struct media_device *mdev, int request_fd)
 {

-- 
2.34.1

[PATCH 2/7] docs: uapi: media: Document Raspberry Pi NV12 column format

[Dave Stevenson]

The Raspberry Pi HEVC decoder uses a tiled format based on
columns for 8 and 10 bit YUV images, so document them as
NV12MT_COL128 and NV12MT_10_COL128.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 .../userspace-api/media/v4l/pixfmt-yuv-planar.rst  | 42 ++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
index b788f6933855..90414491d7b5 100644
--- a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
+++ b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
@@ -827,6 +827,48 @@ Data in the 12 high bits, zeros in the 4 low bits, arranged in little endian ord
       - Cb\ :sub:`11`
       - Cr\ :sub:`11`
 
+NV12MT_COL128 and NV12MT_10_COL128
+----------------------------------
+
+``V4L2_PIX_FMT_NV12MT_COL128`` is a tiled version of
+``V4L2_PIX_FMT_NV12M`` where the two planes are split into 128 byte wide columns
+of Y or interleaved CbCr.
+
+NV12MT_10_COL128 expands that as a 10 bit format where 3 10 bit values are
+packed into a 32bit word. A 128 byte wide column therefore holds 96 samples
+(either Y or interleaved CrCb). That effectively makes it 6 values in a 64 bit
+word for the CbCr plane, as the values always go in pairs.
+
+Bit-packed representation.
+
+.. tabularcolumns:: |p{1.2cm}||p{1.2cm}||p{1.2cm}||p{1.2cm}|p{3.2cm}|p{3.2cm}|
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+    :widths: 8 8 8 8
+
+    * - Y'\ :sub:`00[7:0]`
+      - Y'\ :sub:`01[5:0] (bits 7--2)` Y'\ :sub:`00[9:8]`\ (bits 1--0)
+      - Y'\ :sub:`02[3:0] (bits 7--4)` Y'\ :sub:`01[9:6]`\ (bits 3--0)
+      - unused (bits 7--6)` Y'\ :sub:`02[9:4]`\ (bits 5--0)
+
+.. tabularcolumns:: |p{1.2cm}||p{1.2cm}||p{1.2cm}||p{1.2cm}|p{3.2cm}|p{3.2cm}|
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+    :widths: 12 12 12 12 12 12 12 12
+
+    * - Cb\ :sub:`00[7:0]`
+      - Cr\ :sub:`00[5:0]`\ (bits 7--2) Cb\ :sub:`00[9:8]`\ (bits 1--0)
+      - Cb\ :sub:`01[3:0]`\ (bits 7--4) Cr\ :sub:`00[9:6]`\ (bits 3--0)
+      - unused (bits 7--6) Cb\ :sub:`02[9:4]`\ (bits 5--0)
+      - Cr\ :sub:`01[7:0]`
+      - Cb\ :sub:`02[5:0]`\ (bits 7--2) Cr\ :sub:`01[9:8]`\ (bits 1--0)
+      - Cr\ :sub:`02[3:0]`\ (bits 7--4) Cb\ :sub:`02[9:6]`\ (bits 3--0)
+      - unused (bits 7--6) Cr\ :sub:`02[9:4]`\ (bits 5--0)
+
 
 Fully Planar YUV Formats
 ========================

-- 
2.34.1

[PATCH 3/7] media: ioctl: Add pixel formats NV12MT_COL128 and NV12MT_10_COL128

[Dave Stevenson]

Add V4L2_PIXFMT_NV12MT_COL128 and V4L2_PIXFMT_NV12MT_10_COL128
to describe the Raspberry Pi HEVC decoder NV12 multiplanar formats.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 2 ++
 include/uapi/linux/videodev2.h       | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 0304daa8471d..e510e375a871 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1377,7 +1377,9 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
 	case V4L2_PIX_FMT_NV16M:	descr = "Y/UV 4:2:2 (N-C)"; break;
 	case V4L2_PIX_FMT_NV61M:	descr = "Y/VU 4:2:2 (N-C)"; break;
 	case V4L2_PIX_FMT_NV12MT:	descr = "Y/UV 4:2:0 (64x32 MB, N-C)"; break;
+	case V4L2_PIX_FMT_NV12MT_COL128: descr = "Y/CbCr 4:2:0 (128b cols)"; break;
 	case V4L2_PIX_FMT_NV12MT_16X16:	descr = "Y/UV 4:2:0 (16x16 MB, N-C)"; break;
+	case V4L2_PIX_FMT_NV12MT_10_COL128: descr = "10-bit Y/CbCr 4:2:0 (128b cols)"; break;
 	case V4L2_PIX_FMT_P012M:	descr = "12-bit Y/UV 4:2:0 (N-C)"; break;
 	case V4L2_PIX_FMT_YUV420M:	descr = "Planar YUV 4:2:0 (N-C)"; break;
 	case V4L2_PIX_FMT_YVU420M:	descr = "Planar YVU 4:2:0 (N-C)"; break;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index e7c4dce39007..f8f97aa6a616 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -687,6 +687,11 @@ struct v4l2_pix_format {
 #define V4L2_PIX_FMT_NV12MT_16X16 v4l2_fourcc('V', 'M', '1', '2') /* 12  Y/CbCr 4:2:0 16x16 tiles */
 #define V4L2_PIX_FMT_NV12M_8L128      v4l2_fourcc('N', 'A', '1', '2') /* Y/CbCr 4:2:0 8x128 tiles */
 #define V4L2_PIX_FMT_NV12M_10BE_8L128 v4l2_fourcc_be('N', 'T', '1', '2') /* Y/CbCr 4:2:0 10-bit 8x128 tiles */
+#define V4L2_PIX_FMT_NV12MT_COL128 v4l2_fourcc('N', 'c', '1', '2') /* 12  Y/CbCr 4:2:0 128 pixel wide column */
+#define V4L2_PIX_FMT_NV12MT_10_COL128 v4l2_fourcc('N', 'c', '3', '0')
+								/* Y/CbCr 4:2:0 10bpc, 3x10 packed as 4 bytes in
+								 * a 128 bytes / 96 pixel wide column */
+
 
 /* Bayer formats - see http://www.siliconimaging.com/RGB%20Bayer.htm */
 #define V4L2_PIX_FMT_SBGGR8  v4l2_fourcc('B', 'A', '8', '1') /*  8  BGBG.. GRGR.. */

-- 
2.34.1

[PATCH 4/7] media: dt-bindings: media: Add binding for the Raspberry Pi HEVC decoder

[Dave Stevenson]

Adds a binding for the HEVC decoder found on the BCM2711 / Raspberry Pi 4,
and BCM2712 / Raspberry Pi 5.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 .../bindings/media/raspberrypi,hevc-dec.yaml       | 72 ++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/raspberrypi,hevc-dec.yaml b/Documentation/devicetree/bindings/media/raspberrypi,hevc-dec.yaml
new file mode 100644
index 000000000000..d9e804300297
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/raspberrypi,hevc-dec.yaml
@@ -0,0 +1,72 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/raspberrypi,hevc-dec.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Raspberry Pi HEVC Decoder
+
+maintainers:
+  - John Cox <john.cox@raspberrypi.com>
+  - Dom Cobley <dom@raspberrypi.com>
+  - Dave Stevenson <dave.stevenson@raspberrypi.com>
+  - Raspberry Pi internal review list <kernel-list@raspberrypi.com>
+
+description: |-
+  The Raspberry Pi HEVC decoder is a hardware video decode accelerator block
+  found in the BCM2711 and BCM2712 processors used on Raspberry Pi 4 and 5
+  boards respectively.
+
+properties:
+  compatible:
+    enum:
+      - raspberrypi,hevc-dec
+
+  reg:
+    items:
+      - description: The HEVC main register region
+      - description: The Interrupt control register region
+
+  reg-names:
+    items:
+      - const: intc
+      - const: hevc
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: The HEVC block clock
+
+  clock-names:
+    items:
+      - const: hevc
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    video-codec@7eb10000 {
+        compatible = "raspberrypi,hevc-dec";
+        reg = <0x7eb10000 0x1000>,	/* INTC */
+              <0x7eb00000 0x10000>; /* HEVC */
+        reg-names = "intc",
+                    "hevc";
+
+        interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
+
+        clocks = <&clk 0>;
+        clock-names = "hevc";
+    };
+
+...

-- 
2.34.1

[PATCH 6/7] RFC: v4l2-mem2mem: Remove warning from v4l2_m2m_job_finish

[Dave Stevenson]

From: John Cox <john.cox@raspberrypi.com>

The Raspberry Pi HEVC decoder has a 2 stage pipeline
where the OUTPUT buffer is finished with before the
CAPTURE buffer is ready.

v4l2_m2m_job_finish allows us to do this, however as
the driver handles VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF,
this warn fires on every buffer as drivers that hold capture
buffers are expected to use v4l2_m2m_buf_done_and_job_finish
(introduced with [1]).
That doesn't allow us to handle not returning the destination
buffer.

[1] Commit f8cca8c97a63 ("media: v4l2-mem2mem: support held
capture buffers")

Signed-off-by: John Cox <john.cox@raspberrypi.com>
Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index eb22d6172462..325a518beff7 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -490,13 +490,6 @@ void v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev,
 	unsigned long flags;
 	bool schedule_next;
 
-	/*
-	 * This function should not be used for drivers that support
-	 * holding capture buffers. Those should use
-	 * v4l2_m2m_buf_done_and_job_finish() instead.
-	 */
-	WARN_ON(m2m_ctx->out_q_ctx.q.subsystem_flags &
-		VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF);
 	spin_lock_irqsave(&m2m_dev->job_spinlock, flags);
 	schedule_next = _v4l2_m2m_job_finish(m2m_dev, m2m_ctx);
 	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);

-- 
2.34.1

[PATCH 7/7] arm: dts: bcm2711-rpi: Add HEVC decoder node

[Dave Stevenson]

Add the configuration information for the HEVC decoder.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi | 5 +++++
 arch/arm/boot/dts/broadcom/bcm2711.dtsi     | 9 +++++++++
 2 files changed, 14 insertions(+)

diff --git a/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi b/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi
index 6bf4241fe3b7..43fa2d82cedc 100644
--- a/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi
+++ b/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi
@@ -105,3 +105,8 @@ &vchiq {
 &xhci {
 	power-domains = <&power RPI_POWER_DOMAIN_USB>;
 };
+
+&hevc_dec {
+	clocks = <&firmware_clocks 11>;
+	clock-names = "hevc";
+};
diff --git a/arch/arm/boot/dts/broadcom/bcm2711.dtsi b/arch/arm/boot/dts/broadcom/bcm2711.dtsi
index e4e42af21ef3..0a1f05a26a6a 100644
--- a/arch/arm/boot/dts/broadcom/bcm2711.dtsi
+++ b/arch/arm/boot/dts/broadcom/bcm2711.dtsi
@@ -628,6 +628,15 @@ v3d: gpu@7ec00000 {
 			resets = <&pm BCM2835_RESET_V3D>;
 			interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>;
 		};
+
+		hevc_dec: codec@7eb10000 {
+			compatible = "raspberrypi,hevc-dec";
+			reg = <0x0 0x7eb10000  0x1000>,  /* INTC */
+			      <0x0 0x7eb00000  0x10000>; /* HEVC */
+			reg-names = "intc",
+				    "hevc";
+			interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
+		};
 	};
 };
 

-- 
2.34.1

Re: [PATCH 5/7] media: platform: Add Raspberry Pi HEVC decoder driver

[Dave Stevenson]

On Fri, 20 Dec 2024 at 16:21, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> From: John Cox <john.cox@raspberrypi.com>
>
> The BCM2711 and BCM2712 SoCs used on Rapsberry Pi 4 and Raspberry
> Pi 5 boards include an HEVC decoder block. Add a driver for it.
>
> Signed-off-by: John Cox <john.cox@raspberrypi.com>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  MAINTAINERS                                        |   10 +
>  drivers/media/platform/raspberrypi/Kconfig         |    1 +
>  drivers/media/platform/raspberrypi/Makefile        |    1 +
>  .../media/platform/raspberrypi/hevc_dec/Kconfig    |   17 +
>  .../media/platform/raspberrypi/hevc_dec/Makefile   |    5 +
>  .../media/platform/raspberrypi/hevc_dec/hevc_d.c   |  443 ++++
>  .../media/platform/raspberrypi/hevc_dec/hevc_d.h   |  190 ++
>  .../platform/raspberrypi/hevc_dec/hevc_d_h265.c    | 2629 ++++++++++++++++++++
>  .../platform/raspberrypi/hevc_dec/hevc_d_hw.c      |  376 +++
>  .../platform/raspberrypi/hevc_dec/hevc_d_hw.h      |  303 +++
>  .../platform/raspberrypi/hevc_dec/hevc_d_video.c   |  685 +++++
>  .../platform/raspberrypi/hevc_dec/hevc_d_video.h   |   38 +
>  12 files changed, 4698 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a33a97d5ffff..569b478c44fe 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19567,6 +19567,16 @@ L:     linux-edac@vger.kernel.org
>  S:     Maintained
>  F:     drivers/ras/amd/fmpm.c
>
<snip>

Typical. You read a patch multiple times over, and still miss a "git
add" of a new file

I won't send a new version now as there are bound to be multiple other
things in review, but hevc_d_h265.h should read

// SPDX-License-Identifier: GPL-2.0-or-later
/*
 * Raspberry Pi HEVC driver
 *
 * Copyright (C) 2024 Raspberry Pi Ltd
 *
 */

#ifndef _HEVC_D_H265_H_
#define _HEVC_D_H265_H_
#include "hevc_d.h"

extern const struct v4l2_ctrl_ops hevc_d_hevc_sps_ctrl_ops;
extern const struct v4l2_ctrl_ops hevc_d_hevc_pps_ctrl_ops;

void hevc_d_h265_setup(struct hevc_d_ctx *ctx, struct hevc_d_run *run);
int hevc_d_h265_start(struct hevc_d_ctx *ctx);
void hevc_d_h265_stop(struct hevc_d_ctx *ctx);
void hevc_d_h265_trigger(struct hevc_d_ctx *ctx);

void hevc_d_device_run(void *priv);

#endif

Thanks
  Dave

Re: [PATCH 5/7] media: platform: Add Raspberry Pi HEVC decoder driver

[kernel test robot]

Hi Dave,

kernel test robot noticed the following build warnings:

[auto build test WARNING on e90c9612ac3969cb8206029a26bcd2b6f5d4a942]

url:    https://github.com/intel-lab-lkp/linux/commits/Dave-Stevenson/RFC-media-Add-media_request_-pin-unpin-API/20241221-002633
base:   e90c9612ac3969cb8206029a26bcd2b6f5d4a942
patch link:    https://lore.kernel.org/r/20241220-media-rpi-hevc-dec-v1-5-0ebcc04ed42e%40raspberrypi.com
patch subject: [PATCH 5/7] media: platform: Add Raspberry Pi HEVC decoder driver
reproduce: (https://download.01.org/0day-ci/archive/20241221/202412210220.jZ2hzj8t-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412210220.jZ2hzj8t-lkp@intel.com/

All warnings (new ones prefixed by >>):

   Warning: Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml references a file that doesn't exist: Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
   Warning: Documentation/hwmon/g762.rst references a file that doesn't exist: Documentation/devicetree/bindings/hwmon/g762.txt
   Warning: Documentation/hwmon/isl28022.rst references a file that doesn't exist: Documentation/devicetree/bindings/hwmon/isl,isl28022.yaml
   Warning: Documentation/translations/ja_JP/SubmittingPatches references a file that doesn't exist: linux-2.6.12-vanilla/Documentation/dontdiff
   Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>> Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/media/raspberrypi,rpi_hevc_dec.yaml
   Warning: lib/Kconfig.debug references a file that doesn't exist: Documentation/dev-tools/fault-injection/fault-injection.rst
   Using alabaster theme

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 5/7] media: platform: Add Raspberry Pi HEVC decoder driver

[kernel test robot]

Hi Dave,

kernel test robot noticed the following build errors:

[auto build test ERROR on e90c9612ac3969cb8206029a26bcd2b6f5d4a942]

url:    https://github.com/intel-lab-lkp/linux/commits/Dave-Stevenson/RFC-media-Add-media_request_-pin-unpin-API/20241221-002633
base:   e90c9612ac3969cb8206029a26bcd2b6f5d4a942
patch link:    https://lore.kernel.org/r/20241220-media-rpi-hevc-dec-v1-5-0ebcc04ed42e%40raspberrypi.com
patch subject: [PATCH 5/7] media: platform: Add Raspberry Pi HEVC decoder driver
config: sh-allyesconfig (https://download.01.org/0day-ci/archive/20241222/202412220238.0kZEBbMb-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241222/202412220238.0kZEBbMb-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412220238.0kZEBbMb-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/media/platform/raspberrypi/hevc_dec/hevc_d.c:24:10: fatal error: hevc_d_h265.h: No such file or directory
      24 | #include "hevc_d_h265.h"
         |          ^~~~~~~~~~~~~~~
   compilation terminated.
--
>> drivers/media/platform/raspberrypi/hevc_dec/hevc_d_video.c:21:10: fatal error: hevc_d_h265.h: No such file or directory
      21 | #include "hevc_d_h265.h"
         |          ^~~~~~~~~~~~~~~
   compilation terminated.
--
>> drivers/media/platform/raspberrypi/hevc_dec/hevc_d_h265.c:20:10: fatal error: hevc_d_h265.h: No such file or directory
      20 | #include "hevc_d_h265.h"
         |          ^~~~~~~~~~~~~~~
   compilation terminated.


vim +24 drivers/media/platform/raspberrypi/hevc_dec/hevc_d.c

    22	
    23	#include "hevc_d.h"
  > 24	#include "hevc_d_h265.h"
    25	#include "hevc_d_video.h"
    26	#include "hevc_d_hw.h"
    27	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 5/7] media: platform: Add Raspberry Pi HEVC decoder driver

[kernel test robot]

Hi Dave,

kernel test robot noticed the following build errors:

[auto build test ERROR on e90c9612ac3969cb8206029a26bcd2b6f5d4a942]

url:    https://github.com/intel-lab-lkp/linux/commits/Dave-Stevenson/RFC-media-Add-media_request_-pin-unpin-API/20241221-002633
base:   e90c9612ac3969cb8206029a26bcd2b6f5d4a942
patch link:    https://lore.kernel.org/r/20241220-media-rpi-hevc-dec-v1-5-0ebcc04ed42e%40raspberrypi.com
patch subject: [PATCH 5/7] media: platform: Add Raspberry Pi HEVC decoder driver
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20241222/202412220205.Gf2N9CUb-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241222/202412220205.Gf2N9CUb-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412220205.Gf2N9CUb-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/media/platform/raspberrypi/hevc_dec/hevc_d.c:14:
   In file included from include/linux/platform_device.h:13:
   In file included from include/linux/device.h:32:
   In file included from include/linux/device/driver.h:21:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/s390/include/asm/elf.h:181:
   In file included from arch/s390/include/asm/mmu_context.h:11:
   In file included from arch/s390/include/asm/pgalloc.h:18:
   In file included from include/linux/mm.h:2223:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/media/platform/raspberrypi/hevc_dec/hevc_d.c:24:10: fatal error: 'hevc_d_h265.h' file not found
      24 | #include "hevc_d_h265.h"
         |          ^~~~~~~~~~~~~~~
   4 warnings and 1 error generated.
--
   In file included from drivers/media/platform/raspberrypi/hevc_dec/hevc_d_video.c:14:
   In file included from include/media/videobuf2-dma-contig.h:16:
   In file included from include/media/videobuf2-v4l2.h:16:
   In file included from include/media/videobuf2-core.h:18:
   In file included from include/linux/dma-buf.h:19:
   In file included from include/linux/scatterlist.h:8:
   In file included from include/linux/mm.h:2223:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/media/platform/raspberrypi/hevc_dec/hevc_d_video.c:21:10: fatal error: 'hevc_d_h265.h' file not found
      21 | #include "hevc_d_h265.h"
         |          ^~~~~~~~~~~~~~~
   4 warnings and 1 error generated.
--
   In file included from drivers/media/platform/raspberrypi/hevc_dec/hevc_d_h265.c:17:
   In file included from include/media/videobuf2-dma-contig.h:16:
   In file included from include/media/videobuf2-v4l2.h:16:
   In file included from include/media/videobuf2-core.h:18:
   In file included from include/linux/dma-buf.h:19:
   In file included from include/linux/scatterlist.h:8:
   In file included from include/linux/mm.h:2223:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/media/platform/raspberrypi/hevc_dec/hevc_d_h265.c:20:10: fatal error: 'hevc_d_h265.h' file not found
      20 | #include "hevc_d_h265.h"
         |          ^~~~~~~~~~~~~~~
   4 warnings and 1 error generated.


vim +24 drivers/media/platform/raspberrypi/hevc_dec/hevc_d.c

    22	
    23	#include "hevc_d.h"
  > 24	#include "hevc_d_h265.h"
    25	#include "hevc_d_video.h"
    26	#include "hevc_d_hw.h"
    27	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 4/7] media: dt-bindings: media: Add binding for the Raspberry Pi HEVC decoder

[Rob Herring]

On Fri, Dec 20, 2024 at 04:21:15PM +0000, Dave Stevenson wrote:
> Adds a binding for the HEVC decoder found on the BCM2711 / Raspberry Pi 4,
> and BCM2712 / Raspberry Pi 5.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  .../bindings/media/raspberrypi,hevc-dec.yaml       | 72 ++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/raspberrypi,hevc-dec.yaml b/Documentation/devicetree/bindings/media/raspberrypi,hevc-dec.yaml
> new file mode 100644
> index 000000000000..d9e804300297
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/raspberrypi,hevc-dec.yaml
> @@ -0,0 +1,72 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/raspberrypi,hevc-dec.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Raspberry Pi HEVC Decoder
> +
> +maintainers:
> +  - John Cox <john.cox@raspberrypi.com>
> +  - Dom Cobley <dom@raspberrypi.com>
> +  - Dave Stevenson <dave.stevenson@raspberrypi.com>
> +  - Raspberry Pi internal review list <kernel-list@raspberrypi.com>
> +
> +description: |-

Don't need '|-' if no formatting to preserve.

> +  The Raspberry Pi HEVC decoder is a hardware video decode accelerator block
> +  found in the BCM2711 and BCM2712 processors used on Raspberry Pi 4 and 5
> +  boards respectively.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - raspberrypi,hevc-dec
> +
> +  reg:
> +    items:
> +      - description: The HEVC main register region
> +      - description: The Interrupt control register region
> +
> +  reg-names:
> +    items:
> +      - const: intc
> +      - const: hevc

Doesn't match the description in reg.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: The HEVC block clock
> +
> +  clock-names:
> +    items:
> +      - const: hevc

Not really useful when there is only 1 possible entry and also when it 
is optional.

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - clocks
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    video-codec@7eb10000 {
> +        compatible = "raspberrypi,hevc-dec";
> +        reg = <0x7eb10000 0x1000>,	/* INTC */
> +              <0x7eb00000 0x10000>; /* HEVC */
> +        reg-names = "intc",
> +                    "hevc";
> +
> +        interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
> +
> +        clocks = <&clk 0>;
> +        clock-names = "hevc";
> +    };
> +
> +...
> 
> -- 
> 2.34.1
> 

Re: [PATCH 3/7] media: ioctl: Add pixel formats NV12MT_COL128 and NV12MT_10_COL128

[Dave Stevenson]

Hi Robert

Resending this reply as replying from my phone before the Christmas
break sent it as HTML :-(

On Sat, 21 Dec 2024 at 17:38, Robert Mader <robert.mader@collabora.com> wrote:
>
> Hi, thanks for the patch.
>
> On 20.12.24 17:21, Dave Stevenson wrote:
>
> Add V4L2_PIXFMT_NV12MT_COL128 and V4L2_PIXFMT_NV12MT_10_COL128
> to describe the Raspberry Pi HEVC decoder NV12 multiplanar formats.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/v4l2-core/v4l2-ioctl.c | 2 ++
>  include/uapi/linux/videodev2.h       | 5 +++++
>  2 files changed, 7 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 0304daa8471d..e510e375a871 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1377,7 +1377,9 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
>   case V4L2_PIX_FMT_NV16M: descr = "Y/UV 4:2:2 (N-C)"; break;
>   case V4L2_PIX_FMT_NV61M: descr = "Y/VU 4:2:2 (N-C)"; break;
>   case V4L2_PIX_FMT_NV12MT: descr = "Y/UV 4:2:0 (64x32 MB, N-C)"; break;
> + case V4L2_PIX_FMT_NV12MT_COL128: descr = "Y/CbCr 4:2:0 (128b cols)"; break;
>   case V4L2_PIX_FMT_NV12MT_16X16: descr = "Y/UV 4:2:0 (16x16 MB, N-C)"; break;
> + case V4L2_PIX_FMT_NV12MT_10_COL128: descr = "10-bit Y/CbCr 4:2:0 (128b cols)"; break;
>   case V4L2_PIX_FMT_P012M: descr = "12-bit Y/UV 4:2:0 (N-C)"; break;
>   case V4L2_PIX_FMT_YUV420M: descr = "Planar YUV 4:2:0 (N-C)"; break;
>   case V4L2_PIX_FMT_YVU420M: descr = "Planar YVU 4:2:0 (N-C)"; break;
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index e7c4dce39007..f8f97aa6a616 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -687,6 +687,11 @@ struct v4l2_pix_format {
>  #define V4L2_PIX_FMT_NV12MT_16X16 v4l2_fourcc('V', 'M', '1', '2') /* 12  Y/CbCr 4:2:0 16x16 tiles */
>  #define V4L2_PIX_FMT_NV12M_8L128      v4l2_fourcc('N', 'A', '1', '2') /* Y/CbCr 4:2:0 8x128 tiles */
>  #define V4L2_PIX_FMT_NV12M_10BE_8L128 v4l2_fourcc_be('N', 'T', '1', '2') /* Y/CbCr 4:2:0 10-bit 8x128 tiles */
> +#define V4L2_PIX_FMT_NV12MT_COL128 v4l2_fourcc('N', 'c', '1', '2') /* 12  Y/CbCr 4:2:0 128 pixel wide column */
> +#define V4L2_PIX_FMT_NV12MT_10_COL128 v4l2_fourcc('N', 'c', '3', '0')
>
> Should these be upper-case Cs instead? So they compatible with the previously used downstream values?

No, this is deliberate.

Downstream was using a single planar format, with extra complexity for
determining the chroma offset per column, and weird handling required
on bytesperline.
Having had discussions, switching to a multiplanar format (hence MT in
the name) removes those complexities and means we don't need to do
anything weird in the v4l2 format definitions.

Reusing NC12 and NC30 fourccs will give us grief over backwards
compatibility, hence lower case for the new version.

I have a patch on [1] that adds back in NC12 and NC30 for downstream
just so we don't break existing users, but see no point in upstreaming
that.

> P.S.: Coincidentally Gstreamer just landed support for NC12 in https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/7355 and there is also a link to experimental NC30 patches. So happy to see this series appear upstream :)

Ooh, nice. I'll give it a test when I'm back in the office.

Dave

[1] https://github.com/6by9/linux/commits/rpi-6.12.y-hevc_dec/

> + /* Y/CbCr 4:2:0 10bpc, 3x10 packed as 4 bytes in
> + * a 128 bytes / 96 pixel wide column */
> +
>
>  /* Bayer formats - see http://www.siliconimaging.com/RGB%20Bayer.htm */
>  #define V4L2_PIX_FMT_SBGGR8  v4l2_fourcc('B', 'A', '8', '1') /*  8  BGBG.. GRGR.. */
>

Re: [PATCH 1/7] RFC: media: Add media_request_{pin,unpin} API

[Nicolas Dufresne]

Hi Dave,

I'm very happy to see this effort being resumed.

Le vendredi 20 décembre 2024 à 16:21 +0000, Dave Stevenson a écrit :
> From: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> 
> This is probably not the API we will want to add, but it
> should show what semantics are needed by drivers.
> 
> The goal is to allow the OUTPUT (aka source) buffer and the
> controls associated to a request to be released from the request,
> and in particular return the OUTPUT buffer back to userspace,
> without signalling the media request fd.

My impression is that Hans proposal for media_request_manual_complete() is
better align to the direction we want to take to solve this issue. Mediatek and
Sebastian are currently experimenting with it for the MTK VCODEC driver, which
also have two stages (but in two cores).

https://lore.kernel.org/linux-media/cover.1724928939.git.hverkuil-cisco@xs4all.nl/

I'm not sure if you noticed this work, but having from more feedback on it would
be nice. So far the main challenge we have hit is that it can be difficult to
use when the first stages completes before the device_run function finishes
(reentrant). But we believe its MTK VCODEC software design that introduce this
complexity. With the 2 stage single core design, that issue won't exist, so my
impression is that this should be straightforward for you.

Nicolas

> 
> This is useful for devices that are able to pre-process
> the OUTPUT buffer, therefore able to release it before
> the decoding is finished. These drivers should signal
> the media request fd only after the CAPTURE buffer is done.
> 
> Tested-by: John Cox <john.cox@raspberypi.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/mc/mc-request.c | 35 +++++++++++++++++++++++++++++++++++
>  include/media/media-request.h | 12 ++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c
> index 5edfc2791ce7..b5334389d846 100644
> --- a/drivers/media/mc/mc-request.c
> +++ b/drivers/media/mc/mc-request.c
> @@ -499,3 +499,38 @@ void media_request_object_complete(struct media_request_object *obj)
>  		media_request_put(req);
>  }
>  EXPORT_SYMBOL_GPL(media_request_object_complete);
> +
> +void media_request_pin(struct media_request *req)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&req->lock, flags);
> +	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
> +		goto unlock;
> +	req->num_incomplete_objects++;
> +unlock:
> +	spin_unlock_irqrestore(&req->lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(media_request_pin);
> +
> +void media_request_unpin(struct media_request *req)
> +{
> +	unsigned long flags;
> +	bool completed = false;
> +
> +	spin_lock_irqsave(&req->lock, flags);
> +	if (WARN_ON(!req->num_incomplete_objects) ||
> +	    WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
> +		goto unlock;
> +
> +	if (!--req->num_incomplete_objects) {
> +		req->state = MEDIA_REQUEST_STATE_COMPLETE;
> +		wake_up_interruptible_all(&req->poll_wait);
> +		completed = true;
> +	}
> +unlock:
> +	spin_unlock_irqrestore(&req->lock, flags);
> +	if (completed)
> +		media_request_put(req);
> +}
> +EXPORT_SYMBOL_GPL(media_request_unpin);
> diff --git a/include/media/media-request.h b/include/media/media-request.h
> index d4ac557678a7..c48cfb710959 100644
> --- a/include/media/media-request.h
> +++ b/include/media/media-request.h
> @@ -189,6 +189,10 @@ static inline void media_request_get(struct media_request *req)
>   */
>  void media_request_put(struct media_request *req);
>  
> +void media_request_pin(struct media_request *req);
> +
> +void media_request_unpin(struct media_request *req);
> +
>  /**
>   * media_request_get_by_fd - Get a media request by fd
>   *
> @@ -228,6 +232,14 @@ static inline void media_request_put(struct media_request *req)
>  {
>  }
>  
> +static inline void media_request_pin(struct media_request *req)
> +{
> +}
> +
> +static inline void media_request_unpin(struct media_request *req)
> +{
> +}
> +
>  static inline struct media_request *
>  media_request_get_by_fd(struct media_device *mdev, int request_fd)
>  {
> 

Re: [PATCH 0/7] Raspberry Pi HEVC decoder driver

[Nicolas Dufresne]

Hi Dave,

Le vendredi 20 décembre 2024 à 16:21 +0000, Dave Stevenson a écrit :
> Hi All
> 
> This has been in the pipeline for a while, but I've finally cleaned
> up our HEVC decoder driver to be in a shape to at least get a first
> review.
> John Cox has done almost all of the work under contract to Raspberry
> Pi, and I'm largely just doing the process of patch curation and
> sending.
> 
> There are a couple of questions raised in frameworks.
> The main one is that the codec has 2 independent phases to the decode,
> CABAC and reconstruction. To keep the decoder operating optimally
> means that two requests need to be in process at once, whilst the
> current frameworks don't want to allow as there is an implicit
> assumption of only a single job being active at once, and
> completition returns both buffers and releases the media request.
> 
> The OUTPUT queue buffer is finished with and can be returned at the
> end of phase 1, but the media request is still required for phase 2.
> The frameworks currently force the driver to be returning both
> together via v4l2_m2m_buf_done_and_job_finish. v4l2_m2m_job_finish
> would complete the job without returning the buffer as we need,
> however if the driver has set VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF
> then we have a WARN in v4l2_m2m_job_finish.
> Dropping the WARN as this series is currently doing isn't going to be
> the right answer, but it isn't obvious what the right answer is.
> Discussion required.

I think part of the manual request completion RFC will be to evaluate the impact
on VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF feature. MTK does not support
interleaved interlaced decoding (only alternate), so they didn't have to
implement that feature.

Overall, It would be nice to get your feedback on the new manual request
proposal, which is I believe better then the pin/unpin API you have in this
serie.

> 
> We also have a need to hold on to the media request for phase 2. John
> had discussed this with Ezequiel (and others) a couple of years back,
> and hence suggested a patch that adds media_request_{pin,unpin} to
> grab references on the media request. Discussion required on that
> or a better way of handling it.
> 
> I will apologise in advance for sending this V1 just before I head off
> on the Christmas break, but will respond to things as soon as possible.

One thing missing in this summary is how this driver is being validated
(specially that for this one requires a downstream fork of FFMPEG). To this
report we ask for:

- v4l2-compliance results
- Fluster conformance tests results [1] and I believe you need [2]

[1] https://github.com/fluendo/fluster
[2] https://github.com/fluendo/fluster/pull/179

GStreamer support is there in main now, but without the needed software video
converter for you column tiling, we can't use it for that (i.e. only works
through GL or Wayland).

regards,
Nicolas

> 
> Thanks
>   Dave
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
> Dave Stevenson (4):
>       docs: uapi: media: Document Raspberry Pi NV12 column format
>       media: ioctl: Add pixel formats NV12MT_COL128 and NV12MT_10_COL128
>       media: dt-bindings: media: Add binding for the Raspberry Pi HEVC decoder
>       arm: dts: bcm2711-rpi: Add HEVC decoder node
> 
> Ezequiel Garcia (1):
>       RFC: media: Add media_request_{pin,unpin} API
> 
> John Cox (2):
>       media: platform: Add Raspberry Pi HEVC decoder driver
>       RFC: v4l2-mem2mem: Remove warning from v4l2_m2m_job_finish
> 
>  .../bindings/media/raspberrypi,hevc-dec.yaml       |   72 +
>  .../userspace-api/media/v4l/pixfmt-yuv-planar.rst  |   42 +
>  MAINTAINERS                                        |   10 +
>  arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi        |    5 +
>  arch/arm/boot/dts/broadcom/bcm2711.dtsi            |    9 +
>  drivers/media/mc/mc-request.c                      |   35 +
>  drivers/media/platform/raspberrypi/Kconfig         |    1 +
>  drivers/media/platform/raspberrypi/Makefile        |    1 +
>  .../media/platform/raspberrypi/hevc_dec/Kconfig    |   17 +
>  .../media/platform/raspberrypi/hevc_dec/Makefile   |    5 +
>  .../media/platform/raspberrypi/hevc_dec/hevc_d.c   |  443 ++++
>  .../media/platform/raspberrypi/hevc_dec/hevc_d.h   |  190 ++
>  .../platform/raspberrypi/hevc_dec/hevc_d_h265.c    | 2629 ++++++++++++++++++++
>  .../platform/raspberrypi/hevc_dec/hevc_d_hw.c      |  376 +++
>  .../platform/raspberrypi/hevc_dec/hevc_d_hw.h      |  303 +++
>  .../platform/raspberrypi/hevc_dec/hevc_d_video.c   |  685 +++++
>  .../platform/raspberrypi/hevc_dec/hevc_d_video.h   |   38 +
>  drivers/media/v4l2-core/v4l2-ioctl.c               |    2 +
>  drivers/media/v4l2-core/v4l2-mem2mem.c             |    7 -
>  include/media/media-request.h                      |   12 +
>  include/uapi/linux/videodev2.h                     |    5 +
>  21 files changed, 4880 insertions(+), 7 deletions(-)
> ---
> base-commit: e90c9612ac3969cb8206029a26bcd2b6f5d4a942
> change-id: 20241212-media-rpi-hevc-dec-3b5be739f3bd
> 
> Best regards,

Re: [PATCH 3/7] media: ioctl: Add pixel formats NV12MT_COL128 and NV12MT_10_COL128

[Nicolas Dufresne]

Le jeudi 02 janvier 2025 à 12:52 +0000, Dave Stevenson a écrit :
> Hi Robert
> 
> Resending this reply as replying from my phone before the Christmas
> break sent it as HTML :-(
> 
> On Sat, 21 Dec 2024 at 17:38, Robert Mader <robert.mader@collabora.com> wrote:
> > 
> > Hi, thanks for the patch.
> > 
> > On 20.12.24 17:21, Dave Stevenson wrote:
> > 
> > Add V4L2_PIXFMT_NV12MT_COL128 and V4L2_PIXFMT_NV12MT_10_COL128
> > to describe the Raspberry Pi HEVC decoder NV12 multiplanar formats.
> > 
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-ioctl.c | 2 ++
> >  include/uapi/linux/videodev2.h       | 5 +++++
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > index 0304daa8471d..e510e375a871 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -1377,7 +1377,9 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
> >   case V4L2_PIX_FMT_NV16M: descr = "Y/UV 4:2:2 (N-C)"; break;
> >   case V4L2_PIX_FMT_NV61M: descr = "Y/VU 4:2:2 (N-C)"; break;
> >   case V4L2_PIX_FMT_NV12MT: descr = "Y/UV 4:2:0 (64x32 MB, N-C)"; break;
> > + case V4L2_PIX_FMT_NV12MT_COL128: descr = "Y/CbCr 4:2:0 (128b cols)"; break;
> >   case V4L2_PIX_FMT_NV12MT_16X16: descr = "Y/UV 4:2:0 (16x16 MB, N-C)"; break;
> > + case V4L2_PIX_FMT_NV12MT_10_COL128: descr = "10-bit Y/CbCr 4:2:0 (128b cols)"; break;
> >   case V4L2_PIX_FMT_P012M: descr = "12-bit Y/UV 4:2:0 (N-C)"; break;
> >   case V4L2_PIX_FMT_YUV420M: descr = "Planar YUV 4:2:0 (N-C)"; break;
> >   case V4L2_PIX_FMT_YVU420M: descr = "Planar YVU 4:2:0 (N-C)"; break;
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index e7c4dce39007..f8f97aa6a616 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -687,6 +687,11 @@ struct v4l2_pix_format {
> >  #define V4L2_PIX_FMT_NV12MT_16X16 v4l2_fourcc('V', 'M', '1', '2') /* 12  Y/CbCr 4:2:0 16x16 tiles */
> >  #define V4L2_PIX_FMT_NV12M_8L128      v4l2_fourcc('N', 'A', '1', '2') /* Y/CbCr 4:2:0 8x128 tiles */
> >  #define V4L2_PIX_FMT_NV12M_10BE_8L128 v4l2_fourcc_be('N', 'T', '1', '2') /* Y/CbCr 4:2:0 10-bit 8x128 tiles */
> > +#define V4L2_PIX_FMT_NV12MT_COL128 v4l2_fourcc('N', 'c', '1', '2') /* 12  Y/CbCr 4:2:0 128 pixel wide column */
> > +#define V4L2_PIX_FMT_NV12MT_10_COL128 v4l2_fourcc('N', 'c', '3', '0')
> > 
> > Should these be upper-case Cs instead? So they compatible with the previously used downstream values?
> 
> No, this is deliberate.
> 
> Downstream was using a single planar format, with extra complexity for
> determining the chroma offset per column, and weird handling required
> on bytesperline.
> Having had discussions, switching to a multiplanar format (hence MT in
> the name) removes those complexities and means we don't need to do
> anything weird in the v4l2 format definitions.
> 
> Reusing NC12 and NC30 fourccs will give us grief over backwards
> compatibility, hence lower case for the new version.
> 
> I have a patch on [1] that adds back in NC12 and NC30 for downstream
> just so we don't break existing users, but see no point in upstreaming
> that.

Yes, I think its fair to avoid incompatibility there. Are there matching Mesa
patches coming, since without that we are back to square one, where the format
remains unusable. NC12 have a matching (mainline) DRM_FORMAT_NV12 
DRM_FORMAT_MOD_BROADCOM_SAND128 pair. I believe a new modifier is needed and
will serve both Nc12/30.

Nicolas

> 
> > P.S.: Coincidentally Gstreamer just landed support for NC12 in https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/7355 and there is also a link to experimental NC30 patches. So happy to see this series appear upstream :)
> 
> Ooh, nice. I'll give it a test when I'm back in the office.
> 
> Dave
> 
> [1] https://github.com/6by9/linux/commits/rpi-6.12.y-hevc_dec/
> 
> > + /* Y/CbCr 4:2:0 10bpc, 3x10 packed as 4 bytes in
> > + * a 128 bytes / 96 pixel wide column */
> > +
> > 
> >  /* Bayer formats - see http://www.siliconimaging.com/RGB%20Bayer.htm */
> >  #define V4L2_PIX_FMT_SBGGR8  v4l2_fourcc('B', 'A', '8', '1') /*  8  BGBG.. GRGR.. */
> > 
> 

Re: [PATCH 0/7] Raspberry Pi HEVC decoder driver

[Dave Stevenson]

Hi Nicolas

On Mon, 6 Jan 2025 at 20:46, Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Hi Dave,
>
> Le vendredi 20 décembre 2024 à 16:21 +0000, Dave Stevenson a écrit :
> > Hi All
> >
> > This has been in the pipeline for a while, but I've finally cleaned
> > up our HEVC decoder driver to be in a shape to at least get a first
> > review.
> > John Cox has done almost all of the work under contract to Raspberry
> > Pi, and I'm largely just doing the process of patch curation and
> > sending.
> >
> > There are a couple of questions raised in frameworks.
> > The main one is that the codec has 2 independent phases to the decode,
> > CABAC and reconstruction. To keep the decoder operating optimally
> > means that two requests need to be in process at once, whilst the
> > current frameworks don't want to allow as there is an implicit
> > assumption of only a single job being active at once, and
> > completition returns both buffers and releases the media request.
> >
> > The OUTPUT queue buffer is finished with and can be returned at the
> > end of phase 1, but the media request is still required for phase 2.
> > The frameworks currently force the driver to be returning both
> > together via v4l2_m2m_buf_done_and_job_finish. v4l2_m2m_job_finish
> > would complete the job without returning the buffer as we need,
> > however if the driver has set VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF
> > then we have a WARN in v4l2_m2m_job_finish.
> > Dropping the WARN as this series is currently doing isn't going to be
> > the right answer, but it isn't obvious what the right answer is.
> > Discussion required.
>
> I think part of the manual request completion RFC will be to evaluate the impact
> on VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF feature. MTK does not support
> interleaved interlaced decoding (only alternate), so they didn't have to
> implement that feature.
>
> Overall, It would be nice to get your feedback on the new manual request
> proposal, which is I believe better then the pin/unpin API you have in this
> serie.

I wasn't aware of that series, but I / John will take a look.

> >
> > We also have a need to hold on to the media request for phase 2. John
> > had discussed this with Ezequiel (and others) a couple of years back,
> > and hence suggested a patch that adds media_request_{pin,unpin} to
> > grab references on the media request. Discussion required on that
> > or a better way of handling it.
> >
> > I will apologise in advance for sending this V1 just before I head off
> > on the Christmas break, but will respond to things as soon as possible.
>
> One thing missing in this summary is how this driver is being validated
> (specially that for this one requires a downstream fork of FFMPEG). To this
> report we ask for:
>
> - v4l2-compliance results
> - Fluster conformance tests results [1] and I believe you need [2]
>
> [1] https://github.com/fluendo/fluster
> [2] https://github.com/fluendo/fluster/pull/179

Sure, I'll sort that before doing a V2.

> GStreamer support is there in main now, but without the needed software video
> converter for you column tiling, we can't use it for that (i.e. only works
> through GL or Wayland).

Can you point me at the right place for the software converter?
It's a relatively trivial reformat required to get it back into NV12 /
I420 or 10bit equivalents, so happy to do that. I think John already
has NEON optimised code if desired.

  Dave

> regards,
> Nicolas
>
> >
> > Thanks
> >   Dave
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> > Dave Stevenson (4):
> >       docs: uapi: media: Document Raspberry Pi NV12 column format
> >       media: ioctl: Add pixel formats NV12MT_COL128 and NV12MT_10_COL128
> >       media: dt-bindings: media: Add binding for the Raspberry Pi HEVC decoder
> >       arm: dts: bcm2711-rpi: Add HEVC decoder node
> >
> > Ezequiel Garcia (1):
> >       RFC: media: Add media_request_{pin,unpin} API
> >
> > John Cox (2):
> >       media: platform: Add Raspberry Pi HEVC decoder driver
> >       RFC: v4l2-mem2mem: Remove warning from v4l2_m2m_job_finish
> >
> >  .../bindings/media/raspberrypi,hevc-dec.yaml       |   72 +
> >  .../userspace-api/media/v4l/pixfmt-yuv-planar.rst  |   42 +
> >  MAINTAINERS                                        |   10 +
> >  arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi        |    5 +
> >  arch/arm/boot/dts/broadcom/bcm2711.dtsi            |    9 +
> >  drivers/media/mc/mc-request.c                      |   35 +
> >  drivers/media/platform/raspberrypi/Kconfig         |    1 +
> >  drivers/media/platform/raspberrypi/Makefile        |    1 +
> >  .../media/platform/raspberrypi/hevc_dec/Kconfig    |   17 +
> >  .../media/platform/raspberrypi/hevc_dec/Makefile   |    5 +
> >  .../media/platform/raspberrypi/hevc_dec/hevc_d.c   |  443 ++++
> >  .../media/platform/raspberrypi/hevc_dec/hevc_d.h   |  190 ++
> >  .../platform/raspberrypi/hevc_dec/hevc_d_h265.c    | 2629 ++++++++++++++++++++
> >  .../platform/raspberrypi/hevc_dec/hevc_d_hw.c      |  376 +++
> >  .../platform/raspberrypi/hevc_dec/hevc_d_hw.h      |  303 +++
> >  .../platform/raspberrypi/hevc_dec/hevc_d_video.c   |  685 +++++
> >  .../platform/raspberrypi/hevc_dec/hevc_d_video.h   |   38 +
> >  drivers/media/v4l2-core/v4l2-ioctl.c               |    2 +
> >  drivers/media/v4l2-core/v4l2-mem2mem.c             |    7 -
> >  include/media/media-request.h                      |   12 +
> >  include/uapi/linux/videodev2.h                     |    5 +
> >  21 files changed, 4880 insertions(+), 7 deletions(-)
> > ---
> > base-commit: e90c9612ac3969cb8206029a26bcd2b6f5d4a942
> > change-id: 20241212-media-rpi-hevc-dec-3b5be739f3bd
> >
> > Best regards,
>

Re: [PATCH 3/7] media: ioctl: Add pixel formats NV12MT_COL128 and NV12MT_10_COL128

[Dave Stevenson]

On Mon, 6 Jan 2025 at 20:52, Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Le jeudi 02 janvier 2025 à 12:52 +0000, Dave Stevenson a écrit :
> > Hi Robert
> >
> > Resending this reply as replying from my phone before the Christmas
> > break sent it as HTML :-(
> >
> > On Sat, 21 Dec 2024 at 17:38, Robert Mader <robert.mader@collabora.com> wrote:
> > >
> > > Hi, thanks for the patch.
> > >
> > > On 20.12.24 17:21, Dave Stevenson wrote:
> > >
> > > Add V4L2_PIXFMT_NV12MT_COL128 and V4L2_PIXFMT_NV12MT_10_COL128
> > > to describe the Raspberry Pi HEVC decoder NV12 multiplanar formats.
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-ioctl.c | 2 ++
> > >  include/uapi/linux/videodev2.h       | 5 +++++
> > >  2 files changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > > index 0304daa8471d..e510e375a871 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > > @@ -1377,7 +1377,9 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
> > >   case V4L2_PIX_FMT_NV16M: descr = "Y/UV 4:2:2 (N-C)"; break;
> > >   case V4L2_PIX_FMT_NV61M: descr = "Y/VU 4:2:2 (N-C)"; break;
> > >   case V4L2_PIX_FMT_NV12MT: descr = "Y/UV 4:2:0 (64x32 MB, N-C)"; break;
> > > + case V4L2_PIX_FMT_NV12MT_COL128: descr = "Y/CbCr 4:2:0 (128b cols)"; break;
> > >   case V4L2_PIX_FMT_NV12MT_16X16: descr = "Y/UV 4:2:0 (16x16 MB, N-C)"; break;
> > > + case V4L2_PIX_FMT_NV12MT_10_COL128: descr = "10-bit Y/CbCr 4:2:0 (128b cols)"; break;
> > >   case V4L2_PIX_FMT_P012M: descr = "12-bit Y/UV 4:2:0 (N-C)"; break;
> > >   case V4L2_PIX_FMT_YUV420M: descr = "Planar YUV 4:2:0 (N-C)"; break;
> > >   case V4L2_PIX_FMT_YVU420M: descr = "Planar YVU 4:2:0 (N-C)"; break;
> > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > > index e7c4dce39007..f8f97aa6a616 100644
> > > --- a/include/uapi/linux/videodev2.h
> > > +++ b/include/uapi/linux/videodev2.h
> > > @@ -687,6 +687,11 @@ struct v4l2_pix_format {
> > >  #define V4L2_PIX_FMT_NV12MT_16X16 v4l2_fourcc('V', 'M', '1', '2') /* 12  Y/CbCr 4:2:0 16x16 tiles */
> > >  #define V4L2_PIX_FMT_NV12M_8L128      v4l2_fourcc('N', 'A', '1', '2') /* Y/CbCr 4:2:0 8x128 tiles */
> > >  #define V4L2_PIX_FMT_NV12M_10BE_8L128 v4l2_fourcc_be('N', 'T', '1', '2') /* Y/CbCr 4:2:0 10-bit 8x128 tiles */
> > > +#define V4L2_PIX_FMT_NV12MT_COL128 v4l2_fourcc('N', 'c', '1', '2') /* 12  Y/CbCr 4:2:0 128 pixel wide column */
> > > +#define V4L2_PIX_FMT_NV12MT_10_COL128 v4l2_fourcc('N', 'c', '3', '0')
> > >
> > > Should these be upper-case Cs instead? So they compatible with the previously used downstream values?
> >
> > No, this is deliberate.
> >
> > Downstream was using a single planar format, with extra complexity for
> > determining the chroma offset per column, and weird handling required
> > on bytesperline.
> > Having had discussions, switching to a multiplanar format (hence MT in
> > the name) removes those complexities and means we don't need to do
> > anything weird in the v4l2 format definitions.
> >
> > Reusing NC12 and NC30 fourccs will give us grief over backwards
> > compatibility, hence lower case for the new version.
> >
> > I have a patch on [1] that adds back in NC12 and NC30 for downstream
> > just so we don't break existing users, but see no point in upstreaming
> > that.
>
> Yes, I think its fair to avoid incompatibility there. Are there matching Mesa
> patches coming, since without that we are back to square one, where the format
> remains unusable. NC12 have a matching (mainline) DRM_FORMAT_NV12
> DRM_FORMAT_MOD_BROADCOM_SAND128 pair. I believe a new modifier is needed and
> will serve both Nc12/30.

The current DRM_FORMAT_MOD_BROADCOM_SAND128 taking the column height
as the parameter is apparently contrary to how DRM modifiers are meant
to be used. You're not allowed to have a genuine runtime parameter in
there, and all potential values for parameters have to be listed out
in the in_formats blob.

I have a patch on [1] to add DRM_FORMAT_MOD_BROADCOM_SAND128(0) as a
modifier which then takes the height passed in to addFB2 to be the
column height. With luma and chroma now in independent buffers via the
multi-planar API, that solves the problem, and we don't need a new
modifier. I will submit that to dri-devel in the next week or so.

I've pinged Igalia to sort the equivalent Mesa patch for me.

 Dave

[1] https://github.com/6by9/linux/commits/rpi-6.12.y-hevc_dec patch
"drm/vc4: Add algorithmic handling for SAND". I can't give a hash as I
rebase that branch.


> Nicolas
>
> >
> > > P.S.: Coincidentally Gstreamer just landed support for NC12 in https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/7355 and there is also a link to experimental NC30 patches. So happy to see this series appear upstream :)
> >
> > Ooh, nice. I'll give it a test when I'm back in the office.
> >
> > Dave
> >
> > [1] https://github.com/6by9/linux/commits/rpi-6.12.y-hevc_dec/
> >
> > > + /* Y/CbCr 4:2:0 10bpc, 3x10 packed as 4 bytes in
> > > + * a 128 bytes / 96 pixel wide column */
> > > +
> > >
> > >  /* Bayer formats - see http://www.siliconimaging.com/RGB%20Bayer.htm */
> > >  #define V4L2_PIX_FMT_SBGGR8  v4l2_fourcc('B', 'A', '8', '1') /*  8  BGBG.. GRGR.. */
> > >
> >
>

Re: [PATCH 0/7] Raspberry Pi HEVC decoder driver

[Dave Stevenson]

On Tue, 7 Jan 2025 at 16:13, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Nicolas
>
> On Mon, 6 Jan 2025 at 20:46, Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> >
> > Hi Dave,
> >
> > Le vendredi 20 décembre 2024 à 16:21 +0000, Dave Stevenson a écrit :
> > > Hi All
> > >
> > > This has been in the pipeline for a while, but I've finally cleaned
> > > up our HEVC decoder driver to be in a shape to at least get a first
> > > review.
> > > John Cox has done almost all of the work under contract to Raspberry
> > > Pi, and I'm largely just doing the process of patch curation and
> > > sending.
> > >
> > > There are a couple of questions raised in frameworks.
> > > The main one is that the codec has 2 independent phases to the decode,
> > > CABAC and reconstruction. To keep the decoder operating optimally
> > > means that two requests need to be in process at once, whilst the
> > > current frameworks don't want to allow as there is an implicit
> > > assumption of only a single job being active at once, and
> > > completition returns both buffers and releases the media request.
> > >
> > > The OUTPUT queue buffer is finished with and can be returned at the
> > > end of phase 1, but the media request is still required for phase 2.
> > > The frameworks currently force the driver to be returning both
> > > together via v4l2_m2m_buf_done_and_job_finish. v4l2_m2m_job_finish
> > > would complete the job without returning the buffer as we need,
> > > however if the driver has set VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF
> > > then we have a WARN in v4l2_m2m_job_finish.
> > > Dropping the WARN as this series is currently doing isn't going to be
> > > the right answer, but it isn't obvious what the right answer is.
> > > Discussion required.
> >
> > I think part of the manual request completion RFC will be to evaluate the impact
> > on VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF feature. MTK does not support
> > interleaved interlaced decoding (only alternate), so they didn't have to
> > implement that feature.
> >
> > Overall, It would be nice to get your feedback on the new manual request
> > proposal, which is I believe better then the pin/unpin API you have in this
> > serie.
>
> I wasn't aware of that series, but I / John will take a look.

As I said at the beginning, I'm largely an intermediary here, and may
get things wrong as my codec knowledge is far from comprehensive. I'm
hoping John will correct me if I misrepresent our conversations.

As you say the MTK codec doesn't set
VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF, and hence it can call
v4l2_m2m_job_finish without hitting the WARN.

Your comment about it not supporting interleaved interlaced decoding
confuses me slightly. Almost all the codecs allow a single frame to be
encoded as multiple slices, and I'd be surprised if none of the test
streams exercise that.
Our reading of the situation was that if you have more than one
encoded slice making up the video frame then you are NOT obliged to
submit all of the slices at once via the variable length array
control, and submitting the slices one at a time is permitted. That
means that almost all decoders have to set the
VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF flag to be able to hold onto
the CAPTURE buffer until it has been given all the encoded data to
generate it.

If there isn't a need to support a multi-slice frame being presented
via multiple OUTPUT buffers, then we can cull some code and drop the
flag.
HEVC has no real concept of interlaced content, so no need to worry
about that as the other route to having multiple slices producing one
video frame.

  Dave

> > >
> > > We also have a need to hold on to the media request for phase 2. John
> > > had discussed this with Ezequiel (and others) a couple of years back,
> > > and hence suggested a patch that adds media_request_{pin,unpin} to
> > > grab references on the media request. Discussion required on that
> > > or a better way of handling it.
> > >
> > > I will apologise in advance for sending this V1 just before I head off
> > > on the Christmas break, but will respond to things as soon as possible.
> >
> > One thing missing in this summary is how this driver is being validated
> > (specially that for this one requires a downstream fork of FFMPEG). To this
> > report we ask for:
> >
> > - v4l2-compliance results
> > - Fluster conformance tests results [1] and I believe you need [2]
> >
> > [1] https://github.com/fluendo/fluster
> > [2] https://github.com/fluendo/fluster/pull/179
>
> Sure, I'll sort that before doing a V2.
>
> > GStreamer support is there in main now, but without the needed software video
> > converter for you column tiling, we can't use it for that (i.e. only works
> > through GL or Wayland).
>
> Can you point me at the right place for the software converter?
> It's a relatively trivial reformat required to get it back into NV12 /
> I420 or 10bit equivalents, so happy to do that. I think John already
> has NEON optimised code if desired.
>
>   Dave
>
> > regards,
> > Nicolas
> >
> > >
> > > Thanks
> > >   Dave
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > > Dave Stevenson (4):
> > >       docs: uapi: media: Document Raspberry Pi NV12 column format
> > >       media: ioctl: Add pixel formats NV12MT_COL128 and NV12MT_10_COL128
> > >       media: dt-bindings: media: Add binding for the Raspberry Pi HEVC decoder
> > >       arm: dts: bcm2711-rpi: Add HEVC decoder node
> > >
> > > Ezequiel Garcia (1):
> > >       RFC: media: Add media_request_{pin,unpin} API
> > >
> > > John Cox (2):
> > >       media: platform: Add Raspberry Pi HEVC decoder driver
> > >       RFC: v4l2-mem2mem: Remove warning from v4l2_m2m_job_finish
> > >
> > >  .../bindings/media/raspberrypi,hevc-dec.yaml       |   72 +
> > >  .../userspace-api/media/v4l/pixfmt-yuv-planar.rst  |   42 +
> > >  MAINTAINERS                                        |   10 +
> > >  arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi        |    5 +
> > >  arch/arm/boot/dts/broadcom/bcm2711.dtsi            |    9 +
> > >  drivers/media/mc/mc-request.c                      |   35 +
> > >  drivers/media/platform/raspberrypi/Kconfig         |    1 +
> > >  drivers/media/platform/raspberrypi/Makefile        |    1 +
> > >  .../media/platform/raspberrypi/hevc_dec/Kconfig    |   17 +
> > >  .../media/platform/raspberrypi/hevc_dec/Makefile   |    5 +
> > >  .../media/platform/raspberrypi/hevc_dec/hevc_d.c   |  443 ++++
> > >  .../media/platform/raspberrypi/hevc_dec/hevc_d.h   |  190 ++
> > >  .../platform/raspberrypi/hevc_dec/hevc_d_h265.c    | 2629 ++++++++++++++++++++
> > >  .../platform/raspberrypi/hevc_dec/hevc_d_hw.c      |  376 +++
> > >  .../platform/raspberrypi/hevc_dec/hevc_d_hw.h      |  303 +++
> > >  .../platform/raspberrypi/hevc_dec/hevc_d_video.c   |  685 +++++
> > >  .../platform/raspberrypi/hevc_dec/hevc_d_video.h   |   38 +
> > >  drivers/media/v4l2-core/v4l2-ioctl.c               |    2 +
> > >  drivers/media/v4l2-core/v4l2-mem2mem.c             |    7 -
> > >  include/media/media-request.h                      |   12 +
> > >  include/uapi/linux/videodev2.h                     |    5 +
> > >  21 files changed, 4880 insertions(+), 7 deletions(-)
> > > ---
> > > base-commit: e90c9612ac3969cb8206029a26bcd2b6f5d4a942
> > > change-id: 20241212-media-rpi-hevc-dec-3b5be739f3bd
> > >
> > > Best regards,
> >

Re: [PATCH 2/7] docs: uapi: media: Document Raspberry Pi NV12 column format

[Sakari Ailus]

Hi Dave,

Thanks for the set.

On Fri, Dec 20, 2024 at 04:21:13PM +0000, Dave Stevenson wrote:
> The Raspberry Pi HEVC decoder uses a tiled format based on
> columns for 8 and 10 bit YUV images, so document them as
> NV12MT_COL128 and NV12MT_10_COL128.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  .../userspace-api/media/v4l/pixfmt-yuv-planar.rst  | 42 ++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
> index b788f6933855..90414491d7b5 100644
> --- a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
> +++ b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
> @@ -827,6 +827,48 @@ Data in the 12 high bits, zeros in the 4 low bits, arranged in little endian ord
>        - Cb\ :sub:`11`
>        - Cr\ :sub:`11`
>  
> +NV12MT_COL128 and NV12MT_10_COL128

Please use full macro names of the formats here.

> +----------------------------------
> +
> +``V4L2_PIX_FMT_NV12MT_COL128`` is a tiled version of
> +``V4L2_PIX_FMT_NV12M`` where the two planes are split into 128 byte wide columns
> +of Y or interleaved CbCr.
> +
> +NV12MT_10_COL128 expands that as a 10 bit format where 3 10 bit values are

Ditto.

> +packed into a 32bit word. A 128 byte wide column therefore holds 96 samples
> +(either Y or interleaved CrCb). That effectively makes it 6 values in a 64 bit
> +word for the CbCr plane, as the values always go in pairs.
> +
> +Bit-packed representation.
> +
> +.. tabularcolumns:: |p{1.2cm}||p{1.2cm}||p{1.2cm}||p{1.2cm}|p{3.2cm}|p{3.2cm}|
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths: 8 8 8 8
> +
> +    * - Y'\ :sub:`00[7:0]`
> +      - Y'\ :sub:`01[5:0] (bits 7--2)` Y'\ :sub:`00[9:8]`\ (bits 1--0)
> +      - Y'\ :sub:`02[3:0] (bits 7--4)` Y'\ :sub:`01[9:6]`\ (bits 3--0)
> +      - unused (bits 7--6)` Y'\ :sub:`02[9:4]`\ (bits 5--0)
> +
> +.. tabularcolumns:: |p{1.2cm}||p{1.2cm}||p{1.2cm}||p{1.2cm}|p{3.2cm}|p{3.2cm}|
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths: 12 12 12 12 12 12 12 12
> +
> +    * - Cb\ :sub:`00[7:0]`
> +      - Cr\ :sub:`00[5:0]`\ (bits 7--2) Cb\ :sub:`00[9:8]`\ (bits 1--0)
> +      - Cb\ :sub:`01[3:0]`\ (bits 7--4) Cr\ :sub:`00[9:6]`\ (bits 3--0)
> +      - unused (bits 7--6) Cb\ :sub:`02[9:4]`\ (bits 5--0)
> +      - Cr\ :sub:`01[7:0]`
> +      - Cb\ :sub:`02[5:0]`\ (bits 7--2) Cr\ :sub:`01[9:8]`\ (bits 1--0)
> +      - Cr\ :sub:`02[3:0]`\ (bits 7--4) Cb\ :sub:`02[9:6]`\ (bits 3--0)
> +      - unused (bits 7--6) Cr\ :sub:`02[9:4]`\ (bits 5--0)
> +
>  
>  Fully Planar YUV Formats
>  ========================
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 0/7] Raspberry Pi HEVC decoder driver

[John Cox]

On Mon, 06 Jan 2025 15:46:51 -0500, you wrote:

>Hi Dave,
>
>Le vendredi 20 décembre 2024 à 16:21 +0000, Dave Stevenson a écrit :
>> Hi All
>> 
>> This has been in the pipeline for a while, but I've finally cleaned
>> up our HEVC decoder driver to be in a shape to at least get a first
>> review.
>> John Cox has done almost all of the work under contract to Raspberry
>> Pi, and I'm largely just doing the process of patch curation and
>> sending.
>> 
>> There are a couple of questions raised in frameworks.
>> The main one is that the codec has 2 independent phases to the decode,
>> CABAC and reconstruction. To keep the decoder operating optimally
>> means that two requests need to be in process at once, whilst the
>> current frameworks don't want to allow as there is an implicit
>> assumption of only a single job being active at once, and
>> completition returns both buffers and releases the media request.
>> 
>> The OUTPUT queue buffer is finished with and can be returned at the
>> end of phase 1, but the media request is still required for phase 2.
>> The frameworks currently force the driver to be returning both
>> together via v4l2_m2m_buf_done_and_job_finish. v4l2_m2m_job_finish
>> would complete the job without returning the buffer as we need,
>> however if the driver has set VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF
>> then we have a WARN in v4l2_m2m_job_finish.
>> Dropping the WARN as this series is currently doing isn't going to be
>> the right answer, but it isn't obvious what the right answer is.
>> Discussion required.
>
>I think part of the manual request completion RFC will be to evaluate the impact
>on VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF feature. MTK does not support
>interleaved interlaced decoding (only alternate), so they didn't have to
>implement that feature.
>
>Overall, It would be nice to get your feedback on the new manual request
>proposal, which is I believe better then the pin/unpin API you have in this
>serie.

Is the effect of the manual complete API different from the pin API? Pin
incs the ref count on the media object to prevent competion,
manaul_complete sets a flag to do the same thing. Or have I missed the
point?

I don't mind what call is used as long as the effect is to be able to
delay media object completion. (In my first veraion of the code I
created a dummy object and attached it to the media object, when I
wanted to unpin I deleted the dummy object - pin was just a cleaner
API.)

The pin API is counted and needs no new structure elements (which is
nice), but manual_complete does give a flag that allows other functions
to know that holding on to the media object whilst releasing OUTPUT is
intentional so can be made to work cleanly with things like
v4l2_m2m_job_finish so is probably the better solution (assuming my
understand of how it works is correct). 

I'll try to build a version of the code using manual_complete in the
next few days.

Regards

JC

>> 
>> We also have a need to hold on to the media request for phase 2. John
>> had discussed this with Ezequiel (and others) a couple of years back,
>> and hence suggested a patch that adds media_request_{pin,unpin} to
>> grab references on the media request. Discussion required on that
>> or a better way of handling it.
>> 
>> I will apologise in advance for sending this V1 just before I head off
>> on the Christmas break, but will respond to things as soon as possible.
>
>One thing missing in this summary is how this driver is being validated
>(specially that for this one requires a downstream fork of FFMPEG). To this
>report we ask for:
>
>- v4l2-compliance results
>- Fluster conformance tests results [1] and I believe you need [2]
>
>[1] https://github.com/fluendo/fluster
>[2] https://github.com/fluendo/fluster/pull/179
>
>GStreamer support is there in main now, but without the needed software video
>converter for you column tiling, we can't use it for that (i.e. only works
>through GL or Wayland).
>
>regards,
>Nicolas
>
>> 
>> Thanks
>>   Dave
>> 
>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>> ---
>> Dave Stevenson (4):
>>       docs: uapi: media: Document Raspberry Pi NV12 column format
>>       media: ioctl: Add pixel formats NV12MT_COL128 and NV12MT_10_COL128
>>       media: dt-bindings: media: Add binding for the Raspberry Pi HEVC decoder
>>       arm: dts: bcm2711-rpi: Add HEVC decoder node
>> 
>> Ezequiel Garcia (1):
>>       RFC: media: Add media_request_{pin,unpin} API
>> 
>> John Cox (2):
>>       media: platform: Add Raspberry Pi HEVC decoder driver
>>       RFC: v4l2-mem2mem: Remove warning from v4l2_m2m_job_finish
>> 
>>  .../bindings/media/raspberrypi,hevc-dec.yaml       |   72 +
>>  .../userspace-api/media/v4l/pixfmt-yuv-planar.rst  |   42 +
>>  MAINTAINERS                                        |   10 +
>>  arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi        |    5 +
>>  arch/arm/boot/dts/broadcom/bcm2711.dtsi            |    9 +
>>  drivers/media/mc/mc-request.c                      |   35 +
>>  drivers/media/platform/raspberrypi/Kconfig         |    1 +
>>  drivers/media/platform/raspberrypi/Makefile        |    1 +
>>  .../media/platform/raspberrypi/hevc_dec/Kconfig    |   17 +
>>  .../media/platform/raspberrypi/hevc_dec/Makefile   |    5 +
>>  .../media/platform/raspberrypi/hevc_dec/hevc_d.c   |  443 ++++
>>  .../media/platform/raspberrypi/hevc_dec/hevc_d.h   |  190 ++
>>  .../platform/raspberrypi/hevc_dec/hevc_d_h265.c    | 2629 ++++++++++++++++++++
>>  .../platform/raspberrypi/hevc_dec/hevc_d_hw.c      |  376 +++
>>  .../platform/raspberrypi/hevc_dec/hevc_d_hw.h      |  303 +++
>>  .../platform/raspberrypi/hevc_dec/hevc_d_video.c   |  685 +++++
>>  .../platform/raspberrypi/hevc_dec/hevc_d_video.h   |   38 +
>>  drivers/media/v4l2-core/v4l2-ioctl.c               |    2 +
>>  drivers/media/v4l2-core/v4l2-mem2mem.c             |    7 -
>>  include/media/media-request.h                      |   12 +
>>  include/uapi/linux/videodev2.h                     |    5 +
>>  21 files changed, 4880 insertions(+), 7 deletions(-)
>> ---
>> base-commit: e90c9612ac3969cb8206029a26bcd2b6f5d4a942
>> change-id: 20241212-media-rpi-hevc-dec-3b5be739f3bd
>> 
>> Best regards,

Re: [PATCH 0/7] Raspberry Pi HEVC decoder driver

[Nicolas Dufresne]

Le mardi 07 janvier 2025 à 16:13 +0000, Dave Stevenson a écrit :
> Hi Nicolas
> 
> On Mon, 6 Jan 2025 at 20:46, Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> > 
> > Hi Dave,
> > 
> > Le vendredi 20 décembre 2024 à 16:21 +0000, Dave Stevenson a écrit :
> > > Hi All
> > > 
> > > This has been in the pipeline for a while, but I've finally cleaned
> > > up our HEVC decoder driver to be in a shape to at least get a first
> > > review.
> > > John Cox has done almost all of the work under contract to Raspberry
> > > Pi, and I'm largely just doing the process of patch curation and
> > > sending.
> > > 
> > > There are a couple of questions raised in frameworks.
> > > The main one is that the codec has 2 independent phases to the decode,
> > > CABAC and reconstruction. To keep the decoder operating optimally
> > > means that two requests need to be in process at once, whilst the
> > > current frameworks don't want to allow as there is an implicit
> > > assumption of only a single job being active at once, and
> > > completition returns both buffers and releases the media request.
> > > 
> > > The OUTPUT queue buffer is finished with and can be returned at the
> > > end of phase 1, but the media request is still required for phase 2.
> > > The frameworks currently force the driver to be returning both
> > > together via v4l2_m2m_buf_done_and_job_finish. v4l2_m2m_job_finish
> > > would complete the job without returning the buffer as we need,
> > > however if the driver has set VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF
> > > then we have a WARN in v4l2_m2m_job_finish.
> > > Dropping the WARN as this series is currently doing isn't going to be
> > > the right answer, but it isn't obvious what the right answer is.
> > > Discussion required.
> > 
> > I think part of the manual request completion RFC will be to evaluate the impact
> > on VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF feature. MTK does not support
> > interleaved interlaced decoding (only alternate), so they didn't have to
> > implement that feature.
> > 
> > Overall, It would be nice to get your feedback on the new manual request
> > proposal, which is I believe better then the pin/unpin API you have in this
> > serie.
> 
> I wasn't aware of that series, but I / John will take a look.
> 
> > > 
> > > We also have a need to hold on to the media request for phase 2. John
> > > had discussed this with Ezequiel (and others) a couple of years back,
> > > and hence suggested a patch that adds media_request_{pin,unpin} to
> > > grab references on the media request. Discussion required on that
> > > or a better way of handling it.
> > > 
> > > I will apologise in advance for sending this V1 just before I head off
> > > on the Christmas break, but will respond to things as soon as possible.
> > 
> > One thing missing in this summary is how this driver is being validated
> > (specially that for this one requires a downstream fork of FFMPEG). To this
> > report we ask for:
> > 
> > - v4l2-compliance results
> > - Fluster conformance tests results [1] and I believe you need [2]
> > 
> > [1] https://github.com/fluendo/fluster
> > [2] https://github.com/fluendo/fluster/pull/179
> 
> Sure, I'll sort that before doing a V2.
> 
> > GStreamer support is there in main now, but without the needed software video
> > converter for you column tiling, we can't use it for that (i.e. only works
> > through GL or Wayland).
> 
> Can you point me at the right place for the software converter?
> It's a relatively trivial reformat required to get it back into NV12 /
> I420 or 10bit equivalents, so happy to do that. I think John already
> has NEON optimised code if desired.

Its challenging in the way its implemented in GStreamer Video library. Basically
all formats needs its GstVideoFormatInfo structure to be filled in the static
table, and then minimally a slow path for color conversion, which is reduced to
pack/unpack of a single line of video data. With tiled (or column format) the
notion of line is not as obvious.

https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-base/gst-libs/gst/video/video-format.c

For tile format with fixed dimensions, we have extra information for the tile
layout (GstVideoTileInfo), but we didn't think about full height tiles when that
was added. This information is needed to generically crop images, which is why
its so complicated.

One option we could use to make it fit as a tiled format is to introduce a new
GstVideoTileMode GST_VIDEO_TILE_MODE_COLUMN. Then we set the tiled height to
match the usual HW height step, making it so you have 128 x step tiles, laid out
in column. Alternatively, we can start with a step of 1 (which will make the
slow path a lot slower then needed), and later add an optimization for the
column layout, so it handle full columns.

If you manage to find a good step size, then all you will have to implement is
the backend for gst_video_tile_get_index(), the rest of the slow path will be
generic.

Fast path are possible, with ORC (a cross platform byte code language that can
be JIT into SIMD instructions), but I don't currently see much use for "fast",
since the GPU pretty much always accepts these buffers, so the software path
remains a neat debugging tool and allow conformance testing.

Nicolas

> 
>   Dave
> 
> > regards,
> > Nicolas
> > 
> > > 
> > > Thanks
> > >   Dave
> > > 
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > > Dave Stevenson (4):
> > >       docs: uapi: media: Document Raspberry Pi NV12 column format
> > >       media: ioctl: Add pixel formats NV12MT_COL128 and NV12MT_10_COL128
> > >       media: dt-bindings: media: Add binding for the Raspberry Pi HEVC decoder
> > >       arm: dts: bcm2711-rpi: Add HEVC decoder node
> > > 
> > > Ezequiel Garcia (1):
> > >       RFC: media: Add media_request_{pin,unpin} API
> > > 
> > > John Cox (2):
> > >       media: platform: Add Raspberry Pi HEVC decoder driver
> > >       RFC: v4l2-mem2mem: Remove warning from v4l2_m2m_job_finish
> > > 
> > >  .../bindings/media/raspberrypi,hevc-dec.yaml       |   72 +
> > >  .../userspace-api/media/v4l/pixfmt-yuv-planar.rst  |   42 +
> > >  MAINTAINERS                                        |   10 +
> > >  arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi        |    5 +
> > >  arch/arm/boot/dts/broadcom/bcm2711.dtsi            |    9 +
> > >  drivers/media/mc/mc-request.c                      |   35 +
> > >  drivers/media/platform/raspberrypi/Kconfig         |    1 +
> > >  drivers/media/platform/raspberrypi/Makefile        |    1 +
> > >  .../media/platform/raspberrypi/hevc_dec/Kconfig    |   17 +
> > >  .../media/platform/raspberrypi/hevc_dec/Makefile   |    5 +
> > >  .../media/platform/raspberrypi/hevc_dec/hevc_d.c   |  443 ++++
> > >  .../media/platform/raspberrypi/hevc_dec/hevc_d.h   |  190 ++
> > >  .../platform/raspberrypi/hevc_dec/hevc_d_h265.c    | 2629 ++++++++++++++++++++
> > >  .../platform/raspberrypi/hevc_dec/hevc_d_hw.c      |  376 +++
> > >  .../platform/raspberrypi/hevc_dec/hevc_d_hw.h      |  303 +++
> > >  .../platform/raspberrypi/hevc_dec/hevc_d_video.c   |  685 +++++
> > >  .../platform/raspberrypi/hevc_dec/hevc_d_video.h   |   38 +
> > >  drivers/media/v4l2-core/v4l2-ioctl.c               |    2 +
> > >  drivers/media/v4l2-core/v4l2-mem2mem.c             |    7 -
> > >  include/media/media-request.h                      |   12 +
> > >  include/uapi/linux/videodev2.h                     |    5 +
> > >  21 files changed, 4880 insertions(+), 7 deletions(-)
> > > ---
> > > base-commit: e90c9612ac3969cb8206029a26bcd2b6f5d4a942
> > > change-id: 20241212-media-rpi-hevc-dec-3b5be739f3bd
> > > 
> > > Best regards,
> > 

Re: [PATCH 0/7] Raspberry Pi HEVC decoder driver

[Nicolas Dufresne]

Hi Dave,

Le mardi 07 janvier 2025 à 17:36 +0000, Dave Stevenson a écrit :
> On Tue, 7 Jan 2025 at 16:13, Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> > 
> > Hi Nicolas
> > 
> > On Mon, 6 Jan 2025 at 20:46, Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> > > 
> > > Hi Dave,
> > > 
> > > Le vendredi 20 décembre 2024 à 16:21 +0000, Dave Stevenson a écrit :
> > > > Hi All
> > > > 
> > > > This has been in the pipeline for a while, but I've finally cleaned
> > > > up our HEVC decoder driver to be in a shape to at least get a first
> > > > review.
> > > > John Cox has done almost all of the work under contract to Raspberry
> > > > Pi, and I'm largely just doing the process of patch curation and
> > > > sending.
> > > > 
> > > > There are a couple of questions raised in frameworks.
> > > > The main one is that the codec has 2 independent phases to the decode,
> > > > CABAC and reconstruction. To keep the decoder operating optimally
> > > > means that two requests need to be in process at once, whilst the
> > > > current frameworks don't want to allow as there is an implicit
> > > > assumption of only a single job being active at once, and
> > > > completition returns both buffers and releases the media request.
> > > > 
> > > > The OUTPUT queue buffer is finished with and can be returned at the
> > > > end of phase 1, but the media request is still required for phase 2.
> > > > The frameworks currently force the driver to be returning both
> > > > together via v4l2_m2m_buf_done_and_job_finish. v4l2_m2m_job_finish
> > > > would complete the job without returning the buffer as we need,
> > > > however if the driver has set VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF
> > > > then we have a WARN in v4l2_m2m_job_finish.
> > > > Dropping the WARN as this series is currently doing isn't going to be
> > > > the right answer, but it isn't obvious what the right answer is.
> > > > Discussion required.
> > > 
> > > I think part of the manual request completion RFC will be to evaluate the impact
> > > on VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF feature. MTK does not support
> > > interleaved interlaced decoding (only alternate), so they didn't have to
> > > implement that feature.
> > > 
> > > Overall, It would be nice to get your feedback on the new manual request
> > > proposal, which is I believe better then the pin/unpin API you have in this
> > > serie.
> > 
> > I wasn't aware of that series, but I / John will take a look.
> 
> As I said at the beginning, I'm largely an intermediary here, and may
> get things wrong as my codec knowledge is far from comprehensive. I'm
> hoping John will correct me if I misrepresent our conversations.
> 
> As you say the MTK codec doesn't set
> VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF, and hence it can call
> v4l2_m2m_job_finish without hitting the WARN.
> 
> Your comment about it not supporting interleaved interlaced decoding
> confuses me slightly. Almost all the codecs allow a single frame to be
> encoded as multiple slices, and I'd be surprised if none of the test
> streams exercise that.

MTK VCODEC is a frame based HEVC decoder. All slices are passed within the same
request (up to 600). Upstream, only Cedrus driver is slice based, though for
large number of slices this has performance issues (even though you gain on
latency).

> Our reading of the situation was that if you have more than one
> encoded slice making up the video frame then you are NOT obliged to
> submit all of the slices at once via the variable length array
> control, and submitting the slices one at a time is permitted. That
> means that almost all decoders have to set the
> VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF flag to be able to hold onto
> the CAPTURE buffer until it has been given all the encoded data to
> generate it.

We don't have support for that, a new v4l2_stateless_hevc_decode_mode would be
required. Here's what the spec says:


    * - ``V4L2_STATELESS_HEVC_DECODE_MODE_SLICE_BASED``
      - 0
      - Decoding is done at the slice granularity.
        The OUTPUT buffer must contain a **single** slice.
    * - ``V4L2_STATELESS_HEVC_DECODE_MODE_FRAME_BASED``
      - 1
      - Decoding is done at the frame granularity.
        The OUTPUT buffer must contain **all** slices needed to decode the
        frame.

So one, or all. Additionally, (and doc could be improved here),
V4L2_CID_STATELESS_HEVC_SLICE_PARAMS is strictly required for SLICED_BASED, and
must contain 1 slice. While for the second, some drivers needs it, some don't,
userspace needs to check if the control is present or not.

SLICE_BASED depends on VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF, otherwise you
can't queue capture buffers ahead of time, and loose in throughput. So a new
DYN_SLICE_BASED (with some proper name), would also require it.
VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF is also needed when you need to decode
top and bottom field in the same capture buffer, hence my reference to that (MTK
chromebook firmware don't support that for any codecs).

You could also ignore all this, and implement
V4L2_STATELESS_HEVC_DECODE_MODE_FRAME_BASED on top of your slice based decoder.
No latency gain, but you also no longer need
VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF which let you postpone adding new
helpers to complete the job when this feature is used. Yet, I think the manual
request completion proposal would be a lot better if we had that issue covered.

> 
> If there isn't a need to support a multi-slice frame being presented
> via multiple OUTPUT buffers, then we can cull some code and drop the
> flag.

> HEVC has no real concept of interlaced content, so no need to worry
> about that as the other route to having multiple slices producing one
> video frame.

I've seen non-tiled decoders that internally double the stride, and offset the
same buffer by one line for the second field, to generate interleaved data, but
as you said, this is the exception rather then the rule. :-)

p.s. your understanding is pretty accurate

> 
>   Dave
> 
> > > > 
> > > > We also have a need to hold on to the media request for phase 2. John
> > > > had discussed this with Ezequiel (and others) a couple of years back,
> > > > and hence suggested a patch that adds media_request_{pin,unpin} to
> > > > grab references on the media request. Discussion required on that
> > > > or a better way of handling it.
> > > > 
> > > > I will apologise in advance for sending this V1 just before I head off
> > > > on the Christmas break, but will respond to things as soon as possible.
> > > 
> > > One thing missing in this summary is how this driver is being validated
> > > (specially that for this one requires a downstream fork of FFMPEG). To this
> > > report we ask for:
> > > 
> > > - v4l2-compliance results
> > > - Fluster conformance tests results [1] and I believe you need [2]
> > > 
> > > [1] https://github.com/fluendo/fluster
> > > [2] https://github.com/fluendo/fluster/pull/179
> > 
> > Sure, I'll sort that before doing a V2.
> > 
> > > GStreamer support is there in main now, but without the needed software video
> > > converter for you column tiling, we can't use it for that (i.e. only works
> > > through GL or Wayland).
> > 
> > Can you point me at the right place for the software converter?
> > It's a relatively trivial reformat required to get it back into NV12 /
> > I420 or 10bit equivalents, so happy to do that. I think John already
> > has NEON optimised code if desired.
> > 
> >   Dave
> > 
> > > regards,
> > > Nicolas
> > > 
> > > > 
> > > > Thanks
> > > >   Dave
> > > > 
> > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > ---
> > > > Dave Stevenson (4):
> > > >       docs: uapi: media: Document Raspberry Pi NV12 column format
> > > >       media: ioctl: Add pixel formats NV12MT_COL128 and NV12MT_10_COL128
> > > >       media: dt-bindings: media: Add binding for the Raspberry Pi HEVC decoder
> > > >       arm: dts: bcm2711-rpi: Add HEVC decoder node
> > > > 
> > > > Ezequiel Garcia (1):
> > > >       RFC: media: Add media_request_{pin,unpin} API
> > > > 
> > > > John Cox (2):
> > > >       media: platform: Add Raspberry Pi HEVC decoder driver
> > > >       RFC: v4l2-mem2mem: Remove warning from v4l2_m2m_job_finish
> > > > 
> > > >  .../bindings/media/raspberrypi,hevc-dec.yaml       |   72 +
> > > >  .../userspace-api/media/v4l/pixfmt-yuv-planar.rst  |   42 +
> > > >  MAINTAINERS                                        |   10 +
> > > >  arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi        |    5 +
> > > >  arch/arm/boot/dts/broadcom/bcm2711.dtsi            |    9 +
> > > >  drivers/media/mc/mc-request.c                      |   35 +
> > > >  drivers/media/platform/raspberrypi/Kconfig         |    1 +
> > > >  drivers/media/platform/raspberrypi/Makefile        |    1 +
> > > >  .../media/platform/raspberrypi/hevc_dec/Kconfig    |   17 +
> > > >  .../media/platform/raspberrypi/hevc_dec/Makefile   |    5 +
> > > >  .../media/platform/raspberrypi/hevc_dec/hevc_d.c   |  443 ++++
> > > >  .../media/platform/raspberrypi/hevc_dec/hevc_d.h   |  190 ++
> > > >  .../platform/raspberrypi/hevc_dec/hevc_d_h265.c    | 2629 ++++++++++++++++++++
> > > >  .../platform/raspberrypi/hevc_dec/hevc_d_hw.c      |  376 +++
> > > >  .../platform/raspberrypi/hevc_dec/hevc_d_hw.h      |  303 +++
> > > >  .../platform/raspberrypi/hevc_dec/hevc_d_video.c   |  685 +++++
> > > >  .../platform/raspberrypi/hevc_dec/hevc_d_video.h   |   38 +
> > > >  drivers/media/v4l2-core/v4l2-ioctl.c               |    2 +
> > > >  drivers/media/v4l2-core/v4l2-mem2mem.c             |    7 -
> > > >  include/media/media-request.h                      |   12 +
> > > >  include/uapi/linux/videodev2.h                     |    5 +
> > > >  21 files changed, 4880 insertions(+), 7 deletions(-)
> > > > ---
> > > > base-commit: e90c9612ac3969cb8206029a26bcd2b6f5d4a942
> > > > change-id: 20241212-media-rpi-hevc-dec-3b5be739f3bd
> > > > 
> > > > Best regards,
> > > 

Re: [PATCH 0/7] Raspberry Pi HEVC decoder driver

[Nicolas Dufresne]

Hi John,

Le mercredi 08 janvier 2025 à 09:52 +0000, John Cox a écrit :
> On Mon, 06 Jan 2025 15:46:51 -0500, you wrote:
> 
> > Hi Dave,
> > 
> > Le vendredi 20 décembre 2024 à 16:21 +0000, Dave Stevenson a écrit :
> > > Hi All
> > > 
> > > This has been in the pipeline for a while, but I've finally cleaned
> > > up our HEVC decoder driver to be in a shape to at least get a first
> > > review.
> > > John Cox has done almost all of the work under contract to Raspberry
> > > Pi, and I'm largely just doing the process of patch curation and
> > > sending.
> > > 
> > > There are a couple of questions raised in frameworks.
> > > The main one is that the codec has 2 independent phases to the decode,
> > > CABAC and reconstruction. To keep the decoder operating optimally
> > > means that two requests need to be in process at once, whilst the
> > > current frameworks don't want to allow as there is an implicit
> > > assumption of only a single job being active at once, and
> > > completition returns both buffers and releases the media request.
> > > 
> > > The OUTPUT queue buffer is finished with and can be returned at the
> > > end of phase 1, but the media request is still required for phase 2.
> > > The frameworks currently force the driver to be returning both
> > > together via v4l2_m2m_buf_done_and_job_finish. v4l2_m2m_job_finish
> > > would complete the job without returning the buffer as we need,
> > > however if the driver has set VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF
> > > then we have a WARN in v4l2_m2m_job_finish.
> > > Dropping the WARN as this series is currently doing isn't going to be
> > > the right answer, but it isn't obvious what the right answer is.
> > > Discussion required.
> > 
> > I think part of the manual request completion RFC will be to evaluate the impact
> > on VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF feature. MTK does not support
> > interleaved interlaced decoding (only alternate), so they didn't have to
> > implement that feature.
> > 
> > Overall, It would be nice to get your feedback on the new manual request
> > proposal, which is I believe better then the pin/unpin API you have in this
> > serie.
> 
> Is the effect of the manual complete API different from the pin API? Pin
> incs the ref count on the media object to prevent competion,
> manaul_complete sets a flag to do the same thing. Or have I missed the
> point?

The request contains "objects", usually controls and a src buffer. The state of
the request goes to complete implicitly when the last object is removed. The
manual completion avoids adding ref-count on top of this, and simply disable the
implicit signalling of the request. With that we can signal everything in the
most logical order:

1. Program the register from controls -> mark controls complete
2. Entropy decode the stream -> mark src buffer done
3. Reconstruct the image -> mark dst buffer done
4. Signal the request completion

Before that, you always had to hold on the src buffer, and only mark it done
after the reconstruction was complete and the dst buffer was marked done. That
didn't matter for single function HW of course.

Unlike the pin/unpin, manual completion mode removes the implicit completion
bound to the fact the last "object" is removed from the request, and leave it to
the driver to decide when to actually signal the request. Internally, the give a
reference to the driver of course (similar to pin).

> 
> I don't mind what call is used as long as the effect is to be able to
> delay media object completion. (In my first veraion of the code I
> created a dummy object and attached it to the media object, when I
> wanted to unpin I deleted the dummy object - pin was just a cleaner
> API.)

The manual completion patchset is very similar really in you step back.

> 
> The pin API is counted and needs no new structure elements (which is
> nice), but manual_complete does give a flag that allows other functions
> to know that holding on to the media object whilst releasing OUTPUT is
> intentional so can be made to work cleanly with things like
> v4l2_m2m_job_finish so is probably the better solution (assuming my
> understand of how it works is correct). 
> 
> I'll try to build a version of the code using manual_complete in the
> next few days.

Thanks! your feedback will be very useful.

Nicolas

> 
> Regards
> 
> JC
> 
> > > 
> > > We also have a need to hold on to the media request for phase 2. John
> > > had discussed this with Ezequiel (and others) a couple of years back,
> > > and hence suggested a patch that adds media_request_{pin,unpin} to
> > > grab references on the media request. Discussion required on that
> > > or a better way of handling it.
> > > 
> > > I will apologise in advance for sending this V1 just before I head off
> > > on the Christmas break, but will respond to things as soon as possible.
> > 
> > One thing missing in this summary is how this driver is being validated
> > (specially that for this one requires a downstream fork of FFMPEG). To this
> > report we ask for:
> > 
> > - v4l2-compliance results
> > - Fluster conformance tests results [1] and I believe you need [2]
> > 
> > [1] https://github.com/fluendo/fluster
> > [2] https://github.com/fluendo/fluster/pull/179
> > 
> > GStreamer support is there in main now, but without the needed software video
> > converter for you column tiling, we can't use it for that (i.e. only works
> > through GL or Wayland).
> > 
> > regards,
> > Nicolas
> > 
> > > 
> > > Thanks
> > >   Dave
> > > 
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > > Dave Stevenson (4):
> > >       docs: uapi: media: Document Raspberry Pi NV12 column format
> > >       media: ioctl: Add pixel formats NV12MT_COL128 and NV12MT_10_COL128
> > >       media: dt-bindings: media: Add binding for the Raspberry Pi HEVC decoder
> > >       arm: dts: bcm2711-rpi: Add HEVC decoder node
> > > 
> > > Ezequiel Garcia (1):
> > >       RFC: media: Add media_request_{pin,unpin} API
> > > 
> > > John Cox (2):
> > >       media: platform: Add Raspberry Pi HEVC decoder driver
> > >       RFC: v4l2-mem2mem: Remove warning from v4l2_m2m_job_finish
> > > 
> > >  .../bindings/media/raspberrypi,hevc-dec.yaml       |   72 +
> > >  .../userspace-api/media/v4l/pixfmt-yuv-planar.rst  |   42 +
> > >  MAINTAINERS                                        |   10 +
> > >  arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi        |    5 +
> > >  arch/arm/boot/dts/broadcom/bcm2711.dtsi            |    9 +
> > >  drivers/media/mc/mc-request.c                      |   35 +
> > >  drivers/media/platform/raspberrypi/Kconfig         |    1 +
> > >  drivers/media/platform/raspberrypi/Makefile        |    1 +
> > >  .../media/platform/raspberrypi/hevc_dec/Kconfig    |   17 +
> > >  .../media/platform/raspberrypi/hevc_dec/Makefile   |    5 +
> > >  .../media/platform/raspberrypi/hevc_dec/hevc_d.c   |  443 ++++
> > >  .../media/platform/raspberrypi/hevc_dec/hevc_d.h   |  190 ++
> > >  .../platform/raspberrypi/hevc_dec/hevc_d_h265.c    | 2629 ++++++++++++++++++++
> > >  .../platform/raspberrypi/hevc_dec/hevc_d_hw.c      |  376 +++
> > >  .../platform/raspberrypi/hevc_dec/hevc_d_hw.h      |  303 +++
> > >  .../platform/raspberrypi/hevc_dec/hevc_d_video.c   |  685 +++++
> > >  .../platform/raspberrypi/hevc_dec/hevc_d_video.h   |   38 +
> > >  drivers/media/v4l2-core/v4l2-ioctl.c               |    2 +
> > >  drivers/media/v4l2-core/v4l2-mem2mem.c             |    7 -
> > >  include/media/media-request.h                      |   12 +
> > >  include/uapi/linux/videodev2.h                     |    5 +
> > >  21 files changed, 4880 insertions(+), 7 deletions(-)
> > > ---
> > > base-commit: e90c9612ac3969cb8206029a26bcd2b6f5d4a942
> > > change-id: 20241212-media-rpi-hevc-dec-3b5be739f3bd
> > > 
> > > Best regards,