Hi Jacopo and Bryan On Wed, 8 May 2024 at 13:43, Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> wrote: > > Hi Bryan > > On Wed, May 08, 2024 at 01:30:31PM GMT, Bryan O'Donoghue wrote: > > 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. > > > > Sounds familiar enough > > > > > > > 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. > > > > Without datasheet/devices it really is hard to tell. Cargo cult at > play most probably. For reference certainly imx327/290/462 which are all siblings in the Sony Starvis family do calculate exposure as exposure = 1 frame period - (SHS1 + 1) * (1H period) So 0 = max exposure and bigger values are shorter exposure time. I'm not saying that the imx412 driver is right in doing this as well, but it seems there is a trend with the Sony Starvis family to program exposure in this different manner. Don't discount it as wrong for all drivers! Dave > > > > > > - 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@xxxxxxxxxxxxxxxx> > > > > > > Thanks > > > j > > > > > > > If that's good enough I'll fix the typo and apply your RB. > > Sure > > Thanks > j > > > > > --- > > bod > > >
