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

[Sagar Arun Kamble]

On 9/22/2017 1:19 AM, Oscar Mateo wrote:
>
>
> 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?
>
I feel it makes sense to keep for future changes then. If needed, let us 
update these in separate patch.
Thanks for the review Oscar.