Hi Maxime,
I've taken the whole serie and made some tests on STM32 platform using
DVP parallel interface.
Now JPEG is OK and I've not seen any regressions appart on framerate
control linked to this current patchset.
Here are issues observed around framerate control:
1) Framerate enumeration is buggy and all resolutions are claimed
supporting 15/30/60:
v4l2-ctl --list-formats-ext
ioctl: VIDIOC_ENUM_FMT
Index : 0
Type : Video Capture
Pixel Format: 'JPEG' (compressed)
Name : JFIF JPEG
Size: Discrete 176x144
Interval: Discrete 0.067s (15.000 fps)
Interval: Discrete 0.033s (30.000 fps)
Interval: Discrete 0.017s (60.000 fps)
Size: Discrete 320x240
Interval: Discrete 0.067s (15.000 fps)
Interval: Discrete 0.033s (30.000 fps)
Interval: Discrete 0.017s (60.000 fps)
Size: Discrete 640x480
Interval: Discrete 0.067s (15.000 fps)
Interval: Discrete 0.033s (30.000 fps)
Interval: Discrete 0.017s (60.000 fps)
Size: Discrete 720x480
Interval: Discrete 0.067s (15.000 fps)
Interval: Discrete 0.033s (30.000 fps)
Interval: Discrete 0.017s (60.000 fps)
Size: Discrete 720x576
Interval: Discrete 0.067s (15.000 fps)
Interval: Discrete 0.033s (30.000 fps)
Interval: Discrete 0.017s (60.000 fps)
Size: Discrete 1024x768
Interval: Discrete 0.067s (15.000 fps)
Interval: Discrete 0.033s (30.000 fps)
Interval: Discrete 0.017s (60.000 fps)
Size: Discrete 1280x720
Interval: Discrete 0.067s (15.000 fps)
Interval: Discrete 0.033s (30.000 fps)
Interval: Discrete 0.017s (60.000 fps)
Size: Discrete 1920x1080
Interval: Discrete 0.067s (15.000 fps)
Interval: Discrete 0.033s (30.000 fps)
Interval: Discrete 0.017s (60.000 fps)
Size: Discrete 2592x1944
Interval: Discrete 0.067s (15.000 fps)
Interval: Discrete 0.033s (30.000 fps)
Interval: Discrete 0.017s (60.000 fps)
2) Frame interval setting is returning incorrect value (*1000):
v4l2-ctl --set-parm=15
<
Frame rate set to 15000.000 fps
3) After having fixed 1) and 2), 720x480 60fps is still supported:
Size: Discrete 640x480
Interval: Discrete 0.067s (15.000 fps)
Interval: Discrete 0.033s (30.000 fps)
Interval: Discrete 0.017s (60.000 fps)
Size: Discrete 720x480
Interval: Discrete 0.067s (15.000 fps)
Interval: Discrete 0.033s (30.000 fps)
Interval: Discrete 0.017s (60.000 fps)
Some fixes are proposed below:
On 04/16/2018 02:37 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 <maxime.ripard@xxxxxxxxxxx>
> ---
> drivers/media/i2c/ov5640.c | 33 +++++++++++++++++++++++++--------
> 1 file changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 690ed0238763..c01bbc5f9f34 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -112,6 +112,7 @@ enum ov5640_mode_id {
> enum ov5640_frame_rate {
> OV5640_15_FPS = 0,
> OV5640_30_FPS,
> + OV5640_60_FPS,
> OV5640_NUM_FRAMERATES,
> };
>
> @@ -140,6 +141,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 */
> @@ -1398,12 +1400,19 @@ ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
> /* try to find another mode */
> continue;
>
> + /* Only 640x480 can operate at 60fps (for now) */
> + if (fr == OV5640_60_FPS &&
> + width != 640 && height != 480)
Should be
+ !(width == 640 && height == 480))
otherwise 720x480 is also supported (bug 3))
> + /* try to find another mode */
> + continue;
> +
> break;
> }
> }
>
> + /* VGA is the only mode that supports all the framerates */
> if (nearest && i < 0)
> - mode = &ov5640_mode_data[0];
> + mode = &ov5640_mode_data[OV5640_MODE_VGA_640_480];
Code is missing to reject unsupported framerates when nearest is not
set, this fix enumeration bug 1):
- if (nearest && i < 0)
+ if (i < 0) {
+ /* no match */
+ if (!nearest)
+ return NULL;
mode = &ov5640_mode_data[OV5640_MODE_VGA_640_480];
+ }
Anyway I need to rework this code using new Sakari
v4l2_find_nearest_size() code as stated in:
https://patchwork.linuxtv.org/patch/47767/
>
> return mode;
> }
> @@ -1848,12 +1857,13 @@ 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;
> + ret = OV5640_60_FPS;
> + goto find_mode;
> }
>
> fps = DIV_ROUND_CLOSEST(fi->denominator, fi->numerator);
> @@ -1865,11 +1875,15 @@ 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 is missing here leading to denominator untouched while numerator
has been reset to 1, leading to bug 2).
In fact, original code is valid for only 2 values, min and max, by
adding a third fps value, rework of the conditions are needed using a
"mid" value, see proposed code:
static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
struct v4l2_fract *fi,
u32 width, u32 height)
{
const struct ov5640_mode_info *mode;
- u32 minfps, maxfps, fps;
+ u32 minfps, midfps, maxfps, fps;
int ret;
minfps = ov5640_framerates[OV5640_15_FPS];
- maxfps = ov5640_framerates[OV5640_30_FPS];
+ midfps = 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;
+ ret = OV5640_60_FPS;
+ goto find_mode;
}
fps = DIV_ROUND_CLOSEST(fi->denominator, fi->numerator);
fi->numerator = 1;
- if (fps > maxfps)
+ if ((fps >= maxfps) ||
+ ((2 * fps >= (midfps + maxfps)) && (fps < maxfps)))
fi->denominator = maxfps;
- else if (fps < minfps)
+ else if ((fps <= minfps) ||
+ ((2 * fps <= (minfps + midfps)) && (fps > minfps)))
fi->denominator = minfps;
- else if (2 * fps >= 2 * minfps + (maxfps - minfps))
- fi->denominator = maxfps;
else
- fi->denominator = minfps;
+ fi->denominator = midfps;
- 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;
+find_mode:
mode = ov5640_find_mode(sensor, ret, width, height, false);
return mode ? ret : -EINVAL;
}
This code has been tested using v4l2-ctl:
v4l2-ctl --set-fmt-video=width=640,height=480,pixelformat=RGBP
v4l2-ctl --set-parm=14
v4l2-ctl --set-parm=15
v4l2-ctl --set-parm=16
v4l2-ctl --set-parm=22
v4l2-ctl --set-parm=23
v4l2-ctl --set-parm=24
v4l2-ctl --set-parm=29
v4l2-ctl --set-parm=30
v4l2-ctl --set-parm=31
v4l2-ctl --set-parm=36
v4l2-ctl --set-parm=42
v4l2-ctl --set-parm=45
v4l2-ctl --set-parm=46
v4l2-ctl --set-parm=59
v4l2-ctl --set-parm=60
v4l2-ctl --set-parm=61
root@stm32mp1:~# v4l2-ctl --set-parm=14
Frame rate set to 15.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=15
Frame rate set to 15.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=16
Frame rate set to 15.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=22
Frame rate set to 15.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=23
Frame rate set to 30.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=24
Frame rate set to 30.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=29
Frame rate set to 30.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=30
Frame rate set to 30.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=31
Frame rate set to 30.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=36
Frame rate set to 30.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=42
Frame rate set to 30.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=45
Frame rate set to 60.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=46
Frame rate set to 60.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=59
Frame rate set to 60.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=60
Frame rate set to 60.000 fps
root@stm32mp1:~# v4l2-ctl --set-parm=61
Frame rate set to 60.000 fps
> @@ -2458,8 +2472,11 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
>
> frame_rate = ov5640_try_frame_interval(sensor, &fi->interval,
> mode->hact, mode->vact);
> - if (frame_rate < 0)
> - frame_rate = OV5640_15_FPS;
> + if (frame_rate < 0) {
> + /* Always return a valid frame interval value */
> + fi->interval = sensor->frame_interval;
> + goto out;
> + }
>
> sensor->current_fr = frame_rate;
> sensor->frame_interval = fi->interval;
>
Best regards,
Hugues.
