Re: [PATCH 3/5] media: ov5640: fix wrong binning value in exposure calculation
Hi Hugues,
On Wed, Jul 04, 2018 at 03:29:56PM +, 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
> >> ---
> >> 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
Re: [PATCH 3/5] media: ov5640: fix wrong binning value in exposure calculation
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
>> ---
>> 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, );
>> 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.
Re: [PATCH 3/5] media: ov5640: fix wrong binning value in exposure calculation
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
> ---
> 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, );
> 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
>
signature.asc
Description: PGP signature
[PATCH 3/5] media: ov5640: fix wrong binning value in exposure calculation
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 --- 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, ); 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
