[PATCH v8 1/8] drm/i915: Create GEM runtime resume helper and handle GEM suspend/resume errors

[PATCH v8 1/8] drm/i915: Create GEM runtime resume helper and handle GEM suspend/resume errors

[Sagar Arun Kamble]

Prepared helper i915_gem_runtime_resume to recreate gem setup.
Returning status from i915_gem_runtime_suspend and i915_gem_resume.
This will be placeholder for handling any errors from uC suspend/resume
in upcoming patches. Restructured the suspend/resume routines w.r.t setup
creation and rollback order.
This also fixes issue of ordering of i915_gem_runtime_resume with
intel_runtime_pm_enable_interrupts.

v2: Fixed return from intel_runtime_resume. (Michał Winiarski)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 34 ++++++++++++++++++++--------------
 drivers/gpu/drm/i915/i915_drv.h |  5 +++--
 drivers/gpu/drm/i915/i915_gem.c | 20 ++++++++++++++++++--
 3 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 7056bb2..a3bbf18 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1655,6 +1655,7 @@ static int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state)
 static int i915_drm_resume(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct pci_dev *pdev = dev_priv->drm.pdev;
 	int ret;
 
 	disable_rpm_wakeref_asserts(dev_priv);
@@ -1666,7 +1667,9 @@ static int i915_drm_resume(struct drm_device *dev)
 
 	intel_csr_ucode_resume(dev_priv);
 
-	i915_gem_resume(dev_priv);
+	ret = i915_gem_resume(dev_priv);
+	if (ret)
+		dev_err(&pdev->dev, "GEM resume failed\n");
 
 	i915_restore_state(dev_priv);
 	intel_pps_unlock_regs_wa(dev_priv);
@@ -2495,7 +2498,11 @@ static int intel_runtime_suspend(struct device *kdev)
 	 * We are safe here against re-faults, since the fault handler takes
 	 * an RPM reference.
 	 */
-	i915_gem_runtime_suspend(dev_priv);
+	ret = i915_gem_runtime_suspend(dev_priv);
+	if (ret) {
+		enable_rpm_wakeref_asserts(dev_priv);
+		return ret;
+	}
 
 	intel_guc_suspend(dev_priv);
 
@@ -2515,6 +2522,8 @@ static int intel_runtime_suspend(struct device *kdev)
 		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
 		intel_runtime_pm_enable_interrupts(dev_priv);
 
+		intel_guc_resume(dev_priv);
+		i915_gem_runtime_resume(dev_priv);
 		enable_rpm_wakeref_asserts(dev_priv);
 
 		return ret;
@@ -2567,7 +2576,7 @@ static int intel_runtime_resume(struct device *kdev)
 	struct pci_dev *pdev = to_pci_dev(kdev);
 	struct drm_device *dev = pci_get_drvdata(pdev);
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	int ret = 0;
+	int err = 0, ret;
 
 	if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev_priv)))
 		return -ENODEV;
@@ -2593,16 +2602,9 @@ static int intel_runtime_resume(struct device *kdev)
 	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
 		hsw_disable_pc8(dev_priv);
 	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
-		ret = vlv_resume_prepare(dev_priv, true);
+		err = vlv_resume_prepare(dev_priv, true);
 	}
 
-	/*
-	 * No point of rolling back things in case of an error, as the best
-	 * we can do is to hope that things will still work (and disable RPM).
-	 */
-	i915_gem_init_swizzling(dev_priv);
-	i915_gem_restore_fences(dev_priv);
-
 	intel_runtime_pm_enable_interrupts(dev_priv);
 
 	/*
@@ -2615,14 +2617,18 @@ static int intel_runtime_resume(struct device *kdev)
 
 	intel_enable_ipc(dev_priv);
 
+	ret = i915_gem_runtime_resume(dev_priv);
+	if (!err)
+		err = ret;
+
 	enable_rpm_wakeref_asserts(dev_priv);
 
-	if (ret)
-		DRM_ERROR("Runtime resume failed, disabling it (%d)\n", ret);
+	if (err)
+		DRM_ERROR("Runtime resume failed, disabling it (%d)\n", err);
 	else
 		DRM_DEBUG_KMS("Device resumed\n");
 
-	return ret;
+	return err;
 }
 
 const struct dev_pm_ops i915_pm_ops = {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 61a4be9..69370c1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3471,7 +3471,8 @@ struct i915_vma * __must_check
 int i915_gem_object_unbind(struct drm_i915_gem_object *obj);
 void i915_gem_release_mmap(struct drm_i915_gem_object *obj);
 
-void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv);
+int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv);
+int i915_gem_runtime_resume(struct drm_i915_private *dev_priv);
 
 static inline int __sg_page_count(const struct scatterlist *sg)
 {
@@ -3674,7 +3675,7 @@ void i915_gem_reset_engine(struct intel_engine_cs *engine,
 int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
 			   unsigned int flags);
 int __must_check i915_gem_suspend(struct drm_i915_private *dev_priv);
-void i915_gem_resume(struct drm_i915_private *dev_priv);
+int i915_gem_resume(struct drm_i915_private *dev_priv);
 int i915_gem_fault(struct vm_fault *vmf);
 int i915_gem_object_wait(struct drm_i915_gem_object *obj,
 			 unsigned int flags,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 73eeb6b..dbe181b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2022,7 +2022,7 @@ int i915_gem_fault(struct vm_fault *vmf)
 	intel_runtime_pm_put(i915);
 }
 
-void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
+int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
 {
 	struct drm_i915_gem_object *obj, *on;
 	int i;
@@ -2065,6 +2065,20 @@ void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
 		GEM_BUG_ON(!list_empty(&reg->vma->obj->userfault_link));
 		reg->dirty = true;
 	}
+
+	return 0;
+}
+
+int i915_gem_runtime_resume(struct drm_i915_private *dev_priv)
+{
+	/*
+	 * No point of rolling back things in case of an error, as the best
+	 * we can do is to hope that things will still work (and disable RPM).
+	 */
+	i915_gem_init_swizzling(dev_priv);
+	i915_gem_restore_fences(dev_priv);
+
+	return 0;
 }
 
 static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
@@ -4587,7 +4601,7 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-void i915_gem_resume(struct drm_i915_private *dev_priv)
+int i915_gem_resume(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = &dev_priv->drm;
 
@@ -4603,6 +4617,8 @@ void i915_gem_resume(struct drm_i915_private *dev_priv)
 	dev_priv->gt.resume(dev_priv);
 
 	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
 }
 
 void i915_gem_init_swizzling(struct drm_i915_private *dev_priv)
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v8 2/8] drm/i915: Update GEM suspend/resume flows considering GuC and GEM fences

[Sagar Arun Kamble]

This patch moves GuC suspend/resume handlers to corresponding GEM handlers
and orders them properly in the runtime and system suspend/resume flows.
It also adds documentation for GEM suspend/resume handlers.
i915_gem_restore_fences is GEM resumption task hence it is moved to
i915_gem_resume from i915_restore_state.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c     |  6 -----
 drivers/gpu/drm/i915/i915_gem.c     | 47 ++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/i915_suspend.c |  2 --
 3 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a3bbf18..8d67b8c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1698,8 +1698,6 @@ static int i915_drm_resume(struct drm_device *dev)
 	}
 	mutex_unlock(&dev->struct_mutex);
 
-	intel_guc_resume(dev_priv);
-
 	intel_modeset_init_hw(dev);
 
 	spin_lock_irq(&dev_priv->irq_lock);
@@ -2504,8 +2502,6 @@ static int intel_runtime_suspend(struct device *kdev)
 		return ret;
 	}
 
-	intel_guc_suspend(dev_priv);
-
 	intel_runtime_pm_disable_interrupts(dev_priv);
 
 	ret = 0;
@@ -2591,8 +2587,6 @@ 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);
-
 	if (IS_GEN9_LP(dev_priv)) {
 		bxt_disable_dc9(dev_priv);
 		bxt_display_core_init(dev_priv, true);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index dbe181b..5dcd8c0 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2022,11 +2022,22 @@ int i915_gem_fault(struct vm_fault *vmf)
 	intel_runtime_pm_put(i915);
 }
 
+/**
+ * i915_gem_runtime_suspend() - Finish GEM suspend
+ * @dev_priv: i915 device private
+ *
+ * This function suspends GuC, removes userspace mappings for all GEM obejcts
+ * currently on userfault list and marks fences if any being used as lost.
+ *
+ * Return:	non-zero code on error
+ */
 int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
 {
 	struct drm_i915_gem_object *obj, *on;
 	int i;
 
+	intel_guc_suspend(dev_priv);
+
 	/*
 	 * Only called during RPM suspend. All users of the userfault_list
 	 * must be holding an RPM wakeref to ensure that this can not
@@ -2069,6 +2080,14 @@ int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
+/**
+ * i915_gem_runtime_resume() - Restore GEM state
+ * @dev_priv: i915 device private
+ *
+ * This function inits swizzling, restores fences and resumes GuC.
+ *
+ * Return:	non-zero code on error
+ */
 int i915_gem_runtime_resume(struct drm_i915_private *dev_priv)
 {
 	/*
@@ -2078,6 +2097,8 @@ int i915_gem_runtime_resume(struct drm_i915_private *dev_priv)
 	i915_gem_init_swizzling(dev_priv);
 	i915_gem_restore_fences(dev_priv);
 
+	intel_guc_resume(dev_priv);
+
 	return 0;
 }
 
@@ -4521,6 +4542,16 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
 	}
 }
 
+/**
+ * i915_gem_suspend() - Suspend all GT activity.
+ * @dev_priv: i915 device private
+ *
+ * This function disables RPS, flushes all executing context ensuring
+ * GEM/GT/Engines idleness, cancels all work that needs GT access and suspends
+ * GuC. In the end currently, it also reset the GEM state and GPU HW.
+ *
+ * Return:	non-zero code on error
+ */
 int i915_gem_suspend(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = &dev_priv->drm;
@@ -4553,8 +4584,6 @@ 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);
-
 	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
 	cancel_delayed_work_sync(&dev_priv->gt.retire_work);
 
@@ -4571,6 +4600,8 @@ 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);
+
 	/*
 	 * Neither the BIOS, ourselves or any other kernel
 	 * expects the system to be in execlists mode on startup,
@@ -4601,6 +4632,15 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
+/**
+ * i915_gem_resume() - Resume GT activity.
+ * @dev_priv: i915 device private
+ *
+ * This function restores GTT mappings, restores fences and resets the
+ * context images and resumes GuC.
+ *
+ * Return:	non-zero code on error
+ */
 int i915_gem_resume(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = &dev_priv->drm;
@@ -4609,13 +4649,14 @@ int i915_gem_resume(struct drm_i915_private *dev_priv)
 
 	mutex_lock(&dev->struct_mutex);
 	i915_gem_restore_gtt_mappings(dev_priv);
+	i915_gem_restore_fences(dev_priv);
 
 	/* As we didn't flush the kernel context before suspend, we cannot
 	 * guarantee that the context image is complete. So let's just reset
 	 * it and start again.
 	 */
 	dev_priv->gt.resume(dev_priv);
-
+	intel_guc_resume(dev_priv);
 	mutex_unlock(&dev->struct_mutex);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 5c86925a..8f3aa4d 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -108,8 +108,6 @@ int i915_restore_state(struct drm_i915_private *dev_priv)
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
 
-	i915_gem_restore_fences(dev_priv);
-
 	if (IS_GEN4(dev_priv))
 		pci_write_config_word(pdev, GCDGMBUS,
 				      dev_priv->regfile.saveGCDGMBUS);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v8 3/8] drm/i915: Create uC runtime and system suspend/resume helpers

[Sagar Arun Kamble]

Prepared generic helpers intel_uc_suspend, intel_uc_resume,
intel_uc_runtime_suspend, intel_uc_runtime_resume. These are
called from respective GEM functions.

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)
Added DRM_DEBUG message about skipping changes in intel_uc_resume.
Keeping the uC suspend/resume function definitions close to other
uC functions.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c |  1 -
 drivers/gpu/drm/i915/i915_gem.c | 32 +++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_uc.c | 22 ++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.h |  4 ++++
 4 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 8d67b8c..8bfebe3 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2518,7 +2518,6 @@ static int intel_runtime_suspend(struct device *kdev)
 		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
 		intel_runtime_pm_enable_interrupts(dev_priv);
 
-		intel_guc_resume(dev_priv);
 		i915_gem_runtime_resume(dev_priv);
 		enable_rpm_wakeref_asserts(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5dcd8c0..8e6e2bd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2034,9 +2034,11 @@ int i915_gem_fault(struct vm_fault *vmf)
 int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
 {
 	struct drm_i915_gem_object *obj, *on;
-	int i;
+	int i, ret;
 
-	intel_guc_suspend(dev_priv);
+	ret = intel_uc_runtime_suspend(dev_priv);
+	if (ret)
+		return ret;
 
 	/*
 	 * Only called during RPM suspend. All users of the userfault_list
@@ -2077,7 +2079,7 @@ int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
 		reg->dirty = true;
 	}
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -2097,9 +2099,7 @@ int i915_gem_runtime_resume(struct drm_i915_private *dev_priv)
 	i915_gem_init_swizzling(dev_priv);
 	i915_gem_restore_fences(dev_priv);
 
-	intel_guc_resume(dev_priv);
-
-	return 0;
+	return intel_uc_runtime_resume(dev_priv);
 }
 
 static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
@@ -4600,7 +4600,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 */
 
 	/*
 	 * Neither the BIOS, ourselves or any other kernel
@@ -4644,6 +4646,7 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
 int i915_gem_resume(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = &dev_priv->drm;
+	int ret;
 
 	WARN_ON(dev_priv->gt.awake);
 
@@ -4656,10 +4659,21 @@ int i915_gem_resume(struct drm_i915_private *dev_priv)
 	 * it and start again.
 	 */
 	dev_priv->gt.resume(dev_priv);
-	intel_guc_resume(dev_priv);
+
+	/*
+	 * NB: Full gem reinitialization is being done in i915_drm_resume
+	 * after gem_resume, so currently intel_uc_resume will be of no use.
+	 * Hence, intel_uc_resume is nop currently. 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)
+		i915_gem_set_wedged(dev_priv);
+
 	mutex_unlock(&dev->struct_mutex);
 
-	return 0;
+	return ret;
 }
 
 void i915_gem_init_swizzling(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 2774778..b9376e4 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -481,6 +481,28 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 	i915_ggtt_disable_guc(dev_priv);
 }
 
+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)
+{
+	DRM_DEBUG_DRIVER("uC resume does nothing as full reinit is done\n");
+
+	return 0;
+}
+
 int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
 {
 	WARN(1, "Unexpected send: action=%#x\n", *action);
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 6966349..0a79e17 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v8 4/8] drm/i915/guc: Introduce intel_guc_sanitize

[Sagar Arun Kamble]

Currently GPU is reset at the end of suspend via i915_gem_sanitize.
On resume, GuC will not be loaded until intel_uc_init_hw happens
during GEM resume flow but action to exit sleep wll be send to GuC
considering the FW load status. To make sure we don't invoke that
action update GuC FW load status through new helper
intel_guc_sanitize. Conditional check based on FW fetch status should
take care of driver init flow.

v2: Rebase.

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 ++
 drivers/gpu/drm/i915/intel_uc.c | 8 ++++++++
 drivers/gpu/drm/i915/intel_uc.h | 1 +
 3 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8e6e2bd..0d9a107 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4528,6 +4528,8 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
 		mutex_unlock(&i915->drm.struct_mutex);
 	}
 
+	intel_guc_sanitize(&i915->guc);
+
 	/*
 	 * If we inherit context state from the BIOS or earlier occupants
 	 * of the GPU, the GPU may be in an inconsistent state when we
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index b9376e4..29d43f8 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -581,3 +581,11 @@ int intel_guc_sample_forcewake(struct intel_guc *guc)
 
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
 }
+
+void intel_guc_sanitize(struct intel_guc *guc)
+{
+	if (guc->fw.fetch_status == INTEL_UC_FIRMWARE_SUCCESS)
+		guc->fw.load_status = INTEL_UC_FIRMWARE_PENDING;
+	else
+		guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
+}
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 0a79e17..6163ff9 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -213,6 +213,7 @@ struct intel_huc {
 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);
+void intel_guc_sanitize(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);
 int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v8 5/8] drm/i915/guc: Update GuC ggtt.invalidate/interrupts/communication across RPM suspend/resume

[Sagar Arun Kamble]

Apart from configuring interrupts, we need to update the ggtt invalidate
interface and GuC communication on suspend/resume. This functionality
can be reused for other suspend and reset paths.

v2: Rebase w.r.t removal of GuC code restructuring.

v3: Removed GuC specific helpers as tasks other than send H2G for
sleep/resume are to be done from uc generic functions. (Michal Wajdeczko)

v4: Simplified/Unified the error messaging in uc_runtime_suspend/resume.
(Michal Wajdeczko). Rebase w.r.t i915_modparams change.
Added documentation to intel_uc_runtime_suspend/resume.

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c |  5 ---
 drivers/gpu/drm/i915/intel_uc.c            | 60 +++++++++++++++++++++++++++++-
 2 files changed, 58 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 04f1281..d1d6c0d 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1226,8 +1226,6 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
 	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return 0;
 
-	gen9_disable_guc_interrupts(dev_priv);
-
 	ctx = dev_priv->kernel_context;
 
 	data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
@@ -1252,9 +1250,6 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
 	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return 0;
 
-	if (i915_modparams.guc_log_level >= 0)
-		gen9_enable_guc_interrupts(dev_priv);
-
 	ctx = dev_priv->kernel_context;
 
 	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 29d43f8..7c34bc6 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -481,14 +481,70 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 	i915_ggtt_disable_guc(dev_priv);
 }
 
+/**
+ * intel_uc_runtime_suspend() - Suspend uC operation.
+ * @dev_priv: i915 device private
+ *
+ * This function invokes GuC OS suspension, makes ggtt_invalidate function to
+ * point to non-GuC variant, disables GuC interrupts and disable communication
+ * with GuC.
+ *
+ * Return:	non-zero code on error
+ */
 int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv)
 {
-	return intel_guc_suspend(dev_priv);
+	int ret;
+
+	if (!i915_modparams.enable_guc_loading)
+		return 0;
+
+	ret = intel_guc_suspend(dev_priv);
+	if (ret)
+		goto out;
+
+	i915_ggtt_disable_guc(dev_priv);
+	gen9_disable_guc_interrupts(dev_priv);
+	guc_disable_communication(&dev_priv->guc);
+
+out:
+	if (ret)
+		DRM_ERROR("uC runtime suspend failed (%d)\n", ret);
+	return ret;
 }
 
+/**
+ * intel_uc_runtime_resume() - Resume uC operation.
+ * @dev_priv: i915 device private
+ *
+ * This function enables communication with GuC, enables GuC interrupts,
+ * makes ggtt_invalidate function to point to GuC variant and invokes
+ * GuC OS resumption.
+ *
+ * Return:	non-zero code on error
+ */
 int intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
 {
-	return intel_guc_resume(dev_priv);
+	int ret;
+
+	if (!i915_modparams.enable_guc_loading)
+		return 0;
+
+	ret = guc_enable_communication(&dev_priv->guc);
+	if (ret)
+		goto out;
+
+	if (i915_modparams.guc_log_level >= 0)
+		gen9_enable_guc_interrupts(dev_priv);
+	i915_ggtt_enable_guc(dev_priv);
+
+	ret = intel_guc_resume(dev_priv);
+	if (ret)
+		goto out;
+
+out:
+	if (ret)
+		DRM_ERROR("uC runtime resume failed (%d)\n", ret);
+	return ret;
 }
 
 int intel_uc_suspend(struct drm_i915_private *dev_priv)
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v8 6/8] drm/i915/guc: Update GuC suspend functionality in intel_uc_suspend

[Sagar Arun Kamble]

With this patch we disable GuC submission in i915_drm_suspend path.
This will destroy the client which will be setup back again. We also
reuse the complete sanitization done via intel_uc_runtime_suspend in
this path. Post i915_drm_resume, this state is recreated by
intel_uc_init_hw hence we need not have similar reuse for intel_uc_resume.
This also fixes issue where intel_uc_fini_hw was being called after GPU
reset happening in i915_gem_suspend during i915_driver_unload.

v2: Rebase w.r.t removal of GuC code restructuring. Added struct_mutex
protection for i915_guc_submission_disable.

v3: Rebase w.r.t updated GuC suspend function name.

v4: Added lockdep assert in i915_guc_submission_enable/disable.
Refined intel_uc_suspend to remove unnecessary locals and simplify
return. (Michal Winiarski)
Removed comment in guc_client_free about ignoring failure for
destroy_doorbell. (Oscar)
Rebase w.r.t i915_modparams change.

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 15 ++++++++++++---
 drivers/gpu/drm/i915/intel_uc.c            | 11 +++++++----
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index d1d6c0d..0c56765 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -885,9 +885,6 @@ static void guc_client_free(struct i915_guc_client *client)
 	 * Be sure to drop any locks
 	 */
 
-	/* FIXME: in many cases, by the time we get here the GuC has been
-	 * reset, so we cannot destroy the doorbell properly. Ignore the
-	 * error message for now */
 	destroy_doorbell(client);
 	guc_stage_desc_fini(client->guc, client);
 	i915_gem_object_unpin_map(client->vma->obj);
@@ -1154,6 +1151,12 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 		     sizeof(struct guc_wq_item) *
 		     I915_NUM_ENGINES > GUC_WQ_SIZE);
 
+	/*
+	 * Assert that drm.struct_mutex is held.
+	 * Needed for GuC client vma binding.
+	 */
+	lockdep_assert_held(&dev_priv->drm.struct_mutex);
+
 	if (!client) {
 		client = guc_client_alloc(dev_priv,
 					  INTEL_INFO(dev_priv)->ring_mask,
@@ -1204,6 +1207,12 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
 
+	/*
+	 * Assert that drm.struct_mutex is held.
+	 * Needed for GuC client vma unbinding.
+	 */
+	lockdep_assert_held(&dev_priv->drm.struct_mutex);
+
 	guc_interrupts_release(dev_priv);
 
 	/* Revert back to manual ELSP submission */
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 7c34bc6..d710f0d 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -468,9 +468,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 	if (!i915_modparams.enable_guc_loading)
 		return;
 
-	if (i915_modparams.enable_guc_submission)
-		i915_guc_submission_disable(dev_priv);
-
 	guc_disable_communication(&dev_priv->guc);
 
 	if (i915_modparams.enable_guc_submission) {
@@ -549,7 +546,13 @@ int intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
 
 int intel_uc_suspend(struct drm_i915_private *dev_priv)
 {
-	return intel_guc_suspend(dev_priv);
+	if (i915_modparams.enable_guc_submission) {
+		mutex_lock(&dev_priv->drm.struct_mutex);
+		i915_guc_submission_disable(dev_priv);
+		mutex_unlock(&dev_priv->drm.struct_mutex);
+	}
+
+	return intel_uc_runtime_suspend(dev_priv);
 }
 
 int intel_uc_resume(struct drm_i915_private *dev_priv)
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v8 7/8] drm/i915/guc: Disable GuC submission and suspend it prior to i915 reset

[Sagar Arun Kamble]

Before i915 reset, we need to disable GuC submission and suspend GuC
operations as it is recreated during intel_uc_init_hw. We can't reuse the
intel_uc_suspend functionality as reset path already holds struct_mutex.

v2: Rebase w.r.t removal of GuC code restructuring. Updated reset_prepare
function as struct_mutex is not needed.

v3: Fixed typo in commit message. Made return from intel_uc_reset_prepare
simpler. (Michal)
Rebase w.r.t i915_modparams change.

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 ++
 drivers/gpu/drm/i915/intel_uc.c | 8 ++++++++
 drivers/gpu/drm/i915/intel_uc.h | 1 +
 3 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0d9a107..64b278c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2882,6 +2882,8 @@ int i915_gem_reset_prepare(struct drm_i915_private *dev_priv)
 
 	i915_gem_revoke_fences(dev_priv);
 
+	intel_uc_reset_prepare(dev_priv);
+
 	return err;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index d710f0d..aec3f6b 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -562,6 +562,14 @@ int intel_uc_resume(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
+int intel_uc_reset_prepare(struct drm_i915_private *dev_priv)
+{
+	if (i915_modparams.enable_guc_submission)
+		i915_guc_submission_disable(dev_priv);
+
+	return intel_uc_runtime_suspend(dev_priv);
+}
+
 int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
 {
 	WARN(1, "Unexpected send: action=%#x\n", *action);
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 6163ff9..9b12f3c 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -212,6 +212,7 @@ struct intel_huc {
 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_uc_reset_prepare(struct drm_i915_private *dev_priv);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
 void intel_guc_sanitize(struct intel_guc *guc);
 int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v8 8/8] drm/i915/guc: Fix GuC cleanup in unload path

[Sagar Arun Kamble]

We ensure that GuC is completely suspended and client is destroyed
in i915_gem_suspend during i915_driver_unload. So now intel_uc_fini_hw
should just take care of cleanup,
hence s/intel_uc_fini_hw/intel_uc_cleanup. Correspondingly
we also updated as s/i915_guc_submission_fini/i915_guc_submission_cleanup
Other functionality to disable communication, disable interrupts and
update of ggtt.invalidate is taken care by intel_uc_suspend.

v2: Rebase w.r.t removal of GuC code restructuring.

v3: Removed intel_guc_cleanup. (Michal Wajdeczko)

v4: guc_free_load_err_log() needs to be called without checking
i915.enable_guc_loading as this param is cleared on GuC load failure.
(Michal Wajdeczko)

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c            |  2 +-
 drivers/gpu/drm/i915/i915_guc_submission.c |  2 +-
 drivers/gpu/drm/i915/intel_uc.c            | 14 ++++----------
 drivers/gpu/drm/i915/intel_uc.h            |  4 ++--
 4 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 8bfebe3..4c0a5b5 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -603,7 +603,7 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
 	i915_gem_drain_workqueue(dev_priv);
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
-	intel_uc_fini_hw(dev_priv);
+	intel_uc_cleanup(dev_priv);
 	i915_gem_cleanup_engines(dev_priv);
 	i915_gem_contexts_fini(dev_priv);
 	i915_gem_cleanup_userptr(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 0c56765..176cf55 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1053,7 +1053,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
+void i915_guc_submission_cleanup(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
 
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index aec3f6b..b8e1f97 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -439,7 +439,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	guc_capture_load_err_log(guc);
 err_submission:
 	if (i915_modparams.enable_guc_submission)
-		i915_guc_submission_fini(dev_priv);
+		i915_guc_submission_cleanup(dev_priv);
 err_guc:
 	i915_ggtt_disable_guc(dev_priv);
 
@@ -461,21 +461,15 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
+void intel_uc_cleanup(struct drm_i915_private *dev_priv)
 {
 	guc_free_load_err_log(&dev_priv->guc);
 
 	if (!i915_modparams.enable_guc_loading)
 		return;
 
-	guc_disable_communication(&dev_priv->guc);
-
-	if (i915_modparams.enable_guc_submission) {
-		gen9_disable_guc_interrupts(dev_priv);
-		i915_guc_submission_fini(dev_priv);
-	}
-
-	i915_ggtt_disable_guc(dev_priv);
+	if (i915_modparams.enable_guc_submission)
+		i915_guc_submission_cleanup(dev_priv);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 9b12f3c..936d947 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -207,7 +207,7 @@ struct intel_huc {
 void intel_uc_init_fw(struct drm_i915_private *dev_priv);
 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);
+void intel_uc_cleanup(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);
@@ -240,7 +240,7 @@ static inline void intel_guc_notify(struct intel_guc *guc)
 int i915_guc_submission_init(struct drm_i915_private *dev_priv);
 int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
 void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
-void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
+void i915_guc_submission_cleanup(struct drm_i915_private *dev_priv);
 struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
 
 /* intel_guc_log.c */
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 1/8] drm/i915: Create GEM runtime resume helper and handle GEM suspend/resume errors

[Sagar Arun Kamble]

And I have forgot to amend the ordering of tags cc, s-o-b etc. Sorry for 
the same.


On 9/26/2017 6:54 PM, Sagar Arun Kamble wrote:
> Prepared helper i915_gem_runtime_resume to recreate gem setup.
> Returning status from i915_gem_runtime_suspend and i915_gem_resume.
> This will be placeholder for handling any errors from uC suspend/resume
> in upcoming patches. Restructured the suspend/resume routines w.r.t setup
> creation and rollback order.
> This also fixes issue of ordering of i915_gem_runtime_resume with
> intel_runtime_pm_enable_interrupts.
>
> v2: Fixed return from intel_runtime_resume. (Michał Winiarski)
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c | 34 ++++++++++++++++++++--------------
>   drivers/gpu/drm/i915/i915_drv.h |  5 +++--
>   drivers/gpu/drm/i915/i915_gem.c | 20 ++++++++++++++++++--
>   3 files changed, 41 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 7056bb2..a3bbf18 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1655,6 +1655,7 @@ static int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state)
>   static int i915_drm_resume(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct pci_dev *pdev = dev_priv->drm.pdev;
>   	int ret;
>   
>   	disable_rpm_wakeref_asserts(dev_priv);
> @@ -1666,7 +1667,9 @@ static int i915_drm_resume(struct drm_device *dev)
>   
>   	intel_csr_ucode_resume(dev_priv);
>   
> -	i915_gem_resume(dev_priv);
> +	ret = i915_gem_resume(dev_priv);
> +	if (ret)
> +		dev_err(&pdev->dev, "GEM resume failed\n");
>   
>   	i915_restore_state(dev_priv);
>   	intel_pps_unlock_regs_wa(dev_priv);
> @@ -2495,7 +2498,11 @@ static int intel_runtime_suspend(struct device *kdev)
>   	 * We are safe here against re-faults, since the fault handler takes
>   	 * an RPM reference.
>   	 */
> -	i915_gem_runtime_suspend(dev_priv);
> +	ret = i915_gem_runtime_suspend(dev_priv);
> +	if (ret) {
> +		enable_rpm_wakeref_asserts(dev_priv);
> +		return ret;
> +	}
>   
>   	intel_guc_suspend(dev_priv);
>   
> @@ -2515,6 +2522,8 @@ static int intel_runtime_suspend(struct device *kdev)
>   		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
>   		intel_runtime_pm_enable_interrupts(dev_priv);
>   
> +		intel_guc_resume(dev_priv);
> +		i915_gem_runtime_resume(dev_priv);
>   		enable_rpm_wakeref_asserts(dev_priv);
>   
>   		return ret;
> @@ -2567,7 +2576,7 @@ static int intel_runtime_resume(struct device *kdev)
>   	struct pci_dev *pdev = to_pci_dev(kdev);
>   	struct drm_device *dev = pci_get_drvdata(pdev);
>   	struct drm_i915_private *dev_priv = to_i915(dev);
> -	int ret = 0;
> +	int err = 0, ret;
>   
>   	if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev_priv)))
>   		return -ENODEV;
> @@ -2593,16 +2602,9 @@ static int intel_runtime_resume(struct device *kdev)
>   	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>   		hsw_disable_pc8(dev_priv);
>   	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> -		ret = vlv_resume_prepare(dev_priv, true);
> +		err = vlv_resume_prepare(dev_priv, true);
>   	}
>   
> -	/*
> -	 * No point of rolling back things in case of an error, as the best
> -	 * we can do is to hope that things will still work (and disable RPM).
> -	 */
> -	i915_gem_init_swizzling(dev_priv);
> -	i915_gem_restore_fences(dev_priv);
> -
>   	intel_runtime_pm_enable_interrupts(dev_priv);
>   
>   	/*
> @@ -2615,14 +2617,18 @@ static int intel_runtime_resume(struct device *kdev)
>   
>   	intel_enable_ipc(dev_priv);
>   
> +	ret = i915_gem_runtime_resume(dev_priv);
> +	if (!err)
> +		err = ret;
> +
>   	enable_rpm_wakeref_asserts(dev_priv);
>   
> -	if (ret)
> -		DRM_ERROR("Runtime resume failed, disabling it (%d)\n", ret);
> +	if (err)
> +		DRM_ERROR("Runtime resume failed, disabling it (%d)\n", err);
>   	else
>   		DRM_DEBUG_KMS("Device resumed\n");
>   
> -	return ret;
> +	return err;
>   }
>   
>   const struct dev_pm_ops i915_pm_ops = {
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 61a4be9..69370c1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3471,7 +3471,8 @@ struct i915_vma * __must_check
>   int i915_gem_object_unbind(struct drm_i915_gem_object *obj);
>   void i915_gem_release_mmap(struct drm_i915_gem_object *obj);
>   
> -void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv);
> +int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv);
> +int i915_gem_runtime_resume(struct drm_i915_private *dev_priv);
>   
>   static inline int __sg_page_count(const struct scatterlist *sg)
>   {
> @@ -3674,7 +3675,7 @@ void i915_gem_reset_engine(struct intel_engine_cs *engine,
>   int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
>   			   unsigned int flags);
>   int __must_check i915_gem_suspend(struct drm_i915_private *dev_priv);
> -void i915_gem_resume(struct drm_i915_private *dev_priv);
> +int i915_gem_resume(struct drm_i915_private *dev_priv);
>   int i915_gem_fault(struct vm_fault *vmf);
>   int i915_gem_object_wait(struct drm_i915_gem_object *obj,
>   			 unsigned int flags,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 73eeb6b..dbe181b 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2022,7 +2022,7 @@ int i915_gem_fault(struct vm_fault *vmf)
>   	intel_runtime_pm_put(i915);
>   }
>   
> -void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
> +int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
>   {
>   	struct drm_i915_gem_object *obj, *on;
>   	int i;
> @@ -2065,6 +2065,20 @@ void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
>   		GEM_BUG_ON(!list_empty(&reg->vma->obj->userfault_link));
>   		reg->dirty = true;
>   	}
> +
> +	return 0;
> +}
> +
> +int i915_gem_runtime_resume(struct drm_i915_private *dev_priv)
> +{
> +	/*
> +	 * No point of rolling back things in case of an error, as the best
> +	 * we can do is to hope that things will still work (and disable RPM).
> +	 */
> +	i915_gem_init_swizzling(dev_priv);
> +	i915_gem_restore_fences(dev_priv);
> +
> +	return 0;
>   }
>   
>   static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
> @@ -4587,7 +4601,7 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>   	return ret;
>   }
>   
> -void i915_gem_resume(struct drm_i915_private *dev_priv)
> +int i915_gem_resume(struct drm_i915_private *dev_priv)
>   {
>   	struct drm_device *dev = &dev_priv->drm;
>   
> @@ -4603,6 +4617,8 @@ void i915_gem_resume(struct drm_i915_private *dev_priv)
>   	dev_priv->gt.resume(dev_priv);
>   
>   	mutex_unlock(&dev->struct_mutex);
> +
> +	return 0;
>   }
>   
>   void i915_gem_init_swizzling(struct drm_i915_private *dev_priv)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 1/8] drm/i915: Create GEM runtime resume helper and handle GEM suspend/resume errors

[Chris Wilson]

Quoting Sagar Arun Kamble (2017-09-26 14:24:38)
>  drivers/gpu/drm/i915/i915_drv.c | 34 ++++++++++++++++++++--------------
>  drivers/gpu/drm/i915/i915_drv.h |  5 +++--
>  drivers/gpu/drm/i915/i915_gem.c | 20 ++++++++++++++++++--
>  3 files changed, 41 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 7056bb2..a3bbf18 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1655,6 +1655,7 @@ static int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state)
>  static int i915_drm_resume(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = to_i915(dev);
> +       struct pci_dev *pdev = dev_priv->drm.pdev;
>         int ret;
>  
>         disable_rpm_wakeref_asserts(dev_priv);
> @@ -1666,7 +1667,9 @@ static int i915_drm_resume(struct drm_device *dev)
>  
>         intel_csr_ucode_resume(dev_priv);
>  
> -       i915_gem_resume(dev_priv);
> +       ret = i915_gem_resume(dev_priv);
> +       if (ret)
> +               dev_err(&pdev->dev, "GEM resume failed\n");
>  
>         i915_restore_state(dev_priv);
>         intel_pps_unlock_regs_wa(dev_priv);
> @@ -2615,14 +2617,18 @@ static int intel_runtime_resume(struct device *kdev)
>  
>         intel_enable_ipc(dev_priv);
>  
> +       ret = i915_gem_runtime_resume(dev_priv);
> +       if (!err)
> +               err = ret;
> +
>         enable_rpm_wakeref_asserts(dev_priv);
>  
> -       if (ret)
> -               DRM_ERROR("Runtime resume failed, disabling it (%d)\n", ret);
> +       if (err)
> +               DRM_ERROR("Runtime resume failed, disabling it (%d)\n", err);
>         else
>                 DRM_DEBUG_KMS("Device resumed\n");
>  
> -       return ret;
> +       return err;

What we've tried to do is to limit the damage that can happen if we
fail to re-enable GEM. We have tried to let the device resume, but
mark the device as wedged to prevent future execution, and so let the
device live long enough to be able to show error messages and whatnot
(system critical clients should also survive and fallover to alternative
paths).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 2/8] drm/i915: Update GEM suspend/resume flows considering GuC and GEM fences

[Chris Wilson]

Quoting Sagar Arun Kamble (2017-09-26 14:24:39)
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index dbe181b..5dcd8c0 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2022,11 +2022,22 @@ int i915_gem_fault(struct vm_fault *vmf)
>         intel_runtime_pm_put(i915);
>  }
>  
> +/**
> + * i915_gem_runtime_suspend() - Finish GEM suspend
> + * @dev_priv: i915 device private
> + *
> + * This function suspends GuC, removes userspace mappings for all GEM obejcts
> + * currently on userfault list and marks fences if any being used as lost.
> + *
> + * Return:     non-zero code on error
> + */

> +/**
> + * i915_gem_runtime_resume() - Restore GEM state
> + * @dev_priv: i915 device private
> + *
> + * This function inits swizzling, restores fences and resumes GuC.
> + *
> + * Return:     non-zero code on error
> + */

> +/**
> + * i915_gem_suspend() - Suspend all GT activity.
> + * @dev_priv: i915 device private
> + *
> + * This function disables RPS, flushes all executing context ensuring
> + * GEM/GT/Engines idleness, cancels all work that needs GT access and suspends
> + * GuC. In the end currently, it also reset the GEM state and GPU HW.
> + *
> + * Return:     non-zero code on error
> + */

> +/**
> + * i915_gem_resume() - Resume GT activity.
> + * @dev_priv: i915 device private
> + *
> + * This function restores GTT mappings, restores fences and resets the
> + * context images and resumes GuC.
> + *
> + * Return:     non-zero code on error
> + */

I can very much read what the functions do. These comments blocks are
for telling me when to call them, under what pre/post-conditions and the
overall intent of the function.

Who are these blocks for? This isn't a library interface we expect to be
used, these are hooks.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 3/8] drm/i915: Create uC runtime and system suspend/resume helpers

[Chris Wilson]

Quoting Sagar Arun Kamble (2017-09-26 14:24:40)
> @@ -2077,7 +2079,7 @@ int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
>                 reg->dirty = true;
>         }
>  
> -       return 0;
> +       return ret;
OI! These are written as return 0 because you are very much not meant to
reach this point with an error.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 4/8] drm/i915/guc: Introduce intel_guc_sanitize

[Chris Wilson]

Quoting Sagar Arun Kamble (2017-09-26 14:24:41)
> Currently GPU is reset at the end of suspend via i915_gem_sanitize.
> On resume, GuC will not be loaded until intel_uc_init_hw happens
> during GEM resume flow but action to exit sleep wll be send to GuC
> considering the FW load status. To make sure we don't invoke that
> action update GuC FW load status through new helper
> intel_guc_sanitize. Conditional check based on FW fetch status should
> take care of driver init flow.
> 
> v2: Rebase.
> 
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 2 ++
>  drivers/gpu/drm/i915/intel_uc.c | 8 ++++++++
>  drivers/gpu/drm/i915/intel_uc.h | 1 +
>  3 files changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 8e6e2bd..0d9a107 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4528,6 +4528,8 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
>                 mutex_unlock(&i915->drm.struct_mutex);
>         }
>  
> +       intel_guc_sanitize(&i915->guc);
> +
>         /*
>          * If we inherit context state from the BIOS or earlier occupants
>          * of the GPU, the GPU may be in an inconsistent state when we
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index b9376e4..29d43f8 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -581,3 +581,11 @@ int intel_guc_sample_forcewake(struct intel_guc *guc)
>  
>         return intel_guc_send(guc, action, ARRAY_SIZE(action));
>  }
> +
> +void intel_guc_sanitize(struct intel_guc *guc)
> +{
> +       if (guc->fw.fetch_status == INTEL_UC_FIRMWARE_SUCCESS)
> +               guc->fw.load_status = INTEL_UC_FIRMWARE_PENDING;
> +       else
> +               guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;

No, sanitize should not be carrying old state over. We are supposed to
be scrubbing hw state. (Elsewhere we detaint user state.)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 6/8] drm/i915/guc: Update GuC suspend functionality in intel_uc_suspend

[Chris Wilson]

Quoting Sagar Arun Kamble (2017-09-26 14:24:43)
> With this patch we disable GuC submission in i915_drm_suspend path.
> This will destroy the client which will be setup back again. We also
> reuse the complete sanitization done via intel_uc_runtime_suspend in
> this path. Post i915_drm_resume, this state is recreated by
> intel_uc_init_hw hence we need not have similar reuse for intel_uc_resume.
> This also fixes issue where intel_uc_fini_hw was being called after GPU
> reset happening in i915_gem_suspend during i915_driver_unload.
> 
> v2: Rebase w.r.t removal of GuC code restructuring. Added struct_mutex
> protection for i915_guc_submission_disable.
> 
> v3: Rebase w.r.t updated GuC suspend function name.
> 
> v4: Added lockdep assert in i915_guc_submission_enable/disable.
> Refined intel_uc_suspend to remove unnecessary locals and simplify
> return. (Michal Winiarski)
> Removed comment in guc_client_free about ignoring failure for
> destroy_doorbell. (Oscar)
> Rebase w.r.t i915_modparams change.
> 
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 15 ++++++++++++---
>  drivers/gpu/drm/i915/intel_uc.c            | 11 +++++++----
>  2 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index d1d6c0d..0c56765 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -885,9 +885,6 @@ static void guc_client_free(struct i915_guc_client *client)
>          * Be sure to drop any locks
>          */
>  
> -       /* FIXME: in many cases, by the time we get here the GuC has been
> -        * reset, so we cannot destroy the doorbell properly. Ignore the
> -        * error message for now */
>         destroy_doorbell(client);
>         guc_stage_desc_fini(client->guc, client);
>         i915_gem_object_unpin_map(client->vma->obj);
> @@ -1154,6 +1151,12 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
>                      sizeof(struct guc_wq_item) *
>                      I915_NUM_ENGINES > GUC_WQ_SIZE);
>  
> +       /*
> +        * Assert that drm.struct_mutex is held.

Ahem.

> +        * Needed for GuC client vma binding.
> +        */
> +       lockdep_assert_held(&dev_priv->drm.struct_mutex);

If you don't directly depend on struct_mutex, don't assert it. Otherwise
the person who removes that requirement will get very confused and upset.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 7/8] drm/i915/guc: Disable GuC submission and suspend it prior to i915 reset

[Chris Wilson]

Quoting Sagar Arun Kamble (2017-09-26 14:24:44)
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index d710f0d..aec3f6b 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -562,6 +562,14 @@ int intel_uc_resume(struct drm_i915_private *dev_priv)
>         return 0;
>  }
>  
> +int intel_uc_reset_prepare(struct drm_i915_private *dev_priv)
> +{
> +       if (i915_modparams.enable_guc_submission)
> +               i915_guc_submission_disable(dev_priv);

General rule of thumb is that in disable we want to check some dependent
state setup by enable, not a user parameter.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 6/8] drm/i915/guc: Update GuC suspend functionality in intel_uc_suspend

[Michał Winiarski]

On Tue, Sep 26, 2017 at 02:49:48PM +0100, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2017-09-26 14:24:43)
> > With this patch we disable GuC submission in i915_drm_suspend path.
> > This will destroy the client which will be setup back again. We also
> > reuse the complete sanitization done via intel_uc_runtime_suspend in
> > this path. Post i915_drm_resume, this state is recreated by
> > intel_uc_init_hw hence we need not have similar reuse for intel_uc_resume.
> > This also fixes issue where intel_uc_fini_hw was being called after GPU
> > reset happening in i915_gem_suspend during i915_driver_unload.
> > 
> > v2: Rebase w.r.t removal of GuC code restructuring. Added struct_mutex
> > protection for i915_guc_submission_disable.
> > 
> > v3: Rebase w.r.t updated GuC suspend function name.
> > 
> > v4: Added lockdep assert in i915_guc_submission_enable/disable.
> > Refined intel_uc_suspend to remove unnecessary locals and simplify
> > return. (Michal Winiarski)
> > Removed comment in guc_client_free about ignoring failure for
> > destroy_doorbell. (Oscar)
> > Rebase w.r.t i915_modparams change.
> > 
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Michał Winiarski <michal.winiarski@intel.com>
> > Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_guc_submission.c | 15 ++++++++++++---
> >  drivers/gpu/drm/i915/intel_uc.c            | 11 +++++++----
> >  2 files changed, 19 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> > index d1d6c0d..0c56765 100644
> > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > @@ -885,9 +885,6 @@ static void guc_client_free(struct i915_guc_client *client)
> >          * Be sure to drop any locks
> >          */
> >  
> > -       /* FIXME: in many cases, by the time we get here the GuC has been
> > -        * reset, so we cannot destroy the doorbell properly. Ignore the
> > -        * error message for now */
> >         destroy_doorbell(client);
> >         guc_stage_desc_fini(client->guc, client);
> >         i915_gem_object_unpin_map(client->vma->obj);
> > @@ -1154,6 +1151,12 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
> >                      sizeof(struct guc_wq_item) *
> >                      I915_NUM_ENGINES > GUC_WQ_SIZE);
> >  
> > +       /*
> > +        * Assert that drm.struct_mutex is held.
> 
> Ahem.
> 
> > +        * Needed for GuC client vma binding.
> > +        */
> > +       lockdep_assert_held(&dev_priv->drm.struct_mutex);
> 
> If you don't directly depend on struct_mutex, don't assert it. Otherwise
> the person who removes that requirement will get very confused and upset.
> -Chris

My bad - I suggested that.
Failed to notice *why* we're taking the mutex, and that we're already have
lockdep assert in the right place (__i915_gem_object_release_unless_active).

Sorry :)

-Michał

> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [v8,1/8] drm/i915: Create GEM runtime resume helper and handle GEM suspend/resume errors

[Patchwork]

== Series Details ==

Series: series starting with [v8,1/8] drm/i915: Create GEM runtime resume helper and handle GEM suspend/resume errors
URL   : https://patchwork.freedesktop.org/series/30905/
State : success

== Summary ==

Series 30905v1 series starting with [v8,1/8] drm/i915: Create GEM runtime resume helper and handle GEM suspend/resume errors
https://patchwork.freedesktop.org/api/1.0/series/30905/revisions/1/mbox/

Test drv_module_reload:
        Subgroup basic-reload-inject:
                dmesg-warn -> PASS       (fi-glk-1) fdo#102777

fdo#102777 https://bugs.freedesktop.org/show_bug.cgi?id=102777

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:438s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:464s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:422s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:520s
fi-bwr-2160      total:289  pass:184  dwarn:0   dfail:0   fail:0   skip:105 time:279s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:506s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:493s
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:493s
fi-cfl-s         total:289  pass:223  dwarn:34  dfail:0   fail:0   skip:32  time:539s
fi-cnl-y         total:289  pass:256  dwarn:0   dfail:0   fail:6   skip:27  time:640s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:416s
fi-glk-1         total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:565s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:429s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:406s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:432s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:489s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:464s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:469s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:574s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:599s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:549s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:451s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:748s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:492s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:478s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:564s
fi-snb-2600      total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:411s

6ca114186dba29c0ba0f3e379cbf33fff04faa67 drm-tip: 2017y-09m-26d-13h-21m-14s UTC integration manifest
4d19130569b8 drm/i915/guc: Fix GuC cleanup in unload path
e7d3b96c6a6d drm/i915/guc: Disable GuC submission and suspend it prior to i915 reset
ade16970b7ed drm/i915/guc: Update GuC suspend functionality in intel_uc_suspend
44b688a6e0d6 drm/i915/guc: Update GuC ggtt.invalidate/interrupts/communication across RPM suspend/resume
eb9d53327682 drm/i915/guc: Introduce intel_guc_sanitize
fb500df9fbeb drm/i915: Create uC runtime and system suspend/resume helpers
3a2044a41fcd drm/i915: Update GEM suspend/resume flows considering GuC and GEM fences
e237bcd3d31b drm/i915: Create GEM runtime resume helper and handle GEM suspend/resume errors

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5817/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 6/8] drm/i915/guc: Update GuC suspend functionality in intel_uc_suspend

[Sagar Arun Kamble]

On 9/26/2017 7:27 PM, Michał Winiarski wrote:
> On Tue, Sep 26, 2017 at 02:49:48PM +0100, Chris Wilson wrote:
>> Quoting Sagar Arun Kamble (2017-09-26 14:24:43)
>>> With this patch we disable GuC submission in i915_drm_suspend path.
>>> This will destroy the client which will be setup back again. We also
>>> reuse the complete sanitization done via intel_uc_runtime_suspend in
>>> this path. Post i915_drm_resume, this state is recreated by
>>> intel_uc_init_hw hence we need not have similar reuse for intel_uc_resume.
>>> This also fixes issue where intel_uc_fini_hw was being called after GPU
>>> reset happening in i915_gem_suspend during i915_driver_unload.
>>>
>>> v2: Rebase w.r.t removal of GuC code restructuring. Added struct_mutex
>>> protection for i915_guc_submission_disable.
>>>
>>> v3: Rebase w.r.t updated GuC suspend function name.
>>>
>>> v4: Added lockdep assert in i915_guc_submission_enable/disable.
>>> Refined intel_uc_suspend to remove unnecessary locals and simplify
>>> return. (Michal Winiarski)
>>> Removed comment in guc_client_free about ignoring failure for
>>> destroy_doorbell. (Oscar)
>>> Rebase w.r.t i915_modparams change.
>>>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>>> Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 15 ++++++++++++---
>>>   drivers/gpu/drm/i915/intel_uc.c            | 11 +++++++----
>>>   2 files changed, 19 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> index d1d6c0d..0c56765 100644
>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> @@ -885,9 +885,6 @@ static void guc_client_free(struct i915_guc_client *client)
>>>           * Be sure to drop any locks
>>>           */
>>>   
>>> -       /* FIXME: in many cases, by the time we get here the GuC has been
>>> -        * reset, so we cannot destroy the doorbell properly. Ignore the
>>> -        * error message for now */
>>>          destroy_doorbell(client);
>>>          guc_stage_desc_fini(client->guc, client);
>>>          i915_gem_object_unpin_map(client->vma->obj);
>>> @@ -1154,6 +1151,12 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
>>>                       sizeof(struct guc_wq_item) *
>>>                       I915_NUM_ENGINES > GUC_WQ_SIZE);
>>>   
>>> +       /*
>>> +        * Assert that drm.struct_mutex is held.
>> Ahem.
>>
>>> +        * Needed for GuC client vma binding.
>>> +        */
>>> +       lockdep_assert_held(&dev_priv->drm.struct_mutex);
>> If you don't directly depend on struct_mutex, don't assert it. Otherwise
>> the person who removes that requirement will get very confused and upset.
>> -Chris
> My bad - I suggested that.
> Failed to notice *why* we're taking the mutex, and that we're already have
> lockdep assert in the right place (__i915_gem_object_release_unless_active).
>
> Sorry :)
>
> -Michał
Will remove this. Thanks.
>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 7/8] drm/i915/guc: Disable GuC submission and suspend it prior to i915 reset

[Sagar Arun Kamble]

On 9/26/2017 7:22 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2017-09-26 14:24:44)
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
>> index d710f0d..aec3f6b 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -562,6 +562,14 @@ int intel_uc_resume(struct drm_i915_private *dev_priv)
>>          return 0;
>>   }
>>   
>> +int intel_uc_reset_prepare(struct drm_i915_private *dev_priv)
>> +{
>> +       if (i915_modparams.enable_guc_submission)
>> +               i915_guc_submission_disable(dev_priv);
> General rule of thumb is that in disable we want to check some dependent
> state setup by enable, not a user parameter.
> -Chris
Ok. then will update the disable path to check if execbuf_client is 
setup and call it without checking for modparam.
Thanks.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 4/8] drm/i915/guc: Introduce intel_guc_sanitize

[Sagar Arun Kamble]

On 9/26/2017 7:16 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2017-09-26 14:24:41)
>> Currently GPU is reset at the end of suspend via i915_gem_sanitize.
>> On resume, GuC will not be loaded until intel_uc_init_hw happens
>> during GEM resume flow but action to exit sleep wll be send to GuC
>> considering the FW load status. To make sure we don't invoke that
>> action update GuC FW load status through new helper
>> intel_guc_sanitize. Conditional check based on FW fetch status should
>> take care of driver init flow.
>>
>> v2: Rebase.
>>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem.c | 2 ++
>>   drivers/gpu/drm/i915/intel_uc.c | 8 ++++++++
>>   drivers/gpu/drm/i915/intel_uc.h | 1 +
>>   3 files changed, 11 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 8e6e2bd..0d9a107 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -4528,6 +4528,8 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
>>                  mutex_unlock(&i915->drm.struct_mutex);
>>          }
>>   
>> +       intel_guc_sanitize(&i915->guc);
>> +
>>          /*
>>           * If we inherit context state from the BIOS or earlier occupants
>>           * of the GPU, the GPU may be in an inconsistent state when we
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
>> index b9376e4..29d43f8 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -581,3 +581,11 @@ int intel_guc_sample_forcewake(struct intel_guc *guc)
>>   
>>          return intel_guc_send(guc, action, ARRAY_SIZE(action));
>>   }
>> +
>> +void intel_guc_sanitize(struct intel_guc *guc)
>> +{
>> +       if (guc->fw.fetch_status == INTEL_UC_FIRMWARE_SUCCESS)
>> +               guc->fw.load_status = INTEL_UC_FIRMWARE_PENDING;
>> +       else
>> +               guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
> No, sanitize should not be carrying old state over. We are supposed to
> be scrubbing hw state. (Elsewhere we detaint user state.)
> -Chris
Will make this " guc->fw.load_status = INTEL_UC_FIRMWARE_NONE; " 
unconditionally as GuC FW will not be loaded from hereon.
Will not depend on fetch status.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 3/8] drm/i915: Create uC runtime and system suspend/resume helpers

[Sagar Arun Kamble]

On 9/26/2017 7:13 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2017-09-26 14:24:40)
>> @@ -2077,7 +2079,7 @@ int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
>>                  reg->dirty = true;
>>          }
>>   
>> -       return 0;
>> +       return ret;
> OI! These are written as return 0 because you are very much not meant to
> reach this point with an error.
> -Chris
What if GuC suspend fail? Do we still want to return 0. It might cause 
issues as GuC will be active when PCI Device is set to D3.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 2/8] drm/i915: Update GEM suspend/resume flows considering GuC and GEM fences

[Sagar Arun Kamble]

On 9/26/2017 7:12 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2017-09-26 14:24:39)
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index dbe181b..5dcd8c0 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2022,11 +2022,22 @@ int i915_gem_fault(struct vm_fault *vmf)
>>          intel_runtime_pm_put(i915);
>>   }
>>   
>> +/**
>> + * i915_gem_runtime_suspend() - Finish GEM suspend
>> + * @dev_priv: i915 device private
>> + *
>> + * This function suspends GuC, removes userspace mappings for all GEM obejcts
>> + * currently on userfault list and marks fences if any being used as lost.
>> + *
>> + * Return:     non-zero code on error
>> + */
>> +/**
>> + * i915_gem_runtime_resume() - Restore GEM state
>> + * @dev_priv: i915 device private
>> + *
>> + * This function inits swizzling, restores fences and resumes GuC.
>> + *
>> + * Return:     non-zero code on error
>> + */
>> +/**
>> + * i915_gem_suspend() - Suspend all GT activity.
>> + * @dev_priv: i915 device private
>> + *
>> + * This function disables RPS, flushes all executing context ensuring
>> + * GEM/GT/Engines idleness, cancels all work that needs GT access and suspends
>> + * GuC. In the end currently, it also reset the GEM state and GPU HW.
>> + *
>> + * Return:     non-zero code on error
>> + */
>> +/**
>> + * i915_gem_resume() - Resume GT activity.
>> + * @dev_priv: i915 device private
>> + *
>> + * This function restores GTT mappings, restores fences and resets the
>> + * context images and resumes GuC.
>> + *
>> + * Return:     non-zero code on error
>> + */
> I can very much read what the functions do. These comments blocks are
> for telling me when to call them, under what pre/post-conditions and the
> overall intent of the function.
>
> Who are these blocks for? This isn't a library interface we expect to be
> used, these are hooks.
> -Chris
Since these functions are exported from i915_gem.c and are being used 
from respective drm functions, to make
the semantics available in kernel docs I have added these comments.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 2/8] drm/i915: Update GEM suspend/resume flows considering GuC and GEM fences

[Chris Wilson]

Quoting Sagar Arun Kamble (2017-09-26 15:24:15)
> 
> 
> On 9/26/2017 7:12 PM, Chris Wilson wrote:
> > Quoting Sagar Arun Kamble (2017-09-26 14:24:39)
> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >> index dbe181b..5dcd8c0 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem.c
> >> @@ -2022,11 +2022,22 @@ int i915_gem_fault(struct vm_fault *vmf)
> >>          intel_runtime_pm_put(i915);
> >>   }
> >>   
> >> +/**
> >> + * i915_gem_runtime_suspend() - Finish GEM suspend
> >> + * @dev_priv: i915 device private
> >> + *
> >> + * This function suspends GuC, removes userspace mappings for all GEM obejcts
> >> + * currently on userfault list and marks fences if any being used as lost.
> >> + *
> >> + * Return:     non-zero code on error
> >> + */
> >> +/**
> >> + * i915_gem_runtime_resume() - Restore GEM state
> >> + * @dev_priv: i915 device private
> >> + *
> >> + * This function inits swizzling, restores fences and resumes GuC.
> >> + *
> >> + * Return:     non-zero code on error
> >> + */
> >> +/**
> >> + * i915_gem_suspend() - Suspend all GT activity.
> >> + * @dev_priv: i915 device private
> >> + *
> >> + * This function disables RPS, flushes all executing context ensuring
> >> + * GEM/GT/Engines idleness, cancels all work that needs GT access and suspends
> >> + * GuC. In the end currently, it also reset the GEM state and GPU HW.
> >> + *
> >> + * Return:     non-zero code on error
> >> + */
> >> +/**
> >> + * i915_gem_resume() - Resume GT activity.
> >> + * @dev_priv: i915 device private
> >> + *
> >> + * This function restores GTT mappings, restores fences and resets the
> >> + * context images and resumes GuC.
> >> + *
> >> + * Return:     non-zero code on error
> >> + */
> > I can very much read what the functions do. These comments blocks are
> > for telling me when to call them, under what pre/post-conditions and the
> > overall intent of the function.
> >
> > Who are these blocks for? This isn't a library interface we expect to be
> > used, these are hooks.
> > -Chris
> Since these functions are exported from i915_gem.c and are being used 
> from respective drm functions, to make
> the semantics available in kernel docs I have added these comments.

My point is that this isn't functionality that was exported for somebody
to call, this was functionality that was pulled from i915_drv.c for a
singular purpose. It is driven by that context, and not for general
purpose. So who is the consumer?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 2/8] drm/i915: Update GEM suspend/resume flows considering GuC and GEM fences

[Chris Wilson]

Subject includes the word "fences". That was quite surprising as it
doesn't involve either dma, sw or guc fences; or the fence registers.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 2/8] drm/i915: Update GEM suspend/resume flows considering GuC and GEM fences

[Sagar Arun Kamble]

[-- Attachment #1.1: Type: text/plain, Size: 365 bytes --]



On 9/26/2017 8:18 PM, Chris Wilson wrote:
> Subject includes the word "fences". That was quite surprising as it
> doesn't involve either dma, sw or guc fences; or the fence registers.
> -Chris
This was related to fence registers whose state is updated in 
i915_gem_restore_fences. With this patch I had moved the
this to i915_gem_resume from i915_restore_state.


[-- Attachment #1.2: Type: text/html, Size: 36337 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [v8,1/8] drm/i915: Create GEM runtime resume helper and handle GEM suspend/resume errors

[Patchwork]

== Series Details ==

Series: series starting with [v8,1/8] drm/i915: Create GEM runtime resume helper and handle GEM suspend/resume errors
URL   : https://patchwork.freedesktop.org/series/30905/
State : success

== Summary ==

Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-offscren-pri-indfb-draw-render:
                skip       -> PASS       (shard-hsw)
Test perf:
        Subgroup blocking:
                fail       -> PASS       (shard-hsw) fdo#102252

fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252

shard-hsw        total:2429 pass:1327 dwarn:5   dfail:0   fail:14  skip:1083 time:9856s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5817/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx