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

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

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