| Message ID | 20220525145833.1165437-4-foss+kernel@0leil.net |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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 >
Hi Quentin On Tue, May 31, 2022 at 02:19:21PM +0200, Quentin Schulz wrote: > Hi Jacopo, > > On 5/31/22 12:50, Jacopo Mondi wrote: > > 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. > > > > That makes a lot of sense to me, thanks for your patience and explanations. > > FYI, you're looking at the wrong driver (ov5670 vs ov5675; a mistake I make You know what's depressing ? -I- have a series out for ov5670 :( I'm so sorry, my brain got short-circuited by that probably... > every now and then too :) ). However, the datasheet does say that "The > OV5675 supports a binning mode to provide a lower resolution output while > maintaining the field of view.[...] The OV5675 supports 2x2 binning." so I > assume we're in the same scenario as you just explained. > > Since the OV5675 modes currently supported by the drivers are 4/3 only and > the smaller size mode a result of subsampling, they both have the same CROP > rectangle. > Thankfully the comment still applies to ov5675 then, and both modes have the same (2592, 1944) crop rectangle :) > > > + 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. > > > > Since there's no cropping involved in the current modes, I assume we could > just hardcode the width and height and tackle this limitation later, once we > add more modes or support for configuring cropping (this patch only adds the > getter and not the setter). Fine with me! Sorry again for the slip! > > Cheers, > Quentin
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; + 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; + } + 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, };