Re: [PATCH v5 4/4] media: i2c: ov5675: add .get_selection support

[Quentin Schulz]

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