[PATCH RFC 0/6] media: uapi: h264: First batch of adjusments

[PATCH RFC 0/6] media: uapi: h264: First batch of adjusments

[Boris Brezillon]

Hello,

This is a first batch of adjustments to the stateless H264 decoder
uAPI. The first one is about adding support for per-frame decoding,
which is the only mode supported on some codecs (the hantro G1 block
supports per-slice decoding but not in an way that would allow
efficient multiplexing of several decoding contexts).

The second modification drops the P0/B0/B1 ref lists from the
decode_params control. These lists are no longer needed now that we know
we can build them kernel side based on the DPB.

There are few more changes in the pipe, but I'd like to sync with Paul,
Jonas, Jernej and Nicolas before modifying:
* Enforce order of the scaling list (looks like the rockchip and cedrus
  have different expectations)
* Pass top/bottom field info as flags in the DPB entry: the field
  attached to the capture buffer is not accurate as capture bufs might
  contain both top/bottom (meaning they are actually interlaced), but a
  coded frame might contain only one of these fields. Note
  that there's also a problem with the output -> capture field flag
  propagation we have in copy_metadata() because a coded slice might
  contain only data for top or bottom, but the capture frame might
  contain both. Doing capture->field = output->field means we're lying
  about what's inside the capture buffer (not sure we have
  implementation checking that)
* s/dpb/refs/: looks like we're abusing the term DPB which is supposed
  to be implementation specific. What's provided by the bitstream is a
  list of references that will be used to decode a frame
* ... (add your own)

Feel free to comment on these changes and/or propose alternatives.

Regards,

Boris

Boris Brezillon (6):
  media: uapi: h264: Clarify our expectations regarding NAL header
    format
  media: uapi: h264: Add the concept of decoding mode
  media: uapi: h264: Get rid of the p0/b0/b1 ref-lists
  media: cedrus: Prepare things to support !compound controls
  media: cedrus: Make the slice_params array size limitation more
    explicit
  media: cedrus: Add the H264_DECODING_MODE control

 .../media/uapi/v4l/ext-ctrls-codec.rst        | 52 ++++++++++++---
 drivers/media/v4l2-core/v4l2-ctrls.c          |  9 +++
 drivers/staging/media/sunxi/cedrus/cedrus.c   | 66 +++++++++++++------
 drivers/staging/media/sunxi/cedrus/cedrus.h   |  3 +-
 include/media/h264-ctrls.h                    | 13 ++++
 5 files changed, 112 insertions(+), 31 deletions(-)

-- 
2.20.1

[PATCH RFC 1/6] media: uapi: h264: Clarify our expectations regarding NAL header format

[Boris Brezillon]

Looks like some stateless decoders expect slices to be prefixed with
ANNEX B start codes (they most likely do some kind of bitstream parsing
and/or need that to delimit slices when doing per frame decoding).
Since skipping those start codes for dummy stateless decoders (those
expecting all params to be passed through controls) should be pretty
easy, let's mandate that all slices be prepended with ANNEX B start
codes.

If we ever need to support AVC headers, we can add a new menu control
to select the type of NAL header to use.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Note: we might want to add a nal_header_size field to allow drivers to
quickly find where the actual payload start. That'd be particularly
useful here since ANNEX B start codes can be of arbitrary size
(2+(0x00 bytes) + 1(0x01 byte)). The other option would be to enforce
the number of leading 0x00 bytes (a minimum of 2 is required).
---
 Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
index d6ea2ffd65c5..82547d5de250 100644
--- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
+++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
@@ -1726,6 +1726,7 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     :ref:`h264`, section 7.4.3 "Slice Header Semantics". For further
     documentation, refer to the above specification, unless there is
     an explicit comment stating otherwise.
+    All slices should be prepended with an ANNEX B start code.
 
     .. note::
 
-- 
2.20.1

[PATCH RFC 2/6] media: uapi: h264: Add the concept of decoding mode

[Boris Brezillon]

Some stateless decoders don't support per-slice decoding (or at least
not in a way that would make them efficient or easy to use).
Let's expose a menu to control and expose the supported decoding modes.
Drivers are allowed to support only one decoding but they can support
both too.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 .../media/uapi/v4l/ext-ctrls-codec.rst        | 42 ++++++++++++++++++-
 drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ++++
 include/media/h264-ctrls.h                    | 13 ++++++
 3 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
index 82547d5de250..188f625acb7c 100644
--- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
+++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
@@ -1748,6 +1748,14 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __u32
       - ``size``
       -
+    * - __u32
+      - ``start_byte_offset``
+      - Where the slice payload starts in the output buffer. Useful when
+        operating in per frame decoding mode and decoding multi-slice content.
+        In this case, the output buffer will contain more than one slice and
+        some codecs need to know where each slice starts. Note that this
+        offsets points to the beginning of the slice which is supposed to
+        contain an ANNEX B start code
     * - __u32
       - ``header_bit_size``
       -
@@ -1931,7 +1939,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
       -
     * - __u16
       - ``num_slices``
-      - Number of slices needed to decode the current frame
+      - Number of slices needed to decode the current frame/field. When
+        operating in per-slice decoding mode (see
+        :c:type:`v4l2_mpeg_video_h264_decoding_mode`), this field
+        should always be set to one
     * - __u16
       - ``nal_ref_idc``
       - NAL reference ID value coming from the NAL Unit header
@@ -2022,6 +2033,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
       - 0x00000004
       - The DPB entry is a long term reference frame
 
+``V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE (enum)``
+    Specifies the decoding mode to use. Currently exposes per slice and per
+    frame decoding but new modes might be added later on.
+
+    .. note::
+
+       This menu control is not yet part of the public kernel API and
+       it is expected to change.
+
+.. c:type:: v4l2_mpeg_video_h264_decoding_mode
+
+.. cssclass:: longtable
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       1 1 2
+
+    * - ``V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE``
+      - 0
+      - The decoding is done per slice. v4l2_ctrl_h264_decode_params->num_slices
+        must be set to 1 and the output buffer should contain only one slice.
+    * - ``V4L2_MPEG_VIDEO_H264_DECODING_PER_FRAME``
+      - 1
+      - The decoding is done per frame. v4l2_ctrl_h264_decode_params->num_slices
+        can be > 1. When that happens, the output buffer should contain all
+        slices needed to decode a frame/field, each slice being prefixed by an
+        Annex B NAL header/start-code.
+
 .. _v4l2-mpeg-mpeg2:
 
 ``V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS (struct)``
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 1217d38ea394..72bb3c8882f5 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -406,6 +406,11 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		"Explicit",
 		NULL,
 	};
+	static const char * const h264_decoding_mode[] = {
+		"Per Slice",
+		"Per Frame",
+		NULL,
+	};
 	static const char * const mpeg_mpeg2_level[] = {
 		"Low",
 		"Main",
@@ -637,6 +642,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		return h264_fp_arrangement_type;
 	case V4L2_CID_MPEG_VIDEO_H264_FMO_MAP_TYPE:
 		return h264_fmo_map_type;
+	case V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE:
+		return h264_decoding_mode;
 	case V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL:
 		return mpeg_mpeg2_level;
 	case V4L2_CID_MPEG_VIDEO_MPEG2_PROFILE:
@@ -856,6 +863,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX:		return "H264 Scaling Matrix";
 	case V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS:		return "H264 Slice Parameters";
 	case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS:		return "H264 Decode Parameters";
+	case V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE:		return "H264 Decoding Mode";
 	case V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL:			return "MPEG2 Level";
 	case V4L2_CID_MPEG_VIDEO_MPEG2_PROFILE:			return "MPEG2 Profile";
 	case V4L2_CID_MPEG_VIDEO_MPEG4_I_FRAME_QP:		return "MPEG4 I-Frame QP Value";
@@ -1224,6 +1232,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_MPEG_VIDEO_H264_VUI_SAR_IDC:
 	case V4L2_CID_MPEG_VIDEO_H264_SEI_FP_ARRANGEMENT_TYPE:
 	case V4L2_CID_MPEG_VIDEO_H264_FMO_MAP_TYPE:
+	case V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE:
 	case V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL:
 	case V4L2_CID_MPEG_VIDEO_MPEG2_PROFILE:
 	case V4L2_CID_MPEG_VIDEO_MPEG4_LEVEL:
diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
index e1404d78d6ff..26de2243f6f5 100644
--- a/include/media/h264-ctrls.h
+++ b/include/media/h264-ctrls.h
@@ -26,6 +26,7 @@
 #define V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX	(V4L2_CID_MPEG_BASE+1002)
 #define V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS	(V4L2_CID_MPEG_BASE+1003)
 #define V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS	(V4L2_CID_MPEG_BASE+1004)
+#define V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE	(V4L2_CID_MPEG_BASE+1005)
 
 /* enum v4l2_ctrl_type type values */
 #define V4L2_CTRL_TYPE_H264_SPS			0x0110
@@ -33,6 +34,12 @@
 #define V4L2_CTRL_TYPE_H264_SCALING_MATRIX	0x0112
 #define V4L2_CTRL_TYPE_H264_SLICE_PARAMS	0x0113
 #define V4L2_CTRL_TYPE_H264_DECODE_PARAMS	0x0114
+#define V4L2_CTRL_TYPE_H264_DECODING_MODE	0x0115
+
+enum v4l2_mpeg_video_h264_decoding_mode {
+	V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE,
+	V4L2_MPEG_VIDEO_H264_DECODING_PER_FRAME,
+};
 
 #define V4L2_H264_SPS_CONSTRAINT_SET0_FLAG			0x01
 #define V4L2_H264_SPS_CONSTRAINT_SET1_FLAG			0x02
@@ -111,6 +118,8 @@ struct v4l2_h264_pred_weight_table {
 	struct v4l2_h264_weight_factors weight_factors[2];
 };
 
+#define V4L2_H264_MAX_SLICES_PER_FRAME			16
+
 #define V4L2_H264_SLICE_TYPE_P				0
 #define V4L2_H264_SLICE_TYPE_B				1
 #define V4L2_H264_SLICE_TYPE_I				2
@@ -125,6 +134,10 @@ struct v4l2_h264_pred_weight_table {
 struct v4l2_ctrl_h264_slice_params {
 	/* Size in bytes, including header */
 	__u32 size;
+
+	/* Where the slice starts in the output buffer (expressed in bytes). */
+	__u32 start_byte_offset;
+
 	/* Offset in bits to slice_data() from the beginning of this slice. */
 	__u32 header_bit_size;
 
-- 
2.20.1

[PATCH RFC 3/6] media: uapi: h264: Get rid of the p0/b0/b1 ref-lists

[Boris Brezillon]

Those lists can be extracted from the dpb, let's simplify userspace
life and build that list kernel-side (generic helpers will be provided
for drivers that need this list).

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
index 188f625acb7c..f3edb18ac19a 100644
--- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
+++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
@@ -1946,15 +1946,6 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __u16
       - ``nal_ref_idc``
       - NAL reference ID value coming from the NAL Unit header
-    * - __u8
-      - ``ref_pic_list_p0[32]``
-      - Backward reference list used by P-frames in the original bitstream order
-    * - __u8
-      - ``ref_pic_list_b0[32]``
-      - Backward reference list used by B-frames in the original bitstream order
-    * - __u8
-      - ``ref_pic_list_b1[32]``
-      - Forward reference list used by B-frames in the original bitstream order
     * - __s32
       - ``top_field_order_cnt``
       - Picture Order Count for the coded top field
-- 
2.20.1

[PATCH RFC 4/6] media: cedrus: Prepare things to support !compound controls

[Boris Brezillon]

We are about to add a menu control, so let's make the code more generic
to support other control types.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/staging/media/sunxi/cedrus/cedrus.c | 47 ++++++++++++---------
 drivers/staging/media/sunxi/cedrus/cedrus.h |  3 +-
 2 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index 370937edfc14..378032fe71f9 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -29,44 +29,44 @@
 
 static const struct cedrus_control cedrus_controls[] = {
 	{
-		.id		= V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS,
-		.elem_size	= sizeof(struct v4l2_ctrl_mpeg2_slice_params),
+		.cfg.id		= V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS,
+		.cfg.elem_size	= sizeof(struct v4l2_ctrl_mpeg2_slice_params),
 		.codec		= CEDRUS_CODEC_MPEG2,
 		.required	= true,
 	},
 	{
-		.id		= V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION,
-		.elem_size	= sizeof(struct v4l2_ctrl_mpeg2_quantization),
+		.cfg.id		= V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION,
+		.cfg.elem_size	= sizeof(struct v4l2_ctrl_mpeg2_quantization),
 		.codec		= CEDRUS_CODEC_MPEG2,
 		.required	= false,
 	},
 	{
-		.id		= V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS,
-		.elem_size	= sizeof(struct v4l2_ctrl_h264_decode_params),
+		.cfg.id		= V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS,
+		.cfg.elem_size	= sizeof(struct v4l2_ctrl_h264_decode_params),
 		.codec		= CEDRUS_CODEC_H264,
 		.required	= true,
 	},
 	{
-		.id		= V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS,
-		.elem_size	= sizeof(struct v4l2_ctrl_h264_slice_params),
+		.cfg.id		= V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS,
+		.cfg.elem_size	= sizeof(struct v4l2_ctrl_h264_slice_params),
 		.codec		= CEDRUS_CODEC_H264,
 		.required	= true,
 	},
 	{
-		.id		= V4L2_CID_MPEG_VIDEO_H264_SPS,
-		.elem_size	= sizeof(struct v4l2_ctrl_h264_sps),
+		.cfg.id		= V4L2_CID_MPEG_VIDEO_H264_SPS,
+		.cfg.elem_size	= sizeof(struct v4l2_ctrl_h264_sps),
 		.codec		= CEDRUS_CODEC_H264,
 		.required	= true,
 	},
 	{
-		.id		= V4L2_CID_MPEG_VIDEO_H264_PPS,
-		.elem_size	= sizeof(struct v4l2_ctrl_h264_pps),
+		.cfg.id		= V4L2_CID_MPEG_VIDEO_H264_PPS,
+		.cfg.elem_size	= sizeof(struct v4l2_ctrl_h264_pps),
 		.codec		= CEDRUS_CODEC_H264,
 		.required	= true,
 	},
 	{
-		.id		= V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX,
-		.elem_size	= sizeof(struct v4l2_ctrl_h264_scaling_matrix),
+		.cfg.id		= V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX,
+		.cfg.elem_size	= sizeof(struct v4l2_ctrl_h264_scaling_matrix),
 		.codec		= CEDRUS_CODEC_H264,
 		.required	= true,
 	},
@@ -106,12 +106,21 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev, struct cedrus_ctx *ctx)
 		return -ENOMEM;
 
 	for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {
-		struct v4l2_ctrl_config cfg = {};
+		const struct v4l2_ctrl_config *cfg = &cedrus_controls[i].cfg;
 
-		cfg.elem_size = cedrus_controls[i].elem_size;
-		cfg.id = cedrus_controls[i].id;
+		if (cfg->elem_size)
+			ctrl = v4l2_ctrl_new_custom(hdl, cfg, NULL);
+		else if (cfg->type == V4L2_CTRL_TYPE_MENU ||
+			   cfg->type == V4L2_CTRL_TYPE_INTEGER_MENU)
+			ctrl = v4l2_ctrl_new_std_menu(hdl, NULL,
+						      cfg->id, cfg->max,
+						      cfg->menu_skip_mask,
+						      cfg->def);
+		else
+			ctrl = v4l2_ctrl_new_std(hdl, NULL, cfg->id, cfg->min,
+						 cfg->max, cfg->step,
+						 cfg->def);
 
-		ctrl = v4l2_ctrl_new_custom(hdl, &cfg, NULL);
 		if (hdl->error) {
 			v4l2_err(&dev->v4l2_dev,
 				 "Failed to create new custom control\n");
@@ -178,7 +187,7 @@ static int cedrus_request_validate(struct media_request *req)
 			continue;
 
 		ctrl_test = v4l2_ctrl_request_hdl_ctrl_find(hdl,
-							    cedrus_controls[i].id);
+							    cedrus_controls[i].cfg.id);
 		if (!ctrl_test) {
 			v4l2_info(&ctx->dev->v4l2_dev,
 				  "Missing required codec control\n");
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
index 3f476d0fd981..69c037724d93 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
@@ -49,8 +49,7 @@ enum cedrus_h264_pic_type {
 };
 
 struct cedrus_control {
-	u32			id;
-	u32			elem_size;
+	struct v4l2_ctrl_config	cfg;
 	enum cedrus_codec	codec;
 	unsigned char		required:1;
 };
-- 
2.20.1

[PATCH RFC 5/6] media: cedrus: Make the slice_params array size limitation more explicit

[Boris Brezillon]

The driver only supports per-slice decoding, and in that mode
decode_params->num_slices must be set to 1 and the slice_params array
should contain only one element.

The current code already had this limitation but it made it look like
the slice_params control was a single struct while, according to the
spec, it's actually an array. Make it more explicit by setting dims[0]
and adding a comment explaining why we have this limitation.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/staging/media/sunxi/cedrus/cedrus.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index 378032fe71f9..3661c6a04864 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -49,6 +49,12 @@ static const struct cedrus_control cedrus_controls[] = {
 	{
 		.cfg.id		= V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS,
 		.cfg.elem_size	= sizeof(struct v4l2_ctrl_h264_slice_params),
+		/*
+		 * This driver does not support per-frame decoding (yet?).
+		 * Allow only on per-slice decoding operation, which implies
+		 * that only 1 slice param is passed per decoding operation.
+		 */
+		.cfg.dims[0]	= 1,
 		.codec		= CEDRUS_CODEC_H264,
 		.required	= true,
 	},
-- 
2.20.1

[PATCH RFC 6/6] media: cedrus: Add the H264_DECODING_MODE control

[Boris Brezillon]

The H264 uAPI has been modified to expose 2 operation modes: per slice
and per frame decoding.

The cedrus driver only supports per-slice decoding for now.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/staging/media/sunxi/cedrus/cedrus.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index 3661c6a04864..fe6e6a12409b 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -76,6 +76,19 @@ static const struct cedrus_control cedrus_controls[] = {
 		.codec		= CEDRUS_CODEC_H264,
 		.required	= true,
 	},
+	{
+		.cfg.id		= V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE,
+		.cfg.type	= V4L2_CTRL_TYPE_MENU,
+		/*
+		 * Only per-slice decoding is supported for now. Note that we
+		 * don't need to extract this control value since only one
+		 * value is allowed: V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE.
+		 */
+		.cfg.max	= V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE,
+		.cfg.def	= V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE,
+		.codec		= CEDRUS_CODEC_H264,
+		.required	= true,
+	},
 };
 
 #define CEDRUS_CONTROLS_COUNT	ARRAY_SIZE(cedrus_controls)
-- 
2.20.1

Re: [PATCH RFC 2/6] media: uapi: h264: Add the concept of decoding mode

[Thierry Reding]

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

On Mon, Jun 03, 2019 at 01:09:42PM +0200, Boris Brezillon wrote:
> Some stateless decoders don't support per-slice decoding (or at least
> not in a way that would make them efficient or easy to use).
> Let's expose a menu to control and expose the supported decoding modes.
> Drivers are allowed to support only one decoding but they can support
> both too.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  .../media/uapi/v4l/ext-ctrls-codec.rst        | 42 ++++++++++++++++++-
>  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ++++
>  include/media/h264-ctrls.h                    | 13 ++++++
>  3 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> index 82547d5de250..188f625acb7c 100644
> --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> @@ -1748,6 +1748,14 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      * - __u32
>        - ``size``
>        -
> +    * - __u32
> +      - ``start_byte_offset``
> +      - Where the slice payload starts in the output buffer. Useful when
> +        operating in per frame decoding mode and decoding multi-slice content.
> +        In this case, the output buffer will contain more than one slice and
> +        some codecs need to know where each slice starts. Note that this
> +        offsets points to the beginning of the slice which is supposed to
> +        contain an ANNEX B start code
>      * - __u32
>        - ``header_bit_size``
>        -
> @@ -1931,7 +1939,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>        -
>      * - __u16
>        - ``num_slices``
> -      - Number of slices needed to decode the current frame
> +      - Number of slices needed to decode the current frame/field. When
> +        operating in per-slice decoding mode (see
> +        :c:type:`v4l2_mpeg_video_h264_decoding_mode`), this field
> +        should always be set to one
>      * - __u16
>        - ``nal_ref_idc``
>        - NAL reference ID value coming from the NAL Unit header
> @@ -2022,6 +2033,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>        - 0x00000004
>        - The DPB entry is a long term reference frame
>  
> +``V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE (enum)``
> +    Specifies the decoding mode to use. Currently exposes per slice and per
> +    frame decoding but new modes might be added later on.
> +
> +    .. note::
> +
> +       This menu control is not yet part of the public kernel API and
> +       it is expected to change.
> +
> +.. c:type:: v4l2_mpeg_video_h264_decoding_mode
> +
> +.. cssclass:: longtable
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       1 1 2
> +
> +    * - ``V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE``
> +      - 0
> +      - The decoding is done per slice. v4l2_ctrl_h264_decode_params->num_slices
> +        must be set to 1 and the output buffer should contain only one slice.

I wonder if we need to be that strict. Wouldn't it be possible for
drivers to just iterate over a number of slices and decode each in turn
if userspace passed more than one?

Or perhaps a decoder can batch queue a couple of slices. I'm not sure
how frequent this is, but consider something like a spike in activity on
your system, causing some slices to get delayed so you actually get a
few buffered up before you get a chance to hand them to the V4L2 device.
Processing them all at once may help conceal the lag.

Thierry

> +    * - ``V4L2_MPEG_VIDEO_H264_DECODING_PER_FRAME``
> +      - 1
> +      - The decoding is done per frame. v4l2_ctrl_h264_decode_params->num_slices
> +        can be > 1. When that happens, the output buffer should contain all
> +        slices needed to decode a frame/field, each slice being prefixed by an
> +        Annex B NAL header/start-code.
> +
>  .. _v4l2-mpeg-mpeg2:
>  
>  ``V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS (struct)``
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 1217d38ea394..72bb3c8882f5 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -406,6 +406,11 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"Explicit",
>  		NULL,
>  	};
> +	static const char * const h264_decoding_mode[] = {
> +		"Per Slice",
> +		"Per Frame",
> +		NULL,
> +	};
>  	static const char * const mpeg_mpeg2_level[] = {
>  		"Low",
>  		"Main",
> @@ -637,6 +642,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		return h264_fp_arrangement_type;
>  	case V4L2_CID_MPEG_VIDEO_H264_FMO_MAP_TYPE:
>  		return h264_fmo_map_type;
> +	case V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE:
> +		return h264_decoding_mode;
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL:
>  		return mpeg_mpeg2_level;
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_PROFILE:
> @@ -856,6 +863,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX:		return "H264 Scaling Matrix";
>  	case V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS:		return "H264 Slice Parameters";
>  	case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS:		return "H264 Decode Parameters";
> +	case V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE:		return "H264 Decoding Mode";
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL:			return "MPEG2 Level";
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_PROFILE:			return "MPEG2 Profile";
>  	case V4L2_CID_MPEG_VIDEO_MPEG4_I_FRAME_QP:		return "MPEG4 I-Frame QP Value";
> @@ -1224,6 +1232,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_MPEG_VIDEO_H264_VUI_SAR_IDC:
>  	case V4L2_CID_MPEG_VIDEO_H264_SEI_FP_ARRANGEMENT_TYPE:
>  	case V4L2_CID_MPEG_VIDEO_H264_FMO_MAP_TYPE:
> +	case V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE:
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL:
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_PROFILE:
>  	case V4L2_CID_MPEG_VIDEO_MPEG4_LEVEL:
> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> index e1404d78d6ff..26de2243f6f5 100644
> --- a/include/media/h264-ctrls.h
> +++ b/include/media/h264-ctrls.h
> @@ -26,6 +26,7 @@
>  #define V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX	(V4L2_CID_MPEG_BASE+1002)
>  #define V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS	(V4L2_CID_MPEG_BASE+1003)
>  #define V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS	(V4L2_CID_MPEG_BASE+1004)
> +#define V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE	(V4L2_CID_MPEG_BASE+1005)
>  
>  /* enum v4l2_ctrl_type type values */
>  #define V4L2_CTRL_TYPE_H264_SPS			0x0110
> @@ -33,6 +34,12 @@
>  #define V4L2_CTRL_TYPE_H264_SCALING_MATRIX	0x0112
>  #define V4L2_CTRL_TYPE_H264_SLICE_PARAMS	0x0113
>  #define V4L2_CTRL_TYPE_H264_DECODE_PARAMS	0x0114
> +#define V4L2_CTRL_TYPE_H264_DECODING_MODE	0x0115
> +
> +enum v4l2_mpeg_video_h264_decoding_mode {
> +	V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE,
> +	V4L2_MPEG_VIDEO_H264_DECODING_PER_FRAME,
> +};
>  
>  #define V4L2_H264_SPS_CONSTRAINT_SET0_FLAG			0x01
>  #define V4L2_H264_SPS_CONSTRAINT_SET1_FLAG			0x02
> @@ -111,6 +118,8 @@ struct v4l2_h264_pred_weight_table {
>  	struct v4l2_h264_weight_factors weight_factors[2];
>  };
>  
> +#define V4L2_H264_MAX_SLICES_PER_FRAME			16
> +
>  #define V4L2_H264_SLICE_TYPE_P				0
>  #define V4L2_H264_SLICE_TYPE_B				1
>  #define V4L2_H264_SLICE_TYPE_I				2
> @@ -125,6 +134,10 @@ struct v4l2_h264_pred_weight_table {
>  struct v4l2_ctrl_h264_slice_params {
>  	/* Size in bytes, including header */
>  	__u32 size;
> +
> +	/* Where the slice starts in the output buffer (expressed in bytes). */
> +	__u32 start_byte_offset;
> +
>  	/* Offset in bits to slice_data() from the beginning of this slice. */
>  	__u32 header_bit_size;
>  
> -- 
> 2.20.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC 2/6] media: uapi: h264: Add the concept of decoding mode

[Boris Brezillon]

+Maxime

Oops, just realized Maxime was not Cc-ed on this series.

On Mon, 3 Jun 2019 14:30:20 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Mon, Jun 03, 2019 at 01:09:42PM +0200, Boris Brezillon wrote:
> > Some stateless decoders don't support per-slice decoding (or at least
> > not in a way that would make them efficient or easy to use).
> > Let's expose a menu to control and expose the supported decoding modes.
> > Drivers are allowed to support only one decoding but they can support
> > both too.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  .../media/uapi/v4l/ext-ctrls-codec.rst        | 42 ++++++++++++++++++-
> >  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ++++
> >  include/media/h264-ctrls.h                    | 13 ++++++
> >  3 files changed, 63 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > index 82547d5de250..188f625acb7c 100644
> > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > @@ -1748,6 +1748,14 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >      * - __u32
> >        - ``size``
> >        -
> > +    * - __u32
> > +      - ``start_byte_offset``
> > +      - Where the slice payload starts in the output buffer. Useful when
> > +        operating in per frame decoding mode and decoding multi-slice content.
> > +        In this case, the output buffer will contain more than one slice and
> > +        some codecs need to know where each slice starts. Note that this
> > +        offsets points to the beginning of the slice which is supposed to
> > +        contain an ANNEX B start code
> >      * - __u32
> >        - ``header_bit_size``
> >        -
> > @@ -1931,7 +1939,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >        -
> >      * - __u16
> >        - ``num_slices``
> > -      - Number of slices needed to decode the current frame
> > +      - Number of slices needed to decode the current frame/field. When
> > +        operating in per-slice decoding mode (see
> > +        :c:type:`v4l2_mpeg_video_h264_decoding_mode`), this field
> > +        should always be set to one
> >      * - __u16
> >        - ``nal_ref_idc``
> >        - NAL reference ID value coming from the NAL Unit header
> > @@ -2022,6 +2033,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >        - 0x00000004
> >        - The DPB entry is a long term reference frame
> >  
> > +``V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE (enum)``
> > +    Specifies the decoding mode to use. Currently exposes per slice and per
> > +    frame decoding but new modes might be added later on.
> > +
> > +    .. note::
> > +
> > +       This menu control is not yet part of the public kernel API and
> > +       it is expected to change.
> > +
> > +.. c:type:: v4l2_mpeg_video_h264_decoding_mode
> > +
> > +.. cssclass:: longtable
> > +
> > +.. flat-table::
> > +    :header-rows:  0
> > +    :stub-columns: 0
> > +    :widths:       1 1 2
> > +
> > +    * - ``V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE``
> > +      - 0
> > +      - The decoding is done per slice. v4l2_ctrl_h264_decode_params->num_slices
> > +        must be set to 1 and the output buffer should contain only one slice.  
> 
> I wonder if we need to be that strict. Wouldn't it be possible for
> drivers to just iterate over a number of slices and decode each in turn
> if userspace passed more than one?
> 
> Or perhaps a decoder can batch queue a couple of slices. I'm not sure
> how frequent this is, but consider something like a spike in activity on
> your system, causing some slices to get delayed so you actually get a
> few buffered up before you get a chance to hand them to the V4L2 device.
> Processing them all at once may help conceal the lag.

Hm, so we'd be in some kind of slice-batch mode, which means we could
trigger a decode operation with more than one slice, but not
necessarily all the slices needed to decode a frame. TBH, supporting
per-frame (or the batch mode you suggest) on a HW that supports
per-slice decoding should be pretty simple and has not real impact on
perfs (as you said, it's just a matter of iterating over all the slices
attached to a decode operation), so I'm fine relaxing the rule here and
patching the cedrus driver accordingly (I can't really test the
changes though). Paul, Maxime, what's your opinion?

Re: [PATCH RFC 2/6] media: uapi: h264: Add the concept of decoding mode

[Thierry Reding]

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

On Mon, Jun 03, 2019 at 02:51:13PM +0200, Boris Brezillon wrote:
> +Maxime
> 
> Oops, just realized Maxime was not Cc-ed on this series.
> 
> On Mon, 3 Jun 2019 14:30:20 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Mon, Jun 03, 2019 at 01:09:42PM +0200, Boris Brezillon wrote:
> > > Some stateless decoders don't support per-slice decoding (or at least
> > > not in a way that would make them efficient or easy to use).
> > > Let's expose a menu to control and expose the supported decoding modes.
> > > Drivers are allowed to support only one decoding but they can support
> > > both too.
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > ---
> > >  .../media/uapi/v4l/ext-ctrls-codec.rst        | 42 ++++++++++++++++++-
> > >  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ++++
> > >  include/media/h264-ctrls.h                    | 13 ++++++
> > >  3 files changed, 63 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > index 82547d5de250..188f625acb7c 100644
> > > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > @@ -1748,6 +1748,14 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > >      * - __u32
> > >        - ``size``
> > >        -
> > > +    * - __u32
> > > +      - ``start_byte_offset``
> > > +      - Where the slice payload starts in the output buffer. Useful when
> > > +        operating in per frame decoding mode and decoding multi-slice content.
> > > +        In this case, the output buffer will contain more than one slice and
> > > +        some codecs need to know where each slice starts. Note that this
> > > +        offsets points to the beginning of the slice which is supposed to
> > > +        contain an ANNEX B start code
> > >      * - __u32
> > >        - ``header_bit_size``
> > >        -
> > > @@ -1931,7 +1939,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > >        -
> > >      * - __u16
> > >        - ``num_slices``
> > > -      - Number of slices needed to decode the current frame
> > > +      - Number of slices needed to decode the current frame/field. When
> > > +        operating in per-slice decoding mode (see
> > > +        :c:type:`v4l2_mpeg_video_h264_decoding_mode`), this field
> > > +        should always be set to one
> > >      * - __u16
> > >        - ``nal_ref_idc``
> > >        - NAL reference ID value coming from the NAL Unit header
> > > @@ -2022,6 +2033,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > >        - 0x00000004
> > >        - The DPB entry is a long term reference frame
> > >  
> > > +``V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE (enum)``
> > > +    Specifies the decoding mode to use. Currently exposes per slice and per
> > > +    frame decoding but new modes might be added later on.
> > > +
> > > +    .. note::
> > > +
> > > +       This menu control is not yet part of the public kernel API and
> > > +       it is expected to change.
> > > +
> > > +.. c:type:: v4l2_mpeg_video_h264_decoding_mode
> > > +
> > > +.. cssclass:: longtable
> > > +
> > > +.. flat-table::
> > > +    :header-rows:  0
> > > +    :stub-columns: 0
> > > +    :widths:       1 1 2
> > > +
> > > +    * - ``V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE``
> > > +      - 0
> > > +      - The decoding is done per slice. v4l2_ctrl_h264_decode_params->num_slices
> > > +        must be set to 1 and the output buffer should contain only one slice.  
> > 
> > I wonder if we need to be that strict. Wouldn't it be possible for
> > drivers to just iterate over a number of slices and decode each in turn
> > if userspace passed more than one?
> > 
> > Or perhaps a decoder can batch queue a couple of slices. I'm not sure
> > how frequent this is, but consider something like a spike in activity on
> > your system, causing some slices to get delayed so you actually get a
> > few buffered up before you get a chance to hand them to the V4L2 device.
> > Processing them all at once may help conceal the lag.
> 
> Hm, so we'd be in some kind of slice-batch mode, which means we could
> trigger a decode operation with more than one slice, but not
> necessarily all the slices needed to decode a frame. TBH, supporting
> per-frame (or the batch mode you suggest) on a HW that supports
> per-slice decoding should be pretty simple and has not real impact on
> perfs (as you said, it's just a matter of iterating over all the slices
> attached to a decode operation), so I'm fine relaxing the rule here and
> patching the cedrus driver accordingly (I can't really test the
> changes though). Paul, Maxime, what's your opinion?

We could perhaps have a test program to orchestrate such a scenario. I
think the assumption should obviously still be that we don't cross the
frame boundary using slices in one batch. Just that if a frame was made
up of, say, 4 slices and you first pass 3 slices, then 1, that it'd be
nice if the driver would be able to cope with that. It's something that
could probably even be implemented in the framework as a helper, though
I suspect it'd be just a couple of lines of extra code to wrap a loop
around everything.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC 2/6] media: uapi: h264: Add the concept of decoding mode

[Boris Brezillon]

On Mon, 3 Jun 2019 16:05:26 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Mon, Jun 03, 2019 at 02:51:13PM +0200, Boris Brezillon wrote:
> > +Maxime
> > 
> > Oops, just realized Maxime was not Cc-ed on this series.
> > 
> > On Mon, 3 Jun 2019 14:30:20 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> >   
> > > On Mon, Jun 03, 2019 at 01:09:42PM +0200, Boris Brezillon wrote:  
> > > > Some stateless decoders don't support per-slice decoding (or at least
> > > > not in a way that would make them efficient or easy to use).
> > > > Let's expose a menu to control and expose the supported decoding modes.
> > > > Drivers are allowed to support only one decoding but they can support
> > > > both too.
> > > > 
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > > ---
> > > >  .../media/uapi/v4l/ext-ctrls-codec.rst        | 42 ++++++++++++++++++-
> > > >  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ++++
> > > >  include/media/h264-ctrls.h                    | 13 ++++++
> > > >  3 files changed, 63 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > > index 82547d5de250..188f625acb7c 100644
> > > > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > > @@ -1748,6 +1748,14 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > > >      * - __u32
> > > >        - ``size``
> > > >        -
> > > > +    * - __u32
> > > > +      - ``start_byte_offset``
> > > > +      - Where the slice payload starts in the output buffer. Useful when
> > > > +        operating in per frame decoding mode and decoding multi-slice content.
> > > > +        In this case, the output buffer will contain more than one slice and
> > > > +        some codecs need to know where each slice starts. Note that this
> > > > +        offsets points to the beginning of the slice which is supposed to
> > > > +        contain an ANNEX B start code
> > > >      * - __u32
> > > >        - ``header_bit_size``
> > > >        -
> > > > @@ -1931,7 +1939,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > > >        -
> > > >      * - __u16
> > > >        - ``num_slices``
> > > > -      - Number of slices needed to decode the current frame
> > > > +      - Number of slices needed to decode the current frame/field. When
> > > > +        operating in per-slice decoding mode (see
> > > > +        :c:type:`v4l2_mpeg_video_h264_decoding_mode`), this field
> > > > +        should always be set to one
> > > >      * - __u16
> > > >        - ``nal_ref_idc``
> > > >        - NAL reference ID value coming from the NAL Unit header
> > > > @@ -2022,6 +2033,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > > >        - 0x00000004
> > > >        - The DPB entry is a long term reference frame
> > > >  
> > > > +``V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE (enum)``
> > > > +    Specifies the decoding mode to use. Currently exposes per slice and per
> > > > +    frame decoding but new modes might be added later on.
> > > > +
> > > > +    .. note::
> > > > +
> > > > +       This menu control is not yet part of the public kernel API and
> > > > +       it is expected to change.
> > > > +
> > > > +.. c:type:: v4l2_mpeg_video_h264_decoding_mode
> > > > +
> > > > +.. cssclass:: longtable
> > > > +
> > > > +.. flat-table::
> > > > +    :header-rows:  0
> > > > +    :stub-columns: 0
> > > > +    :widths:       1 1 2
> > > > +
> > > > +    * - ``V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE``
> > > > +      - 0
> > > > +      - The decoding is done per slice. v4l2_ctrl_h264_decode_params->num_slices
> > > > +        must be set to 1 and the output buffer should contain only one slice.    
> > > 
> > > I wonder if we need to be that strict. Wouldn't it be possible for
> > > drivers to just iterate over a number of slices and decode each in turn
> > > if userspace passed more than one?
> > > 
> > > Or perhaps a decoder can batch queue a couple of slices. I'm not sure
> > > how frequent this is, but consider something like a spike in activity on
> > > your system, causing some slices to get delayed so you actually get a
> > > few buffered up before you get a chance to hand them to the V4L2 device.
> > > Processing them all at once may help conceal the lag.  
> > 
> > Hm, so we'd be in some kind of slice-batch mode, which means we could
> > trigger a decode operation with more than one slice, but not
> > necessarily all the slices needed to decode a frame. TBH, supporting
> > per-frame (or the batch mode you suggest) on a HW that supports
> > per-slice decoding should be pretty simple and has not real impact on
> > perfs (as you said, it's just a matter of iterating over all the slices
> > attached to a decode operation), so I'm fine relaxing the rule here and
> > patching the cedrus driver accordingly (I can't really test the
> > changes though). Paul, Maxime, what's your opinion?  
> 
> We could perhaps have a test program to orchestrate such a scenario. I
> think the assumption should obviously still be that we don't cross the
> frame boundary using slices in one batch.

We should definitely forbid mixing slices of different frames in the
same decode operation, since each decode operation is targeting a
single capture buffer.

> Just that if a frame was made
> up of, say, 4 slices and you first pass 3 slices, then 1, that it'd be
> nice if the driver would be able to cope with that.

Yep, that makes sense.

> It's something that
> could probably even be implemented in the framework as a helper, though
> I suspect it'd be just a couple of lines of extra code to wrap a loop
> around everything.

I also thought about providing generic wrappers, both for this case and
the per-slice -> per-frame case (this one would be a bit more
complicated as it implies queuing slices in a bounce buffer and
triggering the decode operation only when we have all slices of a
frame).

Re: [PATCH RFC 5/6] media: cedrus: Make the slice_params array size limitation more explicit

[Jernej Škrabec]

Dne ponedeljek, 03. junij 2019 ob 13:09:45 CEST je Boris Brezillon napisal(a):
> The driver only supports per-slice decoding, and in that mode
> decode_params->num_slices must be set to 1 and the slice_params array
> should contain only one element.

What Cedrus actually needs to know is if this is first slice in frame or not. I 
imagine it resets some stuff internally when first slice is processed.

So if driver won't get all slices of one frame at once, it can't know if this 
is first slice in frame or not. I guess we need additional flag for this.

Best regards,
Jernej

> 
> The current code already had this limitation but it made it look like
> the slice_params control was a single struct while, according to the
> spec, it's actually an array. Make it more explicit by setting dims[0]
> and adding a comment explaining why we have this limitation.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Re: [PATCH RFC 5/6] media: cedrus: Make the slice_params array size limitation more explicit

[Nicolas Dufresne]

Le lundi 03 juin 2019 à 23:48 +0200, Jernej Škrabec a écrit :
> Dne ponedeljek, 03. junij 2019 ob 13:09:45 CEST je Boris Brezillon napisal(a):
> > The driver only supports per-slice decoding, and in that mode
> > decode_params->num_slices must be set to 1 and the slice_params array
> > should contain only one element.
> 
> What Cedrus actually needs to know is if this is first slice in frame or not. I 
> imagine it resets some stuff internally when first slice is processed.
> 
> So if driver won't get all slices of one frame at once, it can't know if this 
> is first slice in frame or not. I guess we need additional flag for this.

A first slice of a frame comes with a new timestamp, so you don't need
a flag for that.

regards,
Nicolas

> 
> Best regards,
> Jernej
> 
> > The current code already had this limitation but it made it look like
> > the slice_params control was a single struct while, according to the
> > spec, it's actually an array. Make it more explicit by setting dims[0]
> > and adding a comment explaining why we have this limitation.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> 
> 

Re: [PATCH RFC 5/6] media: cedrus: Make the slice_params array size limitation more explicit

[Thierry Reding]

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

On Mon, Jun 03, 2019 at 07:55:48PM -0400, Nicolas Dufresne wrote:
> Le lundi 03 juin 2019 à 23:48 +0200, Jernej Škrabec a écrit :
> > Dne ponedeljek, 03. junij 2019 ob 13:09:45 CEST je Boris Brezillon napisal(a):
> > > The driver only supports per-slice decoding, and in that mode
> > > decode_params->num_slices must be set to 1 and the slice_params array
> > > should contain only one element.
> > 
> > What Cedrus actually needs to know is if this is first slice in frame or not. I 
> > imagine it resets some stuff internally when first slice is processed.
> > 
> > So if driver won't get all slices of one frame at once, it can't know if this 
> > is first slice in frame or not. I guess we need additional flag for this.
> 
> A first slice of a frame comes with a new timestamp, so you don't need
> a flag for that.

But slices for the same frame will all have the same timestamp, so we
can't use the timestamp to tell the individual slices apart.

I mentioned this in that other thread, but I think it'd be useful to
pass along the number of each of the slices. Drivers can use this in
order to conceal errors when corrupt slices are detected during the
decode operation.

So if we also passed a slice index along with the offset of the slice in
the bitstream, that should give us enough information to achieve both. A
slice with index 0 is obviously going to be the first slice in a frame.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC 2/6] media: uapi: h264: Add the concept of decoding mode

[Thierry Reding]

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

On Mon, Jun 03, 2019 at 05:37:11PM +0200, Boris Brezillon wrote:
> On Mon, 3 Jun 2019 16:05:26 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Mon, Jun 03, 2019 at 02:51:13PM +0200, Boris Brezillon wrote:
> > > +Maxime
> > > 
> > > Oops, just realized Maxime was not Cc-ed on this series.
> > > 
> > > On Mon, 3 Jun 2019 14:30:20 +0200
> > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > >   
> > > > On Mon, Jun 03, 2019 at 01:09:42PM +0200, Boris Brezillon wrote:  
> > > > > Some stateless decoders don't support per-slice decoding (or at least
> > > > > not in a way that would make them efficient or easy to use).
> > > > > Let's expose a menu to control and expose the supported decoding modes.
> > > > > Drivers are allowed to support only one decoding but they can support
> > > > > both too.
> > > > > 
> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > ---
> > > > >  .../media/uapi/v4l/ext-ctrls-codec.rst        | 42 ++++++++++++++++++-
> > > > >  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ++++
> > > > >  include/media/h264-ctrls.h                    | 13 ++++++
> > > > >  3 files changed, 63 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > > > index 82547d5de250..188f625acb7c 100644
> > > > > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > > > @@ -1748,6 +1748,14 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > > > >      * - __u32
> > > > >        - ``size``
> > > > >        -
> > > > > +    * - __u32
> > > > > +      - ``start_byte_offset``
> > > > > +      - Where the slice payload starts in the output buffer. Useful when
> > > > > +        operating in per frame decoding mode and decoding multi-slice content.
> > > > > +        In this case, the output buffer will contain more than one slice and
> > > > > +        some codecs need to know where each slice starts. Note that this
> > > > > +        offsets points to the beginning of the slice which is supposed to
> > > > > +        contain an ANNEX B start code
> > > > >      * - __u32
> > > > >        - ``header_bit_size``
> > > > >        -
> > > > > @@ -1931,7 +1939,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > > > >        -
> > > > >      * - __u16
> > > > >        - ``num_slices``
> > > > > -      - Number of slices needed to decode the current frame
> > > > > +      - Number of slices needed to decode the current frame/field. When
> > > > > +        operating in per-slice decoding mode (see
> > > > > +        :c:type:`v4l2_mpeg_video_h264_decoding_mode`), this field
> > > > > +        should always be set to one
> > > > >      * - __u16
> > > > >        - ``nal_ref_idc``
> > > > >        - NAL reference ID value coming from the NAL Unit header
> > > > > @@ -2022,6 +2033,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > > > >        - 0x00000004
> > > > >        - The DPB entry is a long term reference frame
> > > > >  
> > > > > +``V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE (enum)``
> > > > > +    Specifies the decoding mode to use. Currently exposes per slice and per
> > > > > +    frame decoding but new modes might be added later on.
> > > > > +
> > > > > +    .. note::
> > > > > +
> > > > > +       This menu control is not yet part of the public kernel API and
> > > > > +       it is expected to change.
> > > > > +
> > > > > +.. c:type:: v4l2_mpeg_video_h264_decoding_mode
> > > > > +
> > > > > +.. cssclass:: longtable
> > > > > +
> > > > > +.. flat-table::
> > > > > +    :header-rows:  0
> > > > > +    :stub-columns: 0
> > > > > +    :widths:       1 1 2
> > > > > +
> > > > > +    * - ``V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE``
> > > > > +      - 0
> > > > > +      - The decoding is done per slice. v4l2_ctrl_h264_decode_params->num_slices
> > > > > +        must be set to 1 and the output buffer should contain only one slice.    
> > > > 
> > > > I wonder if we need to be that strict. Wouldn't it be possible for
> > > > drivers to just iterate over a number of slices and decode each in turn
> > > > if userspace passed more than one?
> > > > 
> > > > Or perhaps a decoder can batch queue a couple of slices. I'm not sure
> > > > how frequent this is, but consider something like a spike in activity on
> > > > your system, causing some slices to get delayed so you actually get a
> > > > few buffered up before you get a chance to hand them to the V4L2 device.
> > > > Processing them all at once may help conceal the lag.  
> > > 
> > > Hm, so we'd be in some kind of slice-batch mode, which means we could
> > > trigger a decode operation with more than one slice, but not
> > > necessarily all the slices needed to decode a frame. TBH, supporting
> > > per-frame (or the batch mode you suggest) on a HW that supports
> > > per-slice decoding should be pretty simple and has not real impact on
> > > perfs (as you said, it's just a matter of iterating over all the slices
> > > attached to a decode operation), so I'm fine relaxing the rule here and
> > > patching the cedrus driver accordingly (I can't really test the
> > > changes though). Paul, Maxime, what's your opinion?  
> > 
> > We could perhaps have a test program to orchestrate such a scenario. I
> > think the assumption should obviously still be that we don't cross the
> > frame boundary using slices in one batch.
> 
> We should definitely forbid mixing slices of different frames in the
> same decode operation, since each decode operation is targeting a
> single capture buffer.
> 
> > Just that if a frame was made
> > up of, say, 4 slices and you first pass 3 slices, then 1, that it'd be
> > nice if the driver would be able to cope with that.
> 
> Yep, that makes sense.
> 
> > It's something that
> > could probably even be implemented in the framework as a helper, though
> > I suspect it'd be just a couple of lines of extra code to wrap a loop
> > around everything.
> 
> I also thought about providing generic wrappers, both for this case and
> the per-slice -> per-frame case (this one would be a bit more
> complicated as it implies queuing slices in a bounce buffer and
> triggering the decode operation only when we have all slices of a
> frame).

I like deferring the addition of that kind of helper until a clear
pattern emerges out of the drivers that need this, just because that
gives us real examples on which to model those helpers. But yeah, I
think it should be possible to have helpers for these for most cases.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC 5/6] media: cedrus: Make the slice_params array size limitation more explicit

[Boris Brezillon]

On Tue, 4 Jun 2019 10:12:10 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Mon, Jun 03, 2019 at 07:55:48PM -0400, Nicolas Dufresne wrote:
> > Le lundi 03 juin 2019 à 23:48 +0200, Jernej Škrabec a écrit :  
> > > Dne ponedeljek, 03. junij 2019 ob 13:09:45 CEST je Boris Brezillon napisal(a):  
> > > > The driver only supports per-slice decoding, and in that mode
> > > > decode_params->num_slices must be set to 1 and the slice_params array
> > > > should contain only one element.  
> > > 
> > > What Cedrus actually needs to know is if this is first slice in frame or not. I 
> > > imagine it resets some stuff internally when first slice is processed.
> > > 
> > > So if driver won't get all slices of one frame at once, it can't know if this 
> > > is first slice in frame or not. I guess we need additional flag for this.  
> > 
> > A first slice of a frame comes with a new timestamp, so you don't need
> > a flag for that.  
> 
> But slices for the same frame will all have the same timestamp, so we
> can't use the timestamp to tell the individual slices apart.

I think Nicolas was suggesting to keep the last decoded slice timestamp
around and check it against the new slice one. If they are different,
that means we are dealing with a new frame, and this slice is the first
in the frame.

> 
> I mentioned this in that other thread, but I think it'd be useful to
> pass along the number of each of the slices. Drivers can use this in
> order to conceal errors when corrupt slices are detected during the
> decode operation.
> 
> So if we also passed a slice index along with the offset of the slice in
> the bitstream, that should give us enough information to achieve both. A
> slice with index 0 is obviously going to be the first slice in a frame.

There's also the ->first_mb_in_slice field which should be 0 for the
first slice and > 0 for others assuming userspace pass slices in order
(might be an issue if we want to support ASO).

Re: [PATCH RFC 5/6] media: cedrus: Make the slice_params array size limitation more explicit

[Nicolas Dufresne]

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

Le mardi 04 juin 2019 à 10:12 +0200, Thierry Reding a écrit :
> On Mon, Jun 03, 2019 at 07:55:48PM -0400, Nicolas Dufresne wrote:
> > Le lundi 03 juin 2019 à 23:48 +0200, Jernej Škrabec a écrit :
> > > Dne ponedeljek, 03. junij 2019 ob 13:09:45 CEST je Boris Brezillon napisal(a):
> > > > The driver only supports per-slice decoding, and in that mode
> > > > decode_params->num_slices must be set to 1 and the slice_params array
> > > > should contain only one element.
> > > 
> > > What Cedrus actually needs to know is if this is first slice in frame or not. I 
> > > imagine it resets some stuff internally when first slice is processed.
> > > 
> > > So if driver won't get all slices of one frame at once, it can't know if this 
> > > is first slice in frame or not. I guess we need additional flag for this.
> > 
> > A first slice of a frame comes with a new timestamp, so you don't need
> > a flag for that.
> 
> But slices for the same frame will all have the same timestamp, so we
> can't use the timestamp to tell the individual slices apart.
> 
> I mentioned this in that other thread, but I think it'd be useful to
> pass along the number of each of the slices. Drivers can use this in
> order to conceal errors when corrupt slices are detected during the
> decode operation.

This is already passed as this is part of the slice header that we both
pass and parse to structure. Each slice have it's first MB indicated
(that standard to H264) and you can deduce the lost slice from that.

> 
> So if we also passed a slice index along with the offset of the slice in
> the bitstream, that should give us enough information to achieve both. A
> slice with index 0 is obviously going to be the first slice in a frame.

We do this in per-frame mode only. The offset of the slice in the
bitstream is always 0 in per-slice mode, since each v4l2 input buffer
is a slice.

> 
> Thierry

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

Re: [PATCH RFC 2/6] media: uapi: h264: Add the concept of decoding mode

[Paul Kocialkowski]

Hi,

Le mardi 04 juin 2019 à 10:16 +0200, Thierry Reding a écrit :
> On Mon, Jun 03, 2019 at 05:37:11PM +0200, Boris Brezillon wrote:
> > On Mon, 3 Jun 2019 16:05:26 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> > 
> > > On Mon, Jun 03, 2019 at 02:51:13PM +0200, Boris Brezillon wrote:
> > > > +Maxime
> > > > 
> > > > Oops, just realized Maxime was not Cc-ed on this series.
> > > > 
> > > > On Mon, 3 Jun 2019 14:30:20 +0200
> > > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > >   
> > > > > On Mon, Jun 03, 2019 at 01:09:42PM +0200, Boris Brezillon wrote:  
> > > > > > Some stateless decoders don't support per-slice decoding (or at least
> > > > > > not in a way that would make them efficient or easy to use).
> > > > > > Let's expose a menu to control and expose the supported decoding modes.
> > > > > > Drivers are allowed to support only one decoding but they can support
> > > > > > both too.
> > > > > > 
> > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > > ---
> > > > > >  .../media/uapi/v4l/ext-ctrls-codec.rst        | 42 ++++++++++++++++++-
> > > > > >  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ++++
> > > > > >  include/media/h264-ctrls.h                    | 13 ++++++
> > > > > >  3 files changed, 63 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > > > > index 82547d5de250..188f625acb7c 100644
> > > > > > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > > > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > > > > @@ -1748,6 +1748,14 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > > > > >      * - __u32
> > > > > >        - ``size``
> > > > > >        -
> > > > > > +    * - __u32
> > > > > > +      - ``start_byte_offset``
> > > > > > +      - Where the slice payload starts in the output buffer. Useful when
> > > > > > +        operating in per frame decoding mode and decoding multi-slice content.
> > > > > > +        In this case, the output buffer will contain more than one slice and
> > > > > > +        some codecs need to know where each slice starts. Note that this
> > > > > > +        offsets points to the beginning of the slice which is supposed to
> > > > > > +        contain an ANNEX B start code
> > > > > >      * - __u32
> > > > > >        - ``header_bit_size``
> > > > > >        -
> > > > > > @@ -1931,7 +1939,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > > > > >        -
> > > > > >      * - __u16
> > > > > >        - ``num_slices``
> > > > > > -      - Number of slices needed to decode the current frame
> > > > > > +      - Number of slices needed to decode the current frame/field. When
> > > > > > +        operating in per-slice decoding mode (see
> > > > > > +        :c:type:`v4l2_mpeg_video_h264_decoding_mode`), this field
> > > > > > +        should always be set to one
> > > > > >      * - __u16
> > > > > >        - ``nal_ref_idc``
> > > > > >        - NAL reference ID value coming from the NAL Unit header
> > > > > > @@ -2022,6 +2033,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > > > > >        - 0x00000004
> > > > > >        - The DPB entry is a long term reference frame
> > > > > >  
> > > > > > +``V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE (enum)``
> > > > > > +    Specifies the decoding mode to use. Currently exposes per slice and per
> > > > > > +    frame decoding but new modes might be added later on.
> > > > > > +
> > > > > > +    .. note::
> > > > > > +
> > > > > > +       This menu control is not yet part of the public kernel API and
> > > > > > +       it is expected to change.
> > > > > > +
> > > > > > +.. c:type:: v4l2_mpeg_video_h264_decoding_mode
> > > > > > +
> > > > > > +.. cssclass:: longtable
> > > > > > +
> > > > > > +.. flat-table::
> > > > > > +    :header-rows:  0
> > > > > > +    :stub-columns: 0
> > > > > > +    :widths:       1 1 2
> > > > > > +
> > > > > > +    * - ``V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE``
> > > > > > +      - 0
> > > > > > +      - The decoding is done per slice. v4l2_ctrl_h264_decode_params->num_slices
> > > > > > +        must be set to 1 and the output buffer should contain only one slice.    
> > > > > 
> > > > > I wonder if we need to be that strict. Wouldn't it be possible for
> > > > > drivers to just iterate over a number of slices and decode each in turn
> > > > > if userspace passed more than one?
> > > > > 
> > > > > Or perhaps a decoder can batch queue a couple of slices. I'm not sure
> > > > > how frequent this is, but consider something like a spike in activity on
> > > > > your system, causing some slices to get delayed so you actually get a
> > > > > few buffered up before you get a chance to hand them to the V4L2 device.
> > > > > Processing them all at once may help conceal the lag.  
> > > > 
> > > > Hm, so we'd be in some kind of slice-batch mode, which means we could
> > > > trigger a decode operation with more than one slice, but not
> > > > necessarily all the slices needed to decode a frame. TBH, supporting
> > > > per-frame (or the batch mode you suggest) on a HW that supports
> > > > per-slice decoding should be pretty simple and has not real impact on
> > > > perfs (as you said, it's just a matter of iterating over all the slices
> > > > attached to a decode operation), so I'm fine relaxing the rule here and
> > > > patching the cedrus driver accordingly (I can't really test the
> > > > changes though). Paul, Maxime, what's your opinion?  

So perhaps we could just allow passing any number of slices with each
request within a frame boundary and have userspace set the
V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF flag accordingly.

I'm not totally sure we need a batching method beyond that, just
submitting requests with groups of slices and the same destination
buffer should work out fine. We can leave it up to the drivers to
handle multiple slices submitted at once.

> > > We could perhaps have a test program to orchestrate such a scenario. I
> > > think the assumption should obviously still be that we don't cross the
> > > frame boundary using slices in one batch.
> > 
> > We should definitely forbid mixing slices of different frames in the
> > same decode operation, since each decode operation is targeting a
> > single capture buffer.

Agreed.

> > > Just that if a frame was made
> > > up of, say, 4 slices and you first pass 3 slices, then 1, that it'd be
> > > nice if the driver would be able to cope with that.
> > 
> > Yep, that makes sense.
> > 
> > > It's something that
> > > could probably even be implemented in the framework as a helper, though
> > > I suspect it'd be just a couple of lines of extra code to wrap a loop
> > > around everything.
> > 
> > I also thought about providing generic wrappers, both for this case and
> > the per-slice -> per-frame case (this one would be a bit more
> > complicated as it implies queuing slices in a bounce buffer and
> > triggering the decode operation only when we have all slices of a
> > frame).
> 
> I like deferring the addition of that kind of helper until a clear
> pattern emerges out of the drivers that need this, just because that
> gives us real examples on which to model those helpers. But yeah, I
> think it should be possible to have helpers for these for most cases.

So it seems that we need some convenient way to iterate over each slice
and configure registers accordingly. Perhaps we could have some common
per-codec helpers with callbacks to set the common controls and iterate
over the per-slice controls.

That would help keep our drivers tidy and understandable, and maybe we
could have start/stop steps too, pretty much like we do in cedrus.

What do you think?

Cheers,

Paul

Re: [PATCH RFC 2/6] media: uapi: h264: Add the concept of decoding mode

[Paul Kocialkowski]

Hi,

Le lundi 03 juin 2019 à 13:09 +0200, Boris Brezillon a écrit :
> Some stateless decoders don't support per-slice decoding (or at least
> not in a way that would make them efficient or easy to use).
> Let's expose a menu to control and expose the supported decoding modes.
> Drivers are allowed to support only one decoding but they can support
> both too.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  .../media/uapi/v4l/ext-ctrls-codec.rst        | 42 ++++++++++++++++++-
>  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ++++
>  include/media/h264-ctrls.h                    | 13 ++++++
>  3 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> index 82547d5de250..188f625acb7c 100644
> --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> @@ -1748,6 +1748,14 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      * - __u32
>        - ``size``
>        -
> +    * - __u32
> +      - ``start_byte_offset``
> +      - Where the slice payload starts in the output buffer. Useful when
> +        operating in per frame decoding mode and decoding multi-slice content.
> +        In this case, the output buffer will contain more than one slice and
> +        some codecs need to know where each slice starts. Note that this
> +        offsets points to the beginning of the slice which is supposed to
> +        contain an ANNEX B start code

Looks good, maybe add a terminating dot.

>      * - __u32
>        - ``header_bit_size``
>        -
> @@ -1931,7 +1939,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>        -
>      * - __u16
>        - ``num_slices``
> -      - Number of slices needed to decode the current frame
> +      - Number of slices needed to decode the current frame/field. When
> +        operating in per-slice decoding mode (see
> +        :c:type:`v4l2_mpeg_video_h264_decoding_mode`), this field
> +        should always be set to one

So maybe we should allow an arbitrary number of slices unless only per-
frame decoding is selected and we need all the slices at once.

>      * - __u16
>        - ``nal_ref_idc``
>        - NAL reference ID value coming from the NAL Unit header
> @@ -2022,6 +2033,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>        - 0x00000004
>        - The DPB entry is a long term reference frame
>  
> +``V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE (enum)``
> +    Specifies the decoding mode to use. Currently exposes per slice and per
> +    frame decoding but new modes might be added later on.

I think it definitey makes sense to have this per-codec.

> +
> +    .. note::
> +
> +       This menu control is not yet part of the public kernel API and
> +       it is expected to change.
> +
> +.. c:type:: v4l2_mpeg_video_h264_decoding_mode
> +
> +.. cssclass:: longtable
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       1 1 2
> +
> +    * - ``V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE``
> +      - 0
> +      - The decoding is done per slice. v4l2_ctrl_h264_decode_params->num_slices
> +        must be set to 1 and the output buffer should contain only one slice.

See above about having arbitrary numbers of slices (within frame
boundary).

> +    * - ``V4L2_MPEG_VIDEO_H264_DECODING_PER_FRAME``
> +      - 1
> +      - The decoding is done per frame. v4l2_ctrl_h264_decode_params->num_slices
> +        can be > 1. When that happens, the output buffer should contain all
> +        slices needed to decode a frame/field, each slice being prefixed by an
> +        Annex B NAL header/start-code.

Looks good!

> +
>  .. _v4l2-mpeg-mpeg2:
>  
>  ``V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS (struct)``
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 1217d38ea394..72bb3c8882f5 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -406,6 +406,11 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"Explicit",
>  		NULL,
>  	};
> +	static const char * const h264_decoding_mode[] = {
> +		"Per Slice",
> +		"Per Frame",
> +		NULL,
> +	};
>  	static const char * const mpeg_mpeg2_level[] = {
>  		"Low",
>  		"Main",
> @@ -637,6 +642,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		return h264_fp_arrangement_type;
>  	case V4L2_CID_MPEG_VIDEO_H264_FMO_MAP_TYPE:
>  		return h264_fmo_map_type;
> +	case V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE:
> +		return h264_decoding_mode;
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL:
>  		return mpeg_mpeg2_level;
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_PROFILE:
> @@ -856,6 +863,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX:		return "H264 Scaling Matrix";
>  	case V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS:		return "H264 Slice Parameters";
>  	case V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS:		return "H264 Decode Parameters";
> +	case V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE:		return "H264 Decoding Mode";
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL:			return "MPEG2 Level";
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_PROFILE:			return "MPEG2 Profile";
>  	case V4L2_CID_MPEG_VIDEO_MPEG4_I_FRAME_QP:		return "MPEG4 I-Frame QP Value";
> @@ -1224,6 +1232,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_MPEG_VIDEO_H264_VUI_SAR_IDC:
>  	case V4L2_CID_MPEG_VIDEO_H264_SEI_FP_ARRANGEMENT_TYPE:
>  	case V4L2_CID_MPEG_VIDEO_H264_FMO_MAP_TYPE:
> +	case V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE:
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL:
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_PROFILE:
>  	case V4L2_CID_MPEG_VIDEO_MPEG4_LEVEL:
> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> index e1404d78d6ff..26de2243f6f5 100644
> --- a/include/media/h264-ctrls.h
> +++ b/include/media/h264-ctrls.h
> @@ -26,6 +26,7 @@
>  #define V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX	(V4L2_CID_MPEG_BASE+1002)
>  #define V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS	(V4L2_CID_MPEG_BASE+1003)
>  #define V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS	(V4L2_CID_MPEG_BASE+1004)
> +#define V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE	(V4L2_CID_MPEG_BASE+1005)
>  
>  /* enum v4l2_ctrl_type type values */
>  #define V4L2_CTRL_TYPE_H264_SPS			0x0110
> @@ -33,6 +34,12 @@
>  #define V4L2_CTRL_TYPE_H264_SCALING_MATRIX	0x0112
>  #define V4L2_CTRL_TYPE_H264_SLICE_PARAMS	0x0113
>  #define V4L2_CTRL_TYPE_H264_DECODE_PARAMS	0x0114
> +#define V4L2_CTRL_TYPE_H264_DECODING_MODE	0x0115
> +
> +enum v4l2_mpeg_video_h264_decoding_mode {
> +	V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE,
> +	V4L2_MPEG_VIDEO_H264_DECODING_PER_FRAME,
> +};
>  
>  #define V4L2_H264_SPS_CONSTRAINT_SET0_FLAG			0x01
>  #define V4L2_H264_SPS_CONSTRAINT_SET1_FLAG			0x02
> @@ -111,6 +118,8 @@ struct v4l2_h264_pred_weight_table {
>  	struct v4l2_h264_weight_factors weight_factors[2];
>  };
>  
> +#define V4L2_H264_MAX_SLICES_PER_FRAME			16
> +
>  #define V4L2_H264_SLICE_TYPE_P				0
>  #define V4L2_H264_SLICE_TYPE_B				1
>  #define V4L2_H264_SLICE_TYPE_I				2
> @@ -125,6 +134,10 @@ struct v4l2_h264_pred_weight_table {
>  struct v4l2_ctrl_h264_slice_params {
>  	/* Size in bytes, including header */
>  	__u32 size;
> +
> +	/* Where the slice starts in the output buffer (expressed in bytes). */

Maybe call it "beginning of the slice" or adapt the following comment
to use the same terminology.

Looks good otherwise, thanks!

Cheers,

Paul

> +	__u32 start_byte_offset;
> +
>  	/* Offset in bits to slice_data() from the beginning of this slice. */
>  	__u32 header_bit_size;
>  

Re: [PATCH RFC 4/6] media: cedrus: Prepare things to support !compound controls

[Paul Kocialkowski]

Hi,

Le lundi 03 juin 2019 à 13:09 +0200, Boris Brezillon a écrit :
> We are about to add a menu control, so let's make the code more generic
> to support other control types.

Did you have a chance to test this or should I better give it a spin
before adding my Reviewed-By?

Cheers,

Paul

> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/staging/media/sunxi/cedrus/cedrus.c | 47 ++++++++++++---------
>  drivers/staging/media/sunxi/cedrus/cedrus.h |  3 +-
>  2 files changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
> index 370937edfc14..378032fe71f9 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -29,44 +29,44 @@
>  
>  static const struct cedrus_control cedrus_controls[] = {
>  	{
> -		.id		= V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS,
> -		.elem_size	= sizeof(struct v4l2_ctrl_mpeg2_slice_params),
> +		.cfg.id		= V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS,
> +		.cfg.elem_size	= sizeof(struct v4l2_ctrl_mpeg2_slice_params),
>  		.codec		= CEDRUS_CODEC_MPEG2,
>  		.required	= true,
>  	},
>  	{
> -		.id		= V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION,
> -		.elem_size	= sizeof(struct v4l2_ctrl_mpeg2_quantization),
> +		.cfg.id		= V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION,
> +		.cfg.elem_size	= sizeof(struct v4l2_ctrl_mpeg2_quantization),
>  		.codec		= CEDRUS_CODEC_MPEG2,
>  		.required	= false,
>  	},
>  	{
> -		.id		= V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS,
> -		.elem_size	= sizeof(struct v4l2_ctrl_h264_decode_params),
> +		.cfg.id		= V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS,
> +		.cfg.elem_size	= sizeof(struct v4l2_ctrl_h264_decode_params),
>  		.codec		= CEDRUS_CODEC_H264,
>  		.required	= true,
>  	},
>  	{
> -		.id		= V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS,
> -		.elem_size	= sizeof(struct v4l2_ctrl_h264_slice_params),
> +		.cfg.id		= V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS,
> +		.cfg.elem_size	= sizeof(struct v4l2_ctrl_h264_slice_params),
>  		.codec		= CEDRUS_CODEC_H264,
>  		.required	= true,
>  	},
>  	{
> -		.id		= V4L2_CID_MPEG_VIDEO_H264_SPS,
> -		.elem_size	= sizeof(struct v4l2_ctrl_h264_sps),
> +		.cfg.id		= V4L2_CID_MPEG_VIDEO_H264_SPS,
> +		.cfg.elem_size	= sizeof(struct v4l2_ctrl_h264_sps),
>  		.codec		= CEDRUS_CODEC_H264,
>  		.required	= true,
>  	},
>  	{
> -		.id		= V4L2_CID_MPEG_VIDEO_H264_PPS,
> -		.elem_size	= sizeof(struct v4l2_ctrl_h264_pps),
> +		.cfg.id		= V4L2_CID_MPEG_VIDEO_H264_PPS,
> +		.cfg.elem_size	= sizeof(struct v4l2_ctrl_h264_pps),
>  		.codec		= CEDRUS_CODEC_H264,
>  		.required	= true,
>  	},
>  	{
> -		.id		= V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX,
> -		.elem_size	= sizeof(struct v4l2_ctrl_h264_scaling_matrix),
> +		.cfg.id		= V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX,
> +		.cfg.elem_size	= sizeof(struct v4l2_ctrl_h264_scaling_matrix),
>  		.codec		= CEDRUS_CODEC_H264,
>  		.required	= true,
>  	},
> @@ -106,12 +106,21 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev, struct cedrus_ctx *ctx)
>  		return -ENOMEM;
>  
>  	for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {
> -		struct v4l2_ctrl_config cfg = {};
> +		const struct v4l2_ctrl_config *cfg = &cedrus_controls[i].cfg;
>  
> -		cfg.elem_size = cedrus_controls[i].elem_size;
> -		cfg.id = cedrus_controls[i].id;
> +		if (cfg->elem_size)
> +			ctrl = v4l2_ctrl_new_custom(hdl, cfg, NULL);
> +		else if (cfg->type == V4L2_CTRL_TYPE_MENU ||
> +			   cfg->type == V4L2_CTRL_TYPE_INTEGER_MENU)
> +			ctrl = v4l2_ctrl_new_std_menu(hdl, NULL,
> +						      cfg->id, cfg->max,
> +						      cfg->menu_skip_mask,
> +						      cfg->def);
> +		else
> +			ctrl = v4l2_ctrl_new_std(hdl, NULL, cfg->id, cfg->min,
> +						 cfg->max, cfg->step,
> +						 cfg->def);
>  
> -		ctrl = v4l2_ctrl_new_custom(hdl, &cfg, NULL);
>  		if (hdl->error) {
>  			v4l2_err(&dev->v4l2_dev,
>  				 "Failed to create new custom control\n");
> @@ -178,7 +187,7 @@ static int cedrus_request_validate(struct media_request *req)
>  			continue;
>  
>  		ctrl_test = v4l2_ctrl_request_hdl_ctrl_find(hdl,
> -							    cedrus_controls[i].id);
> +							    cedrus_controls[i].cfg.id);
>  		if (!ctrl_test) {
>  			v4l2_info(&ctx->dev->v4l2_dev,
>  				  "Missing required codec control\n");
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
> index 3f476d0fd981..69c037724d93 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> @@ -49,8 +49,7 @@ enum cedrus_h264_pic_type {
>  };
>  
>  struct cedrus_control {
> -	u32			id;
> -	u32			elem_size;
> +	struct v4l2_ctrl_config	cfg;
>  	enum cedrus_codec	codec;
>  	unsigned char		required:1;
>  };

Re: [PATCH RFC 5/6] media: cedrus: Make the slice_params array size limitation more explicit

[Paul Kocialkowski]

Hi,

Le mardi 04 juin 2019 à 10:31 -0400, Nicolas Dufresne a écrit :
> Le mardi 04 juin 2019 à 10:12 +0200, Thierry Reding a écrit :
> > On Mon, Jun 03, 2019 at 07:55:48PM -0400, Nicolas Dufresne wrote:
> > > Le lundi 03 juin 2019 à 23:48 +0200, Jernej Škrabec a écrit :
> > > > Dne ponedeljek, 03. junij 2019 ob 13:09:45 CEST je Boris Brezillon napisal(a):
> > > > > The driver only supports per-slice decoding, and in that mode
> > > > > decode_params->num_slices must be set to 1 and the slice_params array
> > > > > should contain only one element.
> > > > 
> > > > What Cedrus actually needs to know is if this is first slice in frame or not. I 
> > > > imagine it resets some stuff internally when first slice is processed.
> > > > 
> > > > So if driver won't get all slices of one frame at once, it can't know if this 
> > > > is first slice in frame or not. I guess we need additional flag for this.
> > > 
> > > A first slice of a frame comes with a new timestamp, so you don't need
> > > a flag for that.
> > 
> > But slices for the same frame will all have the same timestamp, so we
> > can't use the timestamp to tell the individual slices apart.
> > 
> > I mentioned this in that other thread, but I think it'd be useful to
> > pass along the number of each of the slices. Drivers can use this in
> > order to conceal errors when corrupt slices are detected during the
> > decode operation.
> 
> This is already passed as this is part of the slice header that we both
> pass and parse to structure. Each slice have it's first MB indicated
> (that standard to H264) and you can deduce the lost slice from that.
> 
> > So if we also passed a slice index along with the offset of the slice in
> > the bitstream, that should give us enough information to achieve both. A
> > slice with index 0 is obviously going to be the first slice in a frame.
> 
> We do this in per-frame mode only. The offset of the slice in the
> bitstream is always 0 in per-slice mode, since each v4l2 input buffer
> is a slice.

I don't think we need a slice index either, we most likely already have
enough information to know where we're at regarding slices position.

But how about allowing an arbitrary number of slices within frame
boundary in per-slice decoding mode?

Cheers,

Paul

Re: [PATCH RFC 2/6] media: uapi: h264: Add the concept of decoding mode

[Boris Brezillon]

On Wed, 05 Jun 2019 22:48:08 +0200
Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote:

> Hi,
> 
> Le mardi 04 juin 2019 à 10:16 +0200, Thierry Reding a écrit :
> > On Mon, Jun 03, 2019 at 05:37:11PM +0200, Boris Brezillon wrote:  
> > > On Mon, 3 Jun 2019 16:05:26 +0200
> > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > >   
> > > > On Mon, Jun 03, 2019 at 02:51:13PM +0200, Boris Brezillon wrote:  
> > > > > +Maxime
> > > > > 
> > > > > Oops, just realized Maxime was not Cc-ed on this series.
> > > > > 
> > > > > On Mon, 3 Jun 2019 14:30:20 +0200
> > > > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > >     
> > > > > > On Mon, Jun 03, 2019 at 01:09:42PM +0200, Boris Brezillon wrote:    
> > > > > > > Some stateless decoders don't support per-slice decoding (or at least
> > > > > > > not in a way that would make them efficient or easy to use).
> > > > > > > Let's expose a menu to control and expose the supported decoding modes.
> > > > > > > Drivers are allowed to support only one decoding but they can support
> > > > > > > both too.
> > > > > > > 
> > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > > > ---
> > > > > > >  .../media/uapi/v4l/ext-ctrls-codec.rst        | 42 ++++++++++++++++++-
> > > > > > >  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ++++
> > > > > > >  include/media/h264-ctrls.h                    | 13 ++++++
> > > > > > >  3 files changed, 63 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > > > > > index 82547d5de250..188f625acb7c 100644
> > > > > > > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > > > > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > > > > > > @@ -1748,6 +1748,14 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > > > > > >      * - __u32
> > > > > > >        - ``size``
> > > > > > >        -
> > > > > > > +    * - __u32
> > > > > > > +      - ``start_byte_offset``
> > > > > > > +      - Where the slice payload starts in the output buffer. Useful when
> > > > > > > +        operating in per frame decoding mode and decoding multi-slice content.
> > > > > > > +        In this case, the output buffer will contain more than one slice and
> > > > > > > +        some codecs need to know where each slice starts. Note that this
> > > > > > > +        offsets points to the beginning of the slice which is supposed to
> > > > > > > +        contain an ANNEX B start code
> > > > > > >      * - __u32
> > > > > > >        - ``header_bit_size``
> > > > > > >        -
> > > > > > > @@ -1931,7 +1939,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > > > > > >        -
> > > > > > >      * - __u16
> > > > > > >        - ``num_slices``
> > > > > > > -      - Number of slices needed to decode the current frame
> > > > > > > +      - Number of slices needed to decode the current frame/field. When
> > > > > > > +        operating in per-slice decoding mode (see
> > > > > > > +        :c:type:`v4l2_mpeg_video_h264_decoding_mode`), this field
> > > > > > > +        should always be set to one
> > > > > > >      * - __u16
> > > > > > >        - ``nal_ref_idc``
> > > > > > >        - NAL reference ID value coming from the NAL Unit header
> > > > > > > @@ -2022,6 +2033,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> > > > > > >        - 0x00000004
> > > > > > >        - The DPB entry is a long term reference frame
> > > > > > >  
> > > > > > > +``V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE (enum)``
> > > > > > > +    Specifies the decoding mode to use. Currently exposes per slice and per
> > > > > > > +    frame decoding but new modes might be added later on.
> > > > > > > +
> > > > > > > +    .. note::
> > > > > > > +
> > > > > > > +       This menu control is not yet part of the public kernel API and
> > > > > > > +       it is expected to change.
> > > > > > > +
> > > > > > > +.. c:type:: v4l2_mpeg_video_h264_decoding_mode
> > > > > > > +
> > > > > > > +.. cssclass:: longtable
> > > > > > > +
> > > > > > > +.. flat-table::
> > > > > > > +    :header-rows:  0
> > > > > > > +    :stub-columns: 0
> > > > > > > +    :widths:       1 1 2
> > > > > > > +
> > > > > > > +    * - ``V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE``
> > > > > > > +      - 0
> > > > > > > +      - The decoding is done per slice. v4l2_ctrl_h264_decode_params->num_slices
> > > > > > > +        must be set to 1 and the output buffer should contain only one slice.      
> > > > > > 
> > > > > > I wonder if we need to be that strict. Wouldn't it be possible for
> > > > > > drivers to just iterate over a number of slices and decode each in turn
> > > > > > if userspace passed more than one?
> > > > > > 
> > > > > > Or perhaps a decoder can batch queue a couple of slices. I'm not sure
> > > > > > how frequent this is, but consider something like a spike in activity on
> > > > > > your system, causing some slices to get delayed so you actually get a
> > > > > > few buffered up before you get a chance to hand them to the V4L2 device.
> > > > > > Processing them all at once may help conceal the lag.    
> > > > > 
> > > > > Hm, so we'd be in some kind of slice-batch mode, which means we could
> > > > > trigger a decode operation with more than one slice, but not
> > > > > necessarily all the slices needed to decode a frame. TBH, supporting
> > > > > per-frame (or the batch mode you suggest) on a HW that supports
> > > > > per-slice decoding should be pretty simple and has not real impact on
> > > > > perfs (as you said, it's just a matter of iterating over all the slices
> > > > > attached to a decode operation), so I'm fine relaxing the rule here and
> > > > > patching the cedrus driver accordingly (I can't really test the
> > > > > changes though). Paul, Maxime, what's your opinion?    
> 
> So perhaps we could just allow passing any number of slices with each
> request within a frame boundary and have userspace set the
> V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF flag accordingly.
> 
> I'm not totally sure we need a batching method beyond that, just
> submitting requests with groups of slices and the same destination
> buffer should work out fine. We can leave it up to the drivers to
> handle multiple slices submitted at once.

The batching mode we're talking about is exactly that: we let drivers
collect slices and send them through a single decode operation.

> 
> > > > We could perhaps have a test program to orchestrate such a scenario. I
> > > > think the assumption should obviously still be that we don't cross the
> > > > frame boundary using slices in one batch.  
> > > 
> > > We should definitely forbid mixing slices of different frames in the
> > > same decode operation, since each decode operation is targeting a
> > > single capture buffer.  
> 
> Agreed.
> 
> > > > Just that if a frame was made
> > > > up of, say, 4 slices and you first pass 3 slices, then 1, that it'd be
> > > > nice if the driver would be able to cope with that.  
> > > 
> > > Yep, that makes sense.
> > >   
> > > > It's something that
> > > > could probably even be implemented in the framework as a helper, though
> > > > I suspect it'd be just a couple of lines of extra code to wrap a loop
> > > > around everything.  
> > > 
> > > I also thought about providing generic wrappers, both for this case and
> > > the per-slice -> per-frame case (this one would be a bit more
> > > complicated as it implies queuing slices in a bounce buffer and
> > > triggering the decode operation only when we have all slices of a
> > > frame).  
> > 
> > I like deferring the addition of that kind of helper until a clear
> > pattern emerges out of the drivers that need this, just because that
> > gives us real examples on which to model those helpers. But yeah, I
> > think it should be possible to have helpers for these for most cases.  
> 
> So it seems that we need some convenient way to iterate over each slice
> and configure registers accordingly. Perhaps we could have some common
> per-codec helpers with callbacks to set the common controls and iterate
> over the per-slice controls.

Yep, I'm working on extracting common bits out of cedrus/hantro
drivers, that new helper (to iterate over all slices) can be part of
this series.

> 
> That would help keep our drivers tidy and understandable, and maybe we
> could have start/stop steps too, pretty much like we do in cedrus.
> 
> What do you think?

Makes total sense, and I'm all for having generic code provided as
vb2-m2m/codec helpers.

Re: [PATCH RFC 4/6] media: cedrus: Prepare things to support !compound controls

[Boris Brezillon]

On Wed, 05 Jun 2019 22:57:21 +0200
Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote:

> Hi,
> 
> Le lundi 03 juin 2019 à 13:09 +0200, Boris Brezillon a écrit :
> > We are about to add a menu control, so let's make the code more generic
> > to support other control types.  
> 
> Did you have a chance to test this or should I better give it a spin
> before adding my Reviewed-By?

I did not test this, so you'd better test it before giving your
R-b/A-b ;-).

> 
> Cheers,
> 
> Paul
> 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  drivers/staging/media/sunxi/cedrus/cedrus.c | 47 ++++++++++++---------
> >  drivers/staging/media/sunxi/cedrus/cedrus.h |  3 +-
> >  2 files changed, 29 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
> > index 370937edfc14..378032fe71f9 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> > @@ -29,44 +29,44 @@
> >  
> >  static const struct cedrus_control cedrus_controls[] = {
> >  	{
> > -		.id		= V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS,
> > -		.elem_size	= sizeof(struct v4l2_ctrl_mpeg2_slice_params),
> > +		.cfg.id		= V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS,
> > +		.cfg.elem_size	= sizeof(struct v4l2_ctrl_mpeg2_slice_params),
> >  		.codec		= CEDRUS_CODEC_MPEG2,
> >  		.required	= true,
> >  	},
> >  	{
> > -		.id		= V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION,
> > -		.elem_size	= sizeof(struct v4l2_ctrl_mpeg2_quantization),
> > +		.cfg.id		= V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION,
> > +		.cfg.elem_size	= sizeof(struct v4l2_ctrl_mpeg2_quantization),
> >  		.codec		= CEDRUS_CODEC_MPEG2,
> >  		.required	= false,
> >  	},
> >  	{
> > -		.id		= V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS,
> > -		.elem_size	= sizeof(struct v4l2_ctrl_h264_decode_params),
> > +		.cfg.id		= V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS,
> > +		.cfg.elem_size	= sizeof(struct v4l2_ctrl_h264_decode_params),
> >  		.codec		= CEDRUS_CODEC_H264,
> >  		.required	= true,
> >  	},
> >  	{
> > -		.id		= V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS,
> > -		.elem_size	= sizeof(struct v4l2_ctrl_h264_slice_params),
> > +		.cfg.id		= V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS,
> > +		.cfg.elem_size	= sizeof(struct v4l2_ctrl_h264_slice_params),
> >  		.codec		= CEDRUS_CODEC_H264,
> >  		.required	= true,
> >  	},
> >  	{
> > -		.id		= V4L2_CID_MPEG_VIDEO_H264_SPS,
> > -		.elem_size	= sizeof(struct v4l2_ctrl_h264_sps),
> > +		.cfg.id		= V4L2_CID_MPEG_VIDEO_H264_SPS,
> > +		.cfg.elem_size	= sizeof(struct v4l2_ctrl_h264_sps),
> >  		.codec		= CEDRUS_CODEC_H264,
> >  		.required	= true,
> >  	},
> >  	{
> > -		.id		= V4L2_CID_MPEG_VIDEO_H264_PPS,
> > -		.elem_size	= sizeof(struct v4l2_ctrl_h264_pps),
> > +		.cfg.id		= V4L2_CID_MPEG_VIDEO_H264_PPS,
> > +		.cfg.elem_size	= sizeof(struct v4l2_ctrl_h264_pps),
> >  		.codec		= CEDRUS_CODEC_H264,
> >  		.required	= true,
> >  	},
> >  	{
> > -		.id		= V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX,
> > -		.elem_size	= sizeof(struct v4l2_ctrl_h264_scaling_matrix),
> > +		.cfg.id		= V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX,
> > +		.cfg.elem_size	= sizeof(struct v4l2_ctrl_h264_scaling_matrix),
> >  		.codec		= CEDRUS_CODEC_H264,
> >  		.required	= true,
> >  	},
> > @@ -106,12 +106,21 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev, struct cedrus_ctx *ctx)
> >  		return -ENOMEM;
> >  
> >  	for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {
> > -		struct v4l2_ctrl_config cfg = {};
> > +		const struct v4l2_ctrl_config *cfg = &cedrus_controls[i].cfg;
> >  
> > -		cfg.elem_size = cedrus_controls[i].elem_size;
> > -		cfg.id = cedrus_controls[i].id;
> > +		if (cfg->elem_size)
> > +			ctrl = v4l2_ctrl_new_custom(hdl, cfg, NULL);
> > +		else if (cfg->type == V4L2_CTRL_TYPE_MENU ||
> > +			   cfg->type == V4L2_CTRL_TYPE_INTEGER_MENU)
> > +			ctrl = v4l2_ctrl_new_std_menu(hdl, NULL,
> > +						      cfg->id, cfg->max,
> > +						      cfg->menu_skip_mask,
> > +						      cfg->def);
> > +		else
> > +			ctrl = v4l2_ctrl_new_std(hdl, NULL, cfg->id, cfg->min,
> > +						 cfg->max, cfg->step,
> > +						 cfg->def);
> >  
> > -		ctrl = v4l2_ctrl_new_custom(hdl, &cfg, NULL);
> >  		if (hdl->error) {
> >  			v4l2_err(&dev->v4l2_dev,
> >  				 "Failed to create new custom control\n");
> > @@ -178,7 +187,7 @@ static int cedrus_request_validate(struct media_request *req)
> >  			continue;
> >  
> >  		ctrl_test = v4l2_ctrl_request_hdl_ctrl_find(hdl,
> > -							    cedrus_controls[i].id);
> > +							    cedrus_controls[i].cfg.id);
> >  		if (!ctrl_test) {
> >  			v4l2_info(&ctx->dev->v4l2_dev,
> >  				  "Missing required codec control\n");
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
> > index 3f476d0fd981..69c037724d93 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> > @@ -49,8 +49,7 @@ enum cedrus_h264_pic_type {
> >  };
> >  
> >  struct cedrus_control {
> > -	u32			id;
> > -	u32			elem_size;
> > +	struct v4l2_ctrl_config	cfg;
> >  	enum cedrus_codec	codec;
> >  	unsigned char		required:1;
> >  };  
> 

Re: [PATCH RFC 5/6] media: cedrus: Make the slice_params array size limitation more explicit

[Boris Brezillon]

On Wed, 05 Jun 2019 23:01:37 +0200
Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote:

> Hi,
> 
> Le mardi 04 juin 2019 à 10:31 -0400, Nicolas Dufresne a écrit :
> > Le mardi 04 juin 2019 à 10:12 +0200, Thierry Reding a écrit :  
> > > On Mon, Jun 03, 2019 at 07:55:48PM -0400, Nicolas Dufresne wrote:  
> > > > Le lundi 03 juin 2019 à 23:48 +0200, Jernej Škrabec a écrit :  
> > > > > Dne ponedeljek, 03. junij 2019 ob 13:09:45 CEST je Boris Brezillon napisal(a):  
> > > > > > The driver only supports per-slice decoding, and in that mode
> > > > > > decode_params->num_slices must be set to 1 and the slice_params array
> > > > > > should contain only one element.  
> > > > > 
> > > > > What Cedrus actually needs to know is if this is first slice in frame or not. I 
> > > > > imagine it resets some stuff internally when first slice is processed.
> > > > > 
> > > > > So if driver won't get all slices of one frame at once, it can't know if this 
> > > > > is first slice in frame or not. I guess we need additional flag for this.  
> > > > 
> > > > A first slice of a frame comes with a new timestamp, so you don't need
> > > > a flag for that.  
> > > 
> > > But slices for the same frame will all have the same timestamp, so we
> > > can't use the timestamp to tell the individual slices apart.
> > > 
> > > I mentioned this in that other thread, but I think it'd be useful to
> > > pass along the number of each of the slices. Drivers can use this in
> > > order to conceal errors when corrupt slices are detected during the
> > > decode operation.  
> > 
> > This is already passed as this is part of the slice header that we both
> > pass and parse to structure. Each slice have it's first MB indicated
> > (that standard to H264) and you can deduce the lost slice from that.
> >   
> > > So if we also passed a slice index along with the offset of the slice in
> > > the bitstream, that should give us enough information to achieve both. A
> > > slice with index 0 is obviously going to be the first slice in a frame.  
> > 
> > We do this in per-frame mode only. The offset of the slice in the
> > bitstream is always 0 in per-slice mode, since each v4l2 input buffer
> > is a slice.  
> 
> I don't think we need a slice index either, we most likely already have
> enough information to know where we're at regarding slices position.
> 
> But how about allowing an arbitrary number of slices within frame
> boundary in per-slice decoding mode?

Yep, will send a v2 taking that case into consideration.