Joonas Lahtinen <[email protected]> writes: > On Thu, 2017-10-19 at 17:39 +0300, Mika Kuoppala wrote: >> From: Mika Kuoppala <[email protected]> >> >> Instead of trusting that first available port is at index 0, >> use accessor to hide this. This is a preparation for a >> following patches where head can be at arbitrary location >> in the port array. >> >> v2: improved commit message, elsp_ready readability (Chris) >> v3: s/execlist_port_index/execlist_port (Chris) >> v4: rebase to new naming >> v5: fix port_next indexing >> >> Cc: Michał Winiarski <[email protected]> >> Cc: Joonas Lahtinen <[email protected]> >> Cc: Chris Wilson <[email protected]> >> Signed-off-by: Mika Kuoppala <[email protected]> > > <SNIP> > >> @@ -561,15 +563,20 @@ static void port_assign(struct execlist_port *port, >> static void i915_guc_dequeue(struct intel_engine_cs *engine) >> { >> struct intel_engine_execlists * const execlists = &engine->execlists; >> - struct execlist_port *port = execlists->port; >> + struct execlist_port *port; >> struct drm_i915_gem_request *last = NULL; >> - const struct execlist_port * const last_port = >> - &execlists->port[execlists->port_mask]; >> bool submit = false; >> struct rb_node *rb; >> >> - if (port_isset(port)) >> - port++; >> + port = execlists_port_head(execlists); >> + >> + /* >> + * We don't coalesce into last submitted port with guc. >> + * Find first free port, this is safe as we dont dequeue without >> + * atleast last port free. > > "at least" + "the" > > <SNIP> > >> @@ -557,6 +557,9 @@ static void execlists_dequeue(struct intel_engine_cs >> *engine) >> if (!rb) >> goto unlock; >> >> + port = execlists_port_head(execlists); >> + last = port_request(port); >> + >> if (last) { >> /* >> * Don't resubmit or switch until all outstanding >> @@ -564,7 +567,7 @@ static void execlists_dequeue(struct intel_engine_cs >> *engine) >> * know the next preemption status we see corresponds >> * to this ELSP update. >> */ >> - if (port_count(&port[0]) > 1) >> + if (port_count(port) > 1) >> goto unlock; >> >> if (can_preempt(engine) && >> @@ -598,7 +601,7 @@ static void execlists_dequeue(struct intel_engine_cs >> *engine) >> * the driver is unable to keep up the supply of new >> * work). >> */ >> - if (port_count(&port[1])) >> + if (port_count(execlists_port_next(execlists, port))) >> goto unlock; >> >> /* WaIdleLiteRestore:bdw,skl >> @@ -634,7 +637,7 @@ static void execlists_dequeue(struct intel_engine_cs >> *engine) >> * combine this request with the last, then we >> * are done. >> */ >> - if (port == last_port) { >> + if (port == execlists_port_tail(execlists)) { >> __list_del_many(&p->requests, >> &rq->priotree.link); > > Nothing to fix related to this patch, but I was sure next hunk was > going to escape my screen :) Maybe we need to cut the indents a bit. >
I have noticed the same. But I didn't feel like attacking this loop until everything is in place and working. >> @@ -890,7 +902,7 @@ static void intel_lrc_irq_handler(unsigned long data) >> } >> >> /* After the final element, the hw should be idle */ >> - GEM_BUG_ON(port_count(port) == 0 && >> + GEM_BUG_ON(port_count(execlists_port_head(execlists)) >> == 0 && >> !(status & GEN8_CTX_STATUS_ACTIVE_IDLE)); > > Why did you stop trusting port variable here? > We did assing it pre loop before. Now we do it inside the loop. Also I thought I made a favour for reader (and for the bug triager as GEM_BUG_ON might soon show condition in dmesg) to note that it is always the first port count we assert for idleness. -Mika _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
