Hi Hans, On Tue, Oct 4, 2022 at 4:23 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote: > > Hi Yunke, > > On 10/4/22 07:39, Yunke Cao wrote: > > Hi Hans, > > > > Thanks for the review! > > > > On Fri, Sep 30, 2022 at 5:57 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote: > >> > >> > >> > >> On 9/30/22 08:40, Yunke Cao wrote: > >>> From: Hans Verkuil <hverkuil-cisco@xxxxxxxxx> > >>> > >>> Add the capability of retrieving the min and max values of a > >>> compound control. > >>> > >>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx> > >>> Signed-off-by: Yunke Cao <yunkec@xxxxxxxxxx> > >>> --- > >>> Hi Hans, > >>> > >>> I resubmitted because of the discussion in the previous version: > >>> [PATCH v7 5/7] v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL. > >>> It would be great if you can take a look, thanks! > >>> > >>> Changelog since v7: > >>> - Document that the definition of the min/max are provided by compound controls > >>> are defined in control documentation. > >>> - Return error, instead of zeroed memory for v4l2_ctrl_ptr_create(NULL). > >>> > >>> git am from https://lore.kernel.org/all/20191119113457.57833-3-hverkuil-cisco@xxxxxxxxx/ > >>> - Fixed some merge conflits. > >>> - Fixed the build error in drivers/media/platform/qcom/venus. > >>> > >>> .../media/v4l/vidioc-g-ext-ctrls.rst | 9 +- > >>> .../media/videodev2.h.rst.exceptions | 2 + > >>> drivers/media/i2c/imx214.c | 5 +- > >>> .../media/platform/qcom/venus/venc_ctrls.c | 9 +- > >>> drivers/media/v4l2-core/v4l2-ctrls-api.c | 57 +++++-- > >>> drivers/media/v4l2-core/v4l2-ctrls-core.c | 151 ++++++++++++++++-- > >>> drivers/media/v4l2-core/v4l2-ioctl.c | 4 +- > >>> include/media/v4l2-ctrls.h | 34 +++- > >>> include/uapi/linux/videodev2.h | 2 + > >>> 9 files changed, 243 insertions(+), 30 deletions(-) > >>> > >>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > >>> index 927ef397f1ce..928328b604c2 100644 > >>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > >>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > >>> @@ -304,14 +304,19 @@ still cause this situation. > >>> - Which value of the control to get/set/try. > >>> * - :cspan:`2` ``V4L2_CTRL_WHICH_CUR_VAL`` will return the current value of > >>> the control, ``V4L2_CTRL_WHICH_DEF_VAL`` will return the default > >>> + value of the control, ``V4L2_CTRL_WHICH_MIN_VAL`` will return the minimum > >>> + value of the control, ``V4L2_CTRL_WHICH_MAX_VAL`` will return the maximum > >>> value of the control and ``V4L2_CTRL_WHICH_REQUEST_VAL`` indicates that > >>> these controls have to be retrieved from a request or tried/set for > >>> a request. In the latter case the ``request_fd`` field contains the > >>> file descriptor of the request that should be used. If the device > >>> does not support requests, then ``EACCES`` will be returned. > >>> > >>> - When using ``V4L2_CTRL_WHICH_DEF_VAL`` be aware that you can only > >>> - get the default value of the control, you cannot set or try it. > >>> + When using ``V4L2_CTRL_WHICH_DEF_VAL``, ``V4L2_CTRL_WHICH_MIN_VAL`` > >>> + or ``V4L2_CTRL_WHICH_MAX_VAL`` be aware that you can only get the > >>> + default/minimum/maximum value of the control, you cannot set or try it. > >>> + The definition of minimum/maximum values for compound types are provided by > >>> + the control documentation. > >> > >> It is probably a good idea to mention that if the compound type does not > >> document the meaning of minimum/maximum, then it is not supported and querying > >> MIN/MAX will result in -ENODATA (as I suggested in my v2 review). > >> > >> Which compound controls DO support this? I do not see any documentation of min/max > >> in this series. > >> > > > > V4L2_CID_UVC_REGION_OF_INTEREST_RECT supports this. The documentation is > > in patch 10/10: > > > > + Use ``V4L2_CTRL_WHICH_MIN_VAL`` and ``V4L2_CTRL_WHICH_MAX_VAL`` to query > > + the range of rectangle sizes. The left/top coordinates of a minimum or > > + maximum rectangle are always 0. For example, a device can have a minimum > > + ROI rectangle of 1x1@0x0 and a maximum of 640x480@0x0. > > Hmm. The default min/max implementation in v4l2-ctrls*.c is for control types, > i.e. V4L2_CTRL_TYPE_RECT in this case. > > So should this be documented with V4L2_CTRL_TYPE_RECT rather than controls of that > type? > > I believe this belongs to the V4L2_CTRL_TYPE_RECT documentation, with perhaps some > additional comments with the specific controls. > > So for this type I would say that MIN_VAL and MAX_VAL can both return non-0 left > and top values, as long as they are the same. That would make sense in case there > are margins that can't be used for whatever the control uses the rectangle for. > > The V4L2_CID_UVC_REGION_OF_INTEREST_RECT documentation can be more specific and > require that left/top are 0. > Okay, that makes sense. Will add documentation for V4L2_CTRL_TYPE_RECT as well. > > > >>> > >>> For backwards compatibility you can also use a control class here > >>> (see :ref:`ctrl-class`). In that case all controls have to > >>> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > >>> index 828cca8e2daa..6295b0fa5716 100644 > >>> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions > >>> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > >>> @@ -563,6 +563,8 @@ ignore define V4L2_CTRL_DRIVER_PRIV > >>> ignore define V4L2_CTRL_MAX_DIMS > >>> ignore define V4L2_CTRL_WHICH_CUR_VAL > >>> ignore define V4L2_CTRL_WHICH_DEF_VAL > >>> +ignore define V4L2_CTRL_WHICH_MIN_VAL > >>> +ignore define V4L2_CTRL_WHICH_MAX_VAL > >>> ignore define V4L2_CTRL_WHICH_REQUEST_VAL > >>> ignore define V4L2_OUT_CAP_CUSTOM_TIMINGS > >>> ignore define V4L2_CID_MAX_CTRLS > >>> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c > >>> index 83c1737abeec..aa0bfd18f7b7 100644 > >>> --- a/drivers/media/i2c/imx214.c > >>> +++ b/drivers/media/i2c/imx214.c > >>> @@ -1037,7 +1037,10 @@ static int imx214_probe(struct i2c_client *client) > >>> imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls, > >>> NULL, > >>> V4L2_CID_UNIT_CELL_SIZE, > >>> - v4l2_ctrl_ptr_create((void *)&unit_size)); > >>> + v4l2_ctrl_ptr_create((void *)&unit_size), > >>> + v4l2_ctrl_ptr_create(NULL), > >>> + v4l2_ctrl_ptr_create(NULL)); > >>> + > >>> ret = imx214->ctrls.error; > >>> if (ret) { > >>> dev_err(&client->dev, "%s control init failed (%d)\n", > >>> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c > >>> index 7468e43800a9..28eca8f9d148 100644 > >>> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c > >>> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c > >>> @@ -607,11 +607,16 @@ int venc_ctrl_init(struct venus_inst *inst) > >>> > >>> v4l2_ctrl_new_std_compound(&inst->ctrl_handler, &venc_ctrl_ops, > >>> V4L2_CID_COLORIMETRY_HDR10_CLL_INFO, > >>> - v4l2_ctrl_ptr_create(&p_hdr10_cll)); > >>> + v4l2_ctrl_ptr_create(&p_hdr10_cll), > >>> + v4l2_ctrl_ptr_create(NULL), > >>> + v4l2_ctrl_ptr_create(NULL)); > >>> > >>> v4l2_ctrl_new_std_compound(&inst->ctrl_handler, &venc_ctrl_ops, > >>> V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY, > >>> - v4l2_ctrl_ptr_create((void *)&p_hdr10_mastering)); > >>> + v4l2_ctrl_ptr_create((void *)&p_hdr10_mastering), > >>> + v4l2_ctrl_ptr_create(NULL), > >>> + v4l2_ctrl_ptr_create(NULL)); > >>> + > >>> > >>> v4l2_ctrl_new_std_menu(&inst->ctrl_handler, &venc_ctrl_ops, > >>> V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD_TYPE, > >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c > >>> index d0a3aa3806fb..3bfab11fe148 100644 > >>> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c > >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c > >>> @@ -94,6 +94,34 @@ static int def_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl) > >>> return ptr_to_user(c, ctrl, ctrl->p_new); > >>> } > >>> > >>> +/* Helper function: copy the minimum control value back to the caller */ > >>> +static int min_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl) > >>> +{ > >>> + int idx, ret; > >>> + > >>> + for (idx = 0; idx < ctrl->elems; idx++) { > >>> + ret = ctrl->type_ops->minimum(ctrl, idx, ctrl->p_new); > >>> + if (ret) > >>> + return ret; > >>> + } > >>> + > >>> + return ptr_to_user(c, ctrl, ctrl->p_new); > >>> +} > >>> + > >>> +/* Helper function: copy the maximum control value back to the caller */ > >>> +static int max_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl) > >>> +{ > >>> + int idx, ret; > >>> + > >>> + for (idx = 0; idx < ctrl->elems; idx++) { > >>> + ret = ctrl->type_ops->maximum(ctrl, idx, ctrl->p_new); > >>> + if (ret) > >>> + return ret; > >>> + } > >>> + > >>> + return ptr_to_user(c, ctrl, ctrl->p_new); > >>> +} > >> > >> This needs to change: the minimum/maximum ops should work just like the init op: > >> they fill the whole array, not just one element. > >> > >>> + > >>> /* Helper function: copy the caller-provider value as the new control value */ > >>> static int user_to_new(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl) > >>> { > >>> @@ -228,8 +256,8 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl, > >>> cs->error_idx = i; > >>> > >>> if (cs->which && > >>> - cs->which != V4L2_CTRL_WHICH_DEF_VAL && > >>> - cs->which != V4L2_CTRL_WHICH_REQUEST_VAL && > >>> + (cs->which < V4L2_CTRL_WHICH_DEF_VAL || > >>> + cs->which > V4L2_CTRL_WHICH_MAX_VAL) && > >>> V4L2_CTRL_ID2WHICH(id) != cs->which) { > >>> dprintk(vdev, > >>> "invalid which 0x%x or control id 0x%x\n", > >>> @@ -367,8 +395,8 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl, > >>> */ > >>> static int class_check(struct v4l2_ctrl_handler *hdl, u32 which) > >>> { > >>> - if (which == 0 || which == V4L2_CTRL_WHICH_DEF_VAL || > >>> - which == V4L2_CTRL_WHICH_REQUEST_VAL) > >>> + if (which == 0 || (which >= V4L2_CTRL_WHICH_DEF_VAL && > >>> + which <= V4L2_CTRL_WHICH_MAX_VAL)) > >>> return 0; > >>> return find_ref_lock(hdl, which | 1) ? 0 : -EINVAL; > >>> } > >>> @@ -388,10 +416,12 @@ int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl, > >>> struct v4l2_ctrl_helper *helpers = helper; > >>> int ret; > >>> int i, j; > >>> - bool is_default, is_request; > >>> + bool is_default, is_request, is_min, is_max; > >>> > >>> is_default = (cs->which == V4L2_CTRL_WHICH_DEF_VAL); > >>> is_request = (cs->which == V4L2_CTRL_WHICH_REQUEST_VAL); > >>> + is_min = (cs->which == V4L2_CTRL_WHICH_MIN_VAL); > >>> + is_max = (cs->which == V4L2_CTRL_WHICH_MAX_VAL); > >>> > >>> cs->error_idx = cs->count; > >>> cs->which = V4L2_CTRL_ID2WHICH(cs->which); > >>> @@ -431,13 +461,14 @@ int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl, > >>> > >>> /* > >>> * g_volatile_ctrl will update the new control values. > >>> - * This makes no sense for V4L2_CTRL_WHICH_DEF_VAL and > >>> + * This makes no sense for V4L2_CTRL_WHICH_DEF_VAL, > >>> + * V4L2_CTRL_WHICH_MIN_VAL, V4L2_CTRL_WHICH_MAX_VAL and > >>> * V4L2_CTRL_WHICH_REQUEST_VAL. In the case of requests > >>> * it is v4l2_ctrl_request_complete() that copies the > >>> * volatile controls at the time of request completion > >>> * to the request, so you don't want to do that again. > >>> */ > >>> - if (!is_default && !is_request && > >>> + if (!is_default && !is_request && !is_min && !is_max && > >>> ((master->flags & V4L2_CTRL_FLAG_VOLATILE) || > >>> (master->has_volatiles && !is_cur_manual(master)))) { > >>> for (j = 0; j < master->ncontrols; j++) > >>> @@ -466,6 +497,10 @@ int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl, > >>> ret = -ENOMEM; > >>> else if (is_request && ref->p_req_valid) > >>> ret = req_to_user(cs->controls + idx, ref); > >>> + else if (is_min) > >>> + ret = min_to_user(cs->controls + idx, ref->ctrl); > >>> + else if (is_max) > >>> + ret = max_to_user(cs->controls + idx, ref->ctrl); > >>> else if (is_volatile) > >>> ret = new_to_user(cs->controls + idx, ref->ctrl); > >>> else > >>> @@ -563,9 +598,11 @@ int try_set_ext_ctrls_common(struct v4l2_fh *fh, > >>> > >>> cs->error_idx = cs->count; > >>> > >>> - /* Default value cannot be changed */ > >>> - if (cs->which == V4L2_CTRL_WHICH_DEF_VAL) { > >>> - dprintk(vdev, "%s: cannot change default value\n", > >>> + /* Default/minimum/maximum values cannot be changed */ > >>> + if (cs->which == V4L2_CTRL_WHICH_DEF_VAL || > >>> + cs->which == V4L2_CTRL_WHICH_MIN_VAL || > >>> + cs->which == V4L2_CTRL_WHICH_MAX_VAL) { > >>> + dprintk(vdev, "%s: cannot change default/min/max value\n", > >>> video_device_node_name(vdev)); > >>> return -EINVAL; > >>> } > >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c > >>> index 0ac36ebc45dd..e43d5369f6af 100644 > >>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c > >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c > >>> @@ -177,6 +177,30 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx, > >>> } > >>> } > >>> > >>> +static int std_min_compound(const struct v4l2_ctrl *ctrl, u32 idx, > >>> + union v4l2_ctrl_ptr ptr) > >>> +{ > >>> + void *p = ptr.p + idx * ctrl->elem_size; > >>> + > >>> + if (!ctrl->p_min.p_const) > >>> + return -EINVAL; > >>> + > >>> + memcpy(p, ctrl->p_min.p_const, ctrl->elem_size); > >>> + return 0; > >>> +} > >>> + > >>> +static int std_max_compound(const struct v4l2_ctrl *ctrl, u32 idx, > >>> + union v4l2_ctrl_ptr ptr) > >>> +{ > >>> + void *p = ptr.p + idx * ctrl->elem_size; > >>> + > >>> + if (!ctrl->p_max.p_const) > >>> + return -EINVAL; > >>> + > >>> + memcpy(p, ctrl->p_max.p_const, ctrl->elem_size); > >>> + return 0; > >>> +} > >>> + > >>> void v4l2_ctrl_type_op_init(const struct v4l2_ctrl *ctrl, u32 from_idx, > >>> union v4l2_ctrl_ptr ptr) > >>> { > >>> @@ -247,6 +271,96 @@ void v4l2_ctrl_type_op_init(const struct v4l2_ctrl *ctrl, u32 from_idx, > >>> } > >>> EXPORT_SYMBOL(v4l2_ctrl_type_op_init); > >>> > >>> +static int v4l2_ctrl_type_op_minimum(const struct v4l2_ctrl *ctrl, u32 idx, > >>> + union v4l2_ctrl_ptr ptr) > >>> +{ > >>> + int ret; > >>> + > >>> + switch (ctrl->type) { > >>> + case V4L2_CTRL_TYPE_STRING: > >>> + idx *= ctrl->elem_size; > >>> + memset(ptr.p_char + idx, ' ', ctrl->minimum); > >>> + ptr.p_char[idx + ctrl->minimum] = '\0'; > >>> + break; > >>> + case V4L2_CTRL_TYPE_INTEGER64: > >>> + ptr.p_s64[idx] = ctrl->minimum; > >>> + break; > >>> + case V4L2_CTRL_TYPE_INTEGER: > >>> + case V4L2_CTRL_TYPE_INTEGER_MENU: > >>> + case V4L2_CTRL_TYPE_MENU: > >>> + case V4L2_CTRL_TYPE_BITMASK: > >>> + case V4L2_CTRL_TYPE_BOOLEAN: > >>> + ptr.p_s32[idx] = ctrl->minimum; > >>> + break; > >>> + case V4L2_CTRL_TYPE_BUTTON: > >>> + case V4L2_CTRL_TYPE_CTRL_CLASS: > >>> + ptr.p_s32[idx] = 0; > >>> + break; > >>> + case V4L2_CTRL_TYPE_U8: > >>> + ptr.p_u8[idx] = ctrl->minimum; > >>> + break; > >>> + case V4L2_CTRL_TYPE_U16: > >>> + ptr.p_u16[idx] = ctrl->minimum; > >>> + break; > >>> + case V4L2_CTRL_TYPE_U32: > >>> + ptr.p_u32[idx] = ctrl->minimum; > >>> + break; > >>> + default: > >>> + ret = std_min_compound(ctrl, idx, ptr); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + break; > >>> + } > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int v4l2_ctrl_type_op_maximum(const struct v4l2_ctrl *ctrl, u32 idx, > >>> + union v4l2_ctrl_ptr ptr) > >> > >> The v4l2_ctrl_type_op_init, v4l2_ctrl_type_op_minimum and v4l2_ctrl_type_op_maximum > >> functions are all awfully similar. The only difference is ctrl->default_value/minimum/maximum. > >> > >> So perhaps this can be changed to a common function that has this as an argument. > >> > >> Note that these minimum/maximum ops should look like the init op: they need to > >> fill all elements of the control array, not just one element. > >> > > > > Thanks. What about adding a > > static int __v4l2_ctrl_type_op_init(const struct v4l2_ctrl *ctrl, u32 from_idx, > > u32 which, union v4l2_ctrl_ptr ptr) > > and call it from v4l2_ctrl_type_op_init, v4l2_ctrl_type_op_minimum and > > v4l2_ctrl_type_op_maximum? > > Let's see how that looks. > Sounds good. Will try this and apply your other comments in the next version. Best, Yunke > Regards, > > Hans > > > > > Best, > > Yunke > > > >>> +{ > >>> + int ret; > >>> + > >>> + switch (ctrl->type) { > >>> + case V4L2_CTRL_TYPE_STRING: > >>> + idx *= ctrl->elem_size; > >>> + memset(ptr.p_char + idx, ' ', ctrl->maximum); > >>> + ptr.p_char[idx + ctrl->maximum] = '\0'; > >>> + break; > >>> + case V4L2_CTRL_TYPE_INTEGER64: > >>> + ptr.p_s64[idx] = ctrl->maximum; > >>> + break; > >>> + case V4L2_CTRL_TYPE_INTEGER: > >>> + case V4L2_CTRL_TYPE_INTEGER_MENU: > >>> + case V4L2_CTRL_TYPE_MENU: > >>> + case V4L2_CTRL_TYPE_BITMASK: > >>> + case V4L2_CTRL_TYPE_BOOLEAN: > >>> + ptr.p_s32[idx] = ctrl->maximum; > >>> + break; > >>> + case V4L2_CTRL_TYPE_BUTTON: > >>> + case V4L2_CTRL_TYPE_CTRL_CLASS: > >>> + ptr.p_s32[idx] = 0; > >>> + break; > >>> + case V4L2_CTRL_TYPE_U8: > >>> + ptr.p_u8[idx] = ctrl->maximum; > >>> + break; > >>> + case V4L2_CTRL_TYPE_U16: > >>> + ptr.p_u16[idx] = ctrl->maximum; > >>> + break; > >>> + case V4L2_CTRL_TYPE_U32: > >>> + ptr.p_u32[idx] = ctrl->maximum; > >>> + break; > >>> + default: > >>> + ret = std_max_compound(ctrl, idx, ptr); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + break; > >>> + } > >>> + > >>> + return 0; > >>> +} > >>> + > >>> void v4l2_ctrl_type_op_log(const struct v4l2_ctrl *ctrl) > >>> { > >>> union v4l2_ctrl_ptr ptr = ctrl->p_cur; > >>> @@ -1043,6 +1157,8 @@ EXPORT_SYMBOL(v4l2_ctrl_type_op_validate); > >>> static const struct v4l2_ctrl_type_ops std_type_ops = { > >>> .equal = v4l2_ctrl_type_op_equal, > >>> .init = v4l2_ctrl_type_op_init, > >>> + .minimum = v4l2_ctrl_type_op_minimum, > >>> + .maximum = v4l2_ctrl_type_op_maximum, > >>> .log = v4l2_ctrl_type_op_log, > >>> .validate = v4l2_ctrl_type_op_validate, > >>> }; > >>> @@ -1514,7 +1630,10 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, > >>> s64 min, s64 max, u64 step, s64 def, > >>> const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size, > >>> u32 flags, const char * const *qmenu, > >>> - const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def, > >>> + const s64 *qmenu_int, > >>> + const union v4l2_ctrl_ptr p_def, > >>> + const union v4l2_ctrl_ptr p_min, > >>> + const union v4l2_ctrl_ptr p_max, > >>> void *priv) > >>> { > >>> struct v4l2_ctrl *ctrl; > >>> @@ -1674,7 +1793,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, > >>> sz_extra += 2 * tot_ctrl_size; > >>> > >> > >> I would like to have a sanity check: if p_def.p_const == NULL, then > >> p_min and p_max are NULL as well. > >> > >> Also, check that either both p_min and p_max are NULL, or both are not NULL. > >> > >> If either condition is false, then return -EINVAL. > >> > >>> if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p_const) > >>> - sz_extra += elem_size; > >>> + sz_extra += elem_size * 3; > >> > >> I think this can be done a bit smarter: > >> > >> if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p_const) > >> sz_extra += elem_size; > >> if (type >= V4L2_CTRL_COMPOUND_TYPES && p_min.p_const) > >> sz_extra += elem_size * 2; > >> > >> This assumes that the sanity checks mentioned above are done first. > >> > >>> > >>> ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL); > >>> if (ctrl == NULL) { > >>> @@ -1739,6 +1858,13 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, > >>> ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size; > >>> memcpy(ctrl->p_def.p, p_def.p_const, elem_size); > >>> } > >>> + if (type >= V4L2_CTRL_COMPOUND_TYPES && > >>> + p_min.p_const && p_max.p_const) { > >>> + ctrl->p_min.p = ctrl->p_cur.p + 2 * tot_ctrl_size; > >> > >> This is wrong, p_min has size elem_size, not tot_ctrl_size. > >> > >> I'd write this as: > >> > >> ctrl->p_min.p = ctrl->p_def.p + elem_size; > >> memcpy(ctrl->p_min.p, p_min.p_const, elem_size); > >> ctrl->p_max.p = ctrl->p_min.p + elem_size; > >> memcpy(ctrl->p_max.p, p_max.p_const, elem_size); > >> > >>> + memcpy(ctrl->p_min.p, p_min.p_const, elem_size); > >>> + ctrl->p_max.p = ctrl->p_cur.p + 3 * tot_ctrl_size; > >>> + memcpy(ctrl->p_max.p, p_max.p_const, elem_size); > >>> + } > >>> > >>> ctrl->type_ops->init(ctrl, 0, ctrl->p_cur); > >>> cur_to_new(ctrl); > >>> @@ -1790,7 +1916,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl, > >>> type, min, max, > >>> is_menu ? cfg->menu_skip_mask : step, def, > >>> cfg->dims, cfg->elem_size, > >>> - flags, qmenu, qmenu_int, cfg->p_def, priv); > >>> + flags, qmenu, qmenu_int, cfg->p_def, cfg->p_min, > >>> + cfg->p_max, priv); > >>> if (ctrl) > >>> ctrl->is_private = cfg->is_private; > >>> return ctrl; > >>> @@ -1815,7 +1942,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl, > >>> } > >>> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > >>> min, max, step, def, NULL, 0, > >>> - flags, NULL, NULL, ptr_null, NULL); > >>> + flags, NULL, NULL, ptr_null, ptr_null, > >>> + ptr_null, NULL); > >>> } > >>> EXPORT_SYMBOL(v4l2_ctrl_new_std); > >>> > >>> @@ -1848,7 +1976,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl, > >>> } > >>> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > >>> 0, max, mask, def, NULL, 0, > >>> - flags, qmenu, qmenu_int, ptr_null, NULL); > >>> + flags, qmenu, qmenu_int, ptr_null, ptr_null, > >>> + ptr_null, NULL); > >>> } > >>> EXPORT_SYMBOL(v4l2_ctrl_new_std_menu); > >>> > >>> @@ -1880,7 +2009,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, > >>> } > >>> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > >>> 0, max, mask, def, NULL, 0, > >>> - flags, qmenu, NULL, ptr_null, NULL); > >>> + flags, qmenu, NULL, ptr_null, ptr_null, > >>> + ptr_null, NULL); > >>> > >>> } > >>> EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items); > >>> @@ -1888,7 +2018,9 @@ EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items); > >>> /* Helper function for standard compound controls */ > >>> struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, > >>> const struct v4l2_ctrl_ops *ops, u32 id, > >>> - const union v4l2_ctrl_ptr p_def) > >>> + const union v4l2_ctrl_ptr p_def, > >>> + const union v4l2_ctrl_ptr p_min, > >>> + const union v4l2_ctrl_ptr p_max) > >>> { > >>> const char *name; > >>> enum v4l2_ctrl_type type; > >>> @@ -1902,7 +2034,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, > >>> } > >>> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > >>> min, max, step, def, NULL, 0, > >>> - flags, NULL, NULL, p_def, NULL); > >>> + flags, NULL, NULL, p_def, p_min, p_max, NULL); > >>> } > >>> EXPORT_SYMBOL(v4l2_ctrl_new_std_compound); > >>> > >>> @@ -1926,7 +2058,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, > >>> } > >>> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > >>> 0, max, 0, def, NULL, 0, > >>> - flags, NULL, qmenu_int, ptr_null, NULL); > >>> + flags, NULL, qmenu_int, ptr_null, ptr_null, > >>> + ptr_null, NULL); > >>> } > >>> EXPORT_SYMBOL(v4l2_ctrl_new_int_menu); > >>> > >>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > >>> index fddba75d9074..5d49e6d71b64 100644 > >>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c > >>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > >>> @@ -893,7 +893,9 @@ static bool check_ext_ctrls(struct v4l2_ext_controls *c, unsigned long ioctl) > >>> return false; > >>> break; > >>> case V4L2_CTRL_WHICH_DEF_VAL: > >>> - /* Default value cannot be changed */ > >>> + case V4L2_CTRL_WHICH_MIN_VAL: > >>> + case V4L2_CTRL_WHICH_MAX_VAL: > >>> + /* Default, minimum or maximum value cannot be changed */ > >>> if (ioctl == VIDIOC_S_EXT_CTRLS || > >>> ioctl == VIDIOC_TRY_EXT_CTRLS) { > >>> c->error_idx = c->count; > >>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h > >>> index 1846caf9dd53..555b561231ef 100644 > >>> --- a/include/media/v4l2-ctrls.h > >>> +++ b/include/media/v4l2-ctrls.h > >>> @@ -125,6 +125,8 @@ struct v4l2_ctrl_ops { > >>> * > >>> * @equal: return true if all ctrl->elems array elements are equal. > >>> * @init: initialize the value for array elements from from_idx to ctrl->elems. > >>> + * @minimum: set the value to the minimum value of the control. > >>> + * @maximum: set the value to the maximum value of the control. > >>> * @log: log the value. > >>> * @validate: validate the value for ctrl->new_elems array elements. > >>> * Return 0 on success and a negative value otherwise. > >>> @@ -134,6 +136,10 @@ struct v4l2_ctrl_type_ops { > >>> union v4l2_ctrl_ptr ptr1, union v4l2_ctrl_ptr ptr2); > >>> void (*init)(const struct v4l2_ctrl *ctrl, u32 from_idx, > >>> union v4l2_ctrl_ptr ptr); > >>> + int (*minimum)(const struct v4l2_ctrl *ctrl, u32 idx, > >>> + union v4l2_ctrl_ptr ptr); > >>> + int (*maximum)(const struct v4l2_ctrl *ctrl, u32 idx, > >>> + union v4l2_ctrl_ptr ptr); > >>> void (*log)(const struct v4l2_ctrl *ctrl); > >>> int (*validate)(const struct v4l2_ctrl *ctrl, union v4l2_ctrl_ptr ptr); > >>> }; > >>> @@ -239,6 +245,12 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv); > >>> * @p_def: The control's default value represented via a union which > >>> * provides a standard way of accessing control types > >>> * through a pointer (for compound controls only). > >>> + * @p_min: The control's minimum value represented via a union which > >>> + * provides a standard way of accessing control types > >>> + * through a pointer (for compound controls only). > >>> + * @p_max: The control's maximum value represented via a union which > >>> + * provides a standard way of accessing control types > >>> + * through a pointer (for compound controls only). > >>> * @p_cur: The control's current value represented via a union which > >>> * provides a standard way of accessing control types > >>> * through a pointer. > >>> @@ -298,6 +310,8 @@ struct v4l2_ctrl { > >>> } cur; > >>> > >>> union v4l2_ctrl_ptr p_def; > >>> + union v4l2_ctrl_ptr p_min; > >>> + union v4l2_ctrl_ptr p_max; > >>> union v4l2_ctrl_ptr p_new; > >>> union v4l2_ctrl_ptr p_cur; > >>> }; > >>> @@ -417,6 +431,8 @@ struct v4l2_ctrl_handler { > >>> * @step: The control's step value for non-menu controls. > >>> * @def: The control's default value. > >>> * @p_def: The control's default value for compound controls. > >>> + * @p_min: The control's minimum value for compound controls. > >>> + * @p_max: The control's maximum value for compound controls. > >>> * @dims: The size of each dimension. > >>> * @elem_size: The size in bytes of the control. > >>> * @flags: The control's flags. > >>> @@ -446,6 +462,8 @@ struct v4l2_ctrl_config { > >>> u64 step; > >>> s64 def; > >>> union v4l2_ctrl_ptr p_def; > >>> + union v4l2_ctrl_ptr p_min; > >>> + union v4l2_ctrl_ptr p_max; > >>> u32 dims[V4L2_CTRL_MAX_DIMS]; > >>> u32 elem_size; > >>> u32 flags; > >>> @@ -715,17 +733,23 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, > >>> * @ops: The control ops. > >>> * @id: The control ID. > >>> * @p_def: The control's default value. > >>> + * @p_min: The control's minimum value. > >>> + * @p_max: The control's maximum value. > >>> * > >>> - * Sames as v4l2_ctrl_new_std(), but with support to compound controls, thanks > >>> - * to the @p_def field. Use v4l2_ctrl_ptr_create() to create @p_def from a > >>> - * pointer. Use v4l2_ctrl_ptr_create(NULL) if the default value of the > >>> - * compound control should be all zeroes. > >>> + * Same as v4l2_ctrl_new_std(), but with support to compound controls, thanks > >>> + * to the @p_def/min/max field. Use v4l2_ctrl_ptr_create() to create > >>> + * @p_def/min/max from a pointer. Use v4l2_ctrl_ptr_create(NULL) if the > >>> + * default value of the compound control should be all zeroes. Use > >>> + * v4l2_ctrl_ptr_create(NULL) if the min/max value of the compound control > >>> + * is not defined, an error will be returned in this case. > >>> * > >>> */ > >>> struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, > >>> const struct v4l2_ctrl_ops *ops, > >>> u32 id, > >>> - const union v4l2_ctrl_ptr p_def); > >>> + const union v4l2_ctrl_ptr p_def, > >>> + const union v4l2_ctrl_ptr p_min, > >>> + const union v4l2_ctrl_ptr p_max); > >>> > >>> /** > >>> * v4l2_ctrl_new_int_menu() - Create a new standard V4L2 integer menu control. > >>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > >>> index a81951c51325..5f698b49c02e 100644 > >>> --- a/include/uapi/linux/videodev2.h > >>> +++ b/include/uapi/linux/videodev2.h > >>> @@ -1827,6 +1827,8 @@ struct v4l2_ext_controls { > >>> #define V4L2_CTRL_WHICH_CUR_VAL 0 > >>> #define V4L2_CTRL_WHICH_DEF_VAL 0x0f000000 > >>> #define V4L2_CTRL_WHICH_REQUEST_VAL 0x0f010000 > >>> +#define V4L2_CTRL_WHICH_MIN_VAL 0x0f020000 > >>> +#define V4L2_CTRL_WHICH_MAX_VAL 0x0f030000 > >>> > >>> enum v4l2_ctrl_type { > >>> V4L2_CTRL_TYPE_INTEGER = 1, > >> > >> Regards, > >> > >> Hans
