Received-SPF: pass (google.com: domain of linux-kernel+bounces-11410-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 26 Dec 2023 08:23:49 +0000
From: Tomasz Figa <tfiga@chromium.org>
To: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Cc: hverkuil@xs4all.nl, mchehab@kernel.org, linux-kernel@vger.kernel.org,
	linux-media@vger.kernel.org, linux-staging@lists.linux.dev,
	kernel@collabora.com
Subject: Re: [PATCH v5 1/3] videobuf2: core: Rename min_buffers_needed field
 to vb2_queue
Message-ID: <20231226082349.x5ebvmt5dpanrm4k@chromium.org>
References: <20231211133251.150999-1-benjamin.gaignard@collabora.com>
 <20231211133251.150999-2-benjamin.gaignard@collabora.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231211133251.150999-2-benjamin.gaignard@collabora.com>

Hi Benjamin,

On Mon, Dec 11, 2023 at 02:32:49PM +0100, Benjamin Gaignard wrote:
> Rename min_buffers_needed into min_queued_buffers and update
> the documentation about it.
> 

Sorry for being late to the party. I think this is generally a good idea,
thanks for doing this! Just one comment inline.

[snip]
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 40d89f29fa33..8912dff5bde3 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_buffers_needed);
> +	num_buffers = max_t(unsigned int, *count, q->min_queued_buffers + 1);

Where does this + 1 come from here?
Agreed with Hans that this change deserves its own patch with a proper
explanation in its description.

>  	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_buffers_needed)
> +	if (allocated_buffers < q->min_queued_buffers)
>  		ret = -ENOMEM;
>  
>  	/*
> @@ -1653,7 +1653,7 @@ EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>   * @q:		videobuf2 queue
>   *
>   * Attempt to start streaming. When this function is called there must be
> - * at least q->min_buffers_needed buffers queued up (i.e. the minimum
> + * at least q->min_queued_buffers queued up (i.e. the minimum
>   * number of buffers required for the DMA engine to function). If the
>   * @start_streaming op fails it is supposed to return all the driver-owned
>   * buffers back to vb2 in state QUEUED. Check if that happened and if
> @@ -1846,7 +1846,7 @@ int vb2_core_qbuf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb,
>  	 * then we can finally call start_streaming().
>  	 */
>  	if (q->streaming && !q->start_streaming_called &&
> -	    q->queued_count >= q->min_buffers_needed) {
> +	    q->queued_count >= q->min_queued_buffers) {
>  		ret = vb2_start_streaming(q);
>  		if (ret) {
>  			/*
> @@ -2210,9 +2210,9 @@ int vb2_core_streamon(struct vb2_queue *q, unsigned int type)
>  		return -EINVAL;
>  	}
>  
> -	if (q_num_bufs < q->min_buffers_needed) {
> -		dprintk(q, 1, "need at least %u allocated buffers\n",
> -				q->min_buffers_needed);
> +	if (q_num_bufs < q->min_queued_buffers) {
> +		dprintk(q, 1, "need at least %u queued buffers\n",

Note that the value being checked here is the number of allocated buffers,
not queued buffers. So basically we're enforcing here that at the time
STREAMON is called, there is enough buffers allocated to queue the minimum
number of buffers to start streaming, but then later down we're not
actually enforcing that they are queued - if not, the queue start_streaming
operation is deferred until enough buffers are queued.

The question is: Do we really want this to be an error? Or should we just
be consistent with the allocated-but-not-queued case and let the
application allocate more buffers later using CREATE_BUFS?
(Another question: How does an application know how many buffers to
allocate for STREAMON to work?)

That said, this doesn't really affect the correctness of the patch itself.
Just some additional oddity in the current implementation that we could
simplify in the future.

> +			q->min_queued_buffers);
>  		return -EINVAL;
>  	}
>  
[snip]
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 5557d78b6f20..f9a00b6c3c46 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -546,10 +546,13 @@ struct vb2_buf_ops {
>   * @gfp_flags:	additional gfp flags used when allocating the buffers.
>   *		Typically this is 0, but it may be e.g. %GFP_DMA or %__GFP_DMA32
>   *		to force the buffer allocation to a specific memory zone.
> - * @min_buffers_needed: the minimum number of buffers needed before
> + * @min_queued_buffers: the minimum number of queued buffers needed before
>   *		@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 + 1
> + *		buffers will be allocated. Note that VIDIOC_CREATE_BUFS will not
> + *		modify the requested buffer count.

Same here, this + 1 needs a proper explanation.
Also, I don't like this API inconsistency where REQBUFS guarantees the
right number of buffers, but CREATE_BUFS doesn't. In fact, would an
application have a way to know how many buffers to allocate if it simply
wants to use CREATE_BUFS?

(It's generally related to the oddity that I pointed out above. Maybe we
should let the allocation code only handle allocation constraints and not
care about STREAMON constraints?)

[snip]

Best regards,
Tomasz