Hi Laurent
On 26/11/2021 02:56, Laurent Pinchart wrote:
> 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.
Maybe; I'll investigate doing it that way
> I'm sure we'll also nitpick on the names, especially when adding
> documentation. The concept seems fine to me though.
Hah; that's fine, naming things was never my strong suit.
>> 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 ?
I couldn't think of one really.
> Should the IMMUTABLE flag be set as well ?
Yes; I'll add that here too, thanks
>
>> + */
>> +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;
