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. 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 <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) { > - 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
