[PATCH 0/2] drm/i915/guc : Removing GuC loading and submission modparams

[PATCH 0/2] drm/i915/guc : Removing GuC loading and submission modparams

[Sujaritha Sundaresan]

The aim of this series is to remove the enable_guc_loading and
enable_guc_submission module parameters. They are being replaced
by a common, newly defined enable_guc module parameter.

This series has been split from a bigger series that has previously
been sent to this mailing list.

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>

Sujaritha Sundaresan (2):
  drm/i915/guc : Removing enable_guc_loading and enable_guc_submission
    module parameters
  drm/i915/guc : Updating GuC and HuC firmware select function

 drivers/gpu/drm/i915/i915_debugfs.c     | 64 +++++++++++++---------
 drivers/gpu/drm/i915/i915_drv.h         | 12 +++--
 drivers/gpu/drm/i915/i915_gem_context.c |  4 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c     |  2 +-
 drivers/gpu/drm/i915/i915_irq.c         |  4 +-
 drivers/gpu/drm/i915/i915_params.c      | 11 ++--
 drivers/gpu/drm/i915/i915_params.h      |  3 +-
 drivers/gpu/drm/i915/intel_guc.c        |  2 +-
 drivers/gpu/drm/i915/intel_guc_fw.c     | 12 ++---
 drivers/gpu/drm/i915/intel_guc_fw.h     |  2 +-
 drivers/gpu/drm/i915/intel_guc_log.c    |  6 +--
 drivers/gpu/drm/i915/intel_huc.c        |  8 +--
 drivers/gpu/drm/i915/intel_uc.c         | 95 +++++++++++++++++----------------
 13 files changed, 127 insertions(+), 103 deletions(-)

-- 
1.9.1

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

[PATCH v10 1/2] drm/i915/guc : Removing enable_guc_loading and enable_guc_submission module parameters

[Sujaritha Sundaresan]

We currently have two module parameters that control GuC:
"enable_guc_loading" and "enable_guc_submission". Whenever
we need submission=1, we also need loading=1.We also need
loading=1 when we want to want to verify the HuC, which
is every time we have a HuC (but all platforms with HuC
have a GuC and viceversa).

The above module parameters are being replaced by a single
enable_guc modparam.

v2: Clarifying the commit message (Anusha)

v3: Unify seq_puts messages, Re-factoring code as per review (Michal)

v4: Rebase

v5: Separating message unification into a separate patch

v6: Re-factoring code (Sagar, Michal)
    Rebase

v7: Applying review comments (Sagar)
    Rebase

v8: Change to NEEDS_GUC_FW (Chris)
    Applying review comments (Michal)
    Clarifying commit message (Joonas)

v9: Applying review comments (Michal)

v10: Introducing enable_guc modparam
	 Applying review comments (Michal)

Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c     |  6 +--
 drivers/gpu/drm/i915/i915_drv.h         | 12 +++--
 drivers/gpu/drm/i915/i915_gem_context.c |  4 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c     |  2 +-
 drivers/gpu/drm/i915/i915_irq.c         |  4 +-
 drivers/gpu/drm/i915/i915_params.c      | 11 ++--
 drivers/gpu/drm/i915/i915_params.h      |  3 +-
 drivers/gpu/drm/i915/intel_guc.c        |  2 +-
 drivers/gpu/drm/i915/intel_guc_log.c    |  6 +--
 drivers/gpu/drm/i915/intel_uc.c         | 96 ++++++++++++++++++++++----------------
 10 files changed, 77 insertions(+), 69 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index cb3e5aa..c12452d 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2360,7 +2360,7 @@ 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 drm_printer p;
 
-	if (!HAS_HUC_UCODE(dev_priv)) {
+	if (!HAS_HUC(dev_priv)) {
 		seq_puts(m, "not supported\n");
 		return 0;
 	}
@@ -2381,7 +2381,7 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
 	struct drm_printer p;
 	u32 tmp, i;
 
-	if (!HAS_GUC_UCODE(dev_priv)) {
+	if (!HAS_GUC(dev_priv)) {
 		seq_puts(m, "not supported\n");
 		return 0;
 	}
@@ -2466,7 +2466,7 @@ static bool check_guc_submission(struct seq_file *m)
 		seq_printf(m, "GuC submission %s\n",
 				HAS_GUC(dev_priv) ?
 				"not supported" :
-				HAS_GUC_SCHED(dev_priv) ?
+				NEEDS_GUC_LOAD(dev_priv) ?
 				"disabled" :
 				"failed");
 		return false;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2b76625..c4e1c7e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3220,10 +3220,16 @@ static inline unsigned int i915_sg_segment_size(void)
  * properties, so we have separate macros to test them.
  */
 #define HAS_GUC(dev_priv)	((dev_priv)->info.has_guc)
+#define HAS_HUC(dev_priv)	(HAS_GUC(dev_priv))
 #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_FW(dev_priv) \
+	((dev_priv)->guc.fw.fetch_status == INTEL_UC_FIRMWARE_SUCCESS)
+#define HAS_HUC_FW(dev_priv) \
+	((dev_priv)->huc.fw.fetch_status == INTEL_UC_FIRMWARE_SUCCESS)
+
+#define NEEDS_GUC_LOAD(dev_priv) \
+	(HAS_GUC(dev_priv) && HAS_GUC_FW(dev_priv) && \
+	(HAS_HUC_FW(dev_priv) || i915_modparams.enable_guc))
 
 #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 c1efbaf..f9240dd 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -316,7 +316,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_modparams.enable_guc_loading)
+	if (NEEDS_GUC_LOAD(dev_priv))
 		ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
 	else
 		ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
@@ -409,7 +409,7 @@ struct i915_gem_context *
 	i915_gem_context_set_closed(ctx); /* not user accessible */
 	i915_gem_context_clear_bannable(ctx);
 	i915_gem_context_set_force_single_submission(ctx);
-	if (!i915_modparams.enable_guc_submission)
+	if (!i915_modparams.enable_guc)
 		ctx->ring_size = 512 * PAGE_SIZE; /* Max ring buffer size */
 
 	GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index f92a39f..9892239 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3503,7 +3503,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_modparams.enable_guc_loading) {
+	if (NEEDS_GUC_LOAD(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 4fb183a..e3b6fe8 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1400,7 +1400,7 @@ static void snb_gt_irq_handler(struct drm_i915_private *dev_priv,
 
 	if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) {
 		notify_ring(engine);
-		tasklet |= i915_modparams.enable_guc_submission;
+		tasklet |= i915_modparams.enable_guc;
 	}
 
 	if (tasklet)
@@ -4032,7 +4032,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 (HAS_GUC(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 b4faeb6..6a5ce47 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -162,13 +162,10 @@ struct i915_params i915_modparams __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)");
+i915_param_named_unsafe(enable_guc, int, 0400,
+	"Enable GuC submission and loading"
+	"(-1=auto [default], 0=disable GuC loading, 1=enable GuC submision"
+	"2=enable HuC, 3=enable GuC submission and HuC)");
 
 i915_param_named(guc_log_level, int, 0400,
 	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index c729226..7bf4dce 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -44,8 +44,7 @@
 	param(int, disable_power_well, -1) \
 	param(int, enable_ips, 1) \
 	param(int, invert_brightness, 0) \
-	param(int, enable_guc_loading, 0) \
-	param(int, enable_guc_submission, 0) \
+	param(int, enable_guc, -1) \
 	param(int, guc_log_level, -1) \
 	param(char *, guc_firmware_path, NULL) \
 	param(char *, huc_firmware_path, NULL) \
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 823d0c2..629ef5d 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -128,7 +128,7 @@ void intel_guc_init_params(struct intel_guc *guc)
 	}
 
 	/* If GuC submission is enabled, set up additional parameters here */
-	if (i915_modparams.enable_guc_submission) {
+	if (i915_modparams.enable_guc) {
 		u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
 		u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool);
 		u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 76d3eb1..6d7823b 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -505,7 +505,7 @@ static void guc_flush_logs(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
-	if (!i915_modparams.enable_guc_submission ||
+	if (!HAS_GUC_FW(dev_priv) ||
 	    (i915_modparams.guc_log_level < 0))
 		return;
 
@@ -646,7 +646,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 (!i915_modparams.enable_guc_submission ||
+	if (!HAS_GUC_FW(dev_priv) ||
 	    (i915_modparams.guc_log_level < 0))
 		return;
 
@@ -657,7 +657,7 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv)
 
 void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 {
-	if (!i915_modparams.enable_guc_submission)
+	if (!HAS_GUC_FW(dev_priv))
 		return;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 1e2a30a..233f680 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -47,38 +47,45 @@ static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
+/*
+ * With enable_guc defined as follows:
+ *
+ * -1=auto [default]
+ *  0=disable GuC loading
+ *  1=enable GuC submission
+ *  2=enable HuC
+ *  3=enable GuC submission and HuC
+ */
+
 void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 {
-	if (!HAS_GUC(dev_priv)) {
-		if (i915_modparams.enable_guc_loading > 0 ||
-		    i915_modparams.enable_guc_submission > 0)
-			DRM_INFO("Ignoring GuC options, no hardware\n");
-
-		i915_modparams.enable_guc_loading = 0;
-		i915_modparams.enable_guc_submission = 0;
-		return;
+	int auto_enable_guc = 0;
+	// note that in the future this can be defined in more granular way
+	// int auto_enable_guc = !HAS_GUC_FW(dev_priv) ? 0 :
+	//					IS_GEN9(dev_priv) ? 1 :
+	//                  IS_GEN10(dev_priv) ? 2:
+	//                  3;
+
+	/* Making sure that our auto is well defined */
+	GEM_BUG_ON(auto_enable_guc < 0);
+	GEM_BUG_ON((auto_enable_guc > 0) && !HAS_GUC_FW(dev_priv));
+	GEM_BUG_ON((auto_enable_guc & 2) && !HAS_HUC_FW(dev_priv));
+
+	if (i915_modparams.enable_guc < 0)
+		i915_modparams.enable_guc = auto_enable_guc;
+
+	if (i915_modparams.enable_guc > 0) {
+		if (!HAS_GUC(dev_priv) || !HAS_GUC_FW(dev_priv)) {
+			DRM_INFO("Ignoring option enable_guc=%d - %s\n",
+					 i915_modparams.enable_guc,
+				     !HAS_GUC(dev_priv) ? "no GuC hardware" :
+					 "no GuC firmware");
+			i915_modparams.enable_guc = 0;
+		}
 	}
 
-	/* A negative value means "use platform default" */
-	if (i915_modparams.enable_guc_loading < 0)
-		i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
-
-	/* Verify firmware version */
-	if (i915_modparams.enable_guc_loading) {
-		if (HAS_HUC_UCODE(dev_priv))
-			intel_huc_select_fw(&dev_priv->huc);
-
-		if (intel_guc_fw_select(&dev_priv->guc))
-			i915_modparams.enable_guc_loading = 0;
+	if (i915_modparams.enable_guc & 2) {
+		if (!HAS_HUC_FW(dev_priv)) {
+			DRM_INFO("Ignoring option enable_guc=%d - %s\n",
+				i915_modparams.enable_guc, "no HuC firmware");
+			i915_modparams.enable_guc = 0;
+		}
 	}
-
-	/* Can't enable guc submission without guc loaded */
-	if (!i915_modparams.enable_guc_loading)
-		i915_modparams.enable_guc_submission = 0;
-
-	/* A negative value means "use platform default" */
-	if (i915_modparams.enable_guc_submission < 0)
-		i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
 }
 
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
@@ -154,7 +161,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	struct intel_guc *guc = &dev_priv->guc;
 	int ret, attempts;
 
-	if (!i915_modparams.enable_guc_loading)
+	if (!NEEDS_GUC_LOAD(dev_priv))
 		return 0;
 
 	guc_disable_communication(guc);
@@ -163,7 +170,7 @@ 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);
 
-	if (i915_modparams.enable_guc_submission) {
+	if (i915_modparams.enable_guc) {
 		/*
 		 * This is stuff we need to have available at fw load time
 		 * if we are planning to enable submission later
@@ -213,7 +220,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 		goto err_log_capture;
 
 	intel_huc_auth(&dev_priv->huc);
-	if (i915_modparams.enable_guc_submission) {
+	if (i915_modparams.enable_guc) {
 		if (i915_modparams.guc_log_level >= 0)
 			gen9_enable_guc_interrupts(dev_priv);
 
@@ -223,7 +230,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	}
 
 	dev_info(dev_priv->drm.dev, "GuC %s (firmware %s [version %u.%u])\n",
-		 i915_modparams.enable_guc_submission ? "submission enabled" :
+		 i915_modparams.enable_guc ? "submission enabled" :
 							"loaded",
 		 guc->fw.path,
 		 guc->fw.major_ver_found, guc->fw.minor_ver_found);
@@ -245,27 +252,21 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 err_log_capture:
 	guc_capture_load_err_log(guc);
 err_submission:
-	if (i915_modparams.enable_guc_submission)
+	if (i915_modparams.enable_guc)
 		intel_guc_submission_fini(guc);
 err_guc:
 	i915_ggtt_disable_guc(dev_priv);
 
-	if (i915_modparams.enable_guc_loading > 1 ||
-	    i915_modparams.enable_guc_submission > 1) {
+	if (i915_modparams.enable_guc > 1) {
 		DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
 		ret = -EIO;
+	} else if (i915_modparams.enable_guc == 1) {
+		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
+		ret = 0;
 	} else {
-		DRM_NOTE("GuC init failed. Firmware loading disabled.\n");
 		ret = 0;
 	}
 
-	if (i915_modparams.enable_guc_submission) {
-		i915_modparams.enable_guc_submission = 0;
-		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
-	}
-
-	i915_modparams.enable_guc_loading = 0;
-
 	return ret;
 }
 
@@ -275,15 +276,15 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 
 	guc_free_load_err_log(guc);
 
-	if (!i915_modparams.enable_guc_loading)
+	if (!NEEDS_GUC_LOAD(dev_priv))
 		return;
 
-	if (i915_modparams.enable_guc_submission)
+	if (i915_modparams.enable_guc)
 		intel_guc_submission_disable(guc);
 
 	guc_disable_communication(guc);
 
-	if (i915_modparams.enable_guc_submission) {
+	if (i915_modparams.enable_guc) {
 		gen9_disable_guc_interrupts(dev_priv);
 		intel_guc_submission_fini(guc);
 	}
-- 
1.9.1

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

Re: [PATCH v10 1/2] drm/i915/guc : Removing enable_guc_loading and enable_guc_submission module parameters

[Joonas Lahtinen]

+ Li (for WOPCM)

On Mon, 2017-11-27 at 11:54 -0800, Sujaritha Sundaresan wrote:
> We currently have two module parameters that control GuC:
> "enable_guc_loading" and "enable_guc_submission". Whenever
> we need submission=1, we also need loading=1.We also need
> loading=1 when we want to want to verify the HuC, which
> is every time we have a HuC (but all platforms with HuC
> have a GuC and viceversa).
> 
> The above module parameters are being replaced by a single
> enable_guc modparam.
> 
> v2: Clarifying the commit message (Anusha)
> 
> v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
> 
> v4: Rebase
> 
> v5: Separating message unification into a separate patch
> 
> v6: Re-factoring code (Sagar, Michal)
>     Rebase
> 
> v7: Applying review comments (Sagar)
>     Rebase
> 
> v8: Change to NEEDS_GUC_FW (Chris)
>     Applying review comments (Michal)
>     Clarifying commit message (Joonas)
> 
> v9: Applying review comments (Michal)
> 
> v10: Introducing enable_guc modparam
> 	 Applying review comments (Michal)
> 
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>

<SNIP>

> @@ -2466,7 +2466,7 @@ static bool check_guc_submission(struct seq_file *m)
>  		seq_printf(m, "GuC submission %s\n",
>  				HAS_GUC(dev_priv) ?
>  				"not supported" :
> -				HAS_GUC_SCHED(dev_priv) ?
> +				NEEDS_GUC_LOAD(dev_priv) ?
>  				"disabled" :
>  				"failed");

I do not quite follow the logic, here. Even the old logic is reversed?

This patch doesn't seem to be on top of drm-tip, so whatever patch this
was written on top of, needs fixing too.

When sending patches, make sure they actually build on top of drm-tip
not to waste the reviewers' time.

> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3220,10 +3220,16 @@ static inline unsigned int i915_sg_segment_size(void)
>   * properties, so we have separate macros to test them.
>   */
>  #define HAS_GUC(dev_priv)	((dev_priv)->info.has_guc)
> +#define HAS_HUC(dev_priv)	(HAS_GUC(dev_priv))
>  #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_FW(dev_priv) \
> +	((dev_priv)->guc.fw.fetch_status == INTEL_UC_FIRMWARE_SUCCESS)
> +#define HAS_HUC_FW(dev_priv) \
> +	((dev_priv)->huc.fw.fetch_status == INTEL_UC_FIRMWARE_SUCCESS)

This is not a good candidate for HAS_* function as it's a rather
dynamic state. Imagine the HAS_*/IS_* functions as something you may
want to freeze at compile state.

> +
> +#define NEEDS_GUC_LOAD(dev_priv) \
> +	(HAS_GUC(dev_priv) && HAS_GUC_FW(dev_priv) && \
> +	(HAS_HUC_FW(dev_priv) || i915_modparams.enable_guc))

This even less so.

> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -316,7 +316,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_modparams.enable_guc_loading)
> +	if (NEEDS_GUC_LOAD(dev_priv))
>  		ctx->ggtt_offset_bias = GUC_WOPCM_TOP;

We should really make up our mind when we actually need to avoid the
WOPCM. According to Li, it's not bound to the usage of the
microcontrollers but rather their existence.

So the correct condition would be just HAS_GUC()?

> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3503,7 +3503,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_modparams.enable_guc_loading) {
> +	if (NEEDS_GUC_LOAD(dev_priv)) {
>  		ggtt->base.total = min_t(u64, ggtt->base.total, GUC_GGTT_TOP);
>  		ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total);
>  	}

I guess same question applies here. Also, even this function was
correctly named something like intel_guc_needs_loading(), it'd have to
be much less dynamic because we're really not going to run this code
multiple times.

> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -47,38 +47,45 @@ static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> +/*
> + * With enable_guc defined as follows:
> + *
> + * -1=auto [default]
> + *  0=disable GuC loading
> + *  1=enable GuC submission
> + *  2=enable HuC
> + *  3=enable GuC submission and HuC
> + */
> +
>  void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>  {
> -	if (!HAS_GUC(dev_priv)) {
> -		if (i915_modparams.enable_guc_loading > 0 ||
> -		    i915_modparams.enable_guc_submission > 0)
> -			DRM_INFO("Ignoring GuC options, no hardware\n");
> -
> -		i915_modparams.enable_guc_loading = 0;
> -		i915_modparams.enable_guc_submission = 0;
> -		return;
> +	int auto_enable_guc = 0;
> +	// note that in the future this can be defined in more granular way
> +	// int auto_enable_guc = !HAS_GUC_FW(dev_priv) ? 0 :
> +	//					IS_GEN9(dev_priv) ? 1 :
> +	//                  IS_GEN10(dev_priv) ? 2:
> +	//                  3;

Please, no C++ style comments (you don't see them around, do you), and
the pseudocode seems overly detailed for pseudocode.

> +
> +	/* Making sure that our auto is well defined */
> +	GEM_BUG_ON(auto_enable_guc < 0);
> +	GEM_BUG_ON((auto_enable_guc > 0) && !HAS_GUC_FW(dev_priv));
> +	GEM_BUG_ON((auto_enable_guc & 2) && !HAS_HUC_FW(dev_priv));
> +
> +	if (i915_modparams.enable_guc < 0)
> +		i915_modparams.enable_guc = auto_enable_guc;
> +
> +	if (i915_modparams.enable_guc > 0) {
> +		if (!HAS_GUC(dev_priv) || !HAS_GUC_FW(dev_priv)) {
> +			DRM_INFO("Ignoring option enable_guc=%d - %s\n",
> +					 i915_modparams.enable_guc,
> +				     !HAS_GUC(dev_priv) ? "no GuC hardware" :
> +					 "no GuC firmware");
> +			i915_modparams.enable_guc = 0;
> +		}

The indents are way off. As this patch is already at revision 10, I'm
stopping here.

I would suggest starting with some smaller and less "controversial"
patches to get to know the codebase, coding style and other concepts.
It will make your life much more pleasant working on patches that make
actual forward progress.

It is not a good learning experience, and must be frustrating to try to
compose a patch entirely based on the review responses without having
the time to get to know the code.

So I suggest Michal or Sagar will take over this patch, giving it an
overhaul. That's the best action I can see for now as this is standing
between drm-tip and the rest of the GuC patches.

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

Re: [PATCH v10 1/2] drm/i915/guc : Removing enable_guc_loading and enable_guc_submission module parameters

[Sagar Arun Kamble]

On 11/28/2017 1:24 AM, Sujaritha Sundaresan wrote:
> We currently have two module parameters that control GuC:
> "enable_guc_loading" and "enable_guc_submission". Whenever
> we need submission=1, we also need loading=1.We also need
> loading=1 when we want to want to verify the HuC, which
> is every time we have a HuC (but all platforms with HuC
> have a GuC and viceversa).
>
> The above module parameters are being replaced by a single
> enable_guc modparam.
>
> v2: Clarifying the commit message (Anusha)
>
> v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
>
> v4: Rebase
>
> v5: Separating message unification into a separate patch
>
> v6: Re-factoring code (Sagar, Michal)
>      Rebase
>
> v7: Applying review comments (Sagar)
>      Rebase
>
> v8: Change to NEEDS_GUC_FW (Chris)
>      Applying review comments (Michal)
>      Clarifying commit message (Joonas)
>
> v9: Applying review comments (Michal)
>
> v10: Introducing enable_guc modparam
> 	 Applying review comments (Michal)
>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c     |  6 +--
>   drivers/gpu/drm/i915/i915_drv.h         | 12 +++--
>   drivers/gpu/drm/i915/i915_gem_context.c |  4 +-
>   drivers/gpu/drm/i915/i915_gem_gtt.c     |  2 +-
>   drivers/gpu/drm/i915/i915_irq.c         |  4 +-
>   drivers/gpu/drm/i915/i915_params.c      | 11 ++--
>   drivers/gpu/drm/i915/i915_params.h      |  3 +-
>   drivers/gpu/drm/i915/intel_guc.c        |  2 +-
>   drivers/gpu/drm/i915/intel_guc_log.c    |  6 +--
>   drivers/gpu/drm/i915/intel_uc.c         | 96 ++++++++++++++++++++++----------------
>   10 files changed, 77 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index cb3e5aa..c12452d 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2360,7 +2360,7 @@ 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 drm_printer p;
>   
> -	if (!HAS_HUC_UCODE(dev_priv)) {
> +	if (!HAS_HUC(dev_priv)) {
>   		seq_puts(m, "not supported\n");
>   		return 0;
>   	}
> @@ -2381,7 +2381,7 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
>   	struct drm_printer p;
>   	u32 tmp, i;
>   
> -	if (!HAS_GUC_UCODE(dev_priv)) {
> +	if (!HAS_GUC(dev_priv)) {
>   		seq_puts(m, "not supported\n");
>   		return 0;
>   	}
> @@ -2466,7 +2466,7 @@ static bool check_guc_submission(struct seq_file *m)
>   		seq_printf(m, "GuC submission %s\n",
>   				HAS_GUC(dev_priv) ?
This HAS_GUC check is not present in drm-tip. Please rebase the patch.
>   				"not supported" :
> -				HAS_GUC_SCHED(dev_priv) ?
> +				NEEDS_GUC_LOAD(dev_priv) ?
>   				"disabled" :
>   				"failed");
>   		return false;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2b76625..c4e1c7e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3220,10 +3220,16 @@ static inline unsigned int i915_sg_segment_size(void)
>    * properties, so we have separate macros to test them.
>    */
>   #define HAS_GUC(dev_priv)	((dev_priv)->info.has_guc)
> +#define HAS_HUC(dev_priv)	(HAS_GUC(dev_priv))
>   #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_FW(dev_priv) \
> +	((dev_priv)->guc.fw.fetch_status == INTEL_UC_FIRMWARE_SUCCESS)
> +#define HAS_HUC_FW(dev_priv) \
> +	((dev_priv)->huc.fw.fetch_status == INTEL_UC_FIRMWARE_SUCCESS)
> +
> +#define NEEDS_GUC_LOAD(dev_priv) \
> +	(HAS_GUC(dev_priv) && HAS_GUC_FW(dev_priv) && \
> +	(HAS_HUC_FW(dev_priv) || i915_modparams.enable_guc))
Indentation does not look right. A space needed before "(HAS_HUC_FW*"
>   
>   #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 c1efbaf..f9240dd 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -316,7 +316,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_modparams.enable_guc_loading)
> +	if (NEEDS_GUC_LOAD(dev_priv))
>   		ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
>   	else
>   		ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
> @@ -409,7 +409,7 @@ struct i915_gem_context *
>   	i915_gem_context_set_closed(ctx); /* not user accessible */
>   	i915_gem_context_clear_bannable(ctx);
>   	i915_gem_context_set_force_single_submission(ctx);
> -	if (!i915_modparams.enable_guc_submission)
> +	if (!i915_modparams.enable_guc)
This case applies only if GuC is being used for submission. So we should 
check
(enable_guc & 1).
Defining macro for 1, 2 will be good. something like:
#define NEEDS_GUC_SUBMISSION     BIT(0)
#define NEEDS_HUC_LOADING            BIT(1)
>   		ctx->ring_size = 512 * PAGE_SIZE; /* Max ring buffer size */
>   
>   	GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index f92a39f..9892239 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3503,7 +3503,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_modparams.enable_guc_loading) {
> +	if (NEEDS_GUC_LOAD(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 4fb183a..e3b6fe8 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1400,7 +1400,7 @@ static void snb_gt_irq_handler(struct drm_i915_private *dev_priv,
>   
>   	if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) {
>   		notify_ring(engine);
> -		tasklet |= i915_modparams.enable_guc_submission;
> +		tasklet |= i915_modparams.enable_guc;
Also here.
>   	}
>   
>   	if (tasklet)
> @@ -4032,7 +4032,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 (HAS_GUC(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 b4faeb6..6a5ce47 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -162,13 +162,10 @@ struct i915_params i915_modparams __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)");
> +i915_param_named_unsafe(enable_guc, int, 0400,
> +	"Enable GuC submission and loading"
How about "Enable GuC load for submission and/or HuC load"
> +	"(-1=auto [default], 0=disable GuC loading, 1=enable GuC submision"
> +	"2=enable HuC, 3=enable GuC submission and HuC)");
>   
>   i915_param_named(guc_log_level, int, 0400,
>   	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index c729226..7bf4dce 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -44,8 +44,7 @@
>   	param(int, disable_power_well, -1) \
>   	param(int, enable_ips, 1) \
>   	param(int, invert_brightness, 0) \
> -	param(int, enable_guc_loading, 0) \
> -	param(int, enable_guc_submission, 0) \
> +	param(int, enable_guc, -1) \
>   	param(int, guc_log_level, -1) \
>   	param(char *, guc_firmware_path, NULL) \
>   	param(char *, huc_firmware_path, NULL) \
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 823d0c2..629ef5d 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -128,7 +128,7 @@ void intel_guc_init_params(struct intel_guc *guc)
>   	}
>   
>   	/* If GuC submission is enabled, set up additional parameters here */
> -	if (i915_modparams.enable_guc_submission) {
> +	if (i915_modparams.enable_guc) {
This check should check only submission bit from this parameter.
>   		u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
>   		u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool);
>   		u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 76d3eb1..6d7823b 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -505,7 +505,7 @@ static void guc_flush_logs(struct intel_guc *guc)
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   
> -	if (!i915_modparams.enable_guc_submission ||
> +	if (!HAS_GUC_FW(dev_priv) ||
>   	    (i915_modparams.guc_log_level < 0))
>   		return;
>   
> @@ -646,7 +646,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 (!i915_modparams.enable_guc_submission ||
> +	if (!HAS_GUC_FW(dev_priv) ||
>   	    (i915_modparams.guc_log_level < 0))
>   		return;
>   
> @@ -657,7 +657,7 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv)
>   
>   void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>   {
> -	if (!i915_modparams.enable_guc_submission)
> +	if (!HAS_GUC_FW(dev_priv))
>   		return;
>   
>   	mutex_lock(&dev_priv->drm.struct_mutex);
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 1e2a30a..233f680 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -47,38 +47,45 @@ static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv)
>   	return ret;
>   }
>   
> +/*
> + * With enable_guc defined as follows:
> + *
> + * -1=auto [default]
> + *  0=disable GuC loading
> + *  1=enable GuC submission
> + *  2=enable HuC
> + *  3=enable GuC submission and HuC
> + */
> +
>   void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>   {
> -	if (!HAS_GUC(dev_priv)) {
> -		if (i915_modparams.enable_guc_loading > 0 ||
> -		    i915_modparams.enable_guc_submission > 0)
> -			DRM_INFO("Ignoring GuC options, no hardware\n");
> -
> -		i915_modparams.enable_guc_loading = 0;
> -		i915_modparams.enable_guc_submission = 0;
> -		return;
> +	int auto_enable_guc = 0;
> +	// note that in the future this can be defined in more granular way
> +	// int auto_enable_guc = !HAS_GUC_FW(dev_priv) ? 0 :
> +	//					IS_GEN9(dev_priv) ? 1 :
> +	//                  IS_GEN10(dev_priv) ? 2:
> +	//                  3;
Change the comment to comply with linux multi-line comment format.
> +
> +	/* Making sure that our auto is well defined */
> +	GEM_BUG_ON(auto_enable_guc < 0);
> +	GEM_BUG_ON((auto_enable_guc > 0) && !HAS_GUC_FW(dev_priv));
> +	GEM_BUG_ON((auto_enable_guc & 2) && !HAS_HUC_FW(dev_priv));
> +
> +	if (i915_modparams.enable_guc < 0)
> +		i915_modparams.enable_guc = auto_enable_guc;
> +
> +	if (i915_modparams.enable_guc > 0) {
> +		if (!HAS_GUC(dev_priv) || !HAS_GUC_FW(dev_priv)) {
> +			DRM_INFO("Ignoring option enable_guc=%d - %s\n",
> +					 i915_modparams.enable_guc,
> +				     !HAS_GUC(dev_priv) ? "no GuC hardware" :
> +					 "no GuC firmware");
> +			i915_modparams.enable_guc = 0;
> +		}
>   	}
>   
> -	/* A negative value means "use platform default" */
> -	if (i915_modparams.enable_guc_loading < 0)
> -		i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
> -
> -	/* Verify firmware version */
> -	if (i915_modparams.enable_guc_loading) {
> -		if (HAS_HUC_UCODE(dev_priv))
> -			intel_huc_select_fw(&dev_priv->huc);
> -
> -		if (intel_guc_fw_select(&dev_priv->guc))
> -			i915_modparams.enable_guc_loading = 0;
> +	if (i915_modparams.enable_guc & 2) {
> +		if (!HAS_HUC_FW(dev_priv)) {
> +			DRM_INFO("Ignoring option enable_guc=%d - %s\n",
> +				i915_modparams.enable_guc, "no HuC firmware");
> +			i915_modparams.enable_guc = 0;
Clearing only HuC status from enable_guc would be proper I guess. Sorry 
I did not see this earlier.
> +		}
>   	}
> -
> -	/* Can't enable guc submission without guc loaded */
> -	if (!i915_modparams.enable_guc_loading)
> -		i915_modparams.enable_guc_submission = 0;
> -
> -	/* A negative value means "use platform default" */
> -	if (i915_modparams.enable_guc_submission < 0)
> -		i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
>   }
>   
>   void intel_uc_init_early(struct drm_i915_private *dev_priv)
> @@ -154,7 +161,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   	struct intel_guc *guc = &dev_priv->guc;
>   	int ret, attempts;
>   
> -	if (!i915_modparams.enable_guc_loading)
> +	if (!NEEDS_GUC_LOAD(dev_priv))
>   		return 0;
>   
>   	guc_disable_communication(guc);
> @@ -163,7 +170,7 @@ 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);
>   
> -	if (i915_modparams.enable_guc_submission) {
> +	if (i915_modparams.enable_guc) {
only NEEDS_GUC_SUBMISSION check? and similarly during fini
>   		/*
>   		 * This is stuff we need to have available at fw load time
>   		 * if we are planning to enable submission later
> @@ -213,7 +220,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   		goto err_log_capture;
>   
>   	intel_huc_auth(&dev_priv->huc);
> -	if (i915_modparams.enable_guc_submission) {
> +	if (i915_modparams.enable_guc) {
>   		if (i915_modparams.guc_log_level >= 0)
>   			gen9_enable_guc_interrupts(dev_priv);
We need to enable GuC interrupts if enable_guc > 0
but need to enable submission only if (enable_guc & 1)
>   
> @@ -223,7 +230,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   	}
>   
>   	dev_info(dev_priv->drm.dev, "GuC %s (firmware %s [version %u.%u])\n",
> -		 i915_modparams.enable_guc_submission ? "submission enabled" :
> +		 i915_modparams.enable_guc ? "submission enabled" :
>   							"loaded",
This condition also needs proper check and messages.
Move the load message to intel_uc_init_hw and only keep submission here.
>   		 guc->fw.path,
>   		 guc->fw.major_ver_found, guc->fw.minor_ver_found);
> @@ -245,27 +252,21 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>   err_log_capture:
>   	guc_capture_load_err_log(guc);
>   err_submission:
> -	if (i915_modparams.enable_guc_submission)
> +	if (i915_modparams.enable_guc)
>   		intel_guc_submission_fini(guc);
>   err_guc:
>   	i915_ggtt_disable_guc(dev_priv);
>   
> -	if (i915_modparams.enable_guc_loading > 1 ||
> -	    i915_modparams.enable_guc_submission > 1) {
> +	if (i915_modparams.enable_guc > 1) {
>   		DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
>   		ret = -EIO;
> +	} else if (i915_modparams.enable_guc == 1) {
> +		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
Not falling back to execlists mode if enable_guc=3? why?
We can't have -EIO now as the option of "GuC is required" is not present 
anymore.
> +		ret = 0;
>   	} else {
> -		DRM_NOTE("GuC init failed. Firmware loading disabled.\n");
>   		ret = 0;
>   	}
>   
> -	if (i915_modparams.enable_guc_submission) {
> -		i915_modparams.enable_guc_submission = 0;
> -		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> -	}
> -
> -	i915_modparams.enable_guc_loading = 0;
> -
>   	return ret;
>   }
>   
> @@ -275,15 +276,15 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>   
>   	guc_free_load_err_log(guc);
>   
> -	if (!i915_modparams.enable_guc_loading)
> +	if (!NEEDS_GUC_LOAD(dev_priv))
>   		return;
>   
> -	if (i915_modparams.enable_guc_submission)
> +	if (i915_modparams.enable_guc)
Add GuC submission check.
>   		intel_guc_submission_disable(guc);
>   
>   	guc_disable_communication(guc);
>   
> -	if (i915_modparams.enable_guc_submission) {
> +	if (i915_modparams.enable_guc) {
>   		gen9_disable_guc_interrupts(dev_priv);
>   		intel_guc_submission_fini(guc);
>   	}

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

Re: [PATCH v10 1/2] drm/i915/guc : Removing enable_guc_loading and enable_guc_submission module parameters

[Michal Wajdeczko]

On Tue, 28 Nov 2017 11:41:57 +0100, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

>
>
> On 11/28/2017 1:24 AM, Sujaritha Sundaresan wrote:
>> We currently have two module parameters that control GuC:
>> "enable_guc_loading" and "enable_guc_submission". Whenever
>> we need submission=1, we also need loading=1.We also need
>> loading=1 when we want to want to verify the HuC, which
>> is every time we have a HuC (but all platforms with HuC
>> have a GuC and viceversa).
>>

<SNIP>

>> +
>> +	/* Making sure that our auto is well defined */
>> +	GEM_BUG_ON(auto_enable_guc < 0);
>> +	GEM_BUG_ON((auto_enable_guc > 0) && !HAS_GUC_FW(dev_priv));
>> +	GEM_BUG_ON((auto_enable_guc & 2) && !HAS_HUC_FW(dev_priv));
>> +
>> +	if (i915_modparams.enable_guc < 0)
>> +		i915_modparams.enable_guc = auto_enable_guc;
>> +
>> +	if (i915_modparams.enable_guc > 0) {
>> +		if (!HAS_GUC(dev_priv) || !HAS_GUC_FW(dev_priv)) {
>> +			DRM_INFO("Ignoring option enable_guc=%d - %s\n",
>> +					 i915_modparams.enable_guc,
>> +				     !HAS_GUC(dev_priv) ? "no GuC hardware" :
>> +					 "no GuC firmware");
>> +			i915_modparams.enable_guc = 0;
>> +		}
>>   	}
>>   -	/* A negative value means "use platform default" */
>> -	if (i915_modparams.enable_guc_loading < 0)
>> -		i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
>> -
>> -	/* Verify firmware version */
>> -	if (i915_modparams.enable_guc_loading) {
>> -		if (HAS_HUC_UCODE(dev_priv))
>> -			intel_huc_select_fw(&dev_priv->huc);
>> -
>> -		if (intel_guc_fw_select(&dev_priv->guc))
>> -			i915_modparams.enable_guc_loading = 0;
>> +	if (i915_modparams.enable_guc & 2) {
>> +		if (!HAS_HUC_FW(dev_priv)) {
>> +			DRM_INFO("Ignoring option enable_guc=%d - %s\n",
>> +				i915_modparams.enable_guc, "no HuC firmware");
>> +			i915_modparams.enable_guc = 0;
> Clearing only HuC status from enable_guc would be proper I guess. Sorry  
> I did not see this earlier.

My understanding is that if user had specified non-zero positive value of
'enable_guc' then it means that he requests *all* GuC options to be  
available
(something like our old '2=required' mode). If any of required option is  
not
available then we should not try to cherry-pick/drop single option.

Note that if user don't care about specific option, then user should use  
-1(auto)
mode and rely on driver decision what is available and in which  
configuration.
And for 'auto' mode we try to make sure to do not select broken  
configurations.

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

Re: [PATCH v10 1/2] drm/i915/guc : Removing enable_guc_loading and enable_guc_submission module parameters

[Sagar Arun Kamble]

On 11/29/2017 5:44 PM, Michal Wajdeczko wrote:
> On Tue, 28 Nov 2017 11:41:57 +0100, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>>
>>
>> On 11/28/2017 1:24 AM, Sujaritha Sundaresan wrote:
>>> We currently have two module parameters that control GuC:
>>> "enable_guc_loading" and "enable_guc_submission". Whenever
>>> we need submission=1, we also need loading=1.We also need
>>> loading=1 when we want to want to verify the HuC, which
>>> is every time we have a HuC (but all platforms with HuC
>>> have a GuC and viceversa).
>>>
>
> <SNIP>
>
>>> +
>>> +    /* Making sure that our auto is well defined */
>>> +    GEM_BUG_ON(auto_enable_guc < 0);
>>> +    GEM_BUG_ON((auto_enable_guc > 0) && !HAS_GUC_FW(dev_priv));
>>> +    GEM_BUG_ON((auto_enable_guc & 2) && !HAS_HUC_FW(dev_priv));
>>> +
>>> +    if (i915_modparams.enable_guc < 0)
>>> +        i915_modparams.enable_guc = auto_enable_guc;
>>> +
>>> +    if (i915_modparams.enable_guc > 0) {
>>> +        if (!HAS_GUC(dev_priv) || !HAS_GUC_FW(dev_priv)) {
>>> +            DRM_INFO("Ignoring option enable_guc=%d - %s\n",
>>> +                     i915_modparams.enable_guc,
>>> +                     !HAS_GUC(dev_priv) ? "no GuC hardware" :
>>> +                     "no GuC firmware");
>>> +            i915_modparams.enable_guc = 0;
>>> +        }
>>>       }
>>>   -    /* A negative value means "use platform default" */
>>> -    if (i915_modparams.enable_guc_loading < 0)
>>> -        i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
>>> -
>>> -    /* Verify firmware version */
>>> -    if (i915_modparams.enable_guc_loading) {
>>> -        if (HAS_HUC_UCODE(dev_priv))
>>> -            intel_huc_select_fw(&dev_priv->huc);
>>> -
>>> -        if (intel_guc_fw_select(&dev_priv->guc))
>>> -            i915_modparams.enable_guc_loading = 0;
>>> +    if (i915_modparams.enable_guc & 2) {
>>> +        if (!HAS_HUC_FW(dev_priv)) {
>>> +            DRM_INFO("Ignoring option enable_guc=%d - %s\n",
>>> +                i915_modparams.enable_guc, "no HuC firmware");
>>> +            i915_modparams.enable_guc = 0;
>> Clearing only HuC status from enable_guc would be proper I guess. 
>> Sorry I did not see this earlier.
>
> My understanding is that if user had specified non-zero positive value of
> 'enable_guc' then it means that he requests *all* GuC options to be 
> available
> (something like our old '2=required' mode). If any of required option 
> is not
> available then we should not try to cherry-pick/drop single option.
I think we wanted to enable HuC for some platforms but not enable GuC 
submission. Anusha can you
please confirm if such cherry-picking is needed through module parameter.
>
> Note that if user don't care about specific option, then user should 
> use -1(auto)
> mode and rely on driver decision what is available and in which 
> configuration.
> And for 'auto' mode we try to make sure to do not select broken 
> configurations.
In that case we can just have 3 values for enable_guc (if cherry-picking 
is not to be done)
-1: auto (whatever is available)
0: all disabled
1: all enabled if available else all disabled
>
> Michal

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

Re: [PATCH v10 1/2] drm/i915/guc : Removing enable_guc_loading and enable_guc_submission module parameters

[Michal Wajdeczko]

On Wed, 29 Nov 2017 14:40:05 +0100, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

>
>
> On 11/29/2017 5:44 PM, Michal Wajdeczko wrote:
>> On Tue, 28 Nov 2017 11:41:57 +0100, Sagar Arun Kamble  
>> <sagar.a.kamble@intel.com> wrote:
>>
>>>
>>>
>>> On 11/28/2017 1:24 AM, Sujaritha Sundaresan wrote:
>>>> We currently have two module parameters that control GuC:
>>>> "enable_guc_loading" and "enable_guc_submission". Whenever
>>>> we need submission=1, we also need loading=1.We also need
>>>> loading=1 when we want to want to verify the HuC, which
>>>> is every time we have a HuC (but all platforms with HuC
>>>> have a GuC and viceversa).
>>>>
>>
>> <SNIP>
>>
>>>> +
>>>> +    /* Making sure that our auto is well defined */
>>>> +    GEM_BUG_ON(auto_enable_guc < 0);
>>>> +    GEM_BUG_ON((auto_enable_guc > 0) && !HAS_GUC_FW(dev_priv));
>>>> +    GEM_BUG_ON((auto_enable_guc & 2) && !HAS_HUC_FW(dev_priv));
>>>> +
>>>> +    if (i915_modparams.enable_guc < 0)
>>>> +        i915_modparams.enable_guc = auto_enable_guc;
>>>> +
>>>> +    if (i915_modparams.enable_guc > 0) {
>>>> +        if (!HAS_GUC(dev_priv) || !HAS_GUC_FW(dev_priv)) {
>>>> +            DRM_INFO("Ignoring option enable_guc=%d - %s\n",
>>>> +                     i915_modparams.enable_guc,
>>>> +                     !HAS_GUC(dev_priv) ? "no GuC hardware" :
>>>> +                     "no GuC firmware");
>>>> +            i915_modparams.enable_guc = 0;
>>>> +        }
>>>>       }
>>>>   -    /* A negative value means "use platform default" */
>>>> -    if (i915_modparams.enable_guc_loading < 0)
>>>> -        i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
>>>> -
>>>> -    /* Verify firmware version */
>>>> -    if (i915_modparams.enable_guc_loading) {
>>>> -        if (HAS_HUC_UCODE(dev_priv))
>>>> -            intel_huc_select_fw(&dev_priv->huc);
>>>> -
>>>> -        if (intel_guc_fw_select(&dev_priv->guc))
>>>> -            i915_modparams.enable_guc_loading = 0;
>>>> +    if (i915_modparams.enable_guc & 2) {
>>>> +        if (!HAS_HUC_FW(dev_priv)) {
>>>> +            DRM_INFO("Ignoring option enable_guc=%d - %s\n",
>>>> +                i915_modparams.enable_guc, "no HuC firmware");
>>>> +            i915_modparams.enable_guc = 0;
>>> Clearing only HuC status from enable_guc would be proper I guess.  
>>> Sorry I did not see this earlier.
>>
>> My understanding is that if user had specified non-zero positive value  
>> of
>> 'enable_guc' then it means that he requests *all* GuC options to be  
>> available
>> (something like our old '2=required' mode). If any of required option  
>> is not
>> available then we should not try to cherry-pick/drop single option.
> I think we wanted to enable HuC for some platforms but not enable GuC  
> submission. Anusha can you
> please confirm if such cherry-picking is needed through module parameter.

Cherry-picking through module parameter is fine.
And at the same time we should treat them as mandatory options.

I was referring to cherry-picking (or more precisely droping) during call
to sanitize_options(). So when user specify guc_enable as 3(submission+huc)
we should not convert it into 2(submission only).

>>
>> Note that if user don't care about specific option, then user should  
>> use -1(auto)
>> mode and rely on driver decision what is available and in which  
>> configuration.
>> And for 'auto' mode we try to make sure to do not select broken  
>> configurations.
> In that case we can just have 3 values for enable_guc (if cherry-picking  
> is not to be done)
> -1: auto (whatever is available)
> 0: all disabled
> 1: all enabled if available else all disabled
>>
>> Michal
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v10 2/2] drm/i915/guc : Updating GuC and HuC firmware select function

[Sujaritha Sundaresan]

Updating GuC and HuC firmware select function to support removing
i915_modparams.enable_guc_loading module parameter.

v2: Clarifying the commit message (Anusha)

v3: Unify seq_puts messages, Re-factoring code as per review (Michal)

v4: Rebase

v5: Separating message unification into a separate patch

v6: Re-factoring code (Sagar, Michal)
    Rebase

v7: Separating from previuos patch (Sagar)
    Rebase

v8: Including change to intel_uc.c
    Applying review comments (Michal)

v9: Including HAS_HUC macro

v10: Applying review comments (Michal)

Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_fw.c | 15 +++++----------
 drivers/gpu/drm/i915/intel_guc_fw.h |  2 +-
 drivers/gpu/drm/i915/intel_huc.c    |  8 ++++----
 drivers/gpu/drm/i915/intel_uc.c     |  4 ++++
 4 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
index 69ba015..52d126d 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/intel_guc_fw.c
@@ -58,18 +58,18 @@
 
 /**
  * intel_guc_fw_select() - selects GuC firmware for uploading
- *
  * @guc:	intel_guc struct
- *
- * Return: zero when we know firmware, non-zero in other case
  */
-int intel_guc_fw_select(struct intel_guc *guc)
+void intel_guc_fw_select(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
 	intel_uc_fw_init(&guc->fw, INTEL_UC_FW_TYPE_GUC);
 
-	if (i915_modparams.guc_firmware_path) {
+	if (HAS_GUC(dev_priv)) {
+		DRM_ERROR("No GuC FW known for platform with GuC!\n");
+	return;
+	} else if (i915_modparams.guc_firmware_path) {
 		guc->fw.path = i915_modparams.guc_firmware_path;
 		guc->fw.major_ver_wanted = 0;
 		guc->fw.minor_ver_wanted = 0;
@@ -89,12 +89,7 @@ int intel_guc_fw_select(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 {
-		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
-		return -ENOENT;
 	}
-
-	return 0;
 }
 
 static void guc_prepare_xfer(struct intel_guc *guc)
diff --git a/drivers/gpu/drm/i915/intel_guc_fw.h b/drivers/gpu/drm/i915/intel_guc_fw.h
index 023f5ba..7f6ccaf 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.h
+++ b/drivers/gpu/drm/i915/intel_guc_fw.h
@@ -27,7 +27,7 @@
 
 struct intel_guc;
 
-int intel_guc_fw_select(struct intel_guc *guc);
+void intel_guc_fw_select(struct intel_guc *guc);
 int intel_guc_fw_upload(struct intel_guc *guc);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 98d1725..911405d 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -87,7 +87,10 @@ void intel_huc_select_fw(struct intel_huc *huc)
 
 	intel_uc_fw_init(&huc->fw, INTEL_UC_FW_TYPE_HUC);
 
-	if (i915_modparams.huc_firmware_path) {
+	if (HAS_HUC(dev_priv)) {
+		DRM_ERROR("No HuC FW known for platform with HuC!\n");
+	return;
+	} else if (i915_modparams.huc_firmware_path) {
 		huc->fw.path = i915_modparams.huc_firmware_path;
 		huc->fw.major_ver_wanted = 0;
 		huc->fw.minor_ver_wanted = 0;
@@ -107,9 +110,6 @@ 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 {
-		DRM_ERROR("No HuC firmware known for platform with HuC!\n");
-		return;
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 233f680..2fdd7a8 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -91,10 +91,14 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
 {
 	intel_guc_init_early(&dev_priv->guc);
+	intel_guc_fw_select(&dev_priv->guc);
+	intel_huc_select_fw(&dev_priv->huc);
 }
 
 void intel_uc_init_fw(struct drm_i915_private *dev_priv)
 {
+	if (!HAS_GUC(dev_priv))
+		return;
 	intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
 	intel_uc_fw_fetch(dev_priv, &dev_priv->guc.fw);
 }
-- 
1.9.1

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

✗ Fi.CI.BAT: failure for drm/i915/guc : Removing GuC loading and submission modparams

[Patchwork]

== Series Details ==

Series: drm/i915/guc : Removing GuC loading and submission modparams
URL   : https://patchwork.freedesktop.org/series/34490/
State : failure

== Summary ==

Applying: drm/i915/guc : Removing enable_guc_loading and enable_guc_submission module parameters
Patch failed at 0001 drm/i915/guc : Removing enable_guc_loading and enable_guc_submission module parameters

Current HEAD:
commit b60122cc96d8f4f46f13c4e4c6b8226b6f572b9a
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Mon Nov 27 16:41:01 2017 +0000

    drm-tip: 2017y-11m-27d-16h-40m-07s UTC integration manifest

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