| Message ID | 20250225-b4-ov9282-gain-v1-0-a24af2820dde@linux.dev |
|---|---|
| Headers | show |
| Series | Fix analogue gain maximum for ov9282 | expand |
Hi Richard Thanks for the patch On Tue, 25 Feb 2025 at 13:09, Richard Leitner <richard.leitner@linux.dev> wrote: > > Add #define's for the "AEC MANUAL" (0x3503) register and its > values/flags. Use those in the registers single usage within the > `common_regs` struct. > > All values are based on the OV9281 datasheet v1.01 (09.18.2015). > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > drivers/media/i2c/ov9282.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c > index c926842257893c4da3319b847fab9908b5bdaec6..c882a021cf18852237bf9b9524d3de0c5b48cbcb 100644 > --- a/drivers/media/i2c/ov9282.c > +++ b/drivers/media/i2c/ov9282.c > @@ -44,6 +44,15 @@ > #define OV9282_EXPOSURE_STEP 1 > #define OV9282_EXPOSURE_DEFAULT 0x0282 > > +/* AEC/AGC manual */ > +#define OV9282_REG_AEC_MANUAL 0x3503 > +#define OV9282_DIGFRAC_GAIN_DELAY BIT(6) > +#define OV9282_GAIN_CHANGE_DELAY BIT(5) > +#define OV9282_GAIN_DELAY BIT(4) > +#define OV9282_GAIN_PREC16_EN BIT(3) > +#define OV9282_GAIN_MANUAL_AS_SENSGAIN BIT(2) > +#define OV9282_AEC_MANUAL_DEFAULT 0x00 > + > /* Analog gain control */ > #define OV9282_REG_AGAIN 0x3509 > #define OV9282_AGAIN_MIN 0x10 > @@ -214,7 +223,7 @@ static const struct ov9282_reg common_regs[] = { > {0x3030, 0x10}, > {0x3039, 0x32}, > {0x303a, 0x00}, > - {0x3503, 0x08}, > + {OV9282_REG_AEC_MANUAL, OV9282_GAIN_PREC16_EN}, > {0x3505, 0x8c}, > {0x3507, 0x03}, > {0x3508, 0x00}, > > -- > 2.47.2 > >
Hi Richard On Tue, 25 Feb 2025 at 13:09, Richard Leitner <richard.leitner@linux.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@linux.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 > >
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@linux.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. 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? regards;rl > > Dave > > > Signed-off-by: Richard Leitner <richard.leitner@linux.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 > > > >
On Wed, 26 Feb 2025 at 11:07, Richard Leitner <richard.leitner@linux.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@linux.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@linux.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 > > > > > >
Hi Dave, Richard, On Wed, Feb 26, 2025 at 12:52:46PM +0000, Dave Stevenson wrote: > > 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. I can pick the two first patches of the set now.
Hi Sakari, On Wed, Feb 26, 2025 at 01:01:05PM +0000, Sakari Ailus wrote: > Hi Dave, Richard, > > On Wed, Feb 26, 2025 at 12:52:46PM +0000, Dave Stevenson wrote: > > > 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. > > I can pick the two first patches of the set now. Thanks. That would be great! regards;rl > > -- > Regards, > > Sakari Ailus
This series fixes/increases the analogue gain maximum value of the ov9282 sensor driver to the limits specified in the datasheet. It furthermore introduces a new register definition (including its values) and uses available defined registers instead of addresses where possible. This is basically a first improvement/cleanup series for the ov9282 driver with likely more to follow. All register addresses/values are based on the OV9281 datasheet v1.01 (09.18.2015). Signed-off-by: Richard Leitner <richard.leitner@linux.dev> --- Richard Leitner (3): media: i2c: ov9282: use register definitions media: i2c: ov9282: add AEC Manual register definition media: i2c: ov9282: fix analogue gain maximum drivers/media/i2c/ov9282.c | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-) --- base-commit: d082ecbc71e9e0bf49883ee4afd435a77a5101b6 change-id: 20250225-b4-ov9282-gain-ef1cdaba5bfd Best regards,