On 10/11/2017 11:28 PM, Michal Wajdeczko wrote:
On Wed, 11 Oct 2017 19:44:31 +0200, Sagar Arun Kamble
<sagar.a.kamble@xxxxxxxxx> wrote:
On 10/11/2017 11:05 PM, Michal Wajdeczko wrote:
On Wed, 11 Oct 2017 10:54:14 +0200, Sagar Arun Kamble
<sagar.a.kamble@xxxxxxxxx> wrote:
i915_ggtt_enable_guc has to happen first during i915_gem_resume
if GuC loading is enabled before GTT restore. In case GuC is not
loaded this enabling happening during intel_uc_init_hw need to
skipped. (avoid the GEM_BUG_ON)
i915_ggtt_disable_guc at the end of reset/suspend/unload is needed
post GGTT suspend operations. Calling it during uc_sanitize covers
all scenarios. Hence, it is removed from intel_uc_fini_hw. Also these
needto be protected by struct_mutex. Hence struct_mutex locking is
added in i915_gem_sanitize while sanitizing uC. struct_mutex is
already
held during i915_gem_reset_prepare.
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_gem.c | 4 ++++
drivers/gpu/drm/i915/intel_uc.c | 16 +++++++++++-----
2 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c
b/drivers/gpu/drm/i915/i915_gem.c
index a4bbf6c..77a0746 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4759,6 +4759,10 @@ void i915_gem_resume(struct drm_i915_private
*dev_priv)
WARN_ON(dev_priv->gt.awake);
mutex_lock(&dev->struct_mutex);
+ /* We need to notify the guc whenever we change the GGTT */
+ if (i915_modparams.enable_guc_loading)
+ i915_ggtt_enable_guc(dev_priv);
+
i915_gem_restore_gtt_mappings(dev_priv);
i915_gem_restore_fences(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_uc.c
b/drivers/gpu/drm/i915/intel_uc.c
index 9010ab5..0b799fe 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -184,8 +184,14 @@ int intel_uc_init_hw(struct drm_i915_private
*dev_priv)
guc_disable_communication(guc);
gen9_reset_guc_interrupts(dev_priv);
- /* We need to notify the guc whenever we change the GGTT */
- i915_ggtt_enable_guc(dev_priv);
+ /*
+ * We need to notify the guc whenever we change the GGTT.
+ * During resume from sleep we would have already updated the
+ * GGTT invalidate function for GuC during i915_gem_resume so
+ * we need to skip here. Will enable here on driver load/reset.
+ */
+ if (!guc->suspended)
+ i915_ggtt_enable_guc(dev_priv);
if (i915_modparams.enable_guc_submission) {
/*
@@ -309,9 +315,6 @@ void intel_uc_cleanup(struct drm_i915_private
*dev_priv)
guc_free_load_err_log(guc);
i915_guc_submission_cleanup(dev_priv);
-
- if (i915_modparams.enable_guc_loading)
- i915_ggtt_disable_guc(dev_priv);
}
/**
@@ -452,6 +455,9 @@ void intel_uc_sanitize(struct drm_i915_private
*dev_priv)
struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
if (i915_modparams.enable_guc_loading) {
+ if (guc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS)
Hmm, isn't that check redundant ?
uc_sanitize can happen without firmware loaded too in which case we
If uc_sanitize can be loaded without firmware loaded, then I assume
i915_modparams.enable_guc_loading will be cleared too, right ?
I'm just wondering if we need to check both modparam and fw status.
actually load time uc_sanitize is happening before uc_sanitize_options
so enable_guc_loading will have
non-zero value for some platforms. So I think it makes sense to limit
this to only load_status based.
don't want to ggtt_disable_guc.
if we want to ggtt_disable_guc then we should remove the GEM_BUG_ON
in it.
Hmm, this is the way how we run CI tests ;)
yes ... from long time :)
+ i915_ggtt_disable_guc(dev_priv);
+
guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
huc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
}
Btw, what should we do with "suspended" flag during sanitize ?
suspended flag is set to true on suspend and false on resume.
sanitize is done post suspend and before resume so we should not
touch it.
initializing it to false during guc_init_early should take care of
reload (during unload we are suspending gem but we wont resume)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
