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 RAW8, but normal binning
limits the framerate on RAW10 480p [1]. So with this patch we switch to
using special binning (with 2x pixel rate) wherever possible.
[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 | 126 +++++++++++++++++++++++++++++----------------
1 file changed, 81 insertions(+), 45 deletions(-)
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index e4aa6e66b673bb7a8942bf8daf27267c2884ec95..c445987de2c3e933ea9c49ba3e00a15663ef5f2e 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -144,6 +144,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 */
@@ -295,13 +301,13 @@ static const struct imx219_mode supported_modes[] = {
.fll_def = 1707,
},
{
- /* 2x2 binned 30fps mode */
+ /* 2x2 binned 60fps mode */
.width = 1640,
.height = 1232,
.fll_def = 1707,
},
{
- /* 640x480 30fps mode */
+ /* 640x480 60fps mode */
.width = 640,
.height = 480,
.fll_def = 1707,
@@ -356,6 +362,59 @@ 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 enum binning_mode imx219_get_binning(struct imx219 *imx219, u8 *bin_h,
+ u8 *bin_v)
+{
+ struct v4l2_subdev_state *state =
+ v4l2_subdev_get_locked_active_state(&imx219->sd);
+ const struct v4l2_mbus_framefmt *format =
+ v4l2_subdev_state_get_format(state, 0);
+ const struct v4l2_rect *crop = v4l2_subdev_state_get_crop(state, 0);
+
+ *bin_h = crop->width / format->width;
+ *bin_v = crop->height / format->height;
+
+ if (*bin_h == 2 && *bin_v == 2)
+ return BINNING_ANALOG_X2;
+ else if (*bin_h == 2 || *bin_v == 2)
+ /*
+ * Don't use analog binning if only one dimension
+ * is binned, as it crops the other dimension
+ */
+ return BINNING_X2;
+ else
+ return BINNING_NONE;
+}
+
+static inline u32 imx219_get_rate_factor(struct imx219 *imx219)
+{
+ u8 bin_h, bin_v;
+ enum binning_mode binning = imx219_get_binning(imx219, &bin_h, &bin_v);
+
+ if (binning == BINNING_ANALOG_X2)
+ return 2;
+
+ return 1;
+}
+
/* -----------------------------------------------------------------------------
* Controls
*/
@@ -367,10 +426,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;
+ u32 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;
@@ -399,7 +460,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,
@@ -416,7 +477,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
break;
case V4L2_CID_VBLANK:
cci_write(imx219->regmap, IMX219_REG_FRM_LENGTH_A,
- format->height + ctrl->val, &ret);
+ (format->height + ctrl->val) / rate_factor, &ret);
break;
case V4L2_CID_HBLANK:
cci_write(imx219->regmap, IMX219_REG_LINE_LENGTH_A,
@@ -587,29 +648,14 @@ 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;
+ enum binning_mode binning;
+ u8 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);
@@ -620,28 +666,11 @@ 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);
+ binning = imx219_get_binning(imx219, &bin_h, &bin_v);
+ cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H,
+ (bin_h == 2) ? binning : BINNING_NONE, &ret);
+ cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V,
+ (bin_v == 2) ? binning : BINNING_NONE, &ret);
cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
format->width, &ret);
@@ -846,6 +875,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
int exposure_max;
int exposure_def;
int hblank;
+ int pixel_rate;
/* Update limits and set FPS to default */
__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
@@ -874,6 +904,12 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
IMX219_LLP_MAX - mode->width, 1,
IMX219_LLP_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) *
+ imx219_get_rate_factor(imx219);
+ __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate,
+ pixel_rate, 1, pixel_rate);
}
return 0;
--
2.48.1Hi Jai
On Tue, Feb 04, 2025 at 12:34:40PM +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 RAW8, but normal binning
> limits the framerate on RAW10 480p [1]. So with this patch we switch to
> using special binning (with 2x pixel rate) wherever possible.
>
> [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 | 126 +++++++++++++++++++++++++++++----------------
> 1 file changed, 81 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index e4aa6e66b673bb7a8942bf8daf27267c2884ec95..c445987de2c3e933ea9c49ba3e00a15663ef5f2e 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -144,6 +144,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 */
> @@ -295,13 +301,13 @@ static const struct imx219_mode supported_modes[] = {
> .fll_def = 1707,
> },
> {
> - /* 2x2 binned 30fps mode */
> + /* 2x2 binned 60fps mode */
> .width = 1640,
> .height = 1232,
> .fll_def = 1707,
> },
> {
> - /* 640x480 30fps mode */
> + /* 640x480 60fps mode */
> .width = 640,
> .height = 480,
> .fll_def = 1707,
> @@ -356,6 +362,59 @@ 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;
> + }
> +}
Is this change related ?
> +
> +static enum binning_mode imx219_get_binning(struct imx219 *imx219, u8 *bin_h,
> + u8 *bin_v)
> +{
> + struct v4l2_subdev_state *state =
> + v4l2_subdev_get_locked_active_state(&imx219->sd);
> + const struct v4l2_mbus_framefmt *format =
> + v4l2_subdev_state_get_format(state, 0);
> + const struct v4l2_rect *crop = v4l2_subdev_state_get_crop(state, 0);
> +
> + *bin_h = crop->width / format->width;
> + *bin_v = crop->height / format->height;
> +
> + if (*bin_h == 2 && *bin_v == 2)
> + return BINNING_ANALOG_X2;
> + else if (*bin_h == 2 || *bin_v == 2)
> + /*
> + * Don't use analog binning if only one dimension
> + * is binned, as it crops the other dimension
> + */
> + return BINNING_X2;
> + else
> + return BINNING_NONE;
This function is used in two places, for two different reasons:
1) in imx219_get_rate_factor() to know if ANALOG is used
2) in imx219_set_framefmt() to calculate what value to write to the
horizontal/vertical registers
We now know that ANALOG binning is either applied to both horizontal
and vertical directions or it is not activated at all.
I wonder if you should do here
u32 hbin = crop->width / format->width;
u32 vbin = crop->height / format->height;
*h_bin = IMX219_BINNING_NONE;
*v_bin = IMX219_BINNING_NONE;
/*
* Use analog binning if only both dimensions are binned, as the
* other dimension is cropped.
*/
if (hbin == 2 && vbin == 2) {
*h_bin = IMX219_BINNING_X2_ANALOG;
*v_bin = IMX219_BINNING_X2_ANALOG;
return;
}
if (hbin == 2)
*h_bin = IMX219_BINNING_X2;
if (vbin == 2)
*v_bin = IMX219_BINNING_X2;
return;
And make the two callers
imx219_set_framefmt()
imx219_get_binning(imx219, &bin_h, &bin_v);
cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret)
cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret);
imx219_get_rate_factor()
imx219_get_binning(imx219, &bin_h, &bin_v);
return (bin_h & bin_v) == IMX219_BINNING_X2_ANALOG ? 2 : 1;
You could probably get rid of the enum in this way.
Just a small suggestion
> +}
> +
> +static inline u32 imx219_get_rate_factor(struct imx219 *imx219)
> +{
> + u8 bin_h, bin_v;
> + enum binning_mode binning = imx219_get_binning(imx219, &bin_h, &bin_v);
> +
> + if (binning == BINNING_ANALOG_X2)
> + return 2;
> +
> + return 1;
> +}
> +
> /* -----------------------------------------------------------------------------
> * Controls
> */
> @@ -367,10 +426,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;
> + u32 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;
> @@ -399,7 +460,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,
> @@ -416,7 +477,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> break;
> case V4L2_CID_VBLANK:
> cci_write(imx219->regmap, IMX219_REG_FRM_LENGTH_A,
> - format->height + ctrl->val, &ret);
> + (format->height + ctrl->val) / rate_factor, &ret);
Isn't this (and exposure) compensatd by the doubled pixel rate ?
Applications use the pixel rate to compute the line duration and from
there transform the frame duration and the exposure in lines, don't
they ?
Overall, very nice to be able to double the achievable frame rate
without any artifacts! Good job!
Thanks
j
> break;
> case V4L2_CID_HBLANK:
> cci_write(imx219->regmap, IMX219_REG_LINE_LENGTH_A,
> @@ -587,29 +648,14 @@ 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;
> + enum binning_mode binning;
> + u8 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);
> @@ -620,28 +666,11 @@ 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);
> + binning = imx219_get_binning(imx219, &bin_h, &bin_v);
> + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H,
> + (bin_h == 2) ? binning : BINNING_NONE, &ret);
> + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V,
> + (bin_v == 2) ? binning : BINNING_NONE, &ret);
>
> cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
> format->width, &ret);
> @@ -846,6 +875,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> int exposure_max;
> int exposure_def;
> int hblank;
> + int pixel_rate;
>
> /* Update limits and set FPS to default */
> __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> @@ -874,6 +904,12 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> IMX219_LLP_MAX - mode->width, 1,
> IMX219_LLP_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) *
> + imx219_get_rate_factor(imx219);
> + __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate,
> + pixel_rate, 1, pixel_rate);
> }
>
> return 0;
>
> --
> 2.48.1
>
Hi Jacopo,
On Feb 07, 2025 at 17:49:19 +0100, Jacopo Mondi wrote:
> Hi Jai
>
> On Tue, Feb 04, 2025 at 12:34:40PM +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 RAW8, but normal binning
> > limits the framerate on RAW10 480p [1]. So with this patch we switch to
> > using special binning (with 2x pixel rate) wherever possible.
> >
> > [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 | 126 +++++++++++++++++++++++++++++----------------
> > 1 file changed, 81 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index e4aa6e66b673bb7a8942bf8daf27267c2884ec95..c445987de2c3e933ea9c49ba3e00a15663ef5f2e 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -144,6 +144,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 */
> > @@ -295,13 +301,13 @@ static const struct imx219_mode supported_modes[] = {
> > .fll_def = 1707,
> > },
> > {
> > - /* 2x2 binned 30fps mode */
> > + /* 2x2 binned 60fps mode */
> > .width = 1640,
> > .height = 1232,
> > .fll_def = 1707,
> > },
> > {
> > - /* 640x480 30fps mode */
> > + /* 640x480 60fps mode */
> > .width = 640,
> > .height = 480,
> > .fll_def = 1707,
> > @@ -356,6 +362,59 @@ 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;
> > + }
> > +}
>
> Is this change related ?
>
Ah I think I split it out as a separate function during a previous
iteration, but it is not really needed anymore.
> > +
> > +static enum binning_mode imx219_get_binning(struct imx219 *imx219, u8 *bin_h,
> > + u8 *bin_v)
> > +{
> > + struct v4l2_subdev_state *state =
> > + v4l2_subdev_get_locked_active_state(&imx219->sd);
> > + const struct v4l2_mbus_framefmt *format =
> > + v4l2_subdev_state_get_format(state, 0);
> > + const struct v4l2_rect *crop = v4l2_subdev_state_get_crop(state, 0);
> > +
> > + *bin_h = crop->width / format->width;
> > + *bin_v = crop->height / format->height;
> > +
> > + if (*bin_h == 2 && *bin_v == 2)
> > + return BINNING_ANALOG_X2;
> > + else if (*bin_h == 2 || *bin_v == 2)
> > + /*
> > + * Don't use analog binning if only one dimension
> > + * is binned, as it crops the other dimension
> > + */
> > + return BINNING_X2;
> > + else
> > + return BINNING_NONE;
>
> This function is used in two places, for two different reasons:
>
> 1) in imx219_get_rate_factor() to know if ANALOG is used
> 2) in imx219_set_framefmt() to calculate what value to write to the
> horizontal/vertical registers
>
> We now know that ANALOG binning is either applied to both horizontal
> and vertical directions or it is not activated at all.
>
> I wonder if you should do here
>
> u32 hbin = crop->width / format->width;
> u32 vbin = crop->height / format->height;
>
> *h_bin = IMX219_BINNING_NONE;
> *v_bin = IMX219_BINNING_NONE;
>
> /*
> * Use analog binning if only both dimensions are binned, as the
> * other dimension is cropped.
> */
> if (hbin == 2 && vbin == 2) {
> *h_bin = IMX219_BINNING_X2_ANALOG;
> *v_bin = IMX219_BINNING_X2_ANALOG;
>
> return;
> }
>
> if (hbin == 2)
> *h_bin = IMX219_BINNING_X2;
> if (vbin == 2)
> *v_bin = IMX219_BINNING_X2;
>
> return;
>
> And make the two callers
>
> imx219_set_framefmt()
> imx219_get_binning(imx219, &bin_h, &bin_v);
> cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret)
> cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret);
>
> imx219_get_rate_factor()
> imx219_get_binning(imx219, &bin_h, &bin_v);
>
> return (bin_h & bin_v) == IMX219_BINNING_X2_ANALOG ? 2 : 1;
>
Looking at it from scratch, that does look much cleaner way to handle
it. Will do in next revision.
Thanks for the review.
>
> You could probably get rid of the enum in this way.
>
> Just a small suggestion
>
>
> > +}
> > +
> > +static inline u32 imx219_get_rate_factor(struct imx219 *imx219)
> > +{
> > + u8 bin_h, bin_v;
> > + enum binning_mode binning = imx219_get_binning(imx219, &bin_h, &bin_v);
> > +
> > + if (binning == BINNING_ANALOG_X2)
> > + return 2;
> > +
> > + return 1;
> > +}
> > +
> > /* -----------------------------------------------------------------------------
> > * Controls
> > */
> > @@ -367,10 +426,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;
> > + u32 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;
> > @@ -399,7 +460,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,
> > @@ -416,7 +477,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> > break;
> > case V4L2_CID_VBLANK:
> > cci_write(imx219->regmap, IMX219_REG_FRM_LENGTH_A,
> > - format->height + ctrl->val, &ret);
> > + (format->height + ctrl->val) / rate_factor, &ret);
>
>
> Isn't this (and exposure) compensatd by the doubled pixel rate ?
>
> Applications use the pixel rate to compute the line duration and from
> there transform the frame duration and the exposure in lines, don't
> they ?
>
> Overall, very nice to be able to double the achievable frame rate
> without any artifacts! Good job!
>
> Thanks
> j
>
> > break;
> > case V4L2_CID_HBLANK:
> > cci_write(imx219->regmap, IMX219_REG_LINE_LENGTH_A,
> > @@ -587,29 +648,14 @@ 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;
> > + enum binning_mode binning;
> > + u8 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);
> > @@ -620,28 +666,11 @@ 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);
> > + binning = imx219_get_binning(imx219, &bin_h, &bin_v);
> > + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H,
> > + (bin_h == 2) ? binning : BINNING_NONE, &ret);
> > + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V,
> > + (bin_v == 2) ? binning : BINNING_NONE, &ret);
> >
> > cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
> > format->width, &ret);
> > @@ -846,6 +875,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > int exposure_max;
> > int exposure_def;
> > int hblank;
> > + int pixel_rate;
> >
> > /* Update limits and set FPS to default */
> > __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > @@ -874,6 +904,12 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > IMX219_LLP_MAX - mode->width, 1,
> > IMX219_LLP_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) *
> > + imx219_get_rate_factor(imx219);
> > + __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate,
> > + pixel_rate, 1, pixel_rate);
> > }
> >
> > return 0;
> >
> > --
> > 2.48.1
> >
--
Jai
Hi Jacopo, Naush,
On Feb 07, 2025 at 17:49:19 +0100, Jacopo Mondi wrote:
> Hi Jai
>
> On Tue, Feb 04, 2025 at 12:34:40PM +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 RAW8, but normal binning
> > limits the framerate on RAW10 480p [1]. So with this patch we switch to
> > using special binning (with 2x pixel rate) wherever possible.
> >
> > [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>
> > ---
[snip]
> > @@ -367,10 +426,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;
> > + u32 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;
> > @@ -399,7 +460,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,
> > @@ -416,7 +477,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> > break;
> > case V4L2_CID_VBLANK:
> > cci_write(imx219->regmap, IMX219_REG_FRM_LENGTH_A,
> > - format->height + ctrl->val, &ret);
> > + (format->height + ctrl->val) / rate_factor, &ret);
>
>
> Isn't this (and exposure) compensatd by the doubled pixel rate ?
>
The datasheet mentions that FRM_LENGTH_A register is in units of 2xLines
when analog binning mode is selected. And the exposure is also usually
in unit of lines, so I assume that is why the same division was made in
the original commit by Naush [1]
[1] https://github.com/raspberrypi/linux/commit/caebe4fe817b5079
> Applications use the pixel rate to compute the line duration and from
> there transform the frame duration and the exposure in lines, don't
> they ?
While this change doesn't update the user-side of the control values,
only the register values, I wonder if there is a clean way to handle
this without updating some assumptions in the application.
The IMX219 pixel clock behaves differently with analog binning compared
to (most of our) intuitions, where rather than doubling the horizontal
pixel clock, each line is still read-out in the same time but the number
of lines read are halved.. at least that's the best explanation I have
from these results:
https://lore.kernel.org/linux-media/zla2sogd7ov3yz2k2je6zrgh3uzeermowlaixt3qkcioturppo@tswbw354tpdk/
And that is why the total pixel rate is doubled, but the actual line
duration should be the same as a digitally binned or cropped format.
>
> Overall, very nice to be able to double the achievable frame rate
> without any artifacts! Good job!
>
> Thanks
> j
>
Thanks,
Jai
> > break;
> > case V4L2_CID_HBLANK:
> > cci_write(imx219->regmap, IMX219_REG_LINE_LENGTH_A,
[snip]
© 2016 - 2025 Red Hat, Inc.