Re: [PATCH] imx219: selection compliance fixes

[Jacopo Mondi <jacopo@jmondi.org>]

Hi Laurent,

On Wed, Jul 15, 2020 at 02:49:38AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Tue, Jul 14, 2020 at 02:31:46PM +0200, Jacopo Mondi wrote:
> > Hi Hans,
> >
> > On Thu, Jul 02, 2020 at 03:50:04PM +0200, Hans Verkuil wrote:
> > > The top/left crop coordinates were 0, 0 instead of 8, 8 in the
> > > supported_modes array. This was a mismatch with the default values,
> > > so this is corrected. Found with v4l2-compliance.
> > >
> > > Also add V4L2_SEL_TGT_CROP_BOUNDS support: CROP_DEFAULT and CROP_BOUNDS
> > > always go together. Found with v4l2-compliance.
> >
> > I actually introduced this with
> > e6d4ef7d58aa ("media: i2c: imx219: Implement get_selection")
> >
> > >
> > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > > ---
> > >  drivers/media/i2c/imx219.c | 17 +++++++++--------
> > >  1 file changed, 9 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > index 0a546b8e466c..935e2a258ce5 100644
> > > --- a/drivers/media/i2c/imx219.c
> > > +++ b/drivers/media/i2c/imx219.c
> > > @@ -473,8 +473,8 @@ static const struct imx219_mode supported_modes[] = {
> > >  		.width = 3280,
> > >  		.height = 2464,
> > >  		.crop = {
> > > -			.left = 0,
> > > -			.top = 0,
> > > +			.left = 8,
> > > +			.top = 8,
> >
> > Mmmm, why this change ?
> > This values are used to report V4L2_SEL_TGT_CROP rectangle, which
> > according to the documentation is defined as
> > "Crop rectangle. Defines the cropped area."
> > (not a much extensive description :)
> >
> > Clearly this is a faulty definition, and I know from experience how
> > hard is proving to define pixel array properties and in which extent
> > the documentation has to go:
> > https://lists.libcamera.org/pipermail/libcamera-devel/2020-June/009115.html
> >
> > My understanding is that target should report the current crop
> > rectangle, defined from the rectangle retrieved with the
> > V4L2_SEL_TGT_CROP_DEFAULT target, which, according documentation
> > reports the:
> > "Suggested cropping rectangle that covers the “whole picture”.
> > This includes only active pixels and excludes other non-active pixels such
> > as black pixels"
> >
> > The TGT_CROP_DEFAULT then reports the active pixel array portion, and
> > needs to be defined in respect to the TGT_NATIVE_SIZE, which reports
> > the dimensions of the whole pixel matrix, including non-active pixels,
> > optical blanks active and non-active pixels.
> >
> > The TGT_CROP rectangle is hence defined from the CROP_DEFAULT one, and
> > if the 'whole active area' is selected, its top-left corner is placed
> > in position (0, 0) (what's the point of defining it in respect to an
> > area which cannot be read anyway ?)
> >
> > Unless TGT_CROP should be defined in respect to the NATIVE_SIZE
> > rectangle too, but that's not specified anywhere.
> >
> > Anyway, those selection targets badly apply to image sensors, are
> > ill-defined as the definition of active pixels, optical blank (active
> > and non-active) pixels is not provided anywhere, and it's not specified
> > anywhere what is the reference area for each of those rectangles, so I
> > might very well got them wrongly.
>
> My understanding is that both TGT_CROP_DEFAULT and TGT_CROP_BOUNDS are
> relative to TGT_NATIVE_SIZE. BOUNDS defines all the pixels that can be

And what is TGT_CROP reference in your understanding ?

> captured, including optical black and invalid pixels, while DEFAULT
> defines the active area, excluding optical black and invalida pixels. To
> put it another way, DEFAULT is what the kernel recommends applications
> to use if they have no specific requirement and/or no specific knowledge
> about the sensor.
>
> I fully agree this is very under-documented, which also means that my
> understanding may be wrong :-)

With some consensus on this interpretation I would be happy to update
the documentation. I already considered that, but the selection API
does not apply to image sensors only, and giving a description which
is about the pixel array properties might be not totally opportune as
it would rule out other devices like bridges or muxers.

>
> > >  			.width = 3280,
> > >  			.height = 2464
> > >  		},
> > > @@ -489,8 +489,8 @@ static const struct imx219_mode supported_modes[] = {
> > >  		.width = 1920,
> > >  		.height = 1080,
> > >  		.crop = {
> > > -			.left = 680,
> > > -			.top = 692,
> > > +			.left = 8 + 680,
> > > +			.top = 8 + 692,
> > >  			.width = 1920,
> > >  			.height = 1080
> > >  		},
> > > @@ -505,8 +505,8 @@ static const struct imx219_mode supported_modes[] = {
> > >  		.width = 1640,
> > >  		.height = 1232,
> > >  		.crop = {
> > > -			.left = 0,
> > > -			.top = 0,
> > > +			.left = 8,
> > > +			.top = 8,
> > >  			.width = 3280,
> > >  			.height = 2464
> > >  		},
> > > @@ -521,8 +521,8 @@ static const struct imx219_mode supported_modes[] = {
> > >  		.width = 640,
> > >  		.height = 480,
> > >  		.crop = {
> > > -			.left = 1000,
> > > -			.top = 752,
> > > +			.left = 8 + 1000,
> > > +			.top = 8 + 752,
> > >  			.width = 1280,
> > >  			.height = 960
> > >  		},
> > > @@ -1014,6 +1014,7 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
> > >  		return 0;
> > >
> > >  	case V4L2_SEL_TGT_CROP_DEFAULT:
> > > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> >
> > Still not getting what is the purpose of two targets if the "always
> > have to go together" :)
> >
> > >  		sel->r.top = IMX219_PIXEL_ARRAY_TOP;
> > >  		sel->r.left = IMX219_PIXEL_ARRAY_LEFT;
> > >  		sel->r.width = IMX219_PIXEL_ARRAY_WIDTH;
>
> --
> Regards,
>
> Laurent Pinchart