Re: [PATCH v2 5/5] media: ov5640: fix restore of last mode set

[jacopo mondi <jacopo@jmondi.org>]

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

Hi Hugues,
    thanks for the patch

On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote:
> Mode setting depends on last mode set, in particular
> because of exposure calculation when downscale mode
> change between subsampling and scaling.
> At stream on the last mode was wrongly set to current mode,
> so no change was detected and exposure calculation
> was not made, fix this.

I actually see a different issue here...

The issue I see here depends on the format programmed through
set_fmt() never being applied when using the sensor with a media
controller equipped device (in this case an i.MX6 board) through
capture sessions, and the not properly calculated exposure you see may
be a consequence of this.

I'll try to write down what I see, with the help of some debug output.

- At probe time mode 640x460@30 is programmed:
  [    1.651216] ov5640_probe: Initial mode with id: 2

- I set the format on the sensor's pad and it gets not applied but
  marked as pending as the sensor is powered off:

  #media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/320x240 field:none]"
   [   65.611983] ov5640_set_fmt: NEW mode with id: 1 - PENDING

- I start streaming with yavta, and the sensor receives a power on;
  this causes the 'initial' format to be re-programmed and the pending
  change to be ignored:

  #yavta -c10 -n4 -f YUYV -s $320x240  -F"../frame-#.yuv" /dev/video4
   [   69.395018] ov5640_set_power:1805 - on
   [   69.431342] ov5640_restore_mode:1711
   [   69.996882] ov5640_set_mode: Apply mode with id: 0

  The 'ov5640_set_mode()' call from 'ov5640_restore_mode()' clears the
  sensor->pending flag, discarding the newly requested format, for
  this reason, at s_stream() time, the pending flag is not set
  anymore.

Are you using a media-controller system? I suspect in non-mc cases,
the set_fmt is applied through a single power_on/power_off session, not
causing the 'restore_mode()' issue. Is this the case for you or your
issue is differnt?

Edit:
Mita-san tried to address the issue of the output pixel format not
being restored when the image format was restored in
19ad26f9e6e1 ("media: ov5640: add missing output pixel format setting")

I understand the issue he tried to fix, but shouldn't the pending
format (if any) be applied instead of the initial one unconditionally?

Thanks
   j

>
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
>  drivers/media/i2c/ov5640.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index c110a6a..923cc30 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -225,6 +225,7 @@ struct ov5640_dev {
>  	struct v4l2_mbus_framefmt fmt;
>
>  	const struct ov5640_mode_info *current_mode;
> +	const struct ov5640_mode_info *last_mode;
>  	enum ov5640_frame_rate current_fr;
>  	struct v4l2_fract frame_interval;
>
> @@ -1628,6 +1629,9 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
>  	bool auto_exp =  sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO;
>  	int ret;
>
> +	if (!orig_mode)
> +		orig_mode = mode;
> +
>  	dn_mode = mode->dn_mode;
>  	orig_dn_mode = orig_mode->dn_mode;
>
> @@ -1688,6 +1692,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
>  		return ret;
>
>  	sensor->pending_mode_change = false;
> +	sensor->last_mode = mode;
>
>  	return 0;
>
> @@ -2551,7 +2556,8 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
>
>  	if (sensor->streaming == !enable) {
>  		if (enable && sensor->pending_mode_change) {
> -			ret = ov5640_set_mode(sensor, sensor->current_mode);
> +			ret = ov5640_set_mode(sensor, sensor->last_mode);
> +
>  			if (ret)
>  				goto out;
>
> --
> 2.7.4
>

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