[PATCH v3 1/2] drm/i915/guc : Removing enable_guc_loading module

[PATCH v3 1/2] drm/i915/guc : Removing enable_guc_loading module

[Sujaritha Sundaresan]

We currently have two module parameters that control GuC: "enable_guc_loading" and "enable_guc_submission".
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).

v2: Clarifying the commit message (Anusha)
v3: Unify seq_puts messages, correcting inconsistencies (Michal)

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c        | 18 +++---
 drivers/gpu/drm/i915/i915_drv.c            |  4 +-
 drivers/gpu/drm/i915/i915_drv.h            |  1 +
 drivers/gpu/drm/i915/i915_guc_submission.c | 15 ++++-
 drivers/gpu/drm/i915/intel_guc_loader.c    | 50 ++---------------
 drivers/gpu/drm/i915/intel_guc_log.c       |  6 +-
 drivers/gpu/drm/i915/intel_huc.c           | 14 ++---
 drivers/gpu/drm/i915/intel_uc.c            | 90 ++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_uc.h            |  3 +-
 9 files changed, 97 insertions(+), 104 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 7cc53c2..9396de0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1612,7 +1612,7 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
 
 	if (!HAS_FBC(dev_priv)) {
-		seq_puts(m, "FBC unsupported on this chipset\n");
+		seq_puts(m, "not supported\n");
 		return 0;
 	}
 
@@ -1779,7 +1779,7 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
 	unsigned int max_gpu_freq, min_gpu_freq;
 
 	if (!HAS_LLC(dev_priv)) {
-		seq_puts(m, "unsupported on this chipset\n");
+		seq_puts(m, "not supported\n");
 		return 0;
 	}
 
@@ -2339,7 +2339,7 @@ static int i915_huc_load_status_info(struct seq_file *m, void *data)
 	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
 
 	if (!HAS_GUC(dev_priv)){
-		seq_puts(m, "No HuC support in HW\n");
+		seq_puts(m, "not supported\n");
 		return 0;
 	}
 		
@@ -2375,7 +2375,7 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
 	u32 tmp, i;
 
 	if (!HAS_GUC(dev_priv)){
-		seq_puts(m, "No GuC supprot in HW\n");
+		seq_puts(m, "not supported\n");
 		return 0;
 	}
 		
@@ -2476,7 +2476,7 @@ static bool check_guc_submission(struct seq_file *m)
 	const struct intel_guc *guc = &dev_priv->guc;
 
 	if (!guc->execbuf_client) {
-		seq_printf(m, "GuC submission %s\n",
+		seq_printf(m,
 			   HAS_GUC(dev_priv) ?
 			   "disabled" :
 			   "not supported");
@@ -2666,7 +2666,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	bool enabled = false;
 
 	if (!HAS_PSR(dev_priv)) {
-		seq_puts(m, "PSR not supported\n");
+		seq_puts(m, "not supported\n");
 		return 0;
 	}
 
@@ -2819,7 +2819,7 @@ static int i915_runtime_pm_status(struct seq_file *m, void *unused)
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 
 	if (!HAS_RUNTIME_PM(dev_priv))
-		seq_puts(m, "Runtime power management not supported\n");
+		seq_puts(m, "not supported\n");
 
 	seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->gt.awake));
 	seq_printf(m, "IRQs disabled: %s\n",
@@ -3639,7 +3639,7 @@ static void drrs_status_per_crtc(struct seq_file *m,
 		mutex_unlock(&drrs->mutex);
 	} else {
 		/* DRRS not supported. Print the VBT parameter*/
-		seq_puts(m, "\tDRRS Supported : No");
+		seq_puts(m, "not supported\n");
 	}
 	seq_puts(m, "\n");
 }
@@ -5039,4 +5039,4 @@ int i915_debugfs_connector_add(struct drm_connector *connector)
 				    connector, &i915_panel_fops);
 
 	return 0;
-}
+}
\ No newline at end of file
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 00594dc..1c8d136 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_guc_sanitize_submission(dev_priv);
+	intel_uc_sanitize_options(dev_priv);
 
 	intel_gvt_sanitize_options(dev_priv);
 }
@@ -2769,4 +2769,4 @@ static int intel_runtime_resume(struct device *kdev)
 
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/mock_drm.c"
-#endif
+#endif
\ No newline at end of file
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 437c3c6..424cd78 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3068,6 +3068,7 @@ static inline struct scatterlist *__sg_next(struct scatterlist *sg)
 #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)))
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 639e0d8..a15146e 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1109,12 +1109,17 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 
 	vaddr = i915_gem_object_pin_map(guc->stage_desc_pool->obj, I915_MAP_WB);
 	if (IS_ERR(vaddr)) {
-		i915_vma_unpin_and_release(&guc->stage_desc_pool);
-		return PTR_ERR(vaddr);
+		ret = PTR_ERR(vaddr);
+		goto err_vma;
 	}
 
 	guc->stage_desc_pool_vaddr = vaddr;
 
+	ret = intel_guc_log_create(guc);
+	if (ret < 0)
+		goto err_vaddr;
+	
+
 	ret = guc_ads_create(guc);
 	if (ret < 0)
 		goto err_log;
@@ -1125,6 +1130,11 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 
 err_log:
 	intel_guc_log_destroy(guc);
+	
+err_vaddr:
+	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
+err_vma:
+	i915_vma_unpin_and_release(&guc->stage_desc_pool);
 	return ret;
 }
 
@@ -1134,6 +1144,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 
 	ida_destroy(&guc->stage_ids);
 	guc_ads_destroy(guc);
+	intel_guc_log_destroy(guc);
 	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
 	i915_vma_unpin_and_release(&guc->stage_desc_pool);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 673d67a..672c83c 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -375,44 +375,6 @@ 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
@@ -426,7 +388,11 @@ void intel_guc_select_fw(struct intel_guc *guc)
 	guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
 	guc->fw.type = INTEL_UC_FW_TYPE_GUC;
 
-	if (i915.guc_firmware_path) {
+	
+	if (HAS_GUC(dev_priv)){
+			DRM_ERROR("No GuC FW known for a platform with GuC!\n");
+		return;
+	} else if (i915.guc_firmware_path) {
 		guc->fw.path = i915.guc_firmware_path;
 		guc->fw.major_ver_wanted = 0;
 		guc->fw.minor_ver_wanted = 0;
@@ -446,10 +412,6 @@ void intel_guc_select_fw(struct intel_guc *guc)
 		guc->fw.path = I915_GLK_GUC_UCODE;
 		guc->fw.major_ver_wanted = GLK_FW_MAJOR;
 		guc->fw.minor_ver_wanted = GLK_FW_MINOR;
-	} else {
-		if (HAS_GUC(dev_priv))
-			DRM_ERROR("No GuC FW known for a platform with GuC!\n");
-		return;
-	}
+	} 
 
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 592b449..16d3b87 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -502,7 +502,7 @@ static void guc_flush_logs(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
-	if (HAS_GUC_UCODE(dev_priv) || (i915.guc_log_level < 0))
+	if (!i915.enable_guc_submission || (i915.guc_log_level < 0))
 		return;
 
 	/* First disable the interrupts, will be renabled afterwards */
@@ -641,7 +641,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 
 void i915_guc_log_register(struct drm_i915_private *dev_priv)
 {
-	if (HAS_GUC_UCODE(dev_priv) || i915.guc_log_level < 0)
+	if (!i915.enable_guc_submission || i915.guc_log_level < 0)
 		return;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
@@ -651,7 +651,7 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv)
 
 void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 {
-	if (HAS_GUC_UCODE(dev_priv))
+	if (!i915.enable_guc_submission)
 		return;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index bf831a77..57dce67 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -155,7 +155,12 @@ void intel_huc_select_fw(struct intel_huc *huc)
 	huc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
 	huc->fw.type = INTEL_UC_FW_TYPE_HUC;
 
-	if (i915.huc_firmware_path) {
+	
+		/* 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;
+	} else if (i915.huc_firmware_path) {
 		huc->fw.path = i915.huc_firmware_path;
 		huc->fw.major_ver_wanted = 0;
 		huc->fw.minor_ver_wanted = 0;
@@ -175,12 +180,7 @@ void intel_huc_select_fw(struct intel_huc *huc)
 		huc->fw.path = I915_GLK_HUC_UCODE;
 		huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR;
 		huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR;
-	} else {
-		/* 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 44faeac..52687e3 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -60,24 +60,49 @@ static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-static void guc_write_irq_trigger(struct intel_guc *guc)
+void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 {
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	/* 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;
+	}
 
-	I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
+	/* 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;
 }
 
-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);
+static void guc_write_irq_trigger(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
-	mutex_init(&guc->send_mutex);
-	guc->send = intel_guc_send_nop;
-	guc->notify = guc_write_irq_trigger;
+	I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
 }
 
 static void fetch_uc_fw(struct drm_i915_private *dev_priv,
@@ -218,6 +243,12 @@ static void fetch_uc_fw(struct drm_i915_private *dev_priv,
 	uc_fw->fetch_status = INTEL_UC_FIRMWARE_FAIL;
 }
 
+void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
+{
+	__intel_uc_fw_fini(&dev_priv->guc.fw);
+	__intel_uc_fw_fini(&dev_priv->huc.fw);
+}
+
 void intel_uc_init_fw(struct drm_i915_private *dev_priv)
 {
 	if(!HAS_GUC(dev_priv))
@@ -226,10 +257,17 @@ void intel_uc_init_fw(struct drm_i915_private *dev_priv)
 	fetch_uc_fw(dev_priv, &dev_priv->guc.fw);
 }
 
-void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
+void intel_uc_init_early(struct drm_i915_private *dev_priv)
 {
-	__intel_uc_fw_fini(&dev_priv->guc.fw);
-	__intel_uc_fw_fini(&dev_priv->huc.fw);
+	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;
+	guc->notify = guc_write_irq_trigger;
 }
 
 static inline i915_reg_t guc_send_reg(struct intel_guc *guc, u32 i)
@@ -298,20 +336,6 @@ static void guc_disable_communication(struct intel_guc *guc)
 	guc->send = intel_guc_send_nop;
 }
 
-static int guc_shared_objects_init(struct intel_guc *guc)
-{
-	int ret;
-
-	ret = intel_guc_log_create(guc);
-	if (ret < 0)
-		return ret;
-}
-
-static void guc_shared_objects_fini(struct intel_guc *guc)
-{
-	intel_guc_log_destroy(guc);
-}
-
 int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
@@ -325,9 +349,6 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 
 	/* We need to notify the guc whenever we change the GGTT */
 	i915_ggtt_enable_guc(dev_priv);
-	ret = guc_shared_objects_init(guc);
-	if (ret)
-		goto err_guc;
 
 	if (i915.enable_guc_submission) {
 		/*
@@ -336,7 +357,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 		 */
 		ret = i915_guc_submission_init(dev_priv);
 		if (ret)
-			goto err_shared;
+			goto err_guc;
 	}
 
 	/* init WOPCM */
@@ -406,8 +427,6 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 err_submission:
 	if (i915.enable_guc_submission)
 		i915_guc_submission_fini(dev_priv);
-err_shared:
-	guc_shared_objects_fini(guc);
 err_guc:
 	i915_ggtt_disable_guc(dev_priv);
 
@@ -443,7 +462,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 		i915_guc_submission_fini(dev_priv);
 	}
 
-	guc_shared_objects_fini(&dev_priv->guc);
 	i915_ggtt_disable_guc(dev_priv);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index ea2a32b..731ec16 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -220,6 +220,7 @@ 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);
@@ -246,8 +247,8 @@ static inline void intel_guc_notify(struct intel_guc *guc)
 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);
-- 
1.9.1

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

[PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading module

[Sujaritha Sundaresan]

We currently have two module parameters that control GuC: "enable_guc_loading" and "enable_guc_submission".
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).

v2: Clarifying the commit message (Anusha)
v3: Unify seq_puts messages, correcting inconsistencies (Michal)
v4: Rebased

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c     | 22 +++++----
 drivers/gpu/drm/i915/i915_drv.h         | 11 +++--
 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      |  5 --
 drivers/gpu/drm/i915/i915_params.h      |  1 -
 drivers/gpu/drm/i915/intel_guc_loader.c |  6 ++-
 drivers/gpu/drm/i915/intel_uc.c         | 83 ++++++++++++++++++---------------
 9 files changed, 73 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index ca6fa6d..063fbe3 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1616,7 +1616,7 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
 
 	if (!HAS_FBC(dev_priv)) {
-		seq_puts(m, "FBC unsupported on this chipset\n");
+		seq_puts(m, "not supported\n");
 		return 0;
 	}
 
@@ -1783,7 +1783,7 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
 	unsigned int max_gpu_freq, min_gpu_freq;
 
 	if (!HAS_LLC(dev_priv)) {
-		seq_puts(m, "unsupported on this chipset\n");
+		seq_puts(m, "not supported\n");
 		return 0;
 	}
 
@@ -2336,8 +2336,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, "not supported\n");
 		return 0;
+	}
+		
 
 	seq_puts(m, "HuC firmware status:\n");
 	seq_printf(m, "\tpath: %s\n", huc_fw->path);
@@ -2369,8 +2372,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, "not supported\n");
 		return 0;
+	}
+	
 
 	seq_printf(m, "GuC firmware status:\n");
 	seq_printf(m, "\tpath: %s\n",
@@ -2465,7 +2471,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;
@@ -2654,7 +2660,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	bool enabled = false;
 
 	if (!HAS_PSR(dev_priv)) {
-		seq_puts(m, "PSR not supported\n");
+		seq_puts(m, "not supported\n");
 		return 0;
 	}
 
@@ -2807,7 +2813,7 @@ static int i915_runtime_pm_status(struct seq_file *m, void *unused)
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 
 	if (!HAS_RUNTIME_PM(dev_priv))
-		seq_puts(m, "Runtime power management not supported\n");
+		seq_puts(m, "not supported\n");
 
 	seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->gt.awake));
 	seq_printf(m, "IRQs disabled: %s\n",
@@ -3683,7 +3689,7 @@ static void drrs_status_per_crtc(struct seq_file *m,
 		mutex_unlock(&drrs->mutex);
 	} else {
 		/* DRRS not supported. Print the VBT parameter*/
-		seq_puts(m, "\tDRRS Supported : No");
+		seq_puts(m, "not supported\n");
 	}
 	seq_puts(m, "\n");
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6d7d871..bd583f7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3138,11 +3138,14 @@ static inline unsigned int i915_sg_segment_size(void)
  * 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 58a2a44..aaf3c03 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -314,7 +314,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 731ce22..c1aeefb 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3292,7 +3292,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 4d0e8f7..a151688 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3962,7 +3962,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 ddda513..bd0571d 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -63,7 +63,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,
@@ -201,10 +200,6 @@ struct i915_params i915 __read_mostly = {
 	"(0=use value from vbt [default], 1=low power swing(200mV),"
 	"2=default swing(400mV))");
 
-i915_param_named_unsafe(enable_guc_loading, int, 0400,
-	"Enable GuC firmware loading "
-	"(-1=auto, 0=never [default], 1=if available, 2=required)");
-
 i915_param_named_unsafe(enable_guc_submission, int, 0400,
 	"Enable GuC submission "
 	"(-1=auto, 0=never [default], 1=if available, 2=required)");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index ac84470..e783d0b 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -44,7 +44,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..273a452 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -411,9 +411,11 @@ 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_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 0178ba4..b898486 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -62,36 +62,40 @@ static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv)
 
 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;
+	/* 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;
 	}
-
-	/* 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;
+	
+	/* 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
+			 */
 	}
-
-	/* 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);
+	
+	/*
+	* 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;
+	
 }
 
 static void gen8_guc_raise_irq(struct intel_guc *guc)
@@ -106,6 +110,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 +258,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 +341,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 +431,17 @@ 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 +450,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)
-- 
1.9.1

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

Re: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading module

[Srivatsa, Anusha]

>-----Original Message-----
>From: Sundaresan, Sujaritha
>Sent: Thursday, September 21, 2017 11:38 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>; Wajdeczko, Michal
><Michal.Wajdeczko@intel.com>; Srivatsa, Anusha <anusha.srivatsa@intel.com>;
>Mateo Lozano, Oscar <oscar.mateo@intel.com>; Ceraolo Spurio, Daniele
><daniele.ceraolospurio@intel.com>
>Subject: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading module
>
>We currently have two module parameters that control GuC:
>"enable_guc_loading" and "enable_guc_submission".
>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).
>
>v2: Clarifying the commit message (Anusha)
>v3: Unify seq_puts messages, correcting inconsistencies (Michal)
>v4: Rebased
>
>Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>Cc: Oscar Mateo <oscar.mateo@intel.com>
>Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>
>Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>---
> drivers/gpu/drm/i915/i915_debugfs.c     | 22 +++++----
> drivers/gpu/drm/i915/i915_drv.h         | 11 +++--
> 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      |  5 --
> drivers/gpu/drm/i915/i915_params.h      |  1 -
> drivers/gpu/drm/i915/intel_guc_loader.c |  6 ++-
> drivers/gpu/drm/i915/intel_uc.c         | 83 ++++++++++++++++++---------------
> 9 files changed, 73 insertions(+), 61 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>b/drivers/gpu/drm/i915/i915_debugfs.c
>index ca6fa6d..063fbe3 100644
>--- a/drivers/gpu/drm/i915/i915_debugfs.c
>+++ b/drivers/gpu/drm/i915/i915_debugfs.c
>@@ -1616,7 +1616,7 @@ static int i915_fbc_status(struct seq_file *m, void
>*unused)
> 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
>
> 	if (!HAS_FBC(dev_priv)) {
>-		seq_puts(m, "FBC unsupported on this chipset\n");
>+		seq_puts(m, "not supported\n");
> 		return 0;
> 	}
>
>@@ -1783,7 +1783,7 @@ static int i915_ring_freq_table(struct seq_file *m, void
>*unused)
> 	unsigned int max_gpu_freq, min_gpu_freq;
>
> 	if (!HAS_LLC(dev_priv)) {
>-		seq_puts(m, "unsupported on this chipset\n");
>+		seq_puts(m, "not supported\n");
> 		return 0;
> 	}
>
>@@ -2336,8 +2336,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, "not supported\n");
> 		return 0;
>+	}
>+
>
> 	seq_puts(m, "HuC firmware status:\n");
> 	seq_printf(m, "\tpath: %s\n", huc_fw->path); @@ -2369,8 +2372,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, "not supported\n");
> 		return 0;
>+	}
>+
>
> 	seq_printf(m, "GuC firmware status:\n");
> 	seq_printf(m, "\tpath: %s\n",
>@@ -2465,7 +2471,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;
>@@ -2654,7 +2660,7 @@ static int i915_edp_psr_status(struct seq_file *m, void
>*data)
> 	bool enabled = false;
>
> 	if (!HAS_PSR(dev_priv)) {
>-		seq_puts(m, "PSR not supported\n");
>+		seq_puts(m, "not supported\n");
> 		return 0;
> 	}
>
>@@ -2807,7 +2813,7 @@ static int i915_runtime_pm_status(struct seq_file *m,
>void *unused)
> 	struct pci_dev *pdev = dev_priv->drm.pdev;
>
> 	if (!HAS_RUNTIME_PM(dev_priv))
>-		seq_puts(m, "Runtime power management not supported\n");
>+		seq_puts(m, "not supported\n");
>
> 	seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->gt.awake));
> 	seq_printf(m, "IRQs disabled: %s\n",
>@@ -3683,7 +3689,7 @@ static void drrs_status_per_crtc(struct seq_file *m,
> 		mutex_unlock(&drrs->mutex);
> 	} else {
> 		/* DRRS not supported. Print the VBT parameter*/
>-		seq_puts(m, "\tDRRS Supported : No");
>+		seq_puts(m, "not supported\n");
> 	}
> 	seq_puts(m, "\n");
> }
>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>index 6d7d871..bd583f7 100644
>--- a/drivers/gpu/drm/i915/i915_drv.h
>+++ b/drivers/gpu/drm/i915/i915_drv.h
>@@ -3138,11 +3138,14 @@ static inline unsigned int
>i915_sg_segment_size(void)
>  * 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)))

What if there is GuC and we don't want to use it?  The above NEEDS_GUC_LOADING is still going to load it because it is available. So maybe there should only be:

#define NEEDS_GUC_LOADING(dev_priv) \
	(i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv))
That is, load guc since guc submission is enabled or we need guc to authenticate HuC.

Anusha
> #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 58a2a44..aaf3c03 100644
>--- a/drivers/gpu/drm/i915/i915_gem_context.c
>+++ b/drivers/gpu/drm/i915/i915_gem_context.c
>@@ -314,7 +314,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 731ce22..c1aeefb 100644
>--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>@@ -3292,7 +3292,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 4d0e8f7..a151688 100644
>--- a/drivers/gpu/drm/i915/i915_irq.c
>+++ b/drivers/gpu/drm/i915/i915_irq.c
>@@ -3962,7 +3962,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 ddda513..bd0571d 100644
>--- a/drivers/gpu/drm/i915/i915_params.c
>+++ b/drivers/gpu/drm/i915/i915_params.c
>@@ -63,7 +63,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,
>@@ -201,10 +200,6 @@ struct i915_params i915 __read_mostly = {
> 	"(0=use value from vbt [default], 1=low power swing(200mV),"
> 	"2=default swing(400mV))");
>
>-i915_param_named_unsafe(enable_guc_loading, int, 0400,
>-	"Enable GuC firmware loading "
>-	"(-1=auto, 0=never [default], 1=if available, 2=required)");
>-
> i915_param_named_unsafe(enable_guc_submission, int, 0400,
> 	"Enable GuC submission "
> 	"(-1=auto, 0=never [default], 1=if available, 2=required)"); diff --git
>a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
>index ac84470..e783d0b 100644
>--- a/drivers/gpu/drm/i915/i915_params.h
>+++ b/drivers/gpu/drm/i915/i915_params.h
>@@ -44,7 +44,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..273a452 100644
>--- a/drivers/gpu/drm/i915/intel_guc_loader.c
>+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>@@ -411,9 +411,11 @@ 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_uc.c b/drivers/gpu/drm/i915/intel_uc.c
>index 0178ba4..b898486 100644
>--- a/drivers/gpu/drm/i915/intel_uc.c
>+++ b/drivers/gpu/drm/i915/intel_uc.c
>@@ -62,36 +62,40 @@ static int __intel_uc_reset_hw(struct drm_i915_private
>*dev_priv)
>
> 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;
>+	/* 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;
> 	}
>-
>-	/* 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;
>+
>+	/* 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
>+			 */
> 	}
>-
>-	/* 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);
>+
>+	/*
>+	* 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;
>+
> }
>
> static void gen8_guc_raise_irq(struct intel_guc *guc) @@ -106,6 +110,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 +258,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 +341,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 +431,17 @@ 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 +450,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)
>--
>1.9.1

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

Re: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading module

[Sujaritha]

On 09/28/2017 03:36 PM, Srivatsa, Anusha wrote:
>
>> -----Original Message-----
>> From: Sundaresan, Sujaritha
>> Sent: Thursday, September 21, 2017 11:38 AM
>> To: intel-gfx@lists.freedesktop.org
>> Cc: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>; Wajdeczko, Michal
>> <Michal.Wajdeczko@intel.com>; Srivatsa, Anusha <anusha.srivatsa@intel.com>;
>> Mateo Lozano, Oscar <oscar.mateo@intel.com>; Ceraolo Spurio, Daniele
>> <daniele.ceraolospurio@intel.com>
>> Subject: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading module
>>
>> We currently have two module parameters that control GuC:
>> "enable_guc_loading" and "enable_guc_submission".
>> 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).
>>
>> v2: Clarifying the commit message (Anusha)
>> v3: Unify seq_puts messages, correcting inconsistencies (Michal)
>> v4: Rebased
>>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_debugfs.c     | 22 +++++----
>> drivers/gpu/drm/i915/i915_drv.h         | 11 +++--
>> 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      |  5 --
>> drivers/gpu/drm/i915/i915_params.h      |  1 -
>> drivers/gpu/drm/i915/intel_guc_loader.c |  6 ++-
>> drivers/gpu/drm/i915/intel_uc.c         | 83 ++++++++++++++++++---------------
>> 9 files changed, 73 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index ca6fa6d..063fbe3 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1616,7 +1616,7 @@ static int i915_fbc_status(struct seq_file *m, void
>> *unused)
>> 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
>>
>> 	if (!HAS_FBC(dev_priv)) {
>> -		seq_puts(m, "FBC unsupported on this chipset\n");
>> +		seq_puts(m, "not supported\n");
>> 		return 0;
>> 	}
>>
>> @@ -1783,7 +1783,7 @@ static int i915_ring_freq_table(struct seq_file *m, void
>> *unused)
>> 	unsigned int max_gpu_freq, min_gpu_freq;
>>
>> 	if (!HAS_LLC(dev_priv)) {
>> -		seq_puts(m, "unsupported on this chipset\n");
>> +		seq_puts(m, "not supported\n");
>> 		return 0;
>> 	}
>>
>> @@ -2336,8 +2336,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, "not supported\n");
>> 		return 0;
>> +	}
>> +
>>
>> 	seq_puts(m, "HuC firmware status:\n");
>> 	seq_printf(m, "\tpath: %s\n", huc_fw->path); @@ -2369,8 +2372,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, "not supported\n");
>> 		return 0;
>> +	}
>> +
>>
>> 	seq_printf(m, "GuC firmware status:\n");
>> 	seq_printf(m, "\tpath: %s\n",
>> @@ -2465,7 +2471,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;
>> @@ -2654,7 +2660,7 @@ static int i915_edp_psr_status(struct seq_file *m, void
>> *data)
>> 	bool enabled = false;
>>
>> 	if (!HAS_PSR(dev_priv)) {
>> -		seq_puts(m, "PSR not supported\n");
>> +		seq_puts(m, "not supported\n");
>> 		return 0;
>> 	}
>>
>> @@ -2807,7 +2813,7 @@ static int i915_runtime_pm_status(struct seq_file *m,
>> void *unused)
>> 	struct pci_dev *pdev = dev_priv->drm.pdev;
>>
>> 	if (!HAS_RUNTIME_PM(dev_priv))
>> -		seq_puts(m, "Runtime power management not supported\n");
>> +		seq_puts(m, "not supported\n");
>>
>> 	seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->gt.awake));
>> 	seq_printf(m, "IRQs disabled: %s\n",
>> @@ -3683,7 +3689,7 @@ static void drrs_status_per_crtc(struct seq_file *m,
>> 		mutex_unlock(&drrs->mutex);
>> 	} else {
>> 		/* DRRS not supported. Print the VBT parameter*/
>> -		seq_puts(m, "\tDRRS Supported : No");
>> +		seq_puts(m, "not supported\n");
>> 	}
>> 	seq_puts(m, "\n");
>> }
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 6d7d871..bd583f7 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3138,11 +3138,14 @@ static inline unsigned int
>> i915_sg_segment_size(void)
>>   * 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)))
> What if there is GuC and we don't want to use it?  The above NEEDS_GUC_LOADING is still going to load it because it is available. So maybe there should only be:
>
> #define NEEDS_GUC_LOADING(dev_priv) \
> 	(i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv))
> That is, load guc since guc submission is enabled or we need guc to authenticate HuC.
>
> Anusha
Good point. Will make this change.
>> #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 58a2a44..aaf3c03 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>> @@ -314,7 +314,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 731ce22..c1aeefb 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -3292,7 +3292,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 4d0e8f7..a151688 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -3962,7 +3962,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 ddda513..bd0571d 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -63,7 +63,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,
>> @@ -201,10 +200,6 @@ struct i915_params i915 __read_mostly = {
>> 	"(0=use value from vbt [default], 1=low power swing(200mV),"
>> 	"2=default swing(400mV))");
>>
>> -i915_param_named_unsafe(enable_guc_loading, int, 0400,
>> -	"Enable GuC firmware loading "
>> -	"(-1=auto, 0=never [default], 1=if available, 2=required)");
>> -
>> i915_param_named_unsafe(enable_guc_submission, int, 0400,
>> 	"Enable GuC submission "
>> 	"(-1=auto, 0=never [default], 1=if available, 2=required)"); diff --git
>> a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
>> index ac84470..e783d0b 100644
>> --- a/drivers/gpu/drm/i915/i915_params.h
>> +++ b/drivers/gpu/drm/i915/i915_params.h
>> @@ -44,7 +44,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..273a452 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>> @@ -411,9 +411,11 @@ 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_uc.c b/drivers/gpu/drm/i915/intel_uc.c
>> index 0178ba4..b898486 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -62,36 +62,40 @@ static int __intel_uc_reset_hw(struct drm_i915_private
>> *dev_priv)
>>
>> 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;
>> +	/* 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;
>> 	}
>> -
>> -	/* 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;
>> +
>> +	/* 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
>> +			 */
>> 	}
>> -
>> -	/* 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);
>> +
>> +	/*
>> +	* 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;
>> +
>> }
>>
>> static void gen8_guc_raise_irq(struct intel_guc *guc) @@ -106,6 +110,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 +258,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 +341,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 +431,17 @@ 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 +450,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)
>> --
>> 1.9.1
Thanks for the review.

Regards,

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

Re: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading module

[Michal Wajdeczko]

On Fri, 29 Sep 2017 00:36:56 +0200, Srivatsa, Anusha  
<anusha.srivatsa@intel.com> wrote:

>
>
>> -----Original Message-----
>> From: Sundaresan, Sujaritha
>> Sent: Thursday, September 21, 2017 11:38 AM
>> To: intel-gfx@lists.freedesktop.org
>> Cc: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>; Wajdeczko,  
>> Michal
>> <Michal.Wajdeczko@intel.com>; Srivatsa, Anusha  
>> <anusha.srivatsa@intel.com>;
>> Mateo Lozano, Oscar <oscar.mateo@intel.com>; Ceraolo Spurio, Daniele
>> <daniele.ceraolospurio@intel.com>
>> Subject: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading  
>> module
>>
>> We currently have two module parameters that control GuC:
>> "enable_guc_loading" and "enable_guc_submission".
>> 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).
>>
>> v2: Clarifying the commit message (Anusha)
>> v3: Unify seq_puts messages, correcting inconsistencies (Michal)
>> v4: Rebased
>>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> ---

  snip

>> diff --git a/drivers/gpu/drm/i915/i915_drv.h  
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 6d7d871..bd583f7 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3138,11 +3138,14 @@ static inline unsigned int
>> i915_sg_segment_size(void)
>>  * 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)))
>
> What if there is GuC and we don't want to use it?  The above  
> NEEDS_GUC_LOADING is still going to load it because it is available. So  
> maybe there should only be:
>
> #define NEEDS_GUC_LOADING(dev_priv) \
> 	(i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv))
> That is, load guc since guc submission is enabled or we need guc to  
> authenticate HuC.
>

Note that we used HAS_GUC as prerequisite that is then followed by logical
operator AND what guarantees us that we will try to load Guc only if its
available. In your modified macro we will try to load Guc just due to user
provided enable_guc_submission modparam which may not match hw caps.

On other hand we can try to rely on earlier modparam sanitization but I  
would
rather not trust that too much and keep our macro fully independent.

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

Re: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading module

[Srivatsa, Anusha]

>-----Original Message-----
>From: Wajdeczko, Michal
>Sent: Friday, September 29, 2017 12:10 AM
>To: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>; intel-
>gfx@lists.freedesktop.org; Srivatsa, Anusha <anusha.srivatsa@intel.com>
>Cc: Wajdeczko, Michal <Michal.Wajdeczko@intel.com>; Mateo Lozano, Oscar
><oscar.mateo@intel.com>; Ceraolo Spurio, Daniele
><daniele.ceraolospurio@intel.com>
>Subject: Re: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading
>module
>
>On Fri, 29 Sep 2017 00:36:56 +0200, Srivatsa, Anusha
><anusha.srivatsa@intel.com> wrote:
>
>>
>>
>>> -----Original Message-----
>>> From: Sundaresan, Sujaritha
>>> Sent: Thursday, September 21, 2017 11:38 AM
>>> To: intel-gfx@lists.freedesktop.org
>>> Cc: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>; Wajdeczko,
>>> Michal
>>> <Michal.Wajdeczko@intel.com>; Srivatsa, Anusha
>>> <anusha.srivatsa@intel.com>;
>>> Mateo Lozano, Oscar <oscar.mateo@intel.com>; Ceraolo Spurio, Daniele
>>> <daniele.ceraolospurio@intel.com>
>>> Subject: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading
>>> module
>>>
>>> We currently have two module parameters that control GuC:
>>> "enable_guc_loading" and "enable_guc_submission".
>>> 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).
>>>
>>> v2: Clarifying the commit message (Anusha)
>>> v3: Unify seq_puts messages, correcting inconsistencies (Michal)
>>> v4: Rebased
>>>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>
>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>>> ---
>
>  snip
>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index 6d7d871..bd583f7 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -3138,11 +3138,14 @@ static inline unsigned int
>>> i915_sg_segment_size(void)
>>>  * 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)))
>>
>> What if there is GuC and we don't want to use it?  The above
>> NEEDS_GUC_LOADING is still going to load it because it is available. So
>> maybe there should only be:
>>
>> #define NEEDS_GUC_LOADING(dev_priv) \
>> 	(i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv))
>> That is, load guc since guc submission is enabled or we need guc to
>> authenticate HuC.
>>
>
>Note that we used HAS_GUC as prerequisite that is then followed by logical
>operator AND what guarantees us that we will try to load Guc only if its
>available. In your modified macro we will try to load Guc just due to user
>provided enable_guc_submission modparam which may not match hw caps.
>
>On other hand we can try to rely on earlier modparam sanitization but I
>would
>rather not trust that too much and keep our macro fully independent.

Yes, you are right.

Anusha

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

Re: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading module

[Sujaritha]

On 09/29/2017 12:10 AM, Michal Wajdeczko wrote:
> On Fri, 29 Sep 2017 00:36:56 +0200, Srivatsa, Anusha 
> <anusha.srivatsa@intel.com> wrote:
>
>>
>>
>>> -----Original Message-----
>>> From: Sundaresan, Sujaritha
>>> Sent: Thursday, September 21, 2017 11:38 AM
>>> To: intel-gfx@lists.freedesktop.org
>>> Cc: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>; 
>>> Wajdeczko, Michal
>>> <Michal.Wajdeczko@intel.com>; Srivatsa, Anusha 
>>> <anusha.srivatsa@intel.com>;
>>> Mateo Lozano, Oscar <oscar.mateo@intel.com>; Ceraolo Spurio, Daniele
>>> <daniele.ceraolospurio@intel.com>
>>> Subject: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading 
>>> module
>>>
>>> We currently have two module parameters that control GuC:
>>> "enable_guc_loading" and "enable_guc_submission".
>>> 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).
>>>
>>> v2: Clarifying the commit message (Anusha)
>>> v3: Unify seq_puts messages, correcting inconsistencies (Michal)
>>> v4: Rebased
>>>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>
>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>>> ---
>
>  snip
Will do.
>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index 6d7d871..bd583f7 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -3138,11 +3138,14 @@ static inline unsigned int
>>> i915_sg_segment_size(void)
>>>  * 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)))
>>
>> What if there is GuC and we don't want to use it?  The above 
>> NEEDS_GUC_LOADING is still going to load it because it is available. 
>> So maybe there should only be:
>>
>> #define NEEDS_GUC_LOADING(dev_priv) \
>>     (i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv))
>> That is, load guc since guc submission is enabled or we need guc to 
>> authenticate HuC.
>>
>
> Note that we used HAS_GUC as prerequisite that is then followed by 
> logical
> operator AND what guarantees us that we will try to load Guc only if its
> available. In your modified macro we will try to load Guc just due to 
> user
> provided enable_guc_submission modparam which may not match hw caps.
>
> On other hand we can try to rely on earlier modparam sanitization but 
> I would
> rather not trust that too much and keep our macro fully independent.
>
> Michal

Yes, this is a good point.

Thanks for the review.

Regards,

Sujaritha

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