[Intel-gfx] [PATCH v3 00/10] Commit early to GuC

[Intel-gfx] [PATCH v3 00/10] Commit early to GuC

[Daniele Ceraolo Spurio]

Addressed review comments on v2.

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        | 26 ++++----
 drivers/gpu/drm/i915/gt/uc/intel_guc.h        | 15 ++---
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  9 +--
 .../gpu/drm/i915/gt/uc/intel_guc_submission.h | 19 +++++-
 drivers/gpu/drm/i915/gt/uc/intel_huc.c        |  7 ++-
 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         | 59 +++++++++++-------
 drivers/gpu/drm/i915/gt/uc/intel_uc.h         | 62 +++++++++++--------
 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 +-
 22 files changed, 176 insertions(+), 124 deletions(-)

-- 
2.24.1

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

[Intel-gfx] [PATCH v3 01/10] drm/i915/debugfs: Pass guc_log struct to i915_guc_log_info

[Daniele Ceraolo Spurio]

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>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@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 3cae18d1d20c..bafab32318bd 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

[Intel-gfx] [PATCH v3 02/10] drm/i915/guc: Kill USES_GUC macro

[Daniele Ceraolo Spurio]

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>
---
 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 7204e1c3de49..41a00281f364 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 bafab32318bd..ea242d16f1a3 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 da509d9b8895..ef2592035277 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1694,7 +1694,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

[Intel-gfx] [PATCH v3 03/10] drm/i915/guc: Kill USES_GUC_SUBMISSION macro

[Daniele Ceraolo Spurio]

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 cfaf5bbdbcab..04b0ef83b90b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1357,7 +1357,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 7ef68500b2bd..0fc907b74daa 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))
@@ -2923,7 +2923,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) {
@@ -3056,7 +3056,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++) {
@@ -3198,7 +3198,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. */
@@ -3422,7 +3422,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++) {
@@ -3583,7 +3583,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 ea242d16f1a3..47d8c6df9937 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 ef2592035277..23ecaf75597f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1693,9 +1693,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

[Intel-gfx] [PATCH v3 04/10] drm/i915/uc: Update the FW status on injected fetch error

[Daniele Ceraolo Spurio]

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);
-- 
2.24.1

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

[Intel-gfx] [PATCH v3 05/10] drm/i915/uc: autogenerate uC checker functions

[Daniele Ceraolo Spurio]

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

[Intel-gfx] [PATCH v3 06/10] drm/i915/uc: Improve tracking of uC init status

[Daniele Ceraolo Spurio]

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)

v3: s/wants_huc/uses_huc in uc_init_wopcm

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     | 21 +++++++++++---------
 drivers/gpu/drm/i915/gt/uc/intel_uc.h     | 24 ++++++++++++++++++++++-
 5 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index 668b067b71e2..b51adaac8752 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..5825a6c3e0a0 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));
@@ -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

[Intel-gfx] [PATCH v3 07/10] drm/i915/guc: Apply new uC status tracking to GuC submission as well

[Daniele Ceraolo Spurio]

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.

v2: move functions to guc_submission.h (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_guc.c        | 12 ++++----
 drivers/gpu/drm/i915/gt/uc/intel_guc.h        |  7 +----
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  9 ++----
 .../gpu/drm/i915/gt/uc/intel_guc_submission.h | 19 +++++++++++-
 drivers/gpu/drm/i915/gt/uc/intel_uc.c         | 14 ++++-----
 drivers/gpu/drm/i915/gt/uc/intel_uc.h         | 30 +++++++------------
 drivers/gpu/drm/i915/gvt/scheduler.c          |  2 +-
 drivers/gpu/drm/i915/i915_drv.h               |  6 ----
 drivers/gpu/drm/i915/intel_gvt.c              |  2 +-
 9 files changed, 48 insertions(+), 53 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 b51adaac8752..4594ccbeaa34 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;
@@ -173,11 +173,6 @@ static inline int intel_guc_sanitize(struct intel_guc *guc)
 	return 0;
 }
 
-static inline bool intel_guc_is_submission_supported(struct intel_guc *guc)
-{
-	return guc->submission_supported;
-}
-
 static inline void intel_guc_enable_msg(struct intel_guc *guc, u32 mask)
 {
 	spin_lock_irq(&guc->irq_lock);
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_guc_submission.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
index e402a2932592..4cf9d3e50263 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
@@ -8,7 +8,8 @@
 
 #include <linux/types.h>
 
-struct intel_guc;
+#include "intel_guc.h"
+
 struct intel_engine_cs;
 
 void intel_guc_submission_init_early(struct intel_guc *guc);
@@ -20,4 +21,20 @@ int intel_guc_preempt_work_create(struct intel_guc *guc);
 void intel_guc_preempt_work_destroy(struct intel_guc *guc);
 bool intel_engine_in_guc_submission_mode(const struct intel_engine_cs *engine);
 
+static inline bool intel_guc_submission_is_supported(struct intel_guc *guc)
+{
+	/* 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);
+}
+
 #endif
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 5825a6c3e0a0..f81b4d40bc90 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..c1f39bdc115a 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
@@ -7,6 +7,7 @@
 #define _INTEL_UC_H_
 
 #include "intel_guc.h"
+#include "intel_guc_submission.h"
 #include "intel_huc.h"
 #include "i915_params.h"
 
@@ -61,33 +62,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 23ecaf75597f..9d2697c5871a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2003,10 +2003,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

[Intel-gfx] [PATCH v3 08/10] drm/i915/uc: Abort early on uc_init failure

[Daniele Ceraolo Spurio]

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>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@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 f81b4d40bc90..1c74518c2459 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 c1f39bdc115a..5ae7b50b7dc1 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
@@ -17,7 +17,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);
@@ -90,7 +90,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

[Intel-gfx] [PATCH v3 09/10] drm/i915/uc: consolidate firmware cleanup

[Daniele Ceraolo Spurio]

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).

v2: s/ready_to_load/loadable (Michal), only run guc/huc_fini if the
    fw is in loadable state

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>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> #v1
---
 drivers/gpu/drm/i915/gt/uc/intel_guc.c   | 12 +++++-------
 drivers/gpu/drm/i915/gt/uc/intel_huc.c   |  5 +++--
 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, 29 insertions(+), 15 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..819f09ef51fc 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_LOADABLE);
+
 	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;
 }
@@ -384,7 +385,7 @@ void intel_guc_fini(struct intel_guc *guc)
 {
 	struct intel_gt *gt = guc_to_gt(guc);
 
-	if (!intel_uc_fw_is_available(&guc->fw))
+	if (!intel_uc_fw_is_loadable(&guc->fw))
 		return;
 
 	i915_ggtt_disable_guc(gt->ggtt);
@@ -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..a74b65694512 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
@@ -121,19 +121,20 @@ int intel_huc_init(struct intel_huc *huc)
 	if (err)
 		goto out_fini;
 
+	intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_LOADABLE);
+
 	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;
 }
 
 void intel_huc_fini(struct intel_huc *huc)
 {
-	if (!intel_uc_fw_is_available(&huc->fw))
+	if (!intel_uc_fw_is_loadable(&huc->fw))
 		return;
 
 	intel_huc_rsa_data_destroy(huc);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 1c74518c2459..a4cbe06e06bd 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_loadable(&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..5434c07aefa1 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_loadable(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..888ff0de0244 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                         |
+ * +------------+-                   |                             -+
+ * |   init     |                    V                              |
+ * |            |        /------> LOADABLE <----<-----------\       |
  * +------------+-       \         /    \        \           \     -+
  * |            |         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_LOADABLE, /* 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_LOADABLE:
+		return "LOADABLE";
 	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_LOADABLE:
 	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_loadable(struct intel_uc_fw *uc_fw)
+{
+	return __intel_uc_fw_status(uc_fw) >= INTEL_UC_FIRMWARE_LOADABLE;
+}
+
 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_LOADABLE);
 }
 
 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

[Intel-gfx] [PATCH v3 10/10] HAX: drm/i915: default to enable_guc=2

[Daniele Ceraolo Spurio]

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 45323732f099..7b5d32c990bc 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

Re: [Intel-gfx] [PATCH v3 01/10] drm/i915/debugfs: Pass guc_log struct to i915_guc_log_info

[Andi Shyti]

Hi Daniele,

On Tue, Feb 11, 2020 at 04:31:15PM -0800, Daniele Ceraolo Spurio 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>
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>

Reviewed-by: Andi Shyt <andi.shyti@intel.com>

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

Re: [Intel-gfx] [PATCH v3 02/10] drm/i915/guc: Kill USES_GUC macro

[Andi Shyti]

Hi Daniele,

On Tue, Feb 11, 2020 at 04:31:16PM -0800, Daniele Ceraolo Spurio 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>

Reviewed-by: Andi Shyti <andi.shyti@intel.com>

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

Re: [Intel-gfx] [PATCH v3 03/10] drm/i915/guc: Kill USES_GUC_SUBMISSION macro

[Andi Shyti]

Hi Daniele,

On Tue, Feb 11, 2020 at 04:31:17PM -0800, Daniele Ceraolo Spurio wrote:
> 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

Reviewed-by: Andi Shyti <andi.shyti@intel.com>

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

Re: [Intel-gfx] [PATCH v3 05/10] drm/i915/uc: autogenerate uC checker functions

[Andi Shyti]

Hi Daniele,

On Tue, Feb 11, 2020 at 04:31:19PM -0800, Daniele Ceraolo Spurio wrote:
> 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>

interesting, however it can be hard to follow.
I haven't spotted anything wrong (also in the patches that
follow), and I hope you tested it properly :)

Reviewed-by: Andi Shyti <andi.shyti@intel.com>

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

Re: [Intel-gfx] [PATCH v3 08/10] drm/i915/uc: Abort early on uc_init failure

[Andi Shyti]

Hi Daniele,

> +	if (intel_uc_uses_huc(uc)) {
> +		ret = intel_huc_init(huc);

are we ever going to call intel_huc_init() if
!intel_uc_uses_huc()? if not, why don't check intel_uc_uses_huc()
inside intel_huc_init()? just to avoid always the double "if".

Not a big deal though:

Reviewed-by: Andi Shyti <andi.shyti@intel.com>

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

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Commit early to GuC (rev3)

[Patchwork]

== Series Details ==

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

== Summary ==

$ dim checkpatch origin/drm-tip
86c2261830f8 drm/i915/debugfs: Pass guc_log struct to i915_guc_log_info
434763d74348 drm/i915/guc: Kill USES_GUC macro
9c3bd7cffa07 drm/i915/guc: Kill USES_GUC_SUBMISSION macro
53a7bb512097 drm/i915/uc: Update the FW status on injected fetch error
ac5ee731ad09 drm/i915/uc: autogenerate uC checker functions
-:32: CHECK:MACRO_ARG_PRECEDENCE: Macro argument 'x' may be better as '(x)' to avoid precedence issues
#32: 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); \
 }

-:42: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#42: 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)

-:42: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'x' - possible side-effects?
#42: 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
300e9108bf45 drm/i915/uc: Improve tracking of uC init status
c7b983206d15 drm/i915/guc: Apply new uC status tracking to GuC submission as well
-:246: CHECK:MACRO_ARG_PRECEDENCE: Macro argument 'x' may be better as '(x)' to avoid precedence issues
#246: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc.h:65:
+#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); \
 }

-:257: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#257: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc.h:71:
+#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)

-:257: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'x' - possible side-effects?
#257: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc.h:71:
+#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)

-:257: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'func' - possible side-effects?
#257: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc.h:71:
+#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, 250 lines checked
6df67d7fcd06 drm/i915/uc: Abort early on uc_init failure
e5b2f4d6ca34 drm/i915/uc: consolidate firmware cleanup
70b2b644b94a 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

Re: [Intel-gfx] [PATCH v3 08/10] drm/i915/uc: Abort early on uc_init failure

[Daniele Ceraolo Spurio]

On 2/11/20 5:47 PM, Andi Shyti wrote:
> Hi Daniele,
> 
>> +	if (intel_uc_uses_huc(uc)) {
>> +		ret = intel_huc_init(huc);
> 
> are we ever going to call intel_huc_init() if
> !intel_uc_uses_huc()? if not, why don't check intel_uc_uses_huc()
> inside intel_huc_init()? just to avoid always the double "if".

I'm actually plotting another refactoring of the init/fini flows as I 
don't like how we toggle the FW states at the moment. I can bundle this 
change with that.

Daniele

> 
> Not a big deal though:
> 
> Reviewed-by: Andi Shyti <andi.shyti@intel.com>
> 
> Andi
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✓ Fi.CI.BAT: success for Commit early to GuC (rev3)

[Patchwork]

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_7925 -> Patchwork_16535
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-cml-u2:          [PASS][1] -> [FAIL][2] ([i915#217])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/fi-cml-u2/igt@kms_chamelium@common-hpd-after-suspend.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/fi-cml-u2/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [PASS][3] -> [FAIL][4] ([fdo#111407])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
#### Possible fixes ####

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

  * igt@i915_selftest@live_gtt:
    - {fi-tgl-dsi}:       [TIMEOUT][7] ([fdo#112126] / [fdo#112271]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/fi-tgl-dsi/igt@i915_selftest@live_gtt.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/fi-tgl-dsi/igt@i915_selftest@live_gtt.html

  * igt@i915_selftest@live_hangcheck:
    - fi-icl-y:           [INCOMPLETE][9] ([fdo#108569]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/fi-icl-y/igt@i915_selftest@live_hangcheck.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/fi-icl-y/igt@i915_selftest@live_hangcheck.html

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

  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#111407]: https://bugs.freedesktop.org/show_bug.cgi?id=111407
  [fdo#112126]: https://bugs.freedesktop.org/show_bug.cgi?id=112126
  [fdo#112271]: https://bugs.freedesktop.org/show_bug.cgi?id=112271
  [i915#178]: https://gitlab.freedesktop.org/drm/intel/issues/178
  [i915#217]: https://gitlab.freedesktop.org/drm/intel/issues/217
  [i915#937]: https://gitlab.freedesktop.org/drm/intel/issues/937


Participating hosts (42 -> 43)
------------------------------

  Additional (8): fi-ivb-3770 fi-cfl-8109u fi-skl-6700k2 fi-skl-lmem fi-blb-e6850 fi-kbl-r fi-skl-6600u fi-snb-2600 
  Missing    (7): fi-hsw-peppy fi-byt-squawks fi-glk-dsi fi-ctg-p8600 fi-gdg-551 fi-byt-clapper fi-bdw-samus 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7925 -> Patchwork_16535

  CI-20190529: 20190529
  CI_DRM_7925: b266b79c3de9874e0f991b6a9cc284a424094649 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5437: ae42fedfd0c536c560e8e17b06d9c7b94a4e8f0c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_16535: 70b2b644b94a9a333ba373ea47c758c7d581368f @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

70b2b644b94a HAX: drm/i915: default to enable_guc=2
e5b2f4d6ca34 drm/i915/uc: consolidate firmware cleanup
6df67d7fcd06 drm/i915/uc: Abort early on uc_init failure
c7b983206d15 drm/i915/guc: Apply new uC status tracking to GuC submission as well
300e9108bf45 drm/i915/uc: Improve tracking of uC init status
ac5ee731ad09 drm/i915/uc: autogenerate uC checker functions
53a7bb512097 drm/i915/uc: Update the FW status on injected fetch error
9c3bd7cffa07 drm/i915/guc: Kill USES_GUC_SUBMISSION macro
434763d74348 drm/i915/guc: Kill USES_GUC macro
86c2261830f8 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_16535/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 06/10] drm/i915/uc: Improve tracking of uC init status

[John Harrison]

On 2/11/2020 16:31, Daniele Ceraolo Spurio 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)
>
> v3: s/wants_huc/uses_huc in uc_init_wopcm
>
> 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     | 21 +++++++++++---------
>   drivers/gpu/drm/i915/gt/uc/intel_uc.h     | 24 ++++++++++++++++++++++-
>   5 files changed, 50 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index 668b067b71e2..b51adaac8752 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..5825a6c3e0a0 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));
> @@ -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.
Should be another line for 'Running'? I guess the definition of 
'is_running' comes from elsewhere but it would make sense to include it 
in the description here.

Also, it would be good to include the actual function names themselves. 
That way if someone searches on 'intel_uc_uses_guc', for example, they 
will find this description. Especially as searching for them by either 
text or symbol will not find the definition!

With that tweak:
Reviewed-by: John Harrison <John.C.Harrison@Intel.com>


> + */
> +
>   #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

Re: [Intel-gfx] [PATCH v3 07/10] drm/i915/guc: Apply new uC status tracking to GuC submission as well

[John Harrison]

On 2/11/2020 16:31, Daniele Ceraolo Spurio 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.
>
> v2: move functions to guc_submission.h (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_guc.c        | 12 ++++----
>   drivers/gpu/drm/i915/gt/uc/intel_guc.h        |  7 +----
>   .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  9 ++----
>   .../gpu/drm/i915/gt/uc/intel_guc_submission.h | 19 +++++++++++-
>   drivers/gpu/drm/i915/gt/uc/intel_uc.c         | 14 ++++-----
>   drivers/gpu/drm/i915/gt/uc/intel_uc.h         | 30 +++++++------------
>   drivers/gpu/drm/i915/gvt/scheduler.c          |  2 +-
>   drivers/gpu/drm/i915/i915_drv.h               |  6 ----
>   drivers/gpu/drm/i915/intel_gvt.c              |  2 +-
>   9 files changed, 48 insertions(+), 53 deletions(-)

Reviewed-by: John Harrison <John.C.Harrison@Intel.com>

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

Re: [Intel-gfx] [PATCH v3 09/10] drm/i915/uc: consolidate firmware cleanup

[John Harrison]

On 2/11/2020 16:31, Daniele Ceraolo Spurio 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
Is it not worth updating the commit message to use the name that is 
actually in the code now?

Otherwise:
Reviewed-by: John Harrison <John.C.Harrison@Intel.com>

> 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).
>
> v2: s/ready_to_load/loadable (Michal), only run guc/huc_fini if the
>      fw is in loadable state
>
> 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>
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> #v1
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_guc.c   | 12 +++++-------
>   drivers/gpu/drm/i915/gt/uc/intel_huc.c   |  5 +++--
>   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, 29 insertions(+), 15 deletions(-)
>

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

Re: [Intel-gfx] [PATCH v3 06/10] drm/i915/uc: Improve tracking of uC init status

[Daniele Ceraolo Spurio]

On 2/13/20 3:36 PM, John Harrison wrote:
> On 2/11/2020 16:31, Daniele Ceraolo Spurio 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)
>>
>> v3: s/wants_huc/uses_huc in uc_init_wopcm
>>
>> 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     | 21 +++++++++++---------
>>   drivers/gpu/drm/i915/gt/uc/intel_uc.h     | 24 ++++++++++++++++++++++-
>>   5 files changed, 50 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>> index 668b067b71e2..b51adaac8752 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..5825a6c3e0a0 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));
>> @@ -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.
> Should be another line for 'Running'? I guess the definition of 
> 'is_running' comes from elsewhere but it would make sense to include it 
> in the description here.

"Running" belongs to a different set of states, indicating the current 
state of the uC. The ones added here are about what we plan to do with 
the GuC, not what the current state is, so I disagree that it makes 
sense to add "running" here.

> 
> Also, it would be good to include the actual function names themselves. 
> That way if someone searches on 'intel_uc_uses_guc', for example, they 
> will find this description. Especially as searching for them by either 
> text or symbol will not find the definition!
> 

How do you want them added? We don't usually do that for autogenerated 
functions (there is an example of that further down in this file).

Daniele

> With that tweak:
> Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
> 
> 
>> + */
>> +
>>   #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

Re: [Intel-gfx] [PATCH v3 09/10] drm/i915/uc: consolidate firmware cleanup

[Daniele Ceraolo Spurio]

On 2/13/20 3:44 PM, John Harrison wrote:
> On 2/11/2020 16:31, Daniele Ceraolo Spurio 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
> Is it not worth updating the commit message to use the name that is 
> actually in the code now?

D'oh! Missed this one.

Daniele

> 
> Otherwise:
> Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
> 
>> 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).
>>
>> v2: s/ready_to_load/loadable (Michal), only run guc/huc_fini if the
>>      fw is in loadable state
>>
>> 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>
>> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> #v1
>> ---
>>   drivers/gpu/drm/i915/gt/uc/intel_guc.c   | 12 +++++-------
>>   drivers/gpu/drm/i915/gt/uc/intel_huc.c   |  5 +++--
>>   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, 29 insertions(+), 15 deletions(-)
>>
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 06/10] drm/i915/uc: Improve tracking of uC init status

[John Harrison]

On 2/13/2020 15:44, Daniele Ceraolo Spurio wrote:
> On 2/13/20 3:36 PM, John Harrison wrote:
>> On 2/11/2020 16:31, Daniele Ceraolo Spurio 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)
>>>
>>> v3: s/wants_huc/uses_huc in uc_init_wopcm
>>>
>>> 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     | 21 +++++++++++---------
>>>   drivers/gpu/drm/i915/gt/uc/intel_uc.h     | 24 
>>> ++++++++++++++++++++++-
>>>   5 files changed, 50 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>>> index 668b067b71e2..b51adaac8752 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..5825a6c3e0a0 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));
>>> @@ -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.
>> Should be another line for 'Running'? I guess the definition of 
>> 'is_running' comes from elsewhere but it would make sense to include 
>> it in the description here.
>
> "Running" belongs to a different set of states, indicating the current 
> state of the uC. The ones added here are about what we plan to do with 
> the GuC, not what the current state is, so I disagree that it makes 
> sense to add "running" here.
>
>>
>> Also, it would be good to include the actual function names 
>> themselves. That way if someone searches on 'intel_uc_uses_guc', for 
>> example, they will find this description. Especially as searching for 
>> them by either text or symbol will not find the definition!
>>
>
> How do you want them added? We don't usually do that for autogenerated 
> functions (there is an example of that further down in this file).
>
> Daniele
>
An example of documentation? Or an example of more autogenerated 
functions? The intel_uc.h currently in the tree has no documentation in 
it at all! I was just thinking that I personally get frustrated when I 
try to search for a function definition and can't find anything because 
it is some fancy autogenerated thing with no documentation at all. If 
you feel that including the expanded names is too verbose then fine, 
it's hardly a show stopper issue. At least you have added some 
documentation about what the various states mean!

John.

>> With that tweak:
>> Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
>>
>>
>>> + */
>>> +
>>>   #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

Re: [Intel-gfx] [PATCH v3 06/10] drm/i915/uc: Improve tracking of uC init status

[Daniele Ceraolo Spurio]

On 2/13/20 4:04 PM, John Harrison wrote:
> On 2/13/2020 15:44, Daniele Ceraolo Spurio wrote:
>> On 2/13/20 3:36 PM, John Harrison wrote:
>>> On 2/11/2020 16:31, Daniele Ceraolo Spurio 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)
>>>>
>>>> v3: s/wants_huc/uses_huc in uc_init_wopcm
>>>>
>>>> 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     | 21 +++++++++++---------
>>>>   drivers/gpu/drm/i915/gt/uc/intel_uc.h     | 24 
>>>> ++++++++++++++++++++++-
>>>>   5 files changed, 50 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
>>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>>>> index 668b067b71e2..b51adaac8752 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..5825a6c3e0a0 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));
>>>> @@ -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.
>>> Should be another line for 'Running'? I guess the definition of 
>>> 'is_running' comes from elsewhere but it would make sense to include 
>>> it in the description here.
>>
>> "Running" belongs to a different set of states, indicating the current 
>> state of the uC. The ones added here are about what we plan to do with 
>> the GuC, not what the current state is, so I disagree that it makes 
>> sense to add "running" here.
>>
>>>
>>> Also, it would be good to include the actual function names 
>>> themselves. That way if someone searches on 'intel_uc_uses_guc', for 
>>> example, they will find this description. Especially as searching for 
>>> them by either text or symbol will not find the definition!
>>>
>>
>> How do you want them added? We don't usually do that for autogenerated 
>> functions (there is an example of that further down in this file).
>>
>> Daniele
>>
> An example of documentation? Or an example of more autogenerated 
> functions? The intel_uc.h currently in the tree has no documentation in 
> it at all! I was just thinking that I personally get frustrated when I 
> try to search for a function definition and can't find anything because 
> it is some fancy autogenerated thing with no documentation at all. If 
> you feel that including the expanded names is too verbose then fine, 
> it's hardly a show stopper issue. At least you have added some 
> documentation about what the various states mean!
> 

I meant to ask how did you want them documented. There isn't a 1:1 
mapping between the function and the states (4 theoretical states, but 3 
actual recorded states/functions) and the names are subject to change as 
well, which is difficult to mirror in detached documentation. And yes, 
it does also feel a bit too verbose to me :P

Daniele

> John.
> 
>>> With that tweak:
>>> Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
>>>
>>>
>>>> + */
>>>> +
>>>>   #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

Re: [Intel-gfx] [PATCH v3 06/10] drm/i915/uc: Improve tracking of uC init status

[John Harrison]

On 2/13/2020 16:21, Daniele Ceraolo Spurio wrote:
> On 2/13/20 4:04 PM, John Harrison wrote:
>> On 2/13/2020 15:44, Daniele Ceraolo Spurio wrote:
>>> On 2/13/20 3:36 PM, John Harrison wrote:
>>>> On 2/11/2020 16:31, Daniele Ceraolo Spurio 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)
>>>>>
>>>>> v3: s/wants_huc/uses_huc in uc_init_wopcm
>>>>>
>>>>> 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     | 21 +++++++++++---------
>>>>>   drivers/gpu/drm/i915/gt/uc/intel_uc.h     | 24 
>>>>> ++++++++++++++++++++++-
>>>>>   5 files changed, 50 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
>>>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>>>>> index 668b067b71e2..b51adaac8752 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..5825a6c3e0a0 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));
>>>>> @@ -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.
>>>> Should be another line for 'Running'? I guess the definition of 
>>>> 'is_running' comes from elsewhere but it would make sense to 
>>>> include it in the description here.
>>>
>>> "Running" belongs to a different set of states, indicating the 
>>> current state of the uC. The ones added here are about what we plan 
>>> to do with the GuC, not what the current state is, so I disagree 
>>> that it makes sense to add "running" here.
>>>
>>>>
>>>> Also, it would be good to include the actual function names 
>>>> themselves. That way if someone searches on 'intel_uc_uses_guc', 
>>>> for example, they will find this description. Especially as 
>>>> searching for them by either text or symbol will not find the 
>>>> definition!
>>>>
>>>
>>> How do you want them added? We don't usually do that for 
>>> autogenerated functions (there is an example of that further down in 
>>> this file).
>>>
>>> Daniele
>>>
>> An example of documentation? Or an example of more autogenerated 
>> functions? The intel_uc.h currently in the tree has no documentation 
>> in it at all! I was just thinking that I personally get frustrated 
>> when I try to search for a function definition and can't find 
>> anything because it is some fancy autogenerated thing with no 
>> documentation at all. If you feel that including the expanded names 
>> is too verbose then fine, it's hardly a show stopper issue. At least 
>> you have added some documentation about what the various states mean!
>>
>
> I meant to ask how did you want them documented. There isn't a 1:1 
> mapping between the function and the states (4 theoretical states, but 
> 3 actual recorded states/functions) and the names are subject to 
> change as well, which is difficult to mirror in detached 
> documentation. And yes, it does also feel a bit too verbose to me :P

Just ship it then.
Reviewed-by: John Harrison <John.C.Harrison@Intel.com>

>
> Daniele
>
>> John.
>>
>>>> With that tweak:
>>>> Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
>>>>
>>>>
>>>>> + */
>>>>> +
>>>>>   #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

[Intel-gfx] ✓ Fi.CI.IGT: success for Commit early to GuC (rev3)

[Patchwork]

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_7925_full -> Patchwork_16535_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

New tests
---------

  New tests have been introduced between CI_DRM_7925_full and Patchwork_16535_full:

### New IGT tests (3) ###

  * igt@kms_content_protection@atomic-dpms:
    - Statuses : 2 fail(s) 5 skip(s)
    - Exec time: [0.0, 125.61] s

  * igt@kms_psr2_su@frontbuffer:
    - Statuses : 2 pass(s) 6 skip(s)
    - Exec time: [0.0, 0.34] s

  * igt@kms_psr2_su@page_flip:
    - Statuses : 1 pass(s) 7 skip(s)
    - Exec time: [0.0, 0.49] s

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_schedule@preempt-other-chain-bsd:
    - shard-iclb:         [PASS][1] -> [SKIP][2] ([fdo#112146]) +2 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-iclb5/igt@gem_exec_schedule@preempt-other-chain-bsd.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-iclb2/igt@gem_exec_schedule@preempt-other-chain-bsd.html

  * igt@kms_cursor_crc@pipe-c-cursor-64x21-random:
    - shard-hsw:          [PASS][3] -> [DMESG-WARN][4] ([IGT#6])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-hsw5/igt@kms_cursor_crc@pipe-c-cursor-64x21-random.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-hsw1/igt@kms_cursor_crc@pipe-c-cursor-64x21-random.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions:
    - shard-snb:          [PASS][5] -> [SKIP][6] ([fdo#109271]) +4 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-snb5/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-snb2/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html

  * igt@kms_draw_crc@draw-method-xrgb8888-mmap-wc-ytiled:
    - shard-glk:          [PASS][7] -> [FAIL][8] ([i915#52] / [i915#54])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-glk4/igt@kms_draw_crc@draw-method-xrgb8888-mmap-wc-ytiled.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-glk1/igt@kms_draw_crc@draw-method-xrgb8888-mmap-wc-ytiled.html

  * igt@kms_flip@2x-flip-vs-expired-vblank:
    - shard-glk:          [PASS][9] -> [FAIL][10] ([i915#46])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-glk6/igt@kms_flip@2x-flip-vs-expired-vblank.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-glk6/igt@kms_flip@2x-flip-vs-expired-vblank.html

  * igt@kms_flip@plain-flip-fb-recreate:
    - shard-kbl:          [PASS][11] -> [FAIL][12] ([i915#34])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-kbl2/igt@kms_flip@plain-flip-fb-recreate.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-kbl3/igt@kms_flip@plain-flip-fb-recreate.html

  * igt@kms_flip_tiling@flip-x-tiled:
    - shard-skl:          [PASS][13] -> [FAIL][14] ([fdo#108145] / [i915#699])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-skl1/igt@kms_flip_tiling@flip-x-tiled.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-skl9/igt@kms_flip_tiling@flip-x-tiled.html

  * igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-render:
    - shard-skl:          [PASS][15] -> [FAIL][16] ([i915#49]) +2 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-skl1/igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-render.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-skl9/igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-render.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - shard-skl:          [PASS][17] -> [FAIL][18] ([i915#53])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-skl1/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-skl9/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
    - shard-apl:          [PASS][19] -> [DMESG-WARN][20] ([i915#180]) +1 similar issue
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-apl3/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-apl8/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c.html

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-skl:          [PASS][21] -> [FAIL][22] ([fdo#108145]) +1 similar issue
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-skl8/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-skl1/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html

  * igt@kms_plane_lowres@pipe-a-tiling-x:
    - shard-glk:          [PASS][23] -> [FAIL][24] ([i915#899])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-glk4/igt@kms_plane_lowres@pipe-a-tiling-x.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-glk1/igt@kms_plane_lowres@pipe-a-tiling-x.html

  * igt@kms_psr@psr2_cursor_render:
    - shard-iclb:         [PASS][25] -> [SKIP][26] ([fdo#109441]) +2 similar issues
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-iclb2/igt@kms_psr@psr2_cursor_render.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-iclb1/igt@kms_psr@psr2_cursor_render.html

  * igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend:
    - shard-skl:          [PASS][27] -> [INCOMPLETE][28] ([i915#69])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-skl4/igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-skl4/igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend.html

  * igt@perf_pmu@busy-no-semaphores-vcs1:
    - shard-iclb:         [PASS][29] -> [SKIP][30] ([fdo#112080]) +9 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-iclb4/igt@perf_pmu@busy-no-semaphores-vcs1.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-iclb5/igt@perf_pmu@busy-no-semaphores-vcs1.html

  * igt@prime_vgem@fence-wait-bsd2:
    - shard-iclb:         [PASS][31] -> [SKIP][32] ([fdo#109276]) +9 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-iclb4/igt@prime_vgem@fence-wait-bsd2.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-iclb3/igt@prime_vgem@fence-wait-bsd2.html

  
#### Possible fixes ####

  * {igt@gem_ctx_persistence@legacy-engines-mixed-process@bsd}:
    - shard-skl:          [FAIL][33] ([i915#679]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-skl3/igt@gem_ctx_persistence@legacy-engines-mixed-process@bsd.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-skl7/igt@gem_ctx_persistence@legacy-engines-mixed-process@bsd.html

  * {igt@gem_ctx_persistence@legacy-engines-mixed-process@bsd1}:
    - shard-skl:          [INCOMPLETE][35] -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-skl3/igt@gem_ctx_persistence@legacy-engines-mixed-process@bsd1.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-skl7/igt@gem_ctx_persistence@legacy-engines-mixed-process@bsd1.html

  * igt@gem_exec_schedule@pi-common-bsd:
    - shard-iclb:         [SKIP][37] ([i915#677]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-iclb4/igt@gem_exec_schedule@pi-common-bsd.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-iclb5/igt@gem_exec_schedule@pi-common-bsd.html

  * igt@gem_exec_schedule@preempt-queue-bsd2:
    - shard-iclb:         [SKIP][39] ([fdo#109276]) -> [PASS][40] +7 similar issues
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-iclb3/igt@gem_exec_schedule@preempt-queue-bsd2.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-iclb4/igt@gem_exec_schedule@preempt-queue-bsd2.html

  * igt@gem_partial_pwrite_pread@reads:
    - shard-hsw:          [FAIL][41] ([i915#694]) -> [PASS][42] +1 similar issue
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-hsw1/igt@gem_partial_pwrite_pread@reads.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-hsw5/igt@gem_partial_pwrite_pread@reads.html

  * igt@i915_hangman@error-state-capture-vcs1:
    - shard-iclb:         [SKIP][43] ([fdo#112080]) -> [PASS][44] +3 similar issues
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-iclb5/igt@i915_hangman@error-state-capture-vcs1.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-iclb2/igt@i915_hangman@error-state-capture-vcs1.html

  * igt@i915_pm_dc@dc6-dpms:
    - shard-iclb:         [FAIL][45] ([i915#454]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-iclb1/igt@i915_pm_dc@dc6-dpms.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-iclb4/igt@i915_pm_dc@dc6-dpms.html

  * igt@i915_pm_rps@waitboost:
    - shard-iclb:         [FAIL][47] ([i915#413]) -> [PASS][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-iclb8/igt@i915_pm_rps@waitboost.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-iclb6/igt@i915_pm_rps@waitboost.html

  * igt@i915_selftest@live_gtt:
    - shard-hsw:          [TIMEOUT][49] ([fdo#112271]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-hsw5/igt@i915_selftest@live_gtt.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-hsw6/igt@i915_selftest@live_gtt.html

  * igt@i915_suspend@debugfs-reader:
    - shard-iclb:         [INCOMPLETE][51] -> [PASS][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-iclb3/igt@i915_suspend@debugfs-reader.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-iclb5/igt@i915_suspend@debugfs-reader.html

  * igt@kms_color@pipe-a-gamma:
    - shard-tglb:         [FAIL][53] ([i915#1149]) -> [PASS][54]
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-tglb3/igt@kms_color@pipe-a-gamma.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-tglb6/igt@kms_color@pipe-a-gamma.html

  * igt@kms_cursor_crc@pipe-b-cursor-256x256-sliding:
    - shard-skl:          [FAIL][55] ([i915#54]) -> [PASS][56]
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-skl7/igt@kms_cursor_crc@pipe-b-cursor-256x256-sliding.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-skl3/igt@kms_cursor_crc@pipe-b-cursor-256x256-sliding.html

  * igt@kms_cursor_crc@pipe-b-cursor-suspend:
    - shard-apl:          [DMESG-WARN][57] ([i915#180]) -> [PASS][58] +2 similar issues
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-apl4/igt@kms_cursor_crc@pipe-b-cursor-suspend.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-apl8/igt@kms_cursor_crc@pipe-b-cursor-suspend.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-kbl:          [DMESG-WARN][59] ([i915#180]) -> [PASS][60] +4 similar issues
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-kbl4/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-kbl2/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
    - shard-glk:          [FAIL][61] ([i915#79]) -> [PASS][62]
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-glk1/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-glk7/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html

  * igt@kms_flip@flip-vs-suspend:
    - shard-skl:          [INCOMPLETE][63] ([i915#221]) -> [PASS][64]
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-skl5/igt@kms_flip@flip-vs-suspend.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-skl7/igt@kms_flip@flip-vs-suspend.html

  * {igt@kms_hdr@bpc-switch}:
    - shard-skl:          [FAIL][65] ([i915#1188]) -> [PASS][66]
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-skl4/igt@kms_hdr@bpc-switch.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-skl4/igt@kms_hdr@bpc-switch.html

  * igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min:
    - shard-skl:          [FAIL][67] ([fdo#108145]) -> [PASS][68]
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-skl2/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-skl2/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [FAIL][69] ([fdo#108145] / [i915#265]) -> [PASS][70]
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-skl4/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-skl6/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_psr2_su@frontbuffer (NEW):
    - shard-iclb:         [SKIP][71] ([fdo#109642] / [fdo#111068]) -> [PASS][72]
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-iclb6/igt@kms_psr2_su@frontbuffer.html
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-iclb2/igt@kms_psr2_su@frontbuffer.html

  * igt@kms_psr@primary_mmap_cpu:
    - shard-tglb:         [SKIP][73] ([i915#668]) -> [PASS][74] +3 similar issues
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-tglb3/igt@kms_psr@primary_mmap_cpu.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-tglb6/igt@kms_psr@primary_mmap_cpu.html

  * igt@kms_psr@psr2_sprite_mmap_gtt:
    - shard-iclb:         [SKIP][75] ([fdo#109441]) -> [PASS][76] +1 similar issue
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-iclb1/igt@kms_psr@psr2_sprite_mmap_gtt.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-iclb2/igt@kms_psr@psr2_sprite_mmap_gtt.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [FAIL][77] ([i915#31]) -> [PASS][78]
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-kbl6/igt@kms_setmode@basic.html
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-kbl7/igt@kms_setmode@basic.html

  
#### Warnings ####

  * igt@gem_ctx_isolation@vcs1-nonpriv-switch:
    - shard-iclb:         [SKIP][79] ([fdo#112080]) -> [FAIL][80] ([IGT#28])
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-iclb6/igt@gem_ctx_isolation@vcs1-nonpriv-switch.html
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-iclb2/igt@gem_ctx_isolation@vcs1-nonpriv-switch.html

  * igt@gem_tiled_blits@normal:
    - shard-hsw:          [FAIL][81] ([i915#818]) -> [FAIL][82] ([i915#694])
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7925/shard-hsw7/igt@gem_tiled_blits@normal.html
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16535/shard-hsw1/igt@gem_tiled_blits@normal.html

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

  [IGT#28]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/28
  [IGT#6]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/6
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [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
  [fdo#112271]: https://bugs.freedesktop.org/show_bug.cgi?id=112271
  [i915#1149]: https://gitlab.freedesktop.org/drm/intel/issues/1149
  [i915#1188]: https://gitlab.freedesktop.org/drm/intel/issues/1188
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#221]: https://gitlab.freedesktop.org/drm/intel/issues/221
  [i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
  [i915#31]: https://gitlab.freedesktop.org/drm/intel/issues/31
  [i915#34]: https://gitlab.freedesktop.org/drm/intel/issues/34
  [i915#413]: https://gitlab.freedesktop.org/drm/intel/issues/413
  [i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
  [i915#46]: https://gitlab.freedesktop.org/drm/intel/issues/46
  [i915#49]: https://gitlab.freedesktop.org/drm/intel/issues/49
  [i915#52]: https://gitlab.freedesktop.org/drm/intel/issues/52
  [i915#53]: https://gitlab.freedesktop.org/drm/intel/issues/53
  [i915#54]: https://gitlab.freedesktop.org/drm/intel/issues/54
  [i915#668]: https://gitlab.freedesktop.org/drm/intel/issues/668
  [i915#677]: https://gitlab.freedesktop.org/drm/intel/issues/677
  [i915#679]: https://gitlab.freedesktop.org/drm/intel/issues/679
  [i915#69]: https://gitlab.freedesktop.org/drm/intel/issues/69
  [i915#694]: https://gitlab.freedesktop.org/drm/intel/issues/694
  [i915#699]: https://gitlab.freedesktop.org/drm/intel/issues/699
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#818]: https://gitlab.freedesktop.org/drm/intel/issues/818
  [i915#899]: https://gitlab.freedesktop.org/drm/intel/issues/899


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

  No changes in participating hosts


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7925 -> Patchwork_16535

  CI-20190529: 20190529
  CI_DRM_7925: b266b79c3de9874e0f991b6a9cc284a424094649 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5437: ae42fedfd0c536c560e8e17b06d9c7b94a4e8f0c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_16535: 70b2b644b94a9a333ba373ea47c758c7d581368f @ 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_16535/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx