From: "Gjorgji Rosikopulos (Consultant)" <quic_grosikop@quicinc.com>
To: Kieran Bingham <kieran.bingham@ideasonboard.com>,
Bryan O'Donoghue <bryan.odonoghue@linaro.org>,
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: Thu, 9 May 2024 10:47:32 +0300 [thread overview]
Message-ID: <2a244afe-db2a-4b4a-a770-5664c887c8ef@quicinc.com> (raw)
In-Reply-To: <171518540550.191612.743149233311332771@ping.linuxembedded.co.uk>
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 <jacopo.mondi@ideasonboard.com>
>>>>
>>>> Thanks
>>>> j
>>>>
>>>
>>> If that's good enough I'll fix the typo and apply your RB.
>>
>> Sure
>>
>> Thanks
>> j
>>
>>>
>>> ---
>>> bod
>>>
>
next prev parent reply other threads:[~2024-05-09 7:47 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
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) [this message]
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=2a244afe-db2a-4b4a-a770-5664c887c8ef@quicinc.com \
--to=quic_grosikop@quicinc.com \
--cc=bryan.odonoghue@linaro.org \
--cc=daniele.alessandrelli@intel.com \
--cc=jacopo.mondi@ideasonboard.com \
--cc=kieran.bingham@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).