From: Max Zhen <max.zhen@amd.com>
To: Lizhi Hou <lizhi.hou@amd.com>, <ogabbay@kernel.org>,
<quic_jhugo@quicinc.com>, <dri-devel@lists.freedesktop.org>,
<mario.limonciello@amd.com>, <karol.wachowski@linux.intel.com>,
<thomas.hellstrom@linux.intel.com>,
<himal.prasad.ghimiray@intel.com>, <honghuan@amd.com>
Cc: Matthew Brost <matthew.brost@intel.com>,
<linux-kernel@vger.kernel.org>, <sonal.santan@amd.com>
Subject: Re: [PATCH V1 2/2] accel/amdxdna: use drm_gpusvm_pages for user mapping tracking
Date: Mon, 9 Nov 2026 11:44:50 -0800 [thread overview]
Message-ID: <b76bb18c-a0e5-4a49-8866-888df3bd9a30@amd.com> (raw)
In-Reply-To: <20261106184217.2720718-2-lizhi.hou@amd.com>
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;
next prev parent reply other threads:[~2026-11-09 19:45 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
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-06 19:04 ` sashiko-bot
2026-11-09 19:44 ` Max Zhen [this message]
2026-11-10 18:16 ` Lizhi Hou
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
2026-11-09 22:35 ` Matthew Brost
2026-11-10 18:16 ` Lizhi Hou
2026-11-09 22:34 ` Matthew Brost
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b76bb18c-a0e5-4a49-8866-888df3bd9a30@amd.com \
--to=max.zhen@amd.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=himal.prasad.ghimiray@intel.com \
--cc=honghuan@amd.com \
--cc=karol.wachowski@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=lizhi.hou@amd.com \
--cc=mario.limonciello@amd.com \
--cc=matthew.brost@intel.com \
--cc=ogabbay@kernel.org \
--cc=quic_jhugo@quicinc.com \
--cc=sonal.santan@amd.com \
--cc=thomas.hellstrom@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.