Message ID | 20191028212527.24401-1-don.hiatt@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/i915/guc: Skip suspend/resume GuC action on platforms w/o GuC submission | expand |
On Mon, 28 Oct 2019 22:25:27 +0100, <don.hiatt@intel.com> wrote: > From: Don Hiatt <don.hiatt@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/remove entirely as we do > not need to save the GuC submission status. > > v2: Do not suspend/resume the GuC on platforms that do not support > Guc Submission. > > Signed-off-by: Don Hiatt <don.hiatt@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > index 3fdbc935d155..04031564f0b1 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > @@ -572,10 +572,19 @@ void intel_uc_runtime_suspend(struct intel_uc *uc) > if (!intel_guc_is_running(guc)) > return; > + /* > + * If GuC communciation is enabled but submission is not supported, typo > + * we do not need to suspend the GuC but we do need to disable the > + * GuC communication on suspend. > + */ > + if (!guc->submission_supported) Using submission_supported flag directly can be tricky, as today it is always set to false, but in the future it may indicate either that submission is supported by the driver/fw and/or enabled by modparam. There is no guarantee that it will reflect actual runtime status, as even supported/unblocked guc submission may fallback to execlists. We may need something like intel_guc_submission_is_active() that will reflect actual mode of submission currently used by the driver. > + goto guc_disable_comm; and maybe we can move above logic to intel_guc_suspend() to do not introduce extra goto's ? > + > err = intel_guc_suspend(guc); > if (err) > DRM_DEBUG_DRIVER("Failed to suspend GuC, err=%d", err); > +guc_disable_comm: > guc_disable_communication(guc); > } > @@ -605,6 +614,14 @@ static int __uc_resume(struct intel_uc *uc, bool > enable_communication) > if (enable_communication) > guc_enable_communication(guc); > + /* > + * If GuC communciation is enabled but submission is not supported, typo > + * we do not need to resume the GuC but we do need to enable the > + * GuC communication on resume (above). > + */ > + if (!guc->submission_supported) > + return 0; see suspend case comment > + > err = intel_guc_resume(guc); > if (err) { > DRM_DEBUG_DRIVER("Failed to resume GuC, err=%d", err); Thanks, Michal
> From: Wajdeczko, Michal <Michal.Wajdeczko@intel.com> > Sent: Tuesday, October 29, 2019 5:33 AM > To: intel-gfx@lists.freedesktop.org; Hiatt, Don <don.hiatt@intel.com> > Subject: Re: [Intel-gfx] [PATCH] drm/i915/guc: Skip suspend/resume GuC action > on platforms w/o GuC submission > > On Mon, 28 Oct 2019 22:25:27 +0100, <don.hiatt@intel.com> wrote: > > > From: Don Hiatt <don.hiatt@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/remove entirely as we do > > not need to save the GuC submission status. > > > > v2: Do not suspend/resume the GuC on platforms that do not support > > Guc Submission. > > > > Signed-off-by: Don Hiatt <don.hiatt@intel.com> > > --- > > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > index 3fdbc935d155..04031564f0b1 100644 > > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > @@ -572,10 +572,19 @@ void intel_uc_runtime_suspend(struct intel_uc *uc) > > if (!intel_guc_is_running(guc)) > > return; > > + /* > > + * If GuC communciation is enabled but submission is not supported, > > typo > > > + * we do not need to suspend the GuC but we do need to disable the > > + * GuC communication on suspend. > > + */ > > + if (!guc->submission_supported) > > Using submission_supported flag directly can be tricky, as today it > is always set to false, but in the future it may indicate either that > submission is supported by the driver/fw and/or enabled by modparam. > > There is no guarantee that it will reflect actual runtime status, > as even supported/unblocked guc submission may fallback to execlists. > > We may need something like intel_guc_submission_is_active() that will > reflect actual mode of submission currently used by the driver. Hi Michal, I looked at your patch wrt checking the set_default_submission vfunc but as that is for the engine, and here I only have access to the intel_guc struct. I'm not sure just where I can know what the default submission is and then flag it somewhere that I can then check here in the suspend/resume. I'll keep looking (sorry, I'm very new to this code).
On Tue, Oct 29, 2019 at 01:33:22PM +0100, Michal Wajdeczko wrote: > On Mon, 28 Oct 2019 22:25:27 +0100, <don.hiatt@intel.com> wrote: > > + * we do not need to suspend the GuC but we do need to disable the > > + * GuC communication on suspend. > > + */ > > + if (!guc->submission_supported) > > Using submission_supported flag directly can be tricky, as today it > is always set to false, but in the future it may indicate either that > submission is supported by the driver/fw and/or enabled by modparam. > > There is no guarantee that it will reflect actual runtime status, > as even supported/unblocked guc submission may fallback to execlists. > > We may need something like intel_guc_submission_is_active() that will > reflect actual mode of submission currently used by the driver. What about this: diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index ae45651ac73c..acda38a9fec5 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -522,10 +522,19 @@ void intel_uc_runtime_suspend(struct drm_i915_private *i915) if (!intel_guc_is_loaded(guc)) return; + /* + * If GuC communication is enabled but submission is not supported, + * we do not need to suspend the GuC but we do need to disable the + * GuC communication on suspend. + */ + if (!USES_GUC_SUBMISSION(i915)) + goto guc_disable_comm; + err = intel_guc_suspend(guc); if (err) DRM_DEBUG_DRIVER("Failed to suspend GuC, err=%d", err); +guc_disable_comm: guc_disable_communication(guc); } @@ -551,6 +560,14 @@ int intel_uc_resume(struct drm_i915_private *i915) 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 (!USES_GUC_SUBMISSION(i915)) + return 0; + err = intel_guc_resume(guc); if (err) { DRM_DEBUG_DRIVER("Failed to resume GuC, err=%d", err); This is what the backport to stable-5.3 will look like anyway as there's no submission_supported there. The name "USES_GUC_SUBMISSION" does suggest it might reflect actual runtime status... :-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index 3fdbc935d155..04031564f0b1 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -572,10 +572,19 @@ void intel_uc_runtime_suspend(struct intel_uc *uc) if (!intel_guc_is_running(guc)) return; + /* + * If GuC communciation is enabled but submission is not supported, + * we do not need to suspend the GuC but we do need to disable the + * GuC communication on suspend. + */ + if (!guc->submission_supported) + goto guc_disable_comm; + err = intel_guc_suspend(guc); if (err) DRM_DEBUG_DRIVER("Failed to suspend GuC, err=%d", err); +guc_disable_comm: guc_disable_communication(guc); } @@ -605,6 +614,14 @@ static int __uc_resume(struct intel_uc *uc, bool enable_communication) if (enable_communication) guc_enable_communication(guc); + /* + * If GuC communciation 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 (!guc->submission_supported) + return 0; + err = intel_guc_resume(guc); if (err) { DRM_DEBUG_DRIVER("Failed to resume GuC, err=%d", err);