[Intel-gfx] [PATCH] drm/i915/guc: Skip suspend/resume GuC action on platforms w/o GuC submission
Hiatt, Don
don.hiatt at intel.com
Fri Nov 15 23:15:10 UTC 2019
> From: Ceraolo Spurio, Daniele <daniele.ceraolospurio at intel.com>
> Sent: Friday, November 15, 2019 3:10 PM
> To: Hiatt, Don <don.hiatt at intel.com>; intel-gfx at lists.freedesktop.org
> Cc: Ewins, Jon <jon.ewins at intel.com>; KiteStramuort
> <kitestramuort at autistici.org>; S . Zharkoff <s.zharkoff at gmail.com>;
> Wajdeczko, Michal <Michal.Wajdeczko at intel.com>; Summers, Stuart
> <stuart.summers at intel.com>; Chris Wilson <chris at chris-wilson.co.uk>; Tomas
> Janousek <tomi at nomi.cz>
> Subject: Re: [PATCH] drm/i915/guc: Skip suspend/resume GuC action on
> platforms w/o GuC submission
>
>
>
> On 11/15/2019 2:22 PM, Hiatt, Don wrote:
> >
> >> From: Ceraolo Spurio, Daniele <daniele.ceraolospurio at intel.com>
> >> Sent: Friday, November 15, 2019 2:02 PM
> >> To: Hiatt, Don <don.hiatt at intel.com>; intel-gfx at lists.freedesktop.org
> >> Cc: Ewins, Jon <jon.ewins at intel.com>; KiteStramuort
> >> <kitestramuort at autistici.org>; S . Zharkoff <s.zharkoff at gmail.com>;
> >> Wajdeczko, Michal <Michal.Wajdeczko at intel.com>; Summers, Stuart
> >> <stuart.summers at intel.com>; Chris Wilson <chris at chris-wilson.co.uk>;
> Tomas
> >> Janousek <tomi at nomi.cz>
> >> 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 at intel.com wrote:
> >>> From: Don Hiatt <don.hiatt at intel.com>
> >>>
> >>> 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 at autistici.org>
> >>> Reported-by: S. Zharkoff <s.zharkoff at gmail.com>
> >>> 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").
>
Yes, we do as the original reports was with the user specifically enabling
with the driver parameters.
Thanks a lot! The update patch will be posted in a second.
> Daniele
>
> >
> >>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> >>> Cc: Daniele Ceralo Spurio <daniele.ceraolospurio at intel.com>
> >>> Cc: Stuart Summers <stuart.summers at intel.com>
> >>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >>> Tested-by: Tomas Janousek <tomi at nomi.cz>
> >>> Signed-off-by: Don Hiatt <don.hiatt at intel.com>
> >>> ---
> >>> 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
> >>>
More information about the Intel-gfx
mailing list