Re: [PATCH v1 2/5] media: mc: entity: Add entity iterator for media_pipeline

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

On 12/12/2022 16:16, Laurent Pinchart wrote:
> Add a media_pipeline_for_each_entity() macro to iterate over entities in
> a pipeline. This should be used by driver as a replacement of the
> media_graph_walk API, as iterating over the media_pipeline uses the
> cached list of pads and is thus more efficient.
> 
> Deprecate the media_graph_walk API to indicate it shouldn't be used in
> new drivers.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   drivers/media/mc/mc-entity.c | 37 +++++++++++++++++++
>   include/media/media-entity.h | 69 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 106 insertions(+)
> 
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index 70df2050951c..f19bb98071b2 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -950,6 +950,43 @@ __media_pipeline_pad_iter_next(struct media_pipeline *pipe,
>   }
>   EXPORT_SYMBOL_GPL(__media_pipeline_pad_iter_next);
>   
> +int media_pipeline_entity_iter_init(struct media_pipeline *pipe,
> +				    struct media_pipeline_entity_iter *iter)
> +{
> +	return media_entity_enum_init(&iter->ent_enum, pipe->mdev);
> +}
> +EXPORT_SYMBOL_GPL(media_pipeline_entity_iter_init);
> +
> +void media_pipeline_entity_iter_cleanup(struct media_pipeline_entity_iter *iter)
> +{
> +	media_entity_enum_cleanup(&iter->ent_enum);
> +}
> +EXPORT_SYMBOL_GPL(media_pipeline_entity_iter_cleanup);
> +
> +struct media_entity *
> +__media_pipeline_entity_iter_next(struct media_pipeline *pipe,
> +				  struct media_pipeline_entity_iter *iter,
> +				  struct media_entity *entity)
> +{
> +	if (!entity)
> +		iter->cursor = pipe->pads.next;
> +
> +	while (iter->cursor != &pipe->pads) {
> +		struct media_pipeline_pad *ppad;
> +		struct media_entity *entity;
> +
> +		ppad = list_entry(iter->cursor, struct media_pipeline_pad, list);
> +		entity = ppad->pad->entity;
> +		iter->cursor = iter->cursor->next;
> +
> +		if (media_entity_enum_test_and_set(&iter->ent_enum, entity))
> +			return entity;

Shouldn't this be if (!media_entity...)? Doesn't 
media_entity_enum_test_and_set return true if the entity has already 
been seen, and thus we shouldn't return it?

> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(__media_pipeline_entity_iter_next);
> +
>   /* -----------------------------------------------------------------------------
>    * Links management
>    */
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index e881e483b550..1b820cb6fed1 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -139,6 +139,17 @@ struct media_pipeline_pad_iter {
>   	struct list_head *cursor;
>   };
>   
> +/**
> + * struct media_pipeline_entity_iter - Iterator for media_pipeline_for_each_entity
> + *
> + * @ent_enum: The entity enumeration tracker
> + * @cursor: The current element
> + */
> +struct media_pipeline_entity_iter {
> +	struct media_entity_enum ent_enum;
> +	struct list_head *cursor;
> +};
> +

Same question here as for the previous one: can this be in the mc-entity.c?

  Tomi