On Wed, Aug 12, 2026 at 02:16:03PM +0200, Christian KKKnig wrote:
> 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.
>
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
> 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 {
> >>>>
> >>>
> >
>
