RE: [PATCH v2] media: nxp: imx8-isi: Add virtual channel support

Hi Laurent,

Thanks for your review.

-----Original Message-----
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Sent: Friday, March 20, 2026 6:47 AM
To: G.N. Zhou (OSS) <redacted>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Frank Li
[off-list ref]; Sascha Hauer [off-list ref]; Pengutronix
Kernel Team [off-list ref]; Fabio Estevam
[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]
Subject: Re: [PATCH v2] media: nxp: imx8-isi: Add virtual channel support

Hi Guoniu,

Thank you for the patch.

On Tue, Mar 10, 2026 at 02:53:10PM +0800, Guoniu Zhou wrote:

quoted

From: Guoniu Zhou <redacted>

Add virtual channel support for ISI driver.

You can drop this line, it duplicates the subject line.

quoted

The ISI supports different numbers of virtual channels depending on
the platform. i.MX95 supports 8 virtual channels, and i.MX8QXP/QM
support 4 virtual channels. They are used in multiple camera use
cases, such as surround view. Other platforms (such as
i.MX8MN/8MP/8ULP/93/91) don't support virtual channels, and the VC_ID

bits are marked as read-only.

quoted

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Guoniu Zhou <redacted>
---
Changes in v2:
- Add Rb tag from Frank Li
- Fix typo in comment(s/support/supports/)
- Update commit log to include more details about ISI virtual channel support
  on different platform
- Include bitfield.h file to fix following build error
  drivers/media/platform/nxp/imx8-isi/imx8-isi-regs.h:23:65: error:
implicit declaration of function ‘FIELD_PREP’
[-Wimplicit-function-declaration]
- Link to v1:
https://lore.kernel.org/r/20260309-isi_vc-v1-1-fd0b8035d1cd@nxp.com (local)

Changes in v1:
- Depends on
https://lore.kernel.org/linux-media/20251105-isi_imx95-v3-2-3987533cca
1c@nxp.com/
---
 .../media/platform/nxp/imx8-isi/imx8-isi-core.h    |  3 ++
 drivers/media/platform/nxp/imx8-isi/imx8-isi-hw.c  |  4 +-
 .../media/platform/nxp/imx8-isi/imx8-isi-pipe.c    | 43

++++++++++++++++++++++

quoted

 .../media/platform/nxp/imx8-isi/imx8-isi-regs.h    |  6 +--
 4 files changed, 52 insertions(+), 4 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..11a5e395792f11752c44d738
18c8

quoted

25f2f175aa1d 100644

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

@@ -256,6 +256,9 @@ struct mxc_isi_pipe {
 	u8				acquired_res;
 	u8				chained_res;
 	bool				chained;
+
+	/* Virtual channel ID for the ISI channel */
+	u8				vc;
 };

 struct mxc_isi_m2m {

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

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

0187d4ab97e8e28fca9013f6864a094e08f2c570..2babb8573227de9e1aa36d9
a39be

quoted

41b286cf0c57 100644

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

@@ -338,7 +338,9 @@ static void mxc_isi_channel_set_control(struct

mxc_isi_pipe *pipe,

quoted

 	} else {
 		val |= CHNL_CTRL_SRC_TYPE(CHNL_CTRL_SRC_TYPE_DEVICE);
 		val |= CHNL_CTRL_SRC_INPUT(input);
-		val |= CHNL_CTRL_MIPI_VC_ID(0); /* FIXME: For CSI-2 only */
+		val |= CHNL_CTRL_MIPI_VC_ID(pipe->vc);

You're not addressing the FIXME comment, so it should be kept (or ideally
addressed :-)).

Ok, will add it back.

quoted

+		/* Platform like i.MX95, ISI supports 8 virtual channels */
+		val |= CHNL_CTRL_VC_ID_1(pipe->vc >> 2);

This should be done for i.MX95 only. You also need to clear the bit above in the
function just after reading CHNL_CTRL (for i.MX95 only as well), otherwise
switching between different virtual channels won't work.

Yes, you're right. Will fix it in next version.

quoted

 	}

 	mxc_isi_write(pipe, CHNL_CTRL, val); 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..cc4348ea6006ee19243aae3
abceb

quoted

235d00beea4d 100644

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

@@ -232,6 +232,45 @@ static inline struct mxc_isi_pipe *to_isi_pipe(struct

v4l2_subdev *sd)

quoted

 	return container_of(sd, struct mxc_isi_pipe, sd);  }

+static int mxc_isi_get_vc(struct mxc_isi_pipe *pipe) {
+	struct mxc_isi_crossbar *xbar = &pipe->isi->crossbar;
+	struct device *dev = pipe->isi->dev;
+	struct v4l2_mbus_frame_desc source_fd;
+	struct v4l2_mbus_frame_desc_entry *entry = NULL;
+	unsigned int i;
+	int ret;
+
+	ret = v4l2_subdev_call(&xbar->sd, pad, get_frame_desc,
+			       xbar->num_sinks + pipe->id, &source_fd);

I don't see the xbar implementing get_frame_desc(). Am I missing a
dependency ?

Good catch, thank you.

The xbar get_frame_desc() implementation was developed separately and I 
mistakenly didn't include it in this series. You're not missing anything - 
it's my error in patch organization.

I will include it in next version to make this series self-contained and complete.

quoted

+	if (ret < 0) {
+		dev_err(dev, "Failed to get source frame desc from pad %u\n",
+			xbar->num_sinks + pipe->id);
+		return ret;
+	}
+
+	for (i = 0; i < source_fd.num_entries; i++) {
+		if (source_fd.entry[i].stream == 0) {
+			entry = &source_fd.entry[i];
+			break;
+		}
+	}
+
+	if (!entry) {
+		dev_err(dev, "Failed to find stream from source frame desc\n");
+		return -EPIPE;
+	}
+
+	if (entry->bus.csi2.vc >= pipe->isi->pdata->num_channels) {
+		dev_err(dev, "Virtual channel(%d) out of range\n",
+			entry->bus.csi2.vc);
+		return -EINVAL;
+	}
+
+	pipe->vc = entry->bus.csi2.vc;
+	return 0;
+}
+
 int mxc_isi_pipe_enable(struct mxc_isi_pipe *pipe)  {
 	struct mxc_isi_crossbar *xbar = &pipe->isi->crossbar; @@ -280,6
+319,10 @@ int mxc_isi_pipe_enable(struct mxc_isi_pipe *pipe)

 	v4l2_subdev_unlock_state(state);

+	ret = mxc_isi_get_vc(pipe);
+	if (ret)
+		return ret;
+
 	/* Configure the ISI channel. */
 	mxc_isi_channel_config(pipe, input, &in_size, &scale, &crop,
 			       sink_info->encoding, src_info->encoding); diff --git
a/drivers/media/platform/nxp/imx8-isi/imx8-isi-regs.h
b/drivers/media/platform/nxp/imx8-isi/imx8-isi-regs.h
index

1b65eccdf0da4bbc3a77c91e06fccc35d6c7e022..a4036da72f0057265e991087
f21b

quoted

c079bd6c6573 100644

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

@@ -6,6 +6,7 @@
 #ifndef __IMX8_ISI_REGS_H__
 #define __IMX8_ISI_REGS_H__

+#include <linux/bitfield.h>
 #include <linux/bits.h>

 /* ISI Registers Define  */

@@ -19,9 +20,8 @@
 #define CHNL_CTRL_CHAIN_BUF_NO_CHAIN				0
 #define CHNL_CTRL_CHAIN_BUF_2_CHAIN				1
 #define CHNL_CTRL_SW_RST					BIT(24)
-#define CHNL_CTRL_BLANK_PXL(n)					((n) <<

16)

I'm tempted to keep this, in case we'll need to set it later. We can add a
comment to indicate the field is only valid on i.MX8QM and i.MX8QXP:

#define CHNL_CTRL_BLANK_PXL(n)					((n) <<
16)	/* i.MX8{QM,QXP} */

or

#define CHNL_CTRL_BLANK_PXL(n)
	FIELD_PREP(GENMASK(23, 16), (n))	/* i.MX8{QM,QXP} */

quoted

-#define CHNL_CTRL_BLANK_PXL_MASK

	GENMASK(23, 16)

quoted

-#define CHNL_CTRL_MIPI_VC_ID(n)					((n) <<

6)

quoted

+#define CHNL_CTRL_VC_ID_1(n)

	FIELD_PREP(BIT(16), (n))

Please also add a comment to indicate the field is valid on i.MX95 only.

#define CHNL_CTRL_VC_ID_1(n)
	FIELD_PREP(BIT(16), (n))	/* i.MX95 */

Sounds good. I will add a comment in next version to clarify this.

quoted

+#define CHNL_CTRL_MIPI_VC_ID(n)

	FIELD_PREP(GENMASK(7, 6), (n))

quoted

 #define CHNL_CTRL_MIPI_VC_ID_MASK

	GENMASK(7, 6)

quoted

 #define CHNL_CTRL_SRC_TYPE(n)					((n) <<

4)

quoted

 #define CHNL_CTRL_SRC_TYPE_MASK					BIT(4)

---
base-commit: 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f
change-id: 20260309-isi_vc-285fd815140e
prerequisite-patch-id: 6f139a1d54fa3e0632db9b8a736ae27037c5f45a

--
Regards,

Laurent Pinchart