On 03/12/2023 17:46, Dr. David Alan Gilbert wrote:
> * Hans Verkuil (hverkuil-cisco@xxxxxxxxx) 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@xxxxxxxxx>
>> 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@xxxxxxxxxxx>
>
> 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
>>
