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@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxx>
> Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
>
> ---
> 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@xxxxxxxxxxxxxxx/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@xxxxxxxxxxxxxxx/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;
>
