- 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 {
