diff mbox series

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

Message ID 20231127165454.166373-4-benjamin.gaignard@collabora.com (mailing list archive)
State New, archived
Headers show
Series Clean up queue_setup()/min_buffers_needed (ab)use | expand

Commit Message

Benjamin Gaignard Nov. 27, 2023, 4:54 p.m. UTC
'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(-)

Comments

Hans Verkuil Nov. 28, 2023, 10:18 a.m. UTC | #1
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.

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.

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)
Benjamin Gaignard Nov. 28, 2023, 10:23 a.m. UTC | #2
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)
>
diff mbox series

Patch

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;
 	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)