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

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

[Antoine Pichot]

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 3 fix bugs found while testing on this machine, patch 4
adds the placement controls:

- 1: 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).
- 2: 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).
- 3: control events, the one v4l2-compliance failure on the sensor.
- 4: orientation and rotation controls from firmware.

Patches 5 and 6 add the emitter. 5 moves the strobe registers, which
the three mode tables write identically, out of the tables without
functional change. 6 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 and
never more than 776 lines. This matches the Windows driver installed
on 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 modes
Windows Hello uses light alternate frames (0xaa), the other two every
frame (0xff). A register dump taken on a Surface Book 2 during a
Windows Hello sign-in [1] matches the 30 fps alternate mode. V4L2 has
no per-frame illumination flag, so user space tells lit frames from
dark ones by brightness; the dark frames give it the ambient-light
reference a face liveness check uses. 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'.

A linux-surface pull request for the Surface Pro 7+ [2] instead turns
the emitter on unconditionally while 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 7 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. A user-space project for the
Surface Book 3 rotates that model's infrared frames by 90 degrees as
well [3]; the quirk covers only the Book 2, the one checked.

[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

Tested on a Surface Book 2 (i7-8650U) with the series built out of
tree against linux-surface 7.2.12 (test suite, logs and a v4l2loopback
bridge for applications: https://github.com/antoine79/surface-ir-camera):

- link frequency 319200000, pixel rate 63840000, 30.01 fps unchanged
- v4l2-compliance -u on the sensor: 0 failures (was 1)
- --set-subdev-fps 60 / 90: 60.16 / 90.45 fps measured (was 30.01)
- orientation front, rotation 90 (0 before patch 7); the colour sensors
  still report 180
- dark frames and the sensor test pattern unchanged after patch 5
- strobe span read back while streaming: 504 lines at exposure 504,
  776 at exposure 1704 (30 fps), 258 at exposure 552 (90 fps)
- mean pixel value of an indoor infrared frame at analogue gain 128:
  19 unlit; with the strobe enabled the lit frames read 496 at
  exposure 504 and 587 at 1704, the dark frames in between 18 to 20
- strobe off and sensor suspended within 2 s of the stream stopping
- 10 unbind/unload/load cycles: no warning in the kernel log
- Howdy face login enrols and recognises through a v4l2loopback bridge
  that passes the lit frames

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): 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 (7):
  media: i2c: ov7251: Fix link frequency index
  media: i2c: ov7251: Reset vblank when the frame interval changes
  media: i2c: ov7251: Add control event support
  media: i2c: ov7251: Add orientation and rotation controls
  media: i2c: ov7251: Move the strobe registers out of the mode tables
  media: i2c: ov7251: Add strobe output enable control
  media: ipu-bridge: Add a 90 degree rotation quirk for Surface Book 2

 drivers/media/i2c/ov7251.c           | 203 ++++++++++++++++++++++++-----------
 drivers/media/pci/intel/ipu-bridge.c |  22 ++++
 2 files changed, 161 insertions(+), 64 deletions(-)

-- 
2.53.0

[PATCH 1/7] media: i2c: ov7251: Fix link frequency index

[Antoine Pichot]

ov7251_check_hwcfg() stores i, the index into the firmware's
link_frequencies array, in link_freq_idx, which the driver then uses to
index its own link_freq[], pixel_rates[] and pll1[] tables. The matching
driver entry is j. Store j.

Observed on a Surface Book 2, where ipu-bridge lists one link frequency,
319.2 MHz, which is link_freq[1]: V4L2_CID_LINK_FREQ reads 240000000,
and the pixel rate and PLL registers are off the same way.

Fixes: cc125aaa5a78 ("media: i2c: Provide ov7251_check_hwcfg()")
Cc: stable@vger.kernel.org
Signed-off-by: Antoine Pichot <antoine.pichot@gmail.com>
---
 drivers/media/i2c/ov7251.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index fce6344b51..1413f1856e 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -1523,7 +1523,7 @@ static int ov7251_check_hwcfg(struct ov7251 *ov7251)
 		goto out_free_bus_cfg;
 	}
 
-	ov7251->link_freq_idx = i;
+	ov7251->link_freq_idx = j;
 
 out_free_bus_cfg:
 	v4l2_fwnode_endpoint_free(&bus_cfg);
-- 
2.53.0

Re: [PATCH 1/7] media: i2c: ov7251: Fix link frequency index

[D. Manresa]

On Mon, Oct 26, 2026 at 08:10:26PM +0900, Antoine Pichot wrote:
> Observed on a Surface Book 2, where ipu-bridge lists one link frequency,
> 319.2 MHz, which is link_freq[1]: V4L2_CID_LINK_FREQ reads 240000000,
> and the pixel rate and PLL registers are off the same way.

The diagnosis holds on a Surface Pro 7+ (IPU6 Tiger Lake, 8086:9a19).
ipu-bridge gives this machine the same single frequency as yours:

  IPU_SENSOR_CONFIG("INT347E", 1, 319200000)

and unpatched, the sensor reports

  link_frequency ... value=0 (240000000)
  pixel_rate ... value=48000000

exactly the mismatch you describe.

Applying the patch makes this machine worse, though, and since it carries
Cc: stable I think that is worth resolving before it goes in.

The fix does not only correct a reported value: it reprograms the PLL and
moves the link from 480 to 638.4 Mbps.

                     unpatched v6.19     + this patch
  link_frequency     0 (240000000)       1 (319200000)
  pixel_rate         48000000            63840000
  PLL1 0x30b1/b3/b4  0x01 / 0x4b / 0x03  0x04 / 0x85 / 0x01

The IPU6 receiver here does not hold that link. 150-frame captures of the
640x480 30 fps mode, counting dropped buffers from v4l2-ctl and csi2-5
error lines from dmesg (including the "callbacks suppressed" lines):

  unpatched  run 1  dropped 23   csi2 errors ~7216
  unpatched  run 2  dropped 23   csi2 errors 13277
  unpatched  run 3  dropped 13   csi2 errors 4722
  patched    run 1  dropped 106  csi2 errors 120788
  patched    run 3  stalled: 50 frames, then none for 60 s

The errors are DPHY fatal error, SOT sync error, inter-frame short/long
packet discarded and frame sync error. The frames that do arrive look
normal, so this is the link and not the image.

Two things to be honest about. This unit is not a clean baseline: even
unpatched, mainline already logs thousands of CSI-2 errors per run at
30 fps, and the out-of-tree Surface driver sidesteps the whole area with
a vts_boost parameter -- at its production VTS of 3448, 15 fps, I measure
zero CSI-2 errors. And the sample is small: three runs each, one unit.
What makes me report it anyway is that the one-line difference was
consistent in every run, that it introduced hard stalls I never saw
unpatched, and that going back to a build with only that line reverted
gave 568 consecutive frames with zero CSI-2 errors.

One hypothesis tried and failed, so you do not repeat it: the out-of-tree
Surface driver notes that the vendor runs the 319.2 MHz PLL with the MIPI
divider at 0x01 rather than 0x05. Building this patch with mipi_div
changed to 0x01 in the 19_2_mhz_319_2_mhz entry was worse, not better --
3 frames in about four minutes. So the divider alone is not the missing
piece. The vendor path also uses VTS 522, which I did not try.

So either ipu-bridge's advertised link frequency is wrong for this SKU,
or mainline's 319.2 MHz PLL entry needs more than the index fix to be
usable. I have the machine for a few more days and can run whatever would
help decide which.

The rest of the series tests well here; I have replied to the cover letter
with that.

D. Manresa

(Written with AI assistance, directed and reviewed by me.)

Re: [PATCH 1/7] media: i2c: ov7251: Fix link frequency index

[Antoine Pichot]

Hi,

On Tue, Oct 27, 2026 at 03:26:45AM -0700, D. Manresa wrote:
> Applying the patch makes this machine worse, though, and since it carries
> Cc: stable I think that is worth resolving before it goes in.

Agreed, and thank you for the measurements.

The patch selects the 319.2 MHz PLL table, which the index bug has kept
unused on IPU machines since it was added in 2022. That table runs the
PLL oscillator (the VCO of figure 2-12 in the OV7251 datasheet, rev
2.12) out of range: it multiplies before it divides, so the oscillator
sits at 19.2 x 133 = 2553.6 MHz before the divide by 4 brings it down
to 638.4 MHz, and table 2-11 allows 300 to 800 MHz. The 240 MHz table
divides by 3 first and stays at 480 MHz. The datasheet is public at
https://dlscorp.com/wp-content/uploads/2019/02/ov7750_ov7251_csp3_rev1f_ds_2.12.pdf

The Windows driver on the Surface Book 2 uses both orders. Its Windows
Hello mode divides by 4 first, which keeps the oscillator at 638.4 MHz
for the same 638.4 Mbps, and writes 0x1f to 0x4837, the MIPI pixel
clock period register. Its two 640x480 modes keep the mainline order,
with the MIPI divider at 0x01 and 0x4837 at 0x19.

On my Surface Book 2 (IPU3, kernel 7.2) I ran three 150-frame captures
at 30 fps with each of these, the last four with the series applied:

  the 240 MHz table (unpatched)
  the mainline 319.2 MHz table
  the Windows Hello divider order
  the Windows Hello divider order + 0x4837 = 0x1f
  the mainline table + mipi_div = 0x01 (what you tried)

All gave 0 dropped buffers and 0 CSI-2 error lines, so the divider swap
is only a hypothesis for your machine.

> One hypothesis tried and failed, so you do not repeat it: the out-of-tree
> Surface driver notes that the vendor runs the 319.2 MHz PLL with the MIPI
> divider at 0x01 rather than 0x05. Building this patch with mipi_div
> changed to 0x01 in the 19_2_mhz_319_2_mhz entry was worse, not better --
> 3 frames in about four minutes. So the divider alone is not the missing
> piece. The vendor path also uses VTS 522, which I did not try.

The datasheet decodes both as "divide by 1" (0x30b5[2:0]: 2 is /2, 4 is
/4, anything else /1). A frame length (VTS) of 522 belongs to the
Windows driver's 640x480 modes at about 99 fps, so it is no alternative
at 30 fps.

> So either ipu-bridge's advertised link frequency is wrong for this SKU,
> or mainline's 319.2 MHz PLL entry needs more than the index fix to be
> usable. I have the machine for a few more days and can run whatever would
> help decide which.

Could you please try the two patches below, the first alone and then
both, with your 150-frame measurement? They are on
https://github.com/antoine79/surface-ir-camera/tree/test/pll1-vco under
kernel/patches-test/, for git am on top of 1/7.

The first swaps the two dividers of the 319.2 MHz table:

  -  .pre_div = 0x01,
  +  .pre_div = 0x04,
     .mult = 0x85,
  -  .div = 0x04,
  +  .div = 0x01,

The second also writes 0x1f instead of 0x19 to 0x4837 in the three mode
tables.

If the swap helps, v2 puts the PLL table fix before the index fix, both
Cc: stable. If it does not, the question moves to the Pro 7+ receiver:
ipu-bridge asks for 319.2 MHz on both machines, and the Windows driver
programs 638.4 Mbps on the Book 2. Your unpatched numbers point the same
way: at 480 Mbps, three runs logged thousands of error lines and your
568-frame run logged none, with the same timing per line. Could you
please also run 2/7 without 1/7 at 60 and 90 fps? That shows whether the
error count follows the number of frame starts.

After 1/7, pixel_rate reads 63840000, but the pixel array runs at 48 MHz
whatever the link frequency; v2 fixes that in a separate patch. My
319.2 MHz captures above ran at 30.0 fps, which is 48e6 / (928 pixels
per line x 1724 lines); 63.84 MHz would give 39.9 fps. 63.84 MHz is the
CSI-2 bus rate, not what V4L2_CID_PIXEL_RATE reports
(Documentation/driver-api/media/tx-rx.rst).

Thanks,
Antoine

[PATCH 2/7] media: i2c: ov7251: Reset vblank when the frame interval changes

[Antoine Pichot]

Setting a 60 or 90 fps frame interval is accepted and read back, but
the sensor keeps streaming at 30 fps.

The three modes differ only in their frame length (VTS: 1724, 860 and
572 lines), which each mode's register table writes to 0x380e/0x380f.
Since the V4L2_CID_VBLANK control was added, s_stream() applies the
controls with __v4l2_ctrl_handler_setup() after the mode table, so the
VBLANK control rewrites VTS. ov7251_set_frame_interval() switches
current_mode but never updates the VBLANK control, which keeps the 30
fps value (1244, VTS 1724) and overrides the 60 or 90 fps VTS at every
stream start.

ov7251_set_format() has the same issue in the other direction: it
updates the VBLANK range and default for the new mode, but
__v4l2_ctrl_modify_range() keeps the current value when it is still
in range, so after a 60 fps frame interval a format change would go
back to the 30 fps mode with a 60 fps VTS.

Update the VBLANK range and default in ov7251_set_frame_interval() too,
and set VBLANK to the mode's default in both places, as is already
done for exposure and gain. The VBLANK handler then also restores the
exposure range for the new frame length.

Tested on a Microsoft Surface Book 2 (IPU3 CIO2): after
v4l2-ctl --set-subdev-fps pad=0,fps=60 (90), v4l2-ctl measures 60.15
(90.45) fps instead of 30.01, with vblank 380 (92).

Fixes: 1b598f413c7a ("media: i2c: Add vblank control to ov7251 driver")
Cc: stable@vger.kernel.org
Signed-off-by: Antoine Pichot <antoine.pichot@gmail.com>
---
 drivers/media/i2c/ov7251.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index 1413f1856e..8dad747f77 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -1261,6 +1261,10 @@ static int ov7251_set_format(struct v4l2_subdev *sd,
 		if (ret < 0)
 			goto exit;
 
+		ret = __v4l2_ctrl_s_ctrl(ov7251->vblank, vblank_def);
+		if (ret < 0)
+			goto exit;
+
 		ov7251->current_mode = new_mode;
 	}
 
@@ -1414,6 +1418,7 @@ static int ov7251_set_frame_interval(struct v4l2_subdev *subdev,
 {
 	struct ov7251 *ov7251 = to_ov7251(subdev);
 	const struct ov7251_mode_info *new_mode;
+	int vblank_def;
 	int ret = 0;
 
 	/*
@@ -1442,6 +1447,19 @@ static int ov7251_set_frame_interval(struct v4l2_subdev *subdev,
 		if (ret < 0)
 			goto exit;
 
+		vblank_def = new_mode->vts - new_mode->height;
+		ret = __v4l2_ctrl_modify_range(ov7251->vblank,
+					       OV7251_TIMING_MIN_VTS,
+					       OV7251_TIMING_MAX_VTS -
+					       new_mode->height,
+					       1, vblank_def);
+		if (ret < 0)
+			goto exit;
+
+		ret = __v4l2_ctrl_s_ctrl(ov7251->vblank, vblank_def);
+		if (ret < 0)
+			goto exit;
+
 		ov7251->current_mode = new_mode;
 	}
 
-- 
2.53.0

[PATCH 3/7] media: i2c: ov7251: Add control event support

[Antoine Pichot]

The driver exposes writable controls on its subdev node but does not
support control events, so VIDIOC_SUBSCRIBE_EVENT fails and
v4l2-compliance reports:

  fail: v4l2-test-controls.cpp(1128): subscribe event for control
  'User Controls' failed
  test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL

Set V4L2_SUBDEV_FL_HAS_EVENTS and use the standard control event
helpers, as other sensor drivers do.

Signed-off-by: Antoine Pichot <antoine.pichot@gmail.com>
---
 drivers/media/i2c/ov7251.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index 8dad747f77..917285a62d 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -19,6 +19,7 @@
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <media/v4l2-ctrls.h>
+#include <media/v4l2-event.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
@@ -1471,6 +1472,11 @@ static int ov7251_set_frame_interval(struct v4l2_subdev *subdev,
 	return ret;
 }
 
+static const struct v4l2_subdev_core_ops ov7251_core_ops = {
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
 static const struct v4l2_subdev_video_ops ov7251_video_ops = {
 	.s_stream = ov7251_s_stream,
 };
@@ -1487,6 +1493,7 @@ static const struct v4l2_subdev_pad_ops ov7251_subdev_pad_ops = {
 };
 
 static const struct v4l2_subdev_ops ov7251_subdev_ops = {
+	.core = &ov7251_core_ops,
 	.video = &ov7251_video_ops,
 	.pad = &ov7251_subdev_pad_ops,
 };
@@ -1717,7 +1724,8 @@ static int ov7251_probe(struct i2c_client *client)
 
 	v4l2_i2c_subdev_init(&ov7251->sd, client, &ov7251_subdev_ops);
 	ov7251->sd.internal_ops = &ov7251_internal_ops;
-	ov7251->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	ov7251->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
+			    V4L2_SUBDEV_FL_HAS_EVENTS;
 	ov7251->pad.flags = MEDIA_PAD_FL_SOURCE;
 	ov7251->sd.dev = &client->dev;
 	ov7251->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
-- 
2.53.0

Re: [PATCH 3/7] media: i2c: ov7251: Add control event support

[Sakari Ailus]

Hi Antoine,

On Mon, Oct 26, 2026 at 08:10:28PM +0900, Antoine Pichot wrote:
> The driver exposes writable controls on its subdev node but does not
> support control events, so VIDIOC_SUBSCRIBE_EVENT fails and
> v4l2-compliance reports:
> 
>   fail: v4l2-test-controls.cpp(1128): subscribe event for control
>   'User Controls' failed
>   test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL
> 
> Set V4L2_SUBDEV_FL_HAS_EVENTS and use the standard control event
> helpers, as other sensor drivers do.
> 
> Signed-off-by: Antoine Pichot <antoine.pichot@gmail.com>

Instead of adding explicit support for events, could you instead make the
driver to use sub-device state? See commit
e8a5b1df000e1a23c04dadbe99621441e0889f88 for an example.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 3/7] media: i2c: ov7251: Add control event support

[Antoine Pichot]

Hi Sakari,

On Mon, Oct 26, 2026 at 01:34:53PM +0200, Sakari Ailus wrote:
> Instead of adding explicit support for events, could you instead make the
> driver to use sub-device state? See commit
> e8a5b1df000e1a23c04dadbe99621441e0889f88 for an example.

Sure. In v2, patch 3 converts the driver to the sub-device active state,
following ov5645 and the imx219 commit you pointed to. I'll wait a few
days for other comments before sending v2.

Thanks,
Antoine

[PATCH 4/7] media: i2c: ov7251: Add orientation and rotation controls

[Antoine Pichot]

Parse the firmware device properties and register the
V4L2_CID_CAMERA_ORIENTATION and V4L2_CID_CAMERA_SENSOR_ROTATION
controls, as documented in
Documentation/driver-api/media/camera-sensor.rst. libcamera uses them to
place the camera and to correct the image orientation.

While at it, bump the control handler size hint to the number of
controls the driver now creates.

On the Microsoft Surface Book 2 the ACPI _PLD (via ipu-bridge) reports
the IR sensor as front facing with no rotation.

Signed-off-by: Antoine Pichot <antoine.pichot@gmail.com>
---
 drivers/media/i2c/ov7251.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index 917285a62d..b6579a56a5 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -1591,11 +1591,17 @@ static int ov7251_detect_chip(struct ov7251 *ov7251)
 
 static int ov7251_init_ctrls(struct ov7251 *ov7251)
 {
+	struct v4l2_fwnode_device_properties props;
 	int vblank_max, vblank_def;
 	s64 pixel_rate;
 	int hblank;
+	int ret;
 
-	v4l2_ctrl_handler_init(&ov7251->ctrls, 7);
+	ret = v4l2_fwnode_device_parse(ov7251->dev, &props);
+	if (ret)
+		return ret;
+
+	v4l2_ctrl_handler_init(&ov7251->ctrls, 11);
 	ov7251->ctrls.lock = &ov7251->lock;
 
 	v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
@@ -1642,6 +1648,9 @@ static int ov7251_init_ctrls(struct ov7251 *ov7251)
 					   OV7251_TIMING_MIN_VTS, vblank_max, 1,
 					   vblank_def);
 
+	v4l2_ctrl_new_fwnode_properties(&ov7251->ctrls, &ov7251_ctrl_ops,
+					&props);
+
 	ov7251->sd.ctrl_handler = &ov7251->ctrls;
 
 	if (ov7251->ctrls.error) {
-- 
2.53.0

[PATCH 5/7] media: i2c: ov7251: Move the strobe registers out of the mode tables

[Antoine Pichot]

The three mode tables write the same values to the STROBE pad output
enable bit (0x3005) and to the strobe control block (0x3b80-0x3b96).
Move these writes to a separate register array written right after the
mode table at stream start, and name the registers a following patch
needs: SC_REG5 (0x3005, whose bit 3 enables the STROBE pad output), the
strobe frame pattern (0x3b81) and the 32-bit strobe frame span (0x3b8c).

The values written are unchanged, only the order of the writes within
the stream start sequence changes. No functional change intended.

Signed-off-by: Antoine Pichot <antoine.pichot@gmail.com>
---
 drivers/media/i2c/ov7251.c | 93 ++++++++++++++------------------------
 1 file changed, 33 insertions(+), 60 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index b6579a56a5..ad66422d9e 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -53,6 +53,9 @@
 #define OV7251_PLL2_DIVIDER_REG		0x309d
 #define OV7251_PLL2_SYS_DIV_REG		0x309a
 #define OV7251_PLL2_ADC_DIV_REG		0x309b
+#define OV7251_SC_REG5			0x3005
+#define OV7251_STROBE_FRAME_PATTERN	0x3b81
+#define OV7251_STROBE_FRAME_SPAN	0x3b8c
 
 #define OV7251_NATIVE_WIDTH		656
 #define OV7251_NATIVE_HEIGHT		496
@@ -240,8 +243,31 @@ static const struct reg_value ov7251_global_init_setting[] = {
 	{ 0x303b, 0x02 },
 };
 
+/* Strobe output settings, the same for all modes */
+static const struct reg_value ov7251_strobe_regs[] = {
+	{ OV7251_SC_REG5, 0x00 },
+	{ 0x3b80, 0x00 },
+	{ OV7251_STROBE_FRAME_PATTERN, 0xa5 },
+	{ 0x3b82, 0x10 },
+	{ 0x3b83, 0x00 },
+	{ 0x3b84, 0x08 },
+	{ 0x3b85, 0x00 },
+	{ 0x3b86, 0x01 },
+	{ 0x3b87, 0x00 },
+	{ 0x3b88, 0x00 },
+	{ 0x3b89, 0x00 },
+	{ 0x3b8a, 0x00 },
+	{ 0x3b8b, 0x05 },
+	{ OV7251_STROBE_FRAME_SPAN, 0x00 },
+	{ 0x3b8d, 0x00 },
+	{ 0x3b8e, 0x00 },
+	{ 0x3b8f, 0x1a },
+	{ 0x3b94, 0x05 },
+	{ 0x3b95, 0xf2 },
+	{ 0x3b96, 0x40 },
+};
+
 static const struct reg_value ov7251_setting_vga_30fps[] = {
-	{ 0x3005, 0x00 },
 	{ 0x3012, 0xc0 },
 	{ 0x3013, 0xd2 },
 	{ 0x3014, 0x04 },
@@ -314,25 +340,6 @@ static const struct reg_value ov7251_setting_vga_30fps[] = {
 	{ 0x3834, 0x00 },
 	{ 0x3835, 0x0c },
 	{ 0x3837, 0x00 },
-	{ 0x3b80, 0x00 },
-	{ 0x3b81, 0xa5 },
-	{ 0x3b82, 0x10 },
-	{ 0x3b83, 0x00 },
-	{ 0x3b84, 0x08 },
-	{ 0x3b85, 0x00 },
-	{ 0x3b86, 0x01 },
-	{ 0x3b87, 0x00 },
-	{ 0x3b88, 0x00 },
-	{ 0x3b89, 0x00 },
-	{ 0x3b8a, 0x00 },
-	{ 0x3b8b, 0x05 },
-	{ 0x3b8c, 0x00 },
-	{ 0x3b8d, 0x00 },
-	{ 0x3b8e, 0x00 },
-	{ 0x3b8f, 0x1a },
-	{ 0x3b94, 0x05 },
-	{ 0x3b95, 0xf2 },
-	{ 0x3b96, 0x40 },
 	{ 0x3c00, 0x89 },
 	{ 0x3c01, 0x63 },
 	{ 0x3c02, 0x01 },
@@ -369,7 +376,6 @@ static const struct reg_value ov7251_setting_vga_30fps[] = {
 };
 
 static const struct reg_value ov7251_setting_vga_60fps[] = {
-	{ 0x3005, 0x00 },
 	{ 0x3012, 0xc0 },
 	{ 0x3013, 0xd2 },
 	{ 0x3014, 0x04 },
@@ -442,25 +448,6 @@ static const struct reg_value ov7251_setting_vga_60fps[] = {
 	{ 0x3834, 0x00 },
 	{ 0x3835, 0x0c },
 	{ 0x3837, 0x00 },
-	{ 0x3b80, 0x00 },
-	{ 0x3b81, 0xa5 },
-	{ 0x3b82, 0x10 },
-	{ 0x3b83, 0x00 },
-	{ 0x3b84, 0x08 },
-	{ 0x3b85, 0x00 },
-	{ 0x3b86, 0x01 },
-	{ 0x3b87, 0x00 },
-	{ 0x3b88, 0x00 },
-	{ 0x3b89, 0x00 },
-	{ 0x3b8a, 0x00 },
-	{ 0x3b8b, 0x05 },
-	{ 0x3b8c, 0x00 },
-	{ 0x3b8d, 0x00 },
-	{ 0x3b8e, 0x00 },
-	{ 0x3b8f, 0x1a },
-	{ 0x3b94, 0x05 },
-	{ 0x3b95, 0xf2 },
-	{ 0x3b96, 0x40 },
 	{ 0x3c00, 0x89 },
 	{ 0x3c01, 0x63 },
 	{ 0x3c02, 0x01 },
@@ -497,7 +484,6 @@ static const struct reg_value ov7251_setting_vga_60fps[] = {
 };
 
 static const struct reg_value ov7251_setting_vga_90fps[] = {
-	{ 0x3005, 0x00 },
 	{ 0x3012, 0xc0 },
 	{ 0x3013, 0xd2 },
 	{ 0x3014, 0x04 },
@@ -570,25 +556,6 @@ static const struct reg_value ov7251_setting_vga_90fps[] = {
 	{ 0x3834, 0x00 },
 	{ 0x3835, 0x0c },
 	{ 0x3837, 0x00 },
-	{ 0x3b80, 0x00 },
-	{ 0x3b81, 0xa5 },
-	{ 0x3b82, 0x10 },
-	{ 0x3b83, 0x00 },
-	{ 0x3b84, 0x08 },
-	{ 0x3b85, 0x00 },
-	{ 0x3b86, 0x01 },
-	{ 0x3b87, 0x00 },
-	{ 0x3b88, 0x00 },
-	{ 0x3b89, 0x00 },
-	{ 0x3b8a, 0x00 },
-	{ 0x3b8b, 0x05 },
-	{ 0x3b8c, 0x00 },
-	{ 0x3b8d, 0x00 },
-	{ 0x3b8e, 0x00 },
-	{ 0x3b8f, 0x1a },
-	{ 0x3b94, 0x05 },
-	{ 0x3b95, 0xf2 },
-	{ 0x3b96, 0x40 },
 	{ 0x3c00, 0x89 },
 	{ 0x3c01, 0x63 },
 	{ 0x3c02, 0x01 },
@@ -1369,6 +1336,12 @@ static int ov7251_s_stream(struct v4l2_subdev *subdev, int enable)
 				ov7251->current_mode->height);
 			goto err_power_down;
 		}
+		ret = ov7251_set_register_array(ov7251, ov7251_strobe_regs,
+						ARRAY_SIZE(ov7251_strobe_regs));
+		if (ret < 0) {
+			dev_err(ov7251->dev, "could not set strobe defaults\n");
+			goto err_power_down;
+		}
 		ret = __v4l2_ctrl_handler_setup(&ov7251->ctrls);
 		if (ret < 0) {
 			dev_err(ov7251->dev, "could not sync v4l2 controls\n");
-- 
2.53.0

[PATCH 6/7] media: i2c: ov7251: Add strobe output enable control

[Antoine Pichot]

The OV7251 has a STROBE output that pulses during the exposure of
selected frames, meant to drive an illuminator. On the Microsoft
Surface Book 2 the front OV7251 is an infrared camera (Windows Hello)
and its 850 nm emitter is wired to that pin: ACPI describes no separate
LED, GPIO line or regulator for it. The mode tables leave the pad
disabled, so the camera only sees an unlit scene.

Add a V4L2_CID_FLASH_STROBE_OE control, off by default. When enabled
it sets the STROBE pad output enable (SC_REG5 bit 3), strobes
alternate frames (frame pattern 0xaa instead of the default 0xa5, which
lights four frames in eight) and sets the
pulse length (strobe frame span, in lines) to the exposure time,
clamped between 52 lines and 45% of the frame length, never more than
776 lines. When disabled it
restores the defaults the mode tables used to write. The span follows
exposure and vertical blanking changes while the strobe is enabled.

The limits come from the Windows driver, which clamps the span to 52
to 776 lines: 776 lines is 45% of the 1724-line frame of the 30 fps
mode. Expressing the upper limit as a duty cycle keeps the emitter
within that envelope (eye safety) at 60 and 90 fps too, where a fixed
776 lines would let the pulse cover up to 96% of the frame (552 of 572
lines). Keeping the 776-line limit as well bounds the pulse when
vertical blanking stretches the frame, up to 65535 lines.

Alternate frames is what the Windows driver uses for face recognition
on this hardware: its 30 and 60 fps modes write 0xaa, and a register
dump taken during a Windows Hello sign-in on a Surface Book 2 shows the
same. Lighting every frame (0xff, which two of its other modes use)
would double the average emitter power. User space tells lit frames
from dark ones by their brightness; V4L2 has no per-frame illumination
flag yet.

The control is written only while the sensor is powered, and from
__v4l2_ctrl_handler_setup() at stream start, after the mode and strobe
register tables. Setting it while the sensor is idle does not power it
up, and the emitter goes off with the sensor when streaming stops.

On a Surface Book 2 at analogue gain 128 the mean pixel value of the
infrared image of a room is 19 unlit; with the strobe enabled the lit
frames read 496 at exposure 504 (span 504) and 587 at exposure 1704
(span 776), the dark frames in between 18 to 20.
The span read back while streaming is 258 at 90 fps, 45% of the
572-line frame.

Signed-off-by: Antoine Pichot <antoine.pichot@gmail.com>
---
 drivers/media/i2c/ov7251.c | 85 ++++++++++++++++++++++++++++++++++----
 1 file changed, 76 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index ad66422d9e..de650b4457 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -18,6 +18,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/unaligned.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-event.h>
 #include <media/v4l2-fwnode.h>
@@ -54,8 +55,15 @@
 #define OV7251_PLL2_SYS_DIV_REG		0x309a
 #define OV7251_PLL2_ADC_DIV_REG		0x309b
 #define OV7251_SC_REG5			0x3005
+#define OV7251_SC_REG5_STROBE_OE	BIT(3)
 #define OV7251_STROBE_FRAME_PATTERN	0x3b81
+#define OV7251_STROBE_FRAME_PATTERN_ALT	0xaa	/* alternate frames */
+#define OV7251_STROBE_FRAME_PATTERN_DEF	0xa5
 #define OV7251_STROBE_FRAME_SPAN	0x3b8c
+#define OV7251_STROBE_FRAME_SPAN_DEF	0x1a
+#define OV7251_STROBE_SPAN_MIN		52
+#define OV7251_STROBE_SPAN_MAX		776
+#define OV7251_STROBE_MAX_DUTY_PERCENT	45
 
 #define OV7251_NATIVE_WIDTH		656
 #define OV7251_NATIVE_HEIGHT		496
@@ -151,6 +159,7 @@ struct ov7251 {
 	struct v4l2_ctrl *gain;
 	struct v4l2_ctrl *hblank;
 	struct v4l2_ctrl *vblank;
+	struct v4l2_ctrl *strobe;
 
 	/* Cached register values */
 	u8 aec_pk_manual;
@@ -243,11 +252,12 @@ static const struct reg_value ov7251_global_init_setting[] = {
 	{ 0x303b, 0x02 },
 };
 
-/* Strobe output settings, the same for all modes */
+/*
+ * Strobe output settings, the same for all modes. The STROBE_OE control
+ * writes SC_REG5, the frame pattern and the frame span.
+ */
 static const struct reg_value ov7251_strobe_regs[] = {
-	{ OV7251_SC_REG5, 0x00 },
 	{ 0x3b80, 0x00 },
-	{ OV7251_STROBE_FRAME_PATTERN, 0xa5 },
 	{ 0x3b82, 0x10 },
 	{ 0x3b83, 0x00 },
 	{ 0x3b84, 0x08 },
@@ -258,10 +268,6 @@ static const struct reg_value ov7251_strobe_regs[] = {
 	{ 0x3b89, 0x00 },
 	{ 0x3b8a, 0x00 },
 	{ 0x3b8b, 0x05 },
-	{ OV7251_STROBE_FRAME_SPAN, 0x00 },
-	{ 0x3b8d, 0x00 },
-	{ 0x3b8e, 0x00 },
-	{ 0x3b8f, 0x1a },
 	{ 0x3b94, 0x05 },
 	{ 0x3b95, 0xf2 },
 	{ 0x3b96, 0x40 },
@@ -724,7 +730,7 @@ static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val)
 static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val,
 				 u8 num)
 {
-	u8 regbuf[5];
+	u8 regbuf[6];
 	u8 nregbuf = sizeof(reg) + num * sizeof(*val);
 	int ret = 0;
 
@@ -976,6 +982,56 @@ static int ov7251_set_test_pattern(struct ov7251 *ov7251, s32 value)
 	return ret;
 }
 
+static int ov7251_set_strobe_span(struct ov7251 *ov7251)
+{
+	u32 span = OV7251_STROBE_FRAME_SPAN_DEF;
+	u32 vts, max_span;
+	u8 val[4];
+
+	/*
+	 * The pulse starts with the integration: make it cover the exposure,
+	 * from 52 lines up to 45% of the frame and never more than 776 lines.
+	 * The vendor driver caps it at 776 lines, 45% of the 1724 lines 30 fps
+	 * frame; the duty cycle limit keeps the emitter within that envelope
+	 * at higher frame rates, the absolute one with long vertical blanking.
+	 */
+	if (ov7251->strobe->val) {
+		vts = ov7251->current_mode->height + ov7251->vblank->val;
+		max_span = DIV_ROUND_UP(vts * OV7251_STROBE_MAX_DUTY_PERCENT,
+					100);
+		max_span = min(max_span, OV7251_STROBE_SPAN_MAX);
+		span = clamp_t(u32, ov7251->exposure->val,
+			       OV7251_STROBE_SPAN_MIN, max_span);
+	}
+
+	put_unaligned_be32(span, val);
+
+	return ov7251_write_seq_regs(ov7251, OV7251_STROBE_FRAME_SPAN, val,
+				     sizeof(val));
+}
+
+static int ov7251_set_strobe(struct ov7251 *ov7251, s32 value)
+{
+	int ret;
+
+	/*
+	 * The other SC_REG5 bits enable other pads, which the mode tables
+	 * have always left disabled (0x00).
+	 */
+	ret = ov7251_write_reg(ov7251, OV7251_SC_REG5,
+			       value ? OV7251_SC_REG5_STROBE_OE : 0);
+	if (ret)
+		return ret;
+
+	ret = ov7251_write_reg(ov7251, OV7251_STROBE_FRAME_PATTERN,
+			       value ? OV7251_STROBE_FRAME_PATTERN_ALT :
+				       OV7251_STROBE_FRAME_PATTERN_DEF);
+	if (ret)
+		return ret;
+
+	return ov7251_set_strobe_span(ov7251);
+}
+
 static const char * const ov7251_test_pattern_menu[] = {
 	"Disabled",
 	"Vertical Pattern Bars",
@@ -1019,6 +1075,8 @@ static int ov7251_s_ctrl(struct v4l2_ctrl *ctrl)
 	switch (ctrl->id) {
 	case V4L2_CID_EXPOSURE:
 		ret = ov7251_set_exposure(ov7251, ctrl->val);
+		if (!ret && ov7251->strobe->val)
+			ret = ov7251_set_strobe_span(ov7251);
 		break;
 	case V4L2_CID_GAIN:
 		ret = ov7251_set_gain(ov7251, ctrl->val);
@@ -1034,6 +1092,11 @@ static int ov7251_s_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	case V4L2_CID_VBLANK:
 		ret = ov7251_vts_configure(ov7251, ctrl->val);
+		if (!ret && ov7251->strobe->val)
+			ret = ov7251_set_strobe_span(ov7251);
+		break;
+	case V4L2_CID_FLASH_STROBE_OE:
+		ret = ov7251_set_strobe(ov7251, ctrl->val);
 		break;
 	default:
 		ret = -EINVAL;
@@ -1574,7 +1637,7 @@ static int ov7251_init_ctrls(struct ov7251 *ov7251)
 	if (ret)
 		return ret;
 
-	v4l2_ctrl_handler_init(&ov7251->ctrls, 11);
+	v4l2_ctrl_handler_init(&ov7251->ctrls, 12);
 	ov7251->ctrls.lock = &ov7251->lock;
 
 	v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
@@ -1621,6 +1684,10 @@ static int ov7251_init_ctrls(struct ov7251 *ov7251)
 					   OV7251_TIMING_MIN_VTS, vblank_max, 1,
 					   vblank_def);
 
+	ov7251->strobe = v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
+					   V4L2_CID_FLASH_STROBE_OE,
+					   0, 1, 1, 0);
+
 	v4l2_ctrl_new_fwnode_properties(&ov7251->ctrls, &ov7251_ctrl_ops,
 					&props);
 
-- 
2.53.0

Re: [PATCH 6/7] media: i2c: ov7251: Add strobe output enable control

[Andy Shevchenko]

On Mon, Oct 26, 2026 at 08:10:31PM +0900, Antoine Pichot wrote:
> The OV7251 has a STROBE output that pulses during the exposure of
> selected frames, meant to drive an illuminator. On the Microsoft
> Surface Book 2 the front OV7251 is an infrared camera (Windows Hello)
> and its 850 nm emitter is wired to that pin: ACPI describes no separate
> LED, GPIO line or regulator for it. The mode tables leave the pad
> disabled, so the camera only sees an unlit scene.
> 
> Add a V4L2_CID_FLASH_STROBE_OE control, off by default. When enabled
> it sets the STROBE pad output enable (SC_REG5 bit 3), strobes
> alternate frames (frame pattern 0xaa instead of the default 0xa5, which
> lights four frames in eight) and sets the
> pulse length (strobe frame span, in lines) to the exposure time,
> clamped between 52 lines and 45% of the frame length, never more than
> 776 lines. When disabled it
> restores the defaults the mode tables used to write. The span follows
> exposure and vertical blanking changes while the strobe is enabled.
> 
> The limits come from the Windows driver, which clamps the span to 52
> to 776 lines: 776 lines is 45% of the 1724-line frame of the 30 fps
> mode. Expressing the upper limit as a duty cycle keeps the emitter
> within that envelope (eye safety) at 60 and 90 fps too, where a fixed
> 776 lines would let the pulse cover up to 96% of the frame (552 of 572
> lines). Keeping the 776-line limit as well bounds the pulse when
> vertical blanking stretches the frame, up to 65535 lines.
> 
> Alternate frames is what the Windows driver uses for face recognition
> on this hardware: its 30 and 60 fps modes write 0xaa, and a register
> dump taken during a Windows Hello sign-in on a Surface Book 2 shows the
> same. Lighting every frame (0xff, which two of its other modes use)
> would double the average emitter power. User space tells lit frames
> from dark ones by their brightness; V4L2 has no per-frame illumination
> flag yet.
> 
> The control is written only while the sensor is powered, and from
> __v4l2_ctrl_handler_setup() at stream start, after the mode and strobe
> register tables. Setting it while the sensor is idle does not power it
> up, and the emitter goes off with the sensor when streaming stops.
> 
> On a Surface Book 2 at analogue gain 128 the mean pixel value of the
> infrared image of a room is 19 unlit; with the strobe enabled the lit
> frames read 496 at exposure 504 (span 504) and 587 at exposure 1704
> (span 776), the dark frames in between 18 to 20.
> The span read back while streaming is 258 at 90 fps, 45% of the
> 572-line frame.

...

> +static int ov7251_set_strobe_span(struct ov7251 *ov7251)
> +{
> +	u32 span = OV7251_STROBE_FRAME_SPAN_DEF;
> +	u32 vts, max_span;
> +	u8 val[4];

> +	/*
> +	 * The pulse starts with the integration: make it cover the exposure,
> +	 * from 52 lines up to 45% of the frame and never more than 776 lines.
> +	 * The vendor driver caps it at 776 lines, 45% of the 1724 lines 30 fps
> +	 * frame; the duty cycle limit keeps the emitter within that envelope
> +	 * at higher frame rates, the absolute one with long vertical blanking.
> +	 */
> +	if (ov7251->strobe->val) {
> +		vts = ov7251->current_mode->height + ov7251->vblank->val;
> +		max_span = DIV_ROUND_UP(vts * OV7251_STROBE_MAX_DUTY_PERCENT,
> +					100);

units.h provides PERCENT constant.

		max_span = DIV_ROUND_UP(vts * OV7251_STROBE_MAX_DUTY_PERCENT,
					PERCENT);

> +		max_span = min(max_span, OV7251_STROBE_SPAN_MAX);
> +		span = clamp_t(u32, ov7251->exposure->val,
> +			       OV7251_STROBE_SPAN_MIN, max_span);

Can we avoid using _t version of clamp()?

And naturally I would first clamp over the _MAX/_MIN range (this addresses the
previous Q) and then run min() on top.

> +	}
> +
> +	put_unaligned_be32(span, val);
> +
> +	return ov7251_write_seq_regs(ov7251, OV7251_STROBE_FRAME_SPAN, val,
> +				     sizeof(val));

Can we move at some point to CCI accessors (v4l2-cci.h)?

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/7] media: i2c: ov7251: Add strobe output enable control

[Antoine Pichot]

Hi Andy,

Thanks Andy for your comments. The first two are fixed in v2; I'll
take care of the CCI accessors in a follow-up once this is in. I'll
send v2 in a few days to give everyone time to comment.

Cheers,
Antoine

[PATCH 7/7] media: ipu-bridge: Add a 90 degree rotation quirk for Surface Book 2

[Antoine Pichot]

The infrared camera of the Microsoft Surface Book 2, an OmniVision
OV7251 with ACPI ID INT347E, is mounted rotated by 90 degrees. An
upright scene appears rotated 90 degrees clockwise in its frames, with
the top of the scene at the right edge of the image, while a frame
taken at the same moment by the colour camera next to it shows the
scene upright. The degree field of the sensor's SSDB buffer in the ACPI
tables is 0, and so is the rotation field of its _PLD, so
ipu_bridge_parse_rotation() sets the rotation property to 0 and
userspace has no way to know that the image needs correcting.

The firmware is wrong here, and ipu-bridge is where the firmware data
is turned into the rotation property: it already carries DMI (Desktop
Management Interface) quirks for sensors mounted upside down without
the firmware saying so. Add a second table for sensors rotated by 90
degrees, checked the same way, with one entry for the Surface Book 2
infrared sensor. Following V4L2_CID_CAMERA_SENSOR_ROTATION, 90 is the
counter-clockwise rotation to apply to correct the image. The entry
matches on the SKU of the 13 inch model, the one checked; the 15 inch
model has its own SKU and has not been checked. The colour sensors of
this machine (INT33BE and INT347A) already report 180 degrees and are
unchanged.

Tested on a 13 inch Surface Book 2 (SKU Surface_Book_1832) with a
7.2.12 kernel: V4L2_CID_CAMERA_SENSOR_ROTATION on the OV7251 reads 90
instead of 0, the colour sensors still read 180 and the orientation
controls are unchanged.

Signed-off-by: Antoine Pichot <antoine.pichot@gmail.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 3739c4a2a8..51ed4283ce 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -281,6 +281,23 @@ static const struct dmi_system_id upside_down_sensor_dmi_ids[] = {
 	{} /* Terminating entry */
 };
 
+/*
+ * DMI matches for laptops which have their sensor mounted rotated by 90 degrees
+ * without reporting it in the SSDB.
+ */
+static const struct dmi_system_id rotated_90_sensor_dmi_ids[] = {
+	{
+		/* Microsoft Surface Book 2 13 inch, infrared sensor */
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR,
+					"Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Book_1832"),
+		},
+		.driver_data = "INT347E",
+	},
+	{} /* Terminating entry */
+};
+
 static const struct ipu_property_names prop_names = {
 	.clock_frequency = "clock-frequency",
 	.rotation = "rotation",
@@ -474,6 +491,11 @@ static u32 ipu_bridge_parse_rotation(struct acpi_device *adev,
 		if (acpi_dev_hid_match(adev, dmi_id->driver_data))
 			return 180;
 
+	for (dmi_id = dmi_first_match(rotated_90_sensor_dmi_ids); dmi_id;
+	     dmi_id = dmi_first_match(dmi_id + 1))
+		if (acpi_dev_hid_match(adev, dmi_id->driver_data))
+			return 90;
+
 	switch (ssdb->degree) {
 	case IPU_SENSOR_ROTATION_NORMAL:
 		return 0;
-- 
2.56.0

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

[D. Manresa]

Hi Antoine,

I ran the series on a Surface Pro 7+ (IPU6 Tiger Lake, 8086:9a19), the
front-facing OV7251 at i2c 3-0060, on a 6.19-based kernel. Patches 2 to 6
all do what they say here, so:

Tested-by: D. Manresa <dmanresa@gmail.com> # Surface Pro 7+, IPU6 (8086:9a19); patches 2-6 only

I have deliberately scoped that tag. 1/7 is a regression on this machine
and I have replied separately on that patch; please do not read the tag as
covering it. 7/7 is keyed on DMI_PRODUCT_SKU "Surface_Book_1832" and this
machine reports "Surface_Pro_7+_1960", so I did not build or test it.

What each one did here:

2/7: reproduced the bug and the fix. Unpatched, --set-subdev-fps is
accepted and reads back, but the VTS register stays at 1724 and the sensor
keeps delivering 30 fps. With the patch:

    requested   vblank   VTS reg   measured
    90 fps      92       572       90.45 fps
    60 fps      380      860       60.16 fps
    30 fps      1244     1724      30.01 fps

3/7: v4l2-compliance on the subdev goes from

    fail: subscribe event for control 'User Controls' failed
    Total: 44, Succeeded: 43, Failed: 1

to 44 of 44 with no failures.

4/7: camera_orientation reads Front and camera_sensor_rotation 0, both
read-only, consistent with the _PLD here.

5/7: no functional change observed. With the new control off, the
registers read exactly what the mode tables used to write: 0x3005 = 0x00,
0x3b81 = 0xa5, span 26.

6/7: works, and the register behaviour matches your description exactly.
The control appears as strobe_output_enable and defaults to 0. Read back
over i2c mid-stream:

    off:  0x3005 = 0x00   0x3b81 = 0xa5   span 26
    on:   0x3005 = 0x08   0x3b81 = 0xaa   span clamp(exposure, 52,
                                               min(45% of VTS, 776))

Both clamp limits are exact over a 12-point exposure sweep from 1 to 1704
at VTS 1724, and the duty rule holds as VTS varies: 1724 -> 776, 1280 ->
576, 880 -> 396, 680 -> 306, 572 -> 258. That last one reproduces the 258
at 90 fps from your cover letter.

For the illuminator itself I did not trust anything but the sensor: one
568-frame capture, strobe toggled 0/1/0/1/0 inside the single stream,
exposure 776, VTS 1724, gain 16, same scene, zero CSI-2 errors, per-frame
mean of the Y10 data:

    strobe 0:  n=252  mean 517.58  std 0.25   |even-odd| 0.00-0.01
    strobe 1:  n=169  mean 546.27  std 28.81  |even-odd| 57.61
               lit frames 575.4   unlit frames 517.6

With the strobe off there is no per-frame structure at all; with it on the
frames split into two populations 57.6 counts apart, alternating, and the
unlit frames inside an ON segment match the strobe-off level to within 0.3
counts. That is the illuminator firing on alternate frames and nothing
else. The effect is only +5.5% overall because the scene had a lot of
ambient daylight IR; every number above is a within-capture comparison, as
absolute levels drifted between separate captures.

Two notes that might save someone else time.

6/7 does not build on a tree without V4L2_CID_FLASH_STROBE_OE:

  ov7251.c:1688:44: error: 'V4L2_CID_FLASH_STROBE_OE' undeclared

That CID is not added by this series -- it comes from 5be4154f6255
("media: v4l: ctrls: add a control for enabling strobe output"), which
landed after v6.19. Since 6/7 uses v4l2_ctrl_new_std() it needs the
v4l2-ctrls-defs.c entries too, not just the uapi define, or the control is
never created. Worth a word in the cover letter for anyone testing on a
stable tree.

And on this hardware the series supersedes the out-of-tree Surface
driver's ir_led module parameter, which drives the same two registers.
Measured on the shipped module while streaming, that parameter lights
every frame with the mode table's 26-line pulse, about 0.7% duty at its
production VTS of 3448, and never updates the span. 6/7 is strictly better
behaved. They cannot coexist, since 5/7 removes those writes from the mode
tables, so ir_led would have to go when this lands. That is our problem,
not yours.

What I did not test: 7/7, the 60 and 90 fps modes photometrically,
v4l2-compliance streaming, and whether the pulse follows the exposure in
time -- I verified the programmed span, not the pulse position. The
photometric capture was taken with 1/7 reverted, since with it the link
here is too unstable to capture reliably; the register-level strobe
behaviour was re-verified identically with 1 to 6 exactly as posted.

I also had to rebase onto v6.19 to build at all, because mainline's
ov7251.c uses pad-op signatures this kernel does not have; the unpatched
file fails the same way, so that is unrelated to your series. Patches 1 to
6 applied to v6.19 with line offsets of at most one and no fuzz.

D. Manresa

(Written with AI assistance, directed and reviewed by me.)

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

[Antoine Pichot]

Hi,

On Tue, Oct 27, 2026 at 03:26:42AM -0700, D. Manresa wrote:
> Tested-by: D. Manresa <dmanresa@gmail.com> # Surface Pro 7+, IPU6 (8086:9a19); patches 2-6 only

Thank you for the thorough test. v2 carries the tag on 2, 4 and 5.
Patch 6 changed after Andy's review (the span computation rewritten,
same result), so I will carry it there too unless you object. Patch 3
is replaced by a conversion to the sub-device active state, as Sakari
asked.

> That CID is not added by this series -- it comes from 5be4154f6255
> ("media: v4l: ctrls: add a control for enabling strobe output"), which
> landed after v6.19. Since 6/7 uses v4l2_ctrl_new_std() it needs the
> v4l2-ctrls-defs.c entries too, not just the uapi define, or the control is
> never created. Worth a word in the cover letter for anyone testing on a
> stable tree.

The v2 cover letter will say so: 6/7 needs the strobe control from
kernel 7.0 and is not for stable.

> 4/7: camera_orientation reads Front and camera_sensor_rotation 0, both
> read-only, consistent with the _PLD here.

On the Book 2 the firmware reports 0 but the picture comes out turned by
90 degrees, which 7/7 corrects for that model. Could you please check
whether the frames are upright on the Pro 7+ and, if not, tell me which
way they turn? 7/7 can then carry the Pro 7+ as well.

The link question continues on the 1/7 thread.

Thanks,
Antoine