Re: [PATCH v2] media: rkisp1: Improve frame sequence correctness on stats and params buffers

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

Quoting Laurent Pinchart (2025-09-18 23:36:55)
> On Thu, Sep 18, 2025 at 04:22:48PM +0100, Kieran Bingham wrote:
> > Quoting Stefan Klug (2025-09-18 15:54:33)
> > > On the rkisp1 (in my case on a NXP i.MX8 M Plus) the ISP interrupt
> > > handler is sometimes called with RKISP1_CIF_ISP_V_START (start of frame)
> > > and RKISP1_CIF_ISP_FRAME (end of frame) being set at the same time. In
> > > commit 8524fa22fd2f ("media: staging: rkisp1: isp: add a warning and
> > > debugfs var for irq delay") a warning was added for that. There are two
> > > cases where this condition can occur:
> > > 
> > > 1. The v-sync and the frame-end belong to the same frame. This means,
> > >    the irq was heavily delayed and the warning is likely appropriate.
> > > 
> > > 2. The v-sync belongs to the next frame. This can happen if the vertical
> > >    blanking between two frames is very short.
> > > 
> > > The current code always handles case 1 although case 2 is in my
> > > experience the more common case and happens in regular usage. This leads
> > > to incorrect sequence numbers on stats and params buffers which in turn
> > > breaks the regulation in user space. Fix that by adding a frame_active
> > > flag to distinguish between these cases and handle the start of frame
> > > either at the beginning or at the end of the rkisp1_isp_isr().
> > > 
> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > 
> > > ---
> > > 
> > > Hi all,
> > > 
> > > Here is an updated version of the patch with some fixes from the review.
> > > 
> > > Changes in v2:
> > > - Removed test for !frame_active in second v_start handler
> > > - Improved comments
> > > 
> > > Best regards,
> > > Stefan
> > > 
> > > ---
> > >  .../platform/rockchip/rkisp1/rkisp1-common.h  |  1 +
> > >  .../platform/rockchip/rkisp1/rkisp1-isp.c     | 27 +++++++++++++++----
> > >  2 files changed, 23 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > > index ca952fd0829b..adf23416de9a 100644
> > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > > @@ -222,6 +222,7 @@ struct rkisp1_isp {
> > >         struct media_pad pads[RKISP1_ISP_PAD_MAX];
> > >         const struct rkisp1_mbus_info *sink_fmt;
> > >         __u32 frame_sequence;
> > > +       bool frame_active;
> > >  };
> > >  
> > >  /*
> > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > > index 8c29a1c9309a..2e49764d6262 100644
> > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > > @@ -965,6 +965,7 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
> > >         }
> > >  
> > >         isp->frame_sequence = -1;
> > > +       isp->frame_active = false;
> > >  
> > >         sd_state = v4l2_subdev_lock_and_get_active_state(sd);
> > >  
> > > @@ -1086,12 +1087,15 @@ void rkisp1_isp_unregister(struct rkisp1_device *rkisp1)
> > >   * Interrupt handlers
> > >   */
> > >  
> > > -static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
> > > +static void rkisp1_isp_sof(struct rkisp1_isp *isp)
> > >  {
> > >         struct v4l2_event event = {
> > >                 .type = V4L2_EVENT_FRAME_SYNC,
> > >         };
> > >  
> > > +       isp->frame_sequence++;
> > > +       isp->frame_active = true;
> > > +
> > >         event.u.frame_sync.frame_sequence = isp->frame_sequence;
> > >         v4l2_event_queue(isp->sd.devnode, &event);
> > >  }
> > > @@ -1111,15 +1115,20 @@ irqreturn_t rkisp1_isp_isr(int irq, void *ctx)
> > >  
> > >         rkisp1_write(rkisp1, RKISP1_CIF_ISP_ICR, status);
> > >  
> > > -       /* Vertical sync signal, starting generating new frame */
> > > -       if (status & RKISP1_CIF_ISP_V_START) {
> > > -               rkisp1->isp.frame_sequence++;
> > > -               rkisp1_isp_queue_event_sof(&rkisp1->isp);
> > > +       /*
> > > +        * Vertical sync signal, starting new frame. Defer handling of vsync
> > > +        * after RKISP1_CIF_ISP_FRAME if the previous frame was not completed
> > > +        * yet.
> > > +        */
> > > +       if (status & RKISP1_CIF_ISP_V_START && !rkisp1->isp.frame_active) {
> > > +               status &= ~RKISP1_CIF_ISP_V_START;
> > > +               rkisp1_isp_sof(&rkisp1->isp);
> > >                 if (status & RKISP1_CIF_ISP_FRAME) {
> > >                         WARN_ONCE(1, "irq delay is too long, buffers might not be in sync\n");
> > 
> > Now I've read below - I see how in here we've had both a frame start and
> > a frame end without processing an IRQ at all.
> > 
> > I'm trying to figure out if the ISR should always just process frame end
> > events before frame starts ... but then i think we wouldn't catch this
> > case so I suspect this is fine keeping it how things are now.
> > 
> > 
> > >                         rkisp1->debug.irq_delay++;
> > >                 }
> > >         }
> > > +
> > >         if (status & RKISP1_CIF_ISP_PIC_SIZE_ERROR) {
> > >                 /* Clear pic_size_error */
> > >                 isp_err = rkisp1_read(rkisp1, RKISP1_CIF_ISP_ERR);
> > > @@ -1138,6 +1147,7 @@ irqreturn_t rkisp1_isp_isr(int irq, void *ctx)
> > >         if (status & RKISP1_CIF_ISP_FRAME) {
> > >                 u32 isp_ris;
> > >  
> > > +               rkisp1->isp.frame_active = false;
> > >                 rkisp1->debug.complete_frames++;
> > >  
> > >                 /* New frame from the sensor received */
> > > @@ -1152,5 +1162,12 @@ irqreturn_t rkisp1_isp_isr(int irq, void *ctx)
> > >                 rkisp1_params_isr(rkisp1);
> > >         }
> > >  
> > > +       /*
> > > +        * Deferred handling of vsync if RKISP1_CIF_ISP_V_START and
> > > +        * RKISP1_CIF_ISP_FRAME occurrend in the same irq.
> > 
> > s/occurend/occured/
> > 
> > > +        */
> > > +       if (status & RKISP1_CIF_ISP_V_START)
> > > +               rkisp1_isp_sof(&rkisp1->isp);
> > 
> > Aha I see - so this makes sure we /complete/ the frame before we start
> > another one.
> > 
> > That definitely sounds like a very good thing.
> 
> Yes, this seems to be a good improvement.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> There may still be room for improvement, we could measure the time
> between IRQs to estimate the number of lost interrupts. That won't be
> easy to develop, and may be best handled in userspace. If we investigate
> that, it should probably be implemented with generic helpers.
> 
> > I'd be curuious to add a counter for how often we process a frame start
> > and frame end in the same ISR too. That likely still means we've got
> > some undesirable delays?
> 
> There's a counter already, and it's already exported through debugfs :-)

Ok - so I have nothing to stop me also saying this then ;-)

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> > > +
> > >         return IRQ_HANDLED;
> > >  }
> 
> -- 
> Regards,
> 
> Laurent Pinchart