| Message ID | 20240129195954.1110643-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: v4l: async: Fix completion of chained subnotifiers | expand |
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? 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. 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. > > Fixes: 28a1295795d8 ("media: v4l: async: Allow multiple connections between entities") > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > 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, ¬ifier->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, ¬ifier->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, ¬ifier->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(¬ifier->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, ¬ifier_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
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. > > 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 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@ragnatech.se> > > --- > > 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, ¬ifier->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, ¬ifier->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, ¬ifier->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(¬ifier->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, ¬ifier_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 > > -- > Regards, > > Sakari Ailus
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@ragnatech.se> > > > --- > > > 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, ¬ifier->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, ¬ifier->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, ¬ifier->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(¬ifier->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, ¬ifier_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 > >
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? > > > > > > > > > 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. 2. https://lore.kernel.org/linux-renesas-soc/20170524001540.13613-16-niklas.soderlund@ragnatech.se/ > > > 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@ragnatech.se> > > > > --- > > > > 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, ¬ifier->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, ¬ifier->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, ¬ifier->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(¬ifier->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, ¬ifier_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
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. > > > > > > > > > > > > > > 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. :-) > > 2. https://lore.kernel.org/linux-renesas-soc/20170524001540.13613-16-niklas.soderlund@ragnatech.se/ Why is control handler initialisation left to the complete handler? > > > > > > 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@ragnatech.se> > > > > > --- > > > > > 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, ¬ifier->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, ¬ifier->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, ¬ifier->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(¬ifier->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, ¬ifier_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
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@ragnatech.se/ > > 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@ragnatech.se> > > > > > > --- > > > > > > 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, ¬ifier->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, ¬ifier->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, ¬ifier->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(¬ifier->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, ¬ifier_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
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@ragnatech.se/ > > > > 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@ragnatech.se> > > > > > > > --- > > > > > > > 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, ¬ifier->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, ¬ifier->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, ¬ifier->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(¬ifier->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, ¬ifier_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
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, ¬ifier->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, ¬ifier->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, ¬ifier->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(¬ifier->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, ¬ifier_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
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. Fixes: 28a1295795d8 ("media: v4l: async: Allow multiple connections between entities") Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/media/v4l2-core/v4l2-async.c | 68 ++++++++++++++++++++-------- 1 file changed, 49 insertions(+), 19 deletions(-)