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@xxxxxxxxx> > > --- > > 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
