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: Do not blindly copy system CCS pages when shrinking Date: Tue, 05 May 2026 09:41:41 +1000 Message-ID: In-Reply-To: <20260501011907.2331654-1-matthew.brost@intel.com> References: <20260501011907.2331654-1-matthew.brost@intel.com> <20260501011907.2331654-1-matthew.brost@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 **xe_bo_types.h =E2=80=94 new `ccs_used` field** The field placement and documentation look fine. Minor note: the existing t= ree has `struct xe_bb *bb_ccs[]` while the patch context shows `struct xe_m= em_pool_node *bb_ccs[]` =E2=80=94 this is likely just a base mismatch (the = patch description notes v2 changes), not a real problem. **xe_bo.h =E2=80=94 new helpers** The `xe_bo_set_ccs_used()` and `xe_bo_is_ccs_used()` helpers are clean. Bot= h assert the reservation lock is held. The kernel-doc is thorough (perhaps = more than needed for simple one-line helpers, but that's a style preference= and the commit message says Claude assisted with docs). **xe_bo.c =E2=80=94 the core optimization** ```c } else if (handle_system_ccs && new_mem->mem_type =3D=3D XE_PL_SYSTEM && !xe_bo_is_ccs_used(bo)) { fence =3D dma_fence_get_stub(); ``` This is the key change. When `handle_system_ccs` is true but the BO was nev= er used with compression and the destination is system memory, a stub fence= is returned instead of calling `xe_migrate_copy()`. This is correct =E2=80= =94 if CCS was never written, there's nothing to preserve. One concern: the code falls through to the `if (!move_lacks_source)` block = at line 1130, which calls `ttm_bo_move_accel_cleanup()` with the stub fence= . That should be fine since the stub fence is already signaled, but worth c= onfirming that `ttm_bo_move_accel_cleanup()` with an already-signaled fence= and `evict=3Dtrue, pipeline=3Dtrue` works correctly and actually moves the= data pages. The stub fence means no *CCS* copy happens, but the main BO da= ta also doesn't get copied here. Looking more carefully at the flow: the or= iginal path calls `xe_migrate_copy()` which handles *both* main data and CC= S data together. **By returning a stub fence, the patch skips not only CCS = copies but also the main data copy.** This seems like a bug if the BO has a= ctual data pages that need migrating. However, re-reading the condition: `handle_system_ccs` is true only for int= egrated GPUs (`!IS_DGFX`) where CCS pages live in system memory alongside t= he main data. In that case the "move" to system memory when the data is alr= eady in a system-memory-like TT pool may indeed not need a copy =E2=80=94 b= ut this deserves careful analysis. If the BO is moving from TT to SYSTEM pl= acement and there *is* data but no CCS, the normal path would still do `xe_= migrate_copy()` to handle the move. The stub fence would skip that entirely= . **I'd want the author to confirm that this is safe for all TT-to-SYSTEM t= ransitions on integrated GPUs**, or whether the condition should also check= `old_mem_type`. **xe_vm.c =E2=80=94 xe_vm_ops_add_rebind** ```c struct xe_bo *bo =3D xe_vma_bo(vma); ... if (bo && xe_pat_index_get_comp_en(xe_vma_vm(vma)->xe, vma->attr.pat_index)) xe_bo_set_ccs_used(bo); ``` This looks correct. On rebind, if the PAT index has compression enabled, ma= rk the BO. The BO reservation lock should be held at this point since the r= ebind operation requires it. **xe_vm.c =E2=80=94 vma_lock_and_validate** ```c if (flags.request_decompress) err =3D xe_bo_decompress(bo); if (xe_pat_index_get_comp_en(vm->xe, vma->attr.pat_index)) xe_bo_set_ccs_used(bo); ``` Two issues here: 1. **Missing error check**: `xe_bo_decompress()` can fail and set `err` to = a non-zero value. The new code unconditionally calls `xe_bo_set_ccs_used()`= even when `err` is non-zero from the decompress call. While setting the fl= ag on a failed path is relatively harmless (it's a one-way latch and a cons= ervative overapproximation), it would be cleaner to gate it with `if (!err = && ...)` for consistency with the function's error-checking pattern. 2. **Placement outside the `if (bo)` block**: Looking at the patched code, = the new lines are added after `xe_bo_decompress()` but before the closing `= }` of the `if (bo)` block, so they are inside it. This is fine =E2=80=94 `b= o` is non-NULL when this runs. But the indentation in the patch could be cl= earer since there's a blank line before it. **tests/xe_bo.c** ```c xe_bo_set_ccs_used(bo); ``` Correctly sets `ccs_used` before CCS tests so the existing test behavior is= preserved. The BO lock is held at this point. **Overall assessment**: The optimization concept is good and the implementa= tion is mostly clean. The main concern is whether returning `dma_fence_get_= stub()` in `xe_bo_move()` is safe for all cases =E2=80=94 since this replac= es a call to `xe_migrate_copy()` which handles both data and CCS pages, not= just CCS. The author should clarify that the main data doesn't need migrat= ion in these paths, or narrow the condition to only skip the CCS portion of= the copy. The `vma_lock_and_validate` change should also add an `!err` gua= rd for consistency. --- Generated by Claude Code Patch Reviewer