imx219: selection compliance fixes
diff mbox series

Message ID b580ac9d-5ae4-29ce-c81a-a1f98b1d953b@xs4all.nl
State New
Headers show
Series
  • imx219: selection compliance fixes
Related show

Commit Message

Hans Verkuil July 2, 2020, 1:50 p.m. UTC
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.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/i2c/imx219.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Comments

Jacopo Mondi July 14, 2020, 12:31 p.m. UTC | #1
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.

>  			.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;
> --
> 2.27.0
>
Laurent Pinchart July 14, 2020, 11:49 p.m. UTC | #2
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
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 :-)

> >  			.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;
Jacopo Mondi July 15, 2020, 7:19 a.m. UTC | #3
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
Hans Verkuil July 16, 2020, 9:48 a.m. UTC | #4
On 15/07/2020 09:19, Jacopo Mondi wrote:
> 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 :)

Unless changed by calling S_SELECTION(TGT_CROP) the initial crop is equal
to TGT_CROP_DEFAULT, and but TGT_CROP and TGT_CROP_DEFAULT shall be inside
TGT_CROP_BOUNDS. CROP_BOUNDS may be larger than CROP_DEFAULT and describes
the whole area from which you can crop. I.e. in the case of sensors you can
set the crop rectangle to include optical blanks active pixels.

In this driver the initial TGT_CROP rectangle as specified in supported_modes
(aligned with the top-left pixel) was outside CROP_BOUNDS (centered) and also
a mismatch with CROP_DEFAULT (also centered).

>>>
>>> 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 relationship between NATIVE_SIZE and CROP_BOUNDS is not properly defined,
but I would expect that CROP_BOUNDS is inside the NATIVE_SIZE target rectangle.

If NATIVE_SIZE is larger than BOUNDS, then I would expect that the additional
margins are pixels that are invalid or otherwise useless.

The hard rule though is that you can crop anywhere within the CROP_BOUNDS area.

Historically CROP_BOUNDS originated with analog SDTV video capture where it was
possible to capture more data than just the typical 720x576/480 PAL/NTSC active
video area. Analog video was often overscanned, i.e. there was more video data
outside the 'active' video area. That was how CRTs worked. So you could move the
crop window around within the CROP_BOUNDS area, or just capture the full CROP_BOUNDS
are. Although this was often poorly tested/implemented. The bttv driver is one of
the few that could do this.

This is actually simplified since you could do weird things with the horizontal
sample rate as well, effectively changing the pixel aspect ratio, making things
really complicated. It's analog video so while the video lines were discrete,
horizontally you are just sampling a waveform, so you could sample at different
rates if you wanted to. I doubt anyone ever used it since doing that would give
you a huge headache :-)

With digital video interfaces (HDMI, DVI, SDI, DP, etc.) that no longer applies and
for those receivers the initial CROP/CROP_DEFAULT/CROP_BOUNDS rectangles are all
the same, e.g. 1920x1080 for 1080p HDMI video.

>>>
>>> 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 ?

That's the rectangle you are actually cropping. Initially CROP == CROP_DEFAULT
and CROP shall always be inside CROP_BOUNDS. And CROP_BOUNDS shall be equal
or larger than CROP_DEFAULT.

> 
>> 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.

And m2m devices like codecs.

Regards,

	Hans

> 
>>
>>>>  			.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
Laurent Pinchart July 16, 2020, 12:59 p.m. UTC | #5
Hi Hans,

On Thu, Jul 16, 2020 at 11:48:19AM +0200, Hans Verkuil wrote:
> On 15/07/2020 09:19, Jacopo Mondi wrote:
> > On Wed, Jul 15, 2020 at 02:49:38AM +0300, Laurent Pinchart wrote:
> >> On Tue, Jul 14, 2020 at 02:31:46PM +0200, Jacopo Mondi wrote:
> >>> 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 :)
> 
> Unless changed by calling S_SELECTION(TGT_CROP) the initial crop is equal
> to TGT_CROP_DEFAULT, and but TGT_CROP and TGT_CROP_DEFAULT shall be inside
> TGT_CROP_BOUNDS. CROP_BOUNDS may be larger than CROP_DEFAULT and describes
> the whole area from which you can crop. I.e. in the case of sensors you can
> set the crop rectangle to include optical blanks active pixels.
> 
> In this driver the initial TGT_CROP rectangle as specified in supported_modes
> (aligned with the top-left pixel) was outside CROP_BOUNDS (centered) and also
> a mismatch with CROP_DEFAULT (also centered).
> 
> >>> 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 relationship between NATIVE_SIZE and CROP_BOUNDS is not properly defined,
> but I would expect that CROP_BOUNDS is inside the NATIVE_SIZE target rectangle.
> 
> If NATIVE_SIZE is larger than BOUNDS, then I would expect that the additional
> margins are pixels that are invalid or otherwise useless.
> 
> The hard rule though is that you can crop anywhere within the CROP_BOUNDS area.
> 
> Historically CROP_BOUNDS originated with analog SDTV video capture where it was
> possible to capture more data than just the typical 720x576/480 PAL/NTSC active
> video area. Analog video was often overscanned, i.e. there was more video data
> outside the 'active' video area. That was how CRTs worked. So you could move the
> crop window around within the CROP_BOUNDS area, or just capture the full CROP_BOUNDS
> are. Although this was often poorly tested/implemented. The bttv driver is one of
> the few that could do this.
> 
> This is actually simplified since you could do weird things with the horizontal
> sample rate as well, effectively changing the pixel aspect ratio, making things
> really complicated. It's analog video so while the video lines were discrete,
> horizontally you are just sampling a waveform, so you could sample at different
> rates if you wanted to. I doubt anyone ever used it since doing that would give
> you a huge headache :-)
> 
> With digital video interfaces (HDMI, DVI, SDI, DP, etc.) that no longer applies and
> for those receivers the initial CROP/CROP_DEFAULT/CROP_BOUNDS rectangles are all
> the same, e.g. 1920x1080 for 1080p HDMI video.
> 
> >>>
> >>> 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 ?
> 
> That's the rectangle you are actually cropping. Initially CROP == CROP_DEFAULT
> and CROP shall always be inside CROP_BOUNDS. And CROP_BOUNDS shall be equal
> or larger than CROP_DEFAULT.

I think you've missed the point of Jacopo's question. He wasn't asking
if CROP needed to be inside CROP_BOUNDS, but what the reference was for
the left and top coordinates. That is, for all the crop rectangles, what
is the location of the (0,0) point ? Do they all refer to the same
location, or are they relative to each other ? This is not defined.

> >> 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.
> 
> And m2m devices like codecs.
> 
> >>>>  			.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;
Hans Verkuil July 16, 2020, 1:53 p.m. UTC | #6
On 16/07/2020 14:59, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Thu, Jul 16, 2020 at 11:48:19AM +0200, Hans Verkuil wrote:
>> On 15/07/2020 09:19, Jacopo Mondi wrote:
>>> On Wed, Jul 15, 2020 at 02:49:38AM +0300, Laurent Pinchart wrote:
>>>> On Tue, Jul 14, 2020 at 02:31:46PM +0200, Jacopo Mondi wrote:
>>>>> 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 :)
>>
>> Unless changed by calling S_SELECTION(TGT_CROP) the initial crop is equal
>> to TGT_CROP_DEFAULT, and but TGT_CROP and TGT_CROP_DEFAULT shall be inside
>> TGT_CROP_BOUNDS. CROP_BOUNDS may be larger than CROP_DEFAULT and describes
>> the whole area from which you can crop. I.e. in the case of sensors you can
>> set the crop rectangle to include optical blanks active pixels.
>>
>> In this driver the initial TGT_CROP rectangle as specified in supported_modes
>> (aligned with the top-left pixel) was outside CROP_BOUNDS (centered) and also
>> a mismatch with CROP_DEFAULT (also centered).
>>
>>>>> 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 relationship between NATIVE_SIZE and CROP_BOUNDS is not properly defined,
>> but I would expect that CROP_BOUNDS is inside the NATIVE_SIZE target rectangle.
>>
>> If NATIVE_SIZE is larger than BOUNDS, then I would expect that the additional
>> margins are pixels that are invalid or otherwise useless.
>>
>> The hard rule though is that you can crop anywhere within the CROP_BOUNDS area.
>>
>> Historically CROP_BOUNDS originated with analog SDTV video capture where it was
>> possible to capture more data than just the typical 720x576/480 PAL/NTSC active
>> video area. Analog video was often overscanned, i.e. there was more video data
>> outside the 'active' video area. That was how CRTs worked. So you could move the
>> crop window around within the CROP_BOUNDS area, or just capture the full CROP_BOUNDS
>> are. Although this was often poorly tested/implemented. The bttv driver is one of
>> the few that could do this.
>>
>> This is actually simplified since you could do weird things with the horizontal
>> sample rate as well, effectively changing the pixel aspect ratio, making things
>> really complicated. It's analog video so while the video lines were discrete,
>> horizontally you are just sampling a waveform, so you could sample at different
>> rates if you wanted to. I doubt anyone ever used it since doing that would give
>> you a huge headache :-)
>>
>> With digital video interfaces (HDMI, DVI, SDI, DP, etc.) that no longer applies and
>> for those receivers the initial CROP/CROP_DEFAULT/CROP_BOUNDS rectangles are all
>> the same, e.g. 1920x1080 for 1080p HDMI video.
>>
>>>>>
>>>>> 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 ?
>>
>> That's the rectangle you are actually cropping. Initially CROP == CROP_DEFAULT
>> and CROP shall always be inside CROP_BOUNDS. And CROP_BOUNDS shall be equal
>> or larger than CROP_DEFAULT.
> 
> I think you've missed the point of Jacopo's question. He wasn't asking
> if CROP needed to be inside CROP_BOUNDS, but what the reference was for
> the left and top coordinates. That is, for all the crop rectangles, what
> is the location of the (0,0) point ? Do they all refer to the same
> location, or are they relative to each other ? This is not defined.

Ah, I misunderstood.

For analog video it was actually undefined. It could be anything, although typically
the initial crop rectangle was at (0, 0). That meant that the larger BOUNDS area
could be at (-8, -8).

For sensors nothing is defined at the moment, but IMHO the largest rectangle
(i.e. TGT_NATIVE_SIZE) should be at (0, 0). I think negative top-left coordinates
are just weird and can potentially cause signedness issues.

In any case, all target rectangles are relative to the same point since you need
to know where the BOUNDS rectangle is within the larger NATIVE_SIZE rectangle
(ugly name BTW), and ditto for CROP/CROP_DEFAULT within the larger CROP_BOUNDS.

Regards,

	Hans

> 
>>>> 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.
>>
>> And m2m devices like codecs.
>>
>>>>>>  			.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;
>
Laurent Pinchart July 16, 2020, 11:20 p.m. UTC | #7
Hi Hans,

On Thu, Jul 16, 2020 at 03:53:41PM +0200, Hans Verkuil wrote:
> On 16/07/2020 14:59, Laurent Pinchart wrote:
> > On Thu, Jul 16, 2020 at 11:48:19AM +0200, Hans Verkuil wrote:
> >> On 15/07/2020 09:19, Jacopo Mondi wrote:
> >>> On Wed, Jul 15, 2020 at 02:49:38AM +0300, Laurent Pinchart wrote:
> >>>> On Tue, Jul 14, 2020 at 02:31:46PM +0200, Jacopo Mondi wrote:
> >>>>> 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 :)
> >>
> >> Unless changed by calling S_SELECTION(TGT_CROP) the initial crop is equal
> >> to TGT_CROP_DEFAULT, and but TGT_CROP and TGT_CROP_DEFAULT shall be inside
> >> TGT_CROP_BOUNDS. CROP_BOUNDS may be larger than CROP_DEFAULT and describes
> >> the whole area from which you can crop. I.e. in the case of sensors you can
> >> set the crop rectangle to include optical blanks active pixels.
> >>
> >> In this driver the initial TGT_CROP rectangle as specified in supported_modes
> >> (aligned with the top-left pixel) was outside CROP_BOUNDS (centered) and also
> >> a mismatch with CROP_DEFAULT (also centered).
> >>
> >>>>> 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 relationship between NATIVE_SIZE and CROP_BOUNDS is not properly defined,
> >> but I would expect that CROP_BOUNDS is inside the NATIVE_SIZE target rectangle.
> >>
> >> If NATIVE_SIZE is larger than BOUNDS, then I would expect that the additional
> >> margins are pixels that are invalid or otherwise useless.
> >>
> >> The hard rule though is that you can crop anywhere within the CROP_BOUNDS area.
> >>
> >> Historically CROP_BOUNDS originated with analog SDTV video capture where it was
> >> possible to capture more data than just the typical 720x576/480 PAL/NTSC active
> >> video area. Analog video was often overscanned, i.e. there was more video data
> >> outside the 'active' video area. That was how CRTs worked. So you could move the
> >> crop window around within the CROP_BOUNDS area, or just capture the full CROP_BOUNDS
> >> are. Although this was often poorly tested/implemented. The bttv driver is one of
> >> the few that could do this.
> >>
> >> This is actually simplified since you could do weird things with the horizontal
> >> sample rate as well, effectively changing the pixel aspect ratio, making things
> >> really complicated. It's analog video so while the video lines were discrete,
> >> horizontally you are just sampling a waveform, so you could sample at different
> >> rates if you wanted to. I doubt anyone ever used it since doing that would give
> >> you a huge headache :-)
> >>
> >> With digital video interfaces (HDMI, DVI, SDI, DP, etc.) that no longer applies and
> >> for those receivers the initial CROP/CROP_DEFAULT/CROP_BOUNDS rectangles are all
> >> the same, e.g. 1920x1080 for 1080p HDMI video.
> >>
> >>>>>
> >>>>> 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 ?
> >>
> >> That's the rectangle you are actually cropping. Initially CROP == CROP_DEFAULT
> >> and CROP shall always be inside CROP_BOUNDS. And CROP_BOUNDS shall be equal
> >> or larger than CROP_DEFAULT.
> > 
> > I think you've missed the point of Jacopo's question. He wasn't asking
> > if CROP needed to be inside CROP_BOUNDS, but what the reference was for
> > the left and top coordinates. That is, for all the crop rectangles, what
> > is the location of the (0,0) point ? Do they all refer to the same
> > location, or are they relative to each other ? This is not defined.
> 
> Ah, I misunderstood.
> 
> For analog video it was actually undefined. It could be anything, although typically
> the initial crop rectangle was at (0, 0). That meant that the larger BOUNDS area
> could be at (-8, -8).
> 
> For sensors nothing is defined at the moment, but IMHO the largest rectangle
> (i.e. TGT_NATIVE_SIZE) should be at (0, 0). I think negative top-left coordinates
> are just weird and can potentially cause signedness issues.
> 
> In any case, all target rectangles are relative to the same point since you need
> to know where the BOUNDS rectangle is within the larger NATIVE_SIZE rectangle
> (ugly name BTW), and ditto for CROP/CROP_DEFAULT within the larger CROP_BOUNDS.

That makes sense to me, having the same reference for all targets should
make it simpler. Jacopo, do you think that's good ?

> >>>> 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.
> >>
> >> And m2m devices like codecs.
> >>
> >>>>>>  			.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;
Jacopo Mondi July 17, 2020, 6:34 a.m. UTC | #8
Hi Laurent,

On Fri, Jul 17, 2020 at 02:20:31AM +0300, Laurent Pinchart wrote:
> Hi Hans,
>
> On Thu, Jul 16, 2020 at 03:53:41PM +0200, Hans Verkuil wrote:
> > On 16/07/2020 14:59, Laurent Pinchart wrote:
> > > On Thu, Jul 16, 2020 at 11:48:19AM +0200, Hans Verkuil wrote:
> > >> On 15/07/2020 09:19, Jacopo Mondi wrote:
> > >>> On Wed, Jul 15, 2020 at 02:49:38AM +0300, Laurent Pinchart wrote:
> > >>>> On Tue, Jul 14, 2020 at 02:31:46PM +0200, Jacopo Mondi wrote:
> > >>>>> 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 :)
> > >>
> > >> Unless changed by calling S_SELECTION(TGT_CROP) the initial crop is equal
> > >> to TGT_CROP_DEFAULT, and but TGT_CROP and TGT_CROP_DEFAULT shall be inside
> > >> TGT_CROP_BOUNDS. CROP_BOUNDS may be larger than CROP_DEFAULT and describes
> > >> the whole area from which you can crop. I.e. in the case of sensors you can
> > >> set the crop rectangle to include optical blanks active pixels.
> > >>
> > >> In this driver the initial TGT_CROP rectangle as specified in supported_modes
> > >> (aligned with the top-left pixel) was outside CROP_BOUNDS (centered) and also
> > >> a mismatch with CROP_DEFAULT (also centered).
> > >>
> > >>>>> 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 relationship between NATIVE_SIZE and CROP_BOUNDS is not properly defined,
> > >> but I would expect that CROP_BOUNDS is inside the NATIVE_SIZE target rectangle.
> > >>
> > >> If NATIVE_SIZE is larger than BOUNDS, then I would expect that the additional
> > >> margins are pixels that are invalid or otherwise useless.
> > >>
> > >> The hard rule though is that you can crop anywhere within the CROP_BOUNDS area.
> > >>
> > >> Historically CROP_BOUNDS originated with analog SDTV video capture where it was
> > >> possible to capture more data than just the typical 720x576/480 PAL/NTSC active
> > >> video area. Analog video was often overscanned, i.e. there was more video data
> > >> outside the 'active' video area. That was how CRTs worked. So you could move the
> > >> crop window around within the CROP_BOUNDS area, or just capture the full CROP_BOUNDS
> > >> are. Although this was often poorly tested/implemented. The bttv driver is one of
> > >> the few that could do this.
> > >>
> > >> This is actually simplified since you could do weird things with the horizontal
> > >> sample rate as well, effectively changing the pixel aspect ratio, making things
> > >> really complicated. It's analog video so while the video lines were discrete,
> > >> horizontally you are just sampling a waveform, so you could sample at different
> > >> rates if you wanted to. I doubt anyone ever used it since doing that would give
> > >> you a huge headache :-)
> > >>
> > >> With digital video interfaces (HDMI, DVI, SDI, DP, etc.) that no longer applies and
> > >> for those receivers the initial CROP/CROP_DEFAULT/CROP_BOUNDS rectangles are all
> > >> the same, e.g. 1920x1080 for 1080p HDMI video.
> > >>
> > >>>>>
> > >>>>> 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 ?
> > >>
> > >> That's the rectangle you are actually cropping. Initially CROP == CROP_DEFAULT
> > >> and CROP shall always be inside CROP_BOUNDS. And CROP_BOUNDS shall be equal
> > >> or larger than CROP_DEFAULT.
> > >
> > > I think you've missed the point of Jacopo's question. He wasn't asking
> > > if CROP needed to be inside CROP_BOUNDS, but what the reference was for
> > > the left and top coordinates. That is, for all the crop rectangles, what
> > > is the location of the (0,0) point ? Do they all refer to the same
> > > location, or are they relative to each other ? This is not defined.
> >
> > Ah, I misunderstood.
> >
> > For analog video it was actually undefined. It could be anything, although typically
> > the initial crop rectangle was at (0, 0). That meant that the larger BOUNDS area
> > could be at (-8, -8).
> >
> > For sensors nothing is defined at the moment, but IMHO the largest rectangle
> > (i.e. TGT_NATIVE_SIZE) should be at (0, 0). I think negative top-left coordinates
> > are just weird and can potentially cause signedness issues.
> >
> > In any case, all target rectangles are relative to the same point since you need
> > to know where the BOUNDS rectangle is within the larger NATIVE_SIZE rectangle
> > (ugly name BTW), and ditto for CROP/CROP_DEFAULT within the larger CROP_BOUNDS.
>
> That makes sense to me, having the same reference for all targets should
> make it simpler. Jacopo, do you think that's good ?
>

It's ok. We shall update the libcamera counterpart that reads the
rectangles as soon as this patch gets in.

Thanks
  j

> > >>>> 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.
> > >>
> > >> And m2m devices like codecs.
> > >>
> > >>>>>>  			.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
Jacopo Mondi Aug. 1, 2020, 11:19 a.m. UTC | #9
Hi Hans, Laurent,

    sorry, long email, contains a few things on the general definition
    of the selection target, a question for libcamera, and a few more
    details at the end.

Adding Sakari, libcamera ml, Ricardo which helped with the
definition of pixel array properties in libcamera recently and
Dave and Naush as this sensor is the RPi camera module v2 and some
information on the sensor come from their BSP.

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.

Let me try to summarize the outcome of the discussion

1) All selection rectangles are defined in respect to the NATIVE_SIZE
   target, which returns the full pixel array size, which includes
   readable and non-readable pixels. It's top-left corner is in
   position (0,0)

3) CROP_BOUND returns the portion of the full pixel array which can be
   read out, including optical black pixels, and is defined in respect
   to the full pixel array size

2) CROP_DEFAULT returns the portion of the readable part of the pixel array
   which contains valid image data (aka 'active' pixels). It is again
   defined in respect to the full pixel array rectangle returned by
   NATIVE_SIZE target.

With an ack on my understanding, I think it's worth expanding the
target documentation a bit. As I've said I've been hesitant in doing
so, as those targets do not only apply to image sensors, but I think a
section that goes like "If the sub-device represents and image sensor
then the V4L2_SEL_TGT_.. target represents ... "

Laurent: this will have some impact on libcamera as well
https://git.linuxtv.org/libcamera.git/tree/src/libcamera/camera_sensor.cpp#n503
When we read the analogCrop rectangle, we decided it is defined in
respect to the active portion of the pixel array (CROP_DEFAULT) and
not from the full pixel array size (NATIVE_SIZE).

We then should:
1) Back-track on our decision to have analog crop defined in respect
to the active part and decide it should be defined in respect to the
full pixel array: this has implications on the existing IPAs that
assume what we have defined

2) Adjust the analogCrop rectangle by subtracting to its sizes the
values reported by TGT_CROP_DEFAULT.top and TGT_CROP_DEFAULT.left.

I would got for 2) what's your opinion ?

On this specific patch:

>
> 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,

we have
#define IMX219_PIXEL_ARRAY_LEFT		8U
#define IMX219_PIXEL_ARRAY_TOP		8U

Which I would then re-name IMX219_ACTIVE_ARRAY_LEFT and
IMX219_ACTIVE_ARRAY_TOP and re-use it there


>  			.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:

I think this is fine and that's my reasoning:

The IMAX219 pixel array is documented as

        /-------------- 3296 -------------------/
         8                                     8
        +---------------------------------------+    /
        |IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII| 8  |
        |IpppppppppppppppppppppppppppppppppppppI|    |
        |IpppppppppppppppppppppppppppppppppppppI|    |
        |IpppppppppppppppppppppppppppppppppppppI|    |
        |IpppppppppppppppppppppppppppppppppppppI|    |
        |IpppppppppppppppppppppppppppppppppppppI|    |
        |IpppppppppppppppppppppppppppppppppppppI|    |

                            ....                    2480

        |IpppppppppppppppppppppppppppppppppppppI|    |
        |IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII|    |
        |IoooooooooooooooooooooooooooooooooooooI| 16 |
        |IOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOI| 16 |
        |IoooooooooooooooooooooooooooooooooooooI| 8  |
        +---------------------------------------+    /

Where:
        I = invalid active area
        p = valid pixels
        o = Invalid OB area
        O = Valid OB area
        Effective area = 3296x2480
        Active area = 3280x2464

The 'active area' is then sorrounded by 8 invalid rows, 8 invalid
cols at the beginning and the end, and followed by 8 more invalid
rows. Then, 16 invalid black rows follow, then 16 -valid- black
rows, then 8 invalid black rows again.

My understanding is that the whole effective area is sent on the bus,
as the CSI-2 timings report the sizes of the 'effective' area to be
the whole 3296x2480 matrix, and assigns a CSI-2 data-type to the
"Valid OB area" while sets the DataType for invalid areas to Null.

However the registers that select the analog crop work on the 'active
area' only, so there is not way to select "I want the Valid OB area"
only, as the whole 'effective area' is sent on the bus and the CSI-2
receivers filters on the DataType to discard the 'Invalid' lines (to
which a Null DataType is assigned) and capture image data ('active area')
and eventually 'Valid black' pixels (which have a data type assigned).

All of this to say, there's no point in reporting a TGT_BOUND larger
that the active area, as the user cannot select portions outside of it
through the S_SELECTION API, but can only instruct it's CSI-2 receiver
to filter-in the data it desires, which I think we're missing an API
for.

Hans: would you like to go ahead with this patch, or should I take
over and change the libcamera part that parses these properties in one
go ?

Thanks
  j

>  		sel->r.top = IMX219_PIXEL_ARRAY_TOP;
>  		sel->r.left = IMX219_PIXEL_ARRAY_LEFT;
>  		sel->r.width = IMX219_PIXEL_ARRAY_WIDTH;
> --
> 2.27.0
>
Laurent Pinchart Aug. 1, 2020, 3:13 p.m. UTC | #10
Hi Jacopo,

On Sat, Aug 01, 2020 at 01:19:03PM +0200, Jacopo Mondi wrote:
> Hi Hans, Laurent,
> 
>     sorry, long email, contains a few things on the general definition
>     of the selection target, a question for libcamera, and a few more
>     details at the end.
> 
> Adding Sakari, libcamera ml, Ricardo which helped with the
> definition of pixel array properties in libcamera recently and
> Dave and Naush as this sensor is the RPi camera module v2 and some
> information on the sensor come from their BSP.
> 
> 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.
> 
> Let me try to summarize the outcome of the discussion
> 
> 1) All selection rectangles are defined in respect to the NATIVE_SIZE
>    target, which returns the full pixel array size, which includes
>    readable and non-readable pixels. It's top-left corner is in
>    position (0,0)

Yes, except that, to be pedantic, it's not that the top-left corner *is*
in position (0,0) but that the top-left corner is defined as (0,0).
NATIVE_SIZE is the root of the coordinates system, and by definition the
top-left coordinates must be set to (0,0).

> 3) CROP_BOUND returns the portion of the full pixel array which can be
>    read out, including optical black pixels, and is defined in respect
>    to the full pixel array size

Yes. I'd say it's defined in respect to NATIVE_SIZE to avoid the
indirection in the definition (NATIVE_SIZE and pixel array size are the
same).

This maps to the libcamera PixelArraySize property in libcamera.

> 2) CROP_DEFAULT returns the portion of the readable part of the pixel array
>    which contains valid image data (aka 'active' pixels). It is again
>    defined in respect to the full pixel array rectangle returned by
>    NATIVE_SIZE target.

Correct.

This maps to the PixelArrayActiveAreas property in libcamera (assuming
the property contains a single value).

> With an ack on my understanding, I think it's worth expanding the
> target documentation a bit. As I've said I've been hesitant in doing
> so, as those targets do not only apply to image sensors, but I think a
> section that goes like "If the sub-device represents and image sensor
> then the V4L2_SEL_TGT_.. target represents ... "

It's totally fine by me to add a section that defines the targets
precisely for sensors.

> Laurent: this will have some impact on libcamera as well
> https://git.linuxtv.org/libcamera.git/tree/src/libcamera/camera_sensor.cpp#n503
> When we read the analogCrop rectangle, we decided it is defined in
> respect to the active portion of the pixel array (CROP_DEFAULT) and
> not from the full pixel array size (NATIVE_SIZE).

Note that the non-readable portion of NATIVE_SIZE has no impact in
practice. A sensor driver could return NATIVE_SIZE == CROP_BOUND, as
long as CROP_BOUND, CROP_DEFAULT and CROP are all expressed relatively
to NATIVE_SIZE, it will make no difference for userspace.

We could thus mandate that NATIVE_SIZE == CROP_BOUND to simplify things.

> We then should:
> 1) Back-track on our decision to have analog crop defined in respect
> to the active part and decide it should be defined in respect to the
> full pixel array: this has implications on the existing IPAs that
> assume what we have defined
> 
> 2) Adjust the analogCrop rectangle by subtracting to its sizes the
> values reported by TGT_CROP_DEFAULT.top and TGT_CROP_DEFAULT.left.
> 
> I would got for 2) what's your opinion ?

Inside libcamera I would express all crop rectangles relatively to
PixelArraySize, so mapping to V4L2 would require adding/subtracting the
TGT_CROP_DEFAULT offset. That's why requiring NATIVE_SIZE == CROP_BOUND
may simplify things.

> On this specific patch:
> 
> > 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,
> 
> we have
> #define IMX219_PIXEL_ARRAY_LEFT		8U
> #define IMX219_PIXEL_ARRAY_TOP		8U
> 
> Which I would then re-name IMX219_ACTIVE_ARRAY_LEFT and
> IMX219_ACTIVE_ARRAY_TOP and re-use it there
> 
> >  			.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:
> 
> I think this is fine and that's my reasoning:
> 
> The IMAX219 pixel array is documented as
> 
>         /-------------- 3296 -------------------/
>          8                                     8
>         +---------------------------------------+    /
>         |IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII| 8  |
>         |IpppppppppppppppppppppppppppppppppppppI|    |
>         |IpppppppppppppppppppppppppppppppppppppI|    |
>         |IpppppppppppppppppppppppppppppppppppppI|    |
>         |IpppppppppppppppppppppppppppppppppppppI|    |
>         |IpppppppppppppppppppppppppppppppppppppI|    |
>         |IpppppppppppppppppppppppppppppppppppppI|    |
> 
>                             ....                    2480
> 
>         |IpppppppppppppppppppppppppppppppppppppI|    |
>         |IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII|    |
>         |IoooooooooooooooooooooooooooooooooooooI| 16 |
>         |IOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOI| 16 |
>         |IoooooooooooooooooooooooooooooooooooooI| 8  |
>         +---------------------------------------+    /
> 
> Where:
>         I = invalid active area
>         p = valid pixels
>         o = Invalid OB area
>         O = Valid OB area
>         Effective area = 3296x2480
>         Active area = 3280x2464

This doesn't match your diagram above, 8+2464+16+16+8 != 2480.

> The 'active area' is then sorrounded by 8 invalid rows, 8 invalid
> cols at the beginning and the end, and followed by 8 more invalid
> rows. Then, 16 invalid black rows follow, then 16 -valid- black
> rows, then 8 invalid black rows again.
> 
> My understanding is that the whole effective area is sent on the bus,
> as the CSI-2 timings report the sizes of the 'effective' area to be
> the whole 3296x2480 matrix, and assigns a CSI-2 data-type to the
> "Valid OB area" while sets the DataType for invalid areas to Null.
> 
> However the registers that select the analog crop work on the 'active
> area' only, so there is not way to select "I want the Valid OB area"
> only, as the whole 'effective area' is sent on the bus and the CSI-2
> receivers filters on the DataType to discard the 'Invalid' lines (to
> which a Null DataType is assigned) and capture image data ('active area')
> and eventually 'Valid black' pixels (which have a data type assigned).

I'm not aware of CSI-2 receivers that can capture NULL data types. At
most you'll be able to capture OB and active pixels, nothing else.

> All of this to say, there's no point in reporting a TGT_BOUND larger
> that the active area, as the user cannot select portions outside of it
> through the S_SELECTION API, but can only instruct it's CSI-2 receiver
> to filter-in the data it desires, which I think we're missing an API
> for.

This however contradicts your proposal above, where you said that
CROP_BOUND should include the OB lines :-)

> Hans: would you like to go ahead with this patch, or should I take
> over and change the libcamera part that parses these properties in one
> go ?
> 
> >  		sel->r.top = IMX219_PIXEL_ARRAY_TOP;
> >  		sel->r.left = IMX219_PIXEL_ARRAY_LEFT;
> >  		sel->r.width = IMX219_PIXEL_ARRAY_WIDTH;
Jacopo Mondi Aug. 3, 2020, 8:33 a.m. UTC | #11
Hi Laurent,

On Sat, Aug 01, 2020 at 06:13:17PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Sat, Aug 01, 2020 at 01:19:03PM +0200, Jacopo Mondi wrote:
> > Hi Hans, Laurent,
> >
> >     sorry, long email, contains a few things on the general definition
> >     of the selection target, a question for libcamera, and a few more
> >     details at the end.
> >
> > Adding Sakari, libcamera ml, Ricardo which helped with the
> > definition of pixel array properties in libcamera recently and
> > Dave and Naush as this sensor is the RPi camera module v2 and some
> > information on the sensor come from their BSP.
> >
> > 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.
> >
> > Let me try to summarize the outcome of the discussion
> >
> > 1) All selection rectangles are defined in respect to the NATIVE_SIZE
> >    target, which returns the full pixel array size, which includes
> >    readable and non-readable pixels. It's top-left corner is in
> >    position (0,0)
>
> Yes, except that, to be pedantic, it's not that the top-left corner *is*
> in position (0,0) but that the top-left corner is defined as (0,0).
> NATIVE_SIZE is the root of the coordinates system, and by definition the
> top-left coordinates must be set to (0,0).
>

You are pedantically right

> > 3) CROP_BOUND returns the portion of the full pixel array which can be
> >    read out, including optical black pixels, and is defined in respect
> >    to the full pixel array size
>
> Yes. I'd say it's defined in respect to NATIVE_SIZE to avoid the
> indirection in the definition (NATIVE_SIZE and pixel array size are the
> same).
>
> This maps to the libcamera PixelArraySize property in libcamera.
>

Yes it does!

> > 2) CROP_DEFAULT returns the portion of the readable part of the pixel array
> >    which contains valid image data (aka 'active' pixels). It is again
> >    defined in respect to the full pixel array rectangle returned by
> >    NATIVE_SIZE target.
>
> Correct.
>
> This maps to the PixelArrayActiveAreas property in libcamera (assuming
> the property contains a single value).
>

Yes, right now a single Rectangle can be expressed with the existing
V4L2 API

> > With an ack on my understanding, I think it's worth expanding the
> > target documentation a bit. As I've said I've been hesitant in doing
> > so, as those targets do not only apply to image sensors, but I think a
> > section that goes like "If the sub-device represents and image sensor
> > then the V4L2_SEL_TGT_.. target represents ... "
>
> It's totally fine by me to add a section that defines the targets
> precisely for sensors.
>
> > Laurent: this will have some impact on libcamera as well
> > https://git.linuxtv.org/libcamera.git/tree/src/libcamera/camera_sensor.cpp#n503
> > When we read the analogCrop rectangle, we decided it is defined in
> > respect to the active portion of the pixel array (CROP_DEFAULT) and
> > not from the full pixel array size (NATIVE_SIZE).
>
> Note that the non-readable portion of NATIVE_SIZE has no impact in
> practice. A sensor driver could return NATIVE_SIZE == CROP_BOUND, as
> long as CROP_BOUND, CROP_DEFAULT and CROP are all expressed relatively
> to NATIVE_SIZE, it will make no difference for userspace.
>
> We could thus mandate that NATIVE_SIZE == CROP_BOUND to simplify things.
>
> > We then should:
> > 1) Back-track on our decision to have analog crop defined in respect
> > to the active part and decide it should be defined in respect to the
> > full pixel array: this has implications on the existing IPAs that
> > assume what we have defined
> >
> > 2) Adjust the analogCrop rectangle by subtracting to its sizes the
> > values reported by TGT_CROP_DEFAULT.top and TGT_CROP_DEFAULT.left.
> >
> > I would got for 2) what's your opinion ?
>
> Inside libcamera I would express all crop rectangles relatively to
> PixelArraySize, so mapping to V4L2 would require adding/subtracting the
> TGT_CROP_DEFAULT offset. That's why requiring NATIVE_SIZE == CROP_BOUND
> may simplify things.
>

If we want to define libcamera properties relatively to
PixelArraySize (the readable part of the pixel array), then we'll have
to subtract the TGT_CROP_BOUND offsets, not the _DEFAULT one,
otherwise we would express it relatively to the active part of the
pixel array (which would be ok, but as those properties are for IPA,
they might want to be able to read OB pixels out ?)

> > On this specific patch:
> >
> > > 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,
> >
> > we have
> > #define IMX219_PIXEL_ARRAY_LEFT		8U
> > #define IMX219_PIXEL_ARRAY_TOP		8U
> >
> > Which I would then re-name IMX219_ACTIVE_ARRAY_LEFT and
> > IMX219_ACTIVE_ARRAY_TOP and re-use it there
> >
> > >  			.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:
> >
> > I think this is fine and that's my reasoning:
> >
> > The IMAX219 pixel array is documented as
> >
> >         /-------------- 3296 -------------------/
> >          8                                     8
> >         +---------------------------------------+    /
> >         |IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII| 8  |
> >         |IpppppppppppppppppppppppppppppppppppppI|    |
> >         |IpppppppppppppppppppppppppppppppppppppI|    |
> >         |IpppppppppppppppppppppppppppppppppppppI|    |
> >         |IpppppppppppppppppppppppppppppppppppppI|    |
> >         |IpppppppppppppppppppppppppppppppppppppI|    |
> >         |IpppppppppppppppppppppppppppppppppppppI|    |
> >
> >                             ....                    2480
> >
> >         |IpppppppppppppppppppppppppppppppppppppI|    |
> >         |IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII|    |
> >         |IoooooooooooooooooooooooooooooooooooooI| 16 |
> >         |IOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOI| 16 |
> >         |IoooooooooooooooooooooooooooooooooooooI| 8  |
> >         +---------------------------------------+    /
> >
> > Where:
> >         I = invalid active area
> >         p = valid pixels
> >         o = Invalid OB area
> >         O = Valid OB area
> >         Effective area = 3296x2480
> >         Active area = 3280x2464
>
> This doesn't match your diagram above, 8+2464+16+16+8 != 2480.
>

Correct. The 'effective' 2480 size is defined without taking OB pixels
in consideration

> > The 'active area' is then sorrounded by 8 invalid rows, 8 invalid
> > cols at the beginning and the end, and followed by 8 more invalid
> > rows. Then, 16 invalid black rows follow, then 16 -valid- black
> > rows, then 8 invalid black rows again.
> >
> > My understanding is that the whole effective area is sent on the bus,
> > as the CSI-2 timings report the sizes of the 'effective' area to be
> > the whole 3296x2480 matrix, and assigns a CSI-2 data-type to the
> > "Valid OB area" while sets the DataType for invalid areas to Null.
> >
> > However the registers that select the analog crop work on the 'active
> > area' only, so there is not way to select "I want the Valid OB area"
> > only, as the whole 'effective area' is sent on the bus and the CSI-2
> > receivers filters on the DataType to discard the 'Invalid' lines (to
> > which a Null DataType is assigned) and capture image data ('active area')
> > and eventually 'Valid black' pixels (which have a data type assigned).
>
> I'm not aware of CSI-2 receivers that can capture NULL data types. At
> most you'll be able to capture OB and active pixels, nothing else.
>

That's what I meant, Invalid lines are put on the bus but discarded.

At this point, the manual seems to be contradicting, as the CSI-2
timings report a data type for OB pixels and show the whole
'effective' area to be put on the bus, hence I assume it contains
OB pixels (table 13 adn figure 20). But as you noted, what is defined
as 'effective' area by the pixel array drawings does not contain OB lines.

Furthermore, Figure 46 confuses me even more.

>> All of this to say, there's no point in reporting a TGT_BOUND larger
> > that the active area, as the user cannot select portions outside of it
> > through the S_SELECTION API, but can only instruct it's CSI-2 receiver
> > to filter-in the data it desires, which I think we're missing an API
> > for.
>
> This however contradicts your proposal above, where you said that
> CROP_BOUND should include the OB lines :-)
>

It should -if- they are readable through the selection API, ie, you
can select an area with TGT_CROP to read-out which includes OB lines. My
understanding is that this sensor does not allow you to do so, as the
registers that allows to select the analog crop portion range on the
active pixel area portion. Maybe RPi people which worked more with the
sensor know more about this ?

Hence CROP_BOUNDS = CROP_DEFAULT, as this patch does, I think it's fine.

For libcamera this means PixelArraySize == PixelArrayActiveAreas
And analogCrop should be adjusted to take the CROP_BOUND offsets into
account.

A pretty messy topic indeed :/

> > Hans: would you like to go ahead with this patch, or should I take
> > over and change the libcamera part that parses these properties in one
> > go ?
> >
> > >  		sel->r.top = IMX219_PIXEL_ARRAY_TOP;
> > >  		sel->r.left = IMX219_PIXEL_ARRAY_LEFT;
> > >  		sel->r.width = IMX219_PIXEL_ARRAY_WIDTH;
>
> --
> Regards,
>
> Laurent Pinchart
Hans Verkuil Aug. 3, 2020, 8:37 a.m. UTC | #12
On 01/08/2020 13:19, Jacopo Mondi wrote:
> Hans: would you like to go ahead with this patch, or should I take
> over and change the libcamera part that parses these properties in one
> go ?

Feel free to take it over!

Regards,

	Hans

> 
> Thanks
>   j
> 
>>  		sel->r.top = IMX219_PIXEL_ARRAY_TOP;
>>  		sel->r.left = IMX219_PIXEL_ARRAY_LEFT;
>>  		sel->r.width = IMX219_PIXEL_ARRAY_WIDTH;
>> --
>> 2.27.0
>>
Hans Verkuil Aug. 3, 2020, 9:07 a.m. UTC | #13
On 01/08/2020 17:13, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> On Sat, Aug 01, 2020 at 01:19:03PM +0200, Jacopo Mondi wrote:
>> Hi Hans, Laurent,
>>
>>     sorry, long email, contains a few things on the general definition
>>     of the selection target, a question for libcamera, and a few more
>>     details at the end.
>>
>> Adding Sakari, libcamera ml, Ricardo which helped with the
>> definition of pixel array properties in libcamera recently and
>> Dave and Naush as this sensor is the RPi camera module v2 and some
>> information on the sensor come from their BSP.
>>
>> 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.
>>
>> Let me try to summarize the outcome of the discussion
>>
>> 1) All selection rectangles are defined in respect to the NATIVE_SIZE
>>    target, which returns the full pixel array size, which includes
>>    readable and non-readable pixels. It's top-left corner is in
>>    position (0,0)
> 
> Yes, except that, to be pedantic, it's not that the top-left corner *is*
> in position (0,0) but that the top-left corner is defined as (0,0).
> NATIVE_SIZE is the root of the coordinates system, and by definition the
> top-left coordinates must be set to (0,0).
> 
>> 3) CROP_BOUND returns the portion of the full pixel array which can be
>>    read out, including optical black pixels, and is defined in respect
>>    to the full pixel array size
> 
> Yes. I'd say it's defined in respect to NATIVE_SIZE to avoid the
> indirection in the definition (NATIVE_SIZE and pixel array size are the
> same).
> 
> This maps to the libcamera PixelArraySize property in libcamera.
> 
>> 2) CROP_DEFAULT returns the portion of the readable part of the pixel array
>>    which contains valid image data (aka 'active' pixels). It is again
>>    defined in respect to the full pixel array rectangle returned by
>>    NATIVE_SIZE target.
> 
> Correct.
> 
> This maps to the PixelArrayActiveAreas property in libcamera (assuming
> the property contains a single value).
> 
>> With an ack on my understanding, I think it's worth expanding the
>> target documentation a bit. As I've said I've been hesitant in doing
>> so, as those targets do not only apply to image sensors, but I think a
>> section that goes like "If the sub-device represents and image sensor
>> then the V4L2_SEL_TGT_.. target represents ... "
> 
> It's totally fine by me to add a section that defines the targets
> precisely for sensors.
> 
>> Laurent: this will have some impact on libcamera as well
>> https://git.linuxtv.org/libcamera.git/tree/src/libcamera/camera_sensor.cpp#n503
>> When we read the analogCrop rectangle, we decided it is defined in
>> respect to the active portion of the pixel array (CROP_DEFAULT) and
>> not from the full pixel array size (NATIVE_SIZE).
> 
> Note that the non-readable portion of NATIVE_SIZE has no impact in
> practice. A sensor driver could return NATIVE_SIZE == CROP_BOUND, as
> long as CROP_BOUND, CROP_DEFAULT and CROP are all expressed relatively
> to NATIVE_SIZE, it will make no difference for userspace.
> 
> We could thus mandate that NATIVE_SIZE == CROP_BOUND to simplify things.

Grepping for V4L2_SEL_TGT_NATIVE_SIZE shows it being used in three drivers:

imx219, smiapp and coda. The use in coda is bogus (seems to be a left-over of old
code) and it can be removed in that driver.

It's not quite clear how it is used in smiapp: it appears to be mapped to
CROP_BOUNDS as well, but it is only exposed if the drivers knows the native size.

Going back to Sailus' original patch series from 2014 adding the NATIVE_SIZE
target, it appears that it was added as a CROP_BOUNDS replacement. From the
cover letter:

"The two latter patches create a V4L2_SEL_TGT_NATIVE_SIZE target which is
used in the smiapp driver. The CROP_BOUNDS target is still supported as
compatibility means."

and from patch 5/5 ("smiapp: Support V4L2_SEL_TGT_NATIVE_SIZE"):

"Add support for selection target V4L2_SEL_TGT_NATIVE_SIZE. It is equivalent
of what V4L2_SEL_TGT_CROP_BOUNDS used to be. Support for
V4L2_SEL_TGT_CROP_BOUNDS is still supported by the driver as a compatibility
interface."

So in the context of sensors NATIVE_SIZE == CROP_BOUNDS. What I can't remember
is why this new target was added if it acts the same as CROP_BOUNDS.

There is a valid use-case for NATIVE_SIZE for hardware that can create a 'canvas'
of a programmable size in which incoming video streams are composed and the whole
canvas is streamed out. But we don't have such devices at the moment.

Regards,

	Hans
Dave Stevenson Aug. 3, 2020, 9:58 a.m. UTC | #14
Hi Jacopo, Hans, and Laurent.

On Sat, 1 Aug 2020 at 12:15, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Hans, Laurent,
>
>     sorry, long email, contains a few things on the general definition
>     of the selection target, a question for libcamera, and a few more
>     details at the end.
>
> Adding Sakari, libcamera ml, Ricardo which helped with the
> definition of pixel array properties in libcamera recently and
> Dave and Naush as this sensor is the RPi camera module v2 and some
> information on the sensor come from their BSP.

Thanks for cc'ing me in but I'll say that this seems to be a framework
question rather than specifically an IMX219 thing. I'll therefore
leave it to those with far more knowledge on how things are expected
to work within V4L2 to decide how this is meant to work, and how it
should all be defined (and documented!). I have no particular views on
it.

  Dave

> 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.
>
> Let me try to summarize the outcome of the discussion
>
> 1) All selection rectangles are defined in respect to the NATIVE_SIZE
>    target, which returns the full pixel array size, which includes
>    readable and non-readable pixels. It's top-left corner is in
>    position (0,0)
>
> 3) CROP_BOUND returns the portion of the full pixel array which can be
>    read out, including optical black pixels, and is defined in respect
>    to the full pixel array size
>
> 2) CROP_DEFAULT returns the portion of the readable part of the pixel array
>    which contains valid image data (aka 'active' pixels). It is again
>    defined in respect to the full pixel array rectangle returned by
>    NATIVE_SIZE target.
>
> With an ack on my understanding, I think it's worth expanding the
> target documentation a bit. As I've said I've been hesitant in doing
> so, as those targets do not only apply to image sensors, but I think a
> section that goes like "If the sub-device represents and image sensor
> then the V4L2_SEL_TGT_.. target represents ... "
>
> Laurent: this will have some impact on libcamera as well
> https://git.linuxtv.org/libcamera.git/tree/src/libcamera/camera_sensor.cpp#n503
> When we read the analogCrop rectangle, we decided it is defined in
> respect to the active portion of the pixel array (CROP_DEFAULT) and
> not from the full pixel array size (NATIVE_SIZE).
>
> We then should:
> 1) Back-track on our decision to have analog crop defined in respect
> to the active part and decide it should be defined in respect to the
> full pixel array: this has implications on the existing IPAs that
> assume what we have defined
>
> 2) Adjust the analogCrop rectangle by subtracting to its sizes the
> values reported by TGT_CROP_DEFAULT.top and TGT_CROP_DEFAULT.left.
>
> I would got for 2) what's your opinion ?
>
> On this specific patch:
>
> >
> > 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,
>
> we have
> #define IMX219_PIXEL_ARRAY_LEFT         8U
> #define IMX219_PIXEL_ARRAY_TOP          8U
>
> Which I would then re-name IMX219_ACTIVE_ARRAY_LEFT and
> IMX219_ACTIVE_ARRAY_TOP and re-use it there
>
>
> >                       .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:
>
> I think this is fine and that's my reasoning:
>
> The IMAX219 pixel array is documented as
>
>         /-------------- 3296 -------------------/
>          8                                     8
>         +---------------------------------------+    /
>         |IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII| 8  |
>         |IpppppppppppppppppppppppppppppppppppppI|    |
>         |IpppppppppppppppppppppppppppppppppppppI|    |
>         |IpppppppppppppppppppppppppppppppppppppI|    |
>         |IpppppppppppppppppppppppppppppppppppppI|    |
>         |IpppppppppppppppppppppppppppppppppppppI|    |
>         |IpppppppppppppppppppppppppppppppppppppI|    |
>
>                             ....                    2480
>
>         |IpppppppppppppppppppppppppppppppppppppI|    |
>         |IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII|    |
>         |IoooooooooooooooooooooooooooooooooooooI| 16 |
>         |IOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOI| 16 |
>         |IoooooooooooooooooooooooooooooooooooooI| 8  |
>         +---------------------------------------+    /
>
> Where:
>         I = invalid active area
>         p = valid pixels
>         o = Invalid OB area
>         O = Valid OB area
>         Effective area = 3296x2480
>         Active area = 3280x2464
>
> The 'active area' is then sorrounded by 8 invalid rows, 8 invalid
> cols at the beginning and the end, and followed by 8 more invalid
> rows. Then, 16 invalid black rows follow, then 16 -valid- black
> rows, then 8 invalid black rows again.
>
> My understanding is that the whole effective area is sent on the bus,
> as the CSI-2 timings report the sizes of the 'effective' area to be
> the whole 3296x2480 matrix, and assigns a CSI-2 data-type to the
> "Valid OB area" while sets the DataType for invalid areas to Null.
>
> However the registers that select the analog crop work on the 'active
> area' only, so there is not way to select "I want the Valid OB area"
> only, as the whole 'effective area' is sent on the bus and the CSI-2
> receivers filters on the DataType to discard the 'Invalid' lines (to
> which a Null DataType is assigned) and capture image data ('active area')
> and eventually 'Valid black' pixels (which have a data type assigned).
>
> All of this to say, there's no point in reporting a TGT_BOUND larger
> that the active area, as the user cannot select portions outside of it
> through the S_SELECTION API, but can only instruct it's CSI-2 receiver
> to filter-in the data it desires, which I think we're missing an API
> for.
>
> Hans: would you like to go ahead with this patch, or should I take
> over and change the libcamera part that parses these properties in one
> go ?
>
> Thanks
>   j
>
> >               sel->r.top = IMX219_PIXEL_ARRAY_TOP;
> >               sel->r.left = IMX219_PIXEL_ARRAY_LEFT;
> >               sel->r.width = IMX219_PIXEL_ARRAY_WIDTH;
> > --
> > 2.27.0
> >
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series

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,
 			.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:
 		sel->r.top = IMX219_PIXEL_ARRAY_TOP;
 		sel->r.left = IMX219_PIXEL_ARRAY_LEFT;
 		sel->r.width = IMX219_PIXEL_ARRAY_WIDTH;