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.
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
> > >
