+ Michal, On the principle of code motion first, changes second, I'd like to see the clean split-up from Michal before touching the files much. That way git history will be easier to examine. Few comments below. On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote: > Prepared generic helpers intel_uc_suspend, intel_uc_resume, > intel_uc_runtime_suspend, intel_uc_runtime_resume. These are > called from respective GEM functions. > Only exception is intel_uc_resume that needs to be called > w/ or w/o GuC loaded in i915_drm_resume path. Changes to > add WOPCM condition check to load GuC during resume will be added > in later patches. > > v2: Rebase w.r.t removal of GuC code restructuring. > > v3: Calling intel_uc_resume from i915_gem_resume post resuming > i915 gem setup. This is symmetrical with i915_gem_suspend. > Removed error messages from i915 suspend/resume routines as > uC suspend/resume routines will have those. (Michal Wajdeczko) > Declare wedged on uc_suspend failure and uc_resume failure. > (Michał Winiarski) > Keeping the uC suspend/resume function definitions close to other > uC functions. > > v4: Added implementation to intel_uc_resume as GuC resume is > needed to be triggered post reloading the firmware as well. Added > comments about semantics of GuC resume with the firmware reload. > > v5: Updated return from i915_gem_runtime_suspend. Moved the comment > about GuC reload optimization to intel_uc_init_hw. (Michal Wajdeczko) > Updated comments as FIXME. > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> > Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx> <SNIP> > @@ -1698,6 +1698,18 @@ static int i915_drm_resume(struct drm_device *dev) > } > mutex_unlock(&dev->struct_mutex); > > + /* > + * FIXME: Currently we know that at the end of suspend we have done Full I think "beginning of resume" is the one that bites us more. > + * GPU reset and GuC is loaded again during i915_gem_init_hw. > + * Now, send action to GuC to resume back again as earlier call to > + * intel_uc_resume from i915_gem_resume would have done nothing. > + * This needs to be skipped if GuC was not loaded during resume as > + * intel_uc_resume would have been already called from i915_gem_resume. > + */ > + ret = intel_uc_resume(dev_priv); > + if (ret) > + i915_gem_set_wedged(dev_priv); I'm very sure we want to bring the system up with a backup of ELSP only submission *even* if we used GuC when going to sleep. > @@ -4571,7 +4573,9 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv) > if (WARN_ON(!intel_engines_are_idle(dev_priv))) > i915_gem_set_wedged(dev_priv); /* no hope, discard everything */ > > - intel_guc_suspend(dev_priv); > + ret = intel_uc_suspend(dev_priv); > + if (ret) > + i915_gem_set_wedged(dev_priv); /* no hope, discard everything */ Isn't that an exxaragation when we still may have functional ELSP? It may not be a bad idea to first bring up the ELSP submission and when GuC has loaded, switch over. To get rid of the latency. > @@ -4619,7 +4624,16 @@ int i915_gem_resume(struct drm_i915_private *dev_priv) > */ > dev_priv->gt.resume(dev_priv); > > - intel_guc_resume(dev_priv); > + /* > + * FIXME: At the end of suspend, Full GPU reset is done which unloads > + * the GuC firmware. If reset is avoided there, we can check the WOPCM > + * status here to see if GuC is still loaded and just do GuC resume > + * without reloading the firmware back. > + */ Again, we're resetting at both directions, going to suspend and on resume too. I'd classify these bot as more of a TODOs, because we only get some latency that can be improved on, no broken behaviour to fix. Regards, Joonas -- Joonas Lahtinen Open Source Technology Center Intel Corporation _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
