| Message ID | 20231208103908.85874-3-benjamin.gaignard@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up min_buffers_needed misusages | expand |
Hi Benjamin, On 08/12/2023 11:39, Benjamin Gaignard wrote: > Add 'min_reqbufs_allocation' field in vb2_queue structure so drivers > can specificy the minimum number of buffers to allocate when calling > VIDIOC_REQBUFS. > If used this minimum should be higher than the minimum number of > queued buffers needed to start streaming. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > drivers/media/common/videobuf2/videobuf2-core.c | 1 + > include/media/videobuf2-core.h | 6 ++++++ > 2 files changed, 7 insertions(+) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 7b6d89641e66..3bc30d107308 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -866,6 +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); This should be changed to 'q->min_queued_buffers + 1'. Without the '+ 1' you wouldn't have a buffer available to return to userspace. > + num_buffers = max_t(unsigned int, num_buffers, 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)); > /* I'm missing a WARN_ON in vb2_core_queue_init verifying that min_reqbufs_allocation <= max_num_buffers. Also, min_reqbufs_allocation must be > min_queued_buffers + 1. I also think it might be a good idea to add this to vb2_core_queue_init(): if (!q->min_reqbufs_allocation) q->min_reqbufs_allocation = q->min_queued_buffers + 1; Now we can just use q->min_reqbufs_allocation everywhere in vb2. And with that change you can probably simplify the code, e.g. there is no more need to check q->min_queued_buffers + 1 in the code above. > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 6d11cb724e95..252d34cc47d3 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -550,6 +550,11 @@ 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. This needs to be expanded: * 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 allocated when > + * calling VIDIOC_REQBUFS. Used when drivers need a to > + * specify a minimum buffers allocation before setup a queue. > + * If min_queued_buffers < min_queued_buffers then min_queued_buffers > + * is the minimum. I'd rephrase this: * @min_reqbufs_allocation: the minimum number of buffers to be allocated when * calling VIDIOC_REQBUFS. Drivers can set this if there has to * be a certain number of buffers available for the hardware to * work effectively. If set, then @min_reqbufs_allocation must be * larger than @min_queued_buffers + 1. * * This field is only used by VIDIOC_REQBUFS. This allows calling * that ioctl with a buffer count of 1 and it will be automatically * adjusted to a workable buffer count. VIDIOC_CREATE_BUFS will not * modify the requested buffer count. * * If this field is > 3, then it is highly recommended that the * driver implements the V4L2_CID_MIN_BUFFERS_FOR_CAPTURE/OUTPUT * control. Feel free to modify these texts, they are just suggestions. > */ > /* > * Private elements (won't appear at the uAPI book): > @@ -615,6 +620,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]; > Looking through videobuf2-core.c I see one more min_queued_buffers-related issue: In vb2_core_reqbufs() there is this code: if (allocated_buffers < q->min_buffers_needed) ret = -ENOMEM; That should be '< q->min_reqbufs_allocation'. Finally, add a patch updating the test-drivers as per "[PATCH v2 17/36] media: test-drivers: Fix misuse of min_buffers_needed field" + my reply to that. At least the test drivers should use these fields correctly. Regards, Hans
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 7b6d89641e66..3bc30d107308 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -866,6 +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, num_buffers, 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)); /* diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index 6d11cb724e95..252d34cc47d3 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -550,6 +550,11 @@ 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. + * @min_reqbufs_allocation: the minimum number of buffers allocated when + * calling VIDIOC_REQBUFS. Used when drivers need a to + * specify a minimum buffers allocation before setup a queue. + * If min_queued_buffers < min_queued_buffers then min_queued_buffers + * is the minimum. */ /* * Private elements (won't appear at the uAPI book): @@ -615,6 +620,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 vb2_queue structure so drivers can specificy the minimum number of buffers to allocate when calling VIDIOC_REQBUFS. If used this minimum should be higher than the minimum number of queued buffers needed to start streaming. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> --- drivers/media/common/videobuf2/videobuf2-core.c | 1 + include/media/videobuf2-core.h | 6 ++++++ 2 files changed, 7 insertions(+)