On Thu, May 17, 2018 at 12:09 AM, Felix Kuehling <felix.kuehling at amd.com> wrote:
> Hi Oded,
>
> Thanks for working on this! The Makefile changes look good.
>
> Instead of checking and calling function pointers in amdgpu_amdkfd_...
> functions at runtime, couldn't you just define empty stub functions in
> amdgpu_amdkfd.h if KFD is not enabled? I think that would make the code
> shorter and remove the runtime overhead.
>
> Regards,
> Felix
Yes, I can definitely do that. I agree it would be better.
Oded
>
>
> On 2018-05-16 06:09 AM, Oded Gabbay wrote:
>> In case CONFIG_HSA_AMD is not chosen, there is no need to compile amdkfd
>> files that reside inside amdgpu dirver. In addition, because amdkfd
>> depends on x86_64 architecture and amdgpu is not, compiling amdkfd files
>> under i386 architecture can cause compiler errors and warnings.
>>
>> This patch modifies amdgpu's makefile to build amdkfd files only if
>> CONFIG_HSA_AMD is chosen. The only file to be compiled unconditionally
>> is amdgpu_amdkfd.c
>>
>> Direct calls from amdgpu driver proper to functions in other
>> amdgpu_amdkfd_*.c files were changed to calls to functions inside
>> amdgpu_amdkfd.c. These functions call the original functions using a
>> function pointer to allow compilation without the original functions.
>>
>> Signed-off-by: Oded Gabbay <oded.gabbay at gmail.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/Makefile | 13 +++--
>> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 66 ++++++++++++++++++++++--
>> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 48 ++++++++++++-----
>> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 8 ++-
>> drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 2 +-
>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 2 +-
>> 6 files changed, 112 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
>> index f3002020df6c..1464dff1b151 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
>> @@ -56,8 +56,7 @@ amdgpu-y += amdgpu_device.o amdgpu_kms.o \
>>
>> # add asic specific block
>> amdgpu-$(CONFIG_DRM_AMDGPU_CIK)+= cik.o cik_ih.o kv_smc.o kv_dpm.o \
>> - ci_smc.o ci_dpm.o dce_v8_0.o gfx_v7_0.o cik_sdma.o uvd_v4_2.o vce_v2_0.o \
>> - amdgpu_amdkfd_gfx_v7.o
>> + ci_smc.o ci_dpm.o dce_v8_0.o gfx_v7_0.o cik_sdma.o uvd_v4_2.o vce_v2_0.o
>>
>> amdgpu-$(CONFIG_DRM_AMDGPU_SI)+= si.o gmc_v6_0.o gfx_v6_0.o si_ih.o si_dma.o dce_v6_0.o si_dpm.o si_smc.o
>>
>> @@ -126,13 +125,21 @@ amdgpu-y += \
>> vcn_v1_0.o
>>
>> # add amdkfd interfaces
>> +amdgpu-y += amdgpu_amdkfd.o
>> +
>> +ifneq ($(CONFIG_HSA_AMD),)
>> amdgpu-y += \
>> - amdgpu_amdkfd.o \
>> amdgpu_amdkfd_fence.o \
>> amdgpu_amdkfd_gpuvm.o \
>> amdgpu_amdkfd_gfx_v8.o \
>> amdgpu_amdkfd_gfx_v9.o
>>
>> +ifneq ($(CONFIG_DRM_AMDGPU_CIK),)
>> +amdgpu-y += amdgpu_amdkfd_gfx_v7.o
>> +endif
>> +
>> +endif
>> +
>> # add cgs
>> amdgpu-y += amdgpu_cgs.o
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
>> index cd0e8f192e6a..930d27dd6e27 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
>> @@ -27,7 +27,21 @@
>> #include "amdgpu_gfx.h"
>> #include <linux/module.h>
>>
>> +#if defined(CONFIG_HSA_AMD_MODULE) || defined(CONFIG_HSA_AMD)
>> +static const struct amdgpu_amdkfd_if amdkfd_if = {
>> + .fence_check_mm = amdkfd_fence_check_mm,
>> + .unreserve_system_memory_limit = amdkfd_unreserve_system_memory_limit,
>> + .gpuvm_destroy_cb = amdkfd_gpuvm_destroy_cb,
>> + .to_kfd_fence = to_amdgpu_amdkfd_fence,
>> + .evict_userptr = amdkfd_evict_userptr,
>> + .gfx_7_get_functions = amdgpu_amdkfd_gfx_7_get_functions,
>> + .gfx_8_0_get_functions = amdgpu_amdkfd_gfx_8_0_get_functions,
>> + .gfx_9_0_get_functions = amdgpu_amdkfd_gfx_9_0_get_functions
>> +};
>> +#endif
>> +
>> const struct kgd2kfd_calls *kgd2kfd;
>> +const struct amdgpu_amdkfd_if *amdgpu_amdkfd_if;
>> bool (*kgd2kfd_init_p)(unsigned int, const struct kgd2kfd_calls**);
>>
>> static const unsigned int compute_vmid_bitmap = 0xFF00;
>> @@ -50,15 +64,22 @@ int amdgpu_amdkfd_init(void)
>> kgd2kfd = NULL;
>> }
>>
>> +
>> #elif defined(CONFIG_HSA_AMD)
>> +
>> ret = kgd2kfd_init(KFD_INTERFACE_VERSION, &kgd2kfd);
>> if (ret)
>> kgd2kfd = NULL;
>>
>> #else
>> + amdgpu_amdkfd_if = NULL;
>> ret = -ENOENT;
>> #endif
>> +
>> +#if defined(CONFIG_HSA_AMD_MODULE) || defined(CONFIG_HSA_AMD)
>> + amdgpu_amdkfd_if = &amdkfd_if;
>> amdgpu_amdkfd_gpuvm_init_mem_limits();
>> +#endif
>>
>> return ret;
>> }
>> @@ -75,14 +96,14 @@ void amdgpu_amdkfd_device_probe(struct amdgpu_device *adev)
>> {
>> const struct kfd2kgd_calls *kfd2kgd;
>>
>> - if (!kgd2kfd)
>> + if ((!kgd2kfd) || (!amdgpu_amdkfd_if))
>> return;
>>
>> switch (adev->asic_type) {
>> #ifdef CONFIG_DRM_AMDGPU_CIK
>> case CHIP_KAVERI:
>> case CHIP_HAWAII:
>> - kfd2kgd = amdgpu_amdkfd_gfx_7_get_functions();
>> + kfd2kgd = amdgpu_amdkfd_if->gfx_7_get_functions();
>> break;
>> #endif
>> case CHIP_CARRIZO:
>> @@ -90,11 +111,11 @@ void amdgpu_amdkfd_device_probe(struct amdgpu_device *adev)
>> case CHIP_FIJI:
>> case CHIP_POLARIS10:
>> case CHIP_POLARIS11:
>> - kfd2kgd = amdgpu_amdkfd_gfx_8_0_get_functions();
>> + kfd2kgd = amdgpu_amdkfd_if->gfx_8_0_get_functions();
>> break;
>> case CHIP_VEGA10:
>> case CHIP_RAVEN:
>> - kfd2kgd = amdgpu_amdkfd_gfx_9_0_get_functions();
>> + kfd2kgd = amdgpu_amdkfd_if->gfx_9_0_get_functions();
>> break;
>> default:
>> dev_dbg(adev->dev, "kfd not supported on this ASIC\n");
>> @@ -458,3 +479,40 @@ bool amdgpu_amdkfd_is_kfd_vmid(struct amdgpu_device *adev, u32 vmid)
>>
>> return false;
>> }
>> +
>> +bool amdgpu_amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm)
>> +{
>> + if (!amdgpu_amdkfd_if)
>> + return false;
>> +
>> + return amdgpu_amdkfd_if->fence_check_mm(f, mm);
>> +}
>> +
>> +void amdgpu_amdkfd_unreserve_system_memory_limit(struct amdgpu_bo *bo)
>> +{
>> + if (amdgpu_amdkfd_if)
>> + amdgpu_amdkfd_if->unreserve_system_memory_limit(bo);
>> +}
>> +
>> +void amdgpu_amdkfd_gpuvm_destroy_cb(struct amdgpu_device *adev,
>> + struct amdgpu_vm *vm)
>> +{
>> + if (amdgpu_amdkfd_if)
>> + amdgpu_amdkfd_if->gpuvm_destroy_cb(adev, vm);
>> +}
>> +
>> +struct amdgpu_amdkfd_fence *amdgpu_amdkfd_to_kfd_fence(struct dma_fence *f)
>> +{
>> + if (!amdgpu_amdkfd_if)
>> + return NULL;
>> +
>> + return amdgpu_amdkfd_if->to_kfd_fence(f);
>> +}
>> +
>> +int amdgpu_amdkfd_evict_userptr(struct kgd_mem *mem, struct mm_struct *mm)
>> +{
>> + if (!amdgpu_amdkfd_if)
>> + return 0;
>> +
>> + return amdgpu_amdkfd_if->evict_userptr(mem, mm);
>> +}
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>> index 12367a9951e8..d40480887d49 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>> @@ -79,8 +79,8 @@ struct amdgpu_amdkfd_fence {
>>
>> struct amdgpu_amdkfd_fence *amdgpu_amdkfd_fence_create(u64 context,
>> struct mm_struct *mm);
>> -bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm);
>> -struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct dma_fence *f);
>> +bool amdgpu_amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm);
>> +struct amdgpu_amdkfd_fence *amdgpu_amdkfd_to_kfd_fence(struct dma_fence *f);
>>
>> struct amdkfd_process_info {
>> /* List head of all VMs that belong to a KFD process */
>> @@ -120,10 +120,6 @@ int amdgpu_amdkfd_submit_ib(struct kgd_dev *kgd, enum kgd_engine_type engine,
>> uint32_t vmid, uint64_t gpu_addr,
>> uint32_t *ib_cmd, uint32_t ib_len);
>>
>> -struct kfd2kgd_calls *amdgpu_amdkfd_gfx_7_get_functions(void);
>> -struct kfd2kgd_calls *amdgpu_amdkfd_gfx_8_0_get_functions(void);
>> -struct kfd2kgd_calls *amdgpu_amdkfd_gfx_9_0_get_functions(void);
>> -
>> bool amdgpu_amdkfd_is_kfd_vmid(struct amdgpu_device *adev, u32 vmid);
>>
>> /* Shared API */
>> @@ -156,14 +152,14 @@ uint64_t amdgpu_amdkfd_get_vram_usage(struct kgd_dev *kgd);
>>
>> /* GPUVM API */
>> int amdgpu_amdkfd_gpuvm_create_process_vm(struct kgd_dev *kgd, void **vm,
>> - void **process_info,
>> - struct dma_fence **ef);
>> + void **process_info,
>> + struct dma_fence **ef);
>> int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
>> - struct file *filp,
>> - void **vm, void **process_info,
>> - struct dma_fence **ef);
>> + struct file *filp,
>> + void **vm, void **process_info,
>> + struct dma_fence **ef);
>> void amdgpu_amdkfd_gpuvm_destroy_cb(struct amdgpu_device *adev,
>> - struct amdgpu_vm *vm);
>> + struct amdgpu_vm *vm);
>> void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void *vm);
>> uint32_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm);
>> int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>> @@ -181,9 +177,35 @@ int amdgpu_amdkfd_gpuvm_sync_memory(
>> int amdgpu_amdkfd_gpuvm_map_gtt_bo_to_kernel(struct kgd_dev *kgd,
>> struct kgd_mem *mem, void **kptr, uint64_t *size);
>> int amdgpu_amdkfd_gpuvm_restore_process_bos(void *process_info,
>> - struct dma_fence **ef);
>> + struct dma_fence **ef);
>>
>> void amdgpu_amdkfd_gpuvm_init_mem_limits(void);
>> void amdgpu_amdkfd_unreserve_system_memory_limit(struct amdgpu_bo *bo);
>>
>> +/* Function pointers interface between files inside amdgpu, to allow exclusion
>> + * of amdkfd files from compilation of amdgpu when amdkfd driver is not
>> + * enabled
>> + */
>> +
>> +bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm);
>> +void amdkfd_unreserve_system_memory_limit(struct amdgpu_bo *bo);
>> +void amdkfd_gpuvm_destroy_cb(struct amdgpu_device *adev, struct amdgpu_vm *vm);
>> +struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct dma_fence *f);
>> +int amdkfd_evict_userptr(struct kgd_mem *mem, struct mm_struct *mm);
>> +struct kfd2kgd_calls *amdgpu_amdkfd_gfx_7_get_functions(void);
>> +struct kfd2kgd_calls *amdgpu_amdkfd_gfx_8_0_get_functions(void);
>> +struct kfd2kgd_calls *amdgpu_amdkfd_gfx_9_0_get_functions(void);
>> +
>> +struct amdgpu_amdkfd_if {
>> + bool (*fence_check_mm)(struct dma_fence *f, struct mm_struct *mm);
>> + void (*unreserve_system_memory_limit)(struct amdgpu_bo *bo);
>> + void (*gpuvm_destroy_cb)(struct amdgpu_device *adev,
>> + struct amdgpu_vm *vm);
>> + struct amdgpu_amdkfd_fence* (*to_kfd_fence)(struct dma_fence *f);
>> + int (*evict_userptr)(struct kgd_mem *mem, struct mm_struct *mm);
>> + struct kfd2kgd_calls* (*gfx_7_get_functions)(void);
>> + struct kfd2kgd_calls* (*gfx_8_0_get_functions)(void);
>> + struct kfd2kgd_calls* (*gfx_9_0_get_functions)(void);
>> +};
>> +
>> #endif /* AMDGPU_AMDKFD_H_INCLUDED */
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> index 4a6515ad94f8..c1b9865a240a 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> @@ -166,7 +166,7 @@ static void unreserve_system_mem_limit(struct amdgpu_device *adev,
>> spin_unlock(&kfd_mem_limit.mem_limit_lock);
>> }
>>
>> -void amdgpu_amdkfd_unreserve_system_memory_limit(struct amdgpu_bo *bo)
>> +void amdkfd_unreserve_system_memory_limit(struct amdgpu_bo *bo)
>> {
>> spin_lock(&kfd_mem_limit.mem_limit_lock);
>>
>> @@ -1079,8 +1079,7 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
>> return 0;
>> }
>>
>> -void amdgpu_amdkfd_gpuvm_destroy_cb(struct amdgpu_device *adev,
>> - struct amdgpu_vm *vm)
>> +void amdkfd_gpuvm_destroy_cb(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>> {
>> struct amdkfd_process_info *process_info = vm->process_info;
>> struct amdgpu_bo *pd = vm->root.base.bo;
>> @@ -1625,8 +1624,7 @@ int amdgpu_amdkfd_gpuvm_map_gtt_bo_to_kernel(struct kgd_dev *kgd,
>> * restore, where we get updated page addresses. This function only
>> * ensures that GPU access to the BO is stopped.
>> */
>> -int amdgpu_amdkfd_evict_userptr(struct kgd_mem *mem,
>> - struct mm_struct *mm)
>> +int amdkfd_evict_userptr(struct kgd_mem *mem, struct mm_struct *mm)
>> {
>> struct amdkfd_process_info *process_info = mem->process_info;
>> int invalid, evicted_bos;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>> index 2d6f5ec77a68..82472f080a32 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>> @@ -96,7 +96,7 @@ static void *amdgpu_sync_get_owner(struct dma_fence *f)
>> if (s_fence)
>> return s_fence->owner;
>>
>> - kfd_fence = to_amdgpu_amdkfd_fence(f);
>> + kfd_fence = amdgpu_amdkfd_to_kfd_fence(f);
>> if (kfd_fence)
>> return AMDGPU_FENCE_OWNER_KFD;
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index c713d30cba86..256497940a6b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -1219,7 +1219,7 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
>> for (i = 0; i < flist->shared_count; ++i) {
>> f = rcu_dereference_protected(flist->shared[i],
>> reservation_object_held(bo->resv));
>> - if (amdkfd_fence_check_mm(f, current->mm))
>> + if (amdgpu_amdkfd_fence_check_mm(f, current->mm))
>> return false;
>> }
>> }
>
