Re: [PATCH] media: v4l: async: Fix completion of chained subnotifiers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hej Niklas,

On Tue, Jan 30, 2024 at 02:43:41PM +0100, Niklas Söderlund wrote:
> Hi Sakari,
> 
> Thanks for your feedback.
> 
> On 2024-01-30 12:05:33 +0000, Sakari Ailus wrote:
> > Hi Niklas,
> > 
> > Thanks for the patch.
> > 
> > On Mon, Jan 29, 2024 at 08:59:54PM +0100, Niklas Söderlund wrote:
> > > Allowing multiple connections between entities are very useful but the
> > > addition of this feature did not considerate nested subnotifiers.
> > > 
> > > Consider the scenario,
> > > 
> > > rcar-vin.ko     rcar-isp.ko     rcar-csi2.ko    max96712.ko
> > > 
> > > video0 ---->    v4l-subdev0 ->  v4l-subdev1 ->  v4l-subdev2
> > > video1 -´
> > > 
> > > Where each videoX or v4l-subdevX is controlled and register by a
> > > separate instance of the driver listed above it. And each driver
> > > instance registers a notifier (videoX) or a subnotifier (v4l-subdevX)
> > > trying to bind to the device pointed to.
> > > 
> > > If the devices probe in any other except where the vidoeX ones are
> > > probed last only one of them will have their complete callback called,
> > > the one who last registered its notifier. Both of them will however have
> > > their bind() callback called as expected.
> > > 
> > > This is due to v4l2_async_nf_try_complete() only walking the chain from
> > > the subnotifier to one root notifier and completing it while ignoring
> > > all other notifiers the subdevice might be part of. This works if there
> > > are only one subnotifier in the mix. For example if either v4l-subdev0
> > > or v4l-subdev1 was not part of the pipeline above.
> > > 
> > > This patch addresses the issue of nested subnotifiers by instead looking
> > > at all notifiers and try to complete all the ones that contain the
> > > subdevice which subnotifier was completed.
> > 
> > Why do you need this?
> 
> I need this for the use-case described as an example above. In a 
> separate series [1] I remove the rcar-vin workaround for the earlier 
> lack of multiple connections between entities in v4l-async and without a 
> solution this patch tries to address this breaks on some boards that 
> already use nested subnotifiers but for which the rcar-vin workaround 
> addresses.
> 
> > This is also not a bug, the documentation for the complete callback says:
> > 
> >  * @complete:	All connections have been bound successfully. The complete
> >  *		callback is only executed for the root notifier.
> 
> Yes, and here there are two root notifiers. One in the driver 
> registering video0 and the one registering video1. Both notifiers wish 
> to bind to v4l-subdev0. And both notifers have their bind callback 
> called when v4l-subdev0 is registered, but only one have its complete 
> callback called.

In this respect the current framework isn't perfect, it only allows one
parent...

> 
> > 
> > Rather it would be better to get rid of this callback entirely, one reason
> > being the impossibility of error handling. We won't be there for quite some
> > time but extending its scope does go to the other direction.
> 
> I agree this is the way to go. And I could do without it in my use-cases 
> if I was allowed to register the video device at probe time instead of 
> in the complete callback. I have brought this up over the years but 
> always been told that the video device should be registered in the 
> callback handler. If this is no longer true I can rework [1] and a fix 

Are you sure?

I guess there may be differing opinions on the matter but drivers such as
ipu3-cio2 and omap3isp do it in probe. I don't think rcar-vin should be
different in this respect.

> like this wont be needed for my use-cases.
> 
> Looking beyond my use-case do you agree that as long as we do have the 
> complete callback it needs to be supported for nested subnotifiers?
> 
> 1. [PATCH 0/6] media: rcar-vin: Make use of multiple connections in v4l-async
> 
> > 
> > > 
> > > Fixes: 28a1295795d8 ("media: v4l: async: Allow multiple connections between entities")
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-async.c | 68 ++++++++++++++++++++--------
> > >  1 file changed, 49 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > > index 3ec323bd528b..8b603527923c 100644
> > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > @@ -176,15 +176,16 @@ static LIST_HEAD(notifier_list);
> > >  static DEFINE_MUTEX(list_lock);
> > >  
> > >  static struct v4l2_async_connection *
> > > -v4l2_async_find_match(struct v4l2_async_notifier *notifier,
> > > -		      struct v4l2_subdev *sd)
> > > +__v4l2_async_find_in_list(struct v4l2_async_notifier *notifier,
> > > +			  struct v4l2_subdev *sd,
> > > +			  struct list_head *list)
> > >  {
> > >  	bool (*match)(struct v4l2_async_notifier *notifier,
> > >  		      struct v4l2_subdev *sd,
> > >  		      struct v4l2_async_match_desc *match);
> > >  	struct v4l2_async_connection *asc;
> > >  
> > > -	list_for_each_entry(asc, &notifier->waiting_list, asc_entry) {
> > > +	list_for_each_entry(asc, list, asc_entry) {
> > >  		/* bus_type has been verified valid before */
> > >  		switch (asc->match.type) {
> > >  		case V4L2_ASYNC_MATCH_TYPE_I2C:
> > > @@ -207,6 +208,20 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier,
> > >  	return NULL;
> > >  }
> > >  
> > > +static struct v4l2_async_connection *
> > > +v4l2_async_find_match(struct v4l2_async_notifier *notifier,
> > > +		      struct v4l2_subdev *sd)
> > > +{
> > > +	return __v4l2_async_find_in_list(notifier, sd, &notifier->waiting_list);
> > > +}
> > > +
> > > +static struct v4l2_async_connection *
> > > +v4l2_async_find_done(struct v4l2_async_notifier *notifier,
> > > +		     struct v4l2_subdev *sd)
> > > +{
> > > +	return __v4l2_async_find_in_list(notifier, sd, &notifier->done_list);
> > > +}
> > > +
> > >  /* Compare two async match descriptors for equivalence */
> > >  static bool v4l2_async_match_equal(struct v4l2_async_match_desc *match1,
> > >  				   struct v4l2_async_match_desc *match2)
> > > @@ -274,13 +289,14 @@ v4l2_async_nf_can_complete(struct v4l2_async_notifier *notifier)
> > >  }
> > >  
> > >  /*
> > > - * Complete the master notifier if possible. This is done when all async
> > > + * Complete the master notifiers if possible. This is done when all async
> > >   * sub-devices have been bound; v4l2_device is also available then.
> > >   */
> > >  static int
> > >  v4l2_async_nf_try_complete(struct v4l2_async_notifier *notifier)
> > >  {
> > > -	struct v4l2_async_notifier *__notifier = notifier;
> > > +	struct v4l2_async_notifier *n;
> > > +	int ret;
> > >  
> > >  	/* Quick check whether there are still more sub-devices here. */
> > >  	if (!list_empty(&notifier->waiting_list))
> > > @@ -290,24 +306,38 @@ v4l2_async_nf_try_complete(struct v4l2_async_notifier *notifier)
> > >  		dev_dbg(notifier_dev(notifier),
> > >  			"v4l2-async: trying to complete\n");
> > >  
> > > -	/* Check the entire notifier tree; find the root notifier first. */
> > > -	while (notifier->parent)
> > > -		notifier = notifier->parent;
> > > +	/*
> > > +	 * Notifiers without a parent are either a subnotifier that have not
> > > +	 * yet been associated with it is a root notifier or a root notifier
> > > +	 * itself. If it is a root notifier try to complete it.
> > > +	 */
> > > +	if (!notifier->parent) {
> > > +		/* This is root if it has v4l2_dev. */
> > > +		if (!notifier->v4l2_dev) {
> > > +			dev_dbg(notifier_dev(notifier),
> > > +				"v4l2-async: V4L2 device not available\n");
> > > +			return 0;
> > > +		}
> > >  
> > > -	/* This is root if it has v4l2_dev. */
> > > -	if (!notifier->v4l2_dev) {
> > > -		dev_dbg(notifier_dev(__notifier),
> > > -			"v4l2-async: V4L2 device not available\n");
> > > -		return 0;
> > > -	}
> > > +		/* Is everything ready? */
> > > +		if (!v4l2_async_nf_can_complete(notifier))
> > > +			return 0;
> > > +
> > > +		dev_dbg(notifier_dev(notifier), "v4l2-async: complete\n");
> > >  
> > > -	/* Is everything ready? */
> > > -	if (!v4l2_async_nf_can_complete(notifier))
> > > -		return 0;
> > > +		return v4l2_async_nf_call_complete(notifier);
> > > +	}
> > >  
> > > -	dev_dbg(notifier_dev(__notifier), "v4l2-async: complete\n");
> > > +	/* Try to complete all notifiers containing the subdevices. */
> > > +	list_for_each_entry(n, &notifier_list, notifier_entry) {
> > > +		if (v4l2_async_find_done(n, notifier->sd)) {
> > > +			ret = v4l2_async_nf_try_complete(n);
> > > +			if (ret)
> > > +				return ret;
> > > +		}
> > > +	}
> > >  
> > > -	return v4l2_async_nf_call_complete(notifier);
> > > +	return 0;
> > >  }
> > >  
> > >  static int
> > 

-- 
Hälsningar,

Sakari Ailus




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux