[PATCH 0/7] GuC fixes

[PATCH 0/7] GuC fixes

[Michal Wajdeczko]

Misc GuC fixes for upcoming 32.0.3

Michal Wajdeczko (7):
  drm/i915/uc: Use GuC firmware status helper
  drm/i915/uc: Explicitly sanitize GuC/HuC on failure and finish
  drm/i915/uc: Skip GuC HW unwinding if GuC is already dead
  drm/i915/uc: Stop talking with GuC when resetting
  drm/i915/uc: Skip reset preparation if GuC is already dead
  drm/i915/uc: Stop GuC submission during reset prepare
  drm/i915/uc: Don't forget to prepare GuC for the reset

 drivers/gpu/drm/i915/gt/intel_reset.c       |  4 ++
 drivers/gpu/drm/i915/intel_guc_ct.h         |  5 +++
 drivers/gpu/drm/i915/intel_guc_submission.c | 25 +++++++++++
 drivers/gpu/drm/i915/intel_guc_submission.h |  1 +
 drivers/gpu/drm/i915/intel_uc.c             | 46 +++++++++++++++++----
 5 files changed, 72 insertions(+), 9 deletions(-)

-- 
2.19.2

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

[PATCH 1/7] drm/i915/uc: Use GuC firmware status helper

[Michal Wajdeczko]

We already have helper function for checking GuC firmware
load status. Replace existing open-coded checks.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_uc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 1ee70df51627..f67f6224b1df 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -486,7 +486,7 @@ void intel_uc_runtime_suspend(struct drm_i915_private *i915)
 	struct intel_guc *guc = &i915->guc;
 	int err;
 
-	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
+	if (!intel_guc_is_alive(guc))
 		return;
 
 	err = intel_guc_suspend(guc);
@@ -501,7 +501,7 @@ void intel_uc_suspend(struct drm_i915_private *i915)
 	struct intel_guc *guc = &i915->guc;
 	intel_wakeref_t wakeref;
 
-	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
+	if (!intel_guc_is_alive(guc))
 		return;
 
 	with_intel_runtime_pm(i915, wakeref)
@@ -516,7 +516,7 @@ int intel_uc_resume(struct drm_i915_private *i915)
 	if (!USES_GUC(i915))
 		return 0;
 
-	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
+	if (!intel_guc_is_alive(guc))
 		return 0;
 
 	guc_enable_communication(guc);
-- 
2.19.2

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

[PATCH 2/7] drm/i915/uc: Explicitly sanitize GuC/HuC on failure and finish

[Michal Wajdeczko]

Explicitly sanitize GuC/HuC on load failure and when we finish
using them to make sure our fw state tracking is always correct.

While around, use new helper in uc_reset_prepare.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/intel_uc.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index f67f6224b1df..01683d107348 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -337,14 +337,11 @@ void intel_uc_fini(struct drm_i915_private *i915)
 	intel_guc_fini(guc);
 }
 
-void intel_uc_sanitize(struct drm_i915_private *i915)
+static void __uc_sanitize(struct drm_i915_private *i915)
 {
 	struct intel_guc *guc = &i915->guc;
 	struct intel_huc *huc = &i915->huc;
 
-	if (!USES_GUC(i915))
-		return;
-
 	GEM_BUG_ON(!HAS_GUC(i915));
 
 	intel_huc_sanitize(huc);
@@ -353,6 +350,14 @@ void intel_uc_sanitize(struct drm_i915_private *i915)
 	__intel_uc_reset_hw(i915);
 }
 
+void intel_uc_sanitize(struct drm_i915_private *i915)
+{
+	if (!USES_GUC(i915))
+		return;
+
+	__uc_sanitize(i915);
+}
+
 int intel_uc_init_hw(struct drm_i915_private *i915)
 {
 	struct intel_guc *guc = &i915->guc;
@@ -438,6 +443,8 @@ int intel_uc_init_hw(struct drm_i915_private *i915)
 err_log_capture:
 	guc_capture_load_err_log(guc);
 err_out:
+	__uc_sanitize(i915);
+
 	/*
 	 * Note that there is no fallback as either user explicitly asked for
 	 * the GuC or driver default option was to run with the GuC enabled.
@@ -462,6 +469,7 @@ void intel_uc_fini_hw(struct drm_i915_private *i915)
 		intel_guc_submission_disable(guc);
 
 	guc_disable_communication(guc);
+	__uc_sanitize(i915);
 }
 
 /**
@@ -478,7 +486,7 @@ void intel_uc_reset_prepare(struct drm_i915_private *i915)
 		return;
 
 	guc_disable_communication(guc);
-	intel_uc_sanitize(i915);
+	__uc_sanitize(i915);
 }
 
 void intel_uc_runtime_suspend(struct drm_i915_private *i915)
-- 
2.19.2

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

[PATCH 3/7] drm/i915/uc: Skip GuC HW unwinding if GuC is already dead

[Michal Wajdeczko]

We should not attempt to unwind GuC hardware/firmware setup
if we already have sanitized GuC.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/intel_uc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 01683d107348..9d86cd831ea7 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -465,6 +465,9 @@ void intel_uc_fini_hw(struct drm_i915_private *i915)
 
 	GEM_BUG_ON(!HAS_GUC(i915));
 
+	if (!intel_guc_is_alive(guc))
+		return;
+
 	if (USES_GUC_SUBMISSION(i915))
 		intel_guc_submission_disable(guc);
 
-- 
2.19.2

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

[PATCH 4/7] drm/i915/uc: Stop talking with GuC when resetting

[Michal Wajdeczko]

Knowing that GuC will be reset soon, we may stop all communication
immediately without doing graceful cleanup as it is not needed.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_ct.h |  5 +++++
 drivers/gpu/drm/i915/intel_uc.c     | 13 ++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_ct.h b/drivers/gpu/drm/i915/intel_guc_ct.h
index f5e7f0663304..41ba593a4df7 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.h
+++ b/drivers/gpu/drm/i915/intel_guc_ct.h
@@ -96,4 +96,9 @@ void intel_guc_ct_fini(struct intel_guc_ct *ct);
 int intel_guc_ct_enable(struct intel_guc_ct *ct);
 void intel_guc_ct_disable(struct intel_guc_ct *ct);
 
+static inline void intel_guc_ct_stop(struct intel_guc_ct *ct)
+{
+	ct->host_channel.enabled = false;
+}
+
 #endif /* _INTEL_GUC_CT_H_ */
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 9d86cd831ea7..86edfa5ad72e 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -224,6 +224,17 @@ static int guc_enable_communication(struct intel_guc *guc)
 	return 0;
 }
 
+static void guc_stop_communication(struct intel_guc *guc)
+{
+	struct drm_i915_private *i915 = guc_to_i915(guc);
+
+	if (HAS_GUC_CT(i915))
+		intel_guc_ct_stop(&guc->ct);
+
+	guc->send = intel_guc_send_nop;
+	guc->handler = intel_guc_to_host_event_handler_nop;
+}
+
 static void guc_disable_communication(struct intel_guc *guc)
 {
 	struct drm_i915_private *i915 = guc_to_i915(guc);
@@ -488,7 +499,7 @@ void intel_uc_reset_prepare(struct drm_i915_private *i915)
 	if (!USES_GUC(i915))
 		return;
 
-	guc_disable_communication(guc);
+	guc_stop_communication(guc);
 	__uc_sanitize(i915);
 }
 
-- 
2.19.2

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

[PATCH 5/7] drm/i915/uc: Skip reset preparation if GuC is already dead

[Michal Wajdeczko]

We may skip reset preparation steps if GuC is already sanitized.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/intel_uc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 86edfa5ad72e..36c53a42927c 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -499,6 +499,9 @@ void intel_uc_reset_prepare(struct drm_i915_private *i915)
 	if (!USES_GUC(i915))
 		return;
 
+	if (!intel_guc_is_alive(guc))
+		return;
+
 	guc_stop_communication(guc);
 	__uc_sanitize(i915);
 }
-- 
2.19.2

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

[PATCH 6/7] drm/i915/uc: Stop GuC submission during reset prepare

[Michal Wajdeczko]

Knowing that GuC will be reset soon, perform only minimal
cleanup actions (ie. doorbells) without talking with GuC.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_submission.c | 25 +++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc_submission.h |  1 +
 drivers/gpu/drm/i915/intel_uc.c             |  3 +++
 3 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index ea0e3734d37c..8889442a31df 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -1208,6 +1208,12 @@ static void __guc_client_disable(struct intel_guc_client *client)
 	guc_proc_desc_fini(client);
 }
 
+static void __guc_client_stop(struct intel_guc_client *client)
+{
+	__fini_doorbell(client);
+	__update_doorbell_desc(client, GUC_DOORBELL_INVALID);
+}
+
 static int guc_clients_enable(struct intel_guc *guc)
 {
 	int ret;
@@ -1236,6 +1242,15 @@ static void guc_clients_disable(struct intel_guc *guc)
 		__guc_client_disable(guc->execbuf_client);
 }
 
+static void guc_clients_stop(struct intel_guc *guc)
+{
+	if (guc->preempt_client)
+		__guc_client_stop(guc->preempt_client);
+
+	if (guc->execbuf_client)
+		__guc_client_stop(guc->execbuf_client);
+}
+
 /*
  * Set up the memory resources to be shared with the GuC (via the GGTT)
  * at firmware loading time.
@@ -1455,6 +1470,16 @@ void intel_guc_submission_disable(struct intel_guc *guc)
 	guc_clients_disable(guc);
 }
 
+void intel_guc_submission_stop(struct intel_guc *guc)
+{
+	struct drm_i915_private *i915 = guc_to_i915(guc);
+
+	GEM_BUG_ON(i915->gt.awake); /* GT should be parked first */
+
+	guc_interrupts_release(i915);
+	guc_clients_stop(guc);
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/intel_guc.c"
 #endif
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.h b/drivers/gpu/drm/i915/intel_guc_submission.h
index 7d823a513b9c..fbfe7b3b0957 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.h
+++ b/drivers/gpu/drm/i915/intel_guc_submission.h
@@ -81,6 +81,7 @@ struct intel_guc_client {
 
 int intel_guc_submission_init(struct intel_guc *guc);
 int intel_guc_submission_enable(struct intel_guc *guc);
+void intel_guc_submission_stop(struct intel_guc *guc);
 void intel_guc_submission_disable(struct intel_guc *guc);
 void intel_guc_submission_fini(struct intel_guc *guc);
 int intel_guc_preempt_work_create(struct intel_guc *guc);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 36c53a42927c..51dcdb2764ce 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -502,6 +502,9 @@ void intel_uc_reset_prepare(struct drm_i915_private *i915)
 	if (!intel_guc_is_alive(guc))
 		return;
 
+	if (USES_GUC_SUBMISSION(i915))
+		intel_guc_submission_stop(guc);
+
 	guc_stop_communication(guc);
 	__uc_sanitize(i915);
 }
-- 
2.19.2

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

[PATCH 7/7] drm/i915/uc: Don't forget to prepare GuC for the reset

[Michal Wajdeczko]

When we reset engines using ALL_ENGINES mask, we will do
full GPU reset and GuC will be also affected. Let GuC be
prepared for upcoming reset.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_reset.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
index 464369bc55ad..ca6e40b6b4e2 100644
--- a/drivers/gpu/drm/i915/gt/intel_reset.c
+++ b/drivers/gpu/drm/i915/gt/intel_reset.c
@@ -564,6 +564,10 @@ static int gen8_reset_engines(struct drm_i915_private *i915,
 		 */
 	}
 
+	/* We are about to do full GPU reset, don't forget about GuC */
+	if (engine_mask == ALL_ENGINES)
+		intel_uc_reset_prepare(i915);
+
 	if (INTEL_GEN(i915) >= 11)
 		ret = gen11_reset_engines(i915, engine_mask, retry);
 	else
-- 
2.19.2

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

Re: [PATCH 7/7] drm/i915/uc: Don't forget to prepare GuC for the reset

[Chris Wilson]

Quoting Michal Wajdeczko (2019-05-17 17:22:27)
> When we reset engines using ALL_ENGINES mask, we will do
> full GPU reset and GuC will be also affected. Let GuC be
> prepared for upcoming reset.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_reset.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> index 464369bc55ad..ca6e40b6b4e2 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -564,6 +564,10 @@ static int gen8_reset_engines(struct drm_i915_private *i915,
>                  */
>         }
>  
> +       /* We are about to do full GPU reset, don't forget about GuC */
> +       if (engine_mask == ALL_ENGINES)
> +               intel_uc_reset_prepare(i915);

Eh, this is done in reset_prepare already. The only other path to call
intel_gpu_reset() directly is along sanitization, which should also have
already sanitized the guc as well. No?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915/uc: Stop talking with GuC when resetting

[Chris Wilson]

Quoting Michal Wajdeczko (2019-05-17 17:22:24)
> Knowing that GuC will be reset soon, we may stop all communication
> immediately without doing graceful cleanup as it is not needed.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_ct.h |  5 +++++
>  drivers/gpu/drm/i915/intel_uc.c     | 13 ++++++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.h b/drivers/gpu/drm/i915/intel_guc_ct.h
> index f5e7f0663304..41ba593a4df7 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ct.h
> +++ b/drivers/gpu/drm/i915/intel_guc_ct.h
> @@ -96,4 +96,9 @@ void intel_guc_ct_fini(struct intel_guc_ct *ct);
>  int intel_guc_ct_enable(struct intel_guc_ct *ct);
>  void intel_guc_ct_disable(struct intel_guc_ct *ct);
>  
> +static inline void intel_guc_ct_stop(struct intel_guc_ct *ct)
> +{
> +       ct->host_channel.enabled = false;
> +}
> +
>  #endif /* _INTEL_GUC_CT_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 9d86cd831ea7..86edfa5ad72e 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -224,6 +224,17 @@ static int guc_enable_communication(struct intel_guc *guc)
>         return 0;
>  }
>  
> +static void guc_stop_communication(struct intel_guc *guc)
> +{
> +       struct drm_i915_private *i915 = guc_to_i915(guc);
> +
> +       if (HAS_GUC_CT(i915))

Does this not fall out of some local knowledge? Though for the moment it
is harmless to stop a non-existent intel_guc_ct.

> +               intel_guc_ct_stop(&guc->ct);

Any serialisation required here? Or caveats for callers to observe?

> +       guc->send = intel_guc_send_nop;
> +       guc->handler = intel_guc_to_host_event_handler_nop;
> +}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915/uc: Skip reset preparation if GuC is already dead

[Chris Wilson]

Quoting Michal Wajdeczko (2019-05-17 17:22:25)
> We may skip reset preparation steps if GuC is already sanitized.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 86edfa5ad72e..36c53a42927c 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -499,6 +499,9 @@ void intel_uc_reset_prepare(struct drm_i915_private *i915)
>         if (!USES_GUC(i915))
>                 return;
>  
> +       if (!intel_guc_is_alive(guc))
> +               return;

Does it not replace "if (!USES_GUC(i915))"?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915/uc: Stop GuC submission during reset prepare

[Chris Wilson]

Quoting Michal Wajdeczko (2019-05-17 17:22:26)
> +void intel_guc_submission_stop(struct intel_guc *guc)
> +{
> +       struct drm_i915_private *i915 = guc_to_i915(guc);
> +
> +       GEM_BUG_ON(i915->gt.awake); /* GT should be parked first */

How is this true for reset? Note, it's an unlocked read, so READ_ONCE()
or something along GEM_BUG_ON(intel_wakeref_active(&i915->gt.wakeref))

Though I think this is wrong!

i915_reset or i915_gem_set_wedged
 -> reset_prepare
    -> intel_uc_reset_prepare
       -> intel_guc_submission_stop

No locks, no parking. So how does this survive selftests? :(
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm/i915/uc: Don't forget to prepare GuC for the reset

[Michal Wajdeczko]

On Fri, 17 May 2019 18:27:44 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2019-05-17 17:22:27)
>> When we reset engines using ALL_ENGINES mask, we will do
>> full GPU reset and GuC will be also affected. Let GuC be
>> prepared for upcoming reset.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> ---
>>  drivers/gpu/drm/i915/gt/intel_reset.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c  
>> b/drivers/gpu/drm/i915/gt/intel_reset.c
>> index 464369bc55ad..ca6e40b6b4e2 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
>> @@ -564,6 +564,10 @@ static int gen8_reset_engines(struct  
>> drm_i915_private *i915,
>>                  */
>>         }
>>
>> +       /* We are about to do full GPU reset, don't forget about GuC */
>> +       if (engine_mask == ALL_ENGINES)
>> +               intel_uc_reset_prepare(i915);
>
> Eh, this is done in reset_prepare already. The only other path to call
> intel_gpu_reset() directly is along sanitization, which should also have
> already sanitized the guc as well. No?

There is igt_atomic_reset selftest which does not call reset_prepare.
And since we lost GuC in gen6_reset_engines due to GEN6_GRDOM_FULL,
our later graceful goodbye with GuC was not working.

This is hidden with current GuC fw, but with new ICL fw with CT is was  
visible as:

<3> [508.613024] [drm:intel_guc_send_mmio [i915]] *ERROR* MMIO: GuC action  
0x4506 failed with error -110 0x4506
<3> [508.613142] [drm:guc_action_deregister_ct_buffer [i915]] *ERROR* CT:  
deregister SEND buffer failed; owner=0 err=-110
<3> [508.623521] [drm:intel_guc_send_mmio [i915]] *ERROR* MMIO: GuC action  
0x4506 failed with error -110 0x4506
<3> [508.623573] [drm:guc_action_deregister_ct_buffer [i915]] *ERROR* CT:  
deregister RECV buffer failed; owner=0 err=-110

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

Re: [PATCH 7/7] drm/i915/uc: Don't forget to prepare GuC for the reset

[Chris Wilson]

Quoting Michal Wajdeczko (2019-05-17 17:54:53)
> On Fri, 17 May 2019 18:27:44 +0200, Chris Wilson  
> <chris@chris-wilson.co.uk> wrote:
> 
> > Quoting Michal Wajdeczko (2019-05-17 17:22:27)
> >> When we reset engines using ALL_ENGINES mask, we will do
> >> full GPU reset and GuC will be also affected. Let GuC be
> >> prepared for upcoming reset.
> >>
> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/gt/intel_reset.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c  
> >> b/drivers/gpu/drm/i915/gt/intel_reset.c
> >> index 464369bc55ad..ca6e40b6b4e2 100644
> >> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> >> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> >> @@ -564,6 +564,10 @@ static int gen8_reset_engines(struct  
> >> drm_i915_private *i915,
> >>                  */
> >>         }
> >>
> >> +       /* We are about to do full GPU reset, don't forget about GuC */
> >> +       if (engine_mask == ALL_ENGINES)
> >> +               intel_uc_reset_prepare(i915);
> >
> > Eh, this is done in reset_prepare already. The only other path to call
> > intel_gpu_reset() directly is along sanitization, which should also have
> > already sanitized the guc as well. No?
> 
> There is igt_atomic_reset selftest which does not call reset_prepare.
> And since we lost GuC in gen6_reset_engines due to GEN6_GRDOM_FULL,
> our later graceful goodbye with GuC was not working.

Ok, the intent there was to avoid letting any sleeps slip into the
device reset itself (should we need to put it inside a heavy hammer like
stop_machine() to serialise it with direct user access to the hw). I
think it is fair to put that under reset_prepare / reset_finish. Would
also mean moving it out of selftest_hangcheck and into selftest_reset.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915/uc: Stop talking with GuC when resetting

[Michal Wajdeczko]

On Fri, 17 May 2019 18:30:40 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2019-05-17 17:22:24)
>> Knowing that GuC will be reset soon, we may stop all communication
>> immediately without doing graceful cleanup as it is not needed.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_guc_ct.h |  5 +++++
>>  drivers/gpu/drm/i915/intel_uc.c     | 13 ++++++++++++-
>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.h  
>> b/drivers/gpu/drm/i915/intel_guc_ct.h
>> index f5e7f0663304..41ba593a4df7 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_ct.h
>> +++ b/drivers/gpu/drm/i915/intel_guc_ct.h
>> @@ -96,4 +96,9 @@ void intel_guc_ct_fini(struct intel_guc_ct *ct);
>>  int intel_guc_ct_enable(struct intel_guc_ct *ct);
>>  void intel_guc_ct_disable(struct intel_guc_ct *ct);
>>
>> +static inline void intel_guc_ct_stop(struct intel_guc_ct *ct)
>> +{
>> +       ct->host_channel.enabled = false;
>> +}
>> +
>>  #endif /* _INTEL_GUC_CT_H_ */
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 9d86cd831ea7..86edfa5ad72e 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -224,6 +224,17 @@ static int guc_enable_communication(struct  
>> intel_guc *guc)
>>         return 0;
>>  }
>>
>> +static void guc_stop_communication(struct intel_guc *guc)
>> +{
>> +       struct drm_i915_private *i915 = guc_to_i915(guc);
>> +
>> +       if (HAS_GUC_CT(i915))
>
> Does this not fall out of some local knowledge? Though for the moment it
> is harmless to stop a non-existent intel_guc_ct.

We do have CT init/enable/disable/fini functions (all guarded by  
HAS_GUC_CT)
so at the moment we are here, we know that CT was already enabled.

>
>> +               intel_guc_ct_stop(&guc->ct);
>
> Any serialisation required here?

This function might be called from atomic context, so rather not.

> Or caveats for callers to observe?

As we are resetting the GuC, we simply does not allow any further
communication with it, any abusers will see something like this:

<4> [322.401614] Unexpected send: action=0x4000
<4> [322.401648] WARNING: CPU: 3 PID: 1357 at  
drivers/gpu/drm/i915/intel_guc.c:405 intel_guc_send_nop+0xe/0x20 [i915]

>
>> +       guc->send = intel_guc_send_nop;
>> +       guc->handler = intel_guc_to_host_event_handler_nop;
>> +}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm/i915/uc: Don't forget to prepare GuC for the reset

[Chris Wilson]

Quoting Michal Wajdeczko (2019-05-17 17:54:53)
> On Fri, 17 May 2019 18:27:44 +0200, Chris Wilson  
> <chris@chris-wilson.co.uk> wrote:
> 
> > Quoting Michal Wajdeczko (2019-05-17 17:22:27)
> >> When we reset engines using ALL_ENGINES mask, we will do
> >> full GPU reset and GuC will be also affected. Let GuC be
> >> prepared for upcoming reset.
> >>
> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/gt/intel_reset.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c  
> >> b/drivers/gpu/drm/i915/gt/intel_reset.c
> >> index 464369bc55ad..ca6e40b6b4e2 100644
> >> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> >> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> >> @@ -564,6 +564,10 @@ static int gen8_reset_engines(struct  
> >> drm_i915_private *i915,
> >>                  */
> >>         }
> >>
> >> +       /* We are about to do full GPU reset, don't forget about GuC */
> >> +       if (engine_mask == ALL_ENGINES)
> >> +               intel_uc_reset_prepare(i915);
> >
> > Eh, this is done in reset_prepare already. The only other path to call
> > intel_gpu_reset() directly is along sanitization, which should also have
> > already sanitized the guc as well. No?
> 
> There is igt_atomic_reset selftest which does not call reset_prepare.
> And since we lost GuC in gen6_reset_engines due to GEN6_GRDOM_FULL,
> our later graceful goodbye with GuC was not working.

(Apologies if resent.)

Ah, so the intent there was to prevent sleeps slipping into the
device-level reset (so that we could pull it under a heavy hammer like
stop_machine() to serialise the reset with direct user access). At one
point, I hoped we could make i915_reset() completely fast and lockless,
but that was a fleeting fancy.

It looks reasonable to put that under a reset_prepare/reset_finish and
keep the test semantics. That would also mean we have to move it out of
the increasingly misnamed selftest_hangcheck.c into a selftest_reset.c
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915/uc: Skip reset preparation if GuC is already dead

[Michal Wajdeczko]

On Fri, 17 May 2019 18:31:31 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2019-05-17 17:22:25)
>> We may skip reset preparation steps if GuC is already sanitized.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_uc.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 86edfa5ad72e..36c53a42927c 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -499,6 +499,9 @@ void intel_uc_reset_prepare(struct drm_i915_private  
>> *i915)
>>         if (!USES_GUC(i915))
>>                 return;
>>
>> +       if (!intel_guc_is_alive(guc))
>> +               return;
>
> Does it not replace "if (!USES_GUC(i915))"?

Yes it can, as we will never fetch/upload fw if we don't plan to use it ;)

Btw, I'm thinking of renaming intel_guc_is_alive to intel_guc_is_loaded
or intel_guc_is_started to better describe what this function is reporting,
as one can think that intel_guc_is_alive is actually checking that GuC fw
is responsive, which in general might not be the same as "loaded"

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

Re: [PATCH 7/7] drm/i915/uc: Don't forget to prepare GuC for the reset

[Chris Wilson]

Quoting Michal Wajdeczko (2019-05-17 17:54:53)
> On Fri, 17 May 2019 18:27:44 +0200, Chris Wilson  
> <chris@chris-wilson.co.uk> wrote:
> 
> > Quoting Michal Wajdeczko (2019-05-17 17:22:27)
> >> When we reset engines using ALL_ENGINES mask, we will do
> >> full GPU reset and GuC will be also affected. Let GuC be
> >> prepared for upcoming reset.
> >>
> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/gt/intel_reset.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c  
> >> b/drivers/gpu/drm/i915/gt/intel_reset.c
> >> index 464369bc55ad..ca6e40b6b4e2 100644
> >> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> >> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> >> @@ -564,6 +564,10 @@ static int gen8_reset_engines(struct  
> >> drm_i915_private *i915,
> >>                  */
> >>         }
> >>
> >> +       /* We are about to do full GPU reset, don't forget about GuC */
> >> +       if (engine_mask == ALL_ENGINES)
> >> +               intel_uc_reset_prepare(i915);
> >
> > Eh, this is done in reset_prepare already. The only other path to call
> > intel_gpu_reset() directly is along sanitization, which should also have
> > already sanitized the guc as well. No?
> 
> There is igt_atomic_reset selftest which does not call reset_prepare.
> And since we lost GuC in gen6_reset_engines due to GEN6_GRDOM_FULL,
> our later graceful goodbye with GuC was not working.
> 
> This is hidden with current GuC fw, but with new ICL fw with CT is was  
> visible as:

Imagine we fix the selftest, is there a GEM_BUG_ON(intel_uc_active()) we
could put here to enforce sanitization first? Does such a assert want to
be here or up a level in intel_gpu_reset()? 
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915/uc: Skip reset preparation if GuC is already dead

[Chris Wilson]

Quoting Michal Wajdeczko (2019-05-17 18:11:07)
> On Fri, 17 May 2019 18:31:31 +0200, Chris Wilson  
> <chris@chris-wilson.co.uk> wrote:
> 
> > Quoting Michal Wajdeczko (2019-05-17 17:22:25)
> >> We may skip reset preparation steps if GuC is already sanitized.
> >>
> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_uc.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> >> b/drivers/gpu/drm/i915/intel_uc.c
> >> index 86edfa5ad72e..36c53a42927c 100644
> >> --- a/drivers/gpu/drm/i915/intel_uc.c
> >> +++ b/drivers/gpu/drm/i915/intel_uc.c
> >> @@ -499,6 +499,9 @@ void intel_uc_reset_prepare(struct drm_i915_private  
> >> *i915)
> >>         if (!USES_GUC(i915))
> >>                 return;
> >>
> >> +       if (!intel_guc_is_alive(guc))
> >> +               return;
> >
> > Does it not replace "if (!USES_GUC(i915))"?
> 
> Yes it can, as we will never fetch/upload fw if we don't plan to use it ;)
> 
> Btw, I'm thinking of renaming intel_guc_is_alive to intel_guc_is_loaded
> or intel_guc_is_started to better describe what this function is reporting,
> as one can think that intel_guc_is_alive is actually checking that GuC fw
> is responsive, which in general might not be the same as "loaded"

Either seems reasonable, though there might be good reason to have both
:)

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

Re: [PATCH 5/7] drm/i915/uc: Skip reset preparation if GuC is already dead

[Michal Wajdeczko]

On Fri, 17 May 2019 19:14:01 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2019-05-17 18:11:07)
>> On Fri, 17 May 2019 18:31:31 +0200, Chris Wilson
>> <chris@chris-wilson.co.uk> wrote:
>>
>> > Quoting Michal Wajdeczko (2019-05-17 17:22:25)
>> >> We may skip reset preparation steps if GuC is already sanitized.
>> >>
>> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/intel_uc.c | 3 +++
>> >>  1 file changed, 3 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>> >> b/drivers/gpu/drm/i915/intel_uc.c
>> >> index 86edfa5ad72e..36c53a42927c 100644
>> >> --- a/drivers/gpu/drm/i915/intel_uc.c
>> >> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> >> @@ -499,6 +499,9 @@ void intel_uc_reset_prepare(struct  
>> drm_i915_private
>> >> *i915)
>> >>         if (!USES_GUC(i915))
>> >>                 return;
>> >>
>> >> +       if (!intel_guc_is_alive(guc))
>> >> +               return;
>> >
>> > Does it not replace "if (!USES_GUC(i915))"?
>>
>> Yes it can, as we will never fetch/upload fw if we don't plan to use it  
>> ;)
>>
>> Btw, I'm thinking of renaming intel_guc_is_alive to intel_guc_is_loaded
>> or intel_guc_is_started to better describe what this function is  
>> reporting,
>> as one can think that intel_guc_is_alive is actually checking that GuC  
>> fw
>> is responsive, which in general might not be the same as "loaded"
>
> Either seems reasonable, though there might be good reason to have both
> :)
>
> intel_guc_is_loaded
> intel_guc_has_started / intel_guc_is_active

On GuC load failure, or on reset, we immediately sanitize fw load status,
so until we provide real runtime connectivity check, if ever be required,
I assume we can stay with just one function: intel_guc_is_loaded, ok?

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

Re: [PATCH 5/7] drm/i915/uc: Skip reset preparation if GuC is already dead

[Chris Wilson]

Quoting Michal Wajdeczko (2019-05-17 19:01:06)
> On Fri, 17 May 2019 19:14:01 +0200, Chris Wilson  
> <chris@chris-wilson.co.uk> wrote:
> 
> > Quoting Michal Wajdeczko (2019-05-17 18:11:07)
> >> On Fri, 17 May 2019 18:31:31 +0200, Chris Wilson
> >> <chris@chris-wilson.co.uk> wrote:
> >>
> >> > Quoting Michal Wajdeczko (2019-05-17 17:22:25)
> >> >> We may skip reset preparation steps if GuC is already sanitized.
> >> >>
> >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/intel_uc.c | 3 +++
> >> >>  1 file changed, 3 insertions(+)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> >> >> b/drivers/gpu/drm/i915/intel_uc.c
> >> >> index 86edfa5ad72e..36c53a42927c 100644
> >> >> --- a/drivers/gpu/drm/i915/intel_uc.c
> >> >> +++ b/drivers/gpu/drm/i915/intel_uc.c
> >> >> @@ -499,6 +499,9 @@ void intel_uc_reset_prepare(struct  
> >> drm_i915_private
> >> >> *i915)
> >> >>         if (!USES_GUC(i915))
> >> >>                 return;
> >> >>
> >> >> +       if (!intel_guc_is_alive(guc))
> >> >> +               return;
> >> >
> >> > Does it not replace "if (!USES_GUC(i915))"?
> >>
> >> Yes it can, as we will never fetch/upload fw if we don't plan to use it  
> >> ;)
> >>
> >> Btw, I'm thinking of renaming intel_guc_is_alive to intel_guc_is_loaded
> >> or intel_guc_is_started to better describe what this function is  
> >> reporting,
> >> as one can think that intel_guc_is_alive is actually checking that GuC  
> >> fw
> >> is responsive, which in general might not be the same as "loaded"
> >
> > Either seems reasonable, though there might be good reason to have both
> > :)
> >
> > intel_guc_is_loaded
> > intel_guc_has_started / intel_guc_is_active
> 
> On GuC load failure, or on reset, we immediately sanitize fw load status,
> so until we provide real runtime connectivity check, if ever be required,
> I assume we can stay with just one function: intel_guc_is_loaded, ok?

Fine by me,
-Quack
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915/uc: Skip GuC HW unwinding if GuC is already dead

[Chris Wilson]

Quoting Michal Wajdeczko (2019-05-17 17:22:23)
> We should not attempt to unwind GuC hardware/firmware setup
> if we already have sanitized GuC.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 01683d107348..9d86cd831ea7 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -465,6 +465,9 @@ void intel_uc_fini_hw(struct drm_i915_private *i915)
>  
>         GEM_BUG_ON(!HAS_GUC(i915));
>  
> +       if (!intel_guc_is_alive(guc))
> +               return;
> +

Something else along these lines would be avoiding

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

I presume you can think of something a bit more precise to skip guc
selftests if guc initialisation failed.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915/uc: Skip reset preparation if GuC is already dead

[Chris Wilson]

Quoting Michal Wajdeczko (2019-05-17 19:01:06)
> On Fri, 17 May 2019 19:14:01 +0200, Chris Wilson  
> <chris@chris-wilson.co.uk> wrote:
> 
> > Quoting Michal Wajdeczko (2019-05-17 18:11:07)
> >> On Fri, 17 May 2019 18:31:31 +0200, Chris Wilson
> >> <chris@chris-wilson.co.uk> wrote:
> >>
> >> > Quoting Michal Wajdeczko (2019-05-17 17:22:25)
> >> >> We may skip reset preparation steps if GuC is already sanitized.
> >> >>
> >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/intel_uc.c | 3 +++
> >> >>  1 file changed, 3 insertions(+)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> >> >> b/drivers/gpu/drm/i915/intel_uc.c
> >> >> index 86edfa5ad72e..36c53a42927c 100644
> >> >> --- a/drivers/gpu/drm/i915/intel_uc.c
> >> >> +++ b/drivers/gpu/drm/i915/intel_uc.c
> >> >> @@ -499,6 +499,9 @@ void intel_uc_reset_prepare(struct  
> >> drm_i915_private
> >> >> *i915)
> >> >>         if (!USES_GUC(i915))
> >> >>                 return;
> >> >>
> >> >> +       if (!intel_guc_is_alive(guc))
> >> >> +               return;
> >> >
> >> > Does it not replace "if (!USES_GUC(i915))"?
> >>
> >> Yes it can, as we will never fetch/upload fw if we don't plan to use it  
> >> ;)
> >>
> >> Btw, I'm thinking of renaming intel_guc_is_alive to intel_guc_is_loaded
> >> or intel_guc_is_started to better describe what this function is  
> >> reporting,
> >> as one can think that intel_guc_is_alive is actually checking that GuC  
> >> fw
> >> is responsive, which in general might not be the same as "loaded"
> >
> > Either seems reasonable, though there might be good reason to have both
> > :)
> >
> > intel_guc_is_loaded
> > intel_guc_has_started / intel_guc_is_active
> 
> On GuC load failure, or on reset, we immediately sanitize fw load status,
> so until we provide real runtime connectivity check, if ever be required,
> I assume we can stay with just one function: intel_guc_is_loaded, ok?

Would a similar one for huc also work? Would it be reliable enough to
replace HUC_STATUS query? (Seems silly to wake the device up just to
answer if we've loaded the firmware successfully.)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915/uc: Skip reset preparation if GuC is already dead

[Michal Wajdeczko]

On Fri, 17 May 2019 22:08:56 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2019-05-17 19:01:06)
>> On Fri, 17 May 2019 19:14:01 +0200, Chris Wilson
>> <chris@chris-wilson.co.uk> wrote:
>>
>> > Quoting Michal Wajdeczko (2019-05-17 18:11:07)
>> >> On Fri, 17 May 2019 18:31:31 +0200, Chris Wilson
>> >> <chris@chris-wilson.co.uk> wrote:
>> >>
>> >> > Quoting Michal Wajdeczko (2019-05-17 17:22:25)
>> >> >> We may skip reset preparation steps if GuC is already sanitized.
>> >> >>
>> >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> >> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> >> >> ---
>> >> >>  drivers/gpu/drm/i915/intel_uc.c | 3 +++
>> >> >>  1 file changed, 3 insertions(+)
>> >> >>
>> >> >> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>> >> >> b/drivers/gpu/drm/i915/intel_uc.c
>> >> >> index 86edfa5ad72e..36c53a42927c 100644
>> >> >> --- a/drivers/gpu/drm/i915/intel_uc.c
>> >> >> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> >> >> @@ -499,6 +499,9 @@ void intel_uc_reset_prepare(struct
>> >> drm_i915_private
>> >> >> *i915)
>> >> >>         if (!USES_GUC(i915))
>> >> >>                 return;
>> >> >>
>> >> >> +       if (!intel_guc_is_alive(guc))
>> >> >> +               return;
>> >> >
>> >> > Does it not replace "if (!USES_GUC(i915))"?
>> >>
>> >> Yes it can, as we will never fetch/upload fw if we don't plan to use  
>> it
>> >> ;)
>> >>
>> >> Btw, I'm thinking of renaming intel_guc_is_alive to  
>> intel_guc_is_loaded
>> >> or intel_guc_is_started to better describe what this function is
>> >> reporting,
>> >> as one can think that intel_guc_is_alive is actually checking that  
>> GuC
>> >> fw
>> >> is responsive, which in general might not be the same as "loaded"
>> >
>> > Either seems reasonable, though there might be good reason to have  
>> both
>> > :)
>> >
>> > intel_guc_is_loaded
>> > intel_guc_has_started / intel_guc_is_active
>>
>> On GuC load failure, or on reset, we immediately sanitize fw load  
>> status,
>> so until we provide real runtime connectivity check, if ever be  
>> required,
>> I assume we can stay with just one function: intel_guc_is_loaded, ok?
>
> Would a similar one for huc also work? Would it be reliable enough to
> replace HUC_STATUS query? (Seems silly to wake the device up just to
> answer if we've loaded the firmware successfully.)

I'm not sure that we can drop HUC_STATUS query as maybe this bit can
go off (who can confirm that?) but we definitely can replace HAS_HUC
(btw it should be USES_HUC) with new intel_huc_is_loaded

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

✓ Fi.CI.IGT: success for GuC fixes

[Patchwork]

== Series Details ==

Series: GuC fixes
URL   : https://patchwork.freedesktop.org/series/60795/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6096_full -> Patchwork_13035_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

  Here are the changes found in Patchwork_13035_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_bad_reloc@negative-reloc-default:
    - shard-apl:          [PASS][1] -> [INCOMPLETE][2] ([fdo#103927])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-apl8/igt@gem_bad_reloc@negative-reloc-default.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-apl4/igt@gem_bad_reloc@negative-reloc-default.html

  * igt@gem_mmap_gtt@forked-big-copy:
    - shard-iclb:         [PASS][3] -> [TIMEOUT][4] ([fdo#109673])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-iclb3/igt@gem_mmap_gtt@forked-big-copy.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-iclb1/igt@gem_mmap_gtt@forked-big-copy.html

  * igt@gem_workarounds@suspend-resume-context:
    - shard-apl:          [PASS][5] -> [DMESG-WARN][6] ([fdo#108566]) +8 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-apl5/igt@gem_workarounds@suspend-resume-context.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-apl7/igt@gem_workarounds@suspend-resume-context.html

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-iclb:         [PASS][7] -> [SKIP][8] ([fdo#109349])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-iclb3/igt@kms_dp_dsc@basic-dsc-enable-edp.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-snb:          [PASS][9] -> [INCOMPLETE][10] ([fdo#105411])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-snb4/igt@kms_flip@flip-vs-suspend-interruptible.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-snb1/igt@kms_flip@flip-vs-suspend-interruptible.html
    - shard-hsw:          [PASS][11] -> [INCOMPLETE][12] ([fdo#103540])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-hsw6/igt@kms_flip@flip-vs-suspend-interruptible.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-hsw5/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render:
    - shard-iclb:         [PASS][13] -> [FAIL][14] ([fdo#103167]) +5 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-iclb5/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-iclb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render.html

  * igt@kms_psr@psr2_cursor_plane_onoff:
    - shard-iclb:         [PASS][15] -> [SKIP][16] ([fdo#109441])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-iclb2/igt@kms_psr@psr2_cursor_plane_onoff.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-iclb3/igt@kms_psr@psr2_cursor_plane_onoff.html

  * igt@kms_psr@suspend:
    - shard-skl:          [PASS][17] -> [INCOMPLETE][18] ([fdo#107773])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-skl8/igt@kms_psr@suspend.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-skl8/igt@kms_psr@suspend.html

  * igt@perf_pmu@rc6:
    - shard-kbl:          [PASS][19] -> [SKIP][20] ([fdo#109271])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-kbl7/igt@perf_pmu@rc6.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-kbl2/igt@perf_pmu@rc6.html

  
#### Possible fixes ####

  * igt@i915_pm_rpm@pm-tiling:
    - shard-skl:          [INCOMPLETE][21] ([fdo#107807]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-skl8/igt@i915_pm_rpm@pm-tiling.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-skl8/igt@i915_pm_rpm@pm-tiling.html

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
    - shard-glk:          [FAIL][23] ([fdo#105363]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-glk8/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-glk2/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html

  * igt@kms_flip@2x-flip-vs-suspend:
    - shard-hsw:          [INCOMPLETE][25] ([fdo#103540]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-hsw5/igt@kms_flip@2x-flip-vs-suspend.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-hsw6/igt@kms_flip@2x-flip-vs-suspend.html

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-skl:          [FAIL][27] ([fdo#105363]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-skl7/igt@kms_flip@flip-vs-expired-vblank.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-skl10/igt@kms_flip@flip-vs-expired-vblank.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render:
    - shard-iclb:         [FAIL][29] ([fdo#103167]) -> [PASS][30] +4 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-iclb4/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-iclb2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
    - shard-apl:          [DMESG-WARN][31] ([fdo#108566]) -> [PASS][32] +1 similar issue
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-apl8/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-apl2/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html

  * igt@kms_psr@psr2_primary_mmap_cpu:
    - shard-iclb:         [SKIP][33] ([fdo#109441]) -> [PASS][34] +3 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-iclb8/igt@kms_psr@psr2_primary_mmap_cpu.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-iclb2/igt@kms_psr@psr2_primary_mmap_cpu.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [FAIL][35] ([fdo#99912]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-kbl1/igt@kms_setmode@basic.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-kbl1/igt@kms_setmode@basic.html

  * igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend:
    - shard-snb:          [SKIP][37] ([fdo#109271]) -> [PASS][38] +1 similar issue
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-snb6/igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-snb7/igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend.html

  
#### Warnings ####

  * igt@gem_userptr_blits@process-exit-gtt-busy:
    - shard-apl:          [SKIP][39] ([fdo#109271]) -> [INCOMPLETE][40] ([fdo#103927])
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-apl3/igt@gem_userptr_blits@process-exit-gtt-busy.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-apl6/igt@gem_userptr_blits@process-exit-gtt-busy.html

  * igt@i915_pm_rpm@dpms-non-lpsp:
    - shard-skl:          [INCOMPLETE][41] ([fdo#107807]) -> [SKIP][42] ([fdo#109271])
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6096/shard-skl7/igt@i915_pm_rpm@dpms-non-lpsp.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13035/shard-skl9/igt@i915_pm_rpm@dpms-non-lpsp.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109673]: https://bugs.freedesktop.org/show_bug.cgi?id=109673
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * Linux: CI_DRM_6096 -> Patchwork_13035

  CI_DRM_6096: beb32d3348a566a6aafa292c65e2d60a610479c4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4996: 6fe5d254ec1b9b47d61408e1b49a7339876bf1e7 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13035: b6d4e116e3f732747e8d89c5509aae10a85faf29 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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