| Message ID | 20201002083425.4605-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gt: Undo forced context restores after trivial preemptions | expand |
Chris Wilson <chris@chris-wilson.co.uk> writes: > We may try to preempt the currently executing request, only to find that > after unravelling all the dependencies that the original executing > context is still the earliest in the topological sort and re-submitted > back to HW (if we do detect some change in the ELSP that requires > re-submission). However, due to the way we check for wrap-around during > the unravelling, we mark any context that has been submitted just once > (i.e. with the rq->wa_tail set, but the ring->tail earlier) as > potentially wrapping and requiring a forced restore on resubmission. > This was expected to be not a problem, as it was anticipated that most > unwinding for preemption would result in a context switch and the few > that did not would be lost in the noise. It did not take long for > someone to find one particular workload where the cost of those extra > context restores was measurable. > > However, since we know the wa_tail is of fixed size, and we know that a > request must be larger than the wa_tail itself, we can safely maintain > the check for request wrapping and check against a slightly future point > in the ring that includes an expected wa_tail. (That is if the > ring->tail is already set to rq->wa_tail, including another 8 bytes in > the check does not invalidate the incremental wrap detection.) > > Fixes: 8ab3a3812aa9 ("drm/i915/gt: Incrementally check for rewinding") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Cc: Bruce Chang <yu.bruce.chang@intel.com> > Cc: Ramalingam C <ramalingam.c@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: <stable@vger.kernel.org> # v5.4+ > --- > drivers/gpu/drm/i915/gt/intel_lrc.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c > index 287537089c77..3aa05588834b 100644 > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > @@ -1140,9 +1140,8 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine) > > /* Check in case we rollback so far we wrap [size/2] */ My parser hickups. /* Make sure rollback doesn't make hardware think we went forward */ But for this patch, Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > if (intel_ring_direction(rq->ring, > - intel_ring_wrap(rq->ring, > - rq->tail), > - rq->ring->tail) > 0) > + rq->tail, > + rq->ring->tail + 8) > 0) > rq->context->lrc.desc |= CTX_DESC_FORCE_RESTORE; > > active = rq; > -- > 2.20.1
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index 287537089c77..3aa05588834b 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -1140,9 +1140,8 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine) /* Check in case we rollback so far we wrap [size/2] */ if (intel_ring_direction(rq->ring, - intel_ring_wrap(rq->ring, - rq->tail), - rq->ring->tail) > 0) + rq->tail, + rq->ring->tail + 8) > 0) rq->context->lrc.desc |= CTX_DESC_FORCE_RESTORE; active = rq;
We may try to preempt the currently executing request, only to find that after unravelling all the dependencies that the original executing context is still the earliest in the topological sort and re-submitted back to HW (if we do detect some change in the ELSP that requires re-submission). However, due to the way we check for wrap-around during the unravelling, we mark any context that has been submitted just once (i.e. with the rq->wa_tail set, but the ring->tail earlier) as potentially wrapping and requiring a forced restore on resubmission. This was expected to be not a problem, as it was anticipated that most unwinding for preemption would result in a context switch and the few that did not would be lost in the noise. It did not take long for someone to find one particular workload where the cost of those extra context restores was measurable. However, since we know the wa_tail is of fixed size, and we know that a request must be larger than the wa_tail itself, we can safely maintain the check for request wrapping and check against a slightly future point in the ring that includes an expected wa_tail. (That is if the ring->tail is already set to rq->wa_tail, including another 8 bytes in the check does not invalidate the incremental wrap detection.) Fixes: 8ab3a3812aa9 ("drm/i915/gt: Incrementally check for rewinding") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Bruce Chang <yu.bruce.chang@intel.com> Cc: Ramalingam C <ramalingam.c@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: <stable@vger.kernel.org> # v5.4+ --- drivers/gpu/drm/i915/gt/intel_lrc.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)