Hi Dan
On Fri, Jul 04, 2025 at 12:20:18PM +0100, Daniel Scally wrote:
> Add two new members to struct media_entity_operations, along with new
> functions in media-entity.c to traverse a media pipeline and call the
> new operations. The new functions are intended to be used to signal
> to a media pipeline that it has fully started, with the entity ops
> allowing drivers to define some action to be taken when those
> conditions are met.
>
> The combination of the new functions and operations allows drivers
> which are part of a multi-driver pipeline to delay actually starting
> streaming until all of the conditions for streaming succcessfully are
> met across all drivers.
>
> Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
> ---
> Changes in v4:
>
> - Reverted to having the iter variable
>
> Changes in v3:
>
> - Dropped the iter variable now that the pipeline entity
> iterator functions don't need it.
> - Updated documentation to specify Optional and return
> values
>
> Changes in v2:
>
> - Refactored media_pipeline_started() such that the cleanup
> function for media_pipeline_entity_iter is unconditionally
> called
> - Avoided using media_entity_call() helper for operation that
> has return type void to avoid compiler warnings
> ---
> drivers/media/mc/mc-entity.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> include/media/media-entity.h | 29 ++++++++++++++++++++++++++++
> 2 files changed, 75 insertions(+)
>
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index 045590905582054c46656e20463271b1f93fa6b4..d3443537d4304e12cb015630101efba22375c011 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -1053,6 +1053,52 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
> }
> EXPORT_SYMBOL_GPL(__media_pipeline_entity_iter_next);
>
> +int media_pipeline_started(struct media_pipeline *pipe)
> +{
> + struct media_pipeline_entity_iter iter;
> + 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) {
> + ret = media_entity_call(entity, pipeline_started);
> + if (ret && ret != -ENOIOCTLCMD)
> + break;
> + }
> +
> + media_pipeline_entity_iter_cleanup(&iter);
> +
> + ret = ret == -ENOIOCTLCMD ? 0 : ret;
> + if (ret)
> + media_pipeline_stopped(pipe);
If you take my suggestion to limit the return value of
video_device_pipeline_started() to three possible error codes, you
could return -EINVAL here
> +
> + return ret;
and 0 here
> +}
> +EXPORT_SYMBOL_GPL(media_pipeline_started);
> +
> +int media_pipeline_stopped(struct media_pipeline *pipe)
> +{
> + struct media_pipeline_entity_iter iter;
> + 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->ops && entity->ops->pipeline_stopped)
> + entity->ops->pipeline_stopped(entity);
> +
> + media_pipeline_entity_iter_cleanup(&iter);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(media_pipeline_stopped);
> +
> /* -----------------------------------------------------------------------------
> * Links management
> */
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 64cf590b11343f68a456c5870ca2f32917c122f9..ad658f42357ec505c84d9479bbbf18494da7f939 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -269,6 +269,10 @@ struct media_pad {
> * media_entity_has_pad_interdep().
> * Optional: If the operation isn't implemented all pads
> * will be considered as interdependent.
> + * @pipeline_started: Notify this entity that the pipeline it is a part of has
> + * been started
> + * @pipeline_stopped: Notify this entity that the pipeline it is a part of has
> + * been stopped
The documentation of the other functions end with a full stop.
If the operation is optional, I would specify it here like it's done
for other operations
> *
> * .. note::
> *
> @@ -284,6 +288,8 @@ struct media_entity_operations {
> int (*link_validate)(struct media_link *link);
> bool (*has_pad_interdep)(struct media_entity *entity, unsigned int pad0,
> unsigned int pad1);
> + int (*pipeline_started)(struct media_entity *entity);
> + void (*pipeline_stopped)(struct media_entity *entity);
> };
>
> /**
> @@ -1261,6 +1267,29 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
> entity != NULL; \
> entity = __media_pipeline_entity_iter_next((pipe), iter, entity))
>
> +/**
> + * media_pipeline_started - Inform entities in a pipeline that it has started
> + * @pipe: The pipeline
> + *
> + * Iterate on all entities in a media pipeline and call their pipeline_started
> + * member of media_entity_operations. Optional.
I would move "Optional" to the documentation of the media entity
> + *
> + * Return: zero on success, or a negative error code passed through from an
> + * entity's .pipeline_started() operation.
> + */
> +int media_pipeline_started(struct media_pipeline *pipe);
> +
> +/**
> + * media_pipeline_stopped - Inform entities in a pipeline that it has stopped
> + * @pipe: The pipeline
> + *
> + * Iterate on all entities in a media pipeline and call their pipeline_stopped
> + * member of media_entity_operations. Optional.
> + *
> + * Return: zero on success, or -ENOMEM if the iterator initialisation failed.
> + */
> +int media_pipeline_stopped(struct media_pipeline *pipe);
> +
> /**
> * media_pipeline_alloc_start - Mark a pipeline as streaming
> * @pad: Starting pad
>
> --
> 2.34.1
>
>
