[PATCH v4 1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes

[PATCH v4 1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes

[Jackie Li]

After enabled the WOPCM write-once registers locking status checking,
reloading of the i915 module will fail with modparam enable_guc set to 3
(enable GuC and HuC firmware loading) if the module was originally loaded
with enable_guc set to 1 (only enable GuC firmware loading). This is
because WOPCM registers were updated and locked without considering the HuC
FW size. Since we need both GuC and HuC FW sizes to determine the final
layout of WOPCM, we should always calculate the WOPCM layout based on the
actual sizes of the GuC and HuC firmware available for a specific platform
if we need continue to support enable/disable HuC FW loading dynamically
with enable_guc modparam.

This patch splits uC firmware fetching into two stages. First stage is to
fetch the firmware image and verify the firmware header. uC firmware will
be marked as verified and this will make FW info available for following
WOPCM layout calculation. The second stage is to create a GEM object and
copy the FW data into the created GEM object which will only be available
when GuC/HuC loading is enabled by enable_guc modparam. This will guarantee
that the WOPCM layout will be always be calculated correctly without making
any assumptions to the GuC and HuC firmware sizes.

v3:
 - Rebase

v4:
 - Renamed the new parameter add to intel_uc_fw_fetch (Michal)

Signed-off-by: Jackie Li <yaodong.li@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: John Spotswood <john.a.spotswood@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: John Spotswood <john.a.spotswood@intel.com>
---
 drivers/gpu/drm/i915/intel_uc.c    | 14 ++++----------
 drivers/gpu/drm/i915/intel_uc_fw.c | 31 ++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_uc_fw.h |  7 +++++--
 3 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 1cffaf7..73b8f6c 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -172,11 +172,8 @@ void intel_uc_init_early(struct drm_i915_private *i915)
 
 	sanitize_options_early(i915);
 
-	if (USES_GUC(i915))
-		intel_uc_fw_fetch(i915, &guc->fw);
-
-	if (USES_HUC(i915))
-		intel_uc_fw_fetch(i915, &huc->fw);
+	intel_uc_fw_fetch(i915, &guc->fw, USES_GUC(i915));
+	intel_uc_fw_fetch(i915, &huc->fw, USES_HUC(i915));
 }
 
 void intel_uc_cleanup_early(struct drm_i915_private *i915)
@@ -184,11 +181,8 @@ void intel_uc_cleanup_early(struct drm_i915_private *i915)
 	struct intel_guc *guc = &i915->guc;
 	struct intel_huc *huc = &i915->huc;
 
-	if (USES_HUC(i915))
-		intel_uc_fw_fini(&huc->fw);
-
-	if (USES_GUC(i915))
-		intel_uc_fw_fini(&guc->fw);
+	intel_uc_fw_fini(&huc->fw);
+	intel_uc_fw_fini(&guc->fw);
 
 	guc_free_load_err_log(guc);
 }
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
index 6e8e0b5..c1fed06 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/intel_uc_fw.c
@@ -33,11 +33,13 @@
  *
  * @dev_priv: device private
  * @uc_fw: uC firmware
+ * @fetch: whether fetch uC firmware into GEM object or not
  *
- * Fetch uC firmware into GEM obj.
+ * Fetch and verify uC firmware and copy firmware data into GEM object if
+ * @fetch is true.
  */
 void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
-		       struct intel_uc_fw *uc_fw)
+		       struct intel_uc_fw *uc_fw, bool fetch)
 {
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	struct drm_i915_gem_object *obj;
@@ -154,17 +156,24 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 		goto fail;
 	}
 
-	obj = i915_gem_object_create_from_data(dev_priv, fw->data, fw->size);
-	if (IS_ERR(obj)) {
-		err = PTR_ERR(obj);
-		DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
-				 intel_uc_fw_type_repr(uc_fw->type), err);
-		goto fail;
+	uc_fw->size = fw->size;
+	uc_fw->fetch_status = INTEL_UC_FIRMWARE_VERIFIED;
+
+	if (fetch) {
+		obj = i915_gem_object_create_from_data(dev_priv, fw->data,
+						       fw->size);
+		if (IS_ERR(obj)) {
+			err = PTR_ERR(obj);
+			DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
+					 intel_uc_fw_type_repr(uc_fw->type),
+					 err);
+			goto fail;
+		}
+
+		uc_fw->obj = obj;
+		uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
 	}
 
-	uc_fw->obj = obj;
-	uc_fw->size = fw->size;
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
 	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
 			 intel_uc_fw_type_repr(uc_fw->type),
 			 intel_uc_fw_status_repr(uc_fw->fetch_status));
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h
index dc33b12..a1308c0 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/intel_uc_fw.h
@@ -36,6 +36,7 @@ enum intel_uc_fw_status {
 	INTEL_UC_FIRMWARE_FAIL = -1,
 	INTEL_UC_FIRMWARE_NONE = 0,
 	INTEL_UC_FIRMWARE_PENDING,
+	INTEL_UC_FIRMWARE_VERIFIED,
 	INTEL_UC_FIRMWARE_SUCCESS
 };
 
@@ -84,6 +85,8 @@ const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
 		return "NONE";
 	case INTEL_UC_FIRMWARE_PENDING:
 		return "PENDING";
+	case INTEL_UC_FIRMWARE_VERIFIED:
+		return "VERIFIED";
 	case INTEL_UC_FIRMWARE_SUCCESS:
 		return "SUCCESS";
 	}
@@ -131,14 +134,14 @@ static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
  */
 static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw)
 {
-	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
+	if (uc_fw->fetch_status < INTEL_UC_FIRMWARE_VERIFIED)
 		return 0;
 
 	return uc_fw->header_size + uc_fw->ucode_size;
 }
 
 void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
-		       struct intel_uc_fw *uc_fw);
+		       struct intel_uc_fw *uc_fw, bool fetch);
 int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
 		       int (*xfer)(struct intel_uc_fw *uc_fw,
 				   struct i915_vma *vma));
-- 
2.7.4

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

[PATCH v4 2/4] drm/i915: Always set HUC_LOADING_AGENT_GUC bit in WOPCM offset register

[Jackie Li]

The enable_guc modparam is used to enable/disable GuC/HuC FW uploading
dynamically during i915 module loading. If WOPCM offset register was locked
without having HUC_LOADING_AGENT_GUC bit set to 1, the module reloading
with both GuC and HuC FW will fail since we need to set this bit to 1 for
HuC FW uploading.

Since HUC_LOADING_AGENT_GUC bit has no impact on GuC FW uploading, this
patch updates the register updating code to make sure the WOPCM offset
register is always locked with HUC_LOADING_AGENT_GUC bit set to 1 which
will guarantee successful uploading of both GuC and HuC FW. We will further
take care of the locked values in the following enhancement patch.

v4:
 - Fixed typo in commit message (Michal Wajdeczko)

Signed-off-by: Jackie Li <yaodong.li@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: John Spotswood <john.a.spotswood@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: John Spotswood <john.a.spotswood@intel.com>
---
 drivers/gpu/drm/i915/intel_wopcm.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
index 74bf76f..b1c08ca 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -238,8 +238,6 @@ static inline int write_and_verify(struct drm_i915_private *dev_priv,
 int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
 {
 	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
-	u32 huc_agent;
-	u32 mask;
 	int err;
 
 	if (!USES_GUC(dev_priv))
@@ -255,10 +253,10 @@ int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
 	if (err)
 		goto err_out;
 
-	huc_agent = USES_HUC(dev_priv) ? HUC_LOADING_AGENT_GUC : 0;
-	mask = GUC_WOPCM_OFFSET_MASK | GUC_WOPCM_OFFSET_VALID | huc_agent;
 	err = write_and_verify(dev_priv, DMA_GUC_WOPCM_OFFSET,
-			       wopcm->guc.base | huc_agent, mask,
+			       wopcm->guc.base | HUC_LOADING_AGENT_GUC,
+			       GUC_WOPCM_OFFSET_MASK | HUC_LOADING_AGENT_GUC |
+			       GUC_WOPCM_OFFSET_VALID,
 			       GUC_WOPCM_OFFSET_VALID);
 	if (err)
 		goto err_out;
-- 
2.7.4

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

[PATCH v4 3/4] drm/i915: Add code to accept valid locked WOPCM register values

[Jackie Li]

In current code, we only compare the locked WOPCM register values with the
calculated values. However, we can continue loading GuC/HuC firmware if the
locked (or partially locked) values were valid for current GuC/HuC firmware
sizes.

This patch added a new code path to verify whether the locked register
values can be used for GuC/HuC firmware loading, it will recalculate the
verify the new values if these registers were partially locked, so that we
won't fail the GuC/HuC firmware loading even if the locked register values
are different from the calculated ones.

v2:
 - Update WOPCM register only if it's not locked

v4:
 - Fixed typo in code comments (Michal)
 - Refined function names and parameters (Michal)
 - Avoided duplicated function calls (Michal)
 - Refined register updating ordering (Michal)

Signed-off-by: Jackie Li <yaodong.li@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: John Spotswood <john.a.spotswood@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: John Spotswood <john.a.spotswood@intel.com>
---
 drivers/gpu/drm/i915/intel_wopcm.c | 251 ++++++++++++++++++++++++++++---------
 1 file changed, 194 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
index b1c08ca..8fdcc48 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -51,6 +51,8 @@
 /* 8KB from GUC_WOPCM_RESERVED is reserved for GuC stack. */
 #define GUC_WOPCM_STACK_RESERVED	(8 * 1024)
 
+/* GuC WOPCM Size value needs to be aligned to 4KB. */
+#define GUC_WOPCM_SIZE_ALIGNMENT	(1UL << GUC_WOPCM_SIZE_SHIFT)
 /* GuC WOPCM Offset value needs to be aligned to 16KB. */
 #define GUC_WOPCM_OFFSET_ALIGNMENT	(1UL << GUC_WOPCM_OFFSET_SHIFT)
 
@@ -86,60 +88,104 @@ static inline u32 context_reserved_size(struct drm_i915_private *i915)
 		return 0;
 }
 
-static inline int gen9_check_dword_gap(u32 guc_wopcm_base, u32 guc_wopcm_size)
+static inline int gen9_check_dword_gap(struct intel_wopcm *wopcm)
 {
-	u32 offset;
+	u32 gen9_min_guc_wopcm_size = wopcm->guc.base + GEN9_GUC_WOPCM_OFFSET +
+				      sizeof(u32);
 
 	/*
 	 * GuC WOPCM size shall be at least a dword larger than the offset from
 	 * WOPCM base (GuC WOPCM offset from WOPCM base + GEN9_GUC_WOPCM_OFFSET)
 	 * due to hardware limitation on Gen9.
 	 */
-	offset = guc_wopcm_base + GEN9_GUC_WOPCM_OFFSET;
-	if (offset > guc_wopcm_size ||
-	    (guc_wopcm_size - offset) < sizeof(u32)) {
+	if (wopcm->guc.size < gen9_min_guc_wopcm_size) {
 		DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB needed.\n",
-			  guc_wopcm_size / 1024,
-			  (u32)(offset + sizeof(u32)) / 1024);
+			  wopcm->guc.size / 1024,
+			  gen9_min_guc_wopcm_size / 1024);
 		return -E2BIG;
 	}
 
 	return 0;
 }
 
-static inline int gen9_check_huc_fw_fits(u32 guc_wopcm_size, u32 huc_fw_size)
+static inline int gen9_check_huc_fw_fits(struct intel_wopcm *wopcm,
+					 u32 huc_fw_size)
 {
+	u32 available_guc_wopcm = wopcm->guc.size - GUC_WOPCM_RESERVED;
+
 	/*
 	 * On Gen9 & CNL A0, hardware requires the total available GuC WOPCM
 	 * size to be larger than or equal to HuC firmware size. Otherwise,
 	 * firmware uploading would fail.
 	 */
-	if (huc_fw_size > guc_wopcm_size - GUC_WOPCM_RESERVED) {
+	if (huc_fw_size > available_guc_wopcm) {
 		DRM_ERROR("HuC FW (%uKiB) won't fit in GuC WOPCM (%uKiB).\n",
-			  huc_fw_size / 1024,
-			  (guc_wopcm_size - GUC_WOPCM_RESERVED) / 1024);
+			  huc_fw_size / 1024, available_guc_wopcm / 1024);
 		return -E2BIG;
 	}
 
 	return 0;
 }
 
-static inline int check_hw_restriction(struct drm_i915_private *i915,
-				       u32 guc_wopcm_base, u32 guc_wopcm_size,
+static inline int check_hw_restriction(struct intel_wopcm *wopcm,
 				       u32 huc_fw_size)
 {
+	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
 	int err = 0;
 
 	if (IS_GEN9(i915))
-		err = gen9_check_dword_gap(guc_wopcm_base, guc_wopcm_size);
+		err = gen9_check_dword_gap(wopcm);
 
 	if (!err &&
 	    (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0)))
-		err = gen9_check_huc_fw_fits(guc_wopcm_size, huc_fw_size);
+		err = gen9_check_huc_fw_fits(wopcm, huc_fw_size);
 
 	return err;
 }
 
+static inline u32 fw_usable_wopcm_size(struct intel_wopcm *wopcm)
+{
+	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
+	u32 ctx_rsvd_size = context_reserved_size(i915);
+
+	return wopcm->size - ctx_rsvd_size;
+}
+
+static inline u32 min_guc_wopcm_size(u32 guc_fw_size)
+{
+	return ALIGN(guc_fw_size + GUC_WOPCM_RESERVED +
+		     GUC_WOPCM_STACK_RESERVED, GUC_WOPCM_SIZE_ALIGNMENT);
+}
+
+static inline u32 min_guc_wopcm_base(u32 huc_fw_size)
+{
+	return ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
+		     GUC_WOPCM_OFFSET_ALIGNMENT);
+}
+
+static inline u32 max_guc_wopcm_size(u32 max_available_wopcm_size,
+				     u32 min_guc_wopcm_base)
+{
+	if (max_available_wopcm_size > min_guc_wopcm_base)
+		return (max_available_wopcm_size - min_guc_wopcm_base) &
+		       GUC_WOPCM_SIZE_MASK;
+	return 0;
+}
+
+static inline int verify_calculated_values(struct intel_wopcm *wopcm,
+					   u32 guc_fw_size, u32 huc_fw_size)
+{
+	u32 min_size = min_guc_wopcm_size(guc_fw_size);
+
+	if (wopcm->guc.size < min_size) {
+		DRM_ERROR("Need %uKiB WOPCM for GuC FW, %uKiB available.\n",
+			  min_size, wopcm->guc.size / 1024);
+		return -E2BIG;
+	}
+
+	return check_hw_restriction(wopcm, huc_fw_size);
+}
+
 /**
  * intel_wopcm_init() - Initialize the WOPCM structure.
  * @wopcm: pointer to intel_wopcm.
@@ -150,66 +196,135 @@ static inline int check_hw_restriction(struct drm_i915_private *i915,
  * size. It will fail the WOPCM init if any of these checks were failed, so that
  * the following GuC firmware uploading would be aborted.
  *
- * Return: 0 on success, non-zero error code on failure.
+ * Return: 0 on success. Minus error code if registers were locked with
+ * incorrect values. -EIO if registers failed to lock with correct values.
  */
 int intel_wopcm_init(struct intel_wopcm *wopcm)
 {
 	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
 	u32 guc_fw_size = intel_uc_fw_get_upload_size(&i915->guc.fw);
 	u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw);
-	u32 ctx_rsvd = context_reserved_size(i915);
-	u32 guc_wopcm_base;
-	u32 guc_wopcm_size;
-	u32 guc_wopcm_rsvd;
+	u32 fw_usable_size = fw_usable_wopcm_size(wopcm);
 	int err;
 
 	GEM_BUG_ON(!wopcm->size);
 
-	if (guc_fw_size >= wopcm->size) {
+	if (guc_fw_size >= fw_usable_size) {
 		DRM_ERROR("GuC FW (%uKiB) is too big to fit in WOPCM.",
 			  guc_fw_size / 1024);
 		return -E2BIG;
 	}
 
-	if (huc_fw_size >= wopcm->size) {
+	if (huc_fw_size >= fw_usable_size) {
 		DRM_ERROR("HuC FW (%uKiB) is too big to fit in WOPCM.",
 			  huc_fw_size / 1024);
 		return -E2BIG;
 	}
 
-	guc_wopcm_base = ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
-			       GUC_WOPCM_OFFSET_ALIGNMENT);
-	if ((guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
-		DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
-			  guc_wopcm_base / 1024);
-		return -E2BIG;
-	}
-
-	guc_wopcm_size = wopcm->size - guc_wopcm_base - ctx_rsvd;
-	guc_wopcm_size &= GUC_WOPCM_SIZE_MASK;
+	wopcm->guc.base = min_guc_wopcm_base(huc_fw_size);
+	wopcm->guc.size = max_guc_wopcm_size(fw_usable_size, wopcm->guc.base);
 
 	DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB, %uKiB)\n",
-			 guc_wopcm_base / 1024, guc_wopcm_size / 1024);
+			 wopcm->guc.base / 1024,
+			 (wopcm->guc.base + wopcm->guc.size) / 1024);
 
-	guc_wopcm_rsvd = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;
-	if ((guc_fw_size + guc_wopcm_rsvd) > guc_wopcm_size) {
-		DRM_ERROR("Need %uKiB WOPCM for GuC, %uKiB available.\n",
-			  (guc_fw_size + guc_wopcm_rsvd) / 1024,
-			  guc_wopcm_size / 1024);
-		return -E2BIG;
-	}
-
-	err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size,
-				   huc_fw_size);
+	err = verify_calculated_values(wopcm, guc_fw_size, huc_fw_size);
 	if (err)
 		return err;
 
-	wopcm->guc.base = guc_wopcm_base;
-	wopcm->guc.size = guc_wopcm_size;
-
 	return 0;
 }
 
+static inline int verify_locked_values(struct intel_wopcm *wopcm)
+{
+	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
+	u32 guc_fw_size = intel_uc_fw_get_upload_size(&i915->guc.fw);
+	u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw);
+	u32 fw_usable_size = fw_usable_wopcm_size(wopcm);
+	u32 min_base = min_guc_wopcm_base(huc_fw_size);
+
+	/*
+	 * Locked GuC WOPCM base and size could be any values which are large
+	 * enough to lead to a wrap around after add operations.
+	 */
+	if (wopcm->guc.base >= fw_usable_size) {
+		DRM_ERROR("Locked base value %uKiB >= WOPCM size %uKiB.\n",
+			  wopcm->guc.base / 1024,
+			  wopcm->size / 1024);
+		return -E2BIG;
+	}
+
+	if (wopcm->guc.size >= fw_usable_size) {
+		DRM_ERROR("Locked size value %uKiB >= WOPCM size %uKiB.\n",
+			  wopcm->guc.size / 1024,
+			  wopcm->size / 1024);
+		return -E2BIG;
+	}
+
+	if (wopcm->guc.base + wopcm->guc.size  > fw_usable_size) {
+		DRM_ERROR("Need %uKiB WOPCM for firmware, %uKiB available.\n",
+			  (wopcm->guc.base + wopcm->guc.size) / 1024,
+			  fw_usable_size / 1024);
+		return -E2BIG;
+	}
+
+	if (wopcm->guc.base < min_base) {
+		DRM_ERROR("Need %uKiB WOPCM for HuC FW, %uKiB available.\n",
+			  min_base / 1024,
+			  wopcm->guc.base / 1024);
+		return -E2BIG;
+	}
+
+	return verify_calculated_values(wopcm, guc_fw_size, huc_fw_size);
+}
+
+/*
+ * Check WOPCM write-once register status. if locked with different values
+ * from the calculated ones, then this function will try to recalculate the
+ * values when possible (partially locked) and verify the recalculated value.
+ */
+static inline int check_locked_values(struct intel_wopcm *wopcm,
+				      bool *update_size_reg,
+				      bool *update_offset_reg)
+{
+	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
+	u32 huc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->huc.fw);
+	u32 fw_usable_size = fw_usable_wopcm_size(wopcm);
+	u32 offset_val = I915_READ(DMA_GUC_WOPCM_OFFSET);
+	u32 size_val = I915_READ(GUC_WOPCM_SIZE);
+	bool offset_reg_locked = offset_val & GUC_WOPCM_OFFSET_VALID;
+	bool size_reg_locked = size_val & GUC_WOPCM_SIZE_LOCKED;
+
+	*update_size_reg = !size_reg_locked;
+	*update_offset_reg = !offset_reg_locked;
+
+	if (!offset_reg_locked && !size_reg_locked)
+		return 0;
+
+	if (wopcm->guc.base == (offset_val & GUC_WOPCM_OFFSET_MASK) &&
+	    wopcm->guc.size == (size_val & GUC_WOPCM_SIZE_MASK))
+		return 0;
+
+	if (USES_HUC(dev_priv) && offset_reg_locked &&
+	    !(offset_val & HUC_LOADING_AGENT_GUC)) {
+		DRM_ERROR("HUC_LOADING_AGENT_GUC isn't locked for USES_HUC.\n");
+		return -EIO;
+	}
+
+	if (!offset_reg_locked)
+		wopcm->guc.base = min_guc_wopcm_base(huc_fw_size);
+
+	if (!size_reg_locked)
+		wopcm->guc.size = max_guc_wopcm_size(fw_usable_size,
+						     wopcm->guc.base);
+
+	DRM_DEBUG_DRIVER("Recalculated GuC WOPCM Region: [%uKiB, %uKiB)\n",
+			 wopcm->guc.base / 1024,
+			 (wopcm->guc.base + wopcm->guc.size) / 1024);
+
+	return verify_locked_values(wopcm);
+}
+
 static inline int write_and_verify(struct drm_i915_private *dev_priv,
 				   i915_reg_t reg, u32 val, u32 mask,
 				   u32 locked_bit)
@@ -238,6 +353,8 @@ static inline int write_and_verify(struct drm_i915_private *dev_priv,
 int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
 {
 	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
+	bool update_size_reg;
+	bool update_offset_reg;
 	int err;
 
 	if (!USES_GUC(dev_priv))
@@ -247,19 +364,38 @@ int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
 	GEM_BUG_ON(!wopcm->guc.size);
 	GEM_BUG_ON(!wopcm->guc.base);
 
-	err = write_and_verify(dev_priv, GUC_WOPCM_SIZE, wopcm->guc.size,
-			       GUC_WOPCM_SIZE_MASK | GUC_WOPCM_SIZE_LOCKED,
-			       GUC_WOPCM_SIZE_LOCKED);
-	if (err)
+	err = check_locked_values(wopcm, &update_size_reg, &update_offset_reg);
+	if (err) {
+		DRM_ERROR("WOPCM registers are locked with invalid values.\n");
 		goto err_out;
+	}
 
-	err = write_and_verify(dev_priv, DMA_GUC_WOPCM_OFFSET,
-			       wopcm->guc.base | HUC_LOADING_AGENT_GUC,
-			       GUC_WOPCM_OFFSET_MASK | HUC_LOADING_AGENT_GUC |
-			       GUC_WOPCM_OFFSET_VALID,
-			       GUC_WOPCM_OFFSET_VALID);
-	if (err)
-		goto err_out;
+	if (update_offset_reg) {
+		err = write_and_verify(dev_priv, DMA_GUC_WOPCM_OFFSET,
+				       wopcm->guc.base | HUC_LOADING_AGENT_GUC,
+				       GUC_WOPCM_OFFSET_MASK |
+				       HUC_LOADING_AGENT_GUC |
+				       GUC_WOPCM_OFFSET_VALID,
+				       GUC_WOPCM_OFFSET_VALID);
+		if (err) {
+			DRM_ERROR("Failed to GuC WOPCM offset with %#x.\n",
+				  wopcm->guc.base);
+			goto err_out;
+		}
+	}
+
+	if (update_size_reg) {
+		err = write_and_verify(dev_priv, GUC_WOPCM_SIZE,
+				       wopcm->guc.size,
+				       GUC_WOPCM_SIZE_MASK |
+				       GUC_WOPCM_SIZE_LOCKED,
+				       GUC_WOPCM_SIZE_LOCKED);
+		if (err) {
+			DRM_ERROR("Failed to GuC WOPCM size with %#x.\n",
+				  wopcm->guc.size);
+			goto err_out;
+		}
+	}
 
 	return 0;
 
@@ -268,6 +404,7 @@ int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
 	DRM_ERROR("DMA_GUC_WOPCM_OFFSET=%#x\n",
 		  I915_READ(DMA_GUC_WOPCM_OFFSET));
 	DRM_ERROR("GUC_WOPCM_SIZE=%#x\n", I915_READ(GUC_WOPCM_SIZE));
+	DRM_ERROR("A system reboot is required.\n");
 
 	return err;
 }
-- 
2.7.4

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

[PATCH v4 4/4] HAX enable guc for CI

[Jackie Li]

Signed-off-by: Jackie Li <yaodong.li@intel.com>
---
 drivers/gpu/drm/i915/i915_params.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index c963603..53037b5 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -47,7 +47,7 @@ struct drm_printer;
 	param(int, disable_power_well, -1) \
 	param(int, enable_ips, 1) \
 	param(int, invert_brightness, 0) \
-	param(int, enable_guc, 0) \
+	param(int, enable_guc, -1) \
 	param(int, guc_log_level, -1) \
 	param(char *, guc_firmware_path, NULL) \
 	param(char *, huc_firmware_path, NULL) \
-- 
2.7.4

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

✓ Fi.CI.BAT: success for series starting with [v4,1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes

[Patchwork]

== Series Details ==

Series: series starting with [v4,1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes
URL   : https://patchwork.freedesktop.org/series/41909/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4066 -> Patchwork_8739 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/41909/revisions/1/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-bxt-dsi:         NOTRUN -> INCOMPLETE (fdo#103927)

    
    ==== Possible fixes ====

    igt@gem_mmap_gtt@basic-small-bo-tiledx:
      fi-gdg-551:         FAIL (fdo#102575) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-ivb-3520m:       DMESG-WARN (fdo#106084) -> PASS

    
  fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#106084 https://bugs.freedesktop.org/show_bug.cgi?id=106084


== Participating hosts (33 -> 31) ==

  Additional (2): fi-glk-j4005 fi-bxt-dsi 
  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-glk-1 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4066 -> Patchwork_8739

  CI_DRM_4066: e1fbca4821d0700551df233285a5c28db09fd0f6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4441: 83ba5b7d3bde48b383df41792fc9c955a5a23bdb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8739: f6e5a01f3c8931f5b6420e877f8a8e596821926f @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4441: e60d247eb359f044caf0c09904da14e39d7adca1 @ git://anongit.freedesktop.org/piglit


== Linux commits ==

f6e5a01f3c89 HAX enable guc for CI
03aa3dbadc8a drm/i915: Add code to accept valid locked WOPCM register values
c531aeed1378 drm/i915: Always set HUC_LOADING_AGENT_GUC bit in WOPCM offset register
a83dfdf266d5 drm/i915: Always do WOPCM partitioning based on real firmware sizes

== Logs ==

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

✗ Fi.CI.IGT: failure for series starting with [v4,1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes

[Patchwork]

== Series Details ==

Series: series starting with [v4,1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes
URL   : https://patchwork.freedesktop.org/series/41909/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4066_full -> Patchwork_8739_full =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_8739_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8739_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/41909/revisions/1/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_frontbuffer_tracking@fbc-suspend:
      shard-kbl:          PASS -> DMESG-WARN +3

    igt@perf@gen8-unprivileged-single-ctx-counters:
      shard-kbl:          PASS -> FAIL +1
      shard-apl:          PASS -> FAIL

    
    ==== Warnings ====

    igt@drv_missed_irq:
      shard-apl:          PASS -> SKIP

    igt@gem_mocs_settings@mocs-rc6-dirty-render:
      shard-kbl:          SKIP -> PASS +2

    igt@prime_vgem@sync-bsd2:
      shard-kbl:          PASS -> SKIP +31

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_eio@execbuf:
      shard-apl:          PASS -> INCOMPLETE (fdo#103927) +1

    igt@kms_flip@2x-wf_vblank-ts-check:
      shard-hsw:          PASS -> FAIL (fdo#100368)

    igt@kms_flip@modeset-vs-vblank-race:
      shard-hsw:          PASS -> FAIL (fdo#103060) +1

    igt@kms_setmode@basic:
      shard-hsw:          PASS -> FAIL (fdo#99912)

    igt@prime_busy@wait-hang-vebox:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665) +4

    
    ==== Possible fixes ====

    igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy:
      shard-hsw:          FAIL (fdo#105767) -> PASS

    igt@kms_flip@2x-dpms-vs-vblank-race:
      shard-hsw:          FAIL (fdo#103060) -> PASS

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-hsw:          FAIL (fdo#102887) -> PASS

    igt@kms_flip@flip-vs-blocking-wf-vblank:
      shard-hsw:          FAIL (fdo#100368) -> PASS

    igt@kms_frontbuffer_tracking@fbc-farfromfence:
      shard-kbl:          DMESG-WARN (fdo#105602, fdo#103558) -> PASS +1

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105767 https://bugs.freedesktop.org/show_bug.cgi?id=105767
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (9 -> 4) ==

  Missing    (5): shard-glk8 shard-glk6 shard-glk7 shard-glk shard-glkb 


== Build changes ==

    * Linux: CI_DRM_4066 -> Patchwork_8739

  CI_DRM_4066: e1fbca4821d0700551df233285a5c28db09fd0f6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4441: 83ba5b7d3bde48b383df41792fc9c955a5a23bdb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8739: f6e5a01f3c8931f5b6420e877f8a8e596821926f @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4441: e60d247eb359f044caf0c09904da14e39d7adca1 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [PATCH v4 1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes

[Michal Wajdeczko]

On Wed, 18 Apr 2018 19:01:31 +0200, Jackie Li <yaodong.li@intel.com> wrote:

> After enabled the WOPCM write-once registers locking status checking,
> reloading of the i915 module will fail with modparam enable_guc set to 3
> (enable GuC and HuC firmware loading) if the module was originally loaded
> with enable_guc set to 1 (only enable GuC firmware loading). This is
> because WOPCM registers were updated and locked without considering the  
> HuC
> FW size. Since we need both GuC and HuC FW sizes to determine the final
> layout of WOPCM, we should always calculate the WOPCM layout based on the
> actual sizes of the GuC and HuC firmware available for a specific  
> platform
> if we need continue to support enable/disable HuC FW loading dynamically
> with enable_guc modparam.
>
> This patch splits uC firmware fetching into two stages. First stage is to
> fetch the firmware image and verify the firmware header. uC firmware will
> be marked as verified and this will make FW info available for following
> WOPCM layout calculation. The second stage is to create a GEM object and
> copy the FW data into the created GEM object which will only be available
> when GuC/HuC loading is enabled by enable_guc modparam. This will  
> guarantee
> that the WOPCM layout will be always be calculated correctly without  
> making
> any assumptions to the GuC and HuC firmware sizes.
>
> v3:
>  - Rebase
>
> v4:
>  - Renamed the new parameter add to intel_uc_fw_fetch (Michal)
>
> Signed-off-by: Jackie Li <yaodong.li@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: John Spotswood <john.a.spotswood@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Reviewed-by: John Spotswood <john.a.spotswood@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uc.c    | 14 ++++----------
>  drivers/gpu/drm/i915/intel_uc_fw.c | 31 ++++++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_uc_fw.h |  7 +++++--
>  3 files changed, 29 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index 1cffaf7..73b8f6c 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -172,11 +172,8 @@ void intel_uc_init_early(struct drm_i915_private  
> *i915)
> 	sanitize_options_early(i915);
> -	if (USES_GUC(i915))
> -		intel_uc_fw_fetch(i915, &guc->fw);
> -
> -	if (USES_HUC(i915))
> -		intel_uc_fw_fetch(i915, &huc->fw);
> +	intel_uc_fw_fetch(i915, &guc->fw, USES_GUC(i915));
> +	intel_uc_fw_fetch(i915, &huc->fw, USES_HUC(i915));

Again: I'm don't think that unconditional fetch of HuC fw is a right  
choice.
We should look for other options how to support enable_guc 1-->3 scenario.

>  }
> void intel_uc_cleanup_early(struct drm_i915_private *i915)
> @@ -184,11 +181,8 @@ void intel_uc_cleanup_early(struct drm_i915_private  
> *i915)
>  	struct intel_guc *guc = &i915->guc;
>  	struct intel_huc *huc = &i915->huc;
> -	if (USES_HUC(i915))
> -		intel_uc_fw_fini(&huc->fw);
> -
> -	if (USES_GUC(i915))
> -		intel_uc_fw_fini(&guc->fw);
> +	intel_uc_fw_fini(&huc->fw);
> +	intel_uc_fw_fini(&guc->fw);
> 	guc_free_load_err_log(guc);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c  
> b/drivers/gpu/drm/i915/intel_uc_fw.c
> index 6e8e0b5..c1fed06 100644
> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
> @@ -33,11 +33,13 @@
>   *
>   * @dev_priv: device private
>   * @uc_fw: uC firmware
> + * @fetch: whether fetch uC firmware into GEM object or not
>   *
> - * Fetch uC firmware into GEM obj.
> + * Fetch and verify uC firmware and copy firmware data into GEM object  
> if
> + * @fetch is true.
>   */
>  void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> -		       struct intel_uc_fw *uc_fw)
> +		       struct intel_uc_fw *uc_fw, bool fetch)
>  {
>  	struct pci_dev *pdev = dev_priv->drm.pdev;
>  	struct drm_i915_gem_object *obj;
> @@ -154,17 +156,24 @@ void intel_uc_fw_fetch(struct drm_i915_private  
> *dev_priv,
>  		goto fail;
>  	}
> -	obj = i915_gem_object_create_from_data(dev_priv, fw->data, fw->size);
> -	if (IS_ERR(obj)) {
> -		err = PTR_ERR(obj);
> -		DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
> -				 intel_uc_fw_type_repr(uc_fw->type), err);
> -		goto fail;
> +	uc_fw->size = fw->size;
> +	uc_fw->fetch_status = INTEL_UC_FIRMWARE_VERIFIED;

btw, I'm not sure that this new state is needed at all, as you don't
plan to use that fw obj if you only loaded fw blob to read header...

> +
> +	if (fetch) {
> +		obj = i915_gem_object_create_from_data(dev_priv, fw->data,
> +						       fw->size);
> +		if (IS_ERR(obj)) {
> +			err = PTR_ERR(obj);
> +			DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
> +					 intel_uc_fw_type_repr(uc_fw->type),
> +					 err);
> +			goto fail;
> +		}
> +
> +		uc_fw->obj = obj;
> +		uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
>  	}
> -	uc_fw->obj = obj;
> -	uc_fw->size = fw->size;
> -	uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
>  	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
>  			 intel_uc_fw_type_repr(uc_fw->type),
>  			 intel_uc_fw_status_repr(uc_fw->fetch_status));
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h  
> b/drivers/gpu/drm/i915/intel_uc_fw.h
> index dc33b12..a1308c0 100644
> --- a/drivers/gpu/drm/i915/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.h
> @@ -36,6 +36,7 @@ enum intel_uc_fw_status {
>  	INTEL_UC_FIRMWARE_FAIL = -1,
>  	INTEL_UC_FIRMWARE_NONE = 0,
>  	INTEL_UC_FIRMWARE_PENDING,
> +	INTEL_UC_FIRMWARE_VERIFIED,
>  	INTEL_UC_FIRMWARE_SUCCESS
>  };
> @@ -84,6 +85,8 @@ const char *intel_uc_fw_status_repr(enum  
> intel_uc_fw_status status)
>  		return "NONE";
>  	case INTEL_UC_FIRMWARE_PENDING:
>  		return "PENDING";
> +	case INTEL_UC_FIRMWARE_VERIFIED:
> +		return "VERIFIED";
>  	case INTEL_UC_FIRMWARE_SUCCESS:
>  		return "SUCCESS";
>  	}
> @@ -131,14 +134,14 @@ static inline void intel_uc_fw_sanitize(struct  
> intel_uc_fw *uc_fw)
>   */
>  static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw)
>  {
> -	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> +	if (uc_fw->fetch_status < INTEL_UC_FIRMWARE_VERIFIED)
>  		return 0;
> 	return uc_fw->header_size + uc_fw->ucode_size;
>  }
> void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> -		       struct intel_uc_fw *uc_fw);
> +		       struct intel_uc_fw *uc_fw, bool fetch);
>  int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
>  		       int (*xfer)(struct intel_uc_fw *uc_fw,
>  				   struct i915_vma *vma));
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes

[Yaodong Li]

On 04/19/2018 08:45 AM, Michal Wajdeczko wrote:
> On Wed, 18 Apr 2018 19:01:31 +0200, Jackie Li <yaodong.li@intel.com> 
> wrote:
>
>> After enabled the WOPCM write-once registers locking status checking,
>> reloading of the i915 module will fail with modparam enable_guc set to 3
>> (enable GuC and HuC firmware loading) if the module was originally 
>> loaded
>> with enable_guc set to 1 (only enable GuC firmware loading). This is
>> because WOPCM registers were updated and locked without considering 
>> the HuC
>> FW size. Since we need both GuC and HuC FW sizes to determine the final
>> layout of WOPCM, we should always calculate the WOPCM layout based on 
>> the
>> actual sizes of the GuC and HuC firmware available for a specific 
>> platform
>> if we need continue to support enable/disable HuC FW loading dynamically
>> with enable_guc modparam.
>>
>> This patch splits uC firmware fetching into two stages. First stage 
>> is to
>> fetch the firmware image and verify the firmware header. uC firmware 
>> will
>> be marked as verified and this will make FW info available for following
>> WOPCM layout calculation. The second stage is to create a GEM object and
>> copy the FW data into the created GEM object which will only be 
>> available
>> when GuC/HuC loading is enabled by enable_guc modparam. This will 
>> guarantee
>> that the WOPCM layout will be always be calculated correctly without 
>> making
>> any assumptions to the GuC and HuC firmware sizes.
>>
>> v3:
>>  - Rebase
>>
>> v4:
>>  - Renamed the new parameter add to intel_uc_fw_fetch (Michal)
>>
>> Signed-off-by: Jackie Li <yaodong.li@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Winiarski <michal.winiarski@intel.com>
>> Cc: John Spotswood <john.a.spotswood@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Reviewed-by: John Spotswood <john.a.spotswood@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_uc.c    | 14 ++++----------
>>  drivers/gpu/drm/i915/intel_uc_fw.c | 31 ++++++++++++++++++++-----------
>>  drivers/gpu/drm/i915/intel_uc_fw.h |  7 +++++--
>>  3 files changed, 29 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 1cffaf7..73b8f6c 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -172,11 +172,8 @@ void intel_uc_init_early(struct drm_i915_private 
>> *i915)
>>     sanitize_options_early(i915);
>> -    if (USES_GUC(i915))
>> -        intel_uc_fw_fetch(i915, &guc->fw);
>> -
>> -    if (USES_HUC(i915))
>> -        intel_uc_fw_fetch(i915, &huc->fw);
>> +    intel_uc_fw_fetch(i915, &guc->fw, USES_GUC(i915));
>> +    intel_uc_fw_fetch(i915, &huc->fw, USES_HUC(i915));
>
> Again: I'm don't think that unconditional fetch of HuC fw is a right 
> choice.
> We should look for other options how to support enable_guc 1-->3 
> scenario.
>
This is the real fix I can think of to support such a scenario. I think 
the performance
impact here is minimal since it's only one time operation. I will think 
about the
use case of HAS_HUC = 1 and only enable guc submission.

But first I suggest we need to define the expected use case (like I 
mentioned in my last reply):
how to define "support enable_guc 1->3" (whether we should expect some 
system reboot, or
we need guarantee 100% work with no system reboot required)? whether a 
system reboot for
FW changes should be treated as code defects, etc.
>>  }
>> void intel_uc_cleanup_early(struct drm_i915_private *i915)
>> @@ -184,11 +181,8 @@ void intel_uc_cleanup_early(struct 
>> drm_i915_private *i915)
>>      struct intel_guc *guc = &i915->guc;
>>      struct intel_huc *huc = &i915->huc;
>> -    if (USES_HUC(i915))
>> -        intel_uc_fw_fini(&huc->fw);
>> -
>> -    if (USES_GUC(i915))
>> -        intel_uc_fw_fini(&guc->fw);
>> +    intel_uc_fw_fini(&huc->fw);
>> +    intel_uc_fw_fini(&guc->fw);
>>     guc_free_load_err_log(guc);
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c 
>> b/drivers/gpu/drm/i915/intel_uc_fw.c
>> index 6e8e0b5..c1fed06 100644
>> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
>> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
>> @@ -33,11 +33,13 @@
>>   *
>>   * @dev_priv: device private
>>   * @uc_fw: uC firmware
>> + * @fetch: whether fetch uC firmware into GEM object or not
>>   *
>> - * Fetch uC firmware into GEM obj.
>> + * Fetch and verify uC firmware and copy firmware data into GEM 
>> object if
>> + * @fetch is true.
>>   */
>>  void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>> -               struct intel_uc_fw *uc_fw)
>> +               struct intel_uc_fw *uc_fw, bool fetch)
>>  {
>>      struct pci_dev *pdev = dev_priv->drm.pdev;
>>      struct drm_i915_gem_object *obj;
>> @@ -154,17 +156,24 @@ void intel_uc_fw_fetch(struct drm_i915_private 
>> *dev_priv,
>>          goto fail;
>>      }
>> -    obj = i915_gem_object_create_from_data(dev_priv, fw->data, 
>> fw->size);
>> -    if (IS_ERR(obj)) {
>> -        err = PTR_ERR(obj);
>> -        DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
>> -                 intel_uc_fw_type_repr(uc_fw->type), err);
>> -        goto fail;
>> +    uc_fw->size = fw->size;
>> +    uc_fw->fetch_status = INTEL_UC_FIRMWARE_VERIFIED;
>
> btw, I'm not sure that this new state is needed at all, as you don't
> plan to use that fw obj if you only loaded fw blob to read header...
>
we will have the header info stored along with the intel_guc/intel_fw 
anyway.
this state only suggests that valid FW images are available but wasn't 
loaded
to the memory.

Regards,
-Jackie

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