Hi Dan
On Fri, Jul 04, 2025 at 12:20:19PM +0100, Daniel Scally wrote:
> Add helpers to run the new media_pipeline_started() and
> media_pipeline_stopped() functions. The helpers iterate over the
> entities in the pipeline and count the number of video devices and
> compare that to the pipeline's start_count() before acting. This
> allows us to only run the media pipeline callbacks in the event that
> the pipeline has had video_pipeline_start() called for each video
> device.
>
> Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
>
> ---
>
> We could take this further perhaps and include the equivalent routine
> in video_device_pipeline[_alloc]_start()...if none of the entities
> involved have .pipeline_started() or .pipeline_stopped() operations it
> should be harmless, but I'm a bit reluctant to force the choice to run
> those operations on users.
I know I've kind of suggested that, but after all I don't think it's a
very good idea, having this in two steps is probably better. And I
like the fact the v4l2-dev layer operates on the video device counting
and only relies on the mc layer for the callbacks notification.
>
> Changes in v2:
>
> - Adapted now media_pipeline_for_each_entity() takes an iter
> variable
> - Fixed the Return: section of the kerneldoc comments
> ---
> drivers/media/v4l2-core/v4l2-dev.c | 57 ++++++++++++++++++++++++++++++++++++++
> include/media/v4l2-dev.h | 36 ++++++++++++++++++++++++
> 2 files changed, 93 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index c369235113d98ae26c30a1aa386e7d60d541a66e..f3309f8349664f7296a95216a40dd9d9baae8d9e 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -1200,6 +1200,63 @@ struct media_pipeline *video_device_pipeline(struct video_device *vdev)
> }
> EXPORT_SYMBOL_GPL(video_device_pipeline);
>
> +static int __video_device_pipeline_started(struct media_pipeline *pipe)
__function_name() is usually reserved for the non-locking version of
function_name().
This seems to be an helper only used internally by
video_device_pipeline_started() so I would use a different name
something like video_device_has_pipeline_started() ?
> +{
> + struct media_pipeline_entity_iter iter;
> + unsigned int n_video_devices = 0;
> + struct media_entity *entity;
> + int ret;
> +
> + ret = media_pipeline_entity_iter_init(pipe, &iter);
> + if (ret)
> + return ret;
> +
> + media_pipeline_for_each_entity(pipe, &iter, entity) {
> + if (entity->obj_type == MEDIA_ENTITY_TYPE_VIDEO_DEVICE)
> + n_video_devices++;
> + }
> +
> + media_pipeline_entity_iter_cleanup(&iter);
> +
> + return n_video_devices - pipe->start_count;
> +}
> +
> +int video_device_pipeline_started(struct video_device *vdev)
> +{
> + struct media_pipeline *pipe;
> + int ret;
> +
> + pipe = video_device_pipeline(vdev);
> + if (!pipe)
> + return -ENODEV;
> +
> + ret = __video_device_pipeline_started(pipe);
> + if (ret)
> + return ret;
I would not return ret, as it might take random values betwen
n_video_devices and 1. See below on the return value documentation
> +
> + return media_pipeline_started(pipe);
> +}
> +EXPORT_SYMBOL_GPL(video_device_pipeline_started);
> +
> +int video_device_pipeline_stopped(struct video_device *vdev)
> +{
> + struct media_pipeline *pipe;
> + int ret;
> +
> + pipe = video_device_pipeline(vdev);
> + if (!pipe)
> + return -ENODEV;
> +
> + ret = __video_device_pipeline_started(pipe);
> + if (ret)
> + return ret;
ditto
> +
> + media_pipeline_stopped(pipe);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(video_device_pipeline_stopped);
> +
> #endif /* CONFIG_MEDIA_CONTROLLER */
>
> /*
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index 1b6222fab24eda96cbe459b435431c01f7259366..26b4a491024701ef47320aec6a1a680149ba4fc3 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -654,6 +654,42 @@ __must_check int video_device_pipeline_alloc_start(struct video_device *vdev);
> */
> struct media_pipeline *video_device_pipeline(struct video_device *vdev);
>
> +/**
> + * video_device_pipeline_started - Run the pipeline_started() entity operation
> + * for a fully-started media pipeline
> + * @vdev: A video device that's part of the pipeline
> + *
> + * This function checks whether all MEDIA_ENTITY_TYPE_VIDEO_DEVICE entities
> + * connected to a given video device through enabled links have been marked as
I would use the same text as the one from video_device_pipeline_start()
" connected to a given video device through enabled links, either
directly or indirectly,"
> + * streaming through the use of video_device_pipeline_start() or one of its
> + * equivalent functions. If so, media_pipeline_started() is called to inform
> + * entities in the pipeline of that fact. The intention is to provide drivers
> + * with a shortcut for checking whether their pipeline is fully ready to start
> + * processing data.
Not really a shortcut, I would use "mechanism" instead.
I would also specify that:
* entities in the pipeline of that fact. The intention is to provide drivers
* with a mechanism for checking whether their pipeline is fully ready to start
* processing data and call the .pipeline_started() media entity operation
* on all the entities in the pipeline.
> + *
> + * Return: The number of video devices in the pipeline remaining to be started,
> + * or a negative error number on failure.
0 for success as well
I would anyway return 0 for success and a specific error code for the
three failure cases:
-ENOMEM if allocating the iterator fails
-ENODEV if not all video devices have started
-EINVAL if media_pipeline_started() fails
You can document them as (copying from iommu.h)
* Return:
* * 0 - success
* * EINVAL - call to pipeline_started() failed
* * ENOMEM - failed to allocate pipe iterator
* * ENODEV - pipeline not yet fully started
> + */
> +int video_device_pipeline_started(struct video_device *vdev);
> +
> +/**
> + * video_device_pipeline_stopped - Run the pipeline_stopped() entity operation
> + * for a fully-started media pipeline
> + * @vdev: A video device that's part of the pipeline
> + *
> + * This function checks whether all MEDIA_ENTITY_TYPE_VIDEO_DEVICE entities
> + * connected to a given video device through enabled links have been marked as
> + * streaming through the use of video_device_pipeline_start() or one of its
What is the intended semantic here ? The first video device to receive
a streamoff() will trigger media_pipeline_stopped() or should the last
one do that ?
> + * equivalent functions. If so, media_pipeline_stopped() is called for each
> + * entity in the pipeline. The intention is to provide drivers with a shortcut
> + * for checking whether this video device is the first device in the pipeline
> + * to be stopped.
> + *
> + * Return: The number of video devices in the pipeline remaining to be started, or a
> + * negative error number on failure.
> + */
> +int video_device_pipeline_stopped(struct video_device *vdev);
> +
> #endif /* CONFIG_MEDIA_CONTROLLER */
>
> #endif /* _V4L2_DEV_H */
>
> --
> 2.34.1
>
>
