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: Rename drm_atomic_state Date: Tue, 28 Apr 2026 14:55:34 +1000 Message-ID: In-Reply-To: <20260427-drm-drm-atomic-update-v4-0-c0e713bfdf25@kernel.org> References: <20260427-drm-drm-atomic-update-v4-0-c0e713bfdf25@kernel.org> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Overall Series Review Subject: drm: Rename drm_atomic_state Author: Maxime Ripard Patches: 2 Reviewed: 2026-04-28T14:55:34.132450 --- This is a 2-patch series by Maxime Ripard (v4, rebased on 7.1-rc1) to renam= e `struct drm_atomic_state` to `struct drm_atomic_commit` across the entire= DRM subsystem. The motivation is well-established: the term "state" at the= device level is confusing because `drm_atomic_state` doesn't represent the= full device state =E2=80=94 it represents a *commit* (a delta/update to a = subset of objects). Per-object structs like `drm_crtc_state` genuinely repr= esent the full state of that object, so the naming inconsistency causes con= fusion and bugs. **Critical issue: Patch 1/2 is missing from the mbox.** The mbox only conta= ins the cover letter (0/2) and patch 2/2. Patch 1/2 =E2=80=94 "drm: Rename = struct drm_atomic_state to drm_atomic_commit" =E2=80=94 which is the actual= 365-file, ~3500-line mechanical rename, is absent. This makes a full revie= w of the series impossible. Only patch 2/2 (the documentation todo cleanup)= can be reviewed. **Naming concern with the existing `drm_atomic_commit()` function.** After = the rename, both the struct and the existing function `drm_atomic_commit()`= (at `drm_atomic.c:1774`) would share the same base name. Post-rename, the = function signature becomes: ```c int drm_atomic_commit(struct drm_atomic_commit *state) ``` While there's a certain elegance to "commit a commit," this creates ambigui= ty =E2=80=94 `drm_atomic_commit` could refer to either the type or the func= tion depending on context. In C this isn't a compiler issue (struct tags an= d function names occupy different namespaces), but it could confuse develop= ers reading code or grepping the tree. This was presumably discussed in ear= lier revisions (v1 used a different name), but it's worth noting. **Merge conflict risk.** Touching 365 files in one patch guarantees conflic= ts with virtually any in-flight DRM work. The cover letter correctly notes = this should go through drm-next directly to minimize the window of pain. **Skips file concern.** The vkms CI skip file (`vkms-none-skips.txt`) conta= ins commented-out stack traces with function names like `drm_atomic_state_d= efault_clear` and `__drm_atomic_state_free`. If patch 1/2 mechanically rena= mes these, the skip patterns may no longer match actual kernel symbols, pot= entially changing CI behavior. These are in comments (prefixed with `#`), s= o the impact depends on whether the CI tooling parses them. --- Generated by Claude Code Patch Reviewer