Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Calling `radeon_fence_activity()` in `.signaled` callback consumes hardware state without waking up wait queues, leading to permanent hangs. - [Medium] Acquiring an `rw_semaphore` in the `dma_fence_ops.signaled` callback violates wait-context rules and can crash PREEMPT_RT kernels. --
--- Patch [1]: [PATCH] drm/radeon: restore hardware polling in fence_is_signaled to fix performance regression --- commit 23682e8220434ea07b863fe650003e08c35ce798 Author: Wang Jiang <[email protected]> drm/radeon: restore hardware polling in fence_is_signaled to fix performance regression This commit restores hardware polling by calling radeon_fence_activity() wrapped in down_read_trylock() inside radeon_fence_is_signaled(). This is intended to fix performance regressions without re-introducing deadlocks. > diff --git a/drivers/gpu/drm/radeon/radeon_fence.c > b/drivers/gpu/drm/radeon/radeon_fence.c > index 02a40e4750c7d..299865569252d 100644 > --- a/drivers/gpu/drm/radeon/radeon_fence.c > +++ b/drivers/gpu/drm/radeon/radeon_fence.c > @@ -360,6 +360,13 @@ static bool radeon_fence_is_signaled(struct dma_fence *f) > if (atomic64_read(&rdev->fence_drv[ring].last_seq) >= seq) > return true; > > + if (down_read_trylock(&rdev->exclusive_lock)) { > + radeon_fence_activity(rdev, ring); [Severity: High] Does this consume the hardware state and drop the wake signal? When a thread calls dma_fence_is_signaled() via polling, it invokes radeon_fence_is_signaled(), which then calls radeon_fence_activity(). The hardware sequence is read, last_seq is advanced, and it returns true. However, radeon_fence_is_signaled() intentionally drops this return value and skips wake_up_all(). Later, when the actual hardware interrupt fires, radeon_fence_process() calls radeon_fence_activity() again. Because last_seq was already advanced, it sees no new activity (seq <= last_seq), returns false, and skips wake_up_all(). Could this steal the wake event and cause tasks waiting on fences to hang indefinitely? > + up_read(&rdev->exclusive_lock); [Severity: Medium] Is it safe to manipulate an rw_semaphore in the dma_fence_ops.signaled callback? The dma_fence_ops.signaled callback can be called from atomic context or directly from a foreign device's hardirq handler due to cross-device dependencies. While down_read_trylock() avoids sleeping on acquire, up_read() still manipulates the underlying lock state. On PREEMPT_RT, this is an rt_mutex, and manipulating it from hardirq context violates RT constraints, potentially leading to a crash. > + > + if (atomic64_read(&rdev->fence_drv[ring].last_seq) >= seq) > + return true; > + } > return false; > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
