Re: [Patch v2 1/2] drm/amdgpu: use user provided hmm_range buffer in amdgpu_ttm_tt_get_user_pages

["Khatri, Sunil" <sukhatri@amd.com>]

On 10/4/2025 1:53 AM, Chen, Xiaogang wrote:
>
> On 9/26/2025 5:53 AM, Khatri, Sunil wrote:
>>
>> On 9/24/2025 10:27 PM, Kuehling, Felix wrote:
>>> On 2025-09-24 06:01, Sunil Khatri wrote:
>>>> update the amdgpu_ttm_tt_get_user_pages and all dependent function
>>>> along with it callers to use a user allocated hmm_range buffer instead
>>>> hmm layer allocates the buffer.
>>>>
>>>> This is a need to get hmm_range pointers easily accessible
>>>> without accessing the bo and that is a requirement for the
>>>> userqueue to lock the userptrs effectively.
>>>>
>>>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>>>
>>> What's the reason for this change? In the current code, the 
>>> hmm_range is allocated by amdgpu_hmm_range_get_pages and freed by 
>>> amdgpu_hmm_range_get_pages_done. Your change is breaking that symmetry.
>> Sorry i missed your comment. For userqueues locking the userptr bos 
>> and making sure we have valid userptrs at the time of validation 
>> seems too complicated, so along with christian we decided to use 
>> hmm_range list instead and have reference to userptr bo and via 
>> hmm_range private field to be set to bo.
>>
>> Also i did made sure that wherever we are doing get pages and 
>> allocating range the freeing part is taken care of. Specially for 
>> freeing the memory is still done by amdgpu_hmm_range_get_pages_done 
>> only. Please share if anywhere i missed something. Also Christian 
>> brought out the point to have separate alloc/free for hmm_range which 
>> i am working on and will share soon.
>
> This patch has other components to allocate hmm_range, freed it at 
> amdgpu_hmm_range_get_pages_done. This inconsistency makes other 
> components handle error case awkward.  It is better to let other 
> component free hmm_range no matter amdgpu_hmm_range_get_pages success 
> or not. And amdgpu_hmm_range_get_pages(done) alloc/free 
> hmm_range->hmm_pfns. That would be easy to understand and have less 
> chance to make mistake.
Yes, that inconsistency is there and that is to be fixed is taken care 
by having separate alloc/free functions and not use 
amdgpu_hmm_range_get_pages_done and some more clean up done in other 
patch since this code is already merged.

Regards
Sunil khatri

>
> Regards
>
> Xiaogang
>
>>
>> Regards
>> Sunil Khatri
>>
>>> Regards,
>>>   Felix
>>>
>>>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 16 
>>>> ++++++++++++++--
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c           |  6 +++++-
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c          | 10 +++++++---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c          | 11 +----------
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.h          |  2 +-
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c          |  8 +++-----
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h          |  4 ++--
>>>>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c             |  7 +++++--
>>>>   8 files changed, 38 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>> index 7c54fe6b0f5d..4babd37712fb 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>> @@ -1089,8 +1089,15 @@ static int init_user_pages(struct kgd_mem 
>>>> *mem, uint64_t user_addr,
>>>>           return 0;
>>>>       }
>>>>   -    ret = amdgpu_ttm_tt_get_user_pages(bo, &range);
>>>> +    range = kzalloc(sizeof(*range), GFP_KERNEL);
>>>> +    if (unlikely(!range)) {
>>>> +        ret = -ENOMEM;
>>>> +        goto unregister_out;
>>>> +    }
>>>> +
>>>> +    ret = amdgpu_ttm_tt_get_user_pages(bo, range);
>>>>       if (ret) {
>>>> +        kfree(range);
>>>>           if (ret == -EAGAIN)
>>>>               pr_debug("Failed to get user pages, try again\n");
>>>>           else
>>>> @@ -2567,9 +2574,14 @@ static int update_invalid_user_pages(struct 
>>>> amdkfd_process_info *process_info,
>>>>               }
>>>>           }
>>>>   +        mem->range = kzalloc(sizeof(*mem->range), GFP_KERNEL);
>>>> +        if (unlikely(!mem->range))
>>>> +            return -ENOMEM;
>>>>           /* Get updated user pages */
>>>> -        ret = amdgpu_ttm_tt_get_user_pages(bo, &mem->range);
>>>> +        ret = amdgpu_ttm_tt_get_user_pages(bo, mem->range);
>>>>           if (ret) {
>>>> +            kfree(mem->range);
>>>> +            mem->range = NULL;
>>>>               pr_debug("Failed %d to get user pages\n", ret);
>>>>                 /* Return -EFAULT bad address error as success. It 
>>>> will
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> index 744e6ff69814..31eea1c7dac3 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> @@ -884,9 +884,13 @@ static int amdgpu_cs_parser_bos(struct 
>>>> amdgpu_cs_parser *p,
>>>>       amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
>>>>           bool userpage_invalidated = false;
>>>>           struct amdgpu_bo *bo = e->bo;
>>>> +        e->range = kzalloc(sizeof(*e->range), GFP_KERNEL);
>>>> +        if (unlikely(!e->range))
>>>> +            return -ENOMEM;
>>>> +
>>>>           int i;
>>>>   -        r = amdgpu_ttm_tt_get_user_pages(bo, &e->range);
>>>> +        r = amdgpu_ttm_tt_get_user_pages(bo, e->range);
>>>>           if (r)
>>>>               goto out_free_user_pages;
>>>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> index 8524aa55e057..12f0597a3659 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> @@ -571,10 +571,14 @@ int amdgpu_gem_userptr_ioctl(struct 
>>>> drm_device *dev, void *data,
>>>>           goto release_object;
>>>>         if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE) {
>>>> -        r = amdgpu_ttm_tt_get_user_pages(bo, &range);
>>>> -        if (r)
>>>> +        range = kzalloc(sizeof(*range), GFP_KERNEL);
>>>> +        if (unlikely(!range))
>>>> +            return -ENOMEM;
>>>> +        r = amdgpu_ttm_tt_get_user_pages(bo, range);
>>>> +        if (r) {
>>>> +            kfree(range);
>>>>               goto release_object;
>>>> -
>>>> +        }
>>>>           r = amdgpu_bo_reserve(bo, true);
>>>>           if (r)
>>>>               goto user_pages_done;
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>>>> index 2c6a6b858112..53d405a92a14 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>>>> @@ -168,18 +168,13 @@ void amdgpu_hmm_unregister(struct amdgpu_bo *bo)
>>>>   int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier 
>>>> *notifier,
>>>>                      uint64_t start, uint64_t npages, bool readonly,
>>>>                      void *owner,
>>>> -                   struct hmm_range **phmm_range)
>>>> +                   struct hmm_range *hmm_range)
>>>>   {
>>>> -    struct hmm_range *hmm_range;
>>>>       unsigned long end;
>>>>       unsigned long timeout;
>>>>       unsigned long *pfns;
>>>>       int r = 0;
>>>>   -    hmm_range = kzalloc(sizeof(*hmm_range), GFP_KERNEL);
>>>> -    if (unlikely(!hmm_range))
>>>> -        return -ENOMEM;
>>>> -
>>>>       pfns = kvmalloc_array(npages, sizeof(*pfns), GFP_KERNEL);
>>>>       if (unlikely(!pfns)) {
>>>>           r = -ENOMEM;
>>>> @@ -221,15 +216,11 @@ int amdgpu_hmm_range_get_pages(struct 
>>>> mmu_interval_notifier *notifier,
>>>>       hmm_range->start = start;
>>>>       hmm_range->hmm_pfns = pfns;
>>>>   -    *phmm_range = hmm_range;
>>>> -
>>>>       return 0;
>>>>     out_free_pfns:
>>>>       kvfree(pfns);
>>>>   out_free_range:
>>>> -    kfree(hmm_range);
>>>> -
>>>>       if (r == -EBUSY)
>>>>           r = -EAGAIN;
>>>>       return r;
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.h 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.h
>>>> index 953e1d06de20..c54e3c64251a 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.h
>>>> @@ -34,7 +34,7 @@
>>>>   int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier 
>>>> *notifier,
>>>>                      uint64_t start, uint64_t npages, bool readonly,
>>>>                      void *owner,
>>>> -                   struct hmm_range **phmm_range);
>>>> +                   struct hmm_range *hmm_range);
>>>>   bool amdgpu_hmm_range_get_pages_done(struct hmm_range *hmm_range);
>>>>     #if defined(CONFIG_HMM_MIRROR)
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> index 901e0c39a594..046ff2346dab 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> @@ -705,10 +705,11 @@ struct amdgpu_ttm_tt {
>>>>    * memory and start HMM tracking CPU page table update
>>>>    *
>>>>    * Calling function must call amdgpu_ttm_tt_userptr_range_done() 
>>>> once and only
>>>> - * once afterwards to stop HMM tracking
>>>> + * once afterwards to stop HMM tracking. Its the caller 
>>>> responsibility to ensure
>>>> + * that range is a valid memory and it is freed too.
>>>>    */
>>>>   int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo *bo,
>>>> -                 struct hmm_range **range)
>>>> +                 struct hmm_range *range)
>>>>   {
>>>>       struct ttm_tt *ttm = bo->tbo.ttm;
>>>>       struct amdgpu_ttm_tt *gtt = ttm_to_amdgpu_ttm_tt(ttm);
>>>> @@ -718,9 +719,6 @@ int amdgpu_ttm_tt_get_user_pages(struct 
>>>> amdgpu_bo *bo,
>>>>       bool readonly;
>>>>       int r = 0;
>>>>   -    /* Make sure get_user_pages_done() can cleanup gracefully */
>>>> -    *range = NULL;
>>>> -
>>>>       mm = bo->notifier.mm;
>>>>       if (unlikely(!mm)) {
>>>>           DRM_DEBUG_DRIVER("BO is not registered?\n");
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>>> index 6ac94469ed40..a8379b925878 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>>> @@ -191,14 +191,14 @@ uint64_t amdgpu_ttm_domain_start(struct 
>>>> amdgpu_device *adev, uint32_t type);
>>>>     #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>>>>   int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo *bo,
>>>> -                 struct hmm_range **range);
>>>> +                 struct hmm_range *range);
>>>>   void amdgpu_ttm_tt_discard_user_pages(struct ttm_tt *ttm,
>>>>                         struct hmm_range *range);
>>>>   bool amdgpu_ttm_tt_get_user_pages_done(struct ttm_tt *ttm,
>>>>                          struct hmm_range *range);
>>>>   #else
>>>>   static inline int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo *bo,
>>>> -                           struct hmm_range **range)
>>>> +                           struct hmm_range *range)
>>>>   {
>>>>       return -EPERM;
>>>>   }
>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c 
>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>>> index 273f42e3afdd..9f0f14ea93e5 100644
>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>>> @@ -1737,12 +1737,15 @@ static int 
>>>> svm_range_validate_and_map(struct mm_struct *mm,
>>>>               }
>>>>                 WRITE_ONCE(p->svms.faulting_task, current);
>>>> +            hmm_range = kzalloc(sizeof(*hmm_range), GFP_KERNEL);
>>>>               r = amdgpu_hmm_range_get_pages(&prange->notifier, 
>>>> addr, npages,
>>>>                                  readonly, owner,
>>>> -                               &hmm_range);
>>>> +                               hmm_range);
>>>>               WRITE_ONCE(p->svms.faulting_task, NULL);
>>>> -            if (r)
>>>> +            if (r) {
>>>> +                kfree(hmm_range);
>>>>                   pr_debug("failed %d to get svm range pages\n", r);
>>>> +            }
>>>>           } else {
>>>>               r = -EFAULT;
>>>>           }