On 6/14/22 21:11, Paul Elder wrote:
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxx>
> ---
> 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
