[Intel-gfx] [PATCH 1/7] drm/i915/gt: Avoid resetting ring->head outside of its timeline mutex
We manipulate ring->head while active in i915_request_retire underneath the timeline manipulation. We cannot rely on a stable ring->head outside of the timeline->mutex, in particular while setting up the context for resume and reset. Closes: https://gitlab.freedesktop.org/drm/intel/issues/1126 Fixes: 0881954965e3 ("drm/i915: Introduce intel_context.pin_mutex for pin management") Fixes: e5dadff4b093 ("drm/i915: Protect request retirement with timeline->mutex") References: f3c0efc9fe7a ("drm/i915/execlists: Leave resetting ring to intel_ring") Signed-off-by: Chris Wilson Cc: Matthew Auld Cc: Tvrtko Ursulin Cc: Mika Kuoppala --- drivers/gpu/drm/i915/gt/intel_lrc.c| 36 -- drivers/gpu/drm/i915/gt/selftest_lrc.c | 2 +- 2 files changed, 18 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index 929be03bbe7e..70d91ad923ef 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -235,7 +235,8 @@ static void execlists_init_reg_state(u32 *reg_state, bool close); static void __execlists_update_reg_state(const struct intel_context *ce, -const struct intel_engine_cs *engine); +const struct intel_engine_cs *engine, +u32 head); static void mark_eio(struct i915_request *rq) { @@ -1184,12 +1185,11 @@ static void reset_active(struct i915_request *rq, head = rq->tail; else head = active_request(ce->timeline, rq)->head; - ce->ring->head = intel_ring_wrap(ce->ring, head); - intel_ring_update_space(ce->ring); + head = intel_ring_wrap(ce->ring, head); /* Scrub the context image to prevent replaying the previous batch */ restore_default_state(ce, engine); - __execlists_update_reg_state(ce, engine); + __execlists_update_reg_state(ce, engine, head); /* We've switched away, so this should be a no-op, but intent matters */ ce->lrc_desc |= CTX_DESC_FORCE_RESTORE; @@ -2878,16 +2878,17 @@ static void execlists_context_unpin(struct intel_context *ce) static void __execlists_update_reg_state(const struct intel_context *ce, -const struct intel_engine_cs *engine) +const struct intel_engine_cs *engine, +u32 head) { struct intel_ring *ring = ce->ring; u32 *regs = ce->lrc_reg_state; - GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->head)); + GEM_BUG_ON(!intel_ring_offset_valid(ring, head)); GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->tail)); regs[CTX_RING_START] = i915_ggtt_offset(ring->vma); - regs[CTX_RING_HEAD] = ring->head; + regs[CTX_RING_HEAD] = head; regs[CTX_RING_TAIL] = ring->tail; /* RPCS */ @@ -2916,7 +2917,7 @@ __execlists_context_pin(struct intel_context *ce, ce->lrc_desc = lrc_descriptor(ce, engine) | CTX_DESC_FORCE_RESTORE; ce->lrc_reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE; - __execlists_update_reg_state(ce, engine); + __execlists_update_reg_state(ce, engine, ce->ring->tail); return 0; } @@ -2941,7 +2942,7 @@ static void execlists_context_reset(struct intel_context *ce) /* Scrub away the garbage */ execlists_init_reg_state(ce->lrc_reg_state, ce, ce->engine, ce->ring, true); - __execlists_update_reg_state(ce, ce->engine); + __execlists_update_reg_state(ce, ce->engine, ce->ring->tail); ce->lrc_desc |= CTX_DESC_FORCE_RESTORE; } @@ -3538,6 +3539,7 @@ static void __execlists_reset(struct intel_engine_cs *engine, bool stalled) struct intel_engine_execlists * const execlists = &engine->execlists; struct intel_context *ce; struct i915_request *rq; + u32 head; mb(); /* paranoia: read the CSB pointers from after the reset */ clflush(execlists->csb_write); @@ -3565,15 +3567,15 @@ static void __execlists_reset(struct intel_engine_cs *engine, bool stalled) if (i915_request_completed(rq)) { /* Idle context; tidy up the ring so we can restart afresh */ - ce->ring->head = intel_ring_wrap(ce->ring, rq->tail); + head = intel_ring_wrap(ce->ring, rq->tail); goto out_replay; } /* Context has requests still in-flight; it should not be idle! */ GEM_BUG_ON(i915_active_is_idle(&ce->active)); rq = active_request(ce->timeline, rq); - ce->ring->head = intel_ring_wrap(ce->ring, rq->head); - GEM_BUG_ON(ce->ring->head == ce->ring->tail); + head = intel_ring_wrap(ce->ring, rq->head); + GEM_BUG_ON(head == ce->ring->tail); /* * If this request hasn't started yet, e.g. it is waiting on a @@ -3618,10 +3620,9 @@ stat
Re: [Intel-gfx] [PATCH 1/7] drm/i915/gt: Avoid resetting ring->head outside of its timeline mutex
Hi Chris, > We manipulate ring->head while active in i915_request_retire underneath > the timeline manipulation. We cannot rely on a stable ring->head outside > of the timeline->mutex, in particular while setting up the context for > resume and reset. > > Closes: https://gitlab.freedesktop.org/drm/intel/issues/1126 > Fixes: 0881954965e3 ("drm/i915: Introduce intel_context.pin_mutex for pin > management") > Fixes: e5dadff4b093 ("drm/i915: Protect request retirement with > timeline->mutex") > References: f3c0efc9fe7a ("drm/i915/execlists: Leave resetting ring to > intel_ring") > Signed-off-by: Chris Wilson > Cc: Matthew Auld > Cc: Tvrtko Ursulin > Cc: Mika Kuoppala looks OK to me: Reviewed-by: Andi Shyti Andi ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/7] drm/i915/gt: Avoid resetting ring->head outside of its timeline mutex
Chris Wilson writes: > We manipulate ring->head while active in i915_request_retire underneath > the timeline manipulation. We cannot rely on a stable ring->head outside > of the timeline->mutex, in particular while setting up the context for > resume and reset. This solves the immediate problem of ring->head sampling in execlist submission. Future work considerations are to make WRITE_ONCE to ring head even tho it is under timeline and then READ_ONCE on the other, non lockable places. Or atleast the READ_ONCE notation outside of lock. Reviewed-by: Mika Kuoppala > > Closes: https://gitlab.freedesktop.org/drm/intel/issues/1126 > Fixes: 0881954965e3 ("drm/i915: Introduce intel_context.pin_mutex for pin > management") > Fixes: e5dadff4b093 ("drm/i915: Protect request retirement with > timeline->mutex") > References: f3c0efc9fe7a ("drm/i915/execlists: Leave resetting ring to > intel_ring") > Signed-off-by: Chris Wilson > Cc: Matthew Auld > Cc: Tvrtko Ursulin > Cc: Mika Kuoppala > --- > drivers/gpu/drm/i915/gt/intel_lrc.c| 36 -- > drivers/gpu/drm/i915/gt/selftest_lrc.c | 2 +- > 2 files changed, 18 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c > b/drivers/gpu/drm/i915/gt/intel_lrc.c > index 929be03bbe7e..70d91ad923ef 100644 > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > @@ -235,7 +235,8 @@ static void execlists_init_reg_state(u32 *reg_state, >bool close); > static void > __execlists_update_reg_state(const struct intel_context *ce, > - const struct intel_engine_cs *engine); > + const struct intel_engine_cs *engine, > + u32 head); > > static void mark_eio(struct i915_request *rq) > { > @@ -1184,12 +1185,11 @@ static void reset_active(struct i915_request *rq, > head = rq->tail; > else > head = active_request(ce->timeline, rq)->head; > - ce->ring->head = intel_ring_wrap(ce->ring, head); > - intel_ring_update_space(ce->ring); > + head = intel_ring_wrap(ce->ring, head); > > /* Scrub the context image to prevent replaying the previous batch */ > restore_default_state(ce, engine); > - __execlists_update_reg_state(ce, engine); > + __execlists_update_reg_state(ce, engine, head); > > /* We've switched away, so this should be a no-op, but intent matters */ > ce->lrc_desc |= CTX_DESC_FORCE_RESTORE; > @@ -2878,16 +2878,17 @@ static void execlists_context_unpin(struct > intel_context *ce) > > static void > __execlists_update_reg_state(const struct intel_context *ce, > - const struct intel_engine_cs *engine) > + const struct intel_engine_cs *engine, > + u32 head) > { > struct intel_ring *ring = ce->ring; > u32 *regs = ce->lrc_reg_state; > > - GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->head)); > + GEM_BUG_ON(!intel_ring_offset_valid(ring, head)); > GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->tail)); > > regs[CTX_RING_START] = i915_ggtt_offset(ring->vma); > - regs[CTX_RING_HEAD] = ring->head; > + regs[CTX_RING_HEAD] = head; > regs[CTX_RING_TAIL] = ring->tail; > > /* RPCS */ > @@ -2916,7 +2917,7 @@ __execlists_context_pin(struct intel_context *ce, > > ce->lrc_desc = lrc_descriptor(ce, engine) | CTX_DESC_FORCE_RESTORE; > ce->lrc_reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE; > - __execlists_update_reg_state(ce, engine); > + __execlists_update_reg_state(ce, engine, ce->ring->tail); > > return 0; > } > @@ -2941,7 +2942,7 @@ static void execlists_context_reset(struct > intel_context *ce) > /* Scrub away the garbage */ > execlists_init_reg_state(ce->lrc_reg_state, >ce, ce->engine, ce->ring, true); > - __execlists_update_reg_state(ce, ce->engine); > + __execlists_update_reg_state(ce, ce->engine, ce->ring->tail); > > ce->lrc_desc |= CTX_DESC_FORCE_RESTORE; > } > @@ -3538,6 +3539,7 @@ static void __execlists_reset(struct intel_engine_cs > *engine, bool stalled) > struct intel_engine_execlists * const execlists = &engine->execlists; > struct intel_context *ce; > struct i915_request *rq; > + u32 head; > > mb(); /* paranoia: read the CSB pointers from after the reset */ > clflush(execlists->csb_write); > @@ -3565,15 +3567,15 @@ static void __execlists_reset(struct intel_engine_cs > *engine, bool stalled) > > if (i915_request_completed(rq)) { > /* Idle context; tidy up the ring so we can restart afresh */ > - ce->ring->head = intel_ring_wrap(ce->ring, rq->tail); > + head = intel_ring_wrap(ce->ring, rq->tail); > goto out_replay; > } > > /* Context has requests still in-flight;