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/amdgpu: implement synchronous TTM eviction for SVM BOs Date: Sat, 16 May 2026 12:15:38 +1000 Message-ID: In-Reply-To: <20260513095734.69598-4-Junhua.Shen@amd.com> References: <20260513095734.69598-1-Junhua.Shen@amd.com> <20260513095734.69598-4-Junhua.Shen@amd.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 **`amdgpu_svm_bo_is_svm` relies on function pointer comparison:** ```c bool amdgpu_svm_bo_is_svm(struct ttm_buffer_object *tbo) { return tbo->destroy =3D=3D &amdgpu_bo_svm_destroy; } ``` This requires `amdgpu_bo_svm_destroy` to be visible across translation unit= s, but it's declared `static` in `amdgpu_migrate.c`. This means `amdgpu_svm= _bo_is_svm` must also be in `amdgpu_migrate.c`, which it is (added in the s= ame file's diff). However, this coupling is fragile =E2=80=94 if someone mo= ves the function or adds LTO optimizations that merge identical functions, = the pointer comparison breaks. Consider using a flag in the BO instead. **`amdgpu_bo_is_amdgpu_bo` now calls `amdgpu_svm_bo_is_svm` unconditionally= :** ```c if (bo->destroy =3D=3D &amdgpu_bo_destroy || bo->destroy =3D=3D &amdgpu_bo_user_destroy || amdgpu_svm_bo_is_svm(bo)) return true; ``` This adds a function call overhead on the hot path for every BO check, even= when SVM is disabled. The `!IS_ENABLED(CONFIG_DRM_AMDGPU_SVM)` stub return= s `false`, so this is just a minor performance nit =E2=80=94 the compiler s= hould inline and eliminate it. **Eviction in `amdgpu_bo_move` =E2=80=94 no error message on failure:** ```c r =3D amdgpu_svm_bo_evict(abo); if (!r) ttm_bo_move_null(bo, new_mem); return r; ``` If `amdgpu_svm_bo_evict` fails, `r` is returned directly with no logging. T= TM will retry eviction, potentially in an infinite loop. Consider at least = a `dev_warn`. --- Generated by Claude Code Patch Reviewer