Thanks for looking at this Christian. Let me know if there's anything I can do to help.
In the meantime, is there a workaround to avoid the memory leak other than using a kernel from before the HMM change? Thanks, -Joe On Fri, Oct 4, 2019 at 8:02 AM Koenig, Christian <[email protected]> wrote: > Hi Philip, > > Am 04.10.19 um 15:40 schrieb Yang, Philip: > > Thanks Joe for the test, I will add your Tested-by. > > > > Hi Christian, > > > > May you help review? The change removes the get user pages from > > gem_userptr_ioctl, this was done if flags AMDGPU_GEM_USERPTR_VALIDATE is > > set, and delay the get user pages to amdgpu_cs_parser_bos, and check if > > user pages are invalidated when amdgpu_cs_submit. I don't find issue for > > overnight test, but not sure if there is potential side effect. > > Yeah, seen that. > > The AMDGPU_GEM_USERPTR_VALIDATE was explicitly added to cause a > validation during BO creation because of some very stupid applications. > > I didn't wanted to reject that without offering an alternative, but we > seriously can't do this or it would break Vulkan/OpenGL. > > I need more time to take a closer look, > Christian. > > > > > Thanks, > > Philip > > > > On 2019-10-03 3:44 p.m., Yang, Philip wrote: > >> 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 <[email protected]> > >> --- > >> 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) { > >> - 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 [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
