[PATCH 0/4] media: qcom: camss: fixes for SoCs without IOMMU, RDI and CSID1 streaming

[PATCH 0/4] media: qcom: camss: fixes for SoCs without IOMMU, RDI and CSID1 streaming

[Allan Siriani]

These fixes came out of bringing up the CAMSS on MSM8226 (Moto G 2013,
"falcon", MT9M114 front camera on RDI), where the CAMSS is not behind an
IOMMU. The MSM8226 support itself will be sent separately; the problems
fixed here are generic.

1. The write masters take one base address per plane, but the buffers
   come from vb2_dma_sg and only the first scatterlist entry is used.
   Without an IOMMU the buffer is physically scattered and the frame is
   written over unrelated memory: on a 1280x960 RAW10 capture only the
   first 656 lines (the first 1 MiB segment) land in the buffer, and the
   device reboots within a few frames. Use vb2_dma_contig when the device is not IOMMU mapped, and
   refuse non contiguous DMA mappings on the vb2_dma_sg path.

2. In frame based (RDI) mode the VFE 4.1 write master buffer size is
   never programmed. Program WR_BUFFER_CFG as the downstream driver does.

3. video_stop_streaming() bails out on -ENOIOCTLCMD from a subdev
   without .s_stream() (e.g. the MT9M114 pixel array), leaving the
   pipeline started and the buffers with the driver: vb2 WARNs and the
   next STREAMON / "media-ctl -r" fail with -EBUSY.

4. csid_set_power() powers up the VFE of the same index as the CSID,
   and fails with -EINVAL when there is none. On MSM8916/MSM8226 (two
   CSIDs, one VFE behind the ISPIF) streaming from CSID1 fails with
   "Failed to power up pipeline: -22". Skip the VFE power up there.

Build tested (W=1) on next-20261104 for ARM with COMPILE_TEST.

Runtime tested on the MSM8226 (Moto G 2013) with the same changes applied
to a 6.16 based kernel, not yet on linux-next: on the rear AR0543 (RAW10
1280x960, CSIPHY0) every buffer gets exactly sizeimage bytes and nothing
past it, and STREAMOFF/STREAMON and "media-ctl -r" work repeatedly. On the
front MT9M114 (UYVY) the first frame after STREAMON can still run 128
bytes past sizeimage (one burst); later frames are exact. Patch 1 alone
already stops the memory corruption.

Allan Siriani (4):
  media: qcom: camss: use contiguous buffers when there is no IOMMU
  media: qcom: camss: vfe-4-1: program the RDI write master buffer size
  media: qcom: camss: always stop the pipeline in stop_streaming
  media: qcom: camss: do not fail CSID power up without a matching VFE

 drivers/media/platform/qcom/camss/Kconfig     |  1 +
 .../media/platform/qcom/camss/camss-vfe-4-1.c | 34 +++++++-
 .../media/platform/qcom/camss/camss-vfe-4-7.c |  3 +-
 .../media/platform/qcom/camss/camss-vfe-4-8.c |  3 +-
 .../platform/qcom/camss/camss-vfe-gen1.c      |  5 +-
 .../platform/qcom/camss/camss-vfe-gen1.h      |  3 +-
 .../media/platform/qcom/camss/camss-video.c   | 78 ++++++++++++++++---
 drivers/media/platform/qcom/camss/camss.c     | 17 ++--
 8 files changed, 115 insertions(+), 29 deletions(-)


base-commit: 8b7179210b22f8e48a82c5ceacb9d56a9c4cfec4
-- 
2.43.0

[PATCH 1/4] media: qcom: camss: use contiguous buffers when there is no IOMMU

[Allan Siriani]

The VFE write masters are programmed with a single base address per plane
and then write the whole plane from there. video_buf_init() takes that
address from sg_dma_address() of the first scatterlist entry of a
vb2_dma_sg buffer, which is only correct when the whole buffer is mapped
as one contiguous range in the device address space.

That holds when the CAMSS is behind an IOMMU, but not on SoCs where it is
not, such as MSM8226: there vb2_dma_sg allocates scattered pages, the
write master runs past the first segment and the frame lands in memory
that belongs to someone else, which quickly brings the system down.

When the CAMSS device is not IOMMU mapped, use vb2_dma_contig instead,
which allocates physically contiguous (CMA) buffers and checks that
imported USERPTR/DMABUF buffers are contiguous. Keep vb2_dma_sg when there
is an IOMMU, but refuse a buffer whose DMA segments are not contiguous
instead of handing a partial mapping to the hardware.

Signed-off-by: Allan Siriani <allan@fatecpompeia.edu.br>
---
 drivers/media/platform/qcom/camss/Kconfig     |  1 +
 .../media/platform/qcom/camss/camss-video.c   | 64 +++++++++++++++++--
 2 files changed, 58 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/Kconfig b/drivers/media/platform/qcom/camss/Kconfig
index b51745c94..3d3555b55 100644
--- a/drivers/media/platform/qcom/camss/Kconfig
+++ b/drivers/media/platform/qcom/camss/Kconfig
@@ -5,6 +5,7 @@ config VIDEO_QCOM_CAMSS
 	depends on (ARCH_QCOM && IOMMU_DMA) || COMPILE_TEST
 	select MEDIA_CONTROLLER
 	select VIDEO_V4L2_SUBDEV_API
+	select VIDEOBUF2_DMA_CONTIG
 	select VIDEOBUF2_DMA_SG
 	select V4L2_FWNODE
 	select GENERIC_PHY
diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
index 0852eb6f1..2c15546c8 100644
--- a/drivers/media/platform/qcom/camss/camss-video.c
+++ b/drivers/media/platform/qcom/camss/camss-video.c
@@ -7,12 +7,14 @@
  * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
  * Copyright (C) 2015-2018 Linaro Ltd.
  */
+#include <linux/device.h>
 #include <linux/slab.h>
 #include <media/media-entity.h>
 #include <media/v4l2-dev.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-mc.h>
+#include <media/videobuf2-dma-contig.h>
 #include <media/videobuf2-dma-sg.h>
 
 #include "camss-video.h"
@@ -146,6 +148,48 @@ static int video_queue_setup(struct vb2_queue *q,
 	return 0;
 }
 
+/*
+ * The VFE write masters take a single base address per plane and write the
+ * whole plane from there, so every plane must be contiguous in the device
+ * address space. Without an IOMMU the buffers come from vb2_dma_contig, which
+ * guarantees that. With an IOMMU the scatterlist mapping may still be split
+ * in several DMA segments (max_seg_size): accept it only when the segments
+ * follow each other in the IOVA space.
+ */
+static int video_buf_plane_addr(struct camss_video *video,
+				struct vb2_buffer *vb, unsigned int plane,
+				dma_addr_t *addr)
+{
+	struct scatterlist *sg;
+	struct sg_table *sgt;
+	dma_addr_t next;
+	unsigned int i;
+
+	if (video->vb2_q.mem_ops == &vb2_dma_contig_memops) {
+		*addr = vb2_dma_contig_plane_dma_addr(vb, plane);
+		return 0;
+	}
+
+	sgt = vb2_dma_sg_plane_desc(vb, plane);
+	if (!sgt || !sgt->nents)
+		return -EFAULT;
+
+	next = sg_dma_address(sgt->sgl);
+	for_each_sgtable_dma_sg(sgt, sg, i) {
+		if (sg_dma_address(sg) != next) {
+			dev_err(video->camss->dev,
+				"Plane %u is not contiguous in DMA space\n",
+				plane);
+			return -EINVAL;
+		}
+		next += sg_dma_len(sg);
+	}
+
+	*addr = sg_dma_address(sgt->sgl);
+
+	return 0;
+}
+
 static int video_buf_init(struct vb2_buffer *vb)
 {
 	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
@@ -154,15 +198,13 @@ static int video_buf_init(struct vb2_buffer *vb)
 						   vb);
 	const struct v4l2_pix_format_mplane *format =
 						&video->active_fmt.fmt.pix_mp;
-	struct sg_table *sgt;
 	unsigned int i;
+	int ret;
 
 	for (i = 0; i < format->num_planes; i++) {
-		sgt = vb2_dma_sg_plane_desc(vb, i);
-		if (!sgt)
-			return -EFAULT;
-
-		buffer->addr[i] = sg_dma_address(sgt->sgl);
+		ret = video_buf_plane_addr(video, vb, i, &buffer->addr[i]);
+		if (ret < 0)
+			return ret;
 	}
 
 	if (format->pixelformat == V4L2_PIX_FMT_NV12 ||
@@ -699,7 +741,15 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
 
 	q = &video->vb2_q;
 	q->drv_priv = video;
-	q->mem_ops = &vb2_dma_sg_memops;
+	/*
+	 * Without an IOMMU a scatter-gather buffer is physically scattered and
+	 * the write masters, which only take a base address, would run past
+	 * its first segment. Use physically contiguous (CMA) buffers instead.
+	 */
+	if (device_iommu_mapped(video->camss->dev))
+		q->mem_ops = &vb2_dma_sg_memops;
+	else
+		q->mem_ops = &vb2_dma_contig_memops;
 	q->ops = &msm_video_vb2_q_ops;
 	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
 	q->io_modes = VB2_DMABUF | VB2_MMAP | VB2_READ;
-- 
2.43.0

[PATCH 2/4] media: qcom: camss: vfe-4-1: program the RDI write master buffer size

[Allan Siriani]

The RDI outputs use the write masters in frame based mode. In that mode
vfe_wm_frame_based() only sets the FRM_BASED bit of WR_CFG and leaves
WR_BUFFER_CFG and WR_IMAGE_SIZE untouched: they keep their reset value or
whatever a previous line based (PIX) stream left there, so nothing tells
the write master how big the buffer is. If the sensor sends more data
than the configured format, the write master keeps writing past the end
of the V4L2 buffer. On a SoC without an IOMMU in front of the CAMSS
(found on MSM8226, whose VFE 4.0 shares this register layout) that
corrupts unrelated memory.

Program WR_BUFFER_CFG in frame based mode as the downstream VFE 4.0/4.1
driver does (msm_vfe40_axi_cfg_wm_reg(): words per line, lines minus one
and burst length, IMAGE_SIZE unused), from the bytesperline and height
of the active format, and clear it again when the write master is
released.

Pass the active format to the wm_frame_based() op for that. The VFE 4.7
and 4.8 implementations, which use a different register layout, are left
unchanged.

Signed-off-by: Allan Siriani <allan@fatecpompeia.edu.br>
---
 .../media/platform/qcom/camss/camss-vfe-4-1.c | 34 ++++++++++++++++---
 .../media/platform/qcom/camss/camss-vfe-4-7.c |  3 +-
 .../media/platform/qcom/camss/camss-vfe-4-8.c |  3 +-
 .../platform/qcom/camss/camss-vfe-gen1.c      |  5 +--
 .../platform/qcom/camss/camss-vfe-gen1.h      |  3 +-
 5 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
index 9cf1ccdb2..53ebc12c4 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
@@ -107,6 +107,7 @@
 #define VFE_0_BUS_IMAGE_MASTER_n_WR_UB_CFG_OFFSET_SHIFT	16
 #define VFE_0_BUS_IMAGE_MASTER_n_WR_IMAGE_SIZE(n)	(0x080 + 0x24 * (n))
 #define VFE_0_BUS_IMAGE_MASTER_n_WR_BUFFER_CFG(n)	(0x084 + 0x24 * (n))
+#define VFE_0_BUS_IMAGE_MASTER_n_WR_BUFFER_CFG_BURST_LEN	0x3
 #define VFE_0_BUS_IMAGE_MASTER_n_WR_FRAMEDROP_PATTERN(n)	\
 							(0x088 + 0x24 * (n))
 #define VFE_0_BUS_IMAGE_MASTER_n_WR_IRQ_SUBSAMPLE_PATTERN(n)	\
@@ -269,14 +270,39 @@ static void vfe_wm_enable(struct vfe_device *vfe, u8 wm, u8 enable)
 			    1 << VFE_0_BUS_IMAGE_MASTER_n_WR_CFG_WR_PATH_SHIFT);
 }
 
-static void vfe_wm_frame_based(struct vfe_device *vfe, u8 wm, u8 enable)
+/*
+ * In frame based mode (RDI) the write master stores the incoming data as is,
+ * with no line structure. Program its buffer size in WR_BUFFER_CFG (words per
+ * line, lines, burst length) as the downstream msm_vfe40_axi_cfg_wm_reg()
+ * does, and clear WR_IMAGE_SIZE, which may hold a stale line based setup,
+ * so that the write master is bounded by the size of the V4L2 buffer.
+ */
+static void vfe_wm_frame_based(struct vfe_device *vfe, u8 wm,
+			       struct v4l2_pix_format_mplane *pix, u8 enable)
 {
-	if (enable)
+	u32 reg;
+
+	if (enable) {
+		/*
+		 * bytesperline is aligned to 8 bytes (one bus word) on the RDI
+		 * outputs, and sizeimage >= bytesperline * height.
+		 */
+		reg = VFE_0_BUS_IMAGE_MASTER_n_WR_BUFFER_CFG_BURST_LEN;
+		reg |= (min(pix->height, 4096U) - 1) << 4;
+		reg |= (pix->plane_fmt[0].bytesperline / 8) << 16;
+
+		writel_relaxed(0, vfe->base +
+			       VFE_0_BUS_IMAGE_MASTER_n_WR_IMAGE_SIZE(wm));
+		writel_relaxed(reg, vfe->base +
+			       VFE_0_BUS_IMAGE_MASTER_n_WR_BUFFER_CFG(wm));
 		vfe_reg_set(vfe, VFE_0_BUS_IMAGE_MASTER_n_WR_CFG(wm),
 			1 << VFE_0_BUS_IMAGE_MASTER_n_WR_CFG_FRM_BASED_SHIFT);
-	else
+	} else {
 		vfe_reg_clr(vfe, VFE_0_BUS_IMAGE_MASTER_n_WR_CFG(wm),
 			1 << VFE_0_BUS_IMAGE_MASTER_n_WR_CFG_FRM_BASED_SHIFT);
+		writel_relaxed(0, vfe->base +
+			       VFE_0_BUS_IMAGE_MASTER_n_WR_BUFFER_CFG(wm));
+	}
 }
 
 static void vfe_get_wm_sizes(struct v4l2_pix_format_mplane *pix, u8 plane,
@@ -313,7 +339,7 @@ static void vfe_wm_line_based(struct vfe_device *vfe, u32 wm,
 
 		wpl = vfe_word_per_line(pix->pixelformat, bytesperline);
 
-		reg = 0x3;
+		reg = VFE_0_BUS_IMAGE_MASTER_n_WR_BUFFER_CFG_BURST_LEN;
 		reg |= (height - 1) << 4;
 		reg |= wpl << 16;
 
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-7.c b/drivers/media/platform/qcom/camss/camss-vfe-4-7.c
index 76729607d..308c845f2 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-4-7.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-4-7.c
@@ -317,7 +317,8 @@ static void vfe_wm_enable(struct vfe_device *vfe, u8 wm, u8 enable)
 			    1 << VFE_0_BUS_IMAGE_MASTER_n_WR_CFG_WR_PATH_SHIFT);
 }
 
-static void vfe_wm_frame_based(struct vfe_device *vfe, u8 wm, u8 enable)
+static void vfe_wm_frame_based(struct vfe_device *vfe, u8 wm,
+			       struct v4l2_pix_format_mplane *pix, u8 enable)
 {
 	if (enable)
 		vfe_reg_set(vfe, VFE_0_BUS_IMAGE_MASTER_n_WR_ADDR_CFG(wm),
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-8.c b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c
index b2f7d855d..65df06a42 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-4-8.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c
@@ -290,7 +290,8 @@ static void vfe_halt_clear(struct vfe_device *vfe)
 	writel_relaxed(0x0, vfe->base + VFE_0_BUS_BDG_CMD);
 }
 
-static void vfe_wm_frame_based(struct vfe_device *vfe, u8 wm, u8 enable)
+static void vfe_wm_frame_based(struct vfe_device *vfe, u8 wm,
+			       struct v4l2_pix_format_mplane *pix, u8 enable)
 {
 	if (enable)
 		vfe_reg_set(vfe, VFE_0_BUS_IMAGE_MASTER_n_WR_ADDR_CFG(wm),
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-gen1.c b/drivers/media/platform/qcom/camss/camss-vfe-gen1.c
index d84a375e3..f05676c2b 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-gen1.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-gen1.c
@@ -66,7 +66,7 @@ static int vfe_disable_output(struct vfe_line *line)
 	spin_lock_irqsave(&vfe->output_lock, flags);
 
 	if (line->id != VFE_LINE_PIX) {
-		vfe->ops_gen1->wm_frame_based(vfe, output->wm_idx[0], 0);
+		vfe->ops_gen1->wm_frame_based(vfe, output->wm_idx[0], NULL, 0);
 		vfe->ops_gen1->bus_disconnect_wm_from_rdi(vfe, output->wm_idx[0], line->id);
 		vfe->ops_gen1->enable_irq_wm_line(vfe, output->wm_idx[0], line->id, 0);
 		vfe->ops_gen1->set_cgc_override(vfe, output->wm_idx[0], 0);
@@ -244,7 +244,8 @@ static int vfe_enable_output(struct vfe_line *line)
 		vfe->ops_gen1->set_rdi_cid(vfe, line->id, 0);
 		vfe->ops_gen1->wm_set_ub_cfg(vfe, output->wm_idx[0],
 					    (ub_size + 1) * output->wm_idx[0], ub_size);
-		vfe->ops_gen1->wm_frame_based(vfe, output->wm_idx[0], 1);
+		vfe->ops_gen1->wm_frame_based(vfe, output->wm_idx[0],
+						    &line->video_out.active_fmt.fmt.pix_mp, 1);
 		vfe->ops_gen1->wm_enable(vfe, output->wm_idx[0], 1);
 		vfe->ops_gen1->bus_reload_wm(vfe, output->wm_idx[0]);
 	} else {
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-gen1.h b/drivers/media/platform/qcom/camss/camss-vfe-gen1.h
index 6d5f96565..83856c0bc 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-gen1.h
+++ b/drivers/media/platform/qcom/camss/camss-vfe-gen1.h
@@ -44,7 +44,8 @@ struct vfe_hw_ops_gen1 {
 	void (*set_realign_cfg)(struct vfe_device *vfe, struct vfe_line *line, u8 enable);
 	void (*set_qos)(struct vfe_device *vfe);
 	void (*set_xbar_cfg)(struct vfe_device *vfe, struct vfe_output *output, u8 enable);
-	void (*wm_frame_based)(struct vfe_device *vfe, u8 wm, u8 enable);
+	void (*wm_frame_based)(struct vfe_device *vfe, u8 wm,
+			       struct v4l2_pix_format_mplane *pix, u8 enable);
 	void (*wm_line_based)(struct vfe_device *vfe, u32 wm, struct v4l2_pix_format_mplane *pix,
 			      u8 plane, u32 enable);
 	void (*wm_set_ub_cfg)(struct vfe_device *vfe, u8 wm, u16 offset, u16 depth);
-- 
2.43.0

[PATCH 3/4] media: qcom: camss: always stop the pipeline in stop_streaming

[Allan Siriani]

video_stop_streaming() walks the pipeline upstream and calls
.s_stream(0) on every subdev. It gives up on the first non-zero return,
without stopping the media pipeline and without returning the queued
buffers to vb2.

A subdev without .s_stream() returns -ENOIOCTLCMD. video_start_streaming()
already ignores that, but video_stop_streaming() does not, so with such a
subdev in the pipeline, e.g. the pixel array entity of the MT9M114 sensor
driver, every stream off:

- triggers the vb2 WARN about buffers still owned by the driver,
- leaves the pipeline started, so the next VIDIOC_STREAMON and
  "media-ctl -r" fail with -EBUSY.

Treat -ENOIOCTLCMD as success, as at stream on, and only log real
errors, then always stop the pipeline and flush the buffers: a
.stop_streaming() callback cannot fail and must give back all buffers.

Signed-off-by: Allan Siriani <allan@fatecpompeia.edu.br>
---
 drivers/media/platform/qcom/camss/camss-video.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
index 2c15546c8..9091eaa18 100644
--- a/drivers/media/platform/qcom/camss/camss-video.c
+++ b/drivers/media/platform/qcom/camss/camss-video.c
@@ -361,12 +361,16 @@ static void video_stop_streaming(struct vb2_queue *q)
 		entity = pad->entity;
 		subdev = media_entity_to_v4l2_subdev(entity);
 
+		/*
+		 * Subdevs without .s_stream(), such as a sensor pixel array
+		 * entity, return -ENOIOCTLCMD, as at stream on. On a real error
+		 * keep going: the pipeline must be stopped and the buffers
+		 * returned to vb2 in any case.
+		 */
 		ret = v4l2_subdev_call(subdev, video, s_stream, 0);
-
-		if (ret) {
-			dev_err(video->camss->dev, "Video pipeline stop failed: %d\n", ret);
-			return;
-		}
+		if (ret && ret != -ENOIOCTLCMD)
+			dev_err(video->camss->dev,
+				"Failed to stop %s: %d\n", entity->name, ret);
 	}
 
 	video_device_pipeline_stop(vdev);
-- 
2.43.0

[PATCH 4/4] media: qcom: camss: do not fail CSID power up without a matching VFE

[Allan Siriani]

csid_set_power() powers up the VFE with the same index as the CSID
before the CSID itself, through parent_dev_ops->get(). The VFE
implementation, vfe_parent_dev_ops_get(), returns -EINVAL when there is
no VFE with that index.

On the ISPIF based SoCs there are more CSIDs than VFEs: MSM8916 (and
MSM8226) have two CSIDs and a single VFE, and the ISPIF routes either
CSID to it. Streaming from CSID1 then fails right away with

  qcom-camss ...: Failed to power up pipeline: -22

although the pipeline CSID1 -> ISPIF -> VFE0 is valid.

Return 0 when there is no VFE of the same index: there is nothing to
power up for that CSID, the VFE in the pipeline is powered by its own
subdev. vfe_parent_dev_ops_put() already ignores such indexes.

Signed-off-by: Allan Siriani <allan@fatecpompeia.edu.br>
---
 drivers/media/platform/qcom/camss/camss.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index c54185c93..532f6d36d 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -4631,15 +4631,16 @@ static int camss_read_version(struct camss *camss)
 
 static int vfe_parent_dev_ops_get(struct camss *camss, int id)
 {
-	int ret = -EINVAL;
-
-	if (id < camss->res->vfe_num) {
-		struct vfe_device *vfe = &camss->vfe[id];
-
-		ret = vfe_get(vfe);
-	}
+	/*
+	 * On the ISPIF based SoCs any CSID can feed any VFE, and there may be
+	 * more CSIDs than VFEs (MSM8916: two CSIDs, one VFE). A CSID without a
+	 * VFE of the same index has no parent to power up: the VFE that ends
+	 * up in the pipeline is powered by its own subdev.
+	 */
+	if (id >= camss->res->vfe_num)
+		return 0;
 
-	return ret;
+	return vfe_get(&camss->vfe[id]);
 }
 
 static int vfe_parent_dev_ops_put(struct camss *camss, int id)
-- 
2.43.0

Re: [PATCH 1/4] media: qcom: camss: use contiguous buffers when there is no IOMMU

[Loic Poulain]

On Wed, Nov 4, 2026 at 9:23 PM Allan Siriani <allan@fatecpompeia.edu.br> wrote:
>
> The VFE write masters are programmed with a single base address per plane
> and then write the whole plane from there. video_buf_init() takes that
> address from sg_dma_address() of the first scatterlist entry of a
> vb2_dma_sg buffer, which is only correct when the whole buffer is mapped
> as one contiguous range in the device address space.
>
> That holds when the CAMSS is behind an IOMMU, but not on SoCs where it is
> not, such as MSM8226: there vb2_dma_sg allocates scattered pages, the
> write master runs past the first segment and the frame lands in memory
> that belongs to someone else, which quickly brings the system down.
>
> When the CAMSS device is not IOMMU mapped, use vb2_dma_contig instead,
> which allocates physically contiguous (CMA) buffers and checks that
> imported USERPTR/DMABUF buffers are contiguous. Keep vb2_dma_sg when there
> is an IOMMU, but refuse a buffer whose DMA segments are not contiguous
> instead of handing a partial mapping to the hardware.
>
> Signed-off-by: Allan Siriani <allan@fatecpompeia.edu.br>
> ---
>  drivers/media/platform/qcom/camss/Kconfig     |  1 +
>  .../media/platform/qcom/camss/camss-video.c   | 64 +++++++++++++++++--
>  2 files changed, 58 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/Kconfig b/drivers/media/platform/qcom/camss/Kconfig
> index b51745c94..3d3555b55 100644
> --- a/drivers/media/platform/qcom/camss/Kconfig
> +++ b/drivers/media/platform/qcom/camss/Kconfig
> @@ -5,6 +5,7 @@ config VIDEO_QCOM_CAMSS
>         depends on (ARCH_QCOM && IOMMU_DMA) || COMPILE_TEST
>         select MEDIA_CONTROLLER
>         select VIDEO_V4L2_SUBDEV_API
> +       select VIDEOBUF2_DMA_CONTIG
>         select VIDEOBUF2_DMA_SG
>         select V4L2_FWNODE
>         select GENERIC_PHY
> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
> index 0852eb6f1..2c15546c8 100644
> --- a/drivers/media/platform/qcom/camss/camss-video.c
> +++ b/drivers/media/platform/qcom/camss/camss-video.c
> @@ -7,12 +7,14 @@
>   * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
>   * Copyright (C) 2015-2018 Linaro Ltd.
>   */
> +#include <linux/device.h>
>  #include <linux/slab.h>
>  #include <media/media-entity.h>
>  #include <media/v4l2-dev.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-ioctl.h>
>  #include <media/v4l2-mc.h>
> +#include <media/videobuf2-dma-contig.h>
>  #include <media/videobuf2-dma-sg.h>
>
>  #include "camss-video.h"
> @@ -146,6 +148,48 @@ static int video_queue_setup(struct vb2_queue *q,
>         return 0;
>  }
>
> +/*
> + * The VFE write masters take a single base address per plane and write the
> + * whole plane from there, so every plane must be contiguous in the device
> + * address space. Without an IOMMU the buffers come from vb2_dma_contig, which
> + * guarantees that. With an IOMMU the scatterlist mapping may still be split
> + * in several DMA segments (max_seg_size): accept it only when the segments
> + * follow each other in the IOVA space.
> + */
> +static int video_buf_plane_addr(struct camss_video *video,
> +                               struct vb2_buffer *vb, unsigned int plane,
> +                               dma_addr_t *addr)
> +{
> +       struct scatterlist *sg;
> +       struct sg_table *sgt;
> +       dma_addr_t next;
> +       unsigned int i;
> +
> +       if (video->vb2_q.mem_ops == &vb2_dma_contig_memops) {
> +               *addr = vb2_dma_contig_plane_dma_addr(vb, plane);
> +               return 0;
> +       }
> +
> +       sgt = vb2_dma_sg_plane_desc(vb, plane);
> +       if (!sgt || !sgt->nents)
> +               return -EFAULT;
> +
> +       next = sg_dma_address(sgt->sgl);
> +       for_each_sgtable_dma_sg(sgt, sg, i) {
> +               if (sg_dma_address(sg) != next) {
> +                       dev_err(video->camss->dev,
> +                               "Plane %u is not contiguous in DMA space\n",
> +                               plane);
> +                       return -EINVAL;
> +               }
> +               next += sg_dma_len(sg);
> +       }
> +
> +       *addr = sg_dma_address(sgt->sgl);
> +
> +       return 0;
> +}
> +
>  static int video_buf_init(struct vb2_buffer *vb)
>  {
>         struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> @@ -154,15 +198,13 @@ static int video_buf_init(struct vb2_buffer *vb)
>                                                    vb);
>         const struct v4l2_pix_format_mplane *format =
>                                                 &video->active_fmt.fmt.pix_mp;
> -       struct sg_table *sgt;
>         unsigned int i;
> +       int ret;
>
>         for (i = 0; i < format->num_planes; i++) {
> -               sgt = vb2_dma_sg_plane_desc(vb, i);
> -               if (!sgt)
> -                       return -EFAULT;
> -
> -               buffer->addr[i] = sg_dma_address(sgt->sgl);
> +               ret = video_buf_plane_addr(video, vb, i, &buffer->addr[i]);
> +               if (ret < 0)
> +                       return ret;
>         }
>
>         if (format->pixelformat == V4L2_PIX_FMT_NV12 ||
> @@ -699,7 +741,15 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>
>         q = &video->vb2_q;
>         q->drv_priv = video;
> -       q->mem_ops = &vb2_dma_sg_memops;
> +       /*
> +        * Without an IOMMU a scatter-gather buffer is physically scattered and
> +        * the write masters, which only take a base address, would run past
> +        * its first segment. Use physically contiguous (CMA) buffers instead.
> +        */
> +       if (device_iommu_mapped(video->camss->dev))
> +               q->mem_ops = &vb2_dma_sg_memops;
> +       else
> +               q->mem_ops = &vb2_dma_contig_memops;

So I think we should simply use vb2_dma_contig_memops in all cases. We
don't appear to support scatter-gather buffers properly anyway as we
only program a single address per plane and neither the driver nor the
hardware handles multiple DMA segments. We've likely been lucky so far
because the DMA mapping ended up as a single segment, or as contiguous
DMA segments...

>         q->ops = &msm_video_vb2_q_ops;
>         q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
>         q->io_modes = VB2_DMABUF | VB2_MMAP | VB2_READ;
> --
> 2.43.0
>

Re: [PATCH 0/4] media: qcom: camss: fixes for SoCs without IOMMU, RDI and CSID1 streaming

[Konrad Dybcio]

On 11/4/26 9:23 PM, Allan Siriani wrote:
> These fixes came out of bringing up the CAMSS on MSM8226 (Moto G 2013,
> "falcon", MT9M114 front camera on RDI), where the CAMSS is not behind an
> IOMMU. The MSM8226 support itself will be sent separately; the problems
> fixed here are generic.

I believe that there is indeed a IOMMU in front of this, at the very
least in front of the VFE and JPEG. I suppose this is "just" because
upstream doesn't support iommu on that SoC?

You may want to take a look at

https://lore.kernel.org/linux-arm-msm/20260809-msm8974-iommu-upstream-v1-0-87f5cd492560@oss.qualcomm.com/

+Cc Dmitry who poked at that topic

Konrad

Re: [PATCH 1/4] media: qcom: camss: use contiguous buffers when there is no IOMMU

[Bryan O'Donoghue]

On 04/11/2026 20:23, Allan Siriani wrote:
> +static int video_buf_plane_addr(struct camss_video *video,
> +				struct vb2_buffer *vb, unsigned int plane,
> +				dma_addr_t *addr)
> +{
> +	struct scatterlist *sg;
> +	struct sg_table *sgt;
> +	dma_addr_t next;
> +	unsigned int i;
> +
> +	if (video->vb2_q.mem_ops == &vb2_dma_contig_memops) {
> +		*addr = vb2_dma_contig_plane_dma_addr(vb, plane);
> +		return 0;
> +	}
> +
> +	sgt = vb2_dma_sg_plane_desc(vb, plane);
> +	if (!sgt || !sgt->nents)
> +		return -EFAULT;
> +
> +	next = sg_dma_address(sgt->sgl);
> +	for_each_sgtable_dma_sg(sgt, sg, i) {
> +		if (sg_dma_address(sg) != next) {
> +			dev_err(video->camss->dev,
> +				"Plane %u is not contiguous in DMA space\n",
> +				plane);
> +			return -EINVAL;
> +		}
> +		next += sg_dma_len(sg);
> +	}
> +
> +	*addr = sg_dma_address(sgt->sgl);
> +
> +	return 0;
> +}

STOP SENDING THIS SLOP !

---
bod

Re: [PATCH 1/4] media: qcom: camss: use contiguous buffers when there is no IOMMU

[Bryan O'Donoghue]

On 05/11/2026 09:57, Bryan O'Donoghue wrote:
> On 04/11/2026 20:23, Allan Siriani wrote:
>> +static int video_buf_plane_addr(struct camss_video *video,
>> +                struct vb2_buffer *vb, unsigned int plane,
>> +                dma_addr_t *addr)
>> +{
>> +    struct scatterlist *sg;
>> +    struct sg_table *sgt;
>> +    dma_addr_t next;
>> +    unsigned int i;
>> +
>> +    if (video->vb2_q.mem_ops == &vb2_dma_contig_memops) {
>> +        *addr = vb2_dma_contig_plane_dma_addr(vb, plane);
>> +        return 0;
>> +    }
>> +
>> +    sgt = vb2_dma_sg_plane_desc(vb, plane);
>> +    if (!sgt || !sgt->nents)
>> +        return -EFAULT;
>> +
>> +    next = sg_dma_address(sgt->sgl);
>> +    for_each_sgtable_dma_sg(sgt, sg, i) {
>> +        if (sg_dma_address(sg) != next) {
>> +            dev_err(video->camss->dev,
>> +                "Plane %u is not contiguous in DMA space\n",
>> +                plane);
>> +            return -EINVAL;
>> +        }
>> +        next += sg_dma_len(sg);
>> +    }
>> +
>> +    *addr = sg_dma_address(sgt->sgl);
>> +
>> +    return 0;
>> +}
> 
> STOP SENDING THIS SLOP !
> 
> ---
> bod

Oops - pardon me I thought you resent and NAK'd patch.

My mistake :)

---
bod

Re: [PATCH 1/4] media: qcom: camss: use contiguous buffers when there is no IOMMU

[Bryan O'Donoghue]

On 04/11/2026 20:23, Allan Siriani wrote:
> The VFE write masters are programmed with a single base address per plane
> and then write the whole plane from there. video_buf_init() takes that
> address from sg_dma_address() of the first scatterlist entry of a
> vb2_dma_sg buffer, which is only correct when the whole buffer is mapped
> as one contiguous range in the device address space.
> 
> That holds when the CAMSS is behind an IOMMU, but not on SoCs where it is
> not, such as MSM8226: there vb2_dma_sg allocates scattered pages, the
> write master runs past the first segment and the frame lands in memory
> that belongs to someone else, which quickly brings the system down.
> 
> When the CAMSS device is not IOMMU mapped, use vb2_dma_contig instead,
> which allocates physically contiguous (CMA) buffers and checks that
> imported USERPTR/DMABUF buffers are contiguous. Keep vb2_dma_sg when there
> is an IOMMU, but refuse a buffer whose DMA segments are not contiguous
> instead of handing a partial mapping to the hardware.
> 
> Signed-off-by: Allan Siriani <allan@fatecpompeia.edu.br>
> ---
>   drivers/media/platform/qcom/camss/Kconfig     |  1 +
>   .../media/platform/qcom/camss/camss-video.c   | 64 +++++++++++++++++--
>   2 files changed, 58 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/Kconfig b/drivers/media/platform/qcom/camss/Kconfig
> index b51745c94..3d3555b55 100644
> --- a/drivers/media/platform/qcom/camss/Kconfig
> +++ b/drivers/media/platform/qcom/camss/Kconfig
> @@ -5,6 +5,7 @@ config VIDEO_QCOM_CAMSS
>   	depends on (ARCH_QCOM && IOMMU_DMA) || COMPILE_TEST
>   	select MEDIA_CONTROLLER
>   	select VIDEO_V4L2_SUBDEV_API
> +	select VIDEOBUF2_DMA_CONTIG
>   	select VIDEOBUF2_DMA_SG
>   	select V4L2_FWNODE
>   	select GENERIC_PHY
> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
> index 0852eb6f1..2c15546c8 100644
> --- a/drivers/media/platform/qcom/camss/camss-video.c
> +++ b/drivers/media/platform/qcom/camss/camss-video.c
> @@ -7,12 +7,14 @@
>    * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
>    * Copyright (C) 2015-2018 Linaro Ltd.
>    */
> +#include <linux/device.h>
>   #include <linux/slab.h>
>   #include <media/media-entity.h>
>   #include <media/v4l2-dev.h>
>   #include <media/v4l2-device.h>
>   #include <media/v4l2-ioctl.h>
>   #include <media/v4l2-mc.h>
> +#include <media/videobuf2-dma-contig.h>
>   #include <media/videobuf2-dma-sg.h>
>   
>   #include "camss-video.h"
> @@ -146,6 +148,48 @@ static int video_queue_setup(struct vb2_queue *q,
>   	return 0;
>   }
>   
> +/*
> + * The VFE write masters take a single base address per plane and write the
> + * whole plane from there, so every plane must be contiguous in the device
> + * address space. Without an IOMMU the buffers come from vb2_dma_contig, which
> + * guarantees that. With an IOMMU the scatterlist mapping may still be split
> + * in several DMA segments (max_seg_size): accept it only when the segments
> + * follow each other in the IOVA space.
> + */
> +static int video_buf_plane_addr(struct camss_video *video,
> +				struct vb2_buffer *vb, unsigned int plane,
> +				dma_addr_t *addr)
> +{
> +	struct scatterlist *sg;
> +	struct sg_table *sgt;
> +	dma_addr_t next;
> +	unsigned int i;
> +
> +	if (video->vb2_q.mem_ops == &vb2_dma_contig_memops) {
> +		*addr = vb2_dma_contig_plane_dma_addr(vb, plane);
> +		return 0;
> +	}
> +
> +	sgt = vb2_dma_sg_plane_desc(vb, plane);
> +	if (!sgt || !sgt->nents)
> +		return -EFAULT;
> +
> +	next = sg_dma_address(sgt->sgl);
> +	for_each_sgtable_dma_sg(sgt, sg, i) {
> +		if (sg_dma_address(sg) != next) {
> +			dev_err(video->camss->dev,
> +				"Plane %u is not contiguous in DMA space\n",
> +				plane);
> +			return -EINVAL;
> +		}

This is the part that triggered me - we've already said NAK to checking 
that the buffer is contiguous and TBH I'm suspicious of being socially 
engineered about it.

20261029123107.28705-3-xiaqinghuan@mail.ustc.edu.cn

This patch does a similar thing checking for a contiguous IOVA 
allocation but this is a wrong approach.

So, if an allocator returns a buffer you have requested as physical, 
linear or iova contiguous - you don't spend time verifying that - its a 
waste of time.

If you genuinely needed a single segment out of a sg mapping use 
dma_set_max_seg_size() / dma_set_seg_boundary() to constrain the allocation.

Allocating and bailing out is not much use post-fact.
> +		next += sg_dma_len(sg);
> +	}
> +
> +	*addr = sg_dma_address(sgt->sgl);
> +
> +	return 0;
> +}
> +
>   static int video_buf_init(struct vb2_buffer *vb)
>   {
>   	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> @@ -154,15 +198,13 @@ static int video_buf_init(struct vb2_buffer *vb)
>   						   vb);
>   	const struct v4l2_pix_format_mplane *format =
>   						&video->active_fmt.fmt.pix_mp;
> -	struct sg_table *sgt;
>   	unsigned int i;
> +	int ret;
>   
>   	for (i = 0; i < format->num_planes; i++) {
> -		sgt = vb2_dma_sg_plane_desc(vb, i);
> -		if (!sgt)
> -			return -EFAULT;
> -
> -		buffer->addr[i] = sg_dma_address(sgt->sgl);
> +		ret = video_buf_plane_addr(video, vb, i, &buffer->addr[i]);
> +		if (ret < 0)
> +			return ret;
>   	}
>   
>   	if (format->pixelformat == V4L2_PIX_FMT_NV12 ||
> @@ -699,7 +741,15 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>   
>   	q = &video->vb2_q;
>   	q->drv_priv = video;
> -	q->mem_ops = &vb2_dma_sg_memops;
> +	/*
> +	 * Without an IOMMU a scatter-gather buffer is physically scattered and
> +	 * the write masters, which only take a base address, would run past
> +	 * its first segment. Use physically contiguous (CMA) buffers instead.
> +	 */
> +	if (device_iommu_mapped(video->camss->dev))
> +		q->mem_ops = &vb2_dma_sg_memops;
> +	else
> +		q->mem_ops = &vb2_dma_contig_memops;
>   	q->ops = &msm_video_vb2_q_ops;
>   	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
>   	q->io_modes = VB2_DMABUF | VB2_MMAP | VB2_READ;

Re: [PATCH 3/4] media: qcom: camss: always stop the pipeline in stop_streaming

[Bryan O'Donoghue]

On 04/11/2026 20:23, Allan Siriani wrote:
> video_stop_streaming() walks the pipeline upstream and calls
> .s_stream(0) on every subdev. It gives up on the first non-zero return,
> without stopping the media pipeline and without returning the queued
> buffers to vb2.
> 
> A subdev without .s_stream() returns -ENOIOCTLCMD. video_start_streaming()
> already ignores that, but video_stop_streaming() does not, so with such a
> subdev in the pipeline, e.g. the pixel array entity of the MT9M114 sensor
> driver, every stream off:
> 
> - triggers the vb2 WARN about buffers still owned by the driver,
> - leaves the pipeline started, so the next VIDIOC_STREAMON and
>    "media-ctl -r" fail with -EBUSY.
> 
> Treat -ENOIOCTLCMD as success, as at stream on, and only log real
> errors, then always stop the pipeline and flush the buffers: a
> .stop_streaming() callback cannot fail and must give back all buffers.
> 
> Signed-off-by: Allan Siriani <allan@fatecpompeia.edu.br>
Please add a Fixes:

then

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

---
bod