RE: [PATCH v0 1/3] media: chips-media: wave5: Add Support for Background Detection
From: jackson . lee
Date: Thu Mar 19 2026 - 21:34:38 EST
Hi Nicolas
> -----Original Message-----
> From: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>
> Sent: Friday, March 20, 2026 6:18 AM
> To: jackson.lee <jackson.lee@xxxxxxxxxxxxxxx>; mchehab@xxxxxxxxxx;
> hverkuil-cisco@xxxxxxxxx; bob.beckett@xxxxxxxxxxxxx
> Cc: linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; lafley.kim
> <lafley.kim@xxxxxxxxxxxxxxx>; b-brnich@xxxxxx; hverkuil@xxxxxxxxx; Nas
> Chung <nas.chung@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v0 1/3] media: chips-media: wave5: Add Support for
> Background Detection
>
> Le jeudi 19 mars 2026 à 14:32 +0900, Jackson.lee a écrit :
> > From: Jackson Lee <jackson.lee@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>
> > ---
> > 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 ?
>
Background detection is disabled by default because not all use cases benefit from this feature.
When enabled, detected background regions are encoded with fewer bits or skip mode,
which reduces bitrate but may not be desirable in every scenario.
We chose to let the user explicitly opt-in rather than enable it by default.
Thanks for your review.
Jackson
> 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 {