On Sat, Sep 10, 2016 at 4:31 AM, Edward O'Callaghan
<funfunc...@folklore1984.net> wrote:
> Signed-off-by: Edward O'Callaghan <funfunc...@folklore1984.net>
Please give a little explanation why this new function is needed.
In addition, I don't think "radeonkfd/amdkfd: .." is good as we never
used this kind of title before.
Let's just use "drm/amdkfd: ..." as this change only relevant for
amdkfd, even if it changes radeon and amdgpu files.
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 32
> ++++++++++++++++++++---
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 3 ++-
> 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/amdkfd/kfd_topology.c | 6 ++++-
> drivers/gpu/drm/amd/include/kgd_kfd_interface.h | 13 +++++++--
> drivers/gpu/drm/radeon/radeon_kfd.c | 22 +++++++++++++---
> 7 files changed, 66 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> index d080d08..63a2e84 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> @@ -226,14 +226,38 @@ void free_gtt_mem(struct kgd_dev *kgd, void *mem_obj)
> kfree(mem);
> }
>
> -uint64_t get_vmem_size(struct kgd_dev *kgd)
> +void get_local_mem_info(struct kgd_dev *kgd,
> + struct kfd_local_mem_info *mem_info)
> {
Maybe check with BUG_ON that mem_info is not NULL ?
> - struct amdgpu_device *rdev =
> - (struct amdgpu_device *)kgd;
> + struct amdgpu_device *rdev = (struct amdgpu_device *)kgd;
I think the amdgpu coding convention is to use adev (rdev is used in
radeon). Maybe another patch to replace all rdev with adev in this
file ?
> + uint64_t address_mask;
> + resource_size_t aper_limit;
>
> BUG_ON(kgd == NULL);
>
> - return rdev->mc.real_vram_size;
> + memset(mem_info, 0, sizeof(*mem_info));
> +
> + address_mask = ~((1UL << 40) - 1);
> + aper_limit = rdev->mc.aper_base + rdev->mc.aper_size;
> +
> + if (!(rdev->mc.aper_base & address_mask ||
> + aper_limit & address_mask)) {
Could you please add a comment explaining this if statement ? I don't
understand exactly what you meant here.
> + mem_info->local_mem_size_public = rdev->mc.visible_vram_size;
> + mem_info->local_mem_size_private = rdev->mc.real_vram_size -
> + rdev->mc.visible_vram_size;
> + } else {
> + mem_info->local_mem_size_public = 0;
> + mem_info->local_mem_size_private = rdev->mc.real_vram_size;
> + }
> + mem_info->vram_width = rdev->mc.vram_width;
> +
> + if (amdgpu_powerplay || rdev->pm.funcs->get_mclk)
I think its better to use "adev->pp_enabled" instead of directly
checking amdgpu_powerplay. e.g. See what's done inside
amdgpu_dpm_get_mclk
> + mem_info->mem_clk_max = amdgpu_dpm_get_mclk(rdev, false) /
> 100;
Why divide it by 100 ? In the radeon_kfd.c version you didn't do it.
> +
> + pr_debug("amdgpu: address base: 0x%llx limit 0x%llx public 0x%llx
> private 0x%llx\n",
> + rdev->mc.aper_base, aper_limit,
> + mem_info->local_mem_size_public,
> + mem_info->local_mem_size_private);
> }
>
> uint64_t get_gpu_clock_counter(struct kgd_dev *kgd)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index de530f68d..31da026 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -57,7 +57,8 @@ int alloc_gtt_mem(struct kgd_dev *kgd, size_t size,
> void **mem_obj, uint64_t *gpu_addr,
> void **cpu_ptr);
> void free_gtt_mem(struct kgd_dev *kgd, void *mem_obj);
> -uint64_t get_vmem_size(struct kgd_dev *kgd);
> +void get_local_mem_info(struct kgd_dev *kgd,
> + struct kfd_local_mem_info *mem_info);
> uint64_t get_gpu_clock_counter(struct kgd_dev *kgd);
>
> uint32_t get_max_engine_clock_in_mhz(struct kgd_dev *kgd);
> 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 362bedc..dc69cf7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
> @@ -131,7 +131,7 @@ static uint16_t get_fw_version(struct kgd_dev *kgd, enum
> kgd_engine_type type);
> static const struct kfd2kgd_calls kfd2kgd = {
> .init_gtt_mem_allocation = alloc_gtt_mem,
> .free_gtt_mem = free_gtt_mem,
> - .get_vmem_size = get_vmem_size,
> + .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,
> .program_sh_mem_settings = kgd_program_sh_mem_settings,
> 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 04b744d..342a037 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> @@ -90,7 +90,7 @@ static uint16_t get_fw_version(struct kgd_dev *kgd, enum
> kgd_engine_type type);
> static const struct kfd2kgd_calls kfd2kgd = {
> .init_gtt_mem_allocation = alloc_gtt_mem,
> .free_gtt_mem = free_gtt_mem,
> - .get_vmem_size = get_vmem_size,
> + .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,
> .program_sh_mem_settings = kgd_program_sh_mem_settings,
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> index 1e50647..6ea7bd8 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> @@ -1088,6 +1088,7 @@ static void kfd_debug_print_topology(void)
>
> static uint32_t kfd_generate_gpu_id(struct kfd_dev *gpu)
> {
> + struct kfd_local_mem_info local_mem_info;
> uint32_t hashout;
> uint32_t buf[7];
> uint64_t local_mem_size;
> @@ -1096,7 +1097,10 @@ static uint32_t kfd_generate_gpu_id(struct kfd_dev
> *gpu)
> if (!gpu)
> return 0;
>
> - local_mem_size = gpu->kfd2kgd->get_vmem_size(gpu->kgd);
> + gpu->kfd2kgd->get_local_mem_info(gpu->kgd, &local_mem_info);
> +
> + local_mem_size = local_mem_info.local_mem_size_private +
> + local_mem_info.local_mem_size_public;
What about the rest of the local_mem_info data ? Don't we should save
it somewhere ?
>
> buf[0] = gpu->pdev->devfn;
> buf[1] = gpu->pdev->subsystem_vendor;
> diff --git a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> index a09d9f3..14ec3e4 100644
> --- a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> +++ b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> @@ -39,6 +39,14 @@ struct kgd_dev;
>
> struct kgd_mem;
>
> +/* For getting GPU local memory information from KGD */
> +struct kfd_local_mem_info {
> + uint64_t local_mem_size_private;
> + uint64_t local_mem_size_public;
> + uint32_t vram_width;
> + uint32_t mem_clk_max;
> +};
> +
> enum kgd_memory_pool {
> KGD_POOL_SYSTEM_CACHEABLE = 1,
> KGD_POOL_SYSTEM_WRITECOMBINE = 2,
> @@ -85,7 +93,7 @@ struct kgd2kfd_shared_resources {
> *
> * @free_gtt_mem: Frees a buffer that was allocated on the gart aperture
> *
> - * @get_vmem_size: Retrieves (physical) size of VRAM
> + * @get_local_mem_info: Retrieves information about GPU local memory
> *
> * @get_gpu_clock_counter: Retrieves GPU clock counter
> *
> @@ -129,7 +137,8 @@ struct kfd2kgd_calls {
>
> void (*free_gtt_mem)(struct kgd_dev *kgd, void *mem_obj);
>
> - uint64_t (*get_vmem_size)(struct kgd_dev *kgd);
> + void(*get_local_mem_info)(struct kgd_dev *kgd,
> + struct kfd_local_mem_info *mem_info);
> uint64_t (*get_gpu_clock_counter)(struct kgd_dev *kgd);
>
> uint32_t (*get_max_engine_clock_in_mhz)(struct kgd_dev *kgd);
> diff --git a/drivers/gpu/drm/radeon/radeon_kfd.c
> b/drivers/gpu/drm/radeon/radeon_kfd.c
> index 87a9ebb..d3249db 100644
> --- a/drivers/gpu/drm/radeon/radeon_kfd.c
> +++ b/drivers/gpu/drm/radeon/radeon_kfd.c
> @@ -54,7 +54,8 @@ static int alloc_gtt_mem(struct kgd_dev *kgd, size_t size,
>
> static void free_gtt_mem(struct kgd_dev *kgd, void *mem_obj);
>
> -static uint64_t get_vmem_size(struct kgd_dev *kgd);
> +static void get_local_mem_info(struct kgd_dev *kgd,
> + struct kfd_local_mem_info *mem_info);
> static uint64_t get_gpu_clock_counter(struct kgd_dev *kgd);
>
> static uint32_t get_max_engine_clock_in_mhz(struct kgd_dev *kgd);
> @@ -107,7 +108,7 @@ static void write_vmid_invalidate_request(struct kgd_dev
> *kgd, uint8_t vmid);
> static const struct kfd2kgd_calls kfd2kgd = {
> .init_gtt_mem_allocation = alloc_gtt_mem,
> .free_gtt_mem = free_gtt_mem,
> - .get_vmem_size = get_vmem_size,
> + .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,
> .program_sh_mem_settings = kgd_program_sh_mem_settings,
> @@ -301,13 +302,26 @@ static void free_gtt_mem(struct kgd_dev *kgd, void
> *mem_obj)
> kfree(mem);
> }
>
> -static uint64_t get_vmem_size(struct kgd_dev *kgd)
> +static void get_local_mem_info(struct kgd_dev *kgd,
> + struct kfd_local_mem_info *mem_info)
> {
> struct radeon_device *rdev = (struct radeon_device *)kgd;
>
> BUG_ON(kgd == NULL);
>
> - return rdev->mc.real_vram_size;
> + memset(mem_info, 0, sizeof(*mem_info));
> +
> + mem_info->local_mem_size_public = rdev->mc.visible_vram_size;
> + mem_info->local_mem_size_private = rdev->mc.real_vram_size -
> + rdev->mc.visible_vram_size;
> +
> + mem_info->vram_width = rdev->mc.vram_width;
> + mem_info->mem_clk_max = radeon_dpm_get_mclk(rdev, false);
> +
> + pr_debug("radeon: address base: 0x%llx public 0x%llx private
> 0x%llx\n",
> + rdev->mc.aper_base,
> + mem_info->local_mem_size_public,
> + mem_info->local_mem_size_private);
> }
>
> static uint64_t get_gpu_clock_counter(struct kgd_dev *kgd)
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
