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 10:06:13 +1000 Message-ID: In-Reply-To: <20260430190923.2141506-1-matthew.brost@intel.com> References: <20260430190923.2141506-1-matthew.brost@intel.com> <20260430190923.2141506-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_c =E2=80=94 stub fence branch (the core optimization)** ```c + } else if (handle_system_ccs && !bo->ccs_used) { + fence =3D dma_fence_get_stub(); } else { fence =3D 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 pat= h at line 1130 then enters `ttm_bo_move_accel_cleanup()` with an already-si= gnaled fence. This is functionally correct as a no-op move. However, the key question is: **when we later move the BO back (SYSTEM -> T= T) and `ccs_used` has since transitioned to true, what state are the CCS pa= ges in?** If CCS data was never saved to the ttm_tt pages (because we retur= ned 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=3Dtru= e` 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 dat= a 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 =E2=80=94 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. Howe= ver, there is **no documentation of the locking requirements**. Compare wit= h the nearby `atomic_access` field which documents "Protected by BO dma-res= v 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-r= esv lock. */ ``` The kdoc should also mention the one-way nature of the flag ("one-way trans= ition: once set, never cleared"), since this is a key invariant that caller= s rely on. **xe_vm.c =E2=80=94 setting the flag** ```c if (flags.request_decompress) err =3D xe_bo_decompress(bo); + + if (xe_pat_index_get_comp_en(vm->xe, vma->attr.pat_index)) + bo->ccs_used =3D true; } =20 return err; ``` Two observations: 1. **Error path interaction**: If `xe_bo_decompress(bo)` fails, `err` is no= n-zero but `ccs_used` is still set to true. This is conservative (we'd rath= er do unnecessary CCS copies than skip needed ones), so it's correct for sa= fety. But it means the flag is set even though the bind may effectively fai= l. This is fine but worth a brief comment or at least acknowledgment. 2. **Placement**: `ccs_used` is set unconditionally of `flags.validate` =E2= =80=94 it fires whenever a compressed PAT is used, even if we didn't actual= ly validate the BO. This is also conservative and correct. However, setting= it outside the `if (!err && flags.validate)` block means it's set even whe= n validation was never requested. Since this is a one-way transition to tru= e, 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` earlie= r in the function (for non-VM BOs) or implicitly (for VM-internal BOs). The= `bo->ccs_used =3D true` write should be properly serialized. The external = BO path (`!bo->vm`) takes the lock explicitly; the VM-internal BO path does= n't call `drm_exec_lock_obj` but those BOs shouldn't be shrinkable, so it s= houldn't matter. Worth double-checking. **VF CCS interaction**: When the stub fence path fires for a SYSTEM -> TT m= ove with `handle_system_ccs=3Dtrue`, the VF CCS attach code at lines 1160-1= 164 still executes: ```c if (IS_VF_CCS_READY(xe) && ((move_lacks_source && new_mem->mem_type =3D=3D XE_PL_TT) || (old_mem_type =3D=3D XE_PL_SYSTEM && new_mem->mem_type =3D=3D XE_PL_TT= )) && handle_system_ccs) ret =3D 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_b= o` 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 explic= itly initialized rather than relying on zeroed allocation, for consistency = and clarity. **Minor nit**: The field name `ccs_used` could be confused with "CCS is cur= rently in use." A name like `ccs_ever_compressed` or `ccs_comp_bound` would= more clearly convey the one-way "was ever bound with compression" semantic= s. This is just a suggestion. --- Generated by Claude Code Patch Reviewer