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/plane: Add new atomic_create_state callback Date: Wed, 11 Mar 2026 13:09:07 +1000 Message-ID: In-Reply-To: <20260310-drm-mode-config-init-v1-5-de7397c8e1cf@kernel.org> References: <20260310-drm-mode-config-init-v1-0-de7397c8e1cf@kernel.org> <20260310-drm-mode-config-init-v1-5-de7397c8e1cf@kernel.org> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review The implementation is correct but `__drm_atomic_helper_plane_create_state()= ` is a trivial wrapper around `__drm_atomic_helper_plane_reset()`: ```c +void __drm_atomic_helper_plane_create_state(struct drm_plane *plane, + struct drm_plane_state *state) +{ + __drm_atomic_helper_plane_reset(plane, state); +} ``` This is intentional to provide a distinct API that can diverge in the futur= e, but it's worth noting the indirection adds no current value. Additionall= y, `__drm_atomic_helper_plane_reset()` sets `plane->state =3D plane_state`,= which is a side effect. The whole point of `create_state` is "no side effe= cts." The non-underscore variant `drm_atomic_helper_plane_create_state()` a= llocates the state and then calls `__drm_atomic_helper_plane_reset()` which= **assigns `plane->state`** =E2=80=94 but then the caller in `drm_mode_conf= ig_plane_create_state()` also assigns `plane->state`. So the state pointer = is written twice which is harmless but contradicts the "no side effect" int= ent. Consider having the `__create_state` variant call `__drm_atomic_helper= _plane_state_reset()` instead of `__drm_atomic_helper_plane_reset()` to tru= ly avoid the side effect. In `drm_mode_config_reset()`, the error return from `drm_mode_config_plane_= create_state()` is silently ignored: ```c drm_for_each_plane(plane, dev) if (plane->funcs->reset) plane->funcs->reset(plane); + else if (plane->funcs->atomic_create_state) + drm_mode_config_plane_create_state(plane); ``` Since `drm_mode_config_reset()` returns void, this is unavoidable, but it m= eans allocation failures are silently swallowed. Typo in commit message: "software tate" should be "software state". --- Generated by Claude Code Patch Reviewer