diff mbox series

[2/2] media: media-entity: Simplify media_pipeline_start()

Message ID 20220113150042.15630-3-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
State New
Delegated to: Kieran Bingham
Headers show
Series media: mc: Simplify media pipeline start/stop | expand

Commit Message

Laurent Pinchart Jan. 13, 2022, 3 p.m. UTC
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

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@ideasonboard.com>
---
 drivers/media/mc/mc-entity.c | 52 +++++++++++++++---------------------
 include/media/media-entity.h | 11 +++-----
 2 files changed, 26 insertions(+), 37 deletions(-)

Comments

Sakari Ailus Jan. 15, 2022, 5:39 p.m. UTC | #1
Moi,

Thanks for the set.

On Thu, Jan 13, 2022 at 05:00:42PM +0200, Laurent Pinchart wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> 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@ideasonboard.com>
> ---
>  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.


>  }
>  
>  /**
Laurent Pinchart Jan. 15, 2022, 8:02 p.m. UTC | #2
Hi Sakari,

On Sat, Jan 15, 2022 at 07:39:33PM +0200, Sakari Ailus wrote:
> Moi,
> 
> Thanks for the set.

Ole hyvä.

> On Thu, Jan 13, 2022 at 05:00:42PM +0200, Laurent Pinchart wrote:
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > 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@ideasonboard.com>
> > ---
> >  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 usually use "if (pointer)" or "if (!pointer)" without comparing to
NULL, but for some reason, when returning a bool, it feels more explicit
to me to use a comparison. I'm not sure why, maybe because, unlike with
if (), the implicit cast is only apparent when you read the signature of
the function ? Not that it's far away in this case, it's only two lines
up.

> I'll do that when applying if that's fine.

Fine with me, I don't mind either way. Thanks.

> >  }
> >  
> >  /**
Laurent Pinchart Feb. 11, 2022, 9:27 p.m. UTC | #3
Hi Sakari,

On Sat, Jan 15, 2022 at 10:02:21PM +0200, Laurent Pinchart wrote:
> On Sat, Jan 15, 2022 at 07:39:33PM +0200, Sakari Ailus wrote:
> > On Thu, Jan 13, 2022 at 05:00:42PM +0200, Laurent Pinchart wrote:
> > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > 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@ideasonboard.com>
> > > ---
> > >  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 usually use "if (pointer)" or "if (!pointer)" without comparing to
> NULL, but for some reason, when returning a bool, it feels more explicit
> to me to use a comparison. I'm not sure why, maybe because, unlike with
> if (), the implicit cast is only apparent when you read the signature of
> the function ? Not that it's far away in this case, it's only two lines
> up.
> 
> > I'll do that when applying if that's fine.
> 
> Fine with me, I don't mind either way. Thanks.

Will you take this series for v5.18 ?

> > >  }
> > >  
> > >  /**
Sakari Ailus Feb. 15, 2022, 9:28 p.m. UTC | #4
On Fri, Feb 11, 2022 at 11:27:22PM +0200, Laurent Pinchart wrote:
> > > I'll do that when applying if that's fine.
> > 
> > Fine with me, I don't mind either way. Thanks.
> 
> Will you take this series for v5.18 ?

Yes.
diff mbox series

Patch

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;
 }
 
 /**