[PATCH v12 00/11] GEM/GuC Suspend/Resume/Reset fixes and restructuring

[PATCH v12 00/11] 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.

v11: Created separate patch for i915_gem_restore_fences change.
Added changes to disable/destroy GuC without dependency on
enable_guc_loading and enable_guc_submission. Rebased other patches.

v12: Fixed issue in the patch 9 where intel_guc_resume was called from
intel_uc_resume. Should be intel_uc_runtime_resume. CI BAT flagged the
issue. Updated comments for intel_uc_resume internals and comments in
patch 4.

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>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Sagar Arun Kamble (11):
  drm/i915: Create GEM runtime resume helper and handle GEM
    suspend/resume errors
  drm/i915: Update GEM suspend/resume flows with GuC suspend/resume
    functions
  drm/i915: Move i915_gem_restore_fences to i915_gem_resume
  drm/i915: Create uC runtime and system suspend/resume helpers
  drm/i915/guc: Introduce intel_uc_sanitize
  drm/i915/guc: Make GuC related disable/destroy functions not depend on
    i915.enable_guc_submission
  drm/i915/guc: Update i915.enable_guc_loading check in intel_uc_fini_hw
  drm/i915/guc: Update GuC ggtt.invalidate/interrupts/communication
    across RPM suspend/resume
  drm/i915/guc: Update GuC functionality in
    intel_uc_suspend/intel_uc_resume
  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            |  50 +++++++-----
 drivers/gpu/drm/i915/i915_drv.h            |   5 +-
 drivers/gpu/drm/i915/i915_gem.c            |  46 +++++++++--
 drivers/gpu/drm/i915/i915_guc_submission.c |  29 +++----
 drivers/gpu/drm/i915/i915_irq.c            |   6 ++
 drivers/gpu/drm/i915/i915_suspend.c        |   2 -
 drivers/gpu/drm/i915/intel_guc_log.c       |   6 +-
 drivers/gpu/drm/i915/intel_uc.c            | 124 ++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_uc.h            |  10 ++-
 drivers/gpu/drm/i915/intel_uncore.c        |   3 +
 10 files changed, 216 insertions(+), 65 deletions(-)

-- 
1.9.1

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

[PATCH v12 01/11] 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)

v4: Refined return from i915_gem_runtime_suspend. (Michal Wajdeczko)

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 | 33 +++++++++++++++++++--------------
 drivers/gpu/drm/i915/i915_drv.h |  5 +++--
 drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++++--
 3 files changed, 38 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 7056bb2..502ae13 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,9 @@ 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)
+		goto err_gem_suspend;
 
 	intel_guc_suspend(dev_priv);
 
@@ -2513,11 +2518,7 @@ static int intel_runtime_suspend(struct device *kdev)
 
 	if (ret) {
 		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
-		intel_runtime_pm_enable_interrupts(dev_priv);
-
-		enable_rpm_wakeref_asserts(dev_priv);
-
-		return ret;
+		goto err_suspend_complete;
 	}
 
 	intel_uncore_suspend(dev_priv);
@@ -2560,6 +2561,15 @@ static int intel_runtime_suspend(struct device *kdev)
 
 	DRM_DEBUG_KMS("Device suspended\n");
 	return 0;
+
+err_suspend_complete:
+	intel_runtime_pm_enable_interrupts(dev_priv);
+	intel_guc_resume(dev_priv);
+	i915_gem_runtime_resume(dev_priv);
+
+err_gem_suspend:
+	enable_rpm_wakeref_asserts(dev_priv);
+	return ret;
 }
 
 static int intel_runtime_resume(struct device *kdev)
@@ -2596,13 +2606,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 +2618,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 v12 02/11] drm/i915: Update GEM suspend/resume flows with GuC suspend/resume functions

[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.

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

v3: Rebase. Removed i915_gem_restore_fences change from this patch.
(Michal Wajdeczko)

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 | 9 +++++++--
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 502ae13..a9821ae 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);
@@ -2502,8 +2500,6 @@ static int intel_runtime_suspend(struct device *kdev)
 	if (ret)
 		goto err_gem_suspend;
 
-	intel_guc_suspend(dev_priv);
-
 	intel_runtime_pm_disable_interrupts(dev_priv);
 
 	ret = 0;
@@ -2564,7 +2560,6 @@ static int intel_runtime_suspend(struct device *kdev)
 
 err_suspend_complete:
 	intel_runtime_pm_enable_interrupts(dev_priv);
-	intel_guc_resume(dev_priv);
 	i915_gem_runtime_resume(dev_priv);
 
 err_gem_suspend:
@@ -2592,8 +2587,6 @@ static int intel_runtime_resume(struct device *kdev)
 	if (intel_uncore_unclaimed_mmio(dev_priv))
 		DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
 
-	intel_guc_resume(dev_priv);
-
 	if (IS_GEN9_LP(dev_priv)) {
 		bxt_disable_dc9(dev_priv);
 		bxt_display_core_init(dev_priv, true);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 59a88f2..932ac22 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,
@@ -4614,6 +4618,7 @@ int i915_gem_resume(struct drm_i915_private *dev_priv)
 	 */
 	dev_priv->gt.resume(dev_priv);
 
+	intel_guc_resume(dev_priv);
 	mutex_unlock(&dev->struct_mutex);
 
 	return 0;
-- 
1.9.1

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

[PATCH v12 03/11] drm/i915: Move i915_gem_restore_fences to i915_gem_resume

[Sagar Arun Kamble]

i915_gem_restore_fences is GEM resumption task hence it is moved to
i915_gem_resume from i915_restore_state.

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_gem.c     | 1 +
 drivers/gpu/drm/i915/i915_suspend.c | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 932ac22..889b35f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4611,6 +4611,7 @@ 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
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 v12 04/11] 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.

v5: Updated return from i915_gem_runtime_suspend. Moved the comment
about GuC reload optimization to intel_uc_init_hw. (Michal Wajdeczko)
Updated comments as FIXME.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@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 | 12 ++++++++++++
 drivers/gpu/drm/i915/i915_gem.c | 24 +++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_uc.c | 27 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.h |  4 ++++
 4 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a9821ae..66701ee 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -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);
+
 	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 889b35f..1f61d9c 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
@@ -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);
 
@@ -4619,7 +4624,16 @@ int i915_gem_resume(struct drm_i915_private *dev_priv)
 	 */
 	dev_priv->gt.resume(dev_priv);
 
-	intel_guc_resume(dev_priv);
+	/*
+	 * FIXME: At the end of suspend, Full GPU reset is done which unloads
+	 * the GuC firmware. If reset is avoided there, we can check the WOPCM
+	 * status here to see if GuC is still loaded and just do GuC resume
+	 * without reloading the firmware back.
+	 */
+	ret = intel_uc_resume(dev_priv);
+	if (ret)
+		i915_gem_set_wedged(dev_priv);
+
 	mutex_unlock(&dev->struct_mutex);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 2774778..80251ec 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -373,6 +373,13 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 			goto err_guc;
 	}
 
+	/*
+	 * FIXME: Currently, GuC is loaded unconditionally without checking
+	 * if it is already available in WOPCM (generally available after resume
+	 * from sleep state). We can skip the load based on WOPCM status (To be
+	 * decided based on bit 0 of GUC_WOPCM_SIZE).
+	 */
+
 	/* init WOPCM */
 	I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(dev_priv));
 	I915_WRITE(DMA_GUC_WOPCM_OFFSET,
@@ -481,6 +488,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 v12 05/11] drm/i915/guc: Introduce intel_uc_sanitize

[Sagar Arun Kamble]

Currently GPU is reset at the end of suspend via i915_gem_sanitize.
On resume, GuC will not be loaded until intel_uc_init_hw happens
during GEM resume flow but action to exit sleep 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.
load_status indicates HW state and it is sanitized through this new
function intel_uc_sanitize.

v2: Rebase.

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

v4: Reinstated the uC function intel_uc_sanitize. (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>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_uc.c     | 12 ++++++++++++
 drivers/gpu/drm/i915/intel_uc.h     |  1 +
 drivers/gpu/drm/i915/intel_uncore.c |  3 +++
 3 files changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 80251ec..ab26232 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -508,6 +508,18 @@ int intel_uc_resume(struct drm_i915_private *dev_priv)
 	return intel_guc_resume(dev_priv);
 }
 
+void intel_uc_sanitize(struct drm_i915_private *dev_priv)
+{
+	/*
+	 * 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 update.
+	 */
+	dev_priv->guc.fw.load_status = INTEL_UC_FIRMWARE_NONE;
+}
+
 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..ce3cea5 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);
+void intel_uc_sanitize(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);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index b3c3f94..acab013 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1763,6 +1763,9 @@ int intel_gpu_reset(struct drm_i915_private *dev_priv, unsigned engine_mask)
 	}
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 
+	if (engine_mask == ALL_ENGINES)
+		intel_uc_sanitize(dev_priv);
+
 	return ret;
 }
 
-- 
1.9.1

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

[PATCH v12 06/11] drm/i915/guc: Make GuC related disable/destroy functions not depend on i915.enable_guc_submission

[Sagar Arun Kamble]

During GuC load/enable, state is setup by driver that can be looked at
while disabling. So remove the check for i915.enable_guc_submission
parameter in those functions.

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 | 13 ++++++++++---
 drivers/gpu/drm/i915/i915_irq.c            |  6 ++++++
 drivers/gpu/drm/i915/intel_guc_log.c       |  6 ++----
 drivers/gpu/drm/i915/intel_uc.c            | 12 ++++--------
 4 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 04f1281..a2f67ca 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1002,7 +1002,8 @@ static int guc_ads_create(struct intel_guc *guc)
 
 static void guc_ads_destroy(struct intel_guc *guc)
 {
-	i915_vma_unpin_and_release(&guc->ads_vma);
+	if (guc->ads_vma)
+		i915_vma_unpin_and_release(&guc->ads_vma);
 }
 
 /*
@@ -1060,11 +1061,14 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
 
+	WARN_ON_ONCE(!ida_is_empty(&guc->stage_ids));
 	ida_destroy(&guc->stage_ids);
 	guc_ads_destroy(guc);
 	intel_guc_log_destroy(guc);
-	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
-	i915_vma_unpin_and_release(&guc->stage_desc_pool);
+	if (guc->stage_desc_pool) {
+		i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
+		i915_vma_unpin_and_release(&guc->stage_desc_pool);
+	}
 }
 
 static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
@@ -1204,6 +1208,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/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 0b756213..75406ee 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -455,6 +455,9 @@ void gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv)
 
 void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv)
 {
+	if (READ_ONCE(dev_priv->guc.interrupts_enabled))
+		return;
+
 	spin_lock_irq(&dev_priv->irq_lock);
 	if (!dev_priv->guc.interrupts_enabled) {
 		WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
@@ -467,6 +470,9 @@ void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv)
 
 void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv)
 {
+	if (!READ_ONCE(dev_priv->guc.interrupts_enabled))
+		return;
+
 	spin_lock_irq(&dev_priv->irq_lock);
 	dev_priv->guc.interrupts_enabled = false;
 
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 6571d96..73333b6 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -584,7 +584,8 @@ int intel_guc_log_create(struct intel_guc *guc)
 void intel_guc_log_destroy(struct intel_guc *guc)
 {
 	guc_log_runtime_destroy(guc);
-	i915_vma_unpin_and_release(&guc->log.vma);
+	if (guc->log.vma)
+		i915_vma_unpin_and_release(&guc->log.vma);
 }
 
 int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
@@ -653,9 +654,6 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv)
 
 void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 {
-	if (!i915_modparams.enable_guc_submission)
-		return;
-
 	mutex_lock(&dev_priv->drm.struct_mutex);
 	/* GuC logging is currently the only user of Guc2Host interrupts */
 	gen9_disable_guc_interrupts(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index ab26232..ea7c39c 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -445,8 +445,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 err_log_capture:
 	guc_capture_load_err_log(guc);
 err_submission:
-	if (i915_modparams.enable_guc_submission)
-		i915_guc_submission_fini(dev_priv);
+	i915_guc_submission_fini(dev_priv);
 err_guc:
 	i915_ggtt_disable_guc(dev_priv);
 
@@ -475,15 +474,12 @@ 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);
 
-	if (i915_modparams.enable_guc_submission) {
-		gen9_disable_guc_interrupts(dev_priv);
-		i915_guc_submission_fini(dev_priv);
-	}
+	gen9_disable_guc_interrupts(dev_priv);
+	i915_guc_submission_fini(dev_priv);
 
 	i915_ggtt_disable_guc(dev_priv);
 }
-- 
1.9.1

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

[PATCH v12 07/11] drm/i915/guc: Update i915.enable_guc_loading check in intel_uc_fini_hw

[Sagar Arun Kamble]

With most of the GuC disabling now separated from enable_guc_submission
parameter, only function that needs GuC parameter check is
i915_disable_guc_ggtt as that is enabled based on GuC kernel parameters.
Hence i915_disable_guc_ggtt is being called when enable_guc_loading is
set.

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/intel_uc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index ea7c39c..b900e95 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -471,9 +471,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 {
 	guc_free_load_err_log(&dev_priv->guc);
 
-	if (!i915_modparams.enable_guc_loading)
-		return;
-
 	i915_guc_submission_disable(dev_priv);
 
 	guc_disable_communication(&dev_priv->guc);
@@ -481,7 +478,8 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 	gen9_disable_guc_interrupts(dev_priv);
 	i915_guc_submission_fini(dev_priv);
 
-	i915_ggtt_disable_guc(dev_priv);
+	if (i915_modparams.enable_guc_loading)
+		i915_ggtt_disable_guc(dev_priv);
 }
 
 int intel_uc_runtime_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 v12 08/11] 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.

v5: Removed enable_guc_loading based check from intel_uc_runtime_suspend
and intel_uc_runtime_resume and pulled FW load_status based checks from
intel_guc_suspend/resume into these functions. (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_guc_submission.c | 11 ------
 drivers/gpu/drm/i915/intel_uc.c            | 60 +++++++++++++++++++++++++++++-
 2 files changed, 58 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index a2f67ca..ca6c4f9 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1230,11 +1230,6 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
 	struct i915_gem_context *ctx;
 	u32 data[3];
 
-	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;
@@ -1256,12 +1251,6 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
 	struct i915_gem_context *ctx;
 	u32 data[3];
 
-	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 b900e95..c54b302 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -482,14 +482,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 (dev_priv->guc.fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
+		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 (dev_priv->guc.fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
+		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 v12 09/11] drm/i915/guc: Update GuC functionality in intel_uc_suspend/intel_uc_resume

[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. We have added resume functionality but currently it will
not be triggered as GuC is reset at the end of suspend. So we depend
on intel_uc_resume post intel_uc_init_hw in i915_drm_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)

v6: Rebase with enable_guc_submission related change done in earlier
newly introduced patches.

v7: Fixed intel_uc_resume to call intel_uc_runtime_resume and added
comment about need to enable submission later if needed. Commit message
updated. (Sagar)

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            | 18 ++++++++++++++----
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index ca6c4f9..ab1c382 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 c54b302..feb149e 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -471,8 +471,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 {
 	guc_free_load_err_log(&dev_priv->guc);
 
-	i915_guc_submission_disable(dev_priv);
-
 	guc_disable_communication(&dev_priv->guc);
 
 	gen9_disable_guc_interrupts(dev_priv);
@@ -550,12 +548,24 @@ 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)
 {
-	return intel_guc_resume(dev_priv);
+	/*
+	 * FIXME: intel_uc_suspend disables GuC submission. This is
+	 * needed for unload path as well. Also, GuC is reset at the end
+	 * of suspend which makes disabling submission post that not
+	 * possible. Hence we re-enable submission during intel_uc_init_hw.
+	 * Once reset at the end of suspend is removed we need to enable
+	 * GuC submission post resuming GuC.
+	 */
+	return intel_uc_runtime_resume(dev_priv);
 }
 
 void intel_uc_sanitize(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 v12 10/11] 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)

v5: Rebase.

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 1f61d9c..87351d8 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 feb149e..bb0ab1e 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -580,6 +580,13 @@ void intel_uc_sanitize(struct drm_i915_private *dev_priv)
 	dev_priv->guc.fw.load_status = INTEL_UC_FIRMWARE_NONE;
 }
 
+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 ce3cea5..330eb08 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);
 void intel_uc_sanitize(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);
-- 
1.9.1

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

[PATCH v12 11/11] 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)

v5. Rebase.

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            | 12 +++---------
 drivers/gpu/drm/i915/intel_uc.h            |  4 ++--
 4 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 66701ee..b436503 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 ab1c382..849f73e 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1054,7 +1054,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 bb0ab1e..7eb98a3 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -445,7 +445,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 err_log_capture:
 	guc_capture_load_err_log(guc);
 err_submission:
-	i915_guc_submission_fini(dev_priv);
+	i915_guc_submission_cleanup(dev_priv);
 err_guc:
 	i915_ggtt_disable_guc(dev_priv);
 
@@ -467,17 +467,11 @@ 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);
 
-	guc_disable_communication(&dev_priv->guc);
-
-	gen9_disable_guc_interrupts(dev_priv);
-	i915_guc_submission_fini(dev_priv);
-
-	if (i915_modparams.enable_guc_loading)
-		i915_ggtt_disable_guc(dev_priv);
+	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 330eb08..5db332d 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -207,7 +207,7 @@ struct intel_huc {
 void intel_uc_init_fw(struct drm_i915_private *dev_priv);
 void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
 int intel_uc_init_hw(struct drm_i915_private *dev_priv);
-void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
+void intel_uc_cleanup(struct drm_i915_private *dev_priv);
 int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv);
 int intel_uc_runtime_resume(struct drm_i915_private *dev_priv);
 int intel_uc_suspend(struct drm_i915_private *dev_priv);
@@ -240,7 +240,7 @@ static inline void intel_guc_notify(struct intel_guc *guc)
 int i915_guc_submission_init(struct drm_i915_private *dev_priv);
 int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
 void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
-void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
+void i915_guc_submission_cleanup(struct drm_i915_private *dev_priv);
 struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
 
 /* intel_guc_log.c */
-- 
1.9.1

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

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

[Patchwork]

== Series Details ==

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

== Summary ==

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

Test chamelium:
        Subgroup hdmi-crc-fast:
                pass       -> DMESG-WARN (fi-skl-6700k)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                dmesg-fail -> DMESG-WARN (fi-cfl-s) fdo#102294
        Subgroup nonblocking-crc-pipe-c:
                incomplete -> SKIP       (fi-cfl-s)
Test drv_module_reload:
        Subgroup basic-reload-inject:
                pass       -> DMESG-WARN (fi-glk-1) fdo#102777

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

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:441s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:472s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:420s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:518s
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:507s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:494s
fi-cfl-s         total:289  pass:223  dwarn:34  dfail:0   fail:0   skip:32  time:534s
fi-cnl-y         total:289  pass:259  dwarn:0   dfail:0   fail:3   skip:27  time:651s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:414s
fi-glk-1         total:289  pass:259  dwarn:1   dfail:0   fail:0   skip:29  time:568s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:426s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:401s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:429s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:489s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:462s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:469s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:579s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:588s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:542s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:450s
fi-skl-6700k     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:747s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:496s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:479s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:566s
fi-snb-2600      total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:413s

c1ed501217782b35094792696924a1d22ee83c0f drm-tip: 2017y-09m-28d-04h-52m-47s UTC integration manifest
33793ea7acaf drm/i915/guc: Fix GuC cleanup in unload path
d0d5fde9d1de drm/i915/guc: Disable GuC submission and suspend it prior to i915 reset
19b890859702 drm/i915/guc: Update GuC functionality in intel_uc_suspend/intel_uc_resume
afc823451d74 drm/i915/guc: Update GuC ggtt.invalidate/interrupts/communication across RPM suspend/resume
618c9c6ed8a3 drm/i915/guc: Update i915.enable_guc_loading check in intel_uc_fini_hw
4397c4f2e4cb drm/i915/guc: Make GuC related disable/destroy functions not depend on i915.enable_guc_submission
565b5d5937f0 drm/i915/guc: Introduce intel_uc_sanitize
54471d03302b drm/i915: Create uC runtime and system suspend/resume helpers
df5c5562d597 drm/i915: Move i915_gem_restore_fences to i915_gem_resume
32506e3eaec1 drm/i915: Update GEM suspend/resume flows with GuC suspend/resume functions
56ce39172960 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_5843/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: warning for GEM/GuC Suspend/Resume/Reset fixes and restructuring (rev5)

[Sagar Arun Kamble]

On 9/28/2017 12:41 PM, Patchwork wrote:
> == Series Details ==
>
> Series: GEM/GuC Suspend/Resume/Reset fixes and restructuring (rev5)
> URL   : https://patchwork.freedesktop.org/series/30802/
> State : warning
>
> == Summary ==
>
> Series 30802v5 GEM/GuC Suspend/Resume/Reset fixes and restructuring
> https://patchwork.freedesktop.org/api/1.0/series/30802/revisions/5/mbox/
>
> Test chamelium:
>          Subgroup hdmi-crc-fast:
>                  pass       -> DMESG-WARN (fi-skl-6700k)
This looks sporadic. This test passed in trybot with same patchset. 
Filed below bug:
https://bugs.freedesktop.org/show_bug.cgi?id=103019
> Test kms_pipe_crc_basic:
>          Subgroup nonblocking-crc-pipe-b-frame-sequence:
>                  dmesg-fail -> DMESG-WARN (fi-cfl-s) fdo#102294
>          Subgroup nonblocking-crc-pipe-c:
>                  incomplete -> SKIP       (fi-cfl-s)
> Test drv_module_reload:
>          Subgroup basic-reload-inject:
>                  pass       -> DMESG-WARN (fi-glk-1) fdo#102777
>
> fdo#102294 https://bugs.freedesktop.org/show_bug.cgi?id=102294
> fdo#102777 https://bugs.freedesktop.org/show_bug.cgi?id=102777
>
> fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:441s
> fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:472s
> fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:420s
> fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:518s
> 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:507s
> fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:494s
> fi-cfl-s         total:289  pass:223  dwarn:34  dfail:0   fail:0   skip:32  time:534s
> fi-cnl-y         total:289  pass:259  dwarn:0   dfail:0   fail:3   skip:27  time:651s
> fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:414s
> fi-glk-1         total:289  pass:259  dwarn:1   dfail:0   fail:0   skip:29  time:568s
> fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:426s
> fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:401s
> fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:429s
> fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:489s
> fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:462s
> fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:469s
> fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:579s
> fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:588s
> fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:542s
> fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:450s
> fi-skl-6700k     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:747s
> fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:496s
> fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:479s
> fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:566s
> fi-snb-2600      total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:413s
>
> c1ed501217782b35094792696924a1d22ee83c0f drm-tip: 2017y-09m-28d-04h-52m-47s UTC integration manifest
> 33793ea7acaf drm/i915/guc: Fix GuC cleanup in unload path
> d0d5fde9d1de drm/i915/guc: Disable GuC submission and suspend it prior to i915 reset
> 19b890859702 drm/i915/guc: Update GuC functionality in intel_uc_suspend/intel_uc_resume
> afc823451d74 drm/i915/guc: Update GuC ggtt.invalidate/interrupts/communication across RPM suspend/resume
> 618c9c6ed8a3 drm/i915/guc: Update i915.enable_guc_loading check in intel_uc_fini_hw
> 4397c4f2e4cb drm/i915/guc: Make GuC related disable/destroy functions not depend on i915.enable_guc_submission
> 565b5d5937f0 drm/i915/guc: Introduce intel_uc_sanitize
> 54471d03302b drm/i915: Create uC runtime and system suspend/resume helpers
> df5c5562d597 drm/i915: Move i915_gem_restore_fences to i915_gem_resume
> 32506e3eaec1 drm/i915: Update GEM suspend/resume flows with GuC suspend/resume functions
> 56ce39172960 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_5843/

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

Re: [PATCH v12 01/11] drm/i915: Create GEM runtime resume helper and handle GEM suspend/resume errors

[Joonas Lahtinen]

On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble 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)
> 
> v4: Refined return from i915_gem_runtime_suspend. (Michal Wajdeczko)
> 
> 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>

<SNIP>

> @@ -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");

Not DRM_ERROR like other paths?

Also, this might be worth a WARN(). I'm open for discussion.

> @@ -2560,6 +2561,15 @@ static int intel_runtime_suspend(struct device *kdev)
>  
>  	DRM_DEBUG_KMS("Device suspended\n");
>  	return 0;
> +
> +err_suspend_complete:

err_disable:

> +	intel_runtime_pm_enable_interrupts(dev_priv);
> +	intel_guc_resume(dev_priv);
> +	i915_gem_runtime_resume(dev_priv);
> +
> +err_gem_suspend:

err_gem:

We are in "suspend" function, after all.

> +	enable_rpm_wakeref_asserts(dev_priv);
> +	return ret;
>  }

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v12 02/11] drm/i915: Update GEM suspend/resume flows with GuC suspend/resume functions

[Joonas Lahtinen]

On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote:
> This patch moves GuC suspend/resume handlers to corresponding GEM handlers
> and orders them properly in the runtime and system suspend/resume flows.
> 
> v2: Removed documentation of suspend/resume handlers as those are not
> interfaces and are just hooks. (Chris)
> 
> v3: Rebase. Removed i915_gem_restore_fences change from this patch.
> (Michal Wajdeczko)
> 
> 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>

This is the kind of cleanup we need.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v12 03/11] drm/i915: Move i915_gem_restore_fences to i915_gem_resume

[Joonas Lahtinen]

On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote:
> i915_gem_restore_fences is GEM resumption task hence it is moved to
> i915_gem_resume from i915_restore_state.

+ Chris

Didn't I just review this patch elsewhere? Other thread explains that
the emytology of fixing display checks out, what you suggest is more
appropriate. Question is if we should note with a comment?

PS. You seem to have sent this with --suppress-cc, which decreases the
chances of getting the reviews :)

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v12 01/11] drm/i915: Create GEM runtime resume helper and handle GEM suspend/resume errors

[Chris Wilson]

Quoting Joonas Lahtinen (2017-09-29 12:43:48)
> On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble 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)
> > 
> > v4: Refined return from i915_gem_runtime_suspend. (Michal Wajdeczko)
> > 
> > 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>
> 
> <SNIP>
> 
> > @@ -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");
> 
> Not DRM_ERROR like other paths?

Bah, we really need to migrate to dev_err() like all the other cool
drivers.

DRM_DEV_ERROR()

The uppercase is an eyesore, but for an error message, acceptable.
Similarly any user output like DRM_DEV_INFO() could reasonably be shouty
so that it is not missed. So maybe the uppercase is always justified.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v12 05/11] drm/i915/guc: Introduce intel_uc_sanitize

[Joonas Lahtinen]

On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble 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.
> load_status indicates HW state and it is sanitized through this new
> function intel_uc_sanitize.
> 
> v2: Rebase.
> 
> v3: Removed intel_guc_sanitize. Marking load status as NONE at the
> GPU reset point. (Chris/Michal)
> 
> v4: Reinstated the uC function intel_uc_sanitize. (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>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> @@ -508,6 +508,18 @@ int intel_uc_resume(struct drm_i915_private *dev_priv)
>  	return intel_guc_resume(dev_priv);
>  }
>  
> +void intel_uc_sanitize(struct drm_i915_private *dev_priv)
> +{
> +	/*
> +	 * 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 update.
> +	 */
> +	dev_priv->guc.fw.load_status = INTEL_UC_FIRMWARE_NONE;

With what I suggested to Michal, this would be call to
intel_guc_sanitize() (and in future also intel_huc_sanitize()
intel_dmc_sanitize()).

> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1763,6 +1763,9 @@ int intel_gpu_reset(struct drm_i915_private *dev_priv, unsigned engine_mask)
>  	}
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>  
> +	if (engine_mask == ALL_ENGINES)
> +		intel_uc_sanitize(dev_priv);

We could propagate engine_mask to intel_uc_sanitize and let it decide
what it does to keep a clear top level code flow. This also doesn't
seem to depend on if GuC submission is enabled or not.

If we want to be unconditional, wouldn't intel_guc_select_fw would not
be more appropriate in intel_uc_sanitize?

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v12 04/11] drm/i915: Create uC runtime and system suspend/resume helpers

[Joonas Lahtinen]

+ Michal,

On the principle of code motion first, changes second, I'd like to see
the clean split-up from Michal before touching the files much. That way
 git history will be easier to examine.

Few comments below.

On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote:
> Prepared generic helpers intel_uc_suspend, intel_uc_resume,
> intel_uc_runtime_suspend, intel_uc_runtime_resume. These are
> called from respective GEM functions.
> Only exception is intel_uc_resume that needs to be called
> w/ or w/o GuC loaded in i915_drm_resume path. Changes to
> add WOPCM condition check to load GuC during resume will be added
> in later patches.
> 
> v2: Rebase w.r.t removal of GuC code restructuring.
> 
> v3: Calling intel_uc_resume from i915_gem_resume post resuming
> i915 gem setup. This is symmetrical with i915_gem_suspend.
> Removed error messages from i915 suspend/resume routines as
> uC suspend/resume routines will have those. (Michal Wajdeczko)
> Declare wedged on uc_suspend failure and uc_resume failure.
> (Michał Winiarski)
> Keeping the uC suspend/resume function definitions close to other
> uC functions.
> 
> v4: Added implementation to intel_uc_resume as GuC resume is
> needed to be triggered post reloading the firmware as well. Added
> comments about semantics of GuC resume with the firmware reload.
> 
> v5: Updated return from i915_gem_runtime_suspend. Moved the comment
> about GuC reload optimization to intel_uc_init_hw. (Michal Wajdeczko)
> Updated comments as FIXME.
> 
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>

<SNIP>

> @@ -1698,6 +1698,18 @@ static int i915_drm_resume(struct drm_device *dev)
>  	}
>  	mutex_unlock(&dev->struct_mutex);
>  
> +	/*
> +	 * FIXME: Currently we know that at the end of suspend we have done Full

I think "beginning of resume" is the one that bites us more.

> +	 * GPU reset and GuC is loaded again during i915_gem_init_hw.
> +	 * Now, send action to GuC to resume back again as earlier call to
> +	 * intel_uc_resume from i915_gem_resume would have done nothing.
> +	 * This needs to be skipped if GuC was not loaded during resume as
> +	 * intel_uc_resume would have been already called from i915_gem_resume.
> +	 */
> +	ret = intel_uc_resume(dev_priv);
> +	if (ret)
> +		i915_gem_set_wedged(dev_priv);

I'm very sure we want to bring the system up with a backup of ELSP only
submission *even* if we used GuC when going to sleep.

> @@ -4571,7 +4573,9 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>  	if (WARN_ON(!intel_engines_are_idle(dev_priv)))
>  		i915_gem_set_wedged(dev_priv); /* no hope, discard everything */
>  
> -	intel_guc_suspend(dev_priv);
> +	ret = intel_uc_suspend(dev_priv);
> +	if (ret)
> +		i915_gem_set_wedged(dev_priv); /* no hope, discard everything */

Isn't that an exxaragation when we still may have functional ELSP?

It may not be a bad idea to first bring up the ELSP submission and when
GuC has loaded, switch over. To get rid of the latency.

> @@ -4619,7 +4624,16 @@ int i915_gem_resume(struct drm_i915_private *dev_priv)
>  	 */
>  	dev_priv->gt.resume(dev_priv);
>  
> -	intel_guc_resume(dev_priv);
> +	/*
> +	 * FIXME: At the end of suspend, Full GPU reset is done which unloads
> +	 * the GuC firmware. If reset is avoided there, we can check the WOPCM
> +	 * status here to see if GuC is still loaded and just do GuC resume
> +	 * without reloading the firmware back.
> +	 */

Again, we're resetting at both directions, going to suspend and on
resume too. I'd classify these bot as more of a TODOs, because we only
get some latency that can be improved on, no broken behaviour to fix.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v12 06/11] drm/i915/guc: Make GuC related disable/destroy functions not depend on i915.enable_guc_submission

[Joonas Lahtinen]

On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote:
> During GuC load/enable, state is setup by driver that can be looked at
> while disabling. So remove the check for i915.enable_guc_submission
> parameter in those functions.
> 
> 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>

<SNIP>

> @@ -1002,7 +1002,8 @@ static int guc_ads_create(struct intel_guc *guc)
>  
>  static void guc_ads_destroy(struct intel_guc *guc)
>  {
> -	i915_vma_unpin_and_release(&guc->ads_vma);
> +	if (guc->ads_vma)

GEM_BUG_ON(!guc->ads_vma);

> +		i915_vma_unpin_and_release(&guc->ads_vma);
>  }
>  
>  /*
> @@ -1060,11 +1061,14 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc *guc = &dev_priv->guc;
>  
> +	WARN_ON_ONCE(!ida_is_empty(&guc->stage_ids));
>  	ida_destroy(&guc->stage_ids);
>  	guc_ads_destroy(guc);
>  	intel_guc_log_destroy(guc);
> -	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
> -	i915_vma_unpin_and_release(&guc->stage_desc_pool);
> +	if (guc->stage_desc_pool) {

GEM_BUG_ON(!guc->stage_desc_pol) is the right thing.

> +		i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
> +		i915_vma_unpin_and_release(&guc->stage_desc_pool);
> +	}

I'm generally against conditional teardown. If the _init did not fully
succeed, the _fini is never supposed to be called.

>  static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
> @@ -1204,6 +1208,9 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc *guc = &dev_priv->guc;
>  

We may want document pre-requirements assert_lockdep_held() in
enable/disable submission funcs, for a good measure. Then it'll be
easier to convert away from struct_mutex when the time comes.

> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 6571d96..73333b6 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -584,7 +584,8 @@ int intel_guc_log_create(struct intel_guc *guc)
>  void intel_guc_log_destroy(struct intel_guc *guc)
>  {
>  	guc_log_runtime_destroy(guc);
> -	i915_vma_unpin_and_release(&guc->log.vma);
> +	if (guc->log.vma)
> +		i915_vma_unpin_and_release(&guc->log.vma);

Again, GEM_BUG_ON(!guc->log.vma); 

> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -445,8 +445,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  err_log_capture:
>  	guc_capture_load_err_log(guc);
>  err_submission:
> -	if (i915_modparams.enable_guc_submission)
> -		i915_guc_submission_fini(dev_priv);
> +	i915_guc_submission_fini(dev_priv);

No, no unconditional calling of _fini if the _init is not uncoditional
too. You can drop both checks down to the submission_init/_fini funcs
if you want to. For me it's more clear if they're here.

Inside the funcs or right before calling them, when called just from
one place (like I'd prefer here), but most importantly it has to be
symmetric.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v12 07/11] drm/i915/guc: Update i915.enable_guc_loading check in intel_uc_fini_hw

[Joonas Lahtinen]

On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote:
> With most of the GuC disabling now separated from enable_guc_submission
> parameter, only function that needs GuC parameter check is
> i915_disable_guc_ggtt as that is enabled based on GuC kernel parameters.
> Hence i915_disable_guc_ggtt is being called when enable_guc_loading is
> set.
> 
> 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>

This already depends on the previus changes, so I'm waiting for a
re-roll of the series after discussion on the previous patches.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v12 01/11] drm/i915: Create GEM runtime resume helper and handle GEM suspend/resume errors

[Joonas Lahtinen]

On Fri, 2017-09-29 at 12:49 +0100, Chris Wilson wrote:
> Quoting Joonas Lahtinen (2017-09-29 12:43:48)
> > On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble 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)
> > > 
> > > v4: Refined return from i915_gem_runtime_suspend. (Michal Wajdeczko)
> > > 
> > > 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>
> > 
> > <SNIP>
> > 
> > > @@ -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");
> > 
> > Not DRM_ERROR like other paths?
> 
> Bah, we really need to migrate to dev_err() like all the other cool
> drivers.
> 
> DRM_DEV_ERROR()

+1 on that.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v12 01/11] drm/i915: Create GEM runtime resume helper and handle GEM suspend/resume errors

[Sagar Arun Kamble]

On 9/29/2017 6:46 PM, Joonas Lahtinen wrote:
> On Fri, 2017-09-29 at 12:49 +0100, Chris Wilson wrote:
>> Quoting Joonas Lahtinen (2017-09-29 12:43:48)
>>> On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble 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)
>>>>
>>>> v4: Refined return from i915_gem_runtime_suspend. (Michal Wajdeczko)
>>>>
>>>> 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>
>>> <SNIP>
>>>
>>>> @@ -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");
>>> Not DRM_ERROR like other paths?
>> Bah, we really need to migrate to dev_err() like all the other cool
>> drivers.
>>
>> DRM_DEV_ERROR()
> +1 on that.
>
> Regards, Joonas
Sure. Will update. Thanks for review.

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

Re: [PATCH v12 02/11] drm/i915: Update GEM suspend/resume flows with GuC suspend/resume functions

[Sagar Arun Kamble]

On 9/29/2017 5:15 PM, Joonas Lahtinen wrote:
> On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote:
>> This patch moves GuC suspend/resume handlers to corresponding GEM handlers
>> and orders them properly in the runtime and system suspend/resume flows.
>>
>> v2: Removed documentation of suspend/resume handlers as those are not
>> interfaces and are just hooks. (Chris)
>>
>> v3: Rebase. Removed i915_gem_restore_fences change from this patch.
>> (Michal Wajdeczko)
>>
>> 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>
> This is the kind of cleanup we need.
>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>
> Regards, Joonas
Thanks for review.

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

Re: [PATCH v12 03/11] drm/i915: Move i915_gem_restore_fences to i915_gem_resume

[Sagar Arun Kamble]

On 9/29/2017 5:18 PM, Joonas Lahtinen wrote:
> On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote:
>> i915_gem_restore_fences is GEM resumption task hence it is moved to
>> i915_gem_resume from i915_restore_state.
> + Chris
>
> Didn't I just review this patch elsewhere? Other thread explains that
> the emytology of fixing display checks out, what you suggest is more
> appropriate. Question is if we should note with a comment?
>
> PS. You seem to have sent this with --suppress-cc, which decreases the
> chances of getting the reviews :)
No. this was sent without "--suppress-cc". I actually floated two 
revisions yesterday so it might have been missed.
Sorry for the thrash.
How did you figure out "--suppress-cc"? Don't see in the headers
>
> Regards, Joonas

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

Re: [PATCH v12 03/11] drm/i915: Move i915_gem_restore_fences to i915_gem_resume

[Sagar Arun Kamble]

On 9/29/2017 7:29 PM, Sagar Arun Kamble wrote:
>
>
> On 9/29/2017 5:18 PM, Joonas Lahtinen wrote:
>> On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote:
>>> i915_gem_restore_fences is GEM resumption task hence it is moved to
>>> i915_gem_resume from i915_restore_state.
>> + Chris
>>
>> Didn't I just review this patch elsewhere? Other thread explains that
>> the emytology of fixing display checks out, what you suggest is more
>> appropriate. Question is if we should note with a comment?
Yes. I had pulled this patch out of this series today. Chris has taken 
that one for push. Thanks for review.
https://patchwork.freedesktop.org/series/31118/
>>
>> PS. You seem to have sent this with --suppress-cc, which decreases the
>> chances of getting the reviews :)
> No. this was sent without "--suppress-cc". I actually floated two 
> revisions yesterday so it might have been missed.
> Sorry for the thrash.
> How did you figure out "--suppress-cc"? Don't see in the headers
>>
>> Regards, Joonas
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v12 04/11] drm/i915: Create uC runtime and system suspend/resume helpers

[Sagar Arun Kamble]

On 9/29/2017 5:42 PM, Joonas Lahtinen wrote:
> + Michal,
>
> On the principle of code motion first, changes second, I'd like to see
> the clean split-up from Michal before touching the files much. That way
>   git history will be easier to examine.
I think we wanted to get these fixes in prior to restructuring as that 
might take time.
Currently this v12 series has some bit of restructuring included along 
with fixes which has increased patches.
I have separated that today with new series. Will get that in first and 
then follow with fixes series or fixes series based on Michal's split-up 
series.
This is the minor restructuring series that is required for fixes 
series: https://patchwork.freedesktop.org/series/31140/

Michal, please suggest.

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

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

Re: [PATCH v12 05/11] drm/i915/guc: Introduce intel_uc_sanitize

[Sagar Arun Kamble]

On 9/29/2017 5:30 PM, Joonas Lahtinen wrote:
> On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble 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.
>> load_status indicates HW state and it is sanitized through this new
>> function intel_uc_sanitize.
>>
>> v2: Rebase.
>>
>> v3: Removed intel_guc_sanitize. Marking load status as NONE at the
>> GPU reset point. (Chris/Michal)
>>
>> v4: Reinstated the uC function intel_uc_sanitize. (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>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> <SNIP>
>
>> @@ -508,6 +508,18 @@ int intel_uc_resume(struct drm_i915_private *dev_priv)
>>   	return intel_guc_resume(dev_priv);
>>   }
>>   
>> +void intel_uc_sanitize(struct drm_i915_private *dev_priv)
>> +{
>> +	/*
>> +	 * 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 update.
>> +	 */
>> +	dev_priv->guc.fw.load_status = INTEL_UC_FIRMWARE_NONE;
> With what I suggested to Michal, this would be call to
> intel_guc_sanitize() (and in future also intel_huc_sanitize()
> intel_dmc_sanitize()).
Yes.
>
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -1763,6 +1763,9 @@ int intel_gpu_reset(struct drm_i915_private *dev_priv, unsigned engine_mask)
>>   	}
>>   	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>>   
>> +	if (engine_mask == ALL_ENGINES)
>> +		intel_uc_sanitize(dev_priv);
> We could propagate engine_mask to intel_uc_sanitize and let it decide
> what it does to keep a clear top level code flow. This also doesn't
> seem to depend on if GuC submission is enabled or not.
Sure. will make this change.
> If we want to be unconditional, wouldn't intel_guc_select_fw would not
> be more appropriate in intel_uc_sanitize?
Do we want to select different fw across resets? That would mean 
changing i915.guc_firmware_path at runtime which I guess we don't want 
do right?
> Regards, Joonas

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

Re: [PATCH v12 04/11] drm/i915: Create uC runtime and system suspend/resume helpers

[Michal Wajdeczko]

On Fri, 29 Sep 2017 16:13:54 +0200, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

>
>
> On 9/29/2017 5:42 PM, Joonas Lahtinen wrote:
>> + Michal,
>>
>> On the principle of code motion first, changes second, I'd like to see
>> the clean split-up from Michal before touching the files much. That way
>>   git history will be easier to examine.
> I think we wanted to get these fixes in prior to restructuring as that  
> might take time.
> Currently this v12 series has some bit of restructuring included along  
> with fixes which has increased patches.
> I have separated that today with new series. Will get that in first and  
> then follow with fixes series or fixes series based on Michal's split-up  
> series.
> This is the minor restructuring series that is required for fixes  
> series: https://patchwork.freedesktop.org/series/31140/
>
> Michal, please suggest.

Joonas suggestion was to start with all code moves first, then apply your
fixes from your changes to preserve better git history. To speed up I've
already started new 'moves-only' series that should be ready by EOD.

Michal

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

Re: [PATCH v12 06/11] drm/i915/guc: Make GuC related disable/destroy functions not depend on i915.enable_guc_submission

[Sagar Arun Kamble]

On 9/29/2017 5:57 PM, Joonas Lahtinen wrote:
> On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote:
>> During GuC load/enable, state is setup by driver that can be looked at
>> while disabling. So remove the check for i915.enable_guc_submission
>> parameter in those functions.
>>
>> 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>
> <SNIP>
>
>> @@ -1002,7 +1002,8 @@ static int guc_ads_create(struct intel_guc *guc)
>>   
>>   static void guc_ads_destroy(struct intel_guc *guc)
>>   {
>> -	i915_vma_unpin_and_release(&guc->ads_vma);
>> +	if (guc->ads_vma)
> GEM_BUG_ON(!guc->ads_vma);
This check was unnecessary. Suggestion from Chris was to make these 
destroys be self-check based instead of invoking
based on module parameters like enable_guc_submission/loading. In my new 
patch I have removed these checks.
https://patchwork.freedesktop.org/patch/179683/
>
>> +		i915_vma_unpin_and_release(&guc->ads_vma);
>>   }
>>   
>>   /*
>> @@ -1060,11 +1061,14 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
>>   {
>>   	struct intel_guc *guc = &dev_priv->guc;
>>   
>> +	WARN_ON_ONCE(!ida_is_empty(&guc->stage_ids));
>>   	ida_destroy(&guc->stage_ids);
>>   	guc_ads_destroy(guc);
>>   	intel_guc_log_destroy(guc);
>> -	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
>> -	i915_vma_unpin_and_release(&guc->stage_desc_pool);
>> +	if (guc->stage_desc_pool) {
> GEM_BUG_ON(!guc->stage_desc_pol) is the right thing.
stage_desc_pool check is used to enter submission_fini in my latest 
patch. Other than that there are no more checks needed.
https://patchwork.freedesktop.org/patch/179683/
>
>> +		i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
>> +		i915_vma_unpin_and_release(&guc->stage_desc_pool);
>> +	}
> I'm generally against conditional teardown. If the _init did not fully
> succeed, the _fini is never supposed to be called.
Plan is to replace the module parameter based condition to driver state 
based condition.
>>   static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
>> @@ -1204,6 +1208,9 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
>>   {
>>   	struct intel_guc *guc = &dev_priv->guc;
>>   
> We may want document pre-requirements assert_lockdep_held() in
> enable/disable submission funcs, for a good measure. Then it'll be
> easier to convert away from struct_mutex when the time comes.
We removed lockdep assert as mutex is needed by internal functions which 
already have the asserts.
>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
>> index 6571d96..73333b6 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -584,7 +584,8 @@ int intel_guc_log_create(struct intel_guc *guc)
>>   void intel_guc_log_destroy(struct intel_guc *guc)
>>   {
>>   	guc_log_runtime_destroy(guc);
>> -	i915_vma_unpin_and_release(&guc->log.vma);
>> +	if (guc->log.vma)
>> +		i915_vma_unpin_and_release(&guc->log.vma);
> Again, GEM_BUG_ON(!guc->log.vma);
This is unnecessary check.
>
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -445,8 +445,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>>   err_log_capture:
>>   	guc_capture_load_err_log(guc);
>>   err_submission:
>> -	if (i915_modparams.enable_guc_submission)
>> -		i915_guc_submission_fini(dev_priv);
>> +	i915_guc_submission_fini(dev_priv);
> No, no unconditional calling of _fini if the _init is not uncoditional
> too. You can drop both checks down to the submission_init/_fini funcs
> if you want to. For me it's more clear if they're here.
>
> Inside the funcs or right before calling them, when called just from
> one place (like I'd prefer here), but most importantly it has to be
> symmetric.
>
> Regards, Joonas
_fini is still conditional, but inside the function based on various 
states set.

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

Re: [PATCH v12 03/11] drm/i915: Move i915_gem_restore_fences to i915_gem_resume

[Joonas Lahtinen]

On Fri, 2017-09-29 at 19:29 +0530, Sagar Arun Kamble wrote:
> 
> On 9/29/2017 5:18 PM, Joonas Lahtinen wrote:
> > On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote:
> > > i915_gem_restore_fences is GEM resumption task hence it is moved to
> > > i915_gem_resume from i915_restore_state.
> > 
> > + Chris
> > 
> > Didn't I just review this patch elsewhere? Other thread explains that
> > the emytology of fixing display checks out, what you suggest is more
> > appropriate. Question is if we should note with a comment?
> > 
> > PS. You seem to have sent this with --suppress-cc, which decreases the
> > chances of getting the reviews :)
> 
> No. this was sent without "--suppress-cc". I actually floated two 
> revisions yesterday so it might have been missed.
> Sorry for the thrash.
> How did you figure out "--suppress-cc"? Don't see in the headers

There were Cc: entries in the text, but as the patch reached me, there
were none.

Could be the mailing list acting weird, too.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v12 05/11] drm/i915/guc: Introduce intel_uc_sanitize

[Joonas Lahtinen]

On Fri, 2017-09-29 at 19:52 +0530, Sagar Arun Kamble wrote:
> 
> On 9/29/2017 5:30 PM, Joonas Lahtinen wrote:
> > On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble 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.
> > > load_status indicates HW state and it is sanitized through this new
> > > function intel_uc_sanitize.
> > > 
> > > v2: Rebase.
> > > 
> > > v3: Removed intel_guc_sanitize. Marking load status as NONE at the
> > > GPU reset point. (Chris/Michal)
> > > 
> > > v4: Reinstated the uC function intel_uc_sanitize. (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>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > <SNIP>
> > 
> > > @@ -508,6 +508,18 @@ int intel_uc_resume(struct drm_i915_private *dev_priv)
> > >   	return intel_guc_resume(dev_priv);
> > >   }
> > >   
> > > +void intel_uc_sanitize(struct drm_i915_private *dev_priv)
> > > +{
> > > +	/*
> > > +	 * 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 update.
> > > +	 */
> > > +	dev_priv->guc.fw.load_status = INTEL_UC_FIRMWARE_NONE;
> > 
> > With what I suggested to Michal, this would be call to
> > intel_guc_sanitize() (and in future also intel_huc_sanitize()
> > intel_dmc_sanitize()).
> 
> Yes.
> > 
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > @@ -1763,6 +1763,9 @@ int intel_gpu_reset(struct drm_i915_private *dev_priv, unsigned engine_mask)
> > >   	}
> > >   	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> > >   
> > > +	if (engine_mask == ALL_ENGINES)
> > > +		intel_uc_sanitize(dev_priv);
> > 
> > We could propagate engine_mask to intel_uc_sanitize and let it decide
> > what it does to keep a clear top level code flow. This also doesn't
> > seem to depend on if GuC submission is enabled or not.
> 
> Sure. will make this change.
> > If we want to be unconditional, wouldn't intel_guc_select_fw would not
> > be more appropriate in intel_uc_sanitize?
> 
> Do we want to select different fw across resets? That would mean 
> changing i915.guc_firmware_path at runtime which I guess we don't want 
> do right?

That's a good point, intel_uc_sanitize could call intel_guc_sanitize
and intel_guc_sanitize could be called at the beginning of
intel_guc_select_fw too. Quasi-randomly setting the guc firmware load
status was odd.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v12 06/11] drm/i915/guc: Make GuC related disable/destroy functions not depend on i915.enable_guc_submission

[Joonas Lahtinen]

On Sat, 2017-09-30 at 13:52 +0530, Sagar Arun Kamble wrote:
> 
> On 9/29/2017 5:57 PM, Joonas Lahtinen wrote:
> > On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote:
> > > During GuC load/enable, state is setup by driver that can be looked at
> > > while disabling. So remove the check for i915.enable_guc_submission
> > > parameter in those functions.
> > > 
> > > 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>
> > 
> > <SNIP>
> > 
> > > @@ -1002,7 +1002,8 @@ static int guc_ads_create(struct intel_guc *guc)
> > >   
> > >   static void guc_ads_destroy(struct intel_guc *guc)
> > >   {
> > > -	i915_vma_unpin_and_release(&guc->ads_vma);
> > > +	if (guc->ads_vma)
> > 
> > GEM_BUG_ON(!guc->ads_vma);
> 
> This check was unnecessary. Suggestion from Chris was to make these 
> destroys be self-check based instead of invoking
> based on module parameters like enable_guc_submission/loading. In my new 
> patch I have removed these checks.
> https://patchwork.freedesktop.org/patch/179683/

I pinged Chris about this. I do prefer the symmetry, and I think we
could have guc->enable_submission variable based on which
i915_guc_submission_init/fini() would be executed on. No conditionals
in the _init/_fini themselves. You can of course do the
enable_submission check at the beginning of i915_guc_submission_init
and _fini too, if that feels clearer for the upper level code flow.

If _init is called and succeeds, _fini needs to be called. If _init
fails, _fini is to never be called. The check outside or inside will
both adhere to that.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx