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
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
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
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
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
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