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

[PATCH v3 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 v3:
- PATCH 3/3: Calculate binning mode to use instead of hardcoding
  per-resolution
- Link to v2: https://lore.kernel.org/r/20241121-imx219_fixes-v2-0-7b068a60ea40@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 | 159 ++++++++++++++++++++++++++++-----------------
 1 file changed, 101 insertions(+), 58 deletions(-)
---
base-commit: adc218676eef25575469234709c2d87185ca223a
change-id: 20241029-imx219_fixes-bfaac4a56200

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

[PATCH v3 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 v3 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>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.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 v3 2/3] media: i2c: imx219: make HBLANK r/w to allow longer exposures

[Laurent Pinchart]

On Mon, Nov 25, 2024 at 08:36:26PM +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>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.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 */

Just drop the comment, and drop the blank lines, this belongs to the
"V_TIMING internal" section.

> +#define IMX219_PPL_MIN			0x0d78

Why PPL and not HTS ?

> +#define IMX219_PPL_MAX			0x7ff0
> +#define IMX219_REG_HTS			CCI_REG16(0x0162)

The min/max should go below the register definition.

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

The minimum and maximum are identical, is this intentional ?

>  					   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

Rename PPL to HTS here too.

> +		 * 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;
> 

-- 
Regards,

Laurent Pinchart

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

[Dave Stevenson]

Hi Laurent

On Tue, 26 Nov 2024 at 12:16, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Mon, Nov 25, 2024 at 08:36:26PM +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>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.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 */
>
> Just drop the comment, and drop the blank lines, this belongs to the
> "V_TIMING internal" section.
>
> > +#define IMX219_PPL_MIN                       0x0d78
>
> Why PPL and not HTS ?

The IMX219 datasheet defines the register as LINE_LENGTH_A, with
comment line_length_pck.

HTS is not a term used in the imx219 datasheet, so why introduce it to
the driver? I'd go along with a rename to LLP if you wish.
(HTS is more commonly an Omnivision term, not a Sony one).

> > +#define IMX219_PPL_MAX                       0x7ff0
> > +#define IMX219_REG_HTS                       CCI_REG16(0x0162)
>
> The min/max should go below the register definition.
>
> >
> >  #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,
>
> The minimum and maximum are identical, is this intentional ?

The limits should be updated when the format is set, so the values
shouldn't matter when created. However I'd want to check that did
happen seeing as vblank computes the limits here.
It is as easy to set them correctly here too (max = IMX219_PPL_MAX -
mode->width), so you may as well.

> >                                          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
>
> Rename PPL to HTS here too.

Disagree as above. The local variable here should be renamed prev_ppl
or prev_llp.

  Dave

> > +              * 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;
> >
>
> --
> Regards,
>
> Laurent Pinchart

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

[Jai Luthra]

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

Hi Dave,

On Nov 26, 2024 at 13:21:27 +0000, Dave Stevenson wrote:
> Hi Laurent
> 
> On Tue, 26 Nov 2024 at 12:16, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > On Mon, Nov 25, 2024 at 08:36:26PM +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>
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.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 */
> >
> > Just drop the comment, and drop the blank lines, this belongs to the
> > "V_TIMING internal" section.
> >
> > > +#define IMX219_PPL_MIN                       0x0d78
> >
> > Why PPL and not HTS ?
> 
> The IMX219 datasheet defines the register as LINE_LENGTH_A, with
> comment line_length_pck.
> 
> HTS is not a term used in the imx219 datasheet, so why introduce it to
> the driver? I'd go along with a rename to LLP if you wish.
> (HTS is more commonly an Omnivision term, not a Sony one).

The FRAME_LENGTH_A register is also named IMX219_REG_VTS in the driver, 
so I think calling this PPL would be confusing, unless we also rename 
VTS to something else (frame_length_lines -> so FLL?)

Just my two cents but VTS/HTS is at least instantly parsable to my 
eyes.. compared to the sony names.

> 
> > > +#define IMX219_PPL_MAX                       0x7ff0
> > > +#define IMX219_REG_HTS                       CCI_REG16(0x0162)
> >
> > The min/max should go below the register definition.
> >
> > >
> > >  #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,
> >
> > The minimum and maximum are identical, is this intentional ?
> 
> The limits should be updated when the format is set, so the values
> shouldn't matter when created. However I'd want to check that did
> happen seeing as vblank computes the limits here.
> It is as easy to set them correctly here too (max = IMX219_PPL_MAX -
> mode->width), so you may as well.
> 
> > >                                          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
> >
> > Rename PPL to HTS here too.
> 
> Disagree as above. The local variable here should be renamed prev_ppl
> or prev_llp.
> 
>   Dave
> 
> > > +              * 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;
> > >
> >
> > --
> > Regards,
> >
> > Laurent Pinchart

Thanks,
Jai

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

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

[Laurent Pinchart]

Hi Dave,

On Tue, Nov 26, 2024 at 01:21:27PM +0000, Dave Stevenson wrote:
> On Tue, 26 Nov 2024 at 12:16, Laurent Pinchart wrote:
> > On Mon, Nov 25, 2024 at 08:36:26PM +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>
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.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 */
> >
> > Just drop the comment, and drop the blank lines, this belongs to the
> > "V_TIMING internal" section.
> >
> > > +#define IMX219_PPL_MIN                       0x0d78
> >
> > Why PPL and not HTS ?
> 
> The IMX219 datasheet defines the register as LINE_LENGTH_A, with
> comment line_length_pck.
> 
> HTS is not a term used in the imx219 datasheet, so why introduce it to
> the driver? I'd go along with a rename to LLP if you wish.
> (HTS is more commonly an Omnivision term, not a Sony one).

I thought the datasheet used HTS, my bad. Now that you mention it, HTS
is indeed more of an OmniVision term. Sorry for the oversight.

I would then name the macro IMX219_REG_LINE_LENGTH, and rename
IMX219_REG_VTS to IMX219_REG_FRM_LENGTH.

> > > +#define IMX219_PPL_MAX                       0x7ff0
> > > +#define IMX219_REG_HTS                       CCI_REG16(0x0162)
> >
> > The min/max should go below the register definition.
> >
> > >
> > >  #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,
> >
> > The minimum and maximum are identical, is this intentional ?
> 
> The limits should be updated when the format is set, so the values
> shouldn't matter when created. However I'd want to check that did
> happen seeing as vblank computes the limits here.
> It is as easy to set them correctly here too (max = IMX219_PPL_MAX -
> mode->width), so you may as well.

I would at least handle hblank and vblank the same way. If we expect the
values to be updated, a comment to indicate so would be good. The
current code states

	/* Initial vblank/hblank/exposure parameters based on current mode */

> > >                                          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
> >
> > Rename PPL to HTS here too.
> 
> Disagree as above. The local variable here should be renamed prev_ppl
> or prev_llp.

Or prev_line_length. I agree that hts isn't a good name here.

> > > +              * 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;

-- 
Regards,

Laurent Pinchart

[PATCH v3 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.

[1]: https://github.com/raspberrypi/linux/issues/5493

Co-developed-by: Naushir Patuck <naush@raspberrypi.com>
Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Co-developed-by: Vinay Varma <varmavinaym@gmail.com>
Signed-off-by: Vinay Varma <varmavinaym@gmail.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/imx219.c | 120 ++++++++++++++++++++++++++++-----------------
 1 file changed, 76 insertions(+), 44 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 970e6362d0ae3a9078daf337155e83d637bc1ca1..39b85cdee58318b080c867afd68ca33d14d3eda7 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -149,6 +149,12 @@
 #define IMX219_PIXEL_ARRAY_WIDTH	3280U
 #define IMX219_PIXEL_ARRAY_HEIGHT	2464U
 
+enum binning_mode {
+	BINNING_NONE = IMX219_BINNING_NONE,
+	BINNING_X2 = IMX219_BINNING_X2,
+	BINNING_ANALOG_X2 = IMX219_BINNING_X2_ANALOG,
+};
+
 /* Mode : resolution and related config&values */
 struct imx219_mode {
 	/* Frame width */
@@ -337,6 +343,10 @@ struct imx219 {
 
 	/* Two or Four lanes */
 	u8 lanes;
+
+	/* Binning mode */
+	enum binning_mode bin_h;
+	enum binning_mode bin_v;
 };
 
 static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
@@ -362,6 +372,36 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
 	return imx219_mbus_formats[i];
 }
 
+static u32 imx219_get_format_bpp(const struct v4l2_mbus_framefmt *format)
+{
+	switch (format->code) {
+	case MEDIA_BUS_FMT_SRGGB8_1X8:
+	case MEDIA_BUS_FMT_SGRBG8_1X8:
+	case MEDIA_BUS_FMT_SGBRG8_1X8:
+	case MEDIA_BUS_FMT_SBGGR8_1X8:
+		return 8;
+
+	case MEDIA_BUS_FMT_SRGGB10_1X10:
+	case MEDIA_BUS_FMT_SGRBG10_1X10:
+	case MEDIA_BUS_FMT_SGBRG10_1X10:
+	case MEDIA_BUS_FMT_SBGGR10_1X10:
+	default:
+		return 10;
+	}
+}
+
+static int imx219_get_rate_factor(struct imx219 *imx219)
+{
+	switch (imx219->bin_v) {
+	case BINNING_NONE:
+	case BINNING_X2:
+		return 1;
+	case BINNING_ANALOG_X2:
+		return 2;
+	}
+	return -EINVAL;
+}
+
 /* -----------------------------------------------------------------------------
  * Controls
  */
@@ -373,10 +413,12 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
 	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
 	const struct v4l2_mbus_framefmt *format;
 	struct v4l2_subdev_state *state;
+	int rate_factor;
 	int ret = 0;
 
 	state = v4l2_subdev_get_locked_active_state(&imx219->sd);
 	format = v4l2_subdev_state_get_format(state, 0);
+	rate_factor = imx219_get_rate_factor(imx219);
 
 	if (ctrl->id == V4L2_CID_VBLANK) {
 		int exposure_max, exposure_def;
@@ -405,7 +447,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	case V4L2_CID_EXPOSURE:
 		cci_write(imx219->regmap, IMX219_REG_EXPOSURE,
-			  ctrl->val, &ret);
+			  ctrl->val / rate_factor, &ret);
 		break;
 	case V4L2_CID_DIGITAL_GAIN:
 		cci_write(imx219->regmap, IMX219_REG_DIGITAL_GAIN,
@@ -422,7 +464,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	case V4L2_CID_VBLANK:
 		cci_write(imx219->regmap, IMX219_REG_VTS,
-			  format->height + ctrl->val, &ret);
+			  (format->height + ctrl->val) / rate_factor, &ret);
 		break;
 	case V4L2_CID_HBLANK:
 		cci_write(imx219->regmap, IMX219_REG_HTS,
@@ -463,7 +505,8 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
 
 static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
 {
-	return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE;
+	return ((imx219->lanes == 2) ? IMX219_PIXEL_RATE :
+		IMX219_PIXEL_RATE_4LANE) * imx219_get_rate_factor(imx219);
 }
 
 /* Initialize control handlers */
@@ -592,29 +635,12 @@ static int imx219_set_framefmt(struct imx219 *imx219,
 {
 	const struct v4l2_mbus_framefmt *format;
 	const struct v4l2_rect *crop;
-	unsigned int bpp;
-	u64 bin_h, bin_v;
+	u32 bpp;
 	int ret = 0;
 
 	format = v4l2_subdev_state_get_format(state, 0);
 	crop = v4l2_subdev_state_get_crop(state, 0);
-
-	switch (format->code) {
-	case MEDIA_BUS_FMT_SRGGB8_1X8:
-	case MEDIA_BUS_FMT_SGRBG8_1X8:
-	case MEDIA_BUS_FMT_SGBRG8_1X8:
-	case MEDIA_BUS_FMT_SBGGR8_1X8:
-		bpp = 8;
-		break;
-
-	case MEDIA_BUS_FMT_SRGGB10_1X10:
-	case MEDIA_BUS_FMT_SGRBG10_1X10:
-	case MEDIA_BUS_FMT_SGBRG10_1X10:
-	case MEDIA_BUS_FMT_SBGGR10_1X10:
-	default:
-		bpp = 10;
-		break;
-	}
+	bpp = imx219_get_format_bpp(format);
 
 	cci_write(imx219->regmap, IMX219_REG_X_ADD_STA_A,
 		  crop->left - IMX219_PIXEL_ARRAY_LEFT, &ret);
@@ -625,28 +651,8 @@ static int imx219_set_framefmt(struct imx219 *imx219,
 	cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A,
 		  crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret);
 
-	switch (crop->width / format->width) {
-	case 1:
-	default:
-		bin_h = IMX219_BINNING_NONE;
-		break;
-	case 2:
-		bin_h = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
-		break;
-	}
-
-	switch (crop->height / format->height) {
-	case 1:
-	default:
-		bin_v = IMX219_BINNING_NONE;
-		break;
-	case 2:
-		bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
-		break;
-	}
-
-	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret);
-	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret);
+	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, imx219->bin_h, &ret);
+	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, imx219->bin_v, &ret);
 
 	cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
 		  format->width, &ret);
@@ -851,6 +857,27 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 		int exposure_max;
 		int exposure_def;
 		int hblank;
+		int pixel_rate;
+		u32 bpp = imx219_get_format_bpp(format);
+		enum binning_mode binning = BINNING_NONE;
+
+		/*
+		 * For 8-bit formats, analog horizontal binning is required,
+		 * else the output image is garbage.
+		 * For 10-bit formats, analog horizontal binning is optional,
+		 * but still useful as it doubles the effective framerate.
+		 * We can only use it with width <= 1624, as for higher values
+		 * there are blocky artefacts.
+		 *
+		 * Vertical binning should match the horizontal binning mode.
+		 */
+		if (bin_h == 2 && (format->width <= 1624 || bpp == 8))
+			binning = BINNING_ANALOG_X2;
+		else
+			binning = BINNING_X2;
+
+		imx219->bin_h = (bin_h == 2) ? binning : BINNING_NONE;
+		imx219->bin_v = (bin_v == 2) ? binning : BINNING_NONE;
 
 		/* Update limits and set FPS to default */
 		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
@@ -879,6 +906,11 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 					 IMX219_PPL_MAX - mode->width,
 					 1, IMX219_PPL_MIN - mode->width);
 		__v4l2_ctrl_s_ctrl(imx219->hblank, hblank);
+
+		/* Scale the pixel rate based on the mode specific factor */
+		pixel_rate = imx219_get_pixel_rate(imx219);
+		__v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate,
+					 pixel_rate, 1, pixel_rate);
 	}
 
 	return 0;

-- 
2.47.0

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

[Dave Stevenson]

Hi Jai

On Mon, 25 Nov 2024 at 15:07, Jai Luthra <jai.luthra@ideasonboard.com> wrote:
>
> When the analog binning mode is used for high framerate operation,
> the pixel rate is effectively doubled. Account for this when setting up
> the pixel clock rate, and applying the vblank and exposure controls.
>
> The previous logic only used analog binning for 8-bit modes, but normal
> binning limits the framerate on 10-bit 480p [1]. So with this patch we
> switch to using special binning (with 2x pixel rate) for all formats of
> 480p mode and 8-bit 1232p.
>
> [1]: https://github.com/raspberrypi/linux/issues/5493
>
> Co-developed-by: Naushir Patuck <naush@raspberrypi.com>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Co-developed-by: Vinay Varma <varmavinaym@gmail.com>
> Signed-off-by: Vinay Varma <varmavinaym@gmail.com>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/i2c/imx219.c | 120 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 76 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 970e6362d0ae3a9078daf337155e83d637bc1ca1..39b85cdee58318b080c867afd68ca33d14d3eda7 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -149,6 +149,12 @@
>  #define IMX219_PIXEL_ARRAY_WIDTH       3280U
>  #define IMX219_PIXEL_ARRAY_HEIGHT      2464U
>
> +enum binning_mode {
> +       BINNING_NONE = IMX219_BINNING_NONE,
> +       BINNING_X2 = IMX219_BINNING_X2,
> +       BINNING_ANALOG_X2 = IMX219_BINNING_X2_ANALOG,
> +};
> +
>  /* Mode : resolution and related config&values */
>  struct imx219_mode {
>         /* Frame width */
> @@ -337,6 +343,10 @@ struct imx219 {
>
>         /* Two or Four lanes */
>         u8 lanes;
> +
> +       /* Binning mode */
> +       enum binning_mode bin_h;
> +       enum binning_mode bin_v;
>  };
>
>  static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> @@ -362,6 +372,36 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
>         return imx219_mbus_formats[i];
>  }
>
> +static u32 imx219_get_format_bpp(const struct v4l2_mbus_framefmt *format)
> +{
> +       switch (format->code) {
> +       case MEDIA_BUS_FMT_SRGGB8_1X8:
> +       case MEDIA_BUS_FMT_SGRBG8_1X8:
> +       case MEDIA_BUS_FMT_SGBRG8_1X8:
> +       case MEDIA_BUS_FMT_SBGGR8_1X8:
> +               return 8;
> +
> +       case MEDIA_BUS_FMT_SRGGB10_1X10:
> +       case MEDIA_BUS_FMT_SGRBG10_1X10:
> +       case MEDIA_BUS_FMT_SGBRG10_1X10:
> +       case MEDIA_BUS_FMT_SBGGR10_1X10:
> +       default:
> +               return 10;
> +       }
> +}
> +
> +static int imx219_get_rate_factor(struct imx219 *imx219)
> +{
> +       switch (imx219->bin_v) {
> +       case BINNING_NONE:
> +       case BINNING_X2:
> +               return 1;
> +       case BINNING_ANALOG_X2:
> +               return 2;
> +       }
> +       return -EINVAL;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Controls
>   */
> @@ -373,10 +413,12 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
>         struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
>         const struct v4l2_mbus_framefmt *format;
>         struct v4l2_subdev_state *state;
> +       int rate_factor;
>         int ret = 0;
>
>         state = v4l2_subdev_get_locked_active_state(&imx219->sd);
>         format = v4l2_subdev_state_get_format(state, 0);
> +       rate_factor = imx219_get_rate_factor(imx219);
>
>         if (ctrl->id == V4L2_CID_VBLANK) {
>                 int exposure_max, exposure_def;
> @@ -405,7 +447,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
>                 break;
>         case V4L2_CID_EXPOSURE:
>                 cci_write(imx219->regmap, IMX219_REG_EXPOSURE,
> -                         ctrl->val, &ret);
> +                         ctrl->val / rate_factor, &ret);
>                 break;
>         case V4L2_CID_DIGITAL_GAIN:
>                 cci_write(imx219->regmap, IMX219_REG_DIGITAL_GAIN,
> @@ -422,7 +464,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
>                 break;
>         case V4L2_CID_VBLANK:
>                 cci_write(imx219->regmap, IMX219_REG_VTS,
> -                         format->height + ctrl->val, &ret);
> +                         (format->height + ctrl->val) / rate_factor, &ret);
>                 break;
>         case V4L2_CID_HBLANK:
>                 cci_write(imx219->regmap, IMX219_REG_HTS,
> @@ -463,7 +505,8 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
>
>  static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
>  {
> -       return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE;
> +       return ((imx219->lanes == 2) ? IMX219_PIXEL_RATE :
> +               IMX219_PIXEL_RATE_4LANE) * imx219_get_rate_factor(imx219);

This feels wrong.
imx219_get_rate_factor will return the factor based on vertical
binning. I would have expected the pixel rate to change based on
horizontal binning.
You'd need a mode which uses different binning modes in each direction
to verify that though.

(Updating exposure and vblank values based on vertical binning makes
sense, as they are both in terms of lines).

  Dave

>  }
>
>  /* Initialize control handlers */
> @@ -592,29 +635,12 @@ static int imx219_set_framefmt(struct imx219 *imx219,
>  {
>         const struct v4l2_mbus_framefmt *format;
>         const struct v4l2_rect *crop;
> -       unsigned int bpp;
> -       u64 bin_h, bin_v;
> +       u32 bpp;
>         int ret = 0;
>
>         format = v4l2_subdev_state_get_format(state, 0);
>         crop = v4l2_subdev_state_get_crop(state, 0);
> -
> -       switch (format->code) {
> -       case MEDIA_BUS_FMT_SRGGB8_1X8:
> -       case MEDIA_BUS_FMT_SGRBG8_1X8:
> -       case MEDIA_BUS_FMT_SGBRG8_1X8:
> -       case MEDIA_BUS_FMT_SBGGR8_1X8:
> -               bpp = 8;
> -               break;
> -
> -       case MEDIA_BUS_FMT_SRGGB10_1X10:
> -       case MEDIA_BUS_FMT_SGRBG10_1X10:
> -       case MEDIA_BUS_FMT_SGBRG10_1X10:
> -       case MEDIA_BUS_FMT_SBGGR10_1X10:
> -       default:
> -               bpp = 10;
> -               break;
> -       }
> +       bpp = imx219_get_format_bpp(format);
>
>         cci_write(imx219->regmap, IMX219_REG_X_ADD_STA_A,
>                   crop->left - IMX219_PIXEL_ARRAY_LEFT, &ret);
> @@ -625,28 +651,8 @@ static int imx219_set_framefmt(struct imx219 *imx219,
>         cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A,
>                   crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret);
>
> -       switch (crop->width / format->width) {
> -       case 1:
> -       default:
> -               bin_h = IMX219_BINNING_NONE;
> -               break;
> -       case 2:
> -               bin_h = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> -               break;
> -       }
> -
> -       switch (crop->height / format->height) {
> -       case 1:
> -       default:
> -               bin_v = IMX219_BINNING_NONE;
> -               break;
> -       case 2:
> -               bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> -               break;
> -       }
> -
> -       cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret);
> -       cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret);
> +       cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, imx219->bin_h, &ret);
> +       cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, imx219->bin_v, &ret);
>
>         cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
>                   format->width, &ret);
> @@ -851,6 +857,27 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>                 int exposure_max;
>                 int exposure_def;
>                 int hblank;
> +               int pixel_rate;
> +               u32 bpp = imx219_get_format_bpp(format);
> +               enum binning_mode binning = BINNING_NONE;
> +
> +               /*
> +                * For 8-bit formats, analog horizontal binning is required,
> +                * else the output image is garbage.
> +                * For 10-bit formats, analog horizontal binning is optional,
> +                * but still useful as it doubles the effective framerate.
> +                * We can only use it with width <= 1624, as for higher values
> +                * there are blocky artefacts.
> +                *
> +                * Vertical binning should match the horizontal binning mode.
> +                */
> +               if (bin_h == 2 && (format->width <= 1624 || bpp == 8))
> +                       binning = BINNING_ANALOG_X2;
> +               else
> +                       binning = BINNING_X2;
> +
> +               imx219->bin_h = (bin_h == 2) ? binning : BINNING_NONE;
> +               imx219->bin_v = (bin_v == 2) ? binning : BINNING_NONE;
>
>                 /* Update limits and set FPS to default */
>                 __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> @@ -879,6 +906,11 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>                                          IMX219_PPL_MAX - mode->width,
>                                          1, IMX219_PPL_MIN - mode->width);
>                 __v4l2_ctrl_s_ctrl(imx219->hblank, hblank);
> +
> +               /* Scale the pixel rate based on the mode specific factor */
> +               pixel_rate = imx219_get_pixel_rate(imx219);
> +               __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate,
> +                                        pixel_rate, 1, pixel_rate);
>         }
>
>         return 0;
>
> --
> 2.47.0
>

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

[Jai Luthra]

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

Hi Dave,

On Nov 25, 2024 at 18:40:50 +0000, Dave Stevenson wrote:
> Hi Jai
> 
> On Mon, 25 Nov 2024 at 15:07, Jai Luthra <jai.luthra@ideasonboard.com> wrote:
> >
> > When the analog binning mode is used for high framerate operation,
> > the pixel rate is effectively doubled. Account for this when setting up
> > the pixel clock rate, and applying the vblank and exposure controls.
> >
> > The previous logic only used analog binning for 8-bit modes, but normal
> > binning limits the framerate on 10-bit 480p [1]. So with this patch we
> > switch to using special binning (with 2x pixel rate) for all formats of
> > 480p mode and 8-bit 1232p.
> >
> > [1]: https://github.com/raspberrypi/linux/issues/5493
> >
> > Co-developed-by: Naushir Patuck <naush@raspberrypi.com>
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Co-developed-by: Vinay Varma <varmavinaym@gmail.com>
> > Signed-off-by: Vinay Varma <varmavinaym@gmail.com>
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > ---
> >  drivers/media/i2c/imx219.c | 120 ++++++++++++++++++++++++++++-----------------
> >  1 file changed, 76 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 970e6362d0ae3a9078daf337155e83d637bc1ca1..39b85cdee58318b080c867afd68ca33d14d3eda7 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -149,6 +149,12 @@
> >  #define IMX219_PIXEL_ARRAY_WIDTH       3280U
> >  #define IMX219_PIXEL_ARRAY_HEIGHT      2464U
> >
> > +enum binning_mode {
> > +       BINNING_NONE = IMX219_BINNING_NONE,
> > +       BINNING_X2 = IMX219_BINNING_X2,
> > +       BINNING_ANALOG_X2 = IMX219_BINNING_X2_ANALOG,
> > +};
> > +
> >  /* Mode : resolution and related config&values */
> >  struct imx219_mode {
> >         /* Frame width */
> > @@ -337,6 +343,10 @@ struct imx219 {
> >
> >         /* Two or Four lanes */
> >         u8 lanes;
> > +
> > +       /* Binning mode */
> > +       enum binning_mode bin_h;
> > +       enum binning_mode bin_v;
> >  };
> >
> >  static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> > @@ -362,6 +372,36 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
> >         return imx219_mbus_formats[i];
> >  }
> >
> > +static u32 imx219_get_format_bpp(const struct v4l2_mbus_framefmt *format)
> > +{
> > +       switch (format->code) {
> > +       case MEDIA_BUS_FMT_SRGGB8_1X8:
> > +       case MEDIA_BUS_FMT_SGRBG8_1X8:
> > +       case MEDIA_BUS_FMT_SGBRG8_1X8:
> > +       case MEDIA_BUS_FMT_SBGGR8_1X8:
> > +               return 8;
> > +
> > +       case MEDIA_BUS_FMT_SRGGB10_1X10:
> > +       case MEDIA_BUS_FMT_SGRBG10_1X10:
> > +       case MEDIA_BUS_FMT_SGBRG10_1X10:
> > +       case MEDIA_BUS_FMT_SBGGR10_1X10:
> > +       default:
> > +               return 10;
> > +       }
> > +}
> > +
> > +static int imx219_get_rate_factor(struct imx219 *imx219)
> > +{
> > +       switch (imx219->bin_v) {
> > +       case BINNING_NONE:
> > +       case BINNING_X2:
> > +               return 1;
> > +       case BINNING_ANALOG_X2:
> > +               return 2;
> > +       }
> > +       return -EINVAL;
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Controls
> >   */
> > @@ -373,10 +413,12 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> >         struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> >         const struct v4l2_mbus_framefmt *format;
> >         struct v4l2_subdev_state *state;
> > +       int rate_factor;
> >         int ret = 0;
> >
> >         state = v4l2_subdev_get_locked_active_state(&imx219->sd);
> >         format = v4l2_subdev_state_get_format(state, 0);
> > +       rate_factor = imx219_get_rate_factor(imx219);
> >
> >         if (ctrl->id == V4L2_CID_VBLANK) {
> >                 int exposure_max, exposure_def;
> > @@ -405,7 +447,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> >                 break;
> >         case V4L2_CID_EXPOSURE:
> >                 cci_write(imx219->regmap, IMX219_REG_EXPOSURE,
> > -                         ctrl->val, &ret);
> > +                         ctrl->val / rate_factor, &ret);
> >                 break;
> >         case V4L2_CID_DIGITAL_GAIN:
> >                 cci_write(imx219->regmap, IMX219_REG_DIGITAL_GAIN,
> > @@ -422,7 +464,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> >                 break;
> >         case V4L2_CID_VBLANK:
> >                 cci_write(imx219->regmap, IMX219_REG_VTS,
> > -                         format->height + ctrl->val, &ret);
> > +                         (format->height + ctrl->val) / rate_factor, &ret);
> >                 break;
> >         case V4L2_CID_HBLANK:
> >                 cci_write(imx219->regmap, IMX219_REG_HTS,
> > @@ -463,7 +505,8 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
> >
> >  static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
> >  {
> > -       return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE;
> > +       return ((imx219->lanes == 2) ? IMX219_PIXEL_RATE :
> > +               IMX219_PIXEL_RATE_4LANE) * imx219_get_rate_factor(imx219);
> 
> This feels wrong.
> imx219_get_rate_factor will return the factor based on vertical
> binning. I would have expected the pixel rate to change based on
> horizontal binning.
> You'd need a mode which uses different binning modes in each direction
> to verify that though.
> 

I did tests with only vertically-binned and only horizontally-binned 
modes, and the framerate captured seemed to indicate that analog binning 
only changes the pixelrate when it is vertically binned.

With the following driver change on top of my series:

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 39b85cdee583..38039847ded5 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -312,6 +312,16 @@ static const struct imx219_mode supported_modes[] = {
                .height = 1232,
                .vts_def = 1763,
        },
+       {
+               .width = 3280,
+               .height = 1232,
+               .vts_def = 1763,
+       },
+       {
+               .width = 1624,
+               .height = 2464,
+               .vts_def = 1763,
+       },
        {
                /* 640x480 30fps mode */
                .width = 640,

If I configure the sensor to 1624 x 2464 it uses analog horizontal 
binning and no vertical binning.

With the default vblank = 531 and hblank = 1824, it captures 17.66 
frames/s [1]

182400000/((1624+1824)*(2464+531)) = 17.66

So the pixelrate seems to have stayed the same.

It's a stretch, but given that the datasheet mentions VTS unit is in 
2-Lines when using analog binning mode and no change to the HTS unit, I 
think the sensor must be taking the same time to read out a single line 
of pixels irrespective of if they are binned digitally or not.

> (Updating exposure and vblank values based on vertical binning makes
> sense, as they are both in terms of lines).

Agreed, only the pixelrate seems unintuitive.

> 
>   Dave
> 
> >  }
> >
> >  /* Initialize control handlers */
> > @@ -592,29 +635,12 @@ static int imx219_set_framefmt(struct imx219 *imx219,
> >  {
> >         const struct v4l2_mbus_framefmt *format;
> >         const struct v4l2_rect *crop;
> > -       unsigned int bpp;
> > -       u64 bin_h, bin_v;
> > +       u32 bpp;
> >         int ret = 0;
> >
> >         format = v4l2_subdev_state_get_format(state, 0);
> >         crop = v4l2_subdev_state_get_crop(state, 0);
> > -
> > -       switch (format->code) {
> > -       case MEDIA_BUS_FMT_SRGGB8_1X8:
> > -       case MEDIA_BUS_FMT_SGRBG8_1X8:
> > -       case MEDIA_BUS_FMT_SGBRG8_1X8:
> > -       case MEDIA_BUS_FMT_SBGGR8_1X8:
> > -               bpp = 8;
> > -               break;
> > -
> > -       case MEDIA_BUS_FMT_SRGGB10_1X10:
> > -       case MEDIA_BUS_FMT_SGRBG10_1X10:
> > -       case MEDIA_BUS_FMT_SGBRG10_1X10:
> > -       case MEDIA_BUS_FMT_SBGGR10_1X10:
> > -       default:
> > -               bpp = 10;
> > -               break;
> > -       }
> > +       bpp = imx219_get_format_bpp(format);
> >
> >         cci_write(imx219->regmap, IMX219_REG_X_ADD_STA_A,
> >                   crop->left - IMX219_PIXEL_ARRAY_LEFT, &ret);
> > @@ -625,28 +651,8 @@ static int imx219_set_framefmt(struct imx219 *imx219,
> >         cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A,
> >                   crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret);
> >
> > -       switch (crop->width / format->width) {
> > -       case 1:
> > -       default:
> > -               bin_h = IMX219_BINNING_NONE;
> > -               break;
> > -       case 2:
> > -               bin_h = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> > -               break;
> > -       }
> > -
> > -       switch (crop->height / format->height) {
> > -       case 1:
> > -       default:
> > -               bin_v = IMX219_BINNING_NONE;
> > -               break;
> > -       case 2:
> > -               bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> > -               break;
> > -       }
> > -
> > -       cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret);
> > -       cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret);
> > +       cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, imx219->bin_h, &ret);
> > +       cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, imx219->bin_v, &ret);
> >
> >         cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
> >                   format->width, &ret);
> > @@ -851,6 +857,27 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >                 int exposure_max;
> >                 int exposure_def;
> >                 int hblank;
> > +               int pixel_rate;
> > +               u32 bpp = imx219_get_format_bpp(format);
> > +               enum binning_mode binning = BINNING_NONE;
> > +
> > +               /*
> > +                * For 8-bit formats, analog horizontal binning is required,
> > +                * else the output image is garbage.
> > +                * For 10-bit formats, analog horizontal binning is optional,
> > +                * but still useful as it doubles the effective framerate.
> > +                * We can only use it with width <= 1624, as for higher values
> > +                * there are blocky artefacts.
> > +                *
> > +                * Vertical binning should match the horizontal binning mode.
> > +                */
> > +               if (bin_h == 2 && (format->width <= 1624 || bpp == 8))
> > +                       binning = BINNING_ANALOG_X2;
> > +               else
> > +                       binning = BINNING_X2;
> > +
> > +               imx219->bin_h = (bin_h == 2) ? binning : BINNING_NONE;
> > +               imx219->bin_v = (bin_v == 2) ? binning : BINNING_NONE;
> >
> >                 /* Update limits and set FPS to default */
> >                 __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > @@ -879,6 +906,11 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >                                          IMX219_PPL_MAX - mode->width,
> >                                          1, IMX219_PPL_MIN - mode->width);
> >                 __v4l2_ctrl_s_ctrl(imx219->hblank, hblank);
> > +
> > +               /* Scale the pixel rate based on the mode specific factor */
> > +               pixel_rate = imx219_get_pixel_rate(imx219);
> > +               __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate,
> > +                                        pixel_rate, 1, pixel_rate);
> >         }
> >
> >         return 0;
> >
> > --
> > 2.47.0
> >

[1]:

- entity 41: imx219 1-0010 (1 pad, 1 link, 0 routes)
             type V4L2 subdev subtype Sensor flags 0
             device node name /dev/v4l-subdev4
        pad0: SOURCE
                [stream:0 fmt:SRGGB10_1X10/1624x2464 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range
                 crop.bounds:(8,8)/3280x2464
                 crop:(24,8)/3248x2464]
                -> "csis-32e40000.csi":0 [ENABLED]

$ v4l2-ctl -d0 --stream-mmap
<<<<<<<<<<<<<<<<<<< 17.66 fps
<<<<<<<<<<<<<<<<<< 17.66 fps
<<<<^C

$ v4l2-ctl -d/dev/v4l-subdev4 -l | grep blanking
              vertical_blanking 0x009e0901 (int)    : min=32 max=64303 step=1 default=531 value=531
            horizontal_blanking 0x009e0902 (int)    : min=1824 max=31128 step=1 default=1824 value=1824


Thanks,
Jai

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

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

[Sakari Ailus]

Hi Jai,

On Mon, Nov 25, 2024 at 08:36:27PM +0530, Jai Luthra wrote:
> When the analog binning mode is used for high framerate operation,
> the pixel rate is effectively doubled. Account for this when setting up
> the pixel clock rate, and applying the vblank and exposure controls.
> 
> The previous logic only used analog binning for 8-bit modes, but normal
> binning limits the framerate on 10-bit 480p [1]. So with this patch we
> switch to using special binning (with 2x pixel rate) for all formats of
> 480p mode and 8-bit 1232p.
> 
> [1]: https://github.com/raspberrypi/linux/issues/5493
> 
> Co-developed-by: Naushir Patuck <naush@raspberrypi.com>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Co-developed-by: Vinay Varma <varmavinaym@gmail.com>
> Signed-off-by: Vinay Varma <varmavinaym@gmail.com>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/i2c/imx219.c | 120 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 76 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 970e6362d0ae3a9078daf337155e83d637bc1ca1..39b85cdee58318b080c867afd68ca33d14d3eda7 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -149,6 +149,12 @@
>  #define IMX219_PIXEL_ARRAY_WIDTH	3280U
>  #define IMX219_PIXEL_ARRAY_HEIGHT	2464U
>  
> +enum binning_mode {
> +	BINNING_NONE = IMX219_BINNING_NONE,
> +	BINNING_X2 = IMX219_BINNING_X2,
> +	BINNING_ANALOG_X2 = IMX219_BINNING_X2_ANALOG,
> +};
> +
>  /* Mode : resolution and related config&values */
>  struct imx219_mode {
>  	/* Frame width */
> @@ -337,6 +343,10 @@ struct imx219 {
>  
>  	/* Two or Four lanes */
>  	u8 lanes;
> +
> +	/* Binning mode */
> +	enum binning_mode bin_h;
> +	enum binning_mode bin_v;
>  };
>  
>  static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> @@ -362,6 +372,36 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
>  	return imx219_mbus_formats[i];
>  }
>  
> +static u32 imx219_get_format_bpp(const struct v4l2_mbus_framefmt *format)
> +{
> +	switch (format->code) {
> +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> +		return 8;
> +
> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> +	default:
> +		return 10;
> +	}
> +}
> +
> +static int imx219_get_rate_factor(struct imx219 *imx219)
> +{
> +	switch (imx219->bin_v) {
> +	case BINNING_NONE:
> +	case BINNING_X2:
> +		return 1;
> +	case BINNING_ANALOG_X2:
> +		return 2;

FWIW, what the CCS driver does is that it exposes different horizontal
blanking ranges for devices that use analogue binning. The rate is really
about reading pixels and with analogue binning the rate is the same, it's
just that fewer pixels are being (digitally) read (as they are binned). I
wonder if this would be a workable approach for this sensor, too. Of course
if the LLP behaves differently for this sensor, then we should probably
just accept that.

> +	}
> +	return -EINVAL;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Controls
>   */
> @@ -373,10 +413,12 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
>  	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
>  	const struct v4l2_mbus_framefmt *format;
>  	struct v4l2_subdev_state *state;
> +	int rate_factor;

u32?

>  	int ret = 0;
>  
>  	state = v4l2_subdev_get_locked_active_state(&imx219->sd);
>  	format = v4l2_subdev_state_get_format(state, 0);
> +	rate_factor = imx219_get_rate_factor(imx219);
>  
>  	if (ctrl->id == V4L2_CID_VBLANK) {
>  		int exposure_max, exposure_def;
> @@ -405,7 +447,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
>  		break;
>  	case V4L2_CID_EXPOSURE:
>  		cci_write(imx219->regmap, IMX219_REG_EXPOSURE,
> -			  ctrl->val, &ret);
> +			  ctrl->val / rate_factor, &ret);

Isn't the exposure in lines? It shouldn't be affected by the rate change,
shouldn't it?

>  		break;
>  	case V4L2_CID_DIGITAL_GAIN:
>  		cci_write(imx219->regmap, IMX219_REG_DIGITAL_GAIN,
> @@ -422,7 +464,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
>  		break;
>  	case V4L2_CID_VBLANK:
>  		cci_write(imx219->regmap, IMX219_REG_VTS,
> -			  format->height + ctrl->val, &ret);
> +			  (format->height + ctrl->val) / rate_factor, &ret);

The same for vertical blanking.

>  		break;
>  	case V4L2_CID_HBLANK:
>  		cci_write(imx219->regmap, IMX219_REG_HTS,
> @@ -463,7 +505,8 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
>  
>  static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
>  {
> -	return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE;
> +	return ((imx219->lanes == 2) ? IMX219_PIXEL_RATE :
> +		IMX219_PIXEL_RATE_4LANE) * imx219_get_rate_factor(imx219);
>  }
>  
>  /* Initialize control handlers */
> @@ -592,29 +635,12 @@ static int imx219_set_framefmt(struct imx219 *imx219,
>  {
>  	const struct v4l2_mbus_framefmt *format;
>  	const struct v4l2_rect *crop;
> -	unsigned int bpp;
> -	u64 bin_h, bin_v;
> +	u32 bpp;
>  	int ret = 0;
>  
>  	format = v4l2_subdev_state_get_format(state, 0);
>  	crop = v4l2_subdev_state_get_crop(state, 0);
> -
> -	switch (format->code) {
> -	case MEDIA_BUS_FMT_SRGGB8_1X8:
> -	case MEDIA_BUS_FMT_SGRBG8_1X8:
> -	case MEDIA_BUS_FMT_SGBRG8_1X8:
> -	case MEDIA_BUS_FMT_SBGGR8_1X8:
> -		bpp = 8;
> -		break;
> -
> -	case MEDIA_BUS_FMT_SRGGB10_1X10:
> -	case MEDIA_BUS_FMT_SGRBG10_1X10:
> -	case MEDIA_BUS_FMT_SGBRG10_1X10:
> -	case MEDIA_BUS_FMT_SBGGR10_1X10:
> -	default:
> -		bpp = 10;
> -		break;
> -	}
> +	bpp = imx219_get_format_bpp(format);
>  
>  	cci_write(imx219->regmap, IMX219_REG_X_ADD_STA_A,
>  		  crop->left - IMX219_PIXEL_ARRAY_LEFT, &ret);
> @@ -625,28 +651,8 @@ static int imx219_set_framefmt(struct imx219 *imx219,
>  	cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A,
>  		  crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret);
>  
> -	switch (crop->width / format->width) {
> -	case 1:
> -	default:
> -		bin_h = IMX219_BINNING_NONE;
> -		break;
> -	case 2:
> -		bin_h = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> -		break;
> -	}
> -
> -	switch (crop->height / format->height) {
> -	case 1:
> -	default:
> -		bin_v = IMX219_BINNING_NONE;
> -		break;
> -	case 2:
> -		bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> -		break;
> -	}
> -
> -	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret);
> -	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret);
> +	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, imx219->bin_h, &ret);
> +	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, imx219->bin_v, &ret);

Please run:

$ ./scripts/checkpatch.pl --strict --max-line-length=80

>  
>  	cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
>  		  format->width, &ret);
> @@ -851,6 +857,27 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  		int exposure_max;
>  		int exposure_def;
>  		int hblank;
> +		int pixel_rate;
> +		u32 bpp = imx219_get_format_bpp(format);
> +		enum binning_mode binning = BINNING_NONE;
> +
> +		/*
> +		 * For 8-bit formats, analog horizontal binning is required,
> +		 * else the output image is garbage.
> +		 * For 10-bit formats, analog horizontal binning is optional,
> +		 * but still useful as it doubles the effective framerate.
> +		 * We can only use it with width <= 1624, as for higher values
> +		 * there are blocky artefacts.

This comment would benefit from rewrapping.

> +		 *
> +		 * Vertical binning should match the horizontal binning mode.
> +		 */
> +		if (bin_h == 2 && (format->width <= 1624 || bpp == 8))
> +			binning = BINNING_ANALOG_X2;
> +		else
> +			binning = BINNING_X2;
> +
> +		imx219->bin_h = (bin_h == 2) ? binning : BINNING_NONE;
> +		imx219->bin_v = (bin_v == 2) ? binning : BINNING_NONE;

It'd be also nice to move the state information to sub-device state.

>  
>  		/* Update limits and set FPS to default */
>  		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> @@ -879,6 +906,11 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  					 IMX219_PPL_MAX - mode->width,
>  					 1, IMX219_PPL_MIN - mode->width);
>  		__v4l2_ctrl_s_ctrl(imx219->hblank, hblank);
> +
> +		/* Scale the pixel rate based on the mode specific factor */
> +		pixel_rate = imx219_get_pixel_rate(imx219);
> +		__v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate,
> +					 pixel_rate, 1, pixel_rate);
>  	}
>  
>  	return 0;
> 

-- 
Kind regards,

Sakari Ailus

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

[Jai Luthra]

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

Hi Sakari,

On Nov 26, 2024 at 14:36:24 +0000, Sakari Ailus wrote:
> Hi Jai,
> 
> On Mon, Nov 25, 2024 at 08:36:27PM +0530, Jai Luthra wrote:
> > When the analog binning mode is used for high framerate operation,
> > the pixel rate is effectively doubled. Account for this when setting up
> > the pixel clock rate, and applying the vblank and exposure controls.
> > 
> > The previous logic only used analog binning for 8-bit modes, but normal
> > binning limits the framerate on 10-bit 480p [1]. So with this patch we
> > switch to using special binning (with 2x pixel rate) for all formats of
> > 480p mode and 8-bit 1232p.
> > 
> > [1]: https://github.com/raspberrypi/linux/issues/5493
> > 
> > Co-developed-by: Naushir Patuck <naush@raspberrypi.com>
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Co-developed-by: Vinay Varma <varmavinaym@gmail.com>
> > Signed-off-by: Vinay Varma <varmavinaym@gmail.com>
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > ---
> >  drivers/media/i2c/imx219.c | 120 ++++++++++++++++++++++++++++-----------------
> >  1 file changed, 76 insertions(+), 44 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 970e6362d0ae3a9078daf337155e83d637bc1ca1..39b85cdee58318b080c867afd68ca33d14d3eda7 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -149,6 +149,12 @@
> >  #define IMX219_PIXEL_ARRAY_WIDTH	3280U
> >  #define IMX219_PIXEL_ARRAY_HEIGHT	2464U
> >  
> > +enum binning_mode {
> > +	BINNING_NONE = IMX219_BINNING_NONE,
> > +	BINNING_X2 = IMX219_BINNING_X2,
> > +	BINNING_ANALOG_X2 = IMX219_BINNING_X2_ANALOG,
> > +};
> > +
> >  /* Mode : resolution and related config&values */
> >  struct imx219_mode {
> >  	/* Frame width */
> > @@ -337,6 +343,10 @@ struct imx219 {
> >  
> >  	/* Two or Four lanes */
> >  	u8 lanes;
> > +
> > +	/* Binning mode */
> > +	enum binning_mode bin_h;
> > +	enum binning_mode bin_v;
> >  };
> >  
> >  static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> > @@ -362,6 +372,36 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
> >  	return imx219_mbus_formats[i];
> >  }
> >  
> > +static u32 imx219_get_format_bpp(const struct v4l2_mbus_framefmt *format)
> > +{
> > +	switch (format->code) {
> > +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> > +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> > +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> > +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> > +		return 8;
> > +
> > +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> > +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> > +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> > +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> > +	default:
> > +		return 10;
> > +	}
> > +}
> > +
> > +static int imx219_get_rate_factor(struct imx219 *imx219)
> > +{
> > +	switch (imx219->bin_v) {
> > +	case BINNING_NONE:
> > +	case BINNING_X2:
> > +		return 1;
> > +	case BINNING_ANALOG_X2:
> > +		return 2;
> 
> FWIW, what the CCS driver does is that it exposes different horizontal
> blanking ranges for devices that use analogue binning. The rate is really
> about reading pixels and with analogue binning the rate is the same, it's
> just that fewer pixels are being (digitally) read (as they are binned). I
> wonder if this would be a workable approach for this sensor, too. Of course
> if the LLP behaves differently for this sensor, then we should probably
> just accept that.
> 

IMX219 seems to be odd in this case, as the LLP doesn't change during 
analog binning. Shared some more details in this thread:

https://lore.kernel.org/linux-media/20241125-imx219_fixes-v3-0-434fc0b541c8@ideasonboard.com/T/#m1da4206e91db12b8e377dc686935195fc5f4bb68

> > +	}
> > +	return -EINVAL;
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Controls
> >   */
> > @@ -373,10 +413,12 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> >  	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> >  	const struct v4l2_mbus_framefmt *format;
> >  	struct v4l2_subdev_state *state;
> > +	int rate_factor;
> 
> u32?
> 

Fixed.

> >  	int ret = 0;
> >  
> >  	state = v4l2_subdev_get_locked_active_state(&imx219->sd);
> >  	format = v4l2_subdev_state_get_format(state, 0);
> > +	rate_factor = imx219_get_rate_factor(imx219);
> >  
> >  	if (ctrl->id == V4L2_CID_VBLANK) {
> >  		int exposure_max, exposure_def;
> > @@ -405,7 +447,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> >  		break;
> >  	case V4L2_CID_EXPOSURE:
> >  		cci_write(imx219->regmap, IMX219_REG_EXPOSURE,
> > -			  ctrl->val, &ret);
> > +			  ctrl->val / rate_factor, &ret);
> 
> Isn't the exposure in lines? It shouldn't be affected by the rate change,
> shouldn't it?
> 

From the sensor datasheet the unit of FRAME_LENGTH register is updated 
to 2xLines when analog binning is used. And exposure and vertical 
blanking values are also in units of FRAME_LENGTH. This is also 
consistent with the behavior seen while testing.

> >  		break;
> >  	case V4L2_CID_DIGITAL_GAIN:
> >  		cci_write(imx219->regmap, IMX219_REG_DIGITAL_GAIN,
> > @@ -422,7 +464,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> >  		break;
> >  	case V4L2_CID_VBLANK:
> >  		cci_write(imx219->regmap, IMX219_REG_VTS,
> > -			  format->height + ctrl->val, &ret);
> > +			  (format->height + ctrl->val) / rate_factor, &ret);
> 
> The same for vertical blanking.
> 
> >  		break;
> >  	case V4L2_CID_HBLANK:
> >  		cci_write(imx219->regmap, IMX219_REG_HTS,
> > @@ -463,7 +505,8 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
> >  
> >  static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
> >  {
> > -	return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE;
> > +	return ((imx219->lanes == 2) ? IMX219_PIXEL_RATE :
> > +		IMX219_PIXEL_RATE_4LANE) * imx219_get_rate_factor(imx219);
> >  }
> >  
> >  /* Initialize control handlers */
> > @@ -592,29 +635,12 @@ static int imx219_set_framefmt(struct imx219 *imx219,
> >  {
> >  	const struct v4l2_mbus_framefmt *format;
> >  	const struct v4l2_rect *crop;
> > -	unsigned int bpp;
> > -	u64 bin_h, bin_v;
> > +	u32 bpp;
> >  	int ret = 0;
> >  
> >  	format = v4l2_subdev_state_get_format(state, 0);
> >  	crop = v4l2_subdev_state_get_crop(state, 0);
> > -
> > -	switch (format->code) {
> > -	case MEDIA_BUS_FMT_SRGGB8_1X8:
> > -	case MEDIA_BUS_FMT_SGRBG8_1X8:
> > -	case MEDIA_BUS_FMT_SGBRG8_1X8:
> > -	case MEDIA_BUS_FMT_SBGGR8_1X8:
> > -		bpp = 8;
> > -		break;
> > -
> > -	case MEDIA_BUS_FMT_SRGGB10_1X10:
> > -	case MEDIA_BUS_FMT_SGRBG10_1X10:
> > -	case MEDIA_BUS_FMT_SGBRG10_1X10:
> > -	case MEDIA_BUS_FMT_SBGGR10_1X10:
> > -	default:
> > -		bpp = 10;
> > -		break;
> > -	}
> > +	bpp = imx219_get_format_bpp(format);
> >  
> >  	cci_write(imx219->regmap, IMX219_REG_X_ADD_STA_A,
> >  		  crop->left - IMX219_PIXEL_ARRAY_LEFT, &ret);
> > @@ -625,28 +651,8 @@ static int imx219_set_framefmt(struct imx219 *imx219,
> >  	cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A,
> >  		  crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret);
> >  
> > -	switch (crop->width / format->width) {
> > -	case 1:
> > -	default:
> > -		bin_h = IMX219_BINNING_NONE;
> > -		break;
> > -	case 2:
> > -		bin_h = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> > -		break;
> > -	}
> > -
> > -	switch (crop->height / format->height) {
> > -	case 1:
> > -	default:
> > -		bin_v = IMX219_BINNING_NONE;
> > -		break;
> > -	case 2:
> > -		bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> > -		break;
> > -	}
> > -
> > -	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret);
> > -	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret);
> > +	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, imx219->bin_h, &ret);
> > +	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, imx219->bin_v, &ret);
> 
> Please run:
> 
> $ ./scripts/checkpatch.pl --strict --max-line-length=80
> 

Oops, fixed in next revision.

> >  
> >  	cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
> >  		  format->width, &ret);
> > @@ -851,6 +857,27 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >  		int exposure_max;
> >  		int exposure_def;
> >  		int hblank;
> > +		int pixel_rate;
> > +		u32 bpp = imx219_get_format_bpp(format);
> > +		enum binning_mode binning = BINNING_NONE;
> > +
> > +		/*
> > +		 * For 8-bit formats, analog horizontal binning is required,
> > +		 * else the output image is garbage.
> > +		 * For 10-bit formats, analog horizontal binning is optional,
> > +		 * but still useful as it doubles the effective framerate.
> > +		 * We can only use it with width <= 1624, as for higher values
> > +		 * there are blocky artefacts.
> 
> This comment would benefit from rewrapping.
> 

Fixed.

> > +		 *
> > +		 * Vertical binning should match the horizontal binning mode.
> > +		 */
> > +		if (bin_h == 2 && (format->width <= 1624 || bpp == 8))
> > +			binning = BINNING_ANALOG_X2;
> > +		else
> > +			binning = BINNING_X2;
> > +
> > +		imx219->bin_h = (bin_h == 2) ? binning : BINNING_NONE;
> > +		imx219->bin_v = (bin_v == 2) ? binning : BINNING_NONE;
> 
> It'd be also nice to move the state information to sub-device state.
> 

I'm not sure I follow, do you mean the framework should store the 
binning mode, similar to how crop rectangle and interval are stored in 
v4l2_subdev_state?

> >  
> >  		/* Update limits and set FPS to default */
> >  		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > @@ -879,6 +906,11 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >  					 IMX219_PPL_MAX - mode->width,
> >  					 1, IMX219_PPL_MIN - mode->width);
> >  		__v4l2_ctrl_s_ctrl(imx219->hblank, hblank);
> > +
> > +		/* Scale the pixel rate based on the mode specific factor */
> > +		pixel_rate = imx219_get_pixel_rate(imx219);
> > +		__v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate,
> > +					 pixel_rate, 1, pixel_rate);
> >  	}
> >  
> >  	return 0;
> > 
> 
> -- 
> Kind regards,
> 
> Sakari Ailus

Thanks,
Jai

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

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

[Sakari Ailus]

Hi Jai,

On Wed, Nov 27, 2024 at 03:07:14PM +0530, Jai Luthra wrote:
> Hi Sakari,
> 
> On Nov 26, 2024 at 14:36:24 +0000, Sakari Ailus wrote:
> > Hi Jai,
> > 
> > On Mon, Nov 25, 2024 at 08:36:27PM +0530, Jai Luthra wrote:
> > > When the analog binning mode is used for high framerate operation,
> > > the pixel rate is effectively doubled. Account for this when setting up
> > > the pixel clock rate, and applying the vblank and exposure controls.
> > > 
> > > The previous logic only used analog binning for 8-bit modes, but normal
> > > binning limits the framerate on 10-bit 480p [1]. So with this patch we
> > > switch to using special binning (with 2x pixel rate) for all formats of
> > > 480p mode and 8-bit 1232p.
> > > 
> > > [1]: https://github.com/raspberrypi/linux/issues/5493
> > > 
> > > Co-developed-by: Naushir Patuck <naush@raspberrypi.com>
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > Co-developed-by: Vinay Varma <varmavinaym@gmail.com>
> > > Signed-off-by: Vinay Varma <varmavinaym@gmail.com>
> > > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/imx219.c | 120 ++++++++++++++++++++++++++++-----------------
> > >  1 file changed, 76 insertions(+), 44 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > index 970e6362d0ae3a9078daf337155e83d637bc1ca1..39b85cdee58318b080c867afd68ca33d14d3eda7 100644
> > > --- a/drivers/media/i2c/imx219.c
> > > +++ b/drivers/media/i2c/imx219.c
> > > @@ -149,6 +149,12 @@
> > >  #define IMX219_PIXEL_ARRAY_WIDTH	3280U
> > >  #define IMX219_PIXEL_ARRAY_HEIGHT	2464U
> > >  
> > > +enum binning_mode {
> > > +	BINNING_NONE = IMX219_BINNING_NONE,
> > > +	BINNING_X2 = IMX219_BINNING_X2,
> > > +	BINNING_ANALOG_X2 = IMX219_BINNING_X2_ANALOG,
> > > +};
> > > +
> > >  /* Mode : resolution and related config&values */
> > >  struct imx219_mode {
> > >  	/* Frame width */
> > > @@ -337,6 +343,10 @@ struct imx219 {
> > >  
> > >  	/* Two or Four lanes */
> > >  	u8 lanes;
> > > +
> > > +	/* Binning mode */
> > > +	enum binning_mode bin_h;
> > > +	enum binning_mode bin_v;
> > >  };
> > >  
> > >  static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> > > @@ -362,6 +372,36 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
> > >  	return imx219_mbus_formats[i];
> > >  }
> > >  
> > > +static u32 imx219_get_format_bpp(const struct v4l2_mbus_framefmt *format)
> > > +{
> > > +	switch (format->code) {
> > > +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> > > +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> > > +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> > > +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> > > +		return 8;
> > > +
> > > +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> > > +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> > > +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> > > +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> > > +	default:
> > > +		return 10;
> > > +	}
> > > +}
> > > +
> > > +static int imx219_get_rate_factor(struct imx219 *imx219)
> > > +{
> > > +	switch (imx219->bin_v) {
> > > +	case BINNING_NONE:
> > > +	case BINNING_X2:
> > > +		return 1;
> > > +	case BINNING_ANALOG_X2:
> > > +		return 2;
> > 
> > FWIW, what the CCS driver does is that it exposes different horizontal
> > blanking ranges for devices that use analogue binning. The rate is really
> > about reading pixels and with analogue binning the rate is the same, it's
> > just that fewer pixels are being (digitally) read (as they are binned). I
> > wonder if this would be a workable approach for this sensor, too. Of course
> > if the LLP behaves differently for this sensor, then we should probably
> > just accept that.
> > 
> 
> IMX219 seems to be odd in this case, as the LLP doesn't change during 
> analog binning. Shared some more details in this thread:
> 
> https://lore.kernel.org/linux-media/20241125-imx219_fixes-v3-0-434fc0b541c8@ideasonboard.com/T/#m1da4206e91db12b8e377dc686935195fc5f4bb68

Ack.

> 
> > > +	}
> > > +	return -EINVAL;
> > > +}
> > > +
> > >  /* -----------------------------------------------------------------------------
> > >   * Controls
> > >   */
> > > @@ -373,10 +413,12 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> > >  	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > >  	const struct v4l2_mbus_framefmt *format;
> > >  	struct v4l2_subdev_state *state;
> > > +	int rate_factor;
> > 
> > u32?
> > 
> 
> Fixed.
> 
> > >  	int ret = 0;
> > >  
> > >  	state = v4l2_subdev_get_locked_active_state(&imx219->sd);
> > >  	format = v4l2_subdev_state_get_format(state, 0);
> > > +	rate_factor = imx219_get_rate_factor(imx219);
> > >  
> > >  	if (ctrl->id == V4L2_CID_VBLANK) {
> > >  		int exposure_max, exposure_def;
> > > @@ -405,7 +447,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> > >  		break;
> > >  	case V4L2_CID_EXPOSURE:
> > >  		cci_write(imx219->regmap, IMX219_REG_EXPOSURE,
> > > -			  ctrl->val, &ret);
> > > +			  ctrl->val / rate_factor, &ret);
> > 
> > Isn't the exposure in lines? It shouldn't be affected by the rate change,
> > shouldn't it?
> > 
> 
> From the sensor datasheet the unit of FRAME_LENGTH register is updated 
> to 2xLines when analog binning is used. And exposure and vertical 
> blanking values are also in units of FRAME_LENGTH. This is also 
> consistent with the behavior seen while testing.
> 
> > >  		break;
> > >  	case V4L2_CID_DIGITAL_GAIN:
> > >  		cci_write(imx219->regmap, IMX219_REG_DIGITAL_GAIN,
> > > @@ -422,7 +464,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> > >  		break;
> > >  	case V4L2_CID_VBLANK:
> > >  		cci_write(imx219->regmap, IMX219_REG_VTS,
> > > -			  format->height + ctrl->val, &ret);
> > > +			  (format->height + ctrl->val) / rate_factor, &ret);
> > 
> > The same for vertical blanking.
> > 
> > >  		break;
> > >  	case V4L2_CID_HBLANK:
> > >  		cci_write(imx219->regmap, IMX219_REG_HTS,
> > > @@ -463,7 +505,8 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
> > >  
> > >  static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
> > >  {
> > > -	return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE;
> > > +	return ((imx219->lanes == 2) ? IMX219_PIXEL_RATE :
> > > +		IMX219_PIXEL_RATE_4LANE) * imx219_get_rate_factor(imx219);
> > >  }
> > >  
> > >  /* Initialize control handlers */
> > > @@ -592,29 +635,12 @@ static int imx219_set_framefmt(struct imx219 *imx219,
> > >  {
> > >  	const struct v4l2_mbus_framefmt *format;
> > >  	const struct v4l2_rect *crop;
> > > -	unsigned int bpp;
> > > -	u64 bin_h, bin_v;
> > > +	u32 bpp;
> > >  	int ret = 0;
> > >  
> > >  	format = v4l2_subdev_state_get_format(state, 0);
> > >  	crop = v4l2_subdev_state_get_crop(state, 0);
> > > -
> > > -	switch (format->code) {
> > > -	case MEDIA_BUS_FMT_SRGGB8_1X8:
> > > -	case MEDIA_BUS_FMT_SGRBG8_1X8:
> > > -	case MEDIA_BUS_FMT_SGBRG8_1X8:
> > > -	case MEDIA_BUS_FMT_SBGGR8_1X8:
> > > -		bpp = 8;
> > > -		break;
> > > -
> > > -	case MEDIA_BUS_FMT_SRGGB10_1X10:
> > > -	case MEDIA_BUS_FMT_SGRBG10_1X10:
> > > -	case MEDIA_BUS_FMT_SGBRG10_1X10:
> > > -	case MEDIA_BUS_FMT_SBGGR10_1X10:
> > > -	default:
> > > -		bpp = 10;
> > > -		break;
> > > -	}
> > > +	bpp = imx219_get_format_bpp(format);
> > >  
> > >  	cci_write(imx219->regmap, IMX219_REG_X_ADD_STA_A,
> > >  		  crop->left - IMX219_PIXEL_ARRAY_LEFT, &ret);
> > > @@ -625,28 +651,8 @@ static int imx219_set_framefmt(struct imx219 *imx219,
> > >  	cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A,
> > >  		  crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret);
> > >  
> > > -	switch (crop->width / format->width) {
> > > -	case 1:
> > > -	default:
> > > -		bin_h = IMX219_BINNING_NONE;
> > > -		break;
> > > -	case 2:
> > > -		bin_h = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> > > -		break;
> > > -	}
> > > -
> > > -	switch (crop->height / format->height) {
> > > -	case 1:
> > > -	default:
> > > -		bin_v = IMX219_BINNING_NONE;
> > > -		break;
> > > -	case 2:
> > > -		bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> > > -		break;
> > > -	}
> > > -
> > > -	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret);
> > > -	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret);
> > > +	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, imx219->bin_h, &ret);
> > > +	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, imx219->bin_v, &ret);
> > 
> > Please run:
> > 
> > $ ./scripts/checkpatch.pl --strict --max-line-length=80
> > 
> 
> Oops, fixed in next revision.
> 
> > >  
> > >  	cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
> > >  		  format->width, &ret);
> > > @@ -851,6 +857,27 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > >  		int exposure_max;
> > >  		int exposure_def;
> > >  		int hblank;
> > > +		int pixel_rate;
> > > +		u32 bpp = imx219_get_format_bpp(format);
> > > +		enum binning_mode binning = BINNING_NONE;
> > > +
> > > +		/*
> > > +		 * For 8-bit formats, analog horizontal binning is required,
> > > +		 * else the output image is garbage.
> > > +		 * For 10-bit formats, analog horizontal binning is optional,
> > > +		 * but still useful as it doubles the effective framerate.
> > > +		 * We can only use it with width <= 1624, as for higher values
> > > +		 * there are blocky artefacts.
> > 
> > This comment would benefit from rewrapping.
> > 
> 
> Fixed.
> 
> > > +		 *
> > > +		 * Vertical binning should match the horizontal binning mode.
> > > +		 */
> > > +		if (bin_h == 2 && (format->width <= 1624 || bpp == 8))
> > > +			binning = BINNING_ANALOG_X2;
> > > +		else
> > > +			binning = BINNING_X2;
> > > +
> > > +		imx219->bin_h = (bin_h == 2) ? binning : BINNING_NONE;
> > > +		imx219->bin_v = (bin_v == 2) ? binning : BINNING_NONE;
> > 
> > It'd be also nice to move the state information to sub-device state.
> > 
> 
> I'm not sure I follow, do you mean the framework should store the 
> binning mode, similar to how crop rectangle and interval are stored in 
> v4l2_subdev_state?

Yes, please. This is done to the CCS driver as part of the metadata set
(which we can hopefully merge in not too distant future)
<URL:https://git.retiisi.eu/?p=~sailus/linux.git;a=shortlog;h=refs/heads/metadata>.

-- 
Kind regards,

Sakari Ailus

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

[Laurent Pinchart]

On Tue, Dec 10, 2024 at 01:32:41PM +0000, Sakari Ailus wrote:
> On Wed, Nov 27, 2024 at 03:07:14PM +0530, Jai Luthra wrote:
> > On Nov 26, 2024 at 14:36:24 +0000, Sakari Ailus wrote:
> > > On Mon, Nov 25, 2024 at 08:36:27PM +0530, Jai Luthra wrote:
> > > > When the analog binning mode is used for high framerate operation,
> > > > the pixel rate is effectively doubled. Account for this when setting up
> > > > the pixel clock rate, and applying the vblank and exposure controls.
> > > > 
> > > > The previous logic only used analog binning for 8-bit modes, but normal
> > > > binning limits the framerate on 10-bit 480p [1]. So with this patch we
> > > > switch to using special binning (with 2x pixel rate) for all formats of
> > > > 480p mode and 8-bit 1232p.
> > > > 
> > > > [1]: https://github.com/raspberrypi/linux/issues/5493
> > > > 
> > > > Co-developed-by: Naushir Patuck <naush@raspberrypi.com>
> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > Co-developed-by: Vinay Varma <varmavinaym@gmail.com>
> > > > Signed-off-by: Vinay Varma <varmavinaym@gmail.com>
> > > > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > > > ---
> > > >  drivers/media/i2c/imx219.c | 120 ++++++++++++++++++++++++++++-----------------
> > > >  1 file changed, 76 insertions(+), 44 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > index 970e6362d0ae3a9078daf337155e83d637bc1ca1..39b85cdee58318b080c867afd68ca33d14d3eda7 100644
> > > > --- a/drivers/media/i2c/imx219.c
> > > > +++ b/drivers/media/i2c/imx219.c
> > > > @@ -149,6 +149,12 @@
> > > >  #define IMX219_PIXEL_ARRAY_WIDTH	3280U
> > > >  #define IMX219_PIXEL_ARRAY_HEIGHT	2464U
> > > >  
> > > > +enum binning_mode {
> > > > +	BINNING_NONE = IMX219_BINNING_NONE,
> > > > +	BINNING_X2 = IMX219_BINNING_X2,
> > > > +	BINNING_ANALOG_X2 = IMX219_BINNING_X2_ANALOG,
> > > > +};
> > > > +
> > > >  /* Mode : resolution and related config&values */
> > > >  struct imx219_mode {
> > > >  	/* Frame width */
> > > > @@ -337,6 +343,10 @@ struct imx219 {
> > > >  
> > > >  	/* Two or Four lanes */
> > > >  	u8 lanes;
> > > > +
> > > > +	/* Binning mode */
> > > > +	enum binning_mode bin_h;
> > > > +	enum binning_mode bin_v;
> > > >  };
> > > >  
> > > >  static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> > > > @@ -362,6 +372,36 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
> > > >  	return imx219_mbus_formats[i];
> > > >  }
> > > >  
> > > > +static u32 imx219_get_format_bpp(const struct v4l2_mbus_framefmt *format)
> > > > +{
> > > > +	switch (format->code) {
> > > > +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> > > > +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> > > > +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> > > > +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> > > > +		return 8;
> > > > +
> > > > +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> > > > +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> > > > +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> > > > +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> > > > +	default:
> > > > +		return 10;
> > > > +	}
> > > > +}
> > > > +
> > > > +static int imx219_get_rate_factor(struct imx219 *imx219)
> > > > +{
> > > > +	switch (imx219->bin_v) {
> > > > +	case BINNING_NONE:
> > > > +	case BINNING_X2:
> > > > +		return 1;
> > > > +	case BINNING_ANALOG_X2:
> > > > +		return 2;
> > > 
> > > FWIW, what the CCS driver does is that it exposes different horizontal
> > > blanking ranges for devices that use analogue binning. The rate is really
> > > about reading pixels and with analogue binning the rate is the same, it's
> > > just that fewer pixels are being (digitally) read (as they are binned). I
> > > wonder if this would be a workable approach for this sensor, too. Of course
> > > if the LLP behaves differently for this sensor, then we should probably
> > > just accept that.
> > 
> > IMX219 seems to be odd in this case, as the LLP doesn't change during 
> > analog binning. Shared some more details in this thread:
> > 
> > https://lore.kernel.org/linux-media/20241125-imx219_fixes-v3-0-434fc0b541c8@ideasonboard.com/T/#m1da4206e91db12b8e377dc686935195fc5f4bb68
> 
> Ack.
> 
> > > > +	}
> > > > +	return -EINVAL;
> > > > +}
> > > > +
> > > >  /* -----------------------------------------------------------------------------
> > > >   * Controls
> > > >   */
> > > > @@ -373,10 +413,12 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> > > >  	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > > >  	const struct v4l2_mbus_framefmt *format;
> > > >  	struct v4l2_subdev_state *state;
> > > > +	int rate_factor;
> > > 
> > > u32?
> > > 
> > 
> > Fixed.
> > 
> > > >  	int ret = 0;
> > > >  
> > > >  	state = v4l2_subdev_get_locked_active_state(&imx219->sd);
> > > >  	format = v4l2_subdev_state_get_format(state, 0);
> > > > +	rate_factor = imx219_get_rate_factor(imx219);
> > > >  
> > > >  	if (ctrl->id == V4L2_CID_VBLANK) {
> > > >  		int exposure_max, exposure_def;
> > > > @@ -405,7 +447,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> > > >  		break;
> > > >  	case V4L2_CID_EXPOSURE:
> > > >  		cci_write(imx219->regmap, IMX219_REG_EXPOSURE,
> > > > -			  ctrl->val, &ret);
> > > > +			  ctrl->val / rate_factor, &ret);
> > > 
> > > Isn't the exposure in lines? It shouldn't be affected by the rate change,
> > > shouldn't it?
> > > 
> > 
> > From the sensor datasheet the unit of FRAME_LENGTH register is updated 
> > to 2xLines when analog binning is used. And exposure and vertical 
> > blanking values are also in units of FRAME_LENGTH. This is also 
> > consistent with the behavior seen while testing.
> > 
> > > >  		break;
> > > >  	case V4L2_CID_DIGITAL_GAIN:
> > > >  		cci_write(imx219->regmap, IMX219_REG_DIGITAL_GAIN,
> > > > @@ -422,7 +464,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> > > >  		break;
> > > >  	case V4L2_CID_VBLANK:
> > > >  		cci_write(imx219->regmap, IMX219_REG_VTS,
> > > > -			  format->height + ctrl->val, &ret);
> > > > +			  (format->height + ctrl->val) / rate_factor, &ret);
> > > 
> > > The same for vertical blanking.
> > > 
> > > >  		break;
> > > >  	case V4L2_CID_HBLANK:
> > > >  		cci_write(imx219->regmap, IMX219_REG_HTS,
> > > > @@ -463,7 +505,8 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
> > > >  
> > > >  static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
> > > >  {
> > > > -	return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE;
> > > > +	return ((imx219->lanes == 2) ? IMX219_PIXEL_RATE :
> > > > +		IMX219_PIXEL_RATE_4LANE) * imx219_get_rate_factor(imx219);
> > > >  }
> > > >  
> > > >  /* Initialize control handlers */
> > > > @@ -592,29 +635,12 @@ static int imx219_set_framefmt(struct imx219 *imx219,
> > > >  {
> > > >  	const struct v4l2_mbus_framefmt *format;
> > > >  	const struct v4l2_rect *crop;
> > > > -	unsigned int bpp;
> > > > -	u64 bin_h, bin_v;
> > > > +	u32 bpp;
> > > >  	int ret = 0;
> > > >  
> > > >  	format = v4l2_subdev_state_get_format(state, 0);
> > > >  	crop = v4l2_subdev_state_get_crop(state, 0);
> > > > -
> > > > -	switch (format->code) {
> > > > -	case MEDIA_BUS_FMT_SRGGB8_1X8:
> > > > -	case MEDIA_BUS_FMT_SGRBG8_1X8:
> > > > -	case MEDIA_BUS_FMT_SGBRG8_1X8:
> > > > -	case MEDIA_BUS_FMT_SBGGR8_1X8:
> > > > -		bpp = 8;
> > > > -		break;
> > > > -
> > > > -	case MEDIA_BUS_FMT_SRGGB10_1X10:
> > > > -	case MEDIA_BUS_FMT_SGRBG10_1X10:
> > > > -	case MEDIA_BUS_FMT_SGBRG10_1X10:
> > > > -	case MEDIA_BUS_FMT_SBGGR10_1X10:
> > > > -	default:
> > > > -		bpp = 10;
> > > > -		break;
> > > > -	}
> > > > +	bpp = imx219_get_format_bpp(format);
> > > >  
> > > >  	cci_write(imx219->regmap, IMX219_REG_X_ADD_STA_A,
> > > >  		  crop->left - IMX219_PIXEL_ARRAY_LEFT, &ret);
> > > > @@ -625,28 +651,8 @@ static int imx219_set_framefmt(struct imx219 *imx219,
> > > >  	cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A,
> > > >  		  crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret);
> > > >  
> > > > -	switch (crop->width / format->width) {
> > > > -	case 1:
> > > > -	default:
> > > > -		bin_h = IMX219_BINNING_NONE;
> > > > -		break;
> > > > -	case 2:
> > > > -		bin_h = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> > > > -		break;
> > > > -	}
> > > > -
> > > > -	switch (crop->height / format->height) {
> > > > -	case 1:
> > > > -	default:
> > > > -		bin_v = IMX219_BINNING_NONE;
> > > > -		break;
> > > > -	case 2:
> > > > -		bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> > > > -		break;
> > > > -	}
> > > > -
> > > > -	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret);
> > > > -	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret);
> > > > +	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, imx219->bin_h, &ret);
> > > > +	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, imx219->bin_v, &ret);
> > > 
> > > Please run:
> > > 
> > > $ ./scripts/checkpatch.pl --strict --max-line-length=80
> > > 
> > 
> > Oops, fixed in next revision.
> > 
> > > >  
> > > >  	cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
> > > >  		  format->width, &ret);
> > > > @@ -851,6 +857,27 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > > >  		int exposure_max;
> > > >  		int exposure_def;
> > > >  		int hblank;
> > > > +		int pixel_rate;
> > > > +		u32 bpp = imx219_get_format_bpp(format);
> > > > +		enum binning_mode binning = BINNING_NONE;
> > > > +
> > > > +		/*
> > > > +		 * For 8-bit formats, analog horizontal binning is required,
> > > > +		 * else the output image is garbage.
> > > > +		 * For 10-bit formats, analog horizontal binning is optional,
> > > > +		 * but still useful as it doubles the effective framerate.
> > > > +		 * We can only use it with width <= 1624, as for higher values
> > > > +		 * there are blocky artefacts.
> > > 
> > > This comment would benefit from rewrapping.
> > > 
> > 
> > Fixed.
> > 
> > > > +		 *
> > > > +		 * Vertical binning should match the horizontal binning mode.
> > > > +		 */
> > > > +		if (bin_h == 2 && (format->width <= 1624 || bpp == 8))
> > > > +			binning = BINNING_ANALOG_X2;
> > > > +		else
> > > > +			binning = BINNING_X2;
> > > > +
> > > > +		imx219->bin_h = (bin_h == 2) ? binning : BINNING_NONE;
> > > > +		imx219->bin_v = (bin_v == 2) ? binning : BINNING_NONE;
> > > 
> > > It'd be also nice to move the state information to sub-device state.
> > 
> > I'm not sure I follow, do you mean the framework should store the 
> > binning mode, similar to how crop rectangle and interval are stored in 
> > v4l2_subdev_state?

Not the binning mode (or binning factor) as such, but v4l2_subdev_state
stores the format and selection rectangles, and the binning factors
should then be computed at stream on time. The imx219 structure should
not store any state data that is not related to the current hardware
state.

> Yes, please. This is done to the CCS driver as part of the metadata set
> (which we can hopefully merge in not too distant future)
> <URL:https://git.retiisi.eu/?p=~sailus/linux.git;a=shortlog;h=refs/heads/metadata>.

-- 
Regards,

Laurent Pinchart