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/xe/ggtt: hold FORCEWAKE while updating GGTT PTEs on LNL Date: Mon, 25 May 2026 20:52:12 +1000 Message-ID: In-Reply-To: <20260520194426.1334262-1-sonny@milton.pro> References: <20260520194426.1334262-1-sonny@milton.pro> <20260520194426.1334262-1-sonny@milton.pro> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Code correctness:** The usage of `xe_with_force_wake()` is correct. The m= acro declares `fw_ref` as a scoped local variable, so no pre-declaration is= needed. `gt_to_fw(ggtt->tile->primary_gt)` correctly gets the force wake h= andle from the primary GT, and `XE_FW_GT` is the right domain. **`xe_ggtt_clear()` wrapping (lines 277=E2=80=93289):** ```c xe_with_force_wake(fw_ref, gt_to_fw(ggtt->tile->primary_gt), XE_FW_GT) { while (start < end) { ggtt->pt_ops->ggtt_set_pte(ggtt, start, scratch_pte); start +=3D XE_PAGE_SIZE; } } ``` Looks correct. The comment is informative and explains the why clearly. **`xe_ggtt_insert_node_transform()` wrapping (lines 783=E2=80=93795):** ```c xe_with_force_wake(fw_ref, gt_to_fw(ggtt->tile->primary_gt), XE_FW_GT) { if (transform) transform(ggtt, node, pte_flags, ggtt->pt_ops->ggtt_set_pte, arg); else xe_ggtt_map_bo(ggtt, node, bo, pte_flags); } ``` Correct. Note that the `transform` callback receives `ggtt_set_pte` and wil= l call it inside the FORCEWAKE scope, which is the right thing. **`__xe_ggtt_insert_bo_at()` wrapping (lines 867=E2=80=93870):** ```c xe_with_force_wake(fw_ref, gt_to_fw(ggtt->tile->primary_gt), XE_FW_GT) { xe_ggtt_map_bo(ggtt, bo->ggtt_node[tile_id], bo, pte); } ``` Correct. **Nit =E2=80=94 platform scoping:** The FORCEWAKE is taken unconditionally = on all platforms, not just LNL. The commit message frames this as an LNL is= sue. If FORCEWAKE is cheap and idempotent on platforms that don't need it, = this is fine (and is the simpler approach). But if the maintainers want it = gated to affected platforms, a workaround check or platform conditional mig= ht be requested. The patch as submitted is the simpler and safer approach. **Missing coverage of `xe_ggtt_map_bo_unlocked()`:** This function at line = 735 calls `xe_ggtt_map_bo()` without FORCEWAKE protection: ```c void xe_ggtt_map_bo_unlocked(struct xe_ggtt *ggtt, struct xe_bo *bo) { ... xe_ggtt_map_bo(ggtt, bo->ggtt_node[ggtt->tile->id], bo, pte); ... } ``` This is called from `xe_bo_evict.c:208` during post-suspend BO restore. If = the rationale is that GSM writes can hang when the GT is in RC6, this path = has the same exposure. It may warrant the same wrapping, or at minimum a co= mment explaining why it's safe without FORCEWAKE (e.g., if the resume path = guarantees GT is awake at that point). **Missing coverage of SR-IOV paths (`xe_ggtt_assign_locked`, `xe_ggtt_node_= load`):** These write PTEs in loops without FORCEWAKE. They are SR-IOV spec= ific and may be less likely to trigger in practice, but are theoretically v= ulnerable to the same issue. Lower priority since SR-IOV on LNL may not be = a common configuration. **Overall:** The patch is a reasonable, narrowly-scoped fix for a real hang= . The code changes are correct. The main question for the maintainer is whe= ther `xe_ggtt_map_bo_unlocked()` should also be covered, and whether the un= conditional FORCEWAKE (rather than LNL-gated) is acceptable. --- Generated by Claude Code Patch Reviewer