Re: [PATCH] imx219: selection compliance fixes

[Hans Verkuil <hverkuil@xs4all.nl>]

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