Message-ID: <qv5vzwgg4mpdo3yy73hrykmuum3rlkleixrgg5zc5n2kjj3wl3@wmbx34evysiy>
Date: Thu, 9 May 2024 10:01:37 +0200
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: "Gjorgji Rosikopulos (Consultant)" <quic_grosikop@...cinc.com>
Cc: Jacopo Mondi <jacopo.mondi@...asonboard.com>, 
	Bryan O'Donoghue <bryan.odonoghue@...aro.org>, Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, "Paul J. Murphy" <paul.j.murphy@...el.com>, 
	Martina Krasteva <quic_mkrastev@...cinc.com>, Daniele Alessandrelli <daniele.alessandrelli@...el.com>, 
	Mauro Carvalho Chehab <mchehab+huawei@...nel.org>, linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] media: i2c: Fix imx412 exposure control

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@...cinc.com>
>
> ~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@...asonboard.com>
> >>>
> >>> Thanks
> >>>    j
> >>>
> >>
> >> If that's good enough I'll fix the typo and apply your RB.
> >
> > Sure
> >
> > Thanks
> >   j
> >
> >>
> >> ---
> >> bod
> >>
> >

Powered by blists - more mailing lists