[PATCH v2 0/3] media: i2c: imx219: Fixes for blanking and pixel rate

[PATCH v2 0/3] media: i2c: imx219: Fixes for blanking and pixel rate

[Jai Luthra]

This series has a few fixes for improving h/v blanking and pixel rate
reporting for the IMX219 sensor.

These patches are picked and modified (and squashed where applicable)
from the rpi-6.6.y vendor tree.

Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
Changes in v2:
- PATCH 1/3: Add R-By from Jacopo, Dave
- PATCH 2/3:
    - Keep IMX219_PPL_MIN macro value in hex, matching the datasheet
    - Fix prev_hts calculation, by moving it before updating pad format
- PATCH 3/3:
    - Store binning register values in enum binning_mode directly
    - Remove unnecessary pixel_rate variable in imx219_init_controls()
- Link to v1: https://lore.kernel.org/r/20241029-imx219_fixes-v1-0-b45dc3658b4e@ideasonboard.com

---
Dave Stevenson (1):
      media: i2c: imx219: make HBLANK r/w to allow longer exposures

David Plowman (1):
      media: i2c: imx219: Correct the minimum vblanking value

Jai Luthra (1):
      media: i2c: imx219: Scale the pixel rate for analog binning

 drivers/media/i2c/imx219.c | 177 ++++++++++++++++++++++++++++++---------------
 1 file changed, 119 insertions(+), 58 deletions(-)
---
base-commit: adc218676eef25575469234709c2d87185ca223a
change-id: 20241029-imx219_fixes-bfaac4a56200

Best regards,
-- 
Jai Luthra <jai.luthra@ideasonboard.com>

[PATCH v2 1/3] media: i2c: imx219: Correct the minimum vblanking value

[Jai Luthra]

From: David Plowman <david.plowman@raspberrypi.com>

The datasheet for this sensor documents the minimum vblanking as being
32 lines. It does fix some problems with occasional black lines at the
bottom of images (tested on Raspberry Pi).

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/imx219.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index e78a80b2bb2e455c857390b188c128b28c224778..f98aad74fe584a18e2fe7126f92bf294762a54e3 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -74,7 +74,7 @@
 #define IMX219_REG_VTS			CCI_REG16(0x0160)
 #define IMX219_VTS_MAX			0xffff
 
-#define IMX219_VBLANK_MIN		4
+#define IMX219_VBLANK_MIN		32
 
 /* HBLANK control - read only */
 #define IMX219_PPL_DEFAULT		3448

-- 
2.47.0

[PATCH v2 2/3] media: i2c: imx219: make HBLANK r/w to allow longer exposures

[Jai Luthra]

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);
 	}
 
 	return 0;

-- 
2.47.0

Re: [PATCH v2 2/3] media: i2c: imx219: make HBLANK r/w to allow longer exposures

[Jacopo Mondi]

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
>
>

[PATCH v2 3/3] media: i2c: imx219: Scale the pixel rate for analog binning

[Jai Luthra]

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.

To do this cleanly, re-introduce the book-keeping for which binning mode
is used with which resolution/format.

[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 | 138 ++++++++++++++++++++++++++++++---------------
 1 file changed, 94 insertions(+), 44 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 970e6362d0ae3a9078daf337155e83d637bc1ca1..ec795569361987ae30bff234e97fa34600bf5975 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -149,6 +149,18 @@
 #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,
+};
+
+enum binning_bit_depths {
+	BINNING_IDX_8_BIT,
+	BINNING_IDX_10_BIT,
+	BINNING_IDX_MAX
+};
+
 /* Mode : resolution and related config&values */
 struct imx219_mode {
 	/* Frame width */
@@ -158,6 +170,9 @@ struct imx219_mode {
 
 	/* V-timing */
 	unsigned int vts_def;
+
+	/* binning mode based on format code */
+	enum binning_mode binning[BINNING_IDX_MAX];
 };
 
 static const struct cci_reg_sequence imx219_common_regs[] = {
@@ -293,24 +308,40 @@ static const struct imx219_mode supported_modes[] = {
 		.width = 3280,
 		.height = 2464,
 		.vts_def = 3526,
+		.binning = {
+			[BINNING_IDX_8_BIT] = BINNING_NONE,
+			[BINNING_IDX_10_BIT] = BINNING_NONE,
+		},
 	},
 	{
 		/* 1080P 30fps cropped */
 		.width = 1920,
 		.height = 1080,
 		.vts_def = 1763,
+		.binning = {
+			[BINNING_IDX_8_BIT] = BINNING_NONE,
+			[BINNING_IDX_10_BIT] = BINNING_NONE,
+		},
 	},
 	{
 		/* 2x2 binned 30fps mode */
 		.width = 1640,
 		.height = 1232,
 		.vts_def = 1763,
+		.binning = {
+			[BINNING_IDX_8_BIT] = BINNING_ANALOG_X2,
+			[BINNING_IDX_10_BIT] = BINNING_X2,
+		},
 	},
 	{
 		/* 640x480 30fps mode */
 		.width = 640,
 		.height = 480,
 		.vts_def = 1763,
+		.binning = {
+			[BINNING_IDX_8_BIT] = BINNING_ANALOG_X2,
+			[BINNING_IDX_10_BIT] = BINNING_ANALOG_X2,
+		},
 	},
 };
 
@@ -337,6 +368,9 @@ struct imx219 {
 
 	/* Two or Four lanes */
 	u8 lanes;
+
+	/* Binning mode */
+	enum binning_mode binning;
 };
 
 static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
@@ -362,6 +396,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->binning) {
+	case BINNING_NONE:
+	case BINNING_X2:
+		return 1;
+	case BINNING_ANALOG_X2:
+		return 2;
+	}
+	return -EINVAL;
+}
+
 /* -----------------------------------------------------------------------------
  * Controls
  */
@@ -373,10 +437,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 +471,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 +488,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 +529,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 +659,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 +675,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->binning, &ret);
+	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, imx219->binning, &ret);
 
 	cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
 		  format->width, &ret);
@@ -851,6 +881,21 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 		int exposure_max;
 		int exposure_def;
 		int hblank;
+		int pixel_rate;
+
+		/* Update binning mode based on format */
+		switch (imx219_get_format_bpp(format)) {
+		case 8:
+			imx219->binning = mode->binning[BINNING_IDX_8_BIT];
+			break;
+
+		case 10:
+			imx219->binning = mode->binning[BINNING_IDX_10_BIT];
+			break;
+
+		default:
+			imx219->binning = BINNING_NONE;
+		}
 
 		/* Update limits and set FPS to default */
 		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
@@ -879,6 +924,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

Re: [PATCH v2 3/3] media: i2c: imx219: Scale the pixel rate for analog binning

[Jacopo Mondi]

Hi Jai

On Thu, Nov 21, 2024 at 05:38:04PM +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.
>
> To do this cleanly, re-introduce the book-keeping for which binning mode
> is used with which resolution/format.

I think the patch is correct, however this goes a bit in the opposite
direction of making the driver freely configurable. IOW the more we
store in modes, the harder it will become to freely configure the
sensor. I know there are different opinions on how much this is
actually an issue, with valid arguments on both sides, but as I recall
freely configurable was a goal of Laurent's series, let me cc him
explicitly.

>
> [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 | 138 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 94 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 970e6362d0ae3a9078daf337155e83d637bc1ca1..ec795569361987ae30bff234e97fa34600bf5975 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -149,6 +149,18 @@
>  #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,
> +};
> +
> +enum binning_bit_depths {
> +	BINNING_IDX_8_BIT,
> +	BINNING_IDX_10_BIT,
> +	BINNING_IDX_MAX
> +};
> +
>  /* Mode : resolution and related config&values */
>  struct imx219_mode {
>  	/* Frame width */
> @@ -158,6 +170,9 @@ struct imx219_mode {
>
>  	/* V-timing */
>  	unsigned int vts_def;
> +
> +	/* binning mode based on format code */
> +	enum binning_mode binning[BINNING_IDX_MAX];
>  };
>
>  static const struct cci_reg_sequence imx219_common_regs[] = {
> @@ -293,24 +308,40 @@ static const struct imx219_mode supported_modes[] = {
>  		.width = 3280,
>  		.height = 2464,
>  		.vts_def = 3526,
> +		.binning = {
> +			[BINNING_IDX_8_BIT] = BINNING_NONE,
> +			[BINNING_IDX_10_BIT] = BINNING_NONE,
> +		},
>  	},
>  	{
>  		/* 1080P 30fps cropped */
>  		.width = 1920,
>  		.height = 1080,
>  		.vts_def = 1763,
> +		.binning = {
> +			[BINNING_IDX_8_BIT] = BINNING_NONE,
> +			[BINNING_IDX_10_BIT] = BINNING_NONE,
> +		},
>  	},
>  	{
>  		/* 2x2 binned 30fps mode */
>  		.width = 1640,
>  		.height = 1232,
>  		.vts_def = 1763,
> +		.binning = {
> +			[BINNING_IDX_8_BIT] = BINNING_ANALOG_X2,
> +			[BINNING_IDX_10_BIT] = BINNING_X2,
> +		},
>  	},
>  	{
>  		/* 640x480 30fps mode */
>  		.width = 640,
>  		.height = 480,
>  		.vts_def = 1763,
> +		.binning = {
> +			[BINNING_IDX_8_BIT] = BINNING_ANALOG_X2,
> +			[BINNING_IDX_10_BIT] = BINNING_ANALOG_X2,
> +		},
>  	},
>  };
>
> @@ -337,6 +368,9 @@ struct imx219 {
>
>  	/* Two or Four lanes */
>  	u8 lanes;
> +
> +	/* Binning mode */
> +	enum binning_mode binning;
>  };
>
>  static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> @@ -362,6 +396,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->binning) {
> +	case BINNING_NONE:
> +	case BINNING_X2:
> +		return 1;
> +	case BINNING_ANALOG_X2:
> +		return 2;
> +	}
> +	return -EINVAL;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Controls
>   */
> @@ -373,10 +437,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 +471,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 +488,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 +529,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 +659,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 +675,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;

So if I got this right, another way of handling this would be to

		bin_v = (bpp == 8 || (format->width == 640 && format->height = 480)
                      ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;


I'm not sure how much better this is, as then freely configure the
sensor at (random numbers) 720x480@10bpp would not use analog binning
while it might be beneficial. Actually knowing what analog mode is how
it shuold be used would help, but if I recall correctly it wasn't
clear from documentation or not fully clarified by Sony ?

When it comes to scaling PIXEL_RATE, the above switch could be moved
to set_pad_format and store imx219->binning to be
1) used here
2) used in s_ctrl

like this patch does already.

Opinions ?

> -		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->binning, &ret);
> +	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, imx219->binning, &ret);
>
>  	cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
>  		  format->width, &ret);
> @@ -851,6 +881,21 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  		int exposure_max;
>  		int exposure_def;
>  		int hblank;
> +		int pixel_rate;
> +
> +		/* Update binning mode based on format */
> +		switch (imx219_get_format_bpp(format)) {
> +		case 8:
> +			imx219->binning = mode->binning[BINNING_IDX_8_BIT];
> +			break;
> +
> +		case 10:
> +			imx219->binning = mode->binning[BINNING_IDX_10_BIT];
> +			break;
> +
> +		default:
> +			imx219->binning = BINNING_NONE;
> +		}
>
>  		/* Update limits and set FPS to default */
>  		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> @@ -879,6 +924,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
>
>

Re: [PATCH v2 3/3] media: i2c: imx219: Scale the pixel rate for analog binning

[Laurent Pinchart]

On Fri, Nov 22, 2024 at 11:31:58AM +0100, Jacopo Mondi wrote:
> On Thu, Nov 21, 2024 at 05:38:04PM +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.
> >
> > To do this cleanly, re-introduce the book-keeping for which binning mode
> > is used with which resolution/format.
> 
> I think the patch is correct, however this goes a bit in the opposite
> direction of making the driver freely configurable. IOW the more we
> store in modes, the harder it will become to freely configure the
> sensor. I know there are different opinions on how much this is
> actually an issue, with valid arguments on both sides, but as I recall
> freely configurable was a goal of Laurent's series, let me cc him
> explicitly.

Correct. I would like the binning configuration to be computed by the
driver, not hardcoded. Let's do better than this.

> > [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 | 138 ++++++++++++++++++++++++++++++---------------
> >  1 file changed, 94 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 970e6362d0ae3a9078daf337155e83d637bc1ca1..ec795569361987ae30bff234e97fa34600bf5975 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -149,6 +149,18 @@
> >  #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,
> > +};
> > +
> > +enum binning_bit_depths {
> > +	BINNING_IDX_8_BIT,
> > +	BINNING_IDX_10_BIT,
> > +	BINNING_IDX_MAX
> > +};
> > +
> >  /* Mode : resolution and related config&values */
> >  struct imx219_mode {
> >  	/* Frame width */
> > @@ -158,6 +170,9 @@ struct imx219_mode {
> >
> >  	/* V-timing */
> >  	unsigned int vts_def;
> > +
> > +	/* binning mode based on format code */
> > +	enum binning_mode binning[BINNING_IDX_MAX];
> >  };
> >
> >  static const struct cci_reg_sequence imx219_common_regs[] = {
> > @@ -293,24 +308,40 @@ static const struct imx219_mode supported_modes[] = {
> >  		.width = 3280,
> >  		.height = 2464,
> >  		.vts_def = 3526,
> > +		.binning = {
> > +			[BINNING_IDX_8_BIT] = BINNING_NONE,
> > +			[BINNING_IDX_10_BIT] = BINNING_NONE,
> > +		},
> >  	},
> >  	{
> >  		/* 1080P 30fps cropped */
> >  		.width = 1920,
> >  		.height = 1080,
> >  		.vts_def = 1763,
> > +		.binning = {
> > +			[BINNING_IDX_8_BIT] = BINNING_NONE,
> > +			[BINNING_IDX_10_BIT] = BINNING_NONE,
> > +		},
> >  	},
> >  	{
> >  		/* 2x2 binned 30fps mode */
> >  		.width = 1640,
> >  		.height = 1232,
> >  		.vts_def = 1763,
> > +		.binning = {
> > +			[BINNING_IDX_8_BIT] = BINNING_ANALOG_X2,
> > +			[BINNING_IDX_10_BIT] = BINNING_X2,
> > +		},
> >  	},
> >  	{
> >  		/* 640x480 30fps mode */
> >  		.width = 640,
> >  		.height = 480,
> >  		.vts_def = 1763,
> > +		.binning = {
> > +			[BINNING_IDX_8_BIT] = BINNING_ANALOG_X2,
> > +			[BINNING_IDX_10_BIT] = BINNING_ANALOG_X2,
> > +		},
> >  	},
> >  };
> >
> > @@ -337,6 +368,9 @@ struct imx219 {
> >
> >  	/* Two or Four lanes */
> >  	u8 lanes;
> > +
> > +	/* Binning mode */
> > +	enum binning_mode binning;
> >  };
> >
> >  static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> > @@ -362,6 +396,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->binning) {
> > +	case BINNING_NONE:
> > +	case BINNING_X2:
> > +		return 1;
> > +	case BINNING_ANALOG_X2:
> > +		return 2;
> > +	}
> > +	return -EINVAL;
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Controls
> >   */
> > @@ -373,10 +437,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 +471,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 +488,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 +529,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 +659,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 +675,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;
> 
> So if I got this right, another way of handling this would be to
> 
> 		bin_v = (bpp == 8 || (format->width == 640 && format->height = 480)
>                       ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> 
> 
> I'm not sure how much better this is, as then freely configure the
> sensor at (random numbers) 720x480@10bpp would not use analog binning
> while it might be beneficial. Actually knowing what analog mode is how
> it shuold be used would help, but if I recall correctly it wasn't
> clear from documentation or not fully clarified by Sony ?
> 
> When it comes to scaling PIXEL_RATE, the above switch could be moved
> to set_pad_format and store imx219->binning to be
> 1) used here
> 2) used in s_ctrl
> 
> like this patch does already.
> 
> Opinions ?
> 
> > -		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->binning, &ret);
> > +	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, imx219->binning, &ret);
> >
> >  	cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
> >  		  format->width, &ret);
> > @@ -851,6 +881,21 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >  		int exposure_max;
> >  		int exposure_def;
> >  		int hblank;
> > +		int pixel_rate;
> > +
> > +		/* Update binning mode based on format */
> > +		switch (imx219_get_format_bpp(format)) {
> > +		case 8:
> > +			imx219->binning = mode->binning[BINNING_IDX_8_BIT];
> > +			break;
> > +
> > +		case 10:
> > +			imx219->binning = mode->binning[BINNING_IDX_10_BIT];
> > +			break;
> > +
> > +		default:
> > +			imx219->binning = BINNING_NONE;
> > +		}
> >
> >  		/* Update limits and set FPS to default */
> >  		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > @@ -879,6 +924,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;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/3] media: i2c: imx219: Scale the pixel rate for analog binning

[Jai Luthra]

[-- Attachment #1: Type: text/plain, Size: 12576 bytes --]

Hi Dave,

& Laurent, Jacopo,

On Nov 24, 2024 at 05:34:32 +0200, Laurent Pinchart wrote:
> On Fri, Nov 22, 2024 at 11:31:58AM +0100, Jacopo Mondi wrote:
> > On Thu, Nov 21, 2024 at 05:38:04PM +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.
> > >
> > > To do this cleanly, re-introduce the book-keeping for which binning mode
> > > is used with which resolution/format.
> > 
> > I think the patch is correct, however this goes a bit in the opposite
> > direction of making the driver freely configurable. IOW the more we
> > store in modes, the harder it will become to freely configure the
> > sensor. I know there are different opinions on how much this is
> > actually an issue, with valid arguments on both sides, but as I recall
> > freely configurable was a goal of Laurent's series, let me cc him
> > explicitly.
> 
> Correct. I would like the binning configuration to be computed by the
> driver, not hardcoded. Let's do better than this.
> 

From commit ef86447e775fb1f2ced00d4c7fff2c0a1c63f165 we know the special 
"analog" 2x binning mode causes artefacts for RAW10 1640x1232 capture. 
But it works (and is preferred due to higher pixelrate) for RAW10 
640x480.

I did some experimentation with different resolutions, keeping format 
set to RAW10, and analog binning always enabled.

Any width > 1624 causes same artefacts. And any width <= 1624 gives a 
normal image. Height doesn't seem to matter.

Dave do you know somebody in Sony who can clarify why that is the case?

I am planning to send a v3 for this patch using width <= 1624 to decide 
which binning mode to pick.

> > > [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 | 138 ++++++++++++++++++++++++++++++---------------
> > >  1 file changed, 94 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > index 970e6362d0ae3a9078daf337155e83d637bc1ca1..ec795569361987ae30bff234e97fa34600bf5975 100644
> > > --- a/drivers/media/i2c/imx219.c
> > > +++ b/drivers/media/i2c/imx219.c
> > > @@ -149,6 +149,18 @@
> > >  #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,
> > > +};
> > > +
> > > +enum binning_bit_depths {
> > > +	BINNING_IDX_8_BIT,
> > > +	BINNING_IDX_10_BIT,
> > > +	BINNING_IDX_MAX
> > > +};
> > > +
> > >  /* Mode : resolution and related config&values */
> > >  struct imx219_mode {
> > >  	/* Frame width */
> > > @@ -158,6 +170,9 @@ struct imx219_mode {
> > >
> > >  	/* V-timing */
> > >  	unsigned int vts_def;
> > > +
> > > +	/* binning mode based on format code */
> > > +	enum binning_mode binning[BINNING_IDX_MAX];
> > >  };
> > >
> > >  static const struct cci_reg_sequence imx219_common_regs[] = {
> > > @@ -293,24 +308,40 @@ static const struct imx219_mode supported_modes[] = {
> > >  		.width = 3280,
> > >  		.height = 2464,
> > >  		.vts_def = 3526,
> > > +		.binning = {
> > > +			[BINNING_IDX_8_BIT] = BINNING_NONE,
> > > +			[BINNING_IDX_10_BIT] = BINNING_NONE,
> > > +		},
> > >  	},
> > >  	{
> > >  		/* 1080P 30fps cropped */
> > >  		.width = 1920,
> > >  		.height = 1080,
> > >  		.vts_def = 1763,
> > > +		.binning = {
> > > +			[BINNING_IDX_8_BIT] = BINNING_NONE,
> > > +			[BINNING_IDX_10_BIT] = BINNING_NONE,
> > > +		},
> > >  	},
> > >  	{
> > >  		/* 2x2 binned 30fps mode */
> > >  		.width = 1640,
> > >  		.height = 1232,
> > >  		.vts_def = 1763,
> > > +		.binning = {
> > > +			[BINNING_IDX_8_BIT] = BINNING_ANALOG_X2,
> > > +			[BINNING_IDX_10_BIT] = BINNING_X2,
> > > +		},
> > >  	},
> > >  	{
> > >  		/* 640x480 30fps mode */
> > >  		.width = 640,
> > >  		.height = 480,
> > >  		.vts_def = 1763,
> > > +		.binning = {
> > > +			[BINNING_IDX_8_BIT] = BINNING_ANALOG_X2,
> > > +			[BINNING_IDX_10_BIT] = BINNING_ANALOG_X2,
> > > +		},
> > >  	},
> > >  };
> > >
> > > @@ -337,6 +368,9 @@ struct imx219 {
> > >
> > >  	/* Two or Four lanes */
> > >  	u8 lanes;
> > > +
> > > +	/* Binning mode */
> > > +	enum binning_mode binning;
> > >  };
> > >
> > >  static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> > > @@ -362,6 +396,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->binning) {
> > > +	case BINNING_NONE:
> > > +	case BINNING_X2:
> > > +		return 1;
> > > +	case BINNING_ANALOG_X2:
> > > +		return 2;
> > > +	}
> > > +	return -EINVAL;
> > > +}
> > > +
> > >  /* -----------------------------------------------------------------------------
> > >   * Controls
> > >   */
> > > @@ -373,10 +437,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 +471,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 +488,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 +529,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 +659,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 +675,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;
> > 
> > So if I got this right, another way of handling this would be to
> > 
> > 		bin_v = (bpp == 8 || (format->width == 640 && format->height = 480)
> >                       ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> > 
> > 
> > I'm not sure how much better this is, as then freely configure the
> > sensor at (random numbers) 720x480@10bpp would not use analog binning
> > while it might be beneficial. Actually knowing what analog mode is how
> > it shuold be used would help, but if I recall correctly it wasn't
> > clear from documentation or not fully clarified by Sony ?
> > 
> > When it comes to scaling PIXEL_RATE, the above switch could be moved
> > to set_pad_format and store imx219->binning to be
> > 1) used here
> > 2) used in s_ctrl

Agreed, will move it to set_pad_format.

> > 
> > like this patch does already.
> > 
> > Opinions ?
> > 
> > > -		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->binning, &ret);
> > > +	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, imx219->binning, &ret);
> > >
> > >  	cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
> > >  		  format->width, &ret);
> > > @@ -851,6 +881,21 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > >  		int exposure_max;
> > >  		int exposure_def;
> > >  		int hblank;
> > > +		int pixel_rate;
> > > +
> > > +		/* Update binning mode based on format */
> > > +		switch (imx219_get_format_bpp(format)) {
> > > +		case 8:
> > > +			imx219->binning = mode->binning[BINNING_IDX_8_BIT];
> > > +			break;
> > > +
> > > +		case 10:
> > > +			imx219->binning = mode->binning[BINNING_IDX_10_BIT];
> > > +			break;
> > > +
> > > +		default:
> > > +			imx219->binning = BINNING_NONE;
> > > +		}
> > >
> > >  		/* Update limits and set FPS to default */
> > >  		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > > @@ -879,6 +924,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;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Thanks,
Jai

GPG Fingerprint: 4DE0 D818 E5D5 75E8 D45A AFC5 43DE 91F9 249A 7145

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/3] media: i2c: imx219: Scale the pixel rate for analog binning

[Dave Stevenson]

Hi Jai and Laurent

On Mon, 25 Nov 2024 at 12:34, Jai Luthra <jai.luthra@ideasonboard.com> wrote:
>
> Hi Dave,
>
> & Laurent, Jacopo,
>
> On Nov 24, 2024 at 05:34:32 +0200, Laurent Pinchart wrote:
> > On Fri, Nov 22, 2024 at 11:31:58AM +0100, Jacopo Mondi wrote:
> > > On Thu, Nov 21, 2024 at 05:38:04PM +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.
> > > >
> > > > To do this cleanly, re-introduce the book-keeping for which binning mode
> > > > is used with which resolution/format.
> > >
> > > I think the patch is correct, however this goes a bit in the opposite
> > > direction of making the driver freely configurable. IOW the more we
> > > store in modes, the harder it will become to freely configure the
> > > sensor. I know there are different opinions on how much this is
> > > actually an issue, with valid arguments on both sides, but as I recall
> > > freely configurable was a goal of Laurent's series, let me cc him
> > > explicitly.
> >
> > Correct. I would like the binning configuration to be computed by the
> > driver, not hardcoded. Let's do better than this.

So the question over storing in modes versus free-form
cropping/binning was raised at Dublin in 2022, and seems to largely
have gone nowhere since.

In this series Sakari queried IQ issues with adjusting HBLANK on this
sensor. The same query is valid for cropping/binning changes, and
probably more so.

How can you square the circle of IQ issues due to settings that
haven't been validated by the sensor manufacturer vs applying
arbitrary cropping / binning setups?

> From commit ef86447e775fb1f2ced00d4c7fff2c0a1c63f165 we know the special
> "analog" 2x binning mode causes artefacts for RAW10 1640x1232 capture.
> But it works (and is preferred due to higher pixelrate) for RAW10
> 640x480.
>
> I did some experimentation with different resolutions, keeping format
> set to RAW10, and analog binning always enabled.
>
> Any width > 1624 causes same artefacts. And any width <= 1624 gives a
> normal image. Height doesn't seem to matter.
>
> Dave do you know somebody in Sony who can clarify why that is the case?

I can ping my contact at Sony.
Whether they still have the information is a different question.
Previous times it has seemed they need to find an individual who was
on the original product team to answer, so seeing as IMX219 came out
around 2013 that could be tricky.

  Dave

> I am planning to send a v3 for this patch using width <= 1624 to decide
> which binning mode to pick.
>
> > > > [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 | 138 ++++++++++++++++++++++++++++++---------------
> > > >  1 file changed, 94 insertions(+), 44 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > index 970e6362d0ae3a9078daf337155e83d637bc1ca1..ec795569361987ae30bff234e97fa34600bf5975 100644
> > > > --- a/drivers/media/i2c/imx219.c
> > > > +++ b/drivers/media/i2c/imx219.c
> > > > @@ -149,6 +149,18 @@
> > > >  #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,
> > > > +};
> > > > +
> > > > +enum binning_bit_depths {
> > > > + BINNING_IDX_8_BIT,
> > > > + BINNING_IDX_10_BIT,
> > > > + BINNING_IDX_MAX
> > > > +};
> > > > +
> > > >  /* Mode : resolution and related config&values */
> > > >  struct imx219_mode {
> > > >   /* Frame width */
> > > > @@ -158,6 +170,9 @@ struct imx219_mode {
> > > >
> > > >   /* V-timing */
> > > >   unsigned int vts_def;
> > > > +
> > > > + /* binning mode based on format code */
> > > > + enum binning_mode binning[BINNING_IDX_MAX];
> > > >  };
> > > >
> > > >  static const struct cci_reg_sequence imx219_common_regs[] = {
> > > > @@ -293,24 +308,40 @@ static const struct imx219_mode supported_modes[] = {
> > > >           .width = 3280,
> > > >           .height = 2464,
> > > >           .vts_def = 3526,
> > > > +         .binning = {
> > > > +                 [BINNING_IDX_8_BIT] = BINNING_NONE,
> > > > +                 [BINNING_IDX_10_BIT] = BINNING_NONE,
> > > > +         },
> > > >   },
> > > >   {
> > > >           /* 1080P 30fps cropped */
> > > >           .width = 1920,
> > > >           .height = 1080,
> > > >           .vts_def = 1763,
> > > > +         .binning = {
> > > > +                 [BINNING_IDX_8_BIT] = BINNING_NONE,
> > > > +                 [BINNING_IDX_10_BIT] = BINNING_NONE,
> > > > +         },
> > > >   },
> > > >   {
> > > >           /* 2x2 binned 30fps mode */
> > > >           .width = 1640,
> > > >           .height = 1232,
> > > >           .vts_def = 1763,
> > > > +         .binning = {
> > > > +                 [BINNING_IDX_8_BIT] = BINNING_ANALOG_X2,
> > > > +                 [BINNING_IDX_10_BIT] = BINNING_X2,
> > > > +         },
> > > >   },
> > > >   {
> > > >           /* 640x480 30fps mode */
> > > >           .width = 640,
> > > >           .height = 480,
> > > >           .vts_def = 1763,
> > > > +         .binning = {
> > > > +                 [BINNING_IDX_8_BIT] = BINNING_ANALOG_X2,
> > > > +                 [BINNING_IDX_10_BIT] = BINNING_ANALOG_X2,
> > > > +         },
> > > >   },
> > > >  };
> > > >
> > > > @@ -337,6 +368,9 @@ struct imx219 {
> > > >
> > > >   /* Two or Four lanes */
> > > >   u8 lanes;
> > > > +
> > > > + /* Binning mode */
> > > > + enum binning_mode binning;
> > > >  };
> > > >
> > > >  static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> > > > @@ -362,6 +396,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->binning) {
> > > > + case BINNING_NONE:
> > > > + case BINNING_X2:
> > > > +         return 1;
> > > > + case BINNING_ANALOG_X2:
> > > > +         return 2;
> > > > + }
> > > > + return -EINVAL;
> > > > +}
> > > > +
> > > >  /* -----------------------------------------------------------------------------
> > > >   * Controls
> > > >   */
> > > > @@ -373,10 +437,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 +471,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 +488,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 +529,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 +659,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 +675,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;
> > >
> > > So if I got this right, another way of handling this would be to
> > >
> > >             bin_v = (bpp == 8 || (format->width == 640 && format->height = 480)
> > >                       ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> > >
> > >
> > > I'm not sure how much better this is, as then freely configure the
> > > sensor at (random numbers) 720x480@10bpp would not use analog binning
> > > while it might be beneficial. Actually knowing what analog mode is how
> > > it shuold be used would help, but if I recall correctly it wasn't
> > > clear from documentation or not fully clarified by Sony ?
> > >
> > > When it comes to scaling PIXEL_RATE, the above switch could be moved
> > > to set_pad_format and store imx219->binning to be
> > > 1) used here
> > > 2) used in s_ctrl
>
> Agreed, will move it to set_pad_format.
>
> > >
> > > like this patch does already.
> > >
> > > Opinions ?
> > >
> > > > -         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->binning, &ret);
> > > > + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, imx219->binning, &ret);
> > > >
> > > >   cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
> > > >             format->width, &ret);
> > > > @@ -851,6 +881,21 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > > >           int exposure_max;
> > > >           int exposure_def;
> > > >           int hblank;
> > > > +         int pixel_rate;
> > > > +
> > > > +         /* Update binning mode based on format */
> > > > +         switch (imx219_get_format_bpp(format)) {
> > > > +         case 8:
> > > > +                 imx219->binning = mode->binning[BINNING_IDX_8_BIT];
> > > > +                 break;
> > > > +
> > > > +         case 10:
> > > > +                 imx219->binning = mode->binning[BINNING_IDX_10_BIT];
> > > > +                 break;
> > > > +
> > > > +         default:
> > > > +                 imx219->binning = BINNING_NONE;
> > > > +         }
> > > >
> > > >           /* Update limits and set FPS to default */
> > > >           __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > > > @@ -879,6 +924,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;
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
> --
> Thanks,
> Jai
>
> GPG Fingerprint: 4DE0 D818 E5D5 75E8 D45A AFC5 43DE 91F9 249A 7145

Re: [PATCH v2 3/3] media: i2c: imx219: Scale the pixel rate for analog binning

[Laurent Pinchart]

On Mon, Nov 25, 2024 at 04:14:26PM +0000, Dave Stevenson wrote:
> On Mon, 25 Nov 2024 at 12:34, Jai Luthra wrote:
> > On Nov 24, 2024 at 05:34:32 +0200, Laurent Pinchart wrote:
> > > On Fri, Nov 22, 2024 at 11:31:58AM +0100, Jacopo Mondi wrote:
> > > > On Thu, Nov 21, 2024 at 05:38:04PM +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.
> > > > >
> > > > > To do this cleanly, re-introduce the book-keeping for which binning mode
> > > > > is used with which resolution/format.
> > > >
> > > > I think the patch is correct, however this goes a bit in the opposite
> > > > direction of making the driver freely configurable. IOW the more we
> > > > store in modes, the harder it will become to freely configure the
> > > > sensor. I know there are different opinions on how much this is
> > > > actually an issue, with valid arguments on both sides, but as I recall
> > > > freely configurable was a goal of Laurent's series, let me cc him
> > > > explicitly.
> > >
> > > Correct. I would like the binning configuration to be computed by the
> > > driver, not hardcoded. Let's do better than this.
> 
> So the question over storing in modes versus free-form
> cropping/binning was raised at Dublin in 2022, and seems to largely
> have gone nowhere since.
> 
> In this series Sakari queried IQ issues with adjusting HBLANK on this
> sensor. The same query is valid for cropping/binning changes, and
> probably more so.
> 
> How can you square the circle of IQ issues due to settings that
> haven't been validated by the sensor manufacturer vs applying
> arbitrary cropping / binning setups?

Sakari and I have brainstormed options. The one I like best so far is to
have an API for freely configurable drivers to enumerate the "modes"
that have been tested.

> > From commit ef86447e775fb1f2ced00d4c7fff2c0a1c63f165 we know the special
> > "analog" 2x binning mode causes artefacts for RAW10 1640x1232 capture.
> > But it works (and is preferred due to higher pixelrate) for RAW10
> > 640x480.
> >
> > I did some experimentation with different resolutions, keeping format
> > set to RAW10, and analog binning always enabled.
> >
> > Any width > 1624 causes same artefacts. And any width <= 1624 gives a
> > normal image. Height doesn't seem to matter.
> >
> > Dave do you know somebody in Sony who can clarify why that is the case?
> 
> I can ping my contact at Sony.
> Whether they still have the information is a different question.
> Previous times it has seemed they need to find an individual who was
> on the original product team to answer, so seeing as IMX219 came out
> around 2013 that could be tricky.
> 
> > I am planning to send a v3 for this patch using width <= 1624 to decide
> > which binning mode to pick.
> >
> > > > > [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 | 138 ++++++++++++++++++++++++++++++---------------
> > > > >  1 file changed, 94 insertions(+), 44 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > > index 970e6362d0ae3a9078daf337155e83d637bc1ca1..ec795569361987ae30bff234e97fa34600bf5975 100644
> > > > > --- a/drivers/media/i2c/imx219.c
> > > > > +++ b/drivers/media/i2c/imx219.c
> > > > > @@ -149,6 +149,18 @@
> > > > >  #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,
> > > > > +};
> > > > > +
> > > > > +enum binning_bit_depths {
> > > > > + BINNING_IDX_8_BIT,
> > > > > + BINNING_IDX_10_BIT,
> > > > > + BINNING_IDX_MAX
> > > > > +};
> > > > > +
> > > > >  /* Mode : resolution and related config&values */
> > > > >  struct imx219_mode {
> > > > >   /* Frame width */
> > > > > @@ -158,6 +170,9 @@ struct imx219_mode {
> > > > >
> > > > >   /* V-timing */
> > > > >   unsigned int vts_def;
> > > > > +
> > > > > + /* binning mode based on format code */
> > > > > + enum binning_mode binning[BINNING_IDX_MAX];
> > > > >  };
> > > > >
> > > > >  static const struct cci_reg_sequence imx219_common_regs[] = {
> > > > > @@ -293,24 +308,40 @@ static const struct imx219_mode supported_modes[] = {
> > > > >           .width = 3280,
> > > > >           .height = 2464,
> > > > >           .vts_def = 3526,
> > > > > +         .binning = {
> > > > > +                 [BINNING_IDX_8_BIT] = BINNING_NONE,
> > > > > +                 [BINNING_IDX_10_BIT] = BINNING_NONE,
> > > > > +         },
> > > > >   },
> > > > >   {
> > > > >           /* 1080P 30fps cropped */
> > > > >           .width = 1920,
> > > > >           .height = 1080,
> > > > >           .vts_def = 1763,
> > > > > +         .binning = {
> > > > > +                 [BINNING_IDX_8_BIT] = BINNING_NONE,
> > > > > +                 [BINNING_IDX_10_BIT] = BINNING_NONE,
> > > > > +         },
> > > > >   },
> > > > >   {
> > > > >           /* 2x2 binned 30fps mode */
> > > > >           .width = 1640,
> > > > >           .height = 1232,
> > > > >           .vts_def = 1763,
> > > > > +         .binning = {
> > > > > +                 [BINNING_IDX_8_BIT] = BINNING_ANALOG_X2,
> > > > > +                 [BINNING_IDX_10_BIT] = BINNING_X2,
> > > > > +         },
> > > > >   },
> > > > >   {
> > > > >           /* 640x480 30fps mode */
> > > > >           .width = 640,
> > > > >           .height = 480,
> > > > >           .vts_def = 1763,
> > > > > +         .binning = {
> > > > > +                 [BINNING_IDX_8_BIT] = BINNING_ANALOG_X2,
> > > > > +                 [BINNING_IDX_10_BIT] = BINNING_ANALOG_X2,
> > > > > +         },
> > > > >   },
> > > > >  };
> > > > >
> > > > > @@ -337,6 +368,9 @@ struct imx219 {
> > > > >
> > > > >   /* Two or Four lanes */
> > > > >   u8 lanes;
> > > > > +
> > > > > + /* Binning mode */
> > > > > + enum binning_mode binning;
> > > > >  };
> > > > >
> > > > >  static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> > > > > @@ -362,6 +396,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->binning) {
> > > > > + case BINNING_NONE:
> > > > > + case BINNING_X2:
> > > > > +         return 1;
> > > > > + case BINNING_ANALOG_X2:
> > > > > +         return 2;
> > > > > + }
> > > > > + return -EINVAL;
> > > > > +}
> > > > > +
> > > > >  /* -----------------------------------------------------------------------------
> > > > >   * Controls
> > > > >   */
> > > > > @@ -373,10 +437,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 +471,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 +488,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 +529,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 +659,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 +675,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;
> > > >
> > > > So if I got this right, another way of handling this would be to
> > > >
> > > >             bin_v = (bpp == 8 || (format->width == 640 && format->height = 480)
> > > >                       ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> > > >
> > > >
> > > > I'm not sure how much better this is, as then freely configure the
> > > > sensor at (random numbers) 720x480@10bpp would not use analog binning
> > > > while it might be beneficial. Actually knowing what analog mode is how
> > > > it shuold be used would help, but if I recall correctly it wasn't
> > > > clear from documentation or not fully clarified by Sony ?
> > > >
> > > > When it comes to scaling PIXEL_RATE, the above switch could be moved
> > > > to set_pad_format and store imx219->binning to be
> > > > 1) used here
> > > > 2) used in s_ctrl
> >
> > Agreed, will move it to set_pad_format.
> >
> > > >
> > > > like this patch does already.
> > > >
> > > > Opinions ?
> > > >
> > > > > -         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->binning, &ret);
> > > > > + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, imx219->binning, &ret);
> > > > >
> > > > >   cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
> > > > >             format->width, &ret);
> > > > > @@ -851,6 +881,21 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > > > >           int exposure_max;
> > > > >           int exposure_def;
> > > > >           int hblank;
> > > > > +         int pixel_rate;
> > > > > +
> > > > > +         /* Update binning mode based on format */
> > > > > +         switch (imx219_get_format_bpp(format)) {
> > > > > +         case 8:
> > > > > +                 imx219->binning = mode->binning[BINNING_IDX_8_BIT];
> > > > > +                 break;
> > > > > +
> > > > > +         case 10:
> > > > > +                 imx219->binning = mode->binning[BINNING_IDX_10_BIT];
> > > > > +                 break;
> > > > > +
> > > > > +         default:
> > > > > +                 imx219->binning = BINNING_NONE;
> > > > > +         }
> > > > >
> > > > >           /* Update limits and set FPS to default */
> > > > >           __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > > > > @@ -879,6 +924,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;
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
> >
> > --
> > Thanks,
> > Jai
> >
> > GPG Fingerprint: 4DE0 D818 E5D5 75E8 D45A AFC5 43DE 91F9 249A 7145

-- 
Regards,

Laurent Pinchart