[Intel-gfx] [PATCH 1/2] drm/i915/guc: fix GuC suspend/resume

[Michal Wajdeczko]

On Tue, 16 Oct 2018 00:10:08 +0200, Daniele Ceraolo Spurio  
<daniele.ceraolospurio at intel.com> wrote:

> The ENTER/EXIT_S_STATE actions queue the save/restore operation in GuC
> FW and then return, so waiting on the H2H is not enough to guarantee
> GuC is done.
> When all the processing is done, GuC writes 0 to scratch register 14,
> so we can poll on that. Note that GuC does not ensure that the value
> in the register is different from 0 while the action is in progress
> so we need to take care of that ourselves as well.
>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc.c      | 28 +++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_guc_fwif.h |  6 ++++++
>  2 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index 230aea69385d..f238cd7a9dcf 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -521,6 +521,30 @@ int intel_guc_auth_huc(struct intel_guc *guc, u32  
> rsa_offset)
>  	return intel_guc_send(guc, action, ARRAY_SIZE(action));
>  }
> +/*
> + * The ENTER/EXIT_S_STATE actions queue the save/restore operation in  
> GuC FW and
> + * then return, so waiting on the H2H is not enough to guarantee GuC is  
> done.
> + * When all the processing is done, GuC writes 0 to scratch register  
> 14, so we

s/writes 0/writes INTEL_GUC_SLEEP_STATE_SUCCESS ?

> + * can poll on that. Note that GuC does not ensure that the value in the
> + * register is different from 0 while the action is in progress so we  
> need to
> + * take care of that ourselves as well.
> + */
> +static int guc_sleep_state_action(struct intel_guc *guc,
> +				  const u32 *action, u32 len)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	int ret;
> +
> +	I915_WRITE(SOFT_SCRATCH(14), ~0x0);

Do we want to add dedicated name for that scratch register?

Also, as GuC is using scratch[14] then we need to remove it from our send
register pool - patch is here [1]

> +
> +	ret = intel_guc_send(guc, action, len);
> +	if (ret)
> +		return ret;
> +
> +	return intel_wait_for_register(dev_priv, SOFT_SCRATCH(14), ~0x0,
> +				       INTEL_GUC_SLEEP_STATE_SUCCESS, 10);

Maybe it is worth to use __intel_wait_for_register() and print sleep
state error code in case of failure ? And do we really need to wait
full 10ms for SUCCESS if GuC already has reported PREEMPT_TO_IDLE_FAILED
or ENGINE_RESET_FAILED ?

	u32 state;

	ret = __intel_wait_for_register(dev_priv, SOFT_SCRATCH(14), 0x80000000,
				        0, 10, &state);
	if (ret)
		return ret;
	if (status != INTEL_GUC_SLEEP_STATE_SUCCESS) {
		DRM_ERROR("... %u\n", state);
		return -EIO;
	}
	return 0;

> +}
> +
>  /**
>   * intel_guc_suspend() - notify GuC entering suspend state
>   * @guc:	the guc
> @@ -533,7 +557,7 @@ int intel_guc_suspend(struct intel_guc *guc)
>  		intel_guc_ggtt_offset(guc, guc->shared_data)
>  	};
> -	return intel_guc_send(guc, data, ARRAY_SIZE(data));
> +	return guc_sleep_state_action(guc, data, ARRAY_SIZE(data));
>  }
> /**
> @@ -571,7 +595,7 @@ int intel_guc_resume(struct intel_guc *guc)
>  		intel_guc_ggtt_offset(guc, guc->shared_data)
>  	};
> -	return intel_guc_send(guc, data, ARRAY_SIZE(data));
> +	return guc_sleep_state_action(guc, data, ARRAY_SIZE(data));
>  }
> /**
> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h  
> b/drivers/gpu/drm/i915/intel_guc_fwif.h
> index 8382d591c784..b0eb5aabe0a7 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> @@ -687,6 +687,12 @@ enum intel_guc_report_status {
>  	INTEL_GUC_REPORT_STATUS_COMPLETE = 0x4,
>  };
> +enum intel_guc_sleep_state_status {
> +	INTEL_GUC_SLEEP_STATE_SUCCESS = 0x0,
> +	INTEL_GUC_SLEEP_STATE_PREEMPT_TO_IDLE_FAILED = 0x1,
> +	INTEL_GUC_SLEEP_STATE_ENGINE_RESET_FAILED = 0x2

as we waiting for state change, maybe we should explicitly define
	INTEL_GUC_SLEEP_STATE_INVALID_MASK = 0x80000000,
and use it in __intel_wait_for_register ?

> +};
> +
>  #define GUC_LOG_CONTROL_LOGGING_ENABLED	(1 << 0)
>  #define GUC_LOG_CONTROL_VERBOSITY_SHIFT	4
>  #define GUC_LOG_CONTROL_VERBOSITY_MASK	(0xF <<  
> GUC_LOG_CONTROL_VERBOSITY_SHIFT)

Thanks,
Michal

[1] https://patchwork.freedesktop.org/patch/256921/