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@xxxxxx>
> > ---
> > 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:
signature.asc
Description: PGP signature
