Linux-Media Archive on
 help / color / Atom feed
From: Hans Verkuil <>
To: Laurent Pinchart <>,
	Jacopo Mondi <>
Cc: Sakari Ailus <>,
	Ricardo Ribalda Delgado <>,
	Dave Stevenson <>,,
	Linux Media Mailing List <>,
	Sowjanya Komatineni <>,
Subject: Re: [PATCH] imx219: selection compliance fixes
Date: Mon, 3 Aug 2020 11:07:38 +0200
Message-ID: <> (raw)
In-Reply-To: <>

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

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.



  parent reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-02 13:50 Hans Verkuil
2020-07-14 12:31 ` Jacopo Mondi
2020-07-14 23:49   ` Laurent Pinchart
2020-07-15  7:19     ` Jacopo Mondi
2020-07-16  9:48       ` Hans Verkuil
2020-07-16 12:59         ` Laurent Pinchart
2020-07-16 13:53           ` Hans Verkuil
2020-07-16 23:20             ` Laurent Pinchart
2020-07-17  6:34               ` Jacopo Mondi
2020-08-01 11:19 ` Jacopo Mondi
2020-08-01 15:13   ` Laurent Pinchart
2020-08-03  8:33     ` Jacopo Mondi
2020-08-03  9:07     ` Hans Verkuil [this message]
2020-08-03  8:37   ` Hans Verkuil
2020-08-03  9:58   ` [libcamera-devel] " Dave Stevenson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Media Archive on

Archives are clonable:
	git clone --mirror linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ \
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:

AGPL code for this site: git clone