| Message ID | 20220525145833.1165437-4-foss+kernel@0leil.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,1/4] media: dt-bindings: ov5675: document YAML binding | 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 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 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. >> + 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). Cheers, Quentin
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, };