| Message ID | 20220614191127.3420492-38-paul.elder@ideasonboard.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: rkisp1: Cleanups and add support for i.MX8MP | expand |
+ Sakari and Hans On Wed, Jun 15, 2022 at 04:11:09AM +0900, Paul Elder wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > The media_entity_remote_pad() helper function returns the first remote > pad it find connected to a given pad. Beside being possibly ill-named > (as it operates on a pad, not an entity) and non-deterministic (as it > stops at the first enabled link), the fact that it returns the first > match makes it unsuitable for drivers that need to guarantee that a > single link is enabled, for instance when an entity can process data > from one of multiple sources at a time. > > For those use cases, add a new helper function, > media_entity_remote_pad_unique(), that operates on an entity and returns > a remote pad, with a guarantee that only one link is enabled. To ease > its use in drivers, also add an inline wrapper that locates source pads > specifically. A wrapper that locates sink pads can easily be added when > needed. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Documentation/driver-api/media/mc-core.rst | 4 +- > drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ > include/media/media-entity.h | 45 ++++++++++++++++++++++ > 3 files changed, 85 insertions(+), 2 deletions(-) > > diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > index 02481a2513b9..a2d1e32e3abb 100644 > --- a/Documentation/driver-api/media/mc-core.rst > +++ b/Documentation/driver-api/media/mc-core.rst > @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. > > Helper functions can be used to find a link between two given pads, or a pad > connected to another pad through an enabled link > -:c:func:`media_entity_find_link()` and > -:c:func:`media_entity_remote_pad()`. > +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and > +:c:func:`media_entity_remote_source_pad()`). > > Use count and power handling > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > index 11f5207f73aa..1febf5a86be6 100644 > --- a/drivers/media/mc/mc-entity.c > +++ b/drivers/media/mc/mc-entity.c > @@ -9,6 +9,7 @@ > */ > > #include <linux/bitmap.h> > +#include <linux/list.h> > #include <linux/property.h> > #include <linux/slab.h> > #include <media/media-entity.h> > @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) > } > EXPORT_SYMBOL_GPL(media_entity_remote_pad); > > +struct media_pad * > +media_entity_remote_pad_unique(const struct media_entity *entity, > + unsigned int type) > +{ > + struct media_pad *pad = NULL; > + struct media_link *link; > + > + list_for_each_entry(link, &entity->links, list) { > + struct media_pad *local_pad; > + struct media_pad *remote_pad; > + > + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) > + continue; > + > + if (type == MEDIA_PAD_FL_SOURCE) { > + local_pad = link->sink; > + remote_pad = link->source; > + } else { > + local_pad = link->source; > + remote_pad = link->sink; > + } > + > + if (local_pad->entity == entity) { > + if (pad) > + return ERR_PTR(-ENOTUNIQ); > + > + pad = remote_pad; > + } > + } > + > + if (!pad) > + return ERR_PTR(-ENOLINK); > + > + return pad; > +} > +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); > + > static void media_interface_init(struct media_device *mdev, > struct media_interface *intf, > u32 gobj_type, > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > index a9a1c0ec5d1c..33d5f52719a0 100644 > --- a/include/media/media-entity.h > +++ b/include/media/media-entity.h > @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, > */ > struct media_pad *media_entity_remote_pad(const struct media_pad *pad); > > +/** > + * media_entity_remote_pad_unique - Find a remote pad connected to an entity > + * @entity: The entity > + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) > + * > + * Search for and return a remote pad of @type connected to @entity through an > + * enabled link. If multiple (or no) remote pads match these criteria, an error > + * is returned. > + * > + * The uniqueness constraint makes this helper function suitable for entities > + * that support a single active source or sink at a time. > + * > + * Return: A pointer to the remote pad, or one of the following error pointers > + * if an error occurs: > + * > + * * -ENOTUNIQ - Multiple links are enabled > + * * -ENOLINK - No connected pad found > + */ > +struct media_pad * > +media_entity_remote_pad_unique(const struct media_entity *entity, > + unsigned int type); > + > +/** > + * media_entity_remote_source_pad - Find a remote source pad connected to an entity > + * @entity: The entity > + * > + * Search for and return a remote source pad connected to @entity through an > + * enabled link. If multiple (or no) remote pads match these criteria, an error > + * is returned. > + * > + * The uniqueness constraint makes this helper function suitable for entities > + * that support a single active source at a time. > + * > + * Return: A pointer to the remote pad, or one of the following error pointers > + * if an error occurs: > + * > + * * -ENOTUNIQ - Multiple links are enabled > + * * -ENOLINK - No connected pad found > + */ > +static inline struct media_pad * > +media_entity_remote_source_pad(const struct media_entity *entity) > +{ > + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); > +} > + > /** > * media_entity_is_streaming - Test if an entity is part of a streaming pipeline > * @entity: The entity > -- > 2.30.2 >
On 6/14/22 21:11, Paul Elder wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > The media_entity_remote_pad() helper function returns the first remote > pad it find connected to a given pad. Beside being possibly ill-named find -> finds > (as it operates on a pad, not an entity) and non-deterministic (as it > stops at the first enabled link), the fact that it returns the first > match makes it unsuitable for drivers that need to guarantee that a > single link is enabled, for instance when an entity can process data > from one of multiple sources at a time. > > For those use cases, add a new helper function, > media_entity_remote_pad_unique(), that operates on an entity and returns > a remote pad, with a guarantee that only one link is enabled. To ease > its use in drivers, also add an inline wrapper that locates source pads > specifically. A wrapper that locates sink pads can easily be added when > needed. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Documentation/driver-api/media/mc-core.rst | 4 +- > drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ > include/media/media-entity.h | 45 ++++++++++++++++++++++ > 3 files changed, 85 insertions(+), 2 deletions(-) > > diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > index 02481a2513b9..a2d1e32e3abb 100644 > --- a/Documentation/driver-api/media/mc-core.rst > +++ b/Documentation/driver-api/media/mc-core.rst > @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. > > Helper functions can be used to find a link between two given pads, or a pad > connected to another pad through an enabled link > -:c:func:`media_entity_find_link()` and > -:c:func:`media_entity_remote_pad()`. > +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and > +:c:func:`media_entity_remote_source_pad()`). > > Use count and power handling > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > index 11f5207f73aa..1febf5a86be6 100644 > --- a/drivers/media/mc/mc-entity.c > +++ b/drivers/media/mc/mc-entity.c > @@ -9,6 +9,7 @@ > */ > > #include <linux/bitmap.h> > +#include <linux/list.h> > #include <linux/property.h> > #include <linux/slab.h> > #include <media/media-entity.h> > @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) > } > EXPORT_SYMBOL_GPL(media_entity_remote_pad); > > +struct media_pad * > +media_entity_remote_pad_unique(const struct media_entity *entity, > + unsigned int type) > +{ > + struct media_pad *pad = NULL; > + struct media_link *link; > + > + list_for_each_entry(link, &entity->links, list) { > + struct media_pad *local_pad; > + struct media_pad *remote_pad; > + > + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) > + continue; > + > + if (type == MEDIA_PAD_FL_SOURCE) { > + local_pad = link->sink; > + remote_pad = link->source; > + } else { > + local_pad = link->source; > + remote_pad = link->sink; > + } > + > + if (local_pad->entity == entity) { > + if (pad) > + return ERR_PTR(-ENOTUNIQ); > + > + pad = remote_pad; > + } > + } > + > + if (!pad) > + return ERR_PTR(-ENOLINK); > + > + return pad; > +} > +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); > + > static void media_interface_init(struct media_device *mdev, > struct media_interface *intf, > u32 gobj_type, > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > index a9a1c0ec5d1c..33d5f52719a0 100644 > --- a/include/media/media-entity.h > +++ b/include/media/media-entity.h > @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, > */ > struct media_pad *media_entity_remote_pad(const struct media_pad *pad); > > +/** > + * media_entity_remote_pad_unique - Find a remote pad connected to an entity > + * @entity: The entity > + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) > + * > + * Search for and return a remote pad of @type connected to @entity through an > + * enabled link. If multiple (or no) remote pads match these criteria, an error > + * is returned. > + * > + * The uniqueness constraint makes this helper function suitable for entities > + * that support a single active source or sink at a time. > + * > + * Return: A pointer to the remote pad, or one of the following error pointers > + * if an error occurs: > + * > + * * -ENOTUNIQ - Multiple links are enabled > + * * -ENOLINK - No connected pad found > + */ > +struct media_pad * > +media_entity_remote_pad_unique(const struct media_entity *entity, > + unsigned int type); > + > +/** > + * media_entity_remote_source_pad - Find a remote source pad connected to an entity Shouldn't this be called media_entity_remote_source_pad_unique? After all, it has the uniqueness constraint, but that's not reflected in the name. With that change you can add my: Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Regards, Hans > + * @entity: The entity > + * > + * Search for and return a remote source pad connected to @entity through an > + * enabled link. If multiple (or no) remote pads match these criteria, an error > + * is returned. > + * > + * The uniqueness constraint makes this helper function suitable for entities > + * that support a single active source at a time. > + * > + * Return: A pointer to the remote pad, or one of the following error pointers > + * if an error occurs: > + * > + * * -ENOTUNIQ - Multiple links are enabled > + * * -ENOLINK - No connected pad found > + */ > +static inline struct media_pad * > +media_entity_remote_source_pad(const struct media_entity *entity) > +{ > + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); > +} > + > /** > * media_entity_is_streaming - Test if an entity is part of a streaming pipeline > * @entity: The entity
On 6/14/22 21:11, Paul Elder wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > The media_entity_remote_pad() helper function returns the first remote > pad it find connected to a given pad. Beside being possibly ill-named > (as it operates on a pad, not an entity) and non-deterministic (as it > stops at the first enabled link), the fact that it returns the first > match makes it unsuitable for drivers that need to guarantee that a > single link is enabled, for instance when an entity can process data > from one of multiple sources at a time. Question: of all the callers of this function, are there any that really need media_entity_remote_pad() instead of media_pad_remote_pad_unique()? Would it be possible to replace all callers of the old function with the new function? If that's the case, then the _unique suffix can be dropped, since that would effectively be the default. And if a function is needed to handle the case where there are multiple enabled links, then a new function should be created. Also, media_entity_remote_pad() should really be renamed to media_pad_remote_pad_first() or something like that, right? I'm not saying you should, but that's really what it does, as I understand it. Regards, Hans > > For those use cases, add a new helper function, > media_entity_remote_pad_unique(), that operates on an entity and returns > a remote pad, with a guarantee that only one link is enabled. To ease > its use in drivers, also add an inline wrapper that locates source pads > specifically. A wrapper that locates sink pads can easily be added when > needed. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Documentation/driver-api/media/mc-core.rst | 4 +- > drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ > include/media/media-entity.h | 45 ++++++++++++++++++++++ > 3 files changed, 85 insertions(+), 2 deletions(-) > > diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > index 02481a2513b9..a2d1e32e3abb 100644 > --- a/Documentation/driver-api/media/mc-core.rst > +++ b/Documentation/driver-api/media/mc-core.rst > @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. > > Helper functions can be used to find a link between two given pads, or a pad > connected to another pad through an enabled link > -:c:func:`media_entity_find_link()` and > -:c:func:`media_entity_remote_pad()`. > +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and > +:c:func:`media_entity_remote_source_pad()`). > > Use count and power handling > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > index 11f5207f73aa..1febf5a86be6 100644 > --- a/drivers/media/mc/mc-entity.c > +++ b/drivers/media/mc/mc-entity.c > @@ -9,6 +9,7 @@ > */ > > #include <linux/bitmap.h> > +#include <linux/list.h> > #include <linux/property.h> > #include <linux/slab.h> > #include <media/media-entity.h> > @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) > } > EXPORT_SYMBOL_GPL(media_entity_remote_pad); > > +struct media_pad * > +media_entity_remote_pad_unique(const struct media_entity *entity, > + unsigned int type) > +{ > + struct media_pad *pad = NULL; > + struct media_link *link; > + > + list_for_each_entry(link, &entity->links, list) { > + struct media_pad *local_pad; > + struct media_pad *remote_pad; > + > + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) > + continue; > + > + if (type == MEDIA_PAD_FL_SOURCE) { > + local_pad = link->sink; > + remote_pad = link->source; > + } else { > + local_pad = link->source; > + remote_pad = link->sink; > + } > + > + if (local_pad->entity == entity) { > + if (pad) > + return ERR_PTR(-ENOTUNIQ); > + > + pad = remote_pad; > + } > + } > + > + if (!pad) > + return ERR_PTR(-ENOLINK); > + > + return pad; > +} > +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); > + > static void media_interface_init(struct media_device *mdev, > struct media_interface *intf, > u32 gobj_type, > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > index a9a1c0ec5d1c..33d5f52719a0 100644 > --- a/include/media/media-entity.h > +++ b/include/media/media-entity.h > @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, > */ > struct media_pad *media_entity_remote_pad(const struct media_pad *pad); > > +/** > + * media_entity_remote_pad_unique - Find a remote pad connected to an entity > + * @entity: The entity > + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) > + * > + * Search for and return a remote pad of @type connected to @entity through an > + * enabled link. If multiple (or no) remote pads match these criteria, an error > + * is returned. > + * > + * The uniqueness constraint makes this helper function suitable for entities > + * that support a single active source or sink at a time. > + * > + * Return: A pointer to the remote pad, or one of the following error pointers > + * if an error occurs: > + * > + * * -ENOTUNIQ - Multiple links are enabled > + * * -ENOLINK - No connected pad found > + */ > +struct media_pad * > +media_entity_remote_pad_unique(const struct media_entity *entity, > + unsigned int type); > + > +/** > + * media_entity_remote_source_pad - Find a remote source pad connected to an entity > + * @entity: The entity > + * > + * Search for and return a remote source pad connected to @entity through an > + * enabled link. If multiple (or no) remote pads match these criteria, an error > + * is returned. > + * > + * The uniqueness constraint makes this helper function suitable for entities > + * that support a single active source at a time. > + * > + * Return: A pointer to the remote pad, or one of the following error pointers > + * if an error occurs: > + * > + * * -ENOTUNIQ - Multiple links are enabled > + * * -ENOLINK - No connected pad found > + */ > +static inline struct media_pad * > +media_entity_remote_source_pad(const struct media_entity *entity) > +{ > + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); > +} > + > /** > * media_entity_is_streaming - Test if an entity is part of a streaming pipeline > * @entity: The entity
Hello all On 14/06/2022 20:11, Paul Elder wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > The media_entity_remote_pad() helper function returns the first remote > pad it find connected to a given pad. Beside being possibly ill-named > (as it operates on a pad, not an entity) and non-deterministic (as it > stops at the first enabled link), the fact that it returns the first > match makes it unsuitable for drivers that need to guarantee that a > single link is enabled, for instance when an entity can process data > from one of multiple sources at a time. > > For those use cases, add a new helper function, > media_entity_remote_pad_unique(), that operates on an entity and returns > a remote pad, with a guarantee that only one link is enabled. To ease > its use in drivers, also add an inline wrapper that locates source pads > specifically. A wrapper that locates sink pads can easily be added when > needed. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Documentation/driver-api/media/mc-core.rst | 4 +- > drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ > include/media/media-entity.h | 45 ++++++++++++++++++++++ > 3 files changed, 85 insertions(+), 2 deletions(-) > > diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > index 02481a2513b9..a2d1e32e3abb 100644 > --- a/Documentation/driver-api/media/mc-core.rst > +++ b/Documentation/driver-api/media/mc-core.rst > @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. > > Helper functions can be used to find a link between two given pads, or a pad > connected to another pad through an enabled link > -:c:func:`media_entity_find_link()` and > -:c:func:`media_entity_remote_pad()`. > +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and > +:c:func:`media_entity_remote_source_pad()`). > > Use count and power handling > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > index 11f5207f73aa..1febf5a86be6 100644 > --- a/drivers/media/mc/mc-entity.c > +++ b/drivers/media/mc/mc-entity.c > @@ -9,6 +9,7 @@ > */ > > #include <linux/bitmap.h> > +#include <linux/list.h> > #include <linux/property.h> > #include <linux/slab.h> > #include <media/media-entity.h> > @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) > } > EXPORT_SYMBOL_GPL(media_entity_remote_pad); > > +struct media_pad * > +media_entity_remote_pad_unique(const struct media_entity *entity, > + unsigned int type) > +{ > + struct media_pad *pad = NULL; > + struct media_link *link; > + > + list_for_each_entry(link, &entity->links, list) { > + struct media_pad *local_pad; > + struct media_pad *remote_pad; > + > + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) > + continue; Does this need another guard here to make sure the link isn't an ancillary link? Only likely to happen at least in the immediate future where the entity represents a camera sensor, so possibly not applicable here - I couldn't find where the new function is used in the series to check. I _think_ it would actually work ok regardless...media_gobj type-punning makes my brain ache, but I think the local_pad->entity == entity comparison would actually compare the entity->name member of the entity at the end of an ancillary link to the entity parameter, not find a match and so continue the loop without failing, but that feels a bit sub-optimal. > + > + if (type == MEDIA_PAD_FL_SOURCE) { > + local_pad = link->sink; > + remote_pad = link->source; > + } else { > + local_pad = link->source; > + remote_pad = link->sink; > + } > + > + if (local_pad->entity == entity) { > + if (pad) > + return ERR_PTR(-ENOTUNIQ); > + > + pad = remote_pad; > + } > + } > + > + if (!pad) > + return ERR_PTR(-ENOLINK); > + > + return pad; > +} > +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); > + > static void media_interface_init(struct media_device *mdev, > struct media_interface *intf, > u32 gobj_type, > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > index a9a1c0ec5d1c..33d5f52719a0 100644 > --- a/include/media/media-entity.h > +++ b/include/media/media-entity.h > @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, > */ > struct media_pad *media_entity_remote_pad(const struct media_pad *pad); > > +/** > + * media_entity_remote_pad_unique - Find a remote pad connected to an entity > + * @entity: The entity > + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) > + * > + * Search for and return a remote pad of @type connected to @entity through an > + * enabled link. If multiple (or no) remote pads match these criteria, an error > + * is returned. > + * > + * The uniqueness constraint makes this helper function suitable for entities > + * that support a single active source or sink at a time. > + * > + * Return: A pointer to the remote pad, or one of the following error pointers > + * if an error occurs: > + * > + * * -ENOTUNIQ - Multiple links are enabled > + * * -ENOLINK - No connected pad found > + */ > +struct media_pad * > +media_entity_remote_pad_unique(const struct media_entity *entity, > + unsigned int type); > + > +/** > + * media_entity_remote_source_pad - Find a remote source pad connected to an entity > + * @entity: The entity > + * > + * Search for and return a remote source pad connected to @entity through an > + * enabled link. If multiple (or no) remote pads match these criteria, an error > + * is returned. > + * > + * The uniqueness constraint makes this helper function suitable for entities > + * that support a single active source at a time. > + * > + * Return: A pointer to the remote pad, or one of the following error pointers > + * if an error occurs: > + * > + * * -ENOTUNIQ - Multiple links are enabled > + * * -ENOLINK - No connected pad found > + */ > +static inline struct media_pad * > +media_entity_remote_source_pad(const struct media_entity *entity) > +{ > + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); > +} > + > /** > * media_entity_is_streaming - Test if an entity is part of a streaming pipeline > * @entity: The entity
On 17/06/2022 22:34, Daniel Scally wrote: > Hello all > > On 14/06/2022 20:11, Paul Elder wrote: >> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> The media_entity_remote_pad() helper function returns the first remote >> pad it find connected to a given pad. Beside being possibly ill-named >> (as it operates on a pad, not an entity) and non-deterministic (as it >> stops at the first enabled link), the fact that it returns the first >> match makes it unsuitable for drivers that need to guarantee that a >> single link is enabled, for instance when an entity can process data >> from one of multiple sources at a time. >> >> For those use cases, add a new helper function, >> media_entity_remote_pad_unique(), that operates on an entity and returns >> a remote pad, with a guarantee that only one link is enabled. To ease >> its use in drivers, also add an inline wrapper that locates source pads >> specifically. A wrapper that locates sink pads can easily be added when >> needed. >> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> --- >> Documentation/driver-api/media/mc-core.rst | 4 +- >> drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ >> include/media/media-entity.h | 45 ++++++++++++++++++++++ >> 3 files changed, 85 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst >> index 02481a2513b9..a2d1e32e3abb 100644 >> --- a/Documentation/driver-api/media/mc-core.rst >> +++ b/Documentation/driver-api/media/mc-core.rst >> @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. >> >> Helper functions can be used to find a link between two given pads, or a pad >> connected to another pad through an enabled link >> -:c:func:`media_entity_find_link()` and >> -:c:func:`media_entity_remote_pad()`. >> +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and >> +:c:func:`media_entity_remote_source_pad()`). >> >> Use count and power handling >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c >> index 11f5207f73aa..1febf5a86be6 100644 >> --- a/drivers/media/mc/mc-entity.c >> +++ b/drivers/media/mc/mc-entity.c >> @@ -9,6 +9,7 @@ >> */ >> >> #include <linux/bitmap.h> >> +#include <linux/list.h> >> #include <linux/property.h> >> #include <linux/slab.h> >> #include <media/media-entity.h> >> @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) >> } >> EXPORT_SYMBOL_GPL(media_entity_remote_pad); >> >> +struct media_pad * >> +media_entity_remote_pad_unique(const struct media_entity *entity, >> + unsigned int type) >> +{ >> + struct media_pad *pad = NULL; >> + struct media_link *link; >> + >> + list_for_each_entry(link, &entity->links, list) { >> + struct media_pad *local_pad; >> + struct media_pad *remote_pad; >> + >> + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) >> + continue; > Does this need another guard here to make sure the link isn't an > ancillary link? Only likely to happen at least in the immediate future > where the entity represents a camera sensor, so possibly not applicable > here - I couldn't find where the new function is used in the series to > check. I _think_ it would actually work ok regardless...media_gobj > type-punning makes my brain ache, but I think the local_pad->entity == > entity comparison would actually compare the entity->name member of the > entity at the end of an ancillary link to the entity parameter, not find > a match and so continue the loop without failing, but that feels a bit > sub-optimal. > Or perhaps a better approach would be to provide something like a "list_for_each_data_link()" iterator - the potential problem here (as with Quentin's recent issue with the rkisp1 driver) is the assumption that all links are data links, so maybe it's best to just guarantee that if we can. > >> + >> + if (type == MEDIA_PAD_FL_SOURCE) { >> + local_pad = link->sink; >> + remote_pad = link->source; >> + } else { >> + local_pad = link->source; >> + remote_pad = link->sink; >> + } >> + >> + if (local_pad->entity == entity) { >> + if (pad) >> + return ERR_PTR(-ENOTUNIQ); >> + >> + pad = remote_pad; >> + } >> + } >> + >> + if (!pad) >> + return ERR_PTR(-ENOLINK); >> + >> + return pad; >> +} >> +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); >> + >> static void media_interface_init(struct media_device *mdev, >> struct media_interface *intf, >> u32 gobj_type, >> diff --git a/include/media/media-entity.h b/include/media/media-entity.h >> index a9a1c0ec5d1c..33d5f52719a0 100644 >> --- a/include/media/media-entity.h >> +++ b/include/media/media-entity.h >> @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, >> */ >> struct media_pad *media_entity_remote_pad(const struct media_pad *pad); >> >> +/** >> + * media_entity_remote_pad_unique - Find a remote pad connected to an entity >> + * @entity: The entity >> + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) >> + * >> + * Search for and return a remote pad of @type connected to @entity through an >> + * enabled link. If multiple (or no) remote pads match these criteria, an error >> + * is returned. >> + * >> + * The uniqueness constraint makes this helper function suitable for entities >> + * that support a single active source or sink at a time. >> + * >> + * Return: A pointer to the remote pad, or one of the following error pointers >> + * if an error occurs: >> + * >> + * * -ENOTUNIQ - Multiple links are enabled >> + * * -ENOLINK - No connected pad found >> + */ >> +struct media_pad * >> +media_entity_remote_pad_unique(const struct media_entity *entity, >> + unsigned int type); >> + >> +/** >> + * media_entity_remote_source_pad - Find a remote source pad connected to an entity >> + * @entity: The entity >> + * >> + * Search for and return a remote source pad connected to @entity through an >> + * enabled link. If multiple (or no) remote pads match these criteria, an error >> + * is returned. >> + * >> + * The uniqueness constraint makes this helper function suitable for entities >> + * that support a single active source at a time. >> + * >> + * Return: A pointer to the remote pad, or one of the following error pointers >> + * if an error occurs: >> + * >> + * * -ENOTUNIQ - Multiple links are enabled >> + * * -ENOLINK - No connected pad found >> + */ >> +static inline struct media_pad * >> +media_entity_remote_source_pad(const struct media_entity *entity) >> +{ >> + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); >> +} >> + >> /** >> * media_entity_is_streaming - Test if an entity is part of a streaming pipeline >> * @entity: The entity
Hi Daniel, On Fri, Jun 17, 2022 at 11:33:03PM +0100, Daniel Scally wrote: > On 17/06/2022 22:34, Daniel Scally wrote: > > On 14/06/2022 20:11, Paul Elder wrote: > >> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> > >> The media_entity_remote_pad() helper function returns the first remote > >> pad it find connected to a given pad. Beside being possibly ill-named > >> (as it operates on a pad, not an entity) and non-deterministic (as it > >> stops at the first enabled link), the fact that it returns the first > >> match makes it unsuitable for drivers that need to guarantee that a > >> single link is enabled, for instance when an entity can process data > >> from one of multiple sources at a time. > >> > >> For those use cases, add a new helper function, > >> media_entity_remote_pad_unique(), that operates on an entity and returns > >> a remote pad, with a guarantee that only one link is enabled. To ease > >> its use in drivers, also add an inline wrapper that locates source pads > >> specifically. A wrapper that locates sink pads can easily be added when > >> needed. > >> > >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> --- > >> Documentation/driver-api/media/mc-core.rst | 4 +- > >> drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ > >> include/media/media-entity.h | 45 ++++++++++++++++++++++ > >> 3 files changed, 85 insertions(+), 2 deletions(-) > >> > >> diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > >> index 02481a2513b9..a2d1e32e3abb 100644 > >> --- a/Documentation/driver-api/media/mc-core.rst > >> +++ b/Documentation/driver-api/media/mc-core.rst > >> @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. > >> > >> Helper functions can be used to find a link between two given pads, or a pad > >> connected to another pad through an enabled link > >> -:c:func:`media_entity_find_link()` and > >> -:c:func:`media_entity_remote_pad()`. > >> +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and > >> +:c:func:`media_entity_remote_source_pad()`). > >> > >> Use count and power handling > >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > >> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > >> index 11f5207f73aa..1febf5a86be6 100644 > >> --- a/drivers/media/mc/mc-entity.c > >> +++ b/drivers/media/mc/mc-entity.c > >> @@ -9,6 +9,7 @@ > >> */ > >> > >> #include <linux/bitmap.h> > >> +#include <linux/list.h> > >> #include <linux/property.h> > >> #include <linux/slab.h> > >> #include <media/media-entity.h> > >> @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) > >> } > >> EXPORT_SYMBOL_GPL(media_entity_remote_pad); > >> > >> +struct media_pad * > >> +media_entity_remote_pad_unique(const struct media_entity *entity, > >> + unsigned int type) > >> +{ > >> + struct media_pad *pad = NULL; > >> + struct media_link *link; > >> + > >> + list_for_each_entry(link, &entity->links, list) { > >> + struct media_pad *local_pad; > >> + struct media_pad *remote_pad; > >> + > >> + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) > >> + continue; > > > > Does this need another guard here to make sure the link isn't an > > ancillary link? Only likely to happen at least in the immediate future > > where the entity represents a camera sensor, so possibly not applicable > > here - I couldn't find where the new function is used in the series to > > check. I _think_ it would actually work ok regardless...media_gobj > > type-punning makes my brain ache, but I think the local_pad->entity == > > entity comparison would actually compare the entity->name member of the > > entity at the end of an ancillary link to the entity parameter, not find > > a match and so continue the loop without failing, but that feels a bit > > sub-optimal. > > Or perhaps a better approach would be to provide something like a > "list_for_each_data_link()" iterator - the potential problem here (as > with Quentin's recent issue with the rkisp1 driver) is the assumption > that all links are data links, so maybe it's best to just guarantee that > if we can. I agree with you, without a dedicated iterator, we're bound to repeat the mistake over and over. Would you like to submit a patch to add that iterator, or should I ? I'd name it for_each_media_entity_data_link() or something similar. > >> + > >> + if (type == MEDIA_PAD_FL_SOURCE) { > >> + local_pad = link->sink; > >> + remote_pad = link->source; > >> + } else { > >> + local_pad = link->source; > >> + remote_pad = link->sink; > >> + } > >> + > >> + if (local_pad->entity == entity) { > >> + if (pad) > >> + return ERR_PTR(-ENOTUNIQ); > >> + > >> + pad = remote_pad; > >> + } > >> + } > >> + > >> + if (!pad) > >> + return ERR_PTR(-ENOLINK); > >> + > >> + return pad; > >> +} > >> +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); > >> + > >> static void media_interface_init(struct media_device *mdev, > >> struct media_interface *intf, > >> u32 gobj_type, > >> diff --git a/include/media/media-entity.h b/include/media/media-entity.h > >> index a9a1c0ec5d1c..33d5f52719a0 100644 > >> --- a/include/media/media-entity.h > >> +++ b/include/media/media-entity.h > >> @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, > >> */ > >> struct media_pad *media_entity_remote_pad(const struct media_pad *pad); > >> > >> +/** > >> + * media_entity_remote_pad_unique - Find a remote pad connected to an entity > >> + * @entity: The entity > >> + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) > >> + * > >> + * Search for and return a remote pad of @type connected to @entity through an > >> + * enabled link. If multiple (or no) remote pads match these criteria, an error > >> + * is returned. > >> + * > >> + * The uniqueness constraint makes this helper function suitable for entities > >> + * that support a single active source or sink at a time. > >> + * > >> + * Return: A pointer to the remote pad, or one of the following error pointers > >> + * if an error occurs: > >> + * > >> + * * -ENOTUNIQ - Multiple links are enabled > >> + * * -ENOLINK - No connected pad found > >> + */ > >> +struct media_pad * > >> +media_entity_remote_pad_unique(const struct media_entity *entity, > >> + unsigned int type); > >> + > >> +/** > >> + * media_entity_remote_source_pad - Find a remote source pad connected to an entity > >> + * @entity: The entity > >> + * > >> + * Search for and return a remote source pad connected to @entity through an > >> + * enabled link. If multiple (or no) remote pads match these criteria, an error > >> + * is returned. > >> + * > >> + * The uniqueness constraint makes this helper function suitable for entities > >> + * that support a single active source at a time. > >> + * > >> + * Return: A pointer to the remote pad, or one of the following error pointers > >> + * if an error occurs: > >> + * > >> + * * -ENOTUNIQ - Multiple links are enabled > >> + * * -ENOLINK - No connected pad found > >> + */ > >> +static inline struct media_pad * > >> +media_entity_remote_source_pad(const struct media_entity *entity) > >> +{ > >> + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); > >> +} > >> + > >> /** > >> * media_entity_is_streaming - Test if an entity is part of a streaming pipeline > >> * @entity: The entity
Morning Laurent On 17/06/2022 23:40, Laurent Pinchart wrote: > Hi Daniel, > > On Fri, Jun 17, 2022 at 11:33:03PM +0100, Daniel Scally wrote: >> On 17/06/2022 22:34, Daniel Scally wrote: >>> On 14/06/2022 20:11, Paul Elder wrote: >>>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>> >>>> The media_entity_remote_pad() helper function returns the first remote >>>> pad it find connected to a given pad. Beside being possibly ill-named >>>> (as it operates on a pad, not an entity) and non-deterministic (as it >>>> stops at the first enabled link), the fact that it returns the first >>>> match makes it unsuitable for drivers that need to guarantee that a >>>> single link is enabled, for instance when an entity can process data >>>> from one of multiple sources at a time. >>>> >>>> For those use cases, add a new helper function, >>>> media_entity_remote_pad_unique(), that operates on an entity and returns >>>> a remote pad, with a guarantee that only one link is enabled. To ease >>>> its use in drivers, also add an inline wrapper that locates source pads >>>> specifically. A wrapper that locates sink pads can easily be added when >>>> needed. >>>> >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>> --- >>>> Documentation/driver-api/media/mc-core.rst | 4 +- >>>> drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ >>>> include/media/media-entity.h | 45 ++++++++++++++++++++++ >>>> 3 files changed, 85 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst >>>> index 02481a2513b9..a2d1e32e3abb 100644 >>>> --- a/Documentation/driver-api/media/mc-core.rst >>>> +++ b/Documentation/driver-api/media/mc-core.rst >>>> @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. >>>> >>>> Helper functions can be used to find a link between two given pads, or a pad >>>> connected to another pad through an enabled link >>>> -:c:func:`media_entity_find_link()` and >>>> -:c:func:`media_entity_remote_pad()`. >>>> +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and >>>> +:c:func:`media_entity_remote_source_pad()`). >>>> >>>> Use count and power handling >>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c >>>> index 11f5207f73aa..1febf5a86be6 100644 >>>> --- a/drivers/media/mc/mc-entity.c >>>> +++ b/drivers/media/mc/mc-entity.c >>>> @@ -9,6 +9,7 @@ >>>> */ >>>> >>>> #include <linux/bitmap.h> >>>> +#include <linux/list.h> >>>> #include <linux/property.h> >>>> #include <linux/slab.h> >>>> #include <media/media-entity.h> >>>> @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) >>>> } >>>> EXPORT_SYMBOL_GPL(media_entity_remote_pad); >>>> >>>> +struct media_pad * >>>> +media_entity_remote_pad_unique(const struct media_entity *entity, >>>> + unsigned int type) >>>> +{ >>>> + struct media_pad *pad = NULL; >>>> + struct media_link *link; >>>> + >>>> + list_for_each_entry(link, &entity->links, list) { >>>> + struct media_pad *local_pad; >>>> + struct media_pad *remote_pad; >>>> + >>>> + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) >>>> + continue; >>> Does this need another guard here to make sure the link isn't an >>> ancillary link? Only likely to happen at least in the immediate future >>> where the entity represents a camera sensor, so possibly not applicable >>> here - I couldn't find where the new function is used in the series to >>> check. I _think_ it would actually work ok regardless...media_gobj >>> type-punning makes my brain ache, but I think the local_pad->entity == >>> entity comparison would actually compare the entity->name member of the >>> entity at the end of an ancillary link to the entity parameter, not find >>> a match and so continue the loop without failing, but that feels a bit >>> sub-optimal. >> Or perhaps a better approach would be to provide something like a >> "list_for_each_data_link()" iterator - the potential problem here (as >> with Quentin's recent issue with the rkisp1 driver) is the assumption >> that all links are data links, so maybe it's best to just guarantee that >> if we can. > I agree with you, without a dedicated iterator, we're bound to repeat > the mistake over and over. > > Would you like to submit a patch to add that iterator, or should I ? I'd > name it for_each_media_entity_data_link() or something similar. I've started one - I'll send it later (using your function name though, naming things was never my strong suit!) > >>>> + >>>> + if (type == MEDIA_PAD_FL_SOURCE) { >>>> + local_pad = link->sink; >>>> + remote_pad = link->source; >>>> + } else { >>>> + local_pad = link->source; >>>> + remote_pad = link->sink; >>>> + } >>>> + >>>> + if (local_pad->entity == entity) { >>>> + if (pad) >>>> + return ERR_PTR(-ENOTUNIQ); >>>> + >>>> + pad = remote_pad; >>>> + } >>>> + } >>>> + >>>> + if (!pad) >>>> + return ERR_PTR(-ENOLINK); >>>> + >>>> + return pad; >>>> +} >>>> +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); >>>> + >>>> static void media_interface_init(struct media_device *mdev, >>>> struct media_interface *intf, >>>> u32 gobj_type, >>>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h >>>> index a9a1c0ec5d1c..33d5f52719a0 100644 >>>> --- a/include/media/media-entity.h >>>> +++ b/include/media/media-entity.h >>>> @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, >>>> */ >>>> struct media_pad *media_entity_remote_pad(const struct media_pad *pad); >>>> >>>> +/** >>>> + * media_entity_remote_pad_unique - Find a remote pad connected to an entity >>>> + * @entity: The entity >>>> + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) >>>> + * >>>> + * Search for and return a remote pad of @type connected to @entity through an >>>> + * enabled link. If multiple (or no) remote pads match these criteria, an error >>>> + * is returned. >>>> + * >>>> + * The uniqueness constraint makes this helper function suitable for entities >>>> + * that support a single active source or sink at a time. >>>> + * >>>> + * Return: A pointer to the remote pad, or one of the following error pointers >>>> + * if an error occurs: >>>> + * >>>> + * * -ENOTUNIQ - Multiple links are enabled >>>> + * * -ENOLINK - No connected pad found >>>> + */ >>>> +struct media_pad * >>>> +media_entity_remote_pad_unique(const struct media_entity *entity, >>>> + unsigned int type); >>>> + >>>> +/** >>>> + * media_entity_remote_source_pad - Find a remote source pad connected to an entity >>>> + * @entity: The entity >>>> + * >>>> + * Search for and return a remote source pad connected to @entity through an >>>> + * enabled link. If multiple (or no) remote pads match these criteria, an error >>>> + * is returned. >>>> + * >>>> + * The uniqueness constraint makes this helper function suitable for entities >>>> + * that support a single active source at a time. >>>> + * >>>> + * Return: A pointer to the remote pad, or one of the following error pointers >>>> + * if an error occurs: >>>> + * >>>> + * * -ENOTUNIQ - Multiple links are enabled >>>> + * * -ENOLINK - No connected pad found >>>> + */ >>>> +static inline struct media_pad * >>>> +media_entity_remote_source_pad(const struct media_entity *entity) >>>> +{ >>>> + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); >>>> +} >>>> + >>>> /** >>>> * media_entity_is_streaming - Test if an entity is part of a streaming pipeline >>>> * @entity: The entity
Hi Hans, On Fri, Jun 17, 2022 at 01:48:05PM +0200, Hans Verkuil wrote: > On 6/14/22 21:11, Paul Elder wrote: > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > The media_entity_remote_pad() helper function returns the first remote > > pad it find connected to a given pad. Beside being possibly ill-named > > (as it operates on a pad, not an entity) and non-deterministic (as it > > stops at the first enabled link), the fact that it returns the first > > match makes it unsuitable for drivers that need to guarantee that a > > single link is enabled, for instance when an entity can process data > > from one of multiple sources at a time. > > Question: of all the callers of this function, are there any that really > need media_entity_remote_pad() instead of media_pad_remote_pad_unique()? > > Would it be possible to replace all callers of the old function with the > new function? If that's the case, then the _unique suffix can be dropped, > since that would effectively be the default. And if a function is needed > to handle the case where there are multiple enabled links, then a new > function should be created. I don't think so. media_entity_remote_pad() operates on a pad, switching to media_pad_remote_pad_unique() wouldn't work on subdevs that have multiple sink or source pads with one active link each. > Also, media_entity_remote_pad() should really be renamed to > media_pad_remote_pad_first() or something like that, right? I'm not saying > you should, but that's really what it does, as I understand it. Yes, I think that would make sense, and it would freethe media_entity_remote_pad() name, so the new function wouldn't need the _unique suffix. I'll give it a try. > > For those use cases, add a new helper function, > > media_entity_remote_pad_unique(), that operates on an entity and returns > > a remote pad, with a guarantee that only one link is enabled. To ease > > its use in drivers, also add an inline wrapper that locates source pads > > specifically. A wrapper that locates sink pads can easily be added when > > needed. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > Documentation/driver-api/media/mc-core.rst | 4 +- > > drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ > > include/media/media-entity.h | 45 ++++++++++++++++++++++ > > 3 files changed, 85 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > > index 02481a2513b9..a2d1e32e3abb 100644 > > --- a/Documentation/driver-api/media/mc-core.rst > > +++ b/Documentation/driver-api/media/mc-core.rst > > @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. > > > > Helper functions can be used to find a link between two given pads, or a pad > > connected to another pad through an enabled link > > -:c:func:`media_entity_find_link()` and > > -:c:func:`media_entity_remote_pad()`. > > +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and > > +:c:func:`media_entity_remote_source_pad()`). > > > > Use count and power handling > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > > index 11f5207f73aa..1febf5a86be6 100644 > > --- a/drivers/media/mc/mc-entity.c > > +++ b/drivers/media/mc/mc-entity.c > > @@ -9,6 +9,7 @@ > > */ > > > > #include <linux/bitmap.h> > > +#include <linux/list.h> > > #include <linux/property.h> > > #include <linux/slab.h> > > #include <media/media-entity.h> > > @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) > > } > > EXPORT_SYMBOL_GPL(media_entity_remote_pad); > > > > +struct media_pad * > > +media_entity_remote_pad_unique(const struct media_entity *entity, > > + unsigned int type) > > +{ > > + struct media_pad *pad = NULL; > > + struct media_link *link; > > + > > + list_for_each_entry(link, &entity->links, list) { > > + struct media_pad *local_pad; > > + struct media_pad *remote_pad; > > + > > + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) > > + continue; > > + > > + if (type == MEDIA_PAD_FL_SOURCE) { > > + local_pad = link->sink; > > + remote_pad = link->source; > > + } else { > > + local_pad = link->source; > > + remote_pad = link->sink; > > + } > > + > > + if (local_pad->entity == entity) { > > + if (pad) > > + return ERR_PTR(-ENOTUNIQ); > > + > > + pad = remote_pad; > > + } > > + } > > + > > + if (!pad) > > + return ERR_PTR(-ENOLINK); > > + > > + return pad; > > +} > > +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); > > + > > static void media_interface_init(struct media_device *mdev, > > struct media_interface *intf, > > u32 gobj_type, > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > > index a9a1c0ec5d1c..33d5f52719a0 100644 > > --- a/include/media/media-entity.h > > +++ b/include/media/media-entity.h > > @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, > > */ > > struct media_pad *media_entity_remote_pad(const struct media_pad *pad); > > > > +/** > > + * media_entity_remote_pad_unique - Find a remote pad connected to an entity > > + * @entity: The entity > > + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) > > + * > > + * Search for and return a remote pad of @type connected to @entity through an > > + * enabled link. If multiple (or no) remote pads match these criteria, an error > > + * is returned. > > + * > > + * The uniqueness constraint makes this helper function suitable for entities > > + * that support a single active source or sink at a time. > > + * > > + * Return: A pointer to the remote pad, or one of the following error pointers > > + * if an error occurs: > > + * > > + * * -ENOTUNIQ - Multiple links are enabled > > + * * -ENOLINK - No connected pad found > > + */ > > +struct media_pad * > > +media_entity_remote_pad_unique(const struct media_entity *entity, > > + unsigned int type); > > + > > +/** > > + * media_entity_remote_source_pad - Find a remote source pad connected to an entity > > + * @entity: The entity > > + * > > + * Search for and return a remote source pad connected to @entity through an > > + * enabled link. If multiple (or no) remote pads match these criteria, an error > > + * is returned. > > + * > > + * The uniqueness constraint makes this helper function suitable for entities > > + * that support a single active source at a time. > > + * > > + * Return: A pointer to the remote pad, or one of the following error pointers > > + * if an error occurs: > > + * > > + * * -ENOTUNIQ - Multiple links are enabled > > + * * -ENOLINK - No connected pad found > > + */ > > +static inline struct media_pad * > > +media_entity_remote_source_pad(const struct media_entity *entity) > > +{ > > + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); > > +} > > + > > /** > > * media_entity_is_streaming - Test if an entity is part of a streaming pipeline > > * @entity: The entity
Hi Hans, On Sat, Jun 25, 2022 at 08:00:51PM +0300, Laurent Pinchart wrote: > On Fri, Jun 17, 2022 at 01:48:05PM +0200, Hans Verkuil wrote: > > On 6/14/22 21:11, Paul Elder wrote: > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > The media_entity_remote_pad() helper function returns the first remote > > > pad it find connected to a given pad. Beside being possibly ill-named > > > (as it operates on a pad, not an entity) and non-deterministic (as it > > > stops at the first enabled link), the fact that it returns the first > > > match makes it unsuitable for drivers that need to guarantee that a > > > single link is enabled, for instance when an entity can process data > > > from one of multiple sources at a time. > > > > Question: of all the callers of this function, are there any that really > > need media_entity_remote_pad() instead of media_pad_remote_pad_unique()? > > > > Would it be possible to replace all callers of the old function with the > > new function? If that's the case, then the _unique suffix can be dropped, > > since that would effectively be the default. And if a function is needed > > to handle the case where there are multiple enabled links, then a new > > function should be created. > > I don't think so. media_entity_remote_pad() operates on a pad, switching > to media_pad_remote_pad_unique() wouldn't work on subdevs that have > multiple sink or source pads with one active link each. > > > Also, media_entity_remote_pad() should really be renamed to > > media_pad_remote_pad_first() or something like that, right? I'm not saying > > you should, but that's really what it does, as I understand it. > > Yes, I think that would make sense, and it would freethe > media_entity_remote_pad() name, so the new function wouldn't need the > _unique suffix. I'll give it a try. The rename was easy enough, but I've decided to keep the media_entity_remote_pad_unique() name and renamed media_entity_remote_source_pad() to media_entity_remote_source_pad_unique() to make the API clearer. If those names end up being too long, we can rename them later. > > > For those use cases, add a new helper function, > > > media_entity_remote_pad_unique(), that operates on an entity and returns > > > a remote pad, with a guarantee that only one link is enabled. To ease > > > its use in drivers, also add an inline wrapper that locates source pads > > > specifically. A wrapper that locates sink pads can easily be added when > > > needed. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > Documentation/driver-api/media/mc-core.rst | 4 +- > > > drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ > > > include/media/media-entity.h | 45 ++++++++++++++++++++++ > > > 3 files changed, 85 insertions(+), 2 deletions(-) > > > > > > diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > > > index 02481a2513b9..a2d1e32e3abb 100644 > > > --- a/Documentation/driver-api/media/mc-core.rst > > > +++ b/Documentation/driver-api/media/mc-core.rst > > > @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. > > > > > > Helper functions can be used to find a link between two given pads, or a pad > > > connected to another pad through an enabled link > > > -:c:func:`media_entity_find_link()` and > > > -:c:func:`media_entity_remote_pad()`. > > > +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and > > > +:c:func:`media_entity_remote_source_pad()`). > > > > > > Use count and power handling > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > > > index 11f5207f73aa..1febf5a86be6 100644 > > > --- a/drivers/media/mc/mc-entity.c > > > +++ b/drivers/media/mc/mc-entity.c > > > @@ -9,6 +9,7 @@ > > > */ > > > > > > #include <linux/bitmap.h> > > > +#include <linux/list.h> > > > #include <linux/property.h> > > > #include <linux/slab.h> > > > #include <media/media-entity.h> > > > @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) > > > } > > > EXPORT_SYMBOL_GPL(media_entity_remote_pad); > > > > > > +struct media_pad * > > > +media_entity_remote_pad_unique(const struct media_entity *entity, > > > + unsigned int type) > > > +{ > > > + struct media_pad *pad = NULL; > > > + struct media_link *link; > > > + > > > + list_for_each_entry(link, &entity->links, list) { > > > + struct media_pad *local_pad; > > > + struct media_pad *remote_pad; > > > + > > > + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) > > > + continue; > > > + > > > + if (type == MEDIA_PAD_FL_SOURCE) { > > > + local_pad = link->sink; > > > + remote_pad = link->source; > > > + } else { > > > + local_pad = link->source; > > > + remote_pad = link->sink; > > > + } > > > + > > > + if (local_pad->entity == entity) { > > > + if (pad) > > > + return ERR_PTR(-ENOTUNIQ); > > > + > > > + pad = remote_pad; > > > + } > > > + } > > > + > > > + if (!pad) > > > + return ERR_PTR(-ENOLINK); > > > + > > > + return pad; > > > +} > > > +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); > > > + > > > static void media_interface_init(struct media_device *mdev, > > > struct media_interface *intf, > > > u32 gobj_type, > > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > > > index a9a1c0ec5d1c..33d5f52719a0 100644 > > > --- a/include/media/media-entity.h > > > +++ b/include/media/media-entity.h > > > @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, > > > */ > > > struct media_pad *media_entity_remote_pad(const struct media_pad *pad); > > > > > > +/** > > > + * media_entity_remote_pad_unique - Find a remote pad connected to an entity > > > + * @entity: The entity > > > + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) > > > + * > > > + * Search for and return a remote pad of @type connected to @entity through an > > > + * enabled link. If multiple (or no) remote pads match these criteria, an error > > > + * is returned. > > > + * > > > + * The uniqueness constraint makes this helper function suitable for entities > > > + * that support a single active source or sink at a time. > > > + * > > > + * Return: A pointer to the remote pad, or one of the following error pointers > > > + * if an error occurs: > > > + * > > > + * * -ENOTUNIQ - Multiple links are enabled > > > + * * -ENOLINK - No connected pad found > > > + */ > > > +struct media_pad * > > > +media_entity_remote_pad_unique(const struct media_entity *entity, > > > + unsigned int type); > > > + > > > +/** > > > + * media_entity_remote_source_pad - Find a remote source pad connected to an entity > > > + * @entity: The entity > > > + * > > > + * Search for and return a remote source pad connected to @entity through an > > > + * enabled link. If multiple (or no) remote pads match these criteria, an error > > > + * is returned. > > > + * > > > + * The uniqueness constraint makes this helper function suitable for entities > > > + * that support a single active source at a time. > > > + * > > > + * Return: A pointer to the remote pad, or one of the following error pointers > > > + * if an error occurs: > > > + * > > > + * * -ENOTUNIQ - Multiple links are enabled > > > + * * -ENOLINK - No connected pad found > > > + */ > > > +static inline struct media_pad * > > > +media_entity_remote_source_pad(const struct media_entity *entity) > > > +{ > > > + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); > > > +} > > > + > > > /** > > > * media_entity_is_streaming - Test if an entity is part of a streaming pipeline > > > * @entity: The entity
Hi Dan, On Sat, Jun 18, 2022 at 10:35:12AM +0100, Daniel Scally wrote: > On 17/06/2022 23:40, Laurent Pinchart wrote: > > On Fri, Jun 17, 2022 at 11:33:03PM +0100, Daniel Scally wrote: > >> On 17/06/2022 22:34, Daniel Scally wrote: > >>> On 14/06/2022 20:11, Paul Elder wrote: > >>>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>> > >>>> The media_entity_remote_pad() helper function returns the first remote > >>>> pad it find connected to a given pad. Beside being possibly ill-named > >>>> (as it operates on a pad, not an entity) and non-deterministic (as it > >>>> stops at the first enabled link), the fact that it returns the first > >>>> match makes it unsuitable for drivers that need to guarantee that a > >>>> single link is enabled, for instance when an entity can process data > >>>> from one of multiple sources at a time. > >>>> > >>>> For those use cases, add a new helper function, > >>>> media_entity_remote_pad_unique(), that operates on an entity and returns > >>>> a remote pad, with a guarantee that only one link is enabled. To ease > >>>> its use in drivers, also add an inline wrapper that locates source pads > >>>> specifically. A wrapper that locates sink pads can easily be added when > >>>> needed. > >>>> > >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>> --- > >>>> Documentation/driver-api/media/mc-core.rst | 4 +- > >>>> drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ > >>>> include/media/media-entity.h | 45 ++++++++++++++++++++++ > >>>> 3 files changed, 85 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > >>>> index 02481a2513b9..a2d1e32e3abb 100644 > >>>> --- a/Documentation/driver-api/media/mc-core.rst > >>>> +++ b/Documentation/driver-api/media/mc-core.rst > >>>> @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. > >>>> > >>>> Helper functions can be used to find a link between two given pads, or a pad > >>>> connected to another pad through an enabled link > >>>> -:c:func:`media_entity_find_link()` and > >>>> -:c:func:`media_entity_remote_pad()`. > >>>> +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and > >>>> +:c:func:`media_entity_remote_source_pad()`). > >>>> > >>>> Use count and power handling > >>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > >>>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > >>>> index 11f5207f73aa..1febf5a86be6 100644 > >>>> --- a/drivers/media/mc/mc-entity.c > >>>> +++ b/drivers/media/mc/mc-entity.c > >>>> @@ -9,6 +9,7 @@ > >>>> */ > >>>> > >>>> #include <linux/bitmap.h> > >>>> +#include <linux/list.h> > >>>> #include <linux/property.h> > >>>> #include <linux/slab.h> > >>>> #include <media/media-entity.h> > >>>> @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) > >>>> } > >>>> EXPORT_SYMBOL_GPL(media_entity_remote_pad); > >>>> > >>>> +struct media_pad * > >>>> +media_entity_remote_pad_unique(const struct media_entity *entity, > >>>> + unsigned int type) > >>>> +{ > >>>> + struct media_pad *pad = NULL; > >>>> + struct media_link *link; > >>>> + > >>>> + list_for_each_entry(link, &entity->links, list) { > >>>> + struct media_pad *local_pad; > >>>> + struct media_pad *remote_pad; > >>>> + > >>>> + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) > >>>> + continue; > >>> > >>> Does this need another guard here to make sure the link isn't an > >>> ancillary link? Only likely to happen at least in the immediate future > >>> where the entity represents a camera sensor, so possibly not applicable > >>> here - I couldn't find where the new function is used in the series to > >>> check. I _think_ it would actually work ok regardless...media_gobj > >>> type-punning makes my brain ache, but I think the local_pad->entity == > >>> entity comparison would actually compare the entity->name member of the > >>> entity at the end of an ancillary link to the entity parameter, not find > >>> a match and so continue the loop without failing, but that feels a bit > >>> sub-optimal. > >> > >> Or perhaps a better approach would be to provide something like a > >> "list_for_each_data_link()" iterator - the potential problem here (as > >> with Quentin's recent issue with the rkisp1 driver) is the assumption > >> that all links are data links, so maybe it's best to just guarantee that > >> if we can. > >> > > I agree with you, without a dedicated iterator, we're bound to repeat > > the mistake over and over. > > > > Would you like to submit a patch to add that iterator, or should I ? I'd > > name it for_each_media_entity_data_link() or something similar. > > I've started one - I'll send it later (using your function name though, > naming things was never my strong suit!) I've modified this patch to skip non-data links manually, to avoid depending on your series. If the iterator gets merged first, I'll update the media_entity_remote_pad_unique() helper to use it. > >>>> + > >>>> + if (type == MEDIA_PAD_FL_SOURCE) { > >>>> + local_pad = link->sink; > >>>> + remote_pad = link->source; > >>>> + } else { > >>>> + local_pad = link->source; > >>>> + remote_pad = link->sink; > >>>> + } > >>>> + > >>>> + if (local_pad->entity == entity) { > >>>> + if (pad) > >>>> + return ERR_PTR(-ENOTUNIQ); > >>>> + > >>>> + pad = remote_pad; > >>>> + } > >>>> + } > >>>> + > >>>> + if (!pad) > >>>> + return ERR_PTR(-ENOLINK); > >>>> + > >>>> + return pad; > >>>> +} > >>>> +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); > >>>> + > >>>> static void media_interface_init(struct media_device *mdev, > >>>> struct media_interface *intf, > >>>> u32 gobj_type, > >>>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h > >>>> index a9a1c0ec5d1c..33d5f52719a0 100644 > >>>> --- a/include/media/media-entity.h > >>>> +++ b/include/media/media-entity.h > >>>> @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, > >>>> */ > >>>> struct media_pad *media_entity_remote_pad(const struct media_pad *pad); > >>>> > >>>> +/** > >>>> + * media_entity_remote_pad_unique - Find a remote pad connected to an entity > >>>> + * @entity: The entity > >>>> + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) > >>>> + * > >>>> + * Search for and return a remote pad of @type connected to @entity through an > >>>> + * enabled link. If multiple (or no) remote pads match these criteria, an error > >>>> + * is returned. > >>>> + * > >>>> + * The uniqueness constraint makes this helper function suitable for entities > >>>> + * that support a single active source or sink at a time. > >>>> + * > >>>> + * Return: A pointer to the remote pad, or one of the following error pointers > >>>> + * if an error occurs: > >>>> + * > >>>> + * * -ENOTUNIQ - Multiple links are enabled > >>>> + * * -ENOLINK - No connected pad found > >>>> + */ > >>>> +struct media_pad * > >>>> +media_entity_remote_pad_unique(const struct media_entity *entity, > >>>> + unsigned int type); > >>>> + > >>>> +/** > >>>> + * media_entity_remote_source_pad - Find a remote source pad connected to an entity > >>>> + * @entity: The entity > >>>> + * > >>>> + * Search for and return a remote source pad connected to @entity through an > >>>> + * enabled link. If multiple (or no) remote pads match these criteria, an error > >>>> + * is returned. > >>>> + * > >>>> + * The uniqueness constraint makes this helper function suitable for entities > >>>> + * that support a single active source at a time. > >>>> + * > >>>> + * Return: A pointer to the remote pad, or one of the following error pointers > >>>> + * if an error occurs: > >>>> + * > >>>> + * * -ENOTUNIQ - Multiple links are enabled > >>>> + * * -ENOLINK - No connected pad found > >>>> + */ > >>>> +static inline struct media_pad * > >>>> +media_entity_remote_source_pad(const struct media_entity *entity) > >>>> +{ > >>>> + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); > >>>> +} > >>>> + > >>>> /** > >>>> * media_entity_is_streaming - Test if an entity is part of a streaming pipeline > >>>> * @entity: The entity
On 6/25/22 19:00, Laurent Pinchart wrote: > Hi Hans, > > On Fri, Jun 17, 2022 at 01:48:05PM +0200, Hans Verkuil wrote: >> On 6/14/22 21:11, Paul Elder wrote: >>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> >>> The media_entity_remote_pad() helper function returns the first remote >>> pad it find connected to a given pad. Beside being possibly ill-named >>> (as it operates on a pad, not an entity) and non-deterministic (as it >>> stops at the first enabled link), the fact that it returns the first >>> match makes it unsuitable for drivers that need to guarantee that a >>> single link is enabled, for instance when an entity can process data >>> from one of multiple sources at a time. >> >> Question: of all the callers of this function, are there any that really >> need media_entity_remote_pad() instead of media_pad_remote_pad_unique()? >> >> Would it be possible to replace all callers of the old function with the >> new function? If that's the case, then the _unique suffix can be dropped, >> since that would effectively be the default. And if a function is needed >> to handle the case where there are multiple enabled links, then a new >> function should be created. > > I don't think so. media_entity_remote_pad() operates on a pad, switching > to media_pad_remote_pad_unique() wouldn't work on subdevs that have > multiple sink or source pads with one active link each. Do we have those today in the mainline kernel? Just checking... Regards, Hans > >> Also, media_entity_remote_pad() should really be renamed to >> media_pad_remote_pad_first() or something like that, right? I'm not saying >> you should, but that's really what it does, as I understand it. > > Yes, I think that would make sense, and it would freethe > media_entity_remote_pad() name, so the new function wouldn't need the > _unique suffix. I'll give it a try. > >>> For those use cases, add a new helper function, >>> media_entity_remote_pad_unique(), that operates on an entity and returns >>> a remote pad, with a guarantee that only one link is enabled. To ease >>> its use in drivers, also add an inline wrapper that locates source pads >>> specifically. A wrapper that locates sink pads can easily be added when >>> needed. >>> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> Documentation/driver-api/media/mc-core.rst | 4 +- >>> drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ >>> include/media/media-entity.h | 45 ++++++++++++++++++++++ >>> 3 files changed, 85 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst >>> index 02481a2513b9..a2d1e32e3abb 100644 >>> --- a/Documentation/driver-api/media/mc-core.rst >>> +++ b/Documentation/driver-api/media/mc-core.rst >>> @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. >>> >>> Helper functions can be used to find a link between two given pads, or a pad >>> connected to another pad through an enabled link >>> -:c:func:`media_entity_find_link()` and >>> -:c:func:`media_entity_remote_pad()`. >>> +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and >>> +:c:func:`media_entity_remote_source_pad()`). >>> >>> Use count and power handling >>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c >>> index 11f5207f73aa..1febf5a86be6 100644 >>> --- a/drivers/media/mc/mc-entity.c >>> +++ b/drivers/media/mc/mc-entity.c >>> @@ -9,6 +9,7 @@ >>> */ >>> >>> #include <linux/bitmap.h> >>> +#include <linux/list.h> >>> #include <linux/property.h> >>> #include <linux/slab.h> >>> #include <media/media-entity.h> >>> @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) >>> } >>> EXPORT_SYMBOL_GPL(media_entity_remote_pad); >>> >>> +struct media_pad * >>> +media_entity_remote_pad_unique(const struct media_entity *entity, >>> + unsigned int type) >>> +{ >>> + struct media_pad *pad = NULL; >>> + struct media_link *link; >>> + >>> + list_for_each_entry(link, &entity->links, list) { >>> + struct media_pad *local_pad; >>> + struct media_pad *remote_pad; >>> + >>> + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) >>> + continue; >>> + >>> + if (type == MEDIA_PAD_FL_SOURCE) { >>> + local_pad = link->sink; >>> + remote_pad = link->source; >>> + } else { >>> + local_pad = link->source; >>> + remote_pad = link->sink; >>> + } >>> + >>> + if (local_pad->entity == entity) { >>> + if (pad) >>> + return ERR_PTR(-ENOTUNIQ); >>> + >>> + pad = remote_pad; >>> + } >>> + } >>> + >>> + if (!pad) >>> + return ERR_PTR(-ENOLINK); >>> + >>> + return pad; >>> +} >>> +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); >>> + >>> static void media_interface_init(struct media_device *mdev, >>> struct media_interface *intf, >>> u32 gobj_type, >>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h >>> index a9a1c0ec5d1c..33d5f52719a0 100644 >>> --- a/include/media/media-entity.h >>> +++ b/include/media/media-entity.h >>> @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, >>> */ >>> struct media_pad *media_entity_remote_pad(const struct media_pad *pad); >>> >>> +/** >>> + * media_entity_remote_pad_unique - Find a remote pad connected to an entity >>> + * @entity: The entity >>> + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) >>> + * >>> + * Search for and return a remote pad of @type connected to @entity through an >>> + * enabled link. If multiple (or no) remote pads match these criteria, an error >>> + * is returned. >>> + * >>> + * The uniqueness constraint makes this helper function suitable for entities >>> + * that support a single active source or sink at a time. >>> + * >>> + * Return: A pointer to the remote pad, or one of the following error pointers >>> + * if an error occurs: >>> + * >>> + * * -ENOTUNIQ - Multiple links are enabled >>> + * * -ENOLINK - No connected pad found >>> + */ >>> +struct media_pad * >>> +media_entity_remote_pad_unique(const struct media_entity *entity, >>> + unsigned int type); >>> + >>> +/** >>> + * media_entity_remote_source_pad - Find a remote source pad connected to an entity >>> + * @entity: The entity >>> + * >>> + * Search for and return a remote source pad connected to @entity through an >>> + * enabled link. If multiple (or no) remote pads match these criteria, an error >>> + * is returned. >>> + * >>> + * The uniqueness constraint makes this helper function suitable for entities >>> + * that support a single active source at a time. >>> + * >>> + * Return: A pointer to the remote pad, or one of the following error pointers >>> + * if an error occurs: >>> + * >>> + * * -ENOTUNIQ - Multiple links are enabled >>> + * * -ENOLINK - No connected pad found >>> + */ >>> +static inline struct media_pad * >>> +media_entity_remote_source_pad(const struct media_entity *entity) >>> +{ >>> + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); >>> +} >>> + >>> /** >>> * media_entity_is_streaming - Test if an entity is part of a streaming pipeline >>> * @entity: The entity >
Hi Hans, On Thu, Jul 07, 2022 at 08:52:16AM +0200, Hans Verkuil wrote: > On 6/25/22 19:00, Laurent Pinchart wrote: > > On Fri, Jun 17, 2022 at 01:48:05PM +0200, Hans Verkuil wrote: > >> On 6/14/22 21:11, Paul Elder wrote: > >>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> > >>> The media_entity_remote_pad() helper function returns the first remote > >>> pad it find connected to a given pad. Beside being possibly ill-named > >>> (as it operates on a pad, not an entity) and non-deterministic (as it > >>> stops at the first enabled link), the fact that it returns the first > >>> match makes it unsuitable for drivers that need to guarantee that a > >>> single link is enabled, for instance when an entity can process data > >>> from one of multiple sources at a time. > >> > >> Question: of all the callers of this function, are there any that really > >> need media_entity_remote_pad() instead of media_pad_remote_pad_unique()? > >> > >> Would it be possible to replace all callers of the old function with the > >> new function? If that's the case, then the _unique suffix can be dropped, > >> since that would effectively be the default. And if a function is needed > >> to handle the case where there are multiple enabled links, then a new > >> function should be created. > > > > I don't think so. media_entity_remote_pad() operates on a pad, switching > > to media_pad_remote_pad_unique() wouldn't work on subdevs that have > > multiple sink or source pads with one active link each. > > Do we have those today in the mainline kernel? Just checking... Re-reading my reply, it seems I may have been mistaken. We may be able to ditch media_pad_remote_pad_first() and replace it with media_pad_remote_pad_unique() (at least in the majority of cases), but we'll need to carefully review each user. > >> Also, media_entity_remote_pad() should really be renamed to > >> media_pad_remote_pad_first() or something like that, right? I'm not saying > >> you should, but that's really what it does, as I understand it. > > > > Yes, I think that would make sense, and it would freethe > > media_entity_remote_pad() name, so the new function wouldn't need the > > _unique suffix. I'll give it a try. > > > >>> For those use cases, add a new helper function, > >>> media_entity_remote_pad_unique(), that operates on an entity and returns > >>> a remote pad, with a guarantee that only one link is enabled. To ease > >>> its use in drivers, also add an inline wrapper that locates source pads > >>> specifically. A wrapper that locates sink pads can easily be added when > >>> needed. > >>> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> --- > >>> Documentation/driver-api/media/mc-core.rst | 4 +- > >>> drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ > >>> include/media/media-entity.h | 45 ++++++++++++++++++++++ > >>> 3 files changed, 85 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > >>> index 02481a2513b9..a2d1e32e3abb 100644 > >>> --- a/Documentation/driver-api/media/mc-core.rst > >>> +++ b/Documentation/driver-api/media/mc-core.rst > >>> @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. > >>> > >>> Helper functions can be used to find a link between two given pads, or a pad > >>> connected to another pad through an enabled link > >>> -:c:func:`media_entity_find_link()` and > >>> -:c:func:`media_entity_remote_pad()`. > >>> +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and > >>> +:c:func:`media_entity_remote_source_pad()`). > >>> > >>> Use count and power handling > >>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > >>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > >>> index 11f5207f73aa..1febf5a86be6 100644 > >>> --- a/drivers/media/mc/mc-entity.c > >>> +++ b/drivers/media/mc/mc-entity.c > >>> @@ -9,6 +9,7 @@ > >>> */ > >>> > >>> #include <linux/bitmap.h> > >>> +#include <linux/list.h> > >>> #include <linux/property.h> > >>> #include <linux/slab.h> > >>> #include <media/media-entity.h> > >>> @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) > >>> } > >>> EXPORT_SYMBOL_GPL(media_entity_remote_pad); > >>> > >>> +struct media_pad * > >>> +media_entity_remote_pad_unique(const struct media_entity *entity, > >>> + unsigned int type) > >>> +{ > >>> + struct media_pad *pad = NULL; > >>> + struct media_link *link; > >>> + > >>> + list_for_each_entry(link, &entity->links, list) { > >>> + struct media_pad *local_pad; > >>> + struct media_pad *remote_pad; > >>> + > >>> + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) > >>> + continue; > >>> + > >>> + if (type == MEDIA_PAD_FL_SOURCE) { > >>> + local_pad = link->sink; > >>> + remote_pad = link->source; > >>> + } else { > >>> + local_pad = link->source; > >>> + remote_pad = link->sink; > >>> + } > >>> + > >>> + if (local_pad->entity == entity) { > >>> + if (pad) > >>> + return ERR_PTR(-ENOTUNIQ); > >>> + > >>> + pad = remote_pad; > >>> + } > >>> + } > >>> + > >>> + if (!pad) > >>> + return ERR_PTR(-ENOLINK); > >>> + > >>> + return pad; > >>> +} > >>> +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); > >>> + > >>> static void media_interface_init(struct media_device *mdev, > >>> struct media_interface *intf, > >>> u32 gobj_type, > >>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h > >>> index a9a1c0ec5d1c..33d5f52719a0 100644 > >>> --- a/include/media/media-entity.h > >>> +++ b/include/media/media-entity.h > >>> @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, > >>> */ > >>> struct media_pad *media_entity_remote_pad(const struct media_pad *pad); > >>> > >>> +/** > >>> + * media_entity_remote_pad_unique - Find a remote pad connected to an entity > >>> + * @entity: The entity > >>> + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) > >>> + * > >>> + * Search for and return a remote pad of @type connected to @entity through an > >>> + * enabled link. If multiple (or no) remote pads match these criteria, an error > >>> + * is returned. > >>> + * > >>> + * The uniqueness constraint makes this helper function suitable for entities > >>> + * that support a single active source or sink at a time. > >>> + * > >>> + * Return: A pointer to the remote pad, or one of the following error pointers > >>> + * if an error occurs: > >>> + * > >>> + * * -ENOTUNIQ - Multiple links are enabled > >>> + * * -ENOLINK - No connected pad found > >>> + */ > >>> +struct media_pad * > >>> +media_entity_remote_pad_unique(const struct media_entity *entity, > >>> + unsigned int type); > >>> + > >>> +/** > >>> + * media_entity_remote_source_pad - Find a remote source pad connected to an entity > >>> + * @entity: The entity > >>> + * > >>> + * Search for and return a remote source pad connected to @entity through an > >>> + * enabled link. If multiple (or no) remote pads match these criteria, an error > >>> + * is returned. > >>> + * > >>> + * The uniqueness constraint makes this helper function suitable for entities > >>> + * that support a single active source at a time. > >>> + * > >>> + * Return: A pointer to the remote pad, or one of the following error pointers > >>> + * if an error occurs: > >>> + * > >>> + * * -ENOTUNIQ - Multiple links are enabled > >>> + * * -ENOLINK - No connected pad found > >>> + */ > >>> +static inline struct media_pad * > >>> +media_entity_remote_source_pad(const struct media_entity *entity) > >>> +{ > >>> + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); > >>> +} > >>> + > >>> /** > >>> * media_entity_is_streaming - Test if an entity is part of a streaming pipeline > >>> * @entity: The entity
diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst index 02481a2513b9..a2d1e32e3abb 100644 --- a/Documentation/driver-api/media/mc-core.rst +++ b/Documentation/driver-api/media/mc-core.rst @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. Helper functions can be used to find a link between two given pads, or a pad connected to another pad through an enabled link -:c:func:`media_entity_find_link()` and -:c:func:`media_entity_remote_pad()`. +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and +:c:func:`media_entity_remote_source_pad()`). Use count and power handling ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c index 11f5207f73aa..1febf5a86be6 100644 --- a/drivers/media/mc/mc-entity.c +++ b/drivers/media/mc/mc-entity.c @@ -9,6 +9,7 @@ */ #include <linux/bitmap.h> +#include <linux/list.h> #include <linux/property.h> #include <linux/slab.h> #include <media/media-entity.h> @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) } EXPORT_SYMBOL_GPL(media_entity_remote_pad); +struct media_pad * +media_entity_remote_pad_unique(const struct media_entity *entity, + unsigned int type) +{ + struct media_pad *pad = NULL; + struct media_link *link; + + list_for_each_entry(link, &entity->links, list) { + struct media_pad *local_pad; + struct media_pad *remote_pad; + + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) + continue; + + if (type == MEDIA_PAD_FL_SOURCE) { + local_pad = link->sink; + remote_pad = link->source; + } else { + local_pad = link->source; + remote_pad = link->sink; + } + + if (local_pad->entity == entity) { + if (pad) + return ERR_PTR(-ENOTUNIQ); + + pad = remote_pad; + } + } + + if (!pad) + return ERR_PTR(-ENOLINK); + + return pad; +} +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); + static void media_interface_init(struct media_device *mdev, struct media_interface *intf, u32 gobj_type, diff --git a/include/media/media-entity.h b/include/media/media-entity.h index a9a1c0ec5d1c..33d5f52719a0 100644 --- a/include/media/media-entity.h +++ b/include/media/media-entity.h @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, */ struct media_pad *media_entity_remote_pad(const struct media_pad *pad); +/** + * media_entity_remote_pad_unique - Find a remote pad connected to an entity + * @entity: The entity + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) + * + * Search for and return a remote pad of @type connected to @entity through an + * enabled link. If multiple (or no) remote pads match these criteria, an error + * is returned. + * + * The uniqueness constraint makes this helper function suitable for entities + * that support a single active source or sink at a time. + * + * Return: A pointer to the remote pad, or one of the following error pointers + * if an error occurs: + * + * * -ENOTUNIQ - Multiple links are enabled + * * -ENOLINK - No connected pad found + */ +struct media_pad * +media_entity_remote_pad_unique(const struct media_entity *entity, + unsigned int type); + +/** + * media_entity_remote_source_pad - Find a remote source pad connected to an entity + * @entity: The entity + * + * Search for and return a remote source pad connected to @entity through an + * enabled link. If multiple (or no) remote pads match these criteria, an error + * is returned. + * + * The uniqueness constraint makes this helper function suitable for entities + * that support a single active source at a time. + * + * Return: A pointer to the remote pad, or one of the following error pointers + * if an error occurs: + * + * * -ENOTUNIQ - Multiple links are enabled + * * -ENOLINK - No connected pad found + */ +static inline struct media_pad * +media_entity_remote_source_pad(const struct media_entity *entity) +{ + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); +} + /** * media_entity_is_streaming - Test if an entity is part of a streaming pipeline * @entity: The entity