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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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 <sakari.ailus@linux.intel.com>
> > > > > > 
> > > > > > 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 <sakari.ailus@linux.intel.com>
> > > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > > > 
> > > > > > - Make __media_entity_next_routed_pad() return NULL and adjust the
> > > > > >    iterator to handle that
> > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > > ---
> > > > > >   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.

-- 
Sakari Ailus