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@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.  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@xxxxxxxxxxxxxxxx>
> ---
>  .../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);
>  }
>
>