| Message ID | 1507712056-25030-11-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 11 Oct 2017 10:54:05 +0200, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote: > Suspending GuC involves bunch of tasks controlled by GuC OS and some > controlled by Host OS. > > Host needs to disable submission to GuC and any other GuC functions. > Then, > GuC's task is initiated by Host sending action to GuC to enter sleep > state. On this action, GuC preempts engines to idle context and then > saves > internal state to a buffer. It also disables internal interrupts/timers > to > avoid any wake-ups. > After this, Host should disable GuC interrupts, communication with GuC > (intel_guc_send/notify). GGTT invalidate update will have to be done in > conjunction with GTT related suspend/resume tasks. > > v2: Rebase w.r.t removal of GuC code restructuring. > > v3: Removed GuC specific helpers as tasks other than send H2G for > sleep/resume are to be done from uc generic functions. (Michal Wajdeczko) > > v4: Simplified/Unified the error messaging in uc_runtime_suspend/resume. > (Michal Wajdeczko). Rebase w.r.t i915_modparams change. > Added documentation to intel_uc_runtime_suspend/resume. > > v5: Removed enable_guc_loading based check from intel_uc_runtime_suspend > and intel_uc_runtime_resume and pulled FW load_status based checks from > intel_guc_suspend/resume into these functions. (Michal Wajdeczko) > > v6: Adjusted intel_uc_runtime_resume with prototype change to not return > value. > > v7: Rebase. > > v8: Updated commit description and added submission enable/disable in > GuC suspend/resume paths. Removed GGTT invalidate update functions. > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> #6 > --- > drivers/gpu/drm/i915/intel_guc.c | 11 ------- > drivers/gpu/drm/i915/intel_uc.c | 65 > +++++++++++++++++++++++++++++++++++++--- > 2 files changed, 61 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc.c > b/drivers/gpu/drm/i915/intel_guc.c > index 9a2df69..55a0158 100644 > --- a/drivers/gpu/drm/i915/intel_guc.c > +++ b/drivers/gpu/drm/i915/intel_guc.c > @@ -177,11 +177,6 @@ int intel_guc_suspend(struct intel_guc *guc) > struct i915_gem_context *ctx; > u32 data[3]; > - if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) > - return 0; > - > - gen9_disable_guc_interrupts(i915); > - > ctx = i915->kernel_context; > data[0] = INTEL_GUC_ACTION_ENTER_S_STATE; > @@ -204,12 +199,6 @@ int intel_guc_resume(struct intel_guc *guc) > struct i915_gem_context *ctx; > u32 data[3]; > - if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) > - return 0; > - > - if (i915_modparams.guc_log_level >= 0) > - gen9_enable_guc_interrupts(i915); > - > ctx = i915->kernel_context; > data[0] = INTEL_GUC_ACTION_EXIT_S_STATE; > diff --git a/drivers/gpu/drm/i915/intel_uc.c > b/drivers/gpu/drm/i915/intel_uc.c > index b5c132c..297a321 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -284,18 +284,75 @@ void intel_uc_fini_hw(struct drm_i915_private > *dev_priv) > i915_ggtt_disable_guc(dev_priv); > } > +/** > + * intel_uc_suspend() - Suspend uC operation. > + * @dev_priv: i915 device private > + * Ha! found missing kerneldoc ... maybe it can be partially moved to previous patch ? > + * This function disables GuC submission, invokes GuC OS suspension, > + * disables GuC interrupts and disable communication with GuC. > + * > + * Return: non-zero code on error > + */ > int intel_uc_suspend(struct drm_i915_private *dev_priv) > { > - int ret; > + struct intel_guc *guc = &dev_priv->guc; > + int ret = 0; > + > + if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) > + goto out; Hmm, is it ok to report DRM_ERROR if Guc was not started/loaded ? Return 0 seems to be still the best option here. > + > + i915_guc_submission_disable(dev_priv); > - ret = intel_guc_suspend(&dev_priv->guc); > + ret = intel_guc_suspend(guc); > if (ret) > - DRM_ERROR("Failed to suspend GuC\n"); > + goto out_suspend; > + > + gen9_disable_guc_interrupts(dev_priv); > + guc_disable_communication(guc); > + > + goto out; > + > +out_suspend: > + i915_guc_submission_enable(dev_priv); > +out: > + if (ret) > + DRM_ERROR("uC Suspend failed (%d)\n", ret); Unless I read wrong, we are re-enabling guc submission on failure, so maybe error should say something like: DRM_ERROR("Failed to suspend uC, aborting suspend\n"); > return ret; > } > +/** > + * intel_uc_resume() - Resume uC operation. > + * @dev_priv: i915 device private > + * > + * This function enables communication with GuC, enables GuC interrupts, > + * invokes GuC OS resumption and enables GuC submission. > + */ > void intel_uc_resume(struct drm_i915_private *dev_priv) > { > - intel_guc_resume(&dev_priv->guc); > + struct intel_guc *guc = &dev_priv->guc; > + int ret; > + > + if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) > + return; > + > + ret = guc_enable_communication(guc); Hmm, not too early ? CT will try to talk with Guc. > + if (ret) { > + DRM_DEBUG_DRIVER("GuC communication enable failed (%d)\n", ret); > + return; > + } > + > + if (i915_modparams.guc_log_level >= 0) > + gen9_enable_guc_interrupts(dev_priv); > + > + ret = intel_guc_resume(guc); > + if (ret) > + DRM_ERROR("GuC resume failed (%d)." > + "GuC functions may not work\n", ret); > + > + i915_guc_submission_enable(dev_priv); > + > + DRM_DEBUG_DRIVER("GuC submission %s\n", > + i915_guc_submission_enabled(guc) ? > + "enabled" : "disabled"); Hmm, this message can be part of i915_guc_submission_enable > }
On 10/11/2017 9:49 PM, Michal Wajdeczko wrote: > On Wed, 11 Oct 2017 10:54:05 +0200, Sagar Arun Kamble > <sagar.a.kamble@intel.com> wrote: > >> Suspending GuC involves bunch of tasks controlled by GuC OS and some >> controlled by Host OS. >> >> Host needs to disable submission to GuC and any other GuC functions. >> Then, >> GuC's task is initiated by Host sending action to GuC to enter sleep >> state. On this action, GuC preempts engines to idle context and then >> saves >> internal state to a buffer. It also disables internal >> interrupts/timers to >> avoid any wake-ups. >> After this, Host should disable GuC interrupts, communication with GuC >> (intel_guc_send/notify). GGTT invalidate update will have to be done in >> conjunction with GTT related suspend/resume tasks. >> >> v2: Rebase w.r.t removal of GuC code restructuring. >> >> v3: Removed GuC specific helpers as tasks other than send H2G for >> sleep/resume are to be done from uc generic functions. (Michal >> Wajdeczko) >> >> v4: Simplified/Unified the error messaging in uc_runtime_suspend/resume. >> (Michal Wajdeczko). Rebase w.r.t i915_modparams change. >> Added documentation to intel_uc_runtime_suspend/resume. >> >> v5: Removed enable_guc_loading based check from intel_uc_runtime_suspend >> and intel_uc_runtime_resume and pulled FW load_status based checks from >> intel_guc_suspend/resume into these functions. (Michal Wajdeczko) >> >> v6: Adjusted intel_uc_runtime_resume with prototype change to not return >> value. >> >> v7: Rebase. >> >> v8: Updated commit description and added submission enable/disable in >> GuC suspend/resume paths. Removed GGTT invalidate update functions. >> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Michał Winiarski <michal.winiarski@intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> #6 >> --- >> drivers/gpu/drm/i915/intel_guc.c | 11 ------- >> drivers/gpu/drm/i915/intel_uc.c | 65 >> +++++++++++++++++++++++++++++++++++++--- >> 2 files changed, 61 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_guc.c >> b/drivers/gpu/drm/i915/intel_guc.c >> index 9a2df69..55a0158 100644 >> --- a/drivers/gpu/drm/i915/intel_guc.c >> +++ b/drivers/gpu/drm/i915/intel_guc.c >> @@ -177,11 +177,6 @@ int intel_guc_suspend(struct intel_guc *guc) >> struct i915_gem_context *ctx; >> u32 data[3]; >> - if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) >> - return 0; >> - >> - gen9_disable_guc_interrupts(i915); >> - >> ctx = i915->kernel_context; >> data[0] = INTEL_GUC_ACTION_ENTER_S_STATE; >> @@ -204,12 +199,6 @@ int intel_guc_resume(struct intel_guc *guc) >> struct i915_gem_context *ctx; >> u32 data[3]; >> - if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) >> - return 0; >> - >> - if (i915_modparams.guc_log_level >= 0) >> - gen9_enable_guc_interrupts(i915); >> - >> ctx = i915->kernel_context; >> data[0] = INTEL_GUC_ACTION_EXIT_S_STATE; >> diff --git a/drivers/gpu/drm/i915/intel_uc.c >> b/drivers/gpu/drm/i915/intel_uc.c >> index b5c132c..297a321 100644 >> --- a/drivers/gpu/drm/i915/intel_uc.c >> +++ b/drivers/gpu/drm/i915/intel_uc.c >> @@ -284,18 +284,75 @@ void intel_uc_fini_hw(struct drm_i915_private >> *dev_priv) >> i915_ggtt_disable_guc(dev_priv); >> } >> +/** >> + * intel_uc_suspend() - Suspend uC operation. >> + * @dev_priv: i915 device private >> + * > > Ha! found missing kerneldoc ... maybe it can be partially moved to > previous patch ? Yes. > >> + * This function disables GuC submission, invokes GuC OS suspension, >> + * disables GuC interrupts and disable communication with GuC. >> + * >> + * Return: non-zero code on error >> + */ >> int intel_uc_suspend(struct drm_i915_private *dev_priv) >> { >> - int ret; >> + struct intel_guc *guc = &dev_priv->guc; >> + int ret = 0; >> + >> + if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) >> + goto out; > > Hmm, is it ok to report DRM_ERROR if Guc was not started/loaded ? > Return 0 seems to be still the best option here. > This actually handles case for platforms without GuC support and I think we should replace this with enable_guc_loading. and then we can do DRM_ERROR if GuC was not loaded. >> + >> + i915_guc_submission_disable(dev_priv); >> - ret = intel_guc_suspend(&dev_priv->guc); >> + ret = intel_guc_suspend(guc); >> if (ret) >> - DRM_ERROR("Failed to suspend GuC\n"); >> + goto out_suspend; >> + >> + gen9_disable_guc_interrupts(dev_priv); >> + guc_disable_communication(guc); >> + >> + goto out; >> + >> +out_suspend: >> + i915_guc_submission_enable(dev_priv); >> +out: >> + if (ret) >> + DRM_ERROR("uC Suspend failed (%d)\n", ret); > > Unless I read wrong, we are re-enabling guc submission on failure, > so maybe error should say something like: > > DRM_ERROR("Failed to suspend uC, aborting suspend\n"); Sure. > >> return ret; >> } >> +/** >> + * intel_uc_resume() - Resume uC operation. >> + * @dev_priv: i915 device private >> + * >> + * This function enables communication with GuC, enables GuC >> interrupts, >> + * invokes GuC OS resumption and enables GuC submission. >> + */ >> void intel_uc_resume(struct drm_i915_private *dev_priv) >> { >> - intel_guc_resume(&dev_priv->guc); >> + struct intel_guc *guc = &dev_priv->guc; >> + int ret; >> + >> + if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) >> + return; >> + >> + ret = guc_enable_communication(guc); > > Hmm, not too early ? CT will try to talk with Guc. intel_guc_resume will depend on CT mechanism to resume to GuC so CT enabling should happen first. Will keep this as it is v14 and revisit on the need to reorder. > >> + if (ret) { >> + DRM_DEBUG_DRIVER("GuC communication enable failed (%d)\n", >> ret); >> + return; >> + } >> + >> + if (i915_modparams.guc_log_level >= 0) >> + gen9_enable_guc_interrupts(dev_priv); >> + >> + ret = intel_guc_resume(guc); >> + if (ret) >> + DRM_ERROR("GuC resume failed (%d)." >> + "GuC functions may not work\n", ret); >> + >> + i915_guc_submission_enable(dev_priv); >> + >> + DRM_DEBUG_DRIVER("GuC submission %s\n", >> + i915_guc_submission_enabled(guc) ? >> + "enabled" : "disabled"); > > Hmm, this message can be part of i915_guc_submission_enable Ok. will move this message inside. > >> }
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c index 9a2df69..55a0158 100644 --- a/drivers/gpu/drm/i915/intel_guc.c +++ b/drivers/gpu/drm/i915/intel_guc.c @@ -177,11 +177,6 @@ int intel_guc_suspend(struct intel_guc *guc) struct i915_gem_context *ctx; u32 data[3]; - if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) - return 0; - - gen9_disable_guc_interrupts(i915); - ctx = i915->kernel_context; data[0] = INTEL_GUC_ACTION_ENTER_S_STATE; @@ -204,12 +199,6 @@ int intel_guc_resume(struct intel_guc *guc) struct i915_gem_context *ctx; u32 data[3]; - if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) - return 0; - - if (i915_modparams.guc_log_level >= 0) - gen9_enable_guc_interrupts(i915); - ctx = i915->kernel_context; data[0] = INTEL_GUC_ACTION_EXIT_S_STATE; diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index b5c132c..297a321 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -284,18 +284,75 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) i915_ggtt_disable_guc(dev_priv); } +/** + * intel_uc_suspend() - Suspend uC operation. + * @dev_priv: i915 device private + * + * This function disables GuC submission, invokes GuC OS suspension, + * disables GuC interrupts and disable communication with GuC. + * + * Return: non-zero code on error + */ int intel_uc_suspend(struct drm_i915_private *dev_priv) { - int ret; + struct intel_guc *guc = &dev_priv->guc; + int ret = 0; + + if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) + goto out; + + i915_guc_submission_disable(dev_priv); - ret = intel_guc_suspend(&dev_priv->guc); + ret = intel_guc_suspend(guc); if (ret) - DRM_ERROR("Failed to suspend GuC\n"); + goto out_suspend; + + gen9_disable_guc_interrupts(dev_priv); + guc_disable_communication(guc); + + goto out; + +out_suspend: + i915_guc_submission_enable(dev_priv); +out: + if (ret) + DRM_ERROR("uC Suspend failed (%d)\n", ret); return ret; } +/** + * intel_uc_resume() - Resume uC operation. + * @dev_priv: i915 device private + * + * This function enables communication with GuC, enables GuC interrupts, + * invokes GuC OS resumption and enables GuC submission. + */ void intel_uc_resume(struct drm_i915_private *dev_priv) { - intel_guc_resume(&dev_priv->guc); + struct intel_guc *guc = &dev_priv->guc; + int ret; + + if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) + return; + + ret = guc_enable_communication(guc); + if (ret) { + DRM_DEBUG_DRIVER("GuC communication enable failed (%d)\n", ret); + return; + } + + if (i915_modparams.guc_log_level >= 0) + gen9_enable_guc_interrupts(dev_priv); + + ret = intel_guc_resume(guc); + if (ret) + DRM_ERROR("GuC resume failed (%d)." + "GuC functions may not work\n", ret); + + i915_guc_submission_enable(dev_priv); + + DRM_DEBUG_DRIVER("GuC submission %s\n", + i915_guc_submission_enabled(guc) ? + "enabled" : "disabled"); }