| Message ID | 1530709123-12445-4-git-send-email-hugues.fruchet@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Hugues, On Wed, Jul 04, 2018 at 02:58:41PM +0200, Hugues Fruchet wrote: > ov5640_set_mode_exposure_calc() is checking binning value but > binning value read is buggy and binning value set is done > after calling ov5640_set_mode_exposure_calc(), fix all of this. The ov5640_binning_on() function was indeed wrong (side note: that name is confusing, it should be 0v5640_get_binning() to comply with others..) and always returned 0, but I don't see a fix here for the second part of the issue. In facts, during the lenghty exposure calculation process, binning is checked to decide if the preview shutter time should be doubled or not static int ov5640_set_mode_exposure_calc(struct ov5640_dev *sensor, const struct ov5640_mode_info *mode) { ... /* read preview shutter */ ret = ov5640_get_exposure(sensor); if (ret < 0) return ret; prev_shutter = ret; ret = ov5640_binning_on(sensor); if (ret < 0) return ret; if (ret && mode->id != OV5640_MODE_720P_1280_720 && mode->id != OV5640_MODE_1080P_1920_1080) prev_shutter *= 2; ... } My understanding is that reading the value from the register returns the binning settings for the previously configured mode, while the binning value is later updated for the current mode in ov5640_set_mode(), after 'ov5640_set_mode_exposure_calc()' has already been called. Is this ok? Also, I assume the code checks for mode->id to figure out if the mode uses subsampling or scaling. Be aware that for 1280x720 mode, the selected scaling mode depends on the FPS, not only on the mode id as it is assumed here. A final note, the 'ov5640_set_mode_exposure_calc()' also writes VTS to update the shutter time to the newly calculated value. /* write capture shutter */ if (cap_shutter > (cap_vts - 4)) { cap_vts = cap_shutter + 4; ret = ov5640_set_vts(sensor, cap_vts); if (ret < 0) return ret; } Be aware again that VTS is later restored to the mode->vtot value by the 'ov5640_set_timings()' functions, which again, is called later than 'ov5640_set_mode_exposure_calc()'. Wouldn't it be better to postpone exposure calculation after timings and binnings have been set? Thanks j > > Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> > --- > drivers/media/i2c/ov5640.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 7c569de..f9b256e 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -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) > -- > 1.9.1 >
Hi Jacopo, Many thanks for you valuable comments, I hardly understand this exposure code, and still some wrongly exposed images are observed switching from subsampling to scaling modes. Steve, do you have more insight to share with us on this code ? On 07/04/2018 04:38 PM, jacopo mondi wrote: > Hi Hugues, > > On Wed, Jul 04, 2018 at 02:58:41PM +0200, Hugues Fruchet wrote: >> ov5640_set_mode_exposure_calc() is checking binning value but >> binning value read is buggy and binning value set is done >> after calling ov5640_set_mode_exposure_calc(), fix all of this. > > The ov5640_binning_on() function was indeed wrong (side note: that > name is confusing, it should be 0v5640_get_binning() to comply with > others..) and always returned 0, but I don't see a fix here for the > second part of the issue. Mistake from me here, I should have removed "and binning value set is done after calling ov5640_set_mode_exposure_calc()" in commit message. > In facts, during the lenghty exposure > calculation process, binning is checked to decide if the preview > shutter time should be doubled or not > > static int ov5640_set_mode_exposure_calc(struct ov5640_dev *sensor, > const struct ov5640_mode_info *mode) > { > ... > > /* read preview shutter */ > ret = ov5640_get_exposure(sensor); > if (ret < 0) > return ret; > prev_shutter = ret; > ret = ov5640_binning_on(sensor); > if (ret < 0) > return ret; > if (ret && mode->id != OV5640_MODE_720P_1280_720 && > mode->id != OV5640_MODE_1080P_1920_1080) > prev_shutter *= 2; > ... > } > > My understanding is that reading the value from the register returns > the binning settings for the previously configured mode, while the > binning value is later updated for the current mode in > ov5640_set_mode(), after 'ov5640_set_mode_exposure_calc()' has already > been called. Is this ok? This is also my understanding. > > Also, I assume the code checks for mode->id to figure out if the mode > uses subsampling or scaling. Be aware that for 1280x720 mode, the > selected scaling mode depends on the FPS, not only on the mode id as > it is assumed here. This is not what I understand from this array: static const struct ov5640_mode_info ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = { [15fps] {OV5640_MODE_720P_1280_720, SUBSAMPLING, 1280, 1892, 720, 740, ov5640_setting_15fps_720P_1280_720, ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)}, [30fps] {OV5640_MODE_720P_1280_720, SUBSAMPLING, 1280, 1892, 720, 740, ov5640_setting_30fps_720P_1280_720, ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)}, => both modes uses subsampling here > > A final note, the 'ov5640_set_mode_exposure_calc()' also writes VTS to > update the shutter time to the newly calculated value. > > /* write capture shutter */ > if (cap_shutter > (cap_vts - 4)) { > cap_vts = cap_shutter + 4; > ret = ov5640_set_vts(sensor, cap_vts); > if (ret < 0) > return ret; > } > > Be aware again that VTS is later restored to the mode->vtot value by > the 'ov5640_set_timings()' functions, which again, is called later > than 'ov5640_set_mode_exposure_calc()'. > > Wouldn't it be better to postpone exposure calculation after timings > and binnings have been set ? As said, I'm new on all of this but I can give it a try. > > Thanks > j > >> >> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> >> --- >> drivers/media/i2c/ov5640.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >> index 7c569de..f9b256e 100644 >> --- a/drivers/media/i2c/ov5640.c >> +++ b/drivers/media/i2c/ov5640.c >> @@ -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) >> -- >> 1.9.1 >> Best regards, Hugues.
Hi Hugues, On Wed, Jul 04, 2018 at 03:29:56PM +0000, Hugues FRUCHET wrote: > Hi Jacopo, > > Many thanks for you valuable comments, I hardly understand this exposure > code, and still some wrongly exposed images are observed switching from > subsampling to scaling modes. Thank you for the patches... Just out of curiosity, have you ever been able to get images in 1280x720 and 1920x1080 mode? I assume so if you're able to switch between two subsampling modes... > Steve, do you have more insight to share with us on this code ? > > On 07/04/2018 04:38 PM, jacopo mondi wrote: > > Hi Hugues, > > > > On Wed, Jul 04, 2018 at 02:58:41PM +0200, Hugues Fruchet wrote: > >> ov5640_set_mode_exposure_calc() is checking binning value but > >> binning value read is buggy and binning value set is done > >> after calling ov5640_set_mode_exposure_calc(), fix all of this. > > > > The ov5640_binning_on() function was indeed wrong (side note: that > > name is confusing, it should be 0v5640_get_binning() to comply with > > others..) and always returned 0, but I don't see a fix here for the > > second part of the issue. > Mistake from me here, I should have removed "and binning value set is > done after calling ov5640_set_mode_exposure_calc()" in commit message. > > > In facts, during the lenghty exposure > > calculation process, binning is checked to decide if the preview > > shutter time should be doubled or not > > > > static int ov5640_set_mode_exposure_calc(struct ov5640_dev *sensor, > > const struct ov5640_mode_info *mode) > > { > > ... > > > > /* read preview shutter */ > > ret = ov5640_get_exposure(sensor); > > if (ret < 0) > > return ret; > > prev_shutter = ret; > > ret = ov5640_binning_on(sensor); > > if (ret < 0) > > return ret; > > if (ret && mode->id != OV5640_MODE_720P_1280_720 && > > mode->id != OV5640_MODE_1080P_1920_1080) > > prev_shutter *= 2; > > ... > > } > > > > My understanding is that reading the value from the register returns > > the binning settings for the previously configured mode, while the > binning value is later updated for the current mode in > > ov5640_set_mode(), after 'ov5640_set_mode_exposure_calc()' has already > > been called. Is this ok? > > This is also my understanding. > Thanks. This is probably worth fixing. Maybe your exposure issues depend on this.. > > > > Also, I assume the code checks for mode->id to figure out if the mode > > uses subsampling or scaling. Be aware that for 1280x720 mode, the > > selected scaling mode depends on the FPS, not only on the mode id as > > it is assumed here. > > This is not what I understand from this array: > static const struct ov5640_mode_info > ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = { > [15fps] > {OV5640_MODE_720P_1280_720, SUBSAMPLING, > 1280, 1892, 720, 740, > ov5640_setting_15fps_720P_1280_720, > ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)}, > [30fps] > {OV5640_MODE_720P_1280_720, SUBSAMPLING, > 1280, 1892, 720, 740, > ov5640_setting_30fps_720P_1280_720, > ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)}, > > => both modes uses subsampling here You are right, I counted the array entries and 30FPS has a -1 specified as downsizing mode in the last one, so I overlooked it, sorry! So what is mode->id checked for, if 720p and 1080p modes use different downsizing modes? Confused .... > > > > > A final note, the 'ov5640_set_mode_exposure_calc()' also writes VTS to > > update the shutter time to the newly calculated value. > > > > /* write capture shutter */ > > if (cap_shutter > (cap_vts - 4)) { > > cap_vts = cap_shutter + 4; > > ret = ov5640_set_vts(sensor, cap_vts); > > if (ret < 0) > > return ret; > > } > > > > Be aware again that VTS is later restored to the mode->vtot value by > > the 'ov5640_set_timings()' functions, which again, is called later > > than 'ov5640_set_mode_exposure_calc()'. > > > > Wouldn't it be better to postpone exposure calculation after timings > > and binnings have been set ? > > As said, I'm new on all of this but I can give it a try. Thanks, I also see banding filter being calculated twice, and I'm sure there are some other things I'm missing. That exposure calculation procedure seems poorly integrated with the rest of the set_mode() function :/ Thanks j > > > > > Thanks > > j > > > >> > >> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> > >> --- > >> drivers/media/i2c/ov5640.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > >> index 7c569de..f9b256e 100644 > >> --- a/drivers/media/i2c/ov5640.c > >> +++ b/drivers/media/i2c/ov5640.c > >> @@ -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) > >> -- > >> 1.9.1 > >> > > Best regards, > Hugues.
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index 7c569de..f9b256e 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -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)
ov5640_set_mode_exposure_calc() is checking binning value but binning value read is buggy and binning value set is done after calling ov5640_set_mode_exposure_calc(), fix all of this. Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> --- drivers/media/i2c/ov5640.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)