[Intel-gfx] [PATCH v13 10/21] drm/i915/guc: Update uC suspend/resume function separating Host/GuC tasks
Sagar Arun Kamble
sagar.a.kamble at intel.com
Thu Oct 12 06:38:43 UTC 2017
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 at 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 at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Michał Winiarski <michal.winiarski at intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Reviewed-by: Michal Wajdeczko <michal.wajdeczko at 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.
>
>> }
More information about the Intel-gfx
mailing list