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

[PATCH 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>
---
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 | 186 +++++++++++++++++++++++++++++++--------------
 1 file changed, 130 insertions(+), 56 deletions(-)
---
base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e
change-id: 20241029-imx219_fixes-bfaac4a56200

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

[PATCH 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>
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 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 | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index f98aad74fe584a18e2fe7126f92bf294762a54e3..de9230d4ad81f085640be254db9391ae7ad20773 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			3448
+#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,11 @@ 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,
+					   V4L2_CID_HBLANK, hblank,
+					   IMX219_PPL_MIN - mode->width,
 					   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;
@@ -842,6 +847,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+		u32 prev_hts = format->width + imx219->hblank->val;
 		int exposure_max;
 		int exposure_def;
 		int hblank;
@@ -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

[PATCH 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 | 149 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 106 insertions(+), 43 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index de9230d4ad81f085640be254db9391ae7ad20773..140d958f80eb57dfb4ecf1796fcdf77081a662d7 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,
+	BINNING_X2,
+	BINNING_ANALOG_X2,
+};
+
+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 */
@@ -473,7 +540,7 @@ static int imx219_init_controls(struct imx219 *imx219)
 	const struct imx219_mode *mode = &supported_modes[0];
 	struct v4l2_ctrl_handler *ctrl_hdlr;
 	struct v4l2_fwnode_device_properties props;
-	int exposure_max, exposure_def, hblank;
+	int exposure_max, exposure_def, hblank, pixel_rate;
 	int i, ret;
 
 	ctrl_hdlr = &imx219->ctrl_handler;
@@ -482,11 +549,11 @@ static int imx219_init_controls(struct imx219 *imx219)
 		return ret;
 
 	/* By default, PIXEL_RATE is read only */
+	pixel_rate = imx219_get_pixel_rate(imx219);
 	imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
 					       V4L2_CID_PIXEL_RATE,
-					       imx219_get_pixel_rate(imx219),
-					       imx219_get_pixel_rate(imx219), 1,
-					       imx219_get_pixel_rate(imx219));
+					       pixel_rate, pixel_rate, 1,
+					       pixel_rate);
 
 	imx219->link_freq =
 		v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx219_ctrl_ops,
@@ -593,29 +660,13 @@ 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;
+	u64 binning;
+	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);
@@ -626,28 +677,20 @@ 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;
+	switch (imx219->binning) {
+	case BINNING_NONE:
+		binning = 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;
+	case BINNING_X2:
+		binning = IMX219_BINNING_X2;
 		break;
-	case 2:
-		bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
+	case BINNING_ANALOG_X2:
+		binning = IMX219_BINNING_X2_ANALOG;
 		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, binning, &ret);
+	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, binning, &ret);
 
 	cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
 		  format->width, &ret);
@@ -851,6 +894,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 +937,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 2/3] media: i2c: imx219: make HBLANK r/w to allow longer exposures

[Sakari Ailus]

Hi Jai,

On Tue, Oct 29, 2024 at 02:27:36PM +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.

It looks like this changes horizontal blanking at least in some cases. Does
this also work as expected in binned modes, for instance?

Many sensors have image quality related issues on untested albeit
functional line length values.

So my question is: how has this been validated?

> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/i2c/imx219.c | 35 +++++++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index f98aad74fe584a18e2fe7126f92bf294762a54e3..de9230d4ad81f085640be254db9391ae7ad20773 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			3448
> +#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,11 @@ 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,
> +					   V4L2_CID_HBLANK, hblank,
> +					   IMX219_PPL_MIN - mode->width,
>  					   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;
> @@ -842,6 +847,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  	crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
>  
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> +		u32 prev_hts = format->width + imx219->hblank->val;
>  		int exposure_max;
>  		int exposure_def;
>  		int hblank;
> @@ -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;
> 

-- 
Kind regards,

Sakari Ailus

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

[Jacopo Mondi]

Hi Jai

On Tue, Oct 29, 2024 at 02:27:35PM +0530, Jai Luthra wrote:
> 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>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>

Confirmed by the documentation of register 0x114a/0x114b

Reviewed-by: Jacopo Mondi <jacopo.mondi@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
>
>

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

[Jacopo Mondi]

Hi Jai, Dave

On Tue, Oct 29, 2024 at 02:27:36PM +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 | 35 +++++++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index f98aad74fe584a18e2fe7126f92bf294762a54e3..de9230d4ad81f085640be254db9391ae7ad20773 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			3448
> +#define IMX219_PPL_MAX			0x7ff0

nit: I wold have rather made these two either both hex or both
decimal (my preference is for hex as it matches the registers
0x1144-0x1147 registers)

Also, yes:
min_line_length_pck = 0x0d78 = 3448

but:
min_line_blanking_pck = 0xa8 = 168

But as the max supported output width is 3280 and (3448 - 168 = 3280) I
think it's fine listing PLL_MIN only

> +#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);

According to Sakari's comment, should you in the next patch scale
hblank by the rate factor has done for vts and pixel rate ?

> +		break;
>  	case V4L2_CID_TEST_PATTERN_RED:
>  		cci_write(imx219->regmap, IMX219_REG_TESTP_RED,
>  			  ctrl->val, &ret);
> @@ -496,12 +502,11 @@ 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,
> +					   V4L2_CID_HBLANK, hblank,
> +					   IMX219_PPL_MIN - mode->width,

Can't you use 'hblank' again here ?

>  					   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;
> @@ -842,6 +847,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  	crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
>
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> +		u32 prev_hts = format->width + imx219->hblank->val;
>  		int exposure_max;
>  		int exposure_def;
>  		int hblank;
> @@ -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.

Two years ago I wrote this
https://patchwork.linuxtv.org/project/linux-media/patch/20221121181515.34008-2-jacopo@jmondi.org/

which hasn't progressed since then but I presume was based on some
sort of consensus.

Is it worth a respin ?

>  		 */
> -		hblank = IMX219_PPL_DEFAULT - mode->width;
> -		__v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1,
> -					 hblank);
> +		hblank = prev_hts - mode->width;

And as far as I can tell mode->width == format->width because of the
above

	imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code);
	format = v4l2_subdev_state_get_format(state, 0);
	*format = fmt->format;

so here you have

		u32 prev_hts = format->width + imx219->hblank->val;
                hblank = (format->widht + imx219->hblank->val)
                       - format->width;

so that:
                hblank == imx219->hblank->val;


> +		__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);

So here you're writing hblank to the same value (clamped by the
framework in the new limits). So you're not retaining line lenght but
the blanking value, which seems to contradict the comment. Or am I
missing something here ?

Thanks
  j

>  	}
>
>  	return 0;
>
> --
> 2.47.0
>
>

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

[Jacopo Mondi]

Hello

On Tue, Oct 29, 2024 at 02:27:37PM +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.
>
> [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 | 149 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 106 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index de9230d4ad81f085640be254db9391ae7ad20773..140d958f80eb57dfb4ecf1796fcdf77081a662d7 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,
> +	BINNING_X2,
> +	BINNING_ANALOG_X2,

You could assign to the enumerated values the corresponding values of
IMX219_BINNING_... so that in imx219_set_framefmt() you could write
imx219->binning directly

> +};
> +
> +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);

As noticed by Sakari, should hblank be similarly scaled ?

>  		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 */
> @@ -473,7 +540,7 @@ static int imx219_init_controls(struct imx219 *imx219)
>  	const struct imx219_mode *mode = &supported_modes[0];
>  	struct v4l2_ctrl_handler *ctrl_hdlr;
>  	struct v4l2_fwnode_device_properties props;
> -	int exposure_max, exposure_def, hblank;
> +	int exposure_max, exposure_def, hblank, pixel_rate;
>  	int i, ret;
>
>  	ctrl_hdlr = &imx219->ctrl_handler;
> @@ -482,11 +549,11 @@ static int imx219_init_controls(struct imx219 *imx219)
>  		return ret;
>
>  	/* By default, PIXEL_RATE is read only */
> +	pixel_rate = imx219_get_pixel_rate(imx219);
>  	imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
>  					       V4L2_CID_PIXEL_RATE,
> -					       imx219_get_pixel_rate(imx219),
> -					       imx219_get_pixel_rate(imx219), 1,
> -					       imx219_get_pixel_rate(imx219));
> +					       pixel_rate, pixel_rate, 1,
> +					       pixel_rate);

Is this change needed ?

>
>  	imx219->link_freq =
>  		v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx219_ctrl_ops,
> @@ -593,29 +660,13 @@ 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;
> +	u64 binning;

why u64 for a value that gets written to a 16 bits register ?

> +	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);
> @@ -626,28 +677,20 @@ 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;
> +	switch (imx219->binning) {
> +	case BINNING_NONE:
> +		binning = 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;
> +	case BINNING_X2:
> +		binning = IMX219_BINNING_X2;
>  		break;
> -	case 2:
> -		bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> +	case BINNING_ANALOG_X2:
> +		binning = IMX219_BINNING_X2_ANALOG;
>  		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, binning, &ret);
> +	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, binning, &ret);
>
>  	cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
>  		  format->width, &ret);
> @@ -851,6 +894,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 +937,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 1/3] media: i2c: imx219: Correct the minimum vblanking value

[Dave Stevenson]

On Tue, 29 Oct 2024 at 08:58, Jai Luthra <jai.luthra@ideasonboard.com> wrote:
>
> 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>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.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
>

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

[Dave Stevenson]

Hi Sakari

On Fri, 1 Nov 2024 at 08:48, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Hi Jai,
>
> On Tue, Oct 29, 2024 at 02:27:36PM +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.
>
> It looks like this changes horizontal blanking at least in some cases. Does
> this also work as expected in binned modes, for instance?
>
> Many sensors have image quality related issues on untested albeit
> functional line length values.
>
> So my question is: how has this been validated?

Validated by Sony, or others?
I've tested a range of values in all modes and not observed any image
quality issues.

From previous discussions with Sony, they always provide their big
spreadsheet of register values for the specific mode and frame rate
requested. I don't think they even officially state that changing
VTS/FRM_LENGTH_LINES to change the framerate is permitted.
There are some Sony datasheets (eg imx258) that state "set to X. Any
other value please confirm with Sony", but that isn't the case for the
imx219 datasheet. I take that as it is permitted within the defined
ranges.

  Dave

> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > ---
> >  drivers/media/i2c/imx219.c | 35 +++++++++++++++++++++++------------
> >  1 file changed, 23 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index f98aad74fe584a18e2fe7126f92bf294762a54e3..de9230d4ad81f085640be254db9391ae7ad20773 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                       3448
> > +#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,11 @@ 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,
> > +                                        V4L2_CID_HBLANK, hblank,
> > +                                        IMX219_PPL_MIN - mode->width,
> >                                          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;
> > @@ -842,6 +847,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >       crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
> >
> >       if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > +             u32 prev_hts = format->width + imx219->hblank->val;
> >               int exposure_max;
> >               int exposure_def;
> >               int hblank;
> > @@ -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;
> >
>
> --
> Kind regards,
>
> Sakari Ailus

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

[Sakari Ailus]

Hi Dave,

On Thu, Nov 07, 2024 at 12:43:52PM +0000, Dave Stevenson wrote:
> Hi Sakari
> 
> On Fri, 1 Nov 2024 at 08:48, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Jai,
> >
> > On Tue, Oct 29, 2024 at 02:27:36PM +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.
> >
> > It looks like this changes horizontal blanking at least in some cases. Does
> > this also work as expected in binned modes, for instance?
> >
> > Many sensors have image quality related issues on untested albeit
> > functional line length values.
> >
> > So my question is: how has this been validated?
> 
> Validated by Sony, or others?
> I've tested a range of values in all modes and not observed any image
> quality issues.

Somehow at least. :-)

> 
> From previous discussions with Sony, they always provide their big
> spreadsheet of register values for the specific mode and frame rate
> requested. I don't think they even officially state that changing
> VTS/FRM_LENGTH_LINES to change the framerate is permitted.
> There are some Sony datasheets (eg imx258) that state "set to X. Any
> other value please confirm with Sony", but that isn't the case for the
> imx219 datasheet. I take that as it is permitted within the defined
> ranges.

I'm not that much concerned of vertical blanking, changing that within the
valid range has effects on the image itself very seldom. Horizontal
blanking is different though and this is what the patch makes changeable,
including a change in the default value. Of course there are big
differences between sensors here.

-- 
Kind regards,

Sakari Ailus

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

[Dave Stevenson]

Hi Sakari

On Fri, 8 Nov 2024 at 10:30, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Hi Dave,
>
> On Thu, Nov 07, 2024 at 12:43:52PM +0000, Dave Stevenson wrote:
> > Hi Sakari
> >
> > On Fri, 1 Nov 2024 at 08:48, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Jai,
> > >
> > > On Tue, Oct 29, 2024 at 02:27:36PM +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.
> > >
> > > It looks like this changes horizontal blanking at least in some cases. Does
> > > this also work as expected in binned modes, for instance?
> > >
> > > Many sensors have image quality related issues on untested albeit
> > > functional line length values.
> > >
> > > So my question is: how has this been validated?
> >
> > Validated by Sony, or others?
> > I've tested a range of values in all modes and not observed any image
> > quality issues.
>
> Somehow at least. :-)
>
> >
> > From previous discussions with Sony, they always provide their big
> > spreadsheet of register values for the specific mode and frame rate
> > requested. I don't think they even officially state that changing
> > VTS/FRM_LENGTH_LINES to change the framerate is permitted.
> > There are some Sony datasheets (eg imx258) that state "set to X. Any
> > other value please confirm with Sony", but that isn't the case for the
> > imx219 datasheet. I take that as it is permitted within the defined
> > ranges.
>
> I'm not that much concerned of vertical blanking, changing that within the
> valid range has effects on the image itself very seldom. Horizontal
> blanking is different though and this is what the patch makes changeable,
> including a change in the default value. Of course there are big
> differences between sensors here.

The intention was that the default value shouldn't change, and as the
overall PIXELS_PER_LINE value was meant to be retained on a mode
change the value used should only change if an application changes
V4L2_CID_HBLANK. If I blundered in the implementation of that, then
that should be fixed (I know Jacopo made comments, but I haven't had a
chance to investigate).

I doubt we'd get validation from Sony beyond the contents of the
datasheet. Potentially as the sensor is so old they don't have the
information or engineers involved.
I'm happy to set up a test system and capture a set of images with
HBLANK from min to max at some increment. With the same exposure and
gain they should all be identical as long as there isn't any movement
(rolling shutter with longer readout times and all that). Would that
be satisfactory?

For contrast, the IMX290 datasheet states that VMAX shall be fixed at
0x465 for all-pixel mode / 0x2ee for 720p mode, and HMAX should be
changed for frame rate control. As you say, sensors differ.

 Dave

> --
> Kind regards,
>
> Sakari Ailus

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

[Sakari Ailus]

Hi Dave,

On Mon, Nov 11, 2024 at 07:37:56PM +0000, Dave Stevenson wrote:
> Hi Sakari
> 
> On Fri, 8 Nov 2024 at 10:30, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Dave,
> >
> > On Thu, Nov 07, 2024 at 12:43:52PM +0000, Dave Stevenson wrote:
> > > Hi Sakari
> > >
> > > On Fri, 1 Nov 2024 at 08:48, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Jai,
> > > >
> > > > On Tue, Oct 29, 2024 at 02:27:36PM +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.
> > > >
> > > > It looks like this changes horizontal blanking at least in some cases. Does
> > > > this also work as expected in binned modes, for instance?
> > > >
> > > > Many sensors have image quality related issues on untested albeit
> > > > functional line length values.
> > > >
> > > > So my question is: how has this been validated?
> > >
> > > Validated by Sony, or others?
> > > I've tested a range of values in all modes and not observed any image
> > > quality issues.
> >
> > Somehow at least. :-)
> >
> > >
> > > From previous discussions with Sony, they always provide their big
> > > spreadsheet of register values for the specific mode and frame rate
> > > requested. I don't think they even officially state that changing
> > > VTS/FRM_LENGTH_LINES to change the framerate is permitted.
> > > There are some Sony datasheets (eg imx258) that state "set to X. Any
> > > other value please confirm with Sony", but that isn't the case for the
> > > imx219 datasheet. I take that as it is permitted within the defined
> > > ranges.
> >
> > I'm not that much concerned of vertical blanking, changing that within the
> > valid range has effects on the image itself very seldom. Horizontal
> > blanking is different though and this is what the patch makes changeable,
> > including a change in the default value. Of course there are big
> > differences between sensors here.
> 
> The intention was that the default value shouldn't change, and as the
> overall PIXELS_PER_LINE value was meant to be retained on a mode
> change the value used should only change if an application changes
> V4L2_CID_HBLANK. If I blundered in the implementation of that, then
> that should be fixed (I know Jacopo made comments, but I haven't had a
> chance to investigate).

I guess I misread the patch. It indeed should be the same.

> 
> I doubt we'd get validation from Sony beyond the contents of the
> datasheet. Potentially as the sensor is so old they don't have the
> information or engineers involved.
> I'm happy to set up a test system and capture a set of images with
> HBLANK from min to max at some increment. With the same exposure and
> gain they should all be identical as long as there isn't any movement
> (rolling shutter with longer readout times and all that). Would that
> be satisfactory?

Sounds good to me. I just thought how it actually had been tested. :-)

> 
> For contrast, the IMX290 datasheet states that VMAX shall be fixed at
> 0x465 for all-pixel mode / 0x2ee for 720p mode, and HMAX should be
> changed for frame rate control. As you say, sensors differ.

-- 
Kind regards,

Sakari Ailus

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

[Jai Luthra]

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

Hi Jacopo,

Thanks for the review.

On Nov 04, 2024 at 10:33:14 +0100, Jacopo Mondi wrote:
> Hello
> 
> On Tue, Oct 29, 2024 at 02:27:37PM +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.
> >
> > [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 | 149 ++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 106 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index de9230d4ad81f085640be254db9391ae7ad20773..140d958f80eb57dfb4ecf1796fcdf77081a662d7 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,
> > +	BINNING_X2,
> > +	BINNING_ANALOG_X2,
> 
> You could assign to the enumerated values the corresponding values of
> IMX219_BINNING_... so that in imx219_set_framefmt() you could write
> imx219->binning directly
> 

Will do.

> > +};
> > +
> > +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);
> 
> As noticed by Sakari, should hblank be similarly scaled ?
> 

From the datasheet, only the FRM_LENGTH_A (VTS) register unit changes to 
2 x Lines when analog binning is used. LINE_LENGTH_A (HTS) always has 
unit as Pixels.

Trying to scale it made the sensor not stream anything for analog binned 
modes.

> >  		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 */
> > @@ -473,7 +540,7 @@ static int imx219_init_controls(struct imx219 *imx219)
> >  	const struct imx219_mode *mode = &supported_modes[0];
> >  	struct v4l2_ctrl_handler *ctrl_hdlr;
> >  	struct v4l2_fwnode_device_properties props;
> > -	int exposure_max, exposure_def, hblank;
> > +	int exposure_max, exposure_def, hblank, pixel_rate;
> >  	int i, ret;
> >
> >  	ctrl_hdlr = &imx219->ctrl_handler;
> > @@ -482,11 +549,11 @@ static int imx219_init_controls(struct imx219 *imx219)
> >  		return ret;
> >
> >  	/* By default, PIXEL_RATE is read only */
> > +	pixel_rate = imx219_get_pixel_rate(imx219);
> >  	imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> >  					       V4L2_CID_PIXEL_RATE,
> > -					       imx219_get_pixel_rate(imx219),
> > -					       imx219_get_pixel_rate(imx219), 1,
> > -					       imx219_get_pixel_rate(imx219));
> > +					       pixel_rate, pixel_rate, 1,
> > +					       pixel_rate);
> 
> Is this change needed ?
> 

Hmm I guess the compiler would optimize it, I'll revert it.

> >
> >  	imx219->link_freq =
> >  		v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx219_ctrl_ops,
> > @@ -593,29 +660,13 @@ 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;
> > +	u64 binning;
> 
> why u64 for a value that gets written to a 16 bits register ?
> 

Dropped the variable altogether, now that enum stores the reg vals.

> > +	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);
> > @@ -626,28 +677,20 @@ 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;
> > +	switch (imx219->binning) {
> > +	case BINNING_NONE:
> > +		binning = 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;
> > +	case BINNING_X2:
> > +		binning = IMX219_BINNING_X2;
> >  		break;
> > -	case 2:
> > -		bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> > +	case BINNING_ANALOG_X2:
> > +		binning = IMX219_BINNING_X2_ANALOG;
> >  		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, binning, &ret);
> > +	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, binning, &ret);
> >
> >  	cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
> >  		  format->width, &ret);
> > @@ -851,6 +894,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 +937,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
> >
> >

-- 
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 2/3] media: i2c: imx219: make HBLANK r/w to allow longer exposures

[Jai Luthra]

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

Hi Sakari, Dave,

On Nov 12, 2024 at 10:49:24 +0000, Sakari Ailus wrote:
> Hi Dave,
> 
> On Mon, Nov 11, 2024 at 07:37:56PM +0000, Dave Stevenson wrote:
> > Hi Sakari
> > 
> > On Fri, 8 Nov 2024 at 10:30, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Dave,
> > >
> > > On Thu, Nov 07, 2024 at 12:43:52PM +0000, Dave Stevenson wrote:
> > > > Hi Sakari
> > > >
> > > > On Fri, 1 Nov 2024 at 08:48, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > Hi Jai,
> > > > >
> > > > > On Tue, Oct 29, 2024 at 02:27:36PM +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.
> > > > >
> > > > > It looks like this changes horizontal blanking at least in some cases. Does
> > > > > this also work as expected in binned modes, for instance?
> > > > >
> > > > > Many sensors have image quality related issues on untested albeit
> > > > > functional line length values.
> > > > >
> > > > > So my question is: how has this been validated?
> > > >
> > > > Validated by Sony, or others?
> > > > I've tested a range of values in all modes and not observed any image
> > > > quality issues.
> > >
> > > Somehow at least. :-)
> > >
> > > >
> > > > From previous discussions with Sony, they always provide their big
> > > > spreadsheet of register values for the specific mode and frame rate
> > > > requested. I don't think they even officially state that changing
> > > > VTS/FRM_LENGTH_LINES to change the framerate is permitted.
> > > > There are some Sony datasheets (eg imx258) that state "set to X. Any
> > > > other value please confirm with Sony", but that isn't the case for the
> > > > imx219 datasheet. I take that as it is permitted within the defined
> > > > ranges.
> > >
> > > I'm not that much concerned of vertical blanking, changing that within the
> > > valid range has effects on the image itself very seldom. Horizontal
> > > blanking is different though and this is what the patch makes changeable,
> > > including a change in the default value. Of course there are big
> > > differences between sensors here.
> > 
> > The intention was that the default value shouldn't change, and as the
> > overall PIXELS_PER_LINE value was meant to be retained on a mode
> > change the value used should only change if an application changes
> > V4L2_CID_HBLANK. If I blundered in the implementation of that, then
> > that should be fixed (I know Jacopo made comments, but I haven't had a
> > chance to investigate).
> 
> I guess I misread the patch. It indeed should be the same.
> 
> > 
> > I doubt we'd get validation from Sony beyond the contents of the
> > datasheet. Potentially as the sensor is so old they don't have the
> > information or engineers involved.
> > I'm happy to set up a test system and capture a set of images with
> > HBLANK from min to max at some increment. With the same exposure and
> > gain they should all be identical as long as there isn't any movement
> > (rolling shutter with longer readout times and all that). Would that
> > be satisfactory?
> 
> Sounds good to me. I just thought how it actually had been tested. :-)
> 

While not a thorough test, I manually tested with different values for 
horizontal_blanking for both binned and non-binned modes, and the image 
quality looked okay, with expected behaviour (i.e. increase in exposure 
and decrease in frame rate as total pixels per line increase)

I will send a v2 of this series with all the fixes.

> > 
> > For contrast, the IMX290 datasheet states that VMAX shall be fixed at
> > 0x465 for all-pixel mode / 0x2ee for 720p mode, and HMAX should be
> > changed for frame rate control. As you say, sensors differ.
> 
> -- 
> Kind regards,
> 
> Sakari Ailus

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