[RFC PATCH 0/4] use v4l2_get_link_freq() in CSI receiver drivers

[RFC PATCH 0/4] use v4l2_get_link_freq() in CSI receiver drivers

[Andrey Konovalov]

This patchset continues the work discussed in the "[RFC] Repurpose
V4L2_CID_PIXEL_RATE for the sampling rate in the pixel array" thread [1].

This patchset follows up the patchset for camss driver [2] of the same
purpose, and makes the rest of the receiver drivers in drivers/media
and drivers/staging/media to use v4l2_get_link_freq() instead of
V4L2_CID_PIXEL_RATE.

This patchset has been build tested only, as I don't have access to the
hardware. Most of the changes are fairly straightforward except for the
ones in the omap4iss driver. I would appreciate if people with better
knowledge of the omap4iss driver reviewed the last patch in this series,
and checked if I got the logic in the driver right.

[1] https://www.spinics.net/lists/linux-media/msg183183.html
[2] "[PATCH v3 0/3] media: qcom: camss: V4L2_CID_PIXEL_RATE/LINK_FREQ fixes"
    posted earlier today

[RFC PATCH 1/4] media: rcar-vin: use v4l2_get_link_freq() to calculate phypll frequency

[Andrey Konovalov]

To get the link frequency value, or to calculate a parameter depending on
it the receiver driver should use V4L2_CID_LINK_FREQ. If V4L2_CID_LINK_FREQ
control is not implemented in the remote subdevice, the link frequency
can be calculated from V4L2_CID_PIXEL_RATE control value. But the latter
may not give the correct link frequency, and should only be used as the
last resort. v4l2_get_link_freq() does exactly that, so use it instead
of reading V4L2_CID_PIXEL_RATE directly.

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index e06cd512aba2..eec8f9dd9060 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -455,29 +455,25 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp,
 			   unsigned int lanes)
 {
 	struct v4l2_subdev *source;
-	struct v4l2_ctrl *ctrl;
-	u64 mbps;
+	s64 mbps;
 
 	if (!priv->remote)
 		return -ENODEV;
 
 	source = priv->remote;
 
-	/* Read the pixel rate control from remote. */
-	ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
-	if (!ctrl) {
-		dev_err(priv->dev, "no pixel rate control in subdev %s\n",
+	/* Read the link frequency from the remote subdev. */
+	mbps = v4l2_get_link_freq(source->ctrl_handler, bpp, 2 * lanes);
+	if (mbps < 0) {
+		dev_err(priv->dev, "failed to get link rate from subdev %s\n",
 			source->name);
-		return -EINVAL;
+		return mbps;
 	}
-
 	/*
 	 * Calculate the phypll in mbps.
-	 * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes)
 	 * bps = link_freq * 2
 	 */
-	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
-	do_div(mbps, lanes * 1000000);
+	do_div(mbps, 1000000 / 2);
 
 	return mbps;
 }
-- 
2.17.1

[RFC PATCH 2/4] media: ti-vpe: cal: use v4l2_get_link_freq() for DPHY timing configuration

[Andrey Konovalov]

To configure DPHY properly the driver needs to know CSI-2 link frequency.
Instead of calculating it from the value of V4L2_CID_PIXEL_RATE control
(which can give wrong link frequency value for some sensors) call
v4l2_get_link_freq(). It uses V4L2_CID_LINK_FREQ if this control is
implemented in the sensor driver, and falls back to calculating it from
V4L2_CID_PIXEL_RATE otherwise.

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 drivers/media/platform/ti-vpe/cal-camerarx.c | 47 +++++++-------------
 1 file changed, 17 insertions(+), 30 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c
index dd48017859cd..0c1046a1fea4 100644
--- a/drivers/media/platform/ti-vpe/cal-camerarx.c
+++ b/drivers/media/platform/ti-vpe/cal-camerarx.c
@@ -45,22 +45,20 @@ static inline void camerarx_write(struct cal_camerarx *phy, u32 offset, u32 val)
  * ------------------------------------------------------------------
  */
 
-static s64 cal_camerarx_get_external_rate(struct cal_camerarx *phy)
+static s64 cal_camerarx_get_link_freq(struct cal_camerarx *phy)
 {
-	struct v4l2_ctrl *ctrl;
-	s64 rate;
+	u32 num_lanes = phy->endpoint.bus.mipi_csi2.num_data_lanes;
+	s64 freq;
 
-	ctrl = v4l2_ctrl_find(phy->sensor->ctrl_handler, V4L2_CID_PIXEL_RATE);
-	if (!ctrl) {
-		phy_err(phy, "no pixel rate control in subdev: %s\n",
+	freq = v4l2_get_link_freq(phy->sensor->ctrl_handler, phy->fmtinfo->bpp,
+				  num_lanes * 2);
+	if (freq < 0)
+		phy_err(phy, "failed to get link frequency from subdev: %s\n",
 			phy->sensor->name);
-		return -EPIPE;
-	}
-
-	rate = v4l2_ctrl_g_ctrl_int64(ctrl);
-	phy_dbg(3, phy, "sensor Pixel Rate: %llu\n", rate);
+	else
+		phy_dbg(3, phy, "sensor link frequency: %lld\n", freq);
 
-	return rate;
+	return freq;
 }
 
 static void cal_camerarx_lane_config(struct cal_camerarx *phy)
@@ -116,25 +114,14 @@ void cal_camerarx_disable(struct cal_camerarx *phy)
 #define TCLK_MISS	1
 #define TCLK_SETTLE	14
 
-static void cal_camerarx_config(struct cal_camerarx *phy, s64 external_rate)
+static void cal_camerarx_config(struct cal_camerarx *phy, s64 link_freq)
 {
 	unsigned int reg0, reg1;
 	unsigned int ths_term, ths_settle;
 	unsigned int csi2_ddrclk_khz;
-	struct v4l2_fwnode_bus_mipi_csi2 *mipi_csi2 =
-			&phy->endpoint.bus.mipi_csi2;
-	u32 num_lanes = mipi_csi2->num_data_lanes;
 
 	/* DPHY timing configuration */
-
-	/*
-	 * CSI-2 is DDR and we only count used lanes.
-	 *
-	 * csi2_ddrclk_khz = external_rate / 1000
-	 *		   / (2 * num_lanes) * phy->fmtinfo->bpp;
-	 */
-	csi2_ddrclk_khz = div_s64(external_rate * phy->fmtinfo->bpp,
-				  2 * num_lanes * 1000);
+	csi2_ddrclk_khz = div_s64(link_freq, 1000);
 
 	phy_dbg(1, phy, "csi2_ddrclk_khz: %d\n", csi2_ddrclk_khz);
 
@@ -270,14 +257,14 @@ static void cal_camerarx_ppi_disable(struct cal_camerarx *phy)
 
 static int cal_camerarx_start(struct cal_camerarx *phy)
 {
-	s64 external_rate;
+	s64 link_freq;
 	u32 sscounter;
 	u32 val;
 	int ret;
 
-	external_rate = cal_camerarx_get_external_rate(phy);
-	if (external_rate < 0)
-		return external_rate;
+	link_freq = cal_camerarx_get_link_freq(phy);
+	if (link_freq < 0)
+		return link_freq;
 
 	ret = v4l2_subdev_call(phy->sensor, core, s_power, 1);
 	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV) {
@@ -325,7 +312,7 @@ static int cal_camerarx_start(struct cal_camerarx *phy)
 	camerarx_read(phy, CAL_CSI2_PHY_REG0);
 
 	/* Program the PHY timing parameters. */
-	cal_camerarx_config(phy, external_rate);
+	cal_camerarx_config(phy, link_freq);
 
 	/*
 	 *    b. Assert the FORCERXMODE signal.
-- 
2.17.1

[RFC PATCH 3/4] media: st-mipid02: use v4l2_get_link_freq() instead of the custom code

[Andrey Konovalov]

v4l2_get_link_freq() uses the same approach as the one implemented
in the current driver with mipid02_get_link_freq_from_cid_link_freq()
and mipid02_get_link_freq_from_cid_pixel_rate().

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 drivers/media/i2c/st-mipid02.c | 58 ++++++----------------------------
 1 file changed, 10 insertions(+), 48 deletions(-)

diff --git a/drivers/media/i2c/st-mipid02.c b/drivers/media/i2c/st-mipid02.c
index 7f07ef56fbbd..38bb18c48ac7 100644
--- a/drivers/media/i2c/st-mipid02.c
+++ b/drivers/media/i2c/st-mipid02.c
@@ -343,41 +343,6 @@ static int mipid02_detect(struct mipid02_dev *bridge)
 	return mipid02_read_reg(bridge, MIPID02_CLK_LANE_WR_REG1, &reg);
 }
 
-static u32 mipid02_get_link_freq_from_cid_link_freq(struct mipid02_dev *bridge,
-						    struct v4l2_subdev *subdev)
-{
-	struct v4l2_querymenu qm = {.id = V4L2_CID_LINK_FREQ, };
-	struct v4l2_ctrl *ctrl;
-	int ret;
-
-	ctrl = v4l2_ctrl_find(subdev->ctrl_handler, V4L2_CID_LINK_FREQ);
-	if (!ctrl)
-		return 0;
-	qm.index = v4l2_ctrl_g_ctrl(ctrl);
-
-	ret = v4l2_querymenu(subdev->ctrl_handler, &qm);
-	if (ret)
-		return 0;
-
-	return qm.value;
-}
-
-static u32 mipid02_get_link_freq_from_cid_pixel_rate(struct mipid02_dev *bridge,
-						     struct v4l2_subdev *subdev)
-{
-	struct v4l2_fwnode_endpoint *ep = &bridge->rx;
-	struct v4l2_ctrl *ctrl;
-	u32 pixel_clock;
-	u32 bpp = bpp_from_code(bridge->fmt.code);
-
-	ctrl = v4l2_ctrl_find(subdev->ctrl_handler, V4L2_CID_PIXEL_RATE);
-	if (!ctrl)
-		return 0;
-	pixel_clock = v4l2_ctrl_g_ctrl_int64(ctrl);
-
-	return pixel_clock * bpp / (2 * ep->bus.mipi_csi2.num_data_lanes);
-}
-
 /*
  * We need to know link frequency to setup clk_lane_reg1 timings. Link frequency
  * will be computed using connected device V4L2_CID_PIXEL_RATE, bit per pixel
@@ -386,21 +351,18 @@ static u32 mipid02_get_link_freq_from_cid_pixel_rate(struct mipid02_dev *bridge,
 static int mipid02_configure_from_rx_speed(struct mipid02_dev *bridge)
 {
 	struct i2c_client *client = bridge->i2c_client;
-	struct v4l2_subdev *subdev = bridge->s_subdev;
-	u32 link_freq;
-
-	link_freq = mipid02_get_link_freq_from_cid_link_freq(bridge, subdev);
-	if (!link_freq) {
-		link_freq = mipid02_get_link_freq_from_cid_pixel_rate(bridge,
-								      subdev);
-		if (!link_freq) {
-			dev_err(&client->dev, "Failed to get link frequency");
-			return -EINVAL;
-		}
+	s64 freq;
+
+	freq = v4l2_get_link_freq(bridge->s_subdev->ctrl_handler,
+				  bpp_from_code(bridge->fmt.code),
+				  2 * bridge->rx.bus.mipi_csi2.num_data_lanes);
+	if (freq < 0) {
+		dev_err(&client->dev, "Failed to get link frequency");
+		return -EINVAL;
 	}
 
-	dev_dbg(&client->dev, "detect link_freq = %d Hz", link_freq);
-	bridge->r.clk_lane_reg1 |= (2000000000 / link_freq) << 2;
+	dev_dbg(&client->dev, "detect link_freq = %lld Hz", freq);
+	bridge->r.clk_lane_reg1 |= (2000000000 / (u32)freq) << 2;
 
 	return 0;
 }
-- 
2.17.1

[RFC PATCH 4/4] staging: media: omap4iss: use v4l2_get_link_freq() to get the external rate

[Andrey Konovalov]

This driver uses V4L2_CID_PIXEL_RATE to calculate the CSI2 link frequency,
but this may give incorrect result in some cases. Use v4l2_get_link_freq()
instead.

Also the driver used the external_rate field in struct iss_pipeline as a
flag to prevent excessive v4l2_subdev_call's when processing the frames
in single-shot mode. Replace the external_rate with external_lfreq, and
use external_bpp and external_lfreq to call v4l2_subdev_call(get_fmt) and
v4l2_get_link_freq() respectively only once per iss_video_streamon().

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 drivers/staging/media/omap4iss/iss.c        | 12 +-----------
 drivers/staging/media/omap4iss/iss_csiphy.c | 19 ++++++++++++++++---
 drivers/staging/media/omap4iss/iss_video.c  |  2 +-
 drivers/staging/media/omap4iss/iss_video.h  |  2 +-
 4 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
index dae9073e7d3c..0eb7b1b5dcc4 100644
--- a/drivers/staging/media/omap4iss/iss.c
+++ b/drivers/staging/media/omap4iss/iss.c
@@ -131,7 +131,7 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,
 	if (!pipe->external)
 		return 0;
 
-	if (pipe->external_rate)
+	if (pipe->external_bpp)
 		return 0;
 
 	memset(&fmt, 0, sizeof(fmt));
@@ -145,16 +145,6 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,
 
 	pipe->external_bpp = omap4iss_video_format_info(fmt.format.code)->bpp;
 
-	ctrl = v4l2_ctrl_find(pipe->external->ctrl_handler,
-			      V4L2_CID_PIXEL_RATE);
-	if (!ctrl) {
-		dev_warn(iss->dev, "no pixel rate control in subdev %s\n",
-			 pipe->external->name);
-		return -EPIPE;
-	}
-
-	pipe->external_rate = v4l2_ctrl_g_ctrl_int64(ctrl);
-
 	return 0;
 }
 
diff --git a/drivers/staging/media/omap4iss/iss_csiphy.c b/drivers/staging/media/omap4iss/iss_csiphy.c
index 96f2ce045138..cec0cd21f7e0 100644
--- a/drivers/staging/media/omap4iss/iss_csiphy.c
+++ b/drivers/staging/media/omap4iss/iss_csiphy.c
@@ -119,6 +119,7 @@ int omap4iss_csiphy_config(struct iss_device *iss,
 	struct iss_pipeline *pipe = to_iss_pipeline(&csi2_subdev->entity);
 	struct iss_v4l2_subdevs_group *subdevs = pipe->external->host_priv;
 	struct iss_csiphy_dphy_cfg csi2phy;
+	s64 link_freq;
 	int csi2_ddrclk_khz;
 	struct iss_csiphy_lanes_cfg *lanes;
 	unsigned int used_lanes = 0;
@@ -193,9 +194,21 @@ int omap4iss_csiphy_config(struct iss_device *iss,
 	if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos))
 		return -EINVAL;
 
-	csi2_ddrclk_khz = pipe->external_rate / 1000
-		/ (2 * csi2->phy->used_data_lanes)
-		* pipe->external_bpp;
+	if (!pipe->external_lfreq) {
+		link_freq = v4l2_get_link_freq(pipe->external->ctrl_handler,
+					       pipe->external_bpp,
+					       2 * csi2->phy->used_data_lanes);
+		if (link_freq < 0) {
+			dev_warn(iss->dev,
+				 "failed to read the link frequency fromn subdev %s\n",
+				 pipe->external->name);
+			return -EINVAL;
+		}
+
+		pipe->external_lfreq = link_freq;
+	}
+
+	csi2_ddrclk_khz = div_s64(pipe->external_lfreq, 1000);
 
 	/*
 	 * THS_TERM: Programmed value = ceil(12.5 ns/DDRClk period) - 1.
diff --git a/drivers/staging/media/omap4iss/iss_video.c b/drivers/staging/media/omap4iss/iss_video.c
index 66975a37dc85..a654c8d18bbc 100644
--- a/drivers/staging/media/omap4iss/iss_video.c
+++ b/drivers/staging/media/omap4iss/iss_video.c
@@ -872,7 +872,7 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	pipe = entity->pipe
 	     ? to_iss_pipeline(entity) : &video->pipe;
 	pipe->external = NULL;
-	pipe->external_rate = 0;
+	pipe->external_lfreq = 0;
 	pipe->external_bpp = 0;
 
 	ret = media_entity_enum_init(&pipe->ent_enum, entity->graph_obj.mdev);
diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h
index 526281bf0051..2ad5c8483958 100644
--- a/drivers/staging/media/omap4iss/iss_video.h
+++ b/drivers/staging/media/omap4iss/iss_video.h
@@ -86,7 +86,7 @@ struct iss_pipeline {
 	bool error;
 	struct v4l2_fract max_timeperframe;
 	struct v4l2_subdev *external;
-	unsigned int external_rate;
+	s64 external_lfreq;
 	int external_bpp;
 };
 
-- 
2.17.1

Re: [RFC PATCH 4/4] staging: media: omap4iss: use v4l2_get_link_freq() to get the external rate

[Sakari Ailus]

Hi Andrey,

Thanks for the set.

On Wed, Mar 03, 2021 at 09:08:17PM +0300, Andrey Konovalov wrote:
> This driver uses V4L2_CID_PIXEL_RATE to calculate the CSI2 link frequency,
> but this may give incorrect result in some cases. Use v4l2_get_link_freq()
> instead.
> 
> Also the driver used the external_rate field in struct iss_pipeline as a
> flag to prevent excessive v4l2_subdev_call's when processing the frames
> in single-shot mode. Replace the external_rate with external_lfreq, and
> use external_bpp and external_lfreq to call v4l2_subdev_call(get_fmt) and
> v4l2_get_link_freq() respectively only once per iss_video_streamon().
> 
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> ---
>  drivers/staging/media/omap4iss/iss.c        | 12 +-----------
>  drivers/staging/media/omap4iss/iss_csiphy.c | 19 ++++++++++++++++---
>  drivers/staging/media/omap4iss/iss_video.c  |  2 +-
>  drivers/staging/media/omap4iss/iss_video.h  |  2 +-
>  4 files changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
> index dae9073e7d3c..0eb7b1b5dcc4 100644
> --- a/drivers/staging/media/omap4iss/iss.c
> +++ b/drivers/staging/media/omap4iss/iss.c
> @@ -131,7 +131,7 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,
>  	if (!pipe->external)
>  		return 0;
>  
> -	if (pipe->external_rate)
> +	if (pipe->external_bpp)
>  		return 0;
>  
>  	memset(&fmt, 0, sizeof(fmt));
> @@ -145,16 +145,6 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,
>  
>  	pipe->external_bpp = omap4iss_video_format_info(fmt.format.code)->bpp;
>  
> -	ctrl = v4l2_ctrl_find(pipe->external->ctrl_handler,
> -			      V4L2_CID_PIXEL_RATE);
> -	if (!ctrl) {
> -		dev_warn(iss->dev, "no pixel rate control in subdev %s\n",
> -			 pipe->external->name);
> -		return -EPIPE;
> -	}
> -
> -	pipe->external_rate = v4l2_ctrl_g_ctrl_int64(ctrl);
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/staging/media/omap4iss/iss_csiphy.c b/drivers/staging/media/omap4iss/iss_csiphy.c
> index 96f2ce045138..cec0cd21f7e0 100644
> --- a/drivers/staging/media/omap4iss/iss_csiphy.c
> +++ b/drivers/staging/media/omap4iss/iss_csiphy.c
> @@ -119,6 +119,7 @@ int omap4iss_csiphy_config(struct iss_device *iss,
>  	struct iss_pipeline *pipe = to_iss_pipeline(&csi2_subdev->entity);
>  	struct iss_v4l2_subdevs_group *subdevs = pipe->external->host_priv;
>  	struct iss_csiphy_dphy_cfg csi2phy;
> +	s64 link_freq;
>  	int csi2_ddrclk_khz;
>  	struct iss_csiphy_lanes_cfg *lanes;
>  	unsigned int used_lanes = 0;
> @@ -193,9 +194,21 @@ int omap4iss_csiphy_config(struct iss_device *iss,
>  	if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos))
>  		return -EINVAL;
>  
> -	csi2_ddrclk_khz = pipe->external_rate / 1000
> -		/ (2 * csi2->phy->used_data_lanes)
> -		* pipe->external_bpp;
> +	if (!pipe->external_lfreq) {
> +		link_freq = v4l2_get_link_freq(pipe->external->ctrl_handler,

I wonder if you could this unconditionally, and remove external_lfreq field
altogether. The same could be done for external_bpp but that's out of scope
for this patch.

> +					       pipe->external_bpp,
> +					       2 * csi2->phy->used_data_lanes);
> +		if (link_freq < 0) {
> +			dev_warn(iss->dev,
> +				 "failed to read the link frequency fromn subdev %s\n",
> +				 pipe->external->name);
> +			return -EINVAL;
> +		}
> +
> +		pipe->external_lfreq = link_freq;
> +	}
> +
> +	csi2_ddrclk_khz = div_s64(pipe->external_lfreq, 1000);
>  
>  	/*
>  	 * THS_TERM: Programmed value = ceil(12.5 ns/DDRClk period) - 1.
> diff --git a/drivers/staging/media/omap4iss/iss_video.c b/drivers/staging/media/omap4iss/iss_video.c
> index 66975a37dc85..a654c8d18bbc 100644
> --- a/drivers/staging/media/omap4iss/iss_video.c
> +++ b/drivers/staging/media/omap4iss/iss_video.c
> @@ -872,7 +872,7 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
>  	pipe = entity->pipe
>  	     ? to_iss_pipeline(entity) : &video->pipe;
>  	pipe->external = NULL;
> -	pipe->external_rate = 0;
> +	pipe->external_lfreq = 0;
>  	pipe->external_bpp = 0;
>  
>  	ret = media_entity_enum_init(&pipe->ent_enum, entity->graph_obj.mdev);
> diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h
> index 526281bf0051..2ad5c8483958 100644
> --- a/drivers/staging/media/omap4iss/iss_video.h
> +++ b/drivers/staging/media/omap4iss/iss_video.h
> @@ -86,7 +86,7 @@ struct iss_pipeline {
>  	bool error;
>  	struct v4l2_fract max_timeperframe;
>  	struct v4l2_subdev *external;
> -	unsigned int external_rate;
> +	s64 external_lfreq;
>  	int external_bpp;
>  };
>  

-- 
Sakari Ailus

Re: [RFC PATCH 0/4] use v4l2_get_link_freq() in CSI receiver drivers

[Sakari Ailus]

On Wed, Mar 03, 2021 at 09:08:13PM +0300, Andrey Konovalov wrote:
> This patchset continues the work discussed in the "[RFC] Repurpose
> V4L2_CID_PIXEL_RATE for the sampling rate in the pixel array" thread [1].
> 
> This patchset follows up the patchset for camss driver [2] of the same
> purpose, and makes the rest of the receiver drivers in drivers/media
> and drivers/staging/media to use v4l2_get_link_freq() instead of
> V4L2_CID_PIXEL_RATE.
> 
> This patchset has been build tested only, as I don't have access to the
> hardware. Most of the changes are fairly straightforward except for the
> ones in the omap4iss driver. I would appreciate if people with better
> knowledge of the omap4iss driver reviewed the last patch in this series,
> and checked if I got the logic in the driver right.

To me this looks good, hopefully people could test the set, too.

-- 
Sakari Ailus

Re: [RFC PATCH 1/4] media: rcar-vin: use v4l2_get_link_freq() to calculate phypll frequency

[Niklas Söderlund]

Hi Andrey,

Thanks for your patch.

On 2021-03-03 21:08:14 +0300, Andrey Konovalov wrote:
> To get the link frequency value, or to calculate a parameter depending on
> it the receiver driver should use V4L2_CID_LINK_FREQ. If V4L2_CID_LINK_FREQ
> control is not implemented in the remote subdevice, the link frequency
> can be calculated from V4L2_CID_PIXEL_RATE control value. But the latter
> may not give the correct link frequency, and should only be used as the
> last resort. v4l2_get_link_freq() does exactly that, so use it instead
> of reading V4L2_CID_PIXEL_RATE directly.

I like the direction this patch is taking, but I'm a bit concerned about 
that V4L2_CID_LINK_FREQ is not able to replace V4L2_CID_PIXEL_RATE as it 
is designed today. Maybe my concern is unfounded and only reflects my 
own misunderstanding of the API.

When I wrote this code I tried to first do it using V4L2_CID_LINK_FREQ 
but I found no way to be able to express the wide rang of values needed 
for my use-case given that V4L2_CID_LINK_FREQ is a menu control. I had 
to use V4L2_CID_PIXEL_RATE as it allowed me to at runtime calculate and 
report the link speed based on input formats. The Use-cases I need to 
address are where CSI-2 transmitter themself are a bridge in the video 
pipeline, for example

* Case 1 - HDMI video source

HDMI source -> ADV748x (HDMI-to-CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver

The R-Car CSI-2 receiver needs to know the CSI-2 link frequency and 
queries the ADV748x using V4L2_CID_PIXEL_RATE. The ADV748x reports the 
pixel rate based on the HDMI format detected on its sink pad.

This could be done using V4L2_CID_LINK_FREQ, but as it's a menu control 
it becomes rather tricky to populate it with all possible values, but I 
guess it could be doable?

* Case 2 - Multiple video streams over a CSI-2 bus (GMSL)

Camera 1 -|
Camera 2 -|
Camera 3 -|---> MAX9286 (GMSL-to CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver
Camera 4 -|

The MAX9286 has 4 sink pads each connected to an independent camera and 
a single CSI-2 source pad. When streaming starts the MAX9286 computes 
the total CSI-2 link speed as V4L2_CID_PIXEL_RATE based on the format on 
each of it's 4 sink pads.

As in case 1 this could be reported by V4L2_CID_LINK_FREQ but I don't 
see it as feasible to populate the menu control with all possible 
frequencies before hand.

Hopefully this is all easily solvable and I have only misunderstood how 
menu controls work. If not I think this needs to be considered as part 
of this series as otherwise it could leave the CSI-2 bridge drivers 
without a path forward.

> 
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>

I tested this and it works as expected. Also as expected it prints lots 
of warnings about the usage of V4L2_CID_PIXEL_RATE :-) Once I understand 
how I can fix the CSI-2 transmitters used as bridges in the R-Car boards 
I will be happy to add my tag to this series as well as fix the bridge 
drivers.

> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index e06cd512aba2..eec8f9dd9060 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -455,29 +455,25 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp,
>  			   unsigned int lanes)
>  {
>  	struct v4l2_subdev *source;
> -	struct v4l2_ctrl *ctrl;
> -	u64 mbps;
> +	s64 mbps;
>  
>  	if (!priv->remote)
>  		return -ENODEV;
>  
>  	source = priv->remote;
>  
> -	/* Read the pixel rate control from remote. */
> -	ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
> -	if (!ctrl) {
> -		dev_err(priv->dev, "no pixel rate control in subdev %s\n",
> +	/* Read the link frequency from the remote subdev. */
> +	mbps = v4l2_get_link_freq(source->ctrl_handler, bpp, 2 * lanes);
> +	if (mbps < 0) {
> +		dev_err(priv->dev, "failed to get link rate from subdev %s\n",
>  			source->name);
> -		return -EINVAL;
> +		return mbps;
>  	}
> -
>  	/*
>  	 * Calculate the phypll in mbps.
> -	 * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes)
>  	 * bps = link_freq * 2
>  	 */
> -	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
> -	do_div(mbps, lanes * 1000000);
> +	do_div(mbps, 1000000 / 2);
>  
>  	return mbps;
>  }
> -- 
> 2.17.1
> 

-- 
Regards,
Niklas Söderlund

Re: [RFC PATCH 4/4] staging: media: omap4iss: use v4l2_get_link_freq() to get the external rate

[Andrey Konovalov]

Hi Sakari,

On 05.03.2021 18:41, Sakari Ailus wrote:
> Hi Andrey,
> 
> Thanks for the set.
> 
> On Wed, Mar 03, 2021 at 09:08:17PM +0300, Andrey Konovalov wrote:
>> This driver uses V4L2_CID_PIXEL_RATE to calculate the CSI2 link frequency,
>> but this may give incorrect result in some cases. Use v4l2_get_link_freq()
>> instead.
>>
>> Also the driver used the external_rate field in struct iss_pipeline as a
>> flag to prevent excessive v4l2_subdev_call's when processing the frames
>> in single-shot mode. Replace the external_rate with external_lfreq, and
>> use external_bpp and external_lfreq to call v4l2_subdev_call(get_fmt) and
>> v4l2_get_link_freq() respectively only once per iss_video_streamon().
>>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> ---
>>   drivers/staging/media/omap4iss/iss.c        | 12 +-----------
>>   drivers/staging/media/omap4iss/iss_csiphy.c | 19 ++++++++++++++++---
>>   drivers/staging/media/omap4iss/iss_video.c  |  2 +-
>>   drivers/staging/media/omap4iss/iss_video.h  |  2 +-
>>   4 files changed, 19 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
>> index dae9073e7d3c..0eb7b1b5dcc4 100644
>> --- a/drivers/staging/media/omap4iss/iss.c
>> +++ b/drivers/staging/media/omap4iss/iss.c
>> @@ -131,7 +131,7 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,
>>   	if (!pipe->external)
>>   		return 0;
>>   
>> -	if (pipe->external_rate)
>> +	if (pipe->external_bpp)
>>   		return 0;
>>   
>>   	memset(&fmt, 0, sizeof(fmt));
>> @@ -145,16 +145,6 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,
>>   
>>   	pipe->external_bpp = omap4iss_video_format_info(fmt.format.code)->bpp;
>>   
>> -	ctrl = v4l2_ctrl_find(pipe->external->ctrl_handler,
>> -			      V4L2_CID_PIXEL_RATE);
>> -	if (!ctrl) {
>> -		dev_warn(iss->dev, "no pixel rate control in subdev %s\n",
>> -			 pipe->external->name);
>> -		return -EPIPE;
>> -	}
>> -
>> -	pipe->external_rate = v4l2_ctrl_g_ctrl_int64(ctrl);
>> -
>>   	return 0;
>>   }
>>   
>> diff --git a/drivers/staging/media/omap4iss/iss_csiphy.c b/drivers/staging/media/omap4iss/iss_csiphy.c
>> index 96f2ce045138..cec0cd21f7e0 100644
>> --- a/drivers/staging/media/omap4iss/iss_csiphy.c
>> +++ b/drivers/staging/media/omap4iss/iss_csiphy.c
>> @@ -119,6 +119,7 @@ int omap4iss_csiphy_config(struct iss_device *iss,
>>   	struct iss_pipeline *pipe = to_iss_pipeline(&csi2_subdev->entity);
>>   	struct iss_v4l2_subdevs_group *subdevs = pipe->external->host_priv;
>>   	struct iss_csiphy_dphy_cfg csi2phy;
>> +	s64 link_freq;
>>   	int csi2_ddrclk_khz;
>>   	struct iss_csiphy_lanes_cfg *lanes;
>>   	unsigned int used_lanes = 0;
>> @@ -193,9 +194,21 @@ int omap4iss_csiphy_config(struct iss_device *iss,
>>   	if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos))
>>   		return -EINVAL;
>>   
>> -	csi2_ddrclk_khz = pipe->external_rate / 1000
>> -		/ (2 * csi2->phy->used_data_lanes)
>> -		* pipe->external_bpp;
>> +	if (!pipe->external_lfreq) {
>> +		link_freq = v4l2_get_link_freq(pipe->external->ctrl_handler,
> 
> I wonder if you could this unconditionally, and remove external_lfreq field
> altogether. The same could be done for external_bpp but that's out of scope
> for this patch.

Hard to tell.
This might be possible as all this logic to prevent multiple v4l2_subdev_call(get_fmt)
and v4l2_get_link_freq() calls per single iss_video_streamon() seems to be needed
only when ISS operates in memory-to-memory mode. Not sure how link frequency, and
used_lanes are related to memory-to-memory mode... Will try to find out.

Thanks,
Andrey

>> +					       pipe->external_bpp,
>> +					       2 * csi2->phy->used_data_lanes);
>> +		if (link_freq < 0) {
>> +			dev_warn(iss->dev,
>> +				 "failed to read the link frequency fromn subdev %s\n",
>> +				 pipe->external->name);
>> +			return -EINVAL;
>> +		}
>> +
>> +		pipe->external_lfreq = link_freq;
>> +	}
>> +
>> +	csi2_ddrclk_khz = div_s64(pipe->external_lfreq, 1000);
>>   
>>   	/*
>>   	 * THS_TERM: Programmed value = ceil(12.5 ns/DDRClk period) - 1.
>> diff --git a/drivers/staging/media/omap4iss/iss_video.c b/drivers/staging/media/omap4iss/iss_video.c
>> index 66975a37dc85..a654c8d18bbc 100644
>> --- a/drivers/staging/media/omap4iss/iss_video.c
>> +++ b/drivers/staging/media/omap4iss/iss_video.c
>> @@ -872,7 +872,7 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
>>   	pipe = entity->pipe
>>   	     ? to_iss_pipeline(entity) : &video->pipe;
>>   	pipe->external = NULL;
>> -	pipe->external_rate = 0;
>> +	pipe->external_lfreq = 0;
>>   	pipe->external_bpp = 0;
>>   
>>   	ret = media_entity_enum_init(&pipe->ent_enum, entity->graph_obj.mdev);
>> diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h
>> index 526281bf0051..2ad5c8483958 100644
>> --- a/drivers/staging/media/omap4iss/iss_video.h
>> +++ b/drivers/staging/media/omap4iss/iss_video.h
>> @@ -86,7 +86,7 @@ struct iss_pipeline {
>>   	bool error;
>>   	struct v4l2_fract max_timeperframe;
>>   	struct v4l2_subdev *external;
>> -	unsigned int external_rate;
>> +	s64 external_lfreq;
>>   	int external_bpp;
>>   };
>>   
> 

Re: [RFC PATCH 4/4] staging: media: omap4iss: use v4l2_get_link_freq() to get the external rate

[Sakari Ailus]

Hi Andrey,

On Tue, Mar 09, 2021 at 02:24:41PM +0300, Andrey Konovalov wrote:
> Hi Sakari,
> 
> On 05.03.2021 18:41, Sakari Ailus wrote:
> > Hi Andrey,
> > 
> > Thanks for the set.
> > 
> > On Wed, Mar 03, 2021 at 09:08:17PM +0300, Andrey Konovalov wrote:
> > > This driver uses V4L2_CID_PIXEL_RATE to calculate the CSI2 link frequency,
> > > but this may give incorrect result in some cases. Use v4l2_get_link_freq()
> > > instead.
> > > 
> > > Also the driver used the external_rate field in struct iss_pipeline as a
> > > flag to prevent excessive v4l2_subdev_call's when processing the frames
> > > in single-shot mode. Replace the external_rate with external_lfreq, and
> > > use external_bpp and external_lfreq to call v4l2_subdev_call(get_fmt) and
> > > v4l2_get_link_freq() respectively only once per iss_video_streamon().
> > > 
> > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> > > ---
> > >   drivers/staging/media/omap4iss/iss.c        | 12 +-----------
> > >   drivers/staging/media/omap4iss/iss_csiphy.c | 19 ++++++++++++++++---
> > >   drivers/staging/media/omap4iss/iss_video.c  |  2 +-
> > >   drivers/staging/media/omap4iss/iss_video.h  |  2 +-
> > >   4 files changed, 19 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
> > > index dae9073e7d3c..0eb7b1b5dcc4 100644
> > > --- a/drivers/staging/media/omap4iss/iss.c
> > > +++ b/drivers/staging/media/omap4iss/iss.c
> > > @@ -131,7 +131,7 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,
> > >   	if (!pipe->external)
> > >   		return 0;
> > > -	if (pipe->external_rate)
> > > +	if (pipe->external_bpp)
> > >   		return 0;
> > >   	memset(&fmt, 0, sizeof(fmt));
> > > @@ -145,16 +145,6 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,
> > >   	pipe->external_bpp = omap4iss_video_format_info(fmt.format.code)->bpp;
> > > -	ctrl = v4l2_ctrl_find(pipe->external->ctrl_handler,
> > > -			      V4L2_CID_PIXEL_RATE);
> > > -	if (!ctrl) {
> > > -		dev_warn(iss->dev, "no pixel rate control in subdev %s\n",
> > > -			 pipe->external->name);
> > > -		return -EPIPE;
> > > -	}
> > > -
> > > -	pipe->external_rate = v4l2_ctrl_g_ctrl_int64(ctrl);
> > > -
> > >   	return 0;
> > >   }
> > > diff --git a/drivers/staging/media/omap4iss/iss_csiphy.c b/drivers/staging/media/omap4iss/iss_csiphy.c
> > > index 96f2ce045138..cec0cd21f7e0 100644
> > > --- a/drivers/staging/media/omap4iss/iss_csiphy.c
> > > +++ b/drivers/staging/media/omap4iss/iss_csiphy.c
> > > @@ -119,6 +119,7 @@ int omap4iss_csiphy_config(struct iss_device *iss,
> > >   	struct iss_pipeline *pipe = to_iss_pipeline(&csi2_subdev->entity);
> > >   	struct iss_v4l2_subdevs_group *subdevs = pipe->external->host_priv;
> > >   	struct iss_csiphy_dphy_cfg csi2phy;
> > > +	s64 link_freq;
> > >   	int csi2_ddrclk_khz;
> > >   	struct iss_csiphy_lanes_cfg *lanes;
> > >   	unsigned int used_lanes = 0;
> > > @@ -193,9 +194,21 @@ int omap4iss_csiphy_config(struct iss_device *iss,
> > >   	if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos))
> > >   		return -EINVAL;
> > > -	csi2_ddrclk_khz = pipe->external_rate / 1000
> > > -		/ (2 * csi2->phy->used_data_lanes)
> > > -		* pipe->external_bpp;
> > > +	if (!pipe->external_lfreq) {
> > > +		link_freq = v4l2_get_link_freq(pipe->external->ctrl_handler,
> > 
> > I wonder if you could this unconditionally, and remove external_lfreq field
> > altogether. The same could be done for external_bpp but that's out of scope
> > for this patch.
> 
> Hard to tell.
> This might be possible as all this logic to prevent multiple v4l2_subdev_call(get_fmt)
> and v4l2_get_link_freq() calls per single iss_video_streamon() seems to be needed
> only when ISS operates in memory-to-memory mode. Not sure how link frequency, and
> used_lanes are related to memory-to-memory mode... Will try to find out.

It seems the same pattern is used in the omap3isp driver. Both should be
changed at the same time. May well be out of scope now.

-- 
Kind regards,

Sakari Ailus

Re: [RFC PATCH 1/4] media: rcar-vin: use v4l2_get_link_freq() to calculate phypll frequency

[Sakari Ailus]

Hi Niklas,

On Mon, Mar 08, 2021 at 02:46:52PM +0100, Niklas Söderlund wrote:
> Hi Andrey,
> 
> Thanks for your patch.
> 
> On 2021-03-03 21:08:14 +0300, Andrey Konovalov wrote:
> > To get the link frequency value, or to calculate a parameter depending on
> > it the receiver driver should use V4L2_CID_LINK_FREQ. If V4L2_CID_LINK_FREQ
> > control is not implemented in the remote subdevice, the link frequency
> > can be calculated from V4L2_CID_PIXEL_RATE control value. But the latter
> > may not give the correct link frequency, and should only be used as the
> > last resort. v4l2_get_link_freq() does exactly that, so use it instead
> > of reading V4L2_CID_PIXEL_RATE directly.
> 
> I like the direction this patch is taking, but I'm a bit concerned about 
> that V4L2_CID_LINK_FREQ is not able to replace V4L2_CID_PIXEL_RATE as it 
> is designed today. Maybe my concern is unfounded and only reflects my 
> own misunderstanding of the API.
> 
> When I wrote this code I tried to first do it using V4L2_CID_LINK_FREQ 
> but I found no way to be able to express the wide rang of values needed 
> for my use-case given that V4L2_CID_LINK_FREQ is a menu control. I had 

I think we could make it alternatively a 64-bit integer control if that
helps. The helper needs to be adjusted accordingly.

> to use V4L2_CID_PIXEL_RATE as it allowed me to at runtime calculate and 
> report the link speed based on input formats. The Use-cases I need to 
> address are where CSI-2 transmitter themself are a bridge in the video 
> pipeline, for example

Is the actual bus frequency changed based on this?

Depending on the system where this chip is being used, only certain
frequencies may be allowed on that bus. It would be most straightforward to
use only those, but on the other hand, if any frequency can be used and
that is certain, then I have no objections to allowing that either. We
simply would make the link-frequencies property optional.

> 
> * Case 1 - HDMI video source
> 
> HDMI source -> ADV748x (HDMI-to-CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver
> 
> The R-Car CSI-2 receiver needs to know the CSI-2 link frequency and 
> queries the ADV748x using V4L2_CID_PIXEL_RATE. The ADV748x reports the 
> pixel rate based on the HDMI format detected on its sink pad.
> 
> This could be done using V4L2_CID_LINK_FREQ, but as it's a menu control 
> it becomes rather tricky to populate it with all possible values, but I 
> guess it could be doable?
> 
> * Case 2 - Multiple video streams over a CSI-2 bus (GMSL)
> 
> Camera 1 -|
> Camera 2 -|
> Camera 3 -|---> MAX9286 (GMSL-to CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver
> Camera 4 -|
> 
> The MAX9286 has 4 sink pads each connected to an independent camera and 
> a single CSI-2 source pad. When streaming starts the MAX9286 computes 
> the total CSI-2 link speed as V4L2_CID_PIXEL_RATE based on the format on 
> each of it's 4 sink pads.
> 
> As in case 1 this could be reported by V4L2_CID_LINK_FREQ but I don't 
> see it as feasible to populate the menu control with all possible 
> frequencies before hand.
> 
> Hopefully this is all easily solvable and I have only misunderstood how 
> menu controls work. If not I think this needs to be considered as part 
> of this series as otherwise it could leave the CSI-2 bridge drivers 
> without a path forward.
> 
> > 
> > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> 
> I tested this and it works as expected. Also as expected it prints lots 
> of warnings about the usage of V4L2_CID_PIXEL_RATE :-) Once I understand 
> how I can fix the CSI-2 transmitters used as bridges in the R-Car boards 
> I will be happy to add my tag to this series as well as fix the bridge 
> drivers.
> 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++-----------
> >  1 file changed, 7 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index e06cd512aba2..eec8f9dd9060 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -455,29 +455,25 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp,
> >  			   unsigned int lanes)
> >  {
> >  	struct v4l2_subdev *source;
> > -	struct v4l2_ctrl *ctrl;
> > -	u64 mbps;
> > +	s64 mbps;
> >  
> >  	if (!priv->remote)
> >  		return -ENODEV;
> >  
> >  	source = priv->remote;
> >  
> > -	/* Read the pixel rate control from remote. */
> > -	ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
> > -	if (!ctrl) {
> > -		dev_err(priv->dev, "no pixel rate control in subdev %s\n",
> > +	/* Read the link frequency from the remote subdev. */
> > +	mbps = v4l2_get_link_freq(source->ctrl_handler, bpp, 2 * lanes);
> > +	if (mbps < 0) {
> > +		dev_err(priv->dev, "failed to get link rate from subdev %s\n",
> >  			source->name);
> > -		return -EINVAL;
> > +		return mbps;
> >  	}
> > -
> >  	/*
> >  	 * Calculate the phypll in mbps.
> > -	 * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes)
> >  	 * bps = link_freq * 2
> >  	 */
> > -	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
> > -	do_div(mbps, lanes * 1000000);
> > +	do_div(mbps, 1000000 / 2);
> >  
> >  	return mbps;
> >  }
> > -- 
> > 2.17.1
> > 

-- 
Kind regards,

Sakari Ailus

Re: [RFC PATCH 1/4] media: rcar-vin: use v4l2_get_link_freq() to calculate phypll frequency

[Niklas Söderlund]

Hi Sakari,

On 2021-03-23 15:10:41 +0200, Sakari Ailus wrote:
> Hi Niklas,
> 
> On Mon, Mar 08, 2021 at 02:46:52PM +0100, Niklas Söderlund wrote:
> > Hi Andrey,
> > 
> > Thanks for your patch.
> > 
> > On 2021-03-03 21:08:14 +0300, Andrey Konovalov wrote:
> > > To get the link frequency value, or to calculate a parameter depending on
> > > it the receiver driver should use V4L2_CID_LINK_FREQ. If V4L2_CID_LINK_FREQ
> > > control is not implemented in the remote subdevice, the link frequency
> > > can be calculated from V4L2_CID_PIXEL_RATE control value. But the latter
> > > may not give the correct link frequency, and should only be used as the
> > > last resort. v4l2_get_link_freq() does exactly that, so use it instead
> > > of reading V4L2_CID_PIXEL_RATE directly.
> > 
> > I like the direction this patch is taking, but I'm a bit concerned about 
> > that V4L2_CID_LINK_FREQ is not able to replace V4L2_CID_PIXEL_RATE as it 
> > is designed today. Maybe my concern is unfounded and only reflects my 
> > own misunderstanding of the API.
> > 
> > When I wrote this code I tried to first do it using V4L2_CID_LINK_FREQ 
> > but I found no way to be able to express the wide rang of values needed 
> > for my use-case given that V4L2_CID_LINK_FREQ is a menu control. I had 
> 
> I think we could make it alternatively a 64-bit integer control if that
> helps. The helper needs to be adjusted accordingly.

That would solve my concern.

> 
> > to use V4L2_CID_PIXEL_RATE as it allowed me to at runtime calculate and 
> > report the link speed based on input formats. The Use-cases I need to 
> > address are where CSI-2 transmitter themself are a bridge in the video 
> > pipeline, for example
> 
> Is the actual bus frequency changed based on this?

Yes

> 
> Depending on the system where this chip is being used, only certain
> frequencies may be allowed on that bus. It would be most straightforward to
> use only those, but on the other hand, if any frequency can be used and
> that is certain, then I have no objections to allowing that either. We
> simply would make the link-frequencies property optional.

The transmitter is a ADV748x and depending on the video input source 
(HDMI or CVBS) the output frequency changes. Failing to negotiate this 
of course results in the CSI-2 receiver never detecting LP-11.

> 
> > 
> > * Case 1 - HDMI video source
> > 
> > HDMI source -> ADV748x (HDMI-to-CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver
> > 
> > The R-Car CSI-2 receiver needs to know the CSI-2 link frequency and 
> > queries the ADV748x using V4L2_CID_PIXEL_RATE. The ADV748x reports the 
> > pixel rate based on the HDMI format detected on its sink pad.
> > 
> > This could be done using V4L2_CID_LINK_FREQ, but as it's a menu control 
> > it becomes rather tricky to populate it with all possible values, but I 
> > guess it could be doable?
> > 
> > * Case 2 - Multiple video streams over a CSI-2 bus (GMSL)
> > 
> > Camera 1 -|
> > Camera 2 -|
> > Camera 3 -|---> MAX9286 (GMSL-to CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver
> > Camera 4 -|
> > 
> > The MAX9286 has 4 sink pads each connected to an independent camera and 
> > a single CSI-2 source pad. When streaming starts the MAX9286 computes 
> > the total CSI-2 link speed as V4L2_CID_PIXEL_RATE based on the format on 
> > each of it's 4 sink pads.
> > 
> > As in case 1 this could be reported by V4L2_CID_LINK_FREQ but I don't 
> > see it as feasible to populate the menu control with all possible 
> > frequencies before hand.
> > 
> > Hopefully this is all easily solvable and I have only misunderstood how 
> > menu controls work. If not I think this needs to be considered as part 
> > of this series as otherwise it could leave the CSI-2 bridge drivers 
> > without a path forward.
> > 
> > > 
> > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> > 
> > I tested this and it works as expected. Also as expected it prints lots 
> > of warnings about the usage of V4L2_CID_PIXEL_RATE :-) Once I understand 
> > how I can fix the CSI-2 transmitters used as bridges in the R-Car boards 
> > I will be happy to add my tag to this series as well as fix the bridge 
> > drivers.
> > 
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++-----------
> > >  1 file changed, 7 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > index e06cd512aba2..eec8f9dd9060 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > @@ -455,29 +455,25 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp,
> > >  			   unsigned int lanes)
> > >  {
> > >  	struct v4l2_subdev *source;
> > > -	struct v4l2_ctrl *ctrl;
> > > -	u64 mbps;
> > > +	s64 mbps;
> > >  
> > >  	if (!priv->remote)
> > >  		return -ENODEV;
> > >  
> > >  	source = priv->remote;
> > >  
> > > -	/* Read the pixel rate control from remote. */
> > > -	ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
> > > -	if (!ctrl) {
> > > -		dev_err(priv->dev, "no pixel rate control in subdev %s\n",
> > > +	/* Read the link frequency from the remote subdev. */
> > > +	mbps = v4l2_get_link_freq(source->ctrl_handler, bpp, 2 * lanes);
> > > +	if (mbps < 0) {
> > > +		dev_err(priv->dev, "failed to get link rate from subdev %s\n",
> > >  			source->name);
> > > -		return -EINVAL;
> > > +		return mbps;
> > >  	}
> > > -
> > >  	/*
> > >  	 * Calculate the phypll in mbps.
> > > -	 * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes)
> > >  	 * bps = link_freq * 2
> > >  	 */
> > > -	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
> > > -	do_div(mbps, lanes * 1000000);
> > > +	do_div(mbps, 1000000 / 2);
> > >  
> > >  	return mbps;
> > >  }
> > > -- 
> > > 2.17.1
> > > 
> 
> -- 
> Kind regards,
> 
> Sakari Ailus

-- 
Regards,
Niklas Söderlund

Re: [RFC PATCH 1/4] media: rcar-vin: use v4l2_get_link_freq() to calculate phypll frequency

[Laurent Pinchart]

Hi Niklas,

On Tue, Mar 23, 2021 at 02:57:05PM +0100, Niklas Söderlund wrote:
> On 2021-03-23 15:10:41 +0200, Sakari Ailus wrote:
> > On Mon, Mar 08, 2021 at 02:46:52PM +0100, Niklas Söderlund wrote:
> > > On 2021-03-03 21:08:14 +0300, Andrey Konovalov wrote:
> > > > To get the link frequency value, or to calculate a parameter depending on
> > > > it the receiver driver should use V4L2_CID_LINK_FREQ. If V4L2_CID_LINK_FREQ
> > > > control is not implemented in the remote subdevice, the link frequency
> > > > can be calculated from V4L2_CID_PIXEL_RATE control value. But the latter
> > > > may not give the correct link frequency, and should only be used as the
> > > > last resort. v4l2_get_link_freq() does exactly that, so use it instead
> > > > of reading V4L2_CID_PIXEL_RATE directly.
> > > 
> > > I like the direction this patch is taking, but I'm a bit concerned about 
> > > that V4L2_CID_LINK_FREQ is not able to replace V4L2_CID_PIXEL_RATE as it 
> > > is designed today. Maybe my concern is unfounded and only reflects my 
> > > own misunderstanding of the API.
> > > 
> > > When I wrote this code I tried to first do it using V4L2_CID_LINK_FREQ 
> > > but I found no way to be able to express the wide rang of values needed 
> > > for my use-case given that V4L2_CID_LINK_FREQ is a menu control. I had 
> > 
> > I think we could make it alternatively a 64-bit integer control if that
> > helps. The helper needs to be adjusted accordingly.
> 
> That would solve my concern.
> 
> > > to use V4L2_CID_PIXEL_RATE as it allowed me to at runtime calculate and 
> > > report the link speed based on input formats. The Use-cases I need to 
> > > address are where CSI-2 transmitter themself are a bridge in the video 
> > > pipeline, for example
> > 
> > Is the actual bus frequency changed based on this?
> 
> Yes
> 
> > Depending on the system where this chip is being used, only certain
> > frequencies may be allowed on that bus. It would be most straightforward to
> > use only those, but on the other hand, if any frequency can be used and
> > that is certain, then I have no objections to allowing that either. We
> > simply would make the link-frequencies property optional.
> 
> The transmitter is a ADV748x and depending on the video input source 
> (HDMI or CVBS) the output frequency changes. Failing to negotiate this 
> of course results in the CSI-2 receiver never detecting LP-11.
>
> > > * Case 1 - HDMI video source
> > > 
> > > HDMI source -> ADV748x (HDMI-to-CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver
> > > 
> > > The R-Car CSI-2 receiver needs to know the CSI-2 link frequency and 
> > > queries the ADV748x using V4L2_CID_PIXEL_RATE. The ADV748x reports the 
> > > pixel rate based on the HDMI format detected on its sink pad.
> > > 
> > > This could be done using V4L2_CID_LINK_FREQ, but as it's a menu control 
> > > it becomes rather tricky to populate it with all possible values, but I 
> > > guess it could be doable?

There are, generally speaking, two different uses for this information
on the receiver side. We need to configure the DPHY timings that depend
on the link frequency, and we need to configure the functional clock of
the receiver and downstream IP cores to ensure they have enough
bandwidth to absorb all pixels. Those are two fundamentally different
issues:

- The DPHY timings depend on the link frequency, which is a well-defined
  physical concept. We currently compute it from the pixel rate, which
  is a more loosely defined concept (see below). Assuming the
  V4L2_CID_LINK_FREQ control can be made to report the actual link
  frequency (and given that this is the control's purpose, there's no
  other option than making it work, otherwise the control would be
  entirely pointless), possibly by turning it into an INT64 control,
  then that's the right control to use for this purpose.

- The functional clock of the video pipeline need to be able to absord
  the incoming pixels. If the clock is configurable, it means that it
  differs from the CSI-2 receiver clock (derived from the bus), which
  normally implies a FIFO between the CSI-2 receiver and the downstream
  blocks. The main constraint is that the FIFO shouldn't overflow, which
  in practice means that the effective average pixel rate per line on
  the input needs to be smaller or equal than on the output. This
  however doesn't mean that the input clock needs to be higher than the
  output clock, given that not only input and output bus widths can be
  different, but horizontal blanking can also be used to perform timing
  adjustements. For instance, if the input video stream has 1000 active
  and 3000 blanking pixels per line, assuming identical bus widths on
  the input and output side of the FIFO, we could have an output clock
  frequency equal to half of the input clock frequency, as long as the
  FIFO depth is at least 500 pixels. The output side would have a
  horizontal blanking of 1000 pixels. The same applies on the
  transmitter side, as there's often a FIFO between the pixel source
  (the pixel array for a sensor for instance, with a sampling clock
  rate) and the CSI-2 transmitter (running at the bus rate). The pixel
  rate is thus a much more fuzzy concept, isn't well-defined in V4L2,
  and can lead to all kind of interoperability issues. It should only be
  used along with blanking information, in order to perform rate
  adaptation calculations.

> > > * Case 2 - Multiple video streams over a CSI-2 bus (GMSL)
> > > 
> > > Camera 1 -|
> > > Camera 2 -|
> > > Camera 3 -|---> MAX9286 (GMSL-to CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver
> > > Camera 4 -|
> > > 
> > > The MAX9286 has 4 sink pads each connected to an independent camera and 
> > > a single CSI-2 source pad. When streaming starts the MAX9286 computes 
> > > the total CSI-2 link speed as V4L2_CID_PIXEL_RATE based on the format on 
> > > each of it's 4 sink pads.
> > > 
> > > As in case 1 this could be reported by V4L2_CID_LINK_FREQ but I don't 
> > > see it as feasible to populate the menu control with all possible 
> > > frequencies before hand.

As explained above, the CSI-2 frequency doesn't have to match the pixel
rate of the sensor(s). I haven't checked exactly how the MAX9286 handles
clock domains, but in general there source rate and the bus rate can be
different. That's why the link frequency is often a menu control with a
limited set of values (carefully selected by the system designer to
accommodate EMC constraints), while the source rate can vary more
freely. As long as the link frequency provides enough bandwidth, it
doesn't have to be tightly coupled with the pixel rate. For source
devices that have a single clock domain, and adjust the link frequency
to follow the source rate, then turning the link frequency control into
an INT64 would make sense.

> > > Hopefully this is all easily solvable and I have only misunderstood how 
> > > menu controls work. If not I think this needs to be considered as part 
> > > of this series as otherwise it could leave the CSI-2 bridge drivers 
> > > without a path forward.
> > > 
> > > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> > > 
> > > I tested this and it works as expected. Also as expected it prints lots 
> > > of warnings about the usage of V4L2_CID_PIXEL_RATE :-) Once I understand 
> > > how I can fix the CSI-2 transmitters used as bridges in the R-Car boards 
> > > I will be happy to add my tag to this series as well as fix the bridge 
> > > drivers.
> > > 
> > > > ---
> > > >  drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++-----------
> > > >  1 file changed, 7 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > index e06cd512aba2..eec8f9dd9060 100644
> > > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > @@ -455,29 +455,25 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp,
> > > >  			   unsigned int lanes)
> > > >  {
> > > >  	struct v4l2_subdev *source;
> > > > -	struct v4l2_ctrl *ctrl;
> > > > -	u64 mbps;
> > > > +	s64 mbps;
> > > >  
> > > >  	if (!priv->remote)
> > > >  		return -ENODEV;
> > > >  
> > > >  	source = priv->remote;
> > > >  
> > > > -	/* Read the pixel rate control from remote. */
> > > > -	ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
> > > > -	if (!ctrl) {
> > > > -		dev_err(priv->dev, "no pixel rate control in subdev %s\n",
> > > > +	/* Read the link frequency from the remote subdev. */
> > > > +	mbps = v4l2_get_link_freq(source->ctrl_handler, bpp, 2 * lanes);
> > > > +	if (mbps < 0) {
> > > > +		dev_err(priv->dev, "failed to get link rate from subdev %s\n",
> > > >  			source->name);
> > > > -		return -EINVAL;
> > > > +		return mbps;
> > > >  	}
> > > > -
> > > >  	/*
> > > >  	 * Calculate the phypll in mbps.
> > > > -	 * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes)
> > > >  	 * bps = link_freq * 2
> > > >  	 */
> > > > -	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
> > > > -	do_div(mbps, lanes * 1000000);
> > > > +	do_div(mbps, 1000000 / 2);
> > > >  
> > > >  	return mbps;
> > > >  }

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 1/4] media: rcar-vin: use v4l2_get_link_freq() to calculate phypll frequency

[Niklas Söderlund]

Hi Laurent,

Thanks for the good write-up of the two different uses for this type of 
information, it make the situation more clear for me. I really think 
this series moves in the right direction as the current usage of 
V4L2_CID_PIXEL_RATE is then wrong in the rcar-vin driver. The reason 
being to work around the fact that the V4L2_CID_LINK_FREQ is a menu and 
not an INT64 control.

I'm looking at the ADV748x driver sources and it seems it too adjusts 
the link frequency bases on the source rate. With this background do you 
think the right move is to turn V4L2_CID_LINK_FREQ into a INT64 and try 
to remove or redefine V4L2_CID_PIXEL_RATE to better describe to more 
complex parameters you outline below?

On 2021-03-23 23:24:40 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Tue, Mar 23, 2021 at 02:57:05PM +0100, Niklas Söderlund wrote:
> > On 2021-03-23 15:10:41 +0200, Sakari Ailus wrote:
> > > On Mon, Mar 08, 2021 at 02:46:52PM +0100, Niklas Söderlund wrote:
> > > > On 2021-03-03 21:08:14 +0300, Andrey Konovalov wrote:
> > > > > To get the link frequency value, or to calculate a parameter depending on
> > > > > it the receiver driver should use V4L2_CID_LINK_FREQ. If V4L2_CID_LINK_FREQ
> > > > > control is not implemented in the remote subdevice, the link frequency
> > > > > can be calculated from V4L2_CID_PIXEL_RATE control value. But the latter
> > > > > may not give the correct link frequency, and should only be used as the
> > > > > last resort. v4l2_get_link_freq() does exactly that, so use it instead
> > > > > of reading V4L2_CID_PIXEL_RATE directly.
> > > > 
> > > > I like the direction this patch is taking, but I'm a bit concerned about 
> > > > that V4L2_CID_LINK_FREQ is not able to replace V4L2_CID_PIXEL_RATE as it 
> > > > is designed today. Maybe my concern is unfounded and only reflects my 
> > > > own misunderstanding of the API.
> > > > 
> > > > When I wrote this code I tried to first do it using V4L2_CID_LINK_FREQ 
> > > > but I found no way to be able to express the wide rang of values needed 
> > > > for my use-case given that V4L2_CID_LINK_FREQ is a menu control. I had 
> > > 
> > > I think we could make it alternatively a 64-bit integer control if that
> > > helps. The helper needs to be adjusted accordingly.
> > 
> > That would solve my concern.
> > 
> > > > to use V4L2_CID_PIXEL_RATE as it allowed me to at runtime calculate and 
> > > > report the link speed based on input formats. The Use-cases I need to 
> > > > address are where CSI-2 transmitter themself are a bridge in the video 
> > > > pipeline, for example
> > > 
> > > Is the actual bus frequency changed based on this?
> > 
> > Yes
> > 
> > > Depending on the system where this chip is being used, only certain
> > > frequencies may be allowed on that bus. It would be most straightforward to
> > > use only those, but on the other hand, if any frequency can be used and
> > > that is certain, then I have no objections to allowing that either. We
> > > simply would make the link-frequencies property optional.
> > 
> > The transmitter is a ADV748x and depending on the video input source 
> > (HDMI or CVBS) the output frequency changes. Failing to negotiate this 
> > of course results in the CSI-2 receiver never detecting LP-11.
> >
> > > > * Case 1 - HDMI video source
> > > > 
> > > > HDMI source -> ADV748x (HDMI-to-CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver
> > > > 
> > > > The R-Car CSI-2 receiver needs to know the CSI-2 link frequency and 
> > > > queries the ADV748x using V4L2_CID_PIXEL_RATE. The ADV748x reports the 
> > > > pixel rate based on the HDMI format detected on its sink pad.
> > > > 
> > > > This could be done using V4L2_CID_LINK_FREQ, but as it's a menu control 
> > > > it becomes rather tricky to populate it with all possible values, but I 
> > > > guess it could be doable?
> 
> There are, generally speaking, two different uses for this information
> on the receiver side. We need to configure the DPHY timings that depend
> on the link frequency, and we need to configure the functional clock of
> the receiver and downstream IP cores to ensure they have enough
> bandwidth to absorb all pixels. Those are two fundamentally different
> issues:
> 
> - The DPHY timings depend on the link frequency, which is a well-defined
>   physical concept. We currently compute it from the pixel rate, which
>   is a more loosely defined concept (see below). Assuming the
>   V4L2_CID_LINK_FREQ control can be made to report the actual link
>   frequency (and given that this is the control's purpose, there's no
>   other option than making it work, otherwise the control would be
>   entirely pointless), possibly by turning it into an INT64 control,
>   then that's the right control to use for this purpose.
> 
> - The functional clock of the video pipeline need to be able to absord
>   the incoming pixels. If the clock is configurable, it means that it
>   differs from the CSI-2 receiver clock (derived from the bus), which
>   normally implies a FIFO between the CSI-2 receiver and the downstream
>   blocks. The main constraint is that the FIFO shouldn't overflow, which
>   in practice means that the effective average pixel rate per line on
>   the input needs to be smaller or equal than on the output. This
>   however doesn't mean that the input clock needs to be higher than the
>   output clock, given that not only input and output bus widths can be
>   different, but horizontal blanking can also be used to perform timing
>   adjustements. For instance, if the input video stream has 1000 active
>   and 3000 blanking pixels per line, assuming identical bus widths on
>   the input and output side of the FIFO, we could have an output clock
>   frequency equal to half of the input clock frequency, as long as the
>   FIFO depth is at least 500 pixels. The output side would have a
>   horizontal blanking of 1000 pixels. The same applies on the
>   transmitter side, as there's often a FIFO between the pixel source
>   (the pixel array for a sensor for instance, with a sampling clock
>   rate) and the CSI-2 transmitter (running at the bus rate). The pixel
>   rate is thus a much more fuzzy concept, isn't well-defined in V4L2,
>   and can lead to all kind of interoperability issues. It should only be
>   used along with blanking information, in order to perform rate
>   adaptation calculations.
> 
> > > > * Case 2 - Multiple video streams over a CSI-2 bus (GMSL)
> > > > 
> > > > Camera 1 -|
> > > > Camera 2 -|
> > > > Camera 3 -|---> MAX9286 (GMSL-to CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver
> > > > Camera 4 -|
> > > > 
> > > > The MAX9286 has 4 sink pads each connected to an independent camera and 
> > > > a single CSI-2 source pad. When streaming starts the MAX9286 computes 
> > > > the total CSI-2 link speed as V4L2_CID_PIXEL_RATE based on the format on 
> > > > each of it's 4 sink pads.
> > > > 
> > > > As in case 1 this could be reported by V4L2_CID_LINK_FREQ but I don't 
> > > > see it as feasible to populate the menu control with all possible 
> > > > frequencies before hand.
> 
> As explained above, the CSI-2 frequency doesn't have to match the pixel
> rate of the sensor(s). I haven't checked exactly how the MAX9286 handles
> clock domains, but in general there source rate and the bus rate can be
> different. That's why the link frequency is often a menu control with a
> limited set of values (carefully selected by the system designer to
> accommodate EMC constraints), while the source rate can vary more
> freely. As long as the link frequency provides enough bandwidth, it
> doesn't have to be tightly coupled with the pixel rate. For source
> devices that have a single clock domain, and adjust the link frequency
> to follow the source rate, then turning the link frequency control into
> an INT64 would make sense.
> 
> > > > Hopefully this is all easily solvable and I have only misunderstood how 
> > > > menu controls work. If not I think this needs to be considered as part 
> > > > of this series as otherwise it could leave the CSI-2 bridge drivers 
> > > > without a path forward.
> > > > 
> > > > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> > > > 
> > > > I tested this and it works as expected. Also as expected it prints lots 
> > > > of warnings about the usage of V4L2_CID_PIXEL_RATE :-) Once I understand 
> > > > how I can fix the CSI-2 transmitters used as bridges in the R-Car boards 
> > > > I will be happy to add my tag to this series as well as fix the bridge 
> > > > drivers.
> > > > 
> > > > > ---
> > > > >  drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++-----------
> > > > >  1 file changed, 7 insertions(+), 11 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > > index e06cd512aba2..eec8f9dd9060 100644
> > > > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > > @@ -455,29 +455,25 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp,
> > > > >  			   unsigned int lanes)
> > > > >  {
> > > > >  	struct v4l2_subdev *source;
> > > > > -	struct v4l2_ctrl *ctrl;
> > > > > -	u64 mbps;
> > > > > +	s64 mbps;
> > > > >  
> > > > >  	if (!priv->remote)
> > > > >  		return -ENODEV;
> > > > >  
> > > > >  	source = priv->remote;
> > > > >  
> > > > > -	/* Read the pixel rate control from remote. */
> > > > > -	ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
> > > > > -	if (!ctrl) {
> > > > > -		dev_err(priv->dev, "no pixel rate control in subdev %s\n",
> > > > > +	/* Read the link frequency from the remote subdev. */
> > > > > +	mbps = v4l2_get_link_freq(source->ctrl_handler, bpp, 2 * lanes);
> > > > > +	if (mbps < 0) {
> > > > > +		dev_err(priv->dev, "failed to get link rate from subdev %s\n",
> > > > >  			source->name);
> > > > > -		return -EINVAL;
> > > > > +		return mbps;
> > > > >  	}
> > > > > -
> > > > >  	/*
> > > > >  	 * Calculate the phypll in mbps.
> > > > > -	 * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes)
> > > > >  	 * bps = link_freq * 2
> > > > >  	 */
> > > > > -	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
> > > > > -	do_div(mbps, lanes * 1000000);
> > > > > +	do_div(mbps, 1000000 / 2);
> > > > >  
> > > > >  	return mbps;
> > > > >  }
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund

Re: [RFC PATCH 1/4] media: rcar-vin: use v4l2_get_link_freq() to calculate phypll frequency

[Andrey Konovalov]

Hi Niklas, Laurent, and Sakari,

Thank you for your comments, that's a good discussion!

So it looks like changing V4L2_CID_LINK_FREQ control from an integer menu to
an INT64 one works for everyone.

What is the best way to do this change?
It could be single patch which turns V4L2_CID_LINK_FREQ into an INT64, changes
the v4l2_get_link_freq() helper accordingly, and updates all the drivers
implementing or using V4L2_CID_LINK_FREQ (twenty-something drivers). The
change is simple, but the diff is quite large. Doing it as a set of patches
would break git bisecting, so isn't an option.
Or we could make the INT64 variant of the link frequency control a separate
new control (e.g. V4L2_CID_LINK_FREQ_NEW or V4L2_CID_LINK_FREQ_INT64), switch
V4L2_CID_LINK_FREQ users to the new version one by one, then delete the older
version and rename the new one back to V4L2_CID_LINK_FREQ.

This change might affect the userland, but it looks like this control isn't the
one widely used by the applications. E.g. V4L2_CID_LINK_FREQ definition is
present in libcamera (include/linux/v4l2-controls.h), but is not currently
used.

One more question below.

On 25.03.2021 12:39, Niklas Söderlund wrote:
> Hi Laurent,
> 
> Thanks for the good write-up of the two different uses for this type of
> information, it make the situation more clear for me. I really think
> this series moves in the right direction as the current usage of
> V4L2_CID_PIXEL_RATE is then wrong in the rcar-vin driver. The reason
> being to work around the fact that the V4L2_CID_LINK_FREQ is a menu and
> not an INT64 control.
> 
> I'm looking at the ADV748x driver sources and it seems it too adjusts
> the link frequency bases on the source rate. With this background do you
> think the right move is to turn V4L2_CID_LINK_FREQ into a INT64 and try
> to remove or redefine V4L2_CID_PIXEL_RATE to better describe to more
> complex parameters you outline below?
> 
> On 2021-03-23 23:24:40 +0200, Laurent Pinchart wrote:
>> Hi Niklas,
>>
>> On Tue, Mar 23, 2021 at 02:57:05PM +0100, Niklas Söderlund wrote:
>>> On 2021-03-23 15:10:41 +0200, Sakari Ailus wrote:
>>>> On Mon, Mar 08, 2021 at 02:46:52PM +0100, Niklas Söderlund wrote:
>>>>> On 2021-03-03 21:08:14 +0300, Andrey Konovalov wrote:
>>>>>> To get the link frequency value, or to calculate a parameter depending on
>>>>>> it the receiver driver should use V4L2_CID_LINK_FREQ. If V4L2_CID_LINK_FREQ
>>>>>> control is not implemented in the remote subdevice, the link frequency
>>>>>> can be calculated from V4L2_CID_PIXEL_RATE control value. But the latter
>>>>>> may not give the correct link frequency, and should only be used as the
>>>>>> last resort. v4l2_get_link_freq() does exactly that, so use it instead
>>>>>> of reading V4L2_CID_PIXEL_RATE directly.
>>>>>
>>>>> I like the direction this patch is taking, but I'm a bit concerned about
>>>>> that V4L2_CID_LINK_FREQ is not able to replace V4L2_CID_PIXEL_RATE as it
>>>>> is designed today. Maybe my concern is unfounded and only reflects my
>>>>> own misunderstanding of the API.
>>>>>
>>>>> When I wrote this code I tried to first do it using V4L2_CID_LINK_FREQ
>>>>> but I found no way to be able to express the wide rang of values needed
>>>>> for my use-case given that V4L2_CID_LINK_FREQ is a menu control. I had
>>>>
>>>> I think we could make it alternatively a 64-bit integer control if that
>>>> helps. The helper needs to be adjusted accordingly.
>>>
>>> That would solve my concern.
>>>
>>>>> to use V4L2_CID_PIXEL_RATE as it allowed me to at runtime calculate and
>>>>> report the link speed based on input formats. The Use-cases I need to
>>>>> address are where CSI-2 transmitter themself are a bridge in the video
>>>>> pipeline, for example
>>>>
>>>> Is the actual bus frequency changed based on this?
>>>
>>> Yes
>>>
>>>> Depending on the system where this chip is being used, only certain
>>>> frequencies may be allowed on that bus. It would be most straightforward to
>>>> use only those, but on the other hand, if any frequency can be used and
>>>> that is certain, then I have no objections to allowing that either. We
>>>> simply would make the link-frequencies property optional.

Sakari,

Could you please elaborate a bit more on the last sentence ("make the link-frequencies
property optional")?
How should the CSI-2 receiver get the link frequency value if this optional property
is missing? If the transmitter driver doesn't implement V4L2_CID_LINK_FREQ then it must
guarantee that the link frequency calculated from V4L2_CID_PIXEL_RATE gives the correct
value - something like that?
Should the warning message [1] in the v4l2_get_link_freq() helper be rephrased if
V4L2_CID_LINK_FREQ is optional?

Thanks,
Andrey

[1]
https://git.linuxtv.org/media_tree.git/commit/drivers/media/v4l2-core/v4l2-common.c?id=67012d97df931b1be24efa0cac06f20d5be062eb

>>> The transmitter is a ADV748x and depending on the video input source
>>> (HDMI or CVBS) the output frequency changes. Failing to negotiate this
>>> of course results in the CSI-2 receiver never detecting LP-11.
>>>
>>>>> * Case 1 - HDMI video source
>>>>>
>>>>> HDMI source -> ADV748x (HDMI-to-CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver
>>>>>
>>>>> The R-Car CSI-2 receiver needs to know the CSI-2 link frequency and
>>>>> queries the ADV748x using V4L2_CID_PIXEL_RATE. The ADV748x reports the
>>>>> pixel rate based on the HDMI format detected on its sink pad.
>>>>>
>>>>> This could be done using V4L2_CID_LINK_FREQ, but as it's a menu control
>>>>> it becomes rather tricky to populate it with all possible values, but I
>>>>> guess it could be doable?
>>
>> There are, generally speaking, two different uses for this information
>> on the receiver side. We need to configure the DPHY timings that depend
>> on the link frequency, and we need to configure the functional clock of
>> the receiver and downstream IP cores to ensure they have enough
>> bandwidth to absorb all pixels. Those are two fundamentally different
>> issues:
>>
>> - The DPHY timings depend on the link frequency, which is a well-defined
>>    physical concept. We currently compute it from the pixel rate, which
>>    is a more loosely defined concept (see below). Assuming the
>>    V4L2_CID_LINK_FREQ control can be made to report the actual link
>>    frequency (and given that this is the control's purpose, there's no
>>    other option than making it work, otherwise the control would be
>>    entirely pointless), possibly by turning it into an INT64 control,
>>    then that's the right control to use for this purpose.
>>
>> - The functional clock of the video pipeline need to be able to absord
>>    the incoming pixels. If the clock is configurable, it means that it
>>    differs from the CSI-2 receiver clock (derived from the bus), which
>>    normally implies a FIFO between the CSI-2 receiver and the downstream
>>    blocks. The main constraint is that the FIFO shouldn't overflow, which
>>    in practice means that the effective average pixel rate per line on
>>    the input needs to be smaller or equal than on the output. This
>>    however doesn't mean that the input clock needs to be higher than the
>>    output clock, given that not only input and output bus widths can be
>>    different, but horizontal blanking can also be used to perform timing
>>    adjustements. For instance, if the input video stream has 1000 active
>>    and 3000 blanking pixels per line, assuming identical bus widths on
>>    the input and output side of the FIFO, we could have an output clock
>>    frequency equal to half of the input clock frequency, as long as the
>>    FIFO depth is at least 500 pixels. The output side would have a
>>    horizontal blanking of 1000 pixels. The same applies on the
>>    transmitter side, as there's often a FIFO between the pixel source
>>    (the pixel array for a sensor for instance, with a sampling clock
>>    rate) and the CSI-2 transmitter (running at the bus rate). The pixel
>>    rate is thus a much more fuzzy concept, isn't well-defined in V4L2,
>>    and can lead to all kind of interoperability issues. It should only be
>>    used along with blanking information, in order to perform rate
>>    adaptation calculations.
>>
>>>>> * Case 2 - Multiple video streams over a CSI-2 bus (GMSL)
>>>>>
>>>>> Camera 1 -|
>>>>> Camera 2 -|
>>>>> Camera 3 -|---> MAX9286 (GMSL-to CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver
>>>>> Camera 4 -|
>>>>>
>>>>> The MAX9286 has 4 sink pads each connected to an independent camera and
>>>>> a single CSI-2 source pad. When streaming starts the MAX9286 computes
>>>>> the total CSI-2 link speed as V4L2_CID_PIXEL_RATE based on the format on
>>>>> each of it's 4 sink pads.
>>>>>
>>>>> As in case 1 this could be reported by V4L2_CID_LINK_FREQ but I don't
>>>>> see it as feasible to populate the menu control with all possible
>>>>> frequencies before hand.
>>
>> As explained above, the CSI-2 frequency doesn't have to match the pixel
>> rate of the sensor(s). I haven't checked exactly how the MAX9286 handles
>> clock domains, but in general there source rate and the bus rate can be
>> different. That's why the link frequency is often a menu control with a
>> limited set of values (carefully selected by the system designer to
>> accommodate EMC constraints), while the source rate can vary more
>> freely. As long as the link frequency provides enough bandwidth, it
>> doesn't have to be tightly coupled with the pixel rate. For source
>> devices that have a single clock domain, and adjust the link frequency
>> to follow the source rate, then turning the link frequency control into
>> an INT64 would make sense.
>>
>>>>> Hopefully this is all easily solvable and I have only misunderstood how
>>>>> menu controls work. If not I think this needs to be considered as part
>>>>> of this series as otherwise it could leave the CSI-2 bridge drivers
>>>>> without a path forward.
>>>>>
>>>>>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>>>>>
>>>>> I tested this and it works as expected. Also as expected it prints lots
>>>>> of warnings about the usage of V4L2_CID_PIXEL_RATE :-) Once I understand
>>>>> how I can fix the CSI-2 transmitters used as bridges in the R-Car boards
>>>>> I will be happy to add my tag to this series as well as fix the bridge
>>>>> drivers.
>>>>>
>>>>>> ---
>>>>>>   drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++-----------
>>>>>>   1 file changed, 7 insertions(+), 11 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
>>>>>> index e06cd512aba2..eec8f9dd9060 100644
>>>>>> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
>>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
>>>>>> @@ -455,29 +455,25 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp,
>>>>>>   			   unsigned int lanes)
>>>>>>   {
>>>>>>   	struct v4l2_subdev *source;
>>>>>> -	struct v4l2_ctrl *ctrl;
>>>>>> -	u64 mbps;
>>>>>> +	s64 mbps;
>>>>>>   
>>>>>>   	if (!priv->remote)
>>>>>>   		return -ENODEV;
>>>>>>   
>>>>>>   	source = priv->remote;
>>>>>>   
>>>>>> -	/* Read the pixel rate control from remote. */
>>>>>> -	ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
>>>>>> -	if (!ctrl) {
>>>>>> -		dev_err(priv->dev, "no pixel rate control in subdev %s\n",
>>>>>> +	/* Read the link frequency from the remote subdev. */
>>>>>> +	mbps = v4l2_get_link_freq(source->ctrl_handler, bpp, 2 * lanes);
>>>>>> +	if (mbps < 0) {
>>>>>> +		dev_err(priv->dev, "failed to get link rate from subdev %s\n",
>>>>>>   			source->name);
>>>>>> -		return -EINVAL;
>>>>>> +		return mbps;
>>>>>>   	}
>>>>>> -
>>>>>>   	/*
>>>>>>   	 * Calculate the phypll in mbps.
>>>>>> -	 * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes)
>>>>>>   	 * bps = link_freq * 2
>>>>>>   	 */
>>>>>> -	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
>>>>>> -	do_div(mbps, lanes * 1000000);
>>>>>> +	do_div(mbps, 1000000 / 2);
>>>>>>   
>>>>>>   	return mbps;
>>>>>>   }
>>
>> -- 
>> Regards,
>>
>> Laurent Pinchart
>