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_c — stub fence branch (the core optimization)**

```c
+	} else if (handle_system_ccs && !bo->ccs_used) {
+		fence = dma_fence_get_stub();
 	} else {
 		fence = xe_migrate_copy(migrate, bo, bo, old_mem, new_mem,
 					handle_system_ccs);
```

When this branch fires, `move_lacks_source` is false (meaning `ccs_cleared` is already true from a prior clear and old_mem exists). The post-fence path at line 1130 then enters `ttm_bo_move_accel_cleanup()` with an already-signaled fence. This is functionally correct as a no-op move.

However, the key question is: **when we later move the BO back (SYSTEM -> TT) and `ccs_used` has since transitioned to true, what state are the CCS pages in?** If CCS data was never saved to the ttm_tt pages (because we returned a stub fence instead of calling `xe_migrate_copy` to read AUX CCS into the ttm_tt), then restoring from SYSTEM -> TT with `handle_system_ccs=true` would write back whatever garbage is in the ttm_tt CCS pages. The safety of this relies on the initial `xe_migrate_clear` having zeroed the CCS data in the ttm_tt pages, and the CCS pages not being reallocated with garbage across swap cycles. It would be worth adding a comment explaining why this is safe, or alternatively clearing the CCS pages in the ttm_tt when taking this shortcut to make the invariant explicit.

**xe_bo_types.h — new field**

```c
+	/** @ccs_used: true means that CCS region of BO is used */
+	bool ccs_used;
```

The field is placed next to `ccs_cleared`, which is good for locality. However, there is **no documentation of the locking requirements**. Compare with the nearby `atomic_access` field which documents "Protected by BO dma-resv lock." Since `ccs_used` is read in `xe_bo_move` (under resv lock via TTM) and written in `vma_lock_and_validate` (under resv lock via `drm_exec_lock_obj`), it should document this:

```c
/** @ccs_used: true means that CCS region of BO is used. Protected by dma-resv lock. */
```

The kdoc should also mention the one-way nature of the flag ("one-way transition: once set, never cleared"), since this is a key invariant that callers rely on.

**xe_vm.c — setting the flag**

```c
 		if (flags.request_decompress)
 			err = xe_bo_decompress(bo);
+
+		if (xe_pat_index_get_comp_en(vm->xe, vma->attr.pat_index))
+			bo->ccs_used = true;
 	}
 
 	return err;
```

Two observations:

1. **Error path interaction**: If `xe_bo_decompress(bo)` fails, `err` is non-zero but `ccs_used` is still set to true. This is conservative (we'd rather do unnecessary CCS copies than skip needed ones), so it's correct for safety. But it means the flag is set even though the bind may effectively fail. This is fine but worth a brief comment or at least acknowledgment.

2. **Placement**: `ccs_used` is set unconditionally of `flags.validate` — it fires whenever a compressed PAT is used, even if we didn't actually validate the BO. This is also conservative and correct. However, setting it outside the `if (!err && flags.validate)` block means it's set even when validation was never requested. Since this is a one-way transition to true, the worst case is premature CCS copying, not data corruption, so this is acceptable.

3. **Locking**: As discussed, `bo` is locked via `drm_exec_lock_obj` earlier in the function (for non-VM BOs) or implicitly (for VM-internal BOs). The `bo->ccs_used = true` write should be properly serialized. The external BO path (`!bo->vm`) takes the lock explicitly; the VM-internal BO path doesn't call `drm_exec_lock_obj` but those BOs shouldn't be shrinkable, so it shouldn't matter. Worth double-checking.

**VF CCS interaction**: When the stub fence path fires for a SYSTEM -> TT move with `handle_system_ccs=true`, the VF CCS attach code at lines 1160-1164 still executes:

```c
if (IS_VF_CCS_READY(xe) &&
    ((move_lacks_source && new_mem->mem_type == XE_PL_TT) ||
     (old_mem_type == XE_PL_SYSTEM && new_mem->mem_type == XE_PL_TT)) &&
    handle_system_ccs)
    ret = xe_sriov_vf_ccs_attach_bo(bo);
```

This would attach VF CCS to a BO where CCS is not actually used. If VF CCS attachment has a performance or memory cost, this branch should also check `bo->ccs_used`.

**Missing initialization**: `ccs_used` is not explicitly initialized. `xe_bo` is allocated via TTM, and `ccs_cleared` is explicitly set to `false` at line 2354 in `__xe_bo_create_locked`. `ccs_used` should similarly be explicitly initialized rather than relying on zeroed allocation, for consistency and clarity.

**Minor nit**: The field name `ccs_used` could be confused with "CCS is currently in use." A name like `ccs_ever_compressed` or `ccs_comp_bound` would more clearly convey the one-way "was ever bound with compression" semantics. This is just a suggestion.

---
Generated by Claude Code Patch Reviewer