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