| Message ID | 20190729222800.1010-1-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/uc: don't enable communication twice on resume | expand |
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.ceraolospurio@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@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? -Chris
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.ceraolospurio@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Chris Wilson <chris@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? > -Chris > 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); } Daniele
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.ceraolospurio@intel.com> > >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > >> Cc: Chris Wilson <chris@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
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); err = intel_guc_resume(guc); if (err) {
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.ceraolospurio@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/gt/uc/intel_uc.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)