[PATCH 0/2] media: ov5640: MIPI fixes on top of Maxime's v5

[PATCH 0/2] media: ov5640: MIPI fixes on top of Maxime's v5

[Jacopo Mondi]

Hello ov5640-ers,
   these two patches should be applied on top of Maxime's clock tree rework v5
and 'fix' MIPI CSI-2 clock tree configuration.

The first patch is a fix that appeard in various forms on the list several
times: if the image sizes gets updated but not the image format, the size
update gets ignored. I had to fix this to run my FPS tests, and thus I'm
sending the two together. I wish in future a re-work of the image format
handling part, but for now, let's just fix it for v4.21.

The second patch slightly reworks the MIPI clock tree configuration, based on
inputs from Sam. The currently submitted v5 in which Maxime squashed my previous
changes is 'broken'. That's my bad, as explained in the patch change log.

Test results are attacched to patch [2/2].
Changelog for [2/2] is included in the patch itself.

I wish these patches to go in with Maxime awesome clock tree re-work, pending
his ack.

Also, I have tested with an i.MX6Q board, with a CSI-2 2 data lanes setup. There
are still a few things not clear to me in the MIPI clock tree, and I welcome
more testing, preferibly with 1-lanes setups.

Also, I had to re-apply Maxime's series and latest ov5640 patches on v4.19,
as my test board is sort of broken with v4.20-rcX (it shouldn't make any
difference in regard to this series, but I'm pointing it out anyhow).

Recently Adam has been testing quite some ov5640 patches, if you fill like
testing these as well on your setup (which I understand is a MIPI CSI-2 one)
please report the results. Same for all other interested ones :)

Thanks
  j

Jacopo Mondi (2):
  media: i2c: ov5640: Fix set format regression
  media: ov5640: make MIPI clock depend on mode

 drivers/media/i2c/ov5640.c | 110 ++++++++++++++++++++++-----------------------
 1 file changed, 54 insertions(+), 56 deletions(-)

--
2.7.4

[PATCH 1/2] media: ov5640: Fix set format regression

[Jacopo Mondi]

The set_fmt operations updates the sensor format only when the image format
is changed. When only the image sizes gets changed, the format do not get
updated causing the sensor to always report the one that was previously in
use.

Without this patch, updating frame size only fails:
  [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb xfer:srgb ...]

With this patch applied:
  [fmt:UYVY8_2X8/1024x768@1/30 field:none colorspace:srgb xfer:srgb ...]

Fixes: 6949d864776e ("media: ov5640: do not change mode if format or frame
interval is unchanged")
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/ov5640.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 03a031a42b3e..c659efe918a4 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -2160,6 +2160,7 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
 	struct ov5640_dev *sensor = to_ov5640_dev(sd);
 	const struct ov5640_mode_info *new_mode;
 	struct v4l2_mbus_framefmt *mbus_fmt = &format->format;
+	struct v4l2_mbus_framefmt *fmt;
 	int ret;

 	if (format->pad != 0)
@@ -2177,22 +2178,20 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
 	if (ret)
 		goto out;

-	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-		struct v4l2_mbus_framefmt *fmt =
-			v4l2_subdev_get_try_format(sd, cfg, 0);
+	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
+		fmt = v4l2_subdev_get_try_format(sd, cfg, 0);
+	else
+		fmt = &sensor->fmt;

-		*fmt = *mbus_fmt;
-		goto out;
-	}
+	*fmt = *mbus_fmt;

 	if (new_mode != sensor->current_mode) {
 		sensor->current_mode = new_mode;
 		sensor->pending_mode_change = true;
 	}
-	if (mbus_fmt->code != sensor->fmt.code) {
-		sensor->fmt = *mbus_fmt;
+	if (mbus_fmt->code != sensor->fmt.code)
 		sensor->pending_fmt_change = true;
-	}
+
 out:
 	mutex_unlock(&sensor->lock);
 	return ret;
--
2.7.4

[PATCH 2/2] media: ov5640: make MIPI clock depend on mode

[Jacopo Mondi]

The MIPI clock generation tree uses the MIPI_DIV divider to generate both the
MIPI SCALER CLOCK and the PIXEL_CLOCK. It seems the MIPI BIT CLK frequency is
generated by either the MIPI SCALER or the PIXEL CLOCK, depending if the
currently applied image mode uses the scaler or not.

Make the MIPI_DIV selection value depend on the subsampling mode used by the
currently applied image mode.

Tested with:
172x144 320x240, 640x480, 1024x756, 1024x768, 1280x720, 1920x1080 in YUYV mode
at 10, 15, 20, and 30 FPS with MIPI CSI-2 2 lanes mode.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
Maxime,
   this patch is not just a cosmetic updates to the 'set_mipi_pclk()'
comment block as I anticipated, but it actually fix the framerate handling
compared for CSI-2, which in you v5 results halved for most modes. I have
not included any "Fixes:" tag, as I hope this patch will get in with your v5.

That's my bad, as in the patches I sent to be applied on top of your v4 I
forgot to add a change, and the requested SCLK rate was always half of what
it was actually required.

Also, I have left out from this patches most of Sam's improvements (better SCLK
selection policies, and programming of register OV5640_REG_PCLK_PERIOD as for
from testing they're not required, and I don't want to pile more patches than
necessary for the next merge window, not to slow down the clock tree rework
inclusion. We can get back to it after it got merged.

This are the test result obtained by capturing 100 frames with yavta and
inspecting the reported fps results.

Results are pretty neat, 2592x1944 mode apart, which runs a bit slow.

capturing 176x144 @ 10FPS
Captured 100 frames in 10.019398 seconds (9.980640 fps, 505898.657683 B/s).
capturing 176x144 @ 15FPS
Captured 100 frames in 6.681860 seconds (14.965892 fps, 758591.132803 B/s).
capturing 176x144 @ 20FPS
Captured 100 frames in 5.056204 seconds (19.777683 fps, 1002491.196755 B/s).
capturing 176x144 @ 30FPS
Captured 100 frames in 3.357204 seconds (29.786686 fps, 1509827.521040 B/s).

capturing 320x240 @ 10FPS
Captured 100 frames in 10.015432 seconds (9.984591 fps, 1533633.245951 B/s).
capturing 320x240 @ 15FPS
Captured 100 frames in 6.684035 seconds (14.961021 fps, 2298012.872049 B/s).
capturing 320x240 @ 20FPS
Captured 100 frames in 5.019164 seconds (19.923635 fps, 3060270.391218 B/s).
capturing 320x240 @ 30FPS
Captured 100 frames in 3.352991 seconds (29.824115 fps, 4580984.103432 B/s).

capturing 640x480 @ 10FPS
Captured 100 frames in 9.990389 seconds (10.009620 fps, 6149910.678538 B/s).
capturing 640x480 @ 15FPS
Captured 100 frames in 6.856242 seconds (14.585249 fps, 8961176.838123 B/s).
capturing 640x480 @ 20FPS
Captured 100 frames in 5.030264 seconds (19.879670 fps, 12214069.053476 B/s).
capturing 640x480 @ 30FPS
Captured 100 frames in 3.364612 seconds (29.721103 fps, 18260645.750580 B/s).

capturing 720x480 @ 10FPS
Captured 100 frames in 10.022488 seconds (9.977562 fps, 6896491.169279 B/s).
capturing 720x480 @ 15FPS
Captured 100 frames in 6.891968 seconds (14.509643 fps, 10029065.232208 B/s).
capturing 720x480 @ 20FPS
Captured 100 frames in 5.234133 seconds (19.105360 fps, 13205624.616211 B/s).
capturing 720x480 @ 30FPS
Captured 100 frames in 3.602298 seconds (27.760055 fps, 19187750.044688 B/s).

capturing 720x576 @ 10FPS
Captured 100 frames in 10.197244 seconds (9.806571 fps, 8133961.937805 B/s).
capturing 720x576 @ 15FPS
Captured 100 frames in 6.925244 seconds (14.439924 fps, 11977050.339261 B/s).
capturing 720x576 @ 20FPS
Captured 100 frames in 4.999968 seconds (20.000127 fps, 16588905.060854 B/s).
capturing 720x576 @ 30FPS
Captured 100 frames in 3.487568 seconds (28.673276 fps, 23782762.085212 B/s).

capturing 1024x768 @ 10FPS
Captured 100 frames in 10.107128 seconds (9.894007 fps, 15561928.174298 B/s).
capturing 1024x768 @ 15FPS
Captured 100 frames in 6.810568 seconds (14.683062 fps, 23094459.169337 B/s).
capturing 1024x768 @ 20FPS
Captured 100 frames in 5.012039 seconds (19.951960 fps, 31381719.096759 B/s).
capturing 1024x768 @ 30FPS
Captured 100 frames in 3.346338 seconds (29.883407 fps, 47002534.905114 B/s).

capturing 1280x720 @ 10FPS
Captured 100 frames in 9.957613 seconds (10.042567 fps, 18510459.665283 B/s).
capturing 1280x720 @ 15FPS
Captured 100 frames in 6.597751 seconds (15.156679 fps, 27936790.986673 B/s).
capturing 1280x720 @ 20FPS
Captured 100 frames in 4.946115 seconds (20.217888 fps, 37265611.495083 B/s).
capturing 1280x720 @ 30FPS
Captured 100 frames in 3.301329 seconds (30.290825 fps, 55832049.080847 B/s).

capturing 1920x1080 @ 10FPS
Captured 100 frames in 10.024745 seconds (9.975316 fps, 41369629.470131 B/s).
capturing 1920x1080 @ 15FPS
Captured 100 frames in 6.674363 seconds (14.982702 fps, 62136260.577244 B/s).
capturing 1920x1080 @ 20FPS
Captured 100 frames in 5.102174 seconds (19.599488 fps, 81282998.172684 B/s).
capturing 1920x1080 @ 30FPS
Captured 100 frames in 3.341157 seconds (29.929746 fps, 124124642.214916 B/s).

capturing 2592x1944 @ 10FPS
Captured 100 frames in 13.019132 seconds (7.681004 fps, 77406819.428913 B/s).
capturing 2592x1944 @ 15FPS
Captured 100 frames in 8.819705 seconds (11.338248 fps, 114263413.559267 B/s).
capturing 2592x1944 @ 20FPS
Captured 100 frames in 6.876134 seconds (14.543055 fps, 146560487.484508 B/s).
capturing 2592x1944 @ 30FPS
Captured 100 frames in 4.359511 seconds (22.938352 fps, 231165743.130365 B/s).
---
 drivers/media/i2c/ov5640.c | 93 +++++++++++++++++++++++-----------------------
 1 file changed, 46 insertions(+), 47 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index c659efe918a4..13b7a0d04840 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -740,8 +740,11 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
  *                         |    +---------------> SCLK 2X
  *                         |  +-------------+
  *                         +->| PCLK Div    | - reg 0x3108, bits 4-5
- *                            +-+-----------+
- *                              +---------------> PCLK
+ *                            ++------------+
+ *                             +  +-----------+
+ *                             +->|   P_DIV   | - reg 0x3035, bits 0-3
+ *                                +-----+-----+
+ *                                       +------------> PCLK
  *
  * This is deviating from the datasheet at least for the register
  * 0x3108, since it's said here that the PCLK would be clocked from
@@ -751,7 +754,6 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
  *  - the PLL pre-divider output rate should be in the 4-27MHz range
  *  - the PLL multiplier output rate should be in the 500-1000MHz range
  *  - PCLK >= SCLK * 2 in YUV, >= SCLK in Raw or JPEG
- *  - MIPI SCLK = (bpp / lanes) / PCLK
  *
  * In the two latter cases, these constraints are met since our
  * factors are hardcoded. If we were to change that, we would need to
@@ -777,10 +779,11 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
 #define OV5640_SYSDIV_MAX	16

 /*
- * This is supposed to be ranging from 1 to 16, but the value is always
- * set to 2 in the vendor kernels.
+ * Hardcode these values for scaler and non-scaler modes.
+ * FIXME: to be re-calcualted for 1 data lanes setups
  */
-#define OV5640_MIPI_DIV		2
+#define OV5640_MIPI_DIV_PCLK	2
+#define OV5640_MIPI_DIV_SCLK	1

 /*
  * This is supposed to be ranging from 1 to 2, but the value is always
@@ -892,65 +895,61 @@ static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
  * ov5640_set_mipi_pclk() - Calculate the clock tree configuration values
  *			    for the MIPI CSI-2 output.
  *
- * @rate: The requested bandwidth in bytes per second.
- *	  It is calculated as: HTOT * VTOT * FPS * bpp
+ * @rate: The requested bandwidth per lane in bytes per second.
+ *	  'Bandwidth Per Lane' is calculated as:
+ *	  bpl = HTOT * VTOT * FPS * bpp / num_lanes;
  *
  * This function use the requested bandwidth to calculate:
- * - sample_rate = bandwidth / bpp;
- * - mipi_clk = bandwidth / num_lanes / 2; ( / 2 for CSI-2 DDR)
+ * - sample_rate = bpl / (bpp / num_lanes);
+ *	         = bpl / (PLL_RDIV * BIT_DIV * PCLK_DIV * MIPI_DIV / num_lanes);
+ *
+ * - mipi_sclk   = bpl / MIPI_DIV / 2; ( / 2 is for CSI-2 DDR)
  *
- * The bandwidth corresponds to the SYSCLK frequency generated by the
- * PLL pre-divider, the PLL multiplier and the SYS divider (see the clock
- * tree documented here above).
+ * with these fixed parameters:
+ *	PLL_RDIV	= 2;
+ *	BIT_DIVIDER	= 2; (MIPI_BIT_MODE == 8 ? 2 : 2,5);
+ *	PCLK_DIV	= 1;
  *
- * From the SYSCLK frequency, the MIPI CSI-2 clock tree generates the
- * pixel clock and the MIPI BIT clock as follows:
+ * The MIPI clock generation differs for modes that use the scaler and modes
+ * that do not. In case the scaler is in use, the MIPI_SCLK generates the MIPI
+ * BIT CLk, and thus:
  *
- * MIPI_BIT_CLK = SYSCLK / MIPI_DIV / 2;
- * PIXEL_CLK = SYSCLK / PLL_RDVI / BIT_DIVIDER / PCLK_DIV / MIPI_DIV
+ * - mipi_sclk = bpl / MIPI_DIV / 2;
+ *   MIPI_DIV = 1;
  *
- * with this fixed parameters:
- * PLL_RDIV	= 2;
- * BIT_DIVIDER	= 2; (MIPI_BIT_MODE == 8 ? 2 : 2,5);
- * PCLK_DIV	= 1;
+ * For modes that do not go through the scaler, the MIPI BIT CLOCK is generated
+ * from the pixel clock, and thus:
  *
- * With these values we have:
+ * - sample_rate = bpl / (bpp / num_lanes);
+ *	         = bpl / (2 * 2 * 1 * MIPI_DIV / num_lanes);
+ *		 = bpl / (4 * MIPI_DIV / num_lanes);
+ * - MIPI_DIV	 = bpp / (4 * num_lanes);
  *
- * pixel_clock = bandwidth / bpp
- * 	       = bandwidth / 4 / MIPI_DIV;
+ * FIXME: this have been tested with 16bpp and 2 lanes setup only.
+ * MIPI_DIV is fixed to value 2, but it -might- be changed according to the
+ * above formula for setups with 1 lane or image formats with different bpp.
  *
- * And so we can calculate MIPI_DIV as:
- * MIPI_DIV = bpp / 4;
+ * FIXME: this deviates from the sensor manual documentation which is quite
+ * thin on the MIPI clock tree generation part.
  */
 static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor,
 				unsigned long rate)
 {
 	const struct ov5640_mode_info *mode = sensor->current_mode;
-	u8 mipi_div = OV5640_MIPI_DIV;
 	u8 prediv, mult, sysdiv;
+	u8 mipi_div;
 	int ret;

-	/* FIXME:
-	 * Practical experience shows we get a correct frame rate by
-	 * halving the bandwidth rate by 2, to slow down SYSCLK frequency.
-	 * Divide both SYSCLK and MIPI_DIV by two (with OV5640_MIPI_DIV
-	 * currently fixed at value '2', while according to the above
-	 * formula it should have been = bpp / 4 = 4).
-	 *
-	 * So that:
-	 * pixel_clock = bandwidth / 2 / bpp
-	 * 	       = bandwidth / 2 / 4 / MIPI_DIV;
-	 * MIPI_DIV = bpp / 4 / 2;
-	 */
-	rate /= 2;
-
-	/* FIXME:
-	 * High resolution modes (1280x720, 1920x1080) requires an higher
-	 * clock speed. Half the MIPI_DIVIDER value to double the output
-	 * pixel clock and MIPI_CLK speeds.
+	/*
+	 * 1280x720 is reported to use 'SUBSAMPLING' only,
+	 * but according to the sensor manual it goes through the
+	 * scaler before subsampling.
 	 */
-	if (mode->hact > 1024)
-		mipi_div /= 2;
+	if (mode->dn_mode == SCALING ||
+	   (mode->id == OV5640_MODE_720P_1280_720))
+		mipi_div = OV5640_MIPI_DIV_SCLK;
+	else
+		mipi_div = OV5640_MIPI_DIV_PCLK;

 	ov5640_calc_sys_clk(sensor, rate, &prediv, &mult, &sysdiv);

--
2.7.4

Re: [PATCH 0/2] media: ov5640: MIPI fixes on top of Maxime's v5

[Adam Ford]

On Thu, Nov 29, 2018 at 8:48 AM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
>
> Hello ov5640-ers,
>    these two patches should be applied on top of Maxime's clock tree rework v5
> and 'fix' MIPI CSI-2 clock tree configuration.
>
> The first patch is a fix that appeard in various forms on the list several
> times: if the image sizes gets updated but not the image format, the size
> update gets ignored. I had to fix this to run my FPS tests, and thus I'm
> sending the two together. I wish in future a re-work of the image format
> handling part, but for now, let's just fix it for v4.21.
>
> The second patch slightly reworks the MIPI clock tree configuration, based on
> inputs from Sam. The currently submitted v5 in which Maxime squashed my previous
> changes is 'broken'. That's my bad, as explained in the patch change log.
>
> Test results are attacched to patch [2/2].
> Changelog for [2/2] is included in the patch itself.
>
> I wish these patches to go in with Maxime awesome clock tree re-work, pending
> his ack.
>
> Also, I have tested with an i.MX6Q board, with a CSI-2 2 data lanes setup. There
> are still a few things not clear to me in the MIPI clock tree, and I welcome
> more testing, preferibly with 1-lanes setups.
>
> Also, I had to re-apply Maxime's series and latest ov5640 patches on v4.19,
> as my test board is sort of broken with v4.20-rcX (it shouldn't make any
> difference in regard to this series, but I'm pointing it out anyhow).

Greg KH is pulling in some of the first round of fixes to the 4.19.y
branch.  If they're working, we may want to consider having Greg Apply
these there as well.

>
> Recently Adam has been testing quite some ov5640 patches, if you fill like
> testing these as well on your setup (which I understand is a MIPI CSI-2 one)
> please report the results. Same for all other interested ones :)

You are correct, I have an i.MX6 with CSI2 interface.  I'll test
against 4.20-RCx and my 4.19 version with some of the newer patches
pulled down.  I will try to get to these before Monday.

adam

>
> Thanks
>   j
>
> Jacopo Mondi (2):
>   media: i2c: ov5640: Fix set format regression
>   media: ov5640: make MIPI clock depend on mode
>
>  drivers/media/i2c/ov5640.c | 110 ++++++++++++++++++++++-----------------------
>  1 file changed, 54 insertions(+), 56 deletions(-)
>
> --
> 2.7.4
>

Re: [PATCH 2/2] media: ov5640: make MIPI clock depend on mode

[Adam Ford]

On Thu, Nov 29, 2018 at 8:49 AM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
>
> The MIPI clock generation tree uses the MIPI_DIV divider to generate both the
> MIPI SCALER CLOCK and the PIXEL_CLOCK. It seems the MIPI BIT CLK frequency is
> generated by either the MIPI SCALER or the PIXEL CLOCK, depending if the
> currently applied image mode uses the scaler or not.
>
> Make the MIPI_DIV selection value depend on the subsampling mode used by the
> currently applied image mode.
>
> Tested with:
> 172x144 320x240, 640x480, 1024x756, 1024x768, 1280x720, 1920x1080 in YUYV mode
> at 10, 15, 20, and 30 FPS with MIPI CSI-2 2 lanes mode.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

I am not able to apply this patch to 4.19.6, 4.19-RC5, or Linux-media master [1]

Is there a specific branch/repo somewhere I can pull?  I was able to
apply patch 1/2 just fine, but 2/2 wouldn't apply

[1] - git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media.git

adam
> ---
> Maxime,
>    this patch is not just a cosmetic updates to the 'set_mipi_pclk()'
> comment block as I anticipated, but it actually fix the framerate handling
> compared for CSI-2, which in you v5 results halved for most modes. I have
> not included any "Fixes:" tag, as I hope this patch will get in with your v5.
>
> That's my bad, as in the patches I sent to be applied on top of your v4 I
> forgot to add a change, and the requested SCLK rate was always half of what
> it was actually required.
>
> Also, I have left out from this patches most of Sam's improvements (better SCLK
> selection policies, and programming of register OV5640_REG_PCLK_PERIOD as for
> from testing they're not required, and I don't want to pile more patches than
> necessary for the next merge window, not to slow down the clock tree rework
> inclusion. We can get back to it after it got merged.
>
> This are the test result obtained by capturing 100 frames with yavta and
> inspecting the reported fps results.
>
> Results are pretty neat, 2592x1944 mode apart, which runs a bit slow.
>
> capturing 176x144 @ 10FPS
> Captured 100 frames in 10.019398 seconds (9.980640 fps, 505898.657683 B/s).
> capturing 176x144 @ 15FPS
> Captured 100 frames in 6.681860 seconds (14.965892 fps, 758591.132803 B/s).
> capturing 176x144 @ 20FPS
> Captured 100 frames in 5.056204 seconds (19.777683 fps, 1002491.196755 B/s).
> capturing 176x144 @ 30FPS
> Captured 100 frames in 3.357204 seconds (29.786686 fps, 1509827.521040 B/s).
>
> capturing 320x240 @ 10FPS
> Captured 100 frames in 10.015432 seconds (9.984591 fps, 1533633.245951 B/s).
> capturing 320x240 @ 15FPS
> Captured 100 frames in 6.684035 seconds (14.961021 fps, 2298012.872049 B/s).
> capturing 320x240 @ 20FPS
> Captured 100 frames in 5.019164 seconds (19.923635 fps, 3060270.391218 B/s).
> capturing 320x240 @ 30FPS
> Captured 100 frames in 3.352991 seconds (29.824115 fps, 4580984.103432 B/s).
>
> capturing 640x480 @ 10FPS
> Captured 100 frames in 9.990389 seconds (10.009620 fps, 6149910.678538 B/s).
> capturing 640x480 @ 15FPS
> Captured 100 frames in 6.856242 seconds (14.585249 fps, 8961176.838123 B/s).
> capturing 640x480 @ 20FPS
> Captured 100 frames in 5.030264 seconds (19.879670 fps, 12214069.053476 B/s).
> capturing 640x480 @ 30FPS
> Captured 100 frames in 3.364612 seconds (29.721103 fps, 18260645.750580 B/s).
>
> capturing 720x480 @ 10FPS
> Captured 100 frames in 10.022488 seconds (9.977562 fps, 6896491.169279 B/s).
> capturing 720x480 @ 15FPS
> Captured 100 frames in 6.891968 seconds (14.509643 fps, 10029065.232208 B/s).
> capturing 720x480 @ 20FPS
> Captured 100 frames in 5.234133 seconds (19.105360 fps, 13205624.616211 B/s).
> capturing 720x480 @ 30FPS
> Captured 100 frames in 3.602298 seconds (27.760055 fps, 19187750.044688 B/s).
>
> capturing 720x576 @ 10FPS
> Captured 100 frames in 10.197244 seconds (9.806571 fps, 8133961.937805 B/s).
> capturing 720x576 @ 15FPS
> Captured 100 frames in 6.925244 seconds (14.439924 fps, 11977050.339261 B/s).
> capturing 720x576 @ 20FPS
> Captured 100 frames in 4.999968 seconds (20.000127 fps, 16588905.060854 B/s).
> capturing 720x576 @ 30FPS
> Captured 100 frames in 3.487568 seconds (28.673276 fps, 23782762.085212 B/s).
>
> capturing 1024x768 @ 10FPS
> Captured 100 frames in 10.107128 seconds (9.894007 fps, 15561928.174298 B/s).
> capturing 1024x768 @ 15FPS
> Captured 100 frames in 6.810568 seconds (14.683062 fps, 23094459.169337 B/s).
> capturing 1024x768 @ 20FPS
> Captured 100 frames in 5.012039 seconds (19.951960 fps, 31381719.096759 B/s).
> capturing 1024x768 @ 30FPS
> Captured 100 frames in 3.346338 seconds (29.883407 fps, 47002534.905114 B/s).
>
> capturing 1280x720 @ 10FPS
> Captured 100 frames in 9.957613 seconds (10.042567 fps, 18510459.665283 B/s).
> capturing 1280x720 @ 15FPS
> Captured 100 frames in 6.597751 seconds (15.156679 fps, 27936790.986673 B/s).
> capturing 1280x720 @ 20FPS
> Captured 100 frames in 4.946115 seconds (20.217888 fps, 37265611.495083 B/s).
> capturing 1280x720 @ 30FPS
> Captured 100 frames in 3.301329 seconds (30.290825 fps, 55832049.080847 B/s).
>
> capturing 1920x1080 @ 10FPS
> Captured 100 frames in 10.024745 seconds (9.975316 fps, 41369629.470131 B/s).
> capturing 1920x1080 @ 15FPS
> Captured 100 frames in 6.674363 seconds (14.982702 fps, 62136260.577244 B/s).
> capturing 1920x1080 @ 20FPS
> Captured 100 frames in 5.102174 seconds (19.599488 fps, 81282998.172684 B/s).
> capturing 1920x1080 @ 30FPS
> Captured 100 frames in 3.341157 seconds (29.929746 fps, 124124642.214916 B/s).
>
> capturing 2592x1944 @ 10FPS
> Captured 100 frames in 13.019132 seconds (7.681004 fps, 77406819.428913 B/s).
> capturing 2592x1944 @ 15FPS
> Captured 100 frames in 8.819705 seconds (11.338248 fps, 114263413.559267 B/s).
> capturing 2592x1944 @ 20FPS
> Captured 100 frames in 6.876134 seconds (14.543055 fps, 146560487.484508 B/s).
> capturing 2592x1944 @ 30FPS
> Captured 100 frames in 4.359511 seconds (22.938352 fps, 231165743.130365 B/s).
> ---
>  drivers/media/i2c/ov5640.c | 93 +++++++++++++++++++++++-----------------------
>  1 file changed, 46 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index c659efe918a4..13b7a0d04840 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -740,8 +740,11 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
>   *                         |    +---------------> SCLK 2X
>   *                         |  +-------------+
>   *                         +->| PCLK Div    | - reg 0x3108, bits 4-5
> - *                            +-+-----------+
> - *                              +---------------> PCLK
> + *                            ++------------+
> + *                             +  +-----------+
> + *                             +->|   P_DIV   | - reg 0x3035, bits 0-3
> + *                                +-----+-----+
> + *                                       +------------> PCLK
>   *
>   * This is deviating from the datasheet at least for the register
>   * 0x3108, since it's said here that the PCLK would be clocked from
> @@ -751,7 +754,6 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
>   *  - the PLL pre-divider output rate should be in the 4-27MHz range
>   *  - the PLL multiplier output rate should be in the 500-1000MHz range
>   *  - PCLK >= SCLK * 2 in YUV, >= SCLK in Raw or JPEG
> - *  - MIPI SCLK = (bpp / lanes) / PCLK
>   *
>   * In the two latter cases, these constraints are met since our
>   * factors are hardcoded. If we were to change that, we would need to
> @@ -777,10 +779,11 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
>  #define OV5640_SYSDIV_MAX      16
>
>  /*
> - * This is supposed to be ranging from 1 to 16, but the value is always
> - * set to 2 in the vendor kernels.
> + * Hardcode these values for scaler and non-scaler modes.
> + * FIXME: to be re-calcualted for 1 data lanes setups
>   */
> -#define OV5640_MIPI_DIV                2
> +#define OV5640_MIPI_DIV_PCLK   2
> +#define OV5640_MIPI_DIV_SCLK   1
>
>  /*
>   * This is supposed to be ranging from 1 to 2, but the value is always
> @@ -892,65 +895,61 @@ static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
>   * ov5640_set_mipi_pclk() - Calculate the clock tree configuration values
>   *                         for the MIPI CSI-2 output.
>   *
> - * @rate: The requested bandwidth in bytes per second.
> - *       It is calculated as: HTOT * VTOT * FPS * bpp
> + * @rate: The requested bandwidth per lane in bytes per second.
> + *       'Bandwidth Per Lane' is calculated as:
> + *       bpl = HTOT * VTOT * FPS * bpp / num_lanes;
>   *
>   * This function use the requested bandwidth to calculate:
> - * - sample_rate = bandwidth / bpp;
> - * - mipi_clk = bandwidth / num_lanes / 2; ( / 2 for CSI-2 DDR)
> + * - sample_rate = bpl / (bpp / num_lanes);
> + *              = bpl / (PLL_RDIV * BIT_DIV * PCLK_DIV * MIPI_DIV / num_lanes);
> + *
> + * - mipi_sclk   = bpl / MIPI_DIV / 2; ( / 2 is for CSI-2 DDR)
>   *
> - * The bandwidth corresponds to the SYSCLK frequency generated by the
> - * PLL pre-divider, the PLL multiplier and the SYS divider (see the clock
> - * tree documented here above).
> + * with these fixed parameters:
> + *     PLL_RDIV        = 2;
> + *     BIT_DIVIDER     = 2; (MIPI_BIT_MODE == 8 ? 2 : 2,5);
> + *     PCLK_DIV        = 1;
>   *
> - * From the SYSCLK frequency, the MIPI CSI-2 clock tree generates the
> - * pixel clock and the MIPI BIT clock as follows:
> + * The MIPI clock generation differs for modes that use the scaler and modes
> + * that do not. In case the scaler is in use, the MIPI_SCLK generates the MIPI
> + * BIT CLk, and thus:
>   *
> - * MIPI_BIT_CLK = SYSCLK / MIPI_DIV / 2;
> - * PIXEL_CLK = SYSCLK / PLL_RDVI / BIT_DIVIDER / PCLK_DIV / MIPI_DIV
> + * - mipi_sclk = bpl / MIPI_DIV / 2;
> + *   MIPI_DIV = 1;
>   *
> - * with this fixed parameters:
> - * PLL_RDIV    = 2;
> - * BIT_DIVIDER = 2; (MIPI_BIT_MODE == 8 ? 2 : 2,5);
> - * PCLK_DIV    = 1;
> + * For modes that do not go through the scaler, the MIPI BIT CLOCK is generated
> + * from the pixel clock, and thus:
>   *
> - * With these values we have:
> + * - sample_rate = bpl / (bpp / num_lanes);
> + *              = bpl / (2 * 2 * 1 * MIPI_DIV / num_lanes);
> + *              = bpl / (4 * MIPI_DIV / num_lanes);
> + * - MIPI_DIV   = bpp / (4 * num_lanes);
>   *
> - * pixel_clock = bandwidth / bpp
> - *            = bandwidth / 4 / MIPI_DIV;
> + * FIXME: this have been tested with 16bpp and 2 lanes setup only.
> + * MIPI_DIV is fixed to value 2, but it -might- be changed according to the
> + * above formula for setups with 1 lane or image formats with different bpp.
>   *
> - * And so we can calculate MIPI_DIV as:
> - * MIPI_DIV = bpp / 4;
> + * FIXME: this deviates from the sensor manual documentation which is quite
> + * thin on the MIPI clock tree generation part.
>   */
>  static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor,
>                                 unsigned long rate)
>  {
>         const struct ov5640_mode_info *mode = sensor->current_mode;
> -       u8 mipi_div = OV5640_MIPI_DIV;
>         u8 prediv, mult, sysdiv;
> +       u8 mipi_div;
>         int ret;
>
> -       /* FIXME:
> -        * Practical experience shows we get a correct frame rate by
> -        * halving the bandwidth rate by 2, to slow down SYSCLK frequency.
> -        * Divide both SYSCLK and MIPI_DIV by two (with OV5640_MIPI_DIV
> -        * currently fixed at value '2', while according to the above
> -        * formula it should have been = bpp / 4 = 4).
> -        *
> -        * So that:
> -        * pixel_clock = bandwidth / 2 / bpp
> -        *             = bandwidth / 2 / 4 / MIPI_DIV;
> -        * MIPI_DIV = bpp / 4 / 2;
> -        */
> -       rate /= 2;
> -
> -       /* FIXME:
> -        * High resolution modes (1280x720, 1920x1080) requires an higher
> -        * clock speed. Half the MIPI_DIVIDER value to double the output
> -        * pixel clock and MIPI_CLK speeds.
> +       /*
> +        * 1280x720 is reported to use 'SUBSAMPLING' only,
> +        * but according to the sensor manual it goes through the
> +        * scaler before subsampling.
>          */
> -       if (mode->hact > 1024)
> -               mipi_div /= 2;
> +       if (mode->dn_mode == SCALING ||
> +          (mode->id == OV5640_MODE_720P_1280_720))
> +               mipi_div = OV5640_MIPI_DIV_SCLK;
> +       else
> +               mipi_div = OV5640_MIPI_DIV_PCLK;
>
>         ov5640_calc_sys_clk(sensor, rate, &prediv, &mult, &sysdiv);
>
> --
> 2.7.4
>

Re: [PATCH 1/2] media: ov5640: Fix set format regression

[Adam Ford]

On Thu, Nov 29, 2018 at 8:48 AM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
>
> The set_fmt operations updates the sensor format only when the image format
> is changed. When only the image sizes gets changed, the format do not get
> updated causing the sensor to always report the one that was previously in
> use.
>
> Without this patch, updating frame size only fails:
>   [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb xfer:srgb ...]
>
> With this patch applied:
>   [fmt:UYVY8_2X8/1024x768@1/30 field:none colorspace:srgb xfer:srgb ...]
>
> Fixes: 6949d864776e ("media: ov5640: do not change mode if format or frame
> interval is unchanged")
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

For patch 1 of 2 only,

Tested-by: Adam Ford <aford173@gmail.com>    #imx6d with CSI2
interface on 4.19.6 and 4.20-RC5

It would be great if this could be applied to 4.19+

adam
> ---
>  drivers/media/i2c/ov5640.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 03a031a42b3e..c659efe918a4 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -2160,6 +2160,7 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
>         struct ov5640_dev *sensor = to_ov5640_dev(sd);
>         const struct ov5640_mode_info *new_mode;
>         struct v4l2_mbus_framefmt *mbus_fmt = &format->format;
> +       struct v4l2_mbus_framefmt *fmt;
>         int ret;
>
>         if (format->pad != 0)
> @@ -2177,22 +2178,20 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
>         if (ret)
>                 goto out;
>
> -       if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -               struct v4l2_mbus_framefmt *fmt =
> -                       v4l2_subdev_get_try_format(sd, cfg, 0);
> +       if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> +               fmt = v4l2_subdev_get_try_format(sd, cfg, 0);
> +       else
> +               fmt = &sensor->fmt;
>
> -               *fmt = *mbus_fmt;
> -               goto out;
> -       }
> +       *fmt = *mbus_fmt;
>
>         if (new_mode != sensor->current_mode) {
>                 sensor->current_mode = new_mode;
>                 sensor->pending_mode_change = true;
>         }
> -       if (mbus_fmt->code != sensor->fmt.code) {
> -               sensor->fmt = *mbus_fmt;
> +       if (mbus_fmt->code != sensor->fmt.code)
>                 sensor->pending_fmt_change = true;
> -       }
> +
>  out:
>         mutex_unlock(&sensor->lock);
>         return ret;
> --
> 2.7.4
>

Re: [PATCH 2/2] media: ov5640: make MIPI clock depend on mode

[Adam Ford]

On Mon, Dec 3, 2018 at 10:28 AM Adam Ford <aford173@gmail.com> wrote:
>
> On Thu, Nov 29, 2018 at 8:49 AM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> >
> > The MIPI clock generation tree uses the MIPI_DIV divider to generate both the
> > MIPI SCALER CLOCK and the PIXEL_CLOCK. It seems the MIPI BIT CLK frequency is
> > generated by either the MIPI SCALER or the PIXEL CLOCK, depending if the
> > currently applied image mode uses the scaler or not.
> >
> > Make the MIPI_DIV selection value depend on the subsampling mode used by the
> > currently applied image mode.
> >
> > Tested with:
> > 172x144 320x240, 640x480, 1024x756, 1024x768, 1280x720, 1920x1080 in YUYV mode
> > at 10, 15, 20, and 30 FPS with MIPI CSI-2 2 lanes mode.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> I am not able to apply this patch to 4.19.6, 4.19-RC5, or Linux-media master [1]
>
> Is there a specific branch/repo somewhere I can pull?  I was able to
> apply patch 1/2 just fine, but 2/2 wouldn't apply
>

I also attempted to apply to [2] without success.

> [1] - git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media.git

[2] - git://linuxtv.org/media_tree.git

>
> adam
> > ---
> > Maxime,
> >    this patch is not just a cosmetic updates to the 'set_mipi_pclk()'
> > comment block as I anticipated, but it actually fix the framerate handling
> > compared for CSI-2, which in you v5 results halved for most modes. I have
> > not included any "Fixes:" tag, as I hope this patch will get in with your v5.
> >
> > That's my bad, as in the patches I sent to be applied on top of your v4 I
> > forgot to add a change, and the requested SCLK rate was always half of what
> > it was actually required.
> >
> > Also, I have left out from this patches most of Sam's improvements (better SCLK
> > selection policies, and programming of register OV5640_REG_PCLK_PERIOD as for
> > from testing they're not required, and I don't want to pile more patches than
> > necessary for the next merge window, not to slow down the clock tree rework
> > inclusion. We can get back to it after it got merged.
> >
> > This are the test result obtained by capturing 100 frames with yavta and
> > inspecting the reported fps results.
> >
> > Results are pretty neat, 2592x1944 mode apart, which runs a bit slow.
> >
> > capturing 176x144 @ 10FPS
> > Captured 100 frames in 10.019398 seconds (9.980640 fps, 505898.657683 B/s).
> > capturing 176x144 @ 15FPS
> > Captured 100 frames in 6.681860 seconds (14.965892 fps, 758591.132803 B/s).
> > capturing 176x144 @ 20FPS
> > Captured 100 frames in 5.056204 seconds (19.777683 fps, 1002491.196755 B/s).
> > capturing 176x144 @ 30FPS
> > Captured 100 frames in 3.357204 seconds (29.786686 fps, 1509827.521040 B/s).
> >
> > capturing 320x240 @ 10FPS
> > Captured 100 frames in 10.015432 seconds (9.984591 fps, 1533633.245951 B/s).
> > capturing 320x240 @ 15FPS
> > Captured 100 frames in 6.684035 seconds (14.961021 fps, 2298012.872049 B/s).
> > capturing 320x240 @ 20FPS
> > Captured 100 frames in 5.019164 seconds (19.923635 fps, 3060270.391218 B/s).
> > capturing 320x240 @ 30FPS
> > Captured 100 frames in 3.352991 seconds (29.824115 fps, 4580984.103432 B/s).
> >
> > capturing 640x480 @ 10FPS
> > Captured 100 frames in 9.990389 seconds (10.009620 fps, 6149910.678538 B/s).
> > capturing 640x480 @ 15FPS
> > Captured 100 frames in 6.856242 seconds (14.585249 fps, 8961176.838123 B/s).
> > capturing 640x480 @ 20FPS
> > Captured 100 frames in 5.030264 seconds (19.879670 fps, 12214069.053476 B/s).
> > capturing 640x480 @ 30FPS
> > Captured 100 frames in 3.364612 seconds (29.721103 fps, 18260645.750580 B/s).
> >
> > capturing 720x480 @ 10FPS
> > Captured 100 frames in 10.022488 seconds (9.977562 fps, 6896491.169279 B/s).
> > capturing 720x480 @ 15FPS
> > Captured 100 frames in 6.891968 seconds (14.509643 fps, 10029065.232208 B/s).
> > capturing 720x480 @ 20FPS
> > Captured 100 frames in 5.234133 seconds (19.105360 fps, 13205624.616211 B/s).
> > capturing 720x480 @ 30FPS
> > Captured 100 frames in 3.602298 seconds (27.760055 fps, 19187750.044688 B/s).
> >
> > capturing 720x576 @ 10FPS
> > Captured 100 frames in 10.197244 seconds (9.806571 fps, 8133961.937805 B/s).
> > capturing 720x576 @ 15FPS
> > Captured 100 frames in 6.925244 seconds (14.439924 fps, 11977050.339261 B/s).
> > capturing 720x576 @ 20FPS
> > Captured 100 frames in 4.999968 seconds (20.000127 fps, 16588905.060854 B/s).
> > capturing 720x576 @ 30FPS
> > Captured 100 frames in 3.487568 seconds (28.673276 fps, 23782762.085212 B/s).
> >
> > capturing 1024x768 @ 10FPS
> > Captured 100 frames in 10.107128 seconds (9.894007 fps, 15561928.174298 B/s).
> > capturing 1024x768 @ 15FPS
> > Captured 100 frames in 6.810568 seconds (14.683062 fps, 23094459.169337 B/s).
> > capturing 1024x768 @ 20FPS
> > Captured 100 frames in 5.012039 seconds (19.951960 fps, 31381719.096759 B/s).
> > capturing 1024x768 @ 30FPS
> > Captured 100 frames in 3.346338 seconds (29.883407 fps, 47002534.905114 B/s).
> >
> > capturing 1280x720 @ 10FPS
> > Captured 100 frames in 9.957613 seconds (10.042567 fps, 18510459.665283 B/s).
> > capturing 1280x720 @ 15FPS
> > Captured 100 frames in 6.597751 seconds (15.156679 fps, 27936790.986673 B/s).
> > capturing 1280x720 @ 20FPS
> > Captured 100 frames in 4.946115 seconds (20.217888 fps, 37265611.495083 B/s).
> > capturing 1280x720 @ 30FPS
> > Captured 100 frames in 3.301329 seconds (30.290825 fps, 55832049.080847 B/s).
> >
> > capturing 1920x1080 @ 10FPS
> > Captured 100 frames in 10.024745 seconds (9.975316 fps, 41369629.470131 B/s).
> > capturing 1920x1080 @ 15FPS
> > Captured 100 frames in 6.674363 seconds (14.982702 fps, 62136260.577244 B/s).
> > capturing 1920x1080 @ 20FPS
> > Captured 100 frames in 5.102174 seconds (19.599488 fps, 81282998.172684 B/s).
> > capturing 1920x1080 @ 30FPS
> > Captured 100 frames in 3.341157 seconds (29.929746 fps, 124124642.214916 B/s).
> >
> > capturing 2592x1944 @ 10FPS
> > Captured 100 frames in 13.019132 seconds (7.681004 fps, 77406819.428913 B/s).
> > capturing 2592x1944 @ 15FPS
> > Captured 100 frames in 8.819705 seconds (11.338248 fps, 114263413.559267 B/s).
> > capturing 2592x1944 @ 20FPS
> > Captured 100 frames in 6.876134 seconds (14.543055 fps, 146560487.484508 B/s).
> > capturing 2592x1944 @ 30FPS
> > Captured 100 frames in 4.359511 seconds (22.938352 fps, 231165743.130365 B/s).
> > ---
> >  drivers/media/i2c/ov5640.c | 93 +++++++++++++++++++++++-----------------------
> >  1 file changed, 46 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index c659efe918a4..13b7a0d04840 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -740,8 +740,11 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
> >   *                         |    +---------------> SCLK 2X
> >   *                         |  +-------------+
> >   *                         +->| PCLK Div    | - reg 0x3108, bits 4-5
> > - *                            +-+-----------+
> > - *                              +---------------> PCLK
> > + *                            ++------------+
> > + *                             +  +-----------+
> > + *                             +->|   P_DIV   | - reg 0x3035, bits 0-3
> > + *                                +-----+-----+
> > + *                                       +------------> PCLK
> >   *
> >   * This is deviating from the datasheet at least for the register
> >   * 0x3108, since it's said here that the PCLK would be clocked from
> > @@ -751,7 +754,6 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
> >   *  - the PLL pre-divider output rate should be in the 4-27MHz range
> >   *  - the PLL multiplier output rate should be in the 500-1000MHz range
> >   *  - PCLK >= SCLK * 2 in YUV, >= SCLK in Raw or JPEG
> > - *  - MIPI SCLK = (bpp / lanes) / PCLK
> >   *
> >   * In the two latter cases, these constraints are met since our
> >   * factors are hardcoded. If we were to change that, we would need to
> > @@ -777,10 +779,11 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
> >  #define OV5640_SYSDIV_MAX      16
> >
> >  /*
> > - * This is supposed to be ranging from 1 to 16, but the value is always
> > - * set to 2 in the vendor kernels.
> > + * Hardcode these values for scaler and non-scaler modes.
> > + * FIXME: to be re-calcualted for 1 data lanes setups
> >   */
> > -#define OV5640_MIPI_DIV                2
> > +#define OV5640_MIPI_DIV_PCLK   2
> > +#define OV5640_MIPI_DIV_SCLK   1
> >
> >  /*
> >   * This is supposed to be ranging from 1 to 2, but the value is always
> > @@ -892,65 +895,61 @@ static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
> >   * ov5640_set_mipi_pclk() - Calculate the clock tree configuration values
> >   *                         for the MIPI CSI-2 output.
> >   *
> > - * @rate: The requested bandwidth in bytes per second.
> > - *       It is calculated as: HTOT * VTOT * FPS * bpp
> > + * @rate: The requested bandwidth per lane in bytes per second.
> > + *       'Bandwidth Per Lane' is calculated as:
> > + *       bpl = HTOT * VTOT * FPS * bpp / num_lanes;
> >   *
> >   * This function use the requested bandwidth to calculate:
> > - * - sample_rate = bandwidth / bpp;
> > - * - mipi_clk = bandwidth / num_lanes / 2; ( / 2 for CSI-2 DDR)
> > + * - sample_rate = bpl / (bpp / num_lanes);
> > + *              = bpl / (PLL_RDIV * BIT_DIV * PCLK_DIV * MIPI_DIV / num_lanes);
> > + *
> > + * - mipi_sclk   = bpl / MIPI_DIV / 2; ( / 2 is for CSI-2 DDR)
> >   *
> > - * The bandwidth corresponds to the SYSCLK frequency generated by the
> > - * PLL pre-divider, the PLL multiplier and the SYS divider (see the clock
> > - * tree documented here above).
> > + * with these fixed parameters:
> > + *     PLL_RDIV        = 2;
> > + *     BIT_DIVIDER     = 2; (MIPI_BIT_MODE == 8 ? 2 : 2,5);
> > + *     PCLK_DIV        = 1;
> >   *
> > - * From the SYSCLK frequency, the MIPI CSI-2 clock tree generates the
> > - * pixel clock and the MIPI BIT clock as follows:
> > + * The MIPI clock generation differs for modes that use the scaler and modes
> > + * that do not. In case the scaler is in use, the MIPI_SCLK generates the MIPI
> > + * BIT CLk, and thus:
> >   *
> > - * MIPI_BIT_CLK = SYSCLK / MIPI_DIV / 2;
> > - * PIXEL_CLK = SYSCLK / PLL_RDVI / BIT_DIVIDER / PCLK_DIV / MIPI_DIV
> > + * - mipi_sclk = bpl / MIPI_DIV / 2;
> > + *   MIPI_DIV = 1;
> >   *
> > - * with this fixed parameters:
> > - * PLL_RDIV    = 2;
> > - * BIT_DIVIDER = 2; (MIPI_BIT_MODE == 8 ? 2 : 2,5);
> > - * PCLK_DIV    = 1;
> > + * For modes that do not go through the scaler, the MIPI BIT CLOCK is generated
> > + * from the pixel clock, and thus:
> >   *
> > - * With these values we have:
> > + * - sample_rate = bpl / (bpp / num_lanes);
> > + *              = bpl / (2 * 2 * 1 * MIPI_DIV / num_lanes);
> > + *              = bpl / (4 * MIPI_DIV / num_lanes);
> > + * - MIPI_DIV   = bpp / (4 * num_lanes);
> >   *
> > - * pixel_clock = bandwidth / bpp
> > - *            = bandwidth / 4 / MIPI_DIV;
> > + * FIXME: this have been tested with 16bpp and 2 lanes setup only.
> > + * MIPI_DIV is fixed to value 2, but it -might- be changed according to the
> > + * above formula for setups with 1 lane or image formats with different bpp.
> >   *
> > - * And so we can calculate MIPI_DIV as:
> > - * MIPI_DIV = bpp / 4;
> > + * FIXME: this deviates from the sensor manual documentation which is quite
> > + * thin on the MIPI clock tree generation part.
> >   */
> >  static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor,
> >                                 unsigned long rate)
> >  {
> >         const struct ov5640_mode_info *mode = sensor->current_mode;
> > -       u8 mipi_div = OV5640_MIPI_DIV;
> >         u8 prediv, mult, sysdiv;
> > +       u8 mipi_div;
> >         int ret;
> >
> > -       /* FIXME:
> > -        * Practical experience shows we get a correct frame rate by
> > -        * halving the bandwidth rate by 2, to slow down SYSCLK frequency.
> > -        * Divide both SYSCLK and MIPI_DIV by two (with OV5640_MIPI_DIV
> > -        * currently fixed at value '2', while according to the above
> > -        * formula it should have been = bpp / 4 = 4).
> > -        *
> > -        * So that:
> > -        * pixel_clock = bandwidth / 2 / bpp
> > -        *             = bandwidth / 2 / 4 / MIPI_DIV;
> > -        * MIPI_DIV = bpp / 4 / 2;
> > -        */
> > -       rate /= 2;
> > -
> > -       /* FIXME:
> > -        * High resolution modes (1280x720, 1920x1080) requires an higher
> > -        * clock speed. Half the MIPI_DIVIDER value to double the output
> > -        * pixel clock and MIPI_CLK speeds.
> > +       /*
> > +        * 1280x720 is reported to use 'SUBSAMPLING' only,
> > +        * but according to the sensor manual it goes through the
> > +        * scaler before subsampling.
> >          */
> > -       if (mode->hact > 1024)
> > -               mipi_div /= 2;
> > +       if (mode->dn_mode == SCALING ||
> > +          (mode->id == OV5640_MODE_720P_1280_720))
> > +               mipi_div = OV5640_MIPI_DIV_SCLK;
> > +       else
> > +               mipi_div = OV5640_MIPI_DIV_PCLK;
> >
> >         ov5640_calc_sys_clk(sensor, rate, &prediv, &mult, &sysdiv);
> >
> > --
> > 2.7.4
> >

Re: [PATCH 2/2] media: ov5640: make MIPI clock depend on mode

[jacopo mondi]

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

Hi Adam,
    thanks for testing

On Mon, Dec 03, 2018 at 10:28:04AM -0600, Adam Ford wrote:
> On Thu, Nov 29, 2018 at 8:49 AM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> >
> > The MIPI clock generation tree uses the MIPI_DIV divider to generate both the
> > MIPI SCALER CLOCK and the PIXEL_CLOCK. It seems the MIPI BIT CLK frequency is
> > generated by either the MIPI SCALER or the PIXEL CLOCK, depending if the
> > currently applied image mode uses the scaler or not.
> >
> > Make the MIPI_DIV selection value depend on the subsampling mode used by the
> > currently applied image mode.
> >
> > Tested with:
> > 172x144 320x240, 640x480, 1024x756, 1024x768, 1280x720, 1920x1080 in YUYV mode
> > at 10, 15, 20, and 30 FPS with MIPI CSI-2 2 lanes mode.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> I am not able to apply this patch to 4.19.6, 4.19-RC5, or Linux-media master [1]
>
> Is there a specific branch/repo somewhere I can pull?  I was able to
> apply patch 1/2 just fine, but 2/2 wouldn't apply

Reading the cover letter:
"these two patches should be applied on top of Maxime's clock tree rework v5"

Maxime has included those 2 in his v6. You may want to test that one
:)

Thanks
   j

>
> [1] - git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media.git
>
> adam
> > ---
> > Maxime,
> >    this patch is not just a cosmetic updates to the 'set_mipi_pclk()'
> > comment block as I anticipated, but it actually fix the framerate handling
> > compared for CSI-2, which in you v5 results halved for most modes. I have
> > not included any "Fixes:" tag, as I hope this patch will get in with your v5.
> >
> > That's my bad, as in the patches I sent to be applied on top of your v4 I
> > forgot to add a change, and the requested SCLK rate was always half of what
> > it was actually required.
> >
> > Also, I have left out from this patches most of Sam's improvements (better SCLK
> > selection policies, and programming of register OV5640_REG_PCLK_PERIOD as for
> > from testing they're not required, and I don't want to pile more patches than
> > necessary for the next merge window, not to slow down the clock tree rework
> > inclusion. We can get back to it after it got merged.
> >
> > This are the test result obtained by capturing 100 frames with yavta and
> > inspecting the reported fps results.
> >
> > Results are pretty neat, 2592x1944 mode apart, which runs a bit slow.
> >
> > capturing 176x144 @ 10FPS
> > Captured 100 frames in 10.019398 seconds (9.980640 fps, 505898.657683 B/s).
> > capturing 176x144 @ 15FPS
> > Captured 100 frames in 6.681860 seconds (14.965892 fps, 758591.132803 B/s).
> > capturing 176x144 @ 20FPS
> > Captured 100 frames in 5.056204 seconds (19.777683 fps, 1002491.196755 B/s).
> > capturing 176x144 @ 30FPS
> > Captured 100 frames in 3.357204 seconds (29.786686 fps, 1509827.521040 B/s).
> >
> > capturing 320x240 @ 10FPS
> > Captured 100 frames in 10.015432 seconds (9.984591 fps, 1533633.245951 B/s).
> > capturing 320x240 @ 15FPS
> > Captured 100 frames in 6.684035 seconds (14.961021 fps, 2298012.872049 B/s).
> > capturing 320x240 @ 20FPS
> > Captured 100 frames in 5.019164 seconds (19.923635 fps, 3060270.391218 B/s).
> > capturing 320x240 @ 30FPS
> > Captured 100 frames in 3.352991 seconds (29.824115 fps, 4580984.103432 B/s).
> >
> > capturing 640x480 @ 10FPS
> > Captured 100 frames in 9.990389 seconds (10.009620 fps, 6149910.678538 B/s).
> > capturing 640x480 @ 15FPS
> > Captured 100 frames in 6.856242 seconds (14.585249 fps, 8961176.838123 B/s).
> > capturing 640x480 @ 20FPS
> > Captured 100 frames in 5.030264 seconds (19.879670 fps, 12214069.053476 B/s).
> > capturing 640x480 @ 30FPS
> > Captured 100 frames in 3.364612 seconds (29.721103 fps, 18260645.750580 B/s).
> >
> > capturing 720x480 @ 10FPS
> > Captured 100 frames in 10.022488 seconds (9.977562 fps, 6896491.169279 B/s).
> > capturing 720x480 @ 15FPS
> > Captured 100 frames in 6.891968 seconds (14.509643 fps, 10029065.232208 B/s).
> > capturing 720x480 @ 20FPS
> > Captured 100 frames in 5.234133 seconds (19.105360 fps, 13205624.616211 B/s).
> > capturing 720x480 @ 30FPS
> > Captured 100 frames in 3.602298 seconds (27.760055 fps, 19187750.044688 B/s).
> >
> > capturing 720x576 @ 10FPS
> > Captured 100 frames in 10.197244 seconds (9.806571 fps, 8133961.937805 B/s).
> > capturing 720x576 @ 15FPS
> > Captured 100 frames in 6.925244 seconds (14.439924 fps, 11977050.339261 B/s).
> > capturing 720x576 @ 20FPS
> > Captured 100 frames in 4.999968 seconds (20.000127 fps, 16588905.060854 B/s).
> > capturing 720x576 @ 30FPS
> > Captured 100 frames in 3.487568 seconds (28.673276 fps, 23782762.085212 B/s).
> >
> > capturing 1024x768 @ 10FPS
> > Captured 100 frames in 10.107128 seconds (9.894007 fps, 15561928.174298 B/s).
> > capturing 1024x768 @ 15FPS
> > Captured 100 frames in 6.810568 seconds (14.683062 fps, 23094459.169337 B/s).
> > capturing 1024x768 @ 20FPS
> > Captured 100 frames in 5.012039 seconds (19.951960 fps, 31381719.096759 B/s).
> > capturing 1024x768 @ 30FPS
> > Captured 100 frames in 3.346338 seconds (29.883407 fps, 47002534.905114 B/s).
> >
> > capturing 1280x720 @ 10FPS
> > Captured 100 frames in 9.957613 seconds (10.042567 fps, 18510459.665283 B/s).
> > capturing 1280x720 @ 15FPS
> > Captured 100 frames in 6.597751 seconds (15.156679 fps, 27936790.986673 B/s).
> > capturing 1280x720 @ 20FPS
> > Captured 100 frames in 4.946115 seconds (20.217888 fps, 37265611.495083 B/s).
> > capturing 1280x720 @ 30FPS
> > Captured 100 frames in 3.301329 seconds (30.290825 fps, 55832049.080847 B/s).
> >
> > capturing 1920x1080 @ 10FPS
> > Captured 100 frames in 10.024745 seconds (9.975316 fps, 41369629.470131 B/s).
> > capturing 1920x1080 @ 15FPS
> > Captured 100 frames in 6.674363 seconds (14.982702 fps, 62136260.577244 B/s).
> > capturing 1920x1080 @ 20FPS
> > Captured 100 frames in 5.102174 seconds (19.599488 fps, 81282998.172684 B/s).
> > capturing 1920x1080 @ 30FPS
> > Captured 100 frames in 3.341157 seconds (29.929746 fps, 124124642.214916 B/s).
> >
> > capturing 2592x1944 @ 10FPS
> > Captured 100 frames in 13.019132 seconds (7.681004 fps, 77406819.428913 B/s).
> > capturing 2592x1944 @ 15FPS
> > Captured 100 frames in 8.819705 seconds (11.338248 fps, 114263413.559267 B/s).
> > capturing 2592x1944 @ 20FPS
> > Captured 100 frames in 6.876134 seconds (14.543055 fps, 146560487.484508 B/s).
> > capturing 2592x1944 @ 30FPS
> > Captured 100 frames in 4.359511 seconds (22.938352 fps, 231165743.130365 B/s).
> > ---
> >  drivers/media/i2c/ov5640.c | 93 +++++++++++++++++++++++-----------------------
> >  1 file changed, 46 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index c659efe918a4..13b7a0d04840 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -740,8 +740,11 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
> >   *                         |    +---------------> SCLK 2X
> >   *                         |  +-------------+
> >   *                         +->| PCLK Div    | - reg 0x3108, bits 4-5
> > - *                            +-+-----------+
> > - *                              +---------------> PCLK
> > + *                            ++------------+
> > + *                             +  +-----------+
> > + *                             +->|   P_DIV   | - reg 0x3035, bits 0-3
> > + *                                +-----+-----+
> > + *                                       +------------> PCLK
> >   *
> >   * This is deviating from the datasheet at least for the register
> >   * 0x3108, since it's said here that the PCLK would be clocked from
> > @@ -751,7 +754,6 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
> >   *  - the PLL pre-divider output rate should be in the 4-27MHz range
> >   *  - the PLL multiplier output rate should be in the 500-1000MHz range
> >   *  - PCLK >= SCLK * 2 in YUV, >= SCLK in Raw or JPEG
> > - *  - MIPI SCLK = (bpp / lanes) / PCLK
> >   *
> >   * In the two latter cases, these constraints are met since our
> >   * factors are hardcoded. If we were to change that, we would need to
> > @@ -777,10 +779,11 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
> >  #define OV5640_SYSDIV_MAX      16
> >
> >  /*
> > - * This is supposed to be ranging from 1 to 16, but the value is always
> > - * set to 2 in the vendor kernels.
> > + * Hardcode these values for scaler and non-scaler modes.
> > + * FIXME: to be re-calcualted for 1 data lanes setups
> >   */
> > -#define OV5640_MIPI_DIV                2
> > +#define OV5640_MIPI_DIV_PCLK   2
> > +#define OV5640_MIPI_DIV_SCLK   1
> >
> >  /*
> >   * This is supposed to be ranging from 1 to 2, but the value is always
> > @@ -892,65 +895,61 @@ static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
> >   * ov5640_set_mipi_pclk() - Calculate the clock tree configuration values
> >   *                         for the MIPI CSI-2 output.
> >   *
> > - * @rate: The requested bandwidth in bytes per second.
> > - *       It is calculated as: HTOT * VTOT * FPS * bpp
> > + * @rate: The requested bandwidth per lane in bytes per second.
> > + *       'Bandwidth Per Lane' is calculated as:
> > + *       bpl = HTOT * VTOT * FPS * bpp / num_lanes;
> >   *
> >   * This function use the requested bandwidth to calculate:
> > - * - sample_rate = bandwidth / bpp;
> > - * - mipi_clk = bandwidth / num_lanes / 2; ( / 2 for CSI-2 DDR)
> > + * - sample_rate = bpl / (bpp / num_lanes);
> > + *              = bpl / (PLL_RDIV * BIT_DIV * PCLK_DIV * MIPI_DIV / num_lanes);
> > + *
> > + * - mipi_sclk   = bpl / MIPI_DIV / 2; ( / 2 is for CSI-2 DDR)
> >   *
> > - * The bandwidth corresponds to the SYSCLK frequency generated by the
> > - * PLL pre-divider, the PLL multiplier and the SYS divider (see the clock
> > - * tree documented here above).
> > + * with these fixed parameters:
> > + *     PLL_RDIV        = 2;
> > + *     BIT_DIVIDER     = 2; (MIPI_BIT_MODE == 8 ? 2 : 2,5);
> > + *     PCLK_DIV        = 1;
> >   *
> > - * From the SYSCLK frequency, the MIPI CSI-2 clock tree generates the
> > - * pixel clock and the MIPI BIT clock as follows:
> > + * The MIPI clock generation differs for modes that use the scaler and modes
> > + * that do not. In case the scaler is in use, the MIPI_SCLK generates the MIPI
> > + * BIT CLk, and thus:
> >   *
> > - * MIPI_BIT_CLK = SYSCLK / MIPI_DIV / 2;
> > - * PIXEL_CLK = SYSCLK / PLL_RDVI / BIT_DIVIDER / PCLK_DIV / MIPI_DIV
> > + * - mipi_sclk = bpl / MIPI_DIV / 2;
> > + *   MIPI_DIV = 1;
> >   *
> > - * with this fixed parameters:
> > - * PLL_RDIV    = 2;
> > - * BIT_DIVIDER = 2; (MIPI_BIT_MODE == 8 ? 2 : 2,5);
> > - * PCLK_DIV    = 1;
> > + * For modes that do not go through the scaler, the MIPI BIT CLOCK is generated
> > + * from the pixel clock, and thus:
> >   *
> > - * With these values we have:
> > + * - sample_rate = bpl / (bpp / num_lanes);
> > + *              = bpl / (2 * 2 * 1 * MIPI_DIV / num_lanes);
> > + *              = bpl / (4 * MIPI_DIV / num_lanes);
> > + * - MIPI_DIV   = bpp / (4 * num_lanes);
> >   *
> > - * pixel_clock = bandwidth / bpp
> > - *            = bandwidth / 4 / MIPI_DIV;
> > + * FIXME: this have been tested with 16bpp and 2 lanes setup only.
> > + * MIPI_DIV is fixed to value 2, but it -might- be changed according to the
> > + * above formula for setups with 1 lane or image formats with different bpp.
> >   *
> > - * And so we can calculate MIPI_DIV as:
> > - * MIPI_DIV = bpp / 4;
> > + * FIXME: this deviates from the sensor manual documentation which is quite
> > + * thin on the MIPI clock tree generation part.
> >   */
> >  static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor,
> >                                 unsigned long rate)
> >  {
> >         const struct ov5640_mode_info *mode = sensor->current_mode;
> > -       u8 mipi_div = OV5640_MIPI_DIV;
> >         u8 prediv, mult, sysdiv;
> > +       u8 mipi_div;
> >         int ret;
> >
> > -       /* FIXME:
> > -        * Practical experience shows we get a correct frame rate by
> > -        * halving the bandwidth rate by 2, to slow down SYSCLK frequency.
> > -        * Divide both SYSCLK and MIPI_DIV by two (with OV5640_MIPI_DIV
> > -        * currently fixed at value '2', while according to the above
> > -        * formula it should have been = bpp / 4 = 4).
> > -        *
> > -        * So that:
> > -        * pixel_clock = bandwidth / 2 / bpp
> > -        *             = bandwidth / 2 / 4 / MIPI_DIV;
> > -        * MIPI_DIV = bpp / 4 / 2;
> > -        */
> > -       rate /= 2;
> > -
> > -       /* FIXME:
> > -        * High resolution modes (1280x720, 1920x1080) requires an higher
> > -        * clock speed. Half the MIPI_DIVIDER value to double the output
> > -        * pixel clock and MIPI_CLK speeds.
> > +       /*
> > +        * 1280x720 is reported to use 'SUBSAMPLING' only,
> > +        * but according to the sensor manual it goes through the
> > +        * scaler before subsampling.
> >          */
> > -       if (mode->hact > 1024)
> > -               mipi_div /= 2;
> > +       if (mode->dn_mode == SCALING ||
> > +          (mode->id == OV5640_MODE_720P_1280_720))
> > +               mipi_div = OV5640_MIPI_DIV_SCLK;
> > +       else
> > +               mipi_div = OV5640_MIPI_DIV_PCLK;
> >
> >         ov5640_calc_sys_clk(sensor, rate, &prediv, &mult, &sysdiv);
> >
> > --
> > 2.7.4
> >

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