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

[Aishwarya Kothari <aishwaryakothari75@gmail.com>]

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