Re: [PATCH 09/12] media: ov5640: Compute the clock rate at runtime

[Hugues FRUCHET]

Hi Maxime,

Below comments about JPEG issue.

On 03/02/2018 03:34 PM, Maxime Ripard wrote:
> The clock rate, while hardcoded until now, is actually a function of the
> resolution, framerate and bytes per pixel. Now that we have an algorithm to
> adjust our clock rate, we can select it dynamically when we change the
> mode.
> 
> This changes a bit the clock rate being used, with the following effect:
> 
> +--+--+--+--+-+-++---+
> | Hact | Vact | Htot | Vtot | FPS | Hardcoded clock | Computed clock | 
> Deviation |
> +--+--+--+--+-+-++---+
> |  640 |  480 | 1896 | 1080 |  15 |5600 |   61430400 | 8.84 % 
>|
> |  640 |  480 | 1896 | 1080 |  30 |   11200 |  122860800 | 8.84 % 
>|
> | 1024 |  768 | 1896 | 1080 |  15 |5600 |   61430400 | 8.84 % 
>|
> | 1024 |  768 | 1896 | 1080 |  30 |   11200 |  122860800 | 8.84 % 
>|
> |  320 |  240 | 1896 |  984 |  15 |5600 |   55969920 | 0.05 % 
>|
> |  320 |  240 | 1896 |  984 |  30 |   11200 |  111939840 | 0.05 % 
>|
> |  176 |  144 | 1896 |  984 |  15 |5600 |   55969920 | 0.05 % 
>|
> |  176 |  144 | 1896 |  984 |  30 |   11200 |  111939840 | 0.05 % 
>|
> |  720 |  480 | 1896 |  984 |  15 |5600 |   55969920 | 0.05 % 
>|
> |  720 |  480 | 1896 |  984 |  30 |   11200 |  111939840 | 0.05 % 
>|
> |  720 |  576 | 1896 |  984 |  15 |5600 |   55969920 | 0.05 % 
>|
> |  720 |  576 | 1896 |  984 |  30 |   11200 |  111939840 | 0.05 % 
>|
> | 1280 |  720 | 1892 |  740 |  15 |4200 |   42002400 | 0.01 % 
>|
> | 1280 |  720 | 1892 |  740 |  30 |8400 |   84004800 | 0.01 % 
>|
> | 1920 | 1080 | 2500 | 1120 |  15 |8400 |   8400 | 0.00 % 
>|
> | 1920 | 1080 | 2500 | 1120 |  30 |   16800 |  16800 | 0.00 % 
>|
> | 2592 | 1944 | 2844 | 1944 |  15 |8400 |  165862080 | 49.36 
> %   |
> +--+--+--+--+-+-++---+
> 
> Only the 640x480, 1024x768 and 2592x1944 modes are significantly affected
> by the new formula.
> 
> In this case, 640x480 and 1024x768 are actually fixed by this driver.
> Indeed, the sensor was sending data at, for example, 27.33fps instead of
> 30fps. This is -9%, which is roughly what we're seeing in the array.
> Testing these modes with the new clock setup actually fix that error, and
> data are now sent at around 30fps.
> 
> 2592x1944, on the other hand, is probably due to the fact that this mode
> can only be used using MIPI-CSI2, in a two lane mode. This would have to be
> tested though.
> 
> Signed-off-by: Maxime Ripard 
> ---
>   drivers/media/i2c/ov5640.c | 41 +
>   1 file changed, 17 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 323cde27dd8b..bdf378d80e07 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -126,6 +126,12 @@ static const struct ov5640_pixfmt ov5640_formats[] = {
>   { MEDIA_BUS_FMT_RGB565_2X8_BE, V4L2_COLORSPACE_SRGB, },
>   };
>   
> +/*
> + * FIXME: If we ever have something else, we'll obviously need to have
> + * something smarter.
> + */
> +#define OV5640_FORMATS_BPP   2
> +

We have the case with JPEG which is 1 byte.

>   /*
>* FIXME: remove this when a subdev API becomes available
>* to set the MIPI CSI-2 virtual channel.
> @@ -172,7 +178,6 @@ struct ov5640_mode_info {
>   u32 htot;
>   u32 vact;
>   u32 vtot;
> - u32 clock;
>   const struct reg_value *reg_data;
>   u32 reg_data_size;
>   };
> @@ -696,7 +701,6 @@ static const struct reg_value 
> ov5640_setting_15fps_QSXGA_2592_1944[] = {
>   /* power-on sensor init reg table */
>   static const struct ov5640_mode_info ov5640_mode_init_data = {
>   0, SUBSAMPLING, 640, 1896, 480, 984,
> - 11200,
>   ov5640_init_setting_30fps_VGA,
>   ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
>   };
> @@ -706,91 +710,74 @@ 
> ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
>   {
>   {OV5640_MODE_QCIF_176_144, SUBSAMPLING,
>176, 1896, 144, 984,
> -  5600,
>ov5640_setting_15fps_QCIF_176_144,
>ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
>   {OV5640_MODE_QVGA_320_240, SUBSAMPLING,
>320, 1896, 240, 984,
> -  5600,
>ov5640_setting_15fps_QVGA_320_240,
>ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
>   {OV5640_MODE_VGA_640_480, SUBSAMPLING,
>640, 1896, 480, 1080,
> -  5600,
>

[PATCH 09/12] media: ov5640: Compute the clock rate at runtime

[Maxime Ripard]

The clock rate, while hardcoded until now, is actually a function of the
resolution, framerate and bytes per pixel. Now that we have an algorithm to
adjust our clock rate, we can select it dynamically when we change the
mode.

This changes a bit the clock rate being used, with the following effect:

+--+--+--+--+-+-++---+
| Hact | Vact | Htot | Vtot | FPS | Hardcoded clock | Computed clock | 
Deviation |
+--+--+--+--+-+-++---+
|  640 |  480 | 1896 | 1080 |  15 |5600 |   61430400 | 8.84 %   
 |
|  640 |  480 | 1896 | 1080 |  30 |   11200 |  122860800 | 8.84 %   
 |
| 1024 |  768 | 1896 | 1080 |  15 |5600 |   61430400 | 8.84 %   
 |
| 1024 |  768 | 1896 | 1080 |  30 |   11200 |  122860800 | 8.84 %   
 |
|  320 |  240 | 1896 |  984 |  15 |5600 |   55969920 | 0.05 %   
 |
|  320 |  240 | 1896 |  984 |  30 |   11200 |  111939840 | 0.05 %   
 |
|  176 |  144 | 1896 |  984 |  15 |5600 |   55969920 | 0.05 %   
 |
|  176 |  144 | 1896 |  984 |  30 |   11200 |  111939840 | 0.05 %   
 |
|  720 |  480 | 1896 |  984 |  15 |5600 |   55969920 | 0.05 %   
 |
|  720 |  480 | 1896 |  984 |  30 |   11200 |  111939840 | 0.05 %   
 |
|  720 |  576 | 1896 |  984 |  15 |5600 |   55969920 | 0.05 %   
 |
|  720 |  576 | 1896 |  984 |  30 |   11200 |  111939840 | 0.05 %   
 |
| 1280 |  720 | 1892 |  740 |  15 |4200 |   42002400 | 0.01 %   
 |
| 1280 |  720 | 1892 |  740 |  30 |8400 |   84004800 | 0.01 %   
 |
| 1920 | 1080 | 2500 | 1120 |  15 |8400 |   8400 | 0.00 %   
 |
| 1920 | 1080 | 2500 | 1120 |  30 |   16800 |  16800 | 0.00 %   
 |
| 2592 | 1944 | 2844 | 1944 |  15 |8400 |  165862080 | 49.36 %  
 |
+--+--+--+--+-+-++---+

Only the 640x480, 1024x768 and 2592x1944 modes are significantly affected
by the new formula.

In this case, 640x480 and 1024x768 are actually fixed by this driver.
Indeed, the sensor was sending data at, for example, 27.33fps instead of
30fps. This is -9%, which is roughly what we're seeing in the array.
Testing these modes with the new clock setup actually fix that error, and
data are now sent at around 30fps.

2592x1944, on the other hand, is probably due to the fact that this mode
can only be used using MIPI-CSI2, in a two lane mode. This would have to be
tested though.

Signed-off-by: Maxime Ripard 
---
 drivers/media/i2c/ov5640.c | 41 +
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 323cde27dd8b..bdf378d80e07 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -126,6 +126,12 @@ static const struct ov5640_pixfmt ov5640_formats[] = {
{ MEDIA_BUS_FMT_RGB565_2X8_BE, V4L2_COLORSPACE_SRGB, },
 };
 
+/*
+ * FIXME: If we ever have something else, we'll obviously need to have
+ * something smarter.
+ */
+#define OV5640_FORMATS_BPP 2
+
 /*
  * FIXME: remove this when a subdev API becomes available
  * to set the MIPI CSI-2 virtual channel.
@@ -172,7 +178,6 @@ struct ov5640_mode_info {
u32 htot;
u32 vact;
u32 vtot;
-   u32 clock;
const struct reg_value *reg_data;
u32 reg_data_size;
 };
@@ -696,7 +701,6 @@ static const struct reg_value 
ov5640_setting_15fps_QSXGA_2592_1944[] = {
 /* power-on sensor init reg table */
 static const struct ov5640_mode_info ov5640_mode_init_data = {
0, SUBSAMPLING, 640, 1896, 480, 984,
-   11200,
ov5640_init_setting_30fps_VGA,
ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
 };
@@ -706,91 +710,74 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] 
= {
{
{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
 176, 1896, 144, 984,
-5600,
 ov5640_setting_15fps_QCIF_176_144,
 ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
 320, 1896, 240, 984,
-5600,
 ov5640_setting_15fps_QVGA_320_240,
 ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
{OV5640_MODE_VGA_640_480, SUBSAMPLING,
 640, 1896, 480, 1080,
-5600,
 ov5640_setting_15fps_VGA_640_480,
 ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)},
{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
 720, 1896, 480, 984,
-5600,
 ov5640_setting_15fps_NTSC_720_480,
 ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480)},