Messages in this thread Patch in this message | | | Date | Tue, 31 May 2022 12:50:11 +0200 | From | Jacopo Mondi <> | Subject | Re: [PATCH v5 4/4] media: i2c: ov5675: add .get_selection support |
| |
Hi Quentin
On Wed, May 25, 2022 at 04:58:33PM +0200, Quentin Schulz wrote: > From: Quentin Schulz <quentin.schulz@theobroma-systems.com> > > The sensor has 2592*1944 active pixels, surrounded by 16 active dummy > pixels and there are an additional 24 black rows "at the bottom". > > [2624] > +-----+------------------+-----+ > | | 16 dummy | | > +-----+------------------+-----+ > | | | | > | | [2592] | | > | | | | > |16 | valid | 16 |[2000] > |dummy| |dummy| > | | [1944]| | > | | | | > +-----+------------------+-----+ > | | 16 dummy | | > +-----+------------------+-----+ > | | 24 black lines | | > +-----+------------------+-----+ > > The top-left coordinate is gotten from the registers specified in the > modes which are identical for both currently supported modes. > > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> > --- > > v4: > - explicit a bit more the commit log, > - added drawing in the commit log, > - fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help, > > added in v3 > > drivers/media/i2c/ov5675.c | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c > index c1f3c387afde0..384a9ea2372c3 100644 > --- a/drivers/media/i2c/ov5675.c > +++ b/drivers/media/i2c/ov5675.c > @@ -1121,6 +1121,38 @@ static int ov5675_get_format(struct v4l2_subdev *sd, > return 0; > } > > +static int ov5675_get_selection(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_selection *sel) > +{ > + struct ov5675 *ov5675 = to_ov5675(sd); > + > + 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 = 2624; > + sel->r.height = 2000; > + return 0; > + case V4L2_SEL_TGT_CROP: > + sel->r.top = 16; > + sel->r.left = 16; > + sel->r.width = ov5675->cur_mode->width; > + sel->r.height = ov5675->cur_mode->height; > + return 0;
I'm afraid this doesn't match exactly my understanding of the discussion we had.
The driver defines the following modes
/* * OV5670 sensor supports following resolutions with full FOV: * 4:3 ==> {2592x1944, 1296x972, 648x486} * 16:9 ==> {2560x1440, 1280x720, 640x360} */ static const struct ov5670_mode supported_modes[] = { { .width = 2592, .height = 1944, }, { .width = 1296, .height = 972, }, { .width = 648, .height = 486, }, { .width = 2560, .height = 1440, }, { .width = 1280, .height = 720, }, { .width = 640, .height = 360, } };
The comment says all modes retain the "full FOV", which I assume it implies they are obtained by sub-sampling and not cropping.
The first three modes (4:3) are indeed obtained by subsampling the full active pixel array:
(2592,1944) / 2 = (1296,972) / 2 = (648,486)
The last three are obtained by subsampling a slightly cropped portion of the pixel array
(2560,1440) / 2 = (1280,720) / 2 = (640,360)
If you set CROP = cur_mode->[width/height] you will instead report the visible width/height, which as said it's obtained by subsampling (of a slightly cropped portion of the pixel array for the last three ones)
The CROP rectangle is then (2592, 1944) for the first three and (2560, 1440) for the last three.
I would add a v4l2_rect to struct ov5670_mode where to record that and report it here.
> + case V4L2_SEL_TGT_CROP_DEFAULT: > + sel->r.top = 16; > + sel->r.left = 16; > + sel->r.width = supported_modes[0].width; > + sel->r.height = supported_modes[0].height; > + return 0;
You could also define these values instead of fishing in the supported_modes array, to protect against future changes to the array itself. Up to you.
> + } > + return -EINVAL; > +} > + > static int ov5675_enum_mbus_code(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_mbus_code_enum *code) > @@ -1170,6 +1202,7 @@ static const struct v4l2_subdev_video_ops ov5675_video_ops = { > static const struct v4l2_subdev_pad_ops ov5675_pad_ops = { > .set_fmt = ov5675_set_format, > .get_fmt = ov5675_get_format, > + .get_selection = ov5675_get_selection, > .enum_mbus_code = ov5675_enum_mbus_code, > .enum_frame_size = ov5675_enum_frame_size, > }; > -- > 2.36.1 >
| |