Re: [PATCH] imx219: selection compliance fixes

[Jacopo Mondi <jacopo@jmondi.org>]

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