Hi Laurent,
On Mon, 25 Nov 2024 at 10:27, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Naush,
>
> On Mon, Nov 25, 2024 at 09:46:26AM +0000, Naushir Patuck wrote:
> > On Mon, 25 Nov 2024 at 09:23, Laurent Pinchart wrote:
> > > On Mon, Nov 25, 2024 at 08:37:22AM +0000, Naushir Patuck wrote:
> > > > On Sun, 24 Nov 2024 at 07:04, Laurent Pinchart wrote:
> > > > > On Fri, Nov 22, 2024 at 03:48:11PM +0100, Jacopo Mondi wrote:
> > > > > >
> > > > > > With Hans Sakari and Laurent in cc for real now
> > > > > >
> > > > > > On Fri, Nov 22, 2024 at 03:41:31PM +0100, Jacopo Mondi wrote:
> > > > > > > On Fri, Nov 22, 2024 at 11:40:26AM +0000, Naushir Patuck wrote:
> > > > > > > > On Fri, 22 Nov 2024 at 11:16, Jacopo Mondi wrote:
> > > > > > > > > On Fri, Nov 22, 2024 at 08:41:52AM +0000, Naushir Patuck wrote:
> > > > > > > > > > This change aligns the FS/FE interrupt handling with the Raspberry Pi
> > > > > > > > > > kernel downstream Unicam driver.
> > > > > > > > > >
> > > > > > > > > > If we get a simultaneous FS + FE interrupt for the same frame, it cannot
> > > > > > > > > > be marked as completed and returned to userland as the framebuffer will
> > > > > > > > > > be refilled by Unicam on the next sensor frame. Additionally, the
> > > > > > > > > > timestamp will be set to 0 as the FS interrupt handling code will not
> > > > > > > > > > have run yet.
> > > > > > > > > >
> > > > > > > > > > To avoid these problems, the frame is considered dropped in the FE
> > > > > > > > > > handler, and will be returned to userland on the subsequent sensor frame.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Naushir Patuck <naush@xxxxxxxxxxxxxxx>
> > > > > > > > > > ---
> > > > > > > > > > .../media/platform/broadcom/bcm2835-unicam.c | 39 +++++++++++++++++--
> > > > > > > > > > 1 file changed, 35 insertions(+), 4 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > > > > > index f10064107d54..0d2aa25d483f 100644
> > > > > > > > > > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > > > > > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > > > > > @@ -773,10 +773,26 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > > > > > > > > > * as complete, as the HW will reuse that buffer.
> > > > > > > > > > */
> > > > > > > > > > if (node->cur_frm && node->cur_frm != node->next_frm) {
> > > > > > > > > > + /*
> > > > > > > > > > + * This condition checks if FE + FS for the same
> > > > > > > > > > + * frame has occurred. In such cases, we cannot
> > > > > > > > > > + * return out the frame, as no buffer handling
> > > > > > > > > > + * or timestamping has yet been done as part of
> > > > > > > > > > + * the FS handler.
> > > > > > > > > > + */
> > > > > > > > > > + if (!node->cur_frm->vb.vb2_buf.timestamp) {
> > > > > > > > > > + dev_dbg(unicam->v4l2_dev.dev,
> > > > > > > > > > + "ISR: FE without FS, dropping frame\n");
> > > > > > > > > > + continue;
> > > > > > > > > > + }
> > > > > > > > > > +
> > > > > > > > > > unicam_process_buffer_complete(node, sequence);
> > > > > > > > > > + node->cur_frm = node->next_frm;
> > > > > > > > > > + node->next_frm = NULL;
> > > > > > > > > > inc_seq = true;
> > > > > > > > > > + } else {
> > > > > > > > > > + node->cur_frm = node->next_frm;
> > > > > > > > > > }
> > > > > > > > > > - node->cur_frm = node->next_frm;
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > /*
> > > > > > > > > > @@ -812,10 +828,25 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > > > > > > > > > i);
> > > > > > > > > > /*
> > > > > > > > > > * Set the next frame output to go to a dummy frame
> > > > > > > > > > - * if we have not managed to obtain another frame
> > > > > > > > > > - * from the queue.
> > > > > > > > > > + * if no buffer currently queued.
> > > > > > > > > > */
> > > > > > > > > > - unicam_schedule_dummy_buffer(node);
> > > > > > > > > > + if (!node->next_frm ||
> > > > > > > > > > + node->next_frm == node->cur_frm) {
> > > > > > > > > > + unicam_schedule_dummy_buffer(node);
> > > > > > > > > > + } else if (unicam->node[i].cur_frm) {
> > > > > > > > > > + /*
> > > > > > > > > > + * Repeated FS without FE. Hardware will have
> > > > > > > > > > + * swapped buffers, but the cur_frm doesn't
> > > > > > > > > > + * contain valid data. Return cur_frm to the
> > > > > > > > > > + * queue.
> > > > > > > > >
> > > > > > > > > So the buffer gets silently recycled ? Or should it be returned with
> > > > > > > > > errors to userspace ?
> > > > > > > >
> > > > > > > > The buffer silently gets recycled and we dequeue when we are sure it
> > > > > > > > is valid and will not get overwritten. If we were to return to
> > > > > > >
> > > > > > > I haven't find in the v4l2 specs any reference to what the behaviour
> > > > > > > should be.
> > > > > > >
> > > > > > > If I can summarize it: When a frame capture is aborted after the DMA
> > > > > > > transfer has already started, should the corresponding capture buffer
> > > > > > > be returned to the user in error state or the frame drop can go
> > > > > > > silently ignored ?
> > > > >
> > > > > If the DMA tranfer is aborted, I would return the buffer to userspace.
> > > > > This will indicate a frame loss better than deducing it from a gap in
> > > > > the sequence numbers.
> > > > >
> > > > > Is the DMA really aborted here though ?
> > > >
> > > > No, the DMA continues, causing possilbe overwrite/tearing in the
> > > > framebuffer. Hence we defer returning it until we can ensure we don't
> > > > overwrite into the buffer on the next frame.
> > >
> > > If the DMA continues then we certainly can't return the buffer to
> > > userspace. Is it the next frame being DMA'ed to the same buffer, or does
> > > the hardware put it the buffer at the back of its queue ?
> >
> > The next frame will be DMA'ed into the same buffer in this error
> > condition. The hardware really only has a 2-deep buffer queue (current
> > + next frame), and no reliable way of telling if next has been swapped
> > to been swapped.
>
> OK, that makes sense.
>
> In that case, is putting the buffer back to the back of the dma_queue
> the right option ? Shouldn't it be kept current and "just" be completed
> one frame later ? Or did I misunderstand the patch ?
Yes, I agree that the buffer handling logic below does seem
contradictory. I'm going to need time to look into this in more
detail, it's been quite some time since I looked into this. I would
suggest we remove this particular patch from the series until I get a
better understanding of the change.
Regards,
Naush
>
> > > > > > > Cc-ing Hans Sakari and Laurent for opinions.
> > > > > > >
> > > > > > > > userspace with an error, there is still a race condition on the name
> > > > > > > > frame/buffer which will also have to return as error.
> > > > > > >
> > > > > > > I'm sorry I didn't get this part :)
> > > > > > >
> > > > > > > > > > + */
> > > > > > > > > > + spin_lock(&node->dma_queue_lock);
> > > > > > > > > > + list_add_tail(&node->cur_frm->list,
> > > > > > > > > > + &node->dma_queue);
> > > > > > > > > > + spin_unlock(&node->dma_queue_lock);
> > > > > > > > > > + node->cur_frm = node->next_frm;
> > > > > > > > > > + node->next_frm = NULL;
> > > > > > > > > > + }
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > unicam_queue_event_sof(unicam);
>
> --
> Regards,
>
> Laurent Pinchart
