[PATCH 0/3] vb2: do not allow support_requests + min_buffers_needed

[PATCH 0/3] vb2: do not allow support_requests + min_buffers_needed

[Hans Verkuil]

If the vb2_queue field min_buffers_needed is non-zero, then only
allow that if support_requests is false. Otherwise when a request is
queued and vb2_core_qbuf is called, that function could fail if it
needs to call start_streaming(). There is no way for the request API
to propagate such an error back to userspace, and you really don't
want that either.

If this is ever needed, then an API extension is most likely needed.

Currently there are only two drivers that set this combination of
features: cedrus and vivid.

There is no reason for a non-zero min_buffers_needed in the cedrus
driver, so just drop that.

For the vivid driver a new module option was added that allows the
user to control whether requests are not supported, supported (default)
or required. In the latter two cases min_buffers_needed is set to 0
in the vivid driver.

I would appreciate if someone can verify that the cedrus patch doesn't
break anything since that has only been compile tested.

Regards,

	Hans

Hans Verkuil (3):
  cedrus: drop min_buffers_needed.
  vivid: add module option to set request support mode
  videobuf2-core: sanity checks for requests and qbuf

 .../media/common/videobuf2/videobuf2-core.c   | 23 ++++++++++++++++++-
 drivers/media/test-drivers/vivid/vivid-core.c | 14 +++++++++--
 .../staging/media/sunxi/cedrus/cedrus_video.c |  2 --
 3 files changed, 34 insertions(+), 5 deletions(-)

-- 
2.30.2

[PATCH 1/3] cedrus: drop min_buffers_needed.

[Hans Verkuil]

There is no reason for the cedrus driver to set min_buffers_needed.
A non-zero min_buffers_needed can cause problems with the Request API
if start_streaming fails when queueing a buffer from a request.

Since it is not needed for this driver, just remove it.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Ezequiel Garcia <ezequiel@collabora.com>
Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/staging/media/sunxi/cedrus/cedrus_video.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index c589fe9dae70..f3cd452575d4 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -570,7 +570,6 @@ int cedrus_queue_init(void *priv, struct vb2_queue *src_vq,
 	src_vq->io_modes = VB2_MMAP | VB2_DMABUF;
 	src_vq->drv_priv = ctx;
 	src_vq->buf_struct_size = sizeof(struct cedrus_buffer);
-	src_vq->min_buffers_needed = 1;
 	src_vq->ops = &cedrus_qops;
 	src_vq->mem_ops = &vb2_dma_contig_memops;
 	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
@@ -587,7 +586,6 @@ int cedrus_queue_init(void *priv, struct vb2_queue *src_vq,
 	dst_vq->io_modes = VB2_MMAP | VB2_DMABUF;
 	dst_vq->drv_priv = ctx;
 	dst_vq->buf_struct_size = sizeof(struct cedrus_buffer);
-	dst_vq->min_buffers_needed = 1;
 	dst_vq->ops = &cedrus_qops;
 	dst_vq->mem_ops = &vb2_dma_contig_memops;
 	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
-- 
2.30.2

Re: [PATCH 1/3] cedrus: drop min_buffers_needed.

[Ezequiel Garcia]

On Thu, 2021-07-29 at 15:36 +0200, Hans Verkuil wrote:
> There is no reason for the cedrus driver to set min_buffers_needed.
> A non-zero min_buffers_needed can cause problems with the Request API
> if start_streaming fails when queueing a buffer from a request.
> 
> Since it is not needed for this driver, just remove it.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: Ezequiel Garcia <ezequiel@collabora.com>
> Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Adding Daniel in Cc, I think he has a board
ready to run some tests if needed.

Having said that, Hantro doesn't use min_buffers_needed
so this looks correct.

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>

Thanks,
Ezequiel

> ---
>  drivers/staging/media/sunxi/cedrus/cedrus_video.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> index c589fe9dae70..f3cd452575d4 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> @@ -570,7 +570,6 @@ int cedrus_queue_init(void *priv, struct vb2_queue *src_vq,
>         src_vq->io_modes = VB2_MMAP | VB2_DMABUF;
>         src_vq->drv_priv = ctx;
>         src_vq->buf_struct_size = sizeof(struct cedrus_buffer);
> -       src_vq->min_buffers_needed = 1;
>         src_vq->ops = &cedrus_qops;
>         src_vq->mem_ops = &vb2_dma_contig_memops;
>         src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> @@ -587,7 +586,6 @@ int cedrus_queue_init(void *priv, struct vb2_queue *src_vq,
>         dst_vq->io_modes = VB2_MMAP | VB2_DMABUF;
>         dst_vq->drv_priv = ctx;
>         dst_vq->buf_struct_size = sizeof(struct cedrus_buffer);
> -       dst_vq->min_buffers_needed = 1;
>         dst_vq->ops = &cedrus_qops;
>         dst_vq->mem_ops = &vb2_dma_contig_memops;
>         dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;

-- 
Kindly,
Ezequiel

[PATCH 2/3] vivid: add module option to set request support mode

[Hans Verkuil]

Currently vivid supports the Request API, but it also sets
min_buffers_needed in the vb2 queue. But the combination of
support_requests and min_buffers_needed is not allowed due to
the fact that vb2_core_qbuf() isn't supposed to fail when called
from the request framework. And if min_buffers_needed > 0, then
is can call start_streaming() which definitely can fail.

With the new module option you can control if requests are not
allowed (min_buffers_needed is 2 in that case), optionally allowed
or are required. In the latter two cases min_buffers_needed is set
to 0.

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

diff --git a/drivers/media/test-drivers/vivid/vivid-core.c b/drivers/media/test-drivers/vivid/vivid-core.c
index d2bd2653cf54..87f27c7524ec 100644
--- a/drivers/media/test-drivers/vivid/vivid-core.c
+++ b/drivers/media/test-drivers/vivid/vivid-core.c
@@ -177,6 +177,15 @@ 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 supports_requests[VIVID_MAX_DEVS] = {
+	[0 ... (VIVID_MAX_DEVS - 1)] = 1
+};
+module_param_array(supports_requests, uint, NULL, 0444);
+MODULE_PARM_DESC(supports_requests, " support for requests, default is 1.\n"
+			     "\t\t    0 == no support\n"
+			     "\t\t    1 == supports requests\n"
+			     "\t\t    2 == requires requests");
+
 static struct vivid_dev *vivid_devs[VIVID_MAX_DEVS];
 
 const struct v4l2_rect vivid_min_rect = {
@@ -883,10 +892,11 @@ static int vivid_create_queue(struct vivid_dev *dev,
 	q->mem_ops = allocators[dev->inst] == 1 ? &vb2_dma_contig_memops :
 						  &vb2_vmalloc_memops;
 	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
-	q->min_buffers_needed = min_buffers_needed;
+	q->min_buffers_needed = supports_requests[dev->inst] ? 0 : min_buffers_needed;
 	q->lock = &dev->mutex;
 	q->dev = dev->v4l2_dev.dev;
-	q->supports_requests = true;
+	q->supports_requests = supports_requests[dev->inst];
+	q->requires_requests = supports_requests[dev->inst] >= 2;
 	q->allow_cache_hints = (cache_hints[dev->inst] == 1);
 
 	return vb2_queue_init(q);
-- 
2.30.2

[PATCH 3/3] videobuf2-core: sanity checks for requests and qbuf

[Hans Verkuil]

The combination of supports_requests == 1 and min_buffers_needed > 0
is not allowed, WARN on that and return an error.

Also check that if vb2_core_qbuf() is called from req_queue, that it
doesn't return an error, unless it is -EIO.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Ezequiel Garcia <ezequiel@collabora.com>
Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 .../media/common/videobuf2/videobuf2-core.c   | 23 ++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 02281d13505f..3f0e816472d8 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1421,9 +1421,19 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
 static void vb2_req_queue(struct media_request_object *obj)
 {
 	struct vb2_buffer *vb = container_of(obj, struct vb2_buffer, req_obj);
+	int err;
 
 	mutex_lock(vb->vb2_queue->lock);
-	vb2_core_qbuf(vb->vb2_queue, vb->index, NULL, NULL);
+	/*
+	 * There is no method to propagate an error from vb2_core_qbuf(),
+	 * so if this returns a non-0 value, then WARN.
+	 *
+	 * The only exception is -EIO which is returned if q->error is
+	 * set. We just ignore that, and expect this will be caught the
+	 * next time vb2_req_prepare() is called.
+	 */
+	err = vb2_core_qbuf(vb->vb2_queue, vb->index, NULL, NULL);
+	WARN_ON_ONCE(err && err != -EIO);
 	mutex_unlock(vb->vb2_queue->lock);
 }
 
@@ -2331,6 +2341,17 @@ int vb2_core_queue_init(struct vb2_queue *q)
 	if (WARN_ON(q->requires_requests && !q->supports_requests))
 		return -EINVAL;
 
+	/*
+	 * This combination is not allowed since a non-zero value of
+	 * q->min_buffers_needed can cause vb2_core_qbuf() to fail if
+	 * it has to call start_streaming(), and the Request API expects
+	 * that queueing a request (and thus queueing a buffer contained
+	 * in that request) will always succeed. There is no method of
+	 * propagating an error back to userspace.
+	 */
+	if (WARN_ON(q->supports_requests && q->min_buffers_needed))
+		return -EINVAL;
+
 	INIT_LIST_HEAD(&q->queued_list);
 	INIT_LIST_HEAD(&q->done_list);
 	spin_lock_init(&q->done_lock);
-- 
2.30.2

Re: [PATCH 3/3] videobuf2-core: sanity checks for requests and qbuf

[Ezequiel Garcia]

On Thu, 2021-07-29 at 15:36 +0200, Hans Verkuil wrote:
> The combination of supports_requests == 1 and min_buffers_needed > 0
> is not allowed, WARN on that and return an error.
> 
> Also check that if vb2_core_qbuf() is called from req_queue, that it
> doesn't return an error, unless it is -EIO.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: Ezequiel Garcia <ezequiel@collabora.com>
> Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>

Thanks,
Ezequiel