On 9/29/2017 5:42 PM, Joonas Lahtinen wrote:
+ 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.
I think we wanted to get these fixes in prior to restructuring as that
might take time.
Currently this v12 series has some bit of restructuring included along
with fixes which has increased patches.
I have separated that today with new series. Will get that in first and
then follow with fixes series or fixes series based on Michal's split-up
series.
This is the minor restructuring series that is required for fixes
series: https://patchwork.freedesktop.org/series/31140/
Michal, please suggest.
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.
Yes. I have actually rephrased these comments in my next rev but its
held up before below series is merged:
https://patchwork.freedesktop.org/series/31140/
+ * 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.
Yes. In the next rev I am removing this wedged.
@@ -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.
Yes. will be removing this wedged.
@@ -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.
Yes. Will mark these as TODO.
Thanks for the review.
Regards, Joonas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
