[Intel-gfx] [PATCH v13 14/21] drm/i915/uc: Update GEM runtime resume with need for reload of GuC/HuC

[Michal Wajdeczko]

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

> On resume from drm sleep/suspend, we have gem_init_hw path to reload
> the GuC/HuC firmware. However, on resume from runtime suspend we needed
> to add support to reload the GuC/HuC firmware and resume.
> We can leverage intel_uc_init_hw for this based on skip_load_on_resume.
>
> 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>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |  2 +-
>  drivers/gpu/drm/i915/intel_uc.c | 28 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uc.h |  1 +
>  3 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c  
> b/drivers/gpu/drm/i915/i915_gem.c
> index 7d1b7e1..9e257e2 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2113,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_uc_resume(dev_priv);
> +	intel_uc_runtime_resume(dev_priv);
> 	mutex_unlock(&dev_priv->drm.struct_mutex);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index f641872..25acf8f 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -410,3 +410,31 @@ void intel_uc_resume(struct drm_i915_private  
> *dev_priv)
>  		guc->skip_load_on_resume = false;
>  	}
>  }
> +
> +/**
> + * intel_uc_runtime_resume() - Resume uC operation.
> + * @dev_priv: i915 device private
> + *
> + * This function invokes intel_uc_suspend that will if GuC is loaded
                             ^^^^^^^^^^^^^^^^
Please focus on tasks rather than function names.

> + * enable communication with GuC, enable GuC interrupts, invoke GuC OS
> + * resumption and enable GuC submission.
> + * If GuC is not loaded, GuC needs to be loaded and do the entire setup
> + * by leveraging intel_uc_init_hw.
> + *
> + */
> +void intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_guc *guc = &dev_priv->guc;
> +
> +	if (!guc->suspended)
> +		return;
> +
> +	intel_uc_resume(dev_priv);
> +
> +	if (guc->skip_load_on_resume)

Hmm, I may be lost, but I feel that some changes from 13/21 done
in intel_uc_resume() looks like good candidate for this function.

What I'm missing is clear distinction what each function will do,
due to lot of conditions and cross calls.

> +		return;
> +
> +	WARN_ON(guc_wopcm_locked(guc));

Why here?

> +
> +	intel_uc_init_hw(dev_priv);
> +}
> diff --git a/drivers/gpu/drm/i915/intel_uc.h  
> b/drivers/gpu/drm/i915/intel_uc.h
> index 7d9dd9c..f741ccc 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -36,5 +36,6 @@
>  void intel_uc_cleanup(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);
> +void intel_uc_runtime_resume(struct drm_i915_private *dev_priv);
> #endif