[RFC 0/3] media: ov5640: Adjust htot, rework clock tree, add LINK_FREQ

[RFC 0/3] media: ov5640: Adjust htot, rework clock tree, add LINK_FREQ

[Jacopo Mondi]

Hi Hugues Tomi and Sam

   this small series collects Tomi's patch on adjusting htot which has been
floating around for some time with a rework of the clock tree based on
Hugues' and Sam's work on setting pclk_period. It also address the need to
suppport LINK_FREQUENCY control as pointed out by Hugues.

I'm sort of happy with the result as I've removed quite some chrun and the clock
tree calculation is more linear. All modes work except full-resolution which a
bit annoys me, as I can't select it through s_fmt (to be honest I have not
investigated that in detail, that's why an RFC).

Framerate is better than before, but still off for some combinations:
640x480@30 gives me ~40 FPS, 1920x1080@15 gives me ~7.
The other combinations I've tested looks good.

Can I have your opinion on these changes and if they help you with your
platforms?

I've only been able to test YUYV, support for formats with != bpp will need
some work most probably, but that was like this before (although iirc Hugues
has captured JPEG, right ?)

There's a bit more cleanup on top to be done (I've left TODOs around) and
probably the HBLANK calculation should be checked to see if it works with the
new htot values.

Thanks
  j

Jacopo Mondi (2):
  media: i2c: ov5640: Rework CSI-2 clock tree
  media: i2c: ov5640: Add V4L2_CID_LINK_FREQ support

Tomi Valkeinen (1):
  media: i2c: ov5640: Adjust htot

 drivers/media/i2c/ov5640.c | 176 +++++++++++++++++++++++++------------
 1 file changed, 118 insertions(+), 58 deletions(-)

--
2.28.0

[RFC 1/3] media: i2c: ov5640: Adjust htot

[Jacopo Mondi]

From: Tomi Valkeinen <tomi.valkeinen@ti.com>

Adjust htot for most of the modes. The numbers are from the OV5640
datasheet, and with these the driver works more reliably on DRA76 EVM +
OV5640, using 2 datalanes.

Without the patch, I see often ComplexIO (i.e. PHY) errors when
starting the streaming, and 1280x720 does not work at all without this
change.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5640.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 8d0254d0e5ea..117ac22683ad 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -553,42 +553,42 @@ static const struct ov5640_mode_info ov5640_mode_init_data = {
 static const struct ov5640_mode_info
 ov5640_mode_data[OV5640_NUM_MODES] = {
 	{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
-	 176, 1896, 144, 984,
+	 176, 2844, 144, 984,
 	 ov5640_setting_QCIF_176_144,
 	 ARRAY_SIZE(ov5640_setting_QCIF_176_144),
 	 OV5640_30_FPS},
 	{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
-	 320, 1896, 240, 984,
+	 320, 2844, 240, 984,
 	 ov5640_setting_QVGA_320_240,
 	 ARRAY_SIZE(ov5640_setting_QVGA_320_240),
 	 OV5640_30_FPS},
 	{OV5640_MODE_VGA_640_480, SUBSAMPLING,
-	 640, 1896, 480, 1080,
+	 640, 2844, 480, 1080,
 	 ov5640_setting_VGA_640_480,
 	 ARRAY_SIZE(ov5640_setting_VGA_640_480),
 	 OV5640_60_FPS},
 	{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
-	 720, 1896, 480, 984,
+	 720, 2844, 480, 984,
 	 ov5640_setting_NTSC_720_480,
 	 ARRAY_SIZE(ov5640_setting_NTSC_720_480),
 	OV5640_30_FPS},
 	{OV5640_MODE_PAL_720_576, SUBSAMPLING,
-	 720, 1896, 576, 984,
+	 720, 2844, 576, 984,
 	 ov5640_setting_PAL_720_576,
 	 ARRAY_SIZE(ov5640_setting_PAL_720_576),
 	 OV5640_30_FPS},
 	{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
-	 1024, 1896, 768, 1080,
+	 1024, 2844, 768, 1080,
 	 ov5640_setting_XGA_1024_768,
 	 ARRAY_SIZE(ov5640_setting_XGA_1024_768),
 	 OV5640_30_FPS},
 	{OV5640_MODE_720P_1280_720, SUBSAMPLING,
-	 1280, 1892, 720, 740,
+	 1280, 2844, 720, 740,
 	 ov5640_setting_720P_1280_720,
 	 ARRAY_SIZE(ov5640_setting_720P_1280_720),
 	 OV5640_30_FPS},
 	{OV5640_MODE_1080P_1920_1080, SCALING,
-	 1920, 2500, 1080, 1120,
+	 1920, 2844, 1080, 1120,
 	 ov5640_setting_1080P_1920_1080,
 	 ARRAY_SIZE(ov5640_setting_1080P_1920_1080),
 	 OV5640_30_FPS},
-- 
2.28.0

[RFC 2/3] media: i2c: ov5640: Rework CSI-2 clock tree

[Jacopo Mondi]

Re-work the ov5640_set_mipi_pclk() function to calculate the
SYSCLK function based on the CSI-2 link frequency.

Take into account and more clearly document the clock tree constraints
reported in the PLL diagrams. Most values are still fixed and only tested
with 16bpp YUYV formats with a 2 lanes CSI-2 setup.

Tested by capturing and validating images in all the sensor supported
resolutions except full resolution 2592x1944.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/ov5640.c | 125 ++++++++++++++++++++++---------------
 1 file changed, 75 insertions(+), 50 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 117ac22683ad..236c684ca20b 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -88,6 +88,7 @@
 #define OV5640_REG_POLARITY_CTRL00	0x4740
 #define OV5640_REG_MIPI_CTRL00		0x4800
 #define OV5640_REG_DEBUG_MODE		0x4814
+#define OV5640_REG_PCLK_PERIOD		0x4837
 #define OV5640_REG_ISP_FORMAT_MUX_CTRL	0x501f
 #define OV5640_REG_PRE_ISP_TEST_SET1	0x503d
 #define OV5640_REG_SDE_CTRL0		0x5580
@@ -919,67 +920,88 @@ 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 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 = 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)
- *
- * with these fixed parameters:
- *	PLL_RDIV	= 2;
- *	BIT_DIVIDER	= 2; (MIPI_BIT_MODE == 8 ? 2 : 2,5);
- *	PCLK_DIV	= 1;
- *
- * 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_sclk = bpl / MIPI_DIV / 2;
- *   MIPI_DIV = 1;
- *
- * For modes that do not go through the scaler, the MIPI BIT CLOCK is generated
- * from the pixel clock, and thus:
- *
- * - 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);
+ * @rate: The requested bitrate in bits per second.
  *
  * 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.
- *
- * 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 bit_div, mipi_div, pclk_div, sclk_div, sclk2x_div, root_div;
 	u8 prediv, mult, sysdiv;
-	u8 mipi_div;
+	unsigned long link_freq;
+	unsigned long sysclk;
+	u8 pclk_period;
 	int ret;

 	/*
-	 * 1280x720 is reported to use 'SUBSAMPLING' only,
-	 * but according to the sensor manual it goes through the
-	 * scaler before subsampling.
+	 * The 'rate' parameter is the bitrate = VTOT * HTOT * FPS * BPP
+	 *
+	 * Adjust it to represent the CSI-2 link frequency and use it to
+	 * update the associated control.
 	 */
-	if (mode->dn_mode == SCALING ||
-	   (mode->id == OV5640_MODE_720P_1280_720))
-		mipi_div = OV5640_MIPI_DIV_SCLK;
+	link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2;
+
+	/*
+	 * - mipi_div - Assumptions not supported by documentation
+	 *
+	 *   The MIPI clock generation differs for modes that use the scaler and
+	 *   modes that do not.
+	 */
+	if (sensor->current_mode->dn_mode == SCALING)
+		mipi_div = 1;
 	else
-		mipi_div = OV5640_MIPI_DIV_PCLK;
+		mipi_div = 2;
+
+	sysclk = link_freq * 2 * mipi_div;
+	ov5640_calc_sys_clk(sensor, sysclk, &prediv, &mult, &sysdiv);
+
+	/*
+	 * Adjust PLL parameters to maintain the MIPI_SCLK-to-PCLK ratio;
+	 *
+	 * - root_div = 2 (fixed)
+	 * - bit_div : MIPI 8-bit = 2
+	 *	       MIPI 10-bit = 2,5
+	 * - pclk_div = 1 (fixed)
+	 * - pll_div  = (2 lanes ? mipi_div : 2 * mipi_div)
+	 *   2 lanes: MIPI_SCLK = (4 or 5) * PCLK
+	 *   1 lanes: MIPI_SCLK = (8 or 10) * PCLK
+	 *
+	 * TODO: support 10-bit formats
+	 * TODO: support 1 lane
+	 */
+	root_div = OV5640_PLL_CTRL3_PLL_ROOT_DIV_2;
+	bit_div =  OV5640_PLL_CTRL0_MIPI_MODE_8BIT;
+	pclk_div = OV5640_PLL_SYS_ROOT_DIVIDER_BYPASS;

-	ov5640_calc_sys_clk(sensor, rate, &prediv, &mult, &sysdiv);
+	/*
+	 * Scaler clock:
+	 * - YUV: PCLK >= 2 * SCLK
+	 * - RAW or JPEG: PCLK >= SCLK
+	 * - sclk2x_div = sclk_div / 2
+	 *
+	 * TODO: add support for RAW and JPEG modes. To maintain the
+	 * SCLK to PCLK ratio, the sclk_div should probably be
+	 * adjusted.
+	 */
+	sclk_div = ilog2(OV5640_SCLK_ROOT_DIV);
+	sclk2x_div = ilog2(OV5640_SCLK2X_ROOT_DIV);
+
+	/*
+	 * This is called pclk period, but it actually represents the
+	 * sample period expressed in ns with 1-bit decimal (0x01=0.5ns).
+	 *
+	 * - pclk = link_freq * 2 * lanes / bpp
+	 *
+	 * TODO: support 1 data lane; support modes with bpp != 16.
+	 */
+	pclk_period = 2000000000UL / (link_freq / 2);

+	/* Program the clock tree registers. */
 	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL0,
-			     0x0f, OV5640_PLL_CTRL0_MIPI_MODE_8BIT);
+			     0x0f, bit_div);
+	if (ret)
+		return ret;

 	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL1,
 			     0xff, sysdiv << 4 | mipi_div);
@@ -991,12 +1013,16 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor,
 		return ret;

 	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL3,
-			     0x1f, OV5640_PLL_CTRL3_PLL_ROOT_DIV_2 | prediv);
+			     0x1f, root_div | prediv);
+	if (ret)
+		return ret;
+
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x3f,
+			      (pclk_div << 4) | (sclk2x_div << 2) | sclk_div);
 	if (ret)
 		return ret;

-	return ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER,
-			      0x30, OV5640_PLL_SYS_ROOT_DIVIDER_BYPASS);
+	return ov5640_write_reg(sensor, OV5640_REG_PCLK_PERIOD, pclk_period);
 }

 static unsigned long ov5640_calc_pclk(struct ov5640_dev *sensor,
@@ -1775,7 +1801,6 @@ static int ov5640_set_mode(struct ov5640_dev *sensor)
 	 */
 	rate = ov5640_calc_pixel_rate(sensor) * 16;
 	if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
-		rate = rate / sensor->ep.bus.mipi_csi2.num_data_lanes;
 		ret = ov5640_set_mipi_pclk(sensor, rate);
 	} else {
 		rate = rate / sensor->ep.bus.parallel.bus_width;
--
2.28.0

[RFC 3/3] media: i2c: ov5640: Add V4L2_CID_LINK_FREQ support

[Jacopo Mondi]

Add support for the V4L2_CID_LINK_FREQ control in the ov5640
driver.

Unfortunately V4L2_CID_LINK_FREQ is a menu control and its supported
values has to be pre-calculated.

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

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 236c684ca20b..8429269b9d7d 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -144,6 +144,20 @@ static const struct ov5640_pixfmt ov5640_formats[] = {
 	{ MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_COLORSPACE_SRGB, },
 };
 
+static const s64 ov5640_link_freqs[] = {
+	126273600,
+	191116800,
+	184291200,
+	167909760,
+	252547200,
+	331724160,
+	335819520,
+	368582400,
+	382233600,
+	737164800,
+};
+#define OV5640_LINK_FREQS_NUM	ARRAY_SIZE(ov5640_link_freqs)
+
 /*
  * FIXME: remove this when a subdev API becomes available
  * to set the MIPI CSI-2 virtual channel.
@@ -220,6 +234,7 @@ struct ov5640_ctrls {
 	struct v4l2_ctrl *test_pattern;
 	struct v4l2_ctrl *hflip;
 	struct v4l2_ctrl *vflip;
+	struct v4l2_ctrl *link_freq;
 };
 
 struct ov5640_dev {
@@ -928,6 +943,7 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor,
 				unsigned long rate)
 {
 	u8 bit_div, mipi_div, pclk_div, sclk_div, sclk2x_div, root_div;
+	unsigned int freq_index, i;
 	u8 prediv, mult, sysdiv;
 	unsigned long link_freq;
 	unsigned long sysclk;
@@ -941,6 +957,19 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor,
 	 * update the associated control.
 	 */
 	link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2;
+	freq_index = OV5640_LINK_FREQS_NUM - 1;
+	for (i = 0; i < OV5640_LINK_FREQS_NUM; ++i) {
+		if (ov5640_link_freqs[i] == link_freq) {
+			freq_index = i;
+			break;
+		}
+	}
+	WARN_ONCE(i == OV5640_LINK_FREQS_NUM,
+		  "Link frequency %ld not supported", link_freq);
+
+	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, freq_index);
+	if (ret < 0)
+		return ret;
 
 	/*
 	 * - mipi_div - Assumptions not supported by documentation
@@ -2787,6 +2816,12 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
 				       V4L2_CID_POWER_LINE_FREQUENCY_AUTO, 0,
 				       V4L2_CID_POWER_LINE_FREQUENCY_50HZ);
 
+	ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
+						  OV5640_LINK_FREQS_NUM - 1,
+						  0, ov5640_link_freqs);
+	if (ctrls->link_freq)
+		ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
 	if (hdl->error) {
 		ret = hdl->error;
 		goto free_ctrls;
-- 
2.28.0

Re: [RFC 0/3] media: ov5640: Adjust htot, rework clock tree, add LINK_FREQ

[Tomi Valkeinen]

Hi Jacopo,

On 29/10/2020 00:57, Jacopo Mondi wrote:
> Hi Hugues Tomi and Sam
> 
>    this small series collects Tomi's patch on adjusting htot which has been
> floating around for some time with a rework of the clock tree based on
> Hugues' and Sam's work on setting pclk_period. It also address the need to
> suppport LINK_FREQUENCY control as pointed out by Hugues.
> 
> I'm sort of happy with the result as I've removed quite some chrun and the clock
> tree calculation is more linear. All modes work except full-resolution which a
> bit annoys me, as I can't select it through s_fmt (to be honest I have not
> investigated that in detail, that's why an RFC).
> 
> Framerate is better than before, but still off for some combinations:
> 640x480@30 gives me ~40 FPS, 1920x1080@15 gives me ~7.
> The other combinations I've tested looks good.
> 
> Can I have your opinion on these changes and if they help you with your
> platforms?
> 
> I've only been able to test YUYV, support for formats with != bpp will need
> some work most probably, but that was like this before (although iirc Hugues
> has captured JPEG, right ?)
> 
> There's a bit more cleanup on top to be done (I've left TODOs around) and
> probably the HBLANK calculation should be checked to see if it works with the
> new htot values.

Unfortunately the second patch seems to break capture on AM6 EVM + OV5640. The effect is pretty odd.
The picture is very dark, with odd vertical lines, but it's still capturing something as I can see a
correctly shaped shadow of my hand if I wave my hand over the sensor.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC 0/3] media: ov5640: Adjust htot, rework clock tree, add LINK_FREQ

[Jacopo Mondi]

Hi Tomi,
    thanks for testing

On Tue, Nov 03, 2020 at 09:19:17AM +0200, Tomi Valkeinen wrote:
> Hi Jacopo,
>
> On 29/10/2020 00:57, Jacopo Mondi wrote:
> > Hi Hugues Tomi and Sam
> >
> >    this small series collects Tomi's patch on adjusting htot which has been
> > floating around for some time with a rework of the clock tree based on
> > Hugues' and Sam's work on setting pclk_period. It also address the need to
> > suppport LINK_FREQUENCY control as pointed out by Hugues.
> >
> > I'm sort of happy with the result as I've removed quite some chrun and the clock
> > tree calculation is more linear. All modes work except full-resolution which a
> > bit annoys me, as I can't select it through s_fmt (to be honest I have not
> > investigated that in detail, that's why an RFC).
> >
> > Framerate is better than before, but still off for some combinations:
> > 640x480@30 gives me ~40 FPS, 1920x1080@15 gives me ~7.
> > The other combinations I've tested looks good.
> >
> > Can I have your opinion on these changes and if they help you with your
> > platforms?
> >
> > I've only been able to test YUYV, support for formats with != bpp will need
> > some work most probably, but that was like this before (although iirc Hugues
> > has captured JPEG, right ?)
> >
> > There's a bit more cleanup on top to be done (I've left TODOs around) and
> > probably the HBLANK calculation should be checked to see if it works with the
> > new htot values.
>
> Unfortunately the second patch seems to break capture on AM6 EVM + OV5640. The effect is pretty odd.
> The picture is very dark, with odd vertical lines, but it's still capturing something as I can see a
> correctly shaped shadow of my hand if I wave my hand over the sensor.

This saddens me quite a lot :( The current clock tree programming
procedure is horrid and it has been bugging me for 2 years now :(

Is capture broken in all modes, or have you tested a single one ?

>
>  Tomi
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC 0/3] media: ov5640: Adjust htot, rework clock tree, add LINK_FREQ

[Tomi Valkeinen]

On 03/11/2020 10:19, Jacopo Mondi wrote:
> Hi Tomi,
>     thanks for testing
> 
> On Tue, Nov 03, 2020 at 09:19:17AM +0200, Tomi Valkeinen wrote:
>> Hi Jacopo,
>>
>> On 29/10/2020 00:57, Jacopo Mondi wrote:
>>> Hi Hugues Tomi and Sam
>>>
>>>    this small series collects Tomi's patch on adjusting htot which has been
>>> floating around for some time with a rework of the clock tree based on
>>> Hugues' and Sam's work on setting pclk_period. It also address the need to
>>> suppport LINK_FREQUENCY control as pointed out by Hugues.
>>>
>>> I'm sort of happy with the result as I've removed quite some chrun and the clock
>>> tree calculation is more linear. All modes work except full-resolution which a
>>> bit annoys me, as I can't select it through s_fmt (to be honest I have not
>>> investigated that in detail, that's why an RFC).
>>>
>>> Framerate is better than before, but still off for some combinations:
>>> 640x480@30 gives me ~40 FPS, 1920x1080@15 gives me ~7.
>>> The other combinations I've tested looks good.
>>>
>>> Can I have your opinion on these changes and if they help you with your
>>> platforms?
>>>
>>> I've only been able to test YUYV, support for formats with != bpp will need
>>> some work most probably, but that was like this before (although iirc Hugues
>>> has captured JPEG, right ?)
>>>
>>> There's a bit more cleanup on top to be done (I've left TODOs around) and
>>> probably the HBLANK calculation should be checked to see if it works with the
>>> new htot values.
>>
>> Unfortunately the second patch seems to break capture on AM6 EVM + OV5640. The effect is pretty odd.
>> The picture is very dark, with odd vertical lines, but it's still capturing something as I can see a
>> correctly shaped shadow of my hand if I wave my hand over the sensor.
> 
> This saddens me quite a lot :( The current clock tree programming
> procedure is horrid and it has been bugging me for 2 years now :(
> 
> Is capture broken in all modes, or have you tested a single one ?

I tested 640x480, 720x480, 720x576.

I have only this sensor to test the CSI RX on AM6 EVM, so I would not be surprised if there are
issues in the CSI RX driver (too). But this is super frustrating to debug, as the sensor is a badly
documented black box, and I don't have means to probe the CSI lines...

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC 0/3] media: ov5640: Adjust htot, rework clock tree, add LINK_FREQ

[Jacopo Mondi]

Hi again,

On Tue, Nov 03, 2020 at 10:24:38AM +0200, Tomi Valkeinen wrote:
> On 03/11/2020 10:19, Jacopo Mondi wrote:
> > Hi Tomi,
> >     thanks for testing
> >
> > On Tue, Nov 03, 2020 at 09:19:17AM +0200, Tomi Valkeinen wrote:
> >> Hi Jacopo,
> >>
> >> On 29/10/2020 00:57, Jacopo Mondi wrote:
> >>> Hi Hugues Tomi and Sam
> >>>
> >>>    this small series collects Tomi's patch on adjusting htot which has been
> >>> floating around for some time with a rework of the clock tree based on
> >>> Hugues' and Sam's work on setting pclk_period. It also address the need to
> >>> suppport LINK_FREQUENCY control as pointed out by Hugues.
> >>>
> >>> I'm sort of happy with the result as I've removed quite some chrun and the clock
> >>> tree calculation is more linear. All modes work except full-resolution which a
> >>> bit annoys me, as I can't select it through s_fmt (to be honest I have not
> >>> investigated that in detail, that's why an RFC).
> >>>
> >>> Framerate is better than before, but still off for some combinations:
> >>> 640x480@30 gives me ~40 FPS, 1920x1080@15 gives me ~7.
> >>> The other combinations I've tested looks good.
> >>>
> >>> Can I have your opinion on these changes and if they help you with your
> >>> platforms?
> >>>
> >>> I've only been able to test YUYV, support for formats with != bpp will need
> >>> some work most probably, but that was like this before (although iirc Hugues
> >>> has captured JPEG, right ?)
> >>>
> >>> There's a bit more cleanup on top to be done (I've left TODOs around) and
> >>> probably the HBLANK calculation should be checked to see if it works with the
> >>> new htot values.
> >>
> >> Unfortunately the second patch seems to break capture on AM6 EVM + OV5640. The effect is pretty odd.
> >> The picture is very dark, with odd vertical lines, but it's still capturing something as I can see a
> >> correctly shaped shadow of my hand if I wave my hand over the sensor.
> >
> > This saddens me quite a lot :( The current clock tree programming
> > procedure is horrid and it has been bugging me for 2 years now :(
> >
> > Is capture broken in all modes, or have you tested a single one ?
>
> I tested 640x480, 720x480, 720x576.
>
> I have only this sensor to test the CSI RX on AM6 EVM, so I would not be surprised if there are
> issues in the CSI RX driver (too). But this is super frustrating to debug, as the sensor is a badly
> documented black box, and I don't have means to probe the CSI lines...

I see.. I'm sure you noticed, but as you mentioned the 'second patch'
I'll point it out anyway: the series has to be applied in full, as the
last patch adds support for reporting the link frequency, that has
been re-calculated by patch 2/3. On imx6 and on Hugues' platforms
adjusting the receiver's link frequency based on what's reported makes a
difference.

Maybe Hugues can give this series a spin to provide an additional data
point ?

Thanks
   j

>
>  Tomi
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC 0/3] media: ov5640: Adjust htot, rework clock tree, add LINK_FREQ

[Hugues FRUCHET]

Hi Jacopo,

On 11/3/20 9:45 AM, Jacopo Mondi wrote:
> Hi again,
> 
> On Tue, Nov 03, 2020 at 10:24:38AM +0200, Tomi Valkeinen wrote:
>> On 03/11/2020 10:19, Jacopo Mondi wrote:
>>> Hi Tomi,
>>>      thanks for testing
>>>
>>> On Tue, Nov 03, 2020 at 09:19:17AM +0200, Tomi Valkeinen wrote:
>>>> Hi Jacopo,
>>>>
>>>> On 29/10/2020 00:57, Jacopo Mondi wrote:
>>>>> Hi Hugues Tomi and Sam
>>>>>
>>>>>     this small series collects Tomi's patch on adjusting htot which has been
>>>>> floating around for some time with a rework of the clock tree based on
>>>>> Hugues' and Sam's work on setting pclk_period. It also address the need to
>>>>> suppport LINK_FREQUENCY control as pointed out by Hugues.
>>>>>
>>>>> I'm sort of happy with the result as I've removed quite some chrun and the clock
>>>>> tree calculation is more linear. All modes work except full-resolution which a
>>>>> bit annoys me, as I can't select it through s_fmt (to be honest I have not
>>>>> investigated that in detail, that's why an RFC).
>>>>>
>>>>> Framerate is better than before, but still off for some combinations:
>>>>> 640x480@30 gives me ~40 FPS, 1920x1080@15 gives me ~7.
>>>>> The other combinations I've tested looks good.
>>>>>
>>>>> Can I have your opinion on these changes and if they help you with your
>>>>> platforms?
>>>>>
>>>>> I've only been able to test YUYV, support for formats with != bpp will need
>>>>> some work most probably, but that was like this before (although iirc Hugues
>>>>> has captured JPEG, right ?)
>>>>>
>>>>> There's a bit more cleanup on top to be done (I've left TODOs around) and
>>>>> probably the HBLANK calculation should be checked to see if it works with the
>>>>> new htot values.
>>>>
>>>> Unfortunately the second patch seems to break capture on AM6 EVM + OV5640. The effect is pretty odd.
>>>> The picture is very dark, with odd vertical lines, but it's still capturing something as I can see a
>>>> correctly shaped shadow of my hand if I wave my hand over the sensor.
>>>
>>> This saddens me quite a lot :( The current clock tree programming
>>> procedure is horrid and it has been bugging me for 2 years now :(
>>>
>>> Is capture broken in all modes, or have you tested a single one ?
>>
>> I tested 640x480, 720x480, 720x576.
>>
>> I have only this sensor to test the CSI RX on AM6 EVM, so I would not be surprised if there are
>> issues in the CSI RX driver (too). But this is super frustrating to debug, as the sensor is a badly
>> documented black box, and I don't have means to probe the CSI lines...
> 
> I see.. I'm sure you noticed, but as you mentioned the 'second patch'
> I'll point it out anyway: the series has to be applied in full, as the
> last patch adds support for reporting the link frequency, that has
> been re-calculated by patch 2/3. On imx6 and on Hugues' platforms
> adjusting the receiver's link frequency based on what's reported makes a
> difference.
> 
> Maybe Hugues can give this series a spin to provide an additional data
> point ?

For sure, I'll try today.

> 
> Thanks
>     j
> 
>>
>>   Tomi
>>
>> --
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC 0/3] media: ov5640: Adjust htot, rework clock tree, add LINK_FREQ

[Tomi Valkeinen]

On 03/11/2020 10:45, Jacopo Mondi wrote:

>> I tested 640x480, 720x480, 720x576.
>>
>> I have only this sensor to test the CSI RX on AM6 EVM, so I would not be surprised if there are
>> issues in the CSI RX driver (too). But this is super frustrating to debug, as the sensor is a badly
>> documented black box, and I don't have means to probe the CSI lines...
> 
> I see.. I'm sure you noticed, but as you mentioned the 'second patch'
> I'll point it out anyway: the series has to be applied in full, as the
> last patch adds support for reporting the link frequency, that has
> been re-calculated by patch 2/3. On imx6 and on Hugues' platforms
> adjusting the receiver's link frequency based on what's reported makes a
> difference.

Yes, I first tried with all three, then tested one by one, and the second one started failing.

drivers/media/platform/ti-vpe/cal-camerarx.c doesn't use V4L2_CID_LINK_FREQ (it uses
V4L2_CID_PIXEL_RATE), though, so why would the third patch matter? Or do you mean that
V4L2_CID_LINK_FREQ must be used to get ov5640 work? Aren't pixel rate and link freq directly linked?

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC 0/3] media: ov5640: Adjust htot, rework clock tree, add LINK_FREQ

[Jacopo Mondi]

Hi Tomi,

On Tue, Nov 03, 2020 at 11:31:06AM +0200, Tomi Valkeinen wrote:
> On 03/11/2020 10:45, Jacopo Mondi wrote:
>
> >> I tested 640x480, 720x480, 720x576.
> >>
> >> I have only this sensor to test the CSI RX on AM6 EVM, so I would not be surprised if there are
> >> issues in the CSI RX driver (too). But this is super frustrating to debug, as the sensor is a badly
> >> documented black box, and I don't have means to probe the CSI lines...
> >
> > I see.. I'm sure you noticed, but as you mentioned the 'second patch'
> > I'll point it out anyway: the series has to be applied in full, as the
> > last patch adds support for reporting the link frequency, that has
> > been re-calculated by patch 2/3. On imx6 and on Hugues' platforms
> > adjusting the receiver's link frequency based on what's reported makes a
> > difference.
>
> Yes, I first tried with all three, then tested one by one, and the second one started failing.
>

Ok, thanks for the clarification.

> drivers/media/platform/ti-vpe/cal-camerarx.c doesn't use V4L2_CID_LINK_FREQ (it uses
> V4L2_CID_PIXEL_RATE), though, so why would the third patch matter? Or do you mean that
> V4L2_CID_LINK_FREQ must be used to get ov5640 work? Aren't pixel rate and link freq directly linked?

Oh I see. As I read in the driver the PIXEL_RATE control gets only
updated when the frame interval is changed. It should be probably
updated when the mode changes as well. Although, it would be fairly
easy to deduct the pixel rate from the link frequency in the receiver.

>
>  Tomi
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC 0/3] media: ov5640: Adjust htot, rework clock tree, add LINK_FREQ

[Hugues FRUCHET]

Hi Jacopo,

Here is the results of tests with 0V5640 CSI-2 on Avenger96 board.

1) First of all, the framerate is broken, it is almost 2 times greater 
that expected. Checking code it seems that mipi_div is missing when 
computing link_freq:

+	/*
  	 * The 'rate' parameter is the bitrate = VTOT * HTOT * FPS * BPP
  	 *
  	 * Adjust it to represent the CSI-2 link frequency and use it to
  	 * update the associated control.
  	 */
-	link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2;
+	link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2 / mipi_div;

To test the setup I have patched the link frequency control to report 
dynamically the frequency instead of hardcoded value:
+#if 0
  	freq_index = OV5640_LINK_FREQS_NUM - 1;
  	for (i = 0; i < OV5640_LINK_FREQS_NUM; ++i) {
  		if (ov5640_link_freqs[i] == link_freq) {
@@ -966,18 +979,12 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev 
*sensor,
  	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, freq_index);
  	if (ret < 0)
  		return ret;
+#else
+	ov5640_link_freqs[0] = link_freq;
+	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, 0);
+#endif

2) Second problem comes from "media: i2c: ov5640: Adjust htot", this is 
breaking 1024x768@30fps & VGA@30fps which are slowdown to 15fps

3) I have some instabilities when switching between framerate, I have to 
investigate the point. In few words this is a race problem between the 
OV5640 which set the frequency control and the MIPID02 which read the 
frequency control. I'll dig into the issue to see how to fix that.


To summarize:
-------------
1) "media: i2c: ov5640: Rework CSI-2 clock tree"
Almost OK but mipi_div is missing

2) "media: i2c: ov5640: Adjust htot"
Is breaking some resolutions/fps, so better to drop.
Tomi, perhaps could you recheck with the fixed Jacopo serie if you still 
encounter your DPHY error issues ?

With 1) fixed and 2) reverted, I'm back on track and have a successfull 
non-regression on my side + some better figures on some resolutions:
- 1024x768@30fps which was not at the right framerate previously
- 720p@30fps which was not at the right framerate previously
- HD@15fps which was not at the right framerate previously

Please note that I cannot go above HD@15fps on this platform.

* QCIF  176x144 RGB565 15fps => OK, got 15
* QCIF  176x144 YUYV   15fps => OK, got 15
* QCIF  176x144 JPEG   15fps => OK, got 15
* QCIF  176x144 RGB565 30fps => OK, got 30
* QCIF  176x144 YUYV   30fps => OK, got 30
* QCIF  176x144 JPEG   30fps => OK, got 30
* QVGA  320x240 RGB565 15fps => OK, got 15
* QVGA  320x240 YUYV   15fps => OK, got 15
* QVGA  320x240 JPEG   15fps => OK, got 15
* QVGA  320x240 RGB565 30fps => OK, got 29
* QVGA  320x240 YUYV   30fps => OK, got 30
* QVGA  320x240 JPEG   30fps => OK, got 29
* VGA   640x480 RGB565 15fps => OK, got 15
* VGA   640x480 YUYV   15fps => OK, got 15
* VGA   640x480 JPEG   15fps => OK, got 15
* VGA   640x480 RGB565 30fps => OK, got 30
* VGA   640x480 YUYV   30fps => OK, got 30
* VGA   640x480 JPEG   30fps => OK, got 30
* 480p  720x480 RGB565 15fps => OK, got 15
* 480p  720x480 YUYV   15fps => OK, got 15
* 480p  720x480 JPEG   15fps => OK, got 15
* 480p  720x480 RGB565 30fps => OK, got 30
* 480p  720x480 YUYV   30fps => OK, got 30
* 480p  720x480 JPEG   30fps => OK, got 30
* XGA  1024x768 RGB565 15fps => OK, got 15
* XGA  1024x768 YUYV   15fps => OK, got 15
* XGA  1024x768 JPEG   15fps => OK, got 15
* XGA  1024x768 RGB565 30fps => OK, got 30
* XGA  1024x768 YUYV   30fps => OK, got 30
* XGA  1024x768 JPEG   30fps => OK, got 30
* 720p 1280x720 RGB565 15fps => OK, got 15
* 720p 1280x720 YUYV   15fps => OK, got 15
* 720p 1280x720 JPEG   15fps => OK, got 15
* 720p 1280x720 RGB565 30fps => OK, got 30
* 720p 1280x720 YUYV   30fps => OK, got 30
* 720p 1280x720 JPEG   30fps => OK, got 30
* HD  1920x1080 RGB565 15fps => OK, got 15
* HD  1920x1080 YUYV   15fps => OK, got 15
* HD  1920x1080 JPEG   15fps => OK, got 15


So in few words, it sounds good, thanks Jacopo !


On 10/28/20 11:57 PM, Jacopo Mondi wrote:
> Hi Hugues Tomi and Sam
> 
>     this small series collects Tomi's patch on adjusting htot which has been
> floating around for some time with a rework of the clock tree based on
> Hugues' and Sam's work on setting pclk_period. It also address the need to
> suppport LINK_FREQUENCY control as pointed out by Hugues.
> 
> I'm sort of happy with the result as I've removed quite some chrun and the clock
> tree calculation is more linear. All modes work except full-resolution which a
> bit annoys me, as I can't select it through s_fmt (to be honest I have not
> investigated that in detail, that's why an RFC).
> 
> Framerate is better than before, but still off for some combinations:
> 640x480@30 gives me ~40 FPS, 1920x1080@15 gives me ~7.
> The other combinations I've tested looks good.
> 
> Can I have your opinion on these changes and if they help you with your
> platforms?
> 
> I've only been able to test YUYV, support for formats with != bpp will need
> some work most probably, but that was like this before (although iirc Hugues
> has captured JPEG, right ?)
> 
> There's a bit more cleanup on top to be done (I've left TODOs around) and
> probably the HBLANK calculation should be checked to see if it works with the
> new htot values.
> 
> Thanks
>    j
> 
> Jacopo Mondi (2):
>    media: i2c: ov5640: Rework CSI-2 clock tree
>    media: i2c: ov5640: Add V4L2_CID_LINK_FREQ support
> 
> Tomi Valkeinen (1):
>    media: i2c: ov5640: Adjust htot
> 
>   drivers/media/i2c/ov5640.c | 176 +++++++++++++++++++++++++------------
>   1 file changed, 118 insertions(+), 58 deletions(-)
> 
> --
> 2.28.0
> 

Best regards,
Hugues.

Re: [RFC 2/3] media: i2c: ov5640: Rework CSI-2 clock tree

[Hugues FRUCHET]

Hi Jacopo,

On 10/28/20 11:57 PM, Jacopo Mondi wrote:
> Re-work the ov5640_set_mipi_pclk() function to calculate the
> SYSCLK function based on the CSI-2 link frequency.
> 
> Take into account and more clearly document the clock tree constraints
> reported in the PLL diagrams. Most values are still fixed and only tested
> with 16bpp YUYV formats with a 2 lanes CSI-2 setup.
> 
> Tested by capturing and validating images in all the sensor supported
> resolutions except full resolution 2592x1944.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>   drivers/media/i2c/ov5640.c | 125 ++++++++++++++++++++++---------------
>   1 file changed, 75 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 117ac22683ad..236c684ca20b 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -88,6 +88,7 @@
>   #define OV5640_REG_POLARITY_CTRL00	0x4740
>   #define OV5640_REG_MIPI_CTRL00		0x4800
>   #define OV5640_REG_DEBUG_MODE		0x4814
> +#define OV5640_REG_PCLK_PERIOD		0x4837
>   #define OV5640_REG_ISP_FORMAT_MUX_CTRL	0x501f
>   #define OV5640_REG_PRE_ISP_TEST_SET1	0x503d
>   #define OV5640_REG_SDE_CTRL0		0x5580
> @@ -919,67 +920,88 @@ 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 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 = 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)
> - *
> - * with these fixed parameters:
> - *	PLL_RDIV	= 2;
> - *	BIT_DIVIDER	= 2; (MIPI_BIT_MODE == 8 ? 2 : 2,5);
> - *	PCLK_DIV	= 1;
> - *
> - * 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_sclk = bpl / MIPI_DIV / 2;
> - *   MIPI_DIV = 1;
> - *
> - * For modes that do not go through the scaler, the MIPI BIT CLOCK is generated
> - * from the pixel clock, and thus:
> - *
> - * - 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);
> + * @rate: The requested bitrate in bits per second.
>    *
>    * 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.
> - *
> - * 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 bit_div, mipi_div, pclk_div, sclk_div, sclk2x_div, root_div;
>   	u8 prediv, mult, sysdiv;
> -	u8 mipi_div;
> +	unsigned long link_freq;
> +	unsigned long sysclk;
> +	u8 pclk_period;
>   	int ret;
> 
>   	/*
> -	 * 1280x720 is reported to use 'SUBSAMPLING' only,
> -	 * but according to the sensor manual it goes through the
> -	 * scaler before subsampling.
> +	 * The 'rate' parameter is the bitrate = VTOT * HTOT * FPS * BPP
> +	 *
> +	 * Adjust it to represent the CSI-2 link frequency and use it to
> +	 * update the associated control.
>   	 */
> -	if (mode->dn_mode == SCALING ||
> -	   (mode->id == OV5640_MODE_720P_1280_720))
> -		mipi_div = OV5640_MIPI_DIV_SCLK;
> +	link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2;

framerate is broken, it is almost 2 times greater that expected. 
Checking code it seems that mipi_div is missing when computing link_freq:

-	link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2;
+	link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2 / mipi_div;

To test the setup I have patched the link frequency control to report 
dynamically the frequency instead of hardcoded value:
+#if 0
  	freq_index = OV5640_LINK_FREQS_NUM - 1;
  	for (i = 0; i < OV5640_LINK_FREQS_NUM; ++i) {
  		if (ov5640_link_freqs[i] == link_freq) {
@@ -966,18 +979,12 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev 
*sensor,
  	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, freq_index);
  	if (ret < 0)
  		return ret;
+#else
+	ov5640_link_freqs[0] = link_freq;
+	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, 0);
+#endif

> +
> +	/*
> +	 * - mipi_div - Assumptions not supported by documentation
> +	 *
> +	 *   The MIPI clock generation differs for modes that use the scaler and
> +	 *   modes that do not.
> +	 */
> +	if (sensor->current_mode->dn_mode == SCALING)
> +		mipi_div = 1;
>   	else
> -		mipi_div = OV5640_MIPI_DIV_PCLK;
> +		mipi_div = 2;
> +
> +	sysclk = link_freq * 2 * mipi_div;
> +	ov5640_calc_sys_clk(sensor, sysclk, &prediv, &mult, &sysdiv);
> +
> +	/*
> +	 * Adjust PLL parameters to maintain the MIPI_SCLK-to-PCLK ratio;
> +	 *
> +	 * - root_div = 2 (fixed)
> +	 * - bit_div : MIPI 8-bit = 2
> +	 *	       MIPI 10-bit = 2,5
> +	 * - pclk_div = 1 (fixed)
> +	 * - pll_div  = (2 lanes ? mipi_div : 2 * mipi_div)
> +	 *   2 lanes: MIPI_SCLK = (4 or 5) * PCLK
> +	 *   1 lanes: MIPI_SCLK = (8 or 10) * PCLK
> +	 *
> +	 * TODO: support 10-bit formats
> +	 * TODO: support 1 lane
> +	 */
> +	root_div = OV5640_PLL_CTRL3_PLL_ROOT_DIV_2;
> +	bit_div =  OV5640_PLL_CTRL0_MIPI_MODE_8BIT;
> +	pclk_div = OV5640_PLL_SYS_ROOT_DIVIDER_BYPASS;
> 
> -	ov5640_calc_sys_clk(sensor, rate, &prediv, &mult, &sysdiv);
> +	/*
> +	 * Scaler clock:
> +	 * - YUV: PCLK >= 2 * SCLK
> +	 * - RAW or JPEG: PCLK >= SCLK
> +	 * - sclk2x_div = sclk_div / 2
> +	 *
> +	 * TODO: add support for RAW and JPEG modes. To maintain the
> +	 * SCLK to PCLK ratio, the sclk_div should probably be
> +	 * adjusted.
> +	 */
> +	sclk_div = ilog2(OV5640_SCLK_ROOT_DIV);
> +	sclk2x_div = ilog2(OV5640_SCLK2X_ROOT_DIV);
> +
> +	/*
> +	 * This is called pclk period, but it actually represents the
> +	 * sample period expressed in ns with 1-bit decimal (0x01=0.5ns).
> +	 *
> +	 * - pclk = link_freq * 2 * lanes / bpp
> +	 *
> +	 * TODO: support 1 data lane; support modes with bpp != 16.
> +	 */
> +	pclk_period = 2000000000UL / (link_freq / 2);
> 
> +	/* Program the clock tree registers. */
>   	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL0,
> -			     0x0f, OV5640_PLL_CTRL0_MIPI_MODE_8BIT);
> +			     0x0f, bit_div);
> +	if (ret)
> +		return ret;
> 
>   	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL1,
>   			     0xff, sysdiv << 4 | mipi_div);
> @@ -991,12 +1013,16 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor,
>   		return ret;
> 
>   	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL3,
> -			     0x1f, OV5640_PLL_CTRL3_PLL_ROOT_DIV_2 | prediv);
> +			     0x1f, root_div | prediv);
> +	if (ret)
> +		return ret;
> +
> +	ret = ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x3f,
> +			      (pclk_div << 4) | (sclk2x_div << 2) | sclk_div);
>   	if (ret)
>   		return ret;
> 
> -	return ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER,
> -			      0x30, OV5640_PLL_SYS_ROOT_DIVIDER_BYPASS);
> +	return ov5640_write_reg(sensor, OV5640_REG_PCLK_PERIOD, pclk_period);
>   }
> 
>   static unsigned long ov5640_calc_pclk(struct ov5640_dev *sensor,
> @@ -1775,7 +1801,6 @@ static int ov5640_set_mode(struct ov5640_dev *sensor)
>   	 */
>   	rate = ov5640_calc_pixel_rate(sensor) * 16;
>   	if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> -		rate = rate / sensor->ep.bus.mipi_csi2.num_data_lanes;
>   		ret = ov5640_set_mipi_pclk(sensor, rate);
>   	} else {
>   		rate = rate / sensor->ep.bus.parallel.bus_width;
> --
> 2.28.0
> 

BR,
Hugues.

Re: [RFC 1/3] media: i2c: ov5640: Adjust htot

[Hugues FRUCHET]

Hi Jacopo, Tomi,

This patch is breaking 1024x768@30fps & VGA@30fps on my side which are 
slowdown to 15fps.
Tomi, perhaps could you recheck with the fixed Jacopo serie if you still 
encounter your DPHY error issues ?

On 10/28/20 11:57 PM, Jacopo Mondi wrote:
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> Adjust htot for most of the modes. The numbers are from the OV5640
> datasheet, and with these the driver works more reliably on DRA76 EVM +
> OV5640, using 2 datalanes.
> 
> Without the patch, I see often ComplexIO (i.e. PHY) errors when
> starting the streaming, and 1280x720 does not work at all without this
> change.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>   drivers/media/i2c/ov5640.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 8d0254d0e5ea..117ac22683ad 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -553,42 +553,42 @@ static const struct ov5640_mode_info ov5640_mode_init_data = {
>   static const struct ov5640_mode_info
>   ov5640_mode_data[OV5640_NUM_MODES] = {
>   	{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
> -	 176, 1896, 144, 984,
> +	 176, 2844, 144, 984,
>   	 ov5640_setting_QCIF_176_144,
>   	 ARRAY_SIZE(ov5640_setting_QCIF_176_144),
>   	 OV5640_30_FPS},
>   	{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
> -	 320, 1896, 240, 984,
> +	 320, 2844, 240, 984,
>   	 ov5640_setting_QVGA_320_240,
>   	 ARRAY_SIZE(ov5640_setting_QVGA_320_240),
>   	 OV5640_30_FPS},
>   	{OV5640_MODE_VGA_640_480, SUBSAMPLING,
> -	 640, 1896, 480, 1080,
> +	 640, 2844, 480, 1080,
>   	 ov5640_setting_VGA_640_480,
>   	 ARRAY_SIZE(ov5640_setting_VGA_640_480),
>   	 OV5640_60_FPS},
>   	{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
> -	 720, 1896, 480, 984,
> +	 720, 2844, 480, 984,
>   	 ov5640_setting_NTSC_720_480,
>   	 ARRAY_SIZE(ov5640_setting_NTSC_720_480),
>   	OV5640_30_FPS},
>   	{OV5640_MODE_PAL_720_576, SUBSAMPLING,
> -	 720, 1896, 576, 984,
> +	 720, 2844, 576, 984,
>   	 ov5640_setting_PAL_720_576,
>   	 ARRAY_SIZE(ov5640_setting_PAL_720_576),
>   	 OV5640_30_FPS},
>   	{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
> -	 1024, 1896, 768, 1080,
> +	 1024, 2844, 768, 1080,
>   	 ov5640_setting_XGA_1024_768,
>   	 ARRAY_SIZE(ov5640_setting_XGA_1024_768),
>   	 OV5640_30_FPS},
>   	{OV5640_MODE_720P_1280_720, SUBSAMPLING,
> -	 1280, 1892, 720, 740,
> +	 1280, 2844, 720, 740,
>   	 ov5640_setting_720P_1280_720,
>   	 ARRAY_SIZE(ov5640_setting_720P_1280_720),
>   	 OV5640_30_FPS},
>   	{OV5640_MODE_1080P_1920_1080, SCALING,
> -	 1920, 2500, 1080, 1120,
> +	 1920, 2844, 1080, 1120,
>   	 ov5640_setting_1080P_1920_1080,
>   	 ARRAY_SIZE(ov5640_setting_1080P_1920_1080),
>   	 OV5640_30_FPS},
> 

BR,
Hugues.

Re: [RFC 0/3] media: ov5640: Adjust htot, rework clock tree, add LINK_FREQ

[Jacopo Mondi]

Hello Hugues,

    thanks so much for testing

On Tue, Nov 03, 2020 at 04:53:21PM +0000, Hugues FRUCHET wrote:
> Hi Jacopo,
>
> Here is the results of tests with 0V5640 CSI-2 on Avenger96 board.
>
> 1) First of all, the framerate is broken, it is almost 2 times greater
> that expected. Checking code it seems that mipi_div is missing when
> computing link_freq:
>
> +	/*
>   	 * The 'rate' parameter is the bitrate = VTOT * HTOT * FPS * BPP
>   	 *
>   	 * Adjust it to represent the CSI-2 link frequency and use it to
>   	 * update the associated control.
>   	 */
> -	link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2;
> +	link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2 / mipi_div;

I don't think this is correct I'm sorry.

In my platform this fixes (in example) 640x480@30FPS but breaks
640x480@15FPS which now runs at 7.5FPS (with Tomi's patch reverted)..
What a weird behaviour

The reasoning behing link_frequency calculation is that

pixel_rate = vtot * htot * fps
bit_rate = pixel_rate * bpp
link_freq = bit_rate / num_lanes / 2 (CSI-2 DDR)

MIPI_DIV is not yet into play, as we're calculating the CSI-2 clock
lane freqeuency without applying it to the clock tree

In my clock diagram link_freq is what is the MIPI_CLK output
To transform it in SYSCLK you walk the clock tree backward and

sysclk = link_freq * 2 * mipi_div

>
> To test the setup I have patched the link frequency control to report
> dynamically the frequency instead of hardcoded value:
> +#if 0
>   	freq_index = OV5640_LINK_FREQS_NUM - 1;
>   	for (i = 0; i < OV5640_LINK_FREQS_NUM; ++i) {
>   		if (ov5640_link_freqs[i] == link_freq) {
> @@ -966,18 +979,12 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev
> *sensor,
>   	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, freq_index);
>   	if (ret < 0)
>   		return ret;
> +#else
> +	ov5640_link_freqs[0] = link_freq;
> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, 0);
> +#endif

I wonder if this is acceptable for mainline. Pre-calculating the link
frequency is really a pain. I wonder why LINK_FREQ is a menu control
in first place :/

>
> 2) Second problem comes from "media: i2c: ov5640: Adjust htot", this is
> breaking 1024x768@30fps & VGA@30fps which are slowdown to 15fps

Weird, as 'Adjust htot' -increases- the htot values resulting in a
-faster- clock output, right ? Are you sure this is not due to the
above "/ mipi_div;" you've added ?

>
> 3) I have some instabilities when switching between framerate, I have to
> investigate the point. In few words this is a race problem between the
> OV5640 which set the frequency control and the MIPID02 which read the
> frequency control. I'll dig into the issue to see how to fix that.
>
>
> To summarize:
> -------------
> 1) "media: i2c: ov5640: Rework CSI-2 clock tree"
> Almost OK but mipi_div is missing
>
> 2) "media: i2c: ov5640: Adjust htot"
> Is breaking some resolutions/fps, so better to drop.
> Tomi, perhaps could you recheck with the fixed Jacopo serie if you still
> encounter your DPHY error issues ?
>
> With 1) fixed and 2) reverted, I'm back on track and have a successfull
> non-regression on my side + some better figures on some resolutions:
> - 1024x768@30fps which was not at the right framerate previously
> - 720p@30fps which was not at the right framerate previously
> - HD@15fps which was not at the right framerate previously
>
> Please note that I cannot go above HD@15fps on this platform.
>
> * QCIF  176x144 RGB565 15fps => OK, got 15
> * QCIF  176x144 YUYV   15fps => OK, got 15
> * QCIF  176x144 JPEG   15fps => OK, got 15
> * QCIF  176x144 RGB565 30fps => OK, got 30
> * QCIF  176x144 YUYV   30fps => OK, got 30
> * QCIF  176x144 JPEG   30fps => OK, got 30
> * QVGA  320x240 RGB565 15fps => OK, got 15
> * QVGA  320x240 YUYV   15fps => OK, got 15
> * QVGA  320x240 JPEG   15fps => OK, got 15
> * QVGA  320x240 RGB565 30fps => OK, got 29
> * QVGA  320x240 YUYV   30fps => OK, got 30
> * QVGA  320x240 JPEG   30fps => OK, got 29
> * VGA   640x480 RGB565 15fps => OK, got 15
> * VGA   640x480 YUYV   15fps => OK, got 15
> * VGA   640x480 JPEG   15fps => OK, got 15
> * VGA   640x480 RGB565 30fps => OK, got 30
> * VGA   640x480 YUYV   30fps => OK, got 30
> * VGA   640x480 JPEG   30fps => OK, got 30
> * 480p  720x480 RGB565 15fps => OK, got 15
> * 480p  720x480 YUYV   15fps => OK, got 15
> * 480p  720x480 JPEG   15fps => OK, got 15
> * 480p  720x480 RGB565 30fps => OK, got 30
> * 480p  720x480 YUYV   30fps => OK, got 30
> * 480p  720x480 JPEG   30fps => OK, got 30
> * XGA  1024x768 RGB565 15fps => OK, got 15
> * XGA  1024x768 YUYV   15fps => OK, got 15
> * XGA  1024x768 JPEG   15fps => OK, got 15
> * XGA  1024x768 RGB565 30fps => OK, got 30
> * XGA  1024x768 YUYV   30fps => OK, got 30
> * XGA  1024x768 JPEG   30fps => OK, got 30
> * 720p 1280x720 RGB565 15fps => OK, got 15
> * 720p 1280x720 YUYV   15fps => OK, got 15
> * 720p 1280x720 JPEG   15fps => OK, got 15
> * 720p 1280x720 RGB565 30fps => OK, got 30
> * 720p 1280x720 YUYV   30fps => OK, got 30
> * 720p 1280x720 JPEG   30fps => OK, got 30
> * HD  1920x1080 RGB565 15fps => OK, got 15
> * HD  1920x1080 YUYV   15fps => OK, got 15
> * HD  1920x1080 JPEG   15fps => OK, got 15
>
>
> So in few words, it sounds good, thanks Jacopo !

That's sweet, but doesn't match what I see on iMX.6 /o\


>
>
> On 10/28/20 11:57 PM, Jacopo Mondi wrote:
> > Hi Hugues Tomi and Sam
> >
> >     this small series collects Tomi's patch on adjusting htot which has been
> > floating around for some time with a rework of the clock tree based on
> > Hugues' and Sam's work on setting pclk_period. It also address the need to
> > suppport LINK_FREQUENCY control as pointed out by Hugues.
> >
> > I'm sort of happy with the result as I've removed quite some chrun and the clock
> > tree calculation is more linear. All modes work except full-resolution which a
> > bit annoys me, as I can't select it through s_fmt (to be honest I have not
> > investigated that in detail, that's why an RFC).
> >
> > Framerate is better than before, but still off for some combinations:
> > 640x480@30 gives me ~40 FPS, 1920x1080@15 gives me ~7.
> > The other combinations I've tested looks good.
> >
> > Can I have your opinion on these changes and if they help you with your
> > platforms?
> >
> > I've only been able to test YUYV, support for formats with != bpp will need
> > some work most probably, but that was like this before (although iirc Hugues
> > has captured JPEG, right ?)
> >
> > There's a bit more cleanup on top to be done (I've left TODOs around) and
> > probably the HBLANK calculation should be checked to see if it works with the
> > new htot values.
> >
> > Thanks
> >    j
> >
> > Jacopo Mondi (2):
> >    media: i2c: ov5640: Rework CSI-2 clock tree
> >    media: i2c: ov5640: Add V4L2_CID_LINK_FREQ support
> >
> > Tomi Valkeinen (1):
> >    media: i2c: ov5640: Adjust htot
> >
> >   drivers/media/i2c/ov5640.c | 176 +++++++++++++++++++++++++------------
> >   1 file changed, 118 insertions(+), 58 deletions(-)
> >
> > --
> > 2.28.0
> >
>
> Best regards,
> Hugues.

Re: [RFC 0/3] media: ov5640: Adjust htot, rework clock tree, add LINK_FREQ

[Hugues FRUCHET]

Hi Jacopo,

On 11/5/20 11:14 AM, Jacopo Mondi wrote:
> Hello Hugues,
> 
>      thanks so much for testing
> 
> On Tue, Nov 03, 2020 at 04:53:21PM +0000, Hugues FRUCHET wrote:
>> Hi Jacopo,
>>
>> Here is the results of tests with 0V5640 CSI-2 on Avenger96 board.
>>
>> 1) First of all, the framerate is broken, it is almost 2 times greater
>> that expected. Checking code it seems that mipi_div is missing when
>> computing link_freq:
>>
>> +	/*
>>    	 * The 'rate' parameter is the bitrate = VTOT * HTOT * FPS * BPP
>>    	 *
>>    	 * Adjust it to represent the CSI-2 link frequency and use it to
>>    	 * update the associated control.
>>    	 */
>> -	link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2;
>> +	link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2 / mipi_div;
> 
> I don't think this is correct I'm sorry.
> 

But this is what is observed with oscilloscope:

v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=30;v4l2-ctl 
--set-fmt-video=width=640,height=480,pixelformat=JPEG --stream-mmap 
--stream-count=-1
Frame rate set to 30.000 fps
[ 3501.482829] ov5640 1-003c: Bandwidth Per Lane=491443200, 640x480 from 
1896x1080
[ 3501.488822] ov5640 1-003c: ov5640_set_mipi_pclk: __v4l2_ctrl_s_ctrl 0 
122860800 Hz
[ 3501.496415] ov5640 1-003c: sysclk=491443200, _rate=492000000, 
mipi_div=2, prediv=3, mult=123, sysdiv=2
[ 3501.511064] ov5640 1-003c: PCLK PERIOD 0x4837=0x20
[ 3501.569487] st-mipid02 2-0014: clk_lane_reg1=0x41
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.00 fps
Measured #8ns (125MHz) ==> in line with 122860800 Hz

v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl 
--set-fmt-video=width=640,height=480,pixelformat=JPEG --stream-mmap 
--stream-count=-1
Frame rate set to 15.000 fps
[ 5019.240550] ov5640 1-003c: Bandwidth Per Lane=245721600, 640x480 from 
1896x1080
[ 5019.246542] ov5640 1-003c: ov5640_set_mipi_pclk: __v4l2_ctrl_s_ctrl 0 
61430400 Hz
[ 5019.257485] ov5640 1-003c: sysclk=245721600, _rate=246000000, 
mipi_div=2, prediv=3, mult=123, sysdiv=4
[ 5019.271894] ov5640 1-003c: PCLK PERIOD 0x4837=0x41
[ 5019.329693] st-mipid02 2-0014: clk_lane_reg1=0x81
<<<<<<<<<<<<<<<<< 15.09 fps
Measured #16ns (62.5MHz) => in line with 61430400 Hz


> In my platform this fixes (in example) 640x480@30FPS but breaks
> 640x480@15FPS which now runs at 7.5FPS (with Tomi's patch reverted)..
> What a weird behaviour
> 
> The reasoning behing link_frequency calculation is that
> 
> pixel_rate = vtot * htot * fps
> bit_rate = pixel_rate * bpp
> link_freq = bit_rate / num_lanes / 2 (CSI-2 DDR)
> 
> MIPI_DIV is not yet into play, as we're calculating the CSI-2 clock
> lane freqeuency without applying it to the clock tree
> 
> In my clock diagram link_freq is what is the MIPI_CLK output
> To transform it in SYSCLK you walk the clock tree backward and
> 
> sysclk = link_freq * 2 * mipi_div
> 

Could you add in your codebase the debug patches below and measure the 
clock lane frequency with oscilloscope so that we have a chance to 
understand what happens ?


@@ -1842,6 +1842,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor)
  	bool auto_exp =  sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO;
  	unsigned long rate;
  	int ret;
+	struct i2c_client *client = sensor->i2c_client;

  	if (!orig_mode)
  		orig_mode = mode;
@@ -1867,6 +1868,10 @@ static int ov5640_set_mode(struct ov5640_dev *sensor)
  	 * the same rate than YUV, so we can just use 16 bpp all the time.
  	 */
  	rate = ov5640_calc_pixel_rate(sensor) * 16;
+
+	dev_info(&client->dev, "Bandwidth Per Lane=%lu, %dx%d from %dx%d\n",
+		 rate / sensor->ep.bus.mipi_csi2.num_data_lanes, mode->hact, 
mode->vact, mode->htot, mode->vtot);
+
  	if (sensor->ep.bus_type == V4L2_MBUS_CSI2) {



@@ -944,6 +944,8 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev 
*sensor,
  	unsigned long sysclk;
  	u8 pclk_period;
  	int ret;
+	struct i2c_client *client = sensor->i2c_client;
+	unsigned long _rate;


  	sysclk = link_freq * 2 * mipi_div;
-	ov5640_calc_sys_clk(sensor, sysclk, &prediv, &mult, &sysdiv);
+	_rate = ov5640_calc_sys_clk(sensor, sysclk, &prediv, &mult, &sysdiv);
+
+	dev_info(&client->dev, "sysclk=%lu, _rate=%lu, mipi_div=%d, prediv=%d, 
mult=%d, sysdiv=%d\n",
+		 sysclk, _rate, mipi_div, prediv, mult, sysdiv);



>>
>> To test the setup I have patched the link frequency control to report
>> dynamically the frequency instead of hardcoded value:
>> +#if 0
>>    	freq_index = OV5640_LINK_FREQS_NUM - 1;
>>    	for (i = 0; i < OV5640_LINK_FREQS_NUM; ++i) {
>>    		if (ov5640_link_freqs[i] == link_freq) {
>> @@ -966,18 +979,12 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev
>> *sensor,
>>    	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, freq_index);
>>    	if (ret < 0)
>>    		return ret;
>> +#else
>> +	ov5640_link_freqs[0] = link_freq;
>> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, 0);
>> +#endif
> 
> I wonder if this is acceptable for mainline. Pre-calculating the link
> frequency is really a pain. I wonder why LINK_FREQ is a menu control
> in first place :/
> 

Yes would be nice to get rid of that.

>>
>> 2) Second problem comes from "media: i2c: ov5640: Adjust htot", this is
>> breaking 1024x768@30fps & VGA@30fps which are slowdown to 15fps
> 
> Weird, as 'Adjust htot' -increases- the htot values resulting in a
> -faster- clock output, right ? Are you sure this is not due to the
> above "/ mipi_div;" you've added ?
> 

Another explanation is that there are errors so that 1/2 frame is dropped.

>>
>> 3) I have some instabilities when switching between framerate, I have to
>> investigate the point. In few words this is a race problem between the
>> OV5640 which set the frequency control and the MIPID02 which read the
>> frequency control. I'll dig into the issue to see how to fix that.
>>
>>
>> To summarize:
>> -------------
>> 1) "media: i2c: ov5640: Rework CSI-2 clock tree"
>> Almost OK but mipi_div is missing
>>
>> 2) "media: i2c: ov5640: Adjust htot"
>> Is breaking some resolutions/fps, so better to drop.
>> Tomi, perhaps could you recheck with the fixed Jacopo serie if you still
>> encounter your DPHY error issues ?
>>
>> With 1) fixed and 2) reverted, I'm back on track and have a successfull
>> non-regression on my side + some better figures on some resolutions:
>> - 1024x768@30fps which was not at the right framerate previously
>> - 720p@30fps which was not at the right framerate previously
>> - HD@15fps which was not at the right framerate previously
>>
>> Please note that I cannot go above HD@15fps on this platform.
>>
>> * QCIF  176x144 RGB565 15fps => OK, got 15
>> * QCIF  176x144 YUYV   15fps => OK, got 15
>> * QCIF  176x144 JPEG   15fps => OK, got 15
>> * QCIF  176x144 RGB565 30fps => OK, got 30
>> * QCIF  176x144 YUYV   30fps => OK, got 30
>> * QCIF  176x144 JPEG   30fps => OK, got 30
>> * QVGA  320x240 RGB565 15fps => OK, got 15
>> * QVGA  320x240 YUYV   15fps => OK, got 15
>> * QVGA  320x240 JPEG   15fps => OK, got 15
>> * QVGA  320x240 RGB565 30fps => OK, got 29
>> * QVGA  320x240 YUYV   30fps => OK, got 30
>> * QVGA  320x240 JPEG   30fps => OK, got 29
>> * VGA   640x480 RGB565 15fps => OK, got 15
>> * VGA   640x480 YUYV   15fps => OK, got 15
>> * VGA   640x480 JPEG   15fps => OK, got 15
>> * VGA   640x480 RGB565 30fps => OK, got 30
>> * VGA   640x480 YUYV   30fps => OK, got 30
>> * VGA   640x480 JPEG   30fps => OK, got 30
>> * 480p  720x480 RGB565 15fps => OK, got 15
>> * 480p  720x480 YUYV   15fps => OK, got 15
>> * 480p  720x480 JPEG   15fps => OK, got 15
>> * 480p  720x480 RGB565 30fps => OK, got 30
>> * 480p  720x480 YUYV   30fps => OK, got 30
>> * 480p  720x480 JPEG   30fps => OK, got 30
>> * XGA  1024x768 RGB565 15fps => OK, got 15
>> * XGA  1024x768 YUYV   15fps => OK, got 15
>> * XGA  1024x768 JPEG   15fps => OK, got 15
>> * XGA  1024x768 RGB565 30fps => OK, got 30
>> * XGA  1024x768 YUYV   30fps => OK, got 30
>> * XGA  1024x768 JPEG   30fps => OK, got 30
>> * 720p 1280x720 RGB565 15fps => OK, got 15
>> * 720p 1280x720 YUYV   15fps => OK, got 15
>> * 720p 1280x720 JPEG   15fps => OK, got 15
>> * 720p 1280x720 RGB565 30fps => OK, got 30
>> * 720p 1280x720 YUYV   30fps => OK, got 30
>> * 720p 1280x720 JPEG   30fps => OK, got 30
>> * HD  1920x1080 RGB565 15fps => OK, got 15
>> * HD  1920x1080 YUYV   15fps => OK, got 15
>> * HD  1920x1080 JPEG   15fps => OK, got 15
>>
>>
>> So in few words, it sounds good, thanks Jacopo !
> 
> That's sweet, but doesn't match what I see on iMX.6 /o\

Yes, I feel that debug traces and oscilloscope will help to understand 
what happens.

> 
> 
>>
>>
>> On 10/28/20 11:57 PM, Jacopo Mondi wrote:
>>> Hi Hugues Tomi and Sam
>>>
>>>      this small series collects Tomi's patch on adjusting htot which has been
>>> floating around for some time with a rework of the clock tree based on
>>> Hugues' and Sam's work on setting pclk_period. It also address the need to
>>> suppport LINK_FREQUENCY control as pointed out by Hugues.
>>>
>>> I'm sort of happy with the result as I've removed quite some chrun and the clock
>>> tree calculation is more linear. All modes work except full-resolution which a
>>> bit annoys me, as I can't select it through s_fmt (to be honest I have not
>>> investigated that in detail, that's why an RFC).
>>>
>>> Framerate is better than before, but still off for some combinations:
>>> 640x480@30 gives me ~40 FPS, 1920x1080@15 gives me ~7.
>>> The other combinations I've tested looks good.
>>>
>>> Can I have your opinion on these changes and if they help you with your
>>> platforms?
>>>
>>> I've only been able to test YUYV, support for formats with != bpp will need
>>> some work most probably, but that was like this before (although iirc Hugues
>>> has captured JPEG, right ?)
>>>
>>> There's a bit more cleanup on top to be done (I've left TODOs around) and
>>> probably the HBLANK calculation should be checked to see if it works with the
>>> new htot values.
>>>
>>> Thanks
>>>     j
>>>
>>> Jacopo Mondi (2):
>>>     media: i2c: ov5640: Rework CSI-2 clock tree
>>>     media: i2c: ov5640: Add V4L2_CID_LINK_FREQ support
>>>
>>> Tomi Valkeinen (1):
>>>     media: i2c: ov5640: Adjust htot
>>>
>>>    drivers/media/i2c/ov5640.c | 176 +++++++++++++++++++++++++------------
>>>    1 file changed, 118 insertions(+), 58 deletions(-)
>>>
>>> --
>>> 2.28.0
>>>
>>
>> Best regards,
>> Hugues.

BR,
Hugues.

Re: [RFC 0/3] media: ov5640: Adjust htot, rework clock tree, add LINK_FREQ

[Jacopo Mondi]

Hi Hugues,
   thanks for the detail, as soon as I have a bit of time I'll re-look
into this.

But in the meantime, I wonder, are you testing with JPEG only ?
What is the bpp of a JPEG image ?

So far, I only tested with YUYV as that's what I can capture on my
platform...

On Thu, Nov 05, 2020 at 03:33:18PM +0000, Hugues FRUCHET wrote:
> Hi Jacopo,
>
> On 11/5/20 11:14 AM, Jacopo Mondi wrote:
> > Hello Hugues,
> >
> >      thanks so much for testing
> >
> > On Tue, Nov 03, 2020 at 04:53:21PM +0000, Hugues FRUCHET wrote:
> >> Hi Jacopo,
> >>
> >> Here is the results of tests with 0V5640 CSI-2 on Avenger96 board.
> >>
> >> 1) First of all, the framerate is broken, it is almost 2 times greater
> >> that expected. Checking code it seems that mipi_div is missing when
> >> computing link_freq:
> >>
> >> +	/*
> >>    	 * The 'rate' parameter is the bitrate = VTOT * HTOT * FPS * BPP
> >>    	 *
> >>    	 * Adjust it to represent the CSI-2 link frequency and use it to
> >>    	 * update the associated control.
> >>    	 */
> >> -	link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2;
> >> +	link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2 / mipi_div;
> >
> > I don't think this is correct I'm sorry.
> >
>
> But this is what is observed with oscilloscope:
>
> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=30;v4l2-ctl
> --set-fmt-video=width=640,height=480,pixelformat=JPEG --stream-mmap
> --stream-count=-1
> Frame rate set to 30.000 fps
> [ 3501.482829] ov5640 1-003c: Bandwidth Per Lane=491443200, 640x480 from
> 1896x1080
> [ 3501.488822] ov5640 1-003c: ov5640_set_mipi_pclk: __v4l2_ctrl_s_ctrl 0
> 122860800 Hz
> [ 3501.496415] ov5640 1-003c: sysclk=491443200, _rate=492000000,
> mipi_div=2, prediv=3, mult=123, sysdiv=2
> [ 3501.511064] ov5640 1-003c: PCLK PERIOD 0x4837=0x20
> [ 3501.569487] st-mipid02 2-0014: clk_lane_reg1=0x41
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.00 fps
> Measured #8ns (125MHz) ==> in line with 122860800 Hz
>
> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
> --set-fmt-video=width=640,height=480,pixelformat=JPEG --stream-mmap
> --stream-count=-1
> Frame rate set to 15.000 fps
> [ 5019.240550] ov5640 1-003c: Bandwidth Per Lane=245721600, 640x480 from
> 1896x1080
> [ 5019.246542] ov5640 1-003c: ov5640_set_mipi_pclk: __v4l2_ctrl_s_ctrl 0
> 61430400 Hz
> [ 5019.257485] ov5640 1-003c: sysclk=245721600, _rate=246000000,
> mipi_div=2, prediv=3, mult=123, sysdiv=4
> [ 5019.271894] ov5640 1-003c: PCLK PERIOD 0x4837=0x41
> [ 5019.329693] st-mipid02 2-0014: clk_lane_reg1=0x81
> <<<<<<<<<<<<<<<<< 15.09 fps
> Measured #16ns (62.5MHz) => in line with 61430400 Hz
>
>
> > In my platform this fixes (in example) 640x480@30FPS but breaks
> > 640x480@15FPS which now runs at 7.5FPS (with Tomi's patch reverted)..
> > What a weird behaviour
> >
> > The reasoning behing link_frequency calculation is that
> >
> > pixel_rate = vtot * htot * fps
> > bit_rate = pixel_rate * bpp
> > link_freq = bit_rate / num_lanes / 2 (CSI-2 DDR)
> >
> > MIPI_DIV is not yet into play, as we're calculating the CSI-2 clock
> > lane freqeuency without applying it to the clock tree
> >
> > In my clock diagram link_freq is what is the MIPI_CLK output
> > To transform it in SYSCLK you walk the clock tree backward and
> >
> > sysclk = link_freq * 2 * mipi_div
> >
>
> Could you add in your codebase the debug patches below and measure the
> clock lane frequency with oscilloscope so that we have a chance to
> understand what happens ?
>
>
> @@ -1842,6 +1842,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor)
>   	bool auto_exp =  sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO;
>   	unsigned long rate;
>   	int ret;
> +	struct i2c_client *client = sensor->i2c_client;
>
>   	if (!orig_mode)
>   		orig_mode = mode;
> @@ -1867,6 +1868,10 @@ static int ov5640_set_mode(struct ov5640_dev *sensor)
>   	 * the same rate than YUV, so we can just use 16 bpp all the time.
>   	 */
>   	rate = ov5640_calc_pixel_rate(sensor) * 16;
> +
> +	dev_info(&client->dev, "Bandwidth Per Lane=%lu, %dx%d from %dx%d\n",
> +		 rate / sensor->ep.bus.mipi_csi2.num_data_lanes, mode->hact,
> mode->vact, mode->htot, mode->vtot);
> +
>   	if (sensor->ep.bus_type == V4L2_MBUS_CSI2) {
>
>
>
> @@ -944,6 +944,8 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev
> *sensor,
>   	unsigned long sysclk;
>   	u8 pclk_period;
>   	int ret;
> +	struct i2c_client *client = sensor->i2c_client;
> +	unsigned long _rate;
>
>
>   	sysclk = link_freq * 2 * mipi_div;
> -	ov5640_calc_sys_clk(sensor, sysclk, &prediv, &mult, &sysdiv);
> +	_rate = ov5640_calc_sys_clk(sensor, sysclk, &prediv, &mult, &sysdiv);
> +
> +	dev_info(&client->dev, "sysclk=%lu, _rate=%lu, mipi_div=%d, prediv=%d,
> mult=%d, sysdiv=%d\n",
> +		 sysclk, _rate, mipi_div, prediv, mult, sysdiv);
>
>
>
> >>
> >> To test the setup I have patched the link frequency control to report
> >> dynamically the frequency instead of hardcoded value:
> >> +#if 0
> >>    	freq_index = OV5640_LINK_FREQS_NUM - 1;
> >>    	for (i = 0; i < OV5640_LINK_FREQS_NUM; ++i) {
> >>    		if (ov5640_link_freqs[i] == link_freq) {
> >> @@ -966,18 +979,12 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev
> >> *sensor,
> >>    	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, freq_index);
> >>    	if (ret < 0)
> >>    		return ret;
> >> +#else
> >> +	ov5640_link_freqs[0] = link_freq;
> >> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, 0);
> >> +#endif
> >
> > I wonder if this is acceptable for mainline. Pre-calculating the link
> > frequency is really a pain. I wonder why LINK_FREQ is a menu control
> > in first place :/
> >
>
> Yes would be nice to get rid of that.
>
> >>
> >> 2) Second problem comes from "media: i2c: ov5640: Adjust htot", this is
> >> breaking 1024x768@30fps & VGA@30fps which are slowdown to 15fps
> >
> > Weird, as 'Adjust htot' -increases- the htot values resulting in a
> > -faster- clock output, right ? Are you sure this is not due to the
> > above "/ mipi_div;" you've added ?
> >
>
> Another explanation is that there are errors so that 1/2 frame is dropped.
>
> >>
> >> 3) I have some instabilities when switching between framerate, I have to
> >> investigate the point. In few words this is a race problem between the
> >> OV5640 which set the frequency control and the MIPID02 which read the
> >> frequency control. I'll dig into the issue to see how to fix that.
> >>
> >>
> >> To summarize:
> >> -------------
> >> 1) "media: i2c: ov5640: Rework CSI-2 clock tree"
> >> Almost OK but mipi_div is missing
> >>
> >> 2) "media: i2c: ov5640: Adjust htot"
> >> Is breaking some resolutions/fps, so better to drop.
> >> Tomi, perhaps could you recheck with the fixed Jacopo serie if you still
> >> encounter your DPHY error issues ?
> >>
> >> With 1) fixed and 2) reverted, I'm back on track and have a successfull
> >> non-regression on my side + some better figures on some resolutions:
> >> - 1024x768@30fps which was not at the right framerate previously
> >> - 720p@30fps which was not at the right framerate previously
> >> - HD@15fps which was not at the right framerate previously
> >>
> >> Please note that I cannot go above HD@15fps on this platform.
> >>
> >> * QCIF  176x144 RGB565 15fps => OK, got 15
> >> * QCIF  176x144 YUYV   15fps => OK, got 15
> >> * QCIF  176x144 JPEG   15fps => OK, got 15
> >> * QCIF  176x144 RGB565 30fps => OK, got 30
> >> * QCIF  176x144 YUYV   30fps => OK, got 30
> >> * QCIF  176x144 JPEG   30fps => OK, got 30
> >> * QVGA  320x240 RGB565 15fps => OK, got 15
> >> * QVGA  320x240 YUYV   15fps => OK, got 15
> >> * QVGA  320x240 JPEG   15fps => OK, got 15
> >> * QVGA  320x240 RGB565 30fps => OK, got 29
> >> * QVGA  320x240 YUYV   30fps => OK, got 30
> >> * QVGA  320x240 JPEG   30fps => OK, got 29
> >> * VGA   640x480 RGB565 15fps => OK, got 15
> >> * VGA   640x480 YUYV   15fps => OK, got 15
> >> * VGA   640x480 JPEG   15fps => OK, got 15
> >> * VGA   640x480 RGB565 30fps => OK, got 30
> >> * VGA   640x480 YUYV   30fps => OK, got 30
> >> * VGA   640x480 JPEG   30fps => OK, got 30
> >> * 480p  720x480 RGB565 15fps => OK, got 15
> >> * 480p  720x480 YUYV   15fps => OK, got 15
> >> * 480p  720x480 JPEG   15fps => OK, got 15
> >> * 480p  720x480 RGB565 30fps => OK, got 30
> >> * 480p  720x480 YUYV   30fps => OK, got 30
> >> * 480p  720x480 JPEG   30fps => OK, got 30
> >> * XGA  1024x768 RGB565 15fps => OK, got 15
> >> * XGA  1024x768 YUYV   15fps => OK, got 15
> >> * XGA  1024x768 JPEG   15fps => OK, got 15
> >> * XGA  1024x768 RGB565 30fps => OK, got 30
> >> * XGA  1024x768 YUYV   30fps => OK, got 30
> >> * XGA  1024x768 JPEG   30fps => OK, got 30
> >> * 720p 1280x720 RGB565 15fps => OK, got 15
> >> * 720p 1280x720 YUYV   15fps => OK, got 15
> >> * 720p 1280x720 JPEG   15fps => OK, got 15
> >> * 720p 1280x720 RGB565 30fps => OK, got 30
> >> * 720p 1280x720 YUYV   30fps => OK, got 30
> >> * 720p 1280x720 JPEG   30fps => OK, got 30
> >> * HD  1920x1080 RGB565 15fps => OK, got 15
> >> * HD  1920x1080 YUYV   15fps => OK, got 15
> >> * HD  1920x1080 JPEG   15fps => OK, got 15
> >>
> >>
> >> So in few words, it sounds good, thanks Jacopo !
> >
> > That's sweet, but doesn't match what I see on iMX.6 /o\
>
> Yes, I feel that debug traces and oscilloscope will help to understand
> what happens.
>
> >
> >
> >>
> >>
> >> On 10/28/20 11:57 PM, Jacopo Mondi wrote:
> >>> Hi Hugues Tomi and Sam
> >>>
> >>>      this small series collects Tomi's patch on adjusting htot which has been
> >>> floating around for some time with a rework of the clock tree based on
> >>> Hugues' and Sam's work on setting pclk_period. It also address the need to
> >>> suppport LINK_FREQUENCY control as pointed out by Hugues.
> >>>
> >>> I'm sort of happy with the result as I've removed quite some chrun and the clock
> >>> tree calculation is more linear. All modes work except full-resolution which a
> >>> bit annoys me, as I can't select it through s_fmt (to be honest I have not
> >>> investigated that in detail, that's why an RFC).
> >>>
> >>> Framerate is better than before, but still off for some combinations:
> >>> 640x480@30 gives me ~40 FPS, 1920x1080@15 gives me ~7.
> >>> The other combinations I've tested looks good.
> >>>
> >>> Can I have your opinion on these changes and if they help you with your
> >>> platforms?
> >>>
> >>> I've only been able to test YUYV, support for formats with != bpp will need
> >>> some work most probably, but that was like this before (although iirc Hugues
> >>> has captured JPEG, right ?)
> >>>
> >>> There's a bit more cleanup on top to be done (I've left TODOs around) and
> >>> probably the HBLANK calculation should be checked to see if it works with the
> >>> new htot values.
> >>>
> >>> Thanks
> >>>     j
> >>>
> >>> Jacopo Mondi (2):
> >>>     media: i2c: ov5640: Rework CSI-2 clock tree
> >>>     media: i2c: ov5640: Add V4L2_CID_LINK_FREQ support
> >>>
> >>> Tomi Valkeinen (1):
> >>>     media: i2c: ov5640: Adjust htot
> >>>
> >>>    drivers/media/i2c/ov5640.c | 176 +++++++++++++++++++++++++------------
> >>>    1 file changed, 118 insertions(+), 58 deletions(-)
> >>>
> >>> --
> >>> 2.28.0
> >>>
> >>
> >> Best regards,
> >> Hugues.
>
> BR,
> Hugues.