From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: lib/fonts: Store font data for user space with font_data_export() Date: Tue, 10 Mar 2026 12:19:45 +1000 Message-ID: In-Reply-To: <20260309141723.137364-13-tzimmermann@suse.de> References: <20260309141723.137364-1-tzimmermann@suse.de> <20260309141723.137364-13-tzimmermann@suse.de> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Potential user-visible regression for width 17-24 fonts.** The old code for `font->width <=3D 24` used `sizeof(u32)` (4 bytes) as the = internal pitch for reading font data: ```c fontdata +=3D sizeof(u32); /* 4 bytes per scanline internally */ ``` but wrote only 3 bytes to user space. The new code uses `pitch =3D DIV_ROUN= D_UP(font->width, 8) =3D 3` for both reading and writing. If any existing internal font with width 17-24 stores its data with 4-byte-= per-scanline alignment (which the old code assumed), the new code would rea= d the wrong bytes. Let me check: currently there are no built-in fonts with= widths in the 17-24 range, so this only affects user-loaded fonts. For use= r-loaded fonts, `font_data_import()` (patch 11) uses `pitch =3D DIV_ROUND_U= P(width, 8)` which gives 3 for width 17-24. So the import and export are no= w consistent with each other (both use 3-byte pitch), even though they diff= er from the old behavior. The commit message addresses this and references = user-space code, so this appears intentional. The padding calculation: ```c memset(data + glyphsize, 0, pitch * vpitch - glyphsize); data +=3D pitch * vpitch; ``` Where `glyphsize =3D font->height * pitch` and the padding is `pitch * (vpi= tch - font->height)`. This is correct. The early return for missing data: ```c if (!font->width || !font->height || !font->charcount || !font->data) return 0; ``` This returns success (0) when `font->data` is NULL, which matches the origi= nal fbcon code's implicit behavior (it would just skip the copy), but the o= riginal code had `if (!font->data) return 0;` explicitly =E2=80=94 checking= width/height/charcount is new defensive logic that seems fine. --- Generated by Claude Code Patch Reviewer