[PATCH 0/2] media: ov5640: Fix set_timings and auto-exposure

[PATCH 0/2] media: ov5640: Fix set_timings and auto-exposure

[Jacopo Mondi]

Hello,
   the ov5640 driver has received a lot of attentions recently.

Maxime and Sam tackled the clock tree handling and I tried to fix the MIPI
capture on i.MX6 platforms, but none of those patches ever made it to inclusion.

Although a few fixes have been circulating around those series, and it's my
opinion it is worth including them before any other developments takes place.

I'm sending here a re-work of a patch from Sam and Maxime to fix timings setup,
and a small fix for auto-exposure enabling/disabling operations.

Each patch has a comment, on which I would like to have some feedback on.

[1/1] has already received several acks on the mailing list, but never a
formal Reviewed-by or Tested-by, while [2/2] is new.

Thanks
   j

Jacopo Mondi (2):
  media: ov5640: Fix timings setup code
  media: ov5640: Fix auto-exposure disabling

 drivers/media/i2c/ov5640.c | 75 ++++++++++++++++++++--------------------------
 1 file changed, 32 insertions(+), 43 deletions(-)

--
2.7.4

[PATCH 1/2] media: ov5640: Fix timings setup code

[Jacopo Mondi]

As of:
commit 476dec012f4c ("media: ov5640: Add horizontal and vertical totals")
the timings parameters gets programmed separately from the static register
values array.

When changing capture mode, the vertical and horizontal totals gets inspected
by the set_mode_exposure_calc() functions, and only later programmed with the
new values. This means exposure, light banding filter and shutter gain are
calculated using the previous timings, and are thus not correct.

Fix this by programming timings right after the static register value table
has been sent to the sensor in the ov5640_load_regs() function.

Fixes: 476dec012f4c ("media: ov5640: Add horizontal and vertical totals")
Signed-off-by: Samuel Bobrowicz <sam@elite-embedded.com>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

---
This fix has been circulating around for quite some time now, in Maxime clock
tree patches, in Sam dropbox patches and in my latest MIPI fixes patches.
While the rest of the series have not yet been accepted, there is general
consensus this is an actual fix that has to be collected.

I've slightly modified Sam's and Maxime's version I previously sent,
programming timings directly in ov5640_load_regs() function.
You can find Sam's previous version here:
https://www.mail-archive.com/linux-media@vger.kernel.org/msg131654.html
and mine here, with an additional change that aimed to fix MIPI mode, which
I've left out in this version:
https://www.mail-archive.com/linux-media@vger.kernel.org/msg133422.html

Sam, Maxime, I took the liberty of taking your Signed-off-by from the previous
patch, as this was spotted by you first. Is this ok with you?

Thanks
   j
---
---
 drivers/media/i2c/ov5640.c | 50 +++++++++++++++++++---------------------------
 1 file changed, 21 insertions(+), 29 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 1ecbb7a..12b3496 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -908,6 +908,26 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
 }
 
 /* download ov5640 settings to sensor through i2c */
+static int ov5640_set_timings(struct ov5640_dev *sensor,
+			      const struct ov5640_mode_info *mode)
+{
+	int ret;
+
+	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPHO, mode->hact);
+	if (ret < 0)
+		return ret;
+
+	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPVO, mode->vact);
+	if (ret < 0)
+		return ret;
+
+	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_HTS, mode->htot);
+	if (ret < 0)
+		return ret;
+
+	return ov5640_write_reg16(sensor, OV5640_REG_TIMING_VTS, mode->vtot);
+}
+
 static int ov5640_load_regs(struct ov5640_dev *sensor,
 			    const struct ov5640_mode_info *mode)
 {
@@ -935,7 +955,7 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
 			usleep_range(1000 * delay_ms, 1000 * delay_ms + 100);
 	}
 
-	return ret;
+	return ov5640_set_timings(sensor, mode);
 }
 
 /* read exposure, in number of line periods */
@@ -1385,30 +1405,6 @@ static int ov5640_set_virtual_channel(struct ov5640_dev *sensor)
 	return ov5640_write_reg(sensor, OV5640_REG_DEBUG_MODE, temp);
 }
 
-static int ov5640_set_timings(struct ov5640_dev *sensor,
-			      const struct ov5640_mode_info *mode)
-{
-	int ret;
-
-	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPHO, mode->hact);
-	if (ret < 0)
-		return ret;
-
-	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPVO, mode->vact);
-	if (ret < 0)
-		return ret;
-
-	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_HTS, mode->htot);
-	if (ret < 0)
-		return ret;
-
-	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_VTS, mode->vtot);
-	if (ret < 0)
-		return ret;
-
-	return 0;
-}
-
 static const struct ov5640_mode_info *
 ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
 		 int width, int height, bool nearest)
@@ -1652,10 +1648,6 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 	if (ret < 0)
 		return ret;
 
-	ret = ov5640_set_timings(sensor, mode);
-	if (ret < 0)
-		return ret;
-
 	ret = ov5640_set_binning(sensor, dn_mode != SCALING);
 	if (ret < 0)
 		return ret;
-- 
2.7.4

Re: [PATCH 1/2] media: ov5640: Fix timings setup code

[Maxime Ripard]

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

On Wed, Jul 18, 2018 at 01:19:02PM +0200, Jacopo Mondi wrote:
> As of:
> commit 476dec012f4c ("media: ov5640: Add horizontal and vertical totals")
> the timings parameters gets programmed separately from the static register
> values array.
> 
> When changing capture mode, the vertical and horizontal totals gets inspected
> by the set_mode_exposure_calc() functions, and only later programmed with the
> new values. This means exposure, light banding filter and shutter gain are
> calculated using the previous timings, and are thus not correct.
> 
> Fix this by programming timings right after the static register value table
> has been sent to the sensor in the ov5640_load_regs() function.
> 
> Fixes: 476dec012f4c ("media: ov5640: Add horizontal and vertical totals")
> Signed-off-by: Samuel Bobrowicz <sam@elite-embedded.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> ---
> This fix has been circulating around for quite some time now, in Maxime clock
> tree patches, in Sam dropbox patches and in my latest MIPI fixes patches.
> While the rest of the series have not yet been accepted, there is general
> consensus this is an actual fix that has to be collected.
> 
> I've slightly modified Sam's and Maxime's version I previously sent,
> programming timings directly in ov5640_load_regs() function.
> You can find Sam's previous version here:
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg131654.html
> and mine here, with an additional change that aimed to fix MIPI mode, which
> I've left out in this version:
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg133422.html
> 
> Sam, Maxime, I took the liberty of taking your Signed-off-by from the previous
> patch, as this was spotted by you first. Is this ok with you?

Yep, thanks!
Maxime

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

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

Re: [PATCH 1/2] media: ov5640: Fix timings setup code

[Hugues FRUCHET]

Hi Jacopo,

Thanks for this patch, when testing on my side I don't see special 
regression or enhancement with that fix, particularly on image quality 
(exposure, ...),
do you have a test procedure that underline the issue ?
For example on my side when I do 5Mp then QVGA capture, pictures are 
overexposed/greenish:
v4l2-ctl --set-parm=15; v4l2-ctl 
--set-fmt-video=width=2592,height=1944,pixelformat=JPEG --stream-mmap 
--stream-count=1 --stream-to=pic-5Mp.jpeg; v4l2-ctl 
--set-parm=30;weston-image pic-5Mp.jpeg &
v4l2-ctl --set-parm=15; v4l2-ctl 
--set-fmt-video=width=320,height=240,pixelformat=JPEG --stream-mmap 
--stream-count=1 --stream-to=pic-QVGA.jpeg; v4l2-ctl 
--set-parm=30;weston-image pic-QVGA.jpeg &

If I skip the first frames, images captured are OK:

v4l2-ctl --set-parm=15; v4l2-ctl 
--set-fmt-video=width=2592,height=1944,pixelformat=JPEG --stream-mmap 
--stream-count=1 --stream-skip=1 --stream-to=pic-5Mp.jpeg; v4l2-ctl 
--set-parm=30;weston-image pic-5Mp.jpeg &
v4l2-ctl --set-parm=15; v4l2-ctl 
--set-fmt-video=width=320,height=240,pixelformat=JPEG --stream-mmap 
--stream-count=1 --stream-skip=1 --stream-to=pic-QVGA.jpeg; v4l2-ctl 
--set-parm=30;weston-image pic-QVGA.jpeg &


Best regards,
Hugues.

On 07/18/2018 01:19 PM, Jacopo Mondi wrote:
> As of:
> commit 476dec012f4c ("media: ov5640: Add horizontal and vertical totals")
> the timings parameters gets programmed separately from the static register
> values array.
> 
> When changing capture mode, the vertical and horizontal totals gets inspected
> by the set_mode_exposure_calc() functions, and only later programmed with the
> new values. This means exposure, light banding filter and shutter gain are
> calculated using the previous timings, and are thus not correct.
> 
> Fix this by programming timings right after the static register value table
> has been sent to the sensor in the ov5640_load_regs() function.
> 
> Fixes: 476dec012f4c ("media: ov5640: Add horizontal and vertical totals")
> Signed-off-by: Samuel Bobrowicz <sam@elite-embedded.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> ---
> This fix has been circulating around for quite some time now, in Maxime clock
> tree patches, in Sam dropbox patches and in my latest MIPI fixes patches.
> While the rest of the series have not yet been accepted, there is general
> consensus this is an actual fix that has to be collected.
> 
> I've slightly modified Sam's and Maxime's version I previously sent,
> programming timings directly in ov5640_load_regs() function.
> You can find Sam's previous version here:
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg131654.html
> and mine here, with an additional change that aimed to fix MIPI mode, which
> I've left out in this version:
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg133422.html
> 
> Sam, Maxime, I took the liberty of taking your Signed-off-by from the previous
> patch, as this was spotted by you first. Is this ok with you?
> 
> Thanks
>     j
> ---
> ---
>   drivers/media/i2c/ov5640.c | 50 +++++++++++++++++++---------------------------
>   1 file changed, 21 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 1ecbb7a..12b3496 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -908,6 +908,26 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
>   }
>   
>   /* download ov5640 settings to sensor through i2c */
> +static int ov5640_set_timings(struct ov5640_dev *sensor,
> +			      const struct ov5640_mode_info *mode)
> +{
> +	int ret;
> +
> +	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPHO, mode->hact);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPVO, mode->vact);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_HTS, mode->htot);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ov5640_write_reg16(sensor, OV5640_REG_TIMING_VTS, mode->vtot);
> +}
> +
>   static int ov5640_load_regs(struct ov5640_dev *sensor,
>   			    const struct ov5640_mode_info *mode)
>   {
> @@ -935,7 +955,7 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
>   			usleep_range(1000 * delay_ms, 1000 * delay_ms + 100);
>   	}
>   
> -	return ret;
> +	return ov5640_set_timings(sensor, mode);
>   }
>   
>   /* read exposure, in number of line periods */
> @@ -1385,30 +1405,6 @@ static int ov5640_set_virtual_channel(struct ov5640_dev *sensor)
>   	return ov5640_write_reg(sensor, OV5640_REG_DEBUG_MODE, temp);
>   }
>   
> -static int ov5640_set_timings(struct ov5640_dev *sensor,
> -			      const struct ov5640_mode_info *mode)
> -{
> -	int ret;
> -
> -	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPHO, mode->hact);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPVO, mode->vact);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_HTS, mode->htot);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_VTS, mode->vtot);
> -	if (ret < 0)
> -		return ret;
> -
> -	return 0;
> -}
> -
>   static const struct ov5640_mode_info *
>   ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
>   		 int width, int height, bool nearest)
> @@ -1652,10 +1648,6 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
>   	if (ret < 0)
>   		return ret;
>   
> -	ret = ov5640_set_timings(sensor, mode);
> -	if (ret < 0)
> -		return ret;
> -
>   	ret = ov5640_set_binning(sensor, dn_mode != SCALING);
>   	if (ret < 0)
>   		return ret;
> 

Re: [PATCH 1/2] media: ov5640: Fix timings setup code

[jacopo mondi]

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

Hi Hugues,
   I'll reply to your v2 series shortly, but let me point out one
thing before.

+steve

On Tue, Aug 07, 2018 at 08:53:53AM +0000, Hugues FRUCHET wrote:
> Hi Jacopo,
>
> Thanks for this patch, when testing on my side I don't see special
> regression or enhancement with that fix, particularly on image quality
> (exposure, ...),

On imx.6 platforms I need this patch along with the MIPI interface
setup fixes sent here [1] to have capture working.

I messed up a bit, as I've sent the "timings fixes" in two series,
this one I'm replying to and the one I've pasted the link of, even if
those series had two different purposes.

So, this patch in my setup does not fixes an image quality issues, but
instead takes part in solving a problems with MIPI CSI-2 on imx.6
platforms.

I asked Steve Longerbeam to test on his imx.6 platform and he reported
his issues was not fixed and he got blank frames ( I was very
disappointed by the different results we had, but moved on and kept
carrying those patches in my tree, as otherwise MIPI interface failed
to startup).

Now I have asked Steve if he might have blank frames, which might be
an exposure related issue, I never had as I skept the first frames,
which are usually the blank one (you're cc-ed).

I believe too your exposure+gain series should be applied and
should supersed [2/2] of this series, but this timing fix is necessary
for me, and I hope Steve's problem are related to exposure handling
issues your series might fix as it did for me.

> do you have a test procedure that underline the issue ?
> For example on my side when I do 5Mp then QVGA capture, pictures are
> overexposed/greenish:

I do capture with yavta, I usually skept the first 7 frames and then
captured 3. Without exposure fix (my patch or your series) the first
frames are black.

This driver is making our life miserable, isn't it? :)

Thanks
   j

[1] https://www.spinics.net/lists/linux-media/msg137557.html

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

[PATCH 2/2] media: ov5640: Fix auto-exposure disabling

[Jacopo Mondi]

As of:
commit bf4a4b518c20 ("media: ov5640: Don't force the auto exposure state at
start time") auto-exposure got disabled before programming new capture modes to
the sensor. Unfortunately the function used to do that (ov5640_set_exposure())
does not enable/disable auto-exposure engine through register 0x3503[0] bit, but
programs registers [0x3500 - 0x3502] which represent the desired exposure time
when running with manual exposure. As a result, auto-exposure was not actually
disabled at all.

To actually disable auto-exposure, go through the control framework instead of
calling ov5640_set_exposure() function directly.

Also, as auto-gain and auto-exposure are disabled un-conditionally but only
restored to their previous values in ov5640_set_mode_direct() function, move
controls restoring so that their value is re-programmed opportunely after
either ov5640_set_mode_direct() or ov5640_set_mode_exposure_calc() have been
executed.

Fixes: bf4a4b518c20 ("media: ov5640: Don't force the auto exposure state at start time")
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

---
Is it worth doing with auto-gain what we're doing with auto-exposure? Cache the
value and then re-program it instead of unconditionally disable/enable it?

Thanks
  j
---
---
 drivers/media/i2c/ov5640.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 12b3496..bc75cb7 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1588,25 +1588,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,
-				  s32 exposure)
+				  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, exposure);
+	return  ov5640_load_regs(sensor, mode);
 }
 
 static int ov5640_set_mode(struct ov5640_dev *sensor,
@@ -1626,7 +1614,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 		return ret;
 
 	exposure = sensor->ctrls.auto_exp->val;
-	ret = ov5640_set_exposure(sensor, V4L2_EXPOSURE_MANUAL);
+	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, V4L2_EXPOSURE_MANUAL);
 	if (ret)
 		return ret;
 
@@ -1642,12 +1630,21 @@ 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);
 	}
 
 	if (ret < 0)
 		return ret;
 
+	/* Restore auto-gain and auto-exposure after mode has changed. */
+	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
+	if (ret)
+		return ret;
+
+	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, exposure)
+	if (ret)
+		return ret;
+
 	ret = ov5640_set_binning(sensor, dn_mode != SCALING);
 	if (ret < 0)
 		return ret;
-- 
2.7.4

Re: [PATCH 2/2] media: ov5640: Fix auto-exposure disabling

[jacopo mondi]

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

Hi again,

On Wed, Jul 18, 2018 at 01:19:03PM +0200, Jacopo Mondi wrote:
> As of:
> commit bf4a4b518c20 ("media: ov5640: Don't force the auto exposure state at
> start time") auto-exposure got disabled before programming new capture modes to
> the sensor. Unfortunately the function used to do that (ov5640_set_exposure())
> does not enable/disable auto-exposure engine through register 0x3503[0] bit, but
> programs registers [0x3500 - 0x3502] which represent the desired exposure time
> when running with manual exposure. As a result, auto-exposure was not actually
> disabled at all.
>
> To actually disable auto-exposure, go through the control framework instead of
> calling ov5640_set_exposure() function directly.
>
> Also, as auto-gain and auto-exposure are disabled un-conditionally but only
> restored to their previous values in ov5640_set_mode_direct() function, move
> controls restoring so that their value is re-programmed opportunely after
> either ov5640_set_mode_direct() or ov5640_set_mode_exposure_calc() have been
> executed.
>
> Fixes: bf4a4b518c20 ("media: ov5640: Don't force the auto exposure state at start time")
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>
> ---
> Is it worth doing with auto-gain what we're doing with auto-exposure? Cache the
> value and then re-program it instead of unconditionally disable/enable it?

I have missed this patch from Hugues that address almost the same
issue
https://www.mail-archive.com/linux-media@vger.kernel.org/msg133264.html

I feel this new one is simpler, and unless we want to avoid going
through the control framework, it is not worth adding new functions to
handle auto-exposure as Hugues' patch is doing.

Hugues, do you have comments? Feel free to add your sob or rb tags if
you like to.

Thanks
   j

>
> Thanks
>   j
> ---
> ---
>  drivers/media/i2c/ov5640.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 12b3496..bc75cb7 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -1588,25 +1588,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,
> -				  s32 exposure)
> +				  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, exposure);
> +	return  ov5640_load_regs(sensor, mode);
>  }
>
>  static int ov5640_set_mode(struct ov5640_dev *sensor,
> @@ -1626,7 +1614,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
>  		return ret;
>
>  	exposure = sensor->ctrls.auto_exp->val;
> -	ret = ov5640_set_exposure(sensor, V4L2_EXPOSURE_MANUAL);
> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, V4L2_EXPOSURE_MANUAL);
>  	if (ret)
>  		return ret;
>
> @@ -1642,12 +1630,21 @@ 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);
>  	}
>
>  	if (ret < 0)
>  		return ret;
>
> +	/* Restore auto-gain and auto-exposure after mode has changed. */
> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
> +	if (ret)
> +		return ret;
> +
> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, exposure)
> +	if (ret)
> +		return ret;
> +
>  	ret = ov5640_set_binning(sensor, dn_mode != SCALING);
>  	if (ret < 0)
>  		return ret;
> --
> 2.7.4
>

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

Re: [PATCH 2/2] media: ov5640: Fix auto-exposure disabling

[Hugues FRUCHET]

Hi Jacopo,

In serie "[PATCH 0/5] Fix OV5640 exposure & gain"
https://www.mail-archive.com/linux-media@vger.kernel.org/msg133269.html
I've tried to collect fixes around exposure/gain, not only the exposure 
regression and I would prefer to keep it consistent with the associated 
procedure test.
Moreover I dislike the internal use of control framework functions to 
disable/enable exposure/gain, on my opinion this has to be kept simpler
by just disabling/enabling the right registers.
Would it be possible that you test my 5 patches serie on your side ?

Best regards,
Hugues.

On 07/18/2018 03:04 PM, jacopo mondi wrote:
> Hi again,
> 
> On Wed, Jul 18, 2018 at 01:19:03PM +0200, Jacopo Mondi wrote:
>> As of:
>> commit bf4a4b518c20 ("media: ov5640: Don't force the auto exposure state at
>> start time") auto-exposure got disabled before programming new capture modes to
>> the sensor. Unfortunately the function used to do that (ov5640_set_exposure())
>> does not enable/disable auto-exposure engine through register 0x3503[0] bit, but
>> programs registers [0x3500 - 0x3502] which represent the desired exposure time
>> when running with manual exposure. As a result, auto-exposure was not actually
>> disabled at all.
>>
>> To actually disable auto-exposure, go through the control framework instead of
>> calling ov5640_set_exposure() function directly.
>>
>> Also, as auto-gain and auto-exposure are disabled un-conditionally but only
>> restored to their previous values in ov5640_set_mode_direct() function, move
>> controls restoring so that their value is re-programmed opportunely after
>> either ov5640_set_mode_direct() or ov5640_set_mode_exposure_calc() have been
>> executed.
>>
>> Fixes: bf4a4b518c20 ("media: ov5640: Don't force the auto exposure state at start time")
>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>
>> ---
>> Is it worth doing with auto-gain what we're doing with auto-exposure? Cache the
>> value and then re-program it instead of unconditionally disable/enable it?
> 
> I have missed this patch from Hugues that address almost the same
> issue
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg133264.html
> 
> I feel this new one is simpler, and unless we want to avoid going
> through the control framework, it is not worth adding new functions to
> handle auto-exposure as Hugues' patch is doing.
> 
> Hugues, do you have comments? Feel free to add your sob or rb tags if
> you like to.
> 
> Thanks
>     j
> 
>>
>> Thanks
>>    j
>> ---
>> ---
>>   drivers/media/i2c/ov5640.c | 29 +++++++++++++----------------
>>   1 file changed, 13 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>> index 12b3496..bc75cb7 100644
>> --- a/drivers/media/i2c/ov5640.c
>> +++ b/drivers/media/i2c/ov5640.c
>> @@ -1588,25 +1588,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,
>> -				  s32 exposure)
>> +				  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, exposure);
>> +	return  ov5640_load_regs(sensor, mode);
>>   }
>>
>>   static int ov5640_set_mode(struct ov5640_dev *sensor,
>> @@ -1626,7 +1614,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
>>   		return ret;
>>
>>   	exposure = sensor->ctrls.auto_exp->val;
>> -	ret = ov5640_set_exposure(sensor, V4L2_EXPOSURE_MANUAL);
>> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, V4L2_EXPOSURE_MANUAL);
>>   	if (ret)
>>   		return ret;
>>
>> @@ -1642,12 +1630,21 @@ 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);
>>   	}
>>
>>   	if (ret < 0)
>>   		return ret;
>>
>> +	/* Restore auto-gain and auto-exposure after mode has changed. */
>> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, exposure)
>> +	if (ret)
>> +		return ret;
>> +
>>   	ret = ov5640_set_binning(sensor, dn_mode != SCALING);
>>   	if (ret < 0)
>>   		return ret;
>> --
>> 2.7.4
>>

Re: [PATCH 2/2] media: ov5640: Fix auto-exposure disabling

[jacopo mondi]

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

Hi Hugues,

On Tue, Aug 07, 2018 at 08:53:23AM +0000, Hugues FRUCHET wrote:
> Hi Jacopo,
>
> In serie "[PATCH 0/5] Fix OV5640 exposure & gain"
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg133269.html
> I've tried to collect fixes around exposure/gain, not only the exposure
> regression and I would prefer to keep it consistent with the associated
> procedure test.

You're right. Please see my other reply, I mixed two different issues
in this series probably.

> Moreover I dislike the internal use of control framework functions to
> disable/enable exposure/gain, on my opinion this has to be kept simpler
> by just disabling/enabling the right registers.

Why that? I thought changing parameters exposed as controls should go
through the control framework to ensure consistency. Maybe I'm wrong.

> Would it be possible that you test my 5 patches serie on your side ?

I did. I re-based the series on top of my MIPI and timings fixes and
it actually solves the exposure issues I didn't know I had :)

I'll comment on v2 as well as soon as I'll get an answer from Steve on
the CSI-2 issue.

Thanks
   j

>
> Best regards,
> Hugues.
>
> On 07/18/2018 03:04 PM, jacopo mondi wrote:
> > Hi again,
> >
> > On Wed, Jul 18, 2018 at 01:19:03PM +0200, Jacopo Mondi wrote:
> >> As of:
> >> commit bf4a4b518c20 ("media: ov5640: Don't force the auto exposure state at
> >> start time") auto-exposure got disabled before programming new capture modes to
> >> the sensor. Unfortunately the function used to do that (ov5640_set_exposure())
> >> does not enable/disable auto-exposure engine through register 0x3503[0] bit, but
> >> programs registers [0x3500 - 0x3502] which represent the desired exposure time
> >> when running with manual exposure. As a result, auto-exposure was not actually
> >> disabled at all.
> >>
> >> To actually disable auto-exposure, go through the control framework instead of
> >> calling ov5640_set_exposure() function directly.
> >>
> >> Also, as auto-gain and auto-exposure are disabled un-conditionally but only
> >> restored to their previous values in ov5640_set_mode_direct() function, move
> >> controls restoring so that their value is re-programmed opportunely after
> >> either ov5640_set_mode_direct() or ov5640_set_mode_exposure_calc() have been
> >> executed.
> >>
> >> Fixes: bf4a4b518c20 ("media: ov5640: Don't force the auto exposure state at start time")
> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>
> >> ---
> >> Is it worth doing with auto-gain what we're doing with auto-exposure? Cache the
> >> value and then re-program it instead of unconditionally disable/enable it?
> >
> > I have missed this patch from Hugues that address almost the same
> > issue
> > https://www.mail-archive.com/linux-media@vger.kernel.org/msg133264.html
> >
> > I feel this new one is simpler, and unless we want to avoid going
> > through the control framework, it is not worth adding new functions to
> > handle auto-exposure as Hugues' patch is doing.
> >
> > Hugues, do you have comments? Feel free to add your sob or rb tags if
> > you like to.
> >
> > Thanks
> >     j
> >
> >>
> >> Thanks
> >>    j
> >> ---
> >> ---
> >>   drivers/media/i2c/ov5640.c | 29 +++++++++++++----------------
> >>   1 file changed, 13 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> >> index 12b3496..bc75cb7 100644
> >> --- a/drivers/media/i2c/ov5640.c
> >> +++ b/drivers/media/i2c/ov5640.c
> >> @@ -1588,25 +1588,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,
> >> -				  s32 exposure)
> >> +				  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, exposure);
> >> +	return  ov5640_load_regs(sensor, mode);
> >>   }
> >>
> >>   static int ov5640_set_mode(struct ov5640_dev *sensor,
> >> @@ -1626,7 +1614,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
> >>   		return ret;
> >>
> >>   	exposure = sensor->ctrls.auto_exp->val;
> >> -	ret = ov5640_set_exposure(sensor, V4L2_EXPOSURE_MANUAL);
> >> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, V4L2_EXPOSURE_MANUAL);
> >>   	if (ret)
> >>   		return ret;
> >>
> >> @@ -1642,12 +1630,21 @@ 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);
> >>   	}
> >>
> >>   	if (ret < 0)
> >>   		return ret;
> >>
> >> +	/* Restore auto-gain and auto-exposure after mode has changed. */
> >> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, exposure)
> >> +	if (ret)
> >> +		return ret;
> >> +
> >>   	ret = ov5640_set_binning(sensor, dn_mode != SCALING);
> >>   	if (ret < 0)
> >>   		return ret;
> >> --
> >> 2.7.4
> >>

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

Re: [PATCH 2/2] media: ov5640: Fix auto-exposure disabling

[Laurent Pinchart]

Hello,

On Tuesday, 14 August 2018 18:45:25 EEST jacopo mondi wrote:
> On Tue, Aug 07, 2018 at 08:53:23AM +0000, Hugues FRUCHET wrote:
> > Hi Jacopo,
> > 
> > In serie "[PATCH 0/5] Fix OV5640 exposure & gain"
> > https://www.mail-archive.com/linux-media@vger.kernel.org/msg133269.html
> > I've tried to collect fixes around exposure/gain, not only the exposure
> > regression and I would prefer to keep it consistent with the associated
> > procedure test.
> 
> You're right. Please see my other reply, I mixed two different issues
> in this series probably.
> 
> > Moreover I dislike the internal use of control framework functions to
> > disable/enable exposure/gain, on my opinion this has to be kept simpler
> > by just disabling/enabling the right registers.
> 
> Why that? I thought changing parameters exposed as controls should go
> through the control framework to ensure consistency. Maybe I'm wrong.

If I understand the driver correctly, auto-exposure has to be disabled 
temporarily when changing format and size, due to internal hardware 
requirements. The sequence should more or less be

 1. Disable auto-exposure
 2. Configure the format and size
 3. Restore auto-exposure

This sequence is internal to the driver, and should thus not be visible to 
userspace. Going through the control framework to disable and restore auto-
exposure would generate control events that would just confuse userspace. For 
that reason I'd keep all this internal with direct register access instead of 
going through the control framework.

> > Would it be possible that you test my 5 patches serie on your side ?
> 
> I did. I re-based the series on top of my MIPI and timings fixes and
> it actually solves the exposure issues I didn't know I had :)
> 
> I'll comment on v2 as well as soon as I'll get an answer from Steve on
> the CSI-2 issue.
> 
> > On 07/18/2018 03:04 PM, jacopo mondi wrote:
> > > Hi again,
> > > 
> > > On Wed, Jul 18, 2018 at 01:19:03PM +0200, Jacopo Mondi wrote:
> > >> As of:
> > >> commit bf4a4b518c20 ("media: ov5640: Don't force the auto exposure
> > >> state at
> > >> start time") auto-exposure got disabled before programming new capture
> > >> modes to the sensor. Unfortunately the function used to do that
> > >> (ov5640_set_exposure()) does not enable/disable auto-exposure engine
> > >> through register 0x3503[0] bit, but programs registers [0x3500 -
> > >> 0x3502] which represent the desired exposure time when running with
> > >> manual exposure. As a result, auto-exposure was not actually disabled
> > >> at all.
> > >> 
> > >> To actually disable auto-exposure, go through the control framework
> > >> instead of calling ov5640_set_exposure() function directly.
> > >> 
> > >> Also, as auto-gain and auto-exposure are disabled un-conditionally but
> > >> only
> > >> restored to their previous values in ov5640_set_mode_direct() function,
> > >> move controls restoring so that their value is re-programmed
> > >> opportunely after either ov5640_set_mode_direct() or
> > >> ov5640_set_mode_exposure_calc() have been executed.
> > >> 
> > >> Fixes: bf4a4b518c20 ("media: ov5640: Don't force the auto exposure
> > >> state at start time") Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >> 
> > >> ---
> > >> Is it worth doing with auto-gain what we're doing with auto-exposure?
> > >> Cache the value and then re-program it instead of unconditionally
> > >> disable/enable it?> > 
> > > I have missed this patch from Hugues that address almost the same
> > > issue
> > > https://www.mail-archive.com/linux-media@vger.kernel.org/msg133264.html
> > > 
> > > I feel this new one is simpler, and unless we want to avoid going
> > > through the control framework, it is not worth adding new functions to
> > > handle auto-exposure as Hugues' patch is doing.
> > > 
> > > Hugues, do you have comments? Feel free to add your sob or rb tags if
> > > you like to.
> > > 
> > > Thanks
> > > 
> > >     j
> > >> 
> > >> Thanks
> > >> 
> > >>    j
> > >> 
> > >> ---
> > >> ---
> > >> 
> > >>   drivers/media/i2c/ov5640.c | 29 +++++++++++++----------------
> > >>   1 file changed, 13 insertions(+), 16 deletions(-)
> > >> 
> > >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > >> index 12b3496..bc75cb7 100644
> > >> --- a/drivers/media/i2c/ov5640.c
> > >> +++ b/drivers/media/i2c/ov5640.c
> > >> @@ -1588,25 +1588,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,
> > >> -				  s32 exposure)
> > >> +				  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, exposure);
> > >> +	return  ov5640_load_regs(sensor, mode);
> > >> 
> > >>   }
> > >>   
> > >>   static int ov5640_set_mode(struct ov5640_dev *sensor,
> > >> 
> > >> @@ -1626,7 +1614,7 @@ static int ov5640_set_mode(struct ov5640_dev
> > >> *sensor,
> > >> 
> > >>   		return ret;
> > >>   	
> > >>   	exposure = sensor->ctrls.auto_exp->val;
> > >> 
> > >> -	ret = ov5640_set_exposure(sensor, V4L2_EXPOSURE_MANUAL);
> > >> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp,
> > >> V4L2_EXPOSURE_MANUAL);
> > >> 
> > >>   	if (ret)
> > >>   	
> > >>   		return ret;
> > >> 
> > >> @@ -1642,12 +1630,21 @@ 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);
> > >> 
> > >>   	}
> > >>   	
> > >>   	if (ret < 0)
> > >>   	
> > >>   		return ret;
> > >> 
> > >> +	/* Restore auto-gain and auto-exposure after mode has changed. */
> > >> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
> > >> +	if (ret)
> > >> +		return ret;
> > >> +
> > >> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, exposure)
> > >> +	if (ret)
> > >> +		return ret;
> > >> +
> > >> 
> > >>   	ret = ov5640_set_binning(sensor, dn_mode != SCALING);
> > >>   	if (ret < 0)
> > >>   	
> > >>   		return ret;
> > >> 
> > >> --
> > >> 2.7.4


-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: ov5640: Fix auto-exposure disabling

[jacopo mondi]

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

HI Laurent,

On Thu, Sep 06, 2018 at 08:08:50PM +0300, Laurent Pinchart wrote:
> Hello,
>
> On Tuesday, 14 August 2018 18:45:25 EEST jacopo mondi wrote:
> > On Tue, Aug 07, 2018 at 08:53:23AM +0000, Hugues FRUCHET wrote:
> > > Hi Jacopo,
> > >
> > > In serie "[PATCH 0/5] Fix OV5640 exposure & gain"
> > > https://www.mail-archive.com/linux-media@vger.kernel.org/msg133269.html
> > > I've tried to collect fixes around exposure/gain, not only the exposure
> > > regression and I would prefer to keep it consistent with the associated
> > > procedure test.
> >
> > You're right. Please see my other reply, I mixed two different issues
> > in this series probably.
> >
> > > Moreover I dislike the internal use of control framework functions to
> > > disable/enable exposure/gain, on my opinion this has to be kept simpler
> > > by just disabling/enabling the right registers.
> >
> > Why that? I thought changing parameters exposed as controls should go
> > through the control framework to ensure consistency. Maybe I'm wrong.
>
> If I understand the driver correctly, auto-exposure has to be disabled
> temporarily when changing format and size, due to internal hardware
> requirements. The sequence should more or less be
>
>  1. Disable auto-exposure
>  2. Configure the format and size
>  3. Restore auto-exposure
>
> This sequence is internal to the driver, and should thus not be visible to
> userspace. Going through the control framework to disable and restore auto-
> exposure would generate control events that would just confuse userspace. For
> that reason I'd keep all this internal with direct register access instead of
> going through the control framework.

Thanks for the clarification.

Please note this series is superseded by Hugues' exposure and gain
fixes one, and my MIPI CSI-2 startup one (as it includes the timings
fix also sent there).

Thanks
   j
>
> > > Would it be possible that you test my 5 patches serie on your side ?
> >
> > I did. I re-based the series on top of my MIPI and timings fixes and
> > it actually solves the exposure issues I didn't know I had :)
> >
> > I'll comment on v2 as well as soon as I'll get an answer from Steve on
> > the CSI-2 issue.
> >
> > > On 07/18/2018 03:04 PM, jacopo mondi wrote:
> > > > Hi again,
> > > >
> > > > On Wed, Jul 18, 2018 at 01:19:03PM +0200, Jacopo Mondi wrote:
> > > >> As of:
> > > >> commit bf4a4b518c20 ("media: ov5640: Don't force the auto exposure
> > > >> state at
> > > >> start time") auto-exposure got disabled before programming new capture
> > > >> modes to the sensor. Unfortunately the function used to do that
> > > >> (ov5640_set_exposure()) does not enable/disable auto-exposure engine
> > > >> through register 0x3503[0] bit, but programs registers [0x3500 -
> > > >> 0x3502] which represent the desired exposure time when running with
> > > >> manual exposure. As a result, auto-exposure was not actually disabled
> > > >> at all.
> > > >>
> > > >> To actually disable auto-exposure, go through the control framework
> > > >> instead of calling ov5640_set_exposure() function directly.
> > > >>
> > > >> Also, as auto-gain and auto-exposure are disabled un-conditionally but
> > > >> only
> > > >> restored to their previous values in ov5640_set_mode_direct() function,
> > > >> move controls restoring so that their value is re-programmed
> > > >> opportunely after either ov5640_set_mode_direct() or
> > > >> ov5640_set_mode_exposure_calc() have been executed.
> > > >>
> > > >> Fixes: bf4a4b518c20 ("media: ov5640: Don't force the auto exposure
> > > >> state at start time") Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > >>
> > > >> ---
> > > >> Is it worth doing with auto-gain what we're doing with auto-exposure?
> > > >> Cache the value and then re-program it instead of unconditionally
> > > >> disable/enable it?> >
> > > > I have missed this patch from Hugues that address almost the same
> > > > issue
> > > > https://www.mail-archive.com/linux-media@vger.kernel.org/msg133264.html
> > > >
> > > > I feel this new one is simpler, and unless we want to avoid going
> > > > through the control framework, it is not worth adding new functions to
> > > > handle auto-exposure as Hugues' patch is doing.
> > > >
> > > > Hugues, do you have comments? Feel free to add your sob or rb tags if
> > > > you like to.
> > > >
> > > > Thanks
> > > >
> > > >     j
> > > >>
> > > >> Thanks
> > > >>
> > > >>    j
> > > >>
> > > >> ---
> > > >> ---
> > > >>
> > > >>   drivers/media/i2c/ov5640.c | 29 +++++++++++++----------------
> > > >>   1 file changed, 13 insertions(+), 16 deletions(-)
> > > >>
> > > >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > > >> index 12b3496..bc75cb7 100644
> > > >> --- a/drivers/media/i2c/ov5640.c
> > > >> +++ b/drivers/media/i2c/ov5640.c
> > > >> @@ -1588,25 +1588,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,
> > > >> -				  s32 exposure)
> > > >> +				  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, exposure);
> > > >> +	return  ov5640_load_regs(sensor, mode);
> > > >>
> > > >>   }
> > > >>
> > > >>   static int ov5640_set_mode(struct ov5640_dev *sensor,
> > > >>
> > > >> @@ -1626,7 +1614,7 @@ static int ov5640_set_mode(struct ov5640_dev
> > > >> *sensor,
> > > >>
> > > >>   		return ret;
> > > >>
> > > >>   	exposure = sensor->ctrls.auto_exp->val;
> > > >>
> > > >> -	ret = ov5640_set_exposure(sensor, V4L2_EXPOSURE_MANUAL);
> > > >> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp,
> > > >> V4L2_EXPOSURE_MANUAL);
> > > >>
> > > >>   	if (ret)
> > > >>
> > > >>   		return ret;
> > > >>
> > > >> @@ -1642,12 +1630,21 @@ 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);
> > > >>
> > > >>   	}
> > > >>
> > > >>   	if (ret < 0)
> > > >>
> > > >>   		return ret;
> > > >>
> > > >> +	/* Restore auto-gain and auto-exposure after mode has changed. */
> > > >> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
> > > >> +	if (ret)
> > > >> +		return ret;
> > > >> +
> > > >> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, exposure)
> > > >> +	if (ret)
> > > >> +		return ret;
> > > >> +
> > > >>
> > > >>   	ret = ov5640_set_binning(sensor, dn_mode != SCALING);
> > > >>   	if (ret < 0)
> > > >>
> > > >>   		return ret;
> > > >>
> > > >> --
> > > >> 2.7.4
>
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

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