Message-ID: <umnnd72lotwgm46bv43bn3vdvcuqxf3idurp63hbilosx3gs4o@gp63z27qtlxp>
Date: Wed, 27 Nov 2024 15:07:14 +0530
From: Jai Luthra <jai.luthra@...asonboard.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: Dave Stevenson <dave.stevenson@...pberrypi.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Jacopo Mondi <jacopo.mondi@...asonboard.com>, Laurent Pinchart <laurent.pinchart@...asonboard.com>, 
	Naushir Patuck <naush@...pberrypi.com>, Vinay Varma <varmavinaym@...il.com>
Subject: Re: [PATCH v3 3/3] media: i2c: imx219: Scale the pixel rate for
 analog binning

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@...pberrypi.com>
> > Signed-off-by: Naushir Patuck <naush@...pberrypi.com>
> > Co-developed-by: Vinay Varma <varmavinaym@...il.com>
> > Signed-off-by: Vinay Varma <varmavinaym@...il.com>
> > Signed-off-by: Jai Luthra <jai.luthra@...asonboard.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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists