| Message ID | 20181016224648.2326-1-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] drm/i915/guc: fix GuC suspend/resume | expand |
On Wed, 17 Oct 2018 00:46:47 +0200, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote: > The ENTER/EXIT_S_STATE actions queue the save/restore operation in GuC > FW and then return, so waiting on the H2G is not enough to guarantee > GuC is done. > When all the processing is done, GuC writes 0 to scratch register 14, > so we can poll on that. Note that GuC does not ensure that the value > in the register is different from 0 while the action is in progress > so we need to take care of that ourselves as well. > > v2: improve comment, return early on GuC error and improve error > message (Michal) > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Acked-by: Chris Wilson <chris@chris-wilson.co.uk> > --- Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Michal
Quoting Michal Wajdeczko (2018-10-17 17:40:50) > On Wed, 17 Oct 2018 00:46:47 +0200, Daniele Ceraolo Spurio > <daniele.ceraolospurio@intel.com> wrote: > > > The ENTER/EXIT_S_STATE actions queue the save/restore operation in GuC > > FW and then return, so waiting on the H2G is not enough to guarantee > > GuC is done. > > When all the processing is done, GuC writes 0 to scratch register 14, > > so we can poll on that. Note that GuC does not ensure that the value > > in the register is different from 0 while the action is in progress > > so we need to take care of that ourselves as well. > > > > v2: improve comment, return early on GuC error and improve error > > message (Michal) > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > Acked-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Applied, thanks for the fix and review. -Chris
On Wed, 17 Oct 2018 00:46:47 +0200, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote: /snip/ > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h > b/drivers/gpu/drm/i915/intel_guc_fwif.h > index 8382d591c784..1a853cc627e3 100644 > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h > @@ -687,6 +687,13 @@ enum intel_guc_report_status { > INTEL_GUC_REPORT_STATUS_COMPLETE = 0x4, > }; > +enum intel_guc_sleep_state_status { > + INTEL_GUC_SLEEP_STATE_SUCCESS = 0x0, > + INTEL_GUC_SLEEP_STATE_PREEMPT_TO_IDLE_FAILED = 0x1, > + INTEL_GUC_SLEEP_STATE_ENGINE_RESET_FAILED = 0x2 > +}; btw, it used to be 0,1,2 but from some time fw defines above as: INTEL_GUC_SLEEP_STATE_SUCCESS = 0x1, INTEL_GUC_SLEEP_STATE_PREEMPT_TO_IDLE_FAILED = 0x2, INTEL_GUC_SLEEP_STATE_ENGINE_RESET_FAILED = 0x3, Michal
On 26/11/2018 06:51, Michal Wajdeczko wrote: > On Wed, 17 Oct 2018 00:46:47 +0200, Daniele Ceraolo Spurio > <daniele.ceraolospurio@intel.com> wrote: > > /snip/ > >> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h >> b/drivers/gpu/drm/i915/intel_guc_fwif.h >> index 8382d591c784..1a853cc627e3 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h >> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h >> @@ -687,6 +687,13 @@ enum intel_guc_report_status { >> INTEL_GUC_REPORT_STATUS_COMPLETE = 0x4, >> }; >> +enum intel_guc_sleep_state_status { >> + INTEL_GUC_SLEEP_STATE_SUCCESS = 0x0, >> + INTEL_GUC_SLEEP_STATE_PREEMPT_TO_IDLE_FAILED = 0x1, >> + INTEL_GUC_SLEEP_STATE_ENGINE_RESET_FAILED = 0x2 >> +}; > > btw, it used to be 0,1,2 but from some time fw defines above as: > > INTEL_GUC_SLEEP_STATE_SUCCESS = 0x1, > INTEL_GUC_SLEEP_STATE_PREEMPT_TO_IDLE_FAILED = 0x2, > INTEL_GUC_SLEEP_STATE_ENGINE_RESET_FAILED = 0x3, > > Michal Yeah, I think I had already mentioned in some reply that the newer firmware does suspend/resume differently, but I haven't looked at the details. I'm not even sure if polling the register will still be required. Daniele
On 27/11/2018 11:34, Daniele Ceraolo Spurio wrote: > > > On 26/11/2018 06:51, Michal Wajdeczko wrote: >> On Wed, 17 Oct 2018 00:46:47 +0200, Daniele Ceraolo Spurio >> <daniele.ceraolospurio@intel.com> wrote: >> >> /snip/ >> >>> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h >>> b/drivers/gpu/drm/i915/intel_guc_fwif.h >>> index 8382d591c784..1a853cc627e3 100644 >>> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h >>> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h >>> @@ -687,6 +687,13 @@ enum intel_guc_report_status { >>> INTEL_GUC_REPORT_STATUS_COMPLETE = 0x4, >>> }; >>> +enum intel_guc_sleep_state_status { >>> + INTEL_GUC_SLEEP_STATE_SUCCESS = 0x0, >>> + INTEL_GUC_SLEEP_STATE_PREEMPT_TO_IDLE_FAILED = 0x1, >>> + INTEL_GUC_SLEEP_STATE_ENGINE_RESET_FAILED = 0x2 >>> +}; >> >> btw, it used to be 0,1,2 but from some time fw defines above as: >> >> INTEL_GUC_SLEEP_STATE_SUCCESS = 0x1, >> INTEL_GUC_SLEEP_STATE_PREEMPT_TO_IDLE_FAILED = 0x2, >> INTEL_GUC_SLEEP_STATE_ENGINE_RESET_FAILED = 0x3, >> >> Michal > > Yeah, I think I had already mentioned in some reply that the newer > firmware does suspend/resume differently, but I haven't looked at the > details. I'm not even sure if polling the register will still be required. > > Daniele I've confirmed with the GuC team that the differences are mostly internal to GuC and the only change from the kernel perspective is that the enum values have changed. We still need to do the polling, but I guess we'll be able to init the register to zero since all the return values are > 0. Daniele > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c index 230aea69385d..4c61eb94527a 100644 --- a/drivers/gpu/drm/i915/intel_guc.c +++ b/drivers/gpu/drm/i915/intel_guc.c @@ -521,6 +521,44 @@ int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset) return intel_guc_send(guc, action, ARRAY_SIZE(action)); } +/* + * The ENTER/EXIT_S_STATE actions queue the save/restore operation in GuC FW and + * then return, so waiting on the H2G is not enough to guarantee GuC is done. + * When all the processing is done, GuC writes INTEL_GUC_SLEEP_STATE_SUCCESS to + * scratch register 14, so we can poll on that. Note that GuC does not ensure + * that the value in the register is different from + * INTEL_GUC_SLEEP_STATE_SUCCESS while the action is in progress so we need to + * take care of that ourselves as well. + */ +static int guc_sleep_state_action(struct intel_guc *guc, + const u32 *action, u32 len) +{ + struct drm_i915_private *dev_priv = guc_to_i915(guc); + int ret; + u32 status; + + I915_WRITE(SOFT_SCRATCH(14), INTEL_GUC_SLEEP_STATE_INVALID_MASK); + + ret = intel_guc_send(guc, action, len); + if (ret) + return ret; + + ret = __intel_wait_for_register(dev_priv, SOFT_SCRATCH(14), + INTEL_GUC_SLEEP_STATE_INVALID_MASK, + 0, 0, 10, &status); + if (ret) + return ret; + + if (status != INTEL_GUC_SLEEP_STATE_SUCCESS) { + DRM_ERROR("GuC failed to change sleep state. " + "action=0x%x, err=%u\n", + action[0], status); + return -EIO; + } + + return 0; +} + /** * intel_guc_suspend() - notify GuC entering suspend state * @guc: the guc @@ -533,7 +571,7 @@ int intel_guc_suspend(struct intel_guc *guc) intel_guc_ggtt_offset(guc, guc->shared_data) }; - return intel_guc_send(guc, data, ARRAY_SIZE(data)); + return guc_sleep_state_action(guc, data, ARRAY_SIZE(data)); } /** @@ -571,7 +609,7 @@ int intel_guc_resume(struct intel_guc *guc) intel_guc_ggtt_offset(guc, guc->shared_data) }; - return intel_guc_send(guc, data, ARRAY_SIZE(data)); + return guc_sleep_state_action(guc, data, ARRAY_SIZE(data)); } /** diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h index 8382d591c784..1a853cc627e3 100644 --- a/drivers/gpu/drm/i915/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h @@ -687,6 +687,13 @@ enum intel_guc_report_status { INTEL_GUC_REPORT_STATUS_COMPLETE = 0x4, }; +enum intel_guc_sleep_state_status { + INTEL_GUC_SLEEP_STATE_SUCCESS = 0x0, + INTEL_GUC_SLEEP_STATE_PREEMPT_TO_IDLE_FAILED = 0x1, + INTEL_GUC_SLEEP_STATE_ENGINE_RESET_FAILED = 0x2 +}; +#define INTEL_GUC_SLEEP_STATE_INVALID_MASK 0x80000000 + #define GUC_LOG_CONTROL_LOGGING_ENABLED (1 << 0) #define GUC_LOG_CONTROL_VERBOSITY_SHIFT 4 #define GUC_LOG_CONTROL_VERBOSITY_MASK (0xF << GUC_LOG_CONTROL_VERBOSITY_SHIFT)
The ENTER/EXIT_S_STATE actions queue the save/restore operation in GuC FW and then return, so waiting on the H2G is not enough to guarantee GuC is done. When all the processing is done, GuC writes 0 to scratch register 14, so we can poll on that. Note that GuC does not ensure that the value in the register is different from 0 while the action is in progress so we need to take care of that ourselves as well. v2: improve comment, return early on GuC error and improve error message (Michal) Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Acked-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/intel_guc.c | 42 +++++++++++++++++++++++++-- drivers/gpu/drm/i915/intel_guc_fwif.h | 7 +++++ 2 files changed, 47 insertions(+), 2 deletions(-)