Re: [PATCH 3/6] media: imx335: Update the native pixel array width

[Jai Luthra <jai.luthra@ideasonboard.com>]

[-- Attachment #1: Type: text/plain, Size: 4480 bytes --]

Quoting Kieran Bingham (2025-08-13 15:45:59)
> Quoting Jai Luthra (2025-08-13 08:20:34)
> > The sensor datasheet reports actual total number of pixels as 2696x2044.
> 
> Err ...
> 
> Page 2 of the IMX335LQN-D datasheet I have says:
> 
> Total number of pixels: 2704 (H) x 2104 (V) approx 5.69 M pixels
> Number of Effective Pixels: 2616 (H) x 1964 (V) approx 5.14 M pixels
> Number of Active Pixels: 2616 (H) x 1964 (V) approx 5.04 M pixels
> 
> Where does 2696x2044 come from ?
> 
> 
> Argh - then on page 8 - indeed it says
> Total Number of pixels 2696(H) x 2044(V) = 5.51M
> 

Yeah, I don't know which one to use, but the page 8 seems "safer".

> 
> In imx283.c I've moved to defining these windows as a v4l2_rect. I find
> that's a nicer way to convey the rectangles specifically instead of
> through #defines and then they can be directly used to report crop
> rectangles:
> 
> i.e.:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/drivers/media/i2c/imx283.c:
> 
> /* IMX283 native and active pixel array size. */
> static const struct v4l2_rect imx283_native_area = {
>         .top = 0,
>         .left = 0,
>         .width = 5592,
>         .height = 3710,
> };
> 
> static const struct v4l2_rect imx283_active_area = {
>         .top = 40,
>         .left = 108,
>         .width = 5472,
>         .height = 3648,
> };
> 
> Not required - but just an idea (that obviously I like :D)
> 

Ah that is indeed quite nice, will do the same in v2.

> 
> > 
> > This becomes important for supporting 2x2 binning modes that can go
> > beyond the current maximum pixel array width set here.
> > 
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > ---
> >  drivers/media/i2c/imx335.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> > index 6369bdbd2b09ba1f89c143cdf6be061820f2d051..dbf2db4bf9cbfd792ff5865264c6f465eb79a43b 100644
> > --- a/drivers/media/i2c/imx335.c
> > +++ b/drivers/media/i2c/imx335.c
> > @@ -124,10 +124,10 @@
> >  #define IMX335_NUM_DATA_LANES  4
> >  
> >  /* IMX335 native and active pixel array size. */
> > -#define IMX335_NATIVE_WIDTH            2616U
> > -#define IMX335_NATIVE_HEIGHT           1964U
> > -#define IMX335_PIXEL_ARRAY_LEFT                12U
> > -#define IMX335_PIXEL_ARRAY_TOP         12U
> > +#define IMX335_NATIVE_WIDTH            2696U
> 
> The all scan mode on page 56 doesn't show these at all.
> Just 12 + 2592 + 12
> 
> But I think that's the datasheet being inconsistent/restrictive about
> what it says an all scan mode could be.
> 
> It would be interesting to make a 'super resolution' output mode that
> can transmit every pixel possible but not required for this series
> development just for fun tests.
> 

Yeah I think datasheet is being quite restrictive, and even the +12 on both
sides for "color processing" makes it very difficult to parse what the
actual resolution is on the wire, and what they expect to be used post ISP
processing.

The all-pixel scan mode table given in the datasheet is actually different
from what we actually use (we use WINMODE=4 i.e. cropped) for the 2592x1944
mode in driver, as the datasheet settings would give us 2592+24 = 2616
pixels per line.

I didn't want to change the existing mode in this series though. Making the
driver freely-configurable will fix all of this (hopefully).

> I'm torn here - as the datasheet changes it's points of reference to
> make it's "all scan mode" essentially start at 0 which is clearly not
> correct against the 'native' positions.
> 
> So ... I think I'm just going to say I think we don't believe the
> datasheet as we *know* there are more pixels and we are using them so :
> 
> 
> > +#define IMX335_NATIVE_HEIGHT           2044U
> > +#define IMX335_PIXEL_ARRAY_LEFT                52U
> > +#define IMX335_PIXEL_ARRAY_TOP         50U
> 
> I see you have taken the '80' extra pixels on both width and height and
> divided half before and half after centering the window. I have no
> information to say if it's position is otherwise so I think this is all
> we can do:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >  #define IMX335_PIXEL_ARRAY_WIDTH       2592U
> >  #define IMX335_PIXEL_ARRAY_HEIGHT      1944U
> >  
> > 
> > -- 
> > 2.50.1
> >

Thanks,
Jai

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEETeDYGOXVdejUWq/FQ96R+SSacUUFAmikI/wACgkQQ96R+SSa
cUUhrg/8CaJH8DpYJ0aYicgshRm6ZP3Isy9DSxNCy4UC9iKY3+/mNu1N2gewyP3M
zRYujQIhE74IAxh/WaSyn/UdEiyJ2NIzu5fhmAGkN9jEV5ZRhTSW0pAum+WzpMsv
PRgF4dLnkxWon6xoVoxGIIV7kU+dyfWhLy6KO8ES5mWbR0grtx5z7MbNSyeNdXsR
QXgTRUwhSeIfR759Juh5/ZueubFPnC7eYyNjNXgjoRNGHnIDCgK6kze3ZoVUCh4Y
gXniEfuzZNGm87N6TY4HkSUtPyowemUeV9JiwXzMS1hSQ1AqFvYcdYhO1i/CS2Sj
u/YOz4SnON+fB8YVihSoSas5xGWdfmc/qyEI4DpqwrfYH5t0TQJeTruEl4X7EzSc
y4an+wV6o+UzaW12Z58qQLQEHHYYREViNNDnoJaPjApr7NbPWGXE7BzlK+a89Ym/
nI907+jNB7fjCF+HNYQO0lnjbIFDA+l4yzbNe6RZZBN5u19KYPo67Goa7nVQvaPH
64HzZhCR6Ir8K8oIr5oXWWfOQJsUVCLHeDe2jNdrm2mIV2O2NtNrcEd43cucw0ef
bhakI8+TqLup9W/AckBQhR+dQURg/GvZtF/YoCeaInzSXa4ebbn8MIpa76n0jC99
HSn1aHO9Ry3unDS8JOng5bzByphjGDVEEJ+CmLV2DMHT9+o0PVE=
=14ti
-----END PGP SIGNATURE-----