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@xxxxxxxxxx> wrote:
>
> From: André Apitzsch <git@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> ---
> 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
>
>
