| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <170964885738.362031.275064492290886570@ping.linuxembedded.co.uk>
Date: Tue, 05 Mar 2024 14:27:37 +0000
From: Kieran Bingham <kieran.bingham@...asonboard.com>
To: Mikhail Rudenko <mike.rudenko@...il.com>, linux-kernel@...r.kernel.org, linux-media@...r.kernel.org
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>, Laurent Pinchart <laurent.pinchart@...asonboard.com>, Jacopo Mondi <jacopo@...ndi.org>, Tommaso Merciai <tomm.merciai@...il.com>, Christophe JAILLET <christophe.jaillet@...adoo.fr>, Dave Stevenson <dave.stevenson@...pberrypi.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, Mikhail Rudenko <mike.rudenko@...il.com>
Subject: Re: [PATCH v3 17/20] media: i2c: ov4689: Configurable analogue crop
Hi Mikhail,
Quoting Mikhail Rudenko (2024-02-29 16:53:30)
> Implement configurable analogue crop via .set_selectiong call.
s/.set_selectiong/.set_selection/
> ov4689_init_cfg is modified to initialize default subdev selection.
> Offsets are aligned to 2 to preserve Bayer order, selection width is
> aligned to 4 and height to 2 to meet hardware requirements.
>
> Experimentally discovered values of the cropping-related registers and
> vfifo_read_start for various output sizes are used. Default BLC anchor
> positions are used for the default analogue crop, scaling down
> proportionally for the smaller crop sizes.
>
> When analogue crop is adjusted, several consequential actions take
> place: the output format is reset, exposure/vblank/hblank control
> ranges and default values are adjusted accordingly. Additionally,
> ov4689_set_ctrl utilizes pad crop instead of cur_mode width and
> height for HTS and VTS calculation. Also, ov4689_enum_frame_sizes is
> modified to report crop size as available frame size.
>
> Signed-off-by: Mikhail Rudenko <mike.rudenko@...il.com>
As discussed privately, this is hard to do a direct review of the
underlying detailed changes and I don't specifically have this hardware
to test. But I understand and trust the hours you've put in to testing
this, and I can't see anything /wrong/ in this patch - so therefore:
Reviewed-by: Kieran Bingham <kieran.bingham@...asonboard.com>
> ---
> drivers/media/i2c/ov4689.c | 276 ++++++++++++++++++++++++++++---------
> 1 file changed, 212 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c
> index d34699f35270..06f202c12dff 100644
> --- a/drivers/media/i2c/ov4689.c
> +++ b/drivers/media/i2c/ov4689.c
> @@ -45,8 +45,12 @@
> #define OV4689_REG_V_CROP_START CCI_REG16(0x3802)
> #define OV4689_REG_H_CROP_END CCI_REG16(0x3804)
> #define OV4689_REG_V_CROP_END CCI_REG16(0x3806)
> +
> #define OV4689_REG_H_OUTPUT_SIZE CCI_REG16(0x3808)
> +#define OV4689_H_OUTPUT_SIZE_DEFAULT 2688
> +
> #define OV4689_REG_V_OUTPUT_SIZE CCI_REG16(0x380a)
> +#define OV4689_V_OUTPUT_SIZE_DEFAULT 1520
>
> #define OV4689_REG_HTS CCI_REG16(0x380c)
> #define OV4689_HTS_DIVIDER 4
> @@ -96,6 +100,19 @@
> #define OV4689_DUMMY_ROWS 8 /* 8 dummy rows on each side */
> #define OV4689_DUMMY_COLUMNS 16 /* 16 dummy columns on each side */
>
> +/*
> + * These values are not hardware limits, but rather the minimums that
> + * the driver has been tested to.
> + */
> +#define OV4689_H_CROP_MIN 128
> +#define OV4689_V_CROP_MIN 128
> +
> +/*
> + * Minimum working vertical blanking value. Found experimentally at
> + * minimum HTS values.
> + */
> +#define OV4689_VBLANK_MIN 31
> +
> static const char *const ov4689_supply_names[] = {
> "avdd", /* Analog power */
> "dovdd", /* Digital I/O power */
> @@ -134,7 +151,7 @@ struct ov4689 {
> u32 clock_rate;
>
> struct v4l2_ctrl_handler ctrl_handler;
> - struct v4l2_ctrl *exposure;
> + struct v4l2_ctrl *exposure, *hblank, *vblank;
>
> const struct ov4689_mode *cur_mode;
> };
> @@ -320,24 +337,27 @@ static const struct ov4689_gain_range ov4689_gain_ranges[] = {
> },
> };
>
> -static void ov4689_fill_fmt(const struct ov4689_mode *mode,
> - struct v4l2_mbus_framefmt *fmt)
> -{
> - fmt->code = MEDIA_BUS_FMT_SBGGR10_1X10;
> - fmt->width = mode->width;
> - fmt->height = mode->height;
> - fmt->field = V4L2_FIELD_NONE;
> -}
> -
> static int ov4689_set_fmt(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_format *fmt)
> {
> - struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> - struct ov4689 *ov4689 = to_ov4689(sd);
> + struct v4l2_mbus_framefmt *format;
> + struct v4l2_rect *crop;
> +
> + crop = v4l2_subdev_state_get_crop(sd_state, fmt->pad);
> + format = v4l2_subdev_state_get_format(sd_state, fmt->pad);
> +
> + format->width = crop->width;
> + format->height = crop->height;
>
> - /* only one mode supported for now */
> - ov4689_fill_fmt(ov4689->cur_mode, mbus_fmt);
> + format->code = MEDIA_BUS_FMT_SBGGR10_1X10;
> + format->field = V4L2_FIELD_NONE;
> + format->colorspace = V4L2_COLORSPACE_RAW;
> + format->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> + format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> + format->xfer_func = V4L2_XFER_FUNC_NONE;
> +
> + fmt->format = *format;
>
> return 0;
> }
> @@ -357,16 +377,20 @@ static int ov4689_enum_frame_sizes(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_frame_size_enum *fse)
> {
> - if (fse->index >= ARRAY_SIZE(supported_modes))
> + const struct v4l2_rect *crop;
> +
> + if (fse->index >= 1)
> return -EINVAL;
>
> if (fse->code != MEDIA_BUS_FMT_SBGGR10_1X10)
> return -EINVAL;
>
> - fse->min_width = supported_modes[fse->index].width;
> - fse->max_width = supported_modes[fse->index].width;
> - fse->max_height = supported_modes[fse->index].height;
> - fse->min_height = supported_modes[fse->index].height;
> + crop = v4l2_subdev_state_get_crop(sd_state, 0);
> +
> + fse->min_width = crop->width;
> + fse->max_width = crop->width;
> + fse->max_height = crop->height;
> + fse->min_height = crop->height;
>
> return 0;
> }
> @@ -388,20 +412,14 @@ static int ov4689_get_selection(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *state,
> struct v4l2_subdev_selection *sel)
> {
> - if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> - return -EINVAL;
> -
> switch (sel->target) {
> - case V4L2_SEL_TGT_CROP_BOUNDS:
> - sel->r.top = 0;
> - sel->r.left = 0;
> - sel->r.width = OV4689_PIXEL_ARRAY_WIDTH;
> - sel->r.height = OV4689_PIXEL_ARRAY_HEIGHT;
> - return 0;
> case V4L2_SEL_TGT_CROP:
> + sel->r = *v4l2_subdev_state_get_crop(state, sel->pad);
> + return 0;
> + case V4L2_SEL_TGT_CROP_BOUNDS:
> case V4L2_SEL_TGT_CROP_DEFAULT:
> - sel->r.top = OV4689_DUMMY_ROWS;
> sel->r.left = OV4689_DUMMY_COLUMNS;
> + sel->r.top = OV4689_DUMMY_ROWS;
> sel->r.width =
> OV4689_PIXEL_ARRAY_WIDTH - 2 * OV4689_DUMMY_COLUMNS;
> sel->r.height =
> @@ -412,37 +430,141 @@ static int ov4689_get_selection(struct v4l2_subdev *sd,
> return -EINVAL;
> }
>
> -static int ov4689_setup_timings(struct ov4689 *ov4689)
> +/*
> + * Minimum working HTS value for given output width (found
> + * experimentally).
> + */
> +static unsigned int ov4689_hts_min(unsigned int width)
> +{
> + return max_t(unsigned int, 3156, 224 + width * 19 / 16);
> +}
> +
> +static void ov4689_update_ctrl_ranges(struct ov4689 *ov4689,
> + struct v4l2_rect *crop)
> +{
> + struct v4l2_ctrl *exposure = ov4689->exposure;
> + struct v4l2_ctrl *vblank = ov4689->vblank;
> + struct v4l2_ctrl *hblank = ov4689->hblank;
> + s64 def_val, min_val, max_val;
> +
> + min_val = ov4689_hts_min(crop->width) - crop->width;
> + max_val = OV4689_HTS_MAX - crop->width;
> + def_val = clamp_t(s64, hblank->default_value, min_val, max_val);
> + __v4l2_ctrl_modify_range(hblank, min_val, max_val, hblank->step,
> + def_val);
> +
> + min_val = OV4689_VBLANK_MIN;
> + max_val = OV4689_HTS_MAX - crop->width;
> + def_val = clamp_t(s64, vblank->default_value, min_val, max_val);
> + __v4l2_ctrl_modify_range(vblank, min_val, max_val, vblank->step,
> + def_val);
> +
> + min_val = exposure->minimum;
> + max_val = crop->height + vblank->val - 4;
> + def_val = clamp_t(s64, exposure->default_value, min_val, max_val);
> + __v4l2_ctrl_modify_range(exposure, min_val, max_val, exposure->step,
> + def_val);
> +}
> +
> +static int ov4689_set_selection(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + struct v4l2_subdev_selection *sel)
> {
> - const struct ov4689_mode *mode = ov4689->cur_mode;
> + struct ov4689 *ov4689 = to_ov4689(sd);
> + struct v4l2_mbus_framefmt *format;
> + struct v4l2_rect *crop;
> + struct v4l2_rect rect;
> +
> + if (sel->target != V4L2_SEL_TGT_CROP)
> + return -EINVAL;
> +
> + rect.left = clamp(ALIGN(sel->r.left, 2), OV4689_DUMMY_COLUMNS,
> + OV4689_PIXEL_ARRAY_WIDTH);
> + rect.top = clamp(ALIGN(sel->r.top, 2), OV4689_DUMMY_ROWS,
> + OV4689_PIXEL_ARRAY_HEIGHT);
> +
> + rect.width = clamp_t(unsigned int, ALIGN(sel->r.width, 4),
> + OV4689_H_CROP_MIN, OV4689_PIXEL_ARRAY_WIDTH -
> + 2 * OV4689_DUMMY_COLUMNS);
> + rect.height = clamp_t(unsigned int, ALIGN(sel->r.height, 2),
> + OV4689_V_CROP_MIN, OV4689_PIXEL_ARRAY_HEIGHT -
> + 2 * OV4689_DUMMY_ROWS);
> +
> + crop = v4l2_subdev_state_get_crop(state, sel->pad);
> +
> + if (rect.width != crop->width || rect.height != crop->height) {
> + /*
> + * Reset the output image size if the crop rectangle size has
> + * been modified.
> + */
> + format = v4l2_subdev_state_get_format(state, sel->pad);
> + format->width = rect.width;
> + format->height = rect.height;
> +
> + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> + ov4689_update_ctrl_ranges(ov4689, &rect);
> + }
> +
> + *crop = rect;
> + sel->r = rect;
> +
> + return 0;
> +}
> +
> +static int ov4689_setup_timings(struct ov4689 *ov4689,
> + struct v4l2_subdev_state *state)
> +{
> + const struct v4l2_mbus_framefmt *format;
> struct regmap *rm = ov4689->regmap;
> + const struct v4l2_rect *crop;
> int ret = 0;
>
> - cci_write(rm, OV4689_REG_H_CROP_START, 8, &ret);
> - cci_write(rm, OV4689_REG_V_CROP_START, 8, &ret);
> - cci_write(rm, OV4689_REG_H_CROP_END, 2711, &ret);
> - cci_write(rm, OV4689_REG_V_CROP_END, 1531, &ret);
> + format = v4l2_subdev_state_get_format(state, 0);
> + crop = v4l2_subdev_state_get_crop(state, 0);
> +
> + cci_write(rm, OV4689_REG_H_CROP_START, crop->left, &ret);
> + cci_write(rm, OV4689_REG_V_CROP_START, crop->top, &ret);
> + cci_write(rm, OV4689_REG_H_CROP_END, crop->left + crop->width + 1, &ret);
> + cci_write(rm, OV4689_REG_V_CROP_END, crop->top + crop->height + 1, &ret);
>
> - cci_write(rm, OV4689_REG_H_OUTPUT_SIZE, mode->width, &ret);
> - cci_write(rm, OV4689_REG_V_OUTPUT_SIZE, mode->height, &ret);
> + cci_write(rm, OV4689_REG_H_OUTPUT_SIZE, format->width, &ret);
> + cci_write(rm, OV4689_REG_V_OUTPUT_SIZE, format->height, &ret);
>
> - cci_write(rm, OV4689_REG_H_WIN_OFF, 8, &ret);
> - cci_write(rm, OV4689_REG_V_WIN_OFF, 4, &ret);
> + cci_write(rm, OV4689_REG_H_WIN_OFF, 0, &ret);
> + cci_write(rm, OV4689_REG_V_WIN_OFF, 0, &ret);
>
> - cci_write(rm, OV4689_REG_VFIFO_CTRL_01, 167, &ret);
> + /*
> + * Maximum working value of vfifo_read_start for given output
> + * width (found experimentally).
> + */
> + cci_write(rm, OV4689_REG_VFIFO_CTRL_01, format->width / 16 - 1, &ret);
>
> return ret;
> }
>
> -static int ov4689_setup_blc_anchors(struct ov4689 *ov4689)
> +/*
> + * Setup black level compensation anchors. For the default frame width
> + * default anchors positions are used. For smaller crop sizes they are
> + * scaled accordingly.
> + */
> +static int ov4689_setup_blc_anchors(struct ov4689 *ov4689,
> + struct v4l2_subdev_state *state)
> {
> + unsigned int width_def = OV4689_H_OUTPUT_SIZE_DEFAULT;
> struct regmap *rm = ov4689->regmap;
> + const struct v4l2_rect *crop;
> int ret = 0;
>
> - cci_write(rm, OV4689_REG_ANCHOR_LEFT_START, 16, &ret);
> - cci_write(rm, OV4689_REG_ANCHOR_LEFT_END, 1999, &ret);
> - cci_write(rm, OV4689_REG_ANCHOR_RIGHT_START, 2400, &ret);
> - cci_write(rm, OV4689_REG_ANCHOR_RIGHT_END, 2415, &ret);
> + crop = v4l2_subdev_state_get_crop(state, 0);
> +
> + cci_write(rm, OV4689_REG_ANCHOR_LEFT_START,
> + OV4689_ANCHOR_LEFT_START_DEF * crop->width / width_def, &ret);
> + cci_write(rm, OV4689_REG_ANCHOR_LEFT_END,
> + OV4689_ANCHOR_LEFT_END_DEF * crop->width / width_def, &ret);
> + cci_write(rm, OV4689_REG_ANCHOR_RIGHT_START,
> + OV4689_ANCHOR_RIGHT_START_DEF * crop->width / width_def, &ret);
> + cci_write(rm, OV4689_REG_ANCHOR_RIGHT_END,
> + OV4689_ANCHOR_RIGHT_END_DEF * crop->width / width_def, &ret);
>
> return ret;
> }
> @@ -470,13 +592,13 @@ static int ov4689_s_stream(struct v4l2_subdev *sd, int on)
> goto unlock_and_return;
> }
>
> - ret = ov4689_setup_timings(ov4689);
> + ret = ov4689_setup_timings(ov4689, sd_state);
> if (ret) {
> pm_runtime_put(dev);
> goto unlock_and_return;
> }
>
> - ret = ov4689_setup_blc_anchors(ov4689);
> + ret = ov4689_setup_blc_anchors(ov4689, sd_state);
> if (ret) {
> pm_runtime_put(dev);
> goto unlock_and_return;
> @@ -568,10 +690,25 @@ static int __maybe_unused ov4689_power_off(struct device *dev)
> static int ov4689_init_state(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state)
> {
> - struct v4l2_mbus_framefmt *fmt =
> - v4l2_subdev_state_get_format(sd_state, 0);
> + u32 width_def = OV4689_H_OUTPUT_SIZE_DEFAULT;
> + u32 height_def = OV4689_V_OUTPUT_SIZE_DEFAULT;
> +
> + struct v4l2_subdev_selection sel = {
> + .target = V4L2_SEL_TGT_CROP,
> + .r.left = OV4689_DUMMY_COLUMNS,
> + .r.top = OV4689_DUMMY_ROWS,
> + .r.width = width_def,
> + .r.height = height_def,
> + };
> + struct v4l2_subdev_format format = {
> + .format = {
> + .width = width_def,
> + .height = height_def,
> + },
> + };
>
> - ov4689_fill_fmt(&supported_modes[OV4689_MODE_2688_1520], fmt);
> + ov4689_set_selection(sd, sd_state, &sel);
> + ov4689_set_fmt(sd, sd_state, &format);
>
> return 0;
> }
> @@ -590,6 +727,7 @@ static const struct v4l2_subdev_pad_ops ov4689_pad_ops = {
> .get_fmt = v4l2_subdev_get_fmt,
> .set_fmt = ov4689_set_fmt,
> .get_selection = ov4689_get_selection,
> + .set_selection = ov4689_set_selection,
> };
>
> static const struct v4l2_subdev_internal_ops ov4689_internal_ops = {
> @@ -635,20 +773,28 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl)
> struct ov4689 *ov4689 =
> container_of(ctrl->handler, struct ov4689, ctrl_handler);
> struct regmap *regmap = ov4689->regmap;
> + struct v4l2_subdev_state *sd_state;
> struct device *dev = ov4689->dev;
> + struct v4l2_rect *crop;
> + s64 max_expo, def_expo;
> int sensor_gain;
> - s64 max_expo;
> int ret = 0;
>
> + sd_state = v4l2_subdev_get_locked_active_state(&ov4689->subdev);
> + crop = v4l2_subdev_state_get_crop(sd_state, 0);
> +
> /* Propagate change of current control to all related controls */
> switch (ctrl->id) {
> case V4L2_CID_VBLANK:
> /* Update max exposure while meeting expected vblanking */
> - max_expo = ov4689->cur_mode->height + ctrl->val - 4;
> - __v4l2_ctrl_modify_range(ov4689->exposure,
> - ov4689->exposure->minimum, max_expo,
> - ov4689->exposure->step,
> - ov4689->exposure->default_value);
> + max_expo = crop->height + ctrl->val - 4;
> + def_expo = clamp_t(s64, ov4689->exposure->default_value,
> + ov4689->exposure->minimum, max_expo);
> +
> + ret = __v4l2_ctrl_modify_range(ov4689->exposure,
> + ov4689->exposure->minimum,
> + max_expo, ov4689->exposure->step,
> + def_expo);
> break;
> }
>
> @@ -666,14 +812,14 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl)
> break;
> case V4L2_CID_VBLANK:
> cci_write(regmap, OV4689_REG_VTS,
> - ctrl->val + ov4689->cur_mode->height, &ret);
> + ctrl->val + crop->height, &ret);
> break;
> case V4L2_CID_TEST_PATTERN:
> ret = ov4689_enable_test_pattern(ov4689, ctrl->val);
> break;
> case V4L2_CID_HBLANK:
> cci_write(regmap, OV4689_REG_HTS,
> - (ctrl->val + ov4689->cur_mode->width) /
> + (ctrl->val + crop->width) /
> OV4689_HTS_DIVIDER, &ret);
> break;
> case V4L2_CID_VFLIP:
> @@ -739,14 +885,16 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689)
>
> hblank_def = mode->hts_def - mode->width;
> hblank_min = mode->hts_min - mode->width;
> - v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_HBLANK,
> - hblank_min, OV4689_HTS_MAX - mode->width,
> - OV4689_HTS_DIVIDER, hblank_def);
> + ov4689->hblank = v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops,
> + V4L2_CID_HBLANK, hblank_min,
> + OV4689_HTS_MAX - mode->width,
> + OV4689_HTS_DIVIDER, hblank_def);
>
> vblank_def = mode->vts_def - mode->height;
> - v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_VBLANK,
> - vblank_def, OV4689_VTS_MAX - mode->height, 1,
> - vblank_def);
> + ov4689->vblank = v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops,
> + V4L2_CID_VBLANK, OV4689_VBLANK_MIN,
> + OV4689_VTS_MAX - mode->height, 1,
> + vblank_def);
>
> exposure_max = mode->vts_def - 4;
> ov4689->exposure =
> --
> 2.43.0
>
Powered by blists - more mailing lists