On 2019-10-11 4:40 a.m., Christian König wrote: > Am 03.10.19 um 21:44 schrieb Yang, Philip: >> user_pages array should always be freed after validation regardless if >> user pages are changed after bo is created because with HMM change parse >> bo always allocate user pages array to get user pages for userptr bo. >> >> Don't need to get user pages while creating uerptr bo because user pages >> will only be used while validating after parsing userptr bo. >> >> Bugzilla: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962 >> >> v2: remove unused local variable and amend commit >> >> Signed-off-by: Philip Yang <Philip.Yang@xxxxxxx> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +--- >> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 23 +---------------------- >> 2 files changed, 2 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c >> index 49b767b7238f..961186e7113e 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c >> @@ -474,7 +474,6 @@ static int amdgpu_cs_list_validate(struct >> amdgpu_cs_parser *p, >> list_for_each_entry(lobj, validated, tv.head) { >> struct amdgpu_bo *bo = ttm_to_amdgpu_bo(lobj->tv.bo); >> - bool binding_userptr = false; >> struct mm_struct *usermm; >> usermm = amdgpu_ttm_tt_get_usermm(bo->tbo.ttm); >> @@ -491,14 +490,13 @@ static int amdgpu_cs_list_validate(struct >> amdgpu_cs_parser *p, >> amdgpu_ttm_tt_set_user_pages(bo->tbo.ttm, >> lobj->user_pages); >> - binding_userptr = true; >> } >> r = amdgpu_cs_validate(p, bo); >> if (r) >> return r; >> - if (binding_userptr) { >> + if (lobj->user_pages) { >> kvfree(lobj->user_pages); >> lobj->user_pages = NULL; >> } >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c >> index a828e3d0bfbd..3ccd61d69964 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c >> @@ -283,7 +283,6 @@ int amdgpu_gem_create_ioctl(struct drm_device >> *dev, void *data, >> int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data, >> struct drm_file *filp) >> { >> - struct ttm_operation_ctx ctx = { true, false }; >> struct amdgpu_device *adev = dev->dev_private; >> struct drm_amdgpu_gem_userptr *args = data; >> struct drm_gem_object *gobj; >> @@ -326,32 +325,12 @@ int amdgpu_gem_userptr_ioctl(struct drm_device >> *dev, void *data, >> goto release_object; >> } >> - if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE) { > > We can't drop that handling here, it is mandatory to detect an > application bug where an application tries to user an userptr with a VMA > which is not anonymous memory. > > This must be detected and rejected as invalid here. > > I suggest that we allocate a local pages array similar to how we do it > during CS and release that after the function is done. > Thanks for this, we can use bo->tbo.ttm->pages array here to avoid extra alloc/free of pages array because CS uses local pages array and update to bo->tbo.ttm->pages if user pages are moved. I will submit patch v3 for review. > Regards, > Christian. > >> - r = amdgpu_ttm_tt_get_user_pages(bo, bo->tbo.ttm->pages); >> - if (r) >> - goto release_object; >> - >> - r = amdgpu_bo_reserve(bo, true); >> - if (r) >> - goto user_pages_done; >> - >> - amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT); >> - r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); >> - amdgpu_bo_unreserve(bo); >> - if (r) >> - goto user_pages_done; >> - } >> - >> r = drm_gem_handle_create(filp, gobj, &handle); >> if (r) >> - goto user_pages_done; >> + goto release_object; >> args->handle = handle; >> -user_pages_done: >> - if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE) >> - amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm); >> - >> release_object: >> drm_gem_object_put_unlocked(gobj); > _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
