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

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

 



Hej Sakari,

Gentle ping on this.

On 2024-01-31 11:40:46 +0100, Niklas Söderlund wrote:
> Hej Sakari,
> 
> On 2024-01-31 08:21:13 +0000, Sakari Ailus wrote:
> > Hejssan Niklas,
> > 
> > On Tue, Jan 30, 2024 at 04:40:58PM +0100, Niklas Söderlund wrote:
> > > Hi Sakari,
> > > 
> > > On 2024-01-30 15:27:51 +0000, Sakari Ailus wrote:
> > > > 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...
> > > 
> > > With this fix (or something like it) it works with multiple parents ;-) 
> > > If it's not a bug and we drop the Fixes tag do you think this is a step 
> > > in the right direction? Or shall I drop trying to solve my use-case with 
> > > a solution in this area and focus on trying to work around this 
> > > limitation in the driver?
> > 
> > I'll review the patch properly later today.
> 
> Thanks!
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > 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.
> > > 
> > > Yes, I even tried to move it to probe [2] in 2017 to solve a different 
> > > issue at the time. I have also discussed this in person at various 
> > > conferences around that time. But 2017 was a long time ago and if you 
> > > think it's now OK to register the video device at probe time I will do 
> > > so work around my issue that way. But would be nice with a confirmation 
> > > that this is OK before I move down that route.
> > 
> > Two other drivers are already doing it, I don't see why rcar-vin shouldn't.
> > I'm sure there are others as I checked only two. :-)
> 
> Super! I will move in this direction then as I think it makes more sens 
> to register them in probe and is step in the right direction. I will 
> wait for your review feedback on this patch to see if will make the move 
> before or after the change that spoored this patch.
> 
> > 
> > > 
> > > 2.  https://lore.kernel.org/linux-renesas-soc/20170524001540.13613-16-niklas.soderlund@xxxxxxxxxxxx/
> > 
> > Why is control handler initialisation left to the complete handler?
> 
> Good question, not sure why 2017 version of me thought that was a good 
> idea. Today's version of me knows better and will not try something like 
> that.
> 
> > 
> > > 
> > > > 
> > > > > 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
> > > 
> > > -- 
> > > Kind Regards,
> > > Niklas Söderlund
> > 
> > -- 
> > Hälsningar,
> > 
> > Sakari Ailus
> 
> -- 
> Kind Regards,
> Niklas Söderlund

-- 
Kind Regards,
Niklas Söderlund




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux