Quoting Daniele Ceraolo Spurio (2019-07-30 17:05:19) > > > On 7/30/19 1:14 AM, Chris Wilson wrote: > > Quoting Daniele Ceraolo Spurio (2019-07-29 23:28:00) > >> When coming out of S3/S4 we sanitize and re-init the HW, which includes > >> enabling communication during uc_init_hw. We therefore don't want to do > >> that again in uc_resume and can just tell GuC to reload its state. > >> > >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospu...@intel.com> > >> Cc: Michal Wajdeczko <michal.wajdec...@intel.com> > >> Cc: Chris Wilson <ch...@chris-wilson.co.uk> > >> --- > >> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 16 +++++++++++++++- > >> 1 file changed, 15 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >> index fafa9be1e12a..34706a753793 100644 > >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >> @@ -233,11 +233,18 @@ static void guc_disable_interrupts(struct intel_guc > >> *guc) > >> guc->interrupts.disable(guc); > >> } > >> > >> +static bool guc_communication_enabled(struct intel_guc *guc) > >> +{ > >> + return guc->send != intel_guc_send_nop; > >> +} > >> + > >> static int guc_enable_communication(struct intel_guc *guc) > >> { > >> struct drm_i915_private *i915 = guc_to_gt(guc)->i915; > >> int ret; > >> > >> + GEM_BUG_ON(guc_communication_enabled(guc)); > >> + > >> ret = intel_guc_ct_enable(&guc->ct); > >> if (ret) > >> return ret; > >> @@ -558,7 +565,14 @@ int intel_uc_resume(struct intel_uc *uc) > >> if (!intel_guc_is_running(guc)) > >> return 0; > >> > >> - guc_enable_communication(guc); > >> + /* > >> + * When coming out of S3/S4 we sanitize and re-init the HW, so > >> + * communication is already re-enabled at this point and we just > >> need > >> + * to tell GuC to reload its internal state. During runtime resume > >> we > >> + * instead want to re-init everything. > >> + */ > >> + if (!guc_communication_enabled(guc)) > >> + guc_enable_communication(guc); > > > > We distinguish runtime_suspend from suspend, but not runtime_resume from > > resume. Is that distinction worthwhile, could we use it be more strict > > over the expected state? > > The actual actions we want to perform in both cases are the same, apart > from the communication side. What about: > > static int __uc_resume(struct intel_uc *uc, bool enable_comm) > { > if (!intel_guc_is_running()) > return 0; > > GEM_BUG_ON(enable comm == guc_communication_enabled()); > > if (enable_comm) > guc_enable_communication(); > > err = intel_guc_resume(); > } > > intel_uc_runtime_resume() > { > __uc_resume(uc, true); > } > > intel_uc_resume() > { > __uc_resume(uc, false); > }
That works for me. Having uc->suspend unnerved me worrying about what happens if gets out of sync. I like having the link from drm_resume -> uc_resume and drm_runtime_resume -> uc_runtime_resume. -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx