[P v4 00/11] drm/i915/uc: Firmware code reorg

[P v4 00/11] drm/i915/uc: Firmware code reorg

[Michal Wajdeczko]

Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>


Michal Wajdeczko (11):
  drm/i915: Move intel_guc_wopcm_size to intel_guc.c
  drm/i915/guc: Move GuC boot param initialization out of xfer
  drm/i915/guc: Move doc near related definitions
  drm/i915/guc: Rename intel_guc_loader.c to intel_guc_fw.c
  drm/i915/guc: Reorder functions in intel_guc_fw.c
  drm/i915/guc: Move firmware size check out of generic code
  drm/i915/guc: Pick better place for Guc final status message
  drm/i915/uc: Improve debug messages in firmware fetch
  drm/i915/uc: Unify firmware loading
  drm/i915/huc: Move fw select function
  HAX enable GuC submission for CI

 drivers/gpu/drm/i915/Makefile           |   2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c     |   8 +-
 drivers/gpu/drm/i915/i915_params.h      |   4 +-
 drivers/gpu/drm/i915/intel_guc.c        |  99 ++++++++
 drivers/gpu/drm/i915/intel_guc.h        |  16 +-
 drivers/gpu/drm/i915/intel_guc_fw.c     | 256 ++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc_fw.h     |  33 +++
 drivers/gpu/drm/i915/intel_guc_loader.c | 417 --------------------------------
 drivers/gpu/drm/i915/intel_huc.c        | 125 ++++------
 drivers/gpu/drm/i915/intel_uc.c         |  11 +-
 drivers/gpu/drm/i915/intel_uc_fw.c      | 162 ++++++++++---
 drivers/gpu/drm/i915/intel_uc_fw.h      |   9 +
 12 files changed, 590 insertions(+), 552 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_guc_fw.c
 create mode 100644 drivers/gpu/drm/i915/intel_guc_fw.h
 delete mode 100644 drivers/gpu/drm/i915/intel_guc_loader.c

-- 
2.7.4

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

[P v4 01/11] drm/i915: Move intel_guc_wopcm_size to intel_guc.c

[Michal Wajdeczko]

Function intel_guc_wopcm_size didn't fit well in the old place.
With this move upcoming cleanup will be simpler.

Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_guc.c        | 11 +++++++++++
 drivers/gpu/drm/i915/intel_guc_loader.c | 11 -----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index bbe4c32..90c3dd8 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -262,3 +262,14 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
 	i915_gem_object_put(obj);
 	return vma;
 }
+
+u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv)
+{
+	u32 wopcm_size = GUC_WOPCM_TOP;
+
+	/* On BXT, the top of WOPCM is reserved for RC6 context */
+	if (IS_GEN9_LP(dev_priv))
+		wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED;
+
+	return wopcm_size;
+}
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index c7a800a..30b70f5 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -250,17 +250,6 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
 	return ret;
 }
 
-u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv)
-{
-	u32 wopcm_size = GUC_WOPCM_TOP;
-
-	/* On BXT, the top of WOPCM is reserved for RC6 context */
-	if (IS_GEN9_LP(dev_priv))
-		wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED;
-
-	return wopcm_size;
-}
-
 /*
  * Load the GuC firmware blob into the MinuteIA.
  */
-- 
2.7.4

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

[P v4 02/11] drm/i915/guc: Move GuC boot param initialization out of xfer

[Michal Wajdeczko]

We want to keep ucode xfer functions separate from other
initialization. Once separated, add explicit forcewake.

Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_guc.c        | 88 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc.h        |  1 +
 drivers/gpu/drm/i915/intel_guc_loader.c | 85 -------------------------------
 drivers/gpu/drm/i915/intel_uc.c         |  1 +
 4 files changed, 90 insertions(+), 85 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 90c3dd8..d75515c 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -67,6 +67,94 @@ void intel_guc_init_early(struct intel_guc *guc)
 	guc->notify = gen8_guc_raise_irq;
 }
 
+static u32 get_gttype(struct drm_i915_private *dev_priv)
+{
+	/* XXX: GT type based on PCI device ID? field seems unused by fw */
+	return 0;
+}
+
+static u32 get_core_family(struct drm_i915_private *dev_priv)
+{
+	u32 gen = INTEL_GEN(dev_priv);
+
+	switch (gen) {
+	case 9:
+		return GUC_CORE_FAMILY_GEN9;
+
+	default:
+		MISSING_CASE(gen);
+		return GUC_CORE_FAMILY_UNKNOWN;
+	}
+}
+
+/*
+ * Initialise the GuC parameter block before starting the firmware
+ * transfer. These parameters are read by the firmware on startup
+ * and cannot be changed thereafter.
+ */
+void intel_guc_init_params(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	u32 params[GUC_CTL_MAX_DWORDS];
+	int i;
+
+	memset(&params, 0, sizeof(params));
+
+	params[GUC_CTL_DEVICE_INFO] |=
+		(get_gttype(dev_priv) << GUC_CTL_GTTYPE_SHIFT) |
+		(get_core_family(dev_priv) << GUC_CTL_COREFAMILY_SHIFT);
+
+	/*
+	 * GuC ARAT increment is 10 ns. GuC default scheduler quantum is one
+	 * second. This ARAR is calculated by:
+	 * Scheduler-Quantum-in-ns / ARAT-increment-in-ns = 1000000000 / 10
+	 */
+	params[GUC_CTL_ARAT_HIGH] = 0;
+	params[GUC_CTL_ARAT_LOW] = 100000000;
+
+	params[GUC_CTL_WA] |= GUC_CTL_WA_UK_BY_DRIVER;
+
+	params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
+			GUC_CTL_VCS2_ENABLED;
+
+	params[GUC_CTL_LOG_PARAMS] = guc->log.flags;
+
+	if (i915_modparams.guc_log_level >= 0) {
+		params[GUC_CTL_DEBUG] =
+			i915_modparams.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
+	} else
+		params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
+
+	/* If GuC submission is enabled, set up additional parameters here */
+	if (i915_modparams.enable_guc_submission) {
+		u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
+		u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool);
+		u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
+
+		params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT;
+		params[GUC_CTL_DEBUG] |= GUC_ADS_ENABLED;
+
+		pgs >>= PAGE_SHIFT;
+		params[GUC_CTL_CTXINFO] = (pgs << GUC_CTL_BASE_ADDR_SHIFT) |
+			(ctx_in_16 << GUC_CTL_CTXNUM_IN16_SHIFT);
+
+		params[GUC_CTL_FEATURE] |= GUC_CTL_KERNEL_SUBMISSIONS;
+
+		/* Unmask this bit to enable the GuC's internal scheduler */
+		params[GUC_CTL_FEATURE] &= ~GUC_CTL_DISABLE_SCHEDULER;
+	}
+
+	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
+
+	I915_WRITE(SOFT_SCRATCH(0), 0);
+
+	for (i = 0; i < GUC_CTL_MAX_DWORDS; i++)
+		I915_WRITE(SOFT_SCRATCH(1 + i), params[i]);
+
+	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+
+}
+
 int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
 {
 	WARN(1, "Unexpected send: action=%#x\n", *action);
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index aa9a7b5..8b44165 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -95,6 +95,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 
 void intel_guc_init_early(struct intel_guc *guc);
 void intel_guc_init_send_regs(struct intel_guc *guc);
+void intel_guc_init_params(struct intel_guc *guc);
 int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
 int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 30b70f5..d9089bc 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -79,89 +79,6 @@ MODULE_FIRMWARE(I915_KBL_GUC_UCODE);
 #define I915_GLK_GUC_UCODE GUC_FW_PATH(glk, GLK_FW_MAJOR, GLK_FW_MINOR)
 
 
-static u32 get_gttype(struct drm_i915_private *dev_priv)
-{
-	/* XXX: GT type based on PCI device ID? field seems unused by fw */
-	return 0;
-}
-
-static u32 get_core_family(struct drm_i915_private *dev_priv)
-{
-	u32 gen = INTEL_GEN(dev_priv);
-
-	switch (gen) {
-	case 9:
-		return GUC_CORE_FAMILY_GEN9;
-
-	default:
-		MISSING_CASE(gen);
-		return GUC_CORE_FAMILY_UNKNOWN;
-	}
-}
-
-/*
- * Initialise the GuC parameter block before starting the firmware
- * transfer. These parameters are read by the firmware on startup
- * and cannot be changed thereafter.
- */
-static void guc_params_init(struct drm_i915_private *dev_priv)
-{
-	struct intel_guc *guc = &dev_priv->guc;
-	u32 params[GUC_CTL_MAX_DWORDS];
-	int i;
-
-	memset(&params, 0, sizeof(params));
-
-	params[GUC_CTL_DEVICE_INFO] |=
-		(get_gttype(dev_priv) << GUC_CTL_GTTYPE_SHIFT) |
-		(get_core_family(dev_priv) << GUC_CTL_COREFAMILY_SHIFT);
-
-	/*
-	 * GuC ARAT increment is 10 ns. GuC default scheduler quantum is one
-	 * second. This ARAR is calculated by:
-	 * Scheduler-Quantum-in-ns / ARAT-increment-in-ns = 1000000000 / 10
-	 */
-	params[GUC_CTL_ARAT_HIGH] = 0;
-	params[GUC_CTL_ARAT_LOW] = 100000000;
-
-	params[GUC_CTL_WA] |= GUC_CTL_WA_UK_BY_DRIVER;
-
-	params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
-			GUC_CTL_VCS2_ENABLED;
-
-	params[GUC_CTL_LOG_PARAMS] = guc->log.flags;
-
-	if (i915_modparams.guc_log_level >= 0) {
-		params[GUC_CTL_DEBUG] =
-			i915_modparams.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
-	} else
-		params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
-
-	/* If GuC submission is enabled, set up additional parameters here */
-	if (i915_modparams.enable_guc_submission) {
-		u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
-		u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool);
-		u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
-
-		params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT;
-		params[GUC_CTL_DEBUG] |= GUC_ADS_ENABLED;
-
-		pgs >>= PAGE_SHIFT;
-		params[GUC_CTL_CTXINFO] = (pgs << GUC_CTL_BASE_ADDR_SHIFT) |
-			(ctx_in_16 << GUC_CTL_CTXNUM_IN16_SHIFT);
-
-		params[GUC_CTL_FEATURE] |= GUC_CTL_KERNEL_SUBMISSIONS;
-
-		/* Unmask this bit to enable the GuC's internal scheduler */
-		params[GUC_CTL_FEATURE] &= ~GUC_CTL_DISABLE_SCHEDULER;
-	}
-
-	I915_WRITE(SOFT_SCRATCH(0), 0);
-
-	for (i = 0; i < GUC_CTL_MAX_DWORDS; i++)
-		I915_WRITE(SOFT_SCRATCH(1 + i), params[i]);
-}
-
 /*
  * Read the GuC status register (GUC_STATUS) and store it in the
  * specified location; then return a boolean indicating whether
@@ -301,8 +218,6 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
 		I915_WRITE(GUC_ARAT_C6DIS, 0x1FF);
 	}
 
-	guc_params_init(dev_priv);
-
 	ret = guc_ucode_xfer_dma(dev_priv, vma);
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 7b938e8..53fdd9a 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -195,6 +195,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 			goto err_submission;
 
 		intel_huc_init_hw(&dev_priv->huc);
+		intel_guc_init_params(guc);
 		ret = intel_guc_init_hw(&dev_priv->guc);
 		if (ret == 0 || ret != -EAGAIN)
 			break;
-- 
2.7.4

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

[P v4 03/11] drm/i915/guc: Move doc near related definitions

[Michal Wajdeczko]

After Guc code reorg some documentation was left in wrong
place. Move it closer to corresponding definitions.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_guc.h        | 11 +++++++++++
 drivers/gpu/drm/i915/intel_guc_loader.c | 23 -----------------------
 drivers/gpu/drm/i915/intel_uc_fw.h      |  5 +++++
 3 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 8b44165..c7088a7 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -33,6 +33,11 @@
 #include "i915_guc_reg.h"
 #include "i915_vma.h"
 
+/*
+ * Top level structure of guc. It handles firmware loading and manages client
+ * pool and doorbells. intel_guc owns a i915_guc_client to replace the legacy
+ * ExecList submission.
+ */
 struct intel_guc {
 	struct intel_uc_fw fw;
 	struct intel_guc_log log;
@@ -83,6 +88,12 @@ static inline void intel_guc_notify(struct intel_guc *guc)
 	guc->notify(guc);
 }
 
+/*
+ * GuC does not allow any gfx GGTT address that falls into range [0, WOPCM_TOP),
+ * which is reserved for Boot ROM, SRAM and WOPCM. Currently this top address is
+ * 512K. In order to exclude 0-512K address space from GGTT, all gfx objects
+ * used by GuC is pinned with PIN_OFFSET_BIAS along with size of WOPCM.
+ */
 static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 {
 	u32 offset = i915_ggtt_offset(vma);
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index d9089bc..8508b94 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -29,29 +29,6 @@
 #include "i915_drv.h"
 #include "intel_uc.h"
 
-/**
- * DOC: GuC-specific firmware loader
- *
- * intel_guc:
- * Top level structure of guc. It handles firmware loading and manages client
- * pool and doorbells. intel_guc owns a i915_guc_client to replace the legacy
- * ExecList submission.
- *
- * Firmware versioning:
- * The firmware build process will generate a version header file with major and
- * minor version defined. The versions are built into CSS header of firmware.
- * i915 kernel driver set the minimal firmware version required per platform.
- * The firmware installation package will install (symbolic link) proper version
- * of firmware.
- *
- * GuC address space:
- * GuC does not allow any gfx GGTT address that falls into range [0, WOPCM_TOP),
- * which is reserved for Boot ROM, SRAM and WOPCM. Currently this top address is
- * 512K. In order to exclude 0-512K address space from GGTT, all gfx objects
- * used by GuC is pinned with PIN_OFFSET_BIAS along with size of WOPCM.
- *
- */
-
 #define SKL_FW_MAJOR 6
 #define SKL_FW_MINOR 1
 
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h
index c3e9af4..5c01849 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/intel_uc_fw.h
@@ -50,6 +50,11 @@ struct intel_uc_fw {
 	enum intel_uc_fw_status fetch_status;
 	enum intel_uc_fw_status load_status;
 
+	/*
+	 * The firmware build process will generate a version header file with major and
+	 * minor version defined. The versions are built into CSS header of firmware.
+	 * i915 kernel driver set the minimal firmware version required per platform.
+	 */
 	u16 major_ver_wanted;
 	u16 minor_ver_wanted;
 	u16 major_ver_found;
-- 
2.7.4

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

[P v4 04/11] drm/i915/guc: Rename intel_guc_loader.c to intel_guc_fw.c

[Michal Wajdeczko]

Remaining functions in intel_guc_loader.c were focused around
GuC firmware. Rename them to match object-verb pattern and
rename file itself.

Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/Makefile           |   2 +-
 drivers/gpu/drm/i915/intel_guc.h        |   4 +-
 drivers/gpu/drm/i915/intel_guc_fw.c     | 298 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc_fw.h     |  33 ++++
 drivers/gpu/drm/i915/intel_guc_loader.c | 298 --------------------------------
 drivers/gpu/drm/i915/intel_uc.c         |   4 +-
 6 files changed, 335 insertions(+), 304 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_guc_fw.c
 create mode 100644 drivers/gpu/drm/i915/intel_guc_fw.h
 delete mode 100644 drivers/gpu/drm/i915/intel_guc_loader.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 66d23b6..6c3b048 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -64,7 +64,7 @@ i915-y += intel_uc.o \
 	  intel_guc.o \
 	  intel_guc_ct.o \
 	  intel_guc_log.o \
-	  intel_guc_loader.o \
+	  intel_guc_fw.o \
 	  intel_huc.o \
 	  i915_guc_submission.o
 
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index c7088a7..f643e04 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -26,6 +26,7 @@
 #define _INTEL_GUC_H_
 
 #include "intel_uncore.h"
+#include "intel_guc_fw.h"
 #include "intel_guc_fwif.h"
 #include "intel_guc_ct.h"
 #include "intel_guc_log.h"
@@ -114,9 +115,6 @@ int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
 int intel_guc_suspend(struct drm_i915_private *dev_priv);
 int intel_guc_resume(struct drm_i915_private *dev_priv);
 struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
-
-int intel_guc_select_fw(struct intel_guc *guc);
-int intel_guc_init_hw(struct intel_guc *guc);
 u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
new file mode 100644
index 0000000..61d6369
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_guc_fw.c
@@ -0,0 +1,298 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Vinit Azad <vinit.azad@intel.com>
+ *    Ben Widawsky <ben@bwidawsk.net>
+ *    Dave Gordon <david.s.gordon@intel.com>
+ *    Alex Dai <yu.dai@intel.com>
+ */
+
+#include "intel_guc_fw.h"
+#include "i915_drv.h"
+
+#define SKL_FW_MAJOR 6
+#define SKL_FW_MINOR 1
+
+#define BXT_FW_MAJOR 8
+#define BXT_FW_MINOR 7
+
+#define KBL_FW_MAJOR 9
+#define KBL_FW_MINOR 14
+
+#define GLK_FW_MAJOR 10
+#define GLK_FW_MINOR 56
+
+#define GUC_FW_PATH(platform, major, minor) \
+       "i915/" __stringify(platform) "_guc_ver" __stringify(major) "_" __stringify(minor) ".bin"
+
+#define I915_SKL_GUC_UCODE GUC_FW_PATH(skl, SKL_FW_MAJOR, SKL_FW_MINOR)
+MODULE_FIRMWARE(I915_SKL_GUC_UCODE);
+
+#define I915_BXT_GUC_UCODE GUC_FW_PATH(bxt, BXT_FW_MAJOR, BXT_FW_MINOR)
+MODULE_FIRMWARE(I915_BXT_GUC_UCODE);
+
+#define I915_KBL_GUC_UCODE GUC_FW_PATH(kbl, KBL_FW_MAJOR, KBL_FW_MINOR)
+MODULE_FIRMWARE(I915_KBL_GUC_UCODE);
+
+#define I915_GLK_GUC_UCODE GUC_FW_PATH(glk, GLK_FW_MAJOR, GLK_FW_MINOR)
+
+/*
+ * Read the GuC status register (GUC_STATUS) and store it in the
+ * specified location; then return a boolean indicating whether
+ * the value matches either of two values representing completion
+ * of the GuC boot process.
+ *
+ * This is used for polling the GuC status in a wait_for()
+ * loop below.
+ */
+static inline bool guc_ucode_response(struct drm_i915_private *dev_priv,
+				      u32 *status)
+{
+	u32 val = I915_READ(GUC_STATUS);
+	u32 uk_val = val & GS_UKERNEL_MASK;
+	*status = val;
+	return (uk_val == GS_UKERNEL_READY ||
+		((val & GS_MIA_CORE_STATE) && uk_val == GS_UKERNEL_LAPIC_DONE));
+}
+
+/*
+ * Transfer the firmware image to RAM for execution by the microcontroller.
+ *
+ * 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.
+ *
+ * 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.
+ */
+static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
+			      struct i915_vma *vma)
+{
+	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
+	unsigned long offset;
+	struct sg_table *sg = vma->pages;
+	u32 status, rsa[UOS_RSA_SCRATCH_MAX_COUNT];
+	int i, ret = 0;
+
+	/* where RSA signature starts */
+	offset = guc_fw->rsa_offset;
+
+	/* Copy RSA signature from the fw image to HW for verification */
+	sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, sizeof(rsa), offset);
+	for (i = 0; i < UOS_RSA_SCRATCH_MAX_COUNT; i++)
+		I915_WRITE(UOS_RSA_SCRATCH(i), rsa[i]);
+
+	/* The header plus uCode will be copied to WOPCM via DMA, excluding any
+	 * other components */
+	I915_WRITE(DMA_COPY_SIZE, guc_fw->header_size + guc_fw->ucode_size);
+
+	/* Set the source address for the new blob */
+	offset = guc_ggtt_offset(vma) + guc_fw->header_offset;
+	I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset));
+	I915_WRITE(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.
+	 */
+	I915_WRITE(DMA_ADDR_1_LOW, 0x2000);
+	I915_WRITE(DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
+
+	/* Finally start the DMA */
+	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA));
+
+	/*
+	 * Wait for the DMA to complete & the GuC to start up.
+	 * NB: Docs recommend not using the interrupt for completion.
+	 * Measurements indicate this should take no more than 20ms, so a
+	 * timeout here indicates that the GuC has failed and is unusable.
+	 * (Higher levels of the driver will attempt to fall back to
+	 * execlist mode if this happens.)
+	 */
+	ret = wait_for(guc_ucode_response(dev_priv, &status), 100);
+
+	DRM_DEBUG_DRIVER("DMA status 0x%x, GuC status 0x%x\n",
+			I915_READ(DMA_CTRL), status);
+
+	if ((status & GS_BOOTROM_MASK) == GS_BOOTROM_RSA_FAILED) {
+		DRM_ERROR("GuC firmware signature verification failed\n");
+		ret = -ENOEXEC;
+	}
+
+	DRM_DEBUG_DRIVER("returning %d\n", ret);
+
+	return ret;
+}
+
+/*
+ * Load the GuC firmware blob into the MinuteIA.
+ */
+static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
+{
+	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
+	struct i915_vma *vma;
+	int ret;
+
+	ret = i915_gem_object_set_to_gtt_domain(guc_fw->obj, false);
+	if (ret) {
+		DRM_DEBUG_DRIVER("set-domain failed %d\n", ret);
+		return ret;
+	}
+
+	vma = i915_gem_object_ggtt_pin(guc_fw->obj, NULL, 0, 0,
+				       PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
+	if (IS_ERR(vma)) {
+		DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
+		return PTR_ERR(vma);
+	}
+
+	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
+
+	/* Enable MIA caching. GuC clock gating is disabled. */
+	I915_WRITE(GUC_SHIM_CONTROL, GUC_SHIM_CONTROL_VALUE);
+
+	/* WaDisableMinuteIaClockGating:bxt */
+	if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_A1)) {
+		I915_WRITE(GUC_SHIM_CONTROL, (I915_READ(GUC_SHIM_CONTROL) &
+					      ~GUC_ENABLE_MIA_CLOCK_GATING));
+	}
+
+	/* WaC6DisallowByGfxPause:bxt */
+	if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_B0))
+		I915_WRITE(GEN6_GFXPAUSE, 0x30FFF);
+
+	if (IS_GEN9_LP(dev_priv))
+		I915_WRITE(GEN9LP_GT_PM_CONFIG, GT_DOORBELL_ENABLE);
+	else
+		I915_WRITE(GEN9_GT_PM_CONFIG, GT_DOORBELL_ENABLE);
+
+	if (IS_GEN9(dev_priv)) {
+		/* DOP Clock Gating Enable for GuC clocks */
+		I915_WRITE(GEN7_MISCCPCTL, (GEN8_DOP_CLOCK_GATE_GUC_ENABLE |
+					    I915_READ(GEN7_MISCCPCTL)));
+
+		/* allows for 5us (in 10ns units) before GT can go to RC6 */
+		I915_WRITE(GUC_ARAT_C6DIS, 0x1FF);
+	}
+
+	ret = guc_ucode_xfer_dma(dev_priv, vma);
+
+	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+
+	/*
+	 * We keep the object pages for reuse during resume. But we can unpin it
+	 * now that DMA has completed, so it doesn't continue to take up space.
+	 */
+	i915_vma_unpin(vma);
+
+	return ret;
+}
+
+/**
+ * intel_guc_fw_upload() - finish preparing the GuC for activity
+ * @guc: intel_guc structure
+ *
+ * Called during driver loading and also after a GPU reset.
+ *
+ * The main action required here it to load the GuC uCode into the device.
+ * The firmware image should have already been fetched into memory by the
+ * earlier call to intel_guc_init(), so here we need only check that
+ * worked, and then transfer the image to the h/w.
+ *
+ * Return:	non-zero code on error
+ */
+int intel_guc_fw_upload(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	const char *fw_path = guc->fw.path;
+	int ret;
+
+	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
+		fw_path,
+		intel_uc_fw_status_repr(guc->fw.fetch_status),
+		intel_uc_fw_status_repr(guc->fw.load_status));
+
+	if (guc->fw.fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
+		return -EIO;
+
+	guc->fw.load_status = INTEL_UC_FIRMWARE_PENDING;
+
+	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
+		intel_uc_fw_status_repr(guc->fw.fetch_status),
+		intel_uc_fw_status_repr(guc->fw.load_status));
+
+	ret = guc_ucode_xfer(dev_priv);
+
+	if (ret)
+		return -EAGAIN;
+
+	guc->fw.load_status = INTEL_UC_FIRMWARE_SUCCESS;
+
+	DRM_INFO("GuC %s (firmware %s [version %u.%u])\n",
+		 i915_modparams.enable_guc_submission ? "submission enabled" :
+							"loaded",
+		 guc->fw.path,
+		 guc->fw.major_ver_found, guc->fw.minor_ver_found);
+
+	return 0;
+}
+
+/**
+ * intel_guc_fw_select() - selects GuC firmware for loading
+ * @guc:	intel_guc struct
+ *
+ * Return: zero when we know firmware, non-zero in other case
+ */
+int intel_guc_fw_select(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+	intel_uc_fw_init(&guc->fw, INTEL_UC_FW_TYPE_GUC);
+
+	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_SKYLAKE(dev_priv)) {
+		guc->fw.path = I915_SKL_GUC_UCODE;
+		guc->fw.major_ver_wanted = SKL_FW_MAJOR;
+		guc->fw.minor_ver_wanted = SKL_FW_MINOR;
+	} else if (IS_BROXTON(dev_priv)) {
+		guc->fw.path = I915_BXT_GUC_UCODE;
+		guc->fw.major_ver_wanted = BXT_FW_MAJOR;
+		guc->fw.minor_ver_wanted = BXT_FW_MINOR;
+	} else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) {
+		guc->fw.path = I915_KBL_GUC_UCODE;
+		guc->fw.major_ver_wanted = KBL_FW_MAJOR;
+		guc->fw.minor_ver_wanted = KBL_FW_MINOR;
+	} else if (IS_GEMINILAKE(dev_priv)) {
+		guc->fw.path = I915_GLK_GUC_UCODE;
+		guc->fw.major_ver_wanted = GLK_FW_MAJOR;
+		guc->fw.minor_ver_wanted = GLK_FW_MINOR;
+	} else {
+		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
+		return -ENOENT;
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/intel_guc_fw.h b/drivers/gpu/drm/i915/intel_guc_fw.h
new file mode 100644
index 0000000..023f5ba
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_guc_fw.h
@@ -0,0 +1,33 @@
+/*
+ * Copyright © 2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#ifndef _INTEL_GUC_FW_H_
+#define _INTEL_GUC_FW_H_
+
+struct intel_guc;
+
+int intel_guc_fw_select(struct intel_guc *guc);
+int intel_guc_fw_upload(struct intel_guc *guc);
+
+#endif
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
deleted file mode 100644
index 8508b94..0000000
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ /dev/null
@@ -1,298 +0,0 @@
-/*
- * Copyright © 2014 Intel Corporation
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the "Software"),
- * to deal in the Software without restriction, including without limitation
- * the rights to use, copy, modify, merge, publish, distribute, sublicense,
- * and/or sell copies of the Software, and to permit persons to whom the
- * Software is furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice (including the next
- * paragraph) shall be included in all copies or substantial portions of the
- * Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
- * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
- * IN THE SOFTWARE.
- *
- * Authors:
- *    Vinit Azad <vinit.azad@intel.com>
- *    Ben Widawsky <ben@bwidawsk.net>
- *    Dave Gordon <david.s.gordon@intel.com>
- *    Alex Dai <yu.dai@intel.com>
- */
-#include "i915_drv.h"
-#include "intel_uc.h"
-
-#define SKL_FW_MAJOR 6
-#define SKL_FW_MINOR 1
-
-#define BXT_FW_MAJOR 8
-#define BXT_FW_MINOR 7
-
-#define KBL_FW_MAJOR 9
-#define KBL_FW_MINOR 14
-
-#define GLK_FW_MAJOR 10
-#define GLK_FW_MINOR 56
-
-#define GUC_FW_PATH(platform, major, minor) \
-       "i915/" __stringify(platform) "_guc_ver" __stringify(major) "_" __stringify(minor) ".bin"
-
-#define I915_SKL_GUC_UCODE GUC_FW_PATH(skl, SKL_FW_MAJOR, SKL_FW_MINOR)
-MODULE_FIRMWARE(I915_SKL_GUC_UCODE);
-
-#define I915_BXT_GUC_UCODE GUC_FW_PATH(bxt, BXT_FW_MAJOR, BXT_FW_MINOR)
-MODULE_FIRMWARE(I915_BXT_GUC_UCODE);
-
-#define I915_KBL_GUC_UCODE GUC_FW_PATH(kbl, KBL_FW_MAJOR, KBL_FW_MINOR)
-MODULE_FIRMWARE(I915_KBL_GUC_UCODE);
-
-#define I915_GLK_GUC_UCODE GUC_FW_PATH(glk, GLK_FW_MAJOR, GLK_FW_MINOR)
-
-
-/*
- * Read the GuC status register (GUC_STATUS) and store it in the
- * specified location; then return a boolean indicating whether
- * the value matches either of two values representing completion
- * of the GuC boot process.
- *
- * This is used for polling the GuC status in a wait_for()
- * loop below.
- */
-static inline bool guc_ucode_response(struct drm_i915_private *dev_priv,
-				      u32 *status)
-{
-	u32 val = I915_READ(GUC_STATUS);
-	u32 uk_val = val & GS_UKERNEL_MASK;
-	*status = val;
-	return (uk_val == GS_UKERNEL_READY ||
-		((val & GS_MIA_CORE_STATE) && uk_val == GS_UKERNEL_LAPIC_DONE));
-}
-
-/*
- * Transfer the firmware image to RAM for execution by the microcontroller.
- *
- * 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.
- *
- * 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.
- */
-static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
-			      struct i915_vma *vma)
-{
-	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
-	unsigned long offset;
-	struct sg_table *sg = vma->pages;
-	u32 status, rsa[UOS_RSA_SCRATCH_MAX_COUNT];
-	int i, ret = 0;
-
-	/* where RSA signature starts */
-	offset = guc_fw->rsa_offset;
-
-	/* Copy RSA signature from the fw image to HW for verification */
-	sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, sizeof(rsa), offset);
-	for (i = 0; i < UOS_RSA_SCRATCH_MAX_COUNT; i++)
-		I915_WRITE(UOS_RSA_SCRATCH(i), rsa[i]);
-
-	/* The header plus uCode will be copied to WOPCM via DMA, excluding any
-	 * other components */
-	I915_WRITE(DMA_COPY_SIZE, guc_fw->header_size + guc_fw->ucode_size);
-
-	/* Set the source address for the new blob */
-	offset = guc_ggtt_offset(vma) + guc_fw->header_offset;
-	I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset));
-	I915_WRITE(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.
-	 */
-	I915_WRITE(DMA_ADDR_1_LOW, 0x2000);
-	I915_WRITE(DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
-
-	/* Finally start the DMA */
-	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA));
-
-	/*
-	 * Wait for the DMA to complete & the GuC to start up.
-	 * NB: Docs recommend not using the interrupt for completion.
-	 * Measurements indicate this should take no more than 20ms, so a
-	 * timeout here indicates that the GuC has failed and is unusable.
-	 * (Higher levels of the driver will attempt to fall back to
-	 * execlist mode if this happens.)
-	 */
-	ret = wait_for(guc_ucode_response(dev_priv, &status), 100);
-
-	DRM_DEBUG_DRIVER("DMA status 0x%x, GuC status 0x%x\n",
-			I915_READ(DMA_CTRL), status);
-
-	if ((status & GS_BOOTROM_MASK) == GS_BOOTROM_RSA_FAILED) {
-		DRM_ERROR("GuC firmware signature verification failed\n");
-		ret = -ENOEXEC;
-	}
-
-	DRM_DEBUG_DRIVER("returning %d\n", ret);
-
-	return ret;
-}
-
-/*
- * Load the GuC firmware blob into the MinuteIA.
- */
-static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
-{
-	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
-	struct i915_vma *vma;
-	int ret;
-
-	ret = i915_gem_object_set_to_gtt_domain(guc_fw->obj, false);
-	if (ret) {
-		DRM_DEBUG_DRIVER("set-domain failed %d\n", ret);
-		return ret;
-	}
-
-	vma = i915_gem_object_ggtt_pin(guc_fw->obj, NULL, 0, 0,
-				       PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
-	if (IS_ERR(vma)) {
-		DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
-		return PTR_ERR(vma);
-	}
-
-	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
-
-	/* Enable MIA caching. GuC clock gating is disabled. */
-	I915_WRITE(GUC_SHIM_CONTROL, GUC_SHIM_CONTROL_VALUE);
-
-	/* WaDisableMinuteIaClockGating:bxt */
-	if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_A1)) {
-		I915_WRITE(GUC_SHIM_CONTROL, (I915_READ(GUC_SHIM_CONTROL) &
-					      ~GUC_ENABLE_MIA_CLOCK_GATING));
-	}
-
-	/* WaC6DisallowByGfxPause:bxt */
-	if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_B0))
-		I915_WRITE(GEN6_GFXPAUSE, 0x30FFF);
-
-	if (IS_GEN9_LP(dev_priv))
-		I915_WRITE(GEN9LP_GT_PM_CONFIG, GT_DOORBELL_ENABLE);
-	else
-		I915_WRITE(GEN9_GT_PM_CONFIG, GT_DOORBELL_ENABLE);
-
-	if (IS_GEN9(dev_priv)) {
-		/* DOP Clock Gating Enable for GuC clocks */
-		I915_WRITE(GEN7_MISCCPCTL, (GEN8_DOP_CLOCK_GATE_GUC_ENABLE |
-					    I915_READ(GEN7_MISCCPCTL)));
-
-		/* allows for 5us (in 10ns units) before GT can go to RC6 */
-		I915_WRITE(GUC_ARAT_C6DIS, 0x1FF);
-	}
-
-	ret = guc_ucode_xfer_dma(dev_priv, vma);
-
-	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
-
-	/*
-	 * We keep the object pages for reuse during resume. But we can unpin it
-	 * now that DMA has completed, so it doesn't continue to take up space.
-	 */
-	i915_vma_unpin(vma);
-
-	return ret;
-}
-
-/**
- * intel_guc_init_hw() - finish preparing the GuC for activity
- * @guc: intel_guc structure
- *
- * Called during driver loading and also after a GPU reset.
- *
- * The main action required here it to load the GuC uCode into the device.
- * The firmware image should have already been fetched into memory by the
- * earlier call to intel_guc_init(), so here we need only check that
- * worked, and then transfer the image to the h/w.
- *
- * Return:	non-zero code on error
- */
-int intel_guc_init_hw(struct intel_guc *guc)
-{
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	const char *fw_path = guc->fw.path;
-	int ret;
-
-	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
-		fw_path,
-		intel_uc_fw_status_repr(guc->fw.fetch_status),
-		intel_uc_fw_status_repr(guc->fw.load_status));
-
-	if (guc->fw.fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
-		return -EIO;
-
-	guc->fw.load_status = INTEL_UC_FIRMWARE_PENDING;
-
-	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
-		intel_uc_fw_status_repr(guc->fw.fetch_status),
-		intel_uc_fw_status_repr(guc->fw.load_status));
-
-	ret = guc_ucode_xfer(dev_priv);
-
-	if (ret)
-		return -EAGAIN;
-
-	guc->fw.load_status = INTEL_UC_FIRMWARE_SUCCESS;
-
-	DRM_INFO("GuC %s (firmware %s [version %u.%u])\n",
-		 i915_modparams.enable_guc_submission ? "submission enabled" :
-							"loaded",
-		 guc->fw.path,
-		 guc->fw.major_ver_found, guc->fw.minor_ver_found);
-
-	return 0;
-}
-
-/**
- * intel_guc_select_fw() - selects GuC firmware for loading
- * @guc:	intel_guc struct
- *
- * Return: zero when we know firmware, non-zero in other case
- */
-int intel_guc_select_fw(struct intel_guc *guc)
-{
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-
-	intel_uc_fw_init(&guc->fw, INTEL_UC_FW_TYPE_GUC);
-
-	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_SKYLAKE(dev_priv)) {
-		guc->fw.path = I915_SKL_GUC_UCODE;
-		guc->fw.major_ver_wanted = SKL_FW_MAJOR;
-		guc->fw.minor_ver_wanted = SKL_FW_MINOR;
-	} else if (IS_BROXTON(dev_priv)) {
-		guc->fw.path = I915_BXT_GUC_UCODE;
-		guc->fw.major_ver_wanted = BXT_FW_MAJOR;
-		guc->fw.minor_ver_wanted = BXT_FW_MINOR;
-	} else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) {
-		guc->fw.path = I915_KBL_GUC_UCODE;
-		guc->fw.major_ver_wanted = KBL_FW_MAJOR;
-		guc->fw.minor_ver_wanted = KBL_FW_MINOR;
-	} else if (IS_GEMINILAKE(dev_priv)) {
-		guc->fw.path = I915_GLK_GUC_UCODE;
-		guc->fw.major_ver_wanted = GLK_FW_MAJOR;
-		guc->fw.minor_ver_wanted = GLK_FW_MINOR;
-	} else {
-		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
-		return -ENOENT;
-	}
-
-	return 0;
-}
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 53fdd9a..048f5c4 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -68,7 +68,7 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 		if (HAS_HUC_UCODE(dev_priv))
 			intel_huc_select_fw(&dev_priv->huc);
 
-		if (intel_guc_select_fw(&dev_priv->guc))
+		if (intel_guc_fw_select(&dev_priv->guc))
 			i915_modparams.enable_guc_loading = 0;
 	}
 
@@ -196,7 +196,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 
 		intel_huc_init_hw(&dev_priv->huc);
 		intel_guc_init_params(guc);
-		ret = intel_guc_init_hw(&dev_priv->guc);
+		ret = intel_guc_fw_upload(guc);
 		if (ret == 0 || ret != -EAGAIN)
 			break;
 
-- 
2.7.4

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

[P v4 05/11] drm/i915/guc: Reorder functions in intel_guc_fw.c

[Michal Wajdeczko]

Functions should be defined in their use order.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_guc_fw.c | 81 +++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
index 61d6369..71dacc3 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/intel_guc_fw.c
@@ -56,6 +56,47 @@ MODULE_FIRMWARE(I915_KBL_GUC_UCODE);
 
 #define I915_GLK_GUC_UCODE GUC_FW_PATH(glk, GLK_FW_MAJOR, GLK_FW_MINOR)
 
+/**
+ * intel_guc_fw_select() - selects GuC firmware for uploading
+ *
+ * @guc:	intel_guc struct
+ *
+ * Return: zero when we know firmware, non-zero in other case
+ */
+int intel_guc_fw_select(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+	intel_uc_fw_init(&guc->fw, INTEL_UC_FW_TYPE_GUC);
+
+	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_SKYLAKE(dev_priv)) {
+		guc->fw.path = I915_SKL_GUC_UCODE;
+		guc->fw.major_ver_wanted = SKL_FW_MAJOR;
+		guc->fw.minor_ver_wanted = SKL_FW_MINOR;
+	} else if (IS_BROXTON(dev_priv)) {
+		guc->fw.path = I915_BXT_GUC_UCODE;
+		guc->fw.major_ver_wanted = BXT_FW_MAJOR;
+		guc->fw.minor_ver_wanted = BXT_FW_MINOR;
+	} else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) {
+		guc->fw.path = I915_KBL_GUC_UCODE;
+		guc->fw.major_ver_wanted = KBL_FW_MAJOR;
+		guc->fw.minor_ver_wanted = KBL_FW_MINOR;
+	} else if (IS_GEMINILAKE(dev_priv)) {
+		guc->fw.path = I915_GLK_GUC_UCODE;
+		guc->fw.major_ver_wanted = GLK_FW_MAJOR;
+		guc->fw.minor_ver_wanted = GLK_FW_MINOR;
+	} else {
+		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
+		return -ENOENT;
+	}
+
+	return 0;
+}
+
 /*
  * Read the GuC status register (GUC_STATUS) and store it in the
  * specified location; then return a boolean indicating whether
@@ -256,43 +297,3 @@ int intel_guc_fw_upload(struct intel_guc *guc)
 
 	return 0;
 }
-
-/**
- * intel_guc_fw_select() - selects GuC firmware for loading
- * @guc:	intel_guc struct
- *
- * Return: zero when we know firmware, non-zero in other case
- */
-int intel_guc_fw_select(struct intel_guc *guc)
-{
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-
-	intel_uc_fw_init(&guc->fw, INTEL_UC_FW_TYPE_GUC);
-
-	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_SKYLAKE(dev_priv)) {
-		guc->fw.path = I915_SKL_GUC_UCODE;
-		guc->fw.major_ver_wanted = SKL_FW_MAJOR;
-		guc->fw.minor_ver_wanted = SKL_FW_MINOR;
-	} else if (IS_BROXTON(dev_priv)) {
-		guc->fw.path = I915_BXT_GUC_UCODE;
-		guc->fw.major_ver_wanted = BXT_FW_MAJOR;
-		guc->fw.minor_ver_wanted = BXT_FW_MINOR;
-	} else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) {
-		guc->fw.path = I915_KBL_GUC_UCODE;
-		guc->fw.major_ver_wanted = KBL_FW_MAJOR;
-		guc->fw.minor_ver_wanted = KBL_FW_MINOR;
-	} else if (IS_GEMINILAKE(dev_priv)) {
-		guc->fw.path = I915_GLK_GUC_UCODE;
-		guc->fw.major_ver_wanted = GLK_FW_MAJOR;
-		guc->fw.minor_ver_wanted = GLK_FW_MINOR;
-	} else {
-		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
-		return -ENOENT;
-	}
-
-	return 0;
-}
-- 
2.7.4

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

[P v4 06/11] drm/i915/guc: Move firmware size check out of generic code

[Michal Wajdeczko]

Checking GuC firmware size can be done in GuC specific code
right before DMA copy as it is unlikely to fail anyway.

v2: rebased

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> #1
---
 drivers/gpu/drm/i915/intel_guc_fw.c | 7 ++++++-
 drivers/gpu/drm/i915/intel_uc_fw.c  | 8 --------
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
index 71dacc3..020ce26 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/intel_guc_fw.c
@@ -133,6 +133,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
 	unsigned long offset;
 	struct sg_table *sg = vma->pages;
 	u32 status, rsa[UOS_RSA_SCRATCH_MAX_COUNT];
+	u32 size = guc_fw->header_size + guc_fw->ucode_size;
 	int i, ret = 0;
 
 	/* where RSA signature starts */
@@ -145,7 +146,11 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
 
 	/* The header plus uCode will be copied to WOPCM via DMA, excluding any
 	 * other components */
-	I915_WRITE(DMA_COPY_SIZE, guc_fw->header_size + guc_fw->ucode_size);
+	if (size > intel_guc_wopcm_size(dev_priv)) {
+		DRM_ERROR("Firmware is too large to fit in WOPCM\n");
+		return -EFBIG;
+	}
+	I915_WRITE(DMA_COPY_SIZE, size);
 
 	/* Set the source address for the new blob */
 	offset = guc_ggtt_offset(vma) + guc_fw->header_offset;
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
index 766b1cb..482115b 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/intel_uc_fw.c
@@ -108,14 +108,6 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 	 */
 	switch (uc_fw->type) {
 	case INTEL_UC_FW_TYPE_GUC:
-		/* Header and uCode will be loaded to WOPCM. Size of the two. */
-		size = uc_fw->header_size + uc_fw->ucode_size;
-
-		/* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */
-		if (size > intel_guc_wopcm_size(dev_priv)) {
-			DRM_ERROR("Firmware is too large to fit in WOPCM\n");
-			goto fail;
-		}
 		uc_fw->major_ver_found = css->guc.sw_version >> 16;
 		uc_fw->minor_ver_found = css->guc.sw_version & 0xFFFF;
 		break;
-- 
2.7.4

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

[P v4 07/11] drm/i915/guc: Pick better place for Guc final status message

[Michal Wajdeczko]

Guc status message printed right after firmware upload may be too
optimistic, as we may fail on subsequent steps. Move that message
to the end of intel_uc_init_hw where we know the status for sure.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_fw.c | 6 ------
 drivers/gpu/drm/i915/intel_uc.c     | 6 ++++++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
index 020ce26..51be318 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/intel_guc_fw.c
@@ -294,11 +294,5 @@ int intel_guc_fw_upload(struct intel_guc *guc)
 
 	guc->fw.load_status = INTEL_UC_FIRMWARE_SUCCESS;
 
-	DRM_INFO("GuC %s (firmware %s [version %u.%u])\n",
-		 i915_modparams.enable_guc_submission ? "submission enabled" :
-							"loaded",
-		 guc->fw.path,
-		 guc->fw.major_ver_found, guc->fw.minor_ver_found);
-
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 048f5c4..7dd7546 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -222,6 +222,12 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 			goto err_interrupts;
 	}
 
+	DRM_INFO("GuC %s (firmware %s [version %u.%u])\n",
+		 i915_modparams.enable_guc_submission ? "submission enabled" :
+							"loaded",
+		 guc->fw.path,
+		 guc->fw.major_ver_found, guc->fw.minor_ver_found);
+
 	return 0;
 
 	/*
-- 
2.7.4

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

[P v4 08/11] drm/i915/uc: Improve debug messages in firmware fetch

[Michal Wajdeczko]

Time to remove less important info and make messages clear
and consistent.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_uc_fw.c | 73 +++++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
index 482115b..b52d6b6 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/intel_uc_fw.c
@@ -45,26 +45,33 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 	size_t size;
 	int err;
 
+	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
+			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
+
 	if (!uc_fw->path)
 		return;
 
 	uc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
-
-	DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n",
+	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)
-		goto fail;
-	if (!fw)
+	if (err) {
+		DRM_NOTE("%s: Error while requesting firmware\n",
+			 intel_uc_fw_type_repr(uc_fw->type));
 		goto fail;
+	}
 
-	DRM_DEBUG_DRIVER("fetch uC fw from %s succeeded, fw %p\n",
-			 uc_fw->path, fw);
+	DRM_DEBUG_DRIVER("%s fw size %zu ptr %p\n",
+			 intel_uc_fw_type_repr(uc_fw->type), fw->size, fw);
 
 	/* Check the size of the blob before examining buffer contents */
 	if (fw->size < sizeof(struct uc_css_header)) {
-		DRM_NOTE("Firmware header is missing\n");
+		DRM_NOTE("%s: Unexpected firmware size (%zu, min %zu)\n",
+			 intel_uc_fw_type_repr(uc_fw->type),
+			 fw->size, sizeof(struct uc_css_header));
+		err = -ENODATA;
 		goto fail;
 	}
 
@@ -77,7 +84,9 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 			     sizeof(u32);
 
 	if (uc_fw->header_size != sizeof(struct uc_css_header)) {
-		DRM_NOTE("CSS header definition mismatch\n");
+		DRM_NOTE("%s: Mismatched firmware header definition\n",
+			 intel_uc_fw_type_repr(uc_fw->type));
+		err = -ENOEXEC;
 		goto fail;
 	}
 
@@ -87,7 +96,9 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 
 	/* now RSA */
 	if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
-		DRM_NOTE("RSA key size is bad\n");
+		DRM_NOTE("%s: Mismatched firmware RSA key size (%u)\n",
+			 intel_uc_fw_type_repr(uc_fw->type), css->key_size_dw);
+		err = -ENOEXEC;
 		goto fail;
 	}
 	uc_fw->rsa_offset = uc_fw->ucode_offset + uc_fw->ucode_size;
@@ -96,7 +107,9 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 	/* At least, it should have header, uCode and RSA. Size of all three. */
 	size = uc_fw->header_size + uc_fw->ucode_size + uc_fw->rsa_size;
 	if (fw->size < size) {
-		DRM_NOTE("Missing firmware components\n");
+		DRM_NOTE("%s: Truncated firmware (%zu, expected %zu)\n",
+			 intel_uc_fw_type_repr(uc_fw->type), fw->size, size);
+		err = -ENOEXEC;
 		goto fail;
 	}
 
@@ -118,17 +131,21 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 		break;
 
 	default:
-		DRM_ERROR("Unknown firmware type %d\n", uc_fw->type);
-		err = -ENOEXEC;
-		goto fail;
+		MISSING_CASE(uc_fw->type);
+		break;
 	}
 
+	DRM_DEBUG_DRIVER("%s fw version %u.%u (wanted %u.%u)\n",
+			 intel_uc_fw_type_repr(uc_fw->type),
+			 uc_fw->major_ver_found, uc_fw->minor_ver_found,
+			 uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
+
 	if (uc_fw->major_ver_wanted == 0 && uc_fw->minor_ver_wanted == 0) {
-		DRM_NOTE("Skipping %s firmware version check\n",
+		DRM_NOTE("%s: Skipping firmware version check\n",
 			 intel_uc_fw_type_repr(uc_fw->type));
 	} else if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
 		   uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
-		DRM_NOTE("%s firmware version %d.%d, required %d.%d\n",
+		DRM_NOTE("%s: Wrong firmware version (%u.%u, required %u.%u)\n",
 			 intel_uc_fw_type_repr(uc_fw->type),
 			 uc_fw->major_ver_found, uc_fw->minor_ver_found,
 			 uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
@@ -136,10 +153,6 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 		goto fail;
 	}
 
-	DRM_DEBUG_DRIVER("firmware version %d.%d OK (minimum %d.%d)\n",
-			 uc_fw->major_ver_found, uc_fw->minor_ver_found,
-			 uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
-
 	obj = i915_gem_object_create_from_data(dev_priv, fw->data, fw->size);
 	if (IS_ERR(obj)) {
 		err = PTR_ERR(obj);
@@ -148,22 +161,24 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 
 	uc_fw->obj = obj;
 	uc_fw->size = fw->size;
-
-	DRM_DEBUG_DRIVER("uC fw fetch status SUCCESS, obj %p\n",
-			 uc_fw->obj);
+	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));
 
 	release_firmware(fw);
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
 	return;
 
 fail:
-	DRM_WARN("Failed to fetch valid uC firmware from %s (error %d)\n",
-		 uc_fw->path, err);
-	DRM_DEBUG_DRIVER("uC fw fetch status FAIL; err %d, fw %p, obj %p\n",
-			 err, fw, uc_fw->obj);
+	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));
+
+	DRM_WARN("%s: Failed to fetch firmware %s (error %d)\n",
+		 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
 
 	release_firmware(fw);		/* OK even if fw is NULL */
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_FAIL;
 }
 
 /**
-- 
2.7.4

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

[P v4 09/11] drm/i915/uc: Unify firmware loading

[Michal Wajdeczko]

Firmware loading for GuC and Huc are similar. Move common code
into generic function for maximum reuse.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_fw.c | 52 +++---------------------
 drivers/gpu/drm/i915/intel_huc.c    | 53 +++---------------------
 drivers/gpu/drm/i915/intel_uc_fw.c  | 81 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc_fw.h  |  4 ++
 4 files changed, 95 insertions(+), 95 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
index 51be318..4629ff3 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/intel_guc_fw.c
@@ -193,24 +193,13 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
 /*
  * Load the GuC firmware blob into the MinuteIA.
  */
-static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
+static int guc_ucode_xfer(struct intel_uc_fw *guc_fw, struct i915_vma *vma)
 {
-	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
-	struct i915_vma *vma;
+	struct intel_guc *guc = container_of(guc_fw, struct intel_guc, fw);
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	int ret;
 
-	ret = i915_gem_object_set_to_gtt_domain(guc_fw->obj, false);
-	if (ret) {
-		DRM_DEBUG_DRIVER("set-domain failed %d\n", ret);
-		return ret;
-	}
-
-	vma = i915_gem_object_ggtt_pin(guc_fw->obj, NULL, 0, 0,
-				       PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
-	if (IS_ERR(vma)) {
-		DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
-		return PTR_ERR(vma);
-	}
+	GEM_BUG_ON(guc_fw->type != INTEL_UC_FW_TYPE_GUC);
 
 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
 
@@ -245,12 +234,6 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 
-	/*
-	 * We keep the object pages for reuse during resume. But we can unpin it
-	 * now that DMA has completed, so it doesn't continue to take up space.
-	 */
-	i915_vma_unpin(vma);
-
 	return ret;
 }
 
@@ -269,30 +252,5 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
  */
 int intel_guc_fw_upload(struct intel_guc *guc)
 {
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	const char *fw_path = guc->fw.path;
-	int ret;
-
-	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
-		fw_path,
-		intel_uc_fw_status_repr(guc->fw.fetch_status),
-		intel_uc_fw_status_repr(guc->fw.load_status));
-
-	if (guc->fw.fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
-		return -EIO;
-
-	guc->fw.load_status = INTEL_UC_FIRMWARE_PENDING;
-
-	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
-		intel_uc_fw_status_repr(guc->fw.fetch_status),
-		intel_uc_fw_status_repr(guc->fw.load_status));
-
-	ret = guc_ucode_xfer(dev_priv);
-
-	if (ret)
-		return -EAGAIN;
-
-	guc->fw.load_status = INTEL_UC_FIRMWARE_SUCCESS;
-
-	return 0;
+	return intel_uc_fw_upload(&guc->fw, guc_ucode_xfer);
 }
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 4b4cf56..8be3bfe 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -85,26 +85,15 @@ MODULE_FIRMWARE(I915_KBL_HUC_UCODE);
  *
  * Return: 0 on success, non-zero on failure
  */
-static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
+static int huc_ucode_xfer(struct intel_uc_fw *huc_fw, struct i915_vma *vma)
 {
-	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
-	struct i915_vma *vma;
+	struct intel_huc *huc = container_of(huc_fw, struct intel_huc, fw);
+	struct drm_i915_private *dev_priv = huc_to_i915(huc);
 	unsigned long offset = 0;
 	u32 size;
 	int ret;
 
-	ret = i915_gem_object_set_to_gtt_domain(huc_fw->obj, false);
-	if (ret) {
-		DRM_DEBUG_DRIVER("set-domain failed %d\n", ret);
-		return ret;
-	}
-
-	vma = i915_gem_object_ggtt_pin(huc_fw->obj, NULL, 0, 0,
-				PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
-	if (IS_ERR(vma)) {
-		DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
-		return PTR_ERR(vma);
-	}
+	GEM_BUG_ON(huc_fw->type != INTEL_UC_FW_TYPE_HUC);
 
 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
 
@@ -135,12 +124,6 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 
-	/*
-	 * We keep the object pages for reuse during resume. But we can unpin it
-	 * now that DMA has completed, so it doesn't continue to take up space.
-	 */
-	i915_vma_unpin(vma);
-
 	return ret;
 }
 
@@ -194,33 +177,7 @@ void intel_huc_select_fw(struct intel_huc *huc)
  */
 void intel_huc_init_hw(struct intel_huc *huc)
 {
-	struct drm_i915_private *dev_priv = huc_to_i915(huc);
-	int err;
-
-	DRM_DEBUG_DRIVER("%s fw status: fetch %s, load %s\n",
-		huc->fw.path,
-		intel_uc_fw_status_repr(huc->fw.fetch_status),
-		intel_uc_fw_status_repr(huc->fw.load_status));
-
-	if (huc->fw.fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
-		return;
-
-	huc->fw.load_status = INTEL_UC_FIRMWARE_PENDING;
-
-	err = huc_ucode_xfer(dev_priv);
-
-	huc->fw.load_status = err ?
-		INTEL_UC_FIRMWARE_FAIL : INTEL_UC_FIRMWARE_SUCCESS;
-
-	DRM_DEBUG_DRIVER("%s fw status: fetch %s, load %s\n",
-		huc->fw.path,
-		intel_uc_fw_status_repr(huc->fw.fetch_status),
-		intel_uc_fw_status_repr(huc->fw.load_status));
-
-	if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
-		DRM_ERROR("Failed to complete HuC uCode load with ret %d\n", err);
-
-	return;
+	intel_uc_fw_upload(&huc->fw, huc_ucode_xfer);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
index b52d6b6..16dc970 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/intel_uc_fw.c
@@ -182,6 +182,87 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 }
 
 /**
+ * intel_uc_fw_upload - load uC firmware using custom loader
+ *
+ * @uc_fw: uC firmware
+ * @loader: custom uC firmware loader function
+ *
+ * Loads uC firmware using custom loader and updates internal flags.
+ */
+int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
+		       int (*loader)(struct intel_uc_fw *uc_fw,
+				     struct i915_vma *vma))
+{
+	struct i915_vma *vma;
+	int err;
+
+	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 -EIO;
+
+	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));
+
+	/* Pin object with firmware */
+	err = i915_gem_object_set_to_gtt_domain(uc_fw->obj, false);
+	if (err) {
+		DRM_DEBUG_DRIVER("%s fw set-domain failed %d\n",
+				 intel_uc_fw_type_repr(uc_fw->type), err);
+		goto fail;
+	}
+
+	vma = i915_gem_object_ggtt_pin(uc_fw->obj, NULL, 0, 0,
+				       PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
+	if (IS_ERR(vma)) {
+		err = PTR_ERR(vma);
+		DRM_DEBUG_DRIVER("%s fw pin failed %d\n",
+				 intel_uc_fw_type_repr(uc_fw->type),
+				 err);
+		goto fail;
+	}
+
+	/* Call custom loader */
+	err = loader(uc_fw, vma);
+	uc_fw->load_status = err ?
+			     INTEL_UC_FIRMWARE_FAIL :
+			     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));
+
+	/*
+	 * We keep the object pages for reuse during resume. But we can unpin it
+	 * now that DMA has completed, so it doesn't continue to take up space.
+	 */
+	i915_vma_unpin(vma);
+
+	if (err)
+		goto fail;
+
+	DRM_INFO("%s: Loaded firmware %s (version %u.%u)\n",
+		 intel_uc_fw_type_repr(uc_fw->type),
+		 uc_fw->path,
+		 uc_fw->major_ver_found, uc_fw->minor_ver_found);
+
+	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));
+
+	DRM_ERROR("%s: Failed to load firmware %s (error %d)\n",
+		  intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
+
+	return err;
+}
+
+/**
  * intel_uc_fw_fini - cleanup uC firmware
  *
  * @uc_fw: uC firmware
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h
index 5c01849..c3f1af9 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/intel_uc_fw.h
@@ -26,6 +26,7 @@
 #define _INTEL_UC_FW_H_
 
 struct drm_i915_private;
+struct i915_vma;
 
 enum intel_uc_fw_status {
 	INTEL_UC_FIRMWARE_FAIL = -1,
@@ -107,6 +108,9 @@ void intel_uc_fw_init(struct intel_uc_fw *uc_fw, enum intel_uc_fw_type type)
 
 void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 		       struct intel_uc_fw *uc_fw);
+int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
+		       int (*loader)(struct intel_uc_fw *uc_fw,
+				     struct i915_vma *vma));
 void intel_uc_fw_fini(struct intel_uc_fw *uc_fw);
 
 #endif
-- 
2.7.4

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

[P v4 10/11] drm/i915/huc: Move fw select function

[Michal Wajdeczko]

No functional change. Just place the function closer to related
definitions.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_huc.c | 72 ++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 8be3bfe..c8a48cb 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -78,6 +78,42 @@ MODULE_FIRMWARE(I915_KBL_HUC_UCODE);
 	GLK_HUC_FW_MINOR, GLK_BLD_NUM)
 
 /**
+ * intel_huc_select_fw() - selects HuC firmware for loading
+ * @huc:	intel_huc struct
+ */
+void intel_huc_select_fw(struct intel_huc *huc)
+{
+	struct drm_i915_private *dev_priv = huc_to_i915(huc);
+
+	intel_uc_fw_init(&huc->fw, INTEL_UC_FW_TYPE_HUC);
+
+	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 {
+		DRM_ERROR("No HuC firmware known for platform with HuC!\n");
+		return;
+	}
+}
+
+/**
  * huc_ucode_xfer() - DMA's the firmware
  * @dev_priv: the drm_i915_private device
  *
@@ -128,42 +164,6 @@ static int huc_ucode_xfer(struct intel_uc_fw *huc_fw, struct i915_vma *vma)
 }
 
 /**
- * intel_huc_select_fw() - selects HuC firmware for loading
- * @huc:	intel_huc struct
- */
-void intel_huc_select_fw(struct intel_huc *huc)
-{
-	struct drm_i915_private *dev_priv = huc_to_i915(huc);
-
-	intel_uc_fw_init(&huc->fw, INTEL_UC_FW_TYPE_HUC);
-
-	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 {
-		DRM_ERROR("No HuC firmware known for platform with HuC!\n");
-		return;
-	}
-}
-
-/**
  * intel_huc_init_hw() - load HuC uCode to device
  * @huc: intel_huc structure
  *
-- 
2.7.4

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

[P v4 11/11] HAX enable GuC submission for CI

[Michal Wajdeczko]

Also revert ("drm/i915/guc: Assert that we switch between
known ggtt->invalidate functions")

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++------
 drivers/gpu/drm/i915/i915_params.h  | 4 ++--
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 4c60578..a38c6ba 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3570,17 +3570,13 @@ int i915_ggtt_enable_hw(struct drm_i915_private *dev_priv)
 
 void i915_ggtt_enable_guc(struct drm_i915_private *i915)
 {
-	GEM_BUG_ON(i915->ggtt.invalidate != gen6_ggtt_invalidate);
-
 	i915->ggtt.invalidate = guc_ggtt_invalidate;
 }
 
 void i915_ggtt_disable_guc(struct drm_i915_private *i915)
 {
-	/* We should only be called after i915_ggtt_enable_guc() */
-	GEM_BUG_ON(i915->ggtt.invalidate != guc_ggtt_invalidate);
-
-	i915->ggtt.invalidate = gen6_ggtt_invalidate;
+	if (i915->ggtt.invalidate == guc_ggtt_invalidate)
+		i915->ggtt.invalidate = gen6_ggtt_invalidate;
 }
 
 void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index c729226..97c0611 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -44,8 +44,8 @@
 	param(int, disable_power_well, -1) \
 	param(int, enable_ips, 1) \
 	param(int, invert_brightness, 0) \
-	param(int, enable_guc_loading, 0) \
-	param(int, enable_guc_submission, 0) \
+	param(int, enable_guc_loading, 2) \
+	param(int, enable_guc_submission, 2) \
 	param(int, guc_log_level, -1) \
 	param(char *, guc_firmware_path, NULL) \
 	param(char *, huc_firmware_path, NULL) \
-- 
2.7.4

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

Re: [P v4 00/11] drm/i915/uc: Firmware code reorg

[Michal Wajdeczko]

Ooops, sorry for bad prefix: s/P/PATCH

On Tue, 10 Oct 2017 16:51:24 +0200, Michal Wajdeczko  
<michal.wajdeczko@intel.com> wrote:

>
> Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>
>
> Michal Wajdeczko (11):
>   drm/i915: Move intel_guc_wopcm_size to intel_guc.c
>   drm/i915/guc: Move GuC boot param initialization out of xfer
>   drm/i915/guc: Move doc near related definitions
>   drm/i915/guc: Rename intel_guc_loader.c to intel_guc_fw.c
>   drm/i915/guc: Reorder functions in intel_guc_fw.c
>   drm/i915/guc: Move firmware size check out of generic code
>   drm/i915/guc: Pick better place for Guc final status message
>   drm/i915/uc: Improve debug messages in firmware fetch
>   drm/i915/uc: Unify firmware loading
>   drm/i915/huc: Move fw select function
>   HAX enable GuC submission for CI
>
>  drivers/gpu/drm/i915/Makefile           |   2 +-
>  drivers/gpu/drm/i915/i915_gem_gtt.c     |   8 +-
>  drivers/gpu/drm/i915/i915_params.h      |   4 +-
>  drivers/gpu/drm/i915/intel_guc.c        |  99 ++++++++
>  drivers/gpu/drm/i915/intel_guc.h        |  16 +-
>  drivers/gpu/drm/i915/intel_guc_fw.c     | 256 ++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_guc_fw.h     |  33 +++
>  drivers/gpu/drm/i915/intel_guc_loader.c | 417  
> --------------------------------
>  drivers/gpu/drm/i915/intel_huc.c        | 125 ++++------
>  drivers/gpu/drm/i915/intel_uc.c         |  11 +-
>  drivers/gpu/drm/i915/intel_uc_fw.c      | 162 ++++++++++---
>  drivers/gpu/drm/i915/intel_uc_fw.h      |   9 +
>  12 files changed, 590 insertions(+), 552 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_guc_fw.c
>  create mode 100644 drivers/gpu/drm/i915/intel_guc_fw.h
>  delete mode 100644 drivers/gpu/drm/i915/intel_guc_loader.c
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915/uc: Firmware code reorg

[Patchwork]

== Series Details ==

Series: drm/i915/uc: Firmware code reorg
URL   : https://patchwork.freedesktop.org/series/31666/
State : failure

== Summary ==

Series 31666v1 drm/i915/uc: Firmware code reorg
https://patchwork.freedesktop.org/api/1.0/series/31666/revisions/1/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                pass       -> SKIP       (fi-glk-1)
Test drv_hangman:
        Subgroup error-state-basic:
                pass       -> SKIP       (fi-glk-1)
Test gem_busy:
        Subgroup basic-busy-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-hang-default:
                pass       -> SKIP       (fi-glk-1)
Test gem_close_race:
        Subgroup basic-process:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-threads:
                pass       -> SKIP       (fi-glk-1)
Test gem_cpu_reloc:
        Subgroup basic:
                pass       -> SKIP       (fi-glk-1)
Test gem_cs_tlb:
        Subgroup basic-default:
                pass       -> SKIP       (fi-glk-1)
Test gem_ctx_create:
        Subgroup basic:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-files:
                pass       -> SKIP       (fi-glk-1)
Test gem_ctx_exec:
        Subgroup basic:
                pass       -> SKIP       (fi-glk-1)
Test gem_ctx_switch:
        Subgroup basic-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-default-heavy:
                pass       -> SKIP       (fi-glk-1)
Test gem_exec_basic:
        Subgroup basic-blt:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-bsd:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-render:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-vebox:
                pass       -> SKIP       (fi-glk-1)
        Subgroup gtt-blt:
                pass       -> SKIP       (fi-glk-1)
        Subgroup gtt-bsd:
                pass       -> SKIP       (fi-glk-1)
        Subgroup gtt-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup gtt-render:
                pass       -> SKIP       (fi-glk-1)
        Subgroup gtt-vebox:
                pass       -> SKIP       (fi-glk-1)
        Subgroup readonly-blt:
                pass       -> SKIP       (fi-glk-1)
        Subgroup readonly-bsd:
                pass       -> SKIP       (fi-glk-1)
        Subgroup readonly-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup readonly-render:
                pass       -> SKIP       (fi-glk-1)
        Subgroup readonly-vebox:
                pass       -> SKIP       (fi-glk-1)
Test gem_exec_create:
        Subgroup basic:
                pass       -> SKIP       (fi-glk-1)
Test gem_exec_fence:
        Subgroup basic-busy-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-wait-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-await-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup await-hang-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup nb-await-default:
                pass       -> SKIP       (fi-glk-1)
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-batch-kernel-default-wb:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-uc-pro-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-uc-prw-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-uc-ro-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-uc-rw-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-uc-set-default:
                pass       -> SKIP       (fi-glk-1)
        Subgroup basic-wb-pro-default:
                pass       -> SKIP       (fi-glk-1)
WARNING: Long output truncated

cc58e6d2bc38542af8f8031d4b79f4069bba6afc drm-tip: 2017y-10m-10d-15h-40m-22s UTC integration manifest
0c88fac6ee50 HAX enable GuC submission for CI
e0fbddca8865 drm/i915/huc: Move fw select function
60f6548efd03 drm/i915/uc: Unify firmware loading
95591d4be13e drm/i915/uc: Improve debug messages in firmware fetch
cfc9af96a481 drm/i915/guc: Pick better place for Guc final status message
4c019d8c6bf2 drm/i915/guc: Move firmware size check out of generic code
f36e7a27bed8 drm/i915/guc: Reorder functions in intel_guc_fw.c
37f173634576 drm/i915/guc: Rename intel_guc_loader.c to intel_guc_fw.c
235d82ecf4f4 drm/i915/guc: Move doc near related definitions
78302271ab2c drm/i915/guc: Move GuC boot param initialization out of xfer
4111101c66e2 drm/i915: Move intel_guc_wopcm_size to intel_guc.c

== Logs ==

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

Re: [P v4 02/11] drm/i915/guc: Move GuC boot param initialization out of xfer

[Daniele Ceraolo Spurio]

On 10/10/17 07:51, Michal Wajdeczko wrote:
> We want to keep ucode xfer functions separate from other
> initialization. Once separated, add explicit forcewake.
> 
> Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc.c        | 88 +++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_guc.h        |  1 +
>   drivers/gpu/drm/i915/intel_guc_loader.c | 85 -------------------------------
>   drivers/gpu/drm/i915/intel_uc.c         |  1 +
>   4 files changed, 90 insertions(+), 85 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 90c3dd8..d75515c 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -67,6 +67,94 @@ void intel_guc_init_early(struct intel_guc *guc)
>   	guc->notify = gen8_guc_raise_irq;
>   }
>   
> +static u32 get_gttype(struct drm_i915_private *dev_priv)
> +{
> +	/* XXX: GT type based on PCI device ID? field seems unused by fw */
> +	return 0;
> +}
> +
> +static u32 get_core_family(struct drm_i915_private *dev_priv)
> +{
> +	u32 gen = INTEL_GEN(dev_priv);
> +
> +	switch (gen) {
> +	case 9:
> +		return GUC_CORE_FAMILY_GEN9;
> +
> +	default:
> +		MISSING_CASE(gen);
> +		return GUC_CORE_FAMILY_UNKNOWN;
> +	}
> +}
> +
> +/*
> + * Initialise the GuC parameter block before starting the firmware
> + * transfer. These parameters are read by the firmware on startup
> + * and cannot be changed thereafter.
> + */
> +void intel_guc_init_params(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	u32 params[GUC_CTL_MAX_DWORDS];
> +	int i;
> +
> +	memset(&params, 0, sizeof(params));
> +
> +	params[GUC_CTL_DEVICE_INFO] |=
> +		(get_gttype(dev_priv) << GUC_CTL_GTTYPE_SHIFT) |
> +		(get_core_family(dev_priv) << GUC_CTL_COREFAMILY_SHIFT);
> +
> +	/*
> +	 * GuC ARAT increment is 10 ns. GuC default scheduler quantum is one
> +	 * second. This ARAR is calculated by:
> +	 * Scheduler-Quantum-in-ns / ARAT-increment-in-ns = 1000000000 / 10
> +	 */
> +	params[GUC_CTL_ARAT_HIGH] = 0;
> +	params[GUC_CTL_ARAT_LOW] = 100000000;
> +
> +	params[GUC_CTL_WA] |= GUC_CTL_WA_UK_BY_DRIVER;
> +
> +	params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
> +			GUC_CTL_VCS2_ENABLED;
> +
> +	params[GUC_CTL_LOG_PARAMS] = guc->log.flags;
> +
> +	if (i915_modparams.guc_log_level >= 0) {
> +		params[GUC_CTL_DEBUG] =
> +			i915_modparams.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
> +	} else
> +		params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
> +
> +	/* If GuC submission is enabled, set up additional parameters here */
> +	if (i915_modparams.enable_guc_submission) {
> +		u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
> +		u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool);
> +		u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
> +
> +		params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT;
> +		params[GUC_CTL_DEBUG] |= GUC_ADS_ENABLED;
> +
> +		pgs >>= PAGE_SHIFT;
> +		params[GUC_CTL_CTXINFO] = (pgs << GUC_CTL_BASE_ADDR_SHIFT) |
> +			(ctx_in_16 << GUC_CTL_CTXNUM_IN16_SHIFT);
> +
> +		params[GUC_CTL_FEATURE] |= GUC_CTL_KERNEL_SUBMISSIONS;
> +
> +		/* Unmask this bit to enable the GuC's internal scheduler */
> +		params[GUC_CTL_FEATURE] &= ~GUC_CTL_DISABLE_SCHEDULER;
> +	}
> +
> +	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);

I don't think we need this explicit forcewake if we use I915_WRITE(), 
because that should already wake the required wells. If you want to 
explicitly take forcewake then we can use I915_WRITE_FW(). Also, 
FORCEWAKE_BLITTER should be enough.
I'd also add a comment to say that the register are power context saved 
so it's ok to release forcewake here and take it again at xfer time.

> +
> +	I915_WRITE(SOFT_SCRATCH(0), 0);
> +
> +	for (i = 0; i < GUC_CTL_MAX_DWORDS; i++)
> +		I915_WRITE(SOFT_SCRATCH(1 + i), params[i]);
> +
> +	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> +
> +}
> +
>   int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
>   {
>   	WARN(1, "Unexpected send: action=%#x\n", *action);
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index aa9a7b5..8b44165 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -95,6 +95,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>   
>   void intel_guc_init_early(struct intel_guc *guc);
>   void intel_guc_init_send_regs(struct intel_guc *guc);
> +void intel_guc_init_params(struct intel_guc *guc);
>   int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
>   int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
>   int intel_guc_sample_forcewake(struct intel_guc *guc);
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 30b70f5..d9089bc 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -79,89 +79,6 @@ MODULE_FIRMWARE(I915_KBL_GUC_UCODE);
>   #define I915_GLK_GUC_UCODE GUC_FW_PATH(glk, GLK_FW_MAJOR, GLK_FW_MINOR)
>   
>   
> -static u32 get_gttype(struct drm_i915_private *dev_priv)
> -{
> -	/* XXX: GT type based on PCI device ID? field seems unused by fw */
> -	return 0;
> -}
> -
> -static u32 get_core_family(struct drm_i915_private *dev_priv)
> -{
> -	u32 gen = INTEL_GEN(dev_priv);
> -
> -	switch (gen) {
> -	case 9:
> -		return GUC_CORE_FAMILY_GEN9;
> -
> -	default:
> -		MISSING_CASE(gen);
> -		return GUC_CORE_FAMILY_UNKNOWN;
> -	}
> -}
> -
> -/*
> - * Initialise the GuC parameter block before starting the firmware
> - * transfer. These parameters are read by the firmware on startup
> - * and cannot be changed thereafter.
> - */
> -static void guc_params_init(struct drm_i915_private *dev_priv)
> -{
> -	struct intel_guc *guc = &dev_priv->guc;
> -	u32 params[GUC_CTL_MAX_DWORDS];
> -	int i;
> -
> -	memset(&params, 0, sizeof(params));
> -
> -	params[GUC_CTL_DEVICE_INFO] |=
> -		(get_gttype(dev_priv) << GUC_CTL_GTTYPE_SHIFT) |
> -		(get_core_family(dev_priv) << GUC_CTL_COREFAMILY_SHIFT);
> -
> -	/*
> -	 * GuC ARAT increment is 10 ns. GuC default scheduler quantum is one
> -	 * second. This ARAR is calculated by:
> -	 * Scheduler-Quantum-in-ns / ARAT-increment-in-ns = 1000000000 / 10
> -	 */
> -	params[GUC_CTL_ARAT_HIGH] = 0;
> -	params[GUC_CTL_ARAT_LOW] = 100000000;
> -
> -	params[GUC_CTL_WA] |= GUC_CTL_WA_UK_BY_DRIVER;
> -
> -	params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
> -			GUC_CTL_VCS2_ENABLED;
> -
> -	params[GUC_CTL_LOG_PARAMS] = guc->log.flags;
> -
> -	if (i915_modparams.guc_log_level >= 0) {
> -		params[GUC_CTL_DEBUG] =
> -			i915_modparams.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
> -	} else
> -		params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
> -
> -	/* If GuC submission is enabled, set up additional parameters here */
> -	if (i915_modparams.enable_guc_submission) {
> -		u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
> -		u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool);
> -		u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
> -
> -		params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT;
> -		params[GUC_CTL_DEBUG] |= GUC_ADS_ENABLED;
> -
> -		pgs >>= PAGE_SHIFT;
> -		params[GUC_CTL_CTXINFO] = (pgs << GUC_CTL_BASE_ADDR_SHIFT) |
> -			(ctx_in_16 << GUC_CTL_CTXNUM_IN16_SHIFT);
> -
> -		params[GUC_CTL_FEATURE] |= GUC_CTL_KERNEL_SUBMISSIONS;
> -
> -		/* Unmask this bit to enable the GuC's internal scheduler */
> -		params[GUC_CTL_FEATURE] &= ~GUC_CTL_DISABLE_SCHEDULER;
> -	}
> -
> -	I915_WRITE(SOFT_SCRATCH(0), 0);
> -
> -	for (i = 0; i < GUC_CTL_MAX_DWORDS; i++)
> -		I915_WRITE(SOFT_SCRATCH(1 + i), params[i]);
> -}
> -
>   /*
>    * Read the GuC status register (GUC_STATUS) and store it in the
>    * specified location; then return a boolean indicating whether
> @@ -301,8 +218,6 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>   		I915_WRITE(GUC_ARAT_C6DIS, 0x1FF);
>   	}
>   
> -	guc_params_init(dev_priv);
> -
>   	ret = guc_ucode_xfer_dma(dev_priv, vma);
>   
>   	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 7b938e8..53fdd9a 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -195,6 +195,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   			goto err_submission;
>   
>   		intel_huc_init_hw(&dev_priv->huc);
> +		intel_guc_init_params(guc);

Is the value of the registers lost/corrupted on guc reset? if not we 
could just move this outside the retry loop to avoid doing it more than 
once.

Daniele

>   		ret = intel_guc_init_hw(&dev_priv->guc);
>   		if (ret == 0 || ret != -EAGAIN)
>   			break;
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [P v4 01/11] drm/i915: Move intel_guc_wopcm_size to intel_guc.c

[Chris Wilson]

Quoting Michal Wajdeczko (2017-10-10 15:51:25)
> Function intel_guc_wopcm_size didn't fit well in the old place.
> With this move upcoming cleanup will be simpler.

I'll take that at face value, the code is clean motion.
> 
> Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@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: [P v4 02/11] drm/i915/guc: Move GuC boot param initialization out of xfer

[Chris Wilson]

Quoting Michal Wajdeczko (2017-10-10 15:51:26)
> We want to keep ucode xfer functions separate from other
> initialization. Once separated, add explicit forcewake.
> 
> Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc.c        | 88 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_guc.h        |  1 +
>  drivers/gpu/drm/i915/intel_guc_loader.c | 85 -------------------------------
>  drivers/gpu/drm/i915/intel_uc.c         |  1 +
>  4 files changed, 90 insertions(+), 85 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 90c3dd8..d75515c 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -67,6 +67,94 @@ void intel_guc_init_early(struct intel_guc *guc)
>         guc->notify = gen8_guc_raise_irq;
>  }
>  
> +static u32 get_gttype(struct drm_i915_private *dev_priv)
> +{

get_gt_type()

I read it as gtt_type and wondered about the mispelling.

> +       /* XXX: GT type based on PCI device ID? field seems unused by fw */
> +       return 0;
> +}
> +
> +static u32 get_core_family(struct drm_i915_private *dev_priv)
> +{
> +       u32 gen = INTEL_GEN(dev_priv);
> +
> +       switch (gen) {
> +       case 9:
> +               return GUC_CORE_FAMILY_GEN9;
> +
> +       default:
> +               MISSING_CASE(gen);
> +               return GUC_CORE_FAMILY_UNKNOWN;
> +       }

Ok.

> +}
> +
> +/*
> + * Initialise the GuC parameter block before starting the firmware
> + * transfer. These parameters are read by the firmware on startup
> + * and cannot be changed thereafter.
> + */
> +void intel_guc_init_params(struct intel_guc *guc)
> +{
> +       struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +       u32 params[GUC_CTL_MAX_DWORDS];
> +       int i;
> +
> +       memset(&params, 0, sizeof(params));

memset(params, 0, sizeof(params));

> +
> +       params[GUC_CTL_DEVICE_INFO] |=
> +               (get_gttype(dev_priv) << GUC_CTL_GTTYPE_SHIFT) |
> +               (get_core_family(dev_priv) << GUC_CTL_COREFAMILY_SHIFT);
> +
> +       /*
> +        * GuC ARAT increment is 10 ns. GuC default scheduler quantum is one
> +        * second. This ARAR is calculated by:
> +        * Scheduler-Quantum-in-ns / ARAT-increment-in-ns = 1000000000 / 10
> +        */
> +       params[GUC_CTL_ARAT_HIGH] = 0;
> +       params[GUC_CTL_ARAT_LOW] = 100000000;
> +
> +       params[GUC_CTL_WA] |= GUC_CTL_WA_UK_BY_DRIVER;
> +
> +       params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
> +                       GUC_CTL_VCS2_ENABLED;
> +
> +       params[GUC_CTL_LOG_PARAMS] = guc->log.flags;
> +
> +       if (i915_modparams.guc_log_level >= 0) {
> +               params[GUC_CTL_DEBUG] =
> +                       i915_modparams.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
> +       } else
> +               params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;

Where there is one {, there is all. } else { ... }

> +
> +       /* If GuC submission is enabled, set up additional parameters here */
> +       if (i915_modparams.enable_guc_submission) {
> +               u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
> +               u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool);
> +               u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
> +
> +               params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT;
> +               params[GUC_CTL_DEBUG] |= GUC_ADS_ENABLED;
> +
> +               pgs >>= PAGE_SHIFT;
> +               params[GUC_CTL_CTXINFO] = (pgs << GUC_CTL_BASE_ADDR_SHIFT) |
> +                       (ctx_in_16 << GUC_CTL_CTXNUM_IN16_SHIFT);
> +
> +               params[GUC_CTL_FEATURE] |= GUC_CTL_KERNEL_SUBMISSIONS;
> +
> +               /* Unmask this bit to enable the GuC's internal scheduler */
> +               params[GUC_CTL_FEATURE] &= ~GUC_CTL_DISABLE_SCHEDULER;
> +       }
> +
> +       intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
> +
> +       I915_WRITE(SOFT_SCRATCH(0), 0);
> +
> +       for (i = 0; i < GUC_CTL_MAX_DWORDS; i++)

for (i = 1; i <= MAX; i++) ?

> +               I915_WRITE(SOFT_SCRATCH(1 + i), params[i]);
> +
> +       intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> +
> +}

Ok, this is just motion, so disregard the changes above (except make it
checkpatch clean?) in this patch.
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: [P v4 04/11] drm/i915/guc: Rename intel_guc_loader.c to intel_guc_fw.c

[Chris Wilson]

Quoting Michal Wajdeczko (2017-10-10 15:51:28)
> Remaining functions in intel_guc_loader.c were focused around
> GuC firmware. Rename them to match object-verb pattern and
> rename file itself.
> 
> Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@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: [P v4 07/11] drm/i915/guc: Pick better place for Guc final status message

[Chris Wilson]

Quoting Michal Wajdeczko (2017-10-10 15:51:31)
> Guc status message printed right after firmware upload may be too
> optimistic, as we may fail on subsequent steps. Move that message
> to the end of intel_uc_init_hw where we know the status for sure.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_fw.c | 6 ------
>  drivers/gpu/drm/i915/intel_uc.c     | 6 ++++++
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
> index 020ce26..51be318 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_guc_fw.c
> @@ -294,11 +294,5 @@ int intel_guc_fw_upload(struct intel_guc *guc)
>  
>         guc->fw.load_status = INTEL_UC_FIRMWARE_SUCCESS;
>  
> -       DRM_INFO("GuC %s (firmware %s [version %u.%u])\n",
> -                i915_modparams.enable_guc_submission ? "submission enabled" :
> -                                                       "loaded",
> -                guc->fw.path,
> -                guc->fw.major_ver_found, guc->fw.minor_ver_found);
> -
>         return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 048f5c4..7dd7546 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -222,6 +222,12 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>                         goto err_interrupts;
>         }
>  
> +       DRM_INFO("GuC %s (firmware %s [version %u.%u])\n",
> +                i915_modparams.enable_guc_submission ? "submission enabled" :
> +                                                       "loaded",
> +                guc->fw.path,
> +                guc->fw.major_ver_found, guc->fw.minor_ver_found);

Can we also polish it to DRM_DEV_INFO(&dev_priv->dev, ...) ?

And does it really need intel_uc_init_hw in the user facing message?
i.e. just a plain dev_info()?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [P v4 08/11] drm/i915/uc: Improve debug messages in firmware fetch

[Chris Wilson]

Quoting Michal Wajdeczko (2017-10-10 15:51:32)
> Time to remove less important info and make messages clear
> and consistent.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uc_fw.c | 73 +++++++++++++++++++++++---------------
>  1 file changed, 44 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
> index 482115b..b52d6b6 100644
> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
> @@ -45,26 +45,33 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>         size_t size;
>         int err;
>  
> +       DRM_DEBUG_DRIVER("%s fw fetch %s\n",
> +                        intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
> +
>         if (!uc_fw->path)
>                 return;
>  
>         uc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
> -
> -       DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n",
> +       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)
> -               goto fail;
> -       if (!fw)
> +       if (err) {
> +               DRM_NOTE("%s: Error while requesting firmware\n",
> +                        intel_uc_fw_type_repr(uc_fw->type));

So what am I, the user, meant to do? Do I need to worry? What are the
consequences of this?

>                 goto fail;
> +       }

...
fail:
> +       DRM_WARN("%s: Failed to fetch firmware %s (error %d)\n",
> +                intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);

And then my "significant but normal" message has suddenly become a
warning the driver is crippled. Make up your mind, do I need to panic or
not?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [P v4 09/11] drm/i915/uc: Unify firmware loading

[Chris Wilson]

Quoting Michal Wajdeczko (2017-10-10 15:51:33)
> Firmware loading for GuC and Huc are similar. Move common code
> into generic function for maximum reuse.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_fw.c | 52 +++---------------------
>  drivers/gpu/drm/i915/intel_huc.c    | 53 +++---------------------
>  drivers/gpu/drm/i915/intel_uc_fw.c  | 81 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uc_fw.h  |  4 ++
>  4 files changed, 95 insertions(+), 95 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
> index 51be318..4629ff3 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_guc_fw.c
> @@ -193,24 +193,13 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
>  /*
>   * Load the GuC firmware blob into the MinuteIA.
>   */
> -static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
> +static int guc_ucode_xfer(struct intel_uc_fw *guc_fw, struct i915_vma *vma)
>  {
> -       struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
> -       struct i915_vma *vma;
> +       struct intel_guc *guc = container_of(guc_fw, struct intel_guc, fw);
> +       struct drm_i915_private *dev_priv = guc_to_i915(guc);
>         int ret;
>  
> -       ret = i915_gem_object_set_to_gtt_domain(guc_fw->obj, false);
> -       if (ret) {
> -               DRM_DEBUG_DRIVER("set-domain failed %d\n", ret);
> -               return ret;
> -       }
> -
> -       vma = i915_gem_object_ggtt_pin(guc_fw->obj, NULL, 0, 0,
> -                                      PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> -       if (IS_ERR(vma)) {
> -               DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
> -               return PTR_ERR(vma);
> -       }
> +       GEM_BUG_ON(guc_fw->type != INTEL_UC_FW_TYPE_GUC);
>  
>         intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>  
> @@ -245,12 +234,6 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>  
>         intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>  
> -       /*
> -        * We keep the object pages for reuse during resume. But we can unpin it
> -        * now that DMA has completed, so it doesn't continue to take up space.
> -        */
> -       i915_vma_unpin(vma);
> -
>         return ret;
>  }
>  
> @@ -269,30 +252,5 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>   */
>  int intel_guc_fw_upload(struct intel_guc *guc)
>  {
> -       struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -       const char *fw_path = guc->fw.path;
> -       int ret;
> -
> -       DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
> -               fw_path,
> -               intel_uc_fw_status_repr(guc->fw.fetch_status),
> -               intel_uc_fw_status_repr(guc->fw.load_status));
> -
> -       if (guc->fw.fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> -               return -EIO;
> -
> -       guc->fw.load_status = INTEL_UC_FIRMWARE_PENDING;
> -
> -       DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> -               intel_uc_fw_status_repr(guc->fw.fetch_status),
> -               intel_uc_fw_status_repr(guc->fw.load_status));
> -
> -       ret = guc_ucode_xfer(dev_priv);
> -
> -       if (ret)
> -               return -EAGAIN;
> -
> -       guc->fw.load_status = INTEL_UC_FIRMWARE_SUCCESS;
> -
> -       return 0;
> +       return intel_uc_fw_upload(&guc->fw, guc_ucode_xfer);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> index 4b4cf56..8be3bfe 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -85,26 +85,15 @@ MODULE_FIRMWARE(I915_KBL_HUC_UCODE);
>   *
>   * Return: 0 on success, non-zero on failure
>   */
> -static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
> +static int huc_ucode_xfer(struct intel_uc_fw *huc_fw, struct i915_vma *vma)
>  {
> -       struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
> -       struct i915_vma *vma;
> +       struct intel_huc *huc = container_of(huc_fw, struct intel_huc, fw);
> +       struct drm_i915_private *dev_priv = huc_to_i915(huc);
>         unsigned long offset = 0;
>         u32 size;
>         int ret;
>  
> -       ret = i915_gem_object_set_to_gtt_domain(huc_fw->obj, false);
> -       if (ret) {
> -               DRM_DEBUG_DRIVER("set-domain failed %d\n", ret);
> -               return ret;
> -       }
> -
> -       vma = i915_gem_object_ggtt_pin(huc_fw->obj, NULL, 0, 0,
> -                               PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> -       if (IS_ERR(vma)) {
> -               DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
> -               return PTR_ERR(vma);
> -       }
> +       GEM_BUG_ON(huc_fw->type != INTEL_UC_FW_TYPE_HUC);
>  
>         intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>  
> @@ -135,12 +124,6 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
>  
>         intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>  
> -       /*
> -        * We keep the object pages for reuse during resume. But we can unpin it
> -        * now that DMA has completed, so it doesn't continue to take up space.
> -        */
> -       i915_vma_unpin(vma);
> -
>         return ret;
>  }
>  
> @@ -194,33 +177,7 @@ void intel_huc_select_fw(struct intel_huc *huc)
>   */
>  void intel_huc_init_hw(struct intel_huc *huc)
>  {
> -       struct drm_i915_private *dev_priv = huc_to_i915(huc);
> -       int err;
> -
> -       DRM_DEBUG_DRIVER("%s fw status: fetch %s, load %s\n",
> -               huc->fw.path,
> -               intel_uc_fw_status_repr(huc->fw.fetch_status),
> -               intel_uc_fw_status_repr(huc->fw.load_status));
> -
> -       if (huc->fw.fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> -               return;
> -
> -       huc->fw.load_status = INTEL_UC_FIRMWARE_PENDING;
> -
> -       err = huc_ucode_xfer(dev_priv);
> -
> -       huc->fw.load_status = err ?
> -               INTEL_UC_FIRMWARE_FAIL : INTEL_UC_FIRMWARE_SUCCESS;
> -
> -       DRM_DEBUG_DRIVER("%s fw status: fetch %s, load %s\n",
> -               huc->fw.path,
> -               intel_uc_fw_status_repr(huc->fw.fetch_status),
> -               intel_uc_fw_status_repr(huc->fw.load_status));
> -
> -       if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> -               DRM_ERROR("Failed to complete HuC uCode load with ret %d\n", err);
> -
> -       return;
> +       intel_uc_fw_upload(&huc->fw, huc_ucode_xfer);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
> index b52d6b6..16dc970 100644
> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
> @@ -182,6 +182,87 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>  }
>  
>  /**
> + * intel_uc_fw_upload - load uC firmware using custom loader
> + *
> + * @uc_fw: uC firmware
> + * @loader: custom uC firmware loader function
> + *
> + * Loads uC firmware using custom loader and updates internal flags.
> + */
> +int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
> +                      int (*loader)(struct intel_uc_fw *uc_fw,
> +                                    struct i915_vma *vma))

Suggestion s/loader/xfer/, it's a bit more commonplace.

> +{
> +       struct i915_vma *vma;
> +       int err;
> +
> +       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 -EIO;
> +
> +       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));
> +
> +       /* Pin object with firmware */
> +       err = i915_gem_object_set_to_gtt_domain(uc_fw->obj, false);
> +       if (err) {
> +               DRM_DEBUG_DRIVER("%s fw set-domain failed %d\n",
> +                                intel_uc_fw_type_repr(uc_fw->type), err);
> +               goto fail;
> +       }
> +
> +       vma = i915_gem_object_ggtt_pin(uc_fw->obj, NULL, 0, 0,
> +                                      PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> +       if (IS_ERR(vma)) {
> +               err = PTR_ERR(vma);
> +               DRM_DEBUG_DRIVER("%s fw pin failed %d\n",
> +                                intel_uc_fw_type_repr(uc_fw->type),
> +                                err);
> +               goto fail;
> +       }
> +
> +       /* Call custom loader */
> +       err = loader(uc_fw, vma);
> +       uc_fw->load_status = err ?
> +                            INTEL_UC_FIRMWARE_FAIL :
> +                            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));
> +
> +       /*
> +        * We keep the object pages for reuse during resume. But we can unpin it
> +        * now that DMA has completed, so it doesn't continue to take up space.
> +        */
> +       i915_vma_unpin(vma);
> +
> +       if (err)
> +               goto fail;
> +
> +       DRM_INFO("%s: Loaded firmware %s (version %u.%u)\n",
> +                intel_uc_fw_type_repr(uc_fw->type),
> +                uc_fw->path,
> +                uc_fw->major_ver_found, uc_fw->minor_ver_found);
> +
> +       return 0;
> +
> +fail:
> +       uc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;

?  You just set this before.

So maybe,

	err = xfer(uc_fw, vma);
	i915_vma_unpin(vma);
	if (err)
		goto fail;

	uc_fw->last_status = INTEL_UC_FIRMWARE_SUCCESS;
	return 0;

fail:
	uc_fw->last_status = INTEL_UC_FIRMWARE_FAIL;
	...
	return err;

> +       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));
> +
> +       DRM_ERROR("%s: Failed to load firmware %s (error %d)\n",
> +                 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);

It was WARN before AND NOW ITS AN ERROR! PANIC, PANIC, PANIC!

I thought you were aiming to present a clear and consistent message to
the user :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [P v4 10/11] drm/i915/huc: Move fw select function

[Chris Wilson]

Quoting Michal Wajdeczko (2017-10-10 15:51:34)
> No functional change. Just place the function closer to related
> definitions.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@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: [P v4 02/11] drm/i915/guc: Move GuC boot param initialization out of xfer

[Michal Wajdeczko]

On Thu, 12 Oct 2017 10:51:58 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2017-10-10 15:51:26)
>> We want to keep ucode xfer functions separate from other
>> initialization. Once separated, add explicit forcewake.
>>
>> Suggested-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_guc.c        | 88  
>> +++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_guc.h        |  1 +
>>  drivers/gpu/drm/i915/intel_guc_loader.c | 85  
>> -------------------------------
>>  drivers/gpu/drm/i915/intel_uc.c         |  1 +
>>  4 files changed, 90 insertions(+), 85 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c

<snip>

>> +
>> +       for (i = 0; i < GUC_CTL_MAX_DWORDS; i++)
>
> for (i = 1; i <= MAX; i++) ?
>
>> +               I915_WRITE(SOFT_SCRATCH(1 + i), params[i]);

Not so sure, as this loop is over CTL params[] not SCRATCH() regs.

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

Re: [P v4 02/11] drm/i915/guc: Move GuC boot param initialization out of xfer

[Michal Wajdeczko]

On Thu, 12 Oct 2017 00:55:29 +0200, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@intel.com> wrote:

>
>
> On 10/10/17 07:51, Michal Wajdeczko wrote:
>> We want to keep ucode xfer functions separate from other
>> initialization. Once separated, add explicit forcewake.

...

>> +	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>
> I don't think we need this explicit forcewake if we use I915_WRITE(),  
> because that should already wake the required wells. If you want to  
> explicitly take forcewake then we can use I915_WRITE_FW(). Also,  
> FORCEWAKE_BLITTER should be enough.

I would stay with both explicit forcewake and I915_WRITE() first to
be aligned with similar code in send_mmio and second to follow usage
rules listed in description of I915_WRITE_FW().

> I'd also add a comment to say that the register are power context saved  
> so it's ok to release forcewake here and take it again at xfer time.
>

...

>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -195,6 +195,7 @@ int intel_uc_init_hw(struct drm_i915_private  
>> *dev_priv)
>>   			goto err_submission;
>>     		intel_huc_init_hw(&dev_priv->huc);
>> +		intel_guc_init_params(guc);
>
> Is the value of the registers lost/corrupted on guc reset? if not we  
> could just move this outside the retry loop to avoid doing it more than  
> once.
>

In theory they should survive as they are defined as software registers.
However, I'm not sure we can trust Guc (that just failed to load) that
it didn't corrupt them. Also as we retry only for Gen9 cost of this extra
initialization should be minimal.

Thanks for the review,
Michal
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [P v4 08/11] drm/i915/uc: Improve debug messages in firmware fetch

[Michal Wajdeczko]

On Thu, 12 Oct 2017 11:07:21 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2017-10-10 15:51:32)
>> Time to remove less important info and make messages clear
>> and consistent.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_uc_fw.c | 73  
>> +++++++++++++++++++++++---------------
>>  1 file changed, 44 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c  
>> b/drivers/gpu/drm/i915/intel_uc_fw.c
>> index 482115b..b52d6b6 100644
>> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
>> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
>> @@ -45,26 +45,33 @@ void intel_uc_fw_fetch(struct drm_i915_private  
>> *dev_priv,
>>         size_t size;
>>         int err;
>>
>> +       DRM_DEBUG_DRIVER("%s fw fetch %s\n",
>> +                        intel_uc_fw_type_repr(uc_fw->type),  
>> uc_fw->path);
>> +
>>         if (!uc_fw->path)
>>                 return;
>>
>>         uc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
>> -
>> -       DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch  
>> status %s\n",
>> +       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)
>> -               goto fail;
>> -       if (!fw)
>> +       if (err) {
>> +               DRM_NOTE("%s: Error while requesting firmware\n",
>> +                        intel_uc_fw_type_repr(uc_fw->type));
>
> So what am I, the user, meant to do? Do I need to worry? What are the
> consequences of this?

Yes, you can be little worried.
Being here means that driver decided to install *desired* firmware.

We don't know the consequences yet, as there might be a fallback
scenario available (like execlist submission, huc not used)

As we are jumping into fail label, which will start with similar
message, I can downgrade this message into DRM_DEBUG_DRIVER to
avoid duplication.

>
>>                 goto fail;
>> +       }
>
> ...
> fail:
>> +       DRM_WARN("%s: Failed to fetch firmware %s (error %d)\n",
>> +                intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
>
> And then my "significant but normal" message has suddenly become a
> warning the driver is crippled. Make up your mind, do I need to panic or
> not?

Good point. This can be DRM_NOTE.

But maybe we should promote some other existing DRM_NOTEs into:

DRM_WARN("%s: Unexpected firmware size (%zu, min %zu)\n",
DRM_WARN("%s: Mismatched firmware header definition\n",
DRM_WARN("%s: Mismatched firmware header definition\n",
DRM_WARN("%s: Mismatched firmware RSA key size (%u)\n",
DRM_WARN("%s: Truncated firmware (%zu, expected %zu)\n",

as these indicates corrupted/mismatched data (and it's not normal)

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

Re: [P v4 09/11] drm/i915/uc: Unify firmware loading

[Michal Wajdeczko]

On Thu, 12 Oct 2017 11:12:23 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2017-10-10 15:51:33)
>> Firmware loading for GuC and Huc are similar. Move common code
>> into generic function for maximum reuse.
>>

...

>> +       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));
>> +
>> +       DRM_ERROR("%s: Failed to load firmware %s (error %d)\n",
>> +                 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
>
> It was WARN before AND NOW ITS AN ERROR! PANIC, PANIC, PANIC!
>
> I thought you were aiming to present a clear and consistent message to
> the user :)

Final failure of fw load always was a DRM_ERROR:

DRM_ERROR("Failed to complete HuC uCode load with ret %d\n", err);
in old intel_huc_init_hw() in huc.c

DRM_ERROR("GuC init failed\n")
in intel_uc_init_hw() in intel_uc.c

To be more consistent, I can downgrade above message to DRM_WARN
as this is still not normal case when valid firmware fails to load.

Based on above discussion, I think we should also fix messages in
intel_uc_init_hw and report DRM_ERROR only if Guc was "required".

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