Re: [PATCH] drm/amdgpu: use new HMM APIs and helpers

["Yang, Philip" <Philip.Yang@xxxxxxx>]

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(&gtt->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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx