All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/5] drm/i915/guc: Separate GuC doorbell destroy function into doorbell unit and GuC task
@ 2017-11-05 13:39 Sagar Arun Kamble
  2017-11-05 13:39 ` [PATCH 2/5] drm/i915/guc: Release all client doorbells on suspend and acquire on resume Sagar Arun Kamble
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Sagar Arun Kamble @ 2017-11-05 13:39 UTC (permalink / raw)
  To: intel-gfx

Releasing GuC doorbell involves stopping the doorbell unit snooping and
executing doorbell deallocation flow in GuC firmware. When GuC is in sane
state, both steps will be necessary to release doorbell.
If GuC is hung (possibly leading to i915 reset), only doorbell unit
snooping is to be stopped. destroy_doorbell will be called in suspend and
reset flows differently in upcoming patches.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index d14c134..b6486dc 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -207,7 +207,7 @@ static int __create_doorbell(struct i915_guc_client *client)
 	return err;
 }
 
-static int __destroy_doorbell(struct i915_guc_client *client)
+static int __destroy_doorbell(struct i915_guc_client *client, bool notify_guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(client->guc);
 	struct guc_doorbell_info *doorbell;
@@ -225,7 +225,10 @@ static int __destroy_doorbell(struct i915_guc_client *client)
 	if (wait_for_us(!(I915_READ(GEN8_DRBREGL(db_id)) & GEN8_DRB_VALID), 10))
 		WARN_ONCE(true, "Doorbell never became invalid after disable\n");
 
-	return __guc_deallocate_doorbell(client->guc, client->stage_id);
+	if (notify_guc)
+		return __guc_deallocate_doorbell(client->guc, client->stage_id);
+
+	return 0;
 }
 
 static int create_doorbell(struct i915_guc_client *client)
@@ -250,7 +253,11 @@ static int create_doorbell(struct i915_guc_client *client)
 	return ret;
 }
 
-static int destroy_doorbell(struct i915_guc_client *client)
+/*
+ * This function deactivates doorbell monitoring through doorbell unit and
+ * optionally notifies GuC to deallocate the doorbell.
+ */
+static int destroy_doorbell(struct i915_guc_client *client, bool notify_guc)
 {
 	int err;
 
@@ -259,7 +266,7 @@ static int destroy_doorbell(struct i915_guc_client *client)
 	/* XXX: wait for any interrupts */
 	/* XXX: wait for workqueue to drain */
 
-	err = __destroy_doorbell(client);
+	err = __destroy_doorbell(client, notify_guc);
 	if (err)
 		return err;
 
@@ -873,7 +880,7 @@ static int __reset_doorbell(struct i915_guc_client* client, u16 db_id)
 	__update_doorbell_desc(client, db_id);
 	err = __create_doorbell(client);
 	if (!err)
-		err = __destroy_doorbell(client);
+		err = __destroy_doorbell(client, true);
 
 	return err;
 }
@@ -899,7 +906,7 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
 
 		if (has_doorbell(client)) {
 			/* Borrow execbuf_client (we will recreate it later) */
-			destroy_doorbell(client);
+			destroy_doorbell(client, true);
 			recreate_first_client = true;
 		}
 
@@ -925,7 +932,7 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
 
 	ret = __create_doorbell(guc->preempt_client);
 	if (ret) {
-		__destroy_doorbell(guc->execbuf_client);
+		__destroy_doorbell(guc->execbuf_client, true);
 		return ret;
 	}
 
@@ -1043,7 +1050,7 @@ static void guc_client_free(struct i915_guc_client *client)
 	/* FIXME: in many cases, by the time we get here the GuC has been
 	 * reset, so we cannot destroy the doorbell properly. Ignore the
 	 * error message for now */
-	destroy_doorbell(client);
+	destroy_doorbell(client, true);
 	guc_stage_desc_fini(client->guc, client);
 	i915_gem_object_unpin_map(client->vma->obj);
 	i915_vma_unpin_and_release(&client->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] 14+ messages in thread

* [PATCH 2/5] drm/i915/guc: Release all client doorbells on suspend and acquire on resume
  2017-11-05 13:39 [PATCH 1/5] drm/i915/guc: Separate GuC doorbell destroy function into doorbell unit and GuC task Sagar Arun Kamble
@ 2017-11-05 13:39 ` Sagar Arun Kamble
  2017-11-06 12:13   ` Chris Wilson
  2017-11-05 13:39 ` [PATCH 3/5] drm/i915/guc: Deactivate all client doorbells before reset Sagar Arun Kamble
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Sagar Arun Kamble @ 2017-11-05 13:39 UTC (permalink / raw)
  To: intel-gfx

Before GT device suspend, i915 should release GuC client doorbells by
stopping doorbell controller snooping and doorbell deallocation through
GuC. They need to be acquired again on resume. This will also resolve
the driver unload issue with GuC, where doorbell deallocation was being
done post GEM suspend.
Release function is called from guc_suspend prior to sending sleep action
during runtime and drm suspend. Acquiral is different in runtime and drm
resume paths as on drm resume we are currently doing full reinit. Release
should be done in synchronization with acquire hence GuC suspend/resume
along with doorbell release/acquire should be under struct_mutex. Upcoming
suspend and resume restructuring for GuC will update these changes.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c            |  3 +++
 drivers/gpu/drm/i915/i915_gem.c            |  5 +++--
 drivers/gpu/drm/i915/i915_guc_submission.c | 20 ++++++++++++++++----
 drivers/gpu/drm/i915/i915_guc_submission.h |  2 ++
 drivers/gpu/drm/i915/intel_guc.c           |  2 ++
 5 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e7e9e06..3df8a7d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -47,6 +47,7 @@
 #include <drm/i915_drm.h>
 
 #include "i915_drv.h"
+#include "i915_guc_submission.h"
 #include "i915_trace.h"
 #include "i915_vgpu.h"
 #include "intel_drv.h"
@@ -2615,6 +2616,8 @@ static int intel_runtime_resume(struct device *kdev)
 
 	intel_guc_resume(dev_priv);
 
+	i915_guc_clients_acquire_doorbells(&dev_priv->guc);
+
 	if (IS_GEN9_LP(dev_priv)) {
 		bxt_disable_dc9(dev_priv);
 		bxt_display_core_init(dev_priv, true);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6a71805..cbce714 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -29,13 +29,14 @@
 #include <drm/drm_vma_manager.h>
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
+#include "i915_gemfs.h"
 #include "i915_gem_clflush.h"
-#include "i915_vgpu.h"
+#include "i915_guc_submission.h"
 #include "i915_trace.h"
+#include "i915_vgpu.h"
 #include "intel_drv.h"
 #include "intel_frontbuffer.h"
 #include "intel_mocs.h"
-#include "i915_gemfs.h"
 #include <linux/dma-fence-array.h>
 #include <linux/kthread.h>
 #include <linux/reservation.h>
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index b6486dc..b989edf 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1047,10 +1047,6 @@ static void guc_client_free(struct i915_guc_client *client)
 	 * Be sure to drop any locks
 	 */
 
-	/* FIXME: in many cases, by the time we get here the GuC has been
-	 * reset, so we cannot destroy the doorbell properly. Ignore the
-	 * error message for now */
-	destroy_doorbell(client, true);
 	guc_stage_desc_fini(client->guc, client);
 	i915_gem_object_unpin_map(client->vma->obj);
 	i915_vma_unpin_and_release(&client->vma);
@@ -1102,6 +1098,21 @@ static void guc_clients_destroy(struct intel_guc *guc)
 	guc_client_free(client);
 }
 
+void i915_guc_clients_acquire_doorbells(struct intel_guc *guc)
+{
+	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
+		return;
+
+	create_doorbell(guc->execbuf_client);
+	create_doorbell(guc->preempt_client);
+}
+
+void i915_guc_clients_release_doorbells(struct intel_guc *guc)
+{
+	destroy_doorbell(guc->preempt_client, true);
+	destroy_doorbell(guc->execbuf_client, true);
+}
+
 static void guc_policy_init(struct guc_policy *policy)
 {
 	policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
@@ -1423,6 +1434,7 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 	} else {
 		guc_reset_wq(guc->execbuf_client);
 		guc_reset_wq(guc->preempt_client);
+		i915_guc_clients_acquire_doorbells(guc);
 	}
 
 	err = intel_guc_sample_forcewake(guc);
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.h b/drivers/gpu/drm/i915/i915_guc_submission.h
index cb4353b..1d5cf22 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.h
+++ b/drivers/gpu/drm/i915/i915_guc_submission.h
@@ -72,6 +72,8 @@ struct i915_guc_client {
 	u64 submissions[I915_NUM_ENGINES];
 };
 
+void i915_guc_clients_acquire_doorbells(struct intel_guc *guc);
+void i915_guc_clients_release_doorbells(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);
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 9678630..b6eb5ac 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -274,6 +274,8 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
 	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return 0;
 
+	i915_guc_clients_release_doorbells(&dev_priv->guc);
+
 	gen9_disable_guc_interrupts(dev_priv);
 
 	data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
-- 
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] 14+ messages in thread

* [PATCH 3/5] drm/i915/guc: Deactivate all client doorbells before reset
  2017-11-05 13:39 [PATCH 1/5] drm/i915/guc: Separate GuC doorbell destroy function into doorbell unit and GuC task Sagar Arun Kamble
  2017-11-05 13:39 ` [PATCH 2/5] drm/i915/guc: Release all client doorbells on suspend and acquire on resume Sagar Arun Kamble
@ 2017-11-05 13:39 ` Sagar Arun Kamble
  2017-11-05 13:39 ` [PATCH 4/5] drm/i915/guc: Increase wait timeout for doorbell release status update Sagar Arun Kamble
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Sagar Arun Kamble @ 2017-11-05 13:39 UTC (permalink / raw)
  To: intel-gfx

Prior to i915 reset, we need to deactivate all client doorbells
as they will need to be reacquired again post reset. We should
not execute GuC doorbell deallocation flow as GuC might be in
bad state (possibly reason for i915 reset).

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c            |  2 ++
 drivers/gpu/drm/i915/i915_guc_submission.c | 14 ++++++++++++++
 drivers/gpu/drm/i915/i915_guc_submission.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 cbce714..879e318 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2963,6 +2963,8 @@ int i915_gem_reset_prepare(struct drm_i915_private *dev_priv)
 
 	i915_gem_revoke_fences(dev_priv);
 
+	i915_guc_clients_reset_prepare(&dev_priv->guc);
+
 	return err;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index b989edf..21f7fa7 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1113,6 +1113,20 @@ void i915_guc_clients_release_doorbells(struct intel_guc *guc)
 	destroy_doorbell(guc->execbuf_client, true);
 }
 
+void i915_guc_clients_reset_prepare(struct intel_guc *guc)
+{
+	if (!i915_modparams.enable_guc_submission)
+		return;
+
+	/*
+	 * We are only deactivating doorbell unit monitoring. GuC might
+	 * be in bad state and will be reset hence don't execute GuC
+	 * doorbell deallocation flow.
+	 */
+	destroy_doorbell(guc->preempt_client, false);
+	destroy_doorbell(guc->execbuf_client, false);
+}
+
 static void guc_policy_init(struct guc_policy *policy)
 {
 	policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.h b/drivers/gpu/drm/i915/i915_guc_submission.h
index 1d5cf22..91ad505 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.h
+++ b/drivers/gpu/drm/i915/i915_guc_submission.h
@@ -74,6 +74,7 @@ struct i915_guc_client {
 
 void i915_guc_clients_acquire_doorbells(struct intel_guc *guc);
 void i915_guc_clients_release_doorbells(struct intel_guc *guc);
+void i915_guc_clients_reset_prepare(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);
-- 
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] 14+ messages in thread

* [PATCH 4/5] drm/i915/guc: Increase wait timeout for doorbell release status update
  2017-11-05 13:39 [PATCH 1/5] drm/i915/guc: Separate GuC doorbell destroy function into doorbell unit and GuC task Sagar Arun Kamble
  2017-11-05 13:39 ` [PATCH 2/5] drm/i915/guc: Release all client doorbells on suspend and acquire on resume Sagar Arun Kamble
  2017-11-05 13:39 ` [PATCH 3/5] drm/i915/guc: Deactivate all client doorbells before reset Sagar Arun Kamble
@ 2017-11-05 13:39 ` Sagar Arun Kamble
  2017-11-05 15:55   ` Michal Wajdeczko
  2017-11-05 13:39 ` [PATCH 5/5] HAX enable GuC submission for CI Sagar Arun Kamble
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Sagar Arun Kamble @ 2017-11-05 13:39 UTC (permalink / raw)
  To: intel-gfx

Current wait timeout of 10us is very tight as seen on SKL/KBL randomly
for pm_rpm@basic-pci-d3-state. Increase it to 50us.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 21f7fa7..3914415 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -222,7 +222,8 @@ static int __destroy_doorbell(struct i915_guc_client *client, bool notify_guc)
 	/* Doorbell release flow requires that we wait for GEN8_DRB_VALID bit
 	 * to go to zero after updating db_status before we call the GuC to
 	 * release the doorbell */
-	if (wait_for_us(!(I915_READ(GEN8_DRBREGL(db_id)) & GEN8_DRB_VALID), 10))
+	if (wait_for_us(!(I915_READ(GEN8_DRBREGL(db_id)) & GEN8_DRB_VALID),
+			50))
 		WARN_ONCE(true, "Doorbell never became invalid after disable\n");
 
 	if (notify_guc)
-- 
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] 14+ messages in thread

* [PATCH 5/5] HAX enable GuC submission for CI
  2017-11-05 13:39 [PATCH 1/5] drm/i915/guc: Separate GuC doorbell destroy function into doorbell unit and GuC task Sagar Arun Kamble
                   ` (2 preceding siblings ...)
  2017-11-05 13:39 ` [PATCH 4/5] drm/i915/guc: Increase wait timeout for doorbell release status update Sagar Arun Kamble
@ 2017-11-05 13:39 ` Sagar Arun Kamble
  2017-11-05 14:18 ` ✗ Fi.CI.BAT: warning for series starting with [1/5] drm/i915/guc: Separate GuC doorbell destroy function into doorbell unit and GuC task Patchwork
  2017-11-08 12:32 ` ✓ Fi.CI.BAT: success " Patchwork
  5 siblings, 0 replies; 14+ messages in thread
From: Sagar Arun Kamble @ 2017-11-05 13:39 UTC (permalink / raw)
  To: intel-gfx

Also revert ("drm/i915/guc: Assert that we switch between
known ggtt->invalidate functions")
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++------
 drivers/gpu/drm/i915/i915_params.h  | 4 ++--
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 0684d5d..a351ddf 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3562,17 +3562,13 @@ int i915_ggtt_enable_hw(struct drm_i915_private *dev_priv)
 
 void i915_ggtt_enable_guc(struct drm_i915_private *i915)
 {
-	GEM_BUG_ON(i915->ggtt.invalidate != gen6_ggtt_invalidate);
-
 	i915->ggtt.invalidate = guc_ggtt_invalidate;
 }
 
 void i915_ggtt_disable_guc(struct drm_i915_private *i915)
 {
-	/* We should only be called after i915_ggtt_enable_guc() */
-	GEM_BUG_ON(i915->ggtt.invalidate != guc_ggtt_invalidate);
-
-	i915->ggtt.invalidate = gen6_ggtt_invalidate;
+	if (i915->ggtt.invalidate == guc_ggtt_invalidate)
+		i915->ggtt.invalidate = gen6_ggtt_invalidate;
 }
 
 void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index c729226..c38cef0 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -44,8 +44,8 @@
 	param(int, disable_power_well, -1) \
 	param(int, enable_ips, 1) \
 	param(int, invert_brightness, 0) \
-	param(int, enable_guc_loading, 0) \
-	param(int, enable_guc_submission, 0) \
+	param(int, enable_guc_loading, 1) \
+	param(int, enable_guc_submission, 1) \
 	param(int, guc_log_level, -1) \
 	param(char *, guc_firmware_path, NULL) \
 	param(char *, huc_firmware_path, NULL) \
-- 
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] 14+ messages in thread

* ✗ Fi.CI.BAT: warning for series starting with [1/5] drm/i915/guc: Separate GuC doorbell destroy function into doorbell unit and GuC task
  2017-11-05 13:39 [PATCH 1/5] drm/i915/guc: Separate GuC doorbell destroy function into doorbell unit and GuC task Sagar Arun Kamble
                   ` (3 preceding siblings ...)
  2017-11-05 13:39 ` [PATCH 5/5] HAX enable GuC submission for CI Sagar Arun Kamble
@ 2017-11-05 14:18 ` Patchwork
  2017-11-08 12:32 ` ✓ Fi.CI.BAT: success " Patchwork
  5 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2017-11-05 14:18 UTC (permalink / raw)
  To: Sagar Arun Kamble; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/5] drm/i915/guc: Separate GuC doorbell destroy function into doorbell unit and GuC task
URL   : https://patchwork.freedesktop.org/series/33209/
State : warning

== Summary ==

Series 33209v1 series starting with [1/5] drm/i915/guc: Separate GuC doorbell destroy function into doorbell unit and GuC task
https://patchwork.freedesktop.org/api/1.0/series/33209/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-c:
                dmesg-warn -> PASS       (fi-bsw-n3050)
Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> DMESG-WARN (fi-cnl-y)
        Subgroup basic-no-display:
                pass       -> DMESG-WARN (fi-cnl-y)
        Subgroup basic-reload-inject:
                pass       -> DMESG-WARN (fi-cnl-y)

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:444s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:454s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:380s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:538s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:276s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:510s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:512s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:503s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:486s
fi-cnl-y         total:289  pass:259  dwarn:3   dfail:0   fail:0   skip:27  time:626s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:425s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:265s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:588s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:435s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:430s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:428s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:497s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:466s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:574s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:479s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:590s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:577s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:450s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:592s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:649s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:517s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:502s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:464s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:570s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:428s

8b0ae6b50a229dc661a02f4034252ee854cc9b83 drm-tip: 2017y-11m-03d-17h-15m-57s UTC integration manifest
a91c31b1a24c HAX enable GuC submission for CI
32d332ce5a44 drm/i915/guc: Increase wait timeout for doorbell release status update
be93da66d7df drm/i915/guc: Deactivate all client doorbells before reset
b6515b105ab5 drm/i915/guc: Release all client doorbells on suspend and acquire on resume
9e190e74bacd drm/i915/guc: Separate GuC doorbell destroy function into doorbell unit and GuC task

== Logs ==

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

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

* Re: [PATCH 4/5] drm/i915/guc: Increase wait timeout for doorbell release status update
  2017-11-05 13:39 ` [PATCH 4/5] drm/i915/guc: Increase wait timeout for doorbell release status update Sagar Arun Kamble
@ 2017-11-05 15:55   ` Michal Wajdeczko
  2017-11-06  5:43     ` Sagar Arun Kamble
  0 siblings, 1 reply; 14+ messages in thread
From: Michal Wajdeczko @ 2017-11-05 15:55 UTC (permalink / raw)
  To: intel-gfx, Sagar Arun Kamble

On Sun, 05 Nov 2017 14:39:42 +0100, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> Current wait timeout of 10us is very tight as seen on SKL/KBL randomly
> for pm_rpm@basic-pci-d3-state. Increase it to 50us.
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c  
> b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 21f7fa7..3914415 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -222,7 +222,8 @@ static int __destroy_doorbell(struct i915_guc_client  
> *client, bool notify_guc)
>  	/* Doorbell release flow requires that we wait for GEN8_DRB_VALID bit
>  	 * to go to zero after updating db_status before we call the GuC to
>  	 * release the doorbell */
> -	if (wait_for_us(!(I915_READ(GEN8_DRBREGL(db_id)) & GEN8_DRB_VALID),  
> 10))
> +	if (wait_for_us(!(I915_READ(GEN8_DRBREGL(db_id)) & GEN8_DRB_VALID),
> +			50))

Can we use intel_wait_for_register() here ?

>  		WARN_ONCE(true, "Doorbell never became invalid after disable\n");
> 	if (notify_guc)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 4/5] drm/i915/guc: Increase wait timeout for doorbell release status update
  2017-11-05 15:55   ` Michal Wajdeczko
@ 2017-11-06  5:43     ` Sagar Arun Kamble
  0 siblings, 0 replies; 14+ messages in thread
From: Sagar Arun Kamble @ 2017-11-06  5:43 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx



On 11/5/2017 9:25 PM, Michal Wajdeczko wrote:
> On Sun, 05 Nov 2017 14:39:42 +0100, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>> Current wait timeout of 10us is very tight as seen on SKL/KBL randomly
>> for pm_rpm@basic-pci-d3-state. Increase it to 50us.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Michel Thierry <michel.thierry@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_guc_submission.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 21f7fa7..3914415 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -222,7 +222,8 @@ static int __destroy_doorbell(struct 
>> i915_guc_client *client, bool notify_guc)
>>      /* Doorbell release flow requires that we wait for 
>> GEN8_DRB_VALID bit
>>       * to go to zero after updating db_status before we call the GuC to
>>       * release the doorbell */
>> -    if (wait_for_us(!(I915_READ(GEN8_DRBREGL(db_id)) & 
>> GEN8_DRB_VALID), 10))
>> +    if (wait_for_us(!(I915_READ(GEN8_DRBREGL(db_id)) & GEN8_DRB_VALID),
>> +            50))
>
> Can we use intel_wait_for_register() here ?
Yes. Since wait timeout is expected to be in the order of uS (50us for 
now) will be using __intel_wait_for_register_fw.

+        if (__intel_wait_for_register_fw(dev_priv, GEN8_DRBREGL(db_id), 
GEN8_DRB_VALID, 0,
+ 50, 1, NULL))

Is this fine (with slow timeout of 1ms)?

>>          WARN_ONCE(true, "Doorbell never became invalid after 
>> disable\n");
>>     if (notify_guc)

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

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

* Re: [PATCH 2/5] drm/i915/guc: Release all client doorbells on suspend and acquire on resume
  2017-11-05 13:39 ` [PATCH 2/5] drm/i915/guc: Release all client doorbells on suspend and acquire on resume Sagar Arun Kamble
@ 2017-11-06 12:13   ` Chris Wilson
  2017-11-07  6:05     ` Sagar Arun Kamble
  0 siblings, 1 reply; 14+ messages in thread
From: Chris Wilson @ 2017-11-06 12:13 UTC (permalink / raw)
  To: Sagar Arun Kamble, intel-gfx; +Cc: Sagar

Quoting Sagar Arun Kamble (2017-11-05 13:39:40)
> Before GT device suspend, i915 should release GuC client doorbells by
> stopping doorbell controller snooping and doorbell deallocation through
> GuC. They need to be acquired again on resume. This will also resolve
> the driver unload issue with GuC, where doorbell deallocation was being
> done post GEM suspend.
> Release function is called from guc_suspend prior to sending sleep action
> during runtime and drm suspend. Acquiral is different in runtime and drm
> resume paths as on drm resume we are currently doing full reinit. Release
> should be done in synchronization with acquire hence GuC suspend/resume
> along with doorbell release/acquire should be under struct_mutex. Upcoming
> suspend and resume restructuring for GuC will update these changes.
> 
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c            |  3 +++
>  drivers/gpu/drm/i915/i915_gem.c            |  5 +++--
>  drivers/gpu/drm/i915/i915_guc_submission.c | 20 ++++++++++++++++----
>  drivers/gpu/drm/i915/i915_guc_submission.h |  2 ++
>  drivers/gpu/drm/i915/intel_guc.c           |  2 ++
>  5 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e7e9e06..3df8a7d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -47,6 +47,7 @@
>  #include <drm/i915_drm.h>
>  
>  #include "i915_drv.h"
> +#include "i915_guc_submission.h"
>  #include "i915_trace.h"
>  #include "i915_vgpu.h"
>  #include "intel_drv.h"
> @@ -2615,6 +2616,8 @@ static int intel_runtime_resume(struct device *kdev)
>  
>         intel_guc_resume(dev_priv);
>  
> +       i915_guc_clients_acquire_doorbells(&dev_priv->guc);

intel_guc_acquire_doorbells();

Though, if we need to specialise between resume and runtime_resume, I
suggest adding intel_guc_resume() and intel_guc_runtime_resume() entry
points. (We probably should find a better way to distinguish those two
paths, system_resume vs runtime_resume?)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 2/5] drm/i915/guc: Release all client doorbells on suspend and acquire on resume
  2017-11-06 12:13   ` Chris Wilson
@ 2017-11-07  6:05     ` Sagar Arun Kamble
  2017-11-07  9:21       ` Chris Wilson
  0 siblings, 1 reply; 14+ messages in thread
From: Sagar Arun Kamble @ 2017-11-07  6:05 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx
  Cc: Sagar Arun Kamble , " Michał Winiarski , Michel Thierry ,
	Michal Wajdeczko , Arkadiusz Hiler ,
	Joonas Lahtinen



On 11/6/2017 5:43 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2017-11-05 13:39:40)
>> Before GT device suspend, i915 should release GuC client doorbells by
>> stopping doorbell controller snooping and doorbell deallocation through
>> GuC. They need to be acquired again on resume. This will also resolve
>> the driver unload issue with GuC, where doorbell deallocation was being
>> done post GEM suspend.
>> Release function is called from guc_suspend prior to sending sleep action
>> during runtime and drm suspend. Acquiral is different in runtime and drm
>> resume paths as on drm resume we are currently doing full reinit. Release
>> should be done in synchronization with acquire hence GuC suspend/resume
>> along with doorbell release/acquire should be under struct_mutex. Upcoming
>> suspend and resume restructuring for GuC will update these changes.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Michel Thierry <michel.thierry@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c            |  3 +++
>>   drivers/gpu/drm/i915/i915_gem.c            |  5 +++--
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 20 ++++++++++++++++----
>>   drivers/gpu/drm/i915/i915_guc_submission.h |  2 ++
>>   drivers/gpu/drm/i915/intel_guc.c           |  2 ++
>>   5 files changed, 26 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index e7e9e06..3df8a7d 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -47,6 +47,7 @@
>>   #include <drm/i915_drm.h>
>>   
>>   #include "i915_drv.h"
>> +#include "i915_guc_submission.h"
>>   #include "i915_trace.h"
>>   #include "i915_vgpu.h"
>>   #include "intel_drv.h"
>> @@ -2615,6 +2616,8 @@ static int intel_runtime_resume(struct device *kdev)
>>   
>>          intel_guc_resume(dev_priv);
>>   
>> +       i915_guc_clients_acquire_doorbells(&dev_priv->guc);
> intel_guc_acquire_doorbells();
Prefixed "i915_guc_clients" since this modifies submission state 
(clients/doorbells). Should have kept dev_priv as parameter.
what should be the correct option here: intel_guc*(guc) or 
i915_guc*(dev_priv)
>
> Though, if we need to specialise between resume and runtime_resume, I
> suggest adding intel_guc_resume() and intel_guc_runtime_resume() entry
> points. (We probably should find a better way to distinguish those two
> paths, system_resume vs runtime_resume?)
Yes. these will be added in the upcoming GuC suspend/resume 
restructuring series.
Functionality along runtime/system suspend path will be same for GuC. 
Will defer in
the resume path since we are doing full gem reinit on resume from system 
suspend.
> -Chris

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

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

* Re: [PATCH 2/5] drm/i915/guc: Release all client doorbells on suspend and acquire on resume
  2017-11-07  6:05     ` Sagar Arun Kamble
@ 2017-11-07  9:21       ` Chris Wilson
  2017-11-07 10:27         ` Michal Wajdeczko
  0 siblings, 1 reply; 14+ messages in thread
From: Chris Wilson @ 2017-11-07  9:21 UTC (permalink / raw)
  To: Sagar Arun Kamble, intel-gfx
  Cc: =?utf-8?q?Sagar_Arun_Kamble_=2C__=22_Micha=C5=82_Winiarski_=2C__Michel_Th?=

Quoting Sagar Arun Kamble (2017-11-07 06:05:01)
> 
> 
> On 11/6/2017 5:43 PM, Chris Wilson wrote:
> > Quoting Sagar Arun Kamble (2017-11-05 13:39:40)
> >> Before GT device suspend, i915 should release GuC client doorbells by
> >> stopping doorbell controller snooping and doorbell deallocation through
> >> GuC. They need to be acquired again on resume. This will also resolve
> >> the driver unload issue with GuC, where doorbell deallocation was being
> >> done post GEM suspend.
> >> Release function is called from guc_suspend prior to sending sleep action
> >> during runtime and drm suspend. Acquiral is different in runtime and drm
> >> resume paths as on drm resume we are currently doing full reinit. Release
> >> should be done in synchronization with acquire hence GuC suspend/resume
> >> along with doorbell release/acquire should be under struct_mutex. Upcoming
> >> suspend and resume restructuring for GuC will update these changes.
> >>
> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Michał Winiarski <michal.winiarski@intel.com>
> >> Cc: Michel Thierry <michel.thierry@intel.com>
> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_drv.c            |  3 +++
> >>   drivers/gpu/drm/i915/i915_gem.c            |  5 +++--
> >>   drivers/gpu/drm/i915/i915_guc_submission.c | 20 ++++++++++++++++----
> >>   drivers/gpu/drm/i915/i915_guc_submission.h |  2 ++
> >>   drivers/gpu/drm/i915/intel_guc.c           |  2 ++
> >>   5 files changed, 26 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >> index e7e9e06..3df8a7d 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -47,6 +47,7 @@
> >>   #include <drm/i915_drm.h>
> >>   
> >>   #include "i915_drv.h"
> >> +#include "i915_guc_submission.h"
> >>   #include "i915_trace.h"
> >>   #include "i915_vgpu.h"
> >>   #include "intel_drv.h"
> >> @@ -2615,6 +2616,8 @@ static int intel_runtime_resume(struct device *kdev)
> >>   
> >>          intel_guc_resume(dev_priv);
> >>   
> >> +       i915_guc_clients_acquire_doorbells(&dev_priv->guc);
> > intel_guc_acquire_doorbells();
> Prefixed "i915_guc_clients" since this modifies submission state 
> (clients/doorbells). Should have kept dev_priv as parameter.
> what should be the correct option here: intel_guc*(guc) or 
> i915_guc*(dev_priv)

GuC submission is not i915. It is not part of the user facing api.
Operate on intel_guc as you were.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 2/5] drm/i915/guc: Release all client doorbells on suspend and acquire on resume
  2017-11-07  9:21       ` Chris Wilson
@ 2017-11-07 10:27         ` Michal Wajdeczko
  2017-11-07 10:33           ` Sagar Arun Kamble
  0 siblings, 1 reply; 14+ messages in thread
From: Michal Wajdeczko @ 2017-11-07 10:27 UTC (permalink / raw)
  To: Sagar Arun Kamble, intel-gfx, Chris Wilson

On Tue, 07 Nov 2017 10:21:17 +0100, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Sagar Arun Kamble (2017-11-07 06:05:01)
>>
>>
>> On 11/6/2017 5:43 PM, Chris Wilson wrote:
>> > Quoting Sagar Arun Kamble (2017-11-05 13:39:40)
>> >> Before GT device suspend, i915 should release GuC client doorbells by
>> >> stopping doorbell controller snooping and doorbell deallocation  
>> through
>> >> GuC. They need to be acquired again on resume. This will also resolve
>> >> the driver unload issue with GuC, where doorbell deallocation was  
>> being
>> >> done post GEM suspend.
>> >> Release function is called from guc_suspend prior to sending sleep  
>> action
>> >> during runtime and drm suspend. Acquiral is different in runtime and  
>> drm
>> >> resume paths as on drm resume we are currently doing full reinit.  
>> Release
>> >> should be done in synchronization with acquire hence GuC  
>> suspend/resume
>> >> along with doorbell release/acquire should be under struct_mutex.  
>> Upcoming
>> >> suspend and resume restructuring for GuC will update these changes.
>> >>
>> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> >> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> >> Cc: Michel Thierry <michel.thierry@intel.com>
>> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> >> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
>> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> >> ---
>> >>   drivers/gpu/drm/i915/i915_drv.c            |  3 +++
>> >>   drivers/gpu/drm/i915/i915_gem.c            |  5 +++--
>> >>   drivers/gpu/drm/i915/i915_guc_submission.c | 20  
>> ++++++++++++++++----
>> >>   drivers/gpu/drm/i915/i915_guc_submission.h |  2 ++
>> >>   drivers/gpu/drm/i915/intel_guc.c           |  2 ++
>> >>   5 files changed, 26 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c  
>> b/drivers/gpu/drm/i915/i915_drv.c
>> >> index e7e9e06..3df8a7d 100644
>> >> --- a/drivers/gpu/drm/i915/i915_drv.c
>> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> >> @@ -47,6 +47,7 @@
>> >>   #include <drm/i915_drm.h>
>> >>
>> >>   #include "i915_drv.h"
>> >> +#include "i915_guc_submission.h"
>> >>   #include "i915_trace.h"
>> >>   #include "i915_vgpu.h"
>> >>   #include "intel_drv.h"
>> >> @@ -2615,6 +2616,8 @@ static int intel_runtime_resume(struct device  
>> *kdev)
>> >>
>> >>          intel_guc_resume(dev_priv);
>> >>
>> >> +       i915_guc_clients_acquire_doorbells(&dev_priv->guc);
>> > intel_guc_acquire_doorbells();
>> Prefixed "i915_guc_clients" since this modifies submission state
>> (clients/doorbells). Should have kept dev_priv as parameter.
>> what should be the correct option here: intel_guc*(guc) or
>> i915_guc*(dev_priv)
>
> GuC submission is not i915. It is not part of the user facing api.
> Operate on intel_guc as you were.

So if GuC submission is not i915 than maybe to avoid confusion we should
rename i915_guc_submission.c to intel_guc_submission.c ?

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

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

* Re: [PATCH 2/5] drm/i915/guc: Release all client doorbells on suspend and acquire on resume
  2017-11-07 10:27         ` Michal Wajdeczko
@ 2017-11-07 10:33           ` Sagar Arun Kamble
  0 siblings, 0 replies; 14+ messages in thread
From: Sagar Arun Kamble @ 2017-11-07 10:33 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx, Chris Wilson



On 11/7/2017 3:57 PM, Michal Wajdeczko wrote:
> On Tue, 07 Nov 2017 10:21:17 +0100, Chris Wilson 
> <chris@chris-wilson.co.uk> wrote:
>
>> Quoting Sagar Arun Kamble (2017-11-07 06:05:01)
>>>
>>>
>>> On 11/6/2017 5:43 PM, Chris Wilson wrote:
>>> > Quoting Sagar Arun Kamble (2017-11-05 13:39:40)
>>> >> Before GT device suspend, i915 should release GuC client 
>>> doorbells by
>>> >> stopping doorbell controller snooping and doorbell deallocation 
>>> through
>>> >> GuC. They need to be acquired again on resume. This will also 
>>> resolve
>>> >> the driver unload issue with GuC, where doorbell deallocation was 
>>> being
>>> >> done post GEM suspend.
>>> >> Release function is called from guc_suspend prior to sending 
>>> sleep action
>>> >> during runtime and drm suspend. Acquiral is different in runtime 
>>> and drm
>>> >> resume paths as on drm resume we are currently doing full reinit. 
>>> Release
>>> >> should be done in synchronization with acquire hence GuC 
>>> suspend/resume
>>> >> along with doorbell release/acquire should be under struct_mutex. 
>>> Upcoming
>>> >> suspend and resume restructuring for GuC will update these changes.
>>> >>
>>> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> >> Cc: Michał Winiarski <michal.winiarski@intel.com>
>>> >> Cc: Michel Thierry <michel.thierry@intel.com>
>>> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> >> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
>>> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> >> ---
>>> >>   drivers/gpu/drm/i915/i915_drv.c            |  3 +++
>>> >>   drivers/gpu/drm/i915/i915_gem.c            |  5 +++--
>>> >>   drivers/gpu/drm/i915/i915_guc_submission.c | 20 
>>> ++++++++++++++++----
>>> >>   drivers/gpu/drm/i915/i915_guc_submission.h |  2 ++
>>> >>   drivers/gpu/drm/i915/intel_guc.c           |  2 ++
>>> >>   5 files changed, 26 insertions(+), 6 deletions(-)
>>> >>
>>> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>>> b/drivers/gpu/drm/i915/i915_drv.c
>>> >> index e7e9e06..3df8a7d 100644
>>> >> --- a/drivers/gpu/drm/i915/i915_drv.c
>>> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> >> @@ -47,6 +47,7 @@
>>> >>   #include <drm/i915_drm.h>
>>> >>
>>> >>   #include "i915_drv.h"
>>> >> +#include "i915_guc_submission.h"
>>> >>   #include "i915_trace.h"
>>> >>   #include "i915_vgpu.h"
>>> >>   #include "intel_drv.h"
>>> >> @@ -2615,6 +2616,8 @@ static int intel_runtime_resume(struct 
>>> device *kdev)
>>> >>
>>> >>          intel_guc_resume(dev_priv);
>>> >>
>>> >> + i915_guc_clients_acquire_doorbells(&dev_priv->guc);
>>> > intel_guc_acquire_doorbells();
>>> Prefixed "i915_guc_clients" since this modifies submission state
>>> (clients/doorbells). Should have kept dev_priv as parameter.
>>> what should be the correct option here: intel_guc*(guc) or
>>> i915_guc*(dev_priv)
>>
>> GuC submission is not i915. It is not part of the user facing api.
>> Operate on intel_guc as you were.
>
> So if GuC submission is not i915 than maybe to avoid confusion we should
> rename i915_guc_submission.c to intel_guc_submission.c ?
Yes. Will need to update functions that are i915_guc* as well. (spotted 
some in guc_log.c as well.)
>
> Michal

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

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

* ✓ Fi.CI.BAT: success for series starting with [1/5] drm/i915/guc: Separate GuC doorbell destroy function into doorbell unit and GuC task
  2017-11-05 13:39 [PATCH 1/5] drm/i915/guc: Separate GuC doorbell destroy function into doorbell unit and GuC task Sagar Arun Kamble
                   ` (4 preceding siblings ...)
  2017-11-05 14:18 ` ✗ Fi.CI.BAT: warning for series starting with [1/5] drm/i915/guc: Separate GuC doorbell destroy function into doorbell unit and GuC task Patchwork
@ 2017-11-08 12:32 ` Patchwork
  5 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2017-11-08 12:32 UTC (permalink / raw)
  To: Sagar Arun Kamble; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/5] drm/i915/guc: Separate GuC doorbell destroy function into doorbell unit and GuC task
URL   : https://patchwork.freedesktop.org/series/33209/
State : success

== Summary ==

Series 33209v1 series starting with [1/5] drm/i915/guc: Separate GuC doorbell destroy function into doorbell unit and GuC task
https://patchwork.freedesktop.org/api/1.0/series/33209/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-c:
                dmesg-warn -> PASS       (fi-bsw-n3050) fdo#103479

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

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:444s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:454s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:380s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:538s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:276s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:510s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:512s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:503s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:486s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:425s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:265s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:588s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:435s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:430s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:428s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:497s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:466s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:574s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:479s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:590s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:577s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:450s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:592s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:649s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:517s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:502s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:464s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:570s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:428s
Blacklisted hosts:
fi-cnl-y         total:289  pass:259  dwarn:3   dfail:0   fail:0   skip:27  time:626s

8b0ae6b50a229dc661a02f4034252ee854cc9b83 drm-tip: 2017y-11m-03d-17h-15m-57s UTC integration manifest
a91c31b1a24c HAX enable GuC submission for CI
32d332ce5a44 drm/i915/guc: Increase wait timeout for doorbell release status update
be93da66d7df drm/i915/guc: Deactivate all client doorbells before reset
b6515b105ab5 drm/i915/guc: Release all client doorbells on suspend and acquire on resume
9e190e74bacd drm/i915/guc: Separate GuC doorbell destroy function into doorbell unit and GuC task

== Logs ==

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

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

end of thread, other threads:[~2017-11-08 12:32 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-05 13:39 [PATCH 1/5] drm/i915/guc: Separate GuC doorbell destroy function into doorbell unit and GuC task Sagar Arun Kamble
2017-11-05 13:39 ` [PATCH 2/5] drm/i915/guc: Release all client doorbells on suspend and acquire on resume Sagar Arun Kamble
2017-11-06 12:13   ` Chris Wilson
2017-11-07  6:05     ` Sagar Arun Kamble
2017-11-07  9:21       ` Chris Wilson
2017-11-07 10:27         ` Michal Wajdeczko
2017-11-07 10:33           ` Sagar Arun Kamble
2017-11-05 13:39 ` [PATCH 3/5] drm/i915/guc: Deactivate all client doorbells before reset Sagar Arun Kamble
2017-11-05 13:39 ` [PATCH 4/5] drm/i915/guc: Increase wait timeout for doorbell release status update Sagar Arun Kamble
2017-11-05 15:55   ` Michal Wajdeczko
2017-11-06  5:43     ` Sagar Arun Kamble
2017-11-05 13:39 ` [PATCH 5/5] HAX enable GuC submission for CI Sagar Arun Kamble
2017-11-05 14:18 ` ✗ Fi.CI.BAT: warning for series starting with [1/5] drm/i915/guc: Separate GuC doorbell destroy function into doorbell unit and GuC task Patchwork
2017-11-08 12:32 ` ✓ Fi.CI.BAT: success " Patchwork

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.