From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
To: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
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 10:02:03 +0200 [thread overview]
Message-ID: <dvyed4grpazqk7a3tz6dqwpkd76ghtrt4euinxt3kycdeh63ez@ljgfjsfhypix> (raw)
In-Reply-To: <20240506-b4-linux-next-camss-x13s-mmsol-integration-in-test-imx577-fix-v2-1-2e665f072f8f@linaro.org>
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 <bryan.odonoghue@linaro.org> # qrb5165-rb5/imx577
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
> 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-camss-x13s-mmsol-integration-in-test-imx577-fix-v1-1-4b3a9426bde8@linaro.org
> ---
> 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 <jacopo.mondi@ideasonboard.com>
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 <bryan.odonoghue@linaro.org>
>
>
next prev parent reply other threads:[~2024-05-08 8:02 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 [this message]
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)
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=dvyed4grpazqk7a3tz6dqwpkd76ghtrt4euinxt3kycdeh63ez@ljgfjsfhypix \
--to=jacopo.mondi@ideasonboard.com \
--cc=bryan.odonoghue@linaro.org \
--cc=daniele.alessandrelli@intel.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).