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@xxxxxxxxx>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> ---
> 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;
> };
--
Regards,
Laurent Pinchart
