[PATCH v2 0/5] Perma-pin uC firmware and re-enable global reset

[PATCH v2 0/5] Perma-pin uC firmware and re-enable global reset

[Fernando Pacheco]

The intent is to move the GuC and HuC firmware images to the
top of the address space. This portion is inaccessible during
normal GuC operations and should be relatively safe to house
both firmware images. By making the move we can re-enable the
full gpu reset with GuC enabled.

Placing the firmware images above GUC_GGTT_TOP was discussed
previously here:
	https://patchwork.freedesktop.org/patch/273616/

v2:
The decision to rename both the uc_fw init and fini functions
made it easier to pull the bind/unbind operations out of
intel_guc_fw.* and intel_huc_fw.*. The bind/unbind will now
take place within the newly repurposed intel_uc_fw_init/fini.
All other changes should be called out in their respective patches
and should be the direct result of a review comment.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Fernando Pacheco (5):
  drm/i915/uc: Rename uC firmware init/fini functions
  drm/i915/uc: Reserve upper range of GGTT
  drm/i915/uc: Place uC firmware in upper range of GGTT
  Revert "drm/i915/guc: Disable global reset"
  drm/i915/selftests: Check that gpu reset is usable from atomic context

 drivers/gpu/drm/i915/i915_gem.c               |   2 +
 drivers/gpu/drm/i915/i915_gem_gtt.c           |  25 ++--
 drivers/gpu/drm/i915/i915_gem_gtt.h           |   1 +
 drivers/gpu/drm/i915/i915_reset.c             |   3 -
 drivers/gpu/drm/i915/intel_guc.c              |  58 ++++++++-
 drivers/gpu/drm/i915/intel_guc.h              |   2 +
 drivers/gpu/drm/i915/intel_guc_fw.c           |  20 +--
 drivers/gpu/drm/i915/intel_huc.c              |  74 ++++++++---
 drivers/gpu/drm/i915/intel_huc.h              |   6 +-
 drivers/gpu/drm/i915/intel_huc_fw.c           |  49 +++++--
 drivers/gpu/drm/i915/intel_uc.c               |  39 +++++-
 drivers/gpu/drm/i915/intel_uc.h               |   1 +
 drivers/gpu/drm/i915/intel_uc_fw.c            | 122 +++++++++++++-----
 drivers/gpu/drm/i915/intel_uc_fw.h            |  12 +-
 .../gpu/drm/i915/selftests/intel_hangcheck.c  |   6 +-
 15 files changed, 319 insertions(+), 101 deletions(-)

-- 
2.21.0

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

[PATCH v2 1/5] drm/i915/uc: Rename uC firmware init/fini functions

[Fernando Pacheco]

The uC firmware init function is called during
GuC/HuC init early phases. Rename to include "_early"
and properly reflect which phase we are at.

The uC firmware fini function is cleaning up the
state set/created on firmware fetch. Replace
"_fini" with "_cleanup_fetch".

v2: also rename uC fw fini function

Signed-off-by: Fernando Pacheco <fernando.pacheco@intel.com>
---
 drivers/gpu/drm/i915/intel_guc.c    | 6 +++---
 drivers/gpu/drm/i915/intel_guc_fw.c | 2 +-
 drivers/gpu/drm/i915/intel_huc.h    | 2 +-
 drivers/gpu/drm/i915/intel_huc_fw.c | 2 +-
 drivers/gpu/drm/i915/intel_uc_fw.c  | 4 ++--
 drivers/gpu/drm/i915/intel_uc_fw.h  | 5 +++--
 6 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 3aabfa2d9198..d81a02b0f525 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -154,7 +154,7 @@ int intel_guc_init_misc(struct intel_guc *guc)
 
 void intel_guc_fini_misc(struct intel_guc *guc)
 {
-	intel_uc_fw_fini(&guc->fw);
+	intel_uc_fw_cleanup_fetch(&guc->fw);
 	guc_fini_wq(guc);
 }
 
@@ -221,7 +221,7 @@ int intel_guc_init(struct intel_guc *guc)
 err_shared:
 	guc_shared_data_destroy(guc);
 err_fetch:
-	intel_uc_fw_fini(&guc->fw);
+	intel_uc_fw_cleanup_fetch(&guc->fw);
 	return ret;
 }
 
@@ -237,7 +237,7 @@ void intel_guc_fini(struct intel_guc *guc)
 	intel_guc_ads_destroy(guc);
 	intel_guc_log_destroy(&guc->log);
 	guc_shared_data_destroy(guc);
-	intel_uc_fw_fini(&guc->fw);
+	intel_uc_fw_cleanup_fetch(&guc->fw);
 }
 
 static u32 guc_ctl_debug_flags(struct intel_guc *guc)
diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
index 792a551450c7..4385d9ef02bb 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/intel_guc_fw.c
@@ -90,7 +90,7 @@ void intel_guc_fw_init_early(struct intel_guc *guc)
 {
 	struct intel_uc_fw *guc_fw = &guc->fw;
 
-	intel_uc_fw_init(guc_fw, INTEL_UC_FW_TYPE_GUC);
+	intel_uc_fw_init_early(guc_fw, INTEL_UC_FW_TYPE_GUC);
 	guc_fw_select(guc_fw);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
index 7e41d870b509..ce129e301961 100644
--- a/drivers/gpu/drm/i915/intel_huc.h
+++ b/drivers/gpu/drm/i915/intel_huc.h
@@ -42,7 +42,7 @@ int intel_huc_check_status(struct intel_huc *huc);
 
 static inline void intel_huc_fini_misc(struct intel_huc *huc)
 {
-	intel_uc_fw_fini(&huc->fw);
+	intel_uc_fw_cleanup_fetch(&huc->fw);
 }
 
 static inline int intel_huc_sanitize(struct intel_huc *huc)
diff --git a/drivers/gpu/drm/i915/intel_huc_fw.c b/drivers/gpu/drm/i915/intel_huc_fw.c
index 68d47c105939..80a176d91edc 100644
--- a/drivers/gpu/drm/i915/intel_huc_fw.c
+++ b/drivers/gpu/drm/i915/intel_huc_fw.c
@@ -89,7 +89,7 @@ void intel_huc_fw_init_early(struct intel_huc *huc)
 {
 	struct intel_uc_fw *huc_fw = &huc->fw;
 
-	intel_uc_fw_init(huc_fw, INTEL_UC_FW_TYPE_HUC);
+	intel_uc_fw_init_early(huc_fw, INTEL_UC_FW_TYPE_HUC);
 	huc_fw_select(huc_fw);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
index becf05ebae4d..e3e74207a102 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/intel_uc_fw.c
@@ -274,13 +274,13 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
 }
 
 /**
- * intel_uc_fw_fini - cleanup uC firmware
+ * intel_uc_fw_cleanup_fetch - cleanup uC firmware
  *
  * @uc_fw: uC firmware
  *
  * Cleans up uC firmware by releasing the firmware GEM obj.
  */
-void intel_uc_fw_fini(struct intel_uc_fw *uc_fw)
+void intel_uc_fw_cleanup_fetch(struct intel_uc_fw *uc_fw)
 {
 	struct drm_i915_gem_object *obj;
 
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h
index 0e3bd580e267..e6fa8599757c 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/intel_uc_fw.h
@@ -102,7 +102,8 @@ static inline const char *intel_uc_fw_type_repr(enum intel_uc_fw_type type)
 }
 
 static inline
-void intel_uc_fw_init(struct intel_uc_fw *uc_fw, enum intel_uc_fw_type type)
+void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
+			    enum intel_uc_fw_type type)
 {
 	uc_fw->path = NULL;
 	uc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
@@ -144,10 +145,10 @@ static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw)
 
 void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 		       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,
 		       int (*xfer)(struct intel_uc_fw *uc_fw,
 				   struct i915_vma *vma));
-void intel_uc_fw_fini(struct intel_uc_fw *uc_fw);
 void intel_uc_fw_dump(const struct intel_uc_fw *uc_fw, struct drm_printer *p);
 
 #endif
-- 
2.21.0

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

[PATCH v2 2/5] drm/i915/uc: Reserve upper range of GGTT

[Fernando Pacheco]

GuC and HuC depend on struct_mutex for device
reinitialization. Moving away from this dependency
requires perma-pinning the firmware images in GGTT.
The upper portion of the GuC address space has
a sizeable hole (several MB) that is inaccessible
by GuC. Reserve this range within GGTT as it can
comfortably hold GuC/HuC firmware images.

v2: Reserve node rather than insert (Chris)
    Simpler determination of node start/size (Daniele)
    Move reserve/release out to intel_guc.* files

Signed-off-by: Fernando Pacheco <fernando.pacheco@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 25 ++++++++--------
 drivers/gpu/drm/i915/i915_gem_gtt.h |  1 +
 drivers/gpu/drm/i915/intel_guc.c    | 45 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc.h    |  2 ++
 4 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 8f460cc4cc1f..0b4c22e68574 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2752,6 +2752,12 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
 	if (ret)
 		return ret;
 
+	if (USES_GUC(dev_priv)) {
+		ret = intel_guc_reserve_ggtt_top(&dev_priv->guc);
+		if (ret)
+			goto err_reserve;
+	}
+
 	/* Clear any non-preallocated blocks */
 	drm_mm_for_each_hole(entry, &ggtt->vm.mm, hole_start, hole_end) {
 		DRM_DEBUG_KMS("clearing unused GTT space: [%lx, %lx]\n",
@@ -2766,12 +2772,14 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
 	if (INTEL_PPGTT(dev_priv) == INTEL_PPGTT_ALIASING) {
 		ret = i915_gem_init_aliasing_ppgtt(dev_priv);
 		if (ret)
-			goto err;
+			goto err_appgtt;
 	}
 
 	return 0;
 
-err:
+err_appgtt:
+	intel_guc_release_ggtt_top(&dev_priv->guc);
+err_reserve:
 	drm_mm_remove_node(&ggtt->error_capture);
 	return ret;
 }
@@ -2797,6 +2805,8 @@ void i915_ggtt_cleanup_hw(struct drm_i915_private *dev_priv)
 	if (drm_mm_node_allocated(&ggtt->error_capture))
 		drm_mm_remove_node(&ggtt->error_capture);
 
+	intel_guc_release_ggtt_top(&dev_priv->guc);
+
 	if (drm_mm_initialized(&ggtt->vm.mm)) {
 		intel_vgt_deballoon(dev_priv);
 		i915_address_space_fini(&ggtt->vm);
@@ -3369,17 +3379,6 @@ int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv)
 	if (ret)
 		return ret;
 
-	/* Trim the GGTT to fit the GuC mappable upper range (when enabled).
-	 * This is easier than doing range restriction on the fly, as we
-	 * currently don't have any bits spare to pass in this upper
-	 * restriction!
-	 */
-	if (USES_GUC(dev_priv)) {
-		ggtt->vm.total = min_t(u64, ggtt->vm.total, GUC_GGTT_TOP);
-		ggtt->mappable_end =
-			min_t(u64, ggtt->mappable_end, ggtt->vm.total);
-	}
-
 	if ((ggtt->vm.total - 1) >> 32) {
 		DRM_ERROR("We never expected a Global GTT with more than 32bits"
 			  " of address space! Found %lldM!\n",
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index f597f35b109b..b51e779732c3 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -384,6 +384,7 @@ struct i915_ggtt {
 	u32 pin_bias;
 
 	struct drm_mm_node error_capture;
+	struct drm_mm_node uc_fw;
 };
 
 struct i915_hw_ppgtt {
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index d81a02b0f525..ddd246dc3f14 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -721,3 +721,48 @@ u32 intel_guc_reserved_gtt_size(struct intel_guc *guc)
 {
 	return guc_to_i915(guc)->wopcm.guc.size;
 }
+
+static u32 __intel_guc_ggtt_top_offset(struct intel_guc *guc)
+{
+	struct drm_i915_private *i915 = guc_to_i915(guc);
+	struct i915_ggtt *ggtt = &i915->ggtt;
+	struct intel_huc *huc = &i915->huc;
+	u32 guc_fw_size, huc_fw_size;
+	u32 min_reserved_size;
+
+	guc_fw_size = round_up(guc->fw.size, I915_GTT_PAGE_SIZE);
+	huc_fw_size = round_up(huc->fw.size, I915_GTT_PAGE_SIZE);
+
+	min_reserved_size = guc_fw_size + huc_fw_size;
+
+	return min_t(u32, GUC_GGTT_TOP, ggtt->vm.total - min_reserved_size);
+}
+
+int intel_guc_reserve_ggtt_top(struct intel_guc *guc)
+{
+	struct drm_i915_private *i915 = guc_to_i915(guc);
+	struct i915_ggtt *ggtt = &i915->ggtt;
+	u64 start, size;
+	int ret;
+
+	start = __intel_guc_ggtt_top_offset(guc);
+	size = ggtt->vm.total - start;
+
+	ret = i915_gem_gtt_reserve(&ggtt->vm, &ggtt->uc_fw, size,
+				   start, I915_COLOR_UNEVICTABLE,
+				   PIN_NOEVICT);
+
+	if (ret)
+		DRM_DEBUG_DRIVER("GuC: failed to reserve top of ggtt\n");
+
+	return ret;
+}
+
+void intel_guc_release_ggtt_top(struct intel_guc *guc)
+{
+	struct drm_i915_private *i915 = guc_to_i915(guc);
+	struct i915_ggtt *ggtt = &i915->ggtt;
+
+	if (drm_mm_node_allocated(&ggtt->uc_fw))
+		drm_mm_remove_node(&ggtt->uc_fw);
+}
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 2c59ff8d9f39..2494e84831a2 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -173,6 +173,8 @@ int intel_guc_suspend(struct intel_guc *guc);
 int intel_guc_resume(struct intel_guc *guc);
 struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
 u32 intel_guc_reserved_gtt_size(struct intel_guc *guc);
+int intel_guc_reserve_ggtt_top(struct intel_guc *guc);
+void intel_guc_release_ggtt_top(struct intel_guc *guc);
 
 static inline int intel_guc_sanitize(struct intel_guc *guc)
 {
-- 
2.21.0

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

[PATCH v2 3/5] drm/i915/uc: Place uC firmware in upper range of GGTT

[Fernando Pacheco]

Currently we pin the GuC or HuC firmware image just
before uploading. Perma-pin during uC initialization
instead and use the range reserved at the top of the
address space.

Moving the firmware resulted in needing to:
- restore the ggtt mapping during the suspend/resume path.
- use an additional pinning for the rsa signature which will
  be used during HuC auth as addresses above GUC_GGTT_TOP
  do not map through GTT.

v2: Remove call to set to gtt domain
    Do not restore fw gtt mapping unconditionally
    Separate out pin/unpin functions and drop usage of pin/unpin
    Use uc_fw init/fini functions to bind/unbind fw object

Signed-off-by: Fernando Pacheco <fernando.pacheco@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c     |   2 +
 drivers/gpu/drm/i915/intel_guc.c    |   9 ++-
 drivers/gpu/drm/i915/intel_guc_fw.c |  18 +++--
 drivers/gpu/drm/i915/intel_huc.c    |  74 +++++++++++++----
 drivers/gpu/drm/i915/intel_huc.h    |   4 +
 drivers/gpu/drm/i915/intel_huc_fw.c |  47 ++++++++---
 drivers/gpu/drm/i915/intel_uc.c     |  39 ++++++++-
 drivers/gpu/drm/i915/intel_uc.h     |   1 +
 drivers/gpu/drm/i915/intel_uc_fw.c  | 118 +++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_uc_fw.h  |   9 ++-
 10 files changed, 247 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e5462639de0b..0e06a2c7d65f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4508,6 +4508,8 @@ void i915_gem_resume(struct drm_i915_private *i915)
 	i915_gem_restore_gtt_mappings(i915);
 	i915_gem_restore_fences(i915);
 
+	intel_uc_restore_ggtt_mapping(i915);
+
 	/*
 	 * As we didn't flush the kernel context before suspend, we cannot
 	 * guarantee that the context image is complete. So let's just reset
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index ddd246dc3f14..5f3db102c914 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -189,9 +189,13 @@ int intel_guc_init(struct intel_guc *guc)
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	int ret;
 
-	ret = guc_shared_data_create(guc);
+	ret = intel_uc_fw_init(&guc->fw);
 	if (ret)
 		goto err_fetch;
+
+	ret = guc_shared_data_create(guc);
+	if (ret)
+		goto err_fw;
 	GEM_BUG_ON(!guc->shared_data);
 
 	ret = intel_guc_log_create(&guc->log);
@@ -220,6 +224,8 @@ int intel_guc_init(struct intel_guc *guc)
 	intel_guc_log_destroy(&guc->log);
 err_shared:
 	guc_shared_data_destroy(guc);
+err_fw:
+	intel_uc_fw_fini(&guc->fw);
 err_fetch:
 	intel_uc_fw_cleanup_fetch(&guc->fw);
 	return ret;
@@ -237,6 +243,7 @@ void intel_guc_fini(struct intel_guc *guc)
 	intel_guc_ads_destroy(guc);
 	intel_guc_log_destroy(&guc->log);
 	guc_shared_data_destroy(guc);
+	intel_uc_fw_fini(&guc->fw);
 	intel_uc_fw_cleanup_fetch(&guc->fw);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
index 4385d9ef02bb..8b2dcc70b956 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/intel_guc_fw.c
@@ -122,14 +122,16 @@ static void guc_prepare_xfer(struct intel_guc *guc)
 }
 
 /* Copy RSA signature from the fw image to HW for verification */
-static void guc_xfer_rsa(struct intel_guc *guc, struct i915_vma *vma)
+static void guc_xfer_rsa(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	struct intel_uc_fw *fw = &guc->fw;
+	struct sg_table *pages = fw->obj->mm.pages;
 	u32 rsa[UOS_RSA_SCRATCH_COUNT];
 	int i;
 
-	sg_pcopy_to_buffer(vma->pages->sgl, vma->pages->nents,
-			   rsa, sizeof(rsa), guc->fw.rsa_offset);
+	sg_pcopy_to_buffer(pages->sgl, pages->nents,
+			   rsa, sizeof(rsa), fw->rsa_offset);
 
 	for (i = 0; i < UOS_RSA_SCRATCH_COUNT; i++)
 		I915_WRITE(UOS_RSA_SCRATCH(i), rsa[i]);
@@ -201,7 +203,7 @@ static int guc_wait_ucode(struct intel_guc *guc)
  * 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_guc *guc, struct i915_vma *vma)
+static int guc_xfer_ucode(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct intel_uc_fw *guc_fw = &guc->fw;
@@ -214,7 +216,7 @@ static int guc_xfer_ucode(struct intel_guc *guc, struct i915_vma *vma)
 	I915_WRITE(DMA_COPY_SIZE, guc_fw->header_size + guc_fw->ucode_size);
 
 	/* Set the source address for the new blob */
-	offset = intel_guc_ggtt_offset(guc, vma) + guc_fw->header_offset;
+	offset = intel_uc_fw_ggtt_offset(guc_fw) + guc_fw->header_offset;
 	I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset));
 	I915_WRITE(DMA_ADDR_0_HIGH, upper_32_bits(offset) & 0xFFFF);
 
@@ -233,7 +235,7 @@ static int guc_xfer_ucode(struct intel_guc *guc, struct i915_vma *vma)
 /*
  * Load the GuC firmware blob into the MinuteIA.
  */
-static int guc_fw_xfer(struct intel_uc_fw *guc_fw, struct i915_vma *vma)
+static int guc_fw_xfer(struct intel_uc_fw *guc_fw)
 {
 	struct intel_guc *guc = container_of(guc_fw, struct intel_guc, fw);
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -250,9 +252,9 @@ static int guc_fw_xfer(struct intel_uc_fw *guc_fw, struct i915_vma *vma)
 	 * by the DMA engine in one operation, whereas the RSA signature is
 	 * loaded via MMIO.
 	 */
-	guc_xfer_rsa(guc, vma);
+	guc_xfer_rsa(guc);
 
-	ret = guc_xfer_ucode(guc, vma);
+	ret = guc_xfer_ucode(guc);
 
 	intel_uncore_forcewake_put(&dev_priv->uncore, FORCEWAKE_ALL);
 
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 94c04f16a2ad..1ff1fb015e58 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -40,6 +40,61 @@ int intel_huc_init_misc(struct intel_huc *huc)
 	return 0;
 }
 
+static int intel_huc_rsa_data_create(struct intel_huc *huc)
+{
+	struct drm_i915_private *i915 = huc_to_i915(huc);
+	struct intel_guc *guc = &i915->guc;
+	struct i915_vma *vma;
+	void *vaddr;
+
+	/*
+	 * HuC firmware will sit above GUC_GGTT_TOP and will not map
+	 * through GTT. Unfortunately, this means GuC cannot perform
+	 * the HuC auth. as the rsa offset now falls within the GuC
+	 * inaccessible range. We resort to perma-pinning an additional
+	 * vma within the accessible range that only contains the rsa
+	 * signature. The GuC can use this extra pinning to perform
+	 * the authentication since its GGTT offset will be GuC
+	 * accessible.
+	 */
+	vma = intel_guc_allocate_vma(guc, PAGE_SIZE);
+	if (IS_ERR(vma))
+		return PTR_ERR(vma);
+
+	vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
+	if (IS_ERR(vaddr)) {
+		i915_vma_unpin_and_release(&vma, 0);
+		return PTR_ERR(vaddr);
+	}
+
+	huc->rsa_data = vma;
+	huc->rsa_data_vaddr = vaddr;
+
+	return 0;
+}
+
+static void intel_huc_rsa_data_destroy(struct intel_huc *huc)
+{
+	i915_vma_unpin_and_release(&huc->rsa_data, I915_VMA_RELEASE_MAP);
+}
+
+int intel_huc_init(struct intel_huc *huc)
+{
+	int err;
+
+	err = intel_huc_rsa_data_create(huc);
+	if (err)
+		return err;
+
+	return intel_uc_fw_init(&huc->fw);
+}
+
+void intel_huc_fini(struct intel_huc *huc)
+{
+	intel_uc_fw_fini(&huc->fw);
+	intel_huc_rsa_data_destroy(huc);
+}
+
 /**
  * intel_huc_auth() - Authenticate HuC uCode
  * @huc: intel_huc structure
@@ -55,27 +110,17 @@ int intel_huc_auth(struct intel_huc *huc)
 {
 	struct drm_i915_private *i915 = huc_to_i915(huc);
 	struct intel_guc *guc = &i915->guc;
-	struct i915_vma *vma;
 	u32 status;
 	int ret;
 
 	if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return -ENOEXEC;
 
-	vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0,
-				       PIN_OFFSET_BIAS | i915->ggtt.pin_bias);
-	if (IS_ERR(vma)) {
-		ret = PTR_ERR(vma);
-		DRM_ERROR("HuC: Failed to pin huc fw object %d\n", ret);
-		goto fail;
-	}
-
 	ret = intel_guc_auth_huc(guc,
-				 intel_guc_ggtt_offset(guc, vma) +
-				 huc->fw.rsa_offset);
+				 intel_guc_ggtt_offset(guc, huc->rsa_data));
 	if (ret) {
 		DRM_ERROR("HuC: GuC did not ack Auth request %d\n", ret);
-		goto fail_unpin;
+		goto fail;
 	}
 
 	/* Check authentication status, it should be done by now */
@@ -86,14 +131,11 @@ int intel_huc_auth(struct intel_huc *huc)
 					2, 50, &status);
 	if (ret) {
 		DRM_ERROR("HuC: Firmware not verified %#x\n", status);
-		goto fail_unpin;
+		goto fail;
 	}
 
-	i915_vma_unpin(vma);
 	return 0;
 
-fail_unpin:
-	i915_vma_unpin(vma);
 fail:
 	huc->fw.load_status = INTEL_UC_FIRMWARE_FAIL;
 
diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
index ce129e301961..a0c21ae02a99 100644
--- a/drivers/gpu/drm/i915/intel_huc.h
+++ b/drivers/gpu/drm/i915/intel_huc.h
@@ -33,10 +33,14 @@ struct intel_huc {
 	struct intel_uc_fw fw;
 
 	/* HuC-specific additions */
+	struct i915_vma *rsa_data;
+	void *rsa_data_vaddr;
 };
 
 void intel_huc_init_early(struct intel_huc *huc);
 int intel_huc_init_misc(struct intel_huc *huc);
+int intel_huc_init(struct intel_huc *huc);
+void intel_huc_fini(struct intel_huc *huc);
 int intel_huc_auth(struct intel_huc *huc);
 int intel_huc_check_status(struct intel_huc *huc);
 
diff --git a/drivers/gpu/drm/i915/intel_huc_fw.c b/drivers/gpu/drm/i915/intel_huc_fw.c
index 80a176d91edc..44c559526072 100644
--- a/drivers/gpu/drm/i915/intel_huc_fw.c
+++ b/drivers/gpu/drm/i915/intel_huc_fw.c
@@ -93,18 +93,24 @@ void intel_huc_fw_init_early(struct intel_huc *huc)
 	huc_fw_select(huc_fw);
 }
 
-/**
- * huc_fw_xfer() - DMA's the firmware
- * @huc_fw: the firmware descriptor
- * @vma: the firmware image (bound into the GGTT)
- *
- * 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 i915_vma *vma)
+static void huc_xfer_rsa(struct intel_huc *huc)
 {
-	struct intel_huc *huc = container_of(huc_fw, struct intel_huc, fw);
+	struct intel_uc_fw *fw = &huc->fw;
+	struct sg_table *pages = fw->obj->mm.pages;
+
+	/*
+	 * HuC firmware image is outside GuC accessible range.
+	 * Copy the RSA signature out of the image into
+	 * the perma-pinned region set aside for it
+	 */
+	sg_pcopy_to_buffer(pages->sgl, pages->nents,
+			   huc->rsa_data_vaddr, fw->rsa_size,
+			   fw->rsa_offset);
+}
+
+static int huc_xfer_ucode(struct intel_huc *huc)
+{
+	struct intel_uc_fw *huc_fw = &huc->fw;
 	struct drm_i915_private *dev_priv = huc_to_i915(huc);
 	struct intel_uncore *uncore = &dev_priv->uncore;
 	unsigned long offset = 0;
@@ -116,7 +122,7 @@ static int huc_fw_xfer(struct intel_uc_fw *huc_fw, struct i915_vma *vma)
 	intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
 
 	/* Set the source address for the uCode */
-	offset = intel_guc_ggtt_offset(&dev_priv->guc, vma) +
+	offset = intel_uc_fw_ggtt_offset(huc_fw) +
 		 huc_fw->header_offset;
 	intel_uncore_write(uncore, DMA_ADDR_0_LOW,
 			   lower_32_bits(offset));
@@ -150,6 +156,23 @@ static int huc_fw_xfer(struct intel_uc_fw *huc_fw, struct i915_vma *vma)
 	return ret;
 }
 
+/**
+ * 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_huc *huc = container_of(huc_fw, struct intel_huc, fw);
+
+	huc_xfer_rsa(huc);
+
+	return huc_xfer_ucode(huc);
+}
+
 /**
  * intel_huc_fw_upload() - load HuC uCode to device
  * @huc: intel_huc structure
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 25b80ffe71ad..ff4452ded41e 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -280,6 +280,7 @@ void intel_uc_fini_misc(struct drm_i915_private *i915)
 int intel_uc_init(struct drm_i915_private *i915)
 {
 	struct intel_guc *guc = &i915->guc;
+	struct intel_huc *huc = &i915->huc;
 	int ret;
 
 	if (!USES_GUC(i915))
@@ -292,19 +293,30 @@ int intel_uc_init(struct drm_i915_private *i915)
 	if (ret)
 		return ret;
 
+	if (USES_HUC(i915)) {
+		ret = intel_huc_init(huc);
+		if (ret)
+			goto err_guc;
+	}
+
 	if (USES_GUC_SUBMISSION(i915)) {
 		/*
 		 * This is stuff we need to have available at fw load time
 		 * if we are planning to enable submission later
 		 */
 		ret = intel_guc_submission_init(guc);
-		if (ret) {
-			intel_guc_fini(guc);
-			return ret;
-		}
+		if (ret)
+			goto err_huc;
 	}
 
 	return 0;
+
+err_huc:
+	if (USES_HUC(i915))
+		intel_huc_fini(huc);
+err_guc:
+	intel_guc_fini(guc);
+	return ret;
 }
 
 void intel_uc_fini(struct drm_i915_private *i915)
@@ -319,6 +331,9 @@ void intel_uc_fini(struct drm_i915_private *i915)
 	if (USES_GUC_SUBMISSION(i915))
 		intel_guc_submission_fini(guc);
 
+	if (USES_HUC(i915))
+		intel_huc_fini(&i915->huc);
+
 	intel_guc_fini(guc);
 }
 
@@ -488,6 +503,22 @@ int intel_uc_suspend(struct drm_i915_private *i915)
 	return 0;
 }
 
+void intel_uc_restore_ggtt_mapping(struct drm_i915_private *i915)
+{
+	struct intel_guc *guc = &i915->guc;
+
+	if (!USES_GUC(i915))
+		return;
+
+	intel_uc_fw_ggtt_bind(&guc->fw);
+
+	if (USES_HUC(i915)) {
+		struct intel_huc *huc = &i915->huc;
+
+		intel_uc_fw_ggtt_bind(&huc->fw);
+	}
+}
+
 int intel_uc_resume(struct drm_i915_private *i915)
 {
 	struct intel_guc *guc = &i915->guc;
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index c14729786652..e7e2e871700e 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -40,6 +40,7 @@ int intel_uc_init(struct drm_i915_private *dev_priv);
 void intel_uc_fini(struct drm_i915_private *dev_priv);
 void intel_uc_reset_prepare(struct drm_i915_private *i915);
 int intel_uc_suspend(struct drm_i915_private *dev_priv);
+void intel_uc_restore_ggtt_mapping(struct drm_i915_private *i915);
 int intel_uc_resume(struct drm_i915_private *dev_priv);
 
 static inline bool intel_uc_is_using_guc(struct drm_i915_private *i915)
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
index e3e74207a102..02a455ca5985 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/intel_uc_fw.c
@@ -201,11 +201,8 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
  * Return: 0 on success, non-zero on failure.
  */
 int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
-		       int (*xfer)(struct intel_uc_fw *uc_fw,
-				   struct i915_vma *vma))
+		       int (*xfer)(struct intel_uc_fw *uc_fw))
 {
-	struct i915_vma *vma;
-	u32 ggtt_pin_bias;
 	int err;
 
 	DRM_DEBUG_DRIVER("%s fw load %s\n",
@@ -219,33 +216,8 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
 			 intel_uc_fw_type_repr(uc_fw->type),
 			 intel_uc_fw_status_repr(uc_fw->load_status));
 
-	/* Pin object with firmware */
-	err = i915_gem_object_set_to_gtt_domain(uc_fw->obj, false);
-	if (err) {
-		DRM_DEBUG_DRIVER("%s fw set-domain err=%d\n",
-				 intel_uc_fw_type_repr(uc_fw->type), err);
-		goto fail;
-	}
-
-	ggtt_pin_bias = to_i915(uc_fw->obj->base.dev)->ggtt.pin_bias;
-	vma = i915_gem_object_ggtt_pin(uc_fw->obj, NULL, 0, 0,
-				       PIN_OFFSET_BIAS | ggtt_pin_bias);
-	if (IS_ERR(vma)) {
-		err = PTR_ERR(vma);
-		DRM_DEBUG_DRIVER("%s fw ggtt-pin err=%d\n",
-				 intel_uc_fw_type_repr(uc_fw->type), err);
-		goto fail;
-	}
-
 	/* Call custom loader */
-	err = xfer(uc_fw, vma);
-
-	/*
-	 * We keep the object pages for reuse during resume. But we can unpin it
-	 * now that DMA has completed, so it doesn't continue to take up space.
-	 */
-	i915_vma_unpin(vma);
-
+	err = xfer(uc_fw);
 	if (err)
 		goto fail;
 
@@ -273,6 +245,92 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
 	return err;
 }
 
+int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
+{
+	int err;
+
+	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
+		return -ENOEXEC;
+
+	err = i915_gem_object_pin_pages(uc_fw->obj);
+	if (err) {
+		DRM_DEBUG_DRIVER("%s fw pin-pages err=%d\n",
+				 intel_uc_fw_type_repr(uc_fw->type), err);
+		return err;
+	}
+
+	intel_uc_fw_ggtt_bind(uc_fw);
+	return 0;
+}
+
+void intel_uc_fw_fini(struct intel_uc_fw *uc_fw)
+{
+	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
+		return;
+
+	intel_uc_fw_ggtt_unbind(uc_fw);
+	i915_gem_object_unpin_pages(uc_fw->obj);
+}
+
+u32 intel_uc_fw_ggtt_offset(struct intel_uc_fw *uc_fw)
+{
+	struct drm_i915_private *i915 = to_i915(uc_fw->obj->base.dev);
+	struct i915_ggtt *ggtt = &i915->ggtt;
+	struct drm_mm_node *node = &ggtt->uc_fw;
+	u64 offset;
+
+	GEM_BUG_ON(!node->allocated);
+	GEM_BUG_ON(upper_32_bits(node->start));
+	GEM_BUG_ON(upper_32_bits(node->start + node->size - 1));
+
+	offset = node->start;
+
+	if (uc_fw->type == INTEL_UC_FW_TYPE_HUC) {
+		/*
+		 * NOTE: GuC and HuC fw will share the mm allocation range,
+		 * with GuC filling [range_start, range_start + guc_fw_size).
+		 * HuC fw is placed at the next page-aligned slot.
+		 */
+		offset += round_up(i915->guc.fw.size, PAGE_SIZE);
+		GEM_BUG_ON(upper_32_bits(offset));
+	}
+
+	return lower_32_bits(offset);
+}
+
+void intel_uc_fw_ggtt_bind(struct intel_uc_fw *uc_fw)
+{
+	struct drm_i915_gem_object *obj = uc_fw->obj;
+	struct i915_ggtt *ggtt;
+	struct i915_vma dummy;
+
+	GEM_BUG_ON(!obj);
+
+	ggtt = &to_i915(obj->base.dev)->ggtt;
+
+	dummy.node.start = intel_uc_fw_ggtt_offset(uc_fw);
+	dummy.node.size = obj->base.size;
+	dummy.pages = obj->mm.pages;
+	dummy.vm = &ggtt->vm;
+
+	GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj));
+	ggtt->vm.insert_entries(&ggtt->vm, &dummy, obj->cache_level, 0);
+}
+
+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;
+	u64 start;
+
+	GEM_BUG_ON(!obj);
+
+	ggtt = &to_i915(obj->base.dev)->ggtt;
+	start = intel_uc_fw_ggtt_offset(uc_fw);
+
+	ggtt->vm.clear_range(&ggtt->vm, start, obj->base.size);
+}
+
 /**
  * intel_uc_fw_cleanup_fetch - cleanup uC firmware
  *
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h
index e6fa8599757c..8678c33ec8cd 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/intel_uc_fw.h
@@ -27,7 +27,6 @@
 
 struct drm_printer;
 struct drm_i915_private;
-struct i915_vma;
 
 /* 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"
@@ -147,8 +146,12 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 		       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,
-		       int (*xfer)(struct intel_uc_fw *uc_fw,
-				   struct i915_vma *vma));
+		       int (*xfer)(struct intel_uc_fw *uc_fw));
+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);
+void intel_uc_fw_ggtt_bind(struct intel_uc_fw *uc_fw);
+void intel_uc_fw_ggtt_unbind(struct intel_uc_fw *uc_fw);
 void intel_uc_fw_dump(const struct intel_uc_fw *uc_fw, struct drm_printer *p);
 
 #endif
-- 
2.21.0

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

[PATCH v2 4/5] Revert "drm/i915/guc: Disable global reset"

[Fernando Pacheco]

This reverts commit fe62365f9f80a1c1d438c54fba21f5108a182de8.

Signed-off-by: Fernando Pacheco <fernando.pacheco@intel.com>
---
 drivers/gpu/drm/i915/i915_reset.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
index 677d59304e78..1092d16c289c 100644
--- a/drivers/gpu/drm/i915/i915_reset.c
+++ b/drivers/gpu/drm/i915/i915_reset.c
@@ -641,9 +641,6 @@ int intel_gpu_reset(struct drm_i915_private *i915,
 
 bool intel_has_gpu_reset(struct drm_i915_private *i915)
 {
-	if (USES_GUC(i915))
-		return false;
-
 	if (!i915_modparams.reset)
 		return NULL;
 
-- 
2.21.0

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

[PATCH v2 5/5] drm/i915/selftests: Check that gpu reset is usable from atomic context

[Fernando Pacheco]

GPU reset is now available with GuC enabled,
so re-enable our check that this reset is usable
from atomic context.

Signed-off-by: Fernando Pacheco <fernando.pacheco@intel.com>
---
 drivers/gpu/drm/i915/selftests/intel_hangcheck.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
index 050bd1e19e02..2fd33aad8683 100644
--- a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
+++ b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
@@ -1814,9 +1814,6 @@ static int igt_atomic_reset(void *arg)
 
 	/* Check that the resets are usable from atomic context */
 
-	if (USES_GUC_SUBMISSION(i915))
-		return 0; /* guc is dead; long live the guc */
-
 	igt_global_reset_lock(i915);
 	mutex_lock(&i915->drm.struct_mutex);
 	wakeref = intel_runtime_pm_get(i915);
@@ -1846,6 +1843,9 @@ static int igt_atomic_reset(void *arg)
 		force_reset(i915);
 	}
 
+	if (USES_GUC_SUBMISSION(i915))
+		goto unlock;
+
 	if (intel_has_reset_engine(i915)) {
 		struct intel_engine_cs *engine;
 		enum intel_engine_id id;
-- 
2.21.0

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

Re: [PATCH v2 0/5] Perma-pin uC firmware and re-enable global reset

[Fernando Pacheco]

On 4/18/19 4:31 PM, Fernando Pacheco wrote:
> The intent is to move the GuC and HuC firmware images to the
> top of the address space. This portion is inaccessible during
> normal GuC operations and should be relatively safe to house
> both firmware images. By making the move we can re-enable the
> full gpu reset with GuC enabled.
>
> Placing the firmware images above GUC_GGTT_TOP was discussed
> previously here:
> 	https://patchwork.freedesktop.org/patch/273616/
>
> v2:
> The decision to rename both the uc_fw init and fini functions
> made it easier to pull the bind/unbind operations out of
> intel_guc_fw.* and intel_huc_fw.*. The bind/unbind will now
> take place within the newly repurposed intel_uc_fw_init/fini.
> All other changes should be called out in their respective patches
> and should be the direct result of a review comment.

Chris, I didn't address two of your earlier review comments, so I'll try
to do so here:

1. You are correct about the inline function in the first patch, but I felt that
     could be fixed separate from this series.
2. Your comment on not needing to pin the fw pages made sense to me, but I found that the
    explicit pin/unpin was necessary for the binding. Please let me know if I'm missing
    something here!

Thanks,
Fernando

>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>
> Fernando Pacheco (5):
>   drm/i915/uc: Rename uC firmware init/fini functions
>   drm/i915/uc: Reserve upper range of GGTT
>   drm/i915/uc: Place uC firmware in upper range of GGTT
>   Revert "drm/i915/guc: Disable global reset"
>   drm/i915/selftests: Check that gpu reset is usable from atomic context
>
>  drivers/gpu/drm/i915/i915_gem.c               |   2 +
>  drivers/gpu/drm/i915/i915_gem_gtt.c           |  25 ++--
>  drivers/gpu/drm/i915/i915_gem_gtt.h           |   1 +
>  drivers/gpu/drm/i915/i915_reset.c             |   3 -
>  drivers/gpu/drm/i915/intel_guc.c              |  58 ++++++++-
>  drivers/gpu/drm/i915/intel_guc.h              |   2 +
>  drivers/gpu/drm/i915/intel_guc_fw.c           |  20 +--
>  drivers/gpu/drm/i915/intel_huc.c              |  74 ++++++++---
>  drivers/gpu/drm/i915/intel_huc.h              |   6 +-
>  drivers/gpu/drm/i915/intel_huc_fw.c           |  49 +++++--
>  drivers/gpu/drm/i915/intel_uc.c               |  39 +++++-
>  drivers/gpu/drm/i915/intel_uc.h               |   1 +
>  drivers/gpu/drm/i915/intel_uc_fw.c            | 122 +++++++++++++-----
>  drivers/gpu/drm/i915/intel_uc_fw.h            |  12 +-
>  .../gpu/drm/i915/selftests/intel_hangcheck.c  |   6 +-
>  15 files changed, 319 insertions(+), 101 deletions(-)
>

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

✗ Fi.CI.SPARSE: warning for Perma-pin uC firmware and re-enable global reset (rev2)

[Patchwork]

== Series Details ==

Series: Perma-pin uC firmware and re-enable global reset (rev2)
URL   : https://patchwork.freedesktop.org/series/59255/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915/uc: Rename uC firmware init/fini functions
Okay!

Commit: drm/i915/uc: Reserve upper range of GGTT
-O:drivers/gpu/drm/i915/i915_gem_gtt.c:3378:34: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/i915_gem_gtt.c:3380:25: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/i915_gem_gtt.c:3380:25: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_guc.c:738:16: warning: expression using sizeof(void)

Commit: drm/i915/uc: Place uC firmware in upper range of GGTT
Okay!

Commit: Revert "drm/i915/guc: Disable global reset"
Okay!

Commit: drm/i915/selftests: Check that gpu reset is usable from atomic context
Okay!

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

✓ Fi.CI.BAT: success for Perma-pin uC firmware and re-enable global reset (rev2)

[Patchwork]

== Series Details ==

Series: Perma-pin uC firmware and re-enable global reset (rev2)
URL   : https://patchwork.freedesktop.org/series/59255/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5954 -> Patchwork_12838
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/59255/revisions/2/mbox/

Known issues
------------

  Here are the changes found in Patchwork_12838 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_cs_nop@fork-gfx0:
    - fi-icl-u2:          NOTRUN -> SKIP [fdo#109315] +17

  * igt@gem_exec_basic@readonly-bsd1:
    - fi-icl-u2:          NOTRUN -> SKIP [fdo#109276] +3

  * igt@gem_exec_parse@basic-allowed:
    - fi-icl-u2:          NOTRUN -> SKIP [fdo#109289] +1

  * igt@i915_module_load@reload:
    - fi-blb-e6850:       PASS -> INCOMPLETE [fdo#107718]

  * igt@i915_selftest@live_execlists:
    - fi-apl-guc:         PASS -> INCOMPLETE [fdo#103927] / [fdo#109720]

  * igt@kms_chamelium@dp-edid-read:
    - fi-icl-u2:          NOTRUN -> SKIP [fdo#109316] +2

  * igt@kms_chamelium@vga-hpd-fast:
    - fi-icl-u2:          NOTRUN -> SKIP [fdo#109309] +1

  * igt@kms_force_connector_basic@force-edid:
    - fi-glk-dsi:         NOTRUN -> SKIP [fdo#109271] +26

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-bxt-j4205:       NOTRUN -> SKIP [fdo#109271] +47

  * igt@kms_force_connector_basic@prune-stale-modes:
    - fi-icl-u2:          NOTRUN -> SKIP [fdo#109285] +3

  * igt@kms_frontbuffer_tracking@basic:
    - fi-glk-dsi:         NOTRUN -> FAIL [fdo#103167]

  * igt@runner@aborted:
    - fi-apl-guc:         NOTRUN -> FAIL [fdo#108622] / [fdo#109720]

  
#### Possible fixes ####

  * igt@gem_exec_basic@readonly-bsd:
    - fi-icl-u2:          INCOMPLETE [fdo#107713] -> PASS

  * igt@i915_hangman@error-state-basic:
    - fi-kbl-guc:         SKIP [fdo#109271] -> PASS
    - fi-apl-guc:         SKIP [fdo#109271] -> PASS +1
    - fi-cfl-guc:         SKIP [fdo#109271] -> PASS +1

  * igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size:
    - fi-glk-dsi:         INCOMPLETE [fdo#103359] / [k.org#198133] -> PASS

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-a:
    - fi-skl-guc:         SKIP [fdo#109271] -> PASS +1

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
    - fi-byt-clapper:     FAIL [fdo#103191] -> PASS

  
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109309]: https://bugs.freedesktop.org/show_bug.cgi?id=109309
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109316]: https://bugs.freedesktop.org/show_bug.cgi?id=109316
  [fdo#109720]: https://bugs.freedesktop.org/show_bug.cgi?id=109720
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (48 -> 41)
------------------------------

  Additional (1): fi-bxt-j4205 
  Missing    (8): fi-ilk-m540 fi-bsw-cyan fi-ctg-p8600 fi-gdg-551 fi-pnv-d510 fi-icl-y fi-bsw-kefka fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_5954 -> Patchwork_12838

  CI_DRM_5954: a77e0dc060fcd1a2a09412067097685c5101589c @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4957: a765aa108105804c19096554447ad0cb71f64fc3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12838: d6ac18531d37086d90ca6f5e66810fdc81b7983a @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

d6ac18531d37 drm/i915/selftests: Check that gpu reset is usable from atomic context
c564598dff9e Revert "drm/i915/guc: Disable global reset"
274d7b6582e5 drm/i915/uc: Place uC firmware in upper range of GGTT
895570b36948 drm/i915/uc: Reserve upper range of GGTT
298a5ae553fb drm/i915/uc: Rename uC firmware init/fini functions

== Logs ==

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

✓ Fi.CI.IGT: success for Perma-pin uC firmware and re-enable global reset (rev2)

[Patchwork]

== Series Details ==

Series: Perma-pin uC firmware and re-enable global reset (rev2)
URL   : https://patchwork.freedesktop.org/series/59255/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5954_full -> Patchwork_12838_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_12838_full:

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * {igt@audio@hdmi-integrity}:
    - shard-skl:          NOTRUN -> FAIL
    - shard-apl:          NOTRUN -> FAIL

  * {igt@audio@hdmi-integrity-after-suspend}:
    - shard-glk:          TIMEOUT -> FAIL

  
Known issues
------------

  Here are the changes found in Patchwork_12838_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_pm_rpm@gem-execbuf-stress:
    - shard-skl:          PASS -> INCOMPLETE [fdo#107803] / [fdo#107807]

  * igt@i915_pm_rpm@i2c:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#109982]

  * igt@kms_atomic_transition@5x-modeset-transitions:
    - shard-skl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +10

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-e:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +2

  * igt@kms_chamelium@dp-crc-fast:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] +28

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-iclb:         PASS -> SKIP [fdo#109349]

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-skl:          PASS -> FAIL [fdo#105363]

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-apl:          PASS -> DMESG-WARN [fdo#108566] +3

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-render:
    - shard-iclb:         PASS -> FAIL [fdo#103167] +3

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-render:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] +120

  * igt@kms_lease@atomic_implicit_crtc:
    - shard-snb:          NOTRUN -> FAIL [fdo#110279]

  * igt@kms_lease@setcrtc_implicit_plane:
    - shard-snb:          NOTRUN -> FAIL [fdo#110281]

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-e:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +10

  * igt@kms_plane@pixel-format-pipe-b-planes-source-clamping:
    - shard-glk:          PASS -> SKIP [fdo#109271]

  * igt@kms_plane_alpha_blend@pipe-a-alpha-7efc:
    - shard-skl:          NOTRUN -> FAIL [fdo#108145] +3

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-skl:          NOTRUN -> FAIL [fdo#108145] / [fdo#110403]

  * igt@kms_plane_scaling@pipe-b-scaler-with-pixel-format:
    - shard-glk:          PASS -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_psr@psr2_cursor_mmap_cpu:
    - shard-iclb:         PASS -> SKIP [fdo#109441] +2

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-kbl:          PASS -> FAIL [fdo#109016]

  * igt@kms_setmode@basic:
    - shard-apl:          PASS -> FAIL [fdo#99912]

  * igt@kms_vrr@flip-suspend:
    - shard-skl:          NOTRUN -> SKIP [fdo#109271] +123

  
#### Possible fixes ####

  * igt@i915_suspend@fence-restore-untiled:
    - shard-apl:          DMESG-WARN [fdo#108566] -> PASS +5

  * igt@kms_cursor_crc@cursor-256x256-suspend:
    - shard-skl:          INCOMPLETE [fdo#104108] -> PASS

  * igt@kms_cursor_crc@cursor-64x64-suspend:
    - shard-kbl:          DMESG-WARN [fdo#108566] -> PASS

  * igt@kms_flip@plain-flip-fb-recreate:
    - shard-skl:          FAIL [fdo#100368] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-move:
    - shard-iclb:         FAIL [fdo#103167] -> PASS +8

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-skl:          FAIL [fdo#108145] -> PASS

  * igt@kms_psr2_su@page_flip:
    - shard-iclb:         SKIP [fdo#109642] -> PASS

  * igt@kms_psr@psr2_sprite_render:
    - shard-iclb:         SKIP [fdo#109441] -> PASS +2

  * igt@kms_rotation_crc@multiplane-rotation-cropping-bottom:
    - shard-kbl:          DMESG-FAIL [fdo#105763] -> PASS

  * igt@kms_sysfs_edid_timing:
    - shard-iclb:         FAIL [fdo#100047] -> PASS

  
#### Warnings ####

  * igt@i915_pm_rpm@modeset-pc8-residency-stress:
    - shard-skl:          SKIP [fdo#109271] -> INCOMPLETE [fdo#107807]

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
  [fdo#100368]: https://bugs.freedesktop.org/show_bug.cgi?id=100368
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
  [fdo#107803]: https://bugs.freedesktop.org/show_bug.cgi?id=107803
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109016]: https://bugs.freedesktop.org/show_bug.cgi?id=109016
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#109982]: https://bugs.freedesktop.org/show_bug.cgi?id=109982
  [fdo#110279]: https://bugs.freedesktop.org/show_bug.cgi?id=110279
  [fdo#110281]: https://bugs.freedesktop.org/show_bug.cgi?id=110281
  [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 9)
------------------------------

  Missing    (1): shard-hsw 


Build changes
-------------

  * Linux: CI_DRM_5954 -> Patchwork_12838

  CI_DRM_5954: a77e0dc060fcd1a2a09412067097685c5101589c @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4957: a765aa108105804c19096554447ad0cb71f64fc3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12838: d6ac18531d37086d90ca6f5e66810fdc81b7983a @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [PATCH v2 2/5] drm/i915/uc: Reserve upper range of GGTT

[Chris Wilson]

Quoting Fernando Pacheco (2019-04-19 00:31:48)
> GuC and HuC depend on struct_mutex for device
> reinitialization. Moving away from this dependency
> requires perma-pinning the firmware images in GGTT.
> The upper portion of the GuC address space has
> a sizeable hole (several MB) that is inaccessible
> by GuC. Reserve this range within GGTT as it can
> comfortably hold GuC/HuC firmware images.
> 
> v2: Reserve node rather than insert (Chris)
>     Simpler determination of node start/size (Daniele)
>     Move reserve/release out to intel_guc.* files
> 
> Signed-off-by: Fernando Pacheco <fernando.pacheco@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 25 ++++++++--------
>  drivers/gpu/drm/i915/i915_gem_gtt.h |  1 +
>  drivers/gpu/drm/i915/intel_guc.c    | 45 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_guc.h    |  2 ++
>  4 files changed, 60 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 8f460cc4cc1f..0b4c22e68574 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2752,6 +2752,12 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
>         if (ret)
>                 return ret;
>  
> +       if (USES_GUC(dev_priv)) {
> +               ret = intel_guc_reserve_ggtt_top(&dev_priv->guc);
> +               if (ret)
> +                       goto err_reserve;
> +       }
> +
>         /* Clear any non-preallocated blocks */
>         drm_mm_for_each_hole(entry, &ggtt->vm.mm, hole_start, hole_end) {
>                 DRM_DEBUG_KMS("clearing unused GTT space: [%lx, %lx]\n",
> @@ -2766,12 +2772,14 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
>         if (INTEL_PPGTT(dev_priv) == INTEL_PPGTT_ALIASING) {
>                 ret = i915_gem_init_aliasing_ppgtt(dev_priv);
>                 if (ret)
> -                       goto err;
> +                       goto err_appgtt;
>         }
>  
>         return 0;
>  
> -err:
> +err_appgtt:
> +       intel_guc_release_ggtt_top(&dev_priv->guc);
> +err_reserve:
>         drm_mm_remove_node(&ggtt->error_capture);
>         return ret;
>  }
> @@ -2797,6 +2805,8 @@ void i915_ggtt_cleanup_hw(struct drm_i915_private *dev_priv)
>         if (drm_mm_node_allocated(&ggtt->error_capture))
>                 drm_mm_remove_node(&ggtt->error_capture);
>  
> +       intel_guc_release_ggtt_top(&dev_priv->guc);
> +
>         if (drm_mm_initialized(&ggtt->vm.mm)) {
>                 intel_vgt_deballoon(dev_priv);
>                 i915_address_space_fini(&ggtt->vm);
> @@ -3369,17 +3379,6 @@ int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv)
>         if (ret)
>                 return ret;
>  
> -       /* Trim the GGTT to fit the GuC mappable upper range (when enabled).
> -        * This is easier than doing range restriction on the fly, as we
> -        * currently don't have any bits spare to pass in this upper
> -        * restriction!
> -        */
> -       if (USES_GUC(dev_priv)) {
> -               ggtt->vm.total = min_t(u64, ggtt->vm.total, GUC_GGTT_TOP);
> -               ggtt->mappable_end =
> -                       min_t(u64, ggtt->mappable_end, ggtt->vm.total);
> -       }
> -
>         if ((ggtt->vm.total - 1) >> 32) {
>                 DRM_ERROR("We never expected a Global GTT with more than 32bits"
>                           " of address space! Found %lldM!\n",
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index f597f35b109b..b51e779732c3 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -384,6 +384,7 @@ struct i915_ggtt {
>         u32 pin_bias;
>  
>         struct drm_mm_node error_capture;
> +       struct drm_mm_node uc_fw;
>  };
>  
>  struct i915_hw_ppgtt {
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index d81a02b0f525..ddd246dc3f14 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -721,3 +721,48 @@ u32 intel_guc_reserved_gtt_size(struct intel_guc *guc)
>  {
>         return guc_to_i915(guc)->wopcm.guc.size;
>  }
> +
> +static u32 __intel_guc_ggtt_top_offset(struct intel_guc *guc)
> +{
> +       struct drm_i915_private *i915 = guc_to_i915(guc);
> +       struct i915_ggtt *ggtt = &i915->ggtt;
> +       struct intel_huc *huc = &i915->huc;
> +       u32 guc_fw_size, huc_fw_size;
> +       u32 min_reserved_size;
> +
> +       guc_fw_size = round_up(guc->fw.size, I915_GTT_PAGE_SIZE);
> +       huc_fw_size = round_up(huc->fw.size, I915_GTT_PAGE_SIZE);
> +
> +       min_reserved_size = guc_fw_size + huc_fw_size;

In this patch, you should only be using GUC_GGTT_TOP so that the only
change is from excluding the zone to using a reserved node.


So why reserve room for both fw? You should only be binding them for the
xfer (so that you do not have to insert extraneous code outside of
reset).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/5] drm/i915/uc: Place uC firmware in upper range of GGTT

[Chris Wilson]

Quoting Fernando Pacheco (2019-04-19 00:31:49)
> Currently we pin the GuC or HuC firmware image just
> before uploading. Perma-pin during uC initialization
> instead and use the range reserved at the top of the
> address space.
> 
> Moving the firmware resulted in needing to:
> - restore the ggtt mapping during the suspend/resume path.
> - use an additional pinning for the rsa signature which will
>   be used during HuC auth as addresses above GUC_GGTT_TOP
>   do not map through GTT.
> 
> v2: Remove call to set to gtt domain
>     Do not restore fw gtt mapping unconditionally
>     Separate out pin/unpin functions and drop usage of pin/unpin
>     Use uc_fw init/fini functions to bind/unbind fw object
> 
> Signed-off-by: Fernando Pacheco <fernando.pacheco@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c     |   2 +
>  drivers/gpu/drm/i915/intel_guc.c    |   9 ++-
>  drivers/gpu/drm/i915/intel_guc_fw.c |  18 +++--
>  drivers/gpu/drm/i915/intel_huc.c    |  74 +++++++++++++----
>  drivers/gpu/drm/i915/intel_huc.h    |   4 +
>  drivers/gpu/drm/i915/intel_huc_fw.c |  47 ++++++++---
>  drivers/gpu/drm/i915/intel_uc.c     |  39 ++++++++-
>  drivers/gpu/drm/i915/intel_uc.h     |   1 +
>  drivers/gpu/drm/i915/intel_uc_fw.c  | 118 +++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_uc_fw.h  |   9 ++-
>  10 files changed, 247 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e5462639de0b..0e06a2c7d65f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4508,6 +4508,8 @@ void i915_gem_resume(struct drm_i915_private *i915)
>         i915_gem_restore_gtt_mappings(i915);
>         i915_gem_restore_fences(i915);
>  
> +       intel_uc_restore_ggtt_mapping(i915);

Nope. They do not need to be restored as the bindings are only required
for the xfer.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/5] drm/i915/uc: Reserve upper range of GGTT

[Fernando Pacheco]

On 4/19/19 12:14 AM, Chris Wilson wrote:
> Quoting Fernando Pacheco (2019-04-19 00:31:48)
>> GuC and HuC depend on struct_mutex for device
>> reinitialization. Moving away from this dependency
>> requires perma-pinning the firmware images in GGTT.
>> The upper portion of the GuC address space has
>> a sizeable hole (several MB) that is inaccessible
>> by GuC. Reserve this range within GGTT as it can
>> comfortably hold GuC/HuC firmware images.
>>
>> v2: Reserve node rather than insert (Chris)
>>     Simpler determination of node start/size (Daniele)
>>     Move reserve/release out to intel_guc.* files
>>
>> Signed-off-by: Fernando Pacheco <fernando.pacheco@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_gem_gtt.c | 25 ++++++++--------
>>  drivers/gpu/drm/i915/i915_gem_gtt.h |  1 +
>>  drivers/gpu/drm/i915/intel_guc.c    | 45 +++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_guc.h    |  2 ++
>>  4 files changed, 60 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 8f460cc4cc1f..0b4c22e68574 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -2752,6 +2752,12 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
>>         if (ret)
>>                 return ret;
>>  
>> +       if (USES_GUC(dev_priv)) {
>> +               ret = intel_guc_reserve_ggtt_top(&dev_priv->guc);
>> +               if (ret)
>> +                       goto err_reserve;
>> +       }
>> +
>>         /* Clear any non-preallocated blocks */
>>         drm_mm_for_each_hole(entry, &ggtt->vm.mm, hole_start, hole_end) {
>>                 DRM_DEBUG_KMS("clearing unused GTT space: [%lx, %lx]\n",
>> @@ -2766,12 +2772,14 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
>>         if (INTEL_PPGTT(dev_priv) == INTEL_PPGTT_ALIASING) {
>>                 ret = i915_gem_init_aliasing_ppgtt(dev_priv);
>>                 if (ret)
>> -                       goto err;
>> +                       goto err_appgtt;
>>         }
>>  
>>         return 0;
>>  
>> -err:
>> +err_appgtt:
>> +       intel_guc_release_ggtt_top(&dev_priv->guc);
>> +err_reserve:
>>         drm_mm_remove_node(&ggtt->error_capture);
>>         return ret;
>>  }
>> @@ -2797,6 +2805,8 @@ void i915_ggtt_cleanup_hw(struct drm_i915_private *dev_priv)
>>         if (drm_mm_node_allocated(&ggtt->error_capture))
>>                 drm_mm_remove_node(&ggtt->error_capture);
>>  
>> +       intel_guc_release_ggtt_top(&dev_priv->guc);
>> +
>>         if (drm_mm_initialized(&ggtt->vm.mm)) {
>>                 intel_vgt_deballoon(dev_priv);
>>                 i915_address_space_fini(&ggtt->vm);
>> @@ -3369,17 +3379,6 @@ int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv)
>>         if (ret)
>>                 return ret;
>>  
>> -       /* Trim the GGTT to fit the GuC mappable upper range (when enabled).
>> -        * This is easier than doing range restriction on the fly, as we
>> -        * currently don't have any bits spare to pass in this upper
>> -        * restriction!
>> -        */
>> -       if (USES_GUC(dev_priv)) {
>> -               ggtt->vm.total = min_t(u64, ggtt->vm.total, GUC_GGTT_TOP);
>> -               ggtt->mappable_end =
>> -                       min_t(u64, ggtt->mappable_end, ggtt->vm.total);
>> -       }
>> -
>>         if ((ggtt->vm.total - 1) >> 32) {
>>                 DRM_ERROR("We never expected a Global GTT with more than 32bits"
>>                           " of address space! Found %lldM!\n",
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
>> index f597f35b109b..b51e779732c3 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
>> @@ -384,6 +384,7 @@ struct i915_ggtt {
>>         u32 pin_bias;
>>  
>>         struct drm_mm_node error_capture;
>> +       struct drm_mm_node uc_fw;
>>  };
>>  
>>  struct i915_hw_ppgtt {
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
>> index d81a02b0f525..ddd246dc3f14 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -721,3 +721,48 @@ u32 intel_guc_reserved_gtt_size(struct intel_guc *guc)
>>  {
>>         return guc_to_i915(guc)->wopcm.guc.size;
>>  }
>> +
>> +static u32 __intel_guc_ggtt_top_offset(struct intel_guc *guc)
>> +{
>> +       struct drm_i915_private *i915 = guc_to_i915(guc);
>> +       struct i915_ggtt *ggtt = &i915->ggtt;
>> +       struct intel_huc *huc = &i915->huc;
>> +       u32 guc_fw_size, huc_fw_size;
>> +       u32 min_reserved_size;
>> +
>> +       guc_fw_size = round_up(guc->fw.size, I915_GTT_PAGE_SIZE);
>> +       huc_fw_size = round_up(huc->fw.size, I915_GTT_PAGE_SIZE);
>> +
>> +       min_reserved_size = guc_fw_size + huc_fw_size;
> In this patch, you should only be using GUC_GGTT_TOP so that the only
> change is from excluding the zone to using a reserved node.
>
>
> So why reserve room for both fw? You should only be binding them for the
> xfer (so that you do not have to insert extraneous code outside of
> reset).

Sorry. It took a while to understand the larger point you were making
in the first version of these patches. You're right. If I only bind them for the
transfer then there is no need to reserve room for both.

It is unlikely, but should there be a check in case GUC_GGTT_TOP exceeds vm.total and
adjust the start accordingly? Or should we let this bail? GEM_BUG_ON?

Thanks,
Fernando

> -Chris

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

Re: [PATCH v2 2/5] drm/i915/uc: Reserve upper range of GGTT

[Chris Wilson]

Quoting Fernando Pacheco (2019-04-19 18:14:21)
> 
> On 4/19/19 12:14 AM, Chris Wilson wrote:
> > Quoting Fernando Pacheco (2019-04-19 00:31:48)
> >> -       /* Trim the GGTT to fit the GuC mappable upper range (when enabled).
> >> -        * This is easier than doing range restriction on the fly, as we
> >> -        * currently don't have any bits spare to pass in this upper
> >> -        * restriction!
> >> -        */
> >> -       if (USES_GUC(dev_priv)) {
> >> -               ggtt->vm.total = min_t(u64, ggtt->vm.total, GUC_GGTT_TOP);
> >> -               ggtt->mappable_end =
> >> -                       min_t(u64, ggtt->mappable_end, ggtt->vm.total);
> >> -       }

[snip

> It is unlikely, but should there be a check in case GUC_GGTT_TOP exceeds vm.total and
> adjust the start accordingly? Or should we let this bail? GEM_BUG_ON?

If GUC_GGTT_TOP is outside the range of vm.total, that should give us a
bogus address for the drm_mm_reserve_node, and that should fail. That
seems reasonable, if we can't support the guc with the current GGTT
setup we probably do want to bail and get it fixed (as something would
be very wrong with the GGTT probe, one presumes).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx