by Luis Garcia

[permalink] [raw]

Subject: Re: [PATCH v4 02/25] media: i2c: imx258: Make image geometry meet sensor requirements

On 4/15/24 00:25, Alexander Stein wrote:
> Hi,
>
> Am Sonntag, 14. April 2024, 22:34:40 CEST schrieb [email protected]:
>> From: Dave Stevenson <[email protected]>
>>
>> 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 <[email protected]>
>> Reviewed-by: Jacopo Mondi <[email protected]>
>> Signed-off-by: Luis Garcia <[email protected]>
>> Reviewed-by: Pavel Machek <[email protected]>
>> ---
>> 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
>

I assume this is what you are looking for
https://photos.luigi311.com/share/Imk6odsR_44VYsRvfmRwBVoG1TMnXtI61PP4sjssbmKAcNEYuVPRa9W-vIU7rpa-Ask

> 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.
>> 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?
>
> Best regards,
> Alexander
>

Flipping defaults are changed around looks like based on use cases. Patch 20
is what actually brings in flipping and brings in the 4 definitions like you
are expecting

+ /* 10-bit modes. */
+ MEDIA_BUS_FMT_SRGGB10_1X10,
+ MEDIA_BUS_FMT_SGRBG10_1X10,
+ MEDIA_BUS_FMT_SGBRG10_1X10,
+ MEDIA_BUS_FMT_SBGGR10_1X10

2024-04-16 15:19:06

by Dave Stevenson

[permalink] [raw]

Subject: Re: [PATCH v4 02/25] media: i2c: imx258: Make image geometry meet sensor requirements

Hi Alexander

On Mon, 15 Apr 2024 at 07:25, Alexander Stein
<[email protected]> wrote:
>
> Hi,
>
> Am Sonntag, 14. April 2024, 22:34:40 CEST schrieb [email protected]:
> > From: Dave Stevenson <[email protected]>
> >
> > 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 <[email protected]>
> > Reviewed-by: Jacopo Mondi <[email protected]>
> > Signed-off-by: Luis Garcia <[email protected]>
> > Reviewed-by: Pavel Machek <[email protected]>
> > ---
> > 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/
>
>