| Message ID | 20180416123701.15901-12-maxime.ripard@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@bootlin.com>
> ---
> 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.
Hi Hugues, On Tue, May 15, 2018 at 01:33:55PM +0000, Hugues FRUCHET wrote: > 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. Thanks a lot for your feedback, I've (hopefully) fixed all the issues you reported here, most of the time taking your fix directly, except for 2 where I reworked the code instead. > 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) One small question though, I don't seem to have that output for v4l2-ctl, is some hook needed in the v4l2 device for it to work? Maxime
Hi Maxime, Thanks for fixes ! No special modification of v4l2-ctl, I'm using currently v4l-utils 1.12.3. What output do you have ? Best regards, Hugues. On 05/17/2018 10:52 AM, Maxime Ripard wrote: > Hi Hugues, > > On Tue, May 15, 2018 at 01:33:55PM +0000, Hugues FRUCHET wrote: >> 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. > > Thanks a lot for your feedback, I've (hopefully) fixed all the issues > you reported here, most of the time taking your fix directly, except > for 2 where I reworked the code instead. > >> 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) > > One small question though, I don't seem to have that output for > v4l2-ctl, is some hook needed in the v4l2 device for it to work? > > Maxime >
Hi Hugues, On Thu, May 17, 2018 at 01:29:24PM +0000, Hugues FRUCHET wrote: > No special modification of v4l2-ctl, I'm using currently v4l-utils 1.12.3. > What output do you have ? The same one, without the resolution and framerate. I'm pretty sure this is a driver issue and not an usperspace one. Maxime
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) + /* 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]; 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; + + if (fi->denominator == minfps) + ret = OV5640_15_FPS; + else if (fi->denominator == maxfps) + ret = OV5640_60_FPS; else - fi->denominator = minfps; - - ret = (fi->denominator == minfps) ? OV5640_15_FPS : OV5640_30_FPS; + ret = OV5640_30_FPS; +find_mode: mode = ov5640_find_mode(sensor, ret, width, height, false); return mode ? ret : -EINVAL; } @@ -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;
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@bootlin.com> --- drivers/media/i2c/ov5640.c | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-)