| Message ID | 20241125-imx219_fixes-v3-3-434fc0b541c8@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: i2c: imx219: Fixes for blanking and pixel rate | expand |
Hi Dave, On Nov 25, 2024 at 18:40:50 +0000, Dave Stevenson wrote: > Hi Jai > > On Mon, 25 Nov 2024 at 15:07, Jai Luthra <jai.luthra@ideasonboard.com> wrote: > > > > When the analog binning mode is used for high framerate operation, > > the pixel rate is effectively doubled. Account for this when setting up > > the pixel clock rate, and applying the vblank and exposure controls. > > > > The previous logic only used analog binning for 8-bit modes, but normal > > binning limits the framerate on 10-bit 480p [1]. So with this patch we > > switch to using special binning (with 2x pixel rate) for all formats of > > 480p mode and 8-bit 1232p. > > > > [1]: https://github.com/raspberrypi/linux/issues/5493 > > > > Co-developed-by: Naushir Patuck <naush@raspberrypi.com> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Co-developed-by: Vinay Varma <varmavinaym@gmail.com> > > Signed-off-by: Vinay Varma <varmavinaym@gmail.com> > > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com> > > --- > > drivers/media/i2c/imx219.c | 120 ++++++++++++++++++++++++++++----------------- > > 1 file changed, 76 insertions(+), 44 deletions(-) > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > > index 970e6362d0ae3a9078daf337155e83d637bc1ca1..39b85cdee58318b080c867afd68ca33d14d3eda7 100644 > > --- a/drivers/media/i2c/imx219.c > > +++ b/drivers/media/i2c/imx219.c > > @@ -149,6 +149,12 @@ > > #define IMX219_PIXEL_ARRAY_WIDTH 3280U > > #define IMX219_PIXEL_ARRAY_HEIGHT 2464U > > > > +enum binning_mode { > > + BINNING_NONE = IMX219_BINNING_NONE, > > + BINNING_X2 = IMX219_BINNING_X2, > > + BINNING_ANALOG_X2 = IMX219_BINNING_X2_ANALOG, > > +}; > > + > > /* Mode : resolution and related config&values */ > > struct imx219_mode { > > /* Frame width */ > > @@ -337,6 +343,10 @@ struct imx219 { > > > > /* Two or Four lanes */ > > u8 lanes; > > + > > + /* Binning mode */ > > + enum binning_mode bin_h; > > + enum binning_mode bin_v; > > }; > > > > static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd) > > @@ -362,6 +372,36 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code) > > return imx219_mbus_formats[i]; > > } > > > > +static u32 imx219_get_format_bpp(const struct v4l2_mbus_framefmt *format) > > +{ > > + switch (format->code) { > > + case MEDIA_BUS_FMT_SRGGB8_1X8: > > + case MEDIA_BUS_FMT_SGRBG8_1X8: > > + case MEDIA_BUS_FMT_SGBRG8_1X8: > > + case MEDIA_BUS_FMT_SBGGR8_1X8: > > + return 8; > > + > > + case MEDIA_BUS_FMT_SRGGB10_1X10: > > + case MEDIA_BUS_FMT_SGRBG10_1X10: > > + case MEDIA_BUS_FMT_SGBRG10_1X10: > > + case MEDIA_BUS_FMT_SBGGR10_1X10: > > + default: > > + return 10; > > + } > > +} > > + > > +static int imx219_get_rate_factor(struct imx219 *imx219) > > +{ > > + switch (imx219->bin_v) { > > + case BINNING_NONE: > > + case BINNING_X2: > > + return 1; > > + case BINNING_ANALOG_X2: > > + return 2; > > + } > > + return -EINVAL; > > +} > > + > > /* ----------------------------------------------------------------------------- > > * Controls > > */ > > @@ -373,10 +413,12 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); > > const struct v4l2_mbus_framefmt *format; > > struct v4l2_subdev_state *state; > > + int rate_factor; > > int ret = 0; > > > > state = v4l2_subdev_get_locked_active_state(&imx219->sd); > > format = v4l2_subdev_state_get_format(state, 0); > > + rate_factor = imx219_get_rate_factor(imx219); > > > > if (ctrl->id == V4L2_CID_VBLANK) { > > int exposure_max, exposure_def; > > @@ -405,7 +447,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > > break; > > case V4L2_CID_EXPOSURE: > > cci_write(imx219->regmap, IMX219_REG_EXPOSURE, > > - ctrl->val, &ret); > > + ctrl->val / rate_factor, &ret); > > break; > > case V4L2_CID_DIGITAL_GAIN: > > cci_write(imx219->regmap, IMX219_REG_DIGITAL_GAIN, > > @@ -422,7 +464,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > > break; > > case V4L2_CID_VBLANK: > > cci_write(imx219->regmap, IMX219_REG_VTS, > > - format->height + ctrl->val, &ret); > > + (format->height + ctrl->val) / rate_factor, &ret); > > break; > > case V4L2_CID_HBLANK: > > cci_write(imx219->regmap, IMX219_REG_HTS, > > @@ -463,7 +505,8 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = { > > > > static unsigned long imx219_get_pixel_rate(struct imx219 *imx219) > > { > > - return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE; > > + return ((imx219->lanes == 2) ? IMX219_PIXEL_RATE : > > + IMX219_PIXEL_RATE_4LANE) * imx219_get_rate_factor(imx219); > > This feels wrong. > imx219_get_rate_factor will return the factor based on vertical > binning. I would have expected the pixel rate to change based on > horizontal binning. > You'd need a mode which uses different binning modes in each direction > to verify that though. > I did tests with only vertically-binned and only horizontally-binned modes, and the framerate captured seemed to indicate that analog binning only changes the pixelrate when it is vertically binned. With the following driver change on top of my series: diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c index 39b85cdee583..38039847ded5 100644 --- a/drivers/media/i2c/imx219.c +++ b/drivers/media/i2c/imx219.c @@ -312,6 +312,16 @@ static const struct imx219_mode supported_modes[] = { .height = 1232, .vts_def = 1763, }, + { + .width = 3280, + .height = 1232, + .vts_def = 1763, + }, + { + .width = 1624, + .height = 2464, + .vts_def = 1763, + }, { /* 640x480 30fps mode */ .width = 640, If I configure the sensor to 1624 x 2464 it uses analog horizontal binning and no vertical binning. With the default vblank = 531 and hblank = 1824, it captures 17.66 frames/s [1] 182400000/((1624+1824)*(2464+531)) = 17.66 So the pixelrate seems to have stayed the same. It's a stretch, but given that the datasheet mentions VTS unit is in 2-Lines when using analog binning mode and no change to the HTS unit, I think the sensor must be taking the same time to read out a single line of pixels irrespective of if they are binned digitally or not. > (Updating exposure and vblank values based on vertical binning makes > sense, as they are both in terms of lines). Agreed, only the pixelrate seems unintuitive. > > Dave > > > } > > > > /* Initialize control handlers */ > > @@ -592,29 +635,12 @@ static int imx219_set_framefmt(struct imx219 *imx219, > > { > > const struct v4l2_mbus_framefmt *format; > > const struct v4l2_rect *crop; > > - unsigned int bpp; > > - u64 bin_h, bin_v; > > + u32 bpp; > > int ret = 0; > > > > format = v4l2_subdev_state_get_format(state, 0); > > crop = v4l2_subdev_state_get_crop(state, 0); > > - > > - switch (format->code) { > > - case MEDIA_BUS_FMT_SRGGB8_1X8: > > - case MEDIA_BUS_FMT_SGRBG8_1X8: > > - case MEDIA_BUS_FMT_SGBRG8_1X8: > > - case MEDIA_BUS_FMT_SBGGR8_1X8: > > - bpp = 8; > > - break; > > - > > - case MEDIA_BUS_FMT_SRGGB10_1X10: > > - case MEDIA_BUS_FMT_SGRBG10_1X10: > > - case MEDIA_BUS_FMT_SGBRG10_1X10: > > - case MEDIA_BUS_FMT_SBGGR10_1X10: > > - default: > > - bpp = 10; > > - break; > > - } > > + bpp = imx219_get_format_bpp(format); > > > > cci_write(imx219->regmap, IMX219_REG_X_ADD_STA_A, > > crop->left - IMX219_PIXEL_ARRAY_LEFT, &ret); > > @@ -625,28 +651,8 @@ static int imx219_set_framefmt(struct imx219 *imx219, > > cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A, > > crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret); > > > > - switch (crop->width / format->width) { > > - case 1: > > - default: > > - bin_h = IMX219_BINNING_NONE; > > - break; > > - case 2: > > - bin_h = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2; > > - break; > > - } > > - > > - switch (crop->height / format->height) { > > - case 1: > > - default: > > - bin_v = IMX219_BINNING_NONE; > > - break; > > - case 2: > > - bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2; > > - break; > > - } > > - > > - cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret); > > - cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret); > > + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, imx219->bin_h, &ret); > > + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, imx219->bin_v, &ret); > > > > cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE, > > format->width, &ret); > > @@ -851,6 +857,27 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > > int exposure_max; > > int exposure_def; > > int hblank; > > + int pixel_rate; > > + u32 bpp = imx219_get_format_bpp(format); > > + enum binning_mode binning = BINNING_NONE; > > + > > + /* > > + * For 8-bit formats, analog horizontal binning is required, > > + * else the output image is garbage. > > + * For 10-bit formats, analog horizontal binning is optional, > > + * but still useful as it doubles the effective framerate. > > + * We can only use it with width <= 1624, as for higher values > > + * there are blocky artefacts. > > + * > > + * Vertical binning should match the horizontal binning mode. > > + */ > > + if (bin_h == 2 && (format->width <= 1624 || bpp == 8)) > > + binning = BINNING_ANALOG_X2; > > + else > > + binning = BINNING_X2; > > + > > + imx219->bin_h = (bin_h == 2) ? binning : BINNING_NONE; > > + imx219->bin_v = (bin_v == 2) ? binning : BINNING_NONE; > > > > /* Update limits and set FPS to default */ > > __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN, > > @@ -879,6 +906,11 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > > IMX219_PPL_MAX - mode->width, > > 1, IMX219_PPL_MIN - mode->width); > > __v4l2_ctrl_s_ctrl(imx219->hblank, hblank); > > + > > + /* Scale the pixel rate based on the mode specific factor */ > > + pixel_rate = imx219_get_pixel_rate(imx219); > > + __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate, > > + pixel_rate, 1, pixel_rate); > > } > > > > return 0; > > > > -- > > 2.47.0 > > [1]: - entity 41: imx219 1-0010 (1 pad, 1 link, 0 routes) type V4L2 subdev subtype Sensor flags 0 device node name /dev/v4l-subdev4 pad0: SOURCE [stream:0 fmt:SRGGB10_1X10/1624x2464 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range crop.bounds:(8,8)/3280x2464 crop:(24,8)/3248x2464] -> "csis-32e40000.csi":0 [ENABLED] $ v4l2-ctl -d0 --stream-mmap <<<<<<<<<<<<<<<<<<< 17.66 fps <<<<<<<<<<<<<<<<<< 17.66 fps <<<<^C $ v4l2-ctl -d/dev/v4l-subdev4 -l | grep blanking vertical_blanking 0x009e0901 (int) : min=32 max=64303 step=1 default=531 value=531 horizontal_blanking 0x009e0902 (int) : min=1824 max=31128 step=1 default=1824 value=1824 Thanks, Jai
Hi Jai, On Mon, Nov 25, 2024 at 08:36:27PM +0530, Jai Luthra wrote: > When the analog binning mode is used for high framerate operation, > the pixel rate is effectively doubled. Account for this when setting up > the pixel clock rate, and applying the vblank and exposure controls. > > The previous logic only used analog binning for 8-bit modes, but normal > binning limits the framerate on 10-bit 480p [1]. So with this patch we > switch to using special binning (with 2x pixel rate) for all formats of > 480p mode and 8-bit 1232p. > > [1]: https://github.com/raspberrypi/linux/issues/5493 > > Co-developed-by: Naushir Patuck <naush@raspberrypi.com> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Co-developed-by: Vinay Varma <varmavinaym@gmail.com> > Signed-off-by: Vinay Varma <varmavinaym@gmail.com> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com> > --- > drivers/media/i2c/imx219.c | 120 ++++++++++++++++++++++++++++----------------- > 1 file changed, 76 insertions(+), 44 deletions(-) > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index 970e6362d0ae3a9078daf337155e83d637bc1ca1..39b85cdee58318b080c867afd68ca33d14d3eda7 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -149,6 +149,12 @@ > #define IMX219_PIXEL_ARRAY_WIDTH 3280U > #define IMX219_PIXEL_ARRAY_HEIGHT 2464U > > +enum binning_mode { > + BINNING_NONE = IMX219_BINNING_NONE, > + BINNING_X2 = IMX219_BINNING_X2, > + BINNING_ANALOG_X2 = IMX219_BINNING_X2_ANALOG, > +}; > + > /* Mode : resolution and related config&values */ > struct imx219_mode { > /* Frame width */ > @@ -337,6 +343,10 @@ struct imx219 { > > /* Two or Four lanes */ > u8 lanes; > + > + /* Binning mode */ > + enum binning_mode bin_h; > + enum binning_mode bin_v; > }; > > static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd) > @@ -362,6 +372,36 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code) > return imx219_mbus_formats[i]; > } > > +static u32 imx219_get_format_bpp(const struct v4l2_mbus_framefmt *format) > +{ > + switch (format->code) { > + case MEDIA_BUS_FMT_SRGGB8_1X8: > + case MEDIA_BUS_FMT_SGRBG8_1X8: > + case MEDIA_BUS_FMT_SGBRG8_1X8: > + case MEDIA_BUS_FMT_SBGGR8_1X8: > + return 8; > + > + case MEDIA_BUS_FMT_SRGGB10_1X10: > + case MEDIA_BUS_FMT_SGRBG10_1X10: > + case MEDIA_BUS_FMT_SGBRG10_1X10: > + case MEDIA_BUS_FMT_SBGGR10_1X10: > + default: > + return 10; > + } > +} > + > +static int imx219_get_rate_factor(struct imx219 *imx219) > +{ > + switch (imx219->bin_v) { > + case BINNING_NONE: > + case BINNING_X2: > + return 1; > + case BINNING_ANALOG_X2: > + return 2; FWIW, what the CCS driver does is that it exposes different horizontal blanking ranges for devices that use analogue binning. The rate is really about reading pixels and with analogue binning the rate is the same, it's just that fewer pixels are being (digitally) read (as they are binned). I wonder if this would be a workable approach for this sensor, too. Of course if the LLP behaves differently for this sensor, then we should probably just accept that. > + } > + return -EINVAL; > +} > + > /* ----------------------------------------------------------------------------- > * Controls > */ > @@ -373,10 +413,12 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); > const struct v4l2_mbus_framefmt *format; > struct v4l2_subdev_state *state; > + int rate_factor; u32? > int ret = 0; > > state = v4l2_subdev_get_locked_active_state(&imx219->sd); > format = v4l2_subdev_state_get_format(state, 0); > + rate_factor = imx219_get_rate_factor(imx219); > > if (ctrl->id == V4L2_CID_VBLANK) { > int exposure_max, exposure_def; > @@ -405,7 +447,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > break; > case V4L2_CID_EXPOSURE: > cci_write(imx219->regmap, IMX219_REG_EXPOSURE, > - ctrl->val, &ret); > + ctrl->val / rate_factor, &ret); Isn't the exposure in lines? It shouldn't be affected by the rate change, shouldn't it? > break; > case V4L2_CID_DIGITAL_GAIN: > cci_write(imx219->regmap, IMX219_REG_DIGITAL_GAIN, > @@ -422,7 +464,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > break; > case V4L2_CID_VBLANK: > cci_write(imx219->regmap, IMX219_REG_VTS, > - format->height + ctrl->val, &ret); > + (format->height + ctrl->val) / rate_factor, &ret); The same for vertical blanking. > break; > case V4L2_CID_HBLANK: > cci_write(imx219->regmap, IMX219_REG_HTS, > @@ -463,7 +505,8 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = { > > static unsigned long imx219_get_pixel_rate(struct imx219 *imx219) > { > - return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE; > + return ((imx219->lanes == 2) ? IMX219_PIXEL_RATE : > + IMX219_PIXEL_RATE_4LANE) * imx219_get_rate_factor(imx219); > } > > /* Initialize control handlers */ > @@ -592,29 +635,12 @@ static int imx219_set_framefmt(struct imx219 *imx219, > { > const struct v4l2_mbus_framefmt *format; > const struct v4l2_rect *crop; > - unsigned int bpp; > - u64 bin_h, bin_v; > + u32 bpp; > int ret = 0; > > format = v4l2_subdev_state_get_format(state, 0); > crop = v4l2_subdev_state_get_crop(state, 0); > - > - switch (format->code) { > - case MEDIA_BUS_FMT_SRGGB8_1X8: > - case MEDIA_BUS_FMT_SGRBG8_1X8: > - case MEDIA_BUS_FMT_SGBRG8_1X8: > - case MEDIA_BUS_FMT_SBGGR8_1X8: > - bpp = 8; > - break; > - > - case MEDIA_BUS_FMT_SRGGB10_1X10: > - case MEDIA_BUS_FMT_SGRBG10_1X10: > - case MEDIA_BUS_FMT_SGBRG10_1X10: > - case MEDIA_BUS_FMT_SBGGR10_1X10: > - default: > - bpp = 10; > - break; > - } > + bpp = imx219_get_format_bpp(format); > > cci_write(imx219->regmap, IMX219_REG_X_ADD_STA_A, > crop->left - IMX219_PIXEL_ARRAY_LEFT, &ret); > @@ -625,28 +651,8 @@ static int imx219_set_framefmt(struct imx219 *imx219, > cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A, > crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret); > > - switch (crop->width / format->width) { > - case 1: > - default: > - bin_h = IMX219_BINNING_NONE; > - break; > - case 2: > - bin_h = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2; > - break; > - } > - > - switch (crop->height / format->height) { > - case 1: > - default: > - bin_v = IMX219_BINNING_NONE; > - break; > - case 2: > - bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2; > - break; > - } > - > - cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret); > - cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret); > + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, imx219->bin_h, &ret); > + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, imx219->bin_v, &ret); Please run: $ ./scripts/checkpatch.pl --strict --max-line-length=80 > > cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE, > format->width, &ret); > @@ -851,6 +857,27 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > int exposure_max; > int exposure_def; > int hblank; > + int pixel_rate; > + u32 bpp = imx219_get_format_bpp(format); > + enum binning_mode binning = BINNING_NONE; > + > + /* > + * For 8-bit formats, analog horizontal binning is required, > + * else the output image is garbage. > + * For 10-bit formats, analog horizontal binning is optional, > + * but still useful as it doubles the effective framerate. > + * We can only use it with width <= 1624, as for higher values > + * there are blocky artefacts. This comment would benefit from rewrapping. > + * > + * Vertical binning should match the horizontal binning mode. > + */ > + if (bin_h == 2 && (format->width <= 1624 || bpp == 8)) > + binning = BINNING_ANALOG_X2; > + else > + binning = BINNING_X2; > + > + imx219->bin_h = (bin_h == 2) ? binning : BINNING_NONE; > + imx219->bin_v = (bin_v == 2) ? binning : BINNING_NONE; It'd be also nice to move the state information to sub-device state. > > /* Update limits and set FPS to default */ > __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN, > @@ -879,6 +906,11 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > IMX219_PPL_MAX - mode->width, > 1, IMX219_PPL_MIN - mode->width); > __v4l2_ctrl_s_ctrl(imx219->hblank, hblank); > + > + /* Scale the pixel rate based on the mode specific factor */ > + pixel_rate = imx219_get_pixel_rate(imx219); > + __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate, > + pixel_rate, 1, pixel_rate); > } > > return 0; >
Hi Sakari, On Nov 26, 2024 at 14:36:24 +0000, Sakari Ailus wrote: > Hi Jai, > > On Mon, Nov 25, 2024 at 08:36:27PM +0530, Jai Luthra wrote: > > When the analog binning mode is used for high framerate operation, > > the pixel rate is effectively doubled. Account for this when setting up > > the pixel clock rate, and applying the vblank and exposure controls. > > > > The previous logic only used analog binning for 8-bit modes, but normal > > binning limits the framerate on 10-bit 480p [1]. So with this patch we > > switch to using special binning (with 2x pixel rate) for all formats of > > 480p mode and 8-bit 1232p. > > > > [1]: https://github.com/raspberrypi/linux/issues/5493 > > > > Co-developed-by: Naushir Patuck <naush@raspberrypi.com> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Co-developed-by: Vinay Varma <varmavinaym@gmail.com> > > Signed-off-by: Vinay Varma <varmavinaym@gmail.com> > > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com> > > --- > > drivers/media/i2c/imx219.c | 120 ++++++++++++++++++++++++++++----------------- > > 1 file changed, 76 insertions(+), 44 deletions(-) > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > > index 970e6362d0ae3a9078daf337155e83d637bc1ca1..39b85cdee58318b080c867afd68ca33d14d3eda7 100644 > > --- a/drivers/media/i2c/imx219.c > > +++ b/drivers/media/i2c/imx219.c > > @@ -149,6 +149,12 @@ > > #define IMX219_PIXEL_ARRAY_WIDTH 3280U > > #define IMX219_PIXEL_ARRAY_HEIGHT 2464U > > > > +enum binning_mode { > > + BINNING_NONE = IMX219_BINNING_NONE, > > + BINNING_X2 = IMX219_BINNING_X2, > > + BINNING_ANALOG_X2 = IMX219_BINNING_X2_ANALOG, > > +}; > > + > > /* Mode : resolution and related config&values */ > > struct imx219_mode { > > /* Frame width */ > > @@ -337,6 +343,10 @@ struct imx219 { > > > > /* Two or Four lanes */ > > u8 lanes; > > + > > + /* Binning mode */ > > + enum binning_mode bin_h; > > + enum binning_mode bin_v; > > }; > > > > static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd) > > @@ -362,6 +372,36 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code) > > return imx219_mbus_formats[i]; > > } > > > > +static u32 imx219_get_format_bpp(const struct v4l2_mbus_framefmt *format) > > +{ > > + switch (format->code) { > > + case MEDIA_BUS_FMT_SRGGB8_1X8: > > + case MEDIA_BUS_FMT_SGRBG8_1X8: > > + case MEDIA_BUS_FMT_SGBRG8_1X8: > > + case MEDIA_BUS_FMT_SBGGR8_1X8: > > + return 8; > > + > > + case MEDIA_BUS_FMT_SRGGB10_1X10: > > + case MEDIA_BUS_FMT_SGRBG10_1X10: > > + case MEDIA_BUS_FMT_SGBRG10_1X10: > > + case MEDIA_BUS_FMT_SBGGR10_1X10: > > + default: > > + return 10; > > + } > > +} > > + > > +static int imx219_get_rate_factor(struct imx219 *imx219) > > +{ > > + switch (imx219->bin_v) { > > + case BINNING_NONE: > > + case BINNING_X2: > > + return 1; > > + case BINNING_ANALOG_X2: > > + return 2; > > FWIW, what the CCS driver does is that it exposes different horizontal > blanking ranges for devices that use analogue binning. The rate is really > about reading pixels and with analogue binning the rate is the same, it's > just that fewer pixels are being (digitally) read (as they are binned). I > wonder if this would be a workable approach for this sensor, too. Of course > if the LLP behaves differently for this sensor, then we should probably > just accept that. > IMX219 seems to be odd in this case, as the LLP doesn't change during analog binning. Shared some more details in this thread: https://lore.kernel.org/linux-media/20241125-imx219_fixes-v3-0-434fc0b541c8@ideasonboard.com/T/#m1da4206e91db12b8e377dc686935195fc5f4bb68 > > + } > > + return -EINVAL; > > +} > > + > > /* ----------------------------------------------------------------------------- > > * Controls > > */ > > @@ -373,10 +413,12 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); > > const struct v4l2_mbus_framefmt *format; > > struct v4l2_subdev_state *state; > > + int rate_factor; > > u32? > Fixed. > > int ret = 0; > > > > state = v4l2_subdev_get_locked_active_state(&imx219->sd); > > format = v4l2_subdev_state_get_format(state, 0); > > + rate_factor = imx219_get_rate_factor(imx219); > > > > if (ctrl->id == V4L2_CID_VBLANK) { > > int exposure_max, exposure_def; > > @@ -405,7 +447,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > > break; > > case V4L2_CID_EXPOSURE: > > cci_write(imx219->regmap, IMX219_REG_EXPOSURE, > > - ctrl->val, &ret); > > + ctrl->val / rate_factor, &ret); > > Isn't the exposure in lines? It shouldn't be affected by the rate change, > shouldn't it? > From the sensor datasheet the unit of FRAME_LENGTH register is updated to 2xLines when analog binning is used. And exposure and vertical blanking values are also in units of FRAME_LENGTH. This is also consistent with the behavior seen while testing. > > break; > > case V4L2_CID_DIGITAL_GAIN: > > cci_write(imx219->regmap, IMX219_REG_DIGITAL_GAIN, > > @@ -422,7 +464,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > > break; > > case V4L2_CID_VBLANK: > > cci_write(imx219->regmap, IMX219_REG_VTS, > > - format->height + ctrl->val, &ret); > > + (format->height + ctrl->val) / rate_factor, &ret); > > The same for vertical blanking. > > > break; > > case V4L2_CID_HBLANK: > > cci_write(imx219->regmap, IMX219_REG_HTS, > > @@ -463,7 +505,8 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = { > > > > static unsigned long imx219_get_pixel_rate(struct imx219 *imx219) > > { > > - return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE; > > + return ((imx219->lanes == 2) ? IMX219_PIXEL_RATE : > > + IMX219_PIXEL_RATE_4LANE) * imx219_get_rate_factor(imx219); > > } > > > > /* Initialize control handlers */ > > @@ -592,29 +635,12 @@ static int imx219_set_framefmt(struct imx219 *imx219, > > { > > const struct v4l2_mbus_framefmt *format; > > const struct v4l2_rect *crop; > > - unsigned int bpp; > > - u64 bin_h, bin_v; > > + u32 bpp; > > int ret = 0; > > > > format = v4l2_subdev_state_get_format(state, 0); > > crop = v4l2_subdev_state_get_crop(state, 0); > > - > > - switch (format->code) { > > - case MEDIA_BUS_FMT_SRGGB8_1X8: > > - case MEDIA_BUS_FMT_SGRBG8_1X8: > > - case MEDIA_BUS_FMT_SGBRG8_1X8: > > - case MEDIA_BUS_FMT_SBGGR8_1X8: > > - bpp = 8; > > - break; > > - > > - case MEDIA_BUS_FMT_SRGGB10_1X10: > > - case MEDIA_BUS_FMT_SGRBG10_1X10: > > - case MEDIA_BUS_FMT_SGBRG10_1X10: > > - case MEDIA_BUS_FMT_SBGGR10_1X10: > > - default: > > - bpp = 10; > > - break; > > - } > > + bpp = imx219_get_format_bpp(format); > > > > cci_write(imx219->regmap, IMX219_REG_X_ADD_STA_A, > > crop->left - IMX219_PIXEL_ARRAY_LEFT, &ret); > > @@ -625,28 +651,8 @@ static int imx219_set_framefmt(struct imx219 *imx219, > > cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A, > > crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret); > > > > - switch (crop->width / format->width) { > > - case 1: > > - default: > > - bin_h = IMX219_BINNING_NONE; > > - break; > > - case 2: > > - bin_h = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2; > > - break; > > - } > > - > > - switch (crop->height / format->height) { > > - case 1: > > - default: > > - bin_v = IMX219_BINNING_NONE; > > - break; > > - case 2: > > - bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2; > > - break; > > - } > > - > > - cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret); > > - cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret); > > + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, imx219->bin_h, &ret); > > + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, imx219->bin_v, &ret); > > Please run: > > $ ./scripts/checkpatch.pl --strict --max-line-length=80 > Oops, fixed in next revision. > > > > cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE, > > format->width, &ret); > > @@ -851,6 +857,27 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > > int exposure_max; > > int exposure_def; > > int hblank; > > + int pixel_rate; > > + u32 bpp = imx219_get_format_bpp(format); > > + enum binning_mode binning = BINNING_NONE; > > + > > + /* > > + * For 8-bit formats, analog horizontal binning is required, > > + * else the output image is garbage. > > + * For 10-bit formats, analog horizontal binning is optional, > > + * but still useful as it doubles the effective framerate. > > + * We can only use it with width <= 1624, as for higher values > > + * there are blocky artefacts. > > This comment would benefit from rewrapping. > Fixed. > > + * > > + * Vertical binning should match the horizontal binning mode. > > + */ > > + if (bin_h == 2 && (format->width <= 1624 || bpp == 8)) > > + binning = BINNING_ANALOG_X2; > > + else > > + binning = BINNING_X2; > > + > > + imx219->bin_h = (bin_h == 2) ? binning : BINNING_NONE; > > + imx219->bin_v = (bin_v == 2) ? binning : BINNING_NONE; > > It'd be also nice to move the state information to sub-device state. > I'm not sure I follow, do you mean the framework should store the binning mode, similar to how crop rectangle and interval are stored in v4l2_subdev_state? > > > > /* Update limits and set FPS to default */ > > __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN, > > @@ -879,6 +906,11 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > > IMX219_PPL_MAX - mode->width, > > 1, IMX219_PPL_MIN - mode->width); > > __v4l2_ctrl_s_ctrl(imx219->hblank, hblank); > > + > > + /* Scale the pixel rate based on the mode specific factor */ > > + pixel_rate = imx219_get_pixel_rate(imx219); > > + __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate, > > + pixel_rate, 1, pixel_rate); > > } > > > > return 0; > > > > -- > Kind regards, > > Sakari Ailus Thanks, Jai
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c index 970e6362d0ae3a9078daf337155e83d637bc1ca1..39b85cdee58318b080c867afd68ca33d14d3eda7 100644 --- a/drivers/media/i2c/imx219.c +++ b/drivers/media/i2c/imx219.c @@ -149,6 +149,12 @@ #define IMX219_PIXEL_ARRAY_WIDTH 3280U #define IMX219_PIXEL_ARRAY_HEIGHT 2464U +enum binning_mode { + BINNING_NONE = IMX219_BINNING_NONE, + BINNING_X2 = IMX219_BINNING_X2, + BINNING_ANALOG_X2 = IMX219_BINNING_X2_ANALOG, +}; + /* Mode : resolution and related config&values */ struct imx219_mode { /* Frame width */ @@ -337,6 +343,10 @@ struct imx219 { /* Two or Four lanes */ u8 lanes; + + /* Binning mode */ + enum binning_mode bin_h; + enum binning_mode bin_v; }; static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd) @@ -362,6 +372,36 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code) return imx219_mbus_formats[i]; } +static u32 imx219_get_format_bpp(const struct v4l2_mbus_framefmt *format) +{ + switch (format->code) { + case MEDIA_BUS_FMT_SRGGB8_1X8: + case MEDIA_BUS_FMT_SGRBG8_1X8: + case MEDIA_BUS_FMT_SGBRG8_1X8: + case MEDIA_BUS_FMT_SBGGR8_1X8: + return 8; + + case MEDIA_BUS_FMT_SRGGB10_1X10: + case MEDIA_BUS_FMT_SGRBG10_1X10: + case MEDIA_BUS_FMT_SGBRG10_1X10: + case MEDIA_BUS_FMT_SBGGR10_1X10: + default: + return 10; + } +} + +static int imx219_get_rate_factor(struct imx219 *imx219) +{ + switch (imx219->bin_v) { + case BINNING_NONE: + case BINNING_X2: + return 1; + case BINNING_ANALOG_X2: + return 2; + } + return -EINVAL; +} + /* ----------------------------------------------------------------------------- * Controls */ @@ -373,10 +413,12 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); const struct v4l2_mbus_framefmt *format; struct v4l2_subdev_state *state; + int rate_factor; int ret = 0; state = v4l2_subdev_get_locked_active_state(&imx219->sd); format = v4l2_subdev_state_get_format(state, 0); + rate_factor = imx219_get_rate_factor(imx219); if (ctrl->id == V4L2_CID_VBLANK) { int exposure_max, exposure_def; @@ -405,7 +447,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) break; case V4L2_CID_EXPOSURE: cci_write(imx219->regmap, IMX219_REG_EXPOSURE, - ctrl->val, &ret); + ctrl->val / rate_factor, &ret); break; case V4L2_CID_DIGITAL_GAIN: cci_write(imx219->regmap, IMX219_REG_DIGITAL_GAIN, @@ -422,7 +464,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) break; case V4L2_CID_VBLANK: cci_write(imx219->regmap, IMX219_REG_VTS, - format->height + ctrl->val, &ret); + (format->height + ctrl->val) / rate_factor, &ret); break; case V4L2_CID_HBLANK: cci_write(imx219->regmap, IMX219_REG_HTS, @@ -463,7 +505,8 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = { static unsigned long imx219_get_pixel_rate(struct imx219 *imx219) { - return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE; + return ((imx219->lanes == 2) ? IMX219_PIXEL_RATE : + IMX219_PIXEL_RATE_4LANE) * imx219_get_rate_factor(imx219); } /* Initialize control handlers */ @@ -592,29 +635,12 @@ static int imx219_set_framefmt(struct imx219 *imx219, { const struct v4l2_mbus_framefmt *format; const struct v4l2_rect *crop; - unsigned int bpp; - u64 bin_h, bin_v; + u32 bpp; int ret = 0; format = v4l2_subdev_state_get_format(state, 0); crop = v4l2_subdev_state_get_crop(state, 0); - - switch (format->code) { - case MEDIA_BUS_FMT_SRGGB8_1X8: - case MEDIA_BUS_FMT_SGRBG8_1X8: - case MEDIA_BUS_FMT_SGBRG8_1X8: - case MEDIA_BUS_FMT_SBGGR8_1X8: - bpp = 8; - break; - - case MEDIA_BUS_FMT_SRGGB10_1X10: - case MEDIA_BUS_FMT_SGRBG10_1X10: - case MEDIA_BUS_FMT_SGBRG10_1X10: - case MEDIA_BUS_FMT_SBGGR10_1X10: - default: - bpp = 10; - break; - } + bpp = imx219_get_format_bpp(format); cci_write(imx219->regmap, IMX219_REG_X_ADD_STA_A, crop->left - IMX219_PIXEL_ARRAY_LEFT, &ret); @@ -625,28 +651,8 @@ static int imx219_set_framefmt(struct imx219 *imx219, cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A, crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret); - switch (crop->width / format->width) { - case 1: - default: - bin_h = IMX219_BINNING_NONE; - break; - case 2: - bin_h = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2; - break; - } - - switch (crop->height / format->height) { - case 1: - default: - bin_v = IMX219_BINNING_NONE; - break; - case 2: - bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2; - break; - } - - cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret); - cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret); + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, imx219->bin_h, &ret); + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, imx219->bin_v, &ret); cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE, format->width, &ret); @@ -851,6 +857,27 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, int exposure_max; int exposure_def; int hblank; + int pixel_rate; + u32 bpp = imx219_get_format_bpp(format); + enum binning_mode binning = BINNING_NONE; + + /* + * For 8-bit formats, analog horizontal binning is required, + * else the output image is garbage. + * For 10-bit formats, analog horizontal binning is optional, + * but still useful as it doubles the effective framerate. + * We can only use it with width <= 1624, as for higher values + * there are blocky artefacts. + * + * Vertical binning should match the horizontal binning mode. + */ + if (bin_h == 2 && (format->width <= 1624 || bpp == 8)) + binning = BINNING_ANALOG_X2; + else + binning = BINNING_X2; + + imx219->bin_h = (bin_h == 2) ? binning : BINNING_NONE; + imx219->bin_v = (bin_v == 2) ? binning : BINNING_NONE; /* Update limits and set FPS to default */ __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN, @@ -879,6 +906,11 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, IMX219_PPL_MAX - mode->width, 1, IMX219_PPL_MIN - mode->width); __v4l2_ctrl_s_ctrl(imx219->hblank, hblank); + + /* Scale the pixel rate based on the mode specific factor */ + pixel_rate = imx219_get_pixel_rate(imx219); + __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate, + pixel_rate, 1, pixel_rate); } return 0;