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