From: Dave Stevenson <dave.stevenson@raspberrypi.com>
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
Subject: Re: [PATCH v4 02/25] media: i2c: imx258: Make image geometry meet sensor requirements
Date: Tue, 16 Apr 2024 16:18:35 +0100 [thread overview]
Message-ID: <CAPY8ntDMFcNN_kcAd_VWDfPbJ2GAua-TbpyOqLJA-kYJPWz-6A@mail.gmail.com> (raw)
In-Reply-To: <6041186.lOV4Wx5bFT@steina-w>
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 – 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, struct v4l2_subdev_fh *fh)
> > /* Initialize try_fmt */
> > try_fmt->width = supported_modes[0].width;
> > try_fmt->height = supported_modes[0].height;
> > - try_fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> > + try_fmt->code = 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 someone
> 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.pdf
> Best regards,
> Alexander
>
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.tq-group.com/
>
>
next prev parent reply other threads:[~2024-04-16 15:18 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-14 20:34 [PATCH v4 00/25] imx258 improvement series git
2024-04-14 20:34 ` [PATCH v4 01/25] media: i2c: imx258: Remove unused defines git
2024-04-14 20:34 ` [PATCH v4 02/25] media: i2c: imx258: Make image geometry meet sensor requirements git
2024-04-15 6:25 ` Alexander Stein
2024-04-15 16:38 ` Luis Garcia
2024-04-16 15:18 ` Dave Stevenson [this message]
2024-04-14 20:34 ` [PATCH v4 03/25] media: i2c: imx258: Disable digital cropping on binned modes git
2024-04-14 20:34 ` [PATCH v4 04/25] media: i2c: imx258: Remove redundant I2C writes git
2024-04-14 20:34 ` [PATCH v4 05/25] media: i2c: imx258: Add regulator control git
2024-04-14 20:34 ` [PATCH v4 06/25] media: i2c: imx258: Make V4L2_CID_VBLANK configurable git
2024-04-14 20:34 ` [PATCH v4 07/25] media: i2c: imx258: Split out common registers from the mode based ones git
2024-04-14 20:34 ` [PATCH v4 08/25] media: i2c: imx258: Add support for 24MHz clock git
2024-04-14 20:34 ` [PATCH v4 09/25] media: i2c: imx258: Add support for running on 2 CSI data lanes git
2024-04-14 20:34 ` [PATCH v4 10/25] media: i2c: imx258: Follow normal V4L2 behaviours for clipping exposure git
2024-04-14 20:34 ` [PATCH v4 11/25] media: i2c: imx258: Add get_selection for pixel array information git
2024-04-14 20:34 ` [PATCH v4 12/25] media: i2c: imx258: Allow configuration of clock lane behaviour git
2024-04-14 20:34 ` [PATCH v4 13/25] media: i2c: imx258: Correct max FRM_LENGTH_LINES value git
2024-04-14 20:34 ` [PATCH v4 14/25] media: i2c: imx258: Issue reset before starting streaming git
2024-04-14 20:34 ` [PATCH v4 15/25] media: i2c: imx258: Set pixel_rate range to the same as the value git
2024-04-14 20:34 ` [PATCH v4 16/25] media: i2c: imx258: Support faster pixel rate on binned modes git
2024-04-14 20:34 ` [PATCH v4 17/25] dt-bindings: media: imx258: Rename to include vendor prefix git
2024-04-14 20:34 ` [PATCH v4 18/25] dt-bindings: media: imx258: Add alternate compatible strings git
2024-04-14 20:34 ` [PATCH v4 19/25] media: i2c: imx258: Change register settings for variants of the sensor git
2024-04-14 20:34 ` [PATCH v4 20/25] media: i2c: imx258: Make HFLIP and VFLIP controls writable git
2024-04-14 20:34 ` [PATCH v4 21/25] dt-bindings: media: imx258: Add binding for powerdown-gpio git
2024-04-14 20:35 ` [PATCH v4 22/25] media: i2c: imx258: Add support for powerdown gpio git
2024-04-14 20:35 ` [PATCH v4 23/25] media: i2c: imx258: Add support for reset gpio git
2024-04-15 16:00 ` Tommaso Merciai
2024-04-15 16:25 ` Luis Garcia
2024-04-14 20:35 ` [PATCH v4 24/25] media:i2c: imx258: Use v4l2_link_freq_to_bitmap helper git
2024-04-15 15:18 ` Tommaso Merciai
2024-04-15 16:27 ` Luis Garcia
2024-04-15 16:32 ` Tommaso Merciai
2024-04-14 20:35 ` [PATCH v4 25/25] media: i2c: imx258: Convert to new CCI register access helpers git
2024-04-15 14:47 ` Tommaso Merciai
2024-04-15 15:29 ` Luis Garcia
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAPY8ntDMFcNN_kcAd_VWDfPbJ2GAua-TbpyOqLJA-kYJPWz-6A@mail.gmail.com \
--to=dave.stevenson@raspberrypi.com \
--cc=alexander.stein@ew.tq-group.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=festevam@gmail.com \
--cc=git@luigi311.com \
--cc=imx@lists.linux.dev \
--cc=jacopo.mondi@ideasonboard.com \
--cc=kernel@pengutronix.de \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=pavel@ucw.cz \
--cc=phone-devel@vger.kernel.org \
--cc=robh@kernel.org \
--cc=s.hauer@pengutronix.de \
--cc=sakari.ailus@linux.intel.com \
--cc=shawnguo@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).