[v2] media: i2c: Fix imx412 exposure control

Commit Message

Bryan O'Donoghue May 6, 2024, 10:38 p.m. UTC

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.

Take the example of the above cited sensor drivers and directly apply the
requested exposure value from user-space. The 'lpfr' variable still
functions as before but the 'shutter' variable can be dispensed with as a
result.

Once done a similar run of the test application requesting higher exposure
looks like this, with 'exp' written directly to the sensor.

[  133.207884] imx412 20-001a: Received exp 1608, analog gain 0
[  133.207899] imx412 20-001a: Set exp 1608, analog gain 0, lpfr 3546
[  133.905309] imx412 20-001a: Received exp 2844, analog gain 100
[  133.905344] imx412 20-001a: Set exp 2844, analog gain 100, lpfr 3546
[  134.241705] imx412 20-001a: Received exp 3524, analog gain 110
[  134.241775] imx412 20-001a: Set exp 3524, analog gain 110, lpfr 3546

The result is then setting the sensor exposure to lower values results in
darker, less exposure images and vice versa with higher exposure values.

Fixes: 9214e86c0cc1 ("media: i2c: Add imx412 camera sensor driver")
Tested-by: Bryan O'Donoghue <[e-mail]> # qrb5165-rb5/imx577
Signed-off-by: Bryan O'Donoghue <[e-mail]>
---
Using libcamera/SoftISP on a Qualcomm RB5 with the imx577 sensor I found
that unlike on other platforms such as the Lenovo x13s/ov5675 the image was
constantly getting darker and darker.

At first I assumed a bug in SoftISP but, looking into the code it appeared
SoftISP was requesting higher and higher exposure values which resulted in
the image getting progressively darker.

To my mind the software contract between user-space and kernel should be
increasing exposure requests always meant higher exposure but, to be
certain I asked around on IRC.

Those polled agreed in principle that the software contract was consistent
across sensors.

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.

As a first pass I'm submitting the fix for the sensor I have but, I expect
if this fix is acceptable upstream it should be pushed to most of the imx
sensors with what seems to be copy/paste code for the exposure.
---
Changes in v2:
- Fix typo in patch 42 -> 412
- Link to v1: https://lore.kernel.org/r/20240506-b4-linux-next-c[e-mail]
---
 drivers/media/i2c/imx412.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)


---
base-commit: ff3959189f1b97e99497183d76ab9b007bec4c88
change-id: 20240506-b4-linux-next-camss-x13s-mmsol-integration-in-test-imx577-fix-f1fee6070641

Best regards,

Comments

Jacopo Mondi May 8, 2024, 8:02 a.m. UTC | #1

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 ?

>
> Take the example of the above cited sensor drivers and directly apply the
> requested exposure value from user-space. The 'lpfr' variable still
> functions as before but the 'shutter' variable can be dispensed with as a
> result.
>
> Once done a similar run of the test application requesting higher exposure
> looks like this, with 'exp' written directly to the sensor.
>
> [  133.207884] imx412 20-001a: Received exp 1608, analog gain 0
> [  133.207899] imx412 20-001a: Set exp 1608, analog gain 0, lpfr 3546
> [  133.905309] imx412 20-001a: Received exp 2844, analog gain 100
> [  133.905344] imx412 20-001a: Set exp 2844, analog gain 100, lpfr 3546
> [  134.241705] imx412 20-001a: Received exp 3524, analog gain 110
> [  134.241775] imx412 20-001a: Set exp 3524, analog gain 110, lpfr 3546
>
> The result is then setting the sensor exposure to lower values results in
> darker, less exposure images and vice versa with higher exposure values.
>
> Fixes: 9214e86c0cc1 ("media: i2c: Add imx412 camera sensor driver")
> Tested-by: Bryan O'Donoghue <[e-mail]> # qrb5165-rb5/imx577
> Signed-off-by: Bryan O'Donoghue <[e-mail]>
> ---
> Using libcamera/SoftISP on a Qualcomm RB5 with the imx577 sensor I found
> that unlike on other platforms such as the Lenovo x13s/ov5675 the image was
> constantly getting darker and darker.
>
> At first I assumed a bug in SoftISP but, looking into the code it appeared
> SoftISP was requesting higher and higher exposure values which resulted in
> the image getting progressively darker.
>
> To my mind the software contract between user-space and kernel should be
> increasing exposure requests always meant higher exposure but, to be
> certain I asked around on IRC.
>
> Those polled agreed in principle that the software contract was consistent
> across sensors.
>
> 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 ?

>
> As a first pass I'm submitting the fix for the sensor I have but, I expect
> if this fix is acceptable upstream it should be pushed to most of the imx
> sensors with what seems to be copy/paste code for the exposure.
> ---
> Changes in v2:
> - Fix typo in patch 42 -> 412
> - Link to v1: https://lore.kernel.org/r/20240506-b4-linux-next-c[e-mail]
> ---
>  drivers/media/i2c/imx412.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
> index 0efce329525e4..7d1f7af0a9dff 100644
> --- a/drivers/media/i2c/imx412.c
> +++ b/drivers/media/i2c/imx412.c
> @@ -542,14 +542,13 @@ static int imx412_update_controls(struct imx412 *imx412,
>   */
>  static int imx412_update_exp_gain(struct imx412 *imx412, u32 exposure, u32 gain)
>  {
> -	u32 lpfr, shutter;
> +	u32 lpfr;
>  	int ret;
>
>  	lpfr = imx412->vblank + imx412->cur_mode->height;
> -	shutter = lpfr - exposure;
>
> -	dev_dbg(imx412->dev, "Set exp %u, analog gain %u, shutter %u, lpfr %u",
> -		exposure, gain, shutter, lpfr);
> +	dev_dbg(imx412->dev, "Set exp %u, analog gain %u, lpfr %u",
> +		exposure, gain, lpfr);
>
>  	ret = imx412_write_reg(imx412, IMX412_REG_HOLD, 1, 1);
>  	if (ret)
> @@ -559,7 +558,7 @@ static int imx412_update_exp_gain(struct imx412 *imx412, u32 exposure, u32 gain)
>  	if (ret)
>  		goto error_release_group_hold;
>
> -	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 ?

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 <[e-mail]>

Thanks
  j

>  	if (ret)
>  		goto error_release_group_hold;
>
>
> ---
> base-commit: ff3959189f1b97e99497183d76ab9b007bec4c88
> change-id: 20240506-b4-linux-next-camss-x13s-mmsol-integration-in-test-imx577-fix-f1fee6070641
>
> Best regards,
> --
> Bryan O'Donoghue <[e-mail]>
>
>

Bryan O'Donoghue May 8, 2024, 12:30 p.m. UTC | #2

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 <[e-mail]>
> 
> Thanks
>    j
> 

If that's good enough I'll fix the typo and apply your RB.

---
bod

Jacopo Mondi May 8, 2024, 12:43 p.m. UTC | #3

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.

>
> > > -	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 <[e-mail]>
> >
> > Thanks
> >    j
> >
>
> If that's good enough I'll fix the typo and apply your RB.

Sure

Thanks
  j

>
> ---
> bod
>

Gjorgji Rosikopulos (Consultant) May 8, 2024, 4:02 p.m. UTC | #4

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 :-)


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.

However imx412 uses number of lines for exposure.

Reviewed-by: Gjorgji Rosikopulos <[e-mail]>

~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 <[e-mail]>
>>>
>>> Thanks
>>>    j
>>>
>>
>> If that's good enough I'll fix the typo and apply your RB.
> 
> Sure
> 
> Thanks
>   j
> 
>>
>> ---
>> bod
>>
>

Kieran Bingham May 8, 2024, 4:23 p.m. UTC | #5

Quoting Jacopo Mondi (2024-05-08 13:43:34)
> 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);

Is this a copy / paste error (IMX334), or are you using a downstream/alternative
driver?

Upstream implements this:

/**
 * imx335_update_exp_gain() - Set updated exposure and gain
 * @imx335: pointer to imx335 device
 * @exposure: updated exposure value
 * @gain: updated analog gain value
 *
 * Return: 0 if successful, error code otherwise.
 */
static int imx335_update_exp_gain(struct imx335 *imx335, u32 exposure, u32 gain)
{
	u32 lpfr, shutter;
	int ret;

	lpfr = imx335->vblank + imx335->cur_mode->height;
	shutter = lpfr - exposure;

	dev_dbg(imx335->dev, "Set exp %u, analog gain %u, shutter %u, lpfr %u\n",
		exposure, gain, shutter, lpfr);

	ret = imx335_write_reg(imx335, IMX335_REG_HOLD, 1, 1);
	if (ret)
		return ret;

	ret = imx335_write_reg(imx335, IMX335_REG_LPFR, 3, lpfr);
	if (ret)
		goto error_release_group_hold;

	ret = imx335_write_reg(imx335, IMX335_REG_SHUTTER, 3, shutter);
	if (ret)
		goto error_release_group_hold;

	ret = imx335_write_reg(imx335, IMX335_REG_AGAIN, 2, gain);

error_release_group_hold:
	imx335_write_reg(imx335, IMX335_REG_HOLD, 1, 0);

	return ret;
}

> >
> >
> > 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.

We're directly using the IMX335 driver in mainline on the i.MX8MP (and
also validated on Raspberry Pi 5). AGC is operational on both those
platforms with the sensor, so I have no reason to believe there is any
error in the upstream driver:

 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/i2c/imx335.c


--
Kieran


> >
> 
> Without datasheet/devices it really is hard to tell. Cargo cult at
> play most probably.
> 
> >
> > > > - 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 <[e-mail]>
> > >
> > > Thanks
> > >    j
> > >
> >
> > If that's good enough I'll fix the typo and apply your RB.
> 
> Sure
> 
> Thanks
>   j
> 
> >
> > ---
> > bod
> >

Dave Stevenson May 8, 2024, 4:31 p.m. UTC | #6

Hi Jacopo and Bryan

On Wed, 8 May 2024 at 13:43, Jacopo Mondi <[e-mail]> 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 <[e-mail]>
> > >
> > > Thanks
> > >    j
> > >
> >
> > If that's good enough I'll fix the typo and apply your RB.
>
> Sure
>
> Thanks
>   j
>
> >
> > ---
> > bod
> >
>

Bryan O'Donoghue May 9, 2024, 12:32 a.m. UTC | #7

On 08/05/2024 17:31, Dave Stevenson wrote:
> 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.

ack

> 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!

Understood. For the record the rpi imx477 driver writes the CID value 
provided by user-space.

https://github.com/raspberrypi/linux/blob/rpi-6.6.y/drivers/media/i2c/imx477.c#L1376

With an exposure multiplier we don't support upstream at the moment. So, 
I think this patch is the right fix after all.

---
bod

Gjorgji Rosikopulos (Consultant) May 9, 2024, 7:47 a.m. UTC | #8

Hi Kieran,

On 5/8/2024 7:23 PM, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2024-05-08 13:43:34)
>> 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);
> 
> Is this a copy / paste error (IMX334), or are you using a downstream/alternative
> driver?

Those drivers was posted as part of intel keembay project upstream effort.

The drivers where verified but they had some rework during the internal
review process. And it seems there was copy/paste error on imx412 (which
i also missed during the review).

To remove the confusion. there are no issues with imx334 and imx335,
those sensors are using shutter for setting exposure time.

With this change imx412 is also working fine it was verified on our side.

> 
> Upstream implements this:
> 
> /**
>  * imx335_update_exp_gain() - Set updated exposure and gain
>  * @imx335: pointer to imx335 device
>  * @exposure: updated exposure value
>  * @gain: updated analog gain value
>  *
>  * Return: 0 if successful, error code otherwise.
>  */
> static int imx335_update_exp_gain(struct imx335 *imx335, u32 exposure, u32 gain)
> {
> 	u32 lpfr, shutter;
> 	int ret;
> 
> 	lpfr = imx335->vblank + imx335->cur_mode->height;
> 	shutter = lpfr - exposure;
> 
> 	dev_dbg(imx335->dev, "Set exp %u, analog gain %u, shutter %u, lpfr %u\n",
> 		exposure, gain, shutter, lpfr);
> 
> 	ret = imx335_write_reg(imx335, IMX335_REG_HOLD, 1, 1);
> 	if (ret)
> 		return ret;
> 
> 	ret = imx335_write_reg(imx335, IMX335_REG_LPFR, 3, lpfr);
> 	if (ret)
> 		goto error_release_group_hold;
> 
> 	ret = imx335_write_reg(imx335, IMX335_REG_SHUTTER, 3, shutter);
> 	if (ret)
> 		goto error_release_group_hold;
> 
> 	ret = imx335_write_reg(imx335, IMX335_REG_AGAIN, 2, gain);
> 
> error_release_group_hold:
> 	imx335_write_reg(imx335, IMX335_REG_HOLD, 1, 0);
> 
> 	return ret;
> }
> 
>>>
>>>
>>> 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.
> 
> We're directly using the IMX335 driver in mainline on the i.MX8MP (and
> also validated on Raspberry Pi 5). AGC is operational on both those
> platforms with the sensor, so I have no reason to believe there is any
> error in the upstream driver:
> 
>  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/i2c/imx335.c

That is correct, there are no issues with imx334 and imx335.

~Gjorgji

> 
> 
> --
> Kieran
> 
> 
>>>
>>
>> Without datasheet/devices it really is hard to tell. Cargo cult at
>> play most probably.
>>
>>>
>>>>> - 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 <[e-mail]>
>>>>
>>>> Thanks
>>>>    j
>>>>
>>>
>>> If that's good enough I'll fix the typo and apply your RB.
>>
>> Sure
>>
>> Thanks
>>   j
>>
>>>
>>> ---
>>> bod
>>>
>

Jacopo Mondi May 9, 2024, 8:01 a.m. UTC | #9

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 <[e-mail]>
>
> ~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 <[e-mail]>
> >>>
> >>> Thanks
> >>>    j
> >>>
> >>
> >> If that's good enough I'll fix the typo and apply your RB.
> >
> > Sure
> >
> > Thanks
> >   j
> >
> >>
> >> ---
> >> bod
> >>
> >

Gjorgji Rosikopulos (Consultant) May 9, 2024, 8:02 a.m. UTC | #10

Hi Dave,

On 5/8/2024 7:31 PM, Dave Stevenson wrote:
> Hi Jacopo and Bryan
> 
> On Wed, 8 May 2024 at 13:43, Jacopo Mondi <[e-mail]> 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!

Yes we are observing the same, the sensors which are not for mobile
market (and not have anything to do with smia leftover). The exposure
is set using the similar or the same calculation which you have posted.

~Gjorgji

> 
>   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 <[e-mail]>
>>>>
>>>> Thanks
>>>>    j
>>>>
>>>
>>> If that's good enough I'll fix the typo and apply your RB.
>>
>> Sure
>>
>> Thanks
>>   j
>>
>>>
>>> ---
>>> bod
>>>
>>
>

Patch

diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
index 0efce329525e4..7d1f7af0a9dff 100644
--- a/drivers/media/i2c/imx412.c
+++ b/drivers/media/i2c/imx412.c
@@ -542,14 +542,13 @@  static int imx412_update_controls(struct imx412 *imx412,
  */
 static int imx412_update_exp_gain(struct imx412 *imx412, u32 exposure, u32 gain)
 {
-	u32 lpfr, shutter;
+	u32 lpfr;
 	int ret;
 
 	lpfr = imx412->vblank + imx412->cur_mode->height;
-	shutter = lpfr - exposure;
 
-	dev_dbg(imx412->dev, "Set exp %u, analog gain %u, shutter %u, lpfr %u",
-		exposure, gain, shutter, lpfr);
+	dev_dbg(imx412->dev, "Set exp %u, analog gain %u, lpfr %u",
+		exposure, gain, lpfr);
 
 	ret = imx412_write_reg(imx412, IMX412_REG_HOLD, 1, 1);
 	if (ret)
@@ -559,7 +558,7 @@  static int imx412_update_exp_gain(struct imx412 *imx412, u32 exposure, u32 gain)
 	if (ret)
 		goto error_release_group_hold;
 
-	ret = imx412_write_reg(imx412, IMX412_REG_EXPOSURE_CIT, 2, shutter);
+	ret = imx412_write_reg(imx412, IMX412_REG_EXPOSURE_CIT, 2, exposure);
 	if (ret)
 		goto error_release_group_hold;