From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm: writeback: Refactor drm_writeback_connector structure
Date: Mon, 25 May 2026 19:19:14 +1000 [thread overview]
Message-ID: <review-patch1-20260522050313.1800378-2-suraj.kandpal@intel.com> (raw)
In-Reply-To: <20260522050313.1800378-2-suraj.kandpal@intel.com>
Patch Review
This is the main structural change. It moves `drm_writeback_connector` into `drm_connector` as a union with `hdmi`, removes the `base` (drm_connector) member from `drm_writeback_connector`, and updates all drivers.
**Issues:**
1. **Typo in kerneldoc** at the new union member:
```c
+ /**
+ * @writeback: Writeback related valriables.
+ */
```
`valriables` should be `variables`.
2. **`encoder` field dropped without comment.** The old `drm_writeback_connector` had:
```c
struct drm_encoder encoder;
```
This is silently removed. The cover letter says this sits on top of Dmitry's series which likely removes the old-style `drm_writeback_connector_init()` that uses this encoder, but the diffstat shows `drm_writeback_connector_init` still exists and is modified. If the encoder field is expected to be gone, the series should either explicitly state this dependency or include the encoder removal.
3. **`drm_writeback_to_connector()` replaces `drm_connector_to_writeback()`** -- the direction is reversed, which is the whole point. The helper is well-implemented using `container_of` on the union member:
```c
+static inline struct drm_connector *
+drm_writeback_to_connector(struct drm_writeback_connector *wb_connector)
+{
+ return container_of(wb_connector, struct drm_connector, writeback);
+}
```
This is correct.
4. **amdgpu `dm_set_writeback()`** changes `drm_connector_to_writeback(connector)` to `&connector->writeback`:
```c
- struct drm_writeback_connector *wb_conn = drm_connector_to_writeback(connector);
+ struct drm_writeback_connector *wb_conn = &connector->writeback;
```
This is fine and consistent. The `wb_conn` is still needed later in the function body.
5. **malidp_hw.c changes** -- these touch `drm_writeback_signal_completion` calls, but patch 4 later changes the signature of `drm_writeback_signal_completion` from taking `drm_writeback_connector*` to `drm_connector*`. Patch 1 changes the callers to pass `&malidp->mw_connector.writeback`, then patch 4 changes them back to `&malidp->mw_connector`. However, looking at the patch 4 diff, the old_string in patch 4 is `&malidp->mw_connector.writeback` matching what patch 1 produces. The diff in patch 4 shows the malidp_hw.c file reverting to the *original* source hash (`9b845d3f34e1`), which is suspicious -- likely a rebase artifact. The final result after all patches should be correct.
6. **vkms_writeback.c `vkms_wb_cleanup_job()`** -- the intermediate form uses `container_of` manually:
```c
+ struct drm_connector *connector = container_of(wb_conn,
+ struct drm_connector,
+ writeback);
```
This is cleaned up in patch 7 when the callback signature changes.
7. The driver struct changes (e.g., `struct drm_writeback_connector base` -> `struct drm_connector base`) are all mechanically correct for AMD, komeda, malidp, vc4, vkms, rcar-du, and MSM.
---
Generated by Claude Code Patch Reviewer
next prev parent reply other threads:[~2026-05-25 9:19 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-22 5:03 [PATCH v5 0/7] Refactor drm_writeback_connector structure Suraj Kandpal
2026-05-22 5:03 ` [PATCH v5 1/7] drm: writeback: " Suraj Kandpal
2026-05-25 9:19 ` Claude Code Review Bot [this message]
2026-05-22 5:03 ` [PATCH v5 2/7] drm: writeback: Modify writeback init helpers Suraj Kandpal
2026-05-25 9:19 ` Claude review: " Claude Code Review Bot
2026-05-22 5:03 ` [PATCH v5 3/7] drm: writeback: Modify drm_writeback_queue_job helper Suraj Kandpal
2026-05-25 9:19 ` Claude review: " Claude Code Review Bot
2026-05-22 5:03 ` [PATCH v5 4/7] drm: writeback: Modify drm_writeback_signal_completion helper Suraj Kandpal
2026-05-25 9:19 ` Claude review: " Claude Code Review Bot
2026-05-22 5:03 ` [PATCH v5 5/7] drm: writeback: Modify drm_writeback_get_out_fence helper Suraj Kandpal
2026-05-25 9:19 ` Claude review: " Claude Code Review Bot
2026-05-22 5:03 ` [PATCH v5 6/7] drm: writeback: Modify prepare_writeback_job helper Suraj Kandpal
2026-05-25 9:19 ` Claude review: " Claude Code Review Bot
2026-05-22 5:03 ` [PATCH v5 7/7] drm: writeback: Modify cleanup_writeback_job helper Suraj Kandpal
2026-05-25 9:19 ` Claude review: " Claude Code Review Bot
2026-05-22 5:10 ` [PATCH v5 0/7] Refactor drm_writeback_connector structure Kandpal, Suraj
2026-05-25 9:19 ` Claude review: " Claude Code Review Bot
-- strict thread matches above, loose matches on Subject: below --
2026-05-21 5:37 [PATCH v4 0/7] " Suraj Kandpal
2026-05-21 5:37 ` [PATCH v4 1/7] drm: writeback: " Suraj Kandpal
2026-05-25 10:57 ` Claude review: " Claude Code Review Bot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=review-patch1-20260522050313.1800378-2-suraj.kandpal@intel.com \
--to=claude-review@example.com \
--cc=dri-devel-reviews@example.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox