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

[Honglei Huang <honghuan@amd.com>]

On 8/13/26 17:16, Huang, Honglei wrote:

...

>>>
>>
>> So, if I understand your point correctly, the best way to solve the
>> serialization issue between these two locks is to turn them into a single
>> lock. Given that a full re-design of amdgpu_vm could take a significant
>> amount of time, it seems that using drm_gpusvm's notifier_lock as a
>> replacement for eviction_lock in amdgpu_vm would be the more practical
>> short-term solution.
>>
>> Please correct me if I've misunderstood your position.
>>
>> Thanks,
>> Ray
> 
> 
> Hi Christian,
> 
> I made some changes based on your modification to unify the locking,
> only use the notifier lock when svm is enabled,
> instead of using notifier lock and eviciton lock at the same time.
> 
> see diff below, not sure if it is correct, so confirming with you：
> 
> 
>   static inline int amdgpu_vm_begin_critical(struct 
> amdgpu_vm_update_params *p)
>   {
> -    mutex_lock(&p->vm->eviction_lock);
> +    struct amdgpu_vm *vm = p->vm;
> +
> +    if (vm->svm)
> +        down_read(&vm->svm->gpusvm.notifier_lock);
> +    else
> +        mutex_lock(&vm->eviction_lock);
>       p->saved_flags = memalloc_noreclaim_save();
> -    if (p->vm->evicting)
> +    if (vm->evicting)
>           return -EBUSY;
>       if (p->hmm_range && !amdgpu_hmm_range_valid(p->hmm_range))
>           return -EAGAIN;
> @@ -174,8 +181,13 @@ static inline int amdgpu_vm_begin_critical(struct 
> amdgpu_vm_update_params *p)
>    */
>   static inline void amdgpu_vm_end_critical(struct 
> amdgpu_vm_update_params *p)
>   {
> +    struct amdgpu_vm *vm = p->vm;
> +
>       memalloc_noreclaim_restore(p->saved_flags);
> -    mutex_unlock(&p->vm->eviction_lock);
> +    if (vm->svm)
> +        up_read(&vm->svm->gpusvm.notifier_lock);
> +    else
> +        mutex_unlock(&vm->eviction_lock);
>   }
> 
> 
> if above is valid, I have a question that
> some places still not using begin/end critical, using vm->eviction_lock 
> directly, do thoes places need to be changed?
> amdgpu_vm_evictable():
>      scoped_cond_guard(mutex_try, return false, &vm->eviction_lock)
> 
> amdgpu_vm_validate():
>      scoped_guard(mutex, &vm->eviction_lock)
> 
> amdgpu_vm_ready()
>      scoped_guard(mutex, &vm->eviction_lock)
> 
> Regards,
> Honglei
> 

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.

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;
+
      /**
       * @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);
+	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 {
>>>>>>>
>>>>>>
>>>>
>>>
>