[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/guc: GuC suspend path cleanup

[Sujaritha]

On 3/21/19 1:23 PM, Chris Wilson wrote:
> Quoting Sujaritha (2019-03-21 20:02:36)
>> On 3/21/19 1:08 PM, Chris Wilson wrote:
>>> Quoting Sujaritha (2019-03-21 19:41:17)
>>>> On 3/21/19 12:37 PM, Chris Wilson wrote:
>>>>> Quoting Patchwork (2019-03-21 19:26:27)
>>>>>> == Series Details ==
>>>>>>
>>>>>> Series: drm/i915/guc: GuC suspend path cleanup
>>>>>> URL   : https://patchwork.freedesktop.org/series/58370/
>>>>>> State : failure
>>>>>>
>>>>>> == Summary ==
>>>>>>
>>>>>> CI Bug Log - changes from CI_DRM_5789 -> Patchwork_12553
>>>>>> ====================================================
>>>>>>
>>>>>> Summary
>>>>>> -------
>>>>>>
>>>>>>      **FAILURE**
>>>>>>
>>>>>>      Serious unknown changes coming with Patchwork_12553 absolutely need to be
>>>>>>      verified manually.
>>>>>>      
>>>>>>      If you think the reported changes have nothing to do with the changes
>>>>>>      introduced in Patchwork_12553, please notify your bug team to allow them
>>>>>>      to document this new failure mode, which will reduce false positives in CI.
>>>>>>
>>>>>>      External URL: https://patchwork.freedesktop.org/api/1.0/series/58370/revisions/1/mbox/
>>>>>>
>>>>>> Possible new issues
>>>>>> -------------------
>>>>>>
>>>>>>      Here are the unknown changes that may have been introduced in Patchwork_12553:
>>>>>>
>>>>>> ### IGT changes ###
>>>>>>
>>>>>> #### Possible regressions ####
>>>>>>
>>>>>>      * igt at gem_exec_suspend@basic-s3:
>>>>>>        - fi-apl-guc:         PASS -> DMESG-WARN
>>>>> That says we turned the guc off before completing the idle sequence, so
>>>>> the intel_uc_suspend() has to be after the flush_workqueues.
>>>>> -Chris
>>>> But shouldn't this be taken care of by the switch_to_kernel_context_sync ?
>>> Hmm, no, we can't flush the retire worker there (because of
>>> struct_mutex). But it should be taken care! Something to work on :)
>>>
>>>> And would be better have uc_suspend after drain_delayed_work instead of
>>>>
>>>> just after flush_workqueue ?
>>> Basically right at the end; you don't need struct_mutex right? And the
>>> assert that the gt is !awake fits in with the intent to switch guc off.
>>> -Chris
>>
>> Yes at the end, before the GEM_BUG_ON. The struct_mutex is not required.
> I'd go just after. The assert is that GEM is idle, so ready to suspend
> the FW. Worksforme.
> -Chris


Okay sure, I will add it just after the BUG_ON.

-Sujaritha