Claude review: drm/edid: add CTA Video Format Data Block support

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Bug: Wrong constant in step 8 vsync_position (line 5654-5657)**

```c
vsync_position = max(OVT_MIN_VSYNC_LE_LINES,
		     DIV64_U64_ROUND_UP((u64)OVT_MIN_VSYNC_LE_LINES *
					pixel_clock_rate,
					(u64)htotal * (u64)1000000));
```

The numerator uses `OVT_MIN_VSYNC_LE_LINES` (14) but should use `OVT_MIN_VSYNC_LEADING_EDGE` (400). The formula should be computing: `ceil(min_vsync_leading_edge_us * pixel_clock_rate / (htotal * 1000000))`. Using 14 instead of 400 makes the time-based term ~28.6x too small, so `max()` will almost always just return 14 (the line-based minimum), producing incorrect vsync positioning when the time-based constraint should dominate. Should be:

```c
vsync_position = max(OVT_MIN_VSYNC_LE_LINES,
		     DIV64_U64_ROUND_UP((u64)OVT_MIN_VSYNC_LEADING_EDGE *
					pixel_clock_rate,
					(u64)htotal * (u64)1000000));
```

---

**Bug: u32 overflow in `calculate_ovt_min_htotal` step 4 (line 5486)**

```c
u64 min_pixel_clock_rate;
...
min_pixel_clock_rate = max_vrate * min_htotal * min_vtotal;
```

All three operands are `u32`. The multiplication is performed in `u32` arithmetic before assignment to `u64`, so the overflow has already occurred. For high-resolution RIDs (e.g., RID 22: 15360x4320 at max_vrate=480), the product can easily exceed 2^32. For example: 480 * 15600 * 4500 = ~33.7 billion, which overflows u32 (max ~4.29 billion). Fix:

```c
min_pixel_clock_rate = (u64)max_vrate * min_htotal * min_vtotal;
```

Similarly, line 5553 has `pixel_clock_rate = max_vrate * min_resolution` but this is safe since `min_resolution` is `u64`, promoting the multiplication to 64-bit.

---

**Missing EXPORT_SYMBOL for `drm_ovt_mode` (line 5575-5677)**

The function is declared in `include/drm/drm_edid.h` and the v5 changelog says "export drm_ovt_mode()", but there is no `EXPORT_SYMBOL_GPL(drm_ovt_mode)` after the function definition. Any out-of-tree or modular driver calling this function will fail to link. Either add the export symbol or remove the header declaration if it's only intended for internal use within drm_edid.c.

---

**Readability: goto-based loop entry in `calculate_ovt_pixel_clock_rate` (lines 5522-5551)**

```c
do {
	min_resolution = 0;
	v = min_vtotal;

	goto loop_end;

	while (!min_resolution || r <= min_resolution) {
		goto inner_loop_end;

		while (rem || div64_u64(max_vrate * r, (h & ~(h - 1))) >
		       OVT_MAX_CHUNK_RATE) {
			h += htotal_granularity;
			r = (u64)h * (u64)v;
inner_loop_end:
			div64_u64_rem(r, resolution_granularity, &rem);
		}
		...
loop_end:
		h = min_htotal;
		r = (u64)h * (u64)v;
	}
```

This uses gotos to jump into the middle of nested while-loops to perform initialization before the first condition check — a "loop-and-a-half" pattern. While functionally equivalent to do-while loops, it is extremely difficult to read, review, and maintain. This should be rewritten using explicit do-while loops or by restructuring the initialization. For example:

```c
v = min_vtotal;
h = min_htotal;
r = (u64)h * (u64)v;
do {
    div64_u64_rem(r, resolution_granularity, &rem);
    while (rem || ...) {
        h += htotal_granularity;
        r = (u64)h * (u64)v;
        div64_u64_rem(r, resolution_granularity, &rem);
    }
    if (!min_resolution || r < min_resolution) { ... }
    v += vtotal_granularity;
    h = min_htotal;
    r = (u64)h * (u64)v;
} while (!min_resolution || r <= min_resolution);
```

---

**Missing `case 144` in step 1 (line 5595-5633)**

The `video_format_frame_rates` table includes 144 Hz (index 9), and `vfd_has_fr` handles it, but the step 1 switch in `drm_ovt_mode` has no `case 144`. This means 144 Hz falls through to the default: `vtotal_granularity = 1`, `max_vrate = 144`. If this is intentional per the CTA-861-I spec, a comment explaining why would help. If not, it should be grouped like the other rates (perhaps with max_vrate remaining 144, vtotal_granularity = 5).

---

**Typo in comment (line 5574)**

```c
/* OVT Algorthim as specified in CTA-861-I */
```

"Algorthim" should be "Algorithm".

---

**Minor: `struct cta_vfd` uses `bool` for bitfield results (lines 5082-5090)**

```c
struct cta_vfd {
	u8 rid;
	u8 fr_fact;
	bool bfr50;
	bool fr24;
	...
};
```

This works correctly since C `_Bool` coerces non-zero to `true`, but using `u8` with explicit `!!` or a `: 1` bitfield would be more conventional in kernel code. Not blocking.

---

**Step 9 is skipped**

The code has comments for steps 1, 2, 3, 4, 5, 6, 7, 8, and 10, but step 9 is not referenced. This is likely because DRM modes use sync start/end positions rather than front/back porch values, so step 9's porch calculations are folded into step 10. A brief comment noting this would improve clarity.

---

**Data table correctness**: The `rids[]`, `video_format_frame_rates[]`, and `rid_to_vic[][]` tables appear correctly transcribed from CTA-861-I Tables 11, 12, and 13 respectively. The `find_vic` bounds check (`video_format_frame_rates[rate_idx] > 120`) correctly limits `rate_idx - 1` to the range 0-7, matching the `[8]` second dimension of `rid_to_vic`.

---
Generated by Claude Code Patch Reviewer