Re: [PATCH v11 40/56] sample: v4l: Stop direct calls to queue num_buffers field

[Hans Verkuil]

On 16/10/2023 10:23, Hans Verkuil wrote:
> On 12/10/2023 13:46, Benjamin Gaignard wrote:
>> Use vb2_get_num_buffers() to avoid using queue num_buffers field directly.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxxxxx>
>> ---
>>  samples/v4l/v4l2-pci-skeleton.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/samples/v4l/v4l2-pci-skeleton.c b/samples/v4l/v4l2-pci-skeleton.c
>> index a61f94db18d9..a65aa9d1e9da 100644
>> --- a/samples/v4l/v4l2-pci-skeleton.c
>> +++ b/samples/v4l/v4l2-pci-skeleton.c
>> @@ -155,6 +155,7 @@ static int queue_setup(struct vb2_queue *vq,
>>  		       unsigned int sizes[], struct device *alloc_devs[])
>>  {
>>  	struct skeleton *skel = vb2_get_drv_priv(vq);
>> +	unsigned int q_num_bufs = vb2_get_num_buffers(vq);
>>  
>>  	skel->field = skel->format.field;
>>  	if (skel->field == V4L2_FIELD_ALTERNATE) {
>> @@ -167,8 +168,8 @@ static int queue_setup(struct vb2_queue *vq,
>>  		skel->field = V4L2_FIELD_TOP;
>>  	}
>>  
>> -	if (vq->num_buffers + *nbuffers < 3)
>> -		*nbuffers = 3 - vq->num_buffers;
>> +	if (q_num_bufs + *nbuffers < 3)
>> +		*nbuffers = 3 - q_num_bufs;
> 
> This should be dropped, and instead update q->min_buffers_needed from
> 2 to 3.

Actually, that's not quite true. I realized that there is a subtle bug in
the vb2 core and a general misunderstanding of min_buffers_needed in a lot
of drivers.

The min_buffers_needed field describes the minimum number of buffers needed
before the DMA engine can start. This is typically 0, 1 or 2. Once that many
buffers have been queued, then start_streaming callback is called. With fewer
buffers queued the DMA engine cannot start, so this represents a DMA engine
limitation.

Currently vb2 also uses this field as the minimum number of buffers to
allocate. However, that should be one more, so min_buffers_needed+1:
'min_buffers_needed' buffers are in-flight, and you need one more that is
owned by userspace, otherwise you would never be able to dequeue a buffer
if you only created 'min_buffers_needed' buffers.

But I noticed a lot of drivers that misinterpret this value as 'the minimum
number of buffers to allocate', unrelated to any DMA engine limitations.
This is most likely a bug, since this would unnecessarily delay the call to
start_streaming().

In other words, it is a mess.

I think my earlier advice to change min_buffers_needed and drop the check
in queue_setup should be disregarded. If the min_buffers_needed value
is >= the value checked in queue_setup, then you can drop the check. In all
other cases it is safer to keep it.

So in other words, this patch is fine. But e.g. patch 21 needs to keep the
check (although with a fix: *nbuffers = 2 - q_num_bufs).

When this work is done, then I think I need to take a close review at all the
drivers that set min_buffers_needed and/or check the number of buffers in
queue_setup and fix it properly.

Likely we need two different fields: one for the minimum number of buffers
that need to be allocated, and one for the minimum number of buffers that
need to be queued before start_streaming can be called. But that raises
the question how the 'minimum number of buffers that need to be allocated'
would interact with deleting buffers. It's actually not all that easy.

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
>>  
>>  	if (*nplanes)
>>  		return sizes[0] < skel->format.sizeimage ? -EINVAL : 0;
>