All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] drm/i915: Add code to accept valid locked WOPCM register values
@ 2018-03-20 23:18 Jackie Li
  2018-03-20 23:18 ` [PATCH 2/2] HAX enable guc for CI Jackie Li
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Jackie Li @ 2018-03-20 23:18 UTC (permalink / raw)
  To: intel-gfx

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.

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: Michał Winiarski <michal.winiarski@intel.com>
Cc: John Spotswood <john.a.spotswood@intel.com>
---
 drivers/gpu/drm/i915/intel_wopcm.c | 183 +++++++++++++++++++++++++++++++------
 1 file changed, 157 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
index 4117886..fbd8aba 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -138,6 +138,53 @@ static inline int check_hw_restriction(struct drm_i915_private *i915,
 	return err;
 }
 
+static inline u32 calculate_min_guc_wopcm_base(u32 huc_fw_size)
+{
+	return ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
+		     GUC_WOPCM_OFFSET_ALIGNMENT);
+}
+
+static inline u32 calculate_min_guc_wopcm_size(u32 guc_fw_size)
+{
+	return guc_fw_size + GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;
+}
+
+static inline int calculate_max_guc_wopcm_size(struct intel_wopcm *wopcm,
+					       u32 guc_wopcm_base,
+					       u32 *guc_wopcm_size)
+{
+	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
+	u32 ctx_rsvd = context_reserved_size(i915);
+
+	if (guc_wopcm_base >= wopcm->size ||
+	    (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_MASK;
+
+	return 0;
+}
+
+static inline int verify_calculated_values(struct drm_i915_private *i915,
+					   u32 guc_fw_size, u32 huc_fw_size,
+					   u32 guc_wopcm_base,
+					   u32 guc_wopcm_size)
+{
+	if (guc_wopcm_size < calculate_min_guc_wopcm_size(guc_fw_size)) {
+		DRM_ERROR("Need %uKiB WOPCM for GuC FW, %uKiB available.\n",
+			  calculate_min_guc_wopcm_size(guc_fw_size),
+			  guc_wopcm_size / 1024);
+		return -E2BIG;
+	}
+
+	return check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size,
+				    huc_fw_size);
+}
+
 /**
  * intel_wopcm_init() - Initialize the WOPCM structure.
  * @wopcm: pointer to intel_wopcm.
@@ -155,10 +202,8 @@ 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;
 	int err;
 
 	GEM_BUG_ON(!wopcm->size);
@@ -175,30 +220,17 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
 		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;
+	guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);
+	err = calculate_max_guc_wopcm_size(wopcm, guc_wopcm_base,
+					   &guc_wopcm_size);
+	if (err)
+		return err;
 
 	DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB, %uKiB)\n",
 			 guc_wopcm_base / 1024, guc_wopcm_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(i915, guc_fw_size, huc_fw_size,
+				       guc_wopcm_base, guc_wopcm_size);
 	if (err)
 		return err;
 
@@ -208,6 +240,92 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
 	return 0;
 }
 
+static inline int verify_locked_values(struct intel_wopcm *wopcm,
+				       u32 guc_wopcm_base, u32 guc_wopcm_size)
+{
+	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);
+
+	/*
+	 * Locked GuC WOPCM base and size could be any values are large enough
+	 * to lead to a wrap around after performing add operations.
+	 */
+	if (guc_wopcm_base >= wopcm->size) {
+		DRM_ERROR("Locked base value %uKiB >= WOPCM size %uKiB.\n",
+			  guc_wopcm_base / 1024,
+			  wopcm->size / 1024);
+		return -E2BIG;
+	}
+
+	if (guc_wopcm_size >= wopcm->size) {
+		DRM_ERROR("Locked size value %uKiB >= WOPCM size %uKiB.\n",
+			  guc_wopcm_base / 1024,
+			  wopcm->size / 1024);
+		return -E2BIG;
+	}
+
+	if (guc_wopcm_base + guc_wopcm_size + ctx_rsvd > wopcm->size) {
+		DRM_ERROR("Need %uKiB WOPCM in total, %uKiB available.\n",
+			  (guc_wopcm_base + guc_wopcm_size + ctx_rsvd) / 1024,
+			  (wopcm->size) / 1024);
+		return -E2BIG;
+	}
+
+	if (guc_wopcm_base < calculate_min_guc_wopcm_base(huc_fw_size)) {
+		DRM_ERROR("Need %uKiB WOPCM for HuC FW, %uKiB available.\n",
+			  calculate_min_guc_wopcm_base(huc_fw_size) / 1024,
+			  guc_wopcm_base / 1024);
+		return -E2BIG;
+	}
+
+	return verify_calculated_values(i915, guc_fw_size, huc_fw_size,
+					guc_wopcm_base, guc_wopcm_size);
+}
+
+static inline int verify_locked_values_and_update(struct intel_wopcm *wopcm)
+{
+	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 size_val = I915_READ(GUC_WOPCM_SIZE);
+	u32 offset_val = I915_READ(DMA_GUC_WOPCM_OFFSET);
+	bool offset_reg_locked = offset_val & GUC_WOPCM_OFFSET_VALID;
+	bool size_reg_locked = size_val & GUC_WOPCM_SIZE_LOCKED;
+	u32 guc_wopcm_base = offset_val & GUC_WOPCM_OFFSET_MASK;
+	u32 guc_wopcm_size = size_val & GUC_WOPCM_SIZE_MASK;
+	int err;
+
+	if (!offset_reg_locked && !size_reg_locked)
+		return 0;
+
+	if (guc_wopcm_base == wopcm->guc.base &&
+	    guc_wopcm_size == wopcm->guc.size)
+		return 0;
+
+	if (!offset_reg_locked)
+		guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);
+
+	if (!size_reg_locked) {
+		err = calculate_max_guc_wopcm_size(wopcm, guc_wopcm_base,
+						   &guc_wopcm_size);
+		if (err)
+			return err;
+	}
+
+	DRM_DEBUG_DRIVER("Recalculated GuC WOPCM Region: [%uKiB, %uKiB)\n",
+			 guc_wopcm_base / 1024, guc_wopcm_size / 1024);
+
+	err = verify_locked_values(wopcm, guc_wopcm_base, guc_wopcm_size);
+	if (err)
+		return err;
+
+	wopcm->guc.size = guc_wopcm_size;
+	wopcm->guc.base = guc_wopcm_base;
+
+	return 0;
+}
+
 static inline int write_and_verify(struct drm_i915_private *dev_priv,
 				   i915_reg_t reg, u32 val, u32 mask,
 				   u32 locked_bit)
@@ -231,7 +349,8 @@ static inline int write_and_verify(struct drm_i915_private *dev_priv,
  * will verify the register values to make sure the registers are locked with
  * correct values.
  *
- * Return: 0 on success. -EIO if registers were locked with incorrect values.
+ * Return: 0 on success. Minus error code if registered were locked with
+ * incorrect values.-EIO if registers failed to lock with correct values.
  */
 int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
 {
@@ -247,27 +366,39 @@ int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
 	GEM_BUG_ON(!wopcm->guc.size);
 	GEM_BUG_ON(!wopcm->guc.base);
 
+	err = verify_locked_values_and_update(wopcm);
+	if (err) {
+		DRM_ERROR("WOPCM registers are locked with invalid values.\n");
+		goto err_out;
+	}
+
 	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)
+	if (err) {
+		DRM_ERROR("Failed to lock GUC_WOPCM_SIZE with %#x.\n",
+			  wopcm->guc.size);
 		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,
 			       GUC_WOPCM_OFFSET_VALID);
-	if (err)
+	if (err) {
+		DRM_ERROR("Failed to lock DMA_GUC_WOPCM_OFFSET with %#x.\n",
+			  wopcm->guc.base);
 		goto err_out;
+	}
 
 	return 0;
 
 err_out:
-	DRM_ERROR("Failed to init WOPCM registers:\n");
 	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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] HAX enable guc for CI
  2018-03-20 23:18 [PATCH 1/2] drm/i915: Add code to accept valid locked WOPCM register values Jackie Li
@ 2018-03-20 23:18 ` Jackie Li
  2018-03-21  8:36 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Add code to accept valid locked WOPCM register values Patchwork
  2018-03-22 20:38 ` [PATCH 1/2] " Michał Winiarski
  2 siblings, 0 replies; 11+ messages in thread
From: Jackie Li @ 2018-03-20 23:18 UTC (permalink / raw)
  To: intel-gfx

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Add code to accept valid locked WOPCM register values
  2018-03-20 23:18 [PATCH 1/2] drm/i915: Add code to accept valid locked WOPCM register values Jackie Li
  2018-03-20 23:18 ` [PATCH 2/2] HAX enable guc for CI Jackie Li
@ 2018-03-21  8:36 ` Patchwork
  2018-03-22 20:38 ` [PATCH 1/2] " Michał Winiarski
  2 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2018-03-21  8:36 UTC (permalink / raw)
  To: Jackie Li; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/i915: Add code to accept valid locked WOPCM register values
URL   : https://patchwork.freedesktop.org/series/40334/
State : warning

== Summary ==

Series 40334v1 series starting with [1/2] drm/i915: Add code to accept valid locked WOPCM register values
https://patchwork.freedesktop.org/api/1.0/series/40334/revisions/1/mbox/

---- Possible new issues:

Test debugfs_test:
        Subgroup read_all_entries:
                pass       -> DMESG-WARN (fi-skl-6700hq)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> DMESG-WARN (fi-skl-6700hq)
        Subgroup basic-plain-flip:
                pass       -> DMESG-WARN (fi-skl-6700hq)

---- Known issues:

Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                pass       -> FAIL       (fi-gdg-551) fdo#102575
Test kms_busy:
        Subgroup basic-flip-c:
                pass       -> DMESG-WARN (fi-skl-6700hq) fdo#101144 +2
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                incomplete -> PASS       (fi-cfl-s2) fdo#105641

fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144
fdo#105641 https://bugs.freedesktop.org/show_bug.cgi?id=105641

fi-bdw-5557u     total:285  pass:264  dwarn:0   dfail:0   fail:0   skip:21  time:431s
fi-bdw-gvtdvm    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:438s
fi-blb-e6850     total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:380s
fi-bsw-n3050     total:285  pass:239  dwarn:0   dfail:0   fail:0   skip:46  time:543s
fi-bwr-2160      total:285  pass:180  dwarn:0   dfail:0   fail:0   skip:105 time:296s
fi-bxt-dsi       total:285  pass:255  dwarn:0   dfail:0   fail:0   skip:30  time:511s
fi-bxt-j4205     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:514s
fi-byt-j1900     total:285  pass:250  dwarn:0   dfail:0   fail:0   skip:35  time:521s
fi-byt-n2820     total:285  pass:246  dwarn:0   dfail:0   fail:0   skip:39  time:503s
fi-cfl-8700k     total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:412s
fi-cfl-s2        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:574s
fi-cfl-u         total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:512s
fi-cnl-drrs      total:285  pass:254  dwarn:3   dfail:0   fail:0   skip:28  time:525s
fi-elk-e7500     total:285  pass:225  dwarn:1   dfail:0   fail:0   skip:59  time:425s
fi-gdg-551       total:285  pass:176  dwarn:0   dfail:0   fail:1   skip:108 time:313s
fi-glk-1         total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:537s
fi-hsw-4770      total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:409s
fi-ilk-650       total:285  pass:225  dwarn:0   dfail:0   fail:0   skip:60  time:419s
fi-ivb-3520m     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:472s
fi-ivb-3770      total:285  pass:252  dwarn:0   dfail:0   fail:0   skip:33  time:442s
fi-kbl-7500u     total:285  pass:260  dwarn:1   dfail:0   fail:0   skip:24  time:482s
fi-kbl-7567u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:462s
fi-kbl-r         total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:511s
fi-pnv-d510      total:285  pass:219  dwarn:1   dfail:0   fail:0   skip:65  time:650s
fi-skl-6260u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:440s
fi-skl-6600u     total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:531s
fi-skl-6700hq    total:285  pass:253  dwarn:6   dfail:0   fail:0   skip:26  time:554s
fi-skl-6700k2    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:509s
fi-skl-6770hq    total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:501s
fi-skl-guc       total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:428s
fi-snb-2520m     total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:582s
fi-snb-2600      total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:404s

9d737cebc219c821989021a3115424165ff7b052 drm-tip: 2018y-03m-20d-14h-56m-05s UTC integration manifest
d2157a6a49e8 HAX enable guc for CI
044dd4569e9a drm/i915: Add code to accept valid locked WOPCM register values

== Logs ==

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] drm/i915: Add code to accept valid locked WOPCM register values
  2018-03-20 23:18 [PATCH 1/2] drm/i915: Add code to accept valid locked WOPCM register values Jackie Li
  2018-03-20 23:18 ` [PATCH 2/2] HAX enable guc for CI Jackie Li
  2018-03-21  8:36 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Add code to accept valid locked WOPCM register values Patchwork
@ 2018-03-22 20:38 ` Michał Winiarski
  2018-03-22 21:27   ` Yaodong Li
  2 siblings, 1 reply; 11+ messages in thread
From: Michał Winiarski @ 2018-03-22 20:38 UTC (permalink / raw)
  To: Jackie Li; +Cc: intel-gfx

On Tue, Mar 20, 2018 at 04:18:46PM -0700, Jackie Li wrote:
> 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.
> 
> 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: Michał Winiarski <michal.winiarski@intel.com>
> Cc: John Spotswood <john.a.spotswood@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_wopcm.c | 183 +++++++++++++++++++++++++++++++------
>  1 file changed, 157 insertions(+), 26 deletions(-)

[snip]

>  /**
>   * intel_wopcm_init() - Initialize the WOPCM structure.
>   * @wopcm: pointer to intel_wopcm.
> @@ -155,10 +202,8 @@ 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;
>  	int err;
>  
>  	GEM_BUG_ON(!wopcm->size);
> @@ -175,30 +220,17 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
>  		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;
> +	guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);

We already discussed this elsewhere, but just to have this part available
for wider audience:

huc_fw_size is unknown (there may be no huc firmware present) - which means
we're still not able to handle module reload from guc-without-huc into
guc-with-huc

Question remains whether we want to support this scenario, or whether we should
tie GuC and HuC together and refuse to operate early on if either one is not
present. We could remove a lot of code this way...

> +	err = calculate_max_guc_wopcm_size(wopcm, guc_wopcm_base,
> +					   &guc_wopcm_size);
> +	if (err)
> +		return err;
>  
>  	DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB, %uKiB)\n",
>  			 guc_wopcm_base / 1024, guc_wopcm_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(i915, guc_fw_size, huc_fw_size,
> +				       guc_wopcm_base, guc_wopcm_size);

Ok - so we're calculating the values in init...

>  	if (err)
>  		return err;
>  
> @@ -208,6 +240,92 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
>  	return 0;
>  }
>  

[snip]

>  static inline int write_and_verify(struct drm_i915_private *dev_priv,
>  				   i915_reg_t reg, u32 val, u32 mask,
>  				   u32 locked_bit)
> @@ -231,7 +349,8 @@ static inline int write_and_verify(struct drm_i915_private *dev_priv,
>   * will verify the register values to make sure the registers are locked with
>   * correct values.
>   *
> - * Return: 0 on success. -EIO if registers were locked with incorrect values.
> + * Return: 0 on success. Minus error code if registered were locked with
> + * incorrect values.-EIO if registers failed to lock with correct values.
>   */
>  int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
>  {
> @@ -247,27 +366,39 @@ int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
>  	GEM_BUG_ON(!wopcm->guc.size);
>  	GEM_BUG_ON(!wopcm->guc.base);
>  
> +	err = verify_locked_values_and_update(wopcm);
> +	if (err) {
> +		DRM_ERROR("WOPCM registers are locked with invalid values.\n");
> +		goto err_out;
> +	}
> +

...Only to throw everything out if the regs are locked in init_hw?

Since we've changed the logic, I think we should refactor a bit by changing
the ordering.
We could start from checking the regs and partition ourselves if the values are
not locked.

We can then have a common codepath for verification.

>  	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)
> +	if (err) {
> +		DRM_ERROR("Failed to lock GUC_WOPCM_SIZE with %#x.\n",
> +			  wopcm->guc.size);
>  		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,
>  			       GUC_WOPCM_OFFSET_VALID);
> -	if (err)
> +	if (err) {
> +		DRM_ERROR("Failed to lock DMA_GUC_WOPCM_OFFSET with %#x.\n",
> +			  wopcm->guc.base);
>  		goto err_out;
> +	}
>  
>  	return 0;
>  
>  err_out:
> -	DRM_ERROR("Failed to init WOPCM registers:\n");
>  	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");

Functionally I think it's a step in the right direction, but I think we should
try even harder to not ever display this error msg.

-Michał
>  
>  	return err;
>  }
> -- 
> 2.7.4
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] drm/i915: Add code to accept valid locked WOPCM register values
  2018-03-22 20:38 ` [PATCH 1/2] " Michał Winiarski
@ 2018-03-22 21:27   ` Yaodong Li
  2018-03-23 14:01     ` Michał Winiarski
  2018-03-23 14:02     ` Joonas Lahtinen
  0 siblings, 2 replies; 11+ messages in thread
From: Yaodong Li @ 2018-03-22 21:27 UTC (permalink / raw)
  To: Michał Winiarski; +Cc: intel-gfx

On 03/22/2018 01:38 PM, Michał Winiarski wrote:
> On Tue, Mar 20, 2018 at 04:18:46PM -0700, Jackie Li wrote:
>> 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.
>>
>> 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: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: John Spotswood <john.a.spotswood@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_wopcm.c | 183 +++++++++++++++++++++++++++++++------
>>   1 file changed, 157 insertions(+), 26 deletions(-)
> [snip]
>
>>   /**
>>    * intel_wopcm_init() - Initialize the WOPCM structure.
>>    * @wopcm: pointer to intel_wopcm.
>> @@ -155,10 +202,8 @@ 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;
>>   	int err;
>>   
>>   	GEM_BUG_ON(!wopcm->size);
>> @@ -175,30 +220,17 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
>>   		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;
>> +	guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);
> We already discussed this elsewhere, but just to have this part available
> for wider audience:
>
> huc_fw_size is unknown (there may be no huc firmware present) - which means
> we're still not able to handle module reload from guc-without-huc into
> guc-with-huc
>
> Question remains whether we want to support this scenario, or whether we should
> tie GuC and HuC together and refuse to operate early on if either one is not
> present. We could remove a lot of code this way...
Thanks. As we discussed I think we should describe this problem in this way:
we shall not make any assumption on either guc_fw_size and huc_fw_size.
On the other handle, we do need calculate the WOPCM layout based on
both GuC and HuC FW sizes, especially when we want to support modparam
enable_guc to enable/disable HuC FW loading dynamically. That's why I 
suggest
to update the FW fetch code to report the FW size no matter we turn the HuC
FW loading on or not.
Other aspect of the discussion is whether we should support enable_guc 
switching
from 1 to 3 + Adding new HuC FW to the filesystem without a system reboot.
In another word whether we should support FW image updates
(add/delete/update to a new version) in the filesystem without expecting 
a system
reboot. My point is it's unlikely we can support this since the FW sizes 
would likely
change and we need reconfigure the WO register with the latest FW 
available in
the FS, and I think it worth to do it to 100% make sure we won't run 
into any
module loading/init errors.
>
>> +	err = calculate_max_guc_wopcm_size(wopcm, guc_wopcm_base,
>> +					   &guc_wopcm_size);
>> +	if (err)
>> +		return err;
>>   
>>   	DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB, %uKiB)\n",
>>   			 guc_wopcm_base / 1024, guc_wopcm_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(i915, guc_fw_size, huc_fw_size,
>> +				       guc_wopcm_base, guc_wopcm_size);
> Ok - so we're calculating the values in init...
Most likely case, we are going to get these values ready without getting
any forcewake during the init. Then in init_hw we only need to update
these values to the register without wasting anytime on these calculation.
However, this patch introduce more code to handle the rare cases which
these regs were locked with different values from the ones we calculated
in the init phase. In this case, we will try to recalculate/verify these 
locked
or partially locked values and try our best to go with the locked values.
>
>>   	if (err)
>>   		return err;
>>   
>> @@ -208,6 +240,92 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
>>   	return 0;
>>   }
>>   
> [snip]
>
>>   static inline int write_and_verify(struct drm_i915_private *dev_priv,
>>   				   i915_reg_t reg, u32 val, u32 mask,
>>   				   u32 locked_bit)
>> @@ -231,7 +349,8 @@ static inline int write_and_verify(struct drm_i915_private *dev_priv,
>>    * will verify the register values to make sure the registers are locked with
>>    * correct values.
>>    *
>> - * Return: 0 on success. -EIO if registers were locked with incorrect values.
>> + * Return: 0 on success. Minus error code if registered were locked with
>> + * incorrect values.-EIO if registers failed to lock with correct values.
>>    */
>>   int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
>>   {
>> @@ -247,27 +366,39 @@ int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
>>   	GEM_BUG_ON(!wopcm->guc.size);
>>   	GEM_BUG_ON(!wopcm->guc.base);
>>   
>> +	err = verify_locked_values_and_update(wopcm);
>> +	if (err) {
>> +		DRM_ERROR("WOPCM registers are locked with invalid values.\n");
>> +		goto err_out;
>> +	}
>> +
> ...Only to throw everything out if the regs are locked in init_hw?
>
> Since we've changed the logic, I think we should refactor a bit by changing
> the ordering.
> We could start from checking the regs and partition ourselves if the values are
> not locked.
>
> We can then have a common codepath for verification.
We have power domain on at this point so we expect to spend as less
time as possible here. We are expecting the regs were not locked or
locked with the same values as we calculated (e.g. module reloading,
reset, etc) in most case. But for some rare cases (BIOS did upload the
regs) we will have this new path to handle those cases. But these are
all abnormal cases and even should not happen.
>
>>   	err = write_and_verify(dev_priv, GUC_WOPCM_SIZE, wopcm->guc.size,
>>   			       GUC_WOPCM_SIZE_MASK | GUC_WOPCM_SIZE_LOCKED,
>>   			       GUC_WOPCM_SIZE_LOCKED);
Actually, I think we shall decide whether updating the reg based on the
locking status. Will handle it in v2:)
>> -	if (err)
>> +	if (err) {
>> +		DRM_ERROR("Failed to lock GUC_WOPCM_SIZE with %#x.\n",
>> +			  wopcm->guc.size);
>>   		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,
>>   			       GUC_WOPCM_OFFSET_VALID);
>> -	if (err)
>> +	if (err) {
>> +		DRM_ERROR("Failed to lock DMA_GUC_WOPCM_OFFSET with %#x.\n",
>> +			  wopcm->guc.base);
>>   		goto err_out;
>> +	}
>>   
>>   	return 0;
>>   
>>   err_out:
>> -	DRM_ERROR("Failed to init WOPCM registers:\n");
>>   	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");
> Functionally I think it's a step in the right direction, but I think we should
> try even harder to not ever display this error msg.
It shall be very very unlikely if we had clear system configure and FW
update sequence. However, we are still expecting such an error. (e.g. 
Updated
FW images (with larger sizes) and try to load the new FW without a reboot.
we cannot 100% guarantee the correctness since the WOPCM layout is 
locked with
values we calculated based on the old FW status in the FS, thus we will 
report such an error
message.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] drm/i915: Add code to accept valid locked WOPCM register values
  2018-03-22 21:27   ` Yaodong Li
@ 2018-03-23 14:01     ` Michał Winiarski
  2018-03-23 17:33       ` Yaodong Li
  2018-03-23 14:02     ` Joonas Lahtinen
  1 sibling, 1 reply; 11+ messages in thread
From: Michał Winiarski @ 2018-03-23 14:01 UTC (permalink / raw)
  To: Yaodong Li; +Cc: intel-gfx

On Thu, Mar 22, 2018 at 02:27:59PM -0700, Yaodong Li wrote:
> On 03/22/2018 01:38 PM, Michał Winiarski wrote:
> > On Tue, Mar 20, 2018 at 04:18:46PM -0700, Jackie Li wrote:
> > > 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.
> > > 
> > > 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: Michał Winiarski <michal.winiarski@intel.com>
> > > Cc: John Spotswood <john.a.spotswood@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_wopcm.c | 183 +++++++++++++++++++++++++++++++------
> > >   1 file changed, 157 insertions(+), 26 deletions(-)
> > [snip]
> > 
> > >   /**
> > >    * intel_wopcm_init() - Initialize the WOPCM structure.
> > >    * @wopcm: pointer to intel_wopcm.
> > > @@ -155,10 +202,8 @@ 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;
> > >   	int err;
> > >   	GEM_BUG_ON(!wopcm->size);
> > > @@ -175,30 +220,17 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
> > >   		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;
> > > +	guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);
> > We already discussed this elsewhere, but just to have this part available
> > for wider audience:
> > 
> > huc_fw_size is unknown (there may be no huc firmware present) - which means
> > we're still not able to handle module reload from guc-without-huc into
> > guc-with-huc
> > 
> > Question remains whether we want to support this scenario, or whether we should
> > tie GuC and HuC together and refuse to operate early on if either one is not
> > present. We could remove a lot of code this way...
> Thanks. As we discussed I think we should describe this problem in this way:
> we shall not make any assumption on either guc_fw_size and huc_fw_size.

We can make assumptions on guc_fw_size - it's known in all the relevant cases
(enable_guc=1,2,3).
(unless we want to support varying guc_fw_size - more on that later).

> On the other handle, we do need calculate the WOPCM layout based on
> both GuC and HuC FW sizes, especially when we want to support modparam
> enable_guc to enable/disable HuC FW loading dynamically. That's why I
> suggest
> to update the FW fetch code to report the FW size no matter we turn the HuC
> FW loading on or not.

We could do that, but we don't really *need* to do that.
If user doesn't want HuC, why are we reading HuC from the filesystem? (well,
because we decided we're going to base our paritioning around it).

> Other aspect of the discussion is whether we should support enable_guc
> switching
> from 1 to 3 + Adding new HuC FW to the filesystem without a system reboot.
> In another word whether we should support FW image updates
> (add/delete/update to a new version) in the filesystem without expecting a
> system
> reboot. My point is it's unlikely we can support this since the FW sizes
> would likely
> change and we need reconfigure the WO register with the latest FW available
> in
> the FS, and I think it worth to do it to 100% make sure we won't run into
> any
> module loading/init errors.

It's never going to be 100%, but we can at least try to work with 99% ;)

The only reason why you need HuC size is because you're building the
partitioning from the bottom (starting from HuC).
We could just as well start from the top (starting from GuC).

The only thing you gain by starting from the bottom, is that you're much more
tolerant for variations in GuC fw size (because you're paritioning HuC region
precisely, leaving the rest for GuC. Well, that, and the HW workaround).
The only thing that changes when you're starting from the top, is that you're
going the other way around (more tolerant for HuC fw size variations).

I just posted a series that (among other things) tries to do partitioning from
the top.
To make it more tolerant for size variations, we could reserve a little extra
(either for GuC or HuC, depending on the approach).

-Michał

> > 
> > > +	err = calculate_max_guc_wopcm_size(wopcm, guc_wopcm_base,
> > > +					   &guc_wopcm_size);
> > > +	if (err)
> > > +		return err;
> > >   	DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB, %uKiB)\n",
> > >   			 guc_wopcm_base / 1024, guc_wopcm_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(i915, guc_fw_size, huc_fw_size,
> > > +				       guc_wopcm_base, guc_wopcm_size);
> > Ok - so we're calculating the values in init...
> Most likely case, we are going to get these values ready without getting
> any forcewake during the init. Then in init_hw we only need to update
> these values to the register without wasting anytime on these calculation.
> However, this patch introduce more code to handle the rare cases which
> these regs were locked with different values from the ones we calculated
> in the init phase. In this case, we will try to recalculate/verify these
> locked
> or partially locked values and try our best to go with the locked values.
> > 
> > >   	if (err)
> > >   		return err;
> > > @@ -208,6 +240,92 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
> > >   	return 0;
> > >   }
> > [snip]
> > 
> > >   static inline int write_and_verify(struct drm_i915_private *dev_priv,
> > >   				   i915_reg_t reg, u32 val, u32 mask,
> > >   				   u32 locked_bit)
> > > @@ -231,7 +349,8 @@ static inline int write_and_verify(struct drm_i915_private *dev_priv,
> > >    * will verify the register values to make sure the registers are locked with
> > >    * correct values.
> > >    *
> > > - * Return: 0 on success. -EIO if registers were locked with incorrect values.
> > > + * Return: 0 on success. Minus error code if registered were locked with
> > > + * incorrect values.-EIO if registers failed to lock with correct values.
> > >    */
> > >   int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
> > >   {
> > > @@ -247,27 +366,39 @@ int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
> > >   	GEM_BUG_ON(!wopcm->guc.size);
> > >   	GEM_BUG_ON(!wopcm->guc.base);
> > > +	err = verify_locked_values_and_update(wopcm);
> > > +	if (err) {
> > > +		DRM_ERROR("WOPCM registers are locked with invalid values.\n");
> > > +		goto err_out;
> > > +	}
> > > +
> > ...Only to throw everything out if the regs are locked in init_hw?
> > 
> > Since we've changed the logic, I think we should refactor a bit by changing
> > the ordering.
> > We could start from checking the regs and partition ourselves if the values are
> > not locked.
> > 
> > We can then have a common codepath for verification.
> We have power domain on at this point so we expect to spend as less
> time as possible here. We are expecting the regs were not locked or
> locked with the same values as we calculated (e.g. module reloading,
> reset, etc) in most case. But for some rare cases (BIOS did upload the
> regs) we will have this new path to handle those cases. But these are
> all abnormal cases and even should not happen.
> > 
> > >   	err = write_and_verify(dev_priv, GUC_WOPCM_SIZE, wopcm->guc.size,
> > >   			       GUC_WOPCM_SIZE_MASK | GUC_WOPCM_SIZE_LOCKED,
> > >   			       GUC_WOPCM_SIZE_LOCKED);
> Actually, I think we shall decide whether updating the reg based on the
> locking status. Will handle it in v2:)
> > > -	if (err)
> > > +	if (err) {
> > > +		DRM_ERROR("Failed to lock GUC_WOPCM_SIZE with %#x.\n",
> > > +			  wopcm->guc.size);
> > >   		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,
> > >   			       GUC_WOPCM_OFFSET_VALID);
> > > -	if (err)
> > > +	if (err) {
> > > +		DRM_ERROR("Failed to lock DMA_GUC_WOPCM_OFFSET with %#x.\n",
> > > +			  wopcm->guc.base);
> > >   		goto err_out;
> > > +	}
> > >   	return 0;
> > >   err_out:
> > > -	DRM_ERROR("Failed to init WOPCM registers:\n");
> > >   	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");
> > Functionally I think it's a step in the right direction, but I think we should
> > try even harder to not ever display this error msg.
> It shall be very very unlikely if we had clear system configure and FW
> update sequence. However, we are still expecting such an error. (e.g.
> Updated
> FW images (with larger sizes) and try to load the new FW without a reboot.
> we cannot 100% guarantee the correctness since the WOPCM layout is locked
> with
> values we calculated based on the old FW status in the FS, thus we will
> report such an error
> message.
> 
> Regards,
> -Jackie
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] drm/i915: Add code to accept valid locked WOPCM register values
  2018-03-22 21:27   ` Yaodong Li
  2018-03-23 14:01     ` Michał Winiarski
@ 2018-03-23 14:02     ` Joonas Lahtinen
  2018-03-23 14:38       ` Michał Winiarski
  1 sibling, 1 reply; 11+ messages in thread
From: Joonas Lahtinen @ 2018-03-23 14:02 UTC (permalink / raw)
  To: Michał Winiarski, Yaodong Li; +Cc: intel-gfx

Quoting Yaodong Li (2018-03-22 23:27:59)
> On 03/22/2018 01:38 PM, Michał Winiarski wrote:
> > On Tue, Mar 20, 2018 at 04:18:46PM -0700, Jackie Li wrote:
> >> @@ -175,30 +220,17 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
> >>              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;
> >> +    guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);
> > We already discussed this elsewhere, but just to have this part available
> > for wider audience:
> >
> > huc_fw_size is unknown (there may be no huc firmware present) - which means
> > we're still not able to handle module reload from guc-without-huc into
> > guc-with-huc
> >
> > Question remains whether we want to support this scenario, or whether we should
> > tie GuC and HuC together and refuse to operate early on if either one is not
> > present. We could remove a lot of code this way...
> Thanks. As we discussed I think we should describe this problem in this way:
> we shall not make any assumption on either guc_fw_size and huc_fw_size.
> On the other handle, we do need calculate the WOPCM layout based on
> both GuC and HuC FW sizes, especially when we want to support modparam
> enable_guc to enable/disable HuC FW loading dynamically. That's why I 
> suggest
> to update the FW fetch code to report the FW size no matter we turn the HuC
> FW loading on or not.
> Other aspect of the discussion is whether we should support enable_guc 
> switching
> from 1 to 3 + Adding new HuC FW to the filesystem without a system reboot.
> In another word whether we should support FW image updates
> (add/delete/update to a new version) in the filesystem without expecting 
> a system
> reboot. My point is it's unlikely we can support this since the FW sizes 
> would likely
> change and we need reconfigure the WO register with the latest FW 
> available in
> the FS, and I think it worth to do it to 100% make sure we won't run 
> into any
> module loading/init errors.

Did we ask the HW team about this? Surely the intention can't be to both
support dynamic enable/disable but yet having a write-once variable holding
the configuration?

Also, somebody mentioned that these were locked already before loading
i915, so if the firmware sizes have to do with the configuration, how
could that be? We'd have to match BIOS/whatever firmware, and that's not
likely to happen, is it.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] drm/i915: Add code to accept valid locked WOPCM register values
  2018-03-23 14:02     ` Joonas Lahtinen
@ 2018-03-23 14:38       ` Michał Winiarski
  0 siblings, 0 replies; 11+ messages in thread
From: Michał Winiarski @ 2018-03-23 14:38 UTC (permalink / raw)
  To: Joonas Lahtinen; +Cc: intel-gfx

On Fri, Mar 23, 2018 at 04:02:47PM +0200, Joonas Lahtinen wrote:
> Quoting Yaodong Li (2018-03-22 23:27:59)
> > On 03/22/2018 01:38 PM, Michał Winiarski wrote:
> > > On Tue, Mar 20, 2018 at 04:18:46PM -0700, Jackie Li wrote:
> > >> @@ -175,30 +220,17 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
> > >>              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;
> > >> +    guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);
> > > We already discussed this elsewhere, but just to have this part available
> > > for wider audience:
> > >
> > > huc_fw_size is unknown (there may be no huc firmware present) - which means
> > > we're still not able to handle module reload from guc-without-huc into
> > > guc-with-huc
> > >
> > > Question remains whether we want to support this scenario, or whether we should
> > > tie GuC and HuC together and refuse to operate early on if either one is not
> > > present. We could remove a lot of code this way...
> > Thanks. As we discussed I think we should describe this problem in this way:
> > we shall not make any assumption on either guc_fw_size and huc_fw_size.
> > On the other handle, we do need calculate the WOPCM layout based on
> > both GuC and HuC FW sizes, especially when we want to support modparam
> > enable_guc to enable/disable HuC FW loading dynamically. That's why I 
> > suggest
> > to update the FW fetch code to report the FW size no matter we turn the HuC
> > FW loading on or not.
> > Other aspect of the discussion is whether we should support enable_guc 
> > switching
> > from 1 to 3 + Adding new HuC FW to the filesystem without a system reboot.
> > In another word whether we should support FW image updates
> > (add/delete/update to a new version) in the filesystem without expecting 
> > a system
> > reboot. My point is it's unlikely we can support this since the FW sizes 
> > would likely
> > change and we need reconfigure the WO register with the latest FW 
> > available in
> > the FS, and I think it worth to do it to 100% make sure we won't run 
> > into any
> > module loading/init errors.
> 
> Did we ask the HW team about this? Surely the intention can't be to both
> support dynamic enable/disable but yet having a write-once variable holding
> the configuration?
> 
> Also, somebody mentioned that these were locked already before loading
> i915, so if the firmware sizes have to do with the configuration, how
> could that be? We'd have to match BIOS/whatever firmware, and that's not
> likely to happen, is it.

That was me, and that was an error in my environment. We expect to see unlocked
regs on reboot. And even if we don't (e.g. because it's a module reload), trying
to fit isn't really that hard to do (we're reusing most of the code), so I
believe this is the right approach.

-Michał
 
> Regards, Joonas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] drm/i915: Add code to accept valid locked WOPCM register values
  2018-03-23 14:01     ` Michał Winiarski
@ 2018-03-23 17:33       ` Yaodong Li
  2018-03-26  7:15         ` Joonas Lahtinen
  0 siblings, 1 reply; 11+ messages in thread
From: Yaodong Li @ 2018-03-23 17:33 UTC (permalink / raw)
  To: Michał Winiarski; +Cc: intel-gfx

On 03/23/2018 07:01 AM, Michał Winiarski wrote:
> On Thu, Mar 22, 2018 at 02:27:59PM -0700, Yaodong Li wrote:
>> On 03/22/2018 01:38 PM, Michał Winiarski wrote:
>>> On Tue, Mar 20, 2018 at 04:18:46PM -0700, Jackie Li wrote:
>>>> 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.
>>>>
>>>> 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: Michał Winiarski <michal.winiarski@intel.com>
>>>> Cc: John Spotswood <john.a.spotswood@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_wopcm.c | 183 +++++++++++++++++++++++++++++++------
>>>>    1 file changed, 157 insertions(+), 26 deletions(-)
>>> [snip]
>>>
>>>>    /**
>>>>     * intel_wopcm_init() - Initialize the WOPCM structure.
>>>>     * @wopcm: pointer to intel_wopcm.
>>>> @@ -155,10 +202,8 @@ 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;
>>>>    	int err;
>>>>    	GEM_BUG_ON(!wopcm->size);
>>>> @@ -175,30 +220,17 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
>>>>    		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;
>>>> +	guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);
>>> We already discussed this elsewhere, but just to have this part available
>>> for wider audience:
>>>
>>> huc_fw_size is unknown (there may be no huc firmware present) - which means
>>> we're still not able to handle module reload from guc-without-huc into
>>> guc-with-huc
>>>
>>> Question remains whether we want to support this scenario, or whether we should
>>> tie GuC and HuC together and refuse to operate early on if either one is not
>>> present. We could remove a lot of code this way...
>> Thanks. As we discussed I think we should describe this problem in this way:
>> we shall not make any assumption on either guc_fw_size and huc_fw_size.
> We can make assumptions on guc_fw_size - it's known in all the relevant cases
> (enable_guc=1,2,3).
> (unless we want to support varying guc_fw_size - more on that later).
What I meant is we cannot make any assumption one the actual size
of guc and huc fw:)
>> On the other handle, we do need calculate the WOPCM layout based on
>> both GuC and HuC FW sizes, especially when we want to support modparam
>> enable_guc to enable/disable HuC FW loading dynamically. That's why I
>> suggest
>> to update the FW fetch code to report the FW size no matter we turn the HuC
>> FW loading on or not.
> We could do that, but we don't really *need* to do that.
> If user doesn't want HuC, why are we reading HuC from the filesystem? (well,
> because we decided we're going to base our paritioning around it).
Because of the enable_guc=1 and enable_guc=3 case and the fact
that we only can write to these registers for once + we need to know
the size to 100% make sure we do the partitioning correctly.
please refer to my patch set: 
https://patchwork.freedesktop.org/series/40541/
>> Other aspect of the discussion is whether we should support enable_guc
>> switching
>> from 1 to 3 + Adding new HuC FW to the filesystem without a system reboot.
>> In another word whether we should support FW image updates
>> (add/delete/update to a new version) in the filesystem without expecting a
>> system
>> reboot. My point is it's unlikely we can support this since the FW sizes
>> would likely
>> change and we need reconfigure the WO register with the latest FW available
>> in
>> the FS, and I think it worth to do it to 100% make sure we won't run into
>> any
>> module loading/init errors.
> It's never going to be 100%, but we can at least try to work with 99% ;)
With correct FW images, it would be 100%, isn't it?:)
> The only reason why you need HuC size is because you're building the
> partitioning from the bottom (starting from HuC).
> We could just as well start from the top (starting from GuC).
That's right. but either starting from the bottom (based on HuC FW size) or
starting from the top (based on GuC FW size), we are ignoring one fact 
is that
the final layout is depended on both GuC and HuC FW sizes.
> The only thing you gain by starting from the bottom, is that you're much more
> tolerant for variations in GuC fw size (because you're paritioning HuC region
> precisely, leaving the rest for GuC. Well, that, and the HW workaround).
We would guarantee we would have maximum guc wopcm size in this
way which will be the most likely way to meet current hw limitations.
and more important it's simper and easier:)
> The only thing that changes when you're starting from the top, is that you're
> going the other way around (more tolerant for HuC fw size variations).
As I said, I agree that we would likely solve the enable_guc=1 then
enable_guc=3 case with these changes which I think this the the only benefit
that we can get from the starting from the top way.
But my point is just like the from the bottom way, you are ignoring the
HuC FW size. e.g. you will need to grow the guc wopcm size for the hw 
restrictions.
The problem is currently we do have this restriction on huc fw size v.s. 
guc wopcm
size. In you solution, you are actually counting on the assumption that
guc fw size should be large enough so that we can ignore the huc fw size
and expect it still works when we set enable_huc=3. and my answer to
this is yes it will work for the cases that guc fw size is large enough, but
still risky to fail in case of guc fw size < huc_fw_size + 16K. and that 
comes
to my point:
why not make life easier and accurate - If we decide to support dynamically
switching on/off huc fw loading and the fact we can get actual FW sizes,
why not drop all these assumptions and fix it in a way that we won't be
bothered by any FW side changes? :)

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] drm/i915: Add code to accept valid locked WOPCM register values
  2018-03-23 17:33       ` Yaodong Li
@ 2018-03-26  7:15         ` Joonas Lahtinen
  2018-04-03  0:08           ` Yaodong Li
  0 siblings, 1 reply; 11+ messages in thread
From: Joonas Lahtinen @ 2018-03-26  7:15 UTC (permalink / raw)
  To: Michał Winiarski, Yaodong Li; +Cc: intel-gfx

Quoting Yaodong Li (2018-03-23 19:33:15)
> As I said, I agree that we would likely solve the enable_guc=1 then
> enable_guc=3 case with these changes which I think this the the only benefit
> that we can get from the starting from the top way.
> But my point is just like the from the bottom way, you are ignoring the
> HuC FW size. e.g. you will need to grow the guc wopcm size for the hw 
> restrictions.
> The problem is currently we do have this restriction on huc fw size v.s. 
> guc wopcm
> size. In you solution, you are actually counting on the assumption that
> guc fw size should be large enough so that we can ignore the huc fw size
> and expect it still works when we set enable_huc=3. and my answer to
> this is yes it will work for the cases that guc fw size is large enough, but
> still risky to fail in case of guc fw size < huc_fw_size + 16K. and that 
> comes
> to my point:
> why not make life easier and accurate - If we decide to support dynamically
> switching on/off huc fw loading and the fact we can get actual FW sizes,
> why not drop all these assumptions and fix it in a way that we won't be
> bothered by any FW side changes? :)

Is not the GuC vs. HuC FW size limitation going to be fixed for upcoming
platforms?

My gut instinct would be to partition based only on the enabled FW sizes,
whichever it be. Then just require a reboot if after that partitioning,
changing the parameter causes the FW not to fit.

Loading the FW, and writing its size to HW registers when there is a
kernel command line parameter to disable its use, seems unexpected.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] drm/i915: Add code to accept valid locked WOPCM register values
  2018-03-26  7:15         ` Joonas Lahtinen
@ 2018-04-03  0:08           ` Yaodong Li
  0 siblings, 0 replies; 11+ messages in thread
From: Yaodong Li @ 2018-04-03  0:08 UTC (permalink / raw)
  To: Joonas Lahtinen, Michał Winiarski; +Cc: intel-gfx

On 03/26/2018 12:15 AM, Joonas Lahtinen wrote:
> Quoting Yaodong Li (2018-03-23 19:33:15)
>> As I said, I agree that we would likely solve the enable_guc=1 then
>> enable_guc=3 case with these changes which I think this the the only benefit
>> that we can get from the starting from the top way.
>> But my point is just like the from the bottom way, you are ignoring the
>> HuC FW size. e.g. you will need to grow the guc wopcm size for the hw
>> restrictions.
>> The problem is currently we do have this restriction on huc fw size v.s.
>> guc wopcm
>> size. In you solution, you are actually counting on the assumption that
>> guc fw size should be large enough so that we can ignore the huc fw size
>> and expect it still works when we set enable_huc=3. and my answer to
>> this is yes it will work for the cases that guc fw size is large enough, but
>> still risky to fail in case of guc fw size < huc_fw_size + 16K. and that
>> comes
>> to my point:
>> why not make life easier and accurate - If we decide to support dynamically
>> switching on/off huc fw loading and the fact we can get actual FW sizes,
>> why not drop all these assumptions and fix it in a way that we won't be
>> bothered by any FW side changes? :)
> Is not the GuC vs. HuC FW size limitation going to be fixed for upcoming
> platforms?
>
> My gut instinct would be to partition based only on the enabled FW sizes,
> whichever it be. Then just require a reboot if after that partitioning,
> changing the parameter causes the FW not to fit.
That's my thought too. I think it makes sense to reboot the system for
any FW updates, especially when we have these write-once registers.

I believe the main reason we wanted to support enable_guc=1 (GuC FW only)
then enable_guc=3 (GuC + HuC FW) without a system reboot is to facilitate
the debugging.

Regards,
-Jackie

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-04-03  0:11 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-20 23:18 [PATCH 1/2] drm/i915: Add code to accept valid locked WOPCM register values Jackie Li
2018-03-20 23:18 ` [PATCH 2/2] HAX enable guc for CI Jackie Li
2018-03-21  8:36 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Add code to accept valid locked WOPCM register values Patchwork
2018-03-22 20:38 ` [PATCH 1/2] " Michał Winiarski
2018-03-22 21:27   ` Yaodong Li
2018-03-23 14:01     ` Michał Winiarski
2018-03-23 17:33       ` Yaodong Li
2018-03-26  7:15         ` Joonas Lahtinen
2018-04-03  0:08           ` Yaodong Li
2018-03-23 14:02     ` Joonas Lahtinen
2018-03-23 14:38       ` Michał Winiarski

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