[PATCH 5/8] drm/amdkfd: Add function to set queue gws

[PATCH 5/8] drm/amdkfd: Add function to set queue gws

[Zeng, Oak]

Add functions in process queue manager to
set/get queue gws. Also set process's number
of gws used. Currently only one queue in
process can use and use all gws.

Change-Id: I03e480c8692db3eabfc3a188cce8904d5d962ab7
Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h              |  6 +++
 .../gpu/drm/amd/amdkfd/kfd_process_queue_manager.c | 50 ++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 5969e37..40a5c67 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -454,6 +454,9 @@ struct queue_properties {
  *
  * @device: The kfd device that created this queue.
  *
+ * @gws: Pointing to gws kgd_mem if this is a gws control queue; NULL
+ * otherwise.
+ *
  * This structure represents user mode compute queues.
  * It contains all the necessary data to handle such queues.
  *
@@ -475,6 +478,7 @@ struct queue {
 
 	struct kfd_process	*process;
 	struct kfd_dev		*device;
+	void *gws;
 };
 
 /*
@@ -868,6 +872,8 @@ int pqm_update_queue(struct process_queue_manager *pqm, unsigned int qid,
 			struct queue_properties *p);
 int pqm_set_cu_mask(struct process_queue_manager *pqm, unsigned int qid,
 			struct queue_properties *p);
+int pqm_set_gws(struct process_queue_manager *pqm, unsigned int qid,
+			void *gws);
 struct kernel_queue *pqm_get_kernel_queue(struct process_queue_manager *pqm,
 						unsigned int qid);
 int pqm_get_wave_state(struct process_queue_manager *pqm,
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
index e652e25..8e45296 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
@@ -26,6 +26,7 @@
 #include "kfd_device_queue_manager.h"
 #include "kfd_priv.h"
 #include "kfd_kernel_queue.h"
+#include "amdgpu_amdkfd.h"
 
 static inline struct process_queue_node *get_queue_by_qid(
 			struct process_queue_manager *pqm, unsigned int qid)
@@ -74,6 +75,55 @@ void kfd_process_dequeue_from_device(struct kfd_process_device *pdd)
 	pdd->already_dequeued = true;
 }
 
+int pqm_set_gws(struct process_queue_manager *pqm, unsigned int qid,
+			void *gws)
+{
+	struct kfd_dev *dev = NULL;
+	struct process_queue_node *pqn;
+	struct kfd_process_device *pdd;
+
+	pqn = get_queue_by_qid(pqm, qid);
+	if (!pqn) {
+		pr_err("Queue id does not match any known queue\n");
+		return -EINVAL;
+	}
+
+	if (pqn->q)
+		dev = pqn->q->device;
+	if (WARN_ON(!dev))
+		return -ENODEV;
+
+	pdd = kfd_get_process_device_data(dev, pqm->process);
+	if (!pdd) {
+		pr_err("Process device data doesn't exist\n");
+		return -EINVAL;
+	}
+
+	/* Only allow one queue per process can have GWS assigned */
+	list_for_each_entry(pqn, &pqm->queues, process_queue_list) {
+		if (pqn->q && pqn->q->gws)
+			return -EINVAL;
+	}
+
+	pqn->q->gws = gws;
+	pdd->qpd.num_gws = gws ? amdgpu_amdkfd_get_num_gws(dev->kgd) : 0;
+	return 0;
+}
+
+static void *pqm_get_gws(struct process_queue_manager *pqm, unsigned int qid)
+{
+	struct process_queue_node *pqn;
+
+	pqn = get_queue_by_qid(pqm, qid);
+	if (!pqn) {
+		pr_debug("No queue %d exists for get gws operation\n", qid);
+		return NULL;
+	}
+
+	return pqn->q->gws;
+}
+
+
 void kfd_process_dequeue_from_all_devices(struct kfd_process *p)
 {
 	struct kfd_process_device *pdd;
-- 
2.7.4

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

[PATCH 6/8] drm/amdgpu: Add function to add/remove gws to kfd process

[Zeng, Oak]

GWS bo is shared between all kfd processes. Add function to add gws
to kfd process's bo list so gws can be evicted from and restored
for process.

Change-Id: I75d74cfdadb7075ff8b2b68634e205deb73dc1ea
Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h       |   2 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 103 +++++++++++++++++++++--
 2 files changed, 100 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index c00c974..f968bf1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -155,6 +155,8 @@ int amdgpu_amdkfd_alloc_gtt_mem(struct kgd_dev *kgd, size_t size,
 void amdgpu_amdkfd_free_gtt_mem(struct kgd_dev *kgd, void *mem_obj);
 int amdgpu_amdkfd_alloc_gws(struct kgd_dev *kgd, size_t size, void **mem_obj);
 void amdgpu_amdkfd_free_gws(struct kgd_dev *kgd, void *mem_obj);
+int amdgpu_amdkfd_add_gws_to_process(void *info, void *gws, struct kgd_mem **mem);
+int amdgpu_amdkfd_remove_gws_from_process(void *info, void *mem);
 uint32_t amdgpu_amdkfd_get_fw_version(struct kgd_dev *kgd,
 				      enum kgd_engine_type type);
 void amdgpu_amdkfd_get_local_mem_info(struct kgd_dev *kgd,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index e1cae4a..02be141 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -457,6 +457,17 @@ static void add_kgd_mem_to_kfd_bo_list(struct kgd_mem *mem,
 	mutex_unlock(&process_info->lock);
 }
 
+static void remove_kgd_mem_from_kfd_bo_list(struct kgd_mem *mem,
+		struct amdkfd_process_info *process_info)
+{
+	struct ttm_validate_buffer *bo_list_entry;
+
+	bo_list_entry = &mem->validate_list;
+	mutex_lock(&process_info->lock);
+	list_del(&bo_list_entry->head);
+	mutex_unlock(&process_info->lock);
+}
+
 /* Initializes user pages. It registers the MMU notifier and validates
  * the userptr BO in the GTT domain.
  *
@@ -1183,12 +1194,8 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
 
 	if (user_addr) {
 		ret = init_user_pages(*mem, current->mm, user_addr);
-		if (ret) {
-			mutex_lock(&avm->process_info->lock);
-			list_del(&(*mem)->validate_list.head);
-			mutex_unlock(&avm->process_info->lock);
+		if (ret)
 			goto allocate_init_user_pages_failed;
-		}
 	}
 
 	if (offset)
@@ -1197,6 +1204,7 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
 	return 0;
 
 allocate_init_user_pages_failed:
+	remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
 	amdgpu_bo_unref(&bo);
 	/* Don't unreserve system mem limit twice */
 	goto err_reserve_limit;
@@ -2104,3 +2112,88 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef)
 	kfree(pd_bo_list);
 	return ret;
 }
+
+int amdgpu_amdkfd_add_gws_to_process(void *info, void *gws, struct kgd_mem **mem)
+{
+	struct amdkfd_process_info *process_info = (struct amdkfd_process_info *)info;
+	struct amdgpu_bo *gws_bo = (struct amdgpu_bo *)gws;
+	int ret;
+
+	if (!info || !gws)
+		return -EINVAL;
+
+	*mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
+	if (!*mem)
+		return -EINVAL;
+
+	mutex_init(&(*mem)->lock);
+	(*mem)->bo = amdgpu_bo_ref(gws_bo);
+	(*mem)->domain = AMDGPU_GEM_DOMAIN_GWS;
+	(*mem)->process_info = process_info;
+	add_kgd_mem_to_kfd_bo_list(*mem, process_info, false);
+	amdgpu_sync_create(&(*mem)->sync);
+
+
+	/* Validate gws bo the first time it is added to process */
+	mutex_lock(&(*mem)->process_info->lock);
+	ret = amdgpu_bo_reserve(gws_bo, false);
+	if (unlikely(ret)) {
+		pr_err("Reserve gws bo failed %d\n", ret);
+		goto bo_reservation_failure;
+	}
+
+	ret = amdgpu_amdkfd_bo_validate(gws_bo, AMDGPU_GEM_DOMAIN_GWS, false);
+	if (ret) {
+		pr_err("GWS BO validate failed %d\n", ret);
+		goto bo_validation_failure;
+	}
+	/* GWS resource is shared b/t amdgpu and amdkfd
+	 * Add process eviction fence to bo so they can
+	 * evict each other.
+	 */
+	amdgpu_bo_fence(gws_bo, &process_info->eviction_fence->base, true);
+	amdgpu_bo_unreserve(gws_bo);
+	mutex_unlock(&(*mem)->process_info->lock);
+
+	return ret;
+
+bo_validation_failure:
+	amdgpu_bo_unreserve(gws_bo);
+bo_reservation_failure:
+	mutex_unlock(&(*mem)->process_info->lock);
+	amdgpu_sync_free(&(*mem)->sync);
+	remove_kgd_mem_from_kfd_bo_list(*mem, process_info);
+	amdgpu_bo_unref(&gws_bo);
+	mutex_destroy(&(*mem)->lock);
+	kfree(*mem);
+	*mem = NULL;
+	return ret;
+}
+
+int amdgpu_amdkfd_remove_gws_from_process(void *info, void *mem)
+{
+	int ret;
+	struct amdkfd_process_info *process_info = (struct amdkfd_process_info *)info;
+	struct kgd_mem *kgd_mem = (struct kgd_mem *)mem;
+	struct amdgpu_bo *gws_bo = kgd_mem->bo;
+
+	/* Remove BO from process's validate list so restore worker won't touch
+	 * it anymore
+	 */
+	remove_kgd_mem_from_kfd_bo_list(kgd_mem, process_info);
+
+	ret = amdgpu_bo_reserve(gws_bo, false);
+	if (unlikely(ret)) {
+		pr_err("Reserve gws bo failed %d\n", ret);
+		//TODO add BO back to validate_list?
+		return ret;
+	}
+	amdgpu_amdkfd_remove_eviction_fence(gws_bo,
+			process_info->eviction_fence);
+	amdgpu_bo_unreserve(gws_bo);
+	amdgpu_sync_free(&kgd_mem->sync);
+	amdgpu_bo_unref(&gws_bo);
+	mutex_destroy(&kgd_mem->lock);
+	kfree(mem);
+	return 0;
+}
-- 
2.7.4

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

[PATCH 7/8] drm/amdkfd: New IOCTL to allocate queue GWS

[Zeng, Oak]

Add a new kfd ioctl to allocate queue GWS. Queue
GWS is released on queue destroy.

Change-Id: I60153c26a577992ad873e4292e759e5c3d5bbd15
Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c           | 39 ++++++++++++++++++++++
 .../gpu/drm/amd/amdkfd/kfd_process_queue_manager.c |  6 ++++
 include/uapi/linux/kfd_ioctl.h                     | 20 ++++++++++-
 3 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 38ae53f..828bd66 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -1559,6 +1559,43 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file *filep,
 	return err;
 }
 
+static int kfd_ioctl_alloc_queue_gws(struct file *filep,
+		struct kfd_process *p, void *data)
+{
+	int retval;
+	struct kfd_ioctl_alloc_queue_gws_args *args = data;
+	struct kfd_dev *dev = NULL;
+	struct kgd_mem *mem;
+
+	if (args->num_gws == 0)
+		return -EINVAL;
+
+	if (!hws_gws_support ||
+		dev->dqm->sched_policy == KFD_SCHED_POLICY_NO_HWS)
+		return -EINVAL;
+
+	dev = kfd_device_by_id(args->gpu_id);
+	if (!dev) {
+		pr_debug("Could not find gpu id 0x%x\n", args->gpu_id);
+		return -EINVAL;
+	}
+
+	retval = amdgpu_amdkfd_add_gws_to_process(p->kgd_process_info,
+			dev->gws, &mem);
+	if (unlikely(retval))
+		return retval;
+
+	mutex_lock(&p->mutex);
+	retval = pqm_set_gws(&p->pqm, args->queue_id, mem);
+	mutex_unlock(&p->mutex);
+
+	if (unlikely(retval))
+		amdgpu_amdkfd_remove_gws_from_process(p->kgd_process_info, mem);
+
+	args->first_gws = 0;
+	return retval;
+}
+
 static int kfd_ioctl_get_dmabuf_info(struct file *filep,
 		struct kfd_process *p, void *data)
 {
@@ -1761,6 +1798,8 @@ static const struct amdkfd_ioctl_desc amdkfd_ioctls[] = {
 	AMDKFD_IOCTL_DEF(AMDKFD_IOC_IMPORT_DMABUF,
 				kfd_ioctl_import_dmabuf, 0),
 
+	AMDKFD_IOCTL_DEF(AMDKFD_IOC_ALLOC_QUEUE_GWS,
+			kfd_ioctl_alloc_queue_gws, 0),
 };
 
 #define AMDKFD_CORE_IOCTL_COUNT	ARRAY_SIZE(amdkfd_ioctls)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
index 8e45296..e15ad48 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
@@ -363,6 +363,12 @@ int pqm_destroy_queue(struct process_queue_manager *pqm, unsigned int qid)
 		return -1;
 	}
 
+	if (pqm_get_gws(pqm, qid)) {
+		amdgpu_amdkfd_remove_gws_from_process(pqm->process->kgd_process_info,
+				pqm_get_gws(pqm, qid));
+		pqm_set_gws(pqm, qid, NULL);
+	}
+
 	if (pqn->kq) {
 		/* destroy kernel queue (DIQ) */
 		dqm = pqn->kq->dev->dqm;
diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
index 20917c5..912d690 100644
--- a/include/uapi/linux/kfd_ioctl.h
+++ b/include/uapi/linux/kfd_ioctl.h
@@ -410,6 +410,21 @@ struct kfd_ioctl_unmap_memory_from_gpu_args {
 	__u32 n_success;		/* to/from KFD */
 };
 
+/* Allocate GWS for specific queue
+ *
+ * @gpu_id:      device identifier
+ * @queue_id:    queue's id that GWS is allocated for
+ * @num_gws:     how many GWS to allocate
+ * @first_gws:   index of the first GWS allocated.
+ *               only support contiguous GWS allocation
+ */
+struct kfd_ioctl_alloc_queue_gws_args {
+	__u32 gpu_id;		/* to KFD */
+	__u32 queue_id;		/* to KFD */
+	__u32 num_gws;		/* to KFD */
+	__u32 first_gws;	/* from KFD */
+};
+
 struct kfd_ioctl_get_dmabuf_info_args {
 	__u64 size;		/* from KFD */
 	__u64 metadata_ptr;	/* to KFD */
@@ -529,7 +544,10 @@ enum kfd_mmio_remap {
 #define AMDKFD_IOC_IMPORT_DMABUF		\
 		AMDKFD_IOWR(0x1D, struct kfd_ioctl_import_dmabuf_args)
 
+#define AMDKFD_IOC_ALLOC_QUEUE_GWS		\
+		AMDKFD_IOWR(0x22, struct kfd_ioctl_alloc_queue_gws_args)
+
 #define AMDKFD_COMMAND_START		0x01
-#define AMDKFD_COMMAND_END		0x1E
+#define AMDKFD_COMMAND_END		0x23
 
 #endif
-- 
2.7.4

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

[PATCH 1/8] drm/amdkfd: Add gws number to kfd topology node properties

[Zeng, Oak]

Add amdgpu_amdkfd interface to get num_gws and add num_gws
to /sys/class/kfd/kfd/topology/nodes/x/properties. Only report
num_gws if MEC FW support GWS barriers. Currently it is
determined by a environment variable which will be replaced
with MEC FW version check when firmware is ready.

Change-Id: Ie0d00fb20a37ef2856860dbecbe1ad0ca1ef09f7
Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c |  7 +++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    | 10 ++++++++++
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h      |  5 +++++
 drivers/gpu/drm/amd/amdkfd/kfd_topology.c  |  5 +++++
 drivers/gpu/drm/amd/amdkfd/kfd_topology.h  |  1 +
 6 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
index 98326e3b..8151221 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
@@ -544,6 +544,13 @@ uint64_t amdgpu_amdkfd_get_mmio_remap_phys_addr(struct kgd_dev *kgd)
 	return adev->rmmio_remap.bus_addr;
 }
 
+uint32_t amdgpu_amdkfd_get_num_gws(struct kgd_dev *kgd)
+{
+	struct amdgpu_device *adev = (struct amdgpu_device *)kgd;
+
+	return adev->gds.gws.total_size;
+}
+
 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)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index f57f297..5700643 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -169,6 +169,7 @@ int amdgpu_amdkfd_get_dmabuf_info(struct kgd_dev *kgd, int dma_buf_fd,
 uint64_t amdgpu_amdkfd_get_vram_usage(struct kgd_dev *kgd);
 uint64_t amdgpu_amdkfd_get_hive_id(struct kgd_dev *kgd);
 uint64_t amdgpu_amdkfd_get_mmio_remap_phys_addr(struct kgd_dev *kgd);
+uint32_t amdgpu_amdkfd_get_num_gws(struct kgd_dev *kgd);
 uint8_t amdgpu_amdkfd_get_xgmi_hops_count(struct kgd_dev *dst, struct kgd_dev *src);
 
 #define read_user_wptr(mmptr, wptr, dst)				\
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index a334d3b..3a03c2b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -666,6 +666,16 @@ MODULE_PARM_DESC(noretry,
 int halt_if_hws_hang;
 module_param(halt_if_hws_hang, int, 0644);
 MODULE_PARM_DESC(halt_if_hws_hang, "Halt if HWS hang is detected (0 = off (default), 1 = on)");
+
+/**
+ * DOC: hws_gws_support(bool)
+ * Whether HWS support gws barriers. Default value: false (not supported)
+ * This will be replaced with a MEC firmware version check once firmware
+ * is ready
+ */
+bool hws_gws_support;
+module_param(hws_gws_support, bool, 0444);
+MODULE_PARM_DESC(hws_gws_support, "MEC FW support gws barriers (false = not supported (Default), true = supported)");
 #endif
 
 /**
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 8f02d78..338fb07 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -160,6 +160,11 @@ extern int noretry;
  */
 extern int halt_if_hws_hang;
 
+/*
+ * Whether MEC FW support GWS barriers
+ */
+extern bool hws_gws_support;
+
 enum cache_policy {
 	cache_policy_coherent,
 	cache_policy_noncoherent
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
index 2c06d6c..128c72c 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
@@ -454,6 +454,8 @@ static ssize_t node_show(struct kobject *kobj, struct attribute *attr,
 			dev->node_props.lds_size_in_kb);
 	sysfs_show_32bit_prop(buffer, "gds_size_in_kb",
 			dev->node_props.gds_size_in_kb);
+	sysfs_show_32bit_prop(buffer, "num_gws",
+			dev->node_props.num_gws);
 	sysfs_show_32bit_prop(buffer, "wave_front_size",
 			dev->node_props.wave_front_size);
 	sysfs_show_32bit_prop(buffer, "array_count",
@@ -1290,6 +1292,9 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
 	dev->node_props.num_sdma_engines = gpu->device_info->num_sdma_engines;
 	dev->node_props.num_sdma_xgmi_engines =
 				gpu->device_info->num_xgmi_sdma_engines;
+	dev->node_props.num_gws = (hws_gws_support &&
+		dev->gpu->dqm->sched_policy != KFD_SCHED_POLICY_NO_HWS) ?
+		amdgpu_amdkfd_get_num_gws(dev->gpu->kgd) : 0;
 
 	kfd_fill_mem_clk_max_info(dev);
 	kfd_fill_iolink_non_crat_info(dev);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.h b/drivers/gpu/drm/amd/amdkfd/kfd_topology.h
index 949e885..276354a 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.h
@@ -65,6 +65,7 @@ struct kfd_node_properties {
 	uint32_t max_waves_per_simd;
 	uint32_t lds_size_in_kb;
 	uint32_t gds_size_in_kb;
+	uint32_t num_gws;
 	uint32_t wave_front_size;
 	uint32_t array_count;
 	uint32_t simd_arrays_per_engine;
-- 
2.7.4

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

[PATCH 7/8] drm/amdkfd: New IOCTL to allocate queue GWS

[Zeng, Oak]

Add a new kfd ioctl to allocate queue GWS. Queue
GWS is released on queue destroy.

Change-Id: I60153c26a577992ad873e4292e759e5c3d5bbd15
Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 45 ++++++++++++++++++++++++++++++++
 include/uapi/linux/kfd_ioctl.h           | 19 +++++++++++++-
 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 38ae53f..17dd970 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -338,6 +338,11 @@ static int kfd_ioctl_destroy_queue(struct file *filp, struct kfd_process *p,
 
 	mutex_lock(&p->mutex);
 
+	if (pqm_get_gws(&p->pqm, args->queue_id)) {
+		amdgpu_amdkfd_remove_gws_from_process(p->kgd_process_info,
+				pqm_get_gws(&p->pqm, args->queue_id));
+		pqm_set_gws(&p->pqm, args->queue_id, NULL);
+	}
 	retval = pqm_destroy_queue(&p->pqm, args->queue_id);
 
 	mutex_unlock(&p->mutex);
@@ -1559,6 +1564,44 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file *filep,
 	return err;
 }
 
+static int kfd_ioctl_alloc_queue_gws(struct file *filep,
+		struct kfd_process *p, void *data)
+{
+	int retval;
+	struct kfd_ioctl_alloc_queue_gws_args *args = data;
+	struct kfd_dev *dev = NULL;
+	struct kgd_mem *mem;
+
+	if (args->num_gws == 0)
+		return -EINVAL;
+
+	if (!hws_gws_support ||
+		dev->dqm->sched_policy == KFD_SCHED_POLICY_NO_HWS)
+		return -EINVAL;
+
+	dev = kfd_device_by_id(args->gpu_id);
+	if (!dev) {
+		pr_debug("Could not find gpu id 0x%x\n", args->gpu_id);
+		return -EINVAL;
+	}
+
+	retval = amdgpu_amdkfd_add_gws_to_process(p->kgd_process_info,
+			dev->gws, &mem);
+	if (unlikely(retval))
+		return retval;
+
+	mutex_lock(&p->mutex);
+	retval = pqm_set_gws(&p->pqm, args->queue_id, mem);
+	mutex_unlock(&p->mutex);
+
+	if (unlikely(retval))
+		amdgpu_amdkfd_remove_gws_from_process(p->kgd_process_info, mem);
+
+	/* The gws_array parameter is reserved for future extension*/
+	args->gws_array[0] = 0;
+	return retval;
+}
+
 static int kfd_ioctl_get_dmabuf_info(struct file *filep,
 		struct kfd_process *p, void *data)
 {
@@ -1761,6 +1804,8 @@ static const struct amdkfd_ioctl_desc amdkfd_ioctls[] = {
 	AMDKFD_IOCTL_DEF(AMDKFD_IOC_IMPORT_DMABUF,
 				kfd_ioctl_import_dmabuf, 0),
 
+	AMDKFD_IOCTL_DEF(AMDKFD_IOC_ALLOC_QUEUE_GWS,
+			kfd_ioctl_alloc_queue_gws, 0),
 };
 
 #define AMDKFD_CORE_IOCTL_COUNT	ARRAY_SIZE(amdkfd_ioctls)
diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
index 20917c5..1964ab2 100644
--- a/include/uapi/linux/kfd_ioctl.h
+++ b/include/uapi/linux/kfd_ioctl.h
@@ -410,6 +410,20 @@ struct kfd_ioctl_unmap_memory_from_gpu_args {
 	__u32 n_success;		/* to/from KFD */
 };
 
+/* Allocate GWS for specific queue
+ *
+ * @gpu_id:      device identifier
+ * @queue_id:    queue's id that GWS is allocated for
+ * @num_gws:     how many GWS to allocate
+ * @gws_array:   used to return the allocated gws
+ */
+struct kfd_ioctl_alloc_queue_gws_args {
+	__u32 gpu_id;		/* to KFD */
+	__u32 queue_id;		/* to KFD */
+	__u32 num_gws;		/* to KFD */
+	__u32 *gws_array;	/* from KFD */
+};
+
 struct kfd_ioctl_get_dmabuf_info_args {
 	__u64 size;		/* from KFD */
 	__u64 metadata_ptr;	/* to KFD */
@@ -529,7 +543,10 @@ enum kfd_mmio_remap {
 #define AMDKFD_IOC_IMPORT_DMABUF		\
 		AMDKFD_IOWR(0x1D, struct kfd_ioctl_import_dmabuf_args)
 
+#define AMDKFD_IOC_ALLOC_QUEUE_GWS		\
+		AMDKFD_IOWR(0x1E, struct kfd_ioctl_alloc_queue_gws_args)
+
 #define AMDKFD_COMMAND_START		0x01
-#define AMDKFD_COMMAND_END		0x1E
+#define AMDKFD_COMMAND_END		0x1F
 
 #endif
-- 
2.7.4

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

Re: [PATCH 7/8] drm/amdkfd: New IOCTL to allocate queue GWS

[Kuehling, Felix]

On 2019-05-10 12:01 p.m., Zeng, Oak wrote:
> Add a new kfd ioctl to allocate queue GWS. Queue
> GWS is released on queue destroy.
>
> Change-Id: I60153c26a577992ad873e4292e759e5c3d5bbd15
> Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 45 ++++++++++++++++++++++++++++++++
>   include/uapi/linux/kfd_ioctl.h           | 19 +++++++++++++-
>   2 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 38ae53f..17dd970 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -338,6 +338,11 @@ static int kfd_ioctl_destroy_queue(struct file *filp, struct kfd_process *p,
>   
>   	mutex_lock(&p->mutex);
>   
> +	if (pqm_get_gws(&p->pqm, args->queue_id)) {
> +		amdgpu_amdkfd_remove_gws_from_process(p->kgd_process_info,
> +				pqm_get_gws(&p->pqm, args->queue_id));
> +		pqm_set_gws(&p->pqm, args->queue_id, NULL);
> +	}

It would be more robust if this was done inside pqm_destroy_queue. That 
way you'd handle other potential code paths that destroy queues (not 
sure there are any) and you wouldn't need pqm_get_gws exported from PQM.


>   	retval = pqm_destroy_queue(&p->pqm, args->queue_id);
>   
>   	mutex_unlock(&p->mutex);
> @@ -1559,6 +1564,44 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file *filep,
>   	return err;
>   }
>   
> +static int kfd_ioctl_alloc_queue_gws(struct file *filep,
> +		struct kfd_process *p, void *data)
> +{
> +	int retval;
> +	struct kfd_ioctl_alloc_queue_gws_args *args = data;
> +	struct kfd_dev *dev = NULL;
> +	struct kgd_mem *mem;
> +
> +	if (args->num_gws == 0)
> +		return -EINVAL;
> +
> +	if (!hws_gws_support ||
> +		dev->dqm->sched_policy == KFD_SCHED_POLICY_NO_HWS)
> +		return -EINVAL;
> +
> +	dev = kfd_device_by_id(args->gpu_id);
> +	if (!dev) {
> +		pr_debug("Could not find gpu id 0x%x\n", args->gpu_id);
> +		return -EINVAL;
> +	}
> +
> +	retval = amdgpu_amdkfd_add_gws_to_process(p->kgd_process_info,
> +			dev->gws, &mem);
> +	if (unlikely(retval))
> +		return retval;
> +
> +	mutex_lock(&p->mutex);
> +	retval = pqm_set_gws(&p->pqm, args->queue_id, mem);
> +	mutex_unlock(&p->mutex);
> +
> +	if (unlikely(retval))
> +		amdgpu_amdkfd_remove_gws_from_process(p->kgd_process_info, mem);
> +
> +	/* The gws_array parameter is reserved for future extension*/
> +	args->gws_array[0] = 0;
> +	return retval;
> +}
> +
>   static int kfd_ioctl_get_dmabuf_info(struct file *filep,
>   		struct kfd_process *p, void *data)
>   {
> @@ -1761,6 +1804,8 @@ static const struct amdkfd_ioctl_desc amdkfd_ioctls[] = {
>   	AMDKFD_IOCTL_DEF(AMDKFD_IOC_IMPORT_DMABUF,
>   				kfd_ioctl_import_dmabuf, 0),
>   
> +	AMDKFD_IOCTL_DEF(AMDKFD_IOC_ALLOC_QUEUE_GWS,
> +			kfd_ioctl_alloc_queue_gws, 0),
>   };
>   
>   #define AMDKFD_CORE_IOCTL_COUNT	ARRAY_SIZE(amdkfd_ioctls)
> diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
> index 20917c5..1964ab2 100644
> --- a/include/uapi/linux/kfd_ioctl.h
> +++ b/include/uapi/linux/kfd_ioctl.h
> @@ -410,6 +410,20 @@ struct kfd_ioctl_unmap_memory_from_gpu_args {
>   	__u32 n_success;		/* to/from KFD */
>   };
>   
> +/* Allocate GWS for specific queue
> + *
> + * @gpu_id:      device identifier
> + * @queue_id:    queue's id that GWS is allocated for
> + * @num_gws:     how many GWS to allocate
> + * @gws_array:   used to return the allocated gws
> + */
> +struct kfd_ioctl_alloc_queue_gws_args {
> +	__u32 gpu_id;		/* to KFD */
> +	__u32 queue_id;		/* to KFD */
> +	__u32 num_gws;		/* to KFD */
> +	__u32 *gws_array;	/* from KFD */

Don't use pointers in ioctl structures. Use uint64_t. Accessing user 
mode pointers requires copy_to/from_user or similar.

Also prefer to move 64-bit elements to the first element to ensure 
proper alignment, and pad the structure to 64-bit for ABI compatibility.

I'm not sure what your plan is for that gws_array. If it's a pointer to 
a user mode array, then that array needs be allocated by user mode. And 
user mode should probably pass down the size of the array it allocated 
in another parameter.

That said, I think what we want is not an array, but just the index of 
the first GWS entry that was allocated for the queue, which is currently 
always 0. So I'm not sure why you're calling this an "array".


> +};
> +
>   struct kfd_ioctl_get_dmabuf_info_args {
>   	__u64 size;		/* from KFD */
>   	__u64 metadata_ptr;	/* to KFD */
> @@ -529,7 +543,10 @@ enum kfd_mmio_remap {
>   #define AMDKFD_IOC_IMPORT_DMABUF		\
>   		AMDKFD_IOWR(0x1D, struct kfd_ioctl_import_dmabuf_args)
>   
> +#define AMDKFD_IOC_ALLOC_QUEUE_GWS		\
> +		AMDKFD_IOWR(0x1E, struct kfd_ioctl_alloc_queue_gws_args)
> +

This will force us to move some ioctl numbers in amd-kfd-staging and the 
DKMS branch, which will break the ABI of our ROCm releases. Not sure 
there is a good way to avoid that other than leaving a whole in the 
upstream ioctl space. I got push-back on that kind of thing when I 
originally upstreamed KFD. So this is just an FYI.

Regards,
   Felix

>   #define AMDKFD_COMMAND_START		0x01
> -#define AMDKFD_COMMAND_END		0x1E
> +#define AMDKFD_COMMAND_END		0x1F
>   
>   #endif
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 7/8] drm/amdkfd: New IOCTL to allocate queue GWS

[Zeng, Oak]

Hi Felix,

See comments inline [Oak]

Hi Kent, there is one FYI embedded, so be careful when you merge this change back to kfd-staging branch.

Regards,
Oak

-----Original Message-----
From: Kuehling, Felix <Felix.Kuehling@amd.com> 
Sent: Friday, May 10, 2019 4:28 PM
To: Zeng, Oak <Oak.Zeng@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Keely, Sean <Sean.Keely@amd.com>
Subject: Re: [PATCH 7/8] drm/amdkfd: New IOCTL to allocate queue GWS

On 2019-05-10 12:01 p.m., Zeng, Oak wrote:
> Add a new kfd ioctl to allocate queue GWS. Queue GWS is released on 
> queue destroy.
>
> Change-Id: I60153c26a577992ad873e4292e759e5c3d5bbd15
> Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 45 ++++++++++++++++++++++++++++++++
>   include/uapi/linux/kfd_ioctl.h           | 19 +++++++++++++-
>   2 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 38ae53f..17dd970 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -338,6 +338,11 @@ static int kfd_ioctl_destroy_queue(struct file 
> *filp, struct kfd_process *p,
>   
>   	mutex_lock(&p->mutex);
>   
> +	if (pqm_get_gws(&p->pqm, args->queue_id)) {
> +		amdgpu_amdkfd_remove_gws_from_process(p->kgd_process_info,
> +				pqm_get_gws(&p->pqm, args->queue_id));
> +		pqm_set_gws(&p->pqm, args->queue_id, NULL);
> +	}

It would be more robust if this was done inside pqm_destroy_queue. That way you'd handle other potential code paths that destroy queues (not sure there are any) and you wouldn't need pqm_get_gws exported from PQM.

[Oak] Good idea. Will do it. Even though currently there is no other code path destroying a queue using gws, your way is safer for future code change. 


>   	retval = pqm_destroy_queue(&p->pqm, args->queue_id);
>   
>   	mutex_unlock(&p->mutex);
> @@ -1559,6 +1564,44 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file *filep,
>   	return err;
>   }
>   
> +static int kfd_ioctl_alloc_queue_gws(struct file *filep,
> +		struct kfd_process *p, void *data)
> +{
> +	int retval;
> +	struct kfd_ioctl_alloc_queue_gws_args *args = data;
> +	struct kfd_dev *dev = NULL;
> +	struct kgd_mem *mem;
> +
> +	if (args->num_gws == 0)
> +		return -EINVAL;
> +
> +	if (!hws_gws_support ||
> +		dev->dqm->sched_policy == KFD_SCHED_POLICY_NO_HWS)
> +		return -EINVAL;
> +
> +	dev = kfd_device_by_id(args->gpu_id);
> +	if (!dev) {
> +		pr_debug("Could not find gpu id 0x%x\n", args->gpu_id);
> +		return -EINVAL;
> +	}
> +
> +	retval = amdgpu_amdkfd_add_gws_to_process(p->kgd_process_info,
> +			dev->gws, &mem);
> +	if (unlikely(retval))
> +		return retval;
> +
> +	mutex_lock(&p->mutex);
> +	retval = pqm_set_gws(&p->pqm, args->queue_id, mem);
> +	mutex_unlock(&p->mutex);
> +
> +	if (unlikely(retval))
> +		amdgpu_amdkfd_remove_gws_from_process(p->kgd_process_info, mem);
> +
> +	/* The gws_array parameter is reserved for future extension*/
> +	args->gws_array[0] = 0;
> +	return retval;
> +}
> +
>   static int kfd_ioctl_get_dmabuf_info(struct file *filep,
>   		struct kfd_process *p, void *data)
>   {
> @@ -1761,6 +1804,8 @@ static const struct amdkfd_ioctl_desc amdkfd_ioctls[] = {
>   	AMDKFD_IOCTL_DEF(AMDKFD_IOC_IMPORT_DMABUF,
>   				kfd_ioctl_import_dmabuf, 0),
>   
> +	AMDKFD_IOCTL_DEF(AMDKFD_IOC_ALLOC_QUEUE_GWS,
> +			kfd_ioctl_alloc_queue_gws, 0),
>   };
>   
>   #define AMDKFD_CORE_IOCTL_COUNT	ARRAY_SIZE(amdkfd_ioctls)
> diff --git a/include/uapi/linux/kfd_ioctl.h 
> b/include/uapi/linux/kfd_ioctl.h index 20917c5..1964ab2 100644
> --- a/include/uapi/linux/kfd_ioctl.h
> +++ b/include/uapi/linux/kfd_ioctl.h
> @@ -410,6 +410,20 @@ struct kfd_ioctl_unmap_memory_from_gpu_args {
>   	__u32 n_success;		/* to/from KFD */
>   };
>   
> +/* Allocate GWS for specific queue
> + *
> + * @gpu_id:      device identifier
> + * @queue_id:    queue's id that GWS is allocated for
> + * @num_gws:     how many GWS to allocate
> + * @gws_array:   used to return the allocated gws
> + */
> +struct kfd_ioctl_alloc_queue_gws_args {
> +	__u32 gpu_id;		/* to KFD */
> +	__u32 queue_id;		/* to KFD */
> +	__u32 num_gws;		/* to KFD */
> +	__u32 *gws_array;	/* from KFD */

Don't use pointers in ioctl structures. Use uint64_t. Accessing user mode pointers requires copy_to/from_user or similar.

Also prefer to move 64-bit elements to the first element to ensure proper alignment, and pad the structure to 64-bit for ABI compatibility.

I'm not sure what your plan is for that gws_array. If it's a pointer to a user mode array, then that array needs be allocated by user mode. And user mode should probably pass down the size of the array it allocated in another parameter.

That said, I think what we want is not an array, but just the index of the first GWS entry that was allocated for the queue, which is currently always 0. So I'm not sure why you're calling this an "array".

[Oak] For the current design, one queue always get all 64 GWS, so returning the index of the first GWS (0) is not a problem. In the future, is it possible queue can only allocate a few none-contiguous GWS, for example GWS3 and GWS56? If this is the case, we will have to copy an array of gws back to user space.

> +};
> +
>   struct kfd_ioctl_get_dmabuf_info_args {
>   	__u64 size;		/* from KFD */
>   	__u64 metadata_ptr;	/* to KFD */
> @@ -529,7 +543,10 @@ enum kfd_mmio_remap {
>   #define AMDKFD_IOC_IMPORT_DMABUF		\
>   		AMDKFD_IOWR(0x1D, struct kfd_ioctl_import_dmabuf_args)
>   
> +#define AMDKFD_IOC_ALLOC_QUEUE_GWS		\
> +		AMDKFD_IOWR(0x1E, struct kfd_ioctl_alloc_queue_gws_args)
> +

This will force us to move some ioctl numbers in amd-kfd-staging and the DKMS branch, which will break the ABI of our ROCm releases. Not sure there is a good way to avoid that other than leaving a whole in the upstream ioctl space. I got push-back on that kind of thing when I originally upstreamed KFD. So this is just an FYI.
[Oak] Yes, when we merge this changes back to amd-kfd-staging branch, we have to change the ioctl number for GWS allocation to 0x22, to accommodate extra kfd ioctls on kfd-staging branch. @Russell, Kent FYI

Regards,
   Felix

>   #define AMDKFD_COMMAND_START		0x01
> -#define AMDKFD_COMMAND_END		0x1E
> +#define AMDKFD_COMMAND_END		0x1F
>   
>   #endif
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 7/8] drm/amdkfd: New IOCTL to allocate queue GWS

[Kuehling, Felix]

On 2019-05-13 12:03 p.m., Zeng, Oak wrote:
> Hi Felix,
>
> See comments inline [Oak]
>
> Hi Kent, there is one FYI embedded, so be careful when you merge this change back to kfd-staging branch.
>
> Regards,
> Oak
>
> -----Original Message-----
> From: Kuehling, Felix <Felix.Kuehling@amd.com>
> Sent: Friday, May 10, 2019 4:28 PM
> To: Zeng, Oak <Oak.Zeng@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Keely, Sean <Sean.Keely@amd.com>
> Subject: Re: [PATCH 7/8] drm/amdkfd: New IOCTL to allocate queue GWS
>
> On 2019-05-10 12:01 p.m., Zeng, Oak wrote:
>> Add a new kfd ioctl to allocate queue GWS. Queue GWS is released on
>> queue destroy.
>>
>> Change-Id: I60153c26a577992ad873e4292e759e5c3d5bbd15
>> Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 45 ++++++++++++++++++++++++++++++++
>>    include/uapi/linux/kfd_ioctl.h           | 19 +++++++++++++-
>>    2 files changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> index 38ae53f..17dd970 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> @@ -338,6 +338,11 @@ static int kfd_ioctl_destroy_queue(struct file
>> *filp, struct kfd_process *p,
>>    
>>    	mutex_lock(&p->mutex);
>>    
>> +	if (pqm_get_gws(&p->pqm, args->queue_id)) {
>> +		amdgpu_amdkfd_remove_gws_from_process(p->kgd_process_info,
>> +				pqm_get_gws(&p->pqm, args->queue_id));
>> +		pqm_set_gws(&p->pqm, args->queue_id, NULL);
>> +	}
> It would be more robust if this was done inside pqm_destroy_queue. That way you'd handle other potential code paths that destroy queues (not sure there are any) and you wouldn't need pqm_get_gws exported from PQM.
>
> [Oak] Good idea. Will do it. Even though currently there is no other code path destroying a queue using gws, your way is safer for future code change.
>
>
>>    	retval = pqm_destroy_queue(&p->pqm, args->queue_id);
>>    
>>    	mutex_unlock(&p->mutex);
>> @@ -1559,6 +1564,44 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file *filep,
>>    	return err;
>>    }
>>    
>> +static int kfd_ioctl_alloc_queue_gws(struct file *filep,
>> +		struct kfd_process *p, void *data)
>> +{
>> +	int retval;
>> +	struct kfd_ioctl_alloc_queue_gws_args *args = data;
>> +	struct kfd_dev *dev = NULL;
>> +	struct kgd_mem *mem;
>> +
>> +	if (args->num_gws == 0)
>> +		return -EINVAL;
>> +
>> +	if (!hws_gws_support ||
>> +		dev->dqm->sched_policy == KFD_SCHED_POLICY_NO_HWS)
>> +		return -EINVAL;
>> +
>> +	dev = kfd_device_by_id(args->gpu_id);
>> +	if (!dev) {
>> +		pr_debug("Could not find gpu id 0x%x\n", args->gpu_id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	retval = amdgpu_amdkfd_add_gws_to_process(p->kgd_process_info,
>> +			dev->gws, &mem);
>> +	if (unlikely(retval))
>> +		return retval;
>> +
>> +	mutex_lock(&p->mutex);
>> +	retval = pqm_set_gws(&p->pqm, args->queue_id, mem);
>> +	mutex_unlock(&p->mutex);
>> +
>> +	if (unlikely(retval))
>> +		amdgpu_amdkfd_remove_gws_from_process(p->kgd_process_info, mem);
>> +
>> +	/* The gws_array parameter is reserved for future extension*/
>> +	args->gws_array[0] = 0;
>> +	return retval;
>> +}
>> +
>>    static int kfd_ioctl_get_dmabuf_info(struct file *filep,
>>    		struct kfd_process *p, void *data)
>>    {
>> @@ -1761,6 +1804,8 @@ static const struct amdkfd_ioctl_desc amdkfd_ioctls[] = {
>>    	AMDKFD_IOCTL_DEF(AMDKFD_IOC_IMPORT_DMABUF,
>>    				kfd_ioctl_import_dmabuf, 0),
>>    
>> +	AMDKFD_IOCTL_DEF(AMDKFD_IOC_ALLOC_QUEUE_GWS,
>> +			kfd_ioctl_alloc_queue_gws, 0),
>>    };
>>    
>>    #define AMDKFD_CORE_IOCTL_COUNT	ARRAY_SIZE(amdkfd_ioctls)
>> diff --git a/include/uapi/linux/kfd_ioctl.h
>> b/include/uapi/linux/kfd_ioctl.h index 20917c5..1964ab2 100644
>> --- a/include/uapi/linux/kfd_ioctl.h
>> +++ b/include/uapi/linux/kfd_ioctl.h
>> @@ -410,6 +410,20 @@ struct kfd_ioctl_unmap_memory_from_gpu_args {
>>    	__u32 n_success;		/* to/from KFD */
>>    };
>>    
>> +/* Allocate GWS for specific queue
>> + *
>> + * @gpu_id:      device identifier
>> + * @queue_id:    queue's id that GWS is allocated for
>> + * @num_gws:     how many GWS to allocate
>> + * @gws_array:   used to return the allocated gws
>> + */
>> +struct kfd_ioctl_alloc_queue_gws_args {
>> +	__u32 gpu_id;		/* to KFD */
>> +	__u32 queue_id;		/* to KFD */
>> +	__u32 num_gws;		/* to KFD */
>> +	__u32 *gws_array;	/* from KFD */
> Don't use pointers in ioctl structures. Use uint64_t. Accessing user mode pointers requires copy_to/from_user or similar.
>
> Also prefer to move 64-bit elements to the first element to ensure proper alignment, and pad the structure to 64-bit for ABI compatibility.
>
> I'm not sure what your plan is for that gws_array. If it's a pointer to a user mode array, then that array needs be allocated by user mode. And user mode should probably pass down the size of the array it allocated in another parameter.
>
> That said, I think what we want is not an array, but just the index of the first GWS entry that was allocated for the queue, which is currently always 0. So I'm not sure why you're calling this an "array".
>
> [Oak] For the current design, one queue always get all 64 GWS, so returning the index of the first GWS (0) is not a problem. In the future, is it possible queue can only allocate a few none-contiguous GWS, for example GWS3 and GWS56? If this is the case, we will have to copy an array of gws back to user space.

Current HW only support contiguous allocations of GWS. I don't foresee 
that changing. I think we can acknowledge that in the API.


>
>> +};
>> +
>>    struct kfd_ioctl_get_dmabuf_info_args {
>>    	__u64 size;		/* from KFD */
>>    	__u64 metadata_ptr;	/* to KFD */
>> @@ -529,7 +543,10 @@ enum kfd_mmio_remap {
>>    #define AMDKFD_IOC_IMPORT_DMABUF		\
>>    		AMDKFD_IOWR(0x1D, struct kfd_ioctl_import_dmabuf_args)
>>    
>> +#define AMDKFD_IOC_ALLOC_QUEUE_GWS		\
>> +		AMDKFD_IOWR(0x1E, struct kfd_ioctl_alloc_queue_gws_args)
>> +
> This will force us to move some ioctl numbers in amd-kfd-staging and the DKMS branch, which will break the ABI of our ROCm releases. Not sure there is a good way to avoid that other than leaving a whole in the upstream ioctl space. I got push-back on that kind of thing when I originally upstreamed KFD. So this is just an FYI.
> [Oak] Yes, when we merge this changes back to amd-kfd-staging branch, we have to change the ioctl number for GWS allocation to 0x22, to accommodate extra kfd ioctls on kfd-staging branch. @Russell, Kent FYI

No, the other way around. With APIs that are upstream we should have 
amd-kfd-staging match the upstream ABI. That means we have to move the 
other non-upstream ioctl numbers.

Regards,
   Felix


>
> Regards,
>     Felix
>
>>    #define AMDKFD_COMMAND_START		0x01
>> -#define AMDKFD_COMMAND_END		0x1E
>> +#define AMDKFD_COMMAND_END		0x1F
>>    
>>    #endif
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 7/8] drm/amdkfd: New IOCTL to allocate queue GWS

[Christian König]

Am 13.05.19 um 20:12 schrieb Kuehling, Felix:
> On 2019-05-13 12:03 p.m., Zeng, Oak wrote:
>> Hi Felix,
>>
>> See comments inline [Oak]
>>
>> Hi Kent, there is one FYI embedded, so be careful when you merge this change back to kfd-staging branch.
>>
>> Regards,
>> Oak
>>
>> -----Original Message-----
>> From: Kuehling, Felix <Felix.Kuehling@amd.com>
>> Sent: Friday, May 10, 2019 4:28 PM
>> To: Zeng, Oak <Oak.Zeng@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Keely, Sean <Sean.Keely@amd.com>
>> Subject: Re: [PATCH 7/8] drm/amdkfd: New IOCTL to allocate queue GWS
>>
>> On 2019-05-10 12:01 p.m., Zeng, Oak wrote:
>>> Add a new kfd ioctl to allocate queue GWS. Queue GWS is released on
>>> queue destroy.
>>>
>>> Change-Id: I60153c26a577992ad873e4292e759e5c3d5bbd15
>>> Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
>>> ---
>>>     drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 45 ++++++++++++++++++++++++++++++++
>>>     include/uapi/linux/kfd_ioctl.h           | 19 +++++++++++++-
>>>     2 files changed, 63 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> index 38ae53f..17dd970 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> @@ -338,6 +338,11 @@ static int kfd_ioctl_destroy_queue(struct file
>>> *filp, struct kfd_process *p,
>>>     
>>>     	mutex_lock(&p->mutex);
>>>     
>>> +	if (pqm_get_gws(&p->pqm, args->queue_id)) {
>>> +		amdgpu_amdkfd_remove_gws_from_process(p->kgd_process_info,
>>> +				pqm_get_gws(&p->pqm, args->queue_id));
>>> +		pqm_set_gws(&p->pqm, args->queue_id, NULL);
>>> +	}
>> It would be more robust if this was done inside pqm_destroy_queue. That way you'd handle other potential code paths that destroy queues (not sure there are any) and you wouldn't need pqm_get_gws exported from PQM.
>>
>> [Oak] Good idea. Will do it. Even though currently there is no other code path destroying a queue using gws, your way is safer for future code change.
>>
>>
>>>     	retval = pqm_destroy_queue(&p->pqm, args->queue_id);
>>>     
>>>     	mutex_unlock(&p->mutex);
>>> @@ -1559,6 +1564,44 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file *filep,
>>>     	return err;
>>>     }
>>>     
>>> +static int kfd_ioctl_alloc_queue_gws(struct file *filep,
>>> +		struct kfd_process *p, void *data)
>>> +{
>>> +	int retval;
>>> +	struct kfd_ioctl_alloc_queue_gws_args *args = data;
>>> +	struct kfd_dev *dev = NULL;
>>> +	struct kgd_mem *mem;
>>> +
>>> +	if (args->num_gws == 0)
>>> +		return -EINVAL;
>>> +
>>> +	if (!hws_gws_support ||
>>> +		dev->dqm->sched_policy == KFD_SCHED_POLICY_NO_HWS)
>>> +		return -EINVAL;
>>> +
>>> +	dev = kfd_device_by_id(args->gpu_id);
>>> +	if (!dev) {
>>> +		pr_debug("Could not find gpu id 0x%x\n", args->gpu_id);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	retval = amdgpu_amdkfd_add_gws_to_process(p->kgd_process_info,
>>> +			dev->gws, &mem);
>>> +	if (unlikely(retval))
>>> +		return retval;
>>> +
>>> +	mutex_lock(&p->mutex);
>>> +	retval = pqm_set_gws(&p->pqm, args->queue_id, mem);
>>> +	mutex_unlock(&p->mutex);
>>> +
>>> +	if (unlikely(retval))
>>> +		amdgpu_amdkfd_remove_gws_from_process(p->kgd_process_info, mem);
>>> +
>>> +	/* The gws_array parameter is reserved for future extension*/
>>> +	args->gws_array[0] = 0;
>>> +	return retval;
>>> +}
>>> +
>>>     static int kfd_ioctl_get_dmabuf_info(struct file *filep,
>>>     		struct kfd_process *p, void *data)
>>>     {
>>> @@ -1761,6 +1804,8 @@ static const struct amdkfd_ioctl_desc amdkfd_ioctls[] = {
>>>     	AMDKFD_IOCTL_DEF(AMDKFD_IOC_IMPORT_DMABUF,
>>>     				kfd_ioctl_import_dmabuf, 0),
>>>     
>>> +	AMDKFD_IOCTL_DEF(AMDKFD_IOC_ALLOC_QUEUE_GWS,
>>> +			kfd_ioctl_alloc_queue_gws, 0),
>>>     };
>>>     
>>>     #define AMDKFD_CORE_IOCTL_COUNT	ARRAY_SIZE(amdkfd_ioctls)
>>> diff --git a/include/uapi/linux/kfd_ioctl.h
>>> b/include/uapi/linux/kfd_ioctl.h index 20917c5..1964ab2 100644
>>> --- a/include/uapi/linux/kfd_ioctl.h
>>> +++ b/include/uapi/linux/kfd_ioctl.h
>>> @@ -410,6 +410,20 @@ struct kfd_ioctl_unmap_memory_from_gpu_args {
>>>     	__u32 n_success;		/* to/from KFD */
>>>     };
>>>     
>>> +/* Allocate GWS for specific queue
>>> + *
>>> + * @gpu_id:      device identifier
>>> + * @queue_id:    queue's id that GWS is allocated for
>>> + * @num_gws:     how many GWS to allocate
>>> + * @gws_array:   used to return the allocated gws
>>> + */
>>> +struct kfd_ioctl_alloc_queue_gws_args {
>>> +	__u32 gpu_id;		/* to KFD */
>>> +	__u32 queue_id;		/* to KFD */
>>> +	__u32 num_gws;		/* to KFD */
>>> +	__u32 *gws_array;	/* from KFD */
>> Don't use pointers in ioctl structures. Use uint64_t. Accessing user mode pointers requires copy_to/from_user or similar.
>>
>> Also prefer to move 64-bit elements to the first element to ensure proper alignment, and pad the structure to 64-bit for ABI compatibility.
>>
>> I'm not sure what your plan is for that gws_array. If it's a pointer to a user mode array, then that array needs be allocated by user mode. And user mode should probably pass down the size of the array it allocated in another parameter.
>>
>> That said, I think what we want is not an array, but just the index of the first GWS entry that was allocated for the queue, which is currently always 0. So I'm not sure why you're calling this an "array".
>>
>> [Oak] For the current design, one queue always get all 64 GWS, so returning the index of the first GWS (0) is not a problem. In the future, is it possible queue can only allocate a few none-contiguous GWS, for example GWS3 and GWS56? If this is the case, we will have to copy an array of gws back to user space.
> Current HW only support contiguous allocations of GWS. I don't foresee
> that changing. I think we can acknowledge that in the API.

You actually only need to know how many GWS blocks userspace wants to 
use. E.g. the hardware always remaps them starting from 0.

And yes this remapping is always contiguous,
Christian.

>
>
>>> +};
>>> +
>>>     struct kfd_ioctl_get_dmabuf_info_args {
>>>     	__u64 size;		/* from KFD */
>>>     	__u64 metadata_ptr;	/* to KFD */
>>> @@ -529,7 +543,10 @@ enum kfd_mmio_remap {
>>>     #define AMDKFD_IOC_IMPORT_DMABUF		\
>>>     		AMDKFD_IOWR(0x1D, struct kfd_ioctl_import_dmabuf_args)
>>>     
>>> +#define AMDKFD_IOC_ALLOC_QUEUE_GWS		\
>>> +		AMDKFD_IOWR(0x1E, struct kfd_ioctl_alloc_queue_gws_args)
>>> +
>> This will force us to move some ioctl numbers in amd-kfd-staging and the DKMS branch, which will break the ABI of our ROCm releases. Not sure there is a good way to avoid that other than leaving a whole in the upstream ioctl space. I got push-back on that kind of thing when I originally upstreamed KFD. So this is just an FYI.
>> [Oak] Yes, when we merge this changes back to amd-kfd-staging branch, we have to change the ioctl number for GWS allocation to 0x22, to accommodate extra kfd ioctls on kfd-staging branch. @Russell, Kent FYI
> No, the other way around. With APIs that are upstream we should have
> amd-kfd-staging match the upstream ABI. That means we have to move the
> other non-upstream ioctl numbers.
>
> Regards,
>     Felix
>
>
>> Regards,
>>      Felix
>>
>>>     #define AMDKFD_COMMAND_START		0x01
>>> -#define AMDKFD_COMMAND_END		0x1E
>>> +#define AMDKFD_COMMAND_END		0x1F
>>>     
>>>     #endif
> _______________________________________________
> 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