From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
To: 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>
Cc: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Subject: [PATCH v2] media: i2c: Fix imx412 exposure control
Date: Mon, 06 May 2024 23:38:26 +0100 [thread overview]
Message-ID: <20240506-b4-linux-next-camss-x13s-mmsol-integration-in-test-imx577-fix-v2-1-2e665f072f8f@linaro.org> (raw)
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 <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.
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);
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 reply other threads:[~2024-05-06 22:38 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-06 22:38 Bryan O'Donoghue [this message]
2024-05-08 8:02 ` [PATCH v2] media: i2c: Fix imx412 exposure control 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)
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=20240506-b4-linux-next-camss-x13s-mmsol-integration-in-test-imx577-fix-v2-1-2e665f072f8f@linaro.org \
--to=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).