Message-ID: <CAPY8ntA0VWjj2q47sd2AwixjYKrzgP4KddoeDkPyqOepb+4JFg@mail.gmail.com>
Date:   Fri, 27 Oct 2023 14:17:59 +0100
From:   Dave Stevenson <dave.stevenson@...pberrypi.com>
To:     Jacopo Mondi <jacopo.mondi@...asonboard.com>
Cc:     André Apitzsch <git@...tzsch.eu>,
        Ricardo Ribalda <ribalda@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH 4/4] media: i2c: imx214: Add sensor's pixel matrix size

Hi Jacopo

On Fri, 27 Oct 2023 at 12:12, Jacopo Mondi
<jacopo.mondi@...asonboard.com> wrote:
>
> 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@...asonboard.com> 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@...tzsch.eu>
> > > > > > ---
> > > > > >  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" ? )

Yes, OPB is Optical Black. Optically black pixels can be used for
subtracting dark noise from pixels.
OPB lines are often sent on a totally separate CSI-2 data type, so
they will generally not apply to the image buffer that is read out.
See IMX290/462 for an example of that (it uses data type 0x37)
I believe some other sensors use the OPB internally to perform
compensation on-sensor.

I've never fully followed what the full selection API is meant to be
used for beyond determining which parts of the array are cropped out
in particular modes. It's numbers that I extract from the datasheet,
stick in the driver, and the user can do something with them if they
wish.

> > > 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 ?

Sounds about 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 :(

Yup, I noticed that too. Make the selection reflect the register set -
it's all you can do.

I don't know why it got cropped down so much to make a 16:9 full res
mode. Possibly a limit of 4096 on an ISP or receiver somewhere, or
potentially just the use case.
I guess there's nothing stopping an extra mode being added which uses
the full sensor array.

  Dave

> >
> >   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@...asonboard.com>
> > >
> > > >
> > > > 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
> > > > > >
> > > >

Powered by blists - more mailing lists