[PATCH 0/9] uC fw path unification + misc clean-up

[PATCH 0/9] uC fw path unification + misc clean-up

[Daniele Ceraolo Spurio]

The main aim of this patch series is unify the guc_fw and huc_fw
handling paths and improve the related state tracking. Ultimately, I'd
like to reach the point where we can kill the intel_guc_fw and
intel_huc_fw files and just keep the few differences in other files. Not
yet going that far in this series though.

Bundled in is some more gt-fication and minor clean-up.

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>

Daniele Ceraolo Spurio (9):
  drm/i915/uc: Gt-fy uc reset
  drm/i915/uc: Unify uC platform check
  drm/i915/uc: Unify uC FW selection
  drm/i915/uc: Sanitize uC when GT is sanitized
  drm/i915/uc: Unify uc_fw status tracking
  drm/i915/uc: Move xfer rsa logic to common function
  drm/i915/huc: Copy huc rsa only once
  drm/i915/uc: Plumb the gt through fw_upload
  drm/i915/uc: Unify uC firmware upload

 drivers/gpu/drm/i915/gem/i915_gem_pm.c    |   1 -
 drivers/gpu/drm/i915/gt/intel_gt_pm.c     |   2 +
 drivers/gpu/drm/i915/gt/intel_reset.c     |   2 +-
 drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c | 229 ++++++----------------
 drivers/gpu/drm/i915/gt/uc/intel_huc.c    |  25 ++-
 drivers/gpu/drm/i915/gt/uc/intel_huc.h    |   1 -
 drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 184 +++--------------
 drivers/gpu/drm/i915/gt/uc/intel_uc.c     |   8 +-
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c  | 220 ++++++++++++++-------
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h  |  84 +++++---
 drivers/gpu/drm/i915/gt/uc/selftest_guc.c |   4 +-
 drivers/gpu/drm/i915/i915_debugfs.c       |   6 +-
 drivers/gpu/drm/i915/i915_drv.h           |  15 +-
 drivers/gpu/drm/i915/i915_gpu_error.c     |   4 +-
 drivers/gpu/drm/i915/i915_irq.c           |   2 +-
 drivers/gpu/drm/i915/i915_pci.c           |   4 +-
 drivers/gpu/drm/i915/intel_device_info.h  |   2 +-
 drivers/gpu/drm/i915/intel_pm.c           |   4 +-
 drivers/gpu/drm/i915/intel_wopcm.c        |   4 +-
 19 files changed, 330 insertions(+), 471 deletions(-)

-- 
2.22.0

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

[PATCH 1/9] drm/i915/uc: Gt-fy uc reset

[Daniele Ceraolo Spurio]

This was the last place in gt/uc that was still using I915_READ
with the global dev_priv.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_uc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 5ebb0a534718..4480a3dc2449 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -37,17 +37,17 @@ static void guc_free_load_err_log(struct intel_guc *guc);
  */
 static int __intel_uc_reset_hw(struct intel_uc *uc)
 {
-	struct drm_i915_private *dev_priv = uc_to_gt(uc)->i915;
+	struct intel_gt *gt = uc_to_gt(uc);
 	int ret;
 	u32 guc_status;
 
-	ret = intel_reset_guc(&dev_priv->gt);
+	ret = intel_reset_guc(gt);
 	if (ret) {
 		DRM_ERROR("Failed to reset GuC, ret = %d\n", ret);
 		return ret;
 	}
 
-	guc_status = I915_READ(GUC_STATUS);
+	guc_status = intel_uncore_read(gt->uncore, GUC_STATUS);
 	WARN(!(guc_status & GS_MIA_IN_RESET),
 	     "GuC status: 0x%x, MIA core expected to be in reset\n",
 	     guc_status);
-- 
2.22.0

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

[PATCH 2/9] drm/i915/uc: Unify uC platform check

[Daniele Ceraolo Spurio]

We have several HAS_* checks for GuC and HuC but we mostly use HAS_GUC
and HAS_HUC, with only 1 exception. Since our HW always has either
both uC or neither of them, just replace all the checks with a unified
HAS_UC.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_reset.c     |  2 +-
 drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c |  2 +-
 drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c |  2 +-
 drivers/gpu/drm/i915/gt/uc/intel_uc.c     |  2 +-
 drivers/gpu/drm/i915/gt/uc/selftest_guc.c |  4 ++--
 drivers/gpu/drm/i915/i915_debugfs.c       |  6 +++---
 drivers/gpu/drm/i915/i915_drv.h           | 15 ++-------------
 drivers/gpu/drm/i915/i915_gpu_error.c     |  4 ++--
 drivers/gpu/drm/i915/i915_irq.c           |  2 +-
 drivers/gpu/drm/i915/i915_pci.c           |  4 ++--
 drivers/gpu/drm/i915/intel_device_info.h  |  2 +-
 drivers/gpu/drm/i915/intel_pm.c           |  4 ++--
 drivers/gpu/drm/i915/intel_wopcm.c        |  4 ++--
 13 files changed, 21 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
index 55e2ddcbd215..7fd135a5a41f 100644
--- a/drivers/gpu/drm/i915/gt/intel_reset.c
+++ b/drivers/gpu/drm/i915/gt/intel_reset.c
@@ -595,7 +595,7 @@ int intel_reset_guc(struct intel_gt *gt)
 		INTEL_GEN(gt->i915) >= 11 ? GEN11_GRDOM_GUC : GEN9_GRDOM_GUC;
 	int ret;
 
-	GEM_BUG_ON(!HAS_GUC(gt->i915));
+	GEM_BUG_ON(!HAS_UC(gt->i915));
 
 	intel_uncore_forcewake_get(gt->uncore, FORCEWAKE_ALL);
 	ret = gen6_hw_domain_reset(gt, guc_domain);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
index 3dfa40fdbe99..ddd3b2162528 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
@@ -80,7 +80,7 @@ static void guc_fw_select(struct intel_uc_fw *guc_fw)
 
 	GEM_BUG_ON(guc_fw->type != INTEL_UC_FW_TYPE_GUC);
 
-	if (!HAS_GUC(i915)) {
+	if (!HAS_UC(i915)) {
 		guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
 		return;
 	}
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 543854c42d9d..15891cf4bb2c 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
@@ -74,7 +74,7 @@ static void huc_fw_select(struct intel_uc_fw *huc_fw)
 
 	GEM_BUG_ON(huc_fw->type != INTEL_UC_FW_TYPE_HUC);
 
-	if (!HAS_HUC(dev_priv)) {
+	if (!HAS_UC(dev_priv)) {
 		huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
 		return;
 	}
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 4480a3dc2449..0d50b73f6cfe 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -61,7 +61,7 @@ static int __get_platform_enable_guc(struct intel_uc *uc)
 	struct intel_uc_fw *huc_fw = &uc->huc.fw;
 	int enable_guc = 0;
 
-	if (!HAS_GUC(uc_to_gt(uc)->i915))
+	if (!HAS_UC(uc_to_gt(uc)->i915))
 		return 0;
 
 	/* We don't want to enable GuC/HuC on pre-Gen11 by default */
diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
index 93f7c930ab18..d1aa180050c4 100644
--- a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
@@ -134,7 +134,7 @@ static int igt_guc_clients(void *args)
 	struct intel_guc *guc;
 	int err = 0;
 
-	GEM_BUG_ON(!HAS_GUC(dev_priv));
+	GEM_BUG_ON(!HAS_UC(dev_priv));
 	mutex_lock(&dev_priv->drm.struct_mutex);
 	wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
 
@@ -226,7 +226,7 @@ static int igt_guc_doorbells(void *arg)
 	int i, err = 0;
 	u16 db_id;
 
-	GEM_BUG_ON(!HAS_GUC(dev_priv));
+	GEM_BUG_ON(!HAS_UC(dev_priv));
 	mutex_lock(&dev_priv->drm.struct_mutex);
 	wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
 
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 6b84d04a6a28..4249107f9d0d 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1865,7 +1865,7 @@ static int i915_huc_load_status_info(struct seq_file *m, void *data)
 	intel_wakeref_t wakeref;
 	struct drm_printer p;
 
-	if (!HAS_HUC(dev_priv))
+	if (!HAS_UC(dev_priv))
 		return -ENODEV;
 
 	p = drm_seq_file_printer(m);
@@ -1883,7 +1883,7 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
 	intel_wakeref_t wakeref;
 	struct drm_printer p;
 
-	if (!HAS_GUC(dev_priv))
+	if (!HAS_UC(dev_priv))
 		return -ENODEV;
 
 	p = drm_seq_file_printer(m);
@@ -2062,7 +2062,7 @@ static int i915_guc_log_dump(struct seq_file *m, void *data)
 	u32 *log;
 	int i = 0;
 
-	if (!HAS_GUC(dev_priv))
+	if (!HAS_UC(dev_priv))
 		return -ENODEV;
 
 	if (dump_load_err)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0e44cc4b2ca1..06ddc5df12fa 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2271,20 +2271,9 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
 
 #define HAS_IPC(dev_priv)		 (INTEL_INFO(dev_priv)->display.has_ipc)
 
-/*
- * For now, anything with a GuC requires uCode loading, and then supports
- * command submission once loaded. But these are logically independent
- * properties, so we have separate macros to test them.
- */
-#define HAS_GUC(dev_priv)	(INTEL_INFO(dev_priv)->has_guc)
-#define HAS_GUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
-#define HAS_GUC_SCHED(dev_priv)	(HAS_GUC(dev_priv))
-
-/* For now, anything with a GuC has also HuC */
-#define HAS_HUC(dev_priv)	(HAS_GUC(dev_priv))
-#define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
+#define HAS_UC(dev_priv)	(INTEL_INFO(dev_priv)->has_uc)
 
-/* Having a GuC is not the same as using a GuC */
+/* Having a uC is not the same as using a uC */
 #define USES_GUC(dev_priv)		intel_uc_is_using_guc(&(dev_priv)->gt.uc)
 #define USES_GUC_SUBMISSION(dev_priv)	intel_uc_is_using_guc_submission(&(dev_priv)->gt.uc)
 #define USES_HUC(dev_priv)		intel_uc_is_using_huc(&(dev_priv)->gt.uc)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index c5b89bf4d616..1db4d7302734 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -620,7 +620,7 @@ static void err_print_uc(struct drm_i915_error_state_buf *m,
 	const struct i915_gpu_state *error =
 		container_of(error_uc, typeof(*error), uc);
 
-	if (!error->device_info.has_guc)
+	if (!error->device_info.has_uc)
 		return;
 
 	intel_uc_fw_dump(&error_uc->guc_fw, &p);
@@ -1557,7 +1557,7 @@ static void capture_uc_state(struct i915_gpu_state *error)
 	struct intel_uc *uc = &i915->gt.uc;
 
 	/* Capturing uC state won't be useful if there is no GuC */
-	if (!error->device_info.has_guc)
+	if (!error->device_info.has_uc)
 		return;
 
 	error_uc->guc_fw = uc->guc.fw;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 11c73af92597..0b2351a6c490 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4766,7 +4766,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 		dev_priv->l3_parity.remap_info[i] = NULL;
 
 	/* pre-gen11 the guc irqs bits are in the upper 16 bits of the pm reg */
-	if (HAS_GUC_SCHED(dev_priv) && INTEL_GEN(dev_priv) < 11)
+	if (HAS_UC(dev_priv) && INTEL_GEN(dev_priv) < 11)
 		dev_priv->gt.pm_guc_events = GUC_INTR_GUC2HOST << 16;
 
 	/* Let's track the enabled rps events */
diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index 40076ba431d4..b5ece92683fd 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -595,7 +595,7 @@ static const struct intel_device_info intel_cherryview_info = {
 	GEN9_DEFAULT_PAGE_SIZES, \
 	.has_logical_ring_preemption = 1, \
 	.display.has_csr = 1, \
-	.has_guc = 1, \
+	.has_uc = 1, \
 	.display.has_ipc = 1, \
 	.ddb_size = 896
 
@@ -647,7 +647,7 @@ static const struct intel_device_info intel_skylake_gt4_info = {
 	.display.has_dp_mst = 1, \
 	.has_logical_ring_contexts = 1, \
 	.has_logical_ring_preemption = 1, \
-	.has_guc = 1, \
+	.has_uc = 1, \
 	.ppgtt_type = INTEL_PPGTT_FULL, \
 	.ppgtt_size = 48, \
 	.has_reset_engine = 1, \
diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
index 45a9badc9b8e..99e5b22e26f6 100644
--- a/drivers/gpu/drm/i915/intel_device_info.h
+++ b/drivers/gpu/drm/i915/intel_device_info.h
@@ -112,7 +112,6 @@ enum intel_ppgtt_type {
 	func(gpu_reset_clobbers_display); \
 	func(has_reset_engine); \
 	func(has_fpga_dbg); \
-	func(has_guc); \
 	func(has_l3_dpf); \
 	func(has_llc); \
 	func(has_logical_ring_contexts); \
@@ -124,6 +123,7 @@ enum intel_ppgtt_type {
 	func(has_rps); \
 	func(has_runtime_pm); \
 	func(has_snoop); \
+	func(has_uc); \
 	func(has_coherent_ggtt); \
 	func(unfenced_needs_alignment); \
 	func(hws_needs_physical);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 22472f2bd31b..53ef862731f6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7162,7 +7162,7 @@ static void gen11_enable_rc6(struct drm_i915_private *dev_priv)
 	for_each_engine(engine, dev_priv, id)
 		I915_WRITE(RING_MAX_IDLE(engine->mmio_base), 10);
 
-	if (HAS_GUC(dev_priv))
+	if (HAS_UC(dev_priv))
 		I915_WRITE(GUC_MAX_IDLE_COUNT, 0xA);
 
 	I915_WRITE(GEN6_RC_SLEEP, 0);
@@ -7243,7 +7243,7 @@ static void gen9_enable_rc6(struct drm_i915_private *dev_priv)
 	for_each_engine(engine, dev_priv, id)
 		I915_WRITE(RING_MAX_IDLE(engine->mmio_base), 10);
 
-	if (HAS_GUC(dev_priv))
+	if (HAS_UC(dev_priv))
 		I915_WRITE(GUC_MAX_IDLE_COUNT, 0xA);
 
 	I915_WRITE(GEN6_RC_SLEEP, 0);
diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
index fafd4e6a1147..c966d572b0c4 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -74,7 +74,7 @@ void intel_wopcm_init_early(struct intel_wopcm *wopcm)
 {
 	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
 
-	if (!HAS_GUC(i915))
+	if (!HAS_UC(i915))
 		return;
 
 	if (INTEL_GEN(i915) >= 11)
@@ -263,7 +263,7 @@ int intel_wopcm_init_hw(struct intel_wopcm *wopcm, struct intel_gt *gt)
 	if (!USES_GUC(i915))
 		return 0;
 
-	GEM_BUG_ON(!HAS_GUC(i915));
+	GEM_BUG_ON(!HAS_UC(i915));
 	GEM_BUG_ON(!wopcm->guc.size);
 	GEM_BUG_ON(!wopcm->guc.base);
 
-- 
2.22.0

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

[PATCH 3/9] drm/i915/uc: Unify uC FW selection

[Daniele Ceraolo Spurio]

Instead of having 2 identical functions for GuC and HuC firmware
selection, we can unify the selection logic and just use different lists
based on FW type.

Note that the revid is not relevant for current blobs, but the upcoming
CML will be identified as CFL rev 5, so by considering the revid we're
ready for that.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c | 109 ++++++----------------
 drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 102 +++++---------------
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c  |  47 ++++++++++
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h  |  27 +++++-
 4 files changed, 121 insertions(+), 164 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
index ddd3b2162528..86df5147fc0a 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
@@ -31,88 +31,32 @@
 #include "intel_guc_fw.h"
 #include "i915_drv.h"
 
-#define __MAKE_GUC_FW_PATH(KEY) \
+#define __MAKE_GUC_FW_PATH(prefix_, major_, minor_, patch_) \
 	"i915/" \
-	__stringify(KEY##_GUC_FW_PREFIX) "_guc_" \
-	__stringify(KEY##_GUC_FW_MAJOR) "." \
-	__stringify(KEY##_GUC_FW_MINOR) "." \
-	__stringify(KEY##_GUC_FW_PATCH) ".bin"
-
-#define SKL_GUC_FW_PREFIX skl
-#define SKL_GUC_FW_MAJOR 33
-#define SKL_GUC_FW_MINOR 0
-#define SKL_GUC_FW_PATCH 0
-#define SKL_GUC_FIRMWARE_PATH __MAKE_GUC_FW_PATH(SKL)
-MODULE_FIRMWARE(SKL_GUC_FIRMWARE_PATH);
-
-#define BXT_GUC_FW_PREFIX bxt
-#define BXT_GUC_FW_MAJOR 33
-#define BXT_GUC_FW_MINOR 0
-#define BXT_GUC_FW_PATCH 0
-#define BXT_GUC_FIRMWARE_PATH __MAKE_GUC_FW_PATH(BXT)
-MODULE_FIRMWARE(BXT_GUC_FIRMWARE_PATH);
-
-#define KBL_GUC_FW_PREFIX kbl
-#define KBL_GUC_FW_MAJOR 33
-#define KBL_GUC_FW_MINOR 0
-#define KBL_GUC_FW_PATCH 0
-#define KBL_GUC_FIRMWARE_PATH __MAKE_GUC_FW_PATH(KBL)
-MODULE_FIRMWARE(KBL_GUC_FIRMWARE_PATH);
-
-#define GLK_GUC_FW_PREFIX glk
-#define GLK_GUC_FW_MAJOR 33
-#define GLK_GUC_FW_MINOR 0
-#define GLK_GUC_FW_PATCH 0
-#define GLK_GUC_FIRMWARE_PATH __MAKE_GUC_FW_PATH(GLK)
-MODULE_FIRMWARE(GLK_GUC_FIRMWARE_PATH);
-
-#define ICL_GUC_FW_PREFIX icl
-#define ICL_GUC_FW_MAJOR 33
-#define ICL_GUC_FW_MINOR 0
-#define ICL_GUC_FW_PATCH 0
-#define ICL_GUC_FIRMWARE_PATH __MAKE_GUC_FW_PATH(ICL)
-MODULE_FIRMWARE(ICL_GUC_FIRMWARE_PATH);
-
-static void guc_fw_select(struct intel_uc_fw *guc_fw)
-{
-	struct intel_guc *guc = container_of(guc_fw, struct intel_guc, fw);
-	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
-
-	GEM_BUG_ON(guc_fw->type != INTEL_UC_FW_TYPE_GUC);
-
-	if (!HAS_UC(i915)) {
-		guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
-		return;
-	}
-
-	guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
-
-	if (i915_modparams.guc_firmware_path) {
-		guc_fw->path = i915_modparams.guc_firmware_path;
-		guc_fw->major_ver_wanted = 0;
-		guc_fw->minor_ver_wanted = 0;
-	} else if (IS_ICELAKE(i915)) {
-		guc_fw->path = ICL_GUC_FIRMWARE_PATH;
-		guc_fw->major_ver_wanted = ICL_GUC_FW_MAJOR;
-		guc_fw->minor_ver_wanted = ICL_GUC_FW_MINOR;
-	} else if (IS_GEMINILAKE(i915)) {
-		guc_fw->path = GLK_GUC_FIRMWARE_PATH;
-		guc_fw->major_ver_wanted = GLK_GUC_FW_MAJOR;
-		guc_fw->minor_ver_wanted = GLK_GUC_FW_MINOR;
-	} else if (IS_KABYLAKE(i915) || IS_COFFEELAKE(i915)) {
-		guc_fw->path = KBL_GUC_FIRMWARE_PATH;
-		guc_fw->major_ver_wanted = KBL_GUC_FW_MAJOR;
-		guc_fw->minor_ver_wanted = KBL_GUC_FW_MINOR;
-	} else if (IS_BROXTON(i915)) {
-		guc_fw->path = BXT_GUC_FIRMWARE_PATH;
-		guc_fw->major_ver_wanted = BXT_GUC_FW_MAJOR;
-		guc_fw->minor_ver_wanted = BXT_GUC_FW_MINOR;
-	} else if (IS_SKYLAKE(i915)) {
-		guc_fw->path = SKL_GUC_FIRMWARE_PATH;
-		guc_fw->major_ver_wanted = SKL_GUC_FW_MAJOR;
-		guc_fw->minor_ver_wanted = SKL_GUC_FW_MINOR;
-	}
-}
+	__stringify(prefix_) "_guc_" \
+	__stringify(major_) "." \
+	__stringify(minor_) "." \
+	__stringify(patch_) ".bin"
+
+#define GUC_FW_BLOB(prefix_, major_, minor_, patch_) \
+UC_FW_BLOB(prefix_##_guc, major_, minor_, \
+	   __MAKE_GUC_FW_PATH(prefix_, major_, minor_, patch_))
+
+GUC_FW_BLOB(skl, 33, 0, 0);
+GUC_FW_BLOB(bxt, 33, 0, 0);
+GUC_FW_BLOB(kbl, 33, 0, 0);
+GUC_FW_BLOB(glk, 33, 0, 0);
+GUC_FW_BLOB(icl, 33, 0, 0);
+
+/* must be ordered base on platform + revid, from newer to older */
+static const struct intel_uc_platform_requirement guc_fw_blobs[] = {
+	{ INTEL_ICELAKE,	0,	&icl_guc_fw_blob },
+	{ INTEL_COFFEELAKE,	0,	&kbl_guc_fw_blob },
+	{ INTEL_GEMINILAKE,	0,	&glk_guc_fw_blob },
+	{ INTEL_KABYLAKE,	0,	&kbl_guc_fw_blob },
+	{ INTEL_BROXTON,	0,	&bxt_guc_fw_blob },
+	{ INTEL_SKYLAKE,	0,	&skl_guc_fw_blob },
+};
 
 /**
  * intel_guc_fw_init_early() - initializes GuC firmware struct
@@ -125,7 +69,8 @@ void intel_guc_fw_init_early(struct intel_guc *guc)
 	struct intel_uc_fw *guc_fw = &guc->fw;
 
 	intel_uc_fw_init_early(guc_fw, INTEL_UC_FW_TYPE_GUC);
-	guc_fw_select(guc_fw);
+	intel_uc_fw_select(guc_to_gt(guc)->i915, guc_fw,
+			   guc_fw_blobs, ARRAY_SIZE(guc_fw_blobs));
 }
 
 static void guc_prepare_xfer(struct intel_guc *guc)
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 15891cf4bb2c..263977294ef0 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
@@ -23,90 +23,29 @@
  * Note that HuC firmware loading must be done before GuC loading.
  */
 
-#define BXT_HUC_FW_MAJOR 01
-#define BXT_HUC_FW_MINOR 8
-#define BXT_BLD_NUM 2893
-
-#define SKL_HUC_FW_MAJOR 01
-#define SKL_HUC_FW_MINOR 07
-#define SKL_BLD_NUM 1398
-
-#define KBL_HUC_FW_MAJOR 02
-#define KBL_HUC_FW_MINOR 00
-#define KBL_BLD_NUM 1810
-
-#define GLK_HUC_FW_MAJOR 03
-#define GLK_HUC_FW_MINOR 01
-#define GLK_BLD_NUM 2893
-
-#define ICL_HUC_FW_MAJOR 8
-#define ICL_HUC_FW_MINOR 4
-#define ICL_BLD_NUM 3238
-
 #define HUC_FW_PATH(platform, major, minor, bld_num) \
 	"i915/" __stringify(platform) "_huc_ver" __stringify(major) "_" \
 	__stringify(minor) "_" __stringify(bld_num) ".bin"
 
-#define I915_SKL_HUC_UCODE HUC_FW_PATH(skl, SKL_HUC_FW_MAJOR, \
-	SKL_HUC_FW_MINOR, SKL_BLD_NUM)
-MODULE_FIRMWARE(I915_SKL_HUC_UCODE);
-
-#define I915_BXT_HUC_UCODE HUC_FW_PATH(bxt, BXT_HUC_FW_MAJOR, \
-	BXT_HUC_FW_MINOR, BXT_BLD_NUM)
-MODULE_FIRMWARE(I915_BXT_HUC_UCODE);
-
-#define I915_KBL_HUC_UCODE HUC_FW_PATH(kbl, KBL_HUC_FW_MAJOR, \
-	KBL_HUC_FW_MINOR, KBL_BLD_NUM)
-MODULE_FIRMWARE(I915_KBL_HUC_UCODE);
-
-#define I915_GLK_HUC_UCODE HUC_FW_PATH(glk, GLK_HUC_FW_MAJOR, \
-	GLK_HUC_FW_MINOR, GLK_BLD_NUM)
-MODULE_FIRMWARE(I915_GLK_HUC_UCODE);
-
-#define I915_ICL_HUC_UCODE HUC_FW_PATH(icl, ICL_HUC_FW_MAJOR, \
-	ICL_HUC_FW_MINOR, ICL_BLD_NUM)
-MODULE_FIRMWARE(I915_ICL_HUC_UCODE);
-
-static void huc_fw_select(struct intel_uc_fw *huc_fw)
-{
-	struct intel_huc *huc = container_of(huc_fw, struct intel_huc, fw);
-	struct drm_i915_private *dev_priv = huc_to_gt(huc)->i915;
-
-	GEM_BUG_ON(huc_fw->type != INTEL_UC_FW_TYPE_HUC);
-
-	if (!HAS_UC(dev_priv)) {
-		huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
-		return;
-	}
-
-	huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
-
-	if (i915_modparams.huc_firmware_path) {
-		huc_fw->path = i915_modparams.huc_firmware_path;
-		huc_fw->major_ver_wanted = 0;
-		huc_fw->minor_ver_wanted = 0;
-	} else if (IS_SKYLAKE(dev_priv)) {
-		huc_fw->path = I915_SKL_HUC_UCODE;
-		huc_fw->major_ver_wanted = SKL_HUC_FW_MAJOR;
-		huc_fw->minor_ver_wanted = SKL_HUC_FW_MINOR;
-	} else if (IS_BROXTON(dev_priv)) {
-		huc_fw->path = I915_BXT_HUC_UCODE;
-		huc_fw->major_ver_wanted = BXT_HUC_FW_MAJOR;
-		huc_fw->minor_ver_wanted = BXT_HUC_FW_MINOR;
-	} else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) {
-		huc_fw->path = I915_KBL_HUC_UCODE;
-		huc_fw->major_ver_wanted = KBL_HUC_FW_MAJOR;
-		huc_fw->minor_ver_wanted = KBL_HUC_FW_MINOR;
-	} else if (IS_GEMINILAKE(dev_priv)) {
-		huc_fw->path = I915_GLK_HUC_UCODE;
-		huc_fw->major_ver_wanted = GLK_HUC_FW_MAJOR;
-		huc_fw->minor_ver_wanted = GLK_HUC_FW_MINOR;
-	} else if (IS_ICELAKE(dev_priv)) {
-		huc_fw->path = I915_ICL_HUC_UCODE;
-		huc_fw->major_ver_wanted = ICL_HUC_FW_MAJOR;
-		huc_fw->minor_ver_wanted = ICL_HUC_FW_MINOR;
-	}
-}
+#define HUC_FW_BLOB(prefix_, major_, minor_, bld_num_) \
+UC_FW_BLOB(prefix_##_huc, major_, minor_, \
+	   HUC_FW_PATH(prefix_, major_, minor_, bld_num_))
+
+HUC_FW_BLOB(skl, 01, 07, 1398);
+HUC_FW_BLOB(bxt, 01,  8, 2893);
+HUC_FW_BLOB(kbl, 02, 00, 1810);
+HUC_FW_BLOB(glk, 03, 01, 2893);
+HUC_FW_BLOB(icl,  8,  4, 3238);
+
+/* must be ordered base on platform + revid, from newer to older */
+static const struct intel_uc_platform_requirement huc_fw_blobs[] = {
+	{ INTEL_ICELAKE,	0,	&icl_huc_fw_blob },
+	{ INTEL_COFFEELAKE,	0,	&kbl_huc_fw_blob },
+	{ INTEL_GEMINILAKE,	0,	&glk_huc_fw_blob },
+	{ INTEL_KABYLAKE,	0,	&kbl_huc_fw_blob },
+	{ INTEL_BROXTON,	0,	&bxt_huc_fw_blob },
+	{ INTEL_SKYLAKE,	0,	&skl_huc_fw_blob },
+};
 
 /**
  * intel_huc_fw_init_early() - initializes HuC firmware struct
@@ -119,7 +58,8 @@ void intel_huc_fw_init_early(struct intel_huc *huc)
 	struct intel_uc_fw *huc_fw = &huc->fw;
 
 	intel_uc_fw_init_early(huc_fw, INTEL_UC_FW_TYPE_HUC);
-	huc_fw_select(huc_fw);
+	intel_uc_fw_select(huc_to_gt(huc)->i915, huc_fw,
+			   huc_fw_blobs, ARRAY_SIZE(huc_fw_blobs));
 }
 
 static void huc_xfer_rsa(struct intel_huc *huc)
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 8ce7210907c0..faa96748aed3 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -29,6 +29,53 @@
 #include "intel_uc_fw.h"
 #include "i915_drv.h"
 
+/**
+ * intel_uc_fw_select - select the uC firmware to fetch & load
+ * @i915: device private
+ * @uc_fw: uC firmware
+ * @list: list of required firmware for each platform
+ * @length: number of entries in the list
+ *
+ * Select the uC firmware from the provided list based on platform and revid of
+ * the device we're on. If the firmware_path modparam override is set, it takes
+ * precedence over the entries in the list.
+ */
+void intel_uc_fw_select(struct drm_i915_private *i915,
+			struct intel_uc_fw *uc_fw,
+			const struct intel_uc_platform_requirement *list,
+			unsigned int length)
+{
+	GEM_BUG_ON(uc_fw->fetch_status != INTEL_UC_FIRMWARE_UNINITIALIZED);
+
+	if (!HAS_UC(i915)) {
+		uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
+		return;
+	}
+
+	uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
+
+	if (unlikely(i915_modparams.guc_firmware_path &&
+		     uc_fw->type == INTEL_UC_FW_TYPE_GUC)) {
+		uc_fw->path = i915_modparams.guc_firmware_path;
+	} else if (unlikely(i915_modparams.huc_firmware_path &&
+			    uc_fw->type == INTEL_UC_FW_TYPE_HUC)) {
+		uc_fw->path = i915_modparams.huc_firmware_path;
+	} else {
+		enum intel_platform p = INTEL_INFO(i915)->platform;
+		u8 rev = INTEL_REVID(i915);
+		int i;
+
+		for (i = 0; i < length && p <= list[i].p; i++) {
+			if (p == list[i].p && rev >= list[i].first_rev) {
+				uc_fw->path = list[i].blob->path;
+				uc_fw->major_ver_wanted = list[i].blob->major;
+				uc_fw->minor_ver_wanted = list[i].blob->minor;
+				break;
+			}
+		}
+	}
+}
+
 /**
  * intel_uc_fw_fetch - fetch uC firmware
  *
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 833d04d06576..550b626afb15 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
@@ -26,6 +26,7 @@
 #define _INTEL_UC_FW_H_
 
 #include <linux/types.h>
+#include "intel_device_info.h"
 #include "i915_gem.h"
 
 struct drm_printer;
@@ -48,6 +49,26 @@ enum intel_uc_fw_type {
 	INTEL_UC_FW_TYPE_HUC
 };
 
+struct __packed intel_uc_fw_blob {
+	u8 major;
+	u8 minor;
+	const char *path;
+};
+
+#define UC_FW_BLOB(prefix_, major_, minor_, path_) \
+static const struct intel_uc_fw_blob prefix_##_fw_blob = { \
+	.major = (major_), \
+	.minor = (minor_), \
+	.path = path_ \
+}; \
+MODULE_FIRMWARE(path_)
+
+struct __packed intel_uc_platform_requirement {
+	enum intel_platform p;
+	u8 first_rev;
+	const struct intel_uc_fw_blob *blob;
+};
+
 /*
  * This structure encapsulates all the data needed during the process
  * of fetching, caching, and loading the firmware image into the uC.
@@ -164,7 +185,11 @@ static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw)
 	return uc_fw->header_size + uc_fw->ucode_size;
 }
 
-void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
+void intel_uc_fw_select(struct drm_i915_private *i915,
+			struct intel_uc_fw *uc_fw,
+			const struct intel_uc_platform_requirement *blobs_list,
+			unsigned int length);
+void intel_uc_fw_fetch(struct drm_i915_private *i915,
 		       struct intel_uc_fw *uc_fw);
 void intel_uc_fw_cleanup_fetch(struct intel_uc_fw *uc_fw);
 int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
-- 
2.22.0

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

[PATCH 4/9] drm/i915/uc: Sanitize uC when GT is sanitized

[Daniele Ceraolo Spurio]

The microcontrollers are part of GT so it makes logical sense to have
them sanitized at the same time. This also fixed an issue with our
status tracking where the FW load status is not reset around
hibernation.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gem/i915_gem_pm.c | 1 -
 drivers/gpu/drm/i915/gt/intel_gt_pm.c  | 2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
index 8faf262278ae..b5561cbdc5ea 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
@@ -239,7 +239,6 @@ void i915_gem_suspend_late(struct drm_i915_private *i915)
 	}
 	spin_unlock_irqrestore(&i915->mm.obj_lock, flags);
 
-	intel_uc_sanitize(&i915->gt.uc);
 	i915_gem_sanitize(i915);
 }
 
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
index 61ed912341f1..65c0d0c9d543 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
@@ -118,6 +118,8 @@ void intel_gt_sanitize(struct intel_gt *gt, bool force)
 
 	GEM_TRACE("\n");
 
+	intel_uc_sanitize(&gt->uc);
+
 	if (!reset_engines(gt) && !force)
 		return;
 
-- 
2.22.0

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

[PATCH 5/9] drm/i915/uc: Unify uc_fw status tracking

[Daniele Ceraolo Spurio]

We currently track fetch and load status separately, but the 2 are
actually sequential in the uc lifetime (fetch must complete before we
can attempt the load!). Unifying the 2 variables we can better follow
the sequential states and improve our trackng of the uC state.

Also, sprinkle some GEM_BUG_ON to make sure we transition correctly
between states.

Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/uc/intel_huc.c   |  4 +-
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 74 ++++++++++--------------
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 50 ++++++++--------
 3 files changed, 57 insertions(+), 71 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
index bc14439173d7..1868f676d890 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
@@ -117,7 +117,7 @@ int intel_huc_auth(struct intel_huc *huc)
 	struct intel_guc *guc = &gt->uc.guc;
 	int ret;
 
-	if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
+	if (huc->fw.status != INTEL_UC_FIRMWARE_LOADED)
 		return -ENOEXEC;
 
 	ret = intel_guc_auth_huc(guc,
@@ -141,7 +141,7 @@ int intel_huc_auth(struct intel_huc *huc)
 	return 0;
 
 fail:
-	huc->fw.load_status = INTEL_UC_FIRMWARE_FAIL;
+	huc->fw.status = INTEL_UC_FIRMWARE_LOAD_FAIL;
 
 	DRM_ERROR("HuC: Authentication failed %d\n", ret);
 	return ret;
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 faa96748aed3..91b1d9663481 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -45,15 +45,13 @@ void intel_uc_fw_select(struct drm_i915_private *i915,
 			const struct intel_uc_platform_requirement *list,
 			unsigned int length)
 {
-	GEM_BUG_ON(uc_fw->fetch_status != INTEL_UC_FIRMWARE_UNINITIALIZED);
+	GEM_BUG_ON(uc_fw->status != INTEL_UC_FIRMWARE_UNINITIALIZED);
 
 	if (!HAS_UC(i915)) {
-		uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
+		uc_fw->status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
 		return;
 	}
 
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
-
 	if (unlikely(i915_modparams.guc_firmware_path &&
 		     uc_fw->type == INTEL_UC_FW_TYPE_GUC)) {
 		uc_fw->path = i915_modparams.guc_firmware_path;
@@ -74,6 +72,8 @@ void intel_uc_fw_select(struct drm_i915_private *i915,
 			}
 		}
 	}
+
+	uc_fw->status = INTEL_UC_FIRMWARE_SELECTION_DONE;
 }
 
 /**
@@ -95,23 +95,11 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 	int err;
 
 	GEM_BUG_ON(!intel_uc_fw_supported(uc_fw));
-
-	if (!uc_fw->path) {
-		dev_info(dev_priv->drm.dev,
-			 "%s: No firmware was defined for %s!\n",
-			 intel_uc_fw_type_repr(uc_fw->type),
-			 intel_platform_name(INTEL_INFO(dev_priv)->platform));
-		return;
-	}
+	GEM_BUG_ON(!intel_uc_fw_is_selected(uc_fw));
 
 	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
 			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
 
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
-	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
-			 intel_uc_fw_type_repr(uc_fw->type),
-			 intel_uc_fw_status_repr(uc_fw->fetch_status));
-
 	err = request_firmware(&fw, uc_fw->path, &pdev->dev);
 	if (err) {
 		DRM_DEBUG_DRIVER("%s fw request_firmware err=%d\n",
@@ -219,19 +207,17 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 
 	uc_fw->obj = obj;
 	uc_fw->size = fw->size;
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
-	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
-			 intel_uc_fw_type_repr(uc_fw->type),
-			 intel_uc_fw_status_repr(uc_fw->fetch_status));
+	uc_fw->status = INTEL_UC_FIRMWARE_FETCHED;
+	DRM_DEBUG_DRIVER("%s fw fetch done\n",
+			 intel_uc_fw_type_repr(uc_fw->type));
 
 	release_firmware(fw);
 	return;
 
 fail:
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_FAIL;
-	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
-			 intel_uc_fw_type_repr(uc_fw->type),
-			 intel_uc_fw_status_repr(uc_fw->fetch_status));
+	uc_fw->status = INTEL_UC_FIRMWARE_FETCH_FAIL;
+	DRM_DEBUG_DRIVER("%s fw fetch failed\n",
+			 intel_uc_fw_type_repr(uc_fw->type));
 
 	DRM_WARN("%s: Failed to fetch firmware %s (error %d)\n",
 		 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
@@ -287,13 +273,11 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
 	DRM_DEBUG_DRIVER("%s fw load %s\n",
 			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
 
-	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
-		return -ENOEXEC;
+	/* make sure the status was cleared the last time we reset the uc */
+	GEM_BUG_ON(uc_fw->status == INTEL_UC_FIRMWARE_LOADED);
 
-	uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
-	DRM_DEBUG_DRIVER("%s fw load %s\n",
-			 intel_uc_fw_type_repr(uc_fw->type),
-			 intel_uc_fw_status_repr(uc_fw->load_status));
+	if (uc_fw->status < INTEL_UC_FIRMWARE_FETCHED)
+		return -ENOEXEC;
 
 	/* Call custom loader */
 	intel_uc_fw_ggtt_bind(uc_fw);
@@ -302,10 +286,9 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
 	if (err)
 		goto fail;
 
-	uc_fw->load_status = INTEL_UC_FIRMWARE_SUCCESS;
-	DRM_DEBUG_DRIVER("%s fw load %s\n",
-			 intel_uc_fw_type_repr(uc_fw->type),
-			 intel_uc_fw_status_repr(uc_fw->load_status));
+	uc_fw->status = INTEL_UC_FIRMWARE_LOADED;
+	DRM_DEBUG_DRIVER("%s fw load completed\n",
+			 intel_uc_fw_type_repr(uc_fw->type));
 
 	DRM_INFO("%s: Loaded firmware %s (version %u.%u)\n",
 		 intel_uc_fw_type_repr(uc_fw->type),
@@ -315,10 +298,9 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
 	return 0;
 
 fail:
-	uc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;
-	DRM_DEBUG_DRIVER("%s fw load %s\n",
-			 intel_uc_fw_type_repr(uc_fw->type),
-			 intel_uc_fw_status_repr(uc_fw->load_status));
+	uc_fw->status = INTEL_UC_FIRMWARE_LOAD_FAIL;
+	DRM_DEBUG_DRIVER("%s fw load failed\n",
+			 intel_uc_fw_type_repr(uc_fw->type));
 
 	DRM_WARN("%s: Failed to load firmware %s (error %d)\n",
 		 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
@@ -330,7 +312,10 @@ int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
 {
 	int err;
 
-	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
+	/* this should happen before the load! */
+	GEM_BUG_ON(uc_fw->status == INTEL_UC_FIRMWARE_LOADED);
+
+	if (uc_fw->status < INTEL_UC_FIRMWARE_FETCHED)
 		return -ENOEXEC;
 
 	err = i915_gem_object_pin_pages(uc_fw->obj);
@@ -343,7 +328,7 @@ int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
 
 void intel_uc_fw_fini(struct intel_uc_fw *uc_fw)
 {
-	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
+	if (uc_fw->status < INTEL_UC_FIRMWARE_FETCHED)
 		return;
 
 	i915_gem_object_unpin_pages(uc_fw->obj);
@@ -377,7 +362,7 @@ void intel_uc_fw_cleanup_fetch(struct intel_uc_fw *uc_fw)
 	if (obj)
 		i915_gem_object_put(obj);
 
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
+	uc_fw->status = INTEL_UC_FIRMWARE_SELECTION_DONE;
 }
 
 /**
@@ -391,9 +376,8 @@ void intel_uc_fw_dump(const struct intel_uc_fw *uc_fw, struct drm_printer *p)
 {
 	drm_printf(p, "%s firmware: %s\n",
 		   intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
-	drm_printf(p, "\tstatus: fetch %s, load %s\n",
-		   intel_uc_fw_status_repr(uc_fw->fetch_status),
-		   intel_uc_fw_status_repr(uc_fw->load_status));
+	drm_printf(p, "\tstatus: %s\n",
+		   intel_uc_fw_status_repr(uc_fw->status));
 	drm_printf(p, "\tversion: wanted %u.%u, found %u.%u\n",
 		   uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted,
 		   uc_fw->major_ver_found, uc_fw->minor_ver_found);
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 550b626afb15..e0c5a38685e6 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
@@ -36,12 +36,13 @@ struct drm_i915_private;
 #define INTEL_UC_FIRMWARE_URL "https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/i915"
 
 enum intel_uc_fw_status {
-	INTEL_UC_FIRMWARE_NOT_SUPPORTED = -2, /* no uc HW */
-	INTEL_UC_FIRMWARE_FAIL = -1,
+	INTEL_UC_FIRMWARE_LOAD_FAIL = -3,
+	INTEL_UC_FIRMWARE_FETCH_FAIL = -2,
+	INTEL_UC_FIRMWARE_NOT_SUPPORTED = -1, /* no uc HW */
 	INTEL_UC_FIRMWARE_UNINITIALIZED = 0, /* used to catch checks done too early */
-	INTEL_UC_FIRMWARE_NOT_STARTED = 1,
-	INTEL_UC_FIRMWARE_PENDING,
-	INTEL_UC_FIRMWARE_SUCCESS
+	INTEL_UC_FIRMWARE_SELECTION_DONE, /* selection include the "no FW" case */
+	INTEL_UC_FIRMWARE_FETCHED,
+	INTEL_UC_FIRMWARE_LOADED
 };
 
 enum intel_uc_fw_type {
@@ -77,8 +78,7 @@ struct intel_uc_fw {
 	const char *path;
 	size_t size;
 	struct drm_i915_gem_object *obj;
-	enum intel_uc_fw_status fetch_status;
-	enum intel_uc_fw_status load_status;
+	enum intel_uc_fw_status status;
 
 	/*
 	 * The firmware build process will generate a version header file with major and
@@ -103,18 +103,20 @@ static inline
 const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
 {
 	switch (status) {
+	case INTEL_UC_FIRMWARE_LOAD_FAIL:
+		return "LOAD FAIL";
+	case INTEL_UC_FIRMWARE_FETCH_FAIL:
+		return "FETCH FAIL";
 	case INTEL_UC_FIRMWARE_NOT_SUPPORTED:
-		return "N/A - uc HW not available";
-	case INTEL_UC_FIRMWARE_FAIL:
-		return "FAIL";
+		return "N/A";
 	case INTEL_UC_FIRMWARE_UNINITIALIZED:
 		return "UNINITIALIZED";
-	case INTEL_UC_FIRMWARE_NOT_STARTED:
-		return "NOT_STARTED";
-	case INTEL_UC_FIRMWARE_PENDING:
-		return "PENDING";
-	case INTEL_UC_FIRMWARE_SUCCESS:
-		return "SUCCESS";
+	case INTEL_UC_FIRMWARE_SELECTION_DONE:
+		return "SELECTION DONE";
+	case INTEL_UC_FIRMWARE_FETCHED:
+		return "FETCHED";
+	case INTEL_UC_FIRMWARE_LOADED:
+		return "LOADED";
 	}
 	return "<invalid>";
 }
@@ -135,38 +137,38 @@ void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
 			    enum intel_uc_fw_type type)
 {
 	/*
-	 * we use FIRMWARE_UNINITIALIZED to detect checks against fetch_status
+	 * we use FIRMWARE_UNINITIALIZED to detect checks against uc_fw->status
 	 * before we're looked at the HW caps to see if we have uc support
 	 */
 	BUILD_BUG_ON(INTEL_UC_FIRMWARE_UNINITIALIZED);
 
 	uc_fw->path = NULL;
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_UNINITIALIZED;
-	uc_fw->load_status = INTEL_UC_FIRMWARE_NOT_STARTED;
+	uc_fw->status = INTEL_UC_FIRMWARE_UNINITIALIZED;
 	uc_fw->type = type;
 }
 
 static inline bool intel_uc_fw_is_selected(struct intel_uc_fw *uc_fw)
 {
+	GEM_BUG_ON(uc_fw->path && uc_fw->status < INTEL_UC_FIRMWARE_SELECTION_DONE);
 	return uc_fw->path != NULL;
 }
 
 static inline bool intel_uc_fw_is_loaded(struct intel_uc_fw *uc_fw)
 {
-	return uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS;
+	return uc_fw->status == INTEL_UC_FIRMWARE_LOADED;
 }
 
 static inline bool intel_uc_fw_supported(struct intel_uc_fw *uc_fw)
 {
 	/* shouldn't call this before checking hw/blob availability */
-	GEM_BUG_ON(uc_fw->fetch_status == INTEL_UC_FIRMWARE_UNINITIALIZED);
-	return uc_fw->fetch_status != INTEL_UC_FIRMWARE_NOT_SUPPORTED;
+	GEM_BUG_ON(uc_fw->status == INTEL_UC_FIRMWARE_UNINITIALIZED);
+	return uc_fw->status != INTEL_UC_FIRMWARE_NOT_SUPPORTED;
 }
 
 static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
 {
 	if (intel_uc_fw_is_loaded(uc_fw))
-		uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
+		uc_fw->status = INTEL_UC_FIRMWARE_FETCHED;
 }
 
 /**
@@ -179,7 +181,7 @@ static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
  */
 static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw)
 {
-	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
+	if (uc_fw->status < INTEL_UC_FIRMWARE_FETCHED)
 		return 0;
 
 	return uc_fw->header_size + uc_fw->ucode_size;
-- 
2.22.0

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

[PATCH 6/9] drm/i915/uc: Move xfer rsa logic to common function

[Daniele Ceraolo Spurio]

The way we copy the RSA is the same for GuC and HuC, so we can move the
logic in a common function. this will also make any update needed for
local memory easier.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c | 5 +----
 drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 9 +++------
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c  | 9 +++++++++
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h  | 1 +
 4 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
index 86df5147fc0a..8439a1fcfaae 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
@@ -106,13 +106,10 @@ static void guc_prepare_xfer(struct intel_guc *guc)
 static void guc_xfer_rsa(struct intel_guc *guc)
 {
 	struct intel_uncore *uncore = guc_to_gt(guc)->uncore;
-	struct intel_uc_fw *fw = &guc->fw;
-	struct sg_table *pages = fw->obj->mm.pages;
 	u32 rsa[UOS_RSA_SCRATCH_COUNT];
 	int i;
 
-	sg_pcopy_to_buffer(pages->sgl, pages->nents,
-			   rsa, sizeof(rsa), fw->rsa_offset);
+	intel_uc_fw_copy_rsa(&guc->fw, rsa, sizeof(rsa));
 
 	for (i = 0; i < UOS_RSA_SCRATCH_COUNT; i++)
 		intel_uncore_write(uncore, UOS_RSA_SCRATCH(i), rsa[i]);
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 263977294ef0..f200f6d49e60 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
@@ -64,17 +64,14 @@ void intel_huc_fw_init_early(struct intel_huc *huc)
 
 static void huc_xfer_rsa(struct intel_huc *huc)
 {
-	struct intel_uc_fw *fw = &huc->fw;
-	struct sg_table *pages = fw->obj->mm.pages;
-
 	/*
 	 * HuC firmware image is outside GuC accessible range.
 	 * Copy the RSA signature out of the image into
 	 * the perma-pinned region set aside for it
 	 */
-	sg_pcopy_to_buffer(pages->sgl, pages->nents,
-			   huc->rsa_data_vaddr, fw->rsa_size,
-			   fw->rsa_offset);
+	GEM_BUG_ON(huc->fw.rsa_size > huc->rsa_data->size);
+	intel_uc_fw_copy_rsa(&huc->fw, huc->rsa_data_vaddr,
+			     huc->rsa_data->size);
 }
 
 static int huc_xfer_ucode(struct intel_huc *huc)
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 91b1d9663481..3bf68285493b 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -365,6 +365,15 @@ void intel_uc_fw_cleanup_fetch(struct intel_uc_fw *uc_fw)
 	uc_fw->status = INTEL_UC_FIRMWARE_SELECTION_DONE;
 }
 
+void intel_uc_fw_copy_rsa(struct intel_uc_fw *uc_fw, void *dst, u32 max_len)
+{
+	struct sg_table *pages = uc_fw->obj->mm.pages;
+	u32 size = min_t(u32, uc_fw->rsa_size, max_len);
+
+	sg_pcopy_to_buffer(pages->sgl, pages->nents,
+			   dst, size, uc_fw->rsa_offset);
+}
+
 /**
  * intel_uc_fw_dump - dump information about uC firmware
  * @uc_fw: uC firmware
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 e0c5a38685e6..15c760995c11 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
@@ -199,6 +199,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
 int intel_uc_fw_init(struct intel_uc_fw *uc_fw);
 void intel_uc_fw_fini(struct intel_uc_fw *uc_fw);
 u32 intel_uc_fw_ggtt_offset(struct intel_uc_fw *uc_fw);
+void intel_uc_fw_copy_rsa(struct intel_uc_fw *uc_fw, void *dst, u32 max_len);
 void intel_uc_fw_dump(const struct intel_uc_fw *uc_fw, struct drm_printer *p);
 
 #endif
-- 
2.22.0

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

[PATCH 7/9] drm/i915/huc: Copy huc rsa only once

[Daniele Ceraolo Spurio]

The binary is perma-pinned and the rsa is not going to change, so copy
it only once and not on every load.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Fernando Pacheco <fernando.pacheco@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_huc.c    | 21 +++++++++++++++++----
 drivers/gpu/drm/i915/gt/uc/intel_huc.h    |  1 -
 drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 14 --------------
 3 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
index 1868f676d890..daf5996ec989 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
@@ -62,6 +62,7 @@ static int intel_huc_rsa_data_create(struct intel_huc *huc)
 	 * the authentication since its GGTT offset will be GuC
 	 * accessible.
 	 */
+	GEM_BUG_ON(huc->fw.rsa_size > PAGE_SIZE);
 	vma = intel_guc_allocate_vma(guc, PAGE_SIZE);
 	if (IS_ERR(vma))
 		return PTR_ERR(vma);
@@ -72,26 +73,38 @@ static int intel_huc_rsa_data_create(struct intel_huc *huc)
 		return PTR_ERR(vaddr);
 	}
 
+	intel_uc_fw_copy_rsa(&huc->fw, vaddr, vma->size);
+
+	i915_gem_object_unpin_map(vma->obj);
+
 	huc->rsa_data = vma;
-	huc->rsa_data_vaddr = vaddr;
 
 	return 0;
 }
 
 static void intel_huc_rsa_data_destroy(struct intel_huc *huc)
 {
-	i915_vma_unpin_and_release(&huc->rsa_data, I915_VMA_RELEASE_MAP);
+	i915_vma_unpin_and_release(&huc->rsa_data, 0);
 }
 
 int intel_huc_init(struct intel_huc *huc)
 {
 	int err;
 
-	err = intel_huc_rsa_data_create(huc);
+	err = intel_uc_fw_init(&huc->fw);
 	if (err)
 		return err;
 
-	return intel_uc_fw_init(&huc->fw);
+	/*
+	 * HuC firmware image is outside GuC accessible range.
+	 * Copy the RSA signature out of the image into
+	 * a perma-pinned region set aside for it
+	 */
+	err = intel_huc_rsa_data_create(huc);
+	if (err)
+		intel_uc_fw_fini(&huc->fw);
+
+	return err;
 }
 
 void intel_huc_fini(struct intel_huc *huc)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
index 9fa3d4629f2e..3f945115bb47 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
@@ -35,7 +35,6 @@ struct intel_huc {
 
 	/* HuC-specific additions */
 	struct i915_vma *rsa_data;
-	void *rsa_data_vaddr;
 
 	struct {
 		i915_reg_t reg;
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 f200f6d49e60..06aa29f5bf43 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
@@ -62,18 +62,6 @@ void intel_huc_fw_init_early(struct intel_huc *huc)
 			   huc_fw_blobs, ARRAY_SIZE(huc_fw_blobs));
 }
 
-static void huc_xfer_rsa(struct intel_huc *huc)
-{
-	/*
-	 * HuC firmware image is outside GuC accessible range.
-	 * Copy the RSA signature out of the image into
-	 * the perma-pinned region set aside for it
-	 */
-	GEM_BUG_ON(huc->fw.rsa_size > huc->rsa_data->size);
-	intel_uc_fw_copy_rsa(&huc->fw, huc->rsa_data_vaddr,
-			     huc->rsa_data->size);
-}
-
 static int huc_xfer_ucode(struct intel_huc *huc)
 {
 	struct intel_uc_fw *huc_fw = &huc->fw;
@@ -133,8 +121,6 @@ static int huc_fw_xfer(struct intel_uc_fw *huc_fw)
 {
 	struct intel_huc *huc = container_of(huc_fw, struct intel_huc, fw);
 
-	huc_xfer_rsa(huc);
-
 	return huc_xfer_ucode(huc);
 }
 
-- 
2.22.0

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

[PATCH 8/9] drm/i915/uc: Plumb the gt through fw_upload

[Daniele Ceraolo Spurio]

The gt is our new central structure for uc-related code, so we can use
that instead of jumping back to i915 via the fw object. Since we have it
in the upload function it is easy to pass it through the lower levels of
the xfer process instead of continuosly jumping via uc_fw->uc->gt, which
will also make things a bit cleaner for the next patch.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c | 35 +++++++++++------------
 drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 32 ++++++++-------------
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c  | 29 ++++++++++---------
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h  |  8 ++++--
 4 files changed, 48 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
index 8439a1fcfaae..c9bd55e23eec 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
@@ -73,10 +73,8 @@ void intel_guc_fw_init_early(struct intel_guc *guc)
 			   guc_fw_blobs, ARRAY_SIZE(guc_fw_blobs));
 }
 
-static void guc_prepare_xfer(struct intel_guc *guc)
+static void guc_prepare_xfer(struct intel_uncore *uncore)
 {
-	struct intel_gt *gt = guc_to_gt(guc);
-	struct intel_uncore *uncore = gt->uncore;
 	u32 shim_flags = GUC_DISABLE_SRAM_INIT_TO_ZEROES |
 			 GUC_ENABLE_READ_CACHE_LOGIC |
 			 GUC_ENABLE_MIA_CACHING |
@@ -87,12 +85,12 @@ static void guc_prepare_xfer(struct intel_guc *guc)
 	/* Must program this register before loading the ucode with DMA */
 	intel_uncore_write(uncore, GUC_SHIM_CONTROL, shim_flags);
 
-	if (IS_GEN9_LP(gt->i915))
+	if (IS_GEN9_LP(uncore->i915))
 		intel_uncore_write(uncore, GEN9LP_GT_PM_CONFIG, GT_DOORBELL_ENABLE);
 	else
 		intel_uncore_write(uncore, GEN9_GT_PM_CONFIG, GT_DOORBELL_ENABLE);
 
-	if (IS_GEN(gt->i915, 9)) {
+	if (IS_GEN(uncore->i915, 9)) {
 		/* DOP Clock Gating Enable for GuC clocks */
 		intel_uncore_rmw(uncore, GEN7_MISCCPCTL,
 				 0, GEN8_DOP_CLOCK_GATE_GUC_ENABLE);
@@ -103,13 +101,13 @@ static void guc_prepare_xfer(struct intel_guc *guc)
 }
 
 /* Copy RSA signature from the fw image to HW for verification */
-static void guc_xfer_rsa(struct intel_guc *guc)
+static void guc_xfer_rsa(struct intel_uc_fw *guc_fw,
+			 struct intel_uncore *uncore)
 {
-	struct intel_uncore *uncore = guc_to_gt(guc)->uncore;
 	u32 rsa[UOS_RSA_SCRATCH_COUNT];
 	int i;
 
-	intel_uc_fw_copy_rsa(&guc->fw, rsa, sizeof(rsa));
+	intel_uc_fw_copy_rsa(guc_fw, rsa, sizeof(rsa));
 
 	for (i = 0; i < UOS_RSA_SCRATCH_COUNT; i++)
 		intel_uncore_write(uncore, UOS_RSA_SCRATCH(i), rsa[i]);
@@ -184,10 +182,10 @@ static int guc_wait_ucode(struct intel_uncore *uncore)
  * transfer between GTT locations. This functionality is left out of the API
  * for now as there is no need for it.
  */
-static int guc_xfer_ucode(struct intel_guc *guc)
+static int guc_xfer_ucode(struct intel_uc_fw *guc_fw,
+			  struct intel_gt *gt)
 {
-	struct intel_uncore *uncore = guc_to_gt(guc)->uncore;
-	struct intel_uc_fw *guc_fw = &guc->fw;
+	struct intel_uncore *uncore = gt->uncore;
 	unsigned long offset;
 
 	/*
@@ -198,7 +196,7 @@ static int guc_xfer_ucode(struct intel_guc *guc)
 			   guc_fw->header_size + guc_fw->ucode_size);
 
 	/* Set the source address for the new blob */
-	offset = intel_uc_fw_ggtt_offset(guc_fw) + guc_fw->header_offset;
+	offset = intel_uc_fw_ggtt_offset(guc_fw, gt->ggtt) + guc_fw->header_offset;
 	intel_uncore_write(uncore, DMA_ADDR_0_LOW, lower_32_bits(offset));
 	intel_uncore_write(uncore, DMA_ADDR_0_HIGH, upper_32_bits(offset) & 0xFFFF);
 
@@ -218,26 +216,25 @@ static int guc_xfer_ucode(struct intel_guc *guc)
 /*
  * Load the GuC firmware blob into the MinuteIA.
  */
-static int guc_fw_xfer(struct intel_uc_fw *guc_fw)
+static int guc_fw_xfer(struct intel_uc_fw *guc_fw, struct intel_gt *gt)
 {
-	struct intel_guc *guc = container_of(guc_fw, struct intel_guc, fw);
-	struct intel_uncore *uncore = guc_to_gt(guc)->uncore;
+	struct intel_uncore *uncore = gt->uncore;
 	int ret;
 
 	GEM_BUG_ON(guc_fw->type != INTEL_UC_FW_TYPE_GUC);
 
 	intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
 
-	guc_prepare_xfer(guc);
+	guc_prepare_xfer(uncore);
 
 	/*
 	 * Note that GuC needs the CSS header plus uKernel code to be copied
 	 * by the DMA engine in one operation, whereas the RSA signature is
 	 * loaded via MMIO.
 	 */
-	guc_xfer_rsa(guc);
+	guc_xfer_rsa(guc_fw, uncore);
 
-	ret = guc_xfer_ucode(guc);
+	ret = guc_xfer_ucode(guc_fw, gt);
 
 	intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
 
@@ -258,5 +255,5 @@ static int guc_fw_xfer(struct intel_uc_fw *guc_fw)
  */
 int intel_guc_fw_upload(struct intel_guc *guc)
 {
-	return intel_uc_fw_upload(&guc->fw, guc_fw_xfer);
+	return intel_uc_fw_upload(&guc->fw, guc_to_gt(guc), guc_fw_xfer);
 }
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 06aa29f5bf43..41e032149f7e 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
@@ -62,10 +62,17 @@ void intel_huc_fw_init_early(struct intel_huc *huc)
 			   huc_fw_blobs, ARRAY_SIZE(huc_fw_blobs));
 }
 
-static int huc_xfer_ucode(struct intel_huc *huc)
+/**
+ * huc_fw_xfer() - DMA's the firmware
+ * @huc_fw: the firmware descriptor
+ *
+ * Transfer the firmware image to RAM for execution by the microcontroller.
+ *
+ * Return: 0 on success, non-zero on failure
+ */
+static int huc_fw_xfer(struct intel_uc_fw *huc_fw, struct intel_gt *gt)
 {
-	struct intel_uc_fw *huc_fw = &huc->fw;
-	struct intel_uncore *uncore = huc_to_gt(huc)->uncore;
+	struct intel_uncore *uncore = gt->uncore;
 	unsigned long offset = 0;
 	u32 size;
 	int ret;
@@ -75,7 +82,7 @@ static int huc_xfer_ucode(struct intel_huc *huc)
 	intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
 
 	/* Set the source address for the uCode */
-	offset = intel_uc_fw_ggtt_offset(huc_fw) +
+	offset = intel_uc_fw_ggtt_offset(huc_fw, gt->ggtt) +
 		 huc_fw->header_offset;
 	intel_uncore_write(uncore, DMA_ADDR_0_LOW,
 			   lower_32_bits(offset));
@@ -109,21 +116,6 @@ static int huc_xfer_ucode(struct intel_huc *huc)
 	return ret;
 }
 
-/**
- * huc_fw_xfer() - DMA's the firmware
- * @huc_fw: the firmware descriptor
- *
- * Transfer the firmware image to RAM for execution by the microcontroller.
- *
- * Return: 0 on success, non-zero on failure
- */
-static int huc_fw_xfer(struct intel_uc_fw *huc_fw)
-{
-	struct intel_huc *huc = container_of(huc_fw, struct intel_huc, fw);
-
-	return huc_xfer_ucode(huc);
-}
-
 /**
  * intel_huc_fw_upload() - load HuC uCode to device
  * @huc: intel_huc structure
@@ -138,5 +130,5 @@ static int huc_fw_xfer(struct intel_uc_fw *huc_fw)
  */
 int intel_huc_fw_upload(struct intel_huc *huc)
 {
-	return intel_uc_fw_upload(&huc->fw, huc_fw_xfer);
+	return intel_uc_fw_upload(&huc->fw, huc_to_gt(huc), huc_fw_xfer);
 }
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 3bf68285493b..bb6fb64c3936 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -227,12 +227,13 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 	release_firmware(fw);		/* OK even if fw is NULL */
 }
 
-static void intel_uc_fw_ggtt_bind(struct intel_uc_fw *uc_fw)
+static void intel_uc_fw_ggtt_bind(struct intel_uc_fw *uc_fw,
+				  struct intel_gt *gt)
 {
 	struct drm_i915_gem_object *obj = uc_fw->obj;
-	struct i915_ggtt *ggtt = &to_i915(obj->base.dev)->ggtt;
+	struct i915_ggtt *ggtt = gt->ggtt;
 	struct i915_vma dummy = {
-		.node.start = intel_uc_fw_ggtt_offset(uc_fw),
+		.node.start = intel_uc_fw_ggtt_offset(uc_fw, ggtt),
 		.node.size = obj->base.size,
 		.pages = obj->mm.pages,
 		.vm = &ggtt->vm,
@@ -247,11 +248,12 @@ static void intel_uc_fw_ggtt_bind(struct intel_uc_fw *uc_fw)
 	ggtt->vm.insert_entries(&ggtt->vm, &dummy, I915_CACHE_NONE, 0);
 }
 
-static void intel_uc_fw_ggtt_unbind(struct intel_uc_fw *uc_fw)
+static void intel_uc_fw_ggtt_unbind(struct intel_uc_fw *uc_fw,
+				    struct intel_gt *gt)
 {
 	struct drm_i915_gem_object *obj = uc_fw->obj;
-	struct i915_ggtt *ggtt = &to_i915(obj->base.dev)->ggtt;
-	u64 start = intel_uc_fw_ggtt_offset(uc_fw);
+	struct i915_ggtt *ggtt = gt->ggtt;
+	u64 start = intel_uc_fw_ggtt_offset(uc_fw, ggtt);
 
 	ggtt->vm.clear_range(&ggtt->vm, start, obj->base.size);
 }
@@ -259,14 +261,15 @@ static void intel_uc_fw_ggtt_unbind(struct intel_uc_fw *uc_fw)
 /**
  * intel_uc_fw_upload - load uC firmware using custom loader
  * @uc_fw: uC firmware
+ * @gt: the intel_gt structure
  * @xfer: custom uC firmware loader function
  *
  * Loads uC firmware using custom loader and updates internal flags.
  *
  * Return: 0 on success, non-zero on failure.
  */
-int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
-		       int (*xfer)(struct intel_uc_fw *uc_fw))
+int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, struct intel_gt *gt,
+		       int (*xfer)(struct intel_uc_fw *uc_fw, struct intel_gt *gt))
 {
 	int err;
 
@@ -280,9 +283,9 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
 		return -ENOEXEC;
 
 	/* Call custom loader */
-	intel_uc_fw_ggtt_bind(uc_fw);
-	err = xfer(uc_fw);
-	intel_uc_fw_ggtt_unbind(uc_fw);
+	intel_uc_fw_ggtt_bind(uc_fw, gt);
+	err = xfer(uc_fw, gt);
+	intel_uc_fw_ggtt_unbind(uc_fw, gt);
 	if (err)
 		goto fail;
 
@@ -334,10 +337,8 @@ void intel_uc_fw_fini(struct intel_uc_fw *uc_fw)
 	i915_gem_object_unpin_pages(uc_fw->obj);
 }
 
-u32 intel_uc_fw_ggtt_offset(struct intel_uc_fw *uc_fw)
+u32 intel_uc_fw_ggtt_offset(struct intel_uc_fw *uc_fw, struct i915_ggtt *ggtt)
 {
-	struct drm_i915_private *i915 = to_i915(uc_fw->obj->base.dev);
-	struct i915_ggtt *ggtt = &i915->ggtt;
 	struct drm_mm_node *node = &ggtt->uc_fw;
 
 	GEM_BUG_ON(!node->allocated);
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 15c760995c11..4043da69db60 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
@@ -31,6 +31,8 @@
 
 struct drm_printer;
 struct drm_i915_private;
+struct intel_gt;
+struct i915_ggtt;
 
 /* Home of GuC, HuC and DMC firmwares */
 #define INTEL_UC_FIRMWARE_URL "https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/i915"
@@ -194,11 +196,11 @@ void intel_uc_fw_select(struct drm_i915_private *i915,
 void intel_uc_fw_fetch(struct drm_i915_private *i915,
 		       struct intel_uc_fw *uc_fw);
 void intel_uc_fw_cleanup_fetch(struct intel_uc_fw *uc_fw);
-int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
-		       int (*xfer)(struct intel_uc_fw *uc_fw));
+int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, struct intel_gt *gt,
+		       int (*xfer)(struct intel_uc_fw *uc_fw, struct intel_gt *gt));
 int intel_uc_fw_init(struct intel_uc_fw *uc_fw);
 void intel_uc_fw_fini(struct intel_uc_fw *uc_fw);
-u32 intel_uc_fw_ggtt_offset(struct intel_uc_fw *uc_fw);
+u32 intel_uc_fw_ggtt_offset(struct intel_uc_fw *uc_fw, struct i915_ggtt *ggtt);
 void intel_uc_fw_copy_rsa(struct intel_uc_fw *uc_fw, void *dst, u32 max_len);
 void intel_uc_fw_dump(const struct intel_uc_fw *uc_fw, struct drm_printer *p);
 
-- 
2.22.0

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

[PATCH 9/9] drm/i915/uc: Unify uC firmware upload

[Daniele Ceraolo Spurio]

The way we load the firmwares is the same for both GuC and HuC, the only
difference is in the wopcm destination address and the dma flags, so we
easily can move the logic to a common function and pass in offset and
flags. The only other difference in the uplaod path are some the extra
steps that guc does before and after the xfer, but those don't require
the guc fw to be pinned in ggtt and can safely be performed before
calling the uc_upload function.

Note that this patch re-introduces the dma xfer wait for guc loading that
was removed with "drm/i915/guc: Propagate the fw xfer timeout". This is
not going to slow us down on a successful load (the dma has to complete
before fw init can start), but could slightly increase the timeout in case
of a fw init error.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c | 100 +++++-----------------
 drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c |  57 +-----------
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c  |  79 +++++++++++++----
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h  |   4 +-
 4 files changed, 86 insertions(+), 154 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
index c9bd55e23eec..a1e071958822 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
@@ -113,13 +113,6 @@ static void guc_xfer_rsa(struct intel_uc_fw *guc_fw,
 		intel_uncore_write(uncore, UOS_RSA_SCRATCH(i), rsa[i]);
 }
 
-static bool guc_xfer_completed(struct intel_uncore *uncore, u32 *status)
-{
-	/* Did we complete the xfer? */
-	*status = intel_uncore_read(uncore, DMA_CTRL);
-	return !(*status & START_DMA);
-}
-
 /*
  * Read the GuC status register (GUC_STATUS) and store it in the
  * specified location; then return a boolean indicating whether
@@ -166,65 +159,27 @@ static int guc_wait_ucode(struct intel_uncore *uncore)
 		ret = -ENXIO;
 	}
 
-	if (ret == 0 && !guc_xfer_completed(uncore, &status)) {
-		DRM_ERROR("GuC is ready, but the xfer %08x is incomplete\n",
-			  status);
-		ret = -ENXIO;
-	}
-
 	return ret;
 }
 
-/*
- * Transfer the firmware image to RAM for execution by the microcontroller.
+/**
+ * intel_guc_fw_upload() - load GuC uCode to device
+ * @guc: intel_guc structure
  *
- * Architecturally, the DMA engine is bidirectional, and can potentially even
- * transfer between GTT locations. This functionality is left out of the API
- * for now as there is no need for it.
- */
-static int guc_xfer_ucode(struct intel_uc_fw *guc_fw,
-			  struct intel_gt *gt)
-{
-	struct intel_uncore *uncore = gt->uncore;
-	unsigned long offset;
-
-	/*
-	 * The header plus uCode will be copied to WOPCM via DMA, excluding any
-	 * other components
-	 */
-	intel_uncore_write(uncore, DMA_COPY_SIZE,
-			   guc_fw->header_size + guc_fw->ucode_size);
-
-	/* Set the source address for the new blob */
-	offset = intel_uc_fw_ggtt_offset(guc_fw, gt->ggtt) + guc_fw->header_offset;
-	intel_uncore_write(uncore, DMA_ADDR_0_LOW, lower_32_bits(offset));
-	intel_uncore_write(uncore, DMA_ADDR_0_HIGH, upper_32_bits(offset) & 0xFFFF);
-
-	/*
-	 * Set the DMA destination. Current uCode expects the code to be
-	 * loaded at 8k; locations below this are used for the stack.
-	 */
-	intel_uncore_write(uncore, DMA_ADDR_1_LOW, 0x2000);
-	intel_uncore_write(uncore, DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
-
-	/* Finally start the DMA */
-	intel_uncore_write(uncore, DMA_CTRL,
-			   _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA));
-
-	return guc_wait_ucode(uncore);
-}
-/*
- * Load the GuC firmware blob into the MinuteIA.
+ * Called from intel_uc_init_hw() during driver load, resume from sleep and
+ * after a GPU reset.
+ *
+ * The firmware image should have already been fetched into memory, so only
+ * check that fetch succeeded, and then transfer the image to the h/w.
+ *
+ * Return:	non-zero code on error
  */
-static int guc_fw_xfer(struct intel_uc_fw *guc_fw, struct intel_gt *gt)
+int intel_guc_fw_upload(struct intel_guc *guc)
 {
+	struct intel_gt *gt = guc_to_gt(guc);
 	struct intel_uncore *uncore = gt->uncore;
 	int ret;
 
-	GEM_BUG_ON(guc_fw->type != INTEL_UC_FW_TYPE_GUC);
-
-	intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
-
 	guc_prepare_xfer(uncore);
 
 	/*
@@ -232,28 +187,15 @@ static int guc_fw_xfer(struct intel_uc_fw *guc_fw, struct intel_gt *gt)
 	 * by the DMA engine in one operation, whereas the RSA signature is
 	 * loaded via MMIO.
 	 */
-	guc_xfer_rsa(guc_fw, uncore);
+	guc_xfer_rsa(&guc->fw, uncore);
 
-	ret = guc_xfer_ucode(guc_fw, gt);
-
-	intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
-
-	return ret;
-}
+	/*
+	 * Current uCode expects the code to be loaded at 8k; locations below
+	 * this are used for the stack.
+	 */
+	ret = intel_uc_fw_upload(&guc->fw, gt, 0x2000, UOS_MOVE);
+	if (ret)
+		return ret;
 
-/**
- * intel_guc_fw_upload() - load GuC uCode to device
- * @guc: intel_guc structure
- *
- * Called from intel_uc_init_hw() during driver load, resume from sleep and
- * after a GPU reset.
- *
- * The firmware image should have already been fetched into memory, so only
- * check that fetch succeeded, and then transfer the image to the h/w.
- *
- * Return:	non-zero code on error
- */
-int intel_guc_fw_upload(struct intel_guc *guc)
-{
-	return intel_uc_fw_upload(&guc->fw, guc_to_gt(guc), guc_fw_xfer);
+	return guc_wait_ucode(uncore);
 }
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 41e032149f7e..44d2b7e7e845 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
@@ -62,60 +62,6 @@ void intel_huc_fw_init_early(struct intel_huc *huc)
 			   huc_fw_blobs, ARRAY_SIZE(huc_fw_blobs));
 }
 
-/**
- * huc_fw_xfer() - DMA's the firmware
- * @huc_fw: the firmware descriptor
- *
- * Transfer the firmware image to RAM for execution by the microcontroller.
- *
- * Return: 0 on success, non-zero on failure
- */
-static int huc_fw_xfer(struct intel_uc_fw *huc_fw, struct intel_gt *gt)
-{
-	struct intel_uncore *uncore = gt->uncore;
-	unsigned long offset = 0;
-	u32 size;
-	int ret;
-
-	GEM_BUG_ON(huc_fw->type != INTEL_UC_FW_TYPE_HUC);
-
-	intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
-
-	/* Set the source address for the uCode */
-	offset = intel_uc_fw_ggtt_offset(huc_fw, gt->ggtt) +
-		 huc_fw->header_offset;
-	intel_uncore_write(uncore, DMA_ADDR_0_LOW,
-			   lower_32_bits(offset));
-	intel_uncore_write(uncore, DMA_ADDR_0_HIGH,
-			   upper_32_bits(offset) & 0xFFFF);
-
-	/*
-	 * Hardware doesn't look at destination address for HuC. Set it to 0,
-	 * but still program the correct address space.
-	 */
-	intel_uncore_write(uncore, DMA_ADDR_1_LOW, 0);
-	intel_uncore_write(uncore, DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
-
-	size = huc_fw->header_size + huc_fw->ucode_size;
-	intel_uncore_write(uncore, DMA_COPY_SIZE, size);
-
-	/* Start the DMA */
-	intel_uncore_write(uncore, DMA_CTRL,
-			   _MASKED_BIT_ENABLE(HUC_UKERNEL | START_DMA));
-
-	/* Wait for DMA to finish */
-	ret = intel_wait_for_register_fw(uncore, DMA_CTRL, START_DMA, 0, 100);
-
-	DRM_DEBUG_DRIVER("HuC DMA transfer wait over with ret %d\n", ret);
-
-	/* Disable the bits once DMA is over */
-	intel_uncore_write(uncore, DMA_CTRL, _MASKED_BIT_DISABLE(HUC_UKERNEL));
-
-	intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
-
-	return ret;
-}
-
 /**
  * intel_huc_fw_upload() - load HuC uCode to device
  * @huc: intel_huc structure
@@ -130,5 +76,6 @@ static int huc_fw_xfer(struct intel_uc_fw *huc_fw, struct intel_gt *gt)
  */
 int intel_huc_fw_upload(struct intel_huc *huc)
 {
-	return intel_uc_fw_upload(&huc->fw, huc_to_gt(huc), huc_fw_xfer);
+	/* HW doesn't look at destination address for HuC, so set it to 0 */
+	return intel_uc_fw_upload(&huc->fw, huc_to_gt(huc), 0, HUC_UKERNEL);
 }
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 bb6fb64c3936..5a6e239498d2 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -227,13 +227,24 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 	release_firmware(fw);		/* OK even if fw is NULL */
 }
 
+static u32 uc_fw_ggtt_offset(struct intel_uc_fw *uc_fw, struct i915_ggtt *ggtt)
+{
+	struct drm_mm_node *node = &ggtt->uc_fw;
+
+	GEM_BUG_ON(!node->allocated);
+	GEM_BUG_ON(upper_32_bits(node->start));
+	GEM_BUG_ON(upper_32_bits(node->start + node->size - 1));
+
+	return lower_32_bits(node->start);
+}
+
 static void intel_uc_fw_ggtt_bind(struct intel_uc_fw *uc_fw,
 				  struct intel_gt *gt)
 {
 	struct drm_i915_gem_object *obj = uc_fw->obj;
 	struct i915_ggtt *ggtt = gt->ggtt;
 	struct i915_vma dummy = {
-		.node.start = intel_uc_fw_ggtt_offset(uc_fw, ggtt),
+		.node.start = uc_fw_ggtt_offset(uc_fw, ggtt),
 		.node.size = obj->base.size,
 		.pages = obj->mm.pages,
 		.vm = &ggtt->vm,
@@ -253,23 +264,68 @@ static void intel_uc_fw_ggtt_unbind(struct intel_uc_fw *uc_fw,
 {
 	struct drm_i915_gem_object *obj = uc_fw->obj;
 	struct i915_ggtt *ggtt = gt->ggtt;
-	u64 start = intel_uc_fw_ggtt_offset(uc_fw, ggtt);
+	u64 start = uc_fw_ggtt_offset(uc_fw, ggtt);
 
 	ggtt->vm.clear_range(&ggtt->vm, start, obj->base.size);
 }
 
+static int uc_fw_xfer(struct intel_uc_fw *uc_fw, struct intel_gt *gt,
+		      u32 wopcm_offset, u32 dma_flags)
+{
+	struct intel_uncore *uncore = gt->uncore;
+	u64 offset;
+	int ret;
+
+	intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
+
+	/* Set the source address for the uCode */
+	offset = uc_fw_ggtt_offset(uc_fw, gt->ggtt) + uc_fw->header_offset;
+	GEM_BUG_ON(upper_32_bits(offset) & 0xFFFF0000);
+	intel_uncore_write(uncore, DMA_ADDR_0_LOW, lower_32_bits(offset));
+	intel_uncore_write(uncore, DMA_ADDR_0_HIGH, upper_32_bits(offset));
+
+	/* Set the DMA destination */
+	intel_uncore_write(uncore, DMA_ADDR_1_LOW, wopcm_offset);
+	intel_uncore_write(uncore, DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
+
+	/*
+	 * Set the trasfer size. The header plus uCode will be copied to WOPCM
+	 * via DMA, excluding any other components
+	 */
+	intel_uncore_write(uncore, DMA_COPY_SIZE,
+			   uc_fw->header_size + uc_fw->ucode_size);
+
+	/* Start the DMA */
+	intel_uncore_write(uncore, DMA_CTRL,
+			   _MASKED_BIT_ENABLE(dma_flags | START_DMA));
+
+	/* Wait for DMA to finish */
+	ret = intel_wait_for_register_fw(uncore, DMA_CTRL, START_DMA, 0, 100);
+	if (ret)
+		DRM_ERROR("DMA for %s fw failed, err=%d\n",
+			  intel_uc_fw_type_repr(uc_fw->type), ret);
+
+	/* Disable the bits once DMA is over */
+	intel_uncore_write(uncore, DMA_CTRL, _MASKED_BIT_DISABLE(dma_flags));
+
+	intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
+
+	return ret;
+}
+
 /**
  * intel_uc_fw_upload - load uC firmware using custom loader
  * @uc_fw: uC firmware
  * @gt: the intel_gt structure
- * @xfer: custom uC firmware loader function
+ * @wopcm_offset: destination offset in wopcm
+ * @dma_flags: flags for flags for dma ctrl
  *
- * Loads uC firmware using custom loader and updates internal flags.
+ * Loads uC firmware and updates internal flags.
  *
  * Return: 0 on success, non-zero on failure.
  */
 int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, struct intel_gt *gt,
-		       int (*xfer)(struct intel_uc_fw *uc_fw, struct intel_gt *gt))
+		       u32 wopcm_offset, u32 dma_flags)
 {
 	int err;
 
@@ -284,7 +340,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, struct intel_gt *gt,
 
 	/* Call custom loader */
 	intel_uc_fw_ggtt_bind(uc_fw, gt);
-	err = xfer(uc_fw, gt);
+	err = uc_fw_xfer(uc_fw, gt, wopcm_offset, dma_flags);
 	intel_uc_fw_ggtt_unbind(uc_fw, gt);
 	if (err)
 		goto fail;
@@ -337,17 +393,6 @@ void intel_uc_fw_fini(struct intel_uc_fw *uc_fw)
 	i915_gem_object_unpin_pages(uc_fw->obj);
 }
 
-u32 intel_uc_fw_ggtt_offset(struct intel_uc_fw *uc_fw, struct i915_ggtt *ggtt)
-{
-	struct drm_mm_node *node = &ggtt->uc_fw;
-
-	GEM_BUG_ON(!node->allocated);
-	GEM_BUG_ON(upper_32_bits(node->start));
-	GEM_BUG_ON(upper_32_bits(node->start + node->size - 1));
-
-	return lower_32_bits(node->start);
-}
-
 /**
  * intel_uc_fw_cleanup_fetch - cleanup uC firmware
  *
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 4043da69db60..d5db8d12a2c2 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
@@ -32,7 +32,6 @@
 struct drm_printer;
 struct drm_i915_private;
 struct intel_gt;
-struct i915_ggtt;
 
 /* Home of GuC, HuC and DMC firmwares */
 #define INTEL_UC_FIRMWARE_URL "https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/i915"
@@ -197,10 +196,9 @@ void intel_uc_fw_fetch(struct drm_i915_private *i915,
 		       struct intel_uc_fw *uc_fw);
 void intel_uc_fw_cleanup_fetch(struct intel_uc_fw *uc_fw);
 int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, struct intel_gt *gt,
-		       int (*xfer)(struct intel_uc_fw *uc_fw, struct intel_gt *gt));
+		       u32 wopcm_offset, u32 dma_flags);
 int intel_uc_fw_init(struct intel_uc_fw *uc_fw);
 void intel_uc_fw_fini(struct intel_uc_fw *uc_fw);
-u32 intel_uc_fw_ggtt_offset(struct intel_uc_fw *uc_fw, struct i915_ggtt *ggtt);
 void intel_uc_fw_copy_rsa(struct intel_uc_fw *uc_fw, void *dst, u32 max_len);
 void intel_uc_fw_dump(const struct intel_uc_fw *uc_fw, struct drm_printer *p);
 
-- 
2.22.0

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

✗ Fi.CI.CHECKPATCH: warning for uC fw path unification + misc clean-up

[Patchwork]

== Series Details ==

Series: uC fw path unification + misc clean-up
URL   : https://patchwork.freedesktop.org/series/64039/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
1defc4182153 drm/i915/uc: Gt-fy uc reset
ad3b67774d80 drm/i915/uc: Unify uC platform check
5917a7b6440c drm/i915/uc: Unify uC FW selection
-:114: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'major_' - possible side-effects?
#114: FILE: drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c:41:
+#define GUC_FW_BLOB(prefix_, major_, minor_, patch_) \
+UC_FW_BLOB(prefix_##_guc, major_, minor_, \
+	   __MAKE_GUC_FW_PATH(prefix_, major_, minor_, patch_))

-:114: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'minor_' - possible side-effects?
#114: FILE: drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c:41:
+#define GUC_FW_BLOB(prefix_, major_, minor_, patch_) \
+UC_FW_BLOB(prefix_##_guc, major_, minor_, \
+	   __MAKE_GUC_FW_PATH(prefix_, major_, minor_, patch_))

-:238: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'major_' - possible side-effects?
#238: FILE: drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c:30:
+#define HUC_FW_BLOB(prefix_, major_, minor_, bld_num_) \
+UC_FW_BLOB(prefix_##_huc, major_, minor_, \
+	   HUC_FW_PATH(prefix_, major_, minor_, bld_num_))

-:238: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'minor_' - possible side-effects?
#238: FILE: drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c:30:
+#define HUC_FW_BLOB(prefix_, major_, minor_, bld_num_) \
+UC_FW_BLOB(prefix_##_huc, major_, minor_, \
+	   HUC_FW_PATH(prefix_, major_, minor_, bld_num_))

-:350: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'path_' - possible side-effects?
#350: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h:58:
+#define UC_FW_BLOB(prefix_, major_, minor_, path_) \
+static const struct intel_uc_fw_blob prefix_##_fw_blob = { \
+	.major = (major_), \
+	.minor = (minor_), \
+	.path = path_ \
+}; \
+MODULE_FIRMWARE(path_)

total: 0 errors, 0 warnings, 5 checks, 338 lines checked
473061d8d2ff drm/i915/uc: Sanitize uC when GT is sanitized
8f847de27248 drm/i915/uc: Unify uc_fw status tracking
e2ad9c1b0a09 drm/i915/uc: Move xfer rsa logic to common function
23717b64a014 drm/i915/huc: Copy huc rsa only once
f539a9c8d4f1 drm/i915/uc: Plumb the gt through fw_upload
1a6f761311a2 drm/i915/uc: Unify uC firmware upload
-:291: WARNING:TYPO_SPELLING: 'trasfer' may be misspelled - perhaps 'transfer'?
#291: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c:292:
+	 * Set the trasfer size. The header plus uCode will be copied to WOPCM

total: 0 errors, 1 warnings, 0 checks, 334 lines checked

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

✗ Fi.CI.SPARSE: warning for uC fw path unification + misc clean-up

[Patchwork]

== Series Details ==

Series: uC fw path unification + misc clean-up
URL   : https://patchwork.freedesktop.org/series/64039/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915/uc: Gt-fy uc reset
Okay!

Commit: drm/i915/uc: Unify uC platform check
Okay!

Commit: drm/i915/uc: Unify uC FW selection
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)

Commit: drm/i915/uc: Sanitize uC when GT is sanitized
Okay!

Commit: drm/i915/uc: Unify uc_fw status tracking
Okay!

Commit: drm/i915/uc: Move xfer rsa logic to common function
+drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c:371:20: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c:371:20: warning: expression using sizeof(void)

Commit: drm/i915/huc: Copy huc rsa only once
Okay!

Commit: drm/i915/uc: Plumb the gt through fw_upload
Okay!

Commit: drm/i915/uc: Unify uC firmware upload
Okay!

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

✓ Fi.CI.BAT: success for uC fw path unification + misc clean-up

[Patchwork]

== Series Details ==

Series: uC fw path unification + misc clean-up
URL   : https://patchwork.freedesktop.org/series/64039/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6535 -> Patchwork_13717
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/

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

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

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@read_all_entries:
    - fi-icl-u3:          [PASS][1] -> [DMESG-WARN][2] ([fdo#107724])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/fi-icl-u3/igt@debugfs_test@read_all_entries.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/fi-icl-u3/igt@debugfs_test@read_all_entries.html

  * igt@kms_busy@basic-flip-a:
    - fi-kbl-7567u:       [PASS][3] -> [SKIP][4] ([fdo#109271] / [fdo#109278]) +2 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/fi-kbl-7567u/igt@kms_busy@basic-flip-a.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/fi-kbl-7567u/igt@kms_busy@basic-flip-a.html

  * igt@kms_busy@basic-flip-c:
    - fi-kbl-7500u:       [PASS][5] -> [SKIP][6] ([fdo#109271] / [fdo#109278]) +2 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/fi-kbl-7500u/igt@kms_busy@basic-flip-c.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/fi-kbl-7500u/igt@kms_busy@basic-flip-c.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [PASS][7] -> [FAIL][8] ([fdo#109485])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u2:          [PASS][9] -> [FAIL][10] ([fdo#103167])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html

  
#### Possible fixes ####

  * igt@i915_module_load@reload-no-display:
    - fi-icl-u3:          [DMESG-WARN][11] ([fdo#107724]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/fi-icl-u3/igt@i915_module_load@reload-no-display.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/fi-icl-u3/igt@i915_module_load@reload-no-display.html

  
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485


Participating hosts (53 -> 47)
------------------------------

  Additional (2): fi-hsw-peppy fi-icl-dsi 
  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-icl-y fi-byt-clapper fi-bdw-samus 


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

  * Linux: CI_DRM_6535 -> Patchwork_13717

  CI_DRM_6535: ec297a234e92aa258bba523320898f408d0cf147 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5107: 1a5b48671e0863cb723e3d0239e54c828360dc99 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13717: 1a6f761311a20967a9120b5cefafc3c33516b304 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

1a6f761311a2 drm/i915/uc: Unify uC firmware upload
f539a9c8d4f1 drm/i915/uc: Plumb the gt through fw_upload
23717b64a014 drm/i915/huc: Copy huc rsa only once
e2ad9c1b0a09 drm/i915/uc: Move xfer rsa logic to common function
8f847de27248 drm/i915/uc: Unify uc_fw status tracking
473061d8d2ff drm/i915/uc: Sanitize uC when GT is sanitized
5917a7b6440c drm/i915/uc: Unify uC FW selection
ad3b67774d80 drm/i915/uc: Unify uC platform check
1defc4182153 drm/i915/uc: Gt-fy uc reset

== Logs ==

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

✓ Fi.CI.IGT: success for uC fw path unification + misc clean-up

[Patchwork]

== Series Details ==

Series: uC fw path unification + misc clean-up
URL   : https://patchwork.freedesktop.org/series/64039/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6535_full -> Patchwork_13717_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live_hangcheck:
    - shard-iclb:         [PASS][1] -> [INCOMPLETE][2] ([fdo#107713] / [fdo#108569])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-iclb4/igt@i915_selftest@live_hangcheck.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-iclb4/igt@i915_selftest@live_hangcheck.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-apl:          [PASS][3] -> [DMESG-WARN][4] ([fdo#108566]) +2 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-apl1/igt@kms_flip@flip-vs-suspend-interruptible.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-apl7/igt@kms_flip@flip-vs-suspend-interruptible.html
    - shard-kbl:          [PASS][5] -> [DMESG-WARN][6] ([fdo#108566])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-kbl6/igt@kms_flip@flip-vs-suspend-interruptible.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-kbl3/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@basic:
    - shard-iclb:         [PASS][7] -> [FAIL][8] ([fdo#103167]) +5 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-iclb6/igt@kms_frontbuffer_tracking@basic.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-iclb6/igt@kms_frontbuffer_tracking@basic.html

  * igt@kms_psr@psr2_basic:
    - shard-iclb:         [PASS][9] -> [SKIP][10] ([fdo#109441]) +1 similar issue
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-iclb2/igt@kms_psr@psr2_basic.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-iclb7/igt@kms_psr@psr2_basic.html

  * igt@kms_setmode@basic:
    - shard-apl:          [PASS][11] -> [FAIL][12] ([fdo#99912])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-apl7/igt@kms_setmode@basic.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-apl3/igt@kms_setmode@basic.html
    - shard-kbl:          [PASS][13] -> [FAIL][14] ([fdo#99912])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-kbl3/igt@kms_setmode@basic.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-kbl4/igt@kms_setmode@basic.html

  * igt@prime_busy@hang-default:
    - shard-hsw:          [PASS][15] -> [INCOMPLETE][16] ([fdo#103540])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-hsw5/igt@prime_busy@hang-default.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-hsw8/igt@prime_busy@hang-default.html

  * igt@tools_test@tools_test:
    - shard-glk:          [PASS][17] -> [SKIP][18] ([fdo#109271])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-glk5/igt@tools_test@tools_test.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-glk9/igt@tools_test@tools_test.html

  
#### Possible fixes ####

  * igt@gem_ctx_shared@exec-shared-gtt-default:
    - shard-apl:          [FAIL][19] ([fdo#110890]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-apl5/igt@gem_ctx_shared@exec-shared-gtt-default.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-apl1/igt@gem_ctx_shared@exec-shared-gtt-default.html

  * igt@gem_eio@reset-stress:
    - shard-skl:          [FAIL][21] ([fdo#109661]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-skl5/igt@gem_eio@reset-stress.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-skl8/igt@gem_eio@reset-stress.html

  * igt@gem_exec_balancer@smoke:
    - shard-iclb:         [SKIP][23] ([fdo#110854]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-iclb8/igt@gem_exec_balancer@smoke.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-iclb2/igt@gem_exec_balancer@smoke.html

  * igt@gem_exec_suspend@basic-s3:
    - shard-kbl:          [DMESG-WARN][25] ([fdo#108566]) -> [PASS][26] +1 similar issue
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-kbl3/igt@gem_exec_suspend@basic-s3.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-kbl4/igt@gem_exec_suspend@basic-s3.html

  * igt@kms_cursor_legacy@cursor-vs-flip-varying-size:
    - shard-apl:          [INCOMPLETE][27] ([fdo#103927]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-apl1/igt@kms_cursor_legacy@cursor-vs-flip-varying-size.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-apl5/igt@kms_cursor_legacy@cursor-vs-flip-varying-size.html

  * igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-render:
    - shard-iclb:         [FAIL][29] ([fdo#103167]) -> [PASS][30] +4 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-iclb6/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-render.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-iclb6/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-render.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-rte:
    - shard-iclb:         [FAIL][31] ([fdo#103167] / [fdo#110378]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-iclb2/igt@kms_frontbuffer_tracking@fbcpsr-1p-rte.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-iclb7/igt@kms_frontbuffer_tracking@fbcpsr-1p-rte.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
    - shard-apl:          [DMESG-WARN][33] ([fdo#108566]) -> [PASS][34] +1 similar issue
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-apl1/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-apl8/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-skl:          [FAIL][35] ([fdo#108145]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-skl7/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-skl1/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [FAIL][37] ([fdo#108145] / [fdo#110403]) -> [PASS][38] +1 similar issue
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-skl4/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-skl3/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_psr@psr2_suspend:
    - shard-iclb:         [SKIP][39] ([fdo#109441]) -> [PASS][40] +2 similar issues
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/shard-iclb8/igt@kms_psr@psr2_suspend.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13717/shard-iclb2/igt@kms_psr@psr2_suspend.html

  
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109661]: https://bugs.freedesktop.org/show_bug.cgi?id=109661
  [fdo#110378]: https://bugs.freedesktop.org/show_bug.cgi?id=110378
  [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
  [fdo#110854]: https://bugs.freedesktop.org/show_bug.cgi?id=110854
  [fdo#110890]: https://bugs.freedesktop.org/show_bug.cgi?id=110890
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (9 -> 9)
------------------------------

  No changes in participating hosts


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

  * Linux: CI_DRM_6535 -> Patchwork_13717

  CI_DRM_6535: ec297a234e92aa258bba523320898f408d0cf147 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5107: 1a5b48671e0863cb723e3d0239e54c828360dc99 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13717: 1a6f761311a20967a9120b5cefafc3c33516b304 @ 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_13717/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/9] drm/i915/uc: Gt-fy uc reset

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2019-07-23 00:20:40)
> This was the last place in gt/uc that was still using I915_READ
> with the global dev_priv.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/9] drm/i915/uc: Sanitize uC when GT is sanitized

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2019-07-23 00:20:43)
> The microcontrollers are part of GT so it makes logical sense to have
> them sanitized at the same time. This also fixed an issue with our
> status tracking where the FW load status is not reset around
> hibernation.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>

I like the sentiment,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

I just want this is CI'ed by itself to be sure as S4-devices always
catches me off guard :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/9] uC fw path unification + misc clean-up

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2019-07-23 00:20:39)
> The main aim of this patch series is unify the guc_fw and huc_fw
> handling paths and improve the related state tracking. Ultimately, I'd
> like to reach the point where we can kill the intel_guc_fw and
> intel_huc_fw files and just keep the few differences in other files. Not
> yet going that far in this series though.
> 
> Bundled in is some more gt-fication and minor clean-up.

I was honestly hoping for more spam reduction. 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6535/fi-kbl-guc/igt@i915_selftest@live_hangcheck.html
is bordering on the ridiculous signal:noise.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/9] drm/i915/uc: Unify uC platform check

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2019-07-23 00:20:41)
> We have several HAS_* checks for GuC and HuC but we mostly use HAS_GUC
> and HAS_HUC, with only 1 exception. Since our HW always has either
> both uC or neither of them, just replace all the checks with a unified
> HAS_UC.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>

Given that we only had a single .has_[hg]uc entry, calling that .has_uc
makes sense. I trust in your crystal ball that not being able to
distinguish between the availability of different uC is not important.

Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/9] drm/i915/uc: Unify uC FW selection

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2019-07-23 00:20:42)
> +/* must be ordered base on platform + revid, from newer to older */
> +static const struct intel_uc_platform_requirement guc_fw_blobs[] = {
> +       { INTEL_ICELAKE,        0,      &icl_guc_fw_blob },
> +       { INTEL_COFFEELAKE,     0,      &kbl_guc_fw_blob },
> +       { INTEL_GEMINILAKE,     0,      &glk_guc_fw_blob },
> +       { INTEL_KABYLAKE,       0,      &kbl_guc_fw_blob },
> +       { INTEL_BROXTON,        0,      &bxt_guc_fw_blob },
> +       { INTEL_SKYLAKE,        0,      &skl_guc_fw_blob },
> +};

> +/**
> + * intel_uc_fw_select - select the uC firmware to fetch & load
> + * @i915: device private
> + * @uc_fw: uC firmware
> + * @list: list of required firmware for each platform
> + * @length: number of entries in the list
> + *
> + * Select the uC firmware from the provided list based on platform and revid of
> + * the device we're on. If the firmware_path modparam override is set, it takes
> + * precedence over the entries in the list.
> + */
> +void intel_uc_fw_select(struct drm_i915_private *i915,
> +                       struct intel_uc_fw *uc_fw,
> +                       const struct intel_uc_platform_requirement *list,
> +                       unsigned int length)

If this is a list, do we need a length? Is a sentinel not good enough?

> +{
> +       GEM_BUG_ON(uc_fw->fetch_status != INTEL_UC_FIRMWARE_UNINITIALIZED);
> +
> +       if (!HAS_UC(i915)) {
> +               uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
> +               return;
> +       }

if (IS_ENABLED(CONFIG_DRM_I915_SELFTEST)) {
	for (i = 1; i < length; i++)
		if (list[i].first_rev <= list[i-1].first_rev) {
			pr_err("...");
			uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
			return;
		}
}

> +
> +       uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
> +
> +       if (unlikely(i915_modparams.guc_firmware_path &&
> +                    uc_fw->type == INTEL_UC_FW_TYPE_GUC)) {
> +               uc_fw->path = i915_modparams.guc_firmware_path;
> +       } else if (unlikely(i915_modparams.huc_firmware_path &&
> +                           uc_fw->type == INTEL_UC_FW_TYPE_HUC)) {
> +               uc_fw->path = i915_modparams.huc_firmware_path;
> +       } else {
> +               enum intel_platform p = INTEL_INFO(i915)->platform;
> +               u8 rev = INTEL_REVID(i915);
> +               int i;
> +
> +               for (i = 0; i < length && p <= list[i].p; i++) {
> +                       if (p == list[i].p && rev >= list[i].first_rev) {
> +                               uc_fw->path = list[i].blob->path;
> +                               uc_fw->major_ver_wanted = list[i].blob->major;
> +                               uc_fw->minor_ver_wanted = list[i].blob->minor;
> +                               break;
> +                       }
> +               }
> +       }
> +}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/9] drm/i915/uc: Unify uc_fw status tracking

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2019-07-23 00:20:44)
> @@ -219,19 +207,17 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>  
>         uc_fw->obj = obj;
>         uc_fw->size = fw->size;
> -       uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
> -       DRM_DEBUG_DRIVER("%s fw fetch %s\n",
> -                        intel_uc_fw_type_repr(uc_fw->type),
> -                        intel_uc_fw_status_repr(uc_fw->fetch_status));
> +       uc_fw->status = INTEL_UC_FIRMWARE_FETCHED;
> +       DRM_DEBUG_DRIVER("%s fw fetch done\n",
> +                        intel_uc_fw_type_repr(uc_fw->type));

I don't see much value in the success message, they just get followed by
more success messages. Pointless spam imo.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/9] drm/i915/uc: Move xfer rsa logic to common function

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2019-07-23 00:20:45)
> The way we copy the RSA is the same for GuC and HuC, so we can move the
> logic in a common function. this will also make any update needed for
> local memory easier.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c | 5 +----
>  drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 9 +++------
>  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c  | 9 +++++++++
>  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h  | 1 +
>  4 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> index 86df5147fc0a..8439a1fcfaae 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> @@ -106,13 +106,10 @@ static void guc_prepare_xfer(struct intel_guc *guc)
>  static void guc_xfer_rsa(struct intel_guc *guc)
>  {
>         struct intel_uncore *uncore = guc_to_gt(guc)->uncore;
> -       struct intel_uc_fw *fw = &guc->fw;
> -       struct sg_table *pages = fw->obj->mm.pages;
>         u32 rsa[UOS_RSA_SCRATCH_COUNT];
>         int i;
>  
> -       sg_pcopy_to_buffer(pages->sgl, pages->nents,
> -                          rsa, sizeof(rsa), fw->rsa_offset);
> +       intel_uc_fw_copy_rsa(&guc->fw, rsa, sizeof(rsa));

Return num_bytes copied?
GEM_BUG_ON(i < sizeof(rsa)) ?

>  
>         for (i = 0; i < UOS_RSA_SCRATCH_COUNT; i++)
>                 intel_uncore_write(uncore, UOS_RSA_SCRATCH(i), rsa[i]);
> 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 263977294ef0..f200f6d49e60 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> @@ -64,17 +64,14 @@ void intel_huc_fw_init_early(struct intel_huc *huc)
>  
>  static void huc_xfer_rsa(struct intel_huc *huc)
>  {
> -       struct intel_uc_fw *fw = &huc->fw;
> -       struct sg_table *pages = fw->obj->mm.pages;
> -
>         /*
>          * HuC firmware image is outside GuC accessible range.
>          * Copy the RSA signature out of the image into
>          * the perma-pinned region set aside for it
>          */
> -       sg_pcopy_to_buffer(pages->sgl, pages->nents,
> -                          huc->rsa_data_vaddr, fw->rsa_size,
> -                          fw->rsa_offset);
> +       GEM_BUG_ON(huc->fw.rsa_size > huc->rsa_data->size);
> +       intel_uc_fw_copy_rsa(&huc->fw, huc->rsa_data_vaddr,
> +                            huc->rsa_data->size);
>  }
>  
>  static int huc_xfer_ucode(struct intel_huc *huc)
> 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 91b1d9663481..3bf68285493b 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -365,6 +365,15 @@ void intel_uc_fw_cleanup_fetch(struct intel_uc_fw *uc_fw)
>         uc_fw->status = INTEL_UC_FIRMWARE_SELECTION_DONE;
>  }
>  
> +void intel_uc_fw_copy_rsa(struct intel_uc_fw *uc_fw, void *dst, u32 max_len)
> +{
> +       struct sg_table *pages = uc_fw->obj->mm.pages;
> +       u32 size = min_t(u32, uc_fw->rsa_size, max_len);
> +
> +       sg_pcopy_to_buffer(pages->sgl, pages->nents,
> +                          dst, size, uc_fw->rsa_offset);

return sg_pcopy_to_buffer(pages->sgl, pages->nents,
			  dst, size, uc_fw->rsa_offset);

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/9] drm/i915/huc: Copy huc rsa only once

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2019-07-23 00:20:46)
> The binary is perma-pinned and the rsa is not going to change, so copy
> it only once and not on every load.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Fernando Pacheco <fernando.pacheco@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_huc.c    | 21 +++++++++++++++++----
>  drivers/gpu/drm/i915/gt/uc/intel_huc.h    |  1 -
>  drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 14 --------------
>  3 files changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> index 1868f676d890..daf5996ec989 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> @@ -62,6 +62,7 @@ static int intel_huc_rsa_data_create(struct intel_huc *huc)
>          * the authentication since its GGTT offset will be GuC
>          * accessible.
>          */
> +       GEM_BUG_ON(huc->fw.rsa_size > PAGE_SIZE);
>         vma = intel_guc_allocate_vma(guc, PAGE_SIZE);
>         if (IS_ERR(vma))
>                 return PTR_ERR(vma);
> @@ -72,26 +73,38 @@ static int intel_huc_rsa_data_create(struct intel_huc *huc)
>                 return PTR_ERR(vaddr);
>         }
>  
> +       intel_uc_fw_copy_rsa(&huc->fw, vaddr, vma->size);

copied = ^^^
GEM_BUG_ON(copied < huc->fw_.rsa_size);

> +
> +       i915_gem_object_unpin_map(vma->obj);
> +
>         huc->rsa_data = vma;
> -       huc->rsa_data_vaddr = vaddr;
>  
>         return 0;
>  }
>  
>  static void intel_huc_rsa_data_destroy(struct intel_huc *huc)
>  {
> -       i915_vma_unpin_and_release(&huc->rsa_data, I915_VMA_RELEASE_MAP);
> +       i915_vma_unpin_and_release(&huc->rsa_data, 0);
>  }
>  
>  int intel_huc_init(struct intel_huc *huc)
>  {
>         int err;
>  
> -       err = intel_huc_rsa_data_create(huc);
> +       err = intel_uc_fw_init(&huc->fw);
>         if (err)
>                 return err;
>  
> -       return intel_uc_fw_init(&huc->fw);
> +       /*
> +        * HuC firmware image is outside GuC accessible range.
> +        * Copy the RSA signature out of the image into
> +        * a perma-pinned region set aside for it
> +        */
> +       err = intel_huc_rsa_data_create(huc);
> +       if (err)
> +               intel_uc_fw_fini(&huc->fw);
> +
> +       return err;

Joonas would like to say onion early.

>  }
>  
>  void intel_huc_fini(struct intel_huc *huc)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
> index 9fa3d4629f2e..3f945115bb47 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
> @@ -35,7 +35,6 @@ struct intel_huc {
>  
>         /* HuC-specific additions */
>         struct i915_vma *rsa_data;
> -       void *rsa_data_vaddr;
>  
>         struct {
>                 i915_reg_t reg;
> 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 f200f6d49e60..06aa29f5bf43 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> @@ -62,18 +62,6 @@ void intel_huc_fw_init_early(struct intel_huc *huc)
>                            huc_fw_blobs, ARRAY_SIZE(huc_fw_blobs));
>  }
>  
> -static void huc_xfer_rsa(struct intel_huc *huc)
> -{
> -       /*
> -        * HuC firmware image is outside GuC accessible range.
> -        * Copy the RSA signature out of the image into
> -        * the perma-pinned region set aside for it
> -        */
> -       GEM_BUG_ON(huc->fw.rsa_size > huc->rsa_data->size);
> -       intel_uc_fw_copy_rsa(&huc->fw, huc->rsa_data_vaddr,
> -                            huc->rsa_data->size);
> -}
> -
>  static int huc_xfer_ucode(struct intel_huc *huc)
>  {
>         struct intel_uc_fw *huc_fw = &huc->fw;
> @@ -133,8 +121,6 @@ static int huc_fw_xfer(struct intel_uc_fw *huc_fw)
>  {
>         struct intel_huc *huc = container_of(huc_fw, struct intel_huc, fw);
>  
> -       huc_xfer_rsa(huc);
> -
>         return huc_xfer_ucode(huc);
>  }

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/9] drm/i915/uc: Plumb the gt through fw_upload

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2019-07-23 00:20:47)
> The gt is our new central structure for uc-related code, so we can use
> that instead of jumping back to i915 via the fw object. Since we have it
> in the upload function it is easy to pass it through the lower levels of
> the xfer process instead of continuosly jumping via uc_fw->uc->gt, which
> will also make things a bit cleaner for the next patch.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> @@ -103,13 +101,13 @@ static void guc_prepare_xfer(struct intel_guc *guc)
>  }
>  
>  /* Copy RSA signature from the fw image to HW for verification */
> -static void guc_xfer_rsa(struct intel_guc *guc)
> +static void guc_xfer_rsa(struct intel_uc_fw *guc_fw,

intel_uc_fw *guc_fw is rubbing me the wrong way. I would have just used
fw since that is unambiguous here.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 9/9] drm/i915/uc: Unify uC firmware upload

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2019-07-23 00:20:48)
> The way we load the firmwares is the same for both GuC and HuC, the only
> difference is in the wopcm destination address and the dma flags, so we
> easily can move the logic to a common function and pass in offset and
> flags. The only other difference in the uplaod path are some the extra
> steps that guc does before and after the xfer, but those don't require
> the guc fw to be pinned in ggtt and can safely be performed before
> calling the uc_upload function.
> 
> Note that this patch re-introduces the dma xfer wait for guc loading that
> was removed with "drm/i915/guc: Propagate the fw xfer timeout". This is
> not going to slow us down on a successful load (the dma has to complete
> before fw init can start), but could slightly increase the timeout in case
> of a fw init error.

Should not be a problem if all is well. And if not, picking up on an
error earlier is beneficial.

> +static int uc_fw_xfer(struct intel_uc_fw *uc_fw, struct intel_gt *gt,
> +                     u32 wopcm_offset, u32 dma_flags)
> +{
> +       struct intel_uncore *uncore = gt->uncore;
> +       u64 offset;
> +       int ret;
> +
> +       intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
> +
> +       /* Set the source address for the uCode */
> +       offset = uc_fw_ggtt_offset(uc_fw, gt->ggtt) + uc_fw->header_offset;
> +       GEM_BUG_ON(upper_32_bits(offset) & 0xFFFF0000);
> +       intel_uncore_write(uncore, DMA_ADDR_0_LOW, lower_32_bits(offset));

You've taken an explicit fw, so these can all be _fw.

> +       intel_uncore_write(uncore, DMA_ADDR_0_HIGH, upper_32_bits(offset));
> +
> +       /* Set the DMA destination */
> +       intel_uncore_write(uncore, DMA_ADDR_1_LOW, wopcm_offset);
> +       intel_uncore_write(uncore, DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
> +
> +       /*
> +        * Set the trasfer size. The header plus uCode will be copied to WOPCM
> +        * via DMA, excluding any other components
> +        */
> +       intel_uncore_write(uncore, DMA_COPY_SIZE,
> +                          uc_fw->header_size + uc_fw->ucode_size);
> +
> +       /* Start the DMA */
> +       intel_uncore_write(uncore, DMA_CTRL,
> +                          _MASKED_BIT_ENABLE(dma_flags | START_DMA));
> +
> +       /* Wait for DMA to finish */
> +       ret = intel_wait_for_register_fw(uncore, DMA_CTRL, START_DMA, 0, 100);
> +       if (ret)
> +               DRM_ERROR("DMA for %s fw failed, err=%d\n",
> +                         intel_uc_fw_type_repr(uc_fw->type), ret);
> +
> +       /* Disable the bits once DMA is over */
> +       intel_uncore_write(uncore, DMA_CTRL, _MASKED_BIT_DISABLE(dma_flags));
> +
> +       intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
> +
> +       return ret;
> +}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/9] drm/i915/uc: Unify uC platform check

[Michal Wajdeczko]

On Tue, 23 Jul 2019 01:20:41 +0200, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@intel.com> wrote:

> We have several HAS_* checks for GuC and HuC but we mostly use HAS_GUC
> and HAS_HUC, with only 1 exception. Since our HW always has either
> both uC or neither of them, just replace all the checks with a unified
> HAS_UC.

our hardware has also other micro-controller (DMC aka CSR) so maybe to
avoid ambiguity we shall call it HAS_GT_UC ?

on other hand our documentation mostly uses term GUC (see GEN6_GDRST)
so maybe we should not to try to diverse from that and keep using HAS_GUC?

if you insist then we can drop HAS_HUC but imho this macro was useful to
keep code clear (GuC related code was using HAS_GUC, HuC related HAS_HUC)

#define HAS_GT_UC(i915)	(INTEL_INFO(i915)->has_gt_uc)
// GuC and HuC are GT uC
#define HAS_GUC(i915)	HAS_GT_UC(i915)
#define HAS_HUC(i915)	HAS_GT_UC(i915)

and we can avoid referring to hw details where not really needed
(yes, I know, every developer should already know such hw details)

>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_reset.c     |  2 +-
>  drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c |  2 +-
>  drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c |  2 +-
>  drivers/gpu/drm/i915/gt/uc/intel_uc.c     |  2 +-
>  drivers/gpu/drm/i915/gt/uc/selftest_guc.c |  4 ++--
>  drivers/gpu/drm/i915/i915_debugfs.c       |  6 +++---
>  drivers/gpu/drm/i915/i915_drv.h           | 15 ++-------------
>  drivers/gpu/drm/i915/i915_gpu_error.c     |  4 ++--
>  drivers/gpu/drm/i915/i915_irq.c           |  2 +-
>  drivers/gpu/drm/i915/i915_pci.c           |  4 ++--
>  drivers/gpu/drm/i915/intel_device_info.h  |  2 +-
>  drivers/gpu/drm/i915/intel_pm.c           |  4 ++--
>  drivers/gpu/drm/i915/intel_wopcm.c        |  4 ++--
>  13 files changed, 21 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c  
> b/drivers/gpu/drm/i915/gt/intel_reset.c
> index 55e2ddcbd215..7fd135a5a41f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -595,7 +595,7 @@ int intel_reset_guc(struct intel_gt *gt)
>  		INTEL_GEN(gt->i915) >= 11 ? GEN11_GRDOM_GUC : GEN9_GRDOM_GUC;
>  	int ret;
> -	GEM_BUG_ON(!HAS_GUC(gt->i915));
> +	GEM_BUG_ON(!HAS_UC(gt->i915));

there is "guc" in function name
there is "GUC" in bitfield name

but you are checking UC ;)

> 	intel_uncore_forcewake_get(gt->uncore, FORCEWAKE_ALL);
>  	ret = gen6_hw_domain_reset(gt, guc_domain);
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> index 3dfa40fdbe99..ddd3b2162528 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> @@ -80,7 +80,7 @@ static void guc_fw_select(struct intel_uc_fw *guc_fw)
> 	GEM_BUG_ON(guc_fw->type != INTEL_UC_FW_TYPE_GUC);
> -	if (!HAS_GUC(i915)) {
> +	if (!HAS_UC(i915)) {
>  		guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
>  		return;
>  	}
> 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 543854c42d9d..15891cf4bb2c 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> @@ -74,7 +74,7 @@ static void huc_fw_select(struct intel_uc_fw *huc_fw)
> 	GEM_BUG_ON(huc_fw->type != INTEL_UC_FW_TYPE_HUC);
> -	if (!HAS_HUC(dev_priv)) {
> +	if (!HAS_UC(dev_priv)) {
>  		huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
>  		return;
>  	}
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index 4480a3dc2449..0d50b73f6cfe 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -61,7 +61,7 @@ static int __get_platform_enable_guc(struct intel_uc  
> *uc)
>  	struct intel_uc_fw *huc_fw = &uc->huc.fw;
>  	int enable_guc = 0;
> -	if (!HAS_GUC(uc_to_gt(uc)->i915))
> +	if (!HAS_UC(uc_to_gt(uc)->i915))
>  		return 0;
> 	/* We don't want to enable GuC/HuC on pre-Gen11 by default */
> diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c  
> b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
> index 93f7c930ab18..d1aa180050c4 100644
> --- a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
> @@ -134,7 +134,7 @@ static int igt_guc_clients(void *args)
>  	struct intel_guc *guc;
>  	int err = 0;
> -	GEM_BUG_ON(!HAS_GUC(dev_priv));
> +	GEM_BUG_ON(!HAS_UC(dev_priv));
>  	mutex_lock(&dev_priv->drm.struct_mutex);
>  	wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
> @@ -226,7 +226,7 @@ static int igt_guc_doorbells(void *arg)
>  	int i, err = 0;
>  	u16 db_id;
> -	GEM_BUG_ON(!HAS_GUC(dev_priv));
> +	GEM_BUG_ON(!HAS_UC(dev_priv));
>  	mutex_lock(&dev_priv->drm.struct_mutex);
>  	wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c  
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 6b84d04a6a28..4249107f9d0d 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1865,7 +1865,7 @@ static int i915_huc_load_status_info(struct  
> seq_file *m, void *data)
>  	intel_wakeref_t wakeref;
>  	struct drm_printer p;
> -	if (!HAS_HUC(dev_priv))
> +	if (!HAS_UC(dev_priv))
>  		return -ENODEV;
> 	p = drm_seq_file_printer(m);
> @@ -1883,7 +1883,7 @@ static int i915_guc_load_status_info(struct  
> seq_file *m, void *data)
>  	intel_wakeref_t wakeref;
>  	struct drm_printer p;
> -	if (!HAS_GUC(dev_priv))
> +	if (!HAS_UC(dev_priv))
>  		return -ENODEV;
> 	p = drm_seq_file_printer(m);
> @@ -2062,7 +2062,7 @@ static int i915_guc_log_dump(struct seq_file *m,  
> void *data)
>  	u32 *log;
>  	int i = 0;
> -	if (!HAS_GUC(dev_priv))
> +	if (!HAS_UC(dev_priv))
>  		return -ENODEV;
> 	if (dump_load_err)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h  
> b/drivers/gpu/drm/i915/i915_drv.h
> index 0e44cc4b2ca1..06ddc5df12fa 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2271,20 +2271,9 @@ IS_SUBPLATFORM(const struct drm_i915_private  
> *i915,
> #define HAS_IPC(dev_priv)		 (INTEL_INFO(dev_priv)->display.has_ipc)
> -/*
> - * For now, anything with a GuC requires uCode loading, and then  
> supports
> - * command submission once loaded. But these are logically independent
> - * properties, so we have separate macros to test them.
> - */
> -#define HAS_GUC(dev_priv)	(INTEL_INFO(dev_priv)->has_guc)
> -#define HAS_GUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
> -#define HAS_GUC_SCHED(dev_priv)	(HAS_GUC(dev_priv))

yep, _UCODE and _SCHED should be nuked

> -
> -/* For now, anything with a GuC has also HuC */
> -#define HAS_HUC(dev_priv)	(HAS_GUC(dev_priv))
> -#define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
> +#define HAS_UC(dev_priv)	(INTEL_INFO(dev_priv)->has_uc)

time to use i915 instead of dev_priv

> -/* Having a GuC is not the same as using a GuC */
> +/* Having a uC is not the same as using a uC */
>  #define USES_GUC(dev_priv)		intel_uc_is_using_guc(&(dev_priv)->gt.uc)
>  #define  
> USES_GUC_SUBMISSION(dev_priv)	intel_uc_is_using_guc_submission(&(dev_priv)->gt.uc)
>  #define USES_HUC(dev_priv)		intel_uc_is_using_huc(&(dev_priv)->gt.uc)
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c  
> b/drivers/gpu/drm/i915/i915_gpu_error.c
> index c5b89bf4d616..1db4d7302734 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -620,7 +620,7 @@ static void err_print_uc(struct  
> drm_i915_error_state_buf *m,
>  	const struct i915_gpu_state *error =
>  		container_of(error_uc, typeof(*error), uc);
> -	if (!error->device_info.has_guc)
> +	if (!error->device_info.has_uc)
>  		return;
> 	intel_uc_fw_dump(&error_uc->guc_fw, &p);
> @@ -1557,7 +1557,7 @@ static void capture_uc_state(struct i915_gpu_state  
> *error)
>  	struct intel_uc *uc = &i915->gt.uc;
> 	/* Capturing uC state won't be useful if there is no GuC */
> -	if (!error->device_info.has_guc)
> +	if (!error->device_info.has_uc)
>  		return;
> 	error_uc->guc_fw = uc->guc.fw;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c  
> b/drivers/gpu/drm/i915/i915_irq.c
> index 11c73af92597..0b2351a6c490 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4766,7 +4766,7 @@ void intel_irq_init(struct drm_i915_private  
> *dev_priv)
>  		dev_priv->l3_parity.remap_info[i] = NULL;
> 	/* pre-gen11 the guc irqs bits are in the upper 16 bits of the pm reg */
> -	if (HAS_GUC_SCHED(dev_priv) && INTEL_GEN(dev_priv) < 11)
> +	if (HAS_UC(dev_priv) && INTEL_GEN(dev_priv) < 11)
>  		dev_priv->gt.pm_guc_events = GUC_INTR_GUC2HOST << 16;
> 	/* Let's track the enabled rps events */
> diff --git a/drivers/gpu/drm/i915/i915_pci.c  
> b/drivers/gpu/drm/i915/i915_pci.c
> index 40076ba431d4..b5ece92683fd 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -595,7 +595,7 @@ static const struct intel_device_info  
> intel_cherryview_info = {
>  	GEN9_DEFAULT_PAGE_SIZES, \
>  	.has_logical_ring_preemption = 1, \
>  	.display.has_csr = 1, \
> -	.has_guc = 1, \
> +	.has_uc = 1, \
>  	.display.has_ipc = 1, \
>  	.ddb_size = 896
> @@ -647,7 +647,7 @@ static const struct intel_device_info  
> intel_skylake_gt4_info = {
>  	.display.has_dp_mst = 1, \
>  	.has_logical_ring_contexts = 1, \
>  	.has_logical_ring_preemption = 1, \
> -	.has_guc = 1, \
> +	.has_uc = 1, \
>  	.ppgtt_type = INTEL_PPGTT_FULL, \
>  	.ppgtt_size = 48, \
>  	.has_reset_engine = 1, \
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h  
> b/drivers/gpu/drm/i915/intel_device_info.h
> index 45a9badc9b8e..99e5b22e26f6 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -112,7 +112,6 @@ enum intel_ppgtt_type {
>  	func(gpu_reset_clobbers_display); \
>  	func(has_reset_engine); \
>  	func(has_fpga_dbg); \
> -	func(has_guc); \
>  	func(has_l3_dpf); \
>  	func(has_llc); \
>  	func(has_logical_ring_contexts); \
> @@ -124,6 +123,7 @@ enum intel_ppgtt_type {
>  	func(has_rps); \
>  	func(has_runtime_pm); \
>  	func(has_snoop); \
> +	func(has_uc); \
>  	func(has_coherent_ggtt); \
>  	func(unfenced_needs_alignment); \
>  	func(hws_needs_physical);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c  
> b/drivers/gpu/drm/i915/intel_pm.c
> index 22472f2bd31b..53ef862731f6 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7162,7 +7162,7 @@ static void gen11_enable_rc6(struct  
> drm_i915_private *dev_priv)
>  	for_each_engine(engine, dev_priv, id)
>  		I915_WRITE(RING_MAX_IDLE(engine->mmio_base), 10);
> -	if (HAS_GUC(dev_priv))
> +	if (HAS_UC(dev_priv))
>  		I915_WRITE(GUC_MAX_IDLE_COUNT, 0xA);
> 	I915_WRITE(GEN6_RC_SLEEP, 0);
> @@ -7243,7 +7243,7 @@ static void gen9_enable_rc6(struct  
> drm_i915_private *dev_priv)
>  	for_each_engine(engine, dev_priv, id)
>  		I915_WRITE(RING_MAX_IDLE(engine->mmio_base), 10);
> -	if (HAS_GUC(dev_priv))
> +	if (HAS_UC(dev_priv))
>  		I915_WRITE(GUC_MAX_IDLE_COUNT, 0xA);
> 	I915_WRITE(GEN6_RC_SLEEP, 0);
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c  
> b/drivers/gpu/drm/i915/intel_wopcm.c
> index fafd4e6a1147..c966d572b0c4 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
> @@ -74,7 +74,7 @@ void intel_wopcm_init_early(struct intel_wopcm *wopcm)
>  {
>  	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
> -	if (!HAS_GUC(i915))
> +	if (!HAS_UC(i915))
>  		return;
> 	if (INTEL_GEN(i915) >= 11)
> @@ -263,7 +263,7 @@ int intel_wopcm_init_hw(struct intel_wopcm *wopcm,  
> struct intel_gt *gt)
>  	if (!USES_GUC(i915))
>  		return 0;
> -	GEM_BUG_ON(!HAS_GUC(i915));
> +	GEM_BUG_ON(!HAS_UC(i915));

hmm, maybe

	#define HAS_WOPCM(i915)	HAS_GT_UC(i915)

and then

	GEM_BUG_ON(!HAS_WOPCM(i915));

>  	GEM_BUG_ON(!wopcm->guc.size);
>  	GEM_BUG_ON(!wopcm->guc.base);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/9] drm/i915/uc: Unify uC FW selection

[Michal Wajdeczko]

On Tue, 23 Jul 2019 01:20:42 +0200, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@intel.com> wrote:

> +
> +#define GUC_FW_BLOB(prefix_, major_, minor_, patch_) \
> +UC_FW_BLOB(prefix_##_guc, major_, minor_, \
> +	   __MAKE_GUC_FW_PATH(prefix_, major_, minor_, patch_))
> +
> +GUC_FW_BLOB(skl, 33, 0, 0);
> +GUC_FW_BLOB(bxt, 33, 0, 0);
> +GUC_FW_BLOB(kbl, 33, 0, 0);
> +GUC_FW_BLOB(glk, 33, 0, 0);
> +GUC_FW_BLOB(icl, 33, 0, 0);
> +
> +/* must be ordered base on platform + revid, from newer to older */
> +static const struct intel_uc_platform_requirement guc_fw_blobs[] = {
> +	{ INTEL_ICELAKE,	0,	&icl_guc_fw_blob },
> +	{ INTEL_COFFEELAKE,	0,	&kbl_guc_fw_blob },
> +	{ INTEL_GEMINILAKE,	0,	&glk_guc_fw_blob },
> +	{ INTEL_KABYLAKE,	0,	&kbl_guc_fw_blob },
> +	{ INTEL_BROXTON,	0,	&bxt_guc_fw_blob },
> +	{ INTEL_SKYLAKE,	0,	&skl_guc_fw_blob },
> +};

Can we avoid pointers to separate blob definitions ?
What about defining each fw in single line like below

#define INTEL_GUC_FIRMWARE_DEFS(fw_def) \
	fw_def(ICELAKE, 0, 33, 0, 0, icl, GUC) \
	fw_def(COFFEELAKE, 0, 33, 0, 0, kbl, GUC) \
	fw_def(GEMINILAKE, 0, 33, 0, 0, glk, GUC) \
	fw_def(KABYLAKE, 0, 33, 0, 0, kbl, GUC) \
	fw_def(BROXTON, 0, 33, 0, 0, bxt, GUC) \
	fw_def(SKYLAKE, 0, 33, 0, 0, skl, GUC) \
	/* end */

with some extra common helpers

#define TO_MODULE_FIRMWARE(_platform, _rev, _major, _minor, _patch,  
_prefix, _builder) \
	MODULE_FIRMWARE(_builder##_FW_PATH_BUILDER(_prefix, _major, _minor,  
_patch));

#define TO_BLOB_ENTRY(_platform, _rev, _major, _minor, _patch, _prefix,  
_builder) \
{ \
	.platform = INTEL_##_platform, \
	.rev = (_rev), \
	.major = (_major), \
	.minor = (_minor), \
	.patch = (_patch), \
	.blob = _builder##_FW_PATH_BUILDER(_prefix, _major, _minor, _patch), \
},

then we can have immutable

static const struct intel_uc_blob guc_fw_blobs[] = {
INTEL_GUC_FIRMWARE_DEFS(TO_BLOB_ENTRY)
};
INTEL_GUC_FIRMWARE_DEFS(TO_MODULE_FIRMWARE)

(tested locally for feasibility)

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

Re: [PATCH 5/9] drm/i915/uc: Unify uc_fw status tracking

[Michal Wajdeczko]

On Tue, 23 Jul 2019 01:20:44 +0200, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@intel.com> wrote:

> We currently track fetch and load status separately, but the 2 are
> actually sequential in the uc lifetime (fetch must complete before we
> can attempt the load!). Unifying the 2 variables we can better follow
> the sequential states and improve our trackng of the uC state.
>
> Also, sprinkle some GEM_BUG_ON to make sure we transition correctly
> between states.
>
> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_huc.c   |  4 +-
>  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 74 ++++++++++--------------
>  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 50 ++++++++--------
>  3 files changed, 57 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> index bc14439173d7..1868f676d890 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> @@ -117,7 +117,7 @@ int intel_huc_auth(struct intel_huc *huc)
>  	struct intel_guc *guc = &gt->uc.guc;
>  	int ret;
> -	if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> +	if (huc->fw.status != INTEL_UC_FIRMWARE_LOADED)

iirc we have dedicated helper intel_uc_fw_is_loaded() for this

>  		return -ENOEXEC;
> 	ret = intel_guc_auth_huc(guc,
> @@ -141,7 +141,7 @@ int intel_huc_auth(struct intel_huc *huc)
>  	return 0;
> fail:
> -	huc->fw.load_status = INTEL_UC_FIRMWARE_FAIL;
> +	huc->fw.status = INTEL_UC_FIRMWARE_LOAD_FAIL;

hmm, so after we load bad HuC fw but before we authenticate it
we will report it as loaded (aka successful)?

maybe in addition to 'loaded' status there should add extra
'authenticated/running' status ? note that we can also load
the guc but then it can still not boot due to bad signature

> 	DRM_ERROR("HuC: Authentication failed %d\n", ret);
>  	return ret;

/.../

> @@ -95,23 +95,11 @@ void intel_uc_fw_fetch(struct drm_i915_private  
> *dev_priv,
>  	int err;
> 	GEM_BUG_ON(!intel_uc_fw_supported(uc_fw));
> -
> -	if (!uc_fw->path) {
> -		dev_info(dev_priv->drm.dev,
> -			 "%s: No firmware was defined for %s!\n",
> -			 intel_uc_fw_type_repr(uc_fw->type),
> -			 intel_platform_name(INTEL_INFO(dev_priv)->platform));
> -		return;
> -	}
> +	GEM_BUG_ON(!intel_uc_fw_is_selected(uc_fw));
> 	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
>  			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);

I guess we can drop this debug log as request_firmware() will print
fw path on failure and actual loaded fw is printed elsewhere later

> -	uc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
> -	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
> -			 intel_uc_fw_type_repr(uc_fw->type),
> -			 intel_uc_fw_status_repr(uc_fw->fetch_status));
> -
>  	err = request_firmware(&fw, uc_fw->path, &pdev->dev);
>  	if (err) {
>  		DRM_DEBUG_DRIVER("%s fw request_firmware err=%d\n",
> @@ -219,19 +207,17 @@ void intel_uc_fw_fetch(struct drm_i915_private  
> *dev_priv,
> 	uc_fw->obj = obj;
>  	uc_fw->size = fw->size;
> -	uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
> -	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
> -			 intel_uc_fw_type_repr(uc_fw->type),
> -			 intel_uc_fw_status_repr(uc_fw->fetch_status));
> +	uc_fw->status = INTEL_UC_FIRMWARE_FETCHED;
> +	DRM_DEBUG_DRIVER("%s fw fetch done\n",
> +			 intel_uc_fw_type_repr(uc_fw->type));
> 	release_firmware(fw);
>  	return;
> fail:
> -	uc_fw->fetch_status = INTEL_UC_FIRMWARE_FAIL;
> -	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
> -			 intel_uc_fw_type_repr(uc_fw->type),
> -			 intel_uc_fw_status_repr(uc_fw->fetch_status));
> +	uc_fw->status = INTEL_UC_FIRMWARE_FETCH_FAIL;
> +	DRM_DEBUG_DRIVER("%s fw fetch failed\n",
> +			 intel_uc_fw_type_repr(uc_fw->type));
> 	DRM_WARN("%s: Failed to fetch firmware %s (error %d)\n",

I'm wondering if we want to keep our above messages as request_firmware()
will report something like this

[   12.198037] i915 0000:00:02.0: Direct firmware load for i915/XXX failed  
with error -2


>  		 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
> @@ -287,13 +273,11 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
>  	DRM_DEBUG_DRIVER("%s fw load %s\n",
>  			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
> -	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> -		return -ENOEXEC;
> +	/* make sure the status was cleared the last time we reset the uc */
> +	GEM_BUG_ON(uc_fw->status == INTEL_UC_FIRMWARE_LOADED);
> -	uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
> -	DRM_DEBUG_DRIVER("%s fw load %s\n",
> -			 intel_uc_fw_type_repr(uc_fw->type),
> -			 intel_uc_fw_status_repr(uc_fw->load_status));
> +	if (uc_fw->status < INTEL_UC_FIRMWARE_FETCHED)
> +		return -ENOEXEC;
> 	/* Call custom loader */
>  	intel_uc_fw_ggtt_bind(uc_fw);
> @@ -302,10 +286,9 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
>  	if (err)
>  		goto fail;
> -	uc_fw->load_status = INTEL_UC_FIRMWARE_SUCCESS;
> -	DRM_DEBUG_DRIVER("%s fw load %s\n",
> -			 intel_uc_fw_type_repr(uc_fw->type),
> -			 intel_uc_fw_status_repr(uc_fw->load_status));
> +	uc_fw->status = INTEL_UC_FIRMWARE_LOADED;
> +	DRM_DEBUG_DRIVER("%s fw load completed\n",
> +			 intel_uc_fw_type_repr(uc_fw->type));
> 	DRM_INFO("%s: Loaded firmware %s (version %u.%u)\n",
>  		 intel_uc_fw_type_repr(uc_fw->type),
> @@ -315,10 +298,9 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
>  	return 0;
> fail:
> -	uc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;
> -	DRM_DEBUG_DRIVER("%s fw load %s\n",
> -			 intel_uc_fw_type_repr(uc_fw->type),
> -			 intel_uc_fw_status_repr(uc_fw->load_status));
> +	uc_fw->status = INTEL_UC_FIRMWARE_LOAD_FAIL;
> +	DRM_DEBUG_DRIVER("%s fw load failed\n",
> +			 intel_uc_fw_type_repr(uc_fw->type));

maybe for debug purposes we can add helper like

static inline void intel_uc_fw_set_status(uc_fw, status)
{
#ifdef CONFIG_DRM_I915_DEBUG_GUC
	DRM_DEBUG_DRIVER("%s: %s -> %s\n",
		intel_uc_fw_type_repr(uc_fw->type),
		intel_uc_fw_type_repr(uc_fw->status)
		intel_uc_fw_type_repr(status));
#endif
	uc_fw->status = status;
}


> 	DRM_WARN("%s: Failed to load firmware %s (error %d)\n",
>  		 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
> @@ -330,7 +312,10 @@ int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
>  {
>  	int err;
> -	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> +	/* this should happen before the load! */
> +	GEM_BUG_ON(uc_fw->status == INTEL_UC_FIRMWARE_LOADED);
> +
> +	if (uc_fw->status < INTEL_UC_FIRMWARE_FETCHED)
>  		return -ENOEXEC;
> 	err = i915_gem_object_pin_pages(uc_fw->obj);
> @@ -343,7 +328,7 @@ int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
> void intel_uc_fw_fini(struct intel_uc_fw *uc_fw)
>  {
> -	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> +	if (uc_fw->status < INTEL_UC_FIRMWARE_FETCHED)
>  		return;
> 	i915_gem_object_unpin_pages(uc_fw->obj);
> @@ -377,7 +362,7 @@ void intel_uc_fw_cleanup_fetch(struct intel_uc_fw  
> *uc_fw)
>  	if (obj)
>  		i915_gem_object_put(obj);
> -	uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
> +	uc_fw->status = INTEL_UC_FIRMWARE_SELECTION_DONE;
>  }
> /**
> @@ -391,9 +376,8 @@ void intel_uc_fw_dump(const struct intel_uc_fw  
> *uc_fw, struct drm_printer *p)
>  {
>  	drm_printf(p, "%s firmware: %s\n",
>  		   intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
> -	drm_printf(p, "\tstatus: fetch %s, load %s\n",
> -		   intel_uc_fw_status_repr(uc_fw->fetch_status),
> -		   intel_uc_fw_status_repr(uc_fw->load_status));
> +	drm_printf(p, "\tstatus: %s\n",
> +		   intel_uc_fw_status_repr(uc_fw->status));
>  	drm_printf(p, "\tversion: wanted %u.%u, found %u.%u\n",
>  		   uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted,
>  		   uc_fw->major_ver_found, uc_fw->minor_ver_found);
> 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 550b626afb15..e0c5a38685e6 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> @@ -36,12 +36,13 @@ struct drm_i915_private;
>  #define INTEL_UC_FIRMWARE_URL  
> "https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/i915"
> enum intel_uc_fw_status {
> -	INTEL_UC_FIRMWARE_NOT_SUPPORTED = -2, /* no uc HW */
> -	INTEL_UC_FIRMWARE_FAIL = -1,
> +	INTEL_UC_FIRMWARE_LOAD_FAIL = -3,
> +	INTEL_UC_FIRMWARE_FETCH_FAIL = -2,
> +	INTEL_UC_FIRMWARE_NOT_SUPPORTED = -1, /* no uc HW */
>  	INTEL_UC_FIRMWARE_UNINITIALIZED = 0, /* used to catch checks done too  
> early */
> -	INTEL_UC_FIRMWARE_NOT_STARTED = 1,
> -	INTEL_UC_FIRMWARE_PENDING,
> -	INTEL_UC_FIRMWARE_SUCCESS
> +	INTEL_UC_FIRMWARE_SELECTION_DONE, /* selection include the "no FW"  
> case */
> +	INTEL_UC_FIRMWARE_FETCHED,
> +	INTEL_UC_FIRMWARE_LOADED

I was working on something similar but my names were:

	UNINITIALIZED
	|	\--> N/A (no hw or disabled)
	DEFINED (aka selection done)
	|	\-->  MISSING (aka fetch failed)
	AVAILABLE (aka fetched)
	||	\--> FAILED
	UPLOADED
	RUNNING (authenticated)

I was also trying to add extra flag like .auto_selected to decide
if continue with graceful fallback if something went wrong

/.../

> @@ -179,7 +181,7 @@ static inline void intel_uc_fw_sanitize(struct  
> intel_uc_fw *uc_fw)
>   */
>  static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw)
>  {
> -	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> +	if (uc_fw->status < INTEL_UC_FIRMWARE_FETCHED)
>  		return 0;
> 	return uc_fw->header_size + uc_fw->ucode_size;

btw, maybe we should add GEM_BUG_ON here too ?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/9] drm/i915/uc: Unify uC platform check

[Daniele Ceraolo Spurio]

On 7/23/2019 4:19 AM, Michal Wajdeczko wrote:
> On Tue, 23 Jul 2019 01:20:41 +0200, Daniele Ceraolo Spurio 
> <daniele.ceraolospurio@intel.com> wrote:
>
>> We have several HAS_* checks for GuC and HuC but we mostly use HAS_GUC
>> and HAS_HUC, with only 1 exception. Since our HW always has either
>> both uC or neither of them, just replace all the checks with a unified
>> HAS_UC.
>
> our hardware has also other micro-controller (DMC aka CSR) so maybe to
> avoid ambiguity we shall call it HAS_GT_UC ?

ack

>
> on other hand our documentation mostly uses term GUC (see GEN6_GDRST)
> so maybe we should not to try to diverse from that and keep using 
> HAS_GUC?
>
> if you insist then we can drop HAS_HUC but imho this macro was useful to
> keep code clear (GuC related code was using HAS_GUC, HuC related HAS_HUC)
>
> #define HAS_GT_UC(i915)    (INTEL_INFO(i915)->has_gt_uc)
> // GuC and HuC are GT uC
> #define HAS_GUC(i915)    HAS_GT_UC(i915)
> #define HAS_HUC(i915)    HAS_GT_UC(i915)
>
> and we can avoid referring to hw details where not really needed
> (yes, I know, every developer should already know such hw details)
>

My aim here is to unify the 2 paths around the load and just use HAS_UC 
(or HAS_GT_UC) because in the common FW functions we don't know if we're 
touching GuC or HuC. We already logically organize GuC and HuC under 
intel_uc so it should be quite evident from the code that HAS_UC 
includes them both. Mixing HAS_GT_UC in the common paths and HAS_GUC/HUC 
in the dedicated ones seems more confusing IMO.

>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> ---
>>  drivers/gpu/drm/i915/gt/intel_reset.c     |  2 +-
>>  drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c |  2 +-
>>  drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c |  2 +-
>>  drivers/gpu/drm/i915/gt/uc/intel_uc.c     |  2 +-
>>  drivers/gpu/drm/i915/gt/uc/selftest_guc.c |  4 ++--
>>  drivers/gpu/drm/i915/i915_debugfs.c       |  6 +++---
>>  drivers/gpu/drm/i915/i915_drv.h           | 15 ++-------------
>>  drivers/gpu/drm/i915/i915_gpu_error.c     |  4 ++--
>>  drivers/gpu/drm/i915/i915_irq.c           |  2 +-
>>  drivers/gpu/drm/i915/i915_pci.c           |  4 ++--
>>  drivers/gpu/drm/i915/intel_device_info.h  |  2 +-
>>  drivers/gpu/drm/i915/intel_pm.c           |  4 ++--
>>  drivers/gpu/drm/i915/intel_wopcm.c        |  4 ++--
>>  13 files changed, 21 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c 
>> b/drivers/gpu/drm/i915/gt/intel_reset.c
>> index 55e2ddcbd215..7fd135a5a41f 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
>> @@ -595,7 +595,7 @@ int intel_reset_guc(struct intel_gt *gt)
>>          INTEL_GEN(gt->i915) >= 11 ? GEN11_GRDOM_GUC : GEN9_GRDOM_GUC;
>>      int ret;
>> -    GEM_BUG_ON(!HAS_GUC(gt->i915));
>> +    GEM_BUG_ON(!HAS_UC(gt->i915));
>
> there is "guc" in function name
> there is "GUC" in bitfield name
>
> but you are checking UC ;)
>

I see nothing wrong with that, we defined UC as a super-set of GUC. It's 
like e.g. HAS_DISPLAY including all display features.

>>     intel_uncore_forcewake_get(gt->uncore, FORCEWAKE_ALL);
>>      ret = gen6_hw_domain_reset(gt, guc_domain);
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c 
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
>> index 3dfa40fdbe99..ddd3b2162528 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
>> @@ -80,7 +80,7 @@ static void guc_fw_select(struct intel_uc_fw *guc_fw)
>>     GEM_BUG_ON(guc_fw->type != INTEL_UC_FW_TYPE_GUC);
>> -    if (!HAS_GUC(i915)) {
>> +    if (!HAS_UC(i915)) {
>>          guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
>>          return;
>>      }
>> 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 543854c42d9d..15891cf4bb2c 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>> @@ -74,7 +74,7 @@ static void huc_fw_select(struct intel_uc_fw *huc_fw)
>>     GEM_BUG_ON(huc_fw->type != INTEL_UC_FW_TYPE_HUC);
>> -    if (!HAS_HUC(dev_priv)) {
>> +    if (!HAS_UC(dev_priv)) {
>>          huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
>>          return;
>>      }
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c 
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> index 4480a3dc2449..0d50b73f6cfe 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> @@ -61,7 +61,7 @@ static int __get_platform_enable_guc(struct 
>> intel_uc *uc)
>>      struct intel_uc_fw *huc_fw = &uc->huc.fw;
>>      int enable_guc = 0;
>> -    if (!HAS_GUC(uc_to_gt(uc)->i915))
>> +    if (!HAS_UC(uc_to_gt(uc)->i915))
>>          return 0;
>>     /* We don't want to enable GuC/HuC on pre-Gen11 by default */
>> diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c 
>> b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
>> index 93f7c930ab18..d1aa180050c4 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
>> @@ -134,7 +134,7 @@ static int igt_guc_clients(void *args)
>>      struct intel_guc *guc;
>>      int err = 0;
>> -    GEM_BUG_ON(!HAS_GUC(dev_priv));
>> +    GEM_BUG_ON(!HAS_UC(dev_priv));
>>      mutex_lock(&dev_priv->drm.struct_mutex);
>>      wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
>> @@ -226,7 +226,7 @@ static int igt_guc_doorbells(void *arg)
>>      int i, err = 0;
>>      u16 db_id;
>> -    GEM_BUG_ON(!HAS_GUC(dev_priv));
>> +    GEM_BUG_ON(!HAS_UC(dev_priv));
>>      mutex_lock(&dev_priv->drm.struct_mutex);
>>      wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 6b84d04a6a28..4249107f9d0d 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1865,7 +1865,7 @@ static int i915_huc_load_status_info(struct 
>> seq_file *m, void *data)
>>      intel_wakeref_t wakeref;
>>      struct drm_printer p;
>> -    if (!HAS_HUC(dev_priv))
>> +    if (!HAS_UC(dev_priv))
>>          return -ENODEV;
>>     p = drm_seq_file_printer(m);
>> @@ -1883,7 +1883,7 @@ static int i915_guc_load_status_info(struct 
>> seq_file *m, void *data)
>>      intel_wakeref_t wakeref;
>>      struct drm_printer p;
>> -    if (!HAS_GUC(dev_priv))
>> +    if (!HAS_UC(dev_priv))
>>          return -ENODEV;
>>     p = drm_seq_file_printer(m);
>> @@ -2062,7 +2062,7 @@ static int i915_guc_log_dump(struct seq_file 
>> *m, void *data)
>>      u32 *log;
>>      int i = 0;
>> -    if (!HAS_GUC(dev_priv))
>> +    if (!HAS_UC(dev_priv))
>>          return -ENODEV;
>>     if (dump_load_err)
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 0e44cc4b2ca1..06ddc5df12fa 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2271,20 +2271,9 @@ IS_SUBPLATFORM(const struct drm_i915_private 
>> *i915,
>> #define HAS_IPC(dev_priv) (INTEL_INFO(dev_priv)->display.has_ipc)
>> -/*
>> - * For now, anything with a GuC requires uCode loading, and then 
>> supports
>> - * command submission once loaded. But these are logically independent
>> - * properties, so we have separate macros to test them.
>> - */
>> -#define HAS_GUC(dev_priv)    (INTEL_INFO(dev_priv)->has_guc)
>> -#define HAS_GUC_UCODE(dev_priv)    (HAS_GUC(dev_priv))
>> -#define HAS_GUC_SCHED(dev_priv)    (HAS_GUC(dev_priv))
>
> yep, _UCODE and _SCHED should be nuked
>
>> -
>> -/* For now, anything with a GuC has also HuC */
>> -#define HAS_HUC(dev_priv)    (HAS_GUC(dev_priv))
>> -#define HAS_HUC_UCODE(dev_priv)    (HAS_GUC(dev_priv))
>> +#define HAS_UC(dev_priv)    (INTEL_INFO(dev_priv)->has_uc)
>
> time to use i915 instead of dev_priv

ok

>
>> -/* Having a GuC is not the same as using a GuC */
>> +/* Having a uC is not the same as using a uC */
>>  #define USES_GUC(dev_priv) intel_uc_is_using_guc(&(dev_priv)->gt.uc)
>>  #define USES_GUC_SUBMISSION(dev_priv) 
>> intel_uc_is_using_guc_submission(&(dev_priv)->gt.uc)
>>  #define USES_HUC(dev_priv) intel_uc_is_using_huc(&(dev_priv)->gt.uc)
>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c 
>> b/drivers/gpu/drm/i915/i915_gpu_error.c
>> index c5b89bf4d616..1db4d7302734 100644
>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>> @@ -620,7 +620,7 @@ static void err_print_uc(struct 
>> drm_i915_error_state_buf *m,
>>      const struct i915_gpu_state *error =
>>          container_of(error_uc, typeof(*error), uc);
>> -    if (!error->device_info.has_guc)
>> +    if (!error->device_info.has_uc)
>>          return;
>>     intel_uc_fw_dump(&error_uc->guc_fw, &p);
>> @@ -1557,7 +1557,7 @@ static void capture_uc_state(struct 
>> i915_gpu_state *error)
>>      struct intel_uc *uc = &i915->gt.uc;
>>     /* Capturing uC state won't be useful if there is no GuC */
>> -    if (!error->device_info.has_guc)
>> +    if (!error->device_info.has_uc)
>>          return;
>>     error_uc->guc_fw = uc->guc.fw;
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c 
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index 11c73af92597..0b2351a6c490 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -4766,7 +4766,7 @@ void intel_irq_init(struct drm_i915_private 
>> *dev_priv)
>>          dev_priv->l3_parity.remap_info[i] = NULL;
>>     /* pre-gen11 the guc irqs bits are in the upper 16 bits of the pm 
>> reg */
>> -    if (HAS_GUC_SCHED(dev_priv) && INTEL_GEN(dev_priv) < 11)
>> +    if (HAS_UC(dev_priv) && INTEL_GEN(dev_priv) < 11)
>>          dev_priv->gt.pm_guc_events = GUC_INTR_GUC2HOST << 16;
>>     /* Let's track the enabled rps events */
>> diff --git a/drivers/gpu/drm/i915/i915_pci.c 
>> b/drivers/gpu/drm/i915/i915_pci.c
>> index 40076ba431d4..b5ece92683fd 100644
>> --- a/drivers/gpu/drm/i915/i915_pci.c
>> +++ b/drivers/gpu/drm/i915/i915_pci.c
>> @@ -595,7 +595,7 @@ static const struct intel_device_info 
>> intel_cherryview_info = {
>>      GEN9_DEFAULT_PAGE_SIZES, \
>>      .has_logical_ring_preemption = 1, \
>>      .display.has_csr = 1, \
>> -    .has_guc = 1, \
>> +    .has_uc = 1, \
>>      .display.has_ipc = 1, \
>>      .ddb_size = 896
>> @@ -647,7 +647,7 @@ static const struct intel_device_info 
>> intel_skylake_gt4_info = {
>>      .display.has_dp_mst = 1, \
>>      .has_logical_ring_contexts = 1, \
>>      .has_logical_ring_preemption = 1, \
>> -    .has_guc = 1, \
>> +    .has_uc = 1, \
>>      .ppgtt_type = INTEL_PPGTT_FULL, \
>>      .ppgtt_size = 48, \
>>      .has_reset_engine = 1, \
>> diff --git a/drivers/gpu/drm/i915/intel_device_info.h 
>> b/drivers/gpu/drm/i915/intel_device_info.h
>> index 45a9badc9b8e..99e5b22e26f6 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.h
>> +++ b/drivers/gpu/drm/i915/intel_device_info.h
>> @@ -112,7 +112,6 @@ enum intel_ppgtt_type {
>>      func(gpu_reset_clobbers_display); \
>>      func(has_reset_engine); \
>>      func(has_fpga_dbg); \
>> -    func(has_guc); \
>>      func(has_l3_dpf); \
>>      func(has_llc); \
>>      func(has_logical_ring_contexts); \
>> @@ -124,6 +123,7 @@ enum intel_ppgtt_type {
>>      func(has_rps); \
>>      func(has_runtime_pm); \
>>      func(has_snoop); \
>> +    func(has_uc); \
>>      func(has_coherent_ggtt); \
>>      func(unfenced_needs_alignment); \
>>      func(hws_needs_physical);
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c 
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index 22472f2bd31b..53ef862731f6 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -7162,7 +7162,7 @@ static void gen11_enable_rc6(struct 
>> drm_i915_private *dev_priv)
>>      for_each_engine(engine, dev_priv, id)
>>          I915_WRITE(RING_MAX_IDLE(engine->mmio_base), 10);
>> -    if (HAS_GUC(dev_priv))
>> +    if (HAS_UC(dev_priv))
>>          I915_WRITE(GUC_MAX_IDLE_COUNT, 0xA);
>>     I915_WRITE(GEN6_RC_SLEEP, 0);
>> @@ -7243,7 +7243,7 @@ static void gen9_enable_rc6(struct 
>> drm_i915_private *dev_priv)
>>      for_each_engine(engine, dev_priv, id)
>>          I915_WRITE(RING_MAX_IDLE(engine->mmio_base), 10);
>> -    if (HAS_GUC(dev_priv))
>> +    if (HAS_UC(dev_priv))
>>          I915_WRITE(GUC_MAX_IDLE_COUNT, 0xA);
>>     I915_WRITE(GEN6_RC_SLEEP, 0);
>> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c 
>> b/drivers/gpu/drm/i915/intel_wopcm.c
>> index fafd4e6a1147..c966d572b0c4 100644
>> --- a/drivers/gpu/drm/i915/intel_wopcm.c
>> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
>> @@ -74,7 +74,7 @@ void intel_wopcm_init_early(struct intel_wopcm *wopcm)
>>  {
>>      struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
>> -    if (!HAS_GUC(i915))
>> +    if (!HAS_UC(i915))
>>          return;
>>     if (INTEL_GEN(i915) >= 11)
>> @@ -263,7 +263,7 @@ int intel_wopcm_init_hw(struct intel_wopcm 
>> *wopcm, struct intel_gt *gt)
>>      if (!USES_GUC(i915))
>>          return 0;
>> -    GEM_BUG_ON(!HAS_GUC(i915));
>> +    GEM_BUG_ON(!HAS_UC(i915));
>
> hmm, maybe
>
>     #define HAS_WOPCM(i915)    HAS_GT_UC(i915)

This is not technically true. The WOPCM is a piece of memory that is 
there and used on legacy platforms for non-uc things as well (e.g. the 
power ctx save/restore). That's also the reason why I haven't moved the 
wopcm under intel_uc.
What we care about here is programming the uC portion of the wopcm, 
which is why we require the uC. Once we fix the sanitizing of the 
modparam the USES_GUC() above will probably be enough and we should be 
able to drop the BUG_ON.

Daniele

>
> and then
>
>     GEM_BUG_ON(!HAS_WOPCM(i915));
>
>>      GEM_BUG_ON(!wopcm->guc.size);
>>      GEM_BUG_ON(!wopcm->guc.base);

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

Re: [PATCH 3/9] drm/i915/uc: Unify uC FW selection

[Daniele Ceraolo Spurio]

On 7/23/2019 6:22 AM, Michal Wajdeczko wrote:
> On Tue, 23 Jul 2019 01:20:42 +0200, Daniele Ceraolo Spurio 
> <daniele.ceraolospurio@intel.com> wrote:
>
>> +
>> +#define GUC_FW_BLOB(prefix_, major_, minor_, patch_) \
>> +UC_FW_BLOB(prefix_##_guc, major_, minor_, \
>> +       __MAKE_GUC_FW_PATH(prefix_, major_, minor_, patch_))
>> +
>> +GUC_FW_BLOB(skl, 33, 0, 0);
>> +GUC_FW_BLOB(bxt, 33, 0, 0);
>> +GUC_FW_BLOB(kbl, 33, 0, 0);
>> +GUC_FW_BLOB(glk, 33, 0, 0);
>> +GUC_FW_BLOB(icl, 33, 0, 0);
>> +
>> +/* must be ordered base on platform + revid, from newer to older */
>> +static const struct intel_uc_platform_requirement guc_fw_blobs[] = {
>> +    { INTEL_ICELAKE,    0,    &icl_guc_fw_blob },
>> +    { INTEL_COFFEELAKE,    0,    &kbl_guc_fw_blob },
>> +    { INTEL_GEMINILAKE,    0,    &glk_guc_fw_blob },
>> +    { INTEL_KABYLAKE,    0,    &kbl_guc_fw_blob },
>> +    { INTEL_BROXTON,    0,    &bxt_guc_fw_blob },
>> +    { INTEL_SKYLAKE,    0,    &skl_guc_fw_blob },
>> +};
>
> Can we avoid pointers to separate blob definitions ?
> What about defining each fw in single line like below
>
> #define INTEL_GUC_FIRMWARE_DEFS(fw_def) \
>     fw_def(ICELAKE, 0, 33, 0, 0, icl, GUC) \
>     fw_def(COFFEELAKE, 0, 33, 0, 0, kbl, GUC) \
>     fw_def(GEMINILAKE, 0, 33, 0, 0, glk, GUC) \
>     fw_def(KABYLAKE, 0, 33, 0, 0, kbl, GUC) \
>     fw_def(BROXTON, 0, 33, 0, 0, bxt, GUC) \
>     fw_def(SKYLAKE, 0, 33, 0, 0, skl, GUC) \
>     /* end */
>
> with some extra common helpers
>
> #define TO_MODULE_FIRMWARE(_platform, _rev, _major, _minor, _patch, 
> _prefix, _builder) \
>     MODULE_FIRMWARE(_builder##_FW_PATH_BUILDER(_prefix, _major, 
> _minor, _patch));
>
> #define TO_BLOB_ENTRY(_platform, _rev, _major, _minor, _patch, 
> _prefix, _builder) \
> { \
>     .platform = INTEL_##_platform, \
>     .rev = (_rev), \
>     .major = (_major), \
>     .minor = (_minor), \
>     .patch = (_patch), \
>     .blob = _builder##_FW_PATH_BUILDER(_prefix, _major, _minor, 
> _patch), \
> },
>
> then we can have immutable
>
> static const struct intel_uc_blob guc_fw_blobs[] = {
> INTEL_GUC_FIRMWARE_DEFS(TO_BLOB_ENTRY)
> };
> INTEL_GUC_FIRMWARE_DEFS(TO_MODULE_FIRMWARE)
>
> (tested locally for feasibility)
>
> Michal

LGTM, will update. I went with the list because I wanted to ultimately 
unify the GuC and HuC lists into one, but I should be able to set the 
macros appropriately to allow that as the next step.

Daniele

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

Re: [PATCH 2/9] drm/i915/uc: Unify uC platform check

[Daniele Ceraolo Spurio]

<snip>

>>> -
>>> -/* For now, anything with a GuC has also HuC */
>>> -#define HAS_HUC(dev_priv)    (HAS_GUC(dev_priv))
>>> -#define HAS_HUC_UCODE(dev_priv)    (HAS_GUC(dev_priv))
>>> +#define HAS_UC(dev_priv)    (INTEL_INFO(dev_priv)->has_uc)
>>
>> time to use i915 instead of dev_priv
> 
> ok
> 

I've decided against this in the end because all the other HAS_* checks 
in the file use dev_priv and a single i915 sticks out like a sore thumb. 
Probably better to flip them all at the same time when we're ready.

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