| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <175820896821.1246375.16035780810428204673@ping.linuxembedded.co.uk>
Date: Thu, 18 Sep 2025 16:22:48 +0100
From: Kieran Bingham <kieran.bingham@...asonboard.com>
To: Dafna Hirschfeld <dafna@...tmail.com>, Heiko Stuebner <heiko@...ech.de>, Jacopo Mondi <jacopo.mondi@...asonboard.com>, Laurent Pinchart <laurent.pinchart@...asonboard.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, Stefan Klug <stefan.klug@...asonboard.com>, linux-media@...r.kernel.org
Cc: Stefan Klug <stefan.klug@...asonboard.com>, linux-rockchip@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] media: rkisp1: Improve frame sequence correctness on stats and params buffers
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@...asonboard.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.
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?
> +
> return IRQ_HANDLED;
> }
> --
> 2.48.1
>
Powered by blists - more mailing lists