| Message ID | 20230530173000.3060865-15-dave.stevenson@raspberrypi.com |
|---|---|
| State | New |
| Headers | show |
| Series | [01/21] media: i2c: imx258: Remove unused defines | expand |
Hi Dave On Tue, May 30, 2023 at 06:29:53PM +0100, Dave Stevenson wrote: > The sensor has a register CIT_LSHIFT which extends the exposure > and frame times by the specified power of 2 for longer > exposure times. > > Add support for this by configuring this register via V4L2_CID_VBLANK > and extending the V4L2_CID_EXPOSURE range accordingly. > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > drivers/media/i2c/imx258.c | 38 ++++++++++++++++++++++++++++++++------ > 1 file changed, 32 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c > index f5199e3243e8..1e424058fcb9 100644 > --- a/drivers/media/i2c/imx258.c > +++ b/drivers/media/i2c/imx258.c > @@ -69,6 +69,10 @@ > #define IMX258_HDR_RATIO_STEP 1 > #define IMX258_HDR_RATIO_DEFAULT 0x0 > > +/* Long exposure multiplier */ > +#define IMX258_LONG_EXP_SHIFT_MAX 7 > +#define IMX258_LONG_EXP_SHIFT_REG 0x3002 > + > /* Test Pattern Control */ > #define IMX258_REG_TEST_PATTERN 0x0600 > > @@ -629,6 +633,8 @@ struct imx258 { > struct v4l2_ctrl *vblank; > struct v4l2_ctrl *hblank; > struct v4l2_ctrl *exposure; > + /* Current long exposure factor in use. Set through V4L2_CID_VBLANK */ > + unsigned int long_exp_shift; > > /* Current mode */ > const struct imx258_mode *cur_mode; > @@ -793,6 +799,26 @@ static void imx258_adjust_exposure_range(struct imx258 *imx258) > exposure_def); > } > > +static int imx258_set_frame_length(struct imx258 *imx258, unsigned int val) > +{ > + int ret; > + > + imx258->long_exp_shift = 0; > + > + while (val > IMX258_VTS_MAX) { > + imx258->long_exp_shift++; > + val >>= 1; > + } > + > + ret = imx258_write_reg(imx258, IMX258_REG_VTS, > + IMX258_REG_VALUE_16BIT, val); > + if (ret) > + return ret; > + > + return imx258_write_reg(imx258, IMX258_LONG_EXP_SHIFT_REG, > + IMX258_REG_VALUE_08BIT, imx258->long_exp_shift); > +} > + > static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) > { > struct imx258 *imx258 = > @@ -823,7 +849,7 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) > case V4L2_CID_EXPOSURE: > ret = imx258_write_reg(imx258, IMX258_REG_EXPOSURE, > IMX258_REG_VALUE_16BIT, > - ctrl->val); > + ctrl->val >> imx258->long_exp_shift); Shouldn't this be done only if vblank > VTS_MAX ? > break; > case V4L2_CID_DIGITAL_GAIN: > ret = imx258_update_digital_gain(imx258, IMX258_REG_VALUE_16BIT, > @@ -855,9 +881,8 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) > } > break; > case V4L2_CID_VBLANK: > - ret = imx258_write_reg(imx258, IMX258_REG_VTS, > - IMX258_REG_VALUE_16BIT, > - imx258->cur_mode->height + ctrl->val); > + ret = imx258_set_frame_length(imx258, > + imx258->cur_mode->height + ctrl->val); > break; > default: > dev_info(&client->dev, > @@ -983,8 +1008,9 @@ static int imx258_set_pad_format(struct v4l2_subdev *sd, > imx258->cur_mode->height; > __v4l2_ctrl_modify_range( > imx258->vblank, vblank_min, > - IMX258_VTS_MAX - imx258->cur_mode->height, 1, > - vblank_def); > + ((1 << IMX258_LONG_EXP_SHIFT_MAX) * IMX258_VTS_MAX) - > + imx258->cur_mode->height, > + 1, vblank_def); > __v4l2_ctrl_s_ctrl(imx258->vblank, vblank_def); > h_blank = > imx258->link_freq_configs[mode->link_freq_index].pixels_per_line > -- > 2.25.1 >
Hi Jacopo On Fri, 2 Jun 2023 at 14:38, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Dave > > On Tue, May 30, 2023 at 06:29:53PM +0100, Dave Stevenson wrote: > > The sensor has a register CIT_LSHIFT which extends the exposure > > and frame times by the specified power of 2 for longer > > exposure times. > > > > Add support for this by configuring this register via V4L2_CID_VBLANK > > and extending the V4L2_CID_EXPOSURE range accordingly. > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > --- > > drivers/media/i2c/imx258.c | 38 ++++++++++++++++++++++++++++++++------ > > 1 file changed, 32 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c > > index f5199e3243e8..1e424058fcb9 100644 > > --- a/drivers/media/i2c/imx258.c > > +++ b/drivers/media/i2c/imx258.c > > @@ -69,6 +69,10 @@ > > #define IMX258_HDR_RATIO_STEP 1 > > #define IMX258_HDR_RATIO_DEFAULT 0x0 > > > > +/* Long exposure multiplier */ > > +#define IMX258_LONG_EXP_SHIFT_MAX 7 > > +#define IMX258_LONG_EXP_SHIFT_REG 0x3002 > > + > > /* Test Pattern Control */ > > #define IMX258_REG_TEST_PATTERN 0x0600 > > > > @@ -629,6 +633,8 @@ struct imx258 { > > struct v4l2_ctrl *vblank; > > struct v4l2_ctrl *hblank; > > struct v4l2_ctrl *exposure; > > + /* Current long exposure factor in use. Set through V4L2_CID_VBLANK */ > > + unsigned int long_exp_shift; > > > > /* Current mode */ > > const struct imx258_mode *cur_mode; > > @@ -793,6 +799,26 @@ static void imx258_adjust_exposure_range(struct imx258 *imx258) > > exposure_def); > > } > > > > +static int imx258_set_frame_length(struct imx258 *imx258, unsigned int val) > > +{ > > + int ret; > > + > > + imx258->long_exp_shift = 0; > > + > > + while (val > IMX258_VTS_MAX) { > > + imx258->long_exp_shift++; > > + val >>= 1; > > + } > > + > > + ret = imx258_write_reg(imx258, IMX258_REG_VTS, > > + IMX258_REG_VALUE_16BIT, val); > > + if (ret) > > + return ret; > > + > > + return imx258_write_reg(imx258, IMX258_LONG_EXP_SHIFT_REG, > > + IMX258_REG_VALUE_08BIT, imx258->long_exp_shift); > > +} > > + > > static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) > > { > > struct imx258 *imx258 = > > @@ -823,7 +849,7 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) > > case V4L2_CID_EXPOSURE: > > ret = imx258_write_reg(imx258, IMX258_REG_EXPOSURE, > > IMX258_REG_VALUE_16BIT, > > - ctrl->val); > > + ctrl->val >> imx258->long_exp_shift); > > Shouldn't this be done only if vblank > VTS_MAX ? You're partly right, and it's a bug in our imx477 and imx708 drivers too. (cc Naush for info) Computing imx258->long_exp_shift should be done in the early part of imx258_set_ctrl before the pm_runtime_get_if_in_use check. Otherwise the __v4l2_ctrl_handler_setup call will potentially be setting exposure before vblank, and exposure register will be based on the wrong shift. If (vblank + mode->height) <= VTS_MAX then long_exp_shift will be 0, so the value written here will be the same. The slightly more awkward one to handle is that if long_exp_shift changes then we need to recompute and write IMX258_REG_EXPOSURE based on the new shift. You have to love the niggles. Thanks Dave > > break; > > case V4L2_CID_DIGITAL_GAIN: > > ret = imx258_update_digital_gain(imx258, IMX258_REG_VALUE_16BIT, > > @@ -855,9 +881,8 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) > > } > > break; > > case V4L2_CID_VBLANK: > > - ret = imx258_write_reg(imx258, IMX258_REG_VTS, > > - IMX258_REG_VALUE_16BIT, > > - imx258->cur_mode->height + ctrl->val); > > + ret = imx258_set_frame_length(imx258, > > + imx258->cur_mode->height + ctrl->val); > > break; > > default: > > dev_info(&client->dev, > > @@ -983,8 +1008,9 @@ static int imx258_set_pad_format(struct v4l2_subdev *sd, > > imx258->cur_mode->height; > > __v4l2_ctrl_modify_range( > > imx258->vblank, vblank_min, > > - IMX258_VTS_MAX - imx258->cur_mode->height, 1, > > - vblank_def); > > + ((1 << IMX258_LONG_EXP_SHIFT_MAX) * IMX258_VTS_MAX) - > > + imx258->cur_mode->height, > > + 1, vblank_def); > > __v4l2_ctrl_s_ctrl(imx258->vblank, vblank_def); > > h_blank = > > imx258->link_freq_configs[mode->link_freq_index].pixels_per_line > > -- > > 2.25.1 > >
diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c index f5199e3243e8..1e424058fcb9 100644 --- a/drivers/media/i2c/imx258.c +++ b/drivers/media/i2c/imx258.c @@ -69,6 +69,10 @@ #define IMX258_HDR_RATIO_STEP 1 #define IMX258_HDR_RATIO_DEFAULT 0x0 +/* Long exposure multiplier */ +#define IMX258_LONG_EXP_SHIFT_MAX 7 +#define IMX258_LONG_EXP_SHIFT_REG 0x3002 + /* Test Pattern Control */ #define IMX258_REG_TEST_PATTERN 0x0600 @@ -629,6 +633,8 @@ struct imx258 { struct v4l2_ctrl *vblank; struct v4l2_ctrl *hblank; struct v4l2_ctrl *exposure; + /* Current long exposure factor in use. Set through V4L2_CID_VBLANK */ + unsigned int long_exp_shift; /* Current mode */ const struct imx258_mode *cur_mode; @@ -793,6 +799,26 @@ static void imx258_adjust_exposure_range(struct imx258 *imx258) exposure_def); } +static int imx258_set_frame_length(struct imx258 *imx258, unsigned int val) +{ + int ret; + + imx258->long_exp_shift = 0; + + while (val > IMX258_VTS_MAX) { + imx258->long_exp_shift++; + val >>= 1; + } + + ret = imx258_write_reg(imx258, IMX258_REG_VTS, + IMX258_REG_VALUE_16BIT, val); + if (ret) + return ret; + + return imx258_write_reg(imx258, IMX258_LONG_EXP_SHIFT_REG, + IMX258_REG_VALUE_08BIT, imx258->long_exp_shift); +} + static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) { struct imx258 *imx258 = @@ -823,7 +849,7 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) case V4L2_CID_EXPOSURE: ret = imx258_write_reg(imx258, IMX258_REG_EXPOSURE, IMX258_REG_VALUE_16BIT, - ctrl->val); + ctrl->val >> imx258->long_exp_shift); break; case V4L2_CID_DIGITAL_GAIN: ret = imx258_update_digital_gain(imx258, IMX258_REG_VALUE_16BIT, @@ -855,9 +881,8 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) } break; case V4L2_CID_VBLANK: - ret = imx258_write_reg(imx258, IMX258_REG_VTS, - IMX258_REG_VALUE_16BIT, - imx258->cur_mode->height + ctrl->val); + ret = imx258_set_frame_length(imx258, + imx258->cur_mode->height + ctrl->val); break; default: dev_info(&client->dev, @@ -983,8 +1008,9 @@ static int imx258_set_pad_format(struct v4l2_subdev *sd, imx258->cur_mode->height; __v4l2_ctrl_modify_range( imx258->vblank, vblank_min, - IMX258_VTS_MAX - imx258->cur_mode->height, 1, - vblank_def); + ((1 << IMX258_LONG_EXP_SHIFT_MAX) * IMX258_VTS_MAX) - + imx258->cur_mode->height, + 1, vblank_def); __v4l2_ctrl_s_ctrl(imx258->vblank, vblank_def); h_blank = imx258->link_freq_configs[mode->link_freq_index].pixels_per_line
The sensor has a register CIT_LSHIFT which extends the exposure and frame times by the specified power of 2 for longer exposure times. Add support for this by configuring this register via V4L2_CID_VBLANK and extending the V4L2_CID_EXPOSURE range accordingly. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> --- drivers/media/i2c/imx258.c | 38 ++++++++++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 6 deletions(-)