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@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.
> >>>>
> >>>> 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;
> >>>
> >>> 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
--
Regards,
Laurent Pinchart
