Received-SPF: pass (google.com: domain of linux-kernel+bounces-147105-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240414203503.18402-1-git@luigi311.com> <20240414203503.18402-3-git@luigi311.com>
 <6041186.lOV4Wx5bFT@steina-w>
In-Reply-To: <6041186.lOV4Wx5bFT@steina-w>
From: Dave Stevenson <dave.stevenson@raspberrypi.com>
Date: Tue, 16 Apr 2024 16:18:35 +0100
Message-ID: <CAPY8ntDMFcNN_kcAd_VWDfPbJ2GAua-TbpyOqLJA-kYJPWz-6A@mail.gmail.com>
Subject: Re: [PATCH v4 02/25] media: i2c: imx258: Make image geometry meet
 sensor requirements
To: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: linux-media@vger.kernel.org, git@luigi311.com, 
	jacopo.mondi@ideasonboard.com, mchehab@kernel.org, robh@kernel.org, 
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, shawnguo@kernel.org, 
	s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com, 
	sakari.ailus@linux.intel.com, devicetree@vger.kernel.org, imx@lists.linux.dev, 
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, 
	pavel@ucw.cz, phone-devel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Alexander

On Mon, 15 Apr 2024 at 07:25, Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hi,
>
> Am Sonntag, 14. April 2024, 22:34:40 CEST schrieb git@luigi311.com:
> > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >
> > The output image is defined as being 4208x3118 pixels in size.
> > Y_ADD_STA register was set to 0, and Y_ADD_END to 3118, giving
> > 3119 lines total.
> >
> > The datasheet lists a requirement for Y_ADD_STA to be a multiple
> > of a power of 2 depending on binning/scaling mode (2 for full pixel,
> > 4 for x2-bin/scale, 8 for (x2-bin)+(x2-subsample) or x4-bin, or 16
> > for (x4-bin)+(x2-subsample)).
> > (Y_ADD_END =E2=80=93 Y_ADD_STA + 1) also has to be a similar power of 2=
.
> >
> > The current configuration for the full res modes breaks that second
> > requirement, and we can't increase Y_ADD_STA to 1 to retain exactly
> > the same field of view as that then breaks the first requirement.
> > For the binned modes, they are worse off as 3118 is not a multiple of
> > 4.
> >
> > Increase the main mode to 4208x3120 so that it is the same FOV as the
> > binned modes, with Y_ADD_STA at 0.
> > Fix Y_ADD_STA and Y_ADD_END for the binned modes so that they meet the
> > sensor requirements.
> >
> > This does change the Bayer order as the default configuration is for
> > H&V flips to be enabled, so readout is from Y_STA_END to Y_ADD_STA,
> > and this patch has changed Y_STA_END.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Signed-off-by: Luis Garcia <git@luigi311.com>
> > Reviewed-by: Pavel Machek <pavel@ucw.cz>
> > ---
> >  drivers/media/i2c/imx258.c | 26 +++++++++++++-------------
> >  1 file changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > index 2dbafd21dd70..4a7048d834c6 100644
> > --- a/drivers/media/i2c/imx258.c
> > +++ b/drivers/media/i2c/imx258.c
> > [snip]
> > @@ -707,7 +707,7 @@ static int imx258_open(struct v4l2_subdev *sd, stru=
ct v4l2_subdev_fh *fh)
> >       /* Initialize try_fmt */
> >       try_fmt->width =3D supported_modes[0].width;
> >       try_fmt->height =3D supported_modes[0].height;
> > -     try_fmt->code =3D MEDIA_BUS_FMT_SGRBG10_1X10;
> > +     try_fmt->code =3D MEDIA_BUS_FMT_SBGGR10_1X10;
>
> Does someone have access to the data sheet? I am wondering how the
> arrangement of the pixel array is shown. I've the following (identical)
> array for these sensors:
> * imx290/imx327
> * imx219
> * imx415
>
> G B G B
> R G R G
> G B G B
> R G R G

Check the other 3 corners of your diagrams - they are not identical.

> Yet each driver configures a different bus format:
>
> * imx290.c: MEDIA_BUS_FMT_SRGGB10_1X10
> * imx415.c: MEDIA_BUS_FMT_SGBRG10_1X10
> * imx219.c: MEDIA_BUS_FMT_SRGGB10_1X10 (no flip)
>
> imx219 actually defines all 4 10 Bit Bayer patterns and the comment
> indicates this depends on how v or h flip is configured.
> Reading the commit message apparently the same is true for this driver.

Correct.

> Still this is confusing as I would have expected flipping to be disabled =
by
> default, expecting the same Bayer pattern order for all drivers. Can some=
one
> shed some light?

Comparing different families of sensors isn't really valid, and
manufacturers vary too.

imx290 is a Starvis sensor, and has an array of 1945x1109, so all 4
corners are red pixels. It crops an even number of pixels off the
array in the direction of readout, therefore always producing RGGB.

I don't have a datasheet for imx415. Whilst it is also a Starvis
sensor the product brief [1] says it is an array of 3864 x 2228, with
3864 x 2192 effective pixels, which implies it isn't doing the same as
imx290. However the current driver isn't changing the Bayer order
based on flip which contradicts that. Pass as to which is correct.
I can't answer why the default order is GBRG. Presumably the default
window mode used (it doesn't use X_START / Y_START registers) crops an
odd number of lines off the raw array, therefore starting on a GB row.

imx219 (Exmor R) and imx258 (Exmor RS) datasheets have an even number
of pixels in each direction in the array, and whilst the first pixel
read out in the default direction is red, the colours in the opposite
corner is blue, with green in the remaining corners. This is why the
Bayer order changes with flips.

Most Omnivision sensors I've encountered do the same as imx219/258,
whilst OnSemi sensors are the same as imx290. Drivers obviously have
to match whatever the hardware does.

  Dave

[1] https://www.sony-semicon.com/files/62/pdf/p-12_IMX415-AAQR_AAMR_Flyer.p=
df

> Best regards,
> Alexander
>
> --
> TQ-Systems GmbH | M=C3=BChlstra=C3=9Fe 2, Gut Delling | 82229 Seefeld, Ge=
rmany
> Amtsgericht M=C3=BCnchen, HRB 105018
> Gesch=C3=A4ftsf=C3=BChrer: Detlef Schneider, R=C3=BCdiger Stahl, Stefan S=
chneider
> http://www.tq-group.com/
>
>