[Intel-gfx] [PATCH 08/10] drm/i915/guc: Fix GuC HW/SW state cleanup in unload path

[Oscar Mateo]

On 09/21/2017 12:09 PM, Sagar Arun Kamble wrote:
>
>
> On 9/22/2017 12:03 AM, Oscar Mateo wrote:
>> <SNIP>
>>
>>
>> On 09/17/2017 05:17 AM, Sagar Arun Kamble wrote:
>>> Teardown of GuC HW/SW state was not properly done in unload path.
>>> guc_submission_disable was called as part of intel_uc_fini_hw which
>>> happens post gem_unload in the i915_driver_unload path.
>>> s/i915_gem_fini/i915_gem_cleanup as it looks more suitable as that
>>> function does cleanup.
>>> To differentiate the tasks during suspend and unload w.r.t GuC this
>>> patch introduces new function i915_gem_fini which in addition to
>>> disabling GuC interfaces also disables GuC submission during which
>>> communication with GuC is needed for destroying doorbell.
>>> i915_gem_fini is copy of i915_gem_suspend with difference w.r.t
>>> GuC operations. To achieve this, new helpers i915_gem_context_suspend
>>> and i915_gem_suspend_complete are prepared.
>>>
>>
>> Sagar, I just realized this patch would make this comment in 
>> guc_client_free superfluous, right?:
>>
>>     /* FIXME: in many cases, by the time we get here the GuC has been
>>      * reset, so we cannot destroy the doorbell properly. Ignore the
>>      * error message for now */
>>
>> Can you make sure you remove it in this patch?
>>
>> Thanks!
> Yes. Will need to remove this comment :)
> Will make this change as part of 
> https://patchwork.freedesktop.org/patch/178189/
> How about another comment before it? i915_gem_suspend is ensuring no 
> outstanding submissions before coming to
> uc_suspend. So shall we remove that as well?
>
>         /*
>          * XXX: wait for any outstanding submissions before freeing 
> memory.
>          * Be sure to drop any locks
>          */

I have the impression that this comment was here as a future design 
guide for Direct Submission. It seems to be the case for almost all the 
"XXX" comments in that file, like:

     /* XXX: wait for any interrupts */
     /* XXX: wait for workqueue to drain */

Since Direct Submission is not being considered at the moment, maybe we 
can just drop of all them (or transform them into something more 
innocuous than a "XXX").
What do people think?