[PATCH v6 0/2] Add encoder ctrls for long term reference

[PATCH v6 0/2] Add encoder ctrls for long term reference

[Dikshita Agarwal]

This series add the encoder controls for long term reference (LTR)
and support for the same in venus driver.

Dikshita Agarwal (2):
  media: v4l2-ctrl: add controls for long term reference.
  venus: venc: Add support for Long Term Reference (LTR) controls

 .../userspace-api/media/v4l/ext-ctrls-codec.rst    | 18 ++++++++
 drivers/media/platform/qcom/venus/venc_ctrls.c     | 49 +++++++++++++++++++++-
 drivers/media/v4l2-core/v4l2-ctrls.c               | 14 +++++++
 include/uapi/linux/v4l2-controls.h                 |  3 ++
 4 files changed, 83 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH v6 1/2] media: v4l2-ctrl: add controls for long term reference.

[Dikshita Agarwal]

Long Term Reference (LTR) frames are the frames that are encoded
sometime in the past and stored in the DPB buffer list to be used
as reference to encode future frames.
This change adds controls to enable this feature.

Signed-off-by: Dikshita Agarwal <dikshita@codeaurora.org>
---
 .../userspace-api/media/v4l/ext-ctrls-codec.rst        | 18 ++++++++++++++++++
 drivers/media/v4l2-core/v4l2-ctrls.c                   | 14 ++++++++++++++
 include/uapi/linux/v4l2-controls.h                     |  3 +++
 3 files changed, 35 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index 400774c..a37d460 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -3637,3 +3637,21 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
       - Selecting this value specifies that HEVC slices are expected
         to be prefixed by Annex B start codes. According to :ref:`hevc`
         valid start codes can be 3-bytes 0x000001 or 4-bytes 0x00000001.
+
+``V4L2_CID_MPEG_VIDEO_LTR_COUNT (integer)``
+       Specifies the number of Long Term Reference (LTR) frames encoder needs
+       to generate or keep. This is applicable to the H264 and HEVC encoders.
+
+``V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX (integer)``
+       The current frame is marked as a Long Term Reference (LTR) frame
+       and given this LTR index which ranges from 0 to LTR_COUNT-1.
+       This is applicable to the H264 and HEVC encoders and can be applied using
+       Request API.
+       Source Rec. ITU-T H.264 (06/2019); Table 7.9
+
+``V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES (bitmask)``
+       Specifies the Long Term Reference (LTR) frame(s) to be used for
+       encoding the current frame.
+       This provides a bitmask which consists of bits [0, LTR_COUNT-1].
+       This is applicable to the H264 and HEVC encoders and can be applied using
+       Request API.
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 16ab54f..84c1eb8 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -950,6 +950,9 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:		return "Vertical MV Search Range";
 	case V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER:		return "Repeat Sequence Header";
 	case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:		return "Force Key Frame";
+	case V4L2_CID_MPEG_VIDEO_LTR_COUNT:			return "LTR Count";
+	case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:		return "Frame LTR Index";
+	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:		return "Use LTR Frames";
 	case V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS:		return "MPEG-2 Slice Parameters";
 	case V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION:		return "MPEG-2 Quantization Matrices";
 	case V4L2_CID_FWHT_I_FRAME_QP:				return "FWHT I-Frame QP Value";
@@ -1277,6 +1280,17 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:
 		*type = V4L2_CTRL_TYPE_INTEGER;
 		break;
+	case V4L2_CID_MPEG_VIDEO_LTR_COUNT:
+		*type = V4L2_CTRL_TYPE_INTEGER;
+		break;
+	case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:
+		*type = V4L2_CTRL_TYPE_INTEGER;
+		*flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
+		break;
+	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:
+		*type = V4L2_CTRL_TYPE_BITMASK;
+		*flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
+		break;
 	case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
 	case V4L2_CID_PAN_RESET:
 	case V4L2_CID_TILT_RESET:
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index af8dda2..c0bb87b 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -422,6 +422,9 @@ enum v4l2_mpeg_video_multi_slice_mode {
 #define V4L2_CID_MPEG_VIDEO_MV_H_SEARCH_RANGE		(V4L2_CID_CODEC_BASE+227)
 #define V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE		(V4L2_CID_CODEC_BASE+228)
 #define V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME		(V4L2_CID_CODEC_BASE+229)
+#define V4L2_CID_MPEG_VIDEO_LTR_COUNT			(V4L2_CID_CODEC_BASE+230)
+#define V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX		(V4L2_CID_CODEC_BASE+231)
+#define V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES		(V4L2_CID_CODEC_BASE+232)
 
 /* CIDs for the MPEG-2 Part 2 (H.262) codec */
 #define V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL			(V4L2_CID_CODEC_BASE+270)
-- 
2.7.4

[PATCH v6 2/2] venus: venc: Add support for Long Term Reference (LTR) controls

[Dikshita Agarwal]

Add support for below LTR controls in encoder:
- V4L2_CID_MPEG_VIDEO_LTR_COUNT
- V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX
- V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES

Signed-off-by: Dikshita Agarwal <dikshita@codeaurora.org>
---
 drivers/media/platform/qcom/venus/venc_ctrls.c | 49 +++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
index 496ad4d..7d010d8 100644
--- a/drivers/media/platform/qcom/venus/venc_ctrls.c
+++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
@@ -20,6 +20,7 @@
 #define INTRA_REFRESH_MBS_MAX	300
 #define AT_SLICE_BOUNDARY	\
 	V4L2_MPEG_VIDEO_H264_LOOP_FILTER_MODE_DISABLED_AT_SLICE_BOUNDARY
+#define MAX_LTR_FRAME_COUNT 4
 
 static int venc_calc_bpframes(u32 gop_size, u32 conseq_b, u32 *bf, u32 *pf)
 {
@@ -72,6 +73,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
 	struct venc_controls *ctr = &inst->controls.enc;
 	struct hfi_enable en = { .enable = 1 };
 	struct hfi_bitrate brate;
+	struct hfi_ltr_use ltr_use;
+	struct hfi_ltr_mark ltr_mark;
+	struct hfi_ltr_mode ltr_mode;
 	u32 bframes;
 	u32 ptype;
 	int ret;
@@ -259,6 +263,37 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
 		ctr->frame_skip_mode = ctrl->val;
 		break;
+	case V4L2_CID_MPEG_VIDEO_LTR_COUNT:
+		ptype = HFI_PROPERTY_PARAM_VENC_LTRMODE;
+		ltr_mode.ltr_count = ctrl->val;
+		ltr_mode.ltr_mode = HFI_LTR_MODE_MANUAL;
+		ltr_mode.trust_mode = 1;
+		ret = hfi_session_set_property(inst, ptype, &ltr_mode);
+		if (ret) {
+			mutex_unlock(&inst->lock);
+			return ret;
+		}
+		break;
+	case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:
+		ptype = HFI_PROPERTY_CONFIG_VENC_MARKLTRFRAME;
+		ltr_mark.mark_frame = ctrl->val;
+		ret = hfi_session_set_property(inst, ptype, &ltr_mark);
+		if (ret) {
+			mutex_unlock(&inst->lock);
+			return ret;
+		}
+		break;
+	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:
+		ptype = HFI_PROPERTY_CONFIG_VENC_USELTRFRAME;
+		ltr_use.ref_ltr = ctrl->val;
+		ltr_use.use_constrnt = true;
+		ltr_use.frames = 0;
+		ret = hfi_session_set_property(inst, ptype, &ltr_use);
+		if (ret) {
+			mutex_unlock(&inst->lock);
+			return ret;
+		}
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -274,7 +309,7 @@ int venc_ctrl_init(struct venus_inst *inst)
 {
 	int ret;
 
-	ret = v4l2_ctrl_handler_init(&inst->ctrl_handler, 50);
+	ret = v4l2_ctrl_handler_init(&inst->ctrl_handler, 53);
 	if (ret)
 		return ret;
 
@@ -476,6 +511,18 @@ int venc_ctrl_init(struct venus_inst *inst)
 			       (1 << V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT)),
 			       V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED);
 
+	v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
+			  V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES, 0,
+			  (MAX_LTR_FRAME_COUNT - 1), 1, 0);
+
+	v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
+			  V4L2_CID_MPEG_VIDEO_LTR_COUNT, 0,
+			  MAX_LTR_FRAME_COUNT, 1, 0);
+
+	v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
+			  V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX, 0,
+			  (MAX_LTR_FRAME_COUNT - 1), 1, 0);
+
 	ret = inst->ctrl_handler.error;
 	if (ret)
 		goto err;
-- 
2.7.4

Re: [PATCH v6 1/2] media: v4l2-ctrl: add controls for long term reference.

[Hans Verkuil]

On 25/01/2021 06:51, Dikshita Agarwal wrote:
> Long Term Reference (LTR) frames are the frames that are encoded
> sometime in the past and stored in the DPB buffer list to be used
> as reference to encode future frames.
> This change adds controls to enable this feature.
> 
> Signed-off-by: Dikshita Agarwal <dikshita@codeaurora.org>
> ---
>  .../userspace-api/media/v4l/ext-ctrls-codec.rst        | 18 ++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c                   | 14 ++++++++++++++
>  include/uapi/linux/v4l2-controls.h                     |  3 +++
>  3 files changed, 35 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 400774c..a37d460 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -3637,3 +3637,21 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>        - Selecting this value specifies that HEVC slices are expected
>          to be prefixed by Annex B start codes. According to :ref:`hevc`
>          valid start codes can be 3-bytes 0x000001 or 4-bytes 0x00000001.
> +
> +``V4L2_CID_MPEG_VIDEO_LTR_COUNT (integer)``
> +       Specifies the number of Long Term Reference (LTR) frames encoder needs

frames encoder -> frames the encoder

> +       to generate or keep. This is applicable to the H264 and HEVC encoders.

Isn't this really the maximum number of LTR frames? I.e., the actual number
of LTR frames is something that is determined by userspace, right?

And you say 'generate or keep': but the encoder doesn't generate LTR frames,
they are marked as such by userspace. So this text would be just: "to keep".

Or am I wrong?

Regards,

	Hans

> +
> +``V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX (integer)``
> +       The current frame is marked as a Long Term Reference (LTR) frame
> +       and given this LTR index which ranges from 0 to LTR_COUNT-1.
> +       This is applicable to the H264 and HEVC encoders and can be applied using
> +       Request API.
> +       Source Rec. ITU-T H.264 (06/2019); Table 7.9
> +
> +``V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES (bitmask)``
> +       Specifies the Long Term Reference (LTR) frame(s) to be used for
> +       encoding the current frame.
> +       This provides a bitmask which consists of bits [0, LTR_COUNT-1].
> +       This is applicable to the H264 and HEVC encoders and can be applied using
> +       Request API.
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 16ab54f..84c1eb8 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -950,6 +950,9 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:		return "Vertical MV Search Range";
>  	case V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER:		return "Repeat Sequence Header";
>  	case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:		return "Force Key Frame";
> +	case V4L2_CID_MPEG_VIDEO_LTR_COUNT:			return "LTR Count";
> +	case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:		return "Frame LTR Index";
> +	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:		return "Use LTR Frames";
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS:		return "MPEG-2 Slice Parameters";
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION:		return "MPEG-2 Quantization Matrices";
>  	case V4L2_CID_FWHT_I_FRAME_QP:				return "FWHT I-Frame QP Value";
> @@ -1277,6 +1280,17 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:
>  		*type = V4L2_CTRL_TYPE_INTEGER;
>  		break;
> +	case V4L2_CID_MPEG_VIDEO_LTR_COUNT:
> +		*type = V4L2_CTRL_TYPE_INTEGER;
> +		break;
> +	case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:
> +		*type = V4L2_CTRL_TYPE_INTEGER;
> +		*flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> +		break;
> +	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:
> +		*type = V4L2_CTRL_TYPE_BITMASK;
> +		*flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> +		break;
>  	case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
>  	case V4L2_CID_PAN_RESET:
>  	case V4L2_CID_TILT_RESET:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index af8dda2..c0bb87b 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -422,6 +422,9 @@ enum v4l2_mpeg_video_multi_slice_mode {
>  #define V4L2_CID_MPEG_VIDEO_MV_H_SEARCH_RANGE		(V4L2_CID_CODEC_BASE+227)
>  #define V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE		(V4L2_CID_CODEC_BASE+228)
>  #define V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME		(V4L2_CID_CODEC_BASE+229)
> +#define V4L2_CID_MPEG_VIDEO_LTR_COUNT			(V4L2_CID_CODEC_BASE+230)
> +#define V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX		(V4L2_CID_CODEC_BASE+231)
> +#define V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES		(V4L2_CID_CODEC_BASE+232)
>  
>  /* CIDs for the MPEG-2 Part 2 (H.262) codec */
>  #define V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL			(V4L2_CID_CODEC_BASE+270)
> 

Re: [PATCH v6 1/2] media: v4l2-ctrl: add controls for long term reference.

[Hans Verkuil]

On 25/01/2021 06:51, Dikshita Agarwal wrote:
> Long Term Reference (LTR) frames are the frames that are encoded
> sometime in the past and stored in the DPB buffer list to be used
> as reference to encode future frames.
> This change adds controls to enable this feature.
> 
> Signed-off-by: Dikshita Agarwal <dikshita@codeaurora.org>
> ---
>  .../userspace-api/media/v4l/ext-ctrls-codec.rst        | 18 ++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c                   | 14 ++++++++++++++
>  include/uapi/linux/v4l2-controls.h                     |  3 +++
>  3 files changed, 35 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 400774c..a37d460 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -3637,3 +3637,21 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>        - Selecting this value specifies that HEVC slices are expected
>          to be prefixed by Annex B start codes. According to :ref:`hevc`
>          valid start codes can be 3-bytes 0x000001 or 4-bytes 0x00000001.
> +
> +``V4L2_CID_MPEG_VIDEO_LTR_COUNT (integer)``
> +       Specifies the number of Long Term Reference (LTR) frames encoder needs
> +       to generate or keep. This is applicable to the H264 and HEVC encoders.
> +
> +``V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX (integer)``
> +       The current frame is marked as a Long Term Reference (LTR) frame
> +       and given this LTR index which ranges from 0 to LTR_COUNT-1.
> +       This is applicable to the H264 and HEVC encoders and can be applied using
> +       Request API.

You mentioned in reply to my comment that the venus driver didn't support the
Request API that it is also possible to use it without that API.

But that requires more precise documentation. I assume that without the Request
API you would set this control, then queue the buffer containing the frame this
control should apply to, then wait until it is dequeued. Since that's the only
way you can be certain this control is applied to the correct frame.

Is this indeed what you do in your application?

Regards,

	Hans

> +       Source Rec. ITU-T H.264 (06/2019); Table 7.9
> +
> +``V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES (bitmask)``
> +       Specifies the Long Term Reference (LTR) frame(s) to be used for
> +       encoding the current frame.
> +       This provides a bitmask which consists of bits [0, LTR_COUNT-1].
> +       This is applicable to the H264 and HEVC encoders and can be applied using
> +       Request API.
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 16ab54f..84c1eb8 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -950,6 +950,9 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:		return "Vertical MV Search Range";
>  	case V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER:		return "Repeat Sequence Header";
>  	case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:		return "Force Key Frame";
> +	case V4L2_CID_MPEG_VIDEO_LTR_COUNT:			return "LTR Count";
> +	case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:		return "Frame LTR Index";
> +	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:		return "Use LTR Frames";
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS:		return "MPEG-2 Slice Parameters";
>  	case V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION:		return "MPEG-2 Quantization Matrices";
>  	case V4L2_CID_FWHT_I_FRAME_QP:				return "FWHT I-Frame QP Value";
> @@ -1277,6 +1280,17 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:
>  		*type = V4L2_CTRL_TYPE_INTEGER;
>  		break;
> +	case V4L2_CID_MPEG_VIDEO_LTR_COUNT:
> +		*type = V4L2_CTRL_TYPE_INTEGER;
> +		break;
> +	case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:
> +		*type = V4L2_CTRL_TYPE_INTEGER;
> +		*flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> +		break;
> +	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:
> +		*type = V4L2_CTRL_TYPE_BITMASK;
> +		*flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> +		break;
>  	case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
>  	case V4L2_CID_PAN_RESET:
>  	case V4L2_CID_TILT_RESET:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index af8dda2..c0bb87b 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -422,6 +422,9 @@ enum v4l2_mpeg_video_multi_slice_mode {
>  #define V4L2_CID_MPEG_VIDEO_MV_H_SEARCH_RANGE		(V4L2_CID_CODEC_BASE+227)
>  #define V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE		(V4L2_CID_CODEC_BASE+228)
>  #define V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME		(V4L2_CID_CODEC_BASE+229)
> +#define V4L2_CID_MPEG_VIDEO_LTR_COUNT			(V4L2_CID_CODEC_BASE+230)
> +#define V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX		(V4L2_CID_CODEC_BASE+231)
> +#define V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES		(V4L2_CID_CODEC_BASE+232)
>  
>  /* CIDs for the MPEG-2 Part 2 (H.262) codec */
>  #define V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL			(V4L2_CID_CODEC_BASE+270)
> 

Re: [PATCH v6 1/2] media: v4l2-ctrl: add controls for long term reference.

[dikshita]

On 2021-02-01 16:40, Hans Verkuil wrote:
> On 25/01/2021 06:51, Dikshita Agarwal wrote:
>> Long Term Reference (LTR) frames are the frames that are encoded
>> sometime in the past and stored in the DPB buffer list to be used
>> as reference to encode future frames.
>> This change adds controls to enable this feature.
>> 
>> Signed-off-by: Dikshita Agarwal <dikshita@codeaurora.org>
>> ---
>>  .../userspace-api/media/v4l/ext-ctrls-codec.rst        | 18 
>> ++++++++++++++++++
>>  drivers/media/v4l2-core/v4l2-ctrls.c                   | 14 
>> ++++++++++++++
>>  include/uapi/linux/v4l2-controls.h                     |  3 +++
>>  3 files changed, 35 insertions(+)
>> 
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst 
>> b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> index 400774c..a37d460 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> @@ -3637,3 +3637,21 @@ enum v4l2_mpeg_video_hevc_size_of_length_field 
>> -
>>        - Selecting this value specifies that HEVC slices are expected
>>          to be prefixed by Annex B start codes. According to 
>> :ref:`hevc`
>>          valid start codes can be 3-bytes 0x000001 or 4-bytes 
>> 0x00000001.
>> +
>> +``V4L2_CID_MPEG_VIDEO_LTR_COUNT (integer)``
>> +       Specifies the number of Long Term Reference (LTR) frames 
>> encoder needs
> 
> frames encoder -> frames the encoder
> 
>> +       to generate or keep. This is applicable to the H264 and HEVC 
>> encoders.
> 
> Isn't this really the maximum number of LTR frames? I.e., the actual 
> number
> of LTR frames is something that is determined by userspace, right?
> 
> And you say 'generate or keep': but the encoder doesn't generate LTR 
> frames,
> they are marked as such by userspace. So this text would be just: "to 
> keep".
> 
> Or am I wrong?
> 
> Regards,
> 
> 	Hans
> 
Hi Hans,

I Will update this as:
Specifies the maximum number of Long Term Reference (LTR) frames at any 
given time
that the encoder can keep.

Thanks,
Dikshita

>> +
>> +``V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX (integer)``
>> +       The current frame is marked as a Long Term Reference (LTR) 
>> frame
>> +       and given this LTR index which ranges from 0 to LTR_COUNT-1.
>> +       This is applicable to the H264 and HEVC encoders and can be 
>> applied using
>> +       Request API.
>> +       Source Rec. ITU-T H.264 (06/2019); Table 7.9
>> +
>> +``V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES (bitmask)``
>> +       Specifies the Long Term Reference (LTR) frame(s) to be used 
>> for
>> +       encoding the current frame.
>> +       This provides a bitmask which consists of bits [0, 
>> LTR_COUNT-1].
>> +       This is applicable to the H264 and HEVC encoders and can be 
>> applied using
>> +       Request API.
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
>> b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index 16ab54f..84c1eb8 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -950,6 +950,9 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:		return "Vertical MV 
>> Search Range";
>>  	case V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER:		return "Repeat Sequence 
>> Header";
>>  	case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:		return "Force Key Frame";
>> +	case V4L2_CID_MPEG_VIDEO_LTR_COUNT:			return "LTR Count";
>> +	case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:		return "Frame LTR Index";
>> +	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:		return "Use LTR Frames";
>>  	case V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS:		return "MPEG-2 Slice 
>> Parameters";
>>  	case V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION:		return "MPEG-2 
>> Quantization Matrices";
>>  	case V4L2_CID_FWHT_I_FRAME_QP:				return "FWHT I-Frame QP Value";
>> @@ -1277,6 +1280,17 @@ void v4l2_ctrl_fill(u32 id, const char **name, 
>> enum v4l2_ctrl_type *type,
>>  	case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:
>>  		*type = V4L2_CTRL_TYPE_INTEGER;
>>  		break;
>> +	case V4L2_CID_MPEG_VIDEO_LTR_COUNT:
>> +		*type = V4L2_CTRL_TYPE_INTEGER;
>> +		break;
>> +	case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:
>> +		*type = V4L2_CTRL_TYPE_INTEGER;
>> +		*flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
>> +		break;
>> +	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:
>> +		*type = V4L2_CTRL_TYPE_BITMASK;
>> +		*flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
>> +		break;
>>  	case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
>>  	case V4L2_CID_PAN_RESET:
>>  	case V4L2_CID_TILT_RESET:
>> diff --git a/include/uapi/linux/v4l2-controls.h 
>> b/include/uapi/linux/v4l2-controls.h
>> index af8dda2..c0bb87b 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -422,6 +422,9 @@ enum v4l2_mpeg_video_multi_slice_mode {
>>  #define 
>> V4L2_CID_MPEG_VIDEO_MV_H_SEARCH_RANGE		(V4L2_CID_CODEC_BASE+227)
>>  #define 
>> V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE		(V4L2_CID_CODEC_BASE+228)
>>  #define 
>> V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME		(V4L2_CID_CODEC_BASE+229)
>> +#define V4L2_CID_MPEG_VIDEO_LTR_COUNT			(V4L2_CID_CODEC_BASE+230)
>> +#define 
>> V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX		(V4L2_CID_CODEC_BASE+231)
>> +#define V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES		(V4L2_CID_CODEC_BASE+232)
>> 
>>  /* CIDs for the MPEG-2 Part 2 (H.262) codec */
>>  #define V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL			(V4L2_CID_CODEC_BASE+270)
>> 

Re: [PATCH v6 1/2] media: v4l2-ctrl: add controls for long term reference.

[dikshita]

On 2021-02-01 16:50, Hans Verkuil wrote:
> On 25/01/2021 06:51, Dikshita Agarwal wrote:
>> Long Term Reference (LTR) frames are the frames that are encoded
>> sometime in the past and stored in the DPB buffer list to be used
>> as reference to encode future frames.
>> This change adds controls to enable this feature.
>> 
>> Signed-off-by: Dikshita Agarwal <dikshita@codeaurora.org>
>> ---
>>  .../userspace-api/media/v4l/ext-ctrls-codec.rst        | 18 
>> ++++++++++++++++++
>>  drivers/media/v4l2-core/v4l2-ctrls.c                   | 14 
>> ++++++++++++++
>>  include/uapi/linux/v4l2-controls.h                     |  3 +++
>>  3 files changed, 35 insertions(+)
>> 
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst 
>> b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> index 400774c..a37d460 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> @@ -3637,3 +3637,21 @@ enum v4l2_mpeg_video_hevc_size_of_length_field 
>> -
>>        - Selecting this value specifies that HEVC slices are expected
>>          to be prefixed by Annex B start codes. According to 
>> :ref:`hevc`
>>          valid start codes can be 3-bytes 0x000001 or 4-bytes 
>> 0x00000001.
>> +
>> +``V4L2_CID_MPEG_VIDEO_LTR_COUNT (integer)``
>> +       Specifies the number of Long Term Reference (LTR) frames 
>> encoder needs
>> +       to generate or keep. This is applicable to the H264 and HEVC 
>> encoders.
>> +
>> +``V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX (integer)``
>> +       The current frame is marked as a Long Term Reference (LTR) 
>> frame
>> +       and given this LTR index which ranges from 0 to LTR_COUNT-1.
>> +       This is applicable to the H264 and HEVC encoders and can be 
>> applied using
>> +       Request API.
> 
> You mentioned in reply to my comment that the venus driver didn't 
> support the
> Request API that it is also possible to use it without that API.
> 
> But that requires more precise documentation. I assume that without the 
> Request
> API you would set this control, then queue the buffer containing the 
> frame this
> control should apply to, then wait until it is dequeued. Since that's 
> the only
> way you can be certain this control is applied to the correct frame.
> 
> Is this indeed what you do in your application?
> 
> Regards,
> 
> 	Hans
> 
Hi Hans,

Yes, It is possible without request API as well in a non-synchronized 
way.
And we don't need to wait for the frame to be dequeued.
The driver implementation ensures that whenever the LTR control is 
received,
it applies to the frame received after that. Not to frame which would be 
encoded next.
So that it is at least synchronized between driver & encoder.

Thanks,
Dikshita

>> +       Source Rec. ITU-T H.264 (06/2019); Table 7.9
>> +
>> +``V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES (bitmask)``
>> +       Specifies the Long Term Reference (LTR) frame(s) to be used 
>> for
>> +       encoding the current frame.
>> +       This provides a bitmask which consists of bits [0, 
>> LTR_COUNT-1].
>> +       This is applicable to the H264 and HEVC encoders and can be 
>> applied using
>> +       Request API.
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
>> b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index 16ab54f..84c1eb8 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -950,6 +950,9 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:		return "Vertical MV 
>> Search Range";
>>  	case V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER:		return "Repeat Sequence 
>> Header";
>>  	case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:		return "Force Key Frame";
>> +	case V4L2_CID_MPEG_VIDEO_LTR_COUNT:			return "LTR Count";
>> +	case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:		return "Frame LTR Index";
>> +	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:		return "Use LTR Frames";
>>  	case V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS:		return "MPEG-2 Slice 
>> Parameters";
>>  	case V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION:		return "MPEG-2 
>> Quantization Matrices";
>>  	case V4L2_CID_FWHT_I_FRAME_QP:				return "FWHT I-Frame QP Value";
>> @@ -1277,6 +1280,17 @@ void v4l2_ctrl_fill(u32 id, const char **name, 
>> enum v4l2_ctrl_type *type,
>>  	case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:
>>  		*type = V4L2_CTRL_TYPE_INTEGER;
>>  		break;
>> +	case V4L2_CID_MPEG_VIDEO_LTR_COUNT:
>> +		*type = V4L2_CTRL_TYPE_INTEGER;
>> +		break;
>> +	case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:
>> +		*type = V4L2_CTRL_TYPE_INTEGER;
>> +		*flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
>> +		break;
>> +	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:
>> +		*type = V4L2_CTRL_TYPE_BITMASK;
>> +		*flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
>> +		break;
>>  	case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
>>  	case V4L2_CID_PAN_RESET:
>>  	case V4L2_CID_TILT_RESET:
>> diff --git a/include/uapi/linux/v4l2-controls.h 
>> b/include/uapi/linux/v4l2-controls.h
>> index af8dda2..c0bb87b 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -422,6 +422,9 @@ enum v4l2_mpeg_video_multi_slice_mode {
>>  #define 
>> V4L2_CID_MPEG_VIDEO_MV_H_SEARCH_RANGE		(V4L2_CID_CODEC_BASE+227)
>>  #define 
>> V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE		(V4L2_CID_CODEC_BASE+228)
>>  #define 
>> V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME		(V4L2_CID_CODEC_BASE+229)
>> +#define V4L2_CID_MPEG_VIDEO_LTR_COUNT			(V4L2_CID_CODEC_BASE+230)
>> +#define 
>> V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX		(V4L2_CID_CODEC_BASE+231)
>> +#define V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES		(V4L2_CID_CODEC_BASE+232)
>> 
>>  /* CIDs for the MPEG-2 Part 2 (H.262) codec */
>>  #define V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL			(V4L2_CID_CODEC_BASE+270)
>> 

Re: [PATCH v6 1/2] media: v4l2-ctrl: add controls for long term reference.

[Hans Verkuil]

On 04/02/2021 06:01, dikshita@codeaurora.org wrote:
> On 2021-02-01 16:50, Hans Verkuil wrote:
>> On 25/01/2021 06:51, Dikshita Agarwal wrote:
>>> Long Term Reference (LTR) frames are the frames that are encoded
>>> sometime in the past and stored in the DPB buffer list to be used
>>> as reference to encode future frames.
>>> This change adds controls to enable this feature.
>>>
>>> Signed-off-by: Dikshita Agarwal <dikshita@codeaurora.org>
>>> ---
>>>  .../userspace-api/media/v4l/ext-ctrls-codec.rst        | 18 
>>> ++++++++++++++++++
>>>  drivers/media/v4l2-core/v4l2-ctrls.c                   | 14 
>>> ++++++++++++++
>>>  include/uapi/linux/v4l2-controls.h                     |  3 +++
>>>  3 files changed, 35 insertions(+)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst 
>>> b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> index 400774c..a37d460 100644
>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> @@ -3637,3 +3637,21 @@ enum v4l2_mpeg_video_hevc_size_of_length_field 
>>> -
>>>        - Selecting this value specifies that HEVC slices are expected
>>>          to be prefixed by Annex B start codes. According to 
>>> :ref:`hevc`
>>>          valid start codes can be 3-bytes 0x000001 or 4-bytes 
>>> 0x00000001.
>>> +
>>> +``V4L2_CID_MPEG_VIDEO_LTR_COUNT (integer)``
>>> +       Specifies the number of Long Term Reference (LTR) frames 
>>> encoder needs
>>> +       to generate or keep. This is applicable to the H264 and HEVC 
>>> encoders.
>>> +
>>> +``V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX (integer)``
>>> +       The current frame is marked as a Long Term Reference (LTR) 
>>> frame
>>> +       and given this LTR index which ranges from 0 to LTR_COUNT-1.
>>> +       This is applicable to the H264 and HEVC encoders and can be 
>>> applied using
>>> +       Request API.
>>
>> You mentioned in reply to my comment that the venus driver didn't 
>> support the
>> Request API that it is also possible to use it without that API.
>>
>> But that requires more precise documentation. I assume that without the 
>> Request
>> API you would set this control, then queue the buffer containing the 
>> frame this
>> control should apply to, then wait until it is dequeued. Since that's 
>> the only
>> way you can be certain this control is applied to the correct frame.
>>
>> Is this indeed what you do in your application?
>>
>> Regards,
>>
>> 	Hans
>>
> Hi Hans,
> 
> Yes, It is possible without request API as well in a non-synchronized 
> way.
> And we don't need to wait for the frame to be dequeued.
> The driver implementation ensures that whenever the LTR control is 
> received,
> it applies to the frame received after that. Not to frame which would be 
> encoded next.
> So that it is at least synchronized between driver & encoder.

This is highly driver dependent. I'm not even sure this is true for the venus
driver: if you prequeue, say, 4 output buffers to the encoder and call
V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX after the second buffer (so it should
apply to the third), and only after the fourth you call VIDIOC_STREAMON,
does the venus driver still keep track of the order of the queued buffers
and when these controls are set? Once STREAMON is called it looks like it
stays synced since everything is queued into a command queue, if I understand
the code correctly.

The problem is that when controls are applied in relation to queued buffers
is not defined, unless you use the Request API. Typically controls are applied
immediately, so the venus driver is a bit of an anomaly in that respect.

You can make an explicit requirement that these controls apply to the next
queued buffer if no request API is used, but you really must be 100% certain
that the venus driver does that right (and as mentioned, I have my doubts about
the case where you queue buffers before calling STREAMON).

Regards,

	Hans

> 
> Thanks,
> Dikshita
> 
>>> +       Source Rec. ITU-T H.264 (06/2019); Table 7.9
>>> +
>>> +``V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES (bitmask)``
>>> +       Specifies the Long Term Reference (LTR) frame(s) to be used 
>>> for
>>> +       encoding the current frame.
>>> +       This provides a bitmask which consists of bits [0, 
>>> LTR_COUNT-1].
>>> +       This is applicable to the H264 and HEVC encoders and can be 
>>> applied using
>>> +       Request API.
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
>>> b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> index 16ab54f..84c1eb8 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> @@ -950,6 +950,9 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>  	case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:		return "Vertical MV 
>>> Search Range";
>>>  	case V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER:		return "Repeat Sequence 
>>> Header";
>>>  	case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:		return "Force Key Frame";
>>> +	case V4L2_CID_MPEG_VIDEO_LTR_COUNT:			return "LTR Count";
>>> +	case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:		return "Frame LTR Index";
>>> +	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:		return "Use LTR Frames";
>>>  	case V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS:		return "MPEG-2 Slice 
>>> Parameters";
>>>  	case V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION:		return "MPEG-2 
>>> Quantization Matrices";
>>>  	case V4L2_CID_FWHT_I_FRAME_QP:				return "FWHT I-Frame QP Value";
>>> @@ -1277,6 +1280,17 @@ void v4l2_ctrl_fill(u32 id, const char **name, 
>>> enum v4l2_ctrl_type *type,
>>>  	case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:
>>>  		*type = V4L2_CTRL_TYPE_INTEGER;
>>>  		break;
>>> +	case V4L2_CID_MPEG_VIDEO_LTR_COUNT:
>>> +		*type = V4L2_CTRL_TYPE_INTEGER;
>>> +		break;
>>> +	case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:
>>> +		*type = V4L2_CTRL_TYPE_INTEGER;
>>> +		*flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
>>> +		break;
>>> +	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:
>>> +		*type = V4L2_CTRL_TYPE_BITMASK;
>>> +		*flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
>>> +		break;
>>>  	case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
>>>  	case V4L2_CID_PAN_RESET:
>>>  	case V4L2_CID_TILT_RESET:
>>> diff --git a/include/uapi/linux/v4l2-controls.h 
>>> b/include/uapi/linux/v4l2-controls.h
>>> index af8dda2..c0bb87b 100644
>>> --- a/include/uapi/linux/v4l2-controls.h
>>> +++ b/include/uapi/linux/v4l2-controls.h
>>> @@ -422,6 +422,9 @@ enum v4l2_mpeg_video_multi_slice_mode {
>>>  #define 
>>> V4L2_CID_MPEG_VIDEO_MV_H_SEARCH_RANGE		(V4L2_CID_CODEC_BASE+227)
>>>  #define 
>>> V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE		(V4L2_CID_CODEC_BASE+228)
>>>  #define 
>>> V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME		(V4L2_CID_CODEC_BASE+229)
>>> +#define V4L2_CID_MPEG_VIDEO_LTR_COUNT			(V4L2_CID_CODEC_BASE+230)
>>> +#define 
>>> V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX		(V4L2_CID_CODEC_BASE+231)
>>> +#define V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES		(V4L2_CID_CODEC_BASE+232)
>>>
>>>  /* CIDs for the MPEG-2 Part 2 (H.262) codec */
>>>  #define V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL			(V4L2_CID_CODEC_BASE+270)
>>>

Re: [PATCH v6 1/2] media: v4l2-ctrl: add controls for long term reference.

[Nicolas Dufresne]

Le jeudi 04 février 2021 à 11:05 +0100, Hans Verkuil a écrit :
> On 04/02/2021 06:01, dikshita@codeaurora.org wrote:
> > On 2021-02-01 16:50, Hans Verkuil wrote:
> > > On 25/01/2021 06:51, Dikshita Agarwal wrote:
> > > > Long Term Reference (LTR) frames are the frames that are encoded
> > > > sometime in the past and stored in the DPB buffer list to be used
> > > > as reference to encode future frames.
> > > > This change adds controls to enable this feature.
> > > > 
> > > > Signed-off-by: Dikshita Agarwal <dikshita@codeaurora.org>
> > > > ---
> > > >  .../userspace-api/media/v4l/ext-ctrls-codec.rst        | 18 
> > > > ++++++++++++++++++
> > > >  drivers/media/v4l2-core/v4l2-ctrls.c                   | 14 
> > > > ++++++++++++++
> > > >  include/uapi/linux/v4l2-controls.h                     |  3 +++
> > > >  3 files changed, 35 insertions(+)
> > > > 
> > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst 
> > > > b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > > index 400774c..a37d460 100644
> > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > > @@ -3637,3 +3637,21 @@ enum v4l2_mpeg_video_hevc_size_of_length_field 
> > > > -
> > > >        - Selecting this value specifies that HEVC slices are expected
> > > >          to be prefixed by Annex B start codes. According to 
> > > > :ref:`hevc`
> > > >          valid start codes can be 3-bytes 0x000001 or 4-bytes 
> > > > 0x00000001.
> > > > +
> > > > +``V4L2_CID_MPEG_VIDEO_LTR_COUNT (integer)``
> > > > +       Specifies the number of Long Term Reference (LTR) frames 
> > > > encoder needs
> > > > +       to generate or keep. This is applicable to the H264 and HEVC 
> > > > encoders.
> > > > +
> > > > +``V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX (integer)``
> > > > +       The current frame is marked as a Long Term Reference (LTR) 
> > > > frame
> > > > +       and given this LTR index which ranges from 0 to LTR_COUNT-1.
> > > > +       This is applicable to the H264 and HEVC encoders and can be 
> > > > applied using
> > > > +       Request API.
> > > 
> > > You mentioned in reply to my comment that the venus driver didn't 
> > > support the
> > > Request API that it is also possible to use it without that API.
> > > 
> > > But that requires more precise documentation. I assume that without the 
> > > Request
> > > API you would set this control, then queue the buffer containing the 
> > > frame this
> > > control should apply to, then wait until it is dequeued. Since that's 
> > > the only
> > > way you can be certain this control is applied to the correct frame.
> > > 
> > > Is this indeed what you do in your application?
> > > 
> > > Regards,
> > > 
> > >         Hans
> > > 
> > Hi Hans,
> > 
> > Yes, It is possible without request API as well in a non-synchronized 
> > way.
> > And we don't need to wait for the frame to be dequeued.
> > The driver implementation ensures that whenever the LTR control is 
> > received,
> > it applies to the frame received after that. Not to frame which would be 
> > encoded next.
> > So that it is at least synchronized between driver & encoder.
> 
> This is highly driver dependent. I'm not even sure this is true for the venus
> driver: if you prequeue, say, 4 output buffers to the encoder and call
> V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX after the second buffer (so it should
> apply to the third), and only after the fourth you call VIDIOC_STREAMON,
> does the venus driver still keep track of the order of the queued buffers
> and when these controls are set? Once STREAMON is called it looks like it
> stays synced since everything is queued into a command queue, if I understand
> the code correctly.
> 
> The problem is that when controls are applied in relation to queued buffers
> is not defined, unless you use the Request API. Typically controls are applied
> immediately, so the venus driver is a bit of an anomaly in that respect.
> 
> You can make an explicit requirement that these controls apply to the next
> queued buffer if no request API is used, but you really must be 100% certain
> that the venus driver does that right (and as mentioned, I have my doubts
> about
> the case where you queue buffers before calling STREAMON).

Do you propose to start usign request for stateful encoder ? If this is the
case, I'd like to remind that it's not always possible to notify encode
completion in request queue order for this type of HW. Reordering might be
implicit in the firmware design, so the driver may not have any notification
until multiple frames have been encoded.

To resume, we can use request for this type of application, no issues, but
userspace may not switch to waiting on the request for completion as this may
have HW specific behaviour. It will have to resort to polling for READ, and
dequeue from capture queue and figure-out after the fact which request are now
complete.

Nicolas

> 
> Regards,
> 
>         Hans
> 
> > 
> > Thanks,
> > Dikshita
> > 
> > > > +       Source Rec. ITU-T H.264 (06/2019); Table 7.9
> > > > +
> > > > +``V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES (bitmask)``
> > > > +       Specifies the Long Term Reference (LTR) frame(s) to be used 
> > > > for
> > > > +       encoding the current frame.
> > > > +       This provides a bitmask which consists of bits [0, 
> > > > LTR_COUNT-1].
> > > > +       This is applicable to the H264 and HEVC encoders and can be 
> > > > applied using
> > > > +       Request API.
> > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
> > > > b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > > index 16ab54f..84c1eb8 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > > @@ -950,6 +950,9 @@ const char *v4l2_ctrl_get_name(u32 id)
> > > >         case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:             return
> > > > "Vertical MV 
> > > > Search Range";
> > > >         case V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER:             return
> > > > "Repeat Sequence 
> > > > Header";
> > > >         case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:               return
> > > > "Force Key Frame";
> > > > +       case V4L2_CID_MPEG_VIDEO_LTR_COUNT:                     return
> > > > "LTR Count";
> > > > +       case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:               return
> > > > "Frame LTR Index";
> > > > +       case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:                return
> > > > "Use LTR Frames";
> > > >         case V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS:            return
> > > > "MPEG-2 Slice 
> > > > Parameters";
> > > >         case V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION:            return
> > > > "MPEG-2 
> > > > Quantization Matrices";
> > > >         case V4L2_CID_FWHT_I_FRAME_QP:                          return
> > > > "FWHT I-Frame QP Value";
> > > > @@ -1277,6 +1280,17 @@ void v4l2_ctrl_fill(u32 id, const char **name, 
> > > > enum v4l2_ctrl_type *type,
> > > >         case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:
> > > >                 *type = V4L2_CTRL_TYPE_INTEGER;
> > > >                 break;
> > > > +       case V4L2_CID_MPEG_VIDEO_LTR_COUNT:
> > > > +               *type = V4L2_CTRL_TYPE_INTEGER;
> > > > +               break;
> > > > +       case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:
> > > > +               *type = V4L2_CTRL_TYPE_INTEGER;
> > > > +               *flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> > > > +               break;
> > > > +       case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:
> > > > +               *type = V4L2_CTRL_TYPE_BITMASK;
> > > > +               *flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> > > > +               break;
> > > >         case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
> > > >         case V4L2_CID_PAN_RESET:
> > > >         case V4L2_CID_TILT_RESET:
> > > > diff --git a/include/uapi/linux/v4l2-controls.h 
> > > > b/include/uapi/linux/v4l2-controls.h
> > > > index af8dda2..c0bb87b 100644
> > > > --- a/include/uapi/linux/v4l2-controls.h
> > > > +++ b/include/uapi/linux/v4l2-controls.h
> > > > @@ -422,6 +422,9 @@ enum v4l2_mpeg_video_multi_slice_mode {
> > > >  #define 
> > > > V4L2_CID_MPEG_VIDEO_MV_H_SEARCH_RANGE           (V4L2_CID_CODEC_BASE+227
> > > > )
> > > >  #define 
> > > > V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE           (V4L2_CID_CODEC_BASE+228
> > > > )
> > > >  #define 
> > > > V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME             (V4L2_CID_CODEC_BASE+229
> > > > )
> > > > +#define
> > > > V4L2_CID_MPEG_VIDEO_LTR_COUNT                  (V4L2_CID_CODEC_BASE+230)
> > > > +#define 
> > > > V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX             (V4L2_CID_CODEC_BASE+231
> > > > )
> > > > +#define
> > > > V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES             (V4L2_CID_CODEC_BASE+232)
> > > > 
> > > >  /* CIDs for the MPEG-2 Part 2 (H.262) codec */
> > > >  #define
> > > > V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL                        (V4L2_CID_CODEC_B
> > > > ASE+270)
> > > > 
> 

Re: [PATCH v6 1/2] media: v4l2-ctrl: add controls for long term reference.

[Hans Verkuil]

On 10/02/2021 17:03, Nicolas Dufresne wrote:
> Le jeudi 04 février 2021 à 11:05 +0100, Hans Verkuil a écrit :
>> On 04/02/2021 06:01, dikshita@codeaurora.org wrote:
>>> On 2021-02-01 16:50, Hans Verkuil wrote:
>>>> On 25/01/2021 06:51, Dikshita Agarwal wrote:
>>>>> Long Term Reference (LTR) frames are the frames that are encoded
>>>>> sometime in the past and stored in the DPB buffer list to be used
>>>>> as reference to encode future frames.
>>>>> This change adds controls to enable this feature.
>>>>>
>>>>> Signed-off-by: Dikshita Agarwal <dikshita@codeaurora.org>
>>>>> ---
>>>>>  .../userspace-api/media/v4l/ext-ctrls-codec.rst        | 18 
>>>>> ++++++++++++++++++
>>>>>  drivers/media/v4l2-core/v4l2-ctrls.c                   | 14 
>>>>> ++++++++++++++
>>>>>  include/uapi/linux/v4l2-controls.h                     |  3 +++
>>>>>  3 files changed, 35 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst 
>>>>> b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>>> index 400774c..a37d460 100644
>>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>>> @@ -3637,3 +3637,21 @@ enum v4l2_mpeg_video_hevc_size_of_length_field 
>>>>> -
>>>>>        - Selecting this value specifies that HEVC slices are expected
>>>>>          to be prefixed by Annex B start codes. According to 
>>>>> :ref:`hevc`
>>>>>          valid start codes can be 3-bytes 0x000001 or 4-bytes 
>>>>> 0x00000001.
>>>>> +
>>>>> +``V4L2_CID_MPEG_VIDEO_LTR_COUNT (integer)``
>>>>> +       Specifies the number of Long Term Reference (LTR) frames 
>>>>> encoder needs
>>>>> +       to generate or keep. This is applicable to the H264 and HEVC 
>>>>> encoders.
>>>>> +
>>>>> +``V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX (integer)``
>>>>> +       The current frame is marked as a Long Term Reference (LTR) 
>>>>> frame
>>>>> +       and given this LTR index which ranges from 0 to LTR_COUNT-1.
>>>>> +       This is applicable to the H264 and HEVC encoders and can be 
>>>>> applied using
>>>>> +       Request API.
>>>>
>>>> You mentioned in reply to my comment that the venus driver didn't 
>>>> support the
>>>> Request API that it is also possible to use it without that API.
>>>>
>>>> But that requires more precise documentation. I assume that without the 
>>>> Request
>>>> API you would set this control, then queue the buffer containing the 
>>>> frame this
>>>> control should apply to, then wait until it is dequeued. Since that's 
>>>> the only
>>>> way you can be certain this control is applied to the correct frame.
>>>>
>>>> Is this indeed what you do in your application?
>>>>
>>>> Regards,
>>>>
>>>>         Hans
>>>>
>>> Hi Hans,
>>>
>>> Yes, It is possible without request API as well in a non-synchronized 
>>> way.
>>> And we don't need to wait for the frame to be dequeued.
>>> The driver implementation ensures that whenever the LTR control is 
>>> received,
>>> it applies to the frame received after that. Not to frame which would be 
>>> encoded next.
>>> So that it is at least synchronized between driver & encoder.
>>
>> This is highly driver dependent. I'm not even sure this is true for the venus
>> driver: if you prequeue, say, 4 output buffers to the encoder and call
>> V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX after the second buffer (so it should
>> apply to the third), and only after the fourth you call VIDIOC_STREAMON,
>> does the venus driver still keep track of the order of the queued buffers
>> and when these controls are set? Once STREAMON is called it looks like it
>> stays synced since everything is queued into a command queue, if I understand
>> the code correctly.
>>
>> The problem is that when controls are applied in relation to queued buffers
>> is not defined, unless you use the Request API. Typically controls are applied
>> immediately, so the venus driver is a bit of an anomaly in that respect.
>>
>> You can make an explicit requirement that these controls apply to the next
>> queued buffer if no request API is used, but you really must be 100% certain
>> that the venus driver does that right (and as mentioned, I have my doubts
>> about
>> the case where you queue buffers before calling STREAMON).
> 
> Do you propose to start usign request for stateful encoder ? If this is the
> case, I'd like to remind that it's not always possible to notify encode
> completion in request queue order for this type of HW. Reordering might be
> implicit in the firmware design, so the driver may not have any notification
> until multiple frames have been encoded.
> 
> To resume, we can use request for this type of application, no issues, but
> userspace may not switch to waiting on the request for completion as this may
> have HW specific behaviour. It will have to resort to polling for READ, and
> dequeue from capture queue and figure-out after the fact which request are now
> complete.

Good point. Perhaps we should hold off using the request api here, at least for
the time being.

Regards,

	Hans

> 
> Nicolas
> 
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>> Thanks,
>>> Dikshita
>>>
>>>>> +       Source Rec. ITU-T H.264 (06/2019); Table 7.9
>>>>> +
>>>>> +``V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES (bitmask)``
>>>>> +       Specifies the Long Term Reference (LTR) frame(s) to be used 
>>>>> for
>>>>> +       encoding the current frame.
>>>>> +       This provides a bitmask which consists of bits [0, 
>>>>> LTR_COUNT-1].
>>>>> +       This is applicable to the H264 and HEVC encoders and can be 
>>>>> applied using
>>>>> +       Request API.
>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
>>>>> b/drivers/media/v4l2-core/v4l2-ctrls.c
>>>>> index 16ab54f..84c1eb8 100644
>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>>>> @@ -950,6 +950,9 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>>>         case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:             return
>>>>> "Vertical MV 
>>>>> Search Range";
>>>>>         case V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER:             return
>>>>> "Repeat Sequence 
>>>>> Header";
>>>>>         case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:               return
>>>>> "Force Key Frame";
>>>>> +       case V4L2_CID_MPEG_VIDEO_LTR_COUNT:                     return
>>>>> "LTR Count";
>>>>> +       case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:               return
>>>>> "Frame LTR Index";
>>>>> +       case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:                return
>>>>> "Use LTR Frames";
>>>>>         case V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS:            return
>>>>> "MPEG-2 Slice 
>>>>> Parameters";
>>>>>         case V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION:            return
>>>>> "MPEG-2 
>>>>> Quantization Matrices";
>>>>>         case V4L2_CID_FWHT_I_FRAME_QP:                          return
>>>>> "FWHT I-Frame QP Value";
>>>>> @@ -1277,6 +1280,17 @@ void v4l2_ctrl_fill(u32 id, const char **name, 
>>>>> enum v4l2_ctrl_type *type,
>>>>>         case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:
>>>>>                 *type = V4L2_CTRL_TYPE_INTEGER;
>>>>>                 break;
>>>>> +       case V4L2_CID_MPEG_VIDEO_LTR_COUNT:
>>>>> +               *type = V4L2_CTRL_TYPE_INTEGER;
>>>>> +               break;
>>>>> +       case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:
>>>>> +               *type = V4L2_CTRL_TYPE_INTEGER;
>>>>> +               *flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
>>>>> +               break;
>>>>> +       case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:
>>>>> +               *type = V4L2_CTRL_TYPE_BITMASK;
>>>>> +               *flags |= V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
>>>>> +               break;
>>>>>         case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
>>>>>         case V4L2_CID_PAN_RESET:
>>>>>         case V4L2_CID_TILT_RESET:
>>>>> diff --git a/include/uapi/linux/v4l2-controls.h 
>>>>> b/include/uapi/linux/v4l2-controls.h
>>>>> index af8dda2..c0bb87b 100644
>>>>> --- a/include/uapi/linux/v4l2-controls.h
>>>>> +++ b/include/uapi/linux/v4l2-controls.h
>>>>> @@ -422,6 +422,9 @@ enum v4l2_mpeg_video_multi_slice_mode {
>>>>>  #define 
>>>>> V4L2_CID_MPEG_VIDEO_MV_H_SEARCH_RANGE           (V4L2_CID_CODEC_BASE+227
>>>>> )
>>>>>  #define 
>>>>> V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE           (V4L2_CID_CODEC_BASE+228
>>>>> )
>>>>>  #define 
>>>>> V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME             (V4L2_CID_CODEC_BASE+229
>>>>> )
>>>>> +#define
>>>>> V4L2_CID_MPEG_VIDEO_LTR_COUNT                  (V4L2_CID_CODEC_BASE+230)
>>>>> +#define 
>>>>> V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX             (V4L2_CID_CODEC_BASE+231
>>>>> )
>>>>> +#define
>>>>> V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES             (V4L2_CID_CODEC_BASE+232)
>>>>>
>>>>>  /* CIDs for the MPEG-2 Part 2 (H.262) codec */
>>>>>  #define
>>>>> V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL                        (V4L2_CID_CODEC_B
>>>>> ASE+270)
>>>>>
>>
> 
> 

[PATCH v6 0/2] Add encoder ctrls for long term reference

[Dikshita Agarwal]

This series add the encoder controls for long term reference (LTR)
and support for the same in venus driver.

Dikshita Agarwal (2):
  media: v4l2-ctrl: add controls for long term reference.
  venus: venc: Add support for Long Term Reference (LTR) controls

 .../userspace-api/media/v4l/ext-ctrls-codec.rst    | 18 ++++++++
 drivers/media/platform/qcom/venus/venc_ctrls.c     | 49 +++++++++++++++++++++-
 drivers/media/v4l2-core/v4l2-ctrls.c               | 14 +++++++
 include/uapi/linux/v4l2-controls.h                 |  3 ++
 4 files changed, 83 insertions(+), 1 deletion(-)

-- 
2.7.4