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

[Philipp Zabel <p.zabel@pengutronix.de>]

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