On 2018-03-19 12:39 PM, Christian König wrote:
> So coming back to this series once more.
>
> Patch #1, #3 are Reviewed-by: Christian König <christian.koenig at amd.com>.
>
> Patch #2, #4 - #13 and #18-#19 are Acked-by: Christian König
> <christian.koenig at amd.com>.
>
> Patch #14: What's the difference to setting vramlimit=$size_of_bar ?
The debug_largebar option only affects KFD. Graphics can still use all
memory.
>
> Patch #15 & #20: Why is that actually still needed? I thought we have
> fixed all dependencies and can now use the "standard" way of attaching
> fences to reservation objects to do this.
Patch 15 adds a KFD-specific MMU notifier, because the graphics MMU
notifier deals with amdgpu_cs command submission. We need a completely
different treatment of MMU notifiers for KFD. We need to stop user mode
queues.
Patch 20 implements the user mode queue preemption mechanism, and the
corresponding restore function that re-validates userptr BOs before
restarting user mode queues.
I think you're implying that the graphics MMU notifier would wait for
the eviction fence and trigger a regular eviction in KFD. I haven't
tried that. The MMU notifier and userptr eviction mechanism was
implemented in KFD before we had the TTM evictions. We didn't go back
and revisit it after that. There are a few major differences that make
me want to keep the two types of evictions separate, though:
* TTM evictions are the result of memory pressure (triggered by
another process)
o Most MMU notifiers are triggered by something in the same
process (fork, mprotect, etc.)
o Thus the restore delay after MMU notifiers can be much shorter
* TTM evictions are done asynchronously in a delayed worker
o MMU notifiers are synchronous, queues need to be stopped before
returning
* KFD's TTM eviction/restore code doesn't handle userptrs
(get_user_pages, etc)
o MMU notifier restore worker is specialized to handle just userptrs
>
> Saying so I still need to take a closer look at patch #20.
>
> Patch #16: Looks good to me in general, but I think it would be safer
> if we grab a reference to the task structure. Otherwise grabbing pages
> from a mm_struct sounds a bit scary to me.
You're right. I've never seen it cause problems when testing process
termination, probably because during process termination KFD cancels the
delayed worker that calls this function. But I'll fix this and take a
proper reference.
>
> Patch #17: I think it would be better to allocate the node when the
> locks are not held and free it when we find that it isn't used, but no
> big deal.
OK. I'll change that.
Thanks,
 Felix
>
> Regards,
> Christian.
>
> Am 15.03.2018 um 22:27 schrieb Felix Kuehling:
>> Rebased and integrated review feedback from v3:
>> * Removed vm->vm_context field
>> * Use uninterruptible waiting in initial PD validation to avoid
>> ERESTARTSYS
>> * Return number of successful map/unmap operations in failure cases
>> * Facilitate partial retry after failed map/unmap
>> * Added comments with parameter descriptions to new APIs
>> * Defined AMDKFD_IOC_FREE_MEMORY_OF_GPU write-only
>>
>> This patch series also adds Userptr support in patches 15-20.
>>
>> Felix Kuehling (19):
>> Â Â drm/amdgpu: Move KFD-specific fields into struct amdgpu_vm
>> Â Â drm/amdgpu: Fix initial validation of PD BO for KFD VMs
>> Â Â drm/amdgpu: Add helper to turn an existing VM into a compute VM
>> Â Â drm/amdgpu: Add kfd2kgd interface to acquire an existing VM
>> Â Â drm/amdkfd: Create KFD VMs on demand
>> Â Â drm/amdkfd: Remove limit on number of GPUs
>> Â Â drm/amdkfd: Aperture setup for dGPUs
>> Â Â drm/amdkfd: Add per-process IDR for buffer handles
>> Â Â drm/amdkfd: Allocate CWSR trap handler memory for dGPUs
>> Â Â drm/amdkfd: Add TC flush on VMID deallocation for Hawaii
>> Â Â drm/amdkfd: Add ioctls for GPUVM memory management
>> Â Â drm/amdkfd: Kmap event page for dGPUs
>> Â Â drm/amdkfd: Add module option for testing large-BAR functionality
>> Â Â drm/amdgpu: Add MMU notifier type for KFD userptr
>> Â Â drm/amdgpu: Enable amdgpu_ttm_tt_get_user_pages in worker threads
>> Â Â drm/amdgpu: GFP_NOIO while holding locks taken in MMU notifier
>> Â Â drm/amdkfd: GFP_NOIO while holding locks taken in MMU notifier
>> Â Â drm/amdkfd: Add quiesce_mm and resume_mm to kgd2kfd_calls
>> Â Â drm/amdgpu: Add userptr support for KFD
>>
>> Oak Zeng (1):
>> Â Â drm/amdkfd: Populate DRM render device minor
>>
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h        | 37 +-
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c |  1 +
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c |  1 +
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 818
>> ++++++++++++++++++---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c            |  2 +-
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c            | 96 ++-
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_mn.h            | 11 +-
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c           | 30 +-
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c            | 70 ++
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h            | 10 +
>>  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c          | 532
>> ++++++++++++++
>>  drivers/gpu/drm/amd/amdkfd/kfd_crat.c             |  3 +
>>  drivers/gpu/drm/amd/amdkfd/kfd_device.c           | 40 +-
>>  .../gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 22 +-
>>  drivers/gpu/drm/amd/amdkfd/kfd_events.c           | 31 +-
>>  drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c      | 59 +-
>>  drivers/gpu/drm/amd/amdkfd/kfd_module.c           |  7 +
>>  drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c  |  2 +-
>>  drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c   |  2 +-
>>  drivers/gpu/drm/amd/amdkfd/kfd_packet_manager.c   | 37 +
>>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h             | 41 ++
>>  drivers/gpu/drm/amd/amdkfd/kfd_process.c          | 314 +++++++-
>>  drivers/gpu/drm/amd/amdkfd/kfd_topology.c         |  4 +
>>  drivers/gpu/drm/amd/amdkfd/kfd_topology.h         |  1 +
>>  drivers/gpu/drm/amd/include/kgd_kfd_interface.h   | 10 +
>>  include/uapi/linux/kfd_ioctl.h                    | 122 ++-
>> Â 26 files changed, 2090 insertions(+), 213 deletions(-)
>>
>
