[Intel-gfx] [PATCH] drm/i915/guc: Split GuC firmware xfer function into clear steps

Sagar Arun Kamble sagar.a.kamble at intel.com
Mon Oct 30 14:00:52 UTC 2017



On 10/27/2017 10:45 PM, Michal Wajdeczko wrote:
> Transfer of GuC firmware requires few steps that currently
> are implemented in two large functions. Split this code into
> smaller functions to make these steps small and clear.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc_fw.c | 173 ++++++++++++++++++++++--------------
>   1 file changed, 106 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
> index ef67a36..2a10bcf 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_guc_fw.c
> @@ -97,23 +97,53 @@ int intel_guc_fw_select(struct intel_guc *guc)
>   	return 0;
>   }
>   
> -/*
> - * Read the GuC status register (GUC_STATUS) and store it in the
> - * specified location; then return a boolean indicating whether
> - * the value matches either of two values representing completion
> - * of the GuC boot process.
> - *
> - * This is used for polling the GuC status in a wait_for()
> - * loop below.
> - */
> -static inline bool guc_ucode_response(struct drm_i915_private *dev_priv,
> -				      u32 *status)
> +static void guc_ucode_xfer_prepare(struct drm_i915_private *dev_priv)
>   {
> -	u32 val = I915_READ(GUC_STATUS);
> -	u32 uk_val = val & GS_UKERNEL_MASK;
> -	*status = val;
> -	return (uk_val == GS_UKERNEL_READY ||
> -		((val & GS_MIA_CORE_STATE) && uk_val == GS_UKERNEL_LAPIC_DONE));
> +	/* Enable MIA caching. GuC clock gating is disabled. */
Clock gating comment is linked with below WaDisableMinuteIa*. Can you 
update in this patch. Better to drop with Wa name telling the intent.
> +	I915_WRITE(GUC_SHIM_CONTROL, GUC_SHIM_CONTROL_VALUE);
> +
> +	/* WaDisableMinuteIaClockGating:bxt */
> +	if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_A1)) {
> +		I915_WRITE(GUC_SHIM_CONTROL, (I915_READ(GUC_SHIM_CONTROL) &
> +					      ~GUC_ENABLE_MIA_CLOCK_GATING));
> +	}
> +
> +	/* WaC6DisallowByGfxPause:bxt */
> +	if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_B0))
> +		I915_WRITE(GEN6_GFXPAUSE, 0x30FFF);
> +
> +	if (IS_GEN9_LP(dev_priv))
> +		I915_WRITE(GEN9LP_GT_PM_CONFIG, GT_DOORBELL_ENABLE);
> +	else
> +		I915_WRITE(GEN9_GT_PM_CONFIG, GT_DOORBELL_ENABLE);
> +
> +	if (IS_GEN9(dev_priv)) {
> +		/* DOP Clock Gating Enable for GuC clocks */
> +		I915_WRITE(GEN7_MISCCPCTL, (GEN8_DOP_CLOCK_GATE_GUC_ENABLE |
> +					    I915_READ(GEN7_MISCCPCTL)));
> +
> +		/* allows for 5us (in 10ns units) before GT can go to RC6 */
> +		I915_WRITE(GUC_ARAT_C6DIS, 0x1FF);
> +	}
> +}
> +
> +/* Copy RSA signature from the fw image to HW for verification */
> +static int guc_ucode_xfer_rsa(struct intel_uc_fw *guc_fw, struct i915_vma *vma)
> +{
> +	struct intel_guc *guc = container_of(guc_fw, struct intel_guc, fw);
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	struct sg_table *sg = vma->pages;
> +	u32 rsa[UOS_RSA_SCRATCH_MAX_COUNT];
> +	int i;
> +
> +	if (sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, sizeof(rsa),
> +			       guc_fw->rsa_offset) != sizeof(rsa))
> +		return -EINVAL;
> +
> +	for (i = 0; i < UOS_RSA_SCRATCH_MAX_COUNT; i++)
> +		I915_WRITE(UOS_RSA_SCRATCH(i), rsa[i]);
> +
> +	return 0;
>   }
>   
>   /*
> @@ -122,29 +152,19 @@ static inline bool guc_ucode_response(struct drm_i915_private *dev_priv,
>    * Architecturally, the DMA engine is bidirectional, and can potentially even
>    * transfer between GTT locations. This functionality is left out of the API
>    * for now as there is no need for it.
> - *
> - * Note that GuC needs the CSS header plus uKernel code to be copied by the
> - * DMA engine in one operation, whereas the RSA signature is loaded via MMIO.
>    */
> -static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
> -			      struct i915_vma *vma)
> +static int guc_ucode_xfer_dma(struct intel_uc_fw *guc_fw, struct i915_vma *vma)
>   {
> -	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
> +	struct intel_guc *guc = container_of(guc_fw, struct intel_guc, fw);
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   	unsigned long offset;
> -	struct sg_table *sg = vma->pages;
> -	u32 status, rsa[UOS_RSA_SCRATCH_MAX_COUNT];
> -	int i, ret = 0;
> -
> -	/* where RSA signature starts */
> -	offset = guc_fw->rsa_offset;
> -
> -	/* Copy RSA signature from the fw image to HW for verification */
> -	sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, sizeof(rsa), offset);
> -	for (i = 0; i < UOS_RSA_SCRATCH_MAX_COUNT; i++)
> -		I915_WRITE(UOS_RSA_SCRATCH(i), rsa[i]);
> +	u32 status;
> +	int ret;
>   
> -	/* The header plus uCode will be copied to WOPCM via DMA, excluding any
> -	 * other components */
> +	/*
> +	 * The header plus uCode will be copied to WOPCM via DMA, excluding any
> +	 * other components
> +	 */
>   	I915_WRITE(DMA_COPY_SIZE, guc_fw->header_size + guc_fw->ucode_size);
>   
>   	/* Set the source address for the new blob */
> @@ -162,26 +182,55 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
>   	/* Finally start the DMA */
>   	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA));
>   
> +	/* Wait for DMA to finish */
> +	ret = __intel_wait_for_register_fw(dev_priv, DMA_CTRL, START_DMA, 0,
> +					   2, 100, &status);
> +	DRM_DEBUG_DRIVER("GuC DMA status %#x\n", status);
> +
Will it be valuable to create dma_response function for this wait? Might 
be able to use in HuC too.
> +	return ret;
> +}
> +
> +/*
> + * Read the GuC status register (GUC_STATUS) and store it in the
> + * specified location; then return a boolean indicating whether
> + * the value matches either of two values representing completion
> + * of the GuC boot process.
> + *
> + * This is used for polling the GuC status in a wait_for()
> + * loop below.
> + */
> +static inline bool guc_ucode_response(struct intel_guc *guc, u32 *status)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	u32 val = I915_READ(GUC_STATUS);
> +	u32 uk_val = val & GS_UKERNEL_MASK;
> +
> +	*status = val;
> +	return (uk_val == GS_UKERNEL_READY) ||
> +		((val & GS_MIA_CORE_STATE) && (uk_val == GS_UKERNEL_LAPIC_DONE));
> +}
> +
> +static int guc_ucode_wait(struct intel_guc *guc)
> +{
> +	u32 status;
> +	int ret;
> +
>   	/*
> -	 * Wait for the DMA to complete & the GuC to start up.
> +	 * Wait for the GuC to start up.
>   	 * NB: Docs recommend not using the interrupt for completion.
>   	 * Measurements indicate this should take no more than 20ms, so a
>   	 * timeout here indicates that the GuC has failed and is unusable.
>   	 * (Higher levels of the driver will attempt to fall back to
>   	 * execlist mode if this happens.)
>   	 */
> -	ret = wait_for(guc_ucode_response(dev_priv, &status), 100);
> -
> -	DRM_DEBUG_DRIVER("DMA status 0x%x, GuC status 0x%x\n",
> -			I915_READ(DMA_CTRL), status);
> +	ret = wait_for(guc_ucode_response(guc, &status), 100);
> +	DRM_DEBUG_DRIVER("GuC status %#x\n", status);
>   
>   	if ((status & GS_BOOTROM_MASK) == GS_BOOTROM_RSA_FAILED) {
>   		DRM_ERROR("GuC firmware signature verification failed\n");
>   		ret = -ENOEXEC;
>   	}
>   
> -	DRM_DEBUG_DRIVER("returning %d\n", ret);
> -
>   	return ret;
>   }
>   
> @@ -198,34 +247,24 @@ static int guc_ucode_xfer(struct intel_uc_fw *guc_fw, struct i915_vma *vma)
>   
>   	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>   
> -	/* Enable MIA caching. GuC clock gating is disabled. */
> -	I915_WRITE(GUC_SHIM_CONTROL, GUC_SHIM_CONTROL_VALUE);
> -
> -	/* WaDisableMinuteIaClockGating:bxt */
> -	if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_A1)) {
> -		I915_WRITE(GUC_SHIM_CONTROL, (I915_READ(GUC_SHIM_CONTROL) &
> -					      ~GUC_ENABLE_MIA_CLOCK_GATING));
> -	}
> -
> -	/* WaC6DisallowByGfxPause:bxt */
> -	if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_B0))
> -		I915_WRITE(GEN6_GFXPAUSE, 0x30FFF);
> -
> -	if (IS_GEN9_LP(dev_priv))
> -		I915_WRITE(GEN9LP_GT_PM_CONFIG, GT_DOORBELL_ENABLE);
> -	else
> -		I915_WRITE(GEN9_GT_PM_CONFIG, GT_DOORBELL_ENABLE);
> +	guc_ucode_xfer_prepare(dev_priv);
>   
> -	if (IS_GEN9(dev_priv)) {
> -		/* DOP Clock Gating Enable for GuC clocks */
> -		I915_WRITE(GEN7_MISCCPCTL, (GEN8_DOP_CLOCK_GATE_GUC_ENABLE |
> -					    I915_READ(GEN7_MISCCPCTL)));
> +	/*
> +	 * Note that GuC needs the CSS header plus uKernel code to be copied
> +	 * by the DMA engine in one operation, whereas the RSA signature is
> +	 * loaded via MMIO.
> +	 */
> +	ret = guc_ucode_xfer_rsa(guc_fw, vma);
> +	if (ret)
> +		DRM_WARN("GuC firmware signature upload error %d\n", ret);
Unless there is a need that I am unaware of, can we rename as:
s/guc_ucode_xfer/guc_ucode_upload
s/guc_ucode_xfer_rsa/guc_ucode_upload_rsa
s/guc_ucode_xfer_dma/dma_upload_guc_ucode (DMA can xfer GuC/HuC)
>   
> -		/* allows for 5us (in 10ns units) before GT can go to RC6 */
> -		I915_WRITE(GUC_ARAT_C6DIS, 0x1FF);
> -	}
> +	ret = guc_ucode_xfer_dma(guc_fw, vma);
> +	if (ret)
> +		DRM_WARN("GuC firmware upload error %d\n", ret);
>   
> -	ret = guc_ucode_xfer_dma(dev_priv, vma);
> +	ret = guc_ucode_wait(guc);
> +	if (ret)
> +		DRM_ERROR("GuC firmware error %d\n", ret);
>   
>   	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>   



More information about the Intel-gfx mailing list