| Message ID | 20241121-imx219_fixes-v2-2-7b068a60ea40@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: i2c: imx219: Fixes for blanking and pixel rate | expand |
Hi Jai On Thu, Nov 21, 2024 at 05:38:03PM +0530, Jai Luthra wrote: > From: Dave Stevenson <dave.stevenson@raspberrypi.com> > > The HBLANK control was read-only, and always configured such > that the sensor HTS register was 3448. This limited the maximum > exposure time that could be achieved to around 1.26 secs. > > Make HBLANK read/write so that the line time can be extended, > and thereby allow longer exposures (and slower frame rates). > Retain the overall HTS setting when changing modes rather than > resetting it to a default. > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com> > --- > drivers/media/i2c/imx219.c | 37 ++++++++++++++++++++++++------------- > 1 file changed, 24 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index f98aad74fe584a18e2fe7126f92bf294762a54e3..970e6362d0ae3a9078daf337155e83d637bc1ca1 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -76,8 +76,10 @@ > > #define IMX219_VBLANK_MIN 32 > > -/* HBLANK control - read only */ > -#define IMX219_PPL_DEFAULT 3448 > +/* HBLANK control range */ > +#define IMX219_PPL_MIN 0x0d78 > +#define IMX219_PPL_MAX 0x7ff0 > +#define IMX219_REG_HTS CCI_REG16(0x0162) > > #define IMX219_REG_LINE_LENGTH_A CCI_REG16(0x0162) > #define IMX219_REG_X_ADD_STA_A CCI_REG16(0x0164) > @@ -422,6 +424,10 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > cci_write(imx219->regmap, IMX219_REG_VTS, > format->height + ctrl->val, &ret); > break; > + case V4L2_CID_HBLANK: > + cci_write(imx219->regmap, IMX219_REG_HTS, > + format->width + ctrl->val, &ret); > + break; > case V4L2_CID_TEST_PATTERN_RED: > cci_write(imx219->regmap, IMX219_REG_TESTP_RED, > ctrl->val, &ret); > @@ -496,12 +502,10 @@ static int imx219_init_controls(struct imx219 *imx219) > V4L2_CID_VBLANK, IMX219_VBLANK_MIN, > IMX219_VTS_MAX - mode->height, 1, > mode->vts_def - mode->height); > - hblank = IMX219_PPL_DEFAULT - mode->width; > + hblank = IMX219_PPL_MIN - mode->width; > imx219->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, > V4L2_CID_HBLANK, hblank, hblank, > 1, hblank); > - if (imx219->hblank) > - imx219->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > exposure_max = mode->vts_def - 4; > exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ? > exposure_max : IMX219_EXPOSURE_DEFAULT; > @@ -817,6 +821,10 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > struct v4l2_mbus_framefmt *format; > struct v4l2_rect *crop; > unsigned int bin_h, bin_v; > + u32 prev_hts; > + > + format = v4l2_subdev_state_get_format(state, 0); > + prev_hts = format->width + imx219->hblank->val; > > mode = v4l2_find_nearest_size(supported_modes, > ARRAY_SIZE(supported_modes), > @@ -824,8 +832,6 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > fmt->format.width, fmt->format.height); > > imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code); > - > - format = v4l2_subdev_state_get_format(state, 0); > *format = fmt->format; > > /* > @@ -861,13 +867,18 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > exposure_max, imx219->exposure->step, > exposure_def); > /* > - * Currently PPL is fixed to IMX219_PPL_DEFAULT, so hblank > - * depends on mode->width only, and is not changeble in any > - * way other than changing the mode. > + * Retain PPL setting from previous mode so that the > + * line time does not change on a mode change. > + * Limits have to be recomputed as the controls define > + * the blanking only, so PPL values need to have the > + * mode width subtracted. > */ > - hblank = IMX219_PPL_DEFAULT - mode->width; > - __v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1, > - hblank); > + hblank = prev_hts - mode->width; > + __v4l2_ctrl_modify_range(imx219->hblank, > + IMX219_PPL_MIN - mode->width, > + IMX219_PPL_MAX - mode->width, > + 1, IMX219_PPL_MIN - mode->width); > + __v4l2_ctrl_s_ctrl(imx219->hblank, hblank); Thanks, this now looks correct to me Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > } > > return 0; > > -- > 2.47.0 > >
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c index f98aad74fe584a18e2fe7126f92bf294762a54e3..970e6362d0ae3a9078daf337155e83d637bc1ca1 100644 --- a/drivers/media/i2c/imx219.c +++ b/drivers/media/i2c/imx219.c @@ -76,8 +76,10 @@ #define IMX219_VBLANK_MIN 32 -/* HBLANK control - read only */ -#define IMX219_PPL_DEFAULT 3448 +/* HBLANK control range */ +#define IMX219_PPL_MIN 0x0d78 +#define IMX219_PPL_MAX 0x7ff0 +#define IMX219_REG_HTS CCI_REG16(0x0162) #define IMX219_REG_LINE_LENGTH_A CCI_REG16(0x0162) #define IMX219_REG_X_ADD_STA_A CCI_REG16(0x0164) @@ -422,6 +424,10 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) cci_write(imx219->regmap, IMX219_REG_VTS, format->height + ctrl->val, &ret); break; + case V4L2_CID_HBLANK: + cci_write(imx219->regmap, IMX219_REG_HTS, + format->width + ctrl->val, &ret); + break; case V4L2_CID_TEST_PATTERN_RED: cci_write(imx219->regmap, IMX219_REG_TESTP_RED, ctrl->val, &ret); @@ -496,12 +502,10 @@ static int imx219_init_controls(struct imx219 *imx219) V4L2_CID_VBLANK, IMX219_VBLANK_MIN, IMX219_VTS_MAX - mode->height, 1, mode->vts_def - mode->height); - hblank = IMX219_PPL_DEFAULT - mode->width; + hblank = IMX219_PPL_MIN - mode->width; imx219->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, V4L2_CID_HBLANK, hblank, hblank, 1, hblank); - if (imx219->hblank) - imx219->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; exposure_max = mode->vts_def - 4; exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ? exposure_max : IMX219_EXPOSURE_DEFAULT; @@ -817,6 +821,10 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *format; struct v4l2_rect *crop; unsigned int bin_h, bin_v; + u32 prev_hts; + + format = v4l2_subdev_state_get_format(state, 0); + prev_hts = format->width + imx219->hblank->val; mode = v4l2_find_nearest_size(supported_modes, ARRAY_SIZE(supported_modes), @@ -824,8 +832,6 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, fmt->format.width, fmt->format.height); imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code); - - format = v4l2_subdev_state_get_format(state, 0); *format = fmt->format; /* @@ -861,13 +867,18 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, exposure_max, imx219->exposure->step, exposure_def); /* - * Currently PPL is fixed to IMX219_PPL_DEFAULT, so hblank - * depends on mode->width only, and is not changeble in any - * way other than changing the mode. + * Retain PPL setting from previous mode so that the + * line time does not change on a mode change. + * Limits have to be recomputed as the controls define + * the blanking only, so PPL values need to have the + * mode width subtracted. */ - hblank = IMX219_PPL_DEFAULT - mode->width; - __v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1, - hblank); + hblank = prev_hts - mode->width; + __v4l2_ctrl_modify_range(imx219->hblank, + IMX219_PPL_MIN - mode->width, + IMX219_PPL_MAX - mode->width, + 1, IMX219_PPL_MIN - mode->width); + __v4l2_ctrl_s_ctrl(imx219->hblank, hblank); } return 0;