| Message ID | 20220131143245.128089-12-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ov5640: Rework the clock tree programming for MIPI | expand |
Hi Jacopo, Thank you for the patch. On Mon, Jan 31, 2022 at 03:32:35PM +0100, Jacopo Mondi wrote: > Add the VBLANK control which allows to select the duration of the > frame vertical blankings and allows to control the framerate. > > The VBLANK control also modifies the exposure time range, which cannot > exceed the maximum frame length. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/media/i2c/ov5640.c | 50 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index b2961e8e07c3..6eeb50724195 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -31,6 +31,10 @@ > > #define OV5640_LINK_RATE_MAX 490000000U > > +/* FIXME: not documented. */ > +#define OV5640_MIN_VBLANK 24 > +#define OV5640_MAX_VTS 1968 > + > #define OV5640_DEFAULT_SLAVE_ID 0x3c > > #define OV5640_REG_SYS_RESET02 0x3002 > @@ -267,6 +271,7 @@ struct ov5640_ctrls { > struct v4l2_ctrl *pixel_rate; > struct v4l2_ctrl *link_freq; > struct v4l2_ctrl *hblank; > + struct v4l2_ctrl *vblank; > struct { > struct v4l2_ctrl *auto_exp; > struct v4l2_ctrl *exposure; > @@ -2531,6 +2536,7 @@ static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) > struct v4l2_mbus_framefmt *fmt = &sensor->fmt; > enum ov5640_pixel_rate_id pixel_rate_id = mode->pixel_rate; > u32 num_lanes = sensor->ep.bus.mipi_csi2.num_data_lanes; > + s64 exposure_val, exposure_max; > unsigned int hblank; > unsigned int i = 0; > u32 pixel_rate; > @@ -2586,6 +2592,20 @@ static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) > __v4l2_ctrl_modify_range(sensor->ctrls.hblank, > hblank, hblank, 1, hblank); > > + __v4l2_ctrl_modify_range(sensor->ctrls.vblank, > + OV5640_MIN_VBLANK, > + OV5640_MAX_VTS - mode->crop.height, 1, > + mode->vblank_def); > + __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, mode->vblank_def); > + > + exposure_max = mode->crop.height + mode->vblank_def - OV5640_MIN_VBLANK; There's typically a fixed margin of a few lines between the maximum exposure and the total vertical size, but that's usually smaller than the minimum vblank value. Are you sure OV5640_MIN_VBLANK is right here ? > + exposure_val = clamp((s64)sensor->ctrls.exposure->val, > + (s64)sensor->ctrls.exposure->minimum, > + (s64)exposure_max); > + __v4l2_ctrl_modify_range(sensor->ctrls.exposure, > + sensor->ctrls.exposure->minimum, > + exposure_max, 1, exposure_val); > + > return 0; > } > > @@ -2958,6 +2978,15 @@ static int ov5640_set_ctrl_vflip(struct ov5640_dev *sensor, int value) > (BIT(2) | BIT(1)) : 0); > } > > +static int ov5640_set_ctrl_vblank(struct ov5640_dev *sensor, int value) > +{ > + const struct ov5640_mode_info *mode = sensor->current_mode; > + > + /* Update the VTOT timing register value. */ > + return ov5640_write_reg16(sensor, OV5640_REG_TIMING_VTS, > + mode->crop.height + value); > +} > + > static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl) > { > struct v4l2_subdev *sd = ctrl_to_sd(ctrl); > @@ -2988,10 +3017,22 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) > { > struct v4l2_subdev *sd = ctrl_to_sd(ctrl); > struct ov5640_dev *sensor = to_ov5640_dev(sd); > + const struct ov5640_mode_info *mode = sensor->current_mode; > int ret; > > /* v4l2_ctrl_lock() locks our own mutex */ > > + switch (ctrl->id) { > + case V4L2_CID_VBLANK: > + /* Update the exposure range to the newly programmed vblank. */ > + unsigned int max = mode->crop.height + ctrl->val - OV5640_MIN_VBLANK; > + > + __v4l2_ctrl_modify_range(sensor->ctrls.exposure, > + sensor->ctrls.exposure->minimum, > + max, sensor->ctrls.exposure->step, max); Should the default be set to mode->vblank_def ? > + break; > + } > + > /* > * If the device is not powered up by the host driver do > * not apply any controls to H/W at this time. Instead > @@ -3031,6 +3072,9 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) > case V4L2_CID_VFLIP: > ret = ov5640_set_ctrl_vflip(sensor, ctrl->val); > break; > + case V4L2_CID_VBLANK: > + ret = ov5640_set_ctrl_vblank(sensor, ctrl->val); > + break; > default: > ret = -EINVAL; > break; > @@ -3050,6 +3094,7 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) > const struct v4l2_ctrl_ops *ops = &ov5640_ctrl_ops; > struct ov5640_ctrls *ctrls = &sensor->ctrls; > struct v4l2_ctrl_handler *hdl = &ctrls->handler; > + unsigned int max_vblank; > unsigned int hblank; > int ret; > > @@ -3073,6 +3118,11 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) > ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, hblank, > hblank, 1, hblank); > > + max_vblank = OV5640_MAX_VTS - mode->crop.height; > + ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, > + OV5640_MIN_VBLANK, max_vblank, > + 1, mode->vblank_def); > + > /* Auto/manual white balance */ > ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops, > V4L2_CID_AUTO_WHITE_BALANCE,
Hello Jacopo, On 1/31/22 15:32, Jacopo Mondi wrote: > Add the VBLANK control which allows to select the duration of the > frame vertical blankings and allows to control the framerate. > > The VBLANK control also modifies the exposure time range, which cannot > exceed the maximum frame length. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/media/i2c/ov5640.c | 50 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index b2961e8e07c3..6eeb50724195 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -31,6 +31,10 @@ > > #define OV5640_LINK_RATE_MAX 490000000U > > +/* FIXME: not documented. */ > +#define OV5640_MIN_VBLANK 24 > +#define OV5640_MAX_VTS 1968 > + > #define OV5640_DEFAULT_SLAVE_ID 0x3c > > #define OV5640_REG_SYS_RESET02 0x3002 > @@ -267,6 +271,7 @@ struct ov5640_ctrls { > struct v4l2_ctrl *pixel_rate; > struct v4l2_ctrl *link_freq; > struct v4l2_ctrl *hblank; > + struct v4l2_ctrl *vblank; > struct { > struct v4l2_ctrl *auto_exp; > struct v4l2_ctrl *exposure; > @@ -2531,6 +2536,7 @@ static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) > struct v4l2_mbus_framefmt *fmt = &sensor->fmt; > enum ov5640_pixel_rate_id pixel_rate_id = mode->pixel_rate; > u32 num_lanes = sensor->ep.bus.mipi_csi2.num_data_lanes; > + s64 exposure_val, exposure_max; > unsigned int hblank; > unsigned int i = 0; > u32 pixel_rate; > @@ -2586,6 +2592,20 @@ static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) > __v4l2_ctrl_modify_range(sensor->ctrls.hblank, > hblank, hblank, 1, hblank); > > + __v4l2_ctrl_modify_range(sensor->ctrls.vblank, > + OV5640_MIN_VBLANK, > + OV5640_MAX_VTS - mode->crop.height, 1, > + mode->vblank_def); > + __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, mode->vblank_def); > + > + exposure_max = mode->crop.height + mode->vblank_def - OV5640_MIN_VBLANK; > + exposure_val = clamp((s64)sensor->ctrls.exposure->val, > + (s64)sensor->ctrls.exposure->minimum, > + (s64)exposure_max); > + __v4l2_ctrl_modify_range(sensor->ctrls.exposure, > + sensor->ctrls.exposure->minimum, > + exposure_max, 1, exposure_val); > + > return 0; > } > > @@ -2958,6 +2978,15 @@ static int ov5640_set_ctrl_vflip(struct ov5640_dev *sensor, int value) > (BIT(2) | BIT(1)) : 0); > } > > +static int ov5640_set_ctrl_vblank(struct ov5640_dev *sensor, int value) > +{ > + const struct ov5640_mode_info *mode = sensor->current_mode; > + > + /* Update the VTOT timing register value. */ > + return ov5640_write_reg16(sensor, OV5640_REG_TIMING_VTS, > + mode->crop.height + value); > +} > + > static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl) > { > struct v4l2_subdev *sd = ctrl_to_sd(ctrl); > @@ -2988,10 +3017,22 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) > { > struct v4l2_subdev *sd = ctrl_to_sd(ctrl); > struct ov5640_dev *sensor = to_ov5640_dev(sd); > + const struct ov5640_mode_info *mode = sensor->current_mode; > int ret; > > /* v4l2_ctrl_lock() locks our own mutex */ > > + switch (ctrl->id) { > + case V4L2_CID_VBLANK: > + /* Update the exposure range to the newly programmed vblank. */ > + unsigned int max = mode->crop.height + ctrl->val - OV5640_MIN_VBLANK; > + > + __v4l2_ctrl_modify_range(sensor->ctrls.exposure, > + sensor->ctrls.exposure->minimum, > + max, sensor->ctrls.exposure->step, max); > + break; > + }The kernel build reports the errors below with clang toolchain (LLVM=1): drivers/media/i2c/ov5640.c:3124:3: error: expected expression unsigned int max = mode->crop.height + ctrl->val - OV5640_MIN_VBLANK; ^ drivers/media/i2c/ov5640.c:3128:7: error: use of undeclared identifier 'max' max, sensor->ctrls.exposure->step, max); ^ drivers/media/i2c/ov5640.c:3128:42: error: use of undeclared identifier 'max' max, sensor->ctrls.exposure->step, max); ^ 3 errors generated. make[3]: *** [scripts/Makefile.build:288: drivers/media/i2c/ov5640.o] Error 1 Declaring the variable within a compound statement following the label (case), or moving the declaration after the switch fix the build. The former construct might be more aligned with kernel coding use (?). switch(...) { case ...: { unsigned int max = ...; ... } } or switch(...) { unsigned int max = 0; case ...: ... break; } > + > /* > * If the device is not powered up by the host driver do > * not apply any controls to H/W at this time. Instead > @@ -3031,6 +3072,9 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) > case V4L2_CID_VFLIP: > ret = ov5640_set_ctrl_vflip(sensor, ctrl->val); > break; > + case V4L2_CID_VBLANK: > + ret = ov5640_set_ctrl_vblank(sensor, ctrl->val); > + break; > default: > ret = -EINVAL; > break; > @@ -3050,6 +3094,7 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) > const struct v4l2_ctrl_ops *ops = &ov5640_ctrl_ops; > struct ov5640_ctrls *ctrls = &sensor->ctrls; > struct v4l2_ctrl_handler *hdl = &ctrls->handler; > + unsigned int max_vblank; > unsigned int hblank; > int ret; > > @@ -3073,6 +3118,11 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) > ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, hblank, > hblank, 1, hblank); > > + max_vblank = OV5640_MAX_VTS - mode->crop.height; > + ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, > + OV5640_MIN_VBLANK, max_vblank, > + 1, mode->vblank_def); > + > /* Auto/manual white balance */ > ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops, > V4L2_CID_AUTO_WHITE_BALANCE, Regards, Xavier
Hi Laurent, On Wed, Feb 02, 2022 at 11:35:31PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Mon, Jan 31, 2022 at 03:32:35PM +0100, Jacopo Mondi wrote: > > Add the VBLANK control which allows to select the duration of the > > frame vertical blankings and allows to control the framerate. > > > > The VBLANK control also modifies the exposure time range, which cannot > > exceed the maximum frame length. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > drivers/media/i2c/ov5640.c | 50 ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 50 insertions(+) > > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > > index b2961e8e07c3..6eeb50724195 100644 > > --- a/drivers/media/i2c/ov5640.c > > +++ b/drivers/media/i2c/ov5640.c > > @@ -31,6 +31,10 @@ > > > > #define OV5640_LINK_RATE_MAX 490000000U > > > > +/* FIXME: not documented. */ > > +#define OV5640_MIN_VBLANK 24 > > +#define OV5640_MAX_VTS 1968 > > + > > #define OV5640_DEFAULT_SLAVE_ID 0x3c > > > > #define OV5640_REG_SYS_RESET02 0x3002 > > @@ -267,6 +271,7 @@ struct ov5640_ctrls { > > struct v4l2_ctrl *pixel_rate; > > struct v4l2_ctrl *link_freq; > > struct v4l2_ctrl *hblank; > > + struct v4l2_ctrl *vblank; > > struct { > > struct v4l2_ctrl *auto_exp; > > struct v4l2_ctrl *exposure; > > @@ -2531,6 +2536,7 @@ static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) > > struct v4l2_mbus_framefmt *fmt = &sensor->fmt; > > enum ov5640_pixel_rate_id pixel_rate_id = mode->pixel_rate; > > u32 num_lanes = sensor->ep.bus.mipi_csi2.num_data_lanes; > > + s64 exposure_val, exposure_max; > > unsigned int hblank; > > unsigned int i = 0; > > u32 pixel_rate; > > @@ -2586,6 +2592,20 @@ static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) > > __v4l2_ctrl_modify_range(sensor->ctrls.hblank, > > hblank, hblank, 1, hblank); > > > > + __v4l2_ctrl_modify_range(sensor->ctrls.vblank, > > + OV5640_MIN_VBLANK, > > + OV5640_MAX_VTS - mode->crop.height, 1, > > + mode->vblank_def); > > + __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, mode->vblank_def); > > + > > + exposure_max = mode->crop.height + mode->vblank_def - OV5640_MIN_VBLANK; > > There's typically a fixed margin of a few lines between the maximum > exposure and the total vertical size, but that's usually smaller than > the minimum vblank value. Are you sure OV5640_MIN_VBLANK is right here ? > Section "4.6.2 manual exposure control" of the datasheet reports some hints about how to enlarge the total frame size to accomodate the desired exposure. It is not totally clear to me how though. The maximum exposure time seems to be limited not only by VTS, but by (VTS + [0x350Cd,0x350d]). The 0x350c/d registers are said to: In auto exposure mode, the extra exposure values (larger than 1 frame) in registers 0x350C/0x350D automatically change. In manual exposure mode, these registers will not automatically change. The manually set exposure in registers 0x3500~0x3502 must be less than the maximum exposure value in {0x380E, 0x380F} + {0x350C,0x350D} with: - 0x350c,0x350d = PK_VTS - 0x380e,0x380f = VTS I have no idea how manual exposure works with this sensor, as I see 0x350c only being read and never programmed. This anyway does not suggest any margin between VTS+PK_VTS but rahter that the exposure time can be larger than VTS which doesn't make much sense to me (maybe I should just consider 'real VTS' = VTS + PK_VTS). Anyway, being aware of such margins being usually required, I used the min VBLANK value. Which is also not documented but I experimentally tried to progressively shrink until capture did not break. Should I remove it from the above calculation ? > > + exposure_val = clamp((s64)sensor->ctrls.exposure->val, > > + (s64)sensor->ctrls.exposure->minimum, > > + (s64)exposure_max); > > + __v4l2_ctrl_modify_range(sensor->ctrls.exposure, > > + sensor->ctrls.exposure->minimum, > > + exposure_max, 1, exposure_val); > > + > > return 0; > > } > > > > @@ -2958,6 +2978,15 @@ static int ov5640_set_ctrl_vflip(struct ov5640_dev *sensor, int value) > > (BIT(2) | BIT(1)) : 0); > > } > > > > +static int ov5640_set_ctrl_vblank(struct ov5640_dev *sensor, int value) > > +{ > > + const struct ov5640_mode_info *mode = sensor->current_mode; > > + > > + /* Update the VTOT timing register value. */ > > + return ov5640_write_reg16(sensor, OV5640_REG_TIMING_VTS, > > + mode->crop.height + value); > > +} > > + > > static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl) > > { > > struct v4l2_subdev *sd = ctrl_to_sd(ctrl); > > @@ -2988,10 +3017,22 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) > > { > > struct v4l2_subdev *sd = ctrl_to_sd(ctrl); > > struct ov5640_dev *sensor = to_ov5640_dev(sd); > > + const struct ov5640_mode_info *mode = sensor->current_mode; > > int ret; > > > > /* v4l2_ctrl_lock() locks our own mutex */ > > > > + switch (ctrl->id) { > > + case V4L2_CID_VBLANK: > > + /* Update the exposure range to the newly programmed vblank. */ > > + unsigned int max = mode->crop.height + ctrl->val - OV5640_MIN_VBLANK; > > + > > + __v4l2_ctrl_modify_range(sensor->ctrls.exposure, > > + sensor->ctrls.exposure->minimum, > > + max, sensor->ctrls.exposure->step, max); > > Should the default be set to mode->vblank_def ? > It should! > > + break; > > + } > > + > > /* > > * If the device is not powered up by the host driver do > > * not apply any controls to H/W at this time. Instead > > @@ -3031,6 +3072,9 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) > > case V4L2_CID_VFLIP: > > ret = ov5640_set_ctrl_vflip(sensor, ctrl->val); > > break; > > + case V4L2_CID_VBLANK: > > + ret = ov5640_set_ctrl_vblank(sensor, ctrl->val); > > + break; > > default: > > ret = -EINVAL; > > break; > > @@ -3050,6 +3094,7 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) > > const struct v4l2_ctrl_ops *ops = &ov5640_ctrl_ops; > > struct ov5640_ctrls *ctrls = &sensor->ctrls; > > struct v4l2_ctrl_handler *hdl = &ctrls->handler; > > + unsigned int max_vblank; > > unsigned int hblank; > > int ret; > > > > @@ -3073,6 +3118,11 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) > > ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, hblank, > > hblank, 1, hblank); > > > > + max_vblank = OV5640_MAX_VTS - mode->crop.height; > > + ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, > > + OV5640_MIN_VBLANK, max_vblank, > > + 1, mode->vblank_def); > > + > > /* Auto/manual white balance */ > > ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops, > > V4L2_CID_AUTO_WHITE_BALANCE, > > -- > Regards, > > Laurent Pinchart
Hello Xavier On Thu, Feb 03, 2022 at 08:58:37AM +0100, Xavier Roumegue (OSS) wrote: > Hello Jacopo, > > On 1/31/22 15:32, Jacopo Mondi wrote: > > Add the VBLANK control which allows to select the duration of the > > frame vertical blankings and allows to control the framerate. > > > > The VBLANK control also modifies the exposure time range, which cannot > > exceed the maximum frame length. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > drivers/media/i2c/ov5640.c | 50 ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 50 insertions(+) > > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > > index b2961e8e07c3..6eeb50724195 100644 > > --- a/drivers/media/i2c/ov5640.c > > +++ b/drivers/media/i2c/ov5640.c > > @@ -31,6 +31,10 @@ > > #define OV5640_LINK_RATE_MAX 490000000U > > +/* FIXME: not documented. */ > > +#define OV5640_MIN_VBLANK 24 > > +#define OV5640_MAX_VTS 1968 > > + > > #define OV5640_DEFAULT_SLAVE_ID 0x3c > > #define OV5640_REG_SYS_RESET02 0x3002 > > @@ -267,6 +271,7 @@ struct ov5640_ctrls { > > struct v4l2_ctrl *pixel_rate; > > struct v4l2_ctrl *link_freq; > > struct v4l2_ctrl *hblank; > > + struct v4l2_ctrl *vblank; > > struct { > > struct v4l2_ctrl *auto_exp; > > struct v4l2_ctrl *exposure; > > @@ -2531,6 +2536,7 @@ static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) > > struct v4l2_mbus_framefmt *fmt = &sensor->fmt; > > enum ov5640_pixel_rate_id pixel_rate_id = mode->pixel_rate; > > u32 num_lanes = sensor->ep.bus.mipi_csi2.num_data_lanes; > > + s64 exposure_val, exposure_max; > > unsigned int hblank; > > unsigned int i = 0; > > u32 pixel_rate; > > @@ -2586,6 +2592,20 @@ static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) > > __v4l2_ctrl_modify_range(sensor->ctrls.hblank, > > hblank, hblank, 1, hblank); > > + __v4l2_ctrl_modify_range(sensor->ctrls.vblank, > > + OV5640_MIN_VBLANK, > > + OV5640_MAX_VTS - mode->crop.height, 1, > > + mode->vblank_def); > > + __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, mode->vblank_def); > > + > > + exposure_max = mode->crop.height + mode->vblank_def - OV5640_MIN_VBLANK; > > + exposure_val = clamp((s64)sensor->ctrls.exposure->val, > > + (s64)sensor->ctrls.exposure->minimum, > > + (s64)exposure_max); > > + __v4l2_ctrl_modify_range(sensor->ctrls.exposure, > > + sensor->ctrls.exposure->minimum, > > + exposure_max, 1, exposure_val); > > + > > return 0; > > } > > @@ -2958,6 +2978,15 @@ static int ov5640_set_ctrl_vflip(struct ov5640_dev *sensor, int value) > > (BIT(2) | BIT(1)) : 0); > > } > > +static int ov5640_set_ctrl_vblank(struct ov5640_dev *sensor, int value) > > +{ > > + const struct ov5640_mode_info *mode = sensor->current_mode; > > + > > + /* Update the VTOT timing register value. */ > > + return ov5640_write_reg16(sensor, OV5640_REG_TIMING_VTS, > > + mode->crop.height + value); > > +} > > + > > static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl) > > { > > struct v4l2_subdev *sd = ctrl_to_sd(ctrl); > > @@ -2988,10 +3017,22 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) > > { > > struct v4l2_subdev *sd = ctrl_to_sd(ctrl); > > struct ov5640_dev *sensor = to_ov5640_dev(sd); > > + const struct ov5640_mode_info *mode = sensor->current_mode; > > int ret; > > /* v4l2_ctrl_lock() locks our own mutex */ > > + switch (ctrl->id) { > > + case V4L2_CID_VBLANK: > > + /* Update the exposure range to the newly programmed vblank. */ > > + unsigned int max = mode->crop.height + ctrl->val - OV5640_MIN_VBLANK; > > + > > + __v4l2_ctrl_modify_range(sensor->ctrls.exposure, > > + sensor->ctrls.exposure->minimum, > > + max, sensor->ctrls.exposure->step, max); > > + break; > > + }The kernel build reports the errors below with clang toolchain (LLVM=1): > > drivers/media/i2c/ov5640.c:3124:3: error: expected expression > unsigned int max = mode->crop.height + ctrl->val - > OV5640_MIN_VBLANK; > ^ > drivers/media/i2c/ov5640.c:3128:7: error: use of undeclared identifier 'max' > max, sensor->ctrls.exposure->step, > max); > ^ > drivers/media/i2c/ov5640.c:3128:42: error: use of undeclared identifier > 'max' > max, sensor->ctrls.exposure->step, > max); > > ^ > 3 errors generated. > make[3]: *** [scripts/Makefile.build:288: drivers/media/i2c/ov5640.o] Error > 1 > > Declaring the variable within a compound statement following the label > (case), or moving the declaration after the switch fix the build. > The former construct might be more aligned with kernel coding use (?). > > switch(...) { > case ...: { > unsigned int max = ...; > ... > } > } > > or > > switch(...) { > unsigned int max = 0; > case ...: > ... > break; Thanks, I didn't compile with clang :) I'll go for the former, as you say it is more aligned with common practices as far as I can tell. Thanks j > } > > > + > > /* > > * If the device is not powered up by the host driver do > > * not apply any controls to H/W at this time. Instead > > @@ -3031,6 +3072,9 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) > > case V4L2_CID_VFLIP: > > ret = ov5640_set_ctrl_vflip(sensor, ctrl->val); > > break; > > + case V4L2_CID_VBLANK: > > + ret = ov5640_set_ctrl_vblank(sensor, ctrl->val); > > + break; > > default: > > ret = -EINVAL; > > break; > > @@ -3050,6 +3094,7 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) > > const struct v4l2_ctrl_ops *ops = &ov5640_ctrl_ops; > > struct ov5640_ctrls *ctrls = &sensor->ctrls; > > struct v4l2_ctrl_handler *hdl = &ctrls->handler; > > + unsigned int max_vblank; > > unsigned int hblank; > > int ret; > > @@ -3073,6 +3118,11 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) > > ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, hblank, > > hblank, 1, hblank); > > + max_vblank = OV5640_MAX_VTS - mode->crop.height; > > + ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, > > + OV5640_MIN_VBLANK, max_vblank, > > + 1, mode->vblank_def); > > + > > /* Auto/manual white balance */ > > ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops, > > V4L2_CID_AUTO_WHITE_BALANCE, > Regards, > Xavier
Hi Jacopo, On Mon, Feb 07, 2022 at 04:09:12PM +0100, Jacopo Mondi wrote: > On Wed, Feb 02, 2022 at 11:35:31PM +0200, Laurent Pinchart wrote: > > On Mon, Jan 31, 2022 at 03:32:35PM +0100, Jacopo Mondi wrote: > > > Add the VBLANK control which allows to select the duration of the > > > frame vertical blankings and allows to control the framerate. > > > > > > The VBLANK control also modifies the exposure time range, which cannot > > > exceed the maximum frame length. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > drivers/media/i2c/ov5640.c | 50 ++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 50 insertions(+) > > > > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > > > index b2961e8e07c3..6eeb50724195 100644 > > > --- a/drivers/media/i2c/ov5640.c > > > +++ b/drivers/media/i2c/ov5640.c > > > @@ -31,6 +31,10 @@ > > > > > > #define OV5640_LINK_RATE_MAX 490000000U > > > > > > +/* FIXME: not documented. */ > > > +#define OV5640_MIN_VBLANK 24 > > > +#define OV5640_MAX_VTS 1968 > > > + > > > #define OV5640_DEFAULT_SLAVE_ID 0x3c > > > > > > #define OV5640_REG_SYS_RESET02 0x3002 > > > @@ -267,6 +271,7 @@ struct ov5640_ctrls { > > > struct v4l2_ctrl *pixel_rate; > > > struct v4l2_ctrl *link_freq; > > > struct v4l2_ctrl *hblank; > > > + struct v4l2_ctrl *vblank; > > > struct { > > > struct v4l2_ctrl *auto_exp; > > > struct v4l2_ctrl *exposure; > > > @@ -2531,6 +2536,7 @@ static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) > > > struct v4l2_mbus_framefmt *fmt = &sensor->fmt; > > > enum ov5640_pixel_rate_id pixel_rate_id = mode->pixel_rate; > > > u32 num_lanes = sensor->ep.bus.mipi_csi2.num_data_lanes; > > > + s64 exposure_val, exposure_max; > > > unsigned int hblank; > > > unsigned int i = 0; > > > u32 pixel_rate; > > > @@ -2586,6 +2592,20 @@ static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) > > > __v4l2_ctrl_modify_range(sensor->ctrls.hblank, > > > hblank, hblank, 1, hblank); > > > > > > + __v4l2_ctrl_modify_range(sensor->ctrls.vblank, > > > + OV5640_MIN_VBLANK, > > > + OV5640_MAX_VTS - mode->crop.height, 1, > > > + mode->vblank_def); > > > + __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, mode->vblank_def); > > > + > > > + exposure_max = mode->crop.height + mode->vblank_def - OV5640_MIN_VBLANK; > > > > There's typically a fixed margin of a few lines between the maximum > > exposure and the total vertical size, but that's usually smaller than > > the minimum vblank value. Are you sure OV5640_MIN_VBLANK is right here ? > > Section "4.6.2 manual exposure control" of the datasheet reports > some hints about how to enlarge the total frame size to accomodate the > desired exposure. > > It is not totally clear to me how though. The maximum exposure time > seems to be limited not only by VTS, but by > (VTS + [0x350Cd,0x350d]). > > The 0x350c/d registers are said to: > > In auto exposure mode, the extra exposure values (larger than 1 frame) > in registers 0x350C/0x350D automatically change. In manual exposure > mode, these registers will not automatically change. The manually set > exposure in registers 0x3500~0x3502 must be less than the maximum > exposure value in {0x380E, 0x380F} + {0x350C,0x350D} > > > with: > - 0x350c,0x350d = PK_VTS > - 0x380e,0x380f = VTS > > I have no idea how manual exposure works with this sensor, as I see > 0x350c only being read and never programmed. > > This anyway does not suggest any margin between VTS+PK_VTS but rahter > that the exposure time can be larger than VTS which doesn't make much > sense to me (maybe I should just consider 'real VTS' = VTS + PK_VTS). It's quite confusing, as it doesn't tell what PK_VTS is. Note that in the auto-focus application note, section "4.11.1 Shutter" says The shutter of OV5640 controls exposure time. The unit of shutter is line period. Shutter value has limitation for each resolution. The maximum shutter value is VTS(0x380e, 0x380f) - 4. Furthermore, the OV5642 datasheet ([1]) has a slightly different explanation: 4.6.2 manual exposure control To manually change exposure value, you must first set both 0x3503[0] and 0x3503[2], where 0x3503[0] enables a manual exposure control and 0x3503[2] enables manual frame length - the number of lines in each frame or maximum exposure time, which is defined by registers 0x350C and 0x350D. In auto exposure mode, the maximum exposure values in registers 0x350C/0x350D automatically change. In manual exposure mode, these registers will not automatically change. The manually set exposure in registers 0x3500~0x03502 must be less than the maximum exposure value in 0x350C/0x350D. The exposure value in registers 0x3500~0x3502 is in units of line*16 - the low 4 bits (0x3502[3:0]) is the fraction of line, the maximum value in 0x350C/0x350D is in unit of line. If the manually set exposure value is less than one pre-defined frame period (e.g., 1/15 second in 15fps), there is no need to change 0x350C/0x350D. If the exposure value needs to be set beyond the pre-defined frame period; in another words, if the frame period needs to be extended to extend exposure time, then the maximum frame value in 0x350C/0x350D needs to be set first, then the exposure can be set in registers 0x3500~0x3502 accordingly. I know it's a different sensor, so the implementation could be different, but the register addresses and names seem to match. For what it's worth, "PK" could stand for "power keep" according to table 7-5 in that document. I have no idea what that means though :-) [1] https://www.uctronics.com/download/cam_module/OV5642DS.pdf > Anyway, being aware of such margins being usually required, I used the > min VBLANK value. Which is also not documented but I experimentally > tried to progressively shrink until capture did not break. > > Should I remove it from the above calculation ? More research, experimentation and reverse engineering is likely required to figure this out, but I think it's a bit out of scope. Whatever we do now will likely be wrong somehow anyway, so you can pick the option that you think is the most logical, and we'll fix that later if/when needed. > > > + exposure_val = clamp((s64)sensor->ctrls.exposure->val, > > > + (s64)sensor->ctrls.exposure->minimum, > > > + (s64)exposure_max); > > > + __v4l2_ctrl_modify_range(sensor->ctrls.exposure, > > > + sensor->ctrls.exposure->minimum, > > > + exposure_max, 1, exposure_val); > > > + > > > return 0; > > > } > > > > > > @@ -2958,6 +2978,15 @@ static int ov5640_set_ctrl_vflip(struct ov5640_dev *sensor, int value) > > > (BIT(2) | BIT(1)) : 0); > > > } > > > > > > +static int ov5640_set_ctrl_vblank(struct ov5640_dev *sensor, int value) > > > +{ > > > + const struct ov5640_mode_info *mode = sensor->current_mode; > > > + > > > + /* Update the VTOT timing register value. */ > > > + return ov5640_write_reg16(sensor, OV5640_REG_TIMING_VTS, > > > + mode->crop.height + value); > > > +} > > > + > > > static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl) > > > { > > > struct v4l2_subdev *sd = ctrl_to_sd(ctrl); > > > @@ -2988,10 +3017,22 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) > > > { > > > struct v4l2_subdev *sd = ctrl_to_sd(ctrl); > > > struct ov5640_dev *sensor = to_ov5640_dev(sd); > > > + const struct ov5640_mode_info *mode = sensor->current_mode; > > > int ret; > > > > > > /* v4l2_ctrl_lock() locks our own mutex */ > > > > > > + switch (ctrl->id) { > > > + case V4L2_CID_VBLANK: > > > + /* Update the exposure range to the newly programmed vblank. */ > > > + unsigned int max = mode->crop.height + ctrl->val - OV5640_MIN_VBLANK; > > > + > > > + __v4l2_ctrl_modify_range(sensor->ctrls.exposure, > > > + sensor->ctrls.exposure->minimum, > > > + max, sensor->ctrls.exposure->step, max); > > > > Should the default be set to mode->vblank_def ? > > > > It should! > > > > + break; > > > + } > > > + > > > /* > > > * If the device is not powered up by the host driver do > > > * not apply any controls to H/W at this time. Instead > > > @@ -3031,6 +3072,9 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) > > > case V4L2_CID_VFLIP: > > > ret = ov5640_set_ctrl_vflip(sensor, ctrl->val); > > > break; > > > + case V4L2_CID_VBLANK: > > > + ret = ov5640_set_ctrl_vblank(sensor, ctrl->val); > > > + break; > > > default: > > > ret = -EINVAL; > > > break; > > > @@ -3050,6 +3094,7 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) > > > const struct v4l2_ctrl_ops *ops = &ov5640_ctrl_ops; > > > struct ov5640_ctrls *ctrls = &sensor->ctrls; > > > struct v4l2_ctrl_handler *hdl = &ctrls->handler; > > > + unsigned int max_vblank; > > > unsigned int hblank; > > > int ret; > > > > > > @@ -3073,6 +3118,11 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) > > > ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, hblank, > > > hblank, 1, hblank); > > > > > > + max_vblank = OV5640_MAX_VTS - mode->crop.height; > > > + ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, > > > + OV5640_MIN_VBLANK, max_vblank, > > > + 1, mode->vblank_def); > > > + > > > /* Auto/manual white balance */ > > > ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops, > > > V4L2_CID_AUTO_WHITE_BALANCE,
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index b2961e8e07c3..6eeb50724195 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -31,6 +31,10 @@ #define OV5640_LINK_RATE_MAX 490000000U +/* FIXME: not documented. */ +#define OV5640_MIN_VBLANK 24 +#define OV5640_MAX_VTS 1968 + #define OV5640_DEFAULT_SLAVE_ID 0x3c #define OV5640_REG_SYS_RESET02 0x3002 @@ -267,6 +271,7 @@ struct ov5640_ctrls { struct v4l2_ctrl *pixel_rate; struct v4l2_ctrl *link_freq; struct v4l2_ctrl *hblank; + struct v4l2_ctrl *vblank; struct { struct v4l2_ctrl *auto_exp; struct v4l2_ctrl *exposure; @@ -2531,6 +2536,7 @@ static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) struct v4l2_mbus_framefmt *fmt = &sensor->fmt; enum ov5640_pixel_rate_id pixel_rate_id = mode->pixel_rate; u32 num_lanes = sensor->ep.bus.mipi_csi2.num_data_lanes; + s64 exposure_val, exposure_max; unsigned int hblank; unsigned int i = 0; u32 pixel_rate; @@ -2586,6 +2592,20 @@ static int ov5640_update_pixel_rate(struct ov5640_dev *sensor) __v4l2_ctrl_modify_range(sensor->ctrls.hblank, hblank, hblank, 1, hblank); + __v4l2_ctrl_modify_range(sensor->ctrls.vblank, + OV5640_MIN_VBLANK, + OV5640_MAX_VTS - mode->crop.height, 1, + mode->vblank_def); + __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, mode->vblank_def); + + exposure_max = mode->crop.height + mode->vblank_def - OV5640_MIN_VBLANK; + exposure_val = clamp((s64)sensor->ctrls.exposure->val, + (s64)sensor->ctrls.exposure->minimum, + (s64)exposure_max); + __v4l2_ctrl_modify_range(sensor->ctrls.exposure, + sensor->ctrls.exposure->minimum, + exposure_max, 1, exposure_val); + return 0; } @@ -2958,6 +2978,15 @@ static int ov5640_set_ctrl_vflip(struct ov5640_dev *sensor, int value) (BIT(2) | BIT(1)) : 0); } +static int ov5640_set_ctrl_vblank(struct ov5640_dev *sensor, int value) +{ + const struct ov5640_mode_info *mode = sensor->current_mode; + + /* Update the VTOT timing register value. */ + return ov5640_write_reg16(sensor, OV5640_REG_TIMING_VTS, + mode->crop.height + value); +} + static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl) { struct v4l2_subdev *sd = ctrl_to_sd(ctrl); @@ -2988,10 +3017,22 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) { struct v4l2_subdev *sd = ctrl_to_sd(ctrl); struct ov5640_dev *sensor = to_ov5640_dev(sd); + const struct ov5640_mode_info *mode = sensor->current_mode; int ret; /* v4l2_ctrl_lock() locks our own mutex */ + switch (ctrl->id) { + case V4L2_CID_VBLANK: + /* Update the exposure range to the newly programmed vblank. */ + unsigned int max = mode->crop.height + ctrl->val - OV5640_MIN_VBLANK; + + __v4l2_ctrl_modify_range(sensor->ctrls.exposure, + sensor->ctrls.exposure->minimum, + max, sensor->ctrls.exposure->step, max); + break; + } + /* * If the device is not powered up by the host driver do * not apply any controls to H/W at this time. Instead @@ -3031,6 +3072,9 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) case V4L2_CID_VFLIP: ret = ov5640_set_ctrl_vflip(sensor, ctrl->val); break; + case V4L2_CID_VBLANK: + ret = ov5640_set_ctrl_vblank(sensor, ctrl->val); + break; default: ret = -EINVAL; break; @@ -3050,6 +3094,7 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) const struct v4l2_ctrl_ops *ops = &ov5640_ctrl_ops; struct ov5640_ctrls *ctrls = &sensor->ctrls; struct v4l2_ctrl_handler *hdl = &ctrls->handler; + unsigned int max_vblank; unsigned int hblank; int ret; @@ -3073,6 +3118,11 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, hblank, hblank, 1, hblank); + max_vblank = OV5640_MAX_VTS - mode->crop.height; + ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, + OV5640_MIN_VBLANK, max_vblank, + 1, mode->vblank_def); + /* Auto/manual white balance */ ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTO_WHITE_BALANCE,
Add the VBLANK control which allows to select the duration of the frame vertical blankings and allows to control the framerate. The VBLANK control also modifies the exposure time range, which cannot exceed the maximum frame length. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- drivers/media/i2c/ov5640.c | 50 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)