| Message ID | 20230719073012.3739998-1-guoniu.zhou@oss.nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: ov5640: fix vblank unchange issue when work at dvp mode | expand |
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. Regards, Alain On Wed, Jul 19, 2023 at 03:30:12PM +0800, guoniu.zhou@oss.nxp.com wrote: > From: "Guoniu.zhou" <guoniu.zhou@nxp.com> > > 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@nxp.com> > --- > 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 >
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@foss.st.com> > > Sent: 2023年7月19日 19:46 > > To: G.N. Zhou (OSS) <guoniu.zhou@oss.nxp.com> > > Cc: linux-media@vger.kernel.org; mchehab@kernel.org; > > slongerbeam@gmail.com; sakari.ailus@linux.intel.com; > > jacopo.mondi@ideasonboard.com; laurent.pinchart@ideasonboard.com > > 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@oss.nxp.com wrote: > > > From: "Guoniu.zhou" <guoniu.zhou@nxp.com> > > > > > > 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@nxp.com> > > > --- > > > 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 > > >
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,