Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Hans Verkuil <hverkuil@xs4all.nl>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Jacopo Mondi <jacopo@jmondi.org>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>,
	Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>,
	Dave Stevenson <dave.stevenson@raspberrypi.org>,
	naush@raspberrypi.com,
	Linux Media Mailing List <linux-media@vger.kernel.org>,
	Sowjanya Komatineni <skomatineni@nvidia.com>,
	libcamera-devel@lists.libcamera.org
Subject: Re: [PATCH] imx219: selection compliance fixes
Date: Mon, 3 Aug 2020 11:07:38 +0200
Message-ID: <ded11ad4-29b9-1cce-5150-ca8253b1a862@xs4all.nl> (raw)
In-Reply-To: <20200801151317.GF11820@pendragon.ideasonboard.com>

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

  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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

  git send-email \
    --in-reply-to=ded11ad4-29b9-1cce-5150-ca8253b1a862@xs4all.nl \
    --to=hverkuil@xs4all.nl \
    --cc=dave.stevenson@raspberrypi.org \
    --cc=jacopo@jmondi.org \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=libcamera-devel@lists.libcamera.org \
    --cc=linux-media@vger.kernel.org \
    --cc=naush@raspberrypi.com \
    --cc=ricardo.ribalda@gmail.com \
    --cc=sakari.ailus@linux.intel.com \
    --cc=skomatineni@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

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

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 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/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git