[PATCH 0/3] media: bttv: three post-vb2 fixes

[PATCH 0/3] media: bttv: three post-vb2 fixes

[Hans Verkuil]

Dave, thank you for your detailed bttv bug report:

https://lore.kernel.org/linux-media/ZVK_3UmvmOaCv1tc@gallifrey/

It was very helpful and I managed to reproduce these issues.

This patch series fixes them.

The first patch is unrelated, just something I noticed while
debugging this.

The second patch adds back the old bttv behavior of storing the
frame counter in the last 4 bytes of the vbi payload when using
read().

The final patch fixes the vbi read() behavior where it was
alternating between valid data and a zeroed buffer.

I'd appreciate it if you can test this before Dec 11 since I
plan to merge on Dec 11 if I don't hear anything.

Regards,

	Hans

Hans Verkuil (3):
  media: bttv: start_streaming should return a proper error code
  media: bttv: add back vbi hack
  media: videobuf2: request more buffers for vb2_read

 .../media/common/videobuf2/videobuf2-core.c   |  9 +++++--
 drivers/media/pci/bt8xx/bttv-driver.c         | 27 ++++++++++++++++---
 drivers/media/pci/bt8xx/bttv-vbi.c            |  8 +++---
 3 files changed, 33 insertions(+), 11 deletions(-)

-- 
2.42.0

[PATCH 1/3] media: bttv: start_streaming should return a proper error code

[Hans Verkuil]

The start_streaming callback returned 0 or 1 instead of a
proper error code. Fix that.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Fixes: b7ec3212a73a ("media: bttv: convert to vb2")
---
 drivers/media/pci/bt8xx/bttv-driver.c | 6 ++----
 drivers/media/pci/bt8xx/bttv-vbi.c    | 8 +++-----
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
index 09a193bb87df..8e8c9dada67a 100644
--- a/drivers/media/pci/bt8xx/bttv-driver.c
+++ b/drivers/media/pci/bt8xx/bttv-driver.c
@@ -1536,13 +1536,11 @@ static void buf_cleanup(struct vb2_buffer *vb)
 
 static int start_streaming(struct vb2_queue *q, unsigned int count)
 {
-	int ret = 1;
 	int seqnr = 0;
 	struct bttv_buffer *buf;
 	struct bttv *btv = vb2_get_drv_priv(q);
 
-	ret = check_alloc_btres_lock(btv, RESOURCE_VIDEO_STREAM);
-	if (ret == 0) {
+	if (!check_alloc_btres_lock(btv, RESOURCE_VIDEO_STREAM)) {
 		if (btv->field_count)
 			seqnr++;
 		while (!list_empty(&btv->capture)) {
@@ -1553,7 +1551,7 @@ static int start_streaming(struct vb2_queue *q, unsigned int count)
 			vb2_buffer_done(&buf->vbuf.vb2_buf,
 					VB2_BUF_STATE_QUEUED);
 		}
-		return !ret;
+		return -EBUSY;
 	}
 	if (!vb2_is_streaming(&btv->vbiq)) {
 		init_irqreg(btv);
diff --git a/drivers/media/pci/bt8xx/bttv-vbi.c b/drivers/media/pci/bt8xx/bttv-vbi.c
index ab213e51ec95..e489a3acb4b9 100644
--- a/drivers/media/pci/bt8xx/bttv-vbi.c
+++ b/drivers/media/pci/bt8xx/bttv-vbi.c
@@ -123,14 +123,12 @@ static void buf_cleanup_vbi(struct vb2_buffer *vb)
 
 static int start_streaming_vbi(struct vb2_queue *q, unsigned int count)
 {
-	int ret;
 	int seqnr = 0;
 	struct bttv_buffer *buf;
 	struct bttv *btv = vb2_get_drv_priv(q);
 
 	btv->framedrop = 0;
-	ret = check_alloc_btres_lock(btv, RESOURCE_VBI);
-	if (ret == 0) {
+	if (!check_alloc_btres_lock(btv, RESOURCE_VBI)) {
 		if (btv->field_count)
 			seqnr++;
 		while (!list_empty(&btv->vcapture)) {
@@ -141,13 +139,13 @@ static int start_streaming_vbi(struct vb2_queue *q, unsigned int count)
 			vb2_buffer_done(&buf->vbuf.vb2_buf,
 					VB2_BUF_STATE_QUEUED);
 		}
-		return !ret;
+		return -EBUSY;
 	}
 	if (!vb2_is_streaming(&btv->capq)) {
 		init_irqreg(btv);
 		btv->field_count = 0;
 	}
-	return !ret;
+	return 0;
 }
 
 static void stop_streaming_vbi(struct vb2_queue *q)
-- 
2.42.0

[PATCH 2/3] media: bttv: add back vbi hack

[Hans Verkuil]

The old (now removed) videobuf framework had an optional vbi hack where
the sequence number of the frame counter was copied in the last 4 bytes
of the buffer. This hack was active only for the read() interface
(so not for streaming I/O), and it was enabled by bttv. This allowed
applications that used read() for the VBI data to match it with the
corresponding video frame.

When bttv was converted to vb2 this hack was forgotten, but some old
applications rely on this.

So add this back, but this time in the bttv driver rather than in the
vb2 framework.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Fixes: b7ec3212a73a ("media: bttv: convert to vb2")
---
 drivers/media/pci/bt8xx/bttv-driver.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
index 8e8c9dada67a..49a3dd70ec0f 100644
--- a/drivers/media/pci/bt8xx/bttv-driver.c
+++ b/drivers/media/pci/bt8xx/bttv-driver.c
@@ -2772,6 +2772,27 @@ bttv_irq_wakeup_vbi(struct bttv *btv, struct bttv_buffer *wakeup,
 		return;
 	wakeup->vbuf.vb2_buf.timestamp = ktime_get_ns();
 	wakeup->vbuf.sequence = btv->field_count >> 1;
+
+	/*
+	 * Ugly hack for backwards compatibility.
+	 * Some applications expect that the last 4 bytes of
+	 * the VBI data contains the sequence number.
+	 *
+	 * This makes it possible to associate the VBI data
+	 * with the video frame if you use read() to get the
+	 * VBI data.
+	 */
+	if (vb2_fileio_is_active(wakeup->vbuf.vb2_buf.vb2_queue)) {
+		u32 *vaddr = vb2_plane_vaddr(&wakeup->vbuf.vb2_buf, 0);
+		unsigned long size =
+			vb2_get_plane_payload(&wakeup->vbuf.vb2_buf, 0) / 4;
+
+		if (vaddr && size) {
+			vaddr += size - 1;
+			*vaddr = wakeup->vbuf.sequence;
+		}
+	}
+
 	vb2_buffer_done(&wakeup->vbuf.vb2_buf, state);
 	if (btv->field_count == 0)
 		btor(BT848_INT_VSYNC, BT848_INT_MASK);
-- 
2.42.0

Re: [PATCH 2/3] media: bttv: add back vbi hack

[Dr. David Alan Gilbert]

* Hans Verkuil (hverkuil-cisco@xs4all.nl) wrote:
> The old (now removed) videobuf framework had an optional vbi hack where
> the sequence number of the frame counter was copied in the last 4 bytes
> of the buffer. This hack was active only for the read() interface
> (so not for streaming I/O), and it was enabled by bttv. This allowed
> applications that used read() for the VBI data to match it with the
> corresponding video frame.
> 
> When bttv was converted to vb2 this hack was forgotten, but some old
> applications rely on this.
> 
> So add this back, but this time in the bttv driver rather than in the
> vb2 framework.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Fixes: b7ec3212a73a ("media: bttv: convert to vb2")

Thanks; this seems to fix the sequence number errors I was getting,
so:
Tested-by: Dr. David Alan Gilbert <dave@treblig.org>

> ---
>  drivers/media/pci/bt8xx/bttv-driver.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
> index 8e8c9dada67a..49a3dd70ec0f 100644
> --- a/drivers/media/pci/bt8xx/bttv-driver.c
> +++ b/drivers/media/pci/bt8xx/bttv-driver.c
> @@ -2772,6 +2772,27 @@ bttv_irq_wakeup_vbi(struct bttv *btv, struct bttv_buffer *wakeup,
>  		return;
>  	wakeup->vbuf.vb2_buf.timestamp = ktime_get_ns();
>  	wakeup->vbuf.sequence = btv->field_count >> 1;
> +
> +	/*
> +	 * Ugly hack for backwards compatibility.
> +	 * Some applications expect that the last 4 bytes of
> +	 * the VBI data contains the sequence number.
> +	 *
> +	 * This makes it possible to associate the VBI data
> +	 * with the video frame if you use read() to get the
> +	 * VBI data.
> +	 */
> +	if (vb2_fileio_is_active(wakeup->vbuf.vb2_buf.vb2_queue)) {
> +		u32 *vaddr = vb2_plane_vaddr(&wakeup->vbuf.vb2_buf, 0);
> +		unsigned long size =
> +			vb2_get_plane_payload(&wakeup->vbuf.vb2_buf, 0) / 4;
> +
> +		if (vaddr && size) {
> +			vaddr += size - 1;
> +			*vaddr = wakeup->vbuf.sequence;
> +		}
> +	}
> +
>  	vb2_buffer_done(&wakeup->vbuf.vb2_buf, state);
>  	if (btv->field_count == 0)
>  		btor(BT848_INT_VSYNC, BT848_INT_MASK);
> -- 
> 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   |_______/

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

[Hans Verkuil]

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

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

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

[Dr. David Alan Gilbert]

* 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   |_______/

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

[Hans Verkuil]

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

Re: [PATCH 0/3] media: bttv: three post-vb2 fixes

[Dr. David Alan Gilbert]

* Hans Verkuil (hverkuil-cisco@xs4all.nl) wrote:
> Dave, thank you for your detailed bttv bug report:
> 
> https://lore.kernel.org/linux-media/ZVK_3UmvmOaCv1tc@gallifrey/
> 
> It was very helpful and I managed to reproduce these issues.
> 
> This patch series fixes them.
> 
> The first patch is unrelated, just something I noticed while
> debugging this.
> 
> The second patch adds back the old bttv behavior of storing the
> frame counter in the last 4 bytes of the vbi payload when using
> read().
> 
> The final patch fixes the vbi read() behavior where it was
> alternating between valid data and a zeroed buffer.
> 
> I'd appreciate it if you can test this before Dec 11 since I
> plan to merge on Dec 11 if I don't hear anything.

Sure, I'll get on it.

Dave

> Regards,
> 
> 	Hans
> 
> Hans Verkuil (3):
>   media: bttv: start_streaming should return a proper error code
>   media: bttv: add back vbi hack
>   media: videobuf2: request more buffers for vb2_read
> 
>  .../media/common/videobuf2/videobuf2-core.c   |  9 +++++--
>  drivers/media/pci/bt8xx/bttv-driver.c         | 27 ++++++++++++++++---
>  drivers/media/pci/bt8xx/bttv-vbi.c            |  8 +++---
>  3 files changed, 33 insertions(+), 11 deletions(-)
> 
> -- 
> 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   |_______/