[PATCH 2/9] drm/amdgpu: Add PASID management

Kuehling, Felix Felix.Kuehling at amd.com
Sat Aug 26 14:41:38 UTC 2017


> But I'm a bit confused, doesn't his stuff belong into the IOMMU code?

PASIDs work on dGPUs without an IOMMU. They're just device-specific process identifiers. On APUs there is an extra step in KFD that registers a process+PASID with the IOMMU driver. That way the IOMMU knows what to do when it sees address translation requests for a specific PASID from a specific device. This is done by calling amd_iommu_bind_pasid:

    struct kfd_process_device *kfd_bind_process_to_device(struct kfd_dev *dev,
                                                     struct kfd_process *p)
    {
            ...
            err = amd_iommu_bind_pasid(dev->pdev, p->pasid, p->lead_thread);
            ...
    }

The PASID management is done by the device driver, not the IOMMU driver. I think different devices can use different PASIDs for the same process.

Regards,
  Felix

________________________________________
From: Christian König <deathsimple at vodafone.de>
Sent: Saturday, August 26, 2017 9:27 AM
To: Kuehling, Felix; amd-gfx at lists.freedesktop.org
Subject: Re: [PATCH 2/9] drm/amdgpu: Add PASID management

Am 26.08.2017 um 09:19 schrieb Felix Kuehling:
> 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>

The patch itself is Reviewed-by: Christian König <christian.koenig at amd.com>.

But I'm a bit confused, doesn't his stuff belong into the IOMMU code?

Regards,
Christian.

> ---
>   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);




More information about the amd-gfx mailing list