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

From: Jacopo Mondi
Date: Mon Jan 08 2024 - 04:19:56 EST

Next message: Sandeep C S: "[HID Patchsets for Samsung driver v2 0/6] HID Support for Samsung driver"
Previous message: Ingo Molnar: "Re: [PATCH v2 2/3] objtool: make objtool SLS validation fatal when building with CONFIG_SLS=y"
In reply to: Sakari Ailus: "Re: [PATCH] media: i2c: imx219: implement the v4l2 selection api"
Next in thread: Vinay Varma: "[PATCH v2] media: i2c: imx219: implement v4l2 selection api"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx>
> > ---
> > 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
>

Next message: Sandeep C S: "[HID Patchsets for Samsung driver v2 0/6] HID Support for Samsung driver"
Previous message: Ingo Molnar: "Re: [PATCH v2 2/3] objtool: make objtool SLS validation fatal when building with CONFIG_SLS=y"
In reply to: Sakari Ailus: "Re: [PATCH] media: i2c: imx219: implement the v4l2 selection api"
Next in thread: Vinay Varma: "[PATCH v2] media: i2c: imx219: implement v4l2 selection api"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]