From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm: writeback: Modify drm_writeback_signal_completion helper
Date: Mon, 25 May 2026 20:57:36 +1000
Message-ID: <review-patch4-20260521053708.1475129-5-suraj.kandpal@intel.com>
In-Reply-To: <20260521053708.1475129-5-suraj.kandpal@intel.com>
References: <20260521053708.1475129-1-suraj.kandpal@intel.com> <20260521053708.1475129-5-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

Changes `drm_writeback_signal_completion()` to take `struct drm_connector *=
`.

**BUG =E2=80=94 amdgpu `dm_crtc_high_irq` uses wrong field:**

```c
-			drm_writeback_signal_completion(acrtc->wb_conn, 0);
+			drm_writeback_signal_completion(acrtc->connector, 0);
```

`acrtc->connector` is a **pre-existing, unrelated field** on `struct amdgpu=
_crtc` =E2=80=94 it's the general `struct drm_connector *` for the CRTC (li=
ne 502 of `amdgpu_mode.h`), which points to the display connector, NOT the =
writeback connector. `acrtc->wb_conn` is a `struct drm_writeback_connector =
*` (line 513). The correct conversion should be:

```c
drm_writeback_signal_completion(drm_writeback_to_connector(acrtc->wb_conn),=
 0);
```

This bug would cause writeback completion to be signalled on the **wrong co=
nnector**, potentially corrupting the display connector's fence state or cr=
ashing due to the display connector not having writeback data initialized. =
This would likely manifest as a NULL pointer dereference or list corruption=
 in the job_queue when called from an IRQ handler, making it a serious bug.

Note that the surrounding code in `dm_crtc_high_irq` (lines 664-689) still =
accesses `acrtc->wb_conn->job_lock` and `acrtc->wb_conn->job_queue` directl=
y, which are `drm_writeback_connector` fields =E2=80=94 so `wb_conn` is sti=
ll needed. The patch should also update those accesses if the intent is to =
go through `drm_connector`, or leave the signal_completion call using `drm_=
writeback_to_connector(acrtc->wb_conn)`.

**Minor issue:**

1. **Typo in doc comment** =E2=80=94 "whicha" should be "which":
   ```c
   * @connector: The drm connector whicha has the drm_writeback_connector w=
hose
   ```

2. The MSM dpu code correctly uses `drm_writeback_to_connector(wb_enc->wb_c=
onn)`:
   ```c
   drm_writeback_signal_completion(drm_writeback_to_connector(wb_enc->wb_co=
nn), 0);
   ```
   This is the right pattern and highlights that the amdgpu change is wrong.

---
Generated by Claude Code Patch Reviewer