Re: [PATCH v9 02/18] drm/amdgpu: add SVM core header and VM integration

["Christian König" <christian.koenig@amd.com>]

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