[PATCH 0/4] v4l2-subdev: Introduce get_mbus_format pad op

[PATCH 0/4] v4l2-subdev: Introduce get_mbus_format pad op

[Jacopo Mondi]

This series introduces a pad oriented operation, much similar to the existing
s/g_mbus_config subdv video operation, to allow dyanmic negotiation of media
bus parameter.

The existing s/g_mbus_format are on their way to deprecation, due to the fact
they operate at device level being a video op instead of pad level as this new
implementation does.

The use case I'm addressing is described here, in the RFC sent one year ago
on top of Sakari's v4l-multiplexed work, where I tried to extend the frame
descriptor to transport media bus information.

Quoting:
https://patchwork.kernel.org/cover/10855919/
"The use case this series cover is the following one:
the Gen-3 R-Car boards include an ADV748x HDMI/CVBS to CSI-2 converter
connected to its CSI-2 receivers. The ADV748x chip has recently gained support
for routing both HDMI and analogue video streams through its 4 lanes TXA
transmitter, specifically to support the Ebisu board that has a single CSI-2
receiver, compared to all other Gen-3 board where the ADV748x TXes are connected
to different CSI-2 receivers, and where analogue video is streamed out from the
ADV748x single lane TXB transmitter.
To properly support transmission of analogue video through TXA, the number of
data lanes shall be dynamically reduced to 1, in order to comply with the MIPI
CSI-2 minimum clock frequency requirements"

During the discussion of the RFC, Dave reported another use case for media
bus parameter negotiation on his platform:
https://patchwork.kernel.org/patch/10855923/#22569149

Another possible use case is for parallel bus multiplexing, where multiple image
sensor share the parallel bus lines and they get activated alternatively through
an enable signal. While this might not be most clever design, it's often seen
in the wild, and this operation allow receivers to re-configure their bus
parameter in between streaming session.

For now I have left untouched definitions and users of the existing
s/g_mbus_config ops, waiting for feedback on this first implementation.

Thanks
   j

Jacopo Mondi (4):
  media: i2c: adv748x: Adjust TXA data lanes number
  media: v4l2-subdv: Introduce get_mbus_config pad op
  media: i2c: adv748x: Implement get_mbus_config
  media: rcar-vin: csi2: Negotiate data lanes number

 drivers/media/i2c/adv748x/adv748x-core.c    | 31 +++++++---
 drivers/media/i2c/adv748x/adv748x-csi2.c    | 15 +++++
 drivers/media/i2c/adv748x/adv748x.h         |  1 +
 drivers/media/platform/rcar-vin/rcar-csi2.c | 49 ++++++++++++++-
 include/media/v4l2-subdev.h                 | 67 +++++++++++++++++++++
 5 files changed, 153 insertions(+), 10 deletions(-)

--
2.25.1

[PATCH 1/4] media: i2c: adv748x: Adjust TXA data lanes number

[Jacopo Mondi]

When outputting SD-Core output through the TXA MIPI CSI-2 interface,
the number of enabled data lanes should be reduced in order to guarantee
the two video format produced by the SD-Core (480i and 576i) generate a
MIPI CSI-2 link clock frequency compatible with the MIPI D-PHY
specifications.

Limit the number of enabled data lanes to 2, which is guaranteed to
support 480i and 576i formats.

Cache the number of enabled data lanes to be able to report it through
the new get_mbus_config operation introduced in the following patches.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 31 ++++++++++++++++++------
 drivers/media/i2c/adv748x/adv748x.h      |  1 +
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 23e02ff27b17..1fe7f97c6d52 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -241,10 +241,10 @@ static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
 	int ret = 0;
 
 	/* Enable n-lane MIPI */
-	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
+	adv748x_write_check(state, page, 0x00, 0x80 | tx->active_lanes, &ret);
 
 	/* Set Auto DPHY Timing */
-	adv748x_write_check(state, page, 0x00, 0xa0 | tx->num_lanes, &ret);
+	adv748x_write_check(state, page, 0x00, 0xa0 | tx->active_lanes, &ret);
 
 	/* ADI Required Write */
 	if (tx->src == &state->hdmi.sd) {
@@ -270,7 +270,7 @@ static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
 	usleep_range(2000, 2500);
 
 	/* Power-up CSI-TX */
-	adv748x_write_check(state, page, 0x00, 0x20 | tx->num_lanes, &ret);
+	adv748x_write_check(state, page, 0x00, 0x20 | tx->active_lanes, &ret);
 	usleep_range(1000, 1500);
 
 	/* ADI Required Writes */
@@ -292,7 +292,7 @@ static int adv748x_power_down_tx(struct adv748x_csi2 *tx)
 	adv748x_write_check(state, page, 0x1e, 0x00, &ret);
 
 	/* Enable n-lane MIPI */
-	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
+	adv748x_write_check(state, page, 0x00, 0x80 | tx->active_lanes, &ret);
 
 	/* i2c_mipi_pll_en - 1'b1 */
 	adv748x_write_check(state, page, 0xda, 0x01, &ret);
@@ -357,14 +357,29 @@ static int adv748x_link_setup(struct media_entity *entity,
 	if (state->afe.tx) {
 		/* AFE Requires TXA enabled, even when output to TXB */
 		io10 |= ADV748X_IO_10_CSI4_EN;
-		if (is_txa(tx))
+		if (is_txa(tx)) {
+			/*
+			 * Output from the SD-core (480i and 576i) from the TXA
+			 * interface requires reducing the number of enabled
+			 * data lanes in order to guarantee a valid link
+			 * frequency.
+			 */
+			tx->active_lanes = min(tx->num_lanes, 2U);
 			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
-		else
+		} else {
+			/* TXB has a single data lane, no need to adjust. */
 			io10 |= ADV748X_IO_10_CSI1_EN;
+		}
 	}
 
-	if (state->hdmi.tx)
+	if (state->hdmi.tx) {
+		/*
+		 * Restore the number of active lanes, in case we have gone
+		 * through an AFE->TXA streaming sessions.
+		 */
+		tx->active_lanes = tx->num_lanes;
 		io10 |= ADV748X_IO_10_CSI4_EN;
+	}
 
 	return io_clrset(state, ADV748X_IO_10, io10_mask, io10);
 }
@@ -596,6 +611,7 @@ static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
 		}
 
 		state->txa.num_lanes = num_lanes;
+		state->txa.active_lanes = num_lanes;
 		adv_dbg(state, "TXA: using %u lanes\n", state->txa.num_lanes);
 	}
 
@@ -607,6 +623,7 @@ static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
 		}
 
 		state->txb.num_lanes = num_lanes;
+		state->txb.active_lanes = num_lanes;
 		adv_dbg(state, "TXB: using %u lanes\n", state->txb.num_lanes);
 	}
 
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index fccb388ce179..1061f425ece5 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -79,6 +79,7 @@ struct adv748x_csi2 {
 	unsigned int page;
 	unsigned int port;
 	unsigned int num_lanes;
+	unsigned int active_lanes;
 
 	struct media_pad pads[ADV748X_CSI2_NR_PADS];
 	struct v4l2_ctrl_handler ctrl_hdl;
-- 
2.25.1

[PATCH 2/4] media: v4l2-subdv: Introduce get_mbus_config pad op

[Jacopo Mondi]

Introduce a new pad operation to allow retrieving the media bus
configuration on a subdevice pad.

The newly introduced operation reassembles the s/g_mbus_config video
operation, which have been on their way to be deprecated since a long
time.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 include/media/v4l2-subdev.h | 67 +++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 761aa83a3f3c..3a1afc00e094 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -350,6 +350,70 @@ struct v4l2_mbus_frame_desc {
 	unsigned short num_entries;
 };
 
+/**
+ * struct v4l2_mbus_parallel_config - parallel mbus configuration
+ * @hsync_active: hsync active state: true for high, false for low
+ * @vsync_active: vsync active state: true for high, false for low
+ * @pclk_rising: pixel clock active edge: true for rising, false for falling
+ * @data_active: data lines active state: true for high, false for low
+ */
+struct v4l2_mbus_parallel_config {
+	bool hsync_active : 1;
+	bool vsync_active : 1;
+	bool pclk_rising : 1;
+	bool data_active : 1;
+};
+
+/**
+ * struct v4l2_mbus_csi2_dphy_config - MIPI CSI-2 DPHY mbus configuration
+ * @data_lanes: number of data lanes in use
+ * @clock_noncontinuous: non continuous clock enable flag
+ */
+struct v4l2_mbus_csi2_dphy_config {
+	unsigned int data_lanes : 3;
+	bool clock_noncontinuous : 1;
+};
+
+/**
+ * struct v4l2_mbus_csi2_cphy_config - MIPI CSI-2 CPHY mbus configuration
+ *
+ * TODO
+ */
+struct v4l2_mbus_csi2_cphy_config {
+	/* TODO */
+};
+
+/**
+ * struct v4l2_mbus_pad_config - media bus configuration
+ *
+ * Report the subdevice media bus information to inform the caller of the
+ * current bus configuration. The structure describes bus configuration
+ * parameters that might change in-between streaming sessions, in order to allow
+ * the caller to adjust its media bus configuration to match what is reported
+ * here.
+ *
+ * TODO: add '_pad_' to the name to distinguish this from the structure
+ * defined in v4l2_mediabus.h used for the same purpose by the g/s_mbus_config
+ * video operations. Reuse the there defined enum v4l2_mbus_type to define
+ * the bus type.
+ *
+ * @type: mbus type. See &enum v4l2_mbus_type
+ * @parallel: parallel bus configuration parameters.
+ *	      See &struct v4l2_mbus_parallel_config
+ * @csi2_dphy: MIPI CSI-2 DPHY configuration parameters
+ *	       See &struct v4l2_mbus_csi2_dphy_config
+ * @csi2_cphy: MIPI CSI-2 CPHY configuration parameters
+ *	       See &struct v4l2_mbus_csi2_cphy_config
+ */
+struct v4l2_mbus_pad_config {
+	enum v4l2_mbus_type type;
+	union {
+		struct v4l2_mbus_parallel_config parallel;
+		struct v4l2_mbus_csi2_dphy_config csi2_dphy;
+		struct v4l2_mbus_csi2_cphy_config csi2_cphy;
+	};
+};
+
 /**
  * struct v4l2_subdev_video_ops - Callbacks used when v4l device was opened
  *				  in video mode.
@@ -670,6 +734,7 @@ struct v4l2_subdev_pad_config {
  *
  * @set_frame_desc: set the low level media bus frame parameters, @fd array
  *                  may be adjusted by the subdev driver to device capabilities.
+ * @get_mbus_config: get the current mbus configuration
  */
 struct v4l2_subdev_pad_ops {
 	int (*init_cfg)(struct v4l2_subdev *sd,
@@ -710,6 +775,8 @@ struct v4l2_subdev_pad_ops {
 			      struct v4l2_mbus_frame_desc *fd);
 	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
 			      struct v4l2_mbus_frame_desc *fd);
+	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
+			       struct v4l2_mbus_pad_config *config);
 };
 
 /**
-- 
2.25.1

[PATCH 3/4] media: i2c: adv748x: Implement get_mbus_config

[Jacopo Mondi]

Implement the newly introduced get_mbus_config operation to report the
number of currently used data lanes on the MIPI CSI-2 interface.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-csi2.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 2091cda50935..f13563da3ff3 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -214,9 +214,24 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
 	return ret;
 }
 
+static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
+					struct v4l2_mbus_pad_config *config)
+{
+	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
+
+	if (pad != ADV748X_CSI2_SOURCE)
+		return -EINVAL;
+
+	config->type = V4L2_MBUS_CSI2_DPHY;
+	config->csi2_dphy.data_lanes = tx->active_lanes;
+
+	return 0;
+}
+
 static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
 	.get_fmt = adv748x_csi2_get_format,
 	.set_fmt = adv748x_csi2_set_format,
+	.get_mbus_config = adv748x_csi2_get_mbus_config,
 };
 
 /* -----------------------------------------------------------------------------
-- 
2.25.1

[PATCH 4/4] media: rcar-vin: csi2: Negotiate data lanes number

[Jacopo Mondi]

Use the newly introduced get_mbus_config() subdevice pad operation to
retrieve the remote subdevice MIPI CSI-2 bus configuration and configure
the number of active data lanes accordingly.

In order to be able to call the remote subdevice operation cache the
index of the remote pad connected to the single CSI-2 input port.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 49 +++++++++++++++++++--
 1 file changed, 46 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index faa9fb23a2e9..4145e028dcdf 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -363,6 +363,7 @@ struct rcar_csi2 {
 	struct v4l2_async_notifier notifier;
 	struct v4l2_async_subdev asd;
 	struct v4l2_subdev *remote;
+	unsigned short remote_pad;
 
 	struct v4l2_mbus_framefmt mf;
 
@@ -371,6 +372,7 @@ struct rcar_csi2 {
 
 	unsigned short lanes;
 	unsigned char lane_swap[4];
+	unsigned short active_lanes;
 };
 
 static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
@@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
 
 	/* Wait for the clock and data lanes to enter LP-11 state. */
 	for (timeout = 0; timeout <= 20; timeout++) {
-		const u32 lane_mask = (1 << priv->lanes) - 1;
+		const u32 lane_mask = (1 << priv->active_lanes) - 1;
 
 		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
 		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
@@ -471,11 +473,45 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
 	 * bps = link_freq * 2
 	 */
 	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
-	do_div(mbps, priv->lanes * 1000000);
+	do_div(mbps, priv->active_lanes * 1000000);
 
 	return mbps;
 }
 
+static int rcsi2_get_mbus_config(struct rcar_csi2 *priv)
+{
+	struct v4l2_mbus_pad_config mbus_config;
+	int ret;
+
+	memset(&mbus_config, 0, sizeof(mbus_config));
+	ret = v4l2_subdev_call(priv->remote, pad, get_mbus_config,
+			       priv->remote_pad, &mbus_config);
+	if (ret && ret != -ENOIOCTLCMD) {
+		dev_err(priv->dev, "Failed to get remote mbus configuration\n");
+		return ret;
+	} else if (ret == -ENOIOCTLCMD) {
+		dev_dbg(priv->dev, "No remote mbus configuration available\n");
+		priv->active_lanes = priv->lanes;
+		return 0;
+	}
+
+	if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
+		dev_err(priv->dev,
+			"Unsupported mbus type %u\n", mbus_config.type);
+		return -EINVAL;
+	}
+
+	if (mbus_config.csi2_dphy.data_lanes > priv->lanes) {
+		dev_err(priv->dev,
+			"Unsupported mbus config: too many data lanes %u\n",
+			mbus_config.csi2_dphy.data_lanes);
+		return -EINVAL;
+	}
+	priv->active_lanes = mbus_config.csi2_dphy.data_lanes;
+
+	return 0;
+}
+
 static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 {
 	const struct rcar_csi2_format *format;
@@ -490,6 +526,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 	/* Code is validated in set_fmt. */
 	format = rcsi2_code_to_fmt(priv->mf.code);
 
+	/* Get the remote mbus config to get the number of enabled lanes. */
+	ret = rcsi2_get_mbus_config(priv);
+	if (ret)
+		return ret;
+
 	/*
 	 * Enable all supported CSI-2 channels with virtual channel and
 	 * data type matching.
@@ -522,7 +563,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 	}
 
 	phycnt = PHYCNT_ENABLECLK;
-	phycnt |= (1 << priv->lanes) - 1;
+	phycnt |= (1 << priv->active_lanes) - 1;
 
 	mbps = rcsi2_calc_mbps(priv, format->bpp);
 	if (mbps < 0)
@@ -748,6 +789,7 @@ static int rcsi2_notify_bound(struct v4l2_async_notifier *notifier,
 	}
 
 	priv->remote = subdev;
+	priv->remote_pad = pad;
 
 	dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad);
 
@@ -793,6 +835,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
 			priv->lanes);
 		return -EINVAL;
 	}
+	priv->active_lanes = priv->lanes;
 
 	for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) {
 		priv->lane_swap[i] = i < priv->lanes ?
-- 
2.25.1

Re: [PATCH 0/4] v4l2-subdev: Introduce get_mbus_format pad op

[Hyun Kwon]

Hi Jacopo,

Thanks for the patch.

On Fri, 2020-03-13 at 07:40:31 -0700, Jacopo Mondi wrote:
> This series introduces a pad oriented operation, much similar to the existing
> s/g_mbus_config subdv video operation, to allow dyanmic negotiation of media
> bus parameter.
> 
> The existing s/g_mbus_format are on their way to deprecation, due to the fact
> they operate at device level being a video op instead of pad level as this new
> implementation does.
> 
> The use case I'm addressing is described here, in the RFC sent one year ago
> on top of Sakari's v4l-multiplexed work, where I tried to extend the frame
> descriptor to transport media bus information.
> 
> Quoting:
> https://patchwork.kernel.org/cover/10855919/
> "The use case this series cover is the following one:
> the Gen-3 R-Car boards include an ADV748x HDMI/CVBS to CSI-2 converter
> connected to its CSI-2 receivers. The ADV748x chip has recently gained support
> for routing both HDMI and analogue video streams through its 4 lanes TXA
> transmitter, specifically to support the Ebisu board that has a single CSI-2
> receiver, compared to all other Gen-3 board where the ADV748x TXes are connected
> to different CSI-2 receivers, and where analogue video is streamed out from the
> ADV748x single lane TXB transmitter.
> To properly support transmission of analogue video through TXA, the number of
> data lanes shall be dynamically reduced to 1, in order to comply with the MIPI
> CSI-2 minimum clock frequency requirements"
> 
> During the discussion of the RFC, Dave reported another use case for media
> bus parameter negotiation on his platform:
> https://patchwork.kernel.org/patch/10855923/#22569149

And as you know, there's another use case for GMSL, and this patch set helps
negotiation between serializer and de-serializer. :-) I added the GMSL
config for some parameters on top of this, and it works well.

> 
> Another possible use case is for parallel bus multiplexing, where multiple image
> sensor share the parallel bus lines and they get activated alternatively through
> an enable signal. While this might not be most clever design, it's often seen
> in the wild, and this operation allow receivers to re-configure their bus
> parameter in between streaming session.

Even for parallel bus, I'm learning how diverse configurations can be.
For example, some ISP aligns bits in different way, MSB of color component
on d0. I'm sure there are a lot more creative ones out there, so this seems
quite useful to handle such accordingly.

Thanks,
-hyun

> 
> For now I have left untouched definitions and users of the existing
> s/g_mbus_config ops, waiting for feedback on this first implementation.
> 
> Thanks
>    j
> 
> Jacopo Mondi (4):
>   media: i2c: adv748x: Adjust TXA data lanes number
>   media: v4l2-subdv: Introduce get_mbus_config pad op
>   media: i2c: adv748x: Implement get_mbus_config
>   media: rcar-vin: csi2: Negotiate data lanes number
> 
>  drivers/media/i2c/adv748x/adv748x-core.c    | 31 +++++++---
>  drivers/media/i2c/adv748x/adv748x-csi2.c    | 15 +++++
>  drivers/media/i2c/adv748x/adv748x.h         |  1 +
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 49 ++++++++++++++-
>  include/media/v4l2-subdev.h                 | 67 +++++++++++++++++++++
>  5 files changed, 153 insertions(+), 10 deletions(-)
> 
> --
> 2.25.1
> 

Re: [PATCH 2/4] media: v4l2-subdv: Introduce get_mbus_config pad op

[Hyun Kwon]

Hi Jacopo,

Thanks for the patch.

On Fri, 2020-03-13 at 07:40:33 -0700, Jacopo Mondi wrote:
> Introduce a new pad operation to allow retrieving the media bus
> configuration on a subdevice pad.
> 
> The newly introduced operation reassembles the s/g_mbus_config video
> operation, which have been on their way to be deprecated since a long
> time.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  include/media/v4l2-subdev.h | 67 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 761aa83a3f3c..3a1afc00e094 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -350,6 +350,70 @@ struct v4l2_mbus_frame_desc {
>  	unsigned short num_entries;
>  };
>  
> +/**
> + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> + * @hsync_active: hsync active state: true for high, false for low
> + * @vsync_active: vsync active state: true for high, false for low
> + * @pclk_rising: pixel clock active edge: true for rising, false for falling
> + * @data_active: data lines active state: true for high, false for low
> + */
> +struct v4l2_mbus_parallel_config {
> +	bool hsync_active : 1;
> +	bool vsync_active : 1;
> +	bool pclk_rising : 1;
> +	bool data_active : 1;
> +};
> +
> +/**
> + * struct v4l2_mbus_csi2_dphy_config - MIPI CSI-2 DPHY mbus configuration
> + * @data_lanes: number of data lanes in use
> + * @clock_noncontinuous: non continuous clock enable flag
> + */
> +struct v4l2_mbus_csi2_dphy_config {
> +	unsigned int data_lanes : 3;
> +	bool clock_noncontinuous : 1;
> +};
> +
> +/**
> + * struct v4l2_mbus_csi2_cphy_config - MIPI CSI-2 CPHY mbus configuration
> + *
> + * TODO
> + */
> +struct v4l2_mbus_csi2_cphy_config {
> +	/* TODO */
> +};
> +
> +/**
> + * struct v4l2_mbus_pad_config - media bus configuration
> + *
> + * Report the subdevice media bus information to inform the caller of the
> + * current bus configuration. The structure describes bus configuration
> + * parameters that might change in-between streaming sessions, in order to allow
> + * the caller to adjust its media bus configuration to match what is reported
> + * here.
> + *
> + * TODO: add '_pad_' to the name to distinguish this from the structure
> + * defined in v4l2_mediabus.h used for the same purpose by the g/s_mbus_config
> + * video operations. Reuse the there defined enum v4l2_mbus_type to define
> + * the bus type.
> + *
> + * @type: mbus type. See &enum v4l2_mbus_type
> + * @parallel: parallel bus configuration parameters.
> + *	      See &struct v4l2_mbus_parallel_config
> + * @csi2_dphy: MIPI CSI-2 DPHY configuration parameters
> + *	       See &struct v4l2_mbus_csi2_dphy_config
> + * @csi2_cphy: MIPI CSI-2 CPHY configuration parameters
> + *	       See &struct v4l2_mbus_csi2_cphy_config
> + */
> +struct v4l2_mbus_pad_config {
> +	enum v4l2_mbus_type type;
> +	union {
> +		struct v4l2_mbus_parallel_config parallel;
> +		struct v4l2_mbus_csi2_dphy_config csi2_dphy;
> +		struct v4l2_mbus_csi2_cphy_config csi2_cphy;
> +	};
> +};
> +
>  /**
>   * struct v4l2_subdev_video_ops - Callbacks used when v4l device was opened
>   *				  in video mode.
> @@ -670,6 +734,7 @@ struct v4l2_subdev_pad_config {
>   *
>   * @set_frame_desc: set the low level media bus frame parameters, @fd array
>   *                  may be adjusted by the subdev driver to device capabilities.
> + * @get_mbus_config: get the current mbus configuration
>   */
>  struct v4l2_subdev_pad_ops {
>  	int (*init_cfg)(struct v4l2_subdev *sd,
> @@ -710,6 +775,8 @@ struct v4l2_subdev_pad_ops {
>  			      struct v4l2_mbus_frame_desc *fd);
>  	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
>  			      struct v4l2_mbus_frame_desc *fd);
> +	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> +			       struct v4l2_mbus_pad_config *config);

Because this can be used in many different ways, there's more chance it can
be misused. That means, drivers call this in different locations, ex probe,
get format, start stream,,,, and on differnt pads, src or sink. So imagine
one set of drivers call on sink pad, and the other set call on source pad.
It works well only until those are mixed together.

So wouldn't it be better to put some restrictions? One is to document
recommendations. I think this better be called in stream on because
some bus config may change at runtime depending on other configuration.
So any bus config prior to stream-on may be outdated. The other is to
enforce in the code. Some, but maybe not all, can be handled in
v4l2_subdev_call_pad_wrappers, for example allowing this call only on
source pad.

Thanks,
-hyun

>  };
>  
>  /**
> -- 
> 2.25.1
> 

Re: [PATCH 1/4] media: i2c: adv748x: Adjust TXA data lanes number

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2020-03-13 15:40:32 +0100, Jacopo Mondi wrote:
> When outputting SD-Core output through the TXA MIPI CSI-2 interface,
> the number of enabled data lanes should be reduced in order to guarantee
> the two video format produced by the SD-Core (480i and 576i) generate a
> MIPI CSI-2 link clock frequency compatible with the MIPI D-PHY
> specifications.
> 
> Limit the number of enabled data lanes to 2, which is guaranteed to
> support 480i and 576i formats.
> 
> Cache the number of enabled data lanes to be able to report it through
> the new get_mbus_config operation introduced in the following patches.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Small nit, I would switch 1/4 and 2/4 in this series ;-)

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 31 ++++++++++++++++++------
>  drivers/media/i2c/adv748x/adv748x.h      |  1 +
>  2 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 23e02ff27b17..1fe7f97c6d52 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -241,10 +241,10 @@ static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
>  	int ret = 0;
>  
>  	/* Enable n-lane MIPI */
> -	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
> +	adv748x_write_check(state, page, 0x00, 0x80 | tx->active_lanes, &ret);
>  
>  	/* Set Auto DPHY Timing */
> -	adv748x_write_check(state, page, 0x00, 0xa0 | tx->num_lanes, &ret);
> +	adv748x_write_check(state, page, 0x00, 0xa0 | tx->active_lanes, &ret);
>  
>  	/* ADI Required Write */
>  	if (tx->src == &state->hdmi.sd) {
> @@ -270,7 +270,7 @@ static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
>  	usleep_range(2000, 2500);
>  
>  	/* Power-up CSI-TX */
> -	adv748x_write_check(state, page, 0x00, 0x20 | tx->num_lanes, &ret);
> +	adv748x_write_check(state, page, 0x00, 0x20 | tx->active_lanes, &ret);
>  	usleep_range(1000, 1500);
>  
>  	/* ADI Required Writes */
> @@ -292,7 +292,7 @@ static int adv748x_power_down_tx(struct adv748x_csi2 *tx)
>  	adv748x_write_check(state, page, 0x1e, 0x00, &ret);
>  
>  	/* Enable n-lane MIPI */
> -	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
> +	adv748x_write_check(state, page, 0x00, 0x80 | tx->active_lanes, &ret);
>  
>  	/* i2c_mipi_pll_en - 1'b1 */
>  	adv748x_write_check(state, page, 0xda, 0x01, &ret);
> @@ -357,14 +357,29 @@ static int adv748x_link_setup(struct media_entity *entity,
>  	if (state->afe.tx) {
>  		/* AFE Requires TXA enabled, even when output to TXB */
>  		io10 |= ADV748X_IO_10_CSI4_EN;
> -		if (is_txa(tx))
> +		if (is_txa(tx)) {
> +			/*
> +			 * Output from the SD-core (480i and 576i) from the TXA
> +			 * interface requires reducing the number of enabled
> +			 * data lanes in order to guarantee a valid link
> +			 * frequency.
> +			 */
> +			tx->active_lanes = min(tx->num_lanes, 2U);
>  			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> -		else
> +		} else {
> +			/* TXB has a single data lane, no need to adjust. */
>  			io10 |= ADV748X_IO_10_CSI1_EN;
> +		}
>  	}
>  
> -	if (state->hdmi.tx)
> +	if (state->hdmi.tx) {
> +		/*
> +		 * Restore the number of active lanes, in case we have gone
> +		 * through an AFE->TXA streaming sessions.
> +		 */
> +		tx->active_lanes = tx->num_lanes;
>  		io10 |= ADV748X_IO_10_CSI4_EN;
> +	}
>  
>  	return io_clrset(state, ADV748X_IO_10, io10_mask, io10);
>  }
> @@ -596,6 +611,7 @@ static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
>  		}
>  
>  		state->txa.num_lanes = num_lanes;
> +		state->txa.active_lanes = num_lanes;
>  		adv_dbg(state, "TXA: using %u lanes\n", state->txa.num_lanes);
>  	}
>  
> @@ -607,6 +623,7 @@ static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
>  		}
>  
>  		state->txb.num_lanes = num_lanes;
> +		state->txb.active_lanes = num_lanes;
>  		adv_dbg(state, "TXB: using %u lanes\n", state->txb.num_lanes);
>  	}
>  
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index fccb388ce179..1061f425ece5 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -79,6 +79,7 @@ struct adv748x_csi2 {
>  	unsigned int page;
>  	unsigned int port;
>  	unsigned int num_lanes;
> +	unsigned int active_lanes;
>  
>  	struct media_pad pads[ADV748X_CSI2_NR_PADS];
>  	struct v4l2_ctrl_handler ctrl_hdl;
> -- 
> 2.25.1
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 2/4] media: v4l2-subdv: Introduce get_mbus_config pad op

[niklas.soderlund+renesas]

Hi Hyun and Jacopo,

On 2020-04-01 15:30:38 -0700, Hyun Kwon wrote:
> Hi Jacopo,
> 
> Thanks for the patch.
> 
> On Fri, 2020-03-13 at 07:40:33 -0700, Jacopo Mondi wrote:
> > Introduce a new pad operation to allow retrieving the media bus
> > configuration on a subdevice pad.
> > 
> > The newly introduced operation reassembles the s/g_mbus_config video
> > operation, which have been on their way to be deprecated since a long
> > time.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  include/media/v4l2-subdev.h | 67 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> > 
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 761aa83a3f3c..3a1afc00e094 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -350,6 +350,70 @@ struct v4l2_mbus_frame_desc {
> >  	unsigned short num_entries;
> >  };
> >  
> > +/**
> > + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> > + * @hsync_active: hsync active state: true for high, false for low
> > + * @vsync_active: vsync active state: true for high, false for low
> > + * @pclk_rising: pixel clock active edge: true for rising, false for falling
> > + * @data_active: data lines active state: true for high, false for low
> > + */
> > +struct v4l2_mbus_parallel_config {
> > +	bool hsync_active : 1;
> > +	bool vsync_active : 1;
> > +	bool pclk_rising : 1;
> > +	bool data_active : 1;
> > +};
> > +
> > +/**
> > + * struct v4l2_mbus_csi2_dphy_config - MIPI CSI-2 DPHY mbus configuration
> > + * @data_lanes: number of data lanes in use
> > + * @clock_noncontinuous: non continuous clock enable flag
> > + */
> > +struct v4l2_mbus_csi2_dphy_config {
> > +	unsigned int data_lanes : 3;
> > +	bool clock_noncontinuous : 1;
> > +};
> > +
> > +/**
> > + * struct v4l2_mbus_csi2_cphy_config - MIPI CSI-2 CPHY mbus configuration
> > + *
> > + * TODO
> > + */
> > +struct v4l2_mbus_csi2_cphy_config {
> > +	/* TODO */
> > +};
> > +
> > +/**
> > + * struct v4l2_mbus_pad_config - media bus configuration
> > + *
> > + * Report the subdevice media bus information to inform the caller of the
> > + * current bus configuration. The structure describes bus configuration
> > + * parameters that might change in-between streaming sessions, in order to allow
> > + * the caller to adjust its media bus configuration to match what is reported
> > + * here.
> > + *
> > + * TODO: add '_pad_' to the name to distinguish this from the structure
> > + * defined in v4l2_mediabus.h used for the same purpose by the g/s_mbus_config
> > + * video operations. Reuse the there defined enum v4l2_mbus_type to define
> > + * the bus type.
> > + *
> > + * @type: mbus type. See &enum v4l2_mbus_type
> > + * @parallel: parallel bus configuration parameters.
> > + *	      See &struct v4l2_mbus_parallel_config
> > + * @csi2_dphy: MIPI CSI-2 DPHY configuration parameters
> > + *	       See &struct v4l2_mbus_csi2_dphy_config
> > + * @csi2_cphy: MIPI CSI-2 CPHY configuration parameters
> > + *	       See &struct v4l2_mbus_csi2_cphy_config
> > + */
> > +struct v4l2_mbus_pad_config {
> > +	enum v4l2_mbus_type type;
> > +	union {
> > +		struct v4l2_mbus_parallel_config parallel;
> > +		struct v4l2_mbus_csi2_dphy_config csi2_dphy;
> > +		struct v4l2_mbus_csi2_cphy_config csi2_cphy;
> > +	};
> > +};
> > +
> >  /**
> >   * struct v4l2_subdev_video_ops - Callbacks used when v4l device was opened
> >   *				  in video mode.
> > @@ -670,6 +734,7 @@ struct v4l2_subdev_pad_config {
> >   *
> >   * @set_frame_desc: set the low level media bus frame parameters, @fd array
> >   *                  may be adjusted by the subdev driver to device capabilities.
> > + * @get_mbus_config: get the current mbus configuration
> >   */
> >  struct v4l2_subdev_pad_ops {
> >  	int (*init_cfg)(struct v4l2_subdev *sd,
> > @@ -710,6 +775,8 @@ struct v4l2_subdev_pad_ops {
> >  			      struct v4l2_mbus_frame_desc *fd);
> >  	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
> >  			      struct v4l2_mbus_frame_desc *fd);
> > +	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > +			       struct v4l2_mbus_pad_config *config);
> 
> Because this can be used in many different ways, there's more chance it can
> be misused. That means, drivers call this in different locations, ex probe,
> get format, start stream,,,, and on differnt pads, src or sink. So imagine
> one set of drivers call on sink pad, and the other set call on source pad.
> It works well only until those are mixed together.

That subdevice operations can be called at both probe and s_stream() is 
nothing new, I don't thin this is a new problem. But I agree maybe we 
could limit get_mbus_config() in the core to only be valid four source 
pads? Apart from this open question I think this patch looks good.

> 
> So wouldn't it be better to put some restrictions? One is to document
> recommendations. I think this better be called in stream on because
> some bus config may change at runtime depending on other configuration.
> So any bus config prior to stream-on may be outdated. The other is to
> enforce in the code. Some, but maybe not all, can be handled in
> v4l2_subdev_call_pad_wrappers, for example allowing this call only on
> source pad.
> 
> Thanks,
> -hyun
> 
> >  };
> >  
> >  /**
> > -- 
> > 2.25.1
> > 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 3/4] media: i2c: adv748x: Implement get_mbus_config

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2020-03-13 15:40:34 +0100, Jacopo Mondi wrote:
> Implement the newly introduced get_mbus_config operation to report the
> number of currently used data lanes on the MIPI CSI-2 interface.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 2091cda50935..f13563da3ff3 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -214,9 +214,24 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
>  	return ret;
>  }
>  
> +static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
> +					struct v4l2_mbus_pad_config *config)
> +{
> +	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> +
> +	if (pad != ADV748X_CSI2_SOURCE)
> +		return -EINVAL;
> +
> +	config->type = V4L2_MBUS_CSI2_DPHY;
> +	config->csi2_dphy.data_lanes = tx->active_lanes;
> +
> +	return 0;
> +}
> +
>  static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
>  	.get_fmt = adv748x_csi2_get_format,
>  	.set_fmt = adv748x_csi2_set_format,
> +	.get_mbus_config = adv748x_csi2_get_mbus_config,
>  };
>  
>  /* -----------------------------------------------------------------------------
> -- 
> 2.25.1
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 4/4] media: rcar-vin: csi2: Negotiate data lanes number

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2020-03-13 15:40:35 +0100, Jacopo Mondi wrote:
> Use the newly introduced get_mbus_config() subdevice pad operation to
> retrieve the remote subdevice MIPI CSI-2 bus configuration and configure
> the number of active data lanes accordingly.
> 
> In order to be able to call the remote subdevice operation cache the
> index of the remote pad connected to the single CSI-2 input port.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Please s/rcar-vin: csi2:/rcar-csi2:/ in the subject.

> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 49 +++++++++++++++++++--
>  1 file changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index faa9fb23a2e9..4145e028dcdf 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -363,6 +363,7 @@ struct rcar_csi2 {
>  	struct v4l2_async_notifier notifier;
>  	struct v4l2_async_subdev asd;
>  	struct v4l2_subdev *remote;
> +	unsigned short remote_pad;
>  
>  	struct v4l2_mbus_framefmt mf;
>  
> @@ -371,6 +372,7 @@ struct rcar_csi2 {
>  
>  	unsigned short lanes;
>  	unsigned char lane_swap[4];
> +	unsigned short active_lanes;
>  };
>  
>  static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
> @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
>  
>  	/* Wait for the clock and data lanes to enter LP-11 state. */
>  	for (timeout = 0; timeout <= 20; timeout++) {
> -		const u32 lane_mask = (1 << priv->lanes) - 1;
> +		const u32 lane_mask = (1 << priv->active_lanes) - 1;
>  
>  		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
>  		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
> @@ -471,11 +473,45 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
>  	 * bps = link_freq * 2
>  	 */
>  	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
> -	do_div(mbps, priv->lanes * 1000000);
> +	do_div(mbps, priv->active_lanes * 1000000);
>  
>  	return mbps;
>  }
>  
> +static int rcsi2_get_mbus_config(struct rcar_csi2 *priv)

This function do not get the mbus configuration as much as update number 
of used lanes. How about renaming it rcsi2_update_lanes_used() or 
something similar ?

> +{
> +	struct v4l2_mbus_pad_config mbus_config;
> +	int ret;
> +
> +	memset(&mbus_config, 0, sizeof(mbus_config));
> +	ret = v4l2_subdev_call(priv->remote, pad, get_mbus_config,
> +			       priv->remote_pad, &mbus_config);
> +	if (ret && ret != -ENOIOCTLCMD) {
> +		dev_err(priv->dev, "Failed to get remote mbus configuration\n");
> +		return ret;
> +	} else if (ret == -ENOIOCTLCMD) {
> +		dev_dbg(priv->dev, "No remote mbus configuration available\n");
> +		priv->active_lanes = priv->lanes;
> +		return 0;
> +	}

How about something bellow to match style of driver?

    priv->active_lanes = priv->lanes;

    memset(&mbus_config, 0, sizeof(mbus_config));
    ret = v4l2_subdev_call(... get_mbus_config ...)
    if (ret && ret != -ENOIOCTLCMD) {
        dev_err(priv->dev, "Failed to get remote mbus configuration\n");
        return ret;
    }

> +
> +	if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
> +		dev_err(priv->dev,
> +			"Unsupported mbus type %u\n", mbus_config.type);
> +		return -EINVAL;
> +	}
> +
> +	if (mbus_config.csi2_dphy.data_lanes > priv->lanes) {
> +		dev_err(priv->dev,
> +			"Unsupported mbus config: too many data lanes %u\n",
> +			mbus_config.csi2_dphy.data_lanes);
> +		return -EINVAL;
> +	}
> +	priv->active_lanes = mbus_config.csi2_dphy.data_lanes;
> +
> +	return 0;
> +}
> +
>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  {
>  	const struct rcar_csi2_format *format;
> @@ -490,6 +526,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  	/* Code is validated in set_fmt. */
>  	format = rcsi2_code_to_fmt(priv->mf.code);
>  
> +	/* Get the remote mbus config to get the number of enabled lanes. */
> +	ret = rcsi2_get_mbus_config(priv);
> +	if (ret)
> +		return ret;
> +
>  	/*
>  	 * Enable all supported CSI-2 channels with virtual channel and
>  	 * data type matching.
> @@ -522,7 +563,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  	}
>  
>  	phycnt = PHYCNT_ENABLECLK;
> -	phycnt |= (1 << priv->lanes) - 1;
> +	phycnt |= (1 << priv->active_lanes) - 1;
>  
>  	mbps = rcsi2_calc_mbps(priv, format->bpp);
>  	if (mbps < 0)
> @@ -748,6 +789,7 @@ static int rcsi2_notify_bound(struct v4l2_async_notifier *notifier,
>  	}
>  
>  	priv->remote = subdev;
> +	priv->remote_pad = pad;
>  
>  	dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad);
>  
> @@ -793,6 +835,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
>  			priv->lanes);
>  		return -EINVAL;
>  	}
> +	priv->active_lanes = priv->lanes;
>  
>  	for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) {
>  		priv->lane_swap[i] = i < priv->lanes ?
> -- 
> 2.25.1
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 2/4] media: v4l2-subdv: Introduce get_mbus_config pad op

[Sakari Ailus]

Hi Jacopo,

Thanks for the patchset.

"subdev" in the subject.

On Fri, Mar 13, 2020 at 03:40:33PM +0100, Jacopo Mondi wrote:
> Introduce a new pad operation to allow retrieving the media bus
> configuration on a subdevice pad.
> 
> The newly introduced operation reassembles the s/g_mbus_config video
> operation, which have been on their way to be deprecated since a long
> time.

How is this expected to work with existing drivers that just get their
configuration from DT/ACPI? Update to use this API driver by driver as
needed basis, or something else?

Have you thought about setting the configuration as well?

Where is this expected to be implemented? Transmitters only, and to be
called by receiver drivers?

I think ideally the g_mbus_config video op should go with this patchset,
and drivers using it converted. Given the likely small intersection of the
two (drivers usign the old video op), this might be possible to do later on
as well. But in that case g_mbus_config should be deprecated in
documentation.

> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  include/media/v4l2-subdev.h | 67 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 761aa83a3f3c..3a1afc00e094 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -350,6 +350,70 @@ struct v4l2_mbus_frame_desc {
>  	unsigned short num_entries;
>  };
>  
> +/**
> + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> + * @hsync_active: hsync active state: true for high, false for low
> + * @vsync_active: vsync active state: true for high, false for low
> + * @pclk_rising: pixel clock active edge: true for rising, false for falling
> + * @data_active: data lines active state: true for high, false for low
> + */
> +struct v4l2_mbus_parallel_config {
> +	bool hsync_active : 1;
> +	bool vsync_active : 1;
> +	bool pclk_rising : 1;
> +	bool data_active : 1;

unsigned int, please.

> +};
> +
> +/**
> + * struct v4l2_mbus_csi2_dphy_config - MIPI CSI-2 DPHY mbus configuration
> + * @data_lanes: number of data lanes in use
> + * @clock_noncontinuous: non continuous clock enable flag
> + */
> +struct v4l2_mbus_csi2_dphy_config {
> +	unsigned int data_lanes : 3;
> +	bool clock_noncontinuous : 1;

This should be unsigned int as well.

> +};
> +
> +/**
> + * struct v4l2_mbus_csi2_cphy_config - MIPI CSI-2 CPHY mbus configuration
> + *
> + * TODO
> + */
> +struct v4l2_mbus_csi2_cphy_config {
> +	/* TODO */
> +};
> +
> +/**
> + * struct v4l2_mbus_pad_config - media bus configuration
> + *
> + * Report the subdevice media bus information to inform the caller of the
> + * current bus configuration. The structure describes bus configuration
> + * parameters that might change in-between streaming sessions, in order to allow
> + * the caller to adjust its media bus configuration to match what is reported
> + * here.
> + *
> + * TODO: add '_pad_' to the name to distinguish this from the structure
> + * defined in v4l2_mediabus.h used for the same purpose by the g/s_mbus_config
> + * video operations. Reuse the there defined enum v4l2_mbus_type to define
> + * the bus type.
> + *
> + * @type: mbus type. See &enum v4l2_mbus_type
> + * @parallel: parallel bus configuration parameters.
> + *	      See &struct v4l2_mbus_parallel_config
> + * @csi2_dphy: MIPI CSI-2 DPHY configuration parameters
> + *	       See &struct v4l2_mbus_csi2_dphy_config
> + * @csi2_cphy: MIPI CSI-2 CPHY configuration parameters
> + *	       See &struct v4l2_mbus_csi2_cphy_config
> + */
> +struct v4l2_mbus_pad_config {
> +	enum v4l2_mbus_type type;
> +	union {
> +		struct v4l2_mbus_parallel_config parallel;
> +		struct v4l2_mbus_csi2_dphy_config csi2_dphy;
> +		struct v4l2_mbus_csi2_cphy_config csi2_cphy;
> +	};
> +};
> +
>  /**
>   * struct v4l2_subdev_video_ops - Callbacks used when v4l device was opened
>   *				  in video mode.
> @@ -670,6 +734,7 @@ struct v4l2_subdev_pad_config {
>   *
>   * @set_frame_desc: set the low level media bus frame parameters, @fd array
>   *                  may be adjusted by the subdev driver to device capabilities.
> + * @get_mbus_config: get the current mbus configuration
>   */
>  struct v4l2_subdev_pad_ops {
>  	int (*init_cfg)(struct v4l2_subdev *sd,
> @@ -710,6 +775,8 @@ struct v4l2_subdev_pad_ops {
>  			      struct v4l2_mbus_frame_desc *fd);
>  	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
>  			      struct v4l2_mbus_frame_desc *fd);
> +	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> +			       struct v4l2_mbus_pad_config *config);
>  };
>  
>  /**

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/4] media: v4l2-subdv: Introduce get_mbus_config pad op

[Jacopo Mondi]

Hi Niklas, Huyn,
On Wed, Apr 08, 2020 at 12:22:55AM +0200, niklas.soderlund+renesas@ragnatech.se wrote:
> Hi Hyun and Jacopo,
>
> On 2020-04-01 15:30:38 -0700, Hyun Kwon wrote:
> > Hi Jacopo,
> >
> > Thanks for the patch.
> >
> > On Fri, 2020-03-13 at 07:40:33 -0700, Jacopo Mondi wrote:
> > > Introduce a new pad operation to allow retrieving the media bus
> > > configuration on a subdevice pad.
> > >
> > > The newly introduced operation reassembles the s/g_mbus_config video
> > > operation, which have been on their way to be deprecated since a long
> > > time.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  include/media/v4l2-subdev.h | 67 +++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 67 insertions(+)
> > >
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index 761aa83a3f3c..3a1afc00e094 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -350,6 +350,70 @@ struct v4l2_mbus_frame_desc {
> > >  	unsigned short num_entries;
> > >  };
> > >
> > > +/**
> > > + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> > > + * @hsync_active: hsync active state: true for high, false for low
> > > + * @vsync_active: vsync active state: true for high, false for low
> > > + * @pclk_rising: pixel clock active edge: true for rising, false for falling
> > > + * @data_active: data lines active state: true for high, false for low
> > > + */
> > > +struct v4l2_mbus_parallel_config {
> > > +	bool hsync_active : 1;
> > > +	bool vsync_active : 1;
> > > +	bool pclk_rising : 1;
> > > +	bool data_active : 1;
> > > +};
> > > +
> > > +/**
> > > + * struct v4l2_mbus_csi2_dphy_config - MIPI CSI-2 DPHY mbus configuration
> > > + * @data_lanes: number of data lanes in use
> > > + * @clock_noncontinuous: non continuous clock enable flag
> > > + */
> > > +struct v4l2_mbus_csi2_dphy_config {
> > > +	unsigned int data_lanes : 3;
> > > +	bool clock_noncontinuous : 1;
> > > +};
> > > +
> > > +/**
> > > + * struct v4l2_mbus_csi2_cphy_config - MIPI CSI-2 CPHY mbus configuration
> > > + *
> > > + * TODO
> > > + */
> > > +struct v4l2_mbus_csi2_cphy_config {
> > > +	/* TODO */
> > > +};
> > > +
> > > +/**
> > > + * struct v4l2_mbus_pad_config - media bus configuration
> > > + *
> > > + * Report the subdevice media bus information to inform the caller of the
> > > + * current bus configuration. The structure describes bus configuration
> > > + * parameters that might change in-between streaming sessions, in order to allow
> > > + * the caller to adjust its media bus configuration to match what is reported
> > > + * here.
> > > + *
> > > + * TODO: add '_pad_' to the name to distinguish this from the structure
> > > + * defined in v4l2_mediabus.h used for the same purpose by the g/s_mbus_config
> > > + * video operations. Reuse the there defined enum v4l2_mbus_type to define
> > > + * the bus type.
> > > + *
> > > + * @type: mbus type. See &enum v4l2_mbus_type
> > > + * @parallel: parallel bus configuration parameters.
> > > + *	      See &struct v4l2_mbus_parallel_config
> > > + * @csi2_dphy: MIPI CSI-2 DPHY configuration parameters
> > > + *	       See &struct v4l2_mbus_csi2_dphy_config
> > > + * @csi2_cphy: MIPI CSI-2 CPHY configuration parameters
> > > + *	       See &struct v4l2_mbus_csi2_cphy_config
> > > + */
> > > +struct v4l2_mbus_pad_config {
> > > +	enum v4l2_mbus_type type;
> > > +	union {
> > > +		struct v4l2_mbus_parallel_config parallel;
> > > +		struct v4l2_mbus_csi2_dphy_config csi2_dphy;
> > > +		struct v4l2_mbus_csi2_cphy_config csi2_cphy;
> > > +	};
> > > +};
> > > +
> > >  /**
> > >   * struct v4l2_subdev_video_ops - Callbacks used when v4l device was opened
> > >   *				  in video mode.
> > > @@ -670,6 +734,7 @@ struct v4l2_subdev_pad_config {
> > >   *
> > >   * @set_frame_desc: set the low level media bus frame parameters, @fd array
> > >   *                  may be adjusted by the subdev driver to device capabilities.
> > > + * @get_mbus_config: get the current mbus configuration
> > >   */
> > >  struct v4l2_subdev_pad_ops {
> > >  	int (*init_cfg)(struct v4l2_subdev *sd,
> > > @@ -710,6 +775,8 @@ struct v4l2_subdev_pad_ops {
> > >  			      struct v4l2_mbus_frame_desc *fd);
> > >  	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
> > >  			      struct v4l2_mbus_frame_desc *fd);
> > > +	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > > +			       struct v4l2_mbus_pad_config *config);
> >
> > Because this can be used in many different ways, there's more chance it can
> > be misused. That means, drivers call this in different locations, ex probe,
> > get format, start stream,,,, and on differnt pads, src or sink. So imagine
> > one set of drivers call on sink pad, and the other set call on source pad.
> > It works well only until those are mixed together.

I don't think we can right now establish all possible use cases, or
prevent people from shooting in their foot, moreover, the 'right'
usage really depends on the bus in use, and I can't tell where this is
will be used in the wild...

>
> That subdevice operations can be called at both probe and s_stream() is
> nothing new, I don't thin this is a new problem. But I agree maybe we
> could limit get_mbus_config() in the core to only be valid four source
> pads? Apart from this open question I think this patch looks good.
>

I'm a bit skeptical on limiting this to source pads as, again, this
really depends on the bus on which this operation is used. For my
limited knowledge, yes, the use case is always the receiver quering
the transmitter, but I don't feel like ruling out the opposite.

> >
> > So wouldn't it be better to put some restrictions? One is to document
> > recommendations. I think this better be called in stream on because
> > some bus config may change at runtime depending on other configuration.
> > So any bus config prior to stream-on may be outdated. The other is to
> > enforce in the code. Some, but maybe not all, can be handled in
> > v4l2_subdev_call_pad_wrappers, for example allowing this call only on
> > source pad.
> >

I hear your concern, but I think it really depends on the use cases
and I would have an hard time to provide recommendations that
address all use cases.

Is your concern due to some mis-uses example you can describe ?

Thanks
   j

> > Thanks,
> > -hyun
> >
> > >  };
> > >
> > >  /**
> > > --
> > > 2.25.1
> > >
>
> --
> Regards,
> Niklas Söderlund

Re: [PATCH 4/4] media: rcar-vin: csi2: Negotiate data lanes number

[Jacopo Mondi]

Hi Niklas,

On Wed, Apr 08, 2020 at 12:33:31AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2020-03-13 15:40:35 +0100, Jacopo Mondi wrote:
> > Use the newly introduced get_mbus_config() subdevice pad operation to
> > retrieve the remote subdevice MIPI CSI-2 bus configuration and configure
> > the number of active data lanes accordingly.
> >
> > In order to be able to call the remote subdevice operation cache the
> > index of the remote pad connected to the single CSI-2 input port.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> Please s/rcar-vin: csi2:/rcar-csi2:/ in the subject.
>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 49 +++++++++++++++++++--
> >  1 file changed, 46 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index faa9fb23a2e9..4145e028dcdf 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -363,6 +363,7 @@ struct rcar_csi2 {
> >  	struct v4l2_async_notifier notifier;
> >  	struct v4l2_async_subdev asd;
> >  	struct v4l2_subdev *remote;
> > +	unsigned short remote_pad;
> >
> >  	struct v4l2_mbus_framefmt mf;
> >
> > @@ -371,6 +372,7 @@ struct rcar_csi2 {
> >
> >  	unsigned short lanes;
> >  	unsigned char lane_swap[4];
> > +	unsigned short active_lanes;
> >  };
> >
> >  static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
> > @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
> >
> >  	/* Wait for the clock and data lanes to enter LP-11 state. */
> >  	for (timeout = 0; timeout <= 20; timeout++) {
> > -		const u32 lane_mask = (1 << priv->lanes) - 1;
> > +		const u32 lane_mask = (1 << priv->active_lanes) - 1;
> >
> >  		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
> >  		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
> > @@ -471,11 +473,45 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
> >  	 * bps = link_freq * 2
> >  	 */
> >  	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
> > -	do_div(mbps, priv->lanes * 1000000);
> > +	do_div(mbps, priv->active_lanes * 1000000);
> >
> >  	return mbps;
> >  }
> >
> > +static int rcsi2_get_mbus_config(struct rcar_csi2 *priv)
>
> This function do not get the mbus configuration as much as update number
> of used lanes. How about renaming it rcsi2_update_lanes_used() or
> something similar ?
>

You know, I'm not sure. Getting the number of lanes is one part of
getting the remote bus configuration, there might be more and then
we'll need to go through a function rename... I'll think about it...

> > +{
> > +	struct v4l2_mbus_pad_config mbus_config;
> > +	int ret;
> > +
> > +	memset(&mbus_config, 0, sizeof(mbus_config));
> > +	ret = v4l2_subdev_call(priv->remote, pad, get_mbus_config,
> > +			       priv->remote_pad, &mbus_config);
> > +	if (ret && ret != -ENOIOCTLCMD) {
> > +		dev_err(priv->dev, "Failed to get remote mbus configuration\n");
> > +		return ret;
> > +	} else if (ret == -ENOIOCTLCMD) {
> > +		dev_dbg(priv->dev, "No remote mbus configuration available\n");
> > +		priv->active_lanes = priv->lanes;
> > +		return 0;
> > +	}
>
> How about something bellow to match style of driver?
>
>     priv->active_lanes = priv->lanes;
>
>     memset(&mbus_config, 0, sizeof(mbus_config));
>     ret = v4l2_subdev_call(... get_mbus_config ...)
>     if (ret && ret != -ENOIOCTLCMD) {
>         dev_err(priv->dev, "Failed to get remote mbus configuration\n");
>         return ret;
>     }

This would not exit gracefully if -ENOIOCTLCMD, but would continue
inspecting the mbus_config.type and fail with "Unsupported bus type"
which is not what we want...

Unfortunately I thinkg -ENOIOCTLCMD should be handled explicitely :(

>
> > +
> > +	if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
> > +		dev_err(priv->dev,
> > +			"Unsupported mbus type %u\n", mbus_config.type);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (mbus_config.csi2_dphy.data_lanes > priv->lanes) {
> > +		dev_err(priv->dev,
> > +			"Unsupported mbus config: too many data lanes %u\n",
> > +			mbus_config.csi2_dphy.data_lanes);
> > +		return -EINVAL;
> > +	}
> > +	priv->active_lanes = mbus_config.csi2_dphy.data_lanes;
> > +
> > +	return 0;
> > +}
> > +
> >  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  {
> >  	const struct rcar_csi2_format *format;
> > @@ -490,6 +526,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  	/* Code is validated in set_fmt. */
> >  	format = rcsi2_code_to_fmt(priv->mf.code);
> >
> > +	/* Get the remote mbus config to get the number of enabled lanes. */
> > +	ret = rcsi2_get_mbus_config(priv);
> > +	if (ret)
> > +		return ret;
> > +
> >  	/*
> >  	 * Enable all supported CSI-2 channels with virtual channel and
> >  	 * data type matching.
> > @@ -522,7 +563,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  	}
> >
> >  	phycnt = PHYCNT_ENABLECLK;
> > -	phycnt |= (1 << priv->lanes) - 1;
> > +	phycnt |= (1 << priv->active_lanes) - 1;
> >
> >  	mbps = rcsi2_calc_mbps(priv, format->bpp);
> >  	if (mbps < 0)
> > @@ -748,6 +789,7 @@ static int rcsi2_notify_bound(struct v4l2_async_notifier *notifier,
> >  	}
> >
> >  	priv->remote = subdev;
> > +	priv->remote_pad = pad;
> >
> >  	dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad);
> >
> > @@ -793,6 +835,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
> >  			priv->lanes);
> >  		return -EINVAL;
> >  	}
> > +	priv->active_lanes = priv->lanes;
> >
> >  	for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) {
> >  		priv->lane_swap[i] = i < priv->lanes ?
> > --
> > 2.25.1
> >
>
> --
> Regards,
> Niklas Söderlund

Re: [PATCH 2/4] media: v4l2-subdv: Introduce get_mbus_config pad op

[Jacopo Mondi]

Hi Sakari,

On Wed, Apr 08, 2020 at 11:47:04AM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> Thanks for the patchset.
>
> "subdev" in the subject.
>
> On Fri, Mar 13, 2020 at 03:40:33PM +0100, Jacopo Mondi wrote:
> > Introduce a new pad operation to allow retrieving the media bus
> > configuration on a subdevice pad.
> >
> > The newly introduced operation reassembles the s/g_mbus_config video
> > operation, which have been on their way to be deprecated since a long
> > time.
>
> How is this expected to work with existing drivers that just get their
> configuration from DT/ACPI? Update to use this API driver by driver as
> needed basis, or something else?

I think so.

The firmware specifies a somehow immutable value depending on the
hardware configuration (like the number of CSI-2 data lanes actually
wired between a receiver and a transmitter), this operation allow to
specify how many of them are actually in use depending on the
currently configured mode of operation, if ever required, otherwise
the firmware property provided value is used.

Dave's use case is slightly different, as clock continuous or
non-continuous would be specified in the receiver as a default, but
given the ability to switch between to transmitter at run-time, it has
to be refreshed by querying the current configuration using this op,
similar to the "two parallel sensors sharing the same data lines" I
tried to describe in the series' cover letter.

>
> Have you thought about setting the configuration as well?
>

Didn't get it, which configuration ?

> Where is this expected to be implemented? Transmitters only, and to be
> called by receiver drivers?

For the use cases at end at the moment, yes. Hyun had the same concern
in his reply to this series, but I don't feel like ruling out other
usages, as I can't tell at the moment on which busses this could prove
useful.

>
> I think ideally the g_mbus_config video op should go with this patchset,

Agreed

> and drivers using it converted. Given the likely small intersection of the
> two (drivers usign the old video op), this might be possible to do later on
> as well. But in that case g_mbus_config should be deprecated in
> documentation.

I'm fine deprecating the other operation in documentation, but porting
old drivers using the video ops should be done on top, otherwise it
will take some time and I could only compile test them.

Thanks
  j

>
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  include/media/v4l2-subdev.h | 67 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 761aa83a3f3c..3a1afc00e094 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -350,6 +350,70 @@ struct v4l2_mbus_frame_desc {
> >  	unsigned short num_entries;
> >  };
> >
> > +/**
> > + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> > + * @hsync_active: hsync active state: true for high, false for low
> > + * @vsync_active: vsync active state: true for high, false for low
> > + * @pclk_rising: pixel clock active edge: true for rising, false for falling
> > + * @data_active: data lines active state: true for high, false for low
> > + */
> > +struct v4l2_mbus_parallel_config {
> > +	bool hsync_active : 1;
> > +	bool vsync_active : 1;
> > +	bool pclk_rising : 1;
> > +	bool data_active : 1;
>
> unsigned int, please.
>
> > +};
> > +
> > +/**
> > + * struct v4l2_mbus_csi2_dphy_config - MIPI CSI-2 DPHY mbus configuration
> > + * @data_lanes: number of data lanes in use
> > + * @clock_noncontinuous: non continuous clock enable flag
> > + */
> > +struct v4l2_mbus_csi2_dphy_config {
> > +	unsigned int data_lanes : 3;
> > +	bool clock_noncontinuous : 1;
>
> This should be unsigned int as well.
>
> > +};
> > +
> > +/**
> > + * struct v4l2_mbus_csi2_cphy_config - MIPI CSI-2 CPHY mbus configuration
> > + *
> > + * TODO
> > + */
> > +struct v4l2_mbus_csi2_cphy_config {
> > +	/* TODO */
> > +};
> > +
> > +/**
> > + * struct v4l2_mbus_pad_config - media bus configuration
> > + *
> > + * Report the subdevice media bus information to inform the caller of the
> > + * current bus configuration. The structure describes bus configuration
> > + * parameters that might change in-between streaming sessions, in order to allow
> > + * the caller to adjust its media bus configuration to match what is reported
> > + * here.
> > + *
> > + * TODO: add '_pad_' to the name to distinguish this from the structure
> > + * defined in v4l2_mediabus.h used for the same purpose by the g/s_mbus_config
> > + * video operations. Reuse the there defined enum v4l2_mbus_type to define
> > + * the bus type.
> > + *
> > + * @type: mbus type. See &enum v4l2_mbus_type
> > + * @parallel: parallel bus configuration parameters.
> > + *	      See &struct v4l2_mbus_parallel_config
> > + * @csi2_dphy: MIPI CSI-2 DPHY configuration parameters
> > + *	       See &struct v4l2_mbus_csi2_dphy_config
> > + * @csi2_cphy: MIPI CSI-2 CPHY configuration parameters
> > + *	       See &struct v4l2_mbus_csi2_cphy_config
> > + */
> > +struct v4l2_mbus_pad_config {
> > +	enum v4l2_mbus_type type;
> > +	union {
> > +		struct v4l2_mbus_parallel_config parallel;
> > +		struct v4l2_mbus_csi2_dphy_config csi2_dphy;
> > +		struct v4l2_mbus_csi2_cphy_config csi2_cphy;
> > +	};
> > +};
> > +
> >  /**
> >   * struct v4l2_subdev_video_ops - Callbacks used when v4l device was opened
> >   *				  in video mode.
> > @@ -670,6 +734,7 @@ struct v4l2_subdev_pad_config {
> >   *
> >   * @set_frame_desc: set the low level media bus frame parameters, @fd array
> >   *                  may be adjusted by the subdev driver to device capabilities.
> > + * @get_mbus_config: get the current mbus configuration
> >   */
> >  struct v4l2_subdev_pad_ops {
> >  	int (*init_cfg)(struct v4l2_subdev *sd,
> > @@ -710,6 +775,8 @@ struct v4l2_subdev_pad_ops {
> >  			      struct v4l2_mbus_frame_desc *fd);
> >  	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
> >  			      struct v4l2_mbus_frame_desc *fd);
> > +	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > +			       struct v4l2_mbus_pad_config *config);
> >  };
> >
> >  /**
>
> --
> Kind regards,
>
> Sakari Ailus

Re: [PATCH 2/4] media: v4l2-subdv: Introduce get_mbus_config pad op

[Hyun Kwon]

Hi Niklas and Jacop,

On Thu, 2020-04-09 at 00:35:07 -0700, Jacopo Mondi wrote:
> Hi Niklas, Huyn,
> On Wed, Apr 08, 2020 at 12:22:55AM +0200, niklas.soderlund+renesas@ragnatech.se wrote:
> > Hi Hyun and Jacopo,
> >
> > On 2020-04-01 15:30:38 -0700, Hyun Kwon wrote:
> > > Hi Jacopo,
> > >
> > > Thanks for the patch.
> > >
> > > On Fri, 2020-03-13 at 07:40:33 -0700, Jacopo Mondi wrote:
> > > > Introduce a new pad operation to allow retrieving the media bus
> > > > configuration on a subdevice pad.
> > > >
> > > > The newly introduced operation reassembles the s/g_mbus_config video
> > > > operation, which have been on their way to be deprecated since a long
> > > > time.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >  include/media/v4l2-subdev.h | 67 +++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 67 insertions(+)
> > > >
> > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > index 761aa83a3f3c..3a1afc00e094 100644
> > > > --- a/include/media/v4l2-subdev.h
> > > > +++ b/include/media/v4l2-subdev.h
> > > > @@ -350,6 +350,70 @@ struct v4l2_mbus_frame_desc {
> > > >  	unsigned short num_entries;
> > > >  };
> > > >
> > > > +/**
> > > > + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> > > > + * @hsync_active: hsync active state: true for high, false for low
> > > > + * @vsync_active: vsync active state: true for high, false for low
> > > > + * @pclk_rising: pixel clock active edge: true for rising, false for falling
> > > > + * @data_active: data lines active state: true for high, false for low
> > > > + */
> > > > +struct v4l2_mbus_parallel_config {
> > > > +	bool hsync_active : 1;
> > > > +	bool vsync_active : 1;
> > > > +	bool pclk_rising : 1;
> > > > +	bool data_active : 1;
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct v4l2_mbus_csi2_dphy_config - MIPI CSI-2 DPHY mbus configuration
> > > > + * @data_lanes: number of data lanes in use
> > > > + * @clock_noncontinuous: non continuous clock enable flag
> > > > + */
> > > > +struct v4l2_mbus_csi2_dphy_config {
> > > > +	unsigned int data_lanes : 3;
> > > > +	bool clock_noncontinuous : 1;
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct v4l2_mbus_csi2_cphy_config - MIPI CSI-2 CPHY mbus configuration
> > > > + *
> > > > + * TODO
> > > > + */
> > > > +struct v4l2_mbus_csi2_cphy_config {
> > > > +	/* TODO */
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct v4l2_mbus_pad_config - media bus configuration
> > > > + *
> > > > + * Report the subdevice media bus information to inform the caller of the
> > > > + * current bus configuration. The structure describes bus configuration
> > > > + * parameters that might change in-between streaming sessions, in order to allow
> > > > + * the caller to adjust its media bus configuration to match what is reported
> > > > + * here.
> > > > + *
> > > > + * TODO: add '_pad_' to the name to distinguish this from the structure
> > > > + * defined in v4l2_mediabus.h used for the same purpose by the g/s_mbus_config
> > > > + * video operations. Reuse the there defined enum v4l2_mbus_type to define
> > > > + * the bus type.
> > > > + *
> > > > + * @type: mbus type. See &enum v4l2_mbus_type
> > > > + * @parallel: parallel bus configuration parameters.
> > > > + *	      See &struct v4l2_mbus_parallel_config
> > > > + * @csi2_dphy: MIPI CSI-2 DPHY configuration parameters
> > > > + *	       See &struct v4l2_mbus_csi2_dphy_config
> > > > + * @csi2_cphy: MIPI CSI-2 CPHY configuration parameters
> > > > + *	       See &struct v4l2_mbus_csi2_cphy_config
> > > > + */
> > > > +struct v4l2_mbus_pad_config {
> > > > +	enum v4l2_mbus_type type;
> > > > +	union {
> > > > +		struct v4l2_mbus_parallel_config parallel;
> > > > +		struct v4l2_mbus_csi2_dphy_config csi2_dphy;
> > > > +		struct v4l2_mbus_csi2_cphy_config csi2_cphy;
> > > > +	};
> > > > +};
> > > > +
> > > >  /**
> > > >   * struct v4l2_subdev_video_ops - Callbacks used when v4l device was opened
> > > >   *				  in video mode.
> > > > @@ -670,6 +734,7 @@ struct v4l2_subdev_pad_config {
> > > >   *
> > > >   * @set_frame_desc: set the low level media bus frame parameters, @fd array
> > > >   *                  may be adjusted by the subdev driver to device capabilities.
> > > > + * @get_mbus_config: get the current mbus configuration
> > > >   */
> > > >  struct v4l2_subdev_pad_ops {
> > > >  	int (*init_cfg)(struct v4l2_subdev *sd,
> > > > @@ -710,6 +775,8 @@ struct v4l2_subdev_pad_ops {
> > > >  			      struct v4l2_mbus_frame_desc *fd);
> > > >  	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
> > > >  			      struct v4l2_mbus_frame_desc *fd);
> > > > +	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > > > +			       struct v4l2_mbus_pad_config *config);
> > >
> > > Because this can be used in many different ways, there's more chance it can
> > > be misused. That means, drivers call this in different locations, ex probe,
> > > get format, start stream,,,, and on differnt pads, src or sink. So imagine
> > > one set of drivers call on sink pad, and the other set call on source pad.
> > > It works well only until those are mixed together.
> 
> I don't think we can right now establish all possible use cases, or
> prevent people from shooting in their foot, moreover, the 'right'
> usage really depends on the bus in use, and I can't tell where this is
> will be used in the wild...
> 
> >
> > That subdevice operations can be called at both probe and s_stream() is
> > nothing new, I don't thin this is a new problem. But I agree maybe we
> > could limit get_mbus_config() in the core to only be valid four source
> > pads? Apart from this open question I think this patch looks good.
> >
> 
> I'm a bit skeptical on limiting this to source pads as, again, this
> really depends on the bus on which this operation is used. For my
> limited knowledge, yes, the use case is always the receiver quering
> the transmitter, but I don't feel like ruling out the opposite.
> 
> > >
> > > So wouldn't it be better to put some restrictions? One is to document
> > > recommendations. I think this better be called in stream on because
> > > some bus config may change at runtime depending on other configuration.
> > > So any bus config prior to stream-on may be outdated. The other is to
> > > enforce in the code. Some, but maybe not all, can be handled in
> > > v4l2_subdev_call_pad_wrappers, for example allowing this call only on
> > > source pad.
> > >
> 
> I hear your concern, but I think it really depends on the use cases
> and I would have an hard time to provide recommendations that
> address all use cases.
> 
> Is your concern due to some mis-uses example you can describe ?

Yeah, while trying this out, I was thinking how it should be used. I ended
up with a specific way: single direction, starting from stream on,

(streamon) -> max9286 -> (g_mbus_conf) -> max96705 -> (g_mbus_conf) -> sensor1

It's this way because max96705 doesn't have own vsync polarity, and it
should get it from the connected sensor. While someone may implement the same
in complete opposite direction for another set of drivers, starting from
sensor, or even in different entry point,

(s_fmt) -> sensor2 -> (g_mbus_conf) -> some_ser -> (g_mbus_conf) -> some_des

When the sensor2 driver is used with max96705 above, there could be a problem
such as circular calls or getting an outdated value. And it is harder to fix at
that point. So I thought enforcing the direction works for current use cases
(under my visilbity), and may help avoid such issue in future. Probably it may
be just me over-thinking, as it sounds like? :-)

Thanks,
-hyun

> 
> Thanks
>    j
> 
> > > Thanks,
> > > -hyun
> > >
> > > >  };
> > > >
> > > >  /**
> > > > --
> > > > 2.25.1
> > > >
> >
> > --
> > Regards,
> > Niklas Söderlund

Re: [PATCH 2/4] media: v4l2-subdv: Introduce get_mbus_config pad op

[niklas.soderlund+renesas]

Hi Huyn,

On 2020-04-09 17:30:28 -0700, Hyun Kwon wrote:
> Hi Niklas and Jacop,
> 
> On Thu, 2020-04-09 at 00:35:07 -0700, Jacopo Mondi wrote:
> > Hi Niklas, Huyn,
> > On Wed, Apr 08, 2020 at 12:22:55AM +0200, niklas.soderlund+renesas@ragnatech.se wrote:
> > > Hi Hyun and Jacopo,
> > >
> > > On 2020-04-01 15:30:38 -0700, Hyun Kwon wrote:
> > > > Hi Jacopo,
> > > >
> > > > Thanks for the patch.
> > > >
> > > > On Fri, 2020-03-13 at 07:40:33 -0700, Jacopo Mondi wrote:
> > > > > Introduce a new pad operation to allow retrieving the media bus
> > > > > configuration on a subdevice pad.
> > > > >
> > > > > The newly introduced operation reassembles the s/g_mbus_config video
> > > > > operation, which have been on their way to be deprecated since a long
> > > > > time.
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > ---
> > > > >  include/media/v4l2-subdev.h | 67 +++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 67 insertions(+)
> > > > >
> > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > > index 761aa83a3f3c..3a1afc00e094 100644
> > > > > --- a/include/media/v4l2-subdev.h
> > > > > +++ b/include/media/v4l2-subdev.h
> > > > > @@ -350,6 +350,70 @@ struct v4l2_mbus_frame_desc {
> > > > >  	unsigned short num_entries;
> > > > >  };
> > > > >
> > > > > +/**
> > > > > + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> > > > > + * @hsync_active: hsync active state: true for high, false for low
> > > > > + * @vsync_active: vsync active state: true for high, false for low
> > > > > + * @pclk_rising: pixel clock active edge: true for rising, false for falling
> > > > > + * @data_active: data lines active state: true for high, false for low
> > > > > + */
> > > > > +struct v4l2_mbus_parallel_config {
> > > > > +	bool hsync_active : 1;
> > > > > +	bool vsync_active : 1;
> > > > > +	bool pclk_rising : 1;
> > > > > +	bool data_active : 1;
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * struct v4l2_mbus_csi2_dphy_config - MIPI CSI-2 DPHY mbus configuration
> > > > > + * @data_lanes: number of data lanes in use
> > > > > + * @clock_noncontinuous: non continuous clock enable flag
> > > > > + */
> > > > > +struct v4l2_mbus_csi2_dphy_config {
> > > > > +	unsigned int data_lanes : 3;
> > > > > +	bool clock_noncontinuous : 1;
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * struct v4l2_mbus_csi2_cphy_config - MIPI CSI-2 CPHY mbus configuration
> > > > > + *
> > > > > + * TODO
> > > > > + */
> > > > > +struct v4l2_mbus_csi2_cphy_config {
> > > > > +	/* TODO */
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * struct v4l2_mbus_pad_config - media bus configuration
> > > > > + *
> > > > > + * Report the subdevice media bus information to inform the caller of the
> > > > > + * current bus configuration. The structure describes bus configuration
> > > > > + * parameters that might change in-between streaming sessions, in order to allow
> > > > > + * the caller to adjust its media bus configuration to match what is reported
> > > > > + * here.
> > > > > + *
> > > > > + * TODO: add '_pad_' to the name to distinguish this from the structure
> > > > > + * defined in v4l2_mediabus.h used for the same purpose by the g/s_mbus_config
> > > > > + * video operations. Reuse the there defined enum v4l2_mbus_type to define
> > > > > + * the bus type.
> > > > > + *
> > > > > + * @type: mbus type. See &enum v4l2_mbus_type
> > > > > + * @parallel: parallel bus configuration parameters.
> > > > > + *	      See &struct v4l2_mbus_parallel_config
> > > > > + * @csi2_dphy: MIPI CSI-2 DPHY configuration parameters
> > > > > + *	       See &struct v4l2_mbus_csi2_dphy_config
> > > > > + * @csi2_cphy: MIPI CSI-2 CPHY configuration parameters
> > > > > + *	       See &struct v4l2_mbus_csi2_cphy_config
> > > > > + */
> > > > > +struct v4l2_mbus_pad_config {
> > > > > +	enum v4l2_mbus_type type;
> > > > > +	union {
> > > > > +		struct v4l2_mbus_parallel_config parallel;
> > > > > +		struct v4l2_mbus_csi2_dphy_config csi2_dphy;
> > > > > +		struct v4l2_mbus_csi2_cphy_config csi2_cphy;
> > > > > +	};
> > > > > +};
> > > > > +
> > > > >  /**
> > > > >   * struct v4l2_subdev_video_ops - Callbacks used when v4l device was opened
> > > > >   *				  in video mode.
> > > > > @@ -670,6 +734,7 @@ struct v4l2_subdev_pad_config {
> > > > >   *
> > > > >   * @set_frame_desc: set the low level media bus frame parameters, @fd array
> > > > >   *                  may be adjusted by the subdev driver to device capabilities.
> > > > > + * @get_mbus_config: get the current mbus configuration
> > > > >   */
> > > > >  struct v4l2_subdev_pad_ops {
> > > > >  	int (*init_cfg)(struct v4l2_subdev *sd,
> > > > > @@ -710,6 +775,8 @@ struct v4l2_subdev_pad_ops {
> > > > >  			      struct v4l2_mbus_frame_desc *fd);
> > > > >  	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
> > > > >  			      struct v4l2_mbus_frame_desc *fd);
> > > > > +	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > > > > +			       struct v4l2_mbus_pad_config *config);
> > > >
> > > > Because this can be used in many different ways, there's more chance it can
> > > > be misused. That means, drivers call this in different locations, ex probe,
> > > > get format, start stream,,,, and on differnt pads, src or sink. So imagine
> > > > one set of drivers call on sink pad, and the other set call on source pad.
> > > > It works well only until those are mixed together.
> > 
> > I don't think we can right now establish all possible use cases, or
> > prevent people from shooting in their foot, moreover, the 'right'
> > usage really depends on the bus in use, and I can't tell where this is
> > will be used in the wild...
> > 
> > >
> > > That subdevice operations can be called at both probe and s_stream() is
> > > nothing new, I don't thin this is a new problem. But I agree maybe we
> > > could limit get_mbus_config() in the core to only be valid four source
> > > pads? Apart from this open question I think this patch looks good.
> > >
> > 
> > I'm a bit skeptical on limiting this to source pads as, again, this
> > really depends on the bus on which this operation is used. For my
> > limited knowledge, yes, the use case is always the receiver quering
> > the transmitter, but I don't feel like ruling out the opposite.
> > 
> > > >
> > > > So wouldn't it be better to put some restrictions? One is to document
> > > > recommendations. I think this better be called in stream on because
> > > > some bus config may change at runtime depending on other configuration.
> > > > So any bus config prior to stream-on may be outdated. The other is to
> > > > enforce in the code. Some, but maybe not all, can be handled in
> > > > v4l2_subdev_call_pad_wrappers, for example allowing this call only on
> > > > source pad.
> > > >
> > 
> > I hear your concern, but I think it really depends on the use cases
> > and I would have an hard time to provide recommendations that
> > address all use cases.
> > 
> > Is your concern due to some mis-uses example you can describe ?
> 
> Yeah, while trying this out, I was thinking how it should be used. I ended
> up with a specific way: single direction, starting from stream on,
> 
> (streamon) -> max9286 -> (g_mbus_conf) -> max96705 -> (g_mbus_conf) -> sensor1
> 
> It's this way because max96705 doesn't have own vsync polarity, and it
> should get it from the connected sensor. While someone may implement the same
> in complete opposite direction for another set of drivers, starting from
> sensor, or even in different entry point,

I agree with the use-case above.

> 
> (s_fmt) -> sensor2 -> (g_mbus_conf) -> some_ser -> (g_mbus_conf) -> some_des
> 
> When the sensor2 driver is used with max96705 above, there could be a problem
> such as circular calls or getting an outdated value. And it is harder to fix at
> that point. So I thought enforcing the direction works for current use cases
> (under my visilbity), and may help avoid such issue in future. Probably it may
> be just me over-thinking, as it sounds like? :-)

If a format is set on a subdevice we are operating on a subdevice that 
is part of a media device right? If so shall setting the format of the 
different entities of the graph involve cross entry calls? Shall not the 
entire pipeline format be validated at stream_start() and that is the 
time g_mbus_conf() would be involved like in the first case above. I 
might have misunderstood something if so I apologize.

> 
> Thanks,
> -hyun
> 
> > 
> > Thanks
> >    j
> > 
> > > > Thanks,
> > > > -hyun
> > > >
> > > > >  };
> > > > >
> > > > >  /**
> > > > > --
> > > > > 2.25.1
> > > > >
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund

-- 
Regards,
Niklas Söderlund

Re: [PATCH 2/4] media: v4l2-subdv: Introduce get_mbus_config pad op

[Hyun Kwon]

Hi Niklas!

On Fri, 2020-04-10 at 03:24:06 -0700, niklas.soderlund+renesas@ragnatech.se wrote:
> Hi Huyn,
> 
> On 2020-04-09 17:30:28 -0700, Hyun Kwon wrote:
> > Hi Niklas and Jacop,
> > 
> > On Thu, 2020-04-09 at 00:35:07 -0700, Jacopo Mondi wrote:
> > > Hi Niklas, Huyn,
> > > On Wed, Apr 08, 2020 at 12:22:55AM +0200, niklas.soderlund+renesas@ragnatech.se wrote:
> > > > Hi Hyun and Jacopo,
> > > >
> > > > On 2020-04-01 15:30:38 -0700, Hyun Kwon wrote:
> > > > > Hi Jacopo,
> > > > >
> > > > > Thanks for the patch.
> > > > >
> > > > > On Fri, 2020-03-13 at 07:40:33 -0700, Jacopo Mondi wrote:
> > > > > > Introduce a new pad operation to allow retrieving the media bus
> > > > > > configuration on a subdevice pad.
> > > > > >
> > > > > > The newly introduced operation reassembles the s/g_mbus_config video
> > > > > > operation, which have been on their way to be deprecated since a long
> > > > > > time.
> > > > > >
> > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > > ---
> > > > > >  include/media/v4l2-subdev.h | 67 +++++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 67 insertions(+)
> > > > > >
> > > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > > > index 761aa83a3f3c..3a1afc00e094 100644
> > > > > > --- a/include/media/v4l2-subdev.h
> > > > > > +++ b/include/media/v4l2-subdev.h
> > > > > > @@ -350,6 +350,70 @@ struct v4l2_mbus_frame_desc {
> > > > > >  	unsigned short num_entries;
> > > > > >  };
> > > > > >
> > > > > > +/**
> > > > > > + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> > > > > > + * @hsync_active: hsync active state: true for high, false for low
> > > > > > + * @vsync_active: vsync active state: true for high, false for low
> > > > > > + * @pclk_rising: pixel clock active edge: true for rising, false for falling
> > > > > > + * @data_active: data lines active state: true for high, false for low
> > > > > > + */
> > > > > > +struct v4l2_mbus_parallel_config {
> > > > > > +	bool hsync_active : 1;
> > > > > > +	bool vsync_active : 1;
> > > > > > +	bool pclk_rising : 1;
> > > > > > +	bool data_active : 1;
> > > > > > +};
> > > > > > +
> > > > > > +/**
> > > > > > + * struct v4l2_mbus_csi2_dphy_config - MIPI CSI-2 DPHY mbus configuration
> > > > > > + * @data_lanes: number of data lanes in use
> > > > > > + * @clock_noncontinuous: non continuous clock enable flag
> > > > > > + */
> > > > > > +struct v4l2_mbus_csi2_dphy_config {
> > > > > > +	unsigned int data_lanes : 3;
> > > > > > +	bool clock_noncontinuous : 1;
> > > > > > +};
> > > > > > +
> > > > > > +/**
> > > > > > + * struct v4l2_mbus_csi2_cphy_config - MIPI CSI-2 CPHY mbus configuration
> > > > > > + *
> > > > > > + * TODO
> > > > > > + */
> > > > > > +struct v4l2_mbus_csi2_cphy_config {
> > > > > > +	/* TODO */
> > > > > > +};
> > > > > > +
> > > > > > +/**
> > > > > > + * struct v4l2_mbus_pad_config - media bus configuration
> > > > > > + *
> > > > > > + * Report the subdevice media bus information to inform the caller of the
> > > > > > + * current bus configuration. The structure describes bus configuration
> > > > > > + * parameters that might change in-between streaming sessions, in order to allow
> > > > > > + * the caller to adjust its media bus configuration to match what is reported
> > > > > > + * here.
> > > > > > + *
> > > > > > + * TODO: add '_pad_' to the name to distinguish this from the structure
> > > > > > + * defined in v4l2_mediabus.h used for the same purpose by the g/s_mbus_config
> > > > > > + * video operations. Reuse the there defined enum v4l2_mbus_type to define
> > > > > > + * the bus type.
> > > > > > + *
> > > > > > + * @type: mbus type. See &enum v4l2_mbus_type
> > > > > > + * @parallel: parallel bus configuration parameters.
> > > > > > + *	      See &struct v4l2_mbus_parallel_config
> > > > > > + * @csi2_dphy: MIPI CSI-2 DPHY configuration parameters
> > > > > > + *	       See &struct v4l2_mbus_csi2_dphy_config
> > > > > > + * @csi2_cphy: MIPI CSI-2 CPHY configuration parameters
> > > > > > + *	       See &struct v4l2_mbus_csi2_cphy_config
> > > > > > + */
> > > > > > +struct v4l2_mbus_pad_config {
> > > > > > +	enum v4l2_mbus_type type;
> > > > > > +	union {
> > > > > > +		struct v4l2_mbus_parallel_config parallel;
> > > > > > +		struct v4l2_mbus_csi2_dphy_config csi2_dphy;
> > > > > > +		struct v4l2_mbus_csi2_cphy_config csi2_cphy;
> > > > > > +	};
> > > > > > +};
> > > > > > +
> > > > > >  /**
> > > > > >   * struct v4l2_subdev_video_ops - Callbacks used when v4l device was opened
> > > > > >   *				  in video mode.
> > > > > > @@ -670,6 +734,7 @@ struct v4l2_subdev_pad_config {
> > > > > >   *
> > > > > >   * @set_frame_desc: set the low level media bus frame parameters, @fd array
> > > > > >   *                  may be adjusted by the subdev driver to device capabilities.
> > > > > > + * @get_mbus_config: get the current mbus configuration
> > > > > >   */
> > > > > >  struct v4l2_subdev_pad_ops {
> > > > > >  	int (*init_cfg)(struct v4l2_subdev *sd,
> > > > > > @@ -710,6 +775,8 @@ struct v4l2_subdev_pad_ops {
> > > > > >  			      struct v4l2_mbus_frame_desc *fd);
> > > > > >  	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
> > > > > >  			      struct v4l2_mbus_frame_desc *fd);
> > > > > > +	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > > > > > +			       struct v4l2_mbus_pad_config *config);
> > > > >
> > > > > Because this can be used in many different ways, there's more chance it can
> > > > > be misused. That means, drivers call this in different locations, ex probe,
> > > > > get format, start stream,,,, and on differnt pads, src or sink. So imagine
> > > > > one set of drivers call on sink pad, and the other set call on source pad.
> > > > > It works well only until those are mixed together.
> > > 
> > > I don't think we can right now establish all possible use cases, or
> > > prevent people from shooting in their foot, moreover, the 'right'
> > > usage really depends on the bus in use, and I can't tell where this is
> > > will be used in the wild...
> > > 
> > > >
> > > > That subdevice operations can be called at both probe and s_stream() is
> > > > nothing new, I don't thin this is a new problem. But I agree maybe we
> > > > could limit get_mbus_config() in the core to only be valid four source
> > > > pads? Apart from this open question I think this patch looks good.
> > > >
> > > 
> > > I'm a bit skeptical on limiting this to source pads as, again, this
> > > really depends on the bus on which this operation is used. For my
> > > limited knowledge, yes, the use case is always the receiver quering
> > > the transmitter, but I don't feel like ruling out the opposite.
> > > 
> > > > >
> > > > > So wouldn't it be better to put some restrictions? One is to document
> > > > > recommendations. I think this better be called in stream on because
> > > > > some bus config may change at runtime depending on other configuration.
> > > > > So any bus config prior to stream-on may be outdated. The other is to
> > > > > enforce in the code. Some, but maybe not all, can be handled in
> > > > > v4l2_subdev_call_pad_wrappers, for example allowing this call only on
> > > > > source pad.
> > > > >
> > > 
> > > I hear your concern, but I think it really depends on the use cases
> > > and I would have an hard time to provide recommendations that
> > > address all use cases.
> > > 
> > > Is your concern due to some mis-uses example you can describe ?
> > 
> > Yeah, while trying this out, I was thinking how it should be used. I ended
> > up with a specific way: single direction, starting from stream on,
> > 
> > (streamon) -> max9286 -> (g_mbus_conf) -> max96705 -> (g_mbus_conf) -> sensor1
> > 
> > It's this way because max96705 doesn't have own vsync polarity, and it
> > should get it from the connected sensor. While someone may implement the same
> > in complete opposite direction for another set of drivers, starting from
> > sensor, or even in different entry point,
> 
> I agree with the use-case above.
> 
> > 
> > (s_fmt) -> sensor2 -> (g_mbus_conf) -> some_ser -> (g_mbus_conf) -> some_des
> > 
> > When the sensor2 driver is used with max96705 above, there could be a problem
> > such as circular calls or getting an outdated value. And it is harder to fix at
> > that point. So I thought enforcing the direction works for current use cases
> > (under my visilbity), and may help avoid such issue in future. Probably it may
> > be just me over-thinking, as it sounds like? :-)
> 
> If a format is set on a subdevice we are operating on a subdevice that 
> is part of a media device right? If so shall setting the format of the 
> different entities of the graph involve cross entry calls? Shall not the 
> entire pipeline format be validated at stream_start() and that is the 
> time g_mbus_conf() would be involved like in the first case above. I 
> might have misunderstood something if so I apologize.
> 

In this patch, it's fully up to driver implementation, so it's legitimate
if some driver decides to call that in subdev set format and call another
get_mbus_config() within it.


Thanks,
-hyun

Re: [PATCH 2/4] media: v4l2-subdv: Introduce get_mbus_config pad op

[Laurent Pinchart]

Hi Huyn,

On Fri, Apr 10, 2020 at 12:26:26PM -0700, Hyun Kwon wrote:
> On Fri, 2020-04-10 at 03:24:06 -0700, niklas.soderlund+renesas@ragnatech.se wrote:
> > On 2020-04-09 17:30:28 -0700, Hyun Kwon wrote:
> > > On Thu, 2020-04-09 at 00:35:07 -0700, Jacopo Mondi wrote:
> > > > Hi Niklas, Huyn,
> > > > On Wed, Apr 08, 2020 at 12:22:55AM +0200, niklas.soderlund+renesas@ragnatech.se wrote:
> > > > > On 2020-04-01 15:30:38 -0700, Hyun Kwon wrote:
> > > > > > On Fri, 2020-03-13 at 07:40:33 -0700, Jacopo Mondi wrote:
> > > > > > > Introduce a new pad operation to allow retrieving the media bus
> > > > > > > configuration on a subdevice pad.
> > > > > > >
> > > > > > > The newly introduced operation reassembles the s/g_mbus_config video
> > > > > > > operation, which have been on their way to be deprecated since a long
> > > > > > > time.
> > > > > > >
> > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > > > ---
> > > > > > >  include/media/v4l2-subdev.h | 67 +++++++++++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 67 insertions(+)
> > > > > > >
> > > > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > > > > index 761aa83a3f3c..3a1afc00e094 100644
> > > > > > > --- a/include/media/v4l2-subdev.h
> > > > > > > +++ b/include/media/v4l2-subdev.h
> > > > > > > @@ -350,6 +350,70 @@ struct v4l2_mbus_frame_desc {
> > > > > > >  	unsigned short num_entries;
> > > > > > >  };
> > > > > > >
> > > > > > > +/**
> > > > > > > + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> > > > > > > + * @hsync_active: hsync active state: true for high, false for low
> > > > > > > + * @vsync_active: vsync active state: true for high, false for low
> > > > > > > + * @pclk_rising: pixel clock active edge: true for rising, false for falling
> > > > > > > + * @data_active: data lines active state: true for high, false for low
> > > > > > > + */
> > > > > > > +struct v4l2_mbus_parallel_config {
> > > > > > > +	bool hsync_active : 1;
> > > > > > > +	bool vsync_active : 1;
> > > > > > > +	bool pclk_rising : 1;
> > > > > > > +	bool data_active : 1;
> > > > > > > +};
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * struct v4l2_mbus_csi2_dphy_config - MIPI CSI-2 DPHY mbus configuration
> > > > > > > + * @data_lanes: number of data lanes in use
> > > > > > > + * @clock_noncontinuous: non continuous clock enable flag
> > > > > > > + */
> > > > > > > +struct v4l2_mbus_csi2_dphy_config {
> > > > > > > +	unsigned int data_lanes : 3;
> > > > > > > +	bool clock_noncontinuous : 1;
> > > > > > > +};
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * struct v4l2_mbus_csi2_cphy_config - MIPI CSI-2 CPHY mbus configuration
> > > > > > > + *
> > > > > > > + * TODO
> > > > > > > + */
> > > > > > > +struct v4l2_mbus_csi2_cphy_config {
> > > > > > > +	/* TODO */
> > > > > > > +};
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * struct v4l2_mbus_pad_config - media bus configuration
> > > > > > > + *
> > > > > > > + * Report the subdevice media bus information to inform the caller of the
> > > > > > > + * current bus configuration. The structure describes bus configuration
> > > > > > > + * parameters that might change in-between streaming sessions, in order to allow
> > > > > > > + * the caller to adjust its media bus configuration to match what is reported
> > > > > > > + * here.
> > > > > > > + *
> > > > > > > + * TODO: add '_pad_' to the name to distinguish this from the structure
> > > > > > > + * defined in v4l2_mediabus.h used for the same purpose by the g/s_mbus_config
> > > > > > > + * video operations. Reuse the there defined enum v4l2_mbus_type to define
> > > > > > > + * the bus type.
> > > > > > > + *
> > > > > > > + * @type: mbus type. See &enum v4l2_mbus_type
> > > > > > > + * @parallel: parallel bus configuration parameters.
> > > > > > > + *	      See &struct v4l2_mbus_parallel_config
> > > > > > > + * @csi2_dphy: MIPI CSI-2 DPHY configuration parameters
> > > > > > > + *	       See &struct v4l2_mbus_csi2_dphy_config
> > > > > > > + * @csi2_cphy: MIPI CSI-2 CPHY configuration parameters
> > > > > > > + *	       See &struct v4l2_mbus_csi2_cphy_config
> > > > > > > + */
> > > > > > > +struct v4l2_mbus_pad_config {
> > > > > > > +	enum v4l2_mbus_type type;
> > > > > > > +	union {
> > > > > > > +		struct v4l2_mbus_parallel_config parallel;
> > > > > > > +		struct v4l2_mbus_csi2_dphy_config csi2_dphy;
> > > > > > > +		struct v4l2_mbus_csi2_cphy_config csi2_cphy;
> > > > > > > +	};
> > > > > > > +};
> > > > > > > +
> > > > > > >  /**
> > > > > > >   * struct v4l2_subdev_video_ops - Callbacks used when v4l device was opened
> > > > > > >   *				  in video mode.
> > > > > > > @@ -670,6 +734,7 @@ struct v4l2_subdev_pad_config {
> > > > > > >   *
> > > > > > >   * @set_frame_desc: set the low level media bus frame parameters, @fd array
> > > > > > >   *                  may be adjusted by the subdev driver to device capabilities.
> > > > > > > + * @get_mbus_config: get the current mbus configuration
> > > > > > >   */
> > > > > > >  struct v4l2_subdev_pad_ops {
> > > > > > >  	int (*init_cfg)(struct v4l2_subdev *sd,
> > > > > > > @@ -710,6 +775,8 @@ struct v4l2_subdev_pad_ops {
> > > > > > >  			      struct v4l2_mbus_frame_desc *fd);
> > > > > > >  	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
> > > > > > >  			      struct v4l2_mbus_frame_desc *fd);
> > > > > > > +	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > > > > > > +			       struct v4l2_mbus_pad_config *config);
> > > > > >
> > > > > > Because this can be used in many different ways, there's more chance it can
> > > > > > be misused. That means, drivers call this in different locations, ex probe,
> > > > > > get format, start stream,,,, and on differnt pads, src or sink. So imagine
> > > > > > one set of drivers call on sink pad, and the other set call on source pad.
> > > > > > It works well only until those are mixed together.
> > > > 
> > > > I don't think we can right now establish all possible use cases, or
> > > > prevent people from shooting in their foot, moreover, the 'right'
> > > > usage really depends on the bus in use, and I can't tell where this is
> > > > will be used in the wild...
> > > > 
> > > > > That subdevice operations can be called at both probe and s_stream() is
> > > > > nothing new, I don't thin this is a new problem. But I agree maybe we
> > > > > could limit get_mbus_config() in the core to only be valid four source
> > > > > pads? Apart from this open question I think this patch looks good.
> > > > 
> > > > I'm a bit skeptical on limiting this to source pads as, again, this
> > > > really depends on the bus on which this operation is used. For my
> > > > limited knowledge, yes, the use case is always the receiver quering
> > > > the transmitter, but I don't feel like ruling out the opposite.
> > > > 
> > > > > > So wouldn't it be better to put some restrictions? One is to document
> > > > > > recommendations. I think this better be called in stream on because
> > > > > > some bus config may change at runtime depending on other configuration.
> > > > > > So any bus config prior to stream-on may be outdated. The other is to
> > > > > > enforce in the code. Some, but maybe not all, can be handled in
> > > > > > v4l2_subdev_call_pad_wrappers, for example allowing this call only on
> > > > > > source pad.
> > > > 
> > > > I hear your concern, but I think it really depends on the use cases
> > > > and I would have an hard time to provide recommendations that
> > > > address all use cases.
> > > > 
> > > > Is your concern due to some mis-uses example you can describe ?
> > > 
> > > Yeah, while trying this out, I was thinking how it should be used. I ended
> > > up with a specific way: single direction, starting from stream on,
> > > 
> > > (streamon) -> max9286 -> (g_mbus_conf) -> max96705 -> (g_mbus_conf) -> sensor1
> > > 
> > > It's this way because max96705 doesn't have own vsync polarity, and it
> > > should get it from the connected sensor. While someone may implement the same
> > > in complete opposite direction for another set of drivers, starting from
> > > sensor, or even in different entry point,
> > 
> > I agree with the use-case above.
> > 
> > > 
> > > (s_fmt) -> sensor2 -> (g_mbus_conf) -> some_ser -> (g_mbus_conf) -> some_des
> > > 
> > > When the sensor2 driver is used with max96705 above, there could be a problem
> > > such as circular calls or getting an outdated value. And it is harder to fix at
> > > that point. So I thought enforcing the direction works for current use cases
> > > (under my visilbity), and may help avoid such issue in future. Probably it may
> > > be just me over-thinking, as it sounds like? :-)
> > 
> > If a format is set on a subdevice we are operating on a subdevice that 
> > is part of a media device right? If so shall setting the format of the 
> > different entities of the graph involve cross entry calls? Shall not the 
> > entire pipeline format be validated at stream_start() and that is the 
> > time g_mbus_conf() would be involved like in the first case above. I 
> > might have misunderstood something if so I apologize.
> 
> In this patch, it's fully up to driver implementation, so it's legitimate
> if some driver decides to call that in subdev set format and call another
> get_mbus_config() within it.

I share some of the concern that was expressed on this topic. The V4L2
subdev in-kernel API is full of operations that are specified as
generic, and are called at different times and different ways by
different drivers. The different subdevs then implement those operations
differently, as they're only tested with one or a small subset of V4L2
drivers, and we end up with subdevs that implement different and
incompatible behaviours. I think the use cases need to be considered
here, and we should specify the usage of this API in details.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] media: v4l2-subdv: Introduce get_mbus_config pad op

[Jacopo Mondi]

Hello,

On Wed, Apr 15, 2020 at 05:22:20AM +0300, Laurent Pinchart wrote:
> Hi Huyn,
>
> On Fri, Apr 10, 2020 at 12:26:26PM -0700, Hyun Kwon wrote:
> > On Fri, 2020-04-10 at 03:24:06 -0700, niklas.soderlund+renesas@ragnatech.se wrote:
> > > On 2020-04-09 17:30:28 -0700, Hyun Kwon wrote:
> > > > On Thu, 2020-04-09 at 00:35:07 -0700, Jacopo Mondi wrote:
> > > > > Hi Niklas, Huyn,
> > > > > On Wed, Apr 08, 2020 at 12:22:55AM +0200, niklas.soderlund+renesas@ragnatech.se wrote:
> > > > > > On 2020-04-01 15:30:38 -0700, Hyun Kwon wrote:
> > > > > > > On Fri, 2020-03-13 at 07:40:33 -0700, Jacopo Mondi wrote:
> > > > > > > > Introduce a new pad operation to allow retrieving the media bus
> > > > > > > > configuration on a subdevice pad.
> > > > > > > >
> > > > > > > > The newly introduced operation reassembles the s/g_mbus_config video
> > > > > > > > operation, which have been on their way to be deprecated since a long
> > > > > > > > time.
> > > > > > > >
> > > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > > > > ---
> > > > > > > >  include/media/v4l2-subdev.h | 67 +++++++++++++++++++++++++++++++++++++
> > > > > > > >  1 file changed, 67 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > > > > > index 761aa83a3f3c..3a1afc00e094 100644
> > > > > > > > --- a/include/media/v4l2-subdev.h
> > > > > > > > +++ b/include/media/v4l2-subdev.h
> > > > > > > > @@ -350,6 +350,70 @@ struct v4l2_mbus_frame_desc {
> > > > > > > >  	unsigned short num_entries;
> > > > > > > >  };
> > > > > > > >
> > > > > > > > +/**
> > > > > > > > + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> > > > > > > > + * @hsync_active: hsync active state: true for high, false for low
> > > > > > > > + * @vsync_active: vsync active state: true for high, false for low
> > > > > > > > + * @pclk_rising: pixel clock active edge: true for rising, false for falling
> > > > > > > > + * @data_active: data lines active state: true for high, false for low
> > > > > > > > + */
> > > > > > > > +struct v4l2_mbus_parallel_config {
> > > > > > > > +	bool hsync_active : 1;
> > > > > > > > +	bool vsync_active : 1;
> > > > > > > > +	bool pclk_rising : 1;
> > > > > > > > +	bool data_active : 1;
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * struct v4l2_mbus_csi2_dphy_config - MIPI CSI-2 DPHY mbus configuration
> > > > > > > > + * @data_lanes: number of data lanes in use
> > > > > > > > + * @clock_noncontinuous: non continuous clock enable flag
> > > > > > > > + */
> > > > > > > > +struct v4l2_mbus_csi2_dphy_config {
> > > > > > > > +	unsigned int data_lanes : 3;
> > > > > > > > +	bool clock_noncontinuous : 1;
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * struct v4l2_mbus_csi2_cphy_config - MIPI CSI-2 CPHY mbus configuration
> > > > > > > > + *
> > > > > > > > + * TODO
> > > > > > > > + */
> > > > > > > > +struct v4l2_mbus_csi2_cphy_config {
> > > > > > > > +	/* TODO */
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * struct v4l2_mbus_pad_config - media bus configuration
> > > > > > > > + *
> > > > > > > > + * Report the subdevice media bus information to inform the caller of the
> > > > > > > > + * current bus configuration. The structure describes bus configuration
> > > > > > > > + * parameters that might change in-between streaming sessions, in order to allow
> > > > > > > > + * the caller to adjust its media bus configuration to match what is reported
> > > > > > > > + * here.
> > > > > > > > + *
> > > > > > > > + * TODO: add '_pad_' to the name to distinguish this from the structure
> > > > > > > > + * defined in v4l2_mediabus.h used for the same purpose by the g/s_mbus_config
> > > > > > > > + * video operations. Reuse the there defined enum v4l2_mbus_type to define
> > > > > > > > + * the bus type.
> > > > > > > > + *
> > > > > > > > + * @type: mbus type. See &enum v4l2_mbus_type
> > > > > > > > + * @parallel: parallel bus configuration parameters.
> > > > > > > > + *	      See &struct v4l2_mbus_parallel_config
> > > > > > > > + * @csi2_dphy: MIPI CSI-2 DPHY configuration parameters
> > > > > > > > + *	       See &struct v4l2_mbus_csi2_dphy_config
> > > > > > > > + * @csi2_cphy: MIPI CSI-2 CPHY configuration parameters
> > > > > > > > + *	       See &struct v4l2_mbus_csi2_cphy_config
> > > > > > > > + */
> > > > > > > > +struct v4l2_mbus_pad_config {
> > > > > > > > +	enum v4l2_mbus_type type;
> > > > > > > > +	union {
> > > > > > > > +		struct v4l2_mbus_parallel_config parallel;
> > > > > > > > +		struct v4l2_mbus_csi2_dphy_config csi2_dphy;
> > > > > > > > +		struct v4l2_mbus_csi2_cphy_config csi2_cphy;
> > > > > > > > +	};
> > > > > > > > +};
> > > > > > > > +
> > > > > > > >  /**
> > > > > > > >   * struct v4l2_subdev_video_ops - Callbacks used when v4l device was opened
> > > > > > > >   *				  in video mode.
> > > > > > > > @@ -670,6 +734,7 @@ struct v4l2_subdev_pad_config {
> > > > > > > >   *
> > > > > > > >   * @set_frame_desc: set the low level media bus frame parameters, @fd array
> > > > > > > >   *                  may be adjusted by the subdev driver to device capabilities.
> > > > > > > > + * @get_mbus_config: get the current mbus configuration
> > > > > > > >   */
> > > > > > > >  struct v4l2_subdev_pad_ops {
> > > > > > > >  	int (*init_cfg)(struct v4l2_subdev *sd,
> > > > > > > > @@ -710,6 +775,8 @@ struct v4l2_subdev_pad_ops {
> > > > > > > >  			      struct v4l2_mbus_frame_desc *fd);
> > > > > > > >  	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
> > > > > > > >  			      struct v4l2_mbus_frame_desc *fd);
> > > > > > > > +	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > > > > > > > +			       struct v4l2_mbus_pad_config *config);
> > > > > > >
> > > > > > > Because this can be used in many different ways, there's more chance it can
> > > > > > > be misused. That means, drivers call this in different locations, ex probe,
> > > > > > > get format, start stream,,,, and on differnt pads, src or sink. So imagine
> > > > > > > one set of drivers call on sink pad, and the other set call on source pad.
> > > > > > > It works well only until those are mixed together.
> > > > >
> > > > > I don't think we can right now establish all possible use cases, or
> > > > > prevent people from shooting in their foot, moreover, the 'right'
> > > > > usage really depends on the bus in use, and I can't tell where this is
> > > > > will be used in the wild...
> > > > >
> > > > > > That subdevice operations can be called at both probe and s_stream() is
> > > > > > nothing new, I don't thin this is a new problem. But I agree maybe we
> > > > > > could limit get_mbus_config() in the core to only be valid four source
> > > > > > pads? Apart from this open question I think this patch looks good.
> > > > >
> > > > > I'm a bit skeptical on limiting this to source pads as, again, this
> > > > > really depends on the bus on which this operation is used. For my
> > > > > limited knowledge, yes, the use case is always the receiver quering
> > > > > the transmitter, but I don't feel like ruling out the opposite.
> > > > >
> > > > > > > So wouldn't it be better to put some restrictions? One is to document
> > > > > > > recommendations. I think this better be called in stream on because
> > > > > > > some bus config may change at runtime depending on other configuration.
> > > > > > > So any bus config prior to stream-on may be outdated. The other is to
> > > > > > > enforce in the code. Some, but maybe not all, can be handled in
> > > > > > > v4l2_subdev_call_pad_wrappers, for example allowing this call only on
> > > > > > > source pad.
> > > > >
> > > > > I hear your concern, but I think it really depends on the use cases
> > > > > and I would have an hard time to provide recommendations that
> > > > > address all use cases.
> > > > >
> > > > > Is your concern due to some mis-uses example you can describe ?
> > > >
> > > > Yeah, while trying this out, I was thinking how it should be used. I ended
> > > > up with a specific way: single direction, starting from stream on,
> > > >
> > > > (streamon) -> max9286 -> (g_mbus_conf) -> max96705 -> (g_mbus_conf) -> sensor1
> > > >
> > > > It's this way because max96705 doesn't have own vsync polarity, and it
> > > > should get it from the connected sensor. While someone may implement the same
> > > > in complete opposite direction for another set of drivers, starting from
> > > > sensor, or even in different entry point,
> > >
> > > I agree with the use-case above.
> > >
> > > >
> > > > (s_fmt) -> sensor2 -> (g_mbus_conf) -> some_ser -> (g_mbus_conf) -> some_des
> > > >
> > > > When the sensor2 driver is used with max96705 above, there could be a problem
> > > > such as circular calls or getting an outdated value. And it is harder to fix at
> > > > that point. So I thought enforcing the direction works for current use cases
> > > > (under my visilbity), and may help avoid such issue in future. Probably it may
> > > > be just me over-thinking, as it sounds like? :-)
> > >
> > > If a format is set on a subdevice we are operating on a subdevice that
> > > is part of a media device right? If so shall setting the format of the
> > > different entities of the graph involve cross entry calls? Shall not the
> > > entire pipeline format be validated at stream_start() and that is the
> > > time g_mbus_conf() would be involved like in the first case above. I
> > > might have misunderstood something if so I apologize.
> >
> > In this patch, it's fully up to driver implementation, so it's legitimate
> > if some driver decides to call that in subdev set format and call another
> > get_mbus_config() within it.
>
> I share some of the concern that was expressed on this topic. The V4L2
> subdev in-kernel API is full of operations that are specified as
> generic, and are called at different times and different ways by
> different drivers. The different subdevs then implement those operations
> differently, as they're only tested with one or a small subset of V4L2
> drivers, and we end up with subdevs that implement different and
> incompatible behaviours. I think the use cases need to be considered
> here, and we should specify the usage of this API in details.
>

I'm a bit skeptical on the fact we can rule out all usage cases and
properyl capture them with a comment, but if this is felt like a
pressing matter we could add a few hints. I wonder why this operation
is different than the othera kapi-only  pad operations we have
already.

If you all agree this should be limited to fetching information from
source pads (which for all the use cases I know of is true, but I
don't know all the possible use cases) this can be captured, but I
would have an hard time imposing when this should be used, as each
bus/driver is different in requirements and implementation.


> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 2/4] media: v4l2-subdv: Introduce get_mbus_config pad op

[Laurent Pinchart]

Hi Jacopo,

On Wed, Apr 15, 2020 at 09:43:31AM +0200, Jacopo Mondi wrote:
> On Wed, Apr 15, 2020 at 05:22:20AM +0300, Laurent Pinchart wrote:
> > On Fri, Apr 10, 2020 at 12:26:26PM -0700, Hyun Kwon wrote:
> >> On Fri, 2020-04-10 at 03:24:06 -0700, niklas.soderlund+renesas@ragnatech.se wrote:
> >>> On 2020-04-09 17:30:28 -0700, Hyun Kwon wrote:
> >>>> On Thu, 2020-04-09 at 00:35:07 -0700, Jacopo Mondi wrote:
> >>>>> Hi Niklas, Huyn,
> >>>>> On Wed, Apr 08, 2020 at 12:22:55AM +0200, niklas.soderlund+renesas@ragnatech.se wrote:
> >>>>>> On 2020-04-01 15:30:38 -0700, Hyun Kwon wrote:
> >>>>>>> On Fri, 2020-03-13 at 07:40:33 -0700, Jacopo Mondi wrote:
> >>>>>>>> Introduce a new pad operation to allow retrieving the media bus
> >>>>>>>> configuration on a subdevice pad.
> >>>>>>>>
> >>>>>>>> The newly introduced operation reassembles the s/g_mbus_config video
> >>>>>>>> operation, which have been on their way to be deprecated since a long
> >>>>>>>> time.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>>>>>>> ---
> >>>>>>>>  include/media/v4l2-subdev.h | 67 +++++++++++++++++++++++++++++++++++++
> >>>>>>>>  1 file changed, 67 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >>>>>>>> index 761aa83a3f3c..3a1afc00e094 100644
> >>>>>>>> --- a/include/media/v4l2-subdev.h
> >>>>>>>> +++ b/include/media/v4l2-subdev.h
> >>>>>>>> @@ -350,6 +350,70 @@ struct v4l2_mbus_frame_desc {
> >>>>>>>>  	unsigned short num_entries;
> >>>>>>>>  };
> >>>>>>>>
> >>>>>>>> +/**
> >>>>>>>> + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> >>>>>>>> + * @hsync_active: hsync active state: true for high, false for low
> >>>>>>>> + * @vsync_active: vsync active state: true for high, false for low
> >>>>>>>> + * @pclk_rising: pixel clock active edge: true for rising, false for falling
> >>>>>>>> + * @data_active: data lines active state: true for high, false for low
> >>>>>>>> + */
> >>>>>>>> +struct v4l2_mbus_parallel_config {
> >>>>>>>> +	bool hsync_active : 1;
> >>>>>>>> +	bool vsync_active : 1;
> >>>>>>>> +	bool pclk_rising : 1;
> >>>>>>>> +	bool data_active : 1;
> >>>>>>>> +};
> >>>>>>>> +
> >>>>>>>> +/**
> >>>>>>>> + * struct v4l2_mbus_csi2_dphy_config - MIPI CSI-2 DPHY mbus configuration
> >>>>>>>> + * @data_lanes: number of data lanes in use
> >>>>>>>> + * @clock_noncontinuous: non continuous clock enable flag
> >>>>>>>> + */
> >>>>>>>> +struct v4l2_mbus_csi2_dphy_config {
> >>>>>>>> +	unsigned int data_lanes : 3;
> >>>>>>>> +	bool clock_noncontinuous : 1;
> >>>>>>>> +};
> >>>>>>>> +
> >>>>>>>> +/**
> >>>>>>>> + * struct v4l2_mbus_csi2_cphy_config - MIPI CSI-2 CPHY mbus configuration
> >>>>>>>> + *
> >>>>>>>> + * TODO
> >>>>>>>> + */
> >>>>>>>> +struct v4l2_mbus_csi2_cphy_config {
> >>>>>>>> +	/* TODO */
> >>>>>>>> +};
> >>>>>>>> +
> >>>>>>>> +/**
> >>>>>>>> + * struct v4l2_mbus_pad_config - media bus configuration
> >>>>>>>> + *
> >>>>>>>> + * Report the subdevice media bus information to inform the caller of the
> >>>>>>>> + * current bus configuration. The structure describes bus configuration
> >>>>>>>> + * parameters that might change in-between streaming sessions, in order to allow
> >>>>>>>> + * the caller to adjust its media bus configuration to match what is reported
> >>>>>>>> + * here.
> >>>>>>>> + *
> >>>>>>>> + * TODO: add '_pad_' to the name to distinguish this from the structure
> >>>>>>>> + * defined in v4l2_mediabus.h used for the same purpose by the g/s_mbus_config
> >>>>>>>> + * video operations. Reuse the there defined enum v4l2_mbus_type to define
> >>>>>>>> + * the bus type.
> >>>>>>>> + *
> >>>>>>>> + * @type: mbus type. See &enum v4l2_mbus_type
> >>>>>>>> + * @parallel: parallel bus configuration parameters.
> >>>>>>>> + *	      See &struct v4l2_mbus_parallel_config
> >>>>>>>> + * @csi2_dphy: MIPI CSI-2 DPHY configuration parameters
> >>>>>>>> + *	       See &struct v4l2_mbus_csi2_dphy_config
> >>>>>>>> + * @csi2_cphy: MIPI CSI-2 CPHY configuration parameters
> >>>>>>>> + *	       See &struct v4l2_mbus_csi2_cphy_config
> >>>>>>>> + */
> >>>>>>>> +struct v4l2_mbus_pad_config {
> >>>>>>>> +	enum v4l2_mbus_type type;
> >>>>>>>> +	union {
> >>>>>>>> +		struct v4l2_mbus_parallel_config parallel;
> >>>>>>>> +		struct v4l2_mbus_csi2_dphy_config csi2_dphy;
> >>>>>>>> +		struct v4l2_mbus_csi2_cphy_config csi2_cphy;
> >>>>>>>> +	};
> >>>>>>>> +};
> >>>>>>>> +
> >>>>>>>>  /**
> >>>>>>>>   * struct v4l2_subdev_video_ops - Callbacks used when v4l device was opened
> >>>>>>>>   *				  in video mode.
> >>>>>>>> @@ -670,6 +734,7 @@ struct v4l2_subdev_pad_config {
> >>>>>>>>   *
> >>>>>>>>   * @set_frame_desc: set the low level media bus frame parameters, @fd array
> >>>>>>>>   *                  may be adjusted by the subdev driver to device capabilities.
> >>>>>>>> + * @get_mbus_config: get the current mbus configuration
> >>>>>>>>   */
> >>>>>>>>  struct v4l2_subdev_pad_ops {
> >>>>>>>>  	int (*init_cfg)(struct v4l2_subdev *sd,
> >>>>>>>> @@ -710,6 +775,8 @@ struct v4l2_subdev_pad_ops {
> >>>>>>>>  			      struct v4l2_mbus_frame_desc *fd);
> >>>>>>>>  	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
> >>>>>>>>  			      struct v4l2_mbus_frame_desc *fd);
> >>>>>>>> +	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> >>>>>>>> +			       struct v4l2_mbus_pad_config *config);
> >>>>>>>
> >>>>>>> Because this can be used in many different ways, there's more chance it can
> >>>>>>> be misused. That means, drivers call this in different locations, ex probe,
> >>>>>>> get format, start stream,,,, and on differnt pads, src or sink. So imagine
> >>>>>>> one set of drivers call on sink pad, and the other set call on source pad.
> >>>>>>> It works well only until those are mixed together.
> >>>>>
> >>>>> I don't think we can right now establish all possible use cases, or
> >>>>> prevent people from shooting in their foot, moreover, the 'right'
> >>>>> usage really depends on the bus in use, and I can't tell where this is
> >>>>> will be used in the wild...
> >>>>>
> >>>>>> That subdevice operations can be called at both probe and s_stream() is
> >>>>>> nothing new, I don't thin this is a new problem. But I agree maybe we
> >>>>>> could limit get_mbus_config() in the core to only be valid four source
> >>>>>> pads? Apart from this open question I think this patch looks good.
> >>>>>
> >>>>> I'm a bit skeptical on limiting this to source pads as, again, this
> >>>>> really depends on the bus on which this operation is used. For my
> >>>>> limited knowledge, yes, the use case is always the receiver quering
> >>>>> the transmitter, but I don't feel like ruling out the opposite.
> >>>>>
> >>>>>>> So wouldn't it be better to put some restrictions? One is to document
> >>>>>>> recommendations. I think this better be called in stream on because
> >>>>>>> some bus config may change at runtime depending on other configuration.
> >>>>>>> So any bus config prior to stream-on may be outdated. The other is to
> >>>>>>> enforce in the code. Some, but maybe not all, can be handled in
> >>>>>>> v4l2_subdev_call_pad_wrappers, for example allowing this call only on
> >>>>>>> source pad.
> >>>>>
> >>>>> I hear your concern, but I think it really depends on the use cases
> >>>>> and I would have an hard time to provide recommendations that
> >>>>> address all use cases.
> >>>>>
> >>>>> Is your concern due to some mis-uses example you can describe ?
> >>>>
> >>>> Yeah, while trying this out, I was thinking how it should be used. I ended
> >>>> up with a specific way: single direction, starting from stream on,
> >>>>
> >>>> (streamon) -> max9286 -> (g_mbus_conf) -> max96705 -> (g_mbus_conf) -> sensor1
> >>>>
> >>>> It's this way because max96705 doesn't have own vsync polarity, and it
> >>>> should get it from the connected sensor. While someone may implement the same
> >>>> in complete opposite direction for another set of drivers, starting from
> >>>> sensor, or even in different entry point,
> >>>
> >>> I agree with the use-case above.
> >>>
> >>>>
> >>>> (s_fmt) -> sensor2 -> (g_mbus_conf) -> some_ser -> (g_mbus_conf) -> some_des
> >>>>
> >>>> When the sensor2 driver is used with max96705 above, there could be a problem
> >>>> such as circular calls or getting an outdated value. And it is harder to fix at
> >>>> that point. So I thought enforcing the direction works for current use cases
> >>>> (under my visilbity), and may help avoid such issue in future. Probably it may
> >>>> be just me over-thinking, as it sounds like? :-)
> >>>
> >>> If a format is set on a subdevice we are operating on a subdevice that
> >>> is part of a media device right? If so shall setting the format of the
> >>> different entities of the graph involve cross entry calls? Shall not the
> >>> entire pipeline format be validated at stream_start() and that is the
> >>> time g_mbus_conf() would be involved like in the first case above. I
> >>> might have misunderstood something if so I apologize.
> >>
> >> In this patch, it's fully up to driver implementation, so it's legitimate
> >> if some driver decides to call that in subdev set format and call another
> >> get_mbus_config() within it.
> >
> > I share some of the concern that was expressed on this topic. The V4L2
> > subdev in-kernel API is full of operations that are specified as
> > generic, and are called at different times and different ways by
> > different drivers. The different subdevs then implement those operations
> > differently, as they're only tested with one or a small subset of V4L2
> > drivers, and we end up with subdevs that implement different and
> > incompatible behaviours. I think the use cases need to be considered
> > here, and we should specify the usage of this API in details.
> 
> I'm a bit skeptical on the fact we can rule out all usage cases and
> properyl capture them with a comment, but if this is felt like a
> pressing matter we could add a few hints. I wonder why this operation
> is different than the othera kapi-only  pad operations we have
> already.

It shouldn't be different, all operations should have their usage
properly documented :-)

> If you all agree this should be limited to fetching information from
> source pads (which for all the use cases I know of is true, but I
> don't know all the possible use cases) this can be captured, but I
> would have an hard time imposing when this should be used, as each
> bus/driver is different in requirements and implementation.

I don't mind either way, I won't push against this series just because
of that, as far as I'm concerned, it's the whole subdev API that's
problematic, not documenting use cases here won't make anything worse.

-- 
Regards,

Laurent Pinchart