amdkfd - misc fixes + get_local_mem_info() port [V.2]

amdkfd - misc fixes + get_local_mem_info() port [V.2]

[Edward O'Callaghan]

The following series applies on top of either master or the drm-next-4.9
branch. Apart from the couple of misc fixes here we port out the
'get_local_mem_info()' helper out of the ROCm kernel patch mountain
and hook that up so to be mainlined.

Please review,

Edward O'Callaghan (7):

  [PATCH 1/7] amdkfd: Tidy up kfd_generate_gpu_id() uint64_t bitshift
  [PATCH 2/7] radeonkfd/amdkfd: Implement get_local_mem_info()
  [PATCH 3/7] amdkfd: Add some missing memset zero'ing in queue init
  [PATCH 4/7] drm/amdkfd: Make kfd_lookup_process_by_pasid() well
  [PATCH 5/7] drm/amdkfd: Remove duplicate pqm_uninit()
  [PATCH 6/7] drm/amdkfd: Reuse function to find a process through
  [PATCH 7/7] drm/amdkfd: Fix possible infinite loop
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/7] amdkfd: Tidy up kfd_generate_gpu_id() uint64_t bitshift unpack

[Edward O'Callaghan]

Dereference the one time and unpack the lower and upper 32bit
portions with the proper kernel helper macros.

Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
Reviewed-by: Oded Gabbay <oded.gabbay@gmail.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
index 884c96f..1e50647 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
@@ -1090,19 +1090,21 @@ static uint32_t kfd_generate_gpu_id(struct kfd_dev *gpu)
 {
 	uint32_t hashout;
 	uint32_t buf[7];
+	uint64_t local_mem_size;
 	int i;
 
 	if (!gpu)
 		return 0;
 
+	local_mem_size = gpu->kfd2kgd->get_vmem_size(gpu->kgd);
+
 	buf[0] = gpu->pdev->devfn;
 	buf[1] = gpu->pdev->subsystem_vendor;
 	buf[2] = gpu->pdev->subsystem_device;
 	buf[3] = gpu->pdev->device;
 	buf[4] = gpu->pdev->bus->number;
-	buf[5] = (uint32_t)(gpu->kfd2kgd->get_vmem_size(gpu->kgd)
-			& 0xffffffff);
-	buf[6] = (uint32_t)(gpu->kfd2kgd->get_vmem_size(gpu->kgd) >> 32);
+	buf[5] = lower_32_bits(local_mem_size);
+	buf[6] = upper_32_bits(local_mem_size);
 
 	for (i = 0, hashout = 0; i < 7; i++)
 		hashout ^= hash_32(buf[i], KFD_GPU_ID_HASH_WIDTH);
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/7] radeonkfd/amdkfd: Implement get_local_mem_info()

[Edward O'Callaghan]

Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
---
 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)
 {
-	struct amdgpu_device *rdev =
-		(struct amdgpu_device *)kgd;
+	struct amdgpu_device *rdev = (struct amdgpu_device *)kgd;
+	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)) {
+		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)
+		mem_info->mem_clk_max = amdgpu_dpm_get_mclk(rdev, false) / 100;
+
+	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;
 
 	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

[PATCH 3/7] amdkfd: Add some missing memset zero'ing in queue init func

[Edward O'Callaghan]

Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
Reviewed-by: Oded Gabbay <oded.gabbay@gmail.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
index 9beae87..291c69d 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
@@ -47,6 +47,9 @@ static bool initialize(struct kernel_queue *kq, struct kfd_dev *dev,
 	pr_debug("amdkfd: In func %s initializing queue type %d size %d\n",
 			__func__, KFD_QUEUE_TYPE_HIQ, queue_size);
 
+	memset(&prop, 0, sizeof(prop));
+	memset(&nop, 0, sizeof(nop));
+
 	nop.opcode = IT_NOP;
 	nop.type = PM4_TYPE_3;
 	nop.u32all |= PM4_COUNT_ZERO;
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 4/7] drm/amdkfd: Make kfd_lookup_process_by_pasid() well defined

[Edward O'Callaghan]

Ensure we return a NULL on the fail branch so that the call
site may BUG_ON() it.

Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
---
 drivers/gpu/drm/amd/amdkfd/kfd_process.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index 4f3849a..8d78052 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -481,13 +481,14 @@ bool kfd_has_process_device_data(struct kfd_process *p)
 /* This returns with process->mutex locked. */
 struct kfd_process *kfd_lookup_process_by_pasid(unsigned int pasid)
 {
-	struct kfd_process *p;
+	struct kfd_process *p, *ret_p = NULL;
 	unsigned int temp;
 
 	int idx = srcu_read_lock(&kfd_processes_srcu);
 
 	hash_for_each_rcu(kfd_processes_table, temp, p, kfd_processes) {
 		if (p->pasid == pasid) {
+			ret_p = p;
 			mutex_lock(&p->mutex);
 			break;
 		}
@@ -495,5 +496,5 @@ struct kfd_process *kfd_lookup_process_by_pasid(unsigned int pasid)
 
 	srcu_read_unlock(&kfd_processes_srcu, idx);
 
-	return p;
+	return ret_p;
 }
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 5/7] drm/amdkfd: Remove duplicate pqm_uninit()

[Edward O'Callaghan]

pqm_uninit() will be called in kfd_process_notifier_release(), which
is when the process exits. Calling it in kfd_unbind_process_from_device()
is duplicate and in fact incorrect, because pqm should not be
uninitalized as long as the process still exists.

Original author: Yong Zhao <yong.zhao@amd.com>
Hand ported for mainline.

Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
---
 drivers/gpu/drm/amd/amdkfd/kfd_process.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index 8d78052..ef2266a 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -427,8 +427,6 @@ void kfd_unbind_process_from_device(struct kfd_dev *dev, unsigned int pasid)
 			if ((dev->dbgmgr) && (dev->dbgmgr->pasid == p->pasid))
 				kfd_dbgmgr_destroy(dev->dbgmgr);
 
-			pqm_uninit(&p->pqm);
-
 			pdd = kfd_get_process_device_data(dev, p);
 
 			if (!pdd) {
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 6/7] drm/amdkfd: Reuse function to find a process through pasid

[Edward O'Callaghan]

The kfd_lookup_process_by_pasid() is just for that purpose,
so use it instead of repeating the code.

Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
---
 drivers/gpu/drm/amd/amdkfd/kfd_process.c | 58 +++++++++++++-------------------
 1 file changed, 23 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index ef2266a..a94dddc 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -404,56 +404,44 @@ void kfd_unbind_process_from_device(struct kfd_dev *dev, unsigned int pasid)
 {
 	struct kfd_process *p;
 	struct kfd_process_device *pdd;
-	int idx, i;
 
 	BUG_ON(dev == NULL);
 
-	idx = srcu_read_lock(&kfd_processes_srcu);
-
 	/*
 	 * Look for the process that matches the pasid. If there is no such
 	 * process, we either released it in amdkfd's own notifier, or there
 	 * is a bug. Unfortunately, there is no way to tell...
 	 */
-	hash_for_each_rcu(kfd_processes_table, i, p, kfd_processes)
-		if (p->pasid == pasid) {
+	p = kfd_lookup_process_by_pasid(pasid);
+	BUG_ON(!p);
 
-			srcu_read_unlock(&kfd_processes_srcu, idx);
+	pr_debug("Unbinding process %d from IOMMU\n", pasid);
 
-			pr_debug("Unbinding process %d from IOMMU\n", pasid);
-
-			mutex_lock(&p->mutex);
+	if ((dev->dbgmgr) && (dev->dbgmgr->pasid == p->pasid))
+		kfd_dbgmgr_destroy(dev->dbgmgr);
 
-			if ((dev->dbgmgr) && (dev->dbgmgr->pasid == p->pasid))
-				kfd_dbgmgr_destroy(dev->dbgmgr);
-
-			pdd = kfd_get_process_device_data(dev, p);
-
-			if (!pdd) {
-				mutex_unlock(&p->mutex);
-				return;
-			}
-
-			if (pdd->reset_wavefronts) {
-				dbgdev_wave_reset_wavefronts(pdd->dev, p);
-				pdd->reset_wavefronts = false;
-			}
+	pdd = kfd_get_process_device_data(dev, p);
 
-			/*
-			 * Just mark pdd as unbound, because we still need it
-			 * to call amd_iommu_unbind_pasid() in when the
-			 * process exits.
-			 * We don't call amd_iommu_unbind_pasid() here
-			 * because the IOMMU called us.
-			 */
-			pdd->bound = false;
+	if (!pdd) {
+		mutex_unlock(&p->mutex);
+		return;
+	}
 
-			mutex_unlock(&p->mutex);
+	if (pdd->reset_wavefronts) {
+		dbgdev_wave_reset_wavefronts(pdd->dev, p);
+		pdd->reset_wavefronts = false;
+	}
 
-			return;
-		}
+	/*
+	 * Just mark pdd as unbound, because we still need it
+	 * to call amd_iommu_unbind_pasid() in when the
+	 * process exits.
+	 * We don't call amd_iommu_unbind_pasid() here
+	 * because the IOMMU called us.
+	 */
+	pdd->bound = false;
 
-	srcu_read_unlock(&kfd_processes_srcu, idx);
+	mutex_unlock(&p->mutex);
 }
 
 struct kfd_process_device *kfd_get_first_process_device_data(struct kfd_process *p)
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 7/7] drm/amdkfd: Fix possible infinite loop

[Edward O'Callaghan]

When the loop predicating timeout parameter passed happens to
not be a multiple of 20 the unsigned integer will overflow and
the loop will become unbounded.

Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c | 17 +++++++++--------
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c | 17 +++++++++--------
 2 files changed, 18 insertions(+), 16 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 dc69cf7..b0485b4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
@@ -103,11 +103,11 @@ static bool kgd_hqd_is_occupied(struct kgd_dev *kgd, uint64_t queue_address,
 				uint32_t pipe_id, uint32_t queue_id);
 
 static int kgd_hqd_destroy(struct kgd_dev *kgd, uint32_t reset_type,
-				unsigned int timeout, uint32_t pipe_id,
+				unsigned int utimeout, uint32_t pipe_id,
 				uint32_t queue_id);
 static bool kgd_hqd_sdma_is_occupied(struct kgd_dev *kgd, void *mqd);
 static int kgd_hqd_sdma_destroy(struct kgd_dev *kgd, void *mqd,
-				unsigned int timeout);
+				unsigned int utimeout);
 static int kgd_address_watch_disable(struct kgd_dev *kgd);
 static int kgd_address_watch_execute(struct kgd_dev *kgd,
 					unsigned int watch_point_id,
@@ -437,11 +437,12 @@ static bool kgd_hqd_sdma_is_occupied(struct kgd_dev *kgd, void *mqd)
 }
 
 static int kgd_hqd_destroy(struct kgd_dev *kgd, uint32_t reset_type,
-				unsigned int timeout, uint32_t pipe_id,
+				unsigned int utimeout, uint32_t pipe_id,
 				uint32_t queue_id)
 {
 	struct amdgpu_device *adev = get_amdgpu_device(kgd);
 	uint32_t temp;
+	int timeout = utimeout;
 
 	acquire_queue(kgd, pipe_id, queue_id);
 	WREG32(mmCP_HQD_PQ_DOORBELL_CONTROL, 0);
@@ -452,9 +453,8 @@ static int kgd_hqd_destroy(struct kgd_dev *kgd, uint32_t reset_type,
 		temp = RREG32(mmCP_HQD_ACTIVE);
 		if (temp & CP_HQD_ACTIVE__ACTIVE_MASK)
 			break;
-		if (timeout == 0) {
-			pr_err("kfd: cp queue preemption time out (%dms)\n",
-				temp);
+		if (timeout <= 0) {
+			pr_err("kfd: cp queue preemption time out.\n");
 			release_queue(kgd);
 			return -ETIME;
 		}
@@ -467,12 +467,13 @@ static int kgd_hqd_destroy(struct kgd_dev *kgd, uint32_t reset_type,
 }
 
 static int kgd_hqd_sdma_destroy(struct kgd_dev *kgd, void *mqd,
-				unsigned int timeout)
+				unsigned int utimeout)
 {
 	struct amdgpu_device *adev = get_amdgpu_device(kgd);
 	struct cik_sdma_rlc_registers *m;
 	uint32_t sdma_base_addr;
 	uint32_t temp;
+	int timeout = utimeout;
 
 	m = get_sdma_mqd(mqd);
 	sdma_base_addr = get_sdma_base_addr(m);
@@ -485,7 +486,7 @@ static int kgd_hqd_sdma_destroy(struct kgd_dev *kgd, void *mqd,
 		temp = RREG32(sdma_base_addr + mmSDMA0_RLC0_CONTEXT_STATUS);
 		if (temp & SDMA0_STATUS_REG__RB_CMD_IDLE__SHIFT)
 			break;
-		if (timeout == 0)
+		if (timeout <= 0)
 			return -ETIME;
 		msleep(20);
 		timeout -= 20;
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 342a037..a5c027d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
@@ -62,10 +62,10 @@ static bool kgd_hqd_is_occupied(struct kgd_dev *kgd, uint64_t queue_address,
 		uint32_t pipe_id, uint32_t queue_id);
 static bool kgd_hqd_sdma_is_occupied(struct kgd_dev *kgd, void *mqd);
 static int kgd_hqd_destroy(struct kgd_dev *kgd, uint32_t reset_type,
-				unsigned int timeout, uint32_t pipe_id,
+				unsigned int utimeout, uint32_t pipe_id,
 				uint32_t queue_id);
 static int kgd_hqd_sdma_destroy(struct kgd_dev *kgd, void *mqd,
-				unsigned int timeout);
+				unsigned int utimeout);
 static void write_vmid_invalidate_request(struct kgd_dev *kgd, uint8_t vmid);
 static int kgd_address_watch_disable(struct kgd_dev *kgd);
 static int kgd_address_watch_execute(struct kgd_dev *kgd,
@@ -349,11 +349,12 @@ static bool kgd_hqd_sdma_is_occupied(struct kgd_dev *kgd, void *mqd)
 }
 
 static int kgd_hqd_destroy(struct kgd_dev *kgd, uint32_t reset_type,
-				unsigned int timeout, uint32_t pipe_id,
+				unsigned int utimeout, uint32_t pipe_id,
 				uint32_t queue_id)
 {
 	struct amdgpu_device *adev = get_amdgpu_device(kgd);
 	uint32_t temp;
+	int timeout = utimeout;
 
 	acquire_queue(kgd, pipe_id, queue_id);
 
@@ -363,9 +364,8 @@ static int kgd_hqd_destroy(struct kgd_dev *kgd, uint32_t reset_type,
 		temp = RREG32(mmCP_HQD_ACTIVE);
 		if (temp & CP_HQD_ACTIVE__ACTIVE_MASK)
 			break;
-		if (timeout == 0) {
-			pr_err("kfd: cp queue preemption time out (%dms)\n",
-				temp);
+		if (timeout <= 0) {
+			pr_err("kfd: cp queue preemption time out.\n");
 			release_queue(kgd);
 			return -ETIME;
 		}
@@ -378,12 +378,13 @@ static int kgd_hqd_destroy(struct kgd_dev *kgd, uint32_t reset_type,
 }
 
 static int kgd_hqd_sdma_destroy(struct kgd_dev *kgd, void *mqd,
-				unsigned int timeout)
+				unsigned int utimeout)
 {
 	struct amdgpu_device *adev = get_amdgpu_device(kgd);
 	struct cik_sdma_rlc_registers *m;
 	uint32_t sdma_base_addr;
 	uint32_t temp;
+	int timeout = utimeout;
 
 	m = get_sdma_mqd(mqd);
 	sdma_base_addr = get_sdma_base_addr(m);
@@ -396,7 +397,7 @@ static int kgd_hqd_sdma_destroy(struct kgd_dev *kgd, void *mqd,
 		temp = RREG32(sdma_base_addr + mmSDMA0_RLC0_CONTEXT_STATUS);
 		if (temp & SDMA0_STATUS_REG__RB_CMD_IDLE__SHIFT)
 			break;
-		if (timeout == 0)
+		if (timeout <= 0)
 			return -ETIME;
 		msleep(20);
 		timeout -= 20;
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/7] amdkfd: Tidy up kfd_generate_gpu_id() uint64_t bitshift unpack

[Oded Gabbay]

On Sat, Sep 10, 2016 at 4:31 AM, Edward O'Callaghan
<funfunctor@folklore1984.net> wrote:
> Dereference the one time and unpack the lower and upper 32bit
> portions with the proper kernel helper macros.
>
> Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
> Reviewed-by: Oded Gabbay <oded.gabbay@gmail.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> index 884c96f..1e50647 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> @@ -1090,19 +1090,21 @@ static uint32_t kfd_generate_gpu_id(struct kfd_dev *gpu)
>  {
>         uint32_t hashout;
>         uint32_t buf[7];
> +       uint64_t local_mem_size;
>         int i;
>
>         if (!gpu)
>                 return 0;
>
> +       local_mem_size = gpu->kfd2kgd->get_vmem_size(gpu->kgd);
> +
>         buf[0] = gpu->pdev->devfn;
>         buf[1] = gpu->pdev->subsystem_vendor;
>         buf[2] = gpu->pdev->subsystem_device;
>         buf[3] = gpu->pdev->device;
>         buf[4] = gpu->pdev->bus->number;
> -       buf[5] = (uint32_t)(gpu->kfd2kgd->get_vmem_size(gpu->kgd)
> -                       & 0xffffffff);
> -       buf[6] = (uint32_t)(gpu->kfd2kgd->get_vmem_size(gpu->kgd) >> 32);
> +       buf[5] = lower_32_bits(local_mem_size);
> +       buf[6] = upper_32_bits(local_mem_size);
>
>         for (i = 0, hashout = 0; i < 7; i++)
>                 hashout ^= hash_32(buf[i], KFD_GPU_ID_HASH_WIDTH);
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Please change title of patch to "drm/amdkfd: ...."

Oded
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/7] amdkfd: Add some missing memset zero'ing in queue init func

[Oded Gabbay]

On Sat, Sep 10, 2016 at 4:31 AM, Edward O'Callaghan
<funfunctor@folklore1984.net> wrote:
> Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
> Reviewed-by: Oded Gabbay <oded.gabbay@gmail.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
> index 9beae87..291c69d 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
> @@ -47,6 +47,9 @@ static bool initialize(struct kernel_queue *kq, struct kfd_dev *dev,
>         pr_debug("amdkfd: In func %s initializing queue type %d size %d\n",
>                         __func__, KFD_QUEUE_TYPE_HIQ, queue_size);
>
> +       memset(&prop, 0, sizeof(prop));
> +       memset(&nop, 0, sizeof(nop));
> +
>         nop.opcode = IT_NOP;
>         nop.type = PM4_TYPE_3;
>         nop.u32all |= PM4_COUNT_ZERO;
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Please change title of patch to "drm/amdkfd: ..."

Thanks,
    Oded
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/7] drm/amdkfd: Make kfd_lookup_process_by_pasid() well defined

[Oded Gabbay]

On Sat, Sep 10, 2016 at 4:31 AM, Edward O'Callaghan
<funfunctor@folklore1984.net> wrote:
> Ensure we return a NULL on the fail branch so that the call
> site may BUG_ON() it.
>
> Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_process.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 4f3849a..8d78052 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -481,13 +481,14 @@ bool kfd_has_process_device_data(struct kfd_process *p)
>  /* This returns with process->mutex locked. */
>  struct kfd_process *kfd_lookup_process_by_pasid(unsigned int pasid)
>  {
> -       struct kfd_process *p;
> +       struct kfd_process *p, *ret_p = NULL;
>         unsigned int temp;
>
>         int idx = srcu_read_lock(&kfd_processes_srcu);
>
>         hash_for_each_rcu(kfd_processes_table, temp, p, kfd_processes) {
>                 if (p->pasid == pasid) {
> +                       ret_p = p;
>                         mutex_lock(&p->mutex);
>                         break;
>                 }
> @@ -495,5 +496,5 @@ struct kfd_process *kfd_lookup_process_by_pasid(unsigned int pasid)
>
>         srcu_read_unlock(&kfd_processes_srcu, idx);
>
> -       return p;
> +       return ret_p;
>  }
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

This patch is not needed. p will always be a valid pointer on return or NULL.
Unless we hit the "p->pasid == pasid" condition, in which case p is
valid, p will always be NULL when finishing the hash_for_each_rcu
macro.

        Oded
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/7] drm/amdkfd: Remove duplicate pqm_uninit()

[Oded Gabbay]

On Sat, Sep 10, 2016 at 4:31 AM, Edward O'Callaghan
<funfunctor@folklore1984.net> wrote:
> pqm_uninit() will be called in kfd_process_notifier_release(), which
> is when the process exits. Calling it in kfd_unbind_process_from_device()
> is duplicate and in fact incorrect, because pqm should not be
> uninitalized as long as the process still exists.

Hmm, I don't agree with this patch and the above explanation.

kfd_unbind_process_from_device is called from the IOMMUv2 driver, when
its callback from the mm subsystem is called (mn_release). That
callback is called when the process _is_ shutting down. The IOMMUv2
driver then unmaps all its mappings, and if we won't destroy our
queues before that happens (as pqm_uninit does), and we have a pending
operation on the queue which would get submitted before
kfd_process_notifier_release() is called, then I believe we may get an
IOMMU fault.

    Oded

>
> Original author: Yong Zhao <yong.zhao@amd.com>
> Hand ported for mainline.
>
> Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_process.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 8d78052..ef2266a 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -427,8 +427,6 @@ void kfd_unbind_process_from_device(struct kfd_dev *dev, unsigned int pasid)
>                         if ((dev->dbgmgr) && (dev->dbgmgr->pasid == p->pasid))
>                                 kfd_dbgmgr_destroy(dev->dbgmgr);
>
> -                       pqm_uninit(&p->pqm);
> -
>                         pdd = kfd_get_process_device_data(dev, p);
>
>                         if (!pdd) {
> --
> 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

Re: [PATCH 4/7] drm/amdkfd: Make kfd_lookup_process_by_pasid() well defined

[Edward O'Callaghan]

[-- Attachment #1.1.1: Type: text/plain, Size: 2338 bytes --]



On 09/11/2016 01:55 AM, Oded Gabbay wrote:
> On Sat, Sep 10, 2016 at 4:31 AM, Edward O'Callaghan
> <funfunctor-dczkZgxz+BNUPWh3PAxdjQ@public.gmane.org> wrote:
>> Ensure we return a NULL on the fail branch so that the call
>> site may BUG_ON() it.
>>
>> Signed-off-by: Edward O'Callaghan <funfunctor-dczkZgxz+BNUPWh3PAxdjQ@public.gmane.org>
>> ---
>>  drivers/gpu/drm/amd/amdkfd/kfd_process.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> index 4f3849a..8d78052 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> @@ -481,13 +481,14 @@ bool kfd_has_process_device_data(struct kfd_process *p)
>>  /* This returns with process->mutex locked. */
>>  struct kfd_process *kfd_lookup_process_by_pasid(unsigned int pasid)
>>  {
>> -       struct kfd_process *p;
>> +       struct kfd_process *p, *ret_p = NULL;
>>         unsigned int temp;
>>
>>         int idx = srcu_read_lock(&kfd_processes_srcu);
>>
>>         hash_for_each_rcu(kfd_processes_table, temp, p, kfd_processes) {
>>                 if (p->pasid == pasid) {
>> +                       ret_p = p;
>>                         mutex_lock(&p->mutex);
>>                         break;
>>                 }
>> @@ -495,5 +496,5 @@ struct kfd_process *kfd_lookup_process_by_pasid(unsigned int pasid)
>>
>>         srcu_read_unlock(&kfd_processes_srcu, idx);
>>
>> -       return p;
>> +       return ret_p;
>>  }
>> --
>> 2.7.4
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
> This patch is not needed. p will always be a valid pointer on return or NULL.
> Unless we hit the "p->pasid == pasid" condition, in which case p is
> valid, p will always be NULL when finishing the hash_for_each_rcu
> macro.

Ah you are correct, I didn't notice it was initialized inside the macro.

> 
>         Oded
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 6/7] drm/amdkfd: Reuse function to find a process through pasid

[Oded Gabbay]

On Sat, Sep 10, 2016 at 4:31 AM, Edward O'Callaghan
<funfunctor@folklore1984.net> wrote:
> The kfd_lookup_process_by_pasid() is just for that purpose,
> so use it instead of repeating the code.
>
> Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_process.c | 58 +++++++++++++-------------------
>  1 file changed, 23 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index ef2266a..a94dddc 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -404,56 +404,44 @@ void kfd_unbind_process_from_device(struct kfd_dev *dev, unsigned int pasid)
>  {
>         struct kfd_process *p;
>         struct kfd_process_device *pdd;
> -       int idx, i;
>
>         BUG_ON(dev == NULL);
>
> -       idx = srcu_read_lock(&kfd_processes_srcu);
> -
>         /*
>          * Look for the process that matches the pasid. If there is no such
>          * process, we either released it in amdkfd's own notifier, or there
>          * is a bug. Unfortunately, there is no way to tell...
>          */
> -       hash_for_each_rcu(kfd_processes_table, i, p, kfd_processes)
> -               if (p->pasid == pasid) {
> +       p = kfd_lookup_process_by_pasid(pasid);
> +       BUG_ON(!p);
Don't BUG_ON here. Look at the comment above, there is a valid
scenario where p might be NULL. If p is NULL, just exit the function.

>
> -                       srcu_read_unlock(&kfd_processes_srcu, idx);
> +       pr_debug("Unbinding process %d from IOMMU\n", pasid);
>
> -                       pr_debug("Unbinding process %d from IOMMU\n", pasid);
> -
> -                       mutex_lock(&p->mutex);
> +       if ((dev->dbgmgr) && (dev->dbgmgr->pasid == p->pasid))
> +               kfd_dbgmgr_destroy(dev->dbgmgr);
>
> -                       if ((dev->dbgmgr) && (dev->dbgmgr->pasid == p->pasid))
> -                               kfd_dbgmgr_destroy(dev->dbgmgr);
> -
> -                       pdd = kfd_get_process_device_data(dev, p);
> -
> -                       if (!pdd) {
> -                               mutex_unlock(&p->mutex);
> -                               return;
> -                       }
> -
> -                       if (pdd->reset_wavefronts) {
> -                               dbgdev_wave_reset_wavefronts(pdd->dev, p);
> -                               pdd->reset_wavefronts = false;
> -                       }
> +       pdd = kfd_get_process_device_data(dev, p);
>
> -                       /*
> -                        * Just mark pdd as unbound, because we still need it
> -                        * to call amd_iommu_unbind_pasid() in when the
> -                        * process exits.
> -                        * We don't call amd_iommu_unbind_pasid() here
> -                        * because the IOMMU called us.
> -                        */
> -                       pdd->bound = false;
> +       if (!pdd) {
> +               mutex_unlock(&p->mutex);
> +               return;
> +       }
>
> -                       mutex_unlock(&p->mutex);
> +       if (pdd->reset_wavefronts) {
> +               dbgdev_wave_reset_wavefronts(pdd->dev, p);
> +               pdd->reset_wavefronts = false;
> +       }
>
> -                       return;
> -               }
> +       /*
> +        * Just mark pdd as unbound, because we still need it
> +        * to call amd_iommu_unbind_pasid() in when the
> +        * process exits.
> +        * We don't call amd_iommu_unbind_pasid() here
> +        * because the IOMMU called us.
> +        */
> +       pdd->bound = false;
>
> -       srcu_read_unlock(&kfd_processes_srcu, idx);
> +       mutex_unlock(&p->mutex);
>  }
>
>  struct kfd_process_device *kfd_get_first_process_device_data(struct kfd_process *p)
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Assuming the above comment is fixed, this patch is:
Reviewed-by: Oded Gabbay <oded.gabbay@gmail.com>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 7/7] drm/amdkfd: Fix possible infinite loop

[Oded Gabbay]

On Sat, Sep 10, 2016 at 4:31 AM, Edward O'Callaghan
<funfunctor@folklore1984.net> wrote:
> When the loop predicating timeout parameter passed happens to
> not be a multiple of 20 the unsigned integer will overflow and
> the loop will become unbounded.
>
> Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c | 17 +++++++++--------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c | 17 +++++++++--------
>  2 files changed, 18 insertions(+), 16 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 dc69cf7..b0485b4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
> @@ -103,11 +103,11 @@ static bool kgd_hqd_is_occupied(struct kgd_dev *kgd, uint64_t queue_address,
>                                 uint32_t pipe_id, uint32_t queue_id);
>
>  static int kgd_hqd_destroy(struct kgd_dev *kgd, uint32_t reset_type,
> -                               unsigned int timeout, uint32_t pipe_id,
> +                               unsigned int utimeout, uint32_t pipe_id,
>                                 uint32_t queue_id);
>  static bool kgd_hqd_sdma_is_occupied(struct kgd_dev *kgd, void *mqd);
>  static int kgd_hqd_sdma_destroy(struct kgd_dev *kgd, void *mqd,
> -                               unsigned int timeout);
> +                               unsigned int utimeout);
>  static int kgd_address_watch_disable(struct kgd_dev *kgd);
>  static int kgd_address_watch_execute(struct kgd_dev *kgd,
>                                         unsigned int watch_point_id,
> @@ -437,11 +437,12 @@ static bool kgd_hqd_sdma_is_occupied(struct kgd_dev *kgd, void *mqd)
>  }
>
>  static int kgd_hqd_destroy(struct kgd_dev *kgd, uint32_t reset_type,
> -                               unsigned int timeout, uint32_t pipe_id,
> +                               unsigned int utimeout, uint32_t pipe_id,
>                                 uint32_t queue_id)
>  {
>         struct amdgpu_device *adev = get_amdgpu_device(kgd);
>         uint32_t temp;
> +       int timeout = utimeout;
>
>         acquire_queue(kgd, pipe_id, queue_id);
>         WREG32(mmCP_HQD_PQ_DOORBELL_CONTROL, 0);
> @@ -452,9 +453,8 @@ static int kgd_hqd_destroy(struct kgd_dev *kgd, uint32_t reset_type,
>                 temp = RREG32(mmCP_HQD_ACTIVE);
>                 if (temp & CP_HQD_ACTIVE__ACTIVE_MASK)
>                         break;
> -               if (timeout == 0) {
> -                       pr_err("kfd: cp queue preemption time out (%dms)\n",
> -                               temp);
> +               if (timeout <= 0) {
> +                       pr_err("kfd: cp queue preemption time out.\n");
>                         release_queue(kgd);
>                         return -ETIME;
>                 }
> @@ -467,12 +467,13 @@ static int kgd_hqd_destroy(struct kgd_dev *kgd, uint32_t reset_type,
>  }
>
>  static int kgd_hqd_sdma_destroy(struct kgd_dev *kgd, void *mqd,
> -                               unsigned int timeout)
> +                               unsigned int utimeout)
>  {
>         struct amdgpu_device *adev = get_amdgpu_device(kgd);
>         struct cik_sdma_rlc_registers *m;
>         uint32_t sdma_base_addr;
>         uint32_t temp;
> +       int timeout = utimeout;
>
>         m = get_sdma_mqd(mqd);
>         sdma_base_addr = get_sdma_base_addr(m);
> @@ -485,7 +486,7 @@ static int kgd_hqd_sdma_destroy(struct kgd_dev *kgd, void *mqd,
>                 temp = RREG32(sdma_base_addr + mmSDMA0_RLC0_CONTEXT_STATUS);
>                 if (temp & SDMA0_STATUS_REG__RB_CMD_IDLE__SHIFT)
>                         break;
> -               if (timeout == 0)
> +               if (timeout <= 0)
>                         return -ETIME;
>                 msleep(20);
>                 timeout -= 20;
> 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 342a037..a5c027d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> @@ -62,10 +62,10 @@ static bool kgd_hqd_is_occupied(struct kgd_dev *kgd, uint64_t queue_address,
>                 uint32_t pipe_id, uint32_t queue_id);
>  static bool kgd_hqd_sdma_is_occupied(struct kgd_dev *kgd, void *mqd);
>  static int kgd_hqd_destroy(struct kgd_dev *kgd, uint32_t reset_type,
> -                               unsigned int timeout, uint32_t pipe_id,
> +                               unsigned int utimeout, uint32_t pipe_id,
>                                 uint32_t queue_id);
>  static int kgd_hqd_sdma_destroy(struct kgd_dev *kgd, void *mqd,
> -                               unsigned int timeout);
> +                               unsigned int utimeout);
>  static void write_vmid_invalidate_request(struct kgd_dev *kgd, uint8_t vmid);
>  static int kgd_address_watch_disable(struct kgd_dev *kgd);
>  static int kgd_address_watch_execute(struct kgd_dev *kgd,
> @@ -349,11 +349,12 @@ static bool kgd_hqd_sdma_is_occupied(struct kgd_dev *kgd, void *mqd)
>  }
>
>  static int kgd_hqd_destroy(struct kgd_dev *kgd, uint32_t reset_type,
> -                               unsigned int timeout, uint32_t pipe_id,
> +                               unsigned int utimeout, uint32_t pipe_id,
>                                 uint32_t queue_id)
>  {
>         struct amdgpu_device *adev = get_amdgpu_device(kgd);
>         uint32_t temp;
> +       int timeout = utimeout;
>
>         acquire_queue(kgd, pipe_id, queue_id);
>
> @@ -363,9 +364,8 @@ static int kgd_hqd_destroy(struct kgd_dev *kgd, uint32_t reset_type,
>                 temp = RREG32(mmCP_HQD_ACTIVE);
>                 if (temp & CP_HQD_ACTIVE__ACTIVE_MASK)
>                         break;
> -               if (timeout == 0) {
> -                       pr_err("kfd: cp queue preemption time out (%dms)\n",
> -                               temp);
> +               if (timeout <= 0) {
> +                       pr_err("kfd: cp queue preemption time out.\n");
>                         release_queue(kgd);
>                         return -ETIME;
>                 }
> @@ -378,12 +378,13 @@ static int kgd_hqd_destroy(struct kgd_dev *kgd, uint32_t reset_type,
>  }
>
>  static int kgd_hqd_sdma_destroy(struct kgd_dev *kgd, void *mqd,
> -                               unsigned int timeout)
> +                               unsigned int utimeout)
>  {
>         struct amdgpu_device *adev = get_amdgpu_device(kgd);
>         struct cik_sdma_rlc_registers *m;
>         uint32_t sdma_base_addr;
>         uint32_t temp;
> +       int timeout = utimeout;
>
>         m = get_sdma_mqd(mqd);
>         sdma_base_addr = get_sdma_base_addr(m);
> @@ -396,7 +397,7 @@ static int kgd_hqd_sdma_destroy(struct kgd_dev *kgd, void *mqd,
>                 temp = RREG32(sdma_base_addr + mmSDMA0_RLC0_CONTEXT_STATUS);
>                 if (temp & SDMA0_STATUS_REG__RB_CMD_IDLE__SHIFT)
>                         break;
> -               if (timeout == 0)
> +               if (timeout <= 0)
>                         return -ETIME;
>                 msleep(20);
>                 timeout -= 20;
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Nice.
This patch is:
Reviewed-by: Oded Gabbay <oded.gabbay@gmail.com>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/7] radeonkfd/amdkfd: Implement get_local_mem_info()

[Oded Gabbay]

On Sat, Sep 10, 2016 at 4:31 AM, Edward O'Callaghan
<funfunctor@folklore1984.net> wrote:
> Signed-off-by: Edward O'Callaghan <funfunctor@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

Re: [PATCH 5/7] drm/amdkfd: Remove duplicate pqm_uninit()

[Felix Kuehling]

Yong is currently working on a follow-up fix to handle process shutdown
on APUs as well as S3 suspend correctly.

Regards,
  Felix


On 16-09-10 12:06 PM, Oded Gabbay wrote:
> On Sat, Sep 10, 2016 at 4:31 AM, Edward O'Callaghan
> <funfunctor@folklore1984.net> wrote:
>> pqm_uninit() will be called in kfd_process_notifier_release(), which
>> is when the process exits. Calling it in kfd_unbind_process_from_device()
>> is duplicate and in fact incorrect, because pqm should not be
>> uninitalized as long as the process still exists.
> Hmm, I don't agree with this patch and the above explanation.
>
> kfd_unbind_process_from_device is called from the IOMMUv2 driver, when
> its callback from the mm subsystem is called (mn_release). That
> callback is called when the process _is_ shutting down. The IOMMUv2
> driver then unmaps all its mappings, and if we won't destroy our
> queues before that happens (as pqm_uninit does), and we have a pending
> operation on the queue which would get submitted before
> kfd_process_notifier_release() is called, then I believe we may get an
> IOMMU fault.
>
>     Oded
>
>> Original author: Yong Zhao <yong.zhao@amd.com>
>> Hand ported for mainline.
>>
>> Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
>> ---
>>  drivers/gpu/drm/amd/amdkfd/kfd_process.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> index 8d78052..ef2266a 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> @@ -427,8 +427,6 @@ void kfd_unbind_process_from_device(struct kfd_dev *dev, unsigned int pasid)
>>                         if ((dev->dbgmgr) && (dev->dbgmgr->pasid == p->pasid))
>>                                 kfd_dbgmgr_destroy(dev->dbgmgr);
>>
>> -                       pqm_uninit(&p->pqm);
>> -
>>                         pdd = kfd_get_process_device_data(dev, p);
>>
>>                         if (!pdd) {
>> --
>> 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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx