[PATCH] media: imx6-mipi-csi2: Call remote subdev get_mbus_config to get active lanes

[PATCH] media: imx6-mipi-csi2: Call remote subdev get_mbus_config to get active lanes

[Ezequiel Garcia]

Currently, the CSI2 subdevice is using the data-lanes from the
neareast endpoint to config the CSI2 lanes.

While this may work, the proper way to configure the hardware is
to obtain the remote subdevice in v4l2_async_notifier_operations.bound(),
and then call get_mbus_config using the remote subdevice to get
the active lanes.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/staging/media/imx/TODO             |  12 ---
 drivers/staging/media/imx/imx6-mipi-csi2.c | 101 ++++++++++++++++++---
 2 files changed, 90 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/media/imx/TODO b/drivers/staging/media/imx/TODO
index 9cfc1c1e78dc..c575f419204a 100644
--- a/drivers/staging/media/imx/TODO
+++ b/drivers/staging/media/imx/TODO
@@ -2,18 +2,6 @@
 - The Frame Interval Monitor could be exported to v4l2-core for
   general use.
 
-- The CSI subdevice parses its nearest upstream neighbor's device-tree
-  bus config in order to setup the CSI. Laurent Pinchart argues that
-  instead the CSI subdev should call its neighbor's g_mbus_config op
-  (which should be propagated if necessary) to get this info. However
-  Hans Verkuil is planning to remove the g_mbus_config op. For now this
-  driver uses the parsed DT bus config method until this issue is
-  resolved.
-
-  2020-06: g_mbus has been removed in favour of the get_mbus_config pad
-  operation which should be used to avoid parsing the remote endpoint
-  configuration.
-
 - This media driver supports inheriting V4L2 controls to the
   video capture devices, from the subdevices in the capture device's
   pipeline. The controls for each capture device are updated in the
diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
index 94d87d27d389..bf6a61dd34c2 100644
--- a/drivers/staging/media/imx/imx6-mipi-csi2.c
+++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
@@ -42,7 +42,10 @@ struct csi2_dev {
 	struct clk             *pllref_clk;
 	struct clk             *pix_clk; /* what is this? */
 	void __iomem           *base;
-	struct v4l2_fwnode_bus_mipi_csi2 bus;
+
+	struct v4l2_subdev	*remote;
+	unsigned int		remote_pad;
+	unsigned short		data_lanes;
 
 	/* lock to protect all members below */
 	struct mutex lock;
@@ -138,10 +141,8 @@ static void csi2_enable(struct csi2_dev *csi2, bool enable)
 	}
 }
 
-static void csi2_set_lanes(struct csi2_dev *csi2)
+static void csi2_set_lanes(struct csi2_dev *csi2, unsigned int lanes)
 {
-	int lanes = csi2->bus.num_data_lanes;
-
 	writel(lanes - 1, csi2->base + CSI2_N_LANES);
 }
 
@@ -250,12 +251,13 @@ static int __maybe_unused csi2_dphy_wait_ulp(struct csi2_dev *csi2)
 }
 
 /* Waits for low-power LP-11 state on data and clock lanes. */
-static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
+static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2,
+				     unsigned int lanes)
 {
 	u32 mask, reg;
 	int ret;
 
-	mask = PHY_STOPSTATECLK | (((1 << csi2->bus.num_data_lanes) - 1) <<
+	mask = PHY_STOPSTATECLK | (((1 << lanes) - 1) <<
 				   PHY_STOPSTATEDATA_BIT);
 
 	ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
@@ -300,8 +302,56 @@ static void csi2ipu_gasket_init(struct csi2_dev *csi2)
 	writel(reg, csi2->base + CSI2IPU_GASKET);
 }
 
+static int csi2_get_active_lanes(struct csi2_dev *csi2, unsigned int *lanes)
+{
+	struct v4l2_mbus_config mbus_config = { 0 };
+	unsigned int num_lanes = UINT_MAX;
+	int ret;
+
+	*lanes = csi2->data_lanes;
+
+	ret = v4l2_subdev_call(csi2->remote, pad, get_mbus_config,
+			       csi2->remote_pad, &mbus_config);
+	if (ret == -ENOIOCTLCMD) {
+		dev_dbg(csi2->dev, "No remote mbus configuration available\n");
+		return 0;
+	}
+
+	if (ret) {
+		dev_err(csi2->dev, "Failed to get remote mbus configuration\n");
+		return ret;
+	}
+
+	if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
+		dev_err(csi2->dev, "Unsupported media bus type %u\n",
+			mbus_config.type);
+		return -EINVAL;
+	}
+
+	if (mbus_config.flags & V4L2_MBUS_CSI2_1_LANE)
+		num_lanes = 1;
+	else if (mbus_config.flags & V4L2_MBUS_CSI2_2_LANE)
+		num_lanes = 2;
+	else if (mbus_config.flags & V4L2_MBUS_CSI2_3_LANE)
+		num_lanes = 3;
+	else if (mbus_config.flags & V4L2_MBUS_CSI2_4_LANE)
+		num_lanes = 4;
+
+	if (num_lanes > csi2->data_lanes) {
+		dev_err(csi2->dev,
+			"Unsupported mbus config: too many data lanes %u\n",
+			num_lanes);
+		return -EINVAL;
+	}
+
+	*lanes = num_lanes;
+
+	return 0;
+}
+
 static int csi2_start(struct csi2_dev *csi2)
 {
+	unsigned int lanes;
 	int ret;
 
 	ret = clk_prepare_enable(csi2->pix_clk);
@@ -316,12 +366,16 @@ static int csi2_start(struct csi2_dev *csi2)
 	if (ret)
 		goto err_disable_clk;
 
+	ret = csi2_get_active_lanes(csi2, &lanes);
+	if (ret)
+		goto err_disable_clk;
+
 	/* Step 4 */
-	csi2_set_lanes(csi2);
+	csi2_set_lanes(csi2, lanes);
 	csi2_enable(csi2, true);
 
 	/* Step 5 */
-	csi2_dphy_wait_stopstate(csi2);
+	csi2_dphy_wait_stopstate(csi2, lanes);
 
 	/* Step 6 */
 	ret = v4l2_subdev_call(csi2->src_sd, video, s_stream, 1);
@@ -544,12 +598,37 @@ static int csi2_notify_bound(struct v4l2_async_notifier *notifier,
 {
 	struct csi2_dev *csi2 = notifier_to_dev(notifier);
 	struct media_pad *sink = &csi2->sd.entity.pads[CSI2_SINK_PAD];
+	int pad;
+
+	pad = media_entity_get_fwnode_pad(&sd->entity, asd->match.fwnode,
+					  MEDIA_PAD_FL_SOURCE);
+	if (pad < 0) {
+		dev_err(csi2->dev, "Failed to find pad for %s\n", sd->name);
+		return pad;
+	}
+
+	csi2->remote = sd;
+	csi2->remote_pad = pad;
+
+	dev_dbg(csi2->dev, "Bound %s pad: %d\n", sd->name, pad);
 
 	return v4l2_create_fwnode_links_to_pad(sd, sink);
 }
 
+static void csi2_notify_unbind(struct v4l2_async_notifier *notifier,
+			       struct v4l2_subdev *sd,
+			       struct v4l2_async_subdev *asd)
+{
+	struct csi2_dev *csi2 = notifier_to_dev(notifier);
+
+	csi2->remote = NULL;
+
+	dev_dbg(csi2->dev, "Unbind %s\n", sd->name);
+}
+
 static const struct v4l2_async_notifier_operations csi2_notify_ops = {
 	.bound = csi2_notify_bound,
+	.unbind = csi2_notify_unbind,
 };
 
 static int csi2_async_register(struct csi2_dev *csi2)
@@ -572,10 +651,10 @@ static int csi2_async_register(struct csi2_dev *csi2)
 	if (ret)
 		goto err_parse;
 
-	csi2->bus = vep.bus.mipi_csi2;
+	csi2->data_lanes = vep.bus.mipi_csi2.num_data_lanes;
 
-	dev_dbg(csi2->dev, "data lanes: %d\n", csi2->bus.num_data_lanes);
-	dev_dbg(csi2->dev, "flags: 0x%08x\n", csi2->bus.flags);
+	dev_dbg(csi2->dev, "data lanes: %d\n", vep.bus.mipi_csi2.num_data_lanes);
+	dev_dbg(csi2->dev, "flags: 0x%08x\n", vep.bus.mipi_csi2.flags);
 
 	asd = kzalloc(sizeof(*asd), GFP_KERNEL);
 	if (!asd) {
-- 
2.29.2

Re: [PATCH] media: imx6-mipi-csi2: Call remote subdev get_mbus_config to get active lanes

[Laurent Pinchart]

Hi Ezequiel,

Thank you for the patch.

On Tue, Dec 29, 2020 at 07:31:02AM -0300, Ezequiel Garcia wrote:
> Currently, the CSI2 subdevice is using the data-lanes from the
> neareast endpoint to config the CSI2 lanes.
> 
> While this may work, the proper way to configure the hardware is
> to obtain the remote subdevice in v4l2_async_notifier_operations.bound(),
> and then call get_mbus_config using the remote subdevice to get
> the active lanes.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/staging/media/imx/TODO             |  12 ---
>  drivers/staging/media/imx/imx6-mipi-csi2.c | 101 ++++++++++++++++++---
>  2 files changed, 90 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/TODO b/drivers/staging/media/imx/TODO
> index 9cfc1c1e78dc..c575f419204a 100644
> --- a/drivers/staging/media/imx/TODO
> +++ b/drivers/staging/media/imx/TODO
> @@ -2,18 +2,6 @@
>  - The Frame Interval Monitor could be exported to v4l2-core for
>    general use.
>  
> -- The CSI subdevice parses its nearest upstream neighbor's device-tree
> -  bus config in order to setup the CSI. Laurent Pinchart argues that
> -  instead the CSI subdev should call its neighbor's g_mbus_config op
> -  (which should be propagated if necessary) to get this info. However
> -  Hans Verkuil is planning to remove the g_mbus_config op. For now this
> -  driver uses the parsed DT bus config method until this issue is
> -  resolved.
> -
> -  2020-06: g_mbus has been removed in favour of the get_mbus_config pad
> -  operation which should be used to avoid parsing the remote endpoint
> -  configuration.
> -
>  - This media driver supports inheriting V4L2 controls to the
>    video capture devices, from the subdevices in the capture device's
>    pipeline. The controls for each capture device are updated in the
> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
> index 94d87d27d389..bf6a61dd34c2 100644
> --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
> +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
> @@ -42,7 +42,10 @@ struct csi2_dev {
>  	struct clk             *pllref_clk;
>  	struct clk             *pix_clk; /* what is this? */
>  	void __iomem           *base;
> -	struct v4l2_fwnode_bus_mipi_csi2 bus;
> +
> +	struct v4l2_subdev	*remote;
> +	unsigned int		remote_pad;
> +	unsigned short		data_lanes;
>  
>  	/* lock to protect all members below */
>  	struct mutex lock;
> @@ -138,10 +141,8 @@ static void csi2_enable(struct csi2_dev *csi2, bool enable)
>  	}
>  }
>  
> -static void csi2_set_lanes(struct csi2_dev *csi2)
> +static void csi2_set_lanes(struct csi2_dev *csi2, unsigned int lanes)
>  {
> -	int lanes = csi2->bus.num_data_lanes;
> -
>  	writel(lanes - 1, csi2->base + CSI2_N_LANES);
>  }
>  
> @@ -250,12 +251,13 @@ static int __maybe_unused csi2_dphy_wait_ulp(struct csi2_dev *csi2)
>  }
>  
>  /* Waits for low-power LP-11 state on data and clock lanes. */
> -static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
> +static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2,
> +				     unsigned int lanes)
>  {
>  	u32 mask, reg;
>  	int ret;
>  
> -	mask = PHY_STOPSTATECLK | (((1 << csi2->bus.num_data_lanes) - 1) <<
> +	mask = PHY_STOPSTATECLK | (((1 << lanes) - 1) <<
>  				   PHY_STOPSTATEDATA_BIT);

This now holds on a single line.

>  
>  	ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
> @@ -300,8 +302,56 @@ static void csi2ipu_gasket_init(struct csi2_dev *csi2)
>  	writel(reg, csi2->base + CSI2IPU_GASKET);
>  }
>  
> +static int csi2_get_active_lanes(struct csi2_dev *csi2, unsigned int *lanes)

The function could return the number of lanes, instead of using an
output parameter. Up to you.

> +{
> +	struct v4l2_mbus_config mbus_config = { 0 };
> +	unsigned int num_lanes = UINT_MAX;
> +	int ret;
> +
> +	*lanes = csi2->data_lanes;
> +
> +	ret = v4l2_subdev_call(csi2->remote, pad, get_mbus_config,
> +			       csi2->remote_pad, &mbus_config);
> +	if (ret == -ENOIOCTLCMD) {
> +		dev_dbg(csi2->dev, "No remote mbus configuration available\n");
> +		return 0;
> +	}
> +
> +	if (ret) {
> +		dev_err(csi2->dev, "Failed to get remote mbus configuration\n");
> +		return ret;
> +	}
> +
> +	if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
> +		dev_err(csi2->dev, "Unsupported media bus type %u\n",
> +			mbus_config.type);
> +		return -EINVAL;
> +	}
> +
> +	if (mbus_config.flags & V4L2_MBUS_CSI2_1_LANE)
> +		num_lanes = 1;
> +	else if (mbus_config.flags & V4L2_MBUS_CSI2_2_LANE)
> +		num_lanes = 2;
> +	else if (mbus_config.flags & V4L2_MBUS_CSI2_3_LANE)
> +		num_lanes = 3;
> +	else if (mbus_config.flags & V4L2_MBUS_CSI2_4_LANE)
> +		num_lanes = 4;
> +
> +	if (num_lanes > csi2->data_lanes) {
> +		dev_err(csi2->dev,
> +			"Unsupported mbus config: too many data lanes %u\n",
> +			num_lanes);
> +		return -EINVAL;
> +	}
> +
> +	*lanes = num_lanes;
> +
> +	return 0;
> +}

It could make sense to move this to a core V4L2 helper, but it can be
done later.

> +
>  static int csi2_start(struct csi2_dev *csi2)
>  {
> +	unsigned int lanes;
>  	int ret;
>  
>  	ret = clk_prepare_enable(csi2->pix_clk);
> @@ -316,12 +366,16 @@ static int csi2_start(struct csi2_dev *csi2)
>  	if (ret)
>  		goto err_disable_clk;
>  
> +	ret = csi2_get_active_lanes(csi2, &lanes);
> +	if (ret)
> +		goto err_disable_clk;
> +
>  	/* Step 4 */
> -	csi2_set_lanes(csi2);
> +	csi2_set_lanes(csi2, lanes);
>  	csi2_enable(csi2, true);
>  
>  	/* Step 5 */
> -	csi2_dphy_wait_stopstate(csi2);
> +	csi2_dphy_wait_stopstate(csi2, lanes);
>  
>  	/* Step 6 */
>  	ret = v4l2_subdev_call(csi2->src_sd, video, s_stream, 1);
> @@ -544,12 +598,37 @@ static int csi2_notify_bound(struct v4l2_async_notifier *notifier,
>  {
>  	struct csi2_dev *csi2 = notifier_to_dev(notifier);
>  	struct media_pad *sink = &csi2->sd.entity.pads[CSI2_SINK_PAD];
> +	int pad;
> +
> +	pad = media_entity_get_fwnode_pad(&sd->entity, asd->match.fwnode,
> +					  MEDIA_PAD_FL_SOURCE);
> +	if (pad < 0) {
> +		dev_err(csi2->dev, "Failed to find pad for %s\n", sd->name);
> +		return pad;
> +	}
> +
> +	csi2->remote = sd;
> +	csi2->remote_pad = pad;
> +
> +	dev_dbg(csi2->dev, "Bound %s pad: %d\n", sd->name, pad);
>  
>  	return v4l2_create_fwnode_links_to_pad(sd, sink);
>  }
>  
> +static void csi2_notify_unbind(struct v4l2_async_notifier *notifier,
> +			       struct v4l2_subdev *sd,
> +			       struct v4l2_async_subdev *asd)
> +{
> +	struct csi2_dev *csi2 = notifier_to_dev(notifier);
> +
> +	csi2->remote = NULL;
> +
> +	dev_dbg(csi2->dev, "Unbind %s\n", sd->name);

I'm not sure if this debug message is useful, I think I'd drop it.

With these small issues addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +}
> +
>  static const struct v4l2_async_notifier_operations csi2_notify_ops = {
>  	.bound = csi2_notify_bound,
> +	.unbind = csi2_notify_unbind,
>  };
>  
>  static int csi2_async_register(struct csi2_dev *csi2)
> @@ -572,10 +651,10 @@ static int csi2_async_register(struct csi2_dev *csi2)
>  	if (ret)
>  		goto err_parse;
>  
> -	csi2->bus = vep.bus.mipi_csi2;
> +	csi2->data_lanes = vep.bus.mipi_csi2.num_data_lanes;
>  
> -	dev_dbg(csi2->dev, "data lanes: %d\n", csi2->bus.num_data_lanes);
> -	dev_dbg(csi2->dev, "flags: 0x%08x\n", csi2->bus.flags);
> +	dev_dbg(csi2->dev, "data lanes: %d\n", vep.bus.mipi_csi2.num_data_lanes);
> +	dev_dbg(csi2->dev, "flags: 0x%08x\n", vep.bus.mipi_csi2.flags);
>  
>  	asd = kzalloc(sizeof(*asd), GFP_KERNEL);
>  	if (!asd) {

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: imx6-mipi-csi2: Call remote subdev get_mbus_config to get active lanes

[Ezequiel Garcia]

On Tue, 2020-12-29 at 16:26 +0200, Laurent Pinchart wrote:
> Hi Ezequiel,
> 
> Thank you for the patch.
> 
> On Tue, Dec 29, 2020 at 07:31:02AM -0300, Ezequiel Garcia wrote:
> > Currently, the CSI2 subdevice is using the data-lanes from the
> > neareast endpoint to config the CSI2 lanes.
> > 
> > While this may work, the proper way to configure the hardware is
> > to obtain the remote subdevice in v4l2_async_notifier_operations.bound(),
> > and then call get_mbus_config using the remote subdevice to get
> > the active lanes.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/staging/media/imx/TODO             |  12 ---
> >  drivers/staging/media/imx/imx6-mipi-csi2.c | 101 ++++++++++++++++++---
> >  2 files changed, 90 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/staging/media/imx/TODO b/drivers/staging/media/imx/TODO
> > index 9cfc1c1e78dc..c575f419204a 100644
> > --- a/drivers/staging/media/imx/TODO
> > +++ b/drivers/staging/media/imx/TODO
> > @@ -2,18 +2,6 @@
> >  - The Frame Interval Monitor could be exported to v4l2-core for
> >    general use.
> >  
> > -- The CSI subdevice parses its nearest upstream neighbor's device-tree
> > -  bus config in order to setup the CSI. Laurent Pinchart argues that
> > -  instead the CSI subdev should call its neighbor's g_mbus_config op
> > -  (which should be propagated if necessary) to get this info. However
> > -  Hans Verkuil is planning to remove the g_mbus_config op. For now this
> > -  driver uses the parsed DT bus config method until this issue is
> > -  resolved.
> > -
> > -  2020-06: g_mbus has been removed in favour of the get_mbus_config pad
> > -  operation which should be used to avoid parsing the remote endpoint
> > -  configuration.
> > -
> >  - This media driver supports inheriting V4L2 controls to the
> >    video capture devices, from the subdevices in the capture device's
> >    pipeline. The controls for each capture device are updated in the
> > diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
> > index 94d87d27d389..bf6a61dd34c2 100644
> > --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
> > +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
> > @@ -42,7 +42,10 @@ struct csi2_dev {
> >         struct clk             *pllref_clk;
> >         struct clk             *pix_clk; /* what is this? */
> >         void __iomem           *base;
> > -       struct v4l2_fwnode_bus_mipi_csi2 bus;
> > +
> > +       struct v4l2_subdev      *remote;
> > +       unsigned int            remote_pad;
> > +       unsigned short          data_lanes;
> >  
> >         /* lock to protect all members below */
> >         struct mutex lock;
> > @@ -138,10 +141,8 @@ static void csi2_enable(struct csi2_dev *csi2, bool enable)
> >         }
> >  }
> >  
> > -static void csi2_set_lanes(struct csi2_dev *csi2)
> > +static void csi2_set_lanes(struct csi2_dev *csi2, unsigned int lanes)
> >  {
> > -       int lanes = csi2->bus.num_data_lanes;
> > -
> >         writel(lanes - 1, csi2->base + CSI2_N_LANES);
> >  }
> >  
> > @@ -250,12 +251,13 @@ static int __maybe_unused csi2_dphy_wait_ulp(struct csi2_dev *csi2)
> >  }
> >  
> >  /* Waits for low-power LP-11 state on data and clock lanes. */
> > -static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
> > +static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2,
> > +                                    unsigned int lanes)
> >  {
> >         u32 mask, reg;
> >         int ret;
> >  
> > -       mask = PHY_STOPSTATECLK | (((1 << csi2->bus.num_data_lanes) - 1) <<
> > +       mask = PHY_STOPSTATECLK | (((1 << lanes) - 1) <<
> >                                    PHY_STOPSTATEDATA_BIT);
> 
> This now holds on a single line.
> 
> >  
> >         ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
> > @@ -300,8 +302,56 @@ static void csi2ipu_gasket_init(struct csi2_dev *csi2)
> >         writel(reg, csi2->base + CSI2IPU_GASKET);
> >  }
> >  
> > +static int csi2_get_active_lanes(struct csi2_dev *csi2, unsigned int *lanes)
> 
> The function could return the number of lanes, instead of using an
> output parameter. Up to you.
> 

Do you think the checks (num_lanes > csi2->data_lanes, and
type != V4L2_MBUS_CSI2_DPHY) should be moved out
of the function?

> > +{
> > +       struct v4l2_mbus_config mbus_config = { 0 };
> > +       unsigned int num_lanes = UINT_MAX;
> > +       int ret;
> > +
> > +       *lanes = csi2->data_lanes;
> > +
> > +       ret = v4l2_subdev_call(csi2->remote, pad, get_mbus_config,
> > +                              csi2->remote_pad, &mbus_config);
> > +       if (ret == -ENOIOCTLCMD) {
> > +               dev_dbg(csi2->dev, "No remote mbus configuration available\n");
> > +               return 0;
> > +       }
> > +
> > +       if (ret) {
> > +               dev_err(csi2->dev, "Failed to get remote mbus configuration\n");
> > +               return ret;
> > +       }
> > +
> > +       if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
> > +               dev_err(csi2->dev, "Unsupported media bus type %u\n",
> > +                       mbus_config.type);
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (mbus_config.flags & V4L2_MBUS_CSI2_1_LANE)
> > +               num_lanes = 1;
> > +       else if (mbus_config.flags & V4L2_MBUS_CSI2_2_LANE)
> > +               num_lanes = 2;
> > +       else if (mbus_config.flags & V4L2_MBUS_CSI2_3_LANE)
> > +               num_lanes = 3;
> > +       else if (mbus_config.flags & V4L2_MBUS_CSI2_4_LANE)
> > +               num_lanes = 4;
> > +
> > +       if (num_lanes > csi2->data_lanes) {
> > +               dev_err(csi2->dev,
> > +                       "Unsupported mbus config: too many data lanes %u\n",
> > +                       num_lanes);
> > +               return -EINVAL;
> > +       }
> > +
> > +       *lanes = num_lanes;
> > +
> > +       return 0;
> > +}
> 
> It could make sense to move this to a core V4L2 helper, but it can be
> done later.
> 
> > +
> >  static int csi2_start(struct csi2_dev *csi2)
> >  {
> > +       unsigned int lanes;
> >         int ret;
> >  
> >         ret = clk_prepare_enable(csi2->pix_clk);
> > @@ -316,12 +366,16 @@ static int csi2_start(struct csi2_dev *csi2)
> >         if (ret)
> >                 goto err_disable_clk;
> >  
> > +       ret = csi2_get_active_lanes(csi2, &lanes);
> > +       if (ret)
> > +               goto err_disable_clk;
> > +
> >         /* Step 4 */
> > -       csi2_set_lanes(csi2);
> > +       csi2_set_lanes(csi2, lanes);
> >         csi2_enable(csi2, true);
> >  
> >         /* Step 5 */
> > -       csi2_dphy_wait_stopstate(csi2);
> > +       csi2_dphy_wait_stopstate(csi2, lanes);
> >  
> >         /* Step 6 */
> >         ret = v4l2_subdev_call(csi2->src_sd, video, s_stream, 1);
> > @@ -544,12 +598,37 @@ static int csi2_notify_bound(struct v4l2_async_notifier *notifier,
> >  {
> >         struct csi2_dev *csi2 = notifier_to_dev(notifier);
> >         struct media_pad *sink = &csi2->sd.entity.pads[CSI2_SINK_PAD];
> > +       int pad;
> > +
> > +       pad = media_entity_get_fwnode_pad(&sd->entity, asd->match.fwnode,
> > +                                         MEDIA_PAD_FL_SOURCE);
> > +       if (pad < 0) {
> > +               dev_err(csi2->dev, "Failed to find pad for %s\n", sd->name);
> > +               return pad;
> > +       }
> > +
> > +       csi2->remote = sd;
> > +       csi2->remote_pad = pad;
> > +
> > +       dev_dbg(csi2->dev, "Bound %s pad: %d\n", sd->name, pad);
> >  
> >         return v4l2_create_fwnode_links_to_pad(sd, sink);
> >  }
> >  
> > +static void csi2_notify_unbind(struct v4l2_async_notifier *notifier,
> > +                              struct v4l2_subdev *sd,
> > +                              struct v4l2_async_subdev *asd)
> > +{
> > +       struct csi2_dev *csi2 = notifier_to_dev(notifier);
> > +
> > +       csi2->remote = NULL;
> > +
> > +       dev_dbg(csi2->dev, "Unbind %s\n", sd->name);
> 
> I'm not sure if this debug message is useful, I think I'd drop it.
> 

Sure.

> With these small issues addressed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 

Thanks,
Ezequiel

Re: [PATCH] media: imx6-mipi-csi2: Call remote subdev get_mbus_config to get active lanes

[Laurent Pinchart]

Hi Ezequiel,

On Tue, Dec 29, 2020 at 02:59:35PM -0300, Ezequiel Garcia wrote:
> On Tue, 2020-12-29 at 16:26 +0200, Laurent Pinchart wrote:
> > On Tue, Dec 29, 2020 at 07:31:02AM -0300, Ezequiel Garcia wrote:
> > > Currently, the CSI2 subdevice is using the data-lanes from the
> > > neareast endpoint to config the CSI2 lanes.
> > > 
> > > While this may work, the proper way to configure the hardware is
> > > to obtain the remote subdevice in v4l2_async_notifier_operations.bound(),
> > > and then call get_mbus_config using the remote subdevice to get
> > > the active lanes.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > ---
> > >  drivers/staging/media/imx/TODO             |  12 ---
> > >  drivers/staging/media/imx/imx6-mipi-csi2.c | 101 ++++++++++++++++++---
> > >  2 files changed, 90 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/imx/TODO b/drivers/staging/media/imx/TODO
> > > index 9cfc1c1e78dc..c575f419204a 100644
> > > --- a/drivers/staging/media/imx/TODO
> > > +++ b/drivers/staging/media/imx/TODO
> > > @@ -2,18 +2,6 @@
> > >  - The Frame Interval Monitor could be exported to v4l2-core for
> > >    general use.
> > >  
> > > -- The CSI subdevice parses its nearest upstream neighbor's device-tree
> > > -  bus config in order to setup the CSI. Laurent Pinchart argues that
> > > -  instead the CSI subdev should call its neighbor's g_mbus_config op
> > > -  (which should be propagated if necessary) to get this info. However
> > > -  Hans Verkuil is planning to remove the g_mbus_config op. For now this
> > > -  driver uses the parsed DT bus config method until this issue is
> > > -  resolved.
> > > -
> > > -  2020-06: g_mbus has been removed in favour of the get_mbus_config pad
> > > -  operation which should be used to avoid parsing the remote endpoint
> > > -  configuration.
> > > -
> > >  - This media driver supports inheriting V4L2 controls to the
> > >    video capture devices, from the subdevices in the capture device's
> > >    pipeline. The controls for each capture device are updated in the
> > > diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
> > > index 94d87d27d389..bf6a61dd34c2 100644
> > > --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
> > > +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
> > > @@ -42,7 +42,10 @@ struct csi2_dev {
> > >         struct clk             *pllref_clk;
> > >         struct clk             *pix_clk; /* what is this? */
> > >         void __iomem           *base;
> > > -       struct v4l2_fwnode_bus_mipi_csi2 bus;
> > > +
> > > +       struct v4l2_subdev      *remote;
> > > +       unsigned int            remote_pad;
> > > +       unsigned short          data_lanes;
> > >  
> > >         /* lock to protect all members below */
> > >         struct mutex lock;
> > > @@ -138,10 +141,8 @@ static void csi2_enable(struct csi2_dev *csi2, bool enable)
> > >         }
> > >  }
> > >  
> > > -static void csi2_set_lanes(struct csi2_dev *csi2)
> > > +static void csi2_set_lanes(struct csi2_dev *csi2, unsigned int lanes)
> > >  {
> > > -       int lanes = csi2->bus.num_data_lanes;
> > > -
> > >         writel(lanes - 1, csi2->base + CSI2_N_LANES);
> > >  }
> > >  
> > > @@ -250,12 +251,13 @@ static int __maybe_unused csi2_dphy_wait_ulp(struct csi2_dev *csi2)
> > >  }
> > >  
> > >  /* Waits for low-power LP-11 state on data and clock lanes. */
> > > -static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
> > > +static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2,
> > > +                                    unsigned int lanes)
> > >  {
> > >         u32 mask, reg;
> > >         int ret;
> > >  
> > > -       mask = PHY_STOPSTATECLK | (((1 << csi2->bus.num_data_lanes) - 1) <<
> > > +       mask = PHY_STOPSTATECLK | (((1 << lanes) - 1) <<
> > >                                    PHY_STOPSTATEDATA_BIT);
> > 
> > This now holds on a single line.
> > 
> > >  
> > >         ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
> > > @@ -300,8 +302,56 @@ static void csi2ipu_gasket_init(struct csi2_dev *csi2)
> > >         writel(reg, csi2->base + CSI2IPU_GASKET);
> > >  }
> > >  
> > > +static int csi2_get_active_lanes(struct csi2_dev *csi2, unsigned int *lanes)
> > 
> > The function could return the number of lanes, instead of using an
> > output parameter. Up to you.
> > 
> 
> Do you think the checks (num_lanes > csi2->data_lanes, and
> type != V4L2_MBUS_CSI2_DPHY) should be moved out
> of the function?

That's one option, but I was more thinking about returning a < 0 value
in case of error, and a > 0 value in case of success.

> > > +{
> > > +       struct v4l2_mbus_config mbus_config = { 0 };
> > > +       unsigned int num_lanes = UINT_MAX;
> > > +       int ret;
> > > +
> > > +       *lanes = csi2->data_lanes;
> > > +
> > > +       ret = v4l2_subdev_call(csi2->remote, pad, get_mbus_config,
> > > +                              csi2->remote_pad, &mbus_config);
> > > +       if (ret == -ENOIOCTLCMD) {
> > > +               dev_dbg(csi2->dev, "No remote mbus configuration available\n");
> > > +               return 0;
> > > +       }
> > > +
> > > +       if (ret) {
> > > +               dev_err(csi2->dev, "Failed to get remote mbus configuration\n");
> > > +               return ret;
> > > +       }
> > > +
> > > +       if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
> > > +               dev_err(csi2->dev, "Unsupported media bus type %u\n",
> > > +                       mbus_config.type);
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       if (mbus_config.flags & V4L2_MBUS_CSI2_1_LANE)
> > > +               num_lanes = 1;
> > > +       else if (mbus_config.flags & V4L2_MBUS_CSI2_2_LANE)
> > > +               num_lanes = 2;
> > > +       else if (mbus_config.flags & V4L2_MBUS_CSI2_3_LANE)
> > > +               num_lanes = 3;
> > > +       else if (mbus_config.flags & V4L2_MBUS_CSI2_4_LANE)
> > > +               num_lanes = 4;
> > > +
> > > +       if (num_lanes > csi2->data_lanes) {
> > > +               dev_err(csi2->dev,
> > > +                       "Unsupported mbus config: too many data lanes %u\n",
> > > +                       num_lanes);
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       *lanes = num_lanes;
> > > +
> > > +       return 0;
> > > +}
> > 
> > It could make sense to move this to a core V4L2 helper, but it can be
> > done later.
> > 
> > > +
> > >  static int csi2_start(struct csi2_dev *csi2)
> > >  {
> > > +       unsigned int lanes;
> > >         int ret;
> > >  
> > >         ret = clk_prepare_enable(csi2->pix_clk);
> > > @@ -316,12 +366,16 @@ static int csi2_start(struct csi2_dev *csi2)
> > >         if (ret)
> > >                 goto err_disable_clk;
> > >  
> > > +       ret = csi2_get_active_lanes(csi2, &lanes);
> > > +       if (ret)
> > > +               goto err_disable_clk;
> > > +
> > >         /* Step 4 */
> > > -       csi2_set_lanes(csi2);
> > > +       csi2_set_lanes(csi2, lanes);
> > >         csi2_enable(csi2, true);
> > >  
> > >         /* Step 5 */
> > > -       csi2_dphy_wait_stopstate(csi2);
> > > +       csi2_dphy_wait_stopstate(csi2, lanes);
> > >  
> > >         /* Step 6 */
> > >         ret = v4l2_subdev_call(csi2->src_sd, video, s_stream, 1);
> > > @@ -544,12 +598,37 @@ static int csi2_notify_bound(struct v4l2_async_notifier *notifier,
> > >  {
> > >         struct csi2_dev *csi2 = notifier_to_dev(notifier);
> > >         struct media_pad *sink = &csi2->sd.entity.pads[CSI2_SINK_PAD];
> > > +       int pad;
> > > +
> > > +       pad = media_entity_get_fwnode_pad(&sd->entity, asd->match.fwnode,
> > > +                                         MEDIA_PAD_FL_SOURCE);
> > > +       if (pad < 0) {
> > > +               dev_err(csi2->dev, "Failed to find pad for %s\n", sd->name);
> > > +               return pad;
> > > +       }
> > > +
> > > +       csi2->remote = sd;
> > > +       csi2->remote_pad = pad;
> > > +
> > > +       dev_dbg(csi2->dev, "Bound %s pad: %d\n", sd->name, pad);
> > >  
> > >         return v4l2_create_fwnode_links_to_pad(sd, sink);
> > >  }
> > >  
> > > +static void csi2_notify_unbind(struct v4l2_async_notifier *notifier,
> > > +                              struct v4l2_subdev *sd,
> > > +                              struct v4l2_async_subdev *asd)
> > > +{
> > > +       struct csi2_dev *csi2 = notifier_to_dev(notifier);
> > > +
> > > +       csi2->remote = NULL;
> > > +
> > > +       dev_dbg(csi2->dev, "Unbind %s\n", sd->name);
> > 
> > I'm not sure if this debug message is useful, I think I'd drop it.
> 
> Sure.
> 
> > With these small issues addressed,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: imx6-mipi-csi2: Call remote subdev get_mbus_config to get active lanes

[Steve Longerbeam]

On 12/29/20 2:31 AM, Ezequiel Garcia wrote:
> Currently, the CSI2 subdevice is using the data-lanes from the
> neareast endpoint to config the CSI2 lanes.
>
> While this may work, the proper way to configure the hardware is
> to obtain the remote subdevice in v4l2_async_notifier_operations.bound(),
> and then call get_mbus_config using the remote subdevice to get
> the active lanes.
>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>   drivers/staging/media/imx/TODO             |  12 ---
>   drivers/staging/media/imx/imx6-mipi-csi2.c | 101 ++++++++++++++++++---
>   2 files changed, 90 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/staging/media/imx/TODO b/drivers/staging/media/imx/TODO
> index 9cfc1c1e78dc..c575f419204a 100644
> --- a/drivers/staging/media/imx/TODO
> +++ b/drivers/staging/media/imx/TODO
> @@ -2,18 +2,6 @@
>   - The Frame Interval Monitor could be exported to v4l2-core for
>     general use.
>   
> -- The CSI subdevice parses its nearest upstream neighbor's device-tree
> -  bus config in order to setup the CSI. Laurent Pinchart argues that
> -  instead the CSI subdev should call its neighbor's g_mbus_config op
> -  (which should be propagated if necessary) to get this info. However
> -  Hans Verkuil is planning to remove the g_mbus_config op. For now this
> -  driver uses the parsed DT bus config method until this issue is
> -  resolved.

This TODO was actually referring to the fwnode endpoint parsing in 
imx-media-csi.c, not imx6-mipi-csi2.c.

But the same conversion to call .get_mbus_config() instead of endpoint 
parsing could be done in imx-media-csi.c, but there is one imx6 
constraint that is preventing this from happening. The imx6 reference 
manual states that if the CSI is receiving from an input parallel bus 
that is 16-bits wide, the data must go directly to memory via the SMFC 
and not be sent to the IPU's Image Converter ("passthrough" mode):

"37.4.3.9 16 bit camera support

Devices that support 16 bit data bus can be connected to the CSI. This 
can be done in one
of the following ways.

16 bit YUV422
In this mode the CSI receives 2 components per cycle. The CSI is 
programmed to
accept the data as 16 bit generic data. The captured data will be stored 
in the memory
through the SMFC. The IDMAC needs to be programmed to store 16bit 
generic data.
When the data is read back from the memory for further processing in the 
IPU it will
be read as YUV422 data."

Same is said for RGB data to the CSI.

I'm not sure if this restriction is real or not. If this restriction 
were ignored, the fwnode endpoint check "ep->bus.parallel.bus_width >= 
16" could be removed and the only remaining info required to determine 
passthrough mode is available from 'struct v4l2_mbus_config' and the 
input mbus codes, thus allowing the conversion to .get_mbus_config().

Steve


> -
> -  2020-06: g_mbus has been removed in favour of the get_mbus_config pad
> -  operation which should be used to avoid parsing the remote endpoint
> -  configuration.
> -
>   - This media driver supports inheriting V4L2 controls to the
>     video capture devices, from the subdevices in the capture device's
>     pipeline. The controls for each capture device are updated in the
> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
> index 94d87d27d389..bf6a61dd34c2 100644
> --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
> +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
> @@ -42,7 +42,10 @@ struct csi2_dev {
>   	struct clk             *pllref_clk;
>   	struct clk             *pix_clk; /* what is this? */
>   	void __iomem           *base;
> -	struct v4l2_fwnode_bus_mipi_csi2 bus;
> +
> +	struct v4l2_subdev	*remote;
> +	unsigned int		remote_pad;
> +	unsigned short		data_lanes;
>   
>   	/* lock to protect all members below */
>   	struct mutex lock;
> @@ -138,10 +141,8 @@ static void csi2_enable(struct csi2_dev *csi2, bool enable)
>   	}
>   }
>   
> -static void csi2_set_lanes(struct csi2_dev *csi2)
> +static void csi2_set_lanes(struct csi2_dev *csi2, unsigned int lanes)
>   {
> -	int lanes = csi2->bus.num_data_lanes;
> -
>   	writel(lanes - 1, csi2->base + CSI2_N_LANES);
>   }
>   
> @@ -250,12 +251,13 @@ static int __maybe_unused csi2_dphy_wait_ulp(struct csi2_dev *csi2)
>   }
>   
>   /* Waits for low-power LP-11 state on data and clock lanes. */
> -static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
> +static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2,
> +				     unsigned int lanes)
>   {
>   	u32 mask, reg;
>   	int ret;
>   
> -	mask = PHY_STOPSTATECLK | (((1 << csi2->bus.num_data_lanes) - 1) <<
> +	mask = PHY_STOPSTATECLK | (((1 << lanes) - 1) <<
>   				   PHY_STOPSTATEDATA_BIT);
>   
>   	ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
> @@ -300,8 +302,56 @@ static void csi2ipu_gasket_init(struct csi2_dev *csi2)
>   	writel(reg, csi2->base + CSI2IPU_GASKET);
>   }
>   
> +static int csi2_get_active_lanes(struct csi2_dev *csi2, unsigned int *lanes)
> +{
> +	struct v4l2_mbus_config mbus_config = { 0 };
> +	unsigned int num_lanes = UINT_MAX;
> +	int ret;
> +
> +	*lanes = csi2->data_lanes;
> +
> +	ret = v4l2_subdev_call(csi2->remote, pad, get_mbus_config,
> +			       csi2->remote_pad, &mbus_config);
> +	if (ret == -ENOIOCTLCMD) {
> +		dev_dbg(csi2->dev, "No remote mbus configuration available\n");
> +		return 0;
> +	}
> +
> +	if (ret) {
> +		dev_err(csi2->dev, "Failed to get remote mbus configuration\n");
> +		return ret;
> +	}
> +
> +	if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
> +		dev_err(csi2->dev, "Unsupported media bus type %u\n",
> +			mbus_config.type);
> +		return -EINVAL;
> +	}
> +
> +	if (mbus_config.flags & V4L2_MBUS_CSI2_1_LANE)
> +		num_lanes = 1;
> +	else if (mbus_config.flags & V4L2_MBUS_CSI2_2_LANE)
> +		num_lanes = 2;
> +	else if (mbus_config.flags & V4L2_MBUS_CSI2_3_LANE)
> +		num_lanes = 3;
> +	else if (mbus_config.flags & V4L2_MBUS_CSI2_4_LANE)
> +		num_lanes = 4;
> +
> +	if (num_lanes > csi2->data_lanes) {
> +		dev_err(csi2->dev,
> +			"Unsupported mbus config: too many data lanes %u\n",
> +			num_lanes);
> +		return -EINVAL;
> +	}
> +
> +	*lanes = num_lanes;
> +
> +	return 0;
> +}
> +
>   static int csi2_start(struct csi2_dev *csi2)
>   {
> +	unsigned int lanes;
>   	int ret;
>   
>   	ret = clk_prepare_enable(csi2->pix_clk);
> @@ -316,12 +366,16 @@ static int csi2_start(struct csi2_dev *csi2)
>   	if (ret)
>   		goto err_disable_clk;
>   
> +	ret = csi2_get_active_lanes(csi2, &lanes);
> +	if (ret)
> +		goto err_disable_clk;
> +
>   	/* Step 4 */
> -	csi2_set_lanes(csi2);
> +	csi2_set_lanes(csi2, lanes);
>   	csi2_enable(csi2, true);
>   
>   	/* Step 5 */
> -	csi2_dphy_wait_stopstate(csi2);
> +	csi2_dphy_wait_stopstate(csi2, lanes);
>   
>   	/* Step 6 */
>   	ret = v4l2_subdev_call(csi2->src_sd, video, s_stream, 1);
> @@ -544,12 +598,37 @@ static int csi2_notify_bound(struct v4l2_async_notifier *notifier,
>   {
>   	struct csi2_dev *csi2 = notifier_to_dev(notifier);
>   	struct media_pad *sink = &csi2->sd.entity.pads[CSI2_SINK_PAD];
> +	int pad;
> +
> +	pad = media_entity_get_fwnode_pad(&sd->entity, asd->match.fwnode,
> +					  MEDIA_PAD_FL_SOURCE);
> +	if (pad < 0) {
> +		dev_err(csi2->dev, "Failed to find pad for %s\n", sd->name);
> +		return pad;
> +	}
> +
> +	csi2->remote = sd;
> +	csi2->remote_pad = pad;
> +
> +	dev_dbg(csi2->dev, "Bound %s pad: %d\n", sd->name, pad);
>   
>   	return v4l2_create_fwnode_links_to_pad(sd, sink);
>   }
>   
> +static void csi2_notify_unbind(struct v4l2_async_notifier *notifier,
> +			       struct v4l2_subdev *sd,
> +			       struct v4l2_async_subdev *asd)
> +{
> +	struct csi2_dev *csi2 = notifier_to_dev(notifier);
> +
> +	csi2->remote = NULL;
> +
> +	dev_dbg(csi2->dev, "Unbind %s\n", sd->name);
> +}
> +
>   static const struct v4l2_async_notifier_operations csi2_notify_ops = {
>   	.bound = csi2_notify_bound,
> +	.unbind = csi2_notify_unbind,
>   };
>   
>   static int csi2_async_register(struct csi2_dev *csi2)
> @@ -572,10 +651,10 @@ static int csi2_async_register(struct csi2_dev *csi2)
>   	if (ret)
>   		goto err_parse;
>   
> -	csi2->bus = vep.bus.mipi_csi2;
> +	csi2->data_lanes = vep.bus.mipi_csi2.num_data_lanes;
>   
> -	dev_dbg(csi2->dev, "data lanes: %d\n", csi2->bus.num_data_lanes);
> -	dev_dbg(csi2->dev, "flags: 0x%08x\n", csi2->bus.flags);
> +	dev_dbg(csi2->dev, "data lanes: %d\n", vep.bus.mipi_csi2.num_data_lanes);
> +	dev_dbg(csi2->dev, "flags: 0x%08x\n", vep.bus.mipi_csi2.flags);
>   
>   	asd = kzalloc(sizeof(*asd), GFP_KERNEL);
>   	if (!asd) {

Re: [PATCH] media: imx6-mipi-csi2: Call remote subdev get_mbus_config to get active lanes

[Ezequiel Garcia]

Hi Steve,

On Fri, 2021-01-08 at 11:10 -0800, Steve Longerbeam wrote:
> 
> On 12/29/20 2:31 AM, Ezequiel Garcia wrote:
> > Currently, the CSI2 subdevice is using the data-lanes from the
> > neareast endpoint to config the CSI2 lanes.
> > 
> > While this may work, the proper way to configure the hardware is
> > to obtain the remote subdevice in v4l2_async_notifier_operations.bound(),
> > and then call get_mbus_config using the remote subdevice to get
> > the active lanes.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >   drivers/staging/media/imx/TODO             |  12 ---
> >   drivers/staging/media/imx/imx6-mipi-csi2.c | 101 ++++++++++++++++++---
> >   2 files changed, 90 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/staging/media/imx/TODO b/drivers/staging/media/imx/TODO
> > index 9cfc1c1e78dc..c575f419204a 100644
> > --- a/drivers/staging/media/imx/TODO
> > +++ b/drivers/staging/media/imx/TODO
> > @@ -2,18 +2,6 @@
> >   - The Frame Interval Monitor could be exported to v4l2-core for
> >     general use.
> >   
> > -- The CSI subdevice parses its nearest upstream neighbor's device-tree
> > -  bus config in order to setup the CSI. Laurent Pinchart argues that
> > -  instead the CSI subdev should call its neighbor's g_mbus_config op
> > -  (which should be propagated if necessary) to get this info. However
> > -  Hans Verkuil is planning to remove the g_mbus_config op. For now this
> > -  driver uses the parsed DT bus config method until this issue is
> > -  resolved.
> 
> This TODO was actually referring to the fwnode endpoint parsing in 
> imx-media-csi.c, not imx6-mipi-csi2.c.
> 

Ah, OK.

> But the same conversion to call .get_mbus_config() instead of endpoint 
> parsing could be done in imx-media-csi.c, but there is one imx6 
> constraint that is preventing this from happening. The imx6 reference 
> manual states that if the CSI is receiving from an input parallel bus 
> that is 16-bits wide, the data must go directly to memory via the SMFC 
> and not be sent to the IPU's Image Converter ("passthrough" mode):
> 
> "37.4.3.9 16 bit camera support
> 
> Devices that support 16 bit data bus can be connected to the CSI. This 
> can be done in one
> of the following ways.
> 
> 16 bit YUV422
> In this mode the CSI receives 2 components per cycle. The CSI is 
> programmed to
> accept the data as 16 bit generic data. The captured data will be stored 
> in the memory
> through the SMFC. The IDMAC needs to be programmed to store 16bit 
> generic data.
> When the data is read back from the memory for further processing in the 
> IPU it will
> be read as YUV422 data."
> 
> Same is said for RGB data to the CSI.
> 
> I'm not sure if this restriction is real or not. If this restriction 
> were ignored, the fwnode endpoint check "ep->bus.parallel.bus_width >= 
> 16" could be removed and the only remaining info required to determine 
> passthrough mode is available from 'struct v4l2_mbus_config' and the 
> input mbus codes, thus allowing the conversion to .get_mbus_config().
> 

For the sound of this, the above doesn't affect this patch, right?
Also, note there's a v2 submitted:

https://patchwork.linuxtv.org/project/linux-media/patch/20210103154155.318300-1-ezequiel@collabora.com/

Now, there's something I'm not exactly sure about these .get_mbus_config
conversions, being described in the TODO file.

The TODO file should only list what's missing to move the driver
out of staging. Converting to newer APIs doesn't seem a blocker:
there are a ton of drivers using old APIs out there, which is
a natural consequence of how the kernel evolve APIs all the time.

I'm wondering if the other TODO items apply as well, moving
the Frame Interval Monitor to the v4l2-core is something we
can always do at any later point. It shouldn't be a requirement
for destaging.

There's one thing that we must resolve before de-staging.
The media controller topology, which is a form of ABI should
be settled, as that's difficult to change later.

However, this item is not mentioned in the TODO.

So, I was thinking we should remove all the current TODO
items and add something about the media controller topology
stability requirements.

What do you think?

Cheers,
Ezequiel 

Re: [PATCH] media: imx6-mipi-csi2: Call remote subdev get_mbus_config to get active lanes

[Laurent Pinchart]

Hi Ezequiel,

On Mon, Jan 11, 2021 at 12:11:36PM -0300, Ezequiel Garcia wrote:
> On Fri, 2021-01-08 at 11:10 -0800, Steve Longerbeam wrote:
> > On 12/29/20 2:31 AM, Ezequiel Garcia wrote:
> > > Currently, the CSI2 subdevice is using the data-lanes from the
> > > neareast endpoint to config the CSI2 lanes.
> > > 
> > > While this may work, the proper way to configure the hardware is
> > > to obtain the remote subdevice in v4l2_async_notifier_operations.bound(),
> > > and then call get_mbus_config using the remote subdevice to get
> > > the active lanes.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > ---
> > >   drivers/staging/media/imx/TODO             |  12 ---
> > >   drivers/staging/media/imx/imx6-mipi-csi2.c | 101 ++++++++++++++++++---
> > >   2 files changed, 90 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/imx/TODO b/drivers/staging/media/imx/TODO
> > > index 9cfc1c1e78dc..c575f419204a 100644
> > > --- a/drivers/staging/media/imx/TODO
> > > +++ b/drivers/staging/media/imx/TODO
> > > @@ -2,18 +2,6 @@
> > >   - The Frame Interval Monitor could be exported to v4l2-core for
> > >     general use.
> > >   
> > > -- The CSI subdevice parses its nearest upstream neighbor's device-tree
> > > -  bus config in order to setup the CSI. Laurent Pinchart argues that
> > > -  instead the CSI subdev should call its neighbor's g_mbus_config op
> > > -  (which should be propagated if necessary) to get this info. However
> > > -  Hans Verkuil is planning to remove the g_mbus_config op. For now this
> > > -  driver uses the parsed DT bus config method until this issue is
> > > -  resolved.
> > 
> > This TODO was actually referring to the fwnode endpoint parsing in 
> 
> Ah, OK.
> 
> > But the same conversion to call .get_mbus_config() instead of endpoint 
> > parsing could be done in imx-media-csi.c, but there is one imx6 
> > constraint that is preventing this from happening. The imx6 reference 
> > manual states that if the CSI is receiving from an input parallel bus 
> > that is 16-bits wide, the data must go directly to memory via the SMFC 
> > and not be sent to the IPU's Image Converter ("passthrough" mode):
> > 
> > "37.4.3.9 16 bit camera support
> > 
> > Devices that support 16 bit data bus can be connected to the CSI. This  can be done in one
> > of the following ways.
> > 
> > 16 bit YUV422
> > In this mode the CSI receives 2 components per cycle. The CSI is programmed to
> > accept the data as 16 bit generic data. The captured data will be stored in the memory
> > through the SMFC. The IDMAC needs to be programmed to store 16bit generic data.
> > When the data is read back from the memory for further processing in the IPU it will
> > be read as YUV422 data."
> > 
> > Same is said for RGB data to the CSI.
> > 
> > I'm not sure if this restriction is real or not. If this restriction 
> > were ignored, the fwnode endpoint check "ep->bus.parallel.bus_width >= 
> > 16" could be removed and the only remaining info required to determine 
> > passthrough mode is available from 'struct v4l2_mbus_config' and the 
> > input mbus codes, thus allowing the conversion to .get_mbus_config().
> 
> For the sound of this, the above doesn't affect this patch, right?
> Also, note there's a v2 submitted:
> 
> https://patchwork.linuxtv.org/project/linux-media/patch/20210103154155.318300-1-ezequiel@collabora.com/
> 
> Now, there's something I'm not exactly sure about these .get_mbus_config
> conversions, being described in the TODO file.
> 
> The TODO file should only list what's missing to move the driver
> out of staging. Converting to newer APIs doesn't seem a blocker:
> there are a ton of drivers using old APIs out there, which is
> a natural consequence of how the kernel evolve APIs all the time.
> 
> I'm wondering if the other TODO items apply as well, moving
> the Frame Interval Monitor to the v4l2-core is something we
> can always do at any later point. It shouldn't be a requirement
> for destaging.
> 
> There's one thing that we must resolve before de-staging.
> The media controller topology, which is a form of ABI should
> be settled, as that's difficult to change later.
> 
> However, this item is not mentioned in the TODO.
> 
> So, I was thinking we should remove all the current TODO
> items and add something about the media controller topology
> stability requirements.
> 
> What do you think?

If we decide to do so, could you keep the TODO items somewhere ? It's
useful to have a list, they could be moved to the driver source code for
instance.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: imx6-mipi-csi2: Call remote subdev get_mbus_config to get active lanes

[Ezequiel Garcia]

On Tue, 2021-01-12 at 07:07 +0200, Laurent Pinchart wrote:
> Hi Ezequiel,
> 
> On Mon, Jan 11, 2021 at 12:11:36PM -0300, Ezequiel Garcia wrote:
> > On Fri, 2021-01-08 at 11:10 -0800, Steve Longerbeam wrote:
> > > On 12/29/20 2:31 AM, Ezequiel Garcia wrote:
> > > > Currently, the CSI2 subdevice is using the data-lanes from the
> > > > neareast endpoint to config the CSI2 lanes.
> > > > 
> > > > While this may work, the proper way to configure the hardware is
> > > > to obtain the remote subdevice in v4l2_async_notifier_operations.bound(),
> > > > and then call get_mbus_config using the remote subdevice to get
> > > > the active lanes.
> > > > 
> > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > > ---
> > > >   drivers/staging/media/imx/TODO             |  12 ---
> > > >   drivers/staging/media/imx/imx6-mipi-csi2.c | 101 ++++++++++++++++++---
> > > >   2 files changed, 90 insertions(+), 23 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/media/imx/TODO b/drivers/staging/media/imx/TODO
> > > > index 9cfc1c1e78dc..c575f419204a 100644
> > > > --- a/drivers/staging/media/imx/TODO
> > > > +++ b/drivers/staging/media/imx/TODO
> > > > @@ -2,18 +2,6 @@
> > > >   - The Frame Interval Monitor could be exported to v4l2-core for
> > > >     general use.
> > > >   
> > > > -- The CSI subdevice parses its nearest upstream neighbor's device-tree
> > > > -  bus config in order to setup the CSI. Laurent Pinchart argues that
> > > > -  instead the CSI subdev should call its neighbor's g_mbus_config op
> > > > -  (which should be propagated if necessary) to get this info. However
> > > > -  Hans Verkuil is planning to remove the g_mbus_config op. For now this
> > > > -  driver uses the parsed DT bus config method until this issue is
> > > > -  resolved.
> > > 
> > > This TODO was actually referring to the fwnode endpoint parsing in 
> > 
> > Ah, OK.
> > 
> > > But the same conversion to call .get_mbus_config() instead of endpoint 
> > > parsing could be done in imx-media-csi.c, but there is one imx6 
> > > constraint that is preventing this from happening. The imx6 reference 
> > > manual states that if the CSI is receiving from an input parallel bus 
> > > that is 16-bits wide, the data must go directly to memory via the SMFC 
> > > and not be sent to the IPU's Image Converter ("passthrough" mode):
> > > 
> > > "37.4.3.9 16 bit camera support
> > > 
> > > Devices that support 16 bit data bus can be connected to the CSI. This  can be done in one
> > > of the following ways.
> > > 
> > > 16 bit YUV422
> > > In this mode the CSI receives 2 components per cycle. The CSI is programmed to
> > > accept the data as 16 bit generic data. The captured data will be stored in the memory
> > > through the SMFC. The IDMAC needs to be programmed to store 16bit generic data.
> > > When the data is read back from the memory for further processing in the IPU it will
> > > be read as YUV422 data."
> > > 
> > > Same is said for RGB data to the CSI.
> > > 
> > > I'm not sure if this restriction is real or not. If this restriction 
> > > were ignored, the fwnode endpoint check "ep->bus.parallel.bus_width >= 
> > > 16" could be removed and the only remaining info required to determine 
> > > passthrough mode is available from 'struct v4l2_mbus_config' and the 
> > > input mbus codes, thus allowing the conversion to .get_mbus_config().
> > 
> > For the sound of this, the above doesn't affect this patch, right?
> > Also, note there's a v2 submitted:
> > 
> > https://patchwork.linuxtv.org/project/linux-media/patch/20210103154155.318300-1-ezequiel@collabora.com/
> > 
> > Now, there's something I'm not exactly sure about these .get_mbus_config
> > conversions, being described in the TODO file.
> > 
> > The TODO file should only list what's missing to move the driver
> > out of staging. Converting to newer APIs doesn't seem a blocker:
> > there are a ton of drivers using old APIs out there, which is
> > a natural consequence of how the kernel evolve APIs all the time.
> > 
> > I'm wondering if the other TODO items apply as well, moving
> > the Frame Interval Monitor to the v4l2-core is something we
> > can always do at any later point. It shouldn't be a requirement
> > for destaging.
> > 
> > There's one thing that we must resolve before de-staging.
> > The media controller topology, which is a form of ABI should
> > be settled, as that's difficult to change later.
> > 
> > However, this item is not mentioned in the TODO.
> > 
> > So, I was thinking we should remove all the current TODO
> > items and add something about the media controller topology
> > stability requirements.
> > 
> > What do you think?
> 
> If we decide to do so, could you keep the TODO items somewhere ? It's
> useful to have a list, they could be moved to the driver source code for
> instance.
> 

Yes, that makes perfect sense. I'll send a v3 in the next few days,
unless there is any objection on it.

Thanks,
Ezequiel

Re: [PATCH] media: imx6-mipi-csi2: Call remote subdev get_mbus_config to get active lanes

[Steve Longerbeam]

Hi Ezequiel,

On 1/11/21 7:11 AM, Ezequiel Garcia wrote:
> Hi Steve,
>
> On Fri, 2021-01-08 at 11:10 -0800, Steve Longerbeam wrote:
>> On 12/29/20 2:31 AM, Ezequiel Garcia wrote:
>>> Currently, the CSI2 subdevice is using the data-lanes from the
>>> neareast endpoint to config the CSI2 lanes.
>>>
>>> While this may work, the proper way to configure the hardware is
>>> to obtain the remote subdevice in v4l2_async_notifier_operations.bound(),
>>> and then call get_mbus_config using the remote subdevice to get
>>> the active lanes.
>>>
>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>>> ---
>>>    drivers/staging/media/imx/TODO             |  12 ---
>>>    drivers/staging/media/imx/imx6-mipi-csi2.c | 101 ++++++++++++++++++---
>>>    2 files changed, 90 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/imx/TODO b/drivers/staging/media/imx/TODO
>>> index 9cfc1c1e78dc..c575f419204a 100644
>>> --- a/drivers/staging/media/imx/TODO
>>> +++ b/drivers/staging/media/imx/TODO
>>> @@ -2,18 +2,6 @@
>>>    - The Frame Interval Monitor could be exported to v4l2-core for
>>>      general use.
>>>    
>>> -- The CSI subdevice parses its nearest upstream neighbor's device-tree
>>> -  bus config in order to setup the CSI. Laurent Pinchart argues that
>>> -  instead the CSI subdev should call its neighbor's g_mbus_config op
>>> -  (which should be propagated if necessary) to get this info. However
>>> -  Hans Verkuil is planning to remove the g_mbus_config op. For now this
>>> -  driver uses the parsed DT bus config method until this issue is
>>> -  resolved.
>> This TODO was actually referring to the fwnode endpoint parsing in
>> imx-media-csi.c, not imx6-mipi-csi2.c.
>>
> Ah, OK.
>
>> But the same conversion to call .get_mbus_config() instead of endpoint
>> parsing could be done in imx-media-csi.c, but there is one imx6
>> constraint that is preventing this from happening. The imx6 reference
>> manual states that if the CSI is receiving from an input parallel bus
>> that is 16-bits wide, the data must go directly to memory via the SMFC
>> and not be sent to the IPU's Image Converter ("passthrough" mode):
>>
>> "37.4.3.9 16 bit camera support
>>
>> Devices that support 16 bit data bus can be connected to the CSI. This
>> can be done in one
>> of the following ways.
>>
>> 16 bit YUV422
>> In this mode the CSI receives 2 components per cycle. The CSI is
>> programmed to
>> accept the data as 16 bit generic data. The captured data will be stored
>> in the memory
>> through the SMFC. The IDMAC needs to be programmed to store 16bit
>> generic data.
>> When the data is read back from the memory for further processing in the
>> IPU it will
>> be read as YUV422 data."
>>
>> Same is said for RGB data to the CSI.
>>
>> I'm not sure if this restriction is real or not. If this restriction
>> were ignored, the fwnode endpoint check "ep->bus.parallel.bus_width >=
>> 16" could be removed and the only remaining info required to determine
>> passthrough mode is available from 'struct v4l2_mbus_config' and the
>> input mbus codes, thus allowing the conversion to .get_mbus_config().
>>
> For the sound of this, the above doesn't affect this patch, right?

Correct, the conversion to .get_mbus_config() in imx-media-csi.c can be 
a separate patch.

> Also, note there's a v2 submitted:
>
> https://patchwork.linuxtv.org/project/linux-media/patch/20210103154155.318300-1-ezequiel@collabora.com/
>
> Now, there's something I'm not exactly sure about these .get_mbus_config
> conversions, being described in the TODO file.
>
> The TODO file should only list what's missing to move the driver
> out of staging. Converting to newer APIs doesn't seem a blocker:
> there are a ton of drivers using old APIs out there, which is
> a natural consequence of how the kernel evolve APIs all the time.
>
> I'm wondering if the other TODO items apply as well, moving
> the Frame Interval Monitor to the v4l2-core is something we
> can always do at any later point. It shouldn't be a requirement
> for destaging.
>
> There's one thing that we must resolve before de-staging.
> The media controller topology, which is a form of ABI should
> be settled, as that's difficult to change later.
>
> However, this item is not mentioned in the TODO.
>
> So, I was thinking we should remove all the current TODO
> items and add something about the media controller topology
> stability requirements.
>
> What do you think?

The only suggestion I see that would affect the MC topology is Laurent's 
patch that paves the way to make a few of the imx6 media links 
immutable. That's not really a topology change but I agree it needs 
settling before moving out of staging.

Steve

Re: [PATCH] media: imx6-mipi-csi2: Call remote subdev get_mbus_config to get active lanes

[Ezequiel Garcia]

On Wed, 2021-01-13 at 12:13 -0800, Steve Longerbeam wrote:
> Hi Ezequiel,
> 
> On 1/11/21 7:11 AM, Ezequiel Garcia wrote:
> > Hi Steve,
> > 
> > On Fri, 2021-01-08 at 11:10 -0800, Steve Longerbeam wrote:
> > > On 12/29/20 2:31 AM, Ezequiel Garcia wrote:
> > > > Currently, the CSI2 subdevice is using the data-lanes from the
> > > > neareast endpoint to config the CSI2 lanes.
> > > > 
> > > > While this may work, the proper way to configure the hardware is
> > > > to obtain the remote subdevice in v4l2_async_notifier_operations.bound(),
> > > > and then call get_mbus_config using the remote subdevice to get
> > > > the active lanes.
> > > > 
> > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > > ---
> > > >    drivers/staging/media/imx/TODO             |  12 ---
> > > >    drivers/staging/media/imx/imx6-mipi-csi2.c | 101 ++++++++++++++++++---
> > > >    2 files changed, 90 insertions(+), 23 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/media/imx/TODO b/drivers/staging/media/imx/TODO
> > > > index 9cfc1c1e78dc..c575f419204a 100644
> > > > --- a/drivers/staging/media/imx/TODO
> > > > +++ b/drivers/staging/media/imx/TODO
> > > > @@ -2,18 +2,6 @@
> > > >    - The Frame Interval Monitor could be exported to v4l2-core for
> > > >      general use.
> > > >    
> > > > -- The CSI subdevice parses its nearest upstream neighbor's device-tree
> > > > -  bus config in order to setup the CSI. Laurent Pinchart argues that
> > > > -  instead the CSI subdev should call its neighbor's g_mbus_config op
> > > > -  (which should be propagated if necessary) to get this info. However
> > > > -  Hans Verkuil is planning to remove the g_mbus_config op. For now this
> > > > -  driver uses the parsed DT bus config method until this issue is
> > > > -  resolved.
> > > This TODO was actually referring to the fwnode endpoint parsing in
> > > imx-media-csi.c, not imx6-mipi-csi2.c.
> > > 
> > Ah, OK.
> > 
> > > But the same conversion to call .get_mbus_config() instead of endpoint
> > > parsing could be done in imx-media-csi.c, but there is one imx6
> > > constraint that is preventing this from happening. The imx6 reference
> > > manual states that if the CSI is receiving from an input parallel bus
> > > that is 16-bits wide, the data must go directly to memory via the SMFC
> > > and not be sent to the IPU's Image Converter ("passthrough" mode):
> > > 
> > > "37.4.3.9 16 bit camera support
> > > 
> > > Devices that support 16 bit data bus can be connected to the CSI. This
> > > can be done in one
> > > of the following ways.
> > > 
> > > 16 bit YUV422
> > > In this mode the CSI receives 2 components per cycle. The CSI is
> > > programmed to
> > > accept the data as 16 bit generic data. The captured data will be stored
> > > in the memory
> > > through the SMFC. The IDMAC needs to be programmed to store 16bit
> > > generic data.
> > > When the data is read back from the memory for further processing in the
> > > IPU it will
> > > be read as YUV422 data."
> > > 
> > > Same is said for RGB data to the CSI.
> > > 
> > > I'm not sure if this restriction is real or not. If this restriction
> > > were ignored, the fwnode endpoint check "ep->bus.parallel.bus_width >=
> > > 16" could be removed and the only remaining info required to determine
> > > passthrough mode is available from 'struct v4l2_mbus_config' and the
> > > input mbus codes, thus allowing the conversion to .get_mbus_config().
> > > 
> > For the sound of this, the above doesn't affect this patch, right?
> 
> Correct, the conversion to .get_mbus_config() in imx-media-csi.c can be 
> a separate patch.
> 
> > Also, note there's a v2 submitted:
> > 
> > https://patchwork.linuxtv.org/project/linux-media/patch/20210103154155.318300-1-ezequiel@collabora.com/
> > 
> > Now, there's something I'm not exactly sure about these .get_mbus_config
> > conversions, being described in the TODO file.
> > 
> > The TODO file should only list what's missing to move the driver
> > out of staging. Converting to newer APIs doesn't seem a blocker:
> > there are a ton of drivers using old APIs out there, which is
> > a natural consequence of how the kernel evolve APIs all the time.
> > 
> > I'm wondering if the other TODO items apply as well, moving
> > the Frame Interval Monitor to the v4l2-core is something we
> > can always do at any later point. It shouldn't be a requirement
> > for destaging.
> > 
> > There's one thing that we must resolve before de-staging.
> > The media controller topology, which is a form of ABI should
> > be settled, as that's difficult to change later.
> > 
> > However, this item is not mentioned in the TODO.
> > 
> > So, I was thinking we should remove all the current TODO
> > items and add something about the media controller topology
> > stability requirements.
> > 
> > What do you think?
> 
> The only suggestion I see that would affect the MC topology is Laurent's 
> patch that paves the way to make a few of the imx6 media links 
> immutable. That's not really a topology change but I agree it needs 
> settling before moving out of staging.
> 

Let me get a v3 for this get_mbus_config patch,
dropping any changes to the TODO.

Let's wait until Laurent's big series to land,
and then we can maybe start rectifiying the TODO,
even plan how this could be destaged.

Thanks,
Ezequiel