2024-01-07 07:43:15

by Vinay Varma

Subject: [PATCH] media: i2c: imx219: implement the v4l2 selection api

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 <[email protected]>
---
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

2024-01-08 08:45:16

by Sakari Ailus

Subject: Re: [PATCH] media: i2c: imx219: implement the v4l2 selection api

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 <[email protected]>
> ---
> 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

2024-01-08 09:30:38

by Jacopo Mondi

Subject: Re: [PATCH] media: i2c: imx219: implement the v4l2 selection api

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 <[email protected]>
> > ---
> > 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
>

2024-01-09 03:53:31

by Vinay Varma

Subject: [PATCH v2] media: i2c: imx219: implement v4l2 selection api

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 <[email protected]>
---
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

2024-01-09 04:34:01

by Vinay Varma

Subject: Re: [PATCH] media: i2c: imx219: implement the v4l2 selection api

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 <[email protected]>
> > > ---
> > > 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
> >

2024-01-10 21:53:48

by kernel test robot

Subject: Re: [PATCH v2] media: i2c: imx219: implement v4l2 selection api

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/[email protected]/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/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

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

2024-01-16 19:10:20

by Sakari Ailus

Subject: Re: [PATCH] media: i2c: imx219: implement the v4l2 selection api

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

2024-01-16 23:01:47

by Laurent Pinchart

Subject: Re: [PATCH] media: i2c: imx219: implement the v4l2 selection api

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