Quoting Tvrtko Ursulin (2019-09-23 10:51:48)
>
> On 21/09/2019 10:55, Chris Wilson wrote:
> > If we are asked to submit a completed request, just move it onto the
> > active-list without modifying it's payload. If we try to emit the
> > modified payload of a completed request, we risk racing with the
> > ring->head update during retirement which may advance the head past our
> > breadcrumb and so we generate a warning for the emission being behind
> > the RING_HEAD.
> >
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/gt/intel_lrc.c | 45 +++++++++++++----------------
> > drivers/gpu/drm/i915/i915_request.c | 28 ++++++++++--------
> > drivers/gpu/drm/i915/i915_request.h | 2 +-
> > 3 files changed, 37 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 53e823d36b28..53bc4308793c 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -806,6 +806,9 @@ static bool can_merge_rq(const struct i915_request *prev,
> > GEM_BUG_ON(prev == next);
> > GEM_BUG_ON(!assert_priority_queue(prev, next));
> >
> > + if (i915_request_completed(next))
> > + return true;
> > +
>
> This reads very un-intuitive. Why would it always be okay to merge
> possibly unrelated requests? From which it follows it must be a hack of
> some kind - from which it follows it needs a comment to explain it.
We don't submit a known completed request, hence there is no context
switch.
> > @@ -1256,11 +1244,16 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> > GEM_BUG_ON(ve->siblings[0] != engine);
> > }
> >
> > - __i915_request_submit(rq);
> > - if (!i915_request_completed(rq)) {
> > + if (__i915_request_submit(rq)) {
> > submit = true;
> > last = rq;
> > }
> > +
> > + if (!submit) {
> > + spin_unlock(&ve->base.active.lock);
> > + rb = rb_first_cached(&execlists->virtual);
> > + continue;
> > + }
>
> This block also needs a comment I think. It's about skipping until first
> incomplete request in the queue?
It's the same logic as before. It's just saying having detected that we
have a bunch of veng requests, keep searching that tree in decreasing
priority order until it is no longer relevant.
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index 9bd8538b1907..db1a0048a753 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -377,9 +377,10 @@ __i915_request_await_execution(struct i915_request *rq,
> > return 0;
> > }
> >
> > -void __i915_request_submit(struct i915_request *request)
> > +bool __i915_request_submit(struct i915_request *request)
> > {
> > struct intel_engine_cs *engine = request->engine;
> > + bool result = false;
> >
> > GEM_TRACE("%s fence %llx:%lld, current %d\n",
> > engine->name,
> > @@ -389,6 +390,9 @@ void __i915_request_submit(struct i915_request *request)
> > GEM_BUG_ON(!irqs_disabled());
> > lockdep_assert_held(&engine->active.lock);
> >
> > + if (i915_request_completed(request))
> > + goto xfer;
>
> A comment here as well I think to explain what happens next with
> completed requests put on the active list. They just get removed during
> retire? Do they need to even be put on that list?
They get removed during retire. They must be removed from the priority
queue; and if they had been retired already they would not be in the
priority queue, ergo at this point they are completed and not retired.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
