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