| Message ID | 20241122084152.1841419-6-naush@raspberrypi.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: bcm2835-unicam: Upstreaming various improvements | expand |
Hi Naush 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 ? > + */ > + 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); > -- > 2.34.1 > >
Hi Jacopo, On Fri, 22 Nov 2024 at 11:16, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Naush > > 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 userspace with an error, there is still a race condition on the name frame/buffer which will also have to return as error. Regards, Naush > > > + */ > > + 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); > > -- > > 2.34.1 > > > >
Hi Naush On Fri, Nov 22, 2024 at 11:40:26AM +0000, Naushir Patuck wrote: > Hi Jacopo, > > On Fri, 22 Nov 2024 at 11:16, Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Naush > > > > 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 ? 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 :) > Regards, > Naush > > > > > > > + */ > > > + 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); > > > -- > > > 2.34.1 > > > > > > >
With Hans Sakari and Laurent in cc for real now On Fri, Nov 22, 2024 at 03:41:31PM +0100, Jacopo Mondi wrote: > Hi Naush > > On Fri, Nov 22, 2024 at 11:40:26AM +0000, Naushir Patuck wrote: > > Hi Jacopo, > > > > On Fri, 22 Nov 2024 at 11:16, Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi Naush > > > > > > 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 ? > > 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 :) > > > Regards, > > Naush > > > > > > > > > > > + */ > > > > + 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); > > > > -- > > > > 2.34.1 > > > > > > > > > >
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 ? > > 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);
On Sun, 24 Nov 2024 at 07:04, Laurent Pinchart <laurent.pinchart@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@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
Hi Naush, 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 ? > > > > 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);
Hi Laurent, On Mon, 25 Nov 2024 at 09:23, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > 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. 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
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);
Hi Laurent, On Mon, 25 Nov 2024 at 10:27, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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@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 ? 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
Hi Naush, On Mon, Nov 25, 2024 at 10:46:01AM +0000, Naushir Patuck wrote: > On Mon, 25 Nov 2024 at 10:27, Laurent Pinchart wrote: > > 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 ? > > 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. Fine with me. > > > > > > > > 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);
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. + */ + 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);
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(-)