diff mbox series

[3/3] media: videobuf2: request more buffers for vb2_read

Message ID 86ad4808718ff07ab8ac64b62170b789c16b2581.1701349092.git.hverkuil-cisco@xs4all.nl
State Accepted
Commit 350ab13e1382f2afcc2285041a1e75b80d771c2c
Headers show
Series [1/3] media: bttv: start_streaming should return a proper error code | expand

Commit Message

Hans Verkuil Nov. 30, 2023, 12:58 p.m. UTC
The vb2 read support requests 1 buffer, leaving it to the driver
to increase this number to something that works.

Unfortunately, drivers do not deal with this reliably, and in fact
this caused problems for the bttv driver and reading from /dev/vbiX,
causing every other VBI frame to be all 0.

Instead, request as the number of buffers whatever is the maximum of
2 and q->min_buffers_needed+1.

In order to start streaming you need at least q->min_buffers_needed
queued buffers, so add 1 buffer for processing. And if that field
is 0, then choose 2 (again, one buffer is being filled while the
other one is being processed).

This certainly makes more sense than requesting just 1 buffer, and
the VBI bttv support is now working again.

It turns out that the old videobuf1 behavior of bttv was to allocate
8 (video) and 4 (vbi) buffers when used with read(). After the vb2
conversion that changed to 2 for both. With this patch it is 3, which
is really all you need.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Fixes: b7ec3212a73a ("media: bttv: convert to vb2")
---
 drivers/media/common/videobuf2/videobuf2-core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Dr. David Alan Gilbert Dec. 3, 2023, 4:46 p.m. UTC | #1
* Hans Verkuil (hverkuil-cisco@xs4all.nl) wrote:
> The vb2 read support requests 1 buffer, leaving it to the driver
> to increase this number to something that works.
> 
> Unfortunately, drivers do not deal with this reliably, and in fact
> this caused problems for the bttv driver and reading from /dev/vbiX,
> causing every other VBI frame to be all 0.
> 
> Instead, request as the number of buffers whatever is the maximum of
> 2 and q->min_buffers_needed+1.
> 
> In order to start streaming you need at least q->min_buffers_needed
> queued buffers, so add 1 buffer for processing. And if that field
> is 0, then choose 2 (again, one buffer is being filled while the
> other one is being processed).
> 
> This certainly makes more sense than requesting just 1 buffer, and
> the VBI bttv support is now working again.
> 
> It turns out that the old videobuf1 behavior of bttv was to allocate
> 8 (video) and 4 (vbi) buffers when used with read(). After the vb2
> conversion that changed to 2 for both. With this patch it is 3, which
> is really all you need.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Fixes: b7ec3212a73a ("media: bttv: convert to vb2")

This looks like it's working nicely;  I've tested it with both
Alistair's test stream and a real signal, and I'm getting
a consistent 25fps out of the VBI with or without xawtv
grabbing, and the test stream looks good to me.

So,

Tested-by: Dr. David Alan Gilbert <dave@treblig.org>

Thanks for fixing this!

Dave

> ---
>  drivers/media/common/videobuf2/videobuf2-core.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 8c1df829745b..40d89f29fa33 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -2735,9 +2735,14 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
>  		return -EBUSY;
>  
>  	/*
> -	 * Start with count 1, driver can increase it in queue_setup()
> +	 * Start with q->min_buffers_needed + 1, driver can increase it in
> +	 * queue_setup()
> +	 *
> +	 * 'min_buffers_needed' buffers need to be queued up before you
> +	 * can start streaming, plus 1 for userspace (or in this case,
> +	 * kernelspace) processing.
>  	 */
> -	count = 1;
> +	count = max(2, q->min_buffers_needed + 1);
>  
>  	dprintk(q, 3, "setting up file io: mode %s, count %d, read_once %d, write_immediately %d\n",
>  		(read) ? "read" : "write", count, q->fileio_read_once,
> -- 
> 2.42.0
>
Hans Verkuil Dec. 4, 2023, 8:48 a.m. UTC | #2
On 03/12/2023 17:46, Dr. David Alan Gilbert wrote:
> * Hans Verkuil (hverkuil-cisco@xs4all.nl) wrote:
>> The vb2 read support requests 1 buffer, leaving it to the driver
>> to increase this number to something that works.
>>
>> Unfortunately, drivers do not deal with this reliably, and in fact
>> this caused problems for the bttv driver and reading from /dev/vbiX,
>> causing every other VBI frame to be all 0.
>>
>> Instead, request as the number of buffers whatever is the maximum of
>> 2 and q->min_buffers_needed+1.
>>
>> In order to start streaming you need at least q->min_buffers_needed
>> queued buffers, so add 1 buffer for processing. And if that field
>> is 0, then choose 2 (again, one buffer is being filled while the
>> other one is being processed).
>>
>> This certainly makes more sense than requesting just 1 buffer, and
>> the VBI bttv support is now working again.
>>
>> It turns out that the old videobuf1 behavior of bttv was to allocate
>> 8 (video) and 4 (vbi) buffers when used with read(). After the vb2
>> conversion that changed to 2 for both. With this patch it is 3, which
>> is really all you need.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Fixes: b7ec3212a73a ("media: bttv: convert to vb2")
> 
> This looks like it's working nicely;  I've tested it with both
> Alistair's test stream and a real signal, and I'm getting
> a consistent 25fps out of the VBI with or without xawtv
> grabbing, and the test stream looks good to me.
> 
> So,
> 
> Tested-by: Dr. David Alan Gilbert <dave@treblig.org>
> 
> Thanks for fixing this!

Thank you for testing this!

Much appreciated.

Regards,

	Hans

> 
> Dave
> 
>> ---
>>  drivers/media/common/videobuf2/videobuf2-core.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 8c1df829745b..40d89f29fa33 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -2735,9 +2735,14 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
>>  		return -EBUSY;
>>  
>>  	/*
>> -	 * Start with count 1, driver can increase it in queue_setup()
>> +	 * Start with q->min_buffers_needed + 1, driver can increase it in
>> +	 * queue_setup()
>> +	 *
>> +	 * 'min_buffers_needed' buffers need to be queued up before you
>> +	 * can start streaming, plus 1 for userspace (or in this case,
>> +	 * kernelspace) processing.
>>  	 */
>> -	count = 1;
>> +	count = max(2, q->min_buffers_needed + 1);
>>  
>>  	dprintk(q, 3, "setting up file io: mode %s, count %d, read_once %d, write_immediately %d\n",
>>  		(read) ? "read" : "write", count, q->fileio_read_once,
>> -- 
>> 2.42.0
>>
diff mbox series

Patch

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 8c1df829745b..40d89f29fa33 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -2735,9 +2735,14 @@  static int __vb2_init_fileio(struct vb2_queue *q, int read)
 		return -EBUSY;
 
 	/*
-	 * Start with count 1, driver can increase it in queue_setup()
+	 * Start with q->min_buffers_needed + 1, driver can increase it in
+	 * queue_setup()
+	 *
+	 * 'min_buffers_needed' buffers need to be queued up before you
+	 * can start streaming, plus 1 for userspace (or in this case,
+	 * kernelspace) processing.
 	 */
-	count = 1;
+	count = max(2, q->min_buffers_needed + 1);
 
 	dprintk(q, 3, "setting up file io: mode %s, count %d, read_once %d, write_immediately %d\n",
 		(read) ? "read" : "write", count, q->fileio_read_once,