Hi Alain,
> -----Original Message-----
> From: Alain Volmat <alain.volmat@xxxxxxxxxxx>
> Sent: 2023年7月20日 16:02
> 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,
>
> On Thu, Jul 20, 2023 at 03:27:20AM +0000, G.N. Zhou (OSS) wrote:
> > 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.
>
> The exposure control has to be updated following a VBLANK update.
> This is explained in the commit you are fixing. So I think that your fix should not
> only add the update of the vblank but also update the exposure value. You can
> have a look at the commit bce93b827de6 ("media: ov5640: Add VBLANK
> control") especially the update part in the ov5640_update_pixel_rate function.
>
> While it might not be the most beautiful way to do it, probably a simple goto and
> a label could also do the trick.
When update VBLANK control value, actually, it does.
__v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, vblank);
Call ov5640_s_ctrl()
case V4L2_CID_VBLANK:
/* Update the exposure range to the newly programmed vblank. */
timings = ov5640_timings(sensor, mode);
exp_max = mode->height + ctrl->val - 4;
__v4l2_ctrl_modify_range(sensor->ctrls.exposure,
sensor->ctrls.exposure->minimum,
exp_max, sensor->ctrls.exposure->step,
timings->vblank_def);
>
> if (!ov5640_is_csi2(sensor)) {
> __v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
>
> ov5640_calc_pixel_rate(sensor));
>
> goto update_ctrls;
> }
>
> (...)
>
> update_ctrls:
> 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);
>
> exposure_max = timings->crop.height + vblank - 4;
> exposure_val = clamp_t(s32, sensor->ctrls.exposure->val,
> sensor->ctrls.exposure->minimum,
> exposure_max);
>
> (...)
>
> Regards,
> Alain
>
> >
> > >
> > > 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
> > > >
