All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] drm/i915/guc : Removing enable_guc_loading module
@ 2017-08-23 22:11 Sujaritha Sundaresan
  2017-08-23 22:25 ` Srivatsa, Anusha
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Sujaritha Sundaresan @ 2017-08-23 22:11 UTC (permalink / raw)
  To: intel-gfx; +Cc: Joonas Lahtinen, Sujaritha Sundaresan

Whenever we need i915.enable_guc_submission=1, we also need enable_guc_loading=1. We also need enable_guc_loading=1 when we want to verify the HuC, which is every time we have a HuC (but all platforms with HuC have a GuC and viceversa).
We don't need the user to tell when to enable the GuC loading

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@intel.com>
Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c     | 12 +++++--
 drivers/gpu/drm/i915/i915_drv.c         |  2 +-
 drivers/gpu/drm/i915/i915_drv.h         | 10 +++---
 drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c     |  2 +-
 drivers/gpu/drm/i915/i915_irq.c         |  2 +-
 drivers/gpu/drm/i915/i915_params.c      |  6 ----
 drivers/gpu/drm/i915/i915_params.h      |  1 -
 drivers/gpu/drm/i915/intel_guc_loader.c | 48 +++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_huc.c        |  4 ++-
 drivers/gpu/drm/i915/intel_uc.c         | 58 ++++++++-------------------------
 drivers/gpu/drm/i915/intel_uc.h         |  4 +--
 drivers/gpu/drm/i915/intel_uncore.c     |  4 +--
 13 files changed, 80 insertions(+), 75 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 329fb36..7cc53c2 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2338,8 +2338,11 @@ static int i915_huc_load_status_info(struct seq_file *m, void *data)
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
 	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
 
-	if (!HAS_HUC_UCODE(dev_priv))
+	if (!HAS_GUC(dev_priv)){
+		seq_puts(m, "No HuC support in HW\n");
 		return 0;
+	}
+		
 
 	seq_puts(m, "HuC firmware status:\n");
 	seq_printf(m, "\tpath: %s\n", huc_fw->path);
@@ -2371,8 +2374,11 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
 	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
 	u32 tmp, i;
 
-	if (!HAS_GUC_UCODE(dev_priv))
+	if (!HAS_GUC(dev_priv)){
+		seq_puts(m, "No GuC supprot in HW\n");
 		return 0;
+	}
+		
 
 	seq_printf(m, "GuC firmware status:\n");
 	seq_printf(m, "\tpath: %s\n",
@@ -2471,7 +2477,7 @@ static bool check_guc_submission(struct seq_file *m)
 
 	if (!guc->execbuf_client) {
 		seq_printf(m, "GuC submission %s\n",
-			   HAS_GUC_SCHED(dev_priv) ?
+			   HAS_GUC(dev_priv) ?
 			   "disabled" :
 			   "not supported");
 		return false;
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 25de4a9..00594dc 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1046,7 +1046,7 @@ 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 semaphores? %s\n", yesno(i915.semaphores));
 
-	intel_uc_sanitize_options(dev_priv);
+	intel_guc_sanitize_submission(dev_priv);
 
 	intel_gvt_sanitize_options(dev_priv);
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 907603c..8e18c67 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3064,11 +3064,13 @@ static inline struct scatterlist *__sg_next(struct scatterlist *sg)
  * command submission once loaded. But these are logically independent
  * properties, so we have separate macros to test them.
  */
-#define HAS_GUC(dev_priv)	((dev_priv)->info.has_guc)
 #define HAS_GUC_CT(dev_priv)	((dev_priv)->info.has_guc_ct)
-#define HAS_GUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
-#define HAS_GUC_SCHED(dev_priv)	(HAS_GUC(dev_priv))
-#define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
+#define HAS_GUC(dev_priv)			((dev_priv)->info.has.guc)
+#define HAS_GUC_UCODE(dev_priv)		((dev_priv)->guc.fw.path != NULL)
+#define HAS_HUC_UCODE(dev_priv)		((dev_priv)->huc.fw.path != NULL)
+#define NEEDS_GUC_LOADING(dev_priv) \
+	(HAS_GUC(dev_priv) && \
+	(i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))
 
 #define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)->info.has_resource_streamer)
 
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index ed91ac8..5166a43 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -361,7 +361,7 @@ static u32 default_desc_template(const struct drm_i915_private *i915,
 	 * present or not in use we still need a small bias as ring wraparound
 	 * at offset 0 sometimes hangs. No idea why.
 	 */
-	if (HAS_GUC(dev_priv) && i915.enable_guc_loading)
+	if (NEEDS_GUC_LOADING(dev_priv))
 		ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
 	else
 		ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 10aa776..236b759 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2996,7 +2996,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv)
 	 * currently don't have any bits spare to pass in this upper
 	 * restriction!
 	 */
-	if (HAS_GUC(dev_priv) && i915.enable_guc_loading) {
+	if (NEEDS_GUC_LOADING(dev_priv)) {
 		ggtt->base.total = min_t(u64, ggtt->base.total, GUC_GGTT_TOP);
 		ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total);
 	}
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 196caa4..38bc184 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4139,7 +4139,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 	for (i = 0; i < MAX_L3_SLICES; ++i)
 		dev_priv->l3_parity.remap_info[i] = NULL;
 
-	if (HAS_GUC_SCHED(dev_priv))
+	if (NEEDS_GUC_LOADING(dev_priv))
 		dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
 
 	/* Let's track the enabled rps events */
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 14e2c2e..cc0dfca 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -56,7 +56,6 @@ struct i915_params i915 __read_mostly = {
 	.verbose_state_checks = 1,
 	.nuclear_pageflip = 0,
 	.edp_vswing = 0,
-	.enable_guc_loading = 0,
 	.enable_guc_submission = 0,
 	.guc_log_level = -1,
 	.guc_firmware_path = NULL,
@@ -218,11 +217,6 @@ struct i915_params i915 __read_mostly = {
 		 "(0=use value from vbt [default], 1=low power swing(200mV),"
 		 "2=default swing(400mV))");
 
-module_param_named_unsafe(enable_guc_loading, i915.enable_guc_loading, int, 0400);
-MODULE_PARM_DESC(enable_guc_loading,
-		"Enable GuC firmware loading "
-		"(-1=auto, 0=never [default], 1=if available, 2=required)");
-
 module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, int, 0400);
 MODULE_PARM_DESC(enable_guc_submission,
 		"Enable GuC submission "
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index febbfdb..b967c48 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -43,7 +43,6 @@
 	func(int, disable_power_well); \
 	func(int, enable_ips); \
 	func(int, invert_brightness); \
-	func(int, enable_guc_loading); \
 	func(int, enable_guc_submission); \
 	func(int, guc_log_level); \
 	func(char *, guc_firmware_path); \
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 8b0ae7f..673d67a 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -375,13 +375,49 @@ int intel_guc_init_hw(struct intel_guc *guc)
 	return 0;
 }
 
+void intel_guc_sanitize_submission(struct drm_i915_private *dev_priv)
+{
+	/* Verify hardware support */
+	if (!HAS_GUC(dev_priv)){
+		if (i915.enable_guc_submission > 0)
+				DRM_INFO("Ignoring GuC submission enable, no HW\n");
+		i915.enable_guc_submission = 0;
+		return;
+	}
+
+	/* Verify firmware support */
+	if (!HAS_GUC_UCODE(dev_priv)){
+		if (i915.enable_guc_submission == 1){
+			DRM_INFO("Ignoring GuC submission enable, no FW\n");
+			i915.enable_guc_submission = 0;
+			return;
+		}
+
+		if (i915.enable_guc_submission < 0){
+			i915.enable_guc_submission = 0;
+			return;
+		}
+
+		/* 
+		 * If "required"(> 1), let it continue and we will fail later 
+		 * due to lack of firmware
+		 */
+	}
+
+	/*
+	 * A negative value means "use platform default" (enabled if we have
+	 * survived to get here)
+	 */
+	if (i915.enable_guc_submission < 0)
+		i915.enable_guc_submission = 1;
+}
+
+
 /**
  * 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)
+void intel_guc_select_fw(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
@@ -411,9 +447,9 @@ int intel_guc_select_fw(struct intel_guc *guc)
 		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;
+		if (HAS_GUC(dev_priv))
+			DRM_ERROR("No GuC FW known for a platform with GuC!\n");
+		return;
 	}
 
-	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 6145fa0..bf831a77 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -176,7 +176,9 @@ void intel_huc_select_fw(struct intel_huc *huc)
 		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");
+		/* For now, everything with a GuC also has a HuC */
+		if (HAS_GUC(dev_priv))
+			DRM_ERROR("No HuC FW known for a platform with HuC!\n");
 		return;
 	}
 }
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 27e072c..d8fab3a 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -60,40 +60,6 @@ static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
-{
-	if (!HAS_GUC(dev_priv)) {
-		if (i915.enable_guc_loading > 0 ||
-		    i915.enable_guc_submission > 0)
-			DRM_INFO("Ignoring GuC options, no hardware\n");
-
-		i915.enable_guc_loading = 0;
-		i915.enable_guc_submission = 0;
-		return;
-	}
-
-	/* A negative value means "use platform default" */
-	if (i915.enable_guc_loading < 0)
-		i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
-
-	/* Verify firmware version */
-	if (i915.enable_guc_loading) {
-		if (HAS_HUC_UCODE(dev_priv))
-			intel_huc_select_fw(&dev_priv->huc);
-
-		if (intel_guc_select_fw(&dev_priv->guc))
-			i915.enable_guc_loading = 0;
-	}
-
-	/* Can't enable guc submission without guc loaded */
-	if (!i915.enable_guc_loading)
-		i915.enable_guc_submission = 0;
-
-	/* A negative value means "use platform default" */
-	if (i915.enable_guc_submission < 0)
-		i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
-}
-
 static void guc_write_irq_trigger(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -106,6 +72,8 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
 	struct intel_guc *guc = &dev_priv->guc;
 
 	intel_guc_ct_init_early(&guc->ct);
+	intel_guc_select_fw(&dev_priv->guc);
+	intel_huc_select_fw(&dev_priv->huc);
 
 	mutex_init(&guc->send_mutex);
 	guc->send = intel_guc_send_nop;
@@ -252,6 +220,8 @@ static void fetch_uc_fw(struct drm_i915_private *dev_priv,
 
 void intel_uc_init_fw(struct drm_i915_private *dev_priv)
 {
+	if(!HAS_GUC(dev_priv))
+		return;
 	fetch_uc_fw(dev_priv, &dev_priv->huc.fw);
 	fetch_uc_fw(dev_priv, &dev_priv->guc.fw);
 }
@@ -333,7 +303,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	struct intel_guc *guc = &dev_priv->guc;
 	int ret, attempts;
 
-	if (!i915.enable_guc_loading)
+	if (!NEEDS_GUC_LOADING(dev_priv))
 		return 0;
 
 	guc_disable_communication(guc);
@@ -423,18 +393,16 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	i915_ggtt_disable_guc(dev_priv);
 
 	DRM_ERROR("GuC init failed\n");
-	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
+	if (i915.enable_guc_submission > 1) {
+		DRM_NOTE("GuC is required, so marking the GPU as wedged\n");	
 		ret = -EIO;
-	else
-		ret = 0;
 
-	if (i915.enable_guc_submission) {
-		i915.enable_guc_submission = 0;
+	} else if (i915.enable_guc_submission == 1) {
 		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
-	}
-
-	i915.enable_guc_loading = 0;
-	DRM_NOTE("GuC firmware loading disabled\n");
+			i915.enable_guc_submission = 0;
+			ret = 0;
+		} else
+			ret = 0;
 
 	return ret;
 }
@@ -443,7 +411,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 {
 	guc_free_load_err_log(&dev_priv->guc);
 
-	if (!i915.enable_guc_loading)
+	if (!NEEDS_GUC_LOADING(dev_priv))
 		return;
 
 	if (i915.enable_guc_submission)
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 22ae52b..7370b74 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -220,7 +220,6 @@ struct intel_huc {
 };
 
 /* intel_uc.c */
-void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
 void intel_uc_init_fw(struct drm_i915_private *dev_priv);
 void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
@@ -241,13 +240,14 @@ static inline void intel_guc_notify(struct intel_guc *guc)
 }
 
 /* intel_guc_loader.c */
-int intel_guc_select_fw(struct intel_guc *guc);
+void intel_guc_select_fw(struct intel_guc *guc);
 int intel_guc_init_hw(struct intel_guc *guc);
 int intel_guc_suspend(struct drm_i915_private *dev_priv);
 int intel_guc_resume(struct drm_i915_private *dev_priv);
 u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
 
 /* i915_guc_submission.c */
+void intel_guc_sanitize_submission(struct drm_i915_private *dev_priv);
 int i915_guc_submission_init(struct drm_i915_private *dev_priv);
 int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
 int i915_guc_wq_reserve(struct drm_i915_gem_request *rq);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index deb4430..6ef8069 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1734,12 +1734,10 @@ int intel_guc_reset(struct drm_i915_private *dev_priv)
 {
 	int ret;
 
-	if (!HAS_GUC(dev_priv))
-		return -EINVAL;
+	GEM_BUG_ON(!HAS_GUC(dev_priv));
 
 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
 	ret = gen6_hw_domain_reset(dev_priv, GEN9_GRDOM_GUC);
-	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 
 	return ret;
 }
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] drm/i915/guc : Removing enable_guc_loading module
  2017-08-23 22:11 [PATCH 1/2] drm/i915/guc : Removing enable_guc_loading module Sujaritha Sundaresan
@ 2017-08-23 22:25 ` Srivatsa, Anusha
  2017-08-24 12:49 ` Michal Wajdeczko
  2017-08-25 21:32 ` Daniele Ceraolo Spurio
  2 siblings, 0 replies; 4+ messages in thread
From: Srivatsa, Anusha @ 2017-08-23 22:25 UTC (permalink / raw)
  To: intel-gfx; +Cc: Lahtinen, Joonas, Sundaresan, Sujaritha



>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
>Sujaritha Sundaresan
>Sent: Wednesday, August 23, 2017 3:12 PM
>To: intel-gfx@lists.freedesktop.org
>Cc: Lahtinen, Joonas <joonas.lahtinen@intel.com>; Sundaresan, Sujaritha
><sujaritha.sundaresan@intel.com>
>Subject: [Intel-gfx] [PATCH 1/2] drm/i915/guc : Removing enable_guc_loading
>module
>
>Whenever we need i915.enable_guc_submission=1, we also need
>enable_guc_loading=1. We also need enable_guc_loading=1 when we want to
>verify the HuC, which is every time we have a HuC (but all platforms with HuC
>have a GuC and viceversa).
>We don't need the user to tell when to enable the GuC loading

Improve the commit message. " Whenever we need i915.enable_guc_submission=1, we also need
enable_guc_loading=1."  Is confusing and misleading. 

Anusha 

>Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>Cc: Joonas Lahtinen <joonas.lahtinen@intel.com>
>Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>---
> drivers/gpu/drm/i915/i915_debugfs.c     | 12 +++++--
> drivers/gpu/drm/i915/i915_drv.c         |  2 +-
> drivers/gpu/drm/i915/i915_drv.h         | 10 +++---
> drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
> drivers/gpu/drm/i915/i915_gem_gtt.c     |  2 +-
> drivers/gpu/drm/i915/i915_irq.c         |  2 +-
> drivers/gpu/drm/i915/i915_params.c      |  6 ----
> drivers/gpu/drm/i915/i915_params.h      |  1 -
> drivers/gpu/drm/i915/intel_guc_loader.c | 48 +++++++++++++++++++++++----
> drivers/gpu/drm/i915/intel_huc.c        |  4 ++-
> drivers/gpu/drm/i915/intel_uc.c         | 58 ++++++++-------------------------
> drivers/gpu/drm/i915/intel_uc.h         |  4 +--
> drivers/gpu/drm/i915/intel_uncore.c     |  4 +--
> 13 files changed, 80 insertions(+), 75 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>b/drivers/gpu/drm/i915/i915_debugfs.c
>index 329fb36..7cc53c2 100644
>--- a/drivers/gpu/drm/i915/i915_debugfs.c
>+++ b/drivers/gpu/drm/i915/i915_debugfs.c
>@@ -2338,8 +2338,11 @@ static int i915_huc_load_status_info(struct seq_file
>*m, void *data)
> 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> 	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>
>-	if (!HAS_HUC_UCODE(dev_priv))
>+	if (!HAS_GUC(dev_priv)){
>+		seq_puts(m, "No HuC support in HW\n");

If Guc is not available, regardless of HuC being present or not, we cannot validate it. So the message has to be something like- "No GuC support, HuC cannot be validated"

> 		return 0;
>+	}
>+
>
> 	seq_puts(m, "HuC firmware status:\n");
> 	seq_printf(m, "\tpath: %s\n", huc_fw->path); @@ -2371,8 +2374,11 @@
>static int i915_guc_load_status_info(struct seq_file *m, void *data)
> 	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
> 	u32 tmp, i;
>
>-	if (!HAS_GUC_UCODE(dev_priv))
>+	if (!HAS_GUC(dev_priv)){
>+		seq_puts(m, "No GuC supprot in HW\n");
> 		return 0;
>+	}
>+
>
> 	seq_printf(m, "GuC firmware status:\n");
> 	seq_printf(m, "\tpath: %s\n",
>@@ -2471,7 +2477,7 @@ static bool check_guc_submission(struct seq_file *m)
>
> 	if (!guc->execbuf_client) {
> 		seq_printf(m, "GuC submission %s\n",
>-			   HAS_GUC_SCHED(dev_priv) ?
>+			   HAS_GUC(dev_priv) ?
> 			   "disabled" :
> 			   "not supported");
> 		return false;
>diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>index 25de4a9..00594dc 100644
>--- a/drivers/gpu/drm/i915/i915_drv.c
>+++ b/drivers/gpu/drm/i915/i915_drv.c
>@@ -1046,7 +1046,7 @@ 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 semaphores? %s\n",
>yesno(i915.semaphores));
>
>-	intel_uc_sanitize_options(dev_priv);
>+	intel_guc_sanitize_submission(dev_priv);
>
> 	intel_gvt_sanitize_options(dev_priv);
> }
>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>index 907603c..8e18c67 100644
>--- a/drivers/gpu/drm/i915/i915_drv.h
>+++ b/drivers/gpu/drm/i915/i915_drv.h
>@@ -3064,11 +3064,13 @@ static inline struct scatterlist *__sg_next(struct
>scatterlist *sg)
>  * command submission once loaded. But these are logically independent
>  * properties, so we have separate macros to test them.
>  */
>-#define HAS_GUC(dev_priv)	((dev_priv)->info.has_guc)
> #define HAS_GUC_CT(dev_priv)	((dev_priv)->info.has_guc_ct)
>-#define HAS_GUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
>-#define HAS_GUC_SCHED(dev_priv)	(HAS_GUC(dev_priv))
>-#define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
>+#define HAS_GUC(dev_priv)			((dev_priv)->info.has.guc)
>+#define HAS_GUC_UCODE(dev_priv)		((dev_priv)->guc.fw.path !=
>NULL)
>+#define HAS_HUC_UCODE(dev_priv)		((dev_priv)->huc.fw.path !=
>NULL)
>+#define NEEDS_GUC_LOADING(dev_priv) \
>+	(HAS_GUC(dev_priv) && \
>+	(i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))
>
> #define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)-
>>info.has_resource_streamer)
>
>diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
>b/drivers/gpu/drm/i915/i915_gem_context.c
>index ed91ac8..5166a43 100644
>--- a/drivers/gpu/drm/i915/i915_gem_context.c
>+++ b/drivers/gpu/drm/i915/i915_gem_context.c
>@@ -361,7 +361,7 @@ static u32 default_desc_template(const struct
>drm_i915_private *i915,
> 	 * present or not in use we still need a small bias as ring wraparound
> 	 * at offset 0 sometimes hangs. No idea why.
> 	 */
>-	if (HAS_GUC(dev_priv) && i915.enable_guc_loading)
>+	if (NEEDS_GUC_LOADING(dev_priv))
> 		ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
> 	else
> 		ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE; diff --git
>a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>index 10aa776..236b759 100644
>--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>@@ -2996,7 +2996,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private
>*dev_priv)
> 	 * currently don't have any bits spare to pass in this upper
> 	 * restriction!
> 	 */
>-	if (HAS_GUC(dev_priv) && i915.enable_guc_loading) {
>+	if (NEEDS_GUC_LOADING(dev_priv)) {
> 		ggtt->base.total = min_t(u64, ggtt->base.total,
>GUC_GGTT_TOP);
> 		ggtt->mappable_end = min(ggtt->mappable_end, ggtt-
>>base.total);
> 	}
>diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>index 196caa4..38bc184 100644
>--- a/drivers/gpu/drm/i915/i915_irq.c
>+++ b/drivers/gpu/drm/i915/i915_irq.c
>@@ -4139,7 +4139,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
> 	for (i = 0; i < MAX_L3_SLICES; ++i)
> 		dev_priv->l3_parity.remap_info[i] = NULL;
>
>-	if (HAS_GUC_SCHED(dev_priv))
>+	if (NEEDS_GUC_LOADING(dev_priv))
> 		dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
>
> 	/* Let's track the enabled rps events */ diff --git
>a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>index 14e2c2e..cc0dfca 100644
>--- a/drivers/gpu/drm/i915/i915_params.c
>+++ b/drivers/gpu/drm/i915/i915_params.c
>@@ -56,7 +56,6 @@ struct i915_params i915 __read_mostly = {
> 	.verbose_state_checks = 1,
> 	.nuclear_pageflip = 0,
> 	.edp_vswing = 0,
>-	.enable_guc_loading = 0,
> 	.enable_guc_submission = 0,
> 	.guc_log_level = -1,
> 	.guc_firmware_path = NULL,
>@@ -218,11 +217,6 @@ struct i915_params i915 __read_mostly = {
> 		 "(0=use value from vbt [default], 1=low power swing(200mV),"
> 		 "2=default swing(400mV))");
>
>-module_param_named_unsafe(enable_guc_loading, i915.enable_guc_loading,
>int, 0400); -MODULE_PARM_DESC(enable_guc_loading,
>-		"Enable GuC firmware loading "
>-		"(-1=auto, 0=never [default], 1=if available, 2=required)");
>-
> module_param_named_unsafe(enable_guc_submission,
>i915.enable_guc_submission, int, 0400);
>MODULE_PARM_DESC(enable_guc_submission,
> 		"Enable GuC submission "
>diff --git a/drivers/gpu/drm/i915/i915_params.h
>b/drivers/gpu/drm/i915/i915_params.h
>index febbfdb..b967c48 100644
>--- a/drivers/gpu/drm/i915/i915_params.h
>+++ b/drivers/gpu/drm/i915/i915_params.h
>@@ -43,7 +43,6 @@
> 	func(int, disable_power_well); \
> 	func(int, enable_ips); \
> 	func(int, invert_brightness); \
>-	func(int, enable_guc_loading); \
> 	func(int, enable_guc_submission); \
> 	func(int, guc_log_level); \
> 	func(char *, guc_firmware_path); \
>diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
>b/drivers/gpu/drm/i915/intel_guc_loader.c
>index 8b0ae7f..673d67a 100644
>--- a/drivers/gpu/drm/i915/intel_guc_loader.c
>+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>@@ -375,13 +375,49 @@ int intel_guc_init_hw(struct intel_guc *guc)
> 	return 0;
> }
>
>+void intel_guc_sanitize_submission(struct drm_i915_private *dev_priv) {
>+	/* Verify hardware support */
>+	if (!HAS_GUC(dev_priv)){
>+		if (i915.enable_guc_submission > 0)
>+				DRM_INFO("Ignoring GuC submission enable, no
>HW\n");
>+		i915.enable_guc_submission = 0;
>+		return;
>+	}
>+
>+	/* Verify firmware support */
>+	if (!HAS_GUC_UCODE(dev_priv)){
>+		if (i915.enable_guc_submission == 1){
>+			DRM_INFO("Ignoring GuC submission enable, no FW\n");
>+			i915.enable_guc_submission = 0;
>+			return;
>+		}
>+
>+		if (i915.enable_guc_submission < 0){
>+			i915.enable_guc_submission = 0;
>+			return;
>+		}
>+
>+		/*
>+		 * If "required"(> 1), let it continue and we will fail later
>+		 * due to lack of firmware
>+		 */
>+	}
>+
>+	/*
>+	 * A negative value means "use platform default" (enabled if we have
>+	 * survived to get here)
>+	 */
>+	if (i915.enable_guc_submission < 0)
>+		i915.enable_guc_submission = 1;
>+}
>+
>+
> /**
>  * 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)
>+void intel_guc_select_fw(struct intel_guc *guc)
> {
> 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>
>@@ -411,9 +447,9 @@ int intel_guc_select_fw(struct intel_guc *guc)
> 		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;
>+		if (HAS_GUC(dev_priv))
>+			DRM_ERROR("No GuC FW known for a platform with
>GuC!\n");
>+		return;
> 	}
>
>-	return 0;
> }
>diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
>index 6145fa0..bf831a77 100644
>--- a/drivers/gpu/drm/i915/intel_huc.c
>+++ b/drivers/gpu/drm/i915/intel_huc.c
>@@ -176,7 +176,9 @@ void intel_huc_select_fw(struct intel_huc *huc)
> 		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");
>+		/* For now, everything with a GuC also has a HuC */
>+		if (HAS_GUC(dev_priv))
>+			DRM_ERROR("No HuC FW known for a platform with
>HuC!\n");
> 		return;
> 	}
> }
>diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
>index 27e072c..d8fab3a 100644
>--- a/drivers/gpu/drm/i915/intel_uc.c
>+++ b/drivers/gpu/drm/i915/intel_uc.c
>@@ -60,40 +60,6 @@ static int __intel_uc_reset_hw(struct drm_i915_private
>*dev_priv)
> 	return ret;
> }
>
>-void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) -{
>-	if (!HAS_GUC(dev_priv)) {
>-		if (i915.enable_guc_loading > 0 ||
>-		    i915.enable_guc_submission > 0)
>-			DRM_INFO("Ignoring GuC options, no hardware\n");
>-
>-		i915.enable_guc_loading = 0;
>-		i915.enable_guc_submission = 0;
>-		return;
>-	}
>-
>-	/* A negative value means "use platform default" */
>-	if (i915.enable_guc_loading < 0)
>-		i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
>-
>-	/* Verify firmware version */
>-	if (i915.enable_guc_loading) {
>-		if (HAS_HUC_UCODE(dev_priv))
>-			intel_huc_select_fw(&dev_priv->huc);
>-
>-		if (intel_guc_select_fw(&dev_priv->guc))
>-			i915.enable_guc_loading = 0;
>-	}
>-
>-	/* Can't enable guc submission without guc loaded */
>-	if (!i915.enable_guc_loading)
>-		i915.enable_guc_submission = 0;
>-
>-	/* A negative value means "use platform default" */
>-	if (i915.enable_guc_submission < 0)
>-		i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
>-}
>-
> static void guc_write_irq_trigger(struct intel_guc *guc)  {
> 	struct drm_i915_private *dev_priv = guc_to_i915(guc); @@ -106,6 +72,8
>@@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
> 	struct intel_guc *guc = &dev_priv->guc;
>
> 	intel_guc_ct_init_early(&guc->ct);
>+	intel_guc_select_fw(&dev_priv->guc);
>+	intel_huc_select_fw(&dev_priv->huc);
>
> 	mutex_init(&guc->send_mutex);
> 	guc->send = intel_guc_send_nop;
>@@ -252,6 +220,8 @@ static void fetch_uc_fw(struct drm_i915_private
>*dev_priv,
>
> void intel_uc_init_fw(struct drm_i915_private *dev_priv)  {
>+	if(!HAS_GUC(dev_priv))
>+		return;
> 	fetch_uc_fw(dev_priv, &dev_priv->huc.fw);
> 	fetch_uc_fw(dev_priv, &dev_priv->guc.fw);  } @@ -333,7 +303,7 @@ int
>intel_uc_init_hw(struct drm_i915_private *dev_priv)
> 	struct intel_guc *guc = &dev_priv->guc;
> 	int ret, attempts;
>
>-	if (!i915.enable_guc_loading)
>+	if (!NEEDS_GUC_LOADING(dev_priv))
> 		return 0;
>
> 	guc_disable_communication(guc);
>@@ -423,18 +393,16 @@ int intel_uc_init_hw(struct drm_i915_private
>*dev_priv)
> 	i915_ggtt_disable_guc(dev_priv);
>
> 	DRM_ERROR("GuC init failed\n");
>-	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
>+	if (i915.enable_guc_submission > 1) {
>+		DRM_NOTE("GuC is required, so marking the GPU as
>wedged\n");
> 		ret = -EIO;
>-	else
>-		ret = 0;
>
>-	if (i915.enable_guc_submission) {
>-		i915.enable_guc_submission = 0;
>+	} else if (i915.enable_guc_submission == 1) {
> 		DRM_NOTE("Falling back from GuC submission to execlist
>mode\n");
>-	}
>-
>-	i915.enable_guc_loading = 0;
>-	DRM_NOTE("GuC firmware loading disabled\n");
>+			i915.enable_guc_submission = 0;
>+			ret = 0;
>+		} else
>+			ret = 0;
>
> 	return ret;
> }
>@@ -443,7 +411,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>{
> 	guc_free_load_err_log(&dev_priv->guc);
>
>-	if (!i915.enable_guc_loading)
>+	if (!NEEDS_GUC_LOADING(dev_priv))
> 		return;
>
> 	if (i915.enable_guc_submission)
>diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
>index 22ae52b..7370b74 100644
>--- a/drivers/gpu/drm/i915/intel_uc.h
>+++ b/drivers/gpu/drm/i915/intel_uc.h
>@@ -220,7 +220,6 @@ struct intel_huc {
> };
>
> /* intel_uc.c */
>-void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);  void
>intel_uc_init_early(struct drm_i915_private *dev_priv);  void
>intel_uc_init_fw(struct drm_i915_private *dev_priv);  void intel_uc_fini_fw(struct
>drm_i915_private *dev_priv); @@ -241,13 +240,14 @@ static inline void
>intel_guc_notify(struct intel_guc *guc)  }
>
> /* intel_guc_loader.c */
>-int intel_guc_select_fw(struct intel_guc *guc);
>+void intel_guc_select_fw(struct intel_guc *guc);
> int intel_guc_init_hw(struct intel_guc *guc);  int intel_guc_suspend(struct
>drm_i915_private *dev_priv);  int intel_guc_resume(struct drm_i915_private
>*dev_priv);
> u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
>
> /* i915_guc_submission.c */
>+void intel_guc_sanitize_submission(struct drm_i915_private *dev_priv);
> int i915_guc_submission_init(struct drm_i915_private *dev_priv);  int
>i915_guc_submission_enable(struct drm_i915_private *dev_priv);  int
>i915_guc_wq_reserve(struct drm_i915_gem_request *rq); diff --git
>a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>index deb4430..6ef8069 100644
>--- a/drivers/gpu/drm/i915/intel_uncore.c
>+++ b/drivers/gpu/drm/i915/intel_uncore.c
>@@ -1734,12 +1734,10 @@ int intel_guc_reset(struct drm_i915_private
>*dev_priv)  {
> 	int ret;
>
>-	if (!HAS_GUC(dev_priv))
>-		return -EINVAL;
>+	GEM_BUG_ON(!HAS_GUC(dev_priv));
>
> 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
> 	ret = gen6_hw_domain_reset(dev_priv, GEN9_GRDOM_GUC);
>-	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>
> 	return ret;
> }
>--
>1.9.1
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] drm/i915/guc : Removing enable_guc_loading module
  2017-08-23 22:11 [PATCH 1/2] drm/i915/guc : Removing enable_guc_loading module Sujaritha Sundaresan
  2017-08-23 22:25 ` Srivatsa, Anusha
@ 2017-08-24 12:49 ` Michal Wajdeczko
  2017-08-25 21:32 ` Daniele Ceraolo Spurio
  2 siblings, 0 replies; 4+ messages in thread
From: Michal Wajdeczko @ 2017-08-24 12:49 UTC (permalink / raw)
  To: Sujaritha Sundaresan; +Cc: Joonas Lahtinen, intel-gfx

On Wed, Aug 23, 2017 at 03:11:31PM -0700, Sujaritha Sundaresan wrote:
> Whenever we need i915.enable_guc_submission=1, we also need enable_guc_loading=1. We also need enable_guc_loading=1 when we want to verify the HuC, which is every time we have a HuC (but all platforms with HuC have a GuC and viceversa).
> We don't need the user to tell when to enable the GuC loading
> 
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@intel.com>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     | 12 +++++--
>  drivers/gpu/drm/i915/i915_drv.c         |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h         | 10 +++---
>  drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
>  drivers/gpu/drm/i915/i915_gem_gtt.c     |  2 +-
>  drivers/gpu/drm/i915/i915_irq.c         |  2 +-
>  drivers/gpu/drm/i915/i915_params.c      |  6 ----
>  drivers/gpu/drm/i915/i915_params.h      |  1 -
>  drivers/gpu/drm/i915/intel_guc_loader.c | 48 +++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_huc.c        |  4 ++-
>  drivers/gpu/drm/i915/intel_uc.c         | 58 ++++++++-------------------------
>  drivers/gpu/drm/i915/intel_uc.h         |  4 +--
>  drivers/gpu/drm/i915/intel_uncore.c     |  4 +--
>  13 files changed, 80 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 329fb36..7cc53c2 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2338,8 +2338,11 @@ static int i915_huc_load_status_info(struct seq_file *m, void *data)
>  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
>  	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>  
> -	if (!HAS_HUC_UCODE(dev_priv))
> +	if (!HAS_GUC(dev_priv)){
> +		seq_puts(m, "No HuC support in HW\n");

Can we unify such messages? Currently we are using:
	seq_puts(m, "FBC unsupported on this chipset\n");
	seq_puts(m, "not supported\n");
	seq_puts(m, "unsupported on this chipset\n");
	seq_printf(m, "GuC submission %s\n" ... "not supported");
	seq_puts(m, "PSR not supported\n");
	seq_puts(m, "Runtime power management not supported\n");

I would use here simplest one:
	seq_puts(m, "not supported\n")
or with reusable string:
	seq_printf(m, "%s not supported", "Huc");

>  		return 0;
> +	}
> +		
>  
>  	seq_puts(m, "HuC firmware status:\n");
>  	seq_printf(m, "\tpath: %s\n", huc_fw->path);
> @@ -2371,8 +2374,11 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
>  	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>  	u32 tmp, i;
>  
> -	if (!HAS_GUC_UCODE(dev_priv))
> +	if (!HAS_GUC(dev_priv)){
> +		seq_puts(m, "No GuC supprot in HW\n");

Typo

>  		return 0;
> +	}
> +		
>  
>  	seq_printf(m, "GuC firmware status:\n");
>  	seq_printf(m, "\tpath: %s\n",
> @@ -2471,7 +2477,7 @@ static bool check_guc_submission(struct seq_file *m)
>  
>  	if (!guc->execbuf_client) {
>  		seq_printf(m, "GuC submission %s\n",
> -			   HAS_GUC_SCHED(dev_priv) ?
> +			   HAS_GUC(dev_priv) ?
>  			   "disabled" :
>  			   "not supported");
>  		return false;
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 25de4a9..00594dc 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1046,7 +1046,7 @@ 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 semaphores? %s\n", yesno(i915.semaphores));
>  
> -	intel_uc_sanitize_options(dev_priv);
> +	intel_guc_sanitize_submission(dev_priv);
>  
>  	intel_gvt_sanitize_options(dev_priv);
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 907603c..8e18c67 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3064,11 +3064,13 @@ static inline struct scatterlist *__sg_next(struct scatterlist *sg)
>   * command submission once loaded. But these are logically independent
>   * properties, so we have separate macros to test them.
>   */
> -#define HAS_GUC(dev_priv)	((dev_priv)->info.has_guc)
>  #define HAS_GUC_CT(dev_priv)	((dev_priv)->info.has_guc_ct)
> -#define HAS_GUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
> -#define HAS_GUC_SCHED(dev_priv)	(HAS_GUC(dev_priv))
> -#define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
> +#define HAS_GUC(dev_priv)			((dev_priv)->info.has.guc)
> +#define HAS_GUC_UCODE(dev_priv)		((dev_priv)->guc.fw.path != NULL)
> +#define HAS_HUC_UCODE(dev_priv)		((dev_priv)->huc.fw.path != NULL)

Add separation line here

> +#define NEEDS_GUC_LOADING(dev_priv) \
> +	(HAS_GUC(dev_priv) && \
> +	(i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))
>  
>  #define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)->info.has_resource_streamer)
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index ed91ac8..5166a43 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -361,7 +361,7 @@ static u32 default_desc_template(const struct drm_i915_private *i915,
>  	 * present or not in use we still need a small bias as ring wraparound
>  	 * at offset 0 sometimes hangs. No idea why.
>  	 */
> -	if (HAS_GUC(dev_priv) && i915.enable_guc_loading)
> +	if (NEEDS_GUC_LOADING(dev_priv))
>  		ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
>  	else
>  		ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 10aa776..236b759 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2996,7 +2996,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv)
>  	 * currently don't have any bits spare to pass in this upper
>  	 * restriction!
>  	 */
> -	if (HAS_GUC(dev_priv) && i915.enable_guc_loading) {
> +	if (NEEDS_GUC_LOADING(dev_priv)) {
>  		ggtt->base.total = min_t(u64, ggtt->base.total, GUC_GGTT_TOP);
>  		ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total);
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 196caa4..38bc184 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4139,7 +4139,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>  	for (i = 0; i < MAX_L3_SLICES; ++i)
>  		dev_priv->l3_parity.remap_info[i] = NULL;
>  
> -	if (HAS_GUC_SCHED(dev_priv))
> +	if (NEEDS_GUC_LOADING(dev_priv))
>  		dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
>  
>  	/* Let's track the enabled rps events */
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 14e2c2e..cc0dfca 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -56,7 +56,6 @@ struct i915_params i915 __read_mostly = {
>  	.verbose_state_checks = 1,
>  	.nuclear_pageflip = 0,
>  	.edp_vswing = 0,
> -	.enable_guc_loading = 0,
>  	.enable_guc_submission = 0,
>  	.guc_log_level = -1,
>  	.guc_firmware_path = NULL,
> @@ -218,11 +217,6 @@ struct i915_params i915 __read_mostly = {
>  		 "(0=use value from vbt [default], 1=low power swing(200mV),"
>  		 "2=default swing(400mV))");
>  
> -module_param_named_unsafe(enable_guc_loading, i915.enable_guc_loading, int, 0400);
> -MODULE_PARM_DESC(enable_guc_loading,
> -		"Enable GuC firmware loading "
> -		"(-1=auto, 0=never [default], 1=if available, 2=required)");
> -
>  module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, int, 0400);
>  MODULE_PARM_DESC(enable_guc_submission,
>  		"Enable GuC submission "
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index febbfdb..b967c48 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -43,7 +43,6 @@
>  	func(int, disable_power_well); \
>  	func(int, enable_ips); \
>  	func(int, invert_brightness); \
> -	func(int, enable_guc_loading); \
>  	func(int, enable_guc_submission); \
>  	func(int, guc_log_level); \
>  	func(char *, guc_firmware_path); \
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 8b0ae7f..673d67a 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -375,13 +375,49 @@ int intel_guc_init_hw(struct intel_guc *guc)
>  	return 0;
>  }
>  
> +void intel_guc_sanitize_submission(struct drm_i915_private *dev_priv)

I would keep old name "intel_uc_sanitize_options()" as this will be our
entry point to sanitize all "uC" related options (current and future)
And let it stay in intel_uc.c

> +{
> +	/* Verify hardware support */
> +	if (!HAS_GUC(dev_priv)){
> +		if (i915.enable_guc_submission > 0)
> +				DRM_INFO("Ignoring GuC submission enable, no HW\n");
> +		i915.enable_guc_submission = 0;
> +		return;
> +	}
> +
> +	/* Verify firmware support */
> +	if (!HAS_GUC_UCODE(dev_priv)){
> +		if (i915.enable_guc_submission == 1){
> +			DRM_INFO("Ignoring GuC submission enable, no FW\n");
> +			i915.enable_guc_submission = 0;
> +			return;
> +		}
> +
> +		if (i915.enable_guc_submission < 0){
> +			i915.enable_guc_submission = 0;
> +			return;
> +		}
> +
> +		/* 
> +		 * If "required"(> 1), let it continue and we will fail later 
> +		 * due to lack of firmware
> +		 */

This is slightly inconsistent with !HAS_GUC() case above, where we turn off
submission flag and return even if it was previously set to "required".

> +	}
> +
> +	/*
> +	 * A negative value means "use platform default" (enabled if we have
> +	 * survived to get here)
> +	 */
> +	if (i915.enable_guc_submission < 0)
> +		i915.enable_guc_submission = 1;
> +}
> +
> +
>  /**
>   * 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)
> +void intel_guc_select_fw(struct intel_guc *guc)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>  
> @@ -411,9 +447,9 @@ int intel_guc_select_fw(struct intel_guc *guc)
>  		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;
> +		if (HAS_GUC(dev_priv))

Early (and quiet) return shall be at the top of this function.
Or maybe HAS_GUC() should be checked by the caller?

> +			DRM_ERROR("No GuC FW known for a platform with GuC!\n");
> +		return;
>  	}
>  
> -	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> index 6145fa0..bf831a77 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -176,7 +176,9 @@ void intel_huc_select_fw(struct intel_huc *huc)
>  		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");
> +		/* For now, everything with a GuC also has a HuC */
> +		if (HAS_GUC(dev_priv))

Same comment as above.

> +			DRM_ERROR("No HuC FW known for a platform with HuC!\n");
>  		return;
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 27e072c..d8fab3a 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -60,40 +60,6 @@ static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> -void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
> -{
> -	if (!HAS_GUC(dev_priv)) {
> -		if (i915.enable_guc_loading > 0 ||
> -		    i915.enable_guc_submission > 0)
> -			DRM_INFO("Ignoring GuC options, no hardware\n");
> -
> -		i915.enable_guc_loading = 0;
> -		i915.enable_guc_submission = 0;
> -		return;
> -	}
> -
> -	/* A negative value means "use platform default" */
> -	if (i915.enable_guc_loading < 0)
> -		i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
> -
> -	/* Verify firmware version */
> -	if (i915.enable_guc_loading) {
> -		if (HAS_HUC_UCODE(dev_priv))
> -			intel_huc_select_fw(&dev_priv->huc);
> -
> -		if (intel_guc_select_fw(&dev_priv->guc))
> -			i915.enable_guc_loading = 0;
> -	}
> -
> -	/* Can't enable guc submission without guc loaded */
> -	if (!i915.enable_guc_loading)
> -		i915.enable_guc_submission = 0;
> -
> -	/* A negative value means "use platform default" */
> -	if (i915.enable_guc_submission < 0)
> -		i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> -}
> -
>  static void guc_write_irq_trigger(struct intel_guc *guc)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> @@ -106,6 +72,8 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  	struct intel_guc *guc = &dev_priv->guc;
>  
>  	intel_guc_ct_init_early(&guc->ct);
> +	intel_guc_select_fw(&dev_priv->guc);
> +	intel_huc_select_fw(&dev_priv->huc);

Maybe we should check HAS_GUC() before calling "select_fw()" ?

>  
>  	mutex_init(&guc->send_mutex);
>  	guc->send = intel_guc_send_nop;
> @@ -252,6 +220,8 @@ static void fetch_uc_fw(struct drm_i915_private *dev_priv,
>  
>  void intel_uc_init_fw(struct drm_i915_private *dev_priv)
>  {
> +	if(!HAS_GUC(dev_priv))
> +		return;

"fetch_uc_fw" function can handle no fw path case.

>  	fetch_uc_fw(dev_priv, &dev_priv->huc.fw);
>  	fetch_uc_fw(dev_priv, &dev_priv->guc.fw);
>  }
> @@ -333,7 +303,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  	struct intel_guc *guc = &dev_priv->guc;
>  	int ret, attempts;
>  
> -	if (!i915.enable_guc_loading)
> +	if (!NEEDS_GUC_LOADING(dev_priv))
>  		return 0;
>  
>  	guc_disable_communication(guc);
> @@ -423,18 +393,16 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  	i915_ggtt_disable_guc(dev_priv);
>  
>  	DRM_ERROR("GuC init failed\n");
> -	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
> +	if (i915.enable_guc_submission > 1) {
> +		DRM_NOTE("GuC is required, so marking the GPU as wedged\n");	
>  		ret = -EIO;
> -	else
> -		ret = 0;
>  
> -	if (i915.enable_guc_submission) {
> -		i915.enable_guc_submission = 0;
> +	} else if (i915.enable_guc_submission == 1) {
>  		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> -	}
> -
> -	i915.enable_guc_loading = 0;
> -	DRM_NOTE("GuC firmware loading disabled\n");
> +			i915.enable_guc_submission = 0;
> +			ret = 0;
> +		} else
> +			ret = 0;
>  
>  	return ret;
>  }
> @@ -443,7 +411,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>  {
>  	guc_free_load_err_log(&dev_priv->guc);
>  
> -	if (!i915.enable_guc_loading)
> +	if (!NEEDS_GUC_LOADING(dev_priv))
>  		return;
>  
>  	if (i915.enable_guc_submission)
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 22ae52b..7370b74 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -220,7 +220,6 @@ struct intel_huc {
>  };
>  
>  /* intel_uc.c */
> -void intel_uc_sanitize_options(struct drm_i915_private *dev_priv);
>  void intel_uc_init_early(struct drm_i915_private *dev_priv);
>  void intel_uc_init_fw(struct drm_i915_private *dev_priv);
>  void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
> @@ -241,13 +240,14 @@ static inline void intel_guc_notify(struct intel_guc *guc)
>  }
>  
>  /* intel_guc_loader.c */
> -int intel_guc_select_fw(struct intel_guc *guc);
> +void intel_guc_select_fw(struct intel_guc *guc);
>  int intel_guc_init_hw(struct intel_guc *guc);
>  int intel_guc_suspend(struct drm_i915_private *dev_priv);
>  int intel_guc_resume(struct drm_i915_private *dev_priv);
>  u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
>  
>  /* i915_guc_submission.c */
> +void intel_guc_sanitize_submission(struct drm_i915_private *dev_priv);

Wrong function prefix (all other have i915_*)
Mismatched location (diff above says function is in _loader.c)

>  int i915_guc_submission_init(struct drm_i915_private *dev_priv);
>  int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
>  int i915_guc_wq_reserve(struct drm_i915_gem_request *rq);
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index deb4430..6ef8069 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1734,12 +1734,10 @@ int intel_guc_reset(struct drm_i915_private *dev_priv)
>  {
>  	int ret;
>  
> -	if (!HAS_GUC(dev_priv))
> -		return -EINVAL;
> +	GEM_BUG_ON(!HAS_GUC(dev_priv));
>  
>  	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>  	ret = gen6_hw_domain_reset(dev_priv, GEN9_GRDOM_GUC);
> -	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);

This looks like unrelated change wrt commit message.

>  
>  	return ret;
>  }
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] drm/i915/guc : Removing enable_guc_loading module
  2017-08-23 22:11 [PATCH 1/2] drm/i915/guc : Removing enable_guc_loading module Sujaritha Sundaresan
  2017-08-23 22:25 ` Srivatsa, Anusha
  2017-08-24 12:49 ` Michal Wajdeczko
@ 2017-08-25 21:32 ` Daniele Ceraolo Spurio
  2 siblings, 0 replies; 4+ messages in thread
From: Daniele Ceraolo Spurio @ 2017-08-25 21:32 UTC (permalink / raw)
  To: Sujaritha Sundaresan, intel-gfx; +Cc: Joonas Lahtinen



On 23/08/17 15:11, Sujaritha Sundaresan wrote:
> Whenever we need i915.enable_guc_submission=1, we also need enable_guc_loading=1. We also need enable_guc_loading=1 when we want to verify the HuC, which is every time we have a HuC (but all platforms with HuC have a GuC and viceversa).
> We don't need the user to tell when to enable the GuC loading
> 

Drive-by comment: I'd call out more explicitly that with this patch as 
long as both GuC and HuC FW are on the machine they will always be 
loaded, which is a change to the current behavior. I'm not implying that 
the change is bad, but it alters timing in some scenarios (e.g. resume) 
and interested parties might miss it if we aren't explicit about it.

Thanks,
Daniele

> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@intel.com>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> ---
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-08-25 21:32 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-23 22:11 [PATCH 1/2] drm/i915/guc : Removing enable_guc_loading module Sujaritha Sundaresan
2017-08-23 22:25 ` Srivatsa, Anusha
2017-08-24 12:49 ` Michal Wajdeczko
2017-08-25 21:32 ` Daniele Ceraolo Spurio

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.