Hi Gjorgji ! On Wed, May 08, 2024 at 07:02:37PM GMT, Gjorgji Rosikopulos (Consultant) wrote: > Hi Bryan, Jacopo, > > On 5/8/2024 3:43 PM, Jacopo Mondi 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. > > I have explained in previous email. But i will post here as well :-) > Thanks, I have probably missed it! > > As far as i know this issue is only for this imx412 sensor driver. > The sensor driver for imx412 was submitted along with imx335 and imx334, > maybe after all the reworking this was missed. > imx334 and imx335 are using shutter for setting the exposure from where > this calculation is taken. Thanks for clarifying and confirming the other drivers are correct. > > However imx412 uses number of lines for exposure. > > Reviewed-by: Gjorgji Rosikopulos <quic_grosikop@xxxxxxxxxxx> > > ~Gjorgji > > > > >> > >>>> - 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 > >> > >
