lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <zhr3qtslhjnfnt7eora2vh77luspbsu33r2dxxvuodrdsljbdc@dml43w33fhmu>
Date: Mon, 4 Nov 2024 10:33:14 +0100
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Jai Luthra <jai.luthra@...asonboard.com>
Cc: Dave Stevenson <dave.stevenson@...pberrypi.com>, 
	Sakari Ailus <sakari.ailus@...ux.intel.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, 
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Naushir Patuck <naush@...pberrypi.com>, Vinay Varma <varmavinaym@...il.com>
Subject: Re: [PATCH 3/3] media: i2c: imx219: Scale the pixel rate for analog
 binning

Hello

On Tue, Oct 29, 2024 at 02:27:37PM +0530, Jai Luthra wrote:
> When the analog binning mode is used for high framerate operation,
> the pixel rate is effectively doubled. Account for this when setting up
> the pixel clock rate, and applying the vblank and exposure controls.
>
> The previous logic only used analog binning for 8-bit modes, but normal
> binning limits the framerate on 10-bit 480p [1]. So with this patch we
> switch to using special binning (with 2x pixel rate) for all formats of
> 480p mode and 8-bit 1232p.
>
> To do this cleanly, re-introduce the book-keeping for which binning mode
> is used with which resolution/format.
>
> [1]: https://github.com/raspberrypi/linux/issues/5493
>
> Co-developed-by: Naushir Patuck <naush@...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 | 149 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 106 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index de9230d4ad81f085640be254db9391ae7ad20773..140d958f80eb57dfb4ecf1796fcdf77081a662d7 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -149,6 +149,18 @@
>  #define IMX219_PIXEL_ARRAY_WIDTH	3280U
>  #define IMX219_PIXEL_ARRAY_HEIGHT	2464U
>
> +enum binning_mode {
> +	BINNING_NONE,
> +	BINNING_X2,
> +	BINNING_ANALOG_X2,

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

> +};
> +
> +enum binning_bit_depths {
> +	BINNING_IDX_8_BIT,
> +	BINNING_IDX_10_BIT,
> +	BINNING_IDX_MAX
> +};
> +
>  /* Mode : resolution and related config&values */
>  struct imx219_mode {
>  	/* Frame width */
> @@ -158,6 +170,9 @@ struct imx219_mode {
>
>  	/* V-timing */
>  	unsigned int vts_def;
> +
> +	/* binning mode based on format code */
> +	enum binning_mode binning[BINNING_IDX_MAX];
>  };
>
>  static const struct cci_reg_sequence imx219_common_regs[] = {
> @@ -293,24 +308,40 @@ static const struct imx219_mode supported_modes[] = {
>  		.width = 3280,
>  		.height = 2464,
>  		.vts_def = 3526,
> +		.binning = {
> +			[BINNING_IDX_8_BIT] = BINNING_NONE,
> +			[BINNING_IDX_10_BIT] = BINNING_NONE,
> +		},
>  	},
>  	{
>  		/* 1080P 30fps cropped */
>  		.width = 1920,
>  		.height = 1080,
>  		.vts_def = 1763,
> +		.binning = {
> +			[BINNING_IDX_8_BIT] = BINNING_NONE,
> +			[BINNING_IDX_10_BIT] = BINNING_NONE,
> +		},
>  	},
>  	{
>  		/* 2x2 binned 30fps mode */
>  		.width = 1640,
>  		.height = 1232,
>  		.vts_def = 1763,
> +		.binning = {
> +			[BINNING_IDX_8_BIT] = BINNING_ANALOG_X2,
> +			[BINNING_IDX_10_BIT] = BINNING_X2,
> +		},
>  	},
>  	{
>  		/* 640x480 30fps mode */
>  		.width = 640,
>  		.height = 480,
>  		.vts_def = 1763,
> +		.binning = {
> +			[BINNING_IDX_8_BIT] = BINNING_ANALOG_X2,
> +			[BINNING_IDX_10_BIT] = BINNING_ANALOG_X2,
> +		},
>  	},
>  };
>
> @@ -337,6 +368,9 @@ struct imx219 {
>
>  	/* Two or Four lanes */
>  	u8 lanes;
> +
> +	/* Binning mode */
> +	enum binning_mode binning;
>  };
>
>  static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
> @@ -362,6 +396,36 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
>  	return imx219_mbus_formats[i];
>  }
>
> +static u32 imx219_get_format_bpp(const struct v4l2_mbus_framefmt *format)
> +{
> +	switch (format->code) {
> +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> +		return 8;
> +
> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> +	default:
> +		return 10;
> +	}
> +}
> +
> +static int imx219_get_rate_factor(struct imx219 *imx219)
> +{
> +	switch (imx219->binning) {
> +	case BINNING_NONE:
> +	case BINNING_X2:
> +		return 1;
> +	case BINNING_ANALOG_X2:
> +		return 2;
> +	}
> +	return -EINVAL;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Controls
>   */
> @@ -373,10 +437,12 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
>  	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
>  	const struct v4l2_mbus_framefmt *format;
>  	struct v4l2_subdev_state *state;
> +	int rate_factor;
>  	int ret = 0;
>
>  	state = v4l2_subdev_get_locked_active_state(&imx219->sd);
>  	format = v4l2_subdev_state_get_format(state, 0);
> +	rate_factor = imx219_get_rate_factor(imx219);
>
>  	if (ctrl->id == V4L2_CID_VBLANK) {
>  		int exposure_max, exposure_def;
> @@ -405,7 +471,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
>  		break;
>  	case V4L2_CID_EXPOSURE:
>  		cci_write(imx219->regmap, IMX219_REG_EXPOSURE,
> -			  ctrl->val, &ret);
> +			  ctrl->val / rate_factor, &ret);
>  		break;
>  	case V4L2_CID_DIGITAL_GAIN:
>  		cci_write(imx219->regmap, IMX219_REG_DIGITAL_GAIN,
> @@ -422,7 +488,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
>  		break;
>  	case V4L2_CID_VBLANK:
>  		cci_write(imx219->regmap, IMX219_REG_VTS,
> -			  format->height + ctrl->val, &ret);
> +			  (format->height + ctrl->val) / rate_factor, &ret);

As noticed by Sakari, should hblank be similarly scaled ?

>  		break;
>  	case V4L2_CID_HBLANK:
>  		cci_write(imx219->regmap, IMX219_REG_HTS,
> @@ -463,7 +529,8 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
>
>  static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
>  {
> -	return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE;
> +	return ((imx219->lanes == 2) ? IMX219_PIXEL_RATE :
> +		IMX219_PIXEL_RATE_4LANE) * imx219_get_rate_factor(imx219);
>  }
>
>  /* Initialize control handlers */
> @@ -473,7 +540,7 @@ static int imx219_init_controls(struct imx219 *imx219)
>  	const struct imx219_mode *mode = &supported_modes[0];
>  	struct v4l2_ctrl_handler *ctrl_hdlr;
>  	struct v4l2_fwnode_device_properties props;
> -	int exposure_max, exposure_def, hblank;
> +	int exposure_max, exposure_def, hblank, pixel_rate;
>  	int i, ret;
>
>  	ctrl_hdlr = &imx219->ctrl_handler;
> @@ -482,11 +549,11 @@ static int imx219_init_controls(struct imx219 *imx219)
>  		return ret;
>
>  	/* By default, PIXEL_RATE is read only */
> +	pixel_rate = imx219_get_pixel_rate(imx219);
>  	imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
>  					       V4L2_CID_PIXEL_RATE,
> -					       imx219_get_pixel_rate(imx219),
> -					       imx219_get_pixel_rate(imx219), 1,
> -					       imx219_get_pixel_rate(imx219));
> +					       pixel_rate, pixel_rate, 1,
> +					       pixel_rate);

Is this change needed ?

>
>  	imx219->link_freq =
>  		v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx219_ctrl_ops,
> @@ -593,29 +660,13 @@ static int imx219_set_framefmt(struct imx219 *imx219,
>  {
>  	const struct v4l2_mbus_framefmt *format;
>  	const struct v4l2_rect *crop;
> -	unsigned int bpp;
> -	u64 bin_h, bin_v;
> +	u64 binning;

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

> +	u32 bpp;
>  	int ret = 0;
>
>  	format = v4l2_subdev_state_get_format(state, 0);
>  	crop = v4l2_subdev_state_get_crop(state, 0);
> -
> -	switch (format->code) {
> -	case MEDIA_BUS_FMT_SRGGB8_1X8:
> -	case MEDIA_BUS_FMT_SGRBG8_1X8:
> -	case MEDIA_BUS_FMT_SGBRG8_1X8:
> -	case MEDIA_BUS_FMT_SBGGR8_1X8:
> -		bpp = 8;
> -		break;
> -
> -	case MEDIA_BUS_FMT_SRGGB10_1X10:
> -	case MEDIA_BUS_FMT_SGRBG10_1X10:
> -	case MEDIA_BUS_FMT_SGBRG10_1X10:
> -	case MEDIA_BUS_FMT_SBGGR10_1X10:
> -	default:
> -		bpp = 10;
> -		break;
> -	}
> +	bpp = imx219_get_format_bpp(format);
>
>  	cci_write(imx219->regmap, IMX219_REG_X_ADD_STA_A,
>  		  crop->left - IMX219_PIXEL_ARRAY_LEFT, &ret);
> @@ -626,28 +677,20 @@ static int imx219_set_framefmt(struct imx219 *imx219,
>  	cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A,
>  		  crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret);
>
> -	switch (crop->width / format->width) {
> -	case 1:
> -	default:
> -		bin_h = IMX219_BINNING_NONE;
> +	switch (imx219->binning) {
> +	case BINNING_NONE:
> +		binning = IMX219_BINNING_NONE;
>  		break;
> -	case 2:
> -		bin_h = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> -		break;
> -	}
> -
> -	switch (crop->height / format->height) {
> -	case 1:
> -	default:
> -		bin_v = IMX219_BINNING_NONE;
> +	case BINNING_X2:
> +		binning = IMX219_BINNING_X2;
>  		break;
> -	case 2:
> -		bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2;
> +	case BINNING_ANALOG_X2:
> +		binning = IMX219_BINNING_X2_ANALOG;
>  		break;
>  	}
>
> -	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret);
> -	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret);
> +	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, binning, &ret);
> +	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, binning, &ret);
>
>  	cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
>  		  format->width, &ret);
> @@ -851,6 +894,21 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  		int exposure_max;
>  		int exposure_def;
>  		int hblank;
> +		int pixel_rate;
> +
> +		/* Update binning mode based on format */
> +		switch (imx219_get_format_bpp(format)) {
> +		case 8:
> +			imx219->binning = mode->binning[BINNING_IDX_8_BIT];
> +			break;
> +
> +		case 10:
> +			imx219->binning = mode->binning[BINNING_IDX_10_BIT];
> +			break;
> +
> +		default:
> +			imx219->binning = BINNING_NONE;
> +		}
>
>  		/* Update limits and set FPS to default */
>  		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> @@ -879,6 +937,11 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  					 IMX219_PPL_MAX - mode->width,
>  					 1, IMX219_PPL_MIN - mode->width);
>  		__v4l2_ctrl_s_ctrl(imx219->hblank, hblank);
> +
> +		/* Scale the pixel rate based on the mode specific factor */
> +		pixel_rate = imx219_get_pixel_rate(imx219);
> +		__v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate,
> +					 pixel_rate, 1, pixel_rate);
>  	}
>
>  	return 0;
>
> --
> 2.47.0
>
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ