On 11/15/2019 2:22 PM, Hiatt, Don wrote:
From: Ceraolo Spurio, Daniele <daniele.ceraolospurio@xxxxxxxxx>
Sent: Friday, November 15, 2019 2:02 PM
To: Hiatt, Don <don.hiatt@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Ewins, Jon <jon.ewins@xxxxxxxxx>; KiteStramuort
<kitestramuort@xxxxxxxxxxxxx>; S . Zharkoff <s.zharkoff@xxxxxxxxx>;
Wajdeczko, Michal <Michal.Wajdeczko@xxxxxxxxx>; Summers, Stuart
<stuart.summers@xxxxxxxxx>; Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>; Tomas
Janousek <tomi@xxxxxxx>
Subject: Re: [PATCH] drm/i915/guc: Skip suspend/resume GuC action on
platforms w/o GuC submission
On 11/15/19 10:20 AM, don.hiatt@xxxxxxxxx wrote:
From: Don Hiatt <don.hiatt@xxxxxxxxx>
On some platforms (e.g. KBL) that do not support GuC submission, but
the user enabled the GuC communication (e.g for HuC authentication)
calling the GuC EXIT_S_STATE action results in lose of ability to
enter RC6. We can remove the GuC suspend/resume entirely as we do
not need to save the GuC submission status.
Add intel_guc_submission_is_enabled() function to determine if
GuC submission is active.
v2: Do not suspend/resume the GuC on platforms that do not support
Guc Submission.
v3: Fix typo, move suspend logic to remove goto.
v4: Use intel_guc_submission_is_enabled() to check GuC submission
status.
v5: No need to look at engine to determine if submission is enabled.
Squash fix + intel_guc_submission_is_enabled() patch into one.
Reported-by: KiteStramuort <kitestramuort@xxxxxxxxxxxxx>
Reported-by: S. Zharkoff <s.zharkoff@xxxxxxxxx>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111594
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111623
Fixes: f774f0964919 ("drm/i915/guc: Turn on GuC/HuC auto mode")
I don't think this Fixes tag is appropriate since we already reverted
that patch.
Can you suggest an appropriate tag? Chris suggest "guc/huc enabling by default" patch
but I couldn't find it and this one seemed similar.
Do we want to backport this even if guc is off by default and taints on
enabling? if we do, then we probably need to go all the way back to when
we introduced the new binaries that show the problem ("drm/i915/guc:
Updates for GuC 32.0.3 firmware").
Daniele
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Daniele Ceralo Spurio <daniele.ceraolospurio@xxxxxxxxx>
Cc: Stuart Summers <stuart.summers@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Tested-by: Tomas Janousek <tomi@xxxxxxx>
Signed-off-by: Don Hiatt <don.hiatt@xxxxxxxxx>
---
drivers/gpu/drm/i915/gt/uc/intel_guc.c | 7 +++++++
drivers/gpu/drm/i915/gt/uc/intel_uc.c | 8 ++++++++
drivers/gpu/drm/i915/i915_drv.h | 6 ++++++
3 files changed, 21 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
index 019ae6486e8d..92d9305c0d73 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
@@ -553,6 +553,13 @@ int intel_guc_suspend(struct intel_guc *guc)
GUC_POWER_D1, /* any value greater than GUC_POWER_D0
*/
};
+ /*
+ * If GuC communication is enabled but submission is not supported,
+ * we do not need to suspend the GuC.
+ */
+ if (!intel_guc_submission_is_enabled(guc))
+ return 0;
+
/*
* The ENTER_S_STATE action queues the save/restore operation in GuC
FW
* and then returns, so waiting on the H2G is not enough to guarantee
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 629b19377a29..4dd43b99a334 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -605,6 +605,14 @@ static int __uc_resume(struct intel_uc *uc, bool
enable_communication)
if (enable_communication)
guc_enable_communication(guc);
+ /*
+ * If GuC communication is enabled but submission is not supported,
+ * we do not need to resume the GuC but we do need to enable the
+ * GuC communication on resume (above).
+ */
+ if (!intel_guc_submission_is_enabled(guc))
+ return 0;
+
I would put this check inside intel_guc_resume(), for symmetry with the
one you put inside intel_guc_suspend().
Will do.
Daniele
err = intel_guc_resume(guc);
if (err) {
DRM_DEBUG_DRIVER("Failed to resume GuC, err=%d", err);
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index 7bc1d8c102b2..2b879472e760 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2044,4 +2044,10 @@ i915_coherent_map_type(struct drm_i915_private
*i915)
return HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC;
}
+static inline bool intel_guc_submission_is_enabled(struct intel_guc *guc)
+{
+ return intel_guc_is_submission_supported(guc) &&
+ intel_guc_is_running(guc);
+}
+
#endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
