Re: [PATCH v2 13/13] media: bttv: convert to vb2

[Deborah Brouwer <deborah.brouwer@collabora.com>]

On Thu, May 11, 2023 at 05:29:14PM +0200, Hans Verkuil wrote:
> Hi Deb,
> 
> When testing this I was wondering why the sequence counter didn't detect dropped
> frames (which happens when you start/stop a vbi stream while streaming video).
> 
> On 02/05/2023 05:27, Deborah Brouwer wrote:
> > -static void bttv_field_count(struct bttv *btv)
> > -{
> > -	int need_count = 0;
> > -
> > -	if (btv->users)
> > -		need_count++;
> > -
> > -	if (need_count) {
> > -		/* start field counter */
> > -		btor(BT848_INT_VSYNC,BT848_INT_MASK);
> > -	} else {
> > -		/* stop field counter */
> > -		btand(~BT848_INT_VSYNC,BT848_INT_MASK);
> > -		btv->field_count = 0;
> > -	}
> > -}
> 
> This is the root cause: this function is used to turn on VSYNC interrupts
> and in the interrupt handler the field_count is incremented.
> 
> In the vb1 version of this driver this field_count is passed on to vb1, which
> uses it to set the sequence counter to field_count / 2.
> 
> By removing this function the VSYNC irq is never enabled and so field_count is
> always 0. So I think in bttv the seqnr field should be dropped and the field_count
> mechanism re-instated.
> 
> Comparing the number of dropped frames when starting/stopping vbi it looks like
> in both cases (vb1 and vb2) one frame is dropped when starting vbi. But when
> stopping vbi no frames are dropped in the vb1 case, but 3 in the vb2 case.

Ok, I used btv->field_count >> 1 to set the sequence numbers and I added VSYNC to
the interrupt mask when start_streaming() is called. I can see the dropped video frames
in v4l2-ctl when vbi starts and stops (or vice versa), but I am not sure if this means
that we are actually dropping any valid, captured pixels.

The internal FIELD change signal (even/odd) that is picked up by VSYNC seems to run
independently from capturing. For example, out of curiosity, I added VSYNC to the
interrupt mask at probe, and it starts to increment field_count (and so the sequence
numbers) immediately and continuously even though capture isn't enabled.

So I am wondering if field_count is a totally reliable way to assign sequence numbers
to the buffers. For example, using field_count, the buffer sequence numbers start with
3 instead of 0 in both vb1 and vb2 (sometimes it starts at 2 in vb2).
Maybe the reason for this is that the field signal (even/odd) continues to be sent while
the captured pixels are being input into the chip's FIFO buffer. The FIFO buffer is
preparing the pixels for output by adding the timing/control
information but hasn't yet made them available to be picked up by the DMA controller for
output to memory. So, there is a disconnect, when streaming starts, between field_count
(which keeps incrementing) and the actual availability of pixels.

Maybe there is a similar disconnect happening when vbi starts or stops. Instead of actually
dropping pixels what we really have is just a delay in the RISC program that has to cycle
through its instructions. vb2 has made this a bit worse when streaming stops because we wait
for all the buffers to finish. I will investigate more if there is an easy way to fix this in vb2
without changing the RISC program.

> 
> Another thing I discovered is that for PAL the vcr_hack control has to be enabled,
> otherwise the video is full of glitches. This was present before your series, and
> happens even with a video signal from a proper PAL video generator, so this is really
> strange. I can't remember that I needed this in the past, but it has been years
> since I last tested it.
> 
> PAL capture is fine for Top/Bottom/Alternate settings, it only fails for Interlaced
> and Sequential Top/Bottom capture modes.

Interesting to know that this happened in your tests also.
Obviously the easy solution would be just to enable the vcr_hack by default.
It removes the last four scan lines and so prevents the risc program
counter from getting stuck at a buffer memory address (which definitely
causes dropped frames) instead of returning to the main risc program address.

> 
> When I have more time I will dig into this a bit more.
> 
> Regards,
> 
> 	Hans