[Intel-gfx] [PATCH 09/11] drm/i915/execlists: Refactor out can_merge_rq()

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-31 09:19:18)
> 
> On 30/01/2019 18:14, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-01-30 18:05:42)
> >>> @@ -766,8 +774,10 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> >>>                         * second request, and so we never need to tell the
> >>>                         * hardware about the first.
> >>>                         */
> >>> -                     if (last &&
> >>> -                         !can_merge_ctx(rq->hw_context, last->hw_context)) {
> >>> +                     if (last && !can_merge_rq(last, rq)) {
> >>> +                             if (last->hw_context == rq->hw_context)
> >>> +                                     goto done;
> >>
> >> I don't get this added check. AFAICS it will only trigger with GVT
> >> making it not consider filling both ports if possible.
> > 
> > Because we are preparing for can_merge_rq() deciding not to merge the
> > same context. If we do that we can't continue on to the next port and
> > must terminate the loop, violating the trick with the hint in the
> > process.
> > 
> > This changes due to the next patch, per-context freq and probably more
> > that I've forgotten.
> 
> After a second look, I noticed the existing GVT comment a bit lower down 
> which avoids populating port1 already.
> 
> Maybe one thing which would make sense is to re-arange these checks in 
> the order of "priority", like:
> 
>         if (last && !can_merge_rq(...)) {
>                 // naturally highest prio since it is impossible
>                 if (port == last_port)
>                         goto done;
>                 // 2nd highest to account for programming limitation
>                 else if (last->hw_context == rq->hw_context)
>                         goto done;

I was tempted to pull the last_port and context checks together.

>                 // GVT check simplified (I think - since we know last is either 
> different ctx or single submit)
>                 else if (ctx_single_port_submission(rq->hw_context))
>                         goto done;

And that's what I think and I tried to get gvt to clarify that their
checks are excessive. And I'll keep on suggesting that they remove their
poking around inside the scheduler... :-p

But it's definitely something I want out of sight.
-Chris