On 2019-10-11 1:33 p.m., Kuehling, Felix wrote: > On 2019-10-11 10:36 a.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. >> >> v2: remove unused local variable and amend commit >> >> v3: add back get user pages in gem_userptr_ioctl, to detect application >> bug where an userptr VMA is not ananymous memory and reject it. >> >> Bugzilla: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962 >> >> Signed-off-by: Philip Yang <philip.y...@amd.com> >> Tested-by: Joe Barnett <the...@gmail.com> >> Reviewed-by: Christian König <christian.koe...@amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c >> index c18a153b3d2a..e7b39daa22f6 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c >> @@ -476,7 +476,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); >> @@ -493,14 +492,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) { > > This if is not needed. kvfree should be able to handle NULL pointers, > and unconditionally setting the pointer to NULL afterwards is not a > problem either. With that fixed, this commit is > > Reviewed-by: Felix Kuehling <felix.kuehl...@amd.com> > > However, I don't think this should be the final solution. My concern > with this solution is, that you end up freeing and regenerating the > user_pages arrays more frequently than necessary: On every command > submission, even if there was no MMU notifier since the last command > submission. I was hoping we could get back to a solution where we can > maintain the same user_pages array across command submissions, since MMU > notifiers are rare. That should reduce overhead from doing all thos page > table walks in HMM on every command submissions when using userptrs. > Yes, I will have another patch to address this using hmm_range_valid, the idea is to allow hmm range tracking cross gem_userptr_ioctl and cs_ioctl.
Thanks, Philip > Regards, > Felix > > >> kvfree(lobj->user_pages); >> lobj->user_pages = NULL; >> } _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
