[PATCH v7 0/4] drm/i915/guc : Removing enable_guc_loading module and Decoupling logs and ADS from submission

[PATCH v7 0/4] drm/i915/guc : Removing enable_guc_loading module and Decoupling logs and ADS from submission

[Sujaritha Sundaresan]

The first patch simply unifies different seq_puts messages found in debugfs.
Patch 2 and 3 involve replacing te enable_guc_loading module. Patch 4 deals
with decoupling GuC logs and ADS from submission.

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

Sujaritha Sundaresan (4):
  drm/i915 : Unifying seq_puts messages for feature support
  drm/i915/guc : Removing i915_modparams.enable_guc_loading module parameter
  drm/i915/guc : Updating GuC and HuC FW select function
  drm/i915/guc : Decouple logs and ADS from submission

 drivers/gpu/drm/i915/Makefile              |   1 +
 drivers/gpu/drm/i915/i915_debugfs.c        |  22 ++++--
 drivers/gpu/drm/i915/i915_drv.h            |   9 ++-
 drivers/gpu/drm/i915/i915_gem_context.c    |   2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c        |   2 +-
 drivers/gpu/drm/i915/i915_guc_submission.c | 106 +------------------------
 drivers/gpu/drm/i915/i915_irq.c            |   2 +-
 drivers/gpu/drm/i915/i915_params.c         |   4 -
 drivers/gpu/drm/i915/i915_params.h         |   1 -
 drivers/gpu/drm/i915/intel_guc.h           |   1 +
 drivers/gpu/drm/i915/intel_guc_ads.c       | 119 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc_ads.h       |  31 ++++++++
 drivers/gpu/drm/i915/intel_guc_fw.c        |   9 +--
 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           |   4 +-
 drivers/gpu/drm/i915/intel_uc.c            | 108 ++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_uncore.c        |   3 +-
 18 files changed, 264 insertions(+), 168 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.c
 create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.h

-- 
1.9.1

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

[PATCH v7 1/4] drm/i915 : Unifying seq_puts messages for feature support

[Sujaritha Sundaresan]

Unifying the various seq_puts messages in debugfs to the simplest one for
feature support.

v2: Clarifying the commit message (Anusha)

v3: Re-factoring code as per review (Michal)

v4: Rebase

v5: Split from following patch

v6: Re-factoring code (Michal, Sagar)
    Clarifying commit message (Sagar)

v7: Generalizing subject to drm/i915 (Sagar)

Suggested by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@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 | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 40287e9..ac25d63 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1641,7 +1641,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;
 	}
 
@@ -1809,7 +1809,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;
 	}
 
@@ -2361,8 +2361,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_HUC_UCODE(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);
@@ -2394,8 +2397,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_UCODE(dev_priv)) {
+		seq_puts(m, "not supported\n");
 		return 0;
+	}
+
 
 	seq_printf(m, "GuC firmware status:\n");
 	seq_printf(m, "\tpath: %s\n",
@@ -2679,7 +2685,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;
 	}
 
@@ -3546,7 +3552,7 @@ static void drrs_status_per_crtc(struct seq_file *m,
 
 		mutex_lock(&drrs->mutex);
 		/* DRRS Supported */
-		seq_puts(m, "\tDRRS Supported: Yes\n");
+		seq_puts(m, "supported\n");
 
 		/* disable_drrs() will make drrs->dp NULL */
 		if (!drrs->dp) {
@@ -3578,7 +3584,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");
 }
-- 
1.9.1

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

[PATCH v7 2/4] drm/i915/guc : Removing i915_modparams.enable_guc_loading module parameter

[Sujaritha Sundaresan]

We currently have two module parameters that control GuC:
"enable_guc_loading" and "enable_guc_submission". Whenever
we need i915_modparams.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, 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

Suggested by: Oscar Mateo <oscar.mateo@intel.com>
Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c     |  6 +--
 drivers/gpu/drm/i915/i915_drv.h         |  9 +++--
 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      |  4 --
 drivers/gpu/drm/i915/i915_params.h      |  1 -
 drivers/gpu/drm/i915/intel_uc.c         | 69 ++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_uncore.c     |  3 +-
 9 files changed, 50 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index ac25d63..bc31769 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2361,7 +2361,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 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;
 	}
@@ -2397,7 +2397,7 @@ 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;
 	}
@@ -2496,7 +2496,7 @@ static bool check_guc_submission(struct seq_file *m)
 
 	if (!guc->execbuf_client) {
 		seq_printf(m, "GuC submission %s\n",
-			   HAS_GUC_SCHED(dev_priv) ?
+			   HAS_GUC(dev_priv) ?
 			   "disabled" :
 			   "not supported");
 		return false;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index dd141b2..5b9bdd0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3201,9 +3201,12 @@ static inline unsigned int i915_sg_segment_size(void)
  */
 #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_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_modparams.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 5bf96a2..692d609 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_modparams.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 527a2d2..0bbc8f0 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3481,7 +3481,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_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 b1296a5..ec76aac 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4026,7 +4026,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..1c25f45 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -162,10 +162,6 @@ 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)");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index c729226..9e1e231 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -44,7 +44,6 @@
 	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, guc_log_level, -1) \
 	param(char *, guc_firmware_path, NULL) \
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 25bd162..df281525 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -49,36 +49,44 @@ static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv)
 
 void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 {
+	/* Verify Hardware support */
 	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;
+		if (i915_modparams.enable_guc_submission > 0)
+			DRM_INFO("Ignoring GuC submission enable",
+					"no hardware\n");
+			i915_modparams.enable_guc_submission = 0;
 		return;
 	}
 
-	/* 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 support */
+	if (!HAS_GUC_UCODE(dev_priv)) {
+		if (i915_modparams.enable_guc_submission == 1) {
+			DRM_INFO("Ignoring GuC submission enable",
+					"no firmware\n");
+			i915_modparams.enable_guc_submission = 0;
+		return;
+		}
 
-	/* Verify firmware version */
-	if (i915_modparams.enable_guc_loading) {
-		if (HAS_HUC_UCODE(dev_priv))
-			intel_huc_select_fw(&dev_priv->huc);
+		if (i915_modparams.enable_guc_submission < 0) {
+			i915_modparams.enable_guc_submission = 0;
+			return;
+		}
 
-		if (intel_guc_fw_select(&dev_priv->guc))
-			i915_modparams.enable_guc_loading = 0;
+		/*
+		 * If "required" (> 1), let it continue and we will fail later
+		 * due to the lack of firmware
+		 */
+
 	}
 
-	/* 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 paltform default" (enabled if we have
+	 * survived to get here)
+	 */
 
-	/* A negative value means "use platform default" */
 	if (i915_modparams.enable_guc_submission < 0)
-		i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
+		i915_modparams.enable_guc_submission = HAS_GUC(dev_priv);
+
 }
 
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
@@ -88,6 +96,8 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
 
 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);
 }
@@ -154,7 +164,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_LOADING(dev_priv))
 		return 0;
 
 	guc_disable_communication(guc);
@@ -250,22 +260,17 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 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_submission > 1) {
 		DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
 		ret = -EIO;
+	} else if (i915_modparams.enable_guc_submission == 1) {
+		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
+		i915_modparams.enable_guc_submission = 0;
+		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;
 }
 
@@ -273,7 +278,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 {
 	guc_free_load_err_log(&dev_priv->guc);
 
-	if (!i915_modparams.enable_guc_loading)
+	if (!NEEDS_GUC_LOADING(dev_priv))
 		return;
 
 	if (i915_modparams.enable_guc_submission)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 20e3c65c..c631b0e 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1803,8 +1803,7 @@ int intel_guc_reset(struct drm_i915_private *dev_priv)
 {
 	int ret;
 
-	if (!HAS_GUC(dev_priv))
-		return -EINVAL;
+	GEM_BUG_ON(!HAS_GUC(dev_priv));
 
 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
 	ret = gen6_hw_domain_reset(dev_priv, GEN9_GRDOM_GUC);
-- 
1.9.1

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

[PATCH v7 3/4] drm/i915/guc : Updating GuC and HuC FW 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

Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_fw.c | 9 ++++-----
 drivers/gpu/drm/i915/intel_guc_fw.h | 2 +-
 drivers/gpu/drm/i915/intel_huc.c    | 4 +++-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
index ef67a36..5bffeef 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/intel_guc_fw.c
@@ -63,7 +63,7 @@
  *
  * 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);
 
@@ -90,11 +90,10 @@ int intel_guc_fw_select(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 platform with GuC!\n");
+		return;
 	}
-
-	return 0;
 }
 
 /*
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 c8a48cb..fc61779 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -108,7 +108,9 @@ void intel_huc_select_fw(struct intel_huc *huc)
 		huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR;
 		huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR;
 	} else {
-		DRM_ERROR("No HuC firmware known for platform with HuC!\n");
+		/* For now, everything with a GuC also has a HuC */
+		if (HAS_GUC(dev_priv))
+			DRM_ERROR("No HuC FW known for platform with HuC!\n");
 		return;
 	}
 }
-- 
1.9.1

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

[PATCH v7 4/4] drm/i915/guc : Decouple logs and ADS from submission

[Sujaritha Sundaresan]

The Additional Data Struct (ADS) contains objects that are required by
guc post FW load and are not necessarily submission-only (although that's
our current only use-case). If in the future we load GuC with submission
disabled to use some other GuC feature we might still end up requiring
something inside the ADS, so it makes more sense for them to be always
created if GuC is loaded.

Similarly, we still want to access GuC logs even if GuC submission is
disable to debug issues with GuC loading or with wathever we're using
GuC for.

To make a concrete example, the pages used by GuC to save state during
suspend are allocated as part of the ADS.

v3: Group initialization of GuC objects

v2: Decoupling ADS together with logs

v3: Re-factoring code as per review (Michal)

v4: Rebase

v5: Separating group object initialization into next patch
    Clarifying commit message

v6: Reverting to goto err format (Michal)
    Moved guc_ads functions to dedicated file
    Rebase

v7: Rebase

Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/Makefile              |   1 +
 drivers/gpu/drm/i915/i915_guc_submission.c | 106 +------------------------
 drivers/gpu/drm/i915/intel_guc.h           |   1 +
 drivers/gpu/drm/i915/intel_guc_ads.c       | 119 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc_ads.h       |  31 ++++++++
 drivers/gpu/drm/i915/intel_guc_log.c       |   6 +-
 drivers/gpu/drm/i915/intel_uc.c            |  39 +++++++++-
 7 files changed, 195 insertions(+), 108 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.c
 create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 6c3b048..d7ce07e 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -62,6 +62,7 @@ i915-y += i915_cmd_parser.o \
 i915-y += intel_uc.o \
 	  intel_uc_fw.o \
 	  intel_guc.o \
+	  intel_guc_ads.o \
 	  intel_guc_ct.o \
 	  intel_guc_log.o \
 	  intel_guc_fw.o \
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index a2e8114..d3c0b01 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -26,6 +26,7 @@
 #include <trace/events/dma_fence.h>
 
 #include "i915_guc_submission.h"
+#include "intel_guc_ads.h"
 #include "i915_drv.h"
 
 /**
@@ -72,13 +73,6 @@
  * ELSP context descriptor dword into Work Item.
  * See guc_wq_item_append()
  *
- * ADS:
- * The Additional Data Struct (ADS) has pointers for different buffers used by
- * the GuC. One single gem object contains the ADS struct itself (guc_ads), the
- * scheduling policies (guc_policies), a structure describing a collection of
- * register sets (guc_mmio_reg_state) and some extra pages for the GuC to save
- * its internal state for sleep.
- *
  */
 
 static inline bool is_high_priority(struct i915_guc_client* client)
@@ -863,7 +857,7 @@ static void guc_policy_init(struct guc_policy *policy)
 	policy->policy_flags = 0;
 }
 
-static void guc_policies_init(struct guc_policies *policies)
+void i915_guc_policies_init(struct guc_policies *policies)
 {
 	struct guc_policy *policy;
 	u32 p, i;
@@ -883,88 +877,6 @@ static void guc_policies_init(struct guc_policies *policies)
 }
 
 /*
- * The first 80 dwords of the register state context, containing the
- * execlists and ppgtt registers.
- */
-#define LR_HW_CONTEXT_SIZE	(80 * sizeof(u32))
-
-static int guc_ads_create(struct intel_guc *guc)
-{
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	struct i915_vma *vma;
-	struct page *page;
-	/* The ads obj includes the struct itself and buffers passed to GuC */
-	struct {
-		struct guc_ads ads;
-		struct guc_policies policies;
-		struct guc_mmio_reg_state reg_state;
-		u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
-	} __packed *blob;
-	struct intel_engine_cs *engine;
-	enum intel_engine_id id;
-	const u32 skipped_offset = LRC_HEADER_PAGES * PAGE_SIZE;
-	const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE + LR_HW_CONTEXT_SIZE;
-	u32 base;
-
-	GEM_BUG_ON(guc->ads_vma);
-
-	vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
-	if (IS_ERR(vma))
-		return PTR_ERR(vma);
-
-	guc->ads_vma = vma;
-
-	page = i915_vma_first_page(vma);
-	blob = kmap(page);
-
-	/* GuC scheduling policies */
-	guc_policies_init(&blob->policies);
-
-	/* MMIO reg state */
-	for_each_engine(engine, dev_priv, id) {
-		blob->reg_state.white_list[engine->guc_id].mmio_start =
-			engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
-
-		/* Nothing to be saved or restored for now. */
-		blob->reg_state.white_list[engine->guc_id].count = 0;
-	}
-
-	/*
-	 * The GuC requires a "Golden Context" when it reinitialises
-	 * engines after a reset. Here we use the Render ring default
-	 * context, which must already exist and be pinned in the GGTT,
-	 * so its address won't change after we've told the GuC where
-	 * to find it. Note that we have to skip our header (1 page),
-	 * because our GuC shared data is there.
-	 */
-	blob->ads.golden_context_lrca =
-		guc_ggtt_offset(dev_priv->kernel_context->engine[RCS].state) + skipped_offset;
-
-	/*
-	 * The GuC expects us to exclude the portion of the context image that
-	 * it skips from the size it is to read. It starts reading from after
-	 * the execlist context (so skipping the first page [PPHWSP] and 80
-	 * dwords). Weird guc is weird.
-	 */
-	for_each_engine(engine, dev_priv, id)
-		blob->ads.eng_state_size[engine->guc_id] = engine->context_size - skipped_size;
-
-	base = guc_ggtt_offset(vma);
-	blob->ads.scheduler_policies = base + ptr_offset(blob, policies);
-	blob->ads.reg_state_buffer = base + ptr_offset(blob, reg_state_buffer);
-	blob->ads.reg_state_addr = base + ptr_offset(blob, reg_state);
-
-	kunmap(page);
-
-	return 0;
-}
-
-static void guc_ads_destroy(struct intel_guc *guc)
-{
-	i915_vma_unpin_and_release(&guc->ads_vma);
-}
-
-/*
  * Set up the memory resources to be shared with the GuC (via the GGTT)
  * at firmware loading time.
  */
@@ -994,22 +906,10 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 
 	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;
-
 	ida_init(&guc->stage_ids);
 
 	return 0;
 
-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;
@@ -1020,8 +920,6 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 	struct intel_guc *guc = &dev_priv->guc;
 
 	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.h b/drivers/gpu/drm/i915/intel_guc.h
index 418450b..ab2496b 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -115,6 +115,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 int intel_guc_suspend(struct drm_i915_private *dev_priv);
 int intel_guc_resume(struct drm_i915_private *dev_priv);
 struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
+void i915_guc_policies_init(struct guc_policies *policies);
 u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_guc_ads.c b/drivers/gpu/drm/i915/intel_guc_ads.c
new file mode 100644
index 0000000..b6d6442
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_guc_ads.c
@@ -0,0 +1,119 @@
+/*
+ * Copyright © 2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include "intel_uc.h"
+#include "i915_drv.h"
+#include "intel_guc.h"
+#include "i915_guc_submission.h"
+
+/* ADS:
+ * The Additional Data Struct (ADS) has pointers for different buffers used by
+ * the GuC. One single gem object contains the ADS struct itself (guc_ads), the
+ * scheduling policies (guc_policies), a structure describing a collection of
+ * register sets (guc_mmio_reg_state) and some extra pages for the GuC to save
+ * its internal state for sleep.
+ *
+ */
+
+/*
+ * The first 80 dwords of the register state context, containing the
+ * execlists and ppgtt registers.
+ */
+#define LR_HW_CONTEXT_SIZE	(80 * sizeof(u32))
+
+int guc_ads_create(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	struct i915_vma *vma;
+	struct page *page;
+	/* The ads obj includes the struct itself and buffers passed to GuC */
+	struct {
+		struct guc_ads ads;
+		struct guc_policies policies;
+		struct guc_mmio_reg_state reg_state;
+		u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
+	} __packed *blob;
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	const u32 skipped_offset = LRC_HEADER_PAGES * PAGE_SIZE;
+	const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE + LR_HW_CONTEXT_SIZE;
+	u32 base;
+
+	GEM_BUG_ON(guc->ads_vma);
+
+	vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
+	if (IS_ERR(vma))
+		return PTR_ERR(vma);
+
+	guc->ads_vma = vma;
+
+	page = i915_vma_first_page(vma);
+	blob = kmap(page);
+
+	/* GuC scheduling policies */
+	i915_guc_policies_init(&blob->policies);
+
+	/* MMIO reg state */
+	for_each_engine(engine, dev_priv, id) {
+		blob->reg_state.white_list[engine->guc_id].mmio_start =
+			engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
+
+		/* Nothing to be saved or restored for now. */
+		blob->reg_state.white_list[engine->guc_id].count = 0;
+	}
+
+	/*
+	 * The GuC requires a "Golden Context" when it reinitialises
+	 * engines after a reset. Here we use the Render ring default
+	 * context, which must already exist and be pinned in the GGTT,
+	 * so its address won't change after we've told the GuC where
+	 * to find it. Note that we have to skip our header (1 page),
+	 * because our GuC shared data is there.
+	 */
+	blob->ads.golden_context_lrca =
+		guc_ggtt_offset(dev_priv->kernel_context->engine[RCS].state) + skipped_offset;
+
+	/*
+	 * The GuC expects us to exclude the portion of the context image that
+	 * it skips from the size it is to read. It starts reading from after
+	 * the execlist context (so skipping the first page [PPHWSP] and 80
+	 * dwords). Weird guc is weird.
+	 */
+	for_each_engine(engine, dev_priv, id)
+		blob->ads.eng_state_size[engine->guc_id] = engine->context_size - skipped_size;
+
+	base = guc_ggtt_offset(vma);
+	blob->ads.scheduler_policies = base + ptr_offset(blob, policies);
+	blob->ads.reg_state_buffer = base + ptr_offset(blob, reg_state_buffer);
+	blob->ads.reg_state_addr = base + ptr_offset(blob, reg_state);
+
+	kunmap(page);
+
+	return 0;
+}
+
+void guc_ads_destroy(struct intel_guc *guc)
+{
+	i915_vma_unpin_and_release(&guc->ads_vma);
+}
diff --git a/drivers/gpu/drm/i915/intel_guc_ads.h b/drivers/gpu/drm/i915/intel_guc_ads.h
new file mode 100644
index 0000000..9f1ad11
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_guc_ads.h
@@ -0,0 +1,31 @@
+/*
+ * Copyright © 2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#ifndef _INTEL_GUC_ADS_H_
+#define _INTEL_GUC_ADS_H_
+
+int guc_ads_create(struct intel_guc *guc);
+void guc_ads_destroy(struct intel_guc *guc);
+
+#endif
\ No newline at end of file
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 76d3eb1..1616fdb 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 (!NEEDS_GUC_LOADING(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 (!NEEDS_GUC_LOADING(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 (!NEEDS_GUC_LOADING(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 df281525..1f24f35 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -24,6 +24,7 @@
 
 #include "intel_uc.h"
 #include "i915_drv.h"
+#include "intel_guc_ads.h"
 #include "i915_guc_submission.h"
 
 /* Reset GuC providing us with fresh state for both GuC and HuC.
@@ -159,6 +160,35 @@ 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;
+
+	if (guc->ads_vma)
+		return 0;
+
+	ret = intel_guc_log_create(guc);
+	if (ret < 0)
+		goto err_vaddr;
+
+	ret = guc_ads_create(guc);
+	if (ret < 0)
+		goto err_log;
+
+err_vaddr:
+	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
+err_log:
+	intel_guc_log_destroy(guc);
+
+	return ret;
+}
+
+static void guc_shared_objects_fini(struct intel_guc *guc)
+{
+	guc_ads_destroy(guc);
+	intel_guc_log_destroy(guc);
+}
+
 int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
@@ -173,6 +203,10 @@ 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_modparams.enable_guc_submission) {
 		/*
 		 * This is stuff we need to have available at fw load time
@@ -180,7 +214,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 		 */
 		ret = i915_guc_submission_init(dev_priv);
 		if (ret)
-			goto err_guc;
+			goto err_shared;
 	}
 
 	/* init WOPCM */
@@ -257,6 +291,8 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 err_submission:
 	if (i915_modparams.enable_guc_submission)
 		i915_guc_submission_fini(dev_priv);
+err_shared:
+	guc_shared_objects_fini(guc);
 err_guc:
 	i915_ggtt_disable_guc(dev_priv);
 
@@ -291,5 +327,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);
 }
-- 
1.9.1

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

Re: [PATCH v7 2/4] drm/i915/guc : Removing i915_modparams.enable_guc_loading module parameter

[Chris Wilson]

Quoting Sujaritha Sundaresan (2017-10-17 23:50:47)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index dd141b2..5b9bdd0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3201,9 +3201,12 @@ static inline unsigned int i915_sg_segment_size(void)
>   */
>  #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_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_modparams.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))

NEEDS_GUC_FW ?
-Chris
_______________________________________________
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 enable_guc_loading module and Decoupling logs and ADS from submission (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/guc : Removing enable_guc_loading module and Decoupling logs and ADS from submission (rev2)
URL   : https://patchwork.freedesktop.org/series/31677/
State : failure

== Summary ==

  CHK     include/config/kernel.release
  CHK     include/generated/uapi/linux/version.h
  CHK     include/generated/utsrelease.h
  CHK     include/generated/bounds.h
  CHK     include/generated/timeconst.h
  CHK     include/generated/asm-offsets.h
  CALL    scripts/checksyscalls.sh
  CHK     scripts/mod/devicetable-offsets.h
  CHK     include/generated/compile.h
  CHK     kernel/config_data.h
  CC [M]  drivers/gpu/drm/i915/intel_uc.o
In file included from ./include/linux/printk.h:6:0,
                 from ./include/linux/kernel.h:13,
                 from ./include/linux/list.h:8,
                 from ./include/linux/preempt.h:10,
                 from ./include/linux/spinlock.h:50,
                 from drivers/gpu/drm/i915/intel_uncore.h:28,
                 from drivers/gpu/drm/i915/intel_guc.h:28,
                 from drivers/gpu/drm/i915/intel_uc.h:27,
                 from drivers/gpu/drm/i915/intel_uc.c:25:
drivers/gpu/drm/i915/intel_uc.c: In function ‘intel_uc_sanitize_options’:
./include/linux/kern_levels.h:4:18: error: too many arguments for format [-Werror=format-extra-args]
 #define KERN_SOH "\001"  /* ASCII Start Of Header */
                  ^
./include/linux/kern_levels.h:13:19: note: in expansion of macro ‘KERN_SOH’
 #define KERN_INFO KERN_SOH "6" /* informational */
                   ^~~~~~~~
./include/drm/drmP.h:150:16: note: in expansion of macro ‘KERN_INFO’
   printk##once(KERN_##level "[" DRM_NAME "] " fmt, \
                ^~~~~
./include/drm/drmP.h:155:2: note: in expansion of macro ‘_DRM_PRINTK’
  _DRM_PRINTK(, INFO, fmt, ##__VA_ARGS__)
  ^~~~~~~~~~~
drivers/gpu/drm/i915/intel_uc.c:56:4: note: in expansion of macro ‘DRM_INFO’
    DRM_INFO("Ignoring GuC submission enable",
    ^~~~~~~~
./include/linux/kern_levels.h:4:18: error: too many arguments for format [-Werror=format-extra-args]
 #define KERN_SOH "\001"  /* ASCII Start Of Header */
                  ^
./include/linux/kern_levels.h:13:19: note: in expansion of macro ‘KERN_SOH’
 #define KERN_INFO KERN_SOH "6" /* informational */
                   ^~~~~~~~
./include/drm/drmP.h:150:16: note: in expansion of macro ‘KERN_INFO’
   printk##once(KERN_##level "[" DRM_NAME "] " fmt, \
                ^~~~~
./include/drm/drmP.h:155:2: note: in expansion of macro ‘_DRM_PRINTK’
  _DRM_PRINTK(, INFO, fmt, ##__VA_ARGS__)
  ^~~~~~~~~~~
drivers/gpu/drm/i915/intel_uc.c:65:4: note: in expansion of macro ‘DRM_INFO’
    DRM_INFO("Ignoring GuC submission enable",
    ^~~~~~~~
cc1: all warnings being treated as errors
scripts/Makefile.build:313: recipe for target 'drivers/gpu/drm/i915/intel_uc.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_uc.o] Error 1
scripts/Makefile.build:572: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:572: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:572: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:1023: recipe for target 'drivers' failed
make: *** [drivers] Error 2

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

Re: [PATCH v7 1/4] drm/i915 : Unifying seq_puts messages for feature support

[Michal Wajdeczko]

On Wed, 18 Oct 2017 00:50:46 +0200, Sujaritha Sundaresan  
<sujaritha.sundaresan@intel.com> wrote:

> Unifying the various seq_puts messages in debugfs to the simplest one for
> feature support.
>
> v2: Clarifying the commit message (Anusha)
>
> v3: Re-factoring code as per review (Michal)
>
> v4: Rebase
>
> v5: Split from following patch
>
> v6: Re-factoring code (Michal, Sagar)
>     Clarifying commit message (Sagar)
>
> v7: Generalizing subject to drm/i915 (Sagar)
>
> Suggested by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@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 | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c  
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 40287e9..ac25d63 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1641,7 +1641,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;
>  	}
> @@ -1809,7 +1809,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;
>  	}
> @@ -2361,8 +2361,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_HUC_UCODE(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);
> @@ -2394,8 +2397,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_UCODE(dev_priv)) {

Maybe now is a good time to change condition into HAS_GUC ?
Same for the earlier HAS_HUC

> +		seq_puts(m, "not supported\n");
>  		return 0;
> +	}
> +
> 	seq_printf(m, "GuC firmware status:\n");
>  	seq_printf(m, "\tpath: %s\n",
> @@ -2679,7 +2685,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;
>  	}
> @@ -3546,7 +3552,7 @@ static void drrs_status_per_crtc(struct seq_file  
> *m,
> 		mutex_lock(&drrs->mutex);
>  		/* DRRS Supported */
> -		seq_puts(m, "\tDRRS Supported: Yes\n");
> +		seq_puts(m, "supported\n");
> 		/* disable_drrs() will make drrs->dp NULL */
>  		if (!drrs->dp) {
> @@ -3578,7 +3584,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");
>  }

Hmm, the goal of this unification was to provide consistent output
 from those entries that have early return:

	if (!HAS_XXX(dev_priv)) {
		seq_puts(m, "not supported\n");
		return 0;
	}

but this drrs_status_per_crts function is different, so I'm not
sure that unified approach can be applied here

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

Re: [PATCH v7 3/4] drm/i915/guc : Updating GuC and HuC FW select function

[Sagar Arun Kamble]

On 10/18/2017 4:20 AM, Sujaritha Sundaresan wrote:
> 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
>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc_fw.c | 9 ++++-----
>   drivers/gpu/drm/i915/intel_guc_fw.h | 2 +-
>   drivers/gpu/drm/i915/intel_huc.c    | 4 +++-
>   3 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
> index ef67a36..5bffeef 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_guc_fw.c
> @@ -63,7 +63,7 @@
>    *
>    * Return: zero when we know firmware, non-zero in other case
Update this documentation about return.
You have dropped call to fw_select in this series. Can you please check.
>    */
> -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);
>   
> @@ -90,11 +90,10 @@ int intel_guc_fw_select(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))
This should be HAS_GUC
> +			DRM_ERROR("No GuC FW known for platform with GuC!\n");
> +		return;
>   	}
> -
> -	return 0;
>   }
>   
>   /*
> 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 c8a48cb..fc61779 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -108,7 +108,9 @@ void intel_huc_select_fw(struct intel_huc *huc)
>   		huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR;
>   		huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR;
>   	} else {
> -		DRM_ERROR("No HuC firmware known for platform with HuC!\n");
> +		/* For now, everything with a GuC also has a HuC */
> +		if (HAS_GUC(dev_priv))
> +			DRM_ERROR("No HuC FW known for platform with HuC!\n");
>   		return;
>   	}
>   }

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

Re: [PATCH v7 2/4] drm/i915/guc : Removing i915_modparams.enable_guc_loading module parameter

[Joonas Lahtinen]

On Tue, 2017-10-17 at 15:50 -0700, Sujaritha Sundaresan wrote:
> We currently have two module parameters that control GuC:
> "enable_guc_loading" and "enable_guc_submission". Whenever
> we need i915_modparams.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).

I already gave comments about clarifying the commit message, that does
not seem to have been addressed.

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 v7 2/4] drm/i915/guc : Removing i915_modparams.enable_guc_loading module parameter

[Michal Wajdeczko]

On Wed, 18 Oct 2017 00:50:47 +0200, Sujaritha Sundaresan  
<sujaritha.sundaresan@intel.com> wrote:

> We currently have two module parameters that control GuC:
> "enable_guc_loading" and "enable_guc_submission". Whenever
> we need i915_modparams.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, 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
>
> Suggested by: Oscar Mateo <oscar.mateo@intel.com>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     |  6 +--
>  drivers/gpu/drm/i915/i915_drv.h         |  9 +++--
>  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      |  4 --
>  drivers/gpu/drm/i915/i915_params.h      |  1 -
>  drivers/gpu/drm/i915/intel_uc.c         | 69  
> ++++++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_uncore.c     |  3 +-
>  9 files changed, 50 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c  
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index ac25d63..bc31769 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2361,7 +2361,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 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;
>  	}
> @@ -2397,7 +2397,7 @@ 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;
>  	}
> @@ -2496,7 +2496,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");

Btw, there is also 3rd case: "failed" when we have Guc, but something went
wrong with fw loading or enabling...

>  		return false;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h  
> b/drivers/gpu/drm/i915/i915_drv.h
> index dd141b2..5b9bdd0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3201,9 +3201,12 @@ static inline unsigned int  
> i915_sg_segment_size(void)
>   */
>  #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_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_modparams.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))

Hmm, so by the moment we add huc fw definition to the driver we will
enable guc loading, and as huc is always present with guc, this will
silently enable guc loading for all platforms with guc(huc) and there
will be no option to turn this off ...

What if we don't care about Huc functionality ?

If there will be Huc fw, both guc and huc will be loaded.
If there will be no Huc fw on the system (but it will be defined in driver)
then we will generate errors from Huc firware loading, and likely try to  
load
Guc firmware for no purpose ...

> #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 5bf96a2..692d609 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_modparams.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 527a2d2..0bbc8f0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3481,7 +3481,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_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 b1296a5..ec76aac 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4026,7 +4026,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..1c25f45 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -162,10 +162,6 @@ 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)");
> diff --git a/drivers/gpu/drm/i915/i915_params.h  
> b/drivers/gpu/drm/i915/i915_params.h
> index c729226..9e1e231 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -44,7 +44,6 @@
>  	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, guc_log_level, -1) \
>  	param(char *, guc_firmware_path, NULL) \
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index 25bd162..df281525 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -49,36 +49,44 @@ static int __intel_uc_reset_hw(struct  
> drm_i915_private *dev_priv)
> void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>  {
> +	/* Verify Hardware support */
>  	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;
> +		if (i915_modparams.enable_guc_submission > 0)
> +			DRM_INFO("Ignoring GuC submission enable",
> +					"no hardware\n");

Hmm, maybe to give clearer message to the user:

DRM_INFO("Ignoring option %s - no hardware", "enable_guc_submission");

> +			i915_modparams.enable_guc_submission = 0;
>  		return;
>  	}
> -	/* 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 support */
> +	if (!HAS_GUC_UCODE(dev_priv)) {
> +		if (i915_modparams.enable_guc_submission == 1) {

Hmm, I really don't like that we leak here enable_guc_submission = 2
knowing that it will not work ...

> +			DRM_INFO("Ignoring GuC submission enable",
> +					"no firmware\n");

DRM_INFO("Ignoring option %s - no firmware", "enable_guc_submission");

> +			i915_modparams.enable_guc_submission = 0;
> +		return;
> +		}
> -	/* Verify firmware version */
> -	if (i915_modparams.enable_guc_loading) {
> -		if (HAS_HUC_UCODE(dev_priv))
> -			intel_huc_select_fw(&dev_priv->huc);
> +		if (i915_modparams.enable_guc_submission < 0) {
> +			i915_modparams.enable_guc_submission = 0;
> +			return;
> +		}
> -		if (intel_guc_fw_select(&dev_priv->guc))
> -			i915_modparams.enable_guc_loading = 0;
> +		/*
> +		 * If "required" (> 1), let it continue and we will fail later
> +		 * due to the lack of firmware

Hmm, but doing so will break the goal of the 'sanitize' options function

> +		 */
> +
>  	}
> -	/* 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 paltform default" (enabled if we have

Typo

> +	 * survived to get here)
> +	 */
> -	/* A negative value means "use platform default" */
>  	if (i915_modparams.enable_guc_submission < 0)
> -		i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> +		i915_modparams.enable_guc_submission = HAS_GUC(dev_priv);
> +

Drop above extra line

>  }
> void intel_uc_init_early(struct drm_i915_private *dev_priv)
> @@ -88,6 +96,8 @@ void intel_uc_init_early(struct drm_i915_private  
> *dev_priv)
> void intel_uc_init_fw(struct drm_i915_private *dev_priv)
>  {
> +	if (!HAS_GUC(dev_priv))
> +		return;

Do we need this now ?

>  	intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
>  	intel_uc_fw_fetch(dev_priv, &dev_priv->guc.fw);
>  }
> @@ -154,7 +164,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_LOADING(dev_priv))
>  		return 0;
> 	guc_disable_communication(guc);
> @@ -250,22 +260,17 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  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_submission > 1) {
>  		DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
>  		ret = -EIO;
> +	} else if (i915_modparams.enable_guc_submission == 1) {
> +		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> +		i915_modparams.enable_guc_submission = 0;
> +		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;
>  }
> @@ -273,7 +278,7 @@ void intel_uc_fini_hw(struct drm_i915_private  
> *dev_priv)
>  {
>  	guc_free_load_err_log(&dev_priv->guc);
> -	if (!i915_modparams.enable_guc_loading)
> +	if (!NEEDS_GUC_LOADING(dev_priv))
>  		return;
> 	if (i915_modparams.enable_guc_submission)
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c  
> b/drivers/gpu/drm/i915/intel_uncore.c
> index 20e3c65c..c631b0e 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1803,8 +1803,7 @@ int intel_guc_reset(struct drm_i915_private  
> *dev_priv)
>  {
>  	int ret;
> -	if (!HAS_GUC(dev_priv))
> -		return -EINVAL;
> +	GEM_BUG_ON(!HAS_GUC(dev_priv));

Hmm, this looks like unrelated change - please move to separate patch

> 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>  	ret = gen6_hw_domain_reset(dev_priv, GEN9_GRDOM_GUC);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v7 2/4] drm/i915/guc : Removing i915_modparams.enable_guc_loading module parameter

[Sujaritha]

On 10/18/2017 03:58 AM, Joonas Lahtinen wrote:
> On Tue, 2017-10-17 at 15:50 -0700, Sujaritha Sundaresan wrote:
>> We currently have two module parameters that control GuC:
>> "enable_guc_loading" and "enable_guc_submission". Whenever
>> we need i915_modparams.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).
> I already gave comments about clarifying the commit message, that does
> not seem to have been addressed.
>
> Regards, Joonas


Sorry about that, I was hoping to fix the commit message after this 
revision.
I will fix the commit message in the next revised series.

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 v7 2/4] drm/i915/guc : Removing i915_modparams.enable_guc_loading module parameter

[Joonas Lahtinen]

On Wed, 2017-10-18 at 09:25 -0700, Sujaritha wrote:
> 
> On 10/18/2017 03:58 AM, Joonas Lahtinen wrote:
> > On Tue, 2017-10-17 at 15:50 -0700, Sujaritha Sundaresan wrote:
> > > We currently have two module parameters that control GuC:
> > > "enable_guc_loading" and "enable_guc_submission". Whenever
> > > we need i915_modparams.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).
> > 
> > I already gave comments about clarifying the commit message, that does
> > not seem to have been addressed.
> > 
> > Regards, Joonas
> 
> 
> Sorry about that, I was hoping to fix the commit message after this 
> revision.
> I will fix the commit message in the next revised series.

In general, it's good idea to take review comments for all of the
patches and apply all the changes at once to avoid double digit version
numbers :)

Every revision you send to the list, should be one you feel is complete
and ready to be merged. A new revision should only be needed when
something new is brought up in review (usually due to the changes made
from the last revision).

The CI system automatically picks up all patch series from the mailing
list, so sending intermediate versions will only cause unnecessary
load. If you feel the need for iterating some feature more, it's
adviseable to join the IRC channel and ping the reviewer there. That
way you don't have to wait for a day or day and a half to get comments
for the small changes.

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 v7 2/4] drm/i915/guc : Removing i915_modparams.enable_guc_loading module parameter

[Sujaritha]

On 10/18/2017 09:50 AM, Joonas Lahtinen wrote:
> On Wed, 2017-10-18 at 09:25 -0700, Sujaritha wrote:
>> On 10/18/2017 03:58 AM, Joonas Lahtinen wrote:
>>> On Tue, 2017-10-17 at 15:50 -0700, Sujaritha Sundaresan wrote:
>>>> We currently have two module parameters that control GuC:
>>>> "enable_guc_loading" and "enable_guc_submission". Whenever
>>>> we need i915_modparams.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).
>>> I already gave comments about clarifying the commit message, that does
>>> not seem to have been addressed.
>>>
>>> Regards, Joonas
>>
>> Sorry about that, I was hoping to fix the commit message after this
>> revision.
>> I will fix the commit message in the next revised series.
> In general, it's good idea to take review comments for all of the
> patches and apply all the changes at once to avoid double digit version
> numbers :)
>
> Every revision you send to the list, should be one you feel is complete
> and ready to be merged. A new revision should only be needed when
> something new is brought up in review (usually due to the changes made
> from the last revision).
>
> The CI system automatically picks up all patch series from the mailing
> list, so sending intermediate versions will only cause unnecessary
> load. If you feel the need for iterating some feature more, it's
> adviseable to join the IRC channel and ping the reviewer there. That
> way you don't have to wait for a day or day and a half to get comments
> for the small changes.
>
> Regards, Joonas

Will make sure to keep this in mind before sending the next patches.

Thanks,

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

Re: [PATCH v7 3/4] drm/i915/guc : Updating GuC and HuC FW select function

[Michal Wajdeczko]

On Wed, 18 Oct 2017 12:29:13 +0200, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

>
>
> On 10/18/2017 4:20 AM, Sujaritha Sundaresan wrote:
>> 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
>>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_guc_fw.c | 9 ++++-----
>>   drivers/gpu/drm/i915/intel_guc_fw.h | 2 +-
>>   drivers/gpu/drm/i915/intel_huc.c    | 4 +++-
>>   3 files changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c  
>> b/drivers/gpu/drm/i915/intel_guc_fw.c
>> index ef67a36..5bffeef 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_fw.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_fw.c
>> @@ -63,7 +63,7 @@
>>    *
>>    * Return: zero when we know firmware, non-zero in other case
> Update this documentation about return.
> You have dropped call to fw_select in this series. Can you please check.
>>    */
>> -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);
>>   @@ -90,11 +90,10 @@ int intel_guc_fw_select(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))
> This should be HAS_GUC

Hmm, I'm not sure that we really need such check here at all.
We can do proper check *before* calling this function and cover both
Huc and Guc at once.

>> +			DRM_ERROR("No GuC FW known for platform with GuC!\n");
>> +		return;
>>   	}
>> -
>> -	return 0;
>>   }
>>     /*
>> 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 c8a48cb..fc61779 100644
>> --- a/drivers/gpu/drm/i915/intel_huc.c
>> +++ b/drivers/gpu/drm/i915/intel_huc.c
>> @@ -108,7 +108,9 @@ void intel_huc_select_fw(struct intel_huc *huc)
>>   		huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR;
>>   		huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR;
>>   	} else {
>> -		DRM_ERROR("No HuC firmware known for platform with HuC!\n");
>> +		/* For now, everything with a GuC also has a HuC */
>> +		if (HAS_GUC(dev_priv))
>> +			DRM_ERROR("No HuC FW known for platform with HuC!\n");
>>   		return;
>>   	}
>>   }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v7 4/4] drm/i915/guc : Decouple logs and ADS from submission

[Michal Wajdeczko]

On Wed, 18 Oct 2017 00:50:49 +0200, Sujaritha Sundaresan  
<sujaritha.sundaresan@intel.com> wrote:

> The Additional Data Struct (ADS) contains objects that are required by
> guc post FW load and are not necessarily submission-only (although that's
> our current only use-case). If in the future we load GuC with submission
> disabled to use some other GuC feature we might still end up requiring
> something inside the ADS, so it makes more sense for them to be always
> created if GuC is loaded.
>
> Similarly, we still want to access GuC logs even if GuC submission is
> disable to debug issues with GuC loading or with wathever we're using
> GuC for.
>
> To make a concrete example, the pages used by GuC to save state during
> suspend are allocated as part of the ADS.
>
> v3: Group initialization of GuC objects
>
> v2: Decoupling ADS together with logs
>
> v3: Re-factoring code as per review (Michal)
>
> v4: Rebase
>
> v5: Separating group object initialization into next patch
>     Clarifying commit message
>
> v6: Reverting to goto err format (Michal)
>     Moved guc_ads functions to dedicated file
>     Rebase
>
> v7: Rebase
>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile              |   1 +
>  drivers/gpu/drm/i915/i915_guc_submission.c | 106  
> +------------------------
>  drivers/gpu/drm/i915/intel_guc.h           |   1 +
>  drivers/gpu/drm/i915/intel_guc_ads.c       | 119  
> +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_guc_ads.h       |  31 ++++++++
>  drivers/gpu/drm/i915/intel_guc_log.c       |   6 +-
>  drivers/gpu/drm/i915/intel_uc.c            |  39 +++++++++-
>  7 files changed, 195 insertions(+), 108 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.c
>  create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile  
> b/drivers/gpu/drm/i915/Makefile
> index 6c3b048..d7ce07e 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -62,6 +62,7 @@ i915-y += i915_cmd_parser.o \
>  i915-y += intel_uc.o \
>  	  intel_uc_fw.o \
>  	  intel_guc.o \
> +	  intel_guc_ads.o \
>  	  intel_guc_ct.o \
>  	  intel_guc_log.o \
>  	  intel_guc_fw.o \
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c  
> b/drivers/gpu/drm/i915/i915_guc_submission.c
> index a2e8114..d3c0b01 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -26,6 +26,7 @@
>  #include <trace/events/dma_fence.h>
> #include "i915_guc_submission.h"
> +#include "intel_guc_ads.h"

Do we need this include here?

>  #include "i915_drv.h"
> /**
> @@ -72,13 +73,6 @@
>   * ELSP context descriptor dword into Work Item.
>   * See guc_wq_item_append()
>   *
> - * ADS:
> - * The Additional Data Struct (ADS) has pointers for different buffers  
> used by
> - * the GuC. One single gem object contains the ADS struct itself  
> (guc_ads), the
> - * scheduling policies (guc_policies), a structure describing a  
> collection of
> - * register sets (guc_mmio_reg_state) and some extra pages for the GuC  
> to save
> - * its internal state for sleep.
> - *
>   */
> static inline bool is_high_priority(struct i915_guc_client* client)
> @@ -863,7 +857,7 @@ static void guc_policy_init(struct guc_policy  
> *policy)
>  	policy->policy_flags = 0;
>  }
> -static void guc_policies_init(struct guc_policies *policies)
> +void i915_guc_policies_init(struct guc_policies *policies)

Hmm, as policies are part of the ads maybe it would be better to
move this function to _ads.c as well ?

>  {
>  	struct guc_policy *policy;
>  	u32 p, i;
> @@ -883,88 +877,6 @@ static void guc_policies_init(struct guc_policies  
> *policies)
>  }
> /*
> - * The first 80 dwords of the register state context, containing the
> - * execlists and ppgtt registers.
> - */
> -#define LR_HW_CONTEXT_SIZE	(80 * sizeof(u32))
> -
> -static int guc_ads_create(struct intel_guc *guc)
> -{
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	struct i915_vma *vma;
> -	struct page *page;
> -	/* The ads obj includes the struct itself and buffers passed to GuC */
> -	struct {
> -		struct guc_ads ads;
> -		struct guc_policies policies;
> -		struct guc_mmio_reg_state reg_state;
> -		u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
> -	} __packed *blob;
> -	struct intel_engine_cs *engine;
> -	enum intel_engine_id id;
> -	const u32 skipped_offset = LRC_HEADER_PAGES * PAGE_SIZE;
> -	const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE +  
> LR_HW_CONTEXT_SIZE;
> -	u32 base;
> -
> -	GEM_BUG_ON(guc->ads_vma);
> -
> -	vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
> -	if (IS_ERR(vma))
> -		return PTR_ERR(vma);
> -
> -	guc->ads_vma = vma;
> -
> -	page = i915_vma_first_page(vma);
> -	blob = kmap(page);
> -
> -	/* GuC scheduling policies */
> -	guc_policies_init(&blob->policies);
> -
> -	/* MMIO reg state */
> -	for_each_engine(engine, dev_priv, id) {
> -		blob->reg_state.white_list[engine->guc_id].mmio_start =
> -			engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
> -
> -		/* Nothing to be saved or restored for now. */
> -		blob->reg_state.white_list[engine->guc_id].count = 0;
> -	}
> -
> -	/*
> -	 * The GuC requires a "Golden Context" when it reinitialises
> -	 * engines after a reset. Here we use the Render ring default
> -	 * context, which must already exist and be pinned in the GGTT,
> -	 * so its address won't change after we've told the GuC where
> -	 * to find it. Note that we have to skip our header (1 page),
> -	 * because our GuC shared data is there.
> -	 */
> -	blob->ads.golden_context_lrca =
> -		guc_ggtt_offset(dev_priv->kernel_context->engine[RCS].state) +  
> skipped_offset;
> -
> -	/*
> -	 * The GuC expects us to exclude the portion of the context image that
> -	 * it skips from the size it is to read. It starts reading from after
> -	 * the execlist context (so skipping the first page [PPHWSP] and 80
> -	 * dwords). Weird guc is weird.
> -	 */
> -	for_each_engine(engine, dev_priv, id)
> -		blob->ads.eng_state_size[engine->guc_id] = engine->context_size -  
> skipped_size;
> -
> -	base = guc_ggtt_offset(vma);
> -	blob->ads.scheduler_policies = base + ptr_offset(blob, policies);
> -	blob->ads.reg_state_buffer = base + ptr_offset(blob, reg_state_buffer);
> -	blob->ads.reg_state_addr = base + ptr_offset(blob, reg_state);
> -
> -	kunmap(page);
> -
> -	return 0;
> -}
> -
> -static void guc_ads_destroy(struct intel_guc *guc)
> -{
> -	i915_vma_unpin_and_release(&guc->ads_vma);
> -}
> -
> -/*
>   * Set up the memory resources to be shared with the GuC (via the GGTT)
>   * at firmware loading time.
>   */
> @@ -994,22 +906,10 @@ int i915_guc_submission_init(struct  
> drm_i915_private *dev_priv)
> 	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;
> -
>  	ida_init(&guc->stage_ids);
> 	return 0;
> -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;
> @@ -1020,8 +920,6 @@ void i915_guc_submission_fini(struct  
> drm_i915_private *dev_priv)
>  	struct intel_guc *guc = &dev_priv->guc;
> 	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.h  
> b/drivers/gpu/drm/i915/intel_guc.h
> index 418450b..ab2496b 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -115,6 +115,7 @@ static inline u32 guc_ggtt_offset(struct i915_vma  
> *vma)
>  int intel_guc_suspend(struct drm_i915_private *dev_priv);
>  int intel_guc_resume(struct drm_i915_private *dev_priv);
>  struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32  
> size);
> +void i915_guc_policies_init(struct guc_policies *policies);
>  u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
> #endif
> diff --git a/drivers/gpu/drm/i915/intel_guc_ads.c  
> b/drivers/gpu/drm/i915/intel_guc_ads.c
> new file mode 100644
> index 0000000..b6d6442
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_guc_ads.c
> @@ -0,0 +1,119 @@
> +/*
> + * Copyright © 2017 Intel Corporation

2014-2017 as code is mostly moved

> + *
> + * Permission is hereby granted, free of charge, to any person  
> obtaining a
> + * copy of this software and associated documentation files (the  
> "Software"),
> + * to deal in the Software without restriction, including without  
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,  
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the  
> next
> + * paragraph) shall be included in all copies or substantial portions  
> of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,  
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF  
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT  
> SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR  
> OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,  
> ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER  
> DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#include "intel_uc.h"
> +#include "i915_drv.h"
> +#include "intel_guc.h"
> +#include "i915_guc_submission.h"

Please follow this order of headers:

#include <linux/required/headers.h>

#include "intel_guc_ads.h"
#include "other_remaining_headers.h"


> +
> +/* ADS:

Use kerneldoc syntax /**

> + * The Additional Data Struct (ADS) has pointers for different buffers  
> used by
> + * the GuC. One single gem object contains the ADS struct itself  
> (guc_ads), the
> + * scheduling policies (guc_policies), a structure describing a  
> collection of
> + * register sets (guc_mmio_reg_state) and some extra pages for the GuC  
> to save
> + * its internal state for sleep.
> + *
> + */
> +
> +/*
> + * The first 80 dwords of the register state context, containing the
> + * execlists and ppgtt registers.
> + */
> +#define LR_HW_CONTEXT_SIZE	(80 * sizeof(u32))
> +
> +int guc_ads_create(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	struct i915_vma *vma;
> +	struct page *page;
> +	/* The ads obj includes the struct itself and buffers passed to GuC */
> +	struct {
> +		struct guc_ads ads;
> +		struct guc_policies policies;
> +		struct guc_mmio_reg_state reg_state;
> +		u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
> +	} __packed *blob;
> +	struct intel_engine_cs *engine;
> +	enum intel_engine_id id;
> +	const u32 skipped_offset = LRC_HEADER_PAGES * PAGE_SIZE;
> +	const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE +  
> LR_HW_CONTEXT_SIZE;
> +	u32 base;
> +
> +	GEM_BUG_ON(guc->ads_vma);
> +
> +	vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
> +	if (IS_ERR(vma))
> +		return PTR_ERR(vma);
> +
> +	guc->ads_vma = vma;
> +
> +	page = i915_vma_first_page(vma);
> +	blob = kmap(page);
> +
> +	/* GuC scheduling policies */
> +	i915_guc_policies_init(&blob->policies);
> +
> +	/* MMIO reg state */
> +	for_each_engine(engine, dev_priv, id) {
> +		blob->reg_state.white_list[engine->guc_id].mmio_start =
> +			engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
> +
> +		/* Nothing to be saved or restored for now. */
> +		blob->reg_state.white_list[engine->guc_id].count = 0;
> +	}
> +
> +	/*
> +	 * The GuC requires a "Golden Context" when it reinitialises
> +	 * engines after a reset. Here we use the Render ring default
> +	 * context, which must already exist and be pinned in the GGTT,
> +	 * so its address won't change after we've told the GuC where
> +	 * to find it. Note that we have to skip our header (1 page),
> +	 * because our GuC shared data is there.
> +	 */
> +	blob->ads.golden_context_lrca =
> +		guc_ggtt_offset(dev_priv->kernel_context->engine[RCS].state) +  
> skipped_offset;
> +
> +	/*
> +	 * The GuC expects us to exclude the portion of the context image that
> +	 * it skips from the size it is to read. It starts reading from after
> +	 * the execlist context (so skipping the first page [PPHWSP] and 80
> +	 * dwords). Weird guc is weird.
> +	 */
> +	for_each_engine(engine, dev_priv, id)
> +		blob->ads.eng_state_size[engine->guc_id] = engine->context_size -  
> skipped_size;
> +
> +	base = guc_ggtt_offset(vma);
> +	blob->ads.scheduler_policies = base + ptr_offset(blob, policies);
> +	blob->ads.reg_state_buffer = base + ptr_offset(blob, reg_state_buffer);
> +	blob->ads.reg_state_addr = base + ptr_offset(blob, reg_state);
> +
> +	kunmap(page);
> +
> +	return 0;
> +}
> +
> +void guc_ads_destroy(struct intel_guc *guc)
> +{
> +	i915_vma_unpin_and_release(&guc->ads_vma);
> +}
> diff --git a/drivers/gpu/drm/i915/intel_guc_ads.h  
> b/drivers/gpu/drm/i915/intel_guc_ads.h
> new file mode 100644
> index 0000000..9f1ad11
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_guc_ads.h
> @@ -0,0 +1,31 @@
> +/*
> + * Copyright © 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person  
> obtaining a
> + * copy of this software and associated documentation files (the  
> "Software"),
> + * to deal in the Software without restriction, including without  
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,  
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the  
> next
> + * paragraph) shall be included in all copies or substantial portions  
> of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,  
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF  
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT  
> SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR  
> OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,  
> ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER  
> DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#ifndef _INTEL_GUC_ADS_H_
> +#define _INTEL_GUC_ADS_H_
> +

To make this header independent, add forward declaration for:

struct intel_guc;

> +int guc_ads_create(struct intel_guc *guc);
> +void guc_ads_destroy(struct intel_guc *guc);

And add "intel_" prefix to above functions as these are now public/exported

> +
> +#endif
> \ No newline at end of file
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c  
> b/drivers/gpu/drm/i915/intel_guc_log.c
> index 76d3eb1..1616fdb 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 (!NEEDS_GUC_LOADING(dev_priv) ||

Hmm, is this right patch ? maybe this could be done in 2/4

>  	    (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 (!NEEDS_GUC_LOADING(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 (!NEEDS_GUC_LOADING(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 df281525..1f24f35 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -24,6 +24,7 @@
> #include "intel_uc.h"
>  #include "i915_drv.h"
> +#include "intel_guc_ads.h"
>  #include "i915_guc_submission.h"
> /* Reset GuC providing us with fresh state for both GuC and HuC.
> @@ -159,6 +160,35 @@ 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;
> +
> +	if (guc->ads_vma)
> +		return 0;
> +
> +	ret = intel_guc_log_create(guc);
> +	if (ret < 0)
> +		goto err_vaddr;
> +
> +	ret = guc_ads_create(guc);
> +	if (ret < 0)
> +		goto err_log;
> +
> +err_vaddr:
> +	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);

Hmm, this looks like unbalanced cleanup.
Where exactly stage_desc_pool is created ?

> +err_log:
> +	intel_guc_log_destroy(guc);
> +
> +	return ret;
> +}
> +
> +static void guc_shared_objects_fini(struct intel_guc *guc)
> +{
> +	guc_ads_destroy(guc);
> +	intel_guc_log_destroy(guc);
> +}
> +
>  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc *guc = &dev_priv->guc;
> @@ -173,6 +203,10 @@ 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_modparams.enable_guc_submission) {
>  		/*
>  		 * This is stuff we need to have available at fw load time
> @@ -180,7 +214,7 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  		 */
>  		ret = i915_guc_submission_init(dev_priv);
>  		if (ret)
> -			goto err_guc;
> +			goto err_shared;
>  	}
> 	/* init WOPCM */
> @@ -257,6 +291,8 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  err_submission:
>  	if (i915_modparams.enable_guc_submission)
>  		i915_guc_submission_fini(dev_priv);
> +err_shared:
> +	guc_shared_objects_fini(guc);
>  err_guc:
>  	i915_ggtt_disable_guc(dev_priv);
> @@ -291,5 +327,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);
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v7 2/4] drm/i915/guc : Removing i915_modparams.enable_guc_loading module parameter

[Sujaritha]

On 10/17/2017 03:57 PM, Chris Wilson wrote:
> Quoting Sujaritha Sundaresan (2017-10-17 23:50:47)
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index dd141b2..5b9bdd0 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3201,9 +3201,12 @@ static inline unsigned int i915_sg_segment_size(void)
>>    */
>>   #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_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_modparams.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))
> NEEDS_GUC_FW ?
> -Chris
Sure. Will do.

Thanks for the review.

Sujaritha

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

Re: [PATCH v7 2/4] drm/i915/guc : Removing i915_modparams.enable_guc_loading module parameter

[Sujaritha]

On 10/18/2017 04:53 AM, Michal Wajdeczko wrote:
> On Wed, 18 Oct 2017 00:50:47 +0200, Sujaritha Sundaresan 
> <sujaritha.sundaresan@intel.com> wrote:
>
>> We currently have two module parameters that control GuC:
>> "enable_guc_loading" and "enable_guc_submission". Whenever
>> we need i915_modparams.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, 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
>>
>> Suggested by: Oscar Mateo <oscar.mateo@intel.com>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c     |  6 +--
>>  drivers/gpu/drm/i915/i915_drv.h         |  9 +++--
>>  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      |  4 --
>>  drivers/gpu/drm/i915/i915_params.h      |  1 -
>>  drivers/gpu/drm/i915/intel_uc.c         | 69 
>> ++++++++++++++++++---------------
>>  drivers/gpu/drm/i915/intel_uncore.c     |  3 +-
>>  9 files changed, 50 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index ac25d63..bc31769 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2361,7 +2361,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 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;
>>      }
>> @@ -2397,7 +2397,7 @@ 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;
>>      }
>> @@ -2496,7 +2496,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");
>
> Btw, there is also 3rd case: "failed" when we have Guc, but something 
> went
> wrong with fw loading or enabling...
>
>>          return false;
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index dd141b2..5b9bdd0 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3201,9 +3201,12 @@ static inline unsigned int 
>> i915_sg_segment_size(void)
>>   */
>>  #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_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_modparams.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))
>
> Hmm, so by the moment we add huc fw definition to the driver we will
> enable guc loading, and as huc is always present with guc, this will
> silently enable guc loading for all platforms with guc(huc) and there
> will be no option to turn this off ...
>
> What if we don't care about Huc functionality ?
>
> If there will be Huc fw, both guc and huc will be loaded.
> If there will be no Huc fw on the system (but it will be defined in 
> driver)
> then we will generate errors from Huc firware loading, and likely try 
> to load
> Guc firmware for no purpose ...

Hmm, ok. So will the change will be to make the macro

NEEDS_GUC_LOADING(dev_priv) \
     (HAS_GUC(dev_priv) && i915_mopdarams.enable_guc_submission) ?
>
>> #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 5bf96a2..692d609 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_modparams.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 527a2d2..0bbc8f0 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -3481,7 +3481,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_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 b1296a5..ec76aac 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -4026,7 +4026,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..1c25f45 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -162,10 +162,6 @@ 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)");
>> diff --git a/drivers/gpu/drm/i915/i915_params.h 
>> b/drivers/gpu/drm/i915/i915_params.h
>> index c729226..9e1e231 100644
>> --- a/drivers/gpu/drm/i915/i915_params.h
>> +++ b/drivers/gpu/drm/i915/i915_params.h
>> @@ -44,7 +44,6 @@
>>      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, guc_log_level, -1) \
>>      param(char *, guc_firmware_path, NULL) \
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 25bd162..df281525 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -49,36 +49,44 @@ static int __intel_uc_reset_hw(struct 
>> drm_i915_private *dev_priv)
>> void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>>  {
>> +    /* Verify Hardware support */
>>      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;
>> +        if (i915_modparams.enable_guc_submission > 0)
>> +            DRM_INFO("Ignoring GuC submission enable",
>> +                    "no hardware\n");
>
> Hmm, maybe to give clearer message to the user:
>
> DRM_INFO("Ignoring option %s - no hardware", "enable_guc_submission");

Will do.
>
>> + i915_modparams.enable_guc_submission = 0;
>>          return;
>>      }
>> -    /* 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 support */
>> +    if (!HAS_GUC_UCODE(dev_priv)) {
>> +        if (i915_modparams.enable_guc_submission == 1) {
>
> Hmm, I really don't like that we leak here enable_guc_submission = 2
> knowing that it will not work ...

I agree. A 2 is "required", so I will change it to
(i915_modparams.enable_guc_submission > 0).
>
>> +            DRM_INFO("Ignoring GuC submission enable",
>> +                    "no firmware\n");
>
> DRM_INFO("Ignoring option %s - no firmware", "enable_guc_submission");
>
>> + i915_modparams.enable_guc_submission = 0;
>> +        return;
>> +        }
>> -    /* Verify firmware version */
>> -    if (i915_modparams.enable_guc_loading) {
>> -        if (HAS_HUC_UCODE(dev_priv))
>> -            intel_huc_select_fw(&dev_priv->huc);
>> +        if (i915_modparams.enable_guc_submission < 0) {
>> +            i915_modparams.enable_guc_submission = 0;
>> +            return;
>> +        }
>> -        if (intel_guc_fw_select(&dev_priv->guc))
>> -            i915_modparams.enable_guc_loading = 0;
>> +        /*
>> +         * If "required" (> 1), let it continue and we will fail later
>> +         * due to the lack of firmware
>
> Hmm, but doing so will break the goal of the 'sanitize' options function
>
>> +         */
>> +
>>      }
>> -    /* 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 paltform default" (enabled if we 
>> have
>
> Typo
>
>> +     * survived to get here)
>> +     */
>> -    /* A negative value means "use platform default" */
>>      if (i915_modparams.enable_guc_submission < 0)
>> -        i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
>> +        i915_modparams.enable_guc_submission = HAS_GUC(dev_priv);
>> +
>
> Drop above extra line
>
>>  }
>> void intel_uc_init_early(struct drm_i915_private *dev_priv)
>> @@ -88,6 +96,8 @@ void intel_uc_init_early(struct drm_i915_private 
>> *dev_priv)
>> void intel_uc_init_fw(struct drm_i915_private *dev_priv)
>>  {
>> +    if (!HAS_GUC(dev_priv))
>> +        return;
>
> Do we need this now ?
>
>>      intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
>>      intel_uc_fw_fetch(dev_priv, &dev_priv->guc.fw);
>>  }
>> @@ -154,7 +164,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_LOADING(dev_priv))
>>          return 0;
>>     guc_disable_communication(guc);
>> @@ -250,22 +260,17 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>  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_submission > 1) {
>>          DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
>>          ret = -EIO;
>> +    } else if (i915_modparams.enable_guc_submission == 1) {
>> +        DRM_NOTE("Falling back from GuC submission to execlist 
>> mode\n");
>> +        i915_modparams.enable_guc_submission = 0;
>> +        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;
>>  }
>> @@ -273,7 +278,7 @@ void intel_uc_fini_hw(struct drm_i915_private 
>> *dev_priv)
>>  {
>>      guc_free_load_err_log(&dev_priv->guc);
>> -    if (!i915_modparams.enable_guc_loading)
>> +    if (!NEEDS_GUC_LOADING(dev_priv))
>>          return;
>>     if (i915_modparams.enable_guc_submission)
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c 
>> b/drivers/gpu/drm/i915/intel_uncore.c
>> index 20e3c65c..c631b0e 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -1803,8 +1803,7 @@ int intel_guc_reset(struct drm_i915_private 
>> *dev_priv)
>>  {
>>      int ret;
>> -    if (!HAS_GUC(dev_priv))
>> -        return -EINVAL;
>> +    GEM_BUG_ON(!HAS_GUC(dev_priv));
>
> Hmm, this looks like unrelated change - please move to separate patch

Will do
>
>>     intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>>      ret = gen6_hw_domain_reset(dev_priv, GEN9_GRDOM_GUC);

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

Re: [PATCH v7 3/4] drm/i915/guc : Updating GuC and HuC FW select function

[Sujaritha]

On 10/18/2017 03:29 AM, Sagar Arun Kamble wrote:
>
>
> On 10/18/2017 4:20 AM, Sujaritha Sundaresan wrote:
>> 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
>>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_guc_fw.c | 9 ++++-----
>>   drivers/gpu/drm/i915/intel_guc_fw.h | 2 +-
>>   drivers/gpu/drm/i915/intel_huc.c    | 4 +++-
>>   3 files changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c 
>> b/drivers/gpu/drm/i915/intel_guc_fw.c
>> index ef67a36..5bffeef 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_fw.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_fw.c
>> @@ -63,7 +63,7 @@
>>    *
>>    * Return: zero when we know firmware, non-zero in other case
> Update this documentation about return.
> You have dropped call to fw_select in this series. Can you please check.

I will check and update this.
>>    */
>> -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);
>>   @@ -90,11 +90,10 @@ int intel_guc_fw_select(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))
> This should be HAS_GUC

I have decided to drop this check based on Michal's review.
>> +            DRM_ERROR("No GuC FW known for platform with GuC!\n");
>> +        return;
>>       }
>> -
>> -    return 0;
>>   }
>>     /*
>> 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 c8a48cb..fc61779 100644
>> --- a/drivers/gpu/drm/i915/intel_huc.c
>> +++ b/drivers/gpu/drm/i915/intel_huc.c
>> @@ -108,7 +108,9 @@ void intel_huc_select_fw(struct intel_huc *huc)
>>           huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR;
>>           huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR;
>>       } else {
>> -        DRM_ERROR("No HuC firmware known for platform with HuC!\n");
>> +        /* For now, everything with a GuC also has a HuC */
>> +        if (HAS_GUC(dev_priv))
>> +            DRM_ERROR("No HuC FW known for platform with HuC!\n");
>>           return;
>>       }
>>   }
>

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 v7 3/4] drm/i915/guc : Updating GuC and HuC FW select function

[Sujaritha]

On 10/18/2017 02:17 PM, Michal Wajdeczko wrote:
> On Wed, 18 Oct 2017 12:29:13 +0200, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>>
>>
>> On 10/18/2017 4:20 AM, Sujaritha Sundaresan wrote:
>>> 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
>>>
>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_guc_fw.c | 9 ++++-----
>>>   drivers/gpu/drm/i915/intel_guc_fw.h | 2 +-
>>>   drivers/gpu/drm/i915/intel_huc.c    | 4 +++-
>>>   3 files changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c 
>>> b/drivers/gpu/drm/i915/intel_guc_fw.c
>>> index ef67a36..5bffeef 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc_fw.c
>>> +++ b/drivers/gpu/drm/i915/intel_guc_fw.c
>>> @@ -63,7 +63,7 @@
>>>    *
>>>    * Return: zero when we know firmware, non-zero in other case
>> Update this documentation about return.
>> You have dropped call to fw_select in this series. Can you please check.
>>>    */
>>> -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);
>>>   @@ -90,11 +90,10 @@ int intel_guc_fw_select(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))
>> This should be HAS_GUC
>
> Hmm, I'm not sure that we really need such check here at all.
> We can do proper check *before* calling this function and cover both
> Huc and Guc at once.

I will drop the check in the next revision.
>
>>> +            DRM_ERROR("No GuC FW known for platform with GuC!\n");
>>> +        return;
>>>       }
>>> -
>>> -    return 0;
>>>   }
>>>     /*
>>> 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 c8a48cb..fc61779 100644
>>> --- a/drivers/gpu/drm/i915/intel_huc.c
>>> +++ b/drivers/gpu/drm/i915/intel_huc.c
>>> @@ -108,7 +108,9 @@ void intel_huc_select_fw(struct intel_huc *huc)
>>>           huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR;
>>>           huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR;
>>>       } else {
>>> -        DRM_ERROR("No HuC firmware known for platform with HuC!\n");
>>> +        /* For now, everything with a GuC also has a HuC */
>>> +        if (HAS_GUC(dev_priv))
>>> +            DRM_ERROR("No HuC FW known for platform with HuC!\n");
>>>           return;
>>>       }
>>>   }

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 v7 2/4] drm/i915/guc : Removing i915_modparams.enable_guc_loading module parameter

[Sujaritha]

On 10/18/2017 04:53 AM, Michal Wajdeczko wrote:
> On Wed, 18 Oct 2017 00:50:47 +0200, Sujaritha Sundaresan 
> <sujaritha.sundaresan@intel.com> wrote:
>
>> We currently have two module parameters that control GuC:
>> "enable_guc_loading" and "enable_guc_submission". Whenever
>> we need i915_modparams.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, 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
>>
>> Suggested by: Oscar Mateo <oscar.mateo@intel.com>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c     |  6 +--
>>  drivers/gpu/drm/i915/i915_drv.h         |  9 +++--
>>  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      |  4 --
>>  drivers/gpu/drm/i915/i915_params.h      |  1 -
>>  drivers/gpu/drm/i915/intel_uc.c         | 69 
>> ++++++++++++++++++---------------
>>  drivers/gpu/drm/i915/intel_uncore.c     |  3 +-
>>  9 files changed, 50 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index ac25d63..bc31769 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2361,7 +2361,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 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;
>>      }
>> @@ -2397,7 +2397,7 @@ 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;
>>      }
>> @@ -2496,7 +2496,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");
>
> Btw, there is also 3rd case: "failed" when we have Guc, but something 
> went
> wrong with fw loading or enabling...
>
>>          return false;
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index dd141b2..5b9bdd0 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3201,9 +3201,12 @@ static inline unsigned int 
>> i915_sg_segment_size(void)
>>   */
>>  #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_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_modparams.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))
>
> Hmm, so by the moment we add huc fw definition to the driver we will
> enable guc loading, and as huc is always present with guc, this will
> silently enable guc loading for all platforms with guc(huc) and there
> will be no option to turn this off ...
>
> What if we don't care about Huc functionality ?
>
> If there will be Huc fw, both guc and huc will be loaded.
> If there will be no Huc fw on the system (but it will be defined in 
> driver)
> then we will generate errors from Huc firware loading, and likely try 
> to load
> Guc firmware for no purpose ...
>

Yes that is true. So if the user wants to avoid the GuC firmware from 
getting loaded,
they must not have a HuC firmware to be loaded; in addition to not using 
submission.
I think if this is included in the commit message, it will make things 
clearer.
>> #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 5bf96a2..692d609 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_modparams.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 527a2d2..0bbc8f0 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -3481,7 +3481,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_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 b1296a5..ec76aac 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -4026,7 +4026,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..1c25f45 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -162,10 +162,6 @@ 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)");
>> diff --git a/drivers/gpu/drm/i915/i915_params.h 
>> b/drivers/gpu/drm/i915/i915_params.h
>> index c729226..9e1e231 100644
>> --- a/drivers/gpu/drm/i915/i915_params.h
>> +++ b/drivers/gpu/drm/i915/i915_params.h
>> @@ -44,7 +44,6 @@
>>      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, guc_log_level, -1) \
>>      param(char *, guc_firmware_path, NULL) \
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 25bd162..df281525 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -49,36 +49,44 @@ static int __intel_uc_reset_hw(struct 
>> drm_i915_private *dev_priv)
>> void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>>  {
>> +    /* Verify Hardware support */
>>      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;
>> +        if (i915_modparams.enable_guc_submission > 0)
>> +            DRM_INFO("Ignoring GuC submission enable",
>> +                    "no hardware\n");
>
> Hmm, maybe to give clearer message to the user:
>
> DRM_INFO("Ignoring option %s - no hardware", "enable_guc_submission");
>
>> + i915_modparams.enable_guc_submission = 0;
>>          return;
>>      }
>> -    /* 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 support */
>> +    if (!HAS_GUC_UCODE(dev_priv)) {
>> +        if (i915_modparams.enable_guc_submission == 1) {
>
> Hmm, I really don't like that we leak here enable_guc_submission = 2
> knowing that it will not work ...
>
>> +            DRM_INFO("Ignoring GuC submission enable",
>> +                    "no firmware\n");
>
> DRM_INFO("Ignoring option %s - no firmware", "enable_guc_submission");
>
>> + i915_modparams.enable_guc_submission = 0;
>> +        return;
>> +        }
>> -    /* Verify firmware version */
>> -    if (i915_modparams.enable_guc_loading) {
>> -        if (HAS_HUC_UCODE(dev_priv))
>> -            intel_huc_select_fw(&dev_priv->huc);
>> +        if (i915_modparams.enable_guc_submission < 0) {
>> +            i915_modparams.enable_guc_submission = 0;
>> +            return;
>> +        }
>> -        if (intel_guc_fw_select(&dev_priv->guc))
>> -            i915_modparams.enable_guc_loading = 0;
>> +        /*
>> +         * If "required" (> 1), let it continue and we will fail later
>> +         * due to the lack of firmware
>
> Hmm, but doing so will break the goal of the 'sanitize' options function
>
>> +         */
>> +
>>      }
>> -    /* 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 paltform default" (enabled if we 
>> have
>
> Typo
>
>> +     * survived to get here)
>> +     */
>> -    /* A negative value means "use platform default" */
>>      if (i915_modparams.enable_guc_submission < 0)
>> -        i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
>> +        i915_modparams.enable_guc_submission = HAS_GUC(dev_priv);
>> +
>
> Drop above extra line
>
>>  }
>> void intel_uc_init_early(struct drm_i915_private *dev_priv)
>> @@ -88,6 +96,8 @@ void intel_uc_init_early(struct drm_i915_private 
>> *dev_priv)
>> void intel_uc_init_fw(struct drm_i915_private *dev_priv)
>>  {
>> +    if (!HAS_GUC(dev_priv))
>> +        return;
>
> Do we need this now ?
>
>>      intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
>>      intel_uc_fw_fetch(dev_priv, &dev_priv->guc.fw);
>>  }
>> @@ -154,7 +164,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_LOADING(dev_priv))
>>          return 0;
>>     guc_disable_communication(guc);
>> @@ -250,22 +260,17 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>  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_submission > 1) {
>>          DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
>>          ret = -EIO;
>> +    } else if (i915_modparams.enable_guc_submission == 1) {
>> +        DRM_NOTE("Falling back from GuC submission to execlist 
>> mode\n");
>> +        i915_modparams.enable_guc_submission = 0;
>> +        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;
>>  }
>> @@ -273,7 +278,7 @@ void intel_uc_fini_hw(struct drm_i915_private 
>> *dev_priv)
>>  {
>>      guc_free_load_err_log(&dev_priv->guc);
>> -    if (!i915_modparams.enable_guc_loading)
>> +    if (!NEEDS_GUC_LOADING(dev_priv))
>>          return;
>>     if (i915_modparams.enable_guc_submission)
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c 
>> b/drivers/gpu/drm/i915/intel_uncore.c
>> index 20e3c65c..c631b0e 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -1803,8 +1803,7 @@ int intel_guc_reset(struct drm_i915_private 
>> *dev_priv)
>>  {
>>      int ret;
>> -    if (!HAS_GUC(dev_priv))
>> -        return -EINVAL;
>> +    GEM_BUG_ON(!HAS_GUC(dev_priv));
>
> Hmm, this looks like unrelated change - please move to separate patch
>
>>     intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>>      ret = gen6_hw_domain_reset(dev_priv, GEN9_GRDOM_GUC);

Thanks for the review,

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

[PATCH v7 3/4] drm/i915/guc : Updating GuC and HuC FW 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

Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_fw.c | 9 ++++-----
 drivers/gpu/drm/i915/intel_guc_fw.h | 2 +-
 drivers/gpu/drm/i915/intel_huc.c    | 4 +++-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
index ef67a36..5bffeef 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/intel_guc_fw.c
@@ -63,7 +63,7 @@
  *
  * 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);
 
@@ -90,11 +90,10 @@ int intel_guc_fw_select(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 platform with GuC!\n");
+		return;
 	}
-
-	return 0;
 }
 
 /*
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 c8a48cb..fc61779 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -108,7 +108,9 @@ void intel_huc_select_fw(struct intel_huc *huc)
 		huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR;
 		huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR;
 	} else {
-		DRM_ERROR("No HuC firmware known for platform with HuC!\n");
+		/* For now, everything with a GuC also has a HuC */
+		if (HAS_GUC(dev_priv))
+			DRM_ERROR("No HuC FW known for platform with HuC!\n");
 		return;
 	}
 }
-- 
1.9.1

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