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: writeback: Refactor drm_writeback_connector structure Date: Mon, 25 May 2026 20:57:35 +1000 Message-ID: In-Reply-To: <20260521053708.1475129-2-suraj.kandpal@intel.com> References: <20260521053708.1475129-1-suraj.kandpal@intel.com> <20260521053708.1475129-2-suraj.kandpal@intel.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review This is the core patch. The structural change is sound: `drm_writeback_conn= ector` loses its `struct drm_connector base` member and instead gets embedd= ed inside `struct drm_connector` in a union with `hdmi`. The helper `drm_co= nnector_to_writeback()` is replaced with the inverse `drm_writeback_to_conn= ector()` using `container_of`. **Issues:** 1. **Typo in doc comment** =E2=80=94 the `@writeback` kdoc says "valriables= " instead of "variables": ```c /** * @writeback: Writeback related valriables. */ struct drm_writeback_connector writeback; ``` 2. **`drm_writeback_to_connector()` is only safe if the writeback is actual= ly embedded in a `drm_connector`** =E2=80=94 the `container_of` relies on t= he `writeback` field being part of `drm_connector`. This is now always true= by construction, but there's no runtime assertion. The existing `WARN_ON` = in `drm_writeback_get_out_fence` checking `connector_type` is good defensiv= e practice; consider whether `drm_writeback_to_connector()` itself should h= ave a similar check. 3. **Inconsistency in `dm_set_writeback`** =E2=80=94 the amdgpu code change= s `drm_connector_to_writeback(connector)` to a direct `&connector->writebac= k` access: ```c - struct drm_writeback_connector *wb_conn =3D drm_connector_to_writeback= (connector); + struct drm_writeback_connector *wb_conn =3D &connector->writeback; ``` This is fine functionally, but inconsistent with other places in the sam= e patch that use the new `drm_writeback_to_connector()` helper. Since the h= elper now goes the other direction (wb -> connector), this is correct but c= ould use `&connector->writeback` consistently throughout. 4. **malidp_hw.c changes** =E2=80=94 This file is modified in patch 1 (chan= ging `drm_writeback_signal_completion` calls from `&malidp->mw_connector` t= o `&malidp->mw_connector.writeback`) and then again in patch 4 (changing th= em back from `.writeback` to taking the connector directly). This creates a= transient churn. Consider squashing or reordering to avoid changing these = lines twice. 5. **vkms_writeback.c `vkms_wb_cleanup_job`** =E2=80=94 Uses a raw `contain= er_of` instead of `drm_writeback_to_connector()`: ```c struct drm_connector *connector =3D container_of(wb_conn, struct drm_connector, writeback); ``` This is functionally identical to `drm_writeback_to_connector()` but inc= onsistent. Since patch 7 removes this entirely, it's a transient issue but = still worth noting for bisectability. --- Generated by Claude Code Patch Reviewer