| Message ID | 20220628120523.2915913-1-hverkuil-cisco@xs4all.nl |
|---|---|
| Headers | show |
| Series | Add dynamic arrays and v4l2_ctrl_modify_dimensions | expand |
Hi Hans, Thank you for the patch. On Tue, Jun 28, 2022 at 02:05:18PM +0200, Hans Verkuil wrote: > Add a dynamic array test control to help test support for this > feature. > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Acked-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > Tested-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/media/test-drivers/vivid/vivid-ctrls.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/media/test-drivers/vivid/vivid-ctrls.c b/drivers/media/test-drivers/vivid/vivid-ctrls.c > index 7ff8fdfda28e..a78d676575bc 100644 > --- a/drivers/media/test-drivers/vivid/vivid-ctrls.c > +++ b/drivers/media/test-drivers/vivid/vivid-ctrls.c > @@ -34,6 +34,7 @@ > #define VIVID_CID_U8_4D_ARRAY (VIVID_CID_CUSTOM_BASE + 10) > #define VIVID_CID_AREA (VIVID_CID_CUSTOM_BASE + 11) > #define VIVID_CID_RO_INTEGER (VIVID_CID_CUSTOM_BASE + 12) > +#define VIVID_CID_U32_DYN_ARRAY (VIVID_CID_CUSTOM_BASE + 13) > > #define VIVID_CID_VIVID_BASE (0x00f00000 | 0xf000) > #define VIVID_CID_VIVID_CLASS (0x00f00000 | 1) > @@ -190,6 +191,19 @@ static const struct v4l2_ctrl_config vivid_ctrl_u32_array = { > .dims = { 1 }, > }; > > +static const struct v4l2_ctrl_config vivid_ctrl_u32_dyn_array = { > + .ops = &vivid_user_gen_ctrl_ops, > + .id = VIVID_CID_U32_DYN_ARRAY, > + .name = "U32 Dynamic Array", > + .type = V4L2_CTRL_TYPE_U32, > + .flags = V4L2_CTRL_FLAG_DYNAMIC_ARRAY, > + .def = 50, > + .min = 10, > + .max = 90, > + .step = 1, > + .dims = { 100 }, > +}; > + > static const struct v4l2_ctrl_config vivid_ctrl_u16_matrix = { > .ops = &vivid_user_gen_ctrl_ops, > .id = VIVID_CID_U16_MATRIX, > @@ -1625,6 +1639,7 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap, > dev->ro_int32 = v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_ro_int32, NULL); > v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_area, NULL); > v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_array, NULL); > + v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_dyn_array, NULL); > v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u16_matrix, NULL); > v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u8_4d_array, NULL); >
Hi Hans, Thank you for the patch. On Tue, Jun 28, 2022 at 02:05:20PM +0200, Hans Verkuil wrote: > Also allocate space for arrays in struct ctrl_ref. > > This is in preparation for allowing to change the array size from > a driver. > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-ctrls-api.c | 2 +- > drivers/media/v4l2-core/v4l2-ctrls-core.c | 31 ++++++++++++++--------- > include/media/v4l2-ctrls.h | 16 ++++++------ > 3 files changed, 28 insertions(+), 21 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c > index 1b90bd7c4010..6f1b72c59e8e 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c > @@ -467,7 +467,7 @@ int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl, > > if (is_default) > ret = def_to_user(cs->controls + idx, ref->ctrl); > - else if (is_request && ref->p_req_dyn_enomem) > + else if (is_request && ref->p_req_array_enomem) > ret = -ENOMEM; > else if (is_request && ref->p_req_valid) > ret = req_to_user(cs->controls + idx, ref); > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c > index 1372b7b45681..38030a7cb233 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c > @@ -1048,23 +1048,26 @@ void cur_to_new(struct v4l2_ctrl *ctrl) > ptr_to_ptr(ctrl, ctrl->p_cur, ctrl->p_new, ctrl->new_elems); > } > > -static bool req_alloc_dyn_array(struct v4l2_ctrl_ref *ref, u32 elems) > +static bool req_alloc_array(struct v4l2_ctrl_ref *ref, u32 elems) > { > void *tmp; > > - if (elems < ref->p_req_dyn_alloc_elems) > + if (elems == ref->p_req_array_alloc_elems) > + return true; > + if (ref->ctrl->is_dyn_array && > + elems < ref->p_req_array_alloc_elems) > return true; > > tmp = kvmalloc(elems * ref->ctrl->elem_size, GFP_KERNEL); > > if (!tmp) { > - ref->p_req_dyn_enomem = true; > + ref->p_req_array_enomem = true; > return false; > } > - ref->p_req_dyn_enomem = false; > + ref->p_req_array_enomem = false; > kvfree(ref->p_req.p); > ref->p_req.p = tmp; > - ref->p_req_dyn_alloc_elems = elems; > + ref->p_req_array_alloc_elems = elems; > return true; > } > > @@ -1077,7 +1080,7 @@ void new_to_req(struct v4l2_ctrl_ref *ref) > return; > > ctrl = ref->ctrl; > - if (ctrl->is_dyn_array && !req_alloc_dyn_array(ref, ctrl->new_elems)) > + if (ctrl->is_array && !req_alloc_array(ref, ctrl->new_elems)) > return; > > ref->p_req_elems = ctrl->new_elems; > @@ -1094,7 +1097,7 @@ void cur_to_req(struct v4l2_ctrl_ref *ref) > return; > > ctrl = ref->ctrl; > - if (ctrl->is_dyn_array && !req_alloc_dyn_array(ref, ctrl->elems)) > + if (ctrl->is_array && !req_alloc_array(ref, ctrl->elems)) > return; > > ref->p_req_elems = ctrl->elems; > @@ -1123,14 +1126,18 @@ int req_to_new(struct v4l2_ctrl_ref *ref) > return 0; > } > > - /* Not a dynamic array, so just copy the request value */ > - if (!ctrl->is_dyn_array) { > + /* Not an array, so just copy the request value */ > + if (!ctrl->is_array) { > ptr_to_ptr(ctrl, ref->p_req, ctrl->p_new, ctrl->new_elems); > return 0; > } > > /* Sanity check, should never happen */ > - if (WARN_ON(!ref->p_req_dyn_alloc_elems)) > + if (WARN_ON(!ref->p_req_array_alloc_elems)) > + return -ENOMEM; > + > + if (!ctrl->is_dyn_array && > + ref->p_req_elems != ctrl->p_array_alloc_elems) > return -ENOMEM; > > /* > @@ -1243,7 +1250,7 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl) > /* Free all nodes */ > list_for_each_entry_safe(ref, next_ref, &hdl->ctrl_refs, node) { > list_del(&ref->node); > - if (ref->p_req_dyn_alloc_elems) > + if (ref->p_req_array_alloc_elems) > kvfree(ref->p_req.p); > kfree(ref); > } > @@ -1368,7 +1375,7 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl, > if (hdl->error) > return hdl->error; > > - if (allocate_req && !ctrl->is_dyn_array) > + if (allocate_req && !ctrl->is_array) > size_extra_req = ctrl->elems * ctrl->elem_size; > new_ref = kzalloc(sizeof(*new_ref) + size_extra_req, GFP_KERNEL); > if (!new_ref) > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h > index a2f147873265..e0f32e8b886a 100644 > --- a/include/media/v4l2-ctrls.h > +++ b/include/media/v4l2-ctrls.h > @@ -324,15 +324,15 @@ struct v4l2_ctrl { > * from a cluster with multiple controls twice (when the first > * control of a cluster is applied, they all are). > * @p_req_valid: If set, then p_req contains the control value for the request. > - * @p_req_dyn_enomem: If set, then p_req is invalid since allocating space for > - * a dynamic array failed. Attempting to read this value shall > - * result in ENOMEM. Only valid if ctrl->is_dyn_array is true. > - * @p_req_dyn_alloc_elems: The number of elements allocated for the dynamic > - * array. Only valid if @p_req_valid and ctrl->is_dyn_array are > + * @p_req_array_enomem: If set, then p_req is invalid since allocating space for > + * an array failed. Attempting to read this value shall > + * result in ENOMEM. Only valid if ctrl->is_array is true. > + * @p_req_array_alloc_elems: The number of elements allocated for the > + * array. Only valid if @p_req_valid and ctrl->is_array are > * true. > * @p_req_elems: The number of elements in @p_req. This is the same as > * ctrl->elems, except for dynamic arrays. In that case it is in > - * the range of 1 to @p_req_dyn_alloc_elems. Only valid if > + * the range of 1 to @p_req_array_alloc_elems. Only valid if > * @p_req_valid is true. > * @p_req: If the control handler containing this control reference > * is bound to a media request, then this points to the > @@ -354,8 +354,8 @@ struct v4l2_ctrl_ref { > bool from_other_dev; > bool req_done; > bool p_req_valid; > - bool p_req_dyn_enomem; > - u32 p_req_dyn_alloc_elems; > + bool p_req_array_enomem; > + u32 p_req_array_alloc_elems; > u32 p_req_elems; > union v4l2_ctrl_ptr p_req; > };
Hi Hans, Thank you for the patch. On Tue, Jun 28, 2022 at 02:05:21PM +0200, Hans Verkuil wrote: > Add a new function to modify the dimensions of an array control. > > This is typically used if the array size depends on e.g. the currently > selected video format. > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/v4l2-core/v4l2-ctrls-api.c | 34 ++++++++++++++++ > include/media/v4l2-ctrls.h | 49 ++++++++++++++++++++++++ > 2 files changed, 83 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c > index 6f1b72c59e8e..16be31966cb1 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c > @@ -989,6 +989,40 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl, > } > EXPORT_SYMBOL(__v4l2_ctrl_modify_range); > > +int __v4l2_ctrl_modify_dimensions(struct v4l2_ctrl *ctrl, > + u32 dims[V4L2_CTRL_MAX_DIMS]) > +{ > + unsigned int elems = dims[0]; > + unsigned int i; > + void *p_array; > + > + if (!ctrl->is_array || ctrl->is_dyn_array) > + return -EINVAL; > + > + for (i = 1; i < ctrl->nr_of_dims; i++) > + elems *= dims[i]; I would have initialized elems to 1 and started iterating at 0, to avoid splitting the computation in two places. > + if (elems == 0) > + return -EINVAL; > + p_array = kvzalloc(2 * elems * ctrl->elem_size, GFP_KERNEL); There are lots of places where we allocate memory for arrays, with the same computation, and the same pointer arithmetic to set p_cur.p. It would be nice to clean that up at some point and factorize the code out to a separate function. A helper function to calculate the total number of elements would also be helpful, and you could add overflow checks there. > + if (!p_array) > + return -ENOMEM; > + kvfree(ctrl->p_array); > + ctrl->p_array_alloc_elems = elems; > + ctrl->elems = elems; > + ctrl->new_elems = elems; > + ctrl->p_array = p_array; > + ctrl->p_new.p = p_array; > + ctrl->p_cur.p = p_array + elems * ctrl->elem_size; > + for (i = 0; i < ctrl->nr_of_dims; i++) > + ctrl->dims[i] = dims[i]; > + for (i = 0; i < elems; i++) { > + ctrl->type_ops->init(ctrl, i, ctrl->p_cur); > + ctrl->type_ops->init(ctrl, i, ctrl->p_new); Would it be cheaper to call init on only p_cur or p_new, and then memcpy to the other one ? > + } > + return 0; > +} > +EXPORT_SYMBOL(__v4l2_ctrl_modify_dimensions); > + > /* Implement VIDIOC_QUERY_EXT_CTRL */ > int v4l2_query_ext_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_query_ext_ctrl *qc) > { > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h > index e0f32e8b886a..3d039142f870 100644 > --- a/include/media/v4l2-ctrls.h > +++ b/include/media/v4l2-ctrls.h > @@ -963,6 +963,55 @@ static inline int v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl, > return rval; > } > > +/** > + *__v4l2_ctrl_modify_dimensions() - Unlocked variant of v4l2_ctrl_modify_dimensions() > + * > + * @ctrl: The control to update. > + * @dims: The control's new dimensions. > + * > + * Update the dimensions of an array control on the fly. > + * > + * An error is returned if @dims is invalid for this control. > + * > + * The caller is responsible for acquiring the control handler mutex on behalf > + * of __v4l2_ctrl_modify_dimensions(). It would be nice to add lockdep_assert() statements in the unlocked functions. > + * > + * Note: calling this function when the same control is used in pending requests > + * is untested. It should work (a request with the wrong size of the control > + * will drop that control silently), but it will be very confusing. > + */ > +int __v4l2_ctrl_modify_dimensions(struct v4l2_ctrl *ctrl, > + u32 dims[V4L2_CTRL_MAX_DIMS]); > + > +/** > + * v4l2_ctrl_modify_dimensions() - Update the dimensions of an array control. > + * > + * @ctrl: The control to update. > + * @dims: The control's new dimensions. > + * > + * Update the dimensions of a control on the fly. I'd add "The control value is reset to the default". Same for the previous function. Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > + * > + * An error is returned if @dims is invalid for this control type. > + * > + * This function assumes that the control handler is not locked and will > + * take the lock itself. > + * > + * Note: calling this function when the same control is used in pending requests > + * is untested. It should work (a request with the wrong size of the control > + * will drop that control silently), but it will be very confusing. > + */ > +static inline int v4l2_ctrl_modify_dimensions(struct v4l2_ctrl *ctrl, > + u32 dims[V4L2_CTRL_MAX_DIMS]) > +{ > + int rval; > + > + v4l2_ctrl_lock(ctrl); > + rval = __v4l2_ctrl_modify_dimensions(ctrl, dims); > + v4l2_ctrl_unlock(ctrl); > + > + return rval; > +} > + > /** > * v4l2_ctrl_notify() - Function to set a notify callback for a control. > *
Hi Hans, Thank you for the patch. On Tue, Jun 28, 2022 at 02:05:22PM +0200, Hans Verkuil wrote: > Add a new V4L2_EVENT_CTRL_CH_DIMENSIONS change flag that is issued > when the dimensions of an array change as a result of a > __v4l2_ctrl_modify_dimensions() call. > > This will inform userspace that there are new dimensions. While this is easy to add, I'm not sure it will actually be useful in real use cases. Should we delay adding this new API until someone actually needs it ? > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > Documentation/userspace-api/media/v4l/vidioc-dqevent.rst | 5 +++++ > Documentation/userspace-api/media/videodev2.h.rst.exceptions | 1 + > drivers/media/v4l2-core/v4l2-ctrls-api.c | 2 ++ > include/uapi/linux/videodev2.h | 1 + > 4 files changed, 9 insertions(+) > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst > index 6eb40073c906..8db103760930 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst > @@ -332,6 +332,11 @@ call. > - 0x0004 > - This control event was triggered because the minimum, maximum, > step or the default value of the control changed. > + * - ``V4L2_EVENT_CTRL_CH_DIMENSIONS`` > + - 0x0008 > + - This control event was triggered because the dimensions of the > + control changed. Note that the number of dimensions remains the > + same. > > > .. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.5cm}| > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > index 0b91200776f8..274474425b05 100644 > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > @@ -506,6 +506,7 @@ replace define V4L2_EVENT_PRIVATE_START event-type > replace define V4L2_EVENT_CTRL_CH_VALUE ctrl-changes-flags > replace define V4L2_EVENT_CTRL_CH_FLAGS ctrl-changes-flags > replace define V4L2_EVENT_CTRL_CH_RANGE ctrl-changes-flags > +replace define V4L2_EVENT_CTRL_CH_DIMENSIONS ctrl-changes-flags > > replace define V4L2_EVENT_SRC_CH_RESOLUTION src-changes-flags > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c > index 16be31966cb1..47f69de9a067 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c > @@ -1019,6 +1019,8 @@ int __v4l2_ctrl_modify_dimensions(struct v4l2_ctrl *ctrl, > ctrl->type_ops->init(ctrl, i, ctrl->p_cur); > ctrl->type_ops->init(ctrl, i, ctrl->p_new); > } > + send_event(NULL, ctrl, > + V4L2_EVENT_CTRL_CH_VALUE | V4L2_EVENT_CTRL_CH_DIMENSIONS); Sending V4L2_EVENT_CTRL_CH_VALUE propably belongs to the previous patch already. > return 0; > } > EXPORT_SYMBOL(__v4l2_ctrl_modify_dimensions); > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index 9018aa984db3..3971af623c56 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -2398,6 +2398,7 @@ struct v4l2_event_vsync { > #define V4L2_EVENT_CTRL_CH_VALUE (1 << 0) > #define V4L2_EVENT_CTRL_CH_FLAGS (1 << 1) > #define V4L2_EVENT_CTRL_CH_RANGE (1 << 2) > +#define V4L2_EVENT_CTRL_CH_DIMENSIONS (1 << 3) > > struct v4l2_event_ctrl { > __u32 changes;
Hi Hans, On Tue, Jun 28, 2022 at 02:05:15PM +0200, Hans Verkuil wrote: > This series adds support for dynamic array controls and for a > new v4l2_ctrl_modify_dimensions() function to modify the dimensions > of an array control by the driver. > > The dynamic array patches are unchanged and are already used in two > patch series (stateless HEVC uAPI and the dw100 driver), but they are > added here since the last 5 patches that add support for > v4l2_ctrl_modify_dimensions() build on those. > > The vivid driver is also extended with such controls to make it > possible to test this. > > Xavier, the v4l2_ctrl_modify_dimensions() are mostly identical to > the patches I mailed you before and that you added to v6 of dw100. > Just improved documentation and commit logs and a minor checkpatch > fix. For a v7 of your driver, please use this series. I've reviewed the whole series but 2/8 as I'm not familiar enough with the control framework implementation for that one. I think we also need a documentation update to indicate that drivers can modify control dimensions, and that this will reset the control value. It could be done in patch 6/8. > Hans Verkuil (8): > videodev2.h: add V4L2_CTRL_FLAG_DYNAMIC_ARRAY > v4l2-ctrls: add support for dynamically allocated arrays. > vivid: add dynamic array test control > v4l2-ctrls: allocate space for arrays > v4l2-ctrls: alloc arrays in ctrl_ref > v4l2-ctrls: add v4l2_ctrl_modify_dimensions > v4l2-ctrls: add change flag for when dimensions change > vivid: add pixel_array test control > > .../media/v4l/vidioc-dqevent.rst | 5 + > .../media/v4l/vidioc-queryctrl.rst | 8 + > .../media/videodev2.h.rst.exceptions | 2 + > drivers/media/test-drivers/vivid/vivid-core.h | 1 + > .../media/test-drivers/vivid/vivid-ctrls.c | 29 +++ > .../media/test-drivers/vivid/vivid-vid-cap.c | 4 + > drivers/media/v4l2-core/v4l2-ctrls-api.c | 139 ++++++++++--- > drivers/media/v4l2-core/v4l2-ctrls-core.c | 188 +++++++++++++++--- > drivers/media/v4l2-core/v4l2-ctrls-priv.h | 3 +- > drivers/media/v4l2-core/v4l2-ctrls-request.c | 13 +- > include/media/v4l2-ctrls.h | 90 ++++++++- > include/uapi/linux/videodev2.h | 2 + > 12 files changed, 413 insertions(+), 71 deletions(-)
Hi Laurent, On 7/8/22 12:41, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Tue, Jun 28, 2022 at 02:05:22PM +0200, Hans Verkuil wrote: >> Add a new V4L2_EVENT_CTRL_CH_DIMENSIONS change flag that is issued >> when the dimensions of an array change as a result of a >> __v4l2_ctrl_modify_dimensions() call. >> >> This will inform userspace that there are new dimensions. > > While this is easy to add, I'm not sure it will actually be useful in > real use cases. Should we delay adding this new API until someone > actually needs it ? Well, there is a user: dw100. This driver can change dimensions, so any userspace application that subscribes to such a control has to be able to know that the dimensions of that control have changed. Otherwise it will not be able to correctly obtain the control's value. It's not really about this specific driver, it is about the new control feature where dimensions of an array can change. It's also consistent with the other control event change flags. > >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> --- >> Documentation/userspace-api/media/v4l/vidioc-dqevent.rst | 5 +++++ >> Documentation/userspace-api/media/videodev2.h.rst.exceptions | 1 + >> drivers/media/v4l2-core/v4l2-ctrls-api.c | 2 ++ >> include/uapi/linux/videodev2.h | 1 + >> 4 files changed, 9 insertions(+) >> >> diff --git a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst >> index 6eb40073c906..8db103760930 100644 >> --- a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst >> +++ b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst >> @@ -332,6 +332,11 @@ call. >> - 0x0004 >> - This control event was triggered because the minimum, maximum, >> step or the default value of the control changed. >> + * - ``V4L2_EVENT_CTRL_CH_DIMENSIONS`` >> + - 0x0008 >> + - This control event was triggered because the dimensions of the >> + control changed. Note that the number of dimensions remains the >> + same. >> >> >> .. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.5cm}| >> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions >> index 0b91200776f8..274474425b05 100644 >> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions >> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions >> @@ -506,6 +506,7 @@ replace define V4L2_EVENT_PRIVATE_START event-type >> replace define V4L2_EVENT_CTRL_CH_VALUE ctrl-changes-flags >> replace define V4L2_EVENT_CTRL_CH_FLAGS ctrl-changes-flags >> replace define V4L2_EVENT_CTRL_CH_RANGE ctrl-changes-flags >> +replace define V4L2_EVENT_CTRL_CH_DIMENSIONS ctrl-changes-flags >> >> replace define V4L2_EVENT_SRC_CH_RESOLUTION src-changes-flags >> >> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c >> index 16be31966cb1..47f69de9a067 100644 >> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c >> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c >> @@ -1019,6 +1019,8 @@ int __v4l2_ctrl_modify_dimensions(struct v4l2_ctrl *ctrl, >> ctrl->type_ops->init(ctrl, i, ctrl->p_cur); >> ctrl->type_ops->init(ctrl, i, ctrl->p_new); >> } >> + send_event(NULL, ctrl, >> + V4L2_EVENT_CTRL_CH_VALUE | V4L2_EVENT_CTRL_CH_DIMENSIONS); > > Sending V4L2_EVENT_CTRL_CH_VALUE propably belongs to the previous patch > already. True. Regards, Hans > >> return 0; >> } >> EXPORT_SYMBOL(__v4l2_ctrl_modify_dimensions); >> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >> index 9018aa984db3..3971af623c56 100644 >> --- a/include/uapi/linux/videodev2.h >> +++ b/include/uapi/linux/videodev2.h >> @@ -2398,6 +2398,7 @@ struct v4l2_event_vsync { >> #define V4L2_EVENT_CTRL_CH_VALUE (1 << 0) >> #define V4L2_EVENT_CTRL_CH_FLAGS (1 << 1) >> #define V4L2_EVENT_CTRL_CH_RANGE (1 << 2) >> +#define V4L2_EVENT_CTRL_CH_DIMENSIONS (1 << 3) >> >> struct v4l2_event_ctrl { >> __u32 changes; >
Hi Hans, On Fri, Jul 08, 2022 at 12:59:41PM +0200, Hans Verkuil wrote: > On 7/8/22 12:41, Laurent Pinchart wrote: > > On Tue, Jun 28, 2022 at 02:05:22PM +0200, Hans Verkuil wrote: > >> Add a new V4L2_EVENT_CTRL_CH_DIMENSIONS change flag that is issued > >> when the dimensions of an array change as a result of a > >> __v4l2_ctrl_modify_dimensions() call. > >> > >> This will inform userspace that there are new dimensions. > > > > While this is easy to add, I'm not sure it will actually be useful in > > real use cases. Should we delay adding this new API until someone > > actually needs it ? > > Well, there is a user: dw100. This driver can change dimensions, so any > userspace application that subscribes to such a control has to be able to > know that the dimensions of that control have changed. Otherwise it will > not be able to correctly obtain the control's value. I meant a userspace user, not a kernelspace user. Sure, we have test applications that can listen for events, but my experience with libcamera showed me that the control events API is not very usable beside test applications. We don't use it at all in libcamera for that reason, and I think this new event would have the same fate if we don't have a real userspace user to show it's done correctly. > It's not really about this specific driver, it is about the new control > feature where dimensions of an array can change. It's also consistent > with the other control event change flags. > > >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >> --- > >> Documentation/userspace-api/media/v4l/vidioc-dqevent.rst | 5 +++++ > >> Documentation/userspace-api/media/videodev2.h.rst.exceptions | 1 + > >> drivers/media/v4l2-core/v4l2-ctrls-api.c | 2 ++ > >> include/uapi/linux/videodev2.h | 1 + > >> 4 files changed, 9 insertions(+) > >> > >> diff --git a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst > >> index 6eb40073c906..8db103760930 100644 > >> --- a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst > >> +++ b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst > >> @@ -332,6 +332,11 @@ call. > >> - 0x0004 > >> - This control event was triggered because the minimum, maximum, > >> step or the default value of the control changed. > >> + * - ``V4L2_EVENT_CTRL_CH_DIMENSIONS`` > >> + - 0x0008 > >> + - This control event was triggered because the dimensions of the > >> + control changed. Note that the number of dimensions remains the > >> + same. > >> > >> > >> .. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.5cm}| > >> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > >> index 0b91200776f8..274474425b05 100644 > >> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions > >> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > >> @@ -506,6 +506,7 @@ replace define V4L2_EVENT_PRIVATE_START event-type > >> replace define V4L2_EVENT_CTRL_CH_VALUE ctrl-changes-flags > >> replace define V4L2_EVENT_CTRL_CH_FLAGS ctrl-changes-flags > >> replace define V4L2_EVENT_CTRL_CH_RANGE ctrl-changes-flags > >> +replace define V4L2_EVENT_CTRL_CH_DIMENSIONS ctrl-changes-flags > >> > >> replace define V4L2_EVENT_SRC_CH_RESOLUTION src-changes-flags > >> > >> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c > >> index 16be31966cb1..47f69de9a067 100644 > >> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c > >> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c > >> @@ -1019,6 +1019,8 @@ int __v4l2_ctrl_modify_dimensions(struct v4l2_ctrl *ctrl, > >> ctrl->type_ops->init(ctrl, i, ctrl->p_cur); > >> ctrl->type_ops->init(ctrl, i, ctrl->p_new); > >> } > >> + send_event(NULL, ctrl, > >> + V4L2_EVENT_CTRL_CH_VALUE | V4L2_EVENT_CTRL_CH_DIMENSIONS); > > > > Sending V4L2_EVENT_CTRL_CH_VALUE propably belongs to the previous patch > > already. > > True. > > >> return 0; > >> } > >> EXPORT_SYMBOL(__v4l2_ctrl_modify_dimensions); > >> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > >> index 9018aa984db3..3971af623c56 100644 > >> --- a/include/uapi/linux/videodev2.h > >> +++ b/include/uapi/linux/videodev2.h > >> @@ -2398,6 +2398,7 @@ struct v4l2_event_vsync { > >> #define V4L2_EVENT_CTRL_CH_VALUE (1 << 0) > >> #define V4L2_EVENT_CTRL_CH_FLAGS (1 << 1) > >> #define V4L2_EVENT_CTRL_CH_RANGE (1 << 2) > >> +#define V4L2_EVENT_CTRL_CH_DIMENSIONS (1 << 3) > >> > >> struct v4l2_event_ctrl { > >> __u32 changes;
On 7/8/22 13:07, Laurent Pinchart wrote: > Hi Hans, > > On Fri, Jul 08, 2022 at 12:59:41PM +0200, Hans Verkuil wrote: >> On 7/8/22 12:41, Laurent Pinchart wrote: >>> On Tue, Jun 28, 2022 at 02:05:22PM +0200, Hans Verkuil wrote: >>>> Add a new V4L2_EVENT_CTRL_CH_DIMENSIONS change flag that is issued >>>> when the dimensions of an array change as a result of a >>>> __v4l2_ctrl_modify_dimensions() call. >>>> >>>> This will inform userspace that there are new dimensions. >>> >>> While this is easy to add, I'm not sure it will actually be useful in >>> real use cases. Should we delay adding this new API until someone >>> actually needs it ? >> >> Well, there is a user: dw100. This driver can change dimensions, so any >> userspace application that subscribes to such a control has to be able to >> know that the dimensions of that control have changed. Otherwise it will >> not be able to correctly obtain the control's value. > > I meant a userspace user, not a kernelspace user. Sure, we have test > applications that can listen for events, but my experience with > libcamera showed me that the control events API is not very usable > beside test applications. We don't use it at all in libcamera for that > reason, and I think this new event would have the same fate if we don't > have a real userspace user to show it's done correctly. We (Cisco) use control events all the time to detect HDMI signal changes, among others. It all depends on your use case. In this case the event flag IS used by the framework, and is well defined. Whether userspace needs to use it is another matter, and that's something you cannot predict. The problem is when adding API defines that are not used at all in the kernel, but that's not the case here. And frankly, it makes no sense if there is a flag to indicate that the control range changed, but not that the control dimensions changed. It should be there. Regards, Hans > >> It's not really about this specific driver, it is about the new control >> feature where dimensions of an array can change. It's also consistent >> with the other control event change flags. >> >>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >>>> --- >>>> Documentation/userspace-api/media/v4l/vidioc-dqevent.rst | 5 +++++ >>>> Documentation/userspace-api/media/videodev2.h.rst.exceptions | 1 + >>>> drivers/media/v4l2-core/v4l2-ctrls-api.c | 2 ++ >>>> include/uapi/linux/videodev2.h | 1 + >>>> 4 files changed, 9 insertions(+) >>>> >>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst >>>> index 6eb40073c906..8db103760930 100644 >>>> --- a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst >>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst >>>> @@ -332,6 +332,11 @@ call. >>>> - 0x0004 >>>> - This control event was triggered because the minimum, maximum, >>>> step or the default value of the control changed. >>>> + * - ``V4L2_EVENT_CTRL_CH_DIMENSIONS`` >>>> + - 0x0008 >>>> + - This control event was triggered because the dimensions of the >>>> + control changed. Note that the number of dimensions remains the >>>> + same. >>>> >>>> >>>> .. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.5cm}| >>>> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions >>>> index 0b91200776f8..274474425b05 100644 >>>> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions >>>> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions >>>> @@ -506,6 +506,7 @@ replace define V4L2_EVENT_PRIVATE_START event-type >>>> replace define V4L2_EVENT_CTRL_CH_VALUE ctrl-changes-flags >>>> replace define V4L2_EVENT_CTRL_CH_FLAGS ctrl-changes-flags >>>> replace define V4L2_EVENT_CTRL_CH_RANGE ctrl-changes-flags >>>> +replace define V4L2_EVENT_CTRL_CH_DIMENSIONS ctrl-changes-flags >>>> >>>> replace define V4L2_EVENT_SRC_CH_RESOLUTION src-changes-flags >>>> >>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c >>>> index 16be31966cb1..47f69de9a067 100644 >>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c >>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c >>>> @@ -1019,6 +1019,8 @@ int __v4l2_ctrl_modify_dimensions(struct v4l2_ctrl *ctrl, >>>> ctrl->type_ops->init(ctrl, i, ctrl->p_cur); >>>> ctrl->type_ops->init(ctrl, i, ctrl->p_new); >>>> } >>>> + send_event(NULL, ctrl, >>>> + V4L2_EVENT_CTRL_CH_VALUE | V4L2_EVENT_CTRL_CH_DIMENSIONS); >>> >>> Sending V4L2_EVENT_CTRL_CH_VALUE propably belongs to the previous patch >>> already. >> >> True. >> >>>> return 0; >>>> } >>>> EXPORT_SYMBOL(__v4l2_ctrl_modify_dimensions); >>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >>>> index 9018aa984db3..3971af623c56 100644 >>>> --- a/include/uapi/linux/videodev2.h >>>> +++ b/include/uapi/linux/videodev2.h >>>> @@ -2398,6 +2398,7 @@ struct v4l2_event_vsync { >>>> #define V4L2_EVENT_CTRL_CH_VALUE (1 << 0) >>>> #define V4L2_EVENT_CTRL_CH_FLAGS (1 << 1) >>>> #define V4L2_EVENT_CTRL_CH_RANGE (1 << 2) >>>> +#define V4L2_EVENT_CTRL_CH_DIMENSIONS (1 << 3) >>>> >>>> struct v4l2_event_ctrl { >>>> __u32 changes; >
Hi Hans, On Fri, Jul 08, 2022 at 01:33:28PM +0200, Hans Verkuil wrote: > On 7/8/22 13:07, Laurent Pinchart wrote: > > On Fri, Jul 08, 2022 at 12:59:41PM +0200, Hans Verkuil wrote: > >> On 7/8/22 12:41, Laurent Pinchart wrote: > >>> On Tue, Jun 28, 2022 at 02:05:22PM +0200, Hans Verkuil wrote: > >>>> Add a new V4L2_EVENT_CTRL_CH_DIMENSIONS change flag that is issued > >>>> when the dimensions of an array change as a result of a > >>>> __v4l2_ctrl_modify_dimensions() call. > >>>> > >>>> This will inform userspace that there are new dimensions. > >>> > >>> While this is easy to add, I'm not sure it will actually be useful in > >>> real use cases. Should we delay adding this new API until someone > >>> actually needs it ? > >> > >> Well, there is a user: dw100. This driver can change dimensions, so any > >> userspace application that subscribes to such a control has to be able to > >> know that the dimensions of that control have changed. Otherwise it will > >> not be able to correctly obtain the control's value. > > > > I meant a userspace user, not a kernelspace user. Sure, we have test > > applications that can listen for events, but my experience with > > libcamera showed me that the control events API is not very usable > > beside test applications. We don't use it at all in libcamera for that > > reason, and I think this new event would have the same fate if we don't > > have a real userspace user to show it's done correctly. > > We (Cisco) use control events all the time to detect HDMI signal changes, > among others. For things that are asynchronous, I think it really makes sense. > It all depends on your use case. In this case the event flag IS used > by the framework, and is well defined. Whether userspace needs to use it > is another matter, and that's something you cannot predict. True, but shouldn't we refrain from adding APIs until someone actually needs them ? Otherwise it's just code bloat on the kernel side, and it also gets in the way of development as ABI stability has to be guaranteed. We should discuss the control events API at some point btw, maybe in the workshop in Dublin ? > The problem is when adding API defines that are not used at all in the > kernel, but that's not the case here. > > And frankly, it makes no sense if there is a flag to indicate that the > control range changed, but not that the control dimensions changed. It > should be there. I think I would have advised against adding a control range change event if I had known better at the time :-) > >> It's not really about this specific driver, it is about the new control > >> feature where dimensions of an array can change. It's also consistent > >> with the other control event change flags. > >> > >>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >>>> --- > >>>> Documentation/userspace-api/media/v4l/vidioc-dqevent.rst | 5 +++++ > >>>> Documentation/userspace-api/media/videodev2.h.rst.exceptions | 1 + > >>>> drivers/media/v4l2-core/v4l2-ctrls-api.c | 2 ++ > >>>> include/uapi/linux/videodev2.h | 1 + > >>>> 4 files changed, 9 insertions(+) > >>>> > >>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst > >>>> index 6eb40073c906..8db103760930 100644 > >>>> --- a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst > >>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst > >>>> @@ -332,6 +332,11 @@ call. > >>>> - 0x0004 > >>>> - This control event was triggered because the minimum, maximum, > >>>> step or the default value of the control changed. > >>>> + * - ``V4L2_EVENT_CTRL_CH_DIMENSIONS`` > >>>> + - 0x0008 > >>>> + - This control event was triggered because the dimensions of the > >>>> + control changed. Note that the number of dimensions remains the > >>>> + same. > >>>> > >>>> > >>>> .. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.5cm}| > >>>> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > >>>> index 0b91200776f8..274474425b05 100644 > >>>> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions > >>>> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > >>>> @@ -506,6 +506,7 @@ replace define V4L2_EVENT_PRIVATE_START event-type > >>>> replace define V4L2_EVENT_CTRL_CH_VALUE ctrl-changes-flags > >>>> replace define V4L2_EVENT_CTRL_CH_FLAGS ctrl-changes-flags > >>>> replace define V4L2_EVENT_CTRL_CH_RANGE ctrl-changes-flags > >>>> +replace define V4L2_EVENT_CTRL_CH_DIMENSIONS ctrl-changes-flags > >>>> > >>>> replace define V4L2_EVENT_SRC_CH_RESOLUTION src-changes-flags > >>>> > >>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c > >>>> index 16be31966cb1..47f69de9a067 100644 > >>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c > >>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c > >>>> @@ -1019,6 +1019,8 @@ int __v4l2_ctrl_modify_dimensions(struct v4l2_ctrl *ctrl, > >>>> ctrl->type_ops->init(ctrl, i, ctrl->p_cur); > >>>> ctrl->type_ops->init(ctrl, i, ctrl->p_new); > >>>> } > >>>> + send_event(NULL, ctrl, > >>>> + V4L2_EVENT_CTRL_CH_VALUE | V4L2_EVENT_CTRL_CH_DIMENSIONS); > >>> > >>> Sending V4L2_EVENT_CTRL_CH_VALUE propably belongs to the previous patch > >>> already. > >> > >> True. > >> > >>>> return 0; > >>>> } > >>>> EXPORT_SYMBOL(__v4l2_ctrl_modify_dimensions); > >>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > >>>> index 9018aa984db3..3971af623c56 100644 > >>>> --- a/include/uapi/linux/videodev2.h > >>>> +++ b/include/uapi/linux/videodev2.h > >>>> @@ -2398,6 +2398,7 @@ struct v4l2_event_vsync { > >>>> #define V4L2_EVENT_CTRL_CH_VALUE (1 << 0) > >>>> #define V4L2_EVENT_CTRL_CH_FLAGS (1 << 1) > >>>> #define V4L2_EVENT_CTRL_CH_RANGE (1 << 2) > >>>> +#define V4L2_EVENT_CTRL_CH_DIMENSIONS (1 << 3) > >>>> > >>>> struct v4l2_event_ctrl { > >>>> __u32 changes;