[PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Marcel Ziswiler]

From: Aishwarya Kothari <aishwarya.kothari@toradex.com>

Implement the introduced get_mbus_config operation to report the
config of the MIPI CSI-2, BT.656 and Parallel interface.

Signed-off-by: Aishwarya Kothari <aishwarya.kothari@toradex.com>
Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

---

Changes in v2:
- Take care of MIPI CSI-2, BT.656 and Parallel interface as
  pointed out by Jacopo. Thanks!

 drivers/media/i2c/ov5640.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 1536649b9e90..43373416fcba 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -3774,6 +3774,24 @@ 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)
+{
+	struct ov5640_dev *sensor = to_ov5640_dev(sd);
+
+	cfg->type = sensor->ep.bus_type;
+	if (ov5640_is_csi2(sensor)) {
+		cfg->bus.mipi_csi2.num_data_lanes =
+			sensor->ep.bus.mipi_csi2.num_data_lanes;
+		cfg->bus.mipi_csi2.flags = sensor->ep.bus.mipi_csi2.flags;
+	} else {
+		cfg->bus.parallel.flags = sensor->ep.bus.parallel.flags;
+	}
+
+	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,
@@ -3794,6 +3812,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 = {
-- 
2.36.1

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Sakari Ailus]

Hi Marcel,

On Mon, Mar 06, 2023 at 07:36:49AM +0100, Marcel Ziswiler wrote:
> From: Aishwarya Kothari <aishwarya.kothari@toradex.com>
> 
> Implement the introduced get_mbus_config operation to report the
> config of the MIPI CSI-2, BT.656 and Parallel interface.
> 
> Signed-off-by: Aishwarya Kothari <aishwarya.kothari@toradex.com>
> Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> 
> ---
> 
> Changes in v2:
> - Take care of MIPI CSI-2, BT.656 and Parallel interface as
>   pointed out by Jacopo. Thanks!
> 
>  drivers/media/i2c/ov5640.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 1536649b9e90..43373416fcba 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -3774,6 +3774,24 @@ 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)
> +{
> +	struct ov5640_dev *sensor = to_ov5640_dev(sd);
> +
> +	cfg->type = sensor->ep.bus_type;
> +	if (ov5640_is_csi2(sensor)) {
> +		cfg->bus.mipi_csi2.num_data_lanes =
> +			sensor->ep.bus.mipi_csi2.num_data_lanes;
> +		cfg->bus.mipi_csi2.flags = sensor->ep.bus.mipi_csi2.flags;
> +	} else {
> +		cfg->bus.parallel.flags = sensor->ep.bus.parallel.flags;
> +	}
> +
> +	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,
> @@ -3794,6 +3812,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,

What's the reasoning for this patch?

Drivers that don't have e.g. dynamic lane configuration shouldn't need to
implement get_mbus_config.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Francesco Dolcini]

Hello Sakari,

On Tue, Mar 14, 2023 at 02:13:46PM +0200, Sakari Ailus wrote:
> On Mon, Mar 06, 2023 at 07:36:49AM +0100, Marcel Ziswiler wrote:
> > From: Aishwarya Kothari <aishwarya.kothari@toradex.com>
> > 
> > Implement the introduced get_mbus_config operation to report the
> > config of the MIPI CSI-2, BT.656 and Parallel interface.
> > 
> > Signed-off-by: Aishwarya Kothari <aishwarya.kothari@toradex.com>
> > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > 
> > ---
> > 
> > Changes in v2:
> > - Take care of MIPI CSI-2, BT.656 and Parallel interface as
> >   pointed out by Jacopo. Thanks!
> > 
> >  drivers/media/i2c/ov5640.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 1536649b9e90..43373416fcba 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -3774,6 +3774,24 @@ 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)
> > +{
> > +	struct ov5640_dev *sensor = to_ov5640_dev(sd);
> > +
> > +	cfg->type = sensor->ep.bus_type;
> > +	if (ov5640_is_csi2(sensor)) {
> > +		cfg->bus.mipi_csi2.num_data_lanes =
> > +			sensor->ep.bus.mipi_csi2.num_data_lanes;
> > +		cfg->bus.mipi_csi2.flags = sensor->ep.bus.mipi_csi2.flags;
> > +	} else {
> > +		cfg->bus.parallel.flags = sensor->ep.bus.parallel.flags;
> > +	}
> > +
> > +	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,
> > @@ -3794,6 +3812,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,
> 
> What's the reasoning for this patch?

Without this it's not possible to use it on i.MX6,
drivers/staging/media/imx/imx6-mipi-csi2.c requires it, some more
details from Jacopo here [0].

Everything used to work fine up to v5.18, after that kernel version
various changes broke it [1][2] (I assume you are pretty much aware of
the history here, you commented on a few emails).

[0] https://lore.kernel.org/all/20230128100611.7ulsfqqqgscg54gy@uno.localdomain/
[1] https://lore.kernel.org/all/081cc2d3-1f3a-6c14-6dc7-53f976be7b2b@gmail.com/
[2] https://lore.kernel.org/all/cacfe146-101b-35b3-5f66-1a1cabfd342f@gmail.com/

> Drivers that don't have e.g. dynamic lane configuration shouldn't need to
> implement get_mbus_config.

Francesco

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Sakari Ailus]

Hi Francesco,

On Tue, Mar 14, 2023 at 01:32:16PM +0100, Francesco Dolcini wrote:
> Hello Sakari,
> 
> On Tue, Mar 14, 2023 at 02:13:46PM +0200, Sakari Ailus wrote:
> > On Mon, Mar 06, 2023 at 07:36:49AM +0100, Marcel Ziswiler wrote:
> > > From: Aishwarya Kothari <aishwarya.kothari@toradex.com>
> > > 
> > > Implement the introduced get_mbus_config operation to report the
> > > config of the MIPI CSI-2, BT.656 and Parallel interface.
> > > 
> > > Signed-off-by: Aishwarya Kothari <aishwarya.kothari@toradex.com>
> > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > 
> > > ---
> > > 
> > > Changes in v2:
> > > - Take care of MIPI CSI-2, BT.656 and Parallel interface as
> > >   pointed out by Jacopo. Thanks!
> > > 
> > >  drivers/media/i2c/ov5640.c | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > > index 1536649b9e90..43373416fcba 100644
> > > --- a/drivers/media/i2c/ov5640.c
> > > +++ b/drivers/media/i2c/ov5640.c
> > > @@ -3774,6 +3774,24 @@ 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)
> > > +{
> > > +	struct ov5640_dev *sensor = to_ov5640_dev(sd);
> > > +
> > > +	cfg->type = sensor->ep.bus_type;
> > > +	if (ov5640_is_csi2(sensor)) {
> > > +		cfg->bus.mipi_csi2.num_data_lanes =
> > > +			sensor->ep.bus.mipi_csi2.num_data_lanes;
> > > +		cfg->bus.mipi_csi2.flags = sensor->ep.bus.mipi_csi2.flags;
> > > +	} else {
> > > +		cfg->bus.parallel.flags = sensor->ep.bus.parallel.flags;
> > > +	}
> > > +
> > > +	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,
> > > @@ -3794,6 +3812,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,
> > 
> > What's the reasoning for this patch?
> 
> Without this it's not possible to use it on i.MX6,
> drivers/staging/media/imx/imx6-mipi-csi2.c requires it, some more
> details from Jacopo here [0].
> 
> Everything used to work fine up to v5.18, after that kernel version
> various changes broke it [1][2] (I assume you are pretty much aware of
> the history here, you commented on a few emails).
> 
> [0] https://lore.kernel.org/all/20230128100611.7ulsfqqqgscg54gy@uno.localdomain/
> [1] https://lore.kernel.org/all/081cc2d3-1f3a-6c14-6dc7-53f976be7b2b@gmail.com/
> [2] https://lore.kernel.org/all/cacfe146-101b-35b3-5f66-1a1cabfd342f@gmail.com/
> 
> > Drivers that don't have e.g. dynamic lane configuration shouldn't need to
> > implement get_mbus_config.

Not even for staging drivers. The driver should be fixed to get that
information from the endpoint instead.

I don't object having a helper in the framework to do this though. There
are many receiver drivers that need this to work with those devices that
have variable number of lanes.

-- 
Regards,

Sakari Ailus

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Francesco Dolcini]

+Marco

On Tue, Mar 14, 2023 at 02:45:53PM +0200, Sakari Ailus wrote:
> On Tue, Mar 14, 2023 at 01:32:16PM +0100, Francesco Dolcini wrote:
> > On Tue, Mar 14, 2023 at 02:13:46PM +0200, Sakari Ailus wrote:
> > > On Mon, Mar 06, 2023 at 07:36:49AM +0100, Marcel Ziswiler wrote:
> > > > From: Aishwarya Kothari <aishwarya.kothari@toradex.com>
> > > > 
> > > > Implement the introduced get_mbus_config operation to report the
> > > > config of the MIPI CSI-2, BT.656 and Parallel interface.
> > > > 
> > > > Signed-off-by: Aishwarya Kothari <aishwarya.kothari@toradex.com>
> > > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > 
> > > What's the reasoning for this patch?
> > 
> > Without this it's not possible to use it on i.MX6,
> > drivers/staging/media/imx/imx6-mipi-csi2.c requires it, some more
> > details from Jacopo here [0].
> > 
> > Everything used to work fine up to v5.18, after that kernel version
> > various changes broke it [1][2] (I assume you are pretty much aware of
> > the history here, you commented on a few emails).
> > 
> > [0] https://lore.kernel.org/all/20230128100611.7ulsfqqqgscg54gy@uno.localdomain/
> > [1] https://lore.kernel.org/all/081cc2d3-1f3a-6c14-6dc7-53f976be7b2b@gmail.com/
> > [2] https://lore.kernel.org/all/cacfe146-101b-35b3-5f66-1a1cabfd342f@gmail.com/
> > 
> > > Drivers that don't have e.g. dynamic lane configuration shouldn't need to
> > > implement get_mbus_config.
> 
> Not even for staging drivers. The driver should be fixed to get that
> information from the endpoint instead.

This seems exactly the opposite of what commit 
7318abface48 ("media: imx: Use get_mbus_config instead of parsing upstream DT endpoints")
did.

Given that I am somehow confused, but I am not that familiar with this
subsystem, so I guess this is expected :-). Can someone provide some
additional hint here?

> I don't object having a helper in the framework to do this though. There
> are many receiver drivers that need this to work with those devices that
> have variable number of lanes.

Thanks in advance,
Francesco

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Jacopo Mondi]

Hello

On Tue, Mar 14, 2023 at 01:59:06PM +0100, Francesco Dolcini wrote:
> +Marco
>
> On Tue, Mar 14, 2023 at 02:45:53PM +0200, Sakari Ailus wrote:
> > On Tue, Mar 14, 2023 at 01:32:16PM +0100, Francesco Dolcini wrote:
> > > On Tue, Mar 14, 2023 at 02:13:46PM +0200, Sakari Ailus wrote:
> > > > On Mon, Mar 06, 2023 at 07:36:49AM +0100, Marcel Ziswiler wrote:
> > > > > From: Aishwarya Kothari <aishwarya.kothari@toradex.com>
> > > > >
> > > > > Implement the introduced get_mbus_config operation to report the
> > > > > config of the MIPI CSI-2, BT.656 and Parallel interface.
> > > > >
> > > > > Signed-off-by: Aishwarya Kothari <aishwarya.kothari@toradex.com>
> > > > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > >
> > > > What's the reasoning for this patch?
> > >
> > > Without this it's not possible to use it on i.MX6,
> > > drivers/staging/media/imx/imx6-mipi-csi2.c requires it, some more
> > > details from Jacopo here [0].
> > >
> > > Everything used to work fine up to v5.18, after that kernel version
> > > various changes broke it [1][2] (I assume you are pretty much aware of
> > > the history here, you commented on a few emails).
> > >
> > > [0] https://lore.kernel.org/all/20230128100611.7ulsfqqqgscg54gy@uno.localdomain/
> > > [1] https://lore.kernel.org/all/081cc2d3-1f3a-6c14-6dc7-53f976be7b2b@gmail.com/
> > > [2] https://lore.kernel.org/all/cacfe146-101b-35b3-5f66-1a1cabfd342f@gmail.com/
> > >
> > > > Drivers that don't have e.g. dynamic lane configuration shouldn't need to
> > > > implement get_mbus_config.
> >
> > Not even for staging drivers. The driver should be fixed to get that
> > information from the endpoint instead.
>
> This seems exactly the opposite of what commit
> 7318abface48 ("media: imx: Use get_mbus_config instead of parsing upstream DT endpoints")
> did.
>
> Given that I am somehow confused, but I am not that familiar with this
> subsystem, so I guess this is expected :-). Can someone provide some
> additional hint here?
>

As per my understanding, the i.MX6 IPU CSI driver connects to the
CSI-2 receiver and/or two video muxes. One figure's worth a thousands
words: "Figure 19-1. CSI2IPU gasket connectivity" of the IMX6DQRM TRM.

So the local endpoint might not provide the required information on
the bus configuration as it connects to a video-mux.

That's why the imx_media_pipeline_subdev() helper is used in
csi_get_upstream_mbus_config().

My gut feeling is that it would be better to always call
get_mbus_config() on the next subdev (the mux or the CSI-2 rx) and
there parse the local endpoint as it's the mux or the CSI-2 rx that
connect to the actual source.

To be honest my understanding is that this patch has always been
needed to work on imx6 and this is not a regression but something that
was kept as an out-of-tree patch downstream. Is this correct or is
this a regression ? The above mentioned patches that move fwnode
matching to endpoints don't seem related, don't they ?

Thanks
   j

> > I don't object having a helper in the framework to do this though. There
> > are many receiver drivers that need this to work with those devices that
> > have variable number of lanes.
>
> Thanks in advance,
> Francesco
>

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Sakari Ailus]

Hi Jacopo,

On Mon, Mar 20, 2023 at 09:48:44AM +0100, Jacopo Mondi wrote:
> Hello
> 
> On Tue, Mar 14, 2023 at 01:59:06PM +0100, Francesco Dolcini wrote:
> > +Marco
> >
> > On Tue, Mar 14, 2023 at 02:45:53PM +0200, Sakari Ailus wrote:
> > > On Tue, Mar 14, 2023 at 01:32:16PM +0100, Francesco Dolcini wrote:
> > > > On Tue, Mar 14, 2023 at 02:13:46PM +0200, Sakari Ailus wrote:
> > > > > On Mon, Mar 06, 2023 at 07:36:49AM +0100, Marcel Ziswiler wrote:
> > > > > > From: Aishwarya Kothari <aishwarya.kothari@toradex.com>
> > > > > >
> > > > > > Implement the introduced get_mbus_config operation to report the
> > > > > > config of the MIPI CSI-2, BT.656 and Parallel interface.
> > > > > >
> > > > > > Signed-off-by: Aishwarya Kothari <aishwarya.kothari@toradex.com>
> > > > > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > > >
> > > > > What's the reasoning for this patch?
> > > >
> > > > Without this it's not possible to use it on i.MX6,
> > > > drivers/staging/media/imx/imx6-mipi-csi2.c requires it, some more
> > > > details from Jacopo here [0].
> > > >
> > > > Everything used to work fine up to v5.18, after that kernel version
> > > > various changes broke it [1][2] (I assume you are pretty much aware of
> > > > the history here, you commented on a few emails).
> > > >
> > > > [0] https://lore.kernel.org/all/20230128100611.7ulsfqqqgscg54gy@uno.localdomain/
> > > > [1] https://lore.kernel.org/all/081cc2d3-1f3a-6c14-6dc7-53f976be7b2b@gmail.com/
> > > > [2] https://lore.kernel.org/all/cacfe146-101b-35b3-5f66-1a1cabfd342f@gmail.com/
> > > >
> > > > > Drivers that don't have e.g. dynamic lane configuration shouldn't need to
> > > > > implement get_mbus_config.
> > >
> > > Not even for staging drivers. The driver should be fixed to get that
> > > information from the endpoint instead.
> >
> > This seems exactly the opposite of what commit
> > 7318abface48 ("media: imx: Use get_mbus_config instead of parsing upstream DT endpoints")
> > did.
> >
> > Given that I am somehow confused, but I am not that familiar with this
> > subsystem, so I guess this is expected :-). Can someone provide some
> > additional hint here?
> >
> 
> As per my understanding, the i.MX6 IPU CSI driver connects to the
> CSI-2 receiver and/or two video muxes. One figure's worth a thousands
> words: "Figure 19-1. CSI2IPU gasket connectivity" of the IMX6DQRM TRM.

I don't have that document.

> 
> So the local endpoint might not provide the required information on
> the bus configuration as it connects to a video-mux.
> 
> That's why the imx_media_pipeline_subdev() helper is used in
> csi_get_upstream_mbus_config().
> 
> My gut feeling is that it would be better to always call
> get_mbus_config() on the next subdev (the mux or the CSI-2 rx) and
> there parse the local endpoint as it's the mux or the CSI-2 rx that
> connect to the actual source.

Isn't this still a different endpoint in DT? I understand you have a single
pad with two links?

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Laurent Pinchart]

On Mon, Mar 20, 2023 at 11:00:36AM +0200, Sakari Ailus wrote:
> On Mon, Mar 20, 2023 at 09:48:44AM +0100, Jacopo Mondi wrote:
> > On Tue, Mar 14, 2023 at 01:59:06PM +0100, Francesco Dolcini wrote:
> > > On Tue, Mar 14, 2023 at 02:45:53PM +0200, Sakari Ailus wrote:
> > > > On Tue, Mar 14, 2023 at 01:32:16PM +0100, Francesco Dolcini wrote:
> > > > > On Tue, Mar 14, 2023 at 02:13:46PM +0200, Sakari Ailus wrote:
> > > > > > On Mon, Mar 06, 2023 at 07:36:49AM +0100, Marcel Ziswiler wrote:
> > > > > > > From: Aishwarya Kothari <aishwarya.kothari@toradex.com>
> > > > > > >
> > > > > > > Implement the introduced get_mbus_config operation to report the
> > > > > > > config of the MIPI CSI-2, BT.656 and Parallel interface.
> > > > > > >
> > > > > > > Signed-off-by: Aishwarya Kothari <aishwarya.kothari@toradex.com>
> > > > > > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > > > >
> > > > > > What's the reasoning for this patch?
> > > > >
> > > > > Without this it's not possible to use it on i.MX6,
> > > > > drivers/staging/media/imx/imx6-mipi-csi2.c requires it, some more
> > > > > details from Jacopo here [0].
> > > > >
> > > > > Everything used to work fine up to v5.18, after that kernel version
> > > > > various changes broke it [1][2] (I assume you are pretty much aware of
> > > > > the history here, you commented on a few emails).
> > > > >
> > > > > [0] https://lore.kernel.org/all/20230128100611.7ulsfqqqgscg54gy@uno.localdomain/
> > > > > [1] https://lore.kernel.org/all/081cc2d3-1f3a-6c14-6dc7-53f976be7b2b@gmail.com/
> > > > > [2] https://lore.kernel.org/all/cacfe146-101b-35b3-5f66-1a1cabfd342f@gmail.com/
> > > > >
> > > > > > Drivers that don't have e.g. dynamic lane configuration shouldn't need to
> > > > > > implement get_mbus_config.
> > > >
> > > > Not even for staging drivers. The driver should be fixed to get that
> > > > information from the endpoint instead.
> > >
> > > This seems exactly the opposite of what commit
> > > 7318abface48 ("media: imx: Use get_mbus_config instead of parsing upstream DT endpoints")
> > > did.
> > >
> > > Given that I am somehow confused, but I am not that familiar with this
> > > subsystem, so I guess this is expected :-). Can someone provide some
> > > additional hint here?
> > >
> > 
> > As per my understanding, the i.MX6 IPU CSI driver connects to the
> > CSI-2 receiver and/or two video muxes. One figure's worth a thousands
> > words: "Figure 19-1. CSI2IPU gasket connectivity" of the IMX6DQRM TRM.
> 
> I don't have that document.

https://www.nxp.com/webapp/Download?colCode=IMX6DQRM

You'll need a user account on nxp.com though.

> > So the local endpoint might not provide the required information on
> > the bus configuration as it connects to a video-mux.
> > 
> > That's why the imx_media_pipeline_subdev() helper is used in
> > csi_get_upstream_mbus_config().
> > 
> > My gut feeling is that it would be better to always call
> > get_mbus_config() on the next subdev (the mux or the CSI-2 rx) and
> > there parse the local endpoint as it's the mux or the CSI-2 rx that
> > connect to the actual source.
> 
> Isn't this still a different endpoint in DT? I understand you have a single
> pad with two links?

In a (simplified) nutshell,

---------+     +----------+     +---------+     +-----+     +-----+
| Camera | --> | CSI-2 RX | --> | CSI2IPU | --> | Mux | --> | IPU |
| Sensor |     |          |     | Gasket  |     |     |     |     |
---------+     +----------+     +---------+     +-----+     +-----+

All those blocks, except for the gasket, have a node in DT.

The IPU driver needs to know the number of CSI-2 data lanes, which is
encoded in the data-lanes DT property present in both the sensor output
endpoint and the CSI-2 RX input endpoint, but not the other endpoints in
the pipeline.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Sakari Ailus]

Hi Laurent,

On Mon, Mar 20, 2023 at 11:26:02AM +0200, Laurent Pinchart wrote:
> In a (simplified) nutshell,
> 
> ---------+     +----------+     +---------+     +-----+     +-----+
> | Camera | --> | CSI-2 RX | --> | CSI2IPU | --> | Mux | --> | IPU |
> | Sensor |     |          |     | Gasket  |     |     |     |     |
> ---------+     +----------+     +---------+     +-----+     +-----+

Thank you, this is helpful.

I suppose the mux here at least won't actively do anything to the data. So
presumably its endpoint won't contain the active configuration, but its
superset.

> 
> All those blocks, except for the gasket, have a node in DT.
> 
> The IPU driver needs to know the number of CSI-2 data lanes, which is
> encoded in the data-lanes DT property present in both the sensor output
> endpoint and the CSI-2 RX input endpoint, but not the other endpoints in
> the pipeline.

This doesn't yet explain why the sensor would need to implement
get_mbus_config if its bus configuration remains constant.

I suppose those blocks in between would probably need something to convey
their active configuration from upstream sub-devices.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Laurent Pinchart]

On Mon, Mar 20, 2023 at 11:37:33AM +0200, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Mon, Mar 20, 2023 at 11:26:02AM +0200, Laurent Pinchart wrote:
> > In a (simplified) nutshell,
> > 
> > ---------+     +----------+     +---------+     +-----+     +-----+
> > | Camera | --> | CSI-2 RX | --> | CSI2IPU | --> | Mux | --> | IPU |
> > | Sensor |     |          |     | Gasket  |     |     |     |     |
> > ---------+     +----------+     +---------+     +-----+     +-----+
> 
> Thank you, this is helpful.
> 
> I suppose the mux here at least won't actively do anything to the data. So
> presumably its endpoint won't contain the active configuration, but its
> superset.
> 
> > 
> > All those blocks, except for the gasket, have a node in DT.
> > 
> > The IPU driver needs to know the number of CSI-2 data lanes, which is
> > encoded in the data-lanes DT property present in both the sensor output
> > endpoint and the CSI-2 RX input endpoint, but not the other endpoints in
> > the pipeline.
> 
> This doesn't yet explain why the sensor would need to implement
> get_mbus_config if its bus configuration remains constant.

If I recall correctly, the IPU driver calls .g_mbus_config() on the
camera sensor to get the number of lanes, as it can't get it from its
own endpoint. That's a hack, and as Jacopo proposed, calling
.g_mbus_config() on the CSI-2 RX would be better, as the CSI-2 RX driver
can then get the value from its own endpoint, without requiring all
sensor drivers to implement .g_mbus_config().

> I suppose those blocks in between would probably need something to convey
> their active configuration from upstream sub-devices.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Sakari Ailus]

Hi Laurent,

On Mon, Mar 20, 2023 at 11:55:14AM +0200, Laurent Pinchart wrote:
> On Mon, Mar 20, 2023 at 11:37:33AM +0200, Sakari Ailus wrote:
> > Hi Laurent,
> > 
> > On Mon, Mar 20, 2023 at 11:26:02AM +0200, Laurent Pinchart wrote:
> > > In a (simplified) nutshell,
> > > 
> > > ---------+     +----------+     +---------+     +-----+     +-----+
> > > | Camera | --> | CSI-2 RX | --> | CSI2IPU | --> | Mux | --> | IPU |
> > > | Sensor |     |          |     | Gasket  |     |     |     |     |
> > > ---------+     +----------+     +---------+     +-----+     +-----+
> > 
> > Thank you, this is helpful.
> > 
> > I suppose the mux here at least won't actively do anything to the data. So
> > presumably its endpoint won't contain the active configuration, but its
> > superset.
> > 
> > > 
> > > All those blocks, except for the gasket, have a node in DT.
> > > 
> > > The IPU driver needs to know the number of CSI-2 data lanes, which is
> > > encoded in the data-lanes DT property present in both the sensor output
> > > endpoint and the CSI-2 RX input endpoint, but not the other endpoints in
> > > the pipeline.
> > 
> > This doesn't yet explain why the sensor would need to implement
> > get_mbus_config if its bus configuration remains constant.
> 
> If I recall correctly, the IPU driver calls .g_mbus_config() on the
> camera sensor to get the number of lanes, as it can't get it from its
> own endpoint. That's a hack, and as Jacopo proposed, calling
> .g_mbus_config() on the CSI-2 RX would be better, as the CSI-2 RX driver
> can then get the value from its own endpoint, without requiring all
> sensor drivers to implement .g_mbus_config().

The g_mbus_config op could be implemented by the CSI2IPU and mux, by simply
requesting the information from the upstream sub-device. No hacks would be
needed.

-- 
Kind regrads,

Sakari Ailus

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Philipp Zabel]

On Mo, 2023-03-20 at 12:15 +0200, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Mon, Mar 20, 2023 at 11:55:14AM +0200, Laurent Pinchart wrote:
> > On Mon, Mar 20, 2023 at 11:37:33AM +0200, Sakari Ailus wrote:
> > > Hi Laurent,
> > > 
> > > On Mon, Mar 20, 2023 at 11:26:02AM +0200, Laurent Pinchart wrote:
> > > > In a (simplified) nutshell,
> > > > 
> > > > ---------+     +----------+     +---------+     +-----+     +-----+
> > > > > Camera | --> | CSI-2 RX | --> | CSI2IPU | --> | Mux | --> | IPU |
> > > > > Sensor |     |          |     | Gasket  |     |     |     |     |
> > > > ---------+     +----------+     +---------+     +-----+     +-----+
> > > 
> > > Thank you, this is helpful.
> > > 
> > > I suppose the mux here at least won't actively do anything to the data. So
> > > presumably its endpoint won't contain the active configuration, but its
> > > superset.
> > > 
> > > > 
> > > > All those blocks, except for the gasket, have a node in DT.
> > > > 
> > > > The IPU driver needs to know the number of CSI-2 data lanes, which is
> > > > encoded in the data-lanes DT property present in both the sensor output
> > > > endpoint and the CSI-2 RX input endpoint, but not the other endpoints in
> > > > the pipeline.
> > > 
> > > This doesn't yet explain why the sensor would need to implement
> > > get_mbus_config if its bus configuration remains constant.
> > 
> > If I recall correctly, the IPU driver calls .g_mbus_config() on the
> > camera sensor to get the number of lanes, as it can't get it from its
> > own endpoint. That's a hack, and as Jacopo proposed, calling
> > .g_mbus_config() on the CSI-2 RX would be better, as the CSI-2 RX driver
> > can then get the value from its own endpoint, without requiring all
> > sensor drivers to implement .g_mbus_config().
> 
> The g_mbus_config op could be implemented by the CSI2IPU and mux, by simply
> requesting the information from the upstream sub-device. No hacks would be
> needed.

I think implementing get_mbus_config on the mux might be enough. The
IPU driver already recognizes the CSI-2 RX by its grp_id and could
infer that it has to use MIPI CSI-2 mode from that.

The video-mux would have to forward get_mbus_config to its active
upstream port and if the upstream sensor does not implement it read bus
width from the active upstream endpoint.

regards
Philipp

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Laurent Pinchart]

On Mon, Mar 20, 2023 at 02:32:25PM +0100, Philipp Zabel wrote:
> On Mo, 2023-03-20 at 12:15 +0200, Sakari Ailus wrote:
> > On Mon, Mar 20, 2023 at 11:55:14AM +0200, Laurent Pinchart wrote:
> > > On Mon, Mar 20, 2023 at 11:37:33AM +0200, Sakari Ailus wrote:
> > > > On Mon, Mar 20, 2023 at 11:26:02AM +0200, Laurent Pinchart wrote:
> > > > > In a (simplified) nutshell,
> > > > > 
> > > > > ---------+     +----------+     +---------+     +-----+     +-----+
> > > > > > Camera | --> | CSI-2 RX | --> | CSI2IPU | --> | Mux | --> | IPU |
> > > > > > Sensor |     |          |     | Gasket  |     |     |     |     |
> > > > > ---------+     +----------+     +---------+     +-----+     +-----+
> > > > 
> > > > Thank you, this is helpful.
> > > > 
> > > > I suppose the mux here at least won't actively do anything to the data. So
> > > > presumably its endpoint won't contain the active configuration, but its
> > > > superset.
> > > > 
> > > > > 
> > > > > All those blocks, except for the gasket, have a node in DT.
> > > > > 
> > > > > The IPU driver needs to know the number of CSI-2 data lanes, which is
> > > > > encoded in the data-lanes DT property present in both the sensor output
> > > > > endpoint and the CSI-2 RX input endpoint, but not the other endpoints in
> > > > > the pipeline.
> > > > 
> > > > This doesn't yet explain why the sensor would need to implement
> > > > get_mbus_config if its bus configuration remains constant.
> > > 
> > > If I recall correctly, the IPU driver calls .g_mbus_config() on the
> > > camera sensor to get the number of lanes, as it can't get it from its
> > > own endpoint. That's a hack, and as Jacopo proposed, calling
> > > .g_mbus_config() on the CSI-2 RX would be better, as the CSI-2 RX driver
> > > can then get the value from its own endpoint, without requiring all
> > > sensor drivers to implement .g_mbus_config().
> > 
> > The g_mbus_config op could be implemented by the CSI2IPU and mux, by simply
> > requesting the information from the upstream sub-device. No hacks would be
> > needed.
> 
> I think implementing get_mbus_config on the mux might be enough. The
> IPU driver already recognizes the CSI-2 RX by its grp_id and could
> infer that it has to use MIPI CSI-2 mode from that.
> 
> The video-mux would have to forward get_mbus_config to its active
> upstream port and if the upstream sensor does not implement it read bus
> width from the active upstream endpoint.

I'm fine with implementing it in the mux as well, but I think we can
take a shortcut here and call it on the CSI-2 RX from the IPU driver, as
the IPU driver knows about the architecture of the whole pipeline.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Dave Stevenson]

On Mon, 20 Mar 2023 at 14:00, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Mon, Mar 20, 2023 at 02:32:25PM +0100, Philipp Zabel wrote:
> > On Mo, 2023-03-20 at 12:15 +0200, Sakari Ailus wrote:
> > > On Mon, Mar 20, 2023 at 11:55:14AM +0200, Laurent Pinchart wrote:
> > > > On Mon, Mar 20, 2023 at 11:37:33AM +0200, Sakari Ailus wrote:
> > > > > On Mon, Mar 20, 2023 at 11:26:02AM +0200, Laurent Pinchart wrote:
> > > > > > In a (simplified) nutshell,
> > > > > >
> > > > > > ---------+     +----------+     +---------+     +-----+     +-----+
> > > > > > > Camera | --> | CSI-2 RX | --> | CSI2IPU | --> | Mux | --> | IPU |
> > > > > > > Sensor |     |          |     | Gasket  |     |     |     |     |
> > > > > > ---------+     +----------+     +---------+     +-----+     +-----+
> > > > >
> > > > > Thank you, this is helpful.
> > > > >
> > > > > I suppose the mux here at least won't actively do anything to the data. So
> > > > > presumably its endpoint won't contain the active configuration, but its
> > > > > superset.
> > > > >
> > > > > >
> > > > > > All those blocks, except for the gasket, have a node in DT.
> > > > > >
> > > > > > The IPU driver needs to know the number of CSI-2 data lanes, which is
> > > > > > encoded in the data-lanes DT property present in both the sensor output
> > > > > > endpoint and the CSI-2 RX input endpoint, but not the other endpoints in
> > > > > > the pipeline.
> > > > >
> > > > > This doesn't yet explain why the sensor would need to implement
> > > > > get_mbus_config if its bus configuration remains constant.
> > > >
> > > > If I recall correctly, the IPU driver calls .g_mbus_config() on the
> > > > camera sensor to get the number of lanes, as it can't get it from its
> > > > own endpoint. That's a hack, and as Jacopo proposed, calling
> > > > .g_mbus_config() on the CSI-2 RX would be better, as the CSI-2 RX driver
> > > > can then get the value from its own endpoint, without requiring all
> > > > sensor drivers to implement .g_mbus_config().
> > >
> > > The g_mbus_config op could be implemented by the CSI2IPU and mux, by simply
> > > requesting the information from the upstream sub-device. No hacks would be
> > > needed.
> >
> > I think implementing get_mbus_config on the mux might be enough. The
> > IPU driver already recognizes the CSI-2 RX by its grp_id and could
> > infer that it has to use MIPI CSI-2 mode from that.
> >
> > The video-mux would have to forward get_mbus_config to its active
> > upstream port and if the upstream sensor does not implement it read bus
> > width from the active upstream endpoint.
>
> I'm fine with implementing it in the mux as well, but I think we can
> take a shortcut here and call it on the CSI-2 RX from the IPU driver, as
> the IPU driver knows about the architecture of the whole pipeline.

FWIW I have made use of video-mux and implementing g_mbus_config on it
for D-PHY switch chips[1] where the different input ports may have
different configurations. I'll admit that I've made the easy
assumption that it's CSI-2 D-PHY in and out, when it could quite
legitimately be working with any of the other bus types.

I had been intending to send this[2] upstream when I get a chance, but
am I reading imx6q.dtsi[3] correctly in that the mux is accepting
parallel on some ports and CSI-2 on others? The mux hardware is
therefore far more than just a simple switch between inputs? Although
as this is after the CSI2 rx block, this is effectively parallel data
within the SoC, therefore is the configuration and get_mbus_config
really relevant?
I'd like to understand how this is being used on imx6 before trying to
rework my patch into a generic solution.

Thanks
  Dave

[1] eg Arducam's 2 and 4 port muxes -
https://www.arducam.com/product-category/camera-multiplexers/, which
IIRC use OnSemi's FSA644
https://www.onsemi.com/products/interfaces/analog-switches/fsa644
[2] https://github.com/raspberrypi/linux/commit/bf653318475cf4db0ec59e92139f477f7cc0a544
[3] https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/imx6q.dtsi#L349

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Laurent Pinchart]

Hi Dave,

On Mon, Mar 20, 2023 at 06:31:04PM +0000, Dave Stevenson wrote:
> On Mon, 20 Mar 2023 at 14:00, Laurent Pinchart wrote:
> > On Mon, Mar 20, 2023 at 02:32:25PM +0100, Philipp Zabel wrote:
> > > On Mo, 2023-03-20 at 12:15 +0200, Sakari Ailus wrote:
> > > > On Mon, Mar 20, 2023 at 11:55:14AM +0200, Laurent Pinchart wrote:
> > > > > On Mon, Mar 20, 2023 at 11:37:33AM +0200, Sakari Ailus wrote:
> > > > > > On Mon, Mar 20, 2023 at 11:26:02AM +0200, Laurent Pinchart wrote:
> > > > > > > In a (simplified) nutshell,
> > > > > > >
> > > > > > > ---------+     +----------+     +---------+     +-----+     +-----+
> > > > > > > > Camera | --> | CSI-2 RX | --> | CSI2IPU | --> | Mux | --> | IPU |
> > > > > > > > Sensor |     |          |     | Gasket  |     |     |     |     |
> > > > > > > ---------+     +----------+     +---------+     +-----+     +-----+
> > > > > >
> > > > > > Thank you, this is helpful.
> > > > > >
> > > > > > I suppose the mux here at least won't actively do anything to the data. So
> > > > > > presumably its endpoint won't contain the active configuration, but its
> > > > > > superset.
> > > > > >
> > > > > > >
> > > > > > > All those blocks, except for the gasket, have a node in DT.
> > > > > > >
> > > > > > > The IPU driver needs to know the number of CSI-2 data lanes, which is
> > > > > > > encoded in the data-lanes DT property present in both the sensor output
> > > > > > > endpoint and the CSI-2 RX input endpoint, but not the other endpoints in
> > > > > > > the pipeline.
> > > > > >
> > > > > > This doesn't yet explain why the sensor would need to implement
> > > > > > get_mbus_config if its bus configuration remains constant.
> > > > >
> > > > > If I recall correctly, the IPU driver calls .g_mbus_config() on the
> > > > > camera sensor to get the number of lanes, as it can't get it from its
> > > > > own endpoint. That's a hack, and as Jacopo proposed, calling
> > > > > .g_mbus_config() on the CSI-2 RX would be better, as the CSI-2 RX driver
> > > > > can then get the value from its own endpoint, without requiring all
> > > > > sensor drivers to implement .g_mbus_config().
> > > >
> > > > The g_mbus_config op could be implemented by the CSI2IPU and mux, by simply
> > > > requesting the information from the upstream sub-device. No hacks would be
> > > > needed.
> > >
> > > I think implementing get_mbus_config on the mux might be enough. The
> > > IPU driver already recognizes the CSI-2 RX by its grp_id and could
> > > infer that it has to use MIPI CSI-2 mode from that.
> > >
> > > The video-mux would have to forward get_mbus_config to its active
> > > upstream port and if the upstream sensor does not implement it read bus
> > > width from the active upstream endpoint.
> >
> > I'm fine with implementing it in the mux as well, but I think we can
> > take a shortcut here and call it on the CSI-2 RX from the IPU driver, as
> > the IPU driver knows about the architecture of the whole pipeline.
> 
> FWIW I have made use of video-mux and implementing g_mbus_config on it
> for D-PHY switch chips[1] where the different input ports may have
> different configurations. I'll admit that I've made the easy
> assumption that it's CSI-2 D-PHY in and out, when it could quite
> legitimately be working with any of the other bus types.

That's a use case I hadn't considered. .get_mbus_config() makes sense.

> I had been intending to send this[2] upstream when I get a chance, but
> am I reading imx6q.dtsi[3] correctly in that the mux is accepting
> parallel on some ports and CSI-2 on others? The mux hardware is
> therefore far more than just a simple switch between inputs? Although
> as this is after the CSI2 rx block, this is effectively parallel data
> within the SoC, therefore is the configuration and get_mbus_config
> really relevant?

The exact hardware architecture isn't clear, but I indeed expect the mux
to receive parallel data from the CSI-2 RX and parallel inputs.

I had a closer look at the IPU driver code, and realized I was wrong
when I mentioned it needed to know the number of lanes. What the IPU
need is the bus type (CSI-2, BT656 or parallel) and, for parallel buses,
the bus width. It may be possible to refactor the IPU driver code to
replace that information with the media bus code in some places, but not
everywhere. Whether the input comes from the CSI-2 RX could be deduced
internally from the selected mux input, but we can't differentiate BT656
from parallel without querying the mux's input bus type. Calling
.get_mbus_config() on the mux is thus required, and the mux driver
should implement the operation.

> I'd like to understand how this is being used on imx6 before trying to
> rework my patch into a generic solution.
> 
> Thanks
>   Dave
> 
> [1] eg Arducam's 2 and 4 port muxes - https://www.arducam.com/product-category/camera-multiplexers/, which
> IIRC use OnSemi's FSA644 https://www.onsemi.com/products/interfaces/analog-switches/fsa644
> [2] https://github.com/raspberrypi/linux/commit/bf653318475cf4db0ec59e92139f477f7cc0a544
> [3] https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/imx6q.dtsi#L349

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Sakari Ailus]

Hi Laurent,

On Mon, Mar 20, 2023 at 04:00:12PM +0200, Laurent Pinchart wrote:
> On Mon, Mar 20, 2023 at 02:32:25PM +0100, Philipp Zabel wrote:
> > On Mo, 2023-03-20 at 12:15 +0200, Sakari Ailus wrote:
> > > On Mon, Mar 20, 2023 at 11:55:14AM +0200, Laurent Pinchart wrote:
> > > > On Mon, Mar 20, 2023 at 11:37:33AM +0200, Sakari Ailus wrote:
> > > > > On Mon, Mar 20, 2023 at 11:26:02AM +0200, Laurent Pinchart wrote:
> > > > > > In a (simplified) nutshell,
> > > > > > 
> > > > > > ---------+     +----------+     +---------+     +-----+     +-----+
> > > > > > > Camera | --> | CSI-2 RX | --> | CSI2IPU | --> | Mux | --> | IPU |
> > > > > > > Sensor |     |          |     | Gasket  |     |     |     |     |
> > > > > > ---------+     +----------+     +---------+     +-----+     +-----+
> > > > > 
> > > > > Thank you, this is helpful.
> > > > > 
> > > > > I suppose the mux here at least won't actively do anything to the data. So
> > > > > presumably its endpoint won't contain the active configuration, but its
> > > > > superset.
> > > > > 
> > > > > > 
> > > > > > All those blocks, except for the gasket, have a node in DT.
> > > > > > 
> > > > > > The IPU driver needs to know the number of CSI-2 data lanes, which is
> > > > > > encoded in the data-lanes DT property present in both the sensor output
> > > > > > endpoint and the CSI-2 RX input endpoint, but not the other endpoints in
> > > > > > the pipeline.
> > > > > 
> > > > > This doesn't yet explain why the sensor would need to implement
> > > > > get_mbus_config if its bus configuration remains constant.
> > > > 
> > > > If I recall correctly, the IPU driver calls .g_mbus_config() on the
> > > > camera sensor to get the number of lanes, as it can't get it from its
> > > > own endpoint. That's a hack, and as Jacopo proposed, calling
> > > > .g_mbus_config() on the CSI-2 RX would be better, as the CSI-2 RX driver
> > > > can then get the value from its own endpoint, without requiring all
> > > > sensor drivers to implement .g_mbus_config().
> > > 
> > > The g_mbus_config op could be implemented by the CSI2IPU and mux, by simply
> > > requesting the information from the upstream sub-device. No hacks would be
> > > needed.
> > 
> > I think implementing get_mbus_config on the mux might be enough. The
> > IPU driver already recognizes the CSI-2 RX by its grp_id and could
> > infer that it has to use MIPI CSI-2 mode from that.
> > 
> > The video-mux would have to forward get_mbus_config to its active
> > upstream port and if the upstream sensor does not implement it read bus
> > width from the active upstream endpoint.
> 
> I'm fine with implementing it in the mux as well, but I think we can
> take a shortcut here and call it on the CSI-2 RX from the IPU driver, as
> the IPU driver knows about the architecture of the whole pipeline.

If that's the case then I guess that's fine. But can these drivers be used
elsewhere than with IMX6?

It'd be safest to implement g_mbus_config for the all the way to CSI-2 RX.

But if the answer to the question is "no", then making that shortcut should
be fine (and this can be always reworked if need be).

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Philipp Zabel]

On Mo, 2023-03-20 at 11:55 +0200, Laurent Pinchart wrote:
> On Mon, Mar 20, 2023 at 11:37:33AM +0200, Sakari Ailus wrote:
> > Hi Laurent,
> > 
> > On Mon, Mar 20, 2023 at 11:26:02AM +0200, Laurent Pinchart wrote:
> > > In a (simplified) nutshell,
> > > 
> > > ---------+     +----------+     +---------+     +-----+     +-----+
> > > > Camera | --> | CSI-2 RX | --> | CSI2IPU | --> | Mux | --> | IPU |
> > > > Sensor |     |          |     | Gasket  |     |     |     |     |
> > > ---------+     +----------+     +---------+     +-----+     +-----+
> > 
> > Thank you, this is helpful.
> > 
> > I suppose the mux here at least won't actively do anything to the data. So
> > presumably its endpoint won't contain the active configuration, but its
> > superset.
> > 
> > > 
> > > All those blocks, except for the gasket, have a node in DT.
> > > 
> > > The IPU driver needs to know the number of CSI-2 data lanes, which is
> > > encoded in the data-lanes DT property present in both the sensor output
> > > endpoint and the CSI-2 RX input endpoint, but not the other endpoints in
> > > the pipeline.
> > 
> > This doesn't yet explain why the sensor would need to implement
> > get_mbus_config if its bus configuration remains constant.
> 
> If I recall correctly, the IPU driver calls .g_mbus_config() on the
> camera sensor to get the number of lanes, as it can't get it from its
> own endpoint. That's a hack, and as Jacopo proposed, calling
> .g_mbus_config() on the CSI-2 RX would be better, as the CSI-2 RX driver
> can then get the value from its own endpoint, without requiring all
> sensor drivers to implement .g_mbus_config().

The IPU driver doesn't call get_mbus_config, the CSI-2 RX driver does
(csi2_get_active_lanes in imx6-mipi-csi2.c). It could just fall back to
looking at its own endpoint if the upstream driver does not implement
get_mbus_config.

Of course that will only help if the DT contains this information and
all connected lanes are active.

regards
Philipp

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Jacopo Mondi]

Hi Philipp

On Mon, Mar 20, 2023 at 12:26:26PM +0100, Philipp Zabel wrote:
> On Mo, 2023-03-20 at 11:55 +0200, Laurent Pinchart wrote:
> > On Mon, Mar 20, 2023 at 11:37:33AM +0200, Sakari Ailus wrote:
> > > Hi Laurent,
> > >
> > > On Mon, Mar 20, 2023 at 11:26:02AM +0200, Laurent Pinchart wrote:
> > > > In a (simplified) nutshell,
> > > >
> > > > ---------+     +----------+     +---------+     +-----+     +-----+
> > > > > Camera | --> | CSI-2 RX | --> | CSI2IPU | --> | Mux | --> | IPU |
> > > > > Sensor |     |          |     | Gasket  |     |     |     |     |
> > > > ---------+     +----------+     +---------+     +-----+     +-----+
> > >
> > > Thank you, this is helpful.
> > >
> > > I suppose the mux here at least won't actively do anything to the data. So
> > > presumably its endpoint won't contain the active configuration, but its
> > > superset.
> > >
> > > >
> > > > All those blocks, except for the gasket, have a node in DT.
> > > >
> > > > The IPU driver needs to know the number of CSI-2 data lanes, which is
> > > > encoded in the data-lanes DT property present in both the sensor output
> > > > endpoint and the CSI-2 RX input endpoint, but not the other endpoints in
> > > > the pipeline.
> > >
> > > This doesn't yet explain why the sensor would need to implement
> > > get_mbus_config if its bus configuration remains constant.
> >
> > If I recall correctly, the IPU driver calls .g_mbus_config() on the
> > camera sensor to get the number of lanes, as it can't get it from its
> > own endpoint. That's a hack, and as Jacopo proposed, calling
> > .g_mbus_config() on the CSI-2 RX would be better, as the CSI-2 RX driver
> > can then get the value from its own endpoint, without requiring all
> > sensor drivers to implement .g_mbus_config().
>
> The IPU driver doesn't call get_mbus_config, the CSI-2 RX driver does

Am I confusing IPU CSI with CSI-2 ?
https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/imx/imx-media-csi.c#L211

> (csi2_get_active_lanes in imx6-mipi-csi2.c). It could just fall back to
> looking at its own endpoint if the upstream driver does not implement
> get_mbus_config.
>
> Of course that will only help if the DT contains this information and
> all connected lanes are active.

We should assume DTs are correct, otherwise we're screwed most of the
times...

>
> regards
> Philipp

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Philipp Zabel]

On Mo, 2023-03-20 at 13:47 +0100, Jacopo Mondi wrote:
> Hi Philipp
> 
> On Mon, Mar 20, 2023 at 12:26:26PM +0100, Philipp Zabel wrote:
> > On Mo, 2023-03-20 at 11:55 +0200, Laurent Pinchart wrote:
> > > On Mon, Mar 20, 2023 at 11:37:33AM +0200, Sakari Ailus wrote:
> > > > Hi Laurent,
> > > > 
> > > > On Mon, Mar 20, 2023 at 11:26:02AM +0200, Laurent Pinchart wrote:
> > > > > In a (simplified) nutshell,
> > > > > 
> > > > > ---------+     +----------+     +---------+     +-----+     +-----+
> > > > > > Camera | --> | CSI-2 RX | --> | CSI2IPU | --> | Mux | --> | IPU |
> > > > > > Sensor |     |          |     | Gasket  |     |     |     |     |
> > > > > ---------+     +----------+     +---------+     +-----+     +-----+
> > > > 
> > > > Thank you, this is helpful.
> > > > 
> > > > I suppose the mux here at least won't actively do anything to the data. So
> > > > presumably its endpoint won't contain the active configuration, but its
> > > > superset.
> > > > 
> > > > > 
> > > > > All those blocks, except for the gasket, have a node in DT.
> > > > > 
> > > > > The IPU driver needs to know the number of CSI-2 data lanes, which is
> > > > > encoded in the data-lanes DT property present in both the sensor output
> > > > > endpoint and the CSI-2 RX input endpoint, but not the other endpoints in
> > > > > the pipeline.
> > > > 
> > > > This doesn't yet explain why the sensor would need to implement
> > > > get_mbus_config if its bus configuration remains constant.
> > > 
> > > If I recall correctly, the IPU driver calls .g_mbus_config() on the
> > > camera sensor to get the number of lanes, as it can't get it from its
> > > own endpoint. That's a hack, and as Jacopo proposed, calling
> > > .g_mbus_config() on the CSI-2 RX would be better, as the CSI-2 RX driver
> > > can then get the value from its own endpoint, without requiring all
> > > sensor drivers to implement .g_mbus_config().
> > 
> > The IPU driver doesn't call get_mbus_config, the CSI-2 RX driver does
> 
> Am I confusing IPU CSI with CSI-2 ?
> https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/imx/imx-media-csi.c#L211

What the eyesight ... Sorry for the confusion. You are right, it's
right there. Yes, that is IPU CSI calling get_mbus_config.

I had git grepped for get_mbus_config this morning, and was convinced I
only found the call in imx-media-csi2, which is where the number of
lanes is evaluated. The call in imx-media-csi.c is used to determine
whether the sensor is parallel or CSI-2 and whether the CSI has to
operate in bypass mode (for 16-bit parallel bus).

regards
Philipp

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Francesco Dolcini]

On Mon, Mar 20, 2023 at 09:48:44AM +0100, Jacopo Mondi wrote:
> On Tue, Mar 14, 2023 at 01:59:06PM +0100, Francesco Dolcini wrote:
> > On Tue, Mar 14, 2023 at 02:45:53PM +0200, Sakari Ailus wrote:
> > > On Tue, Mar 14, 2023 at 01:32:16PM +0100, Francesco Dolcini wrote:
> > > > On Tue, Mar 14, 2023 at 02:13:46PM +0200, Sakari Ailus wrote:
> > > > > On Mon, Mar 06, 2023 at 07:36:49AM +0100, Marcel Ziswiler wrote:
> > > > > > From: Aishwarya Kothari <aishwarya.kothari@toradex.com>
> > > > > >
> > > > > > Implement the introduced get_mbus_config operation to report the
> > > > > > config of the MIPI CSI-2, BT.656 and Parallel interface.
> > > > > >
> > > > > > Signed-off-by: Aishwarya Kothari <aishwarya.kothari@toradex.com>
> > > > > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > > >
> > > > > What's the reasoning for this patch?
> > > >
> > > > Without this it's not possible to use it on i.MX6,
> > > > drivers/staging/media/imx/imx6-mipi-csi2.c requires it, some more
> > > > details from Jacopo here [0].
> > > >
> > > > Everything used to work fine up to v5.18, after that kernel version
> > > > various changes broke it [1][2] (I assume you are pretty much aware of
> > > > the history here, you commented on a few emails).
> > > >
> > > > [0] https://lore.kernel.org/all/20230128100611.7ulsfqqqgscg54gy@uno.localdomain/
> > > > [1] https://lore.kernel.org/all/081cc2d3-1f3a-6c14-6dc7-53f976be7b2b@gmail.com/
> > > > [2] https://lore.kernel.org/all/cacfe146-101b-35b3-5f66-1a1cabfd342f@gmail.com/
> > > >
> > > > > Drivers that don't have e.g. dynamic lane configuration shouldn't need to
> > > > > implement get_mbus_config.
> > >
> > > Not even for staging drivers. The driver should be fixed to get that
> > > information from the endpoint instead.
> >
> > This seems exactly the opposite of what commit
> > 7318abface48 ("media: imx: Use get_mbus_config instead of parsing upstream DT endpoints")
> > did.
> >
> > Given that I am somehow confused, but I am not that familiar with this
> > subsystem, so I guess this is expected :-). Can someone provide some
> > additional hint here?
> >
> To be honest my understanding is that this patch has always been
> needed to work on imx6 and this is not a regression but something that
> was kept as an out-of-tree patch downstream. Is this correct or is
> this a regression ?

I confirm that v5.18 was/is fine.  Aishwarya: correct? In the end you
tested it, not me :-)

Francesco

Re: [PATCH v2] media: i2c: ov5640: Implement get_mbus_config

[Aishwarya Kothari]

On 20.03.23 12:13, Francesco Dolcini wrote:
> On Mon, Mar 20, 2023 at 09:48:44AM +0100, Jacopo Mondi wrote:
>> On Tue, Mar 14, 2023 at 01:59:06PM +0100, Francesco Dolcini wrote:
>>> On Tue, Mar 14, 2023 at 02:45:53PM +0200, Sakari Ailus wrote:
>>>> On Tue, Mar 14, 2023 at 01:32:16PM +0100, Francesco Dolcini wrote:
>>>>> On Tue, Mar 14, 2023 at 02:13:46PM +0200, Sakari Ailus wrote:
>>>>>> On Mon, Mar 06, 2023 at 07:36:49AM +0100, Marcel Ziswiler wrote:
>>>>>>> From: Aishwarya Kothari <aishwarya.kothari@toradex.com>
>>>>>>>
>>>>>>> Implement the introduced get_mbus_config operation to report the
>>>>>>> config of the MIPI CSI-2, BT.656 and Parallel interface.
>>>>>>>
>>>>>>> Signed-off-by: Aishwarya Kothari <aishwarya.kothari@toradex.com>
>>>>>>> Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
>>>>>>
>>>>>> What's the reasoning for this patch?
>>>>>
>>>>> Without this it's not possible to use it on i.MX6,
>>>>> drivers/staging/media/imx/imx6-mipi-csi2.c requires it, some more
>>>>> details from Jacopo here [0].
>>>>>
>>>>> Everything used to work fine up to v5.18, after that kernel version
>>>>> various changes broke it [1][2] (I assume you are pretty much aware of
>>>>> the history here, you commented on a few emails).
>>>>>
>>>>> [0] https://lore.kernel.org/all/20230128100611.7ulsfqqqgscg54gy@uno.localdomain/
>>>>> [1] https://lore.kernel.org/all/081cc2d3-1f3a-6c14-6dc7-53f976be7b2b@gmail.com/
>>>>> [2] https://lore.kernel.org/all/cacfe146-101b-35b3-5f66-1a1cabfd342f@gmail.com/
>>>>>
>>>>>> Drivers that don't have e.g. dynamic lane configuration shouldn't need to
>>>>>> implement get_mbus_config.
>>>>
>>>> Not even for staging drivers. The driver should be fixed to get that
>>>> information from the endpoint instead.
>>>
>>> This seems exactly the opposite of what commit
>>> 7318abface48 ("media: imx: Use get_mbus_config instead of parsing upstream DT endpoints")
>>> did.
>>>
>>> Given that I am somehow confused, but I am not that familiar with this
>>> subsystem, so I guess this is expected :-). Can someone provide some
>>> additional hint here?
>>>
>> To be honest my understanding is that this patch has always been
>> needed to work on imx6 and this is not a regression but something that
>> was kept as an out-of-tree patch downstream. Is this correct or is
>> this a regression ?
> 
> I confirm that v5.18 was/is fine.  Aishwarya: correct? In the end you
> tested it, not me :-)
> 
> Francesco
> 
> 
It worked on the v5.18 without this patch.

Aishwarya