On 8/12/2026 9:36 PM, Huang Rui wrote:
On Wed, Aug 12, 2026 at 02:16:03PM +0200, Christian KKKnig wrote:
On 8/12/26 11:55, Huang, Honglei wrote:
On 8/12/2026 4:36 PM, Christian König wrote:
On 8/11/26 16:06, Huang, Honglei wrote:
...
+/*
+ * Helpers for amdgpu_svm.svm_lock, the driver_svm_lock registered with GPU
SVM.
+ * Hold it in write mode around structural GPU SVM updates, including
+ * drm_gpusvm_range_find_or_insert() and drm_gpusvm_range_remove().
+ */
+static inline void amdgpu_svm_lock(struct amdgpu_svm *svm)
+{
+ down_write(&svm->svm_lock);
+}
+
+static inline void amdgpu_svm_unlock(struct amdgpu_svm *svm)
+{
+ up_write(&svm->svm_lock);
+}
+
+static inline void amdgpu_svm_assert_locked(struct amdgpu_svm *svm)
+{
+ lockdep_assert_held_write(&svm->svm_lock);
+}
I'm starting to repeat myself, so once more: This stuff doesn't work like that!
The lock the SVM subsystem uses to serialize updates *must* be the
amdgpu_vm->eviction_lock and *not* a separate one.
So clear NAK to having this functions here.
I have explained why eviction lock can not be used as svm lock in V8, previous
version. And it seems like we have a big gap about it.
The eviction lock can not be used for svm lcok.
And the design of svm lock is the core locking design of drmsvm frame work,
without this desgin, this framework lost its soul. So I have to explain how to
use this lock.
I believe we are talking about two different locks.
Yeah, that stuff is more than a bit complicated. The key point is that I still
don't see any of the mandatory changes to amdgpu_vm.c in this patch set.
You may treat svm lock as notifier lock. drm_gpusvm has two of them, and the "no
allocation while held in the MMU notifier" rule applies
to the other one, not to driver_svm_lock.
1) drm_gpusvm has two distinct locks: drivers/gpu/drm/drm_gpusvm.c
- notifier_lock: safeguards the notifier's range RB tree and list, as
well as the range's DMA mappings and sequence number. ... This lock
corresponds to the driver->update lock mentioned in
Documentation/mm/hmm.rst."
And that one here *MUST* be identical to the eviction lock in amdgpu_vm.c
Actually I have a question about this,
The notifier lock protects the CPU pages tables, it prevents the migration/swap
... from MM logic.
And the eviction lock protects the GPU VM page table, it prevents gpu page
table changes from TTM logic.
They have different jobs,
No, exactly that is what we have gotten wrong in the existing KFD SVM
implementation.
when doing a GPU mapping in SVM, cpu page table can not change, casue it
may change the cpu dma addr -> gpu mapping. So must hold it, it is done in
current code, and it is must required by drm gpu svm frame work.
And at the same time the eviciton lock must hold also, casue the GPU page
tables may change by TTM logic.
Those two locks are all need be hold, no conflict, this is just my thought.
Originally the notifier_lock only made sure that the CPU page table updates
where done in order and originally the eviction lock made sure that the GPU
page table updates where done in order, but essentially we need the order for
both.
The point is that the updates need to be serialized. In other words when one
CPU is doing a mapping operation and another CPU is doing an unmap through an
MMU notifier we somehow need to make sure that the coresponding GPU page table
updates execute in the correct order.
So essentially those two locks need to be the same one, and we need to drop the
lock to allocate page tables and when we re-acquire it we need to double check
the sequence number to make sure that no unmap operation happened concurrently.
And yeah I know that this makes things much much more complicated, but it is
definately necessary.
So, if I understand your point correctly, the best way to solve the
serialization issue between these two locks is to turn them into a single
lock. Given that a full re-design of amdgpu_vm could take a significant
amount of time, it seems that using drm_gpusvm's notifier_lock as a
replacement for eviction_lock in amdgpu_vm would be the more practical
short-term solution.
Please correct me if I've misunderstood your position.
Thanks,
Ray
Hi Christian,
I made some changes based on your modification to unify the locking,
only use the notifier lock when svm is enabled,
instead of using notifier lock and eviciton lock at the same time.
see diff below, not sure if it is correct, so confirming with you：
static inline int amdgpu_vm_begin_critical(struct
amdgpu_vm_update_params *p)
{
- mutex_lock(&p->vm->eviction_lock);
+ struct amdgpu_vm *vm = p->vm;
+
+ if (vm->svm)
+ down_read(&vm->svm->gpusvm.notifier_lock);
+ else
+ mutex_lock(&vm->eviction_lock);
p->saved_flags = memalloc_noreclaim_save();
- if (p->vm->evicting)
+ if (vm->evicting)
return -EBUSY;
if (p->hmm_range && !amdgpu_hmm_range_valid(p->hmm_range))
return -EAGAIN;
@@ -174,8 +181,13 @@ static inline int amdgpu_vm_begin_critical(struct
amdgpu_vm_update_params *p)
*/
static inline void amdgpu_vm_end_critical(struct
amdgpu_vm_update_params *p)
{
+ struct amdgpu_vm *vm = p->vm;
+
memalloc_noreclaim_restore(p->saved_flags);
- mutex_unlock(&p->vm->eviction_lock);
+ if (vm->svm)
+ up_read(&vm->svm->gpusvm.notifier_lock);
+ else
+ mutex_unlock(&vm->eviction_lock);
}
if above is valid, I have a question that
some places still not using begin/end critical, using vm->eviction_lock
directly, do thoes places need to be changed?
amdgpu_vm_evictable():
scoped_cond_guard(mutex_try, return false, &vm->eviction_lock)
amdgpu_vm_validate():
scoped_guard(mutex, &vm->eviction_lock)
amdgpu_vm_ready()
scoped_guard(mutex, &vm->eviction_lock)
Regards,
Honglei
Regards,
Christian.
Regards,
Honglei
The background is that XE uses a different page table allocation approach than
amdgpu and we need to drop this lock in amdgpu to be able to allocate page
tables. See function amdgpu_vm_pt_alloc().
With that design here that currently doesn't work at all.
We have two options, either use the drm_gpusvm notifier_lock as eviction_lock
in amdgpu_vm.c or re-design amdgpu_vm.c to use the same approach for allocating
page tables as XE.
Some engineer from Valve is working on re-designing amdgpu_vm.c, but that will
potentially take month if not years.
So my take is that the new SVM code needs to modify amdgpu_vm.c so that the
drm_gpusvm notifier_lock is used as eviction lock by the VM code.
Regards,
Christian.
- driver_svm_lock: In addition to the locking mentioned above, the
driver should implement a lock to safeguard core GPU SVM function
calls that modify state, such as drm_gpusvm_range_find_or_insert and
drm_gpusvm_range_remove.
Two locks, two jobs.
2) The lock held in the MMU notifier is notifier_lock, never driver_svm_lock
drm_gpusvm_notifier_invalidate():
down_write(&gpusvm->notifier_lock);
...
gpusvm->ops->invalidate(gpusvm, notifier, mmu_range);
The driver invalidate callback runs under notifier_lock only. Per the
framework's own notifier example it just unmaps pages
and queues the range to the garbage collector no allocation, and it
does not take driver_svm_lock:
drm_gpusvm_range_unmap_pages(...);
drm_gpusvm_range_set_unmapped(...);
driver_garbage_collector_add(...);
3) driver_svm_lock is by design an allocating, process context lock
drm_gpusvm_range_find_or_insert() asserts it and then allocates under it:
drm_gpusvm_range_find_or_insert():
drm_gpusvm_driver_lock_held(gpusvm);
...
range = drm_gpusvm_range_alloc(...);
... mmu_interval_notifier_insert(), kzalloc
drm_gpusvm_range_remove() asserts it and frees. This is only safe
because driver_svm_lock is a sleepable, reclaim friendly lock that is
never taken from the MMU notifier. Reference counting
handles range *lifetime*, but it does not
serialize tree insert/remove, which is exactly why the framework still
asserts driver_svm_lock on those two entry points regardless of refcount.
Now the three concrete points:
A) Why the primary driver_svm_lock is required
It is a framework requirement, not an amdgpu invention:
- DOC: Locking says the driver "should implement" it.
- drm_gpusvm lockdep-asserts it on every structural entry:
drm_gpusvm_range_find_or_insert() and drm_gpusvm_range_remove() both
call drm_gpusvm_driver_lock_held().
- The reference fault handler holds it across the whole fault:
GC -> find_or_insert -> migrate -> get_pages -> bind.
Xe does exactly this:
- xe_svm.c: drm_gpusvm_driver_set_lock(&vm->svm.gpusvm, &vm->lock);
- xe_pagefault.c: down_write(&vm->lock); before dispatching the fault
- __xe_svm_handle_pagefault(): lockdep_assert_held_write(&vm->lock);
held across GC / find_or_insert / alloc_vram / get_pages / rebind
- xe_svm_garbage_collector(): lockdep_assert_held_write(&vm->lock);
amdgpu's svm_lock is the same driver_svm_lock, used the same way.
B) Why eviction_lock cannot be that lock
This lock eviction_lock can only be grabbed while updating the mapping range.
and that is precisely why it cannot be driver_svm_lock.
driver_svm_lock must wrap find_or_insert, migration, and
drm_gpusvm_range_get_pages
eviction_lock is the opposite by contract:
- It is taken with memalloc_noreclaim_save() in
amdgpu_vm_begin_critical(), specifically so no reclaim happens while
held (to avoid the reclaim -> MMU-notifier deadlock). Holding it
across get_pages/migration breaks that.
- TTM eviction try-locks it: amdgpu_vm_evictable() does
scoped_cond_guard(mutex_try, return false, &vm->eviction_lock) and
sets vm->evicting. Long holds starve eviction.
- It is a plain mutex that the SVM map path re-enters:
amdgpu_svm_range_update_mapping() -> amdgpu_vm_map_range() ->
amdgpu_vm_begin_critical() -> mutex_lock(&vm->eviction_lock). If
eviction_lock were also the outer SVM lock, this is a self-deadlock.
In short, eviction_lock has the contract of notifier_lock , not of
driver_svm_lock. This is also why the current split is correct:
svm_lock (outer) != eviction_lock (inner). Your own rule - "you can't
call the VM code with the lock held, the VM code must take it itself" -
is satisfied today only because they are separate: svm_lock is held
while calling amdgpu_vm_map_range(), and amdgpu_vm_map_range() takes
eviction_lock itself. Merging them is what would violate that rule.
No, they Xe vm->lock and eviction_lock are actually identical in the handling.
They are not. Xe's vm->lock is a rw_semaphore, the "outer most lock" of
the VM , held down_write across the whole fault. amdgpu's
eviction_lock is a mutex taken only inside amdgpu_vm_begin_critical()
during a PT update, under memalloc_noreclaim. Xe's eviction/reclaim
handling is separate from vm->lock. The amdgpu analogue of Xe's vm->lock
is svm_lock, not eviction_lock.
C) Reusing an existing amdgpu_vm lock as the primary lock needs refactor amdgpu
VM
Xe can register vm->lock because Xe's VM was designed with an outer
rw_semaphore held across faults. amdgpu_vm has no such lock: only
eviction_lock , the root PD dma_resv , and a few spinlocks.
So do it like Xe means introducing a dedicated, outer, sleepable VM
lock held across the fault. That lock is exactly svm_lock. Folding it
into struct amdgpu_vm as a general vm->lock is a core amdgpu VM refactor.
Regards,
Honglei
Regards,
Christian.
+
+#if IS_ENABLED(CONFIG_DRM_AMDGPU_SVM)
+void amdgpu_svm_flush_tlb(struct amdgpu_svm *svm);
+
+int amdgpu_svm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm);
+void amdgpu_svm_close(struct amdgpu_vm *vm);
+void amdgpu_svm_fini(struct amdgpu_vm *vm);
+
+void amdgpu_svm_put(struct amdgpu_svm *svm);
+struct amdgpu_svm *amdgpu_svm_lookup_by_pasid(struct amdgpu_device *adev,
+ uint32_t pasid);
+int amdgpu_svm_handle_fault(struct amdgpu_device *adev, uint32_t pasid,
+ uint64_t fault_page, uint64_t ts,
+ bool write_fault);
+bool amdgpu_svm_is_enabled(struct amdgpu_vm *vm);
+
+int amdgpu_gem_svm_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *filp);
+void amdgpu_svm_clean_queue(struct amdgpu_svm *svm,
+ struct list_head *work_list);
+void amdgpu_svm_sync_work(struct amdgpu_svm *svm);
+int amdgpu_svm_garbage_collector(struct amdgpu_svm *svm);
+int amdgpu_svm_apply_attr_change(struct amdgpu_svm *svm,
+ const struct amdgpu_svm_attrs *old_attrs,
+ const struct amdgpu_svm_attrs *new_attrs,
+ unsigned long start_page,
+ unsigned long last_page);
+bool amdgpu_svm_devmem_possible(struct amdgpu_svm *svm);
+#else
+static inline int amdgpu_svm_init(struct amdgpu_device *adev,
+ struct amdgpu_vm *vm)
+{
+ return 0;
+}
+
+static inline void amdgpu_svm_close(struct amdgpu_vm *vm)
+{
+}
+
+static inline void amdgpu_svm_fini(struct amdgpu_vm *vm)
+{
+}
+
+static inline int amdgpu_svm_handle_fault(struct amdgpu_device *adev,
+ uint32_t pasid,
+ uint64_t fault_page,
+ uint64_t ts,
+ bool write_fault)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline bool amdgpu_svm_is_enabled(struct amdgpu_vm *vm)
+{
+ return false;
+}
+
+static inline int amdgpu_gem_svm_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *filp)
+{
+ return -EOPNOTSUPP;
+}
+#endif /* CONFIG_DRM_AMDGPU_SVM */
+
+#endif /* __AMDGPU_SVM_H__ */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index ec1196d390bb7..30463a83e2e60 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -43,6 +43,7 @@ struct amdgpu_bo_va;
struct amdgpu_job;
struct amdgpu_bo_list_entry;
struct amdgpu_bo_vm;
+struct amdgpu_svm;
/*
* GPUVM handling
@@ -373,6 +374,9 @@ struct amdgpu_vm {
/* cached fault info */
struct amdgpu_vm_fault_info fault_info;
+
+ /* SVM experimental implementation */
+ struct amdgpu_svm *svm;
};
struct amdgpu_vm_manager {
