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/atomic: Rework initial state allocation Date: Wed, 27 May 2026 14:30:48 +1000 Message-ID: In-Reply-To: <20260526-drm-mode-config-init-v6-0-852346394200@kernel.org> References: <20260526-drm-mode-config-init-v6-0-852346394200@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/atomic: Rework initial state allocation Author: Maxime Ripard Patches: 20 Reviewed: 2026-05-27T14:30:48.205573 --- This is a well-structured 19-patch series from Maxime Ripard that introduce= s a new `atomic_create_state` callback pattern for DRM planes, CRTCs, and c= onnectors, mirroring what already exists for `drm_private_obj`. The motivat= ion is sound: `reset()` is overloaded (initial state allocation, hardware r= eset, suspend/resume recovery), non-fallible, and behaves differently for d= ifferent object types. Separating pristine state allocation into `atomic_cr= eate_state` makes the initialization path cleaner and fallible. The series is on its 6th revision and has accumulated Reviewed-by tags from= Thomas Zimmermann, Laurent Pinchart, Dmitry Baryshkov, and Tomi Valkeinen = across most patches. The incremental approach (doc fixes, renames, then new= callbacks, then helpers, then driver conversions) is methodical and makes = bisection clean. **Key concerns:** 1. **Missing cleanup on partial failure in `drm_mode_config_create_initial_= state()`** (patch 16): If allocation fails partway through (e.g., a connect= or state allocation fails after planes and CRTCs succeeded), the already-al= located states are leaked. There's no rollback path. 2. **Minor doc typo**: Patch 12 has a double space in "Allocates and initi= alizes" in the `drm_atomic_helper_crtc_create_state()` doc. Also present in= patch 15's `drm_atomic_helper_connector_create_state()`. 3. **`drm_atomic_commit` vs `drm_atomic_state`**: The doc in patch 1 refere= nces `drm_atomic_commit` and `drm_atomic_commit_alloc()`. The upstream tree= currently uses `drm_atomic_state` =E2=80=94 this appears to be part of a s= eparate renaming that must be in the same tree. This is fine if the base co= mmit includes that rename, but worth noting for readers. Overall, this is a clean and well-reasoned series. The main substantive iss= ue is the partial-failure leak in `drm_mode_config_create_initial_state()`. --- --- Generated by Claude Code Patch Reviewer