Re: [PATCH v2 2/2] media: camss: video: factor pipeline stream on/off into a helper

[Loic Poulain]

On Sat, Oct 31, 2026 at 5:11 AM Qianhui Xu <xiaqinghuan@xxxxxxxxxxxxxxxx> 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.  video_stop_streaming() returned early on the first subdevice that
> failed to stop, leaving the rest running while the pipeline was torn down.
>
> Factor the walk into camss_video_subdevs_set_stream(video, bool), used by
> both start and stop.  A start error now unwinds the partial start by
> disabling the pipeline again; streams already in the requested state report
> -EALREADY, so the subdevice that failed to start (never marked enabled) and
> the ones above it (never started) are left untouched.
>
> Locate each subdevice source pad with
> media_entity_remote_source_pad_unique() instead of assuming pads[0], and
> toggle it with v4l2_subdev_enable_streams() and
> v4l2_subdev_disable_streams() instead of the legacy .s_stream() call; the
> camss subdevices only implement .s_stream() so the helpers fall back to it.

So the patch looks better now. However, after looking at
media_entity_remote_source_pad_unique(), it appears that it returns an
error when more than one source link is enabled.

This works with the current driver design, but Bryan may want to take
this into account for his pixel-path support series, as the VFE
subdevice could eventually have multiple active source links (for
example, from both CSID and Parameters).

So the change looks good to me, but I'll leave it to Bryan to decide
whether we should merge it as-is or address that future use case now.

Regards,
Loic

>
> Suggested-by: Loic Poulain <loic.poulain@xxxxxxxxxxxxxxxx>
> Signed-off-by: Qianhui Xu <xiaqinghuan@xxxxxxxxxxxxxxxx>
> ---
>  .../media/platform/qcom/camss/camss-video.c   | 109 +++++++++++-------
>  1 file changed, 65 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
> index 0852eb6f13..9a633ca066 100644
> --- a/drivers/media/platform/qcom/camss/camss-video.c
> +++ b/drivers/media/platform/qcom/camss/camss-video.c
> @@ -249,13 +249,66 @@ static int video_prepare_streaming(struct vb2_queue *q)
>         return ret;
>  }
>
> +/*
> + * camss_video_subdevs_set_stream() - Enable or disable a video pipeline
> + * @video: The video node whose pipeline is toggled
> + * @enable: true to start the subdevices, false to stop them
> + *
> + * Walk the media pipeline upstream from the video node and toggle the stream on
> + * each subdevice.  Each subdevice source pad is located with
> + * media_entity_remote_source_pad_unique() instead of assuming pads[0], and the
> + * stream is toggled with v4l2_subdev_enable_streams()/
> + * v4l2_subdev_disable_streams() rather than the legacy .s_stream() call.  The
> + * camss subdevices only implement .s_stream(), so the helpers fall back to it.
> + *
> + * A stream that is already in the requested state reports -EALREADY.  On the
> + * teardown path this simply skips subdevices that were never started, so a
> + * partially started pipeline is unwound by disabling it again: the subdevice
> + * that failed to start was not marked enabled and is left untouched.  A
> + * subdevice without a streaming operation reports -ENOIOCTLCMD, which is
> + * ignored here to match the previous .s_stream() call sites.
> + *
> + * Return: 0 on success, or the error code of the first subdevice that failed.
> + */
> +static int camss_video_subdevs_set_stream(struct camss_video *video,
> +                                         bool enable)
> +{
> +       struct media_entity *entity = &video->vdev.entity;
> +
> +       while (1) {
> +               struct media_pad *pad;
> +               struct v4l2_subdev *subdev;
> +               int ret;
> +
> +               pad = media_entity_remote_source_pad_unique(entity);
> +               if (IS_ERR(pad))
> +                       break;
> +
> +               if (!is_media_entity_v4l2_subdev(pad->entity))
> +                       break;
> +
> +               subdev = media_entity_to_v4l2_subdev(pad->entity);
> +
> +               if (enable)
> +                       ret = v4l2_subdev_enable_streams(subdev, pad->index,
> +                                                        BIT_ULL(0));
> +               else
> +                       ret = v4l2_subdev_disable_streams(subdev, pad->index,
> +                                                         BIT_ULL(0));
> +
> +               if (ret && ret != -EALREADY && ret != -ENOIOCTLCMD)
> +                       return ret;
> +
> +               entity = pad->entity;
> +       }
> +
> +       return 0;
> +}
> +
>  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_pad *pad;
> -       struct v4l2_subdev *subdev;
>         int ret;
>
>         ret = video_device_pipeline_alloc_start(vdev);
> @@ -268,22 +321,11 @@ 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))
> -                       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, 1);
> -               if (ret < 0 && ret != -ENOIOCTLCMD)
> -                       goto error;
> +       ret = camss_video_subdevs_set_stream(video, true);
> +       if (ret < 0) {
> +               camss_video_subdevs_set_stream(video, false);
> +               dev_err(video->camss->dev, "Video pipeline start failed: %d\n", ret);
> +               goto error;
>         }
>
>         return 0;
> @@ -300,34 +342,13 @@ 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;
> -               }
> -       }
> +       ret = camss_video_subdevs_set_stream(video, false);
> +       if (ret < 0)
> +               dev_err(video->camss->dev, "Video pipeline stop failed: %d\n", ret);
>
> -       video_device_pipeline_stop(vdev);
> +       video_device_pipeline_stop(&video->vdev);
>
>         video->ops->flush_buffers(video, VB2_BUF_STATE_ERROR);
>  }
> --
> 2.53.0
>