Hi André & Ricardo On Thu, 12 Sept 2024 at 14:41, Ricardo Ribalda Delgado <ribalda@xxxxxxxxxx> wrote: > > Hi > > Arent you missing some chage in enum_frame_interval? Raw sensors shouldn't be using [enum|set|get]_frame_interval at all https://www.kernel.org/doc/html/latest/userspace-api/media/drivers/camera-sensor.html#frame-interval-configuration The question now is how to handle the backwards compatibility for any userspace app that might be using this driver and expecting to use the frame_interval calls. Seeing as it only ever allowed a fixed value of 30fps, leaving it as is with a comment as to why it is there would be reasonable in my view. > On Mon, Sep 2, 2024 at 11:53 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 | 112 ++++++++++++++++++++++++++++++++++++--------- > > 1 file changed, 91 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c > > index 3b422cddbdce..9f5a57aebb86 100644 > > --- a/drivers/media/i2c/imx214.c > > +++ b/drivers/media/i2c/imx214.c > > @@ -34,11 +34,18 @@ > > > > /* V-TIMING internal */ > > #define IMX214_REG_FRM_LENGTH_LINES CCI_REG16(0x0340) > > +#define IMX214_VTS_MAX 0xffff > > + > > +#define IMX214_VBLANK_MIN 4 > > + > > +/* 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_MAX (IMX214_VTS_MAX - IMX214_EXPOSURE_OFFSET) > > #define IMX214_EXPOSURE_STEP 1 > > #define IMX214_EXPOSURE_DEFAULT 3184 > > #define IMX214_REG_EXPOSURE_RATIO CCI_REG8(0x0222) > > @@ -189,6 +196,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; > > > > @@ -205,8 +214,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 }, > > @@ -277,8 +284,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 }, > > @@ -362,6 +367,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 }, > > @@ -465,18 +471,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, > > }, > > @@ -629,6 +641,37 @@ static int imx214_set_format(struct v4l2_subdev *sd, > > __crop->width = mode->width; > > __crop->height = mode->height; > > > > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > > + int exposure_max; > > + int exposure_def; > > + int hblank; > > + > > + /* Update limits and set FPS to default */ > > + __v4l2_ctrl_modify_range(imx214->vblank, IMX214_VBLANK_MIN, > > + IMX214_VTS_MAX - mode->height, 1, > > + mode->vts_def - mode->height); > > + __v4l2_ctrl_s_ctrl(imx214->vblank, > > + mode->vts_def - mode->height); > > + > > + /* Update max exposure while meeting expected vblanking */ > > + exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET; > > + exposure_def = (exposure_max < IMX214_EXPOSURE_DEFAULT) ? > > + 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 changeble 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 +721,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; > > > > + state = v4l2_subdev_get_locked_active_state(&imx214->sd); > > + format = v4l2_subdev_state_get_format(state, 0); > > + > > + if (ctrl->id == V4L2_CID_VBLANK) { > > + int exposure_max, exposure_def; > > + > > + /* 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 +751,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); My datasheet says this register is "set up in multiples of 2". (LINE_LENGTH_PCK for HBLANK is "set in multiples of 8") I don't have one of these modules, but is that saying only multiples of 2 are permitted (in which case the step size for the control needs to reflect that), or that setting a value of N is interpreted by the hardware as 2N? Do all the numbers with PIXEL_RATE work out correctly in the frame rate calcs? Reading the spec sheet that 30fps is the max frame rate at full res (4096x2304), and the driver was setting a value of 3194 to this register, I don't see it being interpreted as 2N. Then again having VBLANK at 890 seems pretty high. > > + break; > > default: > > ret = -EINVAL; > > } > > @@ -714,8 +777,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 +789,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; > > > > @@ -739,22 +805,26 @@ static int imx214_ctrls_init(struct imx214 *imx214) > > if (imx214->link_freq) > > imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > > - /* > > - * WARNING! > > - * Values obtained reverse engineering blobs and/or devices. > > - * Ranges and functionality might be wrong. > > - * > > - * Sony, please release some register set documentation for the > > - * device. > > - * > > - * Yours sincerely, Ricardo. > > - */ > > I would like to keep this comment until there is a public document available. I suspect you'll be waiting forever for a document to be officially released. I have a datasheet for IMX214, and I believe Kieran and Jacopo do too. Which specific values do you wish to be verified? > > > + /* 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_VBLANK_MIN being 4 feels plausible, but pretty low. I read the datasheet to say there are 4 embedded data lines per image. Seeing as you have STATS data output enabled as well that makes 5 lines of non-image data per frame, but you're only adding blanking time for 4 lines. As noted above, I think you've also increased the max frame rate significantly above that quoted by Sony. Has that been actually exercised and confirmed to function correctly? Dave > > + IMX214_VTS_MAX - mode->height, 1, > > + 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, > > > > -- > > 2.46.0 > > > > >
