Re: [PATCH v13 10/21] drm/i915/guc: Update uC suspend/resume function separating Host/GuC tasks

["Michal Wajdeczko" <michal.wajdeczko@intel.com>]

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

>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx