Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/i915/selftests: rq->hwsp_seqno[] read after retire in perf_mi_bb_start() --
commit 50caf80a061d2ff30673d4eea9e27e211c297b1d Author: Wentao Liang <[email protected]> drm/i915/selftests: read the seqno before dropping the request This commit moves the read of rq->hwsp_seqno[] in perf_mi_bb_start() before i915_request_put(rq) so the request reference is held while sampling the cycle counts. > diff --git a/drivers/gpu/drm/i915/gt/selftest_engine_cs.c > b/drivers/gpu/drm/i915/gt/selftest_engine_cs.c > index 5ffa5e30f4192..1846f1c7e77ee 100644 > --- a/drivers/gpu/drm/i915/gt/selftest_engine_cs.c > +++ b/drivers/gpu/drm/i915/gt/selftest_engine_cs.c > @@ -196,11 +196,17 @@ static int perf_mi_bb_start(void *arg) > > if (i915_request_wait(rq, 0, HZ / 5) < 0) > err = -EIO; > + > + /* > + * The request can be freed as soon as its last > + * reference is dropped, so read the seqno first. > + */ > + if (!err) > + cycles[i] = rq->hwsp_seqno[3] - > rq->hwsp_seqno[2]; [Severity: Medium] Does holding a reference on rq prevent rq->hwsp_seqno from being redirected away from the timeline HWSP when i915_request_retire() runs? In the scenario described by the commit message, where i915_request_retire() runs before or concurrently with i915_request_wait() returning, i915_request_retire() calls i915_request_mark_complete() before signaling rq->fence and dropping the initial request reference: drivers/gpu/drm/i915/i915_request.c:i915_request_retire() { ... trace_i915_request_retire(rq); i915_request_mark_complete(rq); ... if (!i915_request_signaled(rq)) { spin_lock_irq(&rq->lock); dma_fence_signal_locked(&rq->fence); spin_unlock_irq(&rq->lock); } ... i915_request_put(rq); } And i915_request_mark_complete() rewrites rq->hwsp_seqno to point at the 64-bit rq->fence.seqno field inside struct dma_fence: drivers/gpu/drm/i915/i915_request.h: static inline void i915_request_mark_complete(struct i915_request *rq) { WRITE_ONCE(rq->hwsp_seqno, /* decouple from HWSP */ (u32 *)&rq->fence.seqno); } Once i915_request_mark_complete() has executed, indexing rq->hwsp_seqno[2] and rq->hwsp_seqno[3] in perf_mi_bb_start() reads out of bounds past rq->fence.seqno into adjacent fields of struct dma_fence (rq->fence.flags and rq->fence.refcount) rather than reading the timestamps written by write_timestamp(rq, 2) and write_timestamp(rq, 3) in the timeline HWSP. Should perf_mi_bb_start() save const u32 *hwsp = rq->hwsp_seqno before calling i915_request_add(rq) (similar to __live_rc6_ctx() in drivers/gpu/drm/i915/gt/selftest_rc6.c), or read from ce->timeline->hwsp_seqno directly? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
