| Message ID | 1531912743-24767-2-git-send-email-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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; >
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
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;
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(-)