Re: [PATCH v0 1/3] media: chips-media: wave5: Add Support for Background Detection

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

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

Le jeudi 19 mars 2026 à 14:32 +0900, Jackson.lee a écrit :
> From: Jackson Lee <jackson.lee@chipsnmedia.com>
> 
> Wave5 encoder can be configured to detect a background region in a frame.
> If a background region is detected, it will use less bits or skip mode to
> encode that region. This can assist in lowering the video bitrate for a
> given stream.

Since you are adding a generic control (and I think the feature is pretty clear
an generic), you should make this clear in the text. So start saying you are
adding a generic control for the background detection feature and then quote
that this will be implemented by wave5. Its just a nit on wording, I have no
doubt this option would also exist on other IP and be compatible through the
provided description.

> 
> Signed-off-by: Jackson Lee <jackson.lee@chipsnmedia.com>
> Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 5 +++++
>  drivers/media/platform/chips-media/wave5/wave5-hw.c       | 4 +++-
>  drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c  | 7 +++++++
>  drivers/media/platform/chips-media/wave5/wave5-vpuapi.h   | 1 +
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c                 | 2 ++
>  include/uapi/linux/v4l2-controls.h                        | 2 ++
>  6 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index c8890cb5e00a..b992a0d5c7bf 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -605,6 +605,11 @@ enum v4l2_mpeg_video_frame_skip_mode -
>      chosen data limit then the frame will be skipped. Possible values
>      are:
>  
> +``V4L2_CID_MPEG_VIDEO_BACKGROUND_DETECTION (boolean)``
> +    If enabled then, the encoder detect a background region in frame and

drop then

> +    use low bits or skip mode to encode the background region.
> +    If a lot of scenes are stationary or background, It may help to
> +    reduce the video bitrate. Applicable to the encoder.
>  
>  .. tabularcolumns:: |p{8.2cm}|p{9.3cm}|
>  
> diff --git a/drivers/media/platform/chips-media/wave5/wave5-hw.c
> b/drivers/media/platform/chips-media/wave5/wave5-hw.c
> index 687ce6ccf3ae..c516d125f553 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-hw.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-hw.c

Please, split the API addition from the driver changes.

> @@ -49,6 +49,7 @@
>  
>  #define FASTIO_ADDRESS_MASK		GENMASK(15, 0)
>  #define SEQ_PARAM_PROFILE_MASK		GENMASK(30, 24)
> +#define SEQ_BG_PARAM_REG_DATA		0x3800410
>  
>  static void _wave5_print_reg_err(struct vpu_device *vpu_dev, u32
> reg_fail_reason,
>  				 const char *func);
> @@ -1838,7 +1839,8 @@ int wave5_vpu_enc_init_seq(struct vpu_instance *inst)
>  	vpu_write_reg(inst->dev, W5_CMD_ENC_SEQ_RC_BIT_RATIO_LAYER_4_7, 0);
>  	vpu_write_reg(inst->dev, W5_CMD_ENC_SEQ_ROT_PARAM, rot_mir_mode);
>  
> -	vpu_write_reg(inst->dev, W5_CMD_ENC_SEQ_BG_PARAM, 0);
> +	vpu_write_reg(inst->dev, W5_CMD_ENC_SEQ_BG_PARAM,
> +		      SEQ_BG_PARAM_REG_DATA | p_param->bg_detection);
>  	vpu_write_reg(inst->dev, W5_CMD_ENC_SEQ_CUSTOM_LAMBDA_ADDR, 0);
>  	vpu_write_reg(inst->dev, W5_CMD_ENC_SEQ_CONF_WIN_TOP_BOT,
>  		      p_param->conf_win_bot << 16 | p_param->conf_win_top);
> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> index 7613fcdbafed..6fe01217233f 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> @@ -780,6 +780,9 @@ static int wave5_vpu_enc_s_ctrl(struct v4l2_ctrl *ctrl)
>  	case V4L2_CID_MPEG_VIDEO_BITRATE:
>  		inst->bit_rate = ctrl->val;
>  		break;
> +	case V4L2_CID_MPEG_VIDEO_BACKGROUND_DETECTION:
> +		inst->enc_param.bg_detection = ctrl->val;
> +		break;
>  	case V4L2_CID_MPEG_VIDEO_GOP_SIZE:
>  		inst->enc_param.avc_idr_period = ctrl->val;
>  		break;
> @@ -1205,6 +1208,7 @@ static int wave5_set_enc_openparam(struct enc_open_param
> *open_param,
>  	open_param->wave_param.beta_offset_div2 = input.beta_offset_div2;
>  	open_param->wave_param.decoding_refresh_type =
> input.decoding_refresh_type;
>  	open_param->wave_param.intra_period = input.intra_period;
> +	open_param->wave_param.bg_detection = input.bg_detection;
>  	if (inst->std == W_HEVC_ENC) {
>  		if (input.intra_period == 0) {
>  			open_param->wave_param.decoding_refresh_type =
> DEC_REFRESH_TYPE_IDR;
> @@ -1700,6 +1704,9 @@ static int wave5_vpu_open_enc(struct file *filp)
>  	v4l2_ctrl_new_std(v4l2_ctrl_hdl, &wave5_vpu_enc_ctrl_ops,
>  			  V4L2_CID_MPEG_VIDEO_AU_DELIMITER,
>  			  0, 1, 1, 1);
> +	v4l2_ctrl_new_std(v4l2_ctrl_hdl, &wave5_vpu_enc_ctrl_ops,
> +			  V4L2_CID_MPEG_VIDEO_BACKGROUND_DETECTION,
> +			  0, 1, 1, 0);

The hard question, last parameter here is def, so I read this is disabled by
default. What's the side effect of having this enabled by default ? Did you
already considered that option ?

regards,
Nicolas

>  	v4l2_ctrl_new_std(v4l2_ctrl_hdl, &wave5_vpu_enc_ctrl_ops,
>  			  V4L2_CID_HFLIP,
>  			  0, 1, 1, 0);
> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> index c64135769869..dc31689e0d27 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> @@ -570,6 +570,7 @@ struct enc_wave_param {
>  	u32 transform8x8_enable: 1; /* enable 8x8 intra prediction and 8x8
> transform */
>  	u32 mb_level_rc_enable: 1; /* enable MB-level rate control */
>  	u32 forced_idr_header_enable: 1; /* enable header encoding before IDR
> frame */
> +	u32 bg_detection: 1; /* enable background detection */
>  };
>  
>  struct enc_open_param {
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-
> core/v4l2-ctrls-defs.c
> index 551426c4cd01..e062f2088490 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -889,6 +889,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_DEC_DISPLAY_DELAY:		return
> "Display Delay";
>  	case V4L2_CID_MPEG_VIDEO_DEC_DISPLAY_DELAY_ENABLE:	return
> "Display Delay Enable";
>  	case V4L2_CID_MPEG_VIDEO_AU_DELIMITER:			return
> "Generate Access Unit Delimiters";
> +	case V4L2_CID_MPEG_VIDEO_BACKGROUND_DETECTION:		return
> "Background Detection";
>  	case V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP:		return "H263
> I-Frame QP Value";
>  	case V4L2_CID_MPEG_VIDEO_H263_P_FRAME_QP:		return "H263
> P-Frame QP Value";
>  	case V4L2_CID_MPEG_VIDEO_H263_B_FRAME_QP:		return "H263
> B-Frame QP Value";
> @@ -1296,6 +1297,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
> v4l2_ctrl_type *type,
>  	case V4L2_CID_MPEG_VIDEO_MPEG4_QPEL:
>  	case V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER:
>  	case V4L2_CID_MPEG_VIDEO_AU_DELIMITER:
> +	case V4L2_CID_MPEG_VIDEO_BACKGROUND_DETECTION:
>  	case V4L2_CID_WIDE_DYNAMIC_RANGE:
>  	case V4L2_CID_IMAGE_STABILIZATION:
>  	case V4L2_CID_RDS_RECEPTION:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-
> controls.h
> index 68dd0c4e47b2..939f796261c0 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -464,6 +464,8 @@ enum v4l2_mpeg_video_intra_refresh_period_type {
>  	V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD_TYPE_CYCLIC	= 1,
>  };
>  
> +#define
> V4L2_CID_MPEG_VIDEO_BACKGROUND_DETECTION	(V4L2_CID_CODEC_BASE+238)
> +
>  /* CIDs for the MPEG-2 Part 2 (H.262) codec */
>  #define
> V4L2_CID_MPEG_VIDEO_MPEG2_LEVEL			(V4L2_CID_CODEC_BASE+270)
>  enum v4l2_mpeg_video_mpeg2_level {

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