[Intel-gfx] [PATCH] drm/i915/guc: Fix request re-submission after reset

[Oscar Mateo]

On 03/09/2017 08:50 AM, Tvrtko Ursulin wrote:
>
> On 09/03/2017 08:42, Oscar Mateo wrote:
>> On 03/09/2017 02:05 AM, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>
>>> In order to ensure no missed interrupts we must first re-direct
>>> the interrupts to GuC, and only then re-submit the requests to
>>> be replayed after a GPU reset. Otherwise context switch can fire
>>> before GuC has been set up to receive it triggering more hangs.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
>>> Cc: Oscar Mateo <oscar.mateo at intel.com>
>>> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>>> Cc: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 58
>>> +++++++++++++++++++++++++++---
>>>   drivers/gpu/drm/i915/intel_guc_loader.c    | 47
>>> ------------------------
>>>   2 files changed, 54 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> index beb38e30d0e9..5b8ec0fab563 100644
>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> @@ -936,6 +936,52 @@ static void guc_reset_wq(struct i915_guc_client
>>> *client)
>>>       client->wq_tail = 0;
>>>   }
>>>
>> <SNIP>
>>>   int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
>>>   {
>>>       struct intel_guc *guc = &dev_priv->guc;
>>> @@ -953,13 +999,17 @@ int i915_guc_submission_enable(struct
>>> drm_i915_private *dev_priv)
>>>         /* Take over from manual control of ELSP (execlists) */
>>>       for_each_engine(engine, dev_priv, id) {
>>> -        const int wqi_size = sizeof(struct guc_wq_item);
>>> -        struct drm_i915_gem_request *rq;
>>> -
>>>           engine->submit_request = i915_guc_submit;
>>>           engine->schedule = NULL;
>>> +    }
>>> +
>>> +    guc_interrupts_capture(dev_priv);
>>> +
>>> +    /* Replay the current set of previously submitted requests */
>>> +    for_each_engine(engine, dev_priv, id) {
>>> +        const int wqi_size = sizeof(struct guc_wq_item);
>>> +        struct drm_i915_gem_request *rq;
>>>
>> Don't you want to move the guc_interrupts_release to
>> i915_guc_submission_disable as well?
>
> I can't spot anything broken in that path. We never go in that 
> direction with the live submission so why do you think it is needed?
>
> Regards,
>
> Tvrtko
Just code symmetry: if we are leaving i915_guc_submission_enable to 
capture the interrupts, why doesn't the disable also releases them? 
Maybe it's not very important now, but it makes a lot more sense with my 
series to do proper unwinding of the whole path (I can tackle it there 
if you prefer).