All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Add iterator for an entity's data links
@ 2022-06-21 16:34 Daniel Scally
  2022-06-21 16:34 ` [PATCH 1/2] media: media-entity.h: Add iterator for entity " Daniel Scally
  2022-06-21 16:34 ` [PATCH 2/2] media: entity: Use dedicated data link iterator Daniel Scally
  0 siblings, 2 replies; 10+ messages in thread
From: Daniel Scally @ 2022-06-21 16:34 UTC (permalink / raw)
  To: linux-media; +Cc: sakari.ailus, laurent.pinchart, paul.elder

There are a bunch of places in the kernel where code iterates over an entity's
links to perform some action. Those almost invariably have the implicit
assumption that those links are data links, which might not be true following
the introduction of ancillary links. Add a dedicated iterator that skips any
non data links for use instead, which will allow that assumption to hold true.

Daniel Scally (2):
  media: media-entity.h: Add iterator for entity data links
  media: entity: Use dedicated data link iterator

 drivers/media/mc/mc-entity.c |  6 +++---
 include/media/media-entity.h | 26 ++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] media: media-entity.h: Add iterator for entity data links
  2022-06-21 16:34 [PATCH 0/2] Add iterator for an entity's data links Daniel Scally
@ 2022-06-21 16:34 ` Daniel Scally
  2022-06-22  9:08   ` Jacopo Mondi
  2022-06-22  9:16   ` Laurent Pinchart
  2022-06-21 16:34 ` [PATCH 2/2] media: entity: Use dedicated data link iterator Daniel Scally
  1 sibling, 2 replies; 10+ messages in thread
From: Daniel Scally @ 2022-06-21 16:34 UTC (permalink / raw)
  To: linux-media; +Cc: sakari.ailus, laurent.pinchart, paul.elder

Iterating over the links for an entity is a somewhat common need
through the media subsystem, but generally the assumption is that
they will all be data links. To meet that assumption add a new macro
that iterates through an entity's links and skips non-data links.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 include/media/media-entity.h | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index a9a1c0ec5d1c..b13f67f33508 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -550,6 +550,32 @@ static inline bool media_entity_enum_intersects(
 				 min(ent_enum1->idx_max, ent_enum2->idx_max));
 }
 
+static inline struct media_link *
+__media_entity_next_data_link(struct media_entity *entity,
+			      struct media_link *pos)
+{
+	if (!pos) {
+		list_for_each_entry(pos, &entity->links, list)
+			if ((pos->flags & MEDIA_LNK_FL_LINK_TYPE) ==
+			    MEDIA_LNK_FL_DATA_LINK)
+				return pos;
+
+		return NULL;
+	}
+
+	list_for_each_entry_continue(pos, &entity->links, list)
+		if ((pos->flags & MEDIA_LNK_FL_LINK_TYPE) ==
+		    MEDIA_LNK_FL_DATA_LINK)
+			return pos;
+
+	return NULL;
+}
+
+#define for_each_media_entity_data_link(entity, pos)		\
+	for (pos = __media_entity_next_data_link(entity, NULL);	\
+	     pos;						\
+	     pos = __media_entity_next_data_link(entity, pos))
+
 /**
  * gobj_to_entity - returns the struct &media_entity pointer from the
  *	@gobj contained on it.
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] media: entity: Use dedicated data link iterator
  2022-06-21 16:34 [PATCH 0/2] Add iterator for an entity's data links Daniel Scally
  2022-06-21 16:34 ` [PATCH 1/2] media: media-entity.h: Add iterator for entity " Daniel Scally
@ 2022-06-21 16:34 ` Daniel Scally
  2022-06-22  9:18   ` Laurent Pinchart
  1 sibling, 1 reply; 10+ messages in thread
From: Daniel Scally @ 2022-06-21 16:34 UTC (permalink / raw)
  To: linux-media; +Cc: sakari.ailus, laurent.pinchart, paul.elder

Where iteration over links for an entity is clearly assuming that
all of those links are data links, use the new iterator to guarantee
that that assumption is met.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 drivers/media/mc/mc-entity.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index 11f5207f73aa..f46690fd141d 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -449,7 +449,7 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
 		bitmap_zero(active, entity->num_pads);
 		bitmap_fill(has_no_links, entity->num_pads);
 
-		list_for_each_entry(link, &entity->links, list) {
+		for_each_media_entity_data_link(entity, link) {
 			struct media_pad *pad = link->sink->entity == entity
 						? link->sink : link->source;
 
@@ -888,7 +888,7 @@ media_entity_find_link(struct media_pad *source, struct media_pad *sink)
 {
 	struct media_link *link;
 
-	list_for_each_entry(link, &source->entity->links, list) {
+	for_each_media_entity_data_link(source->entity, link) {
 		if (link->source->entity == source->entity &&
 		    link->source->index == source->index &&
 		    link->sink->entity == sink->entity &&
@@ -904,7 +904,7 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad)
 {
 	struct media_link *link;
 
-	list_for_each_entry(link, &pad->entity->links, list) {
+	for_each_media_entity_data_link(pad->entity, link) {
 		if (!(link->flags & MEDIA_LNK_FL_ENABLED))
 			continue;
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] media: media-entity.h: Add iterator for entity data links
  2022-06-21 16:34 ` [PATCH 1/2] media: media-entity.h: Add iterator for entity " Daniel Scally
@ 2022-06-22  9:08   ` Jacopo Mondi
  2022-06-22  9:17     ` Laurent Pinchart
  2022-06-22  9:40     ` Daniel Scally
  2022-06-22  9:16   ` Laurent Pinchart
  1 sibling, 2 replies; 10+ messages in thread
From: Jacopo Mondi @ 2022-06-22  9:08 UTC (permalink / raw)
  To: Daniel Scally; +Cc: linux-media, sakari.ailus, laurent.pinchart, paul.elder

Hi Dan

On Tue, Jun 21, 2022 at 05:34:56PM +0100, Daniel Scally wrote:
> Iterating over the links for an entity is a somewhat common need
> through the media subsystem, but generally the assumption is that
> they will all be data links. To meet that assumption add a new macro
> that iterates through an entity's links and skips non-data links.

Do you foresee usages of a similar iterator but for ancillary (or
interface) links ?

In that case you could add a 'link_type' flag to
__media_entity_next_data_link

>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
>  include/media/media-entity.h | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index a9a1c0ec5d1c..b13f67f33508 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -550,6 +550,32 @@ static inline bool media_entity_enum_intersects(
>  				 min(ent_enum1->idx_max, ent_enum2->idx_max));
>  }
>
> +static inline struct media_link *

Isn't this a bit too much for inlining ? Also I heard many times that
it's not worth anymore trying to outsmart the compiler and inline is
discouraged in most cases ? (and it kind of makes sense to me, but I
sometimes wonder if that's some form of cargo cult..)

> +__media_entity_next_data_link(struct media_entity *entity,
> +			      struct media_link *pos)
> +{
> +	if (!pos) {
> +		list_for_each_entry(pos, &entity->links, list)

nit: coding style requires you to have braces

------------------------------------------------------------------------------
from Documentation/process/coding-style.rst:
Also, use braces when a loop contains more than a single simple statement:

.. code-block:: c

	while (condition) {
		if (test)
			do_something();
	}
------------------------------------------------------------------------------

> +			if ((pos->flags & MEDIA_LNK_FL_LINK_TYPE) ==
> +			    MEDIA_LNK_FL_DATA_LINK)
> +				return pos;
> +
> +		return NULL;
> +	}
> +
> +	list_for_each_entry_continue(pos, &entity->links, list)
> +		if ((pos->flags & MEDIA_LNK_FL_LINK_TYPE) ==
> +		    MEDIA_LNK_FL_DATA_LINK)
> +			return pos;
> +
> +	return NULL;

I wonder if the same could be achieved with list_for_each_entry_from() ?

	pos = pos ? list_next_entry(pos, list)
		  : list_first_entry(&entity->links, typeof(*pos), list);

        list_for_each_entry_from(pos, ...) {
                if (...)
                        return pos;

        }

        return NULL;

If I'm not mistaken the two versions are functionally equivalent..

The iterator seems a good idea. Do you plan to use it for
"media: rkisp1: Don't create data links for non-sensor subdevs" too,
or changing the list of subdevs to iterate is enough there ?

Thanks
   j

> +}
> +
> +#define for_each_media_entity_data_link(entity, pos)		\
> +	for (pos = __media_entity_next_data_link(entity, NULL);	\
> +	     pos;						\
> +	     pos = __media_entity_next_data_link(entity, pos))
> +
>  /**
>   * gobj_to_entity - returns the struct &media_entity pointer from the
>   *	@gobj contained on it.
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] media: media-entity.h: Add iterator for entity data links
  2022-06-21 16:34 ` [PATCH 1/2] media: media-entity.h: Add iterator for entity " Daniel Scally
  2022-06-22  9:08   ` Jacopo Mondi
@ 2022-06-22  9:16   ` Laurent Pinchart
  1 sibling, 0 replies; 10+ messages in thread
From: Laurent Pinchart @ 2022-06-22  9:16 UTC (permalink / raw)
  To: Daniel Scally; +Cc: linux-media, sakari.ailus, paul.elder

Hi Daniel,

Thank you for the patch.

On Tue, Jun 21, 2022 at 05:34:56PM +0100, Daniel Scally wrote:
> Iterating over the links for an entity is a somewhat common need
> through the media subsystem, but generally the assumption is that
> they will all be data links. To meet that assumption add a new macro
> that iterates through an entity's links and skips non-data links.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
>  include/media/media-entity.h | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index a9a1c0ec5d1c..b13f67f33508 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -550,6 +550,32 @@ static inline bool media_entity_enum_intersects(
>  				 min(ent_enum1->idx_max, ent_enum2->idx_max));
>  }
>  
> +static inline struct media_link *
> +__media_entity_next_data_link(struct media_entity *entity,
> +			      struct media_link *pos)

We could make this more dynamic by passing the link type as an argument,
adding more iteration macros for different link types in the future
would then be easier. Up to you.

> +{
> +	if (!pos) {
> +		list_for_each_entry(pos, &entity->links, list)
> +			if ((pos->flags & MEDIA_LNK_FL_LINK_TYPE) ==
> +			    MEDIA_LNK_FL_DATA_LINK)
> +				return pos;
> +
> +		return NULL;
> +	}
> +
> +	list_for_each_entry_continue(pos, &entity->links, list)
> +		if ((pos->flags & MEDIA_LNK_FL_LINK_TYPE) ==
> +		    MEDIA_LNK_FL_DATA_LINK)
> +			return pos;

This could be simplified to

	if (!pos)
		pos = list_entry(&entity->links, struct media_link, list);

	list_for_each_entry_continue(pos, &entity->links, list) {
		if ((pos->flags & MEDIA_LNK_FL_LINK_TYPE) ==
		    MEDIA_LNK_FL_DATA_LINK)
			return pos;
	}

> +
> +	return NULL;
> +}

This is a bit big for an inline function, could we move the
implementation to the .c file ?

> +
> +#define for_each_media_entity_data_link(entity, pos)		\

s/pos/link/ ?

> +	for (pos = __media_entity_next_data_link(entity, NULL);	\
> +	     pos;						\
> +	     pos = __media_entity_next_data_link(entity, pos))
> +

I'm afraid this needs documentation :-)
(https://lore.kernel.org/linux-media/20220301161156.1119557-13-tomi.valkeinen@ideasonboard.com/
can be used as an example)

>  /**
>   * gobj_to_entity - returns the struct &media_entity pointer from the
>   *	@gobj contained on it.

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] media: media-entity.h: Add iterator for entity data links
  2022-06-22  9:08   ` Jacopo Mondi
@ 2022-06-22  9:17     ` Laurent Pinchart
  2022-06-22  9:22       ` Jacopo Mondi
  2022-06-22  9:40     ` Daniel Scally
  1 sibling, 1 reply; 10+ messages in thread
From: Laurent Pinchart @ 2022-06-22  9:17 UTC (permalink / raw)
  To: Jacopo Mondi; +Cc: Daniel Scally, linux-media, sakari.ailus, paul.elder

On Wed, Jun 22, 2022 at 11:08:59AM +0200, Jacopo Mondi wrote:
> Hi Dan
> 
> On Tue, Jun 21, 2022 at 05:34:56PM +0100, Daniel Scally wrote:
> > Iterating over the links for an entity is a somewhat common need
> > through the media subsystem, but generally the assumption is that
> > they will all be data links. To meet that assumption add a new macro
> > that iterates through an entity's links and skips non-data links.
> 
> Do you foresee usages of a similar iterator but for ancillary (or
> interface) links ?
> 
> In that case you could add a 'link_type' flag to
> __media_entity_next_data_link
> 
> >
> > Signed-off-by: Daniel Scally <djrscally@gmail.com>
> > ---
> >  include/media/media-entity.h | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index a9a1c0ec5d1c..b13f67f33508 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -550,6 +550,32 @@ static inline bool media_entity_enum_intersects(
> >  				 min(ent_enum1->idx_max, ent_enum2->idx_max));
> >  }
> >
> > +static inline struct media_link *
> 
> Isn't this a bit too much for inlining ? Also I heard many times that
> it's not worth anymore trying to outsmart the compiler and inline is
> discouraged in most cases ? (and it kind of makes sense to me, but I
> sometimes wonder if that's some form of cargo cult..)

That's right, but in .h files you need to manually inline, otherwise
you'll end up with one copy per compilation unit.

> > +__media_entity_next_data_link(struct media_entity *entity,
> > +			      struct media_link *pos)
> > +{
> > +	if (!pos) {
> > +		list_for_each_entry(pos, &entity->links, list)
> 
> nit: coding style requires you to have braces
> 
> ------------------------------------------------------------------------------
> from Documentation/process/coding-style.rst:
> Also, use braces when a loop contains more than a single simple statement:
> 
> .. code-block:: c
> 
> 	while (condition) {
> 		if (test)
> 			do_something();
> 	}
> ------------------------------------------------------------------------------
> 
> > +			if ((pos->flags & MEDIA_LNK_FL_LINK_TYPE) ==
> > +			    MEDIA_LNK_FL_DATA_LINK)
> > +				return pos;
> > +
> > +		return NULL;
> > +	}
> > +
> > +	list_for_each_entry_continue(pos, &entity->links, list)
> > +		if ((pos->flags & MEDIA_LNK_FL_LINK_TYPE) ==
> > +		    MEDIA_LNK_FL_DATA_LINK)
> > +			return pos;
> > +
> > +	return NULL;
> 
> I wonder if the same could be achieved with list_for_each_entry_from() ?
> 
> 	pos = pos ? list_next_entry(pos, list)
> 		  : list_first_entry(&entity->links, typeof(*pos), list);
> 
>         list_for_each_entry_from(pos, ...) {
>                 if (...)
>                         return pos;
> 
>         }
> 
>         return NULL;

That's even better than what I've suggested.

> If I'm not mistaken the two versions are functionally equivalent..
> 
> The iterator seems a good idea. Do you plan to use it for
> "media: rkisp1: Don't create data links for non-sensor subdevs" too,
> or changing the list of subdevs to iterate is enough there ?
> 
> > +}
> > +
> > +#define for_each_media_entity_data_link(entity, pos)		\
> > +	for (pos = __media_entity_next_data_link(entity, NULL);	\
> > +	     pos;						\
> > +	     pos = __media_entity_next_data_link(entity, pos))
> > +
> >  /**
> >   * gobj_to_entity - returns the struct &media_entity pointer from the
> >   *	@gobj contained on it.

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] media: entity: Use dedicated data link iterator
  2022-06-21 16:34 ` [PATCH 2/2] media: entity: Use dedicated data link iterator Daniel Scally
@ 2022-06-22  9:18   ` Laurent Pinchart
  0 siblings, 0 replies; 10+ messages in thread
From: Laurent Pinchart @ 2022-06-22  9:18 UTC (permalink / raw)
  To: Daniel Scally; +Cc: linux-media, sakari.ailus, paul.elder

Hi Dan,

Thank you for the patch.

On Tue, Jun 21, 2022 at 05:34:57PM +0100, Daniel Scally wrote:
> Where iteration over links for an entity is clearly assuming that
> all of those links are data links, use the new iterator to guarantee
> that that assumption is met.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/mc/mc-entity.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index 11f5207f73aa..f46690fd141d 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -449,7 +449,7 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
>  		bitmap_zero(active, entity->num_pads);
>  		bitmap_fill(has_no_links, entity->num_pads);
>  
> -		list_for_each_entry(link, &entity->links, list) {
> +		for_each_media_entity_data_link(entity, link) {
>  			struct media_pad *pad = link->sink->entity == entity
>  						? link->sink : link->source;
>  
> @@ -888,7 +888,7 @@ media_entity_find_link(struct media_pad *source, struct media_pad *sink)
>  {
>  	struct media_link *link;
>  
> -	list_for_each_entry(link, &source->entity->links, list) {
> +	for_each_media_entity_data_link(source->entity, link) {
>  		if (link->source->entity == source->entity &&
>  		    link->source->index == source->index &&
>  		    link->sink->entity == sink->entity &&
> @@ -904,7 +904,7 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad)
>  {
>  	struct media_link *link;
>  
> -	list_for_each_entry(link, &pad->entity->links, list) {
> +	for_each_media_entity_data_link(pad->entity, link) {
>  		if (!(link->flags & MEDIA_LNK_FL_ENABLED))
>  			continue;
>  

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] media: media-entity.h: Add iterator for entity data links
  2022-06-22  9:17     ` Laurent Pinchart
@ 2022-06-22  9:22       ` Jacopo Mondi
  2022-06-22  9:30         ` Daniel Scally
  0 siblings, 1 reply; 10+ messages in thread
From: Jacopo Mondi @ 2022-06-22  9:22 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Daniel Scally, linux-media, sakari.ailus, paul.elder

hi Laurent,

On Wed, Jun 22, 2022 at 12:17:56PM +0300, Laurent Pinchart wrote:
> On Wed, Jun 22, 2022 at 11:08:59AM +0200, Jacopo Mondi wrote:
> > Hi Dan
> >
> > On Tue, Jun 21, 2022 at 05:34:56PM +0100, Daniel Scally wrote:
> > > Iterating over the links for an entity is a somewhat common need
> > > through the media subsystem, but generally the assumption is that
> > > they will all be data links. To meet that assumption add a new macro
> > > that iterates through an entity's links and skips non-data links.
> >
> > Do you foresee usages of a similar iterator but for ancillary (or
> > interface) links ?
> >
> > In that case you could add a 'link_type' flag to
> > __media_entity_next_data_link
> >
> > >
> > > Signed-off-by: Daniel Scally <djrscally@gmail.com>
> > > ---
> > >  include/media/media-entity.h | 26 ++++++++++++++++++++++++++
> > >  1 file changed, 26 insertions(+)
> > >
> > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > > index a9a1c0ec5d1c..b13f67f33508 100644
> > > --- a/include/media/media-entity.h
> > > +++ b/include/media/media-entity.h
> > > @@ -550,6 +550,32 @@ static inline bool media_entity_enum_intersects(
> > >  				 min(ent_enum1->idx_max, ent_enum2->idx_max));
> > >  }
> > >
> > > +static inline struct media_link *
> >
> > Isn't this a bit too much for inlining ? Also I heard many times that
> > it's not worth anymore trying to outsmart the compiler and inline is
> > discouraged in most cases ? (and it kind of makes sense to me, but I
> > sometimes wonder if that's some form of cargo cult..)
>
> That's right, but in .h files you need to manually inline, otherwise
> you'll end up with one copy per compilation unit.
>

I was suggesting to move it to a .c file in facts, likely mc-entity.c

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] media: media-entity.h: Add iterator for entity data links
  2022-06-22  9:22       ` Jacopo Mondi
@ 2022-06-22  9:30         ` Daniel Scally
  0 siblings, 0 replies; 10+ messages in thread
From: Daniel Scally @ 2022-06-22  9:30 UTC (permalink / raw)
  To: Jacopo Mondi, Laurent Pinchart; +Cc: linux-media, sakari.ailus, paul.elder

Morning both

On 22/06/2022 10:22, Jacopo Mondi wrote:
> hi Laurent,
>
> On Wed, Jun 22, 2022 at 12:17:56PM +0300, Laurent Pinchart wrote:
>> On Wed, Jun 22, 2022 at 11:08:59AM +0200, Jacopo Mondi wrote:
>>> Hi Dan
>>>
>>> On Tue, Jun 21, 2022 at 05:34:56PM +0100, Daniel Scally wrote:
>>>> Iterating over the links for an entity is a somewhat common need
>>>> through the media subsystem, but generally the assumption is that
>>>> they will all be data links. To meet that assumption add a new macro
>>>> that iterates through an entity's links and skips non-data links.
>>> Do you foresee usages of a similar iterator but for ancillary (or
>>> interface) links ?
>>>
>>> In that case you could add a 'link_type' flag to
>>> __media_entity_next_data_link
>>>
>>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>>> ---
>>>>  include/media/media-entity.h | 26 ++++++++++++++++++++++++++
>>>>  1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>>>> index a9a1c0ec5d1c..b13f67f33508 100644
>>>> --- a/include/media/media-entity.h
>>>> +++ b/include/media/media-entity.h
>>>> @@ -550,6 +550,32 @@ static inline bool media_entity_enum_intersects(
>>>>  				 min(ent_enum1->idx_max, ent_enum2->idx_max));
>>>>  }
>>>>
>>>> +static inline struct media_link *
>>> Isn't this a bit too much for inlining ? Also I heard many times that
>>> it's not worth anymore trying to outsmart the compiler and inline is
>>> discouraged in most cases ? (and it kind of makes sense to me, but I
>>> sometimes wonder if that's some form of cargo cult..)
>> That's right, but in .h files you need to manually inline, otherwise
>> you'll end up with one copy per compilation unit.
>>
> I was suggesting to move it to a .c file in facts, likely mc-entity.c


The one copy per compilation unit problem spurred the inline indeed -
but no problem, I'll move it to the .c


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] media: media-entity.h: Add iterator for entity data links
  2022-06-22  9:08   ` Jacopo Mondi
  2022-06-22  9:17     ` Laurent Pinchart
@ 2022-06-22  9:40     ` Daniel Scally
  1 sibling, 0 replies; 10+ messages in thread
From: Daniel Scally @ 2022-06-22  9:40 UTC (permalink / raw)
  To: Jacopo Mondi; +Cc: linux-media, sakari.ailus, laurent.pinchart, paul.elder

Hi Jacopo

On 22/06/2022 10:08, Jacopo Mondi wrote:
> Hi Dan
>
> On Tue, Jun 21, 2022 at 05:34:56PM +0100, Daniel Scally wrote:
>> Iterating over the links for an entity is a somewhat common need
>> through the media subsystem, but generally the assumption is that
>> they will all be data links. To meet that assumption add a new macro
>> that iterates through an entity's links and skips non-data links.
> Do you foresee usages of a similar iterator but for ancillary (or
> interface) links ?
>
> In that case you could add a 'link_type' flag to
> __media_entity_next_data_link


Ooh this is a great idea - I'm not sure we'd need it for the ancillary
links right now but in the future when the flash gets linked then it
could crop up. I'll add the flag, thanks.

>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>>  include/media/media-entity.h | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>> index a9a1c0ec5d1c..b13f67f33508 100644
>> --- a/include/media/media-entity.h
>> +++ b/include/media/media-entity.h
>> @@ -550,6 +550,32 @@ static inline bool media_entity_enum_intersects(
>>  				 min(ent_enum1->idx_max, ent_enum2->idx_max));
>>  }
>>
>> +static inline struct media_link *
> Isn't this a bit too much for inlining ? Also I heard many times that
> it's not worth anymore trying to outsmart the compiler and inline is
> discouraged in most cases ? (and it kind of makes sense to me, but I
> sometimes wonder if that's some form of cargo cult..)


Probably - I'll move the function to media-entity.c instead.

>
>> +__media_entity_next_data_link(struct media_entity *entity,
>> +			      struct media_link *pos)
>> +{
>> +	if (!pos) {
>> +		list_for_each_entry(pos, &entity->links, list)
> nit: coding style requires you to have braces
>
> ------------------------------------------------------------------------------
> from Documentation/process/coding-style.rst:
> Also, use braces when a loop contains more than a single simple statement:
>
> .. code-block:: c
>
> 	while (condition) {
> 		if (test)
> 			do_something();
> 	}
> ------------------------------------------------------------------------------


Good point, thanks :)

>> +			if ((pos->flags & MEDIA_LNK_FL_LINK_TYPE) ==
>> +			    MEDIA_LNK_FL_DATA_LINK)
>> +				return pos;
>> +
>> +		return NULL;
>> +	}
>> +
>> +	list_for_each_entry_continue(pos, &entity->links, list)
>> +		if ((pos->flags & MEDIA_LNK_FL_LINK_TYPE) ==
>> +		    MEDIA_LNK_FL_DATA_LINK)
>> +			return pos;
>> +
>> +	return NULL;
> I wonder if the same could be achieved with list_for_each_entry_from() ?
>
> 	pos = pos ? list_next_entry(pos, list)
> 		  : list_first_entry(&entity->links, typeof(*pos), list);
>
>         list_for_each_entry_from(pos, ...) {
>                 if (...)
>                         return pos;
>
>         }
>
>         return NULL;
>
> If I'm not mistaken the two versions are functionally equivalent..


Yes that's much better - thanks!

>
> The iterator seems a good idea. Do you plan to use it for
> "media: rkisp1: Don't create data links for non-sensor subdevs" too,
> or changing the list of subdevs to iterate is enough there ?


No it's a slightly different problem there actually, I'm going to switch
that to the list of async subdevs in the same way the ipu3-cio2 works as
you suggested instead.


> Thanks
>    j
>
>> +}
>> +
>> +#define for_each_media_entity_data_link(entity, pos)		\
>> +	for (pos = __media_entity_next_data_link(entity, NULL);	\
>> +	     pos;						\
>> +	     pos = __media_entity_next_data_link(entity, pos))
>> +
>>  /**
>>   * gobj_to_entity - returns the struct &media_entity pointer from the
>>   *	@gobj contained on it.
>> --
>> 2.25.1
>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-06-22  9:40 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-21 16:34 [PATCH 0/2] Add iterator for an entity's data links Daniel Scally
2022-06-21 16:34 ` [PATCH 1/2] media: media-entity.h: Add iterator for entity " Daniel Scally
2022-06-22  9:08   ` Jacopo Mondi
2022-06-22  9:17     ` Laurent Pinchart
2022-06-22  9:22       ` Jacopo Mondi
2022-06-22  9:30         ` Daniel Scally
2022-06-22  9:40     ` Daniel Scally
2022-06-22  9:16   ` Laurent Pinchart
2022-06-21 16:34 ` [PATCH 2/2] media: entity: Use dedicated data link iterator Daniel Scally
2022-06-22  9:18   ` Laurent Pinchart

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.