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

[Tommaso Merciai]

Hi,

On Wed, Jun 08, 2022 at 08:42:09AM +0200, Jacopo Mondi wrote:
> Hi
> 
> On Wed, Jun 08, 2022 at 12:04:05AM +0200, Tommaso Merciai wrote:
> > Hi Quentin/Jacopo,
> >
> > On Tue, Jun 07, 2022 at 06:51:36PM +0200, Jacopo Mondi wrote:
> > > Hi Quentin,
> > >
> > > On Tue, Jun 07, 2022 at 05:33:35PM +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.
> > > >
> > > > There are currently two modes supported by this driver: 2592*1944 and
> > > > 1296*972. The second mode is obtained thanks to subsampling while
> > > > keeping the same field of view (FoV). No cropping involved, hence the
> > > > harcoded values.
> > > >
> > > > Signed-off-by: Quentin Schulz <quentin.schulz@xxxxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > >
> > > > v6:
> > > >  - explicit a bit more the commit log around subsampling for lower
> > > >  resolution modes,
> > > >  - (again) fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
> > > >
> > > > 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 | 21 +++++++++++++++++++++
> > > >  1 file changed, 21 insertions(+)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> > > > index 80840ad7bbb0..2230ff47ef49 100644
> > > > --- a/drivers/media/i2c/ov5675.c
> > > > +++ b/drivers/media/i2c/ov5675.c
> > > > @@ -1121,6 +1121,26 @@ 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)
> > > > +{
> > > > +	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > > > +		return -EINVAL;
> > > > +
> > > > +	switch (sel->target) {
> > > > +	case V4L2_SEL_TGT_CROP:
> > > > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > >
> > > Seem like we have trouble understanding each other, or better, I have
> > > troubles explaining myself most probably :)
> > >
> > > If the dummy/black area is readable, this should just be (0, 0, 2624,
> > > 2000) like it was in your previous version. What has changed that I
> > > have missed ?
> >
> > Taking as reference drivers/media/i2c/ov5693.c and others,
> > seems ok what Quentin have done from my side.
> >
> > Just one thing: maybe is better to avoid magic numbers with more
> > explicit defines like:
> >
> >  + case V4L2_SEL_TGT_CROP_DEFAULT:
> >  +           sel->r.top = OV5675_ACTIVE_START_TOP;
> >  +           sel->r.left = OV5693_ACTIVE_START_LEFT;
> >  +           sel->r.width = OV5693_ACTIVE_WIDTH;
> >  +           sel->r.height = OV5693_ACTIVE_HEIGHT;
> >
> > What do you think about?
> >
> > Thanks,
> > Tommaso
> >
> >
> 
> We have extensively discussed this:
> https://patchwork.linuxtv.org/project/linux-media/patch/20220509143226.531117-4-foss+kernel@xxxxxxxxx/
> https://patchwork.linuxtv.org/project/linux-media/patch/20220525145833.1165437-4-foss+kernel@xxxxxxxxx/
> 
> From the CROP_BOUNDS definition:
> Bounds of the crop rectangle. All valid crop rectangles fit inside the
> crop bounds rectangle.
> 
> From CROP_DEFAULT:
> Suggested cropping rectangle that covers the “whole picture”. This
> includes only active pixels and excludes other non-active pixels such
> as black pixels.
> 
> If (and only if) dummy/inactive pixels can be read out, the BOUNDS
> rectangle should contain them. In this case, as the analog crop
> rectangle is defined with a 16x16 top-left corner (and with a visible
> size of 2592x1944) from a larger rectangle, I presume it means
> dummy/invalid pixls can be read (IOW you can give to the ISP a rectangle
> that includes them).

Thanks for sharing this.

> 
> Anyway, we're discussing details really. I think v5 was almost there
> 
> +	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;
> +	}
> 
> Apart from the fact that TGT_CROP should not report the final image
> size (after binning/digital crop) but the size of the pixel array
> portion processed to obtain the final image.
> 
> 
> +	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:
> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +		sel->r.top = 16;
> +		sel->r.left = 16;
> +		sel->r.width = 2592;
> +		sel->r.height = 1944;
> +		return 0;
> +	}
> 
> Let me remind that (in the context of pleasing libcamera requirements
> as Quentin is doing) all targets apart TGT_CROP are only useful to
> report static properties of the camera, which are of no real use
> unless you start actually looking into reading out black pixels etc
> etc.

Got it.
Thanks for clarifications.

Regards,
Tommaso
> 
> 
> > >
> > > Thanks
> > >   j
> > >
> > >
> > > > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > > > +		sel->r.top = 16;
> > > > +		sel->r.left = 16;
> > > > +		sel->r.width = 2592;
> > > > +		sel->r.height = 1944;
> > > > +		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 +1190,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
> > > >
> >
> > --
> > Tommaso Merciai
> > Embedded Linux Engineer
> > tommaso.merciai@xxxxxxxxxxxxxxxxxxxx
> > __________________________________
> >
> > Amarula Solutions SRL
> > Via Le Canevare 30, 31100 Treviso, Veneto, IT
> > T. +39 042 243 5310
> > info@xxxxxxxxxxxxxxxxxxxx
> > www.amarulasolutions.com

-- 
Tommaso Merciai
Embedded Linux Engineer
tommaso.merciai@xxxxxxxxxxxxxxxxxxxx
__________________________________

Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@xxxxxxxxxxxxxxxxxxxx
www.amarulasolutions.com