| Message ID | 20220810104848.846783-1-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: v4l2-async: fix binding async subdevs with multiple source ports | expand |
Hi, On Wed, Aug 10, 2022 at 12:48:48PM +0200, Philipp Zabel wrote: > Asynchronous subdevice probing on imx-media with imx6-mipi-csi2 is > broken since commit 1f391df44607 ("media: v4l2-async: Use endpoints in > __v4l2_async_nf_add_fwnode_remote()"). > > This is a side effect of imx6-mipi-csi2 having a single subdevice with > four separate source ports connected to different subdevices. Before, > the imx-media-csi and video-mux devices registered async sub-devices > via device fwnode that matched the imx6-mipi-csi2 device on their > respective notifiers. This caused the first asd to be put on the > notifier waiting list, and the other three to return -EEXIST and be > ignored. > > With async subdev registration via endpoint fwnode, all four asds are > distinct and three of them keep dangling on their notifiers after the > first one is bound. > > This patch modifies __v4l2_async_nf_has_async_subdev() to consider > asds matching different fwnode endpoints of the same sub-device equal > if the latter is already bound and matches via device fwnode. Further, > v4l2_async_register_subdev() is modified to remove dangling duplicate > asds that were registered before the sub-device was available to check > its fwnode. > > Fixes: 1f391df44607 ("media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()") > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> Any comments on this? The issue persists on v6.2-rc3. > --- > drivers/media/v4l2-core/v4l2-async.c | 43 ++++++++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 2f1b718a9189..b24220ed7077 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -452,6 +452,22 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > > if (asd_equal(asd, sd->asd)) > return true; > + > + /* > + * If the asd matches an endpoint fwnode, handle sub-devices > + * with device fwnode that were already matched by another asd > + * with a different endpoint fwnode on the same device. > + */ > + if (asd->match_type == V4L2_ASYNC_MATCH_FWNODE && > + fwnode_graph_is_endpoint(asd->match.fwnode) && > + sd->fwnode && !fwnode_graph_is_endpoint(sd->fwnode)) { > + struct fwnode_handle *devnode; > + > + devnode = fwnode_graph_get_port_parent(asd->match.fwnode); > + fwnode_handle_put(devnode); > + if (devnode == sd->fwnode) > + return true; > + } > } > > return false; > @@ -749,6 +765,24 @@ __v4l2_async_nf_add_i2c(struct v4l2_async_notifier *notifier, int adapter_id, > } > EXPORT_SYMBOL_GPL(__v4l2_async_nf_add_i2c); > > +static void v4l2_async_remove_duplicate_matches(struct v4l2_subdev *sd) > +{ > + struct v4l2_async_notifier *notifier; > + > + lockdep_assert_held(&list_lock); > + > + list_for_each_entry(notifier, ¬ifier_list, list) { > + struct v4l2_async_subdev *asd; > + > + asd = v4l2_async_find_match(notifier, sd); > + if (!asd) > + continue; > + > + /* Remove from the waiting list */ > + list_del(&asd->list); > + } > +} > + > int v4l2_async_register_subdev(struct v4l2_subdev *sd) > { > struct v4l2_async_notifier *subdev_notifier; > @@ -783,6 +817,15 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > if (ret) > goto err_unbind; > > + /* > + * At this point the asd is removed from the notifier wait list. > + * There might be other notifiers with asds matching different > + * fwnode endpoints of the same sd remaining. If the sd matches > + * by device fwnode, remove the dangling asds. > + */ > + if (sd->fwnode && !fwnode_graph_is_endpoint(sd->fwnode)) > + v4l2_async_remove_duplicate_matches(sd); > + > ret = v4l2_async_nf_try_complete(notifier); > if (ret) > goto err_unbind; > -- > 2.30.2 > > regards Philipp
Hi Philipp, (CC'ing Francesco) My apologies for not having noticed the patch. On Fri, Jan 13, 2023 at 12:24:56PM +0100, Philipp Zabel wrote: > Hi, > > On Wed, Aug 10, 2022 at 12:48:48PM +0200, Philipp Zabel wrote: > > Asynchronous subdevice probing on imx-media with imx6-mipi-csi2 is > > broken since commit 1f391df44607 ("media: v4l2-async: Use endpoints in > > __v4l2_async_nf_add_fwnode_remote()"). > > > > This is a side effect of imx6-mipi-csi2 having a single subdevice with > > four separate source ports connected to different subdevices. Before, > > the imx-media-csi and video-mux devices registered async sub-devices > > via device fwnode that matched the imx6-mipi-csi2 device on their > > respective notifiers. This caused the first asd to be put on the > > notifier waiting list, and the other three to return -EEXIST and be > > ignored. > > > > With async subdev registration via endpoint fwnode, all four asds are > > distinct and three of them keep dangling on their notifiers after the > > first one is bound. > > > > This patch modifies __v4l2_async_nf_has_async_subdev() to consider > > asds matching different fwnode endpoints of the same sub-device equal > > if the latter is already bound and matches via device fwnode. Further, > > v4l2_async_register_subdev() is modified to remove dangling duplicate > > asds that were registered before the sub-device was available to check > > its fwnode. To make sure I understand this correctly, you need both changes, with the change in __v4l2_async_nf_has_async_subdev() meant to address asds being added after the subdev has been registered, and the change in v4l2_async_register_subdev() meant to address asds that have been added before ? The imx6 ipu drivers implement a "clever hack" to handle the multi-endpoint issue that was never officially supported by v4l2-async. Obviously, as it has worked so far, leaving it broken isn't a very nice option. The fix feels a bit like a hack though, and a better solution would be to allow subdevs to be matched multiple times, by multiple consumers. That's a more intrusive change though, so I could be OK with this as a short term fix, assuming it doesn't break anything else. I would however want to ensure this doesn't get abused by new drivers. Could we add a dev_warn() somewhere to indicate that multi-endpoint matching is not supported and shouldn't be used until fixed ? Sakari, what do you think ? > > Fixes: 1f391df44607 ("media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()") > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > Any comments on this? The issue persists on v6.2-rc3. Francesco, does this fix your issue ? > > --- > > drivers/media/v4l2-core/v4l2-async.c | 43 ++++++++++++++++++++++++++++ > > 1 file changed, 43 insertions(+) > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > index 2f1b718a9189..b24220ed7077 100644 > > --- a/drivers/media/v4l2-core/v4l2-async.c > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > @@ -452,6 +452,22 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > > > > if (asd_equal(asd, sd->asd)) > > return true; > > + > > + /* > > + * If the asd matches an endpoint fwnode, handle sub-devices > > + * with device fwnode that were already matched by another asd > > + * with a different endpoint fwnode on the same device. > > + */ > > + if (asd->match_type == V4L2_ASYNC_MATCH_FWNODE && > > + fwnode_graph_is_endpoint(asd->match.fwnode) && > > + sd->fwnode && !fwnode_graph_is_endpoint(sd->fwnode)) { > > + struct fwnode_handle *devnode; > > + > > + devnode = fwnode_graph_get_port_parent(asd->match.fwnode); > > + fwnode_handle_put(devnode); > > + if (devnode == sd->fwnode) > > + return true; > > + } > > } > > > > return false; > > @@ -749,6 +765,24 @@ __v4l2_async_nf_add_i2c(struct v4l2_async_notifier *notifier, int adapter_id, > > } > > EXPORT_SYMBOL_GPL(__v4l2_async_nf_add_i2c); > > > > +static void v4l2_async_remove_duplicate_matches(struct v4l2_subdev *sd) > > +{ > > + struct v4l2_async_notifier *notifier; > > + > > + lockdep_assert_held(&list_lock); > > + > > + list_for_each_entry(notifier, ¬ifier_list, list) { > > + struct v4l2_async_subdev *asd; > > + > > + asd = v4l2_async_find_match(notifier, sd); > > + if (!asd) > > + continue; > > + > > + /* Remove from the waiting list */ > > + list_del(&asd->list); > > + } > > +} > > + > > int v4l2_async_register_subdev(struct v4l2_subdev *sd) > > { > > struct v4l2_async_notifier *subdev_notifier; > > @@ -783,6 +817,15 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > > if (ret) > > goto err_unbind; > > > > + /* > > + * At this point the asd is removed from the notifier wait list. > > + * There might be other notifiers with asds matching different > > + * fwnode endpoints of the same sd remaining. If the sd matches > > + * by device fwnode, remove the dangling asds. > > + */ > > + if (sd->fwnode && !fwnode_graph_is_endpoint(sd->fwnode)) > > + v4l2_async_remove_duplicate_matches(sd); > > + > > ret = v4l2_async_nf_try_complete(notifier); > > if (ret) > > goto err_unbind;
+ Aishwarya On Mon, Jan 16, 2023 at 03:08:27AM +0200, Laurent Pinchart wrote: > Hi Philipp, > > (CC'ing Francesco) > > My apologies for not having noticed the patch. > > On Fri, Jan 13, 2023 at 12:24:56PM +0100, Philipp Zabel wrote: > > Hi, > > > > On Wed, Aug 10, 2022 at 12:48:48PM +0200, Philipp Zabel wrote: > > > Asynchronous subdevice probing on imx-media with imx6-mipi-csi2 is > > > broken since commit 1f391df44607 ("media: v4l2-async: Use endpoints in > > > __v4l2_async_nf_add_fwnode_remote()"). > > > > > > This is a side effect of imx6-mipi-csi2 having a single subdevice with > > > four separate source ports connected to different subdevices. Before, > > > the imx-media-csi and video-mux devices registered async sub-devices > > > via device fwnode that matched the imx6-mipi-csi2 device on their > > > respective notifiers. This caused the first asd to be put on the > > > notifier waiting list, and the other three to return -EEXIST and be > > > ignored. > > > > > > With async subdev registration via endpoint fwnode, all four asds are > > > distinct and three of them keep dangling on their notifiers after the > > > first one is bound. > > > > > > This patch modifies __v4l2_async_nf_has_async_subdev() to consider > > > asds matching different fwnode endpoints of the same sub-device equal > > > if the latter is already bound and matches via device fwnode. Further, > > > v4l2_async_register_subdev() is modified to remove dangling duplicate > > > asds that were registered before the sub-device was available to check > > > its fwnode. > > To make sure I understand this correctly, you need both changes, with > the change in __v4l2_async_nf_has_async_subdev() meant to address asds > being added after the subdev has been registered, and the change in > v4l2_async_register_subdev() meant to address asds that have been added > before ? > > The imx6 ipu drivers implement a "clever hack" to handle the > multi-endpoint issue that was never officially supported by v4l2-async. > Obviously, as it has worked so far, leaving it broken isn't a very nice > option. The fix feels a bit like a hack though, and a better solution > would be to allow subdevs to be matched multiple times, by multiple > consumers. That's a more intrusive change though, so I could be OK with > this as a short term fix, assuming it doesn't break anything else. > > I would however want to ensure this doesn't get abused by new drivers. > Could we add a dev_warn() somewhere to indicate that multi-endpoint > matching is not supported and shouldn't be used until fixed ? Sakari, > what do you think ? > > > > Fixes: 1f391df44607 ("media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()") > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > > > Any comments on this? The issue persists on v6.2-rc3. > > Francesco, does this fix your issue ? Aishwarya, can you try this patch [0] and verify if it solves the issue? Just provide a Tested-by tag eventually (and hopefully :-) Francesco [0] https://lore.kernel.org/all/20220810104848.846783-1-p.zabel@pengutronix.de/
Hi Philipp, On Fri, Jan 13, 2023 at 12:24:56PM +0100, Philipp Zabel wrote: > Hi, > > On Wed, Aug 10, 2022 at 12:48:48PM +0200, Philipp Zabel wrote: > > Asynchronous subdevice probing on imx-media with imx6-mipi-csi2 is > > broken since commit 1f391df44607 ("media: v4l2-async: Use endpoints in > > __v4l2_async_nf_add_fwnode_remote()"). > > > > This is a side effect of imx6-mipi-csi2 having a single subdevice with > > four separate source ports connected to different subdevices. Before, > > the imx-media-csi and video-mux devices registered async sub-devices > > via device fwnode that matched the imx6-mipi-csi2 device on their > > respective notifiers. This caused the first asd to be put on the > > notifier waiting list, and the other three to return -EEXIST and be > > ignored. > > > > With async subdev registration via endpoint fwnode, all four asds are > > distinct and three of them keep dangling on their notifiers after the > > first one is bound. > > > > This patch modifies __v4l2_async_nf_has_async_subdev() to consider > > asds matching different fwnode endpoints of the same sub-device equal > > if the latter is already bound and matches via device fwnode. Further, > > v4l2_async_register_subdev() is modified to remove dangling duplicate > > asds that were registered before the sub-device was available to check > > its fwnode. > > > > Fixes: 1f391df44607 ("media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()") > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > Any comments on this? The issue persists on v6.2-rc3. My apologies --- this had gotten buried in my inbox. It happens occasionally. I'll try to review this tomorrow.
Hi Laurent, On Mon, Jan 16, 2023 at 03:08:27AM +0200, Laurent Pinchart wrote: [...] > To make sure I understand this correctly, you need both changes, with > the change in __v4l2_async_nf_has_async_subdev() meant to address asds > being added after the subdev has been registered, and the change in > v4l2_async_register_subdev() meant to address asds that have been added > before ? Yes, your understanding is correct. > The imx6 ipu drivers implement a "clever hack" to handle the > multi-endpoint issue that was never officially supported by v4l2-async. That belongs in parentheses all right. I think it might even have been accidental. > Obviously, as it has worked so far, leaving it broken isn't a very nice > option. The fix feels a bit like a hack though, and a better solution > would be to allow subdevs to be matched multiple times, by multiple > consumers. That's a more intrusive change though, so I could be OK with > this as a short term fix, assuming it doesn't break anything else. Right, the intention was that this patch could be applied on stable kernels, as I only had noticed the issue after the release. > I would however want to ensure this doesn't get abused by new drivers. > Could we add a dev_warn() somewhere to indicate that multi-endpoint > matching is not supported and shouldn't be used until fixed ? Sakari, > what do you think ? [...] pr_warn_once() in both places, perhaps? regards Philipp
Hi Philipp, On Mon, Jan 16, 2023 at 02:46:07PM +0100, Philipp Zabel wrote: > On Mon, Jan 16, 2023 at 03:08:27AM +0200, Laurent Pinchart wrote: > [...] > > To make sure I understand this correctly, you need both changes, with > > the change in __v4l2_async_nf_has_async_subdev() meant to address asds > > being added after the subdev has been registered, and the change in > > v4l2_async_register_subdev() meant to address asds that have been added > > before ? > > Yes, your understanding is correct. > > > The imx6 ipu drivers implement a "clever hack" to handle the > > multi-endpoint issue that was never officially supported by v4l2-async. > > That belongs in parentheses all right. I think it might even have been > accidental. There are explicit checks in the video-mux driver and in the ipu driver for -EEXIST, so it was likely done on purpose :-) > > Obviously, as it has worked so far, leaving it broken isn't a very nice > > option. The fix feels a bit like a hack though, and a better solution > > would be to allow subdevs to be matched multiple times, by multiple > > consumers. That's a more intrusive change though, so I could be OK with > > this as a short term fix, assuming it doesn't break anything else. > > Right, the intention was that this patch could be applied on stable > kernels, as I only had noticed the issue after the release. > > > I would however want to ensure this doesn't get abused by new drivers. > > Could we add a dev_warn() somewhere to indicate that multi-endpoint > > matching is not supported and shouldn't be used until fixed ? Sakari, > > what do you think ? > [...] > > pr_warn_once() in both places, perhaps? I'm fine with that.
Hi Philipp, On Wed, Aug 10, 2022 at 12:48:48PM +0200, Philipp Zabel wrote: > Asynchronous subdevice probing on imx-media with imx6-mipi-csi2 is > broken since commit 1f391df44607 ("media: v4l2-async: Use endpoints in > __v4l2_async_nf_add_fwnode_remote()"). > > This is a side effect of imx6-mipi-csi2 having a single subdevice with > four separate source ports connected to different subdevices. Before, > the imx-media-csi and video-mux devices registered async sub-devices > via device fwnode that matched the imx6-mipi-csi2 device on their > respective notifiers. This caused the first asd to be put on the > notifier waiting list, and the other three to return -EEXIST and be > ignored. > > With async subdev registration via endpoint fwnode, all four asds are > distinct and three of them keep dangling on their notifiers after the > first one is bound. > > This patch modifies __v4l2_async_nf_has_async_subdev() to consider > asds matching different fwnode endpoints of the same sub-device equal > if the latter is already bound and matches via device fwnode. Further, > v4l2_async_register_subdev() is modified to remove dangling duplicate > asds that were registered before the sub-device was available to check > its fwnode. > > Fixes: 1f391df44607 ("media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()") > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/media/v4l2-core/v4l2-async.c | 43 ++++++++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 2f1b718a9189..b24220ed7077 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -452,6 +452,22 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > > if (asd_equal(asd, sd->asd)) > return true; > + > + /* > + * If the asd matches an endpoint fwnode, handle sub-devices > + * with device fwnode that were already matched by another asd > + * with a different endpoint fwnode on the same device. > + */ > + if (asd->match_type == V4L2_ASYNC_MATCH_FWNODE && > + fwnode_graph_is_endpoint(asd->match.fwnode) && > + sd->fwnode && !fwnode_graph_is_endpoint(sd->fwnode)) { > + struct fwnode_handle *devnode; > + > + devnode = fwnode_graph_get_port_parent(asd->match.fwnode); > + fwnode_handle_put(devnode); > + if (devnode == sd->fwnode) > + return true; > + } > } > > return false; > @@ -749,6 +765,24 @@ __v4l2_async_nf_add_i2c(struct v4l2_async_notifier *notifier, int adapter_id, > } > EXPORT_SYMBOL_GPL(__v4l2_async_nf_add_i2c); > > +static void v4l2_async_remove_duplicate_matches(struct v4l2_subdev *sd) > +{ > + struct v4l2_async_notifier *notifier; > + > + lockdep_assert_held(&list_lock); > + > + list_for_each_entry(notifier, ¬ifier_list, list) { > + struct v4l2_async_subdev *asd; > + > + asd = v4l2_async_find_match(notifier, sd); > + if (!asd) > + continue; > + > + /* Remove from the waiting list */ > + list_del(&asd->list); This leaves asd->list pointers dangling. > + } > +} > + > int v4l2_async_register_subdev(struct v4l2_subdev *sd) > { > struct v4l2_async_notifier *subdev_notifier; > @@ -783,6 +817,15 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > if (ret) > goto err_unbind; > > + /* > + * At this point the asd is removed from the notifier wait list. > + * There might be other notifiers with asds matching different > + * fwnode endpoints of the same sd remaining. If the sd matches > + * by device fwnode, remove the dangling asds. > + */ > + if (sd->fwnode && !fwnode_graph_is_endpoint(sd->fwnode)) > + v4l2_async_remove_duplicate_matches(sd); > + > ret = v4l2_async_nf_try_complete(notifier); > if (ret) > goto err_unbind; This patch is essentially a workaround, not a fix. The root of the problem is that registering async sub-devices (and thus registering a sub-device later) and link creation (via bound callback) are handled together. They should be separated instead, at least in the v4l2-async framework if not in the interfaces. We could at least have a helper doing the both using existing API for devices that have a single source pad. While merging this might not break anything as such, it would certainly make fixing the underlying problem later on much harder as you'd need to take drivers depending on it into account --- for which I, for instance, don't have hardware for. I'm thus not overly enthusiastic of the idea of merging this.
On Tue, Jan 17, 2023 at 01:16:30PM +0000, Sakari Ailus wrote: > Hi Philipp, > > On Wed, Aug 10, 2022 at 12:48:48PM +0200, Philipp Zabel wrote: > > Asynchronous subdevice probing on imx-media with imx6-mipi-csi2 is > > broken since commit 1f391df44607 ("media: v4l2-async: Use endpoints in > > __v4l2_async_nf_add_fwnode_remote()"). > > > > This is a side effect of imx6-mipi-csi2 having a single subdevice with > > four separate source ports connected to different subdevices. Before, > > the imx-media-csi and video-mux devices registered async sub-devices > > via device fwnode that matched the imx6-mipi-csi2 device on their > > respective notifiers. This caused the first asd to be put on the > > notifier waiting list, and the other three to return -EEXIST and be > > ignored. > > > > With async subdev registration via endpoint fwnode, all four asds are > > distinct and three of them keep dangling on their notifiers after the > > first one is bound. > > > > This patch modifies __v4l2_async_nf_has_async_subdev() to consider > > asds matching different fwnode endpoints of the same sub-device equal > > if the latter is already bound and matches via device fwnode. Further, > > v4l2_async_register_subdev() is modified to remove dangling duplicate > > asds that were registered before the sub-device was available to check > > its fwnode. > > > > Fixes: 1f391df44607 ("media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()") > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > drivers/media/v4l2-core/v4l2-async.c | 43 ++++++++++++++++++++++++++++ > > 1 file changed, 43 insertions(+) > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > index 2f1b718a9189..b24220ed7077 100644 > > --- a/drivers/media/v4l2-core/v4l2-async.c > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > @@ -452,6 +452,22 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > > > > if (asd_equal(asd, sd->asd)) > > return true; > > + > > + /* > > + * If the asd matches an endpoint fwnode, handle sub-devices > > + * with device fwnode that were already matched by another asd > > + * with a different endpoint fwnode on the same device. > > + */ > > + if (asd->match_type == V4L2_ASYNC_MATCH_FWNODE && > > + fwnode_graph_is_endpoint(asd->match.fwnode) && > > + sd->fwnode && !fwnode_graph_is_endpoint(sd->fwnode)) { > > + struct fwnode_handle *devnode; > > + > > + devnode = fwnode_graph_get_port_parent(asd->match.fwnode); > > + fwnode_handle_put(devnode); > > + if (devnode == sd->fwnode) > > + return true; > > + } > > } > > > > return false; > > @@ -749,6 +765,24 @@ __v4l2_async_nf_add_i2c(struct v4l2_async_notifier *notifier, int adapter_id, > > } > > EXPORT_SYMBOL_GPL(__v4l2_async_nf_add_i2c); > > > > +static void v4l2_async_remove_duplicate_matches(struct v4l2_subdev *sd) > > +{ > > + struct v4l2_async_notifier *notifier; > > + > > + lockdep_assert_held(&list_lock); > > + > > + list_for_each_entry(notifier, ¬ifier_list, list) { > > + struct v4l2_async_subdev *asd; > > + > > + asd = v4l2_async_find_match(notifier, sd); > > + if (!asd) > > + continue; > > + > > + /* Remove from the waiting list */ > > + list_del(&asd->list); > > This leaves asd->list pointers dangling. > > > + } > > +} > > + > > int v4l2_async_register_subdev(struct v4l2_subdev *sd) > > { > > struct v4l2_async_notifier *subdev_notifier; > > @@ -783,6 +817,15 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > > if (ret) > > goto err_unbind; > > > > + /* > > + * At this point the asd is removed from the notifier wait list. > > + * There might be other notifiers with asds matching different > > + * fwnode endpoints of the same sd remaining. If the sd matches > > + * by device fwnode, remove the dangling asds. > > + */ > > + if (sd->fwnode && !fwnode_graph_is_endpoint(sd->fwnode)) > > + v4l2_async_remove_duplicate_matches(sd); > > + > > ret = v4l2_async_nf_try_complete(notifier); > > if (ret) > > goto err_unbind; > > This patch is essentially a workaround, not a fix. > > The root of the problem is that registering async sub-devices (and thus > registering a sub-device later) and link creation (via bound callback) are > handled together. I'm not sure to see how that's related to the problem at hand. The issue here is that there are four consumers for one subdevice, and the v4l2-async framework doesn't support that. Two of those consumers could be made to know about each other, but the other two (video-mux) are modelled completely independently. That doesn't seem tied to link creation to me. > They should be separated instead, at least in the > v4l2-async framework if not in the interfaces. We could at least have a > helper doing the both using existing API for devices that have a single > source pad. > > While merging this might not break anything as such, it would certainly > make fixing the underlying problem later on much harder as you'd need to > take drivers depending on it into account --- for which I, for instance, > don't have hardware for. > > I'm thus not overly enthusiastic of the idea of merging this.
Hi, On 17.01.23 14:29, Laurent Pinchart wrote: > On Tue, Jan 17, 2023 at 01:16:30PM +0000, Sakari Ailus wrote: >> Hi Philipp, >> >> On Wed, Aug 10, 2022 at 12:48:48PM +0200, Philipp Zabel wrote: >>> Asynchronous subdevice probing on imx-media with imx6-mipi-csi2 is >>> broken since commit 1f391df44607 ("media: v4l2-async: Use endpoints in >>> __v4l2_async_nf_add_fwnode_remote()"). >>> >>> This is a side effect of imx6-mipi-csi2 having a single subdevice with >>> four separate source ports connected to different subdevices. Before, >>> the imx-media-csi and video-mux devices registered async sub-devices >>> via device fwnode that matched the imx6-mipi-csi2 device on their >>> respective notifiers. This caused the first asd to be put on the >>> notifier waiting list, and the other three to return -EEXIST and be >>> ignored. >>> >>> With async subdev registration via endpoint fwnode, all four asds are >>> distinct and three of them keep dangling on their notifiers after the >>> first one is bound. >>> >>> This patch modifies __v4l2_async_nf_has_async_subdev() to consider >>> asds matching different fwnode endpoints of the same sub-device equal >>> if the latter is already bound and matches via device fwnode. Further, >>> v4l2_async_register_subdev() is modified to remove dangling duplicate >>> asds that were registered before the sub-device was available to check >>> its fwnode. >>> >>> Fixes: 1f391df44607 ("media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()") >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> >>> --- >>> drivers/media/v4l2-core/v4l2-async.c | 43 ++++++++++++++++++++++++++++ >>> 1 file changed, 43 insertions(+) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c >>> index 2f1b718a9189..b24220ed7077 100644 >>> --- a/drivers/media/v4l2-core/v4l2-async.c >>> +++ b/drivers/media/v4l2-core/v4l2-async.c >>> @@ -452,6 +452,22 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, >>> >>> if (asd_equal(asd, sd->asd)) >>> return true; >>> + >>> + /* >>> + * If the asd matches an endpoint fwnode, handle sub-devices >>> + * with device fwnode that were already matched by another asd >>> + * with a different endpoint fwnode on the same device. >>> + */ >>> + if (asd->match_type == V4L2_ASYNC_MATCH_FWNODE && >>> + fwnode_graph_is_endpoint(asd->match.fwnode) && >>> + sd->fwnode && !fwnode_graph_is_endpoint(sd->fwnode)) { >>> + struct fwnode_handle *devnode; >>> + >>> + devnode = fwnode_graph_get_port_parent(asd->match.fwnode); >>> + fwnode_handle_put(devnode); >>> + if (devnode == sd->fwnode) >>> + return true; >>> + } >>> } >>> >>> return false; >>> @@ -749,6 +765,24 @@ __v4l2_async_nf_add_i2c(struct v4l2_async_notifier *notifier, int adapter_id, >>> } >>> EXPORT_SYMBOL_GPL(__v4l2_async_nf_add_i2c); >>> >>> +static void v4l2_async_remove_duplicate_matches(struct v4l2_subdev *sd) >>> +{ >>> + struct v4l2_async_notifier *notifier; >>> + >>> + lockdep_assert_held(&list_lock); >>> + >>> + list_for_each_entry(notifier, ¬ifier_list, list) { >>> + struct v4l2_async_subdev *asd; >>> + >>> + asd = v4l2_async_find_match(notifier, sd); >>> + if (!asd) >>> + continue; >>> + >>> + /* Remove from the waiting list */ >>> + list_del(&asd->list); >> >> This leaves asd->list pointers dangling. >> >>> + } >>> +} >>> + >>> int v4l2_async_register_subdev(struct v4l2_subdev *sd) >>> { >>> struct v4l2_async_notifier *subdev_notifier; >>> @@ -783,6 +817,15 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) >>> if (ret) >>> goto err_unbind; >>> >>> + /* >>> + * At this point the asd is removed from the notifier wait list. >>> + * There might be other notifiers with asds matching different >>> + * fwnode endpoints of the same sd remaining. If the sd matches >>> + * by device fwnode, remove the dangling asds. >>> + */ >>> + if (sd->fwnode && !fwnode_graph_is_endpoint(sd->fwnode)) >>> + v4l2_async_remove_duplicate_matches(sd); >>> + >>> ret = v4l2_async_nf_try_complete(notifier); >>> if (ret) >>> goto err_unbind; >> >> This patch is essentially a workaround, not a fix. >> >> The root of the problem is that registering async sub-devices (and thus >> registering a sub-device later) and link creation (via bound callback) are >> handled together. > > I'm not sure to see how that's related to the problem at hand. The issue > here is that there are four consumers for one subdevice, and the > v4l2-async framework doesn't support that. Two of those consumers could > be made to know about each other, but the other two (video-mux) are > modelled completely independently. That doesn't seem tied to link > creation to me. > >> They should be separated instead, at least in the >> v4l2-async framework if not in the interfaces. We could at least have a >> helper doing the both using existing API for devices that have a single >> source pad. >> >> While merging this might not break anything as such, it would certainly >> make fixing the underlying problem later on much harder as you'd need to >> take drivers depending on it into account --- for which I, for instance, >> don't have hardware for. >> >> I'm thus not overly enthusiastic of the idea of merging this. > I stumbled over the commit 1f391df44607 ("media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()") and started this discussion : https://lore.kernel.org/linux-media/Y8AIRPd4RFYmssal@valkosipuli.retiisi.eu/ I applied this patch on top of the commit c1649ec55708.The hardware used is Apalis iMX6 (i.MX 6Q) with ov5640 mipi-csi2 camera. The /dev/media0 is created and pipeline was configured using below script : root@apalis-imx6-10774951:~# cat ov5640.sh media-ctl -l "'ov5640 1-003c':0 -> 'imx6-mipi-csi2':0[1]" media-ctl -l "'imx6-mipi-csi2':2 -> 'ipu1_csi1':0[1]" media-ctl -l "'ipu1_csi1':2 -> 'ipu1_csi1 capture':0[1]" # Configure pads media-ctl -V "'ov5640 1-003c':0 [fmt:UYVY8_1X16/1920x1080 field:none]" media-ctl -V "'imx6-mipi-csi2':2 [fmt:UYVY8_1X16/1920x1080 field:none]" media-ctl -V "'ipu1_csi1':2 [fmt:UYVY8_1X16/1920x1080 field:none]" But it gives the following error when trying to set up the pipeline: [ 37.211276] ipu1_csi1: entity ov5640 1-003c does not implement get_mbus_config() [ 37.218872] ipu1_csi1: failed to get upstream media bus configuration When adding the missing functionality as follows: diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index e0f908af581b..618c677ec89b 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -3733,6 +3733,17 @@ static int ov5640_init_cfg(struct v4l2_subdev *sd, return 0; } +static int ov5640_get_mbus_config(struct v4l2_subdev *sd, + unsigned int pad, + struct v4l2_mbus_config *cfg) +{ + cfg->type = V4L2_MBUS_CSI2_DPHY; + cfg->bus.mipi_csi2.num_data_lanes = 1; + cfg->bus.mipi_csi2.flags = 0; + + return 0; +} + static const struct v4l2_subdev_core_ops ov5640_core_ops = { .log_status = v4l2_ctrl_subdev_log_status, .subscribe_event = v4l2_ctrl_subdev_subscribe_event, @@ -3753,6 +3764,7 @@ static const struct v4l2_subdev_pad_ops ov5640_pad_ops = { .get_selection = ov5640_get_selection, .enum_frame_size = ov5640_enum_frame_size, .enum_frame_interval = ov5640_enum_frame_interval, + .get_mbus_config = ov5640_get_mbus_config, }; static const struct v4l2_subdev_ops ov5640_subdev_ops = { The script was executed correctly without any errors and the links were created. Now when running the Gstreamer it gives the below output : root@apalis-imx6-10774951:~# gst-launch-1.0 v4l2src device='/dev/video3' ! videoconvert ! waylandsink Setting pipeline to PAUSED ... Pipeline is live and does not need PREROLL ... Pipeline is PREROLLED ... Setting pipeline to PLAYING ... New clock: GstSystemClock [ 192.526110] imx6-mipi-csi2: LP-11 wait timeout, likely a sensor driver bug, expect capture failures. [ 192.535550] imx6-mipi-csi2: phy_state = 0x00000200 [ 192.833456] ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2 [ 195.147977] ipu1_csi1: EOF timeout ERROR: from element /GstPipeline:pipeline0/GstV4l2Src:v4l2src0: Could not read from resource. Additional debug info: ../gst-plugins-good-1.20.3/sys/v4l2/gstv4l2bufferpool.c(1181): gst_v4l2_buffer_pool_poll (): /GstPipeline:pipeline0/GstV4l2Src:v4l2src0: poll error 1: Invalid argument (22) ERROR: from element /GstPipeline:pipeline0/GstV4l2Src:v4l2src0: Failed to allocate a buffer Execution ended after 0:00:03.269527404 Additional debug info: ../gst-plugins-good-1.20.3/sys/v4l2/gstv4l2src.c(1146): gst_v4l2src_create (): /GstPipeline:pipeline0/GstV4l2Src:v4l2src0 Setting pipeline to NULL ... ERROR: from element /GstPipeline:pipeline0/GstV4l2Src:v4l2src0: Internal data stream error. Additional debug info: ../gstreamer-1.20.3/libs/gst/base/gstbasesrc.c(3127): gst_base_src_loop (): /GstPipeline:pipeline0/GstV4l2Src:v4l2src0: streaming stopped, reason error (-5) [ 197.228051] ipu1_csi1: wait last EOF timeout Freeing pipeline ... While going through the dmesg kernel logs I found this : 4.333202] imx6-mipi-csi2 21dc000.mipi: Consider updating driver imx6-mipi-csi2 to match on endpoints [ 4.347001] imx6-mipi-csi2 21dc000.mipi: Consider updating driver imx6-mipi-csi2 to match on endpoints [ 5.173588] video-mux 20e0000.iomuxc-gpr:ipu2_csi1_mux: Consider updating driver video-mux to match on endpoints Any idea why this would fail? Kind regards, Aishwarya
Hi, On Wed, Jan 18, 2023 at 02:14:54PM +0100, Aishwarya Kothari wrote: [...] > I stumbled over the commit 1f391df44607 ("media: v4l2-async: Use endpoints > in __v4l2_async_nf_add_fwnode_remote()") and started this discussion : > https://lore.kernel.org/linux-media/Y8AIRPd4RFYmssal@valkosipuli.retiisi.eu/ > > I applied this patch on top of the commit c1649ec55708.The hardware used is > Apalis iMX6 (i.MX 6Q) with ov5640 mipi-csi2 camera. > > The /dev/media0 is created and pipeline was configured using below script : > root@apalis-imx6-10774951:~# cat ov5640.sh > media-ctl -l "'ov5640 1-003c':0 -> 'imx6-mipi-csi2':0[1]" > media-ctl -l "'imx6-mipi-csi2':2 -> 'ipu1_csi1':0[1]" > media-ctl -l "'ipu1_csi1':2 -> 'ipu1_csi1 capture':0[1]" > # Configure pads > media-ctl -V "'ov5640 1-003c':0 [fmt:UYVY8_1X16/1920x1080 field:none]" > media-ctl -V "'imx6-mipi-csi2':2 [fmt:UYVY8_1X16/1920x1080 field:none]" > media-ctl -V "'ipu1_csi1':2 [fmt:UYVY8_1X16/1920x1080 field:none]" > > But it gives the following error when trying to set up the pipeline: > > [ 37.211276] ipu1_csi1: entity ov5640 1-003c does not implement > get_mbus_config() > [ 37.218872] ipu1_csi1: failed to get upstream media bus configuration > > When adding the missing functionality as follows: > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index e0f908af581b..618c677ec89b 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -3733,6 +3733,17 @@ static int ov5640_init_cfg(struct v4l2_subdev *sd, > return 0; > } > > +static int ov5640_get_mbus_config(struct v4l2_subdev *sd, > + unsigned int pad, > + struct v4l2_mbus_config *cfg) > +{ > + cfg->type = V4L2_MBUS_CSI2_DPHY; > + cfg->bus.mipi_csi2.num_data_lanes = 1; Isn't OV5640 dual-lane by default? > + cfg->bus.mipi_csi2.flags = 0; > + > + return 0; > +} > + > static const struct v4l2_subdev_core_ops ov5640_core_ops = { > .log_status = v4l2_ctrl_subdev_log_status, > .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > @@ -3753,6 +3764,7 @@ static const struct v4l2_subdev_pad_ops ov5640_pad_ops > = { > .get_selection = ov5640_get_selection, > .enum_frame_size = ov5640_enum_frame_size, > .enum_frame_interval = ov5640_enum_frame_interval, > + .get_mbus_config = ov5640_get_mbus_config, > }; > > static const struct v4l2_subdev_ops ov5640_subdev_ops = { > > The script was executed correctly without any errors and the links were > created. Now when running the Gstreamer it gives the below output : > > root@apalis-imx6-10774951:~# gst-launch-1.0 v4l2src device='/dev/video3' ! > videoconvert ! waylandsink > Setting pipeline to PAUSED ... > Pipeline is live and does not need PREROLL ... > Pipeline is PREROLLED ... > Setting pipeline to PLAYING ... > New clock: GstSystemClock > [ 192.526110] imx6-mipi-csi2: LP-11 wait timeout, likely a sensor driver > bug, expect capture failures. > [ 192.535550] imx6-mipi-csi2: phy_state = 0x00000200 This is timing out in the imx6-mipi-csi2 driver, waiting for OV5640 to signal the LP-11 stop state on lane 0 (phy_state bit 4). > [ 192.833456] ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2 A write to the SYS_CTRL0 register failed, presumably trying to clear the software reset or power down bits. Could this be the reason that OV5640 doesn't put the MIPI CSI-2 link into LP-11 as expected? All further errors follow from the timeout above. [...] > While going through the dmesg kernel logs I found this : > > 4.333202] imx6-mipi-csi2 21dc000.mipi: Consider updating driver > imx6-mipi-csi2 to match on endpoints > [ 4.347001] imx6-mipi-csi2 21dc000.mipi: Consider updating driver > imx6-mipi-csi2 to match on endpoints > [ 5.173588] video-mux 20e0000.iomuxc-gpr:ipu2_csi1_mux: Consider updating > driver video-mux to match on endpoints These shouldn't cause any issue. These drivers should be updated to set the subdev->fwnode field to an endpoint fwnode before calling v4l2_async_register_subdev(). regards Philipp
Hi Philipp, On 20.01.23 17:33, Philipp Zabel wrote: > Hi, > > On Wed, Jan 18, 2023 at 02:14:54PM +0100, Aishwarya Kothari wrote: > [...] >> I stumbled over the commit 1f391df44607 ("media: v4l2-async: Use endpoints >> in __v4l2_async_nf_add_fwnode_remote()") and started this discussion : >> https://lore.kernel.org/linux-media/Y8AIRPd4RFYmssal@valkosipuli.retiisi.eu/ >> >> I applied this patch on top of the commit c1649ec55708.The hardware used is >> Apalis iMX6 (i.MX 6Q) with ov5640 mipi-csi2 camera. >> >> The /dev/media0 is created and pipeline was configured using below script : >> root@apalis-imx6-10774951:~# cat ov5640.sh >> media-ctl -l "'ov5640 1-003c':0 -> 'imx6-mipi-csi2':0[1]" >> media-ctl -l "'imx6-mipi-csi2':2 -> 'ipu1_csi1':0[1]" >> media-ctl -l "'ipu1_csi1':2 -> 'ipu1_csi1 capture':0[1]" >> # Configure pads >> media-ctl -V "'ov5640 1-003c':0 [fmt:UYVY8_1X16/1920x1080 field:none]" >> media-ctl -V "'imx6-mipi-csi2':2 [fmt:UYVY8_1X16/1920x1080 field:none]" >> media-ctl -V "'ipu1_csi1':2 [fmt:UYVY8_1X16/1920x1080 field:none]" >> >> But it gives the following error when trying to set up the pipeline: >> >> [ 37.211276] ipu1_csi1: entity ov5640 1-003c does not implement >> get_mbus_config() >> [ 37.218872] ipu1_csi1: failed to get upstream media bus configuration >> >> When adding the missing functionality as follows: >> >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >> index e0f908af581b..618c677ec89b 100644 >> --- a/drivers/media/i2c/ov5640.c >> +++ b/drivers/media/i2c/ov5640.c >> @@ -3733,6 +3733,17 @@ static int ov5640_init_cfg(struct v4l2_subdev *sd, >> return 0; >> } >> >> +static int ov5640_get_mbus_config(struct v4l2_subdev *sd, >> + unsigned int pad, >> + struct v4l2_mbus_config *cfg) >> +{ >> + cfg->type = V4L2_MBUS_CSI2_DPHY; >> + cfg->bus.mipi_csi2.num_data_lanes = 1; > > Isn't OV5640 dual-lane by default? Thank you for pointing this out, I updated the number of data lanes to 2 in this function and applied your patch and everything works now. I tested on top of v6.2-rc5 on an Apalis iMX6Q with an ov5640 camera. Tested-by: Aishwarya Kothari <aishwarya.kothari@toradex.com> > >> + cfg->bus.mipi_csi2.flags = 0; >> + >> + return 0; >> +} >> + >> static const struct v4l2_subdev_core_ops ov5640_core_ops = { >> .log_status = v4l2_ctrl_subdev_log_status, >> .subscribe_event = v4l2_ctrl_subdev_subscribe_event, >> @@ -3753,6 +3764,7 @@ static const struct v4l2_subdev_pad_ops ov5640_pad_ops >> = { >> .get_selection = ov5640_get_selection, >> .enum_frame_size = ov5640_enum_frame_size, >> .enum_frame_interval = ov5640_enum_frame_interval, >> + .get_mbus_config = ov5640_get_mbus_config, >> }; >> >> static const struct v4l2_subdev_ops ov5640_subdev_ops = { >> >> The script was executed correctly without any errors and the links were >> created. Now when running the Gstreamer it gives the below output : >> >> root@apalis-imx6-10774951:~# gst-launch-1.0 v4l2src device='/dev/video3' ! >> videoconvert ! waylandsink >> Setting pipeline to PAUSED ... >> Pipeline is live and does not need PREROLL ... >> Pipeline is PREROLLED ... >> Setting pipeline to PLAYING ... >> New clock: GstSystemClock >> [ 192.526110] imx6-mipi-csi2: LP-11 wait timeout, likely a sensor driver >> bug, expect capture failures. >> [ 192.535550] imx6-mipi-csi2: phy_state = 0x00000200 > > This is timing out in the imx6-mipi-csi2 driver, waiting for OV5640 to > signal the LP-11 stop state on lane 0 (phy_state bit 4). > >> [ 192.833456] ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2 > > A write to the SYS_CTRL0 register failed, presumably trying to clear the > software reset or power down bits. Could this be the reason that OV5640 > doesn't put the MIPI CSI-2 link into LP-11 as expected? > > All further errors follow from the timeout above. > > [...] >> While going through the dmesg kernel logs I found this : >> >> 4.333202] imx6-mipi-csi2 21dc000.mipi: Consider updating driver >> imx6-mipi-csi2 to match on endpoints >> [ 4.347001] imx6-mipi-csi2 21dc000.mipi: Consider updating driver >> imx6-mipi-csi2 to match on endpoints >> [ 5.173588] video-mux 20e0000.iomuxc-gpr:ipu2_csi1_mux: Consider updating >> driver video-mux to match on endpoints > > These shouldn't cause any issue. These drivers should be updated > to set the subdev->fwnode field to an endpoint fwnode before calling > v4l2_async_register_subdev(). > > regards > Philipp > Kind regards, Aishwarya
Hi Laurent, On Tue, Jan 17, 2023 at 03:29:33PM +0200, Laurent Pinchart wrote: > On Tue, Jan 17, 2023 at 01:16:30PM +0000, Sakari Ailus wrote: > > Hi Philipp, > > > > On Wed, Aug 10, 2022 at 12:48:48PM +0200, Philipp Zabel wrote: > > > Asynchronous subdevice probing on imx-media with imx6-mipi-csi2 is > > > broken since commit 1f391df44607 ("media: v4l2-async: Use endpoints in > > > __v4l2_async_nf_add_fwnode_remote()"). > > > > > > This is a side effect of imx6-mipi-csi2 having a single subdevice with > > > four separate source ports connected to different subdevices. Before, > > > the imx-media-csi and video-mux devices registered async sub-devices > > > via device fwnode that matched the imx6-mipi-csi2 device on their > > > respective notifiers. This caused the first asd to be put on the > > > notifier waiting list, and the other three to return -EEXIST and be > > > ignored. > > > > > > With async subdev registration via endpoint fwnode, all four asds are > > > distinct and three of them keep dangling on their notifiers after the > > > first one is bound. > > > > > > This patch modifies __v4l2_async_nf_has_async_subdev() to consider > > > asds matching different fwnode endpoints of the same sub-device equal > > > if the latter is already bound and matches via device fwnode. Further, > > > v4l2_async_register_subdev() is modified to remove dangling duplicate > > > asds that were registered before the sub-device was available to check > > > its fwnode. > > > > > > Fixes: 1f391df44607 ("media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()") > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > > --- > > > drivers/media/v4l2-core/v4l2-async.c | 43 ++++++++++++++++++++++++++++ > > > 1 file changed, 43 insertions(+) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > > index 2f1b718a9189..b24220ed7077 100644 > > > --- a/drivers/media/v4l2-core/v4l2-async.c > > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > > @@ -452,6 +452,22 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > > > > > > if (asd_equal(asd, sd->asd)) > > > return true; > > > + > > > + /* > > > + * If the asd matches an endpoint fwnode, handle sub-devices > > > + * with device fwnode that were already matched by another asd > > > + * with a different endpoint fwnode on the same device. > > > + */ > > > + if (asd->match_type == V4L2_ASYNC_MATCH_FWNODE && > > > + fwnode_graph_is_endpoint(asd->match.fwnode) && > > > + sd->fwnode && !fwnode_graph_is_endpoint(sd->fwnode)) { > > > + struct fwnode_handle *devnode; > > > + > > > + devnode = fwnode_graph_get_port_parent(asd->match.fwnode); > > > + fwnode_handle_put(devnode); > > > + if (devnode == sd->fwnode) > > > + return true; > > > + } > > > } > > > > > > return false; > > > @@ -749,6 +765,24 @@ __v4l2_async_nf_add_i2c(struct v4l2_async_notifier *notifier, int adapter_id, > > > } > > > EXPORT_SYMBOL_GPL(__v4l2_async_nf_add_i2c); > > > > > > +static void v4l2_async_remove_duplicate_matches(struct v4l2_subdev *sd) > > > +{ > > > + struct v4l2_async_notifier *notifier; > > > + > > > + lockdep_assert_held(&list_lock); > > > + > > > + list_for_each_entry(notifier, ¬ifier_list, list) { > > > + struct v4l2_async_subdev *asd; > > > + > > > + asd = v4l2_async_find_match(notifier, sd); > > > + if (!asd) > > > + continue; > > > + > > > + /* Remove from the waiting list */ > > > + list_del(&asd->list); > > > > This leaves asd->list pointers dangling. > > > > > + } > > > +} > > > + > > > int v4l2_async_register_subdev(struct v4l2_subdev *sd) > > > { > > > struct v4l2_async_notifier *subdev_notifier; > > > @@ -783,6 +817,15 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > > > if (ret) > > > goto err_unbind; > > > > > > + /* > > > + * At this point the asd is removed from the notifier wait list. > > > + * There might be other notifiers with asds matching different > > > + * fwnode endpoints of the same sd remaining. If the sd matches > > > + * by device fwnode, remove the dangling asds. > > > + */ > > > + if (sd->fwnode && !fwnode_graph_is_endpoint(sd->fwnode)) > > > + v4l2_async_remove_duplicate_matches(sd); > > > + > > > ret = v4l2_async_nf_try_complete(notifier); > > > if (ret) > > > goto err_unbind; > > > > This patch is essentially a workaround, not a fix. > > > > The root of the problem is that registering async sub-devices (and thus > > registering a sub-device later) and link creation (via bound callback) are > > handled together. > > I'm not sure to see how that's related to the problem at hand. The issue > here is that there are four consumers for one subdevice, and the > v4l2-async framework doesn't support that. Two of those consumers could > be made to know about each other, but the other two (video-mux) are > modelled completely independently. That doesn't seem tied to link > creation to me. It is, partly least. Also async sub-device registration as a sub-device needs to be disconnected from binding a sub-device with a notifier. I don't want to base this on opportunistically removing async sub-devices from the notifier list. That being said, it's been in my plans to improve V4L2 async in these respects. > > > They should be separated instead, at least in the > > v4l2-async framework if not in the interfaces. We could at least have a > > helper doing the both using existing API for devices that have a single > > source pad. > > > > While merging this might not break anything as such, it would certainly > > make fixing the underlying problem later on much harder as you'd need to > > take drivers depending on it into account --- for which I, for instance, > > don't have hardware for. > > > > I'm thus not overly enthusiastic of the idea of merging this.
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c index 2f1b718a9189..b24220ed7077 100644 --- a/drivers/media/v4l2-core/v4l2-async.c +++ b/drivers/media/v4l2-core/v4l2-async.c @@ -452,6 +452,22 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, if (asd_equal(asd, sd->asd)) return true; + + /* + * If the asd matches an endpoint fwnode, handle sub-devices + * with device fwnode that were already matched by another asd + * with a different endpoint fwnode on the same device. + */ + if (asd->match_type == V4L2_ASYNC_MATCH_FWNODE && + fwnode_graph_is_endpoint(asd->match.fwnode) && + sd->fwnode && !fwnode_graph_is_endpoint(sd->fwnode)) { + struct fwnode_handle *devnode; + + devnode = fwnode_graph_get_port_parent(asd->match.fwnode); + fwnode_handle_put(devnode); + if (devnode == sd->fwnode) + return true; + } } return false; @@ -749,6 +765,24 @@ __v4l2_async_nf_add_i2c(struct v4l2_async_notifier *notifier, int adapter_id, } EXPORT_SYMBOL_GPL(__v4l2_async_nf_add_i2c); +static void v4l2_async_remove_duplicate_matches(struct v4l2_subdev *sd) +{ + struct v4l2_async_notifier *notifier; + + lockdep_assert_held(&list_lock); + + list_for_each_entry(notifier, ¬ifier_list, list) { + struct v4l2_async_subdev *asd; + + asd = v4l2_async_find_match(notifier, sd); + if (!asd) + continue; + + /* Remove from the waiting list */ + list_del(&asd->list); + } +} + int v4l2_async_register_subdev(struct v4l2_subdev *sd) { struct v4l2_async_notifier *subdev_notifier; @@ -783,6 +817,15 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) if (ret) goto err_unbind; + /* + * At this point the asd is removed from the notifier wait list. + * There might be other notifiers with asds matching different + * fwnode endpoints of the same sd remaining. If the sd matches + * by device fwnode, remove the dangling asds. + */ + if (sd->fwnode && !fwnode_graph_is_endpoint(sd->fwnode)) + v4l2_async_remove_duplicate_matches(sd); + ret = v4l2_async_nf_try_complete(notifier); if (ret) goto err_unbind;
Asynchronous subdevice probing on imx-media with imx6-mipi-csi2 is broken since commit 1f391df44607 ("media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()"). This is a side effect of imx6-mipi-csi2 having a single subdevice with four separate source ports connected to different subdevices. Before, the imx-media-csi and video-mux devices registered async sub-devices via device fwnode that matched the imx6-mipi-csi2 device on their respective notifiers. This caused the first asd to be put on the notifier waiting list, and the other three to return -EEXIST and be ignored. With async subdev registration via endpoint fwnode, all four asds are distinct and three of them keep dangling on their notifiers after the first one is bound. This patch modifies __v4l2_async_nf_has_async_subdev() to consider asds matching different fwnode endpoints of the same sub-device equal if the latter is already bound and matches via device fwnode. Further, v4l2_async_register_subdev() is modified to remove dangling duplicate asds that were registered before the sub-device was available to check its fwnode. Fixes: 1f391df44607 ("media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()") Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- drivers/media/v4l2-core/v4l2-async.c | 43 ++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)