Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Missing early return in the compute ring search loop causes unintended fall-through to user queue processing. --
--- Patch [1]: [PATCH v6] drm/amd/amdgpu: remove duplicated code in gfx_v11 and gfx_v12 --- commit 554dbb6c1295b4176f2248f06098908b58cf4225 Author: Ulisses Paixao <[email protected]> drm/amd/amdgpu: remove duplicated code in gfx_v11 and gfx_v12 This commit consolidates the shared logic for finding and handling a scheduler fault on a ring into a common function, amdgpu_gfx_handle_priv_fault, in amdgpu_gfx.c. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c > index 1e275c2e7dd3e..3b612e9a5bb8d 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c [ ... ] > @@ -855,6 +856,56 @@ 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 > + * @entry: interrupt vector entry containing fault information > + * @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, > + struct amdgpu_iv_entry *entry, > + u8 me_id, u8 pipe_id, u8 queue_id) > +{ > + struct amdgpu_ring *ring; > + int i; > + > + /* > + * Try KQ first by ring_id (HW slot is authoritative). The > + * KMD compute_hqd_mask contract guarantees KCQ and user queues > + * never share a HW slot. > + */ > + if (!adev->gfx.disable_kq) { > + for (i = 0; i < adev->gfx.num_gfx_rings; i++) { > + ring = &adev->gfx.gfx_ring[i]; > + 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]; > + if (ring->me == me_id && ring->pipe == pipe_id && > + ring->queue == queue_id) > + drm_sched_fault(&ring->sched); [Severity: Medium] Should there be a return statement here? Without an early return, the loop correctly triggers a scheduler fault on a matching compute ring, but then execution erroneously continues to the MES user queue fallback logic below. This could result in spuriously processing a reset IRQ for a user queue if the PASID and doorbell offset happen to alias. > + } > + } > + > + u32 doorbell_offset = entry->src_data[0] & > AMDGPU_CTXID0_DOORBELL_ID_MASK; > + > + /* No KQ matched: HW slot is a MES-scheduled user queue. */ > + if (adev->enable_mes && doorbell_offset) > + amdgpu_userq_process_reset_irq(adev, entry->pasid, > + doorbell_offset); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
