Claude review: drm/xe: Do not blindly copy system CCS pages when shrinking

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**xe_bo_types.h — new `ccs_used` field**

The field placement and documentation look fine. Minor note: the existing tree has `struct xe_bb *bb_ccs[]` while the patch context shows `struct xe_mem_pool_node *bb_ccs[]` — this is likely just a base mismatch (the patch description notes v2 changes), not a real problem.

**xe_bo.h — new helpers**

The `xe_bo_set_ccs_used()` and `xe_bo_is_ccs_used()` helpers are clean. Both 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 — the core optimization**

```c
} else if (handle_system_ccs && new_mem->mem_type == XE_PL_SYSTEM &&
	   !xe_bo_is_ccs_used(bo)) {
	fence = dma_fence_get_stub();
```

This is the key change. When `handle_system_ccs` is true but the BO was never used with compression and the destination is system memory, a stub fence is returned instead of calling `xe_migrate_copy()`. This is correct — 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 confirming that `ttm_bo_move_accel_cleanup()` with an already-signaled fence and `evict=true, pipeline=true` works correctly and actually moves the data pages. The stub fence means no *CCS* copy happens, but the main BO data also doesn't get copied here. Looking more carefully at the flow: the original path calls `xe_migrate_copy()` which handles *both* main data and CCS 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 actual data pages that need migrating.

However, re-reading the condition: `handle_system_ccs` is true only for integrated GPUs (`!IS_DGFX`) where CCS pages live in system memory alongside the main data. In that case the "move" to system memory when the data is already in a system-memory-like TT pool may indeed not need a copy — but this deserves careful analysis. If the BO is moving from TT to SYSTEM placement 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 transitions on integrated GPUs**, or whether the condition should also check `old_mem_type`.

**xe_vm.c — xe_vm_ops_add_rebind**

```c
struct xe_bo *bo = 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, mark the BO. The BO reservation lock should be held at this point since the rebind operation requires it.

**xe_vm.c — vma_lock_and_validate**

```c
	if (flags.request_decompress)
		err = 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 flag on a failed path is relatively harmless (it's a one-way latch and a conservative 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 — `bo` is non-NULL when this runs. But the indentation in the patch could be clearer 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 implementation is mostly clean. The main concern is whether returning `dma_fence_get_stub()` in `xe_bo_move()` is safe for all cases — since this replaces 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 migration 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` guard for consistency.

---
Generated by Claude Code Patch Reviewer