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

[jacopo mondi <jacopo@jmondi.org>]

[-- 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 --]