Hi Dave On Fri, Oct 27, 2023 at 11:29:11AM +0100, Dave Stevenson wrote: > Hi Jacopo > > On Fri, 27 Oct 2023 at 09:57, Jacopo Mondi > <jacopo.mondi@xxxxxxxxxxxxxxxx> wrote: > > > > Hi Andre' > > > > On Wed, Oct 25, 2023 at 11:26:00PM +0200, André Apitzsch wrote: > > > Hi Jacopo, > > > > > > Am Dienstag, dem 24.10.2023 um 09:52 +0200 schrieb Jacopo Mondi: > > > > Hi Andre' > > > > > > > > On Mon, Oct 23, 2023 at 11:47:53PM +0200, André Apitzsch wrote: > > > > > Set effictive and active sensor pixel sizes as shown in product > > > > > > > > s/effictive/effective > > > > > > > > > brief[1]. > > > > > > > > > > [1]: > > > > > https://www.mouser.com/datasheet/2/897/ProductBrief_IMX214_20150428-1289331.pdf > > > > > > > > > > Signed-off-by: André Apitzsch <git@xxxxxxxxxxx> > > > > > --- > > > > > drivers/media/i2c/imx214.c | 39 ++++++++++++++++++++++++++++++++-- > > > > > ----- > > > > > 1 file changed, 32 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/media/i2c/imx214.c > > > > > b/drivers/media/i2c/imx214.c > > > > > index bef8dc36e2d0..a2d441cd8dcd 100644 > > > > > --- a/drivers/media/i2c/imx214.c > > > > > +++ b/drivers/media/i2c/imx214.c > > > > > @@ -36,6 +36,14 @@ > > > > > #define IMX214_EXPOSURE_STEP 1 > > > > > #define IMX214_EXPOSURE_DEFAULT 0x0c70 > > > > > > > > > > +/* IMX214 native and active pixel array size */ > > > > > +#define IMX214_NATIVE_WIDTH 4224U > > > > > +#define IMX214_NATIVE_HEIGHT 3136U > > > > > +#define IMX214_PIXEL_ARRAY_LEFT 8U > > > > > +#define IMX214_PIXEL_ARRAY_TOP 8U > > > > > +#define IMX214_PIXEL_ARRAY_WIDTH 4208U > > > > > +#define IMX214_PIXEL_ARRAY_HEIGHT 3120U > > > > > + > > > > > > > > I do get slightly different numbers from the datasheet version I have > > > > > > > > The sensor is said to have 4224x3208 total pixels of which 4208x3120 > > > > are active ones. > > > > > > > > The pixel array diagram shows 64 "OPB" (optically black ?) lines, > > > > followed by 8 dummy lines, followed by 3120 valid lines. There are 8 > > > > dummy columns at each side of the 4208 valid ones. > > > > > > > > Now, NATIVE which represents the full pixel array size seems to be > > > > 4224x3208 (other parts of the datasheet only report 3200 lines > > > > though) > > > > > > > > BOUNDS represents the readabale array area, which I presume > > > > corresponds to what is named as 'effective area' by the datasheet. It > > > > excludes the OPB lines at the top of the image and seems to be > > > > represented by (0, 64, 4224, 3160). > > > > > > > > CROP_DEFAULT represents the default crop rectangle which covers the > > > > active pixel area, so it excludes 8 more lines of dummy pixels and 8 > > > > dummy columns, which gives a rectangle (8, 72, 4208, 3120) > > > > > > > > Also note that the driver always reports a TGT_CROP rectangle with > > > > top/left points set to 0. If my understanding is correct, V4L2 > > > > selection targets are defined from the most external target > > > > (TGT_NATIVE in this case), and the driver should be corrected to > > > > initialize the crop rectangle with a top-left corner at (8, 72). > > > > > > > > Does this make sense ? > > > > > > As far as I understood, only the effective and active sizes of three > > > sizes provided in the datasheet (total, effective and active) matter. > > > By comparing the values used in imx219.c (and imx415.c) with the ones > > > in the corresponding datasheets [1,2] I assume, that "effective" > > > matches "NATIVE_SIZE", "active" matches "CROP_DEFAULT" and "total" is > > > ignored. > > > > imx219 driver indeed does not consider the OPB areas in the definition > > of the rectangles... > > I know it sounds ridiculous as I've been the one adding selection support to imx219, but I presume we discussed it somewhen in the past: do you happen to remember why we left the OPB area out from the native sizes ? (Does OPB stand for "Optically black" ? ) > > Also looking at the X/Y_ADDR_START value assigned in the register tables > > for full resolution mode (3280x2462) they have value of 0, indeed > > meaning the active area is the only readable one. > > > > Then yes, you're right, for imx219 > > NATIVE = effective > > CROP_DEFAULT = BOUND = active > > I presume you can confirm this, right ? > > > The commit message of 1ed36ecd1459b653cced8929bfb37dba94b64c5d ("media: > > > i2c: imx219: Selection compliance fixes") seems to support me here: > > > > > > > > > The top/left crop coordinates of the TGT_CROP rectangle were set to > > > > (0, 0) instead of (8, 8) which is the offset from the larger physical > > > > pixel array rectangle. > > > > > > This (8, 8) is half the difference between number of effective and > > > active pixels of imx219[1]. > > > > > > Together with the 8 dummy lines and 8 dummy columns you mentioned, I > > > still think my values are right. But I've just started working with > > > V4L2, so I might be wrong. > > > > To actually verify if the 'effective area' is readable or not, we > > should know what register controls the X/Y_ADDR_START value, and > > that's an information I don't have in my version of the datasheet. > > I happen to have an IMX214 datasheet. > X_ADDR_START is 0x0344/5 (set in multiples of 2) > Y_ADDR_START is 0x0346/7 (set in multiples of 4) > X_ADDR_END is 0x0348/9 (set in multiples of 2) > Y_ADDR_END is 0x034a/b (set in multiples of 4) > X_OUTPUT_SIZE 0x034c/d > Y_OUTPUT_SIZE 0x034e/f > > X direction are 13bit values, Y direction are 12 bit. > [12:8] or [11:8] in the low bits of the first register, [7:0] in the > second register. AH thanks! Unfortunately the largest imx214 mode is cropped from full pixel array it seems, so not that helpful :( > > Dave > > > It's however plausible that it behaves the same as imx219, as the > > driver's register sequences seems to program the crop sizes in > > register 0x034c and 0x034e and there's not programmed top-left corner > > there. > > > > Ok then, let's be consistent and do the same as imx219 as you're doing > > here. > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> > > > > > > > > Could you share the imx214 datasheet with me? > > > > > > Best regards, > > > André > > > > > > [1] https://www.arducam.com/downloads/modules/RaspberryPi_camera/IMX219DS.PDF > > > [2] https://www.sony-semicon.com/files/62/pdf/p-12_IMX415-AAQR_AAMR_Flyer.pdf > > > > > > > > Thanks > > > > j > > > > > > > > > > > > > static const char * const imx214_supply_name[] = { > > > > > "vdda", > > > > > "vddd", > > > > > @@ -634,14 +642,31 @@ static int imx214_get_selection(struct > > > > > v4l2_subdev *sd, > > > > > { > > > > > struct imx214 *imx214 = to_imx214(sd); > > > > > > > > > > - if (sel->target != V4L2_SEL_TGT_CROP) > > > > > - return -EINVAL; > > > > > + switch (sel->target) { > > > > > + case V4L2_SEL_TGT_CROP: > > > > > + mutex_lock(&imx214->mutex); > > > > > + sel->r = *__imx214_get_pad_crop(imx214, sd_state, > > > > > sel->pad, > > > > > + sel->which); > > > > > + mutex_unlock(&imx214->mutex); > > > > > + return 0; > > > > > > > > > > - mutex_lock(&imx214->mutex); > > > > > - sel->r = *__imx214_get_pad_crop(imx214, sd_state, sel- > > > > > >pad, > > > > > - sel->which); > > > > > - mutex_unlock(&imx214->mutex); > > > > > - return 0; > > > > > + case V4L2_SEL_TGT_NATIVE_SIZE: > > > > > + sel->r.top = 0; > > > > > + sel->r.left = 0; > > > > > + sel->r.width = IMX214_NATIVE_WIDTH; > > > > > + sel->r.height = IMX214_NATIVE_HEIGHT; > > > > > + return 0; > > > > > + > > > > > + case V4L2_SEL_TGT_CROP_DEFAULT: > > > > > + case V4L2_SEL_TGT_CROP_BOUNDS: > > > > > + sel->r.top = IMX214_PIXEL_ARRAY_TOP; > > > > > + sel->r.left = IMX214_PIXEL_ARRAY_LEFT; > > > > > + sel->r.width = IMX214_PIXEL_ARRAY_WIDTH; > > > > > + sel->r.height = IMX214_PIXEL_ARRAY_HEIGHT; > > > > > + return 0; > > > > > + } > > > > > + > > > > > + return -EINVAL; > > > > > } > > > > > > > > > > static int imx214_entity_init_cfg(struct v4l2_subdev *subdev, > > > > > > > > > > -- > > > > > 2.42.0 > > > > > > > >
