* [PATCHv2 1/6] vb2: add requires_requests bit for stateless codecs
2019-02-11 10:13 [PATCHv2 0/6] vb2/v4l2-ctrls: check if requests are required Hans Verkuil
@ 2019-02-11 10:13 ` Hans Verkuil
2019-02-11 10:13 ` [PATCHv2 2/6] videodev2.h: add V4L2_BUF_CAP_REQUIRES_REQUESTS Hans Verkuil
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Hans Verkuil @ 2019-02-11 10:13 UTC (permalink / raw)
To: linux-media
Cc: Dafna Hirschfeld, Paul Kocialkowski, Ezequiel Garcia, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCHv2 2/6] videodev2.h: add V4L2_BUF_CAP_REQUIRES_REQUESTS
2019-02-11 10:13 [PATCHv2 0/6] vb2/v4l2-ctrls: check if requests are required Hans Verkuil
2019-02-11 10:13 ` [PATCHv2 1/6] vb2: add requires_requests bit for stateless codecs Hans Verkuil
@ 2019-02-11 10:13 ` Hans Verkuil
2019-02-11 10:13 ` [PATCHv2 3/6] cedrus: set requires_requests Hans Verkuil
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Hans Verkuil @ 2019-02-11 10:13 UTC (permalink / raw)
To: linux-media
Cc: Dafna Hirschfeld, Paul Kocialkowski, Ezequiel Garcia, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCHv2 3/6] cedrus: set requires_requests
2019-02-11 10:13 [PATCHv2 0/6] vb2/v4l2-ctrls: check if requests are required Hans Verkuil
2019-02-11 10:13 ` [PATCHv2 1/6] vb2: add requires_requests bit for stateless codecs Hans Verkuil
2019-02-11 10:13 ` [PATCHv2 2/6] videodev2.h: add V4L2_BUF_CAP_REQUIRES_REQUESTS Hans Verkuil
@ 2019-02-11 10:13 ` Hans Verkuil
2019-02-11 10:13 ` [PATCHv2 4/6] videodev2.h: add V4L2_CTRL_FLAG_REQUIRES_REQUESTS Hans Verkuil
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Hans Verkuil @ 2019-02-11 10:13 UTC (permalink / raw)
To: linux-media
Cc: Dafna Hirschfeld, Paul Kocialkowski, Ezequiel Garcia, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCHv2 4/6] videodev2.h: add V4L2_CTRL_FLAG_REQUIRES_REQUESTS
2019-02-11 10:13 [PATCHv2 0/6] vb2/v4l2-ctrls: check if requests are required Hans Verkuil
` (2 preceding siblings ...)
2019-02-11 10:13 ` [PATCHv2 3/6] cedrus: set requires_requests Hans Verkuil
@ 2019-02-11 10:13 ` Hans Verkuil
2019-02-11 13:04 ` Hans Verkuil
2019-02-11 10:13 ` [PATCHv2 5/6] v4l2-ctrls: check for REQUIRES_REQUESTS flag Hans Verkuil
2019-02-11 10:13 ` [PATCHv2 6/6] v4l2-ctrls: mark MPEG2 stateless controls as REQUIRES_REQUESTS Hans Verkuil
5 siblings, 1 reply; 10+ messages in thread
From: Hans Verkuil @ 2019-02-11 10:13 UTC (permalink / raw)
To: linux-media
Cc: Dafna Hirschfeld, Paul Kocialkowski, Ezequiel Garcia, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCHv2 4/6] videodev2.h: add V4L2_CTRL_FLAG_REQUIRES_REQUESTS
2019-02-11 10:13 ` [PATCHv2 4/6] videodev2.h: add V4L2_CTRL_FLAG_REQUIRES_REQUESTS Hans Verkuil
@ 2019-02-11 13:04 ` Hans Verkuil
2019-02-13 10:38 ` Paul Kocialkowski
0 siblings, 1 reply; 10+ messages in thread
From: Hans Verkuil @ 2019-02-11 13:04 UTC (permalink / raw)
To: linux-media; +Cc: Dafna Hirschfeld, Paul Kocialkowski, Ezequiel Garcia
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2 4/6] videodev2.h: add V4L2_CTRL_FLAG_REQUIRES_REQUESTS
2019-02-11 13:04 ` Hans Verkuil
@ 2019-02-13 10:38 ` Paul Kocialkowski
2019-02-13 11:59 ` Hans Verkuil
0 siblings, 1 reply; 10+ messages in thread
From: Paul Kocialkowski @ 2019-02-13 10:38 UTC (permalink / raw)
To: Hans Verkuil, linux-media; +Cc: Dafna Hirschfeld, Ezequiel Garcia
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2 4/6] videodev2.h: add V4L2_CTRL_FLAG_REQUIRES_REQUESTS
2019-02-13 10:38 ` Paul Kocialkowski
@ 2019-02-13 11:59 ` Hans Verkuil
0 siblings, 0 replies; 10+ messages in thread
From: Hans Verkuil @ 2019-02-13 11:59 UTC (permalink / raw)
To: Paul Kocialkowski, linux-media; +Cc: Dafna Hirschfeld, Ezequiel Garcia
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
>>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCHv2 5/6] v4l2-ctrls: check for REQUIRES_REQUESTS flag
2019-02-11 10:13 [PATCHv2 0/6] vb2/v4l2-ctrls: check if requests are required Hans Verkuil
` (3 preceding siblings ...)
2019-02-11 10:13 ` [PATCHv2 4/6] videodev2.h: add V4L2_CTRL_FLAG_REQUIRES_REQUESTS Hans Verkuil
@ 2019-02-11 10:13 ` Hans Verkuil
2019-02-11 10:13 ` [PATCHv2 6/6] v4l2-ctrls: mark MPEG2 stateless controls as REQUIRES_REQUESTS Hans Verkuil
5 siblings, 0 replies; 10+ messages in thread
From: Hans Verkuil @ 2019-02-11 10:13 UTC (permalink / raw)
To: linux-media
Cc: Dafna Hirschfeld, Paul Kocialkowski, Ezequiel Garcia, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCHv2 6/6] v4l2-ctrls: mark MPEG2 stateless controls as REQUIRES_REQUESTS
2019-02-11 10:13 [PATCHv2 0/6] vb2/v4l2-ctrls: check if requests are required Hans Verkuil
` (4 preceding siblings ...)
2019-02-11 10:13 ` [PATCHv2 5/6] v4l2-ctrls: check for REQUIRES_REQUESTS flag Hans Verkuil
@ 2019-02-11 10:13 ` Hans Verkuil
5 siblings, 0 replies; 10+ messages in thread
From: Hans Verkuil @ 2019-02-11 10:13 UTC (permalink / raw)
To: linux-media
Cc: Dafna Hirschfeld, Paul Kocialkowski, Ezequiel Garcia, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread