[PATCH 2/3] drm/amdgpu: Fix fault shader cleaner on MI3

[PATCH 2/3] drm/amdgpu: Fix fault shader cleaner on MI3

[Kent Russell]

MI300/MI350 have a different register layout, and cannot reuse the same
definitions from MI2. The shader cleaner can be reused, but not the
initialization steps.

Also change the dim_x to ensure that we cover all wave slots per SIMD.

Signed-off-by: Kent Russell <kent.russell@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c | 37 +++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c
index dc46133ed506..a9f42d99d5c2 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c
@@ -23,6 +23,7 @@
 #include "amdgpu.h"
 #include "soc15.h"
 #include "soc15d.h"
+#include "soc15_common.h"
 
 #include "gc/gc_9_4_2_offset.h"
 #include "gc/gc_9_4_2_sh_mask.h"
@@ -365,6 +366,20 @@ const struct soc15_reg_entry fault_cleaner_init_regs_aldebaran[] = {
 	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_STATIC_THREAD_MGMT_SE7), 0xffffffff },
 };
 
+static const struct soc15_reg_entry fault_cleaner_init_regs_aqua_vanjaram[] = {
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_RESOURCE_LIMITS), 0x000000000 },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_NUM_THREAD_X), 0x40 },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_NUM_THREAD_Y), 1 },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_NUM_THREAD_Z), 1 },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_PGM_RSRC1), 0x00af0000 },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_PGM_RSRC2), 0x000000c0 },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_PGM_RSRC3), 0x00000000 },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_STATIC_THREAD_MGMT_SE0), 0xffffffff },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_STATIC_THREAD_MGMT_SE1), 0xffffffff },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_STATIC_THREAD_MGMT_SE2), 0xffffffff },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_STATIC_THREAD_MGMT_SE3), 0xffffffff },
+};
+
 static int gfx_v9_4_2_run_shader(struct amdgpu_device *adev,
 				 struct amdgpu_ring *ring,
 				 const u32 *shader_ptr, u32 shader_size,
@@ -723,6 +738,8 @@ static int gfx_v9_clean_shaders(struct amdgpu_device *adev)
 	struct amdgpu_ib wb_ib;
 	struct amdgpu_ring *ring;
 	struct dma_fence *fence = NULL;
+	const struct soc15_reg_entry *init_regs;
+	u32 init_regs_size;
 	u32 pattern = 0xa;
 	int xcc_id;
 
@@ -731,6 +748,20 @@ static int gfx_v9_clean_shaders(struct amdgpu_device *adev)
 		return 0;
 	}
 
+	/* gfx943+ has different register layout compared to gfx942.*/
+	switch (amdgpu_ip_version(adev, GC_HWIP, 0)) {
+	case IP_VERSION(9, 4, 3):
+	case IP_VERSION(9, 4, 4):
+	case IP_VERSION(9, 5, 0):
+		init_regs = fault_cleaner_init_regs_aqua_vanjaram;
+		init_regs_size = ARRAY_SIZE(fault_cleaner_init_regs_aqua_vanjaram);
+		break;
+	default:
+		init_regs = fault_cleaner_init_regs_aldebaran;
+		init_regs_size = ARRAY_SIZE(fault_cleaner_init_regs_aldebaran);
+		break;
+	}
+
 	for_each_inst(xcc_id, adev->gfx.xcc_mask) {
 		ring = &adev->gfx.compute_ring[xcc_id * adev->gfx.num_compute_rings];
 
@@ -752,9 +783,9 @@ static int gfx_v9_clean_shaders(struct amdgpu_device *adev)
 				ring,
 				fault_cleaner_compute_shader_aldebaran,
 				sizeof(fault_cleaner_compute_shader_aldebaran),
-				fault_cleaner_init_regs_aldebaran,
-				ARRAY_SIZE(fault_cleaner_init_regs_aldebaran),
-				adev->gfx.cu_info.number * 32,
+				init_regs,
+				init_regs_size,
+				adev->gfx.cu_info.number * SIMD_ID_MAX * WAVE_ID_MAX,
 				wb_ib.gpu_addr, pattern, &fence);
 
 		if (r) {
-- 
2.43.0

[PATCH 1/3] drm/amdgpu: Run shader cleaner on all XCCs on faults

[Kent Russell]

The previous patch only ran on XCC0, giving us a good chance that we
don't clear the right registers on fault handling in MI3. Loop through
them all to make sure we clear them all.

Signed-off-by: Kent Russell <kent.russell@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c | 78 +++++++++++++++----------
 1 file changed, 47 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c
index f54f54fe535e..dc46133ed506 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c
@@ -716,51 +716,67 @@ static int gfx_v9_4_2_do_vgprs_init(struct amdgpu_device *adev)
 
 static int gfx_v9_clean_shaders(struct amdgpu_device *adev)
 {
-	int r;
+	int r = 0;
 	/* CU_ID: 0~15, SIMD_ID: 0~3, WAVE_ID: 0 ~ 9 */
 	int wb_size = adev->gfx.config.max_shader_engines *
 			 CU_ID_MAX * SIMD_ID_MAX * WAVE_ID_MAX;
 	struct amdgpu_ib wb_ib;
+	struct amdgpu_ring *ring;
 	struct dma_fence *fence = NULL;
 	u32 pattern = 0xa;
+	int xcc_id;
 
-	/* bail if the compute ring is not ready */
-	if (!adev->gfx.compute_ring[0].sched.ready)
+	if (adev->gfx.disable_kq) {
+		dev_warn(adev->dev, "Cannot run shader cleaner with kernel queues disabled");
 		return 0;
-
-	/* allocate the write-back buffer from IB */
-	memset(&wb_ib, 0, sizeof(wb_ib));
-	r = amdgpu_ib_get(adev, NULL, (1 + wb_size) * sizeof(uint32_t),
-			  AMDGPU_IB_POOL_DIRECT, &wb_ib);
-	if (r) {
-		dev_err(adev->dev, "failed to get ib (%d) for wb.\n", r);
-		return r;
 	}
-	memset(wb_ib.ptr, 0, (1 + wb_size) * sizeof(uint32_t));
 
-	r = gfx_v9_4_2_run_shader(adev,
-			&adev->gfx.compute_ring[0],
-			fault_cleaner_compute_shader_aldebaran,
-			sizeof(fault_cleaner_compute_shader_aldebaran),
-			fault_cleaner_init_regs_aldebaran,
-			ARRAY_SIZE(fault_cleaner_init_regs_aldebaran),
-			adev->gfx.cu_info.number * 32,
-			wb_ib.gpu_addr, pattern, &fence);
+	for_each_inst(xcc_id, adev->gfx.xcc_mask) {
+		ring = &adev->gfx.compute_ring[xcc_id * adev->gfx.num_compute_rings];
 
-	if (r) {
-		dev_err(adev->dev, "failed to clear MI200\n");
-		goto pro_end;
-	}
+		/* bail if the compute ring is not ready */
+		if (!ring->sched.ready)
+			continue;
 
-	/* wait for the GPU to finish processing the IB */
-	r = dma_fence_wait(fence, false);
-	if (r)
-		dev_err(adev->dev, "timeout to clear MI200\n");
+		/* allocate the write-back buffer from IB */
+		memset(&wb_ib, 0, sizeof(wb_ib));
+		r = amdgpu_ib_get(adev, NULL, (1 + wb_size) * sizeof(uint32_t),
+				  AMDGPU_IB_POOL_DIRECT, &wb_ib);
+		if (r) {
+			dev_err(adev->dev, "failed to get ib (%d) for wb.\n", r);
+			return r;
+		}
+		memset(wb_ib.ptr, 0, (1 + wb_size) * sizeof(uint32_t));
+
+		r = gfx_v9_4_2_run_shader(adev,
+				ring,
+				fault_cleaner_compute_shader_aldebaran,
+				sizeof(fault_cleaner_compute_shader_aldebaran),
+				fault_cleaner_init_regs_aldebaran,
+				ARRAY_SIZE(fault_cleaner_init_regs_aldebaran),
+				adev->gfx.cu_info.number * 32,
+				wb_ib.gpu_addr, pattern, &fence);
+
+		if (r) {
+			dev_err(adev->dev, "failed to clear GPR on xcc %d\n", xcc_id);
+			goto pro_end;
+		}
+
+		/* wait for the GPU to finish processing the IB */
+		r = dma_fence_wait(fence, false);
+		if (r)
+			dev_err(adev->dev, "timeout to clear GPR on xcc %d\n", xcc_id);
 
 pro_end:
-	if (fence)
-		dma_fence_put(fence);
-	amdgpu_ib_free(&wb_ib, NULL);
+		if (fence) {
+			dma_fence_put(fence);
+			fence = NULL;
+		}
+		amdgpu_ib_free(&wb_ib, NULL);
+
+		if (r)
+			break;
+	}
 
 	if (r)
 		dev_dbg(adev->dev, "Clean GPR Failed\n");
-- 
2.43.0

[PATCH 2/3] drm/amdgpu: Fix fault shader cleaner on MI3

[Kent Russell]

MI300/MI350 have a different register layout, and cannot reuse the same
definitions from MI2. The shader cleaner can be reused, but not the
initialization steps.

Also change the dim_x to ensure that we cover all wave slots per SIMD.

Signed-off-by: Kent Russell <kent.russell@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c | 24 +++++++++++++++++++++---
 drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c | 14 ++++++++++++++
 drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.h |  3 +++
 3 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c
index dc46133ed506..95789f8d7018 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c
@@ -23,12 +23,14 @@
 #include "amdgpu.h"
 #include "soc15.h"
 #include "soc15d.h"
+#include "soc15_common.h"
 
 #include "gc/gc_9_4_2_offset.h"
 #include "gc/gc_9_4_2_sh_mask.h"
 #include "gfx_v9_0.h"
 
 #include "gfx_v9_4_2.h"
+#include "gfx_v9_4_3.h"
 #include "amdgpu_ras.h"
 #include "amdgpu_gfx.h"
 
@@ -723,6 +725,8 @@ static int gfx_v9_clean_shaders(struct amdgpu_device *adev)
 	struct amdgpu_ib wb_ib;
 	struct amdgpu_ring *ring;
 	struct dma_fence *fence = NULL;
+	const struct soc15_reg_entry *init_regs;
+	u32 init_regs_size;
 	u32 pattern = 0xa;
 	int xcc_id;
 
@@ -731,6 +735,20 @@ static int gfx_v9_clean_shaders(struct amdgpu_device *adev)
 		return 0;
 	}
 
+	/* gfx943+ has different register layout compared to gfx942.*/
+	switch (amdgpu_ip_version(adev, GC_HWIP, 0)) {
+	case IP_VERSION(9, 4, 3):
+	case IP_VERSION(9, 4, 4):
+	case IP_VERSION(9, 5, 0):
+		init_regs = gfx_v9_4_3_fault_cleaner_init_regs;
+		init_regs_size = GFX_V9_4_3_FAULT_CLEANER_INIT_REGS_SIZE;
+		break;
+	default:
+		init_regs = fault_cleaner_init_regs_aldebaran;
+		init_regs_size = ARRAY_SIZE(fault_cleaner_init_regs_aldebaran);
+		break;
+	}
+
 	for_each_inst(xcc_id, adev->gfx.xcc_mask) {
 		ring = &adev->gfx.compute_ring[xcc_id * adev->gfx.num_compute_rings];
 
@@ -752,9 +770,9 @@ static int gfx_v9_clean_shaders(struct amdgpu_device *adev)
 				ring,
 				fault_cleaner_compute_shader_aldebaran,
 				sizeof(fault_cleaner_compute_shader_aldebaran),
-				fault_cleaner_init_regs_aldebaran,
-				ARRAY_SIZE(fault_cleaner_init_regs_aldebaran),
-				adev->gfx.cu_info.number * 32,
+				init_regs,
+				init_regs_size,
+				adev->gfx.cu_info.number * SIMD_ID_MAX * WAVE_ID_MAX,
 				wb_ib.gpu_addr, pattern, &fence);
 
 		if (r) {
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
index da031caa4e9e..d352d2ef4a5d 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
@@ -164,6 +164,20 @@ static const struct amdgpu_hwip_reg_entry gc_cp_reg_list_9_4_3[] = {
 
 struct amdgpu_gfx_ras gfx_v9_4_3_ras;
 
+const struct soc15_reg_entry gfx_v9_4_3_fault_cleaner_init_regs[] = {
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_RESOURCE_LIMITS), 0x000000000 },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_NUM_THREAD_X), 0x40 },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_NUM_THREAD_Y), 1 },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_NUM_THREAD_Z), 1 },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_PGM_RSRC1), 0x00af0000 },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_PGM_RSRC2), 0x000000c0 },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_PGM_RSRC3), 0x00000000 },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_STATIC_THREAD_MGMT_SE0), 0xffffffff },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_STATIC_THREAD_MGMT_SE1), 0xffffffff },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_STATIC_THREAD_MGMT_SE2), 0xffffffff },
+	{ SOC15_REG_ENTRY(GC, 0, regCOMPUTE_STATIC_THREAD_MGMT_SE3), 0xffffffff },
+};
+
 static void gfx_v9_4_3_set_ring_funcs(struct amdgpu_device *adev);
 static void gfx_v9_4_3_set_irq_funcs(struct amdgpu_device *adev);
 static void gfx_v9_4_3_set_gds_init(struct amdgpu_device *adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.h b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.h
index 42d67ee0e7ef..e39d3f397259 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.h
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.h
@@ -28,4 +28,7 @@ extern const struct amdgpu_ip_block_version gfx_v9_4_3_ip_block;
 
 extern struct amdgpu_xcp_ip_funcs gfx_v9_4_3_xcp_funcs;
 
+extern const struct soc15_reg_entry gfx_v9_4_3_fault_cleaner_init_regs[];
+#define GFX_V9_4_3_FAULT_CLEANER_INIT_REGS_SIZE 11
+
 #endif /* __GFX_V9_4_3_H__ */
-- 
2.43.0

[PATCH 3/3] drm/amdgpu: Fix fault cleaner race condition

[Kent Russell]

clean_process_queues_cpsch would exit early if the memory was evicted,
which would skip running the shader cleaner

Run the cleaner unconditionally, and track whether the fault was cleaned
with a new boolean in the qcd struct.

Signed-off-by: Kent Russell <kent.russell@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 5 ++++-
 drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c       | 2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h                 | 5 +++++
 3 files changed, 10 insertions(+), 2 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 86b70748617b..03d3a739d627 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
@@ -1571,9 +1571,12 @@ static int clean_process_queues_cpsch(struct device_queue_manager *dqm,
 	int retval = 0;
 
 	dqm_lock(dqm);
-	if (qpd->evicted++ > 0) /* already evicted, do nothing */
+	if (qpd->fault_cleaned)
 		goto out;
 
+	qpd->evicted++;
+	qpd->fault_cleaned = true;
+
 	pdd = qpd_to_pdd(qpd);
 
 	/* The debugger creates processes that temporarily have not acquired
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
index 8dee8c43824d..0bd8e781adec 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_int_process_v9.c
@@ -418,7 +418,7 @@ static void gfx_9_clean_fault(uint16_t pasid)
 	if (!pdd)
 		return;
 
-	/* To Clean the GFX on the faulting device	 */
+	/* To clean the GFX fault state on the faulting device */
 	dqm = pdd->dev->dqm;
 	dqm->ops.clean_process_queues_cpsch(dqm, &pdd->qpd);
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 581fb7ceba40..3292e616d36f 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -695,6 +695,11 @@ struct qcm_process_device {
 	bool is_debug;
 	unsigned int evicted; /* eviction counter, 0=active */
 
+	/* Set after fault clean runs; never cleared.
+	 * Prevents redundant cleaning from pipelined interrupt workers.
+	 */
+	bool fault_cleaned;
+
 	/* This flag tells if we should reset all wavefronts on
 	 * process termination
 	 */
-- 
2.43.0

Re: [PATCH 1/3] drm/amdgpu: Run shader cleaner on all XCCs on faults

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] drm/amdgpu: unsynchronized direct ring submission in
  gfx_v9_clean_shaders()
- [Medium] drm/amdgpu: physical XCC bit index used to index
  compute_ring[]
- [Low] drm/amdgpu: missing Fixes: trailer on multi-XCC fault cleaner
  fix
--

commit 41cd4653f10e59626a5d04f393619df1808489a0
Author: Kent Russell <kent.russell@amd.com>

> drm/amdgpu: Run shader cleaner on all XCCs on faults
>
> The previous patch only ran on XCC0, giving us a good chance that we
> don't clear the right registers on fault handling in MI3. Loop through
> them all to make sure we clear them all.
>
> Signed-off-by: Kent Russell <kent.russell@amd.com>

[Severity: Low]
Since this fixes multi-XCC fault cleaning on MI300, should it include:

  Fixes: 923d09ab1f9a ("drm/amdkfd: Fix TCP XNACK scoreboard reset race on MI300")

> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c
> index f54f54fe535e0..dc46133ed5068 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c
> @@ -716,51 +716,67 @@ static int gfx_v9_4_2_do_vgprs_init(struct amdgpu_device *adev)
>  
>  static int gfx_v9_clean_shaders(struct amdgpu_device *adev)
>  {
> -	int r;
> +	int r = 0;
>  	/* CU_ID: 0~15, SIMD_ID: 0~3, WAVE_ID: 0 ~ 9 */
>  	int wb_size = adev->gfx.config.max_shader_engines *
>  			 CU_ID_MAX * SIMD_ID_MAX * WAVE_ID_MAX;
>  	struct amdgpu_ib wb_ib;
> +	struct amdgpu_ring *ring;
>  	struct dma_fence *fence = NULL;
>  	u32 pattern = 0xa;
> +	int xcc_id;
>  
> -	/* bail if the compute ring is not ready */
> -	if (!adev->gfx.compute_ring[0].sched.ready)
> +	if (adev->gfx.disable_kq) {
> +		dev_warn(adev->dev, "Cannot run shader cleaner with kernel queues disabled");
>  		return 0;
> -
> -	/* allocate the write-back buffer from IB */
> -	memset(&wb_ib, 0, sizeof(wb_ib));
> -	r = amdgpu_ib_get(adev, NULL, (1 + wb_size) * sizeof(uint32_t),
> -			  AMDGPU_IB_POOL_DIRECT, &wb_ib);
> -	if (r) {
> -		dev_err(adev->dev, "failed to get ib (%d) for wb.\n", r);
> -		return r;
>  	}
> -	memset(wb_ib.ptr, 0, (1 + wb_size) * sizeof(uint32_t));
>  
> -	r = gfx_v9_4_2_run_shader(adev,
> -			&adev->gfx.compute_ring[0],
> -			fault_cleaner_compute_shader_aldebaran,
> -			sizeof(fault_cleaner_compute_shader_aldebaran),
> -			fault_cleaner_init_regs_aldebaran,
> -			ARRAY_SIZE(fault_cleaner_init_regs_aldebaran),
> -			adev->gfx.cu_info.number * 32,
> -			wb_ib.gpu_addr, pattern, &fence);
> +	for_each_inst(xcc_id, adev->gfx.xcc_mask) {
> +		ring = &adev->gfx.compute_ring[xcc_id * adev->gfx.num_compute_rings];

[Severity: Medium]
Does for_each_inst(xcc_id, adev->gfx.xcc_mask) index adev->gfx.compute_ring[]
by physical XCC bit position instead of logical XCC index?

In gfx_v9_4_3_sw_init() and amdgpu_gfx_run_cleaner_shader(),
adev->gfx.compute_ring[] is populated and indexed contiguously for
0 <= xcc_id < NUM_XCC(adev->gfx.xcc_mask):

drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c:amdgpu_gfx_run_cleaner_shader() {
	int num_xcc = NUM_XCC(adev->gfx.xcc_mask);
    ...
	for (xcc_id = 0; xcc_id < num_xcc; xcc_id++) {
		for (i = 0; i < adev->gfx.num_compute_rings; i++) {
			ring = &adev->gfx.compute_ring[i + xcc_id * adev->gfx.num_compute_rings];
    ...
}

When lower physical GC instances are harvested in adev->gfx.xcc_mask,
for_each_inst() skips active logical XCCs at the harvested bit positions
(leaving their GPRs and TCP XNACK scoreboard entries uncleared after a VM
fault) and indexes uninitialized compute_ring[] slots at bit positions
>= NUM_XCC(adev->gfx.xcc_mask).

> -	if (r) {
> -		dev_err(adev->dev, "failed to clear MI200\n");
> -		goto pro_end;
> -	}
> +		/* bail if the compute ring is not ready */
> +		if (!ring->sched.ready)
> +			continue;
>  
> -	/* wait for the GPU to finish processing the IB */
> -	r = dma_fence_wait(fence, false);
> -	if (r)
> -		dev_err(adev->dev, "timeout to clear MI200\n");
> +		/* allocate the write-back buffer from IB */
> +		memset(&wb_ib, 0, sizeof(wb_ib));
> +		r = amdgpu_ib_get(adev, NULL, (1 + wb_size) * sizeof(uint32_t),
> +				  AMDGPU_IB_POOL_DIRECT, &wb_ib);
> +		if (r) {
> +			dev_err(adev->dev, "failed to get ib (%d) for wb.\n", r);
> +			return r;
> +		}
> +		memset(wb_ib.ptr, 0, (1 + wb_size) * sizeof(uint32_t));
> +
> +		r = gfx_v9_4_2_run_shader(adev,
> +				ring,
> +				fault_cleaner_compute_shader_aldebaran,
> +				sizeof(fault_cleaner_compute_shader_aldebaran),
> +				fault_cleaner_init_regs_aldebaran,
> +				ARRAY_SIZE(fault_cleaner_init_regs_aldebaran),
> +				adev->gfx.cu_info.number * 32,
> +				wb_ib.gpu_addr, pattern, &fence);

[Severity: High]
Can calling gfx_v9_4_2_run_shader() across all XCC compute rings race on
struct amdgpu_ring and corrupt the hardware ring buffer and fence state?

gfx_v9_4_2_run_shader() submits directly via amdgpu_job_submit_direct() ->
amdgpu_ib_schedule(), which has no internal lock and relies on the DRM GPU
scheduler to serialize ring access.  On multi-partition MI300 devices,
clean_process_queues_cpsch() only holds the per-partition dqm_lock(dqm) and
a shared read lock on adev->reset_domain->sem before calling
hqd_gfx_clean_fault(adev):

CPU0 (node 0 fault wq)                CPU1 (node 1 fault wq / drm_sched)
clean_process_queues_cpsch(dqm0)      clean_process_queues_cpsch(dqm1)
  dqm_lock(dqm0)                        dqm_lock(dqm1)
  hqd_gfx_clean_fault(adev)             hqd_gfx_clean_fault(adev)
    gfx_v9_clean_shaders(adev)            gfx_v9_clean_shaders(adev)
      gfx_v9_4_2_run_shader(ring)           gfx_v9_4_2_run_shader(ring)
        amdgpu_job_submit_direct()            amdgpu_job_submit_direct()
          amdgpu_ib_schedule(ring)              amdgpu_ib_schedule(ring)
                                                  <-- concurrent ring write

Because each partition's fault handler now walks all XCCs in adev rather
than its own partition, concurrent fault handlers (as well as normal jobs
scheduled on ring->sched) can execute amdgpu_ib_schedule(),
amdgpu_fence_emit(), and amdgpu_ring_commit() simultaneously on the same
compute_ring, interleaving PM4 packets and corrupting ring->wptr,
ring->count_dw, and ring->fence_drv.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112134340.4122401-1-kent.russell@amd.com?part=1

Re: [PATCH 2/3] drm/amdgpu: Fix fault shader cleaner on MI3

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/amdgpu: physical XCC bit index used in
  gfx_v9_clean_shaders()
--

commit d2d8466691d7c4222ecf6651152915dffb5f88c8
Author: Kent Russell <kent.russell@amd.com>

drm/amdgpu: Fix fault shader cleaner on MI3

This commit adds MI300/MI350 register initialization definitions for the
fault shader cleaner and updates the dispatch dimension in
gfx_v9_clean_shaders() to cover all wave slots per SIMD.

> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c
> index dc46133ed5068..95789f8d7018a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_2.c

[ ... ]

> @@ -731,6 +735,20 @@ static int gfx_v9_clean_shaders(struct amdgpu_device *adev)
>  		return 0;
>  	}
>  
> +	/* gfx943+ has different register layout compared to gfx942.*/
> +	switch (amdgpu_ip_version(adev, GC_HWIP, 0)) {
> +	case IP_VERSION(9, 4, 3):
> +	case IP_VERSION(9, 4, 4):
> +	case IP_VERSION(9, 5, 0):
> +		init_regs = gfx_v9_4_3_fault_cleaner_init_regs;
> +		init_regs_size = GFX_V9_4_3_FAULT_CLEANER_INIT_REGS_SIZE;
> +		break;
> +	default:
> +		init_regs = fault_cleaner_init_regs_aldebaran;
> +		init_regs_size = ARRAY_SIZE(fault_cleaner_init_regs_aldebaran);
> +		break;
> +	}
> +
>  	for_each_inst(xcc_id, adev->gfx.xcc_mask) {
>  		ring = &adev->gfx.compute_ring[xcc_id * adev->gfx.num_compute_rings];

[Severity: Medium]
Does iterating over adev->gfx.xcc_mask with for_each_inst() index
adev->gfx.compute_ring[] with a physical XCC bit position instead of a
logical XCC index?

In amdgpu_discovery_harvest_ip(), harvested GC instances are cleared from
adev->gfx.xcc_mask with ~BIT(inst), whereas gfx_v9_4_3_sw_init() and
amdgpu_gfx_run_cleaner_shader() populate and index adev->gfx.compute_ring[]
contiguously for logical XCCs 0 .. NUM_XCC(adev->gfx.xcc_mask) - 1.  On
MI300/MI350 parts with harvested lower-numbered XCCs, this loop skips the
logical compute rings at the harvested bit positions (leaving those active
XCCs uncleaned after a VM fault) and reads uninitialized compute_ring
entries at physical bit positions >= NUM_XCC(adev->gfx.xcc_mask).

Iterating from 0 to NUM_XCC(adev->gfx.xcc_mask) - 1 would match how
adev->gfx.compute_ring[] is indexed elsewhere.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112134340.4122401-1-kent.russell@amd.com?part=2

Re: [PATCH 3/3] drm/amdgpu: Fix fault cleaner race condition

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] drm/amdkfd: missing Fixes trailer for
  clean_process_queues_cpsch() race fix
--

commit f7b85995fa163f69dd802bed02a96361c80519f7
Author: Kent Russell <kent.russell@amd.com>

> drm/amdgpu: Fix fault cleaner race condition
>
> clean_process_queues_cpsch would exit early if the memory was evicted,
> which would skip running the shader cleaner
>
> Run the cleaner unconditionally, and track whether the fault was cleaned
> with a new boolean in the qcd struct.
>
> Signed-off-by: Kent Russell <kent.russell@amd.com>

[Severity: Low]
This fixes a bug in clean_process_queues_cpsch() where already-evicted
queues caused an early return before running hqd_gfx_clean_fault().
Should the commit message include:

  Fixes: a6332b847187 ("drm/amdkfd: Fix TCP XNACK scoreboard reset race")

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112134340.4122401-1-kent.russell@amd.com?part=3