public inbox for linux-media@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/5] Fix OV5640 exposure & gain
@ 2018-09-11 13:48 Hugues Fruchet
  2018-09-11 13:48 ` [PATCH v3 1/5] media: ov5640: fix exposure regression Hugues Fruchet
                   ` (8 more replies)
  0 siblings, 9 replies; 14+ messages in thread
From: Hugues Fruchet @ 2018-09-11 13:48 UTC (permalink / raw)
  To: Steve Longerbeam, Sakari Ailus, Jacopo Mondi, Hans Verkuil,
	Mauro Carvalho Chehab
  Cc: linux-media, Hugues Fruchet, Benjamin Gaignard

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.

===========
= 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3 1/5] media: ov5640: fix exposure regression
  2018-09-11 13:48 [PATCH v3 0/5] Fix OV5640 exposure & gain Hugues Fruchet
@ 2018-09-11 13:48 ` Hugues Fruchet
  2018-09-11 13:48 ` [PATCH v3 2/5] media: ov5640: fix auto gain & exposure when changing mode Hugues Fruchet
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 14+ messages in thread
From: Hugues Fruchet @ 2018-09-11 13:48 UTC (permalink / raw)
  To: Steve Longerbeam, Sakari Ailus, Jacopo Mondi, Hans Verkuil,
	Mauro Carvalho Chehab
  Cc: linux-media, Hugues Fruchet, Benjamin Gaignard

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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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)
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 2/5] media: ov5640: fix auto gain & exposure when changing mode
  2018-09-11 13:48 [PATCH v3 0/5] Fix OV5640 exposure & gain Hugues Fruchet
  2018-09-11 13:48 ` [PATCH v3 1/5] media: ov5640: fix exposure regression Hugues Fruchet
@ 2018-09-11 13:48 ` Hugues Fruchet
  2018-09-11 13:48 ` [PATCH v3 3/5] media: ov5640: fix wrong binning value in exposure calculation Hugues Fruchet
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 14+ messages in thread
From: Hugues Fruchet @ 2018-09-11 13:48 UTC (permalink / raw)
  To: Steve Longerbeam, Sakari Ailus, Jacopo Mondi, Hans Verkuil,
	Mauro Carvalho Chehab
  Cc: linux-media, Hugues Fruchet, Benjamin Gaignard

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 3/5] media: ov5640: fix wrong binning value in exposure calculation
  2018-09-11 13:48 [PATCH v3 0/5] Fix OV5640 exposure & gain Hugues Fruchet
  2018-09-11 13:48 ` [PATCH v3 1/5] media: ov5640: fix exposure regression Hugues Fruchet
  2018-09-11 13:48 ` [PATCH v3 2/5] media: ov5640: fix auto gain & exposure when changing mode Hugues Fruchet
@ 2018-09-11 13:48 ` Hugues Fruchet
  2018-09-11 13:48 ` [PATCH v3 4/5] media: ov5640: fix auto controls values when switching to manual mode Hugues Fruchet
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 14+ messages in thread
From: Hugues Fruchet @ 2018-09-11 13:48 UTC (permalink / raw)
  To: Steve Longerbeam, Sakari Ailus, Jacopo Mondi, Hans Verkuil,
	Mauro Carvalho Chehab
  Cc: linux-media, Hugues Fruchet, Benjamin Gaignard

ov5640_set_mode_exposure_calc() is checking binning value but
binning value read is buggy, fix this.
Rename ov5640_binning_on() to ov5640_get_binning() as per other
similar functions.

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

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 7c569de..9fb17b5 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1349,7 +1349,7 @@ static int ov5640_set_ae_target(struct ov5640_dev *sensor, int target)
 	return ov5640_write_reg(sensor, OV5640_REG_AEC_CTRL1F, fast_low);
 }
 
-static int ov5640_binning_on(struct ov5640_dev *sensor)
+static int ov5640_get_binning(struct ov5640_dev *sensor)
 {
 	u8 temp;
 	int ret;
@@ -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)
@@ -1468,7 +1468,7 @@ static int ov5640_set_mode_exposure_calc(struct ov5640_dev *sensor,
 	if (ret < 0)
 		return ret;
 	prev_shutter = ret;
-	ret = ov5640_binning_on(sensor);
+	ret = ov5640_get_binning(sensor);
 	if (ret < 0)
 		return ret;
 	if (ret && mode->id != OV5640_MODE_720P_1280_720 &&
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 4/5] media: ov5640: fix auto controls values when switching to manual mode
  2018-09-11 13:48 [PATCH v3 0/5] Fix OV5640 exposure & gain Hugues Fruchet
                   ` (2 preceding siblings ...)
  2018-09-11 13:48 ` [PATCH v3 3/5] media: ov5640: fix wrong binning value in exposure calculation Hugues Fruchet
@ 2018-09-11 13:48 ` Hugues Fruchet
  2018-09-11 13:48 ` [PATCH v3 5/5] media: ov5640: fix restore of last mode set Hugues Fruchet
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 14+ messages in thread
From: Hugues Fruchet @ 2018-09-11 13:48 UTC (permalink / raw)
  To: Steve Longerbeam, Sakari Ailus, Jacopo Mondi, Hans Verkuil,
	Mauro Carvalho Chehab
  Cc: linux-media, Hugues Fruchet, Benjamin Gaignard

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 9fb17b5..c110a6a 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;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 5/5] media: ov5640: fix restore of last mode set
  2018-09-11 13:48 [PATCH v3 0/5] Fix OV5640 exposure & gain Hugues Fruchet
                   ` (3 preceding siblings ...)
  2018-09-11 13:48 ` [PATCH v3 4/5] media: ov5640: fix auto controls values when switching to manual mode Hugues Fruchet
@ 2018-09-11 13:48 ` Hugues Fruchet
  2018-09-14 16:00 ` [PATCH v3 0/5] Fix OV5640 exposure & gain jacopo mondi
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 14+ messages in thread
From: Hugues Fruchet @ 2018-09-11 13:48 UTC (permalink / raw)
  To: Steve Longerbeam, Sakari Ailus, Jacopo Mondi, Hans Verkuil,
	Mauro Carvalho Chehab
  Cc: linux-media, Hugues Fruchet, Benjamin Gaignard

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 | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index c110a6a..427c2e7 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;
 
@@ -1619,10 +1620,10 @@ static int ov5640_set_mode_direct(struct ov5640_dev *sensor,
 	return ov5640_load_regs(sensor, mode);
 }
 
-static int ov5640_set_mode(struct ov5640_dev *sensor,
-			   const struct ov5640_mode_info *orig_mode)
+static int ov5640_set_mode(struct ov5640_dev *sensor)
 {
 	const struct ov5640_mode_info *mode = sensor->current_mode;
+	const struct ov5640_mode_info *orig_mode = sensor->last_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;
@@ -1688,6 +1689,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 		return ret;
 
 	sensor->pending_mode_change = false;
+	sensor->last_mode = mode;
 
 	return 0;
 
@@ -1713,6 +1715,7 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor)
 	ret = ov5640_load_regs(sensor, &ov5640_mode_init_data);
 	if (ret < 0)
 		return ret;
+	sensor->last_mode = &ov5640_mode_init_data;
 
 	ret = ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x3f,
 			     (ilog2(OV5640_SCLK2X_ROOT_DIVIDER_DEFAULT) << 2) |
@@ -1721,7 +1724,7 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor)
 		return ret;
 
 	/* now restore the last capture mode */
-	ret = ov5640_set_mode(sensor, &ov5640_mode_init_data);
+	ret = ov5640_set_mode(sensor);
 	if (ret < 0)
 		return ret;
 
@@ -2551,7 +2554,7 @@ 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);
 			if (ret)
 				goto out;
 
@@ -2667,6 +2670,7 @@ static int ov5640_probe(struct i2c_client *client,
 	sensor->current_mode =
 		&ov5640_mode_data[OV5640_30_FPS][OV5640_MODE_VGA_640_480];
 	sensor->pending_mode_change = true;
+	sensor->last_mode = sensor->current_mode;
 
 	sensor->ae_target = 52;
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 0/5] Fix OV5640 exposure & gain
  2018-09-11 13:48 [PATCH v3 0/5] Fix OV5640 exposure & gain Hugues Fruchet
                   ` (4 preceding siblings ...)
  2018-09-11 13:48 ` [PATCH v3 5/5] media: ov5640: fix restore of last mode set Hugues Fruchet
@ 2018-09-14 16:00 ` jacopo mondi
  2018-09-14 16:07 ` jacopo mondi
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 14+ messages in thread
From: jacopo mondi @ 2018-09-14 16:00 UTC (permalink / raw)
  To: Hugues Fruchet
  Cc: Steve Longerbeam, Sakari Ailus, Hans Verkuil,
	Mauro Carvalho Chehab, linux-media, Benjamin Gaignard

[-- 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 --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 0/5] Fix OV5640 exposure & gain
  2018-09-11 13:48 [PATCH v3 0/5] Fix OV5640 exposure & gain Hugues Fruchet
                   ` (5 preceding siblings ...)
  2018-09-14 16:00 ` [PATCH v3 0/5] Fix OV5640 exposure & gain jacopo mondi
@ 2018-09-14 16:07 ` jacopo mondi
  2018-09-15 23:02   ` Sakari Ailus
  2018-09-14 17:49 ` Steve Longerbeam
  2018-09-14 18:25 ` Nicolas Dufresne
  8 siblings, 1 reply; 14+ messages in thread
From: jacopo mondi @ 2018-09-14 16:07 UTC (permalink / raw)
  To: Sakari Ailus, hugues.fruchet
  Cc: Steve Longerbeam, Hans Verkuil, Mauro Carvalho Chehab,
	linux-media, Benjamin Gaignard

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

Hi Sakari,

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.

As you offered to collect this series and my CSI-2 fixes I have just
re-sent, you might be interested in this branch:

git://jmondi.org/linux
engicam-imx6q/media-master/ov5640/csi2_init_v4_exposure_v3

I have there re-based this series on top of mine, which is in turn
based on latest media master, where this series do not apply as-is
afaict.

I have added to Hugues' patches my reviewed-by and tested-by tags.
If you prefer to I can send you a pull request, or if you want to have
a chance to review the whole patch list please refer to the above
branch.

Let me know if I can help speeding up the inclusion of these two
series as they fix two real issues of MIPI CSI-2 capture operations
for this sensor.

Thanks
  j

>
> 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.
>
> ===========
> = 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 --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 0/5] Fix OV5640 exposure & gain
  2018-09-11 13:48 [PATCH v3 0/5] Fix OV5640 exposure & gain Hugues Fruchet
                   ` (6 preceding siblings ...)
  2018-09-14 16:07 ` jacopo mondi
@ 2018-09-14 17:49 ` Steve Longerbeam
  2018-09-14 18:25 ` Nicolas Dufresne
  8 siblings, 0 replies; 14+ messages in thread
From: Steve Longerbeam @ 2018-09-14 17:49 UTC (permalink / raw)
  To: Hugues Fruchet, Steve Longerbeam, Sakari Ailus, Jacopo Mondi,
	Hans Verkuil, Mauro Carvalho Chehab
  Cc: linux-media, Benjamin Gaignard

Hi Hughes,

The whole series,

Acked-by: Steve Longerbeam <steve_longerbeam@mentor.com>

and

Tested-by: Steve Longerbeam <steve_longerbeam@mentor.com>
on i.MX6q SabreSD with MIPI CSI-2 OV5640 module


On 09/11/2018 06:48 AM, 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.
>
> ===========
> = 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(-)
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 0/5] Fix OV5640 exposure & gain
  2018-09-11 13:48 [PATCH v3 0/5] Fix OV5640 exposure & gain Hugues Fruchet
                   ` (7 preceding siblings ...)
  2018-09-14 17:49 ` Steve Longerbeam
@ 2018-09-14 18:25 ` Nicolas Dufresne
  8 siblings, 0 replies; 14+ messages in thread
From: Nicolas Dufresne @ 2018-09-14 18:25 UTC (permalink / raw)
  To: Hugues Fruchet, Steve Longerbeam, Sakari Ailus, Jacopo Mondi,
	Hans Verkuil, Mauro Carvalho Chehab
  Cc: linux-media, Benjamin Gaignard

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

Interesting, I just hit this problem yesterday. Same module as Steve,  
with MIPI CSI-2 OV5640 (on Sabre Lite).

Tested-By: Nicolas Dufresne <nicolas.dufresne@collabora.com>

Le mardi 11 septembre 2018 à 15:48 +0200, Hugues Fruchet a écrit :
> 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.
> 
> ===========
> = 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(-)
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 0/5] Fix OV5640 exposure & gain
  2018-09-14 16:07 ` jacopo mondi
@ 2018-09-15 23:02   ` Sakari Ailus
  2018-09-17  7:47     ` jacopo mondi
  0 siblings, 1 reply; 14+ messages in thread
From: Sakari Ailus @ 2018-09-15 23:02 UTC (permalink / raw)
  To: jacopo mondi
  Cc: hugues.fruchet, Steve Longerbeam, Hans Verkuil,
	Mauro Carvalho Chehab, linux-media, Benjamin Gaignard

Hi Jacopo, Hugues,

On Fri, Sep 14, 2018 at 06:07:12PM +0200, jacopo mondi wrote:
> Hi Sakari,
> 
> 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.
> 
> As you offered to collect this series and my CSI-2 fixes I have just
> re-sent, you might be interested in this branch:
> 
> git://jmondi.org/linux
> engicam-imx6q/media-master/ov5640/csi2_init_v4_exposure_v3
> 
> I have there re-based this series on top of mine, which is in turn
> based on latest media master, where this series do not apply as-is
> afaict.
> 
> I have added to Hugues' patches my reviewed-by and tested-by tags.
> If you prefer to I can send you a pull request, or if you want to have
> a chance to review the whole patch list please refer to the above
> branch.
> 
> Let me know if I can help speeding up the inclusion of these two
> series as they fix two real issues of MIPI CSI-2 capture operations
> for this sensor.

I've pushed the patches here:

<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=for-4.20-5>

There was a merge commit and a few extra patches in your tree; I threw them
out. :-)

I also edited the commit messages slightly (format; no change in content)
--- the patches are as-is. I'll still check they look right before sending
a pull request, likely on Monday.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 0/5] Fix OV5640 exposure & gain
  2018-09-15 23:02   ` Sakari Ailus
@ 2018-09-17  7:47     ` jacopo mondi
  2018-09-17 11:40       ` Sakari Ailus
  0 siblings, 1 reply; 14+ messages in thread
From: jacopo mondi @ 2018-09-17  7:47 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: hugues.fruchet, Steve Longerbeam, Hans Verkuil,
	Mauro Carvalho Chehab, linux-media, Benjamin Gaignard

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

Hi Sakari,
        thanks for handling this

On Sun, Sep 16, 2018 at 02:02:30AM +0300, Sakari Ailus wrote:
> Hi Jacopo, Hugues,
>
> On Fri, Sep 14, 2018 at 06:07:12PM +0200, jacopo mondi wrote:
> > Hi Sakari,
> >
> > 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.
> >
> > As you offered to collect this series and my CSI-2 fixes I have just
> > re-sent, you might be interested in this branch:
> >
> > git://jmondi.org/linux
> > engicam-imx6q/media-master/ov5640/csi2_init_v4_exposure_v3
> >
> > I have there re-based this series on top of mine, which is in turn
> > based on latest media master, where this series do not apply as-is
> > afaict.
> >
> > I have added to Hugues' patches my reviewed-by and tested-by tags.
> > If you prefer to I can send you a pull request, or if you want to have
> > a chance to review the whole patch list please refer to the above
> > branch.
> >
> > Let me know if I can help speeding up the inclusion of these two
> > series as they fix two real issues of MIPI CSI-2 capture operations
> > for this sensor.
>
> I've pushed the patches here:
>
> <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=for-4.20-5>
>
> There was a merge commit and a few extra patches in your tree; I threw them
> out. :-)

Yeah, those are a few patches I need for my testing platform... Forgot to
remove them, hope you didn't spend too much time on this.
>
> I also edited the commit messages slightly (format; no change in content)
> --- the patches are as-is. I'll still check they look right before sending
> a pull request, likely on Monday.

Thanks, let me now if I can help.
Cheers
   j

>
> --
> Regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 0/5] Fix OV5640 exposure & gain
  2018-09-17  7:47     ` jacopo mondi
@ 2018-09-17 11:40       ` Sakari Ailus
  2018-09-24  8:11         ` Hugues FRUCHET
  0 siblings, 1 reply; 14+ messages in thread
From: Sakari Ailus @ 2018-09-17 11:40 UTC (permalink / raw)
  To: jacopo mondi
  Cc: Sakari Ailus, hugues.fruchet, Steve Longerbeam, Hans Verkuil,
	Mauro Carvalho Chehab, linux-media, Benjamin Gaignard

On Mon, Sep 17, 2018 at 09:47:19AM +0200, jacopo mondi wrote:
> Hi Sakari,
>         thanks for handling this
> 
> On Sun, Sep 16, 2018 at 02:02:30AM +0300, Sakari Ailus wrote:
> > Hi Jacopo, Hugues,
> >
> > On Fri, Sep 14, 2018 at 06:07:12PM +0200, jacopo mondi wrote:
> > > Hi Sakari,
> > >
> > > 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.
> > >
> > > As you offered to collect this series and my CSI-2 fixes I have just
> > > re-sent, you might be interested in this branch:
> > >
> > > git://jmondi.org/linux
> > > engicam-imx6q/media-master/ov5640/csi2_init_v4_exposure_v3
> > >
> > > I have there re-based this series on top of mine, which is in turn
> > > based on latest media master, where this series do not apply as-is
> > > afaict.
> > >
> > > I have added to Hugues' patches my reviewed-by and tested-by tags.
> > > If you prefer to I can send you a pull request, or if you want to have
> > > a chance to review the whole patch list please refer to the above
> > > branch.
> > >
> > > Let me know if I can help speeding up the inclusion of these two
> > > series as they fix two real issues of MIPI CSI-2 capture operations
> > > for this sensor.
> >
> > I've pushed the patches here:
> >
> > <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=for-4.20-5>
> >
> > There was a merge commit and a few extra patches in your tree; I threw them
> > out. :-)
> 
> Yeah, those are a few patches I need for my testing platform... Forgot to
> remove them, hope you didn't spend too much time on this.

No, it was rather easy to remove them. I've sent a pull request on these:

<URL:https://patchwork.linuxtv.org/patch/52091/>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 0/5] Fix OV5640 exposure & gain
  2018-09-17 11:40       ` Sakari Ailus
@ 2018-09-24  8:11         ` Hugues FRUCHET
  0 siblings, 0 replies; 14+ messages in thread
From: Hugues FRUCHET @ 2018-09-24  8:11 UTC (permalink / raw)
  To: Sakari Ailus, jacopo mondi
  Cc: Sakari Ailus, Steve Longerbeam, Hans Verkuil,
	Mauro Carvalho Chehab, linux-media@vger.kernel.org,
	Benjamin Gaignard

Many thanks for this work Jacopo and Sakari,

On my side I've made some other improvements/fixes on OV5640 that I will 
push in the coming days.

BR,
Hugues.

On 09/17/2018 01:40 PM, Sakari Ailus wrote:
> On Mon, Sep 17, 2018 at 09:47:19AM +0200, jacopo mondi wrote:
>> Hi Sakari,
>>          thanks for handling this
>>
>> On Sun, Sep 16, 2018 at 02:02:30AM +0300, Sakari Ailus wrote:
>>> Hi Jacopo, Hugues,
>>>
>>> On Fri, Sep 14, 2018 at 06:07:12PM +0200, jacopo mondi wrote:
>>>> Hi Sakari,
>>>>
>>>> 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.
>>>>
>>>> As you offered to collect this series and my CSI-2 fixes I have just
>>>> re-sent, you might be interested in this branch:
>>>>
>>>> git://jmondi.org/linux
>>>> engicam-imx6q/media-master/ov5640/csi2_init_v4_exposure_v3
>>>>
>>>> I have there re-based this series on top of mine, which is in turn
>>>> based on latest media master, where this series do not apply as-is
>>>> afaict.
>>>>
>>>> I have added to Hugues' patches my reviewed-by and tested-by tags.
>>>> If you prefer to I can send you a pull request, or if you want to have
>>>> a chance to review the whole patch list please refer to the above
>>>> branch.
>>>>
>>>> Let me know if I can help speeding up the inclusion of these two
>>>> series as they fix two real issues of MIPI CSI-2 capture operations
>>>> for this sensor.
>>>
>>> I've pushed the patches here:
>>>
>>> <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=for-4.20-5>
>>>
>>> There was a merge commit and a few extra patches in your tree; I threw them
>>> out. :-)
>>
>> Yeah, those are a few patches I need for my testing platform... Forgot to
>> remove them, hope you didn't spend too much time on this.
> 
> No, it was rather easy to remove them. I've sent a pull request on these:
> 
> <URL:https://patchwork.linuxtv.org/patch/52091/>
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2018-09-24 14:12 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-11 13:48 [PATCH v3 0/5] Fix OV5640 exposure & gain Hugues Fruchet
2018-09-11 13:48 ` [PATCH v3 1/5] media: ov5640: fix exposure regression Hugues Fruchet
2018-09-11 13:48 ` [PATCH v3 2/5] media: ov5640: fix auto gain & exposure when changing mode Hugues Fruchet
2018-09-11 13:48 ` [PATCH v3 3/5] media: ov5640: fix wrong binning value in exposure calculation Hugues Fruchet
2018-09-11 13:48 ` [PATCH v3 4/5] media: ov5640: fix auto controls values when switching to manual mode Hugues Fruchet
2018-09-11 13:48 ` [PATCH v3 5/5] media: ov5640: fix restore of last mode set Hugues Fruchet
2018-09-14 16:00 ` [PATCH v3 0/5] Fix OV5640 exposure & gain jacopo mondi
2018-09-14 16:07 ` jacopo mondi
2018-09-15 23:02   ` Sakari Ailus
2018-09-17  7:47     ` jacopo mondi
2018-09-17 11:40       ` Sakari Ailus
2018-09-24  8:11         ` Hugues FRUCHET
2018-09-14 17:49 ` Steve Longerbeam
2018-09-14 18:25 ` Nicolas Dufresne

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox