All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] vb2: do not allow support_requests + min_buffers_needed
@ 2021-07-29 13:36 Hans Verkuil
  2021-07-29 13:36 ` [PATCH 1/3] cedrus: drop min_buffers_needed Hans Verkuil
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Hans Verkuil @ 2021-07-29 13:36 UTC (permalink / raw)
  To: linux-media

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/3] cedrus: drop min_buffers_needed.
  2021-07-29 13:36 [PATCH 0/3] vb2: do not allow support_requests + min_buffers_needed Hans Verkuil
@ 2021-07-29 13:36 ` Hans Verkuil
  2021-07-29 15:05   ` Ezequiel Garcia
  2021-07-29 13:36 ` [PATCH 2/3] vivid: add module option to set request support mode Hans Verkuil
  2021-07-29 13:36 ` [PATCH 3/3] videobuf2-core: sanity checks for requests and qbuf Hans Verkuil
  2 siblings, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2021-07-29 13:36 UTC (permalink / raw)
  To: linux-media; +Cc: Hans Verkuil, Ezequiel Garcia, Paul Kocialkowski

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/3] vivid: add module option to set request support mode
  2021-07-29 13:36 [PATCH 0/3] vb2: do not allow support_requests + min_buffers_needed Hans Verkuil
  2021-07-29 13:36 ` [PATCH 1/3] cedrus: drop min_buffers_needed Hans Verkuil
@ 2021-07-29 13:36 ` Hans Verkuil
  2021-07-29 13:36 ` [PATCH 3/3] videobuf2-core: sanity checks for requests and qbuf Hans Verkuil
  2 siblings, 0 replies; 6+ messages in thread
From: Hans Verkuil @ 2021-07-29 13:36 UTC (permalink / raw)
  To: linux-media; +Cc: 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 3/3] videobuf2-core: sanity checks for requests and qbuf
  2021-07-29 13:36 [PATCH 0/3] vb2: do not allow support_requests + min_buffers_needed Hans Verkuil
  2021-07-29 13:36 ` [PATCH 1/3] cedrus: drop min_buffers_needed Hans Verkuil
  2021-07-29 13:36 ` [PATCH 2/3] vivid: add module option to set request support mode Hans Verkuil
@ 2021-07-29 13:36 ` Hans Verkuil
  2021-07-29 15:08   ` Ezequiel Garcia
  2 siblings, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2021-07-29 13:36 UTC (permalink / raw)
  To: linux-media; +Cc: Hans Verkuil, Ezequiel Garcia, Paul Kocialkowski

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/3] cedrus: drop min_buffers_needed.
  2021-07-29 13:36 ` [PATCH 1/3] cedrus: drop min_buffers_needed Hans Verkuil
@ 2021-07-29 15:05   ` Ezequiel Garcia
  0 siblings, 0 replies; 6+ messages in thread
From: Ezequiel Garcia @ 2021-07-29 15:05 UTC (permalink / raw)
  To: Hans Verkuil, linux-media; +Cc: Paul Kocialkowski, Daniel Almeida

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 3/3] videobuf2-core: sanity checks for requests and qbuf
  2021-07-29 13:36 ` [PATCH 3/3] videobuf2-core: sanity checks for requests and qbuf Hans Verkuil
@ 2021-07-29 15:08   ` Ezequiel Garcia
  0 siblings, 0 replies; 6+ messages in thread
From: Ezequiel Garcia @ 2021-07-29 15:08 UTC (permalink / raw)
  To: Hans Verkuil, linux-media
  Cc: Paul Kocialkowski, Daniel Almeida, Nicolas Dufresne

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-07-29 15:08 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-29 13:36 [PATCH 0/3] vb2: do not allow support_requests + min_buffers_needed Hans Verkuil
2021-07-29 13:36 ` [PATCH 1/3] cedrus: drop min_buffers_needed Hans Verkuil
2021-07-29 15:05   ` Ezequiel Garcia
2021-07-29 13:36 ` [PATCH 2/3] vivid: add module option to set request support mode Hans Verkuil
2021-07-29 13:36 ` [PATCH 3/3] videobuf2-core: sanity checks for requests and qbuf Hans Verkuil
2021-07-29 15:08   ` Ezequiel Garcia

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.