| Message ID | 20220628120523.2915913-7-hverkuil-cisco@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| 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: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. > *
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]; + if (elems == 0) + return -EINVAL; + p_array = kvzalloc(2 * elems * ctrl->elem_size, GFP_KERNEL); + 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); + } + 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(). + * + * 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. + * + * 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. *
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(+)