From: Hans Verkuil <hverkuil+cisco@kernel.org>
To: Deepa Guthyappa Madivalara <deepa.madivalara@oss.qualcomm.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Vikash Garodia <vikash.garodia@oss.qualcomm.com>,
Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>,
Abhinav Kumar <abhinav.kumar@linux.dev>,
Bryan O'Donoghue <bod@kernel.org>
Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-msm@vger.kernel.org, kernel test robot <lkp@intel.com>
Subject: Re: [PATCH v2 2/3] media: v4l2-core: Add support for video encoder ROI control
Date: Wed, 15 Jul 2026 10:32:06 +0200 [thread overview]
Message-ID: <1ef14047-f385-4163-ad9d-414702f45695@kernel.org> (raw)
In-Reply-To: <20260714-enc_roi_enable-v2-2-63683f9dbcef@oss.qualcomm.com>
On 14/07/2026 21:00, Deepa Guthyappa Madivalara wrote:
> Add necessary support for controls V4L2_CID_MPEG_VIDEO_ROI_MB_DELTA_QP
> and V4L2_CID_MPEG_VIDEO_ROI_MB_SIZE.
>
> Signed-off-by: Deepa Guthyappa Madivalara <deepa.madivalara@oss.qualcomm.com>
> ---
> drivers/media/v4l2-core/v4l2-ctrls-api.c | 1 +
> drivers/media/v4l2-core/v4l2-ctrls-core.c | 40 +++++++++++++++++++++++++++++--
> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 10 ++++++++
> include/media/v4l2-ctrls.h | 3 ++-
> 4 files changed, 51 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> index 93d8d4012d0f4fef004e417d0aee2ae44b1b30bd..7d41cfd7378baaa929d4da0266c45f731bb54285 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> @@ -980,6 +980,7 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
> case V4L2_CTRL_TYPE_U8:
> case V4L2_CTRL_TYPE_U16:
> case V4L2_CTRL_TYPE_U32:
> + case V4L2_CTRL_TYPE_S8:
> if (ctrl->is_array)
> return -EINVAL;
> ret = check_range(ctrl->type, min, max, step, def);
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index ba047d7d86010bf0cf8f8fbf2dc343883d6bdae0..15b69dce0127e7c8546c2d23ae5458ace10301a0 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> @@ -287,6 +287,14 @@ static void __v4l2_ctrl_type_op_init(const struct v4l2_ctrl *ctrl, u32 from_idx,
> memset(ptr.p_u32 + from_idx, 0, elems * sizeof(u32));
> }
> break;
> + case V4L2_CTRL_TYPE_S8:
> + if (value) {
> + for (i = from_idx; i < tot_elems; i++)
> + ptr.p_s8[i] = value;
> + } else {
> + memset(ptr.p_s8 + from_idx, 0, elems * sizeof(s8));
> + }
> + break;
> default:
> for (i = from_idx; i < tot_elems; i++) {
> switch (which) {
> @@ -367,6 +375,9 @@ void v4l2_ctrl_type_op_log(const struct v4l2_ctrl *ctrl)
> case V4L2_CTRL_TYPE_U32:
> pr_cont("%u", (unsigned)*ptr.p_u32);
> break;
> + case V4L2_CTRL_TYPE_S8:
> + pr_cont("%d", *ptr.p_s8);
> + break;
> case V4L2_CTRL_TYPE_AREA:
> pr_cont("%ux%u", ptr.p_area->width, ptr.p_area->height);
> break;
> @@ -481,6 +492,21 @@ EXPORT_SYMBOL(v4l2_ctrl_type_op_log);
> 0; \
> })
>
> +#define ROUND_TO_RANGE_SIGNED(val, offset_type, ctrl) \
> +({ \
> + offset_type offset; \
> + if ((ctrl)->maximum >= 0 && \
> + val >= (ctrl)->maximum - (s32)((ctrl)->step / 2)) \
> + val = (ctrl)->maximum; \
> + else \
> + val += (s32)((ctrl)->step / 2); \
> + val = clamp_t(typeof(val), val, \
> + (ctrl)->minimum, (ctrl)->maximum); \
> + offset = (val) - (ctrl)->minimum; \
> + offset = (ctrl)->step * (offset / (s32)(ctrl)->step); \
> + val = (ctrl)->minimum + offset; \
> +})
Why is this new define needed? I think you can just use ROUND_TO_RANGE.
Regards,
Hans
> +
> /* Validate a new control */
>
> #define zero_padding(s) \
> @@ -1365,6 +1391,8 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> return -EINVAL;
> break;
>
> + case V4L2_CID_MPEG_VIDEO_ROI_MB_DELTA_QP:
> + break;
> default:
> return -EINVAL;
> }
> @@ -1378,6 +1406,7 @@ static int std_validate_elem(const struct v4l2_ctrl *ctrl, u32 idx,
> size_t len;
> u64 offset;
> s64 val;
> + s32 tmp;
>
> switch ((u32)ctrl->type) {
> case V4L2_CTRL_TYPE_INTEGER:
> @@ -1403,7 +1432,11 @@ static int std_validate_elem(const struct v4l2_ctrl *ctrl, u32 idx,
> return ROUND_TO_RANGE(ptr.p_u16[idx], u16, ctrl);
> case V4L2_CTRL_TYPE_U32:
> return ROUND_TO_RANGE(ptr.p_u32[idx], u32, ctrl);
> -
> + case V4L2_CTRL_TYPE_S8:
> + tmp = ptr.p_s8[idx];
> + ROUND_TO_RANGE_SIGNED(tmp, s32, ctrl);
> + ptr.p_s8[idx] = (s8)tmp;
> + return 0;
> case V4L2_CTRL_TYPE_BOOLEAN:
> ptr.p_s32[idx] = !!ptr.p_s32[idx];
> return 0;
> @@ -1556,6 +1589,7 @@ void cur_to_new(struct v4l2_ctrl *ctrl)
> return;
> if (ctrl->is_dyn_array)
> ctrl->new_elems = ctrl->elems;
> +
> ptr_to_ptr(ctrl, ctrl->p_cur, ctrl->p_new, ctrl->new_elems);
> }
>
> @@ -1998,6 +2032,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> case V4L2_CTRL_TYPE_U32:
> elem_size = sizeof(u32);
> break;
> + case V4L2_CTRL_TYPE_S8:
> + elem_size = sizeof(s8);
> + break;
> case V4L2_CTRL_TYPE_MPEG2_SEQUENCE:
> elem_size = sizeof(struct v4l2_ctrl_mpeg2_sequence);
> break;
> @@ -2215,7 +2252,6 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>
> if (flags & V4L2_CTRL_FLAG_HAS_WHICH_MIN_MAX) {
> void *ptr = ctrl->p_def.p;
> -
> if (p_min.p_const) {
> ptr += elem_size;
> ctrl->p_min.p = ptr;
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index e062f2088490470c42d6c579ff7675be454a29b0..8f895060799ea13443143edd398dfb97f4ba0085 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -974,6 +974,8 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_MPEG_VIDEO_AVERAGE_QP: return "Average QP Value";
> case V4L2_CID_FWHT_I_FRAME_QP: return "FWHT I-Frame QP Value";
> case V4L2_CID_FWHT_P_FRAME_QP: return "FWHT P-Frame QP Value";
> + case V4L2_CID_MPEG_VIDEO_ROI_MB_DELTA_QP: return "Encoder ROI MB Delta QP";
> + case V4L2_CID_MPEG_VIDEO_ROI_MB_SIZE: return "Encoder ROI MB Size";
>
> /* VPX controls */
> case V4L2_CID_MPEG_VIDEO_VPX_NUM_PARTITIONS: return "VPX Number of Partitions";
> @@ -1622,6 +1624,14 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
> *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
> break;
> + case V4L2_CID_MPEG_VIDEO_ROI_MB_DELTA_QP:
> + *type = V4L2_CTRL_TYPE_S8;
> + *flags |= V4L2_CTRL_FLAG_DYNAMIC_ARRAY;
> + break;
> + case V4L2_CID_MPEG_VIDEO_ROI_MB_SIZE:
> + *type = V4L2_CTRL_TYPE_U8;
> + *flags |= V4L2_CTRL_FLAG_READ_ONLY;
> + break;
> default:
> *type = V4L2_CTRL_TYPE_INTEGER;
> break;
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index a1806ddbc797efa52e83cd3f685ef70d5b5483d2..9a68a3555e349f1e55aa01af5c10f08201d62bae 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -27,6 +27,7 @@ struct video_device;
>
> /**
> * union v4l2_ctrl_ptr - A pointer to a control value.
> + * @p_s8: Pointer to a 8-bit signed value.
> * @p_s32: Pointer to a 32-bit signed value.
> * @p_s64: Pointer to a 64-bit signed value.
> * @p_u8: Pointer to a 8-bit unsigned value.
> @@ -61,10 +62,10 @@ struct video_device;
> * @p_const: Pointer to a constant compound value.
> */
> union v4l2_ctrl_ptr {
> + s8 *p_s8;
> s32 *p_s32;
> s64 *p_s64;
> u8 *p_u8;
> - s8 *p_s8;
> u16 *p_u16;
> u32 *p_u32;
> char *p_char;
>
next prev parent reply other threads:[~2026-07-15 8:32 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-07-14 19:00 [PATCH v2 0/3] Implement Region of Interest(ROI) support Deepa Guthyappa Madivalara
2026-07-14 19:00 ` [PATCH v2 1/3] media: uapi: Introduce new control for video encoder ROI Deepa Guthyappa Madivalara
2026-07-15 8:25 ` Hans Verkuil
2026-07-16 22:01 ` Deepa Guthyappa Madivalara
2026-07-14 19:00 ` [PATCH v2 2/3] media: v4l2-core: Add support for video encoder ROI control Deepa Guthyappa Madivalara
2026-07-15 8:32 ` Hans Verkuil [this message]
2026-07-16 21:56 ` Deepa Guthyappa Madivalara
2026-07-14 19:00 ` [PATCH v2 3/3] media: iris: Add ROI support framework for iris video encoder Deepa Guthyappa Madivalara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1ef14047-f385-4163-ad9d-414702f45695@kernel.org \
--to=hverkuil+cisco@kernel.org \
--cc=abhinav.kumar@linux.dev \
--cc=bod@kernel.org \
--cc=deepa.madivalara@oss.qualcomm.com \
--cc=dikshita.agarwal@oss.qualcomm.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=lkp@intel.com \
--cc=mchehab@kernel.org \
--cc=vikash.garodia@oss.qualcomm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox