[RFC 0/7] media: Clean and make H264 stateless uAPI public

[RFC 0/7] media: Clean and make H264 stateless uAPI public

[Ezequiel Garcia]

The recent patch posted by Jernej (which I'm including for context),
encouraged me to address all the known issues in the uAPI.

I hope we can finally make this uAPI interface
public; it would be nice to address the other codec
interfaces so we can move the codec drivers out of staging.

It should be noted that there is already GStreamer native
support for this interface, which will be part of 1.18,
once it's released [1], as well as support in Chromium [2].

The basic idea here is to sanitize the interface,
making sure the structs are aligned to 64-bit,
adding reserved fields for padding where suitable.

These reserved fields can then be used to support future extensions,
in case such need appears.

In addition to this, moving the slice invariant fields
to the per-frame control, makes the frame-mode driver
implementation much nicer and the interface; see patch 6/7 for details.

I'm not adding a MAINTAINERS entry, but I'd like to do so,
so we make sure any uAPI changes are sent to those involved.

Another potential change is the addition of a "Since:" tag to the
control specification, so we can document which kernel version
added the interface. This might prove useful if reserved
fields are then used to extend the interface.

Finally, I'm sneaking here a change from Philipp Zabel
which apparently fell thru the cracks.

[1] https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/tree/master/sys/v4l2codecs
[2] https://chromium.googlesource.com/chromium/src.git/+/refs/heads/master/media/gpu/v4l2/

Ezequiel Garcia (5):
  fixup! media: uapi: h264: update reference lists
  media: uapi: h264: increase size of fields
  media: uapi: h264: pad v4l2_ctrl_h264_pps to 64-bit
  media: uapi: h264: Clean slice invariants syntax elements
  media: uapi: make H264 stateless codec interface public

Jernej Skrabec (1):
  media: uapi: h264: update reference lists

Philipp Zabel (1):
  media: uapi: h264: clarify pic_order_cnt_bit_size field

 .../media/v4l/ext-ctrls-codec.rst             | 135 ++++++++++++------
 drivers/media/v4l2-core/v4l2-ctrls.c          |  31 ++++
 drivers/media/v4l2-core/v4l2-h264.c           |   8 +-
 .../staging/media/hantro/hantro_g1_h264_dec.c |  21 ++-
 drivers/staging/media/hantro/hantro_h264.c    |   3 +-
 drivers/staging/media/hantro/hantro_hw.h      |   5 +-
 drivers/staging/media/rkvdec/rkvdec-h264.c    |   6 +-
 .../staging/media/sunxi/cedrus/cedrus_h264.c  |  15 +-
 include/media/v4l2-ctrls.h                    |   3 +-
 include/media/v4l2-h264.h                     |   5 +-
 .../linux/v4l2-h264-ctrls.h}                  |  73 ++++++----
 11 files changed, 194 insertions(+), 111 deletions(-)
 rename include/{media/h264-ctrls.h => uapi/linux/v4l2-h264-ctrls.h} (88%)

-- 
2.26.0.rc2

[RFC 1/7] media: uapi: h264: update reference lists

[Ezequiel Garcia]

From: Jernej Skrabec <jernej.skrabec@siol.net>

When dealing with with interlaced frames, reference lists must tell if
each particular reference is meant for top or bottom field. This info
is currently not provided at all in the H264 related controls.

Make reference lists hold a structure which will also hold flags along
index into DPB array. Flags will tell if reference is meant for top or
bottom field.

Currently the only user of these lists is Cedrus which is just compile
fixed here. Actual usage of newly introduced flags will come in
following commit.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 .../media/v4l/ext-ctrls-codec.rst             | 40 ++++++++++++++++++-
 .../staging/media/sunxi/cedrus/cedrus_h264.c  |  6 +--
 include/media/h264-ctrls.h                    | 12 +++++-
 3 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index d0d506a444b1..6c36d298db20 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1843,10 +1843,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __u32
       - ``slice_group_change_cycle``
       -
-    * - __u8
+    * - struct :c:type:`v4l2_h264_reference`
       - ``ref_pic_list0[32]``
       - Reference picture list after applying the per-slice modifications
-    * - __u8
+    * - struct :c:type:`v4l2_h264_reference`
       - ``ref_pic_list1[32]``
       - Reference picture list after applying the per-slice modifications
     * - __u32
@@ -1926,6 +1926,42 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
       - ``chroma_offset[32][2]``
       -
 
+``Picture Reference``
+
+.. c:type:: v4l2_h264_reference
+
+.. cssclass:: longtable
+
+.. flat-table:: struct v4l2_h264_reference
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       1 1 2
+
+    * - __u16
+      - ``flags``
+      - See :ref:`Picture Reference Flags <h264_reference_flags>`
+    * - __u8
+      - ``index``
+      -
+
+.. _h264_reference_flags:
+
+``Picture Reference Flags``
+
+.. cssclass:: longtable
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       1 1 2
+
+    * - ``V4L2_H264_REFERENCE_FLAG_TOP_FIELD``
+      - 0x00000001
+      -
+    * - ``V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD``
+      - 0x00000002
+      -
+
 ``V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS (struct)``
     Specifies the decode parameters (as extracted from the bitstream)
     for the associated H264 slice data. This includes the necessary
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
index 54ee2aa423e2..cce527bbdf86 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
@@ -166,8 +166,8 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
 
 static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
 				   struct cedrus_run *run,
-				   const u8 *ref_list, u8 num_ref,
-				   enum cedrus_h264_sram_off sram)
+				   const struct v4l2_h264_reference *ref_list,
+				   u8 num_ref, enum cedrus_h264_sram_off sram)
 {
 	const struct v4l2_ctrl_h264_decode_params *decode = run->h264.decode_params;
 	struct vb2_queue *cap_q;
@@ -188,7 +188,7 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
 		int buf_idx;
 		u8 dpb_idx;
 
-		dpb_idx = ref_list[i];
+		dpb_idx = ref_list[i].index;
 		dpb = &decode->dpb[dpb_idx];
 
 		if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
index 080fd1293c42..9b1cbc9bc38e 100644
--- a/include/media/h264-ctrls.h
+++ b/include/media/h264-ctrls.h
@@ -140,6 +140,14 @@ struct v4l2_h264_pred_weight_table {
 #define V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED	0x04
 #define V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH		0x08
 
+#define V4L2_H264_REFERENCE_FLAG_TOP_FIELD		0x01
+#define V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD		0x02
+
+struct v4l2_h264_reference {
+	__u8 flags;
+	__u8 index;
+};
+
 struct v4l2_ctrl_h264_slice_params {
 	/* Size in bytes, including header */
 	__u32 size;
@@ -182,8 +190,8 @@ struct v4l2_ctrl_h264_slice_params {
 	 * Entries on each list are indices into
 	 * v4l2_ctrl_h264_decode_params.dpb[].
 	 */
-	__u8 ref_pic_list0[32];
-	__u8 ref_pic_list1[32];
+	struct v4l2_h264_reference ref_pic_list0[32];
+	struct v4l2_h264_reference ref_pic_list1[32];
 
 	__u32 flags;
 };
-- 
2.26.0.rc2

[RFC 2/7] fixup! media: uapi: h264: update reference lists

[Ezequiel Garcia]

Align v4l2_h264_reference to 32-bits, giving some room
for future extensions.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 .../userspace-api/media/v4l/ext-ctrls-codec.rst  |  3 +++
 drivers/media/v4l2-core/v4l2-ctrls.c             | 16 ++++++++++++++++
 include/media/h264-ctrls.h                       |  7 +++++--
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index 6c36d298db20..7af12447a5b0 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1943,6 +1943,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __u8
       - ``index``
       -
+    * - __u32
+      - ``reserved``
+      - Applications and drivers must set this to zero.
 
 .. _h264_reference_flags:
 
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 3f3fbcd60cc6..6abd023f10c7 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1721,6 +1721,8 @@ static void std_log(const struct v4l2_ctrl *ctrl)
 
 #define zero_padding(s) \
 	memset(&(s).padding, 0, sizeof((s).padding))
+#define zero_reserved(s) \
+	memset(&(s).reserved, 0, sizeof((s).reserved))
 
 /*
  * Compound controls validation requires setting unused fields/flags to zero
@@ -1731,6 +1733,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 {
 	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
 	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
+	struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
 	struct v4l2_ctrl_hevc_sps *p_hevc_sps;
 	struct v4l2_ctrl_hevc_pps *p_hevc_pps;
 	struct v4l2_ctrl_hevc_slice_params *p_hevc_slice_params;
@@ -1790,7 +1793,20 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 	case V4L2_CTRL_TYPE_H264_SPS:
 	case V4L2_CTRL_TYPE_H264_PPS:
 	case V4L2_CTRL_TYPE_H264_SCALING_MATRIX:
+		break;
 	case V4L2_CTRL_TYPE_H264_SLICE_PARAMS:
+		p_h264_slice_params = p;
+
+		for (i = 0; i < V4L2_H264_REF_PIC_LIST_LEN; i++) {
+			struct v4l2_h264_reference *ref0 =
+				&p_h264_slice_params->ref_pic_list0[i];
+			struct v4l2_h264_reference *ref1 =
+				&p_h264_slice_params->ref_pic_list1[i];
+
+			zero_reserved(*ref0);
+			zero_reserved(*ref1);
+		}
+		break;
 	case V4L2_CTRL_TYPE_H264_DECODE_PARAMS:
 		break;
 
diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
index 9b1cbc9bc38e..c6cbf178c1c9 100644
--- a/include/media/h264-ctrls.h
+++ b/include/media/h264-ctrls.h
@@ -19,6 +19,8 @@
  */
 #define V4L2_H264_NUM_DPB_ENTRIES 16
 
+#define V4L2_H264_REF_PIC_LIST_LEN 32
+
 /* Our pixel format isn't stable at the moment */
 #define V4L2_PIX_FMT_H264_SLICE v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices */
 
@@ -146,6 +148,7 @@ struct v4l2_h264_pred_weight_table {
 struct v4l2_h264_reference {
 	__u8 flags;
 	__u8 index;
+	__u16 reserved;
 };
 
 struct v4l2_ctrl_h264_slice_params {
@@ -190,8 +193,8 @@ struct v4l2_ctrl_h264_slice_params {
 	 * Entries on each list are indices into
 	 * v4l2_ctrl_h264_decode_params.dpb[].
 	 */
-	struct v4l2_h264_reference ref_pic_list0[32];
-	struct v4l2_h264_reference ref_pic_list1[32];
+	struct v4l2_h264_reference ref_pic_list0[V4L2_H264_REF_PIC_LIST_LEN];
+	struct v4l2_h264_reference ref_pic_list1[V4L2_H264_REF_PIC_LIST_LEN];
 
 	__u32 flags;
 };
-- 
2.26.0.rc2

[RFC 3/7] media: uapi: h264: clarify pic_order_cnt_bit_size field

[Ezequiel Garcia]

From: Philipp Zabel <p.zabel@pengutronix.de>

Since pic_order_cnt_bit_size is not a syntax element itself, explicitly
state that it is the total size in bits of the pic_order_cnt_lsb,
delta_pic_order_cnt_bottom, delta_pic_order_cnt[0], and
delta_pic_order_cnt[1] syntax elements contained in the slice.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
[Ezequiel: rebase]
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index 7af12447a5b0..0808a36777b6 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1813,7 +1813,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
       - Size in bits of the dec_ref_pic_marking() syntax element.
     * - __u32
       - ``pic_order_cnt_bit_size``
-      -
+      - Combined size in bits of the picture order count related syntax
+        elements: pic_order_cnt_lsb, delta_pic_order_cnt_bottom,
+        delta_pic_order_cnt0, and delta_pic_order_cnt1.
     * - __u8
       - ``cabac_init_idc``
       -
-- 
2.26.0.rc2

[RFC 4/7] media: uapi: h264: increase size of fields

[Ezequiel Garcia]

Slice header syntax element 'first_mb_in_slice' can point
to the last macroblock, currently the field can only reference
65536 macroblocks which is insufficient for 8K videos.

DPB entry PicNum maximum value is 2*MaxFrameNum for interlaced
content (field_pic_flag=1).

Therefore, increase 'first_mb_in_slice' and 'pic_num'.

The v4l2_h264_dpb_entry struct needs to be padded to avoid a hole,
which will be useful to allow future uAPI extensions.

Note that v4l2_ctrl_h264_slice_params struct will be modified
in a follow-up commit, and so we defer its 64-bit padding.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 .../userspace-api/media/v4l/ext-ctrls-codec.rst          | 7 +++++--
 drivers/media/v4l2-core/v4l2-ctrls.c                     | 9 +++++++++
 include/media/h264-ctrls.h                               | 6 ++++--
 include/media/v4l2-h264.h                                | 2 +-
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index 0808a36777b6..e3b5a28fb965 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1772,7 +1772,7 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __u32
       - ``header_bit_size``
       -
-    * - __u16
+    * - __u32
       - ``first_mb_in_slice``
       -
     * - __u8
@@ -2046,7 +2046,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __u16
       - ``frame_num``
       -
-    * - __u16
+    * - __u8
+      - ``reserved[6]``
+      - Applications and drivers must set this to zero.
+    * - __u32
       - ``pic_num``
       -
     * - __s32
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 6abd023f10c7..a751c14f9c22 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1734,6 +1734,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
 	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
 	struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
+	struct v4l2_ctrl_h264_decode_params *p_h264_dec_params;
 	struct v4l2_ctrl_hevc_sps *p_hevc_sps;
 	struct v4l2_ctrl_hevc_pps *p_hevc_pps;
 	struct v4l2_ctrl_hevc_slice_params *p_hevc_slice_params;
@@ -1808,6 +1809,14 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 		}
 		break;
 	case V4L2_CTRL_TYPE_H264_DECODE_PARAMS:
+		p_h264_dec_params = p;
+
+		for (i = 0; i < V4L2_H264_NUM_DPB_ENTRIES; i++) {
+			struct v4l2_h264_dpb_entry *dpb_entry =
+				&p_h264_dec_params->dpb[i];
+
+			zero_reserved(*dpb_entry);
+		}
 		break;
 
 	case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
index c6cbf178c1c9..a938d16b901c 100644
--- a/include/media/h264-ctrls.h
+++ b/include/media/h264-ctrls.h
@@ -161,7 +161,8 @@ struct v4l2_ctrl_h264_slice_params {
 	/* Offset in bits to slice_data() from the beginning of this slice. */
 	__u32 header_bit_size;
 
-	__u16 first_mb_in_slice;
+	__u32 first_mb_in_slice;
+
 	__u8 slice_type;
 	__u8 pic_parameter_set_id;
 	__u8 colour_plane_id;
@@ -208,7 +209,8 @@ struct v4l2_ctrl_h264_slice_params {
 struct v4l2_h264_dpb_entry {
 	__u64 reference_ts;
 	__u16 frame_num;
-	__u16 pic_num;
+	__u8 reserved[6];
+	__u32 pic_num;
 	/* Note that field is indicated by v4l2_buffer.field */
 	__s32 top_field_order_cnt;
 	__s32 bottom_field_order_cnt;
diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
index bc9ebb560ccf..1a5f26fc2a9a 100644
--- a/include/media/v4l2-h264.h
+++ b/include/media/v4l2-h264.h
@@ -33,7 +33,7 @@ struct v4l2_h264_reflist_builder {
 	struct {
 		s32 pic_order_count;
 		int frame_num;
-		u16 pic_num;
+		u32 pic_num;
 		u16 longterm : 1;
 	} refs[V4L2_H264_NUM_DPB_ENTRIES];
 	s32 cur_pic_order_count;
-- 
2.26.0.rc2

[RFC 5/7] media: uapi: h264: pad v4l2_ctrl_h264_pps to 64-bit

[Ezequiel Garcia]

The struct does not contain 64-bit types, and therefore
doesn't suffer from compatibility issues.

However, having it aligned to 64-bits is cleaner and
has the advantage of allowing future extensions.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 3 +++
 drivers/media/v4l2-core/v4l2-ctrls.c                      | 5 +++++
 include/media/h264-ctrls.h                                | 1 +
 3 files changed, 9 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index e3b5a28fb965..2c682f81aaad 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1662,6 +1662,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __u16
       - ``flags``
       - See :ref:`Picture Parameter Set Flags <h264_pps_flags>`
+    * - __u32
+      - ``reserved``
+      - Applications and drivers must set this to zero.
 
 .. _h264_pps_flags:
 
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index a751c14f9c22..003333b6c7f7 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1735,6 +1735,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
 	struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
 	struct v4l2_ctrl_h264_decode_params *p_h264_dec_params;
+	struct v4l2_ctrl_h264_pps *p_h264_pps;
 	struct v4l2_ctrl_hevc_sps *p_hevc_sps;
 	struct v4l2_ctrl_hevc_pps *p_hevc_pps;
 	struct v4l2_ctrl_hevc_slice_params *p_hevc_slice_params;
@@ -1792,7 +1793,11 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 		break;
 
 	case V4L2_CTRL_TYPE_H264_SPS:
+		break;
 	case V4L2_CTRL_TYPE_H264_PPS:
+		p_h264_pps = p;
+		zero_reserved(*p_h264_pps);
+		break;
 	case V4L2_CTRL_TYPE_H264_SCALING_MATRIX:
 		break;
 	case V4L2_CTRL_TYPE_H264_SLICE_PARAMS:
diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
index a938d16b901c..4119dc4486f3 100644
--- a/include/media/h264-ctrls.h
+++ b/include/media/h264-ctrls.h
@@ -111,6 +111,7 @@ struct v4l2_ctrl_h264_pps {
 	__s8 chroma_qp_index_offset;
 	__s8 second_chroma_qp_index_offset;
 	__u16 flags;
+	__u32 reserved;
 };
 
 struct v4l2_ctrl_h264_scaling_matrix {
-- 
2.26.0.rc2

[RFC 6/7] media: uapi: h264: Clean slice invariants syntax elements

[Ezequiel Garcia]

The H.264 specification requires in its "Slice header semantics"
section that the following values shall be the same in all slice headers:

  pic_parameter_set_id
  frame_num
  field_pic_flag
  bottom_field_flag
  idr_pic_id
  pic_order_cnt_lsb
  delta_pic_order_cnt_bottom
  delta_pic_order_cnt[ 0 ]
  delta_pic_order_cnt[ 1 ]
  sp_for_switch_flag
  slice_group_change_cycle

and can therefore be moved to the per-frame decode parameters control.

Field 'pic_parameter_set_id' is simply removed in this case,
because the PPS control must currently contain the active PPS.

Syntax elements dec_ref_pic_marking() and those related
to pic order count, remain invariant as well, and therefore,
the fields dec_ref_pic_marking_bit_size and pic_order_cnt_bit_size
are also common to all slices.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 .../media/v4l/ext-ctrls-codec.rst             | 84 +++++++++----------
 drivers/media/v4l2-core/v4l2-ctrls.c          |  1 +
 drivers/media/v4l2-core/v4l2-h264.c           |  8 +-
 .../staging/media/hantro/hantro_g1_h264_dec.c | 21 +++--
 drivers/staging/media/hantro/hantro_h264.c    |  3 +-
 drivers/staging/media/rkvdec/rkvdec-h264.c    |  6 +-
 .../staging/media/sunxi/cedrus/cedrus_h264.c  |  9 +-
 include/media/h264-ctrls.h                    | 43 +++++-----
 include/media/v4l2-h264.h                     |  1 -
 9 files changed, 86 insertions(+), 90 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index 2c682f81aaad..24b30ff37d9a 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1781,44 +1781,12 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __u8
       - ``slice_type``
       -
-    * - __u8
-      - ``pic_parameter_set_id``
-      -
     * - __u8
       - ``colour_plane_id``
       -
     * - __u8
       - ``redundant_pic_cnt``
       -
-    * - __u16
-      - ``frame_num``
-      -
-    * - __u16
-      - ``idr_pic_id``
-      -
-    * - __u16
-      - ``pic_order_cnt_lsb``
-      -
-    * - __s32
-      - ``delta_pic_order_cnt_bottom``
-      -
-    * - __s32
-      - ``delta_pic_order_cnt0``
-      -
-    * - __s32
-      - ``delta_pic_order_cnt1``
-      -
-    * - struct :c:type:`v4l2_h264_pred_weight_table`
-      - ``pred_weight_table``
-      -
-    * - __u32
-      - ``dec_ref_pic_marking_bit_size``
-      - Size in bits of the dec_ref_pic_marking() syntax element.
-    * - __u32
-      - ``pic_order_cnt_bit_size``
-      - Combined size in bits of the picture order count related syntax
-        elements: pic_order_cnt_lsb, delta_pic_order_cnt_bottom,
-        delta_pic_order_cnt0, and delta_pic_order_cnt1.
     * - __u8
       - ``cabac_init_idc``
       -
@@ -1845,8 +1813,11 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
       - ``num_ref_idx_l1_active_minus1``
       - If num_ref_idx_active_override_flag is not set, this field must be
         set to the value of num_ref_idx_l1_default_active_minus1.
-    * - __u32
-      - ``slice_group_change_cycle``
+    * - __u8
+      - ``reserved[5]``
+      - Applications and drivers must set this to zero.
+    * - struct :c:type:`v4l2_h264_pred_weight_table`
+      - ``pred_weight_table``
       -
     * - struct :c:type:`v4l2_h264_reference`
       - ``ref_pic_list0[32]``
@@ -1869,17 +1840,11 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     :stub-columns: 0
     :widths:       1 1 2
 
-    * - ``V4L2_H264_SLICE_FLAG_FIELD_PIC``
-      - 0x00000001
-      -
-    * - ``V4L2_H264_SLICE_FLAG_BOTTOM_FIELD``
-      - 0x00000002
-      -
     * - ``V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED``
-      - 0x00000004
+      - 0x00000001
       -
     * - ``V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH``
-      - 0x00000008
+      - 0x00000002
       -
 
 ``Prediction Weight Table``
@@ -2011,6 +1976,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __s32
       - ``bottom_field_order_cnt``
       - Picture Order Count for the coded bottom field
+    * - __u16
+      - ``frame_num``
+      -
+    * - __u16
+      - ``idr_pic_id``
+      -
+    * - __u16
+      - ``pic_order_cnt_lsb``
+      -
+    * - __s32
+      - ``delta_pic_order_cnt_bottom``
+      -
+    * - __s32
+      - ``delta_pic_order_cnt0``
+      -
+    * - __s32
+      - ``delta_pic_order_cnt1``
+      -
+    * - __u32
+      - ``dec_ref_pic_marking_bit_size``
+      - Size in bits of the dec_ref_pic_marking() syntax element.
+    * - __u32
+      - ``pic_order_cnt_bit_size``
+      - Combined size in bits of the picture order count related syntax
+        elements: pic_order_cnt_lsb, delta_pic_order_cnt_bottom,
+        delta_pic_order_cnt0, and delta_pic_order_cnt1.
+    * - __u32
+      - ``slice_group_change_cycle``
+      -
     * - __u32
       - ``flags``
       - See :ref:`Decode Parameters Flags <h264_decode_params_flags>`
@@ -2029,6 +2023,12 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - ``V4L2_H264_DECODE_PARAM_FLAG_IDR_PIC``
       - 0x00000001
       - That picture is an IDR picture
+    * - ``V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC``
+      - 0x00000002
+      -
+    * - ``V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD``
+      - 0x00000004
+      -
 
 .. c:type:: v4l2_h264_dpb_entry
 
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 003333b6c7f7..a21f96c7d077 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1812,6 +1812,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 			zero_reserved(*ref0);
 			zero_reserved(*ref1);
 		}
+		zero_reserved(*p_h264_slice_params);
 		break;
 	case V4L2_CTRL_TYPE_H264_DECODE_PARAMS:
 		p_h264_dec_params = p;
diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
index edf6225f0522..791d24e1b72f 100644
--- a/drivers/media/v4l2-core/v4l2-h264.c
+++ b/drivers/media/v4l2-core/v4l2-h264.c
@@ -18,14 +18,12 @@
  *
  * @b: the builder context to initialize
  * @dec_params: decode parameters control
- * @slice_params: first slice parameters control
  * @sps: SPS control
  * @dpb: DPB to use when creating the reference list
  */
 void
 v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
 		const struct v4l2_ctrl_h264_decode_params *dec_params,
-		const struct v4l2_ctrl_h264_slice_params *slice_params,
 		const struct v4l2_ctrl_h264_sps *sps,
 		const struct v4l2_h264_dpb_entry dpb[V4L2_H264_NUM_DPB_ENTRIES])
 {
@@ -33,13 +31,13 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
 	unsigned int i;
 
 	max_frame_num = 1 << (sps->log2_max_frame_num_minus4 + 4);
-	cur_frame_num = slice_params->frame_num;
+	cur_frame_num = dec_params->frame_num;
 
 	memset(b, 0, sizeof(*b));
-	if (!(slice_params->flags & V4L2_H264_SLICE_FLAG_FIELD_PIC))
+	if (!(dec_params->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC))
 		b->cur_pic_order_count = min(dec_params->bottom_field_order_cnt,
 					     dec_params->top_field_order_cnt);
-	else if (slice_params->flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
+	else if (dec_params->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD)
 		b->cur_pic_order_count = dec_params->bottom_field_order_cnt;
 	else
 		b->cur_pic_order_count = dec_params->top_field_order_cnt;
diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
index 424c648ce9fc..f9839e9c6da5 100644
--- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
+++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
@@ -23,7 +23,6 @@ static void set_params(struct hantro_ctx *ctx)
 {
 	const struct hantro_h264_dec_ctrls *ctrls = &ctx->h264_dec.ctrls;
 	const struct v4l2_ctrl_h264_decode_params *dec_param = ctrls->decode;
-	const struct v4l2_ctrl_h264_slice_params *slices = ctrls->slices;
 	const struct v4l2_ctrl_h264_sps *sps = ctrls->sps;
 	const struct v4l2_ctrl_h264_pps *pps = ctrls->pps;
 	struct vb2_v4l2_buffer *src_buf = hantro_get_src_buf(ctx);
@@ -42,11 +41,11 @@ static void set_params(struct hantro_ctx *ctx)
 
 	if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) &&
 	    (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD ||
-	     slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC))
+	     dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC))
 		reg |= G1_REG_DEC_CTRL0_PIC_INTERLACE_E;
-	if (slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
+	if (dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC)
 		reg |= G1_REG_DEC_CTRL0_PIC_FIELDMODE_E;
-	if (!(slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD))
+	if (!(dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD))
 		reg |= G1_REG_DEC_CTRL0_PIC_TOPFIELD_E;
 	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL0);
 
@@ -75,7 +74,7 @@ static void set_params(struct hantro_ctx *ctx)
 
 	/* Decoder control register 4. */
 	reg = G1_REG_DEC_CTRL4_FRAMENUM_LEN(sps->log2_max_frame_num_minus4 + 4) |
-	      G1_REG_DEC_CTRL4_FRAMENUM(slices[0].frame_num) |
+	      G1_REG_DEC_CTRL4_FRAMENUM(dec_param->frame_num) |
 	      G1_REG_DEC_CTRL4_WEIGHT_BIPR_IDC(pps->weighted_bipred_idc);
 	if (pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE)
 		reg |= G1_REG_DEC_CTRL4_CABAC_E;
@@ -88,8 +87,8 @@ static void set_params(struct hantro_ctx *ctx)
 	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL4);
 
 	/* Decoder control register 5. */
-	reg = G1_REG_DEC_CTRL5_REFPIC_MK_LEN(slices[0].dec_ref_pic_marking_bit_size) |
-	      G1_REG_DEC_CTRL5_IDR_PIC_ID(slices[0].idr_pic_id);
+	reg = G1_REG_DEC_CTRL5_REFPIC_MK_LEN(dec_param->dec_ref_pic_marking_bit_size) |
+	      G1_REG_DEC_CTRL5_IDR_PIC_ID(dec_param->idr_pic_id);
 	if (pps->flags & V4L2_H264_PPS_FLAG_CONSTRAINED_INTRA_PRED)
 		reg |= G1_REG_DEC_CTRL5_CONST_INTRA_E;
 	if (pps->flags & V4L2_H264_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT)
@@ -103,10 +102,10 @@ static void set_params(struct hantro_ctx *ctx)
 	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL5);
 
 	/* Decoder control register 6. */
-	reg = G1_REG_DEC_CTRL6_PPS_ID(slices[0].pic_parameter_set_id) |
+	reg = G1_REG_DEC_CTRL6_PPS_ID(pps->pic_parameter_set_id) |
 	      G1_REG_DEC_CTRL6_REFIDX0_ACTIVE(pps->num_ref_idx_l0_default_active_minus1 + 1) |
 	      G1_REG_DEC_CTRL6_REFIDX1_ACTIVE(pps->num_ref_idx_l1_default_active_minus1 + 1) |
-	      G1_REG_DEC_CTRL6_POC_LENGTH(slices[0].pic_order_cnt_bit_size);
+	      G1_REG_DEC_CTRL6_POC_LENGTH(dec_param->pic_order_cnt_bit_size);
 	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL6);
 
 	/* Error concealment register. */
@@ -246,7 +245,7 @@ static void set_buffers(struct hantro_ctx *ctx)
 	/* Destination (decoded frame) buffer. */
 	dst_dma = hantro_get_dec_buf_addr(ctx, &dst_buf->vb2_buf);
 	/* Adjust dma addr to start at second line for bottom field */
-	if (ctrls->slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
+	if (ctrls->decode->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD)
 		offset = ALIGN(ctx->src_fmt.width, MB_DIM);
 	vdpu_write_relaxed(vpu, dst_dma + offset, G1_REG_ADDR_DST);
 
@@ -265,7 +264,7 @@ static void set_buffers(struct hantro_ctx *ctx)
 		 * DMV buffer is split in two for field encoded frames,
 		 * adjust offset for bottom field
 		 */
-		if (ctrls->slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
+		if (ctrls->decode->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD)
 			offset += 32 * MB_WIDTH(ctx->src_fmt.width) *
 				  MB_HEIGHT(ctx->src_fmt.height);
 		vdpu_write_relaxed(vpu, dst_dma + offset, G1_REG_ADDR_DIR_MV);
diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
index d561f125085a..9890ae7bf3bc 100644
--- a/drivers/staging/media/hantro/hantro_h264.c
+++ b/drivers/staging/media/hantro/hantro_h264.c
@@ -374,8 +374,7 @@ int hantro_h264_dec_prepare_run(struct hantro_ctx *ctx)
 
 	/* Build the P/B{0,1} ref lists. */
 	v4l2_h264_init_reflist_builder(&reflist_builder, ctrls->decode,
-				       &ctrls->slices[0], ctrls->sps,
-				       ctx->h264_dec.dpb);
+				       ctrls->sps, ctx->h264_dec.dpb);
 	v4l2_h264_build_p_ref_list(&reflist_builder, h264_ctx->reflists.p);
 	v4l2_h264_build_b_ref_lists(&reflist_builder, h264_ctx->reflists.b0,
 				    h264_ctx->reflists.b1);
diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 7b66e2743a4f..0f5d519528d8 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -730,7 +730,6 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
 			    struct rkvdec_h264_run *run)
 {
 	const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
-	const struct v4l2_ctrl_h264_slice_params *sl_params = &run->slices_params[0];
 	const struct v4l2_h264_dpb_entry *dpb = dec_params->dpb;
 	struct rkvdec_h264_ctx *h264_ctx = ctx->priv;
 	const struct v4l2_ctrl_h264_sps *sps = run->sps;
@@ -754,7 +753,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
 			continue;
 
 		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM ||
-		    dpb[i].frame_num < sl_params->frame_num) {
+		    dpb[i].frame_num < dec_params->frame_num) {
 			p[i] = dpb[i].frame_num;
 			continue;
 		}
@@ -1093,8 +1092,7 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
 
 	/* Build the P/B{0,1} ref lists. */
 	v4l2_h264_init_reflist_builder(&reflist_builder, run.decode_params,
-				       &run.slices_params[0], run.sps,
-				       run.decode_params->dpb);
+				       run.sps, run.decode_params->dpb);
 	h264_ctx->reflists.num_valid = reflist_builder.num_valid;
 	v4l2_h264_build_p_ref_list(&reflist_builder, h264_ctx->reflists.p);
 	v4l2_h264_build_b_ref_lists(&reflist_builder, h264_ctx->reflists.b0,
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
index cce527bbdf86..e7387315253d 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
@@ -95,7 +95,6 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
 {
 	struct cedrus_h264_sram_ref_pic pic_list[CEDRUS_H264_FRAME_NUM];
 	const struct v4l2_ctrl_h264_decode_params *decode = run->h264.decode_params;
-	const struct v4l2_ctrl_h264_slice_params *slice = run->h264.slice_params;
 	const struct v4l2_ctrl_h264_sps *sps = run->h264.sps;
 	struct vb2_queue *cap_q;
 	struct cedrus_buffer *output_buf;
@@ -144,7 +143,7 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
 	output_buf = vb2_to_cedrus_buffer(&run->dst->vb2_buf);
 	output_buf->codec.h264.position = position;
 
-	if (slice->flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
+	if (decode->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC)
 		output_buf->codec.h264.pic_type = CEDRUS_H264_PIC_TYPE_FIELD;
 	else if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)
 		output_buf->codec.h264.pic_type = CEDRUS_H264_PIC_TYPE_MBAFF;
@@ -414,7 +413,7 @@ static void cedrus_set_params(struct cedrus_ctx *ctx,
 		reg |= VE_H264_SPS_DIRECT_8X8_INFERENCE;
 	cedrus_write(dev, VE_H264_SPS, reg);
 
-	mbaff_pic = !(slice->flags & V4L2_H264_SLICE_FLAG_FIELD_PIC) &&
+	mbaff_pic = !(decode->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC) &&
 		    (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD);
 	pic_width_in_mbs = sps->pic_width_in_mbs_minus1 + 1;
 
@@ -428,9 +427,9 @@ static void cedrus_set_params(struct cedrus_ctx *ctx,
 	reg |= slice->cabac_init_idc & 0x3;
 	if (ctx->fh.m2m_ctx->new_frame)
 		reg |= VE_H264_SHS_FIRST_SLICE_IN_PIC;
-	if (slice->flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
+	if (decode->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC)
 		reg |= VE_H264_SHS_FIELD_PIC;
-	if (slice->flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
+	if (decode->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD)
 		reg |= VE_H264_SHS_BOTTOM_FIELD;
 	if (slice->flags & V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED)
 		reg |= VE_H264_SHS_DIRECT_SPATIAL_MV_PRED;
diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
index 4119dc4486f3..6446ec9f283d 100644
--- a/include/media/h264-ctrls.h
+++ b/include/media/h264-ctrls.h
@@ -138,10 +138,8 @@ struct v4l2_h264_pred_weight_table {
 #define V4L2_H264_SLICE_TYPE_SP				3
 #define V4L2_H264_SLICE_TYPE_SI				4
 
-#define V4L2_H264_SLICE_FLAG_FIELD_PIC			0x01
-#define V4L2_H264_SLICE_FLAG_BOTTOM_FIELD		0x02
-#define V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED	0x04
-#define V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH		0x08
+#define V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED	0x01
+#define V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH		0x02
 
 #define V4L2_H264_REFERENCE_FLAG_TOP_FIELD		0x01
 #define V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD		0x02
@@ -165,22 +163,8 @@ struct v4l2_ctrl_h264_slice_params {
 	__u32 first_mb_in_slice;
 
 	__u8 slice_type;
-	__u8 pic_parameter_set_id;
 	__u8 colour_plane_id;
 	__u8 redundant_pic_cnt;
-	__u16 frame_num;
-	__u16 idr_pic_id;
-	__u16 pic_order_cnt_lsb;
-	__s32 delta_pic_order_cnt_bottom;
-	__s32 delta_pic_order_cnt0;
-	__s32 delta_pic_order_cnt1;
-
-	struct v4l2_h264_pred_weight_table pred_weight_table;
-	/* Size in bits of dec_ref_pic_marking() syntax element. */
-	__u32 dec_ref_pic_marking_bit_size;
-	/* Size in bits of pic order count syntax. */
-	__u32 pic_order_cnt_bit_size;
-
 	__u8 cabac_init_idc;
 	__s8 slice_qp_delta;
 	__s8 slice_qs_delta;
@@ -189,7 +173,9 @@ struct v4l2_ctrl_h264_slice_params {
 	__s8 slice_beta_offset_div2;
 	__u8 num_ref_idx_l0_active_minus1;
 	__u8 num_ref_idx_l1_active_minus1;
-	__u32 slice_group_change_cycle;
+	__u8 reserved[5];
+
+	struct v4l2_h264_pred_weight_table pred_weight_table;
 
 	/*
 	 * Entries on each list are indices into
@@ -218,7 +204,9 @@ struct v4l2_h264_dpb_entry {
 	__u32 flags; /* V4L2_H264_DPB_ENTRY_FLAG_* */
 };
 
-#define V4L2_H264_DECODE_PARAM_FLAG_IDR_PIC	0x01
+#define V4L2_H264_DECODE_PARAM_FLAG_IDR_PIC		0x01
+#define V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC		0x02
+#define V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD	0x04
 
 struct v4l2_ctrl_h264_decode_params {
 	struct v4l2_h264_dpb_entry dpb[V4L2_H264_NUM_DPB_ENTRIES];
@@ -226,6 +214,21 @@ struct v4l2_ctrl_h264_decode_params {
 	__u16 nal_ref_idc;
 	__s32 top_field_order_cnt;
 	__s32 bottom_field_order_cnt;
+
+	__u16 frame_num;
+	__u16 idr_pic_id;
+	__u16 reserved;
+
+	__u16 pic_order_cnt_lsb;
+	__s32 delta_pic_order_cnt_bottom;
+	__s32 delta_pic_order_cnt0;
+	__s32 delta_pic_order_cnt1;
+	/* Size in bits of dec_ref_pic_marking() syntax element. */
+	__u32 dec_ref_pic_marking_bit_size;
+	/* Size in bits of pic order count syntax. */
+	__u32 pic_order_cnt_bit_size;
+	__u32 slice_group_change_cycle;
+
 	__u32 flags; /* V4L2_H264_DECODE_PARAM_FLAG_* */
 };
 
diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
index 1a5f26fc2a9a..f08ba181263d 100644
--- a/include/media/v4l2-h264.h
+++ b/include/media/v4l2-h264.h
@@ -44,7 +44,6 @@ struct v4l2_h264_reflist_builder {
 void
 v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
 		const struct v4l2_ctrl_h264_decode_params *dec_params,
-		const struct v4l2_ctrl_h264_slice_params *slice_params,
 		const struct v4l2_ctrl_h264_sps *sps,
 		const struct v4l2_h264_dpb_entry dpb[V4L2_H264_NUM_DPB_ENTRIES]);
 
-- 
2.26.0.rc2

[RFC 7/7] media: uapi: make H264 stateless codec interface public

[Ezequiel Garcia]

The H264 interface is now ready to be part of the official
public API.

In addition, sanitize header includes.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/staging/media/hantro/hantro_hw.h                  | 5 ++---
 include/media/v4l2-ctrls.h                                | 3 ++-
 include/media/v4l2-h264.h                                 | 2 +-
 .../{media/h264-ctrls.h => uapi/linux/v4l2-h264-ctrls.h}  | 8 ++------
 4 files changed, 7 insertions(+), 11 deletions(-)
 rename include/{media/h264-ctrls.h => uapi/linux/v4l2-h264-ctrls.h} (96%)

diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
index 4053d8710e04..48d5be144319 100644
--- a/drivers/staging/media/hantro/hantro_hw.h
+++ b/drivers/staging/media/hantro/hantro_hw.h
@@ -11,9 +11,8 @@
 
 #include <linux/interrupt.h>
 #include <linux/v4l2-controls.h>
-#include <media/h264-ctrls.h>
-#include <media/mpeg2-ctrls.h>
-#include <media/vp8-ctrls.h>
+
+#include <media/v4l2-ctrls.h>
 #include <media/videobuf2-core.h>
 
 #define DEC_8190_ALIGN_MASK	0x07U
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index f40e2cbb21d3..fc725ba2ebd8 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -13,13 +13,14 @@
 #include <linux/videodev2.h>
 #include <media/media-request.h>
 
+#include <linux/v4l2-h264-ctrls.h>
+
 /*
  * Include the stateless codec compound control definitions.
  * This will move to the public headers once this API is fully stable.
  */
 #include <media/mpeg2-ctrls.h>
 #include <media/fwht-ctrls.h>
-#include <media/h264-ctrls.h>
 #include <media/vp8-ctrls.h>
 #include <media/hevc-ctrls.h>
 
diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
index f08ba181263d..d2314f4d4490 100644
--- a/include/media/v4l2-h264.h
+++ b/include/media/v4l2-h264.h
@@ -10,7 +10,7 @@
 #ifndef _MEDIA_V4L2_H264_H
 #define _MEDIA_V4L2_H264_H
 
-#include <media/h264-ctrls.h>
+#include <media/v4l2-ctrls.h>
 
 /**
  * struct v4l2_h264_reflist_builder - Reference list builder object
diff --git a/include/media/h264-ctrls.h b/include/uapi/linux/v4l2-h264-ctrls.h
similarity index 96%
rename from include/media/h264-ctrls.h
rename to include/uapi/linux/v4l2-h264-ctrls.h
index 6446ec9f283d..a06f60670d68 100644
--- a/include/media/h264-ctrls.h
+++ b/include/uapi/linux/v4l2-h264-ctrls.h
@@ -2,14 +2,10 @@
 /*
  * These are the H.264 state controls for use with stateless H.264
  * codec drivers.
- *
- * It turns out that these structs are not stable yet and will undergo
- * more changes. So keep them private until they are stable and ready to
- * become part of the official public API.
  */
 
-#ifndef _H264_CTRLS_H_
-#define _H264_CTRLS_H_
+#ifndef __LINUX_V4L2_H264_CONTROLS_H
+#define __LINUX_V4L2_H264_CONTROLS_H
 
 #include <linux/videodev2.h>
 
-- 
2.26.0.rc2

Re: [RFC 7/7] media: uapi: make H264 stateless codec interface public

[Hans Verkuil]

On 23/06/2020 20:28, Ezequiel Garcia wrote:
> The H264 interface is now ready to be part of the official
> public API.
> 
> In addition, sanitize header includes.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/staging/media/hantro/hantro_hw.h                  | 5 ++---
>  include/media/v4l2-ctrls.h                                | 3 ++-
>  include/media/v4l2-h264.h                                 | 2 +-
>  .../{media/h264-ctrls.h => uapi/linux/v4l2-h264-ctrls.h}  | 8 ++------
>  4 files changed, 7 insertions(+), 11 deletions(-)
>  rename include/{media/h264-ctrls.h => uapi/linux/v4l2-h264-ctrls.h} (96%)

This isn't quite how this should be done.

The V4L2_PIX_FMT_H264_SLICE define and the V4L2_CTRL_TYPE_H264_* defines should
move to videodev2.h.

The remaining CID defines and the data structures should be moved to v4l2-controls.h.

Yes, I know, v4l2-controls.h is getting large. At some point (could actually be
done in a follow-up patch) the codec controls in v4l2-controls.h should be split off
into their own header (v4l2-codec-controls.h).

One more thing that I was wondering about:

#define V4L2_CID_MPEG_VIDEO_H264_SPS            (V4L2_CID_MPEG_BASE+1000)

These controls are at V4L2_CID_MPEG_BASE+1000. But I was wondering if:

1) wouldn't it be a good thing to use new CID values since this is the actual
   uAPI version? This series changes the layout of several structs, so creating
   new CID values to prevent confusion in applications might be a good idea.

2) related to 1): should we make a new control class for stateless codecs?
   Currently it is mixed in with the stateful codec controls, but I am not so
   sure that that is such a good idea. Creating a separate stateless codec
   control class would be a clean separation of stateful and stateless, and it
   would probably improve the documentation as well.

   The only 'standard' codec control that is used by stateless codecs is
   V4L2_CID_MPEG_VIDEO_H264_PROFILE in hantro, although it is not clear to me
   how it is used. It looks like it is just to report the supported profiles?
   But it isn't present in the cedrus driver, so it's a bit odd.

Thank you for working on finalizing the H264 API.

Regards,

	Hans

> 
> diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> index 4053d8710e04..48d5be144319 100644
> --- a/drivers/staging/media/hantro/hantro_hw.h
> +++ b/drivers/staging/media/hantro/hantro_hw.h
> @@ -11,9 +11,8 @@
>  
>  #include <linux/interrupt.h>
>  #include <linux/v4l2-controls.h>
> -#include <media/h264-ctrls.h>
> -#include <media/mpeg2-ctrls.h>
> -#include <media/vp8-ctrls.h>
> +
> +#include <media/v4l2-ctrls.h>
>  #include <media/videobuf2-core.h>
>  
>  #define DEC_8190_ALIGN_MASK	0x07U
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index f40e2cbb21d3..fc725ba2ebd8 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -13,13 +13,14 @@
>  #include <linux/videodev2.h>
>  #include <media/media-request.h>
>  
> +#include <linux/v4l2-h264-ctrls.h>
> +
>  /*
>   * Include the stateless codec compound control definitions.
>   * This will move to the public headers once this API is fully stable.
>   */
>  #include <media/mpeg2-ctrls.h>
>  #include <media/fwht-ctrls.h>
> -#include <media/h264-ctrls.h>
>  #include <media/vp8-ctrls.h>
>  #include <media/hevc-ctrls.h>
>  
> diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
> index f08ba181263d..d2314f4d4490 100644
> --- a/include/media/v4l2-h264.h
> +++ b/include/media/v4l2-h264.h
> @@ -10,7 +10,7 @@
>  #ifndef _MEDIA_V4L2_H264_H
>  #define _MEDIA_V4L2_H264_H
>  
> -#include <media/h264-ctrls.h>
> +#include <media/v4l2-ctrls.h>
>  
>  /**
>   * struct v4l2_h264_reflist_builder - Reference list builder object
> diff --git a/include/media/h264-ctrls.h b/include/uapi/linux/v4l2-h264-ctrls.h
> similarity index 96%
> rename from include/media/h264-ctrls.h
> rename to include/uapi/linux/v4l2-h264-ctrls.h
> index 6446ec9f283d..a06f60670d68 100644
> --- a/include/media/h264-ctrls.h
> +++ b/include/uapi/linux/v4l2-h264-ctrls.h
> @@ -2,14 +2,10 @@
>  /*
>   * These are the H.264 state controls for use with stateless H.264
>   * codec drivers.
> - *
> - * It turns out that these structs are not stable yet and will undergo
> - * more changes. So keep them private until they are stable and ready to
> - * become part of the official public API.
>   */
>  
> -#ifndef _H264_CTRLS_H_
> -#define _H264_CTRLS_H_
> +#ifndef __LINUX_V4L2_H264_CONTROLS_H
> +#define __LINUX_V4L2_H264_CONTROLS_H
>  
>  #include <linux/videodev2.h>
>  
> 

Re: [RFC 0/7] media: Clean and make H264 stateless uAPI public

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 3511 bytes --]

Le mardi 23 juin 2020 à 15:28 -0300, Ezequiel Garcia a écrit :
> The recent patch posted by Jernej (which I'm including for context),
> encouraged me to address all the known issues in the uAPI.
> 
> I hope we can finally make this uAPI interface
> public; it would be nice to address the other codec
> interfaces so we can move the codec drivers out of staging.
> 
> It should be noted that there is already GStreamer native
> support for this interface, which will be part of 1.18,
> once it's released [1], as well as support in Chromium [2].

Keep in mind, no one did a MultiView and/or SVC implementation in
userspace yet. Multiview seems to share a lot of interlacing, which
ffmpeg implementation covers, but some validation would be nice.

As for SVC, I only started looking at that this means for decoding. It
looks like buffer "holding" mechanism is mostly what we need, I'd would
simply like to verify, as I was told that scaling down might be
required when SVC stream is missing (but perhaps this is optional ?)

> 
> The basic idea here is to sanitize the interface,
> making sure the structs are aligned to 64-bit,
> adding reserved fields for padding where suitable.
> 
> These reserved fields can then be used to support future extensions,
> in case such need appears.
> 
> In addition to this, moving the slice invariant fields
> to the per-frame control, makes the frame-mode driver
> implementation much nicer and the interface; see patch 6/7 for details.
> 
> I'm not adding a MAINTAINERS entry, but I'd like to do so,
> so we make sure any uAPI changes are sent to those involved.
> 
> Another potential change is the addition of a "Since:" tag to the
> control specification, so we can document which kernel version
> added the interface. This might prove useful if reserved
> fields are then used to extend the interface.
> 
> Finally, I'm sneaking here a change from Philipp Zabel
> which apparently fell thru the cracks.

Thanks a lot of working on this.

> 
> [1] https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/tree/master/sys/v4l2codecs
> [2] https://chromium.googlesource.com/chromium/src.git/+/refs/heads/master/media/gpu/v4l2/
> 
> Ezequiel Garcia (5):
>   fixup! media: uapi: h264: update reference lists
>   media: uapi: h264: increase size of fields
>   media: uapi: h264: pad v4l2_ctrl_h264_pps to 64-bit
>   media: uapi: h264: Clean slice invariants syntax elements
>   media: uapi: make H264 stateless codec interface public
> 
> Jernej Skrabec (1):
>   media: uapi: h264: update reference lists
> 
> Philipp Zabel (1):
>   media: uapi: h264: clarify pic_order_cnt_bit_size field
> 
>  .../media/v4l/ext-ctrls-codec.rst             | 135 ++++++++++++------
>  drivers/media/v4l2-core/v4l2-ctrls.c          |  31 ++++
>  drivers/media/v4l2-core/v4l2-h264.c           |   8 +-
>  .../staging/media/hantro/hantro_g1_h264_dec.c |  21 ++-
>  drivers/staging/media/hantro/hantro_h264.c    |   3 +-
>  drivers/staging/media/hantro/hantro_hw.h      |   5 +-
>  drivers/staging/media/rkvdec/rkvdec-h264.c    |   6 +-
>  .../staging/media/sunxi/cedrus/cedrus_h264.c  |  15 +-
>  include/media/v4l2-ctrls.h                    |   3 +-
>  include/media/v4l2-h264.h                     |   5 +-
>  .../linux/v4l2-h264-ctrls.h}                  |  73 ++++++----
>  11 files changed, 194 insertions(+), 111 deletions(-)
>  rename include/{media/h264-ctrls.h => uapi/linux/v4l2-h264-ctrls.h} (88%)
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RFC 2/7] fixup! media: uapi: h264: update reference lists

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 4219 bytes --]

Le mardi 23 juin 2020 à 15:28 -0300, Ezequiel Garcia a écrit :
> Align v4l2_h264_reference to 32-bits, giving some room
> for future extensions.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  .../userspace-api/media/v4l/ext-ctrls-codec.rst  |  3 +++
>  drivers/media/v4l2-core/v4l2-ctrls.c             | 16 ++++++++++++++++
>  include/media/h264-ctrls.h                       |  7 +++++--
>  3 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 6c36d298db20..7af12447a5b0 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1943,6 +1943,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      * - __u8
>        - ``index``
>        -
> +    * - __u32
> +      - ``reserved``
> +      - Applications and drivers must set this to zero.

Is that really appropriate ? There was some effort to keep the controls
small. Also, as these are fixed size, they could be extended with
supplementary C arrays of the same size, set in separate control.

Also, H264 HW is unlikely to evolve, and that covers what DXVA2 and
VAAPI covers already. So it is quite unlikely to ever have to be
extended.

>  
>  .. _h264_reference_flags:
>  
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 3f3fbcd60cc6..6abd023f10c7 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1721,6 +1721,8 @@ static void std_log(const struct v4l2_ctrl *ctrl)
>  
>  #define zero_padding(s) \
>  	memset(&(s).padding, 0, sizeof((s).padding))
> +#define zero_reserved(s) \
> +	memset(&(s).reserved, 0, sizeof((s).reserved))
>  
>  /*
>   * Compound controls validation requires setting unused fields/flags to zero
> @@ -1731,6 +1733,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  {
>  	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
>  	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
> +	struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
>  	struct v4l2_ctrl_hevc_sps *p_hevc_sps;
>  	struct v4l2_ctrl_hevc_pps *p_hevc_pps;
>  	struct v4l2_ctrl_hevc_slice_params *p_hevc_slice_params;
> @@ -1790,7 +1793,20 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  	case V4L2_CTRL_TYPE_H264_SPS:
>  	case V4L2_CTRL_TYPE_H264_PPS:
>  	case V4L2_CTRL_TYPE_H264_SCALING_MATRIX:
> +		break;
>  	case V4L2_CTRL_TYPE_H264_SLICE_PARAMS:
> +		p_h264_slice_params = p;
> +
> +		for (i = 0; i < V4L2_H264_REF_PIC_LIST_LEN; i++) {
> +			struct v4l2_h264_reference *ref0 =
> +				&p_h264_slice_params->ref_pic_list0[i];
> +			struct v4l2_h264_reference *ref1 =
> +				&p_h264_slice_params->ref_pic_list1[i];
> +
> +			zero_reserved(*ref0);
> +			zero_reserved(*ref1);
> +		}
> +		break;
>  	case V4L2_CTRL_TYPE_H264_DECODE_PARAMS:
>  		break;
>  
> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> index 9b1cbc9bc38e..c6cbf178c1c9 100644
> --- a/include/media/h264-ctrls.h
> +++ b/include/media/h264-ctrls.h
> @@ -19,6 +19,8 @@
>   */
>  #define V4L2_H264_NUM_DPB_ENTRIES 16
>  
> +#define V4L2_H264_REF_PIC_LIST_LEN 32
> +
>  /* Our pixel format isn't stable at the moment */
>  #define V4L2_PIX_FMT_H264_SLICE v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices */
>  
> @@ -146,6 +148,7 @@ struct v4l2_h264_pred_weight_table {
>  struct v4l2_h264_reference {
>  	__u8 flags;
>  	__u8 index;
> +	__u16 reserved;
>  };
>  
>  struct v4l2_ctrl_h264_slice_params {
> @@ -190,8 +193,8 @@ struct v4l2_ctrl_h264_slice_params {
>  	 * Entries on each list are indices into
>  	 * v4l2_ctrl_h264_decode_params.dpb[].
>  	 */
> -	struct v4l2_h264_reference ref_pic_list0[32];
> -	struct v4l2_h264_reference ref_pic_list1[32];
> +	struct v4l2_h264_reference ref_pic_list0[V4L2_H264_REF_PIC_LIST_LEN];
> +	struct v4l2_h264_reference ref_pic_list1[V4L2_H264_REF_PIC_LIST_LEN];
>  
>  	__u32 flags;
>  };

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RFC 5/7] media: uapi: h264: pad v4l2_ctrl_h264_pps to 64-bit

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 2831 bytes --]

Le mardi 23 juin 2020 à 15:28 -0300, Ezequiel Garcia a écrit :
> The struct does not contain 64-bit types, and therefore
> doesn't suffer from compatibility issues.
> 
> However, having it aligned to 64-bits is cleaner and
> has the advantage of allowing future extensions.

This one seems a bit random too, it's a final/fixed bitstream syntax.
I'm not certain this is really needed.

> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 3 +++
>  drivers/media/v4l2-core/v4l2-ctrls.c                      | 5 +++++
>  include/media/h264-ctrls.h                                | 1 +
>  3 files changed, 9 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index e3b5a28fb965..2c682f81aaad 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1662,6 +1662,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      * - __u16
>        - ``flags``
>        - See :ref:`Picture Parameter Set Flags <h264_pps_flags>`
> +    * - __u32
> +      - ``reserved``
> +      - Applications and drivers must set this to zero.
>  
>  .. _h264_pps_flags:
>  
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index a751c14f9c22..003333b6c7f7 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1735,6 +1735,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
>  	struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
>  	struct v4l2_ctrl_h264_decode_params *p_h264_dec_params;
> +	struct v4l2_ctrl_h264_pps *p_h264_pps;
>  	struct v4l2_ctrl_hevc_sps *p_hevc_sps;
>  	struct v4l2_ctrl_hevc_pps *p_hevc_pps;
>  	struct v4l2_ctrl_hevc_slice_params *p_hevc_slice_params;
> @@ -1792,7 +1793,11 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  		break;
>  
>  	case V4L2_CTRL_TYPE_H264_SPS:
> +		break;
>  	case V4L2_CTRL_TYPE_H264_PPS:
> +		p_h264_pps = p;
> +		zero_reserved(*p_h264_pps);
> +		break;
>  	case V4L2_CTRL_TYPE_H264_SCALING_MATRIX:
>  		break;
>  	case V4L2_CTRL_TYPE_H264_SLICE_PARAMS:
> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> index a938d16b901c..4119dc4486f3 100644
> --- a/include/media/h264-ctrls.h
> +++ b/include/media/h264-ctrls.h
> @@ -111,6 +111,7 @@ struct v4l2_ctrl_h264_pps {
>  	__s8 chroma_qp_index_offset;
>  	__s8 second_chroma_qp_index_offset;
>  	__u16 flags;
> +	__u32 reserved;
>  };
>  
>  struct v4l2_ctrl_h264_scaling_matrix {

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RFC 4/7] media: uapi: h264: increase size of fields

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 4747 bytes --]

Le mardi 23 juin 2020 à 15:28 -0300, Ezequiel Garcia a écrit :
> Slice header syntax element 'first_mb_in_slice' can point
> to the last macroblock, currently the field can only reference
> 65536 macroblocks which is insufficient for 8K videos.
> 
> DPB entry PicNum maximum value is 2*MaxFrameNum for interlaced
> content (field_pic_flag=1).
> 
> Therefore, increase 'first_mb_in_slice' and 'pic_num'.
> 
> The v4l2_h264_dpb_entry struct needs to be padded to avoid a hole,
> which will be useful to allow future uAPI extensions.
> 
> Note that v4l2_ctrl_h264_slice_params struct will be modified
> in a follow-up commit, and so we defer its 64-bit padding.

This patch includes two changes, with two distinct rationale. Please
split in two, I would also add reference to the spec in the commit
messages. The explanation is also insufficient. Need to mention that
macro-blocks are 16x16, and the bounds for synthetic value
MaxFrameNum (derived from bitstream value) has not be mention either.

> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  .../userspace-api/media/v4l/ext-ctrls-codec.rst          | 7 +++++--
>  drivers/media/v4l2-core/v4l2-ctrls.c                     | 9 +++++++++
>  include/media/h264-ctrls.h                               | 6 ++++--
>  include/media/v4l2-h264.h                                | 2 +-
>  4 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 0808a36777b6..e3b5a28fb965 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1772,7 +1772,7 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      * - __u32
>        - ``header_bit_size``
>        -
> -    * - __u16
> +    * - __u32
>        - ``first_mb_in_slice``
>        -
>      * - __u8
> @@ -2046,7 +2046,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      * - __u16
>        - ``frame_num``
>        -
> -    * - __u16
> +    * - __u8
> +      - ``reserved[6]``
> +      - Applications and drivers must set this to zero.
> +    * - __u32
>        - ``pic_num``
>        -
>      * - __s32
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 6abd023f10c7..a751c14f9c22 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1734,6 +1734,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
>  	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
>  	struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
> +	struct v4l2_ctrl_h264_decode_params *p_h264_dec_params;
>  	struct v4l2_ctrl_hevc_sps *p_hevc_sps;
>  	struct v4l2_ctrl_hevc_pps *p_hevc_pps;
>  	struct v4l2_ctrl_hevc_slice_params *p_hevc_slice_params;
> @@ -1808,6 +1809,14 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  		}
>  		break;
>  	case V4L2_CTRL_TYPE_H264_DECODE_PARAMS:
> +		p_h264_dec_params = p;
> +
> +		for (i = 0; i < V4L2_H264_NUM_DPB_ENTRIES; i++) {
> +			struct v4l2_h264_dpb_entry *dpb_entry =
> +				&p_h264_dec_params->dpb[i];
> +
> +			zero_reserved(*dpb_entry);
> +		}
>  		break;
>  
>  	case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> index c6cbf178c1c9..a938d16b901c 100644
> --- a/include/media/h264-ctrls.h
> +++ b/include/media/h264-ctrls.h
> @@ -161,7 +161,8 @@ struct v4l2_ctrl_h264_slice_params {
>  	/* Offset in bits to slice_data() from the beginning of this slice. */
>  	__u32 header_bit_size;
>  
> -	__u16 first_mb_in_slice;
> +	__u32 first_mb_in_slice;
> +
>  	__u8 slice_type;
>  	__u8 pic_parameter_set_id;
>  	__u8 colour_plane_id;
> @@ -208,7 +209,8 @@ struct v4l2_ctrl_h264_slice_params {
>  struct v4l2_h264_dpb_entry {
>  	__u64 reference_ts;
>  	__u16 frame_num;
> -	__u16 pic_num;
> +	__u8 reserved[6];
> +	__u32 pic_num;
>  	/* Note that field is indicated by v4l2_buffer.field */
>  	__s32 top_field_order_cnt;
>  	__s32 bottom_field_order_cnt;
> diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
> index bc9ebb560ccf..1a5f26fc2a9a 100644
> --- a/include/media/v4l2-h264.h
> +++ b/include/media/v4l2-h264.h
> @@ -33,7 +33,7 @@ struct v4l2_h264_reflist_builder {
>  	struct {
>  		s32 pic_order_count;
>  		int frame_num;
> -		u16 pic_num;
> +		u32 pic_num;
>  		u16 longterm : 1;
>  	} refs[V4L2_H264_NUM_DPB_ENTRIES];
>  	s32 cur_pic_order_count;

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RFC 6/7] media: uapi: h264: Clean slice invariants syntax elements

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 21004 bytes --]

Le mardi 23 juin 2020 à 15:28 -0300, Ezequiel Garcia a écrit :
> The H.264 specification requires in its "Slice header semantics"
> section that the following values shall be the same in all slice headers:
> 
>   pic_parameter_set_id
>   frame_num
>   field_pic_flag
>   bottom_field_flag
>   idr_pic_id
>   pic_order_cnt_lsb
>   delta_pic_order_cnt_bottom
>   delta_pic_order_cnt[ 0 ]
>   delta_pic_order_cnt[ 1 ]
>   sp_for_switch_flag
>   slice_group_change_cycle
> 
> and can therefore be moved to the per-frame decode parameters control.
> 
> Field 'pic_parameter_set_id' is simply removed in this case,
> because the PPS control must currently contain the active PPS.
> 
> Syntax elements dec_ref_pic_marking() and those related
> to pic order count, remain invariant as well, and therefore,
> the fields dec_ref_pic_marking_bit_size and pic_order_cnt_bit_size
> are also common to all slices.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

This looks good to me. Perhaps drivers that don't need the slice_params
(hantro) should simply not implement it ?

Assuming this is/will be thoroughly tested by porting userspace code:

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> ---
>  .../media/v4l/ext-ctrls-codec.rst             | 84 +++++++++----------
>  drivers/media/v4l2-core/v4l2-ctrls.c          |  1 +
>  drivers/media/v4l2-core/v4l2-h264.c           |  8 +-
>  .../staging/media/hantro/hantro_g1_h264_dec.c | 21 +++--
>  drivers/staging/media/hantro/hantro_h264.c    |  3 +-
>  drivers/staging/media/rkvdec/rkvdec-h264.c    |  6 +-
>  .../staging/media/sunxi/cedrus/cedrus_h264.c  |  9 +-
>  include/media/h264-ctrls.h                    | 43 +++++-----
>  include/media/v4l2-h264.h                     |  1 -
>  9 files changed, 86 insertions(+), 90 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 2c682f81aaad..24b30ff37d9a 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1781,44 +1781,12 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      * - __u8
>        - ``slice_type``
>        -
> -    * - __u8
> -      - ``pic_parameter_set_id``
> -      -
>      * - __u8
>        - ``colour_plane_id``
>        -
>      * - __u8
>        - ``redundant_pic_cnt``
>        -
> -    * - __u16
> -      - ``frame_num``
> -      -
> -    * - __u16
> -      - ``idr_pic_id``
> -      -
> -    * - __u16
> -      - ``pic_order_cnt_lsb``
> -      -
> -    * - __s32
> -      - ``delta_pic_order_cnt_bottom``
> -      -
> -    * - __s32
> -      - ``delta_pic_order_cnt0``
> -      -
> -    * - __s32
> -      - ``delta_pic_order_cnt1``
> -      -
> -    * - struct :c:type:`v4l2_h264_pred_weight_table`
> -      - ``pred_weight_table``
> -      -
> -    * - __u32
> -      - ``dec_ref_pic_marking_bit_size``
> -      - Size in bits of the dec_ref_pic_marking() syntax element.
> -    * - __u32
> -      - ``pic_order_cnt_bit_size``
> -      - Combined size in bits of the picture order count related syntax
> -        elements: pic_order_cnt_lsb, delta_pic_order_cnt_bottom,
> -        delta_pic_order_cnt0, and delta_pic_order_cnt1.
>      * - __u8
>        - ``cabac_init_idc``
>        -
> @@ -1845,8 +1813,11 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>        - ``num_ref_idx_l1_active_minus1``
>        - If num_ref_idx_active_override_flag is not set, this field must be
>          set to the value of num_ref_idx_l1_default_active_minus1.
> -    * - __u32
> -      - ``slice_group_change_cycle``
> +    * - __u8
> +      - ``reserved[5]``
> +      - Applications and drivers must set this to zero.
> +    * - struct :c:type:`v4l2_h264_pred_weight_table`
> +      - ``pred_weight_table``
>        -
>      * - struct :c:type:`v4l2_h264_reference`
>        - ``ref_pic_list0[32]``
> @@ -1869,17 +1840,11 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      :stub-columns: 0
>      :widths:       1 1 2
>  
> -    * - ``V4L2_H264_SLICE_FLAG_FIELD_PIC``
> -      - 0x00000001
> -      -
> -    * - ``V4L2_H264_SLICE_FLAG_BOTTOM_FIELD``
> -      - 0x00000002
> -      -
>      * - ``V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED``
> -      - 0x00000004
> +      - 0x00000001
>        -
>      * - ``V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH``
> -      - 0x00000008
> +      - 0x00000002
>        -
>  
>  ``Prediction Weight Table``
> @@ -2011,6 +1976,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      * - __s32
>        - ``bottom_field_order_cnt``
>        - Picture Order Count for the coded bottom field
> +    * - __u16
> +      - ``frame_num``
> +      -
> +    * - __u16
> +      - ``idr_pic_id``
> +      -
> +    * - __u16
> +      - ``pic_order_cnt_lsb``
> +      -
> +    * - __s32
> +      - ``delta_pic_order_cnt_bottom``
> +      -
> +    * - __s32
> +      - ``delta_pic_order_cnt0``
> +      -
> +    * - __s32
> +      - ``delta_pic_order_cnt1``
> +      -
> +    * - __u32
> +      - ``dec_ref_pic_marking_bit_size``
> +      - Size in bits of the dec_ref_pic_marking() syntax element.
> +    * - __u32
> +      - ``pic_order_cnt_bit_size``
> +      - Combined size in bits of the picture order count related syntax
> +        elements: pic_order_cnt_lsb, delta_pic_order_cnt_bottom,
> +        delta_pic_order_cnt0, and delta_pic_order_cnt1.
> +    * - __u32
> +      - ``slice_group_change_cycle``
> +      -
>      * - __u32
>        - ``flags``
>        - See :ref:`Decode Parameters Flags <h264_decode_params_flags>`
> @@ -2029,6 +2023,12 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      * - ``V4L2_H264_DECODE_PARAM_FLAG_IDR_PIC``
>        - 0x00000001
>        - That picture is an IDR picture
> +    * - ``V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC``
> +      - 0x00000002
> +      -
> +    * - ``V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD``
> +      - 0x00000004
> +      -
>  
>  .. c:type:: v4l2_h264_dpb_entry
>  
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 003333b6c7f7..a21f96c7d077 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1812,6 +1812,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  			zero_reserved(*ref0);
>  			zero_reserved(*ref1);
>  		}
> +		zero_reserved(*p_h264_slice_params);
>  		break;
>  	case V4L2_CTRL_TYPE_H264_DECODE_PARAMS:
>  		p_h264_dec_params = p;
> diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
> index edf6225f0522..791d24e1b72f 100644
> --- a/drivers/media/v4l2-core/v4l2-h264.c
> +++ b/drivers/media/v4l2-core/v4l2-h264.c
> @@ -18,14 +18,12 @@
>   *
>   * @b: the builder context to initialize
>   * @dec_params: decode parameters control
> - * @slice_params: first slice parameters control
>   * @sps: SPS control
>   * @dpb: DPB to use when creating the reference list
>   */
>  void
>  v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
>  		const struct v4l2_ctrl_h264_decode_params *dec_params,
> -		const struct v4l2_ctrl_h264_slice_params *slice_params,
>  		const struct v4l2_ctrl_h264_sps *sps,
>  		const struct v4l2_h264_dpb_entry dpb[V4L2_H264_NUM_DPB_ENTRIES])
>  {
> @@ -33,13 +31,13 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
>  	unsigned int i;
>  
>  	max_frame_num = 1 << (sps->log2_max_frame_num_minus4 + 4);
> -	cur_frame_num = slice_params->frame_num;
> +	cur_frame_num = dec_params->frame_num;
>  
>  	memset(b, 0, sizeof(*b));
> -	if (!(slice_params->flags & V4L2_H264_SLICE_FLAG_FIELD_PIC))
> +	if (!(dec_params->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC))
>  		b->cur_pic_order_count = min(dec_params->bottom_field_order_cnt,
>  					     dec_params->top_field_order_cnt);
> -	else if (slice_params->flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
> +	else if (dec_params->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD)
>  		b->cur_pic_order_count = dec_params->bottom_field_order_cnt;
>  	else
>  		b->cur_pic_order_count = dec_params->top_field_order_cnt;
> diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> index 424c648ce9fc..f9839e9c6da5 100644
> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> @@ -23,7 +23,6 @@ static void set_params(struct hantro_ctx *ctx)
>  {
>  	const struct hantro_h264_dec_ctrls *ctrls = &ctx->h264_dec.ctrls;
>  	const struct v4l2_ctrl_h264_decode_params *dec_param = ctrls->decode;
> -	const struct v4l2_ctrl_h264_slice_params *slices = ctrls->slices;
>  	const struct v4l2_ctrl_h264_sps *sps = ctrls->sps;
>  	const struct v4l2_ctrl_h264_pps *pps = ctrls->pps;
>  	struct vb2_v4l2_buffer *src_buf = hantro_get_src_buf(ctx);
> @@ -42,11 +41,11 @@ static void set_params(struct hantro_ctx *ctx)
>  
>  	if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) &&
>  	    (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD ||
> -	     slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC))
> +	     dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC))
>  		reg |= G1_REG_DEC_CTRL0_PIC_INTERLACE_E;
> -	if (slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
> +	if (dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC)
>  		reg |= G1_REG_DEC_CTRL0_PIC_FIELDMODE_E;
> -	if (!(slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD))
> +	if (!(dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD))
>  		reg |= G1_REG_DEC_CTRL0_PIC_TOPFIELD_E;
>  	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL0);
>  
> @@ -75,7 +74,7 @@ static void set_params(struct hantro_ctx *ctx)
>  
>  	/* Decoder control register 4. */
>  	reg = G1_REG_DEC_CTRL4_FRAMENUM_LEN(sps->log2_max_frame_num_minus4 + 4) |
> -	      G1_REG_DEC_CTRL4_FRAMENUM(slices[0].frame_num) |
> +	      G1_REG_DEC_CTRL4_FRAMENUM(dec_param->frame_num) |
>  	      G1_REG_DEC_CTRL4_WEIGHT_BIPR_IDC(pps->weighted_bipred_idc);
>  	if (pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE)
>  		reg |= G1_REG_DEC_CTRL4_CABAC_E;
> @@ -88,8 +87,8 @@ static void set_params(struct hantro_ctx *ctx)
>  	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL4);
>  
>  	/* Decoder control register 5. */
> -	reg = G1_REG_DEC_CTRL5_REFPIC_MK_LEN(slices[0].dec_ref_pic_marking_bit_size) |
> -	      G1_REG_DEC_CTRL5_IDR_PIC_ID(slices[0].idr_pic_id);
> +	reg = G1_REG_DEC_CTRL5_REFPIC_MK_LEN(dec_param->dec_ref_pic_marking_bit_size) |
> +	      G1_REG_DEC_CTRL5_IDR_PIC_ID(dec_param->idr_pic_id);
>  	if (pps->flags & V4L2_H264_PPS_FLAG_CONSTRAINED_INTRA_PRED)
>  		reg |= G1_REG_DEC_CTRL5_CONST_INTRA_E;
>  	if (pps->flags & V4L2_H264_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT)
> @@ -103,10 +102,10 @@ static void set_params(struct hantro_ctx *ctx)
>  	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL5);
>  
>  	/* Decoder control register 6. */
> -	reg = G1_REG_DEC_CTRL6_PPS_ID(slices[0].pic_parameter_set_id) |
> +	reg = G1_REG_DEC_CTRL6_PPS_ID(pps->pic_parameter_set_id) |
>  	      G1_REG_DEC_CTRL6_REFIDX0_ACTIVE(pps->num_ref_idx_l0_default_active_minus1 + 1) |
>  	      G1_REG_DEC_CTRL6_REFIDX1_ACTIVE(pps->num_ref_idx_l1_default_active_minus1 + 1) |
> -	      G1_REG_DEC_CTRL6_POC_LENGTH(slices[0].pic_order_cnt_bit_size);
> +	      G1_REG_DEC_CTRL6_POC_LENGTH(dec_param->pic_order_cnt_bit_size);
>  	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL6);
>  
>  	/* Error concealment register. */
> @@ -246,7 +245,7 @@ static void set_buffers(struct hantro_ctx *ctx)
>  	/* Destination (decoded frame) buffer. */
>  	dst_dma = hantro_get_dec_buf_addr(ctx, &dst_buf->vb2_buf);
>  	/* Adjust dma addr to start at second line for bottom field */
> -	if (ctrls->slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
> +	if (ctrls->decode->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD)
>  		offset = ALIGN(ctx->src_fmt.width, MB_DIM);
>  	vdpu_write_relaxed(vpu, dst_dma + offset, G1_REG_ADDR_DST);
>  
> @@ -265,7 +264,7 @@ static void set_buffers(struct hantro_ctx *ctx)
>  		 * DMV buffer is split in two for field encoded frames,
>  		 * adjust offset for bottom field
>  		 */
> -		if (ctrls->slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
> +		if (ctrls->decode->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD)
>  			offset += 32 * MB_WIDTH(ctx->src_fmt.width) *
>  				  MB_HEIGHT(ctx->src_fmt.height);
>  		vdpu_write_relaxed(vpu, dst_dma + offset, G1_REG_ADDR_DIR_MV);
> diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
> index d561f125085a..9890ae7bf3bc 100644
> --- a/drivers/staging/media/hantro/hantro_h264.c
> +++ b/drivers/staging/media/hantro/hantro_h264.c
> @@ -374,8 +374,7 @@ int hantro_h264_dec_prepare_run(struct hantro_ctx *ctx)
>  
>  	/* Build the P/B{0,1} ref lists. */
>  	v4l2_h264_init_reflist_builder(&reflist_builder, ctrls->decode,
> -				       &ctrls->slices[0], ctrls->sps,
> -				       ctx->h264_dec.dpb);
> +				       ctrls->sps, ctx->h264_dec.dpb);
>  	v4l2_h264_build_p_ref_list(&reflist_builder, h264_ctx->reflists.p);
>  	v4l2_h264_build_b_ref_lists(&reflist_builder, h264_ctx->reflists.b0,
>  				    h264_ctx->reflists.b1);
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index 7b66e2743a4f..0f5d519528d8 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -730,7 +730,6 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
>  			    struct rkvdec_h264_run *run)
>  {
>  	const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
> -	const struct v4l2_ctrl_h264_slice_params *sl_params = &run->slices_params[0];
>  	const struct v4l2_h264_dpb_entry *dpb = dec_params->dpb;
>  	struct rkvdec_h264_ctx *h264_ctx = ctx->priv;
>  	const struct v4l2_ctrl_h264_sps *sps = run->sps;
> @@ -754,7 +753,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
>  			continue;
>  
>  		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM ||
> -		    dpb[i].frame_num < sl_params->frame_num) {
> +		    dpb[i].frame_num < dec_params->frame_num) {
>  			p[i] = dpb[i].frame_num;
>  			continue;
>  		}
> @@ -1093,8 +1092,7 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
>  
>  	/* Build the P/B{0,1} ref lists. */
>  	v4l2_h264_init_reflist_builder(&reflist_builder, run.decode_params,
> -				       &run.slices_params[0], run.sps,
> -				       run.decode_params->dpb);
> +				       run.sps, run.decode_params->dpb);
>  	h264_ctx->reflists.num_valid = reflist_builder.num_valid;
>  	v4l2_h264_build_p_ref_list(&reflist_builder, h264_ctx->reflists.p);
>  	v4l2_h264_build_b_ref_lists(&reflist_builder, h264_ctx->reflists.b0,
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> index cce527bbdf86..e7387315253d 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> @@ -95,7 +95,6 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
>  {
>  	struct cedrus_h264_sram_ref_pic pic_list[CEDRUS_H264_FRAME_NUM];
>  	const struct v4l2_ctrl_h264_decode_params *decode = run->h264.decode_params;
> -	const struct v4l2_ctrl_h264_slice_params *slice = run->h264.slice_params;
>  	const struct v4l2_ctrl_h264_sps *sps = run->h264.sps;
>  	struct vb2_queue *cap_q;
>  	struct cedrus_buffer *output_buf;
> @@ -144,7 +143,7 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
>  	output_buf = vb2_to_cedrus_buffer(&run->dst->vb2_buf);
>  	output_buf->codec.h264.position = position;
>  
> -	if (slice->flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
> +	if (decode->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC)
>  		output_buf->codec.h264.pic_type = CEDRUS_H264_PIC_TYPE_FIELD;
>  	else if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)
>  		output_buf->codec.h264.pic_type = CEDRUS_H264_PIC_TYPE_MBAFF;
> @@ -414,7 +413,7 @@ static void cedrus_set_params(struct cedrus_ctx *ctx,
>  		reg |= VE_H264_SPS_DIRECT_8X8_INFERENCE;
>  	cedrus_write(dev, VE_H264_SPS, reg);
>  
> -	mbaff_pic = !(slice->flags & V4L2_H264_SLICE_FLAG_FIELD_PIC) &&
> +	mbaff_pic = !(decode->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC) &&
>  		    (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD);
>  	pic_width_in_mbs = sps->pic_width_in_mbs_minus1 + 1;
>  
> @@ -428,9 +427,9 @@ static void cedrus_set_params(struct cedrus_ctx *ctx,
>  	reg |= slice->cabac_init_idc & 0x3;
>  	if (ctx->fh.m2m_ctx->new_frame)
>  		reg |= VE_H264_SHS_FIRST_SLICE_IN_PIC;
> -	if (slice->flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
> +	if (decode->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC)
>  		reg |= VE_H264_SHS_FIELD_PIC;
> -	if (slice->flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
> +	if (decode->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD)
>  		reg |= VE_H264_SHS_BOTTOM_FIELD;
>  	if (slice->flags & V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED)
>  		reg |= VE_H264_SHS_DIRECT_SPATIAL_MV_PRED;
> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> index 4119dc4486f3..6446ec9f283d 100644
> --- a/include/media/h264-ctrls.h
> +++ b/include/media/h264-ctrls.h
> @@ -138,10 +138,8 @@ struct v4l2_h264_pred_weight_table {
>  #define V4L2_H264_SLICE_TYPE_SP				3
>  #define V4L2_H264_SLICE_TYPE_SI				4
>  
> -#define V4L2_H264_SLICE_FLAG_FIELD_PIC			0x01
> -#define V4L2_H264_SLICE_FLAG_BOTTOM_FIELD		0x02
> -#define V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED	0x04
> -#define V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH		0x08
> +#define V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED	0x01
> +#define V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH		0x02
>  
>  #define V4L2_H264_REFERENCE_FLAG_TOP_FIELD		0x01
>  #define V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD		0x02
> @@ -165,22 +163,8 @@ struct v4l2_ctrl_h264_slice_params {
>  	__u32 first_mb_in_slice;
>  
>  	__u8 slice_type;
> -	__u8 pic_parameter_set_id;
>  	__u8 colour_plane_id;
>  	__u8 redundant_pic_cnt;
> -	__u16 frame_num;
> -	__u16 idr_pic_id;
> -	__u16 pic_order_cnt_lsb;
> -	__s32 delta_pic_order_cnt_bottom;
> -	__s32 delta_pic_order_cnt0;
> -	__s32 delta_pic_order_cnt1;
> -
> -	struct v4l2_h264_pred_weight_table pred_weight_table;
> -	/* Size in bits of dec_ref_pic_marking() syntax element. */
> -	__u32 dec_ref_pic_marking_bit_size;
> -	/* Size in bits of pic order count syntax. */
> -	__u32 pic_order_cnt_bit_size;
> -
>  	__u8 cabac_init_idc;
>  	__s8 slice_qp_delta;
>  	__s8 slice_qs_delta;
> @@ -189,7 +173,9 @@ struct v4l2_ctrl_h264_slice_params {
>  	__s8 slice_beta_offset_div2;
>  	__u8 num_ref_idx_l0_active_minus1;
>  	__u8 num_ref_idx_l1_active_minus1;
> -	__u32 slice_group_change_cycle;
> +	__u8 reserved[5];
> +
> +	struct v4l2_h264_pred_weight_table pred_weight_table;
>  
>  	/*
>  	 * Entries on each list are indices into
> @@ -218,7 +204,9 @@ struct v4l2_h264_dpb_entry {
>  	__u32 flags; /* V4L2_H264_DPB_ENTRY_FLAG_* */
>  };
>  
> -#define V4L2_H264_DECODE_PARAM_FLAG_IDR_PIC	0x01
> +#define V4L2_H264_DECODE_PARAM_FLAG_IDR_PIC		0x01
> +#define V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC		0x02
> +#define V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD	0x04
>  
>  struct v4l2_ctrl_h264_decode_params {
>  	struct v4l2_h264_dpb_entry dpb[V4L2_H264_NUM_DPB_ENTRIES];
> @@ -226,6 +214,21 @@ struct v4l2_ctrl_h264_decode_params {
>  	__u16 nal_ref_idc;
>  	__s32 top_field_order_cnt;
>  	__s32 bottom_field_order_cnt;
> +
> +	__u16 frame_num;
> +	__u16 idr_pic_id;
> +	__u16 reserved;
> +
> +	__u16 pic_order_cnt_lsb;
> +	__s32 delta_pic_order_cnt_bottom;
> +	__s32 delta_pic_order_cnt0;
> +	__s32 delta_pic_order_cnt1;
> +	/* Size in bits of dec_ref_pic_marking() syntax element. */
> +	__u32 dec_ref_pic_marking_bit_size;
> +	/* Size in bits of pic order count syntax. */
> +	__u32 pic_order_cnt_bit_size;
> +	__u32 slice_group_change_cycle;
> +
>  	__u32 flags; /* V4L2_H264_DECODE_PARAM_FLAG_* */
>  };
>  
> diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
> index 1a5f26fc2a9a..f08ba181263d 100644
> --- a/include/media/v4l2-h264.h
> +++ b/include/media/v4l2-h264.h
> @@ -44,7 +44,6 @@ struct v4l2_h264_reflist_builder {
>  void
>  v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
>  		const struct v4l2_ctrl_h264_decode_params *dec_params,
> -		const struct v4l2_ctrl_h264_slice_params *slice_params,
>  		const struct v4l2_ctrl_h264_sps *sps,
>  		const struct v4l2_h264_dpb_entry dpb[V4L2_H264_NUM_DPB_ENTRIES]);
>  

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RFC 3/7] media: uapi: h264: clarify pic_order_cnt_bit_size field

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 1523 bytes --]

Le mardi 23 juin 2020 à 15:28 -0300, Ezequiel Garcia a écrit :
> From: Philipp Zabel <p.zabel@pengutronix.de>
> 
> Since pic_order_cnt_bit_size is not a syntax element itself, explicitly
> state that it is the total size in bits of the pic_order_cnt_lsb,
> delta_pic_order_cnt_bottom, delta_pic_order_cnt[0], and
> delta_pic_order_cnt[1] syntax elements contained in the slice.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> [Ezequiel: rebase]
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 7af12447a5b0..0808a36777b6 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1813,7 +1813,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>        - Size in bits of the dec_ref_pic_marking() syntax element.
>      * - __u32
>        - ``pic_order_cnt_bit_size``
> -      -
> +      - Combined size in bits of the picture order count related syntax
> +        elements: pic_order_cnt_lsb, delta_pic_order_cnt_bottom,
> +        delta_pic_order_cnt0, and delta_pic_order_cnt1.
>      * - __u8
>        - ``cabac_init_idc``
>        -

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RFC 4/7] media: uapi: h264: increase size of fields

[Ezequiel Garcia]

On Thu, 2020-06-25 at 11:01 -0400, Nicolas Dufresne wrote:
> Le mardi 23 juin 2020 à 15:28 -0300, Ezequiel Garcia a écrit :
> > Slice header syntax element 'first_mb_in_slice' can point
> > to the last macroblock, currently the field can only reference
> > 65536 macroblocks which is insufficient for 8K videos.
> > 
> > DPB entry PicNum maximum value is 2*MaxFrameNum for interlaced
> > content (field_pic_flag=1).
> > 
> > Therefore, increase 'first_mb_in_slice' and 'pic_num'.
> > 
> > The v4l2_h264_dpb_entry struct needs to be padded to avoid a hole,
> > which will be useful to allow future uAPI extensions.
> > 
> > Note that v4l2_ctrl_h264_slice_params struct will be modified
> > in a follow-up commit, and so we defer its 64-bit padding.
> 
> This patch includes two changes, with two distinct rationale. Please
> split in two, I would also add reference to the spec in the commit
> messages.

OK.

> The explanation is also insufficient. Need to mention that
> macro-blocks are 16x16, and the bounds for synthetic value
> MaxFrameNum (derived from bitstream value) has not be mention either.
> 

OK, will do.

Thanks!
Ezequiel

> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  .../userspace-api/media/v4l/ext-ctrls-codec.rst          | 7 +++++--
> >  drivers/media/v4l2-core/v4l2-ctrls.c                     | 9 +++++++++
> >  include/media/h264-ctrls.h                               | 6 ++++--
> >  include/media/v4l2-h264.h                                | 2 +-
> >  4 files changed, 19 insertions(+), 5 deletions(-)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > index 0808a36777b6..e3b5a28fb965 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > @@ -1772,7 +1772,7 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >      * - __u32
> >        - ``header_bit_size``
> >        -
> > -    * - __u16
> > +    * - __u32
> >        - ``first_mb_in_slice``
> >        -
> >      * - __u8
> > @@ -2046,7 +2046,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >      * - __u16
> >        - ``frame_num``
> >        -
> > -    * - __u16
> > +    * - __u8
> > +      - ``reserved[6]``
> > +      - Applications and drivers must set this to zero.
> > +    * - __u32
> >        - ``pic_num``
> >        -
> >      * - __s32
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index 6abd023f10c7..a751c14f9c22 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -1734,6 +1734,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >  	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
> >  	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
> >  	struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
> > +	struct v4l2_ctrl_h264_decode_params *p_h264_dec_params;
> >  	struct v4l2_ctrl_hevc_sps *p_hevc_sps;
> >  	struct v4l2_ctrl_hevc_pps *p_hevc_pps;
> >  	struct v4l2_ctrl_hevc_slice_params *p_hevc_slice_params;
> > @@ -1808,6 +1809,14 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >  		}
> >  		break;
> >  	case V4L2_CTRL_TYPE_H264_DECODE_PARAMS:
> > +		p_h264_dec_params = p;
> > +
> > +		for (i = 0; i < V4L2_H264_NUM_DPB_ENTRIES; i++) {
> > +			struct v4l2_h264_dpb_entry *dpb_entry =
> > +				&p_h264_dec_params->dpb[i];
> > +
> > +			zero_reserved(*dpb_entry);
> > +		}
> >  		break;
> >  
> >  	case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
> > diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> > index c6cbf178c1c9..a938d16b901c 100644
> > --- a/include/media/h264-ctrls.h
> > +++ b/include/media/h264-ctrls.h
> > @@ -161,7 +161,8 @@ struct v4l2_ctrl_h264_slice_params {
> >  	/* Offset in bits to slice_data() from the beginning of this slice. */
> >  	__u32 header_bit_size;
> >  
> > -	__u16 first_mb_in_slice;
> > +	__u32 first_mb_in_slice;
> > +
> >  	__u8 slice_type;
> >  	__u8 pic_parameter_set_id;
> >  	__u8 colour_plane_id;
> > @@ -208,7 +209,8 @@ struct v4l2_ctrl_h264_slice_params {
> >  struct v4l2_h264_dpb_entry {
> >  	__u64 reference_ts;
> >  	__u16 frame_num;
> > -	__u16 pic_num;
> > +	__u8 reserved[6];
> > +	__u32 pic_num;
> >  	/* Note that field is indicated by v4l2_buffer.field */
> >  	__s32 top_field_order_cnt;
> >  	__s32 bottom_field_order_cnt;
> > diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
> > index bc9ebb560ccf..1a5f26fc2a9a 100644
> > --- a/include/media/v4l2-h264.h
> > +++ b/include/media/v4l2-h264.h
> > @@ -33,7 +33,7 @@ struct v4l2_h264_reflist_builder {
> >  	struct {
> >  		s32 pic_order_count;
> >  		int frame_num;
> > -		u16 pic_num;
> > +		u32 pic_num;
> >  		u16 longterm : 1;
> >  	} refs[V4L2_H264_NUM_DPB_ENTRIES];
> >  	s32 cur_pic_order_count;

Re: [RFC 2/7] fixup! media: uapi: h264: update reference lists

[Tomasz Figa]

On Tue, Jun 23, 2020 at 8:29 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> Align v4l2_h264_reference to 32-bits, giving some room
> for future extensions.
>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  .../userspace-api/media/v4l/ext-ctrls-codec.rst  |  3 +++
>  drivers/media/v4l2-core/v4l2-ctrls.c             | 16 ++++++++++++++++
>  include/media/h264-ctrls.h                       |  7 +++++--
>  3 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 6c36d298db20..7af12447a5b0 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1943,6 +1943,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      * - __u8
>        - ``index``
>        -
> +    * - __u32
> +      - ``reserved``
> +      - Applications and drivers must set this to zero.

__u16?

Best regards,
Tomasz

Re: [RFC 2/7] fixup! media: uapi: h264: update reference lists

[Ezequiel Garcia]

On Thu, 2020-06-25 at 17:30 +0200, Tomasz Figa wrote:
> On Tue, Jun 23, 2020 at 8:29 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > Align v4l2_h264_reference to 32-bits, giving some room
> > for future extensions.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  .../userspace-api/media/v4l/ext-ctrls-codec.rst  |  3 +++
> >  drivers/media/v4l2-core/v4l2-ctrls.c             | 16 ++++++++++++++++
> >  include/media/h264-ctrls.h                       |  7 +++++--
> >  3 files changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > index 6c36d298db20..7af12447a5b0 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > @@ -1943,6 +1943,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >      * - __u8
> >        - ``index``
> >        -
> > +    * - __u32
> > +      - ``reserved``
> > +      - Applications and drivers must set this to zero.
> 
> __u16?
> 
> 

Indeed :)

Thanks,
Ezequiel

Re: [RFC 2/7] fixup! media: uapi: h264: update reference lists

[Ezequiel Garcia]

(Adding Jernej, seems I haven't Cced him!)

On Thu, 2020-06-25 at 10:53 -0400, Nicolas Dufresne wrote:
> Le mardi 23 juin 2020 à 15:28 -0300, Ezequiel Garcia a écrit :
> > Align v4l2_h264_reference to 32-bits, giving some room
> > for future extensions.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  .../userspace-api/media/v4l/ext-ctrls-codec.rst  |  3 +++
> >  drivers/media/v4l2-core/v4l2-ctrls.c             | 16 ++++++++++++++++
> >  include/media/h264-ctrls.h                       |  7 +++++--
> >  3 files changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > index 6c36d298db20..7af12447a5b0 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > @@ -1943,6 +1943,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >      * - __u8
> >        - ``index``
> >        -
> > +    * - __u32
> > +      - ``reserved``
> > +      - Applications and drivers must set this to zero.
> 
> Is that really appropriate ? There was some effort to keep the controls
> small. Also, as these are fixed size, they could be extended with
> supplementary C arrays of the same size, set in separate control.
> 

That's a very valid concern.

Currently, each of these v4l2_h264_reference take 64 bytes
(32 x 2 bytes). So it's 128 bytes of references.

Having the reserved field means 256 bytes instead.

Without the reserved field v4l2_ctrl_h264_slice_params is 936 bytes,
the majority of the space taken by v4l2_h264_weight_factors.
So, the reserved field accounts for a small increment there.

Another option would be to split the v4l2_h264_reference lists
to its own control.

However, given the above, it would have more impact to consider
splitting v4l2_h264_pred_weight_table. This table
being used or not depends on the bitstream, so there might be
some value here.

What do you think? 

> Also, H264 HW is unlikely to evolve, and that covers what DXVA2 and
> VAAPI covers already. So it is quite unlikely to ever have to be
> extended.
> 

That may be true indeed. Perhaps we can then agree that references
only need an index and a flag.

Thanks!
Ezequiel

> >  
> >  .. _h264_reference_flags:
> >  
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index 3f3fbcd60cc6..6abd023f10c7 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -1721,6 +1721,8 @@ static void std_log(const struct v4l2_ctrl *ctrl)
> >  
> >  #define zero_padding(s) \
> >  	memset(&(s).padding, 0, sizeof((s).padding))
> > +#define zero_reserved(s) \
> > +	memset(&(s).reserved, 0, sizeof((s).reserved))
> >  
> >  /*
> >   * Compound controls validation requires setting unused fields/flags to zero
> > @@ -1731,6 +1733,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >  {
> >  	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
> >  	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
> > +	struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
> >  	struct v4l2_ctrl_hevc_sps *p_hevc_sps;
> >  	struct v4l2_ctrl_hevc_pps *p_hevc_pps;
> >  	struct v4l2_ctrl_hevc_slice_params *p_hevc_slice_params;
> > @@ -1790,7 +1793,20 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >  	case V4L2_CTRL_TYPE_H264_SPS:
> >  	case V4L2_CTRL_TYPE_H264_PPS:
> >  	case V4L2_CTRL_TYPE_H264_SCALING_MATRIX:
> > +		break;
> >  	case V4L2_CTRL_TYPE_H264_SLICE_PARAMS:
> > +		p_h264_slice_params = p;
> > +
> > +		for (i = 0; i < V4L2_H264_REF_PIC_LIST_LEN; i++) {
> > +			struct v4l2_h264_reference *ref0 =
> > +				&p_h264_slice_params->ref_pic_list0[i];
> > +			struct v4l2_h264_reference *ref1 =
> > +				&p_h264_slice_params->ref_pic_list1[i];
> > +
> > +			zero_reserved(*ref0);
> > +			zero_reserved(*ref1);
> > +		}
> > +		break;
> >  	case V4L2_CTRL_TYPE_H264_DECODE_PARAMS:
> >  		break;
> >  
> > diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> > index 9b1cbc9bc38e..c6cbf178c1c9 100644
> > --- a/include/media/h264-ctrls.h
> > +++ b/include/media/h264-ctrls.h
> > @@ -19,6 +19,8 @@
> >   */
> >  #define V4L2_H264_NUM_DPB_ENTRIES 16
> >  
> > +#define V4L2_H264_REF_PIC_LIST_LEN 32
> > +
> >  /* Our pixel format isn't stable at the moment */
> >  #define V4L2_PIX_FMT_H264_SLICE v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices */
> >  
> > @@ -146,6 +148,7 @@ struct v4l2_h264_pred_weight_table {
> >  struct v4l2_h264_reference {
> >  	__u8 flags;
> >  	__u8 index;
> > +	__u16 reserved;
> >  };
> >  
> >  struct v4l2_ctrl_h264_slice_params {
> > @@ -190,8 +193,8 @@ struct v4l2_ctrl_h264_slice_params {
> >  	 * Entries on each list are indices into
> >  	 * v4l2_ctrl_h264_decode_params.dpb[].
> >  	 */
> > -	struct v4l2_h264_reference ref_pic_list0[32];
> > -	struct v4l2_h264_reference ref_pic_list1[32];
> > +	struct v4l2_h264_reference ref_pic_list0[V4L2_H264_REF_PIC_LIST_LEN];
> > +	struct v4l2_h264_reference ref_pic_list1[V4L2_H264_REF_PIC_LIST_LEN];
> >  
> >  	__u32 flags;
> >  };

Re: [RFC 2/7] fixup! media: uapi: h264: update reference lists

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 6143 bytes --]

Le jeudi 25 juin 2020 à 14:42 -0300, Ezequiel Garcia a écrit :
> (Adding Jernej, seems I haven't Cced him!)
> 
> On Thu, 2020-06-25 at 10:53 -0400, Nicolas Dufresne wrote:
> > Le mardi 23 juin 2020 à 15:28 -0300, Ezequiel Garcia a écrit :
> > > Align v4l2_h264_reference to 32-bits, giving some room
> > > for future extensions.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > ---
> > >  .../userspace-api/media/v4l/ext-ctrls-codec.rst  |  3 +++
> > >  drivers/media/v4l2-core/v4l2-ctrls.c             | 16 ++++++++++++++++
> > >  include/media/h264-ctrls.h                       |  7 +++++--
> > >  3 files changed, 24 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > index 6c36d298db20..7af12447a5b0 100644
> > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > @@ -1943,6 +1943,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > >      * - __u8
> > >        - ``index``
> > >        -
> > > +    * - __u32
> > > +      - ``reserved``
> > > +      - Applications and drivers must set this to zero.
> > 
> > Is that really appropriate ? There was some effort to keep the controls
> > small. Also, as these are fixed size, they could be extended with
> > supplementary C arrays of the same size, set in separate control.
> > 
> 
> That's a very valid concern.
> 
> Currently, each of these v4l2_h264_reference take 64 bytes
> (32 x 2 bytes). So it's 128 bytes of references.
> 
> Having the reserved field means 256 bytes instead.
> 
> Without the reserved field v4l2_ctrl_h264_slice_params is 936 bytes,
> the majority of the space taken by v4l2_h264_weight_factors.
> So, the reserved field accounts for a small increment there.
> 
> Another option would be to split the v4l2_h264_reference lists
> to its own control.
> 
> However, given the above, it would have more impact to consider
> splitting v4l2_h264_pred_weight_table. This table
> being used or not depends on the bitstream, so there might be
> some value here.
> 
> What do you think? 

This seems like a good idea, making the weight table it's own control,
so when the it's not specified in the syntax, we could skip.

> 
> > Also, H264 HW is unlikely to evolve, and that covers what DXVA2 and
> > VAAPI covers already. So it is quite unlikely to ever have to be
> > extended.
> > 
> 
> That may be true indeed. Perhaps we can then agree that references
> only need an index and a flag.

I also see that MVC was never implemented in FFMPEG, I was quite
surprised. Only left eye playback is there, same in gstreamer-vaapi. It
leaves me thinking that all this complexity wasn't wanted. A lot of
stereo movie just make a larger, or higher image to fit the views.

> 
> Thanks!
> Ezequiel
> 
> > >  
> > >  .. _h264_reference_flags:
> > >  
> > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > index 3f3fbcd60cc6..6abd023f10c7 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > @@ -1721,6 +1721,8 @@ static void std_log(const struct v4l2_ctrl *ctrl)
> > >  
> > >  #define zero_padding(s) \
> > >  	memset(&(s).padding, 0, sizeof((s).padding))
> > > +#define zero_reserved(s) \
> > > +	memset(&(s).reserved, 0, sizeof((s).reserved))
> > >  
> > >  /*
> > >   * Compound controls validation requires setting unused fields/flags to zero
> > > @@ -1731,6 +1733,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> > >  {
> > >  	struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
> > >  	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
> > > +	struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
> > >  	struct v4l2_ctrl_hevc_sps *p_hevc_sps;
> > >  	struct v4l2_ctrl_hevc_pps *p_hevc_pps;
> > >  	struct v4l2_ctrl_hevc_slice_params *p_hevc_slice_params;
> > > @@ -1790,7 +1793,20 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> > >  	case V4L2_CTRL_TYPE_H264_SPS:
> > >  	case V4L2_CTRL_TYPE_H264_PPS:
> > >  	case V4L2_CTRL_TYPE_H264_SCALING_MATRIX:
> > > +		break;
> > >  	case V4L2_CTRL_TYPE_H264_SLICE_PARAMS:
> > > +		p_h264_slice_params = p;
> > > +
> > > +		for (i = 0; i < V4L2_H264_REF_PIC_LIST_LEN; i++) {
> > > +			struct v4l2_h264_reference *ref0 =
> > > +				&p_h264_slice_params->ref_pic_list0[i];
> > > +			struct v4l2_h264_reference *ref1 =
> > > +				&p_h264_slice_params->ref_pic_list1[i];
> > > +
> > > +			zero_reserved(*ref0);
> > > +			zero_reserved(*ref1);
> > > +		}
> > > +		break;
> > >  	case V4L2_CTRL_TYPE_H264_DECODE_PARAMS:
> > >  		break;
> > >  
> > > diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> > > index 9b1cbc9bc38e..c6cbf178c1c9 100644
> > > --- a/include/media/h264-ctrls.h
> > > +++ b/include/media/h264-ctrls.h
> > > @@ -19,6 +19,8 @@
> > >   */
> > >  #define V4L2_H264_NUM_DPB_ENTRIES 16
> > >  
> > > +#define V4L2_H264_REF_PIC_LIST_LEN 32
> > > +
> > >  /* Our pixel format isn't stable at the moment */
> > >  #define V4L2_PIX_FMT_H264_SLICE v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices */
> > >  
> > > @@ -146,6 +148,7 @@ struct v4l2_h264_pred_weight_table {
> > >  struct v4l2_h264_reference {
> > >  	__u8 flags;
> > >  	__u8 index;
> > > +	__u16 reserved;
> > >  };
> > >  
> > >  struct v4l2_ctrl_h264_slice_params {
> > > @@ -190,8 +193,8 @@ struct v4l2_ctrl_h264_slice_params {
> > >  	 * Entries on each list are indices into
> > >  	 * v4l2_ctrl_h264_decode_params.dpb[].
> > >  	 */
> > > -	struct v4l2_h264_reference ref_pic_list0[32];
> > > -	struct v4l2_h264_reference ref_pic_list1[32];
> > > +	struct v4l2_h264_reference ref_pic_list0[V4L2_H264_REF_PIC_LIST_LEN];
> > > +	struct v4l2_h264_reference ref_pic_list1[V4L2_H264_REF_PIC_LIST_LEN];
> > >  
> > >  	__u32 flags;
> > >  };
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RFC 7/7] media: uapi: make H264 stateless codec interface public

[Ezequiel Garcia]

On Thu, 2020-06-25 at 09:52 +0200, Hans Verkuil wrote:
> On 23/06/2020 20:28, Ezequiel Garcia wrote:
> > The H264 interface is now ready to be part of the official
> > public API.
> > 
> > In addition, sanitize header includes.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/staging/media/hantro/hantro_hw.h                  | 5 ++---
> >  include/media/v4l2-ctrls.h                                | 3 ++-
> >  include/media/v4l2-h264.h                                 | 2 +-
> >  .../{media/h264-ctrls.h => uapi/linux/v4l2-h264-ctrls.h}  | 8 ++------
> >  4 files changed, 7 insertions(+), 11 deletions(-)
> >  rename include/{media/h264-ctrls.h => uapi/linux/v4l2-h264-ctrls.h} (96%)
> 
> This isn't quite how this should be done.
> 
> The V4L2_PIX_FMT_H264_SLICE define and the V4L2_CTRL_TYPE_H264_* defines should
> move to videodev2.h.
> 

OK.

> The remaining CID defines and the data structures should be moved to v4l2-controls.h.
> 

OK.

> Yes, I know, v4l2-controls.h is getting large. At some point (could actually be
> done in a follow-up patch) the codec controls in v4l2-controls.h should be split off
> into their own header (v4l2-codec-controls.h).
> 

OK, that makes sense.

> One more thing that I was wondering about:
> 
> #define V4L2_CID_MPEG_VIDEO_H264_SPS            (V4L2_CID_MPEG_BASE+1000)
> 
> These controls are at V4L2_CID_MPEG_BASE+1000. But I was wondering if:
> 
> 1) wouldn't it be a good thing to use new CID values since this is the actual
>    uAPI version? This series changes the layout of several structs, so creating
>    new CID values to prevent confusion in applications might be a good idea.
> 
> 2) related to 1): should we make a new control class for stateless codecs?
>    Currently it is mixed in with the stateful codec controls, but I am not so
>    sure that that is such a good idea. Creating a separate stateless codec
>    control class would be a clean separation of stateful and stateless, and it
>    would probably improve the documentation as well.
> 

Good idea.

>    The only 'standard' codec control that is used by stateless codecs is
>    V4L2_CID_MPEG_VIDEO_H264_PROFILE in hantro, although it is not clear to me
>    how it is used. It looks like it is just to report the supported profiles?
>    But it isn't present in the cedrus driver, so it's a bit odd.
> 

I can take a look and add profiles to cedrus as well.

> Thank you for working on finalizing the H264 API.
> 

Thanks for the guidelines and feedback.

I will drop this patch for now, since I think we
now have a clear guideline on how to go public
(which was the goal of this RFC!).

I will move forward the H264 uAPI changes,
and then we can try to push H264, MPEG-2 and VP8
public, as these interfaces are the ones
that seem stable.

Thanks!
Ezequiel  

> Regards,
> 
> 	Hans
> 
> > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> > index 4053d8710e04..48d5be144319 100644
> > --- a/drivers/staging/media/hantro/hantro_hw.h
> > +++ b/drivers/staging/media/hantro/hantro_hw.h
> > @@ -11,9 +11,8 @@
> >  
> >  #include <linux/interrupt.h>
> >  #include <linux/v4l2-controls.h>
> > -#include <media/h264-ctrls.h>
> > -#include <media/mpeg2-ctrls.h>
> > -#include <media/vp8-ctrls.h>
> > +
> > +#include <media/v4l2-ctrls.h>
> >  #include <media/videobuf2-core.h>
> >  
> >  #define DEC_8190_ALIGN_MASK	0x07U
> > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> > index f40e2cbb21d3..fc725ba2ebd8 100644
> > --- a/include/media/v4l2-ctrls.h
> > +++ b/include/media/v4l2-ctrls.h
> > @@ -13,13 +13,14 @@
> >  #include <linux/videodev2.h>
> >  #include <media/media-request.h>
> >  
> > +#include <linux/v4l2-h264-ctrls.h>
> > +
> >  /*
> >   * Include the stateless codec compound control definitions.
> >   * This will move to the public headers once this API is fully stable.
> >   */
> >  #include <media/mpeg2-ctrls.h>
> >  #include <media/fwht-ctrls.h>
> > -#include <media/h264-ctrls.h>
> >  #include <media/vp8-ctrls.h>
> >  #include <media/hevc-ctrls.h>
> >  
> > diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
> > index f08ba181263d..d2314f4d4490 100644
> > --- a/include/media/v4l2-h264.h
> > +++ b/include/media/v4l2-h264.h
> > @@ -10,7 +10,7 @@
> >  #ifndef _MEDIA_V4L2_H264_H
> >  #define _MEDIA_V4L2_H264_H
> >  
> > -#include <media/h264-ctrls.h>
> > +#include <media/v4l2-ctrls.h>
> >  
> >  /**
> >   * struct v4l2_h264_reflist_builder - Reference list builder object
> > diff --git a/include/media/h264-ctrls.h b/include/uapi/linux/v4l2-h264-ctrls.h
> > similarity index 96%
> > rename from include/media/h264-ctrls.h
> > rename to include/uapi/linux/v4l2-h264-ctrls.h
> > index 6446ec9f283d..a06f60670d68 100644
> > --- a/include/media/h264-ctrls.h
> > +++ b/include/uapi/linux/v4l2-h264-ctrls.h
> > @@ -2,14 +2,10 @@
> >  /*
> >   * These are the H.264 state controls for use with stateless H.264
> >   * codec drivers.
> > - *
> > - * It turns out that these structs are not stable yet and will undergo
> > - * more changes. So keep them private until they are stable and ready to
> > - * become part of the official public API.
> >   */
> >  
> > -#ifndef _H264_CTRLS_H_
> > -#define _H264_CTRLS_H_
> > +#ifndef __LINUX_V4L2_H264_CONTROLS_H
> > +#define __LINUX_V4L2_H264_CONTROLS_H
> >  
> >  #include <linux/videodev2.h>
> >  
> > 

Re: [RFC 6/7] media: uapi: h264: Clean slice invariants syntax elements

[Ezequiel Garcia]

On Thu, 2020-06-25 at 11:05 -0400, Nicolas Dufresne wrote:
> Le mardi 23 juin 2020 à 15:28 -0300, Ezequiel Garcia a écrit :
> > The H.264 specification requires in its "Slice header semantics"
> > section that the following values shall be the same in all slice headers:
> > 
> >   pic_parameter_set_id
> >   frame_num
> >   field_pic_flag
> >   bottom_field_flag
> >   idr_pic_id
> >   pic_order_cnt_lsb
> >   delta_pic_order_cnt_bottom
> >   delta_pic_order_cnt[ 0 ]
> >   delta_pic_order_cnt[ 1 ]
> >   sp_for_switch_flag
> >   slice_group_change_cycle
> > 
> > and can therefore be moved to the per-frame decode parameters control.
> > 
> > Field 'pic_parameter_set_id' is simply removed in this case,
> > because the PPS control must currently contain the active PPS.
> > 
> > Syntax elements dec_ref_pic_marking() and those related
> > to pic order count, remain invariant as well, and therefore,
> > the fields dec_ref_pic_marking_bit_size and pic_order_cnt_bit_size
> > are also common to all slices.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> 
> This looks good to me. Perhaps drivers that don't need the slice_params
> (hantro) should simply not implement it ?
> 

Yes, but let's keep that out of this series.

> Assuming this is/will be thoroughly tested by porting userspace code:
> 

Of course!
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 


Thanks,
Ezequiel

> > ---
> >  .../media/v4l/ext-ctrls-codec.rst             | 84 +++++++++----------
> >  drivers/media/v4l2-core/v4l2-ctrls.c          |  1 +
> >  drivers/media/v4l2-core/v4l2-h264.c           |  8 +-
> >  .../staging/media/hantro/hantro_g1_h264_dec.c | 21 +++--
> >  drivers/staging/media/hantro/hantro_h264.c    |  3 +-
> >  drivers/staging/media/rkvdec/rkvdec-h264.c    |  6 +-
> >  .../staging/media/sunxi/cedrus/cedrus_h264.c  |  9 +-
> >  include/media/h264-ctrls.h                    | 43 +++++-----
> >  include/media/v4l2-h264.h                     |  1 -
> >  9 files changed, 86 insertions(+), 90 deletions(-)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > index 2c682f81aaad..24b30ff37d9a 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > @@ -1781,44 +1781,12 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >      * - __u8
> >        - ``slice_type``
> >        -
> > -    * - __u8
> > -      - ``pic_parameter_set_id``
> > -      -
> >      * - __u8
> >        - ``colour_plane_id``
> >        -
> >      * - __u8
> >        - ``redundant_pic_cnt``
> >        -
> > -    * - __u16
> > -      - ``frame_num``
> > -      -
> > -    * - __u16
> > -      - ``idr_pic_id``
> > -      -
> > -    * - __u16
> > -      - ``pic_order_cnt_lsb``
> > -      -
> > -    * - __s32
> > -      - ``delta_pic_order_cnt_bottom``
> > -      -
> > -    * - __s32
> > -      - ``delta_pic_order_cnt0``
> > -      -
> > -    * - __s32
> > -      - ``delta_pic_order_cnt1``
> > -      -
> > -    * - struct :c:type:`v4l2_h264_pred_weight_table`
> > -      - ``pred_weight_table``
> > -      -
> > -    * - __u32
> > -      - ``dec_ref_pic_marking_bit_size``
> > -      - Size in bits of the dec_ref_pic_marking() syntax element.
> > -    * - __u32
> > -      - ``pic_order_cnt_bit_size``
> > -      - Combined size in bits of the picture order count related syntax
> > -        elements: pic_order_cnt_lsb, delta_pic_order_cnt_bottom,
> > -        delta_pic_order_cnt0, and delta_pic_order_cnt1.
> >      * - __u8
> >        - ``cabac_init_idc``
> >        -
> > @@ -1845,8 +1813,11 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >        - ``num_ref_idx_l1_active_minus1``
> >        - If num_ref_idx_active_override_flag is not set, this field must be
> >          set to the value of num_ref_idx_l1_default_active_minus1.
> > -    * - __u32
> > -      - ``slice_group_change_cycle``
> > +    * - __u8
> > +      - ``reserved[5]``
> > +      - Applications and drivers must set this to zero.
> > +    * - struct :c:type:`v4l2_h264_pred_weight_table`
> > +      - ``pred_weight_table``
> >        -
> >      * - struct :c:type:`v4l2_h264_reference`
> >        - ``ref_pic_list0[32]``
> > @@ -1869,17 +1840,11 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >      :stub-columns: 0
> >      :widths:       1 1 2
> >  
> > -    * - ``V4L2_H264_SLICE_FLAG_FIELD_PIC``
> > -      - 0x00000001
> > -      -
> > -    * - ``V4L2_H264_SLICE_FLAG_BOTTOM_FIELD``
> > -      - 0x00000002
> > -      -
> >      * - ``V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED``
> > -      - 0x00000004
> > +      - 0x00000001
> >        -
> >      * - ``V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH``
> > -      - 0x00000008
> > +      - 0x00000002
> >        -
> >  
> >  ``Prediction Weight Table``
> > @@ -2011,6 +1976,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >      * - __s32
> >        - ``bottom_field_order_cnt``
> >        - Picture Order Count for the coded bottom field
> > +    * - __u16
> > +      - ``frame_num``
> > +      -
> > +    * - __u16
> > +      - ``idr_pic_id``
> > +      -
> > +    * - __u16
> > +      - ``pic_order_cnt_lsb``
> > +      -
> > +    * - __s32
> > +      - ``delta_pic_order_cnt_bottom``
> > +      -
> > +    * - __s32
> > +      - ``delta_pic_order_cnt0``
> > +      -
> > +    * - __s32
> > +      - ``delta_pic_order_cnt1``
> > +      -
> > +    * - __u32
> > +      - ``dec_ref_pic_marking_bit_size``
> > +      - Size in bits of the dec_ref_pic_marking() syntax element.
> > +    * - __u32
> > +      - ``pic_order_cnt_bit_size``
> > +      - Combined size in bits of the picture order count related syntax
> > +        elements: pic_order_cnt_lsb, delta_pic_order_cnt_bottom,
> > +        delta_pic_order_cnt0, and delta_pic_order_cnt1.
> > +    * - __u32
> > +      - ``slice_group_change_cycle``
> > +      -
> >      * - __u32
> >        - ``flags``
> >        - See :ref:`Decode Parameters Flags <h264_decode_params_flags>`
> > @@ -2029,6 +2023,12 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >      * - ``V4L2_H264_DECODE_PARAM_FLAG_IDR_PIC``
> >        - 0x00000001
> >        - That picture is an IDR picture
> > +    * - ``V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC``
> > +      - 0x00000002
> > +      -
> > +    * - ``V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD``
> > +      - 0x00000004
> > +      -
> >  
> >  .. c:type:: v4l2_h264_dpb_entry
> >  
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index 003333b6c7f7..a21f96c7d077 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -1812,6 +1812,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >  			zero_reserved(*ref0);
> >  			zero_reserved(*ref1);
> >  		}
> > +		zero_reserved(*p_h264_slice_params);
> >  		break;
> >  	case V4L2_CTRL_TYPE_H264_DECODE_PARAMS:
> >  		p_h264_dec_params = p;
> > diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
> > index edf6225f0522..791d24e1b72f 100644
> > --- a/drivers/media/v4l2-core/v4l2-h264.c
> > +++ b/drivers/media/v4l2-core/v4l2-h264.c
> > @@ -18,14 +18,12 @@
> >   *
> >   * @b: the builder context to initialize
> >   * @dec_params: decode parameters control
> > - * @slice_params: first slice parameters control
> >   * @sps: SPS control
> >   * @dpb: DPB to use when creating the reference list
> >   */
> >  void
> >  v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
> >  		const struct v4l2_ctrl_h264_decode_params *dec_params,
> > -		const struct v4l2_ctrl_h264_slice_params *slice_params,
> >  		const struct v4l2_ctrl_h264_sps *sps,
> >  		const struct v4l2_h264_dpb_entry dpb[V4L2_H264_NUM_DPB_ENTRIES])
> >  {
> > @@ -33,13 +31,13 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
> >  	unsigned int i;
> >  
> >  	max_frame_num = 1 << (sps->log2_max_frame_num_minus4 + 4);
> > -	cur_frame_num = slice_params->frame_num;
> > +	cur_frame_num = dec_params->frame_num;
> >  
> >  	memset(b, 0, sizeof(*b));
> > -	if (!(slice_params->flags & V4L2_H264_SLICE_FLAG_FIELD_PIC))
> > +	if (!(dec_params->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC))
> >  		b->cur_pic_order_count = min(dec_params->bottom_field_order_cnt,
> >  					     dec_params->top_field_order_cnt);
> > -	else if (slice_params->flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
> > +	else if (dec_params->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD)
> >  		b->cur_pic_order_count = dec_params->bottom_field_order_cnt;
> >  	else
> >  		b->cur_pic_order_count = dec_params->top_field_order_cnt;
> > diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> > index 424c648ce9fc..f9839e9c6da5 100644
> > --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> > +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> > @@ -23,7 +23,6 @@ static void set_params(struct hantro_ctx *ctx)
> >  {
> >  	const struct hantro_h264_dec_ctrls *ctrls = &ctx->h264_dec.ctrls;
> >  	const struct v4l2_ctrl_h264_decode_params *dec_param = ctrls->decode;
> > -	const struct v4l2_ctrl_h264_slice_params *slices = ctrls->slices;
> >  	const struct v4l2_ctrl_h264_sps *sps = ctrls->sps;
> >  	const struct v4l2_ctrl_h264_pps *pps = ctrls->pps;
> >  	struct vb2_v4l2_buffer *src_buf = hantro_get_src_buf(ctx);
> > @@ -42,11 +41,11 @@ static void set_params(struct hantro_ctx *ctx)
> >  
> >  	if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) &&
> >  	    (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD ||
> > -	     slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC))
> > +	     dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC))
> >  		reg |= G1_REG_DEC_CTRL0_PIC_INTERLACE_E;
> > -	if (slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
> > +	if (dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC)
> >  		reg |= G1_REG_DEC_CTRL0_PIC_FIELDMODE_E;
> > -	if (!(slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD))
> > +	if (!(dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD))
> >  		reg |= G1_REG_DEC_CTRL0_PIC_TOPFIELD_E;
> >  	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL0);
> >  
> > @@ -75,7 +74,7 @@ static void set_params(struct hantro_ctx *ctx)
> >  
> >  	/* Decoder control register 4. */
> >  	reg = G1_REG_DEC_CTRL4_FRAMENUM_LEN(sps->log2_max_frame_num_minus4 + 4) |
> > -	      G1_REG_DEC_CTRL4_FRAMENUM(slices[0].frame_num) |
> > +	      G1_REG_DEC_CTRL4_FRAMENUM(dec_param->frame_num) |
> >  	      G1_REG_DEC_CTRL4_WEIGHT_BIPR_IDC(pps->weighted_bipred_idc);
> >  	if (pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE)
> >  		reg |= G1_REG_DEC_CTRL4_CABAC_E;
> > @@ -88,8 +87,8 @@ static void set_params(struct hantro_ctx *ctx)
> >  	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL4);
> >  
> >  	/* Decoder control register 5. */
> > -	reg = G1_REG_DEC_CTRL5_REFPIC_MK_LEN(slices[0].dec_ref_pic_marking_bit_size) |
> > -	      G1_REG_DEC_CTRL5_IDR_PIC_ID(slices[0].idr_pic_id);
> > +	reg = G1_REG_DEC_CTRL5_REFPIC_MK_LEN(dec_param->dec_ref_pic_marking_bit_size) |
> > +	      G1_REG_DEC_CTRL5_IDR_PIC_ID(dec_param->idr_pic_id);
> >  	if (pps->flags & V4L2_H264_PPS_FLAG_CONSTRAINED_INTRA_PRED)
> >  		reg |= G1_REG_DEC_CTRL5_CONST_INTRA_E;
> >  	if (pps->flags & V4L2_H264_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT)
> > @@ -103,10 +102,10 @@ static void set_params(struct hantro_ctx *ctx)
> >  	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL5);
> >  
> >  	/* Decoder control register 6. */
> > -	reg = G1_REG_DEC_CTRL6_PPS_ID(slices[0].pic_parameter_set_id) |
> > +	reg = G1_REG_DEC_CTRL6_PPS_ID(pps->pic_parameter_set_id) |
> >  	      G1_REG_DEC_CTRL6_REFIDX0_ACTIVE(pps->num_ref_idx_l0_default_active_minus1 + 1) |
> >  	      G1_REG_DEC_CTRL6_REFIDX1_ACTIVE(pps->num_ref_idx_l1_default_active_minus1 + 1) |
> > -	      G1_REG_DEC_CTRL6_POC_LENGTH(slices[0].pic_order_cnt_bit_size);
> > +	      G1_REG_DEC_CTRL6_POC_LENGTH(dec_param->pic_order_cnt_bit_size);
> >  	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL6);
> >  
> >  	/* Error concealment register. */
> > @@ -246,7 +245,7 @@ static void set_buffers(struct hantro_ctx *ctx)
> >  	/* Destination (decoded frame) buffer. */
> >  	dst_dma = hantro_get_dec_buf_addr(ctx, &dst_buf->vb2_buf);
> >  	/* Adjust dma addr to start at second line for bottom field */
> > -	if (ctrls->slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
> > +	if (ctrls->decode->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD)
> >  		offset = ALIGN(ctx->src_fmt.width, MB_DIM);
> >  	vdpu_write_relaxed(vpu, dst_dma + offset, G1_REG_ADDR_DST);
> >  
> > @@ -265,7 +264,7 @@ static void set_buffers(struct hantro_ctx *ctx)
> >  		 * DMV buffer is split in two for field encoded frames,
> >  		 * adjust offset for bottom field
> >  		 */
> > -		if (ctrls->slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
> > +		if (ctrls->decode->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD)
> >  			offset += 32 * MB_WIDTH(ctx->src_fmt.width) *
> >  				  MB_HEIGHT(ctx->src_fmt.height);
> >  		vdpu_write_relaxed(vpu, dst_dma + offset, G1_REG_ADDR_DIR_MV);
> > diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
> > index d561f125085a..9890ae7bf3bc 100644
> > --- a/drivers/staging/media/hantro/hantro_h264.c
> > +++ b/drivers/staging/media/hantro/hantro_h264.c
> > @@ -374,8 +374,7 @@ int hantro_h264_dec_prepare_run(struct hantro_ctx *ctx)
> >  
> >  	/* Build the P/B{0,1} ref lists. */
> >  	v4l2_h264_init_reflist_builder(&reflist_builder, ctrls->decode,
> > -				       &ctrls->slices[0], ctrls->sps,
> > -				       ctx->h264_dec.dpb);
> > +				       ctrls->sps, ctx->h264_dec.dpb);
> >  	v4l2_h264_build_p_ref_list(&reflist_builder, h264_ctx->reflists.p);
> >  	v4l2_h264_build_b_ref_lists(&reflist_builder, h264_ctx->reflists.b0,
> >  				    h264_ctx->reflists.b1);
> > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > index 7b66e2743a4f..0f5d519528d8 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > @@ -730,7 +730,6 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> >  			    struct rkvdec_h264_run *run)
> >  {
> >  	const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
> > -	const struct v4l2_ctrl_h264_slice_params *sl_params = &run->slices_params[0];
> >  	const struct v4l2_h264_dpb_entry *dpb = dec_params->dpb;
> >  	struct rkvdec_h264_ctx *h264_ctx = ctx->priv;
> >  	const struct v4l2_ctrl_h264_sps *sps = run->sps;
> > @@ -754,7 +753,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> >  			continue;
> >  
> >  		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM ||
> > -		    dpb[i].frame_num < sl_params->frame_num) {
> > +		    dpb[i].frame_num < dec_params->frame_num) {
> >  			p[i] = dpb[i].frame_num;
> >  			continue;
> >  		}
> > @@ -1093,8 +1092,7 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
> >  
> >  	/* Build the P/B{0,1} ref lists. */
> >  	v4l2_h264_init_reflist_builder(&reflist_builder, run.decode_params,
> > -				       &run.slices_params[0], run.sps,
> > -				       run.decode_params->dpb);
> > +				       run.sps, run.decode_params->dpb);
> >  	h264_ctx->reflists.num_valid = reflist_builder.num_valid;
> >  	v4l2_h264_build_p_ref_list(&reflist_builder, h264_ctx->reflists.p);
> >  	v4l2_h264_build_b_ref_lists(&reflist_builder, h264_ctx->reflists.b0,
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > index cce527bbdf86..e7387315253d 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > @@ -95,7 +95,6 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
> >  {
> >  	struct cedrus_h264_sram_ref_pic pic_list[CEDRUS_H264_FRAME_NUM];
> >  	const struct v4l2_ctrl_h264_decode_params *decode = run->h264.decode_params;
> > -	const struct v4l2_ctrl_h264_slice_params *slice = run->h264.slice_params;
> >  	const struct v4l2_ctrl_h264_sps *sps = run->h264.sps;
> >  	struct vb2_queue *cap_q;
> >  	struct cedrus_buffer *output_buf;
> > @@ -144,7 +143,7 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx,
> >  	output_buf = vb2_to_cedrus_buffer(&run->dst->vb2_buf);
> >  	output_buf->codec.h264.position = position;
> >  
> > -	if (slice->flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
> > +	if (decode->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC)
> >  		output_buf->codec.h264.pic_type = CEDRUS_H264_PIC_TYPE_FIELD;
> >  	else if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)
> >  		output_buf->codec.h264.pic_type = CEDRUS_H264_PIC_TYPE_MBAFF;
> > @@ -414,7 +413,7 @@ static void cedrus_set_params(struct cedrus_ctx *ctx,
> >  		reg |= VE_H264_SPS_DIRECT_8X8_INFERENCE;
> >  	cedrus_write(dev, VE_H264_SPS, reg);
> >  
> > -	mbaff_pic = !(slice->flags & V4L2_H264_SLICE_FLAG_FIELD_PIC) &&
> > +	mbaff_pic = !(decode->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC) &&
> >  		    (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD);
> >  	pic_width_in_mbs = sps->pic_width_in_mbs_minus1 + 1;
> >  
> > @@ -428,9 +427,9 @@ static void cedrus_set_params(struct cedrus_ctx *ctx,
> >  	reg |= slice->cabac_init_idc & 0x3;
> >  	if (ctx->fh.m2m_ctx->new_frame)
> >  		reg |= VE_H264_SHS_FIRST_SLICE_IN_PIC;
> > -	if (slice->flags & V4L2_H264_SLICE_FLAG_FIELD_PIC)
> > +	if (decode->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC)
> >  		reg |= VE_H264_SHS_FIELD_PIC;
> > -	if (slice->flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
> > +	if (decode->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD)
> >  		reg |= VE_H264_SHS_BOTTOM_FIELD;
> >  	if (slice->flags & V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED)
> >  		reg |= VE_H264_SHS_DIRECT_SPATIAL_MV_PRED;
> > diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> > index 4119dc4486f3..6446ec9f283d 100644
> > --- a/include/media/h264-ctrls.h
> > +++ b/include/media/h264-ctrls.h
> > @@ -138,10 +138,8 @@ struct v4l2_h264_pred_weight_table {
> >  #define V4L2_H264_SLICE_TYPE_SP				3
> >  #define V4L2_H264_SLICE_TYPE_SI				4
> >  
> > -#define V4L2_H264_SLICE_FLAG_FIELD_PIC			0x01
> > -#define V4L2_H264_SLICE_FLAG_BOTTOM_FIELD		0x02
> > -#define V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED	0x04
> > -#define V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH		0x08
> > +#define V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED	0x01
> > +#define V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH		0x02
> >  
> >  #define V4L2_H264_REFERENCE_FLAG_TOP_FIELD		0x01
> >  #define V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD		0x02
> > @@ -165,22 +163,8 @@ struct v4l2_ctrl_h264_slice_params {
> >  	__u32 first_mb_in_slice;
> >  
> >  	__u8 slice_type;
> > -	__u8 pic_parameter_set_id;
> >  	__u8 colour_plane_id;
> >  	__u8 redundant_pic_cnt;
> > -	__u16 frame_num;
> > -	__u16 idr_pic_id;
> > -	__u16 pic_order_cnt_lsb;
> > -	__s32 delta_pic_order_cnt_bottom;
> > -	__s32 delta_pic_order_cnt0;
> > -	__s32 delta_pic_order_cnt1;
> > -
> > -	struct v4l2_h264_pred_weight_table pred_weight_table;
> > -	/* Size in bits of dec_ref_pic_marking() syntax element. */
> > -	__u32 dec_ref_pic_marking_bit_size;
> > -	/* Size in bits of pic order count syntax. */
> > -	__u32 pic_order_cnt_bit_size;
> > -
> >  	__u8 cabac_init_idc;
> >  	__s8 slice_qp_delta;
> >  	__s8 slice_qs_delta;
> > @@ -189,7 +173,9 @@ struct v4l2_ctrl_h264_slice_params {
> >  	__s8 slice_beta_offset_div2;
> >  	__u8 num_ref_idx_l0_active_minus1;
> >  	__u8 num_ref_idx_l1_active_minus1;
> > -	__u32 slice_group_change_cycle;
> > +	__u8 reserved[5];
> > +
> > +	struct v4l2_h264_pred_weight_table pred_weight_table;
> >  
> >  	/*
> >  	 * Entries on each list are indices into
> > @@ -218,7 +204,9 @@ struct v4l2_h264_dpb_entry {
> >  	__u32 flags; /* V4L2_H264_DPB_ENTRY_FLAG_* */
> >  };
> >  
> > -#define V4L2_H264_DECODE_PARAM_FLAG_IDR_PIC	0x01
> > +#define V4L2_H264_DECODE_PARAM_FLAG_IDR_PIC		0x01
> > +#define V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC		0x02
> > +#define V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD	0x04
> >  
> >  struct v4l2_ctrl_h264_decode_params {
> >  	struct v4l2_h264_dpb_entry dpb[V4L2_H264_NUM_DPB_ENTRIES];
> > @@ -226,6 +214,21 @@ struct v4l2_ctrl_h264_decode_params {
> >  	__u16 nal_ref_idc;
> >  	__s32 top_field_order_cnt;
> >  	__s32 bottom_field_order_cnt;
> > +
> > +	__u16 frame_num;
> > +	__u16 idr_pic_id;
> > +	__u16 reserved;
> > +
> > +	__u16 pic_order_cnt_lsb;
> > +	__s32 delta_pic_order_cnt_bottom;
> > +	__s32 delta_pic_order_cnt0;
> > +	__s32 delta_pic_order_cnt1;
> > +	/* Size in bits of dec_ref_pic_marking() syntax element. */
> > +	__u32 dec_ref_pic_marking_bit_size;
> > +	/* Size in bits of pic order count syntax. */
> > +	__u32 pic_order_cnt_bit_size;
> > +	__u32 slice_group_change_cycle;
> > +
> >  	__u32 flags; /* V4L2_H264_DECODE_PARAM_FLAG_* */
> >  };
> >  
> > diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
> > index 1a5f26fc2a9a..f08ba181263d 100644
> > --- a/include/media/v4l2-h264.h
> > +++ b/include/media/v4l2-h264.h
> > @@ -44,7 +44,6 @@ struct v4l2_h264_reflist_builder {
> >  void
> >  v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
> >  		const struct v4l2_ctrl_h264_decode_params *dec_params,
> > -		const struct v4l2_ctrl_h264_slice_params *slice_params,
> >  		const struct v4l2_ctrl_h264_sps *sps,
> >  		const struct v4l2_h264_dpb_entry dpb[V4L2_H264_NUM_DPB_ENTRIES]);
> >