[PATCH v10 0/9] GEM/GuC Suspend/Resume/Reset fixes and restructuring

[PATCH v10 0/9] GEM/GuC Suspend/Resume/Reset fixes and restructuring

[Sagar Arun Kamble]

Older series can be found at
https://patchwork.freedesktop.org/series/30715/
https://patchwork.freedesktop.org/series/30502/
https://patchwork.freedesktop.org/series/30351/

v1-v8: Part of above three series.

v9:
Fixed patch 1 based on review inputs from Michal Winiarski.
Rebased all patches. Updated ordering of cc, s-o-b, r-b tags for all patches.

v10: Added new patch (6th). Addressed reviews on v9.

Sagar Arun Kamble (9):
  drm/i915: Create GEM runtime resume helper and handle GEM
    suspend/resume errors
  drm/i915: Update GEM suspend/resume flows considering GuC and GEM
    fences
  drm/i915: Create uC runtime and system suspend/resume helpers
  drm/i915/guc: Update GuC load status as NONE on GPU reset
  drm/i915/guc: Update GuC ggtt.invalidate/interrupts/communication
    across RPM suspend/resume
  drm/i915/guc: Check execbuf client to disable submission and don't
    depend on enable_guc_submission
  drm/i915/guc: Update GuC suspend functionality in intel_uc_suspend
  drm/i915/guc: Disable GuC submission and suspend it prior to i915
    reset
  drm/i915/guc: Fix GuC cleanup in unload path

 drivers/gpu/drm/i915/i915_drv.c            | 37 +++++++-----
 drivers/gpu/drm/i915/i915_drv.h            |  5 +-
 drivers/gpu/drm/i915/i915_gem.c            | 53 +++++++++++++++--
 drivers/gpu/drm/i915/i915_guc_submission.c | 13 ++---
 drivers/gpu/drm/i915/i915_suspend.c        |  2 -
 drivers/gpu/drm/i915/intel_uc.c            | 94 +++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_uc.h            |  9 ++-
 drivers/gpu/drm/i915/intel_uncore.c        | 10 ++++
 8 files changed, 179 insertions(+), 44 deletions(-)

-- 
1.9.1

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

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

[Sagar Arun Kamble]

These changes are preparation to handle GuC suspend/resume. Prepared
helper i915_gem_runtime_resume to reinitialize suspended 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)

v3: Not returning status from gem_runtime_resume. (Chris)

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
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>
---
 drivers/gpu/drm/i915/i915_drv.c | 22 +++++++++++++---------
 drivers/gpu/drm/i915/i915_drv.h |  5 +++--
 drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++++--
 3 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 7056bb2..d0a710d 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;
@@ -2596,13 +2605,6 @@ static int intel_runtime_resume(struct device *kdev)
 		ret = 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,6 +2617,8 @@ static int intel_runtime_resume(struct device *kdev)
 
 	intel_enable_ipc(dev_priv);
 
+	i915_gem_runtime_resume(dev_priv);
+
 	enable_rpm_wakeref_asserts(dev_priv);
 
 	if (ret)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b7cba89..42f3d89 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3479,7 +3479,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);
+void i915_gem_runtime_resume(struct drm_i915_private *dev_priv);
 
 static inline int __sg_page_count(const struct scatterlist *sg)
 {
@@ -3682,7 +3683,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..59a88f2 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,18 @@ 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;
+}
+
+void 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);
 }
 
 static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
@@ -4587,7 +4599,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 +4615,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 v10 2/9] 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.
i915_gem_restore_fences is GEM resumption task hence it is moved to
i915_gem_resume from i915_restore_state.

v2: Removed documentation of suspend/resume handlers as those are not
interfaces and are just hooks. (Chris)

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
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>
---
 drivers/gpu/drm/i915/i915_drv.c     |  7 -------
 drivers/gpu/drm/i915/i915_gem.c     | 11 ++++++++---
 drivers/gpu/drm/i915/i915_suspend.c |  2 --
 3 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index d0a710d..174a7c5 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;
@@ -2522,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);
 
@@ -2591,8 +2586,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 59a88f2..11922af 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2027,6 +2027,8 @@ 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
@@ -2077,6 +2079,8 @@ void 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);
 }
 
 static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
@@ -4551,8 +4555,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);
 
@@ -4569,6 +4571,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,
@@ -4607,13 +4611,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 v10 3/9] 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.
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.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 174a7c5..f79646b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1698,6 +1698,16 @@ static int i915_drm_resume(struct drm_device *dev)
 	}
 	mutex_unlock(&dev->struct_mutex);
 
+	/*
+	 * NB: Currently we know that at the end of suspend we have done Full
+	 * GPU reset, Hence 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.
+	 */
+	ret = intel_uc_resume(dev_priv);
+	if (ret)
+		i915_gem_set_wedged(dev_priv);
+
 	intel_modeset_init_hw(dev);
 
 	spin_lock_irq(&dev_priv->irq_lock);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 11922af..c54c9b4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2025,9 +2025,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
@@ -2068,7 +2070,7 @@ int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
 		reg->dirty = true;
 	}
 
-	return 0;
+	return ret;
 }
 
 void i915_gem_runtime_resume(struct drm_i915_private *dev_priv)
@@ -2080,7 +2082,7 @@ void 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);
+	intel_uc_runtime_resume(dev_priv);
 }
 
 static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
@@ -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 */
 
 	/*
 	 * Neither the BIOS, ourselves or any other kernel
@@ -4606,6 +4610,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);
 
@@ -4618,10 +4623,20 @@ 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: At the end of suspend, Full GPU reset is being done which
+	 * wipes/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.
+	 */
+	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)
@@ -4741,7 +4756,12 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
 
 	intel_mocs_init_l3cc_table(dev_priv);
 
-	/* We can't enable contexts until all firmware is loaded */
+	/*
+	 * We can't enable contexts until all firmware is loaded.
+	 * FIXME: GuC is loaded unconditionally currently without checking
+	 * if it is already available in WOPCM. We can skip that load based
+	 * on WOPCM status.
+	 */
 	ret = intel_uc_init_hw(dev_priv);
 	if (ret)
 		goto out;
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 2774778..7b30790 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -481,6 +481,26 @@ 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)
+{
+	return intel_guc_resume(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 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 v10 4/9] drm/i915/guc: Update GuC load status as NONE on GPU reset

[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 can be sent to GuC
considering the FW load status. To make sure we don't invoke that
action update GuC FW load status at the end of GPU reset as NONE.

v2: Rebase.

v3: Removed intel_guc_sanitize. Marking load status as NONE at the
GPU reset point. (Chris/Michal)

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_uncore.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index b3c3f94..83300f3 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1763,6 +1763,16 @@ int intel_gpu_reset(struct drm_i915_private *dev_priv, unsigned engine_mask)
 	}
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 
+	/*
+	 * FIXME: intel_uc_resume currently depends on load_status to resume
+	 * GuC. Since we are resetting Full GPU at the end of suspend, let us
+	 * mark the load status as NONE. Once intel_uc_resume is updated to take
+	 * into consideration GuC load state based on WOPCM, we can skip this
+	 * state change.
+	 */
+	if (engine_mask == ALL_ENGINES)
+		dev_priv->guc.fw.load_status = INTEL_UC_FIRMWARE_NONE;
+
 	return ret;
 }
 
-- 
1.9.1

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

[PATCH v10 5/9] 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.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@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 7b30790..1446b5e 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 v10 6/9] drm/i915/guc: Check execbuf client to disable submission and don't depend on enable_guc_submission

[Sagar Arun Kamble]

We should check dependent state setup by enable path to run disable path
and not depend on the user parameters. i915_guc_submission_disable now
checks if execbuf client is setup and then goes ahead with disabling.

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

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index d1d6c0d..d6aaf43 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1204,6 +1204,9 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
 
+	if (!guc->execbuf_client)
+		return;
+
 	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 1446b5e..c58b3b6 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -468,8 +468,7 @@ 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);
+	i915_guc_submission_disable(dev_priv);
 
 	guc_disable_communication(&dev_priv->guc);
 
-- 
1.9.1

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

[PATCH v10 7/9] 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.

v5: Removed lockdep assert as mutex is needed by internal functions
which already have the asserts. (Chris)
Removed enable_guc_submission check for disabling GuC submission. (Chris)

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

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index d6aaf43..45f2ee8 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);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index c58b3b6..4dd8106 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -468,8 +468,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 	if (!i915_modparams.enable_guc_loading)
 		return;
 
-	i915_guc_submission_disable(dev_priv);
-
 	guc_disable_communication(&dev_priv->guc);
 
 	if (i915_modparams.enable_guc_submission) {
@@ -548,7 +546,11 @@ 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);
+	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 v10 8/9] 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.

v4: Rebase. Removed enable_guc_submission check for disabling GuC
submission. (Chris)

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c54c9b4..19256d8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2865,6 +2865,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 4dd8106..59e6995 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -558,6 +558,13 @@ int intel_uc_resume(struct drm_i915_private *dev_priv)
 	return intel_guc_resume(dev_priv);
 }
 
+int intel_uc_reset_prepare(struct drm_i915_private *dev_priv)
+{
+	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 0a79e17..78ccbd9 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);
 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 v10 9/9] 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)

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@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 f79646b..4223cee 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 45f2ee8..0ac9bd4 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 59e6995..0370265 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 78ccbd9..838a364 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);
@@ -239,7 +239,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

✓ Fi.CI.BAT: success for GEM/GuC Suspend/Resume/Reset fixes and restructuring (rev3)

[Patchwork]

== Series Details ==

Series: GEM/GuC Suspend/Resume/Reset fixes and restructuring (rev3)
URL   : https://patchwork.freedesktop.org/series/30802/
State : success

== Summary ==

Series 30802v3 GEM/GuC Suspend/Resume/Reset fixes and restructuring
https://patchwork.freedesktop.org/api/1.0/series/30802/revisions/3/mbox/

Test chamelium:
        Subgroup dp-crc-fast:
                pass       -> FAIL       (fi-kbl-7500u) fdo#102514
Test pm_rpm:
        Subgroup basic-rte:
                pass       -> DMESG-WARN (fi-cfl-s) fdo#102294

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

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:440s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:475s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:418s
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:278s
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:490s
fi-cfl-s         total:289  pass:222  dwarn:35  dfail:0   fail:0   skip:32  time:547s
fi-cnl-y         total:289  pass:259  dwarn:0   dfail:0   fail:3   skip:27  time:642s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:421s
fi-glk-1         total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:562s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:423s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:404s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:433s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:496s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:463s
fi-kbl-7500u     total:289  pass:263  dwarn:1   dfail:0   fail:1   skip:24  time:458s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:575s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:587s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:543s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:454s
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:486s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:476s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:565s
fi-snb-2600      total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:413s

87ad73bb7b230cb2b75aa3825152ca848232e208 drm-tip: 2017y-09m-27d-08h-54m-19s UTC integration manifest
99129cf9efc0 drm/i915/guc: Fix GuC cleanup in unload path
c1f95480b050 drm/i915/guc: Disable GuC submission and suspend it prior to i915 reset
dcc8af2aa2a5 drm/i915/guc: Update GuC suspend functionality in intel_uc_suspend
40135db3c6ac drm/i915/guc: Check execbuf client to disable submission and don't depend on enable_guc_submission
b7778865cef4 drm/i915/guc: Update GuC ggtt.invalidate/interrupts/communication across RPM suspend/resume
2404ace040ba drm/i915/guc: Update GuC load status as NONE on GPU reset
e0ae61d4cccc drm/i915: Create uC runtime and system suspend/resume helpers
f1ebaaa824ee drm/i915: Update GEM suspend/resume flows considering GuC and GEM fences
1fe68bdbb563 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_5828/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: warning for GEM/GuC Suspend/Resume/Reset fixes and restructuring (rev3)

[Patchwork]

== Series Details ==

Series: GEM/GuC Suspend/Resume/Reset fixes and restructuring (rev3)
URL   : https://patchwork.freedesktop.org/series/30802/
State : warning

== Summary ==

Test perf:
        Subgroup blocking:
                fail       -> PASS       (shard-hsw) fdo#102252
Test kms_cursor_legacy:
        Subgroup cursorA-vs-flipA-atomic-transitions-varying-size:
                pass       -> SKIP       (shard-hsw)
        Subgroup nonblocking-modeset-vs-cursor-atomic:
                pass       -> SKIP       (shard-hsw)
Test kms_draw_crc:
        Subgroup draw-method-xrgb8888-mmap-gtt-untiled:
                pass       -> SKIP       (shard-hsw)

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

shard-hsw        total:2429 pass:1333 dwarn:1   dfail:0   fail:9   skip:1086 time:9907s

== Logs ==

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

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

[Michal Wajdeczko]

On Wed, 27 Sep 2017 11:30:31 +0200, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> These changes are preparation to handle GuC suspend/resume. Prepared
> helper i915_gem_runtime_resume to reinitialize suspended 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)
>
> v3: Not returning status from gem_runtime_resume. (Chris)
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> 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>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 22 +++++++++++++---------
>  drivers/gpu/drm/i915/i915_drv.h |  5 +++--
>  drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++++--
>  3 files changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c  
> b/drivers/gpu/drm/i915/i915_drv.c
> index 7056bb2..d0a710d 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");

btw, code above/below uses DRM_ERROR()

> 	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;

as this is second place where we exit, maybe it's time for

	if (ret)
		goto fail;
fail:
	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;
> @@ -2596,13 +2605,6 @@ static int intel_runtime_resume(struct device  
> *kdev)
>  		ret = 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,6 +2617,8 @@ static int intel_runtime_resume(struct device  
> *kdev)
> 	intel_enable_ipc(dev_priv);
> +	i915_gem_runtime_resume(dev_priv);
> +
>  	enable_rpm_wakeref_asserts(dev_priv);
> 	if (ret)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h  
> b/drivers/gpu/drm/i915/i915_drv.h
> index b7cba89..42f3d89 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3479,7 +3479,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);
> +void i915_gem_runtime_resume(struct drm_i915_private *dev_priv);
> static inline int __sg_page_count(const struct scatterlist *sg)
>  {
> @@ -3682,7 +3683,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..59a88f2 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,18 @@ 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;
> +}
> +
> +void 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);
>  }
> static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object  
> *obj)
> @@ -4587,7 +4599,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 +4615,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 v10 2/9] drm/i915: Update GEM suspend/resume flows considering GuC and GEM fences

[Michal Wajdeczko]

On Wed, 27 Sep 2017 11:30:32 +0200, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> This patch moves GuC suspend/resume handlers to corresponding GEM  
> handlers
> and orders them properly in the runtime and system suspend/resume flows.
> i915_gem_restore_fences is GEM resumption task hence it is moved to
> i915_gem_resume from i915_restore_state.
>
> v2: Removed documentation of suspend/resume handlers as those are not
> interfaces and are just hooks. (Chris)
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> 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>
> ---
>  drivers/gpu/drm/i915/i915_drv.c     |  7 -------
>  drivers/gpu/drm/i915/i915_gem.c     | 11 ++++++++---
>  drivers/gpu/drm/i915/i915_suspend.c |  2 --
>  3 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c  
> b/drivers/gpu/drm/i915/i915_drv.c
> index d0a710d..174a7c5 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;
> @@ -2522,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);
> @@ -2591,8 +2586,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 59a88f2..11922af 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2027,6 +2027,8 @@ 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
> @@ -2077,6 +2079,8 @@ void 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);
>  }
> static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object  
> *obj)
> @@ -4551,8 +4555,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);
> @@ -4569,6 +4571,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,
> @@ -4607,13 +4611,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);
> -

I would leave that empty line

> +	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);
> -

And this move of i915_gem_restore_fences() can likely be in separate patch

Michal

>  	if (IS_GEN4(dev_priv))
>  		pci_write_config_word(pdev, GCDGMBUS,
>  				      dev_priv->regfile.saveGCDGMBUS);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Michal Wajdeczko]

On Wed, 27 Sep 2017 11:30:33 +0200, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> 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.
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++++
>  drivers/gpu/drm/i915/i915_gem.c | 36  
> ++++++++++++++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_uc.c | 20 ++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uc.h |  4 ++++
>  4 files changed, 62 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c  
> b/drivers/gpu/drm/i915/i915_drv.c
> index 174a7c5..f79646b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1698,6 +1698,16 @@ static int i915_drm_resume(struct drm_device *dev)
>  	}
>  	mutex_unlock(&dev->struct_mutex);
> +	/*
> +	 * NB: Currently we know that at the end of suspend we have done Full
> +	 * GPU reset, Hence 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.
> +	 */
> +	ret = intel_uc_resume(dev_priv);
> +	if (ret)
> +		i915_gem_set_wedged(dev_priv);
> +
>  	intel_modeset_init_hw(dev);
> 	spin_lock_irq(&dev_priv->irq_lock);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c  
> b/drivers/gpu/drm/i915/i915_gem.c
> index 11922af..c54c9b4 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2025,9 +2025,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
> @@ -2068,7 +2070,7 @@ int i915_gem_runtime_suspend(struct  
> drm_i915_private *dev_priv)
>  		reg->dirty = true;
>  	}
> -	return 0;
> +	return ret;

Hmm, at this point we know that we didn't fail, so return 0 was fine.

>  }
> void i915_gem_runtime_resume(struct drm_i915_private *dev_priv)
> @@ -2080,7 +2082,7 @@ void 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);
> +	intel_uc_runtime_resume(dev_priv);
>  }
> static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object  
> *obj)
> @@ -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 */
> 	/*
>  	 * Neither the BIOS, ourselves or any other kernel
> @@ -4606,6 +4610,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);
> @@ -4618,10 +4623,20 @@ 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: At the end of suspend, Full GPU reset is being done which
> +	 * wipes/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.
> +	 */
> +	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)
> @@ -4741,7 +4756,12 @@ int i915_gem_init_hw(struct drm_i915_private  
> *dev_priv)
> 	intel_mocs_init_l3cc_table(dev_priv);
> -	/* We can't enable contexts until all firmware is loaded */
> +	/*
> +	 * We can't enable contexts until all firmware is loaded.
> +	 * FIXME: GuC is loaded unconditionally currently without checking
> +	 * if it is already available in WOPCM. We can skip that load based
> +	 * on WOPCM status.

hmm, maybe this comment should be in intel_uc_init_hw ?

> +	 */
>  	ret = intel_uc_init_hw(dev_priv);
>  	if (ret)
>  		goto out;
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index 2774778..7b30790 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -481,6 +481,26 @@ 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)
> +{
> +	return intel_guc_resume(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 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);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v10 4/9] drm/i915/guc: Update GuC load status as NONE on GPU reset

[Michal Wajdeczko]

On Wed, 27 Sep 2017 11:30:34 +0200, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> 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 can be sent to GuC
> considering the FW load status. To make sure we don't invoke that
> action update GuC FW load status at the end of GPU reset as NONE.
>
> v2: Rebase.
>
> v3: Removed intel_guc_sanitize. Marking load status as NONE at the
> GPU reset point. (Chris/Michal)

Hmm, I'm not sure that touching guc private member from the outside
of guc/uc code is a good idea. Maybe we should keep call  
intel_uc_sanitize()
but call it from i915_gem_reset() as that place looks more appropriate?

Michal

>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c  
> b/drivers/gpu/drm/i915/intel_uncore.c
> index b3c3f94..83300f3 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1763,6 +1763,16 @@ int intel_gpu_reset(struct drm_i915_private  
> *dev_priv, unsigned engine_mask)
>  	}
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> +	/*
> +	 * FIXME: intel_uc_resume currently depends on load_status to resume
> +	 * GuC. Since we are resetting Full GPU at the end of suspend, let us
> +	 * mark the load status as NONE. Once intel_uc_resume is updated to  
> take
> +	 * into consideration GuC load state based on WOPCM, we can skip this
> +	 * state change.
> +	 */
> +	if (engine_mask == ALL_ENGINES)
> +		dev_priv->guc.fw.load_status = INTEL_UC_FIRMWARE_NONE;
> +
>  	return ret;
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Michal Wajdeczko]

On Wed, 27 Sep 2017 11:30:35 +0200, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> 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.
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@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;

Btw, this check can be also moved to intel_uc_runtime_suspend()


> -	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;

And above to intel_uc_runtime_resume()

> -	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 7b30790..1446b5e 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)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sagar Arun Kamble]

On 9/27/2017 9:11 PM, Michal Wajdeczko wrote:
> On Wed, 27 Sep 2017 11:30:31 +0200, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>> These changes are preparation to handle GuC suspend/resume. Prepared
>> helper i915_gem_runtime_resume to reinitialize suspended 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)
>>
>> v3: Not returning status from gem_runtime_resume. (Chris)
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> 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>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.c | 22 +++++++++++++---------
>>  drivers/gpu/drm/i915/i915_drv.h |  5 +++--
>>  drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++++--
>>  3 files changed, 32 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index 7056bb2..d0a710d 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");
>
> btw, code above/below uses DRM_ERROR()
This was in symmetry with i915_gem_suspend return handling.
>
>>     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;
>
> as this is second place where we exit, maybe it's time for
>
>     if (ret)
>         goto fail;
> fail:
>     enable_rpm_wakeref_asserts(dev_priv);
>     return ret;
> }
>
Yes. Will update.
>> +    }
>>     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;
>> @@ -2596,13 +2605,6 @@ static int intel_runtime_resume(struct device 
>> *kdev)
>>          ret = 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,6 +2617,8 @@ static int intel_runtime_resume(struct device 
>> *kdev)
>>     intel_enable_ipc(dev_priv);
>> +    i915_gem_runtime_resume(dev_priv);
>> +
>>      enable_rpm_wakeref_asserts(dev_priv);
>>     if (ret)
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index b7cba89..42f3d89 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3479,7 +3479,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);
>> +void i915_gem_runtime_resume(struct drm_i915_private *dev_priv);
>> static inline int __sg_page_count(const struct scatterlist *sg)
>>  {
>> @@ -3682,7 +3683,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..59a88f2 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,18 @@ 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;
>> +}
>> +
>> +void 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);
>>  }
>> static int i915_gem_object_create_mmap_offset(struct 
>> drm_i915_gem_object *obj)
>> @@ -4587,7 +4599,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 +4615,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 v10 2/9] drm/i915: Update GEM suspend/resume flows considering GuC and GEM fences

[Sagar Arun Kamble]

On 9/27/2017 9:17 PM, Michal Wajdeczko wrote:
> On Wed, 27 Sep 2017 11:30:32 +0200, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>> This patch moves GuC suspend/resume handlers to corresponding GEM 
>> handlers
>> and orders them properly in the runtime and system suspend/resume flows.
>> i915_gem_restore_fences is GEM resumption task hence it is moved to
>> i915_gem_resume from i915_restore_state.
>>
>> v2: Removed documentation of suspend/resume handlers as those are not
>> interfaces and are just hooks. (Chris)
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> 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>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.c     |  7 -------
>>  drivers/gpu/drm/i915/i915_gem.c     | 11 ++++++++---
>>  drivers/gpu/drm/i915/i915_suspend.c |  2 --
>>  3 files changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index d0a710d..174a7c5 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;
>> @@ -2522,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);
>> @@ -2591,8 +2586,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 59a88f2..11922af 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2027,6 +2027,8 @@ 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
>> @@ -2077,6 +2079,8 @@ void 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);
>>  }
>> static int i915_gem_object_create_mmap_offset(struct 
>> drm_i915_gem_object *obj)
>> @@ -4551,8 +4555,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);
>> @@ -4569,6 +4571,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,
>> @@ -4607,13 +4611,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);
>> -
>
> I would leave that empty line
Sure. Will update.
>
>> +    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);
>> -
>
> And this move of i915_gem_restore_fences() can likely be in separate 
> patch
>
> Michal
Ok. Will create separate patch.
>
>>      if (IS_GEN4(dev_priv))
>>          pci_write_config_word(pdev, GCDGMBUS,
>>                        dev_priv->regfile.saveGCDGMBUS);

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

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

[Sagar Arun Kamble]

On 9/27/2017 9:26 PM, Michal Wajdeczko wrote:
> On Wed, 27 Sep 2017 11:30:33 +0200, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> 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.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++++
>>  drivers/gpu/drm/i915/i915_gem.c | 36 
>> ++++++++++++++++++++++++++++--------
>>  drivers/gpu/drm/i915/intel_uc.c | 20 ++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_uc.h |  4 ++++
>>  4 files changed, 62 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index 174a7c5..f79646b 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1698,6 +1698,16 @@ static int i915_drm_resume(struct drm_device 
>> *dev)
>>      }
>>      mutex_unlock(&dev->struct_mutex);
>> +    /*
>> +     * NB: Currently we know that at the end of suspend we have done 
>> Full
>> +     * GPU reset, Hence 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.
>> +     */
>> +    ret = intel_uc_resume(dev_priv);
>> +    if (ret)
>> +        i915_gem_set_wedged(dev_priv);
>> +
>>      intel_modeset_init_hw(dev);
>>     spin_lock_irq(&dev_priv->irq_lock);
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index 11922af..c54c9b4 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2025,9 +2025,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
>> @@ -2068,7 +2070,7 @@ int i915_gem_runtime_suspend(struct 
>> drm_i915_private *dev_priv)
>>          reg->dirty = true;
>>      }
>> -    return 0;
>> +    return ret;
>
> Hmm, at this point we know that we didn't fail, so return 0 was fine.
Ok. Will update. ret was also zero and I though returning ret will be 
good in future if we use goto to the end. But can be updated later if 
needed.
>
>>  }
>> void i915_gem_runtime_resume(struct drm_i915_private *dev_priv)
>> @@ -2080,7 +2082,7 @@ void 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);
>> +    intel_uc_runtime_resume(dev_priv);
>>  }
>> static int i915_gem_object_create_mmap_offset(struct 
>> drm_i915_gem_object *obj)
>> @@ -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 */
>>     /*
>>       * Neither the BIOS, ourselves or any other kernel
>> @@ -4606,6 +4610,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);
>> @@ -4618,10 +4623,20 @@ 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: At the end of suspend, Full GPU reset is being done which
>> +     * wipes/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.
>> +     */
>> +    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)
>> @@ -4741,7 +4756,12 @@ int i915_gem_init_hw(struct drm_i915_private 
>> *dev_priv)
>>     intel_mocs_init_l3cc_table(dev_priv);
>> -    /* We can't enable contexts until all firmware is loaded */
>> +    /*
>> +     * We can't enable contexts until all firmware is loaded.
>> +     * FIXME: GuC is loaded unconditionally currently without checking
>> +     * if it is already available in WOPCM. We can skip that load based
>> +     * on WOPCM status.
>
> hmm, maybe this comment should be in intel_uc_init_hw ?
Sure. I will move it there before call to intel_huc_init_hw as this 
WOPCM check applies to HuC as well.
>
>> +     */
>>      ret = intel_uc_init_hw(dev_priv);
>>      if (ret)
>>          goto out;
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 2774778..7b30790 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -481,6 +481,26 @@ 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)
>> +{
>> +    return intel_guc_resume(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 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);

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

Re: [PATCH v10 9/9] drm/i915/guc: Fix GuC cleanup in unload path

[Michal Wajdeczko]

On Wed, 27 Sep 2017 11:30:39 +0200, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> 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)
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@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 f79646b..4223cee 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 45f2ee8..0ac9bd4 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 59e6995..0370265 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)

In patch 6/9 you said we should avoid looking at user params
but here you're still using it ...

> +		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 78ccbd9..838a364 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);
> @@ -239,7 +239,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 */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v10 9/9] drm/i915/guc: Fix GuC cleanup in unload path

[Sagar Arun Kamble]

On 9/27/2017 10:41 PM, Michal Wajdeczko wrote:
> On Wed, 27 Sep 2017 11:30:39 +0200, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>> 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)
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@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 f79646b..4223cee 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 45f2ee8..0ac9bd4 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 59e6995..0370265 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)
>
> In patch 6/9 you said we should avoid looking at user params
> but here you're still using it ...
>
Yes. I thought of taking that up in new series. Should I add another 
patch in this series itself to update such snippets?

>> + 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 78ccbd9..838a364 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);
>> @@ -239,7 +239,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 */

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

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

[Chris Wilson]

Quoting Sagar Arun Kamble (2017-09-27 10:30:31)
> These changes are preparation to handle GuC suspend/resume. Prepared
> helper i915_gem_runtime_resume to reinitialize suspended 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)
> 
> v3: Not returning status from gem_runtime_resume. (Chris)
> 
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> 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>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 22 +++++++++++++---------
>  drivers/gpu/drm/i915/i915_drv.h |  5 +++--
>  drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++++--
>  3 files changed, 32 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 7056bb2..d0a710d 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");

I am still uneasy about this. Later on in the resume we try to reinit
the hw under the assumption that this earlier step succeeded.

Previously we have tried to make sure that if GEM fails, we do not leave
the display in an unusable state. Is that still true?

>  
>         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;
> @@ -2596,13 +2605,6 @@ static int intel_runtime_resume(struct device *kdev)
>                 ret = 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).
> -        */

This comment shouldn't be attached to the following gem init operations
as they cannot fail. If they could, we should be throwing a warning here
as this may result in a change of swizzling/fencing state as seen by
userspace and therefore graphical corruption.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

Quoting Sagar Arun Kamble (2017-09-27 10:30:32)
> @@ -4607,13 +4611,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);

Seconded Michal's suggestion, otherwise it looks very clean.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

Quoting Sagar Arun Kamble (2017-09-27 10:30:33)
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 174a7c5..f79646b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1698,6 +1698,16 @@ static int i915_drm_resume(struct drm_device *dev)
>         }
>         mutex_unlock(&dev->struct_mutex);
>  
> +       /*
> +        * NB: Currently we know that at the end of suspend we have done Full
> +        * GPU reset, Hence 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.
> +        */

When you say "GuC is loaded again during i915_gem_init_hw" are you
thinking of the i915_reset call or i915_drm_resume call? We are using
intel_gpu_reset() to do the full device reset at suspend/resume, so
bypassing the i915_gem_init_hw from i915_reset. Could you clarify what
you mean by "hence"?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v10 6/9] drm/i915/guc: Check execbuf client to disable submission and don't depend on enable_guc_submission

[Chris Wilson]

Quoting Sagar Arun Kamble (2017-09-27 10:30:36)
> We should check dependent state setup by enable path to run disable path
> and not depend on the user parameters. i915_guc_submission_disable now
> checks if execbuf client is setup and then goes ahead with disabling.
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v10 9/9] drm/i915/guc: Fix GuC cleanup in unload path

[Chris Wilson]

Quoting Sagar Arun Kamble (2017-09-27 10:30:39)
> -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);

My preference would be for if (!guc->stage_desc_pool) return; inside
i915_guc_submission_cleanup().
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sagar Arun Kamble]

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



On 9/28/2017 5:15 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2017-09-27 10:30:33)
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 174a7c5..f79646b 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1698,6 +1698,16 @@ static int i915_drm_resume(struct drm_device *dev)
>>          }
>>          mutex_unlock(&dev->struct_mutex);
>>   
>> +       /*
>> +        * NB: Currently we know that at the end of suspend we have done Full
>> +        * GPU reset, Hence 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.
>> +        */
> When you say "GuC is loaded again during i915_gem_init_hw" are you
> thinking of the i915_reset call or i915_drm_resume call? We are using
> intel_gpu_reset() to do the full device reset at suspend/resume, so
> bypassing the i915_gem_init_hw from i915_reset. Could you clarify what
> you mean by "hence"?
> -Chris

Hi Chris,

I've updated the comments in the latest revision.
Please review v12 patches from 
https://patchwork.freedesktop.org/series/30802/.
Sorry I have pushed few more revisions in past few days as one of the 
igt fail due to patch change was not caught by trybot.

 From Latest patch:
@@ -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
+* 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 referred to intel_gpu_reset from i915_gem_suspend here.

Thanks
Sagar


[-- Attachment #1.2: Type: text/html, Size: 39127 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

Re: [PATCH v10 9/9] drm/i915/guc: Fix GuC cleanup in unload path

[Sagar Arun Kamble]

On 9/28/2017 5:25 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2017-09-27 10:30:39)
>> -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);
> My preference would be for if (!guc->stage_desc_pool) return; inside
> i915_guc_submission_cleanup().
> -Chris
Yes. I have taken similar input in the latest patch - 
https://patchwork.freedesktop.org/patch/179405/
In i915_guc_submission_cleanup we can cover destroy of stage_ids and 
stage_desc_pool based on check you have suggested.
  guc_ads_destroy is always required data so should we link with 
stage_desc_pool check?
intel_guc_log is optional so destroy need to be made dependent on 
guc->log.vma

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

Re: [PATCH v10 6/9] drm/i915/guc: Check execbuf client to disable submission and don't depend on enable_guc_submission

[Sagar Arun Kamble]

On 9/28/2017 5:17 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2017-09-27 10:30:36)
>> We should check dependent state setup by enable path to run disable path
>> and not depend on the user parameters. i915_guc_submission_disable now
>> checks if execbuf client is setup and then goes ahead with disabling.
>>
>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
Thanks for the review Chris. I have this change as part of latest patch 
- https://patchwork.freedesktop.org/patch/179405/
Could you please review this patch.

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

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

[Sagar Arun Kamble]

On 9/28/2017 5:10 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2017-09-27 10:30:32)
>> @@ -4607,13 +4611,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);
> Seconded Michal's suggestion, otherwise it looks very clean.
> -Chris
This has been updated in the following patch in the latest revision of 
series:
https://patchwork.freedesktop.org/patch/179403/

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

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

[Sagar Arun Kamble]

On 9/28/2017 5:07 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2017-09-27 10:30:31)
>> These changes are preparation to handle GuC suspend/resume. Prepared
>> helper i915_gem_runtime_resume to reinitialize suspended 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)
>>
>> v3: Not returning status from gem_runtime_resume. (Chris)
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> 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>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c | 22 +++++++++++++---------
>>   drivers/gpu/drm/i915/i915_drv.h |  5 +++--
>>   drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++++--
>>   3 files changed, 32 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 7056bb2..d0a710d 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");
> I am still uneasy about this. Later on in the resume we try to reinit
> the hw under the assumption that this earlier step succeeded.
>
> Previously we have tried to make sure that if GEM fails, we do not leave
> the display in an unusable state. Is that still true?
As part of gem_resume we are resuming GuC and if that fails we are 
declaring gem wedged.
Will that be okay or we ignore the GuC resume failure and go ahead with 
reinit?
>>   
>>          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;
>> @@ -2596,13 +2605,6 @@ static int intel_runtime_resume(struct device *kdev)
>>                  ret = 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).
>> -        */
> This comment shouldn't be attached to the following gem init operations
> as they cannot fail. If they could, we should be throwing a warning here
> as this may result in a change of swizzling/fencing state as seen by
> userspace and therefore graphical corruption.
> -Chris
Ok. Will remove this comment.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v10 9/9] drm/i915/guc: Fix GuC cleanup in unload path

[Sagar Arun Kamble]

On 9/28/2017 6:45 PM, Sagar Arun Kamble wrote:
>
>
> On 9/28/2017 5:25 PM, Chris Wilson wrote:
>> Quoting Sagar Arun Kamble (2017-09-27 10:30:39)
>>> -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);
>> My preference would be for if (!guc->stage_desc_pool) return; inside
>> i915_guc_submission_cleanup().
>> -Chris
> Yes. I have taken similar input in the latest patch - 
> https://patchwork.freedesktop.org/patch/179405/
> In i915_guc_submission_cleanup we can cover destroy of stage_ids and 
> stage_desc_pool based on check you have suggested.
>  guc_ads_destroy is always required data so should we link with 
> stage_desc_pool check?
> intel_guc_log is optional so destroy need to be made dependent on 
> guc->log.vma
Realized that vma need not be checked so the check as you suggested 
looks more subtle here.

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