diff mbox series

[v3,17/20] media: i2c: ov4689: Configurable analogue crop

Message ID 20240229165333.227484-18-mike.rudenko@gmail.com (mailing list archive)
State New, archived
Headers show
Series Omnivision OV4689 refactoring and improvements | expand

Commit Message

Mikhail Rudenko Feb. 29, 2024, 4:53 p.m. UTC
Implement configurable analogue crop via .set_selectiong call.
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@gmail.com>
---
 drivers/media/i2c/ov4689.c | 276 ++++++++++++++++++++++++++++---------
 1 file changed, 212 insertions(+), 64 deletions(-)

Comments

Kieran Bingham March 5, 2024, 2:27 p.m. UTC | #1
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@gmail.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@ideasonboard.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
>
Mikhail Rudenko March 5, 2024, 2:35 p.m. UTC | #2
Hi Kieran,

and thanks for the review!

On 2024-03-05 at 14:27 GMT, Kieran Bingham <kieran.bingham@ideasonboard.com> 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 <mike.rudenko@gmail.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:

Thank you for understanding!

>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.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
>>


--
Best regards,
Mikhail Rudenko
diff mbox series

Patch

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 =