[PATCH v10 1/7] drm/i915/guc: Move GuC WOPCM related code into separate files

[PATCH v10 1/7] drm/i915/guc: Move GuC WOPCM related code into separate files

[Jackie Li]

intel_guc_reg.h should only include definition for GuC registers and
related register bits. Non-register related GuC WOPCM macro definitions
should not be defined in intel_guc_reg.h.

This patch cleans up intel_guc_reg.h and moves GuC WOPCM related code into
new created separate files as future patches will increase the complexity
of determining the GuC WOPCM offset and size.

v8:
 - Fixed naming, coding style issues and typo in commit message (Sagar)
 - Updated commit message to explain why we need create new file for GuC
   WOPCM related code (Chris)

v9:
 - Corrected kernel-doc format (Sagar)
 - Simplified commit message (Michal)
 - Updated license of new created files to use SPDX (Michal)
 - Updated macros and added more comments to be reader friendly (Michal)

v10:
 - Updated license to just use SPDX (Michal/Michel)

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> (v8)
Signed-off-by: Jackie Li <yaodong.li@intel.com>
---
 drivers/gpu/drm/i915/Makefile          |  1 +
 drivers/gpu/drm/i915/intel_guc.c       | 11 -----------
 drivers/gpu/drm/i915/intel_guc.h       |  2 +-
 drivers/gpu/drm/i915/intel_guc_reg.h   |  4 ----
 drivers/gpu/drm/i915/intel_guc_wopcm.c | 28 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc_wopcm.h | 25 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.c        |  2 +-
 drivers/gpu/drm/i915/intel_uc_fw.c     |  2 +-
 8 files changed, 57 insertions(+), 18 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_guc_wopcm.c
 create mode 100644 drivers/gpu/drm/i915/intel_guc_wopcm.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index f55cc02..418d96b 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -89,6 +89,7 @@ i915-y += intel_uc.o \
 	  intel_guc_fw.o \
 	  intel_guc_log.o \
 	  intel_guc_submission.o \
+	  intel_guc_wopcm.o \
 	  intel_huc.o
 
 # autogenerated null render state
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 21140cc..9f45e6d 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -509,14 +509,3 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
 	i915_gem_object_put(obj);
 	return vma;
 }
-
-u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv)
-{
-	u32 wopcm_size = GUC_WOPCM_TOP;
-
-	/* On BXT, the top of WOPCM is reserved for RC6 context */
-	if (IS_GEN9_LP(dev_priv))
-		wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED;
-
-	return wopcm_size;
-}
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 52856a9..9e0a97e 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -31,6 +31,7 @@
 #include "intel_guc_ct.h"
 #include "intel_guc_log.h"
 #include "intel_guc_reg.h"
+#include "intel_guc_wopcm.h"
 #include "intel_uc_fw.h"
 #include "i915_vma.h"
 
@@ -130,6 +131,5 @@ int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
 int intel_guc_suspend(struct drm_i915_private *dev_priv);
 int intel_guc_resume(struct drm_i915_private *dev_priv);
 struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
-u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_guc_reg.h b/drivers/gpu/drm/i915/intel_guc_reg.h
index 19a9247..1f52fb8 100644
--- a/drivers/gpu/drm/i915/intel_guc_reg.h
+++ b/drivers/gpu/drm/i915/intel_guc_reg.h
@@ -68,7 +68,6 @@
 #define DMA_GUC_WOPCM_OFFSET		_MMIO(0xc340)
 #define   HUC_LOADING_AGENT_VCR		  (0<<1)
 #define   HUC_LOADING_AGENT_GUC		  (1<<1)
-#define   GUC_WOPCM_OFFSET_VALUE	  0x80000	/* 512KB */
 #define GUC_MAX_IDLE_COUNT		_MMIO(0xC3E4)
 
 #define HUC_STATUS2             _MMIO(0xD3B0)
@@ -76,9 +75,6 @@
 
 /* Defines WOPCM space available to GuC firmware */
 #define GUC_WOPCM_SIZE			_MMIO(0xc050)
-/* GuC addresses below GUC_WOPCM_TOP don't map through the GTT */
-#define   GUC_WOPCM_TOP			  (0x80 << 12)	/* 512KB */
-#define   BXT_GUC_WOPCM_RC6_RESERVED	  (0x10 << 12)	/* 64KB  */
 
 /* GuC addresses above GUC_GGTT_TOP also don't map through the GTT */
 #define GUC_GGTT_TOP			0xFEE00000
diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.c b/drivers/gpu/drm/i915/intel_guc_wopcm.c
new file mode 100644
index 0000000..3972901
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
@@ -0,0 +1,28 @@
+/*
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright © 2017-2018 Intel Corporation
+ */
+
+#include "intel_guc_wopcm.h"
+#include "i915_drv.h"
+
+/**
+ * intel_guc_wopcm_size() - Get the size of GuC WOPCM.
+ * @guc: intel guc.
+ *
+ * Get the platform specific GuC WOPCM size.
+ *
+ * Return: size of the GuC WOPCM.
+ */
+u32 intel_guc_wopcm_size(struct intel_guc *guc)
+{
+	struct drm_i915_private *i915 = guc_to_i915(guc);
+	u32 size = GUC_WOPCM_TOP;
+
+	/* On BXT, the top of WOPCM is reserved for RC6 context */
+	if (IS_GEN9_LP(i915))
+		size -= BXT_GUC_WOPCM_RC6_RESERVED;
+
+	return size;
+}
diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.h b/drivers/gpu/drm/i915/intel_guc_wopcm.h
new file mode 100644
index 0000000..8c4f693
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.h
@@ -0,0 +1,25 @@
+/*
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright © 2017-2018 Intel Corporation
+ */
+
+#ifndef _INTEL_GUC_WOPCM_H_
+#define _INTEL_GUC_WOPCM_H_
+
+#include <linux/types.h>
+
+struct intel_guc;
+
+/* 512KB static offset from WOPCM base. */
+#define GUC_WOPCM_OFFSET_VALUE		(512 << 10)
+/*
+ * 512KB static GuC WOPCM size from GUC_WOPCM_OFFSET_VALUE to the end of GuC
+ * WOPCM. GuC addresses below GUC_WOPCM_TOP don't map through the GTT.
+ */
+#define GUC_WOPCM_TOP			(512 << 10)
+#define BXT_GUC_WOPCM_RC6_RESERVED	(64 << 10)
+
+u32 intel_guc_wopcm_size(struct intel_guc *guc);
+
+#endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 9f1bac6..1b2831b 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -340,7 +340,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	gen9_reset_guc_interrupts(dev_priv);
 
 	/* init WOPCM */
-	I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(dev_priv));
+	I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(guc));
 	I915_WRITE(DMA_GUC_WOPCM_OFFSET,
 		   GUC_WOPCM_OFFSET_VALUE | HUC_LOADING_AGENT_GUC);
 
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
index 784eff9..24945cf 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/intel_uc_fw.c
@@ -97,7 +97,7 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 
 	/* Header and uCode will be loaded to WOPCM */
 	size = uc_fw->header_size + uc_fw->ucode_size;
-	if (size > intel_guc_wopcm_size(dev_priv)) {
+	if (size > intel_guc_wopcm_size(&dev_priv->guc)) {
 		DRM_WARN("%s: Firmware is too large to fit in WOPCM\n",
 			 intel_uc_fw_type_repr(uc_fw->type));
 		err = -E2BIG;
-- 
2.7.4

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

[PATCH v10 2/7] drm/i915/guc: Rename guc_ggtt_offset to intel_guc_ggtt_offset

[Jackie Li]

GuC related exported functions should start with "intel_guc_" prefix and
pass intel_guc as the first parameter since its guc related. Current
guc_ggtt_offset() failed to follow this code convention and this is a
problem for future patches that needs to access intel_guc data to verify
the GGTT offset against the GuC WOPCM top.

This patch renames the guc_ggtt_offset to intel_guc_ggtt_offset and updates
the related code to pass intel_guc pointer to this function call, so that
we can have a unified coding style for GuC code and also enable the future
patches to get GuC related data from intel_guc to do the offset
verification. Meanwhile, this patch also moves the GUC_GGTT_TOP from
intel_guc_regs.h to intel_guc.h since it is not GuC register related
definition.

v8:
 - Fixed coding style issues and moved GUC_GGTT_TOP to intel_guc.h (Sagar)
 - Updated commit message to explain to reason and motivation to add
   intel_guc as the first parameter of intel_guc_ggtt_offset (Chris)

v9:
 - Fixed code alignment issue due to line break (Chris)

v10:
 - Removed unnecessary comments, redundant code and avoided reuse variable
   to avoid potential issues (Joonas)

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> (v8)
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> (v9)
Signed-off-by: Jackie Li <yaodong.li@intel.com>
---
 drivers/gpu/drm/i915/intel_guc.c            | 11 ++++++-----
 drivers/gpu/drm/i915/intel_guc.h            | 14 ++++++++++++--
 drivers/gpu/drm/i915/intel_guc_ads.c        | 27 ++++++++++++++-------------
 drivers/gpu/drm/i915/intel_guc_ct.c         |  5 +++--
 drivers/gpu/drm/i915/intel_guc_fw.c         |  2 +-
 drivers/gpu/drm/i915/intel_guc_log.c        |  2 +-
 drivers/gpu/drm/i915/intel_guc_reg.h        |  3 ---
 drivers/gpu/drm/i915/intel_guc_submission.c | 10 +++++-----
 drivers/gpu/drm/i915/intel_huc.c            |  6 ++++--
 9 files changed, 46 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 9f45e6d..fdf8cb3 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -269,8 +269,9 @@ void intel_guc_init_params(struct intel_guc *guc)
 
 	/* If GuC submission is enabled, set up additional parameters here */
 	if (USES_GUC_SUBMISSION(dev_priv)) {
-		u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
-		u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool);
+		u32 ads = intel_guc_ggtt_offset(guc,
+						guc->ads_vma) >> PAGE_SHIFT;
+		u32 pgs = intel_guc_ggtt_offset(guc, guc->stage_desc_pool);
 		u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
 
 		params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT;
@@ -418,7 +419,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
 	data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
 	/* any value greater than GUC_POWER_D0 */
 	data[1] = GUC_POWER_D1;
-	data[2] = guc_ggtt_offset(guc->shared_data);
+	data[2] = intel_guc_ggtt_offset(guc, guc->shared_data);
 
 	return intel_guc_send(guc, data, ARRAY_SIZE(data));
 }
@@ -441,7 +442,7 @@ int intel_guc_reset_engine(struct intel_guc *guc,
 	data[3] = 0;
 	data[4] = 0;
 	data[5] = guc->execbuf_client->stage_id;
-	data[6] = guc_ggtt_offset(guc->shared_data);
+	data[6] = intel_guc_ggtt_offset(guc, guc->shared_data);
 
 	return intel_guc_send(guc, data, ARRAY_SIZE(data));
 }
@@ -463,7 +464,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
 
 	data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
 	data[1] = GUC_POWER_D0;
-	data[2] = guc_ggtt_offset(guc->shared_data);
+	data[2] = intel_guc_ggtt_offset(guc, guc->shared_data);
 
 	return intel_guc_send(guc, data, ARRAY_SIZE(data));
 }
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 9e0a97e..50be6de 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -101,13 +101,23 @@ static inline void intel_guc_notify(struct intel_guc *guc)
 	guc->notify(guc);
 }
 
-/*
+/* GuC addresses above GUC_GGTT_TOP also don't map through the GTT */
+#define GUC_GGTT_TOP	0xFEE00000
+
+/**
+ * intel_guc_ggtt_offset() - Get and validate the GGTT offset of @vma
+ * @guc: intel guc.
+ * @vma: i915 graphics virtual memory area.
+ *
  * GuC does not allow any gfx GGTT address that falls into range [0, WOPCM_TOP),
  * which is reserved for Boot ROM, SRAM and WOPCM. Currently this top address is
  * 512K. In order to exclude 0-512K address space from GGTT, all gfx objects
  * used by GuC is pinned with PIN_OFFSET_BIAS along with size of WOPCM.
+ *
+ * Return: GGTT offset that meets the GuC gfx address requirement.
  */
-static inline u32 guc_ggtt_offset(struct i915_vma *vma)
+static inline u32 intel_guc_ggtt_offset(struct intel_guc *guc,
+					struct i915_vma *vma)
 {
 	u32 offset = i915_ggtt_offset(vma);
 
diff --git a/drivers/gpu/drm/i915/intel_guc_ads.c b/drivers/gpu/drm/i915/intel_guc_ads.c
index ac62753..b4b18ce 100644
--- a/drivers/gpu/drm/i915/intel_guc_ads.c
+++ b/drivers/gpu/drm/i915/intel_guc_ads.c
@@ -75,7 +75,7 @@ static void guc_policies_init(struct guc_policies *policies)
 int intel_guc_ads_create(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	struct i915_vma *vma;
+	struct i915_vma *vma, *kernel_ctx_vma;
 	struct page *page;
 	/* The ads obj includes the struct itself and buffers passed to GuC */
 	struct {
@@ -113,17 +113,6 @@ int intel_guc_ads_create(struct intel_guc *guc)
 		blob->reg_state.white_list[engine->guc_id].count = 0;
 	}
 
-	/*
-	 * The GuC requires a "Golden Context" when it reinitialises
-	 * engines after a reset. Here we use the Render ring default
-	 * context, which must already exist and be pinned in the GGTT,
-	 * so its address won't change after we've told the GuC where
-	 * to find it. Note that we have to skip our header (1 page),
-	 * because our GuC shared data is there.
-	 */
-	blob->ads.golden_context_lrca =
-		guc_ggtt_offset(dev_priv->kernel_context->engine[RCS].state) +
-		skipped_offset;
 
 	/*
 	 * The GuC expects us to exclude the portion of the context image that
@@ -135,11 +124,23 @@ int intel_guc_ads_create(struct intel_guc *guc)
 		blob->ads.eng_state_size[engine->guc_id] =
 			engine->context_size - skipped_size;
 
-	base = guc_ggtt_offset(vma);
+	base = intel_guc_ggtt_offset(guc, vma);
 	blob->ads.scheduler_policies = base + ptr_offset(blob, policies);
 	blob->ads.reg_state_buffer = base + ptr_offset(blob, reg_state_buffer);
 	blob->ads.reg_state_addr = base + ptr_offset(blob, reg_state);
 
+	/*
+	 * The GuC requires a "Golden Context" when it reinitialises
+	 * engines after a reset. Here we use the Render ring default
+	 * context, which must already exist and be pinned in the GGTT,
+	 * so its address won't change after we've told the GuC where
+	 * to find it. Note that we have to skip our header (1 page),
+	 * because our GuC shared data is there.
+	 */
+	kernel_ctx_vma = dev_priv->kernel_context->engine[RCS].state;
+	blob->ads.golden_context_lrca =
+		intel_guc_ggtt_offset(guc, kernel_ctx_vma) + skipped_offset;
+
 	kunmap(page);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
index 24ad557..0a0d3d5 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -156,7 +156,8 @@ static int ctch_init(struct intel_guc *guc,
 		err = PTR_ERR(blob);
 		goto err_vma;
 	}
-	DRM_DEBUG_DRIVER("CT: vma base=%#x\n", guc_ggtt_offset(ctch->vma));
+	DRM_DEBUG_DRIVER("CT: vma base=%#x\n",
+			 intel_guc_ggtt_offset(guc, ctch->vma));
 
 	/* store pointers to desc and cmds */
 	for (i = 0; i < ARRAY_SIZE(ctch->ctbs); i++) {
@@ -202,7 +203,7 @@ static int ctch_open(struct intel_guc *guc,
 	}
 
 	/* vma should be already allocated and map'ed */
-	base = guc_ggtt_offset(ctch->vma);
+	base = intel_guc_ggtt_offset(guc, ctch->vma);
 
 	/* (re)initialize descriptors
 	 * cmds buffers are in the second half of the blob page
diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
index 3b09329..178d339 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/intel_guc_fw.c
@@ -165,7 +165,7 @@ static int guc_xfer_ucode(struct intel_guc *guc, struct i915_vma *vma)
 	I915_WRITE(DMA_COPY_SIZE, guc_fw->header_size + guc_fw->ucode_size);
 
 	/* Set the source address for the new blob */
-	offset = guc_ggtt_offset(vma) + guc_fw->header_offset;
+	offset = intel_guc_ggtt_offset(guc, vma) + guc_fw->header_offset;
 	I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset));
 	I915_WRITE(DMA_ADDR_0_HIGH, upper_32_bits(offset) & 0xFFFF);
 
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 7b5074e..33636de 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -638,7 +638,7 @@ int intel_guc_log_create(struct intel_guc *guc)
 		(GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
 		(GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
 
-	offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
+	offset = intel_guc_ggtt_offset(guc, vma) >> PAGE_SHIFT;
 	guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_guc_reg.h b/drivers/gpu/drm/i915/intel_guc_reg.h
index 1f52fb8..de4f78b 100644
--- a/drivers/gpu/drm/i915/intel_guc_reg.h
+++ b/drivers/gpu/drm/i915/intel_guc_reg.h
@@ -76,9 +76,6 @@
 /* Defines WOPCM space available to GuC firmware */
 #define GUC_WOPCM_SIZE			_MMIO(0xc050)
 
-/* GuC addresses above GUC_GGTT_TOP also don't map through the GTT */
-#define GUC_GGTT_TOP			0xFEE00000
-
 #define GEN8_GT_PM_CONFIG		_MMIO(0x138140)
 #define GEN9LP_GT_PM_CONFIG		_MMIO(0x138140)
 #define GEN9_GT_PM_CONFIG		_MMIO(0x13816c)
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index b43b58c..ebfcb9f 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -375,8 +375,8 @@ static void guc_stage_desc_init(struct intel_guc *guc,
 		lrc->context_desc = lower_32_bits(ce->lrc_desc);
 
 		/* The state page is after PPHWSP */
-		lrc->ring_lrca =
-			guc_ggtt_offset(ce->state) + LRC_STATE_PN * PAGE_SIZE;
+		lrc->ring_lrca = intel_guc_ggtt_offset(guc, ce->state) +
+				 LRC_STATE_PN * PAGE_SIZE;
 
 		/* XXX: In direct submission, the GuC wants the HW context id
 		 * here. In proxy submission, it wants the stage id
@@ -384,7 +384,7 @@ static void guc_stage_desc_init(struct intel_guc *guc,
 		lrc->context_id = (client->stage_id << GUC_ELC_CTXID_OFFSET) |
 				(guc_engine_id << GUC_ELC_ENGINE_OFFSET);
 
-		lrc->ring_begin = guc_ggtt_offset(ce->ring->vma);
+		lrc->ring_begin = intel_guc_ggtt_offset(guc, ce->ring->vma);
 		lrc->ring_end = lrc->ring_begin + ce->ring->size - 1;
 		lrc->ring_next_free_location = lrc->ring_begin;
 		lrc->ring_current_tail_pointer_value = 0;
@@ -400,7 +400,7 @@ static void guc_stage_desc_init(struct intel_guc *guc,
 	 * The doorbell, process descriptor, and workqueue are all parts
 	 * of the client object, which the GuC will reference via the GGTT
 	 */
-	gfx_addr = guc_ggtt_offset(client->vma);
+	gfx_addr = intel_guc_ggtt_offset(guc, client->vma);
 	desc->db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
 				client->doorbell_offset;
 	desc->db_trigger_cpu = ptr_to_u64(__get_doorbell(client));
@@ -596,7 +596,7 @@ static void inject_preempt_context(struct work_struct *work)
 	data[3] = engine->guc_id;
 	data[4] = guc->execbuf_client->priority;
 	data[5] = guc->execbuf_client->stage_id;
-	data[6] = guc_ggtt_offset(guc->shared_data);
+	data[6] = intel_guc_ggtt_offset(guc, guc->shared_data);
 
 	if (WARN_ON(intel_guc_send(guc, data, ARRAY_SIZE(data)))) {
 		execlists_clear_active(&engine->execlists,
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 8ed0518..aed9c1c 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -137,7 +137,8 @@ static int huc_ucode_xfer(struct intel_uc_fw *huc_fw, struct i915_vma *vma)
 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
 
 	/* Set the source address for the uCode */
-	offset = guc_ggtt_offset(vma) + huc_fw->header_offset;
+	offset = intel_guc_ggtt_offset(&dev_priv->guc, vma) +
+		 huc_fw->header_offset;
 	I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset));
 	I915_WRITE(DMA_ADDR_0_HIGH, upper_32_bits(offset) & 0xFFFF);
 
@@ -213,7 +214,8 @@ int intel_huc_auth(struct intel_huc *huc)
 	}
 
 	ret = intel_guc_auth_huc(guc,
-				 guc_ggtt_offset(vma) + huc->fw.rsa_offset);
+				 intel_guc_ggtt_offset(guc, vma) +
+				 huc->fw.rsa_offset);
 	if (ret) {
 		DRM_ERROR("HuC: GuC did not ack Auth request %d\n", ret);
 		goto out;
-- 
2.7.4

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

[PATCH v10 3/7] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

[Jackie Li]

Hardware may have specific restrictions on GuC WOPCM offset and size. On
Gen9, the value of the GuC WOPCM size register needs to be larger than the
value of GuC WOPCM offset register + a Gen9 specific offset (144KB) for
reserved GuC WOPCM. Fail to enforce such a restriction on GuC WOPCM size
will lead to GuC firmware execution failures. So we need add code to verify
the GuC WOPCM offset and size to avoid any GuC failures. On the other hand,
with current static GuC WOPCM offset and size values (512KB for both offset
and size), the GuC WOPCM size verification will fail on Gen9 even if it can
be fixed by lowering the GuC WOPCM offset by calculating its value based on
HuC firmware size (which is likely less than 200KB on Gen9), so that we can
have a GuC WOPCM size value which is large enough to pass the GuC WOPCM
size check.

This patch updates the reserved GuC WOPCM size for RC6 context on Gen9 to
24KB to strictly align with the Gen9 GuC WOPCM layout and add support to
return CNL specific hardware reserved GuC WOPCM size. It also adds support
to verify the GuC WOPCM size aganist the Gen9 hardware restrictions.
Meanwhile, it provides a common way to calculate GuC WOPCM offset and size
based on GuC and HuC firmware sizes for all GuC/HuC enabled platforms.
Currently, GuC WOPCM offset is calculated based on HuC firmware size +
reserved WOPCM size while GuC WOPCM size is set to total WOPCM size - GuC
WOPCM offset - hardware reserved GuC WOPCM size. In this case, GuC WOPCM
offset will be updated based on the size of HuC firmware while GuC WOPCM
size will be set to use all the remaining WOPCM space.

v2:
 - Removed intel_wopcm_init (Ville/Sagar/Joonas)
 - Renamed and Moved the intel_wopcm_partition into intel_guc (Sagar)
 - Removed unnecessary function calls (Joonas)
 - Init GuC WOPCM partition as soon as firmware fetching is completed

v3:
 - Fixed indentation issues (Chris)
 - Removed layering violation code (Chris/Michal)
 - Created separat files for GuC wopcm code  (Michal)
 - Used inline function to avoid code duplication (Michal)

v4:
 - Preset the GuC WOPCM top during early GuC init (Chris)
 - Fail intel_uc_init_hw() as soon as GuC WOPCM partitioning failed

v5:
 - Moved GuC DMA WOPCM register updating code into intel_guc_wopcm.c
 - Took care of the locking status before writing to GuC DMA
   Write-Once registers. (Joonas)

v6:
 - Made sure the GuC WOPCM size to be multiple of 4K (4K aligned)

v8:
 - Updated comments and fixed naming issues (Sagar/Joonas)
 - Updated commit message to include more description about the hardware
   restriction on GuC WOPCM size (Sagar)

v9:
 - Minor changes variable names and code comments (Sagar)
 - Added detailed GuC WOPCM layout drawing (Sagar/Michal)
 - Refined macro definitions to be reader friendly (Michal)
 - Removed redundent check to valid flag (Michal)
 - Unified first parameter for exported GuC WOPCM functions (Michal)
 - Refined the name and parameter list of hardware restriction checking
   functions (Michal)

v10:
 - Used shorter function name for internal functions (Joonas)
 - Moved init-ealry function into c file (Joonas)
 - Consolidated and removed redundant size checks (Joonas/Michal)
 - Removed unnecessary unlikely() from code which is only called once
   during boot (Joonas)
 - More fixes to kernel-doc format and content (Michal)
 - Avoided the use of PAGE_MASK for 4K pages (Michal)
 - Added error log messages to error paths (Michal)

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: John Spotswood <john.a.spotswood@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> (v8)
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> (v9)
Signed-off-by: Jackie Li <yaodong.li@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c |   5 +-
 drivers/gpu/drm/i915/intel_guc.c        |   5 +-
 drivers/gpu/drm/i915/intel_guc.h        |  12 +--
 drivers/gpu/drm/i915/intel_guc_reg.h    |   2 +
 drivers/gpu/drm/i915/intel_guc_wopcm.c  | 128 +++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_guc_wopcm.h  |  72 ++++++++++++++++--
 drivers/gpu/drm/i915/intel_huc.c        |   2 +-
 drivers/gpu/drm/i915/intel_uc.c         |  11 ++-
 drivers/gpu/drm/i915/intel_uc_fw.c      |  11 ++-
 drivers/gpu/drm/i915/intel_uc_fw.h      |  16 ++++
 10 files changed, 231 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 3d75f48..414eab2 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -312,12 +312,13 @@ __create_hw_context(struct drm_i915_private *dev_priv,
 	ctx->desc_template =
 		default_desc_template(dev_priv, dev_priv->mm.aliasing_ppgtt);
 
-	/* GuC requires the ring to be placed above GUC_WOPCM_TOP. If GuC is not
+	/*
+	 * GuC requires the ring to be placed above GuC WOPCM top. If GuC is not
 	 * present or not in use we still need a small bias as ring wraparound
 	 * at offset 0 sometimes hangs. No idea why.
 	 */
 	if (USES_GUC(dev_priv))
-		ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
+		ctx->ggtt_offset_bias = dev_priv->guc.wopcm.top;
 	else
 		ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
 
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index fdf8cb3..578e9f4 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -65,6 +65,7 @@ void intel_guc_init_early(struct intel_guc *guc)
 	intel_guc_fw_init_early(guc);
 	intel_guc_ct_init_early(&guc->ct);
 	intel_guc_log_init_early(guc);
+	intel_guc_wopcm_init_early(&guc->wopcm);
 
 	mutex_init(&guc->send_mutex);
 	guc->send = intel_guc_send_nop;
@@ -477,7 +478,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
  * This is a wrapper to create an object for use with the GuC. In order to
  * use it inside the GuC, an object needs to be pinned lifetime, so we allocate
  * both some backing storage and a range inside the Global GTT. We must pin
- * it in the GGTT somewhere other than than [0, GUC_WOPCM_TOP) because that
+ * it in the GGTT somewhere other than than [0, GuC WOPCM top) because that
  * range is reserved inside GuC.
  *
  * Return:	A i915_vma if successful, otherwise an ERR_PTR.
@@ -498,7 +499,7 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
 		goto err;
 
 	ret = i915_vma_pin(vma, 0, PAGE_SIZE,
-			   PIN_GLOBAL | PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
+			   PIN_GLOBAL | PIN_OFFSET_BIAS | guc->wopcm.top);
 	if (ret) {
 		vma = ERR_PTR(ret);
 		goto err;
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 50be6de..06f315e 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -49,6 +49,7 @@ struct intel_guc {
 	struct intel_uc_fw fw;
 	struct intel_guc_log log;
 	struct intel_guc_ct ct;
+	struct intel_guc_wopcm wopcm;
 
 	/* Log snapshot if GuC errors during load */
 	struct drm_i915_gem_object *load_err_log;
@@ -109,10 +110,10 @@ static inline void intel_guc_notify(struct intel_guc *guc)
  * @guc: intel guc.
  * @vma: i915 graphics virtual memory area.
  *
- * GuC does not allow any gfx GGTT address that falls into range [0, WOPCM_TOP),
- * which is reserved for Boot ROM, SRAM and WOPCM. Currently this top address is
- * 512K. In order to exclude 0-512K address space from GGTT, all gfx objects
- * used by GuC is pinned with PIN_OFFSET_BIAS along with size of WOPCM.
+ * GuC does not allow any gfx GGTT address that falls into range
+ * [0, GuC WOPCM top), which is reserved for Boot ROM, SRAM and WOPCM.
+ * All gfx objects used by GuC is pinned with PIN_OFFSET_BIAS along with
+ * top of WOPCM.
  *
  * Return: GGTT offset that meets the GuC gfx address requirement.
  */
@@ -121,7 +122,8 @@ static inline u32 intel_guc_ggtt_offset(struct intel_guc *guc,
 {
 	u32 offset = i915_ggtt_offset(vma);
 
-	GEM_BUG_ON(offset < GUC_WOPCM_TOP);
+	GEM_BUG_ON(!guc->wopcm.valid);
+	GEM_BUG_ON(offset < guc->wopcm.top);
 	GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
 
 	return offset;
diff --git a/drivers/gpu/drm/i915/intel_guc_reg.h b/drivers/gpu/drm/i915/intel_guc_reg.h
index de4f78b..c482df5 100644
--- a/drivers/gpu/drm/i915/intel_guc_reg.h
+++ b/drivers/gpu/drm/i915/intel_guc_reg.h
@@ -75,6 +75,8 @@
 
 /* Defines WOPCM space available to GuC firmware */
 #define GUC_WOPCM_SIZE			_MMIO(0xc050)
+#define   GUC_WOPCM_SIZE_SHIFT		12
+#define   GUC_WOPCM_SIZE_MASK		  (0xfffff << GUC_WOPCM_SIZE_SHIFT)
 
 #define GEN8_GT_PM_CONFIG		_MMIO(0x138140)
 #define GEN9LP_GT_PM_CONFIG		_MMIO(0x138140)
diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.c b/drivers/gpu/drm/i915/intel_guc_wopcm.c
index 3972901..cf832b9 100644
--- a/drivers/gpu/drm/i915/intel_guc_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
@@ -7,22 +7,128 @@
 #include "intel_guc_wopcm.h"
 #include "i915_drv.h"
 
+static inline u32 context_reserved_size(struct intel_guc *guc)
+{
+	struct drm_i915_private *i915 = guc_to_i915(guc);
+
+	if (IS_GEN9_LP(i915))
+		return BXT_GUC_WOPCM_RC6_CTX_RESERVED;
+
+	return 0;
+}
+
+static inline int gen9_check_dword_gap(struct intel_guc_wopcm *guc_wopcm)
+{
+	u32 guc_wopcm_start;
+	u32 delta;
+
+	/*
+	 * GuC WOPCM size is 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.
+	 */
+	guc_wopcm_start = guc_wopcm->offset + GEN9_GUC_WOPCM_OFFSET;
+	if (guc_wopcm_start > guc_wopcm->size)
+		return -E2BIG;
+
+	delta = guc_wopcm->size - guc_wopcm_start;
+	if (delta < sizeof(u32))
+		return -E2BIG;
+
+	return 0;
+}
+
+static inline int guc_wopcm_size_check(struct intel_guc *guc)
+{
+	struct drm_i915_private *i915 = guc_to_i915(guc);
+	struct intel_guc_wopcm *guc_wopcm = &guc->wopcm;
+
+	if (IS_GEN9(i915))
+		return gen9_check_dword_gap(guc_wopcm);
+
+	return 0;
+}
+
 /**
- * intel_guc_wopcm_size() - Get the size of GuC WOPCM.
- * @guc: intel guc.
+ * intel_guc_wopcm_init_early() - Early initialization of the GuC WOPCM.
+ * @guc_wopcm: pointer to intel_guc_wopcm.
  *
- * Get the platform specific GuC WOPCM size.
+ * Setup the GuC WOPCM top to the top of the overall WOPCM. This will guarantee
+ * that the allocation of the GuC accessible objects won't fall into WOPCM when
+ * GuC partition isn't present.
  *
- * Return: size of the GuC WOPCM.
  */
-u32 intel_guc_wopcm_size(struct intel_guc *guc)
+void intel_guc_wopcm_init_early(struct intel_guc_wopcm *guc_wopcm)
 {
-	struct drm_i915_private *i915 = guc_to_i915(guc);
-	u32 size = GUC_WOPCM_TOP;
+	guc_wopcm->top = WOPCM_DEFAULT_SIZE;
+}
 
-	/* On BXT, the top of WOPCM is reserved for RC6 context */
-	if (IS_GEN9_LP(i915))
-		size -= BXT_GUC_WOPCM_RC6_RESERVED;
+/**
+ * intel_guc_wopcm_init() - Initialize the GuC WOPCM.
+ * @guc_wopcm: pointer to intel_guc_wopcm.
+ * @guc_fw_size: size of GuC firmware.
+ * @huc_fw_size: size of HuC firmware.
+ *
+ * Calculate the GuC WOPCM offset and size based on GuC and HuC firmware sizes.
+ * This function will set the GuC WOPCM size to the size of maximum WOPCM
+ * available for GuC. This function will also enforce platform dependent
+ * hardware restrictions on GuC WOPCM offset and size. It will fail the GuC
+ * 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.
+ */
+int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, u32 guc_fw_size,
+			 u32 huc_fw_size)
+{
+	struct intel_guc *guc =
+		container_of(guc_wopcm, struct intel_guc, wopcm);
+	u32 reserved = context_reserved_size(guc);
+	u32 offset, size, top;
+	int err;
+
+	GEM_BUG_ON(!guc_fw_size);
+
+	offset = huc_fw_size + WOPCM_RESERVED_SIZE;
+
+	/* Hardware requires GuC WOPCM offset to be 16K aligned. */
+	offset = ALIGN(offset, GUC_WOPCM_OFFSET_ALIGNMENT);
+	if ((offset + reserved) >= WOPCM_DEFAULT_SIZE) {
+		DRM_DEBUG_DRIVER("GuC WOPCM offset exceeds max WOPCM size.\n");
+		return -E2BIG;
+	}
+
+	top = WOPCM_DEFAULT_SIZE - offset;
+	size = top - reserved;
+
+	/* GuC WOPCM size must be multiple of 4K pages */
+	size &= GUC_WOPCM_SIZE_MASK;
+
+	/*
+	 * GuC firmware size needs to be less than or equal to the size of the
+	 * available GuC WOPCM (total available GuC WOPCM size - reserved size).
+	 * Need extra 8K stack for GuC firmware.
+	 */
+	reserved = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;
+	if ((guc_fw_size + reserved) > size) {
+		DRM_DEBUG_DRIVER("No enough GuC WOPCM for GuC firmware.\n");
+		return -E2BIG;
+	}
+
+	guc->wopcm.offset = offset;
+	guc->wopcm.size = size;
+	guc->wopcm.top = top;
+
+	err = guc_wopcm_size_check(guc);
+	if (err) {
+		DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n");
+		return err;
+	}
+
+	guc->wopcm.valid = 1;
+
+	DRM_DEBUG_DRIVER("GuC WOPCM offset %dKB, size %dKB, top %dKB\n",
+			 offset >> 10, size >> 10, top >> 10);
 
-	return size;
+	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.h b/drivers/gpu/drm/i915/intel_guc_wopcm.h
index 8c4f693..a632caa 100644
--- a/drivers/gpu/drm/i915/intel_guc_wopcm.h
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.h
@@ -11,15 +11,73 @@
 
 struct intel_guc;
 
-/* 512KB static offset from WOPCM base. */
-#define GUC_WOPCM_OFFSET_VALUE		(512 << 10)
 /*
- * 512KB static GuC WOPCM size from GUC_WOPCM_OFFSET_VALUE to the end of GuC
- * WOPCM. GuC addresses below GUC_WOPCM_TOP don't map through the GTT.
+ * The layout of the WOPCM will be determined by GuC WOPCM size and offset
+ * registers settings. Currently, GuC WOPCM code calculates the GuC WOPCM offset
+ * and size values based on a layout as shown below:
+ *
+ *   +=====+==>+====================+<== GuC WOPCM top
+ *   ^     ^   |  HW contexts RSVD  |
+ *   |     |   +--------------------+<== GuC WOPCM size
+ *   |     |   |                    |
+ *   |    GuC  |                    |
+ *   |   WOPCM +--------------------+<== (Platform specific size)
+ *   |     |   |    GuC FW RSVD     |
+ * WOPCM   |   +------------------- +<== (16KB)
+ *   |     v   |   RSVD GuC WOPCM   |
+ *   |     +==>+====================+<== GuC WOPCM offset
+ *   |         |     RSVD WOPCM     |
+ *   |         +------------------- +
+ *   v         |      HuC FW        |
+ *   +========>+====================+<== WOPCM Base
+ *
+ * GuC accessible WOPCM starts at GuC WOPCM offset and ends at GuC WOPCM size.
+ * The top part of the GuC WOPCM is reserved for hardware contexts (e.g. RC6
+ * context). We need to keep tracking the GuC WOPCM top since hardware requires
+ * the GGTT offset of a GuC accessible GEM buffer to be larger than the value of
+ * GuC WOPCM top. The values of GuC WOPCM size and top should be set to the
+ * length from GuC WOPCM offset in bytes.
+ */
+
+/* Default WOPCM size 1MB. */
+#define WOPCM_DEFAULT_SIZE		(1 << 20)
+/* The initial 16KB WOPCM (RSVD WOPCM) is reserved. */
+#define WOPCM_RESERVED_SIZE		(16 << 10)
+
+/* GUC WOPCM offset needs to be 16KB aligned. */
+#define GUC_WOPCM_OFFSET_ALIGNMENT	(16 << 10)
+/* 16KB reserved at the beginning of GuC WOPCM. */
+#define GUC_WOPCM_RESERVED		(16 << 10)
+/* 8KB from GUC_WOPCM_RESERVED is reserved for GuC stack. */
+#define GUC_WOPCM_STACK_RESERVED	(8 << 10)
+/* 24KB at the end of GuC WOPCM is reserved for RC6 CTX on BXT. */
+#define BXT_GUC_WOPCM_RC6_CTX_RESERVED	(24 << 10)
+
+/*
+ * GuC WOPCM starts at 144KB (GUC_WOPCM_RESERVED + 128KB reserved for GuC
+ * firmware loading) from GuC WOPCM offset on BXT.
+ */
+#define GEN9_GUC_WOPCM_OFFSET		(144 << 10)
+
+/**
+ * intel_guc_wopcm - GuC WOPCM related settings.
+ * @offset: GuC WOPCM offset from the WOPCM base.
+ * @size: size of GuC WOPCM for GuC firmware.
+ * @top: start of the non-GuC WOPCM memory.
+ * @valid: whether this structure contains valid (1-valid, 0-invalid) info.
+ *
+ * We simply use this structure to track the GuC use of WOPCM. The layout of
+ * WOPCM would be defined by writing to GuC WOPCM offset and size registers.
  */
-#define GUC_WOPCM_TOP			(512 << 10)
-#define BXT_GUC_WOPCM_RC6_RESERVED	(64 << 10)
+struct intel_guc_wopcm {
+	u32 offset;
+	u32 size;
+	u32 top;
+	u32 valid;
+};
 
-u32 intel_guc_wopcm_size(struct intel_guc *guc);
+void intel_guc_wopcm_init_early(struct intel_guc_wopcm *guc_wopcm);
+int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, u32 guc_size,
+			 u32 huc_size);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index aed9c1c..dc6a6c6 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -206,7 +206,7 @@ int intel_huc_auth(struct intel_huc *huc)
 		return -ENOEXEC;
 
 	vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0,
-				PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
+				PIN_OFFSET_BIAS | guc->wopcm.top);
 	if (IS_ERR(vma)) {
 		ret = PTR_ERR(vma);
 		DRM_ERROR("HuC: Failed to pin huc fw object %d\n", ret);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 1b2831b..c842f36 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -283,6 +283,9 @@ void intel_uc_fini_misc(struct drm_i915_private *dev_priv)
 int intel_uc_init(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
+	struct intel_huc *huc = &dev_priv->huc;
+	u32 guc_fw_size = intel_uc_fw_get_size(&guc->fw);
+	u32 huc_fw_size = intel_uc_fw_get_size(&huc->fw);
 	int ret;
 
 	if (!USES_GUC(dev_priv))
@@ -291,6 +294,10 @@ int intel_uc_init(struct drm_i915_private *dev_priv)
 	if (!HAS_GUC(dev_priv))
 		return -ENODEV;
 
+	ret = intel_guc_wopcm_init(&guc->wopcm, guc_fw_size, huc_fw_size);
+	if (ret)
+		return ret;
+
 	ret = intel_guc_init(guc);
 	if (ret)
 		return ret;
@@ -340,9 +347,9 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	gen9_reset_guc_interrupts(dev_priv);
 
 	/* init WOPCM */
-	I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(guc));
+	I915_WRITE(GUC_WOPCM_SIZE, guc->wopcm.size);
 	I915_WRITE(DMA_GUC_WOPCM_OFFSET,
-		   GUC_WOPCM_OFFSET_VALUE | HUC_LOADING_AGENT_GUC);
+		   guc->wopcm.offset | HUC_LOADING_AGENT_GUC);
 
 	/* WaEnableuKernelHeaderValidFix:skl */
 	/* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
index 24945cf..791263a 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/intel_uc_fw.c
@@ -95,9 +95,13 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 	uc_fw->ucode_offset = uc_fw->header_offset + uc_fw->header_size;
 	uc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
 
-	/* Header and uCode will be loaded to WOPCM */
+	/*
+	 * Header and uCode will be loaded to WOPCM
+	 * Only check the size against the overall available WOPCM here. Will
+	 * continue to check the size during WOPCM partition calculation.
+	 */
 	size = uc_fw->header_size + uc_fw->ucode_size;
-	if (size > intel_guc_wopcm_size(&dev_priv->guc)) {
+	if (size > WOPCM_DEFAULT_SIZE) {
 		DRM_WARN("%s: Firmware is too large to fit in WOPCM\n",
 			 intel_uc_fw_type_repr(uc_fw->type));
 		err = -E2BIG;
@@ -207,6 +211,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
 		       int (*xfer)(struct intel_uc_fw *uc_fw,
 				   struct i915_vma *vma))
 {
+	struct drm_i915_private *i915 = to_i915(uc_fw->obj->base.dev);
 	struct i915_vma *vma;
 	int err;
 
@@ -230,7 +235,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
 	}
 
 	vma = i915_gem_object_ggtt_pin(uc_fw->obj, NULL, 0, 0,
-				       PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
+				       PIN_OFFSET_BIAS | i915->guc.wopcm.top);
 	if (IS_ERR(vma)) {
 		err = PTR_ERR(vma);
 		DRM_DEBUG_DRIVER("%s fw ggtt-pin err=%d\n",
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h
index d5fd460..ed3043d 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/intel_uc_fw.h
@@ -115,6 +115,22 @@ static inline bool intel_uc_fw_is_selected(struct intel_uc_fw *uc_fw)
 	return uc_fw->path != NULL;
 }
 
+/**
+ * intel_uc_fw_get_size() - Get the size of the firmware.
+ * @uc_fw: intel_uc_fw structure.
+ *
+ * Get the size of the firmware that will be placed in WOPCM.
+ *
+ * Return: Size of the firmware, or zero on firmware fetch failure.
+ */
+static inline u32 intel_uc_fw_get_size(struct intel_uc_fw *uc_fw)
+{
+	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
+		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);
 int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
-- 
2.7.4

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

Re: [PATCH v10 3/7] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

[Michal Wajdeczko]

On Tue, 13 Feb 2018 00:45:49 +0100, Jackie Li <yaodong.li@intel.com> wrote:

> Hardware may have specific restrictions on GuC WOPCM offset and size. On
> Gen9, the value of the GuC WOPCM size register needs to be larger than  
> the
> value of GuC WOPCM offset register + a Gen9 specific offset (144KB) for
> reserved GuC WOPCM. Fail to enforce such a restriction on GuC WOPCM size
> will lead to GuC firmware execution failures. So we need add code to  
> verify
> the GuC WOPCM offset and size to avoid any GuC failures. On the other  
> hand,
> with current static GuC WOPCM offset and size values (512KB for both  
> offset
> and size), the GuC WOPCM size verification will fail on Gen9 even if it  
> can
> be fixed by lowering the GuC WOPCM offset by calculating its value based  
> on
> HuC firmware size (which is likely less than 200KB on Gen9), so that we  
> can
> have a GuC WOPCM size value which is large enough to pass the GuC WOPCM
> size check.
>
> This patch updates the reserved GuC WOPCM size for RC6 context on Gen9 to
> 24KB to strictly align with the Gen9 GuC WOPCM layout and add support to
> return CNL specific hardware reserved GuC WOPCM size. It also adds  
> support
> to verify the GuC WOPCM size aganist the Gen9 hardware restrictions.
> Meanwhile, it provides a common way to calculate GuC WOPCM offset and  
> size
> based on GuC and HuC firmware sizes for all GuC/HuC enabled platforms.
> Currently, GuC WOPCM offset is calculated based on HuC firmware size +
> reserved WOPCM size while GuC WOPCM size is set to total WOPCM size - GuC
> WOPCM offset - hardware reserved GuC WOPCM size. In this case, GuC WOPCM
> offset will be updated based on the size of HuC firmware while GuC WOPCM
> size will be set to use all the remaining WOPCM space.
>

Can we add here some documentation tag like

Bspec: 12690, ...

as we did in https://patchwork.kernel.org/patch/10116053/ ?

> v2:
>  - Removed intel_wopcm_init (Ville/Sagar/Joonas)
>  - Renamed and Moved the intel_wopcm_partition into intel_guc (Sagar)
>  - Removed unnecessary function calls (Joonas)
>  - Init GuC WOPCM partition as soon as firmware fetching is completed
>
> v3:
>  - Fixed indentation issues (Chris)
>  - Removed layering violation code (Chris/Michal)
>  - Created separat files for GuC wopcm code  (Michal)
>  - Used inline function to avoid code duplication (Michal)
>
> v4:
>  - Preset the GuC WOPCM top during early GuC init (Chris)
>  - Fail intel_uc_init_hw() as soon as GuC WOPCM partitioning failed
>
> v5:
>  - Moved GuC DMA WOPCM register updating code into intel_guc_wopcm.c
>  - Took care of the locking status before writing to GuC DMA
>    Write-Once registers. (Joonas)
>
> v6:
>  - Made sure the GuC WOPCM size to be multiple of 4K (4K aligned)
>
> v8:
>  - Updated comments and fixed naming issues (Sagar/Joonas)
>  - Updated commit message to include more description about the hardware
>    restriction on GuC WOPCM size (Sagar)
>
> v9:
>  - Minor changes variable names and code comments (Sagar)
>  - Added detailed GuC WOPCM layout drawing (Sagar/Michal)
>  - Refined macro definitions to be reader friendly (Michal)
>  - Removed redundent check to valid flag (Michal)
>  - Unified first parameter for exported GuC WOPCM functions (Michal)
>  - Refined the name and parameter list of hardware restriction checking
>    functions (Michal)
>
> v10:
>  - Used shorter function name for internal functions (Joonas)
>  - Moved init-ealry function into c file (Joonas)
>  - Consolidated and removed redundant size checks (Joonas/Michal)
>  - Removed unnecessary unlikely() from code which is only called once
>    during boot (Joonas)
>  - More fixes to kernel-doc format and content (Michal)
>  - Avoided the use of PAGE_MASK for 4K pages (Michal)
>  - Added error log messages to error paths (Michal)
>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: John Spotswood <john.a.spotswood@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> (v8)
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> (v9)
> Signed-off-by: Jackie Li <yaodong.li@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c |   5 +-
>  drivers/gpu/drm/i915/intel_guc.c        |   5 +-
>  drivers/gpu/drm/i915/intel_guc.h        |  12 +--
>  drivers/gpu/drm/i915/intel_guc_reg.h    |   2 +
>  drivers/gpu/drm/i915/intel_guc_wopcm.c  | 128  
> +++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_guc_wopcm.h  |  72 ++++++++++++++++--
>  drivers/gpu/drm/i915/intel_huc.c        |   2 +-
>  drivers/gpu/drm/i915/intel_uc.c         |  11 ++-
>  drivers/gpu/drm/i915/intel_uc_fw.c      |  11 ++-
>  drivers/gpu/drm/i915/intel_uc_fw.h      |  16 ++++
>  10 files changed, 231 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c  
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 3d75f48..414eab2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -312,12 +312,13 @@ __create_hw_context(struct drm_i915_private  
> *dev_priv,
>  	ctx->desc_template =
>  		default_desc_template(dev_priv, dev_priv->mm.aliasing_ppgtt);
> -	/* GuC requires the ring to be placed above GUC_WOPCM_TOP. If GuC is  
> not
> +	/*
> +	 * GuC requires the ring to be placed above GuC WOPCM top. If GuC is  
> not
>  	 * present or not in use we still need a small bias as ring wraparound
>  	 * at offset 0 sometimes hangs. No idea why.
>  	 */
>  	if (USES_GUC(dev_priv))
> -		ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
> +		ctx->ggtt_offset_bias = dev_priv->guc.wopcm.top;
>  	else
>  		ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index fdf8cb3..578e9f4 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -65,6 +65,7 @@ void intel_guc_init_early(struct intel_guc *guc)
>  	intel_guc_fw_init_early(guc);
>  	intel_guc_ct_init_early(&guc->ct);
>  	intel_guc_log_init_early(guc);
> +	intel_guc_wopcm_init_early(&guc->wopcm);
> 	mutex_init(&guc->send_mutex);
>  	guc->send = intel_guc_send_nop;
> @@ -477,7 +478,7 @@ int intel_guc_resume(struct drm_i915_private  
> *dev_priv)
>   * This is a wrapper to create an object for use with the GuC. In order  
> to
>   * use it inside the GuC, an object needs to be pinned lifetime, so we  
> allocate
>   * both some backing storage and a range inside the Global GTT. We must  
> pin
> - * it in the GGTT somewhere other than than [0, GUC_WOPCM_TOP) because  
> that
> + * it in the GGTT somewhere other than than [0, GuC WOPCM top) because  
> that
>   * range is reserved inside GuC.
>   *
>   * Return:	A i915_vma if successful, otherwise an ERR_PTR.
> @@ -498,7 +499,7 @@ struct i915_vma *intel_guc_allocate_vma(struct  
> intel_guc *guc, u32 size)
>  		goto err;
> 	ret = i915_vma_pin(vma, 0, PAGE_SIZE,
> -			   PIN_GLOBAL | PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> +			   PIN_GLOBAL | PIN_OFFSET_BIAS | guc->wopcm.top);
>  	if (ret) {
>  		vma = ERR_PTR(ret);
>  		goto err;
> diff --git a/drivers/gpu/drm/i915/intel_guc.h  
> b/drivers/gpu/drm/i915/intel_guc.h
> index 50be6de..06f315e 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -49,6 +49,7 @@ struct intel_guc {
>  	struct intel_uc_fw fw;
>  	struct intel_guc_log log;
>  	struct intel_guc_ct ct;
> +	struct intel_guc_wopcm wopcm;
> 	/* Log snapshot if GuC errors during load */
>  	struct drm_i915_gem_object *load_err_log;
> @@ -109,10 +110,10 @@ static inline void intel_guc_notify(struct  
> intel_guc *guc)
>   * @guc: intel guc.
>   * @vma: i915 graphics virtual memory area.
>   *
> - * GuC does not allow any gfx GGTT address that falls into range [0,  
> WOPCM_TOP),
> - * which is reserved for Boot ROM, SRAM and WOPCM. Currently this top  
> address is
> - * 512K. In order to exclude 0-512K address space from GGTT, all gfx  
> objects
> - * used by GuC is pinned with PIN_OFFSET_BIAS along with size of WOPCM.
> + * GuC does not allow any gfx GGTT address that falls into range
> + * [0, GuC WOPCM top), which is reserved for Boot ROM, SRAM and WOPCM.
> + * All gfx objects used by GuC is pinned with PIN_OFFSET_BIAS along with
> + * top of WOPCM.
>   *
>   * Return: GGTT offset that meets the GuC gfx address requirement.
>   */
> @@ -121,7 +122,8 @@ static inline u32 intel_guc_ggtt_offset(struct  
> intel_guc *guc,
>  {
>  	u32 offset = i915_ggtt_offset(vma);
> -	GEM_BUG_ON(offset < GUC_WOPCM_TOP);
> +	GEM_BUG_ON(!guc->wopcm.valid);
> +	GEM_BUG_ON(offset < guc->wopcm.top);
>  	GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
> 	return offset;
> diff --git a/drivers/gpu/drm/i915/intel_guc_reg.h  
> b/drivers/gpu/drm/i915/intel_guc_reg.h
> index de4f78b..c482df5 100644
> --- a/drivers/gpu/drm/i915/intel_guc_reg.h
> +++ b/drivers/gpu/drm/i915/intel_guc_reg.h
> @@ -75,6 +75,8 @@
> /* Defines WOPCM space available to GuC firmware */
>  #define GUC_WOPCM_SIZE			_MMIO(0xc050)
> +#define   GUC_WOPCM_SIZE_SHIFT		12
> +#define   GUC_WOPCM_SIZE_MASK		  (0xfffff << GUC_WOPCM_SIZE_SHIFT)
> #define GEN8_GT_PM_CONFIG		_MMIO(0x138140)
>  #define GEN9LP_GT_PM_CONFIG		_MMIO(0x138140)
> diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.c  
> b/drivers/gpu/drm/i915/intel_guc_wopcm.c
> index 3972901..cf832b9 100644
> --- a/drivers/gpu/drm/i915/intel_guc_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
> @@ -7,22 +7,128 @@
>  #include "intel_guc_wopcm.h"
>  #include "i915_drv.h"
> +static inline u32 context_reserved_size(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *i915 = guc_to_i915(guc);
> +
> +	if (IS_GEN9_LP(i915))
> +		return BXT_GUC_WOPCM_RC6_CTX_RESERVED;
> +
> +	return 0;
> +}
> +
> +static inline int gen9_check_dword_gap(struct intel_guc_wopcm  
> *guc_wopcm)
> +{
> +	u32 guc_wopcm_start;
> +	u32 delta;
> +
> +	/*
> +	 * GuC WOPCM size is 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.
> +	 */
> +	guc_wopcm_start = guc_wopcm->offset + GEN9_GUC_WOPCM_OFFSET;
> +	if (guc_wopcm_start > guc_wopcm->size)
> +		return -E2BIG;
> +
> +	delta = guc_wopcm->size - guc_wopcm_start;
> +	if (delta < sizeof(u32))
> +		return -E2BIG;
> +
> +	return 0;
> +}
> +
> +static inline int guc_wopcm_size_check(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *i915 = guc_to_i915(guc);
> +	struct intel_guc_wopcm *guc_wopcm = &guc->wopcm;
> +
> +	if (IS_GEN9(i915))
> +		return gen9_check_dword_gap(guc_wopcm);
> +
> +	return 0;
> +}
> +
>  /**
> - * intel_guc_wopcm_size() - Get the size of GuC WOPCM.
> - * @guc: intel guc.
> + * intel_guc_wopcm_init_early() - Early initialization of the GuC WOPCM.
> + * @guc_wopcm: pointer to intel_guc_wopcm.
>   *
> - * Get the platform specific GuC WOPCM size.
> + * Setup the GuC WOPCM top to the top of the overall WOPCM. This will  
> guarantee
> + * that the allocation of the GuC accessible objects won't fall into  
> WOPCM when
> + * GuC partition isn't present.
>   *
> - * Return: size of the GuC WOPCM.
>   */
> -u32 intel_guc_wopcm_size(struct intel_guc *guc)
> +void intel_guc_wopcm_init_early(struct intel_guc_wopcm *guc_wopcm)
>  {
> -	struct drm_i915_private *i915 = guc_to_i915(guc);
> -	u32 size = GUC_WOPCM_TOP;
> +	guc_wopcm->top = WOPCM_DEFAULT_SIZE;
> +}
> -	/* On BXT, the top of WOPCM is reserved for RC6 context */
> -	if (IS_GEN9_LP(i915))
> -		size -= BXT_GUC_WOPCM_RC6_RESERVED;
> +/**
> + * intel_guc_wopcm_init() - Initialize the GuC WOPCM.
> + * @guc_wopcm: pointer to intel_guc_wopcm.
> + * @guc_fw_size: size of GuC firmware.
> + * @huc_fw_size: size of HuC firmware.
> + *
> + * Calculate the GuC WOPCM offset and size based on GuC and HuC  
> firmware sizes.
> + * This function will set the GuC WOPCM size to the size of maximum  
> WOPCM
> + * available for GuC. This function will also enforce platform dependent
> + * hardware restrictions on GuC WOPCM offset and size. It will fail the  
> GuC
> + * 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.
> + */
> +int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, u32  
> guc_fw_size,
> +			 u32 huc_fw_size)
> +{
> +	struct intel_guc *guc =
> +		container_of(guc_wopcm, struct intel_guc, wopcm);
> +	u32 reserved = context_reserved_size(guc);
> +	u32 offset, size, top;
> +	int err;
> +
> +	GEM_BUG_ON(!guc_fw_size);
> +
> +	offset = huc_fw_size + WOPCM_RESERVED_SIZE;
> +
> +	/* Hardware requires GuC WOPCM offset to be 16K aligned. */
> +	offset = ALIGN(offset, GUC_WOPCM_OFFSET_ALIGNMENT);
> +	if ((offset + reserved) >= WOPCM_DEFAULT_SIZE) {
> +		DRM_DEBUG_DRIVER("GuC WOPCM offset exceeds max WOPCM size.\n");
> +		return -E2BIG;
> +	}
> +
> +	top = WOPCM_DEFAULT_SIZE - offset;
> +	size = top - reserved;
> +
> +	/* GuC WOPCM size must be multiple of 4K pages */
> +	size &= GUC_WOPCM_SIZE_MASK;
> +
> +	/*
> +	 * GuC firmware size needs to be less than or equal to the size of the
> +	 * available GuC WOPCM (total available GuC WOPCM size - reserved  
> size).
> +	 * Need extra 8K stack for GuC firmware.
> +	 */
> +	reserved = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;
> +	if ((guc_fw_size + reserved) > size) {
> +		DRM_DEBUG_DRIVER("No enough GuC WOPCM for GuC firmware.\n");
> +		return -E2BIG;
> +	}
> +
> +	guc->wopcm.offset = offset;
> +	guc->wopcm.size = size;
> +	guc->wopcm.top = top;
> +
> +	err = guc_wopcm_size_check(guc);
> +	if (err) {
> +		DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n");
> +		return err;
> +	}
> +
> +	guc->wopcm.valid = 1;
> +
> +	DRM_DEBUG_DRIVER("GuC WOPCM offset %dKB, size %dKB, top %dKB\n",

s/KB/KiB

> +			 offset >> 10, size >> 10, top >> 10);

I'm still human and prefer "offset / 1024" over binary shifts.

> -	return size;
> +	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.h  
> b/drivers/gpu/drm/i915/intel_guc_wopcm.h
> index 8c4f693..a632caa 100644
> --- a/drivers/gpu/drm/i915/intel_guc_wopcm.h
> +++ b/drivers/gpu/drm/i915/intel_guc_wopcm.h
> @@ -11,15 +11,73 @@
> struct intel_guc;
> -/* 512KB static offset from WOPCM base. */
> -#define GUC_WOPCM_OFFSET_VALUE		(512 << 10)
>  /*
> - * 512KB static GuC WOPCM size from GUC_WOPCM_OFFSET_VALUE to the end  
> of GuC
> - * WOPCM. GuC addresses below GUC_WOPCM_TOP don't map through the GTT.
> + * The layout of the WOPCM will be determined by GuC WOPCM size and  
> offset
> + * registers settings. Currently, GuC WOPCM code calculates the GuC  
> WOPCM offset
> + * and size values based on a layout as shown below:
> + *
> + *   +=====+==>+====================+<== GuC WOPCM top
> + *   ^     ^   |  HW contexts RSVD  |
> + *   |     |   +--------------------+<== GuC WOPCM size
> + *   |     |   |                    |
> + *   |    GuC  |                    |
> + *   |   WOPCM +--------------------+<== (Platform specific size)
> + *   |     |   |    GuC FW RSVD     |
> + * WOPCM   |   +------------------- +<== (16KB)
> + *   |     v   |   RSVD GuC WOPCM   |
> + *   |     +==>+====================+<== GuC WOPCM offset
> + *   |         |     RSVD WOPCM     |
> + *   |         +------------------- +
> + *   v         |      HuC FW        |
> + *   +========>+====================+<== WOPCM Base
> + *
> + * GuC accessible WOPCM starts at GuC WOPCM offset and ends at GuC  
> WOPCM size.
> + * The top part of the GuC WOPCM is reserved for hardware contexts  
> (e.g. RC6
> + * context). We need to keep tracking the GuC WOPCM top since hardware  
> requires
> + * the GGTT offset of a GuC accessible GEM buffer to be larger than the  
> value of
> + * GuC WOPCM top. The values of GuC WOPCM size and top should be set to  
> the
> + * length from GuC WOPCM offset in bytes.
> + */

Maybe this description can be converted into nice kernel-doc?
And IIRC there was suggestion to move it to .c file.

Also, comparing again your drawing with the spec I think it
should look like this:

    +-------- +====================+ <== WOPCM top
   /|\        |     HW RESERVED    |
    |         +------------------- +
    |         |                    |
    |     +-- +====================+ <== GuC WOPCM top
    |    /|\  |                    |
    |     |   |                    |
    |    GuC  |                    |
    |   WOPCM |    GuC firmware    |
    |    size |                    |
  WOPCM   |   +--------------------+
   size  \|/  |    GuC reserved    |
    |     +-- +====================+ <== GuC WOPCM base (offset from WOPCM  
base)
    |         |   WOPCM RESERVED   |
    |         +--------------------+
   \|/        |    HuC firmware    |
    +-------- +====================+ <== WOPCM base

> +
> +/* Default WOPCM size 1MB. */
> +#define WOPCM_DEFAULT_SIZE		(1 << 20)
> +/* The initial 16KB WOPCM (RSVD WOPCM) is reserved. */
> +#define WOPCM_RESERVED_SIZE		(16 << 10)

I'm not sure that we can describe it as "initial" since it's above HuC

> +
> +/* GUC WOPCM offset needs to be 16KB aligned. */
> +#define GUC_WOPCM_OFFSET_ALIGNMENT	(16 << 10)
> +/* 16KB reserved at the beginning of GuC WOPCM. */
> +#define GUC_WOPCM_RESERVED		(16 << 10)
> +/* 8KB from GUC_WOPCM_RESERVED is reserved for GuC stack. */
> +#define GUC_WOPCM_STACK_RESERVED	(8 << 10)
> +/* 24KB at the end of GuC WOPCM is reserved for RC6 CTX on BXT. */
> +#define BXT_GUC_WOPCM_RC6_CTX_RESERVED	(24 << 10)

Hmm, I'm still not convinced that we correctly attach it to "GuC WOPCM".

In the result, maybe we should stop focusing on "intel_guc_wopcm" and  
define
everything as "intel_wopcm" as it was earlier suggested by Joonas.

Then we can define our struct to match diagram above as

struct intel_wopcm {
	u32 size;
	struct {
		u32 base;
		u32 size;
	} guc;
};

u32 intel_wopcm_guc_top(struct intel_wopcm *wopcm)
{
	return wopcm->guc.base + wopcm->guc.size;
}

> +
> +/*
> + * GuC WOPCM starts at 144KB (GUC_WOPCM_RESERVED + 128KB reserved for  
> GuC
> + * firmware loading) from GuC WOPCM offset on BXT.
> + */
> +#define GEN9_GUC_WOPCM_OFFSET		(144 << 10)

Is this BXT or GEN9 specific ?
There is mismatch between comment and macro name

> +
> +/**
> + * intel_guc_wopcm - GuC WOPCM related settings.
> + * @offset: GuC WOPCM offset from the WOPCM base.
> + * @size: size of GuC WOPCM for GuC firmware.
> + * @top: start of the non-GuC WOPCM memory.
> + * @valid: whether this structure contains valid (1-valid, 0-invalid)  
> info.
> + *
> + * We simply use this structure to track the GuC use of WOPCM. The  
> layout of
> + * WOPCM would be defined by writing to GuC WOPCM offset and size  
> registers.
>   */
> -#define GUC_WOPCM_TOP			(512 << 10)
> -#define BXT_GUC_WOPCM_RC6_RESERVED	(64 << 10)
> +struct intel_guc_wopcm {
> +	u32 offset;
> +	u32 size;
> +	u32 top;
> +	u32 valid;
> +};
> -u32 intel_guc_wopcm_size(struct intel_guc *guc);
> +void intel_guc_wopcm_init_early(struct intel_guc_wopcm *guc_wopcm);
> +int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, u32  
> guc_size,
> +			 u32 huc_size);
> #endif
> diff --git a/drivers/gpu/drm/i915/intel_huc.c  
> b/drivers/gpu/drm/i915/intel_huc.c
> index aed9c1c..dc6a6c6 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -206,7 +206,7 @@ int intel_huc_auth(struct intel_huc *huc)
>  		return -ENOEXEC;
> 	vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0,
> -				PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> +				PIN_OFFSET_BIAS | guc->wopcm.top);
>  	if (IS_ERR(vma)) {
>  		ret = PTR_ERR(vma);
>  		DRM_ERROR("HuC: Failed to pin huc fw object %d\n", ret);
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index 1b2831b..c842f36 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -283,6 +283,9 @@ void intel_uc_fini_misc(struct drm_i915_private  
> *dev_priv)
>  int intel_uc_init(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc *guc = &dev_priv->guc;
> +	struct intel_huc *huc = &dev_priv->huc;
> +	u32 guc_fw_size = intel_uc_fw_get_size(&guc->fw);
> +	u32 huc_fw_size = intel_uc_fw_get_size(&huc->fw);
>  	int ret;
> 	if (!USES_GUC(dev_priv))
> @@ -291,6 +294,10 @@ int intel_uc_init(struct drm_i915_private *dev_priv)
>  	if (!HAS_GUC(dev_priv))
>  		return -ENODEV;
> +	ret = intel_guc_wopcm_init(&guc->wopcm, guc_fw_size, huc_fw_size);
> +	if (ret)
> +		return ret;
> +
>  	ret = intel_guc_init(guc);
>  	if (ret)
>  		return ret;
> @@ -340,9 +347,9 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	gen9_reset_guc_interrupts(dev_priv);
> 	/* init WOPCM */
> -	I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(guc));
> +	I915_WRITE(GUC_WOPCM_SIZE, guc->wopcm.size);
>  	I915_WRITE(DMA_GUC_WOPCM_OFFSET,
> -		   GUC_WOPCM_OFFSET_VALUE | HUC_LOADING_AGENT_GUC);
> +		   guc->wopcm.offset | HUC_LOADING_AGENT_GUC);
> 	/* WaEnableuKernelHeaderValidFix:skl */
>  	/* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c  
> b/drivers/gpu/drm/i915/intel_uc_fw.c
> index 24945cf..791263a 100644
> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
> @@ -95,9 +95,13 @@ void intel_uc_fw_fetch(struct drm_i915_private  
> *dev_priv,
>  	uc_fw->ucode_offset = uc_fw->header_offset + uc_fw->header_size;
>  	uc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
> -	/* Header and uCode will be loaded to WOPCM */
> +	/*
> +	 * Header and uCode will be loaded to WOPCM
> +	 * Only check the size against the overall available WOPCM here. Will
> +	 * continue to check the size during WOPCM partition calculation.
> +	 */

Hmm, maybe we can drop completely this early check from here, as we have
this extra check later ?

>  	size = uc_fw->header_size + uc_fw->ucode_size;
> -	if (size > intel_guc_wopcm_size(&dev_priv->guc)) {
> +	if (size > WOPCM_DEFAULT_SIZE) {
>  		DRM_WARN("%s: Firmware is too large to fit in WOPCM\n",
>  			 intel_uc_fw_type_repr(uc_fw->type));
>  		err = -E2BIG;
> @@ -207,6 +211,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
>  		       int (*xfer)(struct intel_uc_fw *uc_fw,
>  				   struct i915_vma *vma))
>  {
> +	struct drm_i915_private *i915 = to_i915(uc_fw->obj->base.dev);
>  	struct i915_vma *vma;
>  	int err;
> @@ -230,7 +235,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
>  	}
> 	vma = i915_gem_object_ggtt_pin(uc_fw->obj, NULL, 0, 0,
> -				       PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> +				       PIN_OFFSET_BIAS | i915->guc.wopcm.top);
>  	if (IS_ERR(vma)) {
>  		err = PTR_ERR(vma);
>  		DRM_DEBUG_DRIVER("%s fw ggtt-pin err=%d\n",
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h  
> b/drivers/gpu/drm/i915/intel_uc_fw.h
> index d5fd460..ed3043d 100644
> --- a/drivers/gpu/drm/i915/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.h
> @@ -115,6 +115,22 @@ static inline bool intel_uc_fw_is_selected(struct  
> intel_uc_fw *uc_fw)
>  	return uc_fw->path != NULL;
>  }
> +/**
> + * intel_uc_fw_get_size() - Get the size of the firmware.
> + * @uc_fw: intel_uc_fw structure.
> + *
> + * Get the size of the firmware that will be placed in WOPCM.
> + *
> + * Return: Size of the firmware, or zero on firmware fetch failure.
> + */
> +static inline u32 intel_uc_fw_get_size(struct intel_uc_fw *uc_fw)
> +{
> +	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> +		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);
>  int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v10 3/7] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

[Yaodong Li]

On 02/13/2018 07:56 AM, Michal Wajdeczko wrote:
>> +
>> +/* GUC WOPCM offset needs to be 16KB aligned. */
>> +#define GUC_WOPCM_OFFSET_ALIGNMENT    (16 << 10)
>> +/* 16KB reserved at the beginning of GuC WOPCM. */
>> +#define GUC_WOPCM_RESERVED        (16 << 10)
>> +/* 8KB from GUC_WOPCM_RESERVED is reserved for GuC stack. */
>> +#define GUC_WOPCM_STACK_RESERVED    (8 << 10)
>> +/* 24KB at the end of GuC WOPCM is reserved for RC6 CTX on BXT. */
>> +#define BXT_GUC_WOPCM_RC6_CTX_RESERVED    (24 << 10)
>
> Hmm, I'm still not convinced that we correctly attach it to "GuC WOPCM".
Can you explain the reason and more about your concerns?
>
> In the result, maybe we should stop focusing on "intel_guc_wopcm" and 
> define
> everything as "intel_wopcm" as it was earlier suggested by Joonas.
>
> Then we can define our struct to match diagram above as
>
> struct intel_wopcm {
>     u32 size;
>     struct {
>         u32 base;
>         u32 size;
>     } guc;
> };
>
Thanks Michal! but I don't think this is quite clean design. two reason:
0) I agree there should be something like intel_wopcm to describe
     the overall wopcm info, that what I did in my v1 patch series.
     the question is whether we really need it even if we are using
     only static wopcm size value? I think it should be more clear to
     introduce intel_wopcm as  a part of a patch that supports dynamic
     wopcm size calculation.
2) the wopcm region (a.k.a partition) is definitely a concept that should
     exist at least in uc layer. if we chose not to init uc/guc, it would be
     nonsense to init these partitions/info in an intel_wopcm which 
attached to
     drm_i915_private and we have initialized a part of this struct 
(e.g. size).
     to make it clean I will insist to have the guc wopcm region (or maybe
     the other region info) kept in uc level.
As I said the purpose of this series is to enable dynamic GuC WOPCM offset
and size calculation. it's not targeting any code refactoring and only 
serves
as a new feature enabling patch. The design principle of this series was to
provide clear new feature enabling by:
1) align with current code design and implementation.
2) provide correct hardware abstraction.
3) faultlessly enabled these new features. (e.g. dynamic size/offset 
calculation)
I think this series is now in a good shape to meet all above targets.

On the other hand, I do agree there always is some room to make our current
code clearer, but what we are talking about is further code refactoring.
Actually, I've already had some ideas of this. e.g. we could have some new
abstractions such as:

struct intel_wopcm {
     u32 size;
     /*any other global wopcm info*/
}

struct wopcm_region {
     u32 reserved; // reserved size in each region
     u32 offset; // offset of each region
     u32 size; // size of each region
};

struct intel_uc {
     struct wopcm_regions regions[];
     struct intel_uc_fw fws[];
     struct intel_guc guc;
     ...
}

struct intel_guc_dma {
     u32 fw_domains;
     lockable_reg size;
     lockable_reg offset;
     u32 flags; // e.g. loading HuC.
}

guc_dma_init()
guc_dma_hw_init()
guc_dma_xfer()

struct intel_guc {
     struct intel_guc_dma guc_dma;
     /*other guc objects*/
}

This would provide better software layer abstraction. but what I learned
from the 10 versions code submission is we need make things clear enough to
move forward. The lack of uc level abstraction is probably the reason why we
felt so frustrating about this part of code.

Can we just move forward by aligning to the current code implementation?
since what we need now is enable this new features. and we definitely can
provide more code refactoring patches based on these changes later.

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

Re: [PATCH v10 3/7] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

[Michal Wajdeczko]

On Tue, 13 Feb 2018 21:01:04 +0100, Yaodong Li <yaodong.li@intel.com>  
wrote:

> On 02/13/2018 07:56 AM, Michal Wajdeczko wrote:
>>> +
>>> +/* GUC WOPCM offset needs to be 16KB aligned. */
>>> +#define GUC_WOPCM_OFFSET_ALIGNMENT    (16 << 10)
>>> +/* 16KB reserved at the beginning of GuC WOPCM. */
>>> +#define GUC_WOPCM_RESERVED        (16 << 10)
>>> +/* 8KB from GUC_WOPCM_RESERVED is reserved for GuC stack. */
>>> +#define GUC_WOPCM_STACK_RESERVED    (8 << 10)
>>> +/* 24KB at the end of GuC WOPCM is reserved for RC6 CTX on BXT. */
>>> +#define BXT_GUC_WOPCM_RC6_CTX_RESERVED    (24 << 10)
>>
>> Hmm, I'm still not convinced that we correctly attach it to "GuC WOPCM".
> Can you explain the reason and more about your concerns?

Spec says that CTX reserved region is separate from GuC region and is
calculated "against the address of the top of WOPCM".

>>
>> In the result, maybe we should stop focusing on "intel_guc_wopcm" and  
>> define
>> everything as "intel_wopcm" as it was earlier suggested by Joonas.
>>
>> Then we can define our struct to match diagram above as
>>
>> struct intel_wopcm {
>>     u32 size;
>>     struct {
>>         u32 base;
>>         u32 size;
>>     } guc;
>> };
>>
> Thanks Michal! but I don't think this is quite clean design. two reason:
> 0) I agree there should be something like intel_wopcm to describe
>      the overall wopcm info, that what I did in my v1 patch series.
>      the question is whether we really need it even if we are using
>      only static wopcm size value?

Are you sure that WOPCM size is same for all platforms ?

> I think it should be more clear to
>      introduce intel_wopcm as  a part of a patch that supports dynamic
>      wopcm size calculation.

This is exactly what I'm suggesting. Note that spec uses "arbiter" term,
and we can use struct intel_wopcm as placeholder for the partitioning  
results.

> 2) the wopcm region (a.k.a partition) is definitely a concept that should
>      exist at least in uc layer.

I'm not so sure. But definitely it should not be guc only concept.

> if we chose not to init uc/guc, it would be
>      nonsense to init these partitions/info in an intel_wopcm which  
> attached to
>      drm_i915_private and we have initialized a part of this struct  
> (e.g. size).

Why nonsense? Since WOPCM is used/needed by several entities (agents)
then it is obvious that to properly partition wopcm into regions that
will satisfy all agents, arbiter need to know their specific requirements
(like fw sizes in case of HuC or GuC).

>      to make it clean I will insist to have the guc wopcm region (or  
> maybe
>      the other region info) kept in uc level.

It will not be clean if you make calculation in one place and keep partial
results in other places. We can at most setup hw from uc code.

> As I said the purpose of this series is to enable dynamic GuC WOPCM  
> offset
> and size calculation.

Sure. but and all I want is to do it in a right place.

> it's not targeting any code refactoring and only serves
> as a new feature enabling patch. The design principle of this series was  
> to
> provide clear new feature enabling by:
> 1) align with current code design and implementation.
> 2) provide correct hardware abstraction.
> 3) faultlessly enabled these new features. (e.g. dynamic size/offset  
> calculation)
> I think this series is now in a good shape to meet all above targets.
>

So we need arbiter :)

> On the other hand, I do agree there always is some room to make our  
> current
> code clearer, but what we are talking about is further code refactoring.
> Actually, I've already had some ideas of this. e.g. we could have some  
> new
> abstractions such as:
>
> struct intel_wopcm {
>      u32 size;
>      /*any other global wopcm info*/
> }
>
> struct wopcm_region {
>      u32 reserved; // reserved size in each region
>      u32 offset; // offset of each region
>      u32 size; // size of each region
> };
>
> struct intel_uc {
>      struct wopcm_regions regions[];
>      struct intel_uc_fw fws[];
>      struct intel_guc guc;
>      ...
> }
>
> struct intel_guc_dma {
>      u32 fw_domains;
>      lockable_reg size;
>      lockable_reg offset;
>      u32 flags; // e.g. loading HuC.
> }
>
> guc_dma_init()
> guc_dma_hw_init()
> guc_dma_xfer()
>
> struct intel_guc {
>      struct intel_guc_dma guc_dma;
>      /*other guc objects*/
> }
>
> This would provide better software layer abstraction. but what I learned
> from the 10 versions code submission is we need make things clear enough  
> to
> move forward. The lack of uc level abstraction is probably the reason  
> why we
> felt so frustrating about this part of code.

Yep. struct intel_uc will likely make few things cleaner.

>
> Can we just move forward by aligning to the current code implementation?
> since what we need now is enable this new features. and we definitely can
> provide more code refactoring patches based on these changes later.

This is question to the maintainers.

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

Re: [PATCH v10 3/7] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

[Yaodong Li]

On 02/13/2018 07:56 AM, Michal Wajdeczko wrote:
>
> Also, comparing again your drawing with the spec I think it
> should look like this:
>
>    +-------- +====================+ <== WOPCM top
>   /|\        |     HW RESERVED    |
>    |         +------------------- +
>    |         |                    |
>    |     +-- +====================+ <== GuC WOPCM top
>    |    /|\  |                    |
>    |     |   |                    |
>    |    GuC  |                    |
>    |   WOPCM |    GuC firmware    |
>    |    size |                    |
>  WOPCM   |   +--------------------+
>   size  \|/  |    GuC reserved    |
>    |     +-- +====================+ <== GuC WOPCM base (offset from 
> WOPCM base)
>    |         |   WOPCM RESERVED   |
>    |         +--------------------+
>   \|/        |    HuC firmware    |
>    +-------- +====================+ <== WOPCM base
>
>
>> +
>> +/* GUC WOPCM offset needs to be 16KB aligned. */
>> +#define GUC_WOPCM_OFFSET_ALIGNMENT    (16 << 10)
>> +/* 16KB reserved at the beginning of GuC WOPCM. */
>> +#define GUC_WOPCM_RESERVED        (16 << 10)
>> +/* 8KB from GUC_WOPCM_RESERVED is reserved for GuC stack. */
>> +#define GUC_WOPCM_STACK_RESERVED    (8 << 10)
>> +/* 24KB at the end of GuC WOPCM is reserved for RC6 CTX on BXT. */
>> +#define BXT_GUC_WOPCM_RC6_CTX_RESERVED    (24 << 10)
>
> Hmm, I'm still not convinced that we correctly attach it to "GuC WOPCM".
>
> In the result, maybe we should stop focusing on "intel_guc_wopcm" and 
> define
> everything as "intel_wopcm" as it was earlier suggested by Joonas.
>
> Then we can define our struct to match diagram above as
>
> struct intel_wopcm {
>     u32 size;
>     struct {
>         u32 base;
>         u32 size;
>     } guc;
> };
>
> u32 intel_wopcm_guc_top(struct intel_wopcm *wopcm)
> {
>     return wopcm->guc.base + wopcm->guc.size;
> }
More comments about the *top*:-)

The *top* we used in driver code to verify where the non-wopcm guc memory
allocation starts is the offset value from guc.base to WOPCM_TOP. so we 
don't need to
add the guc.base to it.

 From GuC point of view the *top* (boundary between wopcm and non-wopcm 
memory)
is like:
+------------------+ <=== GuC memory end

    Non WOPCM

+------------------+ <=== *top* (This is actually in top of the whole WOPCM)
      CTX Rsvd
+------------------+ <=== *__top*  (wopcm->guc.size)
       WOPCM
+------------------+ <=== guc base (wopcm->guc.base)

actually, we had a discussion whether we should set the guc wopcm and 
non-wopcm
boundary to *__top* but the suggestion we got is to stick to the spec 
description which
says to allocate "non-wopcm memory starts from WOPCM TOP"(even if it seems
we could use *__top* as the boundary:-)). and it's because of this 
reason we called the
wopcm from guc_base to *top* as GuC WOPCM since we count the wopcm between
*__top* to *top* as a part of GuC address space while trying to allocate 
non-wopcm
for GuC (even though GuC won't access CTX Rsvd at allO:-)).

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

Re: [PATCH v10 3/7] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

[Michal Wajdeczko]

On Wed, 14 Feb 2018 03:26:12 +0100, Yaodong Li <yaodong.li@intel.com>  
wrote:

>
> On 02/13/2018 07:56 AM, Michal Wajdeczko wrote:
>>
>> Also, comparing again your drawing with the spec I think it
>> should look like this:
>>
>>    +-------- +====================+ <== WOPCM top
>>   /|\        |     HW RESERVED    |
>>    |         +------------------- +
>>    |         |                    |
>>    |     +-- +====================+ <== GuC WOPCM top
>>    |    /|\  |                    |
>>    |     |   |                    |
>>    |    GuC  |                    |
>>    |   WOPCM |    GuC firmware    |
>>    |    size |                    |
>>  WOPCM   |   +--------------------+
>>   size  \|/  |    GuC reserved    |
>>    |     +-- +====================+ <== GuC WOPCM base (offset from  
>> WOPCM base)
>>    |         |   WOPCM RESERVED   |
>>    |         +--------------------+
>>   \|/        |    HuC firmware    |
>>    +-------- +====================+ <== WOPCM base
>>
>>
>>> +
>>> +/* GUC WOPCM offset needs to be 16KB aligned. */
>>> +#define GUC_WOPCM_OFFSET_ALIGNMENT    (16 << 10)
>>> +/* 16KB reserved at the beginning of GuC WOPCM. */
>>> +#define GUC_WOPCM_RESERVED        (16 << 10)
>>> +/* 8KB from GUC_WOPCM_RESERVED is reserved for GuC stack. */
>>> +#define GUC_WOPCM_STACK_RESERVED    (8 << 10)
>>> +/* 24KB at the end of GuC WOPCM is reserved for RC6 CTX on BXT. */
>>> +#define BXT_GUC_WOPCM_RC6_CTX_RESERVED    (24 << 10)
>>
>> Hmm, I'm still not convinced that we correctly attach it to "GuC WOPCM".
>>
>> In the result, maybe we should stop focusing on "intel_guc_wopcm" and  
>> define
>> everything as "intel_wopcm" as it was earlier suggested by Joonas.
>>
>> Then we can define our struct to match diagram above as
>>
>> struct intel_wopcm {
>>     u32 size;
>>     struct {
>>         u32 base;
>>         u32 size;
>>     } guc;
>> };
>>
>> u32 intel_wopcm_guc_top(struct intel_wopcm *wopcm)
>> {
>>     return wopcm->guc.base + wopcm->guc.size;
>> }
> More comments about the *top*:-)
>
> The *top* we used in driver code to verify where the non-wopcm guc memory
> allocation starts is the offset value from guc.base to WOPCM_TOP. so we  
> don't need to
> add the guc.base to it.
>
>  From GuC point of view the *top* (boundary between wopcm and non-wopcm  
> memory)
> is like:
> +------------------+ <=== GuC memory end
>
>     Non WOPCM
>
> +------------------+ <=== *top* (This is actually in top of the whole  
> WOPCM)
>       CTX Rsvd
> +------------------+ <=== *__top*  (wopcm->guc.size)
>        WOPCM
> +------------------+ <=== guc base (wopcm->guc.base)
>
> actually, we had a discussion whether we should set the guc wopcm and  
> non-wopcm
> boundary to *__top* but the suggestion we got is to stick to the spec  
> description which
> says to allocate "non-wopcm memory starts from WOPCM TOP"(even if it  
> seems
> we could use *__top* as the boundary:-)). and it's because of this  
> reason we called the
> wopcm from guc_base to *top* as GuC WOPCM since we count the wopcm  
> between
> *__top* to *top* as a part of GuC address space while trying to allocate  
> non-wopcm
> for GuC (even though GuC won't access CTX Rsvd at allO:-)).

Ok, so we should program GUC_WOPCM_SIZE to wopcm->guc.size and then
use (wopcm->size - wopcm->guc.base) as offset to i915_vma_pin, right?

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

Re: [PATCH v10 3/7] drm/i915/guc: Implement dynamic GuC WOPCM offset and size

[Yaodong Li]

On 02/14/2018 09:38 AM, Michal Wajdeczko wrote:
> On Wed, 14 Feb 2018 03:26:12 +0100, Yaodong Li <yaodong.li@intel.com> 
> wrote:
>
>>
>> On 02/13/2018 07:56 AM, Michal Wajdeczko wrote:
>>>
>>> Also, comparing again your drawing with the spec I think it
>>> should look like this:
>>>
>>>    +-------- +====================+ <== WOPCM top
>>>   /|\        |     HW RESERVED    |
>>>    |         +------------------- +
>>>    |         |                    |
>>>    |     +-- +====================+ <== GuC WOPCM top
>>>    |    /|\  |                    |
>>>    |     |   |                    |
>>>    |    GuC  |                    |
>>>    |   WOPCM |    GuC firmware    |
>>>    |    size |                    |
>>>  WOPCM   |   +--------------------+
>>>   size  \|/  |    GuC reserved    |
>>>    |     +-- +====================+ <== GuC WOPCM base (offset from 
>>> WOPCM base)
>>>    |         |   WOPCM RESERVED   |
>>>    |         +--------------------+
>>>   \|/        |    HuC firmware    |
>>>    +-------- +====================+ <== WOPCM base
>>>
>>>
>>>> +
>>>> +/* GUC WOPCM offset needs to be 16KB aligned. */
>>>> +#define GUC_WOPCM_OFFSET_ALIGNMENT    (16 << 10)
>>>> +/* 16KB reserved at the beginning of GuC WOPCM. */
>>>> +#define GUC_WOPCM_RESERVED        (16 << 10)
>>>> +/* 8KB from GUC_WOPCM_RESERVED is reserved for GuC stack. */
>>>> +#define GUC_WOPCM_STACK_RESERVED    (8 << 10)
>>>> +/* 24KB at the end of GuC WOPCM is reserved for RC6 CTX on BXT. */
>>>> +#define BXT_GUC_WOPCM_RC6_CTX_RESERVED    (24 << 10)
>>>
>>> Hmm, I'm still not convinced that we correctly attach it to "GuC 
>>> WOPCM".
>>>
>>> In the result, maybe we should stop focusing on "intel_guc_wopcm" 
>>> and define
>>> everything as "intel_wopcm" as it was earlier suggested by Joonas.
>>>
>>> Then we can define our struct to match diagram above as
>>>
>>> struct intel_wopcm {
>>>     u32 size;
>>>     struct {
>>>         u32 base;
>>>         u32 size;
>>>     } guc;
>>> };
>>>
>>> u32 intel_wopcm_guc_top(struct intel_wopcm *wopcm)
>>> {
>>>     return wopcm->guc.base + wopcm->guc.size;
>>> }
>> More comments about the *top*:-)
>>
>> The *top* we used in driver code to verify where the non-wopcm guc 
>> memory
>> allocation starts is the offset value from guc.base to WOPCM_TOP. so 
>> we don't need to
>> add the guc.base to it.
>>
>>  From GuC point of view the *top* (boundary between wopcm and 
>> non-wopcm memory)
>> is like:
>> +------------------+ <=== GuC memory end
>>
>>     Non WOPCM
>>
>> +------------------+ <=== *top* (This is actually in top of the whole 
>> WOPCM)
>>       CTX Rsvd
>> +------------------+ <=== *__top*  (wopcm->guc.size)
>>        WOPCM
>> +------------------+ <=== guc base (wopcm->guc.base)
>>
>> actually, we had a discussion whether we should set the guc wopcm and 
>> non-wopcm
>> boundary to *__top* but the suggestion we got is to stick to the spec 
>> description which
>> says to allocate "non-wopcm memory starts from WOPCM TOP"(even if it 
>> seems
>> we could use *__top* as the boundary:-)). and it's because of this 
>> reason we called the
>> wopcm from guc_base to *top* as GuC WOPCM since we count the wopcm 
>> between
>> *__top* to *top* as a part of GuC address space while trying to 
>> allocate non-wopcm
>> for GuC (even though GuC won't access CTX Rsvd at allO:-)).
>
> Ok, so we should program GUC_WOPCM_SIZE to wopcm->guc.size and then
> use (wopcm->size - wopcm->guc.base) as offset to i915_vma_pin, right?
>
All values of size and top are relevant to guc.base (offsets from 
guc.base), and currently
we are using guc.size + hole between guc.size and ctx reserved (if any) 
+ ctx reserved size
as the offset to i915_vma_pin as it's the actual WOPCM_TOP.:-)

Regards,
-Jackie

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

[PATCH v10 4/7] drm/i915/guc: Add support to return CNL specific reserved GuC WOPCM size

[Jackie Li]

CNL has its specific reserved GuC WOPCM size for RC6 and other hardware
contexts.

This patch updates the code to return CNL specific reserved GuC WOPCM size
for RC6 and other hardware contexts so that the GuC WOPCM size can be
calculated correctly for CNL.

v9:
 - Created a new patch for these changes originally made in v8 4/6 patch of
   this series (Sagar/Michal)

v10:
 - Used if-else ladder to the returning of context sizes (Joonas)

Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> (v9)
Signed-off-by: Jackie Li <yaodong.li@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_wopcm.c | 6 ++++--
 drivers/gpu/drm/i915/intel_guc_wopcm.h | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.c b/drivers/gpu/drm/i915/intel_guc_wopcm.c
index cf832b9..9a276fe 100644
--- a/drivers/gpu/drm/i915/intel_guc_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
@@ -13,8 +13,10 @@ static inline u32 context_reserved_size(struct intel_guc *guc)
 
 	if (IS_GEN9_LP(i915))
 		return BXT_GUC_WOPCM_RC6_CTX_RESERVED;
-
-	return 0;
+	else if (INTEL_GEN(i915) >= 10)
+		return CNL_GUC_WOPCM_HW_CTX_RESERVED;
+	else
+		return 0;
 }
 
 static inline int gen9_check_dword_gap(struct intel_guc_wopcm *guc_wopcm)
diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.h b/drivers/gpu/drm/i915/intel_guc_wopcm.h
index a632caa..13fcab6 100644
--- a/drivers/gpu/drm/i915/intel_guc_wopcm.h
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.h
@@ -52,6 +52,8 @@ struct intel_guc;
 #define GUC_WOPCM_STACK_RESERVED	(8 << 10)
 /* 24KB at the end of GuC WOPCM is reserved for RC6 CTX on BXT. */
 #define BXT_GUC_WOPCM_RC6_CTX_RESERVED	(24 << 10)
+/* 36KB WOPCM reserved at the end of GuC WOPCM on CNL. */
+#define CNL_GUC_WOPCM_HW_CTX_RESERVED	(36 << 10)
 
 /*
  * GuC WOPCM starts at 144KB (GUC_WOPCM_RESERVED + 128KB reserved for GuC
-- 
2.7.4

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

[PATCH v10 5/7] drm/i915/guc: Add HuC firmware size related restriction for Gen9 and CNL A0

[Jackie Li]

On CNL A0 and Gen9, there's a hardware restriction that requires the
available GuC WOPCM size to be larger than or equal to HuC firmware size.

This patch adds new verfication code to ensure the available GuC WOPCM size
to be larger than or equal to HuC firmware size on both Gen9 and CNL A0.

v6:
 - Extended HuC FW size check against GuC WOPCM size to all
   Gen9 and CNL A0 platforms

v7:
 - Fixed patch format issues

v8:
 - Renamed variables and functions to avoid ambiguity (Joonas)
 - Updated commit message and comments to be more comprehensive (Sagar)

v9:
 - Moved code that is not related to restriction check into a separate
   patch and updated the commit message accordingly (Sagar/Michal)
 - Avoided to call uc_get_fw_size for better layer isolation (Michal)

v10:
 - Shorten function names and reorganized size_check code to have clear
   isolation (Joonas)
 - Removed unnecessary comments (Joonas)

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: John Spotswood <john.a.spotswood@intel.com>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> (v8)
Signed-off-by: Jackie Li <yaodong.li@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_wopcm.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.c b/drivers/gpu/drm/i915/intel_guc_wopcm.c
index 9a276fe..0194266 100644
--- a/drivers/gpu/drm/i915/intel_guc_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
@@ -19,6 +19,20 @@ static inline u32 context_reserved_size(struct intel_guc *guc)
 		return 0;
 }
 
+static inline int check_huc_fw_fits(struct intel_guc_wopcm *guc_wopcm,
+				    u32 huc_fw_size)
+{
+	/*
+	 * 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 (guc_wopcm->size - GUC_WOPCM_RESERVED < huc_fw_size)
+		return -E2BIG;
+
+	return 0;
+}
+
 static inline int gen9_check_dword_gap(struct intel_guc_wopcm *guc_wopcm)
 {
 	u32 guc_wopcm_start;
@@ -40,15 +54,19 @@ static inline int gen9_check_dword_gap(struct intel_guc_wopcm *guc_wopcm)
 	return 0;
 }
 
-static inline int guc_wopcm_size_check(struct intel_guc *guc)
+static inline int guc_wopcm_size_check(struct intel_guc *guc, u32 huc_fw_size)
 {
 	struct drm_i915_private *i915 = guc_to_i915(guc);
 	struct intel_guc_wopcm *guc_wopcm = &guc->wopcm;
+	int err = 0;
 
 	if (IS_GEN9(i915))
-		return gen9_check_dword_gap(guc_wopcm);
+		err = gen9_check_dword_gap(guc_wopcm);
 
-	return 0;
+	if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0))
+		err = check_huc_fw_fits(guc_wopcm, huc_fw_size);
+
+	return err;
 }
 
 /**
@@ -121,7 +139,7 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, u32 guc_fw_size,
 	guc->wopcm.size = size;
 	guc->wopcm.top = top;
 
-	err = guc_wopcm_size_check(guc);
+	err = guc_wopcm_size_check(guc, huc_fw_size);
 	if (err) {
 		DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n");
 		return err;
-- 
2.7.4

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

Re: [PATCH v10 5/7] drm/i915/guc: Add HuC firmware size related restriction for Gen9 and CNL A0

[Michal Wajdeczko]

On Tue, 13 Feb 2018 00:45:51 +0100, Jackie Li <yaodong.li@intel.com> wrote:

> On CNL A0 and Gen9, there's a hardware restriction that requires the
> available GuC WOPCM size to be larger than or equal to HuC firmware size.
>
> This patch adds new verfication code to ensure the available GuC WOPCM  
> size
> to be larger than or equal to HuC firmware size on both Gen9 and CNL A0.
>
> v6:
>  - Extended HuC FW size check against GuC WOPCM size to all
>    Gen9 and CNL A0 platforms
>
> v7:
>  - Fixed patch format issues
>
> v8:
>  - Renamed variables and functions to avoid ambiguity (Joonas)
>  - Updated commit message and comments to be more comprehensive (Sagar)
>
> v9:
>  - Moved code that is not related to restriction check into a separate
>    patch and updated the commit message accordingly (Sagar/Michal)
>  - Avoided to call uc_get_fw_size for better layer isolation (Michal)
>
> v10:
>  - Shorten function names and reorganized size_check code to have clear
>    isolation (Joonas)
>  - Removed unnecessary comments (Joonas)
>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: John Spotswood <john.a.spotswood@intel.com>
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> (v8)
> Signed-off-by: Jackie Li <yaodong.li@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_wopcm.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.c  
> b/drivers/gpu/drm/i915/intel_guc_wopcm.c
> index 9a276fe..0194266 100644
> --- a/drivers/gpu/drm/i915/intel_guc_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
> @@ -19,6 +19,20 @@ static inline u32 context_reserved_size(struct  
> intel_guc *guc)
>  		return 0;
>  }
> +static inline int check_huc_fw_fits(struct intel_guc_wopcm *guc_wopcm,
> +				    u32 huc_fw_size)
> +{
> +	/*
> +	 * 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 (guc_wopcm->size - GUC_WOPCM_RESERVED < huc_fw_size)

What if guc_wopcm->size < GUC_WOPCM_RESERVED ?

> +		return -E2BIG;
> +
> +	return 0;
> +}
> +
>  static inline int gen9_check_dword_gap(struct intel_guc_wopcm  
> *guc_wopcm)
>  {
>  	u32 guc_wopcm_start;
> @@ -40,15 +54,19 @@ static inline int gen9_check_dword_gap(struct  
> intel_guc_wopcm *guc_wopcm)
>  	return 0;
>  }
> -static inline int guc_wopcm_size_check(struct intel_guc *guc)
> +static inline int guc_wopcm_size_check(struct intel_guc *guc, u32  
> huc_fw_size)
>  {
>  	struct drm_i915_private *i915 = guc_to_i915(guc);
>  	struct intel_guc_wopcm *guc_wopcm = &guc->wopcm;
> +	int err = 0;
> 	if (IS_GEN9(i915))
> -		return gen9_check_dword_gap(guc_wopcm);
> +		err = gen9_check_dword_gap(guc_wopcm);
> -	return 0;
> +	if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0))
> +		err = check_huc_fw_fits(guc_wopcm, huc_fw_size);

Hmm, what if gen9_check_dword_gap() fails but check_huc_fw_fits() passes ?

> +
> +	return err;
>  }
> /**
> @@ -121,7 +139,7 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm  
> *guc_wopcm, u32 guc_fw_size,
>  	guc->wopcm.size = size;
>  	guc->wopcm.top = top;
> -	err = guc_wopcm_size_check(guc);
> +	err = guc_wopcm_size_check(guc, huc_fw_size);
>  	if (err) {
>  		DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n");
>  		return err;

I'm more and more convinced that we should use "intel_wopcm" to make
partition and all these checks

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

Re: [PATCH v10 5/7] drm/i915/guc: Add HuC firmware size related restriction for Gen9 and CNL A0

[Yaodong Li]

On 02/13/2018 08:06 AM, Michal Wajdeczko wrote:
> +static inline int check_huc_fw_fits(struct intel_guc_wopcm *guc_wopcm,
>> +                    u32 huc_fw_size)
>> +{
>> +    /*
>> +     * 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 (guc_wopcm->size - GUC_WOPCM_RESERVED < huc_fw_size)
>
> What if guc_wopcm->size < GUC_WOPCM_RESERVED ?
>
we've already had following check before this. which had guaranteed
guc_wopcm->size >= guc_fw_size + reserved, thus,
guc_wopcm->size > GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED
so, guc_wopcm->size > GUC_WOPCM_RESERVED:-)

     reserved = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;
     if ((guc_fw_size + reserved) > guc_wopcm->size) {
         DRM_DEBUG_DRIVER("No enough GuC WOPCM for GuC firmware.\n");
         return -E2BIG;
     }
>> +        return -E2BIG;
>> +
>> +    return 0;
>> +}
>> +
>>  static inline int gen9_check_dword_gap(struct intel_guc_wopcm 
>> *guc_wopcm)
>>  {
>>      u32 guc_wopcm_start;
>> @@ -40,15 +54,19 @@ static inline int gen9_check_dword_gap(struct 
>> intel_guc_wopcm *guc_wopcm)
>>      return 0;
>>  }
>> -static inline int guc_wopcm_size_check(struct intel_guc *guc)
>> +static inline int guc_wopcm_size_check(struct intel_guc *guc, u32 
>> huc_fw_size)
>>  {
>>      struct drm_i915_private *i915 = guc_to_i915(guc);
>>      struct intel_guc_wopcm *guc_wopcm = &guc->wopcm;
>> +    int err = 0;
>>     if (IS_GEN9(i915))
>> -        return gen9_check_dword_gap(guc_wopcm);
>> +        err = gen9_check_dword_gap(guc_wopcm);
>> -    return 0;
>> +    if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, 
>> CNL_REVID_A0))
>> +        err = check_huc_fw_fits(guc_wopcm, huc_fw_size);
>
> Hmm, what if gen9_check_dword_gap() fails but check_huc_fw_fits() 
> passes ?
>
oops! will fix this.:-)
>> +
>> +    return err;
>>  }
>> /**
>> @@ -121,7 +139,7 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm 
>> *guc_wopcm, u32 guc_fw_size,
>>      guc->wopcm.size = size;
>>      guc->wopcm.top = top;
>> -    err = guc_wopcm_size_check(guc);
>> +    err = guc_wopcm_size_check(guc, huc_fw_size);
>>      if (err) {
>>          DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n");
>>          return err;
>
> I'm more and more convinced that we should use "intel_wopcm" to make
> partition and all these checks
>
These are all GuC wopcm related, it only checks guc wopcm size. so I am 
wondering whether
I am still misunderstanding anything here?since the purpose of these 
calculation and checks are
clearly all GuC related. To be more precisely GuC DMA related. The 
driver's responsibility is to
calculate the GuC DMA offset and GuC wopcm size values based on guc/huc 
fw sizes and
all these checks are all for the correctness for the GuC  wopcm size.
I don't see any reason why these should be a part of global intel_wopcm 
even if we really
want to keep something like wopcm_regions for both guc/huc(though I 
still don't know what
the benefit real is to keep something like HuC wopcm region except for 
sth like debugfs output?).
even in this case, we still at least keep these as a part of  GuC since 
we really need it to setup
GuC HW :- Or do you mean we should create an intel_wopcm to carry info 
such as
global WOPCM size,guc_fw_size and huc_fw_size? Sorry I am really a 
little bit confused here:-(
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v10 5/7] drm/i915/guc: Add HuC firmware size related restriction for Gen9 and CNL A0

[Michal Wajdeczko]

On Tue, 13 Feb 2018 22:50:53 +0100, Yaodong Li <yaodong.li@intel.com>  
wrote:

> On 02/13/2018 08:06 AM, Michal Wajdeczko wrote:
>> +static inline int check_huc_fw_fits(struct intel_guc_wopcm *guc_wopcm,
>>> +                    u32 huc_fw_size)
>>> +{
>>> +    /*
>>> +     * 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 (guc_wopcm->size - GUC_WOPCM_RESERVED < huc_fw_size)
>>
>> What if guc_wopcm->size < GUC_WOPCM_RESERVED ?
>>
> we've already had following check before this. which had guaranteed
> guc_wopcm->size >= guc_fw_size + reserved, thus,
> guc_wopcm->size > GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED
> so, guc_wopcm->size > GUC_WOPCM_RESERVED:-)
>
>      reserved = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;
>      if ((guc_fw_size + reserved) > guc_wopcm->size) {
>          DRM_DEBUG_DRIVER("No enough GuC WOPCM for GuC firmware.\n");
>          return -E2BIG;
>      }

Hmm, that only proves that current partitioning code is too complicated :)
If you look at diagram it should be possible to implement it as

guc_base = ALIGN(huc_fw_size + WOPCM_RESERVED, KiB(16));
guc_size = ALIGN(guc_fw_size + GUC_WOPCM_RESERVED, KiB(4))
reserved = context_reserved_size(i915);

if (guc_base + guc_size + reserved > WOPCM_DEFAULT_SIZE)
	return -E2BIG;

(E&O)

>>> +        return -E2BIG;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>  static inline int gen9_check_dword_gap(struct intel_guc_wopcm  
>>> *guc_wopcm)
>>>  {
>>>      u32 guc_wopcm_start;
>>> @@ -40,15 +54,19 @@ static inline int gen9_check_dword_gap(struct  
>>> intel_guc_wopcm *guc_wopcm)
>>>      return 0;
>>>  }
>>> -static inline int guc_wopcm_size_check(struct intel_guc *guc)
>>> +static inline int guc_wopcm_size_check(struct intel_guc *guc, u32  
>>> huc_fw_size)
>>>  {
>>>      struct drm_i915_private *i915 = guc_to_i915(guc);
>>>      struct intel_guc_wopcm *guc_wopcm = &guc->wopcm;
>>> +    int err = 0;
>>>     if (IS_GEN9(i915))
>>> -        return gen9_check_dword_gap(guc_wopcm);
>>> +        err = gen9_check_dword_gap(guc_wopcm);
>>> -    return 0;
>>> +    if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0,  
>>> CNL_REVID_A0))
>>> +        err = check_huc_fw_fits(guc_wopcm, huc_fw_size);
>>
>> Hmm, what if gen9_check_dword_gap() fails but check_huc_fw_fits()  
>> passes ?
>>
> oops! will fix this.:-)
>>> +
>>> +    return err;
>>>  }
>>> /**
>>> @@ -121,7 +139,7 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm  
>>> *guc_wopcm, u32 guc_fw_size,
>>>      guc->wopcm.size = size;
>>>      guc->wopcm.top = top;
>>> -    err = guc_wopcm_size_check(guc);
>>> +    err = guc_wopcm_size_check(guc, huc_fw_size);
>>>      if (err) {
>>>          DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n");
>>>          return err;
>>
>> I'm more and more convinced that we should use "intel_wopcm" to make
>> partition and all these checks
>>
> These are all GuC wopcm related, it only checks guc wopcm size. so I am  
> wondering whether
> I am still misunderstanding anything here?since the purpose of these  
> calculation and checks are
> clearly all GuC related. To be more precisely GuC DMA related. The  
> driver's responsibility is to
> calculate the GuC DMA offset and GuC wopcm size values based on guc/huc  
> fw sizes and
> all these checks are all for the correctness for the GuC  wopcm size.
> I don't see any reason why these should be a part of global intel_wopcm  
> even if we really
> want to keep something like wopcm_regions for both guc/huc(though I  
> still don't know what
> the benefit real is to keep something like HuC wopcm region except for  
> sth like debugfs output?).
> even in this case, we still at least keep these as a part of  GuC since  
> we really need it to setup
> GuC HW :- Or do you mean we should create an intel_wopcm to carry info  
> such as
> global WOPCM size,guc_fw_size and huc_fw_size? Sorry I am really a  
> little bit confused here:-(

struct intel_wopcm should carry only results of WOPCM partitioning between
all agents including GuC. There is no need to carry fw sizes as those are
only needed as input for calculations.

You can still program GuC region from uc code.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v10 5/7] drm/i915/guc: Add HuC firmware size related restriction for Gen9 and CNL A0

[Yaodong Li]

On 02/13/2018 02:59 PM, Michal Wajdeczko wrote:
> Hmm, that only proves that current partitioning code is too 
> complicated :)
> If you look at diagram it should be possible to implement it as
current calculation tries to set the maximum available WOPCM to avoid
Gen9 limitations. that the reason I didn't use guc_fw_size + 
GUC_WOPCM_RESERVED
for size calculation.:-)
>
> guc_base = ALIGN(huc_fw_size + WOPCM_RESERVED, KiB(16));
the same as current calculation.
> guc_size = ALIGN(guc_fw_size + GUC_WOPCM_RESERVED, KiB(4))
it's sample but likely run into gen9 gaps HW restriction.:-)
> reserved = context_reserved_size(i915);
>
> if (guc_base + guc_size + reserved > WOPCM_DEFAULT_SIZE)
>     return -E2BIG;
>
> (E&O)
>

>>>> @@ -121,7 +139,7 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm 
>>>> *guc_wopcm, u32 guc_fw_size,
>>>>      guc->wopcm.size = size;
>>>>      guc->wopcm.top = top;
>>>> -    err = guc_wopcm_size_check(guc);
>>>> +    err = guc_wopcm_size_check(guc, huc_fw_size);
>>>>      if (err) {
>>>>          DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n");
>>>>          return err;
>>>
>>> I'm more and more convinced that we should use "intel_wopcm" to make
>>> partition and all these checks
>>>
>> These are all GuC wopcm related, it only checks guc wopcm size. so I 
>> am wondering whether
>> I am still misunderstanding anything here?since the purpose of these 
>> calculation and checks are
>> clearly all GuC related. To be more precisely GuC DMA related. The 
>> driver's responsibility is to
>> calculate the GuC DMA offset and GuC wopcm size values based on 
>> guc/huc fw sizes and
>> all these checks are all for the correctness for the GuC  wopcm size.
>> I don't see any reason why these should be a part of global 
>> intel_wopcm even if we really
>> want to keep something like wopcm_regions for both guc/huc(though I 
>> still don't know what
>> the benefit real is to keep something like HuC wopcm region except 
>> for sth like debugfs output?).
>> even in this case, we still at least keep these as a part of GuC 
>> since we really need it to setup
>> GuC HW :- Or do you mean we should create an intel_wopcm to carry 
>> info such as
>> global WOPCM size,guc_fw_size and huc_fw_size? Sorry I am really a 
>> little bit confused here:-(
>
> struct intel_wopcm should carry only results of WOPCM partitioning 
> between
> all agents including GuC. There is no need to carry fw sizes as those are
> only needed as input for calculations.
>
I understand the point. but what I am trying to explain is that we can 
only focus on GuC WOPCM setting since there's no
need to keep tracking other regions since they are just results of 
setting of GuC WOPCM registers (what we are
trying to do is to figure out a window on the WOPCM for GuC, and we 
don't need to keep tracking info such as
HuC WOPCM size, CTX reserved WOPCM size because KMD driver won't use 
these info anymore).

If you do think it's clearer to have something like 
intel_uc_wopcm.h/intel_uc_wopcm.c I can sure make these changes,
but what I am saying is we won't need to keep likely unused info data in 
KMD. And we always can treat the other parts
of WOPCM as reserved for other HW use. and only take care of what KMD 
needs to do. Please let me know if you
still think we should have something like uc_wopcm. I will work it out.

Regards,
-Jackie






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

Re: [PATCH v10 5/7] drm/i915/guc: Add HuC firmware size related restriction for Gen9 and CNL A0

[Michal Wajdeczko]

On Wed, 14 Feb 2018 01:41:57 +0100, Yaodong Li <yaodong.li@intel.com>  
wrote:

>
>
> On 02/13/2018 02:59 PM, Michal Wajdeczko wrote:
>> Hmm, that only proves that current partitioning code is too complicated  
>> :)
>> If you look at diagram it should be possible to implement it as
> current calculation tries to set the maximum available WOPCM to avoid
> Gen9 limitations. that the reason I didn't use guc_fw_size +  
> GUC_WOPCM_RESERVED
> for size calculation.:-)
>>
>> guc_base = ALIGN(huc_fw_size + WOPCM_RESERVED, KiB(16));
> the same as current calculation.

but definitely simpler and easier to review ;)

>> guc_size = ALIGN(guc_fw_size + GUC_WOPCM_RESERVED, KiB(4))
> it's sample but likely run into gen9 gaps HW restriction.:-)

feel free to add/fix it here ;)

>> reserved = context_reserved_size(i915);
>>
>> if (guc_base + guc_size + reserved > WOPCM_DEFAULT_SIZE)
>>     return -E2BIG;
>>
>> (E&O)
>>
>
>>>>> @@ -121,7 +139,7 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm  
>>>>> *guc_wopcm, u32 guc_fw_size,
>>>>>      guc->wopcm.size = size;
>>>>>      guc->wopcm.top = top;
>>>>> -    err = guc_wopcm_size_check(guc);
>>>>> +    err = guc_wopcm_size_check(guc, huc_fw_size);
>>>>>      if (err) {
>>>>>          DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n");
>>>>>          return err;
>>>>
>>>> I'm more and more convinced that we should use "intel_wopcm" to make
>>>> partition and all these checks
>>>>
>>> These are all GuC wopcm related, it only checks guc wopcm size. so I  
>>> am wondering whether
>>> I am still misunderstanding anything here?since the purpose of these  
>>> calculation and checks are
>>> clearly all GuC related. To be more precisely GuC DMA related. The  
>>> driver's responsibility is to
>>> calculate the GuC DMA offset and GuC wopcm size values based on  
>>> guc/huc fw sizes and
>>> all these checks are all for the correctness for the GuC  wopcm size.
>>> I don't see any reason why these should be a part of global  
>>> intel_wopcm even if we really
>>> want to keep something like wopcm_regions for both guc/huc(though I  
>>> still don't know what
>>> the benefit real is to keep something like HuC wopcm region except for  
>>> sth like debugfs output?).
>>> even in this case, we still at least keep these as a part of GuC since  
>>> we really need it to setup
>>> GuC HW :- Or do you mean we should create an intel_wopcm to carry info  
>>> such as
>>> global WOPCM size,guc_fw_size and huc_fw_size? Sorry I am really a  
>>> little bit confused here:-(
>>
>> struct intel_wopcm should carry only results of WOPCM partitioning  
>> between
>> all agents including GuC. There is no need to carry fw sizes as those  
>> are
>> only needed as input for calculations.
>>
> I understand the point. but what I am trying to explain is that we can  
> only focus on GuC WOPCM setting since there's no
> need to keep tracking other regions since they are just results of  
> setting of GuC WOPCM registers

Wrong, start thinking that values used to program GuC WOPCM registers
are derived from WOPCM partitioning that was done after taking into
account HuC WOPCM size, CTX reserved WOPCM, etc.

Then you can avoid all these tricky reverse calculation that you have.

> (what we are
> trying to do is to figure out a window on the WOPCM for GuC, and we  
> don't need to keep tracking info such as
> HuC WOPCM size, CTX reserved WOPCM size because KMD driver won't use  
> these info anymore).
>
> If you do think it's clearer to have something like  
> intel_uc_wopcm.h/intel_uc_wopcm.c I can sure make these changes,
> but what I am saying is we won't need to keep likely unused info data in  
> KMD. And we always can treat the other parts
> of WOPCM as reserved for other HW use. and only take care of what KMD  
> needs to do. Please let me know if you
> still think we should have something like uc_wopcm. I will work it out.

I'm looking for intel_wopcm that will do partitioning and hold results.
Then you can program GuC WOPCM in static function inside intel_uc.c using
values from intel_wopcm that we guarantee to be *always* valid at that
point.

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

Re: [PATCH v10 5/7] drm/i915/guc: Add HuC firmware size related restriction for Gen9 and CNL A0

[Yaodong Li]

On 02/14/2018 09:24 AM, Michal Wajdeczko wrote:
> On Wed, 14 Feb 2018 01:41:57 +0100, Yaodong Li <yaodong.li@intel.com> 
> wrote:
>
>>
>>
>> On 02/13/2018 02:59 PM, Michal Wajdeczko wrote:
>>> Hmm, that only proves that current partitioning code is too 
>>> complicated :)
>>> If you look at diagram it should be possible to implement it as
>> current calculation tries to set the maximum available WOPCM to avoid
>> Gen9 limitations. that the reason I didn't use guc_fw_size + 
>> GUC_WOPCM_RESERVED
>> for size calculation.:-)
>>>
>>> guc_base = ALIGN(huc_fw_size + WOPCM_RESERVED, KiB(16));
>> the same as current calculation.
>
> but definitely simpler and easier to review ;)
>
>>> guc_size = ALIGN(guc_fw_size + GUC_WOPCM_RESERVED, KiB(4))
>> it's sample but likely run into gen9 gaps HW restriction.:-)
>
> feel free to add/fix it here ;)
It will likely fall into the same steps. But sure will be
>
>>> reserved = context_reserved_size(i915);
>>>
>>> if (guc_base + guc_size + reserved > WOPCM_DEFAULT_SIZE)
>>>     return -E2BIG;
>>>
>>> (E&O)
>>>
>>
>>>>>> @@ -121,7 +139,7 @@ int intel_guc_wopcm_init(struct 
>>>>>> intel_guc_wopcm *guc_wopcm, u32 guc_fw_size,
>>>>>>      guc->wopcm.size = size;
>>>>>>      guc->wopcm.top = top;
>>>>>> -    err = guc_wopcm_size_check(guc);
>>>>>> +    err = guc_wopcm_size_check(guc, huc_fw_size);
>>>>>>      if (err) {
>>>>>>          DRM_DEBUG_DRIVER("GuC WOPCM size check failed.\n");
>>>>>>          return err;
>>>>>
>>>>> I'm more and more convinced that we should use "intel_wopcm" to make
>>>>> partition and all these checks
>>>>>
>>>> These are all GuC wopcm related, it only checks guc wopcm size. so 
>>>> I am wondering whether
>>>> I am still misunderstanding anything here?since the purpose of 
>>>> these calculation and checks are
>>>> clearly all GuC related. To be more precisely GuC DMA related. The 
>>>> driver's responsibility is to
>>>> calculate the GuC DMA offset and GuC wopcm size values based on 
>>>> guc/huc fw sizes and
>>>> all these checks are all for the correctness for the GuC wopcm size.
>>>> I don't see any reason why these should be a part of global 
>>>> intel_wopcm even if we really
>>>> want to keep something like wopcm_regions for both guc/huc(though I 
>>>> still don't know what
>>>> the benefit real is to keep something like HuC wopcm region except 
>>>> for sth like debugfs output?).
>>>> even in this case, we still at least keep these as a part of GuC 
>>>> since we really need it to setup
>>>> GuC HW :- Or do you mean we should create an intel_wopcm to carry 
>>>> info such as
>>>> global WOPCM size,guc_fw_size and huc_fw_size? Sorry I am really a 
>>>> little bit confused here:-(
>>>
>>> struct intel_wopcm should carry only results of WOPCM partitioning 
>>> between
>>> all agents including GuC. There is no need to carry fw sizes as 
>>> those are
>>> only needed as input for calculations.
>>>
>> I understand the point. but what I am trying to explain is that we 
>> can only focus on GuC WOPCM setting since there's no
>> need to keep tracking other regions since they are just results of 
>> setting of GuC WOPCM registers
>
> Wrong, start thinking that values used to program GuC WOPCM registers
> are derived from WOPCM partitioning that was done after taking into
> account HuC WOPCM size, CTX reserved WOPCM, etc.
>
> Then you can avoid all these tricky reverse calculation that you have.

Hmm.I don't think an abstraction would be wrong or right.;-)

In this case, I don't think the abstraction of all WOPCM regions is a 
wise thing to do because
it's unnecessary to keep tracking these only results to more memory unit 
to store
redundant data, especially after we known for sure that no one would use 
these data because
they are likely to consumed by some hw components that are transparent 
to kernel
mode driver or no hw would access them at all - so why bother? this is 
the question I had and
I believe this is the main reason I was not convinced by the idea of 
partitioning and had switched
to current implementation. (Actually, I had similar thought sin my 
earlier patches:-), and I even had
a patch which already has similar implementations, so It would be very 
fast for me to switch to these
after I am convinced).

I really respect every comments and really appreciate each correction. 
and I would even appreciate
more if I was totally convinced by good ideas:-)

Regards,
-Jackie

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

[PATCH v10 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Jackie Li]

GuC WOPCM registers are write-once registers. Current driver code accesses
these registers without checking the accessibility to these registers which
will lead to unpredictable driver behaviors if these registers were touch
by other components (such as faulty BIOS code).

This patch moves the GuC WOPCM registers updating code into
intel_guc_wopcm.c and adds check before and after the update to GuC WOPCM
registers so that we can make sure the driver is in a known state before
and after writing to these write-once registers.

v6:
 - Made sure module reloading won't bug the kernel while doing
   locking status checking

v7:
 - Fixed patch format issues

v8:
 - Fixed coding style issue on register lock bit macro definition (Sagar)

v9:
 - Avoided to use redundant !! to cast uint to bool (Chris)
 - Return error code instead of GEM_BUG_ON for locked with invalid register
   values case (Sagar)
 - Updated guc_wopcm_hw_init to use guc_wopcm as first parameter (Michal)
 - Added code to set and validate the HuC_LOADING_AGENT_GUC bit in GuC
   WOPCM offset register based on the presence of HuC firmware (Michal)
 - Use bit fields instead of macros for GuC WOPCM flags (Michal)

v10:
 - Refined variable names, removed reduntant comments (Joonas)
 - Introduced lockable_reg to handle the write once register write and
   propagate the write error to caller (Joonas)
 - Used lockable_reg abstraction to avoid locking bit check on generic
   i915_reg_t (Michal)
 - Added log message for error paths (Michal)
 - Removed hw_updated flag and only relies on real hardware status

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Jackie Li <yaodong.li@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_reg.h   |   2 +
 drivers/gpu/drm/i915/intel_guc_wopcm.c | 151 ++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_guc_wopcm.h |  10 ++-
 drivers/gpu/drm/i915/intel_uc.c        |   9 +-
 4 files changed, 162 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_reg.h b/drivers/gpu/drm/i915/intel_guc_reg.h
index c482df5..a550078 100644
--- a/drivers/gpu/drm/i915/intel_guc_reg.h
+++ b/drivers/gpu/drm/i915/intel_guc_reg.h
@@ -68,6 +68,8 @@
 #define DMA_GUC_WOPCM_OFFSET		_MMIO(0xc340)
 #define   HUC_LOADING_AGENT_VCR		  (0<<1)
 #define   HUC_LOADING_AGENT_GUC		  (1<<1)
+#define   GUC_WOPCM_OFFSET_SHIFT	14
+#define   GUC_WOPCM_OFFSET_MASK		  (0x3ffff << GUC_WOPCM_OFFSET_SHIFT)
 #define GUC_MAX_IDLE_COUNT		_MMIO(0xC3E4)
 
 #define HUC_STATUS2             _MMIO(0xD3B0)
diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.c b/drivers/gpu/drm/i915/intel_guc_wopcm.c
index 0194266..4043798 100644
--- a/drivers/gpu/drm/i915/intel_guc_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
@@ -83,6 +83,12 @@ void intel_guc_wopcm_init_early(struct intel_guc_wopcm *guc_wopcm)
 	guc_wopcm->top = WOPCM_DEFAULT_SIZE;
 }
 
+static inline
+struct intel_guc *guc_wopcm_to_guc(struct intel_guc_wopcm *guc_wopcm)
+{
+	return container_of(guc_wopcm, struct intel_guc, wopcm);
+}
+
 /**
  * intel_guc_wopcm_init() - Initialize the GuC WOPCM.
  * @guc_wopcm: pointer to intel_guc_wopcm.
@@ -101,13 +107,13 @@ void intel_guc_wopcm_init_early(struct intel_guc_wopcm *guc_wopcm)
 int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, u32 guc_fw_size,
 			 u32 huc_fw_size)
 {
-	struct intel_guc *guc =
-		container_of(guc_wopcm, struct intel_guc, wopcm);
+	struct intel_guc *guc = guc_wopcm_to_guc(guc_wopcm);
 	u32 reserved = context_reserved_size(guc);
 	u32 offset, size, top;
 	int err;
 
 	GEM_BUG_ON(!guc_fw_size);
+	GEM_BUG_ON(guc->wopcm.valid);
 
 	offset = huc_fw_size + WOPCM_RESERVED_SIZE;
 
@@ -138,6 +144,7 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, u32 guc_fw_size,
 	guc->wopcm.offset = offset;
 	guc->wopcm.size = size;
 	guc->wopcm.top = top;
+	guc->wopcm.load_huc_fw = huc_fw_size > 0;
 
 	err = guc_wopcm_size_check(guc, huc_fw_size);
 	if (err) {
@@ -152,3 +159,143 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, u32 guc_fw_size,
 
 	return 0;
 }
+
+struct lockable_reg {
+	const char *name;
+	struct intel_guc *guc;
+	i915_reg_t reg;
+	u32 reg_val;
+	u32 lock_bit;
+	bool (*validate)(struct lockable_reg *lreg, u32 reg_val, u32 new_val);
+};
+
+static inline u32 lockable_reg_read(struct lockable_reg *lreg)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
+
+	lreg->reg_val = I915_READ(lreg->reg);
+
+	return lreg->reg_val;
+}
+
+static inline bool lockable_reg_validate(struct lockable_reg *lreg, u32 new_val)
+{
+	GEM_BUG_ON(!lreg->validate);
+
+	return lreg->validate(lreg, lreg->reg_val, new_val);
+}
+
+static inline bool lockable_reg_locked(struct lockable_reg *lreg)
+{
+	u32 reg_val = lockable_reg_read(lreg);
+
+	return reg_val & lreg->lock_bit;
+}
+
+static inline bool lockable_reg_locked_and_valid(struct lockable_reg *lreg,
+						 u32 new_val)
+{
+	if (lockable_reg_locked(lreg)) {
+		if (lockable_reg_validate(lreg, new_val))
+			return true;
+
+		return false;
+	}
+
+	return false;
+}
+
+static inline bool lockable_reg_write(struct lockable_reg *lreg, u32 val)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
+
+	/*
+	 * Write-once register was locked which may happen with either a faulty
+	 * BIOS code or driver module reloading. We should still return success
+	 * for the write if the register was locked with a valid value.
+	 */
+	if (lockable_reg_locked(lreg)) {
+		if (lockable_reg_validate(lreg, val))
+			goto out;
+
+		DRM_DEBUG_DRIVER("Register %s was locked with invalid value\n",
+				 lreg->name);
+
+		return false;
+	}
+
+	I915_WRITE(lreg->reg, val);
+
+	if (!lockable_reg_locked_and_valid(lreg, val)) {
+		DRM_DEBUG_DRIVER("Failed to lock Register %s\n", lreg->name);
+		return false;
+	}
+
+out:
+	DRM_DEBUG_DRIVER("Write-once register %s locked with valid value %#x\n",
+			 lreg->name, lreg->reg_val);
+
+	return true;
+}
+
+static inline bool size_reg_value_validate(struct lockable_reg *lreg,
+					   u32 reg_val, u32 new_val)
+{
+	u32 reg_size = reg_val & GUC_WOPCM_SIZE_MASK;
+	u32 target_size = new_val & GUC_WOPCM_SIZE_MASK;
+
+	return reg_size == target_size;
+}
+
+static inline bool offset_reg_value_validate(struct lockable_reg *lreg,
+					     u32 reg_val, u32 new_val)
+{
+	u32 mask = GUC_WOPCM_OFFSET_MASK | HUC_LOADING_AGENT_GUC;
+	u32 reg_offset = reg_val & mask;
+	u32 target_offset = new_val & mask;
+
+	return reg_offset == target_offset;
+}
+
+#define DEFINE_LOCKABLE_REG(var, rg, lb, func)	\
+	struct lockable_reg var = {	\
+		.name = #rg,	\
+		.guc = guc_wopcm_to_guc(guc_wopcm),	\
+		.reg = rg,	\
+		.reg_val = 0,	\
+		.lock_bit = lb,	\
+		.validate = func,	\
+	}
+
+/**
+ * intel_guc_wopcm_init_hw() - Setup GuC WOPCM registers.
+ * @guc_wopcm: pointer to intel_guc_wopcm.
+ *
+ * Setup the GuC WOPCM size and offset registers with the stored values. It will
+ * also check the registers locking status to determine whether these registers
+ * are unlocked and can be updated.
+ *
+ * Return: 0 on success. -EIO if registers were locked with incorrect values.
+ */
+int intel_guc_wopcm_init_hw(struct intel_guc_wopcm *guc_wopcm)
+{
+	DEFINE_LOCKABLE_REG(guc_wopcm_size_reg, GUC_WOPCM_SIZE, BIT(0),
+			    size_reg_value_validate);
+	DEFINE_LOCKABLE_REG(guc_wopcm_offset_reg, DMA_GUC_WOPCM_OFFSET, BIT(0),
+			    offset_reg_value_validate);
+	u32 reg_val;
+
+	GEM_BUG_ON(!guc_wopcm->valid);
+
+	reg_val = guc_wopcm->size;
+	if (!lockable_reg_write(&guc_wopcm_size_reg, reg_val))
+		return -EIO;
+
+	reg_val = guc_wopcm->offset;
+	if (guc_wopcm->load_huc_fw)
+		reg_val |= HUC_LOADING_AGENT_GUC;
+	if (!lockable_reg_write(&guc_wopcm_offset_reg, reg_val))
+		return -EIO;
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.h b/drivers/gpu/drm/i915/intel_guc_wopcm.h
index 13fcab6..89dd44c 100644
--- a/drivers/gpu/drm/i915/intel_guc_wopcm.h
+++ b/drivers/gpu/drm/i915/intel_guc_wopcm.h
@@ -66,7 +66,8 @@ struct intel_guc;
  * @offset: GuC WOPCM offset from the WOPCM base.
  * @size: size of GuC WOPCM for GuC firmware.
  * @top: start of the non-GuC WOPCM memory.
- * @valid: whether this structure contains valid (1-valid, 0-invalid) info.
+ * @valid: whether the values in this struct are valid.
+ * @load_huc_fw: whether need to configure GuC to load HuC firmware.
  *
  * We simply use this structure to track the GuC use of WOPCM. The layout of
  * WOPCM would be defined by writing to GuC WOPCM offset and size registers.
@@ -75,11 +76,14 @@ struct intel_guc_wopcm {
 	u32 offset;
 	u32 size;
 	u32 top;
-	u32 valid;
+
+	/* GuC WOPCM flags below. */
+	u32 valid:1;
+	u32 load_huc_fw:1;
 };
 
 void intel_guc_wopcm_init_early(struct intel_guc_wopcm *guc_wopcm);
 int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, u32 guc_size,
 			 u32 huc_size);
-
+int intel_guc_wopcm_init_hw(struct intel_guc_wopcm *guc_wopcm);
 #endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index c842f36..8938096 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -343,14 +343,13 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 
 	GEM_BUG_ON(!HAS_GUC(dev_priv));
 
+	ret = intel_guc_wopcm_init_hw(&guc->wopcm);
+	if (ret)
+		goto err_out;
+
 	guc_disable_communication(guc);
 	gen9_reset_guc_interrupts(dev_priv);
 
-	/* init WOPCM */
-	I915_WRITE(GUC_WOPCM_SIZE, guc->wopcm.size);
-	I915_WRITE(DMA_GUC_WOPCM_OFFSET,
-		   guc->wopcm.offset | HUC_LOADING_AGENT_GUC);
-
 	/* WaEnableuKernelHeaderValidFix:skl */
 	/* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
 	if (IS_GEN9(dev_priv))
-- 
2.7.4

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

Re: [PATCH v10 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Michal Wajdeczko]

On Tue, 13 Feb 2018 00:45:52 +0100, Jackie Li <yaodong.li@intel.com> wrote:

> GuC WOPCM registers are write-once registers. Current driver code  
> accesses
> these registers without checking the accessibility to these registers  
> which
> will lead to unpredictable driver behaviors if these registers were touch
> by other components (such as faulty BIOS code).
>
> This patch moves the GuC WOPCM registers updating code into
> intel_guc_wopcm.c and adds check before and after the update to GuC WOPCM
> registers so that we can make sure the driver is in a known state before
> and after writing to these write-once registers.
>
> v6:
>  - Made sure module reloading won't bug the kernel while doing
>    locking status checking
>
> v7:
>  - Fixed patch format issues
>
> v8:
>  - Fixed coding style issue on register lock bit macro definition (Sagar)
>
> v9:
>  - Avoided to use redundant !! to cast uint to bool (Chris)
>  - Return error code instead of GEM_BUG_ON for locked with invalid  
> register
>    values case (Sagar)
>  - Updated guc_wopcm_hw_init to use guc_wopcm as first parameter (Michal)
>  - Added code to set and validate the HuC_LOADING_AGENT_GUC bit in GuC
>    WOPCM offset register based on the presence of HuC firmware (Michal)
>  - Use bit fields instead of macros for GuC WOPCM flags (Michal)
>
> v10:
>  - Refined variable names, removed reduntant comments (Joonas)
>  - Introduced lockable_reg to handle the write once register write and
>    propagate the write error to caller (Joonas)
>  - Used lockable_reg abstraction to avoid locking bit check on generic
>    i915_reg_t (Michal)
>  - Added log message for error paths (Michal)
>  - Removed hw_updated flag and only relies on real hardware status
>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Jackie Li <yaodong.li@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_reg.h   |   2 +
>  drivers/gpu/drm/i915/intel_guc_wopcm.c | 151  
> ++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_guc_wopcm.h |  10 ++-
>  drivers/gpu/drm/i915/intel_uc.c        |   9 +-
>  4 files changed, 162 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_reg.h  
> b/drivers/gpu/drm/i915/intel_guc_reg.h
> index c482df5..a550078 100644
> --- a/drivers/gpu/drm/i915/intel_guc_reg.h
> +++ b/drivers/gpu/drm/i915/intel_guc_reg.h
> @@ -68,6 +68,8 @@
>  #define DMA_GUC_WOPCM_OFFSET		_MMIO(0xc340)
>  #define   HUC_LOADING_AGENT_VCR		  (0<<1)
>  #define   HUC_LOADING_AGENT_GUC		  (1<<1)
> +#define   GUC_WOPCM_OFFSET_SHIFT	14
> +#define   GUC_WOPCM_OFFSET_MASK		  (0x3ffff << GUC_WOPCM_OFFSET_SHIFT)
>  #define GUC_MAX_IDLE_COUNT		_MMIO(0xC3E4)
> #define HUC_STATUS2             _MMIO(0xD3B0)
> diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.c  
> b/drivers/gpu/drm/i915/intel_guc_wopcm.c
> index 0194266..4043798 100644
> --- a/drivers/gpu/drm/i915/intel_guc_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
> @@ -83,6 +83,12 @@ void intel_guc_wopcm_init_early(struct  
> intel_guc_wopcm *guc_wopcm)
>  	guc_wopcm->top = WOPCM_DEFAULT_SIZE;
>  }
> +static inline
> +struct intel_guc *guc_wopcm_to_guc(struct intel_guc_wopcm *guc_wopcm)
> +{
> +	return container_of(guc_wopcm, struct intel_guc, wopcm);
> +}
> +

This can be moved to patch 3/7

>  /**
>   * intel_guc_wopcm_init() - Initialize the GuC WOPCM.
>   * @guc_wopcm: pointer to intel_guc_wopcm.
> @@ -101,13 +107,13 @@ void intel_guc_wopcm_init_early(struct  
> intel_guc_wopcm *guc_wopcm)
>  int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, u32  
> guc_fw_size,
>  			 u32 huc_fw_size)
>  {
> -	struct intel_guc *guc =
> -		container_of(guc_wopcm, struct intel_guc, wopcm);
> +	struct intel_guc *guc = guc_wopcm_to_guc(guc_wopcm);
>  	u32 reserved = context_reserved_size(guc);
>  	u32 offset, size, top;
>  	int err;
> 	GEM_BUG_ON(!guc_fw_size);
> +	GEM_BUG_ON(guc->wopcm.valid);
> 	offset = huc_fw_size + WOPCM_RESERVED_SIZE;
> @@ -138,6 +144,7 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm  
> *guc_wopcm, u32 guc_fw_size,
>  	guc->wopcm.offset = offset;
>  	guc->wopcm.size = size;
>  	guc->wopcm.top = top;
> +	guc->wopcm.load_huc_fw = huc_fw_size > 0;
> 	err = guc_wopcm_size_check(guc, huc_fw_size);
>  	if (err) {
> @@ -152,3 +159,143 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm  
> *guc_wopcm, u32 guc_fw_size,
> 	return 0;
>  }
> +
> +struct lockable_reg {
> +	const char *name;
> +	struct intel_guc *guc;
> +	i915_reg_t reg;
> +	u32 reg_val;
> +	u32 lock_bit;
> +	bool (*validate)(struct lockable_reg *lreg, u32 reg_val, u32 new_val);
> +};

<quote>
	Feels like a bit over engineering in this way.
</quote>

> +
> +static inline u32 lockable_reg_read(struct lockable_reg *lreg)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
> +
> +	lreg->reg_val = I915_READ(lreg->reg);
> +
> +	return lreg->reg_val;
> +}
> +
> +static inline bool lockable_reg_validate(struct lockable_reg *lreg, u32  
> new_val)
> +{
> +	GEM_BUG_ON(!lreg->validate);
> +
> +	return lreg->validate(lreg, lreg->reg_val, new_val);
> +}
> +
> +static inline bool lockable_reg_locked(struct lockable_reg *lreg)
> +{
> +	u32 reg_val = lockable_reg_read(lreg);
> +
> +	return reg_val & lreg->lock_bit;
> +}
> +
> +static inline bool lockable_reg_locked_and_valid(struct lockable_reg  
> *lreg,
> +						 u32 new_val)
> +{
> +	if (lockable_reg_locked(lreg)) {
> +		if (lockable_reg_validate(lreg, new_val))
> +			return true;
> +
> +		return false;
> +	}
> +
> +	return false;
> +}
> +
> +static inline bool lockable_reg_write(struct lockable_reg *lreg, u32  
> val)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
> +
> +	/*
> +	 * Write-once register was locked which may happen with either a faulty
> +	 * BIOS code or driver module reloading. We should still return success
> +	 * for the write if the register was locked with a valid value.
> +	 */
> +	if (lockable_reg_locked(lreg)) {
> +		if (lockable_reg_validate(lreg, val))
> +			goto out;
> +
> +		DRM_DEBUG_DRIVER("Register %s was locked with invalid value\n",
> +				 lreg->name);
> +
> +		return false;
> +	}
> +
> +	I915_WRITE(lreg->reg, val);
> +
> +	if (!lockable_reg_locked_and_valid(lreg, val)) {
> +		DRM_DEBUG_DRIVER("Failed to lock Register %s\n", lreg->name);
> +		return false;
> +	}

As we acknowledge that there are scenarios where registers can be already
locked, do we really need to make our code so complex ? Maybe

int write_and_verify(struct drm_i915_private *dev_priv,
                      i915_reg_t reg, u32 value, u32 locked_bit)
{
	I915_WRITE(reg, value);

	return I915_READ(reg) != (value | locked_bit) ? -EIO : 0;
}

> +
> +out:
> +	DRM_DEBUG_DRIVER("Write-once register %s locked with valid value  
> %#x\n",
> +			 lreg->name, lreg->reg_val);
> +
> +	return true;
> +}
> +
> +static inline bool size_reg_value_validate(struct lockable_reg *lreg,
> +					   u32 reg_val, u32 new_val)
> +{
> +	u32 reg_size = reg_val & GUC_WOPCM_SIZE_MASK;
> +	u32 target_size = new_val & GUC_WOPCM_SIZE_MASK;
> +
> +	return reg_size == target_size;
> +}
> +
> +static inline bool offset_reg_value_validate(struct lockable_reg *lreg,
> +					     u32 reg_val, u32 new_val)
> +{
> +	u32 mask = GUC_WOPCM_OFFSET_MASK | HUC_LOADING_AGENT_GUC;
> +	u32 reg_offset = reg_val & mask;
> +	u32 target_offset = new_val & mask;
> +
> +	return reg_offset == target_offset;
> +}
> +
> +#define DEFINE_LOCKABLE_REG(var, rg, lb, func)	\
> +	struct lockable_reg var = {	\
> +		.name = #rg,	\
> +		.guc = guc_wopcm_to_guc(guc_wopcm),	\

btw, implicit macro params are evil...

> +		.reg = rg,	\
> +		.reg_val = 0,	\
> +		.lock_bit = lb,	\
> +		.validate = func,	\

...and macro names should be always wrapped into ()

> +	}
> +


> +/**
> + * intel_guc_wopcm_init_hw() - Setup GuC WOPCM registers.
> + * @guc_wopcm: pointer to intel_guc_wopcm.
> + *
> + * Setup the GuC WOPCM size and offset registers with the stored  
> values. It will
> + * also check the registers locking status to determine whether these  
> registers
> + * are unlocked and can be updated.
> + *
> + * Return: 0 on success. -EIO if registers were locked with incorrect  
> values.
> + */
> +int intel_guc_wopcm_init_hw(struct intel_guc_wopcm *guc_wopcm)
> +{
> +	DEFINE_LOCKABLE_REG(guc_wopcm_size_reg, GUC_WOPCM_SIZE, BIT(0),
> +			    size_reg_value_validate);
> +	DEFINE_LOCKABLE_REG(guc_wopcm_offset_reg, DMA_GUC_WOPCM_OFFSET, BIT(0),
> +			    offset_reg_value_validate);
> +	u32 reg_val;
> +
> +	GEM_BUG_ON(!guc_wopcm->valid);
> +
> +	reg_val = guc_wopcm->size;
> +	if (!lockable_reg_write(&guc_wopcm_size_reg, reg_val))
> +		return -EIO;
> +
> +	reg_val = guc_wopcm->offset;
> +	if (guc_wopcm->load_huc_fw)
> +		reg_val |= HUC_LOADING_AGENT_GUC;
> +	if (!lockable_reg_write(&guc_wopcm_offset_reg, reg_val))
> +		return -EIO;
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.h  
> b/drivers/gpu/drm/i915/intel_guc_wopcm.h
> index 13fcab6..89dd44c 100644
> --- a/drivers/gpu/drm/i915/intel_guc_wopcm.h
> +++ b/drivers/gpu/drm/i915/intel_guc_wopcm.h
> @@ -66,7 +66,8 @@ struct intel_guc;
>   * @offset: GuC WOPCM offset from the WOPCM base.
>   * @size: size of GuC WOPCM for GuC firmware.
>   * @top: start of the non-GuC WOPCM memory.
> - * @valid: whether this structure contains valid (1-valid, 0-invalid)  
> info.
> + * @valid: whether the values in this struct are valid.
> + * @load_huc_fw: whether need to configure GuC to load HuC firmware.

I'm not sure that we need to track this flag inside structure.
It is just a parameter for doing partitioning and final check.

As mentioned before, we can avoid this flag and "valid" flag if do
partitioning and validation *before* writing final results to the
struct.

>   *
>   * We simply use this structure to track the GuC use of WOPCM. The  
> layout of
>   * WOPCM would be defined by writing to GuC WOPCM offset and size  
> registers.
> @@ -75,11 +76,14 @@ struct intel_guc_wopcm {
>  	u32 offset;
>  	u32 size;
>  	u32 top;
> -	u32 valid;
> +
> +	/* GuC WOPCM flags below. */
> +	u32 valid:1;
> +	u32 load_huc_fw:1;
>  };
> void intel_guc_wopcm_init_early(struct intel_guc_wopcm *guc_wopcm);
>  int intel_guc_wopcm_init(struct intel_guc_wopcm *guc_wopcm, u32  
> guc_size,
>  			 u32 huc_size);
> -
> +int intel_guc_wopcm_init_hw(struct intel_guc_wopcm *guc_wopcm);
>  #endif
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index c842f36..8938096 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -343,14 +343,13 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
> 	GEM_BUG_ON(!HAS_GUC(dev_priv));
> +	ret = intel_guc_wopcm_init_hw(&guc->wopcm);
> +	if (ret)
> +		goto err_out;
> +
>  	guc_disable_communication(guc);
>  	gen9_reset_guc_interrupts(dev_priv);
> -	/* init WOPCM */
> -	I915_WRITE(GUC_WOPCM_SIZE, guc->wopcm.size);
> -	I915_WRITE(DMA_GUC_WOPCM_OFFSET,
> -		   guc->wopcm.offset | HUC_LOADING_AGENT_GUC);
> -
>  	/* WaEnableuKernelHeaderValidFix:skl */
>  	/* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
>  	if (IS_GEN9(dev_priv))
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v10 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Yaodong Li]

On 02/13/2018 09:39 AM, Michal Wajdeczko wrote:

>> +
>> +static inline u32 lockable_reg_read(struct lockable_reg *lreg)
>> +{
>> +    struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
>> +
>> +    lreg->reg_val = I915_READ(lreg->reg);
>> +
>> +    return lreg->reg_val;
>> +}
>> +
>> +static inline bool lockable_reg_validate(struct lockable_reg *lreg, 
>> u32 new_val)
>> +{
>> +    GEM_BUG_ON(!lreg->validate);
>> +
>> +    return lreg->validate(lreg, lreg->reg_val, new_val);
>> +}
>> +
>> +static inline bool lockable_reg_locked(struct lockable_reg *lreg)
>> +{
>> +    u32 reg_val = lockable_reg_read(lreg);
>> +
>> +    return reg_val & lreg->lock_bit;
>> +}
>> +
>> +static inline bool lockable_reg_locked_and_valid(struct lockable_reg 
>> *lreg,
>> +                         u32 new_val)
>> +{
>> +    if (lockable_reg_locked(lreg)) {
>> +        if (lockable_reg_validate(lreg, new_val))
>> +            return true;
>> +
>> +        return false;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +static inline bool lockable_reg_write(struct lockable_reg *lreg, u32 
>> val)
>> +{
>> +    struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
>> +
>> +    /*
>> +     * Write-once register was locked which may happen with either a 
>> faulty
>> +     * BIOS code or driver module reloading. We should still return 
>> success
>> +     * for the write if the register was locked with a valid value.
>> +     */
>> +    if (lockable_reg_locked(lreg)) {
>> +        if (lockable_reg_validate(lreg, val))
>> +            goto out;
>> +
>> +        DRM_DEBUG_DRIVER("Register %s was locked with invalid value\n",
>> +                 lreg->name);
>> +
>> +        return false;
>> +    }
>> +
>> +    I915_WRITE(lreg->reg, val);
>> +
>> +    if (!lockable_reg_locked_and_valid(lreg, val)) {
>> +        DRM_DEBUG_DRIVER("Failed to lock Register %s\n", lreg->name);
>> +        return false;
>> +    }
>
> As we acknowledge that there are scenarios where registers can be already
> locked, do we really need to make our code so complex ? Maybe
>
> int write_and_verify(struct drm_i915_private *dev_priv,
>                      i915_reg_t reg, u32 value, u32 locked_bit)
> {
>     I915_WRITE(reg, value);
>
>     return I915_READ(reg) != (value | locked_bit) ? -EIO : 0;
> }
>
Yes, I agree it's too complex at least for the validation part. Thanks!

My intention was trying to avoid extra write once we found the reg
was locked and to distinguish between faulty SW behavior and
hardware locking error? but now I feel it's not worth it.:-(
>> +
>> +
>> +#define DEFINE_LOCKABLE_REG(var, rg, lb, func)    \
>> +    struct lockable_reg var = {    \
>> +        .name = #rg,    \
>> +        .guc = guc_wopcm_to_guc(guc_wopcm),    \
>
> btw, implicit macro params are evil...
Agree. but seems we always use similar approach in
I915_READ/WRITE().O:-)
>> +        .reg = rg,    \
>> +        .reg_val = 0,    \
>> +        .lock_bit = lb,    \
>> +        .validate = func,    \
>
> ...and macro names should be always wrapped into ()
>
Thanks!
>> diff --git a/drivers/gpu/drm/i915/intel_guc_wopcm.h 
>> b/drivers/gpu/drm/i915/intel_guc_wopcm.h
>> index 13fcab6..89dd44c 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_wopcm.h
>> +++ b/drivers/gpu/drm/i915/intel_guc_wopcm.h
>> @@ -66,7 +66,8 @@ struct intel_guc;
>>   * @offset: GuC WOPCM offset from the WOPCM base.
>>   * @size: size of GuC WOPCM for GuC firmware.
>>   * @top: start of the non-GuC WOPCM memory.
>> - * @valid: whether this structure contains valid (1-valid, 
>> 0-invalid) info.
>> + * @valid: whether the values in this struct are valid.
>> + * @load_huc_fw: whether need to configure GuC to load HuC firmware.
>
> I'm not sure that we need to track this flag inside structure.
> It is just a parameter for doing partitioning and final check.
>
I think it's related to actual reg configuration. Any suggestions since
we do need it in hw_init to setup offset reg?
> As mentioned before, we can avoid this flag and "valid" flag if do
> partitioning and validation *before* writing final results to the
> struct.
In current code, we do verify the ggtt offset against wopcm top in our 
current code which means
current code won't trust the fact that ggtt offset would never be used 
after uc/guc init failed.
This is the reason for this valid bit (which clearly suggests the struct 
is ready to use) - I won't
assume the ggtt_offset would never be called even if the uc/guc_init 
returned failure.


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

Re: [PATCH v10 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Michal Wajdeczko]

On Wed, 14 Feb 2018 02:18:29 +0100, Yaodong Li <yaodong.li@intel.com>  
wrote:

> On 02/13/2018 09:39 AM, Michal Wajdeczko wrote:
>

<snip>

>>> +
>>> +#define DEFINE_LOCKABLE_REG(var, rg, lb, func)    \
>>> +    struct lockable_reg var = {    \
>>> +        .name = #rg,    \
>>> +        .guc = guc_wopcm_to_guc(guc_wopcm),    \
>>
>> btw, implicit macro params are evil...
> Agree. but seems we always use similar approach in
> I915_READ/WRITE().O:-)

True, but the only reason why it is still not fixed is that
it requires changes in too many places ...

<snip>

>>> + * @load_huc_fw: whether need to configure GuC to load HuC firmware.
>>
>> I'm not sure that we need to track this flag inside structure.
>> It is just a parameter for doing partitioning and final check.
>>
> I think it's related to actual reg configuration. Any suggestions since
> we do need it in hw_init to setup offset reg?

You can do partitioning at intel_wopcm level, but program hw at intel_uc
level when you know for sure if HuC is in use. Note that there is no wrong
in using results from WOPCM partitioning outside of intel_wopmc.

>> As mentioned before, we can avoid this flag and "valid" flag if do
>> partitioning and validation *before* writing final results to the
>> struct.
> In current code, we do verify the ggtt offset against wopcm top in our  
> current code which means
> current code won't trust the fact that ggtt offset would never be used  
> after uc/guc init failed.

But after failure in WOPCM partitioning, which should be done sooner than
uc init, we should abort driver load, so there shall be no access to ggtt.

> This is the reason for this valid bit (which clearly suggests the struct  
> is ready to use) - I won't

I'm not sure that "valid" flag approach is correct - you should trust your
code path, otherwise you will have to add "valid" flags to every structure
and this is not a scalable solution ;)

> assume the ggtt_offset would never be called even if the uc/guc_init  
> returned failure.
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v10 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Yaodong Li]

On 02/14/2018 09:07 AM, Michal Wajdeczko wrote:
> On Wed, 14 Feb 2018 02:18:29 +0100, Yaodong Li <yaodong.li@intel.com> 
> wrote:
>
>> On 02/13/2018 09:39 AM, Michal Wajdeczko wrote:
>>
>
> <snip>
>
>>>> +
>>>> +#define DEFINE_LOCKABLE_REG(var, rg, lb, func)    \
>>>> +    struct lockable_reg var = {    \
>>>> +        .name = #rg,    \
>>>> +        .guc = guc_wopcm_to_guc(guc_wopcm),    \
>>>
>>> btw, implicit macro params are evil...
>> Agree. but seems we always use similar approach in
>> I915_READ/WRITE().O:-)
>
> True, but the only reason why it is still not fixed is that
> it requires changes in too many places ...
Sure. will avoid to use implicit params then:-) Thanks.
>
> <snip>
>
>>>> + * @load_huc_fw: whether need to configure GuC to load HuC firmware.
>>>
>>> I'm not sure that we need to track this flag inside structure.
>>> It is just a parameter for doing partitioning and final check.
>>>
>> I think it's related to actual reg configuration. Any suggestions since
>> we do need it in hw_init to setup offset reg?
>
> You can do partitioning at intel_wopcm level, but program hw at intel_uc
> level when you know for sure if HuC is in use. Note that there is no 
> wrong
> in using results from WOPCM partitioning outside of intel_wopmc.
>
>>> As mentioned before, we can avoid this flag and "valid" flag if do
>>> partitioning and validation *before* writing final results to the
>>> struct.
>> In current code, we do verify the ggtt offset against wopcm top in 
>> our current code which means
>> current code won't trust the fact that ggtt offset would never be 
>> used after uc/guc init failed.
>
> But after failure in WOPCM partitioning, which should be done sooner than
> uc init, we should abort driver load, so there shall be no access to 
> ggtt.
>
I would like to have more discussion on intel_wopcm over 
intel_guc_wopcm. :-)
>> This is the reason for this valid bit (which clearly suggests the 
>> struct is ready to use) - I won't
>
> I'm not sure that "valid" flag approach is correct - you should trust 
> your
> code path, otherwise you will have to add "valid" flags to every 
> structure
> and this is not a scalable solution ;)
>
True. I need some courage here:-). I would doubt myself after seeing 
guc_ggtt_offset
validates every offset against the wopcm->guc.top which is the only 
reason I kept using
this flag:-(

Regards,
-Jackie

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

Re: [PATCH v10 6/7] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Yaodong Li]

On 02/13/2018 05:18 PM, Yaodong Li wrote:
> On 02/13/2018 09:39 AM, Michal Wajdeczko wrote:
>
>>> +
>>> +static inline u32 lockable_reg_read(struct lockable_reg *lreg)
>>> +{
>>> +    struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
>>> +
>>> +    lreg->reg_val = I915_READ(lreg->reg);
>>> +
>>> +    return lreg->reg_val;
>>> +}
>>> +
>>> +static inline bool lockable_reg_validate(struct lockable_reg *lreg, 
>>> u32 new_val)
>>> +{
>>> +    GEM_BUG_ON(!lreg->validate);
>>> +
>>> +    return lreg->validate(lreg, lreg->reg_val, new_val);
>>> +}
>>> +
>>> +static inline bool lockable_reg_locked(struct lockable_reg *lreg)
>>> +{
>>> +    u32 reg_val = lockable_reg_read(lreg);
>>> +
>>> +    return reg_val & lreg->lock_bit;
>>> +}
>>> +
>>> +static inline bool lockable_reg_locked_and_valid(struct 
>>> lockable_reg *lreg,
>>> +                         u32 new_val)
>>> +{
>>> +    if (lockable_reg_locked(lreg)) {
>>> +        if (lockable_reg_validate(lreg, new_val))
>>> +            return true;
>>> +
>>> +        return false;
>>> +    }
>>> +
>>> +    return false;
>>> +}
>>> +
>>> +static inline bool lockable_reg_write(struct lockable_reg *lreg, 
>>> u32 val)
>>> +{
>>> +    struct drm_i915_private *dev_priv = guc_to_i915(lreg->guc);
>>> +
>>> +    /*
>>> +     * Write-once register was locked which may happen with either 
>>> a faulty
>>> +     * BIOS code or driver module reloading. We should still return 
>>> success
>>> +     * for the write if the register was locked with a valid value.
>>> +     */
>>> +    if (lockable_reg_locked(lreg)) {
>>> +        if (lockable_reg_validate(lreg, val))
>>> +            goto out;
>>> +
>>> +        DRM_DEBUG_DRIVER("Register %s was locked with invalid 
>>> value\n",
>>> +                 lreg->name);
>>> +
>>> +        return false;
>>> +    }
>>> +
>>> +    I915_WRITE(lreg->reg, val);
>>> +
>>> +    if (!lockable_reg_locked_and_valid(lreg, val)) {
>>> +        DRM_DEBUG_DRIVER("Failed to lock Register %s\n", lreg->name);
>>> +        return false;
>>> +    }
>>
>> As we acknowledge that there are scenarios where registers can be 
>> already
>> locked, do we really need to make our code so complex ? Maybe
>>
>> int write_and_verify(struct drm_i915_private *dev_priv,
>>                      i915_reg_t reg, u32 value, u32 locked_bit)
>> {
>>     I915_WRITE(reg, value);
>>
>>     return I915_READ(reg) != (value | locked_bit) ? -EIO : 0;
>> }
>>
> Yes, I agree it's too complex at least for the validation part. Thanks!
>
> My intention was trying to avoid extra write once we found the reg
> was locked and to distinguish between faulty SW behavior and
> hardware locking error? but now I feel it's not worth it.:-(
Sorry. I regret:-). I think it makes since to use my complex way
to validate the values because of the rsvd bits these regs.
a comparison I915_READ(reg) != (value | locked_bit) cannot
rule out the possibility that these regs were updated by faulty
software code (e.g. BIOS) with these rsvd bit set to random
values. and it's aways better to not make any assumption to
these rsvd bits. so I will still keep using the complex
way (explicitly clear this rsvd bit before comparison) to do this.
As for the lock_bit_check->write->verify sequence v.s. write->verify
sequence I think I can prefer to first one since it would provide
comprehensive info when any error occurs. so let's keep it?:-)
>>> +
>>> +
>>> +#define DEFINE_LOCKABLE_REG(var, rg, lb, func)    \
>>> +    struct lockable_reg var = {    \
>>> +        .name = #rg,    \
>>> +        .guc = guc_wopcm_to_guc(guc_wopcm),    \
>>
>> btw, implicit macro params are evil...
> Agree. but seems we always use similar approach in
> I915_READ/WRITE().O:-)
I will avoid this implicit macro.
>>> +        .reg = rg,    \
>>> +        .reg_val = 0,    \
>>> +        .lock_bit = lb,    \
>>> +        .validate = func,    \
>>
>> ...and macro names should be always wrapped into ()
>>
> Thanks!
Will Add them.

Regards,
-Jackie


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

[PATCH v10 7/7] HAX Enable GuC Submission for CI

[Jackie Li]

Signed-off-by: Jackie Li <yaodong.li@intel.com>
---
 drivers/gpu/drm/i915/i915_params.c | 2 +-
 drivers/gpu/drm/i915/i915_params.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 08108ce..b49ae20 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -152,7 +152,7 @@ i915_param_named_unsafe(edp_vswing, int, 0400,
 i915_param_named_unsafe(enable_guc, int, 0400,
 	"Enable GuC load for GuC submission and/or HuC load. "
 	"Required functionality can be selected using bitmask values. "
-	"(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
+	"(-1=auto [default], 0=disable, 1=GuC submission, 2=HuC load)");
 
 i915_param_named(guc_log_level, int, 0400,
 	"GuC firmware logging level. Requires GuC to be loaded. "
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 430f5f9..3deae1e 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, 0) \
 	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: failure for series starting with [v10,1/7] drm/i915/guc: Move GuC WOPCM related code into separate files

[Patchwork]

== Series Details ==

Series: series starting with [v10,1/7] drm/i915/guc: Move GuC WOPCM related code into separate files
URL   : https://patchwork.freedesktop.org/series/38119/
State : failure

== Summary ==

Series 38119v1 series starting with [v10,1/7] drm/i915/guc: Move GuC WOPCM related code into separate files
https://patchwork.freedesktop.org/api/1.0/series/38119/revisions/1/mbox/

Test core_auth:
        Subgroup basic-auth:
                pass       -> SKIP       (fi-skl-gvtdvm)
Test core_prop_blob:
        Subgroup basic:
                pass       -> SKIP       (fi-skl-gvtdvm)
Test debugfs_test:
        Subgroup read_all_entries:
                pass       -> SKIP       (fi-skl-gvtdvm)
Test drv_getparams_basic:
        Subgroup basic-eu-total:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup basic-subslice-total:
                pass       -> SKIP       (fi-skl-gvtdvm)
Test drv_hangman:
        Subgroup error-state-basic:
                pass       -> SKIP       (fi-skl-gvtdvm)
Test gem_basic:
        Subgroup bad-close:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup create-close:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup create-fd-close:
                pass       -> SKIP       (fi-skl-gvtdvm)
Test gem_busy:
        Subgroup basic-busy-default:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup basic-hang-default:
                pass       -> SKIP       (fi-skl-gvtdvm) fdo#104108 +2
Test gem_close_race:
        Subgroup basic-process:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup basic-threads:
                pass       -> SKIP       (fi-skl-gvtdvm)
Test gem_cpu_reloc:
        Subgroup basic:
                pass       -> SKIP       (fi-skl-gvtdvm)
Test gem_cs_tlb:
        Subgroup basic-default:
                pass       -> SKIP       (fi-skl-gvtdvm)
Test gem_ctx_create:
        Subgroup basic:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup basic-files:
                pass       -> SKIP       (fi-skl-gvtdvm)
Test gem_ctx_exec:
        Subgroup basic:
                pass       -> SKIP       (fi-skl-gvtdvm)
Test gem_ctx_param:
        Subgroup basic:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup basic-default:
                pass       -> SKIP       (fi-skl-gvtdvm)
Test gem_ctx_switch:
        Subgroup basic-default:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup basic-default-heavy:
                pass       -> SKIP       (fi-skl-gvtdvm)
Test gem_exec_basic:
        Subgroup basic-blt:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup basic-bsd:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup basic-bsd1:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup basic-bsd2:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup basic-default:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup basic-render:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup basic-vebox:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup gtt-blt:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup gtt-bsd:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup gtt-bsd1:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup gtt-bsd2:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup gtt-default:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup gtt-render:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup gtt-vebox:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup readonly-blt:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup readonly-bsd:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup readonly-bsd1:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup readonly-bsd2:
                pass       -> SKIP       (fi-skl-gvtdvm)
        Subgroup readonly-default:
                pass       -> SKIP       (fi-skl-gvtdvm)
WARNING: Long output truncated

83878f486e686dd291ef3566a45103962d7617ed drm-tip: 2018y-02m-12d-17h-43m-07s UTC integration manifest
4d320eb26ae4 HAX Enable GuC Submission for CI
63d5296b9ac7 drm/i915/guc: Check the locking status of GuC WOPCM registers
64f9829bbae8 drm/i915/guc: Add HuC firmware size related restriction for Gen9 and CNL A0
1c10532a78e7 drm/i915/guc: Add support to return CNL specific reserved GuC WOPCM size
13a1710cf100 drm/i915/guc: Implement dynamic GuC WOPCM offset and size
5c1e07e501d5 drm/i915/guc: Rename guc_ggtt_offset to intel_guc_ggtt_offset
c93b3e317960 drm/i915/guc: Move GuC WOPCM related code into separate files

== Logs ==

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