[PATCH 0/3] media: camss: three fixes for the CSID power and VFE streaming paths

[PATCH 0/3] media: camss: three fixes for the CSID power and VFE streaming paths

[Qianhui Xu]

These three patches are fixes found while bringing camss up on a new SoC,
but none of them depends on that new SoC support: each one is against code
that already exists in mainline and can be reviewed on its own.

As posted earlier, the SM8750 specific patches of the previous series are
withdrawn and that work stays with the existing upstream series:
<179325717815.489400.16602455641444097904@mail.ustc.edu.cn>.

1/3 csid: the power-on path repeats its cleanup sequence in every error
    branch and gets the first one wrong: when pm_runtime_resume_and_get()
    fails, the parent device reference taken by parent_dev_ops->get() is
    never dropped, so the VFE stays powered.  Fold the four open-coded
    blocks into a single label chain and route the pm_runtime failure
    through the parent put as well.

2/3 video: the write master of the newer VFE generations can only be
    programmed with a physically contiguous payload, because the frame
    increment registers describe one linear address range.  A buffer whose
    plane was not contiguous was silently written past the end of the
    first scatterlist entry.  Reject such buffers instead, and add a
    prepare_buffer() video operation so SoC code can add its own
    alignment requirements after the generic checks.

    For the contiguity requirement that Bryan asked to see cited: the
    downstream write master driver (camera-kernel
    drivers/cam_isp/isp_hw_mgr/isp_hw/vfe_hw/vfe_bus/cam_vfe_bus_ver3.c)
    programs each write master with a single frame increment of
    stride * height (line 3865) and aligns it to 256 bytes (line 3908),
    with no per-fragment addressing.  If this citation is not considered
    sufficient, patch 2/3 will be dropped.

3/3 video: when one subdevice fails to start, the ones already started
    further up the pipeline were left streaming and the next start tried
    to start them again.  Factor the teardown loop out of
    video_stop_streaming() and run it from the error path; also keep
    stopping the remaining subdevices when one of them fails to stop
    instead of returning early.

All three are build tested with W=1 on drivers/media/platform/qcom/camss/
and apply cleanly on v7.3-rc7.  They touch only camss-csid.c and
camss-video.c, which no patch of the Kaanapali series (v23) or of the
SM8450 series changes, so they apply independently of both.

This series intentionally contains no SoC enablement, no DT binding and no
CSIPHY change; those are being discussed in separate threads.

Qianhui Xu (3):
  media: camss: csid: unwind the power-on path through a single error
    label
  media: camss: video: check that a buffer is DMA contiguous and add a
    prepare hook
  media: camss: video: stop the subdevices already started when a start
    fails

 .../media/platform/qcom/camss/camss-csid.c    | 45 +++++------
 .../media/platform/qcom/camss/camss-video.c   | 78 +++++++++++--------
 .../media/platform/qcom/camss/camss-video.h   |  1 +
 3 files changed, 68 insertions(+), 56 deletions(-)

-- 
2.53.0

[PATCH 1/3] media: camss: csid: unwind the power-on path through a single error label

[Qianhui Xu]

csid_set_power() repeats the same cleanup sequence in every error branch
and gets it wrong in the first one: when pm_runtime_resume_and_get()
fails, the parent device reference taken by parent_dev_ops->get() is
never dropped, so a failing power-on leaks it and the matching VFE stays
powered.

Replace the four open-coded unwind blocks with gotos to a shared label
chain and route the pm_runtime failure through the parent put as well.
No functional change on the success path.

Signed-off-by: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>
---
 .../media/platform/qcom/camss/camss-csid.c    | 45 +++++++++----------
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
index 48459b46a9..592c1dfc97 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.c
+++ b/drivers/media/platform/qcom/camss/camss-csid.c
@@ -696,44 +696,28 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
 
 		ret = pm_runtime_resume_and_get(dev);
 		if (ret < 0)
-			return ret;
+			goto put_parent;
 
 		ret = regulator_bulk_enable(csid->num_supplies,
 					    csid->supplies);
-		if (ret < 0) {
-			pm_runtime_put_sync(dev);
-			return ret;
-		}
+		if (ret < 0)
+			goto put_pm;
 
 		ret = csid_set_clock_rates(csid);
-		if (ret < 0) {
-			regulator_bulk_disable(csid->num_supplies,
-					       csid->supplies);
-			pm_runtime_put_sync(dev);
-			return ret;
-		}
+		if (ret < 0)
+			goto disable_regulators;
 
 		ret = camss_enable_clocks(csid->nclocks, csid->clock, dev);
-		if (ret < 0) {
-			regulator_bulk_disable(csid->num_supplies,
-					       csid->supplies);
-			pm_runtime_put_sync(dev);
-			return ret;
-		}
+		if (ret < 0)
+			goto disable_regulators;
 
 		csid->phy.need_vc_update = true;
 
 		enable_irq(csid->irq);
 
 		ret = csid->res->hw_ops->reset(csid);
-		if (ret < 0) {
-			disable_irq(csid->irq);
-			camss_disable_clocks(csid->nclocks, csid->clock);
-			regulator_bulk_disable(csid->num_supplies,
-					       csid->supplies);
-			pm_runtime_put_sync(dev);
-			return ret;
-		}
+		if (ret < 0)
+			goto disable_clocks;
 
 		csid->res->hw_ops->hw_version(csid);
 	} else {
@@ -746,6 +730,17 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
 	}
 
 	return ret;
+
+disable_clocks:
+	disable_irq(csid->irq);
+	camss_disable_clocks(csid->nclocks, csid->clock);
+disable_regulators:
+	regulator_bulk_disable(csid->num_supplies, csid->supplies);
+put_pm:
+	pm_runtime_put_sync(dev);
+put_parent:
+	csid->res->parent_dev_ops->put(camss, csid->id);
+	return ret;
 }
 
 /*

[PATCH 2/3] media: camss: video: check that a buffer is DMA contiguous and add a prepare hook

[Qianhui Xu]

The write master of the newer VFE generations can only be programmed with
a buffer whose payload is physically contiguous, because the frame
increment registers describe one linear address range.  Check that the DMA
mapping of every plane is contiguous for at least sizeimage bytes and
reject the buffer otherwise, instead of silently writing past the end of
the first scatterlist entry.

Also let the SoC code add its own alignment requirements to a queued
buffer through a new prepare_buffer() video operation, which runs after
the generic checks.

Signed-off-by: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>
---
 .../media/platform/qcom/camss/camss-video.c    | 18 +++++++++++++++++-
 .../media/platform/qcom/camss/camss-video.h    |  1 +
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
index 0852eb6f13..027cf024f9 100644
--- a/drivers/media/platform/qcom/camss/camss-video.c
+++ b/drivers/media/platform/qcom/camss/camss-video.c
@@ -155,7 +155,10 @@ static int video_buf_init(struct vb2_buffer *vb)
 	const struct v4l2_pix_format_mplane *format =
 						&video->active_fmt.fmt.pix_mp;
 	struct sg_table *sgt;
-	unsigned int i;
+	struct scatterlist *sg;
+	dma_addr_t end;
+	unsigned int i, j;
+	size_t contiguous;
 
 	for (i = 0; i < format->num_planes; i++) {
 		sgt = vb2_dma_sg_plane_desc(vb, i);
@@ -163,6 +166,16 @@ static int video_buf_init(struct vb2_buffer *vb)
 			return -EFAULT;
 
 		buffer->addr[i] = sg_dma_address(sgt->sgl);
+		end = buffer->addr[i];
+		contiguous = 0;
+		for_each_sgtable_dma_sg(sgt, sg, j) {
+			if (sg_dma_address(sg) != end)
+				break;
+			contiguous += sg_dma_len(sg);
+			end += sg_dma_len(sg);
+		}
+		if (contiguous < format->plane_fmt[i].sizeimage)
+			return -EINVAL;
 	}
 
 	if (format->pixelformat == V4L2_PIX_FMT_NV12 ||
@@ -193,6 +206,9 @@ static int video_buf_prepare(struct vb2_buffer *vb)
 
 	vbuf->field = V4L2_FIELD_NONE;
 
+	if (video->ops->prepare_buffer)
+		return video->ops->prepare_buffer(video, vb);
+
 	return 0;
 }
 
diff --git a/drivers/media/platform/qcom/camss/camss-video.h b/drivers/media/platform/qcom/camss/camss-video.h
index d3e56e240a..af046ecb7f 100644
--- a/drivers/media/platform/qcom/camss/camss-video.h
+++ b/drivers/media/platform/qcom/camss/camss-video.h
@@ -28,6 +28,7 @@ struct camss_buffer {
 struct camss_video;
 
 struct camss_video_ops {
+	int (*prepare_buffer)(struct camss_video *vid, struct vb2_buffer *vb);
 	int (*queue_buffer)(struct camss_video *vid, struct camss_buffer *buf);
 	int (*flush_buffers)(struct camss_video *vid,
 			     enum vb2_buffer_state state);

Re: [PATCH 2/3] media: camss: video: check that a buffer is DMA contiguous and add a prepare hook

[Loic Poulain]

On Thu, Oct 29, 2026 at 1:31 PM Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn> wrote:
>
> The write master of the newer VFE generations can only be programmed with
> a buffer whose payload is physically contiguous, because the frame
> increment registers describe one linear address range.  Check that the DMA
> mapping of every plane is contiguous for at least sizeimage bytes and
> reject the buffer otherwise, instead of silently writing past the end of
> the first scatterlist entry.

Have you tested the non-contiguous case? It’s a bit surprising to me,
since the device-visible IOVA is still contiguous.

Perhaps this is related to frame subsampling, in which case the frame
buffers themselves would need to remain contiguous to take advantage
of the hardware frame-increment mechanism. But even then, the relevant
address space from the hardware's perspective should still be the
IOVA.

Regards,
Loic

>
> Also let the SoC code add its own alignment requirements to a queued
> buffer through a new prepare_buffer() video operation, which runs after
> the generic checks.
>
> Signed-off-by: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>
> ---
>  .../media/platform/qcom/camss/camss-video.c    | 18 +++++++++++++++++-
>  .../media/platform/qcom/camss/camss-video.h    |  1 +
>  2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
> index 0852eb6f13..027cf024f9 100644
> --- a/drivers/media/platform/qcom/camss/camss-video.c
> +++ b/drivers/media/platform/qcom/camss/camss-video.c
> @@ -155,7 +155,10 @@ static int video_buf_init(struct vb2_buffer *vb)
>         const struct v4l2_pix_format_mplane *format =
>                                                 &video->active_fmt.fmt.pix_mp;
>         struct sg_table *sgt;
> -       unsigned int i;
> +       struct scatterlist *sg;
> +       dma_addr_t end;
> +       unsigned int i, j;
> +       size_t contiguous;
>
>         for (i = 0; i < format->num_planes; i++) {
>                 sgt = vb2_dma_sg_plane_desc(vb, i);
> @@ -163,6 +166,16 @@ static int video_buf_init(struct vb2_buffer *vb)
>                         return -EFAULT;
>
>                 buffer->addr[i] = sg_dma_address(sgt->sgl);
> +               end = buffer->addr[i];
> +               contiguous = 0;
> +               for_each_sgtable_dma_sg(sgt, sg, j) {
> +                       if (sg_dma_address(sg) != end)
> +                               break;
> +                       contiguous += sg_dma_len(sg);
> +                       end += sg_dma_len(sg);
> +               }
> +               if (contiguous < format->plane_fmt[i].sizeimage)
> +                       return -EINVAL;
>         }
>
>         if (format->pixelformat == V4L2_PIX_FMT_NV12 ||
> @@ -193,6 +206,9 @@ static int video_buf_prepare(struct vb2_buffer *vb)
>
>         vbuf->field = V4L2_FIELD_NONE;
>
> +       if (video->ops->prepare_buffer)
> +               return video->ops->prepare_buffer(video, vb);
> +
>         return 0;
>  }
>
> diff --git a/drivers/media/platform/qcom/camss/camss-video.h b/drivers/media/platform/qcom/camss/camss-video.h
> index d3e56e240a..af046ecb7f 100644
> --- a/drivers/media/platform/qcom/camss/camss-video.h
> +++ b/drivers/media/platform/qcom/camss/camss-video.h
> @@ -28,6 +28,7 @@ struct camss_buffer {
>  struct camss_video;
>
>  struct camss_video_ops {
> +       int (*prepare_buffer)(struct camss_video *vid, struct vb2_buffer *vb);
>         int (*queue_buffer)(struct camss_video *vid, struct camss_buffer *buf);
>         int (*flush_buffers)(struct camss_video *vid,
>                              enum vb2_buffer_state state);
>

Re: [PATCH 2/3] media: camss: video: check that a buffer is DMA contiguous and add a prepare hook

[Bryan O'Donoghue]

On 29/10/2026 12:31, Qianhui Xu wrote:
> The write master of the newer VFE generations can only be programmed with
> a buffer whose payload is physically contiguous

The first problem is that is _not_ what your code checks - you should 
use sg_phys() for that - which gets the physical address of a CPU 
scatterlist entry.

What your code currently does is check that the IOVA addresses are 
contiguous.

But what happens is an IOVA allocation is made which is linearly 
contiguous in the IOVA address space and then SMMU entries map the IOVA 
back to physical entries.

So all we need to do is give the IOVA base to CAMSS and it can write 
IOVA buffers - its the SMMU's job to ensure the resulting upstream 
writes land at the right physical address.

> increment registers describe one linear address range.  Check that the DMA
> mapping of every plane is contiguous for at least sizeimage bytes and
> reject the buffer otherwise, instead of silently writing past the end of
> the first scatterlist entry.

> Also let the SoC code add its own alignment requirements to a queued
> buffer through a new prepare_buffer() video operation, which runs after
> the generic checks.
> 
> Signed-off-by: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>

And I can prove it too.

buildroot# uname -a
Linux buildroot 7.3.0-rc1+ #19 SMP PREEMPT Thu Oct 29 13:26:34 GMT 2026 
aarch64 GNU/Linux
buildroot# dmesg | grep -i crd
[    0.000000] Machine model: Qualcomm Technologies, Inc. X1E80100 CRD
buildroot# dmesg | grep "plane 0"
qcom-camss acb7000.isp: plane 0 chunk 0: phys 0x000000089a800000 len 8388608
qcom-camss acb7000.isp: plane 0 chunk 1: phys 0x000000089a600000 len 2097152
qcom-camss acb7000.isp: plane 0 chunk 2: phys 0x0000000899900000 len 1048576
qcom-camss acb7000.isp: plane 0 chunk 3: phys 0x0000000899b00000 len 131072
qcom-camss acb7000.isp: plane 0: 11677696 bytes, 6 cpu entries, 5 
physical breaks, largest physical run 8388608, 1 dma segments, 0 iova 
breaks, iova 0x00000000ff000000

Plane 0 == 11677696 bytes
Largest physical run = 8388608

Interestingly we see some physically adjacent allocations here and then 
also non-contiguous allocations, which is the negation of the thesis, 
constraining IOVA to physically contiguous maps is provably incorrect.

NAK

diff --git a/drivers/media/platform/qcom/camss/camss-video.c 
b/drivers/media/platform/qcom/camss/camss-video.c
index 0852eb6f13..d2df368d5f 100644
--- a/drivers/media/platform/qcom/camss/camss-video.c
+++ b/drivers/media/platform/qcom/camss/camss-video.c
@@ -146,6 +146,44 @@ static int video_queue_setup(struct vb2_queue *q,
  	return 0;
  }

+/* DEBUG: how a plane is laid out in physical and IOVA space */
+static void video_buf_dump_sg(struct camss_video *video, unsigned int 
plane,
+			      struct sg_table *sgt)
+{
+	dma_addr_t iova = sg_dma_address(sgt->sgl);
+	unsigned int j, pbreaks = 0, dbreaks = 0;
+	size_t total = 0, run = 0, maxrun = 0;
+	struct scatterlist *sg;
+	phys_addr_t pend = 0;
+	dma_addr_t dend = 0;
+
+	for_each_sgtable_sg(sgt, sg, j) {
+		if (j && sg_phys(sg) != pend) {
+			pbreaks++;
+			run = 0;
+		}
+		if (j < 4)
+			dev_info(video->camss->dev, "plane %u chunk %u: phys %pa len %u\n",
+				 plane, j, &(phys_addr_t){ sg_phys(sg) }, sg->length);
+		run += sg->length;
+		if (run > maxrun)
+			maxrun = run;
+		total += sg->length;
+		pend = sg_phys(sg) + sg->length;
+	}
+
+	for_each_sgtable_dma_sg(sgt, sg, j) {
+		if (j && sg_dma_address(sg) != dend)
+			dbreaks++;
+		dend = sg_dma_address(sg) + sg_dma_len(sg);
+	}
+
+	dev_info(video->camss->dev,
+		 "plane %u: %zu bytes, %u cpu entries, %u physical breaks, largest 
physical run %zu, %u dma segments, %u iova breaks, iova %pad\n",
+		 plane, total, sgt->orig_nents, pbreaks, maxrun, sgt->nents,
+		 dbreaks, &iova);
+}
+
  static int video_buf_init(struct vb2_buffer *vb)
  {
  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
@@ -163,6 +201,7 @@ static int video_buf_init(struct vb2_buffer *vb)
  			return -EFAULT;

  		buffer->addr[i] = sg_dma_address(sgt->sgl);
+		video_buf_dump_sg(video, i, sgt);
  	}

  	if (format->pixelformat == V4L2_PIX_FMT_NV12 ||

[PATCH 3/3] media: camss: video: stop the subdevices already started when a start fails

[Qianhui Xu]

When one subdevice fails to start, the ones already started further up the
pipeline were left streaming, and the next start would try to start them
again.  Factor the teardown loop out of video_stop_streaming() into
video_stop_subdevices() and run it from the error path, stopping at the
subdevice that failed.

While at it, keep stopping the remaining subdevices when one of them fails
to stop instead of returning early: the pipeline is being torn down, so a
failure there must not leave the rest running.

Signed-off-by: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>
---
 .../media/platform/qcom/camss/camss-video.c   | 60 +++++++++----------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
index 027cf024f9..ee9ffd0432 100644
--- a/drivers/media/platform/qcom/camss/camss-video.c
+++ b/drivers/media/platform/qcom/camss/camss-video.c
@@ -265,11 +265,37 @@ static int video_prepare_streaming(struct vb2_queue *q)
 	return ret;
 }
 
+/* Stop the started subdevices, excluding the one that failed to start. */
+static void video_stop_subdevices(struct camss_video *video,
+				  struct media_entity *stop)
+{
+	struct media_entity *entity = &video->vdev.entity;
+	struct v4l2_subdev *subdev;
+	struct media_pad *pad;
+	int ret;
+
+	while (entity != stop) {
+		pad = &entity->pads[0];
+		if (!(pad->flags & MEDIA_PAD_FL_SINK))
+			break;
+		pad = media_pad_remote_pad_first(pad);
+		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
+			break;
+		entity = pad->entity;
+		if (entity == stop)
+			break;
+		subdev = media_entity_to_v4l2_subdev(entity);
+		ret = v4l2_subdev_call(subdev, video, s_stream, 0);
+		if (ret && ret != -ENOIOCTLCMD)
+			dev_err(video->camss->dev, "Video pipeline stop failed: %d\n", ret);
+	}
+}
+
 static int video_start_streaming(struct vb2_queue *q, unsigned int count)
 {
 	struct camss_video *video = vb2_get_drv_priv(q);
 	struct video_device *vdev = &video->vdev;
-	struct media_entity *entity;
+	struct media_entity *entity = &vdev->entity;
 	struct media_pad *pad;
 	struct v4l2_subdev *subdev;
 	int ret;
@@ -284,7 +310,6 @@ static int video_start_streaming(struct vb2_queue *q, unsigned int count)
 	if (ret < 0)
 		goto error;
 
-	entity = &vdev->entity;
 	while (1) {
 		pad = &entity->pads[0];
 		if (!(pad->flags & MEDIA_PAD_FL_SINK))
@@ -305,6 +330,7 @@ static int video_start_streaming(struct vb2_queue *q, unsigned int count)
 	return 0;
 
 error:
+	video_stop_subdevices(video, entity);
 	video_device_pipeline_stop(vdev);
 
 flush_buffers:
@@ -316,35 +342,9 @@ static int video_start_streaming(struct vb2_queue *q, unsigned int count)
 static void video_stop_streaming(struct vb2_queue *q)
 {
 	struct camss_video *video = vb2_get_drv_priv(q);
-	struct video_device *vdev = &video->vdev;
-	struct media_entity *entity;
-	struct media_pad *pad;
-	struct v4l2_subdev *subdev;
-	int ret;
-
-	entity = &vdev->entity;
-	while (1) {
-		pad = &entity->pads[0];
-		if (!(pad->flags & MEDIA_PAD_FL_SINK))
-			break;
-
-		pad = media_pad_remote_pad_first(pad);
-		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
-			break;
-
-		entity = pad->entity;
-		subdev = media_entity_to_v4l2_subdev(entity);
-
-		ret = v4l2_subdev_call(subdev, video, s_stream, 0);
-
-		if (ret) {
-			dev_err(video->camss->dev, "Video pipeline stop failed: %d\n", ret);
-			return;
-		}
-	}
-
-	video_device_pipeline_stop(vdev);
 
+	video_stop_subdevices(video, NULL);
+	video_device_pipeline_stop(&video->vdev);
 	video->ops->flush_buffers(video, VB2_BUF_STATE_ERROR);
 }
 

Re: [PATCH 3/3] media: camss: video: stop the subdevices already started when a start fails

[Loic Poulain]

On Thu, Oct 29, 2026 at 1:31 PM Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn> wrote:
>
> When one subdevice fails to start, the ones already started further up the
> pipeline were left streaming, and the next start would try to start them
> again.  Factor the teardown loop out of video_stop_streaming() into
> video_stop_subdevices() and run it from the error path, stopping at the
> subdevice that failed.
>
> While at it, keep stopping the remaining subdevices when one of them fails
> to stop instead of returning early: the pipeline is being torn down, so a
> failure there must not leave the rest running.
>
> Signed-off-by: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>
> ---
>  .../media/platform/qcom/camss/camss-video.c   | 60 +++++++++----------
>  1 file changed, 30 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
> index 027cf024f9..ee9ffd0432 100644
> --- a/drivers/media/platform/qcom/camss/camss-video.c
> +++ b/drivers/media/platform/qcom/camss/camss-video.c
> @@ -265,11 +265,37 @@ static int video_prepare_streaming(struct vb2_queue *q)
>         return ret;
>  }
>
> +/* Stop the started subdevices, excluding the one that failed to start. */
> +static void video_stop_subdevices(struct camss_video *video,
> +                                 struct media_entity *stop)
> +{
> +       struct media_entity *entity = &video->vdev.entity;
> +       struct v4l2_subdev *subdev;
> +       struct media_pad *pad;
> +       int ret;
> +
> +       while (entity != stop) {
> +               pad = &entity->pads[0];
> +               if (!(pad->flags & MEDIA_PAD_FL_SINK))
> +                       break;
> +               pad = media_pad_remote_pad_first(pad);
> +               if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
> +                       break;
> +               entity = pad->entity;
> +               if (entity == stop)
> +                       break;
> +               subdev = media_entity_to_v4l2_subdev(entity);
> +               ret = v4l2_subdev_call(subdev, video, s_stream, 0);
> +               if (ret && ret != -ENOIOCTLCMD)
> +                       dev_err(video->camss->dev, "Video pipeline stop failed: %d\n", ret);
> +       }
> +}
> +

Now you're here, It might also make sense to introduce a generic CAMSS
helper to walk the media graph upstream and start/stop all subdevices
in the pipeline, something along the lines of
camss_video_subdevs_set_stream(.. bool enable).
While doing so, you could leverage more modern framework helpers, such as:
- v4l2_subdev_enable_streams() and v4l2_subdev_disable_streams()
instead of legacy s_stream()
- media_entity_remote_source_pad_unique() to locate the upstream
entity rather than relying on pads[0]

Regards,
Loic

>  static int video_start_streaming(struct vb2_queue *q, unsigned int count)
>  {
>         struct camss_video *video = vb2_get_drv_priv(q);
>         struct video_device *vdev = &video->vdev;
> -       struct media_entity *entity;
> +       struct media_entity *entity = &vdev->entity;
>         struct media_pad *pad;
>         struct v4l2_subdev *subdev;
>         int ret;
> @@ -284,7 +310,6 @@ static int video_start_streaming(struct vb2_queue *q, unsigned int count)
>         if (ret < 0)
>                 goto error;
>
> -       entity = &vdev->entity;
>         while (1) {
>                 pad = &entity->pads[0];
>                 if (!(pad->flags & MEDIA_PAD_FL_SINK))
> @@ -305,6 +330,7 @@ static int video_start_streaming(struct vb2_queue *q, unsigned int count)
>         return 0;
>
>  error:
> +       video_stop_subdevices(video, entity);
>         video_device_pipeline_stop(vdev);
>
>  flush_buffers:
> @@ -316,35 +342,9 @@ static int video_start_streaming(struct vb2_queue *q, unsigned int count)
>  static void video_stop_streaming(struct vb2_queue *q)
>  {
>         struct camss_video *video = vb2_get_drv_priv(q);
> -       struct video_device *vdev = &video->vdev;
> -       struct media_entity *entity;
> -       struct media_pad *pad;
> -       struct v4l2_subdev *subdev;
> -       int ret;
> -
> -       entity = &vdev->entity;
> -       while (1) {
> -               pad = &entity->pads[0];
> -               if (!(pad->flags & MEDIA_PAD_FL_SINK))
> -                       break;
> -
> -               pad = media_pad_remote_pad_first(pad);
> -               if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
> -                       break;
> -
> -               entity = pad->entity;
> -               subdev = media_entity_to_v4l2_subdev(entity);
> -
> -               ret = v4l2_subdev_call(subdev, video, s_stream, 0);
> -
> -               if (ret) {
> -                       dev_err(video->camss->dev, "Video pipeline stop failed: %d\n", ret);
> -                       return;
> -               }
> -       }
> -
> -       video_device_pipeline_stop(vdev);
>
> +       video_stop_subdevices(video, NULL);
> +       video_device_pipeline_stop(&video->vdev);
>         video->ops->flush_buffers(video, VB2_BUF_STATE_ERROR);
>  }
>
>