[Intel-gfx] [PATCH] drm/i915: Report request restarts for both execlists/guc

[Michel Thierry]

On 25/04/17 05:30, Chris Wilson wrote:
> On Tue, Apr 25, 2017 at 01:21:47PM +0100, Tvrtko Ursulin wrote:
>>
>> On 25/04/2017 11:38, Chris Wilson wrote:
>>> As we now share the execlist_port[] tracking for both execlists/guc, we
>>> can reset the inflight count on both and report which requests are being
>>> restarted.
>>>

Thanks, one less patch for me (and I arrived late to the party, I see 
it's already merged).

>>> Suggested-by: Michel Thierry <michel.thierry at intel.com>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Michel Thierry <michel.thierry at intel.com>
>>> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_lrc.c | 29 ++++++++++++++++-------------
>>> 1 file changed, 16 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>> index d3612969098f..961f4a2ad498 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -1147,14 +1147,11 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
>>>     return ret;
>>> }
>>>
>>> -static u32 port_seqno(struct execlist_port *port)
>>> -{
>>> -    return port->request ? port->request->global_seqno : 0;
>>> -}
>>> -
>>> static int gen8_init_common_ring(struct intel_engine_cs *engine)
>>> {
>>>     struct drm_i915_private *dev_priv = engine->i915;
>>> +    struct execlist_port *port = engine->execlist_port;
>>> +    unsigned int n;
>>>     int ret;
>>>
>>>     ret = intel_mocs_init_engine(engine);
>>> @@ -1175,16 +1172,22 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
>>>
>>>     /* After a GPU reset, we may have requests to replay */
>>>     clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
>>> -    if (!i915.enable_guc_submission && !execlists_elsp_idle(engine)) {
>>> -            DRM_DEBUG_DRIVER("Restarting %s from requests [0x%x, 0x%x]\n",
>>> -                             engine->name,
>>> -                             port_seqno(&engine->execlist_port[0]),
>>> -                             port_seqno(&engine->execlist_port[1]));
>>> -            engine->execlist_port[0].count = 0;
>>> -            engine->execlist_port[1].count = 0;
>>> -            execlists_submit_ports(engine);
>>> +
>>> +    for (n = 0; n < ARRAY_SIZE(engine->execlist_port); n++) {
>>> +            if (!port[n].request)
>>> +                    break;
>>
>> At some point we'll maybe want to start thinking about the
>> for_each_port_request or something.
>
> Something.
>
>>> +
>>> +            DRM_DEBUG_DRIVER("Restarting %s:%d from 0x%x\n",
>>> +                             engine->name, n,
>>> +                             port[n].request->global_seqno);
>>> +
>>> +            /* Discard the current inflight count */
>>> +            port[n].count = 0;
>>>     }
>>>
>>> +    if (!i915.enable_guc_submission && !execlists_elsp_idle(engine))
>>> +            execlists_submit_ports(engine);
>>> +
>>>     return 0;
>>> }
>>>
>>>
>>
>> Looks okay to me. Someone has plans to start using counts in guc mode?
>
> Spoilers. I moved the submission out of a few locks to reduce lock
> contention (queued_spin_lock_slowpath exists for guc!), makes the CPU
> numbers look better, but bxt/guc is still 3x higher latency. I just hope
> it is broken firmware.

Beating a dead horse?