[v2] libipa: camera_sensor_helper: Add imx471
diff mbox series

Message ID CALr1FG_KCY4FY0mkt==uD87VjKqRRWDec=vjgmf7wigurYZJYQ@mail.gmail.com
State Superseded
Headers show
Series
  • [v2] libipa: camera_sensor_helper: Add imx471
Related show

Commit Message

John Cronin July 29, 2026, 2:43 p.m. UTC 
Add CameraSensorHelper for Sony IMX471 (SONY471A), used on Lenovo
ThinkPad X9-15 and related IPU7 platforms.

The kernel imx471 driver programs analogue gain code 0..800 into
register 0x0204. Use the common Sony linear model G = 1024/(1024-code)
(matching imx355/imx477/imx708) and a 10-bit pedestal of 64 (4096 at
16-bit).

Also register static sensor properties so soft-ISP AGC no longer warns
about missing delays/properties for this model.

Tested on Fedora 44 / ThinkPad X9-15 Gen 1 (kernel 7.1.5, imx471 bound
via IPU7 ISYS). Soft-ISP loads without "Failed to create camera sensor
helper for imx471"; AGC reports gain range 1.0-4.57x and adjusts
exposure under capture.

Signed-off-by: John Cronin <john.cronin@opcenter.com>
Tested-by: John Cronin <john.cronin@opcenter.com>
---
Changes in v2:
- Fix mangled diff --git b/ path from the previous mail (copy error in
MUA send).
  ---/+++ paths and hunk contents were already correct; pure
apply-ready form now.

 src/ipa/libipa/camera_sensor_helper.cpp       | 24 +++++++++++++++++++
 .../sensor/camera_sensor_properties.cpp       | 17 +++++++++++++
 2 files changed, 41 insertions(+)

 			.testPatternModes = {},

Comments

John Cronin July 29, 2026, 2:58 p.m. UTC | #1 
Hi,

For context on the Fedora / IPU7 side, users on Lenovo ThinkPad X9 / Lunar
Lake (SONY471A) are hitting the same soft-ISP gaps this patch addresses
(missing CameraSensorHelper, no static properties, dark/greenish images
under uncalibrated.yaml). See for example:

  https://bugzilla.redhat.com/show_bug.cgi?id=2454119

(comment 11 logs "Failed to create camera sensor helper for imx471" and
the sensor-properties warnings on Fedora 44 + libcamera 0.7.1.)

Happy to adjust the helper if review wants different black level, delays,
or gain-model notes.

Thanks,
John Cronin
Jacopo Mondi July 31, 2026, 9:39 a.m. UTC | #2 
Hi John

FIY, don't send new versions of a series in reply to the previous one.

Separate thread are preferred and help with maintainership.
Not a big deal though

On Wed, Jul 29, 2026 at 10:43:32AM -0400, John Cronin wrote:
> Add CameraSensorHelper for Sony IMX471 (SONY471A), used on Lenovo
> ThinkPad X9-15 and related IPU7 platforms.
>
> The kernel imx471 driver programs analogue gain code 0..800 into
> register 0x0204. Use the common Sony linear model G = 1024/(1024-code)

How did you get to 1024 ? Is this from documentation ?

I read in a prior cover letter that
"AGC range reported as gain 1.0-4.57x"

I can confirm that (if 800 is in decimal) 1024/(1024-800)=4.57
but as 4.57 seems a little weird as max gain value (not saying it's
impossible ofc) let's try to figure out where you got the max gain
information from.

> (matching imx355/imx477/imx708) and a 10-bit pedestal of 64 (4096 at
> 16-bit).

Same question for the black level pedestal.

Does this come from documentation ? Has it been experimentally
validated ?

>
> Also register static sensor properties so soft-ISP AGC no longer warns
> about missing delays/properties for this model.
>
> Tested on Fedora 44 / ThinkPad X9-15 Gen 1 (kernel 7.1.5, imx471 bound
> via IPU7 ISYS). Soft-ISP loads without "Failed to create camera sensor
> helper for imx471"; AGC reports gain range 1.0-4.57x and adjusts
> exposure under capture.
>
> Signed-off-by: John Cronin <john.cronin@opcenter.com>
> Tested-by: John Cronin <john.cronin@opcenter.com>

Well, we hope everybody tests the patches they send out :)

No need for this tag on your own patches

> ---
> Changes in v2:
> - Fix mangled diff --git b/ path from the previous mail (copy error in
> MUA send).
>   ---/+++ paths and hunk contents were already correct; pure
> apply-ready form now.
>
>  src/ipa/libipa/camera_sensor_helper.cpp       | 24 +++++++++++++++++++
>  .../sensor/camera_sensor_properties.cpp       | 17 +++++++++++++

two patches for these two separate changes might be better.
Not a big deal

>  2 files changed, 41 insertions(+)
>
> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp
> b/src/ipa/libipa/camera_sensor_helper.cpp
> index cc16e25..5fb545a 100644
> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -644,6 +644,30 @@ class CameraSensorHelperImx462 : public
> CameraSensorHelperImx290
>  };
>  REGISTER_CAMERA_SENSOR_HELPER("imx462", CameraSensorHelperImx462)
>
> +class CameraSensorHelperImx471 : public CameraSensorHelper
> +{
> +public:
> +	CameraSensorHelperImx471()
> +	{
> +		/*
> +		 * Sony IMX471 (IPU7 / ThinkPad X9, X1 Carbon Gen14).
> +		 *
> +		 * Analogue gain is programmed via V4L2_CID_ANALOGUE_GAIN to
> +		 * register 0x0204. The kernel driver (imx471) exposes range
> +		 * 0..800 and writes the code directly.
> +		 *
> +		 * Sony linear gain model used by several IMX sensors:
> +		 *   G = 1024 / (1024 - code)
> +		 * so code 0 → 1.0x and code 800 → ~4.57x.
> +		 *
> +		 * 10-bit optical black pedestal is typically 64 → 4096 at 16-bit.
> +		 */
> +		blackLevel_ = 4096;
> +		gain_ = AnalogueGainLinear{ 0, 1024, -1, 1024 };
> +	}
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("imx471", CameraSensorHelperImx471)
> +
>  class CameraSensorHelperImx477 : public CameraSensorHelper
>  {
>  public:
> diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp
> b/src/libcamera/sensor/camera_sensor_properties.cpp
> index fc9e184..7ba55dc 100644
> --- a/src/libcamera/sensor/camera_sensor_properties.cpp
> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
> @@ -297,6 +297,23 @@ const CameraSensorProperties
> *CameraSensorProperties::get(const std::string &sen
>  				.hblankDelay = 2
>  			},
>  		} },
> +		{ "imx471", {
> +			/* 1/2.3-type; ~1.4 µm pixels (nm). Delays unverified. */

If delays are not verified, do we want them here or should we keep
them not populated and let libcamera use the defaults (see
the implementation of CameraSensorLegacy::sensorDelays())

> +			.unitCellSize = { 1400, 1400 },
> +			.testPatternModes = {
> +				{ controls::draft::TestPatternModeOff, 0 },
> +				{ controls::draft::TestPatternModeSolidColor, 1 },
> +				{ controls::draft::TestPatternModeColorBars, 2 },
> +				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
> +				{ controls::draft::TestPatternModePn9, 4 },

Test patterns matche the ones reported by the driver!

> +			},
> +			.sensorDelays = {
> +				.exposureDelay = 2,
> +				.gainDelay = 2,
> +				.vblankDelay = 2,
> +				.hblankDelay = 2
> +			},
> +		} },

Thanks
  j

>  		{ "imx477", {
>  			.unitCellSize = { 1550, 1550 },
>  			.testPatternModes = {},
> --
> 2.55.0
John Cronin July 31, 2026, 2:33 p.m. UTC | #3 
Hi Jacopo,

Thanks for the review.

On Fri, Jul 31, 2026 at 11:39:49AM +0200, Jacopo Mondi wrote:
> FIY, don't send new versions of a series in reply to the previous one.
> Separate thread are preferred and help with maintainership.

Noted — v3 is sent as a new thread (cover letter + 2 patches).

> How did you get to 1024 ? Is this from documentation ?
> ... let's try to figure out where you got the max gain information from.

The kernel imx471 driver (drivers/media/i2c/imx471.c) defines:

  IMX471_REG_ANALOG_GAIN  CCI_REG16(0x0204)
  IMX471_ANA_GAIN_MIN     0
  IMX471_ANA_GAIN_MAX     800

and writes V4L2_CID_ANALOGUE_GAIN straight to that register — no
conversion and no documented gain formula in the driver.

I do not have a public IMX471 datasheet. The 1024 linear model is the
same G = 1024/(1024-code) already used for imx355/imx477/imx708 in
camera_sensor_helper.cpp (imx355 also uses that model with a datasheet
note). Under that model, code 800 → ~4.57x is just arithmetic on the
V4L2 max, not an independent "max gain from datasheet" claim. v3 makes
that explicit in the commit message and code comment.

If you prefer a different model once a datasheet is available, happy to
revise.

> Same question for the black level pedestal.
> Does this come from documentation ? Has it been experimentally validated?

Same situation: no public datasheet. 4096 is 64 DN at 10-bit scaled to
16-bit, matching the imx355 helper's pedestal convention. I have not
yet measured a covered-lens dark frame on this laptop (earlier captures
were processed ABGR, not raw Bayer). v3 states that clearly. I can
follow up with a measured blackLevel_ if/when I get a clean raw dark
capture.

> No need for this tag on your own patches

Dropped the self Tested-by in v3.

> two patches for these two separate changes might be better.

Done: helper + properties as 1/2 and 2/2.

> If delays are not verified, do we want them here or should we keep
> them not populated and let libcamera use the defaults

Agreed — v3 uses .sensorDelays = { } so defaults apply.

> Test patterns matche the ones reported by the driver!

Kept as-is; still aligned with imx471_test_pattern_menu[].

v3 series: "[PATCH v3 0/2] Add imx471 soft-ISP sensor support"

Thanks,
John Cronin
Jacopo Mondi July 31, 2026, 3:03 p.m. UTC | #4 
Hi John

On Fri, Jul 31, 2026 at 07:33:01AM -0700, John Cronin wrote:
> Hi Jacopo,
>
> Thanks for the review.
>
> On Fri, Jul 31, 2026 at 11:39:49AM +0200, Jacopo Mondi wrote:
> > FIY, don't send new versions of a series in reply to the previous one.
> > Separate thread are preferred and help with maintainership.
>
> Noted — v3 is sent as a new thread (cover letter + 2 patches).
>
> > How did you get to 1024 ? Is this from documentation ?
> > ... let's try to figure out where you got the max gain information from.
>
> The kernel imx471 driver (drivers/media/i2c/imx471.c) defines:
>
>   IMX471_REG_ANALOG_GAIN  CCI_REG16(0x0204)
>   IMX471_ANA_GAIN_MIN     0
>   IMX471_ANA_GAIN_MAX     800
>
> and writes V4L2_CID_ANALOGUE_GAIN straight to that register — no
> conversion and no documented gain formula in the driver.
>
> I do not have a public IMX471 datasheet. The 1024 linear model is the
> same G = 1024/(1024-code) already used for imx355/imx477/imx708 in
> camera_sensor_helper.cpp (imx355 also uses that model with a datasheet

imx355 has a max register value of 960 which gives you a max gain of
x16

same for imx708

imx477 has a max register value of 978 which gives a max analogue gain
of x22

> note). Under that model, code 800 → ~4.57x is just arithmetic on the
> V4L2 max, not an independent "max gain from datasheet" claim. v3 makes
> that explicit in the commit message and code comment.

Yeah, that's exactly my point, picking 1024 gives you a x4.57 as max gain
which seems unlikely low (although, it is certainly possible and I'm
wrong).

Ideally you should measure the actual image brightness increase in the
image by incrementing the gain values in the register manually.

camshark might help with this with a bit of experimentation. We can
certainly help if you are interested.

>
> If you prefer a different model once a datasheet is available, happy to
> revise.

I would prefer not to upstream an incorrect gain formula instead of
correcting it later on.

>
> > Same question for the black level pedestal.
> > Does this come from documentation ? Has it been experimentally validated?
>
> Same situation: no public datasheet. 4096 is 64 DN at 10-bit scaled to
> 16-bit, matching the imx355 helper's pedestal convention. I have not
> yet measured a covered-lens dark frame on this laptop (earlier captures
> were processed ABGR, not raw Bayer). v3 states that clearly. I can
> follow up with a measured blackLevel_ if/when I get a clean raw dark
> capture.

As per the gain model, that would be ideal.

>
> > No need for this tag on your own patches
>
> Dropped the self Tested-by in v3.
>
> > two patches for these two separate changes might be better.
>
> Done: helper + properties as 1/2 and 2/2.
>
> > If delays are not verified, do we want them here or should we keep
> > them not populated and let libcamera use the defaults
>
> Agreed — v3 uses .sensorDelays = { } so defaults apply.
>
> > Test patterns matche the ones reported by the driver!
>
> Kept as-is; still aligned with imx471_test_pattern_menu[].
>
> v3 series: "[PATCH v3 0/2] Add imx471 soft-ISP sensor support"

Thank for addressing my above comments.


>
> Thanks,
> John Cronin
John Cronin July 31, 2026, 3:14 p.m. UTC | #5 
Hi Jacopo,

On Fri, Jul 31, 2026 at 05:03:31PM +0200, Jacopo Mondi wrote:
> imx355 has a max register value of 960 which gives you a max gain of
> x16
> ...
> Yeah, that's exactly my point, picking 1024 gives you a x4.57 as max gain
> which seems unlikely low (although, it is certainly possible and I'm
> wrong).
>
> Ideally you should measure the actual image brightness increase in the
> image by incrementing the gain values in the register manually.

I measured on ThinkPad X9-15 Gen 1 (Fedora 44, kernel 7.1.5, imx471
via IPU7 ISYS).

Method:
- libcamera cam role=raw, 1928x1088 SRGGB10 (16-bit containers, stride
  3904)
- Lock V4L2 exposure=200, digital_gain=256; step analogue_gain codes
  0..800 on the imx471 subdev (driver max)
- Mean of active-area 10-bit samples; black-subtract using p1 at code 0
  (≈65 DN)

Results (ratio = signal(code)/signal(0) vs G=1024/(1024-code)):

  code   ratio_meas   ratio_pred   rel_err
     0      1.0000       1.0000     0.00%
    50      1.0522       1.0513    +0.08%
   100      1.1101       1.1082    +0.17%
   150      1.1753       1.1716    +0.31%
   200      1.2488       1.2427    +0.49%
   250      1.3280       1.3230    +0.38%
   300      1.4184       1.4144    +0.28%
   400      1.6503       1.6410    +0.57%
   500      1.9703       1.9542    +0.82%
   600      2.4363       2.4151    +0.88%
   700      3.2039       3.1605    +1.37%
   800      4.5692       4.5714    -0.05%

Best-fit k for G=k/(k-code) over the same points is k≈1023 (MSE≈2e-4),
i.e. the 1024 model.

So the low ~4.57x max is real under the kernel's code range 0..800: the
formula matches brightness, while other IMX helpers simply expose higher
max codes (960/978) under the same math.

Black level (min exposure=1, gain=0 raw; not a fully covered lens, but
essentially dark at this exposure):

  mean ≈ 64.1 DN, p50 = 64, p1 = 63  (10-bit)
  → blackLevel_ = 4096 at 16-bit

v4 updates the helper comment / commit message with this evidence
(same code values). Series: "[PATCH v4 0/2] Add imx471 soft-ISP sensor
support" as a new thread.

Happy to re-run with camshark or a covered-lens dark frame if you want
more rigor.

Thanks,
John Cronin
Jacopo Mondi July 31, 2026, 3:36 p.m. UTC | #6 
Hi John

On Fri, Jul 31, 2026 at 08:14:59AM -0700, John Cronin wrote:
> Hi Jacopo,
>
> On Fri, Jul 31, 2026 at 05:03:31PM +0200, Jacopo Mondi wrote:
> > imx355 has a max register value of 960 which gives you a max gain of
> > x16
> > ...
> > Yeah, that's exactly my point, picking 1024 gives you a x4.57 as max gain
> > which seems unlikely low (although, it is certainly possible and I'm
> > wrong).
> >
> > Ideally you should measure the actual image brightness increase in the
> > image by incrementing the gain values in the register manually.
>
> I measured on ThinkPad X9-15 Gen 1 (Fedora 44, kernel 7.1.5, imx471
> via IPU7 ISYS).

Ah thanks, that's great.

>
> Method:
> - libcamera cam role=raw, 1928x1088 SRGGB10 (16-bit containers, stride
>   3904)
> - Lock V4L2 exposure=200, digital_gain=256; step analogue_gain codes
>   0..800 on the imx471 subdev (driver max)
> - Mean of active-area 10-bit samples; black-subtract using p1 at code 0
>   (≈65 DN)

out of curiosity, what's "p1 at code 0" ?

>
> Results (ratio = signal(code)/signal(0) vs G=1024/(1024-code)):
>
>   code   ratio_meas   ratio_pred   rel_err
>      0      1.0000       1.0000     0.00%
>     50      1.0522       1.0513    +0.08%
>    100      1.1101       1.1082    +0.17%
>    150      1.1753       1.1716    +0.31%
>    200      1.2488       1.2427    +0.49%
>    250      1.3280       1.3230    +0.38%
>    300      1.4184       1.4144    +0.28%
>    400      1.6503       1.6410    +0.57%
>    500      1.9703       1.9542    +0.82%
>    600      2.4363       2.4151    +0.88%
>    700      3.2039       3.1605    +1.37%
>    800      4.5692       4.5714    -0.05%
>
> Best-fit k for G=k/(k-code) over the same points is k≈1023 (MSE≈2e-4),
> i.e. the 1024 model.

Nice!

If you feel like sharing your test/measurement setup, this might be
helpful for all the other users wanting to go through the same
procedure for their sensors!


>
> So the low ~4.57x max is real under the kernel's code range 0..800: the
> formula matches brightness, while other IMX helpers simply expose higher
> max codes (960/978) under the same math.
>
> Black level (min exposure=1, gain=0 raw; not a fully covered lens, but
> essentially dark at this exposure):
>
>   mean ≈ 64.1 DN, p50 = 64, p1 = 63  (10-bit)
>   → blackLevel_ = 4096 at 16-bit

I can't judge if reducing exposure/gain is equivalent to covering a
lens, but this seems accurate

>
> v4 updates the helper comment / commit message with this evidence
> (same code values). Series: "[PATCH v4 0/2] Add imx471 soft-ISP sensor
> support" as a new thread.

Thanks, I'll review it right away!

>
> Happy to re-run with camshark or a covered-lens dark frame if you want
> more rigor.
>
> Thanks,
> John Cronin
John Cronin July 31, 2026, 3:44 p.m. UTC | #7 
Hi Jacopo,

On Fri, Jul 31, 2026 at 05:36:17PM +0200, Jacopo Mondi wrote:
> out of curiosity, what's "p1 at code 0" ?

Sorry for the jargon — p1 is the 1st percentile of the 10-bit raw
sample distribution in the active area (sorted samples, value near
the bottom 1%). At analogue_gain code 0 I used that as a rough black
pedestal estimate for black-subtraction when computing
signal(code)/signal(0). For the dedicated black-level measurement I
also reported mean/p50 at min exposure (≈64 DN).

> If you feel like sharing your test/measurement setup, this might be
> helpful for all the other users wanting to go through the same
> procedure for their sensors!

Happy to share. Short procedure:

  1. Capture role=raw (libcamera cam), not processed RGB
  2. Lock V4L2 exposure + digital_gain on the sensor subdev
  3. Step V4L2_CID_ANALOGUE_GAIN across the driver range
  4. Mean of subsampled active-area 10-bit samples
  5. Black-subtract (p1 at min gain, or min-exposure / covered-lens)
  6. Fit ratio vs G = k/(k-code)

Script + sample CSV + write-up (imx471 / IPU7 defaults, adaptable):

  https://gist.github.com/johncronin3/15d9eacbf64f91ecc600fe583686325f

  python3 measure-imx471-gain.py --exposure 200 \\
    --gains 0,50,100,150,200,250,300,400,500,600,700,800 \\
    --out /tmp/imx471-gain-measure

Set LIBCAMERA_CAM if "cam" is not on PATH (or point at a local
libcamera install wrapper). Width/height/stride constants at the top
of the script need adjusting for other sensors/modes.

Happy to rework that into a Documentation/ note in-tree if that would
be more useful than a gist.

> Thanks, I'll review it right away!

Thanks — and thanks for the R-b on 2/2 (properties). Fine with dropping
the redundant test-pattern comment when you apply.

Happy to address anything on 1/2 (helper) once you've had a look.

Thanks,
John Cronin

Patch
diff mbox series 

diff --git a/src/ipa/libipa/camera_sensor_helper.cpp
b/src/ipa/libipa/camera_sensor_helper.cpp
index cc16e25..5fb545a 100644
--- a/src/ipa/libipa/camera_sensor_helper.cpp
+++ b/src/ipa/libipa/camera_sensor_helper.cpp
@@ -644,6 +644,30 @@  class CameraSensorHelperImx462 : public
CameraSensorHelperImx290
 };
 REGISTER_CAMERA_SENSOR_HELPER("imx462", CameraSensorHelperImx462)

+class CameraSensorHelperImx471 : public CameraSensorHelper
+{
+public:
+	CameraSensorHelperImx471()
+	{
+		/*
+		 * Sony IMX471 (IPU7 / ThinkPad X9, X1 Carbon Gen14).
+		 *
+		 * Analogue gain is programmed via V4L2_CID_ANALOGUE_GAIN to
+		 * register 0x0204. The kernel driver (imx471) exposes range
+		 * 0..800 and writes the code directly.
+		 *
+		 * Sony linear gain model used by several IMX sensors:
+		 *   G = 1024 / (1024 - code)
+		 * so code 0 → 1.0x and code 800 → ~4.57x.
+		 *
+		 * 10-bit optical black pedestal is typically 64 → 4096 at 16-bit.
+		 */
+		blackLevel_ = 4096;
+		gain_ = AnalogueGainLinear{ 0, 1024, -1, 1024 };
+	}
+};
+REGISTER_CAMERA_SENSOR_HELPER("imx471", CameraSensorHelperImx471)
+
 class CameraSensorHelperImx477 : public CameraSensorHelper
 {
 public:
diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp
b/src/libcamera/sensor/camera_sensor_properties.cpp
index fc9e184..7ba55dc 100644
--- a/src/libcamera/sensor/camera_sensor_properties.cpp
+++ b/src/libcamera/sensor/camera_sensor_properties.cpp
@@ -297,6 +297,23 @@  const CameraSensorProperties
*CameraSensorProperties::get(const std::string &sen
 				.hblankDelay = 2
 			},
 		} },
+		{ "imx471", {
+			/* 1/2.3-type; ~1.4 µm pixels (nm). Delays unverified. */
+			.unitCellSize = { 1400, 1400 },
+			.testPatternModes = {
+				{ controls::draft::TestPatternModeOff, 0 },
+				{ controls::draft::TestPatternModeSolidColor, 1 },
+				{ controls::draft::TestPatternModeColorBars, 2 },
+				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
+				{ controls::draft::TestPatternModePn9, 4 },
+			},
+			.sensorDelays = {
+				.exposureDelay = 2,
+				.gainDelay = 2,
+				.vblankDelay = 2,
+				.hblankDelay = 2
+			},
+		} },
 		{ "imx477", {
 			.unitCellSize = { 1550, 1550 },