[PATCH v4 0/3] Add g_csi_active_lanes for dynamic active lanes

[PATCH v4 0/3] Add g_csi_active_lanes for dynamic active lanes

[Philipp Zabel]

Some MIPI CSI-2 transmitters, such as TC358743 can dynamically change
the number of active data lanes depending on the bandwidth needs for the
selected video format. This patchset adds a subdevice video operation
g_csi_active_lanes() to let the MIPI CSI-2 receiver query the number of
active lanes and change its configuration accordingly.

Changes since v3 [1]:
- Add g_csi_active_lanes() subdevice video operation,
  implement it in tc358743, and use it in imx6-mipi-csi2.

[1] https://patchwork.linuxtv.org/patch/53331/

regards
Philipp

Philipp Zabel (3):
  media: v4l2-subdev: add g_csi_active_lanes() op
  media: tc358743: fix connected/active CSI-2 lane reporting
  media: imx: ask source subdevice for number of active data lanes

 drivers/media/i2c/tc358743.c               | 44 ++++++++++++++++------
 drivers/staging/media/imx/imx6-mipi-csi2.c |  8 ++--
 include/media/v4l2-subdev.h                |  3 ++
 3 files changed, 39 insertions(+), 16 deletions(-)

-- 
2.20.1

[PATCH v4 1/3] media: v4l2-subdev: add g_csi_active_lanes() op

[Philipp Zabel]

Add a subdevice video operation that allows to query the number
of data lanes a MIPI CSI-2 TX is actively transmitting on.

Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
New in v4.
---
 include/media/v4l2-subdev.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 71f1f2f0da53..bb71eedc38f6 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -411,6 +411,8 @@ struct v4l2_mbus_frame_desc {
  * @s_rx_buffer: set a host allocated memory buffer for the subdev. The subdev
  *	can adjust @size to a lower value and must not write more data to the
  *	buffer starting at @data than the original value of @size.
+ *
+ * @g_csi_active_lanes: Get number of currently active MIPI CSI-2 data lanes.
  */
 struct v4l2_subdev_video_ops {
 	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config);
@@ -441,6 +443,7 @@ struct v4l2_subdev_video_ops {
 			     const struct v4l2_mbus_config *cfg);
 	int (*s_rx_buffer)(struct v4l2_subdev *sd, void *buf,
 			   unsigned int *size);
+	int (*g_csi_active_lanes)(struct v4l2_subdev *sd, u32 *lanes);
 };
 
 /**
-- 
2.20.1

Re: [PATCH v4 1/3] media: v4l2-subdev: add g_csi_active_lanes() op

[Laurent Pinchart]

Hi Philipp,

(CC'ing Sakari, Jacopo, Kieran and Niklas)

Thank you for the patch.

On Tue, Sep 24, 2019 at 01:49:53PM +0200, Philipp Zabel wrote:
> Add a subdevice video operation that allows to query the number
> of data lanes a MIPI CSI-2 TX is actively transmitting on.
> 
> Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> New in v4.
> ---
>  include/media/v4l2-subdev.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 71f1f2f0da53..bb71eedc38f6 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -411,6 +411,8 @@ struct v4l2_mbus_frame_desc {
>   * @s_rx_buffer: set a host allocated memory buffer for the subdev. The subdev
>   *	can adjust @size to a lower value and must not write more data to the
>   *	buffer starting at @data than the original value of @size.
> + *
> + * @g_csi_active_lanes: Get number of currently active MIPI CSI-2 data lanes.
>   */
>  struct v4l2_subdev_video_ops {
>  	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config);
> @@ -441,6 +443,7 @@ struct v4l2_subdev_video_ops {
>  			     const struct v4l2_mbus_config *cfg);
>  	int (*s_rx_buffer)(struct v4l2_subdev *sd, void *buf,
>  			   unsigned int *size);
> +	int (*g_csi_active_lanes)(struct v4l2_subdev *sd, u32 *lanes);

This shouldn't be a video operation but a pad operation, as a subdev
could have multiple CSI-2 pads.

Furthermore, you need to define the semantics of this operation more
precisely. When can it be called, when is the information valid ? Can
the subdev change the number of lanes it supports at runtime ? If so,
how are race conditions avoided ? All this needs to be documented.

Finally, the number of lanes is far from being the only information
about a physical bus that could be interesting for a remote subdev. I
would much prefer a more generic operation to retrieve bus
information/configuration, with a data structure that we will be able to
extend later.

>  };
>  
>  /**

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 1/3] media: v4l2-subdev: add g_csi_active_lanes() op

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 3559 bytes --]

Hello,
   sorry for having missed this

On Wed, Sep 25, 2019 at 04:41:13PM +0300, Laurent Pinchart wrote:
> Hi Philipp,
>
> (CC'ing Sakari, Jacopo, Kieran and Niklas)
>
> Thank you for the patch.
>
> On Tue, Sep 24, 2019 at 01:49:53PM +0200, Philipp Zabel wrote:
> > Add a subdevice video operation that allows to query the number
> > of data lanes a MIPI CSI-2 TX is actively transmitting on.
> >
> > Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > New in v4.
> > ---
> >  include/media/v4l2-subdev.h | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 71f1f2f0da53..bb71eedc38f6 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -411,6 +411,8 @@ struct v4l2_mbus_frame_desc {
> >   * @s_rx_buffer: set a host allocated memory buffer for the subdev. The subdev
> >   *	can adjust @size to a lower value and must not write more data to the
> >   *	buffer starting at @data than the original value of @size.
> > + *
> > + * @g_csi_active_lanes: Get number of currently active MIPI CSI-2 data lanes.
> >   */
> >  struct v4l2_subdev_video_ops {
> >  	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config);
> > @@ -441,6 +443,7 @@ struct v4l2_subdev_video_ops {
> >  			     const struct v4l2_mbus_config *cfg);
> >  	int (*s_rx_buffer)(struct v4l2_subdev *sd, void *buf,
> >  			   unsigned int *size);
> > +	int (*g_csi_active_lanes)(struct v4l2_subdev *sd, u32 *lanes);
>
> This shouldn't be a video operation but a pad operation, as a subdev
> could have multiple CSI-2 pads.
>
> Furthermore, you need to define the semantics of this operation more
> precisely. When can it be called, when is the information valid ? Can
> the subdev change the number of lanes it supports at runtime ? If so,
> how are race conditions avoided ? All this needs to be documented.
>
> Finally, the number of lanes is far from being the only information
> about a physical bus that could be interesting for a remote subdev. I
> would much prefer a more generic operation to retrieve bus
> information/configuration, with a data structure that we will be able to
> extend later.
>

For the record we tried to get those information from the frame
descriptor (the number of used data lanes and the clock
continous/non-continous information to be precise)

This is the RFC series I sent
https://patchwork.kernel.org/project/linux-media/list/?series=92501

Which depends on Sakari's patches:
https://patchwork.kernel.org/patch/10875871/
https://patchwork.kernel.org/patch/10875873/

The latest two were part of a much bigger series that tried to add
support for multiplexed streams. Honestly, it now seems to be that
part could be breakout without involving streams, and re-use that
portion to negotiate the csi-2 bus configuration. I might be wrong
though, and the two parts could not be separate easily (from a very
quick look, after months, it does not seem so).

In the past I spoke against this solution as I would have preferred
leaving frame_desc alone and introduce a bus configuration operation.
I tried a few times and I ended up implementing g_mbus_format but on
pads and not video. Right now with Sakari's and Laurent's ack I would
say re-using frame_desc might actually work and get use a feature
which is needed by many (cc also Dave, as he had a similar issue with
TC358743 iirc)

Thanks
  j


> >  };
> >
> >  /**
>
> --
> Regards,
>
> Laurent Pinchart

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 1/3] media: v4l2-subdev: add g_csi_active_lanes() op

[Philipp Zabel]

Hi Jacopo,

On Wed, 2019-09-25 at 16:51 +0200, Jacopo Mondi wrote:
> Hello,
>    sorry for having missed this

Thank you and Laurent for completing the list of interested parties, I
had completely forgotten about the frame descriptors.

> On Wed, Sep 25, 2019 at 04:41:13PM +0300, Laurent Pinchart wrote:
> > Hi Philipp,
> > 
> > (CC'ing Sakari, Jacopo, Kieran and Niklas)
> > 
> > Thank you for the patch.
> > 
> > On Tue, Sep 24, 2019 at 01:49:53PM +0200, Philipp Zabel wrote:
> > > Add a subdevice video operation that allows to query the number
> > > of data lanes a MIPI CSI-2 TX is actively transmitting on.
> > > 
> > > Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > ---
> > > New in v4.
> > > ---
> > >  include/media/v4l2-subdev.h | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index 71f1f2f0da53..bb71eedc38f6 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -411,6 +411,8 @@ struct v4l2_mbus_frame_desc {
> > >   * @s_rx_buffer: set a host allocated memory buffer for the subdev. The subdev
> > >   *	can adjust @size to a lower value and must not write more data to the
> > >   *	buffer starting at @data than the original value of @size.
> > > + *
> > > + * @g_csi_active_lanes: Get number of currently active MIPI CSI-2 data lanes.
> > >   */
> > >  struct v4l2_subdev_video_ops {
> > >  	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config);
> > > @@ -441,6 +443,7 @@ struct v4l2_subdev_video_ops {
> > >  			     const struct v4l2_mbus_config *cfg);
> > >  	int (*s_rx_buffer)(struct v4l2_subdev *sd, void *buf,
> > >  			   unsigned int *size);
> > > +	int (*g_csi_active_lanes)(struct v4l2_subdev *sd, u32 *lanes);
> > 
> > This shouldn't be a video operation but a pad operation, as a subdev
> > could have multiple CSI-2 pads.
> > 
> > Furthermore, you need to define the semantics of this operation more
> > precisely. When can it be called, when is the information valid ? Can
> > the subdev change the number of lanes it supports at runtime ? If so,
> > how are race conditions avoided ? All this needs to be documented.
> > 
> > Finally, the number of lanes is far from being the only information
> > about a physical bus that could be interesting for a remote subdev. I
> > would much prefer a more generic operation to retrieve bus
> > information/configuration, with a data structure that we will be able to
> > extend later.
> > 
> 
> For the record we tried to get those information from the frame
> descriptor (the number of used data lanes and the clock
> continous/non-continous information to be precise)
> 
> This is the RFC series I sent
> https://patchwork.kernel.org/project/linux-media/list/?series=92501
> 
> Which depends on Sakari's patches:
> https://patchwork.kernel.org/patch/10875871/
> https://patchwork.kernel.org/patch/10875873/
> 
> The latest two were part of a much bigger series that tried to add
> support for multiplexed streams. Honestly, it now seems to be that
> part could be breakout without involving streams, and re-use that
> portion to negotiate the csi-2 bus configuration. I might be wrong
> though, and the two parts could not be separate easily (from a very
> quick look, after months, it does not seem so).
> 
> In the past I spoke against this solution as I would have preferred
> leaving frame_desc alone and introduce a bus configuration operation.
> I tried a few times and I ended up implementing g_mbus_format but on
> pads and not video. Right now with Sakari's and Laurent's ack I would
> say re-using frame_desc might actually work and get use a feature
> which is needed by many (cc also Dave, as he had a similar issue with
> TC358743 iirc)

That looks like it should work just as well. If there is agreement to
add the number of data lanes, (non-)continuous clock flag, and possibly
the chosen link frequency v4l2_mbus_frame_desc, I'll happily dig up
these patches and switch to .get_frame_desc().

regards
Philipp

Re: [PATCH v4 1/3] media: v4l2-subdev: add g_csi_active_lanes() op

[Philipp Zabel]

Hi Laurent,

On Wed, 2019-09-25 at 16:41 +0300, Laurent Pinchart wrote:
> Hi Philipp,
> 
> (CC'ing Sakari, Jacopo, Kieran and Niklas)
> 
> Thank you for the patch.
> 
> On Tue, Sep 24, 2019 at 01:49:53PM +0200, Philipp Zabel wrote:
> > Add a subdevice video operation that allows to query the number
> > of data lanes a MIPI CSI-2 TX is actively transmitting on.
> > 
> > Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > New in v4.
> > ---
> >  include/media/v4l2-subdev.h | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 71f1f2f0da53..bb71eedc38f6 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -411,6 +411,8 @@ struct v4l2_mbus_frame_desc {
> >   * @s_rx_buffer: set a host allocated memory buffer for the subdev. The subdev
> >   *	can adjust @size to a lower value and must not write more data to the
> >   *	buffer starting at @data than the original value of @size.
> > + *
> > + * @g_csi_active_lanes: Get number of currently active MIPI CSI-2 data lanes.
> >   */
> >  struct v4l2_subdev_video_ops {
> >  	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config);
> > @@ -441,6 +443,7 @@ struct v4l2_subdev_video_ops {
> >  			     const struct v4l2_mbus_config *cfg);
> >  	int (*s_rx_buffer)(struct v4l2_subdev *sd, void *buf,
> >  			   unsigned int *size);
> > +	int (*g_csi_active_lanes)(struct v4l2_subdev *sd, u32 *lanes);
> 
> This shouldn't be a video operation but a pad operation, as a subdev
> could have multiple CSI-2 pads.

Right this should be pad specific.

> Furthermore, you need to define the semantics of this operation more
> precisely. When can it be called,

The downstream subdevice connected to this pad is expected to call this
in its s_stream(enable=1) op, right before enabling the MIPI CSI-2 RX,
and then calling s_stream(enable=1) on the same upstream subdevice.

The returned value is a decision by the upstream subdevice driver based
on external factors such as available link-frequencies and mbus frame
format, so it can change whenever those are changed, but not by itself.

> when is the information valid ?

It is valid until the next time the pad's mbus frame format or link
frequency are changed. Since the caller

> Can the subdev change the number of lanes it supports at runtime ?

At least for MIPI CSI-2, no. Are there any buses that can dynamically
change bus width while active?

> If so, how are race conditions avoided ? All this needs to be documented.

I think no, so the only possible race conditions would be with
reconfiguration, which should already be avoided by requiring this to be
called from s_stream(),as the media pipeline is already started and
all configuration locked in place at this point.

> Finally, the number of lanes is far from being the only information
> about a physical bus that could be interesting for a remote subdev. I
> would much prefer a more generic operation to retrieve bus
> information/configuration, with a data structure that we will be able to
> extend later.

This is specifically about configuration chosen by the transmitter, not
physical bus properties, which can be specified in the device tree. The
chosen link frequency (if more than one is specified in DT) could be one
of those values.

regards
Philipp

[PATCH v4 2/3] media: tc358743: fix connected/active CSI-2 lane reporting

[Philipp Zabel]

g_mbus_config was supposed to indicate all supported lane numbers, not
only the number of those currently in active use. Since the TC358743
can dynamically reduce the number of active lanes if the required
bandwidth allows for it, report all lane numbers up to the connected
number of lanes as supported in pdata mode.
In device tree mode, do not report lane count and clock mode at all, as
the receiver driver can determine these from the device tree.

To allow communicating the number of currently active lanes, use the
newly added g_csi_active_lanes() video op.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v3:
 - Use g_csi_active_lanes instead of storing the number of active lanes
   in mbus_config flags.
---
 drivers/media/i2c/tc358743.c | 44 ++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index dbbab75f135e..c2db6419a4b4 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1607,23 +1607,31 @@ static int tc358743_g_mbus_config(struct v4l2_subdev *sd,
 {
 	struct tc358743_state *state = to_state(sd);
 
+	if (state->csi_lanes_in_use > state->bus.num_data_lanes)
+		return -EINVAL;
+
 	cfg->type = V4L2_MBUS_CSI2_DPHY;
+	cfg->flags = 0;
 
-	/* Support for non-continuous CSI-2 clock is missing in the driver */
-	cfg->flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
+	/* In DT mode, there is no need to report the number of data lanes */
+	if (sd->dev->of_node)
+		return 0;
 
-	switch (state->csi_lanes_in_use) {
-	case 1:
-		cfg->flags |= V4L2_MBUS_CSI2_1_LANE;
-		break;
-	case 2:
-		cfg->flags |= V4L2_MBUS_CSI2_2_LANE;
-		break;
+	/* Support for non-continuous CSI-2 clock is missing in pdata mode */
+	cfg->flags |= V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
+
+	switch (state->bus.num_data_lanes) {
+	case 4:
+		cfg->flags |= V4L2_MBUS_CSI2_LANES;
+		/* fallthrough */
 	case 3:
 		cfg->flags |= V4L2_MBUS_CSI2_3_LANE;
-		break;
-	case 4:
-		cfg->flags |= V4L2_MBUS_CSI2_4_LANE;
+		/* fallthrough */
+	case 2:
+		cfg->flags |= V4L2_MBUS_CSI2_2_LANE;
+		/* fallthrough */
+	case 1:
+		cfg->flags |= V4L2_MBUS_CSI2_1_LANE;
 		break;
 	default:
 		return -EINVAL;
@@ -1632,6 +1640,16 @@ static int tc358743_g_mbus_config(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int tc358743_g_csi_active_lanes(struct v4l2_subdev *sd,
+				       u32 *lanes)
+{
+	struct tc358743_state *state = to_state(sd);
+
+	*lanes = state->csi_lanes_in_use;
+
+	return 0;
+}
+
 static int tc358743_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	enable_stream(sd, enable);
@@ -1838,6 +1856,7 @@ static const struct v4l2_subdev_video_ops tc358743_video_ops = {
 	.query_dv_timings = tc358743_query_dv_timings,
 	.g_mbus_config = tc358743_g_mbus_config,
 	.s_stream = tc358743_s_stream,
+	.g_csi_active_lanes = tc358743_g_csi_active_lanes,
 };
 
 static const struct v4l2_subdev_pad_ops tc358743_pad_ops = {
@@ -2052,6 +2071,7 @@ static int tc358743_probe(struct i2c_client *client)
 	if (pdata) {
 		state->pdata = *pdata;
 		state->bus.flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
+		state->bus.num_data_lanes = 4;
 	} else {
 		err = tc358743_probe_of(state);
 		if (err == -ENODEV)
-- 
2.20.1

[PATCH v4 3/3] media: imx: ask source subdevice for number of active data lanes

[Philipp Zabel]

Use the newly added g_csi_active_lanes() video op to determine the
number of active data lanes used by the transmitter. If this subdev
call is not supported or does not return the number of active lanes,
default to using all connected data lanes as before.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v3:
 - Use g_csi_active_lanes instead of g_mbus_config.
---
 drivers/staging/media/imx/imx6-mipi-csi2.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
index bfa4b254c4e4..aa4bf2f89695 100644
--- a/drivers/staging/media/imx/imx6-mipi-csi2.c
+++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
@@ -131,10 +131,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, int lanes)
 {
-	int lanes = csi2->bus.num_data_lanes;
-
 	writel(lanes - 1, csi2->base + CSI2_N_LANES);
 }
 
@@ -295,6 +293,7 @@ static void csi2ipu_gasket_init(struct csi2_dev *csi2)
 
 static int csi2_start(struct csi2_dev *csi2)
 {
+	u32 lanes = 0;
 	int ret;
 
 	ret = clk_prepare_enable(csi2->pix_clk);
@@ -310,7 +309,8 @@ static int csi2_start(struct csi2_dev *csi2)
 		goto err_disable_clk;
 
 	/* Step 4 */
-	csi2_set_lanes(csi2);
+	v4l2_subdev_call(csi2->src_sd, video, g_csi_active_lanes, &lanes);
+	csi2_set_lanes(csi2, lanes ?: csi2->bus.num_data_lanes);
 	csi2_enable(csi2, true);
 
 	/* Step 5 */
-- 
2.20.1