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

[Naushir Patuck]

On Sun, 24 Nov 2024 at 07:04, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> 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 at 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.

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