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

From: Hugues FRUCHET <hugues.fruchet@st.com>
To: Jacopo Mondi <jacopo@jmondi.org>,
	"mchehab@kernel.org" <mchehab@kernel.org>,
	"laurent.pinchart@ideasonboard.com"
	<laurent.pinchart@ideasonboard.com>,
	"maxime.ripard@bootlin.com" <maxime.ripard@bootlin.com>,
	"sam@elite-embedded.com" <sam@elite-embedded.com>,
	"jagan@amarulasolutions.com" <jagan@amarulasolutions.com>,
	"festevam@gmail.com" <festevam@gmail.com>,
	"pza@pengutronix.de" <pza@pengutronix.de>,
	"steve_longerbeam@mentor.com" <steve_longerbeam@mentor.com>,
	"loic.poulain@linaro.org" <loic.poulain@linaro.org>,
	"daniel@zonque.org" <daniel@zonque.org>
Cc: "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
	Sakari Ailus <sakari.ailus@iki.fi>
Subject: Re: [PATCH 1/2] media: ov5640: Fix timings setup code
Date: Tue, 7 Aug 2018 08:53:53 +0000	[thread overview]
Message-ID: <f5e1bcf6-a677-441b-eaec-60901811038e@st.com> (raw)
In-Reply-To: <1531912743-24767-2-git-send-email-jacopo@jmondi.org>

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