Re: [PATCH v4 3/6] media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock

[Fil Dunsky]

Tested-by: Fil Dunsky <filipp.dunsky@xxxxxxxxx>

Scope: patches 3-6. Patch 1 was already in my tree and this machine is
INT33BE, so patches 1 and 2 are not functionally exercised here.

Hardware: Surface Pro 8, IPU6 Tiger Lake (8086:9a19), OV5693 front
sensor at INT33BE:00, OV13858 rear, VD55G0 IR. Kernel 7.2.2 plus the
linux-surface patch set, not the v7.3-rc1 base the series declares;
patch 6 needed that tree's duplicate OVTI5693 entry dropped before it
would apply.

With the series applied, streaming from the ISYS capture node:

  60 frames, SBGGR10 2592x1944, 604661760 bytes, 28.64 fps
  MIPI_CTRL00 (0x4800) read back over i2c while streaming: 0x20

0x20 is the bit-5-only value patch 3 writes, so the clock-noncontinuous
property did reach the sensor driver: the path from the table entry in
patch 4 through to the register is exercised, not merely "the camera
works".

I also ran the negative control, with PCI_DEVICE_ID_INTEL_IPU6 dropped
from the INT33BE entries and nothing else changed. How it fails is
worth recording, because the obvious test misses it:

  - the first capture after boot succeeds, 60 frames at 28.64 fps, with
    0x4800 reading 0x00;
  - every subsequent capture in that boot returns zero bytes and times
    out, with nothing in dmesg;
  - writing 0x20 to 0x4800 over i2c into a stalled stream starts frames
    immediately, reproduced on three separate streams, while clearing
    the bit again mid-stream does not stop them.

With bit 5 set, the same script captures three times in a row without
trouble; I measured that with our downstream driver, which writes 0x2d
unconditionally. So the entry is needed at stream start, and a single
capture after a reboot is not enough to tell whether it is present.

The free first capture appears to be particular to this machine: two
other testers of this series, on a Surface Pro 7+ and a Pro 9, get zero
bytes on the first attempt as well. I have not been able to explain the
difference. It only affects how the entry should be verified, not
whether it is needed.

Patch 5's precedence rule is exercised here as well: INT33BE appears
twice in the table, the generic entry and the Tiger Lake one, and the
bridge connects the sensor once - "Connected 3 cameras", no double
connect. The two sensors that take no flags, OV13858 and the VD55G0 IR
camera, are unaffected; the IR camera still does face authentication.

The teardown "stream stop time out" appears identically with and
without the series, so it is not introduced by it.

One note for out-of-tree builders: patch 4 grows struct ipu_sensor,
which moves the CRC of ipu_bridge_init() and ipu_bridge_parse_ssdb(),
so with CONFIG_MODVERSIONS ipu-bridge and intel-ipu6 have to be built
together. intel-ipu6-isys imports only ipu_bridge_instantiate_vcm,
whose CRC does not move; an unrebuilt intel-ipu6-isys loaded fine
against the new pair on 7.2.2.