Hi Dan,
Thank you for the patch.
On Fri, Nov 26, 2021 at 12:16:02AM +0000, Daniel Scally wrote:
> To describe in the kernel the connection between devices and their
> supporting peripherals (for example, a camera sensor and the vcm
> driving the focusing lens for it), add a new type of media link
> which connects two instances of struct media_entity.
>
> Signed-off-by: Daniel Scally <djrscally@xxxxxxxxx>
> ---
>
> I was tempted to 'fix' the spaces between # and define in
> include/uapi/linux/media.h but eventually decided they were probably deliberate
> but if that's not true I'd fix those too.
>
> drivers/media/mc/mc-entity.c | 30 ++++++++++++++++++++++++++++++
> include/media/media-entity.h | 30 ++++++++++++++++++++++++++++++
> include/uapi/linux/media.h | 1 +
> 3 files changed, 61 insertions(+)
>
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index f40f41977142..9c18b974e117 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -1032,3 +1032,33 @@ void media_remove_intf_links(struct media_interface *intf)
> mutex_unlock(&mdev->graph_mutex);
> }
> EXPORT_SYMBOL_GPL(media_remove_intf_links);
> +
> +struct media_link *media_create_ancillary_link(struct media_entity *primary,
> + struct media_entity *ancillary,
> + u32 flags)
> +{
> + struct media_link *link;
> +
> + link = media_add_link(&primary->links);
> + if (!link)
> + return ERR_PTR(-ENOMEM);
> +
> + link->primary = primary;
> + link->ancillary = ancillary;
> + link->flags = flags | MEDIA_LNK_FL_ANCILLARY_LINK;
> +
> + /* Initialize graph object embedded at the new link */
> + media_gobj_create(primary->graph_obj.mdev, MEDIA_GRAPH_LINK,
> + &link->graph_obj);
> +
> + return link;
> +}
> +EXPORT_SYMBOL_GPL(media_create_ancillary_link);
> +
> +void media_remove_ancillary_link(struct media_link *link)
> +{
> + list_del(&link->list);
> + media_gobj_destroy(&link->graph_obj);
> + kfree(link);
> +}
> +EXPORT_SYMBOL_GPL(media_remove_ancillary_link);
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index fea489f03d57..400b864857ee 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -119,12 +119,16 @@ struct media_pipeline {
> * a pad. In that case, it represents the source pad.
> * @intf: Part of a union. Used only if the first object (gobj0) is
> * an interface.
> + * @primary: Part of a union. Used only if the first object (gobj0) is
> + * an entity and the link type is MEDIA_LNK_FL_ANCILLARY_LINK.
> * @gobj1: Part of a union. Used to get the pointer for the second
> * graph_object of the link.
> * @sink: Part of a union. Used only if the second object (gobj1) is
> * a pad. In that case, it represents the sink pad.
> * @entity: Part of a union. Used only if the second object (gobj1) is
> * an entity.
> + * @ancillary: Part of a union. Used only if the second object (gobj1) is
> + * an entity and the link type is MEDIA_LNK_FL_ANCILLARY_LINK.
> * @reverse: Pointer to the link for the reverse direction of a pad to pad
> * link.
> * @flags: Link flags, as defined in uapi/media.h (MEDIA_LNK_FL_*)
> @@ -137,11 +141,13 @@ struct media_link {
> struct media_gobj *gobj0;
> struct media_pad *source;
> struct media_interface *intf;
> + struct media_entity *primary;
> };
> union {
> struct media_gobj *gobj1;
> struct media_pad *sink;
> struct media_entity *entity;
> + struct media_entity *ancillary;
> };
Those unions are not very nice, but it's not your fault. I however
wonder if we couldn't just use the gobj0 and gobj1 fields, and avoid
adding primary and ancillary.
I'm sure we'll also nitpick on the names, especially when adding
documentation. The concept seems fine to me though.
> struct media_link *reverse;
> unsigned long flags;
> @@ -1104,6 +1110,30 @@ void media_remove_intf_links(struct media_interface *intf);
> * it will issue a call to @operation\(@entity, @args\).
> */
>
> +/**
> + * media_create_ancillary_link() - creates a link between two entities
> + *
> + * @primary: pointer to the primary %media_entity
> + * @ancillary: pointer to the ancillary %media_entity
> + * @flags: Link flags, as defined in
> + * :ref:`include/uapi/linux/media.h <media_header>`
> + * ( seek for ``MEDIA_LNK_FL_*``)
> + *
> + *
> + * Valid values for flags:
> + *
> + * %MEDIA_LNK_FL_ENABLED
> + * Indicates that the two entities are connected pieces of hardware that form
> + * a single logical unit.
> + *
> + * A typical example is a camera lens being linked to the sensor that it is
> + * supporting.
Is there any use case for an ancillary link that wouldn't be enabled ?
Should the IMMUTABLE flag be set as well ?
> + */
> +struct media_link *
> +media_create_ancillary_link(struct media_entity *primary,
> + struct media_entity *ancillary,
> + u32 flags);
> +
> #define media_entity_call(entity, operation, args...) \
> (((entity)->ops && (entity)->ops->operation) ? \
> (entity)->ops->operation((entity) , ##args) : -ENOIOCTLCMD)
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 200fa8462b90..afbae7213d35 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -226,6 +226,7 @@ struct media_pad_desc {
> #define MEDIA_LNK_FL_LINK_TYPE (0xf << 28)
> # define MEDIA_LNK_FL_DATA_LINK (0 << 28)
> # define MEDIA_LNK_FL_INTERFACE_LINK (1 << 28)
> +# define MEDIA_LNK_FL_ANCILLARY_LINK (2 << 28)
>
> struct media_link_desc {
> struct media_pad_desc source;
--
Regards,
Laurent Pinchart
