Re: [PATCH v4] drm/amd/amdgpu: remove duplicated code in gfx_v11 and gfx_v12

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The generic helper `amdgpu_gfx_handle_priv_fault` lacks an `xcc_id` parameter and hardcodes array indexing for a single-XCC configuration, breaking multi-XCC support.
- [Low] Missing early return in the `compute_ring` search loop causes unnecessary iteration.
--

commit 249c05a6da6280bb2499d0d7191928c66dd35d25
Author: Ulisses Paixao <ulissespaixao@usp.br>

drm/amd/amdgpu: remove duplicated code in gfx_v11 and gfx_v12

This patch moves the shared ring-searching logic for handling privileged
instruction faults into a common function amdgpu_gfx_handle_priv_fault.
Hardware-specific decoding remains in the version-specific files.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index 0506b90f318e4..25a4b33f42a81 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
[ ... ]
> @@ -855,6 +855,42 @@ int amdgpu_gfx_enable_kgq(struct amdgpu_device *adev, int xcc_id)
>  	return r;
>  }
>  
> +/**
> + * amdgpu_gfx_handle_priv_fault - Handle privileged instruction fault
> + *
> + * @adev: amdgpu_device pointer
> + * @me_id: micro-engine ID of the faulty ring
> + * @pipe_id: pipe ID of the faulty ring
> + * @queue_id: queue ID of the faulty ring
> + *
> + * This function handles privileged instruction faults by identifying
> + * the faulty ring (gfx or compute) and triggering a scheduler fault
> + */
> +void amdgpu_gfx_handle_priv_fault(struct amdgpu_device *adev,
> +					u8 me_id, u8 pipe_id, u8 queue_id)
> +{
> +	struct amdgpu_ring *ring;
> +	int i;
> +
> +	if (!adev->gfx.disable_kq) {
> +		for (i = 0; i < adev->gfx.num_gfx_rings; i++) {
> +			ring = &adev->gfx.gfx_ring[i];

[Severity: Medium]
Does this logic fully support multi-XCC configurations?

In multi-XCC setups, the gfx_ring array contains rings for all XCCs
continuously. Iterating only up to adev->gfx.num_gfx_rings (the per-XCC
count) strictly searches the rings belonging to XCC 0.

If a privileged instruction fault occurs on a ring belonging to an XCC
other than 0, will it fail to find the matching ring because the search
isn't offset by the xcc_id?

> +			if (ring->me == me_id && ring->pipe == pipe_id &&
> +			    ring->queue == queue_id) {
> +				drm_sched_fault(&ring->sched);
> +				return;
> +			}
> +		}
> +
> +		for (i = 0; i < adev->gfx.num_compute_rings; i++) {
> +			ring = &adev->gfx.compute_ring[i];

[Severity: Medium]
Will this also miss compute rings for XCC > 0, similar to the gfx_ring
loop above?

> +			if (ring->me == me_id && ring->pipe == pipe_id &&
> +			    ring->queue == queue_id)
> +				drm_sched_fault(&ring->sched);

[Severity: Low]
Is there a missing return statement here?

The loop for gfx_ring above added an early return once a match is found,
but this loop continues iterating through the remaining compute rings
needlessly after the target ring is handled.

> +		}
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630205545.23920-1-ulissespaixao@usp.br?part=1