Re: [PATCH 3/5] drm/i915/guc: Simplify intel_guc_load()

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 15/12/2016 15:47, Arkadiusz Hiler wrote:
> Current version of intel_guc_load() does a lot:
>  - cares about submission
>  - loads huc

Not yet, no? So instead you could say that you are preparing the 
groundworks to make adding in the HuC fit better.

>  - implement WA
>
> This change offloads some of the logic to intel_uc_load(), which now
> cares about the above.
>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         |   2 +-
>  drivers/gpu/drm/i915/intel_guc_loader.c | 126 +++++---------------------------
>  drivers/gpu/drm/i915/intel_uc.c         |  83 +++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uc.h         |   8 ++
>  4 files changed, 110 insertions(+), 109 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6af4e85..76b52c6 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4412,7 +4412,7 @@ i915_gem_init_hw(struct drm_i915_private *dev_priv)
>  	intel_mocs_init_l3cc_table(dev_priv);
>
>  	/* We can't enable contexts until all firmware is loaded */
> -	ret = intel_guc_load(dev_priv);
> +	ret = intel_uc_load(dev_priv);
>  	if (ret)
>  		goto out;
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index f8b28b1..b76b556 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -97,7 +97,7 @@ const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status)
>  	}
>  };
>
> -static void guc_interrupts_release(struct drm_i915_private *dev_priv)
> +void guc_interrupts_release(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
> @@ -115,7 +115,7 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv)
>  	I915_WRITE(GUC_WD_VECS_IER, 0);
>  }
>
> -static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
> +void guc_interrupts_capture(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
> @@ -334,7 +334,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
>  	return ret;
>  }
>
> -static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
> +u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
>  {
>  	u32 wopcm_size = GUC_WOPCM_TOP;
>
> @@ -417,7 +417,7 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>
> -static int guc_hw_reset(struct drm_i915_private *dev_priv)
> +int guc_hw_reset(struct drm_i915_private *dev_priv)
>  {
>  	int ret;
>  	u32 guc_status;
> @@ -452,75 +452,37 @@ int intel_guc_load(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>  	const char *fw_path = guc_fw->guc_fw_path;
> -	int retries, ret, err;
> +	int ret;
>
>  	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
>  		fw_path,
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>
> -	/* Loading forbidden, or no firmware to load? */
> -	if (!i915.enable_guc_loading) {
> -		err = 0;
> -		goto fail;
> -	} else if (fw_path == NULL) {
> +	if (fw_path == NULL) {
>  		/* Device is known to have no uCode (e.g. no GuC) */
> -		err = -ENXIO;
> -		goto fail;
> +		return -ENXIO;
>  	} else if (*fw_path == '\0') {
>  		/* Device has a GuC but we don't know what f/w to load? */
>  		WARN(1, "No GuC firmware known for this platform!\n");
> -		err = -ENODEV;
> -		goto fail;
> +		return -ENODEV;
>  	}
>
>  	/* Fetch failed, or already fetched but failed to load? */
>  	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) {
> -		err = -EIO;
> -		goto fail;
> +		return -EIO;
>  	} else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) {
> -		err = -ENOEXEC;
> -		goto fail;
> +		return -ENOEXEC;
>  	}
>
> -	guc_interrupts_release(dev_priv);
> -	gen9_reset_guc_interrupts(dev_priv);
> -
>  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
>
> -	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> -
> -	err = i915_guc_submission_init(dev_priv);
> -	if (err)
> -		goto fail;
> -
> -	/*
> -	 * WaEnableuKernelHeaderValidFix:skl,bxt
> -	 * For BXT, this is only upto B0 but below WA is required for later
> -	 * steppings also so this is extended as well.
> -	 */
>  	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> -	for (retries = 3; ; ) {
> -		/*
> -		 * Always reset the GuC just before (re)loading, so
> -		 * that the state and timing are fairly predictable
> -		 */
> -		err = guc_hw_reset(dev_priv);
> -		if (err)
> -			goto fail;
> +	/* we may want to retry guc ucode transfer */
> +	ret = guc_ucode_xfer(dev_priv);
>
> -		err = guc_ucode_xfer(dev_priv);
> -		if (!err)
> -			break;
> -
> -		if (--retries == 0)
> -			goto fail;
> -
> -		DRM_INFO("GuC fw load failed: %d; will reset and "
> -			 "retry %d more time(s)\n", err, retries);
> -	}
> +	if (ret)
> +		return -EAGAIN;
>
>  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_SUCCESS;
>
> @@ -528,63 +490,7 @@ int intel_guc_load(struct drm_i915_private *dev_priv)
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>
> -	if (i915.enable_guc_submission) {
> -		if (i915.guc_log_level >= 0)
> -			gen9_enable_guc_interrupts(dev_priv);
> -
> -		err = i915_guc_submission_enable(dev_priv);
> -		if (err)
> -			goto fail;
> -		guc_interrupts_capture(dev_priv);
> -	}
> -
>  	return 0;
> -
> -fail:
> -	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
> -		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;

You seem to have lost this state transition at least in the 
enable_guc_loading = 0 case. Was that deliberate? I have no idea if that 
is important so just an observation.

> -
> -	guc_interrupts_release(dev_priv);
> -	i915_guc_submission_disable(dev_priv);
> -	i915_guc_submission_fini(dev_priv);
> -
> -	/*
> -	 * We've failed to load the firmware :(
> -	 *
> -	 * Decide whether to disable GuC submission and fall back to
> -	 * execlist mode, and whether to hide the error by returning
> -	 * zero or to return -EIO, which the caller will treat as a
> -	 * nonfatal error (i.e. it doesn't prevent driver load, but
> -	 * marks the GPU as wedged until reset).
> -	 */
> -	if (i915.enable_guc_loading > 1) {
> -		ret = -EIO;
> -	} else if (i915.enable_guc_submission > 1) {
> -		ret = -EIO;
> -	} else {
> -		ret = 0;
> -	}
> -
> -	if (err == 0 && !HAS_GUC_UCODE(dev_priv))
> -		;	/* Don't mention the GuC! */
> -	else if (err == 0)
> -		DRM_INFO("GuC firmware load skipped\n");
> -	else if (ret != -EIO)
> -		DRM_NOTE("GuC firmware load failed: %d\n", err);
> -	else
> -		DRM_WARN("GuC firmware load failed: %d\n", err);
> -
> -	if (i915.enable_guc_submission) {
> -		if (fw_path == NULL)
> -			DRM_INFO("GuC submission without firmware not supported\n");
> -		if (ret == 0)
> -			DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> -		else
> -			DRM_ERROR("GuC init failed: %d\n", ret);
> -	}
> -	i915.enable_guc_submission = 0;
> -
> -	return ret;
>  }
>
>  static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> @@ -757,6 +663,10 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
>  	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
>  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
>
> +	/* can't enable guc submission without guc */
> +	if (!i915.enable_guc_loading)
> +		i915.enable_guc_submission = 0;
> +
>  	/* Early (and silent) return if GuC loading is disabled */
>  	if (!i915.enable_guc_loading)
>  		return;

You got two same conditions one after another, maybe merge them?

> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 8eec035..4e184edb 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -35,6 +35,89 @@ void intel_uc_init(struct drm_i915_private *dev_priv)
>  	intel_guc_init(dev_priv);
>  }
>
> +int intel_uc_load(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> +	int ret, retries;
> +
> +	/* guc not enabled, nothing to do */
> +	if (!i915.enable_guc_loading)
> +		return 0;
> +
> +	guc_interrupts_release(dev_priv);
> +	gen9_reset_guc_interrupts(dev_priv);
> +
> +	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
> +
> +	if (i915.enable_guc_submission) {
> +		ret = i915_guc_submission_init(dev_priv);
> +		if (ret)
> +			goto fail;
> +	}
> +
> +	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> +	retries = GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS;
> +	while (retries--) {
> +		/*
> +		 * Always reset the GuC just before (re)loading, so
> +		 * that the state and timing are fairly predictable
> +		 */
> +		ret = guc_hw_reset(dev_priv);
> +		if (ret)
> +			goto fail;
> +
> +		ret = intel_guc_load(dev_priv);
> +		if (ret == 0 || ret != -EAGAIN)
> +			break;
> +
> +		DRM_INFO("GuC fw load failed: %d; will reset and "
> +			 "retry %d more time(s)\n", ret, retries);
> +	}
> +
> +	/* did we succeded or run out of retries? */

"Have we succeeded" I think.

> +	if (ret)
> +		goto fail;

There was a debug message round about here I think which logged that the 
firmware was successfully loaded. I think it is good to have it. I think 
even logging the major and minor would be good and maybe even at 
informational level?

> +
> +	if (i915.enable_guc_submission) {
> +		if (i915.guc_log_level >= 0)
> +			gen9_enable_guc_interrupts(dev_priv);
> +
> +		ret = i915_guc_submission_enable(dev_priv);
> +		if (ret)
> +			goto fail;
> +		guc_interrupts_capture(dev_priv);
> +	}
> +
> +	return 0;
> +
> +fail:
> +	/*
> +	 * We've failed to load the firmware :(
> +	 *
> +	 * Decide whether to disable GuC submission and fall back to
> +	 * execlist mode, and whether to hide the error by returning
> +	 * zero or to return -EIO, which the caller will treat as a
> +	 * nonfatal error (i.e. it doesn't prevent driver load, but
> +	 * marks the GPU as wedged until reset).
> +	 */
> +	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
> +		ret = -EIO;
> +	else
> +		ret = 0;
> +
> +	if (i915.enable_guc_submission) {
> +		i915.enable_guc_submission = 0;
> +		DRM_INFO("GuC submission without firmware not supported\n");
> +		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> +	}
> +
> +	guc_interrupts_release(dev_priv);
> +	i915_guc_submission_disable(dev_priv);
> +	i915_guc_submission_fini(dev_priv);
> +
> +	return ret;
> +}
> +
>  /*
>   * Read GuC command/status register (SOFT_SCRATCH_0)
>   * Return true if it contains a response rather than a command
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index ec1a5b2..89c7e1f 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -28,6 +28,9 @@
>  #include "i915_guc_reg.h"
>  #include "intel_ringbuffer.h"
>
> +/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> +#define GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS 3
> +
>  struct drm_i915_gem_request;
>
>  /*
> @@ -171,6 +174,7 @@ struct intel_guc {
>  /* intel_uc.c */
>  void intel_uc_init_early(struct drm_i915_private *dev_priv);
>  void intel_uc_init(struct drm_i915_private *dev_priv);
> +int intel_uc_load(struct drm_i915_private *dev_priv);
>  bool intel_guc_recv(struct drm_i915_private *dev_priv, u32 *status);
>  int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
>  int intel_guc_sample_forcewake(struct intel_guc *guc);
> @@ -185,6 +189,10 @@ extern void intel_guc_fini(struct drm_i915_private *dev_priv);
>  extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
>  extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
>  extern int intel_guc_resume(struct drm_i915_private *dev_priv);
> +void guc_interrupts_release(struct drm_i915_private *dev_priv);
> +void guc_interrupts_capture(struct drm_i915_private *dev_priv);
> +int guc_hw_reset(struct drm_i915_private *dev_priv);
> +u32 guc_wopcm_size(struct drm_i915_private *dev_priv);
>
>  /* i915_guc_submission.c */
>  int i915_guc_submission_init(struct drm_i915_private *dev_priv);
>

I was just reading it so I can review 5/5 as you have asked me to look 
at that one.

Regards,

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