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@xxxxxx>
>> ---
>> 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.
