I have no problem either way. I feel creating a new amdgpu_hwid_mgr is more work and more churn because it would probably come with a bunch of renaming.
Just the PASID manager is very small and I don't expect it to grow. It's much less complex than the VMID manager because PASIDs don't get assigned to processes dynamically. Once a PASID is assigned to a process, it remains the same until that process terminates.
Therefore I'd prefer to leave it in amdgpu_vmid.c. If you feel that file is growing too much in different directions, reorganizing it is out of the scope of this patch series.
Regards,
Felix
________________________________________
From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> on behalf of zhoucm1 <david1.zhou at amd.com>
Sent: Monday, August 28, 2017 3:15:23 AM
To: Christian König; Kuehling, Felix; amd-gfx at lists.freedesktop.org
Subject: Re: [PATCH 2/9] drm/amdgpu: Add PASID management
On 2017å¹´08æ??28æ?¥ 14:45, Christian König wrote:
> I agree that the VM code is growing a bit to much. but the crux is it
> is very close to VMID management, so we should keep the code together.
>
> I wanted to avoid it, but you suggested to separate the VMID
> management quite a while ago. How about moving both VMID as well as
> PASSID into amdgpu_hwid_mgr.c?
If let me choose, I will prefer one is amdgpu_vmid.c, and one is
amdgpu_pasid.c. :)
Anyway, I have no strong opinion on that, depend on your guys like.
Cheers,
David Zhou
>
> Regards,
> Christian.
>
> Am 28.08.2017 um 05:06 schrieb zhoucm1:
>> Could we separate PASID manager to a clean file like amdgpu_pasid.c
>> like what context manager done?
>>
>> Since VM code is growing, and which looks more and more complex.
>>
>> btw: really like many comments about PASID explaination. :)
>>
>> Regards,
>> David Zhou
>> On 2017å¹´08æ??26æ?¥ 15:19, Felix Kuehling wrote:
>>> Allows assigning a PASID to a VM for identifying VMs involved in page
>>> faults. The global PASID manager is also exported in the KFD
>>> interface so that AMDGPU and KFD can share the PASID space.
>>>
>>> PASIDs of different sizes can be requested. On APUs, the PASID size
>>> is deterined by the capabilities of the IOMMU. So KFD must be able
>>> to allocate PASIDs in a smaller range.
>>>
>>> TODO:
>>> * Actually assign PASIDs to VMs
>>> * Update the PASID-VMID mapping registers during CS
>>>
>>> Change-Id: I88c9357a7c584f10e84b5607ac09eba77c833393
>>> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c | 2 +
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c | 2 +
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c | 2 +
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 2 +-
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +-
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 76
>>> ++++++++++++++++++++++-
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 14 ++++-
>>> drivers/gpu/drm/amd/include/kgd_kfd_interface.h | 6 ++
>>> 8 files changed, 101 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
>>> index 3e28d2b..0807d52 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
>>> @@ -188,6 +188,8 @@ static const struct kfd2kgd_calls kfd2kgd = {
>>> .get_local_mem_info = get_local_mem_info,
>>> .get_gpu_clock_counter = get_gpu_clock_counter,
>>> .get_max_engine_clock_in_mhz = get_max_engine_clock_in_mhz,
>>> + .alloc_pasid = amdgpu_vm_alloc_pasid,
>>> + .free_pasid = amdgpu_vm_free_pasid,
>>> .create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
>>> .destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
>>> .get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
>>> index 3b6b4d9..c20c000 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
>>> @@ -162,6 +162,8 @@ static const struct kfd2kgd_calls kfd2kgd = {
>>> .get_local_mem_info = get_local_mem_info,
>>> .get_gpu_clock_counter = get_gpu_clock_counter,
>>> .get_max_engine_clock_in_mhz = get_max_engine_clock_in_mhz,
>>> + .alloc_pasid = amdgpu_vm_alloc_pasid,
>>> + .free_pasid = amdgpu_vm_free_pasid,
>>> .create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
>>> .destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
>>> .create_process_gpumem = create_process_gpumem,
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
>>> index 961369d..bb99c64 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
>>> @@ -209,6 +209,8 @@ static const struct kfd2kgd_calls kfd2kgd = {
>>> .get_local_mem_info = get_local_mem_info,
>>> .get_gpu_clock_counter = get_gpu_clock_counter,
>>> .get_max_engine_clock_in_mhz = get_max_engine_clock_in_mhz,
>>> + .alloc_pasid = amdgpu_vm_alloc_pasid,
>>> + .free_pasid = amdgpu_vm_free_pasid,
>>> .create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
>>> .destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
>>> .create_process_gpumem = create_process_gpumem,
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> index 35f7d77..462011c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> @@ -1397,7 +1397,7 @@ int
>>> amdgpu_amdkfd_gpuvm_create_process_vm(struct kgd_dev *kgd, void **vm,
>>> return -ENOMEM;
>>> /* Initialize the VM context, allocate the page directory
>>> and zero it */
>>> - ret = amdgpu_vm_init(adev, &new_vm->base,
>>> AMDGPU_VM_CONTEXT_COMPUTE);
>>> + ret = amdgpu_vm_init(adev, &new_vm->base,
>>> AMDGPU_VM_CONTEXT_COMPUTE, 0);
>>> if (ret != 0) {
>>> pr_err("Failed init vm ret %d\n", ret);
>>> /* Undo everything related to the new VM context */
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> index e390c01..ba3dc4d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> @@ -825,7 +825,7 @@ int amdgpu_driver_open_kms(struct drm_device
>>> *dev, struct drm_file *file_priv)
>>> }
>>> r = amdgpu_vm_init(adev, &fpriv->vm,
>>> - AMDGPU_VM_CONTEXT_GFX);
>>> + AMDGPU_VM_CONTEXT_GFX, 0);
>>> if (r) {
>>> kfree(fpriv);
>>> goto out_suspend;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 70d7632..c635699 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -27,12 +27,59 @@
>>> */
>>> #include <linux/dma-fence-array.h>
>>> #include <linux/interval_tree_generic.h>
>>> +#include <linux/idr.h>
>>> #include <drm/drmP.h>
>>> #include <drm/amdgpu_drm.h>
>>> #include "amdgpu.h"
>>> #include "amdgpu_trace.h"
>>> /*
>>> + * PASID manager
>>> + *
>>> + * PASIDs are global address space identifiers that can be shared
>>> + * between the GPU, an IOMMU and the driver. VMs on different devices
>>> + * may use the same PASID if they share the same address
>>> + * space. Therefore PASIDs are allocated using a global IDA. VMs are
>>> + * looked up from the PASID per amdgpu_device.
>>> + */
>>> +static DEFINE_IDA(amdgpu_vm_pasid_ida);
>>> +
>>> +/**
>>> + * amdgpu_vm_alloc_pasid - Allocate a PASID
>>> + * @bits: Maximum width of the PASID in bits, must be at least 1
>>> + *
>>> + * Allocates a PASID of the given width while keeping smaller PASIDs
>>> + * available if possible.
>>> + *
>>> + * Returns a positive integer on success. Returns %-EINVAL if bits==0.
>>> + * Returns %-ENOSPC if no PASID was avaliable. Returns %-ENOMEM on
>>> + * memory allocation failure.
>>> + */
>>> +int amdgpu_vm_alloc_pasid(unsigned int bits)
>>> +{
>>> + int pasid = -EINVAL;
>>> +
>>> + for (bits = min(bits, 31U); bits > 0; bits--) {
>>> + pasid = ida_simple_get(&amdgpu_vm_pasid_ida,
>>> + 1U << (bits - 1), 1U << bits,
>>> + GFP_KERNEL);
>>> + if (pasid != -ENOSPC)
>>> + break;
>>> + }
>>> +
>>> + return pasid;
>>> +}
>>> +
>>> +/**
>>> + * amdgpu_vm_free_pasid - Free a PASID
>>> + * @pasid: PASID to free
>>> + */
>>> +void amdgpu_vm_free_pasid(unsigned int pasid)
>>> +{
>>> + ida_simple_remove(&amdgpu_vm_pasid_ida, pasid);
>>> +}
>>> +
>>> +/*
>>> * GPUVM
>>> * GPUVM is similar to the legacy gart on older asics, however
>>> * rather than there being a single global gart table
>>> @@ -2482,7 +2529,7 @@ void amdgpu_vm_adjust_size(struct
>>> amdgpu_device *adev, uint64_t vm_size, uint32_
>>> * Init @vm fields.
>>> */
>>> int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>> - int vm_context)
>>> + int vm_context, unsigned int pasid)
>>> {
>>> const unsigned align = min(AMDGPU_VM_PTB_ALIGN_SIZE,
>>> AMDGPU_VM_PTE_COUNT(adev) * 8);
>>> @@ -2562,6 +2609,19 @@ int amdgpu_vm_init(struct amdgpu_device
>>> *adev, struct amdgpu_vm *vm,
>>> if (r)
>>> goto error_free_root;
>>> + if (pasid) {
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>> + r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, pasid
>>> + 1,
>>> + GFP_ATOMIC);
>>> + spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>> + if (r < 0)
>>> + goto error_free_root;
>>> +
>>> + vm->pasid = pasid;
>>> + }
>>> +
>>> vm->vm_context = vm_context;
>>> if (vm_context == AMDGPU_VM_CONTEXT_COMPUTE) {
>>> mutex_lock(&id_mgr->lock);
>>> @@ -2650,6 +2710,14 @@ void amdgpu_vm_fini(struct amdgpu_device
>>> *adev, struct amdgpu_vm *vm)
>>> mutex_unlock(&id_mgr->lock);
>>> }
>>> + if (vm->pasid) {
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>> + idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>>> + spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>> + }
>>> +
>>> amd_sched_entity_fini(vm->entity.sched, &vm->entity);
>>> if (!RB_EMPTY_ROOT(&vm->va)) {
>>> @@ -2729,6 +2797,9 @@ void amdgpu_vm_manager_init(struct
>>> amdgpu_device *adev)
>>> adev->vm_manager.vm_update_mode = 0;
>>> #endif
>>> + idr_init(&adev->vm_manager.pasid_idr);
>>> + spin_lock_init(&adev->vm_manager.pasid_lock);
>>> +
>>> adev->vm_manager.n_compute_vms = 0;
>>> }
>>> @@ -2743,6 +2814,9 @@ void amdgpu_vm_manager_fini(struct
>>> amdgpu_device *adev)
>>> {
>>> unsigned i, j;
>>> + WARN_ON(!idr_is_empty(&adev->vm_manager.pasid_idr));
>>> + idr_destroy(&adev->vm_manager.pasid_idr);
>>> +
>>> for (i = 0; i < AMDGPU_MAX_VMHUBS; ++i) {
>>> struct amdgpu_vm_id_manager *id_mgr =
>>> &adev->vm_manager.id_mgr[i];
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> index 49e15d7..692b05c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> @@ -25,6 +25,7 @@
>>> #define __AMDGPU_VM_H__
>>> #include <linux/rbtree.h>
>>> +#include <linux/idr.h>
>>> #include "gpu_scheduler.h"
>>> #include "amdgpu_sync.h"
>>> @@ -143,8 +144,9 @@ struct amdgpu_vm {
>>> /* Scheduler entity for page table updates */
>>> struct amd_sched_entity entity;
>>> - /* client id */
>>> + /* client id and PASID (TODO: replace client_id with PASID) */
>>> u64 client_id;
>>> + unsigned int pasid;
>>> /* dedicated to vm */
>>> struct amdgpu_vm_id *reserved_vmid[AMDGPU_MAX_VMHUBS];
>>> @@ -219,14 +221,22 @@ struct amdgpu_vm_manager {
>>> */
>>> int vm_update_mode;
>>> + /* PASID to VM mapping, will be used in interrupt context to
>>> + * look up VM of a page fault
>>> + */
>>> + struct idr pasid_idr;
>>> + spinlock_t pasid_lock;
>>> +
>>> /* Number of Compute VMs, used for detecting Compute activity */
>>> unsigned n_compute_vms;
>>> };
>>> +int amdgpu_vm_alloc_pasid(unsigned int bits);
>>> +void amdgpu_vm_free_pasid(unsigned int pasid);
>>> void amdgpu_vm_manager_init(struct amdgpu_device *adev);
>>> void amdgpu_vm_manager_fini(struct amdgpu_device *adev);
>>> int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>> - int vm_context);
>>> + int vm_context, unsigned int pasid);
>>> void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm
>>> *vm);
>>> void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
>>> struct list_head *validated,
>>> diff --git a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
>>> b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
>>> index e8027b3..5833ef7 100644
>>> --- a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
>>> +++ b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
>>> @@ -188,6 +188,9 @@ struct tile_config {
>>> *
>>> * @get_max_engine_clock_in_mhz: Retrieves maximum GPU clock in MHz
>>> *
>>> + * @alloc_pasid: Allocate a PASID
>>> + * @free_pasid: Free a PASID
>>> + *
>>> * @program_sh_mem_settings: A function that should initiate the
>>> memory
>>> * properties such as main aperture memory type (cache / non
>>> cached) and
>>> * secondary aperture base address, size and memory type.
>>> @@ -264,6 +267,9 @@ struct kfd2kgd_calls {
>>> uint32_t (*get_max_engine_clock_in_mhz)(struct kgd_dev *kgd);
>>> + int (*alloc_pasid)(unsigned int bits);
>>> + void (*free_pasid)(unsigned int pasid);
>>> +
>>> int (*create_process_vm)(struct kgd_dev *kgd, void **vm,
>>> void **process_info);
>>> void (*destroy_process_vm)(struct kgd_dev *kgd, void *vm);
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx at lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
