[PATCH] media: ov5640: fix mode change regression

[PATCH] media: ov5640: fix mode change regression

[Hugues Fruchet]

fixes: 6949d864776e ("media: ov5640: do not change mode if format or frame interval is unchanged").

Symptom was fuzzy image because of JPEG default format
not being changed according to new format selected, fix this.
Init sequence initialises format to YUV422 UYVY but
sensor->fmt initial value was set to JPEG, fix this.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/i2c/ov5640.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 071f4bc..2ddd86d 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -223,6 +223,7 @@ struct ov5640_dev {
 	int power_count;
 
 	struct v4l2_mbus_framefmt fmt;
+	bool pending_fmt_change;
 
 	const struct ov5640_mode_info *current_mode;
 	enum ov5640_frame_rate current_fr;
@@ -255,7 +256,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
  * should be identified and removed to speed register load time
  * over i2c.
  */
-
+/* YUV422 UYVY VGA@30fps */
 static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
 	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
 	{0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
@@ -1968,9 +1969,12 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
 
 	if (new_mode != sensor->current_mode) {
 		sensor->current_mode = new_mode;
-		sensor->fmt = *mbus_fmt;
 		sensor->pending_mode_change = true;
 	}
+	if (mbus_fmt->code != sensor->fmt.code) {
+		sensor->fmt = *mbus_fmt;
+		sensor->pending_fmt_change = true;
+	}
 out:
 	mutex_unlock(&sensor->lock);
 	return ret;
@@ -2544,10 +2548,13 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
 			ret = ov5640_set_mode(sensor, sensor->current_mode);
 			if (ret)
 				goto out;
+		}
 
+		if (enable && sensor->pending_fmt_change) {
 			ret = ov5640_set_framefmt(sensor, &sensor->fmt);
 			if (ret)
 				goto out;
+			sensor->pending_fmt_change = false;
 		}
 
 		if (sensor->ep.bus_type == V4L2_MBUS_CSI2)
@@ -2642,9 +2649,14 @@ static int ov5640_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	sensor->i2c_client = client;
+
+	/*
+	 * default init sequence initialize sensor to
+	 * YUV422 UYVY VGA@30fps
+	 */
 	fmt = &sensor->fmt;
-	fmt->code = ov5640_formats[0].code;
-	fmt->colorspace = ov5640_formats[0].colorspace;
+	fmt->code = MEDIA_BUS_FMT_UYVY8_2X8;
+	fmt->colorspace = V4L2_COLORSPACE_SRGB;
 	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
 	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
 	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
@@ -2656,7 +2668,6 @@ static int ov5640_probe(struct i2c_client *client,
 	sensor->current_fr = OV5640_30_FPS;
 	sensor->current_mode =
 		&ov5640_mode_data[OV5640_30_FPS][OV5640_MODE_VGA_640_480];
-	sensor->pending_mode_change = true;
 
 	sensor->ae_target = 52;
 
-- 
2.7.4

Re: [PATCH] media: ov5640: fix mode change regression

[jacopo mondi]

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

Hi Hugues,
   thanks for the patch

On Thu, Aug 16, 2018 at 11:46:53AM +0200, Hugues Fruchet wrote:
> fixes: 6949d864776e ("media: ov5640: do not change mode if format or frame interval is unchanged").
>
> Symptom was fuzzy image because of JPEG default format
> not being changed according to new format selected, fix this.
> Init sequence initialises format to YUV422 UYVY but
> sensor->fmt initial value was set to JPEG, fix this.
>
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
>  drivers/media/i2c/ov5640.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 071f4bc..2ddd86d 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -223,6 +223,7 @@ struct ov5640_dev {
>  	int power_count;
>
>  	struct v4l2_mbus_framefmt fmt;
> +	bool pending_fmt_change;

The foundamental issue here is that 'struct ov5640_mode_info' and
associated functions do not take the image format into account...
That would be the real fix, but I understand it requires changing and
re-testing a lot of stuff :(

But what if instead of adding more flags, don't we use bitfields in a single
"pending_changes" field? As when, and if, framerate will be made more
'dynamic' and we remove the static 15/30FPS configuration from
ov5640_mode_info, we will have the same problem we have today with
format with framerate too...

Something like:

struct ov5640_dev {
        ...
-       bool pending_mode_change;
+       #define MODE_CHANGE     BIT(0)
+       #define FMT_CHANGE      BIT(1)
+       u8 pending;
        ...
}

>
>  	const struct ov5640_mode_info *current_mode;
>  	enum ov5640_frame_rate current_fr;
> @@ -255,7 +256,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
>   * should be identified and removed to speed register load time
>   * over i2c.
>   */
> -
> +/* YUV422 UYVY VGA@30fps */
>  static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
>  	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
>  	{0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
> @@ -1968,9 +1969,12 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
>
>  	if (new_mode != sensor->current_mode) {
>  		sensor->current_mode = new_mode;
> -		sensor->fmt = *mbus_fmt;
>  		sensor->pending_mode_change = true;
>  	}
> +	if (mbus_fmt->code != sensor->fmt.code) {
> +		sensor->fmt = *mbus_fmt;
> +		sensor->pending_fmt_change = true;
> +	}

That would make this simpler

  		sensor->current_mode = new_mode;
		sensor->fmt = *mbus_fmt;

                if (new_mode != sensor->current_mode)
                        sensor->pending |= MODE_CHANGE;
	        if (mbus_fmt->code != sensor->fmt.code) {
                        sensor->pending |= FMT_CHANGE;


>  out:
>  	mutex_unlock(&sensor->lock);
>  	return ret;
> @@ -2544,10 +2548,13 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
>  			ret = ov5640_set_mode(sensor, sensor->current_mode);
>  			if (ret)
>  				goto out;
> +		}
>
> +		if (enable && sensor->pending_fmt_change) {
>  			ret = ov5640_set_framefmt(sensor, &sensor->fmt);
>  			if (ret)
>  				goto out;
> +			sensor->pending_fmt_change = false;
>  		}
>

And that would be accordingly:

                if (sensor->pending & MODE_CHANGE) {
                       ret = ov5640_set_mode(sensor, sensor->current_mode);
                       ....
                }
                if (sensor->pending & FMT_CHANGE) {
                       ret = ov5640_set_framefmt(sensor, &sensor->fmt);
                       ...
                }

What do you (and others) think?

Thanks
   j

>  		if (sensor->ep.bus_type == V4L2_MBUS_CSI2)
> @@ -2642,9 +2649,14 @@ static int ov5640_probe(struct i2c_client *client,
>  		return -ENOMEM;
>
>  	sensor->i2c_client = client;
> +
> +	/*
> +	 * default init sequence initialize sensor to
> +	 * YUV422 UYVY VGA@30fps
> +	 */
>  	fmt = &sensor->fmt;
> -	fmt->code = ov5640_formats[0].code;
> -	fmt->colorspace = ov5640_formats[0].colorspace;
> +	fmt->code = MEDIA_BUS_FMT_UYVY8_2X8;
> +	fmt->colorspace = V4L2_COLORSPACE_SRGB;
>  	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
>  	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>  	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> @@ -2656,7 +2668,6 @@ static int ov5640_probe(struct i2c_client *client,
>  	sensor->current_fr = OV5640_30_FPS;
>  	sensor->current_mode =
>  		&ov5640_mode_data[OV5640_30_FPS][OV5640_MODE_VGA_640_480];
> -	sensor->pending_mode_change = true;
>
>  	sensor->ae_target = 52;
>
> --
> 2.7.4
>

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

Re: [PATCH] media: ov5640: fix mode change regression

[jacopo mondi]

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

On Tue, Aug 28, 2018 at 02:57:11PM +0200, jacopo mondi wrote:
> Hi Hugues,
>    thanks for the patch
>
> On Thu, Aug 16, 2018 at 11:46:53AM +0200, Hugues Fruchet wrote:
> > fixes: 6949d864776e ("media: ov5640: do not change mode if format or frame interval is unchanged").
> >
> > Symptom was fuzzy image because of JPEG default format
> > not being changed according to new format selected, fix this.
> > Init sequence initialises format to YUV422 UYVY but
> > sensor->fmt initial value was set to JPEG, fix this.
> >
> > Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> > ---
> >  drivers/media/i2c/ov5640.c | 21 ++++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 071f4bc..2ddd86d 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -223,6 +223,7 @@ struct ov5640_dev {
> >  	int power_count;
> >
> >  	struct v4l2_mbus_framefmt fmt;
> > +	bool pending_fmt_change;
>
> The foundamental issue here is that 'struct ov5640_mode_info' and
> associated functions do not take the image format into account...
> That would be the real fix, but I understand it requires changing and
> re-testing a lot of stuff :(
>
> But what if instead of adding more flags, don't we use bitfields in a single
> "pending_changes" field? As when, and if, framerate will be made more
> 'dynamic' and we remove the static 15/30FPS configuration from
> ov5640_mode_info, we will have the same problem we have today with
> format with framerate too...
>
> Something like:
>
> struct ov5640_dev {
>         ...
> -       bool pending_mode_change;
> +       #define MODE_CHANGE     BIT(0)
> +       #define FMT_CHANGE      BIT(1)
> +       u8 pending;
>         ...
> }
>
> >
> >  	const struct ov5640_mode_info *current_mode;
> >  	enum ov5640_frame_rate current_fr;
> > @@ -255,7 +256,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
> >   * should be identified and removed to speed register load time
> >   * over i2c.
> >   */
> > -
> > +/* YUV422 UYVY VGA@30fps */
> >  static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
> >  	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
> >  	{0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
> > @@ -1968,9 +1969,12 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
> >
> >  	if (new_mode != sensor->current_mode) {
> >  		sensor->current_mode = new_mode;
> > -		sensor->fmt = *mbus_fmt;
> >  		sensor->pending_mode_change = true;
> >  	}
> > +	if (mbus_fmt->code != sensor->fmt.code) {
> > +		sensor->fmt = *mbus_fmt;
> > +		sensor->pending_fmt_change = true;
> > +	}
>
> That would make this simpler
>
>   		sensor->current_mode = new_mode;
> 		sensor->fmt = *mbus_fmt;
>
>                 if (new_mode != sensor->current_mode)
>                         sensor->pending |= MODE_CHANGE;
> 	        if (mbus_fmt->code != sensor->fmt.code) {
>                         sensor->pending |= FMT_CHANGE;
>

Yeah, well, this is in wrong order of course :)

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