Re: [PATCH 1/4] media: qcom: camss: use contiguous buffers when there is no IOMMU
From: Bryan O'Donoghue
Date: Thu Nov 05 2026 - 09:44:57 EST
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@xxxxxxxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxxx
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;