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

[Chris Wilson]

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