* [PATCH V1 1/2] drm/gpusvm: add drm_gpusvm_pages unmapped and retry helpers @ 2026-11-06 18:42 Lizhi Hou 2026-11-06 18:42 ` [PATCH V1 2/2] accel/amdxdna: use drm_gpusvm_pages for user mapping tracking Lizhi Hou 2026-11-09 8:56 ` [PATCH V1 1/2] drm/gpusvm: add drm_gpusvm_pages unmapped and retry helpers Huang, Honglei1 0 siblings, 2 replies; 5+ messages in thread From: Lizhi Hou @ 2026-11-06 18:42 UTC (permalink / raw) To: ogabbay, quic_jhugo, dri-devel, mario.limonciello, karol.wachowski, max.zhen, thomas.hellstrom, himal.prasad.ghimiray, honghuan Cc: Matthew Brost, linux-kernel, sonal.santan, Lizhi Hou From: Matthew Brost <matthew.brost@intel.com> Drivers which only use the drm_gpusvm_pages API (no drm_gpusvm_range) have no way to mark pages unmapped from their MMU notifier, as drm_gpusvm_range_set_unmapped() requires a range. They also have to open code the mmu_interval_read_retry() check on the recorded notifier_seq to commit pages faulted with &drm_gpusvm_ctx.no_dma_map. Split the pages part of drm_gpusvm_range_set_unmapped() into drm_gpusvm_pages_set_unmapped(), and add drm_gpusvm_pages_read_retry() which performs the commit side check with a lockdep assert on the notifier lock. Assisted-by: LLM Signed-off-by: Matthew Brost <matthew.brost@intel.com> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com> --- drivers/gpu/drm/drm_gpusvm.c | 49 +++++++++++++++++++++++++----------- include/drm/drm_gpusvm.h | 29 +++++++++++++++++++++ 2 files changed, 64 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c index 03dbe304e4b8f..63a26919f59db 100644 --- a/drivers/gpu/drm/drm_gpusvm.c +++ b/drivers/gpu/drm/drm_gpusvm.c @@ -1761,9 +1761,8 @@ static int drm_gpusvm_dma_map_pages(struct drm_gpusvm *gpusvm, * * With &drm_gpusvm_ctx.no_dma_map no mapping state is recorded, so * drm_gpusvm_pages_valid() never returns true and success is only a snapshot: - * the caller must recheck mmu_interval_read_retry() against the recorded - * &drm_gpusvm_pages.notifier_seq under the notifier lock, and hold it until - * its work is visible to invalidation. + * the caller must recheck drm_gpusvm_pages_read_retry() under the notifier + * lock, and hold it until its work is visible to invalidation. * * Return: 0 on success, negative error code on failure. */ @@ -2021,6 +2020,38 @@ bool drm_gpusvm_has_mapping(struct drm_gpusvm *gpusvm, unsigned long start, } EXPORT_SYMBOL_GPL(drm_gpusvm_has_mapping); +/** + * drm_gpusvm_pages_set_unmapped() - Mark GPU SVM pages as unmapped + * @gpusvm: Pointer to the GPU SVM structure. + * @pages: Pointer to the GPU SVM pages structure(s). + * @pages_count: Number of GPU SVM pages structure(s) passed in. + * + * Mark @pages as unmapped, after which drm_gpusvm_get_pages() fails with + * -EFAULT for them. Intended to be called from the driver's MMU notifier on + * MMU_NOTIFY_UNMAP, and usable without a &drm_gpusvm_range by drivers that + * only use the drm_gpusvm_pages API. The caller must hold the notifier lock + * in write mode. + */ +void drm_gpusvm_pages_set_unmapped(struct drm_gpusvm *gpusvm, + struct drm_gpusvm_pages *pages, + unsigned int pages_count) +{ + unsigned int i; + + lockdep_assert_held_write(&gpusvm->notifier_lock); + + for (i = 0; i < pages_count; ++i) { + struct drm_gpusvm_pages_flags flags = { + .__flags = pages[i].flags.__flags, + }; + + flags.unmapped = true; + /* WRITE_ONCE pairs with READ_ONCE for opportunistic checks */ + WRITE_ONCE(pages[i].flags.__flags, flags.__flags); + } +} +EXPORT_SYMBOL_GPL(drm_gpusvm_pages_set_unmapped); + /** * drm_gpusvm_range_set_unmapped() - Mark a GPU SVM range as unmapped * @range: Pointer to the GPU SVM range structure. @@ -2039,20 +2070,10 @@ void drm_gpusvm_range_set_unmapped(struct drm_gpusvm_range *range, struct drm_gpusvm_range_flags range_flags = { .__flags = range->flags.__flags, }; - unsigned int i; - lockdep_assert_held_write(&range->gpusvm->notifier_lock); + drm_gpusvm_pages_set_unmapped(range->gpusvm, pages, pages_count); range_flags.unmapped = true; - for (i = 0; i < pages_count; ++i) { - struct drm_gpusvm_pages_flags flags = { - .__flags = pages[i].flags.__flags, - }; - - flags.unmapped = true; - /* WRITE_ONCE pairs with READ_ONCE for opportunistic checks */ - WRITE_ONCE(pages[i].flags.__flags, flags.__flags); - } if (drm_gpusvm_range_start(range) < mmu_range->start || drm_gpusvm_range_end(range) > mmu_range->end) range_flags.partial_unmap = true; diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h index 4d31c6bba7455..b43432af56377 100644 --- a/include/drm/drm_gpusvm.h +++ b/include/drm/drm_gpusvm.h @@ -349,6 +349,10 @@ void drm_gpusvm_range_set_unmapped(struct drm_gpusvm_range *range, unsigned int pages_count, const struct mmu_notifier_range *mmu_range); +void drm_gpusvm_pages_set_unmapped(struct drm_gpusvm *gpusvm, + struct drm_gpusvm_pages *pages, + unsigned int pages_count); + int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm, struct drm_gpusvm_pages *svm_pages, unsigned int num_pages, @@ -383,6 +387,31 @@ static inline void drm_gpusvm_init_pages(struct drm_gpusvm_pages *svm_pages, svm_pages->notifier_seq = LONG_MAX; } +/** + * drm_gpusvm_pages_read_retry() - Check whether GPU SVM pages went stale + * @gpusvm: Pointer to the GPU SVM structure. + * @svm_pages: Pointer to the drm_gpusvm_pages. + * @notifier: The notifier passed to drm_gpusvm_get_pages() for @svm_pages. + * + * Check whether @notifier was invalidated since the last successful + * drm_gpusvm_get_pages() for @svm_pages. This is the commit side check for + * pages faulted with &drm_gpusvm_ctx.no_dma_map, where no mapping state is + * tracked. Pages that were never faulted always report as stale. The caller + * must hold the notifier lock and keep holding it until its use of the pages + * is visible to the invalidation path. + * + * Return: True if the pages must be faulted again, false otherwise. + */ +static inline bool +drm_gpusvm_pages_read_retry(struct drm_gpusvm *gpusvm, + struct drm_gpusvm_pages *svm_pages, + struct mmu_interval_notifier *notifier) +{ + lockdep_assert_held(&gpusvm->notifier_lock); + + return mmu_interval_read_retry(notifier, svm_pages->notifier_seq); +} + /** * drm_gpusvm_pages_first_dma() - Resolve the device address array * @svm_pages: Pointer to the drm_gpusvm_pages. base-commit: d2a8cee91bfa8616f713c7c8f539a81e8e3554ea -- 2.34.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH V1 2/2] accel/amdxdna: use drm_gpusvm_pages for user mapping tracking 2026-11-06 18:42 [PATCH V1 1/2] drm/gpusvm: add drm_gpusvm_pages unmapped and retry helpers Lizhi Hou @ 2026-11-06 18:42 ` Lizhi Hou 2026-11-09 19:44 ` Max Zhen 2026-11-09 8:56 ` [PATCH V1 1/2] drm/gpusvm: add drm_gpusvm_pages unmapped and retry helpers Huang, Honglei1 1 sibling, 1 reply; 5+ messages in thread From: Lizhi Hou @ 2026-11-06 18:42 UTC (permalink / raw) To: ogabbay, quic_jhugo, dri-devel, mario.limonciello, karol.wachowski, max.zhen, thomas.hellstrom, himal.prasad.ghimiray, honghuan Cc: Matthew Brost, linux-kernel, sonal.santan, Lizhi Hou From: Matthew Brost <matthew.brost@intel.com> In PASID mode the device accesses user mappings of a BO through the IOMMU, but cannot take page faults, so the driver faults the CPU pages in with HMM before submitting a job and waits for the job from the MMU notifier. The HMM PFNs themselves are never used. The open coded implementation of this has a few problems: - Each mapping embeds a struct hmm_range, and keeps the PFN array it points at allocated for the lifetime of the mapping, although the PFNs are dropped as soon as the fault returns. - The per mapping invalid flag duplicates what the notifier sequence number already tracks, and must be kept in sync with it. - The notifier lock is taken in write mode on the populate path. - Submission locks all reservations, finds the first BO with an invalidated mapping, unlocks everything, faults that single BO in and starts over, once per invalidated BO. drm_gpusvm already implements the fault and retry loop, including faulting without a DMA mapping (&drm_gpusvm_ctx.no_dma_map), and its pages only mode does not require the driver to use drm_gpusvm ranges. Use it: - Replace the device notifier_lock with a pages only struct drm_gpusvm and its notifier lock. - Replace the embedded hmm_range and the invalid/unmapped flags with a struct drm_gpusvm_pages. A mapping is stale when drm_gpusvm_pages_read_retry() says so and it is not unmapped. - Fault in all stale mappings with drm_gpusvm_get_pages() before locking the reservations, then check that none went stale again under the notifier lock, which is held until the job fences are published, as before. - The invalidate callback only bumps the sequence number and marks the mapping unmapped on MMU_NOTIFY_UNMAP under the write lock, before waiting for the jobs. A temporary PFN array is now allocated per fault instead of keeping one per mapping, which also removes the race above. Assisted-by: LLM Signed-off-by: Matthew Brost <matthew.brost@intel.com> [Lizhi: Merge with latest code] Signed-off-by: Lizhi Hou <lizhi.hou@amd.com> --- drivers/accel/amdxdna/Kconfig | 2 +- drivers/accel/amdxdna/aie2_ctx.c | 31 +-- drivers/accel/amdxdna/aie4_ctx.c | 29 +-- drivers/accel/amdxdna/amdxdna_gem.c | 312 +++++++++++++----------- drivers/accel/amdxdna/amdxdna_gem.h | 23 +- drivers/accel/amdxdna/amdxdna_pci_drv.c | 15 +- drivers/accel/amdxdna/amdxdna_pci_drv.h | 3 +- 7 files changed, 213 insertions(+), 202 deletions(-) diff --git a/drivers/accel/amdxdna/Kconfig b/drivers/accel/amdxdna/Kconfig index eddff490b4f75..3721eb580b71a 100644 --- a/drivers/accel/amdxdna/Kconfig +++ b/drivers/accel/amdxdna/Kconfig @@ -7,10 +7,10 @@ config DRM_ACCEL_AMDXDNA depends on PCI && HAS_IOMEM depends on X86_64 depends on AMD_PMF || !AMD_PMF + select DRM_GPUSVM select DRM_SCHED select DRM_GEM_SHMEM_HELPER select FW_LOADER - select HMM_MIRROR help Choose this option to enable support for NPU integrated into AMD client CPUs like AMD Ryzen AI 300 Series. AMD NPU can be used to diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c index e1c0e0689b12e..552c30ea6e387 100644 --- a/drivers/accel/amdxdna/aie2_ctx.c +++ b/drivers/accel/amdxdna/aie2_ctx.c @@ -1116,8 +1116,7 @@ int aie2_cmd_submit(struct amdxdna_hwctx *hwctx, struct amdxdna_sched_job *job, struct amdxdna_dev *xdna = hwctx->client->xdna; struct ww_acquire_ctx acquire_ctx; struct dma_fence_chain *chain; - struct amdxdna_gem_obj *abo; - unsigned long timeout = 0; + unsigned long deadline; int ret, i; ret = down_interruptible(&hwctx->priv->job_sem); @@ -1140,7 +1139,15 @@ int aie2_cmd_submit(struct amdxdna_hwctx *hwctx, struct amdxdna_sched_job *job, goto free_chain; } + deadline = jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT); retry: + for (i = 0; i < job->bo_cnt; i++) { + ret = amdxdna_gem_umap_populate(to_xdna_obj(job->bos[i]), + deadline); + if (ret) + goto cleanup_job; + } + ret = drm_gem_lock_reservations(job->bos, job->bo_cnt, &acquire_ctx); if (ret) { XDNA_WARN(xdna, "Failed to lock BOs, ret %d", ret); @@ -1156,23 +1163,11 @@ int aie2_cmd_submit(struct amdxdna_hwctx *hwctx, struct amdxdna_sched_job *job, } } - down_read(&xdna->notifier_lock); + drm_gpusvm_notifier_lock(&xdna->gpusvm); for (i = 0; i < job->bo_cnt; i++) { - abo = to_xdna_obj(job->bos[i]); - if (abo->mem.map_invalid) { - up_read(&xdna->notifier_lock); + if (amdxdna_gem_umap_stale(to_xdna_obj(job->bos[i]))) { + drm_gpusvm_notifier_unlock(&xdna->gpusvm); drm_gem_unlock_reservations(job->bos, job->bo_cnt, &acquire_ctx); - if (!timeout) { - timeout = jiffies + - msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT); - } else if (time_after(jiffies, timeout)) { - ret = -ETIME; - goto cleanup_job; - } - - ret = amdxdna_populate_range(abo); - if (ret) - goto cleanup_job; goto retry; } } @@ -1192,7 +1187,7 @@ int aie2_cmd_submit(struct amdxdna_hwctx *hwctx, struct amdxdna_sched_job *job, drm_syncobj_add_point(hwctx->priv->syncobj, chain, job->out_fence, *seq); mutex_unlock(&hwctx->priv->io_lock); - up_read(&xdna->notifier_lock); + drm_gpusvm_notifier_unlock(&xdna->gpusvm); drm_gem_unlock_reservations(job->bos, job->bo_cnt, &acquire_ctx); aie2_job_put(job); diff --git a/drivers/accel/amdxdna/aie4_ctx.c b/drivers/accel/amdxdna/aie4_ctx.c index a3f0a69b93b75..0db7dcdf106ba 100644 --- a/drivers/accel/amdxdna/aie4_ctx.c +++ b/drivers/accel/amdxdna/aie4_ctx.c @@ -1059,8 +1059,7 @@ static int fence_job_bos(struct amdxdna_dev *xdna, struct amdxdna_sched_job *job { struct ww_acquire_ctx acquire_ctx; struct drm_gem_object **objs; - struct amdxdna_gem_obj *abo; - unsigned long timeout = 0; + unsigned long deadline; u32 cnt = 0; int ret; u32 i; @@ -1076,7 +1075,13 @@ static int fence_job_bos(struct amdxdna_dev *xdna, struct amdxdna_sched_job *job for (i = 0; i < job->aie4_cmd_bo_cnt; i++) add_lock_obj(objs, &cnt, to_gobj(job->aie4_cmd_bos[i])); + deadline = jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT); retry: + for (i = 0; i < cnt; i++) { + ret = amdxdna_gem_umap_populate(to_xdna_obj(objs[i]), deadline); + if (ret) + goto free_objs; + } ret = drm_gem_lock_reservations(objs, cnt, &acquire_ctx); if (ret) { XDNA_WARN(xdna, "Failed to lock BOs, ret %d", ret); @@ -1091,23 +1096,11 @@ static int fence_job_bos(struct amdxdna_dev *xdna, struct amdxdna_sched_job *job } } - down_read(&xdna->notifier_lock); + drm_gpusvm_notifier_lock(&xdna->gpusvm); for (i = 0; i < cnt; i++) { - abo = to_xdna_obj(objs[i]); - if (abo->mem.map_invalid) { - up_read(&xdna->notifier_lock); + if (amdxdna_gem_umap_stale(to_xdna_obj(objs[i]))) { + drm_gpusvm_notifier_unlock(&xdna->gpusvm); drm_gem_unlock_reservations(objs, cnt, &acquire_ctx); - if (!timeout) { - timeout = jiffies + - msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT); - } else if (time_after(jiffies, timeout)) { - ret = -ETIME; - goto free_objs; - } - - ret = amdxdna_populate_range(abo); - if (ret) - goto free_objs; goto retry; } } @@ -1115,7 +1108,7 @@ static int fence_job_bos(struct amdxdna_dev *xdna, struct amdxdna_sched_job *job job->out_fence = dma_fence_get(job->fence); for (i = 0; i < cnt; i++) dma_resv_add_fence(objs[i]->resv, job->out_fence, DMA_RESV_USAGE_WRITE); - up_read(&xdna->notifier_lock); + drm_gpusvm_notifier_unlock(&xdna->gpusvm); unlock: drm_gem_unlock_reservations(objs, cnt, &acquire_ctx); diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c index 09e48c794e1b0..57d87fa98d671 100644 --- a/drivers/accel/amdxdna/amdxdna_gem.c +++ b/drivers/accel/amdxdna/amdxdna_gem.c @@ -232,37 +232,46 @@ static bool amdxdna_hmm_invalidate(struct mmu_interval_notifier *mni, { struct amdxdna_umap *mapp = container_of(mni, struct amdxdna_umap, notifier); struct amdxdna_gem_obj *abo = mapp->abo; - struct amdxdna_dev *xdna; + struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev); + bool unreg; if (!mmu_notifier_range_blockable(range)) return false; - if (mapp->unmapped) - return true; - - xdna = to_xdna_dev(to_gobj(abo)->dev); XDNA_DBG(xdna, "Invalidating range 0x%lx, 0x%lx, type %d", - mapp->range.start, mapp->range.end, abo->type); + mapp->start, mapp->end, abo->type); - down_write(&xdna->notifier_lock); - abo->mem.map_invalid = true; - mapp->invalid = true; - mmu_interval_set_seq(&mapp->notifier, cur_seq); - up_write(&xdna->notifier_lock); + down_write(&xdna->gpusvm.notifier_lock); + mmu_interval_set_seq(mni, cur_seq); + /* + * Flag the mapping as gone before the wait below, so that a submission + * racing with the unmap does not try to fault it in again. + */ + unreg = !mapp->pages.flags.unmapped && range->event == MMU_NOTIFY_UNMAP; + if (unreg) + drm_gpusvm_pages_set_unmapped(&xdna->gpusvm, &mapp->pages, 1); + up_write(&xdna->gpusvm.notifier_lock); /* * The pages must not be released while the device may still access * them, so this wait must be neither interruptible nor time limited. + * + * Every invalidation of the mapping has to wait, including the ones + * which find it already flagged: the flag is set above before the wait, + * so it does not tell whether the jobs of a concurrent invalidation + * have finished. */ dma_resv_wait_timeout(to_gobj(abo)->resv, DMA_RESV_USAGE_BOOKKEEP, false, MAX_SCHEDULE_TIMEOUT); - if (range->event == MMU_NOTIFY_UNMAP) { - down_write(&xdna->notifier_lock); - mapp->unmapped = true; + /* + * Queue the unregistration only once the jobs are done: it drops the + * last reference of the mapping, and mmu_interval_notifier_remove() + * waits for this callback, which would pin the single worker of the + * ordered WQ_MEM_RECLAIM notifier_wq for as long as a job runs. + */ + if (unreg) queue_work(xdna->notifier_wq, &abo->hmm_unreg_work); - up_write(&xdna->notifier_lock); - } return true; } @@ -271,12 +280,25 @@ static const struct mmu_interval_notifier_ops amdxdna_hmm_ops = { .invalidate = amdxdna_hmm_invalidate, }; -static inline bool compare_range(struct amdxdna_umap *mapp, - struct mm_struct *mm, - unsigned long start, unsigned long end) +static bool amdxdna_umap_match(struct amdxdna_umap *mapp, + struct mm_struct *mm, + unsigned long start, unsigned long end) { - return (!mapp->unmapped && mapp->notifier.mm == mm && - mapp->range.start == start && mapp->range.end == end); + return !mapp->pages.flags.unmapped && mapp->notifier.mm == mm && + mapp->start == start && mapp->end == end; +} + +static bool amdxdna_umap_stale(struct drm_gpusvm *gpusvm, + struct amdxdna_umap *mapp) +{ + /* + * The pages of a BO whose mappings mmap() populated upfront are never + * faulted in. Neither are the ones of a mapping which is gone: device + * accesses through it are rejected by the IOMMU, so there is nothing + * to fault in for it. + */ + return mapp->abo->mem.map_invalid && !mapp->pages.flags.unmapped && + drm_gpusvm_pages_read_retry(gpusvm, &mapp->pages, &mapp->notifier); } static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo, @@ -285,15 +307,28 @@ static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo, struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev); struct amdxdna_umap *mapp; - down_write(&xdna->notifier_lock); + down_write(&xdna->gpusvm.notifier_lock); list_for_each_entry(mapp, &abo->mem.umap_list, node) { - if (!compare_range(mapp, vma->vm_mm, vma->vm_start, vma->vm_end)) + if (!amdxdna_umap_match(mapp, vma->vm_mm, vma->vm_start, vma->vm_end)) continue; - mapp->unmapped = true; + drm_gpusvm_pages_set_unmapped(&xdna->gpusvm, &mapp->pages, 1); queue_work(xdna->notifier_wq, &abo->hmm_unreg_work); } - up_write(&xdna->notifier_lock); + up_write(&xdna->gpusvm.notifier_lock); +} + +static void amdxdna_umap_release(struct kref *ref) +{ + struct amdxdna_umap *mapp = container_of(ref, struct amdxdna_umap, refcnt); + + mmu_interval_notifier_remove(&mapp->notifier); + kfree(mapp); +} + +static void amdxdna_umap_put(struct amdxdna_umap *mapp) +{ + kref_put(&mapp->refcnt, amdxdna_umap_release); } static void amdxdna_hmm_unreg_umaps(struct amdxdna_gem_obj *abo, bool force) @@ -302,121 +337,119 @@ static void amdxdna_hmm_unreg_umaps(struct amdxdna_gem_obj *abo, bool force) struct amdxdna_umap *mapp, *tmp; LIST_HEAD(dead); - down_write(&xdna->notifier_lock); + down_write(&xdna->gpusvm.notifier_lock); list_for_each_entry_safe(mapp, tmp, &abo->mem.umap_list, node) { - if (!force && !mapp->unmapped) + if (!force && !mapp->pages.flags.unmapped) continue; - mapp->unmapped = true; + drm_gpusvm_pages_set_unmapped(&xdna->gpusvm, &mapp->pages, 1); list_move(&mapp->node, &dead); } if (list_empty(&abo->mem.umap_list)) abo->mem.uva = AMDXDNA_INVALID_ADDR; - up_write(&xdna->notifier_lock); + up_write(&xdna->gpusvm.notifier_lock); list_for_each_entry_safe(mapp, tmp, &dead, node) amdxdna_umap_put(mapp); } -static void amdxdna_umap_release(struct kref *ref) +static void amdxdna_hmm_unreg_work(struct work_struct *work) { - struct amdxdna_umap *mapp = container_of(ref, struct amdxdna_umap, refcnt); - - mmu_interval_notifier_remove(&mapp->notifier); - mutex_destroy(&mapp->lock); - kvfree(mapp->range.hmm_pfns); - kfree(mapp); -} + struct amdxdna_gem_obj *abo = container_of(work, struct amdxdna_gem_obj, + hmm_unreg_work); -void amdxdna_umap_put(struct amdxdna_umap *mapp) -{ - kref_put(&mapp->refcnt, amdxdna_umap_release); + amdxdna_hmm_unreg_umaps(abo, false); } -int amdxdna_populate_range(struct amdxdna_gem_obj *abo) +/** + * amdxdna_gem_umap_populate() - Fault in the invalidated user mappings of a BO + * @abo: The BO. + * @deadline: Jiffies after which to give up. + * + * The device accesses user mappings through the PASID and cannot take page + * faults, so the CPU pages of every mapping of @abo invalidated since it was + * last faulted are faulted in again here. This is only a snapshot, the caller + * must check amdxdna_gem_umap_stale() under the notifier lock before using + * the mappings and retry if it fails. + * + * Two hardware contexts may fault the same mapping at the same time. That is + * left unserialized: with &drm_gpusvm_ctx.no_dma_map the only state recorded + * for a mapping is its notifier sequence, and the loser of such a race can + * only store an older sequence, which makes amdxdna_gem_umap_stale() report + * stale and the caller fault again. + * + * Return: 0 on success, -ETIME if @deadline passed, negative error code on + * other failures. + */ +int amdxdna_gem_umap_populate(struct amdxdna_gem_obj *abo, + unsigned long deadline) { struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev); + struct drm_gpusvm *gpusvm = &xdna->gpusvm; struct amdxdna_umap *mapp; - unsigned long timeout; - struct mm_struct *mm; - bool found; + bool found, unmapped; int ret; - timeout = msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT); -again: - found = false; - down_write(&xdna->notifier_lock); - list_for_each_entry(mapp, &abo->mem.umap_list, node) { - /* - * Skip entries that have already been unmapped. - * - * If userspace unmaps the address and later submits I/O using - * it, the IOMMU will reject the access and report a fault. - * Ignore such entries here. - */ - if (mapp->unmapped) - continue; + for (;;) { + struct drm_gpusvm_ctx ctx = { .no_dma_map = 1 }; - if (mapp->invalid && kref_get_unless_zero(&mapp->refcnt)) { - found = true; - break; + found = false; + drm_gpusvm_notifier_lock(gpusvm); + list_for_each_entry(mapp, &abo->mem.umap_list, node) { + if (amdxdna_umap_stale(gpusvm, mapp) && + kref_get_unless_zero(&mapp->refcnt)) { + found = true; + break; + } } - } - - if (!found) { - /* - * This also covers the case where all mappings have been - * removed. There are no invalid mappings left to process. - * Any subsequent I/O using the unmapped address will be - * rejected by the IOMMU. - */ - abo->mem.map_invalid = false; - up_write(&xdna->notifier_lock); - return 0; - } - - up_write(&xdna->notifier_lock); - - mm = mapp->notifier.mm; - if (!mmget_not_zero(mm)) { - amdxdna_umap_put(mapp); - return -EFAULT; - } + drm_gpusvm_notifier_unlock(gpusvm); + if (!found) + return 0; - mutex_lock(&mapp->lock); - ret = hmm_range_fault_unlocked_timeout(&mapp->range, timeout); - if (ret) { - mutex_unlock(&mapp->lock); - goto put_mm; - } + if (time_after(jiffies, deadline)) { + amdxdna_umap_put(mapp); + return -ETIME; + } - down_write(&xdna->notifier_lock); - if (mmu_interval_read_retry(&mapp->notifier, mapp->range.notifier_seq)) { - up_write(&xdna->notifier_lock); - mutex_unlock(&mapp->lock); + ctx.read_only = mapp->read_only; + ret = drm_gpusvm_get_pages(gpusvm, &mapp->pages, 1, + mapp->notifier.mm, &mapp->notifier, + mapp->start, mapp->end, &ctx); + if (ret == -EFAULT) { + drm_gpusvm_notifier_lock(gpusvm); + unmapped = mapp->pages.flags.unmapped; + drm_gpusvm_notifier_unlock(gpusvm); + if (unmapped) + ret = 0; + } amdxdna_umap_put(mapp); - mmput(mm); - goto again; + if (ret) + return ret == -EBUSY ? -ETIME : ret; } - mapp->invalid = false; - up_write(&xdna->notifier_lock); - mutex_unlock(&mapp->lock); - amdxdna_umap_put(mapp); - mmput(mm); - goto again; - -put_mm: - amdxdna_umap_put(mapp); - mmput(mm); - return ret == -EBUSY ? -ETIME : ret; } -static void amdxdna_hmm_unreg_work(struct work_struct *work) +/** + * amdxdna_gem_umap_stale() - Check whether a BO needs its user mappings faulted + * @abo: The BO. + * + * Must be called with the notifier lock held, which must be kept held until + * the fences of the jobs using @abo are published in its reservation object. + * + * Return: True if amdxdna_gem_umap_populate() must be called for @abo. + */ +bool amdxdna_gem_umap_stale(struct amdxdna_gem_obj *abo) { - struct amdxdna_gem_obj *abo = container_of(work, struct amdxdna_gem_obj, - hmm_unreg_work); + struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev); + struct amdxdna_umap *mapp; - amdxdna_hmm_unreg_umaps(abo, false); + lockdep_assert_held(&xdna->gpusvm.notifier_lock); + + list_for_each_entry(mapp, &abo->mem.umap_list, node) { + if (amdxdna_umap_stale(&xdna->gpusvm, mapp)) + return true; + } + + return false; } static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo, @@ -426,7 +459,6 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo, unsigned long len = vma->vm_end - vma->vm_start; unsigned long addr = vma->vm_start; struct amdxdna_umap *mapp; - unsigned long nr_pages; int ret; /* @@ -441,38 +473,34 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo, return 0; } - down_read(&xdna->notifier_lock); + down_read(&xdna->gpusvm.notifier_lock); list_for_each_entry(mapp, &abo->mem.umap_list, node) { - if (compare_range(mapp, current->mm, addr, addr + len)) { - up_read(&xdna->notifier_lock); + if (amdxdna_umap_match(mapp, current->mm, addr, addr + len)) { + up_read(&xdna->gpusvm.notifier_lock); return 0; } } - up_read(&xdna->notifier_lock); + up_read(&xdna->gpusvm.notifier_lock); mapp = kzalloc_obj(*mapp); if (!mapp) return -ENOMEM; - nr_pages = (PAGE_ALIGN(addr + len) - (addr & PAGE_MASK)) >> PAGE_SHIFT; - mapp->range.hmm_pfns = kvzalloc_objs(*mapp->range.hmm_pfns, nr_pages); - if (!mapp->range.hmm_pfns) { - ret = -ENOMEM; - goto free_map; - } + /* + * Never faulted, so stale until the first amdxdna_gem_umap_populate(), + * should the pages of the BO have to be faulted in at all. + */ + drm_gpusvm_init_pages(&mapp->pages, &xdna->ddev); + mapp->start = addr; + mapp->end = addr + len; - mapp->range.notifier = &mapp->notifier; - mapp->range.start = vma->vm_start; - mapp->range.end = vma->vm_end; /* * Access permissions are fixed at mmap() time. Changing them later - * with mprotect() is not supported: the range keeps requesting the + * with mprotect() is not supported: the mapping keeps requesting the * original permissions, so the application may see a fault failure * or an IOMMU fault. */ - mapp->range.default_flags = HMM_PFN_REQ_FAULT; - if (vma->vm_flags & VM_WRITE) - mapp->range.default_flags |= HMM_PFN_REQ_WRITE; + mapp->read_only = !(vma->vm_flags & VM_WRITE); mapp->abo = abo; kref_init(&mapp->refcnt); @@ -483,23 +511,17 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo, &amdxdna_hmm_ops); if (ret) { XDNA_ERR(xdna, "Insert mmu notifier failed, ret %d", ret); - goto free_pfns; + kfree(mapp); + return ret; } - mutex_init(&mapp->lock); - down_write(&xdna->notifier_lock); + down_write(&xdna->gpusvm.notifier_lock); if (list_empty(&abo->mem.umap_list)) abo->mem.uva = addr; list_add_tail(&mapp->node, &abo->mem.umap_list); - up_write(&xdna->notifier_lock); + up_write(&xdna->gpusvm.notifier_lock); return 0; - -free_pfns: - kvfree(mapp->range.hmm_pfns); -free_map: - kfree(mapp); - return ret; } static struct amdxdna_gem_obj * @@ -547,21 +569,13 @@ static void amdxdna_gem_dev_obj_free(struct drm_gem_object *gobj) amdxdna_gem_destroy_obj(abo); } -static void amdxdna_mark_mapp_invalid(struct amdxdna_gem_obj *abo, - struct vm_area_struct *vma) +static void amdxdna_mark_map_invalid(struct amdxdna_gem_obj *abo) { struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev); - struct amdxdna_umap *mapp; - down_write(&xdna->notifier_lock); + down_write(&xdna->gpusvm.notifier_lock); abo->mem.map_invalid = true; - list_for_each_entry(mapp, &abo->mem.umap_list, node) { - if (compare_range(mapp, vma->vm_mm, vma->vm_start, vma->vm_end)) { - mapp->invalid = true; - break; - } - } - up_write(&xdna->notifier_lock); + up_write(&xdna->gpusvm.notifier_lock); } static int amdxdna_remap_pages(struct amdxdna_gem_obj *abo, @@ -628,7 +642,7 @@ static int amdxdna_insert_pages(struct amdxdna_gem_obj *abo, } if (!(vma->vm_flags & VM_PFNMAP)) - amdxdna_mark_mapp_invalid(abo, vma); + amdxdna_mark_map_invalid(abo); /* Drop the reference drm_gem_mmap_obj() acquired.*/ drm_gem_object_put(to_gobj(abo)); diff --git a/drivers/accel/amdxdna/amdxdna_gem.h b/drivers/accel/amdxdna/amdxdna_gem.h index 5c429869992e8..2deead107d6b1 100644 --- a/drivers/accel/amdxdna/amdxdna_gem.h +++ b/drivers/accel/amdxdna/amdxdna_gem.h @@ -7,19 +7,19 @@ #define _AMDXDNA_GEM_H_ #include <drm/drm_gem_shmem_helper.h> -#include <linux/hmm.h> +#include <drm/drm_gpusvm.h> #include <linux/iova.h> #include "amdxdna_pci_drv.h" struct amdxdna_umap { struct mmu_interval_notifier notifier; - struct hmm_range range; + struct drm_gpusvm_pages pages; + unsigned long start; + unsigned long end; + bool read_only; struct amdxdna_gem_obj *abo; struct list_head node; struct kref refcnt; - bool invalid; - bool unmapped; - struct mutex lock; /* protect range */ }; struct amdxdna_mem { @@ -27,10 +27,16 @@ struct amdxdna_mem { u64 dma_addr; size_t size; struct list_head umap_list; + /* + * True, if the pages of the user mappings have to be faulted in before + * the device accesses them. A mapping which mmap() populated upfront, + * which is a VM_PFNMAP one, must not be faulted in: hmm_range_fault() + * rejects such a VMA. + */ bool map_invalid; /* * Cache the first mmap uva as PASID addr, which can be accessed by driver - * without taking notifier_lock. + * without taking the gpusvm notifier lock. */ u64 uva; }; @@ -103,8 +109,9 @@ static inline u64 amdxdna_obj_dma_addr(struct amdxdna_gem_obj *abo) abo->mem.dma_addr : amdxdna_gem_uva(abo); } -void amdxdna_umap_put(struct amdxdna_umap *mapp); -int amdxdna_populate_range(struct amdxdna_gem_obj *abo); +int amdxdna_gem_umap_populate(struct amdxdna_gem_obj *abo, + unsigned long deadline); +bool amdxdna_gem_umap_stale(struct amdxdna_gem_obj *abo); void amdxdna_gem_heap_free(struct amdxdna_client *client, struct amdxdna_gem_obj *abo); struct drm_gem_object * diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.c b/drivers/accel/amdxdna/amdxdna_pci_drv.c index f233eb70a2957..18bd7bdd12969 100644 --- a/drivers/accel/amdxdna/amdxdna_pci_drv.c +++ b/drivers/accel/amdxdna/amdxdna_pci_drv.c @@ -363,6 +363,7 @@ static void amdxdna_xdna_drm_release(struct drm_device *drm, void *res) struct amdxdna_dev *xdna = res; amdxdna_carveout_fini(xdna); + drm_gpusvm_fini(&xdna->gpusvm); } static int amdxdna_probe(struct pci_dev *pdev, const struct pci_device_id *id) @@ -389,18 +390,18 @@ static int amdxdna_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (ret) return ret; - init_rwsem(&xdna->notifier_lock); + ret = drm_gpusvm_init(&xdna->gpusvm, "amdxdna", NULL, 0, 0, 0, NULL, + NULL, 0); + if (ret) + return ret; + INIT_LIST_HEAD(&xdna->client_list); pci_set_drvdata(pdev, xdna); ret = drmm_add_action(ddev, amdxdna_xdna_drm_release, xdna); - if (ret) + if (ret) { + drm_gpusvm_fini(&xdna->gpusvm); return ret; - - if (IS_ENABLED(CONFIG_LOCKDEP)) { - fs_reclaim_acquire(GFP_KERNEL); - might_lock(&xdna->notifier_lock); - fs_reclaim_release(GFP_KERNEL); } ret = amdxdna_iommu_init(xdna); diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.h b/drivers/accel/amdxdna/amdxdna_pci_drv.h index 27593190dcf67..7663b883d4f8b 100644 --- a/drivers/accel/amdxdna/amdxdna_pci_drv.h +++ b/drivers/accel/amdxdna/amdxdna_pci_drv.h @@ -7,6 +7,7 @@ #define _AMDXDNA_PCI_DRV_H_ #include <drm/amdxdna_accel.h> +#include <drm/drm_gpusvm.h> #include <drm/drm_mm.h> #include <drm/drm_print.h> #include <linux/iommu.h> @@ -118,7 +119,7 @@ struct amdxdna_dev { struct list_head client_list; struct mutex client_lock; /* client_list */ struct amdxdna_drm_query_firmware_version fw_ver; - struct rw_semaphore notifier_lock; /* for mmu notifier*/ + struct drm_gpusvm gpusvm; /* notifier lock for user mappings */ struct workqueue_struct *notifier_wq; struct iommu_group *group; -- 2.34.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V1 2/2] accel/amdxdna: use drm_gpusvm_pages for user mapping tracking 2026-11-06 18:42 ` [PATCH V1 2/2] accel/amdxdna: use drm_gpusvm_pages for user mapping tracking Lizhi Hou @ 2026-11-09 19:44 ` Max Zhen 0 siblings, 0 replies; 5+ messages in thread From: Max Zhen @ 2026-11-09 19:44 UTC (permalink / raw) To: Lizhi Hou, ogabbay, quic_jhugo, dri-devel, mario.limonciello, karol.wachowski, thomas.hellstrom, himal.prasad.ghimiray, honghuan Cc: Matthew Brost, linux-kernel, sonal.santan On 11/6/2026 Fri 10:42, Lizhi Hou wrote: > From: Matthew Brost <matthew.brost@intel.com> > > In PASID mode the device accesses user mappings of a BO through the > IOMMU, but cannot take page faults, so the driver faults the CPU pages > in with HMM before submitting a job and waits for the job from the MMU > notifier. The HMM PFNs themselves are never used. > > The open coded implementation of this has a few problems: > > - Each mapping embeds a struct hmm_range, and keeps the PFN array it > points at allocated for the lifetime of the mapping, although the > PFNs are dropped as soon as the fault returns. > - The per mapping invalid flag duplicates what the notifier sequence > number already tracks, and must be kept in sync with it. > - The notifier lock is taken in write mode on the populate path. > - Submission locks all reservations, finds the first BO with an > invalidated mapping, unlocks everything, faults that single BO in and > starts over, once per invalidated BO. > > drm_gpusvm already implements the fault and retry loop, including > faulting without a DMA mapping (&drm_gpusvm_ctx.no_dma_map), and its > pages only mode does not require the driver to use drm_gpusvm ranges. > Use it: > > - Replace the device notifier_lock with a pages only struct drm_gpusvm > and its notifier lock. > - Replace the embedded hmm_range and the invalid/unmapped flags with a > struct drm_gpusvm_pages. A mapping is stale when > drm_gpusvm_pages_read_retry() says so and it is not unmapped. > - Fault in all stale mappings with drm_gpusvm_get_pages() before > locking the reservations, then check that none went stale again > under the notifier lock, which is held until the job fences are > published, as before. > - The invalidate callback only bumps the sequence number and marks the > mapping unmapped on MMU_NOTIFY_UNMAP under the write lock, before > waiting for the jobs. > > A temporary PFN array is now allocated per fault instead of keeping one > per mapping, which also removes the race above. > > Assisted-by: LLM > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > [Lizhi: Merge with latest code] > Signed-off-by: Lizhi Hou <lizhi.hou@amd.com> Reviewed-by: Max Zhen <max.zhen@amd.com> > --- > drivers/accel/amdxdna/Kconfig | 2 +- > drivers/accel/amdxdna/aie2_ctx.c | 31 +-- > drivers/accel/amdxdna/aie4_ctx.c | 29 +-- > drivers/accel/amdxdna/amdxdna_gem.c | 312 +++++++++++++----------- > drivers/accel/amdxdna/amdxdna_gem.h | 23 +- > drivers/accel/amdxdna/amdxdna_pci_drv.c | 15 +- > drivers/accel/amdxdna/amdxdna_pci_drv.h | 3 +- > 7 files changed, 213 insertions(+), 202 deletions(-) > > diff --git a/drivers/accel/amdxdna/Kconfig b/drivers/accel/amdxdna/Kconfig > index eddff490b4f75..3721eb580b71a 100644 > --- a/drivers/accel/amdxdna/Kconfig > +++ b/drivers/accel/amdxdna/Kconfig > @@ -7,10 +7,10 @@ config DRM_ACCEL_AMDXDNA > depends on PCI && HAS_IOMEM > depends on X86_64 > depends on AMD_PMF || !AMD_PMF > + select DRM_GPUSVM > select DRM_SCHED > select DRM_GEM_SHMEM_HELPER > select FW_LOADER > - select HMM_MIRROR > help > Choose this option to enable support for NPU integrated into AMD > client CPUs like AMD Ryzen AI 300 Series. AMD NPU can be used to > diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c > index e1c0e0689b12e..552c30ea6e387 100644 > --- a/drivers/accel/amdxdna/aie2_ctx.c > +++ b/drivers/accel/amdxdna/aie2_ctx.c > @@ -1116,8 +1116,7 @@ int aie2_cmd_submit(struct amdxdna_hwctx *hwctx, struct amdxdna_sched_job *job, > struct amdxdna_dev *xdna = hwctx->client->xdna; > struct ww_acquire_ctx acquire_ctx; > struct dma_fence_chain *chain; > - struct amdxdna_gem_obj *abo; > - unsigned long timeout = 0; > + unsigned long deadline; > int ret, i; > > ret = down_interruptible(&hwctx->priv->job_sem); > @@ -1140,7 +1139,15 @@ int aie2_cmd_submit(struct amdxdna_hwctx *hwctx, struct amdxdna_sched_job *job, > goto free_chain; > } > > + deadline = jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT); > retry: > + for (i = 0; i < job->bo_cnt; i++) { > + ret = amdxdna_gem_umap_populate(to_xdna_obj(job->bos[i]), > + deadline); > + if (ret) > + goto cleanup_job; > + } > + > ret = drm_gem_lock_reservations(job->bos, job->bo_cnt, &acquire_ctx); > if (ret) { > XDNA_WARN(xdna, "Failed to lock BOs, ret %d", ret); > @@ -1156,23 +1163,11 @@ int aie2_cmd_submit(struct amdxdna_hwctx *hwctx, struct amdxdna_sched_job *job, > } > } > > - down_read(&xdna->notifier_lock); > + drm_gpusvm_notifier_lock(&xdna->gpusvm); > for (i = 0; i < job->bo_cnt; i++) { > - abo = to_xdna_obj(job->bos[i]); > - if (abo->mem.map_invalid) { > - up_read(&xdna->notifier_lock); > + if (amdxdna_gem_umap_stale(to_xdna_obj(job->bos[i]))) { > + drm_gpusvm_notifier_unlock(&xdna->gpusvm); > drm_gem_unlock_reservations(job->bos, job->bo_cnt, &acquire_ctx); > - if (!timeout) { > - timeout = jiffies + > - msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT); > - } else if (time_after(jiffies, timeout)) { > - ret = -ETIME; > - goto cleanup_job; > - } > - > - ret = amdxdna_populate_range(abo); > - if (ret) > - goto cleanup_job; > goto retry; > } > } > @@ -1192,7 +1187,7 @@ int aie2_cmd_submit(struct amdxdna_hwctx *hwctx, struct amdxdna_sched_job *job, > drm_syncobj_add_point(hwctx->priv->syncobj, chain, job->out_fence, *seq); > mutex_unlock(&hwctx->priv->io_lock); > > - up_read(&xdna->notifier_lock); > + drm_gpusvm_notifier_unlock(&xdna->gpusvm); > drm_gem_unlock_reservations(job->bos, job->bo_cnt, &acquire_ctx); > > aie2_job_put(job); > diff --git a/drivers/accel/amdxdna/aie4_ctx.c b/drivers/accel/amdxdna/aie4_ctx.c > index a3f0a69b93b75..0db7dcdf106ba 100644 > --- a/drivers/accel/amdxdna/aie4_ctx.c > +++ b/drivers/accel/amdxdna/aie4_ctx.c > @@ -1059,8 +1059,7 @@ static int fence_job_bos(struct amdxdna_dev *xdna, struct amdxdna_sched_job *job > { > struct ww_acquire_ctx acquire_ctx; > struct drm_gem_object **objs; > - struct amdxdna_gem_obj *abo; > - unsigned long timeout = 0; > + unsigned long deadline; > u32 cnt = 0; > int ret; > u32 i; > @@ -1076,7 +1075,13 @@ static int fence_job_bos(struct amdxdna_dev *xdna, struct amdxdna_sched_job *job > for (i = 0; i < job->aie4_cmd_bo_cnt; i++) > add_lock_obj(objs, &cnt, to_gobj(job->aie4_cmd_bos[i])); > > + deadline = jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT); > retry: > + for (i = 0; i < cnt; i++) { > + ret = amdxdna_gem_umap_populate(to_xdna_obj(objs[i]), deadline); > + if (ret) > + goto free_objs; > + } > ret = drm_gem_lock_reservations(objs, cnt, &acquire_ctx); > if (ret) { > XDNA_WARN(xdna, "Failed to lock BOs, ret %d", ret); > @@ -1091,23 +1096,11 @@ static int fence_job_bos(struct amdxdna_dev *xdna, struct amdxdna_sched_job *job > } > } > > - down_read(&xdna->notifier_lock); > + drm_gpusvm_notifier_lock(&xdna->gpusvm); > for (i = 0; i < cnt; i++) { > - abo = to_xdna_obj(objs[i]); > - if (abo->mem.map_invalid) { > - up_read(&xdna->notifier_lock); > + if (amdxdna_gem_umap_stale(to_xdna_obj(objs[i]))) { > + drm_gpusvm_notifier_unlock(&xdna->gpusvm); > drm_gem_unlock_reservations(objs, cnt, &acquire_ctx); > - if (!timeout) { > - timeout = jiffies + > - msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT); > - } else if (time_after(jiffies, timeout)) { > - ret = -ETIME; > - goto free_objs; > - } > - > - ret = amdxdna_populate_range(abo); > - if (ret) > - goto free_objs; > goto retry; > } > } > @@ -1115,7 +1108,7 @@ static int fence_job_bos(struct amdxdna_dev *xdna, struct amdxdna_sched_job *job > job->out_fence = dma_fence_get(job->fence); > for (i = 0; i < cnt; i++) > dma_resv_add_fence(objs[i]->resv, job->out_fence, DMA_RESV_USAGE_WRITE); > - up_read(&xdna->notifier_lock); > + drm_gpusvm_notifier_unlock(&xdna->gpusvm); > > unlock: > drm_gem_unlock_reservations(objs, cnt, &acquire_ctx); > diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c > index 09e48c794e1b0..57d87fa98d671 100644 > --- a/drivers/accel/amdxdna/amdxdna_gem.c > +++ b/drivers/accel/amdxdna/amdxdna_gem.c > @@ -232,37 +232,46 @@ static bool amdxdna_hmm_invalidate(struct mmu_interval_notifier *mni, > { > struct amdxdna_umap *mapp = container_of(mni, struct amdxdna_umap, notifier); > struct amdxdna_gem_obj *abo = mapp->abo; > - struct amdxdna_dev *xdna; > + struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev); > + bool unreg; > > if (!mmu_notifier_range_blockable(range)) > return false; > > - if (mapp->unmapped) > - return true; > - > - xdna = to_xdna_dev(to_gobj(abo)->dev); > XDNA_DBG(xdna, "Invalidating range 0x%lx, 0x%lx, type %d", > - mapp->range.start, mapp->range.end, abo->type); > + mapp->start, mapp->end, abo->type); > > - down_write(&xdna->notifier_lock); > - abo->mem.map_invalid = true; > - mapp->invalid = true; > - mmu_interval_set_seq(&mapp->notifier, cur_seq); > - up_write(&xdna->notifier_lock); > + down_write(&xdna->gpusvm.notifier_lock); > + mmu_interval_set_seq(mni, cur_seq); > + /* > + * Flag the mapping as gone before the wait below, so that a submission > + * racing with the unmap does not try to fault it in again. > + */ > + unreg = !mapp->pages.flags.unmapped && range->event == MMU_NOTIFY_UNMAP; > + if (unreg) > + drm_gpusvm_pages_set_unmapped(&xdna->gpusvm, &mapp->pages, 1); > + up_write(&xdna->gpusvm.notifier_lock); > > /* > * The pages must not be released while the device may still access > * them, so this wait must be neither interruptible nor time limited. > + * > + * Every invalidation of the mapping has to wait, including the ones > + * which find it already flagged: the flag is set above before the wait, > + * so it does not tell whether the jobs of a concurrent invalidation > + * have finished. > */ > dma_resv_wait_timeout(to_gobj(abo)->resv, DMA_RESV_USAGE_BOOKKEEP, > false, MAX_SCHEDULE_TIMEOUT); > > - if (range->event == MMU_NOTIFY_UNMAP) { > - down_write(&xdna->notifier_lock); > - mapp->unmapped = true; > + /* > + * Queue the unregistration only once the jobs are done: it drops the > + * last reference of the mapping, and mmu_interval_notifier_remove() > + * waits for this callback, which would pin the single worker of the > + * ordered WQ_MEM_RECLAIM notifier_wq for as long as a job runs. > + */ > + if (unreg) > queue_work(xdna->notifier_wq, &abo->hmm_unreg_work); > - up_write(&xdna->notifier_lock); > - } > > return true; > } > @@ -271,12 +280,25 @@ static const struct mmu_interval_notifier_ops amdxdna_hmm_ops = { > .invalidate = amdxdna_hmm_invalidate, > }; > > -static inline bool compare_range(struct amdxdna_umap *mapp, > - struct mm_struct *mm, > - unsigned long start, unsigned long end) > +static bool amdxdna_umap_match(struct amdxdna_umap *mapp, > + struct mm_struct *mm, > + unsigned long start, unsigned long end) > { > - return (!mapp->unmapped && mapp->notifier.mm == mm && > - mapp->range.start == start && mapp->range.end == end); > + return !mapp->pages.flags.unmapped && mapp->notifier.mm == mm && > + mapp->start == start && mapp->end == end; > +} > + > +static bool amdxdna_umap_stale(struct drm_gpusvm *gpusvm, > + struct amdxdna_umap *mapp) > +{ > + /* > + * The pages of a BO whose mappings mmap() populated upfront are never > + * faulted in. Neither are the ones of a mapping which is gone: device > + * accesses through it are rejected by the IOMMU, so there is nothing > + * to fault in for it. > + */ > + return mapp->abo->mem.map_invalid && !mapp->pages.flags.unmapped && > + drm_gpusvm_pages_read_retry(gpusvm, &mapp->pages, &mapp->notifier); > } > > static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo, > @@ -285,15 +307,28 @@ static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo, > struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev); > struct amdxdna_umap *mapp; > > - down_write(&xdna->notifier_lock); > + down_write(&xdna->gpusvm.notifier_lock); > list_for_each_entry(mapp, &abo->mem.umap_list, node) { > - if (!compare_range(mapp, vma->vm_mm, vma->vm_start, vma->vm_end)) > + if (!amdxdna_umap_match(mapp, vma->vm_mm, vma->vm_start, vma->vm_end)) > continue; > > - mapp->unmapped = true; > + drm_gpusvm_pages_set_unmapped(&xdna->gpusvm, &mapp->pages, 1); > queue_work(xdna->notifier_wq, &abo->hmm_unreg_work); > } > - up_write(&xdna->notifier_lock); > + up_write(&xdna->gpusvm.notifier_lock); > +} > + > +static void amdxdna_umap_release(struct kref *ref) > +{ > + struct amdxdna_umap *mapp = container_of(ref, struct amdxdna_umap, refcnt); > + > + mmu_interval_notifier_remove(&mapp->notifier); > + kfree(mapp); > +} > + > +static void amdxdna_umap_put(struct amdxdna_umap *mapp) > +{ > + kref_put(&mapp->refcnt, amdxdna_umap_release); > } > > static void amdxdna_hmm_unreg_umaps(struct amdxdna_gem_obj *abo, bool force) > @@ -302,121 +337,119 @@ static void amdxdna_hmm_unreg_umaps(struct amdxdna_gem_obj *abo, bool force) > struct amdxdna_umap *mapp, *tmp; > LIST_HEAD(dead); > > - down_write(&xdna->notifier_lock); > + down_write(&xdna->gpusvm.notifier_lock); > list_for_each_entry_safe(mapp, tmp, &abo->mem.umap_list, node) { > - if (!force && !mapp->unmapped) > + if (!force && !mapp->pages.flags.unmapped) > continue; > > - mapp->unmapped = true; > + drm_gpusvm_pages_set_unmapped(&xdna->gpusvm, &mapp->pages, 1); > list_move(&mapp->node, &dead); > } > if (list_empty(&abo->mem.umap_list)) > abo->mem.uva = AMDXDNA_INVALID_ADDR; > - up_write(&xdna->notifier_lock); > + up_write(&xdna->gpusvm.notifier_lock); > > list_for_each_entry_safe(mapp, tmp, &dead, node) > amdxdna_umap_put(mapp); > } > > -static void amdxdna_umap_release(struct kref *ref) > +static void amdxdna_hmm_unreg_work(struct work_struct *work) > { > - struct amdxdna_umap *mapp = container_of(ref, struct amdxdna_umap, refcnt); > - > - mmu_interval_notifier_remove(&mapp->notifier); > - mutex_destroy(&mapp->lock); > - kvfree(mapp->range.hmm_pfns); > - kfree(mapp); > -} > + struct amdxdna_gem_obj *abo = container_of(work, struct amdxdna_gem_obj, > + hmm_unreg_work); > > -void amdxdna_umap_put(struct amdxdna_umap *mapp) > -{ > - kref_put(&mapp->refcnt, amdxdna_umap_release); > + amdxdna_hmm_unreg_umaps(abo, false); > } > > -int amdxdna_populate_range(struct amdxdna_gem_obj *abo) > +/** > + * amdxdna_gem_umap_populate() - Fault in the invalidated user mappings of a BO > + * @abo: The BO. > + * @deadline: Jiffies after which to give up. > + * > + * The device accesses user mappings through the PASID and cannot take page > + * faults, so the CPU pages of every mapping of @abo invalidated since it was > + * last faulted are faulted in again here. This is only a snapshot, the caller > + * must check amdxdna_gem_umap_stale() under the notifier lock before using > + * the mappings and retry if it fails. > + * > + * Two hardware contexts may fault the same mapping at the same time. That is > + * left unserialized: with &drm_gpusvm_ctx.no_dma_map the only state recorded > + * for a mapping is its notifier sequence, and the loser of such a race can > + * only store an older sequence, which makes amdxdna_gem_umap_stale() report > + * stale and the caller fault again. > + * > + * Return: 0 on success, -ETIME if @deadline passed, negative error code on > + * other failures. > + */ > +int amdxdna_gem_umap_populate(struct amdxdna_gem_obj *abo, > + unsigned long deadline) > { > struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev); > + struct drm_gpusvm *gpusvm = &xdna->gpusvm; > struct amdxdna_umap *mapp; > - unsigned long timeout; > - struct mm_struct *mm; > - bool found; > + bool found, unmapped; > int ret; > > - timeout = msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT); > -again: > - found = false; > - down_write(&xdna->notifier_lock); > - list_for_each_entry(mapp, &abo->mem.umap_list, node) { > - /* > - * Skip entries that have already been unmapped. > - * > - * If userspace unmaps the address and later submits I/O using > - * it, the IOMMU will reject the access and report a fault. > - * Ignore such entries here. > - */ > - if (mapp->unmapped) > - continue; > + for (;;) { > + struct drm_gpusvm_ctx ctx = { .no_dma_map = 1 }; > > - if (mapp->invalid && kref_get_unless_zero(&mapp->refcnt)) { > - found = true; > - break; > + found = false; > + drm_gpusvm_notifier_lock(gpusvm); > + list_for_each_entry(mapp, &abo->mem.umap_list, node) { > + if (amdxdna_umap_stale(gpusvm, mapp) && > + kref_get_unless_zero(&mapp->refcnt)) { > + found = true; > + break; > + } > } > - } > - > - if (!found) { > - /* > - * This also covers the case where all mappings have been > - * removed. There are no invalid mappings left to process. > - * Any subsequent I/O using the unmapped address will be > - * rejected by the IOMMU. > - */ > - abo->mem.map_invalid = false; > - up_write(&xdna->notifier_lock); > - return 0; > - } > - > - up_write(&xdna->notifier_lock); > - > - mm = mapp->notifier.mm; > - if (!mmget_not_zero(mm)) { > - amdxdna_umap_put(mapp); > - return -EFAULT; > - } > + drm_gpusvm_notifier_unlock(gpusvm); > + if (!found) > + return 0; > > - mutex_lock(&mapp->lock); > - ret = hmm_range_fault_unlocked_timeout(&mapp->range, timeout); > - if (ret) { > - mutex_unlock(&mapp->lock); > - goto put_mm; > - } > + if (time_after(jiffies, deadline)) { > + amdxdna_umap_put(mapp); > + return -ETIME; > + } > > - down_write(&xdna->notifier_lock); > - if (mmu_interval_read_retry(&mapp->notifier, mapp->range.notifier_seq)) { > - up_write(&xdna->notifier_lock); > - mutex_unlock(&mapp->lock); > + ctx.read_only = mapp->read_only; > + ret = drm_gpusvm_get_pages(gpusvm, &mapp->pages, 1, > + mapp->notifier.mm, &mapp->notifier, > + mapp->start, mapp->end, &ctx); > + if (ret == -EFAULT) { > + drm_gpusvm_notifier_lock(gpusvm); > + unmapped = mapp->pages.flags.unmapped; > + drm_gpusvm_notifier_unlock(gpusvm); > + if (unmapped) > + ret = 0; > + } > amdxdna_umap_put(mapp); > - mmput(mm); > - goto again; > + if (ret) > + return ret == -EBUSY ? -ETIME : ret; > } > - mapp->invalid = false; > - up_write(&xdna->notifier_lock); > - mutex_unlock(&mapp->lock); > - amdxdna_umap_put(mapp); > - mmput(mm); > - goto again; > - > -put_mm: > - amdxdna_umap_put(mapp); > - mmput(mm); > - return ret == -EBUSY ? -ETIME : ret; > } > > -static void amdxdna_hmm_unreg_work(struct work_struct *work) > +/** > + * amdxdna_gem_umap_stale() - Check whether a BO needs its user mappings faulted > + * @abo: The BO. > + * > + * Must be called with the notifier lock held, which must be kept held until > + * the fences of the jobs using @abo are published in its reservation object. > + * > + * Return: True if amdxdna_gem_umap_populate() must be called for @abo. > + */ > +bool amdxdna_gem_umap_stale(struct amdxdna_gem_obj *abo) > { > - struct amdxdna_gem_obj *abo = container_of(work, struct amdxdna_gem_obj, > - hmm_unreg_work); > + struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev); > + struct amdxdna_umap *mapp; > > - amdxdna_hmm_unreg_umaps(abo, false); > + lockdep_assert_held(&xdna->gpusvm.notifier_lock); > + > + list_for_each_entry(mapp, &abo->mem.umap_list, node) { > + if (amdxdna_umap_stale(&xdna->gpusvm, mapp)) > + return true; > + } > + > + return false; > } > > static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo, > @@ -426,7 +459,6 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo, > unsigned long len = vma->vm_end - vma->vm_start; > unsigned long addr = vma->vm_start; > struct amdxdna_umap *mapp; > - unsigned long nr_pages; > int ret; > > /* > @@ -441,38 +473,34 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo, > return 0; > } > > - down_read(&xdna->notifier_lock); > + down_read(&xdna->gpusvm.notifier_lock); > list_for_each_entry(mapp, &abo->mem.umap_list, node) { > - if (compare_range(mapp, current->mm, addr, addr + len)) { > - up_read(&xdna->notifier_lock); > + if (amdxdna_umap_match(mapp, current->mm, addr, addr + len)) { > + up_read(&xdna->gpusvm.notifier_lock); > return 0; > } > } > - up_read(&xdna->notifier_lock); > + up_read(&xdna->gpusvm.notifier_lock); > > mapp = kzalloc_obj(*mapp); > if (!mapp) > return -ENOMEM; > > - nr_pages = (PAGE_ALIGN(addr + len) - (addr & PAGE_MASK)) >> PAGE_SHIFT; > - mapp->range.hmm_pfns = kvzalloc_objs(*mapp->range.hmm_pfns, nr_pages); > - if (!mapp->range.hmm_pfns) { > - ret = -ENOMEM; > - goto free_map; > - } > + /* > + * Never faulted, so stale until the first amdxdna_gem_umap_populate(), > + * should the pages of the BO have to be faulted in at all. > + */ > + drm_gpusvm_init_pages(&mapp->pages, &xdna->ddev); > + mapp->start = addr; > + mapp->end = addr + len; > > - mapp->range.notifier = &mapp->notifier; > - mapp->range.start = vma->vm_start; > - mapp->range.end = vma->vm_end; > /* > * Access permissions are fixed at mmap() time. Changing them later > - * with mprotect() is not supported: the range keeps requesting the > + * with mprotect() is not supported: the mapping keeps requesting the > * original permissions, so the application may see a fault failure > * or an IOMMU fault. > */ > - mapp->range.default_flags = HMM_PFN_REQ_FAULT; > - if (vma->vm_flags & VM_WRITE) > - mapp->range.default_flags |= HMM_PFN_REQ_WRITE; > + mapp->read_only = !(vma->vm_flags & VM_WRITE); > mapp->abo = abo; > kref_init(&mapp->refcnt); > > @@ -483,23 +511,17 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo, > &amdxdna_hmm_ops); > if (ret) { > XDNA_ERR(xdna, "Insert mmu notifier failed, ret %d", ret); > - goto free_pfns; > + kfree(mapp); > + return ret; > } > > - mutex_init(&mapp->lock); > - down_write(&xdna->notifier_lock); > + down_write(&xdna->gpusvm.notifier_lock); > if (list_empty(&abo->mem.umap_list)) > abo->mem.uva = addr; > list_add_tail(&mapp->node, &abo->mem.umap_list); > - up_write(&xdna->notifier_lock); > + up_write(&xdna->gpusvm.notifier_lock); > > return 0; > - > -free_pfns: > - kvfree(mapp->range.hmm_pfns); > -free_map: > - kfree(mapp); > - return ret; > } > > static struct amdxdna_gem_obj * > @@ -547,21 +569,13 @@ static void amdxdna_gem_dev_obj_free(struct drm_gem_object *gobj) > amdxdna_gem_destroy_obj(abo); > } > > -static void amdxdna_mark_mapp_invalid(struct amdxdna_gem_obj *abo, > - struct vm_area_struct *vma) > +static void amdxdna_mark_map_invalid(struct amdxdna_gem_obj *abo) > { > struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev); > - struct amdxdna_umap *mapp; > > - down_write(&xdna->notifier_lock); > + down_write(&xdna->gpusvm.notifier_lock); > abo->mem.map_invalid = true; > - list_for_each_entry(mapp, &abo->mem.umap_list, node) { > - if (compare_range(mapp, vma->vm_mm, vma->vm_start, vma->vm_end)) { > - mapp->invalid = true; > - break; > - } > - } > - up_write(&xdna->notifier_lock); > + up_write(&xdna->gpusvm.notifier_lock); > } > > static int amdxdna_remap_pages(struct amdxdna_gem_obj *abo, > @@ -628,7 +642,7 @@ static int amdxdna_insert_pages(struct amdxdna_gem_obj *abo, > } > > if (!(vma->vm_flags & VM_PFNMAP)) > - amdxdna_mark_mapp_invalid(abo, vma); > + amdxdna_mark_map_invalid(abo); > > /* Drop the reference drm_gem_mmap_obj() acquired.*/ > drm_gem_object_put(to_gobj(abo)); > diff --git a/drivers/accel/amdxdna/amdxdna_gem.h b/drivers/accel/amdxdna/amdxdna_gem.h > index 5c429869992e8..2deead107d6b1 100644 > --- a/drivers/accel/amdxdna/amdxdna_gem.h > +++ b/drivers/accel/amdxdna/amdxdna_gem.h > @@ -7,19 +7,19 @@ > #define _AMDXDNA_GEM_H_ > > #include <drm/drm_gem_shmem_helper.h> > -#include <linux/hmm.h> > +#include <drm/drm_gpusvm.h> > #include <linux/iova.h> > #include "amdxdna_pci_drv.h" > > struct amdxdna_umap { > struct mmu_interval_notifier notifier; > - struct hmm_range range; > + struct drm_gpusvm_pages pages; > + unsigned long start; > + unsigned long end; > + bool read_only; > struct amdxdna_gem_obj *abo; > struct list_head node; > struct kref refcnt; > - bool invalid; > - bool unmapped; > - struct mutex lock; /* protect range */ > }; > > struct amdxdna_mem { > @@ -27,10 +27,16 @@ struct amdxdna_mem { > u64 dma_addr; > size_t size; > struct list_head umap_list; > + /* > + * True, if the pages of the user mappings have to be faulted in before > + * the device accesses them. A mapping which mmap() populated upfront, > + * which is a VM_PFNMAP one, must not be faulted in: hmm_range_fault() > + * rejects such a VMA. > + */ > bool map_invalid; > /* > * Cache the first mmap uva as PASID addr, which can be accessed by driver > - * without taking notifier_lock. > + * without taking the gpusvm notifier lock. > */ > u64 uva; > }; > @@ -103,8 +109,9 @@ static inline u64 amdxdna_obj_dma_addr(struct amdxdna_gem_obj *abo) > abo->mem.dma_addr : amdxdna_gem_uva(abo); > } > > -void amdxdna_umap_put(struct amdxdna_umap *mapp); > -int amdxdna_populate_range(struct amdxdna_gem_obj *abo); > +int amdxdna_gem_umap_populate(struct amdxdna_gem_obj *abo, > + unsigned long deadline); > +bool amdxdna_gem_umap_stale(struct amdxdna_gem_obj *abo); > void amdxdna_gem_heap_free(struct amdxdna_client *client, struct amdxdna_gem_obj *abo); > > struct drm_gem_object * > diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.c b/drivers/accel/amdxdna/amdxdna_pci_drv.c > index f233eb70a2957..18bd7bdd12969 100644 > --- a/drivers/accel/amdxdna/amdxdna_pci_drv.c > +++ b/drivers/accel/amdxdna/amdxdna_pci_drv.c > @@ -363,6 +363,7 @@ static void amdxdna_xdna_drm_release(struct drm_device *drm, void *res) > struct amdxdna_dev *xdna = res; > > amdxdna_carveout_fini(xdna); > + drm_gpusvm_fini(&xdna->gpusvm); > } > > static int amdxdna_probe(struct pci_dev *pdev, const struct pci_device_id *id) > @@ -389,18 +390,18 @@ static int amdxdna_probe(struct pci_dev *pdev, const struct pci_device_id *id) > if (ret) > return ret; > > - init_rwsem(&xdna->notifier_lock); > + ret = drm_gpusvm_init(&xdna->gpusvm, "amdxdna", NULL, 0, 0, 0, NULL, > + NULL, 0); > + if (ret) > + return ret; > + > INIT_LIST_HEAD(&xdna->client_list); > pci_set_drvdata(pdev, xdna); > > ret = drmm_add_action(ddev, amdxdna_xdna_drm_release, xdna); > - if (ret) > + if (ret) { > + drm_gpusvm_fini(&xdna->gpusvm); > return ret; > - > - if (IS_ENABLED(CONFIG_LOCKDEP)) { > - fs_reclaim_acquire(GFP_KERNEL); > - might_lock(&xdna->notifier_lock); > - fs_reclaim_release(GFP_KERNEL); > } > > ret = amdxdna_iommu_init(xdna); > diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.h b/drivers/accel/amdxdna/amdxdna_pci_drv.h > index 27593190dcf67..7663b883d4f8b 100644 > --- a/drivers/accel/amdxdna/amdxdna_pci_drv.h > +++ b/drivers/accel/amdxdna/amdxdna_pci_drv.h > @@ -7,6 +7,7 @@ > #define _AMDXDNA_PCI_DRV_H_ > > #include <drm/amdxdna_accel.h> > +#include <drm/drm_gpusvm.h> > #include <drm/drm_mm.h> > #include <drm/drm_print.h> > #include <linux/iommu.h> > @@ -118,7 +119,7 @@ struct amdxdna_dev { > struct list_head client_list; > struct mutex client_lock; /* client_list */ > struct amdxdna_drm_query_firmware_version fw_ver; > - struct rw_semaphore notifier_lock; /* for mmu notifier*/ > + struct drm_gpusvm gpusvm; /* notifier lock for user mappings */ > struct workqueue_struct *notifier_wq; > > struct iommu_group *group; ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH V1 1/2] drm/gpusvm: add drm_gpusvm_pages unmapped and retry helpers 2026-11-06 18:42 [PATCH V1 1/2] drm/gpusvm: add drm_gpusvm_pages unmapped and retry helpers Lizhi Hou 2026-11-06 18:42 ` [PATCH V1 2/2] accel/amdxdna: use drm_gpusvm_pages for user mapping tracking Lizhi Hou @ 2026-11-09 8:56 ` Huang, Honglei1 2026-11-09 19:53 ` Lizhi Hou 1 sibling, 1 reply; 5+ messages in thread From: Huang, Honglei1 @ 2026-11-09 8:56 UTC (permalink / raw) To: Hou, Lizhi Cc: Matthew Brost, linux-kernel, Santan, Sonal, Hou, Lizhi, ogabbay, quic_jhugo, dri-devel, Limonciello, Mario, karol.wachowski, Zhen, Max, thomas.hellstrom, himal.prasad.ghimiray AMD General Reviewed-by: Honglei Huang <honghuan@amd.com> -----Original Message----- From: Lizhi Hou <lizhi.hou@amd.com> Sent: Saturday, November 7, 2026 2:42 AM To: ogabbay@kernel.org; quic_jhugo@quicinc.com; dri-devel@lists.freedesktop.org; Limonciello, Mario <Mario.Limonciello@amd.com>; karol.wachowski@linux.intel.com; Zhen, Max <Max.Zhen@amd.com>; thomas.hellstrom@linux.intel.com; himal.prasad.ghimiray@intel.com; Huang, Honglei1 <Honglei1.Huang@amd.com> Cc: Matthew Brost <matthew.brost@intel.com>; linux-kernel@vger.kernel.org; Santan, Sonal <sonal.santan@amd.com>; Hou, Lizhi <lizhi.hou@amd.com> Subject: [PATCH V1 1/2] drm/gpusvm: add drm_gpusvm_pages unmapped and retry helpers From: Matthew Brost <matthew.brost@intel.com> Drivers which only use the drm_gpusvm_pages API (no drm_gpusvm_range) have no way to mark pages unmapped from their MMU notifier, as drm_gpusvm_range_set_unmapped() requires a range. They also have to open code the mmu_interval_read_retry() check on the recorded notifier_seq to commit pages faulted with &drm_gpusvm_ctx.no_dma_map. Split the pages part of drm_gpusvm_range_set_unmapped() into drm_gpusvm_pages_set_unmapped(), and add drm_gpusvm_pages_read_retry() which performs the commit side check with a lockdep assert on the notifier lock. Assisted-by: LLM Signed-off-by: Matthew Brost <matthew.brost@intel.com> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com> --- drivers/gpu/drm/drm_gpusvm.c | 49 +++++++++++++++++++++++++----------- include/drm/drm_gpusvm.h | 29 +++++++++++++++++++++ 2 files changed, 64 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c index 03dbe304e4b8f..63a26919f59db 100644 --- a/drivers/gpu/drm/drm_gpusvm.c +++ b/drivers/gpu/drm/drm_gpusvm.c @@ -1761,9 +1761,8 @@ static int drm_gpusvm_dma_map_pages(struct drm_gpusvm *gpusvm, * * With &drm_gpusvm_ctx.no_dma_map no mapping state is recorded, so * drm_gpusvm_pages_valid() never returns true and success is only a snapshot: - * the caller must recheck mmu_interval_read_retry() against the recorded - * &drm_gpusvm_pages.notifier_seq under the notifier lock, and hold it until - * its work is visible to invalidation. + * the caller must recheck drm_gpusvm_pages_read_retry() under the + notifier + * lock, and hold it until its work is visible to invalidation. * * Return: 0 on success, negative error code on failure. */ @@ -2021,6 +2020,38 @@ bool drm_gpusvm_has_mapping(struct drm_gpusvm *gpusvm, unsigned long start, } EXPORT_SYMBOL_GPL(drm_gpusvm_has_mapping); +/** + * drm_gpusvm_pages_set_unmapped() - Mark GPU SVM pages as unmapped + * @gpusvm: Pointer to the GPU SVM structure. + * @pages: Pointer to the GPU SVM pages structure(s). + * @pages_count: Number of GPU SVM pages structure(s) passed in. + * + * Mark @pages as unmapped, after which drm_gpusvm_get_pages() fails +with + * -EFAULT for them. Intended to be called from the driver's MMU +notifier on + * MMU_NOTIFY_UNMAP, and usable without a &drm_gpusvm_range by drivers +that + * only use the drm_gpusvm_pages API. The caller must hold the notifier +lock + * in write mode. + */ +void drm_gpusvm_pages_set_unmapped(struct drm_gpusvm *gpusvm, + struct drm_gpusvm_pages *pages, + unsigned int pages_count) +{ + unsigned int i; + + lockdep_assert_held_write(&gpusvm->notifier_lock); + + for (i = 0; i < pages_count; ++i) { + struct drm_gpusvm_pages_flags flags = { + .__flags = pages[i].flags.__flags, + }; + + flags.unmapped = true; + /* WRITE_ONCE pairs with READ_ONCE for opportunistic checks */ + WRITE_ONCE(pages[i].flags.__flags, flags.__flags); + } +} +EXPORT_SYMBOL_GPL(drm_gpusvm_pages_set_unmapped); + /** * drm_gpusvm_range_set_unmapped() - Mark a GPU SVM range as unmapped * @range: Pointer to the GPU SVM range structure. @@ -2039,20 +2070,10 @@ void drm_gpusvm_range_set_unmapped(struct drm_gpusvm_range *range, struct drm_gpusvm_range_flags range_flags = { .__flags = range->flags.__flags, }; - unsigned int i; - lockdep_assert_held_write(&range->gpusvm->notifier_lock); + drm_gpusvm_pages_set_unmapped(range->gpusvm, pages, pages_count); range_flags.unmapped = true; - for (i = 0; i < pages_count; ++i) { - struct drm_gpusvm_pages_flags flags = { - .__flags = pages[i].flags.__flags, - }; - - flags.unmapped = true; - /* WRITE_ONCE pairs with READ_ONCE for opportunistic checks */ - WRITE_ONCE(pages[i].flags.__flags, flags.__flags); - } if (drm_gpusvm_range_start(range) < mmu_range->start || drm_gpusvm_range_end(range) > mmu_range->end) range_flags.partial_unmap = true; diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h index 4d31c6bba7455..b43432af56377 100644 --- a/include/drm/drm_gpusvm.h +++ b/include/drm/drm_gpusvm.h @@ -349,6 +349,10 @@ void drm_gpusvm_range_set_unmapped(struct drm_gpusvm_range *range, unsigned int pages_count, const struct mmu_notifier_range *mmu_range); +void drm_gpusvm_pages_set_unmapped(struct drm_gpusvm *gpusvm, + struct drm_gpusvm_pages *pages, + unsigned int pages_count); + int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm, struct drm_gpusvm_pages *svm_pages, unsigned int num_pages, @@ -383,6 +387,31 @@ static inline void drm_gpusvm_init_pages(struct drm_gpusvm_pages *svm_pages, svm_pages->notifier_seq = LONG_MAX; } +/** + * drm_gpusvm_pages_read_retry() - Check whether GPU SVM pages went +stale + * @gpusvm: Pointer to the GPU SVM structure. + * @svm_pages: Pointer to the drm_gpusvm_pages. + * @notifier: The notifier passed to drm_gpusvm_get_pages() for @svm_pages. + * + * Check whether @notifier was invalidated since the last successful + * drm_gpusvm_get_pages() for @svm_pages. This is the commit side check +for + * pages faulted with &drm_gpusvm_ctx.no_dma_map, where no mapping +state is + * tracked. Pages that were never faulted always report as stale. The +caller + * must hold the notifier lock and keep holding it until its use of the +pages + * is visible to the invalidation path. + * + * Return: True if the pages must be faulted again, false otherwise. + */ +static inline bool +drm_gpusvm_pages_read_retry(struct drm_gpusvm *gpusvm, + struct drm_gpusvm_pages *svm_pages, + struct mmu_interval_notifier *notifier) { + lockdep_assert_held(&gpusvm->notifier_lock); + + return mmu_interval_read_retry(notifier, svm_pages->notifier_seq); } + /** * drm_gpusvm_pages_first_dma() - Resolve the device address array * @svm_pages: Pointer to the drm_gpusvm_pages. base-commit: d2a8cee91bfa8616f713c7c8f539a81e8e3554ea -- 2.34.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V1 1/2] drm/gpusvm: add drm_gpusvm_pages unmapped and retry helpers 2026-11-09 8:56 ` [PATCH V1 1/2] drm/gpusvm: add drm_gpusvm_pages unmapped and retry helpers Huang, Honglei1 @ 2026-11-09 19:53 ` Lizhi Hou 0 siblings, 0 replies; 5+ messages in thread From: Lizhi Hou @ 2026-11-09 19:53 UTC (permalink / raw) To: Huang, Honglei1, Matthew Brost Cc: linux-kernel, Santan, Sonal, ogabbay, quic_jhugo, dri-devel, Limonciello, Mario, karol.wachowski, Zhen, Max, thomas.hellstrom, himal.prasad.ghimiray Hi Matthew, How do we converge on this? Both 2 patches are reviewed, is it good to merge them to drm-misc-next? Honglei, Thanks a lot for reviewing the code. Lizhi On 11/9/26 00:56, Huang, Honglei1 wrote: > AMD General > > Reviewed-by: Honglei Huang <honghuan@amd.com> > > -----Original Message----- > From: Lizhi Hou <lizhi.hou@amd.com> > Sent: Saturday, November 7, 2026 2:42 AM > To: ogabbay@kernel.org; quic_jhugo@quicinc.com; dri-devel@lists.freedesktop.org; Limonciello, Mario <Mario.Limonciello@amd.com>; karol.wachowski@linux.intel.com; Zhen, Max <Max.Zhen@amd.com>; thomas.hellstrom@linux.intel.com; himal.prasad.ghimiray@intel.com; Huang, Honglei1 <Honglei1.Huang@amd.com> > Cc: Matthew Brost <matthew.brost@intel.com>; linux-kernel@vger.kernel.org; Santan, Sonal <sonal.santan@amd.com>; Hou, Lizhi <lizhi.hou@amd.com> > Subject: [PATCH V1 1/2] drm/gpusvm: add drm_gpusvm_pages unmapped and retry helpers > > From: Matthew Brost <matthew.brost@intel.com> > > Drivers which only use the drm_gpusvm_pages API (no drm_gpusvm_range) have no way to mark pages unmapped from their MMU notifier, as > drm_gpusvm_range_set_unmapped() requires a range. They also have to open code the mmu_interval_read_retry() check on the recorded notifier_seq to commit pages faulted with &drm_gpusvm_ctx.no_dma_map. > > Split the pages part of drm_gpusvm_range_set_unmapped() into drm_gpusvm_pages_set_unmapped(), and add drm_gpusvm_pages_read_retry() which performs the commit side check with a lockdep assert on the notifier lock. > > Assisted-by: LLM > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > Signed-off-by: Lizhi Hou <lizhi.hou@amd.com> > --- > drivers/gpu/drm/drm_gpusvm.c | 49 +++++++++++++++++++++++++----------- > include/drm/drm_gpusvm.h | 29 +++++++++++++++++++++ > 2 files changed, 64 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c index 03dbe304e4b8f..63a26919f59db 100644 > --- a/drivers/gpu/drm/drm_gpusvm.c > +++ b/drivers/gpu/drm/drm_gpusvm.c > @@ -1761,9 +1761,8 @@ static int drm_gpusvm_dma_map_pages(struct drm_gpusvm *gpusvm, > * > * With &drm_gpusvm_ctx.no_dma_map no mapping state is recorded, so > * drm_gpusvm_pages_valid() never returns true and success is only a snapshot: > - * the caller must recheck mmu_interval_read_retry() against the recorded > - * &drm_gpusvm_pages.notifier_seq under the notifier lock, and hold it until > - * its work is visible to invalidation. > + * the caller must recheck drm_gpusvm_pages_read_retry() under the > + notifier > + * lock, and hold it until its work is visible to invalidation. > * > * Return: 0 on success, negative error code on failure. > */ > @@ -2021,6 +2020,38 @@ bool drm_gpusvm_has_mapping(struct drm_gpusvm *gpusvm, unsigned long start, } EXPORT_SYMBOL_GPL(drm_gpusvm_has_mapping); > > +/** > + * drm_gpusvm_pages_set_unmapped() - Mark GPU SVM pages as unmapped > + * @gpusvm: Pointer to the GPU SVM structure. > + * @pages: Pointer to the GPU SVM pages structure(s). > + * @pages_count: Number of GPU SVM pages structure(s) passed in. > + * > + * Mark @pages as unmapped, after which drm_gpusvm_get_pages() fails > +with > + * -EFAULT for them. Intended to be called from the driver's MMU > +notifier on > + * MMU_NOTIFY_UNMAP, and usable without a &drm_gpusvm_range by drivers > +that > + * only use the drm_gpusvm_pages API. The caller must hold the notifier > +lock > + * in write mode. > + */ > +void drm_gpusvm_pages_set_unmapped(struct drm_gpusvm *gpusvm, > + struct drm_gpusvm_pages *pages, > + unsigned int pages_count) > +{ > + unsigned int i; > + > + lockdep_assert_held_write(&gpusvm->notifier_lock); > + > + for (i = 0; i < pages_count; ++i) { > + struct drm_gpusvm_pages_flags flags = { > + .__flags = pages[i].flags.__flags, > + }; > + > + flags.unmapped = true; > + /* WRITE_ONCE pairs with READ_ONCE for opportunistic checks */ > + WRITE_ONCE(pages[i].flags.__flags, flags.__flags); > + } > +} > +EXPORT_SYMBOL_GPL(drm_gpusvm_pages_set_unmapped); > + > /** > * drm_gpusvm_range_set_unmapped() - Mark a GPU SVM range as unmapped > * @range: Pointer to the GPU SVM range structure. > @@ -2039,20 +2070,10 @@ void drm_gpusvm_range_set_unmapped(struct drm_gpusvm_range *range, > struct drm_gpusvm_range_flags range_flags = { > .__flags = range->flags.__flags, > }; > - unsigned int i; > > - lockdep_assert_held_write(&range->gpusvm->notifier_lock); > + drm_gpusvm_pages_set_unmapped(range->gpusvm, pages, pages_count); > > range_flags.unmapped = true; > - for (i = 0; i < pages_count; ++i) { > - struct drm_gpusvm_pages_flags flags = { > - .__flags = pages[i].flags.__flags, > - }; > - > - flags.unmapped = true; > - /* WRITE_ONCE pairs with READ_ONCE for opportunistic checks */ > - WRITE_ONCE(pages[i].flags.__flags, flags.__flags); > - } > if (drm_gpusvm_range_start(range) < mmu_range->start || > drm_gpusvm_range_end(range) > mmu_range->end) > range_flags.partial_unmap = true; > diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h index 4d31c6bba7455..b43432af56377 100644 > --- a/include/drm/drm_gpusvm.h > +++ b/include/drm/drm_gpusvm.h > @@ -349,6 +349,10 @@ void drm_gpusvm_range_set_unmapped(struct drm_gpusvm_range *range, > unsigned int pages_count, > const struct mmu_notifier_range *mmu_range); > > +void drm_gpusvm_pages_set_unmapped(struct drm_gpusvm *gpusvm, > + struct drm_gpusvm_pages *pages, > + unsigned int pages_count); > + > int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm, > struct drm_gpusvm_pages *svm_pages, > unsigned int num_pages, > @@ -383,6 +387,31 @@ static inline void drm_gpusvm_init_pages(struct drm_gpusvm_pages *svm_pages, > svm_pages->notifier_seq = LONG_MAX; > } > > +/** > + * drm_gpusvm_pages_read_retry() - Check whether GPU SVM pages went > +stale > + * @gpusvm: Pointer to the GPU SVM structure. > + * @svm_pages: Pointer to the drm_gpusvm_pages. > + * @notifier: The notifier passed to drm_gpusvm_get_pages() for @svm_pages. > + * > + * Check whether @notifier was invalidated since the last successful > + * drm_gpusvm_get_pages() for @svm_pages. This is the commit side check > +for > + * pages faulted with &drm_gpusvm_ctx.no_dma_map, where no mapping > +state is > + * tracked. Pages that were never faulted always report as stale. The > +caller > + * must hold the notifier lock and keep holding it until its use of the > +pages > + * is visible to the invalidation path. > + * > + * Return: True if the pages must be faulted again, false otherwise. > + */ > +static inline bool > +drm_gpusvm_pages_read_retry(struct drm_gpusvm *gpusvm, > + struct drm_gpusvm_pages *svm_pages, > + struct mmu_interval_notifier *notifier) { > + lockdep_assert_held(&gpusvm->notifier_lock); > + > + return mmu_interval_read_retry(notifier, svm_pages->notifier_seq); } > + > /** > * drm_gpusvm_pages_first_dma() - Resolve the device address array > * @svm_pages: Pointer to the drm_gpusvm_pages. > > base-commit: d2a8cee91bfa8616f713c7c8f539a81e8e3554ea > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-11-09 19:53 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-11-06 18:42 [PATCH V1 1/2] drm/gpusvm: add drm_gpusvm_pages unmapped and retry helpers Lizhi Hou 2026-11-06 18:42 ` [PATCH V1 2/2] accel/amdxdna: use drm_gpusvm_pages for user mapping tracking Lizhi Hou 2026-11-09 19:44 ` Max Zhen 2026-11-09 8:56 ` [PATCH V1 1/2] drm/gpusvm: add drm_gpusvm_pages unmapped and retry helpers Huang, Honglei1 2026-11-09 19:53 ` Lizhi Hou
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®