On 01.10.25 07:41, Kuehling, Felix wrote:
> On 2025-09-26 06:53, 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 <[email protected]>
>>>
>>> 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.
>
> I don't think this will work. See my reply to your other code review.
> hmm_range->dev_private_owner is not a device-private field that you can use
> to store random driver-data associated with the HMM range.
Ah! Thanks for that very valuable information. I thought it was just a void*
drivers could use for their purpose.
Mhm, that makes the implementation even more complicated that it already is.
Regards,
Christian.
>
> Regards,
> Felix
>
>
>>
>> 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.
>>
>> 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;
>>>> }
