[PATCH v3 0/9] media: i2c: ov7251: fixes and infrared strobe for Surface Book 2

[Antoine Pichot <antoine.pichot@gmail.com>]

The Microsoft Surface Book 2 has an OV7251 infrared camera (used by
Windows Hello) next to its front camera, both on the Intel IPU3 camera
interface. Its 850 nm emitter is wired to the sensor's STROBE output
pin: the ACPI tables describe no LED, GPIO line or regulator for it.
With the mainline driver the camera streams, but always unlit.

Patches 1 to 4 fix bugs found while testing on this machine and on a
Surface Pro 7+, 5 and 6 rework the driver's state handling and add the
placement controls:

- 1: the 19.2 MHz PLL table for the 319.2 MHz link multiplies the
  clock before dividing it, which runs the PLL's voltage-controlled
  oscillator (VCO) at 2553.6 MHz against the 300 to 800 MHz the
  datasheet allows. The Surface Pro 7+ gets CSI-2 errors or stalls in
  most runs at that setting; the Book 2 copes. Dividing first keeps
  the link rate and brings the VCO to 638.4 MHz (Fixes, Cc stable). It
  precedes patch 2, which is what makes the driver select this table.
- 2: the link frequency index is taken from the firmware list instead
  of the driver's table, so the sensor runs at 240 MHz while
  ipu-bridge asks for 319.2 MHz (Fixes, Cc stable).
- 3: V4L2_CID_PIXEL_RATE reports the CSI-2 bus rate instead of the
  pixel array clock, which is 48 MHz at both link frequencies (Fixes,
  Cc stable).
- 4: the 60 and 90 fps frame intervals are accepted but the sensor
  keeps streaming at 30 fps, the vertical blanking control not being
  updated (Fixes, Cc stable).
- 5: conversion to the sub-device active state, as Sakari asked on
  v1 [7]. The core then handles control events and v4l2-compliance
  passes on the sensor.
- 6: orientation and rotation controls from firmware.

Patch 4 only fixes the current behaviour of set_frame_interval, which
Dave Stevenson noted on v1 does not belong in a raw sensor driver that
also exposes the vertical blanking control. Patches 5 to 9 apply on
top of 1 to 4.

Patches 7 and 8 add the emitter.

Patch 7 is Dave Stevenson's, taken from his ov7251 branch with his
agreement. The three mode tables wrote the same 117 registers; the patch
moves them into the global init table, written once after the power-on
reset, and leaves each mode table with the six clock control registers
(0x3016 to 0x301c). The frame length, the only other value that differed
between the tables, is dropped from them since the vertical blanking
control writes it. Two side effects show on the bus: a stream restarted
before the runtime autosuspend powers the sensor down no longer rewrites
the common registers, which the sensor keeps in software standby, and
register 0x3820 holds 0x40 while streaming, the value the mode tables
had always asked for, instead of 0x00; the picture is unchanged. The
datasheet names the bits of the clock control registers as per-block
clock gates and resets and does not say what a cleared bit does. The
30 fps table holds the reset values but for one reset bit, the 60 and
90 fps tables what the machine's Windows driver writes in all four of
its modes. On the Book 2 the sensor streams the same with either set at
all three rates (rate, black level, test pattern, no CSI-2 errors).
Clearing the gates, in [4] and then one at a time, changed only two
blocks the driver does not use (OTP load, group hold) and the timing
control gate, which stops the frames when cleared. Power was not
measured. Whether one set moves to the init table, as [4] proposed, is
left to a follow-up; the rest of Dave's branch (CCI register helpers
among them) waits for this series to land, as agreed on the v2 thread.

Patch 8 adds a V4L2_CID_FLASH_STROBE_OE control that enables the
STROBE pin, pulses it on alternate frames and makes the pulse follow
the exposure, from 52 lines up to 45% of the frame, capped at 776
lines. This matches the Windows driver of this machine (Intel
ov7251.sys 30.18305.6.5127, read with a decompiler): all four of its
streaming modes enable the STROBE pin and clamp the pulse to the
exposure within 52 to 776 lines; the two Windows Hello uses light
alternate frames (0xaa), the other two every frame (0xff). A register
dump from a Surface Book 2 during a Windows Hello sign-in [1] matches
the 30 fps alternate mode. With every frame lit and filmed by a phone
at locked exposure, each pulse is 1.1x (default exposure) to 1.2x
(776-line pulse) as bright as during a Windows Hello sign-in on the
same unit; with alternate frames the average emission is within the
same ratio of Windows'.

Two design choices differ from ov9282, the other sensor with this
control. ov9282 exposes the pulse length as V4L2_CID_FLASH_DURATION;
here the pulse follows the exposure within the envelope the Windows
driver applies at every frame rate, so user space cannot drive the
emitter beyond it, and a duration control can be added later. ov9282
pulses every frame; here the pattern is fixed to alternate frames
because the dark frames are the ambient-light reference that face
recognition subtracts from the lit ones (V4L2 has no per-frame
illumination flag, so user space tells them apart by brightness). A
control to light every frame, asked about on v2 [5] without an answer,
can be added later without changing this one.

A linux-surface pull request for the Surface Pro 7+ [2] turns the
emitter on whenever the sensor streams, with a module parameter to
disable it. This series leaves the choice to user space: the control
is off by default, and the emitter is off whenever the sensor is not
streaming.

Patch 9 makes ipu-bridge report a rotation of 90 degrees for the
infrared sensor, whose frames show an upright scene turned 90 degrees
clockwise while the firmware says 0. It adds a DMI quirk table next to
the existing one for upside-down sensors, with entries for the Surface
Book 2 and the Surface Pro 7+, the two machines checked. The entries
match on the SKU because Microsoft reuses the product name across
sizes and the 15 inch Book 2 was not checked. A user-space project
rotates the Surface Book 3's infrared frames by 90 degrees too [3].

Patch 8 needs V4L2_CID_FLASH_STROBE_OE, added in v7.0 by commit
5be4154f6255 ("media: v4l: ctrls: add a control for enabling strobe
output"), and is not meant for stable. Only patches 1 to 4 apply to
older trees on their own.

[1] https://github.com/linux-surface/linux-surface/issues/739
[2] https://github.com/linux-surface/linux-surface/pull/2252
[3] https://github.com/linux-surface/linux-surface/pull/2289
[4] https://lore.kernel.org/linux-media/20261030142628.2235190-1-antoine.pichot@gmail.com/
[5] https://lore.kernel.org/linux-media/20261030121235.2163478-1-antoine.pichot@gmail.com/

Changes since v2 [6]:
- Patch 1: Reported-by and Closes tags for D. Manresa's report, and
  Dave Stevenson's measurement cited as [1] in the text (Andy
  Shevchenko). The sentence on the 24 MHz tables was wrong, Dave has
  tested them: they are left to a separate change (Dave Stevenson).
- Patch 3: the pixel clock is defined as 48 * HZ_PER_MHZ, and Dave's
  report carries Reported-by and Closes tags (Andy Shevchenko).
- Patch 5: code unchanged; Suggested-by and Link added for Sakari's v1
  request. V4L2 has no guard class for the state lock, so the explicit
  lock and unlock pair stays, as imx290, imx415, ov2740 and ov8858 do in
  s_stream (Andy Shevchenko, answered on the thread). The move to
  enable_streams and disable_streams, which removes the pair, is left to
  a follow-up to keep this series to the fixes and the camera.
- Patch 7: replaced by Dave Stevenson's patch described above, which
  answers his question on v1 about where the strobe registers belong;
  v2's patch moved only the strobe registers out of the mode tables.
  D. Manresa's Tested-by from v2's patch 7 is not carried over, the
  patch being a different one.
- Patch 8: follows the init table of patch 7, which now holds the
  strobe defaults it restores; the commit message now also says that
  the dark frames are the ambient-light reference face recognition
  subtracts.
- Patch 9: D. Manresa's report cited as [1] in the text (Andy
  Shevchenko).
- Rebased on the current media-committers next.

[6] https://lore.kernel.org/linux-media/20261028112154.855185-1-antoine.pichot@gmail.com/
[7] https://lore.kernel.org/linux-media/20261026111032.88355-1-antoine.pichot@gmail.com/

Tested on a Surface Book 2 (i7-8650U) with the series built out of
tree against linux-surface 7.2.12 and 7.2.15 (test suite and logs:
https://github.com/antoine79/surface-ir-camera). The rebase changed
no code: ov7251.c on the base commit is byte-identical to the driver
tested, and both files build on the base with W=1 without warnings.

- link frequency 319200000, pixel rate 48000000, 30.0 fps measured;
  PLL registers read back as programmed while streaming
- 3 x 155 frames at 30 fps and 3 x 155 at 90 fps on the 319.2 MHz
  link: 0 dropped buffers, 0 CSI-2 error lines
- v4l2-compliance 1.32.0 -u on the sensor: 0 failures
- --set-subdev-fps 60 / 90: 60.16 / 90.44 fps measured; mode switches
  from 30 to 90, to 60 and back to 30 fps on a powered sensor give a
  correct first frame every time
- the clock, timing, PLL, flip and strobe registers read back while
  streaming at 30, 60 and 90 fps are those of v2, apart from 0x3820
- orientation front, rotation 90 with patch 9 (on 7.2.12; the 7.2.15
  runs kept the stock ipu-bridge); the colour sensors still report 180
- dark frames and the sensor test pattern unchanged
- strobe span read back while streaming: 504 lines at exposure 504 and
  776 at exposure 1704 (30 fps), 258 at exposure 552 (90 fps); it
  follows exposure changes mid-stream
- mean pixel value of an indoor infrared frame at analogue gain 128
  (12 Nov run; patch 8's message quotes the 25 Oct run): 18 to 19 unlit;
  with the strobe enabled the lit frames read 517 at exposure 504 and
  681 at 1704, the dark frames in between 18 to 21
- Howdy face login recognises the enrolled face through a v4l2loopback
  bridge that passes the lit frames
- strobe off and sensor suspended within 2 s of the stream stopping

D. Manresa's Tested-by tags on patches 4 and 6 date from v1; the changes
those patches make are the same since. The Surface Pro 7+ ran v1 and the
divider fix of patch 1 as a test patch (13 clean 150-frame runs of 13),
not v2 or v3. The remaining v4l2-compliance failures are in the
ipu3-cio2 capture driver (buffer creation and mapping checks) and
unchanged by this series.

I developed this series with an AI coding assistant (Claude) over
interactive sessions: it drafted the code, the commit messages, the
test scripts and this letter from my notes, measurements and register
dumps. The design choices (alternate frames, the pulse limits, the
exact SKU match), the reading of the Windows driver and the
measurements above are mine, and I reviewed and tested every patch on
the hardware before signing it off.

Antoine Pichot (8):
  media: i2c: ov7251: Use in-range PLL1 dividers for the 319.2 MHz link
  media: i2c: ov7251: Fix link frequency index
  media: i2c: ov7251: Report the pixel array rate for both link
    frequencies
  media: i2c: ov7251: Reset vblank when the frame interval changes
  media: i2c: ov7251: Use sub-device active state
  media: i2c: ov7251: Add orientation and rotation controls
  media: i2c: ov7251: Add strobe output enable control
  media: ipu-bridge: Add a rotation quirk for the Surface Book 2 and Pro
    7+

Dave Stevenson (1):
  media: i2c: ov7251: Move the common registers into the global init
    table

 drivers/media/i2c/ov7251.c           | 531 ++++++++-------------------
 drivers/media/pci/intel/ipu-bridge.c |  31 ++
 2 files changed, 191 insertions(+), 371 deletions(-)


base-commit: 3fc9303dba01676c2090768126e5327e341b7761
-- 
2.56.0