From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/edid: add CTA Video Format Data Block support Date: Fri, 27 Feb 2026 13:05:57 +1000 Message-ID: In-Reply-To: <20260225175709.408010-1-someguy@effective-light.com> References: <20260225175709.408010-1-someguy@effective-light.com> <20260225175709.408010-1-someguy@effective-light.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Bug: Wrong constant in step 8 vsync_position (line 5654-5657)** ```c vsync_position =3D 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_VS= YNC_LEADING_EDGE` (400). The formula should be computing: `ceil(min_vsync_l= eading_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 a= lways just return 14 (the line-based minimum), producing incorrect vsync po= sitioning when the time-based constraint should dominate. Should be: ```c vsync_position =3D 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 =3D max_vrate * min_htotal * min_vtotal; ``` All three operands are `u32`. The multiplication is performed in `u32` arit= hmetic before assignment to `u64`, so the overflow has already occurred. Fo= r high-resolution RIDs (e.g., RID 22: 15360x4320 at max_vrate=3D480), the p= roduct can easily exceed 2^32. For example: 480 * 15600 * 4500 =3D ~33.7 bi= llion, which overflows u32 (max ~4.29 billion). Fix: ```c min_pixel_clock_rate =3D (u64)max_vrate * min_htotal * min_vtotal; ``` Similarly, line 5553 has `pixel_clock_rate =3D max_vrate * min_resolution` = but this is safe since `min_resolution` is `u64`, promoting the multiplicat= ion 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 s= ays "export drm_ovt_mode()", but there is no `EXPORT_SYMBOL_GPL(drm_ovt_mod= e)` after the function definition. Any out-of-tree or modular driver callin= g this function will fail to link. Either add the export symbol or remove t= he header declaration if it's only intended for internal use within drm_edi= d.c. --- **Readability: goto-based loop entry in `calculate_ovt_pixel_clock_rate` (l= ines 5522-5551)** ```c do { min_resolution =3D 0; v =3D min_vtotal; goto loop_end; while (!min_resolution || r <=3D min_resolution) { goto inner_loop_end; while (rem || div64_u64(max_vrate * r, (h & ~(h - 1))) > OVT_MAX_CHUNK_RATE) { h +=3D htotal_granularity; r =3D (u64)h * (u64)v; inner_loop_end: div64_u64_rem(r, resolution_granularity, &rem); } ... loop_end: h =3D min_htotal; r =3D (u64)h * (u64)v; } ``` This uses gotos to jump into the middle of nested while-loops to perform in= itialization before the first condition check =E2=80=94 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 exp= licit do-while loops or by restructuring the initialization. For example: ```c v =3D min_vtotal; h =3D min_htotal; r =3D (u64)h * (u64)v; do { div64_u64_rem(r, resolution_granularity, &rem); while (rem || ...) { h +=3D htotal_granularity; r =3D (u64)h * (u64)v; div64_u64_rem(r, resolution_granularity, &rem); } if (!min_resolution || r < min_resolution) { ... } v +=3D vtotal_granularity; h =3D min_htotal; r =3D (u64)h * (u64)v; } while (!min_resolution || r <=3D min_resolution); ``` --- **Missing `case 144` in step 1 (line 5595-5633)** The `video_format_frame_rates` table includes 144 Hz (index 9), and `vfd_ha= s_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 =3D 1= `, `max_vrate =3D 144`. If this is intentional per the CTA-861-I spec, a co= mment explaining why would help. If not, it should be grouped like the othe= r rates (perhaps with max_vrate remaining 144, vtotal_granularity =3D 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 k= ernel 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 posi= tions rather than front/back porch values, so step 9's porch calculations a= re 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_fr= ame_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