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@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 ?
> + */
> + 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
>
>
