From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Refactor drm_writeback_connector structure Date: Mon, 25 May 2026 19:19:14 +1000 Message-ID: In-Reply-To: <20260522050313.1800378-1-suraj.kandpal@intel.com> References: <20260522050313.1800378-1-suraj.kandpal@intel.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Overall Series Review Subject: Refactor drm_writeback_connector structure Author: Suraj Kandpal Patches: 9 Reviewed: 2026-05-25T19:19:14.562559 --- This is v5 of a series that refactors `drm_writeback_connector` by moving it from a standalone struct (that embeds `drm_connector`) into a field within `drm_connector` itself, in a union with the existing `hdmi` connector data. The motivation is sound: drivers like Intel cannot easily work with the current design where the connector is embedded inside `drm_writeback_connector`, because C's "inheritance via embedding" forces the writeback connector to be the outermost allocation owner, which conflicts with driver connector hierarchies. The approach of unionizing writeback with HDMI inside `drm_connector` is reasonable since a connector cannot be both HDMI and writeback simultaneously. The series is well-structured, splitting the core refactor (patch 1) from incremental API migrations (patches 2-7). **Key concerns:** 1. **Build-breaking bug in patch 4** (amdgpu): references `acrtc->connector` which doesn't exist -- the field is `acrtc->wb_conn` and it's a `struct drm_writeback_connector *`, so it needs `drm_writeback_to_connector(acrtc->wb_conn)`. 2. **The `encoder` field is silently dropped** from `drm_writeback_connector` without explanation. The old struct had an embedded `struct drm_encoder encoder` used by `drm_writeback_connector_init()` (the non-`_with_encoder` variant). This series does not address this; the new struct in `drm_connector.h` has no `encoder` field. This means `drm_writeback_connector_init()` (the old-style one that internally creates an encoder) would be broken. The series sits on Dmitry's series [3] which may handle this, but it should be mentioned. 3. **Inconsistency in patch 4** with the `malidp_hw.c` hunk: this is reverted from patch 1's changes, but the reverse direction looks identical to what patch 1 introduced. The diff shows the exact same changes being applied again (patch 1 already changed these lines to `.writeback`), suggesting the series was rebased onto Dmitry's patches that differ from what patch 1 targets. 4. **Minor typo** in the kerneldoc: `valriables` should be `variables` (patch 1, `drm_connector.h`). Overall the design direction is good and most of the mechanical changes are correct. The amdgpu bug is the blocking issue. --- --- Generated by Claude Code Patch Reviewer