diff mbox series

[14/21] media: i2c: imx258: Add support for long exposure modes

Message ID 20230530173000.3060865-15-dave.stevenson@raspberrypi.com (mailing list archive)
State New, archived
Headers show
Series imx258 improvements series | expand

Commit Message

Dave Stevenson May 30, 2023, 5:29 p.m. UTC 
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

Jacopo Mondi June 2, 2023, 1:38 p.m. UTC | #1 
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
>
Dave Stevenson June 2, 2023, 5:12 p.m. UTC | #2 
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 mbox series

Patch

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