[PATCH v3 1/5] drm/i915/guc: Fix GuC pin bias and WOPCM initialization order

[PATCH v3 1/5] drm/i915/guc: Fix GuC pin bias and WOPCM initialization order

[Jakub Bartmiński]

It would seem that we are using uninitialized WOPCM variables when
setting the GuC pin bias. The pin bias has to be set after the WOPCM,
but before the call to i915_gem_contexts_init where the first contexts
are pinned so the safest place to set it seems to be right after
initializing the relevant variables in intel_wopcm_init.

Fixes: f7dc0157e4b5 ("drm/i915/uc: Fetch GuC/HuC firmwares from guc/huc specific init")
Signed-off-by: Jakub Bartmiński <jakub.bartminski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/intel_guc.c   | 21 ---------------------
 drivers/gpu/drm/i915/intel_wopcm.c | 19 +++++++++++++++++++
 2 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index e12bd259df17..47cacd59ac32 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -27,8 +27,6 @@
 #include "intel_guc_submission.h"
 #include "i915_drv.h"
 
-static void guc_init_ggtt_pin_bias(struct intel_guc *guc);
-
 static void gen8_guc_raise_irq(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -142,8 +140,6 @@ int intel_guc_init_misc(struct intel_guc *guc)
 	struct drm_i915_private *i915 = guc_to_i915(guc);
 	int ret;
 
-	guc_init_ggtt_pin_bias(guc);
-
 	ret = guc_init_wq(guc);
 	if (ret)
 		return ret;
@@ -611,23 +607,6 @@ int intel_guc_resume(struct intel_guc *guc)
  * actual GuC WOPCM size.
  */
 
-/**
- * guc_init_ggtt_pin_bias() - Initialize the GuC ggtt_pin_bias value.
- * @guc: intel_guc structure.
- *
- * This function will calculate and initialize the ggtt_pin_bias value based on
- * overall WOPCM size and GuC WOPCM size.
- */
-static void guc_init_ggtt_pin_bias(struct intel_guc *guc)
-{
-	struct drm_i915_private *i915 = guc_to_i915(guc);
-
-	GEM_BUG_ON(!i915->wopcm.size);
-	GEM_BUG_ON(i915->wopcm.size < i915->wopcm.guc.base);
-
-	guc->ggtt_pin_bias = i915->wopcm.size - i915->wopcm.guc.base;
-}
-
 /**
  * intel_guc_allocate_vma() - Allocate a GGTT VMA for GuC usage
  * @guc:	the guc
diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
index 74bf76f3fddc..02f602db9548 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -140,6 +140,23 @@ static inline int check_hw_restriction(struct drm_i915_private *i915,
 	return err;
 }
 
+/**
+ * wopcm_init_guc_ggtt_pin_bias() - Initialize the GuC ggtt_pin_bias value.
+ * @wopcm: pointer to intel_wopcm.
+ *
+ * This function will calculate and initialize the GuC ggtt_pin_bias value based
+ * on overall WOPCM size and GuC WOPCM size.
+ */
+static void wopcm_init_guc_ggtt_pin_bias(struct intel_wopcm *wopcm)
+{
+	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
+
+	GEM_BUG_ON(!wopcm->size);
+	GEM_BUG_ON(wopcm->size < wopcm->guc.base);
+
+	i915->guc.ggtt_pin_bias = wopcm->size - wopcm->guc.base;
+}
+
 /**
  * intel_wopcm_init() - Initialize the WOPCM structure.
  * @wopcm: pointer to intel_wopcm.
@@ -207,6 +224,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
 	wopcm->guc.base = guc_wopcm_base;
 	wopcm->guc.size = guc_wopcm_size;
 
+	wopcm_init_guc_ggtt_pin_bias(wopcm);
+
 	return 0;
 }
 
-- 
2.17.1

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

[PATCH v3 2/5] drm/i915/guc: Move the pin bias value from GuC to GGTT

[Jakub Bartmiński]

Removing the pin bias from GuC allows us to not check for GuC every time
we pin a context, which fixes the assertion error on unresolved GuC
platform default in mock contexts selftest.

With this change the intel_guc_ggtt_offset function has to know the full
declaration of the drm_i915_private structure so it can no longer be
located in the intel_guc.h header file due to include order.

v2:
This also makes it so that there's no need to set GuC variables from
within the WOPCM init function or to move the WOPCM init, while keeping
the correct initialization order. Also for mock tests the pin bias is
left at 0 and we make sure that the pin bias with GuC will not be
smaller than without GuC.

v3:
Avoid unused i915 in intel_guc_ggtt_offset if debug is disabled.

Testcase: igt/drv_selftest/mock_contexts #GuC
Signed-off-by: Jakub Bartmiński <jakub.bartminski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c   | 10 +-------
 drivers/gpu/drm/i915/i915_gem_gtt.c       | 22 ++++++++++++----
 drivers/gpu/drm/i915/i915_gem_gtt.h       |  2 ++
 drivers/gpu/drm/i915/intel_guc.c          | 31 ++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_guc.h          | 28 +-------------------
 drivers/gpu/drm/i915/intel_huc.c          |  2 +-
 drivers/gpu/drm/i915/intel_uc_fw.c        |  2 +-
 drivers/gpu/drm/i915/intel_wopcm.c        | 19 --------------
 drivers/gpu/drm/i915/intel_wopcm.h        |  8 ++++++
 drivers/gpu/drm/i915/selftests/mock_gtt.c |  2 ++
 10 files changed, 60 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index b10770cfccd2..32f96b8cd9c4 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -329,15 +329,7 @@ __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 in Non-WOPCM memory. If GuC is not
-	 * present or not in use we still need a small bias as ring wraparound
-	 * at offset 0 sometimes hangs. No idea why.
-	 */
-	if (USES_GUC(dev_priv))
-		ctx->ggtt_offset_bias = dev_priv->guc.ggtt_pin_bias;
-	else
-		ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
+	ctx->ggtt_offset_bias = dev_priv->ggtt.pin_bias;
 
 	return ctx;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index d0acef299b9c..a61344379832 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2901,7 +2901,7 @@ void i915_gem_fini_aliasing_ppgtt(struct drm_i915_private *i915)
 	ggtt->vm.vma_ops.unbind_vma = ggtt_unbind_vma;
 }
 
-int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
+int i915_gem_init_ggtt(struct drm_i915_private *i915)
 {
 	/* Let GEM Manage all of the aperture.
 	 *
@@ -2912,12 +2912,24 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
 	 * aperture.  One page should be enough to keep any prefetching inside
 	 * of the aperture.
 	 */
-	struct i915_ggtt *ggtt = &dev_priv->ggtt;
+	struct i915_ggtt *ggtt = &i915->ggtt;
 	unsigned long hole_start, hole_end;
 	struct drm_mm_node *entry;
 	int ret;
 
-	ret = intel_vgt_balloon(dev_priv);
+	/*
+	 * We need a small bias as ring wraparound at offset 0
+	 * sometimes hangs. No idea why.
+	 */
+	ggtt->pin_bias = I915_GTT_PAGE_SIZE;
+
+	/* Any objects shared with GuC can't overlap with WOPCM. */
+	if (USES_GUC(i915)) {
+		ggtt->pin_bias = max(ggtt->pin_bias,
+				     intel_wopcm_get_pin_bias(&i915->wopcm));
+	}
+
+	ret = intel_vgt_balloon(i915);
 	if (ret)
 		return ret;
 
@@ -2940,8 +2952,8 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
 	/* And finally clear the reserved guard page */
 	ggtt->vm.clear_range(&ggtt->vm, ggtt->vm.total - PAGE_SIZE, PAGE_SIZE);
 
-	if (USES_PPGTT(dev_priv) && !USES_FULL_PPGTT(dev_priv)) {
-		ret = i915_gem_init_aliasing_ppgtt(dev_priv);
+	if (USES_PPGTT(i915) && !USES_FULL_PPGTT(i915)) {
+		ret = i915_gem_init_aliasing_ppgtt(i915);
 		if (ret)
 			goto err;
 	}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 14e62651010b..71e8cf24c800 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -396,6 +396,8 @@ struct i915_ggtt {
 
 	int mtrr;
 
+	u32 pin_bias;
+
 	struct drm_mm_node error_capture;
 };
 
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 47cacd59ac32..e069573eb08c 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -607,6 +607,29 @@ int intel_guc_resume(struct intel_guc *guc)
  * actual GuC WOPCM size.
  */
 
+/**
+ * intel_guc_ggtt_offset() - Get and validate the GGTT offset of @vma
+ * @guc: intel_guc structure.
+ * @vma: i915 graphics virtual memory area.
+ *
+ * GuC does not allow any gfx GGTT address that falls into range
+ * [0, ggtt.pin_bias), which is reserved for Boot ROM, SRAM and WOPCM.
+ * Currently, in order to exclude [0, ggtt.pin_bias) address space from
+ * GGTT, all gfx objects used by GuC are allocated with intel_guc_allocate_vma()
+ * and pinned with PIN_OFFSET_BIAS along with the value of ggtt.pin_bias.
+ *
+ * Return: GGTT offset of the @vma.
+ */
+u32 intel_guc_ggtt_offset(struct intel_guc *guc, struct i915_vma *vma)
+{
+	u32 offset = i915_ggtt_offset(vma);
+
+	GEM_BUG_ON(offset < i915_vm_to_ggtt(vma->vm)->pin_bias);
+	GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
+
+	return offset;
+}
+
 /**
  * intel_guc_allocate_vma() - Allocate a GGTT VMA for GuC usage
  * @guc:	the guc
@@ -622,21 +645,21 @@ int intel_guc_resume(struct intel_guc *guc)
  */
 struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
 {
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	struct drm_i915_private *i915 = guc_to_i915(guc);
 	struct drm_i915_gem_object *obj;
 	struct i915_vma *vma;
 	int ret;
 
-	obj = i915_gem_object_create(dev_priv, size);
+	obj = i915_gem_object_create(i915, size);
 	if (IS_ERR(obj))
 		return ERR_CAST(obj);
 
-	vma = i915_vma_instance(obj, &dev_priv->ggtt.vm, NULL);
+	vma = i915_vma_instance(obj, &i915->ggtt.vm, NULL);
 	if (IS_ERR(vma))
 		goto err;
 
 	ret = i915_vma_pin(vma, 0, PAGE_SIZE,
-			   PIN_GLOBAL | PIN_OFFSET_BIAS | guc->ggtt_pin_bias);
+			   PIN_GLOBAL | PIN_OFFSET_BIAS | i915->ggtt.pin_bias);
 	if (ret) {
 		vma = ERR_PTR(ret);
 		goto err;
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 4121928a495e..b0e5c99c0338 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -49,9 +49,6 @@ struct intel_guc {
 	struct intel_guc_log log;
 	struct intel_guc_ct ct;
 
-	/* Offset where Non-WOPCM memory starts. */
-	u32 ggtt_pin_bias;
-
 	/* Log snapshot if GuC errors during load */
 	struct drm_i915_gem_object *load_err_log;
 
@@ -123,30 +120,7 @@ static inline void intel_guc_to_host_event_handler(struct intel_guc *guc)
 
 /* GuC addresses above GUC_GGTT_TOP also don't map through the GTT */
 #define GUC_GGTT_TOP	0xFEE00000
-
-/**
- * intel_guc_ggtt_offset() - Get and validate the GGTT offset of @vma
- * @guc: intel_guc structure.
- * @vma: i915 graphics virtual memory area.
- *
- * GuC does not allow any gfx GGTT address that falls into range
- * [0, GuC ggtt_pin_bias), which is reserved for Boot ROM, SRAM and WOPCM.
- * Currently, in order to exclude [0, GuC ggtt_pin_bias) address space from
- * GGTT, all gfx objects used by GuC are allocated with intel_guc_allocate_vma()
- * and pinned with PIN_OFFSET_BIAS along with the value of GuC ggtt_pin_bias.
- *
- * Return: GGTT offset of the @vma.
- */
-static inline u32 intel_guc_ggtt_offset(struct intel_guc *guc,
-					struct i915_vma *vma)
-{
-	u32 offset = i915_ggtt_offset(vma);
-
-	GEM_BUG_ON(offset < guc->ggtt_pin_bias);
-	GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
-
-	return offset;
-}
+u32 intel_guc_ggtt_offset(struct intel_guc *guc, struct i915_vma *vma);
 
 void intel_guc_init_early(struct intel_guc *guc);
 void intel_guc_init_send_regs(struct intel_guc *guc);
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index ffcad5fad6a7..37ef540dd280 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -63,7 +63,7 @@ int intel_huc_auth(struct intel_huc *huc)
 		return -ENOEXEC;
 
 	vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0,
-				       PIN_OFFSET_BIAS | guc->ggtt_pin_bias);
+				       PIN_OFFSET_BIAS | i915->ggtt.pin_bias);
 	if (IS_ERR(vma)) {
 		ret = PTR_ERR(vma);
 		DRM_ERROR("HuC: Failed to pin huc fw object %d\n", ret);
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
index 6e8e0b546743..fd496416087c 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/intel_uc_fw.c
@@ -222,7 +222,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
 		goto fail;
 	}
 
-	ggtt_pin_bias = to_i915(uc_fw->obj->base.dev)->guc.ggtt_pin_bias;
+	ggtt_pin_bias = to_i915(uc_fw->obj->base.dev)->ggtt.pin_bias;
 	vma = i915_gem_object_ggtt_pin(uc_fw->obj, NULL, 0, 0,
 				       PIN_OFFSET_BIAS | ggtt_pin_bias);
 	if (IS_ERR(vma)) {
diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
index 02f602db9548..74bf76f3fddc 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -140,23 +140,6 @@ static inline int check_hw_restriction(struct drm_i915_private *i915,
 	return err;
 }
 
-/**
- * wopcm_init_guc_ggtt_pin_bias() - Initialize the GuC ggtt_pin_bias value.
- * @wopcm: pointer to intel_wopcm.
- *
- * This function will calculate and initialize the GuC ggtt_pin_bias value based
- * on overall WOPCM size and GuC WOPCM size.
- */
-static void wopcm_init_guc_ggtt_pin_bias(struct intel_wopcm *wopcm)
-{
-	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
-
-	GEM_BUG_ON(!wopcm->size);
-	GEM_BUG_ON(wopcm->size < wopcm->guc.base);
-
-	i915->guc.ggtt_pin_bias = wopcm->size - wopcm->guc.base;
-}
-
 /**
  * intel_wopcm_init() - Initialize the WOPCM structure.
  * @wopcm: pointer to intel_wopcm.
@@ -224,8 +207,6 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
 	wopcm->guc.base = guc_wopcm_base;
 	wopcm->guc.size = guc_wopcm_size;
 
-	wopcm_init_guc_ggtt_pin_bias(wopcm);
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_wopcm.h b/drivers/gpu/drm/i915/intel_wopcm.h
index 6298910a384c..c4bb2603c788 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.h
+++ b/drivers/gpu/drm/i915/intel_wopcm.h
@@ -7,6 +7,7 @@
 #ifndef _INTEL_WOPCM_H_
 #define _INTEL_WOPCM_H_
 
+#include "i915_gem.h"
 #include <linux/types.h>
 
 /**
@@ -24,6 +25,13 @@ struct intel_wopcm {
 	} guc;
 };
 
+static inline u32 intel_wopcm_get_pin_bias(struct intel_wopcm *wopcm)
+{
+	GEM_BUG_ON(!wopcm->size);
+	GEM_BUG_ON(wopcm->size < wopcm->guc.base);
+	return wopcm->size - wopcm->guc.base;
+}
+
 void intel_wopcm_init_early(struct intel_wopcm *wopcm);
 int intel_wopcm_init(struct intel_wopcm *wopcm);
 int intel_wopcm_init_hw(struct intel_wopcm *wopcm);
diff --git a/drivers/gpu/drm/i915/selftests/mock_gtt.c b/drivers/gpu/drm/i915/selftests/mock_gtt.c
index a140ea5c3a7c..9f059b74b525 100644
--- a/drivers/gpu/drm/i915/selftests/mock_gtt.c
+++ b/drivers/gpu/drm/i915/selftests/mock_gtt.c
@@ -117,6 +117,8 @@ void mock_init_ggtt(struct drm_i915_private *i915)
 	ggtt->vm.vma_ops.set_pages   = ggtt_set_pages;
 	ggtt->vm.vma_ops.clear_pages = clear_pages;
 
+	ggtt->pin_bias = 0;
+
 	i915_address_space_init(&ggtt->vm, i915);
 }
 
-- 
2.17.1

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

[PATCH v3 3/5] drm/i915: Remove unnecessary ggtt_offset_bias from i915_gem_context

[Jakub Bartmiński]

Since ggtt_offset_bias is now stored in ggtt.pin_bias, it is duplicated
inside i915_gem_context, and can instead be accessed directly from ggtt.

v3:
Added a helper function to retrieve the ggtt.pin_bias from the vma.

Signed-off-by: Jakub Bartmiński <jakub.bartminski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c |  2 --
 drivers/gpu/drm/i915/i915_gem_context.h |  3 ---
 drivers/gpu/drm/i915/intel_lrc.c        | 14 +++++++++-----
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 32f96b8cd9c4..f15a039772db 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -329,8 +329,6 @@ __create_hw_context(struct drm_i915_private *dev_priv,
 	ctx->desc_template =
 		default_desc_template(dev_priv, dev_priv->mm.aliasing_ppgtt);
 
-	ctx->ggtt_offset_bias = dev_priv->ggtt.pin_bias;
-
 	return ctx;
 
 err_pid:
diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
index b116e4942c10..851dad6decd7 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/i915_gem_context.h
@@ -147,9 +147,6 @@ struct i915_gem_context {
 
 	struct i915_sched_attr sched;
 
-	/** ggtt_offset_bias: placement restriction for context objects */
-	u32 ggtt_offset_bias;
-
 	/** engine: per-engine logical HW state */
 	struct intel_context {
 		struct i915_gem_context *gem_context;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 35d37af0cb9a..46661c6e6d1e 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1346,6 +1346,11 @@ static void execlists_context_unpin(struct intel_context *ce)
 	i915_gem_context_put(ce->gem_context);
 }
 
+static inline u32 i915_ggtt_pin_bias(struct i915_vma *vma)
+{
+	return i915_vm_to_ggtt(vma->vm)->pin_bias;
+}
+
 static int __context_pin(struct i915_gem_context *ctx, struct i915_vma *vma)
 {
 	unsigned int flags;
@@ -1362,10 +1367,8 @@ static int __context_pin(struct i915_gem_context *ctx, struct i915_vma *vma)
 			return err;
 	}
 
-	flags = PIN_GLOBAL | PIN_HIGH;
-	if (ctx->ggtt_offset_bias)
-		flags |= PIN_OFFSET_BIAS | ctx->ggtt_offset_bias;
-
+	flags = PIN_GLOBAL | PIN_HIGH |
+		PIN_OFFSET_BIAS | i915_ggtt_pin_bias(vma);
 	return i915_vma_pin(vma, 0, GEN8_LR_CONTEXT_ALIGN, flags);
 }
 
@@ -1392,7 +1395,8 @@ __execlists_context_pin(struct intel_engine_cs *engine,
 		goto unpin_vma;
 	}
 
-	ret = intel_ring_pin(ce->ring, ctx->i915, ctx->ggtt_offset_bias);
+	ret = intel_ring_pin(ce->ring, ctx->i915,
+			     i915_ggtt_pin_bias(ce->ring->vma));
 	if (ret)
 		goto unpin_map;
 
-- 
2.17.1

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

[PATCH v3 4/5] drm/i915: Add a fault injection point after WOPCM init

[Jakub Bartmiński]

Signed-off-by: Jakub Bartmiński <jakub.bartminski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ed2be33ec58a..dd170a293d05 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5475,6 +5475,11 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto err_uc_misc;
 
+	if (i915_inject_load_failure()) {
+		ret = -E2BIG;
+		goto err_uc_misc;
+	}
+
 	/* This is just a security blanket to placate dragons.
 	 * On some systems, we very sporadically observe that the first TLBs
 	 * used by the CS may be stale, despite us poking the TLB reset. If
-- 
2.17.1

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

[PATCH v3 5/5] HAX enable GuC for CI

[Jakub Bartmiński]

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_params.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

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

Re: [PATCH v3 3/5] drm/i915: Remove unnecessary ggtt_offset_bias from i915_gem_context

[Chris Wilson]

Quoting Jakub Bartmiński (2018-07-19 09:35:40)
> +static inline u32 i915_ggtt_pin_bias(struct i915_vma *vma)
> +{
> +       return i915_vm_to_ggtt(vma->vm)->pin_bias;

The hint was to do this earlier in the series to avoid pulling it randomly
from i915->ggtt. If we allocate the vma, then we should have the
vm/i915_ggtt already to hand. If not, pulling it from vma->vm looks more
consistent than delving into a seemingly unrelated address space.

>  static int __context_pin(struct i915_gem_context *ctx, struct i915_vma *vma)
>  {
>         unsigned int flags;
> @@ -1362,10 +1367,8 @@ static int __context_pin(struct i915_gem_context *ctx, struct i915_vma *vma)
>                         return err;
>         }
>  
> -       flags = PIN_GLOBAL | PIN_HIGH;
> -       if (ctx->ggtt_offset_bias)
> -               flags |= PIN_OFFSET_BIAS | ctx->ggtt_offset_bias;
> -
> +       flags = PIN_GLOBAL | PIN_HIGH |
> +               PIN_OFFSET_BIAS | i915_ggtt_pin_bias(vma);

Looks messy :(

>         return i915_vma_pin(vma, 0, GEN8_LR_CONTEXT_ALIGN, flags);
>  }
>  
> @@ -1392,7 +1395,8 @@ __execlists_context_pin(struct intel_engine_cs *engine,
>                 goto unpin_vma;
>         }
>  
> -       ret = intel_ring_pin(ce->ring, ctx->i915, ctx->ggtt_offset_bias);
> +       ret = intel_ring_pin(ce->ring, ctx->i915,
> +                            i915_ggtt_pin_bias(ce->ring->vma));

This indicates that intel_ring_pin() now knows the pin_bias (as it
stored on the i915_ggtt and not on the context) and so no longer needs
it pass it from the context.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/5] drm/i915/guc: Move the pin bias value from GuC to GGTT

[Chris Wilson]

Quoting Jakub Bartmiński (2018-07-19 09:35:39)
> Removing the pin bias from GuC allows us to not check for GuC every time
> we pin a context, which fixes the assertion error on unresolved GuC
> platform default in mock contexts selftest.
> 
> With this change the intel_guc_ggtt_offset function has to know the full
> declaration of the drm_i915_private structure so it can no longer be
> located in the intel_guc.h header file due to include order.
> 
> v2:
> This also makes it so that there's no need to set GuC variables from
> within the WOPCM init function or to move the WOPCM init, while keeping
> the correct initialization order. Also for mock tests the pin bias is
> left at 0 and we make sure that the pin bias with GuC will not be
> smaller than without GuC.
> 
> v3:
> Avoid unused i915 in intel_guc_ggtt_offset if debug is disabled.
> 
> Testcase: igt/drv_selftest/mock_contexts #GuC
> Signed-off-by: Jakub Bartmiński <jakub.bartminski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c   | 10 +-------
>  drivers/gpu/drm/i915/i915_gem_gtt.c       | 22 ++++++++++++----
>  drivers/gpu/drm/i915/i915_gem_gtt.h       |  2 ++
>  drivers/gpu/drm/i915/intel_guc.c          | 31 ++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_guc.h          | 28 +-------------------
>  drivers/gpu/drm/i915/intel_huc.c          |  2 +-
>  drivers/gpu/drm/i915/intel_uc_fw.c        |  2 +-
>  drivers/gpu/drm/i915/intel_wopcm.c        | 19 --------------
>  drivers/gpu/drm/i915/intel_wopcm.h        |  8 ++++++
>  drivers/gpu/drm/i915/selftests/mock_gtt.c |  2 ++
>  10 files changed, 60 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index b10770cfccd2..32f96b8cd9c4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -329,15 +329,7 @@ __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 in Non-WOPCM memory. If GuC is not
> -        * present or not in use we still need a small bias as ring wraparound
> -        * at offset 0 sometimes hangs. No idea why.
> -        */
> -       if (USES_GUC(dev_priv))
> -               ctx->ggtt_offset_bias = dev_priv->guc.ggtt_pin_bias;
> -       else
> -               ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
> +       ctx->ggtt_offset_bias = dev_priv->ggtt.pin_bias;
>  
>         return ctx;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index d0acef299b9c..a61344379832 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2901,7 +2901,7 @@ void i915_gem_fini_aliasing_ppgtt(struct drm_i915_private *i915)
>         ggtt->vm.vma_ops.unbind_vma = ggtt_unbind_vma;
>  }
>  
> -int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
> +int i915_gem_init_ggtt(struct drm_i915_private *i915)
>  {
>         /* Let GEM Manage all of the aperture.
>          *
> @@ -2912,12 +2912,24 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
>          * aperture.  One page should be enough to keep any prefetching inside
>          * of the aperture.
>          */
> -       struct i915_ggtt *ggtt = &dev_priv->ggtt;
> +       struct i915_ggtt *ggtt = &i915->ggtt;
>         unsigned long hole_start, hole_end;
>         struct drm_mm_node *entry;
>         int ret;
>  
> -       ret = intel_vgt_balloon(dev_priv);
> +       /*
> +        * We need a small bias as ring wraparound at offset 0
> +        * sometimes hangs. No idea why.
> +        */
> +       ggtt->pin_bias = I915_GTT_PAGE_SIZE;
> +
> +       /* Any objects shared with GuC can't overlap with WOPCM. */
> +       if (USES_GUC(i915)) {
> +               ggtt->pin_bias = max(ggtt->pin_bias,
> +                                    intel_wopcm_get_pin_bias(&i915->wopcm));
> +       }

That is defeating the purpose of the series. We don't want to make these
decisions here, we just want to ask if intel_wopcm requires it. i.e., at
this point we only know that wopcm may reserve some of the Global GTT
for itself, we don't need to complicate that any further here.

USES_GUC() == modparam, ergo it should never be called in live code.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 4/5] drm/i915: Add a fault injection point after WOPCM init

[Chris Wilson]

Quoting Jakub Bartmiński (2018-07-19 09:35:41)
> Signed-off-by: Jakub Bartmiński <jakub.bartminski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ed2be33ec58a..dd170a293d05 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5475,6 +5475,11 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
>         if (ret)
>                 goto err_uc_misc;
>  
> +       if (i915_inject_load_failure()) {
> +               ret = -E2BIG;
> +               goto err_uc_misc;
> +       }

Push it down to the callee. I want us to exercise what happens when that
fails. Otherwise we just have static fault injection and never need to
worry about the order here.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.SPARSE: warning for series starting with [v3,1/5] drm/i915/guc: Fix GuC pin bias and WOPCM initialization order

[Patchwork]

== Series Details ==

Series: series starting with [v3,1/5] drm/i915/guc: Fix GuC pin bias and WOPCM initialization order
URL   : https://patchwork.freedesktop.org/series/46843/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915/guc: Fix GuC pin bias and WOPCM initialization order
Okay!

Commit: drm/i915/guc: Move the pin bias value from GuC to GGTT
+drivers/gpu/drm/i915/i915_gem_gtt.c:2948:34: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/i915_gem_gtt.c:2948:34: warning: expression using sizeof(void)

Commit: drm/i915: Remove unnecessary ggtt_offset_bias from i915_gem_context
Okay!

Commit: drm/i915: Add a fault injection point after WOPCM init
Okay!

Commit: HAX enable GuC for CI
Okay!

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

✗ Fi.CI.BAT: failure for series starting with [v3,1/5] drm/i915/guc: Fix GuC pin bias and WOPCM initialization order

[Patchwork]

== Series Details ==

Series: series starting with [v3,1/5] drm/i915/guc: Fix GuC pin bias and WOPCM initialization order
URL   : https://patchwork.freedesktop.org/series/46843/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4510 -> Patchwork_9717 =

== Summary - FAILURE ==

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

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

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@drv_selftest@live_guc:
      fi-kbl-7567u:       PASS -> DMESG-WARN
      fi-skl-gvtdvm:      PASS -> DMESG-WARN
      fi-bxt-dsi:         NOTRUN -> DMESG-WARN
      fi-whl-u:           PASS -> DMESG-WARN
      fi-kbl-7560u:       PASS -> DMESG-WARN
      fi-kbl-r:           PASS -> DMESG-WARN
      fi-kbl-x1275:       PASS -> DMESG-WARN
      fi-bxt-j4205:       PASS -> DMESG-WARN
      fi-cfl-s3:          PASS -> DMESG-WARN
      {fi-cfl-8109u}:     PASS -> DMESG-WARN
      fi-kbl-7500u:       PASS -> DMESG-WARN
      fi-cfl-8700k:       PASS -> DMESG-WARN

    igt@drv_selftest@live_hangcheck:
      fi-skl-gvtdvm:      PASS -> DMESG-FAIL
      fi-cfl-s3:          PASS -> DMESG-FAIL
      fi-bxt-dsi:         NOTRUN -> DMESG-FAIL
      fi-bxt-j4205:       PASS -> DMESG-FAIL
      {fi-skl-iommu}:     PASS -> DMESG-FAIL +1

    
    ==== Warnings ====

    igt@drv_selftest@live_guc:
      {fi-skl-iommu}:     PASS -> SKIP +1

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_guc:
      fi-skl-6600u:       PASS -> DMESG-WARN (fdo#107175)
      fi-skl-6260u:       PASS -> DMESG-WARN (fdo#107175)
      fi-skl-6700k2:      PASS -> DMESG-WARN (fdo#107175)
      fi-skl-6770hq:      PASS -> DMESG-WARN (fdo#107175)
      fi-skl-6700hq:      PASS -> DMESG-WARN (fdo#107175)

    igt@drv_selftest@live_hangcheck:
      fi-skl-6770hq:      PASS -> DMESG-FAIL (fdo#107174)

    
    ==== Possible fixes ====

    igt@debugfs_test@read_all_entries:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    igt@drv_selftest@live_workarounds:
      fi-bdw-5557u:       DMESG-FAIL -> PASS
      fi-skl-6700hq:      DMESG-FAIL -> PASS

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

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

  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
  fdo#107175 https://bugs.freedesktop.org/show_bug.cgi?id=107175


== Participating hosts (47 -> 42) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4510 -> Patchwork_9717

  CI_DRM_4510: 31b9b285a571eda0db7a8fd5206d48fcdd0190b3 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4568: 86f7b724ef18986bc58d35558d22e1ed3f8df4f9 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9717: 2306b20a6dea3a4f224d21c00f312f89d216e426 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

2306b20a6dea HAX enable GuC for CI
0fa25edf61e9 drm/i915: Add a fault injection point after WOPCM init
eb5bfd46972b drm/i915: Remove unnecessary ggtt_offset_bias from i915_gem_context
e9dda8797e50 drm/i915/guc: Move the pin bias value from GuC to GGTT
5b24a0eaa5b5 drm/i915/guc: Fix GuC pin bias and WOPCM initialization order

== Logs ==

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