Re: [PATCH 2/2] media: ov5640: Fix auto-exposure disabling

[jacopo mondi <jacopo@jmondi.org>]

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

Hi Hugues,

On Tue, Aug 07, 2018 at 08:53:23AM +0000, Hugues FRUCHET wrote:
> Hi Jacopo,
>
> In serie "[PATCH 0/5] Fix OV5640 exposure & gain"
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg133269.html
> I've tried to collect fixes around exposure/gain, not only the exposure
> regression and I would prefer to keep it consistent with the associated
> procedure test.

You're right. Please see my other reply, I mixed two different issues
in this series probably.

> Moreover I dislike the internal use of control framework functions to
> disable/enable exposure/gain, on my opinion this has to be kept simpler
> by just disabling/enabling the right registers.

Why that? I thought changing parameters exposed as controls should go
through the control framework to ensure consistency. Maybe I'm wrong.

> Would it be possible that you test my 5 patches serie on your side ?

I did. I re-based the series on top of my MIPI and timings fixes and
it actually solves the exposure issues I didn't know I had :)

I'll comment on v2 as well as soon as I'll get an answer from Steve on
the CSI-2 issue.

Thanks
   j

>
> Best regards,
> Hugues.
>
> On 07/18/2018 03:04 PM, jacopo mondi wrote:
> > Hi again,
> >
> > On Wed, Jul 18, 2018 at 01:19:03PM +0200, Jacopo Mondi wrote:
> >> As of:
> >> commit bf4a4b518c20 ("media: ov5640: Don't force the auto exposure state at
> >> start time") auto-exposure got disabled before programming new capture modes to
> >> the sensor. Unfortunately the function used to do that (ov5640_set_exposure())
> >> does not enable/disable auto-exposure engine through register 0x3503[0] bit, but
> >> programs registers [0x3500 - 0x3502] which represent the desired exposure time
> >> when running with manual exposure. As a result, auto-exposure was not actually
> >> disabled at all.
> >>
> >> To actually disable auto-exposure, go through the control framework instead of
> >> calling ov5640_set_exposure() function directly.
> >>
> >> Also, as auto-gain and auto-exposure are disabled un-conditionally but only
> >> restored to their previous values in ov5640_set_mode_direct() function, move
> >> controls restoring so that their value is re-programmed opportunely after
> >> either ov5640_set_mode_direct() or ov5640_set_mode_exposure_calc() have been
> >> executed.
> >>
> >> Fixes: bf4a4b518c20 ("media: ov5640: Don't force the auto exposure state at start time")
> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>
> >> ---
> >> Is it worth doing with auto-gain what we're doing with auto-exposure? Cache the
> >> value and then re-program it instead of unconditionally disable/enable it?
> >
> > I have missed this patch from Hugues that address almost the same
> > issue
> > https://www.mail-archive.com/linux-media@vger.kernel.org/msg133264.html
> >
> > I feel this new one is simpler, and unless we want to avoid going
> > through the control framework, it is not worth adding new functions to
> > handle auto-exposure as Hugues' patch is doing.
> >
> > Hugues, do you have comments? Feel free to add your sob or rb tags if
> > you like to.
> >
> > Thanks
> >     j
> >
> >>
> >> Thanks
> >>    j
> >> ---
> >> ---
> >>   drivers/media/i2c/ov5640.c | 29 +++++++++++++----------------
> >>   1 file changed, 13 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> >> index 12b3496..bc75cb7 100644
> >> --- a/drivers/media/i2c/ov5640.c
> >> +++ b/drivers/media/i2c/ov5640.c
> >> @@ -1588,25 +1588,13 @@ static int ov5640_set_mode_exposure_calc(struct ov5640_dev *sensor,
> >>    * change mode directly
> >>    */
> >>   static int ov5640_set_mode_direct(struct ov5640_dev *sensor,
> >> -				  const struct ov5640_mode_info *mode,
> >> -				  s32 exposure)
> >> +				  const struct ov5640_mode_info *mode)
> >>   {
> >> -	int ret;
> >> -
> >>   	if (!mode->reg_data)
> >>   		return -EINVAL;
> >>
> >>   	/* Write capture setting */
> >> -	ret = ov5640_load_regs(sensor, mode);
> >> -	if (ret < 0)
> >> -		return ret;
> >> -
> >> -	/* turn auto gain/exposure back on for direct mode */
> >> -	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
> >> -	if (ret)
> >> -		return ret;
> >> -
> >> -	return __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, exposure);
> >> +	return  ov5640_load_regs(sensor, mode);
> >>   }
> >>
> >>   static int ov5640_set_mode(struct ov5640_dev *sensor,
> >> @@ -1626,7 +1614,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
> >>   		return ret;
> >>
> >>   	exposure = sensor->ctrls.auto_exp->val;
> >> -	ret = ov5640_set_exposure(sensor, V4L2_EXPOSURE_MANUAL);
> >> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, V4L2_EXPOSURE_MANUAL);
> >>   	if (ret)
> >>   		return ret;
> >>
> >> @@ -1642,12 +1630,21 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
> >>   		 * change inside subsampling or scaling
> >>   		 * download firmware directly
> >>   		 */
> >> -		ret = ov5640_set_mode_direct(sensor, mode, exposure);
> >> +		ret = ov5640_set_mode_direct(sensor, mode);
> >>   	}
> >>
> >>   	if (ret < 0)
> >>   		return ret;
> >>
> >> +	/* Restore auto-gain and auto-exposure after mode has changed. */
> >> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, exposure)
> >> +	if (ret)
> >> +		return ret;
> >> +
> >>   	ret = ov5640_set_binning(sensor, dn_mode != SCALING);
> >>   	if (ret < 0)
> >>   		return ret;
> >> --
> >> 2.7.4
> >>

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