Re: [PATCH 2/7] drm/i915/guc: always reset GuC before loading firmware

[Arun Siluvery <arun.siluvery@linux.intel.com>]

On 21/03/2016 10:16, Dave Gordon wrote:
> After a suspend-resume cycle, the resumed kernel has no idea what the
> booted kernel may have done to the GuC before replacing itself with the
> resumed image. In particular, it may have already loaded the GuC with
> firmware, which will then cause this kernel's attempt to (re)load the
> firmware to fail (GuC program memory is write-once!).
>
> So here we choose to always reset the GuC just before (re)loading the
> firmware, so the hardware should then be in a well-known state, and we
> may even avoid some of the issues arising from unpredictable timing.
>
> Also added some more fields & values to the definition of the GUC_STATUS
> register, which is the key diagnostic indicator if the GuC load fails.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Arun Siluvery <arun.siluvery@linux.intel.com>
> Cc: Alex Dai <yu.dai@intel.com>
>
Please add bugzilla reference.

> ---
>   drivers/gpu/drm/i915/i915_guc_reg.h     | 12 ++++++++--
>   drivers/gpu/drm/i915/intel_guc_loader.c | 40 ++++++++++++++++-----------------
>   2 files changed, 29 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h
> index 94ceee5..80786d9 100644
> --- a/drivers/gpu/drm/i915/i915_guc_reg.h
> +++ b/drivers/gpu/drm/i915/i915_guc_reg.h
> @@ -27,10 +27,12 @@
>   /* Definitions of GuC H/W registers, bits, etc */
>
>   #define GUC_STATUS			_MMIO(0xc000)
> -#define   GS_MIA_IN_RESET		(1 << 0)
> +#define   GS_RESET_SHIFT		0
> +#define   GS_MIA_IN_RESET		  (0x01 << GS_RESET_SHIFT)
>   #define   GS_BOOTROM_SHIFT		1
>   #define   GS_BOOTROM_MASK		  (0x7F << GS_BOOTROM_SHIFT)
>   #define   GS_BOOTROM_RSA_FAILED		  (0x50 << GS_BOOTROM_SHIFT)
> +#define   GS_BOOTROM_JUMP_PASSED	  (0x76 << GS_BOOTROM_SHIFT)
>   #define   GS_UKERNEL_SHIFT		8
>   #define   GS_UKERNEL_MASK		  (0xFF << GS_UKERNEL_SHIFT)
>   #define   GS_UKERNEL_LAPIC_DONE		  (0x30 << GS_UKERNEL_SHIFT)
> @@ -38,7 +40,13 @@
>   #define   GS_UKERNEL_READY		  (0xF0 << GS_UKERNEL_SHIFT)
>   #define   GS_MIA_SHIFT			16
>   #define   GS_MIA_MASK			  (0x07 << GS_MIA_SHIFT)
> -#define   GS_MIA_CORE_STATE		  (1 << GS_MIA_SHIFT)
> +#define   GS_MIA_CORE_STATE		  (0x01 << GS_MIA_SHIFT)
> +#define   GS_MIA_HALT_REQUESTED		  (0x02 << GS_MIA_SHIFT)
> +#define   GS_MIA_ISR_ENTRY		  (0x04 << GS_MIA_SHIFT)
> +#define   GS_AUTH_STATUS_SHIFT		30
> +#define   GS_AUTH_STATUS_MASK		  (0x03 << GS_AUTH_STATUS_SHIFT)
> +#define   GS_AUTH_STATUS_BAD		  (0x01 << GS_AUTH_STATUS_SHIFT)
> +#define   GS_AUTH_STATUS_GOOD		  (0x02 << GS_AUTH_STATUS_SHIFT)
>
>   #define SOFT_SCRATCH(n)			_MMIO(0xc180 + (n) * 4)
>   #define SOFT_SCRATCH_COUNT		16
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index a07c228..a875936 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -387,7 +387,7 @@ int intel_guc_ucode_load(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> -	int err = 0;
> +	int retries, err = 0;
>
>   	if (!i915.enable_guc_submission)
>   		return 0;
> @@ -441,29 +441,26 @@ int intel_guc_ucode_load(struct drm_device *dev)
>   	 * steppings also so this is extended as well.
>   	 */
>   	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> -	err = guc_ucode_xfer(dev_priv);
> -	if (err) {
> -		int retries = 3;
> -
> -		DRM_ERROR("GuC fw load failed, err=%d, attempting reset and retry\n", err);
> -
> -		while (retries--) {
> -			err = i915_reset_guc(dev_priv);
> -			if (err)
> -				break;
> -
> -			err = guc_ucode_xfer(dev_priv);
> -			if (!err) {
> -				DRM_DEBUG_DRIVER("GuC fw reload succeeded after reset\n");
> -				break;
> -			}
> -			DRM_DEBUG_DRIVER("GuC fw reload retries left: %d\n", retries);
> -		}
> -
> +	for (retries = 3; ; ) {
> +		/*
> +		 * Always reset the GuC just before (re)loading, so
> +		 * that the state and timing are fairly predictable
> +		 */
> +		err = i915_reset_guc(dev_priv);
>   		if (err) {
> -			DRM_ERROR("GuC fw reload attempt failed, ret=%d\n", err);
> +			DRM_ERROR("GuC reset failed, err %d\n", err);
>   			goto fail;
>   		}
> +
> +		err = guc_ucode_xfer(dev_priv);
> +		if (!err)
> +			break;
> +
> +		if (--retries == 0)
> +			goto fail;
> +
> +		DRM_INFO("GuC fw load failed, err %d; will reset and "
> +			"retry %d more time(s)\n", err, retries);
>   	}
>
>   	guc_fw->guc_fw_load_status = GUC_FIRMWARE_SUCCESS;
> @@ -485,6 +482,7 @@ int intel_guc_ucode_load(struct drm_device *dev)
>   	return 0;
>
>   fail:
> +	DRM_ERROR("GuC firmware load failed, err %d\n", err);
>   	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
>   		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
>
>
looks good to me,
Reviewed-by: Arun Siluvery <arun.siluvery@linux.intel.com>

regards
Arun

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