Re: [PATCH 11/12] media: ov5640: Add 60 fps support

[Maxime Ripard]

Hi Hugues,

Thanks for all your feedback, I'll merge your suggested changes in the
next iteration.

On Tue, Mar 13, 2018 at 02:32:14PM +, Hugues FRUCHET wrote:
> > if (fi->numerator == 0) {
> > fi->denominator = maxfps;
> > fi->numerator = 1;
> > -   return OV5640_30_FPS;
> > +   return OV5640_60_FPS;
>
> [...]
>
> About 60 fps by default if (fi->numerator == 0): shouldn't we stick to a 
> default value supported by all modes such as 30fps ?

30 fps is not supported by all modes either, so I guess 15 fps would
be a better pick?

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH 11/12] media: ov5640: Add 60 fps support

[Hugues FRUCHET]

Hi Maxime,

See below rest of comments regarding framerate and compliance failure.

On 03/02/2018 03:34 PM, Maxime Ripard wrote:
> Now that we have everything in place to compute the clock rate at runtime,
> we can enable the 60fps framerate for the mode we tested it with.
> 
> Signed-off-by: Maxime Ripard 
> ---
>   drivers/media/i2c/ov5640.c | 20 +++-
>   1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 5510a19281a4..03838f42fb82 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -111,6 +111,7 @@ enum ov5640_mode_id {
>   enum ov5640_frame_rate {
>   OV5640_15_FPS = 0,
>   OV5640_30_FPS,
> + OV5640_60_FPS,
>   OV5640_NUM_FRAMERATES,
>   };
>   
> @@ -144,6 +145,7 @@ MODULE_PARM_DESC(virtual_channel,
>   static const int ov5640_framerates[] = {
>   [OV5640_15_FPS] = 15,
>   [OV5640_30_FPS] = 30,
> + [OV5640_60_FPS] = 60,
>   };
>   
>   /* regulator supplies */
> @@ -1447,6 +1449,11 @@ ov5640_find_mode(struct ov5640_dev *sensor, enum 
> ov5640_frame_rate fr,
>   fr != OV5640_15_FPS)
>   return NULL;
>   
> + /* Only 640x480 can operate at 60fps (for now) */
> + if (fr == OV5640_60_FPS &&
> + width != 640 && height != 480)
> + return NULL;
> +

Same comment as on patchset 10/12, VGA exception put in for loop:

for (i = OV5640_NUM_MODES - 1; i >= 0; i--) {
mode = _mode_data[i];

if (!mode->reg_data)
continue;

if ((nearest && mode->hact <= width &&
 mode->vact <= height) ||
(!nearest && mode->hact == width &&
 mode->vact == height)) {

/* 2592x1944 can only operate at 15fps */
if (mode->hact == 2592 && mode->vact == 1944 &&
fr != OV5640_15_FPS)
continue;/* next lower resolution */

/* Only 640x480 can operate at 60fps (for now) */
if (fr == OV5640_60_FPS &&
!(mode->hact == 640 && mode->vact == 480))
continue;/* next lower resolution */

break;/* select this resolution */
}
}


>   return mode;
>   }
>   
> @@ -1875,12 +1882,12 @@ static int ov5640_try_frame_interval(struct 
> ov5640_dev *sensor,
>   int ret;
>   
>   minfps = ov5640_framerates[OV5640_15_FPS];
> - maxfps = ov5640_framerates[OV5640_30_FPS];
> + maxfps = ov5640_framerates[OV5640_60_FPS];
>   
>   if (fi->numerator == 0) {
>   fi->denominator = maxfps;
>   fi->numerator = 1;
> - return OV5640_30_FPS;
> + return OV5640_60_FPS;

There is a problem here because we don't validate that 60fps is 
supported in the currently set mode, we must inject this framerate value 
in ov5640_find_mode(framerate, width, height) in order to validate it 
(-EINVAL if not supported):

  + ret = OV5640_60_FPS;
  + goto find_mode;
  + }
[...]
+find_mode:
mode = ov5640_find_mode(sensor, frame_rate, width, height, false);
return mode ? ret : -EINVAL;
  }

Then we have to catch error in ov5640_s_frame_interval() and return an 
acceptable frame interval to user:

frame_rate = ov5640_try_frame_interval(sensor, >interval,
   mode->hact, mode->vact);
-   if (frame_rate < 0)
-   frame_rate = OV5640_15_FPS;
-
-   sensor->current_fr = frame_rate;
-   sensor->frame_interval = fi->interval;
We also have to update current framerate only if framerate has been 
validated.

+   if (frame_rate < 0) {
+   /* return a valid frame interval value */
+   fi->interval = sensor->frame_interval;
goto out;
}

+   sensor->current_fr = frame_rate;
+   sensor->frame_interval = fi->interval;
sensor->pending_mode_change = true;
  out:


About 60 fps by default if (fi->numerator == 0): shouldn't we stick to a 
default value supported by all modes such as 30fps ?

>   }
>   
>   fps = DIV_ROUND_CLOSEST(fi->denominator, fi->numerator);
> @@ -1892,10 +1899,13 @@ static int ov5640_try_frame_interval(struct 
> ov5640_dev *sensor,
>   fi->denominator = minfps;
>   else if (2 * fps >= 2 * minfps + (maxfps - minfps))
>   fi->denominator = maxfps;
> - else
> - fi->denominator = minfps;
>   
> - ret = (fi->denominator == minfps) ? OV5640_15_FPS : OV5640_30_FPS;
> + if (fi->denominator == minfps)
> + ret = OV5640_15_FPS;
> + else if (fi->denominator == maxfps)
> + ret = OV5640_60_FPS;
> + else
> + ret = OV5640_30_FPS;
>   
>   

[PATCH 11/12] media: ov5640: Add 60 fps support

[Maxime Ripard]

Now that we have everything in place to compute the clock rate at runtime,
we can enable the 60fps framerate for the mode we tested it with.

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

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 5510a19281a4..03838f42fb82 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -111,6 +111,7 @@ enum ov5640_mode_id {
 enum ov5640_frame_rate {
OV5640_15_FPS = 0,
OV5640_30_FPS,
+   OV5640_60_FPS,
OV5640_NUM_FRAMERATES,
 };
 
@@ -144,6 +145,7 @@ MODULE_PARM_DESC(virtual_channel,
 static const int ov5640_framerates[] = {
[OV5640_15_FPS] = 15,
[OV5640_30_FPS] = 30,
+   [OV5640_60_FPS] = 60,
 };
 
 /* regulator supplies */
@@ -1447,6 +1449,11 @@ ov5640_find_mode(struct ov5640_dev *sensor, enum 
ov5640_frame_rate fr,
fr != OV5640_15_FPS)
return NULL;
 
+   /* Only 640x480 can operate at 60fps (for now) */
+   if (fr == OV5640_60_FPS &&
+   width != 640 && height != 480)
+   return NULL;
+
return mode;
 }
 
@@ -1875,12 +1882,12 @@ static int ov5640_try_frame_interval(struct ov5640_dev 
*sensor,
int ret;
 
minfps = ov5640_framerates[OV5640_15_FPS];
-   maxfps = ov5640_framerates[OV5640_30_FPS];
+   maxfps = ov5640_framerates[OV5640_60_FPS];
 
if (fi->numerator == 0) {
fi->denominator = maxfps;
fi->numerator = 1;
-   return OV5640_30_FPS;
+   return OV5640_60_FPS;
}
 
fps = DIV_ROUND_CLOSEST(fi->denominator, fi->numerator);
@@ -1892,10 +1899,13 @@ static int ov5640_try_frame_interval(struct ov5640_dev 
*sensor,
fi->denominator = minfps;
else if (2 * fps >= 2 * minfps + (maxfps - minfps))
fi->denominator = maxfps;
-   else
-   fi->denominator = minfps;
 
-   ret = (fi->denominator == minfps) ? OV5640_15_FPS : OV5640_30_FPS;
+   if (fi->denominator == minfps)
+   ret = OV5640_15_FPS;
+   else if (fi->denominator == maxfps)
+   ret = OV5640_60_FPS;
+   else
+   ret = OV5640_30_FPS;
 
mode = ov5640_find_mode(sensor, ret, width, height, false);
return mode ? ret : -EINVAL;
-- 
2.14.3