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 = &ov5640_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, &fi->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;
>
> mode = ov5640_fi
