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 <[email protected]> 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/[email protected]?part=1
