media: Add support for the Sony IMX681

The Sony IMX681 is the user-facing camera on the Microsoft Surface Pro 11 for Business (Intel Lunar Lake, IPU7), enumerated as ACPI device SONY0681. Without a driver the camera does not appear at all.

  1/3  dt-bindings: media: Add Sony IMX681            (mine)
  2/3  media: i2c: Add Sony IMX681 sensor driver      (Andre Gilerson's)
  3/3  media: ipu-bridge: Add Sony IMX681             (mine)

The driver is Andre's work, reverse-engineered from I2C traces taken under Windows. I am carrying the submission, not the code: his Signed-off-by is on 2/3 with mine beneath it as the person passing it on. He is away until 28 September, so replies to review in the next few weeks will come from me. I have the hardware and the instrumentation is scripted, so anything you want measured I can measure.

Where the numbers come from
===========================

The parts taken from traces are labelled as such and not dressed up. imx681_init_regs[] is 21 register writes whose individual meaning is not known.

What is derived is written down. The link frequency comes from the PLL configuration visible in the same traces - 19.2 MHz EXCK, PLL2_MUL 303, PLL2_PRE_DIV 3, giving 1939.2 MHz on the bus and therefore 969.6 MHz per lane - and the pixel rate follows from that, the lane count and the bit depth. The gain law and the black level were measured against the sensor rather than read off a datasheet, because there is no public datasheet for this part.

Three things we know are arguable, raised here rather than left for review
=========================================================================

V4L2_CID_ANALOGUE_GAIN advertises 0..1020, which under the 1024/(1024-code) law reads as up to 256x, while the analogue stage stops at code 960. Above that the driver makes up the difference digitally, so the total gain still follows the formula and an AGC's arithmetic is correct - it is buying noise instead of exposure at the top of the range. Splitting it between ANALOGUE_GAIN and DIGITAL_GAIN is the obvious alternative. Andre has not given a view on that one, so if you want it, I will prepare it and put it to him rather than answer for him - which may mean it waits for his return at the end of September.

The chip-ID read is a single cci_read with no retry. This machine has failed it twice, once as 0x0081 and once as 0x0000 against 0x0681 - a sensor answering before it is ready. A retry absorbs that without inventing a longer reset delay. Andre would rather land the driver as written and add this afterwards, and I have kept it that way rather than editing his patch.

It may not be this sensor's fault: the ov13858 on the same board has now returned a wrong chip id once too, 0x1000 against 0xd855, reading it the same unretried way. Two events are not a conclusion, but a retry is cheap.

The input clock rate is read and logged but never checked against the 19.2 MHz the register sequence assumes. Same disposition: a follow-up, not a silent edit.

Testing
=======

On a Surface Pro 11, front camera, with all three patches applied - and on a kernel built from this base, not backported into a distro one:

  # uname -r
  7.3.0-rc1-imx681-medianext+
  # dmesg
  intel-ipu7 0000:00:05.0: Found supported sensor SONY0681:00 (\_SB.PC00.I2C5.CAMF)
  imx681 i2c-SONY0681:00: IMX681 probed successfully: 3844x2640 @ 969600000 Hz link freq
  intel_ipu7_isys.isys intel_ipu7.isys.40: bind imx681 3-0010 nlanes is 2 port is 2

The first of those lines is 3/3 doing its job: without the ipu-bridge entry the IPU does not recognise SONY0681 and nothing binds. No errors or warnings from the driver at all.

streams 3844x2640 SGRBG10, 30.01 fps
brightness vs gain 10.4 / 13.1 / 20.0 / 47.0 for codes 0 / 300 / 700 / 960

The second line is there because a mean brightness on its own cannot tell a dark room from a dead pipeline. Driving the gain and watching the frames follow it settles that; the captured frames are a recognisable room, right way up. The same sweep on a 7.0.0 kernel with the same driver source gives 11.8 / 14.7 / 22.7 / 52.8 - the difference is the room, an hour later.

checkpatch --strict clean on 2/3 and 3/3; on 1/3 only "does MAINTAINERS
need updating?", which 2/3 answers
make dt_binding_check passes, example extracted and compiled
build, W=1 imx681.o and ipu-bridge.o, no warnings

Based on media/next at f9536a8065 ("media: ipu-bridge: Add support additional link frequency").

German reported this HID as a bug on this list on 3 September and has had no reply since; he is on Cc here. He reaches IPU7 through the staging driver rather than ipu-bridge, so if this goes anywhere there is a second machine on a second path ready to try it.

https://lore.kernel.org/linux-media/[email protected]/

Andre Gilerson (1):
media: i2c: Add Sony IMX681 sensor driver

Sergey Lebedev (2):
dt-bindings: media: Add Sony IMX681
media: ipu-bridge: Add Sony IMX681

 .../bindings/media/i2c/sony,imx681.yaml       | 107 +++
 MAINTAINERS                                   |   8 +
 drivers/media/i2c/Kconfig                     |  10 +
 drivers/media/i2c/Makefile                    |   1 +
 drivers/media/i2c/imx681.c                    | 900 ++++++++++++++++++
 drivers/media/pci/intel/ipu-bridge.c          |   2 +
 6 files changed, 1028 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx681.yaml
 create mode 100644 drivers/media/i2c/imx681.c

2.53.0

The Sony IMX681 is a CMOS image sensor used as the user-facing camera on several Microsoft Surface devices. It operates from analog 2.8 V, digital 1.05 V and interface 1.8 V supplies, is programmable over I2C, and outputs 10-bit Bayer data over a two-lane MIPI CSI-2 D-PHY interface.

---
 .../bindings/media/i2c/sony,imx681.yaml       | 107 ++++++++++++++++++
 1 file changed, 107 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx681.yaml

new file mode 100644
index 000000000..6d2e4ddce
@@ -0,0 +1,107 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/sony,imx681.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sony IMX681 CMOS Image Sensor
+
+maintainers:
+  - Andre Gilerson <[email protected]>
+
+description:
+  The Sony IMX681 is a CMOS active pixel image sensor with a square pixel
+  array, used as the user-facing camera on several Microsoft Surface devices.
+  It operates from analog 2.8 V, digital 1.05 V and interface 1.8 V supplies,
+  is programmable over I2C, and outputs 10-bit Bayer data over a two-lane
+  MIPI CSI-2 D-PHY interface.
+
+allOf:
+  - $ref: /schemas/media/video-interface-devices.yaml#
+
+properties:
+  compatible:
+    const: sony,imx681
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description: Input clock (19.2 MHz)
+    maxItems: 1
+
+  avdd-supply:
+    description: Analog power supply (2.8 V)
+
+  dvdd-supply:
+    description: Digital power supply (1.05 V)
+
+  dovdd-supply:
+    description: Interface power supply (1.8 V)
+
+  reset-gpios:
+    description: Sensor reset (XCLR) GPIO, active low
+    maxItems: 1
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    unevaluatedProperties: false
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          data-lanes:
+            items:
+              - const: 1
+              - const: 2
+
+        required:
+          - data-lanes
+          - link-frequencies
+
+    required:
+      - endpoint
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - avdd-supply
+  - dvdd-supply
+  - dovdd-supply
+  - port
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        camera-sensor@10 {
+            compatible = "sony,imx681";
+            reg = <0x10>;
+            clocks = <&clock_cam>;
+            avdd-supply = <&vcc2v8_cam>;
+            dvdd-supply = <&vcc1v05_cam>;
+            dovdd-supply = <&vcc1v8_cam>;
+            reset-gpios = <&gpio 4 GPIO_ACTIVE_LOW>;
+            orientation = <0>;
+            rotation = <0>;
+
+            port {
+                imx681_ep: endpoint {
+                    data-lanes = <1 2>;
+                    link-frequencies = /bits/ 64 <969600000>;
+                    remote-endpoint = <&csi_in>;
+                };
+            };
+        };
+    };
+...

2.53.0

From: Andre Gilerson <[email protected]>

The Sony IMX681 is the user-facing camera on the Microsoft Surface Pro 11 for Business (Intel Lunar Lake, IPU7), where it is enumerated as ACPI device SONY0681. Without a driver the camera does not appear at all.

There is no public documentation for this sensor. The initialisation sequence and the mode registers were recovered from I2C traces taken under Windows, and the driver does not pretend otherwise: imx681_init_regs[] is 21 register writes whose individual meaning is not known.

What is not from the traces is derived and written down. The link frequency comes from the PLL configuration visible in the same traces - 19.2 MHz EXCK, PLL2_MUL 303, PLL2_PRE_DIV 3, giving 1939.2 MHz on the bus and therefore 969.6 MHz per lane - and the pixel rate follows from that, the lane count and the bit depth. The gain law and the black level were measured against the sensor rather than taken from the traces; the numbers are in the cover letter.

The driver uses the streams API, v4l2-cci for register access, the subdev state API and runtime PM, and validates the endpoint's lane count and link frequency against what the firmware describes.

---
 MAINTAINERS                |   8 +
 drivers/media/i2c/Kconfig  |  10 +
 drivers/media/i2c/Makefile |   1 +
 drivers/media/i2c/imx681.c | 900 +++++++++++++++++++++++++++++++++++++
 4 files changed, 919 insertions(+)
 create mode 100644 drivers/media/i2c/imx681.c

index e12dc3ca5..198eaeefa 100644
@@ -25586,6 +25586,14 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/media/i2c/sony,imx678.yaml
 F:	drivers/media/i2c/imx678.c
 
+SONY IMX681 SENSOR DRIVER
+M:	Andre Gilerson <[email protected]>
+L:	[email protected]
+S:	Maintained
+T:	git git://linuxtv.org/media.git
+F:	Documentation/devicetree/bindings/media/i2c/sony,imx681.yaml
+F:	drivers/media/i2c/imx681.c
+
 SONY MEMORYSTICK SUBSYSTEM
 M:	Maxim Levitsky <[email protected]>
 M:	Alex Dubov <[email protected]>

index 5c52007f9..5f7b0d22c 100644
@@ -310,6 +310,16 @@ config VIDEO_IMX678
 	  To compile this driver as a module, choose M here: the
 	  module will be called imx678.
 
+config VIDEO_IMX681
+	tristate "Sony IMX681 sensor support"
+	select V4L2_CCI_I2C
+	help
+	  This is a Video4Linux2 sensor driver for the Sony
+	  IMX681 camera.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called imx681.
+
 config VIDEO_MAX9271_LIB
 	tristate
 

index d04bd5724..8d6bd7466 100644
@@ -63,6 +63,7 @@ obj-$(CONFIG_VIDEO_IMX412) += imx412.o
 obj-$(CONFIG_VIDEO_IMX415) += imx415.o
 obj-$(CONFIG_VIDEO_IMX678) += imx678.o
 obj-$(CONFIG_VIDEO_IMX471) += imx471.o
+obj-$(CONFIG_VIDEO_IMX681) += imx681.o
 obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o
 obj-$(CONFIG_VIDEO_ISL7998X) += isl7998x.o
 obj-$(CONFIG_VIDEO_KS0127) += ks0127.o

2.53.0

Thank you - four of the five are real, and I have checked each against the patch rather than agreeing on sight. All four are fixed in v2, which follows this message. The author is away until 28 September, so v2 comes from me; where a fix reverses something he wrote deliberately I have said so and given the reasoning rather than leaving the question hanging for three weeks.

Confirmed
=========

1. devm_regulator_bulk_get() ignored. probe() logs a dev_dbg and continues, and imx681_power_on() then calls regulator_bulk_enable() on an array nothing filled in. Your use-after-free reading is the sharper one and it holds:
_regulator_bulk_get() unwinds with "while (--i >= 0) regulator_put(consumers[i].consumer)" and leaves the pointers where they are, so a get that fails partway puts freed regulators into supplies[] rather than NULLs. The likely failure is -EPROBE_DEFER, which is ordinary. On this machine INT3472 hands out dummies so the get succeeds, which is exactly why testing did not find it. v2 makes it fatal, via dev_err_probe(), which is also the right handling for the -EPROBE_DEFER case.

2. pm_runtime_get_if_active(). It returns 1, 0, or a negative errno, and "if (!...)" only catches the 0. An error therefore falls through to issue I2C to a possibly-suspended sensor and then pm_runtime_put()s a reference it never took. v2 tests it as "if (pm_runtime_get_if_active(dev) <= 0)". The kerneldoc is explicit that on -EINVAL "the usage_count will remain unmodified", so the put is unbalanced and not merely redundant.

3. Missing pm_runtime_put_noidle() on the probe error path. The reference taken by pm_runtime_get_noresume() is never released, and pm_runtime_set_suspended() is called while it is still held. v2 adds it at error_pm.

Not a bug, and I think demonstrably so
======================================

The NULL-state dereference in imx681_set_pad_format(). The subdev core reaches .set_fmt through v4l2_subdev_call_state_active(), which always supplies a state, and the drivers around it agree: imx415, imx355, ov02c10 and ov08x40 all call v4l2_subdev_state_get_format(state, ...) with no NULL check between them. If the core can hand a NULL state to .set_fmt then this is a subsystem-wide problem rather than one driver's, and I would rather hear that from a maintainer than paper over it here.

The gain clash, which is the interesting one
============================================

Correct, and worse than it looks. The analogue handler writes IMX681_REG_DIGITAL_GAIN on *every* call, not only above code 960: above 960 it writes the spillover it computed, and at or below 960 it writes the 1.0x it initialised the local to. V4L2_CID_DIGITAL_GAIN writes the same register directly. So setting digital gain to 4x and then moving analogue gain anywhere at all puts 1x back into 0x020E - no error returned, and the DIGITAL_GAIN control still reporting 4x.

The v1 cover asked whether the range should be split honestly between the two controls. I have stopped asking and done it, because the two turn out to be one
question: clustering would stop the controls overwriting each other, but it would then need an invented rule for what analogue 1020 together with digital 2x means, and there is no honest answer - they are two names for one register. So v2 advertises ANALOGUE_GAIN as 0..960, the 1x..16x the analogue stage actually does, and DIGITAL_GAIN owns 0x020E alone.

Measured rather than argued, by reading 0x020E over i2c while the sensor streams. It has to be while streaming: at stream start __v4l2_ctrl_handler_setup() applies the controls in creation order, analogue before digital, so digital wins and the clash is invisible. It bites a live AE loop, which is the case that matters.

                                     v1 module     v2 module
  start                              0x0100        0x0100
  after digital_gain = 4x            0x0400        0x0400
  after analogue_gain 500 -> 700     0x0100  <--   0x0400
  DIGITAL_GAIN control reads         1024          1024

Both modules were built from the same tree and swapped with rmmod/insmod, so nothing but the driver differs between the columns. The last row is the point:
the control reports 4x in both, and in v1 the register disagrees with it silently.

For Andre, on your return
=========================

Two of these touch choices that look deliberate in your code, so they are yours to overrule.

The regulator handling. Your comment says "Continue without regulators - INT3472 may handle power", and v2 makes that failure fatal instead. My reading is that the get does not fail for supplies the firmware leaves undescribed - it hands out dummies, which is where this machine's "supply dvdd not found, using dummy regulator" comes from, and the camera still probes with the change in place. If you wrote the comment because you had actually seen the get fail on some machine, then I have removed a workaround you needed: say so and it goes back in v3.

The gain range. I have split it rather than holding v2 for three weeks:
ANALOGUE_GAIN stops at 960 and no longer touches the digital register. Your own define carried the comment "combined analog+digital", and V4L2 has no control meaning combined gain, so I believe this is what the ABI asks for - but it is your design and you can have it back in v3.

And one you did not ask for: the series lists you as M: for SONY IMX681 in MAINTAINERS. That is the convention for a driver's author, but it is also a standing obligation to everyone who reports a bug against it later, so it should be your choice and not mine. It is already in v2 because leaving a new driver with no maintainer entry is worse; say the word and it becomes me, or both of us.

I ran an independent review of the same file before writing this, and it found the same four and nothing else.

One thing worth saying plainly, since v1's cover letter listed its testing at some length. That list - applies to media/next, correct authorship under git am, checkpatch, dt_binding_check, a W=1 build, booted on the hardware with the camera streaming - would not have found a single one of these four. They live on error paths this board never takes and in a control interaction that needs both controls driven while streaming. Testing that the happy path works is not review, and I should not have been as satisfied with that list as I was.

Sergey

The Microsoft Surface Pro 11 for Business (Intel Lunar Lake, IPU7) carries a Sony IMX681 as its user-facing sensor, enumerated as ACPI device SONY0681. Add it so ipu_bridge_connect_sensors() builds its half of the graph; without the entry the sensor is never connected, whatever driver is present.

One link frequency, 969.6 MHz, on two CSI-2 lanes.

---
 drivers/media/pci/intel/ipu-bridge.c | 2 ++
 1 file changed, 2 insertions(+)

index 7e65491fa..68c2eb326 100644
@@ -97,6 +97,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
 	IPU_SENSOR_CONFIG("OVTI5675", 1, 450000000),
 	/* Omnivision OV8856 */
 	IPU_SENSOR_CONFIG("OVTI8856", 3, 180000000, 360000000, 720000000),
+	/* Sony IMX681 */
+	IPU_SENSOR_CONFIG("SONY0681", 1, 969600000),
 	/* Sony IMX471 */
 	IPU_SENSOR_CONFIG("SONY471A", 1, 200000000),
 	/* Sony IMX471 (found on Lenovo X1 Carbon G14) */

2.53.0