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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxx>
> + *
> + * 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.
> *
--
Regards,
Laurent Pinchart
