[libcamera-devel] [PATCH v2 6/6] media: v4l2-async: Create links during v4l2_async_match_notify()

[Daniel Scally]

Hi Sakari

On 02/02/2022 21:48, Daniel Scally wrote:
> Hi Sakari
>
> On 02/02/2022 16:38, Sakari Ailus wrote:
>> Hi Daniel,
>>
>> Thanks for the update.
>>
>> On Sun, Jan 30, 2022 at 11:58:21PM +0000, Daniel Scally wrote:
>>> Upon an async fwnode match, there's some typical behaviour that the
>>> notifier and matching subdev will want to do. For example, a notifier
>>> representing a sensor matching to an async subdev representing its
>>> VCM will want to create an ancillary link to expose that relationship
>>> to userspace.
>>>
>>> To avoid lots of code in individual drivers, try to build these links
>>> within v4l2 core.
>>>
>>> Signed-off-by: Daniel Scally <djrscally at gmail.com>
>>> ---
>>> Changes since v1:
>>>
>>> 	- Added #ifdef guards for CONFIG_MEDIA_CONTROLLER
>>> 	- Some spelling and nomenclature cleanup (Laurent)
>>>
>>> Changes since the rfc:
>>>
>>> 	- None
>>>
>>>  drivers/media/v4l2-core/v4l2-async.c | 56 ++++++++++++++++++++++++++++
>>>  1 file changed, 56 insertions(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>>> index 0404267f1ae4..8980534e755e 100644
>>> --- a/drivers/media/v4l2-core/v4l2-async.c
>>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>>> @@ -275,6 +275,50 @@ v4l2_async_nf_try_complete(struct v4l2_async_notifier *notifier)
>>>  static int
>>>  v4l2_async_nf_try_all_subdevs(struct v4l2_async_notifier *notifier);
>>>  
>>> +static int
>>> +__v4l2_async_create_ancillary_link(struct v4l2_async_notifier *notifier,
>>> +				   struct v4l2_subdev *sd)
>>> +{
>>> +	struct media_link *link = NULL;
>>> +
>>> +#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
>>> +
>>> +	if (sd->entity.function != MEDIA_ENT_F_LENS &&
>>> +	    sd->entity.function != MEDIA_ENT_F_FLASH)
>>> +		return -EINVAL;
>>> +
>>> +	link = media_create_ancillary_link(&notifier->sd->entity, &sd->entity);
>>> +
>>> +#endif
>>> +
>>> +	return IS_ERR(link) ? PTR_ERR(link) : 0;
>>> +}
>>> +
>>> +/*
>>> + * Create links on behalf of the notifier and subdev, where it's obvious what
>>> + * should be done. At the moment, we only support cases where the notifier
>>> + * is a camera sensor and the subdev is a lens controller.
>> I think I'd rather change this so that ancillary links are created for lens
>> and flash subdevs, independently of the function of the notifier subdev.
>>
>> Are there cases where this would go wrong currently, or in the future? I
>> can't think of any right now at least. I guess we could add more
>> information in the future to convey here if needed.
> I don't think doing that would go wrong anyhow...at least not that I
> could think of at the minute. My plan was to add a new function like
> __v4l2_async_create_data_links() and call that (from
> v4l2_async_try_create_links()) where the function of the notifier subdev
> was MEDIA_ENT_F_VID_IF_BRIDGE...you think we shouldn't be doing that? Or
> just that it should be separate?


Gentle ping for this part :)

>>> + *
>>> + * TODO: Create data links if the notifier's function is
>>> + * MEDIA_ENT_F_VID_IF_BRIDGE and the subdev's is MEDIA_ENT_F_CAM_SENSOR.
>>> + */
>>> +static int v4l2_async_try_create_links(struct v4l2_async_notifier *notifier,
>>> +				       struct v4l2_subdev *sd)
>> How about calling this v4l2_async_create_ancillary_links()?
>
> Sure
>
>>> +{
>>> +#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
>>> +
>>> +	if (!notifier->sd)
>>> +		return 0;
>>> +
>>> +	switch (notifier->sd->entity.function) {
>>> +	case MEDIA_ENT_F_CAM_SENSOR:
>>> +		return __v4l2_async_create_ancillary_link(notifier, sd);
>>> +	}
>>> +
>>> +#endif
>>> +	return 0;
>>> +}
>>> +
>>>  static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
>>>  				   struct v4l2_device *v4l2_dev,
>>>  				   struct v4l2_subdev *sd,
>>> @@ -293,6 +337,18 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
>>>  		return ret;
>>>  	}
>>>  
>>> +	/*
>>> +	 * Depending of the function of the entities involved, we may want to
>>> +	 * create links between them (for example between a sensor and its lens
>>> +	 * or between a sensor's source pad and the connected device's sink
>>> +	 * pad).
>>> +	 */
>>> +	ret = v4l2_async_try_create_links(notifier, sd);
>>> +	if (ret) {
>> The notifier's bound function has been called already. The unbound function
>> needs to be thus called here, too.
>
> Thank you, I'll do that in the next version
>
>>> +		v4l2_device_unregister_subdev(sd);
>>> +		return ret;
>>> +	}
>>> +
>>>  	/* Remove from the waiting list */
>>>  	list_del(&asd->list);
>>>  	sd->asd = asd;