Re: [PATCH 2/3] drm/i915/execlists: Move request unwinding to a separate function

Chris Wilson
2017-09-25T13:18:19Z

Quoting Mika Kuoppala (2017-09-25 13:55:26)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
> > In the future, we will want to unwind requests following a preemption
> > point. This requires the same steps as for unwinding upon a reset, so
> > extract the existing code to a separate function for later use.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> > drivers/gpu/drm/i915/intel_lrc.c | 53 +++++++++++++++++++++++++---------------
> > 1 file changed, 33 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index 2c07f3c08bd3..c84831c7ea4a 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -348,6 +348,37 @@ lookup_priolist(struct intel_engine_cs *engine,
> > return ptr_pack_bits(p, first, 1);
> > }
> >
> > +static void unwind_wa_tail(struct drm_i915_gem_request *rq)
> > +{
> > + rq->tail = intel_ring_wrap(rq->ring,
> > + rq->wa_tail - WA_TAIL_DWORDS*sizeof(u32));
> > + assert_ring_tail_valid(rq->ring, rq->tail);
> > +}
> > +
> > +static void unwind_incomplete_requests(struct intel_engine_cs *engine)
> > +{
> > + struct drm_i915_gem_request *rq, *rn;
> > +
> > + lockdep_assert_held(&engine->timeline->lock);
> > + list_for_each_entry_safe_reverse(rq, rn,
> > + &engine->timeline->requests,
> > + link) {
> > + struct i915_priolist *p;
> > +
> > + if (i915_gem_request_completed(rq))
> > + return;
> > +
> > + __i915_gem_request_unsubmit(rq);
> > + unwind_wa_tail(rq);
>
> This here raised my attention. Why do you want
> to do this for all requests now?

It is a safety catch for preemption tasks. The worry being that if we
unwind a rq that was previously lite-restored, then on resubmission we
may not coalesce the requests again until perhaps a later interrupt.
It should never happen in practice. It requires a couple of reorderings
to happen at just the right moment, without triggering preemption...
-Chris