This patch exposes IMX219's crop and compose capabilities
by implementing the selection API. Horizontal and vertical
binning being separate registers, `imx219_binning_goodness`
computes the best possible height and width of the compose
specification using the selection flags. Compose operation
updates the subdev's format object to keep them in sync.

Signed-off-by: Vinay Varma <varmavinaym@gmail.com>
---
 drivers/media/i2c/imx219.c | 222 +++++++++++++++++++++++++++++++------
 1 file changed, 190 insertions(+), 32 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 39943d72c22d..27d85fb7ad51 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -29,6 +29,7 @@
 #include <media/v4l2-event.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-mediabus.h>
+#include <media/v4l2-rect.h>
 
 /* Chip ID */
 #define IMX219_REG_CHIP_ID		CCI_REG16(0x0000)
@@ -73,6 +74,7 @@
 /* V_TIMING internal */
 #define IMX219_REG_VTS			CCI_REG16(0x0160)
 #define IMX219_VTS_MAX			0xffff
+#define IMX219_VTS_DEF 1763
 
 #define IMX219_VBLANK_MIN		32
 
@@ -146,6 +148,7 @@
 #define IMX219_PIXEL_ARRAY_TOP		8U
 #define IMX219_PIXEL_ARRAY_WIDTH	3280U
 #define IMX219_PIXEL_ARRAY_HEIGHT	2464U
+#define IMX219_MIN_COMPOSE_SIZE 8U
 
 /* Mode : resolution and related config&values */
 struct imx219_mode {
@@ -284,6 +287,8 @@ static const u32 imx219_mbus_formats[] = {
 #define IMX219_XCLR_MIN_DELAY_US	6200
 #define IMX219_XCLR_DELAY_RANGE_US	1000
 
+static const u32 binning_ratios[] = { 1, 2 };
+
 /* Mode configs */
 static const struct imx219_mode supported_modes[] = {
 	{
@@ -296,19 +301,19 @@ static const struct imx219_mode supported_modes[] = {
 		/* 1080P 30fps cropped */
 		.width = 1920,
 		.height = 1080,
-		.vts_def = 1763,
+		.vts_def = IMX219_VTS_DEF,
 	},
 	{
 		/* 2x2 binned 30fps mode */
 		.width = 1640,
 		.height = 1232,
-		.vts_def = 1763,
+		.vts_def = IMX219_VTS_DEF,
 	},
 	{
 		/* 640x480 30fps mode */
 		.width = 640,
 		.height = 480,
-		.vts_def = 1763,
+		.vts_def = IMX219_VTS_DEF,
 	},
 };
 
@@ -809,6 +814,39 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static void imx219_refresh_ctrls(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *state,
+				 unsigned int vts_def)
+{
+	int exposure_max;
+	int exposure_def;
+	int hblank;
+	struct imx219 *imx219 = to_imx219(sd);
+	struct v4l2_mbus_framefmt *fmt =
+		v4l2_subdev_get_pad_format(sd, state, 0);
+
+	/* Update limits and set FPS to default */
+	__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
+				 IMX219_VTS_MAX - fmt->height, 1,
+				 vts_def - fmt->height);
+	__v4l2_ctrl_s_ctrl(imx219->vblank, vts_def - fmt->height);
+	/* Update max exposure while meeting expected vblanking */
+	exposure_max = vts_def - 4;
+	exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
+			       exposure_max :
+			       IMX219_EXPOSURE_DEFAULT;
+	__v4l2_ctrl_modify_range(imx219->exposure, imx219->exposure->minimum,
+				 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.
+	 */
+	hblank = IMX219_PPL_DEFAULT - fmt->width;
+	__v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1, hblank);
+}
+
 static int imx219_set_pad_format(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *state,
 				 struct v4l2_subdev_format *fmt)
@@ -816,7 +854,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	struct imx219 *imx219 = to_imx219(sd);
 	const struct imx219_mode *mode;
 	struct v4l2_mbus_framefmt *format;
-	struct v4l2_rect *crop;
+	struct v4l2_rect *crop, *compose;
 	unsigned int bin_h, bin_v;
 
 	mode = v4l2_find_nearest_size(supported_modes,
@@ -842,34 +880,14 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	crop->left = (IMX219_NATIVE_WIDTH - crop->width) / 2;
 	crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
 
-	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		int exposure_max;
-		int exposure_def;
-		int hblank;
-
-		/* Update limits and set FPS to default */
-		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
-					 IMX219_VTS_MAX - mode->height, 1,
-					 mode->vts_def - mode->height);
-		__v4l2_ctrl_s_ctrl(imx219->vblank,
-				   mode->vts_def - mode->height);
-		/* Update max exposure while meeting expected vblanking */
-		exposure_max = mode->vts_def - 4;
-		exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
-			exposure_max : IMX219_EXPOSURE_DEFAULT;
-		__v4l2_ctrl_modify_range(imx219->exposure,
-					 imx219->exposure->minimum,
-					 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.
-		 */
-		hblank = IMX219_PPL_DEFAULT - mode->width;
-		__v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1,
-					 hblank);
-	}
+	compose = v4l2_subdev_get_pad_compose(sd, state, 0);
+	compose->width = format->width;
+	compose->height = format->height;
+	compose->left = 0;
+	compose->top = 0;
+
+	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+		imx219_refresh_ctrls(sd, state, mode->vts_def);
 
 	return 0;
 }
@@ -884,6 +902,11 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
 		return 0;
 	}
 
+	case V4L2_SEL_TGT_COMPOSE: {
+		sel->r = *v4l2_subdev_get_pad_compose(sd, state, 0);
+		return 0;
+	}
+
 	case V4L2_SEL_TGT_NATIVE_SIZE:
 		sel->r.top = 0;
 		sel->r.left = 0;
@@ -900,11 +923,145 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
 		sel->r.height = IMX219_PIXEL_ARRAY_HEIGHT;
 
 		return 0;
+
+	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
+	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+	case V4L2_SEL_TGT_COMPOSE_PADDED:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = IMX219_PIXEL_ARRAY_WIDTH;
+		sel->r.height = IMX219_PIXEL_ARRAY_HEIGHT;
+		return 0;
 	}
 
 	return -EINVAL;
 }
 
+#define IMX219_ROUND(dim, step, flags)                \
+	((flags) & V4L2_SEL_FLAG_GE ?                 \
+		 round_up((dim), (step)) :            \
+		 ((flags) & V4L2_SEL_FLAG_LE ?        \
+			  round_down((dim), (step)) : \
+			  round_down((dim) + (step) / 2, (step))))
+
+static int imx219_set_selection_crop(struct v4l2_subdev *sd,
+				     struct v4l2_subdev_state *sd_state,
+				     struct v4l2_subdev_selection *sel)
+{
+	u32 max_binning;
+	struct v4l2_rect *compose, *crop;
+	struct v4l2_mbus_framefmt *fmt;
+
+	crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
+	if (v4l2_rect_equal(&sel->r, crop))
+		return false;
+	max_binning = binning_ratios[ARRAY_SIZE(binning_ratios) - 1];
+	sel->r.width =
+		clamp(IMX219_ROUND(sel->r.width, max_binning, sel->flags),
+		      max_binning * IMX219_MIN_COMPOSE_SIZE,
+		      IMX219_PIXEL_ARRAY_WIDTH);
+	sel->r.height =
+		clamp(IMX219_ROUND(sel->r.width, max_binning, sel->flags),
+		      max_binning * IMX219_MIN_COMPOSE_SIZE,
+		      IMX219_PIXEL_ARRAY_WIDTH);
+	sel->r.left =
+		min_t(u32, sel->r.left, IMX219_PIXEL_ARRAY_LEFT - sel->r.width);
+	sel->r.top =
+		min_t(u32, sel->r.top, IMX219_PIXEL_ARRAY_TOP - sel->r.top);
+
+	compose = v4l2_subdev_get_pad_compose(sd, sd_state, 0);
+	fmt = v4l2_subdev_get_pad_format(sd, sd_state, 0);
+	*crop = sel->r;
+	compose->height = crop->height;
+	compose->width = crop->width;
+	return true;
+}
+
+static int imx219_binning_goodness(u32 act, u32 ask, u32 flags)
+{
+	const int goodness = 100000;
+	int val = 0;
+
+	if (flags & V4L2_SEL_FLAG_GE)
+		if (act < ask)
+			val -= goodness;
+
+	if (flags & V4L2_SEL_FLAG_LE)
+		if (act > ask)
+			val -= goodness;
+
+	val -= abs(act - ask);
+
+	return val;
+}
+
+static bool imx219_set_selection_compose(struct v4l2_subdev *sd,
+					 struct v4l2_subdev_state *state,
+					 struct v4l2_subdev_selection *sel)
+{
+	int best_goodness;
+	struct v4l2_rect *compose, *crop;
+
+	compose = v4l2_subdev_get_pad_compose(sd, state, 0);
+	if (v4l2_rect_equal(compose, &sel->r))
+		return false;
+
+	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
+
+	best_goodness = INT_MIN;
+	for (int i = 0; i < ARRAY_SIZE(binning_ratios); ++i) {
+		u32 width = crop->width / binning_ratios[i];
+		int goodness = imx219_binning_goodness(width, sel->r.width,
+						       sel->flags);
+		if (goodness > best_goodness) {
+			best_goodness = goodness;
+			compose->width = width;
+		}
+	}
+	best_goodness = INT_MIN;
+	for (int i = 0; i < ARRAY_SIZE(binning_ratios); ++i) {
+		u32 height = crop->height / binning_ratios[i];
+		int goodness = imx219_binning_goodness(height, sel->r.height,
+						       sel->flags);
+		if (goodness > best_goodness) {
+			best_goodness = goodness;
+			compose->height = height;
+		}
+	}
+	return true;
+}
+
+static int imx219_set_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *sd_state,
+				struct v4l2_subdev_selection *sel)
+{
+	bool compose_updated = false;
+
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP:
+		compose_updated = imx219_set_selection_crop(sd, sd_state, sel);
+		break;
+	case V4L2_SEL_TGT_COMPOSE:
+		compose_updated =
+			imx219_set_selection_compose(sd, sd_state, sel);
+		break;
+	default:
+		return -EINVAL;
+	}
+	if (compose_updated) {
+		struct v4l2_rect *compose =
+			v4l2_subdev_get_pad_compose(sd, sd_state, 0);
+		struct v4l2_mbus_framefmt *fmt =
+			v4l2_subdev_get_pad_format(sd, sd_state, 0);
+		fmt->height = compose->height;
+		fmt->width = compose->width;
+	}
+	if (compose_updated && sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+		imx219_refresh_ctrls(sd, sd_state, IMX219_VTS_DEF);
+
+	return 0;
+}
+
 static int imx219_init_cfg(struct v4l2_subdev *sd,
 			   struct v4l2_subdev_state *state)
 {
@@ -938,6 +1095,7 @@ static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
 	.get_fmt = v4l2_subdev_get_fmt,
 	.set_fmt = imx219_set_pad_format,
 	.get_selection = imx219_get_selection,
+	.set_selection = imx219_set_selection,
 	.enum_frame_size = imx219_enum_frame_size,
 };
 
-- 
2.43.0

Hi Vinay,

Thanks for the patch.

On Sun, Jan 07, 2024 at 03:42:59PM +0800, Vinay Varma wrote:
> This patch exposes IMX219's crop and compose capabilities
> by implementing the selection API. Horizontal and vertical
> binning being separate registers, `imx219_binning_goodness`
> computes the best possible height and width of the compose
> specification using the selection flags. Compose operation
> updates the subdev's format object to keep them in sync.

The line length limit here is 75, not ~ 60. Please rewrap.

> 
> Signed-off-by: Vinay Varma <varmavinaym@gmail.com>
> ---
>  drivers/media/i2c/imx219.c | 222 +++++++++++++++++++++++++++++++------
>  1 file changed, 190 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 39943d72c22d..27d85fb7ad51 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -29,6 +29,7 @@
>  #include <media/v4l2-event.h>
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-mediabus.h>
> +#include <media/v4l2-rect.h>
>  
>  /* Chip ID */
>  #define IMX219_REG_CHIP_ID		CCI_REG16(0x0000)
> @@ -73,6 +74,7 @@
>  /* V_TIMING internal */
>  #define IMX219_REG_VTS			CCI_REG16(0x0160)
>  #define IMX219_VTS_MAX			0xffff
> +#define IMX219_VTS_DEF 1763
>  
>  #define IMX219_VBLANK_MIN		32
>  
> @@ -146,6 +148,7 @@
>  #define IMX219_PIXEL_ARRAY_TOP		8U
>  #define IMX219_PIXEL_ARRAY_WIDTH	3280U
>  #define IMX219_PIXEL_ARRAY_HEIGHT	2464U
> +#define IMX219_MIN_COMPOSE_SIZE 8U

Please align 8U with the rest of the macros. Same above.

>  
>  /* Mode : resolution and related config&values */
>  struct imx219_mode {
> @@ -284,6 +287,8 @@ static const u32 imx219_mbus_formats[] = {
>  #define IMX219_XCLR_MIN_DELAY_US	6200
>  #define IMX219_XCLR_DELAY_RANGE_US	1000
>  
> +static const u32 binning_ratios[] = { 1, 2 };
> +
>  /* Mode configs */
>  static const struct imx219_mode supported_modes[] = {
>  	{
> @@ -296,19 +301,19 @@ static const struct imx219_mode supported_modes[] = {
>  		/* 1080P 30fps cropped */
>  		.width = 1920,
>  		.height = 1080,
> -		.vts_def = 1763,
> +		.vts_def = IMX219_VTS_DEF,
>  	},
>  	{
>  		/* 2x2 binned 30fps mode */
>  		.width = 1640,
>  		.height = 1232,
> -		.vts_def = 1763,
> +		.vts_def = IMX219_VTS_DEF,
>  	},
>  	{
>  		/* 640x480 30fps mode */
>  		.width = 640,
>  		.height = 480,
> -		.vts_def = 1763,
> +		.vts_def = IMX219_VTS_DEF,
>  	},
>  };
>  
> @@ -809,6 +814,39 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static void imx219_refresh_ctrls(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_state *state,
> +				 unsigned int vts_def)
> +{
> +	int exposure_max;
> +	int exposure_def;
> +	int hblank;
> +	struct imx219 *imx219 = to_imx219(sd);
> +	struct v4l2_mbus_framefmt *fmt =
> +		v4l2_subdev_get_pad_format(sd, state, 0);
> +
> +	/* Update limits and set FPS to default */
> +	__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> +				 IMX219_VTS_MAX - fmt->height, 1,
> +				 vts_def - fmt->height);
> +	__v4l2_ctrl_s_ctrl(imx219->vblank, vts_def - fmt->height);
> +	/* Update max exposure while meeting expected vblanking */
> +	exposure_max = vts_def - 4;
> +	exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
> +			       exposure_max :
> +			       IMX219_EXPOSURE_DEFAULT;
> +	__v4l2_ctrl_modify_range(imx219->exposure, imx219->exposure->minimum,
> +				 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.
> +	 */
> +	hblank = IMX219_PPL_DEFAULT - fmt->width;
> +	__v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1, hblank);
> +}
> +
>  static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_state *state,
>  				 struct v4l2_subdev_format *fmt)
> @@ -816,7 +854,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  	struct imx219 *imx219 = to_imx219(sd);
>  	const struct imx219_mode *mode;
>  	struct v4l2_mbus_framefmt *format;
> -	struct v4l2_rect *crop;
> +	struct v4l2_rect *crop, *compose;
>  	unsigned int bin_h, bin_v;
>  
>  	mode = v4l2_find_nearest_size(supported_modes,
> @@ -842,34 +880,14 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  	crop->left = (IMX219_NATIVE_WIDTH - crop->width) / 2;
>  	crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
>  
> -	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> -		int exposure_max;
> -		int exposure_def;
> -		int hblank;
> -
> -		/* Update limits and set FPS to default */
> -		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> -					 IMX219_VTS_MAX - mode->height, 1,
> -					 mode->vts_def - mode->height);
> -		__v4l2_ctrl_s_ctrl(imx219->vblank,
> -				   mode->vts_def - mode->height);
> -		/* Update max exposure while meeting expected vblanking */
> -		exposure_max = mode->vts_def - 4;
> -		exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
> -			exposure_max : IMX219_EXPOSURE_DEFAULT;
> -		__v4l2_ctrl_modify_range(imx219->exposure,
> -					 imx219->exposure->minimum,
> -					 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.
> -		 */
> -		hblank = IMX219_PPL_DEFAULT - mode->width;
> -		__v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1,
> -					 hblank);
> -	}
> +	compose = v4l2_subdev_get_pad_compose(sd, state, 0);
> +	compose->width = format->width;
> +	compose->height = format->height;
> +	compose->left = 0;
> +	compose->top = 0;
> +
> +	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> +		imx219_refresh_ctrls(sd, state, mode->vts_def);
>  
>  	return 0;
>  }
> @@ -884,6 +902,11 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
>  		return 0;
>  	}
>  
> +	case V4L2_SEL_TGT_COMPOSE: {
> +		sel->r = *v4l2_subdev_get_pad_compose(sd, state, 0);
> +		return 0;
> +	}

The braces are unnecessary here.

> +
>  	case V4L2_SEL_TGT_NATIVE_SIZE:
>  		sel->r.top = 0;
>  		sel->r.left = 0;
> @@ -900,11 +923,145 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
>  		sel->r.height = IMX219_PIXEL_ARRAY_HEIGHT;
>  
>  		return 0;
> +
> +	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> +	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> +	case V4L2_SEL_TGT_COMPOSE_PADDED:
> +		sel->r.top = 0;
> +		sel->r.left = 0;
> +		sel->r.width = IMX219_PIXEL_ARRAY_WIDTH;
> +		sel->r.height = IMX219_PIXEL_ARRAY_HEIGHT;
> +		return 0;
>  	}
>  
>  	return -EINVAL;
>  }
>  
> +#define IMX219_ROUND(dim, step, flags)                \
> +	((flags) & V4L2_SEL_FLAG_GE ?                 \
> +		 round_up((dim), (step)) :            \
> +		 ((flags) & V4L2_SEL_FLAG_LE ?        \
> +			  round_down((dim), (step)) : \
> +			  round_down((dim) + (step) / 2, (step))))
> +
> +static int imx219_set_selection_crop(struct v4l2_subdev *sd,
> +				     struct v4l2_subdev_state *sd_state,
> +				     struct v4l2_subdev_selection *sel)
> +{
> +	u32 max_binning;
> +	struct v4l2_rect *compose, *crop;
> +	struct v4l2_mbus_framefmt *fmt;
> +
> +	crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> +	if (v4l2_rect_equal(&sel->r, crop))
> +		return false;
> +	max_binning = binning_ratios[ARRAY_SIZE(binning_ratios) - 1];
> +	sel->r.width =
> +		clamp(IMX219_ROUND(sel->r.width, max_binning, sel->flags),
> +		      max_binning * IMX219_MIN_COMPOSE_SIZE,
> +		      IMX219_PIXEL_ARRAY_WIDTH);
> +	sel->r.height =
> +		clamp(IMX219_ROUND(sel->r.width, max_binning, sel->flags),
> +		      max_binning * IMX219_MIN_COMPOSE_SIZE,
> +		      IMX219_PIXEL_ARRAY_WIDTH);
> +	sel->r.left =
> +		min_t(u32, sel->r.left, IMX219_PIXEL_ARRAY_LEFT - sel->r.width);
> +	sel->r.top =
> +		min_t(u32, sel->r.top, IMX219_PIXEL_ARRAY_TOP - sel->r.top);
> +
> +	compose = v4l2_subdev_get_pad_compose(sd, sd_state, 0);
> +	fmt = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> +	*crop = sel->r;
> +	compose->height = crop->height;
> +	compose->width = crop->width;
> +	return true;
> +}
> +
> +static int imx219_binning_goodness(u32 act, u32 ask, u32 flags)
> +{
> +	const int goodness = 100000;
> +	int val = 0;
> +
> +	if (flags & V4L2_SEL_FLAG_GE)
> +		if (act < ask)
> +			val -= goodness;
> +
> +	if (flags & V4L2_SEL_FLAG_LE)
> +		if (act > ask)
> +			val -= goodness;
> +
> +	val -= abs(act - ask);
> +
> +	return val;
> +}
> +
> +static bool imx219_set_selection_compose(struct v4l2_subdev *sd,
> +					 struct v4l2_subdev_state *state,
> +					 struct v4l2_subdev_selection *sel)
> +{
> +	int best_goodness;

This would be nicer if declared after the line below. Think of reverse
Christmas trees.

Similarly for max_binning a few functions up actually as well as variables
in imx219_refresh_ctrls().

> +	struct v4l2_rect *compose, *crop;
> +
> +	compose = v4l2_subdev_get_pad_compose(sd, state, 0);
> +	if (v4l2_rect_equal(compose, &sel->r))
> +		return false;
> +
> +	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> +
> +	best_goodness = INT_MIN;
> +	for (int i = 0; i < ARRAY_SIZE(binning_ratios); ++i) {
> +		u32 width = crop->width / binning_ratios[i];
> +		int goodness = imx219_binning_goodness(width, sel->r.width,
> +						       sel->flags);
> +		if (goodness > best_goodness) {
> +			best_goodness = goodness;
> +			compose->width = width;
> +		}
> +	}
> +	best_goodness = INT_MIN;
> +	for (int i = 0; i < ARRAY_SIZE(binning_ratios); ++i) {
> +		u32 height = crop->height / binning_ratios[i];
> +		int goodness = imx219_binning_goodness(height, sel->r.height,
> +						       sel->flags);
> +		if (goodness > best_goodness) {
> +			best_goodness = goodness;
> +			compose->height = height;
> +		}
> +	}
> +	return true;
> +}
> +
> +static int imx219_set_selection(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_state *sd_state,
> +				struct v4l2_subdev_selection *sel)
> +{
> +	bool compose_updated = false;
> +
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_CROP:
> +		compose_updated = imx219_set_selection_crop(sd, sd_state, sel);
> +		break;
> +	case V4L2_SEL_TGT_COMPOSE:
> +		compose_updated =
> +			imx219_set_selection_compose(sd, sd_state, sel);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	if (compose_updated) {
> +		struct v4l2_rect *compose =
> +			v4l2_subdev_get_pad_compose(sd, sd_state, 0);
> +		struct v4l2_mbus_framefmt *fmt =
> +			v4l2_subdev_get_pad_format(sd, sd_state, 0);

A newline here?

> +		fmt->height = compose->height;
> +		fmt->width = compose->width;
> +	}
> +	if (compose_updated && sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> +		imx219_refresh_ctrls(sd, sd_state, IMX219_VTS_DEF);

Please move this inside the previous condition (where you check just
sel->which).

> +
> +	return 0;
> +}
> +
>  static int imx219_init_cfg(struct v4l2_subdev *sd,
>  			   struct v4l2_subdev_state *state)
>  {
> @@ -938,6 +1095,7 @@ static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
>  	.get_fmt = v4l2_subdev_get_fmt,
>  	.set_fmt = imx219_set_pad_format,
>  	.get_selection = imx219_get_selection,
> +	.set_selection = imx219_set_selection,
>  	.enum_frame_size = imx219_enum_frame_size,
>  };
>  

-- 
Regards,

Sakari Ailus

Hi Sakari, Vinay,

   a more foundamental question is how this usage of the crop/compose
API plays with the fact we enumerate only a limited set of frame
sizes, and now you can get an arbitrary output size. We could get away
by modifying enum_frame_sizes to return a size range (or ranges) but I
wonder if it wouldn't be better to introduce an internal pad to
represent the pixel array where to apply TGT_CROP in combination with
a source pad where we could apply TGT_COMPOSE and an output format.

To be completely honest, binning should be handled with a different
mechanism than the selection API, but that would require a completly
new design.

Laurent: are there plans to introduce internal pad for embedded data
support for this sensor ?

Also, the patch doesn't apply on the most recent media master, please
rebase before resending.

On Mon, Jan 08, 2024 at 08:44:59AM +0000, Sakari Ailus wrote:
> Hi Vinay,
>
> Thanks for the patch.
>
> On Sun, Jan 07, 2024 at 03:42:59PM +0800, Vinay Varma wrote:
> > This patch exposes IMX219's crop and compose capabilities
> > by implementing the selection API. Horizontal and vertical
> > binning being separate registers, `imx219_binning_goodness`
> > computes the best possible height and width of the compose
> > specification using the selection flags. Compose operation
> > updates the subdev's format object to keep them in sync.
>
> The line length limit here is 75, not ~ 60. Please rewrap.
>
> >
> > Signed-off-by: Vinay Varma <varmavinaym@gmail.com>
> > ---
> >  drivers/media/i2c/imx219.c | 222 +++++++++++++++++++++++++++++++------
> >  1 file changed, 190 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 39943d72c22d..27d85fb7ad51 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -29,6 +29,7 @@
> >  #include <media/v4l2-event.h>
> >  #include <media/v4l2-fwnode.h>
> >  #include <media/v4l2-mediabus.h>
> > +#include <media/v4l2-rect.h>
> >
> >  /* Chip ID */
> >  #define IMX219_REG_CHIP_ID		CCI_REG16(0x0000)
> > @@ -73,6 +74,7 @@
> >  /* V_TIMING internal */
> >  #define IMX219_REG_VTS			CCI_REG16(0x0160)
> >  #define IMX219_VTS_MAX			0xffff
> > +#define IMX219_VTS_DEF 1763
> >
> >  #define IMX219_VBLANK_MIN		32
> >
> > @@ -146,6 +148,7 @@
> >  #define IMX219_PIXEL_ARRAY_TOP		8U
> >  #define IMX219_PIXEL_ARRAY_WIDTH	3280U
> >  #define IMX219_PIXEL_ARRAY_HEIGHT	2464U
> > +#define IMX219_MIN_COMPOSE_SIZE 8U
>
> Please align 8U with the rest of the macros. Same above.
>
> >
> >  /* Mode : resolution and related config&values */
> >  struct imx219_mode {
> > @@ -284,6 +287,8 @@ static const u32 imx219_mbus_formats[] = {
> >  #define IMX219_XCLR_MIN_DELAY_US	6200
> >  #define IMX219_XCLR_DELAY_RANGE_US	1000
> >
> > +static const u32 binning_ratios[] = { 1, 2 };
> > +
> >  /* Mode configs */
> >  static const struct imx219_mode supported_modes[] = {
> >  	{
> > @@ -296,19 +301,19 @@ static const struct imx219_mode supported_modes[] = {
> >  		/* 1080P 30fps cropped */
> >  		.width = 1920,
> >  		.height = 1080,
> > -		.vts_def = 1763,
> > +		.vts_def = IMX219_VTS_DEF,
> >  	},
> >  	{
> >  		/* 2x2 binned 30fps mode */
> >  		.width = 1640,
> >  		.height = 1232,
> > -		.vts_def = 1763,
> > +		.vts_def = IMX219_VTS_DEF,
> >  	},
> >  	{
> >  		/* 640x480 30fps mode */
> >  		.width = 640,
> >  		.height = 480,
> > -		.vts_def = 1763,
> > +		.vts_def = IMX219_VTS_DEF,
> >  	},
> >  };
> >
> > @@ -809,6 +814,39 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >
> > +static void imx219_refresh_ctrls(struct v4l2_subdev *sd,
> > +				 struct v4l2_subdev_state *state,
> > +				 unsigned int vts_def)
> > +{
> > +	int exposure_max;
> > +	int exposure_def;
> > +	int hblank;
> > +	struct imx219 *imx219 = to_imx219(sd);
> > +	struct v4l2_mbus_framefmt *fmt =
> > +		v4l2_subdev_get_pad_format(sd, state, 0);
> > +
> > +	/* Update limits and set FPS to default */
> > +	__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > +				 IMX219_VTS_MAX - fmt->height, 1,
> > +				 vts_def - fmt->height);
> > +	__v4l2_ctrl_s_ctrl(imx219->vblank, vts_def - fmt->height);
> > +	/* Update max exposure while meeting expected vblanking */
> > +	exposure_max = vts_def - 4;
> > +	exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
> > +			       exposure_max :
> > +			       IMX219_EXPOSURE_DEFAULT;
> > +	__v4l2_ctrl_modify_range(imx219->exposure, imx219->exposure->minimum,
> > +				 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.
> > +	 */
> > +	hblank = IMX219_PPL_DEFAULT - fmt->width;
> > +	__v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1, hblank);
> > +}
> > +
> >  static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >  				 struct v4l2_subdev_state *state,
> >  				 struct v4l2_subdev_format *fmt)
> > @@ -816,7 +854,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >  	struct imx219 *imx219 = to_imx219(sd);
> >  	const struct imx219_mode *mode;
> >  	struct v4l2_mbus_framefmt *format;
> > -	struct v4l2_rect *crop;
> > +	struct v4l2_rect *crop, *compose;
> >  	unsigned int bin_h, bin_v;
> >
> >  	mode = v4l2_find_nearest_size(supported_modes,
> > @@ -842,34 +880,14 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >  	crop->left = (IMX219_NATIVE_WIDTH - crop->width) / 2;
> >  	crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
> >
> > -	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > -		int exposure_max;
> > -		int exposure_def;
> > -		int hblank;
> > -
> > -		/* Update limits and set FPS to default */
> > -		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > -					 IMX219_VTS_MAX - mode->height, 1,
> > -					 mode->vts_def - mode->height);
> > -		__v4l2_ctrl_s_ctrl(imx219->vblank,
> > -				   mode->vts_def - mode->height);
> > -		/* Update max exposure while meeting expected vblanking */
> > -		exposure_max = mode->vts_def - 4;
> > -		exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
> > -			exposure_max : IMX219_EXPOSURE_DEFAULT;
> > -		__v4l2_ctrl_modify_range(imx219->exposure,
> > -					 imx219->exposure->minimum,
> > -					 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.
> > -		 */
> > -		hblank = IMX219_PPL_DEFAULT - mode->width;
> > -		__v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1,
> > -					 hblank);
> > -	}
> > +	compose = v4l2_subdev_get_pad_compose(sd, state, 0);
> > +	compose->width = format->width;
> > +	compose->height = format->height;
> > +	compose->left = 0;
> > +	compose->top = 0;
> > +
> > +	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > +		imx219_refresh_ctrls(sd, state, mode->vts_def);
> >
> >  	return 0;
> >  }
> > @@ -884,6 +902,11 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
> >  		return 0;
> >  	}
> >
> > +	case V4L2_SEL_TGT_COMPOSE: {
> > +		sel->r = *v4l2_subdev_get_pad_compose(sd, state, 0);
> > +		return 0;
> > +	}
>
> The braces are unnecessary here.
>
> > +
> >  	case V4L2_SEL_TGT_NATIVE_SIZE:
> >  		sel->r.top = 0;
> >  		sel->r.left = 0;
> > @@ -900,11 +923,145 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
> >  		sel->r.height = IMX219_PIXEL_ARRAY_HEIGHT;
> >
> >  		return 0;
> > +
> > +	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> > +	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> > +	case V4L2_SEL_TGT_COMPOSE_PADDED:
> > +		sel->r.top = 0;
> > +		sel->r.left = 0;
> > +		sel->r.width = IMX219_PIXEL_ARRAY_WIDTH;
> > +		sel->r.height = IMX219_PIXEL_ARRAY_HEIGHT;
> > +		return 0;
> >  	}
> >
> >  	return -EINVAL;
> >  }
> >
> > +#define IMX219_ROUND(dim, step, flags)                \
> > +	((flags) & V4L2_SEL_FLAG_GE ?                 \
> > +		 round_up((dim), (step)) :            \
> > +		 ((flags) & V4L2_SEL_FLAG_LE ?        \
> > +			  round_down((dim), (step)) : \
> > +			  round_down((dim) + (step) / 2, (step))))
> > +
> > +static int imx219_set_selection_crop(struct v4l2_subdev *sd,
> > +				     struct v4l2_subdev_state *sd_state,
> > +				     struct v4l2_subdev_selection *sel)
> > +{
> > +	u32 max_binning;
> > +	struct v4l2_rect *compose, *crop;
> > +	struct v4l2_mbus_framefmt *fmt;
> > +
> > +	crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > +	if (v4l2_rect_equal(&sel->r, crop))
> > +		return false;
> > +	max_binning = binning_ratios[ARRAY_SIZE(binning_ratios) - 1];
> > +	sel->r.width =
> > +		clamp(IMX219_ROUND(sel->r.width, max_binning, sel->flags),
> > +		      max_binning * IMX219_MIN_COMPOSE_SIZE,
> > +		      IMX219_PIXEL_ARRAY_WIDTH);
> > +	sel->r.height =
> > +		clamp(IMX219_ROUND(sel->r.width, max_binning, sel->flags),
> > +		      max_binning * IMX219_MIN_COMPOSE_SIZE,
> > +		      IMX219_PIXEL_ARRAY_WIDTH);
> > +	sel->r.left =
> > +		min_t(u32, sel->r.left, IMX219_PIXEL_ARRAY_LEFT - sel->r.width);
> > +	sel->r.top =
> > +		min_t(u32, sel->r.top, IMX219_PIXEL_ARRAY_TOP - sel->r.top);
> > +
> > +	compose = v4l2_subdev_get_pad_compose(sd, sd_state, 0);
> > +	fmt = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > +	*crop = sel->r;
> > +	compose->height = crop->height;
> > +	compose->width = crop->width;
> > +	return true;
> > +}
> > +
> > +static int imx219_binning_goodness(u32 act, u32 ask, u32 flags)
> > +{
> > +	const int goodness = 100000;
> > +	int val = 0;
> > +
> > +	if (flags & V4L2_SEL_FLAG_GE)
> > +		if (act < ask)
> > +			val -= goodness;
> > +
> > +	if (flags & V4L2_SEL_FLAG_LE)
> > +		if (act > ask)
> > +			val -= goodness;
> > +
> > +	val -= abs(act - ask);
> > +
> > +	return val;
> > +}
> > +
> > +static bool imx219_set_selection_compose(struct v4l2_subdev *sd,
> > +					 struct v4l2_subdev_state *state,
> > +					 struct v4l2_subdev_selection *sel)
> > +{
> > +	int best_goodness;
>
> This would be nicer if declared after the line below. Think of reverse
> Christmas trees.
>
> Similarly for max_binning a few functions up actually as well as variables
> in imx219_refresh_ctrls().
>
> > +	struct v4l2_rect *compose, *crop;
> > +
> > +	compose = v4l2_subdev_get_pad_compose(sd, state, 0);
> > +	if (v4l2_rect_equal(compose, &sel->r))
> > +		return false;
> > +
> > +	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> > +
> > +	best_goodness = INT_MIN;
> > +	for (int i = 0; i < ARRAY_SIZE(binning_ratios); ++i) {
> > +		u32 width = crop->width / binning_ratios[i];
> > +		int goodness = imx219_binning_goodness(width, sel->r.width,
> > +						       sel->flags);
> > +		if (goodness > best_goodness) {
> > +			best_goodness = goodness;
> > +			compose->width = width;
> > +		}
> > +	}
> > +	best_goodness = INT_MIN;
> > +	for (int i = 0; i < ARRAY_SIZE(binning_ratios); ++i) {
> > +		u32 height = crop->height / binning_ratios[i];
> > +		int goodness = imx219_binning_goodness(height, sel->r.height,
> > +						       sel->flags);
> > +		if (goodness > best_goodness) {
> > +			best_goodness = goodness;
> > +			compose->height = height;
> > +		}
> > +	}
> > +	return true;
> > +}
> > +
> > +static int imx219_set_selection(struct v4l2_subdev *sd,
> > +				struct v4l2_subdev_state *sd_state,
> > +				struct v4l2_subdev_selection *sel)
> > +{
> > +	bool compose_updated = false;
> > +
> > +	switch (sel->target) {
> > +	case V4L2_SEL_TGT_CROP:
> > +		compose_updated = imx219_set_selection_crop(sd, sd_state, sel);
> > +		break;
> > +	case V4L2_SEL_TGT_COMPOSE:
> > +		compose_updated =
> > +			imx219_set_selection_compose(sd, sd_state, sel);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	if (compose_updated) {
> > +		struct v4l2_rect *compose =
> > +			v4l2_subdev_get_pad_compose(sd, sd_state, 0);
> > +		struct v4l2_mbus_framefmt *fmt =
> > +			v4l2_subdev_get_pad_format(sd, sd_state, 0);
>
> A newline here?
>
> > +		fmt->height = compose->height;
> > +		fmt->width = compose->width;
> > +	}
> > +	if (compose_updated && sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > +		imx219_refresh_ctrls(sd, sd_state, IMX219_VTS_DEF);
>
> Please move this inside the previous condition (where you check just
> sel->which).
>
> > +
> > +	return 0;
> > +}
> > +
> >  static int imx219_init_cfg(struct v4l2_subdev *sd,
> >  			   struct v4l2_subdev_state *state)
> >  {
> > @@ -938,6 +1095,7 @@ static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
> >  	.get_fmt = v4l2_subdev_get_fmt,
> >  	.set_fmt = imx219_set_pad_format,
> >  	.get_selection = imx219_get_selection,
> > +	.set_selection = imx219_set_selection,
> >  	.enum_frame_size = imx219_enum_frame_size,
> >  };
> >
>
> --
> Regards,
>
> Sakari Ailus
>

Hi Jacopo,

On Mon, Jan 08, 2024 at 10:19:35AM +0100, Jacopo Mondi wrote:
> Hi Sakari, Vinay,
> 
>    a more foundamental question is how this usage of the crop/compose
> API plays with the fact we enumerate only a limited set of frame
> sizes, and now you can get an arbitrary output size. We could get away
> by modifying enum_frame_sizes to return a size range (or ranges) but I
> wonder if it wouldn't be better to introduce an internal pad to
> represent the pixel array where to apply TGT_CROP in combination with
> a source pad where we could apply TGT_COMPOSE and an output format.

My earlier review wasn't focussed on the interface at all...

To depart from the current restrictions on single-subdev sensor drivers,
this is one option.

Sensors implement various steps in different orders and different drivers
have different capabilities, too. Mainly there are two classes: freely
configurable drivers such cas CCS and then register list based drivers
where virtually any dependencies between configurations are possible.

We probably can't support both classes with the same API semantics and due
to hardware differencies. The sensor UAPI will be less than uniform it has
been in the past but I don't think this should be an issue.

I wonder how much common understanding we have at this point on how this
API would look like. Probably not much?

-- 
Regards,

Sakari Ailus

Hello,

On Tue, Jan 16, 2024 at 07:09:59PM +0000, Sakari Ailus wrote:
> On Mon, Jan 08, 2024 at 10:19:35AM +0100, Jacopo Mondi wrote:
> > Hi Sakari, Vinay,
> > 
> >    a more foundamental question is how this usage of the crop/compose
> > API plays with the fact we enumerate only a limited set of frame
> > sizes, and now you can get an arbitrary output size. We could get away
> > by modifying enum_frame_sizes to return a size range (or ranges) but I
> > wonder if it wouldn't be better to introduce an internal pad to
> > represent the pixel array where to apply TGT_CROP in combination with
> > a source pad where we could apply TGT_COMPOSE and an output format.

I'm working on patches that implement an internal image pad, as part of
the work to add embedded data support. I hope to post this in the near
future.

> My earlier review wasn't focussed on the interface at all...
> 
> To depart from the current restrictions on single-subdev sensor drivers,
> this is one option.
> 
> Sensors implement various steps in different orders and different drivers
> have different capabilities, too. Mainly there are two classes: freely
> configurable drivers such cas CCS and then register list based drivers
> where virtually any dependencies between configurations are possible.
> 
> We probably can't support both classes with the same API semantics and due
> to hardware differencies. The sensor UAPI will be less than uniform it has
> been in the past but I don't think this should be an issue.
> 
> I wonder how much common understanding we have at this point on how this
> API would look like. Probably not much?

-- 
Regards,

Laurent Pinchart

Hi Jacopo, Sakari,

I have sent out an updated patch with all the changes mentioned here,
but `git send-email` ended up creating a new thread rather than replying
to this one.

On 24/01/08 10:19AM, Jacopo Mondi wrote:
> Hi Sakari, Vinay,
> 
>    a more foundamental question is how this usage of the crop/compose
> API plays with the fact we enumerate only a limited set of frame
> sizes, and now you can get an arbitrary output size. We could get away
> by modifying enum_frame_sizes to return a size range (or ranges) but I
> wonder if it wouldn't be better to introduce an internal pad to
> represent the pixel array where to apply TGT_CROP in combination with
> a source pad where we could apply TGT_COMPOSE and an output format.
While the driver could support stepwise framesizes, to maintain
backwards compatibility do we not have to continue with the existing 4
driscrete framesizes? At the same time, the selection API gives a lot
more control than S_FMT operation in terms of what area to sample that
may not be required for the general use cases.
> 
> To be completely honest, binning should be handled with a different
> mechanism than the selection API, but that would require a completly
> new design.
I agree that a lot of the binning aspects feels very implicit in the
driver - such as whether to use Avg or Sum binning in the case of the
IMX219 driver. However, the driver already uses binning to expose the
various modes of operation. At the same time any implemntation of the
selection API would require to modify binning, if not we will have to
use the binning set by the current mode - which too seems implicit and a
bit restrictive. 
> 
> Laurent: are there plans to introduce internal pad for embedded data
> support for this sensor ?
> 
> Also, the patch doesn't apply on the most recent media master, please
> rebase before resending.
> 
> On Mon, Jan 08, 2024 at 08:44:59AM +0000, Sakari Ailus wrote:
> > Hi Vinay,
> >
> > Thanks for the patch.
> >
> > On Sun, Jan 07, 2024 at 03:42:59PM +0800, Vinay Varma wrote:
> > > This patch exposes IMX219's crop and compose capabilities
> > > by implementing the selection API. Horizontal and vertical
> > > binning being separate registers, `imx219_binning_goodness`
> > > computes the best possible height and width of the compose
> > > specification using the selection flags. Compose operation
> > > updates the subdev's format object to keep them in sync.
> >
> > The line length limit here is 75, not ~ 60. Please rewrap.
> >
> > >
> > > Signed-off-by: Vinay Varma <varmavinaym@gmail.com>
> > > ---
> > >  drivers/media/i2c/imx219.c | 222 +++++++++++++++++++++++++++++++------
> > >  1 file changed, 190 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > index 39943d72c22d..27d85fb7ad51 100644
> > > --- a/drivers/media/i2c/imx219.c
> > > +++ b/drivers/media/i2c/imx219.c
> > > @@ -29,6 +29,7 @@
> > >  #include <media/v4l2-event.h>
> > >  #include <media/v4l2-fwnode.h>
> > >  #include <media/v4l2-mediabus.h>
> > > +#include <media/v4l2-rect.h>
> > >
> > >  /* Chip ID */
> > >  #define IMX219_REG_CHIP_ID		CCI_REG16(0x0000)
> > > @@ -73,6 +74,7 @@
> > >  /* V_TIMING internal */
> > >  #define IMX219_REG_VTS			CCI_REG16(0x0160)
> > >  #define IMX219_VTS_MAX			0xffff
> > > +#define IMX219_VTS_DEF 1763
> > >
> > >  #define IMX219_VBLANK_MIN		32
> > >
> > > @@ -146,6 +148,7 @@
> > >  #define IMX219_PIXEL_ARRAY_TOP		8U
> > >  #define IMX219_PIXEL_ARRAY_WIDTH	3280U
> > >  #define IMX219_PIXEL_ARRAY_HEIGHT	2464U
> > > +#define IMX219_MIN_COMPOSE_SIZE 8U
> >
> > Please align 8U with the rest of the macros. Same above.
> >
> > >
> > >  /* Mode : resolution and related config&values */
> > >  struct imx219_mode {
> > > @@ -284,6 +287,8 @@ static const u32 imx219_mbus_formats[] = {
> > >  #define IMX219_XCLR_MIN_DELAY_US	6200
> > >  #define IMX219_XCLR_DELAY_RANGE_US	1000
> > >
> > > +static const u32 binning_ratios[] = { 1, 2 };
> > > +
> > >  /* Mode configs */
> > >  static const struct imx219_mode supported_modes[] = {
> > >  	{
> > > @@ -296,19 +301,19 @@ static const struct imx219_mode supported_modes[] = {
> > >  		/* 1080P 30fps cropped */
> > >  		.width = 1920,
> > >  		.height = 1080,
> > > -		.vts_def = 1763,
> > > +		.vts_def = IMX219_VTS_DEF,
> > >  	},
> > >  	{
> > >  		/* 2x2 binned 30fps mode */
> > >  		.width = 1640,
> > >  		.height = 1232,
> > > -		.vts_def = 1763,
> > > +		.vts_def = IMX219_VTS_DEF,
> > >  	},
> > >  	{
> > >  		/* 640x480 30fps mode */
> > >  		.width = 640,
> > >  		.height = 480,
> > > -		.vts_def = 1763,
> > > +		.vts_def = IMX219_VTS_DEF,
> > >  	},
> > >  };
> > >
> > > @@ -809,6 +814,39 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> > >  	return 0;
> > >  }
> > >
> > > +static void imx219_refresh_ctrls(struct v4l2_subdev *sd,
> > > +				 struct v4l2_subdev_state *state,
> > > +				 unsigned int vts_def)
> > > +{
> > > +	int exposure_max;
> > > +	int exposure_def;
> > > +	int hblank;
> > > +	struct imx219 *imx219 = to_imx219(sd);
> > > +	struct v4l2_mbus_framefmt *fmt =
> > > +		v4l2_subdev_get_pad_format(sd, state, 0);
> > > +
> > > +	/* Update limits and set FPS to default */
> > > +	__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > > +				 IMX219_VTS_MAX - fmt->height, 1,
> > > +				 vts_def - fmt->height);
> > > +	__v4l2_ctrl_s_ctrl(imx219->vblank, vts_def - fmt->height);
> > > +	/* Update max exposure while meeting expected vblanking */
> > > +	exposure_max = vts_def - 4;
> > > +	exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
> > > +			       exposure_max :
> > > +			       IMX219_EXPOSURE_DEFAULT;
> > > +	__v4l2_ctrl_modify_range(imx219->exposure, imx219->exposure->minimum,
> > > +				 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.
> > > +	 */
> > > +	hblank = IMX219_PPL_DEFAULT - fmt->width;
> > > +	__v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1, hblank);
> > > +}
> > > +
> > >  static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > >  				 struct v4l2_subdev_state *state,
> > >  				 struct v4l2_subdev_format *fmt)
> > > @@ -816,7 +854,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > >  	struct imx219 *imx219 = to_imx219(sd);
> > >  	const struct imx219_mode *mode;
> > >  	struct v4l2_mbus_framefmt *format;
> > > -	struct v4l2_rect *crop;
> > > +	struct v4l2_rect *crop, *compose;
> > >  	unsigned int bin_h, bin_v;
> > >
> > >  	mode = v4l2_find_nearest_size(supported_modes,
> > > @@ -842,34 +880,14 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > >  	crop->left = (IMX219_NATIVE_WIDTH - crop->width) / 2;
> > >  	crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
> > >
> > > -	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > > -		int exposure_max;
> > > -		int exposure_def;
> > > -		int hblank;
> > > -
> > > -		/* Update limits and set FPS to default */
> > > -		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > > -					 IMX219_VTS_MAX - mode->height, 1,
> > > -					 mode->vts_def - mode->height);
> > > -		__v4l2_ctrl_s_ctrl(imx219->vblank,
> > > -				   mode->vts_def - mode->height);
> > > -		/* Update max exposure while meeting expected vblanking */
> > > -		exposure_max = mode->vts_def - 4;
> > > -		exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
> > > -			exposure_max : IMX219_EXPOSURE_DEFAULT;
> > > -		__v4l2_ctrl_modify_range(imx219->exposure,
> > > -					 imx219->exposure->minimum,
> > > -					 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.
> > > -		 */
> > > -		hblank = IMX219_PPL_DEFAULT - mode->width;
> > > -		__v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1,
> > > -					 hblank);
> > > -	}
> > > +	compose = v4l2_subdev_get_pad_compose(sd, state, 0);
> > > +	compose->width = format->width;
> > > +	compose->height = format->height;
> > > +	compose->left = 0;
> > > +	compose->top = 0;
> > > +
> > > +	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > > +		imx219_refresh_ctrls(sd, state, mode->vts_def);
> > >
> > >  	return 0;
> > >  }
> > > @@ -884,6 +902,11 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
> > >  		return 0;
> > >  	}
> > >
> > > +	case V4L2_SEL_TGT_COMPOSE: {
> > > +		sel->r = *v4l2_subdev_get_pad_compose(sd, state, 0);
> > > +		return 0;
> > > +	}
> >
> > The braces are unnecessary here.
> >
> > > +
> > >  	case V4L2_SEL_TGT_NATIVE_SIZE:
> > >  		sel->r.top = 0;
> > >  		sel->r.left = 0;
> > > @@ -900,11 +923,145 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
> > >  		sel->r.height = IMX219_PIXEL_ARRAY_HEIGHT;
> > >
> > >  		return 0;
> > > +
> > > +	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> > > +	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> > > +	case V4L2_SEL_TGT_COMPOSE_PADDED:
> > > +		sel->r.top = 0;
> > > +		sel->r.left = 0;
> > > +		sel->r.width = IMX219_PIXEL_ARRAY_WIDTH;
> > > +		sel->r.height = IMX219_PIXEL_ARRAY_HEIGHT;
> > > +		return 0;
> > >  	}
> > >
> > >  	return -EINVAL;
> > >  }
> > >
> > > +#define IMX219_ROUND(dim, step, flags)                \
> > > +	((flags) & V4L2_SEL_FLAG_GE ?                 \
> > > +		 round_up((dim), (step)) :            \
> > > +		 ((flags) & V4L2_SEL_FLAG_LE ?        \
> > > +			  round_down((dim), (step)) : \
> > > +			  round_down((dim) + (step) / 2, (step))))
> > > +
> > > +static int imx219_set_selection_crop(struct v4l2_subdev *sd,
> > > +				     struct v4l2_subdev_state *sd_state,
> > > +				     struct v4l2_subdev_selection *sel)
> > > +{
> > > +	u32 max_binning;
> > > +	struct v4l2_rect *compose, *crop;
> > > +	struct v4l2_mbus_framefmt *fmt;
> > > +
> > > +	crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > > +	if (v4l2_rect_equal(&sel->r, crop))
> > > +		return false;
> > > +	max_binning = binning_ratios[ARRAY_SIZE(binning_ratios) - 1];
> > > +	sel->r.width =
> > > +		clamp(IMX219_ROUND(sel->r.width, max_binning, sel->flags),
> > > +		      max_binning * IMX219_MIN_COMPOSE_SIZE,
> > > +		      IMX219_PIXEL_ARRAY_WIDTH);
> > > +	sel->r.height =
> > > +		clamp(IMX219_ROUND(sel->r.width, max_binning, sel->flags),
> > > +		      max_binning * IMX219_MIN_COMPOSE_SIZE,
> > > +		      IMX219_PIXEL_ARRAY_WIDTH);
> > > +	sel->r.left =
> > > +		min_t(u32, sel->r.left, IMX219_PIXEL_ARRAY_LEFT - sel->r.width);
> > > +	sel->r.top =
> > > +		min_t(u32, sel->r.top, IMX219_PIXEL_ARRAY_TOP - sel->r.top);
> > > +
> > > +	compose = v4l2_subdev_get_pad_compose(sd, sd_state, 0);
> > > +	fmt = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > > +	*crop = sel->r;
> > > +	compose->height = crop->height;
> > > +	compose->width = crop->width;
> > > +	return true;
> > > +}
> > > +
> > > +static int imx219_binning_goodness(u32 act, u32 ask, u32 flags)
> > > +{
> > > +	const int goodness = 100000;
> > > +	int val = 0;
> > > +
> > > +	if (flags & V4L2_SEL_FLAG_GE)
> > > +		if (act < ask)
> > > +			val -= goodness;
> > > +
> > > +	if (flags & V4L2_SEL_FLAG_LE)
> > > +		if (act > ask)
> > > +			val -= goodness;
> > > +
> > > +	val -= abs(act - ask);
> > > +
> > > +	return val;
> > > +}
> > > +
> > > +static bool imx219_set_selection_compose(struct v4l2_subdev *sd,
> > > +					 struct v4l2_subdev_state *state,
> > > +					 struct v4l2_subdev_selection *sel)
> > > +{
> > > +	int best_goodness;
> >
> > This would be nicer if declared after the line below. Think of reverse
> > Christmas trees.
> >
> > Similarly for max_binning a few functions up actually as well as variables
> > in imx219_refresh_ctrls().
> >
> > > +	struct v4l2_rect *compose, *crop;
> > > +
> > > +	compose = v4l2_subdev_get_pad_compose(sd, state, 0);
> > > +	if (v4l2_rect_equal(compose, &sel->r))
> > > +		return false;
> > > +
> > > +	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> > > +
> > > +	best_goodness = INT_MIN;
> > > +	for (int i = 0; i < ARRAY_SIZE(binning_ratios); ++i) {
> > > +		u32 width = crop->width / binning_ratios[i];
> > > +		int goodness = imx219_binning_goodness(width, sel->r.width,
> > > +						       sel->flags);
> > > +		if (goodness > best_goodness) {
> > > +			best_goodness = goodness;
> > > +			compose->width = width;
> > > +		}
> > > +	}
> > > +	best_goodness = INT_MIN;
> > > +	for (int i = 0; i < ARRAY_SIZE(binning_ratios); ++i) {
> > > +		u32 height = crop->height / binning_ratios[i];
> > > +		int goodness = imx219_binning_goodness(height, sel->r.height,
> > > +						       sel->flags);
> > > +		if (goodness > best_goodness) {
> > > +			best_goodness = goodness;
> > > +			compose->height = height;
> > > +		}
> > > +	}
> > > +	return true;
> > > +}
> > > +
> > > +static int imx219_set_selection(struct v4l2_subdev *sd,
> > > +				struct v4l2_subdev_state *sd_state,
> > > +				struct v4l2_subdev_selection *sel)
> > > +{
> > > +	bool compose_updated = false;
> > > +
> > > +	switch (sel->target) {
> > > +	case V4L2_SEL_TGT_CROP:
> > > +		compose_updated = imx219_set_selection_crop(sd, sd_state, sel);
> > > +		break;
> > > +	case V4L2_SEL_TGT_COMPOSE:
> > > +		compose_updated =
> > > +			imx219_set_selection_compose(sd, sd_state, sel);
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +	if (compose_updated) {
> > > +		struct v4l2_rect *compose =
> > > +			v4l2_subdev_get_pad_compose(sd, sd_state, 0);
> > > +		struct v4l2_mbus_framefmt *fmt =
> > > +			v4l2_subdev_get_pad_format(sd, sd_state, 0);
> >
> > A newline here?
> >
> > > +		fmt->height = compose->height;
> > > +		fmt->width = compose->width;
> > > +	}
> > > +	if (compose_updated && sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > > +		imx219_refresh_ctrls(sd, sd_state, IMX219_VTS_DEF);
> >
> > Please move this inside the previous condition (where you check just
> > sel->which).
> >
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int imx219_init_cfg(struct v4l2_subdev *sd,
> > >  			   struct v4l2_subdev_state *state)
> > >  {
> > > @@ -938,6 +1095,7 @@ static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
> > >  	.get_fmt = v4l2_subdev_get_fmt,
> > >  	.set_fmt = imx219_set_pad_format,
> > >  	.get_selection = imx219_get_selection,
> > > +	.set_selection = imx219_set_selection,
> > >  	.enum_frame_size = imx219_enum_frame_size,
> > >  };
> > >
> >
> > --
> > Regards,
> >
> > Sakari Ailus
> >

This patch exposes the hw's crop and compose capabilities by implementing
the selection API. Horizontal and vertical binning being separate
registers `imx219_binning_goodness` computes the best possible height
and width of the compose specification using the selection flags. Compose
operation updates the subdev's format object to keep them in sync.

Signed-off-by: Vinay Varma <varmavinaym@gmail.com>
---
Changes since v1:
- Rebase on media-master; fix conflicts
- Rewrap commit message to 75 chars
- Fix alignment of a constant
- Remove unnecessary braces
- Reverse christmas tree ordering of variables in new functions
---
 drivers/media/i2c/imx219.c | 223 +++++++++++++++++++++++++++++++------
 1 file changed, 191 insertions(+), 32 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index e17ef2e9d9d0..8a5fee27af45 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -29,6 +29,7 @@
 #include <media/v4l2-event.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-mediabus.h>
+#include <media/v4l2-rect.h>
 
 /* Chip ID */
 #define IMX219_REG_CHIP_ID		CCI_REG16(0x0000)
@@ -73,6 +74,7 @@
 /* V_TIMING internal */
 #define IMX219_REG_VTS			CCI_REG16(0x0160)
 #define IMX219_VTS_MAX			0xffff
+#define IMX219_VTS_DEF			1763
 
 #define IMX219_VBLANK_MIN		4
 
@@ -146,6 +148,7 @@
 #define IMX219_PIXEL_ARRAY_TOP		8U
 #define IMX219_PIXEL_ARRAY_WIDTH	3280U
 #define IMX219_PIXEL_ARRAY_HEIGHT	2464U
+#define IMX219_MIN_COMPOSE_SIZE		8U
 
 /* Mode : resolution and related config&values */
 struct imx219_mode {
@@ -284,6 +287,8 @@ static const u32 imx219_mbus_formats[] = {
 #define IMX219_XCLR_MIN_DELAY_US	6200
 #define IMX219_XCLR_DELAY_RANGE_US	1000
 
+static const u32 binning_ratios[] = { 1, 2 };
+
 /* Mode configs */
 static const struct imx219_mode supported_modes[] = {
 	{
@@ -296,19 +301,19 @@ static const struct imx219_mode supported_modes[] = {
 		/* 1080P 30fps cropped */
 		.width = 1920,
 		.height = 1080,
-		.vts_def = 1763,
+		.vts_def = IMX219_VTS_DEF,
 	},
 	{
 		/* 2x2 binned 30fps mode */
 		.width = 1640,
 		.height = 1232,
-		.vts_def = 1763,
+		.vts_def = IMX219_VTS_DEF,
 	},
 	{
 		/* 640x480 30fps mode */
 		.width = 640,
 		.height = 480,
-		.vts_def = 1763,
+		.vts_def = IMX219_VTS_DEF,
 	},
 };
 
@@ -809,6 +814,38 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static void imx219_refresh_ctrls(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *state,
+				 unsigned int vts_def)
+{
+	struct v4l2_mbus_framefmt *fmt = v4l2_subdev_state_get_format(state, 0);
+	struct imx219 *imx219 = to_imx219(sd);
+	int exposure_max;
+	int exposure_def;
+	int hblank;
+
+	/* Update limits and set FPS to default */
+	__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
+				 IMX219_VTS_MAX - fmt->height, 1,
+				 vts_def - fmt->height);
+	__v4l2_ctrl_s_ctrl(imx219->vblank, vts_def - fmt->height);
+	/* Update max exposure while meeting expected vblanking */
+	exposure_max = vts_def - 4;
+	exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
+			       exposure_max :
+			       IMX219_EXPOSURE_DEFAULT;
+	__v4l2_ctrl_modify_range(imx219->exposure, imx219->exposure->minimum,
+				 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.
+	 */
+	hblank = IMX219_PPL_DEFAULT - fmt->width;
+	__v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1, hblank);
+}
+
 static int imx219_set_pad_format(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *state,
 				 struct v4l2_subdev_format *fmt)
@@ -816,7 +853,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	struct imx219 *imx219 = to_imx219(sd);
 	const struct imx219_mode *mode;
 	struct v4l2_mbus_framefmt *format;
-	struct v4l2_rect *crop;
+	struct v4l2_rect *crop, *compose;
 	unsigned int bin_h, bin_v;
 
 	mode = v4l2_find_nearest_size(supported_modes,
@@ -842,34 +879,14 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	crop->left = (IMX219_NATIVE_WIDTH - crop->width) / 2;
 	crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
 
-	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		int exposure_max;
-		int exposure_def;
-		int hblank;
-
-		/* Update limits and set FPS to default */
-		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
-					 IMX219_VTS_MAX - mode->height, 1,
-					 mode->vts_def - mode->height);
-		__v4l2_ctrl_s_ctrl(imx219->vblank,
-				   mode->vts_def - mode->height);
-		/* Update max exposure while meeting expected vblanking */
-		exposure_max = mode->vts_def - 4;
-		exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
-			exposure_max : IMX219_EXPOSURE_DEFAULT;
-		__v4l2_ctrl_modify_range(imx219->exposure,
-					 imx219->exposure->minimum,
-					 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.
-		 */
-		hblank = IMX219_PPL_DEFAULT - mode->width;
-		__v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1,
-					 hblank);
-	}
+	compose = v4l2_subdev_state_get_compose(state, 0);
+	compose->width = format->width;
+	compose->height = format->height;
+	compose->left = 0;
+	compose->top = 0;
+
+	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+		imx219_refresh_ctrls(sd, state, mode->vts_def);
 
 	return 0;
 }
@@ -884,6 +901,10 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
 		return 0;
 	}
 
+	case V4L2_SEL_TGT_COMPOSE:
+		sel->r = *v4l2_subdev_state_get_compose(state, 0);
+		return 0;
+
 	case V4L2_SEL_TGT_NATIVE_SIZE:
 		sel->r.top = 0;
 		sel->r.left = 0;
@@ -900,11 +921,148 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
 		sel->r.height = IMX219_PIXEL_ARRAY_HEIGHT;
 
 		return 0;
+
+	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
+	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+	case V4L2_SEL_TGT_COMPOSE_PADDED:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = IMX219_PIXEL_ARRAY_WIDTH;
+		sel->r.height = IMX219_PIXEL_ARRAY_HEIGHT;
+		return 0;
 	}
 
 	return -EINVAL;
 }
 
+#define IMX219_ROUND(dim, step, flags)                \
+	((flags) & V4L2_SEL_FLAG_GE ?                 \
+		 round_up((dim), (step)) :            \
+		 ((flags) & V4L2_SEL_FLAG_LE ?        \
+			  round_down((dim), (step)) : \
+			  round_down((dim) + (step) / 2, (step))))
+
+static bool imx219_set_selection_crop(struct v4l2_subdev *sd,
+				      struct v4l2_subdev_state *sd_state,
+				      struct v4l2_subdev_selection *sel)
+{
+	struct v4l2_rect *compose, *crop;
+	struct v4l2_mbus_framefmt *fmt;
+	u32 max_binning;
+
+	crop = v4l2_subdev_state_get_crop(sd_state, 0);
+	if (v4l2_rect_equal(&sel->r, crop))
+		return false;
+	max_binning = binning_ratios[ARRAY_SIZE(binning_ratios) - 1];
+	sel->r.width =
+		clamp(IMX219_ROUND(sel->r.width, max_binning, sel->flags),
+		      max_binning * IMX219_MIN_COMPOSE_SIZE,
+		      IMX219_PIXEL_ARRAY_WIDTH);
+	sel->r.height =
+		clamp(IMX219_ROUND(sel->r.width, max_binning, sel->flags),
+		      max_binning * IMX219_MIN_COMPOSE_SIZE,
+		      IMX219_PIXEL_ARRAY_WIDTH);
+	sel->r.left =
+		min_t(u32, sel->r.left, IMX219_PIXEL_ARRAY_LEFT - sel->r.width);
+	sel->r.top =
+		min_t(u32, sel->r.top, IMX219_PIXEL_ARRAY_TOP - sel->r.top);
+
+	compose = v4l2_subdev_state_get_compose(sd_state, 0);
+	fmt = v4l2_subdev_state_get_format(sd_state, 0);
+	*crop = sel->r;
+	compose->height = crop->height;
+	compose->width = crop->width;
+	return true;
+}
+
+static int imx219_binning_goodness(u32 act, u32 ask, u32 flags)
+{
+	const int goodness = 100000;
+	int val = 0;
+
+	if (flags & V4L2_SEL_FLAG_GE)
+		if (act < ask)
+			val -= goodness;
+
+	if (flags & V4L2_SEL_FLAG_LE)
+		if (act > ask)
+			val -= goodness;
+
+	val -= abs(act - ask);
+
+	return val;
+}
+
+static bool imx219_set_selection_compose(struct v4l2_subdev *sd,
+					 struct v4l2_subdev_state *state,
+					 struct v4l2_subdev_selection *sel)
+{
+	struct v4l2_rect *compose, *crop;
+	int best_goodness;
+
+	compose = v4l2_subdev_state_get_compose(state, 0);
+	if (v4l2_rect_equal(compose, &sel->r))
+		return false;
+
+	crop = v4l2_subdev_state_get_crop(state, 0);
+	best_goodness = INT_MIN;
+	for (int i = 0; i < ARRAY_SIZE(binning_ratios); ++i) {
+		u32 width = crop->width / binning_ratios[i];
+		int goodness = imx219_binning_goodness(width, sel->r.width,
+						       sel->flags);
+
+		if (goodness > best_goodness) {
+			best_goodness = goodness;
+			compose->width = width;
+		}
+	}
+	best_goodness = INT_MIN;
+	for (int i = 0; i < ARRAY_SIZE(binning_ratios); ++i) {
+		u32 height = crop->height / binning_ratios[i];
+		int goodness = imx219_binning_goodness(height, sel->r.height,
+						       sel->flags);
+
+		if (goodness > best_goodness) {
+			best_goodness = goodness;
+			compose->height = height;
+		}
+	}
+	sel->r = *compose;
+	return true;
+}
+
+static int imx219_set_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *sd_state,
+				struct v4l2_subdev_selection *sel)
+{
+	bool compose_updated = false;
+
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP:
+		compose_updated = imx219_set_selection_crop(sd, sd_state, sel);
+		break;
+	case V4L2_SEL_TGT_COMPOSE:
+		compose_updated =
+			imx219_set_selection_compose(sd, sd_state, sel);
+		break;
+	default:
+		return -EINVAL;
+	}
+	if (compose_updated) {
+		struct v4l2_rect *compose =
+			v4l2_subdev_state_get_compose(sd_state, 0);
+		struct v4l2_mbus_framefmt *fmt =
+			v4l2_subdev_state_get_format(sd_state, 0);
+
+		fmt->height = compose->height;
+		fmt->width = compose->width;
+		if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+			imx219_refresh_ctrls(sd, sd_state, IMX219_VTS_DEF);
+	}
+
+	return 0;
+}
+
 static int imx219_init_state(struct v4l2_subdev *sd,
 			     struct v4l2_subdev_state *state)
 {
@@ -937,6 +1095,7 @@ static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
 	.get_fmt = v4l2_subdev_get_fmt,
 	.set_fmt = imx219_set_pad_format,
 	.get_selection = imx219_get_selection,
+	.set_selection = imx219_set_selection,
 	.enum_frame_size = imx219_enum_frame_size,
 };
 
-- 
2.43.0

Hi Vinay,

kernel test robot noticed the following build warnings:

[auto build test WARNING on media-tree/master]
[also build test WARNING on linuxtv-media-stage/master next-20240110]
[cannot apply to sailus-media-tree/streams linus/master v6.7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Vinay-Varma/media-i2c-imx219-implement-v4l2-selection-api/20240109-115310
base:   git://linuxtv.org/media_tree.git master
patch link:    https://lore.kernel.org/r/20240109035045.552097-1-varmavinaym%40gmail.com
patch subject: [PATCH v2] media: i2c: imx219: implement v4l2 selection api
config: x86_64-allmodconfig (https://download.01.org/0day-ci/archive/20240111/202401110518.L0kScfAo-lkp@intel.com/config)
compiler: ClangBuiltLinux clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240111/202401110518.L0kScfAo-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401110518.L0kScfAo-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/media/i2c/imx219.c:950:29: warning: variable 'fmt' set but not used [-Wunused-but-set-variable]
     950 |         struct v4l2_mbus_framefmt *fmt;
         |                                    ^
   1 warning generated.


vim +/fmt +950 drivers/media/i2c/imx219.c

   937	
   938	#define IMX219_ROUND(dim, step, flags)                \
   939		((flags) & V4L2_SEL_FLAG_GE ?                 \
   940			 round_up((dim), (step)) :            \
   941			 ((flags) & V4L2_SEL_FLAG_LE ?        \
   942				  round_down((dim), (step)) : \
   943				  round_down((dim) + (step) / 2, (step))))
   944	
   945	static bool imx219_set_selection_crop(struct v4l2_subdev *sd,
   946					      struct v4l2_subdev_state *sd_state,
   947					      struct v4l2_subdev_selection *sel)
   948	{
   949		struct v4l2_rect *compose, *crop;
 > 950		struct v4l2_mbus_framefmt *fmt;
   951		u32 max_binning;
   952	
   953		crop = v4l2_subdev_state_get_crop(sd_state, 0);
   954		if (v4l2_rect_equal(&sel->r, crop))
   955			return false;
   956		max_binning = binning_ratios[ARRAY_SIZE(binning_ratios) - 1];
   957		sel->r.width =
   958			clamp(IMX219_ROUND(sel->r.width, max_binning, sel->flags),
   959			      max_binning * IMX219_MIN_COMPOSE_SIZE,
   960			      IMX219_PIXEL_ARRAY_WIDTH);
   961		sel->r.height =
   962			clamp(IMX219_ROUND(sel->r.width, max_binning, sel->flags),
   963			      max_binning * IMX219_MIN_COMPOSE_SIZE,
   964			      IMX219_PIXEL_ARRAY_WIDTH);
   965		sel->r.left =
   966			min_t(u32, sel->r.left, IMX219_PIXEL_ARRAY_LEFT - sel->r.width);
   967		sel->r.top =
   968			min_t(u32, sel->r.top, IMX219_PIXEL_ARRAY_TOP - sel->r.top);
   969	
   970		compose = v4l2_subdev_state_get_compose(sd_state, 0);
   971		fmt = v4l2_subdev_state_get_format(sd_state, 0);
   972		*crop = sel->r;
   973		compose->height = crop->height;
   974		compose->width = crop->width;
   975		return true;
   976	}
   977	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki