[PATCH v2 0/5] GuC Scrub vol. 1

[PATCH v2 0/5] GuC Scrub vol. 1

[Arkadiusz Hiler]

After having HuC merged and taking feedback into account, it's time to respin
this series.

General GuC/HuC cleanup simplifying logic, and moving chunks around, as the area
is pretty rusty.

A lot of logic were extracted from intel_guc_load() to other functions - it not
only did handle the actual loading but had WA implementations and had the code
thatenabled submission baked it.

This is first the part of effort to clean it up.

Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Arkadiusz Hiler (5):
  drm/i915/uc: Rename intel_?uc_{setup,load}() to _init_hw()
  drm/i915/uc: Introduce intel_uc_init()
  drm/i915/guc: Extract param logic form guc_init
  drm/i915/guc: Simplify intel_guc_init_hw()
  drm/i915/uc: Simplify fw_path

 drivers/gpu/drm/i915/i915_drv.c         |   5 +-
 drivers/gpu/drm/i915/i915_gem.c         |   2 +-
 drivers/gpu/drm/i915/intel_guc_loader.c | 187 +++++---------------------------
 drivers/gpu/drm/i915/intel_huc.c        |  24 ++--
 drivers/gpu/drm/i915/intel_uc.c         | 134 +++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.h         |  11 +-
 6 files changed, 186 insertions(+), 177 deletions(-)

-- 
2.9.3

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

[PATCH 1/5] drm/i915/uc: Rename intel_?uc_{setup, load}() to _init_hw()

[Arkadiusz Hiler]

GuC historically has two "startup" functions called _init() and _setup()

Then HuC came with it's _init() and _load().

To make naming more consistent this commit renames intel_guc_setup() and
intel_huc_load() to *uc_init_hw() as it it seams more fitting - they are
called from the i915_gem_init_hw().

Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         |  2 +-
 drivers/gpu/drm/i915/intel_guc_loader.c | 14 +++++++-------
 drivers/gpu/drm/i915/intel_huc.c        |  6 +++---
 drivers/gpu/drm/i915/intel_uc.h         |  4 ++--
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7129792..5234c76 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4478,7 +4478,7 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
 	intel_mocs_init_l3cc_table(dev_priv);
 
 	/* We can't enable contexts until all firmware is loaded */
-	ret = intel_guc_setup(dev_priv);
+	ret = intel_guc_init_hw(dev_priv);
 	if (ret)
 		goto out;
 
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 8ef33d8..f5efe28 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -428,19 +428,19 @@ static int guc_hw_reset(struct drm_i915_private *dev_priv)
 }
 
 /**
- * intel_guc_setup() - finish preparing the GuC for activity
+ * intel_guc_init_hw() - finish preparing the GuC for activity
  * @dev_priv:	i915 device private
  *
- * Called from gem_init_hw() during driver loading and also after a GPU reset.
+ * 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.
+ * 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_setup(struct drm_i915_private *dev_priv)
+int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 {
 	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
 	const char *fw_path = guc_fw->path;
@@ -506,7 +506,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
 		if (err)
 			goto fail;
 
-		intel_huc_load(dev_priv);
+		intel_huc_init_hw(dev_priv);
 		err = guc_ucode_xfer(dev_priv);
 		if (!err)
 			break;
@@ -732,7 +732,7 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
  * Called early during driver load, but after GEM is initialised.
  *
  * The firmware will be transferred to the GuC's memory later,
- * when intel_guc_setup() is called.
+ * when intel_guc_init_hw() is called.
  */
 void intel_guc_init(struct drm_i915_private *dev_priv)
 {
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index c144609..94d369b 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -150,7 +150,7 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
  * is not capable or driver yet support it. And there will be no error message
  * for INTEL_UC_FIRMWARE_NONE cases.
  *
- * The DMA-copying to HW is done later when intel_huc_load() is called.
+ * The DMA-copying to HW is done later when intel_huc_init_hw() is called.
  */
 void intel_huc_init(struct drm_i915_private *dev_priv)
 {
@@ -191,7 +191,7 @@ void intel_huc_init(struct drm_i915_private *dev_priv)
 }
 
 /**
- * intel_huc_load() - load HuC uCode to device
+ * intel_huc_init_hw() - load HuC uCode to device
  * @dev_priv: the drm_i915_private device
  *
  * Called from guc_setup() during driver loading and also after a GPU reset.
@@ -203,7 +203,7 @@ void intel_huc_init(struct drm_i915_private *dev_priv)
  *
  * Return:	non-zero code on error
  */
-int intel_huc_load(struct drm_i915_private *dev_priv)
+int intel_huc_init_hw(struct drm_i915_private *dev_priv)
 {
 	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
 	int err;
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index d74f4d3..dd34a1b 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -190,7 +190,7 @@ int intel_guc_sample_forcewake(struct intel_guc *guc);
 
 /* intel_guc_loader.c */
 extern void intel_guc_init(struct drm_i915_private *dev_priv);
-extern int intel_guc_setup(struct drm_i915_private *dev_priv);
+extern int intel_guc_init_hw(struct drm_i915_private *dev_priv);
 extern void intel_guc_fini(struct drm_i915_private *dev_priv);
 extern const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
 extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
@@ -225,7 +225,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 /* intel_huc.c */
 void intel_huc_init(struct drm_i915_private *dev_priv);
 void intel_huc_fini(struct drm_i915_private  *dev_priv);
-int intel_huc_load(struct drm_i915_private *dev_priv);
+int intel_huc_init_hw(struct drm_i915_private *dev_priv);
 void intel_guc_auth_huc(struct drm_i915_private *dev_priv);
 
 #endif
-- 
2.9.3

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

[PATCH 2/5] drm/i915/uc: Introduce intel_uc_init()

[Arkadiusz Hiler]

We will be able to bulk call all firmware _init() function from single
point and offset some general logic there as needed

Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 3 +--
 drivers/gpu/drm/i915/intel_uc.c | 6 ++++++
 drivers/gpu/drm/i915/intel_uc.h | 1 +
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b1457a89..6d0798e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -610,8 +610,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (ret)
 		goto cleanup_irq;
 
-	intel_huc_init(dev_priv);
-	intel_guc_init(dev_priv);
+	intel_uc_init(dev_priv);
 
 	ret = i915_gem_init(dev_priv);
 	if (ret)
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index c46bc85..d9d0566 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -30,6 +30,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
 	mutex_init(&dev_priv->guc.send_mutex);
 }
 
+void intel_uc_init(struct drm_i915_private *dev_priv)
+{
+	intel_huc_init(dev_priv);
+	intel_guc_init(dev_priv);
+}
+
 /*
  * Read GuC command/status register (SOFT_SCRATCH_0)
  * Return true if it contains a response rather than a command
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index dd34a1b..264761a 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -185,6 +185,7 @@ struct intel_huc {
 
 /* intel_uc.c */
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
+void intel_uc_init(struct drm_i915_private *dev_priv);
 int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
 
-- 
2.9.3

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

[PATCH 3/5] drm/i915/guc: Extract param logic form guc_init

[Arkadiusz Hiler]

Let intel_guc_init() focus on determining and fetching the correct
firmware.

This patch introduces intel_sanitize_uc_params() that is called from
intel_uc_init().

Then, if we have GuC, we can call intel_guc_init() conditionally and we
do not have to do internal checks.

Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         |  2 ++
 drivers/gpu/drm/i915/intel_guc_loader.c | 15 +--------------
 drivers/gpu/drm/i915/intel_uc.c         | 25 ++++++++++++++++++++++++-
 3 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6d0798e..e520895 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -994,6 +994,8 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
 
 	i915.semaphores = intel_sanitize_semaphores(dev_priv, i915.semaphores);
 	DRM_DEBUG_DRIVER("use GPU sempahores? %s\n", yesno(i915.semaphores));
+
+	intel_uc_sanitize_params(dev_priv);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index f5efe28..db5713c 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -726,7 +726,7 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 }
 
 /**
- * intel_guc_init() - define parameters and fetch firmware
+ * intel_guc_init() - determine and fetch firmware
  * @dev_priv:	i915 device private
  *
  * Called early during driver load, but after GEM is initialised.
@@ -739,17 +739,6 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
 	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
 	const char *fw_path;
 
-	if (!HAS_GUC(dev_priv)) {
-		i915.enable_guc_loading = 0;
-		i915.enable_guc_submission = 0;
-	} else {
-		/* A negative value means "use platform default" */
-		if (i915.enable_guc_loading < 0)
-			i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
-		if (i915.enable_guc_submission < 0)
-			i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
-	}
-
 	if (!HAS_GUC_UCODE(dev_priv)) {
 		fw_path = NULL;
 	} else if (IS_SKYLAKE(dev_priv)) {
@@ -773,8 +762,6 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
 	guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
 
 	/* Early (and silent) return if GuC loading is disabled */
-	if (!i915.enable_guc_loading)
-		return;
 	if (fw_path == NULL)
 		return;
 	if (*fw_path == '\0')
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index d9d0566..d1ca41d 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -25,6 +25,24 @@
 #include "i915_drv.h"
 #include "intel_uc.h"
 
+void intel_uc_sanitize_params(struct drm_i915_private *dev_priv)
+{
+	if (!HAS_GUC(dev_priv)) {
+		i915.enable_guc_loading = 0;
+		i915.enable_guc_submission = 0;
+	} else {
+		/* A negative value means "use platform default" */
+		if (i915.enable_guc_loading < 0)
+			i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
+		if (i915.enable_guc_submission < 0)
+			i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
+	}
+
+	/* can't enable guc submission without guc loaded */
+	if (!i915.enable_guc_loading)
+		i915.enable_guc_submission = 0;
+}
+
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
 {
 	mutex_init(&dev_priv->guc.send_mutex);
@@ -32,7 +50,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
 
 void intel_uc_init(struct drm_i915_private *dev_priv)
 {
-	intel_huc_init(dev_priv);
+	if (!i915.enable_guc_loading)
+		return;
+
+	if (HAS_HUC_UCODE(dev_priv))
+		intel_huc_init(dev_priv);
+
 	intel_guc_init(dev_priv);
 }
 
-- 
2.9.3

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

[PATCH 4/5] drm/i915/guc: Simplify intel_guc_init_hw()

[Arkadiusz Hiler]

Current version of intel_guc_init_hw() does a lot:
 - cares about submission
 - loads huc
 - implement WA

This change offloads some of the logic to intel_uc_load(), which now
cares about the above.

Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         |   2 +-
 drivers/gpu/drm/i915/intel_guc_loader.c | 136 +++-----------------------------
 drivers/gpu/drm/i915/intel_uc.c         | 105 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.h         |   6 ++
 4 files changed, 124 insertions(+), 125 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5234c76..afc7e14 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4478,7 +4478,7 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
 	intel_mocs_init_l3cc_table(dev_priv);
 
 	/* We can't enable contexts until all firmware is loaded */
-	ret = intel_guc_init_hw(dev_priv);
+	ret = intel_uc_init_hw(dev_priv);
 	if (ret)
 		goto out;
 
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index db5713c..4ea705d 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -91,7 +91,7 @@ const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
 	}
 };
 
-static void guc_interrupts_release(struct drm_i915_private *dev_priv)
+void intel_guc_interrupts_release(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
@@ -109,7 +109,7 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv)
 	I915_WRITE(GUC_WD_VECS_IER, 0);
 }
 
-static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
+void intel_guc_interrupts_capture(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
@@ -409,24 +409,6 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-static int guc_hw_reset(struct drm_i915_private *dev_priv)
-{
-	int ret;
-	u32 guc_status;
-
-	ret = intel_guc_reset(dev_priv);
-	if (ret) {
-		DRM_ERROR("GuC reset failed, ret = %d\n", ret);
-		return ret;
-	}
-
-	guc_status = I915_READ(GUC_STATUS);
-	WARN(!(guc_status & GS_MIA_IN_RESET),
-	     "GuC status: 0x%x, MIA core expected to be in reset\n", guc_status);
-
-	return ret;
-}
-
 /**
  * intel_guc_init_hw() - finish preparing the GuC for activity
  * @dev_priv:	i915 device private
@@ -444,147 +426,53 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 {
 	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
 	const char *fw_path = guc_fw->path;
-	int retries, ret, err;
+	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));
 
-	/* Loading forbidden, or no firmware to load? */
-	if (!i915.enable_guc_loading) {
-		err = 0;
-		goto fail;
-	} else if (fw_path == NULL) {
+	if (fw_path == NULL) {
 		/* Device is known to have no uCode (e.g. no GuC) */
-		err = -ENXIO;
-		goto fail;
+		return -ENXIO;
 	} else if (*fw_path == '\0') {
 		/* Device has a GuC but we don't know what f/w to load? */
 		WARN(1, "No GuC firmware known for this platform!\n");
-		err = -ENODEV;
-		goto fail;
+		return -ENODEV;
 	}
 
 	/* Fetch failed, or already fetched but failed to load? */
 	if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS) {
-		err = -EIO;
-		goto fail;
+		return -EIO;
 	} else if (guc_fw->load_status == INTEL_UC_FIRMWARE_FAIL) {
-		err = -ENOEXEC;
-		goto fail;
+		return -ENOEXEC;
 	}
 
-	guc_interrupts_release(dev_priv);
-	gen9_reset_guc_interrupts(dev_priv);
-
-	/* We need to notify the guc whenever we change the GGTT */
-	i915_ggtt_enable_guc(dev_priv);
-
 	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));
 
-	err = i915_guc_submission_init(dev_priv);
-	if (err)
-		goto fail;
-
 	/*
 	 * WaEnableuKernelHeaderValidFix:skl,bxt
 	 * For BXT, this is only upto B0 but below WA is required for later
 	 * steppings also so this is extended as well.
 	 */
-	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
-	for (retries = 3; ; ) {
-		/*
-		 * Always reset the GuC just before (re)loading, so
-		 * that the state and timing are fairly predictable
-		 */
-		err = guc_hw_reset(dev_priv);
-		if (err)
-			goto fail;
+	ret = guc_ucode_xfer(dev_priv);
 
-		intel_huc_init_hw(dev_priv);
-		err = guc_ucode_xfer(dev_priv);
-		if (!err)
-			break;
-
-		if (--retries == 0)
-			goto fail;
-
-		DRM_INFO("GuC fw load failed: %d; will reset and "
-			 "retry %d more time(s)\n", err, retries);
-	}
+	if (ret)
+		return -EAGAIN;
 
 	guc_fw->load_status = INTEL_UC_FIRMWARE_SUCCESS;
 
-	intel_guc_auth_huc(dev_priv);
-
-	if (i915.enable_guc_submission) {
-		if (i915.guc_log_level >= 0)
-			gen9_enable_guc_interrupts(dev_priv);
-
-		err = i915_guc_submission_enable(dev_priv);
-		if (err)
-			goto fail;
-		guc_interrupts_capture(dev_priv);
-	}
-
 	DRM_INFO("GuC %s (firmware %s [version %u.%u])\n",
 		 i915.enable_guc_submission ? "submission enabled" : "loaded",
 		 guc_fw->path,
 		 guc_fw->major_ver_found, guc_fw->minor_ver_found);
 
 	return 0;
-
-fail:
-	if (guc_fw->load_status == INTEL_UC_FIRMWARE_PENDING)
-		guc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;
-
-	guc_interrupts_release(dev_priv);
-	i915_guc_submission_disable(dev_priv);
-	i915_guc_submission_fini(dev_priv);
-	i915_ggtt_disable_guc(dev_priv);
-
-	/*
-	 * We've failed to load the firmware :(
-	 *
-	 * Decide whether to disable GuC submission and fall back to
-	 * execlist mode, and whether to hide the error by returning
-	 * zero or to return -EIO, which the caller will treat as a
-	 * nonfatal error (i.e. it doesn't prevent driver load, but
-	 * marks the GPU as wedged until reset).
-	 */
-	if (i915.enable_guc_loading > 1) {
-		ret = -EIO;
-	} else if (i915.enable_guc_submission > 1) {
-		ret = -EIO;
-	} else {
-		ret = 0;
-	}
-
-	if (err == 0 && !HAS_GUC_UCODE(dev_priv))
-		;	/* Don't mention the GuC! */
-	else if (err == 0)
-		DRM_INFO("GuC firmware load skipped\n");
-	else if (ret != -EIO)
-		DRM_NOTE("GuC firmware load failed: %d\n", err);
-	else
-		DRM_WARN("GuC firmware load failed: %d\n", err);
-
-	if (i915.enable_guc_submission) {
-		if (fw_path == NULL)
-			DRM_INFO("GuC submission without firmware not supported\n");
-		if (ret == 0)
-			DRM_NOTE("Falling back from GuC submission to execlist mode\n");
-		else
-			DRM_ERROR("GuC init failed: %d\n", ret);
-	}
-	i915.enable_guc_submission = 0;
-
-	return ret;
 }
 
 void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
@@ -782,7 +670,7 @@ void intel_guc_fini(struct drm_i915_private *dev_priv)
 	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
-	guc_interrupts_release(dev_priv);
+	intel_guc_interrupts_release(dev_priv);
 	i915_guc_submission_disable(dev_priv);
 	i915_guc_submission_fini(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index d1ca41d..bba220c 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -25,6 +25,25 @@
 #include "i915_drv.h"
 #include "intel_uc.h"
 
+static int uc_hw_reset(struct drm_i915_private *dev_priv)
+{
+	int ret;
+	u32 guc_status;
+
+	ret = intel_guc_reset(dev_priv);
+	if (ret) {
+		DRM_ERROR("GuC reset failed, ret = %d\n", ret);
+		return ret;
+	}
+
+	guc_status = I915_READ(GUC_STATUS);
+	WARN(!(guc_status & GS_MIA_IN_RESET),
+	     "GuC status: 0x%x, MIA core expected to be in reset\n",
+	     guc_status);
+
+	return ret;
+}
+
 void intel_uc_sanitize_params(struct drm_i915_private *dev_priv)
 {
 	if (!HAS_GUC(dev_priv)) {
@@ -59,6 +78,92 @@ void intel_uc_init(struct drm_i915_private *dev_priv)
 	intel_guc_init(dev_priv);
 }
 
+int intel_uc_init_hw(struct drm_i915_private *dev_priv)
+{
+	int ret, retries;
+
+	/* guc not enabled, nothing to do */
+	if (!i915.enable_guc_loading)
+		return 0;
+
+	intel_guc_interrupts_release(dev_priv);
+	gen9_reset_guc_interrupts(dev_priv);
+
+	/* We need to notify the guc whenever we change the GGTT */
+	i915_ggtt_enable_guc(dev_priv);
+
+	if (i915.enable_guc_submission) {
+		ret = i915_guc_submission_init(dev_priv);
+		if (ret)
+			goto fail;
+	}
+
+	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
+	retries = GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS;
+	while (retries--) {
+		/*
+		 * Always reset the GuC just before (re)loading, so
+		 * that the state and timing are fairly predictable
+		 */
+		ret = uc_hw_reset(dev_priv);
+		if (ret)
+			goto fail;
+
+		intel_huc_init_hw(dev_priv);
+		ret = intel_guc_init_hw(dev_priv);
+		if (ret == 0 || ret != -EAGAIN)
+			break;
+
+		DRM_INFO("GuC fw load failed: %d; will reset and "
+			 "retry %d more time(s)\n", ret, retries);
+	}
+
+	/* did we succeded or run out of retries? */
+	if (ret)
+		goto fail;
+
+	intel_guc_auth_huc(dev_priv);
+	if (i915.enable_guc_submission) {
+		if (i915.guc_log_level >= 0)
+			gen9_enable_guc_interrupts(dev_priv);
+
+		ret = i915_guc_submission_enable(dev_priv);
+		if (ret)
+			goto fail;
+		intel_guc_interrupts_capture(dev_priv);
+	}
+
+	return 0;
+
+fail:
+	/*
+	 * We've failed to load the firmware :(
+	 *
+	 * Decide whether to disable GuC submission and fall back to
+	 * execlist mode, and whether to hide the error by returning
+	 * zero or to return -EIO, which the caller will treat as a
+	 * nonfatal error (i.e. it doesn't prevent driver load, but
+	 * marks the GPU as wedged until reset).
+	 */
+	DRM_ERROR("GuC init failed\n");
+	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
+		ret = -EIO;
+	else
+		ret = 0;
+
+	if (i915.enable_guc_submission) {
+		i915.enable_guc_submission = 0;
+		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
+	}
+
+	i915_ggtt_disable_guc(dev_priv);
+	intel_guc_interrupts_release(dev_priv);
+	i915_guc_submission_disable(dev_priv);
+	i915_guc_submission_fini(dev_priv);
+
+	return ret;
+}
+
 /*
  * Read GuC command/status register (SOFT_SCRATCH_0)
  * Return true if it contains a response rather than a command
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 264761a..fbe774e 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -29,6 +29,8 @@
 #include "intel_ringbuffer.h"
 
 #include "i915_vma.h"
+/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
+#define GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS 3
 
 struct drm_i915_gem_request;
 
@@ -184,8 +186,10 @@ struct intel_huc {
 };
 
 /* intel_uc.c */
+void intel_uc_sanitize_params(struct drm_i915_private *dev_priv);
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
 void intel_uc_init(struct drm_i915_private *dev_priv);
+int intel_uc_init_hw(struct drm_i915_private *dev_priv);
 int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
 
@@ -198,6 +202,8 @@ extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
 extern int intel_guc_resume(struct drm_i915_private *dev_priv);
 void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 	struct intel_uc_fw *uc_fw);
+void intel_guc_interrupts_release(struct drm_i915_private *dev_priv);
+void intel_guc_interrupts_capture(struct drm_i915_private *dev_priv);
 u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
 
 /* i915_guc_submission.c */
-- 
2.9.3

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

[PATCH 5/5] drm/i915/uc: Simplify fw_path

[Arkadiusz Hiler]

Currently fw_path values can represent one of three possible states:

 1) NULL - device without the uC
 2) '\0' - device with the uC but have no firmware
 3) else - device with the uC and we have firmware

Second case is used only to WARN at a later stage.

We can WARN right away and merge cases 1 and 2.

Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 28 +++++++++++-----------------
 drivers/gpu/drm/i915/intel_huc.c        | 18 ++++++++----------
 2 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 4ea705d..f4875c2 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -434,12 +434,8 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
 		intel_uc_fw_status_repr(guc_fw->load_status));
 
 	if (fw_path == NULL) {
-		/* Device is known to have no uCode (e.g. no GuC) */
+		/* We do not have uCode for the device */
 		return -ENXIO;
-	} else if (*fw_path == '\0') {
-		/* Device has a GuC but we don't know what f/w to load? */
-		WARN(1, "No GuC firmware known for this platform!\n");
-		return -ENODEV;
 	}
 
 	/* Fetch failed, or already fetched but failed to load? */
@@ -625,11 +621,13 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 void intel_guc_init(struct drm_i915_private *dev_priv)
 {
 	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
-	const char *fw_path;
+	const char *fw_path = NULL;
 
-	if (!HAS_GUC_UCODE(dev_priv)) {
-		fw_path = NULL;
-	} else if (IS_SKYLAKE(dev_priv)) {
+	guc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
+	guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
+	guc_fw->fw = INTEL_UC_FW_TYPE_GUC;
+
+	if (IS_SKYLAKE(dev_priv)) {
 		fw_path = I915_SKL_GUC_UCODE;
 		guc_fw->major_ver_wanted = SKL_FW_MAJOR;
 		guc_fw->minor_ver_wanted = SKL_FW_MINOR;
@@ -641,24 +639,20 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
 		fw_path = I915_KBL_GUC_UCODE;
 		guc_fw->major_ver_wanted = KBL_FW_MAJOR;
 		guc_fw->minor_ver_wanted = KBL_FW_MINOR;
-	} else {
-		fw_path = "";	/* unknown device */
+	} else if (HAS_GUC_UCODE(dev_priv)) {
+		WARN(1, "No GuC firmware known for platform with GuC!\n");
+		i915.enable_guc_loading = 0;
 	}
 
 	guc_fw->path = fw_path;
-	guc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
-	guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
 
-	/* Early (and silent) return if GuC loading is disabled */
+	/* Early return if we do not have firmware to fetch */
 	if (fw_path == NULL)
 		return;
-	if (*fw_path == '\0')
-		return;
 
 	guc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
 	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
 	intel_uc_fw_fetch(dev_priv, guc_fw);
-	/* status must now be FAIL or SUCCESS */
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 94d369b..70606e6 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -154,18 +154,13 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
  */
 void intel_huc_init(struct drm_i915_private *dev_priv)
 {
-	struct intel_huc *huc = &dev_priv->huc;
-	struct intel_uc_fw *huc_fw = &huc->fw;
+	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
 	const char *fw_path = NULL;
 
-	huc_fw->path = NULL;
 	huc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
 	huc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
 	huc_fw->fw = INTEL_UC_FW_TYPE_HUC;
 
-	if (!HAS_HUC_UCODE(dev_priv))
-		return;
-
 	if (IS_SKYLAKE(dev_priv)) {
 		fw_path = I915_SKL_HUC_UCODE;
 		huc_fw->major_ver_wanted = SKL_HUC_FW_MAJOR;
@@ -178,15 +173,18 @@ void intel_huc_init(struct drm_i915_private *dev_priv)
 		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 (HAS_HUC_UCODE(dev_priv)) {
+		WARN(1, "No HuC firmware known for platform with HuC!\n");
 	}
 
 	huc_fw->path = fw_path;
+
+	/* Early return if we do not have firmware to fetch */
+	if (fw_path == NULL)
+		return;
+
 	huc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
-
 	DRM_DEBUG_DRIVER("HuC firmware pending, path %s\n", fw_path);
-
-	WARN(huc_fw->path == NULL, "HuC present but no fw path\n");
-
 	intel_uc_fw_fetch(dev_priv, huc_fw);
 }
 
-- 
2.9.3

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

Re: [PATCH 1/5] drm/i915/uc: Rename intel_?uc_{setup, load}() to _init_hw()

[Michal Wajdeczko]

On Tue, Feb 14, 2017 at 05:15:37PM +0100, Arkadiusz Hiler wrote:
> GuC historically has two "startup" functions called _init() and _setup()
> 
> Then HuC came with it's _init() and _load().
> 
> To make naming more consistent this commit renames intel_guc_setup() and
> intel_huc_load() to *uc_init_hw() as it it seams more fitting - they are
> called from the i915_gem_init_hw().
> 
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         |  2 +-
>  drivers/gpu/drm/i915/intel_guc_loader.c | 14 +++++++-------
>  drivers/gpu/drm/i915/intel_huc.c        |  6 +++---
>  drivers/gpu/drm/i915/intel_uc.h         |  4 ++--
>  4 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 7129792..5234c76 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4478,7 +4478,7 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
>  	intel_mocs_init_l3cc_table(dev_priv);
>  
>  	/* We can't enable contexts until all firmware is loaded */
> -	ret = intel_guc_setup(dev_priv);
> +	ret = intel_guc_init_hw(dev_priv);
>  	if (ret)
>  		goto out;
>  
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 8ef33d8..f5efe28 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -428,19 +428,19 @@ static int guc_hw_reset(struct drm_i915_private *dev_priv)
>  }
>  
>  /**
> - * intel_guc_setup() - finish preparing the GuC for activity
> + * intel_guc_init_hw() - finish preparing the GuC for activity
>   * @dev_priv:	i915 device private
>   *
> - * Called from gem_init_hw() during driver loading and also after a GPU reset.
> + * 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.
> + * 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_setup(struct drm_i915_private *dev_priv)
> +int intel_guc_init_hw(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>  	const char *fw_path = guc_fw->path;
> @@ -506,7 +506,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>  		if (err)
>  			goto fail;
>  
> -		intel_huc_load(dev_priv);
> +		intel_huc_init_hw(dev_priv);
>  		err = guc_ucode_xfer(dev_priv);
>  		if (!err)
>  			break;
> @@ -732,7 +732,7 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>   * Called early during driver load, but after GEM is initialised.
>   *
>   * The firmware will be transferred to the GuC's memory later,
> - * when intel_guc_setup() is called.
> + * when intel_guc_init_hw() is called.
>   */
>  void intel_guc_init(struct drm_i915_private *dev_priv)

Can we also pick better name for this function?
Hint: it operates mostly on fw struct, so maybe:

	void intel_guc_fw_init(struct drm_i915_private *dev_priv)

Then we can use new intel_guc_init[early]() to hold initialization code that
now is in intel_uc_init_early() which also breaks consistency


>  {
> diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> index c144609..94d369b 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -150,7 +150,7 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
>   * is not capable or driver yet support it. And there will be no error message
>   * for INTEL_UC_FIRMWARE_NONE cases.
>   *
> - * The DMA-copying to HW is done later when intel_huc_load() is called.
> + * The DMA-copying to HW is done later when intel_huc_init_hw() is called.
>   */
>  void intel_huc_init(struct drm_i915_private *dev_priv)
>  {
> @@ -191,7 +191,7 @@ void intel_huc_init(struct drm_i915_private *dev_priv)
>  }
>  
>  /**
> - * intel_huc_load() - load HuC uCode to device
> + * intel_huc_init_hw() - load HuC uCode to device
>   * @dev_priv: the drm_i915_private device
>   *
>   * Called from guc_setup() during driver loading and also after a GPU reset.
> @@ -203,7 +203,7 @@ void intel_huc_init(struct drm_i915_private *dev_priv)
>   *
>   * Return:	non-zero code on error
>   */
> -int intel_huc_load(struct drm_i915_private *dev_priv)
> +int intel_huc_init_hw(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>  	int err;
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index d74f4d3..dd34a1b 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -190,7 +190,7 @@ int intel_guc_sample_forcewake(struct intel_guc *guc);
>  
>  /* intel_guc_loader.c */
>  extern void intel_guc_init(struct drm_i915_private *dev_priv);
> -extern int intel_guc_setup(struct drm_i915_private *dev_priv);
> +extern int intel_guc_init_hw(struct drm_i915_private *dev_priv);
>  extern void intel_guc_fini(struct drm_i915_private *dev_priv);
>  extern const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
>  extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
> @@ -225,7 +225,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>  /* intel_huc.c */
>  void intel_huc_init(struct drm_i915_private *dev_priv);
>  void intel_huc_fini(struct drm_i915_private  *dev_priv);
> -int intel_huc_load(struct drm_i915_private *dev_priv);
> +int intel_huc_init_hw(struct drm_i915_private *dev_priv);

btw, for some functions we are using explicit "extern" (see above)
can we also unify this (one way or another)


Regards,
Michal  

>  void intel_guc_auth_huc(struct drm_i915_private *dev_priv);
>  
>  #endif
> -- 
> 2.9.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915/guc: Extract param logic form guc_init

[Michal Wajdeczko]

On Tue, Feb 14, 2017 at 05:15:39PM +0100, Arkadiusz Hiler wrote:
> Let intel_guc_init() focus on determining and fetching the correct
> firmware.
> 
> This patch introduces intel_sanitize_uc_params() that is called from
> intel_uc_init().
> 
> Then, if we have GuC, we can call intel_guc_init() conditionally and we
> do not have to do internal checks.
> 
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c         |  2 ++
>  drivers/gpu/drm/i915/intel_guc_loader.c | 15 +--------------
>  drivers/gpu/drm/i915/intel_uc.c         | 25 ++++++++++++++++++++++++-
>  3 files changed, 27 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6d0798e..e520895 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -994,6 +994,8 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
>  
>  	i915.semaphores = intel_sanitize_semaphores(dev_priv, i915.semaphores);
>  	DRM_DEBUG_DRIVER("use GPU sempahores? %s\n", yesno(i915.semaphores));
> +
> +	intel_uc_sanitize_params(dev_priv);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index f5efe28..db5713c 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -726,7 +726,7 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>  }
>  
>  /**
> - * intel_guc_init() - define parameters and fetch firmware
> + * intel_guc_init() - determine and fetch firmware

Again, can we have function name that corresponds to its purpose.
Comment alone is not sufficient


>   * @dev_priv:	i915 device private
>   *
>   * Called early during driver load, but after GEM is initialised.
> @@ -739,17 +739,6 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
>  	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>  	const char *fw_path;
>  
> -	if (!HAS_GUC(dev_priv)) {
> -		i915.enable_guc_loading = 0;
> -		i915.enable_guc_submission = 0;
> -	} else {
> -		/* A negative value means "use platform default" */
> -		if (i915.enable_guc_loading < 0)
> -			i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
> -		if (i915.enable_guc_submission < 0)
> -			i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> -	}
> -
>  	if (!HAS_GUC_UCODE(dev_priv)) {
>  		fw_path = NULL;
>  	} else if (IS_SKYLAKE(dev_priv)) {
> @@ -773,8 +762,6 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
>  	guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
>  
>  	/* Early (and silent) return if GuC loading is disabled */

I think this comment was related to the line that you just deleted
 

> -	if (!i915.enable_guc_loading)
> -		return;
>  	if (fw_path == NULL)
>  		return;
>  	if (*fw_path == '\0')
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index d9d0566..d1ca41d 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -25,6 +25,24 @@
>  #include "i915_drv.h"
>  #include "intel_uc.h"
>  
> +void intel_uc_sanitize_params(struct drm_i915_private *dev_priv)
> +{
> +	if (!HAS_GUC(dev_priv)) {

Maybe we should warn user if specified explicit guc params need to be nuked?
 

> +		i915.enable_guc_loading = 0;
> +		i915.enable_guc_submission = 0;
> +	} else {
> +		/* A negative value means "use platform default" */
> +		if (i915.enable_guc_loading < 0)
> +			i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
> +		if (i915.enable_guc_submission < 0)
> +			i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> +	}
> +
> +	/* can't enable guc submission without guc loaded */
> +	if (!i915.enable_guc_loading)
> +		i915.enable_guc_submission = 0;
> +}
> +
>  void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  {
>  	mutex_init(&dev_priv->guc.send_mutex);
> @@ -32,7 +50,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  
>  void intel_uc_init(struct drm_i915_private *dev_priv)
>  {
> -	intel_huc_init(dev_priv);
> +	if (!i915.enable_guc_loading)
> +		return;
> +
> +	if (HAS_HUC_UCODE(dev_priv))

Hmm, if I recall correctly, same condition is checked in huc_init()...
Btw, what approach is more preferred? check before call or inside?


Regards,
Michal   

> +		intel_huc_init(dev_priv);
> +
>  	intel_guc_init(dev_priv);
>  }
>  
> -- 
> 2.9.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915/guc: Simplify intel_guc_init_hw()

[Michal Wajdeczko]

On Tue, Feb 14, 2017 at 05:15:40PM +0100, Arkadiusz Hiler wrote:
> Current version of intel_guc_init_hw() does a lot:
>  - cares about submission
>  - loads huc
>  - implement WA
> 
> This change offloads some of the logic to intel_uc_load(), which now
> cares about the above.
> 
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         |   2 +-
>  drivers/gpu/drm/i915/intel_guc_loader.c | 136 +++-----------------------------
>  drivers/gpu/drm/i915/intel_uc.c         | 105 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uc.h         |   6 ++
>  4 files changed, 124 insertions(+), 125 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 5234c76..afc7e14 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4478,7 +4478,7 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
>  	intel_mocs_init_l3cc_table(dev_priv);
>  
>  	/* We can't enable contexts until all firmware is loaded */
> -	ret = intel_guc_init_hw(dev_priv);
> +	ret = intel_uc_init_hw(dev_priv);
>  	if (ret)
>  		goto out;
>  
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index db5713c..4ea705d 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -91,7 +91,7 @@ const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
>  	}
>  };
>  
> -static void guc_interrupts_release(struct drm_i915_private *dev_priv)
> +void intel_guc_interrupts_release(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
> @@ -109,7 +109,7 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv)
>  	I915_WRITE(GUC_WD_VECS_IER, 0);
>  }
>  
> -static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
> +void intel_guc_interrupts_capture(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
> @@ -409,24 +409,6 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> -static int guc_hw_reset(struct drm_i915_private *dev_priv)
> -{
> -	int ret;
> -	u32 guc_status;
> -
> -	ret = intel_guc_reset(dev_priv);
> -	if (ret) {
> -		DRM_ERROR("GuC reset failed, ret = %d\n", ret);
> -		return ret;
> -	}
> -
> -	guc_status = I915_READ(GUC_STATUS);
> -	WARN(!(guc_status & GS_MIA_IN_RESET),
> -	     "GuC status: 0x%x, MIA core expected to be in reset\n", guc_status);
> -
> -	return ret;
> -}
> -
>  /**
>   * intel_guc_init_hw() - finish preparing the GuC for activity
>   * @dev_priv:	i915 device private
> @@ -444,147 +426,53 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>  	const char *fw_path = guc_fw->path;
> -	int retries, ret, err;
> +	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));
>  
> -	/* Loading forbidden, or no firmware to load? */
> -	if (!i915.enable_guc_loading) {
> -		err = 0;
> -		goto fail;
> -	} else if (fw_path == NULL) {
> +	if (fw_path == NULL) {
>  		/* Device is known to have no uCode (e.g. no GuC) */
> -		err = -ENXIO;
> -		goto fail;
> +		return -ENXIO;
>  	} else if (*fw_path == '\0') {
>  		/* Device has a GuC but we don't know what f/w to load? */
>  		WARN(1, "No GuC firmware known for this platform!\n");
> -		err = -ENODEV;
> -		goto fail;
> +		return -ENODEV;
>  	}
>  
>  	/* Fetch failed, or already fetched but failed to load? */
>  	if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS) {
> -		err = -EIO;
> -		goto fail;
> +		return -EIO;
>  	} else if (guc_fw->load_status == INTEL_UC_FIRMWARE_FAIL) {
> -		err = -ENOEXEC;
> -		goto fail;
> +		return -ENOEXEC;
>  	}
>  
> -	guc_interrupts_release(dev_priv);
> -	gen9_reset_guc_interrupts(dev_priv);
> -
> -	/* We need to notify the guc whenever we change the GGTT */
> -	i915_ggtt_enable_guc(dev_priv);
> -
>  	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));
>  
> -	err = i915_guc_submission_init(dev_priv);
> -	if (err)
> -		goto fail;
> -
>  	/*
>  	 * WaEnableuKernelHeaderValidFix:skl,bxt
>  	 * For BXT, this is only upto B0 but below WA is required for later
>  	 * steppings also so this is extended as well.
>  	 */
> -	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> -	for (retries = 3; ; ) {
> -		/*
> -		 * Always reset the GuC just before (re)loading, so
> -		 * that the state and timing are fairly predictable
> -		 */
> -		err = guc_hw_reset(dev_priv);
> -		if (err)
> -			goto fail;
> +	ret = guc_ucode_xfer(dev_priv);
>  
> -		intel_huc_init_hw(dev_priv);
> -		err = guc_ucode_xfer(dev_priv);
> -		if (!err)
> -			break;
> -
> -		if (--retries == 0)
> -			goto fail;
> -
> -		DRM_INFO("GuC fw load failed: %d; will reset and "
> -			 "retry %d more time(s)\n", err, retries);
> -	}
> +	if (ret)
> +		return -EAGAIN;
>  
>  	guc_fw->load_status = INTEL_UC_FIRMWARE_SUCCESS;
>  
> -	intel_guc_auth_huc(dev_priv);
> -
> -	if (i915.enable_guc_submission) {
> -		if (i915.guc_log_level >= 0)
> -			gen9_enable_guc_interrupts(dev_priv);
> -
> -		err = i915_guc_submission_enable(dev_priv);
> -		if (err)
> -			goto fail;
> -		guc_interrupts_capture(dev_priv);
> -	}
> -
>  	DRM_INFO("GuC %s (firmware %s [version %u.%u])\n",
>  		 i915.enable_guc_submission ? "submission enabled" : "loaded",
>  		 guc_fw->path,
>  		 guc_fw->major_ver_found, guc_fw->minor_ver_found);
>  
>  	return 0;
> -
> -fail:
> -	if (guc_fw->load_status == INTEL_UC_FIRMWARE_PENDING)
> -		guc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;
> -
> -	guc_interrupts_release(dev_priv);
> -	i915_guc_submission_disable(dev_priv);
> -	i915_guc_submission_fini(dev_priv);
> -	i915_ggtt_disable_guc(dev_priv);
> -
> -	/*
> -	 * We've failed to load the firmware :(
> -	 *
> -	 * Decide whether to disable GuC submission and fall back to
> -	 * execlist mode, and whether to hide the error by returning
> -	 * zero or to return -EIO, which the caller will treat as a
> -	 * nonfatal error (i.e. it doesn't prevent driver load, but
> -	 * marks the GPU as wedged until reset).
> -	 */
> -	if (i915.enable_guc_loading > 1) {
> -		ret = -EIO;
> -	} else if (i915.enable_guc_submission > 1) {
> -		ret = -EIO;
> -	} else {
> -		ret = 0;
> -	}
> -
> -	if (err == 0 && !HAS_GUC_UCODE(dev_priv))
> -		;	/* Don't mention the GuC! */
> -	else if (err == 0)
> -		DRM_INFO("GuC firmware load skipped\n");
> -	else if (ret != -EIO)
> -		DRM_NOTE("GuC firmware load failed: %d\n", err);
> -	else
> -		DRM_WARN("GuC firmware load failed: %d\n", err);
> -
> -	if (i915.enable_guc_submission) {
> -		if (fw_path == NULL)
> -			DRM_INFO("GuC submission without firmware not supported\n");
> -		if (ret == 0)
> -			DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> -		else
> -			DRM_ERROR("GuC init failed: %d\n", ret);
> -	}
> -	i915.enable_guc_submission = 0;
> -
> -	return ret;
>  }
>  
>  void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> @@ -782,7 +670,7 @@ void intel_guc_fini(struct drm_i915_private *dev_priv)
>  	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>  
>  	mutex_lock(&dev_priv->drm.struct_mutex);
> -	guc_interrupts_release(dev_priv);
> +	intel_guc_interrupts_release(dev_priv);
>  	i915_guc_submission_disable(dev_priv);
>  	i915_guc_submission_fini(dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index d1ca41d..bba220c 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -25,6 +25,25 @@
>  #include "i915_drv.h"
>  #include "intel_uc.h"
>  
> +static int uc_hw_reset(struct drm_i915_private *dev_priv)

Do you plan to put more resets here?
If this is only for Guc then I would stay with old name

> +{
> +	int ret;
> +	u32 guc_status;
> +
> +	ret = intel_guc_reset(dev_priv);
> +	if (ret) {
> +		DRM_ERROR("GuC reset failed, ret = %d\n", ret);
> +		return ret;
> +	}
> +
> +	guc_status = I915_READ(GUC_STATUS);
> +	WARN(!(guc_status & GS_MIA_IN_RESET),
> +	     "GuC status: 0x%x, MIA core expected to be in reset\n",
> +	     guc_status);
> +
> +	return ret;
> +}
> +
>  void intel_uc_sanitize_params(struct drm_i915_private *dev_priv)
>  {
>  	if (!HAS_GUC(dev_priv)) {
> @@ -59,6 +78,92 @@ void intel_uc_init(struct drm_i915_private *dev_priv)
>  	intel_guc_init(dev_priv);
>  }
>  
> +int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> +{
> +	int ret, retries;
> +
> +	/* guc not enabled, nothing to do */
> +	if (!i915.enable_guc_loading)
> +		return 0;
> +
> +	intel_guc_interrupts_release(dev_priv);
> +	gen9_reset_guc_interrupts(dev_priv);
> +
> +	/* We need to notify the guc whenever we change the GGTT */
> +	i915_ggtt_enable_guc(dev_priv);
> +
> +	if (i915.enable_guc_submission) {

Hmm, if I'm not wrong same condition is inside _init() function...


> +		ret = i915_guc_submission_init(dev_priv);
> +		if (ret)
> +			goto fail;
> +	}
> +
> +	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> +	retries = GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS;
> +	while (retries--) {
> +		/*
> +		 * Always reset the GuC just before (re)loading, so
> +		 * that the state and timing are fairly predictable
> +		 */
> +		ret = uc_hw_reset(dev_priv);
> +		if (ret)
> +			goto fail;
> +
> +		intel_huc_init_hw(dev_priv);
> +		ret = intel_guc_init_hw(dev_priv);
> +		if (ret == 0 || ret != -EAGAIN)
> +			break;
> +
> +		DRM_INFO("GuC fw load failed: %d; will reset and "
> +			 "retry %d more time(s)\n", ret, retries);
> +	}
> +
> +	/* did we succeded or run out of retries? */
> +	if (ret)
> +		goto fail;
> +
> +	intel_guc_auth_huc(dev_priv);
> +	if (i915.enable_guc_submission) {
> +		if (i915.guc_log_level >= 0)
> +			gen9_enable_guc_interrupts(dev_priv);
> +
> +		ret = i915_guc_submission_enable(dev_priv);
> +		if (ret)
> +			goto fail;
> +		intel_guc_interrupts_capture(dev_priv);
> +	}
> +
> +	return 0;
> +
> +fail:
> +	/*
> +	 * We've failed to load the firmware :(
> +	 *
> +	 * Decide whether to disable GuC submission and fall back to
> +	 * execlist mode, and whether to hide the error by returning
> +	 * zero or to return -EIO, which the caller will treat as a
> +	 * nonfatal error (i.e. it doesn't prevent driver load, but
> +	 * marks the GPU as wedged until reset).
> +	 */
> +	DRM_ERROR("GuC init failed\n");
> +	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
> +		ret = -EIO;
> +	else
> +		ret = 0;
> +
> +	if (i915.enable_guc_submission) {
> +		i915.enable_guc_submission = 0;
> +		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> +	}
> +
> +	i915_ggtt_disable_guc(dev_priv);
> +	intel_guc_interrupts_release(dev_priv);
> +	i915_guc_submission_disable(dev_priv);
> +	i915_guc_submission_fini(dev_priv);
> +
> +	return ret;
> +}
> +
>  /*
>   * Read GuC command/status register (SOFT_SCRATCH_0)
>   * Return true if it contains a response rather than a command
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 264761a..fbe774e 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -29,6 +29,8 @@
>  #include "intel_ringbuffer.h"
>  
>  #include "i915_vma.h"
> +/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> +#define GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS 3

Typo


Regards,
Michal


>  
>  struct drm_i915_gem_request;
>  
> @@ -184,8 +186,10 @@ struct intel_huc {
>  };
>  
>  /* intel_uc.c */
> +void intel_uc_sanitize_params(struct drm_i915_private *dev_priv);
>  void intel_uc_init_early(struct drm_i915_private *dev_priv);
>  void intel_uc_init(struct drm_i915_private *dev_priv);
> +int intel_uc_init_hw(struct drm_i915_private *dev_priv);
>  int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
>  int intel_guc_sample_forcewake(struct intel_guc *guc);
>  
> @@ -198,6 +202,8 @@ extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
>  extern int intel_guc_resume(struct drm_i915_private *dev_priv);
>  void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>  	struct intel_uc_fw *uc_fw);
> +void intel_guc_interrupts_release(struct drm_i915_private *dev_priv);
> +void intel_guc_interrupts_capture(struct drm_i915_private *dev_priv);
>  u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
>  
>  /* i915_guc_submission.c */
> -- 
> 2.9.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915/uc: Simplify fw_path

[Michal Wajdeczko]

On Tue, Feb 14, 2017 at 05:15:41PM +0100, Arkadiusz Hiler wrote:
> Currently fw_path values can represent one of three possible states:
> 
>  1) NULL - device without the uC
>  2) '\0' - device with the uC but have no firmware
>  3) else - device with the uC and we have firmware
> 
> Second case is used only to WARN at a later stage.
> 
> We can WARN right away and merge cases 1 and 2.
> 
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_loader.c | 28 +++++++++++-----------------
>  drivers/gpu/drm/i915/intel_huc.c        | 18 ++++++++----------
>  2 files changed, 19 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 4ea705d..f4875c2 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -434,12 +434,8 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
>  		intel_uc_fw_status_repr(guc_fw->load_status));
>  
>  	if (fw_path == NULL) {
> -		/* Device is known to have no uCode (e.g. no GuC) */
> +		/* We do not have uCode for the device */
>  		return -ENXIO;
> -	} else if (*fw_path == '\0') {
> -		/* Device has a GuC but we don't know what f/w to load? */
> -		WARN(1, "No GuC firmware known for this platform!\n");
> -		return -ENODEV;
>  	}
>  
>  	/* Fetch failed, or already fetched but failed to load? */
> @@ -625,11 +621,13 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>  void intel_guc_init(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
> -	const char *fw_path;
> +	const char *fw_path = NULL;

Do we still need this ? Maybe we can directly use guc_fw->path?

>  
> -	if (!HAS_GUC_UCODE(dev_priv)) {
> -		fw_path = NULL;
> -	} else if (IS_SKYLAKE(dev_priv)) {
> +	guc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
> +	guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
> +	guc_fw->fw = INTEL_UC_FW_TYPE_GUC;
> +
> +	if (IS_SKYLAKE(dev_priv)) {
>  		fw_path = I915_SKL_GUC_UCODE;
>  		guc_fw->major_ver_wanted = SKL_FW_MAJOR;
>  		guc_fw->minor_ver_wanted = SKL_FW_MINOR;
> @@ -641,24 +639,20 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
>  		fw_path = I915_KBL_GUC_UCODE;
>  		guc_fw->major_ver_wanted = KBL_FW_MAJOR;
>  		guc_fw->minor_ver_wanted = KBL_FW_MINOR;
> -	} else {
> -		fw_path = "";	/* unknown device */
> +	} else if (HAS_GUC_UCODE(dev_priv)) {
> +		WARN(1, "No GuC firmware known for platform with GuC!\n");
> +		i915.enable_guc_loading = 0;

Can we return here ?

>  	}
>  
>  	guc_fw->path = fw_path;
> -	guc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
> -	guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
>  
> -	/* Early (and silent) return if GuC loading is disabled */
> +	/* Early return if we do not have firmware to fetch */
>  	if (fw_path == NULL)
>  		return;
> -	if (*fw_path == '\0')
> -		return;
>  
>  	guc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
>  	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);

Above two lines should be part of fw_fetch() function called below.
Will cover Huc case as well.

>  	intel_uc_fw_fetch(dev_priv, guc_fw);
> -	/* status must now be FAIL or SUCCESS */
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> index 94d369b..70606e6 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -154,18 +154,13 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
>   */
>  void intel_huc_init(struct drm_i915_private *dev_priv)
>  {
> -	struct intel_huc *huc = &dev_priv->huc;
> -	struct intel_uc_fw *huc_fw = &huc->fw;
> +	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>  	const char *fw_path = NULL;
>  
> -	huc_fw->path = NULL;
>  	huc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
>  	huc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
>  	huc_fw->fw = INTEL_UC_FW_TYPE_HUC;
>  
> -	if (!HAS_HUC_UCODE(dev_priv))
> -		return;
> -
>  	if (IS_SKYLAKE(dev_priv)) {
>  		fw_path = I915_SKL_HUC_UCODE;
>  		huc_fw->major_ver_wanted = SKL_HUC_FW_MAJOR;
> @@ -178,15 +173,18 @@ void intel_huc_init(struct drm_i915_private *dev_priv)
>  		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 (HAS_HUC_UCODE(dev_priv)) {
> +		WARN(1, "No HuC firmware known for platform with HuC!\n");

Can we simplify this into:

	} else {
		WARN(HAS_HUC_UCODE(dev_priv), "No HuC firmware known for platform with HuC!\n");
		return;

Regards,
Michal


>  	}
>  
>  	huc_fw->path = fw_path;
> +
> +	/* Early return if we do not have firmware to fetch */
> +	if (fw_path == NULL)
> +		return;
> +
>  	huc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
> -
>  	DRM_DEBUG_DRIVER("HuC firmware pending, path %s\n", fw_path);
> -
> -	WARN(huc_fw->path == NULL, "HuC present but no fw path\n");
> -
>  	intel_uc_fw_fetch(dev_priv, huc_fw);
>  }
>  
> -- 
> 2.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for GuC Scrub vol. 1 (rev2)

[Patchwork]

== Series Details ==

Series: GuC Scrub vol. 1 (rev2)
URL   : https://patchwork.freedesktop.org/series/16856/
State : success

== Summary ==

Series 16856v2 GuC Scrub vol. 1
https://patchwork.freedesktop.org/api/1.0/series/16856/revisions/2/mbox/

fi-bdw-5557u     total:252  pass:241  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:252  pass:213  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:252  pass:225  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:252  pass:221  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:252  pass:202  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:252  pass:235  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:252  pass:230  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:252  pass:224  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:252  pass:223  dwarn:0   dfail:0   fail:0   skip:29 

68a5f12a5efed9f3e52bc9a3983c2837e4a2ea3e drm-tip: 2017y-02m-14d-19h-35m-15s UTC integration manifest
57addb0 drm/i915/uc: Simplify fw_path
9095210 drm/i915/guc: Simplify intel_guc_init_hw()
447bbeb drm/i915/guc: Extract param logic form guc_init
a793d9d drm/i915/uc: Introduce intel_uc_init()
36e95de drm/i915/uc: Rename intel_?uc_{setup, load}() to _init_hw()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3812/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/uc: Rename intel_?uc_{setup, load}() to _init_hw()

[Chris Wilson]

On Tue, Feb 14, 2017 at 05:15:37PM +0100, Arkadiusz Hiler wrote:
> GuC historically has two "startup" functions called _init() and _setup()
> 
> Then HuC came with it's _init() and _load().
> 
> To make naming more consistent this commit renames intel_guc_setup() and
> intel_huc_load() to *uc_init_hw() as it it seams more fitting - they are
> called from the i915_gem_init_hw().

+The aim is to be consistent in that entry points called during
particular driver init phases (e.g. init_hw) are all suffixed by that
phase. When reading the leaf functions, it should be clear at what stage
during the driver load it is called and therefore what operations are
legal at that point.

(If we repeat that enough times in the changelogs we start believing it
ourselves ;)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/uc: Rename intel_?uc_{setup, load}() to _init_hw()

[Joonas Lahtinen]

On ti, 2017-02-14 at 17:15 +0100, Arkadiusz Hiler wrote:
> GuC historically has two "startup" functions called _init() and _setup()
> 
> Then HuC came with it's _init() and _load().
> 
> To make naming more consistent this commit renames intel_guc_setup() and
> intel_huc_load() to *uc_init_hw() as it it seams more fitting - they are
> called from the i915_gem_init_hw().
> 
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>

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

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915/guc: Extract param logic form guc_init

[Joonas Lahtinen]

On ti, 2017-02-14 at 20:37 +0100, Michal Wajdeczko wrote:
> On Tue, Feb 14, 2017 at 05:15:39PM +0100, Arkadiusz Hiler wrote:
> > 
> > Let intel_guc_init() focus on determining and fetching the correct
> > firmware.
> > 
> > This patch introduces intel_sanitize_uc_params() that is called from
> > intel_uc_init().
> > 
> > Then, if we have GuC, we can call intel_guc_init() conditionally and we
> > do not have to do internal checks.
> > 
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>

<SNIP>

> > @@ -726,7 +726,7 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> >  }
> >  
> >  /**
> > - * intel_guc_init() - define parameters and fetch firmware
> > + * intel_guc_init() - determine and fetch firmware
> 
> Again, can we have function name that corresponds to its purpose.
> Comment alone is not sufficient

I second Michal here, the naming of the functions is rather poor.
Something like intel_guc_fetch_firmware would be more descriptive.

Making it overly generic when we don't have other use yet, will cause
more churn when we get more use (because fortune telling was never our
thing). I'd rather see rather standalone functions, which are then be
called from central point where more can be added, without changing the
existing ones. So this applies to intel_uc_init, too.

It'll be easier to add these generic functions iff we start repeating
calls to a bunch of functions, and we can then see patterns emerging.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915/uc: Simplify fw_path

[Arkadiusz Hiler]

On Tue, Feb 14, 2017 at 09:23:33PM +0100, Michal Wajdeczko wrote:
> On Tue, Feb 14, 2017 at 05:15:41PM +0100, Arkadiusz Hiler wrote:
> > Currently fw_path values can represent one of three possible states:
> > 
> >  1) NULL - device without the uC
> >  2) '\0' - device with the uC but have no firmware
> >  3) else - device with the uC and we have firmware
> > 
> > Second case is used only to WARN at a later stage.
> > 
> > We can WARN right away and merge cases 1 and 2.

[ snip ]

> >  /**
> > diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> > index 94d369b..70606e6 100644
> > --- a/drivers/gpu/drm/i915/intel_huc.c
> > +++ b/drivers/gpu/drm/i915/intel_huc.c
> > @@ -154,18 +154,13 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
> >   */
> >  void intel_huc_init(struct drm_i915_private *dev_priv)
> >  {
> > -	struct intel_huc *huc = &dev_priv->huc;
> > -	struct intel_uc_fw *huc_fw = &huc->fw;
> > +	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
> >  	const char *fw_path = NULL;
> >  
> > -	huc_fw->path = NULL;
> >  	huc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
> >  	huc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
> >  	huc_fw->fw = INTEL_UC_FW_TYPE_HUC;
> >  
> > -	if (!HAS_HUC_UCODE(dev_priv))
> > -		return;
> > -
> >  	if (IS_SKYLAKE(dev_priv)) {
> >  		fw_path = I915_SKL_HUC_UCODE;
> >  		huc_fw->major_ver_wanted = SKL_HUC_FW_MAJOR;
> > @@ -178,15 +173,18 @@ void intel_huc_init(struct drm_i915_private *dev_priv)
> >  		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 (HAS_HUC_UCODE(dev_priv)) {
> > +		WARN(1, "No HuC firmware known for platform with HuC!\n");
> 
> Can we simplify this into:
> 
> 	} else {
> 		WARN(HAS_HUC_UCODE(dev_priv), "No HuC firmware known for platform with HuC!\n");
> 		return;

We can even simplify it further dropping the conditional on `HAS_?_UCODE`.

The function is called only when we have the UCODE - and the macro will
always be true here.

Reset of the feedback incorporated. Thanks!


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

Re: [PATCH 3/5] drm/i915/guc: Extract param logic form guc_init

[Arkadiusz Hiler]

On Wed, Feb 15, 2017 at 02:48:59PM +0200, Joonas Lahtinen wrote:
> On ti, 2017-02-14 at 20:37 +0100, Michal Wajdeczko wrote:
> > On Tue, Feb 14, 2017 at 05:15:39PM +0100, Arkadiusz Hiler wrote:
> > > 
> > > Let intel_guc_init() focus on determining and fetching the correct
> > > firmware.
> > > 
> > > This patch introduces intel_sanitize_uc_params() that is called from
> > > intel_uc_init().
> > > 
> > > Then, if we have GuC, we can call intel_guc_init() conditionally and we
> > > do not have to do internal checks.
> > > 
> > > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> 
> <SNIP>
> 
> > > @@ -726,7 +726,7 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> > >  }
> > >  
> > >  /**
> > > - * intel_guc_init() - define parameters and fetch firmware
> > > + * intel_guc_init() - determine and fetch firmware
> > 
> > Again, can we have function name that corresponds to its purpose.
> > Comment alone is not sufficient
> 
> I second Michal here, the naming of the functions is rather poor.
> Something like intel_guc_fetch_firmware would be more descriptive.
> 
> Making it overly generic when we don't have other use yet, will cause
> more churn when we get more use (because fortune telling was never our
> thing). I'd rather see rather standalone functions, which are then be
> called from central point where more can be added, without changing the
> existing ones. So this applies to intel_uc_init, too.
> 
> It'll be easier to add these generic functions iff we start repeating
> calls to a bunch of functions, and we can then see patterns emerging.

So, couple of possibilities I see:

First:
  a) rename intel_?uc_init to intel_?uc_fw_fetch
  b) embed dev_priv in intel_uc_fw
  c) make them take intel_uc_fw struct only

  intel_uc_fw_ corresponds to struct we have, so it seems better fitting
  than what Joonas proposed.

  now we have intel_uc_fw_fetch that does the actual fetching, which is
  confusing, but we can rename it to something like
  intel_uc_fw_perform_fetch or something similar.


Second:
  Go with what we have and rename it intel_?uc_fw_init as Michal
  proposed.


What do you think?

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