[libcamera-devel] [PATCH] imx219: selection compliance fixes
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Aug 1 17:13:17 CEST 2020
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.
> We then should:
> 1) Back-track on our decision to have analog crop defined in respect
> to the active part and decide it should be defined in respect to the
> full pixel array: this has implications on the existing IPAs that
> assume what we have defined
>
> 2) Adjust the analogCrop rectangle by subtracting to its sizes the
> values reported by TGT_CROP_DEFAULT.top and TGT_CROP_DEFAULT.left.
>
> I would got for 2) what's your opinion ?
Inside libcamera I would express all crop rectangles relatively to
PixelArraySize, so mapping to V4L2 would require adding/subtracting the
TGT_CROP_DEFAULT offset. That's why requiring NATIVE_SIZE == CROP_BOUND
may simplify things.
> On this specific patch:
>
> > Signed-off-by: Hans Verkuil <hverkuil-cisco at 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,
>
> we have
> #define IMX219_PIXEL_ARRAY_LEFT 8U
> #define IMX219_PIXEL_ARRAY_TOP 8U
>
> Which I would then re-name IMX219_ACTIVE_ARRAY_LEFT and
> IMX219_ACTIVE_ARRAY_TOP and re-use it there
>
> > .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:
>
> I think this is fine and that's my reasoning:
>
> The IMAX219 pixel array is documented as
>
> /-------------- 3296 -------------------/
> 8 8
> +---------------------------------------+ /
> |IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII| 8 |
> |IpppppppppppppppppppppppppppppppppppppI| |
> |IpppppppppppppppppppppppppppppppppppppI| |
> |IpppppppppppppppppppppppppppppppppppppI| |
> |IpppppppppppppppppppppppppppppppppppppI| |
> |IpppppppppppppppppppppppppppppppppppppI| |
> |IpppppppppppppppppppppppppppppppppppppI| |
>
> .... 2480
>
> |IpppppppppppppppppppppppppppppppppppppI| |
> |IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII| |
> |IoooooooooooooooooooooooooooooooooooooI| 16 |
> |IOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOI| 16 |
> |IoooooooooooooooooooooooooooooooooooooI| 8 |
> +---------------------------------------+ /
>
> Where:
> I = invalid active area
> p = valid pixels
> o = Invalid OB area
> O = Valid OB area
> Effective area = 3296x2480
> Active area = 3280x2464
This doesn't match your diagram above, 8+2464+16+16+8 != 2480.
> The 'active area' is then sorrounded by 8 invalid rows, 8 invalid
> cols at the beginning and the end, and followed by 8 more invalid
> rows. Then, 16 invalid black rows follow, then 16 -valid- black
> rows, then 8 invalid black rows again.
>
> My understanding is that the whole effective area is sent on the bus,
> as the CSI-2 timings report the sizes of the 'effective' area to be
> the whole 3296x2480 matrix, and assigns a CSI-2 data-type to the
> "Valid OB area" while sets the DataType for invalid areas to Null.
>
> However the registers that select the analog crop work on the 'active
> area' only, so there is not way to select "I want the Valid OB area"
> only, as the whole 'effective area' is sent on the bus and the CSI-2
> receivers filters on the DataType to discard the 'Invalid' lines (to
> which a Null DataType is assigned) and capture image data ('active area')
> and eventually 'Valid black' pixels (which have a data type assigned).
I'm not aware of CSI-2 receivers that can capture NULL data types. At
most you'll be able to capture OB and active pixels, nothing else.
> All of this to say, there's no point in reporting a TGT_BOUND larger
> that the active area, as the user cannot select portions outside of it
> through the S_SELECTION API, but can only instruct it's CSI-2 receiver
> to filter-in the data it desires, which I think we're missing an API
> for.
This however contradicts your proposal above, where you said that
CROP_BOUND should include the OB lines :-)
> Hans: would you like to go ahead with this patch, or should I take
> over and change the libcamera part that parses these properties in one
> go ?
>
> > sel->r.top = IMX219_PIXEL_ARRAY_TOP;
> > sel->r.left = IMX219_PIXEL_ARRAY_LEFT;
> > sel->r.width = IMX219_PIXEL_ARRAY_WIDTH;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list