Hi Laurent,

Thanks for your review.

-----Original Message-----
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Sent: Friday, March 20, 2026 5:34 AM
To: G.N. Zhou (OSS) <redacted>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Shawn Guo
[off-list ref]; Sascha Hauer [off-list ref];
Pengutronix Kernel Team [off-list ref]; Fabio Estevam
[off-list ref]; Stefan Riedmüller [off-list ref];
Jacopo Mondi [off-list ref]; c.hemp@phytec.de; Frank Li
[off-list ref]; Aisheng Dong [off-list ref]; linux-
media@vger.kernel.org; imx@lists.linux.dev; linux-arm-
kernel@lists.infradead.org; linux-kernel@vger.kernel.org; G.N. Zhou
[off-list ref]; stable@vger.kernel.org
Subject: Re: [PATCH RESEND v2] media: nxp: imx8-isi: Fix potential out-of-
bounds issues

Hello Guoniu,

Thank you for the patch.

On Sat, Feb 28, 2026 at 05:35:53PM +0800, Guoniu Zhou wrote:

quoted

From: Guoniu Zhou <redacted>

The maximum downscaling factor supported by ISI can be up to 16. Add
minimum value constraint before applying the setting to hardware.
Otherwise, the process will not respond even when Ctrl+C is executed.

Could you share how you can trigger this from userspace ? I'd like to test it and
see where the absence of response comes from.

Sure, I found this issue on i.MX952 but other platform like i.MX8MN also could reproduce. 

Bellow command using ISI m2m(/dev/video1) function.
root@imx8mnevk:~# v4l2-ctl --list-dev
mxc-isi-cap (platform:32e20000.isi):
        /dev/video0
        /dev/video1
        /dev/media0
$ gst-launch-1.0 videotestsrc num-buffers=100 '!' video/x-raw,width=1920,height=1080,format=BGRx '!' queue '!' v4l2convert '!' video/x-raw,width=32,height=32,format=BGRx '!' queue '!' waylandsink

quoted

Fixes: cf21f328fcaf ("media: nxp: Add i.MX8 ISI driver")
Cc: stable@vger.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Guoniu Zhou <redacted>
---
Changes in v2:
- Fix possible side-effects caused by 'max_val' in CLAMP_DOWNSCALE_16

macro.

quoted

- Add space line between two paragraph in comment.
- Link to v1:
https://lore.kernel.org/r/20260122-isi-v1-1-c3ec6e264f13@nxp.com (local)
---
 drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h | 16
++++++++++++++++  drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
|  6 +++---  drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c |  6
++----
 3 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h

b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
index

3cbd35305af0f8026c4f76b5eb5d0864f8e36dc3..84e134f4d5fb26be652ac3e6
aecd

quoted

459bb2a0d1c6 100644

--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h

@@ -59,6 +59,22 @@ struct v4l2_m2m_dev;
 #define MXC_ISI_M2M			"mxc-isi-m2m"
 #define MXC_MAX_PLANES			3

+/*
+ * ISI scaling engine works in two parts: it performs pre-decimation
+of
+ * the image followed by bilinear filtering to achieve the desired
+ * downscaling factor.
+ *
+ * The decimation filter provides a maximum downscaling factor of 8,
+and
+ * the subsequent bilinear filter provides a maximum downscaling
+factor
+ * of 2. Combined, the maximum scaling factor can be up to 16.
+ */
+#define CLAMP_DOWNSCALE_16(val, max_val)			\

Let's add a MXC_ISI_ prefix.

quoted

+({								\
+	typeof(max_val) __max_val = (max_val);			\
+								\
+	clamp((val), max(1U, __max_val >> 4), __max_val);	\

I think you should round the division up here:

	clamp((val), max(1U, (__max_val + 15) / 16, __max_val);	\

Let's assume the input size (max_val) is 1000. Without rounding up, the output
size will be clamped to 1000 / 16 = 62. This leads to a downscaling factor
slightly above 16.

quoted

+})

Any reason not to make this an inline function instead of a macro ?

Thank you for the suggestion. You're right - an inline function would be better here for type safety
and easier debugging. The macro was initially used without special consideration. I'll change it
to a static inline function in next version.

quoted

+
 struct mxc_isi_dev;
 struct mxc_isi_m2m_ctx;

diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c

b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
index

f425ac7868547da401e86ce5a9b70a9890e72541..8860d89713667d06abc94e
602452

quoted

6fabac46feb7 100644

--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c

@@ -509,9 +509,9 @@ __mxc_isi_m2m_try_fmt_vid(struct

mxc_isi_m2m_ctx *ctx,

quoted

 			  const enum mxc_isi_video_type type)  {
 	if (type == MXC_ISI_VIDEO_M2M_CAP) {
-		/* Downscaling only  */
-		pix->width = min(pix->width, ctx->queues.out.format.width);
-		pix->height = min(pix->height, ctx->queues.out.format.height);
+		/* Downscaling one-sixteenth only  */

There are two spaces after "only".

Will remove it.

quoted

+		pix->width = CLAMP_DOWNSCALE_16(pix->width, ctx-
queues.out.format.width);
+		pix->height = CLAMP_DOWNSCALE_16(pix->height,
+ctx->queues.out.format.height);

To keep lines shorter, you can write

		const struct v4l2_pix_format_mplane *format =
			&ctx->queues.out.format;

		/* Downscaling only, by up to 16. */
		pix->width = CLAMP_DOWNSCALE_16(pix->width, format-

quoted

width);

		pix->height = CLAMP_DOWNSCALE_16(pix->height, format-

quoted

height);

Ok.

quoted

 	}

 	return mxc_isi_format_try(ctx->m2m->pipe, pix, type); diff --git
a/drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c
b/drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c
index

a41c51dd9ce0f2eeb779e9aa2461593b0d635f41..c3ffc8a38d1269c0a4e6493b
4d75

quoted

690f01cc87bb 100644

--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c

@@ -647,10 +647,8 @@ static int mxc_isi_pipe_set_selection(struct

v4l2_subdev *sd,

There's a comment above that states

		/* The sink crop is bound by the sink format downscaling only).
*/

That's not right, let's fix it:

		/*
		 * The ISI supports downscaling only, with a factor up to 16.
		 * Clamp the compose rectangle size accordingly.
		 */

Will fix.

Reading the driver code to check how rounding is done when programming the
hardware, I noticed that we clamp the bilinear scaling ratio to
ISI_DOWNSCALE_THRESHOLD, defined as 0x4000:

	return min_t(u32, from * 0x1000 / (to * *dec),
ISI_DOWNSCALE_THRESHOLD);

The ratio is documented to be a Q2.12 value, so 0x4000 is x1.0. The code seems
wrong to me, what do you think ?

Yes, you're right. ISI_DOWNSCALE_THRESHOLD should be 0x2000 because scaling
down by a factor greater than 2 is not supported with the bilinear filter, will fix in
next version. Thanks for pointing out.

quoted

 		sel->r.left = 0;
 		sel->r.top = 0;
-		sel->r.width = clamp(sel->r.width, MXC_ISI_MIN_WIDTH,
-				     format->width);
-		sel->r.height = clamp(sel->r.height, MXC_ISI_MIN_HEIGHT,
-				      format->height);
+		sel->r.width = CLAMP_DOWNSCALE_16(sel->r.width, format-
width);
+		sel->r.height = CLAMP_DOWNSCALE_16(sel->r.height, format-
height);

 		rect = mxc_isi_pipe_get_pad_compose(pipe, state,
 						    MXC_ISI_PIPE_PAD_SINK);

---
base-commit: eb4ee870747c3a77a9c3c84d84efb64bd481013a
change-id: 20260122-isi-74f87fbb9a6f

--
Regards,

Laurent Pinchart