[PATCH 1/2] drm/amdkfd: Add an optional argument into update queue operation

[PATCH 1/2] drm/amdkfd: Add an optional argument into update queue operation

[Lang Yu]

Currently, queue is updated with data stored in queue_properties.
And all allocated resource in queue_properties will not be freed
until the queue is destroyed.

But some properties(e.g., cu mask) bring some memory management
headaches(e.g., memory leak) and make code complex. Actually they
don't have to persist in queue_properties.

So add an argument into update queue to pass such properties and
remove them from queue_properties.

Signed-off-by: Lang Yu <lang.yu@amd.com>
---
 .../drm/amd/amdkfd/kfd_device_queue_manager.c |  4 ++--
 .../drm/amd/amdkfd/kfd_device_queue_manager.h |  2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h  |  2 +-
 .../gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c  | 18 +++++++--------
 .../gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c  |  8 +++----
 .../gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c   |  8 +++----
 .../gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c   | 22 +++++++++----------
 .../amd/amdkfd/kfd_process_queue_manager.c    |  6 ++---
 8 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
index f8fce9d05f50..7f6f4937eedb 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
@@ -557,7 +557,7 @@ static int destroy_queue_nocpsch(struct device_queue_manager *dqm,
 	return retval;
 }
 
-static int update_queue(struct device_queue_manager *dqm, struct queue *q)
+static int update_queue(struct device_queue_manager *dqm, struct queue *q, void *args)
 {
 	int retval = 0;
 	struct mqd_manager *mqd_mgr;
@@ -605,7 +605,7 @@ static int update_queue(struct device_queue_manager *dqm, struct queue *q)
 		}
 	}
 
-	mqd_mgr->update_mqd(mqd_mgr, q->mqd, &q->properties);
+	mqd_mgr->update_mqd(mqd_mgr, q->mqd, &q->properties, args);
 
 	/*
 	 * check active state vs. the previous state and modify
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
index c8719682c4da..08cfc2a2fdbb 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
@@ -93,7 +93,7 @@ struct device_queue_manager_ops {
 				struct queue *q);
 
 	int	(*update_queue)(struct device_queue_manager *dqm,
-				struct queue *q);
+				struct queue *q, void *args);
 
 	int	(*register_process)(struct device_queue_manager *dqm,
 					struct qcm_process_device *qpd);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
index 6e6918ccedfd..6ddf93629b8c 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
@@ -80,7 +80,7 @@ struct mqd_manager {
 				struct mm_struct *mms);
 
 	void	(*update_mqd)(struct mqd_manager *mm, void *mqd,
-				struct queue_properties *q);
+				struct queue_properties *q, void *args);
 
 	int	(*destroy_mqd)(struct mqd_manager *mm, void *mqd,
 				enum kfd_preempt_type type,
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c
index 064914e1e8d6..8bb2fd4cba41 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c
@@ -135,7 +135,7 @@ static void init_mqd(struct mqd_manager *mm, void **mqd,
 	*mqd = m;
 	if (gart_addr)
 		*gart_addr = addr;
-	mm->update_mqd(mm, m, q);
+	mm->update_mqd(mm, m, q, NULL);
 }
 
 static void init_mqd_sdma(struct mqd_manager *mm, void **mqd,
@@ -152,7 +152,7 @@ static void init_mqd_sdma(struct mqd_manager *mm, void **mqd,
 	if (gart_addr)
 		*gart_addr = mqd_mem_obj->gpu_addr;
 
-	mm->update_mqd(mm, m, q);
+	mm->update_mqd(mm, m, q, NULL);
 }
 
 static void free_mqd(struct mqd_manager *mm, void *mqd,
@@ -185,7 +185,7 @@ static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
 }
 
 static void __update_mqd(struct mqd_manager *mm, void *mqd,
-			struct queue_properties *q, unsigned int atc_bit)
+			struct queue_properties *q, void *args, unsigned int atc_bit)
 {
 	struct cik_mqd *m;
 
@@ -221,9 +221,9 @@ static void __update_mqd(struct mqd_manager *mm, void *mqd,
 }
 
 static void update_mqd(struct mqd_manager *mm, void *mqd,
-			struct queue_properties *q)
+			struct queue_properties *q, void *args)
 {
-	__update_mqd(mm, mqd, q, 1);
+	__update_mqd(mm, mqd, q, args, 1);
 }
 
 static uint32_t read_doorbell_id(void *mqd)
@@ -234,13 +234,13 @@ static uint32_t read_doorbell_id(void *mqd)
 }
 
 static void update_mqd_hawaii(struct mqd_manager *mm, void *mqd,
-			struct queue_properties *q)
+			struct queue_properties *q, void *args)
 {
-	__update_mqd(mm, mqd, q, 0);
+	__update_mqd(mm, mqd, q, args, 0);
 }
 
 static void update_mqd_sdma(struct mqd_manager *mm, void *mqd,
-				struct queue_properties *q)
+				struct queue_properties *q, void *args)
 {
 	struct cik_sdma_rlc_registers *m;
 
@@ -318,7 +318,7 @@ static void init_mqd_hiq(struct mqd_manager *mm, void **mqd,
 }
 
 static void update_mqd_hiq(struct mqd_manager *mm, void *mqd,
-				struct queue_properties *q)
+				struct queue_properties *q, void *args)
 {
 	struct cik_mqd *m;
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
index c7fb59ca597f..6d468b6c8d7d 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
@@ -136,7 +136,7 @@ static void init_mqd(struct mqd_manager *mm, void **mqd,
 	*mqd = m;
 	if (gart_addr)
 		*gart_addr = addr;
-	mm->update_mqd(mm, m, q);
+	mm->update_mqd(mm, m, q, NULL);
 }
 
 static int load_mqd(struct mqd_manager *mm, void *mqd,
@@ -162,7 +162,7 @@ static int hiq_load_mqd_kiq(struct mqd_manager *mm, void *mqd,
 }
 
 static void update_mqd(struct mqd_manager *mm, void *mqd,
-		      struct queue_properties *q)
+		      struct queue_properties *q, void *args)
 {
 	struct v10_compute_mqd *m;
 
@@ -311,7 +311,7 @@ static void init_mqd_sdma(struct mqd_manager *mm, void **mqd,
 	if (gart_addr)
 		*gart_addr = mqd_mem_obj->gpu_addr;
 
-	mm->update_mqd(mm, m, q);
+	mm->update_mqd(mm, m, q, NULL);
 }
 
 static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
@@ -326,7 +326,7 @@ static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
 #define SDMA_RLC_DUMMY_DEFAULT 0xf
 
 static void update_mqd_sdma(struct mqd_manager *mm, void *mqd,
-		struct queue_properties *q)
+		struct queue_properties *q, void *args)
 {
 	struct v10_sdma_mqd *m;
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
index 7f4e102ff4bd..f6c10b1b5f8b 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
@@ -188,7 +188,7 @@ static void init_mqd(struct mqd_manager *mm, void **mqd,
 	*mqd = m;
 	if (gart_addr)
 		*gart_addr = addr;
-	mm->update_mqd(mm, m, q);
+	mm->update_mqd(mm, m, q, NULL);
 }
 
 static int load_mqd(struct mqd_manager *mm, void *mqd,
@@ -212,7 +212,7 @@ static int hiq_load_mqd_kiq(struct mqd_manager *mm, void *mqd,
 }
 
 static void update_mqd(struct mqd_manager *mm, void *mqd,
-		      struct queue_properties *q)
+		      struct queue_properties *q, void *args)
 {
 	struct v9_mqd *m;
 
@@ -366,7 +366,7 @@ static void init_mqd_sdma(struct mqd_manager *mm, void **mqd,
 	if (gart_addr)
 		*gart_addr = mqd_mem_obj->gpu_addr;
 
-	mm->update_mqd(mm, m, q);
+	mm->update_mqd(mm, m, q, NULL);
 }
 
 static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
@@ -381,7 +381,7 @@ static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
 #define SDMA_RLC_DUMMY_DEFAULT 0xf
 
 static void update_mqd_sdma(struct mqd_manager *mm, void *mqd,
-		struct queue_properties *q)
+		struct queue_properties *q, void *args)
 {
 	struct v9_sdma_mqd *m;
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c
index 33dbd22d290f..bddd6d8fdf32 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c
@@ -150,7 +150,7 @@ static void init_mqd(struct mqd_manager *mm, void **mqd,
 	*mqd = m;
 	if (gart_addr)
 		*gart_addr = addr;
-	mm->update_mqd(mm, m, q);
+	mm->update_mqd(mm, m, q, NULL);
 }
 
 static int load_mqd(struct mqd_manager *mm, void *mqd,
@@ -167,8 +167,8 @@ static int load_mqd(struct mqd_manager *mm, void *mqd,
 }
 
 static void __update_mqd(struct mqd_manager *mm, void *mqd,
-			struct queue_properties *q, unsigned int mtype,
-			unsigned int atc_bit)
+			struct queue_properties *q, void *args,
+			unsigned int mtype, unsigned int atc_bit)
 {
 	struct vi_mqd *m;
 
@@ -238,9 +238,9 @@ static void __update_mqd(struct mqd_manager *mm, void *mqd,
 
 
 static void update_mqd(struct mqd_manager *mm, void *mqd,
-			struct queue_properties *q)
+			struct queue_properties *q, void *args)
 {
-	__update_mqd(mm, mqd, q, MTYPE_CC, 1);
+	__update_mqd(mm, mqd, q, args, MTYPE_CC, 1);
 }
 
 static uint32_t read_doorbell_id(void *mqd)
@@ -251,9 +251,9 @@ static uint32_t read_doorbell_id(void *mqd)
 }
 
 static void update_mqd_tonga(struct mqd_manager *mm, void *mqd,
-			struct queue_properties *q)
+			struct queue_properties *q, void *args)
 {
-	__update_mqd(mm, mqd, q, MTYPE_UC, 0);
+	__update_mqd(mm, mqd, q, args, MTYPE_UC, 0);
 }
 
 static int destroy_mqd(struct mqd_manager *mm, void *mqd,
@@ -317,9 +317,9 @@ static void init_mqd_hiq(struct mqd_manager *mm, void **mqd,
 }
 
 static void update_mqd_hiq(struct mqd_manager *mm, void *mqd,
-			struct queue_properties *q)
+			struct queue_properties *q, void *args)
 {
-	__update_mqd(mm, mqd, q, MTYPE_UC, 0);
+	__update_mqd(mm, mqd, q, args, MTYPE_UC, 0);
 }
 
 static void init_mqd_sdma(struct mqd_manager *mm, void **mqd,
@@ -336,7 +336,7 @@ static void init_mqd_sdma(struct mqd_manager *mm, void **mqd,
 	if (gart_addr)
 		*gart_addr = mqd_mem_obj->gpu_addr;
 
-	mm->update_mqd(mm, m, q);
+	mm->update_mqd(mm, m, q, NULL);
 }
 
 static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
@@ -349,7 +349,7 @@ static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
 }
 
 static void update_mqd_sdma(struct mqd_manager *mm, void *mqd,
-		struct queue_properties *q)
+		struct queue_properties *q, void *args)
 {
 	struct vi_sdma_mqd *m;
 
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 243dd1efcdbf..37529592457d 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
@@ -121,7 +121,7 @@ int pqm_set_gws(struct process_queue_manager *pqm, unsigned int qid,
 	pdd->qpd.num_gws = gws ? amdgpu_amdkfd_get_num_gws(dev->kgd) : 0;
 
 	return pqn->q->device->dqm->ops.update_queue(pqn->q->device->dqm,
-							pqn->q);
+							pqn->q, NULL);
 }
 
 void kfd_process_dequeue_from_all_devices(struct kfd_process *p)
@@ -429,7 +429,7 @@ int pqm_update_queue(struct process_queue_manager *pqm, unsigned int qid,
 	pqn->q->properties.priority = p->priority;
 
 	retval = pqn->q->device->dqm->ops.update_queue(pqn->q->device->dqm,
-							pqn->q);
+							pqn->q, NULL);
 	if (retval != 0)
 		return retval;
 
@@ -457,7 +457,7 @@ int pqm_set_cu_mask(struct process_queue_manager *pqm, unsigned int qid,
 	pqn->q->properties.cu_mask = p->cu_mask;
 
 	retval = pqn->q->device->dqm->ops.update_queue(pqn->q->device->dqm,
-							pqn->q);
+							pqn->q, NULL);
 	if (retval != 0)
 		return retval;
 
-- 
2.25.1

[PATCH 2/2] drm/amdkfd: Remove cu mask from struct queue_properties

[Lang Yu]

Actually, cu_mask has been copied to mqd memory and
don't have to persist in queue_properties. Remove it
from queue_properties.

Use struct queue_update_info to wrap queue_properties
and cu mask, then pass it to update queue operation.

Signed-off-by: Lang Yu <lang.yu@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      | 41 +++++++-------
 drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c |  1 -
 .../gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c  | 10 ++--
 .../gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c  | 10 ++--
 .../gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c   | 10 ++--
 .../gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c   | 10 ++--
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h         | 22 ++++++--
 .../amd/amdkfd/kfd_process_queue_manager.c    | 56 ++++++-------------
 8 files changed, 80 insertions(+), 80 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 9317a2e238d0..baa5de9dd361 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -371,7 +371,7 @@ static int kfd_ioctl_update_queue(struct file *filp, struct kfd_process *p,
 {
 	int retval;
 	struct kfd_ioctl_update_queue_args *args = data;
-	struct queue_properties properties;
+	struct queue_update_info qinfo = {0};
 
 	if (args->queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {
 		pr_err("Queue percentage must be between 0 to KFD_MAX_QUEUE_PERCENTAGE\n");
@@ -395,17 +395,18 @@ static int kfd_ioctl_update_queue(struct file *filp, struct kfd_process *p,
 		return -EINVAL;
 	}
 
-	properties.queue_address = args->ring_base_address;
-	properties.queue_size = args->ring_size;
-	properties.queue_percent = args->queue_percentage;
-	properties.priority = args->queue_priority;
+	qinfo.properties.queue_address = args->ring_base_address;
+	qinfo.properties.queue_size = args->ring_size;
+	qinfo.properties.queue_percent = args->queue_percentage;
+	qinfo.properties.priority = args->queue_priority;
+	qinfo.update_flag = UPDATE_FLAG_PROPERTITY;
 
 	pr_debug("Updating queue id %d for pasid 0x%x\n",
 			args->queue_id, p->pasid);
 
 	mutex_lock(&p->mutex);
 
-	retval = pqm_update_queue(&p->pqm, args->queue_id, &properties);
+	retval = pqm_update_queue(&p->pqm, args->queue_id, &qinfo);
 
 	mutex_unlock(&p->mutex);
 
@@ -418,7 +419,7 @@ static int kfd_ioctl_set_cu_mask(struct file *filp, struct kfd_process *p,
 	int retval;
 	const int max_num_cus = 1024;
 	struct kfd_ioctl_set_cu_mask_args *args = data;
-	struct queue_properties properties;
+	struct queue_update_info qinfo = {0};
 	uint32_t __user *cu_mask_ptr = (uint32_t __user *)args->cu_mask_ptr;
 	size_t cu_mask_size = sizeof(uint32_t) * (args->num_cu_mask / 32);
 
@@ -428,8 +429,8 @@ static int kfd_ioctl_set_cu_mask(struct file *filp, struct kfd_process *p,
 		return -EINVAL;
 	}
 
-	properties.cu_mask_count = args->num_cu_mask;
-	if (properties.cu_mask_count == 0) {
+	qinfo.cu_mask.count = args->num_cu_mask;
+	if (qinfo.cu_mask.count == 0) {
 		pr_debug("CU mask cannot be 0");
 		return -EINVAL;
 	}
@@ -438,31 +439,33 @@ static int kfd_ioctl_set_cu_mask(struct file *filp, struct kfd_process *p,
 	 * limit of max_num_cus bits.  We can then just drop any CU mask bits
 	 * past max_num_cus bits and just use the first max_num_cus bits.
 	 */
-	if (properties.cu_mask_count > max_num_cus) {
+	if (qinfo.cu_mask.count > max_num_cus) {
 		pr_debug("CU mask cannot be greater than 1024 bits");
-		properties.cu_mask_count = max_num_cus;
+		qinfo.cu_mask.count = max_num_cus;
 		cu_mask_size = sizeof(uint32_t) * (max_num_cus/32);
 	}
 
-	properties.cu_mask = kzalloc(cu_mask_size, GFP_KERNEL);
-	if (!properties.cu_mask)
+	qinfo.cu_mask.ptr = kzalloc(cu_mask_size, GFP_KERNEL);
+	if (!qinfo.cu_mask.ptr)
 		return -ENOMEM;
 
-	retval = copy_from_user(properties.cu_mask, cu_mask_ptr, cu_mask_size);
+	retval = copy_from_user(qinfo.cu_mask.ptr, cu_mask_ptr, cu_mask_size);
 	if (retval) {
 		pr_debug("Could not copy CU mask from userspace");
-		kfree(properties.cu_mask);
-		return -EFAULT;
+		retval = -EFAULT;
+		goto out;
 	}
 
+	qinfo.update_flag = UPDATE_FLAG_CU_MASK;
+
 	mutex_lock(&p->mutex);
 
-	retval = pqm_set_cu_mask(&p->pqm, args->queue_id, &properties);
+	retval = pqm_update_queue(&p->pqm, args->queue_id, &qinfo);
 
 	mutex_unlock(&p->mutex);
 
-	if (retval)
-		kfree(properties.cu_mask);
+out:
+	kfree(qinfo.cu_mask.ptr);
 
 	return retval;
 }
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
index a2b77d1df854..64b4ac339904 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
@@ -136,7 +136,6 @@ static bool kq_initialize(struct kernel_queue *kq, struct kfd_dev *dev,
 	prop.write_ptr = (uint32_t *) kq->wptr_gpu_addr;
 	prop.eop_ring_buffer_address = kq->eop_gpu_addr;
 	prop.eop_ring_buffer_size = PAGE_SIZE;
-	prop.cu_mask = NULL;
 
 	if (init_queue(&kq->queue, &prop) != 0)
 		goto err_init_queue;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c
index 8bb2fd4cba41..9b3bb8c5b41d 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c
@@ -42,16 +42,18 @@ static inline struct cik_sdma_rlc_registers *get_sdma_mqd(void *mqd)
 }
 
 static void update_cu_mask(struct mqd_manager *mm, void *mqd,
-			struct queue_properties *q)
+			void *args)
 {
 	struct cik_mqd *m;
 	uint32_t se_mask[4] = {0}; /* 4 is the max # of SEs */
+	struct queue_update_info *qinfo = args;
 
-	if (q->cu_mask_count == 0)
+	if (!qinfo || (qinfo->update_flag != UPDATE_FLAG_CU_MASK) ||
+	    !qinfo->cu_mask.ptr)
 		return;
 
 	mqd_symmetrically_map_cu_mask(mm,
-		q->cu_mask, q->cu_mask_count, se_mask);
+		qinfo->cu_mask.ptr, qinfo->cu_mask.count, se_mask);
 
 	m = get_mqd(mqd);
 	m->compute_static_thread_mgmt_se0 = se_mask[0];
@@ -214,7 +216,7 @@ static void __update_mqd(struct mqd_manager *mm, void *mqd,
 	if (q->format == KFD_QUEUE_FORMAT_AQL)
 		m->cp_hqd_pq_control |= NO_UPDATE_RPTR;
 
-	update_cu_mask(mm, mqd, q);
+	update_cu_mask(mm, mqd, args);
 	set_priority(m, q);
 
 	q->is_active = QUEUE_IS_ACTIVE(*q);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
index 6d468b6c8d7d..2f6c285caf70 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
@@ -42,16 +42,18 @@ static inline struct v10_sdma_mqd *get_sdma_mqd(void *mqd)
 }
 
 static void update_cu_mask(struct mqd_manager *mm, void *mqd,
-			   struct queue_properties *q)
+			   void *args)
 {
 	struct v10_compute_mqd *m;
 	uint32_t se_mask[4] = {0}; /* 4 is the max # of SEs */
+	struct queue_update_info *qinfo = args;
 
-	if (q->cu_mask_count == 0)
+	if (!qinfo || (qinfo->update_flag != UPDATE_FLAG_CU_MASK) ||
+	    !qinfo->cu_mask.ptr)
 		return;
 
 	mqd_symmetrically_map_cu_mask(mm,
-		q->cu_mask, q->cu_mask_count, se_mask);
+		qinfo->cu_mask.ptr, qinfo->cu_mask.count, se_mask);
 
 	m = get_mqd(mqd);
 	m->compute_static_thread_mgmt_se0 = se_mask[0];
@@ -218,7 +220,7 @@ static void update_mqd(struct mqd_manager *mm, void *mqd,
 	if (mm->dev->cwsr_enabled)
 		m->cp_hqd_ctx_save_control = 0;
 
-	update_cu_mask(mm, mqd, q);
+	update_cu_mask(mm, mqd, args);
 	set_priority(m, q);
 
 	q->is_active = QUEUE_IS_ACTIVE(*q);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
index f6c10b1b5f8b..a95d061458b9 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
@@ -43,16 +43,18 @@ static inline struct v9_sdma_mqd *get_sdma_mqd(void *mqd)
 }
 
 static void update_cu_mask(struct mqd_manager *mm, void *mqd,
-			struct queue_properties *q)
+			void *args)
 {
 	struct v9_mqd *m;
 	uint32_t se_mask[KFD_MAX_NUM_SE] = {0};
+	struct queue_update_info *qinfo = args;
 
-	if (q->cu_mask_count == 0)
+	if (!qinfo || (qinfo->update_flag != UPDATE_FLAG_CU_MASK) ||
+	    !qinfo->cu_mask.ptr)
 		return;
 
 	mqd_symmetrically_map_cu_mask(mm,
-		q->cu_mask, q->cu_mask_count, se_mask);
+		qinfo->cu_mask.ptr, qinfo->cu_mask.count, se_mask);
 
 	m = get_mqd(mqd);
 	m->compute_static_thread_mgmt_se0 = se_mask[0];
@@ -269,7 +271,7 @@ static void update_mqd(struct mqd_manager *mm, void *mqd,
 	if (mm->dev->cwsr_enabled && q->ctx_save_restore_area_address)
 		m->cp_hqd_ctx_save_control = 0;
 
-	update_cu_mask(mm, mqd, q);
+	update_cu_mask(mm, mqd, args);
 	set_priority(m, q);
 
 	q->is_active = QUEUE_IS_ACTIVE(*q);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c
index bddd6d8fdf32..fc9fdbb29a30 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c
@@ -45,16 +45,18 @@ static inline struct vi_sdma_mqd *get_sdma_mqd(void *mqd)
 }
 
 static void update_cu_mask(struct mqd_manager *mm, void *mqd,
-			struct queue_properties *q)
+			void *args)
 {
 	struct vi_mqd *m;
 	uint32_t se_mask[4] = {0}; /* 4 is the max # of SEs */
+	struct queue_update_info *qinfo = args;
 
-	if (q->cu_mask_count == 0)
+	if (!qinfo || (qinfo->update_flag != UPDATE_FLAG_CU_MASK) ||
+	    !qinfo->cu_mask.ptr)
 		return;
 
 	mqd_symmetrically_map_cu_mask(mm,
-		q->cu_mask, q->cu_mask_count, se_mask);
+		qinfo->cu_mask.ptr, qinfo->cu_mask.count, se_mask);
 
 	m = get_mqd(mqd);
 	m->compute_static_thread_mgmt_se0 = se_mask[0];
@@ -230,7 +232,7 @@ static void __update_mqd(struct mqd_manager *mm, void *mqd,
 			atc_bit << CP_HQD_CTX_SAVE_CONTROL__ATC__SHIFT |
 			mtype << CP_HQD_CTX_SAVE_CONTROL__MTYPE__SHIFT;
 
-	update_cu_mask(mm, mqd, q);
+	update_cu_mask(mm, mqd, args);
 	set_priority(m, q);
 
 	q->is_active = QUEUE_IS_ACTIVE(*q);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 30f08f1606bb..fa7cb8bad0da 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -472,9 +472,6 @@ struct queue_properties {
 	uint32_t ctl_stack_size;
 	uint64_t tba_addr;
 	uint64_t tma_addr;
-	/* Relevant for CU */
-	uint32_t cu_mask_count; /* Must be a multiple of 32 */
-	uint32_t *cu_mask;
 };
 
 #define QUEUE_IS_ACTIVE(q) ((q).queue_size > 0 &&	\
@@ -482,6 +479,23 @@ struct queue_properties {
 			    (q).queue_percent > 0 &&	\
 			    !(q).is_evicted)
 
+enum queue_update_flag {
+	UPDATE_FLAG_PROPERTITY = 0,
+	UPDATE_FLAG_CU_MASK,
+};
+
+struct queue_update_info {
+	union {
+		struct queue_properties properties;
+		struct {
+			uint32_t count; /* Must be a multiple of 32 */
+			uint32_t *ptr;
+		} cu_mask;
+	};
+
+	enum queue_update_flag update_flag;
+};
+
 /**
  * struct queue
  *
@@ -1035,7 +1049,7 @@ int pqm_create_queue(struct process_queue_manager *pqm,
 			    uint32_t *p_doorbell_offset_in_process);
 int pqm_destroy_queue(struct process_queue_manager *pqm, unsigned int qid);
 int pqm_update_queue(struct process_queue_manager *pqm, unsigned int qid,
-			struct queue_properties *p);
+			void *args);
 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,
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 37529592457d..cc81fb36d278 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
@@ -394,8 +394,6 @@ int pqm_destroy_queue(struct process_queue_manager *pqm, unsigned int qid)
 			pdd->qpd.num_gws = 0;
 		}
 
-		kfree(pqn->q->properties.cu_mask);
-		pqn->q->properties.cu_mask = NULL;
 		uninit_queue(pqn->q);
 	}
 
@@ -412,35 +410,14 @@ int pqm_destroy_queue(struct process_queue_manager *pqm, unsigned int qid)
 }
 
 int pqm_update_queue(struct process_queue_manager *pqm, unsigned int qid,
-			struct queue_properties *p)
+			void *args)
 {
-	int retval;
+	int retval = 0;
 	struct process_queue_node *pqn;
+	struct queue_update_info *qinfo = args;
 
-	pqn = get_queue_by_qid(pqm, qid);
-	if (!pqn) {
-		pr_debug("No queue %d exists for update operation\n", qid);
-		return -EFAULT;
-	}
-
-	pqn->q->properties.queue_address = p->queue_address;
-	pqn->q->properties.queue_size = p->queue_size;
-	pqn->q->properties.queue_percent = p->queue_percent;
-	pqn->q->properties.priority = p->priority;
-
-	retval = pqn->q->device->dqm->ops.update_queue(pqn->q->device->dqm,
-							pqn->q, NULL);
-	if (retval != 0)
-		return retval;
-
-	return 0;
-}
-
-int pqm_set_cu_mask(struct process_queue_manager *pqm, unsigned int qid,
-			struct queue_properties *p)
-{
-	int retval;
-	struct process_queue_node *pqn;
+	if (!qinfo)
+		return -EINVAL;
 
 	pqn = get_queue_by_qid(pqm, qid);
 	if (!pqn) {
@@ -448,20 +425,19 @@ int pqm_set_cu_mask(struct process_queue_manager *pqm, unsigned int qid,
 		return -EFAULT;
 	}
 
-	/* Free the old CU mask memory if it is already allocated, then
-	 * allocate memory for the new CU mask.
-	 */
-	kfree(pqn->q->properties.cu_mask);
-
-	pqn->q->properties.cu_mask_count = p->cu_mask_count;
-	pqn->q->properties.cu_mask = p->cu_mask;
-
-	retval = pqn->q->device->dqm->ops.update_queue(pqn->q->device->dqm,
+	if (qinfo->update_flag == UPDATE_FLAG_PROPERTITY) {
+		pqn->q->properties.queue_address = qinfo->properties.queue_address;
+		pqn->q->properties.queue_size = qinfo->properties.queue_size;
+		pqn->q->properties.queue_percent = qinfo->properties.queue_percent;
+		pqn->q->properties.priority = qinfo->properties.priority;
+		retval = pqn->q->device->dqm->ops.update_queue(pqn->q->device->dqm,
 							pqn->q, NULL);
-	if (retval != 0)
-		return retval;
+	} else {
+		retval = pqn->q->device->dqm->ops.update_queue(pqn->q->device->dqm,
+							pqn->q, qinfo);
+	}
 
-	return 0;
+	return retval;
 }
 
 struct kernel_queue *pqm_get_kernel_queue(
-- 
2.25.1

Re: [PATCH 1/2] drm/amdkfd: Add an optional argument into update queue operation

[Felix Kuehling]

Am 2021-10-15 um 4:48 a.m. schrieb Lang Yu:
> Currently, queue is updated with data stored in queue_properties.
> And all allocated resource in queue_properties will not be freed
> until the queue is destroyed.
>
> But some properties(e.g., cu mask) bring some memory management
> headaches(e.g., memory leak) and make code complex. Actually they
> don't have to persist in queue_properties.
>
> So add an argument into update queue to pass such properties and
> remove them from queue_properties.
>
> Signed-off-by: Lang Yu <lang.yu@amd.com>
> ---
>  .../drm/amd/amdkfd/kfd_device_queue_manager.c |  4 ++--
>  .../drm/amd/amdkfd/kfd_device_queue_manager.h |  2 +-
>  drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h  |  2 +-
>  .../gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c  | 18 +++++++--------
>  .../gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c  |  8 +++----
>  .../gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c   |  8 +++----
>  .../gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c   | 22 +++++++++----------
>  .../amd/amdkfd/kfd_process_queue_manager.c    |  6 ++---
>  8 files changed, 35 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> index f8fce9d05f50..7f6f4937eedb 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -557,7 +557,7 @@ static int destroy_queue_nocpsch(struct device_queue_manager *dqm,
>  	return retval;
>  }
>  
> -static int update_queue(struct device_queue_manager *dqm, struct queue *q)
> +static int update_queue(struct device_queue_manager *dqm, struct queue *q, void *args)

Please don't use a void * here. If you don't want to declare the struct
queue_update_info in this patch, you can just declare it as an abstract
type:

    struct queue_update_info;

You can cast NULL to (struct queue_update_info *) without requiring the
structure definition.

Regards,
  Felix


>  {
>  	int retval = 0;
>  	struct mqd_manager *mqd_mgr;
> @@ -605,7 +605,7 @@ static int update_queue(struct device_queue_manager *dqm, struct queue *q)
>  		}
>  	}
>  
> -	mqd_mgr->update_mqd(mqd_mgr, q->mqd, &q->properties);
> +	mqd_mgr->update_mqd(mqd_mgr, q->mqd, &q->properties, args);
>  
>  	/*
>  	 * check active state vs. the previous state and modify
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> index c8719682c4da..08cfc2a2fdbb 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> @@ -93,7 +93,7 @@ struct device_queue_manager_ops {
>  				struct queue *q);
>  
>  	int	(*update_queue)(struct device_queue_manager *dqm,
> -				struct queue *q);
> +				struct queue *q, void *args);
>  
>  	int	(*register_process)(struct device_queue_manager *dqm,
>  					struct qcm_process_device *qpd);
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
> index 6e6918ccedfd..6ddf93629b8c 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
> @@ -80,7 +80,7 @@ struct mqd_manager {
>  				struct mm_struct *mms);
>  
>  	void	(*update_mqd)(struct mqd_manager *mm, void *mqd,
> -				struct queue_properties *q);
> +				struct queue_properties *q, void *args);
>  
>  	int	(*destroy_mqd)(struct mqd_manager *mm, void *mqd,
>  				enum kfd_preempt_type type,
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c
> index 064914e1e8d6..8bb2fd4cba41 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c
> @@ -135,7 +135,7 @@ static void init_mqd(struct mqd_manager *mm, void **mqd,
>  	*mqd = m;
>  	if (gart_addr)
>  		*gart_addr = addr;
> -	mm->update_mqd(mm, m, q);
> +	mm->update_mqd(mm, m, q, NULL);
>  }
>  
>  static void init_mqd_sdma(struct mqd_manager *mm, void **mqd,
> @@ -152,7 +152,7 @@ static void init_mqd_sdma(struct mqd_manager *mm, void **mqd,
>  	if (gart_addr)
>  		*gart_addr = mqd_mem_obj->gpu_addr;
>  
> -	mm->update_mqd(mm, m, q);
> +	mm->update_mqd(mm, m, q, NULL);
>  }
>  
>  static void free_mqd(struct mqd_manager *mm, void *mqd,
> @@ -185,7 +185,7 @@ static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
>  }
>  
>  static void __update_mqd(struct mqd_manager *mm, void *mqd,
> -			struct queue_properties *q, unsigned int atc_bit)
> +			struct queue_properties *q, void *args, unsigned int atc_bit)
>  {
>  	struct cik_mqd *m;
>  
> @@ -221,9 +221,9 @@ static void __update_mqd(struct mqd_manager *mm, void *mqd,
>  }
>  
>  static void update_mqd(struct mqd_manager *mm, void *mqd,
> -			struct queue_properties *q)
> +			struct queue_properties *q, void *args)
>  {
> -	__update_mqd(mm, mqd, q, 1);
> +	__update_mqd(mm, mqd, q, args, 1);
>  }
>  
>  static uint32_t read_doorbell_id(void *mqd)
> @@ -234,13 +234,13 @@ static uint32_t read_doorbell_id(void *mqd)
>  }
>  
>  static void update_mqd_hawaii(struct mqd_manager *mm, void *mqd,
> -			struct queue_properties *q)
> +			struct queue_properties *q, void *args)
>  {
> -	__update_mqd(mm, mqd, q, 0);
> +	__update_mqd(mm, mqd, q, args, 0);
>  }
>  
>  static void update_mqd_sdma(struct mqd_manager *mm, void *mqd,
> -				struct queue_properties *q)
> +				struct queue_properties *q, void *args)
>  {
>  	struct cik_sdma_rlc_registers *m;
>  
> @@ -318,7 +318,7 @@ static void init_mqd_hiq(struct mqd_manager *mm, void **mqd,
>  }
>  
>  static void update_mqd_hiq(struct mqd_manager *mm, void *mqd,
> -				struct queue_properties *q)
> +				struct queue_properties *q, void *args)
>  {
>  	struct cik_mqd *m;
>  
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
> index c7fb59ca597f..6d468b6c8d7d 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
> @@ -136,7 +136,7 @@ static void init_mqd(struct mqd_manager *mm, void **mqd,
>  	*mqd = m;
>  	if (gart_addr)
>  		*gart_addr = addr;
> -	mm->update_mqd(mm, m, q);
> +	mm->update_mqd(mm, m, q, NULL);
>  }
>  
>  static int load_mqd(struct mqd_manager *mm, void *mqd,
> @@ -162,7 +162,7 @@ static int hiq_load_mqd_kiq(struct mqd_manager *mm, void *mqd,
>  }
>  
>  static void update_mqd(struct mqd_manager *mm, void *mqd,
> -		      struct queue_properties *q)
> +		      struct queue_properties *q, void *args)
>  {
>  	struct v10_compute_mqd *m;
>  
> @@ -311,7 +311,7 @@ static void init_mqd_sdma(struct mqd_manager *mm, void **mqd,
>  	if (gart_addr)
>  		*gart_addr = mqd_mem_obj->gpu_addr;
>  
> -	mm->update_mqd(mm, m, q);
> +	mm->update_mqd(mm, m, q, NULL);
>  }
>  
>  static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
> @@ -326,7 +326,7 @@ static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
>  #define SDMA_RLC_DUMMY_DEFAULT 0xf
>  
>  static void update_mqd_sdma(struct mqd_manager *mm, void *mqd,
> -		struct queue_properties *q)
> +		struct queue_properties *q, void *args)
>  {
>  	struct v10_sdma_mqd *m;
>  
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
> index 7f4e102ff4bd..f6c10b1b5f8b 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
> @@ -188,7 +188,7 @@ static void init_mqd(struct mqd_manager *mm, void **mqd,
>  	*mqd = m;
>  	if (gart_addr)
>  		*gart_addr = addr;
> -	mm->update_mqd(mm, m, q);
> +	mm->update_mqd(mm, m, q, NULL);
>  }
>  
>  static int load_mqd(struct mqd_manager *mm, void *mqd,
> @@ -212,7 +212,7 @@ static int hiq_load_mqd_kiq(struct mqd_manager *mm, void *mqd,
>  }
>  
>  static void update_mqd(struct mqd_manager *mm, void *mqd,
> -		      struct queue_properties *q)
> +		      struct queue_properties *q, void *args)
>  {
>  	struct v9_mqd *m;
>  
> @@ -366,7 +366,7 @@ static void init_mqd_sdma(struct mqd_manager *mm, void **mqd,
>  	if (gart_addr)
>  		*gart_addr = mqd_mem_obj->gpu_addr;
>  
> -	mm->update_mqd(mm, m, q);
> +	mm->update_mqd(mm, m, q, NULL);
>  }
>  
>  static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
> @@ -381,7 +381,7 @@ static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
>  #define SDMA_RLC_DUMMY_DEFAULT 0xf
>  
>  static void update_mqd_sdma(struct mqd_manager *mm, void *mqd,
> -		struct queue_properties *q)
> +		struct queue_properties *q, void *args)
>  {
>  	struct v9_sdma_mqd *m;
>  
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c
> index 33dbd22d290f..bddd6d8fdf32 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c
> @@ -150,7 +150,7 @@ static void init_mqd(struct mqd_manager *mm, void **mqd,
>  	*mqd = m;
>  	if (gart_addr)
>  		*gart_addr = addr;
> -	mm->update_mqd(mm, m, q);
> +	mm->update_mqd(mm, m, q, NULL);
>  }
>  
>  static int load_mqd(struct mqd_manager *mm, void *mqd,
> @@ -167,8 +167,8 @@ static int load_mqd(struct mqd_manager *mm, void *mqd,
>  }
>  
>  static void __update_mqd(struct mqd_manager *mm, void *mqd,
> -			struct queue_properties *q, unsigned int mtype,
> -			unsigned int atc_bit)
> +			struct queue_properties *q, void *args,
> +			unsigned int mtype, unsigned int atc_bit)
>  {
>  	struct vi_mqd *m;
>  
> @@ -238,9 +238,9 @@ static void __update_mqd(struct mqd_manager *mm, void *mqd,
>  
>  
>  static void update_mqd(struct mqd_manager *mm, void *mqd,
> -			struct queue_properties *q)
> +			struct queue_properties *q, void *args)
>  {
> -	__update_mqd(mm, mqd, q, MTYPE_CC, 1);
> +	__update_mqd(mm, mqd, q, args, MTYPE_CC, 1);
>  }
>  
>  static uint32_t read_doorbell_id(void *mqd)
> @@ -251,9 +251,9 @@ static uint32_t read_doorbell_id(void *mqd)
>  }
>  
>  static void update_mqd_tonga(struct mqd_manager *mm, void *mqd,
> -			struct queue_properties *q)
> +			struct queue_properties *q, void *args)
>  {
> -	__update_mqd(mm, mqd, q, MTYPE_UC, 0);
> +	__update_mqd(mm, mqd, q, args, MTYPE_UC, 0);
>  }
>  
>  static int destroy_mqd(struct mqd_manager *mm, void *mqd,
> @@ -317,9 +317,9 @@ static void init_mqd_hiq(struct mqd_manager *mm, void **mqd,
>  }
>  
>  static void update_mqd_hiq(struct mqd_manager *mm, void *mqd,
> -			struct queue_properties *q)
> +			struct queue_properties *q, void *args)
>  {
> -	__update_mqd(mm, mqd, q, MTYPE_UC, 0);
> +	__update_mqd(mm, mqd, q, args, MTYPE_UC, 0);
>  }
>  
>  static void init_mqd_sdma(struct mqd_manager *mm, void **mqd,
> @@ -336,7 +336,7 @@ static void init_mqd_sdma(struct mqd_manager *mm, void **mqd,
>  	if (gart_addr)
>  		*gart_addr = mqd_mem_obj->gpu_addr;
>  
> -	mm->update_mqd(mm, m, q);
> +	mm->update_mqd(mm, m, q, NULL);
>  }
>  
>  static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
> @@ -349,7 +349,7 @@ static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
>  }
>  
>  static void update_mqd_sdma(struct mqd_manager *mm, void *mqd,
> -		struct queue_properties *q)
> +		struct queue_properties *q, void *args)
>  {
>  	struct vi_sdma_mqd *m;
>  
> 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 243dd1efcdbf..37529592457d 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> @@ -121,7 +121,7 @@ int pqm_set_gws(struct process_queue_manager *pqm, unsigned int qid,
>  	pdd->qpd.num_gws = gws ? amdgpu_amdkfd_get_num_gws(dev->kgd) : 0;
>  
>  	return pqn->q->device->dqm->ops.update_queue(pqn->q->device->dqm,
> -							pqn->q);
> +							pqn->q, NULL);
>  }
>  
>  void kfd_process_dequeue_from_all_devices(struct kfd_process *p)
> @@ -429,7 +429,7 @@ int pqm_update_queue(struct process_queue_manager *pqm, unsigned int qid,
>  	pqn->q->properties.priority = p->priority;
>  
>  	retval = pqn->q->device->dqm->ops.update_queue(pqn->q->device->dqm,
> -							pqn->q);
> +							pqn->q, NULL);
>  	if (retval != 0)
>  		return retval;
>  
> @@ -457,7 +457,7 @@ int pqm_set_cu_mask(struct process_queue_manager *pqm, unsigned int qid,
>  	pqn->q->properties.cu_mask = p->cu_mask;
>  
>  	retval = pqn->q->device->dqm->ops.update_queue(pqn->q->device->dqm,
> -							pqn->q);
> +							pqn->q, NULL);
>  	if (retval != 0)
>  		return retval;
>  

Re: [PATCH 2/2] drm/amdkfd: Remove cu mask from struct queue_properties

[Felix Kuehling]

Am 2021-10-15 um 4:48 a.m. schrieb Lang Yu:
> +enum queue_update_flag {
> +	UPDATE_FLAG_PROPERTITY = 0,
> +	UPDATE_FLAG_CU_MASK,
> +};
> +
> +struct queue_update_info {
> +	union {
> +		struct queue_properties properties;
> +		struct {
> +			uint32_t count; /* Must be a multiple of 32 */
> +			uint32_t *ptr;
> +		} cu_mask;
> +	};
> +
> +	enum queue_update_flag update_flag;
> +};
> +

This doesn't make sense to me. As I understand it, queue_update_info is
for information that is not stored in queue_properties but only in the
MQDs. Therefore, it should not include the queue_properties.

All the low level functions in the MQD managers get both the
queue_properties and the queue_update_info. So trying to wrap both in
the same union doesn't make sense there either.

I think you only need this because you tried to generalize
pqm_update_queue to handle both updates to queue_properties and CU mask
updates with a single argument. IMO this does not make the interface any
clearer. I think it would be more straight-forward to keep a separate
pqm_set_cu_mask function that takes a queue_update_info parameter. If
you're looking for more generic names, I suggest the following:

  * Rename pqm_update_queue to pqm_update_queue_properties
  * Rename struct queue_update_info to struct mqd_update_info
  * Rename pqm_set_cu_mask to pqm_update_mqd. For now this is only used
    for CU mask (the union has only one struct member for now). It may
    be used for other MQD properties that don't need to be stored in
    queue_properties in the future

Regards,
  Felix

RE: [PATCH 1/2] drm/amdkfd: Add an optional argument into update queue operation

[Yu, Lang]

[AMD Official Use Only]



>-----Original Message-----
>From: Kuehling, Felix <Felix.Kuehling@amd.com>
>Sent: Friday, October 22, 2021 12:46 AM
>To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
><Ray.Huang@amd.com>
>Subject: Re: [PATCH 1/2] drm/amdkfd: Add an optional argument into update
>queue operation
>
>
>Am 2021-10-15 um 4:48 a.m. schrieb Lang Yu:
>> Currently, queue is updated with data stored in queue_properties.
>> And all allocated resource in queue_properties will not be freed until
>> the queue is destroyed.
>>
>> But some properties(e.g., cu mask) bring some memory management
>> headaches(e.g., memory leak) and make code complex. Actually they
>> don't have to persist in queue_properties.
>>
>> So add an argument into update queue to pass such properties and
>> remove them from queue_properties.
>>
>> Signed-off-by: Lang Yu <lang.yu@amd.com>
>> ---
>>  .../drm/amd/amdkfd/kfd_device_queue_manager.c |  4 ++--
>> .../drm/amd/amdkfd/kfd_device_queue_manager.h |  2 +-
>> drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h  |  2 +-
>> .../gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c  | 18 +++++++--------
>> .../gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c  |  8 +++----
>>  .../gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c   |  8 +++----
>>  .../gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c   | 22 +++++++++----------
>>  .../amd/amdkfd/kfd_process_queue_manager.c    |  6 ++---
>>  8 files changed, 35 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>> index f8fce9d05f50..7f6f4937eedb 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>> @@ -557,7 +557,7 @@ static int destroy_queue_nocpsch(struct
>device_queue_manager *dqm,
>>  	return retval;
>>  }
>>
>> -static int update_queue(struct device_queue_manager *dqm, struct
>> queue *q)
>> +static int update_queue(struct device_queue_manager *dqm, struct
>> +queue *q, void *args)
>
>Please don't use a void * here. If you don't want to declare the struct
>queue_update_info in this patch, you can just declare it as an abstract
>type:
>
>    struct queue_update_info;
>
>You can cast NULL to (struct queue_update_info *) without requiring the
>structure definition.

Got it. Thanks!

Regards,
Lang
>
>Regards,
>  Felix
>
>
>>  {
>>  	int retval = 0;
>>  	struct mqd_manager *mqd_mgr;
>> @@ -605,7 +605,7 @@ static int update_queue(struct device_queue_manager
>*dqm, struct queue *q)
>>  		}
>>  	}
>>
>> -	mqd_mgr->update_mqd(mqd_mgr, q->mqd, &q->properties);
>> +	mqd_mgr->update_mqd(mqd_mgr, q->mqd, &q->properties, args);
>>
>>  	/*
>>  	 * check active state vs. the previous state and modify diff --git
>> a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
>> b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
>> index c8719682c4da..08cfc2a2fdbb 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
>> @@ -93,7 +93,7 @@ struct device_queue_manager_ops {
>>  				struct queue *q);
>>
>>  	int	(*update_queue)(struct device_queue_manager *dqm,
>> -				struct queue *q);
>> +				struct queue *q, void *args);
>>
>>  	int	(*register_process)(struct device_queue_manager *dqm,
>>  					struct qcm_process_device *qpd); diff --
>git
>> a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
>> b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
>> index 6e6918ccedfd..6ddf93629b8c 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
>> @@ -80,7 +80,7 @@ struct mqd_manager {
>>  				struct mm_struct *mms);
>>
>>  	void	(*update_mqd)(struct mqd_manager *mm, void *mqd,
>> -				struct queue_properties *q);
>> +				struct queue_properties *q, void *args);
>>
>>  	int	(*destroy_mqd)(struct mqd_manager *mm, void *mqd,
>>  				enum kfd_preempt_type type,
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c
>> index 064914e1e8d6..8bb2fd4cba41 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c
>> @@ -135,7 +135,7 @@ static void init_mqd(struct mqd_manager *mm, void
>**mqd,
>>  	*mqd = m;
>>  	if (gart_addr)
>>  		*gart_addr = addr;
>> -	mm->update_mqd(mm, m, q);
>> +	mm->update_mqd(mm, m, q, NULL);
>>  }
>>
>>  static void init_mqd_sdma(struct mqd_manager *mm, void **mqd, @@
>> -152,7 +152,7 @@ static void init_mqd_sdma(struct mqd_manager *mm, void
>**mqd,
>>  	if (gart_addr)
>>  		*gart_addr = mqd_mem_obj->gpu_addr;
>>
>> -	mm->update_mqd(mm, m, q);
>> +	mm->update_mqd(mm, m, q, NULL);
>>  }
>>
>>  static void free_mqd(struct mqd_manager *mm, void *mqd, @@ -185,7
>> +185,7 @@ static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
>> }
>>
>>  static void __update_mqd(struct mqd_manager *mm, void *mqd,
>> -			struct queue_properties *q, unsigned int atc_bit)
>> +			struct queue_properties *q, void *args, unsigned int
>atc_bit)
>>  {
>>  	struct cik_mqd *m;
>>
>> @@ -221,9 +221,9 @@ static void __update_mqd(struct mqd_manager *mm,
>> void *mqd,  }
>>
>>  static void update_mqd(struct mqd_manager *mm, void *mqd,
>> -			struct queue_properties *q)
>> +			struct queue_properties *q, void *args)
>>  {
>> -	__update_mqd(mm, mqd, q, 1);
>> +	__update_mqd(mm, mqd, q, args, 1);
>>  }
>>
>>  static uint32_t read_doorbell_id(void *mqd) @@ -234,13 +234,13 @@
>> static uint32_t read_doorbell_id(void *mqd)  }
>>
>>  static void update_mqd_hawaii(struct mqd_manager *mm, void *mqd,
>> -			struct queue_properties *q)
>> +			struct queue_properties *q, void *args)
>>  {
>> -	__update_mqd(mm, mqd, q, 0);
>> +	__update_mqd(mm, mqd, q, args, 0);
>>  }
>>
>>  static void update_mqd_sdma(struct mqd_manager *mm, void *mqd,
>> -				struct queue_properties *q)
>> +				struct queue_properties *q, void *args)
>>  {
>>  	struct cik_sdma_rlc_registers *m;
>>
>> @@ -318,7 +318,7 @@ static void init_mqd_hiq(struct mqd_manager *mm,
>> void **mqd,  }
>>
>>  static void update_mqd_hiq(struct mqd_manager *mm, void *mqd,
>> -				struct queue_properties *q)
>> +				struct queue_properties *q, void *args)
>>  {
>>  	struct cik_mqd *m;
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
>> index c7fb59ca597f..6d468b6c8d7d 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
>> @@ -136,7 +136,7 @@ static void init_mqd(struct mqd_manager *mm, void
>**mqd,
>>  	*mqd = m;
>>  	if (gart_addr)
>>  		*gart_addr = addr;
>> -	mm->update_mqd(mm, m, q);
>> +	mm->update_mqd(mm, m, q, NULL);
>>  }
>>
>>  static int load_mqd(struct mqd_manager *mm, void *mqd, @@ -162,7
>> +162,7 @@ static int hiq_load_mqd_kiq(struct mqd_manager *mm, void
>> *mqd,  }
>>
>>  static void update_mqd(struct mqd_manager *mm, void *mqd,
>> -		      struct queue_properties *q)
>> +		      struct queue_properties *q, void *args)
>>  {
>>  	struct v10_compute_mqd *m;
>>
>> @@ -311,7 +311,7 @@ static void init_mqd_sdma(struct mqd_manager *mm,
>void **mqd,
>>  	if (gart_addr)
>>  		*gart_addr = mqd_mem_obj->gpu_addr;
>>
>> -	mm->update_mqd(mm, m, q);
>> +	mm->update_mqd(mm, m, q, NULL);
>>  }
>>
>>  static int load_mqd_sdma(struct mqd_manager *mm, void *mqd, @@ -326,7
>> +326,7 @@ static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
>> #define SDMA_RLC_DUMMY_DEFAULT 0xf
>>
>>  static void update_mqd_sdma(struct mqd_manager *mm, void *mqd,
>> -		struct queue_properties *q)
>> +		struct queue_properties *q, void *args)
>>  {
>>  	struct v10_sdma_mqd *m;
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
>> index 7f4e102ff4bd..f6c10b1b5f8b 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
>> @@ -188,7 +188,7 @@ static void init_mqd(struct mqd_manager *mm, void
>**mqd,
>>  	*mqd = m;
>>  	if (gart_addr)
>>  		*gart_addr = addr;
>> -	mm->update_mqd(mm, m, q);
>> +	mm->update_mqd(mm, m, q, NULL);
>>  }
>>
>>  static int load_mqd(struct mqd_manager *mm, void *mqd, @@ -212,7
>> +212,7 @@ static int hiq_load_mqd_kiq(struct mqd_manager *mm, void
>> *mqd,  }
>>
>>  static void update_mqd(struct mqd_manager *mm, void *mqd,
>> -		      struct queue_properties *q)
>> +		      struct queue_properties *q, void *args)
>>  {
>>  	struct v9_mqd *m;
>>
>> @@ -366,7 +366,7 @@ static void init_mqd_sdma(struct mqd_manager *mm,
>void **mqd,
>>  	if (gart_addr)
>>  		*gart_addr = mqd_mem_obj->gpu_addr;
>>
>> -	mm->update_mqd(mm, m, q);
>> +	mm->update_mqd(mm, m, q, NULL);
>>  }
>>
>>  static int load_mqd_sdma(struct mqd_manager *mm, void *mqd, @@ -381,7
>> +381,7 @@ static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
>> #define SDMA_RLC_DUMMY_DEFAULT 0xf
>>
>>  static void update_mqd_sdma(struct mqd_manager *mm, void *mqd,
>> -		struct queue_properties *q)
>> +		struct queue_properties *q, void *args)
>>  {
>>  	struct v9_sdma_mqd *m;
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c
>> index 33dbd22d290f..bddd6d8fdf32 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c
>> @@ -150,7 +150,7 @@ static void init_mqd(struct mqd_manager *mm, void
>**mqd,
>>  	*mqd = m;
>>  	if (gart_addr)
>>  		*gart_addr = addr;
>> -	mm->update_mqd(mm, m, q);
>> +	mm->update_mqd(mm, m, q, NULL);
>>  }
>>
>>  static int load_mqd(struct mqd_manager *mm, void *mqd, @@ -167,8
>> +167,8 @@ static int load_mqd(struct mqd_manager *mm, void *mqd,  }
>>
>>  static void __update_mqd(struct mqd_manager *mm, void *mqd,
>> -			struct queue_properties *q, unsigned int mtype,
>> -			unsigned int atc_bit)
>> +			struct queue_properties *q, void *args,
>> +			unsigned int mtype, unsigned int atc_bit)
>>  {
>>  	struct vi_mqd *m;
>>
>> @@ -238,9 +238,9 @@ static void __update_mqd(struct mqd_manager *mm,
>> void *mqd,
>>
>>
>>  static void update_mqd(struct mqd_manager *mm, void *mqd,
>> -			struct queue_properties *q)
>> +			struct queue_properties *q, void *args)
>>  {
>> -	__update_mqd(mm, mqd, q, MTYPE_CC, 1);
>> +	__update_mqd(mm, mqd, q, args, MTYPE_CC, 1);
>>  }
>>
>>  static uint32_t read_doorbell_id(void *mqd) @@ -251,9 +251,9 @@
>> static uint32_t read_doorbell_id(void *mqd)  }
>>
>>  static void update_mqd_tonga(struct mqd_manager *mm, void *mqd,
>> -			struct queue_properties *q)
>> +			struct queue_properties *q, void *args)
>>  {
>> -	__update_mqd(mm, mqd, q, MTYPE_UC, 0);
>> +	__update_mqd(mm, mqd, q, args, MTYPE_UC, 0);
>>  }
>>
>>  static int destroy_mqd(struct mqd_manager *mm, void *mqd, @@ -317,9
>> +317,9 @@ static void init_mqd_hiq(struct mqd_manager *mm, void **mqd,
>> }
>>
>>  static void update_mqd_hiq(struct mqd_manager *mm, void *mqd,
>> -			struct queue_properties *q)
>> +			struct queue_properties *q, void *args)
>>  {
>> -	__update_mqd(mm, mqd, q, MTYPE_UC, 0);
>> +	__update_mqd(mm, mqd, q, args, MTYPE_UC, 0);
>>  }
>>
>>  static void init_mqd_sdma(struct mqd_manager *mm, void **mqd, @@
>> -336,7 +336,7 @@ static void init_mqd_sdma(struct mqd_manager *mm, void
>**mqd,
>>  	if (gart_addr)
>>  		*gart_addr = mqd_mem_obj->gpu_addr;
>>
>> -	mm->update_mqd(mm, m, q);
>> +	mm->update_mqd(mm, m, q, NULL);
>>  }
>>
>>  static int load_mqd_sdma(struct mqd_manager *mm, void *mqd, @@ -349,7
>> +349,7 @@ static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
>> }
>>
>>  static void update_mqd_sdma(struct mqd_manager *mm, void *mqd,
>> -		struct queue_properties *q)
>> +		struct queue_properties *q, void *args)
>>  {
>>  	struct vi_sdma_mqd *m;
>>
>> 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 243dd1efcdbf..37529592457d 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>> @@ -121,7 +121,7 @@ int pqm_set_gws(struct process_queue_manager *pqm,
>unsigned int qid,
>>  	pdd->qpd.num_gws = gws ? amdgpu_amdkfd_get_num_gws(dev->kgd) :
>0;
>>
>>  	return pqn->q->device->dqm->ops.update_queue(pqn->q->device->dqm,
>> -							pqn->q);
>> +							pqn->q, NULL);
>>  }
>>
>>  void kfd_process_dequeue_from_all_devices(struct kfd_process *p) @@
>> -429,7 +429,7 @@ int pqm_update_queue(struct process_queue_manager
>*pqm, unsigned int qid,
>>  	pqn->q->properties.priority = p->priority;
>>
>>  	retval = pqn->q->device->dqm->ops.update_queue(pqn->q->device->dqm,
>> -							pqn->q);
>> +							pqn->q, NULL);
>>  	if (retval != 0)
>>  		return retval;
>>
>> @@ -457,7 +457,7 @@ int pqm_set_cu_mask(struct process_queue_manager
>*pqm, unsigned int qid,
>>  	pqn->q->properties.cu_mask = p->cu_mask;
>>
>>  	retval = pqn->q->device->dqm->ops.update_queue(pqn->q->device->dqm,
>> -							pqn->q);
>> +							pqn->q, NULL);
>>  	if (retval != 0)
>>  		return retval;
>>

RE: [PATCH 2/2] drm/amdkfd: Remove cu mask from struct queue_properties

[Yu, Lang]

[AMD Official Use Only]



>-----Original Message-----
>From: Kuehling, Felix <Felix.Kuehling@amd.com>
>Sent: Friday, October 22, 2021 1:11 AM
>To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
><Ray.Huang@amd.com>
>Subject: Re: [PATCH 2/2] drm/amdkfd: Remove cu mask from struct
>queue_properties
>
>Am 2021-10-15 um 4:48 a.m. schrieb Lang Yu:
>> +enum queue_update_flag {
>> +	UPDATE_FLAG_PROPERTITY = 0,
>> +	UPDATE_FLAG_CU_MASK,
>> +};
>> +
>> +struct queue_update_info {
>> +	union {
>> +		struct queue_properties properties;
>> +		struct {
>> +			uint32_t count; /* Must be a multiple of 32 */
>> +			uint32_t *ptr;
>> +		} cu_mask;
>> +	};
>> +
>> +	enum queue_update_flag update_flag;
>> +};
>> +
>
>This doesn't make sense to me. As I understand it, queue_update_info is for
>information that is not stored in queue_properties but only in the MQDs.
>Therefore, it should not include the queue_properties.
>
>All the low level functions in the MQD managers get both the queue_properties
>and the queue_update_info. So trying to wrap both in the same union doesn't
>make sense there either.
>
>I think you only need this because you tried to generalize pqm_update_queue to
>handle both updates to queue_properties and CU mask updates with a single
>argument. IMO this does not make the interface any clearer. I think it would be
>more straight-forward to keep a separate pqm_set_cu_mask function that takes
>a queue_update_info parameter. If you're looking for more generic names, I
>suggest the following:
>
>  * Rename pqm_update_queue to pqm_update_queue_properties
>  * Rename struct queue_update_info to struct mqd_update_info
>  * Rename pqm_set_cu_mask to pqm_update_mqd. For now this is only used
>    for CU mask (the union has only one struct member for now). It may
>    be used for other MQD properties that don't need to be stored in
>    queue_properties in the future

Got it. Thanks for your suggestions!

Regards,
Lang

>
>Regards,
>  Felix
>