Re: [PATCH] media: v4l2-async: fix binding async subdevs with multiple source ports

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

Hi Philipp,

(CC'ing Francesco)

My apologies for not having noticed the patch.

On Fri, Jan 13, 2023 at 12:24:56PM +0100, Philipp Zabel wrote:
> Hi,
> 
> On Wed, Aug 10, 2022 at 12:48:48PM +0200, Philipp Zabel wrote:
> > Asynchronous subdevice probing on imx-media with imx6-mipi-csi2 is
> > broken since commit 1f391df44607 ("media: v4l2-async: Use endpoints in
> > __v4l2_async_nf_add_fwnode_remote()").
> > 
> > This is a side effect of imx6-mipi-csi2 having a single subdevice with
> > four separate source ports connected to different subdevices. Before,
> > the imx-media-csi and video-mux devices registered async sub-devices
> > via device fwnode that matched the imx6-mipi-csi2 device on their
> > respective notifiers. This caused the first asd to be put on the
> > notifier waiting list, and the other three to return -EEXIST and be
> > ignored.
> > 
> > With async subdev registration via endpoint fwnode, all four asds are
> > distinct and three of them keep dangling on their notifiers after the
> > first one is bound.
> > 
> > This patch modifies __v4l2_async_nf_has_async_subdev() to consider
> > asds matching different fwnode endpoints of the same sub-device equal
> > if the latter is already bound and matches via device fwnode. Further,
> > v4l2_async_register_subdev() is modified to remove dangling duplicate
> > asds that were registered before the sub-device was available to check
> > its fwnode.

To make sure I understand this correctly, you need both changes, with
the change in __v4l2_async_nf_has_async_subdev() meant to address asds
being added after the subdev has been registered, and the change in
v4l2_async_register_subdev() meant to address asds that have been added
before ?

The imx6 ipu drivers implement a "clever hack" to handle the
multi-endpoint issue that was never officially supported by v4l2-async.
Obviously, as it has worked so far, leaving it broken isn't a very nice
option. The fix feels a bit like a hack though, and a better solution
would be to allow subdevs to be matched multiple times, by multiple
consumers. That's a more intrusive change though, so I could be OK with
this as a short term fix, assuming it doesn't break anything else.

I would however want to ensure this doesn't get abused by new drivers.
Could we add a dev_warn() somewhere to indicate that multi-endpoint
matching is not supported and shouldn't be used until fixed ? Sakari,
what do you think ?

> > Fixes: 1f391df44607 ("media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()")
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> Any comments on this? The issue persists on v6.2-rc3.

Francesco, does this fix your issue ?

> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 43 ++++++++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 2f1b718a9189..b24220ed7077 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -452,6 +452,22 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
> >  
> >  		if (asd_equal(asd, sd->asd))
> >  			return true;
> > +
> > +		/*
> > +		 * If the asd matches an endpoint fwnode, handle sub-devices
> > +		 * with device fwnode that were already matched by another asd
> > +		 * with a different endpoint fwnode on the same device.
> > +		 */
> > +		if (asd->match_type == V4L2_ASYNC_MATCH_FWNODE &&
> > +		    fwnode_graph_is_endpoint(asd->match.fwnode) &&
> > +		    sd->fwnode && !fwnode_graph_is_endpoint(sd->fwnode)) {
> > +			struct fwnode_handle *devnode;
> > +
> > +			devnode = fwnode_graph_get_port_parent(asd->match.fwnode);
> > +			fwnode_handle_put(devnode);
> > +			if (devnode == sd->fwnode)
> > +				return true;
> > +		}
> >  	}
> >  
> >  	return false;
> > @@ -749,6 +765,24 @@ __v4l2_async_nf_add_i2c(struct v4l2_async_notifier *notifier, int adapter_id,
> >  }
> >  EXPORT_SYMBOL_GPL(__v4l2_async_nf_add_i2c);
> >  
> > +static void v4l2_async_remove_duplicate_matches(struct v4l2_subdev *sd)
> > +{
> > +	struct v4l2_async_notifier *notifier;
> > +
> > +	lockdep_assert_held(&list_lock);
> > +
> > +	list_for_each_entry(notifier, &notifier_list, list) {
> > +		struct v4l2_async_subdev *asd;
> > +
> > +		asd = v4l2_async_find_match(notifier, sd);
> > +		if (!asd)
> > +			continue;
> > +
> > +		/* Remove from the waiting list */
> > +		list_del(&asd->list);
> > +	}
> > +}
> > +
> >  int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> >  {
> >  	struct v4l2_async_notifier *subdev_notifier;
> > @@ -783,6 +817,15 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> >  		if (ret)
> >  			goto err_unbind;
> >  
> > +		/*
> > +		 * At this point the asd is removed from the notifier wait list.
> > +		 * There might be other notifiers with asds matching different
> > +		 * fwnode endpoints of the same sd remaining. If the sd matches
> > +		 * by device fwnode, remove the dangling asds.
> > +		 */
> > +		if (sd->fwnode && !fwnode_graph_is_endpoint(sd->fwnode))
> > +			v4l2_async_remove_duplicate_matches(sd);
> > +
> >  		ret = v4l2_async_nf_try_complete(notifier);
> >  		if (ret)
> >  			goto err_unbind;

-- 
Regards,

Laurent Pinchart