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

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

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;