diff mbox series

[v2,8/8] drm/i915/uc: Unify uC firmware upload

Message ID 20190724022153.8927-9-daniele.ceraolospurio@intel.com (mailing list archive)
State New, archived
Headers show
Series uC fw path unification + misc clean-up | expand

Commit Message

Daniele Ceraolo Spurio July 24, 2019, 2:21 a.m. UTC
The way we load the firmwares is the same for both GuC and HuC, the only
difference is in the wopcm destination address and the dma flags, so we
easily can move the logic to a common function and pass in offset and
flags. The only other difference in the uplaod path are some the extra
steps that guc does before and after the xfer, but those don't require
the guc fw to be pinned in ggtt and can safely be performed before
calling the uc_upload function.

Note that this patch re-introduces the dma xfer wait for guc loading that
was removed with "drm/i915/guc: Propagate the fw xfer timeout". This is
not going to slow us down on a successful load (the dma has to complete
before fw init can start), but could slightly increase the timeout in case
of a fw init error.

v2: use _fw variants for uncore accesses (Chris), fix guc_fw status on
    failed wait.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c | 107 ++++++----------------
 drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c |  57 +-----------
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c  |  79 ++++++++++++----
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h  |   4 +-
 4 files changed, 92 insertions(+), 155 deletions(-)

Comments

Chris Wilson July 24, 2019, 8:26 a.m. UTC | #1
Quoting Daniele Ceraolo Spurio (2019-07-24 03:21:53)
> +static int uc_fw_xfer(struct intel_uc_fw *uc_fw, struct intel_gt *gt,
> +                     u32 wopcm_offset, u32 dma_flags)
> +{
> +       struct intel_uncore *uncore = gt->uncore;
> +       u64 offset;
> +       int ret;
> +
> +       intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
> +
> +       /* Set the source address for the uCode */
> +       offset = uc_fw_ggtt_offset(uc_fw, gt->ggtt) + uc_fw->header_offset;
> +       GEM_BUG_ON(upper_32_bits(offset) & 0xFFFF0000);
> +       intel_uncore_write_fw(uncore, DMA_ADDR_0_LOW, lower_32_bits(offset));
> +       intel_uncore_write_fw(uncore, DMA_ADDR_0_HIGH, upper_32_bits(offset));
> +
> +       /* Set the DMA destination */
> +       intel_uncore_write_fw(uncore, DMA_ADDR_1_LOW, wopcm_offset);
> +       intel_uncore_write_fw(uncore, DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
> +
> +       /*
> +        * Set the transfer size. The header plus uCode will be copied to WOPCM
> +        * via DMA, excluding any other components
> +        */
> +       intel_uncore_write_fw(uncore, DMA_COPY_SIZE,
> +                             uc_fw->header_size + uc_fw->ucode_size);
> +
> +       /* Start the DMA */
> +       intel_uncore_write_fw(uncore, DMA_CTRL,
> +                             _MASKED_BIT_ENABLE(dma_flags | START_DMA));
> +
> +       /* Wait for DMA to finish */
> +       ret = intel_wait_for_register_fw(uncore, DMA_CTRL, START_DMA, 0, 100);
> +       if (ret)
> +               DRM_ERROR("DMA for %s fw failed, err=%d\n",
> +                         intel_uc_fw_type_repr(uc_fw->type), ret);

Note the errno will never be that interesting here, it's a wait. I would
suggest including DMA_CTL as that looks like it contains status bits as
well as control.

And let's note use DRM_ERROR() if we can help it, and use dev_err() when
applicable. (Might be time for a new crusade.)

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
index dcb4ddce659a..0396579cee76 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
@@ -86,13 +86,6 @@  static void guc_xfer_rsa(struct intel_uc_fw *guc_fw,
 		intel_uncore_write(uncore, UOS_RSA_SCRATCH(i), rsa[i]);
 }
 
-static bool guc_xfer_completed(struct intel_uncore *uncore, u32 *status)
-{
-	/* Did we complete the xfer? */
-	*status = intel_uncore_read(uncore, DMA_CTRL);
-	return !(*status & START_DMA);
-}
-
 /*
  * Read the GuC status register (GUC_STATUS) and store it in the
  * specified location; then return a boolean indicating whether
@@ -139,65 +132,27 @@  static int guc_wait_ucode(struct intel_uncore *uncore)
 		ret = -ENXIO;
 	}
 
-	if (ret == 0 && !guc_xfer_completed(uncore, &status)) {
-		DRM_ERROR("GuC is ready, but the xfer %08x is incomplete\n",
-			  status);
-		ret = -ENXIO;
-	}
-
 	return ret;
 }
 
-/*
- * Transfer the firmware image to RAM for execution by the microcontroller.
+/**
+ * intel_guc_fw_upload() - load GuC uCode to device
+ * @guc: intel_guc structure
  *
- * 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.
- */
-static int guc_xfer_ucode(struct intel_uc_fw *guc_fw,
-			  struct intel_gt *gt)
-{
-	struct intel_uncore *uncore = gt->uncore;
-	unsigned long offset;
-
-	/*
-	 * The header plus uCode will be copied to WOPCM via DMA, excluding any
-	 * other components
-	 */
-	intel_uncore_write(uncore, DMA_COPY_SIZE,
-			   guc_fw->header_size + guc_fw->ucode_size);
-
-	/* Set the source address for the new blob */
-	offset = intel_uc_fw_ggtt_offset(guc_fw, gt->ggtt) + guc_fw->header_offset;
-	intel_uncore_write(uncore, DMA_ADDR_0_LOW, lower_32_bits(offset));
-	intel_uncore_write(uncore, DMA_ADDR_0_HIGH, upper_32_bits(offset) & 0xFFFF);
-
-	/*
-	 * Set the DMA destination. Current uCode expects the code to be
-	 * loaded at 8k; locations below this are used for the stack.
-	 */
-	intel_uncore_write(uncore, DMA_ADDR_1_LOW, 0x2000);
-	intel_uncore_write(uncore, DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
-
-	/* Finally start the DMA */
-	intel_uncore_write(uncore, DMA_CTRL,
-			   _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA));
-
-	return guc_wait_ucode(uncore);
-}
-/*
- * Load the GuC firmware blob into the MinuteIA.
+ * Called from intel_uc_init_hw() during driver load, resume from sleep and
+ * after a GPU reset.
+ *
+ * The firmware image should have already been fetched into memory, so only
+ * check that fetch succeeded, and then transfer the image to the h/w.
+ *
+ * Return:	non-zero code on error
  */
-static int guc_fw_xfer(struct intel_uc_fw *guc_fw, struct intel_gt *gt)
+int intel_guc_fw_upload(struct intel_guc *guc)
 {
+	struct intel_gt *gt = guc_to_gt(guc);
 	struct intel_uncore *uncore = gt->uncore;
 	int ret;
 
-	GEM_BUG_ON(guc_fw->type != INTEL_UC_FW_TYPE_GUC);
-
-	intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
-
 	guc_prepare_xfer(uncore);
 
 	/*
@@ -205,32 +160,24 @@  static int guc_fw_xfer(struct intel_uc_fw *guc_fw, struct intel_gt *gt)
 	 * by the DMA engine in one operation, whereas the RSA signature is
 	 * loaded via MMIO.
 	 */
-	guc_xfer_rsa(guc_fw, uncore);
+	guc_xfer_rsa(&guc->fw, uncore);
 
-	ret = guc_xfer_ucode(guc_fw, gt);
-
-	intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
+	/*
+	 * Current uCode expects the code to be loaded at 8k; locations below
+	 * this are used for the stack.
+	 */
+	ret = intel_uc_fw_upload(&guc->fw, gt, 0x2000, UOS_MOVE);
+	if (ret)
+		goto out;
 
-	return ret;
-}
+	ret = guc_wait_ucode(uncore);
+	if (ret)
+		goto out;
 
-/**
- * intel_guc_fw_upload() - load GuC uCode to device
- * @guc: intel_guc structure
- *
- * Called from intel_uc_init_hw() during driver load, resume from sleep and
- * after a GPU reset.
- *
- * The firmware image should have already been fetched into memory, so only
- * check that fetch succeeded, and then transfer the image to the h/w.
- *
- * Return:	non-zero code on error
- */
-int intel_guc_fw_upload(struct intel_guc *guc)
-{
-	int ret = intel_uc_fw_upload(&guc->fw, guc_to_gt(guc), guc_fw_xfer);
-	if (!ret)
-		guc->fw.status = INTEL_UC_FIRMWARE_RUNNING;
+	guc->fw.status = INTEL_UC_FIRMWARE_RUNNING;
+	return 0;
 
+out:
+	guc->fw.status = INTEL_UC_FIRMWARE_LOAD_FAIL;
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
index 05342ee0d156..9eaa6e9a5385 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
@@ -36,60 +36,6 @@  void intel_huc_fw_init_early(struct intel_huc *huc)
 	intel_uc_fw_init_early(huc_to_gt(huc)->i915, huc_fw, INTEL_UC_FW_TYPE_HUC);
 }
 
-/**
- * huc_fw_xfer() - DMA's the firmware
- * @huc_fw: the firmware descriptor
- *
- * Transfer the firmware image to RAM for execution by the microcontroller.
- *
- * Return: 0 on success, non-zero on failure
- */
-static int huc_fw_xfer(struct intel_uc_fw *huc_fw, struct intel_gt *gt)
-{
-	struct intel_uncore *uncore = gt->uncore;
-	unsigned long offset = 0;
-	u32 size;
-	int ret;
-
-	GEM_BUG_ON(huc_fw->type != INTEL_UC_FW_TYPE_HUC);
-
-	intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
-
-	/* Set the source address for the uCode */
-	offset = intel_uc_fw_ggtt_offset(huc_fw, gt->ggtt) +
-		 huc_fw->header_offset;
-	intel_uncore_write(uncore, DMA_ADDR_0_LOW,
-			   lower_32_bits(offset));
-	intel_uncore_write(uncore, DMA_ADDR_0_HIGH,
-			   upper_32_bits(offset) & 0xFFFF);
-
-	/*
-	 * Hardware doesn't look at destination address for HuC. Set it to 0,
-	 * but still program the correct address space.
-	 */
-	intel_uncore_write(uncore, DMA_ADDR_1_LOW, 0);
-	intel_uncore_write(uncore, DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
-
-	size = huc_fw->header_size + huc_fw->ucode_size;
-	intel_uncore_write(uncore, DMA_COPY_SIZE, size);
-
-	/* Start the DMA */
-	intel_uncore_write(uncore, DMA_CTRL,
-			   _MASKED_BIT_ENABLE(HUC_UKERNEL | START_DMA));
-
-	/* Wait for DMA to finish */
-	ret = intel_wait_for_register_fw(uncore, DMA_CTRL, START_DMA, 0, 100);
-
-	DRM_DEBUG_DRIVER("HuC DMA transfer wait over with ret %d\n", ret);
-
-	/* Disable the bits once DMA is over */
-	intel_uncore_write(uncore, DMA_CTRL, _MASKED_BIT_DISABLE(HUC_UKERNEL));
-
-	intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
-
-	return ret;
-}
-
 /**
  * intel_huc_fw_upload() - load HuC uCode to device
  * @huc: intel_huc structure
@@ -104,5 +50,6 @@  static int huc_fw_xfer(struct intel_uc_fw *huc_fw, struct intel_gt *gt)
  */
 int intel_huc_fw_upload(struct intel_huc *huc)
 {
-	return intel_uc_fw_upload(&huc->fw, huc_to_gt(huc), huc_fw_xfer);
+	/* HW doesn't look at destination address for HuC, so set it to 0 */
+	return intel_uc_fw_upload(&huc->fw, huc_to_gt(huc), 0, HUC_UKERNEL);
 }
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index ea1452dbe78c..05feef354f33 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -325,13 +325,24 @@  void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 	release_firmware(fw);		/* OK even if fw is NULL */
 }
 
+static u32 uc_fw_ggtt_offset(struct intel_uc_fw *uc_fw, struct i915_ggtt *ggtt)
+{
+	struct drm_mm_node *node = &ggtt->uc_fw;
+
+	GEM_BUG_ON(!node->allocated);
+	GEM_BUG_ON(upper_32_bits(node->start));
+	GEM_BUG_ON(upper_32_bits(node->start + node->size - 1));
+
+	return lower_32_bits(node->start);
+}
+
 static void intel_uc_fw_ggtt_bind(struct intel_uc_fw *uc_fw,
 				  struct intel_gt *gt)
 {
 	struct drm_i915_gem_object *obj = uc_fw->obj;
 	struct i915_ggtt *ggtt = gt->ggtt;
 	struct i915_vma dummy = {
-		.node.start = intel_uc_fw_ggtt_offset(uc_fw, ggtt),
+		.node.start = uc_fw_ggtt_offset(uc_fw, ggtt),
 		.node.size = obj->base.size,
 		.pages = obj->mm.pages,
 		.vm = &ggtt->vm,
@@ -351,23 +362,68 @@  static void intel_uc_fw_ggtt_unbind(struct intel_uc_fw *uc_fw,
 {
 	struct drm_i915_gem_object *obj = uc_fw->obj;
 	struct i915_ggtt *ggtt = gt->ggtt;
-	u64 start = intel_uc_fw_ggtt_offset(uc_fw, ggtt);
+	u64 start = uc_fw_ggtt_offset(uc_fw, ggtt);
 
 	ggtt->vm.clear_range(&ggtt->vm, start, obj->base.size);
 }
 
+static int uc_fw_xfer(struct intel_uc_fw *uc_fw, struct intel_gt *gt,
+		      u32 wopcm_offset, u32 dma_flags)
+{
+	struct intel_uncore *uncore = gt->uncore;
+	u64 offset;
+	int ret;
+
+	intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
+
+	/* Set the source address for the uCode */
+	offset = uc_fw_ggtt_offset(uc_fw, gt->ggtt) + uc_fw->header_offset;
+	GEM_BUG_ON(upper_32_bits(offset) & 0xFFFF0000);
+	intel_uncore_write_fw(uncore, DMA_ADDR_0_LOW, lower_32_bits(offset));
+	intel_uncore_write_fw(uncore, DMA_ADDR_0_HIGH, upper_32_bits(offset));
+
+	/* Set the DMA destination */
+	intel_uncore_write_fw(uncore, DMA_ADDR_1_LOW, wopcm_offset);
+	intel_uncore_write_fw(uncore, DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
+
+	/*
+	 * Set the transfer size. The header plus uCode will be copied to WOPCM
+	 * via DMA, excluding any other components
+	 */
+	intel_uncore_write_fw(uncore, DMA_COPY_SIZE,
+			      uc_fw->header_size + uc_fw->ucode_size);
+
+	/* Start the DMA */
+	intel_uncore_write_fw(uncore, DMA_CTRL,
+			      _MASKED_BIT_ENABLE(dma_flags | START_DMA));
+
+	/* Wait for DMA to finish */
+	ret = intel_wait_for_register_fw(uncore, DMA_CTRL, START_DMA, 0, 100);
+	if (ret)
+		DRM_ERROR("DMA for %s fw failed, err=%d\n",
+			  intel_uc_fw_type_repr(uc_fw->type), ret);
+
+	/* Disable the bits once DMA is over */
+	intel_uncore_write_fw(uncore, DMA_CTRL, _MASKED_BIT_DISABLE(dma_flags));
+
+	intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
+
+	return ret;
+}
+
 /**
  * intel_uc_fw_upload - load uC firmware using custom loader
  * @uc_fw: uC firmware
  * @gt: the intel_gt structure
- * @xfer: custom uC firmware loader function
+ * @wopcm_offset: destination offset in wopcm
+ * @dma_flags: flags for flags for dma ctrl
  *
- * Loads uC firmware using custom loader and updates internal flags.
+ * Loads uC firmware and updates internal flags.
  *
  * Return: 0 on success, non-zero on failure.
  */
 int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, struct intel_gt *gt,
-		       int (*xfer)(struct intel_uc_fw *uc_fw, struct intel_gt *gt))
+		       u32 wopcm_offset, u32 dma_flags)
 {
 	int err;
 
@@ -381,7 +437,7 @@  int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, struct intel_gt *gt,
 		return -ENOEXEC;
 	/* Call custom loader */
 	intel_uc_fw_ggtt_bind(uc_fw, gt);
-	err = xfer(uc_fw, gt);
+	err = uc_fw_xfer(uc_fw, gt, wopcm_offset, dma_flags);
 	intel_uc_fw_ggtt_unbind(uc_fw, gt);
 	if (err)
 		goto fail;
@@ -434,17 +490,6 @@  void intel_uc_fw_fini(struct intel_uc_fw *uc_fw)
 	i915_gem_object_unpin_pages(uc_fw->obj);
 }
 
-u32 intel_uc_fw_ggtt_offset(struct intel_uc_fw *uc_fw, struct i915_ggtt *ggtt)
-{
-	struct drm_mm_node *node = &ggtt->uc_fw;
-
-	GEM_BUG_ON(!node->allocated);
-	GEM_BUG_ON(upper_32_bits(node->start));
-	GEM_BUG_ON(upper_32_bits(node->start + node->size - 1));
-
-	return lower_32_bits(node->start);
-}
-
 /**
  * intel_uc_fw_cleanup_fetch - cleanup uC firmware
  *
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
index cbd3aac7acf9..5b82ddad5891 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
@@ -31,7 +31,6 @@ 
 struct drm_printer;
 struct drm_i915_private;
 struct intel_gt;
-struct i915_ggtt;
 
 /* Home of GuC, HuC and DMC firmwares */
 #define INTEL_UC_FIRMWARE_URL "https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/i915"
@@ -175,10 +174,9 @@  void intel_uc_fw_fetch(struct drm_i915_private *i915,
 		       struct intel_uc_fw *uc_fw);
 void intel_uc_fw_cleanup_fetch(struct intel_uc_fw *uc_fw);
 int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, struct intel_gt *gt,
-		       int (*xfer)(struct intel_uc_fw *uc_fw, struct intel_gt *gt));
+		       u32 wopcm_offset, u32 dma_flags);
 int intel_uc_fw_init(struct intel_uc_fw *uc_fw);
 void intel_uc_fw_fini(struct intel_uc_fw *uc_fw);
-u32 intel_uc_fw_ggtt_offset(struct intel_uc_fw *uc_fw, struct i915_ggtt *ggtt);
 size_t intel_uc_fw_copy_rsa(struct intel_uc_fw *uc_fw, void *dst, u32 max_len);
 void intel_uc_fw_dump(const struct intel_uc_fw *uc_fw, struct drm_printer *p);