Re: [PATCH] media: imx283: Provide optical black mode

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Tue, Jul 30, 2024 at 12:09:53AM +0530, Umang Jain wrote:
> The IMX283 is capable of delivering optical black regions as part of
> the image capture. These regions support capture of the black levels
> for calibration and are added as extra pixels on top of the full
> resolution capture.
> 
> Supply an extra mode which accounts for this increased size that will
> produce black regions in the output image.

Sorry, but this shouldn't be an extra mode. We need a proper API, you
need to convert the driver to make it freely configurable. Furthermore,
the OB stream should probably be reported by .get_frame_desc().

> Signed-off-by: Umang Jain <umang.jain@xxxxxxxxxxxxxxxx>
> ---
> - YUYV capture sample with Pi5 ISP for side-by-side comparison of OB regions:
> https://gcdnb.pbrd.co/images/XQV29MedwXxg.png
> ---
>  drivers/media/i2c/imx283.c | 68 ++++++++++++++++++++++++++++++++------
>  1 file changed, 58 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
> index 8490618c5071..9a0fe2c34a41 100644
> --- a/drivers/media/i2c/imx283.c
> +++ b/drivers/media/i2c/imx283.c
> @@ -66,6 +66,7 @@
>  #define IMX283_REG_HTRIMMING		CCI_REG8(0x300b)
>  #define   IMX283_MDVREV			BIT(0) /* VFLIP */
>  #define   IMX283_HTRIMMING_EN		BIT(4)
> +#define   IMX283_HOB_EN			BIT(5)
>  
>  #define IMX283_REG_VWINPOS		CCI_REG16_LE(0x300f)
>  #define IMX283_REG_VWIDCUT		CCI_REG16_LE(0x3011)
> @@ -306,6 +307,7 @@ static const struct imx283_input_frequency imx283_frequencies[] = {
>  
>  enum imx283_modes {
>  	IMX283_MODE_0,
> +	IMX283_MODE_0_OB,
>  	IMX283_MODE_1,
>  	IMX283_MODE_1A,
>  	IMX283_MODE_1S,
> @@ -327,6 +329,7 @@ struct imx283_readout_mode {
>  static const struct imx283_readout_mode imx283_readout_modes[] = {
>  	/* All pixel scan modes */
>  	[IMX283_MODE_0] = { 0x04, 0x03, 0x10, 0x00 }, /* 12 bit */
> +	[IMX283_MODE_0_OB] = { 0x04, 0x03, 0x10, 0x00 }, /* 12 bit */
>  	[IMX283_MODE_1] = { 0x04, 0x01, 0x00, 0x00 }, /* 10 bit */
>  	[IMX283_MODE_1A] = { 0x04, 0x01, 0x20, 0x50 }, /* 10 bit */
>  	[IMX283_MODE_1S] = { 0x04, 0x41, 0x20, 0x50 }, /* 10 bit */
> @@ -439,6 +442,36 @@ static const struct imx283_mode supported_modes_12bit[] = {
>  			.height = 3648,
>  		},
>  	},
> +	{
> +		/* 20MPix 21.40 fps readout mode 0 with optical blacks enabled */
> +		.mode = IMX283_MODE_0_OB,
> +		.bpp = 12,
> +		.width = 5472 + 96, /* width + Horizontal optical black */
> +		.height = 3648 + 16, /* height + Vertical optical black */
> +		.min_hmax = 5914, /* 887 @ 480MHz/72MHz */
> +		.min_vmax = 3793, /* Lines */
> +
> +		.veff = 3694,
> +		.vst = 0,
> +		.vct = 0,
> +
> +		.hbin_ratio = 1,
> +		.vbin_ratio = 1,
> +
> +		/* 20.00 FPS */
> +		.default_hmax = 6000, /* 900 @ 480MHz/72MHz */
> +		.default_vmax = 4000,
> +
> +		.min_shr = 11,
> +		.horizontal_ob = 96,
> +		.vertical_ob = 16,
> +		.crop = {
> +			.top = 40,
> +			.left = 108,
> +			.width = 5472 + 96,
> +			.height = 3648 + 16,
> +		},
> +	},
>  	{
>  		/*
>  		 * Readout mode 2 : 2/2 binned mode (2736x1824)
> @@ -793,17 +826,20 @@ static int imx283_set_ctrl(struct v4l2_ctrl *ctrl)
>  		break;
>  
>  	case V4L2_CID_VFLIP:
> +		u32 htrim = IMX283_HTRIMMING_EN;
> +
>  		/*
>  		 * VFLIP is managed by BIT(0) of IMX283_REG_HTRIMMING address, hence
>  		 * both need to be set simultaneously.
>  		 */
> -		if (ctrl->val) {
> -			cci_write(imx283->cci, IMX283_REG_HTRIMMING,
> -				  IMX283_HTRIMMING_EN | IMX283_MDVREV, &ret);
> -		} else {
> -			cci_write(imx283->cci, IMX283_REG_HTRIMMING,
> -				  IMX283_HTRIMMING_EN, &ret);
> -		}
> +		if (ctrl->val)
> +			htrim = IMX283_HTRIMMING_EN | IMX283_MDVREV;
> +
> +		if (mode->mode == IMX283_MODE_0_OB)
> +			htrim |= IMX283_HOB_EN;
> +
> +		cci_write(imx283->cci, IMX283_REG_HTRIMMING, htrim, &ret);
> +
>  		break;
>  
>  	case V4L2_CID_TEST_PATTERN:
> @@ -1010,6 +1046,8 @@ static int imx283_start_streaming(struct imx283 *imx283,
>  	s32 v_pos;
>  	u32 write_v_size;
>  	u32 y_out_size;
> +	u32 htrim_end;
> +	u32 ob_size_v = 0;
>  	int ret = 0;
>  
>  	fmt = v4l2_subdev_state_get_format(state, 0);
> @@ -1057,6 +1095,12 @@ static int imx283_start_streaming(struct imx283 *imx283,
>  		mode->crop.height);
>  
>  	y_out_size = mode->crop.height / mode->vbin_ratio;
> +
> +	if (mode->mode == IMX283_MODE_0_OB) {
> +		y_out_size -= mode->vertical_ob;
> +		ob_size_v = mode->vertical_ob;
> +	}
> +
>  	write_v_size = y_out_size + mode->vertical_ob;
>  	/*
>  	 * cropping start position = (VWINPOS – Vst) × 2
> @@ -1072,12 +1116,16 @@ static int imx283_start_streaming(struct imx283 *imx283,
>  	cci_write(imx283->cci, IMX283_REG_VWIDCUT, v_widcut, &ret);
>  	cci_write(imx283->cci, IMX283_REG_VWINPOS, v_pos, &ret);
>  
> -	cci_write(imx283->cci, IMX283_REG_OB_SIZE_V, mode->vertical_ob, &ret);
> +	cci_write(imx283->cci, IMX283_REG_OB_SIZE_V, ob_size_v, &ret);
> +
> +	htrim_end = mode->crop.left + mode->crop.width;
> +
> +	if (mode->mode == IMX283_MODE_0_OB)
> +		htrim_end -= mode->horizontal_ob * mode->hbin_ratio;
>  
>  	/* TODO: Validate mode->crop is fully contained within imx283_native_area */
>  	cci_write(imx283->cci, IMX283_REG_HTRIMMING_START, mode->crop.left, &ret);
> -	cci_write(imx283->cci, IMX283_REG_HTRIMMING_END,
> -		  mode->crop.left + mode->crop.width, &ret);
> +	cci_write(imx283->cci, IMX283_REG_HTRIMMING_END, htrim_end, &ret);
>  
>  	/* Disable embedded data */
>  	cci_write(imx283->cci, IMX283_REG_EBD_X_OUT_SIZE, 0, &ret);

-- 
Regards,

Laurent Pinchart