[Intel-gfx] [PATCH 2/3] drm/i915/uc: Sanitize uC together with GEM

[Daniele Ceraolo Spurio]

On 08/03/18 13:00, Michal Wajdeczko wrote:
> Instead of dancing around uC on reset/suspend/resume scenarios,
> explicitly sanitize uC when we sanitize GEM to force uC reload
> and start from known beginning.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry at intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c    |  2 ++
>   drivers/gpu/drm/i915/intel_guc.h   |  6 ++++++
>   drivers/gpu/drm/i915/intel_huc.h   |  6 ++++++
>   drivers/gpu/drm/i915/intel_uc.c    | 18 ++++++++++++++++++
>   drivers/gpu/drm/i915/intel_uc.h    |  1 +
>   drivers/gpu/drm/i915/intel_uc_fw.h |  6 ++++++
>   6 files changed, 39 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ab88ca5..49c81ae 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4892,6 +4892,8 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
>   	 */
>   	if (INTEL_GEN(i915) >= 5 && intel_has_gpu_reset(i915))
>   		WARN_ON(intel_gpu_reset(i915, ALL_ENGINES));
> +
> +	intel_uc_sanitize(i915);
>   }
>   
>   int i915_gem_suspend(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index b9424ac..ec8569f 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -132,4 +132,10 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>   struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
>   u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
>   
> +static inline int intel_guc_sanitize(struct intel_guc *guc)
> +{
> +	intel_uc_fw_sanitize(&guc->fw);
> +	return 0;
> +}
> +
>   #endif
> diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
> index 5d6e804..b185850 100644
> --- a/drivers/gpu/drm/i915/intel_huc.h
> +++ b/drivers/gpu/drm/i915/intel_huc.h
> @@ -38,4 +38,10 @@ struct intel_huc {
>   void intel_huc_init_early(struct intel_huc *huc);
>   int intel_huc_auth(struct intel_huc *huc);
>   
> +static inline int intel_huc_sanitize(struct intel_huc *huc)
> +{
> +	intel_uc_fw_sanitize(&huc->fw);
> +	return 0;
> +}
> +
>   #endif
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index a45171c..abd1f7b 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -327,6 +327,24 @@ void intel_uc_fini(struct drm_i915_private *dev_priv)
>   	intel_guc_fini(guc);
>   }
>   
> +void intel_uc_sanitize(struct drm_i915_private *i915)
> +{
> +	struct intel_guc *guc = &i915->guc;
> +	struct intel_huc *huc = &i915->huc;
> +
> +	if (!USES_GUC(i915))
> +		return;
> +
> +	GEM_BUG_ON(!HAS_GUC(i915));
> +
> +	guc_disable_communication(guc);

If you want to do this here for CT buffers you need to call this before 
resetting the GPU otherwise GuC will already be reset and won't be able 
to unregister the buffers. We can also just wipe the shared memory where 
the CT buffer info is stored but that doesn't sound too clean, although 
we might have to add the code for it anyway if we want to support the 
(unlikely) case where GuC dies on some error.

Might also be interesting to release doorbells here to sync that state 
as well, but that's a task for a another time ;)

BTW, are you planning to call this in the i915_reset flow as well or is 
the plan to add another dedicated function?

Thanks,
Daniele

> +
> +	intel_huc_sanitize(huc);
> +	intel_guc_sanitize(guc);
> +
> +	__intel_uc_reset_hw(i915);
> +}
> +
>   int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   {
>   	struct intel_guc *guc = &dev_priv->guc;
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index dce4813..937e611 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -34,6 +34,7 @@
>   void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
>   int intel_uc_init_misc(struct drm_i915_private *dev_priv);
>   void intel_uc_fini_misc(struct drm_i915_private *dev_priv);
> +void intel_uc_sanitize(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_init(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h
> index d5fd460..2601521 100644
> --- a/drivers/gpu/drm/i915/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.h
> @@ -115,6 +115,12 @@ static inline bool intel_uc_fw_is_selected(struct intel_uc_fw *uc_fw)
>   	return uc_fw->path != NULL;
>   }
>   
> +static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
> +{
> +	if (uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS)
> +		uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
> +}
> +
>   void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>   		       struct intel_uc_fw *uc_fw);
>   int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
>