Felix Kuehling
2021-Apr-07 19:34 UTC
[Nouveau] [PATCH 3/8] drm/amdgpu: Implement mmap as GEM object function
On 2021-04-07 7:25 a.m., Christian K?nig wrote:>>>>> +??? /* >>>>> +???? * Don't verify access for KFD BOs. They don't have a GEM >>>>> +???? * object associated with them. >>>>> +???? */ >>>>> +??? if (bo->kfd_bo) >>>>> +??????? goto out; >>>> Who does the access verification now? >>> This is somewhat confusing. >>> >>> I took this check as-is, including the comment, from amdgpu's >>> verify_access function. The verify_access function was called by >>> ttm_bo_mmap. It returned 0 and ttm_bo_mmap did the mapping. >> This is probably a left-over from when we mapped BOs using /dev/kfd. We >> changed this to use /dev/dri/renderD* a long time ago to fix CPU mapping >> invalidations on memory evictions. I think we can let GEM do the access >> check. > > Ok, good to know. > > Thomas can you remove the extra handling in a separate prerequisite > patch? > > If anybody then bisects to this patch we at least know what to do to > get it working again.FWIW, I ran KFDTest test with this shortcut removed on current amd-staging-drm-next + my HMM patch series, and it didn't seem to cause any issues. Regards, ? Felix> > Regards, > Christian.
Felix Kuehling
2021-Apr-07 19:49 UTC
[Nouveau] [PATCH 3/8] drm/amdgpu: Implement mmap as GEM object function
On 2021-04-07 3:34 p.m., Felix Kuehling wrote:> On 2021-04-07 7:25 a.m., Christian K?nig wrote: >>>>>> +??? /* >>>>>> +???? * Don't verify access for KFD BOs. They don't have a GEM >>>>>> +???? * object associated with them. >>>>>> +???? */ >>>>>> +??? if (bo->kfd_bo) >>>>>> +??????? goto out; >>>>> Who does the access verification now? >>>> This is somewhat confusing. >>>> >>>> I took this check as-is, including the comment, from amdgpu's >>>> verify_access function. The verify_access function was called by >>>> ttm_bo_mmap. It returned 0 and ttm_bo_mmap did the mapping. >>> This is probably a left-over from when we mapped BOs using /dev/kfd. We >>> changed this to use /dev/dri/renderD* a long time ago to fix CPU >>> mapping >>> invalidations on memory evictions. I think we can let GEM do the access >>> check. >> >> Ok, good to know. >> >> Thomas can you remove the extra handling in a separate prerequisite >> patch? >> >> If anybody then bisects to this patch we at least know what to do to >> get it working again. > > FWIW, I ran KFDTest test with this shortcut removed on current > amd-staging-drm-next + my HMM patch series, and it didn't seem to > cause any issues.Wait, I celebrated too soon. I was running the wrong kernel. I do see some failures where access is being denied. I need to do more debugging to figure out what's causing that. Regards, ? Felix> > Regards, > ? Felix > > >> >> Regards, >> Christian.