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

["Dr. David Alan Gilbert" <dave@treblig.org>]

* 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
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/