| Message ID | 20231215090813.15610-2-benjamin.gaignard@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add DELETE_BUF ioctl | expand |
Hi Benjamin, Some comments below: On 15/12/2023 10:08, 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. You should also mention that it defaults to min_queued_buffers + 1 instead of min_queued_buffers since otherwise you would not have a buffer available for userspace (or something along those lines). This is a change that needs to be documented in the commit message. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > drivers/media/common/videobuf2/videobuf2-core.c | 10 ++++++++-- > include/media/videobuf2-core.h | 13 +++++++++++++ > 2 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 41a832dd1426..a183edf11586 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -865,7 +865,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)); > /* > @@ -917,7 +917,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; > > /* > @@ -2521,6 +2521,12 @@ int vb2_core_queue_init(struct vb2_queue *q) > if (WARN_ON(q->supports_requests && q->min_queued_buffers)) > return -EINVAL; > > + q->min_reqbufs_allocation = max_t(unsigned int, > + 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); > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 56719a26a46c..7b84b4e2e273 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -553,6 +553,18 @@ struct vb2_buf_ops { > * VIDIOC_REQBUFS will ensure at least @min_queued_buffers > * buffers will be allocated. Note that VIDIOC_CREATE_BUFS will not In this comment it still mentions @min_queued_buffers instead of @min_queued_buffers + 1. > * modify the requested buffer count. > + * @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. There is not actually any check for that. But I think such a check should be added: it makes it much easier to do a 'git grep min_reqbufs_allocation' and only get those drivers that really set it to a non-standard value. > + * 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 "workable buffer count": nice phrase, I like it! > + * 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. Also a nice final sentence. This is also something that is a nice check to add to v4l2-compliance! A patch would be welcome... I think the sentences need to be reordered. How about this (alignment needs to be fixed): * @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. > */ > /* > * Private elements (won't appear at the uAPI book): > @@ -618,6 +630,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, Hans
On 15/12/2023 10:08, 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 | 10 ++++++++-- > include/media/videobuf2-core.h | 13 +++++++++++++ > 2 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 41a832dd1426..a183edf11586 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -865,7 +865,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)); > /* > @@ -917,7 +917,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; > > /* > @@ -2521,6 +2521,12 @@ int vb2_core_queue_init(struct vb2_queue *q) > if (WARN_ON(q->supports_requests && q->min_queued_buffers)) > return -EINVAL; > > + q->min_reqbufs_allocation = max_t(unsigned int, > + q->min_reqbufs_allocation, > + q->min_queued_buffers + 1); I have been thinking about this a bit more. The problem is that if min_queued_buffers == 0, then min_reqbufs_allocation becomes 1. But the minimum requirement should really be 2: one buffer is used to capture a new frame, the other is processed by userspace. So this should be modified as well. I would write this as 'if' statements as well: if (q->min_reqbufs_allocation < q->min_queued_buffers + 1) q->min_reqbufs_allocation = q->min_queued_buffers + 1; if (q->min_reqbufs_allocation < 2) q->min_reqbufs_allocation = 2; You can add comments before each 'if' to clarify why. I'm also wondering what this change to calculating q->min_reqbufs_allocation when the driver leaves it to 0 shouldn't be done in a separate patch, either before or after this one. We're changing the default behavior of REQBUFS slightly, and mixing that in with this patch feels wrong. Note that __vb2_init_fileio() also needs to be updated: it currently uses the same calculation of the initial 'count' value for reqbufs, but that can now just use q->min_reqbufs_allocation directly. 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); > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 56719a26a46c..7b84b4e2e273 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -553,6 +553,18 @@ struct vb2_buf_ops { > * VIDIOC_REQBUFS will ensure at least @min_queued_buffers > * 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. 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. > */ > /* > * Private elements (won't appear at the uAPI book): > @@ -618,6 +630,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 41a832dd1426..a183edf11586 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -865,7 +865,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)); /* @@ -917,7 +917,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; /* @@ -2521,6 +2521,12 @@ int vb2_core_queue_init(struct vb2_queue *q) if (WARN_ON(q->supports_requests && q->min_queued_buffers)) return -EINVAL; + q->min_reqbufs_allocation = max_t(unsigned int, + 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); diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index 56719a26a46c..7b84b4e2e273 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -553,6 +553,18 @@ struct vb2_buf_ops { * VIDIOC_REQBUFS will ensure at least @min_queued_buffers * 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. 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. */ /* * Private elements (won't appear at the uAPI book): @@ -618,6 +630,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 | 10 ++++++++-- include/media/videobuf2-core.h | 13 +++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-)