Moi,
Thanks for the set.
On Thu, Jan 13, 2022 at 05:00:42PM +0200, Laurent Pinchart wrote:
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> The media_pipeline_start() function has two purposes: it constructs a
> pipeline by recording the entities that are part of it, gathered from a
> graph walk, and validate the media links. The pipeline pointer is stored
> in the media_entity structure as part of this process, and the entity's
> stream count is increased, to record that the entity is streaming.
>
> When multiple video nodes are present in a pipeline,
> media_pipeline_start() is typically called on all of them, with the same
> pipeline pointer. This is taken into account in media_pipeline_start()
> by skipping validation for entities that are already part of the
> pipeline, while returning an error if an entity is part of a different
> pipeline.
>
> It turns out that this process is overly complicated. When
> media_pipeline_start() is called for the first time, it constructs the
> full pipeline, adding all entities and validating all the links.
> Subsequent calls to media_pipeline_start() are then nearly no-ops, they
> only increase the stream count on the pipeline and on all entities.
>
> The media_entity stream_count field is used for two purposes: checking
> if the entity is streaming, and detecting when a call to
> media_pipeline_stop() balances needs to reset the entity pipe pointer to
> NULL. The former can easily be replaced by a check of the pipe pointer.
>
> Simplify media_pipeline_start() by avoiding the pipeline walk on all
> calls but the first one, and drop the media_entity stream_count field.
> media_pipeline_stop() is updated accordingly.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/mc/mc-entity.c | 52 +++++++++++++++---------------------
> include/media/media-entity.h | 11 +++-----
> 2 files changed, 26 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index f83e043f0f3b..8ab0913d8d82 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -396,20 +396,21 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
> struct media_link *link;
> int ret;
>
> - if (!pipe->streaming_count++) {
> - ret = media_graph_walk_init(&pipe->graph, mdev);
> - if (ret)
> - goto error_graph_walk_start;
> + if (pipe->streaming_count) {
> + pipe->streaming_count++;
> + return 0;
> }
>
> + ret = media_graph_walk_init(&pipe->graph, mdev);
> + if (ret)
> + return ret;
> +
> media_graph_walk_start(&pipe->graph, entity);
>
> while ((entity = media_graph_walk_next(graph))) {
> DECLARE_BITMAP(active, MEDIA_ENTITY_MAX_PADS);
> DECLARE_BITMAP(has_no_links, MEDIA_ENTITY_MAX_PADS);
>
> - entity->stream_count++;
> -
> if (entity->pipe && entity->pipe != pipe) {
> pr_err("Pipe active for %s. Can't start for %s\n",
> entity->name,
> @@ -418,12 +419,12 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
> goto error;
> }
>
> - entity->pipe = pipe;
> -
> /* Already streaming --- no need to check. */
> - if (entity->stream_count > 1)
> + if (entity->pipe)
> continue;
>
> + entity->pipe = pipe;
> +
> if (!entity->ops || !entity->ops->link_validate)
> continue;
>
> @@ -479,6 +480,8 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
> }
> }
>
> + pipe->streaming_count++;
> +
> return 0;
>
> error:
> @@ -489,24 +492,17 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
> media_graph_walk_start(graph, entity_err);
>
> while ((entity_err = media_graph_walk_next(graph))) {
> - /* Sanity check for negative stream_count */
> - if (!WARN_ON_ONCE(entity_err->stream_count <= 0)) {
> - entity_err->stream_count--;
> - if (entity_err->stream_count == 0)
> - entity_err->pipe = NULL;
> - }
> + entity_err->pipe = NULL;
>
> /*
> - * We haven't increased stream_count further than this
> - * so we quit here.
> + * We haven't started entities further than this so we quit
> + * here.
> */
> if (entity_err == entity)
> break;
> }
>
> -error_graph_walk_start:
> - if (!--pipe->streaming_count)
> - media_graph_walk_cleanup(graph);
> + media_graph_walk_cleanup(graph);
>
> return ret;
> }
> @@ -537,19 +533,15 @@ void __media_pipeline_stop(struct media_entity *entity)
> if (WARN_ON(!pipe))
> return;
>
> + if (--pipe->streaming_count)
> + return;
> +
> media_graph_walk_start(graph, entity);
>
> - while ((entity = media_graph_walk_next(graph))) {
> - /* Sanity check for negative stream_count */
> - if (!WARN_ON_ONCE(entity->stream_count <= 0)) {
> - entity->stream_count--;
> - if (entity->stream_count == 0)
> - entity->pipe = NULL;
> - }
> - }
> + while ((entity = media_graph_walk_next(graph)))
> + entity->pipe = NULL;
>
> - if (!--pipe->streaming_count)
> - media_graph_walk_cleanup(graph);
> + media_graph_walk_cleanup(graph);
>
> }
> EXPORT_SYMBOL_GPL(__media_pipeline_stop);
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 8546f13c42a9..e3c4fd1e3623 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -268,7 +268,6 @@ enum media_entity_type {
> * @pads: Pads array with the size defined by @num_pads.
> * @links: List of data links.
> * @ops: Entity operations.
> - * @stream_count: Stream count for the entity.
> * @use_count: Use count for the entity.
> * @pipe: Pipeline this entity belongs to.
> * @info: Union with devnode information. Kept just for backward
> @@ -283,10 +282,9 @@ enum media_entity_type {
> *
> * .. note::
> *
> - * @stream_count and @use_count reference counts must never be
> - * negative, but are signed integers on purpose: a simple ``WARN_ON(<0)``
> - * check can be used to detect reference count bugs that would make them
> - * negative.
> + * The @use_count reference count must never be negative, but is a signed
> + * integer on purpose: a simple ``WARN_ON(<0)`` check can be used to detect
> + * reference count bugs that would make it negative.
> */
> struct media_entity {
> struct media_gobj graph_obj; /* must be first field in struct */
> @@ -305,7 +303,6 @@ struct media_entity {
>
> const struct media_entity_operations *ops;
>
> - int stream_count;
> int use_count;
>
> struct media_pipeline *pipe;
> @@ -867,7 +864,7 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad);
> */
> static inline bool media_entity_is_streaming(const struct media_entity *entity)
> {
> - return entity->stream_count > 0;
> + return entity->pipe != NULL;
I'd drop "!= NULL" part; it's redundant.
I'll do that when applying if that's fine.
> }
>
> /**
--
Kind regards,
Sakari Ailus
