Re: [PATCH v13 09/21] drm/i915/uc: Create uC suspend and resume functions

From: "Michal Wajdeczko" <michal.wajdeczko@intel.com>
To: intel-gfx@lists.freedesktop.org,
	Sagar Arun Kamble <sagar.a.kamble@intel.com>
Subject: Re: [PATCH v13 09/21] drm/i915/uc: Create uC suspend and resume functions
Date: Wed, 11 Oct 2017 17:57:00 +0200	[thread overview]
Message-ID: <op.y7ybxapfxaggs7@mwajdecz-mobl1.ger.corp.intel.com> (raw)
In-Reply-To: <1507712056-25030-10-git-send-email-sagar.a.kamble@intel.com>

On Wed, 11 Oct 2017 10:54:04 +0200, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> Prepared generic helpers intel_uc_suspend, intel_uc_resume. These are
> called from respective GEM functions. Only exception is intel_uc_resume
> that needs to be called w/ or w/o GuC loaded in i915_drm_resume path.
> Changes to add WOPCM condition check to load GuC during resume will be
> added in later patches.
>
> v2: Rebase w.r.t removal of GuC code restructuring.
>
> v3: Calling intel_uc_resume from i915_gem_resume post resuming
> i915 gem setup. This is symmetrical with i915_gem_suspend.
> Removed error messages from i915 suspend/resume routines as
> uC suspend/resume routines will have those. (Michal Wajdeczko)
> Declare wedged on uc_suspend failure and uc_resume failure.
> (Michał Winiarski)
> Keeping the uC suspend/resume function definitions close to other
> uC functions.
>
> v4: Added implementation to intel_uc_resume as GuC resume is
> needed to be triggered post reloading the firmware as well. Added
> comments about semantics of GuC resume with the firmware reload.
>
> v5: Updated return from i915_gem_runtime_suspend. Moved the comment
> about GuC reload optimization to intel_uc_init_hw. (Michal Wajdeczko)
> Updated comments as FIXME.
>
> v6: Kept error handling for failure from i915_gem_runtime_suspend only.
> We don't want GEM/GuC resume failure to impact intel_runtime_resume or
> i915_drm_resume. GEM suspend failure along i915_drm_suspend can also
> be ignored as we reset GPU post that. Updated comments. (Chris, Joonas)
>
> v7: Removed intel_uc_resume from i915_drm_resume as it will be done as
> part of intel_uc_init_hw in further patches. Removed TODO comments about
> handling GuC load skip on resume. This is to be addressed in further
> patches. Added error return from intel_uc_suspend as we plan to add
> functionality to resume submission in case of suspend failure in further
> patches. Removed runtime uC suspend/resume functions as functionality
> will be similar in both paths.
>
> 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>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 17 +++++++++++------
>  drivers/gpu/drm/i915/intel_uc.c | 16 ++++++++++++++++
>  drivers/gpu/drm/i915/intel_uc.h |  2 ++
>  3 files changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c  
> b/drivers/gpu/drm/i915/i915_gem.c
> index 38447ae..7d1b7e1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2056,11 +2056,13 @@ static void  
> __i915_gem_object_release_mmap(struct drm_i915_gem_object *obj)
>  int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_i915_gem_object *obj, *on;
> -	int i;
> +	int i, ret;
> 	mutex_lock(&dev_priv->drm.struct_mutex);
> -	intel_guc_suspend(&dev_priv->guc);
> +	ret = intel_uc_suspend(dev_priv);
> +	if (ret)
> +		goto out_unlock;
> 	/*
>  	 * Only called during RPM suspend. All users of the userfault_list
> @@ -2098,9 +2100,10 @@ int i915_gem_runtime_suspend(struct  
> drm_i915_private *dev_priv)
>  		reg->dirty = true;
>  	}
> +out_unlock:
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
> -	return 0;
> +	return ret;
>  }
> void i915_gem_runtime_resume(struct drm_i915_private *dev_priv)
> @@ -2110,7 +2113,7 @@ void i915_gem_runtime_resume(struct  
> drm_i915_private *dev_priv)
>  	i915_gem_init_swizzling(dev_priv);
>  	i915_gem_restore_fences(dev_priv);
> -	intel_guc_resume(&dev_priv->guc);
> +	intel_uc_resume(dev_priv);
> 	mutex_unlock(&dev_priv->drm.struct_mutex);
>  }
> @@ -4681,7 +4684,9 @@ int i915_gem_suspend(struct drm_i915_private  
> *dev_priv)
>  		i915_gem_set_wedged(dev_priv); /* no hope, discard everything */
> 	mutex_lock(&dev->struct_mutex);
> -	intel_guc_suspend(&dev_priv->guc);
> +	ret = intel_uc_suspend(dev_priv);
> +	if (ret)
> +		goto err_unlock;
>  	mutex_unlock(&dev->struct_mutex);
> 	/*
> @@ -4730,7 +4735,7 @@ void i915_gem_resume(struct drm_i915_private  
> *dev_priv)
>  	 */
>  	dev_priv->gt.resume(dev_priv);
> -	intel_guc_resume(&dev_priv->guc);
> +	intel_uc_resume(dev_priv);
> 	mutex_unlock(&dev->struct_mutex);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index 7305486..b5c132c 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -283,3 +283,19 @@ void intel_uc_fini_hw(struct drm_i915_private  
> *dev_priv)
>  	if (i915_modparams.enable_guc_loading)
>  		i915_ggtt_disable_guc(dev_priv);
>  }
> +
> +int intel_uc_suspend(struct drm_i915_private *dev_priv)
> +{
> +	int ret;
> +
> +	ret = intel_guc_suspend(&dev_priv->guc);
> +	if (ret)
> +		DRM_ERROR("Failed to suspend GuC\n");
> +
> +	return ret;
> +}
> +
> +void intel_uc_resume(struct drm_i915_private *dev_priv)
> +{
> +	intel_guc_resume(&dev_priv->guc);
> +}

Can we add kerneldoc for above new functions?
With doc added

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>

> diff --git a/drivers/gpu/drm/i915/intel_uc.h  
> b/drivers/gpu/drm/i915/intel_uc.h
> index e18d3bb..e20bc72 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -34,5 +34,7 @@
>  void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
>  int intel_uc_init_hw(struct drm_i915_private *dev_priv);
>  void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
> +int intel_uc_suspend(struct drm_i915_private *dev_priv);
> +void intel_uc_resume(struct drm_i915_private *dev_priv);
> #endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx