On Thu, 2017-10-19 at 17:39 +0300, Mika Kuoppala wrote:
> From: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>
>
> 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 <michal.winiarski@xxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>
<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.
> @@ -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?
Other than that, looks good to me.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
