| Message ID | 20180925083205.2229-4-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] drm/i915/selftests: Smoketest preemption | expand |
On 25/09/2018 09:32, Chris Wilson wrote: > As we are about to allow ourselves to slightly bump the user priority > into a few different sublevels, packthose internal priority lists > into the same i915_priolist to keep the rbtree compact and avoid having > to allocate the default user priority even after the internal bumping. > The downside to having an requests[] rather than a node per active list, > is that we then have to walk over the empty higher priority lists. To > compensate, we track the active buckets and use a small bitmap to skip > over any inactive ones. > > v2: Use MASK of internal levels to simplify our usage. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/intel_engine_cs.c | 6 +- > drivers/gpu/drm/i915/intel_guc_submission.c | 12 ++- > drivers/gpu/drm/i915/intel_lrc.c | 87 ++++++++++++++------- > drivers/gpu/drm/i915/intel_ringbuffer.h | 13 ++- > 4 files changed, 80 insertions(+), 38 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index 217ed3ee1cab..83f2f7774c1f 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -1534,10 +1534,10 @@ void intel_engine_dump(struct intel_engine_cs *engine, > count = 0; > drm_printf(m, "\t\tQueue priority: %d\n", execlists->queue_priority); > for (rb = rb_first_cached(&execlists->queue); rb; rb = rb_next(rb)) { > - struct i915_priolist *p = > - rb_entry(rb, typeof(*p), node); > + struct i915_priolist *p = rb_entry(rb, typeof(*p), node); > + int i; > > - list_for_each_entry(rq, &p->requests, sched.link) { > + priolist_for_each_request(rq, p, i) { > if (count++ < MAX_REQUESTS_TO_SHOW - 1) > print_request(m, rq, "\t\tQ "); > else > diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c > index 4874a212754c..ac862b42f6a1 100644 > --- a/drivers/gpu/drm/i915/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/intel_guc_submission.c > @@ -746,30 +746,28 @@ static bool __guc_dequeue(struct intel_engine_cs *engine) > while ((rb = rb_first_cached(&execlists->queue))) { > struct i915_priolist *p = to_priolist(rb); > struct i915_request *rq, *rn; > + int i; > > - list_for_each_entry_safe(rq, rn, &p->requests, sched.link) { > + priolist_for_each_request_consume(rq, rn, p, i) { > if (last && rq->hw_context != last->hw_context) { > - if (port == last_port) { > - __list_del_many(&p->requests, > - &rq->sched.link); > + if (port == last_port) > goto done; > - } > > if (submit) > port_assign(port, last); > port++; > } > > - INIT_LIST_HEAD(&rq->sched.link); > + list_del_init(&rq->sched.link); > > __i915_request_submit(rq); > trace_i915_request_in(rq, port_index(port, execlists)); > + > last = rq; > submit = true; > } > > rb_erase_cached(&p->node, &execlists->queue); > - INIT_LIST_HEAD(&p->requests); > if (p->priority != I915_PRIORITY_NORMAL) > kmem_cache_free(engine->i915->priorities, p); > } > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index e8de250c3413..b1b3f67d1120 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -259,14 +259,49 @@ intel_lr_context_descriptor_update(struct i915_gem_context *ctx, > ce->lrc_desc = desc; > } > > -static struct i915_priolist * > +static void assert_priolists(struct intel_engine_execlists * const execlists, > + int queue_priority) > +{ > + struct rb_node *rb; > + int last_prio, i; > + > + if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) > + return; > + > + GEM_BUG_ON(rb_first_cached(&execlists->queue) != > + rb_first(&execlists->queue.rb_root)); > + > + last_prio = (queue_priority >> I915_USER_PRIORITY_SHIFT) + 1; > + for (rb = rb_first_cached(&execlists->queue); rb; rb = rb_next(rb)) { > + struct i915_priolist *p = to_priolist(rb); > + > + GEM_BUG_ON(p->priority >= last_prio); > + last_prio = p->priority; > + > + GEM_BUG_ON(!p->used); > + for (i = 0; i < ARRAY_SIZE(p->requests); i++) { > + if (list_empty(&p->requests[i])) > + continue; > + > + GEM_BUG_ON(!(p->used & BIT(i))); > + } > + } > +} > + > +static struct list_head * > lookup_priolist(struct intel_engine_cs *engine, int prio) > { > struct intel_engine_execlists * const execlists = &engine->execlists; > struct i915_priolist *p; > struct rb_node **parent, *rb; > bool first = true; > + int idx, i; > + > + assert_priolists(execlists, INT_MAX); > > + /* buckets sorted from highest [in slot 0] to lowest priority */ > + idx = I915_PRIORITY_MASK - (prio & I915_PRIORITY_MASK); How about: #define I915_INTERNAL_PRIO(p) (p & I915_PRIORITY_MASK) idx = I915_PRIORITY_COUNT - 1 - I915_INTERNAL_PRIO(prio); ? Regards, Tvrtko > + prio >>= I915_USER_PRIORITY_SHIFT; > if (unlikely(execlists->no_priolist)) > prio = I915_PRIORITY_NORMAL; > > @@ -283,7 +318,7 @@ lookup_priolist(struct intel_engine_cs *engine, int prio) > parent = &rb->rb_right; > first = false; > } else { > - return p; > + goto out; > } > } > > @@ -309,11 +344,15 @@ lookup_priolist(struct intel_engine_cs *engine, int prio) > } > > p->priority = prio; > - INIT_LIST_HEAD(&p->requests); > + for (i = 0; i < ARRAY_SIZE(p->requests); i++) > + INIT_LIST_HEAD(&p->requests[i]); > rb_link_node(&p->node, rb, parent); > rb_insert_color_cached(&p->node, &execlists->queue, first); > + p->used = 0; > > - return p; > +out: > + p->used |= BIT(idx); > + return &p->requests[idx]; > } > > static void unwind_wa_tail(struct i915_request *rq) > @@ -325,7 +364,7 @@ static void unwind_wa_tail(struct i915_request *rq) > static void __unwind_incomplete_requests(struct intel_engine_cs *engine) > { > struct i915_request *rq, *rn; > - struct i915_priolist *uninitialized_var(p); > + struct list_head *uninitialized_var(pl); > int last_prio = I915_PRIORITY_INVALID; > > lockdep_assert_held(&engine->timeline.lock); > @@ -342,12 +381,11 @@ static void __unwind_incomplete_requests(struct intel_engine_cs *engine) > GEM_BUG_ON(rq_prio(rq) == I915_PRIORITY_INVALID); > if (rq_prio(rq) != last_prio) { > last_prio = rq_prio(rq); > - p = lookup_priolist(engine, last_prio); > + pl = lookup_priolist(engine, last_prio); > } > GEM_BUG_ON(RB_EMPTY_ROOT(&engine->execlists.queue.rb_root)); > > - GEM_BUG_ON(p->priority != rq_prio(rq)); > - list_add(&rq->sched.link, &p->requests); > + list_add(&rq->sched.link, pl); > } > } > > @@ -665,8 +703,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine) > while ((rb = rb_first_cached(&execlists->queue))) { > struct i915_priolist *p = to_priolist(rb); > struct i915_request *rq, *rn; > + int i; > > - list_for_each_entry_safe(rq, rn, &p->requests, sched.link) { > + priolist_for_each_request_consume(rq, rn, p, i) { > /* > * Can we combine this request with the current port? > * It has to be the same context/ringbuffer and not > @@ -685,11 +724,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine) > * combine this request with the last, then we > * are done. > */ > - if (port == last_port) { > - __list_del_many(&p->requests, > - &rq->sched.link); > + if (port == last_port) > goto done; > - } > > /* > * If GVT overrides us we only ever submit > @@ -699,11 +735,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine) > * request) to the second port. > */ > if (ctx_single_port_submission(last->hw_context) || > - ctx_single_port_submission(rq->hw_context)) { > - __list_del_many(&p->requests, > - &rq->sched.link); > + ctx_single_port_submission(rq->hw_context)) > goto done; > - } > > GEM_BUG_ON(last->hw_context == rq->hw_context); > > @@ -714,15 +747,16 @@ static void execlists_dequeue(struct intel_engine_cs *engine) > GEM_BUG_ON(port_isset(port)); > } > > - INIT_LIST_HEAD(&rq->sched.link); > + list_del_init(&rq->sched.link); > + > __i915_request_submit(rq); > trace_i915_request_in(rq, port_index(port, execlists)); > + > last = rq; > submit = true; > } > > rb_erase_cached(&p->node, &execlists->queue); > - INIT_LIST_HEAD(&p->requests); > if (p->priority != I915_PRIORITY_NORMAL) > kmem_cache_free(engine->i915->priorities, p); > } > @@ -746,6 +780,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine) > */ > execlists->queue_priority = > port != execlists->port ? rq_prio(last) : INT_MIN; > + assert_priolists(execlists, execlists->queue_priority); > > if (submit) { > port_assign(port, last); > @@ -857,16 +892,16 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine) > /* Flush the queued requests to the timeline list (for retiring). */ > while ((rb = rb_first_cached(&execlists->queue))) { > struct i915_priolist *p = to_priolist(rb); > + int i; > > - list_for_each_entry_safe(rq, rn, &p->requests, sched.link) { > - INIT_LIST_HEAD(&rq->sched.link); > + priolist_for_each_request_consume(rq, rn, p, i) { > + list_del_init(&rq->sched.link); > > dma_fence_set_error(&rq->fence, -EIO); > __i915_request_submit(rq); > } > > rb_erase_cached(&p->node, &execlists->queue); > - INIT_LIST_HEAD(&p->requests); > if (p->priority != I915_PRIORITY_NORMAL) > kmem_cache_free(engine->i915->priorities, p); > } > @@ -1072,8 +1107,7 @@ static void queue_request(struct intel_engine_cs *engine, > struct i915_sched_node *node, > int prio) > { > - list_add_tail(&node->link, > - &lookup_priolist(engine, prio)->requests); > + list_add_tail(&node->link, lookup_priolist(engine, prio)); > } > > static void __update_queue(struct intel_engine_cs *engine, int prio) > @@ -1143,7 +1177,7 @@ sched_lock_engine(struct i915_sched_node *node, struct intel_engine_cs *locked) > static void execlists_schedule(struct i915_request *request, > const struct i915_sched_attr *attr) > { > - struct i915_priolist *uninitialized_var(pl); > + struct list_head *uninitialized_var(pl); > struct intel_engine_cs *engine, *last; > struct i915_dependency *dep, *p; > struct i915_dependency stack; > @@ -1242,8 +1276,7 @@ static void execlists_schedule(struct i915_request *request, > pl = lookup_priolist(engine, prio); > last = engine; > } > - GEM_BUG_ON(pl->priority != prio); > - list_move_tail(&node->link, &pl->requests); > + list_move_tail(&node->link, pl); > } else { > /* > * If the request is not in the priolist queue because > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 2dfa585712c2..1534de5bb852 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -190,11 +190,22 @@ enum intel_engine_id { > }; > > struct i915_priolist { > + struct list_head requests[I915_PRIORITY_COUNT]; > struct rb_node node; > - struct list_head requests; > + unsigned long used; > int priority; > }; > > +#define priolist_for_each_request(it, plist, idx) \ > + for (idx = 0; idx < ARRAY_SIZE((plist)->requests); idx++) \ > + list_for_each_entry(it, &(plist)->requests[idx], sched.link) > + > +#define priolist_for_each_request_consume(it, n, plist, idx) \ > + for (; (idx = ffs((plist)->used)); (plist)->used &= ~BIT(idx - 1)) \ > + list_for_each_entry_safe(it, n, \ > + &(plist)->requests[idx - 1], \ > + sched.link) > + > struct st_preempt_hang { > struct completion completion; > bool inject_hang; >
Quoting Tvrtko Ursulin (2018-09-25 10:48:44) > > On 25/09/2018 09:32, Chris Wilson wrote: > > As we are about to allow ourselves to slightly bump the user priority > > into a few different sublevels, packthose internal priority lists > > into the same i915_priolist to keep the rbtree compact and avoid having > > to allocate the default user priority even after the internal bumping. > > The downside to having an requests[] rather than a node per active list, > > is that we then have to walk over the empty higher priority lists. To > > compensate, we track the active buckets and use a small bitmap to skip > > over any inactive ones. > > > > v2: Use MASK of internal levels to simplify our usage. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > --- > > drivers/gpu/drm/i915/intel_engine_cs.c | 6 +- > > drivers/gpu/drm/i915/intel_guc_submission.c | 12 ++- > > drivers/gpu/drm/i915/intel_lrc.c | 87 ++++++++++++++------- > > drivers/gpu/drm/i915/intel_ringbuffer.h | 13 ++- > > 4 files changed, 80 insertions(+), 38 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > > index 217ed3ee1cab..83f2f7774c1f 100644 > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > > @@ -1534,10 +1534,10 @@ void intel_engine_dump(struct intel_engine_cs *engine, > > count = 0; > > drm_printf(m, "\t\tQueue priority: %d\n", execlists->queue_priority); > > for (rb = rb_first_cached(&execlists->queue); rb; rb = rb_next(rb)) { > > - struct i915_priolist *p = > > - rb_entry(rb, typeof(*p), node); > > + struct i915_priolist *p = rb_entry(rb, typeof(*p), node); > > + int i; > > > > - list_for_each_entry(rq, &p->requests, sched.link) { > > + priolist_for_each_request(rq, p, i) { > > if (count++ < MAX_REQUESTS_TO_SHOW - 1) > > print_request(m, rq, "\t\tQ "); > > else > > diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c > > index 4874a212754c..ac862b42f6a1 100644 > > --- a/drivers/gpu/drm/i915/intel_guc_submission.c > > +++ b/drivers/gpu/drm/i915/intel_guc_submission.c > > @@ -746,30 +746,28 @@ static bool __guc_dequeue(struct intel_engine_cs *engine) > > while ((rb = rb_first_cached(&execlists->queue))) { > > struct i915_priolist *p = to_priolist(rb); > > struct i915_request *rq, *rn; > > + int i; > > > > - list_for_each_entry_safe(rq, rn, &p->requests, sched.link) { > > + priolist_for_each_request_consume(rq, rn, p, i) { > > if (last && rq->hw_context != last->hw_context) { > > - if (port == last_port) { > > - __list_del_many(&p->requests, > > - &rq->sched.link); > > + if (port == last_port) > > goto done; > > - } > > > > if (submit) > > port_assign(port, last); > > port++; > > } > > > > - INIT_LIST_HEAD(&rq->sched.link); > > + list_del_init(&rq->sched.link); > > > > __i915_request_submit(rq); > > trace_i915_request_in(rq, port_index(port, execlists)); > > + > > last = rq; > > submit = true; > > } > > > > rb_erase_cached(&p->node, &execlists->queue); > > - INIT_LIST_HEAD(&p->requests); > > if (p->priority != I915_PRIORITY_NORMAL) > > kmem_cache_free(engine->i915->priorities, p); > > } > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > > index e8de250c3413..b1b3f67d1120 100644 > > --- a/drivers/gpu/drm/i915/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/intel_lrc.c > > @@ -259,14 +259,49 @@ intel_lr_context_descriptor_update(struct i915_gem_context *ctx, > > ce->lrc_desc = desc; > > } > > > > -static struct i915_priolist * > > +static void assert_priolists(struct intel_engine_execlists * const execlists, > > + int queue_priority) > > +{ > > + struct rb_node *rb; > > + int last_prio, i; > > + > > + if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) > > + return; > > + > > + GEM_BUG_ON(rb_first_cached(&execlists->queue) != > > + rb_first(&execlists->queue.rb_root)); > > + > > + last_prio = (queue_priority >> I915_USER_PRIORITY_SHIFT) + 1; > > + for (rb = rb_first_cached(&execlists->queue); rb; rb = rb_next(rb)) { > > + struct i915_priolist *p = to_priolist(rb); > > + > > + GEM_BUG_ON(p->priority >= last_prio); > > + last_prio = p->priority; > > + > > + GEM_BUG_ON(!p->used); > > + for (i = 0; i < ARRAY_SIZE(p->requests); i++) { > > + if (list_empty(&p->requests[i])) > > + continue; > > + > > + GEM_BUG_ON(!(p->used & BIT(i))); > > + } > > + } > > +} > > + > > +static struct list_head * > > lookup_priolist(struct intel_engine_cs *engine, int prio) > > { > > struct intel_engine_execlists * const execlists = &engine->execlists; > > struct i915_priolist *p; > > struct rb_node **parent, *rb; > > bool first = true; > > + int idx, i; > > + > > + assert_priolists(execlists, INT_MAX); > > > > + /* buckets sorted from highest [in slot 0] to lowest priority */ > > + idx = I915_PRIORITY_MASK - (prio & I915_PRIORITY_MASK); > > How about: > > #define I915_INTERNAL_PRIO(p) (p & I915_PRIORITY_MASK) Not convinced. It's the only place we use it (one use of MASK later to assert correct less). There's two views, here the user/kernel priority levels are not as important as what we are concerned about are the split lists with the chunking of lowest priority bits. At the extreme we could separate the two and make the chunks BITS_PER_LONG -- I think that's a waste and my goal was simply to avoid kmallocs for the common case of default user priority. The intention is that we don't even think about the user/internal split, and simply consider it an integrated field, with most positive priorities executing first and most negative last. -Chris
On 27/09/2018 14:05, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-09-25 10:48:44) >> >> On 25/09/2018 09:32, Chris Wilson wrote: >>> As we are about to allow ourselves to slightly bump the user priority >>> into a few different sublevels, packthose internal priority lists >>> into the same i915_priolist to keep the rbtree compact and avoid having >>> to allocate the default user priority even after the internal bumping. >>> The downside to having an requests[] rather than a node per active list, >>> is that we then have to walk over the empty higher priority lists. To >>> compensate, we track the active buckets and use a small bitmap to skip >>> over any inactive ones. >>> >>> v2: Use MASK of internal levels to simplify our usage. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_engine_cs.c | 6 +- >>> drivers/gpu/drm/i915/intel_guc_submission.c | 12 ++- >>> drivers/gpu/drm/i915/intel_lrc.c | 87 ++++++++++++++------- >>> drivers/gpu/drm/i915/intel_ringbuffer.h | 13 ++- >>> 4 files changed, 80 insertions(+), 38 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c >>> index 217ed3ee1cab..83f2f7774c1f 100644 >>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >>> @@ -1534,10 +1534,10 @@ void intel_engine_dump(struct intel_engine_cs *engine, >>> count = 0; >>> drm_printf(m, "\t\tQueue priority: %d\n", execlists->queue_priority); >>> for (rb = rb_first_cached(&execlists->queue); rb; rb = rb_next(rb)) { >>> - struct i915_priolist *p = >>> - rb_entry(rb, typeof(*p), node); >>> + struct i915_priolist *p = rb_entry(rb, typeof(*p), node); >>> + int i; >>> >>> - list_for_each_entry(rq, &p->requests, sched.link) { >>> + priolist_for_each_request(rq, p, i) { >>> if (count++ < MAX_REQUESTS_TO_SHOW - 1) >>> print_request(m, rq, "\t\tQ "); >>> else >>> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c >>> index 4874a212754c..ac862b42f6a1 100644 >>> --- a/drivers/gpu/drm/i915/intel_guc_submission.c >>> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c >>> @@ -746,30 +746,28 @@ static bool __guc_dequeue(struct intel_engine_cs *engine) >>> while ((rb = rb_first_cached(&execlists->queue))) { >>> struct i915_priolist *p = to_priolist(rb); >>> struct i915_request *rq, *rn; >>> + int i; >>> >>> - list_for_each_entry_safe(rq, rn, &p->requests, sched.link) { >>> + priolist_for_each_request_consume(rq, rn, p, i) { >>> if (last && rq->hw_context != last->hw_context) { >>> - if (port == last_port) { >>> - __list_del_many(&p->requests, >>> - &rq->sched.link); >>> + if (port == last_port) >>> goto done; >>> - } >>> >>> if (submit) >>> port_assign(port, last); >>> port++; >>> } >>> >>> - INIT_LIST_HEAD(&rq->sched.link); >>> + list_del_init(&rq->sched.link); >>> >>> __i915_request_submit(rq); >>> trace_i915_request_in(rq, port_index(port, execlists)); >>> + >>> last = rq; >>> submit = true; >>> } >>> >>> rb_erase_cached(&p->node, &execlists->queue); >>> - INIT_LIST_HEAD(&p->requests); >>> if (p->priority != I915_PRIORITY_NORMAL) >>> kmem_cache_free(engine->i915->priorities, p); >>> } >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >>> index e8de250c3413..b1b3f67d1120 100644 >>> --- a/drivers/gpu/drm/i915/intel_lrc.c >>> +++ b/drivers/gpu/drm/i915/intel_lrc.c >>> @@ -259,14 +259,49 @@ intel_lr_context_descriptor_update(struct i915_gem_context *ctx, >>> ce->lrc_desc = desc; >>> } >>> >>> -static struct i915_priolist * >>> +static void assert_priolists(struct intel_engine_execlists * const execlists, >>> + int queue_priority) >>> +{ >>> + struct rb_node *rb; >>> + int last_prio, i; >>> + >>> + if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) >>> + return; >>> + >>> + GEM_BUG_ON(rb_first_cached(&execlists->queue) != >>> + rb_first(&execlists->queue.rb_root)); >>> + >>> + last_prio = (queue_priority >> I915_USER_PRIORITY_SHIFT) + 1; >>> + for (rb = rb_first_cached(&execlists->queue); rb; rb = rb_next(rb)) { >>> + struct i915_priolist *p = to_priolist(rb); >>> + >>> + GEM_BUG_ON(p->priority >= last_prio); >>> + last_prio = p->priority; >>> + >>> + GEM_BUG_ON(!p->used); >>> + for (i = 0; i < ARRAY_SIZE(p->requests); i++) { >>> + if (list_empty(&p->requests[i])) >>> + continue; >>> + >>> + GEM_BUG_ON(!(p->used & BIT(i))); >>> + } >>> + } >>> +} >>> + >>> +static struct list_head * >>> lookup_priolist(struct intel_engine_cs *engine, int prio) >>> { >>> struct intel_engine_execlists * const execlists = &engine->execlists; >>> struct i915_priolist *p; >>> struct rb_node **parent, *rb; >>> bool first = true; >>> + int idx, i; >>> + >>> + assert_priolists(execlists, INT_MAX); >>> >>> + /* buckets sorted from highest [in slot 0] to lowest priority */ >>> + idx = I915_PRIORITY_MASK - (prio & I915_PRIORITY_MASK); >> >> How about: >> >> #define I915_INTERNAL_PRIO(p) (p & I915_PRIORITY_MASK) > > Not convinced. It's the only place we use it (one use of MASK later to > assert correct less). There's two views, here the user/kernel priority > levels are not as important as what we are concerned about are the split > lists with the chunking of lowest priority bits. At the extreme we could > separate the two and make the chunks BITS_PER_LONG -- I think that's a > waste and my goal was simply to avoid kmallocs for the common case of > default user priority. > > The intention is that we don't even think about the user/internal split, > and simply consider it an integrated field, with most positive > priorities executing first and most negative last. I just find MASK - INT confusing, those two data flavours do not match in my head. With the local macro I clarified that we are getting an INT from prio, and then the bit you did not quote I suggested we do not substract from the mask but from the count - 1. It is effectively the same thing just IMHO easier to understand while reading the code. You can skip the macro and decrement from the count, I'd be happy with that as well. Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-09-28 14:04:34) > > On 27/09/2018 14:05, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2018-09-25 10:48:44) > >> > >> On 25/09/2018 09:32, Chris Wilson wrote: > >>> As we are about to allow ourselves to slightly bump the user priority > >>> into a few different sublevels, packthose internal priority lists > >>> into the same i915_priolist to keep the rbtree compact and avoid having > >>> to allocate the default user priority even after the internal bumping. > >>> The downside to having an requests[] rather than a node per active list, > >>> is that we then have to walk over the empty higher priority lists. To > >>> compensate, we track the active buckets and use a small bitmap to skip > >>> over any inactive ones. > >>> > >>> v2: Use MASK of internal levels to simplify our usage. > >>> > >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>> --- > >>> drivers/gpu/drm/i915/intel_engine_cs.c | 6 +- > >>> drivers/gpu/drm/i915/intel_guc_submission.c | 12 ++- > >>> drivers/gpu/drm/i915/intel_lrc.c | 87 ++++++++++++++------- > >>> drivers/gpu/drm/i915/intel_ringbuffer.h | 13 ++- > >>> 4 files changed, 80 insertions(+), 38 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > >>> index 217ed3ee1cab..83f2f7774c1f 100644 > >>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c > >>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > >>> @@ -1534,10 +1534,10 @@ void intel_engine_dump(struct intel_engine_cs *engine, > >>> count = 0; > >>> drm_printf(m, "\t\tQueue priority: %d\n", execlists->queue_priority); > >>> for (rb = rb_first_cached(&execlists->queue); rb; rb = rb_next(rb)) { > >>> - struct i915_priolist *p = > >>> - rb_entry(rb, typeof(*p), node); > >>> + struct i915_priolist *p = rb_entry(rb, typeof(*p), node); > >>> + int i; > >>> > >>> - list_for_each_entry(rq, &p->requests, sched.link) { > >>> + priolist_for_each_request(rq, p, i) { > >>> if (count++ < MAX_REQUESTS_TO_SHOW - 1) > >>> print_request(m, rq, "\t\tQ "); > >>> else > >>> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c > >>> index 4874a212754c..ac862b42f6a1 100644 > >>> --- a/drivers/gpu/drm/i915/intel_guc_submission.c > >>> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c > >>> @@ -746,30 +746,28 @@ static bool __guc_dequeue(struct intel_engine_cs *engine) > >>> while ((rb = rb_first_cached(&execlists->queue))) { > >>> struct i915_priolist *p = to_priolist(rb); > >>> struct i915_request *rq, *rn; > >>> + int i; > >>> > >>> - list_for_each_entry_safe(rq, rn, &p->requests, sched.link) { > >>> + priolist_for_each_request_consume(rq, rn, p, i) { > >>> if (last && rq->hw_context != last->hw_context) { > >>> - if (port == last_port) { > >>> - __list_del_many(&p->requests, > >>> - &rq->sched.link); > >>> + if (port == last_port) > >>> goto done; > >>> - } > >>> > >>> if (submit) > >>> port_assign(port, last); > >>> port++; > >>> } > >>> > >>> - INIT_LIST_HEAD(&rq->sched.link); > >>> + list_del_init(&rq->sched.link); > >>> > >>> __i915_request_submit(rq); > >>> trace_i915_request_in(rq, port_index(port, execlists)); > >>> + > >>> last = rq; > >>> submit = true; > >>> } > >>> > >>> rb_erase_cached(&p->node, &execlists->queue); > >>> - INIT_LIST_HEAD(&p->requests); > >>> if (p->priority != I915_PRIORITY_NORMAL) > >>> kmem_cache_free(engine->i915->priorities, p); > >>> } > >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > >>> index e8de250c3413..b1b3f67d1120 100644 > >>> --- a/drivers/gpu/drm/i915/intel_lrc.c > >>> +++ b/drivers/gpu/drm/i915/intel_lrc.c > >>> @@ -259,14 +259,49 @@ intel_lr_context_descriptor_update(struct i915_gem_context *ctx, > >>> ce->lrc_desc = desc; > >>> } > >>> > >>> -static struct i915_priolist * > >>> +static void assert_priolists(struct intel_engine_execlists * const execlists, > >>> + int queue_priority) > >>> +{ > >>> + struct rb_node *rb; > >>> + int last_prio, i; > >>> + > >>> + if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) > >>> + return; > >>> + > >>> + GEM_BUG_ON(rb_first_cached(&execlists->queue) != > >>> + rb_first(&execlists->queue.rb_root)); > >>> + > >>> + last_prio = (queue_priority >> I915_USER_PRIORITY_SHIFT) + 1; > >>> + for (rb = rb_first_cached(&execlists->queue); rb; rb = rb_next(rb)) { > >>> + struct i915_priolist *p = to_priolist(rb); > >>> + > >>> + GEM_BUG_ON(p->priority >= last_prio); > >>> + last_prio = p->priority; > >>> + > >>> + GEM_BUG_ON(!p->used); > >>> + for (i = 0; i < ARRAY_SIZE(p->requests); i++) { > >>> + if (list_empty(&p->requests[i])) > >>> + continue; > >>> + > >>> + GEM_BUG_ON(!(p->used & BIT(i))); > >>> + } > >>> + } > >>> +} > >>> + > >>> +static struct list_head * > >>> lookup_priolist(struct intel_engine_cs *engine, int prio) > >>> { > >>> struct intel_engine_execlists * const execlists = &engine->execlists; > >>> struct i915_priolist *p; > >>> struct rb_node **parent, *rb; > >>> bool first = true; > >>> + int idx, i; > >>> + > >>> + assert_priolists(execlists, INT_MAX); > >>> > >>> + /* buckets sorted from highest [in slot 0] to lowest priority */ > >>> + idx = I915_PRIORITY_MASK - (prio & I915_PRIORITY_MASK); > >> > >> How about: > >> > >> #define I915_INTERNAL_PRIO(p) (p & I915_PRIORITY_MASK) > > > > Not convinced. It's the only place we use it (one use of MASK later to > > assert correct less). There's two views, here the user/kernel priority > > levels are not as important as what we are concerned about are the split > > lists with the chunking of lowest priority bits. At the extreme we could > > separate the two and make the chunks BITS_PER_LONG -- I think that's a > > waste and my goal was simply to avoid kmallocs for the common case of > > default user priority. > > > > The intention is that we don't even think about the user/internal split, > > and simply consider it an integrated field, with most positive > > priorities executing first and most negative last. > > I just find MASK - INT confusing, those two data flavours do not match > in my head. With the local macro I clarified that we are getting an INT > from prio, and then the bit you did not quote I suggested we do not > substract from the mask but from the count - 1. It is effectively the > same thing just IMHO easier to understand while reading the code. You > can skip the macro and decrement from the count, I'd be happy with that > as well. I swear that COUNT-1 was the bit you complained about last time ;) I put the comment there to explain why we reverse the index, as leftmost is highest priority. Preemptive r-b for COUNT-1 - (prio & I915_PRIORITY_MASK) then? -Chris
On 28/09/2018 14:08, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-09-28 14:04:34) >> >> On 27/09/2018 14:05, Chris Wilson wrote: >>> Quoting Tvrtko Ursulin (2018-09-25 10:48:44) >>>> >>>> On 25/09/2018 09:32, Chris Wilson wrote: >>>>> As we are about to allow ourselves to slightly bump the user priority >>>>> into a few different sublevels, packthose internal priority lists >>>>> into the same i915_priolist to keep the rbtree compact and avoid having >>>>> to allocate the default user priority even after the internal bumping. >>>>> The downside to having an requests[] rather than a node per active list, >>>>> is that we then have to walk over the empty higher priority lists. To >>>>> compensate, we track the active buckets and use a small bitmap to skip >>>>> over any inactive ones. >>>>> >>>>> v2: Use MASK of internal levels to simplify our usage. >>>>> >>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/intel_engine_cs.c | 6 +- >>>>> drivers/gpu/drm/i915/intel_guc_submission.c | 12 ++- >>>>> drivers/gpu/drm/i915/intel_lrc.c | 87 ++++++++++++++------- >>>>> drivers/gpu/drm/i915/intel_ringbuffer.h | 13 ++- >>>>> 4 files changed, 80 insertions(+), 38 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c >>>>> index 217ed3ee1cab..83f2f7774c1f 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >>>>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >>>>> @@ -1534,10 +1534,10 @@ void intel_engine_dump(struct intel_engine_cs *engine, >>>>> count = 0; >>>>> drm_printf(m, "\t\tQueue priority: %d\n", execlists->queue_priority); >>>>> for (rb = rb_first_cached(&execlists->queue); rb; rb = rb_next(rb)) { >>>>> - struct i915_priolist *p = >>>>> - rb_entry(rb, typeof(*p), node); >>>>> + struct i915_priolist *p = rb_entry(rb, typeof(*p), node); >>>>> + int i; >>>>> >>>>> - list_for_each_entry(rq, &p->requests, sched.link) { >>>>> + priolist_for_each_request(rq, p, i) { >>>>> if (count++ < MAX_REQUESTS_TO_SHOW - 1) >>>>> print_request(m, rq, "\t\tQ "); >>>>> else >>>>> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c >>>>> index 4874a212754c..ac862b42f6a1 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_guc_submission.c >>>>> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c >>>>> @@ -746,30 +746,28 @@ static bool __guc_dequeue(struct intel_engine_cs *engine) >>>>> while ((rb = rb_first_cached(&execlists->queue))) { >>>>> struct i915_priolist *p = to_priolist(rb); >>>>> struct i915_request *rq, *rn; >>>>> + int i; >>>>> >>>>> - list_for_each_entry_safe(rq, rn, &p->requests, sched.link) { >>>>> + priolist_for_each_request_consume(rq, rn, p, i) { >>>>> if (last && rq->hw_context != last->hw_context) { >>>>> - if (port == last_port) { >>>>> - __list_del_many(&p->requests, >>>>> - &rq->sched.link); >>>>> + if (port == last_port) >>>>> goto done; >>>>> - } >>>>> >>>>> if (submit) >>>>> port_assign(port, last); >>>>> port++; >>>>> } >>>>> >>>>> - INIT_LIST_HEAD(&rq->sched.link); >>>>> + list_del_init(&rq->sched.link); >>>>> >>>>> __i915_request_submit(rq); >>>>> trace_i915_request_in(rq, port_index(port, execlists)); >>>>> + >>>>> last = rq; >>>>> submit = true; >>>>> } >>>>> >>>>> rb_erase_cached(&p->node, &execlists->queue); >>>>> - INIT_LIST_HEAD(&p->requests); >>>>> if (p->priority != I915_PRIORITY_NORMAL) >>>>> kmem_cache_free(engine->i915->priorities, p); >>>>> } >>>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >>>>> index e8de250c3413..b1b3f67d1120 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_lrc.c >>>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c >>>>> @@ -259,14 +259,49 @@ intel_lr_context_descriptor_update(struct i915_gem_context *ctx, >>>>> ce->lrc_desc = desc; >>>>> } >>>>> >>>>> -static struct i915_priolist * >>>>> +static void assert_priolists(struct intel_engine_execlists * const execlists, >>>>> + int queue_priority) >>>>> +{ >>>>> + struct rb_node *rb; >>>>> + int last_prio, i; >>>>> + >>>>> + if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) >>>>> + return; >>>>> + >>>>> + GEM_BUG_ON(rb_first_cached(&execlists->queue) != >>>>> + rb_first(&execlists->queue.rb_root)); >>>>> + >>>>> + last_prio = (queue_priority >> I915_USER_PRIORITY_SHIFT) + 1; >>>>> + for (rb = rb_first_cached(&execlists->queue); rb; rb = rb_next(rb)) { >>>>> + struct i915_priolist *p = to_priolist(rb); >>>>> + >>>>> + GEM_BUG_ON(p->priority >= last_prio); >>>>> + last_prio = p->priority; >>>>> + >>>>> + GEM_BUG_ON(!p->used); >>>>> + for (i = 0; i < ARRAY_SIZE(p->requests); i++) { >>>>> + if (list_empty(&p->requests[i])) >>>>> + continue; >>>>> + >>>>> + GEM_BUG_ON(!(p->used & BIT(i))); >>>>> + } >>>>> + } >>>>> +} >>>>> + >>>>> +static struct list_head * >>>>> lookup_priolist(struct intel_engine_cs *engine, int prio) >>>>> { >>>>> struct intel_engine_execlists * const execlists = &engine->execlists; >>>>> struct i915_priolist *p; >>>>> struct rb_node **parent, *rb; >>>>> bool first = true; >>>>> + int idx, i; >>>>> + >>>>> + assert_priolists(execlists, INT_MAX); >>>>> >>>>> + /* buckets sorted from highest [in slot 0] to lowest priority */ >>>>> + idx = I915_PRIORITY_MASK - (prio & I915_PRIORITY_MASK); >>>> >>>> How about: >>>> >>>> #define I915_INTERNAL_PRIO(p) (p & I915_PRIORITY_MASK) >>> >>> Not convinced. It's the only place we use it (one use of MASK later to >>> assert correct less). There's two views, here the user/kernel priority >>> levels are not as important as what we are concerned about are the split >>> lists with the chunking of lowest priority bits. At the extreme we could >>> separate the two and make the chunks BITS_PER_LONG -- I think that's a >>> waste and my goal was simply to avoid kmallocs for the common case of >>> default user priority. >>> >>> The intention is that we don't even think about the user/internal split, >>> and simply consider it an integrated field, with most positive >>> priorities executing first and most negative last. >> >> I just find MASK - INT confusing, those two data flavours do not match >> in my head. With the local macro I clarified that we are getting an INT >> from prio, and then the bit you did not quote I suggested we do not >> substract from the mask but from the count - 1. It is effectively the >> same thing just IMHO easier to understand while reading the code. You >> can skip the macro and decrement from the count, I'd be happy with that >> as well. > > I swear that COUNT-1 was the bit you complained about last time ;) Blast.. I complained that count was one when it actually should be zero, and you were also negating the mask for extra confusion.. no big harm done I hope? > I put the comment there to explain why we reverse the index, as leftmost > is highest priority. > > Preemptive r-b for COUNT-1 - (prio & I915_PRIORITY_MASK) then? Yes r-b. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index 217ed3ee1cab..83f2f7774c1f 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -1534,10 +1534,10 @@ void intel_engine_dump(struct intel_engine_cs *engine, count = 0; drm_printf(m, "\t\tQueue priority: %d\n", execlists->queue_priority); for (rb = rb_first_cached(&execlists->queue); rb; rb = rb_next(rb)) { - struct i915_priolist *p = - rb_entry(rb, typeof(*p), node); + struct i915_priolist *p = rb_entry(rb, typeof(*p), node); + int i; - list_for_each_entry(rq, &p->requests, sched.link) { + priolist_for_each_request(rq, p, i) { if (count++ < MAX_REQUESTS_TO_SHOW - 1) print_request(m, rq, "\t\tQ "); else diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c index 4874a212754c..ac862b42f6a1 100644 --- a/drivers/gpu/drm/i915/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/intel_guc_submission.c @@ -746,30 +746,28 @@ static bool __guc_dequeue(struct intel_engine_cs *engine) while ((rb = rb_first_cached(&execlists->queue))) { struct i915_priolist *p = to_priolist(rb); struct i915_request *rq, *rn; + int i; - list_for_each_entry_safe(rq, rn, &p->requests, sched.link) { + priolist_for_each_request_consume(rq, rn, p, i) { if (last && rq->hw_context != last->hw_context) { - if (port == last_port) { - __list_del_many(&p->requests, - &rq->sched.link); + if (port == last_port) goto done; - } if (submit) port_assign(port, last); port++; } - INIT_LIST_HEAD(&rq->sched.link); + list_del_init(&rq->sched.link); __i915_request_submit(rq); trace_i915_request_in(rq, port_index(port, execlists)); + last = rq; submit = true; } rb_erase_cached(&p->node, &execlists->queue); - INIT_LIST_HEAD(&p->requests); if (p->priority != I915_PRIORITY_NORMAL) kmem_cache_free(engine->i915->priorities, p); } diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index e8de250c3413..b1b3f67d1120 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -259,14 +259,49 @@ intel_lr_context_descriptor_update(struct i915_gem_context *ctx, ce->lrc_desc = desc; } -static struct i915_priolist * +static void assert_priolists(struct intel_engine_execlists * const execlists, + int queue_priority) +{ + struct rb_node *rb; + int last_prio, i; + + if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) + return; + + GEM_BUG_ON(rb_first_cached(&execlists->queue) != + rb_first(&execlists->queue.rb_root)); + + last_prio = (queue_priority >> I915_USER_PRIORITY_SHIFT) + 1; + for (rb = rb_first_cached(&execlists->queue); rb; rb = rb_next(rb)) { + struct i915_priolist *p = to_priolist(rb); + + GEM_BUG_ON(p->priority >= last_prio); + last_prio = p->priority; + + GEM_BUG_ON(!p->used); + for (i = 0; i < ARRAY_SIZE(p->requests); i++) { + if (list_empty(&p->requests[i])) + continue; + + GEM_BUG_ON(!(p->used & BIT(i))); + } + } +} + +static struct list_head * lookup_priolist(struct intel_engine_cs *engine, int prio) { struct intel_engine_execlists * const execlists = &engine->execlists; struct i915_priolist *p; struct rb_node **parent, *rb; bool first = true; + int idx, i; + + assert_priolists(execlists, INT_MAX); + /* buckets sorted from highest [in slot 0] to lowest priority */ + idx = I915_PRIORITY_MASK - (prio & I915_PRIORITY_MASK); + prio >>= I915_USER_PRIORITY_SHIFT; if (unlikely(execlists->no_priolist)) prio = I915_PRIORITY_NORMAL; @@ -283,7 +318,7 @@ lookup_priolist(struct intel_engine_cs *engine, int prio) parent = &rb->rb_right; first = false; } else { - return p; + goto out; } } @@ -309,11 +344,15 @@ lookup_priolist(struct intel_engine_cs *engine, int prio) } p->priority = prio; - INIT_LIST_HEAD(&p->requests); + for (i = 0; i < ARRAY_SIZE(p->requests); i++) + INIT_LIST_HEAD(&p->requests[i]); rb_link_node(&p->node, rb, parent); rb_insert_color_cached(&p->node, &execlists->queue, first); + p->used = 0; - return p; +out: + p->used |= BIT(idx); + return &p->requests[idx]; } static void unwind_wa_tail(struct i915_request *rq) @@ -325,7 +364,7 @@ static void unwind_wa_tail(struct i915_request *rq) static void __unwind_incomplete_requests(struct intel_engine_cs *engine) { struct i915_request *rq, *rn; - struct i915_priolist *uninitialized_var(p); + struct list_head *uninitialized_var(pl); int last_prio = I915_PRIORITY_INVALID; lockdep_assert_held(&engine->timeline.lock); @@ -342,12 +381,11 @@ static void __unwind_incomplete_requests(struct intel_engine_cs *engine) GEM_BUG_ON(rq_prio(rq) == I915_PRIORITY_INVALID); if (rq_prio(rq) != last_prio) { last_prio = rq_prio(rq); - p = lookup_priolist(engine, last_prio); + pl = lookup_priolist(engine, last_prio); } GEM_BUG_ON(RB_EMPTY_ROOT(&engine->execlists.queue.rb_root)); - GEM_BUG_ON(p->priority != rq_prio(rq)); - list_add(&rq->sched.link, &p->requests); + list_add(&rq->sched.link, pl); } } @@ -665,8 +703,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine) while ((rb = rb_first_cached(&execlists->queue))) { struct i915_priolist *p = to_priolist(rb); struct i915_request *rq, *rn; + int i; - list_for_each_entry_safe(rq, rn, &p->requests, sched.link) { + priolist_for_each_request_consume(rq, rn, p, i) { /* * Can we combine this request with the current port? * It has to be the same context/ringbuffer and not @@ -685,11 +724,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine) * combine this request with the last, then we * are done. */ - if (port == last_port) { - __list_del_many(&p->requests, - &rq->sched.link); + if (port == last_port) goto done; - } /* * If GVT overrides us we only ever submit @@ -699,11 +735,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine) * request) to the second port. */ if (ctx_single_port_submission(last->hw_context) || - ctx_single_port_submission(rq->hw_context)) { - __list_del_many(&p->requests, - &rq->sched.link); + ctx_single_port_submission(rq->hw_context)) goto done; - } GEM_BUG_ON(last->hw_context == rq->hw_context); @@ -714,15 +747,16 @@ static void execlists_dequeue(struct intel_engine_cs *engine) GEM_BUG_ON(port_isset(port)); } - INIT_LIST_HEAD(&rq->sched.link); + list_del_init(&rq->sched.link); + __i915_request_submit(rq); trace_i915_request_in(rq, port_index(port, execlists)); + last = rq; submit = true; } rb_erase_cached(&p->node, &execlists->queue); - INIT_LIST_HEAD(&p->requests); if (p->priority != I915_PRIORITY_NORMAL) kmem_cache_free(engine->i915->priorities, p); } @@ -746,6 +780,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine) */ execlists->queue_priority = port != execlists->port ? rq_prio(last) : INT_MIN; + assert_priolists(execlists, execlists->queue_priority); if (submit) { port_assign(port, last); @@ -857,16 +892,16 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine) /* Flush the queued requests to the timeline list (for retiring). */ while ((rb = rb_first_cached(&execlists->queue))) { struct i915_priolist *p = to_priolist(rb); + int i; - list_for_each_entry_safe(rq, rn, &p->requests, sched.link) { - INIT_LIST_HEAD(&rq->sched.link); + priolist_for_each_request_consume(rq, rn, p, i) { + list_del_init(&rq->sched.link); dma_fence_set_error(&rq->fence, -EIO); __i915_request_submit(rq); } rb_erase_cached(&p->node, &execlists->queue); - INIT_LIST_HEAD(&p->requests); if (p->priority != I915_PRIORITY_NORMAL) kmem_cache_free(engine->i915->priorities, p); } @@ -1072,8 +1107,7 @@ static void queue_request(struct intel_engine_cs *engine, struct i915_sched_node *node, int prio) { - list_add_tail(&node->link, - &lookup_priolist(engine, prio)->requests); + list_add_tail(&node->link, lookup_priolist(engine, prio)); } static void __update_queue(struct intel_engine_cs *engine, int prio) @@ -1143,7 +1177,7 @@ sched_lock_engine(struct i915_sched_node *node, struct intel_engine_cs *locked) static void execlists_schedule(struct i915_request *request, const struct i915_sched_attr *attr) { - struct i915_priolist *uninitialized_var(pl); + struct list_head *uninitialized_var(pl); struct intel_engine_cs *engine, *last; struct i915_dependency *dep, *p; struct i915_dependency stack; @@ -1242,8 +1276,7 @@ static void execlists_schedule(struct i915_request *request, pl = lookup_priolist(engine, prio); last = engine; } - GEM_BUG_ON(pl->priority != prio); - list_move_tail(&node->link, &pl->requests); + list_move_tail(&node->link, pl); } else { /* * If the request is not in the priolist queue because diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 2dfa585712c2..1534de5bb852 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -190,11 +190,22 @@ enum intel_engine_id { }; struct i915_priolist { + struct list_head requests[I915_PRIORITY_COUNT]; struct rb_node node; - struct list_head requests; + unsigned long used; int priority; }; +#define priolist_for_each_request(it, plist, idx) \ + for (idx = 0; idx < ARRAY_SIZE((plist)->requests); idx++) \ + list_for_each_entry(it, &(plist)->requests[idx], sched.link) + +#define priolist_for_each_request_consume(it, n, plist, idx) \ + for (; (idx = ffs((plist)->used)); (plist)->used &= ~BIT(idx - 1)) \ + list_for_each_entry_safe(it, n, \ + &(plist)->requests[idx - 1], \ + sched.link) + struct st_preempt_hang { struct completion completion; bool inject_hang;
As we are about to allow ourselves to slightly bump the user priority into a few different sublevels, packthose internal priority lists into the same i915_priolist to keep the rbtree compact and avoid having to allocate the default user priority even after the internal bumping. The downside to having an requests[] rather than a node per active list, is that we then have to walk over the empty higher priority lists. To compensate, we track the active buckets and use a small bitmap to skip over any inactive ones. v2: Use MASK of internal levels to simplify our usage. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/intel_engine_cs.c | 6 +- drivers/gpu/drm/i915/intel_guc_submission.c | 12 ++- drivers/gpu/drm/i915/intel_lrc.c | 87 ++++++++++++++------- drivers/gpu/drm/i915/intel_ringbuffer.h | 13 ++- 4 files changed, 80 insertions(+), 38 deletions(-)