| Message ID | 20241207-imx214-v3-7-ab60af7ee915@apitzsch.eu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: i2c: imx214: Miscellaneous cleanups and improvements | expand |
In general it looks good to me (besides the comments, ignore the nits if you want to). I'd recommend that you test with lockdep to make sure that we are not missing anything, and I'd like to hear back from Sakari regarding the get_locked_active Thanks! On Sat, Dec 7, 2024 at 9:48 PM André Apitzsch via B4 Relay <devnull+git.apitzsch.eu@kernel.org> wrote: > > From: André Apitzsch <git@apitzsch.eu> > > Add vblank control to allow changing the framerate / > higher exposure values. > > The vblank and hblank controls are needed for libcamera support. > > While at it, fix the minimal exposure time according to the datasheet. > > Signed-off-by: André Apitzsch <git@apitzsch.eu> > --- > drivers/media/i2c/imx214.c | 106 ++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 94 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c > index f1c72db0775eaf4810f762e8798d301c5ad9923c..a7f49dbafe0f54af3c02f5534460fdee88a22fe2 100644 > --- a/drivers/media/i2c/imx214.c > +++ b/drivers/media/i2c/imx214.c > @@ -34,11 +34,17 @@ > > /* V-TIMING internal */ > #define IMX214_REG_FRM_LENGTH_LINES CCI_REG16(0x0340) > +#define IMX214_VTS_MAX 0xffff > + > +#define IMX214_VBLANK_MIN 890 > + > +/* HBLANK control - read only */ > +#define IMX214_PPL_DEFAULT 5008 > > /* Exposure control */ > #define IMX214_REG_EXPOSURE CCI_REG16(0x0202) > -#define IMX214_EXPOSURE_MIN 0 > -#define IMX214_EXPOSURE_MAX 3184 > +#define IMX214_EXPOSURE_OFFSET 10 > +#define IMX214_EXPOSURE_MIN 1 > #define IMX214_EXPOSURE_STEP 1 > #define IMX214_EXPOSURE_DEFAULT 3184 > #define IMX214_REG_EXPOSURE_RATIO CCI_REG8(0x0222) > @@ -187,6 +193,8 @@ struct imx214 { > struct v4l2_ctrl_handler ctrls; > struct v4l2_ctrl *pixel_rate; > struct v4l2_ctrl *link_freq; > + struct v4l2_ctrl *vblank; > + struct v4l2_ctrl *hblank; > struct v4l2_ctrl *exposure; > struct v4l2_ctrl *unit_size; > > @@ -202,8 +210,6 @@ static const struct cci_reg_sequence mode_4096x2304[] = { > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF }, > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH }, > { IMX214_REG_EXPOSURE_RATIO, 1 }, > - { IMX214_REG_FRM_LENGTH_LINES, 3194 }, > - { IMX214_REG_LINE_LENGTH_PCK, 5008 }, > { IMX214_REG_X_ADD_STA, 56 }, > { IMX214_REG_Y_ADD_STA, 408 }, > { IMX214_REG_X_ADD_END, 4151 }, > @@ -274,8 +280,6 @@ static const struct cci_reg_sequence mode_1920x1080[] = { > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF }, > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH }, > { IMX214_REG_EXPOSURE_RATIO, 1 }, > - { IMX214_REG_FRM_LENGTH_LINES, 3194 }, > - { IMX214_REG_LINE_LENGTH_PCK, 5008 }, > { IMX214_REG_X_ADD_STA, 1144 }, > { IMX214_REG_Y_ADD_STA, 1020 }, > { IMX214_REG_X_ADD_END, 3063 }, > @@ -359,6 +363,7 @@ static const struct cci_reg_sequence mode_table_common[] = { > { IMX214_REG_ORIENTATION, 0 }, > { IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK }, > { IMX214_REG_FAST_STANDBY_CTRL, 1 }, > + { IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT }, > { CCI_REG8(0x4550), 0x02 }, > { CCI_REG8(0x4601), 0x00 }, > { CCI_REG8(0x4642), 0x05 }, > @@ -462,18 +467,24 @@ static const struct cci_reg_sequence mode_table_common[] = { > static const struct imx214_mode { > u32 width; > u32 height; > + > + /* V-timing */ > + unsigned int vts_def; > + > unsigned int num_of_regs; > const struct cci_reg_sequence *reg_table; > } imx214_modes[] = { > { > .width = 4096, > .height = 2304, > + .vts_def = 3194, > .num_of_regs = ARRAY_SIZE(mode_4096x2304), > .reg_table = mode_4096x2304, > }, > { > .width = 1920, > .height = 1080, > + .vts_def = 3194, > .num_of_regs = ARRAY_SIZE(mode_1920x1080), > .reg_table = mode_1920x1080, > }, > @@ -626,9 +637,36 @@ static int imx214_set_format(struct v4l2_subdev *sd, > __crop->width = mode->width; > __crop->height = mode->height; > > - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > + int exposure_max; > + int exposure_def; > + int hblank; > + > imx214->cur_mode = mode; > > + /* Update FPS limits */ nit: Update blank limits > + __v4l2_ctrl_modify_range(imx214->vblank, IMX214_VBLANK_MIN, > + IMX214_VTS_MAX - mode->height, 2, > + mode->vts_def - mode->height); Is the handler->lock held when we call this function? Can you try running the code with lockdep? > + > + /* Update max exposure while meeting expected vblanking */ > + exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET; > + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT); > + __v4l2_ctrl_modify_range(imx214->exposure, > + imx214->exposure->minimum, > + exposure_max, imx214->exposure->step, > + exposure_def); > + > + /* > + * Currently PPL is fixed to IMX214_PPL_DEFAULT, so hblank > + * depends on mode->width only, and is not changeable in any > + * way other than changing the mode. > + */ > + hblank = IMX214_PPL_DEFAULT - mode->width; > + __v4l2_ctrl_modify_range(imx214->hblank, hblank, hblank, 1, > + hblank); > + } > + > return 0; > } > > @@ -678,8 +716,25 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl) > { > struct imx214 *imx214 = container_of(ctrl->handler, > struct imx214, ctrls); > + const struct v4l2_mbus_framefmt *format; > + struct v4l2_subdev_state *state; > int ret; > > + if (ctrl->id == V4L2_CID_VBLANK) { > + int exposure_max, exposure_def; > + > + state = v4l2_subdev_get_locked_active_state(&imx214->sd); Sakari, I see that other drivers assume that the active is locked in set_ctrl. Is this correct? > + format = v4l2_subdev_state_get_format(state, 0); > + > + /* Update max exposure while meeting expected vblanking */ > + exposure_max = format->height + ctrl->val - IMX214_EXPOSURE_OFFSET; > + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT); > + __v4l2_ctrl_modify_range(imx214->exposure, > + imx214->exposure->minimum, > + exposure_max, imx214->exposure->step, > + exposure_def); > + } > + > /* > * Applying V4L2 control value only happens > * when power is up for streaming > @@ -691,7 +746,10 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl) > case V4L2_CID_EXPOSURE: > cci_write(imx214->regmap, IMX214_REG_EXPOSURE, ctrl->val, &ret); > break; > - > + case V4L2_CID_VBLANK: No need to read the format here? format = v4l2_subdev_state_get_format(state, 0); > + cci_write(imx214->regmap, IMX214_REG_FRM_LENGTH_LINES, > + format->height + ctrl->val, &ret); > + break; > default: > ret = -EINVAL; > } > @@ -714,8 +772,11 @@ static int imx214_ctrls_init(struct imx214 *imx214) > .width = 1120, > .height = 1120, > }; > + const struct imx214_mode *mode = &imx214_modes[0]; > struct v4l2_fwnode_device_properties props; > struct v4l2_ctrl_handler *ctrl_hdlr; > + int exposure_max, exposure_def; > + int hblank; > int ret; > > ret = v4l2_fwnode_device_parse(imx214->dev, &props); > @@ -723,7 +784,7 @@ static int imx214_ctrls_init(struct imx214 *imx214) > return ret; > > ctrl_hdlr = &imx214->ctrls; > - ret = v4l2_ctrl_handler_init(&imx214->ctrls, 6); > + ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8); > if (ret) > return ret; > > @@ -749,12 +810,27 @@ static int imx214_ctrls_init(struct imx214 *imx214) > * > * Yours sincerely, Ricardo. > */ > + > + /* Initial vblank/hblank/exposure parameters based on current mode */ > + imx214->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops, > + V4L2_CID_VBLANK, IMX214_VBLANK_MIN, > + IMX214_VTS_MAX - mode->height, 2, > + mode->vts_def - mode->height); > + > + hblank = IMX214_PPL_DEFAULT - mode->width; > + imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops, > + V4L2_CID_HBLANK, hblank, hblank, > + 1, hblank); > + if (imx214->hblank) > + imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET; > + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT); > imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops, > V4L2_CID_EXPOSURE, > - IMX214_EXPOSURE_MIN, > - IMX214_EXPOSURE_MAX, > + IMX214_EXPOSURE_MIN, exposure_max, nit: I think it looks nicer with exposure_max in the next line, but ignore if you prefer this way :) > IMX214_EXPOSURE_STEP, > - IMX214_EXPOSURE_DEFAULT); > + exposure_def); > > imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr, > NULL, > @@ -876,6 +952,12 @@ static int imx214_get_frame_interval(struct v4l2_subdev *subdev, > return 0; > } > > +/* > + * Raw sensors should be using the VBLANK and HBLANK controls to determine > + * the frame rate. However this driver was initially added using the > + * [S|G|ENUM]_FRAME_INTERVAL ioctls with a fixed rate of 30fps. > + * Retain the frame_interval ops for backwards compatibility, but they do nothing. > + */ Now that these controls are useless... maybe we can do a dev_warn_once when the user calls it to leave some output in dmesg? > static int imx214_enum_frame_interval(struct v4l2_subdev *subdev, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_frame_interval_enum *fie) > > -- > 2.47.1 > >
On Sun, Dec 8, 2024 at 9:59 PM Ricardo Ribalda Delgado <ribalda@kernel.org> wrote: > > In general it looks good to me (besides the comments, ignore the nits > if you want to). > > I'd recommend that you test with lockdep to make sure that we are not > missing anything, and I'd like to hear back from Sakari regarding the > get_locked_active > > Thanks! > > On Sat, Dec 7, 2024 at 9:48 PM André Apitzsch via B4 Relay > <devnull+git.apitzsch.eu@kernel.org> wrote: > > > > From: André Apitzsch <git@apitzsch.eu> > > > > Add vblank control to allow changing the framerate / > > higher exposure values. > > > > The vblank and hblank controls are needed for libcamera support. > > > > While at it, fix the minimal exposure time according to the datasheet. > > > > Signed-off-by: André Apitzsch <git@apitzsch.eu> > > --- > > drivers/media/i2c/imx214.c | 106 ++++++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 94 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c > > index f1c72db0775eaf4810f762e8798d301c5ad9923c..a7f49dbafe0f54af3c02f5534460fdee88a22fe2 100644 > > --- a/drivers/media/i2c/imx214.c > > +++ b/drivers/media/i2c/imx214.c > > @@ -34,11 +34,17 @@ > > > > /* V-TIMING internal */ > > #define IMX214_REG_FRM_LENGTH_LINES CCI_REG16(0x0340) > > +#define IMX214_VTS_MAX 0xffff > > + > > +#define IMX214_VBLANK_MIN 890 > > + > > +/* HBLANK control - read only */ > > +#define IMX214_PPL_DEFAULT 5008 > > > > /* Exposure control */ > > #define IMX214_REG_EXPOSURE CCI_REG16(0x0202) > > -#define IMX214_EXPOSURE_MIN 0 > > -#define IMX214_EXPOSURE_MAX 3184 > > +#define IMX214_EXPOSURE_OFFSET 10 > > +#define IMX214_EXPOSURE_MIN 1 > > #define IMX214_EXPOSURE_STEP 1 > > #define IMX214_EXPOSURE_DEFAULT 3184 > > #define IMX214_REG_EXPOSURE_RATIO CCI_REG8(0x0222) > > @@ -187,6 +193,8 @@ struct imx214 { > > struct v4l2_ctrl_handler ctrls; > > struct v4l2_ctrl *pixel_rate; > > struct v4l2_ctrl *link_freq; > > + struct v4l2_ctrl *vblank; > > + struct v4l2_ctrl *hblank; > > struct v4l2_ctrl *exposure; > > struct v4l2_ctrl *unit_size; > > > > @@ -202,8 +210,6 @@ static const struct cci_reg_sequence mode_4096x2304[] = { > > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF }, > > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH }, > > { IMX214_REG_EXPOSURE_RATIO, 1 }, > > - { IMX214_REG_FRM_LENGTH_LINES, 3194 }, > > - { IMX214_REG_LINE_LENGTH_PCK, 5008 }, > > { IMX214_REG_X_ADD_STA, 56 }, > > { IMX214_REG_Y_ADD_STA, 408 }, > > { IMX214_REG_X_ADD_END, 4151 }, > > @@ -274,8 +280,6 @@ static const struct cci_reg_sequence mode_1920x1080[] = { > > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF }, > > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH }, > > { IMX214_REG_EXPOSURE_RATIO, 1 }, > > - { IMX214_REG_FRM_LENGTH_LINES, 3194 }, > > - { IMX214_REG_LINE_LENGTH_PCK, 5008 }, > > { IMX214_REG_X_ADD_STA, 1144 }, > > { IMX214_REG_Y_ADD_STA, 1020 }, > > { IMX214_REG_X_ADD_END, 3063 }, > > @@ -359,6 +363,7 @@ static const struct cci_reg_sequence mode_table_common[] = { > > { IMX214_REG_ORIENTATION, 0 }, > > { IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK }, > > { IMX214_REG_FAST_STANDBY_CTRL, 1 }, > > + { IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT }, > > { CCI_REG8(0x4550), 0x02 }, > > { CCI_REG8(0x4601), 0x00 }, > > { CCI_REG8(0x4642), 0x05 }, > > @@ -462,18 +467,24 @@ static const struct cci_reg_sequence mode_table_common[] = { > > static const struct imx214_mode { > > u32 width; > > u32 height; > > + > > + /* V-timing */ > > + unsigned int vts_def; > > + > > unsigned int num_of_regs; > > const struct cci_reg_sequence *reg_table; > > } imx214_modes[] = { > > { > > .width = 4096, > > .height = 2304, > > + .vts_def = 3194, > > .num_of_regs = ARRAY_SIZE(mode_4096x2304), > > .reg_table = mode_4096x2304, > > }, > > { > > .width = 1920, > > .height = 1080, > > + .vts_def = 3194, > > .num_of_regs = ARRAY_SIZE(mode_1920x1080), > > .reg_table = mode_1920x1080, > > }, > > @@ -626,9 +637,36 @@ static int imx214_set_format(struct v4l2_subdev *sd, > > __crop->width = mode->width; > > __crop->height = mode->height; > > > > - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) > > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > > + int exposure_max; > > + int exposure_def; > > + int hblank; > > + > > imx214->cur_mode = mode; > > > > + /* Update FPS limits */ > nit: Update blank limits > > + __v4l2_ctrl_modify_range(imx214->vblank, IMX214_VBLANK_MIN, > > + IMX214_VTS_MAX - mode->height, 2, > > + mode->vts_def - mode->height); > > Is the handler->lock held when we call this function? Can you try > running the code with lockdep? > > + > > + /* Update max exposure while meeting expected vblanking */ > > + exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET; > > + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT); > > + __v4l2_ctrl_modify_range(imx214->exposure, > > + imx214->exposure->minimum, > > + exposure_max, imx214->exposure->step, > > + exposure_def); > > + > > + /* > > + * Currently PPL is fixed to IMX214_PPL_DEFAULT, so hblank > > + * depends on mode->width only, and is not changeable in any > > + * way other than changing the mode. > > + */ > > + hblank = IMX214_PPL_DEFAULT - mode->width; > > + __v4l2_ctrl_modify_range(imx214->hblank, hblank, hblank, 1, > > + hblank); > > + } > > + > > return 0; > > } > > > > @@ -678,8 +716,25 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl) > > { > > struct imx214 *imx214 = container_of(ctrl->handler, > > struct imx214, ctrls); > > + const struct v4l2_mbus_framefmt *format; > > + struct v4l2_subdev_state *state; > > int ret; > > > > + if (ctrl->id == V4L2_CID_VBLANK) { > > + int exposure_max, exposure_def; > > + > > + state = v4l2_subdev_get_locked_active_state(&imx214->sd); > > Sakari, I see that other drivers assume that the active is locked in > set_ctrl. Is this correct? imx214->sd.state_lock = imx214->ctrls.lock; So I guess it is fine :) > > > + format = v4l2_subdev_state_get_format(state, 0); > > + > > + /* Update max exposure while meeting expected vblanking */ > > + exposure_max = format->height + ctrl->val - IMX214_EXPOSURE_OFFSET; > > + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT); > > + __v4l2_ctrl_modify_range(imx214->exposure, > > + imx214->exposure->minimum, > > + exposure_max, imx214->exposure->step, > > + exposure_def); > > + } > > + > > /* > > * Applying V4L2 control value only happens > > * when power is up for streaming > > @@ -691,7 +746,10 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl) > > case V4L2_CID_EXPOSURE: > > cci_write(imx214->regmap, IMX214_REG_EXPOSURE, ctrl->val, &ret); > > break; > > - > > + case V4L2_CID_VBLANK: > No need to read the format here? > format = v4l2_subdev_state_get_format(state, 0); > > + cci_write(imx214->regmap, IMX214_REG_FRM_LENGTH_LINES, > > + format->height + ctrl->val, &ret); > > + break; > > default: > > ret = -EINVAL; > > } > > @@ -714,8 +772,11 @@ static int imx214_ctrls_init(struct imx214 *imx214) > > .width = 1120, > > .height = 1120, > > }; > > + const struct imx214_mode *mode = &imx214_modes[0]; > > struct v4l2_fwnode_device_properties props; > > struct v4l2_ctrl_handler *ctrl_hdlr; > > + int exposure_max, exposure_def; > > + int hblank; > > int ret; > > > > ret = v4l2_fwnode_device_parse(imx214->dev, &props); > > @@ -723,7 +784,7 @@ static int imx214_ctrls_init(struct imx214 *imx214) > > return ret; > > > > ctrl_hdlr = &imx214->ctrls; > > - ret = v4l2_ctrl_handler_init(&imx214->ctrls, 6); > > + ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8); > > if (ret) > > return ret; > > > > @@ -749,12 +810,27 @@ static int imx214_ctrls_init(struct imx214 *imx214) > > * > > * Yours sincerely, Ricardo. > > */ > > + > > + /* Initial vblank/hblank/exposure parameters based on current mode */ > > + imx214->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops, > > + V4L2_CID_VBLANK, IMX214_VBLANK_MIN, > > + IMX214_VTS_MAX - mode->height, 2, > > + mode->vts_def - mode->height); > > + > > + hblank = IMX214_PPL_DEFAULT - mode->width; > > + imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops, > > + V4L2_CID_HBLANK, hblank, hblank, > > + 1, hblank); > > + if (imx214->hblank) > > + imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > + > > + exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET; > > + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT); > > imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops, > > V4L2_CID_EXPOSURE, > > - IMX214_EXPOSURE_MIN, > > - IMX214_EXPOSURE_MAX, > > + IMX214_EXPOSURE_MIN, exposure_max, > nit: I think it looks nicer with exposure_max in the next line, but > ignore if you prefer this way :) > > IMX214_EXPOSURE_STEP, > > - IMX214_EXPOSURE_DEFAULT); > > + exposure_def); > > > > imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr, > > NULL, > > @@ -876,6 +952,12 @@ static int imx214_get_frame_interval(struct v4l2_subdev *subdev, > > return 0; > > } > > > > +/* > > + * Raw sensors should be using the VBLANK and HBLANK controls to determine > > + * the frame rate. However this driver was initially added using the > > + * [S|G|ENUM]_FRAME_INTERVAL ioctls with a fixed rate of 30fps. > > + * Retain the frame_interval ops for backwards compatibility, but they do nothing. > > + */ > > Now that these controls are useless... maybe we can do a dev_warn_once > when the user calls it to leave some output in dmesg? > > static int imx214_enum_frame_interval(struct v4l2_subdev *subdev, > > struct v4l2_subdev_state *sd_state, > > struct v4l2_subdev_frame_interval_enum *fie) > > > > -- > > 2.47.1 > > > >
Hi Ricardo, Am Sonntag, dem 08.12.2024 um 21:59 +0100 schrieb Ricardo Ribalda Delgado: > In general it looks good to me (besides the comments, ignore the nits > if you want to). > > I'd recommend that you test with lockdep to make sure that we are not > missing anything, and I'd like to hear back from Sakari regarding the > get_locked_active > > Thanks! > > On Sat, Dec 7, 2024 at 9:48 PM André Apitzsch via B4 Relay > <devnull+git.apitzsch.eu@kernel.org> wrote: > > > > From: André Apitzsch <git@apitzsch.eu> > > > > Add vblank control to allow changing the framerate / > > higher exposure values. > > > > The vblank and hblank controls are needed for libcamera support. > > > > While at it, fix the minimal exposure time according to the > > datasheet. > > > > Signed-off-by: André Apitzsch <git@apitzsch.eu> > > --- > > drivers/media/i2c/imx214.c | 106 > > ++++++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 94 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/media/i2c/imx214.c > > b/drivers/media/i2c/imx214.c > > index > > f1c72db0775eaf4810f762e8798d301c5ad9923c..a7f49dbafe0f54af3c02f5534 > > 460fdee88a22fe2 100644 > > --- a/drivers/media/i2c/imx214.c > > +++ b/drivers/media/i2c/imx214.c > > @@ -34,11 +34,17 @@ > > > > /* V-TIMING internal */ > > #define IMX214_REG_FRM_LENGTH_LINES CCI_REG16(0x0340) > > +#define IMX214_VTS_MAX 0xffff > > + > > +#define IMX214_VBLANK_MIN 890 > > + > > +/* HBLANK control - read only */ > > +#define IMX214_PPL_DEFAULT 5008 > > > > /* Exposure control */ > > #define IMX214_REG_EXPOSURE CCI_REG16(0x0202) > > -#define IMX214_EXPOSURE_MIN 0 > > -#define IMX214_EXPOSURE_MAX 3184 > > +#define IMX214_EXPOSURE_OFFSET 10 > > +#define IMX214_EXPOSURE_MIN 1 > > #define IMX214_EXPOSURE_STEP 1 > > #define IMX214_EXPOSURE_DEFAULT 3184 > > #define IMX214_REG_EXPOSURE_RATIO CCI_REG8(0x0222) > > @@ -187,6 +193,8 @@ struct imx214 { > > struct v4l2_ctrl_handler ctrls; > > struct v4l2_ctrl *pixel_rate; > > struct v4l2_ctrl *link_freq; > > + struct v4l2_ctrl *vblank; > > + struct v4l2_ctrl *hblank; > > struct v4l2_ctrl *exposure; > > struct v4l2_ctrl *unit_size; > > > > @@ -202,8 +210,6 @@ static const struct cci_reg_sequence > > mode_4096x2304[] = { > > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF }, > > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH > > }, > > { IMX214_REG_EXPOSURE_RATIO, 1 }, > > - { IMX214_REG_FRM_LENGTH_LINES, 3194 }, > > - { IMX214_REG_LINE_LENGTH_PCK, 5008 }, > > { IMX214_REG_X_ADD_STA, 56 }, > > { IMX214_REG_Y_ADD_STA, 408 }, > > { IMX214_REG_X_ADD_END, 4151 }, > > @@ -274,8 +280,6 @@ static const struct cci_reg_sequence > > mode_1920x1080[] = { > > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF }, > > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH > > }, > > { IMX214_REG_EXPOSURE_RATIO, 1 }, > > - { IMX214_REG_FRM_LENGTH_LINES, 3194 }, > > - { IMX214_REG_LINE_LENGTH_PCK, 5008 }, > > { IMX214_REG_X_ADD_STA, 1144 }, > > { IMX214_REG_Y_ADD_STA, 1020 }, > > { IMX214_REG_X_ADD_END, 3063 }, > > @@ -359,6 +363,7 @@ static const struct cci_reg_sequence > > mode_table_common[] = { > > { IMX214_REG_ORIENTATION, 0 }, > > { IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK }, > > { IMX214_REG_FAST_STANDBY_CTRL, 1 }, > > + { IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT }, > > { CCI_REG8(0x4550), 0x02 }, > > { CCI_REG8(0x4601), 0x00 }, > > { CCI_REG8(0x4642), 0x05 }, > > @@ -462,18 +467,24 @@ static const struct cci_reg_sequence > > mode_table_common[] = { > > static const struct imx214_mode { > > u32 width; > > u32 height; > > + > > + /* V-timing */ > > + unsigned int vts_def; > > + > > unsigned int num_of_regs; > > const struct cci_reg_sequence *reg_table; > > } imx214_modes[] = { > > { > > .width = 4096, > > .height = 2304, > > + .vts_def = 3194, > > .num_of_regs = ARRAY_SIZE(mode_4096x2304), > > .reg_table = mode_4096x2304, > > }, > > { > > .width = 1920, > > .height = 1080, > > + .vts_def = 3194, > > .num_of_regs = ARRAY_SIZE(mode_1920x1080), > > .reg_table = mode_1920x1080, > > }, > > @@ -626,9 +637,36 @@ static int imx214_set_format(struct > > v4l2_subdev *sd, > > __crop->width = mode->width; > > __crop->height = mode->height; > > > > - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) > > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > > + int exposure_max; > > + int exposure_def; > > + int hblank; > > + > > imx214->cur_mode = mode; > > > > + /* Update FPS limits */ > nit: Update blank limits > > + __v4l2_ctrl_modify_range(imx214->vblank, > > IMX214_VBLANK_MIN, > > + IMX214_VTS_MAX - mode- > > >height, 2, > > + mode->vts_def - mode- > > >height); > > Is the handler->lock held when we call this function? Can you try > running the code with lockdep? > > + > > + /* Update max exposure while meeting expected > > vblanking */ > > + exposure_max = mode->vts_def - > > IMX214_EXPOSURE_OFFSET; > > + exposure_def = min(exposure_max, > > IMX214_EXPOSURE_DEFAULT); > > + __v4l2_ctrl_modify_range(imx214->exposure, > > + imx214->exposure->minimum, > > + exposure_max, imx214- > > >exposure->step, > > + exposure_def); > > + > > + /* > > + * Currently PPL is fixed to IMX214_PPL_DEFAULT, so > > hblank > > + * depends on mode->width only, and is not > > changeable in any > > + * way other than changing the mode. > > + */ > > + hblank = IMX214_PPL_DEFAULT - mode->width; > > + __v4l2_ctrl_modify_range(imx214->hblank, hblank, > > hblank, 1, > > + hblank); > > + } > > + > > return 0; > > } > > > > @@ -678,8 +716,25 @@ static int imx214_set_ctrl(struct v4l2_ctrl > > *ctrl) > > { > > struct imx214 *imx214 = container_of(ctrl->handler, > > struct imx214, ctrls); > > + const struct v4l2_mbus_framefmt *format; > > + struct v4l2_subdev_state *state; > > int ret; > > > > + if (ctrl->id == V4L2_CID_VBLANK) { > > + int exposure_max, exposure_def; > > + > > + state = > > v4l2_subdev_get_locked_active_state(&imx214->sd); > > Sakari, I see that other drivers assume that the active is locked in > set_ctrl. Is this correct? > > > + format = v4l2_subdev_state_get_format(state, 0); (*) The format is read here. > > + > > + /* Update max exposure while meeting expected > > vblanking */ > > + exposure_max = format->height + ctrl->val - > > IMX214_EXPOSURE_OFFSET; > > + exposure_def = min(exposure_max, > > IMX214_EXPOSURE_DEFAULT); > > + __v4l2_ctrl_modify_range(imx214->exposure, > > + imx214->exposure->minimum, > > + exposure_max, imx214- > > >exposure->step, > > + exposure_def); > > + } > > + > > /* > > * Applying V4L2 control value only happens > > * when power is up for streaming > > @@ -691,7 +746,10 @@ static int imx214_set_ctrl(struct v4l2_ctrl > > *ctrl) > > case V4L2_CID_EXPOSURE: > > cci_write(imx214->regmap, IMX214_REG_EXPOSURE, > > ctrl->val, &ret); > > break; > > - > > + case V4L2_CID_VBLANK: > No need to read the format here? > format = v4l2_subdev_state_get_format(state, > 0); The format is read above (*), so no need to do it here. Note, in both locations we have ctrl->id == V4L2_CID_VBLANK. > > + cci_write(imx214->regmap, > > IMX214_REG_FRM_LENGTH_LINES, > > + format->height + ctrl->val, &ret); > > + break; > > default: > > ret = -EINVAL; > > } > > @@ -714,8 +772,11 @@ static int imx214_ctrls_init(struct imx214 > > *imx214) > > .width = 1120, > > .height = 1120, > > }; > > + const struct imx214_mode *mode = &imx214_modes[0]; > > struct v4l2_fwnode_device_properties props; > > struct v4l2_ctrl_handler *ctrl_hdlr; > > + int exposure_max, exposure_def; > > + int hblank; > > int ret; > > > > ret = v4l2_fwnode_device_parse(imx214->dev, &props); > > @@ -723,7 +784,7 @@ static int imx214_ctrls_init(struct imx214 > > *imx214) > > return ret; > > > > ctrl_hdlr = &imx214->ctrls; > > - ret = v4l2_ctrl_handler_init(&imx214->ctrls, 6); > > + ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8); > > if (ret) > > return ret; > > > > @@ -749,12 +810,27 @@ static int imx214_ctrls_init(struct imx214 > > *imx214) > > * > > * Yours sincerely, Ricardo. > > */ > > + > > + /* Initial vblank/hblank/exposure parameters based on > > current mode */ > > + imx214->vblank = v4l2_ctrl_new_std(ctrl_hdlr, > > &imx214_ctrl_ops, > > + V4L2_CID_VBLANK, > > IMX214_VBLANK_MIN, > > + IMX214_VTS_MAX - mode- > > >height, 2, > > + mode->vts_def - mode- > > >height); > > + > > + hblank = IMX214_PPL_DEFAULT - mode->width; > > + imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr, > > &imx214_ctrl_ops, > > + V4L2_CID_HBLANK, hblank, > > hblank, > > + 1, hblank); > > + if (imx214->hblank) > > + imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > + > > + exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET; > > + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT); > > imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, > > &imx214_ctrl_ops, > > V4L2_CID_EXPOSURE, > > - IMX214_EXPOSURE_MIN, > > - IMX214_EXPOSURE_MAX, > > + IMX214_EXPOSURE_MIN, > > exposure_max, > nit: I think it looks nicer with exposure_max in the next line, but > ignore if you prefer this way :) > > IMX214_EXPOSURE_STEP, > > - > > IMX214_EXPOSURE_DEFAULT); > > + exposure_def); > > > > imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr, > > NULL, > > @@ -876,6 +952,12 @@ static int imx214_get_frame_interval(struct > > v4l2_subdev *subdev, > > return 0; > > } > > > > +/* > > + * Raw sensors should be using the VBLANK and HBLANK controls to > > determine > > + * the frame rate. However this driver was initially added using > > the > > + * [S|G|ENUM]_FRAME_INTERVAL ioctls with a fixed rate of 30fps. > > + * Retain the frame_interval ops for backwards compatibility, but > > they do nothing. > > + */ > > Now that these controls are useless... maybe we can do a > dev_warn_once > when the user calls it to leave some output in dmesg? Sure. We the following be a proper warning? "Use the VBLANK and HBLANK controls to determine the frame rate\n" > > static int imx214_enum_frame_interval(struct v4l2_subdev *subdev, > > struct v4l2_subdev_state *sd_state, > > struct > > v4l2_subdev_frame_interval_enum *fie) > > > > -- > > 2.47.1 > > > >
On Sun, Dec 8, 2024 at 10:35 PM André Apitzsch <git@apitzsch.eu> wrote: > > Hi Ricardo, > > Am Sonntag, dem 08.12.2024 um 21:59 +0100 schrieb Ricardo Ribalda > Delgado: > > In general it looks good to me (besides the comments, ignore the nits > > if you want to). > > > > I'd recommend that you test with lockdep to make sure that we are not > > missing anything, and I'd like to hear back from Sakari regarding the > > get_locked_active > > > > Thanks! > > > > On Sat, Dec 7, 2024 at 9:48 PM André Apitzsch via B4 Relay > > <devnull+git.apitzsch.eu@kernel.org> wrote: > > > > > > From: André Apitzsch <git@apitzsch.eu> > > > > > > Add vblank control to allow changing the framerate / > > > higher exposure values. > > > > > > The vblank and hblank controls are needed for libcamera support. > > > > > > While at it, fix the minimal exposure time according to the > > > datasheet. > > > > > > Signed-off-by: André Apitzsch <git@apitzsch.eu> > > > --- > > > drivers/media/i2c/imx214.c | 106 > > > ++++++++++++++++++++++++++++++++++++++++----- > > > 1 file changed, 94 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/media/i2c/imx214.c > > > b/drivers/media/i2c/imx214.c > > > index > > > f1c72db0775eaf4810f762e8798d301c5ad9923c..a7f49dbafe0f54af3c02f5534 > > > 460fdee88a22fe2 100644 > > > --- a/drivers/media/i2c/imx214.c > > > +++ b/drivers/media/i2c/imx214.c > > > @@ -34,11 +34,17 @@ > > > > > > /* V-TIMING internal */ > > > #define IMX214_REG_FRM_LENGTH_LINES CCI_REG16(0x0340) > > > +#define IMX214_VTS_MAX 0xffff > > > + > > > +#define IMX214_VBLANK_MIN 890 > > > + > > > +/* HBLANK control - read only */ > > > +#define IMX214_PPL_DEFAULT 5008 > > > > > > /* Exposure control */ > > > #define IMX214_REG_EXPOSURE CCI_REG16(0x0202) > > > -#define IMX214_EXPOSURE_MIN 0 > > > -#define IMX214_EXPOSURE_MAX 3184 > > > +#define IMX214_EXPOSURE_OFFSET 10 > > > +#define IMX214_EXPOSURE_MIN 1 > > > #define IMX214_EXPOSURE_STEP 1 > > > #define IMX214_EXPOSURE_DEFAULT 3184 > > > #define IMX214_REG_EXPOSURE_RATIO CCI_REG8(0x0222) > > > @@ -187,6 +193,8 @@ struct imx214 { > > > struct v4l2_ctrl_handler ctrls; > > > struct v4l2_ctrl *pixel_rate; > > > struct v4l2_ctrl *link_freq; > > > + struct v4l2_ctrl *vblank; > > > + struct v4l2_ctrl *hblank; > > > struct v4l2_ctrl *exposure; > > > struct v4l2_ctrl *unit_size; > > > > > > @@ -202,8 +210,6 @@ static const struct cci_reg_sequence > > > mode_4096x2304[] = { > > > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF }, > > > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH > > > }, > > > { IMX214_REG_EXPOSURE_RATIO, 1 }, > > > - { IMX214_REG_FRM_LENGTH_LINES, 3194 }, > > > - { IMX214_REG_LINE_LENGTH_PCK, 5008 }, > > > { IMX214_REG_X_ADD_STA, 56 }, > > > { IMX214_REG_Y_ADD_STA, 408 }, > > > { IMX214_REG_X_ADD_END, 4151 }, > > > @@ -274,8 +280,6 @@ static const struct cci_reg_sequence > > > mode_1920x1080[] = { > > > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF }, > > > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH > > > }, > > > { IMX214_REG_EXPOSURE_RATIO, 1 }, > > > - { IMX214_REG_FRM_LENGTH_LINES, 3194 }, > > > - { IMX214_REG_LINE_LENGTH_PCK, 5008 }, > > > { IMX214_REG_X_ADD_STA, 1144 }, > > > { IMX214_REG_Y_ADD_STA, 1020 }, > > > { IMX214_REG_X_ADD_END, 3063 }, > > > @@ -359,6 +363,7 @@ static const struct cci_reg_sequence > > > mode_table_common[] = { > > > { IMX214_REG_ORIENTATION, 0 }, > > > { IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK }, > > > { IMX214_REG_FAST_STANDBY_CTRL, 1 }, > > > + { IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT }, > > > { CCI_REG8(0x4550), 0x02 }, > > > { CCI_REG8(0x4601), 0x00 }, > > > { CCI_REG8(0x4642), 0x05 }, > > > @@ -462,18 +467,24 @@ static const struct cci_reg_sequence > > > mode_table_common[] = { > > > static const struct imx214_mode { > > > u32 width; > > > u32 height; > > > + > > > + /* V-timing */ > > > + unsigned int vts_def; > > > + > > > unsigned int num_of_regs; > > > const struct cci_reg_sequence *reg_table; > > > } imx214_modes[] = { > > > { > > > .width = 4096, > > > .height = 2304, > > > + .vts_def = 3194, > > > .num_of_regs = ARRAY_SIZE(mode_4096x2304), > > > .reg_table = mode_4096x2304, > > > }, > > > { > > > .width = 1920, > > > .height = 1080, > > > + .vts_def = 3194, > > > .num_of_regs = ARRAY_SIZE(mode_1920x1080), > > > .reg_table = mode_1920x1080, > > > }, > > > @@ -626,9 +637,36 @@ static int imx214_set_format(struct > > > v4l2_subdev *sd, > > > __crop->width = mode->width; > > > __crop->height = mode->height; > > > > > > - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) > > > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > > > + int exposure_max; > > > + int exposure_def; > > > + int hblank; > > > + > > > imx214->cur_mode = mode; > > > > > > + /* Update FPS limits */ > > nit: Update blank limits > > > + __v4l2_ctrl_modify_range(imx214->vblank, > > > IMX214_VBLANK_MIN, > > > + IMX214_VTS_MAX - mode- > > > >height, 2, > > > + mode->vts_def - mode- > > > >height); > > > > Is the handler->lock held when we call this function? Can you try > > running the code with lockdep? > > > + > > > + /* Update max exposure while meeting expected > > > vblanking */ > > > + exposure_max = mode->vts_def - > > > IMX214_EXPOSURE_OFFSET; > > > + exposure_def = min(exposure_max, > > > IMX214_EXPOSURE_DEFAULT); > > > + __v4l2_ctrl_modify_range(imx214->exposure, > > > + imx214->exposure->minimum, > > > + exposure_max, imx214- > > > >exposure->step, > > > + exposure_def); > > > + > > > + /* > > > + * Currently PPL is fixed to IMX214_PPL_DEFAULT, so > > > hblank > > > + * depends on mode->width only, and is not > > > changeable in any > > > + * way other than changing the mode. > > > + */ > > > + hblank = IMX214_PPL_DEFAULT - mode->width; > > > + __v4l2_ctrl_modify_range(imx214->hblank, hblank, > > > hblank, 1, > > > + hblank); > > > + } > > > + > > > return 0; > > > } > > > > > > @@ -678,8 +716,25 @@ static int imx214_set_ctrl(struct v4l2_ctrl > > > *ctrl) > > > { > > > struct imx214 *imx214 = container_of(ctrl->handler, > > > struct imx214, ctrls); > > > + const struct v4l2_mbus_framefmt *format; > > > + struct v4l2_subdev_state *state; > > > int ret; > > > > > > + if (ctrl->id == V4L2_CID_VBLANK) { > > > + int exposure_max, exposure_def; > > > + > > > + state = > > > v4l2_subdev_get_locked_active_state(&imx214->sd); > > > > Sakari, I see that other drivers assume that the active is locked in > > set_ctrl. Is this correct? > > > > > + format = v4l2_subdev_state_get_format(state, 0); > > (*) The format is read here. > > > > + > > > + /* Update max exposure while meeting expected > > > vblanking */ > > > + exposure_max = format->height + ctrl->val - > > > IMX214_EXPOSURE_OFFSET; > > > + exposure_def = min(exposure_max, > > > IMX214_EXPOSURE_DEFAULT); > > > + __v4l2_ctrl_modify_range(imx214->exposure, > > > + imx214->exposure->minimum, > > > + exposure_max, imx214- > > > >exposure->step, > > > + exposure_def); > > > + } > > > + > > > /* > > > * Applying V4L2 control value only happens > > > * when power is up for streaming > > > @@ -691,7 +746,10 @@ static int imx214_set_ctrl(struct v4l2_ctrl > > > *ctrl) > > > case V4L2_CID_EXPOSURE: > > > cci_write(imx214->regmap, IMX214_REG_EXPOSURE, > > > ctrl->val, &ret); > > > break; > > > - > > > + case V4L2_CID_VBLANK: > > No need to read the format here? > > format = v4l2_subdev_state_get_format(state, > > 0); > > The format is read above (*), so no need to do it here. Note, in both > locations we have ctrl->id == V4L2_CID_VBLANK. Can you init format to NULL at the declaration part to make the compiler happy? https://gitlab.freedesktop.org/linux-media/users/patchwork/-/pipelines/1325516/test_report?job_name=build > > > > + cci_write(imx214->regmap, > > > IMX214_REG_FRM_LENGTH_LINES, > > > + format->height + ctrl->val, &ret); > > > + break; > > > default: > > > ret = -EINVAL; > > > } > > > @@ -714,8 +772,11 @@ static int imx214_ctrls_init(struct imx214 > > > *imx214) > > > .width = 1120, > > > .height = 1120, > > > }; > > > + const struct imx214_mode *mode = &imx214_modes[0]; > > > struct v4l2_fwnode_device_properties props; > > > struct v4l2_ctrl_handler *ctrl_hdlr; > > > + int exposure_max, exposure_def; > > > + int hblank; > > > int ret; > > > > > > ret = v4l2_fwnode_device_parse(imx214->dev, &props); > > > @@ -723,7 +784,7 @@ static int imx214_ctrls_init(struct imx214 > > > *imx214) > > > return ret; > > > > > > ctrl_hdlr = &imx214->ctrls; > > > - ret = v4l2_ctrl_handler_init(&imx214->ctrls, 6); > > > + ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8); > > > if (ret) > > > return ret; > > > > > > @@ -749,12 +810,27 @@ static int imx214_ctrls_init(struct imx214 > > > *imx214) > > > * > > > * Yours sincerely, Ricardo. > > > */ > > > + > > > + /* Initial vblank/hblank/exposure parameters based on > > > current mode */ > > > + imx214->vblank = v4l2_ctrl_new_std(ctrl_hdlr, > > > &imx214_ctrl_ops, > > > + V4L2_CID_VBLANK, > > > IMX214_VBLANK_MIN, > > > + IMX214_VTS_MAX - mode- > > > >height, 2, > > > + mode->vts_def - mode- > > > >height); > > > + > > > + hblank = IMX214_PPL_DEFAULT - mode->width; > > > + imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr, > > > &imx214_ctrl_ops, > > > + V4L2_CID_HBLANK, hblank, > > > hblank, > > > + 1, hblank); > > > + if (imx214->hblank) > > > + imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > + > > > + exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET; > > > + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT); > > > imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, > > > &imx214_ctrl_ops, > > > V4L2_CID_EXPOSURE, > > > - IMX214_EXPOSURE_MIN, > > > - IMX214_EXPOSURE_MAX, > > > + IMX214_EXPOSURE_MIN, > > > exposure_max, > > nit: I think it looks nicer with exposure_max in the next line, but > > ignore if you prefer this way :) > > > IMX214_EXPOSURE_STEP, > > > - > > > IMX214_EXPOSURE_DEFAULT); > > > + exposure_def); > > > > > > imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr, > > > NULL, > > > @@ -876,6 +952,12 @@ static int imx214_get_frame_interval(struct > > > v4l2_subdev *subdev, > > > return 0; > > > } > > > > > > +/* > > > + * Raw sensors should be using the VBLANK and HBLANK controls to > > > determine > > > + * the frame rate. However this driver was initially added using > > > the > > > + * [S|G|ENUM]_FRAME_INTERVAL ioctls with a fixed rate of 30fps. > > > + * Retain the frame_interval ops for backwards compatibility, but > > > they do nothing. > > > + */ > > > > Now that these controls are useless... maybe we can do a > > dev_warn_once > > when the user calls it to leave some output in dmesg? > > Sure. We the following be a proper warning? > nit: What about? "frame_interval functions returns an unreliable value for compatibility reasons. Use the VBLANK and HBLANK controls to determine the correct frame rate.\n" > > > > static int imx214_enum_frame_interval(struct v4l2_subdev *subdev, > > > struct v4l2_subdev_state *sd_state, > > > struct > > > v4l2_subdev_frame_interval_enum *fie) > > > > > > -- > > > 2.47.1 > > > > > > >
On Sun, Dec 08, 2024 at 10:19:51PM +0100, Ricardo Ribalda Delgado wrote: > > > + state = v4l2_subdev_get_locked_active_state(&imx214->sd); > > > > Sakari, I see that other drivers assume that the active is locked in > > set_ctrl. Is this correct? > > imx214->sd.state_lock = imx214->ctrls.lock; > > So I guess it is fine :) Yes, it is. Please also run this on the set: $ ./scripts/checkpatch.pl --strict --max-line-length=80
Hi, Am Dienstag, dem 10.12.2024 um 12:34 +0000 schrieb Sakari Ailus: > On Sun, Dec 08, 2024 at 10:19:51PM +0100, Ricardo Ribalda Delgado > wrote: > > > > + state = > > > > v4l2_subdev_get_locked_active_state(&imx214->sd); > > > > > > Sakari, I see that other drivers assume that the active is locked > > > in > > > set_ctrl. Is this correct? > > > > imx214->sd.state_lock = imx214->ctrls.lock; > > > > So I guess it is fine :) > > Yes, it is. > > Please also run this on the set: > > $ ./scripts/checkpatch.pl --strict --max-line-length=80 > there are a few lines that exceed 80 columns: WARNING: line length of 85 exceeds 80 columns #163: FILE: drivers/media/i2c/imx214.c:576: + imx214_update_pad_format(imx214, mode, &format->format, format->format.code); -- WARNING: line length of 81 exceeds 80 columns #88: FILE: drivers/media/i2c/imx214.c:983: + return dev_err_probe(dev, ret, "failed to set xclk frequency\n"); -- WARNING: line length of 82 exceeds 80 columns #138: FILE: drivers/media/i2c/imx214.c:1039: + dev_err_probe(dev, ret, "failed to register sensor sub-device\n"); -- WARNING: line length of 86 exceeds 80 columns #491: FILE: drivers/media/i2c/imx214.c:359: + { IMX214_REG_EXCK_FREQ, IMX214_EXCK_FREQ(IMX214_DEFAULT_CLK_FREQ / 1000000) }, -- WARNING: line length of 83 exceeds 80 columns #177: FILE: drivers/media/i2c/imx214.c:730: + exposure_max = format->height + ctrl->val - IMX214_EXPOSURE_OFFSET; -- WARNING: line length of 85 exceeds 80 columns #85: FILE: drivers/media/i2c/imx214.c:1231: + if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ_LEGACY) { Is the strict 80 columns limit really necessary, as it would decrease readability? Best regards, André
Hi André, On Sat, Dec 14, 2024 at 11:45:16PM +0100, André Apitzsch wrote: > Hi, > > Am Dienstag, dem 10.12.2024 um 12:34 +0000 schrieb Sakari Ailus: > > On Sun, Dec 08, 2024 at 10:19:51PM +0100, Ricardo Ribalda Delgado > > wrote: > > > > > + state = > > > > > v4l2_subdev_get_locked_active_state(&imx214->sd); > > > > > > > > Sakari, I see that other drivers assume that the active is locked > > > > in > > > > set_ctrl. Is this correct? > > > > > > imx214->sd.state_lock = imx214->ctrls.lock; > > > > > > So I guess it is fine :) > > > > Yes, it is. > > > > Please also run this on the set: > > > > $ ./scripts/checkpatch.pl --strict --max-line-length=80 > > > > there are a few lines that exceed 80 columns: > > WARNING: line length of 85 exceeds 80 columns > #163: FILE: drivers/media/i2c/imx214.c:576: > + imx214_update_pad_format(imx214, mode, &format->format, format->format.code); > -- > WARNING: line length of 81 exceeds 80 columns > #88: FILE: drivers/media/i2c/imx214.c:983: > + return dev_err_probe(dev, ret, "failed to set xclk frequency\n"); > -- > WARNING: line length of 82 exceeds 80 columns > #138: FILE: drivers/media/i2c/imx214.c:1039: > + dev_err_probe(dev, ret, "failed to register sensor sub-device\n"); > -- > WARNING: line length of 86 exceeds 80 columns > #491: FILE: drivers/media/i2c/imx214.c:359: > + { IMX214_REG_EXCK_FREQ, IMX214_EXCK_FREQ(IMX214_DEFAULT_CLK_FREQ / 1000000) }, > -- > WARNING: line length of 83 exceeds 80 columns > #177: FILE: drivers/media/i2c/imx214.c:730: > + exposure_max = format->height + ctrl->val - IMX214_EXPOSURE_OFFSET; > -- > WARNING: line length of 85 exceeds 80 columns > #85: FILE: drivers/media/i2c/imx214.c:1231: > + if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ_LEGACY) { > > > Is the strict 80 columns limit really necessary, as it would decrease > readability? For the array that could be the case but I think I'd wrap the rest.
Hi, Am Sonntag, dem 08.12.2024 um 21:59 +0100 schrieb Ricardo Ribalda Delgado: > In general it looks good to me (besides the comments, ignore the nits > if you want to). > > I'd recommend that you test with lockdep to make sure that we are not > missing anything, and I'd like to hear back from Sakari regarding the > get_locked_active > > Thanks! > > On Sat, Dec 7, 2024 at 9:48 PM André Apitzsch via B4 Relay > <devnull+git.apitzsch.eu@kernel.org> wrote: > > > > From: André Apitzsch <git@apitzsch.eu> > > > > Add vblank control to allow changing the framerate / > > higher exposure values. > > > > The vblank and hblank controls are needed for libcamera support. > > > > While at it, fix the minimal exposure time according to the > > datasheet. > > > > Signed-off-by: André Apitzsch <git@apitzsch.eu> > > --- > > drivers/media/i2c/imx214.c | 106 > > ++++++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 94 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/media/i2c/imx214.c > > b/drivers/media/i2c/imx214.c > > index > > f1c72db0775eaf4810f762e8798d301c5ad9923c..a7f49dbafe0f54af3c02f5534 > > 460fdee88a22fe2 100644 > > --- a/drivers/media/i2c/imx214.c > > +++ b/drivers/media/i2c/imx214.c > > @@ -34,11 +34,17 @@ > > > > /* V-TIMING internal */ > > #define IMX214_REG_FRM_LENGTH_LINES CCI_REG16(0x0340) > > +#define IMX214_VTS_MAX 0xffff > > + > > +#define IMX214_VBLANK_MIN 890 > > + > > +/* HBLANK control - read only */ > > +#define IMX214_PPL_DEFAULT 5008 > > > > /* Exposure control */ > > #define IMX214_REG_EXPOSURE CCI_REG16(0x0202) > > -#define IMX214_EXPOSURE_MIN 0 > > -#define IMX214_EXPOSURE_MAX 3184 > > +#define IMX214_EXPOSURE_OFFSET 10 > > +#define IMX214_EXPOSURE_MIN 1 > > #define IMX214_EXPOSURE_STEP 1 > > #define IMX214_EXPOSURE_DEFAULT 3184 > > #define IMX214_REG_EXPOSURE_RATIO CCI_REG8(0x0222) > > @@ -187,6 +193,8 @@ struct imx214 { > > struct v4l2_ctrl_handler ctrls; > > struct v4l2_ctrl *pixel_rate; > > struct v4l2_ctrl *link_freq; > > + struct v4l2_ctrl *vblank; > > + struct v4l2_ctrl *hblank; > > struct v4l2_ctrl *exposure; > > struct v4l2_ctrl *unit_size; > > > > @@ -202,8 +210,6 @@ static const struct cci_reg_sequence > > mode_4096x2304[] = { > > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF }, > > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH > > }, > > { IMX214_REG_EXPOSURE_RATIO, 1 }, > > - { IMX214_REG_FRM_LENGTH_LINES, 3194 }, > > - { IMX214_REG_LINE_LENGTH_PCK, 5008 }, > > { IMX214_REG_X_ADD_STA, 56 }, > > { IMX214_REG_Y_ADD_STA, 408 }, > > { IMX214_REG_X_ADD_END, 4151 }, > > @@ -274,8 +280,6 @@ static const struct cci_reg_sequence > > mode_1920x1080[] = { > > { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF }, > > { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH > > }, > > { IMX214_REG_EXPOSURE_RATIO, 1 }, > > - { IMX214_REG_FRM_LENGTH_LINES, 3194 }, > > - { IMX214_REG_LINE_LENGTH_PCK, 5008 }, > > { IMX214_REG_X_ADD_STA, 1144 }, > > { IMX214_REG_Y_ADD_STA, 1020 }, > > { IMX214_REG_X_ADD_END, 3063 }, > > @@ -359,6 +363,7 @@ static const struct cci_reg_sequence > > mode_table_common[] = { > > { IMX214_REG_ORIENTATION, 0 }, > > { IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK }, > > { IMX214_REG_FAST_STANDBY_CTRL, 1 }, > > + { IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT }, > > { CCI_REG8(0x4550), 0x02 }, > > { CCI_REG8(0x4601), 0x00 }, > > { CCI_REG8(0x4642), 0x05 }, > > @@ -462,18 +467,24 @@ static const struct cci_reg_sequence > > mode_table_common[] = { > > static const struct imx214_mode { > > u32 width; > > u32 height; > > + > > + /* V-timing */ > > + unsigned int vts_def; > > + > > unsigned int num_of_regs; > > const struct cci_reg_sequence *reg_table; > > } imx214_modes[] = { > > { > > .width = 4096, > > .height = 2304, > > + .vts_def = 3194, > > .num_of_regs = ARRAY_SIZE(mode_4096x2304), > > .reg_table = mode_4096x2304, > > }, > > { > > .width = 1920, > > .height = 1080, > > + .vts_def = 3194, > > .num_of_regs = ARRAY_SIZE(mode_1920x1080), > > .reg_table = mode_1920x1080, > > }, > > @@ -626,9 +637,36 @@ static int imx214_set_format(struct > > v4l2_subdev *sd, > > __crop->width = mode->width; > > __crop->height = mode->height; > > > > - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) > > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > > + int exposure_max; > > + int exposure_def; > > + int hblank; > > + > > imx214->cur_mode = mode; > > > > + /* Update FPS limits */ > nit: Update blank limits > > + __v4l2_ctrl_modify_range(imx214->vblank, > > IMX214_VBLANK_MIN, > > + IMX214_VTS_MAX - mode- > > >height, 2, > > + mode->vts_def - mode- > > >height); > > Is the handler->lock held when we call this function? I'm not sure how to test this. > Can you try > running the code with lockdep? /proc/lockdep contains > 00000000f9299231 FD: 89 BD: 6 +.+.: imx214:901:(&imx214->ctrls)->_lock (no idea how to interpret this line) and dmesg doesn't print anything, when changing the vblank value. Best regards, André > > + > > + /* Update max exposure while meeting expected > > vblanking */ > > + exposure_max = mode->vts_def - > > IMX214_EXPOSURE_OFFSET; > > + exposure_def = min(exposure_max, > > IMX214_EXPOSURE_DEFAULT); > > + __v4l2_ctrl_modify_range(imx214->exposure, > > + imx214->exposure->minimum, > > + exposure_max, imx214- > > >exposure->step, > > + exposure_def); > > + > > + /* > > + * Currently PPL is fixed to IMX214_PPL_DEFAULT, so > > hblank > > + * depends on mode->width only, and is not > > changeable in any > > + * way other than changing the mode. > > + */ > > + hblank = IMX214_PPL_DEFAULT - mode->width; > > + __v4l2_ctrl_modify_range(imx214->hblank, hblank, > > hblank, 1, > > + hblank); > > + } > > + > > return 0; > > } > > > > @@ -678,8 +716,25 @@ static int imx214_set_ctrl(struct v4l2_ctrl > > *ctrl) > > { > > struct imx214 *imx214 = container_of(ctrl->handler, > > struct imx214, ctrls); > > + const struct v4l2_mbus_framefmt *format; > > + struct v4l2_subdev_state *state; > > int ret; > > > > + if (ctrl->id == V4L2_CID_VBLANK) { > > + int exposure_max, exposure_def; > > + > > + state = > > v4l2_subdev_get_locked_active_state(&imx214->sd); > > Sakari, I see that other drivers assume that the active is locked in > set_ctrl. Is this correct? > > > + format = v4l2_subdev_state_get_format(state, 0); > > + > > + /* Update max exposure while meeting expected > > vblanking */ > > + exposure_max = format->height + ctrl->val - > > IMX214_EXPOSURE_OFFSET; > > + exposure_def = min(exposure_max, > > IMX214_EXPOSURE_DEFAULT); > > + __v4l2_ctrl_modify_range(imx214->exposure, > > + imx214->exposure->minimum, > > + exposure_max, imx214- > > >exposure->step, > > + exposure_def); > > + } > > + > > /* > > * Applying V4L2 control value only happens > > * when power is up for streaming > > @@ -691,7 +746,10 @@ static int imx214_set_ctrl(struct v4l2_ctrl > > *ctrl) > > case V4L2_CID_EXPOSURE: > > cci_write(imx214->regmap, IMX214_REG_EXPOSURE, > > ctrl->val, &ret); > > break; > > - > > + case V4L2_CID_VBLANK: > No need to read the format here? > format = v4l2_subdev_state_get_format(state, > 0); > > + cci_write(imx214->regmap, > > IMX214_REG_FRM_LENGTH_LINES, > > + format->height + ctrl->val, &ret); > > + break; > > default: > > ret = -EINVAL; > > } > > @@ -714,8 +772,11 @@ static int imx214_ctrls_init(struct imx214 > > *imx214) > > .width = 1120, > > .height = 1120, > > }; > > + const struct imx214_mode *mode = &imx214_modes[0]; > > struct v4l2_fwnode_device_properties props; > > struct v4l2_ctrl_handler *ctrl_hdlr; > > + int exposure_max, exposure_def; > > + int hblank; > > int ret; > > > > ret = v4l2_fwnode_device_parse(imx214->dev, &props); > > @@ -723,7 +784,7 @@ static int imx214_ctrls_init(struct imx214 > > *imx214) > > return ret; > > > > ctrl_hdlr = &imx214->ctrls; > > - ret = v4l2_ctrl_handler_init(&imx214->ctrls, 6); > > + ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8); > > if (ret) > > return ret; > > > > @@ -749,12 +810,27 @@ static int imx214_ctrls_init(struct imx214 > > *imx214) > > * > > * Yours sincerely, Ricardo. > > */ > > + > > + /* Initial vblank/hblank/exposure parameters based on > > current mode */ > > + imx214->vblank = v4l2_ctrl_new_std(ctrl_hdlr, > > &imx214_ctrl_ops, > > + V4L2_CID_VBLANK, > > IMX214_VBLANK_MIN, > > + IMX214_VTS_MAX - mode- > > >height, 2, > > + mode->vts_def - mode- > > >height); > > + > > + hblank = IMX214_PPL_DEFAULT - mode->width; > > + imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr, > > &imx214_ctrl_ops, > > + V4L2_CID_HBLANK, hblank, > > hblank, > > + 1, hblank); > > + if (imx214->hblank) > > + imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > + > > + exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET; > > + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT); > > imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, > > &imx214_ctrl_ops, > > V4L2_CID_EXPOSURE, > > - IMX214_EXPOSURE_MIN, > > - IMX214_EXPOSURE_MAX, > > + IMX214_EXPOSURE_MIN, > > exposure_max, > nit: I think it looks nicer with exposure_max in the next line, but > ignore if you prefer this way :) > > IMX214_EXPOSURE_STEP, > > - > > IMX214_EXPOSURE_DEFAULT); > > + exposure_def); > > > > imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr, > > NULL, > > @@ -876,6 +952,12 @@ static int imx214_get_frame_interval(struct > > v4l2_subdev *subdev, > > return 0; > > } > > > > +/* > > + * Raw sensors should be using the VBLANK and HBLANK controls to > > determine > > + * the frame rate. However this driver was initially added using > > the > > + * [S|G|ENUM]_FRAME_INTERVAL ioctls with a fixed rate of 30fps. > > + * Retain the frame_interval ops for backwards compatibility, but > > they do nothing. > > + */ > > Now that these controls are useless... maybe we can do a > dev_warn_once > when the user calls it to leave some output in dmesg? > > static int imx214_enum_frame_interval(struct v4l2_subdev *subdev, > > struct v4l2_subdev_state *sd_state, > > struct > > v4l2_subdev_frame_interval_enum *fie) > > > > -- > > 2.47.1 > > > >
> > Is the handler->lock held when we call this function? > > I'm not sure how to test this. > > > Can you try > > running the code with lockdep? > > /proc/lockdep contains > > > 00000000f9299231 FD: 89 BD: 6 +.+.: imx214:901:(&imx214->ctrls)->_lock > > (no idea how to interpret this line) > > and dmesg doesn't print anything, when changing the vblank value. > If dmesg does not complain after using it I am happy. Thanks! :)
Hi André, On Sun, Dec 15, 2024 at 11:35:41PM +0100, André Apitzsch wrote: > > > @@ -626,9 +637,36 @@ static int imx214_set_format(struct > > > v4l2_subdev *sd, > > > __crop->width = mode->width; > > > __crop->height = mode->height; > > > > > > - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) > > > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > > > + int exposure_max; > > > + int exposure_def; > > > + int hblank; > > > + > > > imx214->cur_mode = mode; > > > > > > + /* Update FPS limits */ > > nit: Update blank limits > > > + __v4l2_ctrl_modify_range(imx214->vblank, > > > IMX214_VBLANK_MIN, > > > + IMX214_VTS_MAX - mode- > > > >height, 2, > > > + mode->vts_def - mode- > > > >height); > > > > Is the handler->lock held when we call this function? > > I'm not sure how to test this. I'd say "yes" because the first patch sets the control handler lock to the sub-device state lock.
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c index f1c72db0775eaf4810f762e8798d301c5ad9923c..a7f49dbafe0f54af3c02f5534460fdee88a22fe2 100644 --- a/drivers/media/i2c/imx214.c +++ b/drivers/media/i2c/imx214.c @@ -34,11 +34,17 @@ /* V-TIMING internal */ #define IMX214_REG_FRM_LENGTH_LINES CCI_REG16(0x0340) +#define IMX214_VTS_MAX 0xffff + +#define IMX214_VBLANK_MIN 890 + +/* HBLANK control - read only */ +#define IMX214_PPL_DEFAULT 5008 /* Exposure control */ #define IMX214_REG_EXPOSURE CCI_REG16(0x0202) -#define IMX214_EXPOSURE_MIN 0 -#define IMX214_EXPOSURE_MAX 3184 +#define IMX214_EXPOSURE_OFFSET 10 +#define IMX214_EXPOSURE_MIN 1 #define IMX214_EXPOSURE_STEP 1 #define IMX214_EXPOSURE_DEFAULT 3184 #define IMX214_REG_EXPOSURE_RATIO CCI_REG8(0x0222) @@ -187,6 +193,8 @@ struct imx214 { struct v4l2_ctrl_handler ctrls; struct v4l2_ctrl *pixel_rate; struct v4l2_ctrl *link_freq; + struct v4l2_ctrl *vblank; + struct v4l2_ctrl *hblank; struct v4l2_ctrl *exposure; struct v4l2_ctrl *unit_size; @@ -202,8 +210,6 @@ static const struct cci_reg_sequence mode_4096x2304[] = { { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF }, { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH }, { IMX214_REG_EXPOSURE_RATIO, 1 }, - { IMX214_REG_FRM_LENGTH_LINES, 3194 }, - { IMX214_REG_LINE_LENGTH_PCK, 5008 }, { IMX214_REG_X_ADD_STA, 56 }, { IMX214_REG_Y_ADD_STA, 408 }, { IMX214_REG_X_ADD_END, 4151 }, @@ -274,8 +280,6 @@ static const struct cci_reg_sequence mode_1920x1080[] = { { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF }, { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH }, { IMX214_REG_EXPOSURE_RATIO, 1 }, - { IMX214_REG_FRM_LENGTH_LINES, 3194 }, - { IMX214_REG_LINE_LENGTH_PCK, 5008 }, { IMX214_REG_X_ADD_STA, 1144 }, { IMX214_REG_Y_ADD_STA, 1020 }, { IMX214_REG_X_ADD_END, 3063 }, @@ -359,6 +363,7 @@ static const struct cci_reg_sequence mode_table_common[] = { { IMX214_REG_ORIENTATION, 0 }, { IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK }, { IMX214_REG_FAST_STANDBY_CTRL, 1 }, + { IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT }, { CCI_REG8(0x4550), 0x02 }, { CCI_REG8(0x4601), 0x00 }, { CCI_REG8(0x4642), 0x05 }, @@ -462,18 +467,24 @@ static const struct cci_reg_sequence mode_table_common[] = { static const struct imx214_mode { u32 width; u32 height; + + /* V-timing */ + unsigned int vts_def; + unsigned int num_of_regs; const struct cci_reg_sequence *reg_table; } imx214_modes[] = { { .width = 4096, .height = 2304, + .vts_def = 3194, .num_of_regs = ARRAY_SIZE(mode_4096x2304), .reg_table = mode_4096x2304, }, { .width = 1920, .height = 1080, + .vts_def = 3194, .num_of_regs = ARRAY_SIZE(mode_1920x1080), .reg_table = mode_1920x1080, }, @@ -626,9 +637,36 @@ static int imx214_set_format(struct v4l2_subdev *sd, __crop->width = mode->width; __crop->height = mode->height; - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) { + int exposure_max; + int exposure_def; + int hblank; + imx214->cur_mode = mode; + /* Update FPS limits */ + __v4l2_ctrl_modify_range(imx214->vblank, IMX214_VBLANK_MIN, + IMX214_VTS_MAX - mode->height, 2, + mode->vts_def - mode->height); + + /* Update max exposure while meeting expected vblanking */ + exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET; + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT); + __v4l2_ctrl_modify_range(imx214->exposure, + imx214->exposure->minimum, + exposure_max, imx214->exposure->step, + exposure_def); + + /* + * Currently PPL is fixed to IMX214_PPL_DEFAULT, so hblank + * depends on mode->width only, and is not changeable in any + * way other than changing the mode. + */ + hblank = IMX214_PPL_DEFAULT - mode->width; + __v4l2_ctrl_modify_range(imx214->hblank, hblank, hblank, 1, + hblank); + } + return 0; } @@ -678,8 +716,25 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl) { struct imx214 *imx214 = container_of(ctrl->handler, struct imx214, ctrls); + const struct v4l2_mbus_framefmt *format; + struct v4l2_subdev_state *state; int ret; + if (ctrl->id == V4L2_CID_VBLANK) { + int exposure_max, exposure_def; + + state = v4l2_subdev_get_locked_active_state(&imx214->sd); + format = v4l2_subdev_state_get_format(state, 0); + + /* Update max exposure while meeting expected vblanking */ + exposure_max = format->height + ctrl->val - IMX214_EXPOSURE_OFFSET; + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT); + __v4l2_ctrl_modify_range(imx214->exposure, + imx214->exposure->minimum, + exposure_max, imx214->exposure->step, + exposure_def); + } + /* * Applying V4L2 control value only happens * when power is up for streaming @@ -691,7 +746,10 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl) case V4L2_CID_EXPOSURE: cci_write(imx214->regmap, IMX214_REG_EXPOSURE, ctrl->val, &ret); break; - + case V4L2_CID_VBLANK: + cci_write(imx214->regmap, IMX214_REG_FRM_LENGTH_LINES, + format->height + ctrl->val, &ret); + break; default: ret = -EINVAL; } @@ -714,8 +772,11 @@ static int imx214_ctrls_init(struct imx214 *imx214) .width = 1120, .height = 1120, }; + const struct imx214_mode *mode = &imx214_modes[0]; struct v4l2_fwnode_device_properties props; struct v4l2_ctrl_handler *ctrl_hdlr; + int exposure_max, exposure_def; + int hblank; int ret; ret = v4l2_fwnode_device_parse(imx214->dev, &props); @@ -723,7 +784,7 @@ static int imx214_ctrls_init(struct imx214 *imx214) return ret; ctrl_hdlr = &imx214->ctrls; - ret = v4l2_ctrl_handler_init(&imx214->ctrls, 6); + ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8); if (ret) return ret; @@ -749,12 +810,27 @@ static int imx214_ctrls_init(struct imx214 *imx214) * * Yours sincerely, Ricardo. */ + + /* Initial vblank/hblank/exposure parameters based on current mode */ + imx214->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops, + V4L2_CID_VBLANK, IMX214_VBLANK_MIN, + IMX214_VTS_MAX - mode->height, 2, + mode->vts_def - mode->height); + + hblank = IMX214_PPL_DEFAULT - mode->width; + imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops, + V4L2_CID_HBLANK, hblank, hblank, + 1, hblank); + if (imx214->hblank) + imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET; + exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT); imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops, V4L2_CID_EXPOSURE, - IMX214_EXPOSURE_MIN, - IMX214_EXPOSURE_MAX, + IMX214_EXPOSURE_MIN, exposure_max, IMX214_EXPOSURE_STEP, - IMX214_EXPOSURE_DEFAULT); + exposure_def); imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr, NULL, @@ -876,6 +952,12 @@ static int imx214_get_frame_interval(struct v4l2_subdev *subdev, return 0; } +/* + * Raw sensors should be using the VBLANK and HBLANK controls to determine + * the frame rate. However this driver was initially added using the + * [S|G|ENUM]_FRAME_INTERVAL ioctls with a fixed rate of 30fps. + * Retain the frame_interval ops for backwards compatibility, but they do nothing. + */ static int imx214_enum_frame_interval(struct v4l2_subdev *subdev, struct v4l2_subdev_state *sd_state, struct v4l2_subdev_frame_interval_enum *fie)