| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPY8ntD3k48cPfOQK2iEveLHahhFMV9JMwfq3sV-SvnrsZkxng@mail.gmail.com>
Date: Wed, 26 Feb 2025 12:52:46 +0000
From: Dave Stevenson <dave.stevenson@...pberrypi.com>
To: Richard Leitner <richard.leitner@...ux.dev>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] media: i2c: ov9282: fix analogue gain maximum
On Wed, 26 Feb 2025 at 11:07, Richard Leitner <richard.leitner@...ux.dev> wrote:
>
> Hi Dave,
> thanks for the quick and detailled reply!
>
> On Tue, Feb 25, 2025 at 03:30:16PM +0000, Dave Stevenson wrote:
> > Hi Richard
> >
> > On Tue, 25 Feb 2025 at 13:09, Richard Leitner <richard.leitner@...ux.dev> wrote:
> > >
> > > The sensors analogue gain is stored within two "LONG GAIN" registers
> > > (0x3508 and 0x3509) where the first one holds the upper 5 bits of the
> > > value. The second register (0x3509) holds the lower 4 bits of the gain
> > > value in its upper 4 bits. The lower 4 register bits are fraction bits.
> > >
> > > This patch changes the gain control to adhere to the datasheet and
> > > make the "upper gain register" (0x3508) accessible via the gain control,
> > > resulting in a new maximum of 0x1fff instead of 0xff.
> > >
> > > As the "upper gain register" is now written during exposure/gain update
> > > remove the hard-coded 0x00 write to it from common_regs.
> > >
> > > We cover only the "real gain format" use-case. The "sensor gain
> > > format" one is ignored as based on the hard-coded "AEC MANUAL" register
> > > configuration it is disabled.
> > >
> > > All values are based on the OV9281 datasheet v1.01 (09.18.2015).
> >
> > My web searches turn up a 1.53 from Jan 2019 -
> > http://www.sinotimes-tech.com/product/20220217221034589.pdf
> > That lists 0x3508 as DEBUG, not LONG_GAIN.
>
> Thanks. That helps a lot :-)
>
> >
> > The current range allows analogue gain to x15.9375.
> > Expanding it to 0x1ff.f would be up to x511.9375. I believe that
> > equates to ~54dB as we're scaling voltages, not power. The spec sheet
> > for the sensor lists S/N of 38dB and dynamic range of 68dB, so x511
> > will be almost pure noise.
> >
> > Doing a very basic test using i2ctransfer to set gain values whilst
> > the sensor is running suggests that the image is the same regardless
> > of bits 2-4 of 0x3508. Setting either bits 0 or 1 increases the gain
> > by around x8.5, but they don't combine.
> >
> > Overall can I ask how you've tested that a range up to 0x1fff works,
> > and on which module? I currently don't believe this works as intended.
>
> I've done some basic testing on a vision components OV9281 module.
> Nonetheless it seems I should have done more testing... I just ran a
> "gain sweep" test and it seems you are perfectly right...
>
> The lower two bits of 0x3508 have some kind of offset and "flattening" effect
> on the applied gain, like shown in the plot (X is the gain, Y is the reported
> mean brightness of the picture, read by magick).
>
> 45 +---------------------------------------------------------------------+
> | + + + + + |
> | A AA |
> 40 |-+ AAA AA +-|
> | AA A AAA |
> 35 |-+ AA AAA A +-|
> | AA AAAA AAAAAA|
> | AA AAAA AAAAAA |
> 30 |-+ AA AAAA AAAAAA +-|
> | AA AAA AAAAAA |
> | A A A |
> 25 |-+ AA +-|
> | A |
> | A |
> 20 |-+ AA +-|
> | A |
> 15 |-AA +-|
> |A |
> |A + + + + + |
> 10 +---------------------------------------------------------------------+
> 0x0080 0x0100 0x0180 0x0200 0x0280 0x0300
> gain
>
> This pattern repeats up to 0x1fff, so I guess all other bits of 0x3508
> have no effect on the gain (like shown in the plot attached as png, as
> it got way to big for ascii).
>
> I'm sorry for the inconvenience caused... I've somehow messed up my
> initial tests.
No problem - this is why we review and test patches.
My general view would be that anything over x64 on analogue gain would
be unusual, and on most sensors x16 is about the limit of useful gain.
> Thank you again for your feedback!
>
> So please feel free to ignore this patch. Should I send a new series
> with just the two minor patches of this series? Or should I include them
> in the next series for the ov9282 driver?
Up to Sakari.
The other two patches have my R-b responses, so taking those two and
ignoring this one should be fairly simple, but it just depends upon
workflows.
Dave
> regards;rl
>
> >
> > Dave
> >
> > > Signed-off-by: Richard Leitner <richard.leitner@...ux.dev>
> > > ---
> > > drivers/media/i2c/ov9282.c | 17 ++++++++++-------
> > > 1 file changed, 10 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > index c882a021cf18852237bf9b9524d3de0c5b48cbcb..e6effb2b42d4d5d0ca3d924df59c60512f9ce65d 100644
> > > --- a/drivers/media/i2c/ov9282.c
> > > +++ b/drivers/media/i2c/ov9282.c
> > > @@ -54,11 +54,11 @@
> > > #define OV9282_AEC_MANUAL_DEFAULT 0x00
> > >
> > > /* Analog gain control */
> > > -#define OV9282_REG_AGAIN 0x3509
> > > -#define OV9282_AGAIN_MIN 0x10
> > > -#define OV9282_AGAIN_MAX 0xff
> > > -#define OV9282_AGAIN_STEP 1
> > > -#define OV9282_AGAIN_DEFAULT 0x10
> > > +#define OV9282_REG_AGAIN 0x3508
> > > +#define OV9282_AGAIN_MIN 0x0010
> > > +#define OV9282_AGAIN_MAX 0x1fff
> > > +#define OV9282_AGAIN_STEP 0x0001
> > > +#define OV9282_AGAIN_DEFAULT 0x0010
> > >
> > > /* Group hold register */
> > > #define OV9282_REG_HOLD 0x3308
> > > @@ -226,7 +226,6 @@ static const struct ov9282_reg common_regs[] = {
> > > {OV9282_REG_AEC_MANUAL, OV9282_GAIN_PREC16_EN},
> > > {0x3505, 0x8c},
> > > {0x3507, 0x03},
> > > - {0x3508, 0x00},
> > > {0x3610, 0x80},
> > > {0x3611, 0xa0},
> > > {0x3620, 0x6e},
> > > @@ -605,7 +604,11 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
> > > if (ret)
> > > goto error_release_group_hold;
> > >
> > > - ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, gain);
> > > + ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, (gain >> 8) & 0x1f);
> > > + if (ret)
> > > + goto error_release_group_hold;
> > > +
> > > + ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN + 1, 1, gain & 0xff);
> > >
> > > error_release_group_hold:
> > > ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 0);
> > >
> > > --
> > > 2.47.2
> > >
> > >
Powered by blists - more mailing lists