| Message ID | 20240314153226.197445-3-benjamin.gaignard@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add REMOVE_BUF ioctl | expand |
Em Thu, 14 Mar 2024 16:32:19 +0100 Benjamin Gaignard <benjamin.gaignard@collabora.com> escreveu: > Add 'min_reqbufs_allocation' field in the vb2_queue structure so drivers > can specify the minimum number of buffers to allocate when calling > VIDIOC_REQBUFS. > When initializing the queue, v4l2 core makes sure that the following > constraints are respected: > - the minimum number of buffers to allocate must be at least 2 because > one buffer is used by the hardware while the other is being processed > by userspace. > -if the driver needs 'min_queued_buffers' in the queue before calling > start_streaming(), then the minimum requirement is 'min_queued_buffers + 1' > to keep at least one buffer available for userspace. > > Simplify __vb2_init_fileio() by using 'min_reqbufs_allocation' directly > to avoid duplicating the minimum number of buffers to allocate computation. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> LGTM. Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org> > --- > .../media/common/videobuf2/videobuf2-core.c | 38 +++++++++++-------- > include/media/videobuf2-core.h | 15 +++++++- > 2 files changed, 37 insertions(+), 16 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index d8b3c04cb3b5..58c495b253ce 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -866,7 +866,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > /* > * Make sure the requested values and current defaults are sane. > */ > - num_buffers = max_t(unsigned int, *count, q->min_queued_buffers); > + num_buffers = max_t(unsigned int, *count, q->min_reqbufs_allocation); > num_buffers = min_t(unsigned int, num_buffers, q->max_num_buffers); > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > /* > @@ -918,7 +918,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > * There is no point in continuing if we can't allocate the minimum > * number of buffers needed by this vb2_queue. > */ > - if (allocated_buffers < q->min_queued_buffers) > + if (allocated_buffers < q->min_reqbufs_allocation) > ret = -ENOMEM; > > /* > @@ -2524,6 +2524,25 @@ int vb2_core_queue_init(struct vb2_queue *q) > if (WARN_ON(q->supports_requests && q->min_queued_buffers)) > return -EINVAL; > > + /* > + * The minimum requirement is 2: one buffer is used > + * by the hardware while the other is being processed by userspace. > + */ > + if (q->min_reqbufs_allocation < 2) > + q->min_reqbufs_allocation = 2; > + > + /* > + * If the driver needs 'min_queued_buffers' in the queue before > + * calling start_streaming() then the minimum requirement is > + * 'min_queued_buffers + 1' to keep at least one buffer available > + * for userspace. > + */ > + if (q->min_reqbufs_allocation < q->min_queued_buffers + 1) > + q->min_reqbufs_allocation = q->min_queued_buffers + 1; > + > + if (WARN_ON(q->min_reqbufs_allocation > q->max_num_buffers)) > + return -EINVAL; > + > INIT_LIST_HEAD(&q->queued_list); > INIT_LIST_HEAD(&q->done_list); > spin_lock_init(&q->done_lock); > @@ -2717,7 +2736,6 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) > struct vb2_fileio_data *fileio; > struct vb2_buffer *vb; > int i, ret; > - unsigned int count = 0; > > /* > * Sanity check > @@ -2738,18 +2756,8 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) > if (q->streaming || vb2_get_num_buffers(q) > 0) > return -EBUSY; > > - /* > - * Start with q->min_queued_buffers + 1, driver can increase it in > - * queue_setup() > - * > - * 'min_queued_buffers' buffers need to be queued up before you > - * can start streaming, plus 1 for userspace (or in this case, > - * kernelspace) processing. > - */ > - count = max(2, q->min_queued_buffers + 1); > - > dprintk(q, 3, "setting up file io: mode %s, count %d, read_once %d, write_immediately %d\n", > - (read) ? "read" : "write", count, q->fileio_read_once, > + (read) ? "read" : "write", q->min_reqbufs_allocation, q->fileio_read_once, > q->fileio_write_immediately); > > fileio = kzalloc(sizeof(*fileio), GFP_KERNEL); > @@ -2763,7 +2771,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) > * Request buffers and use MMAP type to force driver > * to allocate buffers by itself. > */ > - fileio->count = count; > + fileio->count = q->min_reqbufs_allocation; > fileio->memory = VB2_MEMORY_MMAP; > fileio->type = q->type; > q->fileio = fileio; > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 667bf9ee1101..4a8b9135cec8 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -549,9 +549,21 @@ struct vb2_buf_ops { > * @start_streaming can be called. Used when a DMA engine > * cannot be started unless at least this number of buffers > * have been queued into the driver. > - * VIDIOC_REQBUFS will ensure at least @min_queued_buffers > + * VIDIOC_REQBUFS will ensure at least @min_queued_buffers + 1 > * buffers will be allocated. Note that VIDIOC_CREATE_BUFS will not > * modify the requested buffer count. > + * @min_reqbufs_allocation: the minimum number of buffers to be allocated when > + * calling VIDIOC_REQBUFS. Note that VIDIOC_CREATE_BUFS will *not* > + * modify the requested buffer count and does not use this field. > + * Drivers can set this if there has to be a certain number of > + * buffers available for the hardware to work effectively. > + * This allows calling VIDIOC_REQBUFS with a buffer count of 1 and > + * it will be automatically adjusted to a workable buffer count. > + * If set, then @min_reqbufs_allocation must be larger than > + * @min_queued_buffers + 1. > + * If this field is > 3, then it is highly recommended that the > + * driver implements the V4L2_CID_MIN_BUFFERS_FOR_CAPTURE/OUTPUT > + * control. > * @alloc_devs: &struct device memory type/allocator-specific per-plane device > */ > /* > @@ -622,6 +634,7 @@ struct vb2_queue { > u32 timestamp_flags; > gfp_t gfp_flags; > u32 min_queued_buffers; > + u32 min_reqbufs_allocation; > > struct device *alloc_devs[VB2_MAX_PLANES]; > Thanks, Mauro
Hi Benjamin, On Thu, Mar 14, 2024 at 04:32:19PM +0100, Benjamin Gaignard wrote: > Add 'min_reqbufs_allocation' field in the vb2_queue structure so drivers > can specify the minimum number of buffers to allocate when calling > VIDIOC_REQBUFS. > When initializing the queue, v4l2 core makes sure that the following > constraints are respected: > - the minimum number of buffers to allocate must be at least 2 because > one buffer is used by the hardware while the other is being processed > by userspace. This breaks userspace for the Renesas vsp1 driver :-( Unit tests fail, as some of them rely on operation with a single buffer. The vsp1 is a memory-to-memory processing engine, so operating with a single buffer is fine in some use cases. I would argue that for live capture devices there are valid use cases to operate with a single buffer too. Changing this by default will break use cases. How can we relax this check ? Should I simply submit a patch that lowers the minimum to one buffer ? > -if the driver needs 'min_queued_buffers' in the queue before calling > start_streaming(), then the minimum requirement is 'min_queued_buffers + 1' > to keep at least one buffer available for userspace. > > Simplify __vb2_init_fileio() by using 'min_reqbufs_allocation' directly > to avoid duplicating the minimum number of buffers to allocate computation. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > .../media/common/videobuf2/videobuf2-core.c | 38 +++++++++++-------- > include/media/videobuf2-core.h | 15 +++++++- > 2 files changed, 37 insertions(+), 16 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index d8b3c04cb3b5..58c495b253ce 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -866,7 +866,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > /* > * Make sure the requested values and current defaults are sane. > */ > - num_buffers = max_t(unsigned int, *count, q->min_queued_buffers); > + num_buffers = max_t(unsigned int, *count, q->min_reqbufs_allocation); > num_buffers = min_t(unsigned int, num_buffers, q->max_num_buffers); > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > /* > @@ -918,7 +918,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > * There is no point in continuing if we can't allocate the minimum > * number of buffers needed by this vb2_queue. > */ > - if (allocated_buffers < q->min_queued_buffers) > + if (allocated_buffers < q->min_reqbufs_allocation) > ret = -ENOMEM; > > /* > @@ -2524,6 +2524,25 @@ int vb2_core_queue_init(struct vb2_queue *q) > if (WARN_ON(q->supports_requests && q->min_queued_buffers)) > return -EINVAL; > > + /* > + * The minimum requirement is 2: one buffer is used > + * by the hardware while the other is being processed by userspace. > + */ > + if (q->min_reqbufs_allocation < 2) > + q->min_reqbufs_allocation = 2; > + > + /* > + * If the driver needs 'min_queued_buffers' in the queue before > + * calling start_streaming() then the minimum requirement is > + * 'min_queued_buffers + 1' to keep at least one buffer available > + * for userspace. > + */ > + if (q->min_reqbufs_allocation < q->min_queued_buffers + 1) > + q->min_reqbufs_allocation = q->min_queued_buffers + 1; > + > + if (WARN_ON(q->min_reqbufs_allocation > q->max_num_buffers)) > + return -EINVAL; > + > INIT_LIST_HEAD(&q->queued_list); > INIT_LIST_HEAD(&q->done_list); > spin_lock_init(&q->done_lock); > @@ -2717,7 +2736,6 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) > struct vb2_fileio_data *fileio; > struct vb2_buffer *vb; > int i, ret; > - unsigned int count = 0; > > /* > * Sanity check > @@ -2738,18 +2756,8 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) > if (q->streaming || vb2_get_num_buffers(q) > 0) > return -EBUSY; > > - /* > - * Start with q->min_queued_buffers + 1, driver can increase it in > - * queue_setup() > - * > - * 'min_queued_buffers' buffers need to be queued up before you > - * can start streaming, plus 1 for userspace (or in this case, > - * kernelspace) processing. > - */ > - count = max(2, q->min_queued_buffers + 1); > - > dprintk(q, 3, "setting up file io: mode %s, count %d, read_once %d, write_immediately %d\n", > - (read) ? "read" : "write", count, q->fileio_read_once, > + (read) ? "read" : "write", q->min_reqbufs_allocation, q->fileio_read_once, > q->fileio_write_immediately); > > fileio = kzalloc(sizeof(*fileio), GFP_KERNEL); > @@ -2763,7 +2771,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) > * Request buffers and use MMAP type to force driver > * to allocate buffers by itself. > */ > - fileio->count = count; > + fileio->count = q->min_reqbufs_allocation; > fileio->memory = VB2_MEMORY_MMAP; > fileio->type = q->type; > q->fileio = fileio; > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 667bf9ee1101..4a8b9135cec8 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -549,9 +549,21 @@ struct vb2_buf_ops { > * @start_streaming can be called. Used when a DMA engine > * cannot be started unless at least this number of buffers > * have been queued into the driver. > - * VIDIOC_REQBUFS will ensure at least @min_queued_buffers > + * VIDIOC_REQBUFS will ensure at least @min_queued_buffers + 1 > * buffers will be allocated. Note that VIDIOC_CREATE_BUFS will not > * modify the requested buffer count. > + * @min_reqbufs_allocation: the minimum number of buffers to be allocated when > + * calling VIDIOC_REQBUFS. Note that VIDIOC_CREATE_BUFS will *not* > + * modify the requested buffer count and does not use this field. > + * Drivers can set this if there has to be a certain number of > + * buffers available for the hardware to work effectively. > + * This allows calling VIDIOC_REQBUFS with a buffer count of 1 and > + * it will be automatically adjusted to a workable buffer count. > + * If set, then @min_reqbufs_allocation must be larger than > + * @min_queued_buffers + 1. > + * If this field is > 3, then it is highly recommended that the > + * driver implements the V4L2_CID_MIN_BUFFERS_FOR_CAPTURE/OUTPUT > + * control. > * @alloc_devs: &struct device memory type/allocator-specific per-plane device > */ > /* > @@ -622,6 +634,7 @@ struct vb2_queue { > u32 timestamp_flags; > gfp_t gfp_flags; > u32 min_queued_buffers; > + u32 min_reqbufs_allocation; > > struct device *alloc_devs[VB2_MAX_PLANES]; >
On Thu, Aug 22, 2024 at 8:28 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Benjamin, > > On Thu, Mar 14, 2024 at 04:32:19PM +0100, Benjamin Gaignard wrote: > > Add 'min_reqbufs_allocation' field in the vb2_queue structure so drivers > > can specify the minimum number of buffers to allocate when calling > > VIDIOC_REQBUFS. > > When initializing the queue, v4l2 core makes sure that the following > > constraints are respected: > > - the minimum number of buffers to allocate must be at least 2 because > > one buffer is used by the hardware while the other is being processed > > by userspace. > > This breaks userspace for the Renesas vsp1 driver :-( Unit tests fail, > as some of them rely on operation with a single buffer. > > The vsp1 is a memory-to-memory processing engine, so operating with a > single buffer is fine in some use cases. I would argue that for live > capture devices there are valid use cases to operate with a single > buffer too. Changing this by default will break use cases. > > How can we relax this check ? Should I simply submit a patch that lowers > the minimum to one buffer ? > Uhm, I didn't notice this when reading this series. (Actually if I recall correctly, originally it didn't have this restriction.) Indeed, I don't see what's wrong with just having 1 buffer. If performance is not a concern, it's okay to just serialize the operation on 1 buffer. If you would be so nice to send it, I think a patch that changes min_reqbufs_allocation to max(max(1, min_reqbufs_allocation), min_queued_buffers)) would be good. That said, Benjamin, Hans, are we missing something by any chance? :) Best, Tomasz > > -if the driver needs 'min_queued_buffers' in the queue before calling > > start_streaming(), then the minimum requirement is 'min_queued_buffers + 1' > > to keep at least one buffer available for userspace. > > > > Simplify __vb2_init_fileio() by using 'min_reqbufs_allocation' directly > > to avoid duplicating the minimum number of buffers to allocate computation. > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > > --- > > .../media/common/videobuf2/videobuf2-core.c | 38 +++++++++++-------- > > include/media/videobuf2-core.h | 15 +++++++- > > 2 files changed, 37 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > > index d8b3c04cb3b5..58c495b253ce 100644 > > --- a/drivers/media/common/videobuf2/videobuf2-core.c > > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > > @@ -866,7 +866,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > > /* > > * Make sure the requested values and current defaults are sane. > > */ > > - num_buffers = max_t(unsigned int, *count, q->min_queued_buffers); > > + num_buffers = max_t(unsigned int, *count, q->min_reqbufs_allocation); > > num_buffers = min_t(unsigned int, num_buffers, q->max_num_buffers); > > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > > /* > > @@ -918,7 +918,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > > * There is no point in continuing if we can't allocate the minimum > > * number of buffers needed by this vb2_queue. > > */ > > - if (allocated_buffers < q->min_queued_buffers) > > + if (allocated_buffers < q->min_reqbufs_allocation) > > ret = -ENOMEM; > > > > /* > > @@ -2524,6 +2524,25 @@ int vb2_core_queue_init(struct vb2_queue *q) > > if (WARN_ON(q->supports_requests && q->min_queued_buffers)) > > return -EINVAL; > > > > + /* > > + * The minimum requirement is 2: one buffer is used > > + * by the hardware while the other is being processed by userspace. > > + */ > > + if (q->min_reqbufs_allocation < 2) > > + q->min_reqbufs_allocation = 2; > > + > > + /* > > + * If the driver needs 'min_queued_buffers' in the queue before > > + * calling start_streaming() then the minimum requirement is > > + * 'min_queued_buffers + 1' to keep at least one buffer available > > + * for userspace. > > + */ > > + if (q->min_reqbufs_allocation < q->min_queued_buffers + 1) > > + q->min_reqbufs_allocation = q->min_queued_buffers + 1; > > + > > + if (WARN_ON(q->min_reqbufs_allocation > q->max_num_buffers)) > > + return -EINVAL; > > + > > INIT_LIST_HEAD(&q->queued_list); > > INIT_LIST_HEAD(&q->done_list); > > spin_lock_init(&q->done_lock); > > @@ -2717,7 +2736,6 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) > > struct vb2_fileio_data *fileio; > > struct vb2_buffer *vb; > > int i, ret; > > - unsigned int count = 0; > > > > /* > > * Sanity check > > @@ -2738,18 +2756,8 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) > > if (q->streaming || vb2_get_num_buffers(q) > 0) > > return -EBUSY; > > > > - /* > > - * Start with q->min_queued_buffers + 1, driver can increase it in > > - * queue_setup() > > - * > > - * 'min_queued_buffers' buffers need to be queued up before you > > - * can start streaming, plus 1 for userspace (or in this case, > > - * kernelspace) processing. > > - */ > > - count = max(2, q->min_queued_buffers + 1); > > - > > dprintk(q, 3, "setting up file io: mode %s, count %d, read_once %d, write_immediately %d\n", > > - (read) ? "read" : "write", count, q->fileio_read_once, > > + (read) ? "read" : "write", q->min_reqbufs_allocation, q->fileio_read_once, > > q->fileio_write_immediately); > > > > fileio = kzalloc(sizeof(*fileio), GFP_KERNEL); > > @@ -2763,7 +2771,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) > > * Request buffers and use MMAP type to force driver > > * to allocate buffers by itself. > > */ > > - fileio->count = count; > > + fileio->count = q->min_reqbufs_allocation; > > fileio->memory = VB2_MEMORY_MMAP; > > fileio->type = q->type; > > q->fileio = fileio; > > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > > index 667bf9ee1101..4a8b9135cec8 100644 > > --- a/include/media/videobuf2-core.h > > +++ b/include/media/videobuf2-core.h > > @@ -549,9 +549,21 @@ struct vb2_buf_ops { > > * @start_streaming can be called. Used when a DMA engine > > * cannot be started unless at least this number of buffers > > * have been queued into the driver. > > - * VIDIOC_REQBUFS will ensure at least @min_queued_buffers > > + * VIDIOC_REQBUFS will ensure at least @min_queued_buffers + 1 > > * buffers will be allocated. Note that VIDIOC_CREATE_BUFS will not > > * modify the requested buffer count. > > + * @min_reqbufs_allocation: the minimum number of buffers to be allocated when > > + * calling VIDIOC_REQBUFS. Note that VIDIOC_CREATE_BUFS will *not* > > + * modify the requested buffer count and does not use this field. > > + * Drivers can set this if there has to be a certain number of > > + * buffers available for the hardware to work effectively. > > + * This allows calling VIDIOC_REQBUFS with a buffer count of 1 and > > + * it will be automatically adjusted to a workable buffer count. > > + * If set, then @min_reqbufs_allocation must be larger than > > + * @min_queued_buffers + 1. > > + * If this field is > 3, then it is highly recommended that the > > + * driver implements the V4L2_CID_MIN_BUFFERS_FOR_CAPTURE/OUTPUT > > + * control. > > * @alloc_devs: &struct device memory type/allocator-specific per-plane device > > */ > > /* > > @@ -622,6 +634,7 @@ struct vb2_queue { > > u32 timestamp_flags; > > gfp_t gfp_flags; > > u32 min_queued_buffers; > > + u32 min_reqbufs_allocation; > > > > struct device *alloc_devs[VB2_MAX_PLANES]; > > > > -- > Regards, > > Laurent Pinchart
On 22/08/2024 03:29, Tomasz Figa wrote: > On Thu, Aug 22, 2024 at 8:28 AM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: >> >> Hi Benjamin, >> >> On Thu, Mar 14, 2024 at 04:32:19PM +0100, Benjamin Gaignard wrote: >>> Add 'min_reqbufs_allocation' field in the vb2_queue structure so drivers >>> can specify the minimum number of buffers to allocate when calling >>> VIDIOC_REQBUFS. >>> When initializing the queue, v4l2 core makes sure that the following >>> constraints are respected: >>> - the minimum number of buffers to allocate must be at least 2 because >>> one buffer is used by the hardware while the other is being processed >>> by userspace. >> >> This breaks userspace for the Renesas vsp1 driver :-( Unit tests fail, >> as some of them rely on operation with a single buffer. >> >> The vsp1 is a memory-to-memory processing engine, so operating with a >> single buffer is fine in some use cases. I would argue that for live >> capture devices there are valid use cases to operate with a single >> buffer too. Changing this by default will break use cases. >> >> How can we relax this check ? Should I simply submit a patch that lowers >> the minimum to one buffer ? >> > > Uhm, I didn't notice this when reading this series. (Actually if I > recall correctly, originally it didn't have this restriction.) > > Indeed, I don't see what's wrong with just having 1 buffer. If > performance is not a concern, it's okay to just serialize the > operation on 1 buffer. > > If you would be so nice to send it, I think a patch that changes > min_reqbufs_allocation to max(max(1, min_reqbufs_allocation), > min_queued_buffers)) would be good. > > That said, Benjamin, Hans, are we missing something by any chance? :) See my proposal below: > > Best, > Tomasz > >>> -if the driver needs 'min_queued_buffers' in the queue before calling >>> start_streaming(), then the minimum requirement is 'min_queued_buffers + 1' >>> to keep at least one buffer available for userspace. >>> >>> Simplify __vb2_init_fileio() by using 'min_reqbufs_allocation' directly >>> to avoid duplicating the minimum number of buffers to allocate computation. >>> >>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >>> --- >>> .../media/common/videobuf2/videobuf2-core.c | 38 +++++++++++-------- >>> include/media/videobuf2-core.h | 15 +++++++- >>> 2 files changed, 37 insertions(+), 16 deletions(-) >>> >>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c >>> index d8b3c04cb3b5..58c495b253ce 100644 >>> --- a/drivers/media/common/videobuf2/videobuf2-core.c >>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c >>> @@ -866,7 +866,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, >>> /* >>> * Make sure the requested values and current defaults are sane. >>> */ >>> - num_buffers = max_t(unsigned int, *count, q->min_queued_buffers); >>> + num_buffers = max_t(unsigned int, *count, q->min_reqbufs_allocation); >>> num_buffers = min_t(unsigned int, num_buffers, q->max_num_buffers); >>> memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); >>> /* >>> @@ -918,7 +918,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, >>> * There is no point in continuing if we can't allocate the minimum >>> * number of buffers needed by this vb2_queue. >>> */ >>> - if (allocated_buffers < q->min_queued_buffers) >>> + if (allocated_buffers < q->min_reqbufs_allocation) >>> ret = -ENOMEM; >>> >>> /* >>> @@ -2524,6 +2524,25 @@ int vb2_core_queue_init(struct vb2_queue *q) >>> if (WARN_ON(q->supports_requests && q->min_queued_buffers)) >>> return -EINVAL; >>> >>> + /* >>> + * The minimum requirement is 2: one buffer is used >>> + * by the hardware while the other is being processed by userspace. >>> + */ >>> + if (q->min_reqbufs_allocation < 2) This should be: if (!q->min_reqbufs_allocation) >>> + q->min_reqbufs_allocation = 2; and vsp1 should set q->min_reqbufs_allocation to 1. >>> + >>> + /* >>> + * If the driver needs 'min_queued_buffers' in the queue before >>> + * calling start_streaming() then the minimum requirement is >>> + * 'min_queued_buffers + 1' to keep at least one buffer available >>> + * for userspace. >>> + */ >>> + if (q->min_reqbufs_allocation < q->min_queued_buffers + 1) >>> + q->min_reqbufs_allocation = q->min_queued_buffers + 1; The reasoning behind all this is that VIDIOC_REBUFS is expected to allocate a sane (i.e. workable and efficient) number of buffers. So if the DMA engine requires at least X buffers queued, then the minimum reqbufs allocation is at least q->min_queued_buffers + 1, otherwise you would never be able to get a buffer back. That's also why q->min_reqbufs_allocation was set to a minimum of 2: one buffer in flight, one buffer processed by userspace. That made the queue_setup call simpler for quite a few drivers that manually set the buffer count to 2. For most drivers, allocating 2 buffers makes perfect sense. But if a driver sets q->min_reqbufs_allocation explicitly to 1, then that should be honored, and my proposed change above will do that. Laurent, if you agree with this, just post patches for this. Regards, Hans >>> + >>> + if (WARN_ON(q->min_reqbufs_allocation > q->max_num_buffers)) >>> + return -EINVAL; >>> + >>> INIT_LIST_HEAD(&q->queued_list); >>> INIT_LIST_HEAD(&q->done_list); >>> spin_lock_init(&q->done_lock); >>> @@ -2717,7 +2736,6 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) >>> struct vb2_fileio_data *fileio; >>> struct vb2_buffer *vb; >>> int i, ret; >>> - unsigned int count = 0; >>> >>> /* >>> * Sanity check >>> @@ -2738,18 +2756,8 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) >>> if (q->streaming || vb2_get_num_buffers(q) > 0) >>> return -EBUSY; >>> >>> - /* >>> - * Start with q->min_queued_buffers + 1, driver can increase it in >>> - * queue_setup() >>> - * >>> - * 'min_queued_buffers' buffers need to be queued up before you >>> - * can start streaming, plus 1 for userspace (or in this case, >>> - * kernelspace) processing. >>> - */ >>> - count = max(2, q->min_queued_buffers + 1); >>> - >>> dprintk(q, 3, "setting up file io: mode %s, count %d, read_once %d, write_immediately %d\n", >>> - (read) ? "read" : "write", count, q->fileio_read_once, >>> + (read) ? "read" : "write", q->min_reqbufs_allocation, q->fileio_read_once, >>> q->fileio_write_immediately); >>> >>> fileio = kzalloc(sizeof(*fileio), GFP_KERNEL); >>> @@ -2763,7 +2771,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) >>> * Request buffers and use MMAP type to force driver >>> * to allocate buffers by itself. >>> */ >>> - fileio->count = count; >>> + fileio->count = q->min_reqbufs_allocation; >>> fileio->memory = VB2_MEMORY_MMAP; >>> fileio->type = q->type; >>> q->fileio = fileio; >>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h >>> index 667bf9ee1101..4a8b9135cec8 100644 >>> --- a/include/media/videobuf2-core.h >>> +++ b/include/media/videobuf2-core.h >>> @@ -549,9 +549,21 @@ struct vb2_buf_ops { >>> * @start_streaming can be called. Used when a DMA engine >>> * cannot be started unless at least this number of buffers >>> * have been queued into the driver. >>> - * VIDIOC_REQBUFS will ensure at least @min_queued_buffers >>> + * VIDIOC_REQBUFS will ensure at least @min_queued_buffers + 1 >>> * buffers will be allocated. Note that VIDIOC_CREATE_BUFS will not >>> * modify the requested buffer count. >>> + * @min_reqbufs_allocation: the minimum number of buffers to be allocated when >>> + * calling VIDIOC_REQBUFS. Note that VIDIOC_CREATE_BUFS will *not* >>> + * modify the requested buffer count and does not use this field. >>> + * Drivers can set this if there has to be a certain number of >>> + * buffers available for the hardware to work effectively. >>> + * This allows calling VIDIOC_REQBUFS with a buffer count of 1 and >>> + * it will be automatically adjusted to a workable buffer count. >>> + * If set, then @min_reqbufs_allocation must be larger than >>> + * @min_queued_buffers + 1. >>> + * If this field is > 3, then it is highly recommended that the >>> + * driver implements the V4L2_CID_MIN_BUFFERS_FOR_CAPTURE/OUTPUT >>> + * control. >>> * @alloc_devs: &struct device memory type/allocator-specific per-plane device >>> */ >>> /* >>> @@ -622,6 +634,7 @@ struct vb2_queue { >>> u32 timestamp_flags; >>> gfp_t gfp_flags; >>> u32 min_queued_buffers; >>> + u32 min_reqbufs_allocation; >>> >>> struct device *alloc_devs[VB2_MAX_PLANES]; >>> >> >> -- >> Regards, >> >> Laurent Pinchart >
Hello, On Thu, Aug 22, 2024 at 08:11:17AM +0200, Hans Verkuil wrote: > On 22/08/2024 03:29, Tomasz Figa wrote: > > On Thu, Aug 22, 2024 at 8:28 AM Laurent Pinchart wrote: > >> On Thu, Mar 14, 2024 at 04:32:19PM +0100, Benjamin Gaignard wrote: > >>> Add 'min_reqbufs_allocation' field in the vb2_queue structure so drivers > >>> can specify the minimum number of buffers to allocate when calling > >>> VIDIOC_REQBUFS. > >>> When initializing the queue, v4l2 core makes sure that the following > >>> constraints are respected: > >>> - the minimum number of buffers to allocate must be at least 2 because > >>> one buffer is used by the hardware while the other is being processed > >>> by userspace. > >> > >> This breaks userspace for the Renesas vsp1 driver :-( Unit tests fail, > >> as some of them rely on operation with a single buffer. > >> > >> The vsp1 is a memory-to-memory processing engine, so operating with a > >> single buffer is fine in some use cases. I would argue that for live > >> capture devices there are valid use cases to operate with a single > >> buffer too. Changing this by default will break use cases. > >> > >> How can we relax this check ? Should I simply submit a patch that lowers > >> the minimum to one buffer ? > >> > > > > Uhm, I didn't notice this when reading this series. (Actually if I > > recall correctly, originally it didn't have this restriction.) > > > > Indeed, I don't see what's wrong with just having 1 buffer. If > > performance is not a concern, it's okay to just serialize the > > operation on 1 buffer. > > > > If you would be so nice to send it, I think a patch that changes > > min_reqbufs_allocation to max(max(1, min_reqbufs_allocation), > > min_queued_buffers)) would be good. > > > > That said, Benjamin, Hans, are we missing something by any chance? :) > > See my proposal below: > > >>> -if the driver needs 'min_queued_buffers' in the queue before calling > >>> start_streaming(), then the minimum requirement is 'min_queued_buffers + 1' > >>> to keep at least one buffer available for userspace. > >>> > >>> Simplify __vb2_init_fileio() by using 'min_reqbufs_allocation' directly > >>> to avoid duplicating the minimum number of buffers to allocate computation. > >>> > >>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > >>> --- > >>> .../media/common/videobuf2/videobuf2-core.c | 38 +++++++++++-------- > >>> include/media/videobuf2-core.h | 15 +++++++- > >>> 2 files changed, 37 insertions(+), 16 deletions(-) > >>> > >>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > >>> index d8b3c04cb3b5..58c495b253ce 100644 > >>> --- a/drivers/media/common/videobuf2/videobuf2-core.c > >>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c > >>> @@ -866,7 +866,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > >>> /* > >>> * Make sure the requested values and current defaults are sane. > >>> */ > >>> - num_buffers = max_t(unsigned int, *count, q->min_queued_buffers); > >>> + num_buffers = max_t(unsigned int, *count, q->min_reqbufs_allocation); > >>> num_buffers = min_t(unsigned int, num_buffers, q->max_num_buffers); > >>> memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > >>> /* > >>> @@ -918,7 +918,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > >>> * There is no point in continuing if we can't allocate the minimum > >>> * number of buffers needed by this vb2_queue. > >>> */ > >>> - if (allocated_buffers < q->min_queued_buffers) > >>> + if (allocated_buffers < q->min_reqbufs_allocation) > >>> ret = -ENOMEM; > >>> > >>> /* > >>> @@ -2524,6 +2524,25 @@ int vb2_core_queue_init(struct vb2_queue *q) > >>> if (WARN_ON(q->supports_requests && q->min_queued_buffers)) > >>> return -EINVAL; > >>> > >>> + /* > >>> + * The minimum requirement is 2: one buffer is used > >>> + * by the hardware while the other is being processed by userspace. > >>> + */ > >>> + if (q->min_reqbufs_allocation < 2) > > This should be: > > if (!q->min_reqbufs_allocation) > > >>> + q->min_reqbufs_allocation = 2; > > and vsp1 should set q->min_reqbufs_allocation to 1. That would work to fix the issue with vsp1, but I don't think it's enough. See below. > >>> + > >>> + /* > >>> + * If the driver needs 'min_queued_buffers' in the queue before > >>> + * calling start_streaming() then the minimum requirement is > >>> + * 'min_queued_buffers + 1' to keep at least one buffer available > >>> + * for userspace. > >>> + */ > >>> + if (q->min_reqbufs_allocation < q->min_queued_buffers + 1) > >>> + q->min_reqbufs_allocation = q->min_queued_buffers + 1; > > The reasoning behind all this is that VIDIOC_REBUFS is expected to allocate > a sane (i.e. workable and efficient) number of buffers. > > So if the DMA engine requires at least X buffers queued, then the minimum > reqbufs allocation is at least q->min_queued_buffers + 1, otherwise you > would never be able to get a buffer back. > > That's also why q->min_reqbufs_allocation was set to a minimum of 2: one > buffer in flight, one buffer processed by userspace. That made the queue_setup > call simpler for quite a few drivers that manually set the buffer count to 2. > For most drivers, allocating 2 buffers makes perfect sense. For devices that can absolutely not work with less than two buffers, setting the minimum to 2 is fine. That is however not the majority use case, and that's why setting the default in the vb2 core, overriding all the drivers that haven't been patched, doesn't sound like a good idea to me. Generally speaking, 2 is in many cases too low. In the common use case of capture and display, you will need a minimum of 3 buffers. Use cases are the responsibility of userspace, we shouldn't try to be too smart here. I actually want to lower the number of buffers and require camera drivers to support operating with a single buffer as a general rule. Drivers that hold on the last buffer until a new one is provided are very painful to use, they're causing issues with libcamera. > But if a driver sets q->min_reqbufs_allocation explicitly to 1, then that > should be honored, and my proposed change above will do that. > > Laurent, if you agree with this, just post patches for this. > > >>> + > >>> + if (WARN_ON(q->min_reqbufs_allocation > q->max_num_buffers)) > >>> + return -EINVAL; > >>> + > >>> INIT_LIST_HEAD(&q->queued_list); > >>> INIT_LIST_HEAD(&q->done_list); > >>> spin_lock_init(&q->done_lock); > >>> @@ -2717,7 +2736,6 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) > >>> struct vb2_fileio_data *fileio; > >>> struct vb2_buffer *vb; > >>> int i, ret; > >>> - unsigned int count = 0; > >>> > >>> /* > >>> * Sanity check > >>> @@ -2738,18 +2756,8 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) > >>> if (q->streaming || vb2_get_num_buffers(q) > 0) > >>> return -EBUSY; > >>> > >>> - /* > >>> - * Start with q->min_queued_buffers + 1, driver can increase it in > >>> - * queue_setup() > >>> - * > >>> - * 'min_queued_buffers' buffers need to be queued up before you > >>> - * can start streaming, plus 1 for userspace (or in this case, > >>> - * kernelspace) processing. > >>> - */ > >>> - count = max(2, q->min_queued_buffers + 1); > >>> - > >>> dprintk(q, 3, "setting up file io: mode %s, count %d, read_once %d, write_immediately %d\n", > >>> - (read) ? "read" : "write", count, q->fileio_read_once, > >>> + (read) ? "read" : "write", q->min_reqbufs_allocation, q->fileio_read_once, > >>> q->fileio_write_immediately); > >>> > >>> fileio = kzalloc(sizeof(*fileio), GFP_KERNEL); > >>> @@ -2763,7 +2771,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) > >>> * Request buffers and use MMAP type to force driver > >>> * to allocate buffers by itself. > >>> */ > >>> - fileio->count = count; > >>> + fileio->count = q->min_reqbufs_allocation; > >>> fileio->memory = VB2_MEMORY_MMAP; > >>> fileio->type = q->type; > >>> q->fileio = fileio; > >>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > >>> index 667bf9ee1101..4a8b9135cec8 100644 > >>> --- a/include/media/videobuf2-core.h > >>> +++ b/include/media/videobuf2-core.h > >>> @@ -549,9 +549,21 @@ struct vb2_buf_ops { > >>> * @start_streaming can be called. Used when a DMA engine > >>> * cannot be started unless at least this number of buffers > >>> * have been queued into the driver. > >>> - * VIDIOC_REQBUFS will ensure at least @min_queued_buffers > >>> + * VIDIOC_REQBUFS will ensure at least @min_queued_buffers + 1 > >>> * buffers will be allocated. Note that VIDIOC_CREATE_BUFS will not > >>> * modify the requested buffer count. > >>> + * @min_reqbufs_allocation: the minimum number of buffers to be allocated when > >>> + * calling VIDIOC_REQBUFS. Note that VIDIOC_CREATE_BUFS will *not* > >>> + * modify the requested buffer count and does not use this field. > >>> + * Drivers can set this if there has to be a certain number of > >>> + * buffers available for the hardware to work effectively. > >>> + * This allows calling VIDIOC_REQBUFS with a buffer count of 1 and > >>> + * it will be automatically adjusted to a workable buffer count. > >>> + * If set, then @min_reqbufs_allocation must be larger than > >>> + * @min_queued_buffers + 1. > >>> + * If this field is > 3, then it is highly recommended that the > >>> + * driver implements the V4L2_CID_MIN_BUFFERS_FOR_CAPTURE/OUTPUT > >>> + * control. > >>> * @alloc_devs: &struct device memory type/allocator-specific per-plane device > >>> */ > >>> /* > >>> @@ -622,6 +634,7 @@ struct vb2_queue { > >>> u32 timestamp_flags; > >>> gfp_t gfp_flags; > >>> u32 min_queued_buffers; > >>> + u32 min_reqbufs_allocation; > >>> > >>> struct device *alloc_devs[VB2_MAX_PLANES]; > >>>
On 22/08/2024 14:20, Laurent Pinchart wrote: > Hello, > > On Thu, Aug 22, 2024 at 08:11:17AM +0200, Hans Verkuil wrote: >> On 22/08/2024 03:29, Tomasz Figa wrote: >>> On Thu, Aug 22, 2024 at 8:28 AM Laurent Pinchart wrote: >>>> On Thu, Mar 14, 2024 at 04:32:19PM +0100, Benjamin Gaignard wrote: >>>>> Add 'min_reqbufs_allocation' field in the vb2_queue structure so drivers >>>>> can specify the minimum number of buffers to allocate when calling >>>>> VIDIOC_REQBUFS. >>>>> When initializing the queue, v4l2 core makes sure that the following >>>>> constraints are respected: >>>>> - the minimum number of buffers to allocate must be at least 2 because >>>>> one buffer is used by the hardware while the other is being processed >>>>> by userspace. >>>> >>>> This breaks userspace for the Renesas vsp1 driver :-( Unit tests fail, >>>> as some of them rely on operation with a single buffer. >>>> >>>> The vsp1 is a memory-to-memory processing engine, so operating with a >>>> single buffer is fine in some use cases. I would argue that for live >>>> capture devices there are valid use cases to operate with a single >>>> buffer too. Changing this by default will break use cases. >>>> >>>> How can we relax this check ? Should I simply submit a patch that lowers >>>> the minimum to one buffer ? >>>> >>> >>> Uhm, I didn't notice this when reading this series. (Actually if I >>> recall correctly, originally it didn't have this restriction.) >>> >>> Indeed, I don't see what's wrong with just having 1 buffer. If >>> performance is not a concern, it's okay to just serialize the >>> operation on 1 buffer. >>> >>> If you would be so nice to send it, I think a patch that changes >>> min_reqbufs_allocation to max(max(1, min_reqbufs_allocation), >>> min_queued_buffers)) would be good. >>> >>> That said, Benjamin, Hans, are we missing something by any chance? :) >> >> See my proposal below: >> >>>>> -if the driver needs 'min_queued_buffers' in the queue before calling >>>>> start_streaming(), then the minimum requirement is 'min_queued_buffers + 1' >>>>> to keep at least one buffer available for userspace. >>>>> >>>>> Simplify __vb2_init_fileio() by using 'min_reqbufs_allocation' directly >>>>> to avoid duplicating the minimum number of buffers to allocate computation. >>>>> >>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >>>>> --- >>>>> .../media/common/videobuf2/videobuf2-core.c | 38 +++++++++++-------- >>>>> include/media/videobuf2-core.h | 15 +++++++- >>>>> 2 files changed, 37 insertions(+), 16 deletions(-) >>>>> >>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c >>>>> index d8b3c04cb3b5..58c495b253ce 100644 >>>>> --- a/drivers/media/common/videobuf2/videobuf2-core.c >>>>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c >>>>> @@ -866,7 +866,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, >>>>> /* >>>>> * Make sure the requested values and current defaults are sane. >>>>> */ >>>>> - num_buffers = max_t(unsigned int, *count, q->min_queued_buffers); >>>>> + num_buffers = max_t(unsigned int, *count, q->min_reqbufs_allocation); >>>>> num_buffers = min_t(unsigned int, num_buffers, q->max_num_buffers); >>>>> memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); >>>>> /* >>>>> @@ -918,7 +918,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, >>>>> * There is no point in continuing if we can't allocate the minimum >>>>> * number of buffers needed by this vb2_queue. >>>>> */ >>>>> - if (allocated_buffers < q->min_queued_buffers) >>>>> + if (allocated_buffers < q->min_reqbufs_allocation) >>>>> ret = -ENOMEM; >>>>> >>>>> /* >>>>> @@ -2524,6 +2524,25 @@ int vb2_core_queue_init(struct vb2_queue *q) >>>>> if (WARN_ON(q->supports_requests && q->min_queued_buffers)) >>>>> return -EINVAL; >>>>> >>>>> + /* >>>>> + * The minimum requirement is 2: one buffer is used >>>>> + * by the hardware while the other is being processed by userspace. >>>>> + */ >>>>> + if (q->min_reqbufs_allocation < 2) >> >> This should be: >> >> if (!q->min_reqbufs_allocation) >> >>>>> + q->min_reqbufs_allocation = 2; >> >> and vsp1 should set q->min_reqbufs_allocation to 1. > > That would work to fix the issue with vsp1, but I don't think it's > enough. See below. > >>>>> + >>>>> + /* >>>>> + * If the driver needs 'min_queued_buffers' in the queue before >>>>> + * calling start_streaming() then the minimum requirement is >>>>> + * 'min_queued_buffers + 1' to keep at least one buffer available >>>>> + * for userspace. >>>>> + */ >>>>> + if (q->min_reqbufs_allocation < q->min_queued_buffers + 1) >>>>> + q->min_reqbufs_allocation = q->min_queued_buffers + 1; >> >> The reasoning behind all this is that VIDIOC_REBUFS is expected to allocate >> a sane (i.e. workable and efficient) number of buffers. >> >> So if the DMA engine requires at least X buffers queued, then the minimum >> reqbufs allocation is at least q->min_queued_buffers + 1, otherwise you >> would never be able to get a buffer back. >> >> That's also why q->min_reqbufs_allocation was set to a minimum of 2: one >> buffer in flight, one buffer processed by userspace. That made the queue_setup >> call simpler for quite a few drivers that manually set the buffer count to 2. >> For most drivers, allocating 2 buffers makes perfect sense. > > For devices that can absolutely not work with less than two buffers, > setting the minimum to 2 is fine. That is however not the majority use > case, and that's why setting the default in the vb2 core, overriding all > the drivers that haven't been patched, doesn't sound like a good idea to > me. > > Generally speaking, 2 is in many cases too low. In the common use case > of capture and display, you will need a minimum of 3 buffers. Use cases > are the responsibility of userspace, we shouldn't try to be too smart > here. > > I actually want to lower the number of buffers and require camera > drivers to support operating with a single buffer as a general rule. > Drivers that hold on the last buffer until a new one is provided are > very painful to use, they're causing issues with libcamera. I thought about it some more, and I agree with you. So just post a patch to drop that 'if (q->min_reqbufs_allocation < 2)' bit. Regards, Hans > >> But if a driver sets q->min_reqbufs_allocation explicitly to 1, then that >> should be honored, and my proposed change above will do that. >> >> Laurent, if you agree with this, just post patches for this. >> >>>>> + >>>>> + if (WARN_ON(q->min_reqbufs_allocation > q->max_num_buffers)) >>>>> + return -EINVAL; >>>>> + >>>>> INIT_LIST_HEAD(&q->queued_list); >>>>> INIT_LIST_HEAD(&q->done_list); >>>>> spin_lock_init(&q->done_lock); >>>>> @@ -2717,7 +2736,6 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) >>>>> struct vb2_fileio_data *fileio; >>>>> struct vb2_buffer *vb; >>>>> int i, ret; >>>>> - unsigned int count = 0; >>>>> >>>>> /* >>>>> * Sanity check >>>>> @@ -2738,18 +2756,8 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) >>>>> if (q->streaming || vb2_get_num_buffers(q) > 0) >>>>> return -EBUSY; >>>>> >>>>> - /* >>>>> - * Start with q->min_queued_buffers + 1, driver can increase it in >>>>> - * queue_setup() >>>>> - * >>>>> - * 'min_queued_buffers' buffers need to be queued up before you >>>>> - * can start streaming, plus 1 for userspace (or in this case, >>>>> - * kernelspace) processing. >>>>> - */ >>>>> - count = max(2, q->min_queued_buffers + 1); >>>>> - >>>>> dprintk(q, 3, "setting up file io: mode %s, count %d, read_once %d, write_immediately %d\n", >>>>> - (read) ? "read" : "write", count, q->fileio_read_once, >>>>> + (read) ? "read" : "write", q->min_reqbufs_allocation, q->fileio_read_once, >>>>> q->fileio_write_immediately); >>>>> >>>>> fileio = kzalloc(sizeof(*fileio), GFP_KERNEL); >>>>> @@ -2763,7 +2771,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) >>>>> * Request buffers and use MMAP type to force driver >>>>> * to allocate buffers by itself. >>>>> */ >>>>> - fileio->count = count; >>>>> + fileio->count = q->min_reqbufs_allocation; >>>>> fileio->memory = VB2_MEMORY_MMAP; >>>>> fileio->type = q->type; >>>>> q->fileio = fileio; >>>>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h >>>>> index 667bf9ee1101..4a8b9135cec8 100644 >>>>> --- a/include/media/videobuf2-core.h >>>>> +++ b/include/media/videobuf2-core.h >>>>> @@ -549,9 +549,21 @@ struct vb2_buf_ops { >>>>> * @start_streaming can be called. Used when a DMA engine >>>>> * cannot be started unless at least this number of buffers >>>>> * have been queued into the driver. >>>>> - * VIDIOC_REQBUFS will ensure at least @min_queued_buffers >>>>> + * VIDIOC_REQBUFS will ensure at least @min_queued_buffers + 1 >>>>> * buffers will be allocated. Note that VIDIOC_CREATE_BUFS will not >>>>> * modify the requested buffer count. >>>>> + * @min_reqbufs_allocation: the minimum number of buffers to be allocated when >>>>> + * calling VIDIOC_REQBUFS. Note that VIDIOC_CREATE_BUFS will *not* >>>>> + * modify the requested buffer count and does not use this field. >>>>> + * Drivers can set this if there has to be a certain number of >>>>> + * buffers available for the hardware to work effectively. >>>>> + * This allows calling VIDIOC_REQBUFS with a buffer count of 1 and >>>>> + * it will be automatically adjusted to a workable buffer count. >>>>> + * If set, then @min_reqbufs_allocation must be larger than >>>>> + * @min_queued_buffers + 1. >>>>> + * If this field is > 3, then it is highly recommended that the >>>>> + * driver implements the V4L2_CID_MIN_BUFFERS_FOR_CAPTURE/OUTPUT >>>>> + * control. >>>>> * @alloc_devs: &struct device memory type/allocator-specific per-plane device >>>>> */ >>>>> /* >>>>> @@ -622,6 +634,7 @@ struct vb2_queue { >>>>> u32 timestamp_flags; >>>>> gfp_t gfp_flags; >>>>> u32 min_queued_buffers; >>>>> + u32 min_reqbufs_allocation; >>>>> >>>>> struct device *alloc_devs[VB2_MAX_PLANES]; >>>>> >
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index d8b3c04cb3b5..58c495b253ce 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -866,7 +866,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, /* * Make sure the requested values and current defaults are sane. */ - num_buffers = max_t(unsigned int, *count, q->min_queued_buffers); + num_buffers = max_t(unsigned int, *count, q->min_reqbufs_allocation); num_buffers = min_t(unsigned int, num_buffers, q->max_num_buffers); memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); /* @@ -918,7 +918,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, * There is no point in continuing if we can't allocate the minimum * number of buffers needed by this vb2_queue. */ - if (allocated_buffers < q->min_queued_buffers) + if (allocated_buffers < q->min_reqbufs_allocation) ret = -ENOMEM; /* @@ -2524,6 +2524,25 @@ int vb2_core_queue_init(struct vb2_queue *q) if (WARN_ON(q->supports_requests && q->min_queued_buffers)) return -EINVAL; + /* + * The minimum requirement is 2: one buffer is used + * by the hardware while the other is being processed by userspace. + */ + if (q->min_reqbufs_allocation < 2) + q->min_reqbufs_allocation = 2; + + /* + * If the driver needs 'min_queued_buffers' in the queue before + * calling start_streaming() then the minimum requirement is + * 'min_queued_buffers + 1' to keep at least one buffer available + * for userspace. + */ + if (q->min_reqbufs_allocation < q->min_queued_buffers + 1) + q->min_reqbufs_allocation = q->min_queued_buffers + 1; + + if (WARN_ON(q->min_reqbufs_allocation > q->max_num_buffers)) + return -EINVAL; + INIT_LIST_HEAD(&q->queued_list); INIT_LIST_HEAD(&q->done_list); spin_lock_init(&q->done_lock); @@ -2717,7 +2736,6 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) struct vb2_fileio_data *fileio; struct vb2_buffer *vb; int i, ret; - unsigned int count = 0; /* * Sanity check @@ -2738,18 +2756,8 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) if (q->streaming || vb2_get_num_buffers(q) > 0) return -EBUSY; - /* - * Start with q->min_queued_buffers + 1, driver can increase it in - * queue_setup() - * - * 'min_queued_buffers' buffers need to be queued up before you - * can start streaming, plus 1 for userspace (or in this case, - * kernelspace) processing. - */ - count = max(2, q->min_queued_buffers + 1); - dprintk(q, 3, "setting up file io: mode %s, count %d, read_once %d, write_immediately %d\n", - (read) ? "read" : "write", count, q->fileio_read_once, + (read) ? "read" : "write", q->min_reqbufs_allocation, q->fileio_read_once, q->fileio_write_immediately); fileio = kzalloc(sizeof(*fileio), GFP_KERNEL); @@ -2763,7 +2771,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) * Request buffers and use MMAP type to force driver * to allocate buffers by itself. */ - fileio->count = count; + fileio->count = q->min_reqbufs_allocation; fileio->memory = VB2_MEMORY_MMAP; fileio->type = q->type; q->fileio = fileio; diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index 667bf9ee1101..4a8b9135cec8 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -549,9 +549,21 @@ struct vb2_buf_ops { * @start_streaming can be called. Used when a DMA engine * cannot be started unless at least this number of buffers * have been queued into the driver. - * VIDIOC_REQBUFS will ensure at least @min_queued_buffers + * VIDIOC_REQBUFS will ensure at least @min_queued_buffers + 1 * buffers will be allocated. Note that VIDIOC_CREATE_BUFS will not * modify the requested buffer count. + * @min_reqbufs_allocation: the minimum number of buffers to be allocated when + * calling VIDIOC_REQBUFS. Note that VIDIOC_CREATE_BUFS will *not* + * modify the requested buffer count and does not use this field. + * Drivers can set this if there has to be a certain number of + * buffers available for the hardware to work effectively. + * This allows calling VIDIOC_REQBUFS with a buffer count of 1 and + * it will be automatically adjusted to a workable buffer count. + * If set, then @min_reqbufs_allocation must be larger than + * @min_queued_buffers + 1. + * If this field is > 3, then it is highly recommended that the + * driver implements the V4L2_CID_MIN_BUFFERS_FOR_CAPTURE/OUTPUT + * control. * @alloc_devs: &struct device memory type/allocator-specific per-plane device */ /* @@ -622,6 +634,7 @@ struct vb2_queue { u32 timestamp_flags; gfp_t gfp_flags; u32 min_queued_buffers; + u32 min_reqbufs_allocation; struct device *alloc_devs[VB2_MAX_PLANES];
Add 'min_reqbufs_allocation' field in the vb2_queue structure so drivers can specify the minimum number of buffers to allocate when calling VIDIOC_REQBUFS. When initializing the queue, v4l2 core makes sure that the following constraints are respected: - the minimum number of buffers to allocate must be at least 2 because one buffer is used by the hardware while the other is being processed by userspace. -if the driver needs 'min_queued_buffers' in the queue before calling start_streaming(), then the minimum requirement is 'min_queued_buffers + 1' to keep at least one buffer available for userspace. Simplify __vb2_init_fileio() by using 'min_reqbufs_allocation' directly to avoid duplicating the minimum number of buffers to allocate computation. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> --- .../media/common/videobuf2/videobuf2-core.c | 38 +++++++++++-------- include/media/videobuf2-core.h | 15 +++++++- 2 files changed, 37 insertions(+), 16 deletions(-)