Re: [PATCH v1 5/5] drivers: media: bcm2835-unicam: Correctly handle FS + FE ISR condition

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@raspberrypi.com>
> > > > > > > > > ---
> > > > > > > > >  .../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 ?

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