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

[sashiko-bot@kernel.org]

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