Messages in this thread | | | Date | Wed, 8 May 2024 13:30:31 +0100 | Subject | Re: [PATCH v2] media: i2c: Fix imx412 exposure control | From | Bryan O'Donoghue <> |
| |
On 08/05/2024 09:02, Jacopo Mondi wrote: > Hi Bryan > > On Mon, May 06, 2024 at 11:38:26PM GMT, Bryan O'Donoghue wrote: >> Currently we have the following algorithm to calculate what value should be >> written to the exposure control of imx412. >> >> lpfr = imx412->vblank + imx412->cur_mode->height; >> shutter = lpfr - exposure; >> >> The 'shutter' value is given to IMX412_REG_EXPOSURE_CIT however, the above >> algorithm will result in the value given to IMX412_REG_EXPOSURE_CIT >> decreasing as the requested exposure value from user-space goes up. >> >> e.g. >> [ 2255.713989] imx412 20-001a: Received exp 1608, analog gain 0 >> [ 2255.714002] imx412 20-001a: Set exp 1608, analog gain 0, shutter 1938, lpfr 3546 >> [ 2256.302770] imx412 20-001a: Received exp 2586, analog gain 100 >> [ 2256.302800] imx412 20-001a: Set exp 2586, analog gain 100, shutter 960, lpfr 3546 >> [ 2256.753755] imx412 20-001a: Received exp 3524, analog gain 110 >> [ 2256.753772] imx412 20-001a: Set exp 3524, analog gain 110, shutter 22, lpfr 3546 >> >> This behaviour results in the image having less exposure as the requested >> exposure value from user-space increases. >> >> Other sensor drivers such as ov5675, imx218, hid556 and others take the >> requested exposure value and directly. > > has the phrase been truncated or is it me reading it wrong ?
Sod's law says no matter how many times you send yourself a patch before sending it to LKML you'll find a typo ~ 2 seconds after reading your patch on LKML.
>> Looking at the range of imx sensors, it appears this particular error has >> been replicated a number of times but, I haven't so far really drilled into >> each sensor. > > Ouch, what other driver have the same issue ?
So without data sheet or sensor its hard to say if these are correct or incorrect, it's the same basic calculation though.
drivers/media/i2c/imx334.c::imx334_update_exp_gain()
lpfr = imx334->vblank + imx334->cur_mode->height; shutter = lpfr - exposure;
ret = imx334_write_reg(imx334, IMX334_REG_SHUTTER, 3, shutter);
drivers/media/i2c/imx335.c::imx335_update_exp_gain()
lpfr = imx335->vblank + imx335->cur_mode->height; shutter = lpfr - exposure;
ret = imx335_write_reg(imx335, IMX334_REG_SHUTTER, 3, shutter);
Looking again I'm inclined to believe the imx334/imx335 stuff is probably correct for those sensors, got copied to imx412/imx577 and misapplied to the EXPOSURE control in imx412.
>> - ret = imx412_write_reg(imx412, IMX412_REG_EXPOSURE_CIT, 2, shutter); >> + ret = imx412_write_reg(imx412, IMX412_REG_EXPOSURE_CIT, 2, exposure); > > No datasheet here, can you confirm the IMX412_REG_EXPOSURE_CIT > register is actually in lines ?
Looks like.
From downstream "coarseIntgTimeAddr"
imx577_sensor.xml <coarseIntgTimeAddr>0x0202</coarseIntgTimeAddr>
imx586/imx586_sensor.cpp pRegSettingsInfo->regSetting[regCount].registerAddr = pExposureData->pRegInfo->coarseIntgTimeAddr + 1;
pRegSettingsInfo->regSetting[regCount].registerData = (lineCount & 0xFF);
> Apart from that, as the CID_EXPOSURE control limit are correctly > updated when a new VBLANK is set by taking into account the exposure > margins, I think writing the control value to the register is the > right thing to do (if the register is in lines of course) > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Thanks > j >
If that's good enough I'll fix the typo and apply your RB.
--- bod
| |