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: introduce SVM range migration decision layer Date: Tue, 28 Apr 2026 14:43:04 +1000 Message-ID: In-Reply-To: <20260427100522.7014-4-Junhua.Shen@amd.com> References: <20260427100522.7014-1-Junhua.Shen@amd.com> <20260427100522.7014-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 **Overall:** Clean, minimal patch. The decision layer is well-factored. **Issue: `range_needs_migrate_to_vram` doesn't check for `AMDGPU_SVM_MIGRAT= E_TO_VRAM` explicitly:** ```c +static bool +range_needs_migrate_to_vram(struct drm_gpusvm_range *range, + enum amdgpu_svm_migrate_mode mode) +{ + if (!range->pages.flags.migrate_devmem) + return false; + + if (mode =3D=3D AMDGPU_SVM_MIGRATE_TO_SYSMEM) + return false; ``` This means if `mode =3D=3D AMDGPU_SVM_MIGRATE_NONE` (which is excluded by t= he WARN_ON caller, but still), it would return true. The function is only c= alled from `amdgpu_svm_range_migrate_range()` which validates mode, so this= is safe in practice but the logic could be clearer. **Nit:** `amdgpu_pagemap_capable()` checks `adev->gmc.is_app_apu` =E2=80=94= the name `is_app_apu` seems like an unusual field. Confirm this is the rig= ht check for "this device can do VRAM migration." **Question:** The `AMDGPU_SVM_MIGRATE_PREFERRED` mode is defined in the enu= m but never handled in `amdgpu_svm_range_migrate_range()` =E2=80=94 it trig= gers the WARN_ON. The resolution happens in the caller (`amdgpu_svm_range_m= ap` in patch 5). This layering is correct but the docstring on the enum sho= uld make it even more explicit that PREFERRED must be resolved before calli= ng migrate_range. --- Generated by Claude Code Patch Reviewer