[PATCH v1 0/2] Intra-refresh type control

[PATCH v1 0/2] Intra-refresh type control

[Dikshita Agarwal]

Hi,

This series add a new intra-refresh type control for encoders.
this can be used to specify which intra refresh to be enabled,
random, cyclic or none.

Change since v0:
 Dropped INTRA_REFRESH_TYPE_NONE as it was not needed.
 Intra refresh period value as zero will disable the intra
 refresh.

Thanks,
Dikshita

Dikshita Agarwal (2):
  media: v4l2-ctrls: Add intra-refresh type control
  venus: venc: Add support for intra-refresh mode

 .../userspace-api/media/v4l/ext-ctrls-codec.rst    | 23 ++++++++++++++++++++++
 drivers/media/platform/qcom/venus/core.h           |  1 +
 drivers/media/platform/qcom/venus/venc.c           |  3 ++-
 drivers/media/platform/qcom/venus/venc_ctrls.c     |  8 ++++++++
 drivers/media/v4l2-core/v4l2-ctrls-defs.c          |  9 +++++++++
 include/uapi/linux/v4l2-controls.h                 |  5 +++++
 6 files changed, 48 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH v1 1/2] media: v4l2-ctrls: Add intra-refresh type control

[Dikshita Agarwal]

From: Dikshita Agarwal <quic_dikshita@quicinc.com>

Add a control to set intra-refresh type.

Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
---
 .../userspace-api/media/v4l/ext-ctrls-codec.rst    | 23 ++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-ctrls-defs.c          |  9 +++++++++
 include/uapi/linux/v4l2-controls.h                 |  5 +++++
 3 files changed, 37 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index e141f0e..54b42e1 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1180,6 +1180,29 @@ enum v4l2_mpeg_video_h264_entropy_mode -
     is set to non zero value.
     Applicable to H264, H263 and MPEG4 encoder.
 
+``V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE (enum)``
+
+enum v4l2_mpeg_video_intra_refresh_type -
+    Sets the type of intra refresh. The period to refresh
+    the whole frame is specified by V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD.
+    Note if the client sets this control to either ``V4L2_MPEG_VIDEO_INTRA_REFRESH_RANDOM``
+    or ``V4L2_MPEG_VIDEO_INTRA_REFRESH_CYCLIC`` the ``V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB``
+    control shall be ignored.
+    Applicable to H264, H263 and MPEG4 encoder. Possible values are:
+
+.. tabularcolumns:: |p{9.6cm}|p{7.9cm}|
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+
+    * - ``V4L2_MPEG_VIDEO_INTRA_REFRESH_RANDOM``
+      - The whole frame is completely refreshed randomly
+      after the specified period.
+    * - ``V4L2_MPEG_VIDEO_INTRA_REFRESH_CYCLIC``
+      - The whole frame MBs are completely refreshed in cyclic order
+      after the specified period.
+
 ``V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD (integer)``
     Intra macroblock refresh period. This sets the period to refresh
     the whole frame. In other words, this defines the number of frames
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
index 54ca4e6..f13f587 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
@@ -572,6 +572,11 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		"VBV/CPB Limit",
 		NULL,
 	};
+	static const char * const intra_refresh_type[] = {
+		"Random",
+		"Cyclic",
+		NULL,
+	};
 
 	switch (id) {
 	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
@@ -705,6 +710,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		return hevc_start_code;
 	case V4L2_CID_CAMERA_ORIENTATION:
 		return camera_orientation;
+	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE:
+		return intra_refresh_type;
 	default:
 		return NULL;
 	}
@@ -834,6 +841,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_MPEG_VIDEO_DECODER_SLICE_INTERFACE:	return "Decoder Slice Interface";
 	case V4L2_CID_MPEG_VIDEO_DECODER_MPEG4_DEBLOCK_FILTER:	return "MPEG4 Loop Filter Enable";
 	case V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB:	return "Number of Intra Refresh MBs";
+	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE:		return "Intra Refresh Type";
 	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD:		return "Intra Refresh Period";
 	case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:		return "Frame Level Rate Control Enable";
 	case V4L2_CID_MPEG_VIDEO_MB_RC_ENABLE:			return "H264 MB Level Rate Control";
@@ -1360,6 +1368,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_STATELESS_H264_DECODE_MODE:
 	case V4L2_CID_STATELESS_H264_START_CODE:
 	case V4L2_CID_CAMERA_ORIENTATION:
+	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE:
 		*type = V4L2_CTRL_TYPE_MENU;
 		break;
 	case V4L2_CID_LINK_FREQ:
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index c8e0f84..9650b71 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -443,6 +443,11 @@ enum v4l2_mpeg_video_multi_slice_mode {
 #define V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES		(V4L2_CID_CODEC_BASE+234)
 #define V4L2_CID_MPEG_VIDEO_DEC_CONCEAL_COLOR		(V4L2_CID_CODEC_BASE+235)
 #define V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD	(V4L2_CID_CODEC_BASE+236)
+#define V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE		(V4L2_CID_CODEC_BASE+237)
+enum v4l2_mpeg_video_intra_refresh_type {
+	V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_RANDOM	= 0,
+	V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_CYCLIC	= 1,
+};
 
 /* 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 v1 2/2] venus: venc: Add support for intra-refresh mode

[Dikshita Agarwal]

From: Dikshita Agarwal <quic_dikshita@quicinc.com>

Add support for intra-refresh type v4l2 control.

Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
---
 drivers/media/platform/qcom/venus/core.h       | 1 +
 drivers/media/platform/qcom/venus/venc.c       | 3 ++-
 drivers/media/platform/qcom/venus/venc_ctrls.c | 8 ++++++++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 7c3bac0..814ec3c 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -260,6 +260,7 @@ struct venc_controls {
 
 	u32 header_mode;
 	bool aud_enable;
+	u32 intra_refresh_mode;
 	u32 intra_refresh_period;
 
 	struct {
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 84bafc3..e8b8135 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -893,8 +893,9 @@ static int venc_set_properties(struct venus_inst *inst)
 				mbs++;
 			mbs /= ctr->intra_refresh_period;
 
-			intra_refresh.mode = HFI_INTRA_REFRESH_RANDOM;
 			intra_refresh.cir_mbs = mbs;
+			if (ctr->intra_refresh_mode == V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_RANDOM)
+				intra_refresh.mode = HFI_INTRA_REFRESH_RANDOM;
 		}
 
 		ptype = HFI_PROPERTY_PARAM_VENC_INTRA_REFRESH;
diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
index 1ada42d..c54b46f 100644
--- a/drivers/media/platform/qcom/venus/venc_ctrls.c
+++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
@@ -316,6 +316,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
 		ctr->mastering = *ctrl->p_new.p_hdr10_mastering;
 		break;
+	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE:
+		ctr->intra_refresh_mode = ctrl->val;
+		break;
 	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD:
 		ctr->intra_refresh_period = ctrl->val;
 		break;
@@ -582,6 +585,11 @@ int venc_ctrl_init(struct venus_inst *inst)
 				   V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY,
 				   v4l2_ctrl_ptr_create(NULL));
 
+	v4l2_ctrl_new_std_menu(&inst->ctrl_handler, &venc_ctrl_ops,
+			       V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE,
+			       V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_RANDOM,
+			       0, V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_RANDOM);
+
 	v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
 			  V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD, 0,
 			  ((4096 * 2304) >> 8), 1, 0);
-- 
2.7.4

Re: [PATCH v1 1/2] media: v4l2-ctrls: Add intra-refresh type control

[Nicolas Dufresne]

Le lundi 24 janvier 2022 à 15:41 +0530, Dikshita Agarwal a écrit :
> From: Dikshita Agarwal <quic_dikshita@quicinc.com>
> 
> Add a control to set intra-refresh type.
> 
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>

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

> ---
>  .../userspace-api/media/v4l/ext-ctrls-codec.rst    | 23 ++++++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c          |  9 +++++++++
>  include/uapi/linux/v4l2-controls.h                 |  5 +++++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index e141f0e..54b42e1 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1180,6 +1180,29 @@ enum v4l2_mpeg_video_h264_entropy_mode -
>      is set to non zero value.
>      Applicable to H264, H263 and MPEG4 encoder.
>  
> +``V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE (enum)``
> +
> +enum v4l2_mpeg_video_intra_refresh_type -
> +    Sets the type of intra refresh. The period to refresh
> +    the whole frame is specified by V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD.
> +    Note if the client sets this control to either ``V4L2_MPEG_VIDEO_INTRA_REFRESH_RANDOM``
> +    or ``V4L2_MPEG_VIDEO_INTRA_REFRESH_CYCLIC`` the ``V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB``
> +    control shall be ignored.
> +    Applicable to H264, H263 and MPEG4 encoder. Possible values are:
> +
> +.. tabularcolumns:: |p{9.6cm}|p{7.9cm}|
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - ``V4L2_MPEG_VIDEO_INTRA_REFRESH_RANDOM``
> +      - The whole frame is completely refreshed randomly
> +      after the specified period.
> +    * - ``V4L2_MPEG_VIDEO_INTRA_REFRESH_CYCLIC``
> +      - The whole frame MBs are completely refreshed in cyclic order
> +      after the specified period.
> +
>  ``V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD (integer)``
>      Intra macroblock refresh period. This sets the period to refresh
>      the whole frame. In other words, this defines the number of frames
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 54ca4e6..f13f587 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -572,6 +572,11 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"VBV/CPB Limit",
>  		NULL,
>  	};
> +	static const char * const intra_refresh_type[] = {
> +		"Random",
> +		"Cyclic",
> +		NULL,
> +	};
>  
>  	switch (id) {
>  	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
> @@ -705,6 +710,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		return hevc_start_code;
>  	case V4L2_CID_CAMERA_ORIENTATION:
>  		return camera_orientation;
> +	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE:
> +		return intra_refresh_type;
>  	default:
>  		return NULL;
>  	}
> @@ -834,6 +841,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_DECODER_SLICE_INTERFACE:	return "Decoder Slice Interface";
>  	case V4L2_CID_MPEG_VIDEO_DECODER_MPEG4_DEBLOCK_FILTER:	return "MPEG4 Loop Filter Enable";
>  	case V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB:	return "Number of Intra Refresh MBs";
> +	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE:		return "Intra Refresh Type";
>  	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD:		return "Intra Refresh Period";
>  	case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:		return "Frame Level Rate Control Enable";
>  	case V4L2_CID_MPEG_VIDEO_MB_RC_ENABLE:			return "H264 MB Level Rate Control";
> @@ -1360,6 +1368,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_STATELESS_H264_DECODE_MODE:
>  	case V4L2_CID_STATELESS_H264_START_CODE:
>  	case V4L2_CID_CAMERA_ORIENTATION:
> +	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE:
>  		*type = V4L2_CTRL_TYPE_MENU;
>  		break;
>  	case V4L2_CID_LINK_FREQ:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index c8e0f84..9650b71 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -443,6 +443,11 @@ enum v4l2_mpeg_video_multi_slice_mode {
>  #define V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES		(V4L2_CID_CODEC_BASE+234)
>  #define V4L2_CID_MPEG_VIDEO_DEC_CONCEAL_COLOR		(V4L2_CID_CODEC_BASE+235)
>  #define V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD	(V4L2_CID_CODEC_BASE+236)
> +#define V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE		(V4L2_CID_CODEC_BASE+237)
> +enum v4l2_mpeg_video_intra_refresh_type {
> +	V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_RANDOM	= 0,
> +	V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_CYCLIC	= 1,
> +};
>  
>  /* CIDs for the MPEG-2 Part 2 (H.262) codec */
>  #define V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL			(V4L2_CID_CODEC_BASE+270)

Re: [PATCH v1 1/2] media: v4l2-ctrls: Add intra-refresh type control

[Hans Verkuil]

Hi Dikshita,

Some comments below:

On 1/24/22 11:11, Dikshita Agarwal wrote:
> From: Dikshita Agarwal <quic_dikshita@quicinc.com>
> 
> Add a control to set intra-refresh type.
> 
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> ---
>  .../userspace-api/media/v4l/ext-ctrls-codec.rst    | 23 ++++++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c          |  9 +++++++++
>  include/uapi/linux/v4l2-controls.h                 |  5 +++++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index e141f0e..54b42e1 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1180,6 +1180,29 @@ enum v4l2_mpeg_video_h264_entropy_mode -
>      is set to non zero value.
>      Applicable to H264, H263 and MPEG4 encoder.
>  
> +``V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE (enum)``
> +
> +enum v4l2_mpeg_video_intra_refresh_type -
> +    Sets the type of intra refresh. The period to refresh
> +    the whole frame is specified by V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD.
> +    Note if the client sets this control to either ``V4L2_MPEG_VIDEO_INTRA_REFRESH_RANDOM``
> +    or ``V4L2_MPEG_VIDEO_INTRA_REFRESH_CYCLIC`` the ``V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB``
> +    control shall be ignored.

Since this control has only two possible values, that would mean that, if this control
is present, then REFRESH_MB is always ignored.

It seems to me that you need a third option here that specifically selects the REFRESH_MB
method.

Also, this needs to be documented as well in REFRESH_MB (i.e. it is ignored if this TYPE
control is present and is set to something other than REFRESH_MB).

> +    Applicable to H264, H263 and MPEG4 encoder. Possible values are:
> +
> +.. tabularcolumns:: |p{9.6cm}|p{7.9cm}|
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - ``V4L2_MPEG_VIDEO_INTRA_REFRESH_RANDOM``

I think you should add _TYPE after REFRESH in these names to clearly specify
that this is setting the refresh *type*.

> +      - The whole frame is completely refreshed randomly
> +      after the specified period.
> +    * - ``V4L2_MPEG_VIDEO_INTRA_REFRESH_CYCLIC``
> +      - The whole frame MBs are completely refreshed in cyclic order
> +      after the specified period.
> +
>  ``V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD (integer)``
>      Intra macroblock refresh period. This sets the period to refresh
>      the whole frame. In other words, this defines the number of frames
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 54ca4e6..f13f587 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -572,6 +572,11 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"VBV/CPB Limit",
>  		NULL,
>  	};
> +	static const char * const intra_refresh_type[] = {
> +		"Random",
> +		"Cyclic",
> +		NULL,
> +	};
>  
>  	switch (id) {
>  	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
> @@ -705,6 +710,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		return hevc_start_code;
>  	case V4L2_CID_CAMERA_ORIENTATION:
>  		return camera_orientation;
> +	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE:
> +		return intra_refresh_type;
>  	default:
>  		return NULL;
>  	}
> @@ -834,6 +841,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_DECODER_SLICE_INTERFACE:	return "Decoder Slice Interface";
>  	case V4L2_CID_MPEG_VIDEO_DECODER_MPEG4_DEBLOCK_FILTER:	return "MPEG4 Loop Filter Enable";
>  	case V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB:	return "Number of Intra Refresh MBs";
> +	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE:		return "Intra Refresh Type";
>  	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD:		return "Intra Refresh Period";
>  	case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:		return "Frame Level Rate Control Enable";
>  	case V4L2_CID_MPEG_VIDEO_MB_RC_ENABLE:			return "H264 MB Level Rate Control";
> @@ -1360,6 +1368,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_STATELESS_H264_DECODE_MODE:
>  	case V4L2_CID_STATELESS_H264_START_CODE:
>  	case V4L2_CID_CAMERA_ORIENTATION:
> +	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE:
>  		*type = V4L2_CTRL_TYPE_MENU;
>  		break;
>  	case V4L2_CID_LINK_FREQ:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index c8e0f84..9650b71 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -443,6 +443,11 @@ enum v4l2_mpeg_video_multi_slice_mode {
>  #define V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES		(V4L2_CID_CODEC_BASE+234)
>  #define V4L2_CID_MPEG_VIDEO_DEC_CONCEAL_COLOR		(V4L2_CID_CODEC_BASE+235)
>  #define V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD	(V4L2_CID_CODEC_BASE+236)
> +#define V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE		(V4L2_CID_CODEC_BASE+237)
> +enum v4l2_mpeg_video_intra_refresh_type {
> +	V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_RANDOM	= 0,
> +	V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_CYCLIC	= 1,
> +};
>  
>  /* CIDs for the MPEG-2 Part 2 (H.262) codec */
>  #define V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL			(V4L2_CID_CODEC_BASE+270)

Regards,

	Hans

Re: [PATCH v1 1/2] media: v4l2-ctrls: Add intra-refresh type control

[dikshita]

On 2022-02-15 13:51, Hans Verkuil wrote:
> Hi Dikshita,
> 
> Some comments below:
> 
> On 1/24/22 11:11, Dikshita Agarwal wrote:
>> From: Dikshita Agarwal <quic_dikshita@quicinc.com>
>> 
>> Add a control to set intra-refresh type.
>> 
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>> ---
>>  .../userspace-api/media/v4l/ext-ctrls-codec.rst    | 23 
>> ++++++++++++++++++++++
>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c          |  9 +++++++++
>>  include/uapi/linux/v4l2-controls.h                 |  5 +++++
>>  3 files changed, 37 insertions(+)
>> 
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst 
>> b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> index e141f0e..54b42e1 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> @@ -1180,6 +1180,29 @@ enum v4l2_mpeg_video_h264_entropy_mode -
>>      is set to non zero value.
>>      Applicable to H264, H263 and MPEG4 encoder.
>> 
>> +``V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE (enum)``
>> +
>> +enum v4l2_mpeg_video_intra_refresh_type -
>> +    Sets the type of intra refresh. The period to refresh
>> +    the whole frame is specified by 
>> V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD.
>> +    Note if the client sets this control to either 
>> ``V4L2_MPEG_VIDEO_INTRA_REFRESH_RANDOM``
>> +    or ``V4L2_MPEG_VIDEO_INTRA_REFRESH_CYCLIC`` the 
>> ``V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB``
>> +    control shall be ignored.
> 
> Since this control has only two possible values, that would mean that,
> if this control
> is present, then REFRESH_MB is always ignored.
> 
> It seems to me that you need a third option here that specifically
> selects the REFRESH_MB
> method.
> 
> Also, this needs to be documented as well in REFRESH_MB (i.e. it is
> ignored if this TYPE
> control is present and is set to something other than REFRESH_MB).
> 

Hi Hans,

I don't think we need to add that as the third option in this control.

So, there are two ways to set intra refresh to driver, it can be either 
MB based or Frame-based.
Currently, we have two v4l2 controls in place
1. V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB -> this is MB based and 
only applicable for cyclic
2. V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD -> this is frame based and 
has no type associated to it
    and it is up to the driver to decide the type i.e Random or Cyclic.

with this new control V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE, we are 
introducing
a way for the client to set the type of intra refresh, either cyclic or 
random.

Thanks,
Dikshita

>> +    Applicable to H264, H263 and MPEG4 encoder. Possible values are:
>> +
>> +.. tabularcolumns:: |p{9.6cm}|p{7.9cm}|
>> +
>> +.. flat-table::
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +
>> +    * - ``V4L2_MPEG_VIDEO_INTRA_REFRESH_RANDOM``
> 
> I think you should add _TYPE after REFRESH in these names to clearly 
> specify
> that this is setting the refresh *type*.
> 
>> +      - The whole frame is completely refreshed randomly
>> +      after the specified period.
>> +    * - ``V4L2_MPEG_VIDEO_INTRA_REFRESH_CYCLIC``
>> +      - The whole frame MBs are completely refreshed in cyclic order
>> +      after the specified period.
>> +
>>  ``V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD (integer)``
>>      Intra macroblock refresh period. This sets the period to refresh
>>      the whole frame. In other words, this defines the number of 
>> frames
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c 
>> b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> index 54ca4e6..f13f587 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> @@ -572,6 +572,11 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>  		"VBV/CPB Limit",
>>  		NULL,
>>  	};
>> +	static const char * const intra_refresh_type[] = {
>> +		"Random",
>> +		"Cyclic",
>> +		NULL,
>> +	};
>> 
>>  	switch (id) {
>>  	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
>> @@ -705,6 +710,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>  		return hevc_start_code;
>>  	case V4L2_CID_CAMERA_ORIENTATION:
>>  		return camera_orientation;
>> +	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE:
>> +		return intra_refresh_type;
>>  	default:
>>  		return NULL;
>>  	}
>> @@ -834,6 +841,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_MPEG_VIDEO_DECODER_SLICE_INTERFACE:	return "Decoder 
>> Slice Interface";
>>  	case V4L2_CID_MPEG_VIDEO_DECODER_MPEG4_DEBLOCK_FILTER:	return "MPEG4 
>> Loop Filter Enable";
>>  	case V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB:	return "Number of 
>> Intra Refresh MBs";
>> +	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE:		return "Intra Refresh 
>> Type";
>>  	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD:		return "Intra 
>> Refresh Period";
>>  	case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:		return "Frame Level Rate 
>> Control Enable";
>>  	case V4L2_CID_MPEG_VIDEO_MB_RC_ENABLE:			return "H264 MB Level Rate 
>> Control";
>> @@ -1360,6 +1368,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, 
>> enum v4l2_ctrl_type *type,
>>  	case V4L2_CID_STATELESS_H264_DECODE_MODE:
>>  	case V4L2_CID_STATELESS_H264_START_CODE:
>>  	case V4L2_CID_CAMERA_ORIENTATION:
>> +	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE:
>>  		*type = V4L2_CTRL_TYPE_MENU;
>>  		break;
>>  	case V4L2_CID_LINK_FREQ:
>> diff --git a/include/uapi/linux/v4l2-controls.h 
>> b/include/uapi/linux/v4l2-controls.h
>> index c8e0f84..9650b71 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -443,6 +443,11 @@ enum v4l2_mpeg_video_multi_slice_mode {
>>  #define V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES		(V4L2_CID_CODEC_BASE+234)
>>  #define 
>> V4L2_CID_MPEG_VIDEO_DEC_CONCEAL_COLOR		(V4L2_CID_CODEC_BASE+235)
>>  #define 
>> V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD	(V4L2_CID_CODEC_BASE+236)
>> +#define 
>> V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE		(V4L2_CID_CODEC_BASE+237)
>> +enum v4l2_mpeg_video_intra_refresh_type {
>> +	V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_RANDOM	= 0,
>> +	V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_CYCLIC	= 1,
>> +};
>> 
>>  /* CIDs for the MPEG-2 Part 2 (H.262) codec */
>>  #define V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL			(V4L2_CID_CODEC_BASE+270)
> 
> Regards,
> 
> 	Hans

Re: [PATCH v1 1/2] media: v4l2-ctrls: Add intra-refresh type control

[Hans Verkuil]

Hi Dikshita,

On 2/21/22 07:02, dikshita@codeaurora.org wrote:
> On 2022-02-15 13:51, Hans Verkuil wrote:
>> Hi Dikshita,
>>
>> Some comments below:
>>
>> On 1/24/22 11:11, Dikshita Agarwal wrote:
>>> From: Dikshita Agarwal <quic_dikshita@quicinc.com>
>>>
>>> Add a control to set intra-refresh type.
>>>
>>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>>> ---
>>>  .../userspace-api/media/v4l/ext-ctrls-codec.rst    | 23 
>>> ++++++++++++++++++++++
>>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c          |  9 +++++++++
>>>  include/uapi/linux/v4l2-controls.h                 |  5 +++++
>>>  3 files changed, 37 insertions(+)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst 
>>> b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> index e141f0e..54b42e1 100644
>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> @@ -1180,6 +1180,29 @@ enum v4l2_mpeg_video_h264_entropy_mode -
>>>      is set to non zero value.
>>>      Applicable to H264, H263 and MPEG4 encoder.
>>>
>>> +``V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE (enum)``
>>> +
>>> +enum v4l2_mpeg_video_intra_refresh_type -
>>> +    Sets the type of intra refresh. The period to refresh
>>> +    the whole frame is specified by 
>>> V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD.
>>> +    Note if the client sets this control to either 
>>> ``V4L2_MPEG_VIDEO_INTRA_REFRESH_RANDOM``
>>> +    or ``V4L2_MPEG_VIDEO_INTRA_REFRESH_CYCLIC`` the 
>>> ``V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB``
>>> +    control shall be ignored.
>>
>> Since this control has only two possible values, that would mean that,
>> if this control
>> is present, then REFRESH_MB is always ignored.
>>
>> It seems to me that you need a third option here that specifically
>> selects the REFRESH_MB
>> method.
>>
>> Also, this needs to be documented as well in REFRESH_MB (i.e. it is
>> ignored if this TYPE
>> control is present and is set to something other than REFRESH_MB).
>>
> 
> Hi Hans,
> 
> I don't think we need to add that as the third option in this control.
> 
> So, there are two ways to set intra refresh to driver, it can be either 
> MB based or Frame-based.
> Currently, we have two v4l2 controls in place
> 1. V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB -> this is MB based and 
> only applicable for cyclic
> 2. V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD -> this is frame based and 
> has no type associated to it
>     and it is up to the driver to decide the type i.e Random or Cyclic.
> 
> with this new control V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE, we are 
> introducing
> a way for the client to set the type of intra refresh, either cyclic or 
> random.

Ah, right. But then it really is a naming issue that causes the confusion.

It would all be clear if you name this control V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD_TYPE
and the enums V4L2_MPEG_VIDEO_INTRA_REFRESH_PERIOD_TYPE_RANDOM and
V4L2_MPEG_VIDEO_INTRA_REFRESH_PERIOD_TYPE_CYCLIC.

Yes, I know, the names are quite long, but there are so many codec controls that
being descriptive is more important than being concise.

With these new names it is clear that this type control only relates to the
refresh period and not to the refresh MB.

You should also document that if this type control is not present, then it is
undefined what refresh period type is used (or something along those lines).

Regards,

	Hans

> 
> Thanks,
> Dikshita
> 
>>> +    Applicable to H264, H263 and MPEG4 encoder. Possible values are:
>>> +
>>> +.. tabularcolumns:: |p{9.6cm}|p{7.9cm}|
>>> +
>>> +.. flat-table::
>>> +    :header-rows:  0
>>> +    :stub-columns: 0
>>> +
>>> +    * - ``V4L2_MPEG_VIDEO_INTRA_REFRESH_RANDOM``
>>
>> I think you should add _TYPE after REFRESH in these names to clearly 
>> specify
>> that this is setting the refresh *type*.
>>
>>> +      - The whole frame is completely refreshed randomly
>>> +      after the specified period.
>>> +    * - ``V4L2_MPEG_VIDEO_INTRA_REFRESH_CYCLIC``
>>> +      - The whole frame MBs are completely refreshed in cyclic order
>>> +      after the specified period.
>>> +
>>>  ``V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD (integer)``
>>>      Intra macroblock refresh period. This sets the period to refresh
>>>      the whole frame. In other words, this defines the number of 
>>> frames
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c 
>>> b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>> index 54ca4e6..f13f587 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>> @@ -572,6 +572,11 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>>  		"VBV/CPB Limit",
>>>  		NULL,
>>>  	};
>>> +	static const char * const intra_refresh_type[] = {
>>> +		"Random",
>>> +		"Cyclic",
>>> +		NULL,
>>> +	};
>>>
>>>  	switch (id) {
>>>  	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
>>> @@ -705,6 +710,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>>  		return hevc_start_code;
>>>  	case V4L2_CID_CAMERA_ORIENTATION:
>>>  		return camera_orientation;
>>> +	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE:
>>> +		return intra_refresh_type;
>>>  	default:
>>>  		return NULL;
>>>  	}
>>> @@ -834,6 +841,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>  	case V4L2_CID_MPEG_VIDEO_DECODER_SLICE_INTERFACE:	return "Decoder 
>>> Slice Interface";
>>>  	case V4L2_CID_MPEG_VIDEO_DECODER_MPEG4_DEBLOCK_FILTER:	return "MPEG4 
>>> Loop Filter Enable";
>>>  	case V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB:	return "Number of 
>>> Intra Refresh MBs";
>>> +	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE:		return "Intra Refresh 
>>> Type";
>>>  	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD:		return "Intra 
>>> Refresh Period";
>>>  	case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:		return "Frame Level Rate 
>>> Control Enable";
>>>  	case V4L2_CID_MPEG_VIDEO_MB_RC_ENABLE:			return "H264 MB Level Rate 
>>> Control";
>>> @@ -1360,6 +1368,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, 
>>> enum v4l2_ctrl_type *type,
>>>  	case V4L2_CID_STATELESS_H264_DECODE_MODE:
>>>  	case V4L2_CID_STATELESS_H264_START_CODE:
>>>  	case V4L2_CID_CAMERA_ORIENTATION:
>>> +	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE:
>>>  		*type = V4L2_CTRL_TYPE_MENU;
>>>  		break;
>>>  	case V4L2_CID_LINK_FREQ:
>>> diff --git a/include/uapi/linux/v4l2-controls.h 
>>> b/include/uapi/linux/v4l2-controls.h
>>> index c8e0f84..9650b71 100644
>>> --- a/include/uapi/linux/v4l2-controls.h
>>> +++ b/include/uapi/linux/v4l2-controls.h
>>> @@ -443,6 +443,11 @@ enum v4l2_mpeg_video_multi_slice_mode {
>>>  #define V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES		(V4L2_CID_CODEC_BASE+234)
>>>  #define 
>>> V4L2_CID_MPEG_VIDEO_DEC_CONCEAL_COLOR		(V4L2_CID_CODEC_BASE+235)
>>>  #define 
>>> V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD	(V4L2_CID_CODEC_BASE+236)
>>> +#define 
>>> V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_TYPE		(V4L2_CID_CODEC_BASE+237)
>>> +enum v4l2_mpeg_video_intra_refresh_type {
>>> +	V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_RANDOM	= 0,
>>> +	V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_CYCLIC	= 1,
>>> +};
>>>
>>>  /* CIDs for the MPEG-2 Part 2 (H.262) codec */
>>>  #define V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL			(V4L2_CID_CODEC_BASE+270)
>>
>> Regards,
>>
>> 	Hans