by Mikhail Rudenko

[permalink] [raw]

Subject: Re: [PATCH v3 17/20] media: i2c: ov4689: Configurable analogue crop

Hi Kieran,

and thanks for the review!

On 2024-03-05 at 14:27 GMT, Kieran Bingham <[email protected]> wrote:

> Hi Mikhail,
>
> Quoting Mikhail Rudenko (2024-02-29 16:53:30)
>> Implement configurable analogue crop via .set_selectiong call.
>
> s/.set_selectiong/.set_selection/

Ack, will fix in v4.

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

Thank you for understanding!

>
> Reviewed-by: Kieran Bingham <[email protected]>
>

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

--
Best regards,
Mikhail Rudenko