Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx> writes:
> Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> writes:
>
>> 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.
>>
>
> 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.
My apologies. Now rereading this it is indeed that last port
count we need to check for hw idleness.
-Mika
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
