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>
---
 .../platform/rockchip/rkisp1/rkisp1-common.h    |  1 +
 .../media/platform/rockchip/rkisp1/rkisp1-isp.c | 17 +++++++++++++----
 2 files changed, 14 insertions(+), 4 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..1469075b2d45 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);
 }
@@ -1112,14 +1116,15 @@ 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);
+	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");
 			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 +1143,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 +1158,8 @@ irqreturn_t rkisp1_isp_isr(int irq, void *ctx)
 		rkisp1_params_isr(rkisp1);
 	}
 
+	if (status & RKISP1_CIF_ISP_V_START && !rkisp1->isp.frame_active)
+		rkisp1_isp_sof(&rkisp1->isp);
+
 	return IRQ_HANDLED;
 }
-- 
2.48.1

Hi Stefan

On Mon, Sep 08, 2025 at 11:41:48AM +0200, Stefan Klug wrote:
> 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

I would rather argue that 8524fa22fd2f is possibily wrong, and case 1)
would imply the interrupt has been delayed for the whole frame
duration (+ blanking), which seems unlikely to me ?

True we handle stats collection and parameters programming in irq
context, which is less than ideal and could take time (I wonder if we
should use a threaded irq, but that's a different problem)

If that's the case and we only should care about 2) would simply
handling RKISP1_CIF_ISP_FRAME before RKISP1_CIF_ISP_V_START be enough ?

> 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>
> ---
>  .../platform/rockchip/rkisp1/rkisp1-common.h    |  1 +
>  .../media/platform/rockchip/rkisp1/rkisp1-isp.c | 17 +++++++++++++----
>  2 files changed, 14 insertions(+), 4 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..1469075b2d45 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);
>  }
> @@ -1112,14 +1116,15 @@ 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);
> +	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");
>  			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 +1143,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 +1158,8 @@ irqreturn_t rkisp1_isp_isr(int irq, void *ctx)
>  		rkisp1_params_isr(rkisp1);
>  	}
>
> +	if (status & RKISP1_CIF_ISP_V_START && !rkisp1->isp.frame_active)

I think you can drop the  && !rkisp1->isp.frame_active because if you
get here and 'status & RKISP1_CIF_ISP_V_START' it means that:

1) frame_active was false and you have entered the above

        if (status & RKISP1_CIF_ISP_V_START && !rkisp1->isp.frame_active) {

   and now the RKISP1_CIF_ISP_V_START bit in 'status' has been cleared
   so you don't need to handle VSYNC here

2) frame_active was true and you delayed handling V_START till here.
   If also ISP_FRAME was set, frame_start has been set to false here
   above. If ISP_FRAME was not set then it has been delivered by a
   previous interrupt and then frame_start is false.

So I guess it's enough to check if at this point RKISP1_CIF_ISP_V_START
is still set in 'status' ?

However, as said, it's seems unlikely to that your above 1) can
happen. Have you ever hit a WARN() again with this patch applied ?

> +		rkisp1_isp_sof(&rkisp1->isp);
> +
>  	return IRQ_HANDLED;
>  }
> --
> 2.48.1
>
>

Hi Jacopo,

Thank you for the review.

Quoting Jacopo Mondi (2025-09-15 18:55:44)
> Hi Stefan
> 
> On Mon, Sep 08, 2025 at 11:41:48AM +0200, Stefan Klug wrote:
> > 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
> 
> I would rather argue that 8524fa22fd2f is possibily wrong, and case 1)
> would imply the interrupt has been delayed for the whole frame
> duration (+ blanking), which seems unlikely to me ?

I am not completely sure about that. I didn't hunt for that condition.
Note that RKISP1_CIF_ISP_FRAME comes before the blanking. So I could
imagine that this might occur for very small sensor crop rectangles at
high datarates.

> 
> True we handle stats collection and parameters programming in irq
> context, which is less than ideal and could take time (I wonder if we
> should use a threaded irq, but that's a different problem)
> 
> If that's the case and we only should care about 2) would simply
> handling RKISP1_CIF_ISP_FRAME before RKISP1_CIF_ISP_V_START be enough ?

That was my first try. But it felt not right to run a whole
rkisp1_isp_isr() with frame_sequence being set to -1. And as I believe
that there is at least a slight chance that 1) might occur, I'd prefer
frame_sequence to be 0 in that case.

> 
> > 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>
> > ---
> >  .../platform/rockchip/rkisp1/rkisp1-common.h    |  1 +
> >  .../media/platform/rockchip/rkisp1/rkisp1-isp.c | 17 +++++++++++++----
> >  2 files changed, 14 insertions(+), 4 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..1469075b2d45 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);
> >  }
> > @@ -1112,14 +1116,15 @@ 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);
> > +     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");
> >                       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 +1143,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 +1158,8 @@ irqreturn_t rkisp1_isp_isr(int irq, void *ctx)
> >               rkisp1_params_isr(rkisp1);
> >       }
> >
> > +     if (status & RKISP1_CIF_ISP_V_START && !rkisp1->isp.frame_active)
> 
> I think you can drop the  && !rkisp1->isp.frame_active because if you
> get here and 'status & RKISP1_CIF_ISP_V_START' it means that:
> 
> 1) frame_active was false and you have entered the above
> 
>         if (status & RKISP1_CIF_ISP_V_START && !rkisp1->isp.frame_active) {
> 
>    and now the RKISP1_CIF_ISP_V_START bit in 'status' has been cleared
>    so you don't need to handle VSYNC here
> 
> 2) frame_active was true and you delayed handling V_START till here.
>    If also ISP_FRAME was set, frame_start has been set to false here
>    above. If ISP_FRAME was not set then it has been delivered by a
>    previous interrupt and then frame_start is false.
> 
> So I guess it's enough to check if at this point RKISP1_CIF_ISP_V_START
> is still set in 'status' ?

I think you are right. I can't come up with a sane condition where
frame_active==true and RKISP1_CIF_ISP_V_START is set and we *don't* want
to increase the frame count. I'll drop that condition.

> 
> However, as said, it's seems unlikely to that your above 1) can
> happen. Have you ever hit a WARN() again with this patch applied ?

I don't remember seeing it again. But as noted above, I didn't try to
provoke it and took the "better safe than sorry" route. Could you go
with that?

Best regards,
Stefan

> 
> > +             rkisp1_isp_sof(&rkisp1->isp);
> > +
> >       return IRQ_HANDLED;
> >  }
> > --
> > 2.48.1
> >
> >

Hi Stefan

On Tue, Sep 16, 2025 at 09:49:12AM +0200, Stefan Klug wrote:
> Hi Jacopo,
>
> Thank you for the review.
>
> Quoting Jacopo Mondi (2025-09-15 18:55:44)
> > Hi Stefan
> >
> > On Mon, Sep 08, 2025 at 11:41:48AM +0200, Stefan Klug wrote:
> > > 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
> >
> > I would rather argue that 8524fa22fd2f is possibily wrong, and case 1)
> > would imply the interrupt has been delayed for the whole frame
> > duration (+ blanking), which seems unlikely to me ?
>
> I am not completely sure about that. I didn't hunt for that condition.
> Note that RKISP1_CIF_ISP_FRAME comes before the blanking. So I could
> imagine that this might occur for very small sensor crop rectangles at
> high datarates.

Indeed, very short frame durations and minimum blankins might increase
the likeliness of 1).

Would be intersting to measure and compare with the time spent in IRQ,
but it's quite an exercize.

>
> >
> > True we handle stats collection and parameters programming in irq
> > context, which is less than ideal and could take time (I wonder if we
> > should use a threaded irq, but that's a different problem)
> >
> > If that's the case and we only should care about 2) would simply
> > handling RKISP1_CIF_ISP_FRAME before RKISP1_CIF_ISP_V_START be enough ?
>
> That was my first try. But it felt not right to run a whole
> rkisp1_isp_isr() with frame_sequence being set to -1. And as I believe

You mean for the first frame in case of both V_START and ISP_FRAME
occour in the same irq ?

> that there is at least a slight chance that 1) might occur, I'd prefer
> frame_sequence to be 0 in that case.

also because otherwise you would get stats and param buffers with
sequence -1

>
> >
> > > 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>
> > > ---
> > >  .../platform/rockchip/rkisp1/rkisp1-common.h    |  1 +
> > >  .../media/platform/rockchip/rkisp1/rkisp1-isp.c | 17 +++++++++++++----
> > >  2 files changed, 14 insertions(+), 4 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..1469075b2d45 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);
> > >  }
> > > @@ -1112,14 +1116,15 @@ 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);
> > > +     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");
> > >                       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 +1143,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 +1158,8 @@ irqreturn_t rkisp1_isp_isr(int irq, void *ctx)
> > >               rkisp1_params_isr(rkisp1);
> > >       }
> > >
> > > +     if (status & RKISP1_CIF_ISP_V_START && !rkisp1->isp.frame_active)
> >
> > I think you can drop the  && !rkisp1->isp.frame_active because if you
> > get here and 'status & RKISP1_CIF_ISP_V_START' it means that:
> >
> > 1) frame_active was false and you have entered the above
> >
> >         if (status & RKISP1_CIF_ISP_V_START && !rkisp1->isp.frame_active) {
> >
> >    and now the RKISP1_CIF_ISP_V_START bit in 'status' has been cleared
> >    so you don't need to handle VSYNC here
> >
> > 2) frame_active was true and you delayed handling V_START till here.
> >    If also ISP_FRAME was set, frame_start has been set to false here
> >    above. If ISP_FRAME was not set then it has been delivered by a
> >    previous interrupt and then frame_start is false.
> >
> > So I guess it's enough to check if at this point RKISP1_CIF_ISP_V_START
> > is still set in 'status' ?
>
> I think you are right. I can't come up with a sane condition where
> frame_active==true and RKISP1_CIF_ISP_V_START is set and we *don't* want
> to increase the frame count. I'll drop that condition.
>
> >
> > However, as said, it's seems unlikely to that your above 1) can
> > happen. Have you ever hit a WARN() again with this patch applied ?
>
> I don't remember seeing it again. But as noted above, I didn't try to
> provoke it and took the "better safe than sorry" route. Could you go
> with that?
>

Fine indeed..

Could you please comment on the two VSYNC conditions ?
Something like"

	/*
         * Vertical sync signal, starting generating new frame.
         * Defer handling of vsync after ISP_FRAME if the we still have to
         * complete the previous frame.
         */
	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");
			rkisp1->debug.irq_delay++;
		}
	}

        ...

        /*
         * Deferred handling of vsync if V_START and ISP_FRAME
         * occurrend in the same irq.
         */
	if (status & RKISP1_CIF_ISP_V_START)
		rkisp1_isp_sof(&rkisp1->isp);

Up to you


> Best regards,
> Stefan
>
> >
> > > +             rkisp1_isp_sof(&rkisp1->isp);
> > > +
> > >       return IRQ_HANDLED;
> > >  }
> > > --
> > > 2.48.1
> > >
> > >