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

[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

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

[D. Manresa]

Hi,

On Tue, Oct 27, 2026 at 10:26:37PM +0900, Antoine Pichot wrote:
> Could you please try the two patches below, the first alone and then
> both, with your 150-frame measurement?

Done. The swap fixes the link completely on this receiver. Details
below, but first a correction to my own numbers, because they are wrong
in the archive.

Correction to the dropped-buffer figures in my previous mail
------------------------------------------------------------

I read v4l2-ctl's output wrong. It prints a line roughly once a second,
and "dropped buffers: N" is the drops in that interval, not a running
total; the suffix is omitted entirely when there were none. The column I
posted was the last line of each run, so it was the drops in the final
second, not the drops in the run. Summed over the run the figures are
some fifty times larger. The CSI-2 error counts came from dmesg and are
unaffected.

With that fixed, 1/7 alone at 30 fps, three 150-frame runs:

  dropped (sum over run)   1181    388    926
  CSI-2 error lines        >=4996  >=1931 >=3785
  elapsed for 150 frames   47.0 s  18.8 s 38.4 s

A clean run of the same capture takes 5.2 s. Error totals remain lower
bounds: the printk is rate-limited and I add the "callbacks suppressed"
counts, but the suppressions of the last partial interval are never
printed.

The divider swap
----------------

Your 0001 removes the problem here. Every run: 150 of 150 frames in
5.20 to 5.24 s at 30.00 fps, zero dropped buffers, zero CSI-2 error
lines. Thirteen runs. Readback over i2c mid-stream, with 0x0100 = 0x01
confirming the sensor was streaming:

  build          30b0 30b1 30b3 30b4 30b5 4837  link_freq  pixel_rate
  unpatched      0x0a 0x01 0x4b 0x03 0x05 0x19  240000000  48000000
  1/7            0x0a 0x04 0x85 0x01 0x05 0x19  319200000  63840000
  1/7+0001       0x0a 0x01 0x85 0x04 0x05 0x19  319200000  63840000
  1/7+0001+0002  0x0a 0x01 0x85 0x04 0x05 0x1f  319200000  63840000

The only difference from 1/7 alone is the two dividers, with the link
frequency and pixel rate unchanged, so this is the VCO as you say and
not the link speed.

0002 adds nothing measurable: ten runs, the same perfect result as 0001
alone. 0x4837 = 0x1f is confirmed in the hardware, so that is a real
null and not an unapplied patch. It does no harm either.

Why I ran more than three of each
---------------------------------

Because on this machine each stream start is a lottery and three runs
cannot see it. A run is either perfectly clean or uniformly broken for
its whole duration, with nothing in between, and the unpatched driver
began failing later in the session too, so results taken in blocks are
confounded by drift.

So I interleaved: unpatched, 1/7, 1/7+0001, 1/7+0001+0002, one 150-frame
run of each per cycle, swapping the module every time, seven cycles.

  build            runs with any CSI-2 error   stalls
  unpatched                1 of 7                 1
  1/7                      4 of 7                 3
  1/7+0001                 0 of 7                 0
  1/7+0001+0002            0 of 7                 0

Pooled with the block runs: unpatched 3 bad of 13, 1/7 7 bad of 10,
1/7+0001 0 of 13, 1/7+0001+0002 0 of 10. The +0001 build was also
re-checked immediately after the worst runs of the session and was still
clean, so this is not simply a good patch of time.

One caveat for the record: this receiver intermittently fails to lock
even with a completely unpatched driver, 3 of 13 runs at 30 fps
including two 60-second stalls. The Pro 7+ has a baseline instability of
its own. 1/7 makes it far more likely; 0001 removed it entirely in this
sample.

So v2 with the PLL table fix before the index fix, both Cc: stable,
matches what I measure here. Send it and I will run it on this machine
while it is still with me.

2/7 at 60 and 90 fps, without 1/7
---------------------------------

The error count follows frame starts, not the link. The link is 240 MHz
/ 480 Mbps at all three rates (0x30b3 = 0x4b, 0x30b4 = 0x03 confirmed by
readback), and vblank follows the mode as 2/7 intends: 1244, 380, 92.

One module load, then 30, 60 and 90 fps interleaved, five cycles. That
block fell entirely in the bad regime, which is what makes the rates
comparable:

  rate    errors per frame start   errors per second   mean dropped/run
  30 fps          3.62                   104                 867
  60 fps          3.70                   204                 517
  90 fps          2.72                   222                 262

Per second the count doubles from 30 to 60 fps over an identical link.
Per frame start the three collapse onto each other, and 90 fps is about
25% lower per frame while carrying three times the payload of 30 fps.
The bad 1/7 runs at 319.2 MHz sit in the same range, 3.5 to 4.6 errors
per frame, so the per-frame cost of a bad lock is the same at both link
frequencies.

That reads as a failure to lock at stream start which then persists for
the whole stream, rather than anything accumulating with transported
bits. It is also why errors per run vary by two orders of magnitude
while errors per frame do not.

On the cover letter
-------------------

6/7: I would rather re-run it than have the tag carried across a rewrite
I did not test. Send me the v2 version and I will repeat the clamp sweep
and the photometry while the machine is still here. If v2 goes out
before that, carry it, since you say the result is unchanged, but I
would rather earn it.

Thank you for the pixel_rate explanation. I had taken 63840000 at face
value.

7/7: I will answer on the orientation in a separate mail.

D. Manresa

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

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

[Dan Scally]

Hello

On 27/10/2026 10:26, D. Manresa wrote:

<snip>

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

It would be worth validating the settings for the 319.2MHz link frequency to make sure that the PLL 
is being configured correctly. The link frequency itself advertised by the ipu-bridge strictly just 
needs to be one that the sensor driver is capable of configuring, but ideally we would make sure 
that the sensor can configure one that these platforms tend to use, as the platform designers will 
generally choose it carefully to be appropriate for their device. When I made these changes I 
checked and the driver for my Surface Go 2 was setting 319.2 MHz, so I would be more suspicious of 
the driver's PLL settings than that value.

Dan

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

[Dave Stevenson]

Hi Antoine and D. Manresa

On Tue, 27 Oct 2026 at 10:27, D. Manresa <dmanresa@gmail.com> wrote:
>
> 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.

I'll agree that the link frequency isn't abided by, and your patch
does fix that. However testing on a DT based system (Raspberry Pi)
with a 24MHz extclk I'm also seeing issues.

I'm working with the datasheet from
https://static6.arrow.com/aropdfconversion/dedba0a4f2b52e9b6587341a0e31cd115a2c3a46/ov7750_ov7251_csp3_rev1f_ds_2.12.pdf

As I read it, the pixelrate is configured via PLL2. Those settings are
identical between the 2 link frequencies, so the pixel rate should be
48000000 in both cases. Without that I was getting 75fps from
libcamera when I asked for 100fps. 100 * 480 / 638.4 = 75.1fps.
The settings for PLL2 with a 24MHz extclk exactly follow the sample
PLL configuration in table 2.10 of the datasheet. For 24MHz the
multiplier is 0x28 / 40dec vs 0x32 / 50dec for 19.2MHz, resulting in
the same rate.

The settings for PLL1 seem to be all over the shop if the datasheet is correct.

With a 24MHz extclk and measuring with an oscilloscope, the "240MHz
link freq" actually runs at 400MHz. The "319.2MHz link frequency" runs
at 454MHz.
If I follow the datasheet, then the 240MHz mode does fall out at 400MHz
For the 319.2MHz mode I reckon that the VCO is well out of spec -
2.128GHz when the limit is 800MHz.

The numbers for the 19.2MHz modes look sane in their ratios, but I
can't test them.
The 240MHz link freq does a prediv of 3, multiply by 75, and divider
of 1, which keeps things in range, and 19.2 / 3 * 75 / 1 = 480MPix/s,
or 240MHz.
The 319.2MHz link frequency uses a prediv of 1 and multiply by 0x85 /
133 gives a VCO frequency of 2.55GHz, when again the limit is 800MHz.
That then gets divided by 4 afterwards for a supposed result of
638.4MPix/s or 319.2MHz.

@D. Manresa it'd be worth trying using a prediv instead of (post)div:
static const struct ov7251_pll1_cfg ov7251_pll1_cfg_19_2_mhz_319_2_mhz = {
    .pre_div = 0x04,
    .mult = 0x85,
    .div = 0x01,
    .pix_div = 0x0a,
    .mipi_div = 0x05,
};

So I'll agree the patch is correct, but it'll show up other failures
that were currently masked. I'm not fussed about the 24MHz mode, but
it sounds like there are devices using the 19.2MHz / 319.2MHz mode.

(I've also noted that the driver allows V4L2_CID_VBLANK to go down to
1, when 40 or below seem to give corrupt images for me. Implementing
set_frame_interval for a raw sensor is also wrong, particularly when
V4L2_CID_VBLANK is also implemented. I'll see if I can find some time
to patch it up).

Hope that helps.
  Dave

> 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

[D. Manresa]

Hi Dave,

On Tue, 27 Oct 2026 at 16:50, Dave Stevenson wrote:
> @D. Manresa it'd be worth trying using a prediv instead of (post)div:
>     .pre_div = 0x04,
>     .mult = 0x85,
>     .div = 0x01,

Our mails crossed, and that is exactly the change I had just finished
measuring, from a test patch Antoine sent me. It fixes the link on this
receiver: thirteen 150-frame runs at 30 fps, every one 150 of 150 in
5.2 s with zero dropped buffers and zero CSI-2 error lines, against
seven bad runs out of ten with the index fix alone, three of them hard
stalls. i2c readback mid-stream confirms 0x30b4 = 0x04 and 0x30b1 =
0x01 with link_frequency still 319200000, so the link rate is unchanged
and only the VCO moved. Full numbers are in my other mail on this
thread, which went out a few minutes after yours arrived and does not
mention you only because I had not yet seen it.

So the 19.2 MHz / 319.2 MHz mode you could not test behaves exactly as
your 24 MHz oscilloscope measurements predict.

Dan's instinct in the parallel branch was right too: it was the
driver's PLL settings and not the value ipu-bridge advertises.

One data point on VBLANK, since you raised it. With 2/7 of this series
the 90 fps mode runs at vblank 92, and in its clean runs it logged no
CSI-2 errors and no corruption I could see here. I did not go below 40,
so I can neither confirm nor contradict that part.

D. Manresa

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

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

[Dan Scally]

Hi Antoine

On 26/10/2026 11:10, Antoine Pichot wrote:
> 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>
> ---

Thanks for the patch, this is right:

Reviewed-by: Daniel Scally <dan.scally@ideasonboard.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);

[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