All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] drm/amdkfd: Use asic specific fn to configure grace period
@ 2025-01-14 19:52 Elena Sakhnovitch
  2025-01-14 19:52 ` [PATCH 2/3] drm/amdgpu: Don't modify grace_period in helper function Elena Sakhnovitch
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Elena Sakhnovitch @ 2025-01-14 19:52 UTC (permalink / raw)
  To: amd-gfx; +Cc: elena.sakhnovitch, Harish Kasiviswanathan, Elena Sakhnovitch

From: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com>

Currently, grace period is modified only for gfx943 APU. In the future
this might need to be set for other ASICs too. Either ways, asic
specific values should be handled by asic specific functions.

Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com>
Signed-off-by: Elena Sakhnovitch <Elena.Sakhnovitch@amd.com>
---
 .../drm/amd/amdkfd/kfd_device_queue_manager.c | 24 ++++++-------------
 .../drm/amd/amdkfd/kfd_device_queue_manager.h |  3 ++-
 .../gpu/drm/amd/amdkfd/kfd_packet_manager.c   |  3 +++
 .../drm/amd/amdkfd/kfd_packet_manager_v9.c    | 10 ++++++++
 4 files changed, 22 insertions(+), 18 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 f157494bfdb1..4369308a74e7 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
@@ -1859,26 +1859,16 @@ static int start_cpsch(struct device_queue_manager *dqm)
 	/* clear hang status when driver try to start the hw scheduler */
 	dqm->sched_running = true;
 
-	if (!dqm->dev->kfd->shared_resources.enable_mes)
+	if (!dqm->dev->kfd->shared_resources.enable_mes) {
 		execute_queues_cpsch(dqm, KFD_UNMAP_QUEUES_FILTER_DYNAMIC_QUEUES, 0, USE_DEFAULT_GRACE_PERIOD);
-
-	/* Set CWSR grace period to 1x1000 cycle for GFX9.4.3 APU */
-	if (amdgpu_emu_mode == 0 && dqm->dev->adev->gmc.is_app_apu &&
-	    (KFD_GC_VERSION(dqm->dev) == IP_VERSION(9, 4, 3))) {
-		uint32_t reg_offset = 0;
-		uint32_t grace_period = 1;
-
-		retval = pm_update_grace_period(&dqm->packet_mgr,
-						grace_period);
+		retval = pm_update_grace_period(&dqm->packet_mgr, SET_ASIC_OPTIMIZED_GRACE_PERIOD);
 		if (retval)
-			dev_err(dev, "Setting grace timeout failed\n");
-		else if (dqm->dev->kfd2kgd->build_grace_period_packet_info)
-			/* Update dqm->wait_times maintained in software */
-			dqm->dev->kfd2kgd->build_grace_period_packet_info(
-					dqm->dev->adev,	dqm->wait_times,
-					grace_period, &reg_offset,
-					&dqm->wait_times);
+			dev_err(dev, "Setting optimized grace timeout failed\n");
 	}
+	if (dqm->dev->kfd2kgd->get_iq_wait_times)
+		dqm->dev->kfd2kgd->get_iq_wait_times(dqm->dev->adev,
+					&dqm->wait_times,
+					ffs(dqm->dev->xcc_mask) - 1);
 
 	/* setup per-queue reset detection buffer  */
 	num_hw_queue_slots =  dqm->dev->kfd->shared_resources.num_queue_per_pipe *
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 09ab36f8e8c6..fb3419993612 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
@@ -37,7 +37,8 @@
 
 #define KFD_MES_PROCESS_QUANTUM		100000
 #define KFD_MES_GANG_QUANTUM		10000
-#define USE_DEFAULT_GRACE_PERIOD 0xffffffff
+#define USE_DEFAULT_GRACE_PERIOD	0xffffffff
+#define SET_ASIC_OPTIMIZED_GRACE_PERIOD	0xfffffffe
 
 struct device_process_node {
 	struct qcm_process_device *qpd;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager.c
index 4984b41cd372..518c6ec23a75 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager.c
@@ -403,6 +403,9 @@ int pm_update_grace_period(struct packet_manager *pm, uint32_t grace_period)
 	int retval = 0;
 	uint32_t *buffer, size;
 
+	if (!pm->pmf->set_grace_period || !pm->pmf->set_grace_period_size)
+		return 0;
+
 	size = pm->pmf->set_grace_period_size;
 
 	mutex_lock(&pm->lock);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
index d56525201155..fde212242129 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
@@ -302,9 +302,19 @@ static int pm_set_grace_period_v9(struct packet_manager *pm,
 		uint32_t grace_period)
 {
 	struct pm4_mec_write_data_mmio *packet;
+	struct device_queue_manager *dqm = pm->dqm;
 	uint32_t reg_offset = 0;
 	uint32_t reg_data = 0;
 
+	if (grace_period == SET_ASIC_OPTIMIZED_GRACE_PERIOD) {
+		/* Set CWSR grace period to 1x1000 cycle for GFX9.4.3 APU */
+		if (amdgpu_emu_mode == 0 && dqm->dev->adev->gmc.is_app_apu &&
+		    KFD_GC_VERSION(dqm->dev) == IP_VERSION(9, 4, 3))
+			grace_period = 1;
+		else
+			return 0;
+	}
+
 	pm->dqm->dev->kfd2kgd->build_grace_period_packet_info(
 			pm->dqm->dev->adev,
 			pm->dqm->wait_times,
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/3] drm/amdgpu: Don't modify grace_period in helper function
  2025-01-14 19:52 [PATCH 1/3] drm/amdkfd: Use asic specific fn to configure grace period Elena Sakhnovitch
@ 2025-01-14 19:52 ` Elena Sakhnovitch
  2025-02-06 21:57   ` Chen, Xiaogang
  2025-01-14 19:52 ` [PATCH 3/3] drm/amdgpu: Set lower queue retry timeout for gfx9 family Elena Sakhnovitch
  2025-02-06 21:56 ` [PATCH 1/3] drm/amdkfd: Use asic specific fn to configure grace period Chen, Xiaogang
  2 siblings, 1 reply; 8+ messages in thread
From: Elena Sakhnovitch @ 2025-01-14 19:52 UTC (permalink / raw)
  To: amd-gfx; +Cc: elena.sakhnovitch, Harish Kasiviswanathan, Elena Sakhnovitch

From: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com>

build_grace_period_packet_info is asic helper function that fetches the
correct format. It is the responsibility of the caller to validate the
value.

Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com>
Signed-off-by: Elena Sakhnovitch <Elena.Sakhnovitch@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c | 18 ++++++------------
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c  | 17 ++++++-----------
 .../gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c | 12 ++++++++++++
 3 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
index 62176d607bef..8e72dcff8867 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
@@ -1029,18 +1029,12 @@ void kgd_gfx_v10_build_grace_period_packet_info(struct amdgpu_device *adev,
 {
 	*reg_data = wait_times;
 
-	/*
-	 * The CP cannont handle a 0 grace period input and will result in
-	 * an infinite grace period being set so set to 1 to prevent this.
-	 */
-	if (grace_period == 0)
-		grace_period = 1;
-
-	*reg_data = REG_SET_FIELD(*reg_data,
-			CP_IQ_WAIT_TIME2,
-			SCH_WAVE,
-			grace_period);
-
+	if (grace_period) {
+		*reg_data = REG_SET_FIELD(*reg_data,
+				CP_IQ_WAIT_TIME2,
+				SCH_WAVE,
+				grace_period);
+	}
 	*reg_offset = SOC15_REG_OFFSET(GC, 0, mmCP_IQ_WAIT_TIME2);
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
index 441568163e20..04c86a229a23 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
@@ -1085,17 +1085,12 @@ void kgd_gfx_v9_build_grace_period_packet_info(struct amdgpu_device *adev,
 {
 	*reg_data = wait_times;
 
-	/*
-	 * The CP cannot handle a 0 grace period input and will result in
-	 * an infinite grace period being set so set to 1 to prevent this.
-	 */
-	if (grace_period == 0)
-		grace_period = 1;
-
-	*reg_data = REG_SET_FIELD(*reg_data,
-			CP_IQ_WAIT_TIME2,
-			SCH_WAVE,
-			grace_period);
+	if (grace_period) {
+		*reg_data = REG_SET_FIELD(*reg_data,
+				CP_IQ_WAIT_TIME2,
+				SCH_WAVE,
+				grace_period);
+	}
 
 	*reg_offset = SOC15_REG_OFFSET(GC, 0, mmCP_IQ_WAIT_TIME2);
 }
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
index fde212242129..adc7f7c78a18 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
@@ -306,6 +306,18 @@ static int pm_set_grace_period_v9(struct packet_manager *pm,
 	uint32_t reg_offset = 0;
 	uint32_t reg_data = 0;
 
+	/*
+	 * The CP cannot handle a 0 grace period input and will result in
+	 * an infinite grace period being set so set to 1 to prevent this.
+	 */
+	if (!grace_period) {
+		pr_debug("Invalid grace_period. Setting default value 0x%x\n",
+			 pm->dqm->wait_times);
+		if (WARN_ON((pm->dqm->wait_times & CP_IQ_WAIT_TIME2__SCH_WAVE_MASK)
+			== 0))
+			return -EINVAL;
+	}
+
 	if (grace_period == SET_ASIC_OPTIMIZED_GRACE_PERIOD) {
 		/* Set CWSR grace period to 1x1000 cycle for GFX9.4.3 APU */
 		if (amdgpu_emu_mode == 0 && dqm->dev->adev->gmc.is_app_apu &&
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/3] drm/amdgpu: Set lower queue retry timeout for gfx9 family
  2025-01-14 19:52 [PATCH 1/3] drm/amdkfd: Use asic specific fn to configure grace period Elena Sakhnovitch
  2025-01-14 19:52 ` [PATCH 2/3] drm/amdgpu: Don't modify grace_period in helper function Elena Sakhnovitch
@ 2025-01-14 19:52 ` Elena Sakhnovitch
  2025-02-06 22:27   ` Russell, Kent
  2025-02-06 21:56 ` [PATCH 1/3] drm/amdkfd: Use asic specific fn to configure grace period Chen, Xiaogang
  2 siblings, 1 reply; 8+ messages in thread
From: Elena Sakhnovitch @ 2025-01-14 19:52 UTC (permalink / raw)
  To: amd-gfx; +Cc: elena.sakhnovitch, Harish Kasiviswanathan, Elena Sakhnovitch

From: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com>

Set more optimized queue retry timeout for gfx9 family starting with
arcturus.

Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com>
Signed-off-by: Elena Sakhnovitch <Elena.Sakhnovitch@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.h |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c  |  8 ++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.h  |  1 +
 drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c | 14 +++++++++++++-
 drivers/gpu/drm/amd/include/kgd_kfd_interface.h    |  1 +
 6 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
index 8e72dcff8867..652c695d04e2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
@@ -1024,6 +1024,7 @@ void kgd_gfx_v10_get_iq_wait_times(struct amdgpu_device *adev,
 void kgd_gfx_v10_build_grace_period_packet_info(struct amdgpu_device *adev,
 						uint32_t wait_times,
 						uint32_t grace_period,
+						uint32_t que_sleep,
 						uint32_t *reg_offset,
 						uint32_t *reg_data)
 {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.h
index 9efd2dd4fdd7..11aedaa8a0b9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.h
@@ -54,6 +54,7 @@ void kgd_gfx_v10_get_iq_wait_times(struct amdgpu_device *adev,
 void kgd_gfx_v10_build_grace_period_packet_info(struct amdgpu_device *adev,
 					       uint32_t wait_times,
 					       uint32_t grace_period,
+					       uint32_t que_sleep,
 					       uint32_t *reg_offset,
 					       uint32_t *reg_data);
 uint64_t kgd_gfx_v10_hqd_get_pq_addr(struct amdgpu_device *adev,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
index 04c86a229a23..d93a0285f225 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
@@ -1080,6 +1080,7 @@ void kgd_gfx_v9_get_cu_occupancy(struct amdgpu_device *adev,
 void kgd_gfx_v9_build_grace_period_packet_info(struct amdgpu_device *adev,
 		uint32_t wait_times,
 		uint32_t grace_period,
+		uint32_t que_sleep,
 		uint32_t *reg_offset,
 		uint32_t *reg_data)
 {
@@ -1092,6 +1093,13 @@ void kgd_gfx_v9_build_grace_period_packet_info(struct amdgpu_device *adev,
 				grace_period);
 	}
 
+	if (que_sleep) {
+		*reg_data = REG_SET_FIELD(*reg_data,
+				CP_IQ_WAIT_TIME2,
+				QUE_SLEEP,
+				que_sleep);
+	}
+
 	*reg_offset = SOC15_REG_OFFSET(GC, 0, mmCP_IQ_WAIT_TIME2);
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.h
index b6a91a552aa4..3f159d477f5b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.h
@@ -100,6 +100,7 @@ void kgd_gfx_v9_get_iq_wait_times(struct amdgpu_device *adev,
 void kgd_gfx_v9_build_grace_period_packet_info(struct amdgpu_device *adev,
 					       uint32_t wait_times,
 					       uint32_t grace_period,
+					       uint32_t que_sleep,
 					       uint32_t *reg_offset,
 					       uint32_t *reg_data);
 uint64_t kgd_gfx_v9_hqd_get_pq_addr(struct amdgpu_device *adev,
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
index adc7f7c78a18..4de8106d14ba 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
@@ -305,6 +305,7 @@ static int pm_set_grace_period_v9(struct packet_manager *pm,
 	struct device_queue_manager *dqm = pm->dqm;
 	uint32_t reg_offset = 0;
 	uint32_t reg_data = 0;
+	uint32_t que_sleep = 0;
 
 	/*
 	 * The CP cannot handle a 0 grace period input and will result in
@@ -319,18 +320,29 @@ static int pm_set_grace_period_v9(struct packet_manager *pm,
 	}
 
 	if (grace_period == SET_ASIC_OPTIMIZED_GRACE_PERIOD) {
+		/* Reduce CP_IQ_WAIT_TIME2.QUE_SLEEP to 0x1 from default 0x40.
+		 * On a 1GHz machine this is roughly 1 microsecond, which is
+		 * about how long it takes to load data out of memory during
+		 * queue connect
+		 * QUE_SLEEP: Wait Count for Dequeue Retry.
+		 */
+		if (KFD_GC_VERSION(dqm->dev) >= IP_VERSION(9, 4, 1) &&
+		    KFD_GC_VERSION(dqm->dev) < IP_VERSION(10, 0, 0))
+			que_sleep = 1;
+
 		/* Set CWSR grace period to 1x1000 cycle for GFX9.4.3 APU */
 		if (amdgpu_emu_mode == 0 && dqm->dev->adev->gmc.is_app_apu &&
 		    KFD_GC_VERSION(dqm->dev) == IP_VERSION(9, 4, 3))
 			grace_period = 1;
 		else
-			return 0;
+			grace_period = 0; /* 0 will keep the default value */
 	}
 
 	pm->dqm->dev->kfd2kgd->build_grace_period_packet_info(
 			pm->dqm->dev->adev,
 			pm->dqm->wait_times,
 			grace_period,
+			que_sleep,
 			&reg_offset,
 			&reg_data);
 
diff --git a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
index e3e635a31b8a..1ed3fbedf50b 100644
--- a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
+++ b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
@@ -316,6 +316,7 @@ struct kfd2kgd_calls {
 	void (*build_grace_period_packet_info)(struct amdgpu_device *adev,
 			uint32_t wait_times,
 			uint32_t grace_period,
+			uint32_t que_sleep,
 			uint32_t *reg_offset,
 			uint32_t *reg_data);
 	void (*get_cu_occupancy)(struct amdgpu_device *adev,
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] drm/amdkfd: Use asic specific fn to configure grace period
  2025-01-14 19:52 [PATCH 1/3] drm/amdkfd: Use asic specific fn to configure grace period Elena Sakhnovitch
  2025-01-14 19:52 ` [PATCH 2/3] drm/amdgpu: Don't modify grace_period in helper function Elena Sakhnovitch
  2025-01-14 19:52 ` [PATCH 3/3] drm/amdgpu: Set lower queue retry timeout for gfx9 family Elena Sakhnovitch
@ 2025-02-06 21:56 ` Chen, Xiaogang
  2 siblings, 0 replies; 8+ messages in thread
From: Chen, Xiaogang @ 2025-02-06 21:56 UTC (permalink / raw)
  To: Elena Sakhnovitch, amd-gfx; +Cc: Harish Kasiviswanathan

[-- Attachment #1: Type: text/plain, Size: 5332 bytes --]


On 1/14/2025 1:52 PM, Elena Sakhnovitch wrote:
> From: Harish Kasiviswanathan<Harish.Kasiviswanathan@amd.com>
>
> Currently, grace period is modified only for gfx943 APU. In the future
> this might need to be set for other ASICs too. Either ways, asic
> specific values should be handled by asic specific functions.
>
> Signed-off-by: Harish Kasiviswanathan<Harish.Kasiviswanathan@amd.com>
> Signed-off-by: Elena Sakhnovitch<Elena.Sakhnovitch@amd.com>
> ---
>   .../drm/amd/amdkfd/kfd_device_queue_manager.c | 24 ++++++-------------
>   .../drm/amd/amdkfd/kfd_device_queue_manager.h |  3 ++-
>   .../gpu/drm/amd/amdkfd/kfd_packet_manager.c   |  3 +++
>   .../drm/amd/amdkfd/kfd_packet_manager_v9.c    | 10 ++++++++
>   4 files changed, 22 insertions(+), 18 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 f157494bfdb1..4369308a74e7 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -1859,26 +1859,16 @@ static int start_cpsch(struct device_queue_manager *dqm)
>   	/* clear hang status when driver try to start the hw scheduler */
>   	dqm->sched_running = true;
>   
> -	if (!dqm->dev->kfd->shared_resources.enable_mes)
> +	if (!dqm->dev->kfd->shared_resources.enable_mes) {
>   		execute_queues_cpsch(dqm, KFD_UNMAP_QUEUES_FILTER_DYNAMIC_QUEUES, 0, USE_DEFAULT_GRACE_PERIOD);
> -
> -	/* Set CWSR grace period to 1x1000 cycle for GFX9.4.3 APU */
> -	if (amdgpu_emu_mode == 0 && dqm->dev->adev->gmc.is_app_apu &&
> -	    (KFD_GC_VERSION(dqm->dev) == IP_VERSION(9, 4, 3))) {
> -		uint32_t reg_offset = 0;
> -		uint32_t grace_period = 1;
> -
> -		retval = pm_update_grace_period(&dqm->packet_mgr,
> -						grace_period);
> +		retval = pm_update_grace_period(&dqm->packet_mgr, SET_ASIC_OPTIMIZED_GRACE_PERIOD);
>   		if (retval)
> -			dev_err(dev, "Setting grace timeout failed\n");
> -		else if (dqm->dev->kfd2kgd->build_grace_period_packet_info)
> -			/* Update dqm->wait_times maintained in software */
> -			dqm->dev->kfd2kgd->build_grace_period_packet_info(
> -					dqm->dev->adev,	dqm->wait_times,
> -					grace_period, &reg_offset,
> -					&dqm->wait_times);
> +			dev_err(dev, "Setting optimized grace timeout failed\n");
>   	}
> +	if (dqm->dev->kfd2kgd->get_iq_wait_times)
> +		dqm->dev->kfd2kgd->get_iq_wait_times(dqm->dev->adev,
> +					&dqm->wait_times,
> +					ffs(dqm->dev->xcc_mask) - 1);
why put it here? it seems not related to this patch.
>   
>   	/* setup per-queue reset detection buffer  */
>   	num_hw_queue_slots =  dqm->dev->kfd->shared_resources.num_queue_per_pipe *
> 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 09ab36f8e8c6..fb3419993612 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> @@ -37,7 +37,8 @@
>   
>   #define KFD_MES_PROCESS_QUANTUM		100000
>   #define KFD_MES_GANG_QUANTUM		10000
> -#define USE_DEFAULT_GRACE_PERIOD 0xffffffff
> +#define USE_DEFAULT_GRACE_PERIOD	0xffffffff
what is difference between the two lines above, or just add spaces?
> +#define SET_ASIC_OPTIMIZED_GRACE_PERIOD	0xfffffffe
>   
>   struct device_process_node {
>   	struct qcm_process_device *qpd;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager.c
> index 4984b41cd372..518c6ec23a75 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager.c
> @@ -403,6 +403,9 @@ int pm_update_grace_period(struct packet_manager *pm, uint32_t grace_period)
>   	int retval = 0;
>   	uint32_t *buffer, size;
>   
> +	if (!pm->pmf->set_grace_period || !pm->pmf->set_grace_period_size)
> +		return 0;
> +
>   	size = pm->pmf->set_grace_period_size;
>   
>   	mutex_lock(&pm->lock);
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> index d56525201155..fde212242129 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> @@ -302,9 +302,19 @@ static int pm_set_grace_period_v9(struct packet_manager *pm,
>   		uint32_t grace_period)
>   {
>   	struct pm4_mec_write_data_mmio *packet;
> +	struct device_queue_manager *dqm = pm->dqm;
>   	uint32_t reg_offset = 0;
>   	uint32_t reg_data = 0;
>   
> +	if (grace_period == SET_ASIC_OPTIMIZED_GRACE_PERIOD) {
this patch purpose is setting grace_period at asic specific file. Do we 
need check "grace_period == SET_ASIC_OPTIMIZED_GRACE_PERIOD"? I think we 
can set grace_period directly at asic specific file.
> +		/* Set CWSR grace period to 1x1000 cycle for GFX9.4.3 APU */
> +		if (amdgpu_emu_mode == 0 && dqm->dev->adev->gmc.is_app_apu &&
> +		    KFD_GC_VERSION(dqm->dev) == IP_VERSION(9, 4, 3))
> +			grace_period = 1;
> +		else
> +			return 0;

why return 0 here? that skip following build_grace_period_packet_info 
for no-GFX9.4.3 AP. As above, set grace_period directly is more straight 
forward at asic specific file.

Regards

Xiaogang

> +	}
> +
>   	pm->dqm->dev->kfd2kgd->build_grace_period_packet_info(
>   			pm->dqm->dev->adev,
>   			pm->dqm->wait_times,

[-- Attachment #2: Type: text/html, Size: 6854 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] drm/amdgpu: Don't modify grace_period in helper function
  2025-01-14 19:52 ` [PATCH 2/3] drm/amdgpu: Don't modify grace_period in helper function Elena Sakhnovitch
@ 2025-02-06 21:57   ` Chen, Xiaogang
  2025-02-06 23:45     ` Kasiviswanathan, Harish
  0 siblings, 1 reply; 8+ messages in thread
From: Chen, Xiaogang @ 2025-02-06 21:57 UTC (permalink / raw)
  To: Elena Sakhnovitch, amd-gfx; +Cc: Harish Kasiviswanathan

[-- Attachment #1: Type: text/plain, Size: 3774 bytes --]


On 1/14/2025 1:52 PM, Elena Sakhnovitch wrote:
> From: Harish Kasiviswanathan<Harish.Kasiviswanathan@amd.com>
>
> build_grace_period_packet_info is asic helper function that fetches the
> correct format. It is the responsibility of the caller to validate the
> value.
but what is hurt to valid it at asic function? each asic may has its own 
requirement on grace_period, so has its own checking.
>
> Signed-off-by: Harish Kasiviswanathan<Harish.Kasiviswanathan@amd.com>
> Signed-off-by: Elena Sakhnovitch<Elena.Sakhnovitch@amd.com>
> ---
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c | 18 ++++++------------
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c  | 17 ++++++-----------
>   .../gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c | 12 ++++++++++++
>   3 files changed, 24 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
> index 62176d607bef..8e72dcff8867 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
> @@ -1029,18 +1029,12 @@ void kgd_gfx_v10_build_grace_period_packet_info(struct amdgpu_device *adev,
>   {
>   	*reg_data = wait_times;
>   
> -	/*
> -	 * The CP cannont handle a 0 grace period input and will result in
> -	 * an infinite grace period being set so set to 1 to prevent this.
> -	 */
> -	if (grace_period == 0)
> -		grace_period = 1;
> -
> -	*reg_data = REG_SET_FIELD(*reg_data,
> -			CP_IQ_WAIT_TIME2,
> -			SCH_WAVE,
> -			grace_period);
> -
> +	if (grace_period) {
> +		*reg_data = REG_SET_FIELD(*reg_data,
> +				CP_IQ_WAIT_TIME2,
> +				SCH_WAVE,
> +				grace_period);
> +	}
>   	*reg_offset = SOC15_REG_OFFSET(GC, 0, mmCP_IQ_WAIT_TIME2);
>   }
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> index 441568163e20..04c86a229a23 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> @@ -1085,17 +1085,12 @@ void kgd_gfx_v9_build_grace_period_packet_info(struct amdgpu_device *adev,
>   {
>   	*reg_data = wait_times;
>   
> -	/*
> -	 * The CP cannot handle a 0 grace period input and will result in
> -	 * an infinite grace period being set so set to 1 to prevent this.
> -	 */
> -	if (grace_period == 0)
> -		grace_period = 1;
> -
> -	*reg_data = REG_SET_FIELD(*reg_data,
> -			CP_IQ_WAIT_TIME2,
> -			SCH_WAVE,
> -			grace_period);
> +	if (grace_period) {
> +		*reg_data = REG_SET_FIELD(*reg_data,
> +				CP_IQ_WAIT_TIME2,
> +				SCH_WAVE,
> +				grace_period);
> +	}
>   
>   	*reg_offset = SOC15_REG_OFFSET(GC, 0, mmCP_IQ_WAIT_TIME2);
>   }
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> index fde212242129..adc7f7c78a18 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> @@ -306,6 +306,18 @@ static int pm_set_grace_period_v9(struct packet_manager *pm,
>   	uint32_t reg_offset = 0;
>   	uint32_t reg_data = 0;
>   
> +	/*
> +	 * The CP cannot handle a 0 grace period input and will result in
> +	 * an infinite grace period being set so set to 1 to prevent this.
> +	 */
> +	if (!grace_period) {
> +		pr_debug("Invalid grace_period. Setting default value 0x%x\n",
> +			 pm->dqm->wait_times);
> +		if (WARN_ON((pm->dqm->wait_times & CP_IQ_WAIT_TIME2__SCH_WAVE_MASK)
> +			== 0))
> +			return -EINVAL;

should set grace_period to 1 here?

Regards

Xiaogang

> +	}
> +
>   	if (grace_period == SET_ASIC_OPTIMIZED_GRACE_PERIOD) {
>   		/* Set CWSR grace period to 1x1000 cycle for GFX9.4.3 APU */
>   		if (amdgpu_emu_mode == 0 && dqm->dev->adev->gmc.is_app_apu &&

[-- Attachment #2: Type: text/html, Size: 4691 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH 3/3] drm/amdgpu: Set lower queue retry timeout for gfx9 family
  2025-01-14 19:52 ` [PATCH 3/3] drm/amdgpu: Set lower queue retry timeout for gfx9 family Elena Sakhnovitch
@ 2025-02-06 22:27   ` Russell, Kent
  2025-02-06 23:09     ` Jay Cornwall
  0 siblings, 1 reply; 8+ messages in thread
From: Russell, Kent @ 2025-02-06 22:27 UTC (permalink / raw)
  To: Sakhnovitch, Elena (Elen), amd-gfx@lists.freedesktop.org,
	Cornwall, Jay
  Cc: Sakhnovitch, Elena (Elen), Kasiviswanathan,  Harish,
	Sakhnovitch, Elena (Elen)

[AMD Official Use Only - AMD Internal Distribution Only]

Ping (plus Jay)

 Kent

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Elena
> Sakhnovitch
> Sent: Tuesday, January 14, 2025 2:53 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Sakhnovitch, Elena (Elen) <Elena.Sakhnovitch@amd.com>; Kasiviswanathan,
> Harish <Harish.Kasiviswanathan@amd.com>; Sakhnovitch, Elena (Elen)
> <Elena.Sakhnovitch@amd.com>
> Subject: [PATCH 3/3] drm/amdgpu: Set lower queue retry timeout for gfx9 family
>
> From: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com>
>
> Set more optimized queue retry timeout for gfx9 family starting with
> arcturus.
>
> Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com>
> Signed-off-by: Elena Sakhnovitch <Elena.Sakhnovitch@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.h |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c  |  8 ++++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.h  |  1 +
>  drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c | 14 +++++++++++++-
>  drivers/gpu/drm/amd/include/kgd_kfd_interface.h    |  1 +
>  6 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
> index 8e72dcff8867..652c695d04e2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
> @@ -1024,6 +1024,7 @@ void kgd_gfx_v10_get_iq_wait_times(struct
> amdgpu_device *adev,
>  void kgd_gfx_v10_build_grace_period_packet_info(struct amdgpu_device *adev,
>                                               uint32_t wait_times,
>                                               uint32_t grace_period,
> +                                             uint32_t que_sleep,
>                                               uint32_t *reg_offset,
>                                               uint32_t *reg_data)
>  {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.h
> index 9efd2dd4fdd7..11aedaa8a0b9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.h
> @@ -54,6 +54,7 @@ void kgd_gfx_v10_get_iq_wait_times(struct amdgpu_device
> *adev,
>  void kgd_gfx_v10_build_grace_period_packet_info(struct amdgpu_device *adev,
>                                              uint32_t wait_times,
>                                              uint32_t grace_period,
> +                                            uint32_t que_sleep,
>                                              uint32_t *reg_offset,
>                                              uint32_t *reg_data);
>  uint64_t kgd_gfx_v10_hqd_get_pq_addr(struct amdgpu_device *adev,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> index 04c86a229a23..d93a0285f225 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> @@ -1080,6 +1080,7 @@ void kgd_gfx_v9_get_cu_occupancy(struct
> amdgpu_device *adev,
>  void kgd_gfx_v9_build_grace_period_packet_info(struct amdgpu_device *adev,
>               uint32_t wait_times,
>               uint32_t grace_period,
> +             uint32_t que_sleep,
>               uint32_t *reg_offset,
>               uint32_t *reg_data)
>  {
> @@ -1092,6 +1093,13 @@ void kgd_gfx_v9_build_grace_period_packet_info(struct
> amdgpu_device *adev,
>                               grace_period);
>       }
>
> +     if (que_sleep) {
> +             *reg_data = REG_SET_FIELD(*reg_data,
> +                             CP_IQ_WAIT_TIME2,
> +                             QUE_SLEEP,
> +                             que_sleep);
> +     }
> +
>       *reg_offset = SOC15_REG_OFFSET(GC, 0, mmCP_IQ_WAIT_TIME2);
>  }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.h
> index b6a91a552aa4..3f159d477f5b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.h
> @@ -100,6 +100,7 @@ void kgd_gfx_v9_get_iq_wait_times(struct amdgpu_device
> *adev,
>  void kgd_gfx_v9_build_grace_period_packet_info(struct amdgpu_device *adev,
>                                              uint32_t wait_times,
>                                              uint32_t grace_period,
> +                                            uint32_t que_sleep,
>                                              uint32_t *reg_offset,
>                                              uint32_t *reg_data);
>  uint64_t kgd_gfx_v9_hqd_get_pq_addr(struct amdgpu_device *adev,
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> index adc7f7c78a18..4de8106d14ba 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
> @@ -305,6 +305,7 @@ static int pm_set_grace_period_v9(struct packet_manager
> *pm,
>       struct device_queue_manager *dqm = pm->dqm;
>       uint32_t reg_offset = 0;
>       uint32_t reg_data = 0;
> +     uint32_t que_sleep = 0;
>
>       /*
>        * The CP cannot handle a 0 grace period input and will result in
> @@ -319,18 +320,29 @@ static int pm_set_grace_period_v9(struct
> packet_manager *pm,
>       }
>
>       if (grace_period == SET_ASIC_OPTIMIZED_GRACE_PERIOD) {
> +             /* Reduce CP_IQ_WAIT_TIME2.QUE_SLEEP to 0x1 from default
> 0x40.
> +              * On a 1GHz machine this is roughly 1 microsecond, which is
> +              * about how long it takes to load data out of memory during
> +              * queue connect
> +              * QUE_SLEEP: Wait Count for Dequeue Retry.
> +              */
> +             if (KFD_GC_VERSION(dqm->dev) >= IP_VERSION(9, 4, 1) &&
> +                 KFD_GC_VERSION(dqm->dev) < IP_VERSION(10, 0, 0))
> +                     que_sleep = 1;
> +
>               /* Set CWSR grace period to 1x1000 cycle for GFX9.4.3 APU */
>               if (amdgpu_emu_mode == 0 && dqm->dev->adev->gmc.is_app_apu
> &&
>                   KFD_GC_VERSION(dqm->dev) == IP_VERSION(9, 4, 3))
>                       grace_period = 1;
>               else
> -                     return 0;
> +                     grace_period = 0; /* 0 will keep the default value */
>       }
>
>       pm->dqm->dev->kfd2kgd->build_grace_period_packet_info(
>                       pm->dqm->dev->adev,
>                       pm->dqm->wait_times,
>                       grace_period,
> +                     que_sleep,
>                       &reg_offset,
>                       &reg_data);
>
> diff --git a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> index e3e635a31b8a..1ed3fbedf50b 100644
> --- a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> +++ b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> @@ -316,6 +316,7 @@ struct kfd2kgd_calls {
>       void (*build_grace_period_packet_info)(struct amdgpu_device *adev,
>                       uint32_t wait_times,
>                       uint32_t grace_period,
> +                     uint32_t que_sleep,
>                       uint32_t *reg_offset,
>                       uint32_t *reg_data);
>       void (*get_cu_occupancy)(struct amdgpu_device *adev,
> --
> 2.34.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] drm/amdgpu: Set lower queue retry timeout for gfx9 family
  2025-02-06 22:27   ` Russell, Kent
@ 2025-02-06 23:09     ` Jay Cornwall
  0 siblings, 0 replies; 8+ messages in thread
From: Jay Cornwall @ 2025-02-06 23:09 UTC (permalink / raw)
  To: Russell, Kent, Sakhnovitch, Elena (Elen),
	amd-gfx@lists.freedesktop.org
  Cc: Kasiviswanathan, Harish

On 2/6/2025 16:27, Russell, Kent wrote:

> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> Ping (plus Jay)

Sorry, I'd need the whole patch chain to review.

As a general comment CP_IQ_WAIT_TIME2.QUE_SLEEP is tangential to 
SCH_WAVE. I'm not sure it's useful to tie these together.

SCH_WAVE is lowered when a debugger attaches to reduce context switching 
latency, which it triggers at a high frequency. Higher latency (the 
default) is desirable in normal use to allow short-running wavefronts to 
complete rather than taking the slow save path.

QUE_SLEEP controls the interval between checks for unmet conditions 
(e.g. WAIT_REG_MEM or CUs fully occupied). On contemporary ASICs it's 
best to minimize this by default. The cost is additional bandwidth 
consumed by CP when polling memory, but it's not substantial.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH 2/3] drm/amdgpu: Don't modify grace_period in helper function
  2025-02-06 21:57   ` Chen, Xiaogang
@ 2025-02-06 23:45     ` Kasiviswanathan, Harish
  0 siblings, 0 replies; 8+ messages in thread
From: Kasiviswanathan, Harish @ 2025-02-06 23:45 UTC (permalink / raw)
  To: Chen, Xiaogang, Sakhnovitch, Elena (Elen),
	amd-gfx@lists.freedesktop.org

[Public]

From: Chen, Xiaogang <Xiaogang.Chen@amd.com>
Sent: Thursday, February 6, 2025 4:57 PM
To: Sakhnovitch, Elena (Elen) <Elena.Sakhnovitch@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Kasiviswanathan, Harish <Harish.Kasiviswanathan@amd.com>
Subject: Re: [PATCH 2/3] drm/amdgpu: Don't modify grace_period in helper function


On 1/14/2025 1:52 PM, Elena Sakhnovitch wrote:
From: Harish Kasiviswanathan mailto:Harish.Kasiviswanathan@amd.com

build_grace_period_packet_info is asic helper function that fetches the
correct format. It is the responsibility of the caller to validate the
value.
but what is hurt to valid it at asic function? each asic may has its own requirement on grace_period, so has its own checking.

[HK]:  build_grace_period_packet_info is a helper function to build the packet and that should be it. It need not check or update what value goes inside the packet.

Signed-off-by: Harish Kasiviswanathan mailto:Harish.Kasiviswanathan@amd.com
Signed-off-by: Elena Sakhnovitch mailto:Elena.Sakhnovitch@amd.com
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c | 18 ++++++------------
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c  | 17 ++++++-----------
 .../gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c | 12 ++++++++++++
 3 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
index 62176d607bef..8e72dcff8867 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
@@ -1029,18 +1029,12 @@ void kgd_gfx_v10_build_grace_period_packet_info(struct amdgpu_device *adev,
 {
        *reg_data = wait_times;

-       /*
-        * The CP cannont handle a 0 grace period input and will result in
-        * an infinite grace period being set so set to 1 to prevent this.
-        */
-       if (grace_period == 0)
-               grace_period = 1;
-
-       *reg_data = REG_SET_FIELD(*reg_data,
-                       CP_IQ_WAIT_TIME2,
-                       SCH_WAVE,
-                       grace_period);
-
+       if (grace_period) {
+               *reg_data = REG_SET_FIELD(*reg_data,
+                               CP_IQ_WAIT_TIME2,
+                               SCH_WAVE,
+                               grace_period);
+       }
        *reg_offset = SOC15_REG_OFFSET(GC, 0, mmCP_IQ_WAIT_TIME2);
 }

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
index 441568163e20..04c86a229a23 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
@@ -1085,17 +1085,12 @@ void kgd_gfx_v9_build_grace_period_packet_info(struct amdgpu_device *adev,
 {
        *reg_data = wait_times;

-       /*
-        * The CP cannot handle a 0 grace period input and will result in
-        * an infinite grace period being set so set to 1 to prevent this.
-        */
-       if (grace_period == 0)
-               grace_period = 1;
-
-       *reg_data = REG_SET_FIELD(*reg_data,
-                       CP_IQ_WAIT_TIME2,
-                       SCH_WAVE,
-                       grace_period);
+       if (grace_period) {
+               *reg_data = REG_SET_FIELD(*reg_data,
+                               CP_IQ_WAIT_TIME2,
+                               SCH_WAVE,
+                               grace_period);
+       }

        *reg_offset = SOC15_REG_OFFSET(GC, 0, mmCP_IQ_WAIT_TIME2);
 }
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
index fde212242129..adc7f7c78a18 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_packet_manager_v9.c
@@ -306,6 +306,18 @@ static int pm_set_grace_period_v9(struct packet_manager *pm,
        uint32_t reg_offset = 0;
        uint32_t reg_data = 0;

+       /*
+        * The CP cannot handle a 0 grace period input and will result in
+        * an infinite grace period being set so set to 1 to prevent this.
+        */
+       if (!grace_period) {
+               pr_debug("Invalid grace_period. Setting default value 0x%x\n",
+                        pm->dqm->wait_times);
+               if (WARN_ON((pm->dqm->wait_times & CP_IQ_WAIT_TIME2__SCH_WAVE_MASK)
+                       == 0))
+                       return -EINVAL;
should set grace_period to 1 here?
[HK]:  Previously grace_period==1 was chosen randomly to guard against invalid value 0. Instead of that better to set it to default value.


Regards
Xiaogang

+       }
+
        if (grace_period == SET_ASIC_OPTIMIZED_GRACE_PERIOD) {
                /* Set CWSR grace period to 1x1000 cycle for GFX9.4.3 APU */
                if (amdgpu_emu_mode == 0 && dqm->dev->adev->gmc.is_app_apu &&

^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2025-02-06 23:50 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-14 19:52 [PATCH 1/3] drm/amdkfd: Use asic specific fn to configure grace period Elena Sakhnovitch
2025-01-14 19:52 ` [PATCH 2/3] drm/amdgpu: Don't modify grace_period in helper function Elena Sakhnovitch
2025-02-06 21:57   ` Chen, Xiaogang
2025-02-06 23:45     ` Kasiviswanathan, Harish
2025-01-14 19:52 ` [PATCH 3/3] drm/amdgpu: Set lower queue retry timeout for gfx9 family Elena Sakhnovitch
2025-02-06 22:27   ` Russell, Kent
2025-02-06 23:09     ` Jay Cornwall
2025-02-06 21:56 ` [PATCH 1/3] drm/amdkfd: Use asic specific fn to configure grace period Chen, Xiaogang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.