diff mbox series

[v2,12/30] media: entity: Add an iterator helper for connected pads

Message ID 20181101233144.31507-13-niklas.soderlund+renesas@ragnatech.se (mailing list archive)
State Not Applicable
Delegated to: Geert Uytterhoeven
Headers show
Series v4l: add support for multiplexed streams | expand

Commit Message

Niklas Söderlund Nov. 1, 2018, 11:31 p.m. UTC 
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>
---
 include/media/media-entity.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Laurent Pinchart Jan. 15, 2019, 11:24 p.m. UTC | #1 
Hi Niklas,

Thank you for the patch.

On Fri, Nov 02, 2018 at 12:31:26AM +0100, Niklas Söderlund 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>
> ---
>  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 9540d2af80f19805..4bb1b568e1ac4795 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -936,6 +936,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_for_routed_pads_next(
> +	struct media_pad *start, struct media_pad *iter)
> +{
> +	struct media_entity *entity = start->entity;
> +
> +	while (iter < &entity->pads[entity->num_pads] &&
> +	       !media_entity_has_route(entity, start->index, iter->index))
> +		iter++;
> +
> +	return iter;

Returning a pointer past the end of the array is asking for trouble. I
think we should return NULL in that case, and adapt the check in
media_entity_for_routed_pads() accordingly.

> +}
> +
> +/**
> + * media_entity_for_routed_pads - Iterate over entity pads connected by routes
> + *
> + * @start: The stating pad

s/stating/starting/

> + * @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_routed_pads(start, iter)			\

Maybe media_entity_for_each_routed_pad() ? Or just
for_each_entity_routed_pad() ?

> +	for (iter = __media_entity_for_routed_pads_next(		\

And how about __media_entity_next_routed_pad() ?

> +		     start, (start)->entity->pads);			\
> +	     iter < &(start)->entity->pads[(start)->entity->num_pads];	\
> +	     iter = __media_entity_for_routed_pads_next(start, iter + 1))
> +
>  /**
>   * media_graph_walk_cleanup - Release resources used by graph walk.
>   *
Sakari Ailus Jan. 22, 2019, 3:36 p.m. UTC | #2 
Hi Laurent,

On Wed, Jan 16, 2019 at 01:24:05AM +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Fri, Nov 02, 2018 at 12:31:26AM +0100, Niklas Söderlund 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>
> > ---
> >  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 9540d2af80f19805..4bb1b568e1ac4795 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -936,6 +936,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_for_routed_pads_next(
> > +	struct media_pad *start, struct media_pad *iter)
> > +{
> > +	struct media_entity *entity = start->entity;
> > +
> > +	while (iter < &entity->pads[entity->num_pads] &&
> > +	       !media_entity_has_route(entity, start->index, iter->index))
> > +		iter++;
> > +
> > +	return iter;
> 
> Returning a pointer past the end of the array is asking for trouble. I
> think we should return NULL in that case, and adapt the check in
> media_entity_for_routed_pads() accordingly.

Well, that pointer never gets used, in a similar manner than simply looping
over an array. I wouldn't change that --- this function is also integrally
a part of media_entity_for_each_routed_pad() implementation below.

> 
> > +}
> > +
> > +/**
> > + * media_entity_for_routed_pads - Iterate over entity pads connected by routes
> > + *
> > + * @start: The stating pad
> 
> s/stating/starting/
> 
> > + * @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_routed_pads(start, iter)			\
> 
> Maybe media_entity_for_each_routed_pad() ? Or just

I'd prefer this one: we have media_entity_ prefix for pretty much
everything that handles entities.

> for_each_entity_routed_pad() ?
> 
> > +	for (iter = __media_entity_for_routed_pads_next(		\
> 
> And how about __media_entity_next_routed_pad() ?

Yes.

> 
> > +		     start, (start)->entity->pads);			\
> > +	     iter < &(start)->entity->pads[(start)->entity->num_pads];	\
> > +	     iter = __media_entity_for_routed_pads_next(start, iter + 1))
> > +
> >  /**
> >   * media_graph_walk_cleanup - Release resources used by graph walk.
> >   *
>
Laurent Pinchart Jan. 22, 2019, 3:38 p.m. UTC | #3 
Hi Sakari,

On Tue, Jan 22, 2019 at 05:36:55PM +0200, Sakari Ailus wrote:
> On Wed, Jan 16, 2019 at 01:24:05AM +0200, Laurent Pinchart wrote:
> > On Fri, Nov 02, 2018 at 12:31:26AM +0100, Niklas Söderlund 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>
> >> ---
> >>  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 9540d2af80f19805..4bb1b568e1ac4795 100644
> >> --- a/include/media/media-entity.h
> >> +++ b/include/media/media-entity.h
> >> @@ -936,6 +936,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_for_routed_pads_next(
> >> +	struct media_pad *start, struct media_pad *iter)
> >> +{
> >> +	struct media_entity *entity = start->entity;
> >> +
> >> +	while (iter < &entity->pads[entity->num_pads] &&
> >> +	       !media_entity_has_route(entity, start->index, iter->index))
> >> +		iter++;
> >> +
> >> +	return iter;
> > 
> > Returning a pointer past the end of the array is asking for trouble. I
> > think we should return NULL in that case, and adapt the check in
> > media_entity_for_routed_pads() accordingly.
> 
> Well, that pointer never gets used, in a similar manner than simply looping
> over an array. I wouldn't change that --- this function is also integrally
> a part of media_entity_for_each_routed_pad() implementation below.

I know it doesn't get used, but it's still asking for trouble, it will
be easy to use it by mistake. Returning NULL would be clearer.

> >> +}
> >> +
> >> +/**
> >> + * media_entity_for_routed_pads - Iterate over entity pads connected by routes
> >> + *
> >> + * @start: The stating pad
> > 
> > s/stating/starting/
> > 
> >> + * @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_routed_pads(start, iter)			\
> > 
> > Maybe media_entity_for_each_routed_pad() ? Or just
> 
> I'd prefer this one: we have media_entity_ prefix for pretty much
> everything that handles entities.

It's a bit long, but OK.

> > for_each_entity_routed_pad() ?
> > 
> >> +	for (iter = __media_entity_for_routed_pads_next(		\
> > 
> > And how about __media_entity_next_routed_pad() ?
> 
> Yes.
> 
> > 
> >> +		     start, (start)->entity->pads);			\
> >> +	     iter < &(start)->entity->pads[(start)->entity->num_pads];	\
> >> +	     iter = __media_entity_for_routed_pads_next(start, iter + 1))
> >> +
> >>  /**
> >>   * media_graph_walk_cleanup - Release resources used by graph walk.
> >>   *
Sakari Ailus Jan. 22, 2019, 4:21 p.m. UTC | #4 
On Tue, Jan 22, 2019 at 05:38:48PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tue, Jan 22, 2019 at 05:36:55PM +0200, Sakari Ailus wrote:
> > On Wed, Jan 16, 2019 at 01:24:05AM +0200, Laurent Pinchart wrote:
> > > On Fri, Nov 02, 2018 at 12:31:26AM +0100, Niklas Söderlund 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>
> > >> ---
> > >>  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 9540d2af80f19805..4bb1b568e1ac4795 100644
> > >> --- a/include/media/media-entity.h
> > >> +++ b/include/media/media-entity.h
> > >> @@ -936,6 +936,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_for_routed_pads_next(
> > >> +	struct media_pad *start, struct media_pad *iter)
> > >> +{
> > >> +	struct media_entity *entity = start->entity;
> > >> +
> > >> +	while (iter < &entity->pads[entity->num_pads] &&
> > >> +	       !media_entity_has_route(entity, start->index, iter->index))
> > >> +		iter++;
> > >> +
> > >> +	return iter;
> > > 
> > > Returning a pointer past the end of the array is asking for trouble. I
> > > think we should return NULL in that case, and adapt the check in
> > > media_entity_for_routed_pads() accordingly.
> > 
> > Well, that pointer never gets used, in a similar manner than simply looping
> > over an array. I wouldn't change that --- this function is also integrally
> > a part of media_entity_for_each_routed_pad() implementation below.
> 
> I know it doesn't get used, but it's still asking for trouble, it will
> be easy to use it by mistake. Returning NULL would be clearer.

Hmph. Well, that's a fair point, too... The caller must be also changed to
cope with NULL then. It'd be nice to see the end result of that for it's
already fairly complex.

> 
> > >> +}
> > >> +
> > >> +/**
> > >> + * media_entity_for_routed_pads - Iterate over entity pads connected by routes
> > >> + *
> > >> + * @start: The stating pad
> > > 
> > > s/stating/starting/
> > > 
> > >> + * @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_routed_pads(start, iter)			\
> > > 
> > > Maybe media_entity_for_each_routed_pad() ? Or just
> > 
> > I'd prefer this one: we have media_entity_ prefix for pretty much
> > everything that handles entities.
> 
> It's a bit long, but OK.
> 
> > > for_each_entity_routed_pad() ?
> > > 
> > >> +	for (iter = __media_entity_for_routed_pads_next(		\
> > > 
> > > And how about __media_entity_next_routed_pad() ?
> > 
> > Yes.
> > 
> > > 
> > >> +		     start, (start)->entity->pads);			\
> > >> +	     iter < &(start)->entity->pads[(start)->entity->num_pads];	\
> > >> +	     iter = __media_entity_for_routed_pads_next(start, iter + 1))
> > >> +
> > >>  /**
> > >>   * media_graph_walk_cleanup - Release resources used by graph walk.
> > >>   *
> 
> -- 
> Regards,
> 
> Laurent Pinchart
diff mbox series

Patch

diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 9540d2af80f19805..4bb1b568e1ac4795 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -936,6 +936,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_for_routed_pads_next(
+	struct media_pad *start, struct media_pad *iter)
+{
+	struct media_entity *entity = start->entity;
+
+	while (iter < &entity->pads[entity->num_pads] &&
+	       !media_entity_has_route(entity, start->index, iter->index))
+		iter++;
+
+	return iter;
+}
+
+/**
+ * media_entity_for_routed_pads - 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_routed_pads(start, iter)			\
+	for (iter = __media_entity_for_routed_pads_next(		\
+		     start, (start)->entity->pads);			\
+	     iter < &(start)->entity->pads[(start)->entity->num_pads];	\
+	     iter = __media_entity_for_routed_pads_next(start, iter + 1))
+
 /**
  * media_graph_walk_cleanup - Release resources used by graph walk.
  *