mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Lizhi Hou <lizhi.hou@amd.com>
To: Max Zhen <max.zhen@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: Tue, 10 Nov 2026 10:16:39 -0800	[thread overview]
Message-ID: <a12185d5-fe6c-4714-78b9-5c45a45c7c30@amd.com> (raw)
In-Reply-To: <b76bb18c-a0e5-4a49-8866-888df3bd9a30@amd.com>

Applied drm-misc-next

On 11/9/26 11:44, Max Zhen wrote:
>
>
> 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;
>

  reply	other threads:[~2026-11-10 18:16 UTC|newest]

Thread overview: 9+ 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-09 19:44   ` Max Zhen
2026-11-10 18:16     ` Lizhi Hou [this message]
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=a12185d5-fe6c-4714-78b9-5c45a45c7c30@amd.com \
    --to=lizhi.hou@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=mario.limonciello@amd.com \
    --cc=matthew.brost@intel.com \
    --cc=max.zhen@amd.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 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®