Re: [PATCH 3/3] drm/amdkfd: Consolidate MQD manager functions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Am 2022-02-07 um 12:50 schrieb Joshi, Mukul:
[AMD Official Use Only]



-----Original Message-----
From: Kuehling, Felix <Felix.Kuehling@xxxxxxx>
Sent: Monday, February 7, 2022 10:43 AM
To: Joshi, Mukul <Mukul.Joshi@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Subject: Re: [PATCH 3/3] drm/amdkfd: Consolidate MQD manager functions


Am 2022-02-04 um 18:45 schrieb Mukul Joshi:
A few MQD manager functions are duplicated for all versions of MQD
manager. Remove this duplication by moving the common functions into
kfd_mqd_manager.c file.

Signed-off-by: Mukul Joshi <mukul.joshi@xxxxxxx>
---
   drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c  | 63
+++++++++++++++++
   drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h  | 27 ++++++++
   .../gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c  | 54 ---------------
   .../gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c  | 61 -----------------
   .../gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c   | 68 -------------------
   .../gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c   | 53 ---------------
   6 files changed, 90 insertions(+), 236 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
index e2825ad4d699..f4a6af98db2d 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
@@ -173,3 +173,66 @@ void mqd_symmetrically_map_cu_mask(struct
mqd_manager *mm,
   		}
   	}
   }
+
+int hiq_load_mqd_kiq(struct mqd_manager *mm, void *mqd,
+		     uint32_t pipe_id, uint32_t queue_id,
+		     struct queue_properties *p, struct mm_struct *mms)
Since these functions are no longer static, they should get an appropriate name
prefix to avoid future namespace collisions. Just a kfd_ prefix will do.

I think there are existing functions in this file that could use the same treatment
(in a separate patch).


+{
+	return mm->dev->kfd2kgd->hiq_mqd_load(mm->dev->adev, mqd,
pipe_id,
+					      queue_id, p->doorbell_off);
+}
+
+int destroy_mqd(struct mqd_manager *mm, void *mqd,
+		enum kfd_preempt_type type, unsigned int timeout,
+		uint32_t pipe_id,uint32_t queue_id)
This function is only applicable to CP queues. Therefore I'd give it a
more specific name, e.g. kfd_destroy_cp_mqd. Similar for the other
non-SDMA functions below.

We define destroy_hqd for HIQ (and DIQ ) also. Same for free_mqd and other functions.
I guess that’s why we have _sdma for SDMA queues, and the rest use a generic name.
Maybe leaving it without '_cp' is better here. What do you think?

I still think calling out the queue type in the function name makes sense, if the function is not applicable to all queue types. In some cases there is overlap. For example HIQ and DIQ would use destroy_hqd_cp. But HIQ uses free_mqd_hiq_sdma.

I guess it still leaves some ambiguity because CP is used as superset of CP, HIQ and DIQ in some cases, but only CP compute queues in others. If you want, you could use "CP" to mean the superset and "compute" for CP compute queues only.

Regards,
  Felix



Regards,
Mukul

Regards,
    Felix


+{
+	return mm->dev->kfd2kgd->hqd_destroy(mm->dev->adev, mqd, type,
timeout,
+						pipe_id, queue_id);
+}
+
+void free_mqd(struct mqd_manager *mm, void *mqd,
+	      struct kfd_mem_obj *mqd_mem_obj)
+{
+	if (mqd_mem_obj->gtt_mem) {
+		amdgpu_amdkfd_free_gtt_mem(mm->dev->adev,
mqd_mem_obj->gtt_mem);
+		kfree(mqd_mem_obj);
+	} else {
+		kfd_gtt_sa_free(mm->dev, mqd_mem_obj);
+	}
+}
+
+bool is_occupied(struct mqd_manager *mm, void *mqd,
+		 uint64_t queue_address, uint32_t pipe_id,
+		 uint32_t queue_id)
+{
+	return mm->dev->kfd2kgd->hqd_is_occupied(mm->dev->adev,
queue_address,
+						pipe_id, queue_id);
+}
+
+int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
+		  uint32_t pipe_id, uint32_t queue_id,
+		  struct queue_properties *p, struct mm_struct *mms)
+{
+	return mm->dev->kfd2kgd->hqd_sdma_load(mm->dev->adev, mqd,
+						(uint32_t __user *)p-
write_ptr,
+						mms);
+}
+
+/*
+ * preempt type here is ignored because there is only one way
+ * to preempt sdma queue
+ */
+int destroy_mqd_sdma(struct mqd_manager *mm, void *mqd,
+		     enum kfd_preempt_type type,
+		     unsigned int timeout, uint32_t pipe_id,
+		     uint32_t queue_id)
+{
+	return mm->dev->kfd2kgd->hqd_sdma_destroy(mm->dev->adev, mqd,
timeout);
+}
+
+bool is_occupied_sdma(struct mqd_manager *mm, void *mqd,
+		      uint64_t queue_address, uint32_t pipe_id,
+		      uint32_t queue_id)
+{
+	return mm->dev->kfd2kgd->hqd_sdma_is_occupied(mm->dev->adev,
mqd);
+}
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
index 23486a23df84..76f20637b938 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
@@ -136,4 +136,31 @@ void mqd_symmetrically_map_cu_mask(struct
mqd_manager *mm,
   		const uint32_t *cu_mask, uint32_t cu_mask_count,
   		uint32_t *se_mask);

+int hiq_load_mqd_kiq(struct mqd_manager *mm, void *mqd,
+		uint32_t pipe_id, uint32_t queue_id,
+		struct queue_properties *p, struct mm_struct *mms);
+
+int destroy_mqd(struct mqd_manager *mm, void *mqd,
+		enum kfd_preempt_type type, unsigned int timeout,
+		uint32_t pipe_id,uint32_t queue_id);
+
+void free_mqd(struct mqd_manager *mm, void *mqd,
+		struct kfd_mem_obj *mqd_mem_obj);
+
+bool is_occupied(struct mqd_manager *mm, void *mqd,
+		 uint64_t queue_address, uint32_t pipe_id,
+		 uint32_t queue_id);
+
+int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
+		uint32_t pipe_id, uint32_t queue_id,
+		struct queue_properties *p, struct mm_struct *mms);
+
+int destroy_mqd_sdma(struct mqd_manager *mm, void *mqd,
+		enum kfd_preempt_type type,unsigned int timeout,
+		uint32_t pipe_id, uint32_t queue_id);
+
+bool is_occupied_sdma(struct mqd_manager *mm, void *mqd,
+		uint64_t queue_address, uint32_t pipe_id,
+		uint32_t queue_id);
+
   #endif /* KFD_MQD_MANAGER_H_ */
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 96e3303fa27c..81b6b3d5f2e7 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c
@@ -156,13 +156,6 @@ static void init_mqd_sdma(struct mqd_manager
*mm, void **mqd,
   	mm->update_mqd(mm, m, q, NULL);
   }

-static void free_mqd(struct mqd_manager *mm, void *mqd,
-			struct kfd_mem_obj *mqd_mem_obj)
-{
-	kfd_gtt_sa_free(mm->dev, mqd_mem_obj);
-}
-
-
   static int load_mqd(struct mqd_manager *mm, void *mqd, uint32_t pipe_id,
   		    uint32_t queue_id, struct queue_properties *p,
   		    struct mm_struct *mms)
@@ -176,15 +169,6 @@ static int load_mqd(struct mqd_manager *mm, void
*mqd, uint32_t pipe_id,
   					  wptr_shift, wptr_mask, mms);
   }

-static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
-			 uint32_t pipe_id, uint32_t queue_id,
-			 struct queue_properties *p, struct mm_struct *mms)
-{
-	return mm->dev->kfd2kgd->hqd_sdma_load(mm->dev->adev, mqd,
-					       (uint32_t __user *)p->write_ptr,
-					       mms);
-}
-
   static void __update_mqd(struct mqd_manager *mm, void *mqd,
   			struct queue_properties *q, struct mqd_update_info
*minfo,
   			unsigned int atc_bit)
@@ -271,15 +255,6 @@ static void update_mqd_sdma(struct mqd_manager
*mm, void *mqd,
   	q->is_active = QUEUE_IS_ACTIVE(*q);
   }

-static int destroy_mqd(struct mqd_manager *mm, void *mqd,
-			enum kfd_preempt_type type,
-			unsigned int timeout, uint32_t pipe_id,
-			uint32_t queue_id)
-{
-	return mm->dev->kfd2kgd->hqd_destroy(mm->dev->adev, mqd, type,
timeout,
-					pipe_id, queue_id);
-}
-
   static void checkpoint_mqd(struct mqd_manager *mm, void *mqd, void
*mqd_dst, void *ctl_stack_dst)
   {
   	struct cik_mqd *m;
@@ -351,35 +326,6 @@ static void restore_mqd_sdma(struct mqd_manager
*mm, void **mqd,
   	qp->is_active = 0;
   }

-/*
- * preempt type here is ignored because there is only one way
- * to preempt sdma queue
- */
-static int destroy_mqd_sdma(struct mqd_manager *mm, void *mqd,
-				enum kfd_preempt_type type,
-				unsigned int timeout, uint32_t pipe_id,
-				uint32_t queue_id)
-{
-	return mm->dev->kfd2kgd->hqd_sdma_destroy(mm->dev->adev, mqd,
timeout);
-}
-
-static bool is_occupied(struct mqd_manager *mm, void *mqd,
-			uint64_t queue_address,	uint32_t pipe_id,
-			uint32_t queue_id)
-{
-
-	return mm->dev->kfd2kgd->hqd_is_occupied(mm->dev->adev,
queue_address,
-					pipe_id, queue_id);
-
-}
-
-static bool is_occupied_sdma(struct mqd_manager *mm, void *mqd,
-			uint64_t queue_address,	uint32_t pipe_id,
-			uint32_t queue_id)
-{
-	return mm->dev->kfd2kgd->hqd_sdma_is_occupied(mm->dev->adev,
mqd);
-}
-
   /*
    * HIQ MQD Implementation, concrete implementation for HIQ MQD
implementation.
    * The HIQ queue in Kaveri is using the same MQD structure as all the user
mode
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 0cc8679c24fa..8324757a1cf5 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
@@ -154,14 +154,6 @@ static int load_mqd(struct mqd_manager *mm, void
*mqd,
   	return r;
   }

-static int hiq_load_mqd_kiq(struct mqd_manager *mm, void *mqd,
-			    uint32_t pipe_id, uint32_t queue_id,
-			    struct queue_properties *p, struct mm_struct *mms)
-{
-	return mm->dev->kfd2kgd->hiq_mqd_load(mm->dev->adev, mqd,
pipe_id,
-					      queue_id, p->doorbell_off);
-}
-
   static void update_mqd(struct mqd_manager *mm, void *mqd,
   			struct queue_properties *q,
   			struct mqd_update_info *minfo)
@@ -233,31 +225,6 @@ static uint32_t read_doorbell_id(void *mqd)
   	return m->queue_doorbell_id0;
   }

-static int destroy_mqd(struct mqd_manager *mm, void *mqd,
-		       enum kfd_preempt_type type,
-		       unsigned int timeout, uint32_t pipe_id,
-		       uint32_t queue_id)
-{
-	return mm->dev->kfd2kgd->hqd_destroy
-		(mm->dev->adev, mqd, type, timeout,
-		 pipe_id, queue_id);
-}
-
-static void free_mqd(struct mqd_manager *mm, void *mqd,
-			struct kfd_mem_obj *mqd_mem_obj)
-{
-	kfd_gtt_sa_free(mm->dev, mqd_mem_obj);
-}
-
-static bool is_occupied(struct mqd_manager *mm, void *mqd,
-			uint64_t queue_address,	uint32_t pipe_id,
-			uint32_t queue_id)
-{
-	return mm->dev->kfd2kgd->hqd_is_occupied(
-		mm->dev->adev, queue_address,
-		pipe_id, queue_id);
-}
-
   static int get_wave_state(struct mqd_manager *mm, void *mqd,
   			  void __user *ctl_stack,
   			  u32 *ctl_stack_used_size,
@@ -352,15 +319,6 @@ static void init_mqd_sdma(struct mqd_manager
*mm, void **mqd,
   	mm->update_mqd(mm, m, q, NULL);
   }

-static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
-		uint32_t pipe_id, uint32_t queue_id,
-		struct queue_properties *p, struct mm_struct *mms)
-{
-	return mm->dev->kfd2kgd->hqd_sdma_load(mm->dev->adev, mqd,
-					       (uint32_t __user *)p->write_ptr,
-					       mms);
-}
-
   #define SDMA_RLC_DUMMY_DEFAULT 0xf

   static void update_mqd_sdma(struct mqd_manager *mm, void *mqd,
@@ -390,25 +348,6 @@ static void update_mqd_sdma(struct mqd_manager
*mm, void *mqd,
   	q->is_active = QUEUE_IS_ACTIVE(*q);
   }

-/*
- *  * preempt type here is ignored because there is only one way
- *  * to preempt sdma queue
- */
-static int destroy_mqd_sdma(struct mqd_manager *mm, void *mqd,
-		enum kfd_preempt_type type,
-		unsigned int timeout, uint32_t pipe_id,
-		uint32_t queue_id)
-{
-	return mm->dev->kfd2kgd->hqd_sdma_destroy(mm->dev->adev, mqd,
timeout);
-}
-
-static bool is_occupied_sdma(struct mqd_manager *mm, void *mqd,
-		uint64_t queue_address, uint32_t pipe_id,
-		uint32_t queue_id)
-{
-	return mm->dev->kfd2kgd->hqd_sdma_is_occupied(mm->dev->adev,
mqd);
-}
-
   static void checkpoint_mqd_sdma(struct mqd_manager *mm,
   				void *mqd,
   				void *mqd_dst,
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 87da4329dbf2..007886b2961e 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
@@ -204,14 +204,6 @@ static int load_mqd(struct mqd_manager *mm, void
*mqd,
   					  wptr_shift, 0, mms);
   }

-static int hiq_load_mqd_kiq(struct mqd_manager *mm, void *mqd,
-			    uint32_t pipe_id, uint32_t queue_id,
-			    struct queue_properties *p, struct mm_struct *mms)
-{
-	return mm->dev->kfd2kgd->hiq_mqd_load(mm->dev->adev, mqd,
pipe_id,
-					      queue_id, p->doorbell_off);
-}
-
   static void update_mqd(struct mqd_manager *mm, void *mqd,
   			struct queue_properties *q,
   			struct mqd_update_info *minfo)
@@ -285,38 +277,6 @@ static uint32_t read_doorbell_id(void *mqd)
   	return m->queue_doorbell_id0;
   }

-static int destroy_mqd(struct mqd_manager *mm, void *mqd,
-			enum kfd_preempt_type type,
-			unsigned int timeout, uint32_t pipe_id,
-			uint32_t queue_id)
-{
-	return mm->dev->kfd2kgd->hqd_destroy
-		(mm->dev->adev, mqd, type, timeout,
-		pipe_id, queue_id);
-}
-
-static void free_mqd(struct mqd_manager *mm, void *mqd,
-			struct kfd_mem_obj *mqd_mem_obj)
-{
-	struct kfd_dev *kfd = mm->dev;
-
-	if (mqd_mem_obj->gtt_mem) {
-		amdgpu_amdkfd_free_gtt_mem(kfd->adev, mqd_mem_obj-
gtt_mem);
-		kfree(mqd_mem_obj);
-	} else {
-		kfd_gtt_sa_free(mm->dev, mqd_mem_obj);
-	}
-}
-
-static bool is_occupied(struct mqd_manager *mm, void *mqd,
-			uint64_t queue_address,	uint32_t pipe_id,
-			uint32_t queue_id)
-{
-	return mm->dev->kfd2kgd->hqd_is_occupied(
-		mm->dev->adev, queue_address,
-		pipe_id, queue_id);
-}
-
   static int get_wave_state(struct mqd_manager *mm, void *mqd,
   			  void __user *ctl_stack,
   			  u32 *ctl_stack_used_size,
@@ -422,15 +382,6 @@ static void init_mqd_sdma(struct mqd_manager
*mm, void **mqd,
   	mm->update_mqd(mm, m, q, NULL);
   }

-static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
-		uint32_t pipe_id, uint32_t queue_id,
-		struct queue_properties *p, struct mm_struct *mms)
-{
-	return mm->dev->kfd2kgd->hqd_sdma_load(mm->dev->adev, mqd,
-					       (uint32_t __user *)p->write_ptr,
-					       mms);
-}
-
   #define SDMA_RLC_DUMMY_DEFAULT 0xf

   static void update_mqd_sdma(struct mqd_manager *mm, void *mqd,
@@ -460,25 +411,6 @@ static void update_mqd_sdma(struct mqd_manager
*mm, void *mqd,
   	q->is_active = QUEUE_IS_ACTIVE(*q);
   }

-/*
- *  * preempt type here is ignored because there is only one way
- *  * to preempt sdma queue
- */
-static int destroy_mqd_sdma(struct mqd_manager *mm, void *mqd,
-		enum kfd_preempt_type type,
-		unsigned int timeout, uint32_t pipe_id,
-		uint32_t queue_id)
-{
-	return mm->dev->kfd2kgd->hqd_sdma_destroy(mm->dev->adev, mqd,
timeout);
-}
-
-static bool is_occupied_sdma(struct mqd_manager *mm, void *mqd,
-		uint64_t queue_address, uint32_t pipe_id,
-		uint32_t queue_id)
-{
-	return mm->dev->kfd2kgd->hqd_sdma_is_occupied(mm->dev->adev,
mqd);
-}
-
   static void checkpoint_mqd_sdma(struct mqd_manager *mm,
   				void *mqd,
   				void *mqd_dst,
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 137b208135a0..c0405bbe8e36 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c
@@ -259,31 +259,6 @@ static void update_mqd_tonga(struct mqd_manager
*mm, void *mqd,
   	__update_mqd(mm, mqd, q, minfo, MTYPE_UC, 0);
   }

-static int destroy_mqd(struct mqd_manager *mm, void *mqd,
-			enum kfd_preempt_type type,
-			unsigned int timeout, uint32_t pipe_id,
-			uint32_t queue_id)
-{
-	return mm->dev->kfd2kgd->hqd_destroy
-		(mm->dev->adev, mqd, type, timeout,
-		pipe_id, queue_id);
-}
-
-static void free_mqd(struct mqd_manager *mm, void *mqd,
-			struct kfd_mem_obj *mqd_mem_obj)
-{
-	kfd_gtt_sa_free(mm->dev, mqd_mem_obj);
-}
-
-static bool is_occupied(struct mqd_manager *mm, void *mqd,
-			uint64_t queue_address,	uint32_t pipe_id,
-			uint32_t queue_id)
-{
-	return mm->dev->kfd2kgd->hqd_is_occupied(
-		mm->dev->adev, queue_address,
-		pipe_id, queue_id);
-}
-
   static int get_wave_state(struct mqd_manager *mm, void *mqd,
   			  void __user *ctl_stack,
   			  u32 *ctl_stack_used_size,
@@ -385,15 +360,6 @@ static void init_mqd_sdma(struct mqd_manager
*mm, void **mqd,
   	mm->update_mqd(mm, m, q, NULL);
   }

-static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
-		uint32_t pipe_id, uint32_t queue_id,
-		struct queue_properties *p, struct mm_struct *mms)
-{
-	return mm->dev->kfd2kgd->hqd_sdma_load(mm->dev->adev, mqd,
-					       (uint32_t __user *)p->write_ptr,
-					       mms);
-}
-
   static void update_mqd_sdma(struct mqd_manager *mm, void *mqd,
   			struct queue_properties *q,
   			struct mqd_update_info *minfo)
@@ -422,25 +388,6 @@ static void update_mqd_sdma(struct mqd_manager
*mm, void *mqd,
   	q->is_active = QUEUE_IS_ACTIVE(*q);
   }

-/*
- *  * preempt type here is ignored because there is only one way
- *  * to preempt sdma queue
- */
-static int destroy_mqd_sdma(struct mqd_manager *mm, void *mqd,
-		enum kfd_preempt_type type,
-		unsigned int timeout, uint32_t pipe_id,
-		uint32_t queue_id)
-{
-	return mm->dev->kfd2kgd->hqd_sdma_destroy(mm->dev->adev, mqd,
timeout);
-}
-
-static bool is_occupied_sdma(struct mqd_manager *mm, void *mqd,
-		uint64_t queue_address, uint32_t pipe_id,
-		uint32_t queue_id)
-{
-	return mm->dev->kfd2kgd->hqd_sdma_is_occupied(mm->dev->adev,
mqd);
-}
-
   static void checkpoint_mqd_sdma(struct mqd_manager *mm,
   				void *mqd,
   				void *mqd_dst,



[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux