| Message ID | 20161007160107.5074-3-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Philipp, On Fri, Oct 07, 2016 at 06:00:47PM +0200, Philipp Zabel wrote: > Currently the v4l2_async_notifier needs to be given a list of matches > for all expected subdevices on creation. When chaining subdevices that > are asynchronously probed via device tree, the bridge device that sets > up the notifier does not know the complete list of subdevices, as it > can only parse its own device tree node to obtain information about > the nearest neighbor subdevices. > To support indirectly connected subdevices, we need to support amending > the existing notifier waiting list with newly found neighbor subdevices > with each registered subdevice. Could you elaborate a little what's the exact use case for this? What kind of a device? Thanks.
Hi Philipp, On Fri, Oct 07, 2016 at 06:00:47PM +0200, Philipp Zabel wrote: > Currently the v4l2_async_notifier needs to be given a list of matches > for all expected subdevices on creation. When chaining subdevices that > are asynchronously probed via device tree, the bridge device that sets > up the notifier does not know the complete list of subdevices, as it > can only parse its own device tree node to obtain information about > the nearest neighbor subdevices. > To support indirectly connected subdevices, we need to support amending > the existing notifier waiting list with newly found neighbor subdevices > with each registered subdevice. > > This can be achieved by adding new v42l_async_subdev matches to the > notifier waiting list during the v4l2_subdev registered callback, which > is called under the list lock from either v4l2_async_register_subdev or > v4l2_async_notifier_register. For this purpose a new exported function > __v4l2_async_notifier_add_subdev is added. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/media/v4l2-core/v4l2-async.c | 78 ++++++++++++++++++++++++++++++++++-- > include/media/v4l2-async.h | 12 ++++++ > 2 files changed, 86 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index c4f1930..404eeea 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -109,6 +109,7 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier, > if (ret < 0) > return ret; > } > + > /* Move from the global subdevice list to notifier's done */ > list_move(&sd->async_list, ¬ifier->done); > > @@ -158,7 +159,7 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, > struct v4l2_async_notifier *notifier) > { > struct v4l2_async_subdev *asd; > - int ret; > + struct list_head *tail; > int i; > > if (!notifier->num_subdevs || notifier->num_subdevs > V4L2_MAX_SUBDEVS) > @@ -191,17 +192,71 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, > /* Keep also completed notifiers on the list */ > list_add(¬ifier->list, ¬ifier_list); > > + do { > + int ret; > + > + tail = notifier->waiting.prev; > + > + ret = v4l2_async_test_notify_all(notifier); > + if (ret < 0) { > + mutex_unlock(&list_lock); > + return ret; > + } > + > + /* > + * If entries were added to the notifier waiting list, check > + * again if the corresponding subdevices are already available. > + */ > + } while (tail != notifier->waiting.prev); > + > mutex_unlock(&list_lock); > > - return ret; > + return 0; > } > EXPORT_SYMBOL(v4l2_async_notifier_register); > > +int __v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier, > + struct v4l2_async_subdev *asd) > +{ > + struct v4l2_async_subdev *tmp_asd; > + > + lockdep_assert_held(&list_lock); > + > + if (asd->match_type != V4L2_ASYNC_MATCH_OF) > + return -EINVAL; > + > + /* > + * First check if the same notifier is already on the waiting or done > + * lists. This can happen if a subdevice with multiple outputs is added > + * by all its downstream subdevices. > + */ > + list_for_each_entry(tmp_asd, ¬ifier->waiting, list) > + if (tmp_asd->match.of.node == asd->match.of.node) > + return 0; > + list_for_each_entry(tmp_asd, ¬ifier->done, list) > + if (tmp_asd->match.of.node == asd->match.of.node) > + return 0; > + > + /* > + * Add the new async subdev to the notifier waiting list, so > + * v4l2_async_belongs may use it to compare against entries in > + * subdev_list. > + * In case the subdev matching asd has already been passed in the > + * subdev_list walk in v4l2_async_notifier_register, or if > + * we are called from v4l2_async_register_subdev, the subdev_list > + * will have to be walked again. > + */ > + list_add_tail(&asd->list, ¬ifier->waiting); > + > + return 0; > +} > + > void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > { > struct v4l2_subdev *sd, *tmp; > - unsigned int notif_n_subdev = notifier->num_subdevs; > - unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS); > + unsigned int notif_n_subdev = 0; > + unsigned int n_subdev; > + struct list_head *list; > struct device **dev; > int i = 0; > > @@ -218,6 +273,10 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > > list_del(¬ifier->list); > > + list_for_each(list, ¬ifier->done) > + ++notif_n_subdev; > + n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS); > + Shouldn't this change go to a separate patch? It seems unrelated. > list_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) { > struct device *d; > > @@ -294,8 +353,19 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > list_for_each_entry(notifier, ¬ifier_list, list) { > struct v4l2_async_subdev *asd = v4l2_async_belongs(notifier, sd); > if (asd) { > + struct list_head *tail = notifier->waiting.prev; > int ret = v4l2_async_test_notify(notifier, sd, asd); > + > + /* > + * If entries were added to the notifier waiting list, > + * check if the corresponding subdevices are already > + * available. > + */ > + if (tail != notifier->waiting.prev) > + ret = v4l2_async_test_notify_all(notifier); > + > mutex_unlock(&list_lock); > + > return ret; > } > } > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > index 8e2a236..e4e4b11 100644 > --- a/include/media/v4l2-async.h > +++ b/include/media/v4l2-async.h > @@ -114,6 +114,18 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, > struct v4l2_async_notifier *notifier); > > /** > + * __v4l2_async_notifier_add_subdev - adds a subdevice to the notifier waitlist > + * > + * @v4l2_notifier: notifier the calling subdev is bound to s/v4l2_// > + * @asd: asynchronous subdev match > + * > + * To be called from inside a subdevices' registered_async callback to add > + * additional subdevices to the notifier waiting list. > + */ > +int __v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier, > + struct v4l2_async_subdev *asd); > + > +/** > * v4l2_async_notifier_unregister - unregisters a subdevice asynchronous notifier > * > * @notifier: pointer to &struct v4l2_async_notifier
On Fri, Oct 7, 2016 at 11:52 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote: > Hi Philipp, > > On Fri, Oct 07, 2016 at 06:00:47PM +0200, Philipp Zabel wrote: >> Currently the v4l2_async_notifier needs to be given a list of matches >> for all expected subdevices on creation. When chaining subdevices that >> are asynchronously probed via device tree, the bridge device that sets >> up the notifier does not know the complete list of subdevices, as it >> can only parse its own device tree node to obtain information about >> the nearest neighbor subdevices. >> To support indirectly connected subdevices, we need to support amending >> the existing notifier waiting list with newly found neighbor subdevices >> with each registered subdevice. > > Could you elaborate a little what's the exact use case for this? What kind > of a device? On i.MX6 there's a DW MIPI CSI2 host -> Mux -> IPU/CSI path and all three are asynchronous subdevices in my patchset and only the last one is directly known to the media device from the device tree, since each driver should only parse its own device tree node an can not follow the of_graph over multiple steps. Another use case I have seen in the wild are external GPIO controlled multiplexers or LVDS serializer/deserializer pairs between a parallel camera and parallel capture interface. In each case the bridge node can only determine its closest neighbor from the device tree (the mux, the LVDS deserializer) but does not know about the indirectly connected device nodes further upstream. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Philipp, On Wed, Oct 12, 2016 at 03:26:48PM +0200, Philipp Zabel wrote: > On Fri, Oct 7, 2016 at 11:52 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > Hi Philipp, > > > > On Fri, Oct 07, 2016 at 06:00:47PM +0200, Philipp Zabel wrote: > >> Currently the v4l2_async_notifier needs to be given a list of matches > >> for all expected subdevices on creation. When chaining subdevices that > >> are asynchronously probed via device tree, the bridge device that sets > >> up the notifier does not know the complete list of subdevices, as it > >> can only parse its own device tree node to obtain information about > >> the nearest neighbor subdevices. > >> To support indirectly connected subdevices, we need to support amending > >> the existing notifier waiting list with newly found neighbor subdevices > >> with each registered subdevice. > > > > Could you elaborate a little what's the exact use case for this? What kind > > of a device? > > On i.MX6 there's a > > DW MIPI CSI2 host -> Mux -> IPU/CSI > > path and all three are asynchronous subdevices in my patchset and only > the last one is directly known to the media device from the device > tree, since each driver should only parse its own device tree node an > can not follow the of_graph over multiple steps. Ok. Are all these devices part of the SoC? Is the mux doing something else than just sitting in between the two? :-) > Another use case I have seen in the wild are external GPIO controlled > multiplexers or LVDS serializer/deserializer pairs between a parallel > camera and parallel capture interface. In each case the bridge node > can only determine its closest neighbor from the device tree (the mux, > the LVDS deserializer) but does not know about the indirectly > connected device nodes further upstream. Yeah, true.
Am Donnerstag, den 13.10.2016, 14:56 +0300 schrieb Sakari Ailus: > Hi Philipp, > > On Wed, Oct 12, 2016 at 03:26:48PM +0200, Philipp Zabel wrote: > > On Fri, Oct 7, 2016 at 11:52 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > Hi Philipp, > > > > > > On Fri, Oct 07, 2016 at 06:00:47PM +0200, Philipp Zabel wrote: > > >> Currently the v4l2_async_notifier needs to be given a list of matches > > >> for all expected subdevices on creation. When chaining subdevices that > > >> are asynchronously probed via device tree, the bridge device that sets > > >> up the notifier does not know the complete list of subdevices, as it > > >> can only parse its own device tree node to obtain information about > > >> the nearest neighbor subdevices. > > >> To support indirectly connected subdevices, we need to support amending > > >> the existing notifier waiting list with newly found neighbor subdevices > > >> with each registered subdevice. > > > > > > Could you elaborate a little what's the exact use case for this? What kind > > > of a device? > > > > On i.MX6 there's a > > > > DW MIPI CSI2 host -> Mux -> IPU/CSI > > > > path and all three are asynchronous subdevices in my patchset and only > > the last one is directly known to the media device from the device > > tree, since each driver should only parse its own device tree node an > > can not follow the of_graph over multiple steps. > > Ok. Are all these devices part of the SoC? Is the mux doing something else > than just sitting in between the two? :-) Yes, in this case the muxes are part of the SoC. Depending on the SoC variant they have a different number of inputs, and they are controlled via the IOMUXC block that also controls the pin configuration. They don't have any additional functionality beyond selecting one active input. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Samstag, den 08.10.2016, 01:43 +0300 schrieb Sakari Ailus: [...] > > void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > > { > > struct v4l2_subdev *sd, *tmp; > > - unsigned int notif_n_subdev = notifier->num_subdevs; > > - unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS); > > + unsigned int notif_n_subdev = 0; > > + unsigned int n_subdev; > > + struct list_head *list; > > struct device **dev; > > int i = 0; > > > > @@ -218,6 +273,10 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > > > > list_del(¬ifier->list); > > > > + list_for_each(list, ¬ifier->done) > > + ++notif_n_subdev; > > + n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS); > > + > > Shouldn't this change go to a separate patch? It seems unrelated. Thanks, this was intended to count the notifier done list instead of relying on notifier->num_subdevs because of the additional asynchronous subdevs added to the notifier that are not part of the original array. Unfortunately this change is a few lines too late, it belongs before the device cache is allocated. I'll fix this and add a comment. I don't want to increment notifier->num_subdevs in __v4l2_async_notifier_add_subdev because the caller of v4l2_async_notifier_register might still use it to measure the original array. > > list_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) { > > struct device *d; > > > > @@ -294,8 +353,19 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > > list_for_each_entry(notifier, ¬ifier_list, list) { > > struct v4l2_async_subdev *asd = v4l2_async_belongs(notifier, sd); > > if (asd) { > > + struct list_head *tail = notifier->waiting.prev; > > int ret = v4l2_async_test_notify(notifier, sd, asd); > > + > > + /* > > + * If entries were added to the notifier waiting list, > > + * check if the corresponding subdevices are already > > + * available. > > + */ > > + if (tail != notifier->waiting.prev) > > + ret = v4l2_async_test_notify_all(notifier); > > + > > mutex_unlock(&list_lock); > > + > > return ret; > > } > > } > > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > > index 8e2a236..e4e4b11 100644 > > --- a/include/media/v4l2-async.h > > +++ b/include/media/v4l2-async.h > > @@ -114,6 +114,18 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, > > struct v4l2_async_notifier *notifier); > > > > /** > > + * __v4l2_async_notifier_add_subdev - adds a subdevice to the notifier waitlist > > + * > > + * @v4l2_notifier: notifier the calling subdev is bound to > > s/v4l2_// I'd be happy to, but why should the v4l2 prefix be removed? regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14/10/16 16:48, Philipp Zabel wrote: > Am Samstag, den 08.10.2016, 01:43 +0300 schrieb Sakari Ailus: > [...] > [...] >>> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h >>> index 8e2a236..e4e4b11 100644 >>> --- a/include/media/v4l2-async.h >>> +++ b/include/media/v4l2-async.h >>> @@ -114,6 +114,18 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, >>> struct v4l2_async_notifier *notifier); >>> >>> /** >>> + * __v4l2_async_notifier_add_subdev - adds a subdevice to the notifier waitlist >>> + * >>> + * @v4l2_notifier: notifier the calling subdev is bound to >> s/v4l2_// > I'd be happy to, but why should the v4l2 prefix be removed? > > regards > Philipp I think Sakari is just pointing out that the comment doesn't match the function argument name. Regards, Ian -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Freitag, den 14.10.2016, 18:06 +0100 schrieb Ian Arkver: > On 14/10/16 16:48, Philipp Zabel wrote: > > Am Samstag, den 08.10.2016, 01:43 +0300 schrieb Sakari Ailus: > > [...] > > [...] > >>> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > >>> index 8e2a236..e4e4b11 100644 > >>> --- a/include/media/v4l2-async.h > >>> +++ b/include/media/v4l2-async.h > >>> @@ -114,6 +114,18 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, > >>> struct v4l2_async_notifier *notifier); > >>> > >>> /** > >>> + * __v4l2_async_notifier_add_subdev - adds a subdevice to the notifier waitlist > >>> + * > >>> + * @v4l2_notifier: notifier the calling subdev is bound to > >> s/v4l2_// > > I'd be happy to, but why should the v4l2 prefix be removed? > > > > regards > > Philipp > I think Sakari is just pointing out that the comment doesn't match the > function argument name. Ouch, that's very obvious now that I understand :) Thank you for pointing this out. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c index c4f1930..404eeea 100644 --- a/drivers/media/v4l2-core/v4l2-async.c +++ b/drivers/media/v4l2-core/v4l2-async.c @@ -109,6 +109,7 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier, if (ret < 0) return ret; } + /* Move from the global subdevice list to notifier's done */ list_move(&sd->async_list, ¬ifier->done); @@ -158,7 +159,7 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, struct v4l2_async_notifier *notifier) { struct v4l2_async_subdev *asd; - int ret; + struct list_head *tail; int i; if (!notifier->num_subdevs || notifier->num_subdevs > V4L2_MAX_SUBDEVS) @@ -191,17 +192,71 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, /* Keep also completed notifiers on the list */ list_add(¬ifier->list, ¬ifier_list); + do { + int ret; + + tail = notifier->waiting.prev; + + ret = v4l2_async_test_notify_all(notifier); + if (ret < 0) { + mutex_unlock(&list_lock); + return ret; + } + + /* + * If entries were added to the notifier waiting list, check + * again if the corresponding subdevices are already available. + */ + } while (tail != notifier->waiting.prev); + mutex_unlock(&list_lock); - return ret; + return 0; } EXPORT_SYMBOL(v4l2_async_notifier_register); +int __v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier, + struct v4l2_async_subdev *asd) +{ + struct v4l2_async_subdev *tmp_asd; + + lockdep_assert_held(&list_lock); + + if (asd->match_type != V4L2_ASYNC_MATCH_OF) + return -EINVAL; + + /* + * First check if the same notifier is already on the waiting or done + * lists. This can happen if a subdevice with multiple outputs is added + * by all its downstream subdevices. + */ + list_for_each_entry(tmp_asd, ¬ifier->waiting, list) + if (tmp_asd->match.of.node == asd->match.of.node) + return 0; + list_for_each_entry(tmp_asd, ¬ifier->done, list) + if (tmp_asd->match.of.node == asd->match.of.node) + return 0; + + /* + * Add the new async subdev to the notifier waiting list, so + * v4l2_async_belongs may use it to compare against entries in + * subdev_list. + * In case the subdev matching asd has already been passed in the + * subdev_list walk in v4l2_async_notifier_register, or if + * we are called from v4l2_async_register_subdev, the subdev_list + * will have to be walked again. + */ + list_add_tail(&asd->list, ¬ifier->waiting); + + return 0; +} + void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) { struct v4l2_subdev *sd, *tmp; - unsigned int notif_n_subdev = notifier->num_subdevs; - unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS); + unsigned int notif_n_subdev = 0; + unsigned int n_subdev; + struct list_head *list; struct device **dev; int i = 0; @@ -218,6 +273,10 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) list_del(¬ifier->list); + list_for_each(list, ¬ifier->done) + ++notif_n_subdev; + n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS); + list_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) { struct device *d; @@ -294,8 +353,19 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) list_for_each_entry(notifier, ¬ifier_list, list) { struct v4l2_async_subdev *asd = v4l2_async_belongs(notifier, sd); if (asd) { + struct list_head *tail = notifier->waiting.prev; int ret = v4l2_async_test_notify(notifier, sd, asd); + + /* + * If entries were added to the notifier waiting list, + * check if the corresponding subdevices are already + * available. + */ + if (tail != notifier->waiting.prev) + ret = v4l2_async_test_notify_all(notifier); + mutex_unlock(&list_lock); + return ret; } } diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h index 8e2a236..e4e4b11 100644 --- a/include/media/v4l2-async.h +++ b/include/media/v4l2-async.h @@ -114,6 +114,18 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, struct v4l2_async_notifier *notifier); /** + * __v4l2_async_notifier_add_subdev - adds a subdevice to the notifier waitlist + * + * @v4l2_notifier: notifier the calling subdev is bound to + * @asd: asynchronous subdev match + * + * To be called from inside a subdevices' registered_async callback to add + * additional subdevices to the notifier waiting list. + */ +int __v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier, + struct v4l2_async_subdev *asd); + +/** * v4l2_async_notifier_unregister - unregisters a subdevice asynchronous notifier * * @notifier: pointer to &struct v4l2_async_notifier
Currently the v4l2_async_notifier needs to be given a list of matches for all expected subdevices on creation. When chaining subdevices that are asynchronously probed via device tree, the bridge device that sets up the notifier does not know the complete list of subdevices, as it can only parse its own device tree node to obtain information about the nearest neighbor subdevices. To support indirectly connected subdevices, we need to support amending the existing notifier waiting list with newly found neighbor subdevices with each registered subdevice. This can be achieved by adding new v42l_async_subdev matches to the notifier waiting list during the v4l2_subdev registered callback, which is called under the list lock from either v4l2_async_register_subdev or v4l2_async_notifier_register. For this purpose a new exported function __v4l2_async_notifier_add_subdev is added. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- drivers/media/v4l2-core/v4l2-async.c | 78 ++++++++++++++++++++++++++++++++++-- include/media/v4l2-async.h | 12 ++++++ 2 files changed, 86 insertions(+), 4 deletions(-)