[PATCH 0/5] Fix OV5640 exposure & gain

[PATCH 0/5] Fix OV5640 exposure & gain

[Hugues Fruchet]

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.



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 | 120 ++++++++++++++++++++++++++-------------------
 1 file changed, 70 insertions(+), 50 deletions(-)

-- 
1.9.1

[PATCH 1/5] media: ov5640: fix exposure regression

[Hugues Fruchet]

fixes: bf4a4b518c20 ("media: ov5640: Don't force the auto exposure state at start time").

Symptom was black image when capturing HD or 5Mp picture
due to manual exposure set to 1 while it was intended to
set autoexposure to "manual", fix this.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/i2c/ov5640.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 1ecbb7a..4b9da8b 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -938,6 +938,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
 	return ret;
 }
 
+static int ov5640_set_autoexposure(struct ov5640_dev *sensor, bool on)
+{
+	return ov5640_mod_reg(sensor, OV5640_REG_AEC_PK_MANUAL,
+			      BIT(0), on ? 0 : BIT(0));
+}
+
 /* read exposure, in number of line periods */
 static int ov5640_get_exposure(struct ov5640_dev *sensor)
 {
@@ -1593,7 +1599,7 @@ static int ov5640_set_mode_exposure_calc(struct ov5640_dev *sensor,
  */
 static int ov5640_set_mode_direct(struct ov5640_dev *sensor,
 				  const struct ov5640_mode_info *mode,
-				  s32 exposure)
+				  bool auto_exp)
 {
 	int ret;
 
@@ -1610,7 +1616,8 @@ static int ov5640_set_mode_direct(struct ov5640_dev *sensor,
 	if (ret)
 		return ret;
 
-	return __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, exposure);
+	return __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, auto_exp ?
+				  V4L2_EXPOSURE_AUTO : V4L2_EXPOSURE_MANUAL);
 }
 
 static int ov5640_set_mode(struct ov5640_dev *sensor,
@@ -1618,7 +1625,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 {
 	const struct ov5640_mode_info *mode = sensor->current_mode;
 	enum ov5640_downsize_mode dn_mode, orig_dn_mode;
-	s32 exposure;
+	bool auto_exp =  sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO;
 	int ret;
 
 	dn_mode = mode->dn_mode;
@@ -1629,8 +1636,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 	if (ret)
 		return ret;
 
-	exposure = sensor->ctrls.auto_exp->val;
-	ret = ov5640_set_exposure(sensor, V4L2_EXPOSURE_MANUAL);
+	ret = ov5640_set_autoexposure(sensor, false);
 	if (ret)
 		return ret;
 
@@ -1646,7 +1652,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 		 * change inside subsampling or scaling
 		 * download firmware directly
 		 */
-		ret = ov5640_set_mode_direct(sensor, mode, exposure);
+		ret = ov5640_set_mode_direct(sensor, mode, auto_exp);
 	}
 
 	if (ret < 0)
-- 
1.9.1

[PATCH 2/5] media: ov5640: fix auto gain & exposure when changing mode

[Hugues Fruchet]

Ensure that auto gain and auto exposure are well restored
when changing mode.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/i2c/ov5640.c | 96 ++++++++++++++++++++++++++--------------------
 1 file changed, 54 insertions(+), 42 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 4b9da8b..7c569de 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1000,6 +1000,18 @@ static int ov5640_get_gain(struct ov5640_dev *sensor)
 	return gain & 0x3ff;
 }
 
+static int ov5640_set_gain(struct ov5640_dev *sensor, int gain)
+{
+	return ov5640_write_reg16(sensor, OV5640_REG_AEC_PK_REAL_GAIN,
+				  (u16)gain & 0x3ff);
+}
+
+static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
+{
+	return ov5640_mod_reg(sensor, OV5640_REG_AEC_PK_MANUAL,
+			      BIT(1), on ? 0 : BIT(1));
+}
+
 static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
 {
 	int ret;
@@ -1577,7 +1589,7 @@ static int ov5640_set_mode_exposure_calc(struct ov5640_dev *sensor,
 	}
 
 	/* set capture gain */
-	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.gain, cap_gain16);
+	ret = ov5640_set_gain(sensor, cap_gain16);
 	if (ret)
 		return ret;
 
@@ -1590,7 +1602,7 @@ static int ov5640_set_mode_exposure_calc(struct ov5640_dev *sensor,
 	}
 
 	/* set exposure */
-	return __v4l2_ctrl_s_ctrl(sensor->ctrls.exposure, cap_shutter);
+	return ov5640_set_exposure(sensor, cap_shutter);
 }
 
 /*
@@ -1598,26 +1610,13 @@ static int ov5640_set_mode_exposure_calc(struct ov5640_dev *sensor,
  * change mode directly
  */
 static int ov5640_set_mode_direct(struct ov5640_dev *sensor,
-				  const struct ov5640_mode_info *mode,
-				  bool auto_exp)
+				  const struct ov5640_mode_info *mode)
 {
-	int ret;
-
 	if (!mode->reg_data)
 		return -EINVAL;
 
 	/* Write capture setting */
-	ret = ov5640_load_regs(sensor, mode);
-	if (ret < 0)
-		return ret;
-
-	/* turn auto gain/exposure back on for direct mode */
-	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
-	if (ret)
-		return ret;
-
-	return __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, auto_exp ?
-				  V4L2_EXPOSURE_AUTO : V4L2_EXPOSURE_MANUAL);
+	return ov5640_load_regs(sensor, mode);
 }
 
 static int ov5640_set_mode(struct ov5640_dev *sensor,
@@ -1625,6 +1624,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 {
 	const struct ov5640_mode_info *mode = sensor->current_mode;
 	enum ov5640_downsize_mode dn_mode, orig_dn_mode;
+	bool auto_gain = sensor->ctrls.auto_gain->val == 1;
 	bool auto_exp =  sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO;
 	int ret;
 
@@ -1632,19 +1632,23 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 	orig_dn_mode = orig_mode->dn_mode;
 
 	/* auto gain and exposure must be turned off when changing modes */
-	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 0);
-	if (ret)
-		return ret;
+	if (auto_gain) {
+		ret = ov5640_set_autogain(sensor, false);
+		if (ret)
+			return ret;
+	}
 
-	ret = ov5640_set_autoexposure(sensor, false);
-	if (ret)
-		return ret;
+	if (auto_exp) {
+		ret = ov5640_set_autoexposure(sensor, false);
+		if (ret)
+			goto restore_auto_gain;
+	}
 
 	if ((dn_mode == SUBSAMPLING && orig_dn_mode == SCALING) ||
 	    (dn_mode == SCALING && orig_dn_mode == SUBSAMPLING)) {
 		/*
 		 * change between subsampling and scaling
-		 * go through exposure calucation
+		 * go through exposure calculation
 		 */
 		ret = ov5640_set_mode_exposure_calc(sensor, mode);
 	} else {
@@ -1652,11 +1656,16 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 		 * change inside subsampling or scaling
 		 * download firmware directly
 		 */
-		ret = ov5640_set_mode_direct(sensor, mode, auto_exp);
+		ret = ov5640_set_mode_direct(sensor, mode);
 	}
-
 	if (ret < 0)
-		return ret;
+		goto restore_auto_exp_gain;
+
+	/* restore auto gain and exposure */
+	if (auto_gain)
+		ov5640_set_autogain(sensor, true);
+	if (auto_exp)
+		ov5640_set_autoexposure(sensor, true);
 
 	ret = ov5640_set_timings(sensor, mode);
 	if (ret < 0)
@@ -1681,6 +1690,15 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 	sensor->pending_mode_change = false;
 
 	return 0;
+
+restore_auto_exp_gain:
+	if (auto_exp)
+		ov5640_set_autoexposure(sensor, true);
+restore_auto_gain:
+	if (auto_gain)
+		ov5640_set_autogain(sensor, true);
+
+	return ret;
 }
 
 static int ov5640_set_framefmt(struct ov5640_dev *sensor,
@@ -2141,20 +2159,20 @@ static int ov5640_set_ctrl_white_balance(struct ov5640_dev *sensor, int awb)
 	return ret;
 }
 
-static int ov5640_set_ctrl_exposure(struct ov5640_dev *sensor, int exp)
+static int ov5640_set_ctrl_exposure(struct ov5640_dev *sensor,
+				    enum v4l2_exposure_auto_type auto_exposure)
 {
 	struct ov5640_ctrls *ctrls = &sensor->ctrls;
-	bool auto_exposure = (exp == V4L2_EXPOSURE_AUTO);
+	bool auto_exp = (auto_exposure == V4L2_EXPOSURE_AUTO);
 	int ret = 0;
 
 	if (ctrls->auto_exp->is_new) {
-		ret = ov5640_mod_reg(sensor, OV5640_REG_AEC_PK_MANUAL,
-				     BIT(0), auto_exposure ? 0 : BIT(0));
+		ret = ov5640_set_autoexposure(sensor, auto_exp);
 		if (ret)
 			return ret;
 	}
 
-	if (!auto_exposure && ctrls->exposure->is_new) {
+	if (!auto_exp && ctrls->exposure->is_new) {
 		u16 max_exp;
 
 		ret = ov5640_read_reg16(sensor, OV5640_REG_AEC_PK_VTS,
@@ -2174,25 +2192,19 @@ static int ov5640_set_ctrl_exposure(struct ov5640_dev *sensor, int exp)
 	return ret;
 }
 
-static int ov5640_set_ctrl_gain(struct ov5640_dev *sensor, int auto_gain)
+static int ov5640_set_ctrl_gain(struct ov5640_dev *sensor, bool auto_gain)
 {
 	struct ov5640_ctrls *ctrls = &sensor->ctrls;
 	int ret = 0;
 
 	if (ctrls->auto_gain->is_new) {
-		ret = ov5640_mod_reg(sensor, OV5640_REG_AEC_PK_MANUAL,
-				     BIT(1),
-				     ctrls->auto_gain->val ? 0 : BIT(1));
+		ret = ov5640_set_autogain(sensor, auto_gain);
 		if (ret)
 			return ret;
 	}
 
-	if (!auto_gain && ctrls->gain->is_new) {
-		u16 gain = (u16)ctrls->gain->val;
-
-		ret = ov5640_write_reg16(sensor, OV5640_REG_AEC_PK_REAL_GAIN,
-					 gain & 0x3ff);
-	}
+	if (!auto_gain && ctrls->gain->is_new)
+		ret = ov5640_set_gain(sensor, ctrls->gain->val);
 
 	return ret;
 }
-- 
1.9.1

[PATCH 3/5] media: ov5640: fix wrong binning value in exposure calculation

[Hugues Fruchet]

ov5640_set_mode_exposure_calc() is checking binning value but
binning value read is buggy and binning value set is done
after calling ov5640_set_mode_exposure_calc(), fix all of this.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/i2c/ov5640.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 7c569de..f9b256e 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1357,8 +1357,8 @@ static int ov5640_binning_on(struct ov5640_dev *sensor)
 	ret = ov5640_read_reg(sensor, OV5640_REG_TIMING_TC_REG21, &temp);
 	if (ret)
 		return ret;
-	temp &= 0xfe;
-	return temp ? 1 : 0;
+
+	return temp & BIT(0);
 }
 
 static int ov5640_set_binning(struct ov5640_dev *sensor, bool enable)
-- 
1.9.1

[PATCH 4/5] media: ov5640: fix auto controls values when switching to manual mode

[Hugues Fruchet]

When switching from auto to manual mode, V4L2 core is calling
g_volatile_ctrl() in manual mode in order to get the manual initial value.
Remove the manual mode check/return to not break this behaviour.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/i2c/ov5640.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index f9b256e..a307f1e 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -2277,16 +2277,12 @@ static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 
 	switch (ctrl->id) {
 	case V4L2_CID_AUTOGAIN:
-		if (!ctrl->val)
-			return 0;
 		val = ov5640_get_gain(sensor);
 		if (val < 0)
 			return val;
 		sensor->ctrls.gain->val = val;
 		break;
 	case V4L2_CID_EXPOSURE_AUTO:
-		if (ctrl->val == V4L2_EXPOSURE_MANUAL)
-			return 0;
 		val = ov5640_get_exposure(sensor);
 		if (val < 0)
 			return val;
-- 
1.9.1

[PATCH 5/5] media: ov5640: fix restore of last mode set

[Hugues Fruchet]

Mode setting depends on last mode set, in particular
because of exposure calculation when downscale mode
change between subsampling and scaling.
At stream on the last mode was wrongly set to current mode,
so no change was detected and exposure calculation
was not made, fix this.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/i2c/ov5640.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index a307f1e..f3fb140 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -225,6 +225,7 @@ struct ov5640_dev {
 	struct v4l2_mbus_framefmt fmt;
 
 	const struct ov5640_mode_info *current_mode;
+	const struct ov5640_mode_info *last_mode;
 	enum ov5640_frame_rate current_fr;
 	struct v4l2_fract frame_interval;
 
@@ -1628,6 +1629,9 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 	bool auto_exp =  sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO;
 	int ret;
 
+	if (!orig_mode)
+		orig_mode = mode;
+
 	dn_mode = mode->dn_mode;
 	orig_dn_mode = orig_mode->dn_mode;
 
@@ -1688,6 +1692,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 		return ret;
 
 	sensor->pending_mode_change = false;
+	sensor->last_mode = mode;
 
 	return 0;
 
@@ -2551,7 +2556,8 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
 
 	if (sensor->streaming == !enable) {
 		if (enable && sensor->pending_mode_change) {
-			ret = ov5640_set_mode(sensor, sensor->current_mode);
+			ret = ov5640_set_mode(sensor, sensor->last_mode);
+
 			if (ret)
 				goto out;
 
-- 
1.9.1

Re: [PATCH 3/5] media: ov5640: fix wrong binning value in exposure calculation

[jacopo mondi]

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

Hi Hugues,

On Wed, Jul 04, 2018 at 02:58:41PM +0200, Hugues Fruchet wrote:
> ov5640_set_mode_exposure_calc() is checking binning value but
> binning value read is buggy and binning value set is done
> after calling ov5640_set_mode_exposure_calc(), fix all of this.

The ov5640_binning_on() function was indeed wrong (side note: that
name is confusing, it should be 0v5640_get_binning() to comply with
others..) and always returned 0, but I don't see a fix here for the
second part of the issue. In facts, during the lenghty exposure
calculation process, binning is checked to decide if the preview
shutter time should be doubled or not

static int ov5640_set_mode_exposure_calc(struct ov5640_dev *sensor,
					 const struct ov5640_mode_info *mode)
{
        ...

	/* read preview shutter */
	ret = ov5640_get_exposure(sensor);
	if (ret < 0)
		return ret;
	prev_shutter = ret;
	ret = ov5640_binning_on(sensor);
	if (ret < 0)
		return ret;
	if (ret && mode->id != OV5640_MODE_720P_1280_720 &&
	    mode->id != OV5640_MODE_1080P_1920_1080)
		prev_shutter *= 2;
        ...
}

My understanding is that reading the value from the register returns
the binning settings for the previously configured mode, while the
binning value is later updated for the current mode in
ov5640_set_mode(), after 'ov5640_set_mode_exposure_calc()' has already
been called. Is this ok?

Also, I assume the code checks for mode->id to figure out if the mode
uses subsampling or scaling. Be aware that for 1280x720 mode, the
selected scaling mode depends on the FPS, not only on the mode id as
it is assumed here.

A final note, the 'ov5640_set_mode_exposure_calc()' also writes VTS to
update the shutter time to the newly calculated value.

	/* write capture shutter */
	if (cap_shutter > (cap_vts - 4)) {
		cap_vts = cap_shutter + 4;
		ret = ov5640_set_vts(sensor, cap_vts);
		if (ret < 0)
			return ret;
	}

Be aware again that VTS is later restored to the mode->vtot value by
the 'ov5640_set_timings()' functions, which again, is called later
than 'ov5640_set_mode_exposure_calc()'.

Wouldn't it be better to postpone exposure calculation after timings
and binnings have been set?

Thanks
   j

>
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
>  drivers/media/i2c/ov5640.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 7c569de..f9b256e 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -1357,8 +1357,8 @@ static int ov5640_binning_on(struct ov5640_dev *sensor)
>  	ret = ov5640_read_reg(sensor, OV5640_REG_TIMING_TC_REG21, &temp);
>  	if (ret)
>  		return ret;
> -	temp &= 0xfe;
> -	return temp ? 1 : 0;
> +
> +	return temp & BIT(0);
>  }
>
>  static int ov5640_set_binning(struct ov5640_dev *sensor, bool enable)
> --
> 1.9.1
>

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

Re: [PATCH 3/5] media: ov5640: fix wrong binning value in exposure calculation

[Hugues FRUCHET]

Hi Jacopo,

Many thanks for you valuable comments, I hardly understand this exposure 
code, and still some wrongly exposed images are observed switching from 
subsampling to scaling modes.
Steve, do you have more insight to share with us on this code ?

On 07/04/2018 04:38 PM, jacopo mondi wrote:
> Hi Hugues,
> 
> On Wed, Jul 04, 2018 at 02:58:41PM +0200, Hugues Fruchet wrote:
>> ov5640_set_mode_exposure_calc() is checking binning value but
>> binning value read is buggy and binning value set is done
>> after calling ov5640_set_mode_exposure_calc(), fix all of this.
> 
> The ov5640_binning_on() function was indeed wrong (side note: that
> name is confusing, it should be 0v5640_get_binning() to comply with
> others..) and always returned 0, but I don't see a fix here for the
> second part of the issue.
Mistake from me here, I should have removed "and binning value set is 
done after calling ov5640_set_mode_exposure_calc()" in commit message.

> In facts, during the lenghty exposure
> calculation process, binning is checked to decide if the preview
> shutter time should be doubled or not
> 
> static int ov5640_set_mode_exposure_calc(struct ov5640_dev *sensor,
> 					 const struct ov5640_mode_info *mode)
> {
>          ...
> 
> 	/* read preview shutter */
> 	ret = ov5640_get_exposure(sensor);
> 	if (ret < 0)
> 		return ret;
> 	prev_shutter = ret;
> 	ret = ov5640_binning_on(sensor);
> 	if (ret < 0)
> 		return ret;
> 	if (ret && mode->id != OV5640_MODE_720P_1280_720 &&
> 	    mode->id != OV5640_MODE_1080P_1920_1080)
> 		prev_shutter *= 2;
>          ...
> }
> 
> My understanding is that reading the value from the register returns
> the binning settings for the previously configured mode, while the > binning value is later updated for the current mode in
> ov5640_set_mode(), after 'ov5640_set_mode_exposure_calc()' has already
> been called. Is this ok?

This is also my understanding.

> 
> Also, I assume the code checks for mode->id to figure out if the mode
> uses subsampling or scaling. Be aware that for 1280x720 mode, the
> selected scaling mode depends on the FPS, not only on the mode id as
> it is assumed here.

This is not what I understand from this array:
static const struct ov5640_mode_info
ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
[15fps]
		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
		 1280, 1892, 720, 740,
		 ov5640_setting_15fps_720P_1280_720,
		 ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
[30fps]
		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
		 1280, 1892, 720, 740,
		 ov5640_setting_30fps_720P_1280_720,
		 ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)},

=> both modes uses subsampling here

> 
> A final note, the 'ov5640_set_mode_exposure_calc()' also writes VTS to
> update the shutter time to the newly calculated value.
> 
> 	/* write capture shutter */
> 	if (cap_shutter > (cap_vts - 4)) {
> 		cap_vts = cap_shutter + 4;
> 		ret = ov5640_set_vts(sensor, cap_vts);
> 		if (ret < 0)
> 			return ret;
> 	}
> 
> Be aware again that VTS is later restored to the mode->vtot value by
> the 'ov5640_set_timings()' functions, which again, is called later
> than 'ov5640_set_mode_exposure_calc()'.
> 
> Wouldn't it be better to postpone exposure calculation after timings
> and binnings have been set ?

As said, I'm new on all of this but I can give it a try.

> 
> Thanks
>     j
> 
>>
>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>> ---
>>   drivers/media/i2c/ov5640.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>> index 7c569de..f9b256e 100644
>> --- a/drivers/media/i2c/ov5640.c
>> +++ b/drivers/media/i2c/ov5640.c
>> @@ -1357,8 +1357,8 @@ static int ov5640_binning_on(struct ov5640_dev *sensor)
>>   	ret = ov5640_read_reg(sensor, OV5640_REG_TIMING_TC_REG21, &temp);
>>   	if (ret)
>>   		return ret;
>> -	temp &= 0xfe;
>> -	return temp ? 1 : 0;
>> +
>> +	return temp & BIT(0);
>>   }
>>
>>   static int ov5640_set_binning(struct ov5640_dev *sensor, bool enable)
>> --
>> 1.9.1
>>

Best regards,
Hugues.

Re: [PATCH 3/5] media: ov5640: fix wrong binning value in exposure calculation

[jacopo mondi]

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

Hi Hugues,

On Wed, Jul 04, 2018 at 03:29:56PM +0000, Hugues FRUCHET wrote:
> Hi Jacopo,
>
> Many thanks for you valuable comments, I hardly understand this exposure
> code, and still some wrongly exposed images are observed switching from
> subsampling to scaling modes.

Thank you for the patches...

Just out of curiosity, have you ever been able to get images in 1280x720
and 1920x1080 mode? I assume so if you're able to switch between two
subsampling modes...

> Steve, do you have more insight to share with us on this code ?
>
> On 07/04/2018 04:38 PM, jacopo mondi wrote:
> > Hi Hugues,
> >
> > On Wed, Jul 04, 2018 at 02:58:41PM +0200, Hugues Fruchet wrote:
> >> ov5640_set_mode_exposure_calc() is checking binning value but
> >> binning value read is buggy and binning value set is done
> >> after calling ov5640_set_mode_exposure_calc(), fix all of this.
> >
> > The ov5640_binning_on() function was indeed wrong (side note: that
> > name is confusing, it should be 0v5640_get_binning() to comply with
> > others..) and always returned 0, but I don't see a fix here for the
> > second part of the issue.
> Mistake from me here, I should have removed "and binning value set is
> done after calling ov5640_set_mode_exposure_calc()" in commit message.
>
> > In facts, during the lenghty exposure
> > calculation process, binning is checked to decide if the preview
> > shutter time should be doubled or not
> >
> > static int ov5640_set_mode_exposure_calc(struct ov5640_dev *sensor,
> > 					 const struct ov5640_mode_info *mode)
> > {
> >          ...
> >
> > 	/* read preview shutter */
> > 	ret = ov5640_get_exposure(sensor);
> > 	if (ret < 0)
> > 		return ret;
> > 	prev_shutter = ret;
> > 	ret = ov5640_binning_on(sensor);
> > 	if (ret < 0)
> > 		return ret;
> > 	if (ret && mode->id != OV5640_MODE_720P_1280_720 &&
> > 	    mode->id != OV5640_MODE_1080P_1920_1080)
> > 		prev_shutter *= 2;
> >          ...
> > }
> >
> > My understanding is that reading the value from the register returns
> > the binning settings for the previously configured mode, while the > binning value is later updated for the current mode in
> > ov5640_set_mode(), after 'ov5640_set_mode_exposure_calc()' has already
> > been called. Is this ok?
>
> This is also my understanding.
>

Thanks. This is probably worth fixing. Maybe your exposure issues
depend on this..

> >
> > Also, I assume the code checks for mode->id to figure out if the mode
> > uses subsampling or scaling. Be aware that for 1280x720 mode, the
> > selected scaling mode depends on the FPS, not only on the mode id as
> > it is assumed here.
>
> This is not what I understand from this array:
> static const struct ov5640_mode_info
> ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
> [15fps]
> 		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
> 		 1280, 1892, 720, 740,
> 		 ov5640_setting_15fps_720P_1280_720,
> 		 ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
> [30fps]
> 		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
> 		 1280, 1892, 720, 740,
> 		 ov5640_setting_30fps_720P_1280_720,
> 		 ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)},
>
> => both modes uses subsampling here

You are right, I counted the array entries and 30FPS has a -1
specified as downsizing mode in the last one, so I overlooked it, sorry!

So what is mode->id checked for, if 720p and 1080p modes use different
downsizing modes? Confused ....

>
> >
> > A final note, the 'ov5640_set_mode_exposure_calc()' also writes VTS to
> > update the shutter time to the newly calculated value.
> >
> > 	/* write capture shutter */
> > 	if (cap_shutter > (cap_vts - 4)) {
> > 		cap_vts = cap_shutter + 4;
> > 		ret = ov5640_set_vts(sensor, cap_vts);
> > 		if (ret < 0)
> > 			return ret;
> > 	}
> >
> > Be aware again that VTS is later restored to the mode->vtot value by
> > the 'ov5640_set_timings()' functions, which again, is called later
> > than 'ov5640_set_mode_exposure_calc()'.
> >
> > Wouldn't it be better to postpone exposure calculation after timings
> > and binnings have been set ?
>
> As said, I'm new on all of this but I can give it a try.

Thanks, I also see banding filter being calculated twice, and I'm sure
there are some other things I'm missing. That exposure calculation
procedure seems poorly integrated with the rest of the set_mode()
function :/

Thanks
   j
>
> >
> > Thanks
> >     j
> >
> >>
> >> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> >> ---
> >>   drivers/media/i2c/ov5640.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> >> index 7c569de..f9b256e 100644
> >> --- a/drivers/media/i2c/ov5640.c
> >> +++ b/drivers/media/i2c/ov5640.c
> >> @@ -1357,8 +1357,8 @@ static int ov5640_binning_on(struct ov5640_dev *sensor)
> >>   	ret = ov5640_read_reg(sensor, OV5640_REG_TIMING_TC_REG21, &temp);
> >>   	if (ret)
> >>   		return ret;
> >> -	temp &= 0xfe;
> >> -	return temp ? 1 : 0;
> >> +
> >> +	return temp & BIT(0);
> >>   }
> >>
> >>   static int ov5640_set_binning(struct ov5640_dev *sensor, bool enable)
> >> --
> >> 1.9.1
> >>
>
> Best regards,
> Hugues.

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