[PATCH 0/8] Add dynamic arrays and v4l2_ctrl_modify_dimensions

[PATCH 0/8] Add dynamic arrays and v4l2_ctrl_modify_dimensions

[Hans Verkuil]

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.

Regards,

	Hans

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(-)

-- 
2.35.1

[PATCH 1/8] videodev2.h: add V4L2_CTRL_FLAG_DYNAMIC_ARRAY

[Hans Verkuil]

Add a new flag that indicates that this control is a dynamically sized
array. Also document this flag.

Currently dynamically sized arrays are limited to one dimensional arrays,
but that might change in the future if there is a need for it.

The initial use-case of dynamic arrays are stateless codecs. A frame
can be divided in many slices, so you want to provide an array containing
slice information for each slice. Typically the number of slices is small,
but the standard allow for hundreds or thousands of slices. Dynamic arrays
are a good solution since sizing the array for the worst case would waste
substantial amounts of memory.

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>
---
 .../userspace-api/media/v4l/vidioc-queryctrl.rst          | 8 ++++++++
 .../userspace-api/media/videodev2.h.rst.exceptions        | 1 +
 include/uapi/linux/videodev2.h                            | 1 +
 3 files changed, 10 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
index 88f630252d98..a20dfa2a933b 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
@@ -625,6 +625,14 @@ See also the examples in :ref:`control`.
 	``V4L2_CTRL_FLAG_GRABBED`` flag when buffers are allocated or
 	streaming is in progress since most drivers do not support changing
 	the format in that case.
+    * - ``V4L2_CTRL_FLAG_DYNAMIC_ARRAY``
+      - 0x0800
+      - This control is a dynamically sized 1-dimensional array. It
+        behaves the same as a regular array, except that the number
+	of elements as reported by the ``elems`` field is between 1 and
+	``dims[0]``. So setting the control with a differently sized
+	array will change the ``elems`` field when the control is
+	queried afterwards.
 
 Return Value
 ============
diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
index 9cbb7a0c354a..0b91200776f8 100644
--- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
+++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
@@ -379,6 +379,7 @@ replace define V4L2_CTRL_FLAG_VOLATILE control-flags
 replace define V4L2_CTRL_FLAG_HAS_PAYLOAD control-flags
 replace define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE control-flags
 replace define V4L2_CTRL_FLAG_MODIFY_LAYOUT control-flags
+replace define V4L2_CTRL_FLAG_DYNAMIC_ARRAY control-flags
 
 replace define V4L2_CTRL_FLAG_NEXT_CTRL control
 replace define V4L2_CTRL_FLAG_NEXT_COMPOUND control
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 5311ac4fde35..9018aa984db3 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1890,6 +1890,7 @@ struct v4l2_querymenu {
 #define V4L2_CTRL_FLAG_HAS_PAYLOAD	0x0100
 #define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE	0x0200
 #define V4L2_CTRL_FLAG_MODIFY_LAYOUT	0x0400
+#define V4L2_CTRL_FLAG_DYNAMIC_ARRAY	0x0800
 
 /*  Query flags, to be ORed with the control ID */
 #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000
-- 
2.35.1

Re: [PATCH 1/8] videodev2.h: add V4L2_CTRL_FLAG_DYNAMIC_ARRAY

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Tue, Jun 28, 2022 at 02:05:16PM +0200, Hans Verkuil wrote:
> Add a new flag that indicates that this control is a dynamically sized
> array. Also document this flag.
> 
> Currently dynamically sized arrays are limited to one dimensional arrays,
> but that might change in the future if there is a need for it.
> 
> The initial use-case of dynamic arrays are stateless codecs. A frame
> can be divided in many slices, so you want to provide an array containing
> slice information for each slice. Typically the number of slices is small,
> but the standard allow for hundreds or thousands of slices. Dynamic arrays
> are a good solution since sizing the array for the worst case would waste
> substantial amounts of memory.
> 
> 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>

> ---
>  .../userspace-api/media/v4l/vidioc-queryctrl.rst          | 8 ++++++++
>  .../userspace-api/media/videodev2.h.rst.exceptions        | 1 +
>  include/uapi/linux/videodev2.h                            | 1 +
>  3 files changed, 10 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> index 88f630252d98..a20dfa2a933b 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> @@ -625,6 +625,14 @@ See also the examples in :ref:`control`.
>  	``V4L2_CTRL_FLAG_GRABBED`` flag when buffers are allocated or
>  	streaming is in progress since most drivers do not support changing
>  	the format in that case.
> +    * - ``V4L2_CTRL_FLAG_DYNAMIC_ARRAY``
> +      - 0x0800
> +      - This control is a dynamically sized 1-dimensional array. It
> +        behaves the same as a regular array, except that the number
> +	of elements as reported by the ``elems`` field is between 1 and
> +	``dims[0]``. So setting the control with a differently sized
> +	array will change the ``elems`` field when the control is
> +	queried afterwards.
>  
>  Return Value
>  ============
> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> index 9cbb7a0c354a..0b91200776f8 100644
> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> @@ -379,6 +379,7 @@ replace define V4L2_CTRL_FLAG_VOLATILE control-flags
>  replace define V4L2_CTRL_FLAG_HAS_PAYLOAD control-flags
>  replace define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE control-flags
>  replace define V4L2_CTRL_FLAG_MODIFY_LAYOUT control-flags
> +replace define V4L2_CTRL_FLAG_DYNAMIC_ARRAY control-flags
>  
>  replace define V4L2_CTRL_FLAG_NEXT_CTRL control
>  replace define V4L2_CTRL_FLAG_NEXT_COMPOUND control
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 5311ac4fde35..9018aa984db3 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1890,6 +1890,7 @@ struct v4l2_querymenu {
>  #define V4L2_CTRL_FLAG_HAS_PAYLOAD	0x0100
>  #define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE	0x0200
>  #define V4L2_CTRL_FLAG_MODIFY_LAYOUT	0x0400
> +#define V4L2_CTRL_FLAG_DYNAMIC_ARRAY	0x0800
>  
>  /*  Query flags, to be ORed with the control ID */
>  #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000

-- 
Regards,

Laurent Pinchart

[PATCH 2/8] v4l2-ctrls: add support for dynamically allocated arrays.

[Hans Verkuil]

Implement support for dynamically allocated arrays.

Most of the changes concern keeping track of the number of elements
of the array and the number of elements allocated for the array and
reallocating memory if needed.

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>
---
 drivers/media/v4l2-core/v4l2-ctrls-api.c     | 103 ++++++++---
 drivers/media/v4l2-core/v4l2-ctrls-core.c    | 182 +++++++++++++++----
 drivers/media/v4l2-core/v4l2-ctrls-priv.h    |   3 +-
 drivers/media/v4l2-core/v4l2-ctrls-request.c |  13 +-
 include/media/v4l2-ctrls.h                   |  42 ++++-
 5 files changed, 272 insertions(+), 71 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
index db9baa0bd05f..50d012ba3c02 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
@@ -97,29 +97,47 @@ 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 caller-provider value to the given control value */
-static int user_to_ptr(struct v4l2_ext_control *c,
-		       struct v4l2_ctrl *ctrl,
-		       union v4l2_ctrl_ptr ptr)
+/* 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)
 {
 	int ret;
 	u32 size;
 
-	ctrl->is_new = 1;
+	ctrl->is_new = 0;
+	if (ctrl->is_dyn_array &&
+	    c->size > ctrl->p_dyn_alloc_elems * ctrl->elem_size) {
+		void *old = ctrl->p_dyn;
+		void *tmp = kvzalloc(2 * c->size, GFP_KERNEL);
+
+		if (!tmp)
+			return -ENOMEM;
+		memcpy(tmp, ctrl->p_new.p, ctrl->elems * ctrl->elem_size);
+		memcpy(tmp + c->size, ctrl->p_cur.p, ctrl->elems * ctrl->elem_size);
+		ctrl->p_new.p = tmp;
+		ctrl->p_cur.p = tmp + c->size;
+		ctrl->p_dyn = tmp;
+		ctrl->p_dyn_alloc_elems = c->size / ctrl->elem_size;
+		kvfree(old);
+	}
+
 	if (ctrl->is_ptr && !ctrl->is_string) {
+		unsigned int elems = c->size / ctrl->elem_size;
 		unsigned int idx;
 
-		ret = copy_from_user(ptr.p, c->ptr, c->size) ? -EFAULT : 0;
-		if (ret || !ctrl->is_array)
-			return ret;
-		for (idx = c->size / ctrl->elem_size; idx < ctrl->elems; idx++)
-			ctrl->type_ops->init(ctrl, idx, ptr);
+		if (copy_from_user(ctrl->p_new.p, c->ptr, c->size))
+			return -EFAULT;
+		ctrl->is_new = 1;
+		if (ctrl->is_dyn_array)
+			ctrl->new_elems = elems;
+		else if (ctrl->is_array)
+			for (idx = elems; idx < ctrl->elems; idx++)
+				ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
 		return 0;
 	}
 
 	switch (ctrl->type) {
 	case V4L2_CTRL_TYPE_INTEGER64:
-		*ptr.p_s64 = c->value64;
+		*ctrl->p_new.p_s64 = c->value64;
 		break;
 	case V4L2_CTRL_TYPE_STRING:
 		size = c->size;
@@ -127,32 +145,27 @@ static int user_to_ptr(struct v4l2_ext_control *c,
 			return -ERANGE;
 		if (size > ctrl->maximum + 1)
 			size = ctrl->maximum + 1;
-		ret = copy_from_user(ptr.p_char, c->string, size) ? -EFAULT : 0;
+		ret = copy_from_user(ctrl->p_new.p_char, c->string, size) ? -EFAULT : 0;
 		if (!ret) {
-			char last = ptr.p_char[size - 1];
+			char last = ctrl->p_new.p_char[size - 1];
 
-			ptr.p_char[size - 1] = 0;
+			ctrl->p_new.p_char[size - 1] = 0;
 			/*
 			 * If the string was longer than ctrl->maximum,
 			 * then return an error.
 			 */
-			if (strlen(ptr.p_char) == ctrl->maximum && last)
+			if (strlen(ctrl->p_new.p_char) == ctrl->maximum && last)
 				return -ERANGE;
 		}
 		return ret;
 	default:
-		*ptr.p_s32 = c->value;
+		*ctrl->p_new.p_s32 = c->value;
 		break;
 	}
+	ctrl->is_new = 1;
 	return 0;
 }
 
-/* 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)
-{
-	return user_to_ptr(c, ctrl, ctrl->p_new);
-}
-
 /*
  * VIDIOC_G/TRY/S_EXT_CTRLS implementation
  */
@@ -254,7 +267,31 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 			have_clusters = true;
 		if (ctrl->cluster[0] != ctrl)
 			ref = find_ref_lock(hdl, ctrl->cluster[0]->id);
-		if (ctrl->is_ptr && !ctrl->is_string) {
+		if (ctrl->is_dyn_array) {
+			unsigned int max_size = ctrl->dims[0] * ctrl->elem_size;
+			unsigned int tot_size = ctrl->elem_size;
+
+			if (cs->which == V4L2_CTRL_WHICH_REQUEST_VAL)
+				tot_size *= ref->p_req_elems;
+			else
+				tot_size *= ctrl->elems;
+
+			c->size = ctrl->elem_size * (c->size / ctrl->elem_size);
+			if (get) {
+				if (c->size < tot_size) {
+					c->size = tot_size;
+					return -ENOSPC;
+				}
+				c->size = tot_size;
+			} else {
+				if (c->size > max_size) {
+					c->size = max_size;
+					return -ENOSPC;
+				}
+				if (!c->size)
+					return -EFAULT;
+			}
+		} else if (ctrl->is_ptr && !ctrl->is_string) {
 			unsigned int tot_size = ctrl->elems * ctrl->elem_size;
 
 			if (c->size < tot_size) {
@@ -346,7 +383,7 @@ static int class_check(struct v4l2_ctrl_handler *hdl, u32 which)
  *
  * Note that v4l2_g_ext_ctrls_common() with 'which' set to
  * V4L2_CTRL_WHICH_REQUEST_VAL is only called if the request was
- * completed, and in that case valid_p_req is true for all controls.
+ * completed, and in that case p_req_valid is true for all controls.
  */
 int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl,
 			    struct v4l2_ext_controls *cs,
@@ -430,7 +467,9 @@ 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->valid_p_req)
+			else if (is_request && ref->p_req_dyn_enomem)
+				ret = -ENOMEM;
+			else if (is_request && ref->p_req_valid)
 				ret = req_to_user(cs->controls + idx, ref);
 			else if (is_volatile)
 				ret = new_to_user(cs->controls + idx, ref->ctrl);
@@ -457,6 +496,17 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct video_device *vdev,
 }
 EXPORT_SYMBOL(v4l2_g_ext_ctrls);
 
+/* Validate a new control */
+static int validate_new(const struct v4l2_ctrl *ctrl, union v4l2_ctrl_ptr p_new)
+{
+	unsigned int idx;
+	int err = 0;
+
+	for (idx = 0; !err && idx < ctrl->new_elems; idx++)
+		err = ctrl->type_ops->validate(ctrl, idx, p_new);
+	return err;
+}
+
 /* Validate controls. */
 static int validate_ctrls(struct v4l2_ext_controls *cs,
 			  struct v4l2_ctrl_helper *helpers,
@@ -872,6 +922,9 @@ int __v4l2_ctrl_s_ctrl_compound(struct v4l2_ctrl *ctrl,
 	/* It's a driver bug if this happens. */
 	if (WARN_ON(ctrl->type != type))
 		return -EINVAL;
+	/* Setting dynamic arrays is not (yet?) supported. */
+	if (WARN_ON(ctrl->is_dyn_array))
+		return -EINVAL;
 	memcpy(ctrl->p_new.p, p, ctrl->elems * ctrl->elem_size);
 	return set_ctrl(NULL, ctrl, 0);
 }
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
index 949c1884d9c1..ff8a61f24d0a 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
@@ -991,11 +991,12 @@ EXPORT_SYMBOL(v4l2_ctrl_notify);
 
 /* Copy the one value to another. */
 static void ptr_to_ptr(struct v4l2_ctrl *ctrl,
-		       union v4l2_ctrl_ptr from, union v4l2_ctrl_ptr to)
+		       union v4l2_ctrl_ptr from, union v4l2_ctrl_ptr to,
+		       unsigned int elems)
 {
 	if (ctrl == NULL)
 		return;
-	memcpy(to.p, from.p_const, ctrl->elems * ctrl->elem_size);
+	memcpy(to.p, from.p_const, elems * ctrl->elem_size);
 }
 
 /* Copy the new value to the current value. */
@@ -1008,8 +1009,11 @@ void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags)
 
 	/* has_changed is set by cluster_changed */
 	changed = ctrl->has_changed;
-	if (changed)
-		ptr_to_ptr(ctrl, ctrl->p_new, ctrl->p_cur);
+	if (changed) {
+		if (ctrl->is_dyn_array)
+			ctrl->elems = ctrl->new_elems;
+		ptr_to_ptr(ctrl, ctrl->p_new, ctrl->p_cur, ctrl->elems);
+	}
 
 	if (ch_flags & V4L2_EVENT_CTRL_CH_FLAGS) {
 		/* Note: CH_FLAGS is only set for auto clusters. */
@@ -1039,36 +1043,122 @@ void cur_to_new(struct v4l2_ctrl *ctrl)
 {
 	if (ctrl == NULL)
 		return;
-	ptr_to_ptr(ctrl, ctrl->p_cur, ctrl->p_new);
+	if (ctrl->is_dyn_array)
+		ctrl->new_elems = ctrl->elems;
+	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)
+{
+	void *tmp;
+
+	if (elems < ref->p_req_dyn_alloc_elems)
+		return true;
+
+	tmp = kvmalloc(elems * ref->ctrl->elem_size, GFP_KERNEL);
+
+	if (!tmp) {
+		ref->p_req_dyn_enomem = true;
+		return false;
+	}
+	ref->p_req_dyn_enomem = false;
+	kvfree(ref->p_req.p);
+	ref->p_req.p = tmp;
+	ref->p_req_dyn_alloc_elems = elems;
+	return true;
 }
 
 /* Copy the new value to the request value */
 void new_to_req(struct v4l2_ctrl_ref *ref)
 {
+	struct v4l2_ctrl *ctrl;
+
 	if (!ref)
 		return;
-	ptr_to_ptr(ref->ctrl, ref->ctrl->p_new, ref->p_req);
-	ref->valid_p_req = true;
+
+	ctrl = ref->ctrl;
+	if (ctrl->is_dyn_array && !req_alloc_dyn_array(ref, ctrl->new_elems))
+		return;
+
+	ref->p_req_elems = ctrl->new_elems;
+	ptr_to_ptr(ctrl, ctrl->p_new, ref->p_req, ref->p_req_elems);
+	ref->p_req_valid = true;
 }
 
 /* Copy the current value to the request value */
 void cur_to_req(struct v4l2_ctrl_ref *ref)
 {
+	struct v4l2_ctrl *ctrl;
+
 	if (!ref)
 		return;
-	ptr_to_ptr(ref->ctrl, ref->ctrl->p_cur, ref->p_req);
-	ref->valid_p_req = true;
+
+	ctrl = ref->ctrl;
+	if (ctrl->is_dyn_array && !req_alloc_dyn_array(ref, ctrl->elems))
+		return;
+
+	ref->p_req_elems = ctrl->elems;
+	ptr_to_ptr(ctrl, ctrl->p_cur, ref->p_req, ctrl->elems);
+	ref->p_req_valid = true;
 }
 
 /* Copy the request value to the new value */
-void req_to_new(struct v4l2_ctrl_ref *ref)
+int req_to_new(struct v4l2_ctrl_ref *ref)
 {
+	struct v4l2_ctrl *ctrl;
+
 	if (!ref)
-		return;
-	if (ref->valid_p_req)
-		ptr_to_ptr(ref->ctrl, ref->p_req, ref->ctrl->p_new);
-	else
-		ptr_to_ptr(ref->ctrl, ref->ctrl->p_cur, ref->ctrl->p_new);
+		return 0;
+
+	ctrl = ref->ctrl;
+
+	/*
+	 * This control was never set in the request, so just use the current
+	 * value.
+	 */
+	if (!ref->p_req_valid) {
+		if (ctrl->is_dyn_array)
+			ctrl->new_elems = ctrl->elems;
+		ptr_to_ptr(ctrl, ctrl->p_cur, ctrl->p_new, ctrl->new_elems);
+		return 0;
+	}
+
+	/* Not a dynamic array, so just copy the request value */
+	if (!ctrl->is_dyn_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))
+		return -ENOMEM;
+
+	/*
+	 * Check if the number of elements in the request is more than the
+	 * elements in ctrl->p_dyn. If so, attempt to realloc ctrl->p_dyn.
+	 * Note that p_dyn is allocated with twice the number of elements
+	 * in the dynamic array since it has to store both the current and
+	 * new value of such a control.
+	 */
+	if (ref->p_req_elems > ctrl->p_dyn_alloc_elems) {
+		unsigned int sz = ref->p_req_elems * ctrl->elem_size;
+		void *old = ctrl->p_dyn;
+		void *tmp = kvzalloc(2 * sz, GFP_KERNEL);
+
+		if (!tmp)
+			return -ENOMEM;
+		memcpy(tmp, ctrl->p_new.p, ctrl->elems * ctrl->elem_size);
+		memcpy(tmp + sz, ctrl->p_cur.p, ctrl->elems * ctrl->elem_size);
+		ctrl->p_new.p = tmp;
+		ctrl->p_cur.p = tmp + sz;
+		ctrl->p_dyn = tmp;
+		ctrl->p_dyn_alloc_elems = ref->p_req_elems;
+		kvfree(old);
+	}
+
+	ctrl->new_elems = ref->p_req_elems;
+	ptr_to_ptr(ctrl, ref->p_req, ctrl->p_new, ctrl->new_elems);
+	return 0;
 }
 
 /* Control range checking */
@@ -1110,17 +1200,6 @@ int check_range(enum v4l2_ctrl_type type,
 	}
 }
 
-/* Validate a new control */
-int validate_new(const struct v4l2_ctrl *ctrl, union v4l2_ctrl_ptr p_new)
-{
-	unsigned idx;
-	int err = 0;
-
-	for (idx = 0; !err && idx < ctrl->elems; idx++)
-		err = ctrl->type_ops->validate(ctrl, idx, p_new);
-	return err;
-}
-
 /* Set the handler's error code if it wasn't set earlier already */
 static inline int handler_set_err(struct v4l2_ctrl_handler *hdl, int err)
 {
@@ -1164,6 +1243,8 @@ 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)
+			kvfree(ref->p_req.p);
 		kfree(ref);
 	}
 	/* Free all controls owned by the handler */
@@ -1171,6 +1252,7 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
 		list_del(&ctrl->node);
 		list_for_each_entry_safe(sev, next_sev, &ctrl->ev_subs, node)
 			list_del(&sev->node);
+		kvfree(ctrl->p_dyn);
 		kvfree(ctrl);
 	}
 	kvfree(hdl->buckets);
@@ -1286,7 +1368,7 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl,
 	if (hdl->error)
 		return hdl->error;
 
-	if (allocate_req)
+	if (allocate_req && !ctrl->is_dyn_array)
 		size_extra_req = ctrl->elems * ctrl->elem_size;
 	new_ref = kzalloc(sizeof(*new_ref) + size_extra_req, GFP_KERNEL);
 	if (!new_ref)
@@ -1460,7 +1542,6 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 			elem_size = sizeof(s32);
 		break;
 	}
-	tot_ctrl_size = elem_size * elems;
 
 	/* Sanity checks */
 	if (id == 0 || name == NULL || !elem_size ||
@@ -1481,17 +1562,33 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 		handler_set_err(hdl, -EINVAL);
 		return NULL;
 	}
+	if (flags & V4L2_CTRL_FLAG_DYNAMIC_ARRAY) {
+		/*
+		 * For now only support this for one-dimensional arrays only.
+		 *
+		 * This can be relaxed in the future, but this will
+		 * require more effort.
+		 */
+		if (nr_of_dims != 1) {
+			handler_set_err(hdl, -EINVAL);
+			return NULL;
+		}
+		/* Start with just 1 element */
+		elems = 1;
+	}
 
+	tot_ctrl_size = elem_size * elems;
 	sz_extra = 0;
 	if (type == V4L2_CTRL_TYPE_BUTTON)
 		flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
 			V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
 	else if (type == V4L2_CTRL_TYPE_CTRL_CLASS)
 		flags |= V4L2_CTRL_FLAG_READ_ONLY;
-	else if (type == V4L2_CTRL_TYPE_INTEGER64 ||
-		 type == V4L2_CTRL_TYPE_STRING ||
-		 type >= V4L2_CTRL_COMPOUND_TYPES ||
-		 is_array)
+	else if (!(flags & V4L2_CTRL_FLAG_DYNAMIC_ARRAY) &&
+		 (type == V4L2_CTRL_TYPE_INTEGER64 ||
+		  type == V4L2_CTRL_TYPE_STRING ||
+		  type >= V4L2_CTRL_COMPOUND_TYPES ||
+		  is_array))
 		sz_extra += 2 * tot_ctrl_size;
 
 	if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p_const)
@@ -1520,7 +1617,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	ctrl->is_ptr = is_array || type >= V4L2_CTRL_COMPOUND_TYPES || ctrl->is_string;
 	ctrl->is_int = !ctrl->is_ptr && type != V4L2_CTRL_TYPE_INTEGER64;
 	ctrl->is_array = is_array;
+	ctrl->is_dyn_array = !!(flags & V4L2_CTRL_FLAG_DYNAMIC_ARRAY);
 	ctrl->elems = elems;
+	ctrl->new_elems = elems;
 	ctrl->nr_of_dims = nr_of_dims;
 	if (nr_of_dims)
 		memcpy(ctrl->dims, dims, nr_of_dims * sizeof(dims[0]));
@@ -1533,6 +1632,16 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	ctrl->cur.val = ctrl->val = def;
 	data = &ctrl[1];
 
+	if (ctrl->is_dyn_array) {
+		ctrl->p_dyn_alloc_elems = elems;
+		ctrl->p_dyn = kvzalloc(2 * elems * elem_size, GFP_KERNEL);
+		if (!ctrl->p_dyn) {
+			kvfree(ctrl);
+			return NULL;
+		}
+		data = ctrl->p_dyn;
+	}
+
 	if (!ctrl->is_int) {
 		ctrl->p_new.p = data;
 		ctrl->p_cur.p = data + tot_ctrl_size;
@@ -1542,7 +1651,10 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	}
 
 	if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p_const) {
-		ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size;
+		if (ctrl->is_dyn_array)
+			ctrl->p_def.p = &ctrl[1];
+		else
+			ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size;
 		memcpy(ctrl->p_def.p, p_def.p_const, elem_size);
 	}
 
@@ -1552,6 +1664,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	}
 
 	if (handler_new_ref(hdl, ctrl, NULL, false, false)) {
+		kvfree(ctrl->p_dyn);
 		kvfree(ctrl);
 		return NULL;
 	}
@@ -1889,6 +2002,9 @@ static int cluster_changed(struct v4l2_ctrl *master)
 			continue;
 		}
 
+		if (ctrl->elems != ctrl->new_elems)
+			ctrl_changed = true;
+
 		for (idx = 0; !ctrl_changed && idx < ctrl->elems; idx++)
 			ctrl_changed = !ctrl->type_ops->equal(ctrl, idx,
 				ctrl->p_cur, ctrl->p_new);
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-priv.h b/drivers/media/v4l2-core/v4l2-ctrls-priv.h
index d4bf2c716f97..aba6176fab6c 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-priv.h
+++ b/drivers/media/v4l2-core/v4l2-ctrls-priv.h
@@ -57,10 +57,9 @@ void cur_to_new(struct v4l2_ctrl *ctrl);
 void cur_to_req(struct v4l2_ctrl_ref *ref);
 void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags);
 void new_to_req(struct v4l2_ctrl_ref *ref);
-void req_to_new(struct v4l2_ctrl_ref *ref);
+int req_to_new(struct v4l2_ctrl_ref *ref);
 void send_initial_event(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl);
 void send_event(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 changes);
-int validate_new(const struct v4l2_ctrl *ctrl, union v4l2_ctrl_ptr p_new);
 int handler_new_ref(struct v4l2_ctrl_handler *hdl,
 		    struct v4l2_ctrl *ctrl,
 		    struct v4l2_ctrl_ref **ctrl_ref,
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-request.c b/drivers/media/v4l2-core/v4l2-ctrls-request.c
index 7d098f287fd9..c637049d7a2b 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-request.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-request.c
@@ -143,7 +143,7 @@ v4l2_ctrl_request_hdl_ctrl_find(struct v4l2_ctrl_handler *hdl, u32 id)
 {
 	struct v4l2_ctrl_ref *ref = find_ref_lock(hdl, id);
 
-	return (ref && ref->valid_p_req) ? ref->ctrl : NULL;
+	return (ref && ref->p_req_valid) ? ref->ctrl : NULL;
 }
 EXPORT_SYMBOL_GPL(v4l2_ctrl_request_hdl_ctrl_find);
 
@@ -373,7 +373,7 @@ void v4l2_ctrl_request_complete(struct media_request *req,
 			v4l2_ctrl_unlock(master);
 			continue;
 		}
-		if (ref->valid_p_req)
+		if (ref->p_req_valid)
 			continue;
 
 		/* Copy the current control value into the request */
@@ -442,7 +442,7 @@ int v4l2_ctrl_request_setup(struct media_request *req,
 				struct v4l2_ctrl_ref *r =
 					find_ref(hdl, master->cluster[i]->id);
 
-				if (r->valid_p_req) {
+				if (r->p_req_valid) {
 					have_new_data = true;
 					break;
 				}
@@ -458,7 +458,11 @@ int v4l2_ctrl_request_setup(struct media_request *req,
 				struct v4l2_ctrl_ref *r =
 					find_ref(hdl, master->cluster[i]->id);
 
-				req_to_new(r);
+				ret = req_to_new(r);
+				if (ret) {
+					v4l2_ctrl_unlock(master);
+					goto error;
+				}
 				master->cluster[i]->is_new = 1;
 				r->req_done = true;
 			}
@@ -490,6 +494,7 @@ int v4l2_ctrl_request_setup(struct media_request *req,
 			break;
 	}
 
+error:
 	media_request_object_put(obj);
 	return ret;
 }
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index b3ce438f1329..f4105de8a8d2 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -185,6 +185,8 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
  *		and/or has type %V4L2_CTRL_TYPE_STRING. In other words, &struct
  *		v4l2_ext_control uses field p to point to the data.
  * @is_array: If set, then this control contains an N-dimensional array.
+ * @is_dyn_array: If set, then this control contains a dynamically sized 1-dimensional array.
+ *		If this is set, then @is_array is also set.
  * @has_volatiles: If set, then one or more members of the cluster are volatile.
  *		Drivers should never touch this flag.
  * @call_notify: If set, then call the handler's notify function whenever the
@@ -205,6 +207,9 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
  * @step:	The control's step value for non-menu controls.
  * @elems:	The number of elements in the N-dimensional array.
  * @elem_size:	The size in bytes of the control.
+ * @new_elems:	The number of elements in p_new. This is the same as @elems,
+ *		except for dynamic arrays. In that case it is in the range of
+ *		1 to @p_dyn_alloc_elems.
  * @dims:	The size of each dimension.
  * @nr_of_dims:The number of dimensions in @dims.
  * @menu_skip_mask: The control's skip mask for menu controls. This makes it
@@ -223,15 +228,21 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
  *		:math:`ceil(\frac{maximum - minimum}{step}) + 1`.
  *		Used only if the @type is %V4L2_CTRL_TYPE_INTEGER_MENU.
  * @flags:	The control's flags.
- * @cur:	Structure to store the current value.
- * @cur.val:	The control's current value, if the @type is represented via
- *		a u32 integer (see &enum v4l2_ctrl_type).
- * @val:	The control's new s32 value.
  * @priv:	The control's private pointer. For use by the driver. It is
  *		untouched by the control framework. Note that this pointer is
  *		not freed when the control is deleted. Should this be needed
  *		then a new internal bitfield can be added to tell the framework
  *		to free this pointer.
+ * @p_dyn:	Pointer to the dynamically allocated array. Only valid if
+ *		@is_dyn_array is true.
+ * @p_dyn_alloc_elems: The number of elements in the dynamically allocated
+ *		array for both the cur and new values. So @p_dyn is actually
+ *		sized for 2 * @p_dyn_alloc_elems * @elem_size. Only valid if
+ *		@is_dyn_array is true.
+ * @cur:	Structure to store the current value.
+ * @cur.val:	The control's current value, if the @type is represented via
+ *		a u32 integer (see &enum v4l2_ctrl_type).
+ * @val:	The control's new s32 value.
  * @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).
@@ -260,6 +271,7 @@ struct v4l2_ctrl {
 	unsigned int is_string:1;
 	unsigned int is_ptr:1;
 	unsigned int is_array:1;
+	unsigned int is_dyn_array:1;
 	unsigned int has_volatiles:1;
 	unsigned int call_notify:1;
 	unsigned int manual_mode_value:8;
@@ -272,6 +284,7 @@ struct v4l2_ctrl {
 	s64 minimum, maximum, default_value;
 	u32 elems;
 	u32 elem_size;
+	u32 new_elems;
 	u32 dims[V4L2_CTRL_MAX_DIMS];
 	u32 nr_of_dims;
 	union {
@@ -284,6 +297,8 @@ struct v4l2_ctrl {
 	};
 	unsigned long flags;
 	void *priv;
+	void *p_dyn;
+	u32 p_dyn_alloc_elems;
 	s32 val;
 	struct {
 		s32 val;
@@ -309,12 +324,22 @@ struct v4l2_ctrl {
  *		the control has been applied. This prevents applying controls
  *		from a cluster with multiple controls twice (when the first
  *		control of a cluster is applied, they all are).
- * @valid_p_req: If set, then p_req contains the control value for the request.
+ * @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
+ *		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
+ *		@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
  *		value of the control that must be applied when the request
  *		is executed, or to the value of the control at the time
- *		that the request was completed. If @valid_p_req is false,
+ *		that the request was completed. If @p_req_valid is false,
  *		then this control was never set for this request and the
  *		control will not be updated when this request is applied.
  *
@@ -329,7 +354,10 @@ struct v4l2_ctrl_ref {
 	struct v4l2_ctrl_helper *helper;
 	bool from_other_dev;
 	bool req_done;
-	bool valid_p_req;
+	bool p_req_valid;
+	bool p_req_dyn_enomem;
+	u32 p_req_dyn_alloc_elems;
+	u32 p_req_elems;
 	union v4l2_ctrl_ptr p_req;
 };
 
-- 
2.35.1

[PATCH 3/8] vivid: add dynamic array test control

[Hans Verkuil]

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>
---
 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);
 
-- 
2.35.1

Re: [PATCH 3/8] vivid: add dynamic array test control

[Laurent Pinchart]

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);
>  

-- 
Regards,

Laurent Pinchart

[PATCH 4/8] v4l2-ctrls: allocate space for arrays

[Hans Verkuil]

Just like dynamic arrays, also allocate space for regular arrays.

This is in preparation for allowing to change the array size from
a driver.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/v4l2-core/v4l2-ctrls-api.c  |  8 +++---
 drivers/media/v4l2-core/v4l2-ctrls-core.c | 33 +++++++++++------------
 include/media/v4l2-ctrls.h                | 17 ++++++------
 3 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
index 50d012ba3c02..1b90bd7c4010 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
@@ -105,8 +105,8 @@ static int user_to_new(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
 
 	ctrl->is_new = 0;
 	if (ctrl->is_dyn_array &&
-	    c->size > ctrl->p_dyn_alloc_elems * ctrl->elem_size) {
-		void *old = ctrl->p_dyn;
+	    c->size > ctrl->p_array_alloc_elems * ctrl->elem_size) {
+		void *old = ctrl->p_array;
 		void *tmp = kvzalloc(2 * c->size, GFP_KERNEL);
 
 		if (!tmp)
@@ -115,8 +115,8 @@ static int user_to_new(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
 		memcpy(tmp + c->size, ctrl->p_cur.p, ctrl->elems * ctrl->elem_size);
 		ctrl->p_new.p = tmp;
 		ctrl->p_cur.p = tmp + c->size;
-		ctrl->p_dyn = tmp;
-		ctrl->p_dyn_alloc_elems = c->size / ctrl->elem_size;
+		ctrl->p_array = tmp;
+		ctrl->p_array_alloc_elems = c->size / ctrl->elem_size;
 		kvfree(old);
 	}
 
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
index ff8a61f24d0a..1372b7b45681 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
@@ -1135,14 +1135,14 @@ int req_to_new(struct v4l2_ctrl_ref *ref)
 
 	/*
 	 * Check if the number of elements in the request is more than the
-	 * elements in ctrl->p_dyn. If so, attempt to realloc ctrl->p_dyn.
-	 * Note that p_dyn is allocated with twice the number of elements
+	 * elements in ctrl->p_array. If so, attempt to realloc ctrl->p_array.
+	 * Note that p_array is allocated with twice the number of elements
 	 * in the dynamic array since it has to store both the current and
 	 * new value of such a control.
 	 */
-	if (ref->p_req_elems > ctrl->p_dyn_alloc_elems) {
+	if (ref->p_req_elems > ctrl->p_array_alloc_elems) {
 		unsigned int sz = ref->p_req_elems * ctrl->elem_size;
-		void *old = ctrl->p_dyn;
+		void *old = ctrl->p_array;
 		void *tmp = kvzalloc(2 * sz, GFP_KERNEL);
 
 		if (!tmp)
@@ -1151,8 +1151,8 @@ int req_to_new(struct v4l2_ctrl_ref *ref)
 		memcpy(tmp + sz, ctrl->p_cur.p, ctrl->elems * ctrl->elem_size);
 		ctrl->p_new.p = tmp;
 		ctrl->p_cur.p = tmp + sz;
-		ctrl->p_dyn = tmp;
-		ctrl->p_dyn_alloc_elems = ref->p_req_elems;
+		ctrl->p_array = tmp;
+		ctrl->p_array_alloc_elems = ref->p_req_elems;
 		kvfree(old);
 	}
 
@@ -1252,7 +1252,7 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
 		list_del(&ctrl->node);
 		list_for_each_entry_safe(sev, next_sev, &ctrl->ev_subs, node)
 			list_del(&sev->node);
-		kvfree(ctrl->p_dyn);
+		kvfree(ctrl->p_array);
 		kvfree(ctrl);
 	}
 	kvfree(hdl->buckets);
@@ -1584,11 +1584,10 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 			V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
 	else if (type == V4L2_CTRL_TYPE_CTRL_CLASS)
 		flags |= V4L2_CTRL_FLAG_READ_ONLY;
-	else if (!(flags & V4L2_CTRL_FLAG_DYNAMIC_ARRAY) &&
+	else if (!is_array &&
 		 (type == V4L2_CTRL_TYPE_INTEGER64 ||
 		  type == V4L2_CTRL_TYPE_STRING ||
-		  type >= V4L2_CTRL_COMPOUND_TYPES ||
-		  is_array))
+		  type >= V4L2_CTRL_COMPOUND_TYPES))
 		sz_extra += 2 * tot_ctrl_size;
 
 	if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p_const)
@@ -1632,14 +1631,14 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	ctrl->cur.val = ctrl->val = def;
 	data = &ctrl[1];
 
-	if (ctrl->is_dyn_array) {
-		ctrl->p_dyn_alloc_elems = elems;
-		ctrl->p_dyn = kvzalloc(2 * elems * elem_size, GFP_KERNEL);
-		if (!ctrl->p_dyn) {
+	if (ctrl->is_array) {
+		ctrl->p_array_alloc_elems = elems;
+		ctrl->p_array = kvzalloc(2 * elems * elem_size, GFP_KERNEL);
+		if (!ctrl->p_array) {
 			kvfree(ctrl);
 			return NULL;
 		}
-		data = ctrl->p_dyn;
+		data = ctrl->p_array;
 	}
 
 	if (!ctrl->is_int) {
@@ -1651,7 +1650,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	}
 
 	if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p_const) {
-		if (ctrl->is_dyn_array)
+		if (ctrl->is_array)
 			ctrl->p_def.p = &ctrl[1];
 		else
 			ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size;
@@ -1664,7 +1663,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	}
 
 	if (handler_new_ref(hdl, ctrl, NULL, false, false)) {
-		kvfree(ctrl->p_dyn);
+		kvfree(ctrl->p_array);
 		kvfree(ctrl);
 		return NULL;
 	}
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index f4105de8a8d2..a2f147873265 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -209,7 +209,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
  * @elem_size:	The size in bytes of the control.
  * @new_elems:	The number of elements in p_new. This is the same as @elems,
  *		except for dynamic arrays. In that case it is in the range of
- *		1 to @p_dyn_alloc_elems.
+ *		1 to @p_array_alloc_elems.
  * @dims:	The size of each dimension.
  * @nr_of_dims:The number of dimensions in @dims.
  * @menu_skip_mask: The control's skip mask for menu controls. This makes it
@@ -233,12 +233,11 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
  *		not freed when the control is deleted. Should this be needed
  *		then a new internal bitfield can be added to tell the framework
  *		to free this pointer.
- * @p_dyn:	Pointer to the dynamically allocated array. Only valid if
- *		@is_dyn_array is true.
- * @p_dyn_alloc_elems: The number of elements in the dynamically allocated
- *		array for both the cur and new values. So @p_dyn is actually
- *		sized for 2 * @p_dyn_alloc_elems * @elem_size. Only valid if
- *		@is_dyn_array is true.
+ * @p_array:	Pointer to the allocated array. Only valid if @is_array is true.
+ * @p_array_alloc_elems: The number of elements in the allocated
+ *		array for both the cur and new values. So @p_array is actually
+ *		sized for 2 * @p_array_alloc_elems * @elem_size. Only valid if
+ *		@is_array is true.
  * @cur:	Structure to store the current value.
  * @cur.val:	The control's current value, if the @type is represented via
  *		a u32 integer (see &enum v4l2_ctrl_type).
@@ -297,8 +296,8 @@ struct v4l2_ctrl {
 	};
 	unsigned long flags;
 	void *priv;
-	void *p_dyn;
-	u32 p_dyn_alloc_elems;
+	void *p_array;
+	u32 p_array_alloc_elems;
 	s32 val;
 	struct {
 		s32 val;
-- 
2.35.1

Re: [PATCH 4/8] v4l2-ctrls: allocate space for arrays

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Tue, Jun 28, 2022 at 02:05:19PM +0200, Hans Verkuil wrote:
> Just like dynamic arrays, also allocate space for regular arrays.
> 
> 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  |  8 +++---
>  drivers/media/v4l2-core/v4l2-ctrls-core.c | 33 +++++++++++------------
>  include/media/v4l2-ctrls.h                | 17 ++++++------
>  3 files changed, 28 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> index 50d012ba3c02..1b90bd7c4010 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> @@ -105,8 +105,8 @@ static int user_to_new(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
>  
>  	ctrl->is_new = 0;
>  	if (ctrl->is_dyn_array &&
> -	    c->size > ctrl->p_dyn_alloc_elems * ctrl->elem_size) {
> -		void *old = ctrl->p_dyn;
> +	    c->size > ctrl->p_array_alloc_elems * ctrl->elem_size) {
> +		void *old = ctrl->p_array;
>  		void *tmp = kvzalloc(2 * c->size, GFP_KERNEL);
>  
>  		if (!tmp)
> @@ -115,8 +115,8 @@ static int user_to_new(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
>  		memcpy(tmp + c->size, ctrl->p_cur.p, ctrl->elems * ctrl->elem_size);
>  		ctrl->p_new.p = tmp;
>  		ctrl->p_cur.p = tmp + c->size;
> -		ctrl->p_dyn = tmp;
> -		ctrl->p_dyn_alloc_elems = c->size / ctrl->elem_size;
> +		ctrl->p_array = tmp;
> +		ctrl->p_array_alloc_elems = c->size / ctrl->elem_size;
>  		kvfree(old);
>  	}
>  
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index ff8a61f24d0a..1372b7b45681 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> @@ -1135,14 +1135,14 @@ int req_to_new(struct v4l2_ctrl_ref *ref)
>  
>  	/*
>  	 * Check if the number of elements in the request is more than the
> -	 * elements in ctrl->p_dyn. If so, attempt to realloc ctrl->p_dyn.
> -	 * Note that p_dyn is allocated with twice the number of elements
> +	 * elements in ctrl->p_array. If so, attempt to realloc ctrl->p_array.
> +	 * Note that p_array is allocated with twice the number of elements
>  	 * in the dynamic array since it has to store both the current and
>  	 * new value of such a control.
>  	 */
> -	if (ref->p_req_elems > ctrl->p_dyn_alloc_elems) {
> +	if (ref->p_req_elems > ctrl->p_array_alloc_elems) {
>  		unsigned int sz = ref->p_req_elems * ctrl->elem_size;
> -		void *old = ctrl->p_dyn;
> +		void *old = ctrl->p_array;
>  		void *tmp = kvzalloc(2 * sz, GFP_KERNEL);
>  
>  		if (!tmp)
> @@ -1151,8 +1151,8 @@ int req_to_new(struct v4l2_ctrl_ref *ref)
>  		memcpy(tmp + sz, ctrl->p_cur.p, ctrl->elems * ctrl->elem_size);
>  		ctrl->p_new.p = tmp;
>  		ctrl->p_cur.p = tmp + sz;
> -		ctrl->p_dyn = tmp;
> -		ctrl->p_dyn_alloc_elems = ref->p_req_elems;
> +		ctrl->p_array = tmp;
> +		ctrl->p_array_alloc_elems = ref->p_req_elems;
>  		kvfree(old);
>  	}
>  
> @@ -1252,7 +1252,7 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
>  		list_del(&ctrl->node);
>  		list_for_each_entry_safe(sev, next_sev, &ctrl->ev_subs, node)
>  			list_del(&sev->node);
> -		kvfree(ctrl->p_dyn);
> +		kvfree(ctrl->p_array);
>  		kvfree(ctrl);
>  	}
>  	kvfree(hdl->buckets);
> @@ -1584,11 +1584,10 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  			V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
>  	else if (type == V4L2_CTRL_TYPE_CTRL_CLASS)
>  		flags |= V4L2_CTRL_FLAG_READ_ONLY;
> -	else if (!(flags & V4L2_CTRL_FLAG_DYNAMIC_ARRAY) &&
> +	else if (!is_array &&
>  		 (type == V4L2_CTRL_TYPE_INTEGER64 ||
>  		  type == V4L2_CTRL_TYPE_STRING ||
> -		  type >= V4L2_CTRL_COMPOUND_TYPES ||
> -		  is_array))
> +		  type >= V4L2_CTRL_COMPOUND_TYPES))
>  		sz_extra += 2 * tot_ctrl_size;
>  
>  	if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p_const)
> @@ -1632,14 +1631,14 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  	ctrl->cur.val = ctrl->val = def;
>  	data = &ctrl[1];
>  
> -	if (ctrl->is_dyn_array) {
> -		ctrl->p_dyn_alloc_elems = elems;
> -		ctrl->p_dyn = kvzalloc(2 * elems * elem_size, GFP_KERNEL);
> -		if (!ctrl->p_dyn) {
> +	if (ctrl->is_array) {
> +		ctrl->p_array_alloc_elems = elems;
> +		ctrl->p_array = kvzalloc(2 * elems * elem_size, GFP_KERNEL);
> +		if (!ctrl->p_array) {
>  			kvfree(ctrl);
>  			return NULL;
>  		}
> -		data = ctrl->p_dyn;
> +		data = ctrl->p_array;
>  	}
>  
>  	if (!ctrl->is_int) {
> @@ -1651,7 +1650,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  	}
>  
>  	if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p_const) {
> -		if (ctrl->is_dyn_array)
> +		if (ctrl->is_array)
>  			ctrl->p_def.p = &ctrl[1];
>  		else
>  			ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size;
> @@ -1664,7 +1663,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  	}
>  
>  	if (handler_new_ref(hdl, ctrl, NULL, false, false)) {
> -		kvfree(ctrl->p_dyn);
> +		kvfree(ctrl->p_array);
>  		kvfree(ctrl);
>  		return NULL;
>  	}
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index f4105de8a8d2..a2f147873265 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -209,7 +209,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>   * @elem_size:	The size in bytes of the control.
>   * @new_elems:	The number of elements in p_new. This is the same as @elems,
>   *		except for dynamic arrays. In that case it is in the range of
> - *		1 to @p_dyn_alloc_elems.
> + *		1 to @p_array_alloc_elems.
>   * @dims:	The size of each dimension.
>   * @nr_of_dims:The number of dimensions in @dims.
>   * @menu_skip_mask: The control's skip mask for menu controls. This makes it
> @@ -233,12 +233,11 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>   *		not freed when the control is deleted. Should this be needed
>   *		then a new internal bitfield can be added to tell the framework
>   *		to free this pointer.
> - * @p_dyn:	Pointer to the dynamically allocated array. Only valid if
> - *		@is_dyn_array is true.
> - * @p_dyn_alloc_elems: The number of elements in the dynamically allocated
> - *		array for both the cur and new values. So @p_dyn is actually
> - *		sized for 2 * @p_dyn_alloc_elems * @elem_size. Only valid if
> - *		@is_dyn_array is true.
> + * @p_array:	Pointer to the allocated array. Only valid if @is_array is true.
> + * @p_array_alloc_elems: The number of elements in the allocated
> + *		array for both the cur and new values. So @p_array is actually
> + *		sized for 2 * @p_array_alloc_elems * @elem_size. Only valid if
> + *		@is_array is true.
>   * @cur:	Structure to store the current value.
>   * @cur.val:	The control's current value, if the @type is represented via
>   *		a u32 integer (see &enum v4l2_ctrl_type).
> @@ -297,8 +296,8 @@ struct v4l2_ctrl {
>  	};
>  	unsigned long flags;
>  	void *priv;
> -	void *p_dyn;
> -	u32 p_dyn_alloc_elems;
> +	void *p_array;
> +	u32 p_array_alloc_elems;
>  	s32 val;
>  	struct {
>  		s32 val;

-- 
Regards,

Laurent Pinchart

[PATCH 5/8] v4l2-ctrls: alloc arrays in ctrl_ref

[Hans Verkuil]

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>
---
 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;
 };
-- 
2.35.1

Re: [PATCH 5/8] v4l2-ctrls: alloc arrays in ctrl_ref

[Laurent Pinchart]

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;
>  };

-- 
Regards,

Laurent Pinchart

[PATCH 6/8] v4l2-ctrls: add v4l2_ctrl_modify_dimensions

[Hans Verkuil]

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];
+	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.
  *
-- 
2.35.1

Re: [PATCH 6/8] v4l2-ctrls: add v4l2_ctrl_modify_dimensions

[Laurent Pinchart]

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.
>   *

-- 
Regards,

Laurent Pinchart

[PATCH 7/8] v4l2-ctrls: add change flag for when dimensions change

[Hans Verkuil]

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.

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);
 	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;
-- 
2.35.1

Re: [PATCH 7/8] v4l2-ctrls: add change flag for when dimensions change

[Laurent Pinchart]

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;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/8] v4l2-ctrls: add change flag for when dimensions change

[Hans Verkuil]

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;
> 

Re: [PATCH 7/8] v4l2-ctrls: add change flag for when dimensions change

[Laurent Pinchart]

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;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/8] v4l2-ctrls: add change flag for when dimensions change

[Hans Verkuil]

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;
> 

Re: [PATCH 7/8] v4l2-ctrls: add change flag for when dimensions change

[Laurent Pinchart]

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;

-- 
Regards,

Laurent Pinchart

[PATCH 8/8] vivid: add pixel_array test control

[Hans Verkuil]

This control will change dimensions according to the source resolution.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/test-drivers/vivid/vivid-core.h    |  1 +
 drivers/media/test-drivers/vivid/vivid-ctrls.c   | 14 ++++++++++++++
 drivers/media/test-drivers/vivid/vivid-vid-cap.c |  4 ++++
 3 files changed, 19 insertions(+)

diff --git a/drivers/media/test-drivers/vivid/vivid-core.h b/drivers/media/test-drivers/vivid/vivid-core.h
index 176b72cb143b..e7b23ebc705e 100644
--- a/drivers/media/test-drivers/vivid/vivid-core.h
+++ b/drivers/media/test-drivers/vivid/vivid-core.h
@@ -227,6 +227,7 @@ struct vivid_dev {
 	struct v4l2_ctrl		*bitmask;
 	struct v4l2_ctrl		*int_menu;
 	struct v4l2_ctrl		*ro_int32;
+	struct v4l2_ctrl		*pixel_array;
 	struct v4l2_ctrl		*test_pattern;
 	struct v4l2_ctrl		*colorspace;
 	struct v4l2_ctrl		*rgb_range_cap;
diff --git a/drivers/media/test-drivers/vivid/vivid-ctrls.c b/drivers/media/test-drivers/vivid/vivid-ctrls.c
index a78d676575bc..f98a651842ce 100644
--- a/drivers/media/test-drivers/vivid/vivid-ctrls.c
+++ b/drivers/media/test-drivers/vivid/vivid-ctrls.c
@@ -35,6 +35,7 @@
 #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_U8_PIXEL_ARRAY	(VIVID_CID_CUSTOM_BASE + 14)
 
 #define VIVID_CID_VIVID_BASE		(0x00f00000 | 0xf000)
 #define VIVID_CID_VIVID_CLASS		(0x00f00000 | 1)
@@ -228,6 +229,18 @@ static const struct v4l2_ctrl_config vivid_ctrl_u8_4d_array = {
 	.dims = { 2, 3, 4, 5 },
 };
 
+static const struct v4l2_ctrl_config vivid_ctrl_u8_pixel_array = {
+	.ops = &vivid_user_gen_ctrl_ops,
+	.id = VIVID_CID_U8_PIXEL_ARRAY,
+	.name = "U8 Pixel Array",
+	.type = V4L2_CTRL_TYPE_U8,
+	.def = 0x80,
+	.min = 0x00,
+	.max = 0xff,
+	.step = 1,
+	.dims = { 640, 360 },
+};
+
 static const char * const vivid_ctrl_menu_strings[] = {
 	"Menu Item 0 (Skipped)",
 	"Menu Item 1",
@@ -1642,6 +1655,7 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
 	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);
+	dev->pixel_array = v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u8_pixel_array, NULL);
 
 	if (dev->has_vid_cap) {
 		/* Image Processing Controls */
diff --git a/drivers/media/test-drivers/vivid/vivid-vid-cap.c b/drivers/media/test-drivers/vivid/vivid-vid-cap.c
index b9caa4b26209..6dc4091fcabb 100644
--- a/drivers/media/test-drivers/vivid/vivid-vid-cap.c
+++ b/drivers/media/test-drivers/vivid/vivid-vid-cap.c
@@ -381,6 +381,7 @@ static enum tpg_pixel_aspect vivid_get_pixel_aspect(const struct vivid_dev *dev)
 void vivid_update_format_cap(struct vivid_dev *dev, bool keep_controls)
 {
 	struct v4l2_bt_timings *bt = &dev->dv_timings_cap[dev->input].bt;
+	u32 dims[V4L2_CTRL_MAX_DIMS] = {};
 	unsigned size;
 	u64 pixelclock;
 
@@ -459,6 +460,9 @@ void vivid_update_format_cap(struct vivid_dev *dev, bool keep_controls)
 	tpg_s_video_aspect(&dev->tpg, vivid_get_video_aspect(dev));
 	tpg_s_pixel_aspect(&dev->tpg, vivid_get_pixel_aspect(dev));
 	tpg_update_mv_step(&dev->tpg);
+	dims[0] = dev->src_rect.width;
+	dims[1] = dev->src_rect.height;
+	v4l2_ctrl_modify_dimensions(dev->pixel_array, dims);
 }
 
 /* Map the field to something that is valid for the current input */
-- 
2.35.1

Re: [PATCH 8/8] vivid: add pixel_array test control

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Tue, Jun 28, 2022 at 02:05:23PM +0200, Hans Verkuil wrote:
> This control will change dimensions according to the source resolution.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/test-drivers/vivid/vivid-core.h    |  1 +
>  drivers/media/test-drivers/vivid/vivid-ctrls.c   | 14 ++++++++++++++
>  drivers/media/test-drivers/vivid/vivid-vid-cap.c |  4 ++++
>  3 files changed, 19 insertions(+)
> 
> diff --git a/drivers/media/test-drivers/vivid/vivid-core.h b/drivers/media/test-drivers/vivid/vivid-core.h
> index 176b72cb143b..e7b23ebc705e 100644
> --- a/drivers/media/test-drivers/vivid/vivid-core.h
> +++ b/drivers/media/test-drivers/vivid/vivid-core.h
> @@ -227,6 +227,7 @@ struct vivid_dev {
>  	struct v4l2_ctrl		*bitmask;
>  	struct v4l2_ctrl		*int_menu;
>  	struct v4l2_ctrl		*ro_int32;
> +	struct v4l2_ctrl		*pixel_array;
>  	struct v4l2_ctrl		*test_pattern;
>  	struct v4l2_ctrl		*colorspace;
>  	struct v4l2_ctrl		*rgb_range_cap;
> diff --git a/drivers/media/test-drivers/vivid/vivid-ctrls.c b/drivers/media/test-drivers/vivid/vivid-ctrls.c
> index a78d676575bc..f98a651842ce 100644
> --- a/drivers/media/test-drivers/vivid/vivid-ctrls.c
> +++ b/drivers/media/test-drivers/vivid/vivid-ctrls.c
> @@ -35,6 +35,7 @@
>  #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_U8_PIXEL_ARRAY	(VIVID_CID_CUSTOM_BASE + 14)
>  
>  #define VIVID_CID_VIVID_BASE		(0x00f00000 | 0xf000)
>  #define VIVID_CID_VIVID_CLASS		(0x00f00000 | 1)
> @@ -228,6 +229,18 @@ static const struct v4l2_ctrl_config vivid_ctrl_u8_4d_array = {
>  	.dims = { 2, 3, 4, 5 },
>  };
>  
> +static const struct v4l2_ctrl_config vivid_ctrl_u8_pixel_array = {
> +	.ops = &vivid_user_gen_ctrl_ops,
> +	.id = VIVID_CID_U8_PIXEL_ARRAY,
> +	.name = "U8 Pixel Array",
> +	.type = V4L2_CTRL_TYPE_U8,
> +	.def = 0x80,
> +	.min = 0x00,
> +	.max = 0xff,
> +	.step = 1,
> +	.dims = { 640, 360 },
> +};
> +
>  static const char * const vivid_ctrl_menu_strings[] = {
>  	"Menu Item 0 (Skipped)",
>  	"Menu Item 1",
> @@ -1642,6 +1655,7 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
>  	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);
> +	dev->pixel_array = v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u8_pixel_array, NULL);
>  
>  	if (dev->has_vid_cap) {
>  		/* Image Processing Controls */
> diff --git a/drivers/media/test-drivers/vivid/vivid-vid-cap.c b/drivers/media/test-drivers/vivid/vivid-vid-cap.c
> index b9caa4b26209..6dc4091fcabb 100644
> --- a/drivers/media/test-drivers/vivid/vivid-vid-cap.c
> +++ b/drivers/media/test-drivers/vivid/vivid-vid-cap.c
> @@ -381,6 +381,7 @@ static enum tpg_pixel_aspect vivid_get_pixel_aspect(const struct vivid_dev *dev)
>  void vivid_update_format_cap(struct vivid_dev *dev, bool keep_controls)
>  {
>  	struct v4l2_bt_timings *bt = &dev->dv_timings_cap[dev->input].bt;
> +	u32 dims[V4L2_CTRL_MAX_DIMS] = {};
>  	unsigned size;
>  	u64 pixelclock;
>  
> @@ -459,6 +460,9 @@ void vivid_update_format_cap(struct vivid_dev *dev, bool keep_controls)
>  	tpg_s_video_aspect(&dev->tpg, vivid_get_video_aspect(dev));
>  	tpg_s_pixel_aspect(&dev->tpg, vivid_get_pixel_aspect(dev));
>  	tpg_update_mv_step(&dev->tpg);
> +	dims[0] = dev->src_rect.width;
> +	dims[1] = dev->src_rect.height;
> +	v4l2_ctrl_modify_dimensions(dev->pixel_array, dims);

The implementation looks fine, but calling the init function (twice) on
each element will make vivid_update_format_cap() pretty slow. How about
going for a downsampled resolution here ?

Also, this made me wonder if v4l2_ctrl_modify_dimensions() should return
without doing anything if the new dimensions are identical to the
current ones.

>  }
>  
>  /* Map the field to something that is valid for the current input */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 8/8] vivid: add pixel_array test control

[Hans Verkuil]

On 7/8/22 12:43, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Tue, Jun 28, 2022 at 02:05:23PM +0200, Hans Verkuil wrote:
>> This control will change dimensions according to the source resolution.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/media/test-drivers/vivid/vivid-core.h    |  1 +
>>  drivers/media/test-drivers/vivid/vivid-ctrls.c   | 14 ++++++++++++++
>>  drivers/media/test-drivers/vivid/vivid-vid-cap.c |  4 ++++
>>  3 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/media/test-drivers/vivid/vivid-core.h b/drivers/media/test-drivers/vivid/vivid-core.h
>> index 176b72cb143b..e7b23ebc705e 100644
>> --- a/drivers/media/test-drivers/vivid/vivid-core.h
>> +++ b/drivers/media/test-drivers/vivid/vivid-core.h
>> @@ -227,6 +227,7 @@ struct vivid_dev {
>>  	struct v4l2_ctrl		*bitmask;
>>  	struct v4l2_ctrl		*int_menu;
>>  	struct v4l2_ctrl		*ro_int32;
>> +	struct v4l2_ctrl		*pixel_array;
>>  	struct v4l2_ctrl		*test_pattern;
>>  	struct v4l2_ctrl		*colorspace;
>>  	struct v4l2_ctrl		*rgb_range_cap;
>> diff --git a/drivers/media/test-drivers/vivid/vivid-ctrls.c b/drivers/media/test-drivers/vivid/vivid-ctrls.c
>> index a78d676575bc..f98a651842ce 100644
>> --- a/drivers/media/test-drivers/vivid/vivid-ctrls.c
>> +++ b/drivers/media/test-drivers/vivid/vivid-ctrls.c
>> @@ -35,6 +35,7 @@
>>  #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_U8_PIXEL_ARRAY	(VIVID_CID_CUSTOM_BASE + 14)
>>  
>>  #define VIVID_CID_VIVID_BASE		(0x00f00000 | 0xf000)
>>  #define VIVID_CID_VIVID_CLASS		(0x00f00000 | 1)
>> @@ -228,6 +229,18 @@ static const struct v4l2_ctrl_config vivid_ctrl_u8_4d_array = {
>>  	.dims = { 2, 3, 4, 5 },
>>  };
>>  
>> +static const struct v4l2_ctrl_config vivid_ctrl_u8_pixel_array = {
>> +	.ops = &vivid_user_gen_ctrl_ops,
>> +	.id = VIVID_CID_U8_PIXEL_ARRAY,
>> +	.name = "U8 Pixel Array",
>> +	.type = V4L2_CTRL_TYPE_U8,
>> +	.def = 0x80,
>> +	.min = 0x00,
>> +	.max = 0xff,
>> +	.step = 1,
>> +	.dims = { 640, 360 },
>> +};
>> +
>>  static const char * const vivid_ctrl_menu_strings[] = {
>>  	"Menu Item 0 (Skipped)",
>>  	"Menu Item 1",
>> @@ -1642,6 +1655,7 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
>>  	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);
>> +	dev->pixel_array = v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u8_pixel_array, NULL);
>>  
>>  	if (dev->has_vid_cap) {
>>  		/* Image Processing Controls */
>> diff --git a/drivers/media/test-drivers/vivid/vivid-vid-cap.c b/drivers/media/test-drivers/vivid/vivid-vid-cap.c
>> index b9caa4b26209..6dc4091fcabb 100644
>> --- a/drivers/media/test-drivers/vivid/vivid-vid-cap.c
>> +++ b/drivers/media/test-drivers/vivid/vivid-vid-cap.c
>> @@ -381,6 +381,7 @@ static enum tpg_pixel_aspect vivid_get_pixel_aspect(const struct vivid_dev *dev)
>>  void vivid_update_format_cap(struct vivid_dev *dev, bool keep_controls)
>>  {
>>  	struct v4l2_bt_timings *bt = &dev->dv_timings_cap[dev->input].bt;
>> +	u32 dims[V4L2_CTRL_MAX_DIMS] = {};
>>  	unsigned size;
>>  	u64 pixelclock;
>>  
>> @@ -459,6 +460,9 @@ void vivid_update_format_cap(struct vivid_dev *dev, bool keep_controls)
>>  	tpg_s_video_aspect(&dev->tpg, vivid_get_video_aspect(dev));
>>  	tpg_s_pixel_aspect(&dev->tpg, vivid_get_pixel_aspect(dev));
>>  	tpg_update_mv_step(&dev->tpg);
>> +	dims[0] = dev->src_rect.width;
>> +	dims[1] = dev->src_rect.height;
>> +	v4l2_ctrl_modify_dimensions(dev->pixel_array, dims);
> 
> The implementation looks fine, but calling the init function (twice) on
> each element will make vivid_update_format_cap() pretty slow. How about
> going for a downsampled resolution here ?

Good point.

> 
> Also, this made me wonder if v4l2_ctrl_modify_dimensions() should return
> without doing anything if the new dimensions are identical to the
> current ones.

I think this should be a driver decision: if it calls v4l2_ctrl_modify_dimensions()
then I think it should expect that function to reset the array to default values.

If it doesn't want to do that if the dimensions are unchanged, then it can
test for that and simply not call this function.

Regards,

	Hans

> 
>>  }
>>  
>>  /* Map the field to something that is valid for the current input */
> 

Re: [PATCH 0/8] Add dynamic arrays and v4l2_ctrl_modify_dimensions

[Laurent Pinchart]

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(-)

-- 
Regards,

Laurent Pinchart