linux-kernel - Re: [PATCH 3/3] media: i2c: ov9282: fix analogue gain maximum

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPY8ntCLUVX91+QYbFk-bHQumuvs70CuDLe85VZRTS2wW-tYHw@mail.gmail.com>
Date: Tue, 25 Feb 2025 15:30:16 +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

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.

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.

  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
>
>