[Intel-gfx] [PATCH v2] drm/i915: Only enqueue already completed requests

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-09-23 11:44:01)
> 
> On 23/09/2019 11:32, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 0a4812ebd184..8c1ea5c315ac 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -799,6 +799,17 @@ static bool can_merge_rq(const struct i915_request *prev,
> >       GEM_BUG_ON(prev == next);
> >       GEM_BUG_ON(!assert_priority_queue(prev, next));
> >   
> > +     /*
> > +      * We not submit known completed requests. Therefore if the next
> 
> "We _do_ not submit"?

Verbs are for the weak.

> > +      * request is already completed, we can pretend to merge it in
> > +      * with the previous context (and we will skip updating the ELSP
> > +      * and tracking). Thus hopefully keeping the ELSP full with active
> > +      * contexts, despite the best efforts of preempt-to-busy to confuse
> > +      * us.
> > +      */
> > +     if (i915_request_completed(next))
> > +             return true;
> 
> It works with the current use of can_merge_rq but leaves a bit of a 
> concern for the future. I did not come up with any interesting 
> GEM_BUG_ONs to add. I was thinking along the lines of making sure we 
> never end up coalescing different contexts to the same port. But no 
> ideas how to do that.

That should just be

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 96dd95d8252c..924cd51e1140 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1322,7 +1322,13 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
                                if (!merge) {
                                        *port = execlists_schedule_in(last, port - execlists->pending);
                                        port++;
+                                       last = NULL;
                                }
+
+                               GEM_BUG_ON(last &&
+                                          !can_merge_ctx(last->hw_context,
+                                                         rq->hw_context));
+
                                submit = true;
                                last = rq;
                        }

> > @@ -1249,11 +1245,23 @@ 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;
> >                       }
> > +
> > +                     /*
> > +                      * Hmm, we have a bunch of virtual engine requests,
> > +                      * but the first one was already complete (thanks
> 
> Complete or completed? Not sure myself..

Hmm. I suppose you can treat it as complete == "whole", completed ==
"finished". +d for consistency.
-Chris