All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/9] GuC Fixes, Minor restructuring changes and v9+ logging change
@ 2017-09-20 17:38 Sagar Arun Kamble
  2017-09-20 17:38 ` [PATCH v4 1/9] drm/i915: Create uc runtime and system suspend/resume helpers Sagar Arun Kamble
                   ` (10 more replies)
  0 siblings, 11 replies; 29+ messages in thread
From: Sagar Arun Kamble @ 2017-09-20 17:38 UTC (permalink / raw)
  To: intel-gfx

This series is based on reviews from https://patchwork.freedesktop.org/series/30351/.
Due to changing priority and complexity of restructuring, this patch series has gone
through >5 revisions but would want to maintain the series w.r.t above base series.
W.r.t above series this is rev4. Older series can be found at
https://patchwork.freedesktop.org/series/30502/.

Sagar Arun Kamble (9):
  drm/i915: Create uc runtime and system suspend/resume helpers
  drm/i915/guc: Update prototype/name of GuC suspend/resume fns and move
    to intel_uc.c
  drm/i915/guc: Update GuC ggtt.invalidate/interrupts/communication
    across RPM suspend/resume
  drm/i915/guc: Update suspend functionality in intel_uc_suspend path
  drm/i915/guc: Disable GuC submission and suspend it prior to i915
    reset
  drm/i915/guc: Fix GuC cleanup in unload path
  drm/i915/guc: Remove i915_guc_log_unregister
  drm/i915/guc: Enable default/critical logging in GuC by default from
    GuC v9
  drm/i915: Reorganize HuC authentication

 drivers/gpu/drm/i915/i915_drv.c            |  26 +++-
 drivers/gpu/drm/i915/i915_gem.c            |   9 +-
 drivers/gpu/drm/i915/i915_guc_submission.c |  54 +-------
 drivers/gpu/drm/i915/intel_guc_fwif.h      |   4 +-
 drivers/gpu/drm/i915/intel_guc_log.c       |  25 ++--
 drivers/gpu/drm/i915/intel_huc.c           |  22 ++--
 drivers/gpu/drm/i915/intel_uc.c            | 195 ++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_uc.h            |  15 ++-
 8 files changed, 241 insertions(+), 109 deletions(-)

-- 
1.9.1

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v4 1/9] drm/i915: Create uc runtime and system suspend/resume helpers
  2017-09-20 17:38 [PATCH v4 0/9] GuC Fixes, Minor restructuring changes and v9+ logging change Sagar Arun Kamble
@ 2017-09-20 17:38 ` Sagar Arun Kamble
  2017-09-21 17:16   ` Michał Winiarski
  2017-09-20 17:38 ` [PATCH v4 2/9] drm/i915/guc: Update prototype/name of GuC suspend/resume fns and move to intel_uc.c Sagar Arun Kamble
                   ` (9 subsequent siblings)
  10 siblings, 1 reply; 29+ messages in thread
From: Sagar Arun Kamble @ 2017-09-20 17:38 UTC (permalink / raw)
  To: intel-gfx

Prepared generic helpers intel_uc_suspend, intel_uc_resume,
intel_uc_runtime_suspend, intel_uc_runtime_resume. Added
error handling to all the calls for suspend/resume.

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

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5c111ea..8635f40 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1691,7 +1691,15 @@ static int i915_drm_resume(struct drm_device *dev)
 	}
 	mutex_unlock(&dev->struct_mutex);
 
-	intel_guc_resume(dev_priv);
+	/*
+	 * NB: Full gem reinitialization is being done above, hence
+	 * intel_uc_resume will be of no use. Currently intel_uc_resume
+	 * is nop. If full reinitialization is removed, will  need to put
+	 * functionality to resume from sleep in intel_uc_resume.
+	 */
+	ret = intel_uc_resume(dev_priv);
+	if (ret)
+		DRM_ERROR("failed to resume uc\n");
 
 	intel_modeset_init_hw(dev);
 
@@ -2493,7 +2501,12 @@ static int intel_runtime_suspend(struct device *kdev)
 	 */
 	i915_gem_runtime_suspend(dev_priv);
 
-	intel_guc_suspend(dev_priv);
+	ret = intel_uc_runtime_suspend(dev_priv);
+	if (ret) {
+		DRM_ERROR("uc runtime suspend failed, disabling it(%d)\n", ret);
+		enable_rpm_wakeref_asserts(dev_priv);
+		return ret;
+	}
 
 	intel_runtime_pm_disable_interrupts(dev_priv);
 
@@ -2578,7 +2591,11 @@ static int intel_runtime_resume(struct device *kdev)
 	if (intel_uncore_unclaimed_mmio(dev_priv))
 		DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
 
-	intel_guc_resume(dev_priv);
+	ret = intel_uc_runtime_resume(dev_priv);
+	if (ret) {
+		DRM_ERROR("uc runtime resume failed (%d)\n", ret);
+		return ret;
+	}
 
 	if (IS_GEN9_LP(dev_priv)) {
 		bxt_disable_dc9(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c4bf348..dd56d45 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4539,7 +4539,11 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
 	i915_gem_contexts_lost(dev_priv);
 	mutex_unlock(&dev->struct_mutex);
 
-	intel_guc_suspend(dev_priv);
+	ret = intel_uc_suspend(dev_priv);
+	if (ret) {
+		DRM_ERROR("uc suspend failed (%d)\n", ret);
+		goto out;
+	}
 
 	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
 	cancel_delayed_work_sync(&dev_priv->gt.retire_work);
@@ -4583,6 +4587,7 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
 
 err_unlock:
 	mutex_unlock(&dev->struct_mutex);
+out:
 	intel_runtime_pm_put(dev_priv);
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 0178ba4..8e4d8b0 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -537,3 +537,23 @@ int intel_guc_sample_forcewake(struct intel_guc *guc)
 
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
 }
+
+int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv)
+{
+	return intel_guc_suspend(dev_priv);
+}
+
+int intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
+{
+	return intel_guc_resume(dev_priv);
+}
+
+int intel_uc_suspend(struct drm_i915_private *dev_priv)
+{
+	return intel_guc_suspend(dev_priv);
+}
+
+int intel_uc_resume(struct drm_i915_private *dev_priv)
+{
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 7703c9a..3d33a51 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -208,6 +208,10 @@ struct intel_huc {
 void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
 int intel_uc_init_hw(struct drm_i915_private *dev_priv);
 void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
+int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv);
+int intel_uc_runtime_resume(struct drm_i915_private *dev_priv);
+int intel_uc_suspend(struct drm_i915_private *dev_priv);
+int intel_uc_resume(struct drm_i915_private *dev_priv);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
 int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
 int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH v4 2/9] drm/i915/guc: Update prototype/name of GuC suspend/resume fns and move to intel_uc.c
  2017-09-20 17:38 [PATCH v4 0/9] GuC Fixes, Minor restructuring changes and v9+ logging change Sagar Arun Kamble
  2017-09-20 17:38 ` [PATCH v4 1/9] drm/i915: Create uc runtime and system suspend/resume helpers Sagar Arun Kamble
@ 2017-09-20 17:38 ` Sagar Arun Kamble
  2017-09-20 21:25   ` Michal Wajdeczko
  2017-09-20 17:38 ` [PATCH v4 3/9] drm/i915/guc: Update GuC ggtt.invalidate/interrupts/communication across RPM suspend/resume Sagar Arun Kamble
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 29+ messages in thread
From: Sagar Arun Kamble @ 2017-09-20 17:38 UTC (permalink / raw)
  To: intel-gfx

Renamed intel_guc_suspend to intel_guc_enter_sleep and intel_guc_resume
to intel_guc_exit_sleep to match GuC nomenclature compatibility.
We plan to use intel_guc_suspend and intel_guc_resume through
intel_uc_suspend and intel_uc_resume in the path i915_drm_suspend and
i915_drm_resume respectively for better naming.
Also, with this patch we pass intel_guc struct as parameter to enter_sleep
and exit_sleep functions as they are GuC specific and they are moved to
intel_uc.c as static functions called from uc generic functions.

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

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

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index e191d56..94efe32 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1205,55 +1205,3 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
 	guc_client_free(guc->execbuf_client);
 	guc->execbuf_client = NULL;
 }
-
-/**
- * intel_guc_suspend() - notify GuC entering suspend state
- * @dev_priv:	i915 device private
- */
-int intel_guc_suspend(struct drm_i915_private *dev_priv)
-{
-	struct intel_guc *guc = &dev_priv->guc;
-	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;
-	/* any value greater than GUC_POWER_D0 */
-	data[1] = GUC_POWER_D1;
-	/* first page is shared data with GuC */
-	data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + LRC_GUCSHR_PN * PAGE_SIZE;
-
-	return intel_guc_send(guc, data, ARRAY_SIZE(data));
-}
-
-/**
- * intel_guc_resume() - notify GuC resuming from suspend state
- * @dev_priv:	i915 device private
- */
-int intel_guc_resume(struct drm_i915_private *dev_priv)
-{
-	struct intel_guc *guc = &dev_priv->guc;
-	struct i915_gem_context *ctx;
-	u32 data[3];
-
-	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
-		return 0;
-
-	if (i915.guc_log_level >= 0)
-		gen9_enable_guc_interrupts(dev_priv);
-
-	ctx = dev_priv->kernel_context;
-
-	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
-	data[1] = GUC_POWER_D0;
-	/* first page is shared data with GuC */
-	data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + LRC_GUCSHR_PN * PAGE_SIZE;
-
-	return intel_guc_send(guc, data, ARRAY_SIZE(data));
-}
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 8e4d8b0..0dbb4b9 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -538,19 +538,71 @@ int intel_guc_sample_forcewake(struct intel_guc *guc)
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
 }
 
+/**
+ * intel_guc_enter_sleep() - notify GuC entering sleep state
+ * @guc:	guc structure
+ */
+static int intel_guc_enter_sleep(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	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;
+	/* any value greater than GUC_POWER_D0 */
+	data[1] = GUC_POWER_D1;
+	/* first page is shared data with GuC */
+	data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + LRC_GUCSHR_PN * PAGE_SIZE;
+
+	return intel_guc_send(guc, data, ARRAY_SIZE(data));
+}
+
+/**
+ * intel_guc_exit_sleep() - notify GuC exit from sleep state
+ * @guc:	guc structure
+ */
+static int intel_guc_exit_sleep(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	struct i915_gem_context *ctx;
+	u32 data[3];
+
+	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
+		return 0;
+
+	if (i915.guc_log_level >= 0)
+		gen9_enable_guc_interrupts(dev_priv);
+
+	ctx = dev_priv->kernel_context;
+
+	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
+	data[1] = GUC_POWER_D0;
+	/* first page is shared data with GuC */
+	data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + LRC_GUCSHR_PN * PAGE_SIZE;
+
+	return intel_guc_send(guc, data, ARRAY_SIZE(data));
+}
+
 int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv)
 {
-	return intel_guc_suspend(dev_priv);
+	return intel_guc_enter_sleep(&dev_priv->guc);
 }
 
 int intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
 {
-	return intel_guc_resume(dev_priv);
+	return intel_guc_exit_sleep(&dev_priv->guc);
 }
 
 int intel_uc_suspend(struct drm_i915_private *dev_priv)
 {
-	return intel_guc_suspend(dev_priv);
+	return intel_guc_enter_sleep(&dev_priv->guc);
 }
 
 int intel_uc_resume(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 3d33a51..5f49d13 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -229,8 +229,6 @@ static inline void intel_guc_notify(struct intel_guc *guc)
 /* intel_guc_loader.c */
 int intel_guc_select_fw(struct intel_guc *guc);
 int intel_guc_init_hw(struct intel_guc *guc);
-int intel_guc_suspend(struct drm_i915_private *dev_priv);
-int intel_guc_resume(struct drm_i915_private *dev_priv);
 u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
 
 /* i915_guc_submission.c */
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH v4 3/9] drm/i915/guc: Update GuC ggtt.invalidate/interrupts/communication across RPM suspend/resume
  2017-09-20 17:38 [PATCH v4 0/9] GuC Fixes, Minor restructuring changes and v9+ logging change Sagar Arun Kamble
  2017-09-20 17:38 ` [PATCH v4 1/9] drm/i915: Create uc runtime and system suspend/resume helpers Sagar Arun Kamble
  2017-09-20 17:38 ` [PATCH v4 2/9] drm/i915/guc: Update prototype/name of GuC suspend/resume fns and move to intel_uc.c Sagar Arun Kamble
@ 2017-09-20 17:38 ` Sagar Arun Kamble
  2017-09-20 21:33   ` Michal Wajdeczko
  2017-09-20 17:38 ` [PATCH v4 4/9] drm/i915/guc: Update suspend functionality in intel_uc_suspend path Sagar Arun Kamble
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 29+ messages in thread
From: Sagar Arun Kamble @ 2017-09-20 17:38 UTC (permalink / raw)
  To: intel-gfx

Apart from configuring interrupts, we need to update the ggtt invalidate
interface and GuC communication on suspend. This functionality can be
reused for other suspend and reset paths.
Prepared GuC specific helpers to handle these suspend/resume tasks
namely - intel_guc_runtime_suspend, intel_guc_runtime_resume.

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

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_uc.c | 66 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 59 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 0dbb4b9..fa698db 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -551,8 +551,6 @@ static int intel_guc_enter_sleep(struct intel_guc *guc)
 	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;
@@ -577,9 +575,6 @@ static int intel_guc_exit_sleep(struct intel_guc *guc)
 	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return 0;
 
-	if (i915.guc_log_level >= 0)
-		gen9_enable_guc_interrupts(dev_priv);
-
 	ctx = dev_priv->kernel_context;
 
 	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
@@ -590,14 +585,71 @@ static int intel_guc_exit_sleep(struct intel_guc *guc)
 	return intel_guc_send(guc, data, ARRAY_SIZE(data));
 }
 
+int intel_guc_runtime_suspend(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	int ret;
+
+	ret = intel_guc_enter_sleep(guc);
+	if (ret) {
+		DRM_ERROR("GuC enter sleep failed (%d)\n", ret);
+		return ret;
+	}
+
+	i915_ggtt_disable_guc(dev_priv);
+	gen9_disable_guc_interrupts(dev_priv);
+
+	return 0;
+}
+
+int intel_guc_runtime_resume(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	int ret;
+
+	if (i915.guc_log_level >= 0)
+		gen9_enable_guc_interrupts(dev_priv);
+	i915_ggtt_enable_guc(dev_priv);
+
+	ret = intel_guc_exit_sleep(guc);
+	if (ret) {
+		DRM_ERROR("GuC exit sleep failed (%d)\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv)
 {
-	return intel_guc_enter_sleep(&dev_priv->guc);
+	int ret;
+
+	if (!i915.enable_guc_loading)
+		return 0;
+
+	ret = intel_guc_runtime_suspend(&dev_priv->guc);
+	if (ret)
+		return ret;
+
+	guc_disable_communication(&dev_priv->guc);
+
+	return 0;
 }
 
 int intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
 {
-	return intel_guc_exit_sleep(&dev_priv->guc);
+	int ret;
+
+	if (!i915.enable_guc_loading)
+		return 0;
+
+	ret = guc_enable_communication(&dev_priv->guc);
+	if (ret) {
+		DRM_ERROR("GuC enable communication failed (%d)\n", ret);
+		return ret;
+	}
+
+	return intel_guc_runtime_resume(&dev_priv->guc);
 }
 
 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

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH v4 4/9] drm/i915/guc: Update suspend functionality in intel_uc_suspend path
  2017-09-20 17:38 [PATCH v4 0/9] GuC Fixes, Minor restructuring changes and v9+ logging change Sagar Arun Kamble
                   ` (2 preceding siblings ...)
  2017-09-20 17:38 ` [PATCH v4 3/9] drm/i915/guc: Update GuC ggtt.invalidate/interrupts/communication across RPM suspend/resume Sagar Arun Kamble
@ 2017-09-20 17:38 ` Sagar Arun Kamble
  2017-09-21 16:46   ` Michał Winiarski
  2017-09-20 17:38 ` [PATCH v4 5/9] drm/i915/guc: Disable GuC submission and suspend it prior to i915 reset Sagar Arun Kamble
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 29+ messages in thread
From: Sagar Arun Kamble @ 2017-09-20 17:38 UTC (permalink / raw)
  To: intel-gfx

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

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

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

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index fa698db..0c7e45c7 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -446,9 +446,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 	if (!i915.enable_guc_loading)
 		return;
 
-	if (i915.enable_guc_submission)
-		i915_guc_submission_disable(dev_priv);
-
 	guc_disable_communication(&dev_priv->guc);
 
 	if (i915.enable_guc_submission) {
@@ -654,7 +651,20 @@ int intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
 
 int intel_uc_suspend(struct drm_i915_private *dev_priv)
 {
-	return intel_guc_enter_sleep(&dev_priv->guc);
+	struct drm_device *dev = &dev_priv->drm;
+	int ret;
+
+	if (i915.enable_guc_submission) {
+		mutex_lock(&dev->struct_mutex);
+		i915_guc_submission_disable(dev_priv);
+		mutex_unlock(&dev->struct_mutex);
+	}
+
+	ret = intel_uc_runtime_suspend(dev_priv);
+	if (ret)
+		return ret;
+
+	return 0;
 }
 
 int intel_uc_resume(struct drm_i915_private *dev_priv)
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH v4 5/9] drm/i915/guc: Disable GuC submission and suspend it prior to i915 reset
  2017-09-20 17:38 [PATCH v4 0/9] GuC Fixes, Minor restructuring changes and v9+ logging change Sagar Arun Kamble
                   ` (3 preceding siblings ...)
  2017-09-20 17:38 ` [PATCH v4 4/9] drm/i915/guc: Update suspend functionality in intel_uc_suspend path Sagar Arun Kamble
@ 2017-09-20 17:38 ` Sagar Arun Kamble
  2017-09-20 21:45   ` Michal Wajdeczko
  2017-09-20 17:38 ` [PATCH v4 6/9] drm/i915/guc: Fix GuC cleanup in unload path Sagar Arun Kamble
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 29+ messages in thread
From: Sagar Arun Kamble @ 2017-09-20 17:38 UTC (permalink / raw)
  To: intel-gfx

Before i915 reset we need to disable GuC submission and suspend GuC
operarions 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.

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index dd56d45..76e1bb2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2847,6 +2847,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 0c7e45c7..aac8526 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -671,3 +671,17 @@ int intel_uc_resume(struct drm_i915_private *dev_priv)
 {
 	return 0;
 }
+
+int intel_uc_reset_prepare(struct drm_i915_private *dev_priv)
+{
+	int ret;
+
+	if (i915.enable_guc_submission)
+		i915_guc_submission_disable(dev_priv);
+
+	ret = intel_uc_runtime_suspend(dev_priv);
+	if (ret)
+		return ret;
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 5f49d13..069c2b2 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -212,6 +212,7 @@ struct intel_huc {
 int intel_uc_runtime_resume(struct drm_i915_private *dev_priv);
 int intel_uc_suspend(struct drm_i915_private *dev_priv);
 int intel_uc_resume(struct drm_i915_private *dev_priv);
+int intel_uc_reset_prepare(struct drm_i915_private *dev_priv);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
 int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
 int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH v4 6/9] drm/i915/guc: Fix GuC cleanup in unload path
  2017-09-20 17:38 [PATCH v4 0/9] GuC Fixes, Minor restructuring changes and v9+ logging change Sagar Arun Kamble
                   ` (4 preceding siblings ...)
  2017-09-20 17:38 ` [PATCH v4 5/9] drm/i915/guc: Disable GuC submission and suspend it prior to i915 reset Sagar Arun Kamble
@ 2017-09-20 17:38 ` Sagar Arun Kamble
  2017-09-20 21:53   ` Michal Wajdeczko
  2017-09-20 17:38 ` [PATCH v4 7/9] drm/i915/guc: Remove i915_guc_log_unregister Sagar Arun Kamble
                   ` (4 subsequent siblings)
  10 siblings, 1 reply; 29+ messages in thread
From: Sagar Arun Kamble @ 2017-09-20 17:38 UTC (permalink / raw)
  To: intel-gfx

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.
With this patch we are also doing guc_free_load_err_log only
if i915.enable_guc_loading is set.
Created intel_guc_cleanup function to wrap the cleanup functions
specific to GuC.

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

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 8635f40..6f36ced 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -602,7 +602,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 94efe32..12f1195 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1050,7 +1050,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 aac8526..8c42344 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -418,7 +418,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	guc_capture_load_err_log(guc);
 err_submission:
 	if (i915.enable_guc_submission)
-		i915_guc_submission_fini(dev_priv);
+		i915_guc_submission_cleanup(dev_priv);
 err_guc:
 	i915_ggtt_disable_guc(dev_priv);
 
@@ -439,21 +439,22 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
+static void intel_guc_cleanup(struct intel_guc *guc)
 {
-	guc_free_load_err_log(&dev_priv->guc);
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+	if (i915.enable_guc_submission)
+		i915_guc_submission_cleanup(dev_priv);
+}
 
+void intel_uc_cleanup(struct drm_i915_private *dev_priv)
+{
 	if (!i915.enable_guc_loading)
 		return;
 
-	guc_disable_communication(&dev_priv->guc);
-
-	if (i915.enable_guc_submission) {
-		gen9_disable_guc_interrupts(dev_priv);
-		i915_guc_submission_fini(dev_priv);
-	}
+	guc_free_load_err_log(&dev_priv->guc);
 
-	i915_ggtt_disable_guc(dev_priv);
+	intel_guc_cleanup(&dev_priv->guc);
 }
 
 int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 069c2b2..8557e33 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);
@@ -236,7 +236,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

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH v4 7/9] drm/i915/guc: Remove i915_guc_log_unregister
  2017-09-20 17:38 [PATCH v4 0/9] GuC Fixes, Minor restructuring changes and v9+ logging change Sagar Arun Kamble
                   ` (5 preceding siblings ...)
  2017-09-20 17:38 ` [PATCH v4 6/9] drm/i915/guc: Fix GuC cleanup in unload path Sagar Arun Kamble
@ 2017-09-20 17:38 ` Sagar Arun Kamble
  2017-09-20 20:58   ` Michal Wajdeczko
  2017-09-20 17:38 ` [PATCH v4 8/9] drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9 Sagar Arun Kamble
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 29+ messages in thread
From: Sagar Arun Kamble @ 2017-09-20 17:38 UTC (permalink / raw)
  To: intel-gfx

Functionality needed to disable GuC interrupts and cleanup the
runtime/relay data structures is already covered in the unload path
via intel_guc_fini_hw and intel_guc_cleanup hence remove
i915_guc_log_unregister

v2: Removed the function i915_guc_log_unregister.

v3: Rebase as intel_guc.h is removed.

v4: Rebase as intel_guc.h is created again. :)

v5: Rebase as intel_guc.h is removed.

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      |  1 -
 drivers/gpu/drm/i915/intel_guc_log.c | 12 ------------
 drivers/gpu/drm/i915/intel_uc.h      |  1 -
 3 files changed, 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6f36ced..c69a30a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1252,7 +1252,6 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
 	i915_perf_unregister(dev_priv);
 
 	i915_teardown_sysfs(dev_priv);
-	i915_guc_log_unregister(dev_priv);
 	drm_dev_unregister(&dev_priv->drm);
 
 	i915_gem_shrinker_cleanup(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 16d3b87..3c45681 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -648,15 +648,3 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv)
 	guc_log_late_setup(&dev_priv->guc);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 }
-
-void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
-{
-	if (!i915.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);
-	guc_log_runtime_destroy(&dev_priv->guc);
-	mutex_unlock(&dev_priv->drm.struct_mutex);
-}
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 8557e33..c2c104a 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -244,7 +244,6 @@ static inline void intel_guc_notify(struct intel_guc *guc)
 void intel_guc_log_destroy(struct intel_guc *guc);
 int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
 void i915_guc_log_register(struct drm_i915_private *dev_priv);
-void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
 
 static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 {
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH v4 8/9] drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9
  2017-09-20 17:38 [PATCH v4 0/9] GuC Fixes, Minor restructuring changes and v9+ logging change Sagar Arun Kamble
                   ` (6 preceding siblings ...)
  2017-09-20 17:38 ` [PATCH v4 7/9] drm/i915/guc: Remove i915_guc_log_unregister Sagar Arun Kamble
@ 2017-09-20 17:38 ` Sagar Arun Kamble
  2017-09-20 17:38 ` [PATCH v4 9/9] drm/i915: Reorganize HuC authentication Sagar Arun Kamble
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 29+ messages in thread
From: Sagar Arun Kamble @ 2017-09-20 17:38 UTC (permalink / raw)
  To: intel-gfx; +Cc: Chheda Harsh J, Fry Gregory P

With GuC v9, new type of Default/critical logging in GuC to enable
capturing minimal important logs in production systems efficiently.
This patch enables this logging in GuC by default always. It should
be noted that streaming support with half-full interrupt mechanism
that is present for normal logging is not present for this type of
logging.

v2: Emulated GuC critical logging through i915.guc_log_level.

v3: Commit message update. Enable default/critical logging in GuC always.
Fixed RPM wake during guc_log_unregister in the unload path.

v4: Moved RPM wake change to separate patch. Removed GUC_DEBUG_RESERVED
and updated name of new bit to be version agnostic. Updated parameter to
struct intel_guc * and name of macro NEEDS_GUC_CRITICAL_LOGGING.
Removed explicit clearing of GUC_CRITICAL_LOGGING_DISABLED from
params[GUC_CTL_DEBUG] as it is unnecessary. (Michal Wajdeczko)

v5: Removed GUC_CRITICAL_LOGGING_DISABLED. Added HAS_GUC check to
GUC_NEEDS_CRITICAL_LOGGING. (Michal Wajdeczko)

v6: More refined version of GUC_NEEDS_CRITICAL_LOGGING. Commit message
update. (Michal Wajdeczko)

Cc: Chheda Harsh J <harsh.j.chheda@intel.com>
Cc: Fry Gregory P <gregory.p.fry@intel.com>
Cc: Spotswood John A <john.a.spotswood@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_fwif.h |  4 ++--
 drivers/gpu/drm/i915/intel_guc_log.c  | 13 ++++++++++++-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 7eb6b4f..fed875a 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -127,7 +127,6 @@
 #define   GUC_PROFILE_ENABLED		(1 << 7)
 #define   GUC_WQ_TRACK_ENABLED		(1 << 8)
 #define   GUC_ADS_ENABLED		(1 << 9)
-#define   GUC_DEBUG_RESERVED		(1 << 10)
 #define   GUC_ADS_ADDR_SHIFT		11
 #define   GUC_ADS_ADDR_MASK		0xfffff800
 
@@ -539,7 +538,8 @@ struct guc_log_buffer_state {
 		u32 logging_enabled:1;
 		u32 reserved1:3;
 		u32 verbosity:4;
-		u32 reserved2:24;
+		u32 critical_logging_enabled:1;
+		u32 reserved2:23;
 	};
 	u32 value;
 } __packed;
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 3c45681..b820175 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -586,10 +586,18 @@ void intel_guc_log_destroy(struct intel_guc *guc)
 	i915_vma_unpin_and_release(&guc->log.vma);
 }
 
+/*
+ * Critical logging in GuC is to be enabled always from GuC v9+.
+ * (for KBL - v9.39+)
+ */
+#define GUC_NEEDS_CRITICAL_LOGGING(guc)	\
+	(HAS_GUC(guc_to_i915(guc)) && \
+	 (guc->fw.major_ver_found >= 9) && \
+	 (guc->fw.minor_ver_found >= (IS_KABYLAKE(guc_to_i915(guc)) ? 39 : 0)))
+
 int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 {
 	struct intel_guc *guc = &dev_priv->guc;
-
 	union guc_log_control log_param;
 	int ret;
 
@@ -603,6 +611,9 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 	if (!log_param.logging_enabled && (i915.guc_log_level < 0))
 		return 0;
 
+	if (GUC_NEEDS_CRITICAL_LOGGING(guc))
+		log_param.critical_logging_enabled = 1;
+
 	ret = guc_log_control(guc, log_param.value);
 	if (ret < 0) {
 		DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", ret);
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH v4 9/9] drm/i915: Reorganize HuC authentication
  2017-09-20 17:38 [PATCH v4 0/9] GuC Fixes, Minor restructuring changes and v9+ logging change Sagar Arun Kamble
                   ` (7 preceding siblings ...)
  2017-09-20 17:38 ` [PATCH v4 8/9] drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9 Sagar Arun Kamble
@ 2017-09-20 17:38 ` Sagar Arun Kamble
  2017-09-20 18:47 ` ✓ Fi.CI.BAT: success for GuC Fixes, Minor restructuring changes and v9+ logging change Patchwork
  2017-09-20 20:55 ` ✗ Fi.CI.IGT: failure " Patchwork
  10 siblings, 0 replies; 29+ messages in thread
From: Sagar Arun Kamble @ 2017-09-20 17:38 UTC (permalink / raw)
  To: intel-gfx

Prepared intel_auth_huc to separate HuC specific functionality
from GuC send action. Created new header intel_huc.h to group
HuC specific declarations.

v2: Changed argument preparation for AUTHENTICATE_HUC.
s/intel_auth_huc/intel_huc_auth. Deferred creation of intel_huc.h
to later patch.

v3: Rebase as intel_guc.h is removed. Added param description to
intel_huc_auth. (Michal)

v4: Rebase as intel_guc.h is added again. :)

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

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

diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 6145fa0..d3da4d3 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -225,19 +225,16 @@ void intel_huc_init_hw(struct intel_huc *huc)
 }
 
 /**
- * intel_guc_auth_huc() - authenticate ucode
- * @dev_priv: the drm_i915_device
- *
- * Triggers a HuC fw authentication request to the GuC via intel_guc_action_
- * authenticate_huc interface.
+ * intel_huc_auth() - authenticate ucode
+ * @huc: intel_huc structure
  */
-void intel_guc_auth_huc(struct drm_i915_private *dev_priv)
+void intel_huc_auth(struct intel_huc *huc)
 {
+	struct drm_i915_private *dev_priv = huc_to_i915(huc);
 	struct intel_guc *guc = &dev_priv->guc;
-	struct intel_huc *huc = &dev_priv->huc;
 	struct i915_vma *vma;
+	u32 offset;
 	int ret;
-	u32 data[2];
 
 	if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return;
@@ -250,11 +247,8 @@ void intel_guc_auth_huc(struct drm_i915_private *dev_priv)
 		return;
 	}
 
-	/* Specify auth action and where public signature is. */
-	data[0] = INTEL_GUC_ACTION_AUTHENTICATE_HUC;
-	data[1] = guc_ggtt_offset(vma) + huc->fw.rsa_offset;
-
-	ret = intel_guc_send(guc, data, ARRAY_SIZE(data));
+	offset = guc_ggtt_offset(vma) + huc->fw.rsa_offset;
+	ret = intel_guc_auth_huc(guc, offset);
 	if (ret) {
 		DRM_ERROR("HuC: GuC did not ack Auth request %d\n", ret);
 		goto out;
@@ -266,7 +260,6 @@ void intel_guc_auth_huc(struct drm_i915_private *dev_priv)
 				HUC_FW_VERIFIED,
 				HUC_FW_VERIFIED,
 				50);
-
 	if (ret) {
 		DRM_ERROR("HuC: Authentication failed %d\n", ret);
 		goto out;
@@ -275,4 +268,3 @@ void intel_guc_auth_huc(struct drm_i915_private *dev_priv)
 out:
 	i915_vma_unpin(vma);
 }
-
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 8c42344..6a64cd0 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -328,6 +328,24 @@ static void guc_disable_communication(struct intel_guc *guc)
 	guc->send = intel_guc_send_nop;
 }
 
+/**
+ * intel_guc_auth_huc() - authenticate ucode
+ * @guc: struct intel_guc*
+ * @offset: rsa offset w.r.t ggtt base of huc vma
+ *
+ * triggers a huc fw authentication request to the guc via intel_guc_send
+ * authenticate_huc interface.
+ */
+int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset)
+{
+	u32 action[] = {
+		INTEL_GUC_ACTION_AUTHENTICATE_HUC,
+		rsa_offset
+	};
+
+	return intel_guc_send(guc, action, ARRAY_SIZE(action));
+}
+
 int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
@@ -390,7 +408,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto err_log_capture;
 
-	intel_guc_auth_huc(dev_priv);
+	intel_huc_auth(&dev_priv->huc);
 	if (i915.enable_guc_submission) {
 		if (i915.guc_log_level >= 0)
 			gen9_enable_guc_interrupts(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index c2c104a..cc2ee4f 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -216,6 +216,7 @@ struct intel_huc {
 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);
+int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
 
 static inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
 {
@@ -256,6 +257,6 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 /* intel_huc.c */
 void intel_huc_select_fw(struct intel_huc *huc);
 void intel_huc_init_hw(struct intel_huc *huc);
-void intel_guc_auth_huc(struct drm_i915_private *dev_priv);
+void intel_huc_auth(struct intel_huc *huc);
 
 #endif
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* ✓ Fi.CI.BAT: success for GuC Fixes, Minor restructuring changes and v9+ logging change
  2017-09-20 17:38 [PATCH v4 0/9] GuC Fixes, Minor restructuring changes and v9+ logging change Sagar Arun Kamble
                   ` (8 preceding siblings ...)
  2017-09-20 17:38 ` [PATCH v4 9/9] drm/i915: Reorganize HuC authentication Sagar Arun Kamble
@ 2017-09-20 18:47 ` Patchwork
  2017-09-20 20:55 ` ✗ Fi.CI.IGT: failure " Patchwork
  10 siblings, 0 replies; 29+ messages in thread
From: Patchwork @ 2017-09-20 18:47 UTC (permalink / raw)
  To: Sagar Arun Kamble; +Cc: intel-gfx

== Series Details ==

Series: GuC Fixes, Minor restructuring changes and v9+ logging change
URL   : https://patchwork.freedesktop.org/series/30666/
State : success

== Summary ==

Series 30666v1 GuC Fixes, Minor restructuring changes and v9+ logging change
https://patchwork.freedesktop.org/api/1.0/series/30666/revisions/1/mbox/

Test gem_ringfill:
        Subgroup basic-default-hang:
                incomplete -> DMESG-WARN (fi-pnv-d510) fdo#101600
Test gem_sync:
        Subgroup basic-each:
                dmesg-warn -> PASS       (fi-kbl-7500u)
Test kms_addfb_basic:
        Subgroup bad-pitch-256:
                dmesg-warn -> PASS       (fi-kbl-7500u)
        Subgroup invalid-get-prop-any:
                dmesg-warn -> PASS       (fi-kbl-7500u)
        Subgroup unused-offsets:
                dmesg-warn -> PASS       (fi-kbl-7500u)
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                fail       -> PASS       (fi-snb-2600) fdo#100215 +1
Test kms_frontbuffer_tracking:
        Subgroup basic:
                dmesg-warn -> PASS       (fi-bdw-5557u) fdo#102473
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                incomplete -> PASS       (fi-kbl-7500u) fdo#102850

fdo#101600 https://bugs.freedesktop.org/show_bug.cgi?id=101600
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#102473 https://bugs.freedesktop.org/show_bug.cgi?id=102473
fdo#102850 https://bugs.freedesktop.org/show_bug.cgi?id=102850

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:442s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:476s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:423s
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:276s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:498s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:492s
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:492s
fi-cfl-s         total:289  pass:223  dwarn:34  dfail:0   fail:0   skip:32  time:539s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:422s
fi-glk-1         total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:562s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:425s
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:433s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:490s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:457s
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:575s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:583s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:541s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:450s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:750s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:482s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:473s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:567s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:1   skip:39  time:423s

939fdb0533e7b2cb97a192864fc18005072f6739 drm-tip: 2017y-09m-20d-17h-36m-21s UTC integration manifest
8d090da1d0f2 drm/i915: Reorganize HuC authentication
c192fa0005fe drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9
d8d0cddfa0d9 drm/i915/guc: Remove i915_guc_log_unregister
351d0a34f71b drm/i915/guc: Fix GuC cleanup in unload path
84d348c5a970 drm/i915/guc: Disable GuC submission and suspend it prior to i915 reset
d534d9fecfa8 drm/i915/guc: Update suspend functionality in intel_uc_suspend path
f02b0b985159 drm/i915/guc: Update GuC ggtt.invalidate/interrupts/communication across RPM suspend/resume
d71299f76d10 drm/i915/guc: Update prototype/name of GuC suspend/resume fns and move to intel_uc.c
94e8ceb4ae5f drm/i915: Create uc runtime and system suspend/resume helpers

== Logs ==

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* ✗ Fi.CI.IGT: failure for GuC Fixes, Minor restructuring changes and v9+ logging change
  2017-09-20 17:38 [PATCH v4 0/9] GuC Fixes, Minor restructuring changes and v9+ logging change Sagar Arun Kamble
                   ` (9 preceding siblings ...)
  2017-09-20 18:47 ` ✓ Fi.CI.BAT: success for GuC Fixes, Minor restructuring changes and v9+ logging change Patchwork
@ 2017-09-20 20:55 ` Patchwork
  2017-09-21  3:11   ` Kamble, Sagar A
  10 siblings, 1 reply; 29+ messages in thread
From: Patchwork @ 2017-09-20 20:55 UTC (permalink / raw)
  To: Sagar Arun Kamble; +Cc: intel-gfx

== Series Details ==

Series: GuC Fixes, Minor restructuring changes and v9+ logging change
URL   : https://patchwork.freedesktop.org/series/30666/
State : failure

== Summary ==

Test kms_flip:
        Subgroup modeset-vs-vblank-race:
                pass       -> FAIL       (shard-hsw)
Test perf:
        Subgroup blocking:
                fail       -> PASS       (shard-hsw) fdo#102252 +1

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

shard-hsw        total:2317 pass:1247 dwarn:2   dfail:0   fail:11  skip:1057 time:9646s

== Logs ==

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 7/9] drm/i915/guc: Remove i915_guc_log_unregister
  2017-09-20 17:38 ` [PATCH v4 7/9] drm/i915/guc: Remove i915_guc_log_unregister Sagar Arun Kamble
@ 2017-09-20 20:58   ` Michal Wajdeczko
  2017-09-21 17:31     ` Sagar Arun Kamble
  0 siblings, 1 reply; 29+ messages in thread
From: Michal Wajdeczko @ 2017-09-20 20:58 UTC (permalink / raw)
  To: intel-gfx, Sagar Arun Kamble

On Wed, 20 Sep 2017 19:38:22 +0200, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> Functionality needed to disable GuC interrupts and cleanup the
> runtime/relay data structures is already covered in the unload path
> via intel_guc_fini_hw and intel_guc_cleanup hence remove
> i915_guc_log_unregister
>
> v2: Removed the function i915_guc_log_unregister.
>
> v3: Rebase as intel_guc.h is removed.
>
> v4: Rebase as intel_guc.h is created again. :)
>
> v5: Rebase as intel_guc.h is removed.
>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>

I'm afraid I've to revoke my r-b as with removal of the log_unregister()
we will loose symmetry with log_register() where relay_late_setup_files()
was hidden, and we should still clean it up in i915_driver_unregister()

Michal

> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |  1 -
>  drivers/gpu/drm/i915/intel_guc_log.c | 12 ------------
>  drivers/gpu/drm/i915/intel_uc.h      |  1 -
>  3 files changed, 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c  
> b/drivers/gpu/drm/i915/i915_drv.c
> index 6f36ced..c69a30a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1252,7 +1252,6 @@ static void i915_driver_unregister(struct  
> drm_i915_private *dev_priv)
>  	i915_perf_unregister(dev_priv);
> 	i915_teardown_sysfs(dev_priv);
> -	i915_guc_log_unregister(dev_priv);
>  	drm_dev_unregister(&dev_priv->drm);
> 	i915_gem_shrinker_cleanup(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c  
> b/drivers/gpu/drm/i915/intel_guc_log.c
> index 16d3b87..3c45681 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -648,15 +648,3 @@ void i915_guc_log_register(struct drm_i915_private  
> *dev_priv)
>  	guc_log_late_setup(&dev_priv->guc);
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
>  }
> -
> -void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
> -{
> -	if (!i915.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);
> -	guc_log_runtime_destroy(&dev_priv->guc);
> -	mutex_unlock(&dev_priv->drm.struct_mutex);
> -}
> diff --git a/drivers/gpu/drm/i915/intel_uc.h  
> b/drivers/gpu/drm/i915/intel_uc.h
> index 8557e33..c2c104a 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -244,7 +244,6 @@ static inline void intel_guc_notify(struct intel_guc  
> *guc)
>  void intel_guc_log_destroy(struct intel_guc *guc);
>  int i915_guc_log_control(struct drm_i915_private *dev_priv, u64  
> control_val);
>  void i915_guc_log_register(struct drm_i915_private *dev_priv);
> -void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
> static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>  {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 2/9] drm/i915/guc: Update prototype/name of GuC suspend/resume fns and move to intel_uc.c
  2017-09-20 17:38 ` [PATCH v4 2/9] drm/i915/guc: Update prototype/name of GuC suspend/resume fns and move to intel_uc.c Sagar Arun Kamble
@ 2017-09-20 21:25   ` Michal Wajdeczko
  2017-09-21 16:40     ` Sagar Arun Kamble
  0 siblings, 1 reply; 29+ messages in thread
From: Michal Wajdeczko @ 2017-09-20 21:25 UTC (permalink / raw)
  To: intel-gfx, Sagar Arun Kamble

On Wed, 20 Sep 2017 19:38:17 +0200, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> Renamed intel_guc_suspend to intel_guc_enter_sleep and intel_guc_resume
> to intel_guc_exit_sleep to match GuC nomenclature compatibility.
> We plan to use intel_guc_suspend and intel_guc_resume through
> intel_uc_suspend and intel_uc_resume in the path i915_drm_suspend and
> i915_drm_resume respectively for better naming.
> Also, with this patch we pass intel_guc struct as parameter to  
> enter_sleep
> and exit_sleep functions as they are GuC specific and they are moved to
> intel_uc.c as static functions called from uc generic functions.
>

I'm not sure that we need this semi-refactoring right now.
We can return to this later and do it right at once.

Michal

> v2: Rebase w.r.t removal of GuC code restructuring.
>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 52  
> ---------------------------
>  drivers/gpu/drm/i915/intel_uc.c            | 58  
> ++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_uc.h            |  2 --
>  3 files changed, 55 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c  
> b/drivers/gpu/drm/i915/i915_guc_submission.c
> index e191d56..94efe32 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -1205,55 +1205,3 @@ void i915_guc_submission_disable(struct  
> drm_i915_private *dev_priv)
>  	guc_client_free(guc->execbuf_client);
>  	guc->execbuf_client = NULL;
>  }
> -
> -/**
> - * intel_guc_suspend() - notify GuC entering suspend state
> - * @dev_priv:	i915 device private
> - */
> -int intel_guc_suspend(struct drm_i915_private *dev_priv)
> -{
> -	struct intel_guc *guc = &dev_priv->guc;
> -	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;
> -	/* any value greater than GUC_POWER_D0 */
> -	data[1] = GUC_POWER_D1;
> -	/* first page is shared data with GuC */
> -	data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + LRC_GUCSHR_PN *  
> PAGE_SIZE;
> -
> -	return intel_guc_send(guc, data, ARRAY_SIZE(data));
> -}
> -
> -/**
> - * intel_guc_resume() - notify GuC resuming from suspend state
> - * @dev_priv:	i915 device private
> - */
> -int intel_guc_resume(struct drm_i915_private *dev_priv)
> -{
> -	struct intel_guc *guc = &dev_priv->guc;
> -	struct i915_gem_context *ctx;
> -	u32 data[3];
> -
> -	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> -		return 0;
> -
> -	if (i915.guc_log_level >= 0)
> -		gen9_enable_guc_interrupts(dev_priv);
> -
> -	ctx = dev_priv->kernel_context;
> -
> -	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
> -	data[1] = GUC_POWER_D0;
> -	/* first page is shared data with GuC */
> -	data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + LRC_GUCSHR_PN *  
> PAGE_SIZE;
> -
> -	return intel_guc_send(guc, data, ARRAY_SIZE(data));
> -}
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index 8e4d8b0..0dbb4b9 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -538,19 +538,71 @@ int intel_guc_sample_forcewake(struct intel_guc  
> *guc)
>  	return intel_guc_send(guc, action, ARRAY_SIZE(action));
>  }
> +/**
> + * intel_guc_enter_sleep() - notify GuC entering sleep state
> + * @guc:	guc structure
> + */
> +static int intel_guc_enter_sleep(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	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;
> +	/* any value greater than GUC_POWER_D0 */
> +	data[1] = GUC_POWER_D1;
> +	/* first page is shared data with GuC */
> +	data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + LRC_GUCSHR_PN *  
> PAGE_SIZE;
> +
> +	return intel_guc_send(guc, data, ARRAY_SIZE(data));
> +}
> +
> +/**
> + * intel_guc_exit_sleep() - notify GuC exit from sleep state
> + * @guc:	guc structure
> + */
> +static int intel_guc_exit_sleep(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	struct i915_gem_context *ctx;
> +	u32 data[3];
> +
> +	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> +		return 0;
> +
> +	if (i915.guc_log_level >= 0)
> +		gen9_enable_guc_interrupts(dev_priv);
> +
> +	ctx = dev_priv->kernel_context;
> +
> +	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
> +	data[1] = GUC_POWER_D0;
> +	/* first page is shared data with GuC */
> +	data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + LRC_GUCSHR_PN *  
> PAGE_SIZE;
> +
> +	return intel_guc_send(guc, data, ARRAY_SIZE(data));
> +}
> +
>  int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv)
>  {
> -	return intel_guc_suspend(dev_priv);
> +	return intel_guc_enter_sleep(&dev_priv->guc);
>  }
> int intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
>  {
> -	return intel_guc_resume(dev_priv);
> +	return intel_guc_exit_sleep(&dev_priv->guc);
>  }
> int intel_uc_suspend(struct drm_i915_private *dev_priv)
>  {
> -	return intel_guc_suspend(dev_priv);
> +	return intel_guc_enter_sleep(&dev_priv->guc);
>  }
> int intel_uc_resume(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/intel_uc.h  
> b/drivers/gpu/drm/i915/intel_uc.h
> index 3d33a51..5f49d13 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -229,8 +229,6 @@ static inline void intel_guc_notify(struct intel_guc  
> *guc)
>  /* intel_guc_loader.c */
>  int intel_guc_select_fw(struct intel_guc *guc);
>  int intel_guc_init_hw(struct intel_guc *guc);
> -int intel_guc_suspend(struct drm_i915_private *dev_priv);
> -int intel_guc_resume(struct drm_i915_private *dev_priv);
>  u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
> /* i915_guc_submission.c */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 3/9] drm/i915/guc: Update GuC ggtt.invalidate/interrupts/communication across RPM suspend/resume
  2017-09-20 17:38 ` [PATCH v4 3/9] drm/i915/guc: Update GuC ggtt.invalidate/interrupts/communication across RPM suspend/resume Sagar Arun Kamble
@ 2017-09-20 21:33   ` Michal Wajdeczko
  2017-09-21 16:43     ` Sagar Arun Kamble
  0 siblings, 1 reply; 29+ messages in thread
From: Michal Wajdeczko @ 2017-09-20 21:33 UTC (permalink / raw)
  To: intel-gfx, Sagar Arun Kamble

On Wed, 20 Sep 2017 19:38:18 +0200, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> Apart from configuring interrupts, we need to update the ggtt invalidate
> interface and GuC communication on suspend. This functionality can be
> reused for other suspend and reset paths.
> Prepared GuC specific helpers to handle these suspend/resume tasks
> namely - intel_guc_runtime_suspend, intel_guc_runtime_resume.
>
> v2: Rebase w.r.t removal of GuC code restructuring.
>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uc.c | 66  
> ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 59 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index 0dbb4b9..fa698db 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -551,8 +551,6 @@ static int intel_guc_enter_sleep(struct intel_guc  
> *guc)
>  	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;
> @@ -577,9 +575,6 @@ static int intel_guc_exit_sleep(struct intel_guc  
> *guc)
>  	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>  		return 0;
> -	if (i915.guc_log_level >= 0)
> -		gen9_enable_guc_interrupts(dev_priv);
> -
>  	ctx = dev_priv->kernel_context;
> 	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
> @@ -590,14 +585,71 @@ static int intel_guc_exit_sleep(struct intel_guc  
> *guc)
>  	return intel_guc_send(guc, data, ARRAY_SIZE(data));
>  }
> +int intel_guc_runtime_suspend(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	int ret;
> +
> +	ret = intel_guc_enter_sleep(guc);
> +	if (ret) {
> +		DRM_ERROR("GuC enter sleep failed (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	i915_ggtt_disable_guc(dev_priv);
> +	gen9_disable_guc_interrupts(dev_priv);

To match existing approach in intel_uc_init_hw() move interrupts
control to uc_runtime_suspend()

> +
> +	return 0;
> +}
> +
> +int intel_guc_runtime_resume(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	int ret;
> +
> +	if (i915.guc_log_level >= 0)
> +		gen9_enable_guc_interrupts(dev_priv);

Similar here

> +	i915_ggtt_enable_guc(dev_priv);
> +
> +	ret = intel_guc_exit_sleep(guc);
> +	if (ret) {
> +		DRM_ERROR("GuC exit sleep failed (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv)
>  {
> -	return intel_guc_enter_sleep(&dev_priv->guc);
> +	int ret;
> +
> +	if (!i915.enable_guc_loading)
> +		return 0;
> +
> +	ret = intel_guc_runtime_suspend(&dev_priv->guc);
> +	if (ret)
> +		return ret;
> +
> +	guc_disable_communication(&dev_priv->guc);
> +
> +	return 0;
>  }
> int intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
>  {
> -	return intel_guc_exit_sleep(&dev_priv->guc);
> +	int ret;
> +
> +	if (!i915.enable_guc_loading)
> +		return 0;
> +
> +	ret = guc_enable_communication(&dev_priv->guc);
> +	if (ret) {
> +		DRM_ERROR("GuC enable communication failed (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	return intel_guc_runtime_resume(&dev_priv->guc);
>  }
> int intel_uc_suspend(struct drm_i915_private *dev_priv)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 5/9] drm/i915/guc: Disable GuC submission and suspend it prior to i915 reset
  2017-09-20 17:38 ` [PATCH v4 5/9] drm/i915/guc: Disable GuC submission and suspend it prior to i915 reset Sagar Arun Kamble
@ 2017-09-20 21:45   ` Michal Wajdeczko
  0 siblings, 0 replies; 29+ messages in thread
From: Michal Wajdeczko @ 2017-09-20 21:45 UTC (permalink / raw)
  To: intel-gfx, Sagar Arun Kamble

On Wed, 20 Sep 2017 19:38:20 +0200, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> Before i915 reset we need to disable GuC submission and suspend GuC
> operarions 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.
>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |  2 ++
>  drivers/gpu/drm/i915/intel_uc.c | 14 ++++++++++++++
>  drivers/gpu/drm/i915/intel_uc.h |  1 +
>  3 files changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c  
> b/drivers/gpu/drm/i915/i915_gem.c
> index dd56d45..76e1bb2 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2847,6 +2847,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 0c7e45c7..aac8526 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -671,3 +671,17 @@ int intel_uc_resume(struct drm_i915_private  
> *dev_priv)
>  {
>  	return 0;
>  }
> +
> +int intel_uc_reset_prepare(struct drm_i915_private *dev_priv)
> +{
> +	int ret;
> +
> +	if (i915.enable_guc_submission)
> +		i915_guc_submission_disable(dev_priv);
> +
> +	ret = intel_uc_runtime_suspend(dev_priv);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

Make it trivial:

	return intel_uc_runtime_suspend(dev_priv);

With above fixed, you can add my r-b

Michal

> +}
> diff --git a/drivers/gpu/drm/i915/intel_uc.h  
> b/drivers/gpu/drm/i915/intel_uc.h
> index 5f49d13..069c2b2 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -212,6 +212,7 @@ struct intel_huc {
>  int intel_uc_runtime_resume(struct drm_i915_private *dev_priv);
>  int intel_uc_suspend(struct drm_i915_private *dev_priv);
>  int intel_uc_resume(struct drm_i915_private *dev_priv);
> +int intel_uc_reset_prepare(struct drm_i915_private *dev_priv);
>  int intel_guc_sample_forcewake(struct intel_guc *guc);
>  int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32  
> len);
>  int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32  
> len);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 6/9] drm/i915/guc: Fix GuC cleanup in unload path
  2017-09-20 17:38 ` [PATCH v4 6/9] drm/i915/guc: Fix GuC cleanup in unload path Sagar Arun Kamble
@ 2017-09-20 21:53   ` Michal Wajdeczko
  0 siblings, 0 replies; 29+ messages in thread
From: Michal Wajdeczko @ 2017-09-20 21:53 UTC (permalink / raw)
  To: intel-gfx, Sagar Arun Kamble

On Wed, 20 Sep 2017 19:38:21 +0200, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> We ensure that GuC is completely suspended and client is destroyed
> in i915_gem_suspend during i915_driver_unload. So now intel_uc_fini_hw
> should just take care of cleanup,
> hence s/intel_uc_fini_hw/intel_uc_cleanup. Correspondingly
> we also updated as s/i915_guc_submission_fini/i915_guc_submission_cleanup
> Other functionality to disable communication, disable interrupts and
> update of ggtt.invalidate is taken care by intel_uc_suspend.
> With this patch we are also doing guc_free_load_err_log only
> if i915.enable_guc_loading is set.
> Created intel_guc_cleanup function to wrap the cleanup functions
> specific to GuC.

This last step seems to be one too far. Try again without it.

Michal

>
> v2: Rebase w.r.t removal of GuC code restructuring.
>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c            |  2 +-
>  drivers/gpu/drm/i915/i915_guc_submission.c |  2 +-
>  drivers/gpu/drm/i915/intel_uc.c            | 21 +++++++++++----------
>  drivers/gpu/drm/i915/intel_uc.h            |  4 ++--
>  4 files changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c  
> b/drivers/gpu/drm/i915/i915_drv.c
> index 8635f40..6f36ced 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -602,7 +602,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 94efe32..12f1195 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -1050,7 +1050,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 aac8526..8c42344 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -418,7 +418,7 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	guc_capture_load_err_log(guc);
>  err_submission:
>  	if (i915.enable_guc_submission)
> -		i915_guc_submission_fini(dev_priv);
> +		i915_guc_submission_cleanup(dev_priv);
>  err_guc:
>  	i915_ggtt_disable_guc(dev_priv);
> @@ -439,21 +439,22 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	return ret;
>  }
> -void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
> +static void intel_guc_cleanup(struct intel_guc *guc)
>  {
> -	guc_free_load_err_log(&dev_priv->guc);
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> +	if (i915.enable_guc_submission)
> +		i915_guc_submission_cleanup(dev_priv);
> +}
> +void intel_uc_cleanup(struct drm_i915_private *dev_priv)
> +{
>  	if (!i915.enable_guc_loading)
>  		return;
> -	guc_disable_communication(&dev_priv->guc);
> -
> -	if (i915.enable_guc_submission) {
> -		gen9_disable_guc_interrupts(dev_priv);
> -		i915_guc_submission_fini(dev_priv);
> -	}
> +	guc_free_load_err_log(&dev_priv->guc);
> -	i915_ggtt_disable_guc(dev_priv);
> +	intel_guc_cleanup(&dev_priv->guc);
>  }
> int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
> diff --git a/drivers/gpu/drm/i915/intel_uc.h  
> b/drivers/gpu/drm/i915/intel_uc.h
> index 069c2b2..8557e33 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);
> @@ -236,7 +236,7 @@ static inline void intel_guc_notify(struct intel_guc  
> *guc)
>  int i915_guc_submission_init(struct drm_i915_private *dev_priv);
>  int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
>  void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
> -void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
> +void i915_guc_submission_cleanup(struct drm_i915_private *dev_priv);
>  struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32  
> size);
> /* intel_guc_log.c */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: ✗ Fi.CI.IGT: failure for GuC Fixes, Minor restructuring changes and v9+ logging change
  2017-09-20 20:55 ` ✗ Fi.CI.IGT: failure " Patchwork
@ 2017-09-21  3:11   ` Kamble, Sagar A
  0 siblings, 0 replies; 29+ messages in thread
From: Kamble, Sagar A @ 2017-09-21  3:11 UTC (permalink / raw)
  To: intel-gfx

Test kms_flip:
        Subgroup modeset-vs-vblank-race:
                pass       -> FAIL       (shard-hsw)

https://bugs.freedesktop.org/show_bug.cgi?id=102917


-----Original Message-----
From: Patchwork [mailto:patchwork@emeril.freedesktop.org] 
Sent: Thursday, September 21, 2017 2:25 AM
To: Kamble, Sagar A <sagar.a.kamble@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: ✗ Fi.CI.IGT: failure for GuC Fixes, Minor restructuring changes and v9+ logging change

== Series Details ==

Series: GuC Fixes, Minor restructuring changes and v9+ logging change
URL   : https://patchwork.freedesktop.org/series/30666/
State : failure

== Summary ==

Test kms_flip:
        Subgroup modeset-vs-vblank-race:
                pass       -> FAIL       (shard-hsw)
Test perf:
        Subgroup blocking:
                fail       -> PASS       (shard-hsw) fdo#102252 +1

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

shard-hsw        total:2317 pass:1247 dwarn:2   dfail:0   fail:11  skip:1057 time:9646s

== Logs ==

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 2/9] drm/i915/guc: Update prototype/name of GuC suspend/resume fns and move to intel_uc.c
  2017-09-20 21:25   ` Michal Wajdeczko
@ 2017-09-21 16:40     ` Sagar Arun Kamble
  0 siblings, 0 replies; 29+ messages in thread
From: Sagar Arun Kamble @ 2017-09-21 16:40 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx



On 9/21/2017 2:55 AM, Michal Wajdeczko wrote:
> On Wed, 20 Sep 2017 19:38:17 +0200, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>> Renamed intel_guc_suspend to intel_guc_enter_sleep and intel_guc_resume
>> to intel_guc_exit_sleep to match GuC nomenclature compatibility.
>> We plan to use intel_guc_suspend and intel_guc_resume through
>> intel_uc_suspend and intel_uc_resume in the path i915_drm_suspend and
>> i915_drm_resume respectively for better naming.
>> Also, with this patch we pass intel_guc struct as parameter to 
>> enter_sleep
>> and exit_sleep functions as they are GuC specific and they are moved to
>> intel_uc.c as static functions called from uc generic functions.
>>
>
> I'm not sure that we need this semi-refactoring right now.
> We can return to this later and do it right at once.
>
> Michal
Ok. Will return to this later. Dropping in next series.
>
>> v2: Rebase w.r.t removal of GuC code restructuring.
>>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_guc_submission.c | 52 
>> ---------------------------
>>  drivers/gpu/drm/i915/intel_uc.c            | 58 
>> ++++++++++++++++++++++++++++--
>>  drivers/gpu/drm/i915/intel_uc.h            |  2 --
>>  3 files changed, 55 insertions(+), 57 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index e191d56..94efe32 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -1205,55 +1205,3 @@ void i915_guc_submission_disable(struct 
>> drm_i915_private *dev_priv)
>>      guc_client_free(guc->execbuf_client);
>>      guc->execbuf_client = NULL;
>>  }
>> -
>> -/**
>> - * intel_guc_suspend() - notify GuC entering suspend state
>> - * @dev_priv:    i915 device private
>> - */
>> -int intel_guc_suspend(struct drm_i915_private *dev_priv)
>> -{
>> -    struct intel_guc *guc = &dev_priv->guc;
>> -    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;
>> -    /* any value greater than GUC_POWER_D0 */
>> -    data[1] = GUC_POWER_D1;
>> -    /* first page is shared data with GuC */
>> -    data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + 
>> LRC_GUCSHR_PN * PAGE_SIZE;
>> -
>> -    return intel_guc_send(guc, data, ARRAY_SIZE(data));
>> -}
>> -
>> -/**
>> - * intel_guc_resume() - notify GuC resuming from suspend state
>> - * @dev_priv:    i915 device private
>> - */
>> -int intel_guc_resume(struct drm_i915_private *dev_priv)
>> -{
>> -    struct intel_guc *guc = &dev_priv->guc;
>> -    struct i915_gem_context *ctx;
>> -    u32 data[3];
>> -
>> -    if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>> -        return 0;
>> -
>> -    if (i915.guc_log_level >= 0)
>> -        gen9_enable_guc_interrupts(dev_priv);
>> -
>> -    ctx = dev_priv->kernel_context;
>> -
>> -    data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
>> -    data[1] = GUC_POWER_D0;
>> -    /* first page is shared data with GuC */
>> -    data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + 
>> LRC_GUCSHR_PN * PAGE_SIZE;
>> -
>> -    return intel_guc_send(guc, data, ARRAY_SIZE(data));
>> -}
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 8e4d8b0..0dbb4b9 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -538,19 +538,71 @@ int intel_guc_sample_forcewake(struct intel_guc 
>> *guc)
>>      return intel_guc_send(guc, action, ARRAY_SIZE(action));
>>  }
>> +/**
>> + * intel_guc_enter_sleep() - notify GuC entering sleep state
>> + * @guc:    guc structure
>> + */
>> +static int intel_guc_enter_sleep(struct intel_guc *guc)
>> +{
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +    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;
>> +    /* any value greater than GUC_POWER_D0 */
>> +    data[1] = GUC_POWER_D1;
>> +    /* first page is shared data with GuC */
>> +    data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + 
>> LRC_GUCSHR_PN * PAGE_SIZE;
>> +
>> +    return intel_guc_send(guc, data, ARRAY_SIZE(data));
>> +}
>> +
>> +/**
>> + * intel_guc_exit_sleep() - notify GuC exit from sleep state
>> + * @guc:    guc structure
>> + */
>> +static int intel_guc_exit_sleep(struct intel_guc *guc)
>> +{
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +    struct i915_gem_context *ctx;
>> +    u32 data[3];
>> +
>> +    if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>> +        return 0;
>> +
>> +    if (i915.guc_log_level >= 0)
>> +        gen9_enable_guc_interrupts(dev_priv);
>> +
>> +    ctx = dev_priv->kernel_context;
>> +
>> +    data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
>> +    data[1] = GUC_POWER_D0;
>> +    /* first page is shared data with GuC */
>> +    data[2] = guc_ggtt_offset(ctx->engine[RCS].state) + 
>> LRC_GUCSHR_PN * PAGE_SIZE;
>> +
>> +    return intel_guc_send(guc, data, ARRAY_SIZE(data));
>> +}
>> +
>>  int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv)
>>  {
>> -    return intel_guc_suspend(dev_priv);
>> +    return intel_guc_enter_sleep(&dev_priv->guc);
>>  }
>> int intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
>>  {
>> -    return intel_guc_resume(dev_priv);
>> +    return intel_guc_exit_sleep(&dev_priv->guc);
>>  }
>> int intel_uc_suspend(struct drm_i915_private *dev_priv)
>>  {
>> -    return intel_guc_suspend(dev_priv);
>> +    return intel_guc_enter_sleep(&dev_priv->guc);
>>  }
>> int intel_uc_resume(struct drm_i915_private *dev_priv)
>> diff --git a/drivers/gpu/drm/i915/intel_uc.h 
>> b/drivers/gpu/drm/i915/intel_uc.h
>> index 3d33a51..5f49d13 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>> @@ -229,8 +229,6 @@ static inline void intel_guc_notify(struct 
>> intel_guc *guc)
>>  /* intel_guc_loader.c */
>>  int intel_guc_select_fw(struct intel_guc *guc);
>>  int intel_guc_init_hw(struct intel_guc *guc);
>> -int intel_guc_suspend(struct drm_i915_private *dev_priv);
>> -int intel_guc_resume(struct drm_i915_private *dev_priv);
>>  u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
>> /* i915_guc_submission.c */

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 3/9] drm/i915/guc: Update GuC ggtt.invalidate/interrupts/communication across RPM suspend/resume
  2017-09-20 21:33   ` Michal Wajdeczko
@ 2017-09-21 16:43     ` Sagar Arun Kamble
  0 siblings, 0 replies; 29+ messages in thread
From: Sagar Arun Kamble @ 2017-09-21 16:43 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx



On 9/21/2017 3:03 AM, Michal Wajdeczko wrote:
> On Wed, 20 Sep 2017 19:38:18 +0200, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>> Apart from configuring interrupts, we need to update the ggtt invalidate
>> interface and GuC communication on suspend. This functionality can be
>> reused for other suspend and reset paths.
>> Prepared GuC specific helpers to handle these suspend/resume tasks
>> namely - intel_guc_runtime_suspend, intel_guc_runtime_resume.
>>
>> v2: Rebase w.r.t removal of GuC code restructuring.
>>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_uc.c | 66 
>> ++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 59 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 0dbb4b9..fa698db 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -551,8 +551,6 @@ static int intel_guc_enter_sleep(struct intel_guc 
>> *guc)
>>      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;
>> @@ -577,9 +575,6 @@ static int intel_guc_exit_sleep(struct intel_guc 
>> *guc)
>>      if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>>          return 0;
>> -    if (i915.guc_log_level >= 0)
>> -        gen9_enable_guc_interrupts(dev_priv);
>> -
>>      ctx = dev_priv->kernel_context;
>>     data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
>> @@ -590,14 +585,71 @@ static int intel_guc_exit_sleep(struct 
>> intel_guc *guc)
>>      return intel_guc_send(guc, data, ARRAY_SIZE(data));
>>  }
>> +int intel_guc_runtime_suspend(struct intel_guc *guc)
>> +{
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +    int ret;
>> +
>> +    ret = intel_guc_enter_sleep(guc);
>> +    if (ret) {
>> +        DRM_ERROR("GuC enter sleep failed (%d)\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    i915_ggtt_disable_guc(dev_priv);
>> +    gen9_disable_guc_interrupts(dev_priv);
>
> To match existing approach in intel_uc_init_hw() move interrupts
> control to uc_runtime_suspend()
Yes. Will update as suggested.
>
>> +
>> +    return 0;
>> +}
>> +
>> +int intel_guc_runtime_resume(struct intel_guc *guc)
>> +{
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +    int ret;
>> +
>> +    if (i915.guc_log_level >= 0)
>> +        gen9_enable_guc_interrupts(dev_priv);
>
> Similar here
Yes. Will update as suggested.
>
>> +    i915_ggtt_enable_guc(dev_priv);
>> +
>> +    ret = intel_guc_exit_sleep(guc);
>> +    if (ret) {
>> +        DRM_ERROR("GuC exit sleep failed (%d)\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv)
>>  {
>> -    return intel_guc_enter_sleep(&dev_priv->guc);
>> +    int ret;
>> +
>> +    if (!i915.enable_guc_loading)
>> +        return 0;
>> +
>> +    ret = intel_guc_runtime_suspend(&dev_priv->guc);
>> +    if (ret)
>> +        return ret;
>> +
>> +    guc_disable_communication(&dev_priv->guc);
>> +
>> +    return 0;
>>  }
>> int intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
>>  {
>> -    return intel_guc_exit_sleep(&dev_priv->guc);
>> +    int ret;
>> +
>> +    if (!i915.enable_guc_loading)
>> +        return 0;
>> +
>> +    ret = guc_enable_communication(&dev_priv->guc);
>> +    if (ret) {
>> +        DRM_ERROR("GuC enable communication failed (%d)\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    return intel_guc_runtime_resume(&dev_priv->guc);
>>  }
>> int intel_uc_suspend(struct drm_i915_private *dev_priv)

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 4/9] drm/i915/guc: Update suspend functionality in intel_uc_suspend path
  2017-09-20 17:38 ` [PATCH v4 4/9] drm/i915/guc: Update suspend functionality in intel_uc_suspend path Sagar Arun Kamble
@ 2017-09-21 16:46   ` Michał Winiarski
  2017-09-21 16:55     ` Sagar Arun Kamble
  0 siblings, 1 reply; 29+ messages in thread
From: Michał Winiarski @ 2017-09-21 16:46 UTC (permalink / raw)
  To: Sagar Arun Kamble; +Cc: intel-gfx

On Wed, Sep 20, 2017 at 11:08:19PM +0530, Sagar Arun Kamble wrote:
> With this patch we disable GuC submission in i915_drm_suspend. This will
> destroy the client which will be setup back again. We also reuse the
> complete sanitization done via intel_uc_runtime_suspend in this path.
> Post drm resume this state is recreated by intel_uc_init_hw hence we need
> not have similar reuse for intel_uc_resume.
> This also fixes issue where intel_uc_fini_hw was being called after GPU
> reset happening in i915_gem_suspend in i915_driver_unload.
> 
> v2: Rebase w.r.t removal of GuC code restructuring. Added struct_mutex
> protection for i915_guc_submission_disable.
> 
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uc.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index fa698db..0c7e45c7 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -446,9 +446,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>  	if (!i915.enable_guc_loading)
>  		return;
>  
> -	if (i915.enable_guc_submission)
> -		i915_guc_submission_disable(dev_priv);
> -
>  	guc_disable_communication(&dev_priv->guc);
>  
>  	if (i915.enable_guc_submission) {
> @@ -654,7 +651,20 @@ int intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
>  
>  int intel_uc_suspend(struct drm_i915_private *dev_priv)
>  {
> -	return intel_guc_enter_sleep(&dev_priv->guc);
> +	struct drm_device *dev = &dev_priv->drm;
> +	int ret;

Drop the locals.
I'd drop the dev, and just go through dev_priv in this case.

> +
> +	if (i915.enable_guc_submission) {
> +		mutex_lock(&dev->struct_mutex);
> +		i915_guc_submission_disable(dev_priv);
> +		mutex_unlock(&dev->struct_mutex);
> +	}

Since we're starting to use i915_guc_submission_disable from different places,
some of which without struct_mutex already held (like this one), it would be a
good idea to add a lockdep assert documenting this requirement inside both
i915_guc_submission_enable and i915_guc_submission_disable.
It could be even squeezed in with this patch IMHO.

> +
> +	ret = intel_uc_runtime_suspend(dev_priv);
> +	if (ret)
> +		return ret;

return intel_guc_runtime_suspend(dev_priv);

With that:

Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>

-Michał

> +
> +	return 0;
>  }
>  
>  int intel_uc_resume(struct drm_i915_private *dev_priv)
> -- 
> 1.9.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 4/9] drm/i915/guc: Update suspend functionality in intel_uc_suspend path
  2017-09-21 16:46   ` Michał Winiarski
@ 2017-09-21 16:55     ` Sagar Arun Kamble
  2017-09-21 20:52       ` Michał Winiarski
  0 siblings, 1 reply; 29+ messages in thread
From: Sagar Arun Kamble @ 2017-09-21 16:55 UTC (permalink / raw)
  To: Michał Winiarski; +Cc: intel-gfx



On 9/21/2017 10:16 PM, Michał Winiarski wrote:
> On Wed, Sep 20, 2017 at 11:08:19PM +0530, Sagar Arun Kamble wrote:
>> With this patch we disable GuC submission in i915_drm_suspend. This will
>> destroy the client which will be setup back again. We also reuse the
>> complete sanitization done via intel_uc_runtime_suspend in this path.
>> Post drm resume this state is recreated by intel_uc_init_hw hence we need
>> not have similar reuse for intel_uc_resume.
>> This also fixes issue where intel_uc_fini_hw was being called after GPU
>> reset happening in i915_gem_suspend in i915_driver_unload.
>>
>> v2: Rebase w.r.t removal of GuC code restructuring. Added struct_mutex
>> protection for i915_guc_submission_disable.
>>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_uc.c | 18 ++++++++++++++----
>>   1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
>> index fa698db..0c7e45c7 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -446,9 +446,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>>   	if (!i915.enable_guc_loading)
>>   		return;
>>   
>> -	if (i915.enable_guc_submission)
>> -		i915_guc_submission_disable(dev_priv);
>> -
>>   	guc_disable_communication(&dev_priv->guc);
>>   
>>   	if (i915.enable_guc_submission) {
>> @@ -654,7 +651,20 @@ int intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
>>   
>>   int intel_uc_suspend(struct drm_i915_private *dev_priv)
>>   {
>> -	return intel_guc_enter_sleep(&dev_priv->guc);
>> +	struct drm_device *dev = &dev_priv->drm;
>> +	int ret;
> Drop the locals.
> I'd drop the dev, and just go through dev_priv in this case.
Sure. Will update.
>
>> +
>> +	if (i915.enable_guc_submission) {
>> +		mutex_lock(&dev->struct_mutex);
>> +		i915_guc_submission_disable(dev_priv);
>> +		mutex_unlock(&dev->struct_mutex);
>> +	}
> Since we're starting to use i915_guc_submission_disable from different places,
> some of which without struct_mutex already held (like this one), it would be a
> good idea to add a lockdep assert documenting this requirement inside both
> i915_guc_submission_enable and i915_guc_submission_disable.
> It could be even squeezed in with this patch IMHO.
Sure. Will update.
>
>> +
>> +	ret = intel_uc_runtime_suspend(dev_priv);
>> +	if (ret)
>> +		return ret;
> return intel_guc_runtime_suspend(dev_priv);
Did you really mean intel_*guc*_runtime_suspend here or was typo?
>
> With that:
>
> Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
>
> -Michał
>
>> +
>> +	return 0;
>>   }
>>   
>>   int intel_uc_resume(struct drm_i915_private *dev_priv)
>> -- 
>> 1.9.1
>>

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 1/9] drm/i915: Create uc runtime and system suspend/resume helpers
  2017-09-20 17:38 ` [PATCH v4 1/9] drm/i915: Create uc runtime and system suspend/resume helpers Sagar Arun Kamble
@ 2017-09-21 17:16   ` Michał Winiarski
  2017-09-21 17:26     ` Sagar Arun Kamble
  0 siblings, 1 reply; 29+ messages in thread
From: Michał Winiarski @ 2017-09-21 17:16 UTC (permalink / raw)
  To: Sagar Arun Kamble; +Cc: intel-gfx

On Wed, Sep 20, 2017 at 11:08:16PM +0530, Sagar Arun Kamble wrote:
> Prepared generic helpers intel_uc_suspend, intel_uc_resume,
> intel_uc_runtime_suspend, intel_uc_runtime_resume. Added
> error handling to all the calls for suspend/resume.

I find the error handling done this way quite surprising...
More below.

> 
> v2: Rebase w.r.t removal of GuC code restructuring.
> 
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 23 ++++++++++++++++++++---
>  drivers/gpu/drm/i915/i915_gem.c |  7 ++++++-
>  drivers/gpu/drm/i915/intel_uc.c | 20 ++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uc.h |  4 ++++
>  4 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 5c111ea..8635f40 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1691,7 +1691,15 @@ static int i915_drm_resume(struct drm_device *dev)
>  	}
>  	mutex_unlock(&dev->struct_mutex);
>  
> -	intel_guc_resume(dev_priv);
> +	/*
> +	 * NB: Full gem reinitialization is being done above, hence
> +	 * intel_uc_resume will be of no use. Currently intel_uc_resume
> +	 * is nop. If full reinitialization is removed, will  need to put
> +	 * functionality to resume from sleep in intel_uc_resume.
> +	 */
> +	ret = intel_uc_resume(dev_priv);
> +	if (ret)
> +		DRM_ERROR("failed to resume uc\n");
>  
>  	intel_modeset_init_hw(dev);
>  
> @@ -2493,7 +2501,12 @@ static int intel_runtime_suspend(struct device *kdev)
>  	 */
>  	i915_gem_runtime_suspend(dev_priv);
>  
> -	intel_guc_suspend(dev_priv);
> +	ret = intel_uc_runtime_suspend(dev_priv);
> +	if (ret) {
> +		DRM_ERROR("uc runtime suspend failed, disabling it(%d)\n", ret);
> +		enable_rpm_wakeref_asserts(dev_priv);
> +		return ret;

Early exit?

> +	}
>  
>  	intel_runtime_pm_disable_interrupts(dev_priv);
>  
> @@ -2578,7 +2591,11 @@ static int intel_runtime_resume(struct device *kdev)
>  	if (intel_uncore_unclaimed_mmio(dev_priv))
>  		DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
>  
> -	intel_guc_resume(dev_priv);
> +	ret = intel_uc_runtime_resume(dev_priv);
> +	if (ret) {
> +		DRM_ERROR("uc runtime resume failed (%d)\n", ret);
> +		return ret;

Same here.

> +	}
>  
>  	if (IS_GEN9_LP(dev_priv)) {
>  		bxt_disable_dc9(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c4bf348..dd56d45 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4539,7 +4539,11 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>  	i915_gem_contexts_lost(dev_priv);
>  	mutex_unlock(&dev->struct_mutex);
>  
> -	intel_guc_suspend(dev_priv);
> +	ret = intel_uc_suspend(dev_priv);
> +	if (ret) {
> +		DRM_ERROR("uc suspend failed (%d)\n", ret);
> +		goto out;

Should we really exit early if GuC sleep action fails?

In all of those cases - is this really something critical? Shouldn't we go
through the rest of the suspend/resume dance even if we fail to talk with GuC?

-Michał

> +	}
>  
>  	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
>  	cancel_delayed_work_sync(&dev_priv->gt.retire_work);
> @@ -4583,6 +4587,7 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>  
>  err_unlock:
>  	mutex_unlock(&dev->struct_mutex);
> +out:
>  	intel_runtime_pm_put(dev_priv);
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 0178ba4..8e4d8b0 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -537,3 +537,23 @@ int intel_guc_sample_forcewake(struct intel_guc *guc)
>  
>  	return intel_guc_send(guc, action, ARRAY_SIZE(action));
>  }
> +
> +int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv)
> +{
> +	return intel_guc_suspend(dev_priv);
> +}
> +
> +int intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
> +{
> +	return intel_guc_resume(dev_priv);
> +}
> +
> +int intel_uc_suspend(struct drm_i915_private *dev_priv)
> +{
> +	return intel_guc_suspend(dev_priv);
> +}
> +
> +int intel_uc_resume(struct drm_i915_private *dev_priv)
> +{
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 7703c9a..3d33a51 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -208,6 +208,10 @@ struct intel_huc {
>  void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
>  int intel_uc_init_hw(struct drm_i915_private *dev_priv);
>  void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
> +int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv);
> +int intel_uc_runtime_resume(struct drm_i915_private *dev_priv);
> +int intel_uc_suspend(struct drm_i915_private *dev_priv);
> +int intel_uc_resume(struct drm_i915_private *dev_priv);
>  int intel_guc_sample_forcewake(struct intel_guc *guc);
>  int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
>  int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
> -- 
> 1.9.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 1/9] drm/i915: Create uc runtime and system suspend/resume helpers
  2017-09-21 17:16   ` Michał Winiarski
@ 2017-09-21 17:26     ` Sagar Arun Kamble
  0 siblings, 0 replies; 29+ messages in thread
From: Sagar Arun Kamble @ 2017-09-21 17:26 UTC (permalink / raw)
  To: Michał Winiarski; +Cc: intel-gfx



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

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 7/9] drm/i915/guc: Remove i915_guc_log_unregister
  2017-09-20 20:58   ` Michal Wajdeczko
@ 2017-09-21 17:31     ` Sagar Arun Kamble
  2017-09-21 17:40       ` Sagar Arun Kamble
  0 siblings, 1 reply; 29+ messages in thread
From: Sagar Arun Kamble @ 2017-09-21 17:31 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx



On 9/21/2017 2:28 AM, Michal Wajdeczko wrote:
> On Wed, 20 Sep 2017 19:38:22 +0200, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>> Functionality needed to disable GuC interrupts and cleanup the
>> runtime/relay data structures is already covered in the unload path
>> via intel_guc_fini_hw and intel_guc_cleanup hence remove
>> i915_guc_log_unregister
>>
>> v2: Removed the function i915_guc_log_unregister.
>>
>> v3: Rebase as intel_guc.h is removed.
>>
>> v4: Rebase as intel_guc.h is created again. :)
>>
>> v5: Rebase as intel_guc.h is removed.
>>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>
> I'm afraid I've to revoke my r-b as with removal of the log_unregister()
> we will loose symmetry with log_register() where relay_late_setup_files()
> was hidden, and we should still clean it up in i915_driver_unregister()
>
> Michal
Agree with you. We will need to maintain symmetry. Will revisit with 
separate patches.
Better to integrate in the CT series I guess as there are multiple 
logging related
interrupt enable/disable occurrences.

>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.c      |  1 -
>>  drivers/gpu/drm/i915/intel_guc_log.c | 12 ------------
>>  drivers/gpu/drm/i915/intel_uc.h      |  1 -
>>  3 files changed, 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index 6f36ced..c69a30a 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1252,7 +1252,6 @@ static void i915_driver_unregister(struct 
>> drm_i915_private *dev_priv)
>>      i915_perf_unregister(dev_priv);
>>     i915_teardown_sysfs(dev_priv);
>> -    i915_guc_log_unregister(dev_priv);
>>      drm_dev_unregister(&dev_priv->drm);
>>     i915_gem_shrinker_cleanup(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index 16d3b87..3c45681 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -648,15 +648,3 @@ void i915_guc_log_register(struct 
>> drm_i915_private *dev_priv)
>>      guc_log_late_setup(&dev_priv->guc);
>>      mutex_unlock(&dev_priv->drm.struct_mutex);
>>  }
>> -
>> -void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>> -{
>> -    if (!i915.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);
>> -    guc_log_runtime_destroy(&dev_priv->guc);
>> -    mutex_unlock(&dev_priv->drm.struct_mutex);
>> -}
>> diff --git a/drivers/gpu/drm/i915/intel_uc.h 
>> b/drivers/gpu/drm/i915/intel_uc.h
>> index 8557e33..c2c104a 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>> @@ -244,7 +244,6 @@ static inline void intel_guc_notify(struct 
>> intel_guc *guc)
>>  void intel_guc_log_destroy(struct intel_guc *guc);
>>  int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 
>> control_val);
>>  void i915_guc_log_register(struct drm_i915_private *dev_priv);
>> -void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
>> static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>>  {

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 7/9] drm/i915/guc: Remove i915_guc_log_unregister
  2017-09-21 17:31     ` Sagar Arun Kamble
@ 2017-09-21 17:40       ` Sagar Arun Kamble
  2017-09-21 20:39         ` Michal Wajdeczko
  0 siblings, 1 reply; 29+ messages in thread
From: Sagar Arun Kamble @ 2017-09-21 17:40 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx



On 9/21/2017 11:01 PM, Sagar Arun Kamble wrote:
>
>
> On 9/21/2017 2:28 AM, Michal Wajdeczko wrote:
>> On Wed, 20 Sep 2017 19:38:22 +0200, Sagar Arun Kamble 
>> <sagar.a.kamble@intel.com> wrote:
>>
>>> Functionality needed to disable GuC interrupts and cleanup the
>>> runtime/relay data structures is already covered in the unload path
>>> via intel_guc_fini_hw and intel_guc_cleanup hence remove
>>> i915_guc_log_unregister
>>>
>>> v2: Removed the function i915_guc_log_unregister.
>>>
>>> v3: Rebase as intel_guc.h is removed.
>>>
>>> v4: Rebase as intel_guc.h is created again. :)
>>>
>>> v5: Rebase as intel_guc.h is removed.
>>>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>
>> I'm afraid I've to revoke my r-b as with removal of the log_unregister()
>> we will loose symmetry with log_register() where 
>> relay_late_setup_files()
>> was hidden, and we should still clean it up in i915_driver_unregister()
>>
>> Michal
> Agree with you. We will need to maintain symmetry. Will revisit with 
> separate patches.
> Better to integrate in the CT series I guess as there are multiple 
> logging related
> interrupt enable/disable occurrences.
Without this patch we are getting RPM WARN which I had floated patch 
initally for.
https://patchwork.freedesktop.org/patch/176538/
Kindly let me know if we should get that patch in till we fix the 
logging/CT interrupt interaction.

[173707.659868] RPM wakelock ref not held during HW access
[173707.659897] ------------[ cut here ]------------
[173707.659969] WARNING: CPU: 1 PID: 20769 at 
drivers/gpu/drm/i915/intel_drv.h:1800 fwtable_write32+0x1ad/0x1d0 [i915]
[173707.660228]  gen6_disable_pm_irq+0x47/0x50 [i915]
[173707.660269]  gen9_disable_guc_interrupts+0x33/0x60 [i915]
[173707.660332]  i915_guc_log_unregister+0x2e/0x50 [i915]
[173707.660373]  i915_driver_unload+0x4d/0x180 [i915]
[173707.660415]  i915_pci_remove+0x19/0x30 [i915]
[173707.660423]  pci_device_remove+0x39/0xc0

>
>
>>
>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_drv.c      |  1 -
>>>  drivers/gpu/drm/i915/intel_guc_log.c | 12 ------------
>>>  drivers/gpu/drm/i915/intel_uc.h      |  1 -
>>>  3 files changed, 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>>> b/drivers/gpu/drm/i915/i915_drv.c
>>> index 6f36ced..c69a30a 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> @@ -1252,7 +1252,6 @@ static void i915_driver_unregister(struct 
>>> drm_i915_private *dev_priv)
>>>      i915_perf_unregister(dev_priv);
>>>     i915_teardown_sysfs(dev_priv);
>>> -    i915_guc_log_unregister(dev_priv);
>>>      drm_dev_unregister(&dev_priv->drm);
>>>     i915_gem_shrinker_cleanup(dev_priv);
>>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
>>> b/drivers/gpu/drm/i915/intel_guc_log.c
>>> index 16d3b87..3c45681 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>>> @@ -648,15 +648,3 @@ void i915_guc_log_register(struct 
>>> drm_i915_private *dev_priv)
>>>      guc_log_late_setup(&dev_priv->guc);
>>>      mutex_unlock(&dev_priv->drm.struct_mutex);
>>>  }
>>> -
>>> -void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>>> -{
>>> -    if (!i915.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);
>>> -    guc_log_runtime_destroy(&dev_priv->guc);
>>> -    mutex_unlock(&dev_priv->drm.struct_mutex);
>>> -}
>>> diff --git a/drivers/gpu/drm/i915/intel_uc.h 
>>> b/drivers/gpu/drm/i915/intel_uc.h
>>> index 8557e33..c2c104a 100644
>>> --- a/drivers/gpu/drm/i915/intel_uc.h
>>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>>> @@ -244,7 +244,6 @@ static inline void intel_guc_notify(struct 
>>> intel_guc *guc)
>>>  void intel_guc_log_destroy(struct intel_guc *guc);
>>>  int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 
>>> control_val);
>>>  void i915_guc_log_register(struct drm_i915_private *dev_priv);
>>> -void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
>>> static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>>>  {
>

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 7/9] drm/i915/guc: Remove i915_guc_log_unregister
  2017-09-21 17:40       ` Sagar Arun Kamble
@ 2017-09-21 20:39         ` Michal Wajdeczko
  2017-09-22  4:22           ` Sagar Arun Kamble
  0 siblings, 1 reply; 29+ messages in thread
From: Michal Wajdeczko @ 2017-09-21 20:39 UTC (permalink / raw)
  To: intel-gfx, Sagar Arun Kamble

On Thu, 21 Sep 2017 19:40:04 +0200, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

>
>
> On 9/21/2017 11:01 PM, Sagar Arun Kamble wrote:
>>
>>
>> On 9/21/2017 2:28 AM, Michal Wajdeczko wrote:
>>> On Wed, 20 Sep 2017 19:38:22 +0200, Sagar Arun Kamble  
>>> <sagar.a.kamble@intel.com> wrote:
>>>
>>>> Functionality needed to disable GuC interrupts and cleanup the
>>>> runtime/relay data structures is already covered in the unload path
>>>> via intel_guc_fini_hw and intel_guc_cleanup hence remove
>>>> i915_guc_log_unregister
>>>>
>>>> v2: Removed the function i915_guc_log_unregister.
>>>>
>>>> v3: Rebase as intel_guc.h is removed.
>>>>
>>>> v4: Rebase as intel_guc.h is created again. :)
>>>>
>>>> v5: Rebase as intel_guc.h is removed.
>>>>
>>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>
>>> I'm afraid I've to revoke my r-b as with removal of the  
>>> log_unregister()
>>> we will loose symmetry with log_register() where  
>>> relay_late_setup_files()
>>> was hidden, and we should still clean it up in i915_driver_unregister()
>>>
>>> Michal
>> Agree with you. We will need to maintain symmetry. Will revisit with  
>> separate patches.
>> Better to integrate in the CT series I guess as there are multiple  
>> logging related
>> interrupt enable/disable occurrences.
> Without this patch we are getting RPM WARN which I had floated patch  
> initally for.
> https://patchwork.freedesktop.org/patch/176538/
> Kindly let me know if we should get that patch in till we fix the  
> logging/CT interrupt interaction.
>

Can we only call relay_close() here (without any irq/runtime cleanup,
as that will be covered in unload path) - that way we will keep
symmetry and avoid RPM issue ...

Michal

> [173707.659868] RPM wakelock ref not held during HW access
> [173707.659897] ------------[ cut here ]------------
> [173707.659969] WARNING: CPU: 1 PID: 20769 at  
> drivers/gpu/drm/i915/intel_drv.h:1800 fwtable_write32+0x1ad/0x1d0 [i915]
> [173707.660228]  gen6_disable_pm_irq+0x47/0x50 [i915]
> [173707.660269]  gen9_disable_guc_interrupts+0x33/0x60 [i915]
> [173707.660332]  i915_guc_log_unregister+0x2e/0x50 [i915]
> [173707.660373]  i915_driver_unload+0x4d/0x180 [i915]
> [173707.660415]  i915_pci_remove+0x19/0x30 [i915]
> [173707.660423]  pci_device_remove+0x39/0xc0
>
>>
>>
>>>
>>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/i915_drv.c      |  1 -
>>>>  drivers/gpu/drm/i915/intel_guc_log.c | 12 ------------
>>>>  drivers/gpu/drm/i915/intel_uc.h      |  1 -
>>>>  3 files changed, 14 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c  
>>>> b/drivers/gpu/drm/i915/i915_drv.c
>>>> index 6f36ced..c69a30a 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>>> @@ -1252,7 +1252,6 @@ static void i915_driver_unregister(struct  
>>>> drm_i915_private *dev_priv)
>>>>      i915_perf_unregister(dev_priv);
>>>>     i915_teardown_sysfs(dev_priv);
>>>> -    i915_guc_log_unregister(dev_priv);
>>>>      drm_dev_unregister(&dev_priv->drm);
>>>>     i915_gem_shrinker_cleanup(dev_priv);
>>>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c  
>>>> b/drivers/gpu/drm/i915/intel_guc_log.c
>>>> index 16d3b87..3c45681 100644
>>>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>>>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>>>> @@ -648,15 +648,3 @@ void i915_guc_log_register(struct  
>>>> drm_i915_private *dev_priv)
>>>>      guc_log_late_setup(&dev_priv->guc);
>>>>      mutex_unlock(&dev_priv->drm.struct_mutex);
>>>>  }
>>>> -
>>>> -void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>>>> -{
>>>> -    if (!i915.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);
>>>> -    guc_log_runtime_destroy(&dev_priv->guc);
>>>> -    mutex_unlock(&dev_priv->drm.struct_mutex);
>>>> -}
>>>> diff --git a/drivers/gpu/drm/i915/intel_uc.h  
>>>> b/drivers/gpu/drm/i915/intel_uc.h
>>>> index 8557e33..c2c104a 100644
>>>> --- a/drivers/gpu/drm/i915/intel_uc.h
>>>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>>>> @@ -244,7 +244,6 @@ static inline void intel_guc_notify(struct  
>>>> intel_guc *guc)
>>>>  void intel_guc_log_destroy(struct intel_guc *guc);
>>>>  int i915_guc_log_control(struct drm_i915_private *dev_priv, u64  
>>>> control_val);
>>>>  void i915_guc_log_register(struct drm_i915_private *dev_priv);
>>>> -void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
>>>> static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>>>>  {
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 4/9] drm/i915/guc: Update suspend functionality in intel_uc_suspend path
  2017-09-21 16:55     ` Sagar Arun Kamble
@ 2017-09-21 20:52       ` Michał Winiarski
  0 siblings, 0 replies; 29+ messages in thread
From: Michał Winiarski @ 2017-09-21 20:52 UTC (permalink / raw)
  To: Sagar Arun Kamble; +Cc: intel-gfx

On Thu, Sep 21, 2017 at 10:25:50PM +0530, Sagar Arun Kamble wrote:

[SNIP]

> > > +	ret = intel_uc_runtime_suspend(dev_priv);
> > > +	if (ret)
> > > +		return ret;
> > return intel_guc_runtime_suspend(dev_priv);
> Did you really mean intel_*guc*_runtime_suspend here or was typo?

Typo - sorry about that.

-Michał

> > 
> > With that:
> > 
> > Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
> > 
> > -Michał
> > 
> > > +
> > > +	return 0;
> > >   }
> > >   int intel_uc_resume(struct drm_i915_private *dev_priv)
> > > -- 
> > > 1.9.1
> > > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 7/9] drm/i915/guc: Remove i915_guc_log_unregister
  2017-09-21 20:39         ` Michal Wajdeczko
@ 2017-09-22  4:22           ` Sagar Arun Kamble
  0 siblings, 0 replies; 29+ messages in thread
From: Sagar Arun Kamble @ 2017-09-22  4:22 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx



On 9/22/2017 2:09 AM, Michal Wajdeczko wrote:
> On Thu, 21 Sep 2017 19:40:04 +0200, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>>
>>
>> On 9/21/2017 11:01 PM, Sagar Arun Kamble wrote:
>>>
>>>
>>> On 9/21/2017 2:28 AM, Michal Wajdeczko wrote:
>>>> On Wed, 20 Sep 2017 19:38:22 +0200, Sagar Arun Kamble 
>>>> <sagar.a.kamble@intel.com> wrote:
>>>>
>>>>> Functionality needed to disable GuC interrupts and cleanup the
>>>>> runtime/relay data structures is already covered in the unload path
>>>>> via intel_guc_fini_hw and intel_guc_cleanup hence remove
>>>>> i915_guc_log_unregister
>>>>>
>>>>> v2: Removed the function i915_guc_log_unregister.
>>>>>
>>>>> v3: Rebase as intel_guc.h is removed.
>>>>>
>>>>> v4: Rebase as intel_guc.h is created again. :)
>>>>>
>>>>> v5: Rebase as intel_guc.h is removed.
>>>>>
>>>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>>> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>>
>>>> I'm afraid I've to revoke my r-b as with removal of the 
>>>> log_unregister()
>>>> we will loose symmetry with log_register() where 
>>>> relay_late_setup_files()
>>>> was hidden, and we should still clean it up in 
>>>> i915_driver_unregister()
>>>>
>>>> Michal
>>> Agree with you. We will need to maintain symmetry. Will revisit with 
>>> separate patches.
>>> Better to integrate in the CT series I guess as there are multiple 
>>> logging related
>>> interrupt enable/disable occurrences.
>> Without this patch we are getting RPM WARN which I had floated patch 
>> initally for.
>> https://patchwork.freedesktop.org/patch/176538/
>> Kindly let me know if we should get that patch in till we fix the 
>> logging/CT interrupt interaction.
>>
>
> Can we only call relay_close() here (without any irq/runtime cleanup,
> as that will be covered in unload path) - that way we will keep
> symmetry and avoid RPM issue ...
>
> Michal
Yes. Will need to handle the sync. between unload closing the relay_chan 
and reads from relay_chan.
Will share patch for this.
>
>> [173707.659868] RPM wakelock ref not held during HW access
>> [173707.659897] ------------[ cut here ]------------
>> [173707.659969] WARNING: CPU: 1 PID: 20769 at 
>> drivers/gpu/drm/i915/intel_drv.h:1800 fwtable_write32+0x1ad/0x1d0 [i915]
>> [173707.660228]  gen6_disable_pm_irq+0x47/0x50 [i915]
>> [173707.660269]  gen9_disable_guc_interrupts+0x33/0x60 [i915]
>> [173707.660332]  i915_guc_log_unregister+0x2e/0x50 [i915]
>> [173707.660373]  i915_driver_unload+0x4d/0x180 [i915]
>> [173707.660415]  i915_pci_remove+0x19/0x30 [i915]
>> [173707.660423]  pci_device_remove+0x39/0xc0
>>
>>>
>>>
>>>>
>>>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>>>> ---
>>>>>  drivers/gpu/drm/i915/i915_drv.c      |  1 -
>>>>>  drivers/gpu/drm/i915/intel_guc_log.c | 12 ------------
>>>>>  drivers/gpu/drm/i915/intel_uc.h      |  1 -
>>>>>  3 files changed, 14 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>>>>> b/drivers/gpu/drm/i915/i915_drv.c
>>>>> index 6f36ced..c69a30a 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>>>> @@ -1252,7 +1252,6 @@ static void i915_driver_unregister(struct 
>>>>> drm_i915_private *dev_priv)
>>>>>      i915_perf_unregister(dev_priv);
>>>>>     i915_teardown_sysfs(dev_priv);
>>>>> -    i915_guc_log_unregister(dev_priv);
>>>>>      drm_dev_unregister(&dev_priv->drm);
>>>>>     i915_gem_shrinker_cleanup(dev_priv);
>>>>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
>>>>> b/drivers/gpu/drm/i915/intel_guc_log.c
>>>>> index 16d3b87..3c45681 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>>>>> @@ -648,15 +648,3 @@ void i915_guc_log_register(struct 
>>>>> drm_i915_private *dev_priv)
>>>>>      guc_log_late_setup(&dev_priv->guc);
>>>>>      mutex_unlock(&dev_priv->drm.struct_mutex);
>>>>>  }
>>>>> -
>>>>> -void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>>>>> -{
>>>>> -    if (!i915.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);
>>>>> -    guc_log_runtime_destroy(&dev_priv->guc);
>>>>> -    mutex_unlock(&dev_priv->drm.struct_mutex);
>>>>> -}
>>>>> diff --git a/drivers/gpu/drm/i915/intel_uc.h 
>>>>> b/drivers/gpu/drm/i915/intel_uc.h
>>>>> index 8557e33..c2c104a 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_uc.h
>>>>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>>>>> @@ -244,7 +244,6 @@ static inline void intel_guc_notify(struct 
>>>>> intel_guc *guc)
>>>>>  void intel_guc_log_destroy(struct intel_guc *guc);
>>>>>  int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 
>>>>> control_val);
>>>>>  void i915_guc_log_register(struct drm_i915_private *dev_priv);
>>>>> -void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
>>>>> static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>>>>>  {
>>>

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, other threads:[~2017-09-22  4:22 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-20 17:38 [PATCH v4 0/9] GuC Fixes, Minor restructuring changes and v9+ logging change Sagar Arun Kamble
2017-09-20 17:38 ` [PATCH v4 1/9] drm/i915: Create uc runtime and system suspend/resume helpers Sagar Arun Kamble
2017-09-21 17:16   ` Michał Winiarski
2017-09-21 17:26     ` Sagar Arun Kamble
2017-09-20 17:38 ` [PATCH v4 2/9] drm/i915/guc: Update prototype/name of GuC suspend/resume fns and move to intel_uc.c Sagar Arun Kamble
2017-09-20 21:25   ` Michal Wajdeczko
2017-09-21 16:40     ` Sagar Arun Kamble
2017-09-20 17:38 ` [PATCH v4 3/9] drm/i915/guc: Update GuC ggtt.invalidate/interrupts/communication across RPM suspend/resume Sagar Arun Kamble
2017-09-20 21:33   ` Michal Wajdeczko
2017-09-21 16:43     ` Sagar Arun Kamble
2017-09-20 17:38 ` [PATCH v4 4/9] drm/i915/guc: Update suspend functionality in intel_uc_suspend path Sagar Arun Kamble
2017-09-21 16:46   ` Michał Winiarski
2017-09-21 16:55     ` Sagar Arun Kamble
2017-09-21 20:52       ` Michał Winiarski
2017-09-20 17:38 ` [PATCH v4 5/9] drm/i915/guc: Disable GuC submission and suspend it prior to i915 reset Sagar Arun Kamble
2017-09-20 21:45   ` Michal Wajdeczko
2017-09-20 17:38 ` [PATCH v4 6/9] drm/i915/guc: Fix GuC cleanup in unload path Sagar Arun Kamble
2017-09-20 21:53   ` Michal Wajdeczko
2017-09-20 17:38 ` [PATCH v4 7/9] drm/i915/guc: Remove i915_guc_log_unregister Sagar Arun Kamble
2017-09-20 20:58   ` Michal Wajdeczko
2017-09-21 17:31     ` Sagar Arun Kamble
2017-09-21 17:40       ` Sagar Arun Kamble
2017-09-21 20:39         ` Michal Wajdeczko
2017-09-22  4:22           ` Sagar Arun Kamble
2017-09-20 17:38 ` [PATCH v4 8/9] drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9 Sagar Arun Kamble
2017-09-20 17:38 ` [PATCH v4 9/9] drm/i915: Reorganize HuC authentication Sagar Arun Kamble
2017-09-20 18:47 ` ✓ Fi.CI.BAT: success for GuC Fixes, Minor restructuring changes and v9+ logging change Patchwork
2017-09-20 20:55 ` ✗ Fi.CI.IGT: failure " Patchwork
2017-09-21  3:11   ` Kamble, Sagar A

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.