Re: [PATCH v4 2/2] videobuf2: Add min_reqbufs_allocation field to vb2_queue structure

[Hans Verkuil <hverkuil@xs4all.nl>]

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