linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] MPEG-2 stateless API cleanup
@ 2020-11-30 18:52 Ezequiel Garcia
  2020-11-30 18:52 ` [PATCH v3 1/3] media: uapi: mpeg2: Cleanup flags Ezequiel Garcia
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Ezequiel Garcia @ 2020-11-30 18:52 UTC (permalink / raw)
  To: linux-media, linux-kernel
  Cc: kernel, Jonas Karlman, Hans Verkuil, Nicolas Dufresne,
	Philipp Zabel, Maxime Ripard, Paul Kocialkowski, Jernej Skrabec,
	Ezequiel Garcia

Hello everyone,

Now that H.264 API is finally parked, and with all the lessons
learned from it, here's a revisit to MPEG-2.

The biggest changes are:

* Drop slice size and offset parameters, given the API doesn't
  seem to be designed to work per-slice.

* Convert all the 1-bit syntax elements from structure fields
  to bits in a bit field flags field, following the other codec APIs.

* Split the sequence, picture and slice headers to separate controls.

The series is tested by Jonas on RK3288 (Hantro G1) with FFmpeg + Kodi,
with some fixes that I included here. Thanks a lot Jonas!

v3:
* No API changes, just minor boilerplate fixes for the new
  controls to be properly exposed, initialized and validated.

v2:
* Fixed bad use of boolean negation in a flag, which
  was fortunately reported by 0day bot.

Ezequiel Garcia (3):
  media: uapi: mpeg2: Cleanup flags
  media: uapi: mpeg2: Remove unused slice size and offset
  media: uapi: mpeg2: Split sequence and picture parameters

 .../media/v4l/ext-ctrls-codec.rst             | 159 +++++++++++-------
 .../media/v4l/pixfmt-compressed.rst           |   5 +-
 drivers/media/v4l2-core/v4l2-ctrls.c          |  71 +++++---
 drivers/staging/media/hantro/hantro_drv.c     |  10 ++
 .../media/hantro/hantro_g1_mpeg2_dec.c        |  82 +++++----
 .../media/hantro/rk3399_vpu_hw_mpeg2_dec.c    |  82 +++++----
 drivers/staging/media/sunxi/cedrus/cedrus.c   |  14 ++
 drivers/staging/media/sunxi/cedrus/cedrus.h   |   2 +
 .../staging/media/sunxi/cedrus/cedrus_mpeg2.c |  49 +++---
 include/media/mpeg2-ctrls.h                   | 153 +++++++++++++----
 include/media/v4l2-ctrls.h                    |   4 +
 11 files changed, 407 insertions(+), 224 deletions(-)

-- 
2.27.0


^ permalink raw reply	[flat|nested] 7+ messages in thread
* [PATCH v3 1/3] media: uapi: mpeg2: Cleanup flags
  2020-11-30 18:52 [PATCH v3 0/3] MPEG-2 stateless API cleanup Ezequiel Garcia
@ 2020-11-30 18:52 ` Ezequiel Garcia
  2020-11-30 18:52 ` [PATCH v3 2/3] media: uapi: mpeg2: Remove unused slice size and offset Ezequiel Garcia
  2020-11-30 18:52 ` [PATCH v3 3/3] media: uapi: mpeg2: Split sequence and picture parameters Ezequiel Garcia
  2 siblings, 0 replies; 7+ messages in thread
From: Ezequiel Garcia @ 2020-11-30 18:52 UTC (permalink / raw)
  To: linux-media, linux-kernel
  Cc: kernel, Jonas Karlman, Hans Verkuil, Nicolas Dufresne,
	Philipp Zabel, Maxime Ripard, Paul Kocialkowski, Jernej Skrabec,
	Ezequiel Garcia

Our current MPEG-2 uAPI uses 1-byte fields for MPEG-2
boolean syntax elements. Clean these by adding a 'flags'
field and flag macro for each boolean syntax element.

Move quantization "load" flags to struct v4l2_mpeg2_picture,
so the applications can skip passing the quantization matrices
(by not setting the MPEG2_QUANTIZATION control in decode request).

A follow-up change will refactor this uAPI so we don't need
to add padding fields just yet.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Tested-by: Jonas Karlman <jonas@kwiboo.se>
---
 .../media/v4l/ext-ctrls-codec.rst             | 108 +++++++++++-------
 drivers/media/v4l2-core/v4l2-ctrls.c          |  14 +--
 .../media/hantro/hantro_g1_mpeg2_dec.c        |  76 ++++++------
 .../media/hantro/rk3399_vpu_hw_mpeg2_dec.c    |  76 ++++++------
 .../staging/media/sunxi/cedrus/cedrus_mpeg2.c |  42 +++----
 include/media/mpeg2-ctrls.h                   |  47 +++++---
 6 files changed, 196 insertions(+), 167 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index e815fffd1cd8..29da24d01aba 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1583,13 +1583,28 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
       - ``profile_and_level_indication``
       - The current profile and level indication as extracted from the
 	bitstream.
-    * - __u8
-      - ``progressive_sequence``
-      - Indication that all the frames for the sequence are progressive instead
-	of interlaced.
     * - __u8
       - ``chroma_format``
       - The chrominance sub-sampling format (1: 4:2:0, 2: 4:2:2, 3: 4:4:4).
+    * - __u32
+      - ``flags``
+      - See :ref:`MPEG-2 Sequence Flags <mpeg2_sequence_flags>`.
+
+.. _mpeg2_sequence_flags:
+
+``MPEG-2 Sequence Flags``
+
+.. cssclass:: longtable
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       1 1 2
+
+    * - ``V4L2_MPEG2_SEQ_FLAG_PROGRESSIVE``
+      - 0x00000001
+      - Indication that all the frames for the sequence are progressive instead
+	of interlaced.
 
 .. c:type:: v4l2_mpeg2_picture
 
@@ -1618,30 +1633,60 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
       - ``picture_structure``
       - Picture structure (1: interlaced top field, 2: interlaced bottom field,
 	3: progressive frame).
-    * - __u8
-      - ``top_field_first``
-      - If set to 1 and interlaced stream, top field is output first.
-    * - __u8
-      - ``frame_pred_frame_dct``
-      - If set to 1, only frame-DCT and frame prediction are used.
-    * - __u8
-      - ``concealment_motion_vectors``
-      -  If set to 1, motion vectors are coded for intra macroblocks.
-    * - __u8
-      - ``q_scale_type``
+    * - __u32
+      - ``flags``
+      - See :ref:`MPEG-2 Picture Flags <mpeg2_picture_flags>`.
+
+
+.. _mpeg2_picture_flags:
+
+``MPEG-2 Picture Flags``
+
+.. cssclass:: longtable
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       1 1 2
+
+    * - ``V4L2_MPEG2_PIC_FLAG_TOP_FIELD_FIRST``
+      - 0x00000001
+      - If set and it's an interlaced stream, top field is output first.
+    * - ``V4L2_MPEG2_PIC_FLAG_FRAME_PRED_DCT``
+      - 0x00000002
+      - If set only frame-DCT and frame prediction are used.
+    * - ``V4L2_MPEG2_PIC_FLAG_CONCEALMENT_MV``
+      - 0x00000004
+      -  If set motion vectors are coded for intra macroblocks.
+    * - ``V4L2_MPEG2_PIC_FLAG_Q_SCALE_TYPE``
+      - 0x00000008
       - This flag affects the inverse quantization process.
-    * - __u8
-      - ``intra_vlc_format``
+    * - ``V4L2_MPEG2_PIC_FLAG_INTRA_VLC``
+      - 0x00000010
       - This flag affects the decoding of transform coefficient data.
-    * - __u8
-      - ``alternate_scan``
+    * - ``V4L2_MPEG2_PIC_FLAG_ALT_SCAN``
+      - 0x00000020
       - This flag affects the decoding of transform coefficient data.
-    * - __u8
-      - ``repeat_first_field``
+    * - ``V4L2_MPEG2_PIC_FLAG_REPEAT_FIRST``
+      - 0x00000040
       - This flag affects the decoding process of progressive frames.
-    * - __u16
-      - ``progressive_frame``
+    * - ``V4L2_MPEG2_PIC_FLAG_PROGRESSIVE``
+      - 0x00000080
       - Indicates whether the current frame is progressive.
+    * - ``V4L2_MPEG2_PIC_FLAG_LOAD_INTRA``
+    * - 0x00000100
+      - Indicate whether to load the user-specified intra quantiser matrix.
+    * - ``V4L2_MPEG2_PIC_FLAG_LOAD_NON_INTRA``
+    * - 0x00000200
+      - Indicate whether to load the user-specified non-intra quantiser matrix.
+    * - ``V4L2_MPEG2_PIC_FLAG_LOAD_CHROMA_INTRA``
+    * - 0x00000400
+      - Indicate whether to load the user-specified chroma intra quantiser
+        matrix, only relevant for 4:2:2 and 4:4:4 YUV formats.
+    * - ``V4L2_MPEG2_PIC_FLAG_LOAD_CHROMA_NON_INTRA``
+    * -  0x0000800
+      - Indicate whether to load the user-specified chroma non-intra quantiser
+        matrix, only relevant for 4:2:2 and 4:4:4 YUV formats.
 
 ``V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION (struct)``
     Specifies quantization matrices (as extracted from the bitstream) for the
@@ -1667,23 +1712,6 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     :stub-columns: 0
     :widths:       1 1 2
 
-    * - __u8
-      - ``load_intra_quantiser_matrix``
-      - One bit to indicate whether to load the ``intra_quantiser_matrix`` data.
-    * - __u8
-      - ``load_non_intra_quantiser_matrix``
-      - One bit to indicate whether to load the ``non_intra_quantiser_matrix``
-	data.
-    * - __u8
-      - ``load_chroma_intra_quantiser_matrix``
-      - One bit to indicate whether to load the
-	``chroma_intra_quantiser_matrix`` data, only relevant for non-4:2:0 YUV
-	formats.
-    * - __u8
-      - ``load_chroma_non_intra_quantiser_matrix``
-      - One bit to indicate whether to load the
-	``chroma_non_intra_quantiser_matrix`` data, only relevant for non-4:2:0
-	YUV formats.
     * - __u8
       - ``intra_quantiser_matrix[64]``
       - The quantization matrix coefficients for intra-coded frames, in zigzag
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 88231ba7b0fa..dd8f3bb4fbb9 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1647,7 +1647,7 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 		/* interlaced top field */
 		p_mpeg2_slice_params->picture.picture_structure = 1;
 		p_mpeg2_slice_params->picture.picture_coding_type =
-					V4L2_MPEG2_PICTURE_CODING_TYPE_I;
+					V4L2_MPEG2_PIC_CODING_TYPE_I;
 		break;
 	case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
 		p_vp8_frame_header = p;
@@ -1834,18 +1834,18 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 		}
 
 		switch (p_mpeg2_slice_params->picture.picture_structure) {
-		case 1: /* interlaced top field */
-		case 2: /* interlaced bottom field */
-		case 3: /* progressive */
+		case V4L2_MPEG2_PIC_TOP_FIELD:
+		case V4L2_MPEG2_PIC_BOTTOM_FIELD:
+		case V4L2_MPEG2_PIC_FRAME:
 			break;
 		default:
 			return -EINVAL;
 		}
 
 		switch (p_mpeg2_slice_params->picture.picture_coding_type) {
-		case V4L2_MPEG2_PICTURE_CODING_TYPE_I:
-		case V4L2_MPEG2_PICTURE_CODING_TYPE_P:
-		case V4L2_MPEG2_PICTURE_CODING_TYPE_B:
+		case V4L2_MPEG2_PIC_CODING_TYPE_I:
+		case V4L2_MPEG2_PIC_CODING_TYPE_P:
+		case V4L2_MPEG2_PIC_CODING_TYPE_B:
 			break;
 		default:
 			return -EINVAL;
diff --git a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
index 6386a3989bfe..9fe442244eb9 100644
--- a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
+++ b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
@@ -77,10 +77,6 @@
 
 #define G1_REG_APF_THRESHOLD(v)		(((v) << 0) & GENMASK(13, 0))
 
-#define PICT_TOP_FIELD     1
-#define PICT_BOTTOM_FIELD  2
-#define PICT_FRAME         3
-
 static void
 hantro_g1_mpeg2_dec_set_quantization(struct hantro_dev *vpu,
 				     struct hantro_ctx *ctx)
@@ -99,19 +95,19 @@ static void
 hantro_g1_mpeg2_dec_set_buffers(struct hantro_dev *vpu, struct hantro_ctx *ctx,
 				struct vb2_buffer *src_buf,
 				struct vb2_buffer *dst_buf,
-				const struct v4l2_mpeg2_sequence *sequence,
-				const struct v4l2_mpeg2_picture *picture,
+				const struct v4l2_mpeg2_sequence *seq,
+				const struct v4l2_mpeg2_picture *pic,
 				const struct v4l2_ctrl_mpeg2_slice_params *slice_params)
 {
 	dma_addr_t forward_addr = 0, backward_addr = 0;
 	dma_addr_t current_addr, addr;
 
-	switch (picture->picture_coding_type) {
-	case V4L2_MPEG2_PICTURE_CODING_TYPE_B:
+	switch (pic->picture_coding_type) {
+	case V4L2_MPEG2_PIC_CODING_TYPE_B:
 		backward_addr = hantro_get_ref(ctx,
 					       slice_params->backward_ref_ts);
 		fallthrough;
-	case V4L2_MPEG2_PICTURE_CODING_TYPE_P:
+	case V4L2_MPEG2_PIC_CODING_TYPE_P:
 		forward_addr = hantro_get_ref(ctx,
 					      slice_params->forward_ref_ts);
 	}
@@ -124,7 +120,7 @@ hantro_g1_mpeg2_dec_set_buffers(struct hantro_dev *vpu, struct hantro_ctx *ctx,
 	addr = hantro_get_dec_buf_addr(ctx, dst_buf);
 	current_addr = addr;
 
-	if (picture->picture_structure == PICT_BOTTOM_FIELD)
+	if (pic->picture_structure == V4L2_MPEG2_PIC_BOTTOM_FIELD)
 		addr += ALIGN(ctx->dst_fmt.width, 16);
 	vdpu_write_relaxed(vpu, addr, G1_REG_DEC_OUT_BASE);
 
@@ -134,18 +130,18 @@ hantro_g1_mpeg2_dec_set_buffers(struct hantro_dev *vpu, struct hantro_ctx *ctx,
 		backward_addr = current_addr;
 
 	/* Set forward ref frame (top/bottom field) */
-	if (picture->picture_structure == PICT_FRAME ||
-	    picture->picture_coding_type == V4L2_MPEG2_PICTURE_CODING_TYPE_B ||
-	    (picture->picture_structure == PICT_TOP_FIELD &&
-	     picture->top_field_first) ||
-	    (picture->picture_structure == PICT_BOTTOM_FIELD &&
-	     !picture->top_field_first)) {
+	if (pic->picture_structure == V4L2_MPEG2_PIC_FRAME ||
+	    pic->picture_coding_type == V4L2_MPEG2_PIC_CODING_TYPE_B ||
+	    (pic->picture_structure == V4L2_MPEG2_PIC_TOP_FIELD &&
+	     pic->flags & V4L2_MPEG2_PIC_FLAG_TOP_FIELD_FIRST) ||
+	    (pic->picture_structure == V4L2_MPEG2_PIC_BOTTOM_FIELD &&
+	     !(pic->flags & V4L2_MPEG2_PIC_FLAG_TOP_FIELD_FIRST))) {
 		vdpu_write_relaxed(vpu, forward_addr, G1_REG_REFER0_BASE);
 		vdpu_write_relaxed(vpu, forward_addr, G1_REG_REFER1_BASE);
-	} else if (picture->picture_structure == PICT_TOP_FIELD) {
+	} else if (pic->picture_structure == V4L2_MPEG2_PIC_TOP_FIELD) {
 		vdpu_write_relaxed(vpu, forward_addr, G1_REG_REFER0_BASE);
 		vdpu_write_relaxed(vpu, current_addr, G1_REG_REFER1_BASE);
-	} else if (picture->picture_structure == PICT_BOTTOM_FIELD) {
+	} else if (pic->picture_structure == V4L2_MPEG2_PIC_BOTTOM_FIELD) {
 		vdpu_write_relaxed(vpu, current_addr, G1_REG_REFER0_BASE);
 		vdpu_write_relaxed(vpu, forward_addr, G1_REG_REFER1_BASE);
 	}
@@ -160,8 +156,8 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx)
 	struct hantro_dev *vpu = ctx->dev;
 	struct vb2_v4l2_buffer *src_buf, *dst_buf;
 	const struct v4l2_ctrl_mpeg2_slice_params *slice_params;
-	const struct v4l2_mpeg2_sequence *sequence;
-	const struct v4l2_mpeg2_picture *picture;
+	const struct v4l2_mpeg2_sequence *seq;
+	const struct v4l2_mpeg2_picture *pic;
 	u32 reg;
 
 	src_buf = hantro_get_src_buf(ctx);
@@ -172,8 +168,8 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx)
 
 	slice_params = hantro_get_ctrl(ctx,
 				       V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS);
-	sequence = &slice_params->sequence;
-	picture = &slice_params->picture;
+	seq = &slice_params->sequence;
+	pic = &slice_params->picture;
 
 	reg = G1_REG_DEC_AXI_RD_ID(0) |
 	      G1_REG_DEC_TIMEOUT_E(1) |
@@ -193,11 +189,11 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx)
 
 	reg = G1_REG_DEC_MODE(5) |
 	      G1_REG_RLC_MODE_E(0) |
-	      G1_REG_PIC_INTERLACE_E(!sequence->progressive_sequence) |
-	      G1_REG_PIC_FIELDMODE_E(picture->picture_structure != PICT_FRAME) |
-	      G1_REG_PIC_B_E(picture->picture_coding_type == V4L2_MPEG2_PICTURE_CODING_TYPE_B) |
-	      G1_REG_PIC_INTER_E(picture->picture_coding_type != V4L2_MPEG2_PICTURE_CODING_TYPE_I) |
-	      G1_REG_PIC_TOPFIELD_E(picture->picture_structure == PICT_TOP_FIELD) |
+	      G1_REG_PIC_INTERLACE_E(!(seq->flags & V4L2_MPEG2_SEQ_FLAG_PROGRESSIVE)) |
+	      G1_REG_PIC_FIELDMODE_E(pic->picture_structure != V4L2_MPEG2_PIC_FRAME) |
+	      G1_REG_PIC_B_E(pic->picture_coding_type == V4L2_MPEG2_PIC_CODING_TYPE_B) |
+	      G1_REG_PIC_INTER_E(pic->picture_coding_type != V4L2_MPEG2_PIC_CODING_TYPE_I) |
+	      G1_REG_PIC_TOPFIELD_E(pic->picture_structure == V4L2_MPEG2_PIC_TOP_FIELD) |
 	      G1_REG_FWD_INTERLACE_E(0) |
 	      G1_REG_FILTERING_DIS(1) |
 	      G1_REG_WRITE_MVS_E(0) |
@@ -206,27 +202,27 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx)
 
 	reg = G1_REG_PIC_MB_WIDTH(MB_WIDTH(ctx->dst_fmt.width)) |
 	      G1_REG_PIC_MB_HEIGHT_P(MB_HEIGHT(ctx->dst_fmt.height)) |
-	      G1_REG_ALT_SCAN_E(picture->alternate_scan) |
-	      G1_REG_TOPFIELDFIRST_E(picture->top_field_first);
+	      G1_REG_ALT_SCAN_E(pic->flags & V4L2_MPEG2_PIC_FLAG_ALT_SCAN) |
+	      G1_REG_TOPFIELDFIRST_E(pic->flags & V4L2_MPEG2_PIC_FLAG_TOP_FIELD_FIRST);
 	vdpu_write_relaxed(vpu, reg, G1_SWREG(4));
 
 	reg = G1_REG_STRM_START_BIT(slice_params->data_bit_offset) |
-	      G1_REG_QSCALE_TYPE(picture->q_scale_type) |
-	      G1_REG_CON_MV_E(picture->concealment_motion_vectors) |
-	      G1_REG_INTRA_DC_PREC(picture->intra_dc_precision) |
-	      G1_REG_INTRA_VLC_TAB(picture->intra_vlc_format) |
-	      G1_REG_FRAME_PRED_DCT(picture->frame_pred_frame_dct);
+	      G1_REG_QSCALE_TYPE(pic->flags & V4L2_MPEG2_PIC_FLAG_Q_SCALE_TYPE) |
+	      G1_REG_CON_MV_E(pic->flags & V4L2_MPEG2_PIC_FLAG_CONCEALMENT_MV) |
+	      G1_REG_INTRA_DC_PREC(pic->intra_dc_precision) |
+	      G1_REG_INTRA_VLC_TAB(pic->flags & V4L2_MPEG2_PIC_FLAG_INTRA_VLC) |
+	      G1_REG_FRAME_PRED_DCT(pic->flags & V4L2_MPEG2_PIC_FLAG_FRAME_PRED_DCT);
 	vdpu_write_relaxed(vpu, reg, G1_SWREG(5));
 
 	reg = G1_REG_INIT_QP(1) |
 	      G1_REG_STREAM_LEN(slice_params->bit_size >> 3);
 	vdpu_write_relaxed(vpu, reg, G1_SWREG(6));
 
-	reg = G1_REG_ALT_SCAN_FLAG_E(picture->alternate_scan) |
-	      G1_REG_FCODE_FWD_HOR(picture->f_code[0][0]) |
-	      G1_REG_FCODE_FWD_VER(picture->f_code[0][1]) |
-	      G1_REG_FCODE_BWD_HOR(picture->f_code[1][0]) |
-	      G1_REG_FCODE_BWD_VER(picture->f_code[1][1]) |
+	reg = G1_REG_ALT_SCAN_FLAG_E(pic->flags & V4L2_MPEG2_PIC_FLAG_ALT_SCAN) |
+	      G1_REG_FCODE_FWD_HOR(pic->f_code[0][0]) |
+	      G1_REG_FCODE_FWD_VER(pic->f_code[0][1]) |
+	      G1_REG_FCODE_BWD_HOR(pic->f_code[1][0]) |
+	      G1_REG_FCODE_BWD_VER(pic->f_code[1][1]) |
 	      G1_REG_MV_ACCURACY_FWD(1) |
 	      G1_REG_MV_ACCURACY_BWD(1);
 	vdpu_write_relaxed(vpu, reg, G1_SWREG(18));
@@ -242,7 +238,7 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx)
 
 	hantro_g1_mpeg2_dec_set_buffers(vpu, ctx, &src_buf->vb2_buf,
 					&dst_buf->vb2_buf,
-					sequence, picture, slice_params);
+					seq, pic, slice_params);
 
 	hantro_end_prepare_run(ctx);
 
diff --git a/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c b/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c
index f610fa5b4335..9fdbf942cb99 100644
--- a/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c
+++ b/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c
@@ -79,10 +79,6 @@
 #define VDPU_REG_MV_ACCURACY_FWD(v)	((v) ? BIT(2) : 0)
 #define VDPU_REG_MV_ACCURACY_BWD(v)	((v) ? BIT(1) : 0)
 
-#define PICT_TOP_FIELD     1
-#define PICT_BOTTOM_FIELD  2
-#define PICT_FRAME         3
-
 static void
 rk3399_vpu_mpeg2_dec_set_quantization(struct hantro_dev *vpu,
 				      struct hantro_ctx *ctx)
@@ -101,19 +97,19 @@ rk3399_vpu_mpeg2_dec_set_buffers(struct hantro_dev *vpu,
 				 struct hantro_ctx *ctx,
 				 struct vb2_buffer *src_buf,
 				 struct vb2_buffer *dst_buf,
-				 const struct v4l2_mpeg2_sequence *sequence,
-				 const struct v4l2_mpeg2_picture *picture,
+				 const struct v4l2_mpeg2_sequence *seq,
+				 const struct v4l2_mpeg2_picture *pic,
 				 const struct v4l2_ctrl_mpeg2_slice_params *slice_params)
 {
 	dma_addr_t forward_addr = 0, backward_addr = 0;
 	dma_addr_t current_addr, addr;
 
-	switch (picture->picture_coding_type) {
-	case V4L2_MPEG2_PICTURE_CODING_TYPE_B:
+	switch (pic->picture_coding_type) {
+	case V4L2_MPEG2_PIC_CODING_TYPE_B:
 		backward_addr = hantro_get_ref(ctx,
 					       slice_params->backward_ref_ts);
 		fallthrough;
-	case V4L2_MPEG2_PICTURE_CODING_TYPE_P:
+	case V4L2_MPEG2_PIC_CODING_TYPE_P:
 		forward_addr = hantro_get_ref(ctx,
 					      slice_params->forward_ref_ts);
 	}
@@ -126,7 +122,7 @@ rk3399_vpu_mpeg2_dec_set_buffers(struct hantro_dev *vpu,
 	addr = vb2_dma_contig_plane_dma_addr(dst_buf, 0);
 	current_addr = addr;
 
-	if (picture->picture_structure == PICT_BOTTOM_FIELD)
+	if (pic->picture_structure == V4L2_MPEG2_PIC_BOTTOM_FIELD)
 		addr += ALIGN(ctx->dst_fmt.width, 16);
 	vdpu_write_relaxed(vpu, addr, VDPU_REG_DEC_OUT_BASE);
 
@@ -136,18 +132,18 @@ rk3399_vpu_mpeg2_dec_set_buffers(struct hantro_dev *vpu,
 		backward_addr = current_addr;
 
 	/* Set forward ref frame (top/bottom field) */
-	if (picture->picture_structure == PICT_FRAME ||
-	    picture->picture_coding_type == V4L2_MPEG2_PICTURE_CODING_TYPE_B ||
-	    (picture->picture_structure == PICT_TOP_FIELD &&
-	     picture->top_field_first) ||
-	    (picture->picture_structure == PICT_BOTTOM_FIELD &&
-	     !picture->top_field_first)) {
+	if (pic->picture_structure == V4L2_MPEG2_PIC_FRAME ||
+	    pic->picture_coding_type == V4L2_MPEG2_PIC_CODING_TYPE_B ||
+	    (pic->picture_structure == V4L2_MPEG2_PIC_TOP_FIELD &&
+	     pic->flags & V4L2_MPEG2_PIC_TOP_FIELD) ||
+	    (pic->picture_structure == V4L2_MPEG2_PIC_BOTTOM_FIELD &&
+	     !(pic->flags & V4L2_MPEG2_PIC_TOP_FIELD))) {
 		vdpu_write_relaxed(vpu, forward_addr, VDPU_REG_REFER0_BASE);
 		vdpu_write_relaxed(vpu, forward_addr, VDPU_REG_REFER1_BASE);
-	} else if (picture->picture_structure == PICT_TOP_FIELD) {
+	} else if (pic->picture_structure == V4L2_MPEG2_PIC_TOP_FIELD) {
 		vdpu_write_relaxed(vpu, forward_addr, VDPU_REG_REFER0_BASE);
 		vdpu_write_relaxed(vpu, current_addr, VDPU_REG_REFER1_BASE);
-	} else if (picture->picture_structure == PICT_BOTTOM_FIELD) {
+	} else if (pic->picture_structure == V4L2_MPEG2_PIC_BOTTOM_FIELD) {
 		vdpu_write_relaxed(vpu, current_addr, VDPU_REG_REFER0_BASE);
 		vdpu_write_relaxed(vpu, forward_addr, VDPU_REG_REFER1_BASE);
 	}
@@ -162,8 +158,8 @@ void rk3399_vpu_mpeg2_dec_run(struct hantro_ctx *ctx)
 	struct hantro_dev *vpu = ctx->dev;
 	struct vb2_v4l2_buffer *src_buf, *dst_buf;
 	const struct v4l2_ctrl_mpeg2_slice_params *slice_params;
-	const struct v4l2_mpeg2_sequence *sequence;
-	const struct v4l2_mpeg2_picture *picture;
+	const struct v4l2_mpeg2_sequence *seq;
+	const struct v4l2_mpeg2_picture *pic;
 	u32 reg;
 
 	src_buf = hantro_get_src_buf(ctx);
@@ -173,8 +169,8 @@ void rk3399_vpu_mpeg2_dec_run(struct hantro_ctx *ctx)
 
 	slice_params = hantro_get_ctrl(ctx,
 				       V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS);
-	sequence = &slice_params->sequence;
-	picture = &slice_params->picture;
+	seq = &slice_params->sequence;
+	pic = &slice_params->picture;
 
 	reg = VDPU_REG_DEC_ADV_PRE_DIS(0) |
 	      VDPU_REG_DEC_SCMD_DIS(0) |
@@ -209,11 +205,11 @@ void rk3399_vpu_mpeg2_dec_run(struct hantro_ctx *ctx)
 	vdpu_write_relaxed(vpu, reg, VDPU_SWREG(56));
 
 	reg = VDPU_REG_RLC_MODE_E(0) |
-	      VDPU_REG_PIC_INTERLACE_E(!sequence->progressive_sequence) |
-	      VDPU_REG_PIC_FIELDMODE_E(picture->picture_structure != PICT_FRAME) |
-	      VDPU_REG_PIC_B_E(picture->picture_coding_type == V4L2_MPEG2_PICTURE_CODING_TYPE_B) |
-	      VDPU_REG_PIC_INTER_E(picture->picture_coding_type != V4L2_MPEG2_PICTURE_CODING_TYPE_I) |
-	      VDPU_REG_PIC_TOPFIELD_E(picture->picture_structure == PICT_TOP_FIELD) |
+	      VDPU_REG_PIC_INTERLACE_E(!(seq->flags & V4L2_MPEG2_SEQ_FLAG_PROGRESSIVE)) |
+	      VDPU_REG_PIC_FIELDMODE_E(pic->picture_structure != V4L2_MPEG2_PIC_FRAME) |
+	      VDPU_REG_PIC_B_E(pic->picture_coding_type == V4L2_MPEG2_PIC_CODING_TYPE_B) |
+	      VDPU_REG_PIC_INTER_E(pic->picture_coding_type != V4L2_MPEG2_PIC_CODING_TYPE_I) |
+	      VDPU_REG_PIC_TOPFIELD_E(pic->picture_structure == V4L2_MPEG2_PIC_TOP_FIELD) |
 	      VDPU_REG_FWD_INTERLACE_E(0) |
 	      VDPU_REG_WRITE_MVS_E(0) |
 	      VDPU_REG_DEC_TIMEOUT_E(1) |
@@ -222,23 +218,23 @@ void rk3399_vpu_mpeg2_dec_run(struct hantro_ctx *ctx)
 
 	reg = VDPU_REG_PIC_MB_WIDTH(MB_WIDTH(ctx->dst_fmt.width)) |
 	      VDPU_REG_PIC_MB_HEIGHT_P(MB_HEIGHT(ctx->dst_fmt.height)) |
-	      VDPU_REG_ALT_SCAN_E(picture->alternate_scan) |
-	      VDPU_REG_TOPFIELDFIRST_E(picture->top_field_first);
+	      VDPU_REG_ALT_SCAN_E(pic->flags & V4L2_MPEG2_PIC_FLAG_ALT_SCAN) |
+	      VDPU_REG_TOPFIELDFIRST_E(pic->flags & V4L2_MPEG2_PIC_FLAG_TOP_FIELD_FIRST);
 	vdpu_write_relaxed(vpu, reg, VDPU_SWREG(120));
 
 	reg = VDPU_REG_STRM_START_BIT(slice_params->data_bit_offset) |
-	      VDPU_REG_QSCALE_TYPE(picture->q_scale_type) |
-	      VDPU_REG_CON_MV_E(picture->concealment_motion_vectors) |
-	      VDPU_REG_INTRA_DC_PREC(picture->intra_dc_precision) |
-	      VDPU_REG_INTRA_VLC_TAB(picture->intra_vlc_format) |
-	      VDPU_REG_FRAME_PRED_DCT(picture->frame_pred_frame_dct);
+	      VDPU_REG_QSCALE_TYPE(pic->flags & V4L2_MPEG2_PIC_FLAG_Q_SCALE_TYPE) |
+	      VDPU_REG_CON_MV_E(pic->flags & V4L2_MPEG2_PIC_FLAG_CONCEALMENT_MV) |
+	      VDPU_REG_INTRA_DC_PREC(pic->intra_dc_precision) |
+	      VDPU_REG_INTRA_VLC_TAB(pic->flags & V4L2_MPEG2_PIC_FLAG_INTRA_VLC) |
+	      VDPU_REG_FRAME_PRED_DCT(pic->flags & V4L2_MPEG2_PIC_FLAG_FRAME_PRED_DCT);
 	vdpu_write_relaxed(vpu, reg, VDPU_SWREG(122));
 
-	reg = VDPU_REG_ALT_SCAN_FLAG_E(picture->alternate_scan) |
-	      VDPU_REG_FCODE_FWD_HOR(picture->f_code[0][0]) |
-	      VDPU_REG_FCODE_FWD_VER(picture->f_code[0][1]) |
-	      VDPU_REG_FCODE_BWD_HOR(picture->f_code[1][0]) |
-	      VDPU_REG_FCODE_BWD_VER(picture->f_code[1][1]) |
+	reg = VDPU_REG_ALT_SCAN_FLAG_E(pic->flags & V4L2_MPEG2_PIC_FLAG_ALT_SCAN) |
+	      VDPU_REG_FCODE_FWD_HOR(pic->f_code[0][0]) |
+	      VDPU_REG_FCODE_FWD_VER(pic->f_code[0][1]) |
+	      VDPU_REG_FCODE_BWD_HOR(pic->f_code[1][0]) |
+	      VDPU_REG_FCODE_BWD_VER(pic->f_code[1][1]) |
 	      VDPU_REG_MV_ACCURACY_FWD(1) |
 	      VDPU_REG_MV_ACCURACY_BWD(1);
 	vdpu_write_relaxed(vpu, reg, VDPU_SWREG(136));
@@ -247,7 +243,7 @@ void rk3399_vpu_mpeg2_dec_run(struct hantro_ctx *ctx)
 
 	rk3399_vpu_mpeg2_dec_set_buffers(vpu, ctx, &src_buf->vb2_buf,
 					 &dst_buf->vb2_buf,
-					 sequence, picture, slice_params);
+					 seq, pic, slice_params);
 
 	/* Kick the watchdog and start decoding */
 	hantro_end_prepare_run(ctx);
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
index 8bcd6b8f9e2d..855536702bc1 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
@@ -75,8 +75,8 @@ static void cedrus_mpeg2_irq_disable(struct cedrus_ctx *ctx)
 static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 {
 	const struct v4l2_ctrl_mpeg2_slice_params *slice_params;
-	const struct v4l2_mpeg2_sequence *sequence;
-	const struct v4l2_mpeg2_picture *picture;
+	const struct v4l2_mpeg2_sequence *seq;
+	const struct v4l2_mpeg2_picture *pic;
 	const struct v4l2_ctrl_mpeg2_quantization *quantization;
 	dma_addr_t src_buf_addr, dst_luma_addr, dst_chroma_addr;
 	dma_addr_t fwd_luma_addr, fwd_chroma_addr;
@@ -90,8 +90,8 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 	u32 reg;
 
 	slice_params = run->mpeg2.slice_params;
-	sequence = &slice_params->sequence;
-	picture = &slice_params->picture;
+	seq = &slice_params->sequence;
+	pic = &slice_params->picture;
 
 	quantization = run->mpeg2.quantization;
 
@@ -100,7 +100,7 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 
 	/* Set intra quantization matrix. */
 
-	if (quantization && quantization->load_intra_quantiser_matrix)
+	if (pic->flags & V4L2_MPEG2_PIC_FLAG_LOAD_INTRA)
 		matrix = quantization->intra_quantiser_matrix;
 	else
 		matrix = intra_quantization_matrix_default;
@@ -114,7 +114,7 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 
 	/* Set non-intra quantization matrix. */
 
-	if (quantization && quantization->load_non_intra_quantiser_matrix)
+	if (pic->flags & V4L2_MPEG2_PIC_FLAG_LOAD_NON_INTRA)
 		matrix = quantization->non_intra_quantiser_matrix;
 	else
 		matrix = non_intra_quantization_matrix_default;
@@ -128,19 +128,19 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 
 	/* Set MPEG picture header. */
 
-	reg = VE_DEC_MPEG_MP12HDR_SLICE_TYPE(picture->picture_coding_type);
-	reg |= VE_DEC_MPEG_MP12HDR_F_CODE(0, 0, picture->f_code[0][0]);
-	reg |= VE_DEC_MPEG_MP12HDR_F_CODE(0, 1, picture->f_code[0][1]);
-	reg |= VE_DEC_MPEG_MP12HDR_F_CODE(1, 0, picture->f_code[1][0]);
-	reg |= VE_DEC_MPEG_MP12HDR_F_CODE(1, 1, picture->f_code[1][1]);
-	reg |= VE_DEC_MPEG_MP12HDR_INTRA_DC_PRECISION(picture->intra_dc_precision);
-	reg |= VE_DEC_MPEG_MP12HDR_INTRA_PICTURE_STRUCTURE(picture->picture_structure);
-	reg |= VE_DEC_MPEG_MP12HDR_TOP_FIELD_FIRST(picture->top_field_first);
-	reg |= VE_DEC_MPEG_MP12HDR_FRAME_PRED_FRAME_DCT(picture->frame_pred_frame_dct);
-	reg |= VE_DEC_MPEG_MP12HDR_CONCEALMENT_MOTION_VECTORS(picture->concealment_motion_vectors);
-	reg |= VE_DEC_MPEG_MP12HDR_Q_SCALE_TYPE(picture->q_scale_type);
-	reg |= VE_DEC_MPEG_MP12HDR_INTRA_VLC_FORMAT(picture->intra_vlc_format);
-	reg |= VE_DEC_MPEG_MP12HDR_ALTERNATE_SCAN(picture->alternate_scan);
+	reg = VE_DEC_MPEG_MP12HDR_SLICE_TYPE(pic->picture_coding_type);
+	reg |= VE_DEC_MPEG_MP12HDR_F_CODE(0, 0, pic->f_code[0][0]);
+	reg |= VE_DEC_MPEG_MP12HDR_F_CODE(0, 1, pic->f_code[0][1]);
+	reg |= VE_DEC_MPEG_MP12HDR_F_CODE(1, 0, pic->f_code[1][0]);
+	reg |= VE_DEC_MPEG_MP12HDR_F_CODE(1, 1, pic->f_code[1][1]);
+	reg |= VE_DEC_MPEG_MP12HDR_INTRA_DC_PRECISION(pic->intra_dc_precision);
+	reg |= VE_DEC_MPEG_MP12HDR_INTRA_PICTURE_STRUCTURE(pic->picture_structure);
+	reg |= VE_DEC_MPEG_MP12HDR_TOP_FIELD_FIRST(pic->flags & V4L2_MPEG2_PIC_FLAG_TOP_FIELD_FIRST);
+	reg |= VE_DEC_MPEG_MP12HDR_FRAME_PRED_FRAME_DCT(pic->flags & V4L2_MPEG2_PIC_FLAG_FRAME_PRED_DCT);
+	reg |= VE_DEC_MPEG_MP12HDR_CONCEALMENT_MOTION_VECTORS(pic->flags & V4L2_MPEG2_PIC_FLAG_CONCEALMENT_MV);
+	reg |= VE_DEC_MPEG_MP12HDR_Q_SCALE_TYPE(pic->flags & V4L2_MPEG2_PIC_FLAG_Q_SCALE_TYPE);
+	reg |= VE_DEC_MPEG_MP12HDR_INTRA_VLC_FORMAT(pic->flags & V4L2_MPEG2_PIC_FLAG_INTRA_VLC);
+	reg |= VE_DEC_MPEG_MP12HDR_ALTERNATE_SCAN(pic->flags & V4L2_MPEG2_PIC_FLAG_ALT_SCAN);
 	reg |= VE_DEC_MPEG_MP12HDR_FULL_PEL_FORWARD_VECTOR(0);
 	reg |= VE_DEC_MPEG_MP12HDR_FULL_PEL_BACKWARD_VECTOR(0);
 
@@ -148,8 +148,8 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 
 	/* Set frame dimensions. */
 
-	reg = VE_DEC_MPEG_PICCODEDSIZE_WIDTH(sequence->horizontal_size);
-	reg |= VE_DEC_MPEG_PICCODEDSIZE_HEIGHT(sequence->vertical_size);
+	reg = VE_DEC_MPEG_PICCODEDSIZE_WIDTH(seq->horizontal_size);
+	reg |= VE_DEC_MPEG_PICCODEDSIZE_HEIGHT(seq->vertical_size);
 
 	cedrus_write(dev, VE_DEC_MPEG_PICCODEDSIZE, reg);
 
diff --git a/include/media/mpeg2-ctrls.h b/include/media/mpeg2-ctrls.h
index 2a4ae6701166..4bc85cd99b15 100644
--- a/include/media/mpeg2-ctrls.h
+++ b/include/media/mpeg2-ctrls.h
@@ -18,10 +18,7 @@
 #define V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS 0x0103
 #define	V4L2_CTRL_TYPE_MPEG2_QUANTIZATION 0x0104
 
-#define V4L2_MPEG2_PICTURE_CODING_TYPE_I	1
-#define V4L2_MPEG2_PICTURE_CODING_TYPE_P	2
-#define V4L2_MPEG2_PICTURE_CODING_TYPE_B	3
-#define V4L2_MPEG2_PICTURE_CODING_TYPE_D	4
+#define V4L2_MPEG2_SEQ_FLAG_PROGRESSIVE		0x0001
 
 struct v4l2_mpeg2_sequence {
 	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Sequence header */
@@ -31,10 +28,33 @@ struct v4l2_mpeg2_sequence {
 
 	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Sequence extension */
 	__u16	profile_and_level_indication;
-	__u8	progressive_sequence;
 	__u8	chroma_format;
+
+	__u32	flags;
 };
 
+#define V4L2_MPEG2_PIC_CODING_TYPE_I			1
+#define V4L2_MPEG2_PIC_CODING_TYPE_P			2
+#define V4L2_MPEG2_PIC_CODING_TYPE_B			3
+#define V4L2_MPEG2_PIC_CODING_TYPE_D			4
+
+#define V4L2_MPEG2_PIC_TOP_FIELD			0x1
+#define V4L2_MPEG2_PIC_BOTTOM_FIELD			0x2
+#define V4L2_MPEG2_PIC_FRAME				0x3
+
+#define V4L2_MPEG2_PIC_FLAG_TOP_FIELD_FIRST		0x0001
+#define V4L2_MPEG2_PIC_FLAG_FRAME_PRED_DCT		0x0002
+#define V4L2_MPEG2_PIC_FLAG_CONCEALMENT_MV		0x0004
+#define V4L2_MPEG2_PIC_FLAG_Q_SCALE_TYPE		0x0008
+#define V4L2_MPEG2_PIC_FLAG_INTRA_VLC			0x0010
+#define V4L2_MPEG2_PIC_FLAG_ALT_SCAN			0x0020
+#define V4L2_MPEG2_PIC_FLAG_REPEAT_FIRST		0x0040
+#define V4L2_MPEG2_PIC_FLAG_PROGRESSIVE			0x0080
+#define V4L2_MPEG2_PIC_FLAG_LOAD_INTRA			0x0100
+#define V4L2_MPEG2_PIC_FLAG_LOAD_NON_INTRA		0x0200
+#define V4L2_MPEG2_PIC_FLAG_LOAD_CHROMA_INTRA		0x0400
+#define V4L2_MPEG2_PIC_FLAG_LOAD_CHROMA_NON_INTRA	0x0800
+
 struct v4l2_mpeg2_picture {
 	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Picture header */
 	__u8	picture_coding_type;
@@ -43,14 +63,8 @@ struct v4l2_mpeg2_picture {
 	__u8	f_code[2][2];
 	__u8	intra_dc_precision;
 	__u8	picture_structure;
-	__u8	top_field_first;
-	__u8	frame_pred_frame_dct;
-	__u8	concealment_motion_vectors;
-	__u8	q_scale_type;
-	__u8	intra_vlc_format;
-	__u8	alternate_scan;
-	__u8	repeat_first_field;
-	__u16	progressive_frame;
+
+	__u32	flags;
 };
 
 struct v4l2_ctrl_mpeg2_slice_params {
@@ -66,13 +80,8 @@ struct v4l2_ctrl_mpeg2_slice_params {
 	__u32	quantiser_scale_code;
 };
 
+/* ISO/IEC 13818-2, ITU-T Rec. H.262: Quant matrix extension */
 struct v4l2_ctrl_mpeg2_quantization {
-	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Quant matrix extension */
-	__u8	load_intra_quantiser_matrix;
-	__u8	load_non_intra_quantiser_matrix;
-	__u8	load_chroma_intra_quantiser_matrix;
-	__u8	load_chroma_non_intra_quantiser_matrix;
-
 	__u8	intra_quantiser_matrix[64];
 	__u8	non_intra_quantiser_matrix[64];
 	__u8	chroma_intra_quantiser_matrix[64];
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* [PATCH v3 2/3] media: uapi: mpeg2: Remove unused slice size and offset
  2020-11-30 18:52 [PATCH v3 0/3] MPEG-2 stateless API cleanup Ezequiel Garcia
  2020-11-30 18:52 ` [PATCH v3 1/3] media: uapi: mpeg2: Cleanup flags Ezequiel Garcia
@ 2020-11-30 18:52 ` Ezequiel Garcia
  2020-11-30 18:52 ` [PATCH v3 3/3] media: uapi: mpeg2: Split sequence and picture parameters Ezequiel Garcia
  2 siblings, 0 replies; 7+ messages in thread
From: Ezequiel Garcia @ 2020-11-30 18:52 UTC (permalink / raw)
  To: linux-media, linux-kernel
  Cc: kernel, Jonas Karlman, Hans Verkuil, Nicolas Dufresne,
	Philipp Zabel, Maxime Ripard, Paul Kocialkowski, Jernej Skrabec,
	Ezequiel Garcia

The MPEG2_SLICE_PARAMS control is designed to refer to a
single slice. Conversely, in each request the OUTPUT buffer
is expected to hold a single slice, so the offset
and size fields are not needed.

The start of the slice is aligned to the start of the buffer,
and the buffer's plane payload size is the slice size.

To reduce the API surface and avoid confusion drop these
fields from control.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Tested-by: Jonas Karlman <jonas@kwiboo.se>
---
 Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 6 ------
 drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c        | 4 ++--
 drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c    | 4 ++--
 drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c         | 7 +++----
 include/media/mpeg2-ctrls.h                               | 2 --
 5 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index 29da24d01aba..9a804f3037d8 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1526,12 +1526,6 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     :stub-columns: 0
     :widths:       1 1 2
 
-    * - __u32
-      - ``bit_size``
-      - Size (in bits) of the current slice data.
-    * - __u32
-      - ``data_bit_offset``
-      - Offset (in bits) to the video data in the current slice data.
     * - struct :c:type:`v4l2_mpeg2_sequence`
       - ``sequence``
       - Structure with MPEG-2 sequence metadata, merging relevant fields from
diff --git a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
index 9fe442244eb9..b6086474d31f 100644
--- a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
+++ b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
@@ -206,7 +206,7 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx)
 	      G1_REG_TOPFIELDFIRST_E(pic->flags & V4L2_MPEG2_PIC_FLAG_TOP_FIELD_FIRST);
 	vdpu_write_relaxed(vpu, reg, G1_SWREG(4));
 
-	reg = G1_REG_STRM_START_BIT(slice_params->data_bit_offset) |
+	reg = G1_REG_STRM_START_BIT(0) |
 	      G1_REG_QSCALE_TYPE(pic->flags & V4L2_MPEG2_PIC_FLAG_Q_SCALE_TYPE) |
 	      G1_REG_CON_MV_E(pic->flags & V4L2_MPEG2_PIC_FLAG_CONCEALMENT_MV) |
 	      G1_REG_INTRA_DC_PREC(pic->intra_dc_precision) |
@@ -215,7 +215,7 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx)
 	vdpu_write_relaxed(vpu, reg, G1_SWREG(5));
 
 	reg = G1_REG_INIT_QP(1) |
-	      G1_REG_STREAM_LEN(slice_params->bit_size >> 3);
+	      G1_REG_STREAM_LEN(vb2_get_plane_payload(&src_buf->vb2_buf, 0));
 	vdpu_write_relaxed(vpu, reg, G1_SWREG(6));
 
 	reg = G1_REG_ALT_SCAN_FLAG_E(pic->flags & V4L2_MPEG2_PIC_FLAG_ALT_SCAN) |
diff --git a/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c b/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c
index 9fdbf942cb99..28eb77b0569b 100644
--- a/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c
+++ b/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c
@@ -179,7 +179,7 @@ void rk3399_vpu_mpeg2_dec_run(struct hantro_ctx *ctx)
 	vdpu_write_relaxed(vpu, reg, VDPU_SWREG(50));
 
 	reg = VDPU_REG_INIT_QP(1) |
-	      VDPU_REG_STREAM_LEN(slice_params->bit_size >> 3);
+	      VDPU_REG_STREAM_LEN(vb2_get_plane_payload(&src_buf->vb2_buf, 0));
 	vdpu_write_relaxed(vpu, reg, VDPU_SWREG(51));
 
 	reg = VDPU_REG_APF_THRESHOLD(8) |
@@ -222,7 +222,7 @@ void rk3399_vpu_mpeg2_dec_run(struct hantro_ctx *ctx)
 	      VDPU_REG_TOPFIELDFIRST_E(pic->flags & V4L2_MPEG2_PIC_FLAG_TOP_FIELD_FIRST);
 	vdpu_write_relaxed(vpu, reg, VDPU_SWREG(120));
 
-	reg = VDPU_REG_STRM_START_BIT(slice_params->data_bit_offset) |
+	reg = VDPU_REG_STRM_START_BIT(0) |
 	      VDPU_REG_QSCALE_TYPE(pic->flags & V4L2_MPEG2_PIC_FLAG_Q_SCALE_TYPE) |
 	      VDPU_REG_CON_MV_E(pic->flags & V4L2_MPEG2_PIC_FLAG_CONCEALMENT_MV) |
 	      VDPU_REG_INTRA_DC_PREC(pic->intra_dc_precision) |
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
index 855536702bc1..6a99893cdc77 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
@@ -186,10 +186,9 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 
 	/* Source offset and length in bits. */
 
-	cedrus_write(dev, VE_DEC_MPEG_VLD_OFFSET,
-		     slice_params->data_bit_offset);
+	cedrus_write(dev, VE_DEC_MPEG_VLD_OFFSET, 0);
 
-	reg = slice_params->bit_size - slice_params->data_bit_offset;
+	reg = vb2_get_plane_payload(&run->src->vb2_buf, 0) * 8;
 	cedrus_write(dev, VE_DEC_MPEG_VLD_LEN, reg);
 
 	/* Source beginning and end addresses. */
@@ -203,7 +202,7 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 
 	cedrus_write(dev, VE_DEC_MPEG_VLD_ADDR, reg);
 
-	reg = src_buf_addr + DIV_ROUND_UP(slice_params->bit_size, 8);
+	reg = src_buf_addr + vb2_get_plane_payload(&run->src->vb2_buf, 0);
 	cedrus_write(dev, VE_DEC_MPEG_VLD_END_ADDR, reg);
 
 	/* Macroblock address: start at the beginning. */
diff --git a/include/media/mpeg2-ctrls.h b/include/media/mpeg2-ctrls.h
index 4bc85cd99b15..038206c7e6f5 100644
--- a/include/media/mpeg2-ctrls.h
+++ b/include/media/mpeg2-ctrls.h
@@ -68,8 +68,6 @@ struct v4l2_mpeg2_picture {
 };
 
 struct v4l2_ctrl_mpeg2_slice_params {
-	__u32	bit_size;
-	__u32	data_bit_offset;
 	__u64	backward_ref_ts;
 	__u64	forward_ref_ts;
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* [PATCH v3 3/3] media: uapi: mpeg2: Split sequence and picture parameters
  2020-11-30 18:52 [PATCH v3 0/3] MPEG-2 stateless API cleanup Ezequiel Garcia
  2020-11-30 18:52 ` [PATCH v3 1/3] media: uapi: mpeg2: Cleanup flags Ezequiel Garcia
  2020-11-30 18:52 ` [PATCH v3 2/3] media: uapi: mpeg2: Remove unused slice size and offset Ezequiel Garcia
@ 2020-11-30 18:52 ` Ezequiel Garcia
  2020-12-02 15:11   ` Hans Verkuil
  2 siblings, 1 reply; 7+ messages in thread
From: Ezequiel Garcia @ 2020-11-30 18:52 UTC (permalink / raw)
  To: linux-media, linux-kernel
  Cc: kernel, Jonas Karlman, Hans Verkuil, Nicolas Dufresne,
	Philipp Zabel, Maxime Ripard, Paul Kocialkowski, Jernej Skrabec,
	Ezequiel Garcia

Typically, bitstreams are composed of one sequence header NAL unit,
followed by a number of picture header and picture coding extension
NAL units. Each picture can be composed by a number of slices.

Let's split the MPEG-2 uAPI to follow these semantics more closely,
allowing more usage flexibility. Having these controls split
allows applications to set a sequence control at the beginning
of a sequence, and then set a picture control for each frame.

While here add padding fields where needed, and document
the uAPI header thoroughly.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Tested-by: Jonas Karlman <jonas@kwiboo.se>
---
 .../media/v4l/ext-ctrls-codec.rst             |  47 ++++++--
 .../media/v4l/pixfmt-compressed.rst           |   5 +-
 drivers/media/v4l2-core/v4l2-ctrls.c          |  57 +++++++--
 drivers/staging/media/hantro/hantro_drv.c     |  10 ++
 .../media/hantro/hantro_g1_mpeg2_dec.c        |  14 ++-
 .../media/hantro/rk3399_vpu_hw_mpeg2_dec.c    |  14 ++-
 drivers/staging/media/sunxi/cedrus/cedrus.c   |  14 +++
 drivers/staging/media/sunxi/cedrus/cedrus.h   |   2 +
 .../staging/media/sunxi/cedrus/cedrus_mpeg2.c |   8 +-
 include/media/mpeg2-ctrls.h                   | 110 +++++++++++++++---
 include/media/v4l2-ctrls.h                    |   4 +
 11 files changed, 224 insertions(+), 61 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index 9a804f3037d8..a789866ebda4 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1526,14 +1526,6 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     :stub-columns: 0
     :widths:       1 1 2
 
-    * - struct :c:type:`v4l2_mpeg2_sequence`
-      - ``sequence``
-      - Structure with MPEG-2 sequence metadata, merging relevant fields from
-	the sequence header and sequence extension parts of the bitstream.
-    * - struct :c:type:`v4l2_mpeg2_picture`
-      - ``picture``
-      - Structure with MPEG-2 picture metadata, merging relevant fields from
-	the picture header and picture coding extension parts of the bitstream.
     * - __u64
       - ``backward_ref_ts``
       - Timestamp of the V4L2 capture buffer to use as backward reference, used
@@ -1551,14 +1543,28 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __u32
       - ``quantiser_scale_code``
       - Code used to determine the quantization scale to use for the IDCT.
+    * - __u8
+      - ``reserved``
+      - Applications and drivers must set this to zero.
 
-.. c:type:: v4l2_mpeg2_sequence
+``V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE (struct)``
+    Specifies the sequence parameters (as extracted from the bitstream) for the
+    associated MPEG-2 slice data. This includes fields matching the syntax
+    elements from the sequence header and sequence extension parts of the
+    bitstream as specified by :ref:`mpeg2part2`.
+
+    .. note::
+
+       This compound control is not yet part of the public kernel API and
+       it is expected to change.
+
+.. c:type:: v4l2_ctrl_mpeg2_sequence
 
 .. cssclass:: longtable
 
 .. tabularcolumns:: |p{1.5cm}|p{6.3cm}|p{9.4cm}|
 
-.. flat-table:: struct v4l2_mpeg2_sequence
+.. flat-table:: struct v4l2_ctrl_mpeg2_sequence
     :header-rows:  0
     :stub-columns: 0
     :widths:       1 1 2
@@ -1580,6 +1586,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __u8
       - ``chroma_format``
       - The chrominance sub-sampling format (1: 4:2:0, 2: 4:2:2, 3: 4:4:4).
+    * - __u8
+      - ``reserved``
+      - Applications and drivers must set this to zero.
     * - __u32
       - ``flags``
       - See :ref:`MPEG-2 Sequence Flags <mpeg2_sequence_flags>`.
@@ -1600,13 +1609,24 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
       - Indication that all the frames for the sequence are progressive instead
 	of interlaced.
 
-.. c:type:: v4l2_mpeg2_picture
+``V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE (struct)``
+    Specifies the picture parameters (as extracted from the bitstream) for the
+    associated MPEG-2 slice data. This includes fields matching the syntax
+    elements from the picture header and picture coding extension parts of the
+    bitstream as specified by :ref:`mpeg2part2`.
+
+    .. note::
+
+       This compound control is not yet part of the public kernel API and
+       it is expected to change.
+
+.. c:type:: v4l2_ctrl_mpeg2_picture
 
 .. cssclass:: longtable
 
 .. tabularcolumns:: |p{1.5cm}|p{6.3cm}|p{9.4cm}|
 
-.. flat-table:: struct v4l2_mpeg2_picture
+.. flat-table:: struct v4l2_ctrl_mpeg2_picture
     :header-rows:  0
     :stub-columns: 0
     :widths:       1 1 2
@@ -1627,6 +1647,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
       - ``picture_structure``
       - Picture structure (1: interlaced top field, 2: interlaced bottom field,
 	3: progressive frame).
+    * - __u8
+      - ``reserved``
+      - Applications and drivers must set this to zero.
     * - __u32
       - ``flags``
       - See :ref:`MPEG-2 Picture Flags <mpeg2_picture_flags>`.
diff --git a/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst b/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
index b8899262d8de..0c22f3f99ce4 100644
--- a/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
+++ b/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
@@ -108,8 +108,9 @@ Compressed Formats
 	This format is adapted for stateless video decoders that implement a
 	MPEG-2 pipeline (using the :ref:`mem2mem` and :ref:`media-request-api`).
 	Metadata associated with the frame to decode is required to be passed
-	through the ``V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS`` control and
-	quantization matrices can optionally be specified through the
+	through the ``V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE``,
+        ``V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE``, and ``V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS``
+        controls. Quantization matrices can optionally be specified through the
 	``V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION`` control.
 	See the :ref:`associated Codec Control IDs <v4l2-mpeg-mpeg2>`.
 	Exactly one output and one capture buffer must be provided for use with
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index dd8f3bb4fbb9..a43baa9367ab 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -941,6 +941,8 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:		return "Vertical MV Search Range";
 	case V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER:		return "Repeat Sequence Header";
 	case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:		return "Force Key Frame";
+	case V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE:		return "MPEG-2 Sequence Header";
+	case V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE:			return "MPEG-2 Picture Header";
 	case V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS:		return "MPEG-2 Slice Parameters";
 	case V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION:		return "MPEG-2 Quantization Matrices";
 	case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS:			return "FWHT Stateless Parameters";
@@ -1427,6 +1429,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_RDS_TX_ALT_FREQS:
 		*type = V4L2_CTRL_TYPE_U32;
 		break;
+	case V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE:
+		*type = V4L2_CTRL_TYPE_MPEG2_SEQUENCE;
+		break;
+	case V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE:
+		*type = V4L2_CTRL_TYPE_MPEG2_PICTURE;
+		break;
 	case V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS:
 		*type = V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS;
 		break;
@@ -1625,7 +1633,8 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
 static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 			      union v4l2_ctrl_ptr ptr)
 {
-	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
+	struct v4l2_ctrl_mpeg2_sequence *p_mpeg2_sequence;
+	struct v4l2_ctrl_mpeg2_picture *p_mpeg2_picture;
 	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
 	void *p = ptr.p + idx * ctrl->elem_size;
 
@@ -1640,13 +1649,18 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 	 * v4l2_ctrl_type enum.
 	 */
 	switch ((u32)ctrl->type) {
-	case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
-		p_mpeg2_slice_params = p;
+	case V4L2_CTRL_TYPE_MPEG2_SEQUENCE:
+		p_mpeg2_sequence = p;
+
 		/* 4:2:0 */
-		p_mpeg2_slice_params->sequence.chroma_format = 1;
+		p_mpeg2_sequence->chroma_format = 1;
+		break;
+	case V4L2_CTRL_TYPE_MPEG2_PICTURE:
+		p_mpeg2_picture = p;
+
 		/* interlaced top field */
-		p_mpeg2_slice_params->picture.picture_structure = 1;
-		p_mpeg2_slice_params->picture.picture_coding_type =
+		p_mpeg2_picture->picture_structure = 1;
+		p_mpeg2_picture->picture_coding_type =
 					V4L2_MPEG2_PIC_CODING_TYPE_I;
 		break;
 	case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
@@ -1796,6 +1810,8 @@ static void std_log(const struct v4l2_ctrl *ctrl)
 static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 				 union v4l2_ctrl_ptr ptr)
 {
+	struct v4l2_ctrl_mpeg2_sequence *p_mpeg2_sequence;
+	struct v4l2_ctrl_mpeg2_picture *p_mpeg2_picture;
 	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
 	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
 	struct v4l2_ctrl_h264_sps *p_h264_sps;
@@ -1811,10 +1827,10 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 	unsigned int i;
 
 	switch ((u32)ctrl->type) {
-	case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
-		p_mpeg2_slice_params = p;
+	case V4L2_CTRL_TYPE_MPEG2_SEQUENCE:
+		p_mpeg2_sequence = p;
 
-		switch (p_mpeg2_slice_params->sequence.chroma_format) {
+		switch (p_mpeg2_sequence->chroma_format) {
 		case 1: /* 4:2:0 */
 		case 2: /* 4:2:2 */
 		case 3: /* 4:4:4 */
@@ -1822,8 +1838,13 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 		default:
 			return -EINVAL;
 		}
+		zero_reserved(*p_mpeg2_sequence);
+		break;
+
+	case V4L2_CTRL_TYPE_MPEG2_PICTURE:
+		p_mpeg2_picture = p;
 
-		switch (p_mpeg2_slice_params->picture.intra_dc_precision) {
+		switch (p_mpeg2_picture->intra_dc_precision) {
 		case 0: /* 8 bits */
 		case 1: /* 9 bits */
 		case 2: /* 10 bits */
@@ -1833,7 +1854,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 			return -EINVAL;
 		}
 
-		switch (p_mpeg2_slice_params->picture.picture_structure) {
+		switch (p_mpeg2_picture->picture_structure) {
 		case V4L2_MPEG2_PIC_TOP_FIELD:
 		case V4L2_MPEG2_PIC_BOTTOM_FIELD:
 		case V4L2_MPEG2_PIC_FRAME:
@@ -1842,7 +1863,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 			return -EINVAL;
 		}
 
-		switch (p_mpeg2_slice_params->picture.picture_coding_type) {
+		switch (p_mpeg2_picture->picture_coding_type) {
 		case V4L2_MPEG2_PIC_CODING_TYPE_I:
 		case V4L2_MPEG2_PIC_CODING_TYPE_P:
 		case V4L2_MPEG2_PIC_CODING_TYPE_B:
@@ -1850,7 +1871,13 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 		default:
 			return -EINVAL;
 		}
+		zero_reserved(*p_mpeg2_picture);
+		break;
 
+	case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
+		p_mpeg2_slice_params = p;
+
+		zero_reserved(*p_mpeg2_slice_params);
 		break;
 
 	case V4L2_CTRL_TYPE_MPEG2_QUANTIZATION:
@@ -2749,6 +2776,12 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	case V4L2_CTRL_TYPE_U32:
 		elem_size = sizeof(u32);
 		break;
+	case V4L2_CTRL_TYPE_MPEG2_SEQUENCE:
+		elem_size = sizeof(struct v4l2_ctrl_mpeg2_sequence);
+		break;
+	case V4L2_CTRL_TYPE_MPEG2_PICTURE:
+		elem_size = sizeof(struct v4l2_ctrl_mpeg2_picture);
+		break;
 	case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
 		elem_size = sizeof(struct v4l2_ctrl_mpeg2_slice_params);
 		break;
diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index e5f200e64993..e589eccd5644 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -286,6 +286,16 @@ static const struct hantro_ctrl controls[] = {
 			.def = 50,
 			.ops = &hantro_jpeg_ctrl_ops,
 		},
+	}, {
+		.codec = HANTRO_MPEG2_DECODER,
+		.cfg = {
+			.id = V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE,
+		},
+	}, {
+		.codec = HANTRO_MPEG2_DECODER,
+		.cfg = {
+			.id = V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE,
+		},
 	}, {
 		.codec = HANTRO_MPEG2_DECODER,
 		.cfg = {
diff --git a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
index b6086474d31f..1a6ca49441f4 100644
--- a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
+++ b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
@@ -95,8 +95,8 @@ static void
 hantro_g1_mpeg2_dec_set_buffers(struct hantro_dev *vpu, struct hantro_ctx *ctx,
 				struct vb2_buffer *src_buf,
 				struct vb2_buffer *dst_buf,
-				const struct v4l2_mpeg2_sequence *seq,
-				const struct v4l2_mpeg2_picture *pic,
+				const struct v4l2_ctrl_mpeg2_sequence *seq,
+				const struct v4l2_ctrl_mpeg2_picture *pic,
 				const struct v4l2_ctrl_mpeg2_slice_params *slice_params)
 {
 	dma_addr_t forward_addr = 0, backward_addr = 0;
@@ -156,8 +156,8 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx)
 	struct hantro_dev *vpu = ctx->dev;
 	struct vb2_v4l2_buffer *src_buf, *dst_buf;
 	const struct v4l2_ctrl_mpeg2_slice_params *slice_params;
-	const struct v4l2_mpeg2_sequence *seq;
-	const struct v4l2_mpeg2_picture *pic;
+	const struct v4l2_ctrl_mpeg2_sequence *seq;
+	const struct v4l2_ctrl_mpeg2_picture *pic;
 	u32 reg;
 
 	src_buf = hantro_get_src_buf(ctx);
@@ -168,8 +168,10 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx)
 
 	slice_params = hantro_get_ctrl(ctx,
 				       V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS);
-	seq = &slice_params->sequence;
-	pic = &slice_params->picture;
+	seq = hantro_get_ctrl(ctx,
+			      V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE);
+	pic = hantro_get_ctrl(ctx,
+			      V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE);
 
 	reg = G1_REG_DEC_AXI_RD_ID(0) |
 	      G1_REG_DEC_TIMEOUT_E(1) |
diff --git a/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c b/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c
index 28eb77b0569b..45ab5ca32221 100644
--- a/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c
+++ b/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c
@@ -97,8 +97,8 @@ rk3399_vpu_mpeg2_dec_set_buffers(struct hantro_dev *vpu,
 				 struct hantro_ctx *ctx,
 				 struct vb2_buffer *src_buf,
 				 struct vb2_buffer *dst_buf,
-				 const struct v4l2_mpeg2_sequence *seq,
-				 const struct v4l2_mpeg2_picture *pic,
+				 const struct v4l2_ctrl_mpeg2_sequence *seq,
+				 const struct v4l2_ctrl_mpeg2_picture *pic,
 				 const struct v4l2_ctrl_mpeg2_slice_params *slice_params)
 {
 	dma_addr_t forward_addr = 0, backward_addr = 0;
@@ -158,8 +158,8 @@ void rk3399_vpu_mpeg2_dec_run(struct hantro_ctx *ctx)
 	struct hantro_dev *vpu = ctx->dev;
 	struct vb2_v4l2_buffer *src_buf, *dst_buf;
 	const struct v4l2_ctrl_mpeg2_slice_params *slice_params;
-	const struct v4l2_mpeg2_sequence *seq;
-	const struct v4l2_mpeg2_picture *pic;
+	const struct v4l2_ctrl_mpeg2_sequence *seq;
+	const struct v4l2_ctrl_mpeg2_picture *pic;
 	u32 reg;
 
 	src_buf = hantro_get_src_buf(ctx);
@@ -169,8 +169,10 @@ void rk3399_vpu_mpeg2_dec_run(struct hantro_ctx *ctx)
 
 	slice_params = hantro_get_ctrl(ctx,
 				       V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS);
-	seq = &slice_params->sequence;
-	pic = &slice_params->picture;
+	seq = hantro_get_ctrl(ctx,
+			      V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE);
+	pic = hantro_get_ctrl(ctx,
+			      V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE);
 
 	reg = VDPU_REG_DEC_ADV_PRE_DIS(0) |
 	      VDPU_REG_DEC_SCMD_DIS(0) |
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index 4e91c372d585..3f9fab6ddbaa 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -29,6 +29,20 @@
 #include "cedrus_hw.h"
 
 static const struct cedrus_control cedrus_controls[] = {
+	{
+		.cfg = {
+			.id	= V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE,
+		},
+		.codec		= CEDRUS_CODEC_MPEG2,
+		.required	= true,
+	},
+	{
+		.cfg = {
+			.id	= V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE,
+		},
+		.codec		= CEDRUS_CODEC_MPEG2,
+		.required	= true,
+	},
 	{
 		.cfg = {
 			.id	= V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS,
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
index fece505272b4..c286b7312386 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
@@ -68,6 +68,8 @@ struct cedrus_h264_run {
 };
 
 struct cedrus_mpeg2_run {
+	const struct v4l2_ctrl_mpeg2_sequence		*sequence;
+	const struct v4l2_ctrl_mpeg2_picture		*picture;
 	const struct v4l2_ctrl_mpeg2_slice_params	*slice_params;
 	const struct v4l2_ctrl_mpeg2_quantization	*quantization;
 };
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
index 6a99893cdc77..ea52778b36f9 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
@@ -75,8 +75,8 @@ static void cedrus_mpeg2_irq_disable(struct cedrus_ctx *ctx)
 static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 {
 	const struct v4l2_ctrl_mpeg2_slice_params *slice_params;
-	const struct v4l2_mpeg2_sequence *seq;
-	const struct v4l2_mpeg2_picture *pic;
+	const struct v4l2_ctrl_mpeg2_sequence *seq;
+	const struct v4l2_ctrl_mpeg2_picture *pic;
 	const struct v4l2_ctrl_mpeg2_quantization *quantization;
 	dma_addr_t src_buf_addr, dst_luma_addr, dst_chroma_addr;
 	dma_addr_t fwd_luma_addr, fwd_chroma_addr;
@@ -90,8 +90,8 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 	u32 reg;
 
 	slice_params = run->mpeg2.slice_params;
-	seq = &slice_params->sequence;
-	pic = &slice_params->picture;
+	seq = run->mpeg2.sequence;
+	pic = run->mpeg2.picture;
 
 	quantization = run->mpeg2.quantization;
 
diff --git a/include/media/mpeg2-ctrls.h b/include/media/mpeg2-ctrls.h
index 038206c7e6f5..edcbf67668ac 100644
--- a/include/media/mpeg2-ctrls.h
+++ b/include/media/mpeg2-ctrls.h
@@ -12,24 +12,46 @@
 #define _MPEG2_CTRLS_H_
 
 #define V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS		(V4L2_CID_CODEC_BASE+250)
-#define V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION		(V4L2_CID_CODEC_BASE+251)
+#define V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE		(V4L2_CID_CODEC_BASE+251)
+#define V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE		(V4L2_CID_CODEC_BASE+252)
+#define V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION		(V4L2_CID_CODEC_BASE+253)
 
 /* enum v4l2_ctrl_type type values */
-#define V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS 0x0103
-#define	V4L2_CTRL_TYPE_MPEG2_QUANTIZATION 0x0104
+#define V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS 0x0130
+#define V4L2_CTRL_TYPE_MPEG2_SEQUENCE 0x0131
+#define V4L2_CTRL_TYPE_MPEG2_PICTURE 0x0132
+#define V4L2_CTRL_TYPE_MPEG2_QUANTIZATION 0x0133
 
 #define V4L2_MPEG2_SEQ_FLAG_PROGRESSIVE		0x0001
 
-struct v4l2_mpeg2_sequence {
-	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Sequence header */
+/**
+ * struct v4l2_ctrl_mpeg2_sequence - MPEG-2 sequence header
+ *
+ * All the members on this structure match the sequence header and sequence
+ * extension syntaxes as specified by the MPEG-2 specification.
+ *
+ * Fields horizontal_size, vertical_size and vbv_buffer_size are a
+ * combination of respective _value and extension syntax elements,
+ * as described in section 6.3.3 "Sequence header".
+ *
+ * @horizontal_size: combination of elements horizontal_size_value and
+ * horizontal_size_extension.
+ * @vertical_size: combination of elements vertical_size_value and
+ * vertical_size_extension.
+ * @vbv_buffer_size: combination of elements vbv_buffer_size_value and
+ * vbv_buffer_size_extension.
+ * @profile_and_level_indication: see MPEG-2 specification.
+ * @chroma_format: see MPEG-2 specification.
+ * @reserved: padding field. Should be zeroed by applications.
+ * @flags: see V4L2_MPEG2_SEQ_FLAG_{}.
+ */
+struct v4l2_ctrl_mpeg2_sequence {
 	__u16	horizontal_size;
 	__u16	vertical_size;
 	__u32	vbv_buffer_size;
-
-	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Sequence extension */
 	__u16	profile_and_level_indication;
 	__u8	chroma_format;
-
+	__u8	reserved;
 	__u32	flags;
 };
 
@@ -55,30 +77,80 @@ struct v4l2_mpeg2_sequence {
 #define V4L2_MPEG2_PIC_FLAG_LOAD_CHROMA_INTRA		0x0400
 #define V4L2_MPEG2_PIC_FLAG_LOAD_CHROMA_NON_INTRA	0x0800
 
-struct v4l2_mpeg2_picture {
-	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Picture header */
+/**
+ * struct v4l2_ctrl_mpeg2_picture - MPEG-2 picture header
+ *
+ * All the members on this structure match the picture header and picture
+ * coding extension syntaxes as specified by the MPEG-2 specification.
+ *
+ * In particular, the set of quantization load flags V4L2_MPEG2_PIC_FLAG_LOAD_{}
+ * are specified here in order to allow applications to pass non-default
+ * quantization matrices. In this case, applications are expected to use
+ * V4L2_CTRL_TYPE_MPEG2_QUANTIZATION to pass the values of non-default
+ * matrices.
+ *
+ * @picture_coding_type: see MPEG-2 specification.
+ * @f_code[2][2]: see MPEG-2 specification.
+ * @intra_dc_precision: see MPEG-2 specification.
+ * @picture_structure: see V4L2_MPEG2_PIC_{}_FIELD.
+ * @reserved: padding field. Should be zeroed by applications.
+ * @flags: see V4L2_MPEG2_PIC_FLAG_{}.
+ */
+struct v4l2_ctrl_mpeg2_picture {
 	__u8	picture_coding_type;
-
-	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Picture coding extension */
 	__u8	f_code[2][2];
 	__u8	intra_dc_precision;
 	__u8	picture_structure;
-
+	__u8	reserved;
 	__u32	flags;
 };
 
+/**
+ * struct v4l2_ctrl_mpeg2_slice_params - MPEG-2 slice header
+ *
+ * @backward_ref_ts: timestamp of the V4L2 capture buffer to use as
+ * reference for backward prediction.
+ * @forward_ref_ts: timestamp of the V4L2 capture buffer to use as
+ * reference for forward prediction. These timestamp refers to the
+ * timestamp field in struct v4l2_buffer. Use v4l2_timeval_to_ns()
+ * to convert the struct timeval to a __u64.
+ * @quantiser_scale_code: quantiser scale integer matching an
+ * homonymous syntax element.
+ * @reserved: padding field. Should be zeroed by applications.
+ */
 struct v4l2_ctrl_mpeg2_slice_params {
 	__u64	backward_ref_ts;
 	__u64	forward_ref_ts;
-
-	struct v4l2_mpeg2_sequence sequence;
-	struct v4l2_mpeg2_picture picture;
-
-	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Slice */
 	__u32	quantiser_scale_code;
+	__u32	reserved;
 };
 
-/* ISO/IEC 13818-2, ITU-T Rec. H.262: Quant matrix extension */
+/**
+ * struct v4l2_ctrl_mpeg2_quantization - MPEG-2 quantization
+ *
+ * Quantization matrices as specified by section 6.3.7
+ * "Quant matrix extension".
+ *
+ * Applications are expected to set the quantization matrices load
+ * flags V4L2_MPEG2_PIC_FLAG_LOAD_{} in struct v4l2_ctrl_mpeg2_picture
+ * to tell the kernel that a non-default matrix shall be used
+ * to decode the picture.
+ *
+ * @intra_quantiser_matrix: The quantization matrix coefficients
+ * for intra-coded frames, in zigzag scanning order. It is relevant
+ * for both luma and chroma components, although it can be superseded
+ * by the chroma-specific matrix for non-4:2:0 YUV formats.
+ * @non_intra_quantiser_matrix: The quantization matrix coefficients
+ * for non-intra-coded frames, in zigzag scanning order. It is relevant
+ * for both luma and chroma components, although it can be superseded
+ * by the chroma-specific matrix for non-4:2:0 YUV formats.
+ * @chroma_intra_quantiser_matrix: The quantization matrix coefficients
+ * for the chominance component of intra-coded frames, in zigzag scanning
+ * order. Only relevant for 4:2:2 and 4:4:4 YUV formats.
+ * @chroma_non_intra_quantiser_matrix: The quantization matrix coefficients
+ * for the chrominance component of non-intra-coded frames, in zigzag scanning
+ * order. Only relevant for 4:2:2 and 4:4:4 YUV formats.
+ */
 struct v4l2_ctrl_mpeg2_quantization {
 	__u8	intra_quantiser_matrix[64];
 	__u8	non_intra_quantiser_matrix[64];
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index d25b38f78229..86f6c3ef7104 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -42,6 +42,8 @@ struct video_device;
  * @p_u16:			Pointer to a 16-bit unsigned value.
  * @p_u32:			Pointer to a 32-bit unsigned value.
  * @p_char:			Pointer to a string.
+ * @p_mpeg2_sequence:		Pointer to a MPEG2 sequence structure.
+ * @p_mpeg2_picture:		Pointer to a MPEG2 picture structure.
  * @p_mpeg2_slice_params:	Pointer to a MPEG2 slice parameters structure.
  * @p_mpeg2_quantization:	Pointer to a MPEG2 quantization data structure.
  * @p_fwht_params:		Pointer to a FWHT stateless parameters structure.
@@ -66,6 +68,8 @@ union v4l2_ctrl_ptr {
 	u16 *p_u16;
 	u32 *p_u32;
 	char *p_char;
+	struct v4l2_ctrl_mpeg2_sequence *p_sequence;
+	struct v4l2_ctrl_mpeg2_picture *p_picture;
 	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
 	struct v4l2_ctrl_mpeg2_quantization *p_mpeg2_quantization;
 	struct v4l2_ctrl_fwht_params *p_fwht_params;
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* Re: [PATCH v3 3/3] media: uapi: mpeg2: Split sequence and picture parameters
  2020-11-30 18:52 ` [PATCH v3 3/3] media: uapi: mpeg2: Split sequence and picture parameters Ezequiel Garcia
@ 2020-12-02 15:11   ` Hans Verkuil
  2020-12-02 21:46     ` Ezequiel Garcia
  0 siblings, 1 reply; 7+ messages in thread
From: Hans Verkuil @ 2020-12-02 15:11 UTC (permalink / raw)
  To: Ezequiel Garcia, linux-media, linux-kernel
  Cc: kernel, Jonas Karlman, Nicolas Dufresne, Philipp Zabel,
	Maxime Ripard, Paul Kocialkowski, Jernej Skrabec

Hi Ezequiel,

Some small comments:

On 30/11/2020 19:52, Ezequiel Garcia wrote:
> Typically, bitstreams are composed of one sequence header NAL unit,
> followed by a number of picture header and picture coding extension
> NAL units. Each picture can be composed by a number of slices.
> 
> Let's split the MPEG-2 uAPI to follow these semantics more closely,
> allowing more usage flexibility. Having these controls split
> allows applications to set a sequence control at the beginning
> of a sequence, and then set a picture control for each frame.
> 
> While here add padding fields where needed, and document
> the uAPI header thoroughly.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Tested-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  .../media/v4l/ext-ctrls-codec.rst             |  47 ++++++--
>  .../media/v4l/pixfmt-compressed.rst           |   5 +-
>  drivers/media/v4l2-core/v4l2-ctrls.c          |  57 +++++++--
>  drivers/staging/media/hantro/hantro_drv.c     |  10 ++
>  .../media/hantro/hantro_g1_mpeg2_dec.c        |  14 ++-
>  .../media/hantro/rk3399_vpu_hw_mpeg2_dec.c    |  14 ++-
>  drivers/staging/media/sunxi/cedrus/cedrus.c   |  14 +++
>  drivers/staging/media/sunxi/cedrus/cedrus.h   |   2 +
>  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c |   8 +-
>  include/media/mpeg2-ctrls.h                   | 110 +++++++++++++++---
>  include/media/v4l2-ctrls.h                    |   4 +
>  11 files changed, 224 insertions(+), 61 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 9a804f3037d8..a789866ebda4 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1526,14 +1526,6 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      :stub-columns: 0
>      :widths:       1 1 2
>  
> -    * - struct :c:type:`v4l2_mpeg2_sequence`
> -      - ``sequence``
> -      - Structure with MPEG-2 sequence metadata, merging relevant fields from
> -	the sequence header and sequence extension parts of the bitstream.
> -    * - struct :c:type:`v4l2_mpeg2_picture`
> -      - ``picture``
> -      - Structure with MPEG-2 picture metadata, merging relevant fields from
> -	the picture header and picture coding extension parts of the bitstream.
>      * - __u64
>        - ``backward_ref_ts``
>        - Timestamp of the V4L2 capture buffer to use as backward reference, used
> @@ -1551,14 +1543,28 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      * - __u32
>        - ``quantiser_scale_code``
>        - Code used to determine the quantization scale to use for the IDCT.
> +    * - __u8
> +      - ``reserved``
> +      - Applications and drivers must set this to zero.
>  
> -.. c:type:: v4l2_mpeg2_sequence
> +``V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE (struct)``
> +    Specifies the sequence parameters (as extracted from the bitstream) for the
> +    associated MPEG-2 slice data. This includes fields matching the syntax
> +    elements from the sequence header and sequence extension parts of the
> +    bitstream as specified by :ref:`mpeg2part2`.
> +
> +    .. note::
> +
> +       This compound control is not yet part of the public kernel API and
> +       it is expected to change.
> +
> +.. c:type:: v4l2_ctrl_mpeg2_sequence
>  
>  .. cssclass:: longtable
>  
>  .. tabularcolumns:: |p{1.5cm}|p{6.3cm}|p{9.4cm}|
>  
> -.. flat-table:: struct v4l2_mpeg2_sequence
> +.. flat-table:: struct v4l2_ctrl_mpeg2_sequence
>      :header-rows:  0
>      :stub-columns: 0
>      :widths:       1 1 2
> @@ -1580,6 +1586,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      * - __u8
>        - ``chroma_format``
>        - The chrominance sub-sampling format (1: 4:2:0, 2: 4:2:2, 3: 4:4:4).
> +    * - __u8
> +      - ``reserved``
> +      - Applications and drivers must set this to zero.
>      * - __u32
>        - ``flags``
>        - See :ref:`MPEG-2 Sequence Flags <mpeg2_sequence_flags>`.
> @@ -1600,13 +1609,24 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>        - Indication that all the frames for the sequence are progressive instead
>  	of interlaced.
>  
> -.. c:type:: v4l2_mpeg2_picture
> +``V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE (struct)``
> +    Specifies the picture parameters (as extracted from the bitstream) for the
> +    associated MPEG-2 slice data. This includes fields matching the syntax
> +    elements from the picture header and picture coding extension parts of the
> +    bitstream as specified by :ref:`mpeg2part2`.
> +
> +    .. note::
> +
> +       This compound control is not yet part of the public kernel API and
> +       it is expected to change.
> +
> +.. c:type:: v4l2_ctrl_mpeg2_picture
>  
>  .. cssclass:: longtable
>  
>  .. tabularcolumns:: |p{1.5cm}|p{6.3cm}|p{9.4cm}|
>  
> -.. flat-table:: struct v4l2_mpeg2_picture
> +.. flat-table:: struct v4l2_ctrl_mpeg2_picture
>      :header-rows:  0
>      :stub-columns: 0
>      :widths:       1 1 2
> @@ -1627,6 +1647,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>        - ``picture_structure``
>        - Picture structure (1: interlaced top field, 2: interlaced bottom field,
>  	3: progressive frame).
> +    * - __u8
> +      - ``reserved``
> +      - Applications and drivers must set this to zero.
>      * - __u32
>        - ``flags``
>        - See :ref:`MPEG-2 Picture Flags <mpeg2_picture_flags>`.
> diff --git a/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst b/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
> index b8899262d8de..0c22f3f99ce4 100644
> --- a/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
> +++ b/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
> @@ -108,8 +108,9 @@ Compressed Formats
>  	This format is adapted for stateless video decoders that implement a
>  	MPEG-2 pipeline (using the :ref:`mem2mem` and :ref:`media-request-api`).
>  	Metadata associated with the frame to decode is required to be passed
> -	through the ``V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS`` control and
> -	quantization matrices can optionally be specified through the
> +	through the ``V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE``,
> +        ``V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE``, and ``V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS``
> +        controls. Quantization matrices can optionally be specified through the
>  	``V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION`` control.
>  	See the :ref:`associated Codec Control IDs <v4l2-mpeg-mpeg2>`.
>  	Exactly one output and one capture buffer must be provided for use with
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index dd8f3bb4fbb9..a43baa9367ab 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -941,6 +941,8 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:		return "Vertical MV Search Range";
>  	case V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER:		return "Repeat Sequence Header";
>  	case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:		return "Force Key Frame";
> +	case V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE:		return "MPEG-2 Sequence Header";
> +	case V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE:			return "MPEG-2 Picture Header";
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS:		return "MPEG-2 Slice Parameters";
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION:		return "MPEG-2 Quantization Matrices";
>  	case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS:			return "FWHT Stateless Parameters";
> @@ -1427,6 +1429,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_RDS_TX_ALT_FREQS:
>  		*type = V4L2_CTRL_TYPE_U32;
>  		break;
> +	case V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE:
> +		*type = V4L2_CTRL_TYPE_MPEG2_SEQUENCE;
> +		break;
> +	case V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE:
> +		*type = V4L2_CTRL_TYPE_MPEG2_PICTURE;
> +		break;
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS:
>  		*type = V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS;
>  		break;
> @@ -1625,7 +1633,8 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
>  static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  			      union v4l2_ctrl_ptr ptr)
>  {
> -	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
> +	struct v4l2_ctrl_mpeg2_sequence *p_mpeg2_sequence;
> +	struct v4l2_ctrl_mpeg2_picture *p_mpeg2_picture;
>  	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
>  	void *p = ptr.p + idx * ctrl->elem_size;
>  
> @@ -1640,13 +1649,18 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  	 * v4l2_ctrl_type enum.
>  	 */
>  	switch ((u32)ctrl->type) {
> -	case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
> -		p_mpeg2_slice_params = p;
> +	case V4L2_CTRL_TYPE_MPEG2_SEQUENCE:
> +		p_mpeg2_sequence = p;
> +
>  		/* 4:2:0 */
> -		p_mpeg2_slice_params->sequence.chroma_format = 1;
> +		p_mpeg2_sequence->chroma_format = 1;
> +		break;
> +	case V4L2_CTRL_TYPE_MPEG2_PICTURE:
> +		p_mpeg2_picture = p;
> +
>  		/* interlaced top field */
> -		p_mpeg2_slice_params->picture.picture_structure = 1;
> -		p_mpeg2_slice_params->picture.picture_coding_type =
> +		p_mpeg2_picture->picture_structure = 1;
> +		p_mpeg2_picture->picture_coding_type =
>  					V4L2_MPEG2_PIC_CODING_TYPE_I;
>  		break;
>  	case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:

You should also add the MPEG2 types to std_log(). None of them are logged
there.

> @@ -1796,6 +1810,8 @@ static void std_log(const struct v4l2_ctrl *ctrl)
>  static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  				 union v4l2_ctrl_ptr ptr)
>  {
> +	struct v4l2_ctrl_mpeg2_sequence *p_mpeg2_sequence;
> +	struct v4l2_ctrl_mpeg2_picture *p_mpeg2_picture;
>  	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
>  	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
>  	struct v4l2_ctrl_h264_sps *p_h264_sps;
> @@ -1811,10 +1827,10 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  	unsigned int i;
>  
>  	switch ((u32)ctrl->type) {
> -	case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
> -		p_mpeg2_slice_params = p;
> +	case V4L2_CTRL_TYPE_MPEG2_SEQUENCE:
> +		p_mpeg2_sequence = p;
>  
> -		switch (p_mpeg2_slice_params->sequence.chroma_format) {
> +		switch (p_mpeg2_sequence->chroma_format) {
>  		case 1: /* 4:2:0 */
>  		case 2: /* 4:2:2 */
>  		case 3: /* 4:4:4 */
> @@ -1822,8 +1838,13 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  		default:
>  			return -EINVAL;
>  		}
> +		zero_reserved(*p_mpeg2_sequence);
> +		break;
> +
> +	case V4L2_CTRL_TYPE_MPEG2_PICTURE:
> +		p_mpeg2_picture = p;
>  
> -		switch (p_mpeg2_slice_params->picture.intra_dc_precision) {
> +		switch (p_mpeg2_picture->intra_dc_precision) {
>  		case 0: /* 8 bits */
>  		case 1: /* 9 bits */
>  		case 2: /* 10 bits */
> @@ -1833,7 +1854,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  			return -EINVAL;
>  		}
>  
> -		switch (p_mpeg2_slice_params->picture.picture_structure) {
> +		switch (p_mpeg2_picture->picture_structure) {
>  		case V4L2_MPEG2_PIC_TOP_FIELD:
>  		case V4L2_MPEG2_PIC_BOTTOM_FIELD:
>  		case V4L2_MPEG2_PIC_FRAME:
> @@ -1842,7 +1863,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  			return -EINVAL;
>  		}
>  
> -		switch (p_mpeg2_slice_params->picture.picture_coding_type) {
> +		switch (p_mpeg2_picture->picture_coding_type) {
>  		case V4L2_MPEG2_PIC_CODING_TYPE_I:
>  		case V4L2_MPEG2_PIC_CODING_TYPE_P:
>  		case V4L2_MPEG2_PIC_CODING_TYPE_B:
> @@ -1850,7 +1871,13 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  		default:
>  			return -EINVAL;
>  		}
> +		zero_reserved(*p_mpeg2_picture);
> +		break;
>  
> +	case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
> +		p_mpeg2_slice_params = p;
> +
> +		zero_reserved(*p_mpeg2_slice_params);
>  		break;
>  
>  	case V4L2_CTRL_TYPE_MPEG2_QUANTIZATION:
> @@ -2749,6 +2776,12 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  	case V4L2_CTRL_TYPE_U32:
>  		elem_size = sizeof(u32);
>  		break;
> +	case V4L2_CTRL_TYPE_MPEG2_SEQUENCE:
> +		elem_size = sizeof(struct v4l2_ctrl_mpeg2_sequence);
> +		break;
> +	case V4L2_CTRL_TYPE_MPEG2_PICTURE:
> +		elem_size = sizeof(struct v4l2_ctrl_mpeg2_picture);
> +		break;
>  	case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
>  		elem_size = sizeof(struct v4l2_ctrl_mpeg2_slice_params);
>  		break;
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index e5f200e64993..e589eccd5644 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -286,6 +286,16 @@ static const struct hantro_ctrl controls[] = {
>  			.def = 50,
>  			.ops = &hantro_jpeg_ctrl_ops,
>  		},
> +	}, {
> +		.codec = HANTRO_MPEG2_DECODER,
> +		.cfg = {
> +			.id = V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE,
> +		},
> +	}, {
> +		.codec = HANTRO_MPEG2_DECODER,
> +		.cfg = {
> +			.id = V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE,
> +		},
>  	}, {
>  		.codec = HANTRO_MPEG2_DECODER,
>  		.cfg = {
> diff --git a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
> index b6086474d31f..1a6ca49441f4 100644
> --- a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
> @@ -95,8 +95,8 @@ static void
>  hantro_g1_mpeg2_dec_set_buffers(struct hantro_dev *vpu, struct hantro_ctx *ctx,
>  				struct vb2_buffer *src_buf,
>  				struct vb2_buffer *dst_buf,
> -				const struct v4l2_mpeg2_sequence *seq,
> -				const struct v4l2_mpeg2_picture *pic,
> +				const struct v4l2_ctrl_mpeg2_sequence *seq,
> +				const struct v4l2_ctrl_mpeg2_picture *pic,
>  				const struct v4l2_ctrl_mpeg2_slice_params *slice_params)
>  {
>  	dma_addr_t forward_addr = 0, backward_addr = 0;
> @@ -156,8 +156,8 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx)
>  	struct hantro_dev *vpu = ctx->dev;
>  	struct vb2_v4l2_buffer *src_buf, *dst_buf;
>  	const struct v4l2_ctrl_mpeg2_slice_params *slice_params;
> -	const struct v4l2_mpeg2_sequence *seq;
> -	const struct v4l2_mpeg2_picture *pic;
> +	const struct v4l2_ctrl_mpeg2_sequence *seq;
> +	const struct v4l2_ctrl_mpeg2_picture *pic;
>  	u32 reg;
>  
>  	src_buf = hantro_get_src_buf(ctx);
> @@ -168,8 +168,10 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx)
>  
>  	slice_params = hantro_get_ctrl(ctx,
>  				       V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS);
> -	seq = &slice_params->sequence;
> -	pic = &slice_params->picture;
> +	seq = hantro_get_ctrl(ctx,
> +			      V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE);
> +	pic = hantro_get_ctrl(ctx,
> +			      V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE);
>  
>  	reg = G1_REG_DEC_AXI_RD_ID(0) |
>  	      G1_REG_DEC_TIMEOUT_E(1) |
> diff --git a/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c b/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c
> index 28eb77b0569b..45ab5ca32221 100644
> --- a/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c
> +++ b/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c
> @@ -97,8 +97,8 @@ rk3399_vpu_mpeg2_dec_set_buffers(struct hantro_dev *vpu,
>  				 struct hantro_ctx *ctx,
>  				 struct vb2_buffer *src_buf,
>  				 struct vb2_buffer *dst_buf,
> -				 const struct v4l2_mpeg2_sequence *seq,
> -				 const struct v4l2_mpeg2_picture *pic,
> +				 const struct v4l2_ctrl_mpeg2_sequence *seq,
> +				 const struct v4l2_ctrl_mpeg2_picture *pic,
>  				 const struct v4l2_ctrl_mpeg2_slice_params *slice_params)
>  {
>  	dma_addr_t forward_addr = 0, backward_addr = 0;
> @@ -158,8 +158,8 @@ void rk3399_vpu_mpeg2_dec_run(struct hantro_ctx *ctx)
>  	struct hantro_dev *vpu = ctx->dev;
>  	struct vb2_v4l2_buffer *src_buf, *dst_buf;
>  	const struct v4l2_ctrl_mpeg2_slice_params *slice_params;
> -	const struct v4l2_mpeg2_sequence *seq;
> -	const struct v4l2_mpeg2_picture *pic;
> +	const struct v4l2_ctrl_mpeg2_sequence *seq;
> +	const struct v4l2_ctrl_mpeg2_picture *pic;
>  	u32 reg;
>  
>  	src_buf = hantro_get_src_buf(ctx);
> @@ -169,8 +169,10 @@ void rk3399_vpu_mpeg2_dec_run(struct hantro_ctx *ctx)
>  
>  	slice_params = hantro_get_ctrl(ctx,
>  				       V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS);
> -	seq = &slice_params->sequence;
> -	pic = &slice_params->picture;
> +	seq = hantro_get_ctrl(ctx,
> +			      V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE);
> +	pic = hantro_get_ctrl(ctx,
> +			      V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE);
>  
>  	reg = VDPU_REG_DEC_ADV_PRE_DIS(0) |
>  	      VDPU_REG_DEC_SCMD_DIS(0) |
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
> index 4e91c372d585..3f9fab6ddbaa 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -29,6 +29,20 @@
>  #include "cedrus_hw.h"
>  
>  static const struct cedrus_control cedrus_controls[] = {
> +	{
> +		.cfg = {
> +			.id	= V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE,
> +		},
> +		.codec		= CEDRUS_CODEC_MPEG2,
> +		.required	= true,
> +	},
> +	{
> +		.cfg = {
> +			.id	= V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE,
> +		},
> +		.codec		= CEDRUS_CODEC_MPEG2,
> +		.required	= true,
> +	},
>  	{
>  		.cfg = {
>  			.id	= V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS,
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
> index fece505272b4..c286b7312386 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> @@ -68,6 +68,8 @@ struct cedrus_h264_run {
>  };
>  
>  struct cedrus_mpeg2_run {
> +	const struct v4l2_ctrl_mpeg2_sequence		*sequence;
> +	const struct v4l2_ctrl_mpeg2_picture		*picture;
>  	const struct v4l2_ctrl_mpeg2_slice_params	*slice_params;
>  	const struct v4l2_ctrl_mpeg2_quantization	*quantization;
>  };
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> index 6a99893cdc77..ea52778b36f9 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> @@ -75,8 +75,8 @@ static void cedrus_mpeg2_irq_disable(struct cedrus_ctx *ctx)
>  static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
>  {
>  	const struct v4l2_ctrl_mpeg2_slice_params *slice_params;
> -	const struct v4l2_mpeg2_sequence *seq;
> -	const struct v4l2_mpeg2_picture *pic;
> +	const struct v4l2_ctrl_mpeg2_sequence *seq;
> +	const struct v4l2_ctrl_mpeg2_picture *pic;
>  	const struct v4l2_ctrl_mpeg2_quantization *quantization;
>  	dma_addr_t src_buf_addr, dst_luma_addr, dst_chroma_addr;
>  	dma_addr_t fwd_luma_addr, fwd_chroma_addr;
> @@ -90,8 +90,8 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
>  	u32 reg;
>  
>  	slice_params = run->mpeg2.slice_params;
> -	seq = &slice_params->sequence;
> -	pic = &slice_params->picture;
> +	seq = run->mpeg2.sequence;
> +	pic = run->mpeg2.picture;
>  
>  	quantization = run->mpeg2.quantization;
>  
> diff --git a/include/media/mpeg2-ctrls.h b/include/media/mpeg2-ctrls.h
> index 038206c7e6f5..edcbf67668ac 100644
> --- a/include/media/mpeg2-ctrls.h
> +++ b/include/media/mpeg2-ctrls.h
> @@ -12,24 +12,46 @@
>  #define _MPEG2_CTRLS_H_
>  
>  #define V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS		(V4L2_CID_CODEC_BASE+250)
> -#define V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION		(V4L2_CID_CODEC_BASE+251)
> +#define V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE		(V4L2_CID_CODEC_BASE+251)
> +#define V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE		(V4L2_CID_CODEC_BASE+252)
> +#define V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION		(V4L2_CID_CODEC_BASE+253)
>  
>  /* enum v4l2_ctrl_type type values */
> -#define V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS 0x0103
> -#define	V4L2_CTRL_TYPE_MPEG2_QUANTIZATION 0x0104
> +#define V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS 0x0130
> +#define V4L2_CTRL_TYPE_MPEG2_SEQUENCE 0x0131
> +#define V4L2_CTRL_TYPE_MPEG2_PICTURE 0x0132
> +#define V4L2_CTRL_TYPE_MPEG2_QUANTIZATION 0x0133
>  
>  #define V4L2_MPEG2_SEQ_FLAG_PROGRESSIVE		0x0001
>  
> -struct v4l2_mpeg2_sequence {
> -	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Sequence header */
> +/**
> + * struct v4l2_ctrl_mpeg2_sequence - MPEG-2 sequence header
> + *
> + * All the members on this structure match the sequence header and sequence
> + * extension syntaxes as specified by the MPEG-2 specification.
> + *
> + * Fields horizontal_size, vertical_size and vbv_buffer_size are a
> + * combination of respective _value and extension syntax elements,

Is that a stray '_' or is it really '_value'? Just double-checking :-)

> + * as described in section 6.3.3 "Sequence header".
> + *
> + * @horizontal_size: combination of elements horizontal_size_value and
> + * horizontal_size_extension.
> + * @vertical_size: combination of elements vertical_size_value and
> + * vertical_size_extension.
> + * @vbv_buffer_size: combination of elements vbv_buffer_size_value and
> + * vbv_buffer_size_extension.
> + * @profile_and_level_indication: see MPEG-2 specification.
> + * @chroma_format: see MPEG-2 specification.
> + * @reserved: padding field. Should be zeroed by applications.
> + * @flags: see V4L2_MPEG2_SEQ_FLAG_{}.
> + */
> +struct v4l2_ctrl_mpeg2_sequence {
>  	__u16	horizontal_size;
>  	__u16	vertical_size;
>  	__u32	vbv_buffer_size;
> -
> -	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Sequence extension */
>  	__u16	profile_and_level_indication;
>  	__u8	chroma_format;
> -
> +	__u8	reserved;
>  	__u32	flags;
>  };
>  
> @@ -55,30 +77,80 @@ struct v4l2_mpeg2_sequence {
>  #define V4L2_MPEG2_PIC_FLAG_LOAD_CHROMA_INTRA		0x0400
>  #define V4L2_MPEG2_PIC_FLAG_LOAD_CHROMA_NON_INTRA	0x0800
>  
> -struct v4l2_mpeg2_picture {
> -	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Picture header */
> +/**
> + * struct v4l2_ctrl_mpeg2_picture - MPEG-2 picture header
> + *
> + * All the members on this structure match the picture header and picture
> + * coding extension syntaxes as specified by the MPEG-2 specification.
> + *
> + * In particular, the set of quantization load flags V4L2_MPEG2_PIC_FLAG_LOAD_{}
> + * are specified here in order to allow applications to pass non-default
> + * quantization matrices. In this case, applications are expected to use
> + * V4L2_CTRL_TYPE_MPEG2_QUANTIZATION to pass the values of non-default
> + * matrices.
> + *
> + * @picture_coding_type: see MPEG-2 specification.
> + * @f_code[2][2]: see MPEG-2 specification.
> + * @intra_dc_precision: see MPEG-2 specification.
> + * @picture_structure: see V4L2_MPEG2_PIC_{}_FIELD.
> + * @reserved: padding field. Should be zeroed by applications.
> + * @flags: see V4L2_MPEG2_PIC_FLAG_{}.
> + */
> +struct v4l2_ctrl_mpeg2_picture {
>  	__u8	picture_coding_type;
> -
> -	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Picture coding extension */
>  	__u8	f_code[2][2];
>  	__u8	intra_dc_precision;
>  	__u8	picture_structure;
> -
> +	__u8	reserved;
>  	__u32	flags;
>  };
>  
> +/**
> + * struct v4l2_ctrl_mpeg2_slice_params - MPEG-2 slice header
> + *
> + * @backward_ref_ts: timestamp of the V4L2 capture buffer to use as
> + * reference for backward prediction.
> + * @forward_ref_ts: timestamp of the V4L2 capture buffer to use as
> + * reference for forward prediction. These timestamp refers to the
> + * timestamp field in struct v4l2_buffer. Use v4l2_timeval_to_ns()
> + * to convert the struct timeval to a __u64.
> + * @quantiser_scale_code: quantiser scale integer matching an
> + * homonymous syntax element.
> + * @reserved: padding field. Should be zeroed by applications.
> + */
>  struct v4l2_ctrl_mpeg2_slice_params {
>  	__u64	backward_ref_ts;
>  	__u64	forward_ref_ts;
> -
> -	struct v4l2_mpeg2_sequence sequence;
> -	struct v4l2_mpeg2_picture picture;
> -
> -	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Slice */
>  	__u32	quantiser_scale_code;
> +	__u32	reserved;
>  };
>  
> -/* ISO/IEC 13818-2, ITU-T Rec. H.262: Quant matrix extension */
> +/**
> + * struct v4l2_ctrl_mpeg2_quantization - MPEG-2 quantization
> + *
> + * Quantization matrices as specified by section 6.3.7
> + * "Quant matrix extension".
> + *
> + * Applications are expected to set the quantization matrices load
> + * flags V4L2_MPEG2_PIC_FLAG_LOAD_{} in struct v4l2_ctrl_mpeg2_picture
> + * to tell the kernel that a non-default matrix shall be used
> + * to decode the picture.
> + *
> + * @intra_quantiser_matrix: The quantization matrix coefficients
> + * for intra-coded frames, in zigzag scanning order. It is relevant
> + * for both luma and chroma components, although it can be superseded
> + * by the chroma-specific matrix for non-4:2:0 YUV formats.
> + * @non_intra_quantiser_matrix: The quantization matrix coefficients
> + * for non-intra-coded frames, in zigzag scanning order. It is relevant
> + * for both luma and chroma components, although it can be superseded
> + * by the chroma-specific matrix for non-4:2:0 YUV formats.
> + * @chroma_intra_quantiser_matrix: The quantization matrix coefficients
> + * for the chominance component of intra-coded frames, in zigzag scanning
> + * order. Only relevant for 4:2:2 and 4:4:4 YUV formats.
> + * @chroma_non_intra_quantiser_matrix: The quantization matrix coefficients
> + * for the chrominance component of non-intra-coded frames, in zigzag scanning
> + * order. Only relevant for 4:2:2 and 4:4:4 YUV formats.
> + */
>  struct v4l2_ctrl_mpeg2_quantization {
>  	__u8	intra_quantiser_matrix[64];
>  	__u8	non_intra_quantiser_matrix[64];
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index d25b38f78229..86f6c3ef7104 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -42,6 +42,8 @@ struct video_device;
>   * @p_u16:			Pointer to a 16-bit unsigned value.
>   * @p_u32:			Pointer to a 32-bit unsigned value.
>   * @p_char:			Pointer to a string.
> + * @p_mpeg2_sequence:		Pointer to a MPEG2 sequence structure.
> + * @p_mpeg2_picture:		Pointer to a MPEG2 picture structure.
>   * @p_mpeg2_slice_params:	Pointer to a MPEG2 slice parameters structure.
>   * @p_mpeg2_quantization:	Pointer to a MPEG2 quantization data structure.
>   * @p_fwht_params:		Pointer to a FWHT stateless parameters structure.
> @@ -66,6 +68,8 @@ union v4l2_ctrl_ptr {
>  	u16 *p_u16;
>  	u32 *p_u32;
>  	char *p_char;
> +	struct v4l2_ctrl_mpeg2_sequence *p_sequence;
> +	struct v4l2_ctrl_mpeg2_picture *p_picture;

Should start with p_mpeg2_ for both pointers.

>  	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
>  	struct v4l2_ctrl_mpeg2_quantization *p_mpeg2_quantization;
>  	struct v4l2_ctrl_fwht_params *p_fwht_params;
> 

Regards,

	Hans

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH v3 3/3] media: uapi: mpeg2: Split sequence and picture parameters
  2020-12-02 15:11   ` Hans Verkuil
@ 2020-12-02 21:46     ` Ezequiel Garcia
  2020-12-03  8:51       ` Hans Verkuil
  0 siblings, 1 reply; 7+ messages in thread
From: Ezequiel Garcia @ 2020-12-02 21:46 UTC (permalink / raw)
  To: Hans Verkuil, linux-media, linux-kernel
  Cc: kernel, Jonas Karlman, Nicolas Dufresne, Philipp Zabel,
	Maxime Ripard, Paul Kocialkowski, Jernej Skrabec

On Wed, 2020-12-02 at 16:11 +0100, Hans Verkuil wrote:
> Hi Ezequiel,
> 
> Some small comments:
> 
> On 30/11/2020 19:52, Ezequiel Garcia wrote:
> > Typically, bitstreams are composed of one sequence header NAL unit,
> > followed by a number of picture header and picture coding extension
> > NAL units. Each picture can be composed by a number of slices.
> > 
> > Let's split the MPEG-2 uAPI to follow these semantics more closely,
> > allowing more usage flexibility. Having these controls split
> > allows applications to set a sequence control at the beginning
> > of a sequence, and then set a picture control for each frame.
> > 
> > While here add padding fields where needed, and document
> > the uAPI header thoroughly.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > Tested-by: Jonas Karlman <jonas@kwiboo.se>
> > ---
> >  .../media/v4l/ext-ctrls-codec.rst             |  47 ++++++--
> >  .../media/v4l/pixfmt-compressed.rst           |   5 +-
> >  drivers/media/v4l2-core/v4l2-ctrls.c          |  57 +++++++--
> >  drivers/staging/media/hantro/hantro_drv.c     |  10 ++
> >  .../media/hantro/hantro_g1_mpeg2_dec.c        |  14 ++-
> >  .../media/hantro/rk3399_vpu_hw_mpeg2_dec.c    |  14 ++-
> >  drivers/staging/media/sunxi/cedrus/cedrus.c   |  14 +++
> >  drivers/staging/media/sunxi/cedrus/cedrus.h   |   2 +
> >  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c |   8 +-
> >  include/media/mpeg2-ctrls.h                   | 110 +++++++++++++++---
> >  include/media/v4l2-ctrls.h                    |   4 +
> >  11 files changed, 224 insertions(+), 61 deletions(-)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > index 9a804f3037d8..a789866ebda4 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > @@ -1526,14 +1526,6 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >      :stub-columns: 0
> >      :widths:       1 1 2
> >  
> > -    * - struct :c:type:`v4l2_mpeg2_sequence`
> > -      - ``sequence``
> > -      - Structure with MPEG-2 sequence metadata, merging relevant fields from
> > -	the sequence header and sequence extension parts of the bitstream.
> > -    * - struct :c:type:`v4l2_mpeg2_picture`
> > -      - ``picture``
> > -      - Structure with MPEG-2 picture metadata, merging relevant fields from
> > -	the picture header and picture coding extension parts of the bitstream.
> >      * - __u64
> >        - ``backward_ref_ts``
> >        - Timestamp of the V4L2 capture buffer to use as backward reference, used
> > @@ -1551,14 +1543,28 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >      * - __u32
> >        - ``quantiser_scale_code``
> >        - Code used to determine the quantization scale to use for the IDCT.
> > +    * - __u8
> > +      - ``reserved``
> > +      - Applications and drivers must set this to zero.
> >  
> > -.. c:type:: v4l2_mpeg2_sequence
> > +``V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE (struct)``
> > +    Specifies the sequence parameters (as extracted from the bitstream) for the
> > +    associated MPEG-2 slice data. This includes fields matching the syntax
> > +    elements from the sequence header and sequence extension parts of the
> > +    bitstream as specified by :ref:`mpeg2part2`.
> > +
> > +    .. note::
> > +
> > +       This compound control is not yet part of the public kernel API and
> > +       it is expected to change.
> > +
> > +.. c:type:: v4l2_ctrl_mpeg2_sequence
> >  
> >  .. cssclass:: longtable
> >  
> >  .. tabularcolumns:: |p{1.5cm}|p{6.3cm}|p{9.4cm}|
> >  
> > -.. flat-table:: struct v4l2_mpeg2_sequence
> > +.. flat-table:: struct v4l2_ctrl_mpeg2_sequence
> >      :header-rows:  0
> >      :stub-columns: 0
> >      :widths:       1 1 2
> > @@ -1580,6 +1586,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >      * - __u8
> >        - ``chroma_format``
> >        - The chrominance sub-sampling format (1: 4:2:0, 2: 4:2:2, 3: 4:4:4).
> > +    * - __u8
> > +      - ``reserved``
> > +      - Applications and drivers must set this to zero.
> >      * - __u32
> >        - ``flags``
> >        - See :ref:`MPEG-2 Sequence Flags <mpeg2_sequence_flags>`.
> > @@ -1600,13 +1609,24 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >        - Indication that all the frames for the sequence are progressive instead
> >  	of interlaced.
> >  
> > -.. c:type:: v4l2_mpeg2_picture
> > +``V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE (struct)``
> > +    Specifies the picture parameters (as extracted from the bitstream) for the
> > +    associated MPEG-2 slice data. This includes fields matching the syntax
> > +    elements from the picture header and picture coding extension parts of the
> > +    bitstream as specified by :ref:`mpeg2part2`.
> > +
> > +    .. note::
> > +
> > +       This compound control is not yet part of the public kernel API and
> > +       it is expected to change.
> > +
> > +.. c:type:: v4l2_ctrl_mpeg2_picture
> >  
> >  .. cssclass:: longtable
> >  
> >  .. tabularcolumns:: |p{1.5cm}|p{6.3cm}|p{9.4cm}|
> >  
> > -.. flat-table:: struct v4l2_mpeg2_picture
> > +.. flat-table:: struct v4l2_ctrl_mpeg2_picture
> >      :header-rows:  0
> >      :stub-columns: 0
> >      :widths:       1 1 2
> > @@ -1627,6 +1647,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >        - ``picture_structure``
> >        - Picture structure (1: interlaced top field, 2: interlaced bottom field,
> >  	3: progressive frame).
> > +    * - __u8
> > +      - ``reserved``
> > +      - Applications and drivers must set this to zero.
> >      * - __u32
> >        - ``flags``
> >        - See :ref:`MPEG-2 Picture Flags <mpeg2_picture_flags>`.
> > diff --git a/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst b/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
> > index b8899262d8de..0c22f3f99ce4 100644
> > --- a/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
> > +++ b/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
> > @@ -108,8 +108,9 @@ Compressed Formats
> >  	This format is adapted for stateless video decoders that implement a
> >  	MPEG-2 pipeline (using the :ref:`mem2mem` and :ref:`media-request-api`).
> >  	Metadata associated with the frame to decode is required to be passed
> > -	through the ``V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS`` control and
> > -	quantization matrices can optionally be specified through the
> > +	through the ``V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE``,
> > +        ``V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE``, and ``V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS``
> > +        controls. Quantization matrices can optionally be specified through the
> >  	``V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION`` control.
> >  	See the :ref:`associated Codec Control IDs <v4l2-mpeg-mpeg2>`.
> >  	Exactly one output and one capture buffer must be provided for use with
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index dd8f3bb4fbb9..a43baa9367ab 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -941,6 +941,8 @@ const char *v4l2_ctrl_get_name(u32 id)
> >  	case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:		return "Vertical MV Search Range";
> >  	case V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER:		return "Repeat Sequence Header";
> >  	case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:		return "Force Key Frame";
> > +	case V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE:		return "MPEG-2 Sequence Header";
> > +	case V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE:			return "MPEG-2 Picture Header";
> >  	case V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS:		return "MPEG-2 Slice Parameters";
> >  	case V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION:		return "MPEG-2 Quantization Matrices";
> >  	case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS:			return "FWHT Stateless Parameters";
> > @@ -1427,6 +1429,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >  	case V4L2_CID_RDS_TX_ALT_FREQS:
> >  		*type = V4L2_CTRL_TYPE_U32;
> >  		break;
> > +	case V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE:
> > +		*type = V4L2_CTRL_TYPE_MPEG2_SEQUENCE;
> > +		break;
> > +	case V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE:
> > +		*type = V4L2_CTRL_TYPE_MPEG2_PICTURE;
> > +		break;
> >  	case V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS:
> >  		*type = V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS;
> >  		break;
> > @@ -1625,7 +1633,8 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
> >  static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >  			      union v4l2_ctrl_ptr ptr)
> >  {
> > -	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
> > +	struct v4l2_ctrl_mpeg2_sequence *p_mpeg2_sequence;
> > +	struct v4l2_ctrl_mpeg2_picture *p_mpeg2_picture;
> >  	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
> >  	void *p = ptr.p + idx * ctrl->elem_size;
> >  
> > @@ -1640,13 +1649,18 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >  	 * v4l2_ctrl_type enum.
> >  	 */
> >  	switch ((u32)ctrl->type) {
> > -	case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
> > -		p_mpeg2_slice_params = p;
> > +	case V4L2_CTRL_TYPE_MPEG2_SEQUENCE:
> > +		p_mpeg2_sequence = p;
> > +
> >  		/* 4:2:0 */
> > -		p_mpeg2_slice_params->sequence.chroma_format = 1;
> > +		p_mpeg2_sequence->chroma_format = 1;
> > +		break;
> > +	case V4L2_CTRL_TYPE_MPEG2_PICTURE:
> > +		p_mpeg2_picture = p;
> > +
> >  		/* interlaced top field */
> > -		p_mpeg2_slice_params->picture.picture_structure = 1;
> > -		p_mpeg2_slice_params->picture.picture_coding_type =
> > +		p_mpeg2_picture->picture_structure = 1;
> > +		p_mpeg2_picture->picture_coding_type =
> >  					V4L2_MPEG2_PIC_CODING_TYPE_I;
> >  		break;
> >  	case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
> 
> You should also add the MPEG2 types to std_log(). None of them are logged
> there.
> 

I was reserving that stuff for the destaging effort.

Perhaps we are comfortable with destaging on the next iteration?

> > @@ -1796,6 +1810,8 @@ static void std_log(const struct v4l2_ctrl *ctrl)
> >  static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >  				 union v4l2_ctrl_ptr ptr)
> >  {
> > +	struct v4l2_ctrl_mpeg2_sequence *p_mpeg2_sequence;
> > +	struct v4l2_ctrl_mpeg2_picture *p_mpeg2_picture;
> >  	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
> >  	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
> >  	struct v4l2_ctrl_h264_sps *p_h264_sps;
> > @@ -1811,10 +1827,10 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >  	unsigned int i;
> >  
> >  	switch ((u32)ctrl->type) {
> > -	case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
> > -		p_mpeg2_slice_params = p;
> > +	case V4L2_CTRL_TYPE_MPEG2_SEQUENCE:
> > +		p_mpeg2_sequence = p;
> >  
> > -		switch (p_mpeg2_slice_params->sequence.chroma_format) {
> > +		switch (p_mpeg2_sequence->chroma_format) {
> >  		case 1: /* 4:2:0 */
> >  		case 2: /* 4:2:2 */
> >  		case 3: /* 4:4:4 */
> > @@ -1822,8 +1838,13 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >  		default:
> >  			return -EINVAL;
> >  		}
> > +		zero_reserved(*p_mpeg2_sequence);
> > +		break;
> > +
> > +	case V4L2_CTRL_TYPE_MPEG2_PICTURE:
> > +		p_mpeg2_picture = p;
> >  
> > -		switch (p_mpeg2_slice_params->picture.intra_dc_precision) {
> > +		switch (p_mpeg2_picture->intra_dc_precision) {
> >  		case 0: /* 8 bits */
> >  		case 1: /* 9 bits */
> >  		case 2: /* 10 bits */
> > @@ -1833,7 +1854,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >  			return -EINVAL;
> >  		}
> >  
> > -		switch (p_mpeg2_slice_params->picture.picture_structure) {
> > +		switch (p_mpeg2_picture->picture_structure) {
> >  		case V4L2_MPEG2_PIC_TOP_FIELD:
> >  		case V4L2_MPEG2_PIC_BOTTOM_FIELD:
> >  		case V4L2_MPEG2_PIC_FRAME:
> > @@ -1842,7 +1863,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >  			return -EINVAL;
> >  		}
> >  
> > -		switch (p_mpeg2_slice_params->picture.picture_coding_type) {
> > +		switch (p_mpeg2_picture->picture_coding_type) {
> >  		case V4L2_MPEG2_PIC_CODING_TYPE_I:
> >  		case V4L2_MPEG2_PIC_CODING_TYPE_P:
> >  		case V4L2_MPEG2_PIC_CODING_TYPE_B:
> > @@ -1850,7 +1871,13 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >  		default:
> >  			return -EINVAL;
> >  		}
> > +		zero_reserved(*p_mpeg2_picture);
> > +		break;
> >  
> > +	case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
> > +		p_mpeg2_slice_params = p;
> > +
> > +		zero_reserved(*p_mpeg2_slice_params);
> >  		break;
> >  
> >  	case V4L2_CTRL_TYPE_MPEG2_QUANTIZATION:
> > @@ -2749,6 +2776,12 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> >  	case V4L2_CTRL_TYPE_U32:
> >  		elem_size = sizeof(u32);
> >  		break;
> > +	case V4L2_CTRL_TYPE_MPEG2_SEQUENCE:
> > +		elem_size = sizeof(struct v4l2_ctrl_mpeg2_sequence);
> > +		break;
> > +	case V4L2_CTRL_TYPE_MPEG2_PICTURE:
> > +		elem_size = sizeof(struct v4l2_ctrl_mpeg2_picture);
> > +		break;
> >  	case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
> >  		elem_size = sizeof(struct v4l2_ctrl_mpeg2_slice_params);
> >  		break;
> > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> > index e5f200e64993..e589eccd5644 100644
> > --- a/drivers/staging/media/hantro/hantro_drv.c
> > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > @@ -286,6 +286,16 @@ static const struct hantro_ctrl controls[] = {
> >  			.def = 50,
> >  			.ops = &hantro_jpeg_ctrl_ops,
> >  		},
> > +	}, {
> > +		.codec = HANTRO_MPEG2_DECODER,
> > +		.cfg = {
> > +			.id = V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE,
> > +		},
> > +	}, {
> > +		.codec = HANTRO_MPEG2_DECODER,
> > +		.cfg = {
> > +			.id = V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE,
> > +		},
> >  	}, {
> >  		.codec = HANTRO_MPEG2_DECODER,
> >  		.cfg = {
> > diff --git a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
> > index b6086474d31f..1a6ca49441f4 100644
> > --- a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
> > +++ b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
> > @@ -95,8 +95,8 @@ static void
> >  hantro_g1_mpeg2_dec_set_buffers(struct hantro_dev *vpu, struct hantro_ctx *ctx,
> >  				struct vb2_buffer *src_buf,
> >  				struct vb2_buffer *dst_buf,
> > -				const struct v4l2_mpeg2_sequence *seq,
> > -				const struct v4l2_mpeg2_picture *pic,
> > +				const struct v4l2_ctrl_mpeg2_sequence *seq,
> > +				const struct v4l2_ctrl_mpeg2_picture *pic,
> >  				const struct v4l2_ctrl_mpeg2_slice_params *slice_params)
> >  {
> >  	dma_addr_t forward_addr = 0, backward_addr = 0;
> > @@ -156,8 +156,8 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx)
> >  	struct hantro_dev *vpu = ctx->dev;
> >  	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> >  	const struct v4l2_ctrl_mpeg2_slice_params *slice_params;
> > -	const struct v4l2_mpeg2_sequence *seq;
> > -	const struct v4l2_mpeg2_picture *pic;
> > +	const struct v4l2_ctrl_mpeg2_sequence *seq;
> > +	const struct v4l2_ctrl_mpeg2_picture *pic;
> >  	u32 reg;
> >  
> >  	src_buf = hantro_get_src_buf(ctx);
> > @@ -168,8 +168,10 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx)
> >  
> >  	slice_params = hantro_get_ctrl(ctx,
> >  				       V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS);
> > -	seq = &slice_params->sequence;
> > -	pic = &slice_params->picture;
> > +	seq = hantro_get_ctrl(ctx,
> > +			      V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE);
> > +	pic = hantro_get_ctrl(ctx,
> > +			      V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE);
> >  
> >  	reg = G1_REG_DEC_AXI_RD_ID(0) |
> >  	      G1_REG_DEC_TIMEOUT_E(1) |
> > diff --git a/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c b/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c
> > index 28eb77b0569b..45ab5ca32221 100644
> > --- a/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c
> > +++ b/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c
> > @@ -97,8 +97,8 @@ rk3399_vpu_mpeg2_dec_set_buffers(struct hantro_dev *vpu,
> >  				 struct hantro_ctx *ctx,
> >  				 struct vb2_buffer *src_buf,
> >  				 struct vb2_buffer *dst_buf,
> > -				 const struct v4l2_mpeg2_sequence *seq,
> > -				 const struct v4l2_mpeg2_picture *pic,
> > +				 const struct v4l2_ctrl_mpeg2_sequence *seq,
> > +				 const struct v4l2_ctrl_mpeg2_picture *pic,
> >  				 const struct v4l2_ctrl_mpeg2_slice_params *slice_params)
> >  {
> >  	dma_addr_t forward_addr = 0, backward_addr = 0;
> > @@ -158,8 +158,8 @@ void rk3399_vpu_mpeg2_dec_run(struct hantro_ctx *ctx)
> >  	struct hantro_dev *vpu = ctx->dev;
> >  	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> >  	const struct v4l2_ctrl_mpeg2_slice_params *slice_params;
> > -	const struct v4l2_mpeg2_sequence *seq;
> > -	const struct v4l2_mpeg2_picture *pic;
> > +	const struct v4l2_ctrl_mpeg2_sequence *seq;
> > +	const struct v4l2_ctrl_mpeg2_picture *pic;
> >  	u32 reg;
> >  
> >  	src_buf = hantro_get_src_buf(ctx);
> > @@ -169,8 +169,10 @@ void rk3399_vpu_mpeg2_dec_run(struct hantro_ctx *ctx)
> >  
> >  	slice_params = hantro_get_ctrl(ctx,
> >  				       V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS);
> > -	seq = &slice_params->sequence;
> > -	pic = &slice_params->picture;
> > +	seq = hantro_get_ctrl(ctx,
> > +			      V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE);
> > +	pic = hantro_get_ctrl(ctx,
> > +			      V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE);
> >  
> >  	reg = VDPU_REG_DEC_ADV_PRE_DIS(0) |
> >  	      VDPU_REG_DEC_SCMD_DIS(0) |
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
> > index 4e91c372d585..3f9fab6ddbaa 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> > @@ -29,6 +29,20 @@
> >  #include "cedrus_hw.h"
> >  
> >  static const struct cedrus_control cedrus_controls[] = {
> > +	{
> > +		.cfg = {
> > +			.id	= V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE,
> > +		},
> > +		.codec		= CEDRUS_CODEC_MPEG2,
> > +		.required	= true,
> > +	},
> > +	{
> > +		.cfg = {
> > +			.id	= V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE,
> > +		},
> > +		.codec		= CEDRUS_CODEC_MPEG2,
> > +		.required	= true,
> > +	},
> >  	{
> >  		.cfg = {
> >  			.id	= V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS,
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
> > index fece505272b4..c286b7312386 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> > @@ -68,6 +68,8 @@ struct cedrus_h264_run {
> >  };
> >  
> >  struct cedrus_mpeg2_run {
> > +	const struct v4l2_ctrl_mpeg2_sequence		*sequence;
> > +	const struct v4l2_ctrl_mpeg2_picture		*picture;
> >  	const struct v4l2_ctrl_mpeg2_slice_params	*slice_params;
> >  	const struct v4l2_ctrl_mpeg2_quantization	*quantization;
> >  };
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > index 6a99893cdc77..ea52778b36f9 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > @@ -75,8 +75,8 @@ static void cedrus_mpeg2_irq_disable(struct cedrus_ctx *ctx)
> >  static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
> >  {
> >  	const struct v4l2_ctrl_mpeg2_slice_params *slice_params;
> > -	const struct v4l2_mpeg2_sequence *seq;
> > -	const struct v4l2_mpeg2_picture *pic;
> > +	const struct v4l2_ctrl_mpeg2_sequence *seq;
> > +	const struct v4l2_ctrl_mpeg2_picture *pic;
> >  	const struct v4l2_ctrl_mpeg2_quantization *quantization;
> >  	dma_addr_t src_buf_addr, dst_luma_addr, dst_chroma_addr;
> >  	dma_addr_t fwd_luma_addr, fwd_chroma_addr;
> > @@ -90,8 +90,8 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
> >  	u32 reg;
> >  
> >  	slice_params = run->mpeg2.slice_params;
> > -	seq = &slice_params->sequence;
> > -	pic = &slice_params->picture;
> > +	seq = run->mpeg2.sequence;
> > +	pic = run->mpeg2.picture;
> >  
> >  	quantization = run->mpeg2.quantization;
> >  
> > diff --git a/include/media/mpeg2-ctrls.h b/include/media/mpeg2-ctrls.h
> > index 038206c7e6f5..edcbf67668ac 100644
> > --- a/include/media/mpeg2-ctrls.h
> > +++ b/include/media/mpeg2-ctrls.h
> > @@ -12,24 +12,46 @@
> >  #define _MPEG2_CTRLS_H_
> >  
> >  #define V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS		(V4L2_CID_CODEC_BASE+250)
> > -#define V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION		(V4L2_CID_CODEC_BASE+251)
> > +#define V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE		(V4L2_CID_CODEC_BASE+251)
> > +#define V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE		(V4L2_CID_CODEC_BASE+252)
> > +#define V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION		(V4L2_CID_CODEC_BASE+253)
> >  
> >  /* enum v4l2_ctrl_type type values */
> > -#define V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS 0x0103
> > -#define	V4L2_CTRL_TYPE_MPEG2_QUANTIZATION 0x0104
> > +#define V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS 0x0130
> > +#define V4L2_CTRL_TYPE_MPEG2_SEQUENCE 0x0131
> > +#define V4L2_CTRL_TYPE_MPEG2_PICTURE 0x0132
> > +#define V4L2_CTRL_TYPE_MPEG2_QUANTIZATION 0x0133
> >  
> >  #define V4L2_MPEG2_SEQ_FLAG_PROGRESSIVE		0x0001
> >  
> > -struct v4l2_mpeg2_sequence {
> > -	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Sequence header */
> > +/**
> > + * struct v4l2_ctrl_mpeg2_sequence - MPEG-2 sequence header
> > + *
> > + * All the members on this structure match the sequence header and sequence
> > + * extension syntaxes as specified by the MPEG-2 specification.
> > + *
> > + * Fields horizontal_size, vertical_size and vbv_buffer_size are a
> > + * combination of respective _value and extension syntax elements,
> 
> Is that a stray '_' or is it really '_value'? Just double-checking :-)
> 

Probably it's a typo. BTW, I wonder if that sentence is clear enough.
What I wanted to clarify is that this horizontal_size field is
a combination of syntax elements horizontal_size_value
and horizontal_size_extension syntax elements.

(and same for vertical_size)

> > + * as described in section 6.3.3 "Sequence header".
> > + *
> > + * @horizontal_size: combination of elements horizontal_size_value and
> > + * horizontal_size_extension.
> > + * @vertical_size: combination of elements vertical_size_value and
> > + * vertical_size_extension.
> > + * @vbv_buffer_size: combination of elements vbv_buffer_size_value and
> > + * vbv_buffer_size_extension.
> > + * @profile_and_level_indication: see MPEG-2 specification.
> > + * @chroma_format: see MPEG-2 specification.
> > + * @reserved: padding field. Should be zeroed by applications.
> > + * @flags: see V4L2_MPEG2_SEQ_FLAG_{}.
> > + */
> > +struct v4l2_ctrl_mpeg2_sequence {
> >  	__u16	horizontal_size;
> >  	__u16	vertical_size;
> >  	__u32	vbv_buffer_size;
> > -
> > -	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Sequence extension */
> >  	__u16	profile_and_level_indication;
> >  	__u8	chroma_format;
> > -
> > +	__u8	reserved;
> >  	__u32	flags;
> >  };
> >  
> > @@ -55,30 +77,80 @@ struct v4l2_mpeg2_sequence {
> >  #define V4L2_MPEG2_PIC_FLAG_LOAD_CHROMA_INTRA		0x0400
> >  #define V4L2_MPEG2_PIC_FLAG_LOAD_CHROMA_NON_INTRA	0x0800
> >  
> > -struct v4l2_mpeg2_picture {
> > -	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Picture header */
> > +/**
> > + * struct v4l2_ctrl_mpeg2_picture - MPEG-2 picture header
> > + *
> > + * All the members on this structure match the picture header and picture
> > + * coding extension syntaxes as specified by the MPEG-2 specification.
> > + *
> > + * In particular, the set of quantization load flags V4L2_MPEG2_PIC_FLAG_LOAD_{}
> > + * are specified here in order to allow applications to pass non-default
> > + * quantization matrices. In this case, applications are expected to use
> > + * V4L2_CTRL_TYPE_MPEG2_QUANTIZATION to pass the values of non-default
> > + * matrices.
> > + *
> > + * @picture_coding_type: see MPEG-2 specification.
> > + * @f_code[2][2]: see MPEG-2 specification.
> > + * @intra_dc_precision: see MPEG-2 specification.
> > + * @picture_structure: see V4L2_MPEG2_PIC_{}_FIELD.
> > + * @reserved: padding field. Should be zeroed by applications.
> > + * @flags: see V4L2_MPEG2_PIC_FLAG_{}.
> > + */
> > +struct v4l2_ctrl_mpeg2_picture {
> >  	__u8	picture_coding_type;
> > -
> > -	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Picture coding extension */
> >  	__u8	f_code[2][2];
> >  	__u8	intra_dc_precision;
> >  	__u8	picture_structure;
> > -
> > +	__u8	reserved;
> >  	__u32	flags;
> >  };
> >  
> > +/**
> > + * struct v4l2_ctrl_mpeg2_slice_params - MPEG-2 slice header
> > + *
> > + * @backward_ref_ts: timestamp of the V4L2 capture buffer to use as
> > + * reference for backward prediction.
> > + * @forward_ref_ts: timestamp of the V4L2 capture buffer to use as
> > + * reference for forward prediction. These timestamp refers to the
> > + * timestamp field in struct v4l2_buffer. Use v4l2_timeval_to_ns()
> > + * to convert the struct timeval to a __u64.
> > + * @quantiser_scale_code: quantiser scale integer matching an
> > + * homonymous syntax element.
> > + * @reserved: padding field. Should be zeroed by applications.
> > + */
> >  struct v4l2_ctrl_mpeg2_slice_params {
> >  	__u64	backward_ref_ts;
> >  	__u64	forward_ref_ts;
> > -
> > -	struct v4l2_mpeg2_sequence sequence;
> > -	struct v4l2_mpeg2_picture picture;
> > -
> > -	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Slice */
> >  	__u32	quantiser_scale_code;
> > +	__u32	reserved;
> >  };
> >  
> > -/* ISO/IEC 13818-2, ITU-T Rec. H.262: Quant matrix extension */
> > +/**
> > + * struct v4l2_ctrl_mpeg2_quantization - MPEG-2 quantization
> > + *
> > + * Quantization matrices as specified by section 6.3.7
> > + * "Quant matrix extension".
> > + *
> > + * Applications are expected to set the quantization matrices load
> > + * flags V4L2_MPEG2_PIC_FLAG_LOAD_{} in struct v4l2_ctrl_mpeg2_picture
> > + * to tell the kernel that a non-default matrix shall be used
> > + * to decode the picture.
> > + *
> > + * @intra_quantiser_matrix: The quantization matrix coefficients
> > + * for intra-coded frames, in zigzag scanning order. It is relevant
> > + * for both luma and chroma components, although it can be superseded
> > + * by the chroma-specific matrix for non-4:2:0 YUV formats.
> > + * @non_intra_quantiser_matrix: The quantization matrix coefficients
> > + * for non-intra-coded frames, in zigzag scanning order. It is relevant
> > + * for both luma and chroma components, although it can be superseded
> > + * by the chroma-specific matrix for non-4:2:0 YUV formats.
> > + * @chroma_intra_quantiser_matrix: The quantization matrix coefficients
> > + * for the chominance component of intra-coded frames, in zigzag scanning
> > + * order. Only relevant for 4:2:2 and 4:4:4 YUV formats.
> > + * @chroma_non_intra_quantiser_matrix: The quantization matrix coefficients
> > + * for the chrominance component of non-intra-coded frames, in zigzag scanning
> > + * order. Only relevant for 4:2:2 and 4:4:4 YUV formats.
> > + */
> >  struct v4l2_ctrl_mpeg2_quantization {
> >  	__u8	intra_quantiser_matrix[64];
> >  	__u8	non_intra_quantiser_matrix[64];
> > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> > index d25b38f78229..86f6c3ef7104 100644
> > --- a/include/media/v4l2-ctrls.h
> > +++ b/include/media/v4l2-ctrls.h
> > @@ -42,6 +42,8 @@ struct video_device;
> >   * @p_u16:			Pointer to a 16-bit unsigned value.
> >   * @p_u32:			Pointer to a 32-bit unsigned value.
> >   * @p_char:			Pointer to a string.
> > + * @p_mpeg2_sequence:		Pointer to a MPEG2 sequence structure.
> > + * @p_mpeg2_picture:		Pointer to a MPEG2 picture structure.
> >   * @p_mpeg2_slice_params:	Pointer to a MPEG2 slice parameters structure.
> >   * @p_mpeg2_quantization:	Pointer to a MPEG2 quantization data structure.
> >   * @p_fwht_params:		Pointer to a FWHT stateless parameters structure.
> > @@ -66,6 +68,8 @@ union v4l2_ctrl_ptr {
> >  	u16 *p_u16;
> >  	u32 *p_u32;
> >  	char *p_char;
> > +	struct v4l2_ctrl_mpeg2_sequence *p_sequence;
> > +	struct v4l2_ctrl_mpeg2_picture *p_picture;
> 
> Should start with p_mpeg2_ for both pointers.
> 

Oh, Jonas already pointed this out, it's an overlook.

Thanks,
Ezequiel

> >  	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
> >  	struct v4l2_ctrl_mpeg2_quantization *p_mpeg2_quantization;
> >  	struct v4l2_ctrl_fwht_params *p_fwht_params;
> > 
> 
> Regards,
> 
> 	Hans



^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH v3 3/3] media: uapi: mpeg2: Split sequence and picture parameters
  2020-12-02 21:46     ` Ezequiel Garcia
@ 2020-12-03  8:51       ` Hans Verkuil
  0 siblings, 0 replies; 7+ messages in thread
From: Hans Verkuil @ 2020-12-03  8:51 UTC (permalink / raw)
  To: Ezequiel Garcia, linux-media, linux-kernel
  Cc: kernel, Jonas Karlman, Nicolas Dufresne, Philipp Zabel,
	Maxime Ripard, Paul Kocialkowski, Jernej Skrabec

On 02/12/2020 22:46, Ezequiel Garcia wrote:
> On Wed, 2020-12-02 at 16:11 +0100, Hans Verkuil wrote:
>> Hi Ezequiel,
>>
>> Some small comments:
>>
>> On 30/11/2020 19:52, Ezequiel Garcia wrote:
>>> Typically, bitstreams are composed of one sequence header NAL unit,
>>> followed by a number of picture header and picture coding extension
>>> NAL units. Each picture can be composed by a number of slices.
>>>
>>> Let's split the MPEG-2 uAPI to follow these semantics more closely,
>>> allowing more usage flexibility. Having these controls split
>>> allows applications to set a sequence control at the beginning
>>> of a sequence, and then set a picture control for each frame.
>>>
>>> While here add padding fields where needed, and document
>>> the uAPI header thoroughly.
>>>
>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>>> Tested-by: Jonas Karlman <jonas@kwiboo.se>
>>> ---
>>>  .../media/v4l/ext-ctrls-codec.rst             |  47 ++++++--
>>>  .../media/v4l/pixfmt-compressed.rst           |   5 +-
>>>  drivers/media/v4l2-core/v4l2-ctrls.c          |  57 +++++++--
>>>  drivers/staging/media/hantro/hantro_drv.c     |  10 ++
>>>  .../media/hantro/hantro_g1_mpeg2_dec.c        |  14 ++-
>>>  .../media/hantro/rk3399_vpu_hw_mpeg2_dec.c    |  14 ++-
>>>  drivers/staging/media/sunxi/cedrus/cedrus.c   |  14 +++
>>>  drivers/staging/media/sunxi/cedrus/cedrus.h   |   2 +
>>>  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c |   8 +-
>>>  include/media/mpeg2-ctrls.h                   | 110 +++++++++++++++---
>>>  include/media/v4l2-ctrls.h                    |   4 +
>>>  11 files changed, 224 insertions(+), 61 deletions(-)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> index 9a804f3037d8..a789866ebda4 100644
>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> @@ -1526,14 +1526,6 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>>>      :stub-columns: 0
>>>      :widths:       1 1 2
>>>  
>>> -    * - struct :c:type:`v4l2_mpeg2_sequence`
>>> -      - ``sequence``
>>> -      - Structure with MPEG-2 sequence metadata, merging relevant fields from
>>> -	the sequence header and sequence extension parts of the bitstream.
>>> -    * - struct :c:type:`v4l2_mpeg2_picture`
>>> -      - ``picture``
>>> -      - Structure with MPEG-2 picture metadata, merging relevant fields from
>>> -	the picture header and picture coding extension parts of the bitstream.
>>>      * - __u64
>>>        - ``backward_ref_ts``
>>>        - Timestamp of the V4L2 capture buffer to use as backward reference, used
>>> @@ -1551,14 +1543,28 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>>>      * - __u32
>>>        - ``quantiser_scale_code``
>>>        - Code used to determine the quantization scale to use for the IDCT.
>>> +    * - __u8
>>> +      - ``reserved``
>>> +      - Applications and drivers must set this to zero.
>>>  
>>> -.. c:type:: v4l2_mpeg2_sequence
>>> +``V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE (struct)``
>>> +    Specifies the sequence parameters (as extracted from the bitstream) for the
>>> +    associated MPEG-2 slice data. This includes fields matching the syntax
>>> +    elements from the sequence header and sequence extension parts of the
>>> +    bitstream as specified by :ref:`mpeg2part2`.
>>> +
>>> +    .. note::
>>> +
>>> +       This compound control is not yet part of the public kernel API and
>>> +       it is expected to change.
>>> +
>>> +.. c:type:: v4l2_ctrl_mpeg2_sequence
>>>  
>>>  .. cssclass:: longtable
>>>  
>>>  .. tabularcolumns:: |p{1.5cm}|p{6.3cm}|p{9.4cm}|
>>>  
>>> -.. flat-table:: struct v4l2_mpeg2_sequence
>>> +.. flat-table:: struct v4l2_ctrl_mpeg2_sequence
>>>      :header-rows:  0
>>>      :stub-columns: 0
>>>      :widths:       1 1 2
>>> @@ -1580,6 +1586,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>>>      * - __u8
>>>        - ``chroma_format``
>>>        - The chrominance sub-sampling format (1: 4:2:0, 2: 4:2:2, 3: 4:4:4).
>>> +    * - __u8
>>> +      - ``reserved``
>>> +      - Applications and drivers must set this to zero.
>>>      * - __u32
>>>        - ``flags``
>>>        - See :ref:`MPEG-2 Sequence Flags <mpeg2_sequence_flags>`.
>>> @@ -1600,13 +1609,24 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>>>        - Indication that all the frames for the sequence are progressive instead
>>>  	of interlaced.
>>>  
>>> -.. c:type:: v4l2_mpeg2_picture
>>> +``V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE (struct)``
>>> +    Specifies the picture parameters (as extracted from the bitstream) for the
>>> +    associated MPEG-2 slice data. This includes fields matching the syntax
>>> +    elements from the picture header and picture coding extension parts of the
>>> +    bitstream as specified by :ref:`mpeg2part2`.
>>> +
>>> +    .. note::
>>> +
>>> +       This compound control is not yet part of the public kernel API and
>>> +       it is expected to change.
>>> +
>>> +.. c:type:: v4l2_ctrl_mpeg2_picture
>>>  
>>>  .. cssclass:: longtable
>>>  
>>>  .. tabularcolumns:: |p{1.5cm}|p{6.3cm}|p{9.4cm}|
>>>  
>>> -.. flat-table:: struct v4l2_mpeg2_picture
>>> +.. flat-table:: struct v4l2_ctrl_mpeg2_picture
>>>      :header-rows:  0
>>>      :stub-columns: 0
>>>      :widths:       1 1 2
>>> @@ -1627,6 +1647,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>>>        - ``picture_structure``
>>>        - Picture structure (1: interlaced top field, 2: interlaced bottom field,
>>>  	3: progressive frame).
>>> +    * - __u8
>>> +      - ``reserved``
>>> +      - Applications and drivers must set this to zero.
>>>      * - __u32
>>>        - ``flags``
>>>        - See :ref:`MPEG-2 Picture Flags <mpeg2_picture_flags>`.
>>> diff --git a/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst b/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
>>> index b8899262d8de..0c22f3f99ce4 100644
>>> --- a/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
>>> +++ b/Documentation/userspace-api/media/v4l/pixfmt-compressed.rst
>>> @@ -108,8 +108,9 @@ Compressed Formats
>>>  	This format is adapted for stateless video decoders that implement a
>>>  	MPEG-2 pipeline (using the :ref:`mem2mem` and :ref:`media-request-api`).
>>>  	Metadata associated with the frame to decode is required to be passed
>>> -	through the ``V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS`` control and
>>> -	quantization matrices can optionally be specified through the
>>> +	through the ``V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE``,
>>> +        ``V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE``, and ``V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS``
>>> +        controls. Quantization matrices can optionally be specified through the
>>>  	``V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION`` control.
>>>  	See the :ref:`associated Codec Control IDs <v4l2-mpeg-mpeg2>`.
>>>  	Exactly one output and one capture buffer must be provided for use with
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> index dd8f3bb4fbb9..a43baa9367ab 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> @@ -941,6 +941,8 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>  	case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:		return "Vertical MV Search Range";
>>>  	case V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER:		return "Repeat Sequence Header";
>>>  	case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:		return "Force Key Frame";
>>> +	case V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE:		return "MPEG-2 Sequence Header";
>>> +	case V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE:			return "MPEG-2 Picture Header";
>>>  	case V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS:		return "MPEG-2 Slice Parameters";
>>>  	case V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION:		return "MPEG-2 Quantization Matrices";
>>>  	case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS:			return "FWHT Stateless Parameters";
>>> @@ -1427,6 +1429,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>>  	case V4L2_CID_RDS_TX_ALT_FREQS:
>>>  		*type = V4L2_CTRL_TYPE_U32;
>>>  		break;
>>> +	case V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE:
>>> +		*type = V4L2_CTRL_TYPE_MPEG2_SEQUENCE;
>>> +		break;
>>> +	case V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE:
>>> +		*type = V4L2_CTRL_TYPE_MPEG2_PICTURE;
>>> +		break;
>>>  	case V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS:
>>>  		*type = V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS;
>>>  		break;
>>> @@ -1625,7 +1633,8 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
>>>  static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>>  			      union v4l2_ctrl_ptr ptr)
>>>  {
>>> -	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
>>> +	struct v4l2_ctrl_mpeg2_sequence *p_mpeg2_sequence;
>>> +	struct v4l2_ctrl_mpeg2_picture *p_mpeg2_picture;
>>>  	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
>>>  	void *p = ptr.p + idx * ctrl->elem_size;
>>>  
>>> @@ -1640,13 +1649,18 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>>  	 * v4l2_ctrl_type enum.
>>>  	 */
>>>  	switch ((u32)ctrl->type) {
>>> -	case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
>>> -		p_mpeg2_slice_params = p;
>>> +	case V4L2_CTRL_TYPE_MPEG2_SEQUENCE:
>>> +		p_mpeg2_sequence = p;
>>> +
>>>  		/* 4:2:0 */
>>> -		p_mpeg2_slice_params->sequence.chroma_format = 1;
>>> +		p_mpeg2_sequence->chroma_format = 1;
>>> +		break;
>>> +	case V4L2_CTRL_TYPE_MPEG2_PICTURE:
>>> +		p_mpeg2_picture = p;
>>> +
>>>  		/* interlaced top field */
>>> -		p_mpeg2_slice_params->picture.picture_structure = 1;
>>> -		p_mpeg2_slice_params->picture.picture_coding_type =
>>> +		p_mpeg2_picture->picture_structure = 1;
>>> +		p_mpeg2_picture->picture_coding_type =
>>>  					V4L2_MPEG2_PIC_CODING_TYPE_I;
>>>  		break;
>>>  	case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
>>
>> You should also add the MPEG2 types to std_log(). None of them are logged
>> there.
>>
> 
> I was reserving that stuff for the destaging effort.

I prefer to have these MPEG2 types added to std_log(). We are also updating
std_init and std_validate, so it makes sense to add it to std_log as well.

It also reduces the destaging effort a bit.

> 
> Perhaps we are comfortable with destaging on the next iteration?

It depends mostly on the codec experts, if they are happy with it, then we
can destage.

> 
>>> @@ -1796,6 +1810,8 @@ static void std_log(const struct v4l2_ctrl *ctrl)
>>>  static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>>  				 union v4l2_ctrl_ptr ptr)
>>>  {
>>> +	struct v4l2_ctrl_mpeg2_sequence *p_mpeg2_sequence;
>>> +	struct v4l2_ctrl_mpeg2_picture *p_mpeg2_picture;
>>>  	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
>>>  	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
>>>  	struct v4l2_ctrl_h264_sps *p_h264_sps;
>>> @@ -1811,10 +1827,10 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>>  	unsigned int i;
>>>  
>>>  	switch ((u32)ctrl->type) {
>>> -	case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
>>> -		p_mpeg2_slice_params = p;
>>> +	case V4L2_CTRL_TYPE_MPEG2_SEQUENCE:
>>> +		p_mpeg2_sequence = p;
>>>  
>>> -		switch (p_mpeg2_slice_params->sequence.chroma_format) {
>>> +		switch (p_mpeg2_sequence->chroma_format) {
>>>  		case 1: /* 4:2:0 */
>>>  		case 2: /* 4:2:2 */
>>>  		case 3: /* 4:4:4 */
>>> @@ -1822,8 +1838,13 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>>  		default:
>>>  			return -EINVAL;
>>>  		}
>>> +		zero_reserved(*p_mpeg2_sequence);
>>> +		break;
>>> +
>>> +	case V4L2_CTRL_TYPE_MPEG2_PICTURE:
>>> +		p_mpeg2_picture = p;
>>>  
>>> -		switch (p_mpeg2_slice_params->picture.intra_dc_precision) {
>>> +		switch (p_mpeg2_picture->intra_dc_precision) {
>>>  		case 0: /* 8 bits */
>>>  		case 1: /* 9 bits */
>>>  		case 2: /* 10 bits */
>>> @@ -1833,7 +1854,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>>  			return -EINVAL;
>>>  		}
>>>  
>>> -		switch (p_mpeg2_slice_params->picture.picture_structure) {
>>> +		switch (p_mpeg2_picture->picture_structure) {
>>>  		case V4L2_MPEG2_PIC_TOP_FIELD:
>>>  		case V4L2_MPEG2_PIC_BOTTOM_FIELD:
>>>  		case V4L2_MPEG2_PIC_FRAME:
>>> @@ -1842,7 +1863,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>>  			return -EINVAL;
>>>  		}
>>>  
>>> -		switch (p_mpeg2_slice_params->picture.picture_coding_type) {
>>> +		switch (p_mpeg2_picture->picture_coding_type) {
>>>  		case V4L2_MPEG2_PIC_CODING_TYPE_I:
>>>  		case V4L2_MPEG2_PIC_CODING_TYPE_P:
>>>  		case V4L2_MPEG2_PIC_CODING_TYPE_B:
>>> @@ -1850,7 +1871,13 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>>  		default:
>>>  			return -EINVAL;
>>>  		}
>>> +		zero_reserved(*p_mpeg2_picture);
>>> +		break;
>>>  
>>> +	case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
>>> +		p_mpeg2_slice_params = p;
>>> +
>>> +		zero_reserved(*p_mpeg2_slice_params);
>>>  		break;
>>>  
>>>  	case V4L2_CTRL_TYPE_MPEG2_QUANTIZATION:
>>> @@ -2749,6 +2776,12 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>>  	case V4L2_CTRL_TYPE_U32:
>>>  		elem_size = sizeof(u32);
>>>  		break;
>>> +	case V4L2_CTRL_TYPE_MPEG2_SEQUENCE:
>>> +		elem_size = sizeof(struct v4l2_ctrl_mpeg2_sequence);
>>> +		break;
>>> +	case V4L2_CTRL_TYPE_MPEG2_PICTURE:
>>> +		elem_size = sizeof(struct v4l2_ctrl_mpeg2_picture);
>>> +		break;
>>>  	case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
>>>  		elem_size = sizeof(struct v4l2_ctrl_mpeg2_slice_params);
>>>  		break;
>>> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
>>> index e5f200e64993..e589eccd5644 100644
>>> --- a/drivers/staging/media/hantro/hantro_drv.c
>>> +++ b/drivers/staging/media/hantro/hantro_drv.c
>>> @@ -286,6 +286,16 @@ static const struct hantro_ctrl controls[] = {
>>>  			.def = 50,
>>>  			.ops = &hantro_jpeg_ctrl_ops,
>>>  		},
>>> +	}, {
>>> +		.codec = HANTRO_MPEG2_DECODER,
>>> +		.cfg = {
>>> +			.id = V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE,
>>> +		},
>>> +	}, {
>>> +		.codec = HANTRO_MPEG2_DECODER,
>>> +		.cfg = {
>>> +			.id = V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE,
>>> +		},
>>>  	}, {
>>>  		.codec = HANTRO_MPEG2_DECODER,
>>>  		.cfg = {
>>> diff --git a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
>>> index b6086474d31f..1a6ca49441f4 100644
>>> --- a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
>>> +++ b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c
>>> @@ -95,8 +95,8 @@ static void
>>>  hantro_g1_mpeg2_dec_set_buffers(struct hantro_dev *vpu, struct hantro_ctx *ctx,
>>>  				struct vb2_buffer *src_buf,
>>>  				struct vb2_buffer *dst_buf,
>>> -				const struct v4l2_mpeg2_sequence *seq,
>>> -				const struct v4l2_mpeg2_picture *pic,
>>> +				const struct v4l2_ctrl_mpeg2_sequence *seq,
>>> +				const struct v4l2_ctrl_mpeg2_picture *pic,
>>>  				const struct v4l2_ctrl_mpeg2_slice_params *slice_params)
>>>  {
>>>  	dma_addr_t forward_addr = 0, backward_addr = 0;
>>> @@ -156,8 +156,8 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx)
>>>  	struct hantro_dev *vpu = ctx->dev;
>>>  	struct vb2_v4l2_buffer *src_buf, *dst_buf;
>>>  	const struct v4l2_ctrl_mpeg2_slice_params *slice_params;
>>> -	const struct v4l2_mpeg2_sequence *seq;
>>> -	const struct v4l2_mpeg2_picture *pic;
>>> +	const struct v4l2_ctrl_mpeg2_sequence *seq;
>>> +	const struct v4l2_ctrl_mpeg2_picture *pic;
>>>  	u32 reg;
>>>  
>>>  	src_buf = hantro_get_src_buf(ctx);
>>> @@ -168,8 +168,10 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx)
>>>  
>>>  	slice_params = hantro_get_ctrl(ctx,
>>>  				       V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS);
>>> -	seq = &slice_params->sequence;
>>> -	pic = &slice_params->picture;
>>> +	seq = hantro_get_ctrl(ctx,
>>> +			      V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE);
>>> +	pic = hantro_get_ctrl(ctx,
>>> +			      V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE);
>>>  
>>>  	reg = G1_REG_DEC_AXI_RD_ID(0) |
>>>  	      G1_REG_DEC_TIMEOUT_E(1) |
>>> diff --git a/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c b/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c
>>> index 28eb77b0569b..45ab5ca32221 100644
>>> --- a/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c
>>> +++ b/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c
>>> @@ -97,8 +97,8 @@ rk3399_vpu_mpeg2_dec_set_buffers(struct hantro_dev *vpu,
>>>  				 struct hantro_ctx *ctx,
>>>  				 struct vb2_buffer *src_buf,
>>>  				 struct vb2_buffer *dst_buf,
>>> -				 const struct v4l2_mpeg2_sequence *seq,
>>> -				 const struct v4l2_mpeg2_picture *pic,
>>> +				 const struct v4l2_ctrl_mpeg2_sequence *seq,
>>> +				 const struct v4l2_ctrl_mpeg2_picture *pic,
>>>  				 const struct v4l2_ctrl_mpeg2_slice_params *slice_params)
>>>  {
>>>  	dma_addr_t forward_addr = 0, backward_addr = 0;
>>> @@ -158,8 +158,8 @@ void rk3399_vpu_mpeg2_dec_run(struct hantro_ctx *ctx)
>>>  	struct hantro_dev *vpu = ctx->dev;
>>>  	struct vb2_v4l2_buffer *src_buf, *dst_buf;
>>>  	const struct v4l2_ctrl_mpeg2_slice_params *slice_params;
>>> -	const struct v4l2_mpeg2_sequence *seq;
>>> -	const struct v4l2_mpeg2_picture *pic;
>>> +	const struct v4l2_ctrl_mpeg2_sequence *seq;
>>> +	const struct v4l2_ctrl_mpeg2_picture *pic;
>>>  	u32 reg;
>>>  
>>>  	src_buf = hantro_get_src_buf(ctx);
>>> @@ -169,8 +169,10 @@ void rk3399_vpu_mpeg2_dec_run(struct hantro_ctx *ctx)
>>>  
>>>  	slice_params = hantro_get_ctrl(ctx,
>>>  				       V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS);
>>> -	seq = &slice_params->sequence;
>>> -	pic = &slice_params->picture;
>>> +	seq = hantro_get_ctrl(ctx,
>>> +			      V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE);
>>> +	pic = hantro_get_ctrl(ctx,
>>> +			      V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE);
>>>  
>>>  	reg = VDPU_REG_DEC_ADV_PRE_DIS(0) |
>>>  	      VDPU_REG_DEC_SCMD_DIS(0) |
>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
>>> index 4e91c372d585..3f9fab6ddbaa 100644
>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
>>> @@ -29,6 +29,20 @@
>>>  #include "cedrus_hw.h"
>>>  
>>>  static const struct cedrus_control cedrus_controls[] = {
>>> +	{
>>> +		.cfg = {
>>> +			.id	= V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE,
>>> +		},
>>> +		.codec		= CEDRUS_CODEC_MPEG2,
>>> +		.required	= true,
>>> +	},
>>> +	{
>>> +		.cfg = {
>>> +			.id	= V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE,
>>> +		},
>>> +		.codec		= CEDRUS_CODEC_MPEG2,
>>> +		.required	= true,
>>> +	},
>>>  	{
>>>  		.cfg = {
>>>  			.id	= V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS,
>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
>>> index fece505272b4..c286b7312386 100644
>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
>>> @@ -68,6 +68,8 @@ struct cedrus_h264_run {
>>>  };
>>>  
>>>  struct cedrus_mpeg2_run {
>>> +	const struct v4l2_ctrl_mpeg2_sequence		*sequence;
>>> +	const struct v4l2_ctrl_mpeg2_picture		*picture;
>>>  	const struct v4l2_ctrl_mpeg2_slice_params	*slice_params;
>>>  	const struct v4l2_ctrl_mpeg2_quantization	*quantization;
>>>  };
>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
>>> index 6a99893cdc77..ea52778b36f9 100644
>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
>>> @@ -75,8 +75,8 @@ static void cedrus_mpeg2_irq_disable(struct cedrus_ctx *ctx)
>>>  static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
>>>  {
>>>  	const struct v4l2_ctrl_mpeg2_slice_params *slice_params;
>>> -	const struct v4l2_mpeg2_sequence *seq;
>>> -	const struct v4l2_mpeg2_picture *pic;
>>> +	const struct v4l2_ctrl_mpeg2_sequence *seq;
>>> +	const struct v4l2_ctrl_mpeg2_picture *pic;
>>>  	const struct v4l2_ctrl_mpeg2_quantization *quantization;
>>>  	dma_addr_t src_buf_addr, dst_luma_addr, dst_chroma_addr;
>>>  	dma_addr_t fwd_luma_addr, fwd_chroma_addr;
>>> @@ -90,8 +90,8 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
>>>  	u32 reg;
>>>  
>>>  	slice_params = run->mpeg2.slice_params;
>>> -	seq = &slice_params->sequence;
>>> -	pic = &slice_params->picture;
>>> +	seq = run->mpeg2.sequence;
>>> +	pic = run->mpeg2.picture;
>>>  
>>>  	quantization = run->mpeg2.quantization;
>>>  
>>> diff --git a/include/media/mpeg2-ctrls.h b/include/media/mpeg2-ctrls.h
>>> index 038206c7e6f5..edcbf67668ac 100644
>>> --- a/include/media/mpeg2-ctrls.h
>>> +++ b/include/media/mpeg2-ctrls.h
>>> @@ -12,24 +12,46 @@
>>>  #define _MPEG2_CTRLS_H_
>>>  
>>>  #define V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS		(V4L2_CID_CODEC_BASE+250)
>>> -#define V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION		(V4L2_CID_CODEC_BASE+251)
>>> +#define V4L2_CID_MPEG_VIDEO_MPEG2_SEQUENCE		(V4L2_CID_CODEC_BASE+251)
>>> +#define V4L2_CID_MPEG_VIDEO_MPEG2_PICTURE		(V4L2_CID_CODEC_BASE+252)
>>> +#define V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION		(V4L2_CID_CODEC_BASE+253)
>>>  
>>>  /* enum v4l2_ctrl_type type values */
>>> -#define V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS 0x0103
>>> -#define	V4L2_CTRL_TYPE_MPEG2_QUANTIZATION 0x0104
>>> +#define V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS 0x0130
>>> +#define V4L2_CTRL_TYPE_MPEG2_SEQUENCE 0x0131
>>> +#define V4L2_CTRL_TYPE_MPEG2_PICTURE 0x0132
>>> +#define V4L2_CTRL_TYPE_MPEG2_QUANTIZATION 0x0133
>>>  
>>>  #define V4L2_MPEG2_SEQ_FLAG_PROGRESSIVE		0x0001
>>>  
>>> -struct v4l2_mpeg2_sequence {
>>> -	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Sequence header */
>>> +/**
>>> + * struct v4l2_ctrl_mpeg2_sequence - MPEG-2 sequence header
>>> + *
>>> + * All the members on this structure match the sequence header and sequence
>>> + * extension syntaxes as specified by the MPEG-2 specification.
>>> + *
>>> + * Fields horizontal_size, vertical_size and vbv_buffer_size are a
>>> + * combination of respective _value and extension syntax elements,
>>
>> Is that a stray '_' or is it really '_value'? Just double-checking :-)
>>
> 
> Probably it's a typo. BTW, I wonder if that sentence is clear enough.
> What I wanted to clarify is that this horizontal_size field is
> a combination of syntax elements horizontal_size_value
> and horizontal_size_extension syntax elements.
> 
> (and same for vertical_size)

After reading up on it I would suggest to just drop the 'combining _value and
_extension' bit. The MPEG-2 specification clearly defines how the horizontal_size
is obtained, so I don't think that needs to be documented here.

Instead I would document what it actually means. E.g.

@horizontal_size: the width of the displayable part of the luminance
component of pictures in samples.

Optionally you can add this sentence: "Derived from horizontal_size_value
and horizontal_size_extension."

But to be honest, I think that is not needed.

Regards,

	Hans

> 
>>> + * as described in section 6.3.3 "Sequence header".
>>> + *
>>> + * @horizontal_size: combination of elements horizontal_size_value and
>>> + * horizontal_size_extension.
>>> + * @vertical_size: combination of elements vertical_size_value and
>>> + * vertical_size_extension.
>>> + * @vbv_buffer_size: combination of elements vbv_buffer_size_value and
>>> + * vbv_buffer_size_extension.
>>> + * @profile_and_level_indication: see MPEG-2 specification.
>>> + * @chroma_format: see MPEG-2 specification.
>>> + * @reserved: padding field. Should be zeroed by applications.
>>> + * @flags: see V4L2_MPEG2_SEQ_FLAG_{}.
>>> + */
>>> +struct v4l2_ctrl_mpeg2_sequence {
>>>  	__u16	horizontal_size;
>>>  	__u16	vertical_size;
>>>  	__u32	vbv_buffer_size;
>>> -
>>> -	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Sequence extension */
>>>  	__u16	profile_and_level_indication;
>>>  	__u8	chroma_format;
>>> -
>>> +	__u8	reserved;
>>>  	__u32	flags;
>>>  };
>>>  
>>> @@ -55,30 +77,80 @@ struct v4l2_mpeg2_sequence {
>>>  #define V4L2_MPEG2_PIC_FLAG_LOAD_CHROMA_INTRA		0x0400
>>>  #define V4L2_MPEG2_PIC_FLAG_LOAD_CHROMA_NON_INTRA	0x0800
>>>  
>>> -struct v4l2_mpeg2_picture {
>>> -	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Picture header */
>>> +/**
>>> + * struct v4l2_ctrl_mpeg2_picture - MPEG-2 picture header
>>> + *
>>> + * All the members on this structure match the picture header and picture
>>> + * coding extension syntaxes as specified by the MPEG-2 specification.
>>> + *
>>> + * In particular, the set of quantization load flags V4L2_MPEG2_PIC_FLAG_LOAD_{}
>>> + * are specified here in order to allow applications to pass non-default
>>> + * quantization matrices. In this case, applications are expected to use
>>> + * V4L2_CTRL_TYPE_MPEG2_QUANTIZATION to pass the values of non-default
>>> + * matrices.
>>> + *
>>> + * @picture_coding_type: see MPEG-2 specification.
>>> + * @f_code[2][2]: see MPEG-2 specification.
>>> + * @intra_dc_precision: see MPEG-2 specification.
>>> + * @picture_structure: see V4L2_MPEG2_PIC_{}_FIELD.
>>> + * @reserved: padding field. Should be zeroed by applications.
>>> + * @flags: see V4L2_MPEG2_PIC_FLAG_{}.
>>> + */
>>> +struct v4l2_ctrl_mpeg2_picture {
>>>  	__u8	picture_coding_type;
>>> -
>>> -	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Picture coding extension */
>>>  	__u8	f_code[2][2];
>>>  	__u8	intra_dc_precision;
>>>  	__u8	picture_structure;
>>> -
>>> +	__u8	reserved;
>>>  	__u32	flags;
>>>  };
>>>  
>>> +/**
>>> + * struct v4l2_ctrl_mpeg2_slice_params - MPEG-2 slice header
>>> + *
>>> + * @backward_ref_ts: timestamp of the V4L2 capture buffer to use as
>>> + * reference for backward prediction.
>>> + * @forward_ref_ts: timestamp of the V4L2 capture buffer to use as
>>> + * reference for forward prediction. These timestamp refers to the
>>> + * timestamp field in struct v4l2_buffer. Use v4l2_timeval_to_ns()
>>> + * to convert the struct timeval to a __u64.
>>> + * @quantiser_scale_code: quantiser scale integer matching an
>>> + * homonymous syntax element.
>>> + * @reserved: padding field. Should be zeroed by applications.
>>> + */
>>>  struct v4l2_ctrl_mpeg2_slice_params {
>>>  	__u64	backward_ref_ts;
>>>  	__u64	forward_ref_ts;
>>> -
>>> -	struct v4l2_mpeg2_sequence sequence;
>>> -	struct v4l2_mpeg2_picture picture;
>>> -
>>> -	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Slice */
>>>  	__u32	quantiser_scale_code;
>>> +	__u32	reserved;
>>>  };
>>>  
>>> -/* ISO/IEC 13818-2, ITU-T Rec. H.262: Quant matrix extension */
>>> +/**
>>> + * struct v4l2_ctrl_mpeg2_quantization - MPEG-2 quantization
>>> + *
>>> + * Quantization matrices as specified by section 6.3.7
>>> + * "Quant matrix extension".
>>> + *
>>> + * Applications are expected to set the quantization matrices load
>>> + * flags V4L2_MPEG2_PIC_FLAG_LOAD_{} in struct v4l2_ctrl_mpeg2_picture
>>> + * to tell the kernel that a non-default matrix shall be used
>>> + * to decode the picture.
>>> + *
>>> + * @intra_quantiser_matrix: The quantization matrix coefficients
>>> + * for intra-coded frames, in zigzag scanning order. It is relevant
>>> + * for both luma and chroma components, although it can be superseded
>>> + * by the chroma-specific matrix for non-4:2:0 YUV formats.
>>> + * @non_intra_quantiser_matrix: The quantization matrix coefficients
>>> + * for non-intra-coded frames, in zigzag scanning order. It is relevant
>>> + * for both luma and chroma components, although it can be superseded
>>> + * by the chroma-specific matrix for non-4:2:0 YUV formats.
>>> + * @chroma_intra_quantiser_matrix: The quantization matrix coefficients
>>> + * for the chominance component of intra-coded frames, in zigzag scanning
>>> + * order. Only relevant for 4:2:2 and 4:4:4 YUV formats.
>>> + * @chroma_non_intra_quantiser_matrix: The quantization matrix coefficients
>>> + * for the chrominance component of non-intra-coded frames, in zigzag scanning
>>> + * order. Only relevant for 4:2:2 and 4:4:4 YUV formats.
>>> + */
>>>  struct v4l2_ctrl_mpeg2_quantization {
>>>  	__u8	intra_quantiser_matrix[64];
>>>  	__u8	non_intra_quantiser_matrix[64];
>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>>> index d25b38f78229..86f6c3ef7104 100644
>>> --- a/include/media/v4l2-ctrls.h
>>> +++ b/include/media/v4l2-ctrls.h
>>> @@ -42,6 +42,8 @@ struct video_device;
>>>   * @p_u16:			Pointer to a 16-bit unsigned value.
>>>   * @p_u32:			Pointer to a 32-bit unsigned value.
>>>   * @p_char:			Pointer to a string.
>>> + * @p_mpeg2_sequence:		Pointer to a MPEG2 sequence structure.
>>> + * @p_mpeg2_picture:		Pointer to a MPEG2 picture structure.
>>>   * @p_mpeg2_slice_params:	Pointer to a MPEG2 slice parameters structure.
>>>   * @p_mpeg2_quantization:	Pointer to a MPEG2 quantization data structure.
>>>   * @p_fwht_params:		Pointer to a FWHT stateless parameters structure.
>>> @@ -66,6 +68,8 @@ union v4l2_ctrl_ptr {
>>>  	u16 *p_u16;
>>>  	u32 *p_u32;
>>>  	char *p_char;
>>> +	struct v4l2_ctrl_mpeg2_sequence *p_sequence;
>>> +	struct v4l2_ctrl_mpeg2_picture *p_picture;
>>
>> Should start with p_mpeg2_ for both pointers.
>>
> 
> Oh, Jonas already pointed this out, it's an overlook.
> 
> Thanks,
> Ezequiel
> 
>>>  	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
>>>  	struct v4l2_ctrl_mpeg2_quantization *p_mpeg2_quantization;
>>>  	struct v4l2_ctrl_fwht_params *p_fwht_params;
>>>
>>
>> Regards,
>>
>> 	Hans
> 
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-12-03  8:53 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-30 18:52 [PATCH v3 0/3] MPEG-2 stateless API cleanup Ezequiel Garcia
2020-11-30 18:52 ` [PATCH v3 1/3] media: uapi: mpeg2: Cleanup flags Ezequiel Garcia
2020-11-30 18:52 ` [PATCH v3 2/3] media: uapi: mpeg2: Remove unused slice size and offset Ezequiel Garcia
2020-11-30 18:52 ` [PATCH v3 3/3] media: uapi: mpeg2: Split sequence and picture parameters Ezequiel Garcia
2020-12-02 15:11   ` Hans Verkuil
2020-12-02 21:46     ` Ezequiel Garcia
2020-12-03  8:51       ` Hans Verkuil

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).