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