Re: [PATCH 3/5] media: ov5640: fix wrong binning value in exposure calculation

[jacopo mondi <jacopo@jmondi.org>]

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

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.

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