On 9/21/2017 10:46 PM, Michał Winiarski wrote:
On Wed, Sep 20, 2017 at 11:08:16PM +0530, Sagar Arun Kamble wrote:
Prepared generic helpers intel_uc_suspend, intel_uc_resume,
intel_uc_runtime_suspend, intel_uc_runtime_resume. Added
error handling to all the calls for suspend/resume.
I find the error handling done this way quite surprising...
More below.
v2: Rebase w.r.t removal of GuC code restructuring.
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_drv.c | 23 ++++++++++++++++++++---
drivers/gpu/drm/i915/i915_gem.c | 7 ++++++-
drivers/gpu/drm/i915/intel_uc.c | 20 ++++++++++++++++++++
drivers/gpu/drm/i915/intel_uc.h | 4 ++++
4 files changed, 50 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5c111ea..8635f40 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1691,7 +1691,15 @@ static int i915_drm_resume(struct drm_device *dev)
}
mutex_unlock(&dev->struct_mutex);
- intel_guc_resume(dev_priv);
+ /*
+ * NB: Full gem reinitialization is being done above, hence
+ * intel_uc_resume will be of no use. Currently intel_uc_resume
+ * is nop. If full reinitialization is removed, will need to put
+ * functionality to resume from sleep in intel_uc_resume.
+ */
+ ret = intel_uc_resume(dev_priv);
+ if (ret)
+ DRM_ERROR("failed to resume uc\n");
intel_modeset_init_hw(dev);
@@ -2493,7 +2501,12 @@ static int intel_runtime_suspend(struct device *kdev)
*/
i915_gem_runtime_suspend(dev_priv);
- intel_guc_suspend(dev_priv);
+ ret = intel_uc_runtime_suspend(dev_priv);
+ if (ret) {
+ DRM_ERROR("uc runtime suspend failed, disabling it(%d)\n", ret);
+ enable_rpm_wakeref_asserts(dev_priv);
+ return ret;
Early exit?
+ }
intel_runtime_pm_disable_interrupts(dev_priv);
@@ -2578,7 +2591,11 @@ static int intel_runtime_resume(struct device *kdev)
if (intel_uncore_unclaimed_mmio(dev_priv))
DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
- intel_guc_resume(dev_priv);
+ ret = intel_uc_runtime_resume(dev_priv);
+ if (ret) {
+ DRM_ERROR("uc runtime resume failed (%d)\n", ret);
+ return ret;
Same here.
+ }
if (IS_GEN9_LP(dev_priv)) {
bxt_disable_dc9(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c4bf348..dd56d45 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4539,7 +4539,11 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
i915_gem_contexts_lost(dev_priv);
mutex_unlock(&dev->struct_mutex);
- intel_guc_suspend(dev_priv);
+ ret = intel_uc_suspend(dev_priv);
+ if (ret) {
+ DRM_ERROR("uc suspend failed (%d)\n", ret);
+ goto out;
Should we really exit early if GuC sleep action fails?
In all of those cases - is this really something critical? Shouldn't we go
through the rest of the suspend/resume dance even if we fail to talk with GuC?
-Michał
Yes. Failure in GuC suspend/resume has to be critical. Essentially while
going into RPM suspend
we do want to ensure GuC is paused and dont want to go ahead and set PCI
device state to D3 even if
we knew GuC is active.
Similarly for resume. If we want to resume with i915 execlists when GuC
resume fails that would be
different code change.
+ }
cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
cancel_delayed_work_sync(&dev_priv->gt.retire_work);
@@ -4583,6 +4587,7 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
err_unlock:
mutex_unlock(&dev->struct_mutex);
+out:
intel_runtime_pm_put(dev_priv);
return ret;
}
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 0178ba4..8e4d8b0 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -537,3 +537,23 @@ int intel_guc_sample_forcewake(struct intel_guc *guc)
return intel_guc_send(guc, action, ARRAY_SIZE(action));
}
+
+int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv)
+{
+ return intel_guc_suspend(dev_priv);
+}
+
+int intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
+{
+ return intel_guc_resume(dev_priv);
+}
+
+int intel_uc_suspend(struct drm_i915_private *dev_priv)
+{
+ return intel_guc_suspend(dev_priv);
+}
+
+int intel_uc_resume(struct drm_i915_private *dev_priv)
+{
+ return 0;
+}
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 7703c9a..3d33a51 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -208,6 +208,10 @@ struct intel_huc {
void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
int intel_uc_init_hw(struct drm_i915_private *dev_priv);
void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
+int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv);
+int intel_uc_runtime_resume(struct drm_i915_private *dev_priv);
+int intel_uc_suspend(struct drm_i915_private *dev_priv);
+int intel_uc_resume(struct drm_i915_private *dev_priv);
int intel_guc_sample_forcewake(struct intel_guc *guc);
int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
