Re: [PATCH v3 0/5] Fix OV5640 exposure & gain

[jacopo mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 5081 bytes --]

Hi  Hugues,
   thanks for the patches

On Tue, Sep 11, 2018 at 03:48:16PM +0200, Hugues Fruchet wrote:
> This patch serie fixes some problems around exposure & gain in OV5640 driver.
>
> The 4th patch about autocontrols requires also a fix in v4l2-ctrls.c:
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg133164.html
>
> Here is the test procedure used for exposure & gain controls check:
> 1) Preview in background
> $> gst-launch-1.0 v4l2src ! "video/x-raw, width=640, Height=480" ! queue ! waylandsink -e &
> 2) Check gain & exposure values
> $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)"
>                        exposure (int)    : min=0 max=65535 step=1 default=0 value=330 flags=inactive, volatile
>                            gain (int)    : min=0 max=1023 step=1 default=0 value=19 flags=inactive, volatile
> 3) Put finger in front of camera and check that gain/exposure values are changing:
> $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)"
>                        exposure (int)    : min=0 max=65535 step=1 default=0 value=660 flags=inactive, volatile
>                            gain (int)    : min=0 max=1023 step=1 default=0 value=37 flags=inactive, volatile
> 4) switch to manual mode, image exposition must not change
> $> v4l2-ctl --set-ctrl=gain_automatic=0
> $> v4l2-ctl --set-ctrl=auto_exposure=1
> Note the "1" for manual exposure.
>
> 5) Check current gain/exposure values:
> $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)"
>                        exposure (int)    : min=0 max=65535 step=1 default=0 value=330
>                            gain (int)    : min=0 max=1023 step=1 default=0 value=20
>
> 6) Put finger behind camera and check that gain/exposure values are NOT changing:
> $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)"
>                        exposure (int)    : min=0 max=65535 step=1 default=0 value=330
>                            gain (int)    : min=0 max=1023 step=1 default=0 value=20
> 7) Update exposure, check that it is well changed on display and that same value is returned:
> $> v4l2-ctl --set-ctrl=exposure=100
> $> v4l2-ctl --get-ctrl=exposure
> exposure: 100
>
> 9) Update gain, check that it is well changed on display and that same value is returned:
> $> v4l2-ctl --set-ctrl=gain=10
> $> v4l2-ctl --get-ctrl=gain
> gain: 10
>
> 10) Switch back to auto gain/exposure, verify that image is correct and values returned are correct:
> $> v4l2-ctl --set-ctrl=gain_automatic=1
> $> v4l2-ctl --set-ctrl=auto_exposure=0
> $> v4l2-ctl --all | grep -e exposure -e gain | grep "(int)"
>                        exposure (int)    : min=0 max=65535 step=1 default=0 value=330 flags=inactive, volatile
>                            gain (int)    : min=0 max=1023 step=1 default=0 value=22 flags=inactive, volatile
> Note the "0" for auto exposure.
>

I've tested on my side and I can confirm the exposure and gain when in
auto-mode changes as expected, and it is possible to switch back and
forth between auto and manual modes.

The patches also fixes an issue when capturing frames, as the first
two/three frames where always black in my setup before this series.

(While streaming to gstreamers' fakesink)
# v4l2-ctl --get-ctrl "exposure" --get-ctrl "gain" -d /dev/video4
exposure: 885
gain: 50

(Point a light in front of the sensor)
# v4l2-ctl --get-ctrl "exposure" --get-ctrl "gain" -d /dev/video4
exposure: 17
gain: 19

(Disable auto-gain and auto-exposure)
# v4l2-ctl  -d /dev/video4 --set-ctrl=auto_exposure=1
# v4l2-ctl  -d /dev/video4 --set-ctrl=gain_automatic=0
# v4l2-ctl  -d /dev/video4 --set-ctrl=exposure=100
# v4l2-ctl --get-ctrl "exposure" --get-ctrl "gain" -d /dev/video4
exposure: 100
gain: 46

(Re-enable auto-exp and auto-gain)
# v4l2-ctl  -d /dev/video4 --set-ctrl=auto_exposure=0
# v4l2-ctl  -d /dev/video4 --set-ctrl=gain_automatic=1
# v4l2-ctl --get-ctrl "exposure" --get-ctrl "gain" -d /dev/video4
exposure: 885
gain: 46

(Finger on the sensor)
# v4l2-ctl --get-ctrl "exposure" --get-ctrl "gain" -d /dev/video4
exposure: 885
gain: 248

(Point a light on the sensor)
exposure: 16
gain: 19

So please add my
Tested-by: Jacopo Mondi <jacopo@jmondi.org>
to this series.

Thanks
   j

> ===========
> = history =
> ===========
> version 3:
>   - Change patch 5/5 by removing set_mode() orig_mode parameter as per jacopo' suggestion:
>     https://www.spinics.net/lists/linux-media/msg139457.html
>
> version 2:
>   - Fix patch 3/5 commit comment and rename binning function as per jacopo' suggestion:
>     https://www.mail-archive.com/linux-media@vger.kernel.org/msg133272.html
>
> Hugues Fruchet (5):
>   media: ov5640: fix exposure regression
>   media: ov5640: fix auto gain & exposure when changing mode
>   media: ov5640: fix wrong binning value in exposure calculation
>   media: ov5640: fix auto controls values when switching to manual mode
>   media: ov5640: fix restore of last mode set
>
>  drivers/media/i2c/ov5640.c | 128 ++++++++++++++++++++++++++-------------------
>  1 file changed, 73 insertions(+), 55 deletions(-)
>
> --
> 2.7.4
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]