[PATCHv2 0/6] vb2/v4l2-ctrls: check if requests are required

[PATCHv2 0/6] vb2/v4l2-ctrls: check if requests are required

[Hans Verkuil]

Currently the vb2 supports_requests bitfield only indicates if the
Request API is supported by the vb2_queue. But for stateless
codecs the use of the Request API is actually a requirement.

So add a requires_requests bitfield and corresponding capability
to indicate that userspace has to use requests. And of course
reject direct VIDIOC_QBUF calls (i.e. V4L2_BUF_FLAG_REQUEST_FD
isn't set) if requires_requests is set.

Finally set this bitfield in the cedrus driver.

Do the same for controls: it makes no sense to set state controls
for stateless codecs directly without going through a request.
Add a new flag to indicate this and check it.

Regards,

	Hans

Hans Verkuil (6):
  vb2: add requires_requests bit for stateless codecs
  videodev2.h: add V4L2_BUF_CAP_REQUIRES_REQUESTS
  cedrus: set requires_requests
  videodev2.h: add V4L2_CTRL_FLAG_REQUIRES_REQUESTS
  v4l2-ctrls: check for REQUIRES_REQUESTS flag
  v4l2-ctrls: mark MPEG2 stateless controls as REQUIRES_REQUESTS

 .../media/uapi/v4l/vidioc-queryctrl.rst       |  4 ++++
 .../media/uapi/v4l/vidioc-reqbufs.rst         |  4 ++++
 .../media/videodev2.h.rst.exceptions          |  1 +
 .../media/common/videobuf2/videobuf2-core.c   |  5 +++-
 .../media/common/videobuf2/videobuf2-v4l2.c   |  6 +++++
 drivers/media/v4l2-core/v4l2-ctrls.c          |  5 ++++
 .../staging/media/sunxi/cedrus/cedrus_video.c |  1 +
 include/media/videobuf2-core.h                |  3 +++
 include/uapi/linux/videodev2.h                | 24 ++++++++++---------
 9 files changed, 41 insertions(+), 12 deletions(-)

-- 
2.20.1

[PATCHv2 1/6] vb2: add requires_requests bit for stateless codecs

[Hans Verkuil]

Stateless codecs require the use of the Request API as opposed of it
being optional.

So add a bit to indicate this and let vb2 check for this.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/common/videobuf2/videobuf2-core.c | 5 ++++-
 drivers/media/common/videobuf2/videobuf2-v4l2.c | 6 ++++++
 include/media/videobuf2-core.h                  | 3 +++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 34cc87ca8d59..2b1e93e429be 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1516,7 +1516,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
 
 	if ((req && q->uses_qbuf) ||
 	    (!req && vb->state != VB2_BUF_STATE_IN_REQUEST &&
-	     q->uses_requests)) {
+	     (q->uses_requests || q->requires_requests))) {
 		dprintk(1, "queue in wrong mode (qbuf vs requests)\n");
 		return -EBUSY;
 	}
@@ -2244,6 +2244,9 @@ int vb2_core_queue_init(struct vb2_queue *q)
 	    WARN_ON(!q->ops->buf_queue))
 		return -EINVAL;
 
+	if (WARN_ON(q->requires_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 3aeaea3af42a..8b15b7762c56 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -385,6 +385,10 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
 			dprintk(1, "%s: queue uses requests\n", opname);
 			return -EBUSY;
 		}
+		if (q->requires_requests) {
+			dprintk(1, "%s: queue requires requests\n", opname);
+			return -EACCES;
+		}
 		return 0;
 	} else if (!q->supports_requests) {
 		dprintk(1, "%s: queue does not support requests\n", opname);
@@ -657,6 +661,8 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
 #ifdef CONFIG_MEDIA_CONTROLLER_REQUEST_API
 	if (q->supports_requests)
 		*caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
+	if (q->requires_requests)
+		*caps |= V4L2_BUF_CAP_REQUIRES_REQUESTS;
 #endif
 }
 
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 06142c1469cc..7f76bc65453e 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -481,6 +481,8 @@ struct vb2_buf_ops {
  *              has not been called. This is a vb1 idiom that has been adopted
  *              also by vb2.
  * @supports_requests: this queue supports the Request API.
+ * @requires_requests: this queue requires the Request API. If this is set to 1,
+ *		then supports_requests must be set to 1 as well.
  * @uses_qbuf:	qbuf was used directly for this queue. Set to 1 the first
  *		time this is called. Set to 0 when the queue is canceled.
  *		If this is 1, then you cannot queue buffers from a request.
@@ -555,6 +557,7 @@ struct vb2_queue {
 	unsigned			allow_zero_bytesused:1;
 	unsigned		   quirk_poll_must_check_waiting_for_buffers:1;
 	unsigned			supports_requests:1;
+	unsigned			requires_requests:1;
 	unsigned			uses_qbuf:1;
 	unsigned			uses_requests:1;
 
-- 
2.20.1

[PATCHv2 2/6] videodev2.h: add V4L2_BUF_CAP_REQUIRES_REQUESTS

[Hans Verkuil]

Add capability to indicate that requests are required instead of
merely supported.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 Documentation/media/uapi/v4l/vidioc-reqbufs.rst | 4 ++++
 include/uapi/linux/videodev2.h                  | 1 +
 2 files changed, 5 insertions(+)

diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
index d7faef10e39b..d42a3d9a7db3 100644
--- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
+++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
@@ -125,6 +125,7 @@ aborting or finishing any DMA in progress, an implicit
 .. _V4L2-BUF-CAP-SUPPORTS-DMABUF:
 .. _V4L2-BUF-CAP-SUPPORTS-REQUESTS:
 .. _V4L2-BUF-CAP-SUPPORTS-ORPHANED-BUFS:
+.. _V4L2-BUF-CAP-REQUIRES-REQUESTS:
 
 .. cssclass:: longtable
 
@@ -150,6 +151,9 @@ aborting or finishing any DMA in progress, an implicit
       - The kernel allows calling :ref:`VIDIOC_REQBUFS` while buffers are still
         mapped or exported via DMABUF. These orphaned buffers will be freed
         when they are unmapped or when the exported DMABUF fds are closed.
+    * - ``V4L2_BUF_CAP_REQUIRES_REQUESTS``
+      - 0x00000020
+      - This buffer type requires the use of :ref:`requests <media-request-api>`.
 
 Return Value
 ============
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 9a920f071ff9..7f035d44666e 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -891,6 +891,7 @@ struct v4l2_requestbuffers {
 #define V4L2_BUF_CAP_SUPPORTS_DMABUF	(1 << 2)
 #define V4L2_BUF_CAP_SUPPORTS_REQUESTS	(1 << 3)
 #define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS (1 << 4)
+#define V4L2_BUF_CAP_REQUIRES_REQUESTS	(1 << 5)
 
 /**
  * struct v4l2_plane - plane info for multi-planar buffers
-- 
2.20.1

[PATCHv2 3/6] cedrus: set requires_requests

[Hans Verkuil]

The cedrus stateless decoder requires the use of request, so
indicate this by setting requires_requests to 1.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/staging/media/sunxi/cedrus/cedrus_video.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index b5cc79389d67..2c77d82a8527 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -526,6 +526,7 @@ int cedrus_queue_init(void *priv, struct vb2_queue *src_vq,
 	src_vq->lock = &ctx->dev->dev_mutex;
 	src_vq->dev = ctx->dev->dev;
 	src_vq->supports_requests = true;
+	src_vq->requires_requests = true;
 
 	ret = vb2_queue_init(src_vq);
 	if (ret)
-- 
2.20.1

[PATCHv2 4/6] videodev2.h: add V4L2_CTRL_FLAG_REQUIRES_REQUESTS

[Hans Verkuil]

Indicate if a control can only be set through a request, as opposed
to being set directly. This is necessary for stateless codecs where
it makes no sense to set the state controls directly.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 .../media/uapi/v4l/vidioc-queryctrl.rst       |  4 ++++
 .../media/videodev2.h.rst.exceptions          |  1 +
 include/uapi/linux/videodev2.h                | 23 ++++++++++---------
 3 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
index f824162d0ea9..b08c69cedb92 100644
--- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
+++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
@@ -539,6 +539,10 @@ 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_REQUIRES_REQUESTS``
+      - 0x0800
+      - This control cannot be set directly, but only through a request
+        (i.e. by setting ``which`` to ``V4L2_CTRL_WHICH_REQUEST_VAL``).
 
 
 Return Value
diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
index 64d348e67df9..0caa72014dba 100644
--- a/Documentation/media/videodev2.h.rst.exceptions
+++ b/Documentation/media/videodev2.h.rst.exceptions
@@ -351,6 +351,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_REQUIRES_REQUESTS 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 7f035d44666e..a78bfdc1df97 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1736,17 +1736,18 @@ struct v4l2_querymenu {
 } __attribute__ ((packed));
 
 /*  Control flags  */
-#define V4L2_CTRL_FLAG_DISABLED		0x0001
-#define V4L2_CTRL_FLAG_GRABBED		0x0002
-#define V4L2_CTRL_FLAG_READ_ONLY	0x0004
-#define V4L2_CTRL_FLAG_UPDATE		0x0008
-#define V4L2_CTRL_FLAG_INACTIVE		0x0010
-#define V4L2_CTRL_FLAG_SLIDER		0x0020
-#define V4L2_CTRL_FLAG_WRITE_ONLY	0x0040
-#define V4L2_CTRL_FLAG_VOLATILE		0x0080
-#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_DISABLED			0x0001
+#define V4L2_CTRL_FLAG_GRABBED			0x0002
+#define V4L2_CTRL_FLAG_READ_ONLY		0x0004
+#define V4L2_CTRL_FLAG_UPDATE			0x0008
+#define V4L2_CTRL_FLAG_INACTIVE			0x0010
+#define V4L2_CTRL_FLAG_SLIDER			0x0020
+#define V4L2_CTRL_FLAG_WRITE_ONLY		0x0040
+#define V4L2_CTRL_FLAG_VOLATILE			0x0080
+#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_REQUIRES_REQUESTS	0x0800
 
 /*  Query flags, to be ORed with the control ID */
 #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000
-- 
2.20.1

Re: [PATCHv2 4/6] videodev2.h: add V4L2_CTRL_FLAG_REQUIRES_REQUESTS

[Hans Verkuil]

On 2/11/19 11:13 AM, Hans Verkuil wrote:
> Indicate if a control can only be set through a request, as opposed
> to being set directly. This is necessary for stateless codecs where
> it makes no sense to set the state controls directly.

Kwiboo on irc pointed out that this clashes with this line the in Initialization
section of the stateless decoder API:

"Call VIDIOC_S_EXT_CTRLS() to set all the controls (parsed headers, etc.) required
 by the OUTPUT format to enumerate the CAPTURE formats."

So for now ignore patches 4-6: I need to think about this some more.

My worry here is what happens when userspace is adding these controls to a
request and at the same time sets them directly. That may cause weird side-effects.

Regards,

	Hans

> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  .../media/uapi/v4l/vidioc-queryctrl.rst       |  4 ++++
>  .../media/videodev2.h.rst.exceptions          |  1 +
>  include/uapi/linux/videodev2.h                | 23 ++++++++++---------
>  3 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> index f824162d0ea9..b08c69cedb92 100644
> --- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> @@ -539,6 +539,10 @@ 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_REQUIRES_REQUESTS``
> +      - 0x0800
> +      - This control cannot be set directly, but only through a request
> +        (i.e. by setting ``which`` to ``V4L2_CTRL_WHICH_REQUEST_VAL``).
>  
>  
>  Return Value
> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
> index 64d348e67df9..0caa72014dba 100644
> --- a/Documentation/media/videodev2.h.rst.exceptions
> +++ b/Documentation/media/videodev2.h.rst.exceptions
> @@ -351,6 +351,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_REQUIRES_REQUESTS 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 7f035d44666e..a78bfdc1df97 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1736,17 +1736,18 @@ struct v4l2_querymenu {
>  } __attribute__ ((packed));
>  
>  /*  Control flags  */
> -#define V4L2_CTRL_FLAG_DISABLED		0x0001
> -#define V4L2_CTRL_FLAG_GRABBED		0x0002
> -#define V4L2_CTRL_FLAG_READ_ONLY	0x0004
> -#define V4L2_CTRL_FLAG_UPDATE		0x0008
> -#define V4L2_CTRL_FLAG_INACTIVE		0x0010
> -#define V4L2_CTRL_FLAG_SLIDER		0x0020
> -#define V4L2_CTRL_FLAG_WRITE_ONLY	0x0040
> -#define V4L2_CTRL_FLAG_VOLATILE		0x0080
> -#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_DISABLED			0x0001
> +#define V4L2_CTRL_FLAG_GRABBED			0x0002
> +#define V4L2_CTRL_FLAG_READ_ONLY		0x0004
> +#define V4L2_CTRL_FLAG_UPDATE			0x0008
> +#define V4L2_CTRL_FLAG_INACTIVE			0x0010
> +#define V4L2_CTRL_FLAG_SLIDER			0x0020
> +#define V4L2_CTRL_FLAG_WRITE_ONLY		0x0040
> +#define V4L2_CTRL_FLAG_VOLATILE			0x0080
> +#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_REQUIRES_REQUESTS	0x0800
>  
>  /*  Query flags, to be ORed with the control ID */
>  #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000
> 

Re: [PATCHv2 4/6] videodev2.h: add V4L2_CTRL_FLAG_REQUIRES_REQUESTS

[Paul Kocialkowski]

Hi,

On Mon, 2019-02-11 at 14:04 +0100, Hans Verkuil wrote:
> On 2/11/19 11:13 AM, Hans Verkuil wrote:
> > Indicate if a control can only be set through a request, as opposed
> > to being set directly. This is necessary for stateless codecs where
> > it makes no sense to set the state controls directly.
> 
> Kwiboo on irc pointed out that this clashes with this line the in Initialization
> section of the stateless decoder API:
> 
> "Call VIDIOC_S_EXT_CTRLS() to set all the controls (parsed headers, etc.) required
>  by the OUTPUT format to enumerate the CAPTURE formats."
> 
> So for now ignore patches 4-6: I need to think about this some more.
> 
> My worry here is what happens when userspace is adding these controls to a
> request and at the same time sets them directly. That may cause weird side-effects.

This seems to be a very legitimate concern, as nothing guarantees that
the controls setup by v4l2_ctrl_request_setup won't be overridden
before the driver uses them.

One solution could be to mark the controls as "in use" when the request
has new data for them, clear that in v4l2_ctrl_request_complete and
return EBUSY when trying to set the control in between the two calls.

This way, we ensure that any control set via a request will retain the
value passed with the request, which is independent from the control
itself (so we don't need special handling for stateless codec
controls). It also allows setting the control outside of a request for
enumerating formats.

What do you think?

Cheers,

Paul

> Regards,
> 
> 	Hans
> 
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > ---
> >  .../media/uapi/v4l/vidioc-queryctrl.rst       |  4 ++++
> >  .../media/videodev2.h.rst.exceptions          |  1 +
> >  include/uapi/linux/videodev2.h                | 23 ++++++++++---------
> >  3 files changed, 17 insertions(+), 11 deletions(-)
> > 
> > diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> > index f824162d0ea9..b08c69cedb92 100644
> > --- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> > +++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> > @@ -539,6 +539,10 @@ 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_REQUIRES_REQUESTS``
> > +      - 0x0800
> > +      - This control cannot be set directly, but only through a request
> > +        (i.e. by setting ``which`` to ``V4L2_CTRL_WHICH_REQUEST_VAL``).
> >  
> >  
> >  Return Value
> > diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
> > index 64d348e67df9..0caa72014dba 100644
> > --- a/Documentation/media/videodev2.h.rst.exceptions
> > +++ b/Documentation/media/videodev2.h.rst.exceptions
> > @@ -351,6 +351,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_REQUIRES_REQUESTS 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 7f035d44666e..a78bfdc1df97 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -1736,17 +1736,18 @@ struct v4l2_querymenu {
> >  } __attribute__ ((packed));
> >  
> >  /*  Control flags  */
> > -#define V4L2_CTRL_FLAG_DISABLED		0x0001
> > -#define V4L2_CTRL_FLAG_GRABBED		0x0002
> > -#define V4L2_CTRL_FLAG_READ_ONLY	0x0004
> > -#define V4L2_CTRL_FLAG_UPDATE		0x0008
> > -#define V4L2_CTRL_FLAG_INACTIVE		0x0010
> > -#define V4L2_CTRL_FLAG_SLIDER		0x0020
> > -#define V4L2_CTRL_FLAG_WRITE_ONLY	0x0040
> > -#define V4L2_CTRL_FLAG_VOLATILE		0x0080
> > -#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_DISABLED			0x0001
> > +#define V4L2_CTRL_FLAG_GRABBED			0x0002
> > +#define V4L2_CTRL_FLAG_READ_ONLY		0x0004
> > +#define V4L2_CTRL_FLAG_UPDATE			0x0008
> > +#define V4L2_CTRL_FLAG_INACTIVE			0x0010
> > +#define V4L2_CTRL_FLAG_SLIDER			0x0020
> > +#define V4L2_CTRL_FLAG_WRITE_ONLY		0x0040
> > +#define V4L2_CTRL_FLAG_VOLATILE			0x0080
> > +#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_REQUIRES_REQUESTS	0x0800
> >  
> >  /*  Query flags, to be ORed with the control ID */
> >  #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000
> > 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCHv2 4/6] videodev2.h: add V4L2_CTRL_FLAG_REQUIRES_REQUESTS

[Hans Verkuil]

On 2/13/19 11:38 AM, Paul Kocialkowski wrote:
> Hi,
> 
> On Mon, 2019-02-11 at 14:04 +0100, Hans Verkuil wrote:
>> On 2/11/19 11:13 AM, Hans Verkuil wrote:
>>> Indicate if a control can only be set through a request, as opposed
>>> to being set directly. This is necessary for stateless codecs where
>>> it makes no sense to set the state controls directly.
>>
>> Kwiboo on irc pointed out that this clashes with this line the in Initialization
>> section of the stateless decoder API:
>>
>> "Call VIDIOC_S_EXT_CTRLS() to set all the controls (parsed headers, etc.) required
>>  by the OUTPUT format to enumerate the CAPTURE formats."
>>
>> So for now ignore patches 4-6: I need to think about this some more.
>>
>> My worry here is what happens when userspace is adding these controls to a
>> request and at the same time sets them directly. That may cause weird side-effects.
> 
> This seems to be a very legitimate concern, as nothing guarantees that
> the controls setup by v4l2_ctrl_request_setup won't be overridden
> before the driver uses them.
> 
> One solution could be to mark the controls as "in use" when the request
> has new data for them, clear that in v4l2_ctrl_request_complete and
> return EBUSY when trying to set the control in between the two calls.
> 
> This way, we ensure that any control set via a request will retain the
> value passed with the request, which is independent from the control
> itself (so we don't need special handling for stateless codec
> controls). It also allows setting the control outside of a request for
> enumerating formats.
> 
> What do you think?

That's a good idea. I'll see if I can make that work.

Regards,

	Hans

> 
> Cheers,
> 
> Paul
> 
>> Regards,
>>
>> 	Hans
>>
>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> ---
>>>  .../media/uapi/v4l/vidioc-queryctrl.rst       |  4 ++++
>>>  .../media/videodev2.h.rst.exceptions          |  1 +
>>>  include/uapi/linux/videodev2.h                | 23 ++++++++++---------
>>>  3 files changed, 17 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
>>> index f824162d0ea9..b08c69cedb92 100644
>>> --- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
>>> +++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
>>> @@ -539,6 +539,10 @@ 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_REQUIRES_REQUESTS``
>>> +      - 0x0800
>>> +      - This control cannot be set directly, but only through a request
>>> +        (i.e. by setting ``which`` to ``V4L2_CTRL_WHICH_REQUEST_VAL``).
>>>  
>>>  
>>>  Return Value
>>> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
>>> index 64d348e67df9..0caa72014dba 100644
>>> --- a/Documentation/media/videodev2.h.rst.exceptions
>>> +++ b/Documentation/media/videodev2.h.rst.exceptions
>>> @@ -351,6 +351,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_REQUIRES_REQUESTS 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 7f035d44666e..a78bfdc1df97 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -1736,17 +1736,18 @@ struct v4l2_querymenu {
>>>  } __attribute__ ((packed));
>>>  
>>>  /*  Control flags  */
>>> -#define V4L2_CTRL_FLAG_DISABLED		0x0001
>>> -#define V4L2_CTRL_FLAG_GRABBED		0x0002
>>> -#define V4L2_CTRL_FLAG_READ_ONLY	0x0004
>>> -#define V4L2_CTRL_FLAG_UPDATE		0x0008
>>> -#define V4L2_CTRL_FLAG_INACTIVE		0x0010
>>> -#define V4L2_CTRL_FLAG_SLIDER		0x0020
>>> -#define V4L2_CTRL_FLAG_WRITE_ONLY	0x0040
>>> -#define V4L2_CTRL_FLAG_VOLATILE		0x0080
>>> -#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_DISABLED			0x0001
>>> +#define V4L2_CTRL_FLAG_GRABBED			0x0002
>>> +#define V4L2_CTRL_FLAG_READ_ONLY		0x0004
>>> +#define V4L2_CTRL_FLAG_UPDATE			0x0008
>>> +#define V4L2_CTRL_FLAG_INACTIVE			0x0010
>>> +#define V4L2_CTRL_FLAG_SLIDER			0x0020
>>> +#define V4L2_CTRL_FLAG_WRITE_ONLY		0x0040
>>> +#define V4L2_CTRL_FLAG_VOLATILE			0x0080
>>> +#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_REQUIRES_REQUESTS	0x0800
>>>  
>>>  /*  Query flags, to be ORed with the control ID */
>>>  #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000
>>>

[PATCHv2 5/6] v4l2-ctrls: check for REQUIRES_REQUESTS flag

[Hans Verkuil]

Attempts to get/try/set controls that require requests
without actually specifying a request are now rejected.

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

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 99308dac2daa..25f80f0eba69 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -3150,6 +3150,9 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 		ctrl = ref->ctrl;
 		if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED)
 			return -EINVAL;
+		if ((ctrl->flags & V4L2_CTRL_FLAG_REQUIRES_REQUESTS) &&
+		    !hdl->req_obj.req)
+			return -EACCES;
 
 		if (ctrl->cluster[0]->ncontrols > 1)
 			have_clusters = true;
-- 
2.20.1

[PATCHv2 6/6] v4l2-ctrls: mark MPEG2 stateless controls as REQUIRES_REQUESTS

[Hans Verkuil]

These stateless codec controls can only be used in combination with
a request. So mark them as such.

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

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 25f80f0eba69..7825c8d66498 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1299,9 +1299,11 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		break;
 	case V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS:
 		*type = V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS;
+		*flags |= V4L2_CTRL_FLAG_REQUIRES_REQUESTS;
 		break;
 	case V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION:
 		*type = V4L2_CTRL_TYPE_MPEG2_QUANTIZATION;
+		*flags |= V4L2_CTRL_FLAG_REQUIRES_REQUESTS;
 		break;
 	default:
 		*type = V4L2_CTRL_TYPE_INTEGER;
-- 
2.20.1