[v5,12/24] media: entity: Add an iterator helper for connected pads

Commit Message

Tomi Valkeinen April 15, 2021, 1:04 p.m. UTC

From: Sakari Ailus <[e-mail]>

Add a helper macro for iterating over pads that are connected through
enabled routes. This can be used to find all the connected pads within an
entity, for instance starting from the pad which has been obtained during
the graph walk.

Signed-off-by: Sakari Ailus <[e-mail]>
Reviewed-by: Niklas Söderlund <[e-mail]>

- Make __media_entity_next_routed_pad() return NULL and adjust the
  iterator to handle that
Signed-off-by: Jacopo Mondi <[e-mail]>
---
 include/media/media-entity.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Laurent Pinchart April 18, 2021, 6:20 p.m. UTC | #1

Hi Tomi and Sakari,

Thank you for the patch.

On Thu, Apr 15, 2021 at 04:04:38PM +0300, Tomi Valkeinen wrote:
> From: Sakari Ailus <[e-mail]>
> 
> Add a helper macro for iterating over pads that are connected through
> enabled routes. This can be used to find all the connected pads within an
> entity, for instance starting from the pad which has been obtained during
> the graph walk.
> 
> Signed-off-by: Sakari Ailus <[e-mail]>
> Reviewed-by: Niklas Söderlund <[e-mail]>
> 
> - Make __media_entity_next_routed_pad() return NULL and adjust the
>   iterator to handle that
> Signed-off-by: Jacopo Mondi <[e-mail]>
> ---
>  include/media/media-entity.h | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 73de1c335e4e..edd6f60ed6b4 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -916,6 +916,33 @@ __must_check int media_graph_walk_init(
>  bool media_entity_has_route(struct media_entity *entity, unsigned int pad0,
>  			    unsigned int pad1);
>  
> +static inline struct media_pad *__media_entity_next_routed_pad(
> +	struct media_pad *start, struct media_pad *iter)
> +{
> +	struct media_entity *entity = start->entity;
> +
> +	for (; iter < &entity->pads[entity->num_pads]; iter++)
> +		if (media_entity_has_route(entity, start->index, iter->index))
> +			return iter;

I'd use curly braces.

> +
> +	return NULL;
> +}

Does this need to be inlined ?

> +
> +/**
> + * media_entity_for_each_routed_pad - Iterate over entity pads connected by routes

"routed" sounds a bit weird. Would media_entity_for_each_connected_pad()
be a better name ?

> + *
> + * @start: The stating pad

s/stating/starting/

> + * @iter: The iterator pad
> + *
> + * Iterate over all pads connected through routes from a given pad

"from the @start pad"

> + * within an entity. The iteration will include the starting pad itself.

s/starting/@start/

I wonder if it wouldn't be more logical to not include the start pad.
That wouldn't match the current usage patterns, which would need to be
adapted accordingly, but I'm worried that including the start pad will
lead to annoying bugs in the future. Maybe I worry too much.

And now that I reread the patch, I also wonder if "start" is a good
name, as it implies we start the enumeration from a given pad, while we
enumerate all pads connected to a given pad. I'm not sure what a better
name would be though, maybe just pad ?

Reviewed-by: Laurent Pinchart <[e-mail]>

> + */
> +#define media_entity_for_each_routed_pad(start, iter)			\
> +	for (iter = __media_entity_next_routed_pad(			\
> +		     start, (start)->entity->pads);			\
> +	     iter != NULL;						\
> +	     iter = __media_entity_next_routed_pad(start, iter + 1))
> +
>  /**
>   * media_graph_walk_cleanup - Release resources used by graph walk.
>   *

Sakari Ailus April 20, 2021, 11:48 a.m. UTC | #2

Hi Laurent,

Thanks for the review.

On Sun, Apr 18, 2021 at 09:20:03PM +0300, Laurent Pinchart wrote:
> Hi Tomi and Sakari,
> 
> Thank you for the patch.
> 
> On Thu, Apr 15, 2021 at 04:04:38PM +0300, Tomi Valkeinen wrote:
> > From: Sakari Ailus <[e-mail]>
> > 
> > Add a helper macro for iterating over pads that are connected through
> > enabled routes. This can be used to find all the connected pads within an
> > entity, for instance starting from the pad which has been obtained during
> > the graph walk.
> > 
> > Signed-off-by: Sakari Ailus <[e-mail]>
> > Reviewed-by: Niklas Söderlund <[e-mail]>
> > 
> > - Make __media_entity_next_routed_pad() return NULL and adjust the
> >   iterator to handle that
> > Signed-off-by: Jacopo Mondi <[e-mail]>
> > ---
> >  include/media/media-entity.h | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index 73de1c335e4e..edd6f60ed6b4 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -916,6 +916,33 @@ __must_check int media_graph_walk_init(
> >  bool media_entity_has_route(struct media_entity *entity, unsigned int pad0,
> >  			    unsigned int pad1);
> >  
> > +static inline struct media_pad *__media_entity_next_routed_pad(
> > +	struct media_pad *start, struct media_pad *iter)
> > +{
> > +	struct media_entity *entity = start->entity;
> > +
> > +	for (; iter < &entity->pads[entity->num_pads]; iter++)
> > +		if (media_entity_has_route(entity, start->index, iter->index))
> > +			return iter;
> 
> I'd use curly braces.
> 
> > +
> > +	return NULL;
> > +}
> 
> Does this need to be inlined ?

I guess it doesn't have to. It's used inside loops and it's rather small so
I think it should be fine that way.

> 
> > +
> > +/**
> > + * media_entity_for_each_routed_pad - Iterate over entity pads connected by routes
> 
> "routed" sounds a bit weird. Would media_entity_for_each_connected_pad()
> be a better name ?

"Connected" is often used in context of links. We're dealing with routes
here, so I thought "routed" is appropriate to avoid confusion.

> 
> > + *
> > + * @start: The stating pad
> 
> s/stating/starting/
> 
> > + * @iter: The iterator pad
> > + *
> > + * Iterate over all pads connected through routes from a given pad
> 
> "from the @start pad"
> 
> > + * within an entity. The iteration will include the starting pad itself.
> 
> s/starting/@start/
> 
> I wonder if it wouldn't be more logical to not include the start pad.
> That wouldn't match the current usage patterns, which would need to be
> adapted accordingly, but I'm worried that including the start pad will
> lead to annoying bugs in the future. Maybe I worry too much.

The aim here is to find all pads that are routed to another pad within the
same entity. If you remove the start pad, it becomes a task harder than
difficult.

> 
> And now that I reread the patch, I also wonder if "start" is a good
> name, as it implies we start the enumeration from a given pad, while we
> enumerate all pads connected to a given pad. I'm not sure what a better
> name would be though, maybe just pad ?

There are two pads here. Therefore explicitly calling them something else
makes sense IMO.

> 
> Reviewed-by: Laurent Pinchart <[e-mail]>

I agree on the comments I didn't reply to.

Thank you!

> 
> > + */
> > +#define media_entity_for_each_routed_pad(start, iter)			\
> > +	for (iter = __media_entity_next_routed_pad(			\
> > +		     start, (start)->entity->pads);			\
> > +	     iter != NULL;						\
> > +	     iter = __media_entity_next_routed_pad(start, iter + 1))
> > +
> >  /**
> >   * media_graph_walk_cleanup - Release resources used by graph walk.
> >   *
>

Laurent Pinchart April 29, 2021, 1:33 a.m. UTC | #3

Hi Sakari,

On Tue, Apr 20, 2021 at 02:48:25PM +0300, Sakari Ailus wrote:
> On Sun, Apr 18, 2021 at 09:20:03PM +0300, Laurent Pinchart wrote:
> > On Thu, Apr 15, 2021 at 04:04:38PM +0300, Tomi Valkeinen wrote:
> > > From: Sakari Ailus <[e-mail]>
> > > 
> > > Add a helper macro for iterating over pads that are connected through
> > > enabled routes. This can be used to find all the connected pads within an
> > > entity, for instance starting from the pad which has been obtained during
> > > the graph walk.
> > > 
> > > Signed-off-by: Sakari Ailus <[e-mail]>
> > > Reviewed-by: Niklas Söderlund <[e-mail]>
> > > 
> > > - Make __media_entity_next_routed_pad() return NULL and adjust the
> > >   iterator to handle that
> > > Signed-off-by: Jacopo Mondi <[e-mail]>
> > > ---
> > >  include/media/media-entity.h | 27 +++++++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > > 
> > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > > index 73de1c335e4e..edd6f60ed6b4 100644
> > > --- a/include/media/media-entity.h
> > > +++ b/include/media/media-entity.h
> > > @@ -916,6 +916,33 @@ __must_check int media_graph_walk_init(
> > >  bool media_entity_has_route(struct media_entity *entity, unsigned int pad0,
> > >  			    unsigned int pad1);
> > >  
> > > +static inline struct media_pad *__media_entity_next_routed_pad(
> > > +	struct media_pad *start, struct media_pad *iter)
> > > +{
> > > +	struct media_entity *entity = start->entity;
> > > +
> > > +	for (; iter < &entity->pads[entity->num_pads]; iter++)
> > > +		if (media_entity_has_route(entity, start->index, iter->index))
> > > +			return iter;
> > 
> > I'd use curly braces.
> > 
> > > +
> > > +	return NULL;
> > > +}
> > 
> > Does this need to be inlined ?
> 
> I guess it doesn't have to. It's used inside loops and it's rather small so
> I think it should be fine that way.

It may not be that small. I'd rather let the compiler decide whether to
inline it or not.

> > > +
> > > +/**
> > > + * media_entity_for_each_routed_pad - Iterate over entity pads connected by routes
> > 
> > "routed" sounds a bit weird. Would media_entity_for_each_connected_pad()
> > be a better name ?
> 
> "Connected" is often used in context of links. We're dealing with routes
> here, so I thought "routed" is appropriate to avoid confusion.

I understand the confusion, maybe we can find a better term that would
be different than "connected". "routed" really sounds weird in this
context.

> > > + *
> > > + * @start: The stating pad
> > 
> > s/stating/starting/
> > 
> > > + * @iter: The iterator pad
> > > + *
> > > + * Iterate over all pads connected through routes from a given pad
> > 
> > "from the @start pad"
> > 
> > > + * within an entity. The iteration will include the starting pad itself.
> > 
> > s/starting/@start/
> > 
> > I wonder if it wouldn't be more logical to not include the start pad.
> > That wouldn't match the current usage patterns, which would need to be
> > adapted accordingly, but I'm worried that including the start pad will
> > lead to annoying bugs in the future. Maybe I worry too much.
> 
> The aim here is to find all pads that are routed to another pad within the
> same entity. If you remove the start pad, it becomes a task harder than
> difficult.

Intuitively, "all pads that are routed to another pad" doesn't include
the "another pad". I'm not opposed to including the start pad as that's
what the current usage patterns need, but we should then rename the
macro accordingly as its current name is counter-intuitive.

> > And now that I reread the patch, I also wonder if "start" is a good
> > name, as it implies we start the enumeration from a given pad, while we
> > enumerate all pads connected to a given pad. I'm not sure what a better
> > name would be though, maybe just pad ?
> 
> There are two pads here. Therefore explicitly calling them something else
> makes sense IMO.

Makes sense, but "start" isn't a good name as we're not starting
anything.

> > Reviewed-by: Laurent Pinchart <[e-mail]>
> 
> I agree on the comments I didn't reply to.
> 
> Thank you!
> 
> > > + */
> > > +#define media_entity_for_each_routed_pad(start, iter)			\
> > > +	for (iter = __media_entity_next_routed_pad(			\
> > > +		     start, (start)->entity->pads);			\
> > > +	     iter != NULL;						\
> > > +	     iter = __media_entity_next_routed_pad(start, iter + 1))
> > > +
> > >  /**
> > >   * media_graph_walk_cleanup - Release resources used by graph walk.
> > >   *

Sakari Ailus April 29, 2021, 11:56 a.m. UTC | #4

Hi Laurent,

On Thu, Apr 29, 2021 at 04:33:48AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tue, Apr 20, 2021 at 02:48:25PM +0300, Sakari Ailus wrote:
> > On Sun, Apr 18, 2021 at 09:20:03PM +0300, Laurent Pinchart wrote:
> > > On Thu, Apr 15, 2021 at 04:04:38PM +0300, Tomi Valkeinen wrote:
> > > > From: Sakari Ailus <[e-mail]>
> > > > 
> > > > Add a helper macro for iterating over pads that are connected through
> > > > enabled routes. This can be used to find all the connected pads within an
> > > > entity, for instance starting from the pad which has been obtained during
> > > > the graph walk.
> > > > 
> > > > Signed-off-by: Sakari Ailus <[e-mail]>
> > > > Reviewed-by: Niklas Söderlund <[e-mail]>
> > > > 
> > > > - Make __media_entity_next_routed_pad() return NULL and adjust the
> > > >   iterator to handle that
> > > > Signed-off-by: Jacopo Mondi <[e-mail]>
> > > > ---
> > > >  include/media/media-entity.h | 27 +++++++++++++++++++++++++++
> > > >  1 file changed, 27 insertions(+)
> > > > 
> > > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > > > index 73de1c335e4e..edd6f60ed6b4 100644
> > > > --- a/include/media/media-entity.h
> > > > +++ b/include/media/media-entity.h
> > > > @@ -916,6 +916,33 @@ __must_check int media_graph_walk_init(
> > > >  bool media_entity_has_route(struct media_entity *entity, unsigned int pad0,
> > > >  			    unsigned int pad1);
> > > >  
> > > > +static inline struct media_pad *__media_entity_next_routed_pad(
> > > > +	struct media_pad *start, struct media_pad *iter)
> > > > +{
> > > > +	struct media_entity *entity = start->entity;
> > > > +
> > > > +	for (; iter < &entity->pads[entity->num_pads]; iter++)
> > > > +		if (media_entity_has_route(entity, start->index, iter->index))
> > > > +			return iter;
> > > 
> > > I'd use curly braces.
> > > 
> > > > +
> > > > +	return NULL;
> > > > +}
> > > 
> > > Does this need to be inlined ?
> > 
> > I guess it doesn't have to. It's used inside loops and it's rather small so
> > I think it should be fine that way.
> 
> It may not be that small. I'd rather let the compiler decide whether to
> inline it or not.

Works for me.

> 
> > > > +
> > > > +/**
> > > > + * media_entity_for_each_routed_pad - Iterate over entity pads connected by routes
> > > 
> > > "routed" sounds a bit weird. Would media_entity_for_each_connected_pad()
> > > be a better name ?
> > 
> > "Connected" is often used in context of links. We're dealing with routes
> > here, so I thought "routed" is appropriate to avoid confusion.
> 
> I understand the confusion, maybe we can find a better term that would
> be different than "connected". "routed" really sounds weird in this
> context.

I'm fine with connected.

> 
> > > > + *
> > > > + * @start: The stating pad
> > > 
> > > s/stating/starting/
> > > 
> > > > + * @iter: The iterator pad
> > > > + *
> > > > + * Iterate over all pads connected through routes from a given pad
> > > 
> > > "from the @start pad"
> > > 
> > > > + * within an entity. The iteration will include the starting pad itself.
> > > 
> > > s/starting/@start/
> > > 
> > > I wonder if it wouldn't be more logical to not include the start pad.
> > > That wouldn't match the current usage patterns, which would need to be
> > > adapted accordingly, but I'm worried that including the start pad will
> > > lead to annoying bugs in the future. Maybe I worry too much.
> > 
> > The aim here is to find all pads that are routed to another pad within the
> > same entity. If you remove the start pad, it becomes a task harder than
> > difficult.
> 
> Intuitively, "all pads that are routed to another pad" doesn't include
> the "another pad". I'm not opposed to including the start pad as that's
> what the current usage patterns need, but we should then rename the
> macro accordingly as its current name is counter-intuitive.

I'm certainly not opposed to that. But it shouldn't be too much longer than
what's already there.

> 
> > > And now that I reread the patch, I also wonder if "start" is a good
> > > name, as it implies we start the enumeration from a given pad, while we
> > > enumerate all pads connected to a given pad. I'm not sure what a better
> > > name would be though, maybe just pad ?
> > 
> > There are two pads here. Therefore explicitly calling them something else
> > makes sense IMO.
> 
> Makes sense, but "start" isn't a good name as we're not starting
> anything.

"start" is not a verb here. It's where the iteration *starts*.

> 
> > > Reviewed-by: Laurent Pinchart <[e-mail]>
> > 
> > I agree on the comments I didn't reply to.
> > 
> > Thank you!
> > 
> > > > + */
> > > > +#define media_entity_for_each_routed_pad(start, iter)			\
> > > > +	for (iter = __media_entity_next_routed_pad(			\
> > > > +		     start, (start)->entity->pads);			\
> > > > +	     iter != NULL;						\
> > > > +	     iter = __media_entity_next_routed_pad(start, iter + 1))
> > > > +
> > > >  /**
> > > >   * media_graph_walk_cleanup - Release resources used by graph walk.
> > > >   *
>

Tomi Valkeinen April 29, 2021, 12:04 p.m. UTC | #5

On 29/04/2021 14:56, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Thu, Apr 29, 2021 at 04:33:48AM +0300, Laurent Pinchart wrote:
>> Hi Sakari,
>>
>> On Tue, Apr 20, 2021 at 02:48:25PM +0300, Sakari Ailus wrote:
>>> On Sun, Apr 18, 2021 at 09:20:03PM +0300, Laurent Pinchart wrote:
>>>> On Thu, Apr 15, 2021 at 04:04:38PM +0300, Tomi Valkeinen wrote:
>>>>> From: Sakari Ailus <[e-mail]>
>>>>>
>>>>> Add a helper macro for iterating over pads that are connected through
>>>>> enabled routes. This can be used to find all the connected pads within an
>>>>> entity, for instance starting from the pad which has been obtained during
>>>>> the graph walk.
>>>>>
>>>>> Signed-off-by: Sakari Ailus <[e-mail]>
>>>>> Reviewed-by: Niklas Söderlund <[e-mail]>
>>>>>
>>>>> - Make __media_entity_next_routed_pad() return NULL and adjust the
>>>>>    iterator to handle that
>>>>> Signed-off-by: Jacopo Mondi <[e-mail]>
>>>>> ---
>>>>>   include/media/media-entity.h | 27 +++++++++++++++++++++++++++
>>>>>   1 file changed, 27 insertions(+)
>>>>>
>>>>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>>>>> index 73de1c335e4e..edd6f60ed6b4 100644
>>>>> --- a/include/media/media-entity.h
>>>>> +++ b/include/media/media-entity.h
>>>>> @@ -916,6 +916,33 @@ __must_check int media_graph_walk_init(
>>>>>   bool media_entity_has_route(struct media_entity *entity, unsigned int pad0,
>>>>>   			    unsigned int pad1);
>>>>>   
>>>>> +static inline struct media_pad *__media_entity_next_routed_pad(
>>>>> +	struct media_pad *start, struct media_pad *iter)
>>>>> +{
>>>>> +	struct media_entity *entity = start->entity;
>>>>> +
>>>>> +	for (; iter < &entity->pads[entity->num_pads]; iter++)
>>>>> +		if (media_entity_has_route(entity, start->index, iter->index))
>>>>> +			return iter;
>>>>
>>>> I'd use curly braces.
>>>>
>>>>> +
>>>>> +	return NULL;
>>>>> +}
>>>>
>>>> Does this need to be inlined ?
>>>
>>> I guess it doesn't have to. It's used inside loops and it's rather small so
>>> I think it should be fine that way.
>>
>> It may not be that small. I'd rather let the compiler decide whether to
>> inline it or not.
> 
> Works for me.
> 
>>
>>>>> +
>>>>> +/**
>>>>> + * media_entity_for_each_routed_pad - Iterate over entity pads connected by routes
>>>>
>>>> "routed" sounds a bit weird. Would media_entity_for_each_connected_pad()
>>>> be a better name ?
>>>
>>> "Connected" is often used in context of links. We're dealing with routes
>>> here, so I thought "routed" is appropriate to avoid confusion.
>>
>> I understand the confusion, maybe we can find a better term that would
>> be different than "connected". "routed" really sounds weird in this
>> context.
> 
> I'm fine with connected.
> 
>>
>>>>> + *
>>>>> + * @start: The stating pad
>>>>
>>>> s/stating/starting/
>>>>
>>>>> + * @iter: The iterator pad
>>>>> + *
>>>>> + * Iterate over all pads connected through routes from a given pad
>>>>
>>>> "from the @start pad"
>>>>
>>>>> + * within an entity. The iteration will include the starting pad itself.
>>>>
>>>> s/starting/@start/
>>>>
>>>> I wonder if it wouldn't be more logical to not include the start pad.
>>>> That wouldn't match the current usage patterns, which would need to be
>>>> adapted accordingly, but I'm worried that including the start pad will
>>>> lead to annoying bugs in the future. Maybe I worry too much.
>>>
>>> The aim here is to find all pads that are routed to another pad within the
>>> same entity. If you remove the start pad, it becomes a task harder than
>>> difficult.
>>
>> Intuitively, "all pads that are routed to another pad" doesn't include
>> the "another pad". I'm not opposed to including the start pad as that's
>> what the current usage patterns need, but we should then rename the
>> macro accordingly as its current name is counter-intuitive.
> 
> I'm certainly not opposed to that. But it shouldn't be too much longer than
> what's already there.
> 
>>
>>>> And now that I reread the patch, I also wonder if "start" is a good
>>>> name, as it implies we start the enumeration from a given pad, while we
>>>> enumerate all pads connected to a given pad. I'm not sure what a better
>>>> name would be though, maybe just pad ?
>>>
>>> There are two pads here. Therefore explicitly calling them something else
>>> makes sense IMO.
>>
>> Makes sense, but "start" isn't a good name as we're not starting
>> anything.
> 
> "start" is not a verb here. It's where the iteration *starts*.

Hmm, no, the 'start' is a filter here, isn't it? The macro iterates over 
all pads which have a route to 'start'.

  Tomi

Sakari Ailus April 29, 2021, 12:07 p.m. UTC | #6

On Thu, Apr 29, 2021 at 03:04:38PM +0300, Tomi Valkeinen wrote:
> On 29/04/2021 14:56, Sakari Ailus wrote:
> > Hi Laurent,
> > 
> > On Thu, Apr 29, 2021 at 04:33:48AM +0300, Laurent Pinchart wrote:
> > > Hi Sakari,
> > > 
> > > On Tue, Apr 20, 2021 at 02:48:25PM +0300, Sakari Ailus wrote:
> > > > On Sun, Apr 18, 2021 at 09:20:03PM +0300, Laurent Pinchart wrote:
> > > > > On Thu, Apr 15, 2021 at 04:04:38PM +0300, Tomi Valkeinen wrote:
> > > > > > From: Sakari Ailus <[e-mail]>
> > > > > > 
> > > > > > Add a helper macro for iterating over pads that are connected through
> > > > > > enabled routes. This can be used to find all the connected pads within an
> > > > > > entity, for instance starting from the pad which has been obtained during
> > > > > > the graph walk.
> > > > > > 
> > > > > > Signed-off-by: Sakari Ailus <[e-mail]>
> > > > > > Reviewed-by: Niklas Söderlund <[e-mail]>
> > > > > > 
> > > > > > - Make __media_entity_next_routed_pad() return NULL and adjust the
> > > > > >    iterator to handle that
> > > > > > Signed-off-by: Jacopo Mondi <[e-mail]>
> > > > > > ---
> > > > > >   include/media/media-entity.h | 27 +++++++++++++++++++++++++++
> > > > > >   1 file changed, 27 insertions(+)
> > > > > > 
> > > > > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > > > > > index 73de1c335e4e..edd6f60ed6b4 100644
> > > > > > --- a/include/media/media-entity.h
> > > > > > +++ b/include/media/media-entity.h
> > > > > > @@ -916,6 +916,33 @@ __must_check int media_graph_walk_init(
> > > > > >   bool media_entity_has_route(struct media_entity *entity, unsigned int pad0,
> > > > > >   			    unsigned int pad1);
> > > > > > +static inline struct media_pad *__media_entity_next_routed_pad(
> > > > > > +	struct media_pad *start, struct media_pad *iter)
> > > > > > +{
> > > > > > +	struct media_entity *entity = start->entity;
> > > > > > +
> > > > > > +	for (; iter < &entity->pads[entity->num_pads]; iter++)
> > > > > > +		if (media_entity_has_route(entity, start->index, iter->index))
> > > > > > +			return iter;
> > > > > 
> > > > > I'd use curly braces.
> > > > > 
> > > > > > +
> > > > > > +	return NULL;
> > > > > > +}
> > > > > 
> > > > > Does this need to be inlined ?
> > > > 
> > > > I guess it doesn't have to. It's used inside loops and it's rather small so
> > > > I think it should be fine that way.
> > > 
> > > It may not be that small. I'd rather let the compiler decide whether to
> > > inline it or not.
> > 
> > Works for me.
> > 
> > > 
> > > > > > +
> > > > > > +/**
> > > > > > + * media_entity_for_each_routed_pad - Iterate over entity pads connected by routes
> > > > > 
> > > > > "routed" sounds a bit weird. Would media_entity_for_each_connected_pad()
> > > > > be a better name ?
> > > > 
> > > > "Connected" is often used in context of links. We're dealing with routes
> > > > here, so I thought "routed" is appropriate to avoid confusion.
> > > 
> > > I understand the confusion, maybe we can find a better term that would
> > > be different than "connected". "routed" really sounds weird in this
> > > context.
> > 
> > I'm fine with connected.
> > 
> > > 
> > > > > > + *
> > > > > > + * @start: The stating pad
> > > > > 
> > > > > s/stating/starting/
> > > > > 
> > > > > > + * @iter: The iterator pad
> > > > > > + *
> > > > > > + * Iterate over all pads connected through routes from a given pad
> > > > > 
> > > > > "from the @start pad"
> > > > > 
> > > > > > + * within an entity. The iteration will include the starting pad itself.
> > > > > 
> > > > > s/starting/@start/
> > > > > 
> > > > > I wonder if it wouldn't be more logical to not include the start pad.
> > > > > That wouldn't match the current usage patterns, which would need to be
> > > > > adapted accordingly, but I'm worried that including the start pad will
> > > > > lead to annoying bugs in the future. Maybe I worry too much.
> > > > 
> > > > The aim here is to find all pads that are routed to another pad within the
> > > > same entity. If you remove the start pad, it becomes a task harder than
> > > > difficult.
> > > 
> > > Intuitively, "all pads that are routed to another pad" doesn't include
> > > the "another pad". I'm not opposed to including the start pad as that's
> > > what the current usage patterns need, but we should then rename the
> > > macro accordingly as its current name is counter-intuitive.
> > 
> > I'm certainly not opposed to that. But it shouldn't be too much longer than
> > what's already there.
> > 
> > > 
> > > > > And now that I reread the patch, I also wonder if "start" is a good
> > > > > name, as it implies we start the enumeration from a given pad, while we
> > > > > enumerate all pads connected to a given pad. I'm not sure what a better
> > > > > name would be though, maybe just pad ?
> > > > 
> > > > There are two pads here. Therefore explicitly calling them something else
> > > > makes sense IMO.
> > > 
> > > Makes sense, but "start" isn't a good name as we're not starting
> > > anything.
> > 
> > "start" is not a verb here. It's where the iteration *starts*.
> 
> Hmm, no, the 'start' is a filter here, isn't it? The macro iterates over all
> pads which have a route to 'start'.

The iteration starts from "start", but it does not return all pads, only
the connected ones.

But feel free to use another name if you have a better one.

Tomi Valkeinen April 29, 2021, 12:14 p.m. UTC | #7

On 29/04/2021 15:07, Sakari Ailus wrote:
> On Thu, Apr 29, 2021 at 03:04:38PM +0300, Tomi Valkeinen wrote:
>> On 29/04/2021 14:56, Sakari Ailus wrote:
>>> Hi Laurent,
>>>
>>> On Thu, Apr 29, 2021 at 04:33:48AM +0300, Laurent Pinchart wrote:
>>>> Hi Sakari,
>>>>
>>>> On Tue, Apr 20, 2021 at 02:48:25PM +0300, Sakari Ailus wrote:
>>>>> On Sun, Apr 18, 2021 at 09:20:03PM +0300, Laurent Pinchart wrote:
>>>>>> On Thu, Apr 15, 2021 at 04:04:38PM +0300, Tomi Valkeinen wrote:
>>>>>>> From: Sakari Ailus <[e-mail]>
>>>>>>>
>>>>>>> Add a helper macro for iterating over pads that are connected through
>>>>>>> enabled routes. This can be used to find all the connected pads within an
>>>>>>> entity, for instance starting from the pad which has been obtained during
>>>>>>> the graph walk.
>>>>>>>
>>>>>>> Signed-off-by: Sakari Ailus <[e-mail]>
>>>>>>> Reviewed-by: Niklas Söderlund <[e-mail]>
>>>>>>>
>>>>>>> - Make __media_entity_next_routed_pad() return NULL and adjust the
>>>>>>>     iterator to handle that
>>>>>>> Signed-off-by: Jacopo Mondi <[e-mail]>
>>>>>>> ---
>>>>>>>    include/media/media-entity.h | 27 +++++++++++++++++++++++++++
>>>>>>>    1 file changed, 27 insertions(+)
>>>>>>>
>>>>>>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>>>>>>> index 73de1c335e4e..edd6f60ed6b4 100644
>>>>>>> --- a/include/media/media-entity.h
>>>>>>> +++ b/include/media/media-entity.h
>>>>>>> @@ -916,6 +916,33 @@ __must_check int media_graph_walk_init(
>>>>>>>    bool media_entity_has_route(struct media_entity *entity, unsigned int pad0,
>>>>>>>    			    unsigned int pad1);
>>>>>>> +static inline struct media_pad *__media_entity_next_routed_pad(
>>>>>>> +	struct media_pad *start, struct media_pad *iter)
>>>>>>> +{
>>>>>>> +	struct media_entity *entity = start->entity;
>>>>>>> +
>>>>>>> +	for (; iter < &entity->pads[entity->num_pads]; iter++)
>>>>>>> +		if (media_entity_has_route(entity, start->index, iter->index))
>>>>>>> +			return iter;
>>>>>>
>>>>>> I'd use curly braces.
>>>>>>
>>>>>>> +
>>>>>>> +	return NULL;
>>>>>>> +}
>>>>>>
>>>>>> Does this need to be inlined ?
>>>>>
>>>>> I guess it doesn't have to. It's used inside loops and it's rather small so
>>>>> I think it should be fine that way.
>>>>
>>>> It may not be that small. I'd rather let the compiler decide whether to
>>>> inline it or not.
>>>
>>> Works for me.
>>>
>>>>
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * media_entity_for_each_routed_pad - Iterate over entity pads connected by routes
>>>>>>
>>>>>> "routed" sounds a bit weird. Would media_entity_for_each_connected_pad()
>>>>>> be a better name ?
>>>>>
>>>>> "Connected" is often used in context of links. We're dealing with routes
>>>>> here, so I thought "routed" is appropriate to avoid confusion.
>>>>
>>>> I understand the confusion, maybe we can find a better term that would
>>>> be different than "connected". "routed" really sounds weird in this
>>>> context.
>>>
>>> I'm fine with connected.
>>>
>>>>
>>>>>>> + *
>>>>>>> + * @start: The stating pad
>>>>>>
>>>>>> s/stating/starting/
>>>>>>
>>>>>>> + * @iter: The iterator pad
>>>>>>> + *
>>>>>>> + * Iterate over all pads connected through routes from a given pad
>>>>>>
>>>>>> "from the @start pad"
>>>>>>
>>>>>>> + * within an entity. The iteration will include the starting pad itself.
>>>>>>
>>>>>> s/starting/@start/
>>>>>>
>>>>>> I wonder if it wouldn't be more logical to not include the start pad.
>>>>>> That wouldn't match the current usage patterns, which would need to be
>>>>>> adapted accordingly, but I'm worried that including the start pad will
>>>>>> lead to annoying bugs in the future. Maybe I worry too much.
>>>>>
>>>>> The aim here is to find all pads that are routed to another pad within the
>>>>> same entity. If you remove the start pad, it becomes a task harder than
>>>>> difficult.
>>>>
>>>> Intuitively, "all pads that are routed to another pad" doesn't include
>>>> the "another pad". I'm not opposed to including the start pad as that's
>>>> what the current usage patterns need, but we should then rename the
>>>> macro accordingly as its current name is counter-intuitive.
>>>
>>> I'm certainly not opposed to that. But it shouldn't be too much longer than
>>> what's already there.
>>>
>>>>
>>>>>> And now that I reread the patch, I also wonder if "start" is a good
>>>>>> name, as it implies we start the enumeration from a given pad, while we
>>>>>> enumerate all pads connected to a given pad. I'm not sure what a better
>>>>>> name would be though, maybe just pad ?
>>>>>
>>>>> There are two pads here. Therefore explicitly calling them something else
>>>>> makes sense IMO.
>>>>
>>>> Makes sense, but "start" isn't a good name as we're not starting
>>>> anything.
>>>
>>> "start" is not a verb here. It's where the iteration *starts*.
>>
>> Hmm, no, the 'start' is a filter here, isn't it? The macro iterates over all
>> pads which have a route to 'start'.
> 
> The iteration starts from "start", but it does not return all pads, only
> the connected ones.

No, I don't think it does. It starts from the first pad, and if that is 
connected (has route to 'start') it returns that the first pad.

> But feel free to use another name if you have a better one.

I don't have one, even if I tried. "common_connected_pad" is a bit too 
long =).

Although 'start' hints that it would be the start for the iteration, 
which is misleading. So perhaps 'connected' is better, if only to remove 
the confusion.

  Tomi

Patch

diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 73de1c335e4e..edd6f60ed6b4 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -916,6 +916,33 @@  __must_check int media_graph_walk_init(
 bool media_entity_has_route(struct media_entity *entity, unsigned int pad0,
 			    unsigned int pad1);
 
+static inline struct media_pad *__media_entity_next_routed_pad(
+	struct media_pad *start, struct media_pad *iter)
+{
+	struct media_entity *entity = start->entity;
+
+	for (; iter < &entity->pads[entity->num_pads]; iter++)
+		if (media_entity_has_route(entity, start->index, iter->index))
+			return iter;
+
+	return NULL;
+}
+
+/**
+ * media_entity_for_each_routed_pad - Iterate over entity pads connected by routes
+ *
+ * @start: The stating pad
+ * @iter: The iterator pad
+ *
+ * Iterate over all pads connected through routes from a given pad
+ * within an entity. The iteration will include the starting pad itself.
+ */
+#define media_entity_for_each_routed_pad(start, iter)			\
+	for (iter = __media_entity_next_routed_pad(			\
+		     start, (start)->entity->pads);			\
+	     iter != NULL;						\
+	     iter = __media_entity_next_routed_pad(start, iter + 1))
+
 /**
  * media_graph_walk_cleanup - Release resources used by graph walk.
  *