All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/guc: Split GuC firmware xfer function into clear steps
@ 2017-10-27 17:15 Michal Wajdeczko
  2017-10-27 17:38 ` ✗ Fi.CI.BAT: failure for " Patchwork
  2017-10-30 14:00 ` [PATCH] " Sagar Arun Kamble
  0 siblings, 2 replies; 5+ messages in thread
From: Michal Wajdeczko @ 2017-10-27 17:15 UTC (permalink / raw)
  To: intel-gfx

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@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@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. */
+	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);
+
+	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);
 
-		/* 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);
 
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915/guc: Split GuC firmware xfer function into clear steps
  2017-10-27 17:15 [PATCH] drm/i915/guc: Split GuC firmware xfer function into clear steps Michal Wajdeczko
@ 2017-10-27 17:38 ` Patchwork
  2017-10-30 14:00 ` [PATCH] " Sagar Arun Kamble
  1 sibling, 0 replies; 5+ messages in thread
From: Patchwork @ 2017-10-27 17:38 UTC (permalink / raw)
  To: Michal Wajdeczko; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/guc: Split GuC firmware xfer function into clear steps
URL   : https://patchwork.freedesktop.org/series/32768/
State : failure

== Summary ==

Series 32768v1 drm/i915/guc: Split GuC firmware xfer function into clear steps
https://patchwork.freedesktop.org/api/1.0/series/32768/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-wb-ro-default:
                pass       -> INCOMPLETE (fi-cnl-y)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> DMESG-WARN (fi-bdw-5557u) fdo#102473

fdo#102473 https://bugs.freedesktop.org/show_bug.cgi?id=102473

fi-bdw-5557u     total:289  pass:267  dwarn:1   dfail:0   fail:0   skip:21  time:436s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:447s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:373s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:522s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:265s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:495s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:493s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:491s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:470s
fi-cnl-y         total:71   pass:54   dwarn:0   dfail:0   fail:0   skip:16 
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:421s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:249s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:583s
fi-glk-dsi       total:289  pass:258  dwarn:0   dfail:0   fail:1   skip:30  time:488s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:428s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:428s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:420s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:488s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:455s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:487s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:569s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:482s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:584s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:545s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:451s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:594s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:647s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:517s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:494s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:454s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:562s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:417s

4efa630855362c267af94785e50948bb60615bfe drm-tip: 2017y-10m-27d-15h-25m-04s UTC integration manifest
edcb016319f6 drm/i915/guc: Split GuC firmware xfer function into clear steps

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6242/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915/guc: Split GuC firmware xfer function into clear steps
  2017-10-27 17:15 [PATCH] drm/i915/guc: Split GuC firmware xfer function into clear steps Michal Wajdeczko
  2017-10-27 17:38 ` ✗ Fi.CI.BAT: failure for " Patchwork
@ 2017-10-30 14:00 ` Sagar Arun Kamble
  2017-11-01 14:24   ` Michal Wajdeczko
  1 sibling, 1 reply; 5+ messages in thread
From: Sagar Arun Kamble @ 2017-10-30 14:00 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx



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@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@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);
>   

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915/guc: Split GuC firmware xfer function into clear steps
  2017-10-30 14:00 ` [PATCH] " Sagar Arun Kamble
@ 2017-11-01 14:24   ` Michal Wajdeczko
  2017-11-01 14:39     ` Sagar Arun Kamble
  0 siblings, 1 reply; 5+ messages in thread
From: Michal Wajdeczko @ 2017-11-01 14:24 UTC (permalink / raw)
  To: intel-gfx, Sagar Arun Kamble

On Mon, 30 Oct 2017 15:00:52 +0100, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

>
>
> 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@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@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.

Not yet. And I would rather export intel_guc_dma_xfer_ucode() and do this
in separate patch that will make this function also usable for HuC  
transfer.

>> +	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

Note that intel_uc_fw_upload() takes "xfer" func pointer as parameter..

> s/guc_ucode_xfer_rsa/guc_ucode_upload_rsa

guc_mmio_xfer_rsa ?

> s/guc_ucode_xfer_dma/dma_upload_guc_ucode (DMA can xfer GuC/HuC)

guc_dma_xfer_ucode ?

>>   -		/* 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);
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915/guc: Split GuC firmware xfer function into clear steps
  2017-11-01 14:24   ` Michal Wajdeczko
@ 2017-11-01 14:39     ` Sagar Arun Kamble
  0 siblings, 0 replies; 5+ messages in thread
From: Sagar Arun Kamble @ 2017-11-01 14:39 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx



On 11/1/2017 7:54 PM, Michal Wajdeczko wrote:
> On Mon, 30 Oct 2017 15:00:52 +0100, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>>
>>
>> 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@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: Sagar Arun Kamble <sagar.a.kamble@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.
>
> Not yet. And I would rather export intel_guc_dma_xfer_ucode() and do this
> in separate patch that will make this function also usable for HuC 
> transfer.
Sure.
>
>>> +    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
>
> Note that intel_uc_fw_upload() takes "xfer" func pointer as parameter..
Wanted either upload (except for xfer in "dma xfer") or xfer everywhere. 
Can be done in separate patch I guess.
>
>> s/guc_ucode_xfer_rsa/guc_ucode_upload_rsa
>
> guc_mmio_xfer_rsa ?
This looks good.
>
>> s/guc_ucode_xfer_dma/dma_upload_guc_ucode (DMA can xfer GuC/HuC)
>
> guc_dma_xfer_ucode ?
This too looks good.
>
>>>   -        /* 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);
>>>

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-11-01 14:39 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-27 17:15 [PATCH] drm/i915/guc: Split GuC firmware xfer function into clear steps Michal Wajdeczko
2017-10-27 17:38 ` ✗ Fi.CI.BAT: failure for " Patchwork
2017-10-30 14:00 ` [PATCH] " Sagar Arun Kamble
2017-11-01 14:24   ` Michal Wajdeczko
2017-11-01 14:39     ` Sagar Arun Kamble

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.