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

[D. Manresa <dmanresa@gmail.com>]

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.)