Re: [PATCH 5/5] media: v4l2-async: Create links during v4l2_async_match_notify()

[Daniel Scally <djrscally@gmail.com>]

Hi Laurent, Sakari

On 15/12/2021 09:55, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wed, Dec 15, 2021 at 11:44:29AM +0200, Sakari Ailus wrote:
>> On Wed, Dec 15, 2021 at 11:04:44AM +0200, Laurent Pinchart wrote:
>>> On Tue, Dec 14, 2021 at 11:41:27PM +0000, Daniel Scally wrote:
>>>> On 14/12/2021 23:01, Laurent Pinchart wrote:
>>>>> On Tue, Dec 14, 2021 at 10:36:01PM +0000, Daniel Scally wrote:
>>>>>> On 14/12/2021 22:22, Laurent Pinchart wrote:
>>>>>>> On Mon, Dec 13, 2021 at 11:28:49PM +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@gmail.com>
>>>>>>>> ---
>>>>>>>> Changes since the rfc:
>>>>>>>>
>>>>>>>> 	- None
>>>>>>>>
>>>>>>>>  drivers/media/v4l2-core/v4l2-async.c | 51 ++++++++++++++++++++++++++++
>>>>>>>>  1 file changed, 51 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>>>>>>>> index 0404267f1ae4..6575b1cbe95f 100644
>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-async.c
>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>>>>>>>> @@ -275,6 +275,45 @@ 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;
>>>>>>>> +
>>>>>>>> +	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,
>>>>>>>
>>>>>>> Is there a guarantee at this point that notifier->sd->entity has already
>>>>>>> been registered with the media_device ? That's done by
>>>>>>> media_device_register_entity() called from
>>>>>>> v4l2_device_register_subdev().
>>>>>>
>>>>>> v4l2_async_match_notify() calls v4l2_device_register_subdev() before the
>>>>>> point that I've added the call to v4l2_async_try_create_links(), so I
>>>>>> think that's covered there.
>>>>>
>>>>> It calls it on sd, not notifier->sd. It's the latter that concerns me.
>>>>
>>>> Ah, you're right of course...I guess in that case the notifier->sd would
>>>> get registered during the v4l2_async_match_notify() where the sensor
>>>> driver's subdev is sd, but I don't think there's any guarantee that that
>>>> would happen first...I haven't traced it through but my guess is that it
>>>> would depend on the order in which the ipu3-cio2, sensor and lens
>>>> controller drivers probed. I'll check to try and be sure how it works
>>>> tomorrow
>>>
>>> I was looking at media_device_register_entity(), and it sets
>>>
>>> 	INIT_LIST_HEAD(&entity->links);
>>> 	entity->num_links = 0;
>>> 	entity->num_backlinks = 0;
>>>
>>> If we create links before that, things may go bad.

Yep, that definitely looks like it would make things go badly wrong. I'm
building with a delayed ov8865 probe now, let's see what happens...

>>
>> Yes.
>>
>> There's a guarantee that the notifier's complete callback is called once
>> the notifier's subdevs as well as sub-notifiers are bound and complete. But
>> there's no guarantee on the initialisation of related entities.
>>
>> Especially for sensors, the async subdev is registered after the sensor's
>> own async notifier.
>>
>> I wonder if the ugly registered callback could be used for this purpose.
>> Better of course would be to avoid that.
> 
> I'd really like all these links to be created automatically by the code,
> but given the very loosely defined rules regarding entity
> initialization, I'm worried this may not be possible without quite a bit
> of cleanup first :-(


Yeah. At present at least the primary entity would need to be linked to
the media dev, as it's taking primary->graph_obj.mdev as the pointer to
use in media_gobj_create() in media_create_ancillary_link(). That's a
pretty big problem actually...but I'd really like to try and solve it as
we could cut a lot of code out the other drivers if we do the same thing
for the data links.

One way around it might be to defer matching in v4l2_async_find_match()
if the notifier's subdev hasn't picked up an mdev itself yet...which
could guarantee the ordering but sort of breaks the asynchronicity of it
all. I'm almost certainly missing some other reason that that's a
terrible idea too.

I'll try and explore some ways to do this that still keeps the link
setup within core - thanks for pointing it out Laurent

> It looks like quite a bit of the work done in
> media_device_register_entity() could (and likely should) be moved to
> media_entity_init(), but I'm not sure if that would be enough to
> properly fix the issue.

I guess you mean media_entity_pads_init()? Or media_device_init()?
>