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
