All of lore.kernel.org
 help / color / mirror / Atom feed
* [Intel-gfx] [PATCH v2 00/10] Commit early to GuC
@ 2020-02-03 23:28 Daniele Ceraolo Spurio
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 01/10] drm/i915/debugfs: Pass guc_log struct to i915_guc_log_info Daniele Ceraolo Spurio
                   ` (12 more replies)
  0 siblings, 13 replies; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2020-02-03 23:28 UTC (permalink / raw)
  To: intel-gfx

Main difference from v1 is that I've dropped the patch to start the
re-org of guc submission setup (I plan to re-send that as part of [1],
after Chris' intel_lrc.c split patches land) and added a patch to be
less aggressive in cleaning up the firmware objects.

Some chunks have also be split to their own patches, as requested by
Michal, to better isolate the changes.

[1] https://patchwork.freedesktop.org/series/72562/

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>

Daniele Ceraolo Spurio (10):
  drm/i915/debugfs: Pass guc_log struct to i915_guc_log_info
  drm/i915/guc: Kill USES_GUC macro
  drm/i915/guc: Kill USES_GUC_SUBMISSION macro
  drm/i915/uc: Update the FW status on injected fetch error
  drm/i915/uc: autogenerate uC checker functions
  drm/i915/uc: Improve tracking of uC init status
  drm/i915/guc: Apply new uC status tracking to GuC submission as well
  drm/i915/uc: Abort early on uc_init failure
  drm/i915/uc: consolidate firmware cleanup
  HAX: drm/i915: default to enable_guc=2

 drivers/gpu/drm/i915/gem/i915_gem_context.c   |  2 +-
 drivers/gpu/drm/i915/gt/intel_ggtt.c          |  2 +-
 drivers/gpu/drm/i915/gt/intel_gt.c            |  4 +-
 drivers/gpu/drm/i915/gt/selftest_hangcheck.c  |  2 +-
 drivers/gpu/drm/i915/gt/selftest_lrc.c        | 12 ++--
 drivers/gpu/drm/i915/gt/selftest_reset.c      |  2 +-
 drivers/gpu/drm/i915/gt/uc/intel_guc.c        | 24 ++++----
 drivers/gpu/drm/i915/gt/uc/intel_guc.h        | 25 ++++++--
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  9 +--
 drivers/gpu/drm/i915/gt/uc/intel_huc.c        |  5 +-
 drivers/gpu/drm/i915/gt/uc/intel_huc.h        |  8 ++-
 drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c     |  2 +-
 drivers/gpu/drm/i915/gt/uc/intel_uc.c         | 61 +++++++++++--------
 drivers/gpu/drm/i915/gt/uc/intel_uc.h         | 61 +++++++++++--------
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c      |  9 ++-
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h      | 18 +++++-
 drivers/gpu/drm/i915/gvt/scheduler.c          |  3 +-
 drivers/gpu/drm/i915/i915_debugfs.c           | 25 ++++----
 drivers/gpu/drm/i915/i915_drv.h               | 10 ---
 drivers/gpu/drm/i915/i915_params.h            |  2 +-
 drivers/gpu/drm/i915/intel_gvt.c              |  2 +-
 21 files changed, 169 insertions(+), 119 deletions(-)

-- 
2.24.1

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

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

* [Intel-gfx] [PATCH v2 01/10] drm/i915/debugfs: Pass guc_log struct to i915_guc_log_info
  2020-02-03 23:28 [Intel-gfx] [PATCH v2 00/10] Commit early to GuC Daniele Ceraolo Spurio
@ 2020-02-03 23:28 ` Daniele Ceraolo Spurio
  2020-02-04 17:05   ` Michal Wajdeczko
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 02/10] drm/i915/guc: Kill USES_GUC macro Daniele Ceraolo Spurio
                   ` (11 subsequent siblings)
  12 siblings, 1 reply; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2020-02-03 23:28 UTC (permalink / raw)
  To: intel-gfx

The log struct is the only thing the function needs (apart from
the seq_file), so we can pass just that instead of the whole dev_priv.

v2: Split this change to its own patch (Michal)

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index e75e8212f03b..7264c0ff766c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1753,10 +1753,8 @@ stringify_guc_log_type(enum guc_log_buffer_type type)
 	return "";
 }
 
-static void i915_guc_log_info(struct seq_file *m,
-			      struct drm_i915_private *dev_priv)
+static void i915_guc_log_info(struct seq_file *m, struct intel_guc_log *log)
 {
-	struct intel_guc_log *log = &dev_priv->gt.uc.guc.log;
 	enum guc_log_buffer_type type;
 
 	if (!intel_guc_log_relay_created(log)) {
@@ -1784,7 +1782,7 @@ static int i915_guc_info(struct seq_file *m, void *data)
 	if (!USES_GUC(dev_priv))
 		return -ENODEV;
 
-	i915_guc_log_info(m, dev_priv);
+	i915_guc_log_info(m, &dev_priv->gt.uc.guc.log);
 
 	/* Add more as required ... */
 
-- 
2.24.1

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

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

* [Intel-gfx] [PATCH v2 02/10] drm/i915/guc: Kill USES_GUC macro
  2020-02-03 23:28 [Intel-gfx] [PATCH v2 00/10] Commit early to GuC Daniele Ceraolo Spurio
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 01/10] drm/i915/debugfs: Pass guc_log struct to i915_guc_log_info Daniele Ceraolo Spurio
@ 2020-02-03 23:28 ` Daniele Ceraolo Spurio
  2020-02-04 17:18   ` Michal Wajdeczko
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 03/10] drm/i915/guc: Kill USES_GUC_SUBMISSION macro Daniele Ceraolo Spurio
                   ` (10 subsequent siblings)
  12 siblings, 1 reply; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2020-02-03 23:28 UTC (permalink / raw)
  To: intel-gfx

use intel_uc_uses_guc() directly instead, to be consistent in the way we
check what we want to do with the GuC.

v2: split guc_log_info changes to their own patch (Michal)

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_ggtt.c |  2 +-
 drivers/gpu/drm/i915/i915_debugfs.c  | 15 +++++++++------
 drivers/gpu/drm/i915/i915_drv.h      |  1 -
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index 7dae91e0d002..048240b19772 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -427,7 +427,7 @@ static int ggtt_reserve_guc_top(struct i915_ggtt *ggtt)
 	u64 size;
 	int ret;
 
-	if (!USES_GUC(ggtt->vm.i915))
+	if (!intel_uc_uses_guc(&ggtt->vm.gt->uc))
 		return 0;
 
 	GEM_BUG_ON(ggtt->vm.total <= GUC_GGTT_TOP);
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 7264c0ff766c..bfdcb657780b 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1778,11 +1778,12 @@ static void i915_guc_log_info(struct seq_file *m, struct intel_guc_log *log)
 static int i915_guc_info(struct seq_file *m, void *data)
 {
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
+	struct intel_uc *uc = &dev_priv->gt.uc;
 
-	if (!USES_GUC(dev_priv))
+	if (!intel_uc_uses_guc(uc))
 		return -ENODEV;
 
-	i915_guc_log_info(m, &dev_priv->gt.uc.guc.log);
+	i915_guc_log_info(m, &uc->guc.log);
 
 	/* Add more as required ... */
 
@@ -1883,11 +1884,12 @@ static int i915_guc_log_dump(struct seq_file *m, void *data)
 static int i915_guc_log_level_get(void *data, u64 *val)
 {
 	struct drm_i915_private *dev_priv = data;
+	struct intel_uc *uc = &dev_priv->gt.uc;
 
-	if (!USES_GUC(dev_priv))
+	if (!intel_uc_uses_guc(uc))
 		return -ENODEV;
 
-	*val = intel_guc_log_get_level(&dev_priv->gt.uc.guc.log);
+	*val = intel_guc_log_get_level(&uc->guc.log);
 
 	return 0;
 }
@@ -1895,11 +1897,12 @@ static int i915_guc_log_level_get(void *data, u64 *val)
 static int i915_guc_log_level_set(void *data, u64 val)
 {
 	struct drm_i915_private *dev_priv = data;
+	struct intel_uc *uc = &dev_priv->gt.uc;
 
-	if (!USES_GUC(dev_priv))
+	if (!intel_uc_uses_guc(uc))
 		return -ENODEV;
 
-	return intel_guc_log_set_level(&dev_priv->gt.uc.guc.log, val);
+	return intel_guc_log_set_level(&uc->guc.log, val);
 }
 
 DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_level_fops,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a71ff233cc55..a8e1ec7d571f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1701,7 +1701,6 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
 #define HAS_GT_UC(dev_priv)	(INTEL_INFO(dev_priv)->has_gt_uc)
 
 /* Having GuC is not the same as using GuC */
-#define USES_GUC(dev_priv)		intel_uc_uses_guc(&(dev_priv)->gt.uc)
 #define USES_GUC_SUBMISSION(dev_priv)	intel_uc_uses_guc_submission(&(dev_priv)->gt.uc)
 
 #define HAS_POOLED_EU(dev_priv)	(INTEL_INFO(dev_priv)->has_pooled_eu)
-- 
2.24.1

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

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

* [Intel-gfx] [PATCH v2 03/10] drm/i915/guc: Kill USES_GUC_SUBMISSION macro
  2020-02-03 23:28 [Intel-gfx] [PATCH v2 00/10] Commit early to GuC Daniele Ceraolo Spurio
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 01/10] drm/i915/debugfs: Pass guc_log struct to i915_guc_log_info Daniele Ceraolo Spurio
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 02/10] drm/i915/guc: Kill USES_GUC macro Daniele Ceraolo Spurio
@ 2020-02-03 23:28 ` Daniele Ceraolo Spurio
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 04/10] drm/i915/uc: Update the FW status on injected fetch error Daniele Ceraolo Spurio
                   ` (9 subsequent siblings)
  12 siblings, 0 replies; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2020-02-03 23:28 UTC (permalink / raw)
  To: intel-gfx

use intel_uc_uses_guc_submission() directly instead, to be consistent in
the way we check what we want to do with the GuC.

v2: do not go through ctx->vm->gt, use i915->gt instead

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> #v1
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c  |  2 +-
 drivers/gpu/drm/i915/gt/selftest_hangcheck.c |  2 +-
 drivers/gpu/drm/i915/gt/selftest_lrc.c       | 12 ++++++------
 drivers/gpu/drm/i915/gt/selftest_reset.c     |  2 +-
 drivers/gpu/drm/i915/gvt/scheduler.c         |  3 ++-
 drivers/gpu/drm/i915/i915_debugfs.c          |  6 +++---
 drivers/gpu/drm/i915/i915_drv.h              |  3 ---
 drivers/gpu/drm/i915/intel_gvt.c             |  2 +-
 8 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index fad62d768f08..19b0204c6770 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1442,7 +1442,7 @@ set_engines__load_balance(struct i915_user_extension __user *base, void *data)
 	if (!HAS_EXECLISTS(i915))
 		return -ENODEV;
 
-	if (USES_GUC_SUBMISSION(i915))
+	if (intel_uc_uses_guc_submission(&i915->gt.uc))
 		return -ENODEV; /* not implement yet */
 
 	if (get_user(idx, &ext->engine_index))
diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
index 3e5e6c86e843..c3514ec7b8db 100644
--- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
+++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
@@ -1640,7 +1640,7 @@ static int igt_reset_engines_atomic(void *arg)
 	if (!intel_has_reset_engine(gt))
 		return 0;
 
-	if (USES_GUC_SUBMISSION(gt->i915))
+	if (intel_uc_uses_guc_submission(&gt->uc))
 		return 0;
 
 	igt_global_reset_lock(gt);
diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c
index 79b9f7d092e4..0d18d30630fd 100644
--- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
+++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
@@ -1808,7 +1808,7 @@ static int live_suppress_self_preempt(void *arg)
 	if (!HAS_LOGICAL_RING_PREEMPTION(gt->i915))
 		return 0;
 
-	if (USES_GUC_SUBMISSION(gt->i915))
+	if (intel_uc_uses_guc_submission(&gt->uc))
 		return 0; /* presume black blox */
 
 	if (intel_vgpu_active(gt->i915))
@@ -3044,7 +3044,7 @@ static int live_virtual_engine(void *arg)
 	unsigned int class, inst;
 	int err;
 
-	if (USES_GUC_SUBMISSION(gt->i915))
+	if (intel_uc_uses_guc_submission(&gt->uc))
 		return 0;
 
 	for_each_engine(engine, gt, id) {
@@ -3177,7 +3177,7 @@ static int live_virtual_mask(void *arg)
 	unsigned int class, inst;
 	int err;
 
-	if (USES_GUC_SUBMISSION(gt->i915))
+	if (intel_uc_uses_guc_submission(&gt->uc))
 		return 0;
 
 	for (class = 0; class <= MAX_ENGINE_CLASS; class++) {
@@ -3319,7 +3319,7 @@ static int live_virtual_preserved(void *arg)
 	 * are preserved.
 	 */
 
-	if (USES_GUC_SUBMISSION(gt->i915))
+	if (intel_uc_uses_guc_submission(&gt->uc))
 		return 0;
 
 	/* As we use CS_GPR we cannot run before they existed on all engines. */
@@ -3543,7 +3543,7 @@ static int live_virtual_bond(void *arg)
 	unsigned int class, inst;
 	int err;
 
-	if (USES_GUC_SUBMISSION(gt->i915))
+	if (intel_uc_uses_guc_submission(&gt->uc))
 		return 0;
 
 	for (class = 0; class <= MAX_ENGINE_CLASS; class++) {
@@ -3704,7 +3704,7 @@ static int live_virtual_reset(void *arg)
 	 * forgotten.
 	 */
 
-	if (USES_GUC_SUBMISSION(gt->i915))
+	if (intel_uc_uses_guc_submission(&gt->uc))
 		return 0;
 
 	if (!intel_has_reset_engine(gt))
diff --git a/drivers/gpu/drm/i915/gt/selftest_reset.c b/drivers/gpu/drm/i915/gt/selftest_reset.c
index 6ad6aca315f6..35406ecdf0b2 100644
--- a/drivers/gpu/drm/i915/gt/selftest_reset.c
+++ b/drivers/gpu/drm/i915/gt/selftest_reset.c
@@ -115,7 +115,7 @@ static int igt_atomic_engine_reset(void *arg)
 	if (!intel_has_reset_engine(gt))
 		return 0;
 
-	if (USES_GUC_SUBMISSION(gt->i915))
+	if (intel_uc_uses_guc_submission(&gt->uc))
 		return 0;
 
 	intel_gt_pm_get(gt);
diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
index 685d1e04a5ff..5fe00ee6bd1b 100644
--- a/drivers/gpu/drm/i915/gvt/scheduler.c
+++ b/drivers/gpu/drm/i915/gvt/scheduler.c
@@ -1246,7 +1246,8 @@ int intel_vgpu_setup_submission(struct intel_vgpu *vgpu)
 		ce->vm = i915_vm_get(&ppgtt->vm);
 		intel_context_set_single_submission(ce);
 
-		if (!USES_GUC_SUBMISSION(i915)) { /* Max ring buffer size */
+		/* Max ring buffer size */
+		if (!intel_uc_uses_guc_submission(&engine->gt->uc)) {
 			const unsigned int ring_size = 512 * SZ_4K;
 
 			ce->ring = __intel_context_ring_size(ring_size);
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index bfdcb657780b..01307a67dc91 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1793,11 +1793,11 @@ static int i915_guc_info(struct seq_file *m, void *data)
 static int i915_guc_stage_pool(struct seq_file *m, void *data)
 {
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
-	const struct intel_guc *guc = &dev_priv->gt.uc.guc;
-	struct guc_stage_desc *desc = guc->stage_desc_pool_vaddr;
+	struct intel_uc *uc = &dev_priv->gt.uc;
+	struct guc_stage_desc *desc = uc->guc.stage_desc_pool_vaddr;
 	int index;
 
-	if (!USES_GUC_SUBMISSION(dev_priv))
+	if (!intel_uc_uses_guc_submission(uc))
 		return -ENODEV;
 
 	for (index = 0; index < GUC_MAX_STAGE_DESCRIPTORS; index++, desc++) {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a8e1ec7d571f..0a45240af38d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1700,9 +1700,6 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
 
 #define HAS_GT_UC(dev_priv)	(INTEL_INFO(dev_priv)->has_gt_uc)
 
-/* Having GuC is not the same as using GuC */
-#define USES_GUC_SUBMISSION(dev_priv)	intel_uc_uses_guc_submission(&(dev_priv)->gt.uc)
-
 #define HAS_POOLED_EU(dev_priv)	(INTEL_INFO(dev_priv)->has_pooled_eu)
 
 #define HAS_GLOBAL_MOCS_REGISTERS(dev_priv)	(INTEL_INFO(dev_priv)->has_global_mocs)
diff --git a/drivers/gpu/drm/i915/intel_gvt.c b/drivers/gpu/drm/i915/intel_gvt.c
index 38ebd5562c7c..5c189bc31da5 100644
--- a/drivers/gpu/drm/i915/intel_gvt.c
+++ b/drivers/gpu/drm/i915/intel_gvt.c
@@ -105,7 +105,7 @@ int intel_gvt_init(struct drm_i915_private *dev_priv)
 		return 0;
 	}
 
-	if (USES_GUC_SUBMISSION(dev_priv)) {
+	if (intel_uc_uses_guc_submission(&dev_priv->gt.uc)) {
 		drm_err(&dev_priv->drm,
 			"i915 GVT-g loading failed due to Graphics virtualization is not yet supported with GuC submission\n");
 		return -EIO;
-- 
2.24.1

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

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

* [Intel-gfx] [PATCH v2 04/10] drm/i915/uc: Update the FW status on injected fetch error
  2020-02-03 23:28 [Intel-gfx] [PATCH v2 00/10] Commit early to GuC Daniele Ceraolo Spurio
                   ` (2 preceding siblings ...)
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 03/10] drm/i915/guc: Kill USES_GUC_SUBMISSION macro Daniele Ceraolo Spurio
@ 2020-02-03 23:28 ` Daniele Ceraolo Spurio
  2020-02-04 17:20   ` Michal Wajdeczko
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 05/10] drm/i915/uc: autogenerate uC checker functions Daniele Ceraolo Spurio
                   ` (8 subsequent siblings)
  12 siblings, 1 reply; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2020-02-03 23:28 UTC (permalink / raw)
  To: intel-gfx

In a follow up patch we will rely on the fact that the status always
moves away from "SELECTED" after the fetch is attempted to decide what
to do with the GuC.

v2: Split this change to its own patch (Michal)

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index 8ee0a0c7f447..c9c094a73399 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -279,7 +279,7 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
 
 	err = i915_inject_probe_error(i915, -ENXIO);
 	if (err)
-		return err;
+		goto fail;
 
 	__force_fw_fetch_failures(uc_fw, -EINVAL);
 	__force_fw_fetch_failures(uc_fw, -ESTALE);
-- 
2.24.1

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

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

* [Intel-gfx] [PATCH v2 05/10] drm/i915/uc: autogenerate uC checker functions
  2020-02-03 23:28 [Intel-gfx] [PATCH v2 00/10] Commit early to GuC Daniele Ceraolo Spurio
                   ` (3 preceding siblings ...)
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 04/10] drm/i915/uc: Update the FW status on injected fetch error Daniele Ceraolo Spurio
@ 2020-02-03 23:28 ` Daniele Ceraolo Spurio
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 06/10] drm/i915/uc: Improve tracking of uC init status Daniele Ceraolo Spurio
                   ` (7 subsequent siblings)
  12 siblings, 0 replies; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2020-02-03 23:28 UTC (permalink / raw)
  To: intel-gfx

We want to map uC-level checks to GuC/HuC-level ones. The mapping from
the uC state to the GuC/HuC one follows the same pattern for all the
functions:

 uc_xxx_guc() -> guc_is_yyy()

So we can easily use a macro to autogenerate the functions via macros by
passing in the 2 mapped states.

v2: Split this change to its own patch (Michal)

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_uc.h | 30 ++++++++++++---------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
index 49c913524686..2ce993ceb60a 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
@@ -40,15 +40,21 @@ void intel_uc_runtime_suspend(struct intel_uc *uc);
 int intel_uc_resume(struct intel_uc *uc);
 int intel_uc_runtime_resume(struct intel_uc *uc);
 
-static inline bool intel_uc_supports_guc(struct intel_uc *uc)
-{
-	return intel_guc_is_supported(&uc->guc);
+#define __uc_state_checker(x, state, required) \
+static inline bool intel_uc_##state##_##x(struct intel_uc *uc) \
+{ \
+	return intel_##x##_is_##required(&uc->x); \
 }
 
-static inline bool intel_uc_uses_guc(struct intel_uc *uc)
-{
-	return intel_guc_is_enabled(&uc->guc);
-}
+#define uc_state_checkers(x) \
+__uc_state_checker(x, supports, supported) \
+__uc_state_checker(x, uses, enabled)
+
+uc_state_checkers(guc);
+uc_state_checkers(huc);
+
+#undef uc_state_checkers
+#undef __uc_state_checker
 
 static inline bool intel_uc_supports_guc_submission(struct intel_uc *uc)
 {
@@ -60,16 +66,6 @@ static inline bool intel_uc_uses_guc_submission(struct intel_uc *uc)
 	return intel_guc_is_submission_supported(&uc->guc);
 }
 
-static inline bool intel_uc_supports_huc(struct intel_uc *uc)
-{
-	return intel_uc_supports_guc(uc);
-}
-
-static inline bool intel_uc_uses_huc(struct intel_uc *uc)
-{
-	return intel_huc_is_enabled(&uc->huc);
-}
-
 #define intel_uc_ops_function(_NAME, _OPS, _TYPE, _RET) \
 static inline _TYPE intel_uc_##_NAME(struct intel_uc *uc) \
 { \
-- 
2.24.1

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

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

* [Intel-gfx] [PATCH v2 06/10] drm/i915/uc: Improve tracking of uC init status
  2020-02-03 23:28 [Intel-gfx] [PATCH v2 00/10] Commit early to GuC Daniele Ceraolo Spurio
                   ` (4 preceding siblings ...)
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 05/10] drm/i915/uc: autogenerate uC checker functions Daniele Ceraolo Spurio
@ 2020-02-03 23:28 ` Daniele Ceraolo Spurio
  2020-02-04 17:54   ` Michal Wajdeczko
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 07/10] drm/i915/guc: Apply new uC status tracking to GuC submission as well Daniele Ceraolo Spurio
                   ` (6 subsequent siblings)
  12 siblings, 1 reply; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2020-02-03 23:28 UTC (permalink / raw)
  To: intel-gfx

To be able to setup GuC submission functions during engine init we need
to commit to using GuC as soon as possible.
Currently, the only thing that can stop us from using the
microcontrollers once we've fetched the blobs is a fundamental
error (e.g. OOM); given that if we hit such an error we can't really
fall-back to anything, we can "officialize" the FW fetching completion
as the moment at which we're committing to using GuC.

To better differentiate this case, the uses_guc check, which indicates
that GuC is supported and was selected in modparam, is renamed to
wants_guc and a new uses_guc is introduced to represent the case were
we're committed to using the GuC. Note that uses_guc does still not imply
that the blob is actually loaded on the HW (is_running is the check for
that). Also, since we need to have attempted the fetch for the result
of uses_guc to be meaningful, we need to make sure we've moved away
from INTEL_UC_FIRMWARE_SELECTED.

All the GuC changes have been mirrored on the HuC for coherency.

v2: split fetch return changes and new macros to their own patches,
    support HuC only if GuC is wanted, improve "used" state
    description (Michal)

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Fernando Pacheco <fernando.pacheco@intel.com> #v1
---
 drivers/gpu/drm/i915/gt/uc/intel_guc.h    |  8 +++++++-
 drivers/gpu/drm/i915/gt/uc/intel_huc.h    |  8 +++++++-
 drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c |  2 +-
 drivers/gpu/drm/i915/gt/uc/intel_uc.c     | 23 ++++++++++++----------
 drivers/gpu/drm/i915/gt/uc/intel_uc.h     | 24 ++++++++++++++++++++++-
 5 files changed, 51 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index 7ca9e5159f05..f6b33745ae0b 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -143,11 +143,17 @@ static inline bool intel_guc_is_supported(struct intel_guc *guc)
 	return intel_uc_fw_is_supported(&guc->fw);
 }
 
-static inline bool intel_guc_is_enabled(struct intel_guc *guc)
+static inline bool intel_guc_is_wanted(struct intel_guc *guc)
 {
 	return intel_uc_fw_is_enabled(&guc->fw);
 }
 
+static inline bool intel_guc_is_used(struct intel_guc *guc)
+{
+	GEM_BUG_ON(__intel_uc_fw_status(&guc->fw) == INTEL_UC_FIRMWARE_SELECTED);
+	return intel_uc_fw_is_available(&guc->fw);
+}
+
 static inline bool intel_guc_is_fw_running(struct intel_guc *guc)
 {
 	return intel_uc_fw_is_running(&guc->fw);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
index 644c059fe01d..a40b9cfc6c22 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
@@ -41,11 +41,17 @@ static inline bool intel_huc_is_supported(struct intel_huc *huc)
 	return intel_uc_fw_is_supported(&huc->fw);
 }
 
-static inline bool intel_huc_is_enabled(struct intel_huc *huc)
+static inline bool intel_huc_is_wanted(struct intel_huc *huc)
 {
 	return intel_uc_fw_is_enabled(&huc->fw);
 }
 
+static inline bool intel_huc_is_used(struct intel_huc *huc)
+{
+	GEM_BUG_ON(__intel_uc_fw_status(&huc->fw) == INTEL_UC_FIRMWARE_SELECTED);
+	return intel_uc_fw_is_available(&huc->fw);
+}
+
 static inline bool intel_huc_is_authenticated(struct intel_huc *huc)
 {
 	return intel_uc_fw_is_running(&huc->fw);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
index eee193bf2cc4..9cdf4cbe691c 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
@@ -20,7 +20,7 @@ void intel_huc_fw_init_early(struct intel_huc *huc)
 	struct drm_i915_private *i915 = gt->i915;
 
 	intel_uc_fw_init_early(&huc->fw, INTEL_UC_FW_TYPE_HUC,
-			       intel_uc_uses_guc(uc),
+			       intel_uc_wants_guc(uc),
 			       INTEL_INFO(i915)->platform, INTEL_REVID(i915));
 }
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index affc4d6f9ead..654d7c0c757a 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -48,17 +48,17 @@ static void __confirm_options(struct intel_uc *uc)
 	DRM_DEV_DEBUG_DRIVER(i915->drm.dev,
 			     "enable_guc=%d (guc:%s submission:%s huc:%s)\n",
 			     i915_modparams.enable_guc,
-			     yesno(intel_uc_uses_guc(uc)),
+			     yesno(intel_uc_wants_guc(uc)),
 			     yesno(intel_uc_uses_guc_submission(uc)),
-			     yesno(intel_uc_uses_huc(uc)));
+			     yesno(intel_uc_wants_huc(uc)));
 
 	if (i915_modparams.enable_guc == -1)
 		return;
 
 	if (i915_modparams.enable_guc == 0) {
-		GEM_BUG_ON(intel_uc_uses_guc(uc));
+		GEM_BUG_ON(intel_uc_wants_guc(uc));
 		GEM_BUG_ON(intel_uc_uses_guc_submission(uc));
-		GEM_BUG_ON(intel_uc_uses_huc(uc));
+		GEM_BUG_ON(intel_uc_wants_huc(uc));
 		return;
 	}
 
@@ -93,7 +93,7 @@ void intel_uc_init_early(struct intel_uc *uc)
 
 	__confirm_options(uc);
 
-	if (intel_uc_uses_guc(uc))
+	if (intel_uc_wants_guc(uc))
 		uc->ops = &uc_ops_on;
 	else
 		uc->ops = &uc_ops_off;
@@ -257,13 +257,13 @@ static void __uc_fetch_firmwares(struct intel_uc *uc)
 {
 	int err;
 
-	GEM_BUG_ON(!intel_uc_uses_guc(uc));
+	GEM_BUG_ON(!intel_uc_wants_guc(uc));
 
 	err = intel_uc_fw_fetch(&uc->guc.fw);
 	if (err)
 		return;
 
-	if (intel_uc_uses_huc(uc))
+	if (intel_uc_wants_huc(uc))
 		intel_uc_fw_fetch(&uc->huc.fw);
 }
 
@@ -279,7 +279,10 @@ static void __uc_init(struct intel_uc *uc)
 	struct intel_huc *huc = &uc->huc;
 	int ret;
 
-	GEM_BUG_ON(!intel_uc_uses_guc(uc));
+	GEM_BUG_ON(!intel_uc_wants_guc(uc));
+
+	if (!intel_uc_uses_guc(uc))
+		return;
 
 	/* XXX: GuC submission is unavailable for now */
 	GEM_BUG_ON(intel_uc_supports_guc_submission(uc));
@@ -322,7 +325,7 @@ static int uc_init_wopcm(struct intel_uc *uc)
 	struct intel_uncore *uncore = gt->uncore;
 	u32 base = intel_wopcm_guc_base(&gt->i915->wopcm);
 	u32 size = intel_wopcm_guc_size(&gt->i915->wopcm);
-	u32 huc_agent = intel_uc_uses_huc(uc) ? HUC_LOADING_AGENT_GUC : 0;
+	u32 huc_agent = intel_uc_wants_huc(uc) ? HUC_LOADING_AGENT_GUC : 0;
 	u32 mask;
 	int err;
 
@@ -402,7 +405,7 @@ static int __uc_init_hw(struct intel_uc *uc)
 	int ret, attempts;
 
 	GEM_BUG_ON(!intel_uc_supports_guc(uc));
-	GEM_BUG_ON(!intel_uc_uses_guc(uc));
+	GEM_BUG_ON(!intel_uc_wants_guc(uc));
 
 	if (!intel_uc_fw_is_available(&guc->fw)) {
 		ret = __uc_check_hw(uc) ||
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
index 2ce993ceb60a..a41aaf353f88 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
@@ -40,6 +40,27 @@ void intel_uc_runtime_suspend(struct intel_uc *uc);
 int intel_uc_resume(struct intel_uc *uc);
 int intel_uc_runtime_resume(struct intel_uc *uc);
 
+/*
+ * We need to know as early as possible if we're going to use GuC or not to
+ * take the correct setup paths. Additionally, once we've started loading the
+ * GuC, it is unsafe to keep executing without it because some parts of the HW,
+ * a subset of which is not cleaned on GT reset, will start expecting the GuC FW
+ * to be running.
+ * To solve both these requirements, we commit to using the microcontrollers if
+ * the relevant modparam is set and the blobs are found on the system. At this
+ * stage, the only thing that can stop us from attempting to load the blobs on
+ * the HW and use them is a fundamental issue (e.g. no memory for our
+ * structures); if we hit such a problem during driver load we're broken even
+ * without GuC, so there is no point in trying to fall back.
+ *
+ * Given the above, we can be in one of 4 states, with the last one implying
+ * we're committed to using the microcontroller:
+ * - Not supported: not available in HW and/or firmware not defined.
+ * - Supported: available in HW and firmware defined.
+ * - Wanted: supported + enabled in modparam.
+ * - In use: wanted + firmware found on the system and successfully fetched.
+ */
+
 #define __uc_state_checker(x, state, required) \
 static inline bool intel_uc_##state##_##x(struct intel_uc *uc) \
 { \
@@ -48,7 +69,8 @@ static inline bool intel_uc_##state##_##x(struct intel_uc *uc) \
 
 #define uc_state_checkers(x) \
 __uc_state_checker(x, supports, supported) \
-__uc_state_checker(x, uses, enabled)
+__uc_state_checker(x, wants, wanted) \
+__uc_state_checker(x, uses, used)
 
 uc_state_checkers(guc);
 uc_state_checkers(huc);
-- 
2.24.1

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

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

* [Intel-gfx] [PATCH v2 07/10] drm/i915/guc: Apply new uC status tracking to GuC submission as well
  2020-02-03 23:28 [Intel-gfx] [PATCH v2 00/10] Commit early to GuC Daniele Ceraolo Spurio
                   ` (5 preceding siblings ...)
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 06/10] drm/i915/uc: Improve tracking of uC init status Daniele Ceraolo Spurio
@ 2020-02-03 23:28 ` Daniele Ceraolo Spurio
  2020-02-04 18:06   ` Michal Wajdeczko
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 08/10] drm/i915/uc: Abort early on uc_init failure Daniele Ceraolo Spurio
                   ` (5 subsequent siblings)
  12 siblings, 1 reply; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2020-02-03 23:28 UTC (permalink / raw)
  To: intel-gfx

To be able to differentiate the before and after of our commitment to
GuC submission, which will be used in follow-up patches to early set-up
the submission structures.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc.c        | 12 ++++----
 drivers/gpu/drm/i915/gt/uc/intel_guc.h        | 17 +++++++++--
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  9 ++----
 drivers/gpu/drm/i915/gt/uc/intel_uc.c         | 14 ++++-----
 drivers/gpu/drm/i915/gt/uc/intel_uc.h         | 29 +++++++------------
 drivers/gpu/drm/i915/gvt/scheduler.c          |  2 +-
 drivers/gpu/drm/i915/i915_drv.h               |  6 ----
 drivers/gpu/drm/i915/intel_gvt.c              |  2 +-
 8 files changed, 42 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
index c4c1523da7a6..f96d1bdf4bf2 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
@@ -207,7 +207,7 @@ static u32 guc_ctl_feature_flags(struct intel_guc *guc)
 {
 	u32 flags = 0;
 
-	if (!intel_guc_is_submission_supported(guc))
+	if (!intel_guc_submission_is_used(guc))
 		flags |= GUC_CTL_DISABLE_SCHEDULER;
 
 	return flags;
@@ -217,7 +217,7 @@ static u32 guc_ctl_ctxinfo_flags(struct intel_guc *guc)
 {
 	u32 flags = 0;
 
-	if (intel_guc_is_submission_supported(guc)) {
+	if (intel_guc_submission_is_used(guc)) {
 		u32 ctxnum, base;
 
 		base = intel_guc_ggtt_offset(guc, guc->stage_desc_pool);
@@ -348,7 +348,7 @@ int intel_guc_init(struct intel_guc *guc)
 	if (ret)
 		goto err_ads;
 
-	if (intel_guc_is_submission_supported(guc)) {
+	if (intel_guc_submission_is_used(guc)) {
 		/*
 		 * This is stuff we need to have available at fw load time
 		 * if we are planning to enable submission later
@@ -389,7 +389,7 @@ void intel_guc_fini(struct intel_guc *guc)
 
 	i915_ggtt_disable_guc(gt->ggtt);
 
-	if (intel_guc_is_submission_supported(guc))
+	if (intel_guc_submission_is_used(guc))
 		intel_guc_submission_fini(guc);
 
 	intel_guc_ct_fini(&guc->ct);
@@ -544,7 +544,7 @@ int intel_guc_suspend(struct intel_guc *guc)
 	 * If GuC communication is enabled but submission is not supported,
 	 * we do not need to suspend the GuC.
 	 */
-	if (!intel_guc_submission_is_enabled(guc))
+	if (!intel_guc_submission_is_used(guc) || !intel_guc_is_ready(guc))
 		return 0;
 
 	/*
@@ -609,7 +609,7 @@ int intel_guc_resume(struct intel_guc *guc)
 	 * we do not need to resume the GuC but we do need to enable the
 	 * GuC communication on resume (above).
 	 */
-	if (!intel_guc_submission_is_enabled(guc))
+	if (!intel_guc_submission_is_used(guc) || !intel_guc_is_ready(guc))
 		return 0;
 
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index f6b33745ae0b..1aba4d2c15b3 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -39,7 +39,7 @@ struct intel_guc {
 		void (*disable)(struct intel_guc *guc);
 	} interrupts;
 
-	bool submission_supported;
+	bool submission_selected;
 
 	struct i915_vma *ads_vma;
 	struct __guc_ads_blob *ads_blob;
@@ -172,9 +172,20 @@ static inline int intel_guc_sanitize(struct intel_guc *guc)
 	return 0;
 }
 
-static inline bool intel_guc_is_submission_supported(struct intel_guc *guc)
+static inline bool intel_guc_submission_is_supported(struct intel_guc *guc)
 {
-	return guc->submission_supported;
+	/* XXX: GuC submission is unavailable for now */
+	return false;
+}
+
+static inline bool intel_guc_submission_is_wanted(struct intel_guc *guc)
+{
+	return guc->submission_selected;
+}
+
+static inline bool intel_guc_submission_is_used(struct intel_guc *guc)
+{
+	return intel_guc_is_used(guc) && intel_guc_submission_is_wanted(guc);
 }
 
 static inline void intel_guc_enable_msg(struct intel_guc *guc, u32 mask)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 9e42324fdecd..1beaa77f9bb6 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -660,12 +660,9 @@ void intel_guc_submission_disable(struct intel_guc *guc)
 	guc_proc_desc_fini(guc);
 }
 
-static bool __guc_submission_support(struct intel_guc *guc)
+static bool __guc_submission_selected(struct intel_guc *guc)
 {
-	/* XXX: GuC submission is unavailable for now */
-	return false;
-
-	if (!intel_guc_is_supported(guc))
+	if (!intel_guc_submission_is_supported(guc))
 		return false;
 
 	return i915_modparams.enable_guc & ENABLE_GUC_SUBMISSION;
@@ -673,7 +670,7 @@ static bool __guc_submission_support(struct intel_guc *guc)
 
 void intel_guc_submission_init_early(struct intel_guc *guc)
 {
-	guc->submission_supported = __guc_submission_support(guc);
+	guc->submission_selected = __guc_submission_selected(guc);
 }
 
 bool intel_engine_in_guc_submission_mode(const struct intel_engine_cs *engine)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 654d7c0c757a..175fa6361ff2 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -49,7 +49,7 @@ static void __confirm_options(struct intel_uc *uc)
 			     "enable_guc=%d (guc:%s submission:%s huc:%s)\n",
 			     i915_modparams.enable_guc,
 			     yesno(intel_uc_wants_guc(uc)),
-			     yesno(intel_uc_uses_guc_submission(uc)),
+			     yesno(intel_uc_wants_guc_submission(uc)),
 			     yesno(intel_uc_wants_huc(uc)));
 
 	if (i915_modparams.enable_guc == -1)
@@ -57,7 +57,7 @@ static void __confirm_options(struct intel_uc *uc)
 
 	if (i915_modparams.enable_guc == 0) {
 		GEM_BUG_ON(intel_uc_wants_guc(uc));
-		GEM_BUG_ON(intel_uc_uses_guc_submission(uc));
+		GEM_BUG_ON(intel_uc_wants_guc_submission(uc));
 		GEM_BUG_ON(intel_uc_wants_huc(uc));
 		return;
 	}
@@ -285,7 +285,7 @@ static void __uc_init(struct intel_uc *uc)
 		return;
 
 	/* XXX: GuC submission is unavailable for now */
-	GEM_BUG_ON(intel_uc_supports_guc_submission(uc));
+	GEM_BUG_ON(intel_uc_uses_guc_submission(uc));
 
 	ret = intel_guc_init(guc);
 	if (ret) {
@@ -410,7 +410,7 @@ static int __uc_init_hw(struct intel_uc *uc)
 	if (!intel_uc_fw_is_available(&guc->fw)) {
 		ret = __uc_check_hw(uc) ||
 		      intel_uc_fw_is_overridden(&guc->fw) ||
-		      intel_uc_supports_guc_submission(uc) ?
+		      intel_uc_wants_guc_submission(uc) ?
 		      intel_uc_fw_status_to_error(guc->fw.status) : 0;
 		goto err_out;
 	}
@@ -462,14 +462,14 @@ static int __uc_init_hw(struct intel_uc *uc)
 	if (ret)
 		goto err_communication;
 
-	if (intel_uc_supports_guc_submission(uc))
+	if (intel_uc_uses_guc_submission(uc))
 		intel_guc_submission_enable(guc);
 
 	dev_info(i915->drm.dev, "%s firmware %s version %u.%u %s:%s\n",
 		 intel_uc_fw_type_repr(INTEL_UC_FW_TYPE_GUC), guc->fw.path,
 		 guc->fw.major_ver_found, guc->fw.minor_ver_found,
 		 "submission",
-		 enableddisabled(intel_uc_supports_guc_submission(uc)));
+		 enableddisabled(intel_uc_uses_guc_submission(uc)));
 
 	if (intel_uc_uses_huc(uc)) {
 		dev_info(i915->drm.dev, "%s firmware %s version %u.%u %s:%s\n",
@@ -511,7 +511,7 @@ static void __uc_fini_hw(struct intel_uc *uc)
 	if (!intel_guc_is_fw_running(guc))
 		return;
 
-	if (intel_uc_supports_guc_submission(uc))
+	if (intel_uc_uses_guc_submission(uc))
 		intel_guc_submission_disable(guc);
 
 	if (guc_communication_enabled(guc))
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
index a41aaf353f88..35ce8a6be88b 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
@@ -61,33 +61,24 @@ int intel_uc_runtime_resume(struct intel_uc *uc);
  * - In use: wanted + firmware found on the system and successfully fetched.
  */
 
-#define __uc_state_checker(x, state, required) \
-static inline bool intel_uc_##state##_##x(struct intel_uc *uc) \
+#define __uc_state_checker(x, func, state, required) \
+static inline bool intel_uc_##state##_##func(struct intel_uc *uc) \
 { \
-	return intel_##x##_is_##required(&uc->x); \
+	return intel_##func##_is_##required(&uc->x); \
 }
 
-#define uc_state_checkers(x) \
-__uc_state_checker(x, supports, supported) \
-__uc_state_checker(x, wants, wanted) \
-__uc_state_checker(x, uses, used)
+#define uc_state_checkers(x, func) \
+__uc_state_checker(x, func, supports, supported) \
+__uc_state_checker(x, func, wants, wanted) \
+__uc_state_checker(x, func, uses, used)
 
-uc_state_checkers(guc);
-uc_state_checkers(huc);
+uc_state_checkers(guc, guc);
+uc_state_checkers(huc, huc);
+uc_state_checkers(guc, guc_submission);
 
 #undef uc_state_checkers
 #undef __uc_state_checker
 
-static inline bool intel_uc_supports_guc_submission(struct intel_uc *uc)
-{
-	return intel_guc_is_submission_supported(&uc->guc);
-}
-
-static inline bool intel_uc_uses_guc_submission(struct intel_uc *uc)
-{
-	return intel_guc_is_submission_supported(&uc->guc);
-}
-
 #define intel_uc_ops_function(_NAME, _OPS, _TYPE, _RET) \
 static inline _TYPE intel_uc_##_NAME(struct intel_uc *uc) \
 { \
diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
index 5fe00ee6bd1b..e8c0885df978 100644
--- a/drivers/gpu/drm/i915/gvt/scheduler.c
+++ b/drivers/gpu/drm/i915/gvt/scheduler.c
@@ -1247,7 +1247,7 @@ int intel_vgpu_setup_submission(struct intel_vgpu *vgpu)
 		intel_context_set_single_submission(ce);
 
 		/* Max ring buffer size */
-		if (!intel_uc_uses_guc_submission(&engine->gt->uc)) {
+		if (!intel_uc_wants_guc_submission(&engine->gt->uc)) {
 			const unsigned int ring_size = 512 * SZ_4K;
 
 			ce->ring = __intel_context_ring_size(ring_size);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0a45240af38d..d21039ba2b4f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2010,10 +2010,4 @@ i915_coherent_map_type(struct drm_i915_private *i915)
 	return HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC;
 }
 
-static inline bool intel_guc_submission_is_enabled(struct intel_guc *guc)
-{
-	return intel_guc_is_submission_supported(guc) &&
-	       intel_guc_is_ready(guc);
-}
-
 #endif
diff --git a/drivers/gpu/drm/i915/intel_gvt.c b/drivers/gpu/drm/i915/intel_gvt.c
index 5c189bc31da5..e73fd752adef 100644
--- a/drivers/gpu/drm/i915/intel_gvt.c
+++ b/drivers/gpu/drm/i915/intel_gvt.c
@@ -105,7 +105,7 @@ int intel_gvt_init(struct drm_i915_private *dev_priv)
 		return 0;
 	}
 
-	if (intel_uc_uses_guc_submission(&dev_priv->gt.uc)) {
+	if (intel_uc_wants_guc_submission(&dev_priv->gt.uc)) {
 		drm_err(&dev_priv->drm,
 			"i915 GVT-g loading failed due to Graphics virtualization is not yet supported with GuC submission\n");
 		return -EIO;
-- 
2.24.1

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

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

* [Intel-gfx] [PATCH v2 08/10] drm/i915/uc: Abort early on uc_init failure
  2020-02-03 23:28 [Intel-gfx] [PATCH v2 00/10] Commit early to GuC Daniele Ceraolo Spurio
                   ` (6 preceding siblings ...)
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 07/10] drm/i915/guc: Apply new uC status tracking to GuC submission as well Daniele Ceraolo Spurio
@ 2020-02-03 23:28 ` Daniele Ceraolo Spurio
  2020-02-04 18:15   ` Michal Wajdeczko
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 09/10] drm/i915/uc: consolidate firmware cleanup Daniele Ceraolo Spurio
                   ` (4 subsequent siblings)
  12 siblings, 1 reply; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2020-02-03 23:28 UTC (permalink / raw)
  To: intel-gfx

Now that we can differentiate wants vs uses GuC/HuC, intel_uc_init is
restricted to running only if we have successfully fetched the required
blob(s) and are committed to using the microcontroller(s).
The only remaining thing that can go wrong in uc_init is the allocation
of GuC/HuC related objects; if we get such a failure better to bail out
immediately instead of wedging later, like we do for e.g.
intel_engines_init, since without objects we can't use the HW, including
not being able to attempt the firmware load.

While at it, remove the unneeded fw_cleanup call (this is handled
outside of gt_init) and add a probe failure injection point for testing.
Also, update the logs for <g/h>uc_init failures to probe_failure() since
they will cause the driver load to fail.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Fernando Pacheco <fernando.pacheco@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt.c     |  4 +++-
 drivers/gpu/drm/i915/gt/uc/intel_guc.c |  2 +-
 drivers/gpu/drm/i915/gt/uc/intel_huc.c |  2 +-
 drivers/gpu/drm/i915/gt/uc/intel_uc.c  | 24 +++++++++++++++++-------
 drivers/gpu/drm/i915/gt/uc/intel_uc.h  |  4 ++--
 5 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index f1f1b306e0af..cd64f81a3e60 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -592,7 +592,9 @@ int intel_gt_init(struct intel_gt *gt)
 	if (err)
 		goto err_engines;
 
-	intel_uc_init(&gt->uc);
+	err = intel_uc_init(&gt->uc);
+	if (err)
+		goto err_engines;
 
 	err = intel_gt_resume(gt);
 	if (err)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
index f96d1bdf4bf2..97b9c71a6fd4 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
@@ -376,7 +376,7 @@ int intel_guc_init(struct intel_guc *guc)
 	intel_uc_fw_fini(&guc->fw);
 err_fetch:
 	intel_uc_fw_cleanup_fetch(&guc->fw);
-	DRM_DEV_DEBUG_DRIVER(gt->i915->drm.dev, "failed with %d\n", ret);
+	i915_probe_error(gt->i915, "failed with %d\n", ret);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
index 32a069841c14..5f448d0e360b 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
@@ -127,7 +127,7 @@ int intel_huc_init(struct intel_huc *huc)
 	intel_uc_fw_fini(&huc->fw);
 out:
 	intel_uc_fw_cleanup_fetch(&huc->fw);
-	DRM_DEV_DEBUG_DRIVER(i915->drm.dev, "failed with %d\n", err);
+	i915_probe_error(i915, "failed with %d\n", err);
 	return err;
 }
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 175fa6361ff2..a119b7776973 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -273,7 +273,7 @@ static void __uc_cleanup_firmwares(struct intel_uc *uc)
 	intel_uc_fw_cleanup_fetch(&uc->guc.fw);
 }
 
-static void __uc_init(struct intel_uc *uc)
+static int __uc_init(struct intel_uc *uc)
 {
 	struct intel_guc *guc = &uc->guc;
 	struct intel_huc *huc = &uc->huc;
@@ -282,19 +282,29 @@ static void __uc_init(struct intel_uc *uc)
 	GEM_BUG_ON(!intel_uc_wants_guc(uc));
 
 	if (!intel_uc_uses_guc(uc))
-		return;
+		return 0;
+
+	if (i915_inject_probe_failure(uc_to_gt(uc)->i915))
+		return -ENOMEM;
 
 	/* XXX: GuC submission is unavailable for now */
 	GEM_BUG_ON(intel_uc_uses_guc_submission(uc));
 
 	ret = intel_guc_init(guc);
-	if (ret) {
-		intel_uc_fw_cleanup_fetch(&huc->fw);
-		return;
+	if (ret)
+		return ret;
+
+	if (intel_uc_uses_huc(uc)) {
+		ret = intel_huc_init(huc);
+		if (ret)
+			goto out_guc;
 	}
 
-	if (intel_uc_uses_huc(uc))
-		intel_huc_init(huc);
+	return 0;
+
+out_guc:
+	intel_guc_fini(guc);
+	return ret;
 }
 
 static void __uc_fini(struct intel_uc *uc)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
index 35ce8a6be88b..480965504679 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
@@ -16,7 +16,7 @@ struct intel_uc_ops {
 	int (*sanitize)(struct intel_uc *uc);
 	void (*init_fw)(struct intel_uc *uc);
 	void (*fini_fw)(struct intel_uc *uc);
-	void (*init)(struct intel_uc *uc);
+	int (*init)(struct intel_uc *uc);
 	void (*fini)(struct intel_uc *uc);
 	int (*init_hw)(struct intel_uc *uc);
 	void (*fini_hw)(struct intel_uc *uc);
@@ -89,7 +89,7 @@ static inline _TYPE intel_uc_##_NAME(struct intel_uc *uc) \
 intel_uc_ops_function(sanitize, sanitize, int, 0);
 intel_uc_ops_function(fetch_firmwares, init_fw, void, );
 intel_uc_ops_function(cleanup_firmwares, fini_fw, void, );
-intel_uc_ops_function(init, init, void, );
+intel_uc_ops_function(init, init, int, 0);
 intel_uc_ops_function(fini, fini, void, );
 intel_uc_ops_function(init_hw, init_hw, int, 0);
 intel_uc_ops_function(fini_hw, fini_hw, void, );
-- 
2.24.1

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

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

* [Intel-gfx] [PATCH v2 09/10] drm/i915/uc: consolidate firmware cleanup
  2020-02-03 23:28 [Intel-gfx] [PATCH v2 00/10] Commit early to GuC Daniele Ceraolo Spurio
                   ` (7 preceding siblings ...)
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 08/10] drm/i915/uc: Abort early on uc_init failure Daniele Ceraolo Spurio
@ 2020-02-03 23:28 ` Daniele Ceraolo Spurio
  2020-02-06 19:09   ` Michal Wajdeczko
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 10/10] HAX: drm/i915: default to enable_guc=2 Daniele Ceraolo Spurio
                   ` (3 subsequent siblings)
  12 siblings, 1 reply; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2020-02-03 23:28 UTC (permalink / raw)
  To: intel-gfx

We are quite trigger happy in cleaning up the firmware blobs, as we do
so from several error/fini paths in GuC/HuC/uC code. We do have the
__uc_cleanup_firmwares cleanup function, which unwinds
__uc_fetch_firmwares and is already called both from the error path of
gem_init and from gem_driver_release, so let's stop cleaning up from
all the other paths.

The fact that we're not cleaning the firmware immediately means that
we can't consider firmware availability as an indication of
initialization success. A "READY_TO_LOAD" status has been added to
indicate that the initialization was successful, to be used to
selectively load HuC only if HuC init has completed (HuC init failure
is not considered a fatal error).

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc.c   | 10 ++++------
 drivers/gpu/drm/i915/gt/uc/intel_huc.c   |  3 ++-
 drivers/gpu/drm/i915/gt/uc/intel_uc.c    |  2 +-
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c |  7 +++++--
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 18 +++++++++++++++---
 5 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
index 97b9c71a6fd4..2d05de570bdf 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
@@ -333,7 +333,7 @@ int intel_guc_init(struct intel_guc *guc)
 
 	ret = intel_uc_fw_init(&guc->fw);
 	if (ret)
-		goto err_fetch;
+		goto out;
 
 	ret = intel_guc_log_create(&guc->log);
 	if (ret)
@@ -364,6 +364,8 @@ int intel_guc_init(struct intel_guc *guc)
 	/* We need to notify the guc whenever we change the GGTT */
 	i915_ggtt_enable_guc(gt->ggtt);
 
+	intel_uc_fw_change_status(&guc->fw, INTEL_UC_FIRMWARE_READY_TO_LOAD);
+
 	return 0;
 
 err_ct:
@@ -374,8 +376,7 @@ int intel_guc_init(struct intel_guc *guc)
 	intel_guc_log_destroy(&guc->log);
 err_fw:
 	intel_uc_fw_fini(&guc->fw);
-err_fetch:
-	intel_uc_fw_cleanup_fetch(&guc->fw);
+out:
 	i915_probe_error(gt->i915, "failed with %d\n", ret);
 	return ret;
 }
@@ -397,9 +398,6 @@ void intel_guc_fini(struct intel_guc *guc)
 	intel_guc_ads_destroy(guc);
 	intel_guc_log_destroy(&guc->log);
 	intel_uc_fw_fini(&guc->fw);
-	intel_uc_fw_cleanup_fetch(&guc->fw);
-
-	intel_uc_fw_change_status(&guc->fw, INTEL_UC_FIRMWARE_DISABLED);
 }
 
 /*
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
index 5f448d0e360b..2e4f4a8e791e 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
@@ -121,12 +121,13 @@ int intel_huc_init(struct intel_huc *huc)
 	if (err)
 		goto out_fini;
 
+	intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_READY_TO_LOAD);
+
 	return 0;
 
 out_fini:
 	intel_uc_fw_fini(&huc->fw);
 out:
-	intel_uc_fw_cleanup_fetch(&huc->fw);
 	i915_probe_error(i915, "failed with %d\n", err);
 	return err;
 }
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index a119b7776973..4c55b1285cbc 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -417,7 +417,7 @@ static int __uc_init_hw(struct intel_uc *uc)
 	GEM_BUG_ON(!intel_uc_supports_guc(uc));
 	GEM_BUG_ON(!intel_uc_wants_guc(uc));
 
-	if (!intel_uc_fw_is_available(&guc->fw)) {
+	if (!intel_uc_fw_is_ready_to_load(&guc->fw)) {
 		ret = __uc_check_hw(uc) ||
 		      intel_uc_fw_is_overridden(&guc->fw) ||
 		      intel_uc_wants_guc_submission(uc) ?
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index c9c094a73399..3759569ec000 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -501,7 +501,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, u32 dst_offset, u32 dma_flags)
 	if (err)
 		return err;
 
-	if (!intel_uc_fw_is_available(uc_fw))
+	if (!intel_uc_fw_is_ready_to_load(uc_fw))
 		return -ENOEXEC;
 
 	/* Call custom loader */
@@ -544,7 +544,10 @@ int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
 
 void intel_uc_fw_fini(struct intel_uc_fw *uc_fw)
 {
-	intel_uc_fw_cleanup_fetch(uc_fw);
+	if (i915_gem_object_has_pinned_pages(uc_fw->obj))
+		i915_gem_object_unpin_pages(uc_fw->obj);
+
+	intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_AVAILABLE);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
index 1f30543d0d2d..ba3c362aeca2 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
@@ -29,8 +29,11 @@ struct intel_gt;
  * |            |                 SELECTED                          |
  * +------------+-               /   |   \                         -+
  * |            |    MISSING <--/    |    \--> ERROR                |
- * |   fetch    |                    |                              |
- * |            |        /------> AVAILABLE <---<-----------\       |
+ * |   fetch    |                    V                              |
+ * |            |                 AVAILABLE                         |
+ * |            |                    |                              |
+ * |            |                    V                              |
+ * |            |        /----> READY TO LOAD <---<---------\       |
  * +------------+-       \         /    \        \           \     -+
  * |            |         FAIL <--<      \--> TRANSFERRED     \     |
  * |   upload   |                  \           /   \          /     |
@@ -46,6 +49,7 @@ enum intel_uc_fw_status {
 	INTEL_UC_FIRMWARE_MISSING, /* blob not found on the system */
 	INTEL_UC_FIRMWARE_ERROR, /* invalid format or version */
 	INTEL_UC_FIRMWARE_AVAILABLE, /* blob found and copied in mem */
+	INTEL_UC_FIRMWARE_READY_TO_LOAD, /* all fw-required objects are ready */
 	INTEL_UC_FIRMWARE_FAIL, /* failed to xfer or init/auth the fw */
 	INTEL_UC_FIRMWARE_TRANSFERRED, /* dma xfer done */
 	INTEL_UC_FIRMWARE_RUNNING /* init/auth done */
@@ -115,6 +119,8 @@ const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
 		return "ERROR";
 	case INTEL_UC_FIRMWARE_AVAILABLE:
 		return "AVAILABLE";
+	case INTEL_UC_FIRMWARE_READY_TO_LOAD:
+		return "READY TO LOAD";
 	case INTEL_UC_FIRMWARE_FAIL:
 		return "FAIL";
 	case INTEL_UC_FIRMWARE_TRANSFERRED:
@@ -143,6 +149,7 @@ static inline int intel_uc_fw_status_to_error(enum intel_uc_fw_status status)
 	case INTEL_UC_FIRMWARE_SELECTED:
 		return -ESTALE;
 	case INTEL_UC_FIRMWARE_AVAILABLE:
+	case INTEL_UC_FIRMWARE_READY_TO_LOAD:
 	case INTEL_UC_FIRMWARE_TRANSFERRED:
 	case INTEL_UC_FIRMWARE_RUNNING:
 		return 0;
@@ -184,6 +191,11 @@ static inline bool intel_uc_fw_is_available(struct intel_uc_fw *uc_fw)
 	return __intel_uc_fw_status(uc_fw) >= INTEL_UC_FIRMWARE_AVAILABLE;
 }
 
+static inline bool intel_uc_fw_is_ready_to_load(struct intel_uc_fw *uc_fw)
+{
+	return __intel_uc_fw_status(uc_fw) == INTEL_UC_FIRMWARE_READY_TO_LOAD;
+}
+
 static inline bool intel_uc_fw_is_loaded(struct intel_uc_fw *uc_fw)
 {
 	return __intel_uc_fw_status(uc_fw) >= INTEL_UC_FIRMWARE_TRANSFERRED;
@@ -202,7 +214,7 @@ static inline bool intel_uc_fw_is_overridden(const struct intel_uc_fw *uc_fw)
 static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
 {
 	if (intel_uc_fw_is_loaded(uc_fw))
-		intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_AVAILABLE);
+		intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_READY_TO_LOAD);
 }
 
 static inline u32 __intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw)
-- 
2.24.1

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

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

* [Intel-gfx] [PATCH v2 10/10] HAX: drm/i915: default to enable_guc=2
  2020-02-03 23:28 [Intel-gfx] [PATCH v2 00/10] Commit early to GuC Daniele Ceraolo Spurio
                   ` (8 preceding siblings ...)
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 09/10] drm/i915/uc: consolidate firmware cleanup Daniele Ceraolo Spurio
@ 2020-02-03 23:28 ` Daniele Ceraolo Spurio
  2020-02-04 21:02 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Commit early to GuC (rev2) Patchwork
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2020-02-03 23:28 UTC (permalink / raw)
  To: intel-gfx

To enable GuC and HuC loading on all gen9+ CI machines.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@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 cb16410c2ada..7f6b3cbcd74f 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -56,7 +56,7 @@ struct drm_printer;
 	param(int, disable_power_well, -1, 0400) \
 	param(int, enable_ips, 1, 0600) \
 	param(int, invert_brightness, 0, 0600) \
-	param(int, enable_guc, 0, 0400) \
+	param(int, enable_guc, 2, 0400) \
 	param(int, guc_log_level, -1, 0400) \
 	param(char *, guc_firmware_path, NULL, 0400) \
 	param(char *, huc_firmware_path, NULL, 0400) \
-- 
2.24.1

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

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

* Re: [Intel-gfx] [PATCH v2 01/10] drm/i915/debugfs: Pass guc_log struct to i915_guc_log_info
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 01/10] drm/i915/debugfs: Pass guc_log struct to i915_guc_log_info Daniele Ceraolo Spurio
@ 2020-02-04 17:05   ` Michal Wajdeczko
  2020-02-04 21:10     ` Daniele Ceraolo Spurio
  2020-02-05  7:58     ` Jani Nikula
  0 siblings, 2 replies; 30+ messages in thread
From: Michal Wajdeczko @ 2020-02-04 17:05 UTC (permalink / raw)
  To: intel-gfx, Daniele Ceraolo Spurio

On Tue, 04 Feb 2020 00:28:29 +0100, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@intel.com> wrote:

> The log struct is the only thing the function needs (apart from
> the seq_file), so we can pass just that instead of the whole dev_priv.
>
> v2: Split this change to its own patch (Michal)
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c  
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index e75e8212f03b..7264c0ff766c 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1753,10 +1753,8 @@ stringify_guc_log_type(enum guc_log_buffer_type  
> type)
>  	return "";
>  }
> -static void i915_guc_log_info(struct seq_file *m,
> -			      struct drm_i915_private *dev_priv)
> +static void i915_guc_log_info(struct seq_file *m, struct intel_guc_log  
> *log)

maybe to avoid polluting i915_debugfs.c we should move this function to
gt/uc/intel_guc_log.c as universal printer:

void intel_guc_log_info(struct intel_guc_log *log, struct drm_printer *p)

>  {
> -	struct intel_guc_log *log = &dev_priv->gt.uc.guc.log;
>  	enum guc_log_buffer_type type;
> 	if (!intel_guc_log_relay_created(log)) {
> @@ -1784,7 +1782,7 @@ static int i915_guc_info(struct seq_file *m, void  
> *data)
>  	if (!USES_GUC(dev_priv))
>  		return -ENODEV;
> -	i915_guc_log_info(m, dev_priv);
> +	i915_guc_log_info(m, &dev_priv->gt.uc.guc.log);

too many dots ... this is "guc" info function, maybe we should have:

	struct intel_guc *guc = &dev_priv->gt.uc.guc;
or
	struct intel_gt *gt = &dev_priv->gt;
	struct intel_uc *uc = &gt->uc;
	struct intel_guc *guc = &uc->guc;

as that "guc" is likely to be reused in "more" below

> 	/* Add more as required ... */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v2 02/10] drm/i915/guc: Kill USES_GUC macro
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 02/10] drm/i915/guc: Kill USES_GUC macro Daniele Ceraolo Spurio
@ 2020-02-04 17:18   ` Michal Wajdeczko
  0 siblings, 0 replies; 30+ messages in thread
From: Michal Wajdeczko @ 2020-02-04 17:18 UTC (permalink / raw)
  To: intel-gfx, Daniele Ceraolo Spurio

On Tue, 04 Feb 2020 00:28:30 +0100, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@intel.com> wrote:

> use intel_uc_uses_guc() directly instead, to be consistent in the way we
> check what we want to do with the GuC.
>
> v2: split guc_log_info changes to their own patch (Michal)
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>

some diff could be smaller after updating 1/10

> ---
>  drivers/gpu/drm/i915/gt/intel_ggtt.c |  2 +-
>  drivers/gpu/drm/i915/i915_debugfs.c  | 15 +++++++++------
>  drivers/gpu/drm/i915/i915_drv.h      |  1 -
>  3 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c  
> b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> index 7dae91e0d002..048240b19772 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> @@ -427,7 +427,7 @@ static int ggtt_reserve_guc_top(struct i915_ggtt  
> *ggtt)
>  	u64 size;
>  	int ret;
> -	if (!USES_GUC(ggtt->vm.i915))
> +	if (!intel_uc_uses_guc(&ggtt->vm.gt->uc))
>  		return 0;
> 	GEM_BUG_ON(ggtt->vm.total <= GUC_GGTT_TOP);
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c  
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 7264c0ff766c..bfdcb657780b 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1778,11 +1778,12 @@ static void i915_guc_log_info(struct seq_file  
> *m, struct intel_guc_log *log)
>  static int i915_guc_info(struct seq_file *m, void *data)
>  {
>  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> +	struct intel_uc *uc = &dev_priv->gt.uc;
> -	if (!USES_GUC(dev_priv))
> +	if (!intel_uc_uses_guc(uc))
>  		return -ENODEV;
> -	i915_guc_log_info(m, &dev_priv->gt.uc.guc.log);
> +	i915_guc_log_info(m, &uc->guc.log);
> 	/* Add more as required ... */
> @@ -1883,11 +1884,12 @@ static int i915_guc_log_dump(struct seq_file *m,  
> void *data)
>  static int i915_guc_log_level_get(void *data, u64 *val)
>  {
>  	struct drm_i915_private *dev_priv = data;
> +	struct intel_uc *uc = &dev_priv->gt.uc;
> -	if (!USES_GUC(dev_priv))
> +	if (!intel_uc_uses_guc(uc))
>  		return -ENODEV;
> -	*val = intel_guc_log_get_level(&dev_priv->gt.uc.guc.log);
> +	*val = intel_guc_log_get_level(&uc->guc.log);
> 	return 0;
>  }
> @@ -1895,11 +1897,12 @@ static int i915_guc_log_level_get(void *data,  
> u64 *val)
>  static int i915_guc_log_level_set(void *data, u64 val)
>  {
>  	struct drm_i915_private *dev_priv = data;
> +	struct intel_uc *uc = &dev_priv->gt.uc;
> -	if (!USES_GUC(dev_priv))
> +	if (!intel_uc_uses_guc(uc))
>  		return -ENODEV;
> -	return intel_guc_log_set_level(&dev_priv->gt.uc.guc.log, val);
> +	return intel_guc_log_set_level(&uc->guc.log, val);
>  }
> DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_level_fops,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h  
> b/drivers/gpu/drm/i915/i915_drv.h
> index a71ff233cc55..a8e1ec7d571f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1701,7 +1701,6 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
>  #define HAS_GT_UC(dev_priv)	(INTEL_INFO(dev_priv)->has_gt_uc)
> /* Having GuC is not the same as using GuC */
> -#define USES_GUC(dev_priv)		intel_uc_uses_guc(&(dev_priv)->gt.uc)
>  #define  
> USES_GUC_SUBMISSION(dev_priv)	intel_uc_uses_guc_submission(&(dev_priv)->gt.uc)
> #define HAS_POOLED_EU(dev_priv)	(INTEL_INFO(dev_priv)->has_pooled_eu)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v2 04/10] drm/i915/uc: Update the FW status on injected fetch error
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 04/10] drm/i915/uc: Update the FW status on injected fetch error Daniele Ceraolo Spurio
@ 2020-02-04 17:20   ` Michal Wajdeczko
  0 siblings, 0 replies; 30+ messages in thread
From: Michal Wajdeczko @ 2020-02-04 17:20 UTC (permalink / raw)
  To: intel-gfx, Daniele Ceraolo Spurio

On Tue, 04 Feb 2020 00:28:32 +0100, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@intel.com> wrote:

> In a follow up patch we will rely on the fact that the status always
> moves away from "SELECTED" after the fetch is attempted to decide what
> to do with the GuC.
>
> v2: Split this change to its own patch (Michal)
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> ---

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>

>  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> index 8ee0a0c7f447..c9c094a73399 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -279,7 +279,7 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
> 	err = i915_inject_probe_error(i915, -ENXIO);
>  	if (err)
> -		return err;
> +		goto fail;
> 	__force_fw_fetch_failures(uc_fw, -EINVAL);
>  	__force_fw_fetch_failures(uc_fw, -ESTALE);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v2 06/10] drm/i915/uc: Improve tracking of uC init status
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 06/10] drm/i915/uc: Improve tracking of uC init status Daniele Ceraolo Spurio
@ 2020-02-04 17:54   ` Michal Wajdeczko
  2020-02-04 21:19     ` Daniele Ceraolo Spurio
  0 siblings, 1 reply; 30+ messages in thread
From: Michal Wajdeczko @ 2020-02-04 17:54 UTC (permalink / raw)
  To: intel-gfx, Daniele Ceraolo Spurio

On Tue, 04 Feb 2020 00:28:34 +0100, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@intel.com> wrote:

> To be able to setup GuC submission functions during engine init we need
> to commit to using GuC as soon as possible.
> Currently, the only thing that can stop us from using the
> microcontrollers once we've fetched the blobs is a fundamental
> error (e.g. OOM); given that if we hit such an error we can't really
> fall-back to anything, we can "officialize" the FW fetching completion
> as the moment at which we're committing to using GuC.
>
> To better differentiate this case, the uses_guc check, which indicates
> that GuC is supported and was selected in modparam, is renamed to
> wants_guc and a new uses_guc is introduced to represent the case were
> we're committed to using the GuC. Note that uses_guc does still not imply
> that the blob is actually loaded on the HW (is_running is the check for
> that). Also, since we need to have attempted the fetch for the result
> of uses_guc to be meaningful, we need to make sure we've moved away
> from INTEL_UC_FIRMWARE_SELECTED.
>
> All the GuC changes have been mirrored on the HuC for coherency.
>
> v2: split fetch return changes and new macros to their own patches,
>     support HuC only if GuC is wanted, improve "used" state
>     description (Michal)
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Reviewed-by: Fernando Pacheco <fernando.pacheco@intel.com> #v1
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc.h    |  8 +++++++-
>  drivers/gpu/drm/i915/gt/uc/intel_huc.h    |  8 +++++++-
>  drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c |  2 +-
>  drivers/gpu/drm/i915/gt/uc/intel_uc.c     | 23 ++++++++++++----------
>  drivers/gpu/drm/i915/gt/uc/intel_uc.h     | 24 ++++++++++++++++++++++-
>  5 files changed, 51 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h  
> b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index 7ca9e5159f05..f6b33745ae0b 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> @@ -143,11 +143,17 @@ static inline bool intel_guc_is_supported(struct  
> intel_guc *guc)
>  	return intel_uc_fw_is_supported(&guc->fw);
>  }
> -static inline bool intel_guc_is_enabled(struct intel_guc *guc)
> +static inline bool intel_guc_is_wanted(struct intel_guc *guc)
>  {
>  	return intel_uc_fw_is_enabled(&guc->fw);
>  }
> +static inline bool intel_guc_is_used(struct intel_guc *guc)
> +{
> +	GEM_BUG_ON(__intel_uc_fw_status(&guc->fw) ==  
> INTEL_UC_FIRMWARE_SELECTED);
> +	return intel_uc_fw_is_available(&guc->fw);
> +}
> +
>  static inline bool intel_guc_is_fw_running(struct intel_guc *guc)
>  {
>  	return intel_uc_fw_is_running(&guc->fw);
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h  
> b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
> index 644c059fe01d..a40b9cfc6c22 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
> @@ -41,11 +41,17 @@ static inline bool intel_huc_is_supported(struct  
> intel_huc *huc)
>  	return intel_uc_fw_is_supported(&huc->fw);
>  }
> -static inline bool intel_huc_is_enabled(struct intel_huc *huc)
> +static inline bool intel_huc_is_wanted(struct intel_huc *huc)
>  {
>  	return intel_uc_fw_is_enabled(&huc->fw);
>  }
> +static inline bool intel_huc_is_used(struct intel_huc *huc)
> +{
> +	GEM_BUG_ON(__intel_uc_fw_status(&huc->fw) ==  
> INTEL_UC_FIRMWARE_SELECTED);
> +	return intel_uc_fw_is_available(&huc->fw);
> +}
> +
>  static inline bool intel_huc_is_authenticated(struct intel_huc *huc)
>  {
>  	return intel_uc_fw_is_running(&huc->fw);
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> index eee193bf2cc4..9cdf4cbe691c 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> @@ -20,7 +20,7 @@ void intel_huc_fw_init_early(struct intel_huc *huc)
>  	struct drm_i915_private *i915 = gt->i915;
> 	intel_uc_fw_init_early(&huc->fw, INTEL_UC_FW_TYPE_HUC,
> -			       intel_uc_uses_guc(uc),
> +			       intel_uc_wants_guc(uc),
>  			       INTEL_INFO(i915)->platform, INTEL_REVID(i915));
>  }
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index affc4d6f9ead..654d7c0c757a 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -48,17 +48,17 @@ static void __confirm_options(struct intel_uc *uc)
>  	DRM_DEV_DEBUG_DRIVER(i915->drm.dev,
>  			     "enable_guc=%d (guc:%s submission:%s huc:%s)\n",
>  			     i915_modparams.enable_guc,
> -			     yesno(intel_uc_uses_guc(uc)),
> +			     yesno(intel_uc_wants_guc(uc)),
>  			     yesno(intel_uc_uses_guc_submission(uc)),
> -			     yesno(intel_uc_uses_huc(uc)));
> +			     yesno(intel_uc_wants_huc(uc)));
> 	if (i915_modparams.enable_guc == -1)
>  		return;
> 	if (i915_modparams.enable_guc == 0) {
> -		GEM_BUG_ON(intel_uc_uses_guc(uc));
> +		GEM_BUG_ON(intel_uc_wants_guc(uc));
>  		GEM_BUG_ON(intel_uc_uses_guc_submission(uc));
> -		GEM_BUG_ON(intel_uc_uses_huc(uc));
> +		GEM_BUG_ON(intel_uc_wants_huc(uc));
>  		return;
>  	}
> @@ -93,7 +93,7 @@ void intel_uc_init_early(struct intel_uc *uc)
> 	__confirm_options(uc);
> -	if (intel_uc_uses_guc(uc))
> +	if (intel_uc_wants_guc(uc))
>  		uc->ops = &uc_ops_on;
>  	else
>  		uc->ops = &uc_ops_off;
> @@ -257,13 +257,13 @@ static void __uc_fetch_firmwares(struct intel_uc  
> *uc)
>  {
>  	int err;
> -	GEM_BUG_ON(!intel_uc_uses_guc(uc));
> +	GEM_BUG_ON(!intel_uc_wants_guc(uc));
> 	err = intel_uc_fw_fetch(&uc->guc.fw);
>  	if (err)
>  		return;
> -	if (intel_uc_uses_huc(uc))
> +	if (intel_uc_wants_huc(uc))
>  		intel_uc_fw_fetch(&uc->huc.fw);
>  }
> @@ -279,7 +279,10 @@ static void __uc_init(struct intel_uc *uc)
>  	struct intel_huc *huc = &uc->huc;
>  	int ret;
> -	GEM_BUG_ON(!intel_uc_uses_guc(uc));
> +	GEM_BUG_ON(!intel_uc_wants_guc(uc));
> +
> +	if (!intel_uc_uses_guc(uc))
> +		return;
> 	/* XXX: GuC submission is unavailable for now */
>  	GEM_BUG_ON(intel_uc_supports_guc_submission(uc));
> @@ -322,7 +325,7 @@ static int uc_init_wopcm(struct intel_uc *uc)
>  	struct intel_uncore *uncore = gt->uncore;
>  	u32 base = intel_wopcm_guc_base(&gt->i915->wopcm);
>  	u32 size = intel_wopcm_guc_size(&gt->i915->wopcm);
> -	u32 huc_agent = intel_uc_uses_huc(uc) ? HUC_LOADING_AGENT_GUC : 0;
> +	u32 huc_agent = intel_uc_wants_huc(uc) ? HUC_LOADING_AGENT_GUC : 0;
>  	u32 mask;
>  	int err;
> @@ -402,7 +405,7 @@ static int __uc_init_hw(struct intel_uc *uc)
>  	int ret, attempts;
> 	GEM_BUG_ON(!intel_uc_supports_guc(uc));
> -	GEM_BUG_ON(!intel_uc_uses_guc(uc));
> +	GEM_BUG_ON(!intel_uc_wants_guc(uc));
> 	if (!intel_uc_fw_is_available(&guc->fw)) {
>  		ret = __uc_check_hw(uc) ||
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h  
> b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> index 2ce993ceb60a..a41aaf353f88 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> @@ -40,6 +40,27 @@ void intel_uc_runtime_suspend(struct intel_uc *uc);
>  int intel_uc_resume(struct intel_uc *uc);
>  int intel_uc_runtime_resume(struct intel_uc *uc);
> +/*
> + * We need to know as early as possible if we're going to use GuC or  
> not to
> + * take the correct setup paths. Additionally, once we've started  
> loading the
> + * GuC, it is unsafe to keep executing without it because some parts of  
> the HW,
> + * a subset of which is not cleaned on GT reset, will start expecting  
> the GuC FW
> + * to be running.
> + * To solve both these requirements, we commit to using the  
> microcontrollers if
> + * the relevant modparam is set and the blobs are found on the system.  
> At this
> + * stage, the only thing that can stop us from attempting to load the  
> blobs on
> + * the HW and use them is a fundamental issue (e.g. no memory for our
> + * structures); if we hit such a problem during driver load we're  
> broken even
> + * without GuC, so there is no point in trying to fall back.
> + *
> + * Given the above, we can be in one of 4 states, with the last one  
> implying
> + * we're committed to using the microcontroller:

nit: maybe we should document below state names in capitals as:
NOT_SUPPORTED, WANTED, IN_USE, ...

> + * - Not supported: not available in HW and/or firmware not defined.
> + * - Supported: available in HW and firmware defined.
> + * - Wanted: supported + enabled in modparam.

hmm, we are checking modparam during fw path selection, thus for me
there is no difference between SUPPORTED and WANTED, what I missed?

maybe we only need 3 states: NOT_SUPPORTED, WANTED, IN_USE.

> + * - In use: wanted + firmware found on the system and successfully  
> fetched.

In what state we will be after unsuccessful fetch ? still WANTED ?

> + */
> +
>  #define __uc_state_checker(x, state, required) \
>  static inline bool intel_uc_##state##_##x(struct intel_uc *uc) \
>  { \
> @@ -48,7 +69,8 @@ static inline bool intel_uc_##state##_##x(struct  
> intel_uc *uc) \
> #define uc_state_checkers(x) \
>  __uc_state_checker(x, supports, supported) \
> -__uc_state_checker(x, uses, enabled)
> +__uc_state_checker(x, wants, wanted) \
> +__uc_state_checker(x, uses, used)
> uc_state_checkers(guc);
>  uc_state_checkers(huc);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v2 07/10] drm/i915/guc: Apply new uC status tracking to GuC submission as well
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 07/10] drm/i915/guc: Apply new uC status tracking to GuC submission as well Daniele Ceraolo Spurio
@ 2020-02-04 18:06   ` Michal Wajdeczko
  2020-02-04 21:24     ` Daniele Ceraolo Spurio
  0 siblings, 1 reply; 30+ messages in thread
From: Michal Wajdeczko @ 2020-02-04 18:06 UTC (permalink / raw)
  To: intel-gfx, Daniele Ceraolo Spurio

On Tue, 04 Feb 2020 00:28:35 +0100, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@intel.com> wrote:

> To be able to differentiate the before and after of our commitment to
> GuC submission, which will be used in follow-up patches to early set-up
> the submission structures.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc.c        | 12 ++++----
>  drivers/gpu/drm/i915/gt/uc/intel_guc.h        | 17 +++++++++--
>  .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  9 ++----
>  drivers/gpu/drm/i915/gt/uc/intel_uc.c         | 14 ++++-----
>  drivers/gpu/drm/i915/gt/uc/intel_uc.h         | 29 +++++++------------
>  drivers/gpu/drm/i915/gvt/scheduler.c          |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h               |  6 ----
>  drivers/gpu/drm/i915/intel_gvt.c              |  2 +-
>  8 files changed, 42 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> index c4c1523da7a6..f96d1bdf4bf2 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> @@ -207,7 +207,7 @@ static u32 guc_ctl_feature_flags(struct intel_guc  
> *guc)
>  {
>  	u32 flags = 0;
> -	if (!intel_guc_is_submission_supported(guc))
> +	if (!intel_guc_submission_is_used(guc))
>  		flags |= GUC_CTL_DISABLE_SCHEDULER;
> 	return flags;
> @@ -217,7 +217,7 @@ static u32 guc_ctl_ctxinfo_flags(struct intel_guc  
> *guc)
>  {
>  	u32 flags = 0;
> -	if (intel_guc_is_submission_supported(guc)) {
> +	if (intel_guc_submission_is_used(guc)) {
>  		u32 ctxnum, base;
> 		base = intel_guc_ggtt_offset(guc, guc->stage_desc_pool);
> @@ -348,7 +348,7 @@ int intel_guc_init(struct intel_guc *guc)
>  	if (ret)
>  		goto err_ads;
> -	if (intel_guc_is_submission_supported(guc)) {
> +	if (intel_guc_submission_is_used(guc)) {
>  		/*
>  		 * This is stuff we need to have available at fw load time
>  		 * if we are planning to enable submission later
> @@ -389,7 +389,7 @@ void intel_guc_fini(struct intel_guc *guc)
> 	i915_ggtt_disable_guc(gt->ggtt);
> -	if (intel_guc_is_submission_supported(guc))
> +	if (intel_guc_submission_is_used(guc))
>  		intel_guc_submission_fini(guc);
> 	intel_guc_ct_fini(&guc->ct);
> @@ -544,7 +544,7 @@ int intel_guc_suspend(struct intel_guc *guc)
>  	 * If GuC communication is enabled but submission is not supported,
>  	 * we do not need to suspend the GuC.
>  	 */
> -	if (!intel_guc_submission_is_enabled(guc))
> +	if (!intel_guc_submission_is_used(guc) || !intel_guc_is_ready(guc))
>  		return 0;
> 	/*
> @@ -609,7 +609,7 @@ int intel_guc_resume(struct intel_guc *guc)
>  	 * we do not need to resume the GuC but we do need to enable the
>  	 * GuC communication on resume (above).
>  	 */
> -	if (!intel_guc_submission_is_enabled(guc))
> +	if (!intel_guc_submission_is_used(guc) || !intel_guc_is_ready(guc))
>  		return 0;
> 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h  
> b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index f6b33745ae0b..1aba4d2c15b3 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> @@ -39,7 +39,7 @@ struct intel_guc {
>  		void (*disable)(struct intel_guc *guc);
>  	} interrupts;
> -	bool submission_supported;
> +	bool submission_selected;
> 	struct i915_vma *ads_vma;
>  	struct __guc_ads_blob *ads_blob;
> @@ -172,9 +172,20 @@ static inline int intel_guc_sanitize(struct  
> intel_guc *guc)
>  	return 0;
>  }
> -static inline bool intel_guc_is_submission_supported(struct intel_guc  
> *guc)
> +static inline bool intel_guc_submission_is_supported(struct intel_guc  
> *guc)
>  {
> -	return guc->submission_supported;
> +	/* XXX: GuC submission is unavailable for now */
> +	return false;
> +}
> +
> +static inline bool intel_guc_submission_is_wanted(struct intel_guc *guc)
> +{
> +	return guc->submission_selected;
> +}
> +
> +static inline bool intel_guc_submission_is_used(struct intel_guc *guc)
> +{
> +	return intel_guc_is_used(guc) && intel_guc_submission_is_wanted(guc);
>  }

can we keep all intel_guc_submission_xx() functions in  
intel_guc_submission.h?
or is it circular-dependency issue ? but these functions are not on  
hot-path so
maybe can be moved to .c to break that dependency ?

or is it due to auto generator changes below ?

> static inline void intel_guc_enable_msg(struct intel_guc *guc, u32 mask)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index 9e42324fdecd..1beaa77f9bb6 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -660,12 +660,9 @@ void intel_guc_submission_disable(struct intel_guc  
> *guc)
>  	guc_proc_desc_fini(guc);
>  }
> -static bool __guc_submission_support(struct intel_guc *guc)
> +static bool __guc_submission_selected(struct intel_guc *guc)
>  {
> -	/* XXX: GuC submission is unavailable for now */
> -	return false;
> -
> -	if (!intel_guc_is_supported(guc))
> +	if (!intel_guc_submission_is_supported(guc))
>  		return false;
> 	return i915_modparams.enable_guc & ENABLE_GUC_SUBMISSION;
> @@ -673,7 +670,7 @@ static bool __guc_submission_support(struct  
> intel_guc *guc)
> void intel_guc_submission_init_early(struct intel_guc *guc)
>  {
> -	guc->submission_supported = __guc_submission_support(guc);
> +	guc->submission_selected = __guc_submission_selected(guc);
>  }
> bool intel_engine_in_guc_submission_mode(const struct intel_engine_cs  
> *engine)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index 654d7c0c757a..175fa6361ff2 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -49,7 +49,7 @@ static void __confirm_options(struct intel_uc *uc)
>  			     "enable_guc=%d (guc:%s submission:%s huc:%s)\n",
>  			     i915_modparams.enable_guc,
>  			     yesno(intel_uc_wants_guc(uc)),
> -			     yesno(intel_uc_uses_guc_submission(uc)),
> +			     yesno(intel_uc_wants_guc_submission(uc)),
>  			     yesno(intel_uc_wants_huc(uc)));
> 	if (i915_modparams.enable_guc == -1)
> @@ -57,7 +57,7 @@ static void __confirm_options(struct intel_uc *uc)
> 	if (i915_modparams.enable_guc == 0) {
>  		GEM_BUG_ON(intel_uc_wants_guc(uc));
> -		GEM_BUG_ON(intel_uc_uses_guc_submission(uc));
> +		GEM_BUG_ON(intel_uc_wants_guc_submission(uc));
>  		GEM_BUG_ON(intel_uc_wants_huc(uc));
>  		return;
>  	}
> @@ -285,7 +285,7 @@ static void __uc_init(struct intel_uc *uc)
>  		return;
> 	/* XXX: GuC submission is unavailable for now */
> -	GEM_BUG_ON(intel_uc_supports_guc_submission(uc));
> +	GEM_BUG_ON(intel_uc_uses_guc_submission(uc));
> 	ret = intel_guc_init(guc);
>  	if (ret) {
> @@ -410,7 +410,7 @@ static int __uc_init_hw(struct intel_uc *uc)
>  	if (!intel_uc_fw_is_available(&guc->fw)) {
>  		ret = __uc_check_hw(uc) ||
>  		      intel_uc_fw_is_overridden(&guc->fw) ||
> -		      intel_uc_supports_guc_submission(uc) ?
> +		      intel_uc_wants_guc_submission(uc) ?
>  		      intel_uc_fw_status_to_error(guc->fw.status) : 0;
>  		goto err_out;
>  	}
> @@ -462,14 +462,14 @@ static int __uc_init_hw(struct intel_uc *uc)
>  	if (ret)
>  		goto err_communication;
> -	if (intel_uc_supports_guc_submission(uc))
> +	if (intel_uc_uses_guc_submission(uc))
>  		intel_guc_submission_enable(guc);
> 	dev_info(i915->drm.dev, "%s firmware %s version %u.%u %s:%s\n",
>  		 intel_uc_fw_type_repr(INTEL_UC_FW_TYPE_GUC), guc->fw.path,
>  		 guc->fw.major_ver_found, guc->fw.minor_ver_found,
>  		 "submission",
> -		 enableddisabled(intel_uc_supports_guc_submission(uc)));
> +		 enableddisabled(intel_uc_uses_guc_submission(uc)));
> 	if (intel_uc_uses_huc(uc)) {
>  		dev_info(i915->drm.dev, "%s firmware %s version %u.%u %s:%s\n",
> @@ -511,7 +511,7 @@ static void __uc_fini_hw(struct intel_uc *uc)
>  	if (!intel_guc_is_fw_running(guc))
>  		return;
> -	if (intel_uc_supports_guc_submission(uc))
> +	if (intel_uc_uses_guc_submission(uc))
>  		intel_guc_submission_disable(guc);
> 	if (guc_communication_enabled(guc))
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h  
> b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> index a41aaf353f88..35ce8a6be88b 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> @@ -61,33 +61,24 @@ int intel_uc_runtime_resume(struct intel_uc *uc);
>   * - In use: wanted + firmware found on the system and successfully  
> fetched.
>   */
> -#define __uc_state_checker(x, state, required) \
> -static inline bool intel_uc_##state##_##x(struct intel_uc *uc) \
> +#define __uc_state_checker(x, func, state, required) \
> +static inline bool intel_uc_##state##_##func(struct intel_uc *uc) \
>  { \
> -	return intel_##x##_is_##required(&uc->x); \
> +	return intel_##func##_is_##required(&uc->x); \
>  }
> -#define uc_state_checkers(x) \
> -__uc_state_checker(x, supports, supported) \
> -__uc_state_checker(x, wants, wanted) \
> -__uc_state_checker(x, uses, used)
> +#define uc_state_checkers(x, func) \
> +__uc_state_checker(x, func, supports, supported) \
> +__uc_state_checker(x, func, wants, wanted) \
> +__uc_state_checker(x, func, uses, used)
> -uc_state_checkers(guc);
> -uc_state_checkers(huc);
> +uc_state_checkers(guc, guc);
> +uc_state_checkers(huc, huc);
> +uc_state_checkers(guc, guc_submission);
> #undef uc_state_checkers
>  #undef __uc_state_checker
> -static inline bool intel_uc_supports_guc_submission(struct intel_uc *uc)
> -{
> -	return intel_guc_is_submission_supported(&uc->guc);
> -}
> -
> -static inline bool intel_uc_uses_guc_submission(struct intel_uc *uc)
> -{
> -	return intel_guc_is_submission_supported(&uc->guc);
> -}
> -
>  #define intel_uc_ops_function(_NAME, _OPS, _TYPE, _RET) \
>  static inline _TYPE intel_uc_##_NAME(struct intel_uc *uc) \
>  { \
> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c  
> b/drivers/gpu/drm/i915/gvt/scheduler.c
> index 5fe00ee6bd1b..e8c0885df978 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> @@ -1247,7 +1247,7 @@ int intel_vgpu_setup_submission(struct intel_vgpu  
> *vgpu)
>  		intel_context_set_single_submission(ce);
> 		/* Max ring buffer size */
> -		if (!intel_uc_uses_guc_submission(&engine->gt->uc)) {
> +		if (!intel_uc_wants_guc_submission(&engine->gt->uc)) {
>  			const unsigned int ring_size = 512 * SZ_4K;
> 			ce->ring = __intel_context_ring_size(ring_size);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h  
> b/drivers/gpu/drm/i915/i915_drv.h
> index 0a45240af38d..d21039ba2b4f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2010,10 +2010,4 @@ i915_coherent_map_type(struct drm_i915_private  
> *i915)
>  	return HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC;
>  }
> -static inline bool intel_guc_submission_is_enabled(struct intel_guc  
> *guc)
> -{
> -	return intel_guc_is_submission_supported(guc) &&
> -	       intel_guc_is_ready(guc);
> -}
> -
>  #endif
> diff --git a/drivers/gpu/drm/i915/intel_gvt.c  
> b/drivers/gpu/drm/i915/intel_gvt.c
> index 5c189bc31da5..e73fd752adef 100644
> --- a/drivers/gpu/drm/i915/intel_gvt.c
> +++ b/drivers/gpu/drm/i915/intel_gvt.c
> @@ -105,7 +105,7 @@ int intel_gvt_init(struct drm_i915_private *dev_priv)
>  		return 0;
>  	}
> -	if (intel_uc_uses_guc_submission(&dev_priv->gt.uc)) {
> +	if (intel_uc_wants_guc_submission(&dev_priv->gt.uc)) {
>  		drm_err(&dev_priv->drm,
>  			"i915 GVT-g loading failed due to Graphics virtualization is not yet  
> supported with GuC submission\n");
>  		return -EIO;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v2 08/10] drm/i915/uc: Abort early on uc_init failure
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 08/10] drm/i915/uc: Abort early on uc_init failure Daniele Ceraolo Spurio
@ 2020-02-04 18:15   ` Michal Wajdeczko
  2020-02-04 21:26     ` Daniele Ceraolo Spurio
  0 siblings, 1 reply; 30+ messages in thread
From: Michal Wajdeczko @ 2020-02-04 18:15 UTC (permalink / raw)
  To: intel-gfx, Daniele Ceraolo Spurio

On Tue, 04 Feb 2020 00:28:36 +0100, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@intel.com> wrote:

> Now that we can differentiate wants vs uses GuC/HuC, intel_uc_init is
> restricted to running only if we have successfully fetched the required
> blob(s) and are committed to using the microcontroller(s).
> The only remaining thing that can go wrong in uc_init is the allocation
> of GuC/HuC related objects; if we get such a failure better to bail out
> immediately instead of wedging later, like we do for e.g.
> intel_engines_init, since without objects we can't use the HW, including
> not being able to attempt the firmware load.
>
> While at it, remove the unneeded fw_cleanup call (this is handled
> outside of gt_init) and add a probe failure injection point for testing.
> Also, update the logs for <g/h>uc_init failures to probe_failure() since
> they will cause the driver load to fail.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Reviewed-by: Fernando Pacheco <fernando.pacheco@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_gt.c     |  4 +++-
>  drivers/gpu/drm/i915/gt/uc/intel_guc.c |  2 +-
>  drivers/gpu/drm/i915/gt/uc/intel_huc.c |  2 +-
>  drivers/gpu/drm/i915/gt/uc/intel_uc.c  | 24 +++++++++++++++++-------
>  drivers/gpu/drm/i915/gt/uc/intel_uc.h  |  4 ++--
>  5 files changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c  
> b/drivers/gpu/drm/i915/gt/intel_gt.c
> index f1f1b306e0af..cd64f81a3e60 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -592,7 +592,9 @@ int intel_gt_init(struct intel_gt *gt)
>  	if (err)
>  		goto err_engines;
> -	intel_uc_init(&gt->uc);
> +	err = intel_uc_init(&gt->uc);
> +	if (err)
> +		goto err_engines;
> 	err = intel_gt_resume(gt);
>  	if (err)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> index f96d1bdf4bf2..97b9c71a6fd4 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> @@ -376,7 +376,7 @@ int intel_guc_init(struct intel_guc *guc)
>  	intel_uc_fw_fini(&guc->fw);
>  err_fetch:
>  	intel_uc_fw_cleanup_fetch(&guc->fw);

btw, we will cleanup all uc firmwares in i915_gem_driver_release()
so maybe we don't need to cleanup it here

> -	DRM_DEV_DEBUG_DRIVER(gt->i915->drm.dev, "failed with %d\n", ret);
> +	i915_probe_error(gt->i915, "failed with %d\n", ret);
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> index 32a069841c14..5f448d0e360b 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> @@ -127,7 +127,7 @@ int intel_huc_init(struct intel_huc *huc)
>  	intel_uc_fw_fini(&huc->fw);
>  out:
>  	intel_uc_fw_cleanup_fetch(&huc->fw);

and here

> -	DRM_DEV_DEBUG_DRIVER(i915->drm.dev, "failed with %d\n", err);
> +	i915_probe_error(i915, "failed with %d\n", err);
>  	return err;
>  }
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index 175fa6361ff2..a119b7776973 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -273,7 +273,7 @@ static void __uc_cleanup_firmwares(struct intel_uc  
> *uc)
>  	intel_uc_fw_cleanup_fetch(&uc->guc.fw);
>  }
> -static void __uc_init(struct intel_uc *uc)
> +static int __uc_init(struct intel_uc *uc)
>  {
>  	struct intel_guc *guc = &uc->guc;
>  	struct intel_huc *huc = &uc->huc;
> @@ -282,19 +282,29 @@ static void __uc_init(struct intel_uc *uc)
>  	GEM_BUG_ON(!intel_uc_wants_guc(uc));
> 	if (!intel_uc_uses_guc(uc))
> -		return;
> +		return 0;
> +
> +	if (i915_inject_probe_failure(uc_to_gt(uc)->i915))
> +		return -ENOMEM;
> 	/* XXX: GuC submission is unavailable for now */
>  	GEM_BUG_ON(intel_uc_uses_guc_submission(uc));
> 	ret = intel_guc_init(guc);
> -	if (ret) {
> -		intel_uc_fw_cleanup_fetch(&huc->fw);
> -		return;
> +	if (ret)
> +		return ret;
> +
> +	if (intel_uc_uses_huc(uc)) {
> +		ret = intel_huc_init(huc);
> +		if (ret)
> +			goto out_guc;
>  	}
> -	if (intel_uc_uses_huc(uc))
> -		intel_huc_init(huc);
> +	return 0;
> +
> +out_guc:
> +	intel_guc_fini(guc);
> +	return ret;
>  }
> static void __uc_fini(struct intel_uc *uc)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h  
> b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> index 35ce8a6be88b..480965504679 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> @@ -16,7 +16,7 @@ struct intel_uc_ops {
>  	int (*sanitize)(struct intel_uc *uc);
>  	void (*init_fw)(struct intel_uc *uc);
>  	void (*fini_fw)(struct intel_uc *uc);
> -	void (*init)(struct intel_uc *uc);
> +	int (*init)(struct intel_uc *uc);
>  	void (*fini)(struct intel_uc *uc);
>  	int (*init_hw)(struct intel_uc *uc);
>  	void (*fini_hw)(struct intel_uc *uc);
> @@ -89,7 +89,7 @@ static inline _TYPE intel_uc_##_NAME(struct intel_uc  
> *uc) \
>  intel_uc_ops_function(sanitize, sanitize, int, 0);
>  intel_uc_ops_function(fetch_firmwares, init_fw, void, );
>  intel_uc_ops_function(cleanup_firmwares, fini_fw, void, );
> -intel_uc_ops_function(init, init, void, );
> +intel_uc_ops_function(init, init, int, 0);
>  intel_uc_ops_function(fini, fini, void, );
>  intel_uc_ops_function(init_hw, init_hw, int, 0);
>  intel_uc_ops_function(fini_hw, fini_hw, void, );

with redundant (?) fw cleanups clarified,

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Commit early to GuC (rev2)
  2020-02-03 23:28 [Intel-gfx] [PATCH v2 00/10] Commit early to GuC Daniele Ceraolo Spurio
                   ` (9 preceding siblings ...)
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 10/10] HAX: drm/i915: default to enable_guc=2 Daniele Ceraolo Spurio
@ 2020-02-04 21:02 ` Patchwork
  2020-02-04 21:23 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
  2020-02-06 21:15 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
  12 siblings, 0 replies; 30+ messages in thread
From: Patchwork @ 2020-02-04 21:02 UTC (permalink / raw)
  To: Daniele Ceraolo Spurio; +Cc: intel-gfx

== Series Details ==

Series: Commit early to GuC (rev2)
URL   : https://patchwork.freedesktop.org/series/72031/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
e5cb7d5a536e drm/i915/debugfs: Pass guc_log struct to i915_guc_log_info
ebe1197848d5 drm/i915/guc: Kill USES_GUC macro
2243201c583e drm/i915/guc: Kill USES_GUC_SUBMISSION macro
f0481b5c0bfb drm/i915/uc: Update the FW status on injected fetch error
0cecb99111fb drm/i915/uc: autogenerate uC checker functions
-:31: CHECK:MACRO_ARG_PRECEDENCE: Macro argument 'x' may be better as '(x)' to avoid precedence issues
#31: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc.h:43:
+#define __uc_state_checker(x, state, required) \
+static inline bool intel_uc_##state##_##x(struct intel_uc *uc) \
+{ \
+	return intel_##x##_is_##required(&uc->x); \
 }

-:41: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#41: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc.h:49:
+#define uc_state_checkers(x) \
+__uc_state_checker(x, supports, supported) \
+__uc_state_checker(x, uses, enabled)

-:41: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'x' - possible side-effects?
#41: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc.h:49:
+#define uc_state_checkers(x) \
+__uc_state_checker(x, supports, supported) \
+__uc_state_checker(x, uses, enabled)

total: 1 errors, 0 warnings, 2 checks, 44 lines checked
a5eb1097207f drm/i915/uc: Improve tracking of uC init status
0cd48e992b8b drm/i915/guc: Apply new uC status tracking to GuC submission as well
-:212: CHECK:MACRO_ARG_PRECEDENCE: Macro argument 'x' may be better as '(x)' to avoid precedence issues
#212: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc.h:64:
+#define __uc_state_checker(x, func, state, required) \
+static inline bool intel_uc_##state##_##func(struct intel_uc *uc) \
 { \
+	return intel_##func##_is_##required(&uc->x); \
 }

-:223: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#223: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc.h:70:
+#define uc_state_checkers(x, func) \
+__uc_state_checker(x, func, supports, supported) \
+__uc_state_checker(x, func, wants, wanted) \
+__uc_state_checker(x, func, uses, used)

-:223: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'x' - possible side-effects?
#223: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc.h:70:
+#define uc_state_checkers(x, func) \
+__uc_state_checker(x, func, supports, supported) \
+__uc_state_checker(x, func, wants, wanted) \
+__uc_state_checker(x, func, uses, used)

-:223: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'func' - possible side-effects?
#223: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc.h:70:
+#define uc_state_checkers(x, func) \
+__uc_state_checker(x, func, supports, supported) \
+__uc_state_checker(x, func, wants, wanted) \
+__uc_state_checker(x, func, uses, used)

total: 1 errors, 0 warnings, 3 checks, 225 lines checked
61eee547ff65 drm/i915/uc: Abort early on uc_init failure
1d318ae28200 drm/i915/uc: consolidate firmware cleanup
e4ad6a295fb1 HAX: drm/i915: default to enable_guc=2

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

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

* Re: [Intel-gfx] [PATCH v2 01/10] drm/i915/debugfs: Pass guc_log struct to i915_guc_log_info
  2020-02-04 17:05   ` Michal Wajdeczko
@ 2020-02-04 21:10     ` Daniele Ceraolo Spurio
  2020-02-04 23:06       ` Daniele Ceraolo Spurio
  2020-02-05  7:58     ` Jani Nikula
  1 sibling, 1 reply; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2020-02-04 21:10 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx



On 2/4/20 9:05 AM, Michal Wajdeczko wrote:
> On Tue, 04 Feb 2020 00:28:29 +0100, Daniele Ceraolo Spurio 
> <daniele.ceraolospurio@intel.com> wrote:
> 
>> The log struct is the only thing the function needs (apart from
>> the seq_file), so we can pass just that instead of the whole dev_priv.
>>
>> v2: Split this change to its own patch (Michal)
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: John Harrison <John.C.Harrison@Intel.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index e75e8212f03b..7264c0ff766c 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1753,10 +1753,8 @@ stringify_guc_log_type(enum guc_log_buffer_type 
>> type)
>>      return "";
>>  }
>> -static void i915_guc_log_info(struct seq_file *m,
>> -                  struct drm_i915_private *dev_priv)
>> +static void i915_guc_log_info(struct seq_file *m, struct 
>> intel_guc_log *log)
> 
> maybe to avoid polluting i915_debugfs.c we should move this function to
> gt/uc/intel_guc_log.c as universal printer:
> 
> void intel_guc_log_info(struct intel_guc_log *log, struct drm_printer *p)
> 

ok

>>  {
>> -    struct intel_guc_log *log = &dev_priv->gt.uc.guc.log;
>>      enum guc_log_buffer_type type;
>>     if (!intel_guc_log_relay_created(log)) {
>> @@ -1784,7 +1782,7 @@ static int i915_guc_info(struct seq_file *m, 
>> void *data)
>>      if (!USES_GUC(dev_priv))
>>          return -ENODEV;
>> -    i915_guc_log_info(m, dev_priv);
>> +    i915_guc_log_info(m, &dev_priv->gt.uc.guc.log);
> 
> too many dots ... this is "guc" info function, maybe we should have:

This is changed in the next patch to use intel_uc. It made more sense to 
me to keep that change in the patch that introduced a second use for the 
variable.

Daniele

> 
>      struct intel_guc *guc = &dev_priv->gt.uc.guc;
> or
>      struct intel_gt *gt = &dev_priv->gt;
>      struct intel_uc *uc = &gt->uc;
>      struct intel_guc *guc = &uc->guc;
> 
> as that "guc" is likely to be reused in "more" below
> 
>>     /* Add more as required ... */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v2 06/10] drm/i915/uc: Improve tracking of uC init status
  2020-02-04 17:54   ` Michal Wajdeczko
@ 2020-02-04 21:19     ` Daniele Ceraolo Spurio
  0 siblings, 0 replies; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2020-02-04 21:19 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx



On 2/4/20 9:54 AM, Michal Wajdeczko wrote:
> On Tue, 04 Feb 2020 00:28:34 +0100, Daniele Ceraolo Spurio 
> <daniele.ceraolospurio@intel.com> wrote:
> 
>> To be able to setup GuC submission functions during engine init we need
>> to commit to using GuC as soon as possible.
>> Currently, the only thing that can stop us from using the
>> microcontrollers once we've fetched the blobs is a fundamental
>> error (e.g. OOM); given that if we hit such an error we can't really
>> fall-back to anything, we can "officialize" the FW fetching completion
>> as the moment at which we're committing to using GuC.
>>
>> To better differentiate this case, the uses_guc check, which indicates
>> that GuC is supported and was selected in modparam, is renamed to
>> wants_guc and a new uses_guc is introduced to represent the case were
>> we're committed to using the GuC. Note that uses_guc does still not imply
>> that the blob is actually loaded on the HW (is_running is the check for
>> that). Also, since we need to have attempted the fetch for the result
>> of uses_guc to be meaningful, we need to make sure we've moved away
>> from INTEL_UC_FIRMWARE_SELECTED.
>>
>> All the GuC changes have been mirrored on the HuC for coherency.
>>
>> v2: split fetch return changes and new macros to their own patches,
>>     support HuC only if GuC is wanted, improve "used" state
>>     description (Michal)
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: John Harrison <John.C.Harrison@Intel.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Reviewed-by: Fernando Pacheco <fernando.pacheco@intel.com> #v1
>> ---
>>  drivers/gpu/drm/i915/gt/uc/intel_guc.h    |  8 +++++++-
>>  drivers/gpu/drm/i915/gt/uc/intel_huc.h    |  8 +++++++-
>>  drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c |  2 +-
>>  drivers/gpu/drm/i915/gt/uc/intel_uc.c     | 23 ++++++++++++----------
>>  drivers/gpu/drm/i915/gt/uc/intel_uc.h     | 24 ++++++++++++++++++++++-
>>  5 files changed, 51 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>> index 7ca9e5159f05..f6b33745ae0b 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>> @@ -143,11 +143,17 @@ static inline bool intel_guc_is_supported(struct 
>> intel_guc *guc)
>>      return intel_uc_fw_is_supported(&guc->fw);
>>  }
>> -static inline bool intel_guc_is_enabled(struct intel_guc *guc)
>> +static inline bool intel_guc_is_wanted(struct intel_guc *guc)
>>  {
>>      return intel_uc_fw_is_enabled(&guc->fw);
>>  }
>> +static inline bool intel_guc_is_used(struct intel_guc *guc)
>> +{
>> +    GEM_BUG_ON(__intel_uc_fw_status(&guc->fw) == 
>> INTEL_UC_FIRMWARE_SELECTED);
>> +    return intel_uc_fw_is_available(&guc->fw);
>> +}
>> +
>>  static inline bool intel_guc_is_fw_running(struct intel_guc *guc)
>>  {
>>      return intel_uc_fw_is_running(&guc->fw);
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h 
>> b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
>> index 644c059fe01d..a40b9cfc6c22 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
>> @@ -41,11 +41,17 @@ static inline bool intel_huc_is_supported(struct 
>> intel_huc *huc)
>>      return intel_uc_fw_is_supported(&huc->fw);
>>  }
>> -static inline bool intel_huc_is_enabled(struct intel_huc *huc)
>> +static inline bool intel_huc_is_wanted(struct intel_huc *huc)
>>  {
>>      return intel_uc_fw_is_enabled(&huc->fw);
>>  }
>> +static inline bool intel_huc_is_used(struct intel_huc *huc)
>> +{
>> +    GEM_BUG_ON(__intel_uc_fw_status(&huc->fw) == 
>> INTEL_UC_FIRMWARE_SELECTED);
>> +    return intel_uc_fw_is_available(&huc->fw);
>> +}
>> +
>>  static inline bool intel_huc_is_authenticated(struct intel_huc *huc)
>>  {
>>      return intel_uc_fw_is_running(&huc->fw);
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c 
>> b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>> index eee193bf2cc4..9cdf4cbe691c 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>> @@ -20,7 +20,7 @@ void intel_huc_fw_init_early(struct intel_huc *huc)
>>      struct drm_i915_private *i915 = gt->i915;
>>     intel_uc_fw_init_early(&huc->fw, INTEL_UC_FW_TYPE_HUC,
>> -                   intel_uc_uses_guc(uc),
>> +                   intel_uc_wants_guc(uc),
>>                     INTEL_INFO(i915)->platform, INTEL_REVID(i915));
>>  }
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c 
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> index affc4d6f9ead..654d7c0c757a 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> @@ -48,17 +48,17 @@ static void __confirm_options(struct intel_uc *uc)
>>      DRM_DEV_DEBUG_DRIVER(i915->drm.dev,
>>                   "enable_guc=%d (guc:%s submission:%s huc:%s)\n",
>>                   i915_modparams.enable_guc,
>> -                 yesno(intel_uc_uses_guc(uc)),
>> +                 yesno(intel_uc_wants_guc(uc)),
>>                   yesno(intel_uc_uses_guc_submission(uc)),
>> -                 yesno(intel_uc_uses_huc(uc)));
>> +                 yesno(intel_uc_wants_huc(uc)));
>>     if (i915_modparams.enable_guc == -1)
>>          return;
>>     if (i915_modparams.enable_guc == 0) {
>> -        GEM_BUG_ON(intel_uc_uses_guc(uc));
>> +        GEM_BUG_ON(intel_uc_wants_guc(uc));
>>          GEM_BUG_ON(intel_uc_uses_guc_submission(uc));
>> -        GEM_BUG_ON(intel_uc_uses_huc(uc));
>> +        GEM_BUG_ON(intel_uc_wants_huc(uc));
>>          return;
>>      }
>> @@ -93,7 +93,7 @@ void intel_uc_init_early(struct intel_uc *uc)
>>     __confirm_options(uc);
>> -    if (intel_uc_uses_guc(uc))
>> +    if (intel_uc_wants_guc(uc))
>>          uc->ops = &uc_ops_on;
>>      else
>>          uc->ops = &uc_ops_off;
>> @@ -257,13 +257,13 @@ static void __uc_fetch_firmwares(struct intel_uc 
>> *uc)
>>  {
>>      int err;
>> -    GEM_BUG_ON(!intel_uc_uses_guc(uc));
>> +    GEM_BUG_ON(!intel_uc_wants_guc(uc));
>>     err = intel_uc_fw_fetch(&uc->guc.fw);
>>      if (err)
>>          return;
>> -    if (intel_uc_uses_huc(uc))
>> +    if (intel_uc_wants_huc(uc))
>>          intel_uc_fw_fetch(&uc->huc.fw);
>>  }
>> @@ -279,7 +279,10 @@ static void __uc_init(struct intel_uc *uc)
>>      struct intel_huc *huc = &uc->huc;
>>      int ret;
>> -    GEM_BUG_ON(!intel_uc_uses_guc(uc));
>> +    GEM_BUG_ON(!intel_uc_wants_guc(uc));
>> +
>> +    if (!intel_uc_uses_guc(uc))
>> +        return;
>>     /* XXX: GuC submission is unavailable for now */
>>      GEM_BUG_ON(intel_uc_supports_guc_submission(uc));
>> @@ -322,7 +325,7 @@ static int uc_init_wopcm(struct intel_uc *uc)
>>      struct intel_uncore *uncore = gt->uncore;
>>      u32 base = intel_wopcm_guc_base(&gt->i915->wopcm);
>>      u32 size = intel_wopcm_guc_size(&gt->i915->wopcm);
>> -    u32 huc_agent = intel_uc_uses_huc(uc) ? HUC_LOADING_AGENT_GUC : 0;
>> +    u32 huc_agent = intel_uc_wants_huc(uc) ? HUC_LOADING_AGENT_GUC : 0;

this should've been uses_huc, since this is post fetch.

>>      u32 mask;
>>      int err;
>> @@ -402,7 +405,7 @@ static int __uc_init_hw(struct intel_uc *uc)
>>      int ret, attempts;
>>     GEM_BUG_ON(!intel_uc_supports_guc(uc));
>> -    GEM_BUG_ON(!intel_uc_uses_guc(uc));
>> +    GEM_BUG_ON(!intel_uc_wants_guc(uc));
>>     if (!intel_uc_fw_is_available(&guc->fw)) {
>>          ret = __uc_check_hw(uc) ||
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h 
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> index 2ce993ceb60a..a41aaf353f88 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> @@ -40,6 +40,27 @@ void intel_uc_runtime_suspend(struct intel_uc *uc);
>>  int intel_uc_resume(struct intel_uc *uc);
>>  int intel_uc_runtime_resume(struct intel_uc *uc);
>> +/*
>> + * We need to know as early as possible if we're going to use GuC or 
>> not to
>> + * take the correct setup paths. Additionally, once we've started 
>> loading the
>> + * GuC, it is unsafe to keep executing without it because some parts 
>> of the HW,
>> + * a subset of which is not cleaned on GT reset, will start expecting 
>> the GuC FW
>> + * to be running.
>> + * To solve both these requirements, we commit to using the 
>> microcontrollers if
>> + * the relevant modparam is set and the blobs are found on the 
>> system. At this
>> + * stage, the only thing that can stop us from attempting to load the 
>> blobs on
>> + * the HW and use them is a fundamental issue (e.g. no memory for our
>> + * structures); if we hit such a problem during driver load we're 
>> broken even
>> + * without GuC, so there is no point in trying to fall back.
>> + *
>> + * Given the above, we can be in one of 4 states, with the last one 
>> implying
>> + * we're committed to using the microcontroller:
> 
> nit: maybe we should document below state names in capitals as:
> NOT_SUPPORTED, WANTED, IN_USE, ...
> 
>> + * - Not supported: not available in HW and/or firmware not defined.
>> + * - Supported: available in HW and firmware defined.
>> + * - Wanted: supported + enabled in modparam.
> 
> hmm, we are checking modparam during fw path selection, thus for me
> there is no difference between SUPPORTED and WANTED, what I missed?
> 
> maybe we only need 3 states: NOT_SUPPORTED, WANTED, IN_USE.
> 

enable_guc=0 maps to SUPPORTED on platforms that have the GuC, i.e. the 
HW has it but we haven't turned it on. NOT_SUPPORTED is for older 
platforms that don't have the microcontroller, while WANTED is for 
enable_guc != 0 on platforms that do support the GuC. Supported vs 
wanted is mainly used for error logging. Note than NOT_SUPPORTED is not 
a separate state in the code, but it is quite literally !(SUPPORTED), so 
the actual states are indeed 3.

>> + * - In use: wanted + firmware found on the system and successfully 
>> fetched.
> 
> In what state we will be after unsuccessful fetch ? still WANTED ?
> 

yes, on purpose. This reflects the current behavior and I believe it'll 
be useful in case we need to conditionally unwind anything that has been 
done before the fetch.

Daniele

>> + */
>> +
>>  #define __uc_state_checker(x, state, required) \
>>  static inline bool intel_uc_##state##_##x(struct intel_uc *uc) \
>>  { \
>> @@ -48,7 +69,8 @@ static inline bool intel_uc_##state##_##x(struct 
>> intel_uc *uc) \
>> #define uc_state_checkers(x) \
>>  __uc_state_checker(x, supports, supported) \
>> -__uc_state_checker(x, uses, enabled)
>> +__uc_state_checker(x, wants, wanted) \
>> +__uc_state_checker(x, uses, used)
>> uc_state_checkers(guc);
>>  uc_state_checkers(huc);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* [Intel-gfx] ✓ Fi.CI.BAT: success for Commit early to GuC (rev2)
  2020-02-03 23:28 [Intel-gfx] [PATCH v2 00/10] Commit early to GuC Daniele Ceraolo Spurio
                   ` (10 preceding siblings ...)
  2020-02-04 21:02 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Commit early to GuC (rev2) Patchwork
@ 2020-02-04 21:23 ` Patchwork
  2020-02-06 21:15 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
  12 siblings, 0 replies; 30+ messages in thread
From: Patchwork @ 2020-02-04 21:23 UTC (permalink / raw)
  To: Daniele Ceraolo Spurio; +Cc: intel-gfx

== Series Details ==

Series: Commit early to GuC (rev2)
URL   : https://patchwork.freedesktop.org/series/72031/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_7864 -> Patchwork_16406
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/index.html

Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_parallel@fds:
    - fi-byt-n2820:       [PASS][1] -> [FAIL][2] ([i915#694])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/fi-byt-n2820/igt@gem_exec_parallel@fds.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/fi-byt-n2820/igt@gem_exec_parallel@fds.html

  
#### Possible fixes ####

  * igt@i915_pm_rpm@module-reload:
    - fi-skl-6770hq:      [FAIL][3] ([i915#178]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html

  * igt@i915_selftest@live_gtt:
    - fi-skl-6600u:       [TIMEOUT][5] ([fdo#111732] / [fdo#112271]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/fi-skl-6600u/igt@i915_selftest@live_gtt.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/fi-skl-6600u/igt@i915_selftest@live_gtt.html

  * igt@i915_selftest@live_perf:
    - fi-apl-guc:         [INCOMPLETE][7] ([fdo#103927]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/fi-apl-guc/igt@i915_selftest@live_perf.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/fi-apl-guc/igt@i915_selftest@live_perf.html

  
#### Warnings ####

  * igt@gem_exec_parallel@contexts:
    - fi-byt-n2820:       [TIMEOUT][9] ([fdo#112271]) -> [FAIL][10] ([i915#694])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/fi-byt-n2820/igt@gem_exec_parallel@contexts.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/fi-byt-n2820/igt@gem_exec_parallel@contexts.html

  * igt@i915_selftest@live_blt:
    - fi-hsw-4770:        [DMESG-FAIL][11] ([i915#725]) -> [DMESG-FAIL][12] ([i915#770])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/fi-hsw-4770/igt@i915_selftest@live_blt.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/fi-hsw-4770/igt@i915_selftest@live_blt.html

  
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#111732]: https://bugs.freedesktop.org/show_bug.cgi?id=111732
  [fdo#112271]: https://bugs.freedesktop.org/show_bug.cgi?id=112271
  [i915#178]: https://gitlab.freedesktop.org/drm/intel/issues/178
  [i915#694]: https://gitlab.freedesktop.org/drm/intel/issues/694
  [i915#725]: https://gitlab.freedesktop.org/drm/intel/issues/725
  [i915#770]: https://gitlab.freedesktop.org/drm/intel/issues/770


Participating hosts (50 -> 38)
------------------------------

  Additional (3): fi-skl-lmem fi-glk-dsi fi-snb-2520m 
  Missing    (15): fi-ilk-m540 fi-bdw-samus fi-bsw-n3050 fi-byt-j1900 fi-hsw-4200u fi-byt-squawks fi-ilk-650 fi-ctg-p8600 fi-gdg-551 fi-elk-e7500 fi-blb-e6850 fi-tgl-y fi-byt-clapper fi-bsw-nick fi-skl-6700k2 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7864 -> Patchwork_16406

  CI-20190529: 20190529
  CI_DRM_7864: 5a140e2fc771e4c8b10d14e2db7bfb4996ee9d8a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5417: 33cc93c8ba5daa0b7498f297a4f626844d895d06 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_16406: e4ad6a295fb15116c45e09ae490589bb06ad736d @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

e4ad6a295fb1 HAX: drm/i915: default to enable_guc=2
1d318ae28200 drm/i915/uc: consolidate firmware cleanup
61eee547ff65 drm/i915/uc: Abort early on uc_init failure
0cd48e992b8b drm/i915/guc: Apply new uC status tracking to GuC submission as well
a5eb1097207f drm/i915/uc: Improve tracking of uC init status
0cecb99111fb drm/i915/uc: autogenerate uC checker functions
f0481b5c0bfb drm/i915/uc: Update the FW status on injected fetch error
2243201c583e drm/i915/guc: Kill USES_GUC_SUBMISSION macro
ebe1197848d5 drm/i915/guc: Kill USES_GUC macro
e5cb7d5a536e drm/i915/debugfs: Pass guc_log struct to i915_guc_log_info

== Logs ==

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

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

* Re: [Intel-gfx] [PATCH v2 07/10] drm/i915/guc: Apply new uC status tracking to GuC submission as well
  2020-02-04 18:06   ` Michal Wajdeczko
@ 2020-02-04 21:24     ` Daniele Ceraolo Spurio
  0 siblings, 0 replies; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2020-02-04 21:24 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx



On 2/4/20 10:06 AM, Michal Wajdeczko wrote:
> On Tue, 04 Feb 2020 00:28:35 +0100, Daniele Ceraolo Spurio 
> <daniele.ceraolospurio@intel.com> wrote:
> 
>> To be able to differentiate the before and after of our commitment to
>> GuC submission, which will be used in follow-up patches to early set-up
>> the submission structures.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> ---
>>  drivers/gpu/drm/i915/gt/uc/intel_guc.c        | 12 ++++----
>>  drivers/gpu/drm/i915/gt/uc/intel_guc.h        | 17 +++++++++--
>>  .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  9 ++----
>>  drivers/gpu/drm/i915/gt/uc/intel_uc.c         | 14 ++++-----
>>  drivers/gpu/drm/i915/gt/uc/intel_uc.h         | 29 +++++++------------
>>  drivers/gpu/drm/i915/gvt/scheduler.c          |  2 +-
>>  drivers/gpu/drm/i915/i915_drv.h               |  6 ----
>>  drivers/gpu/drm/i915/intel_gvt.c              |  2 +-
>>  8 files changed, 42 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c 
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> index c4c1523da7a6..f96d1bdf4bf2 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> @@ -207,7 +207,7 @@ static u32 guc_ctl_feature_flags(struct intel_guc 
>> *guc)
>>  {
>>      u32 flags = 0;
>> -    if (!intel_guc_is_submission_supported(guc))
>> +    if (!intel_guc_submission_is_used(guc))
>>          flags |= GUC_CTL_DISABLE_SCHEDULER;
>>     return flags;
>> @@ -217,7 +217,7 @@ static u32 guc_ctl_ctxinfo_flags(struct intel_guc 
>> *guc)
>>  {
>>      u32 flags = 0;
>> -    if (intel_guc_is_submission_supported(guc)) {
>> +    if (intel_guc_submission_is_used(guc)) {
>>          u32 ctxnum, base;
>>         base = intel_guc_ggtt_offset(guc, guc->stage_desc_pool);
>> @@ -348,7 +348,7 @@ int intel_guc_init(struct intel_guc *guc)
>>      if (ret)
>>          goto err_ads;
>> -    if (intel_guc_is_submission_supported(guc)) {
>> +    if (intel_guc_submission_is_used(guc)) {
>>          /*
>>           * This is stuff we need to have available at fw load time
>>           * if we are planning to enable submission later
>> @@ -389,7 +389,7 @@ void intel_guc_fini(struct intel_guc *guc)
>>     i915_ggtt_disable_guc(gt->ggtt);
>> -    if (intel_guc_is_submission_supported(guc))
>> +    if (intel_guc_submission_is_used(guc))
>>          intel_guc_submission_fini(guc);
>>     intel_guc_ct_fini(&guc->ct);
>> @@ -544,7 +544,7 @@ int intel_guc_suspend(struct intel_guc *guc)
>>       * If GuC communication is enabled but submission is not supported,
>>       * we do not need to suspend the GuC.
>>       */
>> -    if (!intel_guc_submission_is_enabled(guc))
>> +    if (!intel_guc_submission_is_used(guc) || !intel_guc_is_ready(guc))
>>          return 0;
>>     /*
>> @@ -609,7 +609,7 @@ int intel_guc_resume(struct intel_guc *guc)
>>       * we do not need to resume the GuC but we do need to enable the
>>       * GuC communication on resume (above).
>>       */
>> -    if (!intel_guc_submission_is_enabled(guc))
>> +    if (!intel_guc_submission_is_used(guc) || !intel_guc_is_ready(guc))
>>          return 0;
>>     return intel_guc_send(guc, action, ARRAY_SIZE(action));
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>> index f6b33745ae0b..1aba4d2c15b3 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>> @@ -39,7 +39,7 @@ struct intel_guc {
>>          void (*disable)(struct intel_guc *guc);
>>      } interrupts;
>> -    bool submission_supported;
>> +    bool submission_selected;
>>     struct i915_vma *ads_vma;
>>      struct __guc_ads_blob *ads_blob;
>> @@ -172,9 +172,20 @@ static inline int intel_guc_sanitize(struct 
>> intel_guc *guc)
>>      return 0;
>>  }
>> -static inline bool intel_guc_is_submission_supported(struct intel_guc 
>> *guc)
>> +static inline bool intel_guc_submission_is_supported(struct intel_guc 
>> *guc)
>>  {
>> -    return guc->submission_supported;
>> +    /* XXX: GuC submission is unavailable for now */
>> +    return false;
>> +}
>> +
>> +static inline bool intel_guc_submission_is_wanted(struct intel_guc *guc)
>> +{
>> +    return guc->submission_selected;
>> +}
>> +
>> +static inline bool intel_guc_submission_is_used(struct intel_guc *guc)
>> +{
>> +    return intel_guc_is_used(guc) && 
>> intel_guc_submission_is_wanted(guc);
>>  }
> 
> can we keep all intel_guc_submission_xx() functions in 
> intel_guc_submission.h?
> or is it circular-dependency issue ? but these functions are not on 
> hot-path so
> maybe can be moved to .c to break that dependency ?
> 
> or is it due to auto generator changes below ?
> 

I haven't actually tried to move them to intel_guc_submission.h. 
intel_guc_is_submission_supported was here so I just kept the other 
related functions here as well. IMO including intel_guc.h from 
intel_guc_submission.h shouldn't be an issue given the close relation 
between the 2, so it should be ok to move the functions.

Daniele

>> static inline void intel_guc_enable_msg(struct intel_guc *guc, u32 mask)
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> index 9e42324fdecd..1beaa77f9bb6 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> @@ -660,12 +660,9 @@ void intel_guc_submission_disable(struct 
>> intel_guc *guc)
>>      guc_proc_desc_fini(guc);
>>  }
>> -static bool __guc_submission_support(struct intel_guc *guc)
>> +static bool __guc_submission_selected(struct intel_guc *guc)
>>  {
>> -    /* XXX: GuC submission is unavailable for now */
>> -    return false;
>> -
>> -    if (!intel_guc_is_supported(guc))
>> +    if (!intel_guc_submission_is_supported(guc))
>>          return false;
>>     return i915_modparams.enable_guc & ENABLE_GUC_SUBMISSION;
>> @@ -673,7 +670,7 @@ static bool __guc_submission_support(struct 
>> intel_guc *guc)
>> void intel_guc_submission_init_early(struct intel_guc *guc)
>>  {
>> -    guc->submission_supported = __guc_submission_support(guc);
>> +    guc->submission_selected = __guc_submission_selected(guc);
>>  }
>> bool intel_engine_in_guc_submission_mode(const struct intel_engine_cs 
>> *engine)
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c 
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> index 654d7c0c757a..175fa6361ff2 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> @@ -49,7 +49,7 @@ static void __confirm_options(struct intel_uc *uc)
>>                   "enable_guc=%d (guc:%s submission:%s huc:%s)\n",
>>                   i915_modparams.enable_guc,
>>                   yesno(intel_uc_wants_guc(uc)),
>> -                 yesno(intel_uc_uses_guc_submission(uc)),
>> +                 yesno(intel_uc_wants_guc_submission(uc)),
>>                   yesno(intel_uc_wants_huc(uc)));
>>     if (i915_modparams.enable_guc == -1)
>> @@ -57,7 +57,7 @@ static void __confirm_options(struct intel_uc *uc)
>>     if (i915_modparams.enable_guc == 0) {
>>          GEM_BUG_ON(intel_uc_wants_guc(uc));
>> -        GEM_BUG_ON(intel_uc_uses_guc_submission(uc));
>> +        GEM_BUG_ON(intel_uc_wants_guc_submission(uc));
>>          GEM_BUG_ON(intel_uc_wants_huc(uc));
>>          return;
>>      }
>> @@ -285,7 +285,7 @@ static void __uc_init(struct intel_uc *uc)
>>          return;
>>     /* XXX: GuC submission is unavailable for now */
>> -    GEM_BUG_ON(intel_uc_supports_guc_submission(uc));
>> +    GEM_BUG_ON(intel_uc_uses_guc_submission(uc));
>>     ret = intel_guc_init(guc);
>>      if (ret) {
>> @@ -410,7 +410,7 @@ static int __uc_init_hw(struct intel_uc *uc)
>>      if (!intel_uc_fw_is_available(&guc->fw)) {
>>          ret = __uc_check_hw(uc) ||
>>                intel_uc_fw_is_overridden(&guc->fw) ||
>> -              intel_uc_supports_guc_submission(uc) ?
>> +              intel_uc_wants_guc_submission(uc) ?
>>                intel_uc_fw_status_to_error(guc->fw.status) : 0;
>>          goto err_out;
>>      }
>> @@ -462,14 +462,14 @@ static int __uc_init_hw(struct intel_uc *uc)
>>      if (ret)
>>          goto err_communication;
>> -    if (intel_uc_supports_guc_submission(uc))
>> +    if (intel_uc_uses_guc_submission(uc))
>>          intel_guc_submission_enable(guc);
>>     dev_info(i915->drm.dev, "%s firmware %s version %u.%u %s:%s\n",
>>           intel_uc_fw_type_repr(INTEL_UC_FW_TYPE_GUC), guc->fw.path,
>>           guc->fw.major_ver_found, guc->fw.minor_ver_found,
>>           "submission",
>> -         enableddisabled(intel_uc_supports_guc_submission(uc)));
>> +         enableddisabled(intel_uc_uses_guc_submission(uc)));
>>     if (intel_uc_uses_huc(uc)) {
>>          dev_info(i915->drm.dev, "%s firmware %s version %u.%u %s:%s\n",
>> @@ -511,7 +511,7 @@ static void __uc_fini_hw(struct intel_uc *uc)
>>      if (!intel_guc_is_fw_running(guc))
>>          return;
>> -    if (intel_uc_supports_guc_submission(uc))
>> +    if (intel_uc_uses_guc_submission(uc))
>>          intel_guc_submission_disable(guc);
>>     if (guc_communication_enabled(guc))
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h 
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> index a41aaf353f88..35ce8a6be88b 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> @@ -61,33 +61,24 @@ int intel_uc_runtime_resume(struct intel_uc *uc);
>>   * - In use: wanted + firmware found on the system and successfully 
>> fetched.
>>   */
>> -#define __uc_state_checker(x, state, required) \
>> -static inline bool intel_uc_##state##_##x(struct intel_uc *uc) \
>> +#define __uc_state_checker(x, func, state, required) \
>> +static inline bool intel_uc_##state##_##func(struct intel_uc *uc) \
>>  { \
>> -    return intel_##x##_is_##required(&uc->x); \
>> +    return intel_##func##_is_##required(&uc->x); \
>>  }
>> -#define uc_state_checkers(x) \
>> -__uc_state_checker(x, supports, supported) \
>> -__uc_state_checker(x, wants, wanted) \
>> -__uc_state_checker(x, uses, used)
>> +#define uc_state_checkers(x, func) \
>> +__uc_state_checker(x, func, supports, supported) \
>> +__uc_state_checker(x, func, wants, wanted) \
>> +__uc_state_checker(x, func, uses, used)
>> -uc_state_checkers(guc);
>> -uc_state_checkers(huc);
>> +uc_state_checkers(guc, guc);
>> +uc_state_checkers(huc, huc);
>> +uc_state_checkers(guc, guc_submission);
>> #undef uc_state_checkers
>>  #undef __uc_state_checker
>> -static inline bool intel_uc_supports_guc_submission(struct intel_uc *uc)
>> -{
>> -    return intel_guc_is_submission_supported(&uc->guc);
>> -}
>> -
>> -static inline bool intel_uc_uses_guc_submission(struct intel_uc *uc)
>> -{
>> -    return intel_guc_is_submission_supported(&uc->guc);
>> -}
>> -
>>  #define intel_uc_ops_function(_NAME, _OPS, _TYPE, _RET) \
>>  static inline _TYPE intel_uc_##_NAME(struct intel_uc *uc) \
>>  { \
>> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c 
>> b/drivers/gpu/drm/i915/gvt/scheduler.c
>> index 5fe00ee6bd1b..e8c0885df978 100644
>> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
>> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
>> @@ -1247,7 +1247,7 @@ int intel_vgpu_setup_submission(struct 
>> intel_vgpu *vgpu)
>>          intel_context_set_single_submission(ce);
>>         /* Max ring buffer size */
>> -        if (!intel_uc_uses_guc_submission(&engine->gt->uc)) {
>> +        if (!intel_uc_wants_guc_submission(&engine->gt->uc)) {
>>              const unsigned int ring_size = 512 * SZ_4K;
>>             ce->ring = __intel_context_ring_size(ring_size);
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 0a45240af38d..d21039ba2b4f 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2010,10 +2010,4 @@ i915_coherent_map_type(struct drm_i915_private 
>> *i915)
>>      return HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC;
>>  }
>> -static inline bool intel_guc_submission_is_enabled(struct intel_guc 
>> *guc)
>> -{
>> -    return intel_guc_is_submission_supported(guc) &&
>> -           intel_guc_is_ready(guc);
>> -}
>> -
>>  #endif
>> diff --git a/drivers/gpu/drm/i915/intel_gvt.c 
>> b/drivers/gpu/drm/i915/intel_gvt.c
>> index 5c189bc31da5..e73fd752adef 100644
>> --- a/drivers/gpu/drm/i915/intel_gvt.c
>> +++ b/drivers/gpu/drm/i915/intel_gvt.c
>> @@ -105,7 +105,7 @@ int intel_gvt_init(struct drm_i915_private *dev_priv)
>>          return 0;
>>      }
>> -    if (intel_uc_uses_guc_submission(&dev_priv->gt.uc)) {
>> +    if (intel_uc_wants_guc_submission(&dev_priv->gt.uc)) {
>>          drm_err(&dev_priv->drm,
>>              "i915 GVT-g loading failed due to Graphics virtualization 
>> is not yet supported with GuC submission\n");
>>          return -EIO;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v2 08/10] drm/i915/uc: Abort early on uc_init failure
  2020-02-04 18:15   ` Michal Wajdeczko
@ 2020-02-04 21:26     ` Daniele Ceraolo Spurio
  2020-02-06 15:33       ` Michal Wajdeczko
  0 siblings, 1 reply; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2020-02-04 21:26 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx



On 2/4/20 10:15 AM, Michal Wajdeczko wrote:
> On Tue, 04 Feb 2020 00:28:36 +0100, Daniele Ceraolo Spurio 
> <daniele.ceraolospurio@intel.com> wrote:
> 
>> Now that we can differentiate wants vs uses GuC/HuC, intel_uc_init is
>> restricted to running only if we have successfully fetched the required
>> blob(s) and are committed to using the microcontroller(s).
>> The only remaining thing that can go wrong in uc_init is the allocation
>> of GuC/HuC related objects; if we get such a failure better to bail out
>> immediately instead of wedging later, like we do for e.g.
>> intel_engines_init, since without objects we can't use the HW, including
>> not being able to attempt the firmware load.
>>
>> While at it, remove the unneeded fw_cleanup call (this is handled
>> outside of gt_init) and add a probe failure injection point for testing.
>> Also, update the logs for <g/h>uc_init failures to probe_failure() since
>> they will cause the driver load to fail.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: John Harrison <John.C.Harrison@Intel.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Reviewed-by: Fernando Pacheco <fernando.pacheco@intel.com>
>> ---
>>  drivers/gpu/drm/i915/gt/intel_gt.c     |  4 +++-
>>  drivers/gpu/drm/i915/gt/uc/intel_guc.c |  2 +-
>>  drivers/gpu/drm/i915/gt/uc/intel_huc.c |  2 +-
>>  drivers/gpu/drm/i915/gt/uc/intel_uc.c  | 24 +++++++++++++++++-------
>>  drivers/gpu/drm/i915/gt/uc/intel_uc.h  |  4 ++--
>>  5 files changed, 24 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
>> b/drivers/gpu/drm/i915/gt/intel_gt.c
>> index f1f1b306e0af..cd64f81a3e60 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
>> @@ -592,7 +592,9 @@ int intel_gt_init(struct intel_gt *gt)
>>      if (err)
>>          goto err_engines;
>> -    intel_uc_init(&gt->uc);
>> +    err = intel_uc_init(&gt->uc);
>> +    if (err)
>> +        goto err_engines;
>>     err = intel_gt_resume(gt);
>>      if (err)
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c 
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> index f96d1bdf4bf2..97b9c71a6fd4 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> @@ -376,7 +376,7 @@ int intel_guc_init(struct intel_guc *guc)
>>      intel_uc_fw_fini(&guc->fw);
>>  err_fetch:
>>      intel_uc_fw_cleanup_fetch(&guc->fw);
> 
> btw, we will cleanup all uc firmwares in i915_gem_driver_release()
> so maybe we don't need to cleanup it here
> 
>> -    DRM_DEV_DEBUG_DRIVER(gt->i915->drm.dev, "failed with %d\n", ret);
>> +    i915_probe_error(gt->i915, "failed with %d\n", ret);
>>      return ret;
>>  }
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c 
>> b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>> index 32a069841c14..5f448d0e360b 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>> @@ -127,7 +127,7 @@ int intel_huc_init(struct intel_huc *huc)
>>      intel_uc_fw_fini(&huc->fw);
>>  out:
>>      intel_uc_fw_cleanup_fetch(&huc->fw);
> 
> and here
> 
>> -    DRM_DEV_DEBUG_DRIVER(i915->drm.dev, "failed with %d\n", err);
>> +    i915_probe_error(i915, "failed with %d\n", err);
>>      return err;
>>  }
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c 
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> index 175fa6361ff2..a119b7776973 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> @@ -273,7 +273,7 @@ static void __uc_cleanup_firmwares(struct intel_uc 
>> *uc)
>>      intel_uc_fw_cleanup_fetch(&uc->guc.fw);
>>  }
>> -static void __uc_init(struct intel_uc *uc)
>> +static int __uc_init(struct intel_uc *uc)
>>  {
>>      struct intel_guc *guc = &uc->guc;
>>      struct intel_huc *huc = &uc->huc;
>> @@ -282,19 +282,29 @@ static void __uc_init(struct intel_uc *uc)
>>      GEM_BUG_ON(!intel_uc_wants_guc(uc));
>>     if (!intel_uc_uses_guc(uc))
>> -        return;
>> +        return 0;
>> +
>> +    if (i915_inject_probe_failure(uc_to_gt(uc)->i915))
>> +        return -ENOMEM;
>>     /* XXX: GuC submission is unavailable for now */
>>      GEM_BUG_ON(intel_uc_uses_guc_submission(uc));
>>     ret = intel_guc_init(guc);
>> -    if (ret) {
>> -        intel_uc_fw_cleanup_fetch(&huc->fw);
>> -        return;
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (intel_uc_uses_huc(uc)) {
>> +        ret = intel_huc_init(huc);
>> +        if (ret)
>> +            goto out_guc;
>>      }
>> -    if (intel_uc_uses_huc(uc))
>> -        intel_huc_init(huc);
>> +    return 0;
>> +
>> +out_guc:
>> +    intel_guc_fini(guc);
>> +    return ret;
>>  }
>> static void __uc_fini(struct intel_uc *uc)
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h 
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> index 35ce8a6be88b..480965504679 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> @@ -16,7 +16,7 @@ struct intel_uc_ops {
>>      int (*sanitize)(struct intel_uc *uc);
>>      void (*init_fw)(struct intel_uc *uc);
>>      void (*fini_fw)(struct intel_uc *uc);
>> -    void (*init)(struct intel_uc *uc);
>> +    int (*init)(struct intel_uc *uc);
>>      void (*fini)(struct intel_uc *uc);
>>      int (*init_hw)(struct intel_uc *uc);
>>      void (*fini_hw)(struct intel_uc *uc);
>> @@ -89,7 +89,7 @@ static inline _TYPE intel_uc_##_NAME(struct intel_uc 
>> *uc) \
>>  intel_uc_ops_function(sanitize, sanitize, int, 0);
>>  intel_uc_ops_function(fetch_firmwares, init_fw, void, );
>>  intel_uc_ops_function(cleanup_firmwares, fini_fw, void, );
>> -intel_uc_ops_function(init, init, void, );
>> +intel_uc_ops_function(init, init, int, 0);
>>  intel_uc_ops_function(fini, fini, void, );
>>  intel_uc_ops_function(init_hw, init_hw, int, 0);
>>  intel_uc_ops_function(fini_hw, fini_hw, void, );
> 
> with redundant (?) fw cleanups clarified,
> 
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>

The re-org of the fw_cleanups is done separately in the next patch, 
since it isn't as straightforward as just dropping the call. Is that 
enough for the r-b on this patch or do you want something more?

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

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

* Re: [Intel-gfx] [PATCH v2 01/10] drm/i915/debugfs: Pass guc_log struct to i915_guc_log_info
  2020-02-04 21:10     ` Daniele Ceraolo Spurio
@ 2020-02-04 23:06       ` Daniele Ceraolo Spurio
  2020-02-06 15:19         ` Michal Wajdeczko
  0 siblings, 1 reply; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2020-02-04 23:06 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx



On 2/4/20 1:10 PM, Daniele Ceraolo Spurio wrote:
> 
> 
> On 2/4/20 9:05 AM, Michal Wajdeczko wrote:
>> On Tue, 04 Feb 2020 00:28:29 +0100, Daniele Ceraolo Spurio 
>> <daniele.ceraolospurio@intel.com> wrote:
>>
>>> The log struct is the only thing the function needs (apart from
>>> the seq_file), so we can pass just that instead of the whole dev_priv.
>>>
>>> v2: Split this change to its own patch (Michal)
>>>
>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: John Harrison <John.C.Harrison@Intel.com>
>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_debugfs.c | 6 ++----
>>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index e75e8212f03b..7264c0ff766c 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -1753,10 +1753,8 @@ stringify_guc_log_type(enum 
>>> guc_log_buffer_type type)
>>>      return "";
>>>  }
>>> -static void i915_guc_log_info(struct seq_file *m,
>>> -                  struct drm_i915_private *dev_priv)
>>> +static void i915_guc_log_info(struct seq_file *m, struct 
>>> intel_guc_log *log)
>>
>> maybe to avoid polluting i915_debugfs.c we should move this function to
>> gt/uc/intel_guc_log.c as universal printer:
>>
>> void intel_guc_log_info(struct intel_guc_log *log, struct drm_printer *p)
>>
> 
> ok
> 

I've started looking at this and then I noticed that other guc debugfs 
files can be moved as well and possibly squashed together, so IMO it'd 
be better to change them all in one go. Since such a rework doesn't 
belong in this series, do you mind if I keep this patch as-is and 
follow-up with the debugfs cleanup after this is merged?

Daniele

>>>  {
>>> -    struct intel_guc_log *log = &dev_priv->gt.uc.guc.log;
>>>      enum guc_log_buffer_type type;
>>>     if (!intel_guc_log_relay_created(log)) {
>>> @@ -1784,7 +1782,7 @@ static int i915_guc_info(struct seq_file *m, 
>>> void *data)
>>>      if (!USES_GUC(dev_priv))
>>>          return -ENODEV;
>>> -    i915_guc_log_info(m, dev_priv);
>>> +    i915_guc_log_info(m, &dev_priv->gt.uc.guc.log);
>>
>> too many dots ... this is "guc" info function, maybe we should have:
> 
> This is changed in the next patch to use intel_uc. It made more sense to 
> me to keep that change in the patch that introduced a second use for the 
> variable.
> 
> Daniele
> 
>>
>>      struct intel_guc *guc = &dev_priv->gt.uc.guc;
>> or
>>      struct intel_gt *gt = &dev_priv->gt;
>>      struct intel_uc *uc = &gt->uc;
>>      struct intel_guc *guc = &uc->guc;
>>
>> as that "guc" is likely to be reused in "more" below
>>
>>>     /* Add more as required ... */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v2 01/10] drm/i915/debugfs: Pass guc_log struct to i915_guc_log_info
  2020-02-04 17:05   ` Michal Wajdeczko
  2020-02-04 21:10     ` Daniele Ceraolo Spurio
@ 2020-02-05  7:58     ` Jani Nikula
  2020-02-05 20:37       ` Daniele Ceraolo Spurio
  1 sibling, 1 reply; 30+ messages in thread
From: Jani Nikula @ 2020-02-05  7:58 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx, Daniele Ceraolo Spurio

On Tue, 04 Feb 2020, "Michal Wajdeczko" <michal.wajdeczko@intel.com> wrote:
> On Tue, 04 Feb 2020 00:28:29 +0100, Daniele Ceraolo Spurio  
> <daniele.ceraolospurio@intel.com> wrote:
>
>> The log struct is the only thing the function needs (apart from
>> the seq_file), so we can pass just that instead of the whole dev_priv.
>>
>> v2: Split this change to its own patch (Michal)
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: John Harrison <John.C.Harrison@Intel.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c  
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index e75e8212f03b..7264c0ff766c 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1753,10 +1753,8 @@ stringify_guc_log_type(enum guc_log_buffer_type  
>> type)
>>  	return "";
>>  }
>> -static void i915_guc_log_info(struct seq_file *m,
>> -			      struct drm_i915_private *dev_priv)
>> +static void i915_guc_log_info(struct seq_file *m, struct intel_guc_log  
>> *log)
>
> maybe to avoid polluting i915_debugfs.c we should move this function to
> gt/uc/intel_guc_log.c as universal printer:

On that note, I'm going to split display bits from i915_debugfs.c to a
file of its own under display/ [1]. I think there's enough guc/huc stuff
to warrant moving them to a separate file maybe under gt/uc/.


BR,
Jani.

[1] http://patchwork.freedesktop.org/patch/msgid/20200204151810.8189-1-jani.nikula@intel.com


>
> void intel_guc_log_info(struct intel_guc_log *log, struct drm_printer *p)
>
>>  {
>> -	struct intel_guc_log *log = &dev_priv->gt.uc.guc.log;
>>  	enum guc_log_buffer_type type;
>> 	if (!intel_guc_log_relay_created(log)) {
>> @@ -1784,7 +1782,7 @@ static int i915_guc_info(struct seq_file *m, void  
>> *data)
>>  	if (!USES_GUC(dev_priv))
>>  		return -ENODEV;
>> -	i915_guc_log_info(m, dev_priv);
>> +	i915_guc_log_info(m, &dev_priv->gt.uc.guc.log);
>
> too many dots ... this is "guc" info function, maybe we should have:
>
> 	struct intel_guc *guc = &dev_priv->gt.uc.guc;
> or
> 	struct intel_gt *gt = &dev_priv->gt;
> 	struct intel_uc *uc = &gt->uc;
> 	struct intel_guc *guc = &uc->guc;
>
> as that "guc" is likely to be reused in "more" below
>
>> 	/* Add more as required ... */
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v2 01/10] drm/i915/debugfs: Pass guc_log struct to i915_guc_log_info
  2020-02-05  7:58     ` Jani Nikula
@ 2020-02-05 20:37       ` Daniele Ceraolo Spurio
  0 siblings, 0 replies; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2020-02-05 20:37 UTC (permalink / raw)
  To: Jani Nikula, Michal Wajdeczko, intel-gfx, Shyti, Andi



On 2/4/20 11:58 PM, Jani Nikula wrote:
> On Tue, 04 Feb 2020, "Michal Wajdeczko" <michal.wajdeczko@intel.com> wrote:
>> On Tue, 04 Feb 2020 00:28:29 +0100, Daniele Ceraolo Spurio
>> <daniele.ceraolospurio@intel.com> wrote:
>>
>>> The log struct is the only thing the function needs (apart from
>>> the seq_file), so we can pass just that instead of the whole dev_priv.
>>>
>>> v2: Split this change to its own patch (Michal)
>>>
>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: John Harrison <John.C.Harrison@Intel.com>
>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_debugfs.c | 6 ++----
>>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index e75e8212f03b..7264c0ff766c 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -1753,10 +1753,8 @@ stringify_guc_log_type(enum guc_log_buffer_type
>>> type)
>>>   	return "";
>>>   }
>>> -static void i915_guc_log_info(struct seq_file *m,
>>> -			      struct drm_i915_private *dev_priv)
>>> +static void i915_guc_log_info(struct seq_file *m, struct intel_guc_log
>>> *log)
>>
>> maybe to avoid polluting i915_debugfs.c we should move this function to
>> gt/uc/intel_guc_log.c as universal printer:
> 
> On that note, I'm going to split display bits from i915_debugfs.c to a
> file of its own under display/ [1]. I think there's enough guc/huc stuff
> to warrant moving them to a separate file maybe under gt/uc/.

Andi is already looking at moving all the GT bits under gt/, so the uc 
functions will move as part of that.

Daniele

> 
> 
> BR,
> Jani.
> 
> [1] http://patchwork.freedesktop.org/patch/msgid/20200204151810.8189-1-jani.nikula@intel.com
> 
> 
>>
>> void intel_guc_log_info(struct intel_guc_log *log, struct drm_printer *p)
>>
>>>   {
>>> -	struct intel_guc_log *log = &dev_priv->gt.uc.guc.log;
>>>   	enum guc_log_buffer_type type;
>>> 	if (!intel_guc_log_relay_created(log)) {
>>> @@ -1784,7 +1782,7 @@ static int i915_guc_info(struct seq_file *m, void
>>> *data)
>>>   	if (!USES_GUC(dev_priv))
>>>   		return -ENODEV;
>>> -	i915_guc_log_info(m, dev_priv);
>>> +	i915_guc_log_info(m, &dev_priv->gt.uc.guc.log);
>>
>> too many dots ... this is "guc" info function, maybe we should have:
>>
>> 	struct intel_guc *guc = &dev_priv->gt.uc.guc;
>> or
>> 	struct intel_gt *gt = &dev_priv->gt;
>> 	struct intel_uc *uc = &gt->uc;
>> 	struct intel_guc *guc = &uc->guc;
>>
>> as that "guc" is likely to be reused in "more" below
>>
>>> 	/* Add more as required ... */
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v2 01/10] drm/i915/debugfs: Pass guc_log struct to i915_guc_log_info
  2020-02-04 23:06       ` Daniele Ceraolo Spurio
@ 2020-02-06 15:19         ` Michal Wajdeczko
  0 siblings, 0 replies; 30+ messages in thread
From: Michal Wajdeczko @ 2020-02-06 15:19 UTC (permalink / raw)
  To: intel-gfx, Daniele Ceraolo Spurio

>
> I've started looking at this and then I noticed that other guc debugfs  
> files can be moved as well and possibly squashed together, so IMO it'd  
> be better to change them all in one go. Since such a rework doesn't  
> belong in this series, do you mind if I keep this patch as-is and  
> follow-up with the debugfs cleanup after this is merged?

sure, with promise that there will be follow-up series,

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>

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

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

* Re: [Intel-gfx] [PATCH v2 08/10] drm/i915/uc: Abort early on uc_init failure
  2020-02-04 21:26     ` Daniele Ceraolo Spurio
@ 2020-02-06 15:33       ` Michal Wajdeczko
  0 siblings, 0 replies; 30+ messages in thread
From: Michal Wajdeczko @ 2020-02-06 15:33 UTC (permalink / raw)
  To: intel-gfx, Daniele Ceraolo Spurio

>>  with redundant (?) fw cleanups clarified,
>>  Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>
> The re-org of the fw_cleanups is done separately in the next patch,  
> since it isn't as straightforward as just dropping the call. Is that  
> enough for the r-b on this patch or do you want something more?

somehow I missed this "next patch", above r-b stands
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH v2 09/10] drm/i915/uc: consolidate firmware cleanup
  2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 09/10] drm/i915/uc: consolidate firmware cleanup Daniele Ceraolo Spurio
@ 2020-02-06 19:09   ` Michal Wajdeczko
  0 siblings, 0 replies; 30+ messages in thread
From: Michal Wajdeczko @ 2020-02-06 19:09 UTC (permalink / raw)
  To: intel-gfx, Daniele Ceraolo Spurio

On Tue, 04 Feb 2020 00:28:37 +0100, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@intel.com> wrote:

> We are quite trigger happy in cleaning up the firmware blobs, as we do
> so from several error/fini paths in GuC/HuC/uC code. We do have the
> __uc_cleanup_firmwares cleanup function, which unwinds
> __uc_fetch_firmwares and is already called both from the error path of
> gem_init and from gem_driver_release, so let's stop cleaning up from
> all the other paths.
>
> The fact that we're not cleaning the firmware immediately means that
> we can't consider firmware availability as an indication of
> initialization success. A "READY_TO_LOAD" status has been added to

what about s/READY_TO_LOAD/LOADABLE ?

> indicate that the initialization was successful, to be used to
> selectively load HuC only if HuC init has completed (HuC init failure
> is not considered a fatal error).
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc.c   | 10 ++++------
>  drivers/gpu/drm/i915/gt/uc/intel_huc.c   |  3 ++-
>  drivers/gpu/drm/i915/gt/uc/intel_uc.c    |  2 +-
>  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c |  7 +++++--
>  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 18 +++++++++++++++---
>  5 files changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> index 97b9c71a6fd4..2d05de570bdf 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> @@ -333,7 +333,7 @@ int intel_guc_init(struct intel_guc *guc)
> 	ret = intel_uc_fw_init(&guc->fw);
>  	if (ret)
> -		goto err_fetch;
> +		goto out;
> 	ret = intel_guc_log_create(&guc->log);
>  	if (ret)
> @@ -364,6 +364,8 @@ int intel_guc_init(struct intel_guc *guc)
>  	/* We need to notify the guc whenever we change the GGTT */
>  	i915_ggtt_enable_guc(gt->ggtt);
> +	intel_uc_fw_change_status(&guc->fw, INTEL_UC_FIRMWARE_READY_TO_LOAD);
> +
>  	return 0;
> err_ct:
> @@ -374,8 +376,7 @@ int intel_guc_init(struct intel_guc *guc)
>  	intel_guc_log_destroy(&guc->log);
>  err_fw:
>  	intel_uc_fw_fini(&guc->fw);
> -err_fetch:
> -	intel_uc_fw_cleanup_fetch(&guc->fw);
> +out:
>  	i915_probe_error(gt->i915, "failed with %d\n", ret);
>  	return ret;
>  }
> @@ -397,9 +398,6 @@ void intel_guc_fini(struct intel_guc *guc)
>  	intel_guc_ads_destroy(guc);
>  	intel_guc_log_destroy(&guc->log);
>  	intel_uc_fw_fini(&guc->fw);
> -	intel_uc_fw_cleanup_fetch(&guc->fw);
> -
> -	intel_uc_fw_change_status(&guc->fw, INTEL_UC_FIRMWARE_DISABLED);
>  }
> /*
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> index 5f448d0e360b..2e4f4a8e791e 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> @@ -121,12 +121,13 @@ int intel_huc_init(struct intel_huc *huc)
>  	if (err)
>  		goto out_fini;
> +	intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_READY_TO_LOAD);
> +
>  	return 0;
> out_fini:
>  	intel_uc_fw_fini(&huc->fw);
>  out:
> -	intel_uc_fw_cleanup_fetch(&huc->fw);
>  	i915_probe_error(i915, "failed with %d\n", err);
>  	return err;
>  }
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index a119b7776973..4c55b1285cbc 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -417,7 +417,7 @@ static int __uc_init_hw(struct intel_uc *uc)
>  	GEM_BUG_ON(!intel_uc_supports_guc(uc));
>  	GEM_BUG_ON(!intel_uc_wants_guc(uc));
> -	if (!intel_uc_fw_is_available(&guc->fw)) {
> +	if (!intel_uc_fw_is_ready_to_load(&guc->fw)) {
>  		ret = __uc_check_hw(uc) ||
>  		      intel_uc_fw_is_overridden(&guc->fw) ||
>  		      intel_uc_wants_guc_submission(uc) ?
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> index c9c094a73399..3759569ec000 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -501,7 +501,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,  
> u32 dst_offset, u32 dma_flags)
>  	if (err)
>  		return err;
> -	if (!intel_uc_fw_is_available(uc_fw))
> +	if (!intel_uc_fw_is_ready_to_load(uc_fw))
>  		return -ENOEXEC;
> 	/* Call custom loader */
> @@ -544,7 +544,10 @@ int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
> void intel_uc_fw_fini(struct intel_uc_fw *uc_fw)
>  {
> -	intel_uc_fw_cleanup_fetch(uc_fw);
> +	if (i915_gem_object_has_pinned_pages(uc_fw->obj))
> +		i915_gem_object_unpin_pages(uc_fw->obj);
> +
> +	intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_AVAILABLE);
>  }
> /**
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h  
> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> index 1f30543d0d2d..ba3c362aeca2 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> @@ -29,8 +29,11 @@ struct intel_gt;
>   * |            |                 SELECTED                          |
>   * +------------+-               /   |   \                         -+
>   * |            |    MISSING <--/    |    \--> ERROR                |
> - * |   fetch    |                    |                              |
> - * |            |        /------> AVAILABLE <---<-----------\       |
> + * |   fetch    |                    V                              |
> + * |            |                 AVAILABLE                         |
> + * |            |                    |                              |

as we change status during "init" phase, we should also mark it here

       +------------+-                                                 -+
       |   init     |                                                   |

> + * |            |                    V                              |
> + * |            |        /----> READY TO LOAD <---<---------\       |
>   * +------------+-       \         /    \        \           \     -+
>   * |            |         FAIL <--<      \--> TRANSFERRED     \     |
>   * |   upload   |                  \           /   \          /     |
> @@ -46,6 +49,7 @@ enum intel_uc_fw_status {
>  	INTEL_UC_FIRMWARE_MISSING, /* blob not found on the system */
>  	INTEL_UC_FIRMWARE_ERROR, /* invalid format or version */
>  	INTEL_UC_FIRMWARE_AVAILABLE, /* blob found and copied in mem */
> +	INTEL_UC_FIRMWARE_READY_TO_LOAD, /* all fw-required objects are ready  
> */
>  	INTEL_UC_FIRMWARE_FAIL, /* failed to xfer or init/auth the fw */
>  	INTEL_UC_FIRMWARE_TRANSFERRED, /* dma xfer done */
>  	INTEL_UC_FIRMWARE_RUNNING /* init/auth done */
> @@ -115,6 +119,8 @@ const char *intel_uc_fw_status_repr(enum  
> intel_uc_fw_status status)
>  		return "ERROR";
>  	case INTEL_UC_FIRMWARE_AVAILABLE:
>  		return "AVAILABLE";
> +	case INTEL_UC_FIRMWARE_READY_TO_LOAD:
> +		return "READY TO LOAD";
>  	case INTEL_UC_FIRMWARE_FAIL:
>  		return "FAIL";
>  	case INTEL_UC_FIRMWARE_TRANSFERRED:
> @@ -143,6 +149,7 @@ static inline int intel_uc_fw_status_to_error(enum  
> intel_uc_fw_status status)
>  	case INTEL_UC_FIRMWARE_SELECTED:
>  		return -ESTALE;
>  	case INTEL_UC_FIRMWARE_AVAILABLE:
> +	case INTEL_UC_FIRMWARE_READY_TO_LOAD:
>  	case INTEL_UC_FIRMWARE_TRANSFERRED:
>  	case INTEL_UC_FIRMWARE_RUNNING:
>  		return 0;
> @@ -184,6 +191,11 @@ static inline bool intel_uc_fw_is_available(struct  
> intel_uc_fw *uc_fw)
>  	return __intel_uc_fw_status(uc_fw) >= INTEL_UC_FIRMWARE_AVAILABLE;
>  }
> +static inline bool intel_uc_fw_is_ready_to_load(struct intel_uc_fw  
> *uc_fw)
> +{
> +	return __intel_uc_fw_status(uc_fw) == INTEL_UC_FIRMWARE_READY_TO_LOAD;
> +}
> +
>  static inline bool intel_uc_fw_is_loaded(struct intel_uc_fw *uc_fw)
>  {
>  	return __intel_uc_fw_status(uc_fw) >= INTEL_UC_FIRMWARE_TRANSFERRED;
> @@ -202,7 +214,7 @@ static inline bool intel_uc_fw_is_overridden(const  
> struct intel_uc_fw *uc_fw)
>  static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
>  {
>  	if (intel_uc_fw_is_loaded(uc_fw))
> -		intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_AVAILABLE);
> +		intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_READY_TO_LOAD);
>  }
> static inline u32 __intel_uc_fw_get_upload_size(struct intel_uc_fw  
> *uc_fw)

in the future we should never try to hide intermediate states,
with above nits,

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* [Intel-gfx] ✓ Fi.CI.IGT: success for Commit early to GuC (rev2)
  2020-02-03 23:28 [Intel-gfx] [PATCH v2 00/10] Commit early to GuC Daniele Ceraolo Spurio
                   ` (11 preceding siblings ...)
  2020-02-04 21:23 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
@ 2020-02-06 21:15 ` Patchwork
  12 siblings, 0 replies; 30+ messages in thread
From: Patchwork @ 2020-02-06 21:15 UTC (permalink / raw)
  To: Daniele Ceraolo Spurio; +Cc: intel-gfx

== Series Details ==

Series: Commit early to GuC (rev2)
URL   : https://patchwork.freedesktop.org/series/72031/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_7864_full -> Patchwork_16406_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_shared@exec-shared-gtt-blt:
    - shard-tglb:         [PASS][1] -> [FAIL][2] ([i915#616])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-tglb5/igt@gem_ctx_shared@exec-shared-gtt-blt.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-tglb7/igt@gem_ctx_shared@exec-shared-gtt-blt.html

  * igt@gem_eio@in-flight-contexts-1us:
    - shard-hsw:          [PASS][3] -> [INCOMPLETE][4] ([CI#80] / [i915#61])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-hsw7/igt@gem_eio@in-flight-contexts-1us.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-hsw2/igt@gem_eio@in-flight-contexts-1us.html

  * igt@gem_exec_schedule@in-order-bsd:
    - shard-iclb:         [PASS][5] -> [SKIP][6] ([fdo#112146]) +2 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-iclb3/igt@gem_exec_schedule@in-order-bsd.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-iclb1/igt@gem_exec_schedule@in-order-bsd.html

  * igt@gem_exec_schedule@out-order-bsd2:
    - shard-iclb:         [PASS][7] -> [SKIP][8] ([fdo#109276]) +13 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-iclb1/igt@gem_exec_schedule@out-order-bsd2.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-iclb8/igt@gem_exec_schedule@out-order-bsd2.html

  * igt@gem_exec_schedule@pi-userfault-bsd:
    - shard-iclb:         [PASS][9] -> [SKIP][10] ([i915#677])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-iclb3/igt@gem_exec_schedule@pi-userfault-bsd.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-iclb1/igt@gem_exec_schedule@pi-userfault-bsd.html

  * igt@gem_ppgtt@flink-and-close-vma-leak:
    - shard-glk:          [PASS][11] -> [FAIL][12] ([i915#644])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-glk1/igt@gem_ppgtt@flink-and-close-vma-leak.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-glk9/igt@gem_ppgtt@flink-and-close-vma-leak.html
    - shard-apl:          [PASS][13] -> [FAIL][14] ([i915#644])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-apl6/igt@gem_ppgtt@flink-and-close-vma-leak.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-apl3/igt@gem_ppgtt@flink-and-close-vma-leak.html
    - shard-tglb:         [PASS][15] -> [FAIL][16] ([i915#644])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-tglb8/igt@gem_ppgtt@flink-and-close-vma-leak.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-tglb2/igt@gem_ppgtt@flink-and-close-vma-leak.html

  * igt@i915_pm_rpm@system-suspend:
    - shard-kbl:          [PASS][17] -> [INCOMPLETE][18] ([fdo#103665] / [i915#151]) +3 similar issues
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-kbl6/igt@i915_pm_rpm@system-suspend.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-kbl2/igt@i915_pm_rpm@system-suspend.html

  * igt@i915_pm_rpm@system-suspend-devices:
    - shard-iclb:         [PASS][19] -> [INCOMPLETE][20] ([i915#189]) +2 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-iclb1/igt@i915_pm_rpm@system-suspend-devices.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-iclb7/igt@i915_pm_rpm@system-suspend-devices.html

  * igt@i915_pm_rpm@system-suspend-execbuf:
    - shard-skl:          [PASS][21] -> [INCOMPLETE][22] ([i915#151] / [i915#69]) +1 similar issue
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-skl7/igt@i915_pm_rpm@system-suspend-execbuf.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-skl4/igt@i915_pm_rpm@system-suspend-execbuf.html

  * igt@i915_pm_rpm@system-suspend-modeset:
    - shard-glk:          [PASS][23] -> [INCOMPLETE][24] ([i915#58] / [k.org#198133]) +3 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-glk6/igt@i915_pm_rpm@system-suspend-modeset.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-glk9/igt@i915_pm_rpm@system-suspend-modeset.html
    - shard-apl:          [PASS][25] -> [INCOMPLETE][26] ([fdo#103927]) +3 similar issues
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-apl6/igt@i915_pm_rpm@system-suspend-modeset.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-apl8/igt@i915_pm_rpm@system-suspend-modeset.html

  * igt@i915_suspend@debugfs-reader:
    - shard-skl:          [PASS][27] -> [INCOMPLETE][28] ([i915#69])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-skl8/igt@i915_suspend@debugfs-reader.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-skl6/igt@i915_suspend@debugfs-reader.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions:
    - shard-skl:          [PASS][29] -> [FAIL][30] ([IGT#5])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-skl7/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-skl4/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-skl:          [PASS][31] -> [FAIL][32] ([i915#46])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-skl1/igt@kms_flip@flip-vs-expired-vblank.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-skl10/igt@kms_flip@flip-vs-expired-vblank.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c-frame-sequence:
    - shard-skl:          [PASS][33] -> [FAIL][34] ([i915#53])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-skl7/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c-frame-sequence.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-skl10/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c-frame-sequence.html

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-skl:          [PASS][35] -> [FAIL][36] ([fdo#108145] / [i915#265])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-skl5/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-skl4/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html

  * igt@kms_psr2_su@page_flip:
    - shard-iclb:         [PASS][37] -> [SKIP][38] ([fdo#109642] / [fdo#111068])
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-iclb2/igt@kms_psr2_su@page_flip.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-iclb6/igt@kms_psr2_su@page_flip.html

  * igt@kms_psr@psr2_primary_page_flip:
    - shard-iclb:         [PASS][39] -> [SKIP][40] ([fdo#109441]) +1 similar issue
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-iclb2/igt@kms_psr@psr2_primary_page_flip.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-iclb6/igt@kms_psr@psr2_primary_page_flip.html

  * igt@perf_pmu@busy-check-all-vcs1:
    - shard-iclb:         [PASS][41] -> [SKIP][42] ([fdo#112080]) +8 similar issues
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-iclb1/igt@perf_pmu@busy-check-all-vcs1.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-iclb8/igt@perf_pmu@busy-check-all-vcs1.html

  
#### Possible fixes ####

  * igt@gem_exec_schedule@pi-distinct-iova-bsd:
    - shard-iclb:         [SKIP][43] ([i915#677]) -> [PASS][44] +1 similar issue
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-iclb1/igt@gem_exec_schedule@pi-distinct-iova-bsd.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-iclb8/igt@gem_exec_schedule@pi-distinct-iova-bsd.html

  * igt@gem_exec_schedule@preempt-other-chain-bsd:
    - shard-iclb:         [SKIP][45] ([fdo#112146]) -> [PASS][46] +3 similar issues
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-iclb2/igt@gem_exec_schedule@preempt-other-chain-bsd.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-iclb6/igt@gem_exec_schedule@preempt-other-chain-bsd.html

  * igt@gem_partial_pwrite_pread@writes-after-reads-display:
    - shard-hsw:          [FAIL][47] ([i915#694]) -> [PASS][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-hsw7/igt@gem_partial_pwrite_pread@writes-after-reads-display.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-hsw2/igt@gem_partial_pwrite_pread@writes-after-reads-display.html

  * igt@gem_softpin@noreloc-s3:
    - shard-apl:          [DMESG-WARN][49] ([i915#180]) -> [PASS][50] +2 similar issues
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-apl6/igt@gem_softpin@noreloc-s3.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-apl6/igt@gem_softpin@noreloc-s3.html

  * igt@i915_pm_dc@dc6-psr:
    - shard-skl:          [FAIL][51] ([i915#454]) -> [PASS][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-skl8/igt@i915_pm_dc@dc6-psr.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-skl6/igt@i915_pm_dc@dc6-psr.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-kbl:          [DMESG-WARN][53] ([i915#180]) -> [PASS][54] +5 similar issues
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-kbl3/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-kbl6/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_flip@2x-flip-vs-expired-vblank:
    - shard-glk:          [FAIL][55] ([i915#79]) -> [PASS][56]
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-glk1/igt@kms_flip@2x-flip-vs-expired-vblank.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-glk9/igt@kms_flip@2x-flip-vs-expired-vblank.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-skl:          [FAIL][57] ([i915#79]) -> [PASS][58]
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-skl6/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-skl7/igt@kms_flip@flip-vs-expired-vblank-interruptible.html

  * igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
    - shard-skl:          [FAIL][59] ([fdo#108145]) -> [PASS][60]
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-skl10/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-skl6/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html

  * igt@kms_plane_lowres@pipe-a-tiling-y:
    - shard-glk:          [FAIL][61] ([i915#899]) -> [PASS][62]
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-glk8/igt@kms_plane_lowres@pipe-a-tiling-y.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-glk8/igt@kms_plane_lowres@pipe-a-tiling-y.html

  * igt@kms_psr@psr2_sprite_plane_move:
    - shard-iclb:         [SKIP][63] ([fdo#109441]) -> [PASS][64]
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-iclb7/igt@kms_psr@psr2_sprite_plane_move.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-iclb2/igt@kms_psr@psr2_sprite_plane_move.html

  * igt@kms_setmode@basic:
    - shard-apl:          [FAIL][65] ([i915#31]) -> [PASS][66]
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-apl2/igt@kms_setmode@basic.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-apl7/igt@kms_setmode@basic.html

  * igt@perf_pmu@busy-no-semaphores-vcs1:
    - shard-iclb:         [SKIP][67] ([fdo#112080]) -> [PASS][68] +8 similar issues
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-iclb7/igt@perf_pmu@busy-no-semaphores-vcs1.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-iclb1/igt@perf_pmu@busy-no-semaphores-vcs1.html

  * igt@prime_busy@hang-bsd2:
    - shard-iclb:         [SKIP][69] ([fdo#109276]) -> [PASS][70] +19 similar issues
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-iclb7/igt@prime_busy@hang-bsd2.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-iclb1/igt@prime_busy@hang-bsd2.html

  * igt@prime_mmap_coherency@ioctl-errors:
    - shard-hsw:          [FAIL][71] ([i915#831]) -> [PASS][72]
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-hsw5/igt@prime_mmap_coherency@ioctl-errors.html
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-hsw5/igt@prime_mmap_coherency@ioctl-errors.html

  
#### Warnings ####

  * igt@i915_selftest@live_blt:
    - shard-hsw:          [DMESG-FAIL][73] ([i915#725]) -> [DMESG-FAIL][74] ([i915#553] / [i915#725])
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7864/shard-hsw7/igt@i915_selftest@live_blt.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16406/shard-hsw1/igt@i915_selftest@live_blt.html

  
  [CI#80]: https://gitlab.freedesktop.org/gfx-ci/i915-infra/issues/80
  [IGT#5]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/5
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#112080]: https://bugs.freedesktop.org/show_bug.cgi?id=112080
  [fdo#112146]: https://bugs.freedesktop.org/show_bug.cgi?id=112146
  [i915#151]: https://gitlab.freedesktop.org/drm/intel/issues/151
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#189]: https://gitlab.freedesktop.org/drm/intel/issues/189
  [i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
  [i915#31]: https://gitlab.freedesktop.org/drm/intel/issues/31
  [i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
  [i915#46]: https://gitlab.freedesktop.org/drm/intel/issues/46
  [i915#53]: https://gitlab.freedesktop.org/drm/intel/issues/53
  [i915#553]: https://gitlab.freedesktop.org/drm/intel/issues/553
  [i915#58]: https://gitlab.freedesktop.org/drm/intel/issues/58
  [i915#61]: https://gitlab.freedesktop.org/drm/intel/issues/61
  [i915#616]: https://gitlab.freedesktop.org/drm/intel/issues/616
  [i915#644]: https://gitlab.freedesktop.org/drm/intel/issues/644
  [i915#677]: https://gitlab.freedesktop.org/drm/intel/issues/677
  [i915#69]: https://gitlab.freedesktop.org/drm/intel/issues/69
  [i915#694]: https://gitlab.freedesktop.org/drm/intel/issues/694
  [i915#725]: https://gitlab.freedesktop.org/drm/intel/issues/725
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#831]: https://gitlab.freedesktop.org/drm/intel/issues/831
  [i915#899]: https://gitlab.freedesktop.org/drm/intel/issues/899
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7864 -> Patchwork_16406

  CI-20190529: 20190529
  CI_DRM_7864: 5a140e2fc771e4c8b10d14e2db7bfb4996ee9d8a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5417: 33cc93c8ba5daa0b7498f297a4f626844d895d06 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_16406: e4ad6a295fb15116c45e09ae490589bb06ad736d @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

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

end of thread, other threads:[~2020-02-06 21:15 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-03 23:28 [Intel-gfx] [PATCH v2 00/10] Commit early to GuC Daniele Ceraolo Spurio
2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 01/10] drm/i915/debugfs: Pass guc_log struct to i915_guc_log_info Daniele Ceraolo Spurio
2020-02-04 17:05   ` Michal Wajdeczko
2020-02-04 21:10     ` Daniele Ceraolo Spurio
2020-02-04 23:06       ` Daniele Ceraolo Spurio
2020-02-06 15:19         ` Michal Wajdeczko
2020-02-05  7:58     ` Jani Nikula
2020-02-05 20:37       ` Daniele Ceraolo Spurio
2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 02/10] drm/i915/guc: Kill USES_GUC macro Daniele Ceraolo Spurio
2020-02-04 17:18   ` Michal Wajdeczko
2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 03/10] drm/i915/guc: Kill USES_GUC_SUBMISSION macro Daniele Ceraolo Spurio
2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 04/10] drm/i915/uc: Update the FW status on injected fetch error Daniele Ceraolo Spurio
2020-02-04 17:20   ` Michal Wajdeczko
2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 05/10] drm/i915/uc: autogenerate uC checker functions Daniele Ceraolo Spurio
2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 06/10] drm/i915/uc: Improve tracking of uC init status Daniele Ceraolo Spurio
2020-02-04 17:54   ` Michal Wajdeczko
2020-02-04 21:19     ` Daniele Ceraolo Spurio
2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 07/10] drm/i915/guc: Apply new uC status tracking to GuC submission as well Daniele Ceraolo Spurio
2020-02-04 18:06   ` Michal Wajdeczko
2020-02-04 21:24     ` Daniele Ceraolo Spurio
2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 08/10] drm/i915/uc: Abort early on uc_init failure Daniele Ceraolo Spurio
2020-02-04 18:15   ` Michal Wajdeczko
2020-02-04 21:26     ` Daniele Ceraolo Spurio
2020-02-06 15:33       ` Michal Wajdeczko
2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 09/10] drm/i915/uc: consolidate firmware cleanup Daniele Ceraolo Spurio
2020-02-06 19:09   ` Michal Wajdeczko
2020-02-03 23:28 ` [Intel-gfx] [PATCH v2 10/10] HAX: drm/i915: default to enable_guc=2 Daniele Ceraolo Spurio
2020-02-04 21:02 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Commit early to GuC (rev2) Patchwork
2020-02-04 21:23 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-02-06 21:15 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork

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.