* [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition.
@ 2017-11-04 0:18 Jackie Li
2017-11-04 0:18 ` [PATCH 2/2] HAX enable guc submission for CI Jackie Li
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Jackie Li @ 2017-11-04 0:18 UTC (permalink / raw)
To: intel-gfx; +Cc: Sujaritha Sundaresan
Static WOPCM partitioning would lead to GuC loading failure
if the GuC/HuC firmware size exceeded current static 512KB
partition boundary.
This patch enabled the dynamical calculation of the WOPCM aperture
used by GuC/HuC firmware. GuC WOPCM offset was set to
HuC size + reserved WOPCM size. GuC WOPCM size was set to
total WOPCM size - GuC WOPCM offset - RC6CTX size.
Signed-off-by: Jackie Li <yaodong.li@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: 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>
---
drivers/gpu/drm/i915/i915_drv.c | 15 ++++
drivers/gpu/drm/i915/i915_drv.h | 13 ++++
drivers/gpu/drm/i915/i915_gem_context.c | 4 +-
drivers/gpu/drm/i915/i915_guc_reg.h | 18 ++++-
drivers/gpu/drm/i915/intel_guc.c | 46 ++++++++++--
drivers/gpu/drm/i915/intel_guc.h | 18 +----
drivers/gpu/drm/i915/intel_huc.c | 3 +-
drivers/gpu/drm/i915/intel_uc.c | 128 +++++++++++++++++++++++++++++++-
drivers/gpu/drm/i915/intel_uc_fw.c | 12 ++-
9 files changed, 223 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e7e9e06..19509fd 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -623,6 +623,15 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
WARN_ON(!list_empty(&dev_priv->contexts.list));
}
+static void i915_wopcm_init(struct drm_i915_private *dev_priv)
+{
+ struct intel_wopcm_info *wopcm = &dev_priv->wopcm;
+
+ wopcm->size = WOPCM_DEFAULT_SIZE;
+
+ DRM_DEBUG_DRIVER("WOPCM size: %dKB\n", wopcm->size >> 10);
+}
+
static int i915_load_modeset_init(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = to_i915(dev);
@@ -670,6 +679,12 @@ static int i915_load_modeset_init(struct drm_device *dev)
if (ret)
goto cleanup_irq;
+ /*
+ * Get the wopcm memory info.
+ * NOTE: this need to be called before init FW.
+ */
+ i915_wopcm_init(dev_priv);
+
intel_uc_init_fw(dev_priv);
ret = i915_gem_init(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 72bb5b5..61cd290 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2235,6 +2235,16 @@ struct intel_cdclk_state {
u8 voltage_level;
};
+struct intel_wopcm_info {
+ u32 size;
+};
+
+struct intel_wopcm_partition {
+ u32 guc_wopcm_offset;
+ u32 guc_wopcm_size;
+ u32 guc_wopcm_top;
+};
+
struct drm_i915_private {
struct drm_device drm;
@@ -2258,6 +2268,9 @@ struct drm_i915_private {
struct intel_huc huc;
struct intel_guc guc;
+ struct intel_wopcm_info wopcm;
+ struct intel_wopcm_partition wopcm_partition;
+
struct intel_csr csr;
struct intel_gmbus gmbus[GMBUS_NUM_PINS];
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 10affb3..7347fd7 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -312,12 +312,12 @@ __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 (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
- ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
+ ctx->ggtt_offset_bias = intel_guc_wopcm_top(dev_priv);
else
ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h
index 35cf991..d309884 100644
--- a/drivers/gpu/drm/i915/i915_guc_reg.h
+++ b/drivers/gpu/drm/i915/i915_guc_reg.h
@@ -67,17 +67,27 @@
#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)
#define HUC_FW_VERIFIED (1<<7)
/* Defines WOPCM space available to GuC firmware */
+/* default WOPCM size 1MB */
+#define WOPCM_DEFAULT_SIZE (0x1 << 20)
+/* reserved WOPCM size 16KB */
+#define WOPCM_RESERVED_SIZE (0x4000)
+/* GUC WOPCM Offset need to be 16KB aligned */
+#define WOPCM_OFFSET_ALIGNMENT (0x4000)
+/* 8KB stack reserved for GuC FW*/
+#define GUC_WOPCM_STACK_RESERVED (0x2000)
+/* 24KB WOPCM reserved for RC6 CTX on BXT */
+#define BXT_WOPCM_RC6_RESERVED (0x6000)
+
+#define GEN9_GUC_WOPCM_DELTA 4
+#define GEN9_GUC_WOPCM_OFFSET (0x24000)
+
#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.c b/drivers/gpu/drm/i915/intel_guc.c
index 9678630..0efcfb4 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -337,7 +337,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.
@@ -358,7 +358,8 @@ 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 |
+ intel_guc_wopcm_top(dev_priv));
if (ret) {
vma = ERR_PTR(ret);
goto err;
@@ -373,11 +374,42 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv)
{
- u32 wopcm_size = GUC_WOPCM_TOP;
+ struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
- /* On BXT, the top of WOPCM is reserved for RC6 context */
- if (IS_GEN9_LP(dev_priv))
- wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED;
+ GEM_BUG_ON(!wp->guc_wopcm_size);
- return wopcm_size;
+ return wp->guc_wopcm_size;
+}
+
+u32 intel_guc_wopcm_top(struct drm_i915_private *dev_priv)
+{
+ struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
+
+ GEM_BUG_ON(!dev_priv->wopcm.size);
+
+ return wp->guc_wopcm_top ? wp->guc_wopcm_top : dev_priv->wopcm.size;
+}
+
+u32 intel_guc_wopcm_offset(struct drm_i915_private *dev_priv)
+{
+ struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
+
+ GEM_BUG_ON(!wp->guc_wopcm_size);
+
+ return wp->guc_wopcm_offset;
+}
+
+/*
+ * GuC does not allow any gfx GGTT address that falls into range [0, 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.
+ */
+u32 guc_ggtt_offset(struct i915_vma *vma)
+{
+ struct drm_i915_private *dev_priv = vma->vm->i915;
+ u32 offset = i915_ggtt_offset(vma);
+
+ GEM_BUG_ON(offset < intel_guc_wopcm_top(dev_priv));
+ GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
+ return offset;
}
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 607e025..1493de0 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -100,21 +100,6 @@ static inline void intel_guc_notify(struct intel_guc *guc)
guc->notify(guc);
}
-/*
- * 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.
- */
-static inline u32 guc_ggtt_offset(struct i915_vma *vma)
-{
- u32 offset = i915_ggtt_offset(vma);
-
- GEM_BUG_ON(offset < GUC_WOPCM_TOP);
- GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
-
- return offset;
-}
void intel_guc_init_early(struct intel_guc *guc);
void intel_guc_init_send_regs(struct intel_guc *guc);
@@ -127,5 +112,8 @@ 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);
+u32 intel_guc_wopcm_top(struct drm_i915_private *dev_priv);
+u32 intel_guc_wopcm_offset(struct drm_i915_private *dev_priv);
+u32 guc_ggtt_offset(struct i915_vma *vma);
#endif
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 98d1725..a985aa5 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -202,7 +202,8 @@ void intel_huc_auth(struct intel_huc *huc)
return;
vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0,
- PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
+ PIN_OFFSET_BIAS |
+ intel_guc_wopcm_top(i915));
if (IS_ERR(vma)) {
DRM_ERROR("failed to pin huc fw object %d\n",
(int)PTR_ERR(vma));
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index aec2954..83b2516 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -86,6 +86,125 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
intel_guc_init_early(&dev_priv->guc);
}
+static u32 rc6_context_size(struct drm_i915_private *dev_priv)
+{
+ /* On BXT, the top of WOPCM is reserved for RC6 context */
+ if (IS_GEN9_LP(dev_priv))
+ return BXT_WOPCM_RC6_RESERVED;
+
+ return 0;
+}
+
+static int do_wopcm_partition(struct drm_i915_private *dev_priv,
+ u32 offset, u32 reserved_size)
+{
+ struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
+ u32 aligned_offset = ALIGN(offset, WOPCM_OFFSET_ALIGNMENT);
+
+ if (offset >= dev_priv->wopcm.size)
+ return -E2BIG;
+
+ if (reserved_size >= dev_priv->wopcm.size)
+ return -E2BIG;
+
+ if ((aligned_offset + reserved_size) >= dev_priv->wopcm.size)
+ return -E2BIG;
+
+ wp->guc_wopcm_offset = aligned_offset;
+ wp->guc_wopcm_top = dev_priv->wopcm.size - wp->guc_wopcm_offset;
+ wp->guc_wopcm_size = wp->guc_wopcm_top - reserved_size;
+
+ return 0;
+}
+
+static int intel_uc_init_wopcm_partition(struct drm_i915_private *dev_priv)
+{
+ struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
+ struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
+ struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
+ size_t huc_size, guc_size;
+ u32 offset;
+ u32 reserved;
+ u32 wopcm_base;
+ u32 delta;
+ int err;
+
+ /*Return if WOPCM partition has been initialized*/
+ if (wp->guc_wopcm_size)
+ return 0;
+
+ GEM_BUG_ON(!dev_priv->wopcm.size);
+
+ /*No need to do partition if failed to fetch both GuC and HuC FW*/
+ if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS &&
+ huc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
+ return -EIO;
+
+ huc_size = 0;
+ guc_size = 0;
+ offset = WOPCM_RESERVED_SIZE;
+ reserved = rc6_context_size(dev_priv);
+
+ if (huc_fw->fetch_status == INTEL_UC_FIRMWARE_SUCCESS)
+ huc_size = huc_fw->header_size + huc_fw->ucode_size;
+
+ err = do_wopcm_partition(dev_priv, offset + huc_size, reserved);
+ if (err) {
+ if (!huc_size)
+ goto pt_done;
+
+ /*partition failed with HuC FW, block HuC loading*/
+ huc_size = 0;
+
+ /*partition without loading HuC FW*/
+ err = do_wopcm_partition(dev_priv, offset, reserved);
+ if (err)
+ goto pt_done;
+ }
+
+ /*
+ * Check hardware restriction on Gen9
+ * GuC WOPCM size is at least 4 bytes larger than GuC WOPCM base due
+ * to hardware limitation on Gen9.
+ */
+ if (IS_GEN9(dev_priv)) {
+ wopcm_base = wp->guc_wopcm_offset + GEN9_GUC_WOPCM_OFFSET;
+ if (unlikely(wopcm_base > wp->guc_wopcm_size))
+ goto pt_done;
+
+ delta = wp->guc_wopcm_size - wopcm_base;
+ if (unlikely(delta < GEN9_GUC_WOPCM_DELTA))
+ goto pt_done;
+ }
+
+ if (guc_fw->fetch_status == INTEL_UC_FIRMWARE_SUCCESS) {
+ guc_size = guc_fw->header_size + guc_fw->ucode_size;
+ /*need 8K stack for GuC*/
+ guc_size += GUC_WOPCM_STACK_RESERVED;
+ }
+
+ if (guc_size > wp->guc_wopcm_size)
+ guc_size = 0;
+
+pt_done:
+ if (!huc_size) {
+ DRM_ERROR("HuC firmware is too large to fit in WOPCM\n");
+ intel_uc_fw_fini(huc_fw);
+ }
+
+ if (!guc_size) {
+ DRM_ERROR("GuC firmware is too large to fit in WOPCM\n");
+ intel_uc_fw_fini(guc_fw);
+ }
+
+ DRM_DEBUG_DRIVER("GuC WOPCM offset %dKB, size %dKB, top %dKB\n",
+ wp->guc_wopcm_offset >> 10,
+ wp->guc_wopcm_size >> 10,
+ wp->guc_wopcm_top >> 10);
+
+ return err;
+}
+
void intel_uc_init_fw(struct drm_i915_private *dev_priv)
{
intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
@@ -157,6 +276,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
if (!i915_modparams.enable_guc_loading)
return 0;
+ /*init WOPCM partition*/
+ ret = intel_uc_init_wopcm_partition(dev_priv);
+ if (ret)
+ goto err_wopcm;
+
guc_disable_communication(guc);
gen9_reset_guc_interrupts(dev_priv);
@@ -176,7 +300,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
/* init WOPCM */
I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(dev_priv));
I915_WRITE(DMA_GUC_WOPCM_OFFSET,
- GUC_WOPCM_OFFSET_VALUE | HUC_LOADING_AGENT_GUC);
+ intel_guc_wopcm_offset(dev_priv) | HUC_LOADING_AGENT_GUC);
/* WaEnableuKernelHeaderValidFix:skl */
/* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
@@ -249,7 +373,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
i915_guc_submission_fini(dev_priv);
err_guc:
i915_ggtt_disable_guc(dev_priv);
-
+err_wopcm:
if (i915_modparams.enable_guc_loading > 1 ||
i915_modparams.enable_guc_submission > 1) {
DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
index 973888e..aefba13 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)) {
+ if (size > dev_priv->wopcm.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,8 @@ 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 |
+ intel_guc_wopcm_top(i915));
if (IS_ERR(vma)) {
err = PTR_ERR(vma);
DRM_DEBUG_DRIVER("%s fw ggtt-pin err=%d\n",
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] HAX enable guc submission for CI
2017-11-04 0:18 [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition Jackie Li
@ 2017-11-04 0:18 ` Jackie Li
2017-11-04 0:38 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: implemented dynamic WOPCM partition Patchwork
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Jackie Li @ 2017-11-04 0:18 UTC (permalink / raw)
To: intel-gfx
Signed-off-by: Jackie Li <yaodong.li@intel.com>
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++------
drivers/gpu/drm/i915/i915_params.h | 4 ++--
2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 0684d5d..a351ddf 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3562,17 +3562,13 @@ int i915_ggtt_enable_hw(struct drm_i915_private *dev_priv)
void i915_ggtt_enable_guc(struct drm_i915_private *i915)
{
- GEM_BUG_ON(i915->ggtt.invalidate != gen6_ggtt_invalidate);
-
i915->ggtt.invalidate = guc_ggtt_invalidate;
}
void i915_ggtt_disable_guc(struct drm_i915_private *i915)
{
- /* We should only be called after i915_ggtt_enable_guc() */
- GEM_BUG_ON(i915->ggtt.invalidate != guc_ggtt_invalidate);
-
- i915->ggtt.invalidate = gen6_ggtt_invalidate;
+ if (i915->ggtt.invalidate == guc_ggtt_invalidate)
+ i915->ggtt.invalidate = gen6_ggtt_invalidate;
}
void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index c729226..c38cef0 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -44,8 +44,8 @@
param(int, disable_power_well, -1) \
param(int, enable_ips, 1) \
param(int, invert_brightness, 0) \
- param(int, enable_guc_loading, 0) \
- param(int, enable_guc_submission, 0) \
+ param(int, enable_guc_loading, 1) \
+ param(int, enable_guc_submission, 1) \
param(int, guc_log_level, -1) \
param(char *, guc_firmware_path, NULL) \
param(char *, huc_firmware_path, NULL) \
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: implemented dynamic WOPCM partition.
2017-11-04 0:18 [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition Jackie Li
2017-11-04 0:18 ` [PATCH 2/2] HAX enable guc submission for CI Jackie Li
@ 2017-11-04 0:38 ` Patchwork
2017-11-06 8:16 ` [PATCH 1/2] " Sagar Arun Kamble
2017-11-07 10:52 ` Joonas Lahtinen
3 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2017-11-04 0:38 UTC (permalink / raw)
To: Jackie Li; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/2] drm/i915: implemented dynamic WOPCM partition.
URL : https://patchwork.freedesktop.org/series/33166/
State : failure
== Summary ==
Series 33166v1 series starting with [1/2] drm/i915: implemented dynamic WOPCM partition.
https://patchwork.freedesktop.org/api/1.0/series/33166/revisions/1/mbox/
Test chamelium:
Subgroup dp-crc-fast:
pass -> FAIL (fi-kbl-7500u) fdo#102514
Test gem_basic:
Subgroup create-close:
dmesg-warn -> PASS (fi-cfl-s)
Subgroup create-fd-close:
dmesg-warn -> PASS (fi-cfl-s)
Test gem_sync:
Subgroup basic-many-each:
pass -> FAIL (fi-kbl-7500u)
pass -> FAIL (fi-kbl-7560u)
Test kms_pipe_crc_basic:
Subgroup read-crc-pipe-b-frame-sequence:
pass -> FAIL (fi-skl-6700k)
Subgroup read-crc-pipe-c:
dmesg-warn -> PASS (fi-bsw-n3050)
Test drv_module_reload:
Subgroup basic-reload:
pass -> DMESG-WARN (fi-cnl-y)
Subgroup basic-no-display:
pass -> DMESG-WARN (fi-cnl-y)
Subgroup basic-reload-inject:
pass -> DMESG-WARN (fi-cnl-y)
fdo#102514 https://bugs.freedesktop.org/show_bug.cgi?id=102514
fi-bdw-5557u total:289 pass:268 dwarn:0 dfail:0 fail:0 skip:21 time:440s
fi-bdw-gvtdvm total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:451s
fi-blb-e6850 total:289 pass:223 dwarn:1 dfail:0 fail:0 skip:65 time:380s
fi-bsw-n3050 total:289 pass:243 dwarn:0 dfail:0 fail:0 skip:46 time:533s
fi-bwr-2160 total:289 pass:183 dwarn:0 dfail:0 fail:0 skip:106 time:276s
fi-bxt-dsi total:289 pass:259 dwarn:0 dfail:0 fail:0 skip:30 time:514s
fi-bxt-j4205 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:516s
fi-byt-j1900 total:289 pass:253 dwarn:1 dfail:0 fail:0 skip:35 time:501s
fi-byt-n2820 total:289 pass:249 dwarn:1 dfail:0 fail:0 skip:39 time:492s
fi-cfl-s total:289 pass:254 dwarn:3 dfail:0 fail:0 skip:32 time:560s
fi-cnl-y total:289 pass:259 dwarn:3 dfail:0 fail:0 skip:27 time:623s
fi-elk-e7500 total:289 pass:229 dwarn:0 dfail:0 fail:0 skip:60 time:428s
fi-gdg-551 total:289 pass:178 dwarn:1 dfail:0 fail:1 skip:109 time:262s
fi-glk-1 total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:583s
fi-hsw-4770 total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:432s
fi-hsw-4770r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:429s
fi-ilk-650 total:289 pass:228 dwarn:0 dfail:0 fail:0 skip:61 time:426s
fi-ivb-3520m total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:493s
fi-ivb-3770 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:463s
fi-kbl-7500u total:289 pass:262 dwarn:1 dfail:0 fail:2 skip:24 time:484s
fi-kbl-7560u total:289 pass:269 dwarn:0 dfail:0 fail:1 skip:19 time:578s
fi-kbl-7567u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:476s
fi-kbl-r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:585s
fi-pnv-d510 total:289 pass:222 dwarn:1 dfail:0 fail:0 skip:66 time:576s
fi-skl-6260u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:451s
fi-skl-6600u total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:593s
fi-skl-6700hq total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:647s
fi-skl-6700k total:289 pass:264 dwarn:0 dfail:0 fail:1 skip:24 time:534s
fi-skl-6770hq total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:506s
fi-skl-gvtdvm total:289 pass:266 dwarn:0 dfail:0 fail:0 skip:23 time:456s
fi-snb-2520m total:289 pass:250 dwarn:0 dfail:0 fail:0 skip:39 time:572s
fi-snb-2600 total:289 pass:249 dwarn:0 dfail:0 fail:0 skip:40 time:418s
8b0ae6b50a229dc661a02f4034252ee854cc9b83 drm-tip: 2017y-11m-03d-17h-15m-57s UTC integration manifest
cefdb54d87e8 HAX enable guc submission for CI
3b3d7b841574 drm/i915: implemented dynamic WOPCM partition.
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6957/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition.
2017-11-04 0:18 [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition Jackie Li
2017-11-04 0:18 ` [PATCH 2/2] HAX enable guc submission for CI Jackie Li
2017-11-04 0:38 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: implemented dynamic WOPCM partition Patchwork
@ 2017-11-06 8:16 ` Sagar Arun Kamble
2017-11-06 20:22 ` Yaodong Li
2017-11-07 10:52 ` Joonas Lahtinen
3 siblings, 1 reply; 11+ messages in thread
From: Sagar Arun Kamble @ 2017-11-06 8:16 UTC (permalink / raw)
To: Jackie Li, intel-gfx; +Cc: Sujaritha Sundaresan
Please update the subject to "Implement dynamic WOPCM partitioning"
Also, commit description should be written in present tense form.
On 11/4/2017 5:48 AM, Jackie Li wrote:
> Static WOPCM partitioning would lead to GuC loading failure
> if the GuC/HuC firmware size exceeded current static 512KB
> partition boundary.
>
> This patch enabled the dynamical calculation of the WOPCM aperture
> used by GuC/HuC firmware. GuC WOPCM offset was set to
> HuC size + reserved WOPCM size. GuC WOPCM size was set to
> total WOPCM size - GuC WOPCM offset - RC6CTX size.
Could you tell briefly here what is being done to wopcm offset and size
in this patch.
>
> Signed-off-by: Jackie Li <yaodong.li@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: 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>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 15 ++++
> drivers/gpu/drm/i915/i915_drv.h | 13 ++++
> drivers/gpu/drm/i915/i915_gem_context.c | 4 +-
> drivers/gpu/drm/i915/i915_guc_reg.h | 18 ++++-
> drivers/gpu/drm/i915/intel_guc.c | 46 ++++++++++--
> drivers/gpu/drm/i915/intel_guc.h | 18 +----
> drivers/gpu/drm/i915/intel_huc.c | 3 +-
> drivers/gpu/drm/i915/intel_uc.c | 128 +++++++++++++++++++++++++++++++-
> drivers/gpu/drm/i915/intel_uc_fw.c | 12 ++-
> 9 files changed, 223 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e7e9e06..19509fd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -623,6 +623,15 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
> WARN_ON(!list_empty(&dev_priv->contexts.list));
> }
>
> +static void i915_wopcm_init(struct drm_i915_private *dev_priv)
> +{
> + struct intel_wopcm_info *wopcm = &dev_priv->wopcm;
> +
> + wopcm->size = WOPCM_DEFAULT_SIZE;
> +
> + DRM_DEBUG_DRIVER("WOPCM size: %dKB\n", wopcm->size >> 10);
> +}
> +
> static int i915_load_modeset_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -670,6 +679,12 @@ static int i915_load_modeset_init(struct drm_device *dev)
> if (ret)
> goto cleanup_irq;
>
> + /*
> + * Get the wopcm memory info.
> + * NOTE: this need to be called before init FW.
> + */
> + i915_wopcm_init(dev_priv);
> +
I think this call might be better if done from
i915_driver_init_hw->i915_ggtt_probe_hw->*_gmch_probe after
gem_init_stolen as this is part of stolen memory. Might help performing
any validation w.r.t stolen memory alloc.
> intel_uc_init_fw(dev_priv);
>
> ret = i915_gem_init(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 72bb5b5..61cd290 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2235,6 +2235,16 @@ struct intel_cdclk_state {
> u8 voltage_level;
> };
>
> +struct intel_wopcm_info {
> + u32 size;
> +};
> +
> +struct intel_wopcm_partition {
> + u32 guc_wopcm_offset;
> + u32 guc_wopcm_size;
> + u32 guc_wopcm_top;
> +};
> +
*_partition can become part of *_info. This can be named as
intel_guc_wopcm_partition and drop "guc_" prefix from variable names.
> struct drm_i915_private {
> struct drm_device drm;
>
> @@ -2258,6 +2268,9 @@ struct drm_i915_private {
> struct intel_huc huc;
> struct intel_guc guc;
>
> + struct intel_wopcm_info wopcm;
> + struct intel_wopcm_partition wopcm_partition;
> +
> struct intel_csr csr;
>
> struct intel_gmbus gmbus[GMBUS_NUM_PINS];
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 10affb3..7347fd7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -312,12 +312,12 @@ __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 (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
> - ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
> + ctx->ggtt_offset_bias = intel_guc_wopcm_top(dev_priv);
> else
> ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h
> index 35cf991..d309884 100644
> --- a/drivers/gpu/drm/i915/i915_guc_reg.h
> +++ b/drivers/gpu/drm/i915/i915_guc_reg.h
> @@ -67,17 +67,27 @@
> #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)
> #define HUC_FW_VERIFIED (1<<7)
>
> /* Defines WOPCM space available to GuC firmware */
WOPCM memory map here in the documentation will be helpful.
> +/* default WOPCM size 1MB */
> +#define WOPCM_DEFAULT_SIZE (0x1 << 20)
> +/* reserved WOPCM size 16KB */
> +#define WOPCM_RESERVED_SIZE (0x4000)
> +/* GUC WOPCM Offset need to be 16KB aligned */
> +#define WOPCM_OFFSET_ALIGNMENT (0x4000)
> +/* 8KB stack reserved for GuC FW*/
> +#define GUC_WOPCM_STACK_RESERVED (0x2000)
> +/* 24KB WOPCM reserved for RC6 CTX on BXT */
> +#define BXT_WOPCM_RC6_RESERVED (0x6000)
> +
> +#define GEN9_GUC_WOPCM_DELTA 4
> +#define GEN9_GUC_WOPCM_OFFSET (0x24000)
> +
> #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.c b/drivers/gpu/drm/i915/intel_guc.c
> index 9678630..0efcfb4 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -337,7 +337,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.
> @@ -358,7 +358,8 @@ 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 |
> + intel_guc_wopcm_top(dev_priv));
> if (ret) {
> vma = ERR_PTR(ret);
> goto err;
> @@ -373,11 +374,42 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
>
> u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv)
> {
> - u32 wopcm_size = GUC_WOPCM_TOP;
> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
>
> - /* On BXT, the top of WOPCM is reserved for RC6 context */
> - if (IS_GEN9_LP(dev_priv))
> - wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED;
> + GEM_BUG_ON(!wp->guc_wopcm_size);
>
> - return wopcm_size;
> + return wp->guc_wopcm_size;
> +}
> +
> +u32 intel_guc_wopcm_top(struct drm_i915_private *dev_priv)
> +{
> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
> +
> + GEM_BUG_ON(!dev_priv->wopcm.size);
> +
> + return wp->guc_wopcm_top ? wp->guc_wopcm_top : dev_priv->wopcm.size;
> +}
> +
> +u32 intel_guc_wopcm_offset(struct drm_i915_private *dev_priv)
> +{
> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
> +
> + GEM_BUG_ON(!wp->guc_wopcm_size);
> +
> + return wp->guc_wopcm_offset;
> +}
> +
> +/*
> + * GuC does not allow any gfx GGTT address that falls into range [0, WOPCM_TOP),
change 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.
> + */
> +u32 guc_ggtt_offset(struct i915_vma *vma)
This move can be in a separate patch. Instead, I think all above
functions need to be moved to intel_guc.c.
> +{
> + struct drm_i915_private *dev_priv = vma->vm->i915;
> + u32 offset = i915_ggtt_offset(vma);
> +
> + GEM_BUG_ON(offset < intel_guc_wopcm_top(dev_priv));
> + GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
> + return offset;
> }
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 607e025..1493de0 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -100,21 +100,6 @@ static inline void intel_guc_notify(struct intel_guc *guc)
> guc->notify(guc);
> }
>
> -/*
> - * 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.
> - */
> -static inline u32 guc_ggtt_offset(struct i915_vma *vma)
> -{
> - u32 offset = i915_ggtt_offset(vma);
> -
> - GEM_BUG_ON(offset < GUC_WOPCM_TOP);
> - GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
> -
> - return offset;
> -}
>
> void intel_guc_init_early(struct intel_guc *guc);
> void intel_guc_init_send_regs(struct intel_guc *guc);
> @@ -127,5 +112,8 @@ 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);
> +u32 intel_guc_wopcm_top(struct drm_i915_private *dev_priv);
> +u32 intel_guc_wopcm_offset(struct drm_i915_private *dev_priv);
> +u32 guc_ggtt_offset(struct i915_vma *vma);
>
> #endif
> diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> index 98d1725..a985aa5 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -202,7 +202,8 @@ void intel_huc_auth(struct intel_huc *huc)
> return;
>
> vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0,
> - PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> + PIN_OFFSET_BIAS |
> + intel_guc_wopcm_top(i915));
> if (IS_ERR(vma)) {
> DRM_ERROR("failed to pin huc fw object %d\n",
> (int)PTR_ERR(vma));
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index aec2954..83b2516 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -86,6 +86,125 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
> intel_guc_init_early(&dev_priv->guc);
> }
>
> +static u32 rc6_context_size(struct drm_i915_private *dev_priv)
> +{
> + /* On BXT, the top of WOPCM is reserved for RC6 context */
> + if (IS_GEN9_LP(dev_priv))
> + return BXT_WOPCM_RC6_RESERVED;
> +
> + return 0;
> +}
> +
> +static int do_wopcm_partition(struct drm_i915_private *dev_priv,
> + u32 offset, u32 reserved_size)
Alignment issue. parameters should match the open braces spacing above.
Can use checkpatch to spot such issues.
> +{
> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
> + u32 aligned_offset = ALIGN(offset, WOPCM_OFFSET_ALIGNMENT);
> +
> + if (offset >= dev_priv->wopcm.size)
> + return -E2BIG;
> +
> + if (reserved_size >= dev_priv->wopcm.size)
> + return -E2BIG;
> +
> + if ((aligned_offset + reserved_size) >= dev_priv->wopcm.size)
> + return -E2BIG;
> +
> + wp->guc_wopcm_offset = aligned_offset;
> + wp->guc_wopcm_top = dev_priv->wopcm.size - wp->guc_wopcm_offset;
> + wp->guc_wopcm_size = wp->guc_wopcm_top - reserved_size;
> +
> + return 0;
> +}
> +
> +static int intel_uc_init_wopcm_partition(struct drm_i915_private *dev_priv)
> +{
> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
> + struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
> + struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
> + size_t huc_size, guc_size;
> + u32 offset;
> + u32 reserved;
> + u32 wopcm_base;
> + u32 delta;
> + int err;
> +
> + /*Return if WOPCM partition has been initialized*/
> + if (wp->guc_wopcm_size)
> + return 0;
> +
> + GEM_BUG_ON(!dev_priv->wopcm.size);
> +
> + /*No need to do partition if failed to fetch both GuC and HuC FW*/
> + if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS &&
> + huc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> + return -EIO;
> +
> + huc_size = 0;
> + guc_size = 0;
> + offset = WOPCM_RESERVED_SIZE;
> + reserved = rc6_context_size(dev_priv);
> +
> + if (huc_fw->fetch_status == INTEL_UC_FIRMWARE_SUCCESS)
> + huc_size = huc_fw->header_size + huc_fw->ucode_size;
> +
> + err = do_wopcm_partition(dev_priv, offset + huc_size, reserved);
> + if (err) {
> + if (!huc_size)
> + goto pt_done;
> +
> + /*partition failed with HuC FW, block HuC loading*/
> + huc_size = 0;
> +
> + /*partition without loading HuC FW*/
> + err = do_wopcm_partition(dev_priv, offset, reserved);
> + if (err)
> + goto pt_done;
> + }
> +
> + /*
> + * Check hardware restriction on Gen9
> + * GuC WOPCM size is at least 4 bytes larger than GuC WOPCM base due
> + * to hardware limitation on Gen9.
> + */
> + if (IS_GEN9(dev_priv)) {
> + wopcm_base = wp->guc_wopcm_offset + GEN9_GUC_WOPCM_OFFSET;
Should this GEN9_GUC_WOPCM_OFFSET be added to "offset" prior to calling
do_wopcm_partition?
Then it will update wp->guc_wopcm_offset for Gen9 as well. Or that
update is not needed?
> + if (unlikely(wopcm_base > wp->guc_wopcm_size))
> + goto pt_done;
> +
> + delta = wp->guc_wopcm_size - wopcm_base;
> + if (unlikely(delta < GEN9_GUC_WOPCM_DELTA))
> + goto pt_done;
> + }
> +
> + if (guc_fw->fetch_status == INTEL_UC_FIRMWARE_SUCCESS) {
> + guc_size = guc_fw->header_size + guc_fw->ucode_size;
> + /*need 8K stack for GuC*/
> + guc_size += GUC_WOPCM_STACK_RESERVED;
> + }
> +
> + if (guc_size > wp->guc_wopcm_size)
> + guc_size = 0;
> +
> +pt_done:
> + if (!huc_size) {
> + DRM_ERROR("HuC firmware is too large to fit in WOPCM\n");
> + intel_uc_fw_fini(huc_fw);
> + }
> +
> + if (!guc_size) {
> + DRM_ERROR("GuC firmware is too large to fit in WOPCM\n");
> + intel_uc_fw_fini(guc_fw);
> + }
> +
> + DRM_DEBUG_DRIVER("GuC WOPCM offset %dKB, size %dKB, top %dKB\n",
> + wp->guc_wopcm_offset >> 10,
> + wp->guc_wopcm_size >> 10,
> + wp->guc_wopcm_top >> 10);
> +
> + return err;
> +}
> +
> void intel_uc_init_fw(struct drm_i915_private *dev_priv)
> {
> intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
> @@ -157,6 +276,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> if (!i915_modparams.enable_guc_loading)
> return 0;
>
> + /*init WOPCM partition*/
> + ret = intel_uc_init_wopcm_partition(dev_priv);
> + if (ret)
> + goto err_wopcm;
> +
> guc_disable_communication(guc);
> gen9_reset_guc_interrupts(dev_priv);
>
> @@ -176,7 +300,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> /* init WOPCM */
> I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(dev_priv));
> I915_WRITE(DMA_GUC_WOPCM_OFFSET,
> - GUC_WOPCM_OFFSET_VALUE | HUC_LOADING_AGENT_GUC);
> + intel_guc_wopcm_offset(dev_priv) | HUC_LOADING_AGENT_GUC);
>
> /* WaEnableuKernelHeaderValidFix:skl */
> /* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
> @@ -249,7 +373,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> i915_guc_submission_fini(dev_priv);
> err_guc:
> i915_ggtt_disable_guc(dev_priv);
> -
> +err_wopcm:
> if (i915_modparams.enable_guc_loading > 1 ||
> i915_modparams.enable_guc_submission > 1) {
> DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
> index 973888e..aefba13 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)) {
> + if (size > dev_priv->wopcm.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,8 @@ 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 |
> + intel_guc_wopcm_top(i915));
> if (IS_ERR(vma)) {
> err = PTR_ERR(vma);
> DRM_DEBUG_DRIVER("%s fw ggtt-pin err=%d\n",
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition.
2017-11-06 8:16 ` [PATCH 1/2] " Sagar Arun Kamble
@ 2017-11-06 20:22 ` Yaodong Li
0 siblings, 0 replies; 11+ messages in thread
From: Yaodong Li @ 2017-11-06 20:22 UTC (permalink / raw)
To: Sagar Arun Kamble, intel-gfx; +Cc: Sujaritha Sundaresan
On 11/06/2017 12:16 AM, Sagar Arun Kamble wrote:
> Please update the subject to "Implement dynamic WOPCM partitioning"
> Also, commit description should be written in present tense form.
Will update it in v2.
>
> On 11/4/2017 5:48 AM, Jackie Li wrote:
>> Static WOPCM partitioning would lead to GuC loading failure
>> if the GuC/HuC firmware size exceeded current static 512KB
>> partition boundary.
>>
>> This patch enabled the dynamical calculation of the WOPCM aperture
>> used by GuC/HuC firmware. GuC WOPCM offset was set to
>> HuC size + reserved WOPCM size. GuC WOPCM size was set to
>> total WOPCM size - GuC WOPCM offset - RC6CTX size.
> Could you tell briefly here what is being done to wopcm offset and
> size in this patch.
Will update the description in v2.
>>
>> Signed-off-by: Jackie Li <yaodong.li@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: 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>
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 15 ++++
>> drivers/gpu/drm/i915/i915_drv.h | 13 ++++
>> drivers/gpu/drm/i915/i915_gem_context.c | 4 +-
>> drivers/gpu/drm/i915/i915_guc_reg.h | 18 ++++-
>> drivers/gpu/drm/i915/intel_guc.c | 46 ++++++++++--
>> drivers/gpu/drm/i915/intel_guc.h | 18 +----
>> drivers/gpu/drm/i915/intel_huc.c | 3 +-
>> drivers/gpu/drm/i915/intel_uc.c | 128
>> +++++++++++++++++++++++++++++++-
>> drivers/gpu/drm/i915/intel_uc_fw.c | 12 ++-
>> 9 files changed, 223 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index e7e9e06..19509fd 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -623,6 +623,15 @@ static void i915_gem_fini(struct
>> drm_i915_private *dev_priv)
>> WARN_ON(!list_empty(&dev_priv->contexts.list));
>> }
>> +static void i915_wopcm_init(struct drm_i915_private *dev_priv)
>> +{
>> + struct intel_wopcm_info *wopcm = &dev_priv->wopcm;
>> +
>> + wopcm->size = WOPCM_DEFAULT_SIZE;
>> +
>> + DRM_DEBUG_DRIVER("WOPCM size: %dKB\n", wopcm->size >> 10);
>> +}
>> +
>> static int i915_load_modeset_init(struct drm_device *dev)
>> {
>> struct drm_i915_private *dev_priv = to_i915(dev);
>> @@ -670,6 +679,12 @@ static int i915_load_modeset_init(struct
>> drm_device *dev)
>> if (ret)
>> goto cleanup_irq;
>> + /*
>> + * Get the wopcm memory info.
>> + * NOTE: this need to be called before init FW.
>> + */
>> + i915_wopcm_init(dev_priv);
>> +
> I think this call might be better if done from
> i915_driver_init_hw->i915_ggtt_probe_hw->*_gmch_probe after
> gem_init_stolen as this is part of stolen memory. Might help
> performing any validation w.r.t stolen memory alloc.
I am planing the reuse the info from stolen init to create the
description in a separate patch. Likely, I will move it
right after gem_init_stolen, or make it as a part of stolen init, or
even drop this data structure completely.
>> intel_uc_init_fw(dev_priv);
>> ret = i915_gem_init(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 72bb5b5..61cd290 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2235,6 +2235,16 @@ struct intel_cdclk_state {
>> u8 voltage_level;
>> };
>> +struct intel_wopcm_info {
>> + u32 size;
>> +};
>> +
>> +struct intel_wopcm_partition {
>> + u32 guc_wopcm_offset;
>> + u32 guc_wopcm_size;
>> + u32 guc_wopcm_top;
>> +};
>> +
> *_partition can become part of *_info. This can be named as
> intel_guc_wopcm_partition and drop "guc_" prefix from variable names.
intel_wopcm_info was used for description of overall wopcm info while
intel_wopcm_partition is about how we do the partition and it's guc
related. I agree that we can rename it to intel_guc_wopcm_partition and
remove "guc_" prefix. but I think it makes sense keep them as
separated structures since intel_wopcm_info will be updated to reuse
info from stolen init.
>> struct drm_i915_private {
>> struct drm_device drm;
>> @@ -2258,6 +2268,9 @@ struct drm_i915_private {
>> struct intel_huc huc;
>> struct intel_guc guc;
>> + struct intel_wopcm_info wopcm;
>> + struct intel_wopcm_partition wopcm_partition;
>> +
>> struct intel_csr csr;
>> struct intel_gmbus gmbus[GMBUS_NUM_PINS];
>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
>> b/drivers/gpu/drm/i915/i915_gem_context.c
>> index 10affb3..7347fd7 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>> @@ -312,12 +312,12 @@ __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 (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
>> - ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
>> + ctx->ggtt_offset_bias = intel_guc_wopcm_top(dev_priv);
>> else
>> ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
>> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h
>> b/drivers/gpu/drm/i915/i915_guc_reg.h
>> index 35cf991..d309884 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_guc_reg.h
>> @@ -67,17 +67,27 @@
>> #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)
>> #define HUC_FW_VERIFIED (1<<7)
>> /* Defines WOPCM space available to GuC firmware */
> WOPCM memory map here in the documentation will be helpful.
we check on this:)
>> +/* default WOPCM size 1MB */
>> +#define WOPCM_DEFAULT_SIZE (0x1 << 20)
>> +/* reserved WOPCM size 16KB */
>> +#define WOPCM_RESERVED_SIZE (0x4000)
>> +/* GUC WOPCM Offset need to be 16KB aligned */
>> +#define WOPCM_OFFSET_ALIGNMENT (0x4000)
>> +/* 8KB stack reserved for GuC FW*/
>> +#define GUC_WOPCM_STACK_RESERVED (0x2000)
>> +/* 24KB WOPCM reserved for RC6 CTX on BXT */
>> +#define BXT_WOPCM_RC6_RESERVED (0x6000)
>> +
>> +#define GEN9_GUC_WOPCM_DELTA 4
>> +#define GEN9_GUC_WOPCM_OFFSET (0x24000)
>> +
>> #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.c
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index 9678630..0efcfb4 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -337,7 +337,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.
>> @@ -358,7 +358,8 @@ 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 |
>> + intel_guc_wopcm_top(dev_priv));
>> if (ret) {
>> vma = ERR_PTR(ret);
>> goto err;
>> @@ -373,11 +374,42 @@ struct i915_vma *intel_guc_allocate_vma(struct
>> intel_guc *guc, u32 size)
>> u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv)
>> {
>> - u32 wopcm_size = GUC_WOPCM_TOP;
>> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
>> - /* On BXT, the top of WOPCM is reserved for RC6 context */
>> - if (IS_GEN9_LP(dev_priv))
>> - wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED;
>> + GEM_BUG_ON(!wp->guc_wopcm_size);
>> - return wopcm_size;
>> + return wp->guc_wopcm_size;
>> +}
>> +
>> +u32 intel_guc_wopcm_top(struct drm_i915_private *dev_priv)
>> +{
>> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
>> +
>> + GEM_BUG_ON(!dev_priv->wopcm.size);
>> +
>> + return wp->guc_wopcm_top ? wp->guc_wopcm_top :
>> dev_priv->wopcm.size;
>> +}
>> +
>> +u32 intel_guc_wopcm_offset(struct drm_i915_private *dev_priv)
>> +{
>> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
>> +
>> + GEM_BUG_ON(!wp->guc_wopcm_size);
>> +
>> + return wp->guc_wopcm_offset;
>> +}
>> +
>> +/*
>> + * GuC does not allow any gfx GGTT address that falls into range [0,
>> WOPCM_TOP),
> change WOPCM_TOP
Thanks. will update it.
>> + * 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.
>> + */
>> +u32 guc_ggtt_offset(struct i915_vma *vma)
> This move can be in a separate patch. Instead, I think all above
> functions need to be moved to intel_guc.c.
I think it's clear enough to make this move as part of this patch since
the reason for this move is because we
are now using a dynamic calculated wopcm top instead of a static WOPCM_TOP.
All these functions are in intel_guc.c now.
>> +{
>> + struct drm_i915_private *dev_priv = vma->vm->i915;
>> + u32 offset = i915_ggtt_offset(vma);
>> +
>> + GEM_BUG_ON(offset < intel_guc_wopcm_top(dev_priv));
>> + GEM_BUG_ON(range_overflows_t(u64, offset, vma->size,
>> GUC_GGTT_TOP));
>> + return offset;
>> }
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index 607e025..1493de0 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -100,21 +100,6 @@ static inline void intel_guc_notify(struct
>> intel_guc *guc)
>> guc->notify(guc);
>> }
>> -/*
>> - * 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.
>> - */
>> -static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>> -{
>> - u32 offset = i915_ggtt_offset(vma);
>> -
>> - GEM_BUG_ON(offset < GUC_WOPCM_TOP);
>> - GEM_BUG_ON(range_overflows_t(u64, offset, vma->size,
>> GUC_GGTT_TOP));
>> -
>> - return offset;
>> -}
>> void intel_guc_init_early(struct intel_guc *guc);
>> void intel_guc_init_send_regs(struct intel_guc *guc);
>> @@ -127,5 +112,8 @@ 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);
>> +u32 intel_guc_wopcm_top(struct drm_i915_private *dev_priv);
>> +u32 intel_guc_wopcm_offset(struct drm_i915_private *dev_priv);
>> +u32 guc_ggtt_offset(struct i915_vma *vma);
>> #endif
>> diff --git a/drivers/gpu/drm/i915/intel_huc.c
>> b/drivers/gpu/drm/i915/intel_huc.c
>> index 98d1725..a985aa5 100644
>> --- a/drivers/gpu/drm/i915/intel_huc.c
>> +++ b/drivers/gpu/drm/i915/intel_huc.c
>> @@ -202,7 +202,8 @@ void intel_huc_auth(struct intel_huc *huc)
>> return;
>> vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0,
>> - PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
>> + PIN_OFFSET_BIAS |
>> + intel_guc_wopcm_top(i915));
>> if (IS_ERR(vma)) {
>> DRM_ERROR("failed to pin huc fw object %d\n",
>> (int)PTR_ERR(vma));
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index aec2954..83b2516 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -86,6 +86,125 @@ void intel_uc_init_early(struct drm_i915_private
>> *dev_priv)
>> intel_guc_init_early(&dev_priv->guc);
>> }
>> +static u32 rc6_context_size(struct drm_i915_private *dev_priv)
>> +{
>> + /* On BXT, the top of WOPCM is reserved for RC6 context */
>> + if (IS_GEN9_LP(dev_priv))
>> + return BXT_WOPCM_RC6_RESERVED;
>> +
>> + return 0;
>> +}
>> +
>> +static int do_wopcm_partition(struct drm_i915_private *dev_priv,
>> + u32 offset, u32 reserved_size)
> Alignment issue. parameters should match the open braces spacing
> above. Can use checkpatch to spot such issues.
This patch has been check with checkpatch script. However, if that's the
convention, I will modify the code to follow it:)
>
>> +{
>> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
>> + u32 aligned_offset = ALIGN(offset, WOPCM_OFFSET_ALIGNMENT);
>> +
>> + if (offset >= dev_priv->wopcm.size)
>> + return -E2BIG;
>> +
>> + if (reserved_size >= dev_priv->wopcm.size)
>> + return -E2BIG;
>> +
>> + if ((aligned_offset + reserved_size) >= dev_priv->wopcm.size)
>> + return -E2BIG;
>> +
>> + wp->guc_wopcm_offset = aligned_offset;
>> + wp->guc_wopcm_top = dev_priv->wopcm.size - wp->guc_wopcm_offset;
>> + wp->guc_wopcm_size = wp->guc_wopcm_top - reserved_size;
>> +
>> + return 0;
>> +}
>> +
>> +static int intel_uc_init_wopcm_partition(struct drm_i915_private
>> *dev_priv)
>> +{
>> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
>> + struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>> + struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>> + size_t huc_size, guc_size;
>> + u32 offset;
>> + u32 reserved;
>> + u32 wopcm_base;
>> + u32 delta;
>> + int err;
>> +
>> + /*Return if WOPCM partition has been initialized*/
>> + if (wp->guc_wopcm_size)
>> + return 0;
>> +
>> + GEM_BUG_ON(!dev_priv->wopcm.size);
>> +
>> + /*No need to do partition if failed to fetch both GuC and HuC FW*/
>> + if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS &&
>> + huc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
>> + return -EIO;
>> +
>> + huc_size = 0;
>> + guc_size = 0;
>> + offset = WOPCM_RESERVED_SIZE;
>> + reserved = rc6_context_size(dev_priv);
>> +
>> + if (huc_fw->fetch_status == INTEL_UC_FIRMWARE_SUCCESS)
>> + huc_size = huc_fw->header_size + huc_fw->ucode_size;
>> +
>> + err = do_wopcm_partition(dev_priv, offset + huc_size, reserved);
>> + if (err) {
>> + if (!huc_size)
>> + goto pt_done;
>> +
>> + /*partition failed with HuC FW, block HuC loading*/
>> + huc_size = 0;
>> +
>> + /*partition without loading HuC FW*/
>> + err = do_wopcm_partition(dev_priv, offset, reserved);
>> + if (err)
>> + goto pt_done;
>> + }
>> +
>> + /*
>> + * Check hardware restriction on Gen9
>> + * GuC WOPCM size is at least 4 bytes larger than GuC WOPCM base
>> due
>> + * to hardware limitation on Gen9.
>> + */
>> + if (IS_GEN9(dev_priv)) {
>> + wopcm_base = wp->guc_wopcm_offset + GEN9_GUC_WOPCM_OFFSET;
> Should this GEN9_GUC_WOPCM_OFFSET be added to "offset" prior to
> calling do_wopcm_partition?
> Then it will update wp->guc_wopcm_offset for Gen9 as well. Or that
> update is not needed?
This check is only for gen9 and it's only for validating whether the guc
size could meet hardware
requirement. we don't need to include this offset in guc_wopcm_offset.
>> + if (unlikely(wopcm_base > wp->guc_wopcm_size))
>> + goto pt_done;
>> +
>> + delta = wp->guc_wopcm_size - wopcm_base;
>> + if (unlikely(delta < GEN9_GUC_WOPCM_DELTA))
>> + goto pt_done;
>> + }
>> +
>> + if (guc_fw->fetch_status == INTEL_UC_FIRMWARE_SUCCESS) {
>> + guc_size = guc_fw->header_size + guc_fw->ucode_size;
>> + /*need 8K stack for GuC*/
>> + guc_size += GUC_WOPCM_STACK_RESERVED;
>> + }
>> +
>> + if (guc_size > wp->guc_wopcm_size)
>> + guc_size = 0;
>> +
>> +pt_done:
>> + if (!huc_size) {
>> + DRM_ERROR("HuC firmware is too large to fit in WOPCM\n");
>> + intel_uc_fw_fini(huc_fw);
>> + }
>> +
>> + if (!guc_size) {
>> + DRM_ERROR("GuC firmware is too large to fit in WOPCM\n");
>> + intel_uc_fw_fini(guc_fw);
>> + }
>> +
>> + DRM_DEBUG_DRIVER("GuC WOPCM offset %dKB, size %dKB, top %dKB\n",
>> + wp->guc_wopcm_offset >> 10,
>> + wp->guc_wopcm_size >> 10,
>> + wp->guc_wopcm_top >> 10);
>> +
>> + return err;
>> +}
>> +
>> void intel_uc_init_fw(struct drm_i915_private *dev_priv)
>> {
>> intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
>> @@ -157,6 +276,11 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> if (!i915_modparams.enable_guc_loading)
>> return 0;
>> + /*init WOPCM partition*/
>> + ret = intel_uc_init_wopcm_partition(dev_priv);
>> + if (ret)
>> + goto err_wopcm;
>> +
>> guc_disable_communication(guc);
>> gen9_reset_guc_interrupts(dev_priv);
>> @@ -176,7 +300,7 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> /* init WOPCM */
>> I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(dev_priv));
>> I915_WRITE(DMA_GUC_WOPCM_OFFSET,
>> - GUC_WOPCM_OFFSET_VALUE | HUC_LOADING_AGENT_GUC);
>> + intel_guc_wopcm_offset(dev_priv) | HUC_LOADING_AGENT_GUC);
>> /* WaEnableuKernelHeaderValidFix:skl */
>> /* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
>> @@ -249,7 +373,7 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> i915_guc_submission_fini(dev_priv);
>> err_guc:
>> i915_ggtt_disable_guc(dev_priv);
>> -
>> +err_wopcm:
>> if (i915_modparams.enable_guc_loading > 1 ||
>> i915_modparams.enable_guc_submission > 1) {
>> DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
>> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c
>> b/drivers/gpu/drm/i915/intel_uc_fw.c
>> index 973888e..aefba13 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)) {
>> + if (size > dev_priv->wopcm.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,8 @@ 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 |
>> + intel_guc_wopcm_top(i915));
>> if (IS_ERR(vma)) {
>> err = PTR_ERR(vma);
>> DRM_DEBUG_DRIVER("%s fw ggtt-pin err=%d\n",
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition.
2017-11-04 0:18 [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition Jackie Li
` (2 preceding siblings ...)
2017-11-06 8:16 ` [PATCH 1/2] " Sagar Arun Kamble
@ 2017-11-07 10:52 ` Joonas Lahtinen
2017-11-09 22:18 ` Yaodong Li
3 siblings, 1 reply; 11+ messages in thread
From: Joonas Lahtinen @ 2017-11-07 10:52 UTC (permalink / raw)
To: Jackie Li, intel-gfx; +Cc: Sujaritha Sundaresan
On Fri, 2017-11-03 at 17:18 -0700, Jackie Li wrote:
> Static WOPCM partitioning would lead to GuC loading failure
> if the GuC/HuC firmware size exceeded current static 512KB
> partition boundary.
>
> This patch enabled the dynamical calculation of the WOPCM aperture
> used by GuC/HuC firmware. GuC WOPCM offset was set to
> HuC size + reserved WOPCM size. GuC WOPCM size was set to
> total WOPCM size - GuC WOPCM offset - RC6CTX size.
>
> Signed-off-by: Jackie Li <yaodong.li@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: 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>
<SNIP>
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -623,6 +623,15 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
> WARN_ON(!list_empty(&dev_priv->contexts.list));
> }
>
> +static void i915_wopcm_init(struct drm_i915_private *dev_priv)
Use "struct drm_i915_private *i915" when introducing new code that is
not fiddling with MMIOs.
> +{
> + struct intel_wopcm_info *wopcm = &dev_priv->wopcm;
> +
> + wopcm->size = WOPCM_DEFAULT_SIZE;
> +
> + DRM_DEBUG_DRIVER("WOPCM size: %dKB\n", wopcm->size >> 10);
> +}
> +
> static int i915_load_modeset_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -670,6 +679,12 @@ static int i915_load_modeset_init(struct drm_device *dev)
> if (ret)
> goto cleanup_irq;
>
> + /*
> + * Get the wopcm memory info.
> + * NOTE: this need to be called before init FW.
> + */
Useless comments. Comments should always answer question "why?", not
"what?". And "why?" only needs answering when the code is more obscure
than this. So when the code reads clearly and you don't need comments
inside the function bodies.
> + i915_wopcm_init(dev_priv);
> +
> intel_uc_init_fw(dev_priv);
WOPCM is the reserved memory for the uC's. Why couldn't it be inside
the *_uc_* functions? It's only relevant when you have the uCs.
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -337,7 +337,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.
> @@ -358,7 +358,8 @@ 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 |
> + intel_guc_wopcm_top(dev_priv));
Could read just as "guc->wopcm.top"? Because we're not going to change
it runtime, we could avoid a function. It's a one-off programmable
register after all.
> @@ -373,11 +374,42 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
>
> u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv)
> {
> - u32 wopcm_size = GUC_WOPCM_TOP;
> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
>
> - /* On BXT, the top of WOPCM is reserved for RC6 context */
> - if (IS_GEN9_LP(dev_priv))
> - wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED;
> + GEM_BUG_ON(!wp->guc_wopcm_size);
>
> - return wopcm_size;
> + return wp->guc_wopcm_size;
> +}
> +
> +u32 intel_guc_wopcm_top(struct drm_i915_private *dev_priv)
> +{
> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
> +
> + GEM_BUG_ON(!dev_priv->wopcm.size);
> +
> + return wp->guc_wopcm_top ? wp->guc_wopcm_top : dev_priv->wopcm.size;
> +}
> +
> +u32 intel_guc_wopcm_offset(struct drm_i915_private *dev_priv)
> +{
> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
> +
> + GEM_BUG_ON(!wp->guc_wopcm_size);
> +
> + return wp->guc_wopcm_offset;
> +}
> +
> +/*
> + * GuC does not allow any gfx GGTT address that falls into range [0, 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.
> + */
> +u32 guc_ggtt_offset(struct i915_vma *vma)
> +{
> + struct drm_i915_private *dev_priv = vma->vm->i915;
> + u32 offset = i915_ggtt_offset(vma);
> +
> + GEM_BUG_ON(offset < intel_guc_wopcm_top(dev_priv));
> + GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
> + return offset;
> }
Function prefixes and parameters are not following the convention.
There also needs to be proper kerneldoc for newly exposed functions.
Then, to the big question, why not address this the way of just once
calculating the hole? Then simply using the calculated values just like
*stolen_reserved* properties.
Apart from the high level questions, please sync with somebody local to
your office about the i915 coding style conventions, it'll more
efficient to go through rest of the code in more interactive manner for
a better learning experience.
We should try to avoid making even the simplest operations a function
call (especially one that is not inline). And as far as I understand,
the register can be written exactly once so we are not expecting that
dynamic operation.
While going through the code, please also implement the feature of
reading the WOPCM register and going with that value, if it's been
written to already when driver is initializing. We could be the second
driver initialization after 'rmmod i915' for example.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition.
2017-11-07 10:52 ` Joonas Lahtinen
@ 2017-11-09 22:18 ` Yaodong Li
0 siblings, 0 replies; 11+ messages in thread
From: Yaodong Li @ 2017-11-09 22:18 UTC (permalink / raw)
To: Joonas Lahtinen, intel-gfx; +Cc: Sujaritha Sundaresan
On 11/07/2017 02:52 AM, Joonas Lahtinen wrote:
> On Fri, 2017-11-03 at 17:18 -0700, Jackie Li wrote:
>> Static WOPCM partitioning would lead to GuC loading failure
>> if the GuC/HuC firmware size exceeded current static 512KB
>> partition boundary.
>>
>> This patch enabled the dynamical calculation of the WOPCM aperture
>> used by GuC/HuC firmware. GuC WOPCM offset was set to
>> HuC size + reserved WOPCM size. GuC WOPCM size was set to
>> total WOPCM size - GuC WOPCM offset - RC6CTX size.
>>
>> Signed-off-by: Jackie Li <yaodong.li@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: 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>
> <SNIP>
>
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -623,6 +623,15 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
>> WARN_ON(!list_empty(&dev_priv->contexts.list));
>> }
>>
>> +static void i915_wopcm_init(struct drm_i915_private *dev_priv)
> Use "struct drm_i915_private *i915" when introducing new code that is
> not fiddling with MMIOs.
Will update it.
>> +{
>> + struct intel_wopcm_info *wopcm = &dev_priv->wopcm;
>> +
>> + wopcm->size = WOPCM_DEFAULT_SIZE;
>> +
>> + DRM_DEBUG_DRIVER("WOPCM size: %dKB\n", wopcm->size >> 10);
>> +}
>> +
>> static int i915_load_modeset_init(struct drm_device *dev)
>> {
>> struct drm_i915_private *dev_priv = to_i915(dev);
>> @@ -670,6 +679,12 @@ static int i915_load_modeset_init(struct drm_device *dev)
>> if (ret)
>> goto cleanup_irq;
>>
>> + /*
>> + * Get the wopcm memory info.
>> + * NOTE: this need to be called before init FW.
>> + */
> Useless comments. Comments should always answer question "why?", not
> "what?". And "why?" only needs answering when the code is more obscure
> than this. So when the code reads clearly and you don't need comments
> inside the function bodies.
Will remove it.
>> + i915_wopcm_init(dev_priv);
>> +
>> intel_uc_init_fw(dev_priv);
> WOPCM is the reserved memory for the uC's. Why couldn't it be inside
> the *_uc_* functions? It's only relevant when you have the uCs.
They are related, but different concepts. WOPCM will be there no
matter whether we use uC or not. On the other hand, we need get
the wopcm size before fetching the FW and creating kernel context
which means intel_uc_init_fw would be the best place in uC code to
call this init function. in this case it will be called no matter
whether uC's
active or not. I think it makes more sense to merge it into ggtt/stolen
init.
(or would drop this call completely if possible.)
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -337,7 +337,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.
>> @@ -358,7 +358,8 @@ 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 |
>> + intel_guc_wopcm_top(dev_priv));
> Could read just as "guc->wopcm.top"? Because we're not going to change
> it runtime, we could avoid a function. It's a one-off programmable
> register after all.
Good idea! The reason to put it into a function was to do an assert
to make sure wopcm partition was initialized with valid offset, size values.
However, we would never get here if wopcm partition initialization failed,
so will update the code to access the value directly.
>> @@ -373,11 +374,42 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
>>
>> u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv)
>> {
>> - u32 wopcm_size = GUC_WOPCM_TOP;
>> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
>>
>> - /* On BXT, the top of WOPCM is reserved for RC6 context */
>> - if (IS_GEN9_LP(dev_priv))
>> - wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED;
>> + GEM_BUG_ON(!wp->guc_wopcm_size);
>>
>> - return wopcm_size;
>> + return wp->guc_wopcm_size;
>> +}
>> +
>> +u32 intel_guc_wopcm_top(struct drm_i915_private *dev_priv)
>> +{
>> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
>> +
>> + GEM_BUG_ON(!dev_priv->wopcm.size);
>> +
>> + return wp->guc_wopcm_top ? wp->guc_wopcm_top : dev_priv->wopcm.size;
>> +}
>> +
>> +u32 intel_guc_wopcm_offset(struct drm_i915_private *dev_priv)
>> +{
>> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
>> +
>> + GEM_BUG_ON(!wp->guc_wopcm_size);
>> +
>> + return wp->guc_wopcm_offset;
>> +}
>> +
>> +/*
>> + * GuC does not allow any gfx GGTT address that falls into range [0, 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.
>> + */
>> +u32 guc_ggtt_offset(struct i915_vma *vma)
>> +{
>> + struct drm_i915_private *dev_priv = vma->vm->i915;
>> + u32 offset = i915_ggtt_offset(vma);
>> +
>> + GEM_BUG_ON(offset < intel_guc_wopcm_top(dev_priv));
>> + GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
>> + return offset;
>> }
> Function prefixes and parameters are not following the convention.
> There also needs to be proper kerneldoc for newly exposed functions.
>
> Then, to the big question, why not address this the way of just once
> calculating the hole? Then simply using the calculated values just like
> *stolen_reserved* properties.
>
> Apart from the high level questions, please sync with somebody local to
> your office about the i915 coding style conventions, it'll more
> efficient to go through rest of the code in more interactive manner for
> a better learning experience.
>
> We should try to avoid making even the simplest operations a function
> call (especially one that is not inline). And as far as I understand,
> the register can be written exactly once so we are not expecting that
> dynamic operation.
>
> While going through the code, please also implement the feature of
> reading the WOPCM register and going with that value, if it's been
> written to already when driver is initializing. We could be the second
> driver initialization after 'rmmod i915' for example.
Actually, I was trying to following the original coding style here :o)
(e.g. there were functions such as intel_guc_wopcm_size in original
i915 code).
As for the reason for using function calls, I thought it will be safer to
have an assertion before returning these values. However, we do have
reasons not to do such a check (since these functions won't even be called
after guc initialization failure, expect for wopcm_top which we need it
in kernel
context creation). so I probably rework the patch to remove unnecessary
use of function calls in v2.
Regarding the inline, two reasons didn't use inline functions:
a) Wanted to be consistent with existing code convention.
b) No way to use *static inline* to implement this in intel_guc.h since
they need to access dev_priv fields. This can be solve by moving
guc wopcm partition into intel_guc. However, it's still a problem for
guc_gtt_offset if we still want to do GEM_BUG_ON(offset < wopcm top).
Any suggestions here?
I will definitely triple check with an expert for more feedbacks regarding
the i915 coding style and convention. it's always a pleasure to receive
valuable
feedbacks and new knowledges :-)
Thanks for the comments and explanation!
>
> Regards, Joonas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition.
2017-11-06 13:24 ` Ville Syrjälä
@ 2017-11-06 18:20 ` Yaodong Li
0 siblings, 0 replies; 11+ messages in thread
From: Yaodong Li @ 2017-11-06 18:20 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, Sujaritha Sundaresan
On 11/06/2017 05:24 AM, Ville Syrjälä wrote:
> On Fri, Nov 03, 2017 at 05:01:09PM -0700, Jackie Li wrote:
>> Static WOPCM partitioning would lead to GuC loading failure
>> if the GuC/HuC firmware size exceeded current static 512KB
>> partition boundary.
>>
>> This patch enabled the dynamical calculation of the WOPCM aperture
>> used by GuC/HuC firmware. GuC WOPCM offset was set to
>> HuC size + reserved WOPCM size. GuC WOPCM size was set to
>> total WOPCM size - GuC WOPCM offset - RC6CTX size.
>>
>> Signed-off-by: Jackie Li <yaodong.li@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Reviewed-by: John Spotswood <john.a.spotswood@intel.com>
>> Reviewed-by: Oscar Mateo <oscar.mateo@intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 15 ++++
>> drivers/gpu/drm/i915/i915_drv.h | 13 ++++
>> drivers/gpu/drm/i915/i915_gem_context.c | 4 +-
>> drivers/gpu/drm/i915/i915_guc_reg.h | 18 ++++-
>> drivers/gpu/drm/i915/intel_guc.c | 46 ++++++++++--
>> drivers/gpu/drm/i915/intel_guc.h | 18 +----
>> drivers/gpu/drm/i915/intel_huc.c | 3 +-
>> drivers/gpu/drm/i915/intel_uc.c | 128 +++++++++++++++++++++++++++++++-
>> drivers/gpu/drm/i915/intel_uc_fw.c | 12 ++-
>> 9 files changed, 223 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index e7e9e06..19509fd 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -623,6 +623,15 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
>> WARN_ON(!list_empty(&dev_priv->contexts.list));
>> }
>>
>> +static void i915_wopcm_init(struct drm_i915_private *dev_priv)
>> +{
>> + struct intel_wopcm_info *wopcm = &dev_priv->wopcm;
>> +
>> + wopcm->size = WOPCM_DEFAULT_SIZE;
>> +
>> + DRM_DEBUG_DRIVER("WOPCM size: %dKB\n", wopcm->size >> 10);
>> +}
>> +
>> static int i915_load_modeset_init(struct drm_device *dev)
>> {
>> struct drm_i915_private *dev_priv = to_i915(dev);
>> @@ -670,6 +679,12 @@ static int i915_load_modeset_init(struct drm_device *dev)
>> if (ret)
>> goto cleanup_irq;
>>
>> + /*
>> + * Get the wopcm memory info.
>> + * NOTE: this need to be called before init FW.
>> + */
>> + i915_wopcm_init(dev_priv);
> Is this guc wopcm somehow related to the normal wopcm? And if so
> are you planning to use the wopcm information we extract from the
> hardware in stolen init? If this is just related to the guc then
> it would seem better to bury this in some guc code.
Thanks for the comments. Yes. I am planning to reuse these
information from stolen init to create an description of overall wopcm info.
the guc related use of wopcm was encapsulated in intel_wopcm_partition.
The reason I put the initialization code here is that we are doing
a size check against the wopcm size during intel_uc_fw_fetch (so
that we won't waste time to create a gem object for a firmware object
whose size is larger than wopcm size) and we also need to guarantee the
wopcm info are available before the creation of the kernel gem context
since we need place the gem context above wopcm when guc is active.
>> +
>> intel_uc_init_fw(dev_priv);
>>
>> ret = i915_gem_init(dev_priv);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition.
2017-11-04 0:01 Jackie Li
2017-11-04 0:16 ` Yaodong Li
@ 2017-11-06 13:24 ` Ville Syrjälä
2017-11-06 18:20 ` Yaodong Li
1 sibling, 1 reply; 11+ messages in thread
From: Ville Syrjälä @ 2017-11-06 13:24 UTC (permalink / raw)
To: Jackie Li; +Cc: intel-gfx, Sujaritha Sundaresan
On Fri, Nov 03, 2017 at 05:01:09PM -0700, Jackie Li wrote:
> Static WOPCM partitioning would lead to GuC loading failure
> if the GuC/HuC firmware size exceeded current static 512KB
> partition boundary.
>
> This patch enabled the dynamical calculation of the WOPCM aperture
> used by GuC/HuC firmware. GuC WOPCM offset was set to
> HuC size + reserved WOPCM size. GuC WOPCM size was set to
> total WOPCM size - GuC WOPCM offset - RC6CTX size.
>
> Signed-off-by: Jackie Li <yaodong.li@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Reviewed-by: John Spotswood <john.a.spotswood@intel.com>
> Reviewed-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 15 ++++
> drivers/gpu/drm/i915/i915_drv.h | 13 ++++
> drivers/gpu/drm/i915/i915_gem_context.c | 4 +-
> drivers/gpu/drm/i915/i915_guc_reg.h | 18 ++++-
> drivers/gpu/drm/i915/intel_guc.c | 46 ++++++++++--
> drivers/gpu/drm/i915/intel_guc.h | 18 +----
> drivers/gpu/drm/i915/intel_huc.c | 3 +-
> drivers/gpu/drm/i915/intel_uc.c | 128 +++++++++++++++++++++++++++++++-
> drivers/gpu/drm/i915/intel_uc_fw.c | 12 ++-
> 9 files changed, 223 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e7e9e06..19509fd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -623,6 +623,15 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
> WARN_ON(!list_empty(&dev_priv->contexts.list));
> }
>
> +static void i915_wopcm_init(struct drm_i915_private *dev_priv)
> +{
> + struct intel_wopcm_info *wopcm = &dev_priv->wopcm;
> +
> + wopcm->size = WOPCM_DEFAULT_SIZE;
> +
> + DRM_DEBUG_DRIVER("WOPCM size: %dKB\n", wopcm->size >> 10);
> +}
> +
> static int i915_load_modeset_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -670,6 +679,12 @@ static int i915_load_modeset_init(struct drm_device *dev)
> if (ret)
> goto cleanup_irq;
>
> + /*
> + * Get the wopcm memory info.
> + * NOTE: this need to be called before init FW.
> + */
> + i915_wopcm_init(dev_priv);
Is this guc wopcm somehow related to the normal wopcm? And if so
are you planning to use the wopcm information we extract from the
hardware in stolen init? If this is just related to the guc then
it would seem better to bury this in some guc code.
> +
> intel_uc_init_fw(dev_priv);
>
> ret = i915_gem_init(dev_priv);
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition.
2017-11-04 0:01 Jackie Li
@ 2017-11-04 0:16 ` Yaodong Li
2017-11-06 13:24 ` Ville Syrjälä
1 sibling, 0 replies; 11+ messages in thread
From: Yaodong Li @ 2017-11-04 0:16 UTC (permalink / raw)
To: intel-gfx; +Cc: Sujaritha Sundaresan
On 11/03/2017 05:01 PM, Jackie Li wrote:
> Static WOPCM partitioning would lead to GuC loading failure
> if the GuC/HuC firmware size exceeded current static 512KB
> partition boundary.
>
> This patch enabled the dynamical calculation of the WOPCM aperture
> used by GuC/HuC firmware. GuC WOPCM offset was set to
> HuC size + reserved WOPCM size. GuC WOPCM size was set to
> total WOPCM size - GuC WOPCM offset - RC6CTX size.
>
> Signed-off-by: Jackie Li <yaodong.li@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Reviewed-by: John Spotswood <john.a.spotswood@intel.com>
> Reviewed-by: Oscar Mateo <oscar.mateo@intel.com>
Sorry, these Reviewed-by should be Cc. I will resend the patch.
> ---
> drivers/gpu/drm/i915/i915_drv.c | 15 ++++
> drivers/gpu/drm/i915/i915_drv.h | 13 ++++
> drivers/gpu/drm/i915/i915_gem_context.c | 4 +-
> drivers/gpu/drm/i915/i915_guc_reg.h | 18 ++++-
> drivers/gpu/drm/i915/intel_guc.c | 46 ++++++++++--
> drivers/gpu/drm/i915/intel_guc.h | 18 +----
> drivers/gpu/drm/i915/intel_huc.c | 3 +-
> drivers/gpu/drm/i915/intel_uc.c | 128 +++++++++++++++++++++++++++++++-
> drivers/gpu/drm/i915/intel_uc_fw.c | 12 ++-
> 9 files changed, 223 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e7e9e06..19509fd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -623,6 +623,15 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
> WARN_ON(!list_empty(&dev_priv->contexts.list));
> }
>
> +static void i915_wopcm_init(struct drm_i915_private *dev_priv)
> +{
> + struct intel_wopcm_info *wopcm = &dev_priv->wopcm;
> +
> + wopcm->size = WOPCM_DEFAULT_SIZE;
> +
> + DRM_DEBUG_DRIVER("WOPCM size: %dKB\n", wopcm->size >> 10);
> +}
> +
> static int i915_load_modeset_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -670,6 +679,12 @@ static int i915_load_modeset_init(struct drm_device *dev)
> if (ret)
> goto cleanup_irq;
>
> + /*
> + * Get the wopcm memory info.
> + * NOTE: this need to be called before init FW.
> + */
> + i915_wopcm_init(dev_priv);
> +
> intel_uc_init_fw(dev_priv);
>
> ret = i915_gem_init(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 72bb5b5..61cd290 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2235,6 +2235,16 @@ struct intel_cdclk_state {
> u8 voltage_level;
> };
>
> +struct intel_wopcm_info {
> + u32 size;
> +};
> +
> +struct intel_wopcm_partition {
> + u32 guc_wopcm_offset;
> + u32 guc_wopcm_size;
> + u32 guc_wopcm_top;
> +};
> +
> struct drm_i915_private {
> struct drm_device drm;
>
> @@ -2258,6 +2268,9 @@ struct drm_i915_private {
> struct intel_huc huc;
> struct intel_guc guc;
>
> + struct intel_wopcm_info wopcm;
> + struct intel_wopcm_partition wopcm_partition;
> +
> struct intel_csr csr;
>
> struct intel_gmbus gmbus[GMBUS_NUM_PINS];
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 10affb3..7347fd7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -312,12 +312,12 @@ __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 (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
> - ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
> + ctx->ggtt_offset_bias = intel_guc_wopcm_top(dev_priv);
> else
> ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h
> index 35cf991..d309884 100644
> --- a/drivers/gpu/drm/i915/i915_guc_reg.h
> +++ b/drivers/gpu/drm/i915/i915_guc_reg.h
> @@ -67,17 +67,27 @@
> #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)
> #define HUC_FW_VERIFIED (1<<7)
>
> /* Defines WOPCM space available to GuC firmware */
> +/* default WOPCM size 1MB */
> +#define WOPCM_DEFAULT_SIZE (0x1 << 20)
> +/* reserved WOPCM size 16KB */
> +#define WOPCM_RESERVED_SIZE (0x4000)
> +/* GUC WOPCM Offset need to be 16KB aligned */
> +#define WOPCM_OFFSET_ALIGNMENT (0x4000)
> +/* 8KB stack reserved for GuC FW*/
> +#define GUC_WOPCM_STACK_RESERVED (0x2000)
> +/* 24KB WOPCM reserved for RC6 CTX on BXT */
> +#define BXT_WOPCM_RC6_RESERVED (0x6000)
> +
> +#define GEN9_GUC_WOPCM_DELTA 4
> +#define GEN9_GUC_WOPCM_OFFSET (0x24000)
> +
> #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.c b/drivers/gpu/drm/i915/intel_guc.c
> index 9678630..0efcfb4 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -337,7 +337,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.
> @@ -358,7 +358,8 @@ 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 |
> + intel_guc_wopcm_top(dev_priv));
> if (ret) {
> vma = ERR_PTR(ret);
> goto err;
> @@ -373,11 +374,42 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
>
> u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv)
> {
> - u32 wopcm_size = GUC_WOPCM_TOP;
> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
>
> - /* On BXT, the top of WOPCM is reserved for RC6 context */
> - if (IS_GEN9_LP(dev_priv))
> - wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED;
> + GEM_BUG_ON(!wp->guc_wopcm_size);
>
> - return wopcm_size;
> + return wp->guc_wopcm_size;
> +}
> +
> +u32 intel_guc_wopcm_top(struct drm_i915_private *dev_priv)
> +{
> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
> +
> + GEM_BUG_ON(!dev_priv->wopcm.size);
> +
> + return wp->guc_wopcm_top ? wp->guc_wopcm_top : dev_priv->wopcm.size;
> +}
> +
> +u32 intel_guc_wopcm_offset(struct drm_i915_private *dev_priv)
> +{
> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
> +
> + GEM_BUG_ON(!wp->guc_wopcm_size);
> +
> + return wp->guc_wopcm_offset;
> +}
> +
> +/*
> + * GuC does not allow any gfx GGTT address that falls into range [0, 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.
> + */
> +u32 guc_ggtt_offset(struct i915_vma *vma)
> +{
> + struct drm_i915_private *dev_priv = vma->vm->i915;
> + u32 offset = i915_ggtt_offset(vma);
> +
> + GEM_BUG_ON(offset < intel_guc_wopcm_top(dev_priv));
> + GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
> + return offset;
> }
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 607e025..1493de0 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -100,21 +100,6 @@ static inline void intel_guc_notify(struct intel_guc *guc)
> guc->notify(guc);
> }
>
> -/*
> - * 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.
> - */
> -static inline u32 guc_ggtt_offset(struct i915_vma *vma)
> -{
> - u32 offset = i915_ggtt_offset(vma);
> -
> - GEM_BUG_ON(offset < GUC_WOPCM_TOP);
> - GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
> -
> - return offset;
> -}
>
> void intel_guc_init_early(struct intel_guc *guc);
> void intel_guc_init_send_regs(struct intel_guc *guc);
> @@ -127,5 +112,8 @@ 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);
> +u32 intel_guc_wopcm_top(struct drm_i915_private *dev_priv);
> +u32 intel_guc_wopcm_offset(struct drm_i915_private *dev_priv);
> +u32 guc_ggtt_offset(struct i915_vma *vma);
>
> #endif
> diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> index 98d1725..a985aa5 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -202,7 +202,8 @@ void intel_huc_auth(struct intel_huc *huc)
> return;
>
> vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0,
> - PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> + PIN_OFFSET_BIAS |
> + intel_guc_wopcm_top(i915));
> if (IS_ERR(vma)) {
> DRM_ERROR("failed to pin huc fw object %d\n",
> (int)PTR_ERR(vma));
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index aec2954..83b2516 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -86,6 +86,125 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
> intel_guc_init_early(&dev_priv->guc);
> }
>
> +static u32 rc6_context_size(struct drm_i915_private *dev_priv)
> +{
> + /* On BXT, the top of WOPCM is reserved for RC6 context */
> + if (IS_GEN9_LP(dev_priv))
> + return BXT_WOPCM_RC6_RESERVED;
> +
> + return 0;
> +}
> +
> +static int do_wopcm_partition(struct drm_i915_private *dev_priv,
> + u32 offset, u32 reserved_size)
> +{
> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
> + u32 aligned_offset = ALIGN(offset, WOPCM_OFFSET_ALIGNMENT);
> +
> + if (offset >= dev_priv->wopcm.size)
> + return -E2BIG;
> +
> + if (reserved_size >= dev_priv->wopcm.size)
> + return -E2BIG;
> +
> + if ((aligned_offset + reserved_size) >= dev_priv->wopcm.size)
> + return -E2BIG;
> +
> + wp->guc_wopcm_offset = aligned_offset;
> + wp->guc_wopcm_top = dev_priv->wopcm.size - wp->guc_wopcm_offset;
> + wp->guc_wopcm_size = wp->guc_wopcm_top - reserved_size;
> +
> + return 0;
> +}
> +
> +static int intel_uc_init_wopcm_partition(struct drm_i915_private *dev_priv)
> +{
> + struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
> + struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
> + struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
> + size_t huc_size, guc_size;
> + u32 offset;
> + u32 reserved;
> + u32 wopcm_base;
> + u32 delta;
> + int err;
> +
> + /*Return if WOPCM partition has been initialized*/
> + if (wp->guc_wopcm_size)
> + return 0;
> +
> + GEM_BUG_ON(!dev_priv->wopcm.size);
> +
> + /*No need to do partition if failed to fetch both GuC and HuC FW*/
> + if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS &&
> + huc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> + return -EIO;
> +
> + huc_size = 0;
> + guc_size = 0;
> + offset = WOPCM_RESERVED_SIZE;
> + reserved = rc6_context_size(dev_priv);
> +
> + if (huc_fw->fetch_status == INTEL_UC_FIRMWARE_SUCCESS)
> + huc_size = huc_fw->header_size + huc_fw->ucode_size;
> +
> + err = do_wopcm_partition(dev_priv, offset + huc_size, reserved);
> + if (err) {
> + if (!huc_size)
> + goto pt_done;
> +
> + /*partition failed with HuC FW, block HuC loading*/
> + huc_size = 0;
> +
> + /*partition without loading HuC FW*/
> + err = do_wopcm_partition(dev_priv, offset, reserved);
> + if (err)
> + goto pt_done;
> + }
> +
> + /*
> + * Check hardware restriction on Gen9
> + * GuC WOPCM size is at least 4 bytes larger than GuC WOPCM base due
> + * to hardware limitation on Gen9.
> + */
> + if (IS_GEN9(dev_priv)) {
> + wopcm_base = wp->guc_wopcm_offset + GEN9_GUC_WOPCM_OFFSET;
> + if (unlikely(wopcm_base > wp->guc_wopcm_size))
> + goto pt_done;
> +
> + delta = wp->guc_wopcm_size - wopcm_base;
> + if (unlikely(delta < GEN9_GUC_WOPCM_DELTA))
> + goto pt_done;
> + }
> +
> + if (guc_fw->fetch_status == INTEL_UC_FIRMWARE_SUCCESS) {
> + guc_size = guc_fw->header_size + guc_fw->ucode_size;
> + /*need 8K stack for GuC*/
> + guc_size += GUC_WOPCM_STACK_RESERVED;
> + }
> +
> + if (guc_size > wp->guc_wopcm_size)
> + guc_size = 0;
> +
> +pt_done:
> + if (!huc_size) {
> + DRM_ERROR("HuC firmware is too large to fit in WOPCM\n");
> + intel_uc_fw_fini(huc_fw);
> + }
> +
> + if (!guc_size) {
> + DRM_ERROR("GuC firmware is too large to fit in WOPCM\n");
> + intel_uc_fw_fini(guc_fw);
> + }
> +
> + DRM_DEBUG_DRIVER("GuC WOPCM offset %dKB, size %dKB, top %dKB\n",
> + wp->guc_wopcm_offset >> 10,
> + wp->guc_wopcm_size >> 10,
> + wp->guc_wopcm_top >> 10);
> +
> + return err;
> +}
> +
> void intel_uc_init_fw(struct drm_i915_private *dev_priv)
> {
> intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
> @@ -157,6 +276,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> if (!i915_modparams.enable_guc_loading)
> return 0;
>
> + /*init WOPCM partition*/
> + ret = intel_uc_init_wopcm_partition(dev_priv);
> + if (ret)
> + goto err_wopcm;
> +
> guc_disable_communication(guc);
> gen9_reset_guc_interrupts(dev_priv);
>
> @@ -176,7 +300,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> /* init WOPCM */
> I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(dev_priv));
> I915_WRITE(DMA_GUC_WOPCM_OFFSET,
> - GUC_WOPCM_OFFSET_VALUE | HUC_LOADING_AGENT_GUC);
> + intel_guc_wopcm_offset(dev_priv) | HUC_LOADING_AGENT_GUC);
>
> /* WaEnableuKernelHeaderValidFix:skl */
> /* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
> @@ -249,7 +373,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> i915_guc_submission_fini(dev_priv);
> err_guc:
> i915_ggtt_disable_guc(dev_priv);
> -
> +err_wopcm:
> if (i915_modparams.enable_guc_loading > 1 ||
> i915_modparams.enable_guc_submission > 1) {
> DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
> index 973888e..aefba13 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)) {
> + if (size > dev_priv->wopcm.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,8 @@ 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 |
> + intel_guc_wopcm_top(i915));
> if (IS_ERR(vma)) {
> err = PTR_ERR(vma);
> DRM_DEBUG_DRIVER("%s fw ggtt-pin err=%d\n",
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition.
@ 2017-11-04 0:01 Jackie Li
2017-11-04 0:16 ` Yaodong Li
2017-11-06 13:24 ` Ville Syrjälä
0 siblings, 2 replies; 11+ messages in thread
From: Jackie Li @ 2017-11-04 0:01 UTC (permalink / raw)
To: intel-gfx; +Cc: Sujaritha Sundaresan
Static WOPCM partitioning would lead to GuC loading failure
if the GuC/HuC firmware size exceeded current static 512KB
partition boundary.
This patch enabled the dynamical calculation of the WOPCM aperture
used by GuC/HuC firmware. GuC WOPCM offset was set to
HuC size + reserved WOPCM size. GuC WOPCM size was set to
total WOPCM size - GuC WOPCM offset - RC6CTX size.
Signed-off-by: Jackie Li <yaodong.li@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Reviewed-by: John Spotswood <john.a.spotswood@intel.com>
Reviewed-by: Oscar Mateo <oscar.mateo@intel.com>
---
drivers/gpu/drm/i915/i915_drv.c | 15 ++++
drivers/gpu/drm/i915/i915_drv.h | 13 ++++
drivers/gpu/drm/i915/i915_gem_context.c | 4 +-
drivers/gpu/drm/i915/i915_guc_reg.h | 18 ++++-
drivers/gpu/drm/i915/intel_guc.c | 46 ++++++++++--
drivers/gpu/drm/i915/intel_guc.h | 18 +----
drivers/gpu/drm/i915/intel_huc.c | 3 +-
drivers/gpu/drm/i915/intel_uc.c | 128 +++++++++++++++++++++++++++++++-
drivers/gpu/drm/i915/intel_uc_fw.c | 12 ++-
9 files changed, 223 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e7e9e06..19509fd 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -623,6 +623,15 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
WARN_ON(!list_empty(&dev_priv->contexts.list));
}
+static void i915_wopcm_init(struct drm_i915_private *dev_priv)
+{
+ struct intel_wopcm_info *wopcm = &dev_priv->wopcm;
+
+ wopcm->size = WOPCM_DEFAULT_SIZE;
+
+ DRM_DEBUG_DRIVER("WOPCM size: %dKB\n", wopcm->size >> 10);
+}
+
static int i915_load_modeset_init(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = to_i915(dev);
@@ -670,6 +679,12 @@ static int i915_load_modeset_init(struct drm_device *dev)
if (ret)
goto cleanup_irq;
+ /*
+ * Get the wopcm memory info.
+ * NOTE: this need to be called before init FW.
+ */
+ i915_wopcm_init(dev_priv);
+
intel_uc_init_fw(dev_priv);
ret = i915_gem_init(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 72bb5b5..61cd290 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2235,6 +2235,16 @@ struct intel_cdclk_state {
u8 voltage_level;
};
+struct intel_wopcm_info {
+ u32 size;
+};
+
+struct intel_wopcm_partition {
+ u32 guc_wopcm_offset;
+ u32 guc_wopcm_size;
+ u32 guc_wopcm_top;
+};
+
struct drm_i915_private {
struct drm_device drm;
@@ -2258,6 +2268,9 @@ struct drm_i915_private {
struct intel_huc huc;
struct intel_guc guc;
+ struct intel_wopcm_info wopcm;
+ struct intel_wopcm_partition wopcm_partition;
+
struct intel_csr csr;
struct intel_gmbus gmbus[GMBUS_NUM_PINS];
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 10affb3..7347fd7 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -312,12 +312,12 @@ __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 (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
- ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
+ ctx->ggtt_offset_bias = intel_guc_wopcm_top(dev_priv);
else
ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h
index 35cf991..d309884 100644
--- a/drivers/gpu/drm/i915/i915_guc_reg.h
+++ b/drivers/gpu/drm/i915/i915_guc_reg.h
@@ -67,17 +67,27 @@
#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)
#define HUC_FW_VERIFIED (1<<7)
/* Defines WOPCM space available to GuC firmware */
+/* default WOPCM size 1MB */
+#define WOPCM_DEFAULT_SIZE (0x1 << 20)
+/* reserved WOPCM size 16KB */
+#define WOPCM_RESERVED_SIZE (0x4000)
+/* GUC WOPCM Offset need to be 16KB aligned */
+#define WOPCM_OFFSET_ALIGNMENT (0x4000)
+/* 8KB stack reserved for GuC FW*/
+#define GUC_WOPCM_STACK_RESERVED (0x2000)
+/* 24KB WOPCM reserved for RC6 CTX on BXT */
+#define BXT_WOPCM_RC6_RESERVED (0x6000)
+
+#define GEN9_GUC_WOPCM_DELTA 4
+#define GEN9_GUC_WOPCM_OFFSET (0x24000)
+
#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.c b/drivers/gpu/drm/i915/intel_guc.c
index 9678630..0efcfb4 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -337,7 +337,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.
@@ -358,7 +358,8 @@ 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 |
+ intel_guc_wopcm_top(dev_priv));
if (ret) {
vma = ERR_PTR(ret);
goto err;
@@ -373,11 +374,42 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv)
{
- u32 wopcm_size = GUC_WOPCM_TOP;
+ struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
- /* On BXT, the top of WOPCM is reserved for RC6 context */
- if (IS_GEN9_LP(dev_priv))
- wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED;
+ GEM_BUG_ON(!wp->guc_wopcm_size);
- return wopcm_size;
+ return wp->guc_wopcm_size;
+}
+
+u32 intel_guc_wopcm_top(struct drm_i915_private *dev_priv)
+{
+ struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
+
+ GEM_BUG_ON(!dev_priv->wopcm.size);
+
+ return wp->guc_wopcm_top ? wp->guc_wopcm_top : dev_priv->wopcm.size;
+}
+
+u32 intel_guc_wopcm_offset(struct drm_i915_private *dev_priv)
+{
+ struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
+
+ GEM_BUG_ON(!wp->guc_wopcm_size);
+
+ return wp->guc_wopcm_offset;
+}
+
+/*
+ * GuC does not allow any gfx GGTT address that falls into range [0, 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.
+ */
+u32 guc_ggtt_offset(struct i915_vma *vma)
+{
+ struct drm_i915_private *dev_priv = vma->vm->i915;
+ u32 offset = i915_ggtt_offset(vma);
+
+ GEM_BUG_ON(offset < intel_guc_wopcm_top(dev_priv));
+ GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
+ return offset;
}
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 607e025..1493de0 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -100,21 +100,6 @@ static inline void intel_guc_notify(struct intel_guc *guc)
guc->notify(guc);
}
-/*
- * 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.
- */
-static inline u32 guc_ggtt_offset(struct i915_vma *vma)
-{
- u32 offset = i915_ggtt_offset(vma);
-
- GEM_BUG_ON(offset < GUC_WOPCM_TOP);
- GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
-
- return offset;
-}
void intel_guc_init_early(struct intel_guc *guc);
void intel_guc_init_send_regs(struct intel_guc *guc);
@@ -127,5 +112,8 @@ 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);
+u32 intel_guc_wopcm_top(struct drm_i915_private *dev_priv);
+u32 intel_guc_wopcm_offset(struct drm_i915_private *dev_priv);
+u32 guc_ggtt_offset(struct i915_vma *vma);
#endif
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 98d1725..a985aa5 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -202,7 +202,8 @@ void intel_huc_auth(struct intel_huc *huc)
return;
vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0,
- PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
+ PIN_OFFSET_BIAS |
+ intel_guc_wopcm_top(i915));
if (IS_ERR(vma)) {
DRM_ERROR("failed to pin huc fw object %d\n",
(int)PTR_ERR(vma));
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index aec2954..83b2516 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -86,6 +86,125 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
intel_guc_init_early(&dev_priv->guc);
}
+static u32 rc6_context_size(struct drm_i915_private *dev_priv)
+{
+ /* On BXT, the top of WOPCM is reserved for RC6 context */
+ if (IS_GEN9_LP(dev_priv))
+ return BXT_WOPCM_RC6_RESERVED;
+
+ return 0;
+}
+
+static int do_wopcm_partition(struct drm_i915_private *dev_priv,
+ u32 offset, u32 reserved_size)
+{
+ struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
+ u32 aligned_offset = ALIGN(offset, WOPCM_OFFSET_ALIGNMENT);
+
+ if (offset >= dev_priv->wopcm.size)
+ return -E2BIG;
+
+ if (reserved_size >= dev_priv->wopcm.size)
+ return -E2BIG;
+
+ if ((aligned_offset + reserved_size) >= dev_priv->wopcm.size)
+ return -E2BIG;
+
+ wp->guc_wopcm_offset = aligned_offset;
+ wp->guc_wopcm_top = dev_priv->wopcm.size - wp->guc_wopcm_offset;
+ wp->guc_wopcm_size = wp->guc_wopcm_top - reserved_size;
+
+ return 0;
+}
+
+static int intel_uc_init_wopcm_partition(struct drm_i915_private *dev_priv)
+{
+ struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
+ struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
+ struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
+ size_t huc_size, guc_size;
+ u32 offset;
+ u32 reserved;
+ u32 wopcm_base;
+ u32 delta;
+ int err;
+
+ /*Return if WOPCM partition has been initialized*/
+ if (wp->guc_wopcm_size)
+ return 0;
+
+ GEM_BUG_ON(!dev_priv->wopcm.size);
+
+ /*No need to do partition if failed to fetch both GuC and HuC FW*/
+ if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS &&
+ huc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
+ return -EIO;
+
+ huc_size = 0;
+ guc_size = 0;
+ offset = WOPCM_RESERVED_SIZE;
+ reserved = rc6_context_size(dev_priv);
+
+ if (huc_fw->fetch_status == INTEL_UC_FIRMWARE_SUCCESS)
+ huc_size = huc_fw->header_size + huc_fw->ucode_size;
+
+ err = do_wopcm_partition(dev_priv, offset + huc_size, reserved);
+ if (err) {
+ if (!huc_size)
+ goto pt_done;
+
+ /*partition failed with HuC FW, block HuC loading*/
+ huc_size = 0;
+
+ /*partition without loading HuC FW*/
+ err = do_wopcm_partition(dev_priv, offset, reserved);
+ if (err)
+ goto pt_done;
+ }
+
+ /*
+ * Check hardware restriction on Gen9
+ * GuC WOPCM size is at least 4 bytes larger than GuC WOPCM base due
+ * to hardware limitation on Gen9.
+ */
+ if (IS_GEN9(dev_priv)) {
+ wopcm_base = wp->guc_wopcm_offset + GEN9_GUC_WOPCM_OFFSET;
+ if (unlikely(wopcm_base > wp->guc_wopcm_size))
+ goto pt_done;
+
+ delta = wp->guc_wopcm_size - wopcm_base;
+ if (unlikely(delta < GEN9_GUC_WOPCM_DELTA))
+ goto pt_done;
+ }
+
+ if (guc_fw->fetch_status == INTEL_UC_FIRMWARE_SUCCESS) {
+ guc_size = guc_fw->header_size + guc_fw->ucode_size;
+ /*need 8K stack for GuC*/
+ guc_size += GUC_WOPCM_STACK_RESERVED;
+ }
+
+ if (guc_size > wp->guc_wopcm_size)
+ guc_size = 0;
+
+pt_done:
+ if (!huc_size) {
+ DRM_ERROR("HuC firmware is too large to fit in WOPCM\n");
+ intel_uc_fw_fini(huc_fw);
+ }
+
+ if (!guc_size) {
+ DRM_ERROR("GuC firmware is too large to fit in WOPCM\n");
+ intel_uc_fw_fini(guc_fw);
+ }
+
+ DRM_DEBUG_DRIVER("GuC WOPCM offset %dKB, size %dKB, top %dKB\n",
+ wp->guc_wopcm_offset >> 10,
+ wp->guc_wopcm_size >> 10,
+ wp->guc_wopcm_top >> 10);
+
+ return err;
+}
+
void intel_uc_init_fw(struct drm_i915_private *dev_priv)
{
intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
@@ -157,6 +276,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
if (!i915_modparams.enable_guc_loading)
return 0;
+ /*init WOPCM partition*/
+ ret = intel_uc_init_wopcm_partition(dev_priv);
+ if (ret)
+ goto err_wopcm;
+
guc_disable_communication(guc);
gen9_reset_guc_interrupts(dev_priv);
@@ -176,7 +300,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
/* init WOPCM */
I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(dev_priv));
I915_WRITE(DMA_GUC_WOPCM_OFFSET,
- GUC_WOPCM_OFFSET_VALUE | HUC_LOADING_AGENT_GUC);
+ intel_guc_wopcm_offset(dev_priv) | HUC_LOADING_AGENT_GUC);
/* WaEnableuKernelHeaderValidFix:skl */
/* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
@@ -249,7 +373,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
i915_guc_submission_fini(dev_priv);
err_guc:
i915_ggtt_disable_guc(dev_priv);
-
+err_wopcm:
if (i915_modparams.enable_guc_loading > 1 ||
i915_modparams.enable_guc_submission > 1) {
DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
index 973888e..aefba13 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)) {
+ if (size > dev_priv->wopcm.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,8 @@ 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 |
+ intel_guc_wopcm_top(i915));
if (IS_ERR(vma)) {
err = PTR_ERR(vma);
DRM_DEBUG_DRIVER("%s fw ggtt-pin err=%d\n",
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-11-09 22:19 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-04 0:18 [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition Jackie Li
2017-11-04 0:18 ` [PATCH 2/2] HAX enable guc submission for CI Jackie Li
2017-11-04 0:38 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: implemented dynamic WOPCM partition Patchwork
2017-11-06 8:16 ` [PATCH 1/2] " Sagar Arun Kamble
2017-11-06 20:22 ` Yaodong Li
2017-11-07 10:52 ` Joonas Lahtinen
2017-11-09 22:18 ` Yaodong Li
-- strict thread matches above, loose matches on Subject: below --
2017-11-04 0:01 Jackie Li
2017-11-04 0:16 ` Yaodong Li
2017-11-06 13:24 ` Ville Syrjälä
2017-11-06 18:20 ` Yaodong Li
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.