[PATCH] media: v4l2-async: fix binding async subdevs with multiple source ports

[PATCH] media: v4l2-async: fix binding async subdevs with multiple source ports

[Philipp Zabel]

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, &notifier_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

Re: [PATCH] media: v4l2-async: fix binding async subdevs with multiple source ports

[Philipp Zabel]

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, &notifier_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

Re: [PATCH] media: v4l2-async: fix binding async subdevs with multiple source ports

[Laurent Pinchart]

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, &notifier_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;

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: v4l2-async: fix binding async subdevs with multiple source ports

[Francesco Dolcini]

+ 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/

Re: [PATCH] media: v4l2-async: fix binding async subdevs with multiple source ports

[Sakari Ailus]

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.

-- 
Sakari Ailus

Re: [PATCH] media: v4l2-async: fix binding async subdevs with multiple source ports

[Philipp Zabel]

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

Re: [PATCH] media: v4l2-async: fix binding async subdevs with multiple source ports

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: v4l2-async: fix binding async subdevs with multiple source ports

[Sakari Ailus]

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, &notifier_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.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH] media: v4l2-async: fix binding async subdevs with multiple source ports

[Laurent Pinchart]

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, &notifier_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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: v4l2-async: fix binding async subdevs with multiple source ports

[Aishwarya Kothari]

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, &notifier_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

Re: [PATCH] media: v4l2-async: fix binding async subdevs with multiple source ports

[Philipp Zabel]

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

Re: [PATCH] media: v4l2-async: fix binding async subdevs with multiple source ports

[Aishwarya Kothari]

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

Re: [PATCH] media: v4l2-async: fix binding async subdevs with multiple source ports

[Sakari Ailus]

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, &notifier_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.

-- 
Regards,

Sakari Ailus