Re: [PATCH 4/5] media: entity: Add support for ancillary links

[Daniel Scally <djrscally@gmail.com>]

Hi Laurent, sorry - realised I never replied to this one

On 14/12/2021 22:14, Laurent Pinchart wrote:
> Hi Daniel,
>
> Thank you for the patch.
>
> On Tue, Dec 14, 2021 at 09:54:32PM +0000, Daniel Scally wrote:
>> On 14/12/2021 21:25, Sakari Ailus wrote:
>>> On Mon, Dec 13, 2021 at 11:28:48PM +0000, Daniel Scally wrote:
>>>> Add functions to create and destroy ancillary links, so that they
>>>> don't need to be manually created by users.
>>>>
>>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>>> ---
>>>> Changes since the rfc:
>>>>
>>>> 	- (Laurent) Set gobj0 and gobj1 directly instead of the other union
>>>> 	members
>>>> 	- (Laurent) Added MEDIA_LNK_FL_IMMUTABLE to the kerneldoc for the new
>>>> 	create function
>>>>
>>>>  drivers/media/mc/mc-entity.c | 30 ++++++++++++++++++++++++++++++
>>>>  include/media/media-entity.h | 29 +++++++++++++++++++++++++++++
>>>>  2 files changed, 59 insertions(+)
>>>>
>>>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
>>>> index aeddc3f6310e..4e39e100ea03 100644
>>>> --- a/drivers/media/mc/mc-entity.c
>>>> +++ b/drivers/media/mc/mc-entity.c
>>>> @@ -1052,3 +1052,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);
> Not a candidate for this series, but returning an error pointer from
> media_add_link() could be nice.
>
>>>> +	if (!link)
>>>> +		return ERR_PTR(-ENOMEM);
>>>> +
>>>> +	link->gobj0 = &primary->graph_obj;
>>>> +	link->gobj1 = &ancillary->graph_obj;
>>>> +	link->flags = flags | MEDIA_LNK_FL_ANCILLARY_LINK;
>>>> +
>>>> +	/* Initialize graph object embedded at the new link */
> s/embedded at/embedded in/ ?
>
>>>> +	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);
> Non-static (and especially exported) functions must be declared in a
> header file. You don't seem to use this function anywhere in this series
> though, is it a leftover ?
>
>>>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>>>> index fea489f03d57..f7b1738cef88 100644
>>>> --- a/include/media/media-entity.h
>>>> +++ b/include/media/media-entity.h
>>>> @@ -1104,6 +1104,35 @@ 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
> s/link/ancillary link/
>
>>>> + *
>>>> + * @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.
>>>> + *
>>>> + * %MEDIA_LNK_FL_IMMUTABLE
>>>> + *   Indicates that the link enabled state can't be modified at runtime. If
>>>> + *   %MEDIA_LNK_FL_IMMUTABLE is set, then %MEDIA_LNK_FL_ENABLED must also be
>>>> + *   set, since an immutable link is always enabled.
>>> What's the use case for both of the flags?
>>>
>>> I know the flags are there but what will they mean in practice for
>>> ancillary links?
>> Within the kernel, I don't think they have any effect now (without patch
>> #3 of this series the graph iteration would have tried to walk them). I
>> mostly intended that they would be set so that future userspace users
>> wouldn't be able to flag them as disabled.
> How about removing the flags parameter, hardcoding both
> MEDIA_LNK_FL_ENABLED and MEDIA_LNK_FL_IMMUTABLE inside the function, and
> specifying in the userspace API documentation that both flags are always
> set of ancillary links ?


This is ok by me

>
> Thinking a bit further, what would be the implications of changing this
> rule in the future ? I don't know what use cases may require that, but
> let's assume we start exposing mutable ancillary links, or mutable and
> disabled ancillary links. How will existing userspace react to that, do
> we need to specify rules in the uAPI documentation to tell userspace how
> to prepare for the future ?


Yeah maybe; at the moment my libcamera implementation wouldn't treat a
disabled link any differently, and for a lens at least I can't really
see any situation where a disabled link would be valid. Perhaps multiple
LED drivers supporting the same sensor for different flash modes might
be a thing though, in which case it might be helpful to disable one of
the ancillary links so that you can flag a particular driver as the
active one, or something long those lines.


So perhaps the uAPI docs should explain that both ENABLED and IMMUTABLE
are currently hardcoded, but that userspace should still check for an
enabled link before following it.

>
>>>> + */
>>>> +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)