> From: Ceraolo Spurio, Daniele <[email protected]> > Sent: Friday, November 15, 2019 3:10 PM > To: Hiatt, Don <[email protected]>; [email protected] > Cc: Ewins, Jon <[email protected]>; KiteStramuort > <[email protected]>; S . Zharkoff <[email protected]>; > Wajdeczko, Michal <[email protected]>; Summers, Stuart > <[email protected]>; Chris Wilson <[email protected]>; Tomas > Janousek <[email protected]> > 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 <[email protected]> > >> Sent: Friday, November 15, 2019 2:02 PM > >> To: Hiatt, Don <[email protected]>; [email protected] > >> Cc: Ewins, Jon <[email protected]>; KiteStramuort > >> <[email protected]>; S . Zharkoff <[email protected]>; > >> Wajdeczko, Michal <[email protected]>; Summers, Stuart > >> <[email protected]>; Chris Wilson <[email protected]>; > Tomas > >> Janousek <[email protected]> > >> Subject: Re: [PATCH] drm/i915/guc: Skip suspend/resume GuC action on > >> platforms w/o GuC submission > >> > >> > >> > >> On 11/15/19 10:20 AM, [email protected] wrote: > >>> From: Don Hiatt <[email protected]> > >>> > >>> 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 <[email protected]> > >>> Reported-by: S. Zharkoff <[email protected]> > >>> 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 <[email protected]> > >>> Cc: Daniele Ceralo Spurio <[email protected]> > >>> Cc: Stuart Summers <[email protected]> > >>> Cc: Chris Wilson <[email protected]> > >>> Tested-by: Tomas Janousek <[email protected]> > >>> Signed-off-by: Don Hiatt <[email protected]> > >>> --- > >>> 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 [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
