On 8/25/26 09:42, Honglei Huang wrote:
> On 8/13/26 17:16, Huang, Honglei wrote:
...
> Hi Christian,
>
> Following up on my previous mail, I changed the change. The diff to the
> existing VM code is below.
>
> Does this look correct to you? for unify notifier lock and eviction lock.
That looks mostly correct to me, but I would change it quite a bit.
>
> diff:
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
> @@ -28,6 +28,7 @@
> #include "amdgpu_hmm.h"
> #include "amdgpu_vm.h"
> +#include "amdgpu_svm.h"
> @@ -66,6 +67,9 @@ struct amdgpu_vm_update_params {
> bool unlocked;
>
> + /** @svm_locked: caller already holds the drm_gpusvm notifier_lock */
> + bool svm_locked;
> +
Drop that, this approach is nonsense. The caller must always hold the
drm_gpusvm lock.
> /**
> * @pages_addr:
> @@ -143,6 +147,30 @@
> +/* SVM VMs serialize eviction under the notifier_lock (write); others use
> eviction_lock. */
> +static inline void amdgpu_vm_eviction_lock(struct amdgpu_vm *vm)
> +{
> + if (amdgpu_svm_is_enabled(vm))
> + down_write(&vm->svm->gpusvm.notifier_lock);
I would either make the vm->svm mandatory or rename vm->eviction_lock to
something like vm->notifier_lock and make it a pointer to the rw_semaphore
which should be used.
Regards,
Christian.
> + else
> + mutex_lock(&vm->eviction_lock);
> +}
> +
> +static inline void amdgpu_vm_eviction_unlock(struct amdgpu_vm *vm)
> +{
> + if (amdgpu_svm_is_enabled(vm))
> + up_write(&vm->svm->gpusvm.notifier_lock);
> + else
> + mutex_unlock(&vm->eviction_lock);
> +}
> +
> +static inline bool amdgpu_vm_eviction_trylock(struct amdgpu_vm *vm)
> +{
> + if (amdgpu_svm_is_enabled(vm))
> + return down_write_trylock(&vm->svm->gpusvm.notifier_lock);
> + return mutex_trylock(&vm->eviction_lock);
> +}
> +
> static inline int amdgpu_vm_begin_critical(struct amdgpu_vm_update_params *p)
> {
> + if (amdgpu_svm_is_enabled(p->vm)) {
> + if (p->svm_locked)
> + lockdep_assert_held(&p->vm->svm->gpusvm.notifier_lock);
> + else
> + down_read(&p->vm->svm->gpusvm.notifier_lock);
> + p->saved_flags = memalloc_noreclaim_save();
> + if (p->vm->evicting)
> + return -EBUSY;
> + return 0;
> + }
> +
> mutex_lock(&p->vm->eviction_lock);
> p->saved_flags = memalloc_noreclaim_save();
> if (p->vm->evicting)
> @@ static inline void amdgpu_vm_end_critical(struct amdgpu_vm_update_params
> *p)
> memalloc_noreclaim_restore(p->saved_flags);
> + if (amdgpu_svm_is_enabled(p->vm)) {
> + if (!p->svm_locked)
> + up_read(&p->vm->svm->gpusvm.notifier_lock);
> + return;
> + }
> mutex_unlock(&p->vm->eviction_lock);
> }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ amdgpu_vm_validate():
> - scoped_guard(mutex, &vm->eviction_lock)
> - vm->evicting = false;
> + amdgpu_vm_eviction_lock(vm);
> + vm->evicting = false;
> + amdgpu_vm_eviction_unlock(vm);
> @@ amdgpu_vm_ready():
> - scoped_guard(mutex, &vm->eviction_lock)
> - ret = !vm->evicting;
> + amdgpu_vm_eviction_lock(vm);
> + ret = !vm->evicting;
> + amdgpu_vm_eviction_unlock(vm);
> @@ amdgpu_vm_evictable():
> - scoped_cond_guard(mutex_try, return false, &vm->eviction_lock) {
> - if (!dma_fence_is_signaled(vm->last_unlocked))
> - return false;
> - vm->evicting = true;
> - }
> + if (!amdgpu_vm_eviction_trylock(vm))
> + return false;
> + if (!dma_fence_is_signaled(vm->last_unlocked)) {
> + amdgpu_vm_eviction_unlock(vm);
> + return false;
> + }
> + vm->evicting = true;
> + amdgpu_vm_eviction_unlock(vm);
> return true;
> @@ amdgpu_vm_map_range() / amdgpu_vm_unmap_range(): /* +bool svm_locked
> param, params.svm_locked = svm_locked; non-SVM callers pass false */
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ /* amdgpu_vm_map_range()/amdgpu_vm_unmap_range() declarations: +bool
> svm_locked */
>
> 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 {
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>
>
