From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
To: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
"Paul J. Murphy" <paul.j.murphy@intel.com>,
Martina Krasteva <quic_mkrastev@quicinc.com>,
Daniele Alessandrelli <daniele.alessandrelli@intel.com>,
Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] media: i2c: Fix imx412 exposure control
Date: Wed, 8 May 2024 13:30:31 +0100 [thread overview]
Message-ID: <20a0300a-ac16-456c-840a-e272f49050a8@linaro.org> (raw)
In-Reply-To: <dvyed4grpazqk7a3tz6dqwpkd76ghtrt4euinxt3kycdeh63ez@ljgfjsfhypix>
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
next prev parent reply other threads:[~2024-05-08 12:30 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-06 22:38 [PATCH v2] media: i2c: Fix imx412 exposure control Bryan O'Donoghue
2024-05-08 8:02 ` Jacopo Mondi
2024-05-08 12:30 ` Bryan O'Donoghue [this message]
2024-05-08 12:43 ` Jacopo Mondi
2024-05-08 16:02 ` Gjorgji Rosikopulos (Consultant)
2024-05-09 8:01 ` Jacopo Mondi
2024-05-08 16:23 ` Kieran Bingham
2024-05-09 7:47 ` Gjorgji Rosikopulos (Consultant)
2024-05-08 16:31 ` Dave Stevenson
2024-05-09 0:32 ` Bryan O'Donoghue
2024-05-09 8:02 ` Gjorgji Rosikopulos (Consultant)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20a0300a-ac16-456c-840a-e272f49050a8@linaro.org \
--to=bryan.odonoghue@linaro.org \
--cc=daniele.alessandrelli@intel.com \
--cc=jacopo.mondi@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab+huawei@kernel.org \
--cc=mchehab@kernel.org \
--cc=paul.j.murphy@intel.com \
--cc=quic_mkrastev@quicinc.com \
--cc=sakari.ailus@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).