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

[Maxime Ripard]

Hi Hugues,

On Thu, May 17, 2018 at 01:29:24PM +, 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

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


signature.asc
Description: PGP signature

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

[Hugues FRUCHET]

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 +, 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
> 

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

[Maxime Ripard]

Hi Hugues,

On Tue, May 15, 2018 at 01:33:55PM +, 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

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


signature.asc
Description: PGP signature

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

[Hugues FRUCHET]

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 
> ---
>   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;
>   }
>   }