Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers v4
On 2019-06-04 16:13, Yang, Philip wrote:
> HMM provides new APIs and helps in kernel 5.2-rc1 to simplify driver
> path. The old hmm APIs are deprecated and will be removed in future.
>
> Below are changes in driver:
>
> 1. Change hmm_vma_fault to hmm_range_register and hmm_range_fault which
> supports range with multiple vmas, remove the multiple vmas handle path
> and data structure.
> 2. Change hmm_vma_range_done to hmm_range_unregister.
> 3. Use default flags to avoid pre-fill pfn arrays.
> 4. Use new hmm_device_ helpers.
>
> v2: retry if hmm_range_fault returns -EAGAIN
> v3: define MAX_RETRY_HMM_RANGE_FAULT, skip drop mmap_sem if retry
> v4: fine grained up_read(mm->mmap_sem)
>
> Change-Id: I1a00f88459f3b96c9536933e9a17eb1ebaa78113
> Signed-off-by: Philip Yang
Reviewed-by: Felix Kuehling
Thanks,
Felix
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c | 1 -
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 152 +++-
> 2 files changed, 68 insertions(+), 85 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> index 41ccee49a224..e0bb47ce570b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> @@ -519,7 +519,6 @@ void amdgpu_hmm_init_range(struct hmm_range *range)
> range->flags = hmm_range_flags;
> range->values = hmm_range_values;
> range->pfn_shift = PAGE_SHIFT;
> - range->pfns = NULL;
> INIT_LIST_HEAD(>list);
> }
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index eb4b85061c7e..00790207086a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -711,8 +711,7 @@ struct amdgpu_ttm_tt {
> struct task_struct *usertask;
> uint32_tuserflags;
> #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
> - struct hmm_range*ranges;
> - int nr_ranges;
> + struct hmm_range*range;
> #endif
> };
>
> @@ -725,57 +724,36 @@ struct amdgpu_ttm_tt {
>*/
> #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>
> -/* Support Userptr pages cross max 16 vmas */
> -#define MAX_NR_VMAS (16)
> +#define MAX_RETRY_HMM_RANGE_FAULT16
>
> int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages)
> {
> struct amdgpu_ttm_tt *gtt = (void *)ttm;
> struct mm_struct *mm = gtt->usertask->mm;
> unsigned long start = gtt->userptr;
> - unsigned long end = start + ttm->num_pages * PAGE_SIZE;
> - struct vm_area_struct *vma = NULL, *vmas[MAX_NR_VMAS];
> - struct hmm_range *ranges;
> - unsigned long nr_pages, i;
> - uint64_t *pfns, f;
> + struct vm_area_struct *vma;
> + struct hmm_range *range;
> + unsigned long i;
> + uint64_t *pfns;
> + int retry = 0;
> int r = 0;
>
> if (!mm) /* Happens during process shutdown */
> return -ESRCH;
>
> - down_read(>mmap_sem);
> -
> - /* user pages may cross multiple VMAs */
> - gtt->nr_ranges = 0;
> - do {
> - unsigned long vm_start;
> -
> - if (gtt->nr_ranges >= MAX_NR_VMAS) {
> - DRM_ERROR("Too many VMAs in userptr range\n");
> - r = -EFAULT;
> - goto out;
> - }
> -
> - vm_start = vma ? vma->vm_end : start;
> - vma = find_vma(mm, vm_start);
> - if (unlikely(!vma || vm_start < vma->vm_start)) {
> - r = -EFAULT;
> - goto out;
> - }
> - vmas[gtt->nr_ranges++] = vma;
> - } while (end > vma->vm_end);
> -
> - DRM_DEBUG_DRIVER("0x%lx nr_ranges %d pages 0x%lx\n",
> - start, gtt->nr_ranges, ttm->num_pages);
> -
> + vma = find_vma(mm, start);
> + if (unlikely(!vma || start < vma->vm_start)) {
> + r = -EFAULT;
> + goto out;
> + }
> if (unlikely((gtt->userflags & AMDGPU_GEM_USERPTR_ANONONLY) &&
> - vmas[0]->vm_file)) {
> + vma->vm_file)) {
> r = -EPERM;
> goto out;
> }
>
> - ranges = kvmalloc_array(gtt->nr_ranges, sizeof(*ranges), GFP_KERNEL);
> - if (unlikely(!ranges)) {
> + range = kzalloc(sizeof(*range), GFP_KERNEL);
> + if (unlikely(!range)) {
> r = -ENOMEM;
> goto out;
> }
> @@ -786,61 +764,67 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm,
> struct page **pages)
> goto out_free_ranges;
> }
>
> - for (i = 0; i < gtt->nr_ranges; i++)
> - amdgpu_hmm_init_range([i]);
> + amdgpu_hmm_init_range(range);
> + range->default_flags = range->flags[HMM_PFN_VALID];
> + range->default_flags |= amdgpu_ttm_tt_is_readonly(ttm) ?
> + 0 :
Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers v3
[+Jerome]
On 2019-06-03 7:20 p.m., Yang, Philip wrote:
>
> On 2019-06-03 5:02 p.m., Kuehling, Felix wrote:
>> On 2019-06-03 2:44 p.m., Yang, Philip wrote:
>>> HMM provides new APIs and helps in kernel 5.2-rc1 to simplify driver
>>> path. The old hmm APIs are deprecated and will be removed in future.
>>>
>>> Below are changes in driver:
>>>
>>> 1. Change hmm_vma_fault to hmm_range_register and hmm_range_fault which
>>> supports range with multiple vmas, remove the multiple vmas handle path
>>> and data structure.
>>> 2. Change hmm_vma_range_done to hmm_range_unregister.
>>> 3. Use default flags to avoid pre-fill pfn arrays.
>>> 4. Use new hmm_device_ helpers.
>>>
>>> v2: retry if hmm_range_fault returns -EAGAIN
>>> v3: define MAX_RETRY_HMM_RANGE_FAULT, skip drop mmap_sem if retry
>> I think dropping mmap_sem for retry was correct in v2 of this patch. Now
>> you will try to take the mmap_sem multiple times without dropping it if
>> you retry.
>>
>> We don't need to hold the mmap_sem during hmm_range_wait_until_valid.
>>
> hmm_range_fault will drop the mmap_sem internally before returning
> -EAGAIN, I think it does this on purpose to avoid upper level call retry
> whithout release mmap_sem in between.
Good catch.
Looks like the documentation of hmm_range_fault is outdated. It says
-EAGAIN can only be returned if block==false, and that mmap_sem is not
dropped if block==true. Both of these statements are no longer true.
The example code in Documentation/vm/hmm.rst drops the mmap_sem
explicitly after hmm_range_snapshot returned -EAGAIN. It seems that
hmm_range_snapshot behaves subtly differently from hmm_range_fault with
respect to dropping the mmap_sem. That's very confusing and annoying.
Regards,
Felix
> I find v2 was incorrect after
> reading hmm code again today. I should mention details of this change in
> my commit, see more below.
>
> Philip
>
>> See more below ...
>>
>>
>>> Change-Id: I1a00f88459f3b96c9536933e9a17eb1ebaa78113
>>> Signed-off-by: Philip Yang
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c | 1 -
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 149 ++--
>>> 2 files changed, 64 insertions(+), 86 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
>>> index 41ccee49a224..e0bb47ce570b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
>>> @@ -519,7 +519,6 @@ void amdgpu_hmm_init_range(struct hmm_range *range)
>>> range->flags = hmm_range_flags;
>>> range->values = hmm_range_values;
>>> range->pfn_shift = PAGE_SHIFT;
>>> - range->pfns = NULL;
>>> INIT_LIST_HEAD(>list);
>>> }
>>> }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> index eb4b85061c7e..9de6a2f5e572 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> @@ -711,8 +711,7 @@ struct amdgpu_ttm_tt {
>>> struct task_struct *usertask;
>>> uint32_tuserflags;
>>> #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>>> - struct hmm_range*ranges;
>>> - int nr_ranges;
>>> + struct hmm_range*range;
>>> #endif
>>> };
>>>
>>> @@ -725,57 +724,36 @@ struct amdgpu_ttm_tt {
>>> */
>>> #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>>>
>>> -/* Support Userptr pages cross max 16 vmas */
>>> -#define MAX_NR_VMAS(16)
>>> +#define MAX_RETRY_HMM_RANGE_FAULT 16
>>>
>>> int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page
>>> **pages)
>>> {
>>> struct amdgpu_ttm_tt *gtt = (void *)ttm;
>>> struct mm_struct *mm = gtt->usertask->mm;
>>> unsigned long start = gtt->userptr;
>>> - unsigned long end = start + ttm->num_pages * PAGE_SIZE;
>>> - struct vm_area_struct *vma = NULL, *vmas[MAX_NR_VMAS];
>>> - struct hmm_range *ranges;
>>> - unsigned long nr_pages, i;
>>> - uint64_t *pfns, f;
>>> + struct vm_area_struct *vma;
>>> + struct hmm_range *range;
>>> + unsigned long i;
>>> + uint64_t *pfns;
>>> + int retry = 0;
>>> int r = 0;
>>>
>>> if (!mm) /* Happens during process shutdown */
>>> return -ESRCH;
>>>
>>> - down_read(>mmap_sem);
>>> -
>>> - /* user pages may cross multiple VMAs */
>>> - gtt->nr_ranges = 0;
>>> - do {
>>> - unsigned long vm_start;
>>> -
>>> - if (gtt->nr_ranges >= MAX_NR_VMAS) {
>>> - DRM_ERROR("Too many VMAs in userptr range\n");
>>> - r = -EFAULT;
>>> - goto out;
>>> - }
>>> -
>>> - vm_start = vma ? vma->vm_end : start;
>>> - vma = find_vma(mm, vm_start);
>>> - if (unlikely(!vma || vm_start <
Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers v3
On 2019-06-03 5:02 p.m., Kuehling, Felix wrote:
> On 2019-06-03 2:44 p.m., Yang, Philip wrote:
>> HMM provides new APIs and helps in kernel 5.2-rc1 to simplify driver
>> path. The old hmm APIs are deprecated and will be removed in future.
>>
>> Below are changes in driver:
>>
>> 1. Change hmm_vma_fault to hmm_range_register and hmm_range_fault which
>> supports range with multiple vmas, remove the multiple vmas handle path
>> and data structure.
>> 2. Change hmm_vma_range_done to hmm_range_unregister.
>> 3. Use default flags to avoid pre-fill pfn arrays.
>> 4. Use new hmm_device_ helpers.
>>
>> v2: retry if hmm_range_fault returns -EAGAIN
>> v3: define MAX_RETRY_HMM_RANGE_FAULT, skip drop mmap_sem if retry
>
> I think dropping mmap_sem for retry was correct in v2 of this patch. Now
> you will try to take the mmap_sem multiple times without dropping it if
> you retry.
>
> We don't need to hold the mmap_sem during hmm_range_wait_until_valid.
>
hmm_range_fault will drop the mmap_sem internally before returning
-EAGAIN, I think it does this on purpose to avoid upper level call retry
whithout release mmap_sem in between. I find v2 was incorrect after
reading hmm code again today. I should mention details of this change in
my commit, see more below.
Philip
> See more below ...
>
>
>>
>> Change-Id: I1a00f88459f3b96c9536933e9a17eb1ebaa78113
>> Signed-off-by: Philip Yang
>> ---
>>drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c | 1 -
>>drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 149 ++--
>>2 files changed, 64 insertions(+), 86 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
>> index 41ccee49a224..e0bb47ce570b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
>> @@ -519,7 +519,6 @@ void amdgpu_hmm_init_range(struct hmm_range *range)
>> range->flags = hmm_range_flags;
>> range->values = hmm_range_values;
>> range->pfn_shift = PAGE_SHIFT;
>> -range->pfns = NULL;
>> INIT_LIST_HEAD(>list);
>> }
>>}
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index eb4b85061c7e..9de6a2f5e572 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -711,8 +711,7 @@ struct amdgpu_ttm_tt {
>> struct task_struct *usertask;
>> uint32_tuserflags;
>>#if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>> -struct hmm_range*ranges;
>> -int nr_ranges;
>> +struct hmm_range*range;
>>#endif
>>};
>>
>> @@ -725,57 +724,36 @@ struct amdgpu_ttm_tt {
>> */
>>#if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>>
>> -/* Support Userptr pages cross max 16 vmas */
>> -#define MAX_NR_VMAS (16)
>> +#define MAX_RETRY_HMM_RANGE_FAULT 16
>>
>>int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages)
>>{
>> struct amdgpu_ttm_tt *gtt = (void *)ttm;
>> struct mm_struct *mm = gtt->usertask->mm;
>> unsigned long start = gtt->userptr;
>> -unsigned long end = start + ttm->num_pages * PAGE_SIZE;
>> -struct vm_area_struct *vma = NULL, *vmas[MAX_NR_VMAS];
>> -struct hmm_range *ranges;
>> -unsigned long nr_pages, i;
>> -uint64_t *pfns, f;
>> +struct vm_area_struct *vma;
>> +struct hmm_range *range;
>> +unsigned long i;
>> +uint64_t *pfns;
>> +int retry = 0;
>> int r = 0;
>>
>> if (!mm) /* Happens during process shutdown */
>> return -ESRCH;
>>
>> -down_read(>mmap_sem);
>> -
>> -/* user pages may cross multiple VMAs */
>> -gtt->nr_ranges = 0;
>> -do {
>> -unsigned long vm_start;
>> -
>> -if (gtt->nr_ranges >= MAX_NR_VMAS) {
>> -DRM_ERROR("Too many VMAs in userptr range\n");
>> -r = -EFAULT;
>> -goto out;
>> -}
>> -
>> -vm_start = vma ? vma->vm_end : start;
>> -vma = find_vma(mm, vm_start);
>> -if (unlikely(!vma || vm_start < vma->vm_start)) {
>> -r = -EFAULT;
>> -goto out;
>> -}
>> -vmas[gtt->nr_ranges++] = vma;
>> -} while (end > vma->vm_end);
>> -
>> -DRM_DEBUG_DRIVER("0x%lx nr_ranges %d pages 0x%lx\n",
>> -start, gtt->nr_ranges, ttm->num_pages);
>> -
>> +vma = find_vma(mm, start);
>> +if (unlikely(!vma || start < vma->vm_start)) {
>> +r = -EFAULT;
>> +goto out;
>> +}
>> if (unlikely((gtt->userflags & AMDGPU_GEM_USERPTR_ANONONLY) &&
>> -vmas[0]->vm_file)) {
>> +vma->vm_file)) {
>> r = -EPERM;
>> goto out;
>> }
>>
>> -ranges = kvmalloc_array(gtt->nr_ranges, sizeof(*ranges), GFP_KERNEL);
>> -if
Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers v3
On 2019-06-03 2:44 p.m., Yang, Philip wrote:
> HMM provides new APIs and helps in kernel 5.2-rc1 to simplify driver
> path. The old hmm APIs are deprecated and will be removed in future.
>
> Below are changes in driver:
>
> 1. Change hmm_vma_fault to hmm_range_register and hmm_range_fault which
> supports range with multiple vmas, remove the multiple vmas handle path
> and data structure.
> 2. Change hmm_vma_range_done to hmm_range_unregister.
> 3. Use default flags to avoid pre-fill pfn arrays.
> 4. Use new hmm_device_ helpers.
>
> v2: retry if hmm_range_fault returns -EAGAIN
> v3: define MAX_RETRY_HMM_RANGE_FAULT, skip drop mmap_sem if retry
I think dropping mmap_sem for retry was correct in v2 of this patch. Now
you will try to take the mmap_sem multiple times without dropping it if
you retry.
We don't need to hold the mmap_sem during hmm_range_wait_until_valid.
See more below ...
>
> Change-Id: I1a00f88459f3b96c9536933e9a17eb1ebaa78113
> Signed-off-by: Philip Yang
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c | 1 -
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 149 ++--
> 2 files changed, 64 insertions(+), 86 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> index 41ccee49a224..e0bb47ce570b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> @@ -519,7 +519,6 @@ void amdgpu_hmm_init_range(struct hmm_range *range)
> range->flags = hmm_range_flags;
> range->values = hmm_range_values;
> range->pfn_shift = PAGE_SHIFT;
> - range->pfns = NULL;
> INIT_LIST_HEAD(>list);
> }
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index eb4b85061c7e..9de6a2f5e572 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -711,8 +711,7 @@ struct amdgpu_ttm_tt {
> struct task_struct *usertask;
> uint32_tuserflags;
> #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
> - struct hmm_range*ranges;
> - int nr_ranges;
> + struct hmm_range*range;
> #endif
> };
>
> @@ -725,57 +724,36 @@ struct amdgpu_ttm_tt {
>*/
> #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>
> -/* Support Userptr pages cross max 16 vmas */
> -#define MAX_NR_VMAS (16)
> +#define MAX_RETRY_HMM_RANGE_FAULT16
>
> int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages)
> {
> struct amdgpu_ttm_tt *gtt = (void *)ttm;
> struct mm_struct *mm = gtt->usertask->mm;
> unsigned long start = gtt->userptr;
> - unsigned long end = start + ttm->num_pages * PAGE_SIZE;
> - struct vm_area_struct *vma = NULL, *vmas[MAX_NR_VMAS];
> - struct hmm_range *ranges;
> - unsigned long nr_pages, i;
> - uint64_t *pfns, f;
> + struct vm_area_struct *vma;
> + struct hmm_range *range;
> + unsigned long i;
> + uint64_t *pfns;
> + int retry = 0;
> int r = 0;
>
> if (!mm) /* Happens during process shutdown */
> return -ESRCH;
>
> - down_read(>mmap_sem);
> -
> - /* user pages may cross multiple VMAs */
> - gtt->nr_ranges = 0;
> - do {
> - unsigned long vm_start;
> -
> - if (gtt->nr_ranges >= MAX_NR_VMAS) {
> - DRM_ERROR("Too many VMAs in userptr range\n");
> - r = -EFAULT;
> - goto out;
> - }
> -
> - vm_start = vma ? vma->vm_end : start;
> - vma = find_vma(mm, vm_start);
> - if (unlikely(!vma || vm_start < vma->vm_start)) {
> - r = -EFAULT;
> - goto out;
> - }
> - vmas[gtt->nr_ranges++] = vma;
> - } while (end > vma->vm_end);
> -
> - DRM_DEBUG_DRIVER("0x%lx nr_ranges %d pages 0x%lx\n",
> - start, gtt->nr_ranges, ttm->num_pages);
> -
> + vma = find_vma(mm, start);
> + if (unlikely(!vma || start < vma->vm_start)) {
> + r = -EFAULT;
> + goto out;
> + }
> if (unlikely((gtt->userflags & AMDGPU_GEM_USERPTR_ANONONLY) &&
> - vmas[0]->vm_file)) {
> + vma->vm_file)) {
> r = -EPERM;
> goto out;
> }
>
> - ranges = kvmalloc_array(gtt->nr_ranges, sizeof(*ranges), GFP_KERNEL);
> - if (unlikely(!ranges)) {
> + range = kzalloc(sizeof(*range), GFP_KERNEL);
> + if (unlikely(!range)) {
> r = -ENOMEM;
> goto out;
> }
> @@ -786,61 +764,62 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm,
> struct page **pages)
> goto out_free_ranges;
> }
>
> - for (i = 0; i < gtt->nr_ranges; i++)
> - amdgpu_hmm_init_range([i]);
> + amdgpu_hmm_init_range(range);
> +
Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers
On 2019-06-03 7:23 a.m., Christian König wrote: > Am 03.06.19 um 12:17 schrieb Christian König: >> Am 01.06.19 um 00:01 schrieb Kuehling, Felix: >>> On 2019-05-31 5:32 p.m., Yang, Philip wrote: On 2019-05-31 3:42 p.m., Kuehling, Felix wrote: > On 2019-05-31 1:28 p.m., Yang, Philip wrote: >> On 2019-05-30 6:36 p.m., Kuehling, Felix wrote: #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR) - if (gtt->ranges && - ttm->pages[0] == hmm_pfn_to_page(>ranges[0], - gtt->ranges[0].pfns[0])) + if (gtt->range && + ttm->pages[0] == hmm_device_entry_to_page(gtt->range, + gtt->range->pfns[0])) >>> I think just checking gtt->range here is enough. If gtt->range is >>> not >>> NULL here, we're leaking memory. >>> >> If just checking gtt->range, there is a false warning in amdgpu_test >> userptr case in amdgpu_cs_list_validate path. If userptr is >> invalidated, >> then ttm->pages[0] is outdated pages, lobj->user_pages is new >> pages, it >> goes to ttm_tt_unbind first to unpin old pages (this causes false >> warning) then call amdgpu_ttm_tt_set_user_pages. > But doesn't that mean we're leaking the gtt->range somewhere? > ttm_tt_unbind is called from ttm_tt_destroy and amdgpu_cs_list_validate, the later one causes false warning. ttm_ttm_destory path is fine to only check gtt->range. Double checked, amdgpu_ttm_tt_get_user_pages and amdgpu_ttm_tt_get_user_pages_done always match in both paths, so no leak gtt->range. 1. amdgpu_gem_userptr_ioctl amdgpu_ttm_tt_get_user_pages ttm->pages for userptr pages amdgpu_ttm_tt_get_user_pages_done 2. amdgpu_cs_ioctl amdgpu_cs_parser_bos amdgpu_ttm_tt_get_user_pages if (userpage_invalidated) e->user_pages for new pages amdgpu_cs_list_validate if (userpage_invalidated) ttm_tt_unbind ttm->pages // this causes warning amdgpu_ttm_tt_set_user_pages(ttm->pages, e->user_pages) >>> Hmm, I think amdgpu_cs is doing something weird there. It does some >>> double book-keeping of the user pages in the BO list and the TTM BO. We >>> did something similar for KFD and simplified it when moving to HMM. It >>> could probably be simplified for amdgpu_cs as well. But not in this >>> patch series. >> >> That actually sounds like a bug to me. >> >> It is most likely a leftover from the time when we couldn't get the >> pages for a BO while the BO was reserved. > > Mhm, at least it's not racy in the way I thought it would be. But it is > certainly still overkill and should be simplified. > > Philip are you taking a look or should I tackle this? > Hi Christian, I will submit another patch to simplify amdgpu_cs_ioctl path, please help review it. Thanks, Philip > Thanks, > Christian. > >> >> >> Going to take a closer look, >> Christian. >> >>> >>> I'll review your updated change. >>> >>> Thanks, >>> Felix >>> >>> amdgpu_cs_submit amdgpu_ttm_tt_get_user_pages_done > Regards, > Felix > > >> I will submit patch v2, to add retry if hmm_range_fault returns >> -EAGAIN. >> use kzalloc to allocate small size range. >> >> Thanks, >> Philip >> >>> Regards, >>> Felix >>> >>> >>> ___ >>> amd-gfx mailing list >>> amd-gfx@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers
Am 03.06.19 um 12:17 schrieb Christian König: Am 01.06.19 um 00:01 schrieb Kuehling, Felix: On 2019-05-31 5:32 p.m., Yang, Philip wrote: On 2019-05-31 3:42 p.m., Kuehling, Felix wrote: On 2019-05-31 1:28 p.m., Yang, Philip wrote: On 2019-05-30 6:36 p.m., Kuehling, Felix wrote: #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR) - if (gtt->ranges && - ttm->pages[0] == hmm_pfn_to_page(>ranges[0], - gtt->ranges[0].pfns[0])) + if (gtt->range && + ttm->pages[0] == hmm_device_entry_to_page(gtt->range, + gtt->range->pfns[0])) I think just checking gtt->range here is enough. If gtt->range is not NULL here, we're leaking memory. If just checking gtt->range, there is a false warning in amdgpu_test userptr case in amdgpu_cs_list_validate path. If userptr is invalidated, then ttm->pages[0] is outdated pages, lobj->user_pages is new pages, it goes to ttm_tt_unbind first to unpin old pages (this causes false warning) then call amdgpu_ttm_tt_set_user_pages. But doesn't that mean we're leaking the gtt->range somewhere? ttm_tt_unbind is called from ttm_tt_destroy and amdgpu_cs_list_validate, the later one causes false warning. ttm_ttm_destory path is fine to only check gtt->range. Double checked, amdgpu_ttm_tt_get_user_pages and amdgpu_ttm_tt_get_user_pages_done always match in both paths, so no leak gtt->range. 1. amdgpu_gem_userptr_ioctl amdgpu_ttm_tt_get_user_pages ttm->pages for userptr pages amdgpu_ttm_tt_get_user_pages_done 2. amdgpu_cs_ioctl amdgpu_cs_parser_bos amdgpu_ttm_tt_get_user_pages if (userpage_invalidated) e->user_pages for new pages amdgpu_cs_list_validate if (userpage_invalidated) ttm_tt_unbind ttm->pages // this causes warning amdgpu_ttm_tt_set_user_pages(ttm->pages, e->user_pages) Hmm, I think amdgpu_cs is doing something weird there. It does some double book-keeping of the user pages in the BO list and the TTM BO. We did something similar for KFD and simplified it when moving to HMM. It could probably be simplified for amdgpu_cs as well. But not in this patch series. That actually sounds like a bug to me. It is most likely a leftover from the time when we couldn't get the pages for a BO while the BO was reserved. Mhm, at least it's not racy in the way I thought it would be. But it is certainly still overkill and should be simplified. Philip are you taking a look or should I tackle this? Thanks, Christian. Going to take a closer look, Christian. I'll review your updated change. Thanks, Felix amdgpu_cs_submit amdgpu_ttm_tt_get_user_pages_done Regards, Felix I will submit patch v2, to add retry if hmm_range_fault returns -EAGAIN. use kzalloc to allocate small size range. Thanks, Philip Regards, Felix ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers
Am 01.06.19 um 00:01 schrieb Kuehling, Felix: On 2019-05-31 5:32 p.m., Yang, Philip wrote: On 2019-05-31 3:42 p.m., Kuehling, Felix wrote: On 2019-05-31 1:28 p.m., Yang, Philip wrote: On 2019-05-30 6:36 p.m., Kuehling, Felix wrote: #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR) - if (gtt->ranges && - ttm->pages[0] == hmm_pfn_to_page(>ranges[0], -gtt->ranges[0].pfns[0])) + if (gtt->range && + ttm->pages[0] == hmm_device_entry_to_page(gtt->range, + gtt->range->pfns[0])) I think just checking gtt->range here is enough. If gtt->range is not NULL here, we're leaking memory. If just checking gtt->range, there is a false warning in amdgpu_test userptr case in amdgpu_cs_list_validate path. If userptr is invalidated, then ttm->pages[0] is outdated pages, lobj->user_pages is new pages, it goes to ttm_tt_unbind first to unpin old pages (this causes false warning) then call amdgpu_ttm_tt_set_user_pages. But doesn't that mean we're leaking the gtt->range somewhere? ttm_tt_unbind is called from ttm_tt_destroy and amdgpu_cs_list_validate, the later one causes false warning. ttm_ttm_destory path is fine to only check gtt->range. Double checked, amdgpu_ttm_tt_get_user_pages and amdgpu_ttm_tt_get_user_pages_done always match in both paths, so no leak gtt->range. 1. amdgpu_gem_userptr_ioctl amdgpu_ttm_tt_get_user_pages ttm->pages for userptr pages amdgpu_ttm_tt_get_user_pages_done 2. amdgpu_cs_ioctl amdgpu_cs_parser_bos amdgpu_ttm_tt_get_user_pages if (userpage_invalidated) e->user_pages for new pages amdgpu_cs_list_validate if (userpage_invalidated) ttm_tt_unbind ttm->pages // this causes warning amdgpu_ttm_tt_set_user_pages(ttm->pages, e->user_pages) Hmm, I think amdgpu_cs is doing something weird there. It does some double book-keeping of the user pages in the BO list and the TTM BO. We did something similar for KFD and simplified it when moving to HMM. It could probably be simplified for amdgpu_cs as well. But not in this patch series. That actually sounds like a bug to me. It is most likely a leftover from the time when we couldn't get the pages for a BO while the BO was reserved. Going to take a closer look, Christian. I'll review your updated change. Thanks, Felix amdgpu_cs_submit amdgpu_ttm_tt_get_user_pages_done Regards, Felix I will submit patch v2, to add retry if hmm_range_fault returns -EAGAIN. use kzalloc to allocate small size range. Thanks, Philip Regards, Felix ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers v2
On 2019-05-31 3:09 p.m., Yang, Philip wrote:
> HMM provides new APIs and helps in kernel 5.2-rc1 to simplify driver
> path. The old hmm APIs are deprecated and will be removed in future.
>
> Below are changes in driver:
>
> 1. Change hmm_vma_fault to hmm_range_register and hmm_range_fault which
> supports range with multiple vmas, remove the multiple vmas handle path
> and data structure.
> 2. Change hmm_vma_range_done to hmm_range_unregister.
> 3. Use default flags to avoid pre-fill pfn arrays.
> 4. Use new hmm_device_ helpers.
>
> v2: retry if hmm_range_fault returns -EAGAIN
>
> Change-Id: I1a00f88459f3b96c9536933e9a17eb1ebaa78113
> Signed-off-by: Philip Yang
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c | 1 -
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 150 ++--
> 2 files changed, 63 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> index 41ccee49a224..e0bb47ce570b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> @@ -519,7 +519,6 @@ void amdgpu_hmm_init_range(struct hmm_range *range)
> range->flags = hmm_range_flags;
> range->values = hmm_range_values;
> range->pfn_shift = PAGE_SHIFT;
> - range->pfns = NULL;
> INIT_LIST_HEAD(>list);
> }
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index eb4b85061c7e..f129160939bc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -711,8 +711,7 @@ struct amdgpu_ttm_tt {
> struct task_struct *usertask;
> uint32_tuserflags;
> #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
> - struct hmm_range*ranges;
> - int nr_ranges;
> + struct hmm_range*range;
> #endif
> };
>
> @@ -725,57 +724,34 @@ struct amdgpu_ttm_tt {
>*/
> #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>
> -/* Support Userptr pages cross max 16 vmas */
> -#define MAX_NR_VMAS (16)
> -
> int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages)
> {
> struct amdgpu_ttm_tt *gtt = (void *)ttm;
> struct mm_struct *mm = gtt->usertask->mm;
> unsigned long start = gtt->userptr;
> - unsigned long end = start + ttm->num_pages * PAGE_SIZE;
> - struct vm_area_struct *vma = NULL, *vmas[MAX_NR_VMAS];
> - struct hmm_range *ranges;
> - unsigned long nr_pages, i;
> - uint64_t *pfns, f;
> + struct vm_area_struct *vma;
> + struct hmm_range *range;
> + unsigned long i;
> + uint64_t *pfns;
> + int retry = 0;
> int r = 0;
>
> if (!mm) /* Happens during process shutdown */
> return -ESRCH;
>
> - down_read(>mmap_sem);
> -
> - /* user pages may cross multiple VMAs */
> - gtt->nr_ranges = 0;
> - do {
> - unsigned long vm_start;
> -
> - if (gtt->nr_ranges >= MAX_NR_VMAS) {
> - DRM_ERROR("Too many VMAs in userptr range\n");
> - r = -EFAULT;
> - goto out;
> - }
> -
> - vm_start = vma ? vma->vm_end : start;
> - vma = find_vma(mm, vm_start);
> - if (unlikely(!vma || vm_start < vma->vm_start)) {
> - r = -EFAULT;
> - goto out;
> - }
> - vmas[gtt->nr_ranges++] = vma;
> - } while (end > vma->vm_end);
> -
> - DRM_DEBUG_DRIVER("0x%lx nr_ranges %d pages 0x%lx\n",
> - start, gtt->nr_ranges, ttm->num_pages);
> -
> + vma = find_vma(mm, start);
> + if (unlikely(!vma || start < vma->vm_start)) {
> + r = -EFAULT;
> + goto out;
> + }
> if (unlikely((gtt->userflags & AMDGPU_GEM_USERPTR_ANONONLY) &&
> - vmas[0]->vm_file)) {
> + vma->vm_file)) {
> r = -EPERM;
> goto out;
> }
>
> - ranges = kvmalloc_array(gtt->nr_ranges, sizeof(*ranges), GFP_KERNEL);
> - if (unlikely(!ranges)) {
> + range = kzalloc(sizeof(*range), GFP_KERNEL);
> + if (unlikely(!range)) {
> r = -ENOMEM;
> goto out;
> }
> @@ -786,61 +762,61 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm,
> struct page **pages)
> goto out_free_ranges;
> }
>
> - for (i = 0; i < gtt->nr_ranges; i++)
> - amdgpu_hmm_init_range([i]);
> -
> - f = ranges[0].flags[HMM_PFN_VALID];
> - f |= amdgpu_ttm_tt_is_readonly(ttm) ?
> - 0 : ranges[0].flags[HMM_PFN_WRITE];
> - memset64(pfns, f, ttm->num_pages);
> + amdgpu_hmm_init_range(range);
> + range->default_flags = range->flags[HMM_PFN_VALID];
> + range->default_flags |= amdgpu_ttm_tt_is_readonly(ttm) ?
> + 0 :
Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers
On 2019-05-31 5:32 p.m., Yang, Philip wrote: > > On 2019-05-31 3:42 p.m., Kuehling, Felix wrote: >> On 2019-05-31 1:28 p.m., Yang, Philip wrote: >>> On 2019-05-30 6:36 p.m., Kuehling, Felix wrote: > > #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR) > - if (gtt->ranges && > - ttm->pages[0] == hmm_pfn_to_page(>ranges[0], > - gtt->ranges[0].pfns[0])) > + if (gtt->range && > + ttm->pages[0] == hmm_device_entry_to_page(gtt->range, > + gtt->range->pfns[0])) I think just checking gtt->range here is enough. If gtt->range is not NULL here, we're leaking memory. >>> If just checking gtt->range, there is a false warning in amdgpu_test >>> userptr case in amdgpu_cs_list_validate path. If userptr is invalidated, >>> then ttm->pages[0] is outdated pages, lobj->user_pages is new pages, it >>> goes to ttm_tt_unbind first to unpin old pages (this causes false >>> warning) then call amdgpu_ttm_tt_set_user_pages. >> But doesn't that mean we're leaking the gtt->range somewhere? >> > ttm_tt_unbind is called from ttm_tt_destroy and amdgpu_cs_list_validate, > the later one causes false warning. ttm_ttm_destory path is fine to only > check gtt->range. > > Double checked, amdgpu_ttm_tt_get_user_pages and > amdgpu_ttm_tt_get_user_pages_done always match in both paths, so no leak > gtt->range. > > 1. amdgpu_gem_userptr_ioctl > amdgpu_ttm_tt_get_user_pages > ttm->pages for userptr pages > amdgpu_ttm_tt_get_user_pages_done > > 2. amdgpu_cs_ioctl > amdgpu_cs_parser_bos > amdgpu_ttm_tt_get_user_pages > if (userpage_invalidated) > e->user_pages for new pages > amdgpu_cs_list_validate > if (userpage_invalidated) >ttm_tt_unbind ttm->pages // this causes warning >amdgpu_ttm_tt_set_user_pages(ttm->pages, e->user_pages) Hmm, I think amdgpu_cs is doing something weird there. It does some double book-keeping of the user pages in the BO list and the TTM BO. We did something similar for KFD and simplified it when moving to HMM. It could probably be simplified for amdgpu_cs as well. But not in this patch series. I'll review your updated change. Thanks, Felix > amdgpu_cs_submit > amdgpu_ttm_tt_get_user_pages_done > >> Regards, >> Felix >> >> >>> I will submit patch v2, to add retry if hmm_range_fault returns -EAGAIN. >>> use kzalloc to allocate small size range. >>> >>> Thanks, >>> Philip >>> Regards, Felix ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers
On 2019-05-31 3:42 p.m., Kuehling, Felix wrote: > On 2019-05-31 1:28 p.m., Yang, Philip wrote: >> >> On 2019-05-30 6:36 p.m., Kuehling, Felix wrote: #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR) - if (gtt->ranges && - ttm->pages[0] == hmm_pfn_to_page(>ranges[0], - gtt->ranges[0].pfns[0])) + if (gtt->range && + ttm->pages[0] == hmm_device_entry_to_page(gtt->range, +gtt->range->pfns[0])) >>> I think just checking gtt->range here is enough. If gtt->range is not >>> NULL here, we're leaking memory. >>> >> If just checking gtt->range, there is a false warning in amdgpu_test >> userptr case in amdgpu_cs_list_validate path. If userptr is invalidated, >> then ttm->pages[0] is outdated pages, lobj->user_pages is new pages, it >> goes to ttm_tt_unbind first to unpin old pages (this causes false >> warning) then call amdgpu_ttm_tt_set_user_pages. > > But doesn't that mean we're leaking the gtt->range somewhere? > ttm_tt_unbind is called from ttm_tt_destroy and amdgpu_cs_list_validate, the later one causes false warning. ttm_ttm_destory path is fine to only check gtt->range. Double checked, amdgpu_ttm_tt_get_user_pages and amdgpu_ttm_tt_get_user_pages_done always match in both paths, so no leak gtt->range. 1. amdgpu_gem_userptr_ioctl amdgpu_ttm_tt_get_user_pages ttm->pages for userptr pages amdgpu_ttm_tt_get_user_pages_done 2. amdgpu_cs_ioctl amdgpu_cs_parser_bos amdgpu_ttm_tt_get_user_pages if (userpage_invalidated) e->user_pages for new pages amdgpu_cs_list_validate if (userpage_invalidated) ttm_tt_unbind ttm->pages // this causes warning amdgpu_ttm_tt_set_user_pages(ttm->pages, e->user_pages) amdgpu_cs_submit amdgpu_ttm_tt_get_user_pages_done > Regards, > Felix > > >> >> I will submit patch v2, to add retry if hmm_range_fault returns -EAGAIN. >> use kzalloc to allocate small size range. >> >> Thanks, >> Philip >> >>> Regards, >>> Felix >>> >>> ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers
On 2019-05-31 1:28 p.m., Yang, Philip wrote: > > On 2019-05-30 6:36 p.m., Kuehling, Felix wrote: >>> >>> #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR) >>> - if (gtt->ranges && >>> - ttm->pages[0] == hmm_pfn_to_page(>ranges[0], >>> -gtt->ranges[0].pfns[0])) >>> + if (gtt->range && >>> + ttm->pages[0] == hmm_device_entry_to_page(gtt->range, >>> + gtt->range->pfns[0])) >> I think just checking gtt->range here is enough. If gtt->range is not >> NULL here, we're leaking memory. >> > If just checking gtt->range, there is a false warning in amdgpu_test > userptr case in amdgpu_cs_list_validate path. If userptr is invalidated, > then ttm->pages[0] is outdated pages, lobj->user_pages is new pages, it > goes to ttm_tt_unbind first to unpin old pages (this causes false > warning) then call amdgpu_ttm_tt_set_user_pages. But doesn't that mean we're leaking the gtt->range somewhere? Regards, Felix > > I will submit patch v2, to add retry if hmm_range_fault returns -EAGAIN. > use kzalloc to allocate small size range. > > Thanks, > Philip > >> Regards, >> Felix >> >> ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers
On 2019-05-30 6:36 p.m., Kuehling, Felix wrote: >> >>#if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR) >> -if (gtt->ranges && >> -ttm->pages[0] == hmm_pfn_to_page(>ranges[0], >> - gtt->ranges[0].pfns[0])) >> +if (gtt->range && >> +ttm->pages[0] == hmm_device_entry_to_page(gtt->range, >> + gtt->range->pfns[0])) > I think just checking gtt->range here is enough. If gtt->range is not > NULL here, we're leaking memory. > If just checking gtt->range, there is a false warning in amdgpu_test userptr case in amdgpu_cs_list_validate path. If userptr is invalidated, then ttm->pages[0] is outdated pages, lobj->user_pages is new pages, it goes to ttm_tt_unbind first to unpin old pages (this causes false warning) then call amdgpu_ttm_tt_set_user_pages. I will submit patch v2, to add retry if hmm_range_fault returns -EAGAIN. use kzalloc to allocate small size range. Thanks, Philip > Regards, > Felix > > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers
This is a nice simplification. See a few comments inline.
On 2019-05-30 10:41 a.m., Yang, Philip wrote:
> HMM provides new APIs and helps in kernel 5.2-rc1 to simplify driver
> path. The old hmm APIs are deprecated and will be removed in future.
>
> Below are changes in driver:
>
> 1. Change hmm_vma_fault to hmm_range_register and hmm_range_fault which
> supports range with multiple vmas, remove the multiple vmas handle path
> and data structure.
> 2. Change hmm_vma_range_done to hmm_range_unregister.
> 3. Use default flags to avoid pre-fill pfn arrays.
> 4. Use new hmm_device_ helpers.
>
> Change-Id: I1a00f88459f3b96c9536933e9a17eb1ebaa78113
> Signed-off-by: Philip Yang
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c | 1 -
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 140 +---
> 2 files changed, 53 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> index 41ccee49a224..e0bb47ce570b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
> @@ -519,7 +519,6 @@ void amdgpu_hmm_init_range(struct hmm_range *range)
> range->flags = hmm_range_flags;
> range->values = hmm_range_values;
> range->pfn_shift = PAGE_SHIFT;
> - range->pfns = NULL;
> INIT_LIST_HEAD(>list);
> }
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 7138dc1dd1f4..25a9fcb409c6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -711,8 +711,7 @@ struct amdgpu_ttm_tt {
> struct task_struct *usertask;
> uint32_tuserflags;
> #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
> - struct hmm_range*ranges;
> - int nr_ranges;
> + struct hmm_range*range;
> #endif
> };
>
> @@ -725,57 +724,33 @@ struct amdgpu_ttm_tt {
>*/
> #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>
> -/* Support Userptr pages cross max 16 vmas */
> -#define MAX_NR_VMAS (16)
> -
> int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages)
> {
> struct amdgpu_ttm_tt *gtt = (void *)ttm;
> struct mm_struct *mm = gtt->usertask->mm;
> unsigned long start = gtt->userptr;
> - unsigned long end = start + ttm->num_pages * PAGE_SIZE;
> - struct vm_area_struct *vma = NULL, *vmas[MAX_NR_VMAS];
> - struct hmm_range *ranges;
> - unsigned long nr_pages, i;
> - uint64_t *pfns, f;
> + struct vm_area_struct *vma;
> + struct hmm_range *range;
> + unsigned long i;
> + uint64_t *pfns;
> int r = 0;
>
> if (!mm) /* Happens during process shutdown */
> return -ESRCH;
>
> - down_read(>mmap_sem);
> -
> - /* user pages may cross multiple VMAs */
> - gtt->nr_ranges = 0;
> - do {
> - unsigned long vm_start;
> -
> - if (gtt->nr_ranges >= MAX_NR_VMAS) {
> - DRM_ERROR("Too many VMAs in userptr range\n");
> - r = -EFAULT;
> - goto out;
> - }
> -
> - vm_start = vma ? vma->vm_end : start;
> - vma = find_vma(mm, vm_start);
> - if (unlikely(!vma || vm_start < vma->vm_start)) {
> - r = -EFAULT;
> - goto out;
> - }
> - vmas[gtt->nr_ranges++] = vma;
> - } while (end > vma->vm_end);
> -
> - DRM_DEBUG_DRIVER("0x%lx nr_ranges %d pages 0x%lx\n",
> - start, gtt->nr_ranges, ttm->num_pages);
> -
> + vma = find_vma(mm, start);
> + if (unlikely(!vma || start < vma->vm_start)) {
> + r = -EFAULT;
> + goto out;
> + }
> if (unlikely((gtt->userflags & AMDGPU_GEM_USERPTR_ANONONLY) &&
> - vmas[0]->vm_file)) {
> + vma->vm_file)) {
> r = -EPERM;
> goto out;
> }
>
> - ranges = kvmalloc_array(gtt->nr_ranges, sizeof(*ranges), GFP_KERNEL);
> - if (unlikely(!ranges)) {
> + range = kvmalloc(sizeof(*range), GFP_KERNEL | __GFP_ZERO);
A single range is pretty small. You can probably allocate that with
kmalloc now (or kcalloc/kzalloc since you specified GFP_ZERO).
> + if (unlikely(!range)) {
> r = -ENOMEM;
> goto out;
> }
> @@ -786,61 +761,52 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm,
> struct page **pages)
> goto out_free_ranges;
> }
>
> - for (i = 0; i < gtt->nr_ranges; i++)
> - amdgpu_hmm_init_range([i]);
> + amdgpu_hmm_init_range(range);
> + range->default_flags = range->flags[HMM_PFN_VALID];
> + range->default_flags |= amdgpu_ttm_tt_is_readonly(ttm) ?
> + 0 : range->flags[HMM_PFN_WRITE];
> + range->pfn_flags_mask = 0;
> + range->pfns
