Hi Alain,
> -----Original Message-----
> From: Alain Volmat <alain.volmat@xxxxxxxxxxx>
> Sent: 2023年7月19日 19:46
> To: G.N. Zhou (OSS) <guoniu.zhou@xxxxxxxxxxx>
> Cc: linux-media@xxxxxxxxxxxxxxx; mchehab@xxxxxxxxxx;
> slongerbeam@xxxxxxxxx; sakari.ailus@xxxxxxxxxxxxxxx;
> jacopo.mondi@xxxxxxxxxxxxxxxx; laurent.pinchart@xxxxxxxxxxxxxxxx
> Subject: Re: [PATCH] media: ov5640: fix vblank unchange issue when work at
> dvp mode
>
> Caution: This is an external email. Please take care when clicking links or opening
> attachments. When in doubt, report the message using the 'Report this email'
> button
>
>
> Hi Guoniu,
>
> I came up to the same conclusion about wrong vblank when trying to make the
> OV5640 work in DVP mode so I agree about this modification.
>
> However I think other ctrls also have the same issue, at least exposure. I am
> wondering if we should keep the splitted code as currently and add back the
> missing ctrl in the DVP mode path or rework code to apply ctrl in both modes ?
> Basically link_freq / pixelrate handling differ between DVP and MIPI but it should
> be same handling for other ctrls I think.
As you know, the patch is for VBLANK control added in " bce93b827de6 ("media: ov5640: Add VBLANK control")" and I prefer and follow "one patch do one thing" rule.
>
> Regards,
> Alain
>
> On Wed, Jul 19, 2023 at 03:30:12PM +0800, guoniu.zhou@xxxxxxxxxxx wrote:
> > From: "Guoniu.zhou" <guoniu.zhou@xxxxxxx>
> >
> > The value of V4L2_CID_VBLANK control is initialized to default vblank
> > value of 640x480 when driver probe. When OV5640 work at DVP mode, the
> > control value won't update and lead to sensor can't output data if the
> > resolution remain the same as last time since incorrect total vertical
> > size. So update it when there is a new value applied.
> >
> > Fixes: bce93b827de6 ("media: ov5640: Add VBLANK control")
> > Signed-off-by: Guoniu.zhou <guoniu.zhou@xxxxxxx>
> > ---
> > drivers/media/i2c/ov5640.c | 19 ++++++++++++++-----
> > 1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 36b509714c8c..2f742f5f95fd 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -2854,12 +2854,22 @@ static int ov5640_try_fmt_internal(struct
> v4l2_subdev *sd,
> > return 0;
> > }
> >
> > +static void __v4l2_ctrl_vblank_update(struct ov5640_dev *sensor, u32
> > +vblank) {
> > + const struct ov5640_mode_info *mode = sensor->current_mode;
> > +
> > + __v4l2_ctrl_modify_range(sensor->ctrls.vblank,
> OV5640_MIN_VBLANK,
> > + OV5640_MAX_VTS - mode->height, 1,
> > + vblank);
> > +
> > + __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, vblank); }
> > +
> > static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) {
> > const struct ov5640_mode_info *mode = sensor->current_mode;
> > enum ov5640_pixel_rate_id pixel_rate_id = mode->pixel_rate;
> > struct v4l2_mbus_framefmt *fmt = &sensor->fmt;
> > - const struct ov5640_timings *timings;
> > + const struct ov5640_timings *timings = ov5640_timings(sensor,
> > + mode);
> > s32 exposure_val, exposure_max;
> > unsigned int hblank;
> > unsigned int i = 0;
> > @@ -2878,6 +2888,8 @@ static int ov5640_update_pixel_rate(struct
> ov5640_dev *sensor)
> > __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
> >
> > ov5640_calc_pixel_rate(sensor));
> >
> > + __v4l2_ctrl_vblank_update(sensor, timings->vblank_def);
> > +
> > return 0;
> > }
> >
> > @@ -2920,15 +2932,12 @@ static int ov5640_update_pixel_rate(struct
> ov5640_dev *sensor)
> > __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, pixel_rate);
> > __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, i);
> >
> > - timings = ov5640_timings(sensor, mode);
> > hblank = timings->htot - mode->width;
> > __v4l2_ctrl_modify_range(sensor->ctrls.hblank,
> > hblank, hblank, 1, hblank);
> >
> > vblank = timings->vblank_def;
> > - __v4l2_ctrl_modify_range(sensor->ctrls.vblank,
> OV5640_MIN_VBLANK,
> > - OV5640_MAX_VTS - mode->height, 1,
> vblank);
> > - __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, vblank);
> > + __v4l2_ctrl_vblank_update(sensor, vblank);
> >
> > exposure_max = timings->crop.height + vblank - 4;
> > exposure_val = clamp_t(s32, sensor->ctrls.exposure->val,
> > --
> > 2.37.1
> >
