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