[PATCH v2 0/5] Fix OV5640 exposure & gain

[PATCH v2 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.

===========
= history =
===========
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 | 124 ++++++++++++++++++++++++++-------------------
 1 file changed, 72 insertions(+), 52 deletions(-)

-- 
2.7.4

[PATCH v2 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)
-- 
2.7.4

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

[Laurent Pinchart]

Hi Hugues,

Thank you for the patch.

On Monday, 13 August 2018 13:19:42 EEST Hugues Fruchet wrote:
> 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)

-- 
Regards,

Laurent Pinchart

[PATCH v2 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;
 }
-- 
2.7.4

[PATCH v2 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, fix this.
Rename ov5640_binning_on() to ov5640_get_binning() as per other
similar functions.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.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

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

[Laurent Pinchart]

Hi Hugues,

Thank you for the patch.

On Monday, 13 August 2018 13:19:44 EEST Hugues Fruchet wrote:
> 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>

Reiewed-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 &&


-- 
Regards,

Laurent Pinchart

[PATCH v2 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 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

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

[Laurent Pinchart]

Hi Hugues,

Thank you for the patch.

On Monday, 13 August 2018 13:19:45 EEST Hugues Fruchet wrote:
> 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;

What is this even supposed to do ? Only the V4L2_CID_GAIN and 
V4L2_CID_EXPOSURE have the volatile flag set. Why can't this code be replaced 
with

	case V4L2_CID_GAIN:
  		val = ov5640_get_gain(sensor);
  		if (val < 0)
  			return val;
  		ctrl->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;

And same here.

-- 
Regards,

Laurent Pinchart

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

[Hugues FRUCHET]

Hi Laurent,

On 09/06/2018 03:31 PM, Laurent Pinchart wrote:
> Hi Hugues,
> 
> Thank you for the patch.
> 
> On Monday, 13 August 2018 13:19:45 EEST Hugues Fruchet wrote:
>> 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;
> 
> What is this even supposed to do ? Only the V4L2_CID_GAIN and
> V4L2_CID_EXPOSURE have the volatile flag set. Why can't this code be replaced
> with

This is because V4L2_CID_AUTOGAIN & V4L2_CID_GAIN are declared as 
auto-cluster:
  static int ov5640_init_controls(struct ov5640_dev *sensor)
	/* Auto/manual gain */
	ctrls->auto_gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTOGAIN,
					     0, 1, 1, 1);
	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN,
					0, 1023, 1, 0);
[...]
	v4l2_ctrl_auto_cluster(2, &ctrls->auto_gain, 0, true);

By checking many other drivers that are using clustered auto controls, 
they are all doing that way:

ctrls->auto_x = v4l2_ctrl_new_std(CID_X_AUTO..
ctrls->x = v4l2_ctrl_new_std(CID_X..
v4l2_ctrl_auto_cluster(2, &ctrls->auto, 0, true);

g_volatile_ctrl(ctrl)
   switch (ctrl->id) {
    case CID_X_AUTO:
      ctrls->x->val = READ_REG()

> 
> 	case V4L2_CID_GAIN:
>    		val = ov5640_get_gain(sensor);
>    		if (val < 0)
>    			return val;
>    		ctrl->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;
> 
> And same here.
> 

Best regards,
Hugues.

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

[Laurent Pinchart]

Hi Hugues,

On Monday, 10 September 2018 13:23:41 EEST Hugues FRUCHET wrote:
> On 09/06/2018 03:31 PM, Laurent Pinchart wrote:
> > On Monday, 13 August 2018 13:19:45 EEST Hugues Fruchet wrote:
> > 
> >> 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;
> > 
> > What is this even supposed to do ? Only the V4L2_CID_GAIN and
> > V4L2_CID_EXPOSURE have the volatile flag set. Why can't this code be
> > replaced with
> 
> This is because V4L2_CID_AUTOGAIN & V4L2_CID_GAIN are declared as 
> auto-cluster:
>   static int ov5640_init_controls(struct ov5640_dev *sensor)
> 	/* Auto/manual gain */
> 	ctrls->auto_gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTOGAIN,
> 					     0, 1, 1, 1);
> 	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN,
> 					0, 1023, 1, 0);
> [...]
> 	v4l2_ctrl_auto_cluster(2, &ctrls->auto_gain, 0, true);
> 
> By checking many other drivers that are using clustered auto controls, 
> they are all doing that way:
> 
> ctrls->auto_x = v4l2_ctrl_new_std(CID_X_AUTO..
> ctrls->x = v4l2_ctrl_new_std(CID_X..
> v4l2_ctrl_auto_cluster(2, &ctrls->auto, 0, true);
> 
> g_volatile_ctrl(ctrl)
>    switch (ctrl->id) {
>     case CID_X_AUTO:
>       ctrls->x->val = READ_REG()

Seems like cargo-cult to me. Why is this better than the construct below ?

> > 	case V4L2_CID_GAIN:
> >    		val = ov5640_get_gain(sensor);
> >    		if (val < 0)
> >    			return val;
> >    		ctrl->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;
> > 
> > And same here.

-- 
Regards,

Laurent Pinchart

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

[Hugues FRUCHET]

Hi Laurent,

On 09/10/2018 12:46 PM, Laurent Pinchart wrote:
> Hi Hugues,
> 
> On Monday, 10 September 2018 13:23:41 EEST Hugues FRUCHET wrote:
>> On 09/06/2018 03:31 PM, Laurent Pinchart wrote:
>>> On Monday, 13 August 2018 13:19:45 EEST Hugues Fruchet wrote:
>>>
>>>> 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;
>>>
>>> What is this even supposed to do ? Only the V4L2_CID_GAIN and
>>> V4L2_CID_EXPOSURE have the volatile flag set. Why can't this code be
>>> replaced with
>>
>> This is because V4L2_CID_AUTOGAIN & V4L2_CID_GAIN are declared as
>> auto-cluster:
>>    static int ov5640_init_controls(struct ov5640_dev *sensor)
>> 	/* Auto/manual gain */
>> 	ctrls->auto_gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTOGAIN,
>> 					     0, 1, 1, 1);
>> 	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN,
>> 					0, 1023, 1, 0);
>> [...]
>> 	v4l2_ctrl_auto_cluster(2, &ctrls->auto_gain, 0, true);
>>
>> By checking many other drivers that are using clustered auto controls,
>> they are all doing that way:
>>
>> ctrls->auto_x = v4l2_ctrl_new_std(CID_X_AUTO..
>> ctrls->x = v4l2_ctrl_new_std(CID_X..
>> v4l2_ctrl_auto_cluster(2, &ctrls->auto, 0, true);
>>
>> g_volatile_ctrl(ctrl)
>>     switch (ctrl->id) {
>>      case CID_X_AUTO:
>>        ctrls->x->val = READ_REG()
> 
> Seems like cargo-cult to me. Why is this better than the construct below ?
> 

I have done the changes as per your suggestion, but behaviour is broken: 
when autogain control is on and I read gain value, gain is not refreshed 
with current gain value from sensor, but stick to last manual value set.

Moreover I've checked in vivid how it is done and still we have the 
structure of code I've already mentionned:

static int vivid_user_vid_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
{
	struct vivid_dev *dev = container_of(ctrl->handler, struct vivid_dev, 
ctrl_hdl_user_vid);

	switch (ctrl->id) {
	case V4L2_CID_AUTOGAIN:
		dev->gain->val = dev->jiffies_vid_cap & 0xff;
		break;
	}
	return 0;
}


>>> 	case V4L2_CID_GAIN:
>>>     		val = ov5640_get_gain(sensor);
>>>     		if (val < 0)
>>>     			return val;
>>>     		ctrl->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;
>>>
>>> And same here.
> 

BR Hugues.

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

[Laurent Pinchart]

Hi Hugues,

(Hans, there's a question for you below)

On Monday, 10 September 2018 17:43:27 EEST Hugues FRUCHET wrote:
> On 09/10/2018 12:46 PM, Laurent Pinchart wrote:
> > On Monday, 10 September 2018 13:23:41 EEST Hugues FRUCHET wrote:
> >> On 09/06/2018 03:31 PM, Laurent Pinchart wrote:
> >>> On Monday, 13 August 2018 13:19:45 EEST Hugues Fruchet wrote:
> >>>
> >>>> 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;
> >>>
> >>> What is this even supposed to do ? Only the V4L2_CID_GAIN and
> >>> V4L2_CID_EXPOSURE have the volatile flag set. Why can't this code be
> >>> replaced with
> >>
> >> This is because V4L2_CID_AUTOGAIN & V4L2_CID_GAIN are declared as
> >> auto-cluster:
> >> 
> >>    static int ov5640_init_controls(struct ov5640_dev *sensor)
> >> 	
> >> 	/* Auto/manual gain */
> >> 	ctrls->auto_gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTOGAIN,
> >> 					     0, 1, 1, 1);
> >> 	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN,
> >> 					0, 1023, 1, 0);
> >> 
> >> [...]
> >> 
> >> 	v4l2_ctrl_auto_cluster(2, &ctrls->auto_gain, 0, true);
> >>
> >> By checking many other drivers that are using clustered auto controls,
> >> they are all doing that way:
> >>
> >> ctrls->auto_x = v4l2_ctrl_new_std(CID_X_AUTO..
> >> ctrls->x = v4l2_ctrl_new_std(CID_X..
> >> v4l2_ctrl_auto_cluster(2, &ctrls->auto, 0, true);
> >>
> >> g_volatile_ctrl(ctrl)
> >>     switch (ctrl->id) {
> >>      case CID_X_AUTO:
> >>        ctrls->x->val = READ_REG()
> > 
> > Seems like cargo-cult to me. Why is this better than the construct below
> > ?
> 
> I have done the changes as per your suggestion, but behaviour is broken: 
> when autogain control is on and I read gain value, gain is not refreshed 
> with current gain value from sensor, but stick to last manual value set.

This looks like an issue in the control framework. We shouldn't have to force 
all drivers to implement workarounds.

Hans, what's your opinion ?

> Moreover I've checked in vivid how it is done and still we have the 
> structure of code I've already mentionned:
> 
> static int vivid_user_vid_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> {
> 	struct vivid_dev *dev = container_of(ctrl->handler, struct vivid_dev, 
> ctrl_hdl_user_vid);
> 
> 	switch (ctrl->id) {
> 	case V4L2_CID_AUTOGAIN:
> 		dev->gain->val = dev->jiffies_vid_cap & 0xff;
> 		break;
> 	}
> 	return 0;
> }
> 
> >>> 	case V4L2_CID_GAIN:
> >>>     		val = ov5640_get_gain(sensor);
> >>>     		if (val < 0)
> >>>     			return val;
> >>>     		ctrl->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;
> >>>
> >>> And same here.

-- 
Regards,

Laurent Pinchart

[PATCH v2 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 c110a6a..923cc30 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;
 
-- 
2.7.4

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

[jacopo mondi]

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

Hi Hugues,
    thanks for the patch

On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote:
> 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.

I actually see a different issue here...

The issue I see here depends on the format programmed through
set_fmt() never being applied when using the sensor with a media
controller equipped device (in this case an i.MX6 board) through
capture sessions, and the not properly calculated exposure you see may
be a consequence of this.

I'll try to write down what I see, with the help of some debug output.

- At probe time mode 640x460@30 is programmed:
  [    1.651216] ov5640_probe: Initial mode with id: 2

- I set the format on the sensor's pad and it gets not applied but
  marked as pending as the sensor is powered off:

  #media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/320x240 field:none]"
   [   65.611983] ov5640_set_fmt: NEW mode with id: 1 - PENDING

- I start streaming with yavta, and the sensor receives a power on;
  this causes the 'initial' format to be re-programmed and the pending
  change to be ignored:

  #yavta -c10 -n4 -f YUYV -s $320x240  -F"../frame-#.yuv" /dev/video4
   [   69.395018] ov5640_set_power:1805 - on
   [   69.431342] ov5640_restore_mode:1711
   [   69.996882] ov5640_set_mode: Apply mode with id: 0

  The 'ov5640_set_mode()' call from 'ov5640_restore_mode()' clears the
  sensor->pending flag, discarding the newly requested format, for
  this reason, at s_stream() time, the pending flag is not set
  anymore.

Are you using a media-controller system? I suspect in non-mc cases,
the set_fmt is applied through a single power_on/power_off session, not
causing the 'restore_mode()' issue. Is this the case for you or your
issue is differnt?

Edit:
Mita-san tried to address the issue of the output pixel format not
being restored when the image format was restored in
19ad26f9e6e1 ("media: ov5640: add missing output pixel format setting")

I understand the issue he tried to fix, but shouldn't the pending
format (if any) be applied instead of the initial one unconditionally?

Thanks
   j

>
> 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 c110a6a..923cc30 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;
>
> --
> 2.7.4
>

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

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

[Hugues FRUCHET]

Hi Jacopo,

On 08/16/2018 12:10 PM, jacopo mondi wrote:
> Hi Hugues,
>      thanks for the patch
> 
> On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote:
>> 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.
> 
> I actually see a different issue here...

Which problem do you have exactly, you got a VGA JPEG instead of a QVGA 
YUYV ?

> 
> The issue I see here depends on the format programmed through
> set_fmt() never being applied when using the sensor with a media
> controller equipped device (in this case an i.MX6 board) through
> capture sessions, and the not properly calculated exposure you see may
> be a consequence of this.
> 
> I'll try to write down what I see, with the help of some debug output.
> 
> - At probe time mode 640x460@30 is programmed:
>    [    1.651216] ov5640_probe: Initial mode with id: 2
> 
> - I set the format on the sensor's pad and it gets not applied but
>    marked as pending as the sensor is powered off:
> 
>    #media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/320x240 field:none]"
>     [   65.611983] ov5640_set_fmt: NEW mode with id: 1 - PENDING

So here sensor->current_mode is set to <1>;//QVGA
and sensor->pending_mode_change is set to true;

> 
> - I start streaming with yavta, and the sensor receives a power on;
>    this causes the 'initial' format to be re-programmed and the pending
>    change to be ignored:
> 
>    #yavta -c10 -n4 -f YUYV -s $320x240  -F"../frame-#.yuv" /dev/video4
>     [   69.395018] ov5640_set_power:1805 - on
>     [   69.431342] ov5640_restore_mode:1711
>     [   69.996882] ov5640_set_mode: Apply mode with id: 0
> 
>    The 'ov5640_set_mode()' call from 'ov5640_restore_mode()' clears the
>    sensor->pending flag, discarding the newly requested format, for
>    this reason, at s_stream() time, the pending flag is not set
>    anymore.

OK but before clearing sensor->pending_mode_change, set_mode() is
loading registers corresponding to sensor->current_mode:
static int ov5640_set_mode(struct ov5640_dev *sensor,
			   const struct ov5640_mode_info *orig_mode)
{
==>	const struct ov5640_mode_info *mode = sensor->current_mode;
...
	ret = ov5640_set_mode_direct(sensor, mode, exposure);

=> so mode <1> is expected to be set now, so I don't understand your trace:
">     [   69.996882] ov5640_set_mode: Apply mode with id: 0"
Which variable do you trace that shows "0" ?


> 
> Are you using a media-controller system? I suspect in non-mc cases,
> the set_fmt is applied through a single power_on/power_off session, not
> causing the 'restore_mode()' issue. Is this the case for you or your
> issue is differnt?
> 
> Edit:
> Mita-san tried to address the issue of the output pixel format not
> being restored when the image format was restored in
> 19ad26f9e6e1 ("media: ov5640: add missing output pixel format setting")
> 
> I understand the issue he tried to fix, but shouldn't the pending
> format (if any) be applied instead of the initial one unconditionally?

This is what does the ov5640_restore_mode(), set the current mode 
(sensor->current_mode), that is done through this line:
	/* now restore the last capture mode */
	ret = ov5640_set_mode(sensor, &ov5640_mode_init_data);
=> note that the comment above is weird, in fact it is the "current" 
mode that is set.
=> note also that the 2nd parameter is not the mode to be set but the 
previously applied mode ! (ie loaded in ov5640 registers). This is used
to decide if we have to go to the "set_mode_exposure_calc" or 
"set_mode_direct".

the ov5640_restore_mode() also set the current pixel format 
(sensor->fmt), that is done through this line:
	return ov5640_set_framefmt(sensor, &sensor->fmt);
==> This is what have fixed Mita-san, this line was missing previously, 
leading to "mode registers" being loaded but not the "pixel format 
registers".


PS: There are two other "set mode" related changes that are related to this:
1) 6949d864776e ("media: ov5640: do not change mode if format or frame 
interval is unchanged")
=> this is merged in media master, unfortunately I've introduced a 
regression on "pixel format" side that I've fixed in this patchset :
2) https://www.mail-archive.com/linux-media@vger.kernel.org/msg134413.html
Symptom was a noisy image when capturing QVGA YUV (in fact captured as 
JPEG data).


Best regards,
Hugues.

> 
> Thanks
>     j
> 
>>
>> 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 c110a6a..923cc30 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;
>>
>> --
>> 2.7.4
>>

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

[jacopo mondi]

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

Hi Hugues,

On Thu, Aug 16, 2018 at 03:07:54PM +0000, Hugues FRUCHET wrote:
> Hi Jacopo,
>
> On 08/16/2018 12:10 PM, jacopo mondi wrote:
> > Hi Hugues,
> >      thanks for the patch
> >
> > On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote:
> >> 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.
> >
> > I actually see a different issue here...
>
> Which problem do you have exactly, you got a VGA JPEG instead of a QVGA
> YUYV ?
>

Not a matter of image format but sizes. I printed out the format
applied and it seems to me it was always the initial one.
On a second thought, a pipeline with a mis-configuration would not
have ever started streaming, so I should have investigated better.

> >
> > The issue I see here depends on the format programmed through
> > set_fmt() never being applied when using the sensor with a media
> > controller equipped device (in this case an i.MX6 board) through
> > capture sessions, and the not properly calculated exposure you see may
> > be a consequence of this.
> >
> > I'll try to write down what I see, with the help of some debug output.
> >
> > - At probe time mode 640x460@30 is programmed:
> >    [    1.651216] ov5640_probe: Initial mode with id: 2
> >
> > - I set the format on the sensor's pad and it gets not applied but
> >    marked as pending as the sensor is powered off:
> >
> >    #media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/320x240 field:none]"
> >     [   65.611983] ov5640_set_fmt: NEW mode with id: 1 - PENDING
>
> So here sensor->current_mode is set to <1>;//QVGA
> and sensor->pending_mode_change is set to true;
>
> >
> > - I start streaming with yavta, and the sensor receives a power on;
> >    this causes the 'initial' format to be re-programmed and the pending
> >    change to be ignored:
> >
> >    #yavta -c10 -n4 -f YUYV -s $320x240  -F"../frame-#.yuv" /dev/video4
> >     [   69.395018] ov5640_set_power:1805 - on
> >     [   69.431342] ov5640_restore_mode:1711
> >     [   69.996882] ov5640_set_mode: Apply mode with id: 0
> >
> >    The 'ov5640_set_mode()' call from 'ov5640_restore_mode()' clears the
> >    sensor->pending flag, discarding the newly requested format, for
> >    this reason, at s_stream() time, the pending flag is not set
> >    anymore.
>
> OK but before clearing sensor->pending_mode_change, set_mode() is
> loading registers corresponding to sensor->current_mode:
> static int ov5640_set_mode(struct ov5640_dev *sensor,
> 			   const struct ov5640_mode_info *orig_mode)
> {
> ==>	const struct ov5640_mode_info *mode = sensor->current_mode;
> ...
> 	ret = ov5640_set_mode_direct(sensor, mode, exposure);
>
> => so mode <1> is expected to be set now, so I don't understand your trace:
> ">     [   69.996882] ov5640_set_mode: Apply mode with id: 0"
> Which variable do you trace that shows "0" ?
>
>
> >
> > Are you using a media-controller system? I suspect in non-mc cases,
> > the set_fmt is applied through a single power_on/power_off session, not
> > causing the 'restore_mode()' issue. Is this the case for you or your
> > issue is differnt?
> >
> > Edit:
> > Mita-san tried to address the issue of the output pixel format not
> > being restored when the image format was restored in
> > 19ad26f9e6e1 ("media: ov5640: add missing output pixel format setting")
> >
> > I understand the issue he tried to fix, but shouldn't the pending
> > format (if any) be applied instead of the initial one unconditionally?
>
> This is what does the ov5640_restore_mode(), set the current mode
> (sensor->current_mode), that is done through this line:
> 	/* now restore the last capture mode */
> 	ret = ov5640_set_mode(sensor, &ov5640_mode_init_data);
> => note that the comment above is weird, in fact it is the "current"
> mode that is set.
> => note also that the 2nd parameter is not the mode to be set but the
> previously applied mode ! (ie loaded in ov5640 registers). This is used
> to decide if we have to go to the "set_mode_exposure_calc" or
> "set_mode_direct".

This is where I got lost... Sorry for the sloppy comment, now I get
what your patch was for :)


>
> the ov5640_restore_mode() also set the current pixel format
> (sensor->fmt), that is done through this line:
> 	return ov5640_set_framefmt(sensor, &sensor->fmt);
> ==> This is what have fixed Mita-san, this line was missing previously,
> leading to "mode registers" being loaded but not the "pixel format
> registers".
>
>
> PS: There are two other "set mode" related changes that are related to this:
> 1) 6949d864776e ("media: ov5640: do not change mode if format or frame
> interval is unchanged")
> => this is merged in media master, unfortunately I've introduced a
> regression on "pixel format" side that I've fixed in this patchset :
> 2) https://www.mail-archive.com/linux-media@vger.kernel.org/msg134413.html
> Symptom was a noisy image when capturing QVGA YUV (in fact captured as
> JPEG data).

I see, thanks for the detailed explanation!

Thanks
   j

>
>
> Best regards,
> Hugues.
>
> >
> > Thanks
> >     j
> >
> >>
> >> 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 c110a6a..923cc30 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;
> >>
> >> --
> >> 2.7.4
> >>

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

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

[Laurent Pinchart]

Hello Hugues,

On Thursday, 16 August 2018 18:07:54 EEST Hugues FRUCHET wrote:
> On 08/16/2018 12:10 PM, jacopo mondi wrote:
> > On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote:
> > 
> >> 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.
> > 
> > I actually see a different issue here...
> 
> Which problem do you have exactly, you got a VGA JPEG instead of a QVGA 
> YUYV ?
> 
> > The issue I see here depends on the format programmed through
> > set_fmt() never being applied when using the sensor with a media
> > controller equipped device (in this case an i.MX6 board) through
> > capture sessions, and the not properly calculated exposure you see may
> > be a consequence of this.
> > 
> > I'll try to write down what I see, with the help of some debug output.
> > 
> > - At probe time mode 640x460@30 is programmed:
> > 
> >    [    1.651216] ov5640_probe: Initial mode with id: 2
> > 
> > - I set the format on the sensor's pad and it gets not applied but
> >    marked as pending as the sensor is powered off:
> > 
> >    #media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/320x240
> >    field:none]"
> >     [   65.611983] ov5640_set_fmt: NEW mode with id: 1 - PENDING
> 
> So here sensor->current_mode is set to <1>;//QVGA
> and sensor->pending_mode_change is set to true;
> 
> > - I start streaming with yavta, and the sensor receives a power on;
> >    this causes the 'initial' format to be re-programmed and the pending
> >    change to be ignored:
> > 
> >    #yavta -c10 -n4 -f YUYV -s $320x240  -F"../frame-#.yuv" /dev/video4
> >    
> >     [   69.395018] ov5640_set_power:1805 - on
> >     [   69.431342] ov5640_restore_mode:1711
> >     [   69.996882] ov5640_set_mode: Apply mode with id: 0
> > 
> >    The 'ov5640_set_mode()' call from 'ov5640_restore_mode()' clears the
> >    sensor->pending flag, discarding the newly requested format, for
> >    this reason, at s_stream() time, the pending flag is not set
> >    anymore.
> 
> OK but before clearing sensor->pending_mode_change, set_mode() is
> loading registers corresponding to sensor->current_mode:
> static int ov5640_set_mode(struct ov5640_dev *sensor,
> 			   const struct ov5640_mode_info *orig_mode)
> {
> ==>	const struct ov5640_mode_info *mode = sensor->current_mode;
> ...
> 	ret = ov5640_set_mode_direct(sensor, mode, exposure);
> 
> => so mode <1> is expected to be set now, so I don't understand your trace:
> ">     [   69.996882] ov5640_set_mode: Apply mode with id: 0"
> Which variable do you trace that shows "0" ?
> 
> > Are you using a media-controller system? I suspect in non-mc cases,
> > the set_fmt is applied through a single power_on/power_off session, not
> > causing the 'restore_mode()' issue. Is this the case for you or your
> > issue is differnt?
> > 
> > Edit:
> > Mita-san tried to address the issue of the output pixel format not
> > being restored when the image format was restored in
> > 19ad26f9e6e1 ("media: ov5640: add missing output pixel format setting")
> > 
> > I understand the issue he tried to fix, but shouldn't the pending
> > format (if any) be applied instead of the initial one unconditionally?
> 
> This is what does the ov5640_restore_mode(), set the current mode 
> (sensor->current_mode), that is done through this line:
> 	/* now restore the last capture mode */
> 	ret = ov5640_set_mode(sensor, &ov5640_mode_init_data);
> => note that the comment above is weird, in fact it is the "current" 
> mode that is set.
> => note also that the 2nd parameter is not the mode to be set but the 
> previously applied mode ! (ie loaded in ov5640 registers). This is used
> to decide if we have to go to the "set_mode_exposure_calc" or 
> "set_mode_direct".
> 
> the ov5640_restore_mode() also set the current pixel format 
> (sensor->fmt), that is done through this line:
> 	return ov5640_set_framefmt(sensor, &sensor->fmt);
> ==> This is what have fixed Mita-san, this line was missing previously, 
> leading to "mode registers" being loaded but not the "pixel format 
> registers".

This seems overly complicated to me. Why do we have to set the mode at power 
on time at all, why can't we do it at stream on time only, and simplify all 
this logic ?

> PS: There are two other "set mode" related changes that are related to
> this:
> 1) 6949d864776e ("media: ov5640: do not change mode if format or
> frame interval is unchanged")
> => this is merged in media master, unfortunately I've introduced a 
> regression on "pixel format" side that I've fixed in this patchset :
> 2) https://www.mail-archive.com/linux-media@vger.kernel.org/msg134413.html
> Symptom was a noisy image when capturing QVGA YUV (in fact captured as 
> JPEG data).

[snip]

-- 
Regards,

Laurent Pinchart

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

[Hugues FRUCHET]

Hi Laurent, Steve,

On 09/07/2018 04:18 PM, Laurent Pinchart wrote:
> Hello Hugues,
> 
> On Thursday, 16 August 2018 18:07:54 EEST Hugues FRUCHET wrote:
>> On 08/16/2018 12:10 PM, jacopo mondi wrote:
>>> On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote:
>>>
>>>> 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.
>>>
>>> I actually see a different issue here...
>>
>> Which problem do you have exactly, you got a VGA JPEG instead of a QVGA
>> YUYV ?
>>
>>> The issue I see here depends on the format programmed through
>>> set_fmt() never being applied when using the sensor with a media
>>> controller equipped device (in this case an i.MX6 board) through
>>> capture sessions, and the not properly calculated exposure you see may
>>> be a consequence of this.
>>>
>>> I'll try to write down what I see, with the help of some debug output.
>>>
>>> - At probe time mode 640x460@30 is programmed:
>>>
>>>     [    1.651216] ov5640_probe: Initial mode with id: 2
>>>
>>> - I set the format on the sensor's pad and it gets not applied but
>>>     marked as pending as the sensor is powered off:
>>>
>>>     #media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/320x240
>>>     field:none]"
>>>      [   65.611983] ov5640_set_fmt: NEW mode with id: 1 - PENDING
>>
>> So here sensor->current_mode is set to <1>;//QVGA
>> and sensor->pending_mode_change is set to true;
>>
>>> - I start streaming with yavta, and the sensor receives a power on;
>>>     this causes the 'initial' format to be re-programmed and the pending
>>>     change to be ignored:
>>>
>>>     #yavta -c10 -n4 -f YUYV -s $320x240  -F"../frame-#.yuv" /dev/video4
>>>     
>>>      [   69.395018] ov5640_set_power:1805 - on
>>>      [   69.431342] ov5640_restore_mode:1711
>>>      [   69.996882] ov5640_set_mode: Apply mode with id: 0
>>>
>>>     The 'ov5640_set_mode()' call from 'ov5640_restore_mode()' clears the
>>>     sensor->pending flag, discarding the newly requested format, for
>>>     this reason, at s_stream() time, the pending flag is not set
>>>     anymore.
>>
>> OK but before clearing sensor->pending_mode_change, set_mode() is
>> loading registers corresponding to sensor->current_mode:
>> static int ov5640_set_mode(struct ov5640_dev *sensor,
>> 			   const struct ov5640_mode_info *orig_mode)
>> {
>> ==>	const struct ov5640_mode_info *mode = sensor->current_mode;
>> ...
>> 	ret = ov5640_set_mode_direct(sensor, mode, exposure);
>>
>> => so mode <1> is expected to be set now, so I don't understand your trace:
>> ">     [   69.996882] ov5640_set_mode: Apply mode with id: 0"
>> Which variable do you trace that shows "0" ?
>>
>>> Are you using a media-controller system? I suspect in non-mc cases,
>>> the set_fmt is applied through a single power_on/power_off session, not
>>> causing the 'restore_mode()' issue. Is this the case for you or your
>>> issue is differnt?
>>>
>>> Edit:
>>> Mita-san tried to address the issue of the output pixel format not
>>> being restored when the image format was restored in
>>> 19ad26f9e6e1 ("media: ov5640: add missing output pixel format setting")
>>>
>>> I understand the issue he tried to fix, but shouldn't the pending
>>> format (if any) be applied instead of the initial one unconditionally?
>>
>> This is what does the ov5640_restore_mode(), set the current mode
>> (sensor->current_mode), that is done through this line:
>> 	/* now restore the last capture mode */
>> 	ret = ov5640_set_mode(sensor, &ov5640_mode_init_data);
>> => note that the comment above is weird, in fact it is the "current"
>> mode that is set.
>> => note also that the 2nd parameter is not the mode to be set but the
>> previously applied mode ! (ie loaded in ov5640 registers). This is used
>> to decide if we have to go to the "set_mode_exposure_calc" or
>> "set_mode_direct".
>>
>> the ov5640_restore_mode() also set the current pixel format
>> (sensor->fmt), that is done through this line:
>> 	return ov5640_set_framefmt(sensor, &sensor->fmt);
>> ==> This is what have fixed Mita-san, this line was missing previously,
>> leading to "mode registers" being loaded but not the "pixel format
>> registers".
> 
> This seems overly complicated to me. Why do we have to set the mode at power
> on time at all, why can't we do it at stream on time only, and simplify all
> this logic ?
> 

I'm not the author of this driver, Steve do you know the origin and the 
gain to do so ?
Anyway, I would prefer that we stabilize currently existing code before 
going to larger changes.

>> PS: There are two other "set mode" related changes that are related to
>> this:
>> 1) 6949d864776e ("media: ov5640: do not change mode if format or
>> frame interval is unchanged")
>> => this is merged in media master, unfortunately I've introduced a
>> regression on "pixel format" side that I've fixed in this patchset :
>> 2) https://www.mail-archive.com/linux-media@vger.kernel.org/msg134413.html
>> Symptom was a noisy image when capturing QVGA YUV (in fact captured as
>> JPEG data).
> 
> [snip]
> 
BR Hugues.

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

[Laurent Pinchart]

Hi Hugues,

On Monday, 10 September 2018 18:14:45 EEST Hugues FRUCHET wrote:
> On 09/07/2018 04:18 PM, Laurent Pinchart wrote:
> > On Thursday, 16 August 2018 18:07:54 EEST Hugues FRUCHET wrote:
> >> On 08/16/2018 12:10 PM, jacopo mondi wrote:
> >>> On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote:
> >>>
> >>>> 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.
> >>>
> >>> I actually see a different issue here...
> >>
> >> Which problem do you have exactly, you got a VGA JPEG instead of a QVGA
> >> YUYV ?
> >>
> >>> The issue I see here depends on the format programmed through
> >>> set_fmt() never being applied when using the sensor with a media
> >>> controller equipped device (in this case an i.MX6 board) through
> >>> capture sessions, and the not properly calculated exposure you see may
> >>> be a consequence of this.
> >>>
> >>> I'll try to write down what I see, with the help of some debug output.
> >>>
> >>> - At probe time mode 640x460@30 is programmed:
> >>>
> >>>     [    1.651216] ov5640_probe: Initial mode with id: 2
> >>>
> >>> - I set the format on the sensor's pad and it gets not applied but
> >>>     marked as pending as the sensor is powered off:
 >>>
> >>>     #media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/320x240
> >>>     field:none]"
> >>>      [   65.611983] ov5640_set_fmt: NEW mode with id: 1 - PENDING
> >>
> >> So here sensor->current_mode is set to <1>;//QVGA
> >> and sensor->pending_mode_change is set to true;
> >>
> >>> - I start streaming with yavta, and the sensor receives a power on;
> >>>     this causes the 'initial' format to be re-programmed and the
> >>>     pending change to be ignored:
> >>>
> >>>     #yavta -c10 -n4 -f YUYV -s $320x240  -F"../frame-#.yuv" /dev/video4
> >>>     
> >>>      [   69.395018] ov5640_set_power:1805 - on
> >>>      [   69.431342] ov5640_restore_mode:1711
> >>>      [   69.996882] ov5640_set_mode: Apply mode with id: 0
> >>>
> >>>     The 'ov5640_set_mode()' call from 'ov5640_restore_mode()' clears
> >>>     the sensor->pending flag, discarding the newly requested format, for
> >>>     this reason, at s_stream() time, the pending flag is not set
> >>>     anymore.
> >>
> >> OK but before clearing sensor->pending_mode_change, set_mode() is
> >> loading registers corresponding to sensor->current_mode:
> >> 
> >> static int ov5640_set_mode(struct ov5640_dev *sensor,
> >> 			   const struct ov5640_mode_info *orig_mode)
> >> {
> >> ==>	const struct ov5640_mode_info *mode = sensor->current_mode;
> >> ...
> >> 	ret = ov5640_set_mode_direct(sensor, mode, exposure);
> >>
> >> => so mode <1> is expected to be set now, so I don't understand your
> >> trace:
> >> ">     [   69.996882] ov5640_set_mode: Apply mode with id: 0"
> >> Which variable do you trace that shows "0" ?
> >>
> >>> Are you using a media-controller system? I suspect in non-mc cases,
> >>> the set_fmt is applied through a single power_on/power_off session, not
> >>> causing the 'restore_mode()' issue. Is this the case for you or your
> >>> issue is differnt?
> >>>
> >>> Edit:
> >>> Mita-san tried to address the issue of the output pixel format not
> >>> being restored when the image format was restored in
> >>> 19ad26f9e6e1 ("media: ov5640: add missing output pixel format setting")
> >>>
> >>> I understand the issue he tried to fix, but shouldn't the pending
> >>> format (if any) be applied instead of the initial one unconditionally?
> >>
> >> This is what does the ov5640_restore_mode(), set the current mode
> >> (sensor->current_mode), that is done through this line:
> >> 
> >> 	/* now restore the last capture mode */
> >> 	ret = ov5640_set_mode(sensor, &ov5640_mode_init_data);
> >> 
> >> => note that the comment above is weird, in fact it is the "current"
> >> mode that is set.
> >> => note also that the 2nd parameter is not the mode to be set but the
> >> previously applied mode ! (ie loaded in ov5640 registers). This is used
> >> to decide if we have to go to the "set_mode_exposure_calc" or
> >> "set_mode_direct".
> >>
> >> the ov5640_restore_mode() also set the current pixel format
> >> (sensor->fmt), that is done through this line:
> >> 
> >> 	return ov5640_set_framefmt(sensor, &sensor->fmt);
> >> 
> >> ==> This is what have fixed Mita-san, this line was missing previously,
> >> leading to "mode registers" being loaded but not the "pixel format
> >> registers".
> > 
> > This seems overly complicated to me. Why do we have to set the mode at
> > power on time at all, why can't we do it at stream on time only, and
> > simplify all this logic ?
> 
> I'm not the author of this driver, Steve do you know the origin and the 
> gain to do so ? Anyway, I would prefer that we stabilize currently existing
> code before going to larger changes.

I'm not opposed to that, but it's then pretty hard to review the patches, when 
they replace hard to read code with other hard to read code :-)

Eventually we should really clean this up.

> >> PS: There are two other "set mode" related changes that are related to
> >> this:
> >> 1) 6949d864776e ("media: ov5640: do not change mode if format or
> >> frame interval is unchanged")
> >> => this is merged in media master, unfortunately I've introduced a
> >> regression on "pixel format" side that I've fixed in this patchset :
> >> 2)
> >> https://www.mail-archive.com/linux-media@vger.kernel.org/msg134413.html
> >> Symptom was a noisy image when capturing QVGA YUV (in fact captured as
> >> JPEG data).
> > 
> > [snip]

-- 
Regards,

Laurent Pinchart

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

[Hugues FRUCHET]

Hi Laurent,

On 09/10/2018 10:56 PM, Laurent Pinchart wrote:
> Hi Hugues,
> 
> On Monday, 10 September 2018 18:14:45 EEST Hugues FRUCHET wrote:
>> On 09/07/2018 04:18 PM, Laurent Pinchart wrote:
>>> On Thursday, 16 August 2018 18:07:54 EEST Hugues FRUCHET wrote:
>>>> On 08/16/2018 12:10 PM, jacopo mondi wrote:
>>>>> On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote:
>>>>>
>>>>>> 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.
>>>>>
>>>>> I actually see a different issue here...
>>>>
>>>> Which problem do you have exactly, you got a VGA JPEG instead of a QVGA
>>>> YUYV ?
>>>>
>>>>> The issue I see here depends on the format programmed through
>>>>> set_fmt() never being applied when using the sensor with a media
>>>>> controller equipped device (in this case an i.MX6 board) through
>>>>> capture sessions, and the not properly calculated exposure you see may
>>>>> be a consequence of this.
>>>>>
>>>>> I'll try to write down what I see, with the help of some debug output.
>>>>>
>>>>> - At probe time mode 640x460@30 is programmed:
>>>>>
>>>>>      [    1.651216] ov5640_probe: Initial mode with id: 2
>>>>>
>>>>> - I set the format on the sensor's pad and it gets not applied but
>>>>>      marked as pending as the sensor is powered off:
>   >>>
>>>>>      #media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/320x240
>>>>>      field:none]"
>>>>>       [   65.611983] ov5640_set_fmt: NEW mode with id: 1 - PENDING
>>>>
>>>> So here sensor->current_mode is set to <1>;//QVGA
>>>> and sensor->pending_mode_change is set to true;
>>>>
>>>>> - I start streaming with yavta, and the sensor receives a power on;
>>>>>      this causes the 'initial' format to be re-programmed and the
>>>>>      pending change to be ignored:
>>>>>
>>>>>      #yavta -c10 -n4 -f YUYV -s $320x240  -F"../frame-#.yuv" /dev/video4
>>>>>      
>>>>>       [   69.395018] ov5640_set_power:1805 - on
>>>>>       [   69.431342] ov5640_restore_mode:1711
>>>>>       [   69.996882] ov5640_set_mode: Apply mode with id: 0
>>>>>
>>>>>      The 'ov5640_set_mode()' call from 'ov5640_restore_mode()' clears
>>>>>      the sensor->pending flag, discarding the newly requested format, for
>>>>>      this reason, at s_stream() time, the pending flag is not set
>>>>>      anymore.
>>>>
>>>> OK but before clearing sensor->pending_mode_change, set_mode() is
>>>> loading registers corresponding to sensor->current_mode:
>>>>
>>>> static int ov5640_set_mode(struct ov5640_dev *sensor,
>>>> 			   const struct ov5640_mode_info *orig_mode)
>>>> {
>>>> ==>	const struct ov5640_mode_info *mode = sensor->current_mode;
>>>> ...
>>>> 	ret = ov5640_set_mode_direct(sensor, mode, exposure);
>>>>
>>>> => so mode <1> is expected to be set now, so I don't understand your
>>>> trace:
>>>> ">     [   69.996882] ov5640_set_mode: Apply mode with id: 0"
>>>> Which variable do you trace that shows "0" ?
>>>>
>>>>> Are you using a media-controller system? I suspect in non-mc cases,
>>>>> the set_fmt is applied through a single power_on/power_off session, not
>>>>> causing the 'restore_mode()' issue. Is this the case for you or your
>>>>> issue is differnt?
>>>>>
>>>>> Edit:
>>>>> Mita-san tried to address the issue of the output pixel format not
>>>>> being restored when the image format was restored in
>>>>> 19ad26f9e6e1 ("media: ov5640: add missing output pixel format setting")
>>>>>
>>>>> I understand the issue he tried to fix, but shouldn't the pending
>>>>> format (if any) be applied instead of the initial one unconditionally?
>>>>
>>>> This is what does the ov5640_restore_mode(), set the current mode
>>>> (sensor->current_mode), that is done through this line:
>>>>
>>>> 	/* now restore the last capture mode */
>>>> 	ret = ov5640_set_mode(sensor, &ov5640_mode_init_data);
>>>>
>>>> => note that the comment above is weird, in fact it is the "current"
>>>> mode that is set.
>>>> => note also that the 2nd parameter is not the mode to be set but the
>>>> previously applied mode ! (ie loaded in ov5640 registers). This is used
>>>> to decide if we have to go to the "set_mode_exposure_calc" or
>>>> "set_mode_direct".
>>>>
>>>> the ov5640_restore_mode() also set the current pixel format
>>>> (sensor->fmt), that is done through this line:
>>>>
>>>> 	return ov5640_set_framefmt(sensor, &sensor->fmt);
>>>>
>>>> ==> This is what have fixed Mita-san, this line was missing previously,
>>>> leading to "mode registers" being loaded but not the "pixel format
>>>> registers".
>>>
>>> This seems overly complicated to me. Why do we have to set the mode at
>>> power on time at all, why can't we do it at stream on time only, and
>>> simplify all this logic ?
>>
>> I'm not the author of this driver, Steve do you know the origin and the
>> gain to do so ? Anyway, I would prefer that we stabilize currently existing
>> code before going to larger changes.
> 
> I'm not opposed to that, but it's then pretty hard to review the patches, when
> they replace hard to read code with other hard to read code :-)
> 
> Eventually we should really clean this up.

No problem to cleanup that code, I will be really happy to simplify that 
stuff, but there are lot of stakeholders now so better to isolate that 
exact change in a new serie and ask for a non-regression campaign.

> 
>>>> PS: There are two other "set mode" related changes that are related to
>>>> this:
>>>> 1) 6949d864776e ("media: ov5640: do not change mode if format or
>>>> frame interval is unchanged")
>>>> => this is merged in media master, unfortunately I've introduced a
>>>> regression on "pixel format" side that I've fixed in this patchset :
>>>> 2)
>>>> https://www.mail-archive.com/linux-media@vger.kernel.org/msg134413.html
>>>> Symptom was a noisy image when capturing QVGA YUV (in fact captured as
>>>> JPEG data).
>>>
>>> [snip]
> 

BR Hugues.

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

[jacopo mondi]

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

Hi Hugues,
 one more comment on this patch..

On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote:
> 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 c110a6a..923cc30 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;
> +

Am I wrong or with the introduction of last_mode we could drop the
'orig_mode' parameter (which has confused me already :/ ) from the
set_mode() function?

Just set here 'orig_mode = sensor->last_mode' and make sure last_mode
is intialized properly at probe time...

Or is there some other value in keeping the orig_mode parameter here?

Thanks
   j

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

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

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

[Hugues FRUCHET]

Hi Jacopo,

On 08/25/2018 04:53 PM, jacopo mondi wrote:
> Hi Hugues,
>   one more comment on this patch..
> 
> On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote:
>> 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 c110a6a..923cc30 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;
>> +
> 
> Am I wrong or with the introduction of last_mode we could drop the
> 'orig_mode' parameter (which has confused me already :/ ) from the
> set_mode() function?
> 
> Just set here 'orig_mode = sensor->last_mode' and make sure last_mode
> is intialized properly at probe time...
> 
> Or is there some other value in keeping the orig_mode parameter here?
> 
> Thanks
>     j

I'm fine with that change, will push it in v3.


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

BR Hugues.