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