Re: [PATCH V4 14/19] accel/amdxdna: Move HMM invalidate wait into common GEM code

[Lizhi Hou <lizhi.hou@amd.com>]

On 10/13/26 11:45, Lizhi Hou wrote:
>
> On 10/9/26 15:53, David Zhang wrote:
>> When a user mapping of a BO is invalidated, the driver must wait for
>> the BO's reservation fences before the pages are released. AIE2 does
>> this in aie2_hmm_invalidate() through the .hmm_invalidate callback,
>> and AIE4 needs the same wait.
>>
>> Do the wait directly in amdxdna_hmm_invalidate() in amdxdna_gem.c and
>> remove the .hmm_invalidate callback and aie2_hmm_invalidate(). Devices
>> that do not attach fences to user BOs return from the wait at once.
>>
>> Make the wait uninterruptible. With an interruptible wait, a pending
>> signal, such as SIGKILL during process exit, ends the wait early and
>> the pages can be freed while the NPU may still access them.
>
> This is a bug fix. Please add "Fixes" tag and change the title to 
> reflect the fix.

It is actually better to separate the bug fix and the movement to common 
code.

I would take the fix:

https://gitlab.freedesktop.org/mbrost/xe-kernel-driver-svn-perf-6-15-2025/-/commit/dfa1a332824421f1713388a86331a4c4d84f680f

Then moving to common code on top.

Thanks,

Lizhi

>
> Lizhi
>
>>
>> Co-developed-by: Hayden Laccabue <Hayden.Laccabue@amd.com>
>> Signed-off-by: Hayden Laccabue <Hayden.Laccabue@amd.com>
>> Signed-off-by: David Zhang <yidong.zhang@amd.com>
>> ---
>>   drivers/accel/amdxdna/aie2_ctx.c        | 15 ---------------
>>   drivers/accel/amdxdna/aie2_pci.c        |  1 -
>>   drivers/accel/amdxdna/aie2_pci.h        |  1 -
>>   drivers/accel/amdxdna/amdxdna_gem.c     | 14 ++++++++++++--
>>   drivers/accel/amdxdna/amdxdna_pci_drv.h |  1 -
>>   5 files changed, 12 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/accel/amdxdna/aie2_ctx.c 
>> b/drivers/accel/amdxdna/aie2_ctx.c
>> index d927c8c9d557..8c1b29964f8f 100644
>> --- a/drivers/accel/amdxdna/aie2_ctx.c
>> +++ b/drivers/accel/amdxdna/aie2_ctx.c
>> @@ -1277,21 +1277,6 @@ int aie2_cmd_submit(struct amdxdna_hwctx 
>> *hwctx, struct amdxdna_sched_job *job,
>>       return ret;
>>   }
>>   -void aie2_hmm_invalidate(struct amdxdna_gem_obj *abo,
>> -             unsigned long cur_seq)
>> -{
>> -    struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
>> -    struct drm_gem_object *gobj = to_gobj(abo);
>> -    long ret;
>> -
>> -    ret = dma_resv_wait_timeout(gobj->resv, DMA_RESV_USAGE_BOOKKEEP,
>> -                    true, MAX_SCHEDULE_TIMEOUT);
>> -    if (!ret)
>> -        XDNA_ERR(xdna, "Failed to wait for bo, ret %ld", ret);
>> -    else if (ret == -ERESTARTSYS)
>> -        XDNA_DBG(xdna, "Wait for bo interrupted by signal");
>> -}
>> -
>>   int aie2_hwctx_heap_expand(struct amdxdna_hwctx *hwctx,
>>                  struct amdxdna_gem_obj *heap)
>>   {
>> diff --git a/drivers/accel/amdxdna/aie2_pci.c 
>> b/drivers/accel/amdxdna/aie2_pci.c
>> index 1b029d33a24b..690ad035463c 100644
>> --- a/drivers/accel/amdxdna/aie2_pci.c
>> +++ b/drivers/accel/amdxdna/aie2_pci.c
>> @@ -1228,7 +1228,6 @@ const struct amdxdna_dev_ops aie2_ops = {
>>       .hwctx_config = aie2_hwctx_config,
>>       .hwctx_sync_debug_bo = aie2_hwctx_sync_debug_bo,
>>       .cmd_submit = aie2_cmd_submit,
>> -    .hmm_invalidate = aie2_hmm_invalidate,
>>       .get_array = aie2_get_array,
>>       .get_dev_revision = aie2_get_dev_rev,
>>       .hwctx_heap_expand = aie2_hwctx_heap_expand,
>> diff --git a/drivers/accel/amdxdna/aie2_pci.h 
>> b/drivers/accel/amdxdna/aie2_pci.h
>> index 709e7545c65f..13391275df31 100644
>> --- a/drivers/accel/amdxdna/aie2_pci.h
>> +++ b/drivers/accel/amdxdna/aie2_pci.h
>> @@ -272,7 +272,6 @@ int aie2_hwctx_sync_debug_bo(struct amdxdna_hwctx 
>> *hwctx, u32 debug_bo_hdl);
>>   void aie2_hwctx_suspend(struct amdxdna_client *client);
>>   int aie2_hwctx_resume(struct amdxdna_client *client);
>>   int aie2_cmd_submit(struct amdxdna_hwctx *hwctx, struct 
>> amdxdna_sched_job *job, u64 *seq);
>> -void aie2_hmm_invalidate(struct amdxdna_gem_obj *abo, unsigned long 
>> cur_seq);
>>   int aie2_hwctx_heap_expand(struct amdxdna_hwctx *hwctx, struct 
>> amdxdna_gem_obj *heap);
>>     #endif /* _AIE2_PCI_H_ */
>> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c 
>> b/drivers/accel/amdxdna/amdxdna_gem.c
>> index f4832337ec31..e1b72cef51bd 100644
>> --- a/drivers/accel/amdxdna/amdxdna_gem.c
>> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
>> @@ -12,6 +12,7 @@
>>   #include <drm/gpu_scheduler.h>
>>   #include <linux/dma-buf.h>
>>   #include <linux/dma-direct.h>
>> +#include <linux/dma-resv.h>
>>   #include <linux/iosys-map.h>
>>   #include <linux/pagemap.h>
>>   #include <linux/swap.h>
>> @@ -232,6 +233,7 @@ 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;
>> +    long ret;
>>         if (!mmu_notifier_range_blockable(range))
>>           return false;
>> @@ -249,8 +251,16 @@ static bool amdxdna_hmm_invalidate(struct 
>> mmu_interval_notifier *mni,
>>       mmu_interval_set_seq(&mapp->notifier, cur_seq);
>>       up_write(&xdna->notifier_lock);
>>   -    if (xdna->dev_info->ops->hmm_invalidate)
>> -        xdna->dev_info->ops->hmm_invalidate(abo, cur_seq);
>> +    /*
>> +     * Wait uninterruptibly. Returning early on a signal would let the
>> +     * pages be freed while the NPU may still access them. This relies
>> +     * on job fences signaling in finite time; job timeout detection
>> +     * and recovery (TDR) for AIE4 will be added in a separate change.
>> +     */
>> +    ret = dma_resv_wait_timeout(to_gobj(abo)->resv, 
>> DMA_RESV_USAGE_BOOKKEEP,
>> +                    false, MAX_SCHEDULE_TIMEOUT);
>> +    if (!ret)
>> +        XDNA_ERR(xdna, "Failed to wait for bo, ret %ld", ret);
>>         if (range->event == MMU_NOTIFY_UNMAP) {
>>           down_write(&xdna->notifier_lock);
>> diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.h 
>> b/drivers/accel/amdxdna/amdxdna_pci_drv.h
>> index 11f46ec738d7..2a1d6b33363c 100644
>> --- a/drivers/accel/amdxdna/amdxdna_pci_drv.h
>> +++ b/drivers/accel/amdxdna/amdxdna_pci_drv.h
>> @@ -63,7 +63,6 @@ struct amdxdna_dev_ops {
>>       int (*hwctx_config)(struct amdxdna_hwctx *hwctx, u32 type, u64 
>> value, void *buf, u32 size);
>>       int (*hwctx_sync_debug_bo)(struct amdxdna_hwctx *hwctx, u32 
>> debug_bo_hdl);
>>       int (*hwctx_heap_expand)(struct amdxdna_hwctx *hwctx, struct 
>> amdxdna_gem_obj *heap);
>> -    void (*hmm_invalidate)(struct amdxdna_gem_obj *abo, unsigned 
>> long cur_seq);
>>       int (*cmd_submit)(struct amdxdna_hwctx *hwctx, struct 
>> amdxdna_sched_job *job, u64 *seq);
>>       int (*cmd_wait)(struct amdxdna_hwctx *hwctx, u64 seq, u32 
>> timeout);
>>       int (*get_aie_info)(struct amdxdna_client *client, struct 
>> amdxdna_drm_get_info *args);