Hi Jacopo
On Fri, 2 Jun 2023 at 14:38, Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > ---
> > 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
> >
