On 10/12/2017 2:38 PM, Joonas Lahtinen wrote:
On Wed, 2017-10-11 at 20:20 +0200, Michal Wajdeczko wrote:
On Wed, 11 Oct 2017 20:09:10 +0200, Sagar Arun Kamble
<sagar.a.kamble@xxxxxxxxx> wrote:
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
Hmm, so maybe we should call intel_sanitize_options() from or right after
i915_driver_init_early() ? It looks that all 'sanitize-options' are using
only device info flags, there is no MMIO access. Chris/Joonas?
Will be hard to answer any question on patch 19/21, when the preceding
patches are still under debate.
What I meant with focusing on single series at a time is that we first
discuss on any changes to get the first patches merged in reasonable
sized chunks of few patches per merge. During that discussion, lets
hold the dependent series from the mailing list for a while, because
the existing patches will cause changes and invalidate the reviews.
Now I have multiple dozens of e-mails in my inbox, and we're not going
to make progress that way.
Sorry. I thought the first reorg series that was merged last time was
the only gate for GEM/GuC fixes series.
There are not a lot conflicts with current reorg series. But I now
understand that I should have held onto the new revision.
Will wait for these changes to go in.
My understanding is that Michal's patches on
better organizing the GuC code are the first ones, so lets focus on
those first. So lets get back to this series after we're done with the
code organization sanitization.
Regards, Joonas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
