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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.
> 
> 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 :-(

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.

-- 
Regards,

Laurent Pinchart