[14/21] media: i2c: imx258: Add support for long exposure modes
Commit Message
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(-)
Comments
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
> >
@@ -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