[RFC PATCHv2 00/11] Dynamic arrays and read-only requests

[RFC PATCHv2 00/11] Dynamic arrays and read-only requests

[Hans Verkuil]

This RFC series adds support for dynamic arrays and for read-only
requests. This respins and combines these older series:

Adding dynamic array controls:

https://patchwork.linuxtv.org/project/linux-media/cover/20210428101841.696059-1-hverkuil-cisco@xs4all.nl/

Adding support for read-only requests:

https://patchwork.linuxtv.org/project/linux-media/cover/20200818143719.102128-1-hverkuil-cisco@xs4all.nl/

The dynamic array support (patches 1-3) is pretty much the same as the
original series, just rebased.

The read-only request support (patches 4-11) is greatly simplified compared
to the original due to this patch that was merged recently:

https://patchwork.linuxtv.org/project/linux-media/patch/20210428101841.696059-2-hverkuil-cisco@xs4all.nl/

This made the read-only request implementation a lot easier.

Patches 1-3 and 4-11 are independent of one another.

The only reason this series is marked RFC is that it needs some
documentation improvements (or at the least, verification that
everything is properly documented), and it needs a real driver
that implements the feature.

I have CC-ed those who I know are/have been working on either
dynamic arrays or read-only requests, so if you want to get this
into the kernel, then submit patches for a driver, and include the
support from this RFC series.

I'll post a short series next for v4l-utils, adding support for
these two features.

Regards,

	Hans

Hans Verkuil (11):
  videodev2.h: add V4L2_CTRL_FLAG_DYNAMIC_ARRAY
  v4l2-ctrls: add support for dynamically allocated arrays.
  vivid: add dynamic array test control
  media: document read-only requests
  videodev2.h: add V4L2_BUF_CAP_SUPPORTS_RO_REQUESTS
  videobuf2-core: add vb2_request_buffer_first()
  videobuf2-v4l2.c: vb2_request_validate: support RO requests
  v4l2-mem2mem.c: allow requests for capture queues
  vivid: add ro_requests module option
  vim2m: support read-only requests on the capture queue
  vicodec: add support for read-only requests

 Documentation/admin-guide/media/vivid.rst     |  10 +
 .../mediactl/media-request-ioc-queue.rst      |   5 +
 .../media/mediactl/request-api.rst            |  11 ++
 .../media/v4l/vidioc-queryctrl.rst            |   8 +
 .../media/v4l/vidioc-reqbufs.rst              |   6 +
 .../media/common/videobuf2/videobuf2-core.c   |  22 +++
 .../media/common/videobuf2/videobuf2-v4l2.c   |  23 ++-
 .../media/test-drivers/vicodec/vicodec-core.c |  67 +++----
 drivers/media/test-drivers/vim2m.c            |  10 +-
 drivers/media/test-drivers/vivid/vivid-core.c |  10 +
 .../media/test-drivers/vivid/vivid-ctrls.c    |  15 ++
 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 +-
 drivers/media/v4l2-core/v4l2-mem2mem.c        |  16 +-
 include/media/v4l2-ctrls.h                    |  42 +++-
 include/media/videobuf2-core.h                |   8 +
 include/uapi/linux/videodev2.h                |   2 +
 19 files changed, 440 insertions(+), 116 deletions(-)

-- 
2.30.2

[RFC PATCHv2 01/11] 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>
---
 .../userspace-api/media/v4l/vidioc-queryctrl.rst          | 8 ++++++++
 include/uapi/linux/videodev2.h                            | 1 +
 2 files changed, 9 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
index 07e54029e1e9..a1a0dbff4a6f 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
@@ -601,6 +601,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/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 9260791b8438..7222fc855d6b 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1869,6 +1869,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.30.2

Re: [RFC PATCHv2 01/11] videodev2.h: add V4L2_CTRL_FLAG_DYNAMIC_ARRAY

[Nicolas Dufresne]

Hi Hans,

Le jeudi 10 juin 2021 à 13:36 +0200, Hans Verkuil a écrit :
> 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>
> ---
>  .../userspace-api/media/v4l/vidioc-queryctrl.rst          | 8 ++++++++
>  include/uapi/linux/videodev2.h                            | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> index 07e54029e1e9..a1a0dbff4a6f 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> @@ -601,6 +601,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

The minimum of 1 item is indeed logical for slices (or tiles in AV1), but I
think we should remove this limitation so it won't get in the way in other
applications. There is otherwise no way (other then creating a "invalid entry"
value) to signal that the array is now empty. The driver could erroneously endup
using ancient values.

Perhaps this can be waved later when we need it ? Would that be backward
compatible ?

> +	array will change the ``elems`` field when the control is
> +	queried afterwards.
>  
>  Return Value
>  ============
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 9260791b8438..7222fc855d6b 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1869,6 +1869,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

[RFC PATCHv2 02/11] 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>
---
 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 081439224357..8bd19b6c7ad0 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
@@ -792,11 +792,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. */
@@ -809,8 +810,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. */
@@ -840,36 +844,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 */
@@ -911,17 +1001,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)
 {
@@ -966,6 +1045,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 */
@@ -973,6 +1054,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);
@@ -1088,7 +1170,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)
@@ -1250,7 +1332,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 ||
@@ -1271,17 +1352,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)
@@ -1310,7 +1407,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]));
@@ -1323,6 +1422,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;
@@ -1332,7 +1441,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);
 	}
 
@@ -1342,6 +1454,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;
 	}
@@ -1679,6 +1792,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 575b59fbac77..ebd9cef13309 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -181,6 +181,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
@@ -201,6 +203,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
@@ -219,15 +224,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).
@@ -256,6 +267,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;
@@ -268,6 +280,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 {
@@ -280,6 +293,8 @@ struct v4l2_ctrl {
 	};
 	unsigned long flags;
 	void *priv;
+	void *p_dyn;
+	u32 p_dyn_alloc_elems;
 	s32 val;
 	struct {
 		s32 val;
@@ -305,12 +320,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.
  *
@@ -325,7 +350,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.30.2

Re: [RFC PATCHv2 02/11] v4l2-ctrls: add support for dynamically allocated arrays.

[Daniel Almeida]

Hi Hans!

I have been working on testing this lately as you know.

 From the example you've added to vivid, i.e.:

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

+	v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_dyn_array, NULL);


I was under the impression that it'd be enough to pass the id and 
V4L2_CTRL_FLAG_DYNAMIC_ARRAY in the config before calling 
v4l2_ctrl_new_custom. Apparently that's not the case though, because 
v4l2_ctrl_fill will then set its own flags if I understood correctly, i.e.:


	if (name == NULL)
		v4l2_ctrl_fill(cfg->id, &name, &type, &min, &max, &step,
							&def,&flags)

<snip>

	*name = v4l2_ctrl_get_name(id);
	*flags = 0


<snip>


To be honest, I didn't quite understand whether you wanted individual 
drivers to signal they want to treat a given control as a dynamic array 
or whether I should add that flag in the switch statement in 
v4l2_ctrl_fill, thereby enabling this feature by default for all drivers 
that use that control if I understood correctly.

If the former, I was expecting to communicate it to userspace via a menu 
control, e.g. for h264 and cedrus:

enum v4l2_stateless_h264_decode_mode {
  	V4L2_STATELESS_H264_DECODE_MODE_SLICE_BASED, /* i.e. a single slice 
per request */
  	V4L2_STATELESS_H264_DECODE_MODE_FRAME_BASED,
+	V4L2_STATELESS_H264_DECODE_MODE_SLICE_ARRAY_BASED, /* i.e. an array of 
slices per request */
  };

@@ -56,6 +56,7 @@ static const struct cedrus_control cedrus_controls[] = {
  	{
  		.cfg = {
  			.id	= V4L2_CID_STATELESS_H264_SLICE_PARAMS,
+			.flags  = V4L2_CTRL_FLAG_DYNAMIC_ARRAY,
  		},
  		.codec		= CEDRUS_CODEC_H264,
  	},
@@ -86,7 +87,7 @@ static const struct cedrus_control cedrus_controls[] = {
  	{
  		.cfg = {
  			.id	= V4L2_CID_STATELESS_H264_DECODE_MODE,
-			.max	= V4L2_STATELESS_H264_DECODE_MODE_SLICE_BASED,
+			.max	= V4L2_STATELESS_H264_DECODE_MODE_SLICE_ARRAY_BASED,
  			.def	= V4L2_STATELESS_H264_DECODE_MODE_SLICE_BASED,
  		},
  		.codec		= CEDRUS_CODEC_H264,

-- Daniel

Re: [RFC PATCHv2 02/11] v4l2-ctrls: add support for dynamically allocated arrays.

[Daniel Almeida]

I am sending this again because apparently I sent it originally from my 
personal email by mistake.

---------------


Hi Hans!

I have been working on testing this lately as you know.

 From the example you've added to vivid, i.e.:

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

+    v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_dyn_array, NULL);


I was under the impression that it'd be enough to pass the id and 
V4L2_CTRL_FLAG_DYNAMIC_ARRAY in the config before calling 
v4l2_ctrl_new_custom. Apparently that's not the case though, because 
v4l2_ctrl_fill will then set its own flags if I understood correctly, i.e.:


     if (name == NULL)
         v4l2_ctrl_fill(cfg->id, &name, &type, &min, &max, &step,
                             &def,&flags)

<snip>

     *name = v4l2_ctrl_get_name(id);
     *flags = 0


<snip>


To be honest, I didn't quite understand whether you wanted individual 
drivers to signal they want to treat a given control as a dynamic array 
or whether I should add that flag in the switch statement in 
v4l2_ctrl_fill, thereby enabling this feature by default for all drivers 
that use that control if I understood correctly.

If the former, I was expecting to communicate it to userspace via a menu 
control, e.g. for h264 and cedrus:

enum v4l2_stateless_h264_decode_mode {
      V4L2_STATELESS_H264_DECODE_MODE_SLICE_BASED, /* i.e. a single 
slice per request */
      V4L2_STATELESS_H264_DECODE_MODE_FRAME_BASED,
+    V4L2_STATELESS_H264_DECODE_MODE_SLICE_ARRAY_BASED, /* i.e. an array 
of slices per request */
  };

@@ -56,6 +56,7 @@ static const struct cedrus_control cedrus_controls[] = {
      {
          .cfg = {
              .id    = V4L2_CID_STATELESS_H264_SLICE_PARAMS,
+            .flags  = V4L2_CTRL_FLAG_DYNAMIC_ARRAY,
          },
          .codec        = CEDRUS_CODEC_H264,
      },
@@ -86,7 +87,7 @@ static const struct cedrus_control cedrus_controls[] = {
      {
          .cfg = {
              .id    = V4L2_CID_STATELESS_H264_DECODE_MODE,
-            .max    = V4L2_STATELESS_H264_DECODE_MODE_SLICE_BASED,
+            .max    = V4L2_STATELESS_H264_DECODE_MODE_SLICE_ARRAY_BASED,
              .def    = V4L2_STATELESS_H264_DECODE_MODE_SLICE_BASED,
          },
          .codec        = CEDRUS_CODEC_H264,

-- Daniel

Re: [RFC PATCHv2 02/11] v4l2-ctrls: add support for dynamically allocated arrays.

[John Cox]

Hi

>I am sending this again because apparently I sent it originally from my 
>personal email by mistake.
>
>---------------
>
>
>Hi Hans!
>
>I have been working on testing this lately as you know.
>
> From the example you've added to vivid, i.e.:
>
>+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 },
>+};
>+
>
>+    v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_dyn_array, NULL);
>
>
>I was under the impression that it'd be enough to pass the id and 
>V4L2_CTRL_FLAG_DYNAMIC_ARRAY in the config before calling 
>v4l2_ctrl_new_custom. Apparently that's not the case though, because 
>v4l2_ctrl_fill will then set its own flags if I understood correctly, i.e.:
>
>
>     if (name == NULL)
>         v4l2_ctrl_fill(cfg->id, &name, &type, &min, &max, &step,
>                             &def,&flags)
>
><snip>
>
>     *name = v4l2_ctrl_get_name(id);
>     *flags = 0
>
>
><snip>

Yeah - I spotted that too.  Its not too hard to write a full config but
it would be nicer not to.

>To be honest, I didn't quite understand whether you wanted individual 
>drivers to signal they want to treat a given control as a dynamic array 
>or whether I should add that flag in the switch statement in 
>v4l2_ctrl_fill, thereby enabling this feature by default for all drivers 
>that use that control if I understood correctly.
>
>If the former, I was expecting to communicate it to userspace via a menu 
>control, e.g. for h264 and cedrus:
>
>enum v4l2_stateless_h264_decode_mode {
>      V4L2_STATELESS_H264_DECODE_MODE_SLICE_BASED, /* i.e. a single 
>slice per request */
>      V4L2_STATELESS_H264_DECODE_MODE_FRAME_BASED,
>+    V4L2_STATELESS_H264_DECODE_MODE_SLICE_ARRAY_BASED, /* i.e. an array 
>of slices per request */
>  };
>
>@@ -56,6 +56,7 @@ static const struct cedrus_control cedrus_controls[] = {
>      {
>          .cfg = {
>              .id    = V4L2_CID_STATELESS_H264_SLICE_PARAMS,
>+            .flags  = V4L2_CTRL_FLAG_DYNAMIC_ARRAY,
>          },
>          .codec        = CEDRUS_CODEC_H264,
>      },
>@@ -86,7 +87,7 @@ static const struct cedrus_control cedrus_controls[] = {
>      {
>          .cfg = {
>              .id    = V4L2_CID_STATELESS_H264_DECODE_MODE,
>-            .max    = V4L2_STATELESS_H264_DECODE_MODE_SLICE_BASED,
>+            .max    = V4L2_STATELESS_H264_DECODE_MODE_SLICE_ARRAY_BASED,
>              .def    = V4L2_STATELESS_H264_DECODE_MODE_SLICE_BASED,
>          },
>          .codec        = CEDRUS_CODEC_H264,

My test code (for ffmpeg & the rpi H265 decoder) used the same ID and
the caller tested to see if V4L2_CTRL_FLAG_DYNAMIC_ARRAY was set on the
control.  If it was then it sent slice headers in a block otherwise it
sent them one at a time and as it happens the driver will work just fine
if the array control is used to send slices in (say) groups of 16.  This
worked well for me, and was the simplest coding solution, as I had to
cope with both cases at both ends anyway. This probably isn't "the V4L2
way" though

Regards

John Cox

Re: [RFC PATCHv2 02/11] v4l2-ctrls: add support for dynamically allocated arrays.

[Hans Verkuil]

Hi Daniel,

Sorry for the delay, it's been very busy lately...

On 26/06/2021 18:42, Daniel Almeida wrote:
> I am sending this again because apparently I sent it originally from my 
> personal email by mistake.
> 
> ---------------
> 
> 
> Hi Hans!
> 
> I have been working on testing this lately as you know.
> 
>  From the example you've added to vivid, i.e.:
> 
> +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 },
> +};
> +
> 
> +    v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_dyn_array, NULL);
> 
> 
> I was under the impression that it'd be enough to pass the id and 
> V4L2_CTRL_FLAG_DYNAMIC_ARRAY in the config before calling 
> v4l2_ctrl_new_custom. Apparently that's not the case though, because 
> v4l2_ctrl_fill will then set its own flags if I understood correctly, i.e.:
> 
> 
>      if (name == NULL)
>          v4l2_ctrl_fill(cfg->id, &name, &type, &min, &max, &step,
>                              &def,&flags)

v4l2_ctrl_fill doesn't know about custom controls. It only handles standard
controls. For custom controls you always need to fill in everything.

> 
> <snip>
> 
>      *name = v4l2_ctrl_get_name(id);
>      *flags = 0
> 
> 
> <snip>
> 
> 
> To be honest, I didn't quite understand whether you wanted individual 
> drivers to signal they want to treat a given control as a dynamic array 
> or whether I should add that flag in the switch statement in 
> v4l2_ctrl_fill, thereby enabling this feature by default for all drivers 
> that use that control if I understood correctly.

If a standard control (i.e. not a private driver control) is a dynamic array,
then that flag must be set in v4l2_ctrl_fill().

> 
> If the former, I was expecting to communicate it to userspace via a menu 
> control, e.g. for h264 and cedrus:
> 
> enum v4l2_stateless_h264_decode_mode {
>       V4L2_STATELESS_H264_DECODE_MODE_SLICE_BASED, /* i.e. a single 
> slice per request */
>       V4L2_STATELESS_H264_DECODE_MODE_FRAME_BASED,
> +    V4L2_STATELESS_H264_DECODE_MODE_SLICE_ARRAY_BASED, /* i.e. an array 
> of slices per request */
>   };
> 
> @@ -56,6 +56,7 @@ static const struct cedrus_control cedrus_controls[] = {
>       {
>           .cfg = {
>               .id    = V4L2_CID_STATELESS_H264_SLICE_PARAMS,
> +            .flags  = V4L2_CTRL_FLAG_DYNAMIC_ARRAY,
>           },
>           .codec        = CEDRUS_CODEC_H264,
>       },
> @@ -86,7 +87,7 @@ static const struct cedrus_control cedrus_controls[] = {
>       {
>           .cfg = {
>               .id    = V4L2_CID_STATELESS_H264_DECODE_MODE,
> -            .max    = V4L2_STATELESS_H264_DECODE_MODE_SLICE_BASED,
> +            .max    = V4L2_STATELESS_H264_DECODE_MODE_SLICE_ARRAY_BASED,
>               .def    = V4L2_STATELESS_H264_DECODE_MODE_SLICE_BASED,
>           },
>           .codec        = CEDRUS_CODEC_H264,
> 
> -- Daniel
> 

If a driver supports arrays of slice params, then it will need to create a
new V4L2_CID_STATELESS_H264_SLICE_PARAMS_ARRAY control. You can't just change
V4L2_CID_STATELESS_H264_SLICE_PARAMS from a regular compound control to a
dynamic array control, they have to be different controls.

A driver can support either V4L2_CID_STATELESS_H264_SLICE_PARAMS,
V4L2_CID_STATELESS_H264_SLICE_PARAMS_ARRAY or both, with the corresponding modes
enumerated in V4L2_CID_STATELESS_H264_DECODE_MODE.

I hope this helps.

Regards,

	Hans

Re: [RFC PATCHv2 02/11] v4l2-ctrls: add support for dynamically allocated arrays.

[Ezequiel Garcia]

[..]

> 
> 
>  I tested & used this on my AV1 uapi patches [0] and this feature works just fine as far as I can tell.
>  
>  Tested-By: Daniel Almeida <daniel.almeida@collabora.com>
>  
>  Can this please be queued for 5.15?
>  

Hi Daniel,

I believe your reply wasn't plain text so might have been filtered.

Also, I'd say we want to have users actually using the interface,
before we consider merging it.
-- 
Kindly,
Ezequiel

Re: [RFC PATCHv2 02/11] v4l2-ctrls: add support for dynamically allocated arrays.

[John Cox]

Hi

>[..]
>
>> 
>> 
>>  I tested & used this on my AV1 uapi patches [0] and this feature works just fine as far as I can tell.
>>  
>>  Tested-By: Daniel Almeida <daniel.almeida@collabora.com>
>>  
>>  Can this please be queued for 5.15?
>>  
>
>Hi Daniel,
>
>I believe your reply wasn't plain text so might have been filtered.
>
>Also, I'd say we want to have users actually using the interface,
>before we consider merging it.

What constitutes using? We (Pi) are in the process of adding it in the
next version of our V4L2 H265 decoder (when we update to 5.14), or is it
only used once merged?

(Yes - I really must get round to trying to release our decoder)

Ta

John Cox

Re: [RFC PATCHv2 02/11] v4l2-ctrls: add support for dynamically allocated arrays.

[Ezequiel Garcia]

Hi John,

On Wed, 11 Aug 2021 at 12:59, John Cox <jc@kynesim.co.uk> wrote:
>
> Hi
>
> >[..]
> >
> >>
> >>
> >>  I tested & used this on my AV1 uapi patches [0] and this feature works just fine as far as I can tell.
> >>
> >>  Tested-By: Daniel Almeida <daniel.almeida@collabora.com>
> >>
> >>  Can this please be queued for 5.15?
> >>
> >
> >Hi Daniel,
> >
> >I believe your reply wasn't plain text so might have been filtered.
> >
> >Also, I'd say we want to have users actually using the interface,
> >before we consider merging it.
>
> What constitutes using? We (Pi) are in the process of adding it in the
> next version of our V4L2 H265 decoder (when we update to 5.14), or is it
> only used once merged?
>

Out of tree users have traditionally been assigned a low score.
Some kernel folks don't even count out of tree as user,
but  not me, I do count any user :-)

We definitely care about Pi and about other out of tree drivers
and users, as long as there's a plan towards mainline.

Thanks,
Ezequiel

[RFC PATCHv2 03/11] 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>
---
 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 8dc50fe22972..e9272eab1451 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)
@@ -189,6 +190,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,
@@ -1616,6 +1630,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.30.2

[RFC PATCHv2 04/11] media: document read-only requests

[Hans Verkuil]

This patch documents read-only requests, including the new
V4L2_BUF_CAP_SUPPORTS_RO_REQUESTS capability flag.

Based in part on a patch from Yunfei Dong <yunfei.dong@mediatek.com>.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 .../media/mediactl/media-request-ioc-queue.rst        |  5 +++++
 .../userspace-api/media/mediactl/request-api.rst      | 11 +++++++++++
 .../userspace-api/media/v4l/vidioc-reqbufs.rst        |  6 ++++++
 3 files changed, 22 insertions(+)

diff --git a/Documentation/userspace-api/media/mediactl/media-request-ioc-queue.rst b/Documentation/userspace-api/media/mediactl/media-request-ioc-queue.rst
index 04b33db2bb45..aee65ad765fc 100644
--- a/Documentation/userspace-api/media/mediactl/media-request-ioc-queue.rst
+++ b/Documentation/userspace-api/media/mediactl/media-request-ioc-queue.rst
@@ -51,6 +51,11 @@ queued directly and you next try to queue a request, or vice versa.
 A request must contain at least one buffer, otherwise this ioctl will
 return an ``ENOENT`` error.
 
+If the :ref:`buffer capabilities <v4l2-buf-capabilities>` indicate that
+only read-only requests are supported, then the request can only contain
+buffers. If anything else is present then this ioctl will return an
+``EINVAL`` error.
+
 Return Value
 ============
 
diff --git a/Documentation/userspace-api/media/mediactl/request-api.rst b/Documentation/userspace-api/media/mediactl/request-api.rst
index 6c4cbd9f08a5..71259cff590f 100644
--- a/Documentation/userspace-api/media/mediactl/request-api.rst
+++ b/Documentation/userspace-api/media/mediactl/request-api.rst
@@ -30,6 +30,13 @@ with different configurations in advance, knowing that the configuration will be
 applied when needed to get the expected result. Configuration values at the time
 of request completion are also available for reading.
 
+In some cases it does not make sense for user-space to associate configuration
+values with a frame, instead it only makes sense to retrieving configuration
+values at the time of request completion. In that case read-only requests can be
+used that only allow buffers to be queued and not configuration values. Which
+type of requests (regular or read-only) are supported is signalled through
+:ref:`buffer capabilities <v4l2-buf-capabilities>`.
+
 General Usage
 -------------
 
@@ -72,6 +79,10 @@ A queued request cannot be modified anymore.
    output buffers, not for capture buffers. Attempting to add a capture buffer
    to a request will result in an ``EBADR`` error.
 
+If the buffer type supports only read-only requests, and the request contains
+configuration values such as controls, then ``EINVAL`` is returned since no
+configuration values are allowed when submitting a read-only request.
+
 If the request contains configurations for multiple entities, individual drivers
 may synchronize so the requested pipeline's topology is applied before the
 buffers are processed. Media controller drivers do a best effort implementation
diff --git a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
index 50ea72043bb0..0451da50de51 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
@@ -117,6 +117,7 @@ aborting or finishing any DMA in progress, an implicit
 .. _V4L2-BUF-CAP-SUPPORTS-ORPHANED-BUFS:
 .. _V4L2-BUF-CAP-SUPPORTS-M2M-HOLD-CAPTURE-BUF:
 .. _V4L2-BUF-CAP-SUPPORTS-MMAP-CACHE-HINTS:
+.. _V4L2-BUF-CAP-SUPPORTS-RO-REQUESTS:
 
 .. raw:: latex
 
@@ -143,6 +144,7 @@ aborting or finishing any DMA in progress, an implicit
     * - ``V4L2_BUF_CAP_SUPPORTS_REQUESTS``
       - 0x00000008
       - This buffer type supports :ref:`requests <media-request-api>`.
+        This flag is mutually exclusive with ``V4L2_BUF_CAP_SUPPORTS_RO_REQUESTS``.
     * - ``V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS``
       - 0x00000010
       - The kernel allows calling :ref:`VIDIOC_REQBUFS` while buffers are still
@@ -160,6 +162,10 @@ aborting or finishing any DMA in progress, an implicit
         queue is used for :ref:`memory mapping <mmap>` streaming I/O. See
         :ref:`V4L2_BUF_FLAG_NO_CACHE_INVALIDATE <V4L2-BUF-FLAG-NO-CACHE-INVALIDATE>` and
         :ref:`V4L2_BUF_FLAG_NO_CACHE_CLEAN <V4L2-BUF-FLAG-NO-CACHE-CLEAN>`.
+    * - ``V4L2_BUF_CAP_SUPPORTS_RO_REQUESTS``
+      - 0x00000080
+      - This buffer type supports read-only :ref:`requests <media-request-api>`.
+        This flag is mutually exclusive with ``V4L2_BUF_CAP_SUPPORTS_REQUESTS``.
 
 .. raw:: latex
 
-- 
2.30.2

[RFC PATCHv2 05/11] videodev2.h: add V4L2_BUF_CAP_SUPPORTS_RO_REQUESTS

[Hans Verkuil]

This patch adds support for the V4L2_BUF_CAP_SUPPORTS_RO_REQUESTS
flag. This flag is used for Read-Only Requests.

Based on a patch from Yunfei Dong <yunfei.dong@mediatek.com>.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/common/videobuf2/videobuf2-core.c | 3 +++
 drivers/media/common/videobuf2/videobuf2-v4l2.c | 4 +++-
 drivers/media/v4l2-core/v4l2-mem2mem.c          | 3 ++-
 include/media/videobuf2-core.h                  | 1 +
 include/uapi/linux/videodev2.h                  | 1 +
 5 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 02281d13505f..10c552638878 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -2331,6 +2331,9 @@ int vb2_core_queue_init(struct vb2_queue *q)
 	if (WARN_ON(q->requires_requests && !q->supports_requests))
 		return -EINVAL;
 
+	if (WARN_ON(q->supports_ro_requests && !q->supports_requests))
+		return -EINVAL;
+
 	INIT_LIST_HEAD(&q->queued_list);
 	INIT_LIST_HEAD(&q->done_list);
 	spin_lock_init(&q->done_lock);
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 7e96f67c60ba..c63ee7bc3331 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -712,7 +712,9 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
 	if (q->allow_cache_hints && q->io_modes & VB2_MMAP)
 		*caps |= V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS;
 #ifdef CONFIG_MEDIA_CONTROLLER_REQUEST_API
-	if (q->supports_requests)
+	if (q->supports_ro_requests)
+		*caps |= V4L2_BUF_CAP_SUPPORTS_RO_REQUESTS;
+	else if (q->supports_requests)
 		*caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
 #endif
 }
diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index e7f4bf5bc8dd..0d7c27a07224 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -753,7 +753,8 @@ int v4l2_m2m_qbuf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 
 	vq = v4l2_m2m_get_vq(m2m_ctx, buf->type);
 	if (V4L2_TYPE_IS_CAPTURE(vq->type) &&
-	    (buf->flags & V4L2_BUF_FLAG_REQUEST_FD)) {
+	    (buf->flags & V4L2_BUF_FLAG_REQUEST_FD) &&
+	    !vq->supports_ro_requests) {
 		dprintk("%s: requests cannot be used with capture buffers\n",
 			__func__);
 		return -EPERM;
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 12955cb460d2..0760f967365a 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -578,6 +578,7 @@ struct vb2_queue {
 	unsigned int			allow_zero_bytesused:1;
 	unsigned int		   quirk_poll_must_check_waiting_for_buffers:1;
 	unsigned int			supports_requests:1;
+	unsigned int			supports_ro_requests:1;
 	unsigned int			requires_requests:1;
 	unsigned int			uses_qbuf:1;
 	unsigned int			uses_requests:1;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 7222fc855d6b..a70126ad0899 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -964,6 +964,7 @@ struct v4l2_requestbuffers {
 #define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS		(1 << 4)
 #define V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF	(1 << 5)
 #define V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS		(1 << 6)
+#define V4L2_BUF_CAP_SUPPORTS_RO_REQUESTS		(1 << 7)
 
 /**
  * struct v4l2_plane - plane info for multi-planar buffers
-- 
2.30.2

[RFC PATCHv2 06/11] videobuf2-core: add vb2_request_buffer_first()

[Hans Verkuil]

Add the vb2_request_buffer_first() helper function to obtain the
first vb2 buffer in the request.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 .../media/common/videobuf2/videobuf2-core.c   | 19 +++++++++++++++++++
 include/media/videobuf2-core.h                |  7 +++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 10c552638878..3157fefe6500 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1477,6 +1477,25 @@ unsigned int vb2_request_buffer_cnt(struct media_request *req)
 }
 EXPORT_SYMBOL_GPL(vb2_request_buffer_cnt);
 
+struct vb2_buffer *vb2_request_buffer_first(struct media_request *req)
+{
+	struct media_request_object *obj;
+	struct vb2_buffer *vb = NULL;
+	unsigned long flags;
+
+	spin_lock_irqsave(&req->lock, flags);
+	list_for_each_entry(obj, &req->objects, list) {
+		if (vb2_request_object_is_buffer(obj)) {
+			vb = container_of(obj, struct vb2_buffer, req_obj);
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&req->lock, flags);
+
+	return vb;
+}
+EXPORT_SYMBOL_GPL(vb2_request_buffer_first);
+
 int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
 {
 	struct vb2_buffer *vb;
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 0760f967365a..f508ce6b3152 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -1258,4 +1258,11 @@ bool vb2_request_object_is_buffer(struct media_request_object *obj);
  */
 unsigned int vb2_request_buffer_cnt(struct media_request *req);
 
+/**
+ * vb2_request_buffer_first() - return the first buffer in the request
+ *
+ * @req:	the request.
+ */
+struct vb2_buffer *vb2_request_buffer_first(struct media_request *req);
+
 #endif /* _MEDIA_VIDEOBUF2_CORE_H */
-- 
2.30.2

[RFC PATCHv2 07/11] videobuf2-v4l2.c: vb2_request_validate: support RO requests

[Hans Verkuil]

If the vb2_queue for which the request is validated only supports
read-only requests, then check if non-buffer objects are in the
request and return -EINVAL if that is the case.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 .../media/common/videobuf2/videobuf2-v4l2.c   | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index c63ee7bc3331..93897c3f32e9 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -1278,12 +1278,20 @@ EXPORT_SYMBOL_GPL(vb2_ops_wait_finish);
 int vb2_request_validate(struct media_request *req)
 {
 	struct media_request_object *obj;
+	struct vb2_queue *q = NULL;
+	bool buffers_only = true;
 	int ret = 0;
 
-	if (!vb2_request_buffer_cnt(req))
-		return -ENOENT;
-
 	list_for_each_entry(obj, &req->objects, list) {
+		if (!vb2_request_object_is_buffer(obj)) {
+			buffers_only = false;
+		} else if (!q) {
+			struct vb2_buffer *vb;
+
+			vb = container_of(obj, struct vb2_buffer, req_obj);
+			q = vb->vb2_queue;
+		}
+
 		if (!obj->ops->prepare)
 			continue;
 
@@ -1292,6 +1300,11 @@ int vb2_request_validate(struct media_request *req)
 			break;
 	}
 
+	if (!q)
+		ret = -ENOENT;
+	else if (q->supports_ro_requests && !buffers_only)
+		ret = -EINVAL;
+
 	if (ret) {
 		list_for_each_entry_continue_reverse(obj, &req->objects, list)
 			if (obj->ops->unprepare)
-- 
2.30.2

[RFC PATCHv2 08/11] v4l2-mem2mem.c: allow requests for capture queues

[Hans Verkuil]

Capture queues can support requests as well, so add support for this.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 0d7c27a07224..e068ffa9006b 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -20,6 +20,7 @@
 #include <media/v4l2-device.h>
 #include <media/v4l2-fh.h>
 #include <media/v4l2-event.h>
+#include <media/v4l2-ctrls.h>
 
 MODULE_DESCRIPTION("Mem to mem device framework for videobuf");
 MODULE_AUTHOR("Pawel Osciak, <pawel@osciak.com>");
@@ -1284,10 +1285,14 @@ void v4l2_m2m_request_queue(struct media_request *req)
 		if (vb2_request_object_is_buffer(obj)) {
 			/* Sanity checks */
 			vb = container_of(obj, struct vb2_buffer, req_obj);
-			WARN_ON(!V4L2_TYPE_IS_OUTPUT(vb->vb2_queue->type));
-			m2m_ctx_obj = container_of(vb->vb2_queue,
-						   struct v4l2_m2m_ctx,
-						   out_q_ctx.q);
+			if (V4L2_TYPE_IS_OUTPUT(vb->vb2_queue->type))
+				m2m_ctx_obj = container_of(vb->vb2_queue,
+							   struct v4l2_m2m_ctx,
+							   out_q_ctx.q);
+			else
+				m2m_ctx_obj = container_of(vb->vb2_queue,
+							   struct v4l2_m2m_ctx,
+							   cap_q_ctx.q);
 			WARN_ON(m2m_ctx && m2m_ctx_obj != m2m_ctx);
 			m2m_ctx = m2m_ctx_obj;
 		}
-- 
2.30.2

[RFC PATCHv2 09/11] vivid: add ro_requests module option

[Hans Verkuil]

Add the ro_requests module option to test Read-Only Requests with vivid.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 Documentation/admin-guide/media/vivid.rst     | 10 ++++++++++
 drivers/media/test-drivers/vivid/vivid-core.c | 10 ++++++++++
 2 files changed, 20 insertions(+)

diff --git a/Documentation/admin-guide/media/vivid.rst b/Documentation/admin-guide/media/vivid.rst
index 6d7175f96f74..423a61797a1d 100644
--- a/Documentation/admin-guide/media/vivid.rst
+++ b/Documentation/admin-guide/media/vivid.rst
@@ -302,6 +302,16 @@ all configurable using the following module options:
 		- 0: forbid hints
 		- 1: allow hints
 
+- ro_requests:
+
+	specifies if the capture device supports the standard Request API (i.e.
+	userspace can set controls in a request before queueing it), or
+	the Read-Only Request API (userspace can only read back controls after
+	the request was completed). Default is 0.
+
+		- 0: regular requests
+		- 1: read-only requests
+
 Taken together, all these module options allow you to precisely customize
 the driver behavior and test your application with all sorts of permutations.
 It is also very suitable to emulate hardware that is not yet available, e.g.
diff --git a/drivers/media/test-drivers/vivid/vivid-core.c b/drivers/media/test-drivers/vivid/vivid-core.c
index ca0ebf6ad9cc..641d8c75e8c9 100644
--- a/drivers/media/test-drivers/vivid/vivid-core.c
+++ b/drivers/media/test-drivers/vivid/vivid-core.c
@@ -177,6 +177,14 @@ MODULE_PARM_DESC(cache_hints, " user-space cache hints, default is 0.\n"
 			     "\t\t    0 == forbid\n"
 			     "\t\t    1 == allow");
 
+static unsigned int ro_requests[VIVID_MAX_DEVS] = {
+	[0 ... (VIVID_MAX_DEVS - 1)] = 0
+};
+module_param_array(ro_requests, uint, NULL, 0444);
+MODULE_PARM_DESC(ro_requests, " use read-only requests for video capture instead of regular requests, default is 0.\n"
+			     "\t\t    0 == regular requests\n"
+			     "\t\t    1 == read-only requests");
+
 static struct vivid_dev *vivid_devs[VIVID_MAX_DEVS];
 
 const struct v4l2_rect vivid_min_rect = {
@@ -847,6 +855,8 @@ static int vivid_create_queue(struct vivid_dev *dev,
 	q->lock = &dev->mutex;
 	q->dev = dev->v4l2_dev.dev;
 	q->supports_requests = true;
+	if (V4L2_TYPE_IS_CAPTURE(buf_type))
+		q->supports_ro_requests = (ro_requests[dev->inst] == 1);
 	q->allow_cache_hints = (cache_hints[dev->inst] == 1);
 
 	return vb2_queue_init(q);
-- 
2.30.2

[RFC PATCHv2 10/11] vim2m: support read-only requests on the capture queue

[Hans Verkuil]

Added support for read-only requests on the capture queue
in order to test this feature.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/test-drivers/vim2m.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/media/test-drivers/vim2m.c b/drivers/media/test-drivers/vim2m.c
index d714fe50afe5..8bd794a19629 100644
--- a/drivers/media/test-drivers/vim2m.c
+++ b/drivers/media/test-drivers/vim2m.c
@@ -602,7 +602,11 @@ static void device_run(void *priv)
 	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
 	dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
 
-	/* Apply request controls if any */
+	/*
+	 * Apply request controls if any.
+	 * The dst_buf queue has read-only requests, so no need to
+	 * setup any controls for that buffer.
+	 */
 	v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
 				&ctx->hdl);
 
@@ -611,6 +615,8 @@ static void device_run(void *priv)
 	/* Complete request controls if any */
 	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
 				   &ctx->hdl);
+	v4l2_ctrl_request_complete(dst_buf->vb2_buf.req_obj.req,
+				   &ctx->hdl);
 
 	/* Run delayed work, which simulates a hardware irq  */
 	schedule_delayed_work(&ctx->work_run, msecs_to_jiffies(ctx->transtime));
@@ -1138,6 +1144,8 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
 	dst_vq->mem_ops = &vb2_vmalloc_memops;
 	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
 	dst_vq->lock = &ctx->vb_mutex;
+	dst_vq->supports_requests = true;
+	dst_vq->supports_ro_requests = true;
 
 	return vb2_queue_init(dst_vq);
 }
-- 
2.30.2

[RFC PATCHv2 11/11] vicodec: add support for read-only requests

[Hans Verkuil]

Add support for read-only requests for the stateless decoder capture
queue in order to test this feature.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 .../media/test-drivers/vicodec/vicodec-core.c | 67 +++++++++----------
 1 file changed, 32 insertions(+), 35 deletions(-)

diff --git a/drivers/media/test-drivers/vicodec/vicodec-core.c b/drivers/media/test-drivers/vicodec/vicodec-core.c
index 33f1c893c1b6..d4d2b9535d05 100644
--- a/drivers/media/test-drivers/vicodec/vicodec-core.c
+++ b/drivers/media/test-drivers/vicodec/vicodec-core.c
@@ -267,6 +267,11 @@ static int device_process(struct vicodec_ctx *ctx,
 	if (ctx->is_stateless) {
 		struct media_request *src_req = src_vb->vb2_buf.req_obj.req;
 
+		/*
+		 * Apply request controls if any.
+		 * The dst_vb queue has read-only requests, so no need to
+		 * setup any controls for that buffer.
+		 */
 		ret = v4l2_ctrl_request_setup(src_req, &ctx->hdl);
 		if (ret)
 			return ret;
@@ -408,11 +413,12 @@ static void device_run(void *priv)
 	struct vb2_v4l2_buffer *src_buf, *dst_buf;
 	struct vicodec_q_data *q_src, *q_dst;
 	u32 state;
-	struct media_request *src_req;
+	struct media_request *src_req, *dst_req;
 
 	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
 	dst_buf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
 	src_req = src_buf->vb2_buf.req_obj.req;
+	dst_req = dst_buf->vb2_buf.req_obj.req;
 
 	q_src = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
 	q_dst = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
@@ -452,6 +458,8 @@ static void device_run(void *priv)
 	spin_unlock(ctx->lock);
 	if (ctx->is_stateless && src_req)
 		v4l2_ctrl_request_complete(src_req, &ctx->hdl);
+	if (ctx->is_stateless && dst_req)
+		v4l2_ctrl_request_complete(dst_req, &ctx->hdl);
 
 	if (ctx->is_enc)
 		v4l2_m2m_job_finish(dev->stateful_enc.m2m_dev, ctx->fh.m2m_ctx);
@@ -1728,6 +1736,8 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
 	dst_vq->mem_ops = &vb2_vmalloc_memops;
 	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
 	dst_vq->lock = src_vq->lock;
+	dst_vq->supports_requests = ctx->is_stateless;
+	dst_vq->supports_ro_requests = ctx->is_stateless;
 
 	return vb2_queue_init(dst_vq);
 }
@@ -1947,34 +1957,19 @@ static int vicodec_release(struct file *file)
 
 static int vicodec_request_validate(struct media_request *req)
 {
-	struct media_request_object *obj;
+	struct vb2_buffer *vb = vb2_request_buffer_first(req);
 	struct v4l2_ctrl_handler *parent_hdl, *hdl;
-	struct vicodec_ctx *ctx = NULL;
+	struct vicodec_ctx *ctx;
 	struct v4l2_ctrl *ctrl;
-	unsigned int count;
-
-	list_for_each_entry(obj, &req->objects, list) {
-		struct vb2_buffer *vb;
-
-		if (vb2_request_object_is_buffer(obj)) {
-			vb = container_of(obj, struct vb2_buffer, req_obj);
-			ctx = vb2_get_drv_priv(vb->vb2_queue);
-
-			break;
-		}
-	}
 
-	if (!ctx) {
-		pr_err("No buffer was provided with the request\n");
+	if (!vb) {
+		dev_info(req->mdev->dev,
+			 "No buffer was provided with the request\n");
 		return -ENOENT;
 	}
+	ctx = vb2_get_drv_priv(vb->vb2_queue);
 
-	count = vb2_request_buffer_cnt(req);
-	if (!count) {
-		v4l2_info(&ctx->dev->v4l2_dev,
-			  "No buffer was provided with the request\n");
-		return -ENOENT;
-	} else if (count > 1) {
+	if (vb2_request_buffer_cnt(req) > 1) {
 		v4l2_info(&ctx->dev->v4l2_dev,
 			  "More than one buffer was provided with the request\n");
 		return -EINVAL;
@@ -1982,18 +1977,20 @@ static int vicodec_request_validate(struct media_request *req)
 
 	parent_hdl = &ctx->hdl;
 
-	hdl = v4l2_ctrl_request_hdl_find(req, parent_hdl);
-	if (!hdl) {
-		v4l2_info(&ctx->dev->v4l2_dev, "Missing codec control\n");
-		return -ENOENT;
-	}
-	ctrl = v4l2_ctrl_request_hdl_ctrl_find(hdl,
-					       vicodec_ctrl_stateless_state.id);
-	v4l2_ctrl_request_hdl_put(hdl);
-	if (!ctrl) {
-		v4l2_info(&ctx->dev->v4l2_dev,
-			  "Missing required codec control\n");
-		return -ENOENT;
+	if (vb->vb2_queue->is_output) {
+		hdl = v4l2_ctrl_request_hdl_find(req, parent_hdl);
+		if (!hdl) {
+			v4l2_info(&ctx->dev->v4l2_dev, "Missing codec control\n");
+			return -ENOENT;
+		}
+		ctrl = v4l2_ctrl_request_hdl_ctrl_find(hdl,
+						       vicodec_ctrl_stateless_state.id);
+		v4l2_ctrl_request_hdl_put(hdl);
+		if (!ctrl) {
+			v4l2_info(&ctx->dev->v4l2_dev,
+				  "Missing required codec control\n");
+			return -ENOENT;
+		}
 	}
 
 	return vb2_request_validate(req);
-- 
2.30.2