Re: [PATCH 03/55] media: usb: cx231xx: Stop abusing of min_buffers_needed field

[Benjamin Gaignard <benjamin.gaignard@collabora.com>]

Le 28/11/2023 à 11:18, Hans Verkuil a écrit :
> On 27/11/2023 17:54, Benjamin Gaignard wrote:
>> 'min_buffers_needed' is suppose to be used to indicate the number
>> of buffers needed by DMA engine to start streaming.
>> cx231xx driver doesn't use DMA engine and just want to specify
>> the minimum number of buffers to allocate when calling VIDIOC_REQBUFS.
>> That 'min_reqbufs_allocation' field purpose so use it.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>>   drivers/media/usb/cx231xx/cx231xx-417.c   | 2 +-
>>   drivers/media/usb/cx231xx/cx231xx-video.c | 4 ++--
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/usb/cx231xx/cx231xx-417.c b/drivers/media/usb/cx231xx/cx231xx-417.c
>> index 45973fe690b2..66043ed50c8e 100644
>> --- a/drivers/media/usb/cx231xx/cx231xx-417.c
>> +++ b/drivers/media/usb/cx231xx/cx231xx-417.c
>> @@ -1782,7 +1782,7 @@ int cx231xx_417_register(struct cx231xx *dev)
>>   	q->ops = &cx231xx_video_qops;
>>   	q->mem_ops = &vb2_vmalloc_memops;
>>   	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>> -	q->min_buffers_needed = 1;
>> +	q->min_reqbufs_allocation = 1;
> There is no point setting min_reqbufs_allocation to 1: you can't allocate
> less than 1 buffer after all.

Does that mean I should just remove this line ?

>
> It is different in that respect from min_buffers_needed: you can call
> VIDIOC_STREAMON (and thus start_streaming) without any buffers queued.
>
> This also suggests a better name for min_buffers_needed: min_queued_buffers
>
> So 'min_queued_buffers' buffers have to be queued before start_streaming can
> be called.

Ok I will change that in V2

Regards,
Benjamin

>
> The old min_buffers_needed mixed up the two requirements of the minimum
> number of buffers to allocate in REQBUFS and the minimum number of buffers
> that need to be queued before you can start streaming. Separating these two
> meanings should make things easier to understand.
>
> The only relationship between the two is that min_reqbufs_allocation >
> min_queued_buffers, otherwise you would end up in a state where the
> driver would just cycle buffers and never be able to return a buffer
> to userspace to process.
>
> Regards,
>
> 	Hans
>
>>   	q->lock = &dev->lock;
>>   	err = vb2_queue_init(q);
>>   	if (err)
>> diff --git a/drivers/media/usb/cx231xx/cx231xx-video.c b/drivers/media/usb/cx231xx/cx231xx-video.c
>> index c8eb4222319d..df572c466bfb 100644
>> --- a/drivers/media/usb/cx231xx/cx231xx-video.c
>> +++ b/drivers/media/usb/cx231xx/cx231xx-video.c
>> @@ -1811,7 +1811,7 @@ int cx231xx_register_analog_devices(struct cx231xx *dev)
>>   	q->ops = &cx231xx_video_qops;
>>   	q->mem_ops = &vb2_vmalloc_memops;
>>   	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>> -	q->min_buffers_needed = 1;
>> +	q->min_reqbufs_allocation = 1;
>>   	q->lock = &dev->lock;
>>   	ret = vb2_queue_init(q);
>>   	if (ret)
>> @@ -1871,7 +1871,7 @@ int cx231xx_register_analog_devices(struct cx231xx *dev)
>>   	q->ops = &cx231xx_vbi_qops;
>>   	q->mem_ops = &vb2_vmalloc_memops;
>>   	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>> -	q->min_buffers_needed = 1;
>> +	q->min_reqbufs_allocation = 1;
>>   	q->lock = &dev->lock;
>>   	ret = vb2_queue_init(q);
>>   	if (ret)
>