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

[PATCH v9 0/8] 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. Patches 4 and
5 deal with removing dependancies on enable_guc_submission.

Patch 6 introduces the enable_guc parameter. Patches 7 and 8 deal with
decoupling GuC logs and ADS from submission.

Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>

Sujaritha Sundaresan (8):
  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 firmware select function
  drm/i915/guc : Updating GuC logs to remove enable_guc_submission
    parameter
  drm/i915/guc : GEM_BUG_ON for GuC reset function
  drm/i915/guc : Introducing enable_guc module parameter
  drm/i915/guc : Decouple logs and ADS from submission
  drm/i915/guc : Calling intel_guc_init in i915_gem_init

 drivers/gpu/drm/i915/Makefile               |    1 +
 drivers/gpu/drm/i915/i915_debugfs.c         |   59 +-
 drivers/gpu/drm/i915/i915_drv.h             |    9 +-
 drivers/gpu/drm/i915/i915_gem.c             |   18 +-
 drivers/gpu/drm/i915/i915_gem_context.c     |    4 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c         |    2 +-
 drivers/gpu/drm/i915/i915_irq.c             |    4 +-
 drivers/gpu/drm/i915/i915_params.c          |   11 +-
 drivers/gpu/drm/i915/i915_params.h          |    3 +-
 drivers/gpu/drm/i915/intel_guc.c            |   67 +-
 drivers/gpu/drm/i915/intel_guc.h            |    8 +-
 drivers/gpu/drm/i915/intel_guc_ads.c        |  147 +++
 drivers/gpu/drm/i915/intel_guc_ads.h        |   33 +
 drivers/gpu/drm/i915/intel_guc_fw.c         |    6 +-
 drivers/gpu/drm/i915/intel_guc_fw.h         |    2 +-
 drivers/gpu/drm/i915/intel_guc_log.c        |    6 +-
 drivers/gpu/drm/i915/intel_guc_submission.c | 1286 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc_submission.h |   85 ++
 drivers/gpu/drm/i915/intel_huc.c            |    3 +-
 drivers/gpu/drm/i915/intel_uc.c             |  101 +--
 drivers/gpu/drm/i915/intel_uncore.c         |    3 +-
 21 files changed, 1750 insertions(+), 112 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.c
 create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.h
 create mode 100644 drivers/gpu/drm/i915/intel_guc_submission.c
 create mode 100644 drivers/gpu/drm/i915/intel_guc_submission.h

-- 
1.9.1

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

[PATCH v9 1/8] 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)

v8: Omitting DRRS seq_puts unification (Michal)

v9: Including the HAS_HUC condition (Michal)
    Updating more functions with unified message (Sagar)

Suggested by : Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 53 +++++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index add6af4..462e448 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,10 @@ static int i915_huc_load_status_info(struct seq_file *m, void *data)
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
 	struct drm_printer p;
 
-	if (!HAS_HUC_UCODE(dev_priv))
+	if (!HAS_HUC(dev_priv)) {
+		seq_puts(m, "not supported\n");
 		return 0;
+	}
 
 	p = drm_seq_file_printer(m);
 	intel_uc_fw_dump(&dev_priv->huc.fw, &p);
@@ -2380,8 +2382,10 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
 	struct drm_printer p;
 	u32 tmp, i;
 
-	if (!HAS_GUC_UCODE(dev_priv))
+	if (!HAS_GUC(dev_priv)) {
+		seq_puts(m, "not supported\n");
 		return 0;
+	}
 
 	p = drm_seq_file_printer(m);
 	intel_uc_fw_dump(&dev_priv->guc.fw, &p);
@@ -2461,9 +2465,11 @@ 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) ?
-			   "disabled" :
-			   "not supported");
+				HAS_GUC(dev_priv) ?
+				"not supported" :
+				NEEDS_GUC_FW(dev_priv) ?
+				"disabled" :
+				"failed");
 		return false;
 	}
 
@@ -2652,7 +2658,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;
 	}
 
@@ -2805,7 +2811,7 @@ static int i915_runtime_pm_status(struct seq_file *m, void *unused)
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 
 	if (!HAS_RUNTIME_PM(dev_priv))
-		seq_puts(m, "Runtime power management not supported\n");
+		seq_puts(m, "not supported\n");
 
 	seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->gt.awake));
 	seq_printf(m, "IRQs disabled: %s\n",
@@ -3407,9 +3413,13 @@ static int i915_ipc_status_show(struct seq_file *m, void *data)
 static int i915_ipc_status_open(struct inode *inode, struct file *file)
 {
 	struct drm_i915_private *dev_priv = inode->i_private;
+	struct seq_file *m;
 
-	if (!HAS_IPC(dev_priv))
-		return -ENODEV;
+	if (!HAS_IPC(dev_priv)) {
+		seq_puts(m, "not supported\n");
+		return 0;
+	}
+
 
 	return single_open(file, i915_ipc_status_show, dev_priv);
 }
@@ -3914,9 +3924,12 @@ static int cur_wm_latency_show(struct seq_file *m, void *data)
 static int pri_wm_latency_open(struct inode *inode, struct file *file)
 {
 	struct drm_i915_private *dev_priv = inode->i_private;
+	struct seq_file *m;
 
-	if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv))
-		return -ENODEV;
+	if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv)) {
+		seq_puts(m, "not supported\n");
+		return 0;
+	}
 
 	return single_open(file, pri_wm_latency_show, dev_priv);
 }
@@ -3924,9 +3937,12 @@ static int pri_wm_latency_open(struct inode *inode, struct file *file)
 static int spr_wm_latency_open(struct inode *inode, struct file *file)
 {
 	struct drm_i915_private *dev_priv = inode->i_private;
+	struct seq_file *m;
 
-	if (HAS_GMCH_DISPLAY(dev_priv))
-		return -ENODEV;
+	if (HAS_GMCH_DISPLAY(dev_priv)) {
+		seq_puts(m, "not supported\n");
+		return 0;
+	}
 
 	return single_open(file, spr_wm_latency_show, dev_priv);
 }
@@ -3934,9 +3950,12 @@ static int spr_wm_latency_open(struct inode *inode, struct file *file)
 static int cur_wm_latency_open(struct inode *inode, struct file *file)
 {
 	struct drm_i915_private *dev_priv = inode->i_private;
+	struct seq_file *m;
 
-	if (HAS_GMCH_DISPLAY(dev_priv))
-		return -ENODEV;
+	if (HAS_GMCH_DISPLAY(dev_priv)) {
+		seq_puts(m, "not supported\n");
+		return 0;
+	}
 
 	return single_open(file, cur_wm_latency_show, dev_priv);
 }
-- 
1.9.1

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

[PATCH v9 2/8] 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 submission=1, we also need loading=1.We also need
loading=1 when we want to want to verify the HuC, which
is every time we have a HuC (but all platforms with HuC
have a GuC and viceversa).

Also if we have HuC have firmware to be loaded, we need to
have GuC to actually load it. So if the user wants to avoid
the GuC from getting loaded, they must not have a HuC
firmware to be loaded, in addition to not using submission.

v2: Clarifying the commit message (Anusha)

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

v4: Rebase

v5: Separating message unification into a separate patch

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

v7: Applying review comments (Sagar)
    Rebase

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

v9: Applying review comments (Michal)

Suggested by; Oscar Mateo <oscar.mateo@intel.com>
Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_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         | 59 ++++++++++++++++------------------
 7 files changed, 35 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c94f34f..798fa8a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3219,10 +3219,13 @@ static inline unsigned int i915_sg_segment_size(void)
  * properties, so we have separate macros to test them.
  */
 #define HAS_GUC(dev_priv)	((dev_priv)->info.has_guc)
+#define HAS_HUC(dev_priv)	(HAS_GUC(dev_priv))
 #define HAS_GUC_CT(dev_priv)	((dev_priv)->info.has_guc_ct)
-#define HAS_GUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
-#define HAS_GUC_SCHED(dev_priv)	(HAS_GUC(dev_priv))
-#define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
+#define HAS_GUC_UCODE(dev_priv)	((dev_priv)->guc.fw.path != NULL)
+#define HAS_HUC_UCODE(dev_priv)	((dev_priv)->huc.fw.path != NULL)
+
+#define NEEDS_GUC_FW(dev_priv) \
+		(HAS_GUC(dev_priv) && i915_modparams.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 c05c3d7..6a819c0 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -316,7 +316,7 @@ static u32 default_desc_template(const struct drm_i915_private *i915,
 	 * present or not in use we still need a small bias as ring wraparound
 	 * at offset 0 sometimes hangs. No idea why.
 	 */
-	if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
+	if (NEEDS_GUC_FW(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 1e40eeb..b634edf 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3476,7 +3476,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_FW(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 ff00e46..a414bca 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4032,7 +4032,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 	for (i = 0; i < MAX_L3_SLICES; ++i)
 		dev_priv->l3_parity.remap_info[i] = NULL;
 
-	if (HAS_GUC_SCHED(dev_priv))
+	if (HAS_GUC(dev_priv))
 		dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
 
 	/* Let's track the enabled rps events */
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index b4faeb6..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 e85b268..648e59c 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -49,36 +49,35 @@ 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 version */
 	if (!HAS_GUC(dev_priv)) {
-		if (i915_modparams.enable_guc_loading > 0 ||
-		    i915_modparams.enable_guc_submission > 0)
+		if (i915_modparams.enable_guc_submission > 0)
+			DRM_INFO("Ignoring option %s - no hardware", "enable_guc_submission");
-
-		i915_modparams.enable_guc_loading = 0;
 		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 version */
-	if (i915_modparams.enable_guc_loading) {
-		if (HAS_HUC_UCODE(dev_priv))
-			intel_huc_select_fw(&dev_priv->huc);
-
-		if (intel_guc_fw_select(&dev_priv->guc))
-			i915_modparams.enable_guc_loading = 0;
+	/* Verify Firmware version */
+	if (!HAS_HUC_UCODE(dev_priv)) {
+		if (i915_modparams.enable_guc_submission > 0) {
+			DRM_INFO("Ignoring option %s - no firmware", "enable_guc_submission");
+			i915_modparams.enable_guc_submission = 0;
+		return;
+		}
+
+		if (i915_modparams.enable_guc_submission < 0) {
+			i915_modparams.enable_guc_submission = 0;
+		return;
+		}
 	}
 
-	/* Can't enable guc submission without guc loaded */
-	if (!i915_modparams.enable_guc_loading)
-		i915_modparams.enable_guc_submission = 0;
+	/*
+	 * A negative value means "use platform default" (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);
+	if (i915_modparams.enable_guc_submission == 1)
+		i915_modparams.enable_guc_submission = HAS_GUC(dev_priv);
 }
 
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
@@ -154,7 +153,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_FW(dev_priv))
 		return 0;
 
 	guc_disable_communication(guc);
@@ -250,22 +249,16 @@ 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");
+		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 +266,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_FW(dev_priv))
 		return;
 
 	if (i915_modparams.enable_guc_submission)
-- 
1.9.1

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

[PATCH v9 3/8] drm/i915/guc : Updating GuC and HuC firmware select function

[Sujaritha Sundaresan]

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

v2: Clarifying the commit message (Anusha)

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

v4: Rebase

v5: Separating message unification into a separate patch

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

v7: Separating from previuos patch (Sagar)
    Rebase

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

v9: Including HAS_HUC macro

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

diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
index 69ba015..112c89d 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);
 
@@ -91,10 +91,8 @@ int intel_guc_fw_select(struct intel_guc *guc)
 		guc->fw.minor_ver_wanted = GLK_FW_MINOR;
 	} else {
 		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
-		return -ENOENT;
+		return;
 	}
-
-	return 0;
 }
 
 static void guc_prepare_xfer(struct intel_guc *guc)
diff --git a/drivers/gpu/drm/i915/intel_guc_fw.h b/drivers/gpu/drm/i915/intel_guc_fw.h
index 023f5ba..7f6ccaf 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.h
+++ b/drivers/gpu/drm/i915/intel_guc_fw.h
@@ -27,7 +27,7 @@
 
 struct intel_guc;
 
-int intel_guc_fw_select(struct intel_guc *guc);
+void intel_guc_fw_select(struct intel_guc *guc);
 int intel_guc_fw_upload(struct intel_guc *guc);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 98d1725..7f4bbc1 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -108,7 +108,8 @@ 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");
+		if (HAS_HUC(dev_priv))
+			DRM_ERROR("No HuC firmware known for platform with HuC!\n");
 		return;
 	}
 }
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 648e59c..320165a 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -82,11 +82,18 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
 {
+	struct intel_guc *guc = &dev_priv->guc;
+	struct intel_huc *huc = &dev_priv->huc;
+
 	intel_guc_init_early(&dev_priv->guc);
+	intel_guc_fw_select(guc);
+	intel_huc_select_fw(huc);
 }
 
 void intel_uc_init_fw(struct drm_i915_private *dev_priv)
 {
+	if (!HAS_GUC(dev_priv))
+		return;
 	intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
 	intel_uc_fw_fetch(dev_priv, &dev_priv->guc.fw);
 }
-- 
1.9.1

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

[PATCH v9 4/8] drm/i915/guc : Updating GuC logs to remove enable_guc_submission parameter

[Sujaritha Sundaresan]

Replacing conditions to remove dependance on enable_guc_submission

v9: Including guc_log_level in the condition (Sagar)

Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_log.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 76d3eb1..4dbe5be 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_FW(dev_priv) ||
 	    (i915_modparams.guc_log_level < 0))
 		return;
 
@@ -646,7 +646,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 
 void i915_guc_log_register(struct drm_i915_private *dev_priv)
 {
-	if (!i915_modparams.enable_guc_submission ||
+	if (!NEEDS_GUC_FW(dev_priv) ||
 	    (i915_modparams.guc_log_level < 0))
 		return;
 
@@ -657,7 +657,7 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv)
 
 void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 {
-	if (!i915_modparams.enable_guc_submission)
+	if (!NEEDS_GUC_FW(dev_priv))
 		return;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
-- 
1.9.1

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

[PATCH v9 5/8] drm/i915/guc : GEM_BUG_ON for GuC reset function

[Sujaritha Sundaresan]

Including GEM_BUG_ON for GuC reset function in intel_uncore.

Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 211acee7..653ef4e 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1928,8 +1928,7 @@ int intel_reset_guc(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 v9 6/8] drm/i915/guc : Introducing enable_guc module parameter

[Sujaritha Sundaresan]

Replacing enable_guc_submission with enable_guc modparam.
In effect enable_guc is replacing enable_guc_loading and
enable_guc_submission.

Suggested by : Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  2 +-
 drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
 drivers/gpu/drm/i915/i915_irq.c         |  2 +-
 drivers/gpu/drm/i915/i915_params.c      |  7 ++++---
 drivers/gpu/drm/i915/i915_params.h      |  2 +-
 drivers/gpu/drm/i915/intel_guc.c        |  2 +-
 drivers/gpu/drm/i915/intel_uc.c         | 35 +++++++++++++++++----------------
 7 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 798fa8a..ad73cee 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3225,7 +3225,7 @@ static inline unsigned int i915_sg_segment_size(void)
 #define HAS_HUC_UCODE(dev_priv)	((dev_priv)->huc.fw.path != NULL)
 
 #define NEEDS_GUC_FW(dev_priv) \
-		(HAS_GUC(dev_priv) && i915_modparams.enable_guc_submission)
+		(HAS_GUC(dev_priv) && i915_modparams.enable_guc)
 
 #define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)->info.has_resource_streamer)
 
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 6a819c0..43210df 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -409,7 +409,7 @@ struct i915_gem_context *
 	i915_gem_context_set_closed(ctx); /* not user accessible */
 	i915_gem_context_clear_bannable(ctx);
 	i915_gem_context_set_force_single_submission(ctx);
-	if (!i915_modparams.enable_guc_submission)
+	if (!i915_modparams.enable_guc)
 		ctx->ring_size = 512 * PAGE_SIZE; /* Max ring buffer size */
 
 	GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a414bca..693b345 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1400,7 +1400,7 @@ static void snb_gt_irq_handler(struct drm_i915_private *dev_priv,
 
 	if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) {
 		notify_ring(engine);
-		tasklet |= i915_modparams.enable_guc_submission;
+		tasklet |= i915_modparams.enable_guc;
 	}
 
 	if (tasklet)
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 1c25f45..51cf6bd 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -162,9 +162,10 @@ struct i915_params i915_modparams __read_mostly = {
 	"(0=use value from vbt [default], 1=low power swing(200mV),"
 	"2=default swing(400mV))");
 
-i915_param_named_unsafe(enable_guc_submission, int, 0400,
-	"Enable GuC submission "
-	"(-1=auto, 0=never [default], 1=if available, 2=required)");
+i915_param_named_unsafe(enable_guc, int, 0400,
+	"Enable GuC submission and loading "
+	"(-1=auto [default], 0=No GuC or HuC, 1=Load & use GuC, HuC on the side"
+	" 2=Load GuC only for HuC)");
 
 i915_param_named(guc_log_level, int, 0400,
 	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 9e1e231..7bf4dce 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -44,7 +44,7 @@
 	param(int, disable_power_well, -1) \
 	param(int, enable_ips, 1) \
 	param(int, invert_brightness, 0) \
-	param(int, enable_guc_submission, 0) \
+	param(int, enable_guc, -1) \
 	param(int, guc_log_level, -1) \
 	param(char *, guc_firmware_path, NULL) \
 	param(char *, huc_firmware_path, NULL) \
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 823d0c2..629ef5d 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -128,7 +128,7 @@ void intel_guc_init_params(struct intel_guc *guc)
 	}
 
 	/* If GuC submission is enabled, set up additional parameters here */
-	if (i915_modparams.enable_guc_submission) {
+	if (i915_modparams.enable_guc) {
 		u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
 		u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool);
 		u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 320165a..980de7a 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -51,33 +51,34 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
 {
 	/* Verify Hardware version */
 	if (!HAS_GUC(dev_priv)) {
-		if (i915_modparams.enable_guc_submission > 0)
+		if (i915_modparams.enable_guc > 0)
-			DRM_INFO("Ignoring option %s - no hardware", "enable_guc_submission");
+			DRM_INFO("Ignoring option %s - no hardware", "enable_guc");
-		i915_modparams.enable_guc_submission = 0;
+		i915_modparams.enable_guc = 0;
 		return;
 	}
 
 	/* Verify Firmware version */
 	if (!HAS_HUC_UCODE(dev_priv)) {
-		if (i915_modparams.enable_guc_submission > 0) {
+		if (i915_modparams.enable_guc > 0) {
-			DRM_INFO("Ignoring option %s - no firmware", "enable_guc_submission");
+			DRM_INFO("Ignoring option %s - no firmware", "enable_guc");
-			i915_modparams.enable_guc_submission = 0;
+			i915_modparams.enable_guc = 0;
 		return;
 		}
 	
-		if (i915_modparams.enable_guc_submission < 0) {
-			i915_modparams.enable_guc_submission = 0;
-		return;
+		if (i915_modparams.enable_guc < 0) {
+			i915_modparams.enable_guc = 0;
+			return;
 		}
 	}
+
 
 	/*
 	 * A negative value means "use platform default" (enabled if we have
 	 * survived to get here)
 	 */
 
-	if (i915_modparams.enable_guc_submission == 1)
-		i915_modparams.enable_guc_submission = HAS_GUC(dev_priv);
+	if (i915_modparams.enable_guc < 0)
+		i915_modparams.enable_guc = 1;
 }
 
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
@@ -169,7 +170,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	/* We need to notify the guc whenever we change the GGTT */
 	i915_ggtt_enable_guc(dev_priv);
 
-	if (i915_modparams.enable_guc_submission) {
+	if (i915_modparams.enable_guc) {
 		/*
 		 * This is stuff we need to have available at fw load time
 		 * if we are planning to enable submission later
@@ -219,7 +220,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 		goto err_log_capture;
 
 	intel_huc_auth(&dev_priv->huc);
-	if (i915_modparams.enable_guc_submission) {
+	if (i915_modparams.enable_guc) {
 		if (i915_modparams.guc_log_level >= 0)
 			gen9_enable_guc_interrupts(dev_priv);
 
@@ -229,7 +230,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	}
 
 	dev_info(dev_priv->drm.dev, "GuC %s (firmware %s [version %u.%u])\n",
-		 i915_modparams.enable_guc_submission ? "submission enabled" :
+		 i915_modparams.enable_guc ? "submission enabled" :
 							"loaded",
 		 guc->fw.path,
 		 guc->fw.major_ver_found, guc->fw.minor_ver_found);
@@ -251,15 +252,15 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 err_log_capture:
 	guc_capture_load_err_log(guc);
 err_submission:
-	if (i915_modparams.enable_guc_submission)
+	if (i915_modparams.enable_guc)
 		intel_guc_submission_fini(guc);
 err_guc:
 	i915_ggtt_disable_guc(dev_priv);
 
-	if (i915_modparams.enable_guc_submission > 1) {
+	if (i915_modparams.enable_guc > 1) {
 		DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
 		ret = -EIO;
-	} else if (i915_modparams.enable_guc_submission == 1) {
+	} else if (i915_modparams.enable_guc == 1) {
 		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
 		ret = 0;
 	} else {
@@ -276,12 +277,12 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 	if (!NEEDS_GUC_FW(dev_priv))
 		return;
 
-	if (i915_modparams.enable_guc_submission)
+	if (i915_modparams.enable_guc)
 		intel_guc_submission_disable(&dev_priv->guc);
 
 	guc_disable_communication(&dev_priv->guc);
 
-	if (i915_modparams.enable_guc_submission) {
+	if (i915_modparams.enable_guc) {
 		gen9_disable_guc_interrupts(dev_priv);
 		intel_guc_submission_fini(&dev_priv->guc);
 	}
-- 
1.9.1

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

[PATCH v9 7/8] 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

v8: Applying review comments (Michal)

v9: Defining intel_guc_init function (Sagar)
    Applying review comments (Michal, Sagar)

Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/Makefile               |   1 +
 drivers/gpu/drm/i915/intel_guc.c            |  63 ++++++++++
 drivers/gpu/drm/i915/intel_guc.h            |   2 +
 drivers/gpu/drm/i915/intel_guc_ads.c        | 147 ++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc_ads.h        |  33 +++++
 drivers/gpu/drm/i915/intel_guc_submission.c | 189 +---------------------------
 drivers/gpu/drm/i915/intel_guc_submission.h |   9 +-
 drivers/gpu/drm/i915/intel_uc.c             |  16 +--
 8 files changed, 261 insertions(+), 199 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 9469c37..694e7ca 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -82,6 +82,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_fw.o \
 	  intel_guc_log.o \
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 629ef5d..c688586 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -23,6 +23,7 @@
  */
 
 #include "intel_guc.h"
+#include "intel_guc_ads.h"
 #include "intel_guc_submission.h"
 #include "i915_drv.h"
 
@@ -224,6 +225,68 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 	return ret;
 }
 
+/*
+ * Set up the memory resources to be shared with the GuC (via the GGTT)
+ * at firmware loading time.
+ */
+
+int intel_guc_init(struct intel_guc *guc)
+{
+	int ret;
+
+	if (guc->stage_desc_pool)
+		return 0;
+
+	ret = guc_stage_desc_pool_create(guc);
+	if (ret)
+		return ret;
+	/*
+	 * Keep static analysers happy, let them know that we allocated the
+	 * vma after testing that it didn't exist earlier.
+	 */
+	GEM_BUG_ON(!guc->stage_desc_pool);
+
+	ret = guc_shared_data_create(guc);
+	if (ret)
+		goto err_stage_desc_pool;
+	GEM_BUG_ON(!guc->shared_data);
+
+	ret = intel_guc_log_create(guc);
+	if (ret < 0)
+		goto err_shared_data;
+
+	ret = guc_preempt_work_create(guc);
+	if (ret)
+		goto err_log;
+	GEM_BUG_ON(!guc->preempt_wq);
+
+	ret = intel_guc_ads_create(guc);
+	if (ret < 0)
+		goto err_wq;
+	GEM_BUG_ON(!guc->ads_vma);
+
+	return 0;
+
+err_wq:
+	guc_preempt_work_destroy(guc);
+err_log:
+	intel_guc_log_destroy(guc);
+err_shared_data:
+	guc_shared_data_destroy(guc);
+err_stage_desc_pool:
+	guc_stage_desc_pool_destroy(guc);
+	return ret;
+}
+
+void intel_guc_fini(struct intel_guc *guc)
+{
+	intel_guc_ads_destroy(guc);
+	guc_preempt_work_destroy(guc);
+	intel_guc_log_destroy(guc);
+	guc_shared_data_destroy(guc);
+	guc_stage_desc_pool_destroy(guc);
+}
+
 int intel_guc_sample_forcewake(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 75c4cfe..a805c79 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -121,6 +121,8 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 void intel_guc_init_params(struct intel_guc *guc);
 int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
 int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
+int intel_guc_init(struct intel_guc *guc);
+void intel_guc_fini(struct intel_guc *guc);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
 int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
 int intel_guc_suspend(struct drm_i915_private *dev_priv);
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..c97704d
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_guc_ads.c
@@ -0,0 +1,147 @@
+/*
+ * Copyright © 2014-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_guc_ads.h"
+#include "intel_uc.h"
+#include "i915_drv.h"
+#include "intel_guc_submission.h"
+
+/**
+ * DOC: GuC Additional Data Struct
+ * 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 void guc_policy_init(struct guc_policy *policy)
+{
+	policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
+	policy->preemption_time = POLICY_DEFAULT_PREEMPTION_TIME_US;
+	policy->fault_time = POLICY_DEFAULT_FAULT_TIME_US;
+	policy->policy_flags = 0;
+}
+
+static void guc_policies_init(struct guc_policies *policies)
+{
+	struct guc_policy *policy;
+	u32 p, i;
+
+	policies->dpc_promote_time = POLICY_DEFAULT_DPC_PROMOTE_TIME_US;
+	policies->max_num_work_items = POLICY_MAX_NUM_WI;
+
+	for (p = 0; p < GUC_CLIENT_PRIORITY_NUM; p++) {
+		for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
+			policy = &policies->policy[p][i];
+
+			guc_policy_init(policy);
+		}
+	}
+
+	policies->is_valid = 1;
+}
+
+/*
+ * The first 80 dwords of the register state context, containing the
+ * execlists and ppgtt registers.
+ */
+#define LR_HW_CONTEXT_SIZE	(80 * sizeof(u32))
+
+int intel_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;
+}
+
+void intel_guc_ads_destroy(struct intel_guc *guc)
+{
+	i915_vma_unpin_and_release(&guc->ads_vma);
+}
+
\ No newline at end of file
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..cf5821a
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_guc_ads.h
@@ -0,0 +1,33 @@
+/*
+ * 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_
+
+struct intel_guc;
+
+int intel_guc_ads_create(struct intel_guc *guc);
+void intel_guc_ads_destroy(struct intel_guc *guc);
+
+#endif
+
\ No newline at end of file
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index cca56cc..ce7d96f1 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -73,13 +73,6 @@
  * ELSP context descriptor dword into Work Item.
  * See guc_add_request()
  *
- * 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 intel_guc_client* client)
@@ -316,7 +309,7 @@ static void guc_proc_desc_init(struct intel_guc *guc,
 	desc->priority = client->priority;
 }
 
-static int guc_stage_desc_pool_create(struct intel_guc *guc)
+int guc_stage_desc_pool_create(struct intel_guc *guc)
 {
 	struct i915_vma *vma;
 	void *vaddr;
@@ -340,7 +333,7 @@ static int guc_stage_desc_pool_create(struct intel_guc *guc)
 	return 0;
 }
 
-static void guc_stage_desc_pool_destroy(struct intel_guc *guc)
+void guc_stage_desc_pool_destroy(struct intel_guc *guc)
 {
 	ida_destroy(&guc->stage_ids);
 	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
@@ -444,7 +437,7 @@ static void guc_stage_desc_fini(struct intel_guc *guc,
 	memset(desc, 0, sizeof(*desc));
 }
 
-static int guc_shared_data_create(struct intel_guc *guc)
+int guc_shared_data_create(struct intel_guc *guc)
 {
 	struct i915_vma *vma;
 	void *vaddr;
@@ -465,7 +458,7 @@ static int guc_shared_data_create(struct intel_guc *guc)
 	return 0;
 }
 
-static void guc_shared_data_destroy(struct intel_guc *guc)
+void guc_shared_data_destroy(struct intel_guc *guc)
 {
 	i915_gem_object_unpin_map(guc->shared_data->obj);
 	i915_vma_unpin_and_release(&guc->shared_data);
@@ -1095,116 +1088,7 @@ static void guc_clients_destroy(struct intel_guc *guc)
 	guc_client_free(client);
 }
 
-static void guc_policy_init(struct guc_policy *policy)
-{
-	policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
-	policy->preemption_time = POLICY_DEFAULT_PREEMPTION_TIME_US;
-	policy->fault_time = POLICY_DEFAULT_FAULT_TIME_US;
-	policy->policy_flags = 0;
-}
-
-static void guc_policies_init(struct guc_policies *policies)
-{
-	struct guc_policy *policy;
-	u32 p, i;
-
-	policies->dpc_promote_time = POLICY_DEFAULT_DPC_PROMOTE_TIME_US;
-	policies->max_num_work_items = POLICY_MAX_NUM_WI;
-
-	for (p = 0; p < GUC_CLIENT_PRIORITY_NUM; p++) {
-		for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
-			policy = &policies->policy[p][i];
-
-			guc_policy_init(policy);
-		}
-	}
-
-	policies->is_valid = 1;
-}
-
-/*
- * 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);
-}
-
-static int guc_preempt_work_create(struct intel_guc *guc)
+int guc_preempt_work_create(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct intel_engine_cs *engine;
@@ -1236,7 +1120,7 @@ static int guc_preempt_work_create(struct intel_guc *guc)
 	return 0;
 }
 
-static void guc_preempt_work_destroy(struct intel_guc *guc)
+void guc_preempt_work_destroy(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct intel_engine_cs *engine;
@@ -1249,67 +1133,6 @@ static void guc_preempt_work_destroy(struct intel_guc *guc)
 	guc->preempt_wq = NULL;
 }
 
-/*
- * Set up the memory resources to be shared with the GuC (via the GGTT)
- * at firmware loading time.
- */
-int intel_guc_submission_init(struct intel_guc *guc)
-{
-	int ret;
-
-	if (guc->stage_desc_pool)
-		return 0;
-
-	ret = guc_stage_desc_pool_create(guc);
-	if (ret)
-		return ret;
-	/*
-	 * Keep static analysers happy, let them know that we allocated the
-	 * vma after testing that it didn't exist earlier.
-	 */
-	GEM_BUG_ON(!guc->stage_desc_pool);
-
-	ret = guc_shared_data_create(guc);
-	if (ret)
-		goto err_stage_desc_pool;
-	GEM_BUG_ON(!guc->shared_data);
-
-	ret = intel_guc_log_create(guc);
-	if (ret < 0)
-		goto err_shared_data;
-
-	ret = guc_preempt_work_create(guc);
-	if (ret)
-		goto err_log;
-	GEM_BUG_ON(!guc->preempt_wq);
-
-	ret = guc_ads_create(guc);
-	if (ret < 0)
-		goto err_wq;
-	GEM_BUG_ON(!guc->ads_vma);
-
-	return 0;
-
-err_wq:
-	guc_preempt_work_destroy(guc);
-err_log:
-	intel_guc_log_destroy(guc);
-err_shared_data:
-	guc_shared_data_destroy(guc);
-err_stage_desc_pool:
-	guc_stage_desc_pool_destroy(guc);
-	return ret;
-}
-
-void intel_guc_submission_fini(struct intel_guc *guc)
-{
-	guc_ads_destroy(guc);
-	guc_preempt_work_destroy(guc);
-	intel_guc_log_destroy(guc);
-	guc_shared_data_destroy(guc);
-	guc_stage_desc_pool_destroy(guc);
-}
-
 static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
 {
 	struct intel_rps *rps = &dev_priv->gt_pm.rps;
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.h b/drivers/gpu/drm/i915/intel_guc_submission.h
index 3e46916..15af6ef 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.h
+++ b/drivers/gpu/drm/i915/intel_guc_submission.h
@@ -72,9 +72,14 @@ struct intel_guc_client {
 	u64 submissions[I915_NUM_ENGINES];
 };
 
-int intel_guc_submission_init(struct intel_guc *guc);
 int intel_guc_submission_enable(struct intel_guc *guc);
 void intel_guc_submission_disable(struct intel_guc *guc);
-void intel_guc_submission_fini(struct intel_guc *guc);
+
+int guc_stage_desc_pool_create(struct intel_guc *guc);
+int guc_shared_data_create(struct intel_guc *guc);
+int guc_preempt_work_create(struct intel_guc *guc);
+void guc_stage_desc_pool_destroy(struct intel_guc *guc);
+void guc_shared_data_destroy(struct intel_guc *guc);
+void guc_preempt_work_destroy(struct intel_guc *guc);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 980de7a..0e2c4a6 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -170,16 +170,6 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	/* We need to notify the guc whenever we change the GGTT */
 	i915_ggtt_enable_guc(dev_priv);
 
-	if (i915_modparams.enable_guc) {
-		/*
-		 * This is stuff we need to have available at fw load time
-		 * if we are planning to enable submission later
-		 */
-		ret = intel_guc_submission_init(guc);
-		if (ret)
-			goto err_guc;
-	}
-
 	/* init WOPCM */
 	I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(dev_priv));
 	I915_WRITE(DMA_GUC_WOPCM_OFFSET,
@@ -253,9 +243,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	guc_capture_load_err_log(guc);
 err_submission:
 	if (i915_modparams.enable_guc)
-		intel_guc_submission_fini(guc);
-err_guc:
-	i915_ggtt_disable_guc(dev_priv);
+		intel_guc_fini(guc);
 
 	if (i915_modparams.enable_guc > 1) {
 		DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
@@ -284,7 +272,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 
 	if (i915_modparams.enable_guc) {
 		gen9_disable_guc_interrupts(dev_priv);
-		intel_guc_submission_fini(&dev_priv->guc);
+		intel_guc_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

[PATCH v9 8/8] drm/i915/guc : Calling intel_guc_init in i915_gem_init

[Sujaritha Sundaresan]

Placing the call to intel_guc_init after i915_gem_contexts_init,
based on the dependency within i915_gem_init.

Will move the function if required, depending on the review
comments.

Suggested by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 889ae88..c877a5d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -36,6 +36,7 @@
 #include "intel_frontbuffer.h"
 #include "intel_mocs.h"
 #include "i915_gemfs.h"
+#include "intel_guc.h"
 #include <linux/dma-fence-array.h>
 #include <linux/kthread.h>
 #include <linux/reservation.h>
@@ -4972,6 +4973,7 @@ bool intel_sanitize_semaphores(struct drm_i915_private *dev_priv, int value)
 
 int i915_gem_init(struct drm_i915_private *dev_priv)
 {
+	struct intel_guc *guc = &dev_priv->guc;
 	int ret;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
@@ -5015,6 +5017,18 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto out_unlock;
 
+	ret = intel_guc_init(guc);
+
+		if (i915_modparams.enable_guc) {
+			/*
+			 * This is stuff we need to have available at fw load time
+			 * if we are planning to enable submission later
+			 */
+			ret = intel_guc_init(guc);
+			if (ret)
+				goto err_shared;
+		}
+
 	ret = intel_engines_init(dev_priv);
 	if (ret)
 		goto out_unlock;
@@ -5035,7 +5049,8 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 out_unlock:
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
-
+err_shared:
+	intel_guc_fini(guc);
 	return ret;
 }
 
@@ -5192,6 +5207,7 @@ void i915_gem_load_cleanup(struct drm_i915_private *dev_priv)
 	rcu_barrier();
 
 	i915_gemfs_fini(dev_priv);
+	intel_guc_fini(&dev_priv->guc);
 }
 
 int i915_gem_freeze(struct drm_i915_private *dev_priv)
-- 
1.9.1

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

Re: [PATCH v9 1/8] drm/i915 : Unifying seq_puts messages for feature support

[Chris Wilson]

Quoting Sujaritha Sundaresan (2017-11-11 00:06:31)
> Unifying the various seq_puts messages in debugfs to the simplest one for
> feature support.

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

Re: [PATCH v9 1/8] drm/i915 : Unifying seq_puts messages for feature support

[Michal Wajdeczko]

On Sat, 11 Nov 2017 01:06:31 +0100, 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)
>
> v8: Omitting DRRS seq_puts unification (Michal)
>
> v9: Including the HAS_HUC condition (Michal)
>     Updating more functions with unified message (Sagar)
>
> Suggested by : Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 53  
> +++++++++++++++++++++++++------------
>  1 file changed, 36 insertions(+), 17 deletions(-)
>

<snip>

> @@ -2361,8 +2361,10 @@ static int i915_huc_load_status_info(struct  
> seq_file *m, void *data)
>  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
>  	struct drm_printer p;
> -	if (!HAS_HUC_UCODE(dev_priv))
> +	if (!HAS_HUC(dev_priv)) {

Hmm, HAS_HUC is not available yet, it will be introduced by patch 2/8

<snip>

> @@ -2461,9 +2465,11 @@ 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) ?
> -			   "disabled" :
> -			   "not supported");
> +				HAS_GUC(dev_priv) ?
> +				"not supported" :
> +				NEEDS_GUC_FW(dev_priv) ?

NEEDS_GUC_FW will also be introduced by patch 2/8

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

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

[Michal Wajdeczko]

On Sat, 11 Nov 2017 01:06:32 +0100, 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 submission=1, we also need loading=1.We also need
> loading=1 when we want to want to verify the HuC, which
> is every time we have a HuC (but all platforms with HuC
> have a GuC and viceversa).
>
> Also if we have HuC have firmware to be loaded, we need to
> have GuC to actually load it. So if the user wants to avoid
> the GuC from getting loaded, they must not have a HuC
> firmware to be loaded, in addition to not using submission.
>
> v2: Clarifying the commit message (Anusha)
>
> v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
>
> v4: Rebase
>
> v5: Separating message unification into a separate patch
>
> v6: Re-factoring code (Sagar, Michal)
>     Rebase
>
> v7: Applying review comments (Sagar)
>     Rebase
>
> v8: Change to NEEDS_GUC_FW (Chris)
>     Applying review comments (Michal)
>     Clarifying commit message (Joonas)
>
> v9: Applying review comments (Michal)
>
> Suggested by; Oscar Mateo <oscar.mateo@intel.com>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_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         | 59  
> ++++++++++++++++------------------
>  7 files changed, 35 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h  
> b/drivers/gpu/drm/i915/i915_drv.h
> index c94f34f..798fa8a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3219,10 +3219,13 @@ static inline unsigned int  
> i915_sg_segment_size(void)
>   * properties, so we have separate macros to test them.
>   */
>  #define HAS_GUC(dev_priv)	((dev_priv)->info.has_guc)
> +#define HAS_HUC(dev_priv)	(HAS_GUC(dev_priv))
>  #define HAS_GUC_CT(dev_priv)	((dev_priv)->info.has_guc_ct)
> -#define HAS_GUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
> -#define HAS_GUC_SCHED(dev_priv)	(HAS_GUC(dev_priv))
> -#define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
> +#define HAS_GUC_UCODE(dev_priv)	((dev_priv)->guc.fw.path != NULL)
> +#define HAS_HUC_UCODE(dev_priv)	((dev_priv)->huc.fw.path != NULL)

Btw, can we avoid adding ucode as alias for firmware and just use fw:

#define HAS_GUC_FW(dev_priv)	((dev_priv)->guc.fw.path != NULL)
#define HAS_HUC_FW(dev_priv)	((dev_priv)->huc.fw.path != NULL)

Also, maybe we should rather rely on fetch status instead of path:

#define HAS_GUC_FW(dev_priv)	((dev_priv)->guc.fw.fetch_status ==  
INTEL_UC_FIRMWARE_SUCCESS)
#define HAS_HUC_FW(dev_priv)	((dev_priv)->huc.fw.fetch_status ==  
INTEL_UC_FIRMWARE_SUCCESS)

> +
> +#define NEEDS_GUC_FW(dev_priv) \
> +		(HAS_GUC(dev_priv) && i915_modparams.enable_guc_submission)

With updated earlier macro names, our new macro can be defined as:

#define NEEDS_GUC_LOAD(dev_priv) \
	(HAS_GUC(dev_priv) && \
	 HAS_GUC_FW(dev_priv) && \
	 ( HAS_HUC_FW(dev_priv) || i915_modparams.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 c05c3d7..6a819c0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -316,7 +316,7 @@ static u32 default_desc_template(const struct  
> drm_i915_private *i915,
>  	 * present or not in use we still need a small bias as ring wraparound
>  	 * at offset 0 sometimes hangs. No idea why.
>  	 */
> -	if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
> +	if (NEEDS_GUC_FW(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 1e40eeb..b634edf 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3476,7 +3476,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_FW(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 ff00e46..a414bca 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4032,7 +4032,7 @@ void intel_irq_init(struct drm_i915_private  
> *dev_priv)
>  	for (i = 0; i < MAX_L3_SLICES; ++i)
>  		dev_priv->l3_parity.remap_info[i] = NULL;
> -	if (HAS_GUC_SCHED(dev_priv))
> +	if (HAS_GUC(dev_priv))
>  		dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
> 	/* Let's track the enabled rps events */
> diff --git a/drivers/gpu/drm/i915/i915_params.c  
> b/drivers/gpu/drm/i915/i915_params.c
> index b4faeb6..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 e85b268..648e59c 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -49,36 +49,35 @@ 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 version */
>  	if (!HAS_GUC(dev_priv)) {
> -		if (i915_modparams.enable_guc_loading > 0 ||
> -		    i915_modparams.enable_guc_submission > 0)
> +		if (i915_modparams.enable_guc_submission > 0)
> +			DRM_INFO("Ignoring option %s - no hardware",  
> "enable_guc_submission");
> -
> -		i915_modparams.enable_guc_loading = 0;
>  		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 version */
> -	if (i915_modparams.enable_guc_loading) {
> -		if (HAS_HUC_UCODE(dev_priv))
> -			intel_huc_select_fw(&dev_priv->huc);
> -
> -		if (intel_guc_fw_select(&dev_priv->guc))
> -			i915_modparams.enable_guc_loading = 0;
> +	/* Verify Firmware version */
> +	if (!HAS_HUC_UCODE(dev_priv)) {

s/HUC/GUC as we rather should check for GuC fw code here

> +		if (i915_modparams.enable_guc_submission > 0) {
> +			DRM_INFO("Ignoring option %s - no firmware",  
> "enable_guc_submission");
> +			i915_modparams.enable_guc_submission = 0;
> +		return;
> +		}
> +
> +		if (i915_modparams.enable_guc_submission < 0) {
> +			i915_modparams.enable_guc_submission = 0;
> +		return;
> +		}

Hmm, maybe we combine both above cases into:

	if (!HAS_GUC_UCODE(dev_priv)) {
		if (i915_modparams.enable_guc_submission > 0) {
			DRM_INFO("Ignoring option %s - %s\n",
			         "enable_guc_submission",
			         HAS_GUC(dev_priv) ?
			         "no firmware" : "no hardware");

>  	}
> -	/* Can't enable guc submission without guc loaded */
> -	if (!i915_modparams.enable_guc_loading)
> -		i915_modparams.enable_guc_submission = 0;
> +	/*
> +	 * A negative value means "use platform default" (enabled if we have
> +	 * survived to get here)
> +	 */

Hmm, this comment is little misleading, as we don't have any per platform  
flag
for enabling GuC submission. For now it will be always enabled or  
disabled...

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

Hmm, HAS_GUC is always 1 at this point, so above looks like NOP

>  }
> void intel_uc_init_early(struct drm_i915_private *dev_priv)
> @@ -154,7 +153,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_FW(dev_priv))
>  		return 0;
> 	guc_disable_communication(guc);
> @@ -250,22 +249,16 @@ 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");
> +		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 +266,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_FW(dev_priv))
>  		return;
> 	if (i915_modparams.enable_guc_submission)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v9 3/8] drm/i915/guc : Updating GuC and HuC firmware select function

[Michal Wajdeczko]

On Sat, 11 Nov 2017 01:06:33 +0100, Sujaritha Sundaresan  
<sujaritha.sundaresan@intel.com> 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
>
> v8: Including change to intel_uc.c
>     Applying review comments (Michal)
>
> v9: Including HAS_HUC macro
>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_fw.c | 6 ++----
>  drivers/gpu/drm/i915/intel_guc_fw.h | 2 +-
>  drivers/gpu/drm/i915/intel_huc.c    | 3 ++-
>  drivers/gpu/drm/i915/intel_uc.c     | 7 +++++++
>  4 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c  
> b/drivers/gpu/drm/i915/intel_guc_fw.c
> index 69ba015..112c89d 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);
> @@ -91,10 +91,8 @@ int intel_guc_fw_select(struct intel_guc *guc)
>  		guc->fw.minor_ver_wanted = GLK_FW_MINOR;
>  	} else {
>  		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
> -		return -ENOENT;
> +		return;
>  	}
> -
> -	return 0;
>  }
> static void guc_prepare_xfer(struct intel_guc *guc)
> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.h  
> b/drivers/gpu/drm/i915/intel_guc_fw.h
> index 023f5ba..7f6ccaf 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fw.h
> +++ b/drivers/gpu/drm/i915/intel_guc_fw.h
> @@ -27,7 +27,7 @@
> struct intel_guc;
> -int intel_guc_fw_select(struct intel_guc *guc);
> +void intel_guc_fw_select(struct intel_guc *guc);
>  int intel_guc_fw_upload(struct intel_guc *guc);
> #endif
> diff --git a/drivers/gpu/drm/i915/intel_huc.c  
> b/drivers/gpu/drm/i915/intel_huc.c
> index 98d1725..7f4bbc1 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -108,7 +108,8 @@ 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");
> +		if (HAS_HUC(dev_priv))

Can we move this check to the caller ? see intel_uc_init_fw
And what about GuC ?

> +			DRM_ERROR("No HuC firmware known for platform with HuC!\n");
>  		return;
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index 648e59c..320165a 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -82,11 +82,18 @@ void intel_uc_sanitize_options(struct  
> drm_i915_private *dev_priv)
> void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  {
> +	struct intel_guc *guc = &dev_priv->guc;
> +	struct intel_huc *huc = &dev_priv->huc;
> +
>  	intel_guc_init_early(&dev_priv->guc);

Btw, now you can pass 'guc' as param

> +	intel_guc_fw_select(guc);
> +	intel_huc_select_fw(huc);
>  }
> void intel_uc_init_fw(struct drm_i915_private *dev_priv)
>  {
> +	if (!HAS_GUC(dev_priv))
> +		return;
>  	intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
>  	intel_uc_fw_fetch(dev_priv, &dev_priv->guc.fw);
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v9 4/8] drm/i915/guc : Updating GuC logs to remove enable_guc_submission parameter

[Michal Wajdeczko]

On Sat, 11 Nov 2017 01:06:34 +0100, Sujaritha Sundaresan  
<sujaritha.sundaresan@intel.com> wrote:

> Replacing conditions to remove dependance on enable_guc_submission

typo ;)

>
> v9: Including guc_log_level in the condition (Sagar)
>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_log.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c  
> b/drivers/gpu/drm/i915/intel_guc_log.c
> index 76d3eb1..4dbe5be 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_FW(dev_priv) ||

Hmm, maybe in all these places we should rather check GuC firmware load
status directly? We don't care here why it was loaded, we just want to
verify that it is available.

>  	    (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_FW(dev_priv) ||
>  	    (i915_modparams.guc_log_level < 0))
>  		return;
> @@ -657,7 +657,7 @@ void i915_guc_log_register(struct drm_i915_private  
> *dev_priv)
> void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>  {
> -	if (!i915_modparams.enable_guc_submission)
> +	if (!NEEDS_GUC_FW(dev_priv))
>  		return;
> 	mutex_lock(&dev_priv->drm.struct_mutex);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v9 6/8] drm/i915/guc : Introducing enable_guc module parameter

[Michal Wajdeczko]

On Sat, 11 Nov 2017 01:06:36 +0100, Sujaritha Sundaresan  
<sujaritha.sundaresan@intel.com> wrote:

> Replacing enable_guc_submission with enable_guc modparam.
> In effect enable_guc is replacing enable_guc_loading and
> enable_guc_submission.

Maybe it will be better if we replace guc_loading and guc_submission
modparams with single guc_enable in one patch, as now you are touching
almost the same places again...

>
> Suggested by : Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  2 +-
>  drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
>  drivers/gpu/drm/i915/i915_irq.c         |  2 +-
>  drivers/gpu/drm/i915/i915_params.c      |  7 ++++---
>  drivers/gpu/drm/i915/i915_params.h      |  2 +-
>  drivers/gpu/drm/i915/intel_guc.c        |  2 +-
>  drivers/gpu/drm/i915/intel_uc.c         | 35  
> +++++++++++++++++----------------
>  7 files changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h  
> b/drivers/gpu/drm/i915/i915_drv.h
> index 798fa8a..ad73cee 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3225,7 +3225,7 @@ static inline unsigned int  
> i915_sg_segment_size(void)
>  #define HAS_HUC_UCODE(dev_priv)	((dev_priv)->huc.fw.path != NULL)
> #define NEEDS_GUC_FW(dev_priv) \
> -		(HAS_GUC(dev_priv) && i915_modparams.enable_guc_submission)
> +		(HAS_GUC(dev_priv) && i915_modparams.enable_guc)
> #define HAS_RESOURCE_STREAMER(dev_priv)  
> ((dev_priv)->info.has_resource_streamer)
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c  
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 6a819c0..43210df 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -409,7 +409,7 @@ struct i915_gem_context *
>  	i915_gem_context_set_closed(ctx); /* not user accessible */
>  	i915_gem_context_clear_bannable(ctx);
>  	i915_gem_context_set_force_single_submission(ctx);
> -	if (!i915_modparams.enable_guc_submission)
> +	if (!i915_modparams.enable_guc)
>  		ctx->ring_size = 512 * PAGE_SIZE; /* Max ring buffer size */
> 	GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
> diff --git a/drivers/gpu/drm/i915/i915_irq.c  
> b/drivers/gpu/drm/i915/i915_irq.c
> index a414bca..693b345 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1400,7 +1400,7 @@ static void snb_gt_irq_handler(struct  
> drm_i915_private *dev_priv,
> 	if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) {
>  		notify_ring(engine);
> -		tasklet |= i915_modparams.enable_guc_submission;
> +		tasklet |= i915_modparams.enable_guc;
>  	}
> 	if (tasklet)
> diff --git a/drivers/gpu/drm/i915/i915_params.c  
> b/drivers/gpu/drm/i915/i915_params.c
> index 1c25f45..51cf6bd 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -162,9 +162,10 @@ struct i915_params i915_modparams __read_mostly = {
>  	"(0=use value from vbt [default], 1=low power swing(200mV),"
>  	"2=default swing(400mV))");
> -i915_param_named_unsafe(enable_guc_submission, int, 0400,
> -	"Enable GuC submission "
> -	"(-1=auto, 0=never [default], 1=if available, 2=required)");
> +i915_param_named_unsafe(enable_guc, int, 0400,
> +	"Enable GuC submission and loading "
> +	"(-1=auto [default], 0=No GuC or HuC, 1=Load & use GuC, HuC on the  
> side"
> +	" 2=Load GuC only for HuC)");

What about direct description:

"Enable GuC"
-1=auto [default]
  0=disable GuC loading
  1=enable GuC submission
  2=enable HuC
  3=enable GuC submission and HuC

> i915_param_named(guc_log_level, int, 0400,
>  	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
> diff --git a/drivers/gpu/drm/i915/i915_params.h  
> b/drivers/gpu/drm/i915/i915_params.h
> index 9e1e231..7bf4dce 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -44,7 +44,7 @@
>  	param(int, disable_power_well, -1) \
>  	param(int, enable_ips, 1) \
>  	param(int, invert_brightness, 0) \
> -	param(int, enable_guc_submission, 0) \
> +	param(int, enable_guc, -1) \
>  	param(int, guc_log_level, -1) \
>  	param(char *, guc_firmware_path, NULL) \
>  	param(char *, huc_firmware_path, NULL) \
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index 823d0c2..629ef5d 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -128,7 +128,7 @@ void intel_guc_init_params(struct intel_guc *guc)
>  	}
> 	/* If GuC submission is enabled, set up additional parameters here */
> -	if (i915_modparams.enable_guc_submission) {
> +	if (i915_modparams.enable_guc) {
>  		u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
>  		u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool);
>  		u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index 320165a..980de7a 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -51,33 +51,34 @@ void intel_uc_sanitize_options(struct  
> drm_i915_private *dev_priv)
>  {
>  	/* Verify Hardware version */
>  	if (!HAS_GUC(dev_priv)) {
> -		if (i915_modparams.enable_guc_submission > 0)
> +		if (i915_modparams.enable_guc > 0)
> -			DRM_INFO("Ignoring option %s - no hardware",  
> "enable_guc_submission");
> +			DRM_INFO("Ignoring option %s - no hardware", "enable_guc");
> -		i915_modparams.enable_guc_submission = 0;
> +		i915_modparams.enable_guc = 0;
>  		return;
>  	}
> 	/* Verify Firmware version */
>  	if (!HAS_HUC_UCODE(dev_priv)) {
> -		if (i915_modparams.enable_guc_submission > 0) {
> +		if (i915_modparams.enable_guc > 0) {
> -			DRM_INFO("Ignoring option %s - no firmware",  
> "enable_guc_submission");
> +			DRM_INFO("Ignoring option %s - no firmware", "enable_guc");
> -			i915_modparams.enable_guc_submission = 0;
> +			i915_modparams.enable_guc = 0;
>  		return;
>  		}
>  	
> -		if (i915_modparams.enable_guc_submission < 0) {
> -			i915_modparams.enable_guc_submission = 0;
> -		return;
> +		if (i915_modparams.enable_guc < 0) {
> +			i915_modparams.enable_guc = 0;
> +			return;
>  		}
>  	}
> +
> 	/*
>  	 * A negative value means "use platform default" (enabled if we have
>  	 * survived to get here)
>  	 */
> -	if (i915_modparams.enable_guc_submission == 1)
> -		i915_modparams.enable_guc_submission = HAS_GUC(dev_priv);
> +	if (i915_modparams.enable_guc < 0)
> +		i915_modparams.enable_guc = 1;
>  }
> void intel_uc_init_early(struct drm_i915_private *dev_priv)
> @@ -169,7 +170,7 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	/* We need to notify the guc whenever we change the GGTT */
>  	i915_ggtt_enable_guc(dev_priv);
> -	if (i915_modparams.enable_guc_submission) {
> +	if (i915_modparams.enable_guc) {
>  		/*
>  		 * This is stuff we need to have available at fw load time
>  		 * if we are planning to enable submission later
> @@ -219,7 +220,7 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  		goto err_log_capture;
> 	intel_huc_auth(&dev_priv->huc);
> -	if (i915_modparams.enable_guc_submission) {
> +	if (i915_modparams.enable_guc) {
>  		if (i915_modparams.guc_log_level >= 0)
>  			gen9_enable_guc_interrupts(dev_priv);
> @@ -229,7 +230,7 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	}
> 	dev_info(dev_priv->drm.dev, "GuC %s (firmware %s [version %u.%u])\n",
> -		 i915_modparams.enable_guc_submission ? "submission enabled" :
> +		 i915_modparams.enable_guc ? "submission enabled" :
>  							"loaded",
>  		 guc->fw.path,
>  		 guc->fw.major_ver_found, guc->fw.minor_ver_found);
> @@ -251,15 +252,15 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  err_log_capture:
>  	guc_capture_load_err_log(guc);
>  err_submission:
> -	if (i915_modparams.enable_guc_submission)
> +	if (i915_modparams.enable_guc)
>  		intel_guc_submission_fini(guc);
>  err_guc:
>  	i915_ggtt_disable_guc(dev_priv);
> -	if (i915_modparams.enable_guc_submission > 1) {
> +	if (i915_modparams.enable_guc > 1) {
>  		DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
>  		ret = -EIO;
> -	} else if (i915_modparams.enable_guc_submission == 1) {
> +	} else if (i915_modparams.enable_guc == 1) {
>  		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
>  		ret = 0;
>  	} else {
> @@ -276,12 +277,12 @@ void intel_uc_fini_hw(struct drm_i915_private  
> *dev_priv)
>  	if (!NEEDS_GUC_FW(dev_priv))
>  		return;
> -	if (i915_modparams.enable_guc_submission)
> +	if (i915_modparams.enable_guc)
>  		intel_guc_submission_disable(&dev_priv->guc);
> 	guc_disable_communication(&dev_priv->guc);
> -	if (i915_modparams.enable_guc_submission) {
> +	if (i915_modparams.enable_guc) {
>  		gen9_disable_guc_interrupts(dev_priv);
>  		intel_guc_submission_fini(&dev_priv->guc);
>  	}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v9 7/8] drm/i915/guc : Decouple logs and ADS from submission

[Michal Wajdeczko]

On Sat, 11 Nov 2017 01:06:37 +0100, 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
>
> v8: Applying review comments (Michal)
>
> v9: Defining intel_guc_init function (Sagar)
>     Applying review comments (Michal, Sagar)
>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile               |   1 +
>  drivers/gpu/drm/i915/intel_guc.c            |  63 ++++++++++
>  drivers/gpu/drm/i915/intel_guc.h            |   2 +
>  drivers/gpu/drm/i915/intel_guc_ads.c        | 147 ++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_guc_ads.h        |  33 +++++
>  drivers/gpu/drm/i915/intel_guc_submission.c | 189  
> +---------------------------
>  drivers/gpu/drm/i915/intel_guc_submission.h |   9 +-
>  drivers/gpu/drm/i915/intel_uc.c             |  16 +--
>  8 files changed, 261 insertions(+), 199 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 9469c37..694e7ca 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -82,6 +82,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_fw.o \
>  	  intel_guc_log.o \
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index 629ef5d..c688586 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -23,6 +23,7 @@
>   */
> #include "intel_guc.h"
> +#include "intel_guc_ads.h"
>  #include "intel_guc_submission.h"
>  #include "i915_drv.h"
> @@ -224,6 +225,68 @@ int intel_guc_send_mmio(struct intel_guc *guc,  
> const u32 *action, u32 len)
>  	return ret;
>  }
> +/*
> + * Set up the memory resources to be shared with the GuC (via the GGTT)
> + * at firmware loading time.
> + */
> +
> +int intel_guc_init(struct intel_guc *guc)
> +{
> +	int ret;
> +
> +	if (guc->stage_desc_pool)
> +		return 0;
> +
> +	ret = guc_stage_desc_pool_create(guc);
> +	if (ret)
> +		return ret;
> +	/*
> +	 * Keep static analysers happy, let them know that we allocated the
> +	 * vma after testing that it didn't exist earlier.
> +	 */
> +	GEM_BUG_ON(!guc->stage_desc_pool);
> +
> +	ret = guc_shared_data_create(guc);
> +	if (ret)
> +		goto err_stage_desc_pool;
> +	GEM_BUG_ON(!guc->shared_data);
> +
> +	ret = intel_guc_log_create(guc);
> +	if (ret < 0)
> +		goto err_shared_data;
> +
> +	ret = guc_preempt_work_create(guc);
> +	if (ret)
> +		goto err_log;
> +	GEM_BUG_ON(!guc->preempt_wq);
> +
> +	ret = intel_guc_ads_create(guc);
> +	if (ret < 0)
> +		goto err_wq;
> +	GEM_BUG_ON(!guc->ads_vma);
> +
> +	return 0;
> +
> +err_wq:
> +	guc_preempt_work_destroy(guc);
> +err_log:
> +	intel_guc_log_destroy(guc);
> +err_shared_data:
> +	guc_shared_data_destroy(guc);
> +err_stage_desc_pool:
> +	guc_stage_desc_pool_destroy(guc);
> +	return ret;
> +}
> +
> +void intel_guc_fini(struct intel_guc *guc)
> +{
> +	intel_guc_ads_destroy(guc);
> +	guc_preempt_work_destroy(guc);
> +	intel_guc_log_destroy(guc);
> +	guc_shared_data_destroy(guc);
> +	guc_stage_desc_pool_destroy(guc);
> +}
> +
>  int intel_guc_sample_forcewake(struct intel_guc *guc)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> diff --git a/drivers/gpu/drm/i915/intel_guc.h  
> b/drivers/gpu/drm/i915/intel_guc.h
> index 75c4cfe..a805c79 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -121,6 +121,8 @@ static inline u32 guc_ggtt_offset(struct i915_vma  
> *vma)
>  void intel_guc_init_params(struct intel_guc *guc);
>  int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32  
> len);
>  int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32  
> len);
> +int intel_guc_init(struct intel_guc *guc);
> +void intel_guc_fini(struct intel_guc *guc);
>  int intel_guc_sample_forcewake(struct intel_guc *guc);
>  int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
>  int intel_guc_suspend(struct drm_i915_private *dev_priv);
> 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..c97704d
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_guc_ads.c
> @@ -0,0 +1,147 @@
> +/*
> + * Copyright © 2014-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_guc_ads.h"
> +#include "intel_uc.h"
> +#include "i915_drv.h"
> +#include "intel_guc_submission.h"
> +
> +/**
> + * DOC: GuC Additional Data Struct
> + * 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 void guc_policy_init(struct guc_policy *policy)
> +{
> +	policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
> +	policy->preemption_time = POLICY_DEFAULT_PREEMPTION_TIME_US;
> +	policy->fault_time = POLICY_DEFAULT_FAULT_TIME_US;
> +	policy->policy_flags = 0;
> +}
> +
> +static void guc_policies_init(struct guc_policies *policies)
> +{
> +	struct guc_policy *policy;
> +	u32 p, i;
> +
> +	policies->dpc_promote_time = POLICY_DEFAULT_DPC_PROMOTE_TIME_US;
> +	policies->max_num_work_items = POLICY_MAX_NUM_WI;
> +
> +	for (p = 0; p < GUC_CLIENT_PRIORITY_NUM; p++) {
> +		for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
> +			policy = &policies->policy[p][i];
> +
> +			guc_policy_init(policy);
> +		}
> +	}
> +
> +	policies->is_valid = 1;
> +}
> +
> +/*
> + * The first 80 dwords of the register state context, containing the
> + * execlists and ppgtt registers.
> + */
> +#define LR_HW_CONTEXT_SIZE	(80 * sizeof(u32))
> +
> +int intel_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;
> +}
> +
> +void intel_guc_ads_destroy(struct intel_guc *guc)
> +{
> +	i915_vma_unpin_and_release(&guc->ads_vma);
> +}
> +
> \ No newline at end of file
> 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..cf5821a
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_guc_ads.h
> @@ -0,0 +1,33 @@
> +/*
> + * 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_
> +
> +struct intel_guc;
> +
> +int intel_guc_ads_create(struct intel_guc *guc);
> +void intel_guc_ads_destroy(struct intel_guc *guc);
> +
> +#endif
> +
> \ No newline at end of file
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c  
> b/drivers/gpu/drm/i915/intel_guc_submission.c
> index cca56cc..ce7d96f1 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -73,13 +73,6 @@
>   * ELSP context descriptor dword into Work Item.
>   * See guc_add_request()
>   *
> - * 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 intel_guc_client* client)
> @@ -316,7 +309,7 @@ static void guc_proc_desc_init(struct intel_guc *guc,
>  	desc->priority = client->priority;
>  }
> -static int guc_stage_desc_pool_create(struct intel_guc *guc)
> +int guc_stage_desc_pool_create(struct intel_guc *guc)
>  {
>  	struct i915_vma *vma;
>  	void *vaddr;
> @@ -340,7 +333,7 @@ static int guc_stage_desc_pool_create(struct  
> intel_guc *guc)
>  	return 0;
>  }
> -static void guc_stage_desc_pool_destroy(struct intel_guc *guc)
> +void guc_stage_desc_pool_destroy(struct intel_guc *guc)
>  {
>  	ida_destroy(&guc->stage_ids);
>  	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
> @@ -444,7 +437,7 @@ static void guc_stage_desc_fini(struct intel_guc  
> *guc,
>  	memset(desc, 0, sizeof(*desc));
>  }
> -static int guc_shared_data_create(struct intel_guc *guc)
> +int guc_shared_data_create(struct intel_guc *guc)
>  {
>  	struct i915_vma *vma;
>  	void *vaddr;
> @@ -465,7 +458,7 @@ static int guc_shared_data_create(struct intel_guc  
> *guc)
>  	return 0;
>  }
> -static void guc_shared_data_destroy(struct intel_guc *guc)
> +void guc_shared_data_destroy(struct intel_guc *guc)
>  {
>  	i915_gem_object_unpin_map(guc->shared_data->obj);
>  	i915_vma_unpin_and_release(&guc->shared_data);
> @@ -1095,116 +1088,7 @@ static void guc_clients_destroy(struct intel_guc  
> *guc)
>  	guc_client_free(client);
>  }
> -static void guc_policy_init(struct guc_policy *policy)
> -{
> -	policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
> -	policy->preemption_time = POLICY_DEFAULT_PREEMPTION_TIME_US;
> -	policy->fault_time = POLICY_DEFAULT_FAULT_TIME_US;
> -	policy->policy_flags = 0;
> -}
> -
> -static void guc_policies_init(struct guc_policies *policies)
> -{
> -	struct guc_policy *policy;
> -	u32 p, i;
> -
> -	policies->dpc_promote_time = POLICY_DEFAULT_DPC_PROMOTE_TIME_US;
> -	policies->max_num_work_items = POLICY_MAX_NUM_WI;
> -
> -	for (p = 0; p < GUC_CLIENT_PRIORITY_NUM; p++) {
> -		for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
> -			policy = &policies->policy[p][i];
> -
> -			guc_policy_init(policy);
> -		}
> -	}
> -
> -	policies->is_valid = 1;
> -}
> -
> -/*
> - * 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);
> -}
> -
> -static int guc_preempt_work_create(struct intel_guc *guc)
> +int guc_preempt_work_create(struct intel_guc *guc)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>  	struct intel_engine_cs *engine;
> @@ -1236,7 +1120,7 @@ static int guc_preempt_work_create(struct  
> intel_guc *guc)
>  	return 0;
>  }
> -static void guc_preempt_work_destroy(struct intel_guc *guc)
> +void guc_preempt_work_destroy(struct intel_guc *guc)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>  	struct intel_engine_cs *engine;
> @@ -1249,67 +1133,6 @@ static void guc_preempt_work_destroy(struct  
> intel_guc *guc)
>  	guc->preempt_wq = NULL;
>  }
> -/*
> - * Set up the memory resources to be shared with the GuC (via the GGTT)
> - * at firmware loading time.
> - */
> -int intel_guc_submission_init(struct intel_guc *guc)
> -{
> -	int ret;
> -
> -	if (guc->stage_desc_pool)
> -		return 0;
> -
> -	ret = guc_stage_desc_pool_create(guc);
> -	if (ret)
> -		return ret;
> -	/*
> -	 * Keep static analysers happy, let them know that we allocated the
> -	 * vma after testing that it didn't exist earlier.
> -	 */
> -	GEM_BUG_ON(!guc->stage_desc_pool);
> -
> -	ret = guc_shared_data_create(guc);
> -	if (ret)
> -		goto err_stage_desc_pool;
> -	GEM_BUG_ON(!guc->shared_data);
> -
> -	ret = intel_guc_log_create(guc);
> -	if (ret < 0)
> -		goto err_shared_data;
> -
> -	ret = guc_preempt_work_create(guc);
> -	if (ret)
> -		goto err_log;
> -	GEM_BUG_ON(!guc->preempt_wq);
> -
> -	ret = guc_ads_create(guc);
> -	if (ret < 0)
> -		goto err_wq;
> -	GEM_BUG_ON(!guc->ads_vma);
> -
> -	return 0;
> -
> -err_wq:
> -	guc_preempt_work_destroy(guc);
> -err_log:
> -	intel_guc_log_destroy(guc);
> -err_shared_data:
> -	guc_shared_data_destroy(guc);
> -err_stage_desc_pool:
> -	guc_stage_desc_pool_destroy(guc);
> -	return ret;
> -}
> -
> -void intel_guc_submission_fini(struct intel_guc *guc)
> -{
> -	guc_ads_destroy(guc);
> -	guc_preempt_work_destroy(guc);
> -	intel_guc_log_destroy(guc);
> -	guc_shared_data_destroy(guc);
> -	guc_stage_desc_pool_destroy(guc);
> -}
> -
>  static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_rps *rps = &dev_priv->gt_pm.rps;
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.h  
> b/drivers/gpu/drm/i915/intel_guc_submission.h
> index 3e46916..15af6ef 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.h
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.h
> @@ -72,9 +72,14 @@ struct intel_guc_client {
>  	u64 submissions[I915_NUM_ENGINES];
>  };
> -int intel_guc_submission_init(struct intel_guc *guc);
>  int intel_guc_submission_enable(struct intel_guc *guc);
>  void intel_guc_submission_disable(struct intel_guc *guc);
> -void intel_guc_submission_fini(struct intel_guc *guc);
> +
> +int guc_stage_desc_pool_create(struct intel_guc *guc);
> +int guc_shared_data_create(struct intel_guc *guc);
> +int guc_preempt_work_create(struct intel_guc *guc);
> +void guc_stage_desc_pool_destroy(struct intel_guc *guc);
> +void guc_shared_data_destroy(struct intel_guc *guc);
> +void guc_preempt_work_destroy(struct intel_guc *guc);

Do we really need to export all these functions ?
Maybe all you need is to split guc_submission_init/fini
and move only unrelated code to guc_init/fini

Also note that files exported by this file shall start
with "intel_guc_submission_" prefix

> #endif
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index 980de7a..0e2c4a6 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -170,16 +170,6 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	/* We need to notify the guc whenever we change the GGTT */
>  	i915_ggtt_enable_guc(dev_priv);
> -	if (i915_modparams.enable_guc) {
> -		/*
> -		 * This is stuff we need to have available at fw load time
> -		 * if we are planning to enable submission later
> -		 */
> -		ret = intel_guc_submission_init(guc);
> -		if (ret)
> -			goto err_guc;
> -	}
> -

Is it ok to just remove this code now?
This may render driver unusable until next patch, where this code
will be reintroduced again...

>  	/* init WOPCM */
>  	I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(dev_priv));
>  	I915_WRITE(DMA_GUC_WOPCM_OFFSET,
> @@ -253,9 +243,7 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	guc_capture_load_err_log(guc);
>  err_submission:
>  	if (i915_modparams.enable_guc)
> -		intel_guc_submission_fini(guc);
> -err_guc:
> -	i915_ggtt_disable_guc(dev_priv);
> +		intel_guc_fini(guc);
> 	if (i915_modparams.enable_guc > 1) {
>  		DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
> @@ -284,7 +272,7 @@ void intel_uc_fini_hw(struct drm_i915_private  
> *dev_priv)
> 	if (i915_modparams.enable_guc) {
>  		gen9_disable_guc_interrupts(dev_priv);
> -		intel_guc_submission_fini(&dev_priv->guc);
> +		intel_guc_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 v9 8/8] drm/i915/guc : Calling intel_guc_init in i915_gem_init

[Michal Wajdeczko]

On Sat, 11 Nov 2017 01:06:38 +0100, Sujaritha Sundaresan  
<sujaritha.sundaresan@intel.com> wrote:

> Placing the call to intel_guc_init after i915_gem_contexts_init,
> based on the dependency within i915_gem_init.
>
> Will move the function if required, depending on the review
> comments.
>
> Suggested by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c  
> b/drivers/gpu/drm/i915/i915_gem.c
> index 889ae88..c877a5d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -36,6 +36,7 @@
>  #include "intel_frontbuffer.h"
>  #include "intel_mocs.h"
>  #include "i915_gemfs.h"
> +#include "intel_guc.h"
>  #include <linux/dma-fence-array.h>
>  #include <linux/kthread.h>
>  #include <linux/reservation.h>
> @@ -4972,6 +4973,7 @@ bool intel_sanitize_semaphores(struct  
> drm_i915_private *dev_priv, int value)
> int i915_gem_init(struct drm_i915_private *dev_priv)
>  {
> +	struct intel_guc *guc = &dev_priv->guc;
>  	int ret;
> 	mutex_lock(&dev_priv->drm.struct_mutex);
> @@ -5015,6 +5017,18 @@ int i915_gem_init(struct drm_i915_private  
> *dev_priv)
>  	if (ret)
>  		goto out_unlock;
> +	ret = intel_guc_init(guc);
> +
> +		if (i915_modparams.enable_guc) {
> +			/*
> +			 * This is stuff we need to have available at fw load time
> +			 * if we are planning to enable submission later
> +			 */
> +			ret = intel_guc_init(guc);
> +			if (ret)
> +				goto err_shared;
> +		}
> +

Why there are two calls to guc_init ?

Also note that this approach breaks previous idea to hide GuC/HuC internals
to the rest of the driver by exposing only high level "uc" functions.

>  	ret = intel_engines_init(dev_priv);
>  	if (ret)
>  		goto out_unlock;
> @@ -5035,7 +5049,8 @@ int i915_gem_init(struct drm_i915_private  
> *dev_priv)
>  out_unlock:
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
> -
> +err_shared:
> +	intel_guc_fini(guc);
>  	return ret;
>  }
> @@ -5192,6 +5207,7 @@ void i915_gem_load_cleanup(struct drm_i915_private  
> *dev_priv)
>  	rcu_barrier();
> 	i915_gemfs_fini(dev_priv);
> +	intel_guc_fini(&dev_priv->guc);
>  }
> int i915_gem_freeze(struct drm_i915_private *dev_priv)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v9 8/8] drm/i915/guc : Calling intel_guc_init in i915_gem_init

[Sujaritha]

On 11/12/2017 09:22 AM, Michal Wajdeczko wrote:
> On Sat, 11 Nov 2017 01:06:38 +0100, Sujaritha Sundaresan 
> <sujaritha.sundaresan@intel.com> wrote:
>
>> Placing the call to intel_guc_init after i915_gem_contexts_init,
>> based on the dependency within i915_gem_init.
>>
>> Will move the function if required, depending on the review
>> comments.
>>
>> Suggested by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_gem.c | 18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index 889ae88..c877a5d 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -36,6 +36,7 @@
>>  #include "intel_frontbuffer.h"
>>  #include "intel_mocs.h"
>>  #include "i915_gemfs.h"
>> +#include "intel_guc.h"
>>  #include <linux/dma-fence-array.h>
>>  #include <linux/kthread.h>
>>  #include <linux/reservation.h>
>> @@ -4972,6 +4973,7 @@ bool intel_sanitize_semaphores(struct 
>> drm_i915_private *dev_priv, int value)
>> int i915_gem_init(struct drm_i915_private *dev_priv)
>>  {
>> +    struct intel_guc *guc = &dev_priv->guc;
>>      int ret;
>>     mutex_lock(&dev_priv->drm.struct_mutex);
>> @@ -5015,6 +5017,18 @@ int i915_gem_init(struct drm_i915_private 
>> *dev_priv)
>>      if (ret)
>>          goto out_unlock;
>> +    ret = intel_guc_init(guc);
>> +
>> +        if (i915_modparams.enable_guc) {
>> +            /*
>> +             * This is stuff we need to have available at fw load time
>> +             * if we are planning to enable submission later
>> +             */
>> +            ret = intel_guc_init(guc);
>> +            if (ret)
>> +                goto err_shared;
>> +        }
>> +
>
> Why there are two calls to guc_init ?
>
> Also note that this approach breaks previous idea to hide GuC/HuC 
> internals
> to the rest of the driver by exposing only high level "uc" functions.
>

I will call intel_guc_init (dev_priv) inside intel_uc_init(dev_priv). I 
will send the corrected patch asap.
I'm also considering splitting the series. I will split the module 
parameter changes and the decoupling
changes into two series.
>>      ret = intel_engines_init(dev_priv);
>>      if (ret)
>>          goto out_unlock;
>> @@ -5035,7 +5049,8 @@ int i915_gem_init(struct drm_i915_private 
>> *dev_priv)
>>  out_unlock:
>>      intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>>      mutex_unlock(&dev_priv->drm.struct_mutex);
>> -
>> +err_shared:
>> +    intel_guc_fini(guc);
>>      return ret;
>>  }
>> @@ -5192,6 +5207,7 @@ void i915_gem_load_cleanup(struct 
>> drm_i915_private *dev_priv)
>>      rcu_barrier();
>>     i915_gemfs_fini(dev_priv);
>> +    intel_guc_fini(&dev_priv->guc);
>>  }
>> int i915_gem_freeze(struct drm_i915_private *dev_priv)

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

Re: [PATCH v9 1/8] drm/i915 : Unifying seq_puts messages for feature support

[Sujaritha]

On 11/12/2017 08:18 AM, Michal Wajdeczko wrote:
> On Sat, 11 Nov 2017 01:06:31 +0100, 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)
>>
>> v8: Omitting DRRS seq_puts unification (Michal)
>>
>> v9: Including the HAS_HUC condition (Michal)
>>     Updating more functions with unified message (Sagar)
>>
>> Suggested by : Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c | 53 
>> +++++++++++++++++++++++++------------
>>  1 file changed, 36 insertions(+), 17 deletions(-)
>>
>
> <snip>
>
>> @@ -2361,8 +2361,10 @@ static int i915_huc_load_status_info(struct 
>> seq_file *m, void *data)
>>      struct drm_i915_private *dev_priv = node_to_i915(m->private);
>>      struct drm_printer p;
>> -    if (!HAS_HUC_UCODE(dev_priv))
>> +    if (!HAS_HUC(dev_priv)) {
>
> Hmm, HAS_HUC is not available yet, it will be introduced by patch 2/8
>
> <snip>
>
>> @@ -2461,9 +2465,11 @@ 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) ?
>> -               "disabled" :
>> -               "not supported");
>> +                HAS_GUC(dev_priv) ?
>> +                "not supported" :
>> +                NEEDS_GUC_FW(dev_priv) ?
>
> NEEDS_GUC_FW will also be introduced by patch 2/8
>
> Michal

I will not make the macro changes in this patch. I will also move this 
patch out of the series,
as Sagar had suggested in a review to a previous version, since there 
will be no dependence
on any of the other changes.

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 v9 3/8] drm/i915/guc : Updating GuC and HuC firmware select function

[Sujaritha]

On 11/12/2017 08:25 AM, Michal Wajdeczko wrote:
> On Sat, 11 Nov 2017 01:06:33 +0100, Sujaritha Sundaresan 
> <sujaritha.sundaresan@intel.com> 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
>>
>> v8: Including change to intel_uc.c
>>     Applying review comments (Michal)
>>
>> v9: Including HAS_HUC macro
>>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_guc_fw.c | 6 ++----
>>  drivers/gpu/drm/i915/intel_guc_fw.h | 2 +-
>>  drivers/gpu/drm/i915/intel_huc.c    | 3 ++-
>>  drivers/gpu/drm/i915/intel_uc.c     | 7 +++++++
>>  4 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c 
>> b/drivers/gpu/drm/i915/intel_guc_fw.c
>> index 69ba015..112c89d 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);
>> @@ -91,10 +91,8 @@ int intel_guc_fw_select(struct intel_guc *guc)
>>          guc->fw.minor_ver_wanted = GLK_FW_MINOR;
>>      } else {
>>          DRM_ERROR("No GuC firmware known for platform with GuC!\n");
>> -        return -ENOENT;
>> +        return;
>>      }
>> -
>> -    return 0;
>>  }
>> static void guc_prepare_xfer(struct intel_guc *guc)
>> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.h 
>> b/drivers/gpu/drm/i915/intel_guc_fw.h
>> index 023f5ba..7f6ccaf 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_fw.h
>> +++ b/drivers/gpu/drm/i915/intel_guc_fw.h
>> @@ -27,7 +27,7 @@
>> struct intel_guc;
>> -int intel_guc_fw_select(struct intel_guc *guc);
>> +void intel_guc_fw_select(struct intel_guc *guc);
>>  int intel_guc_fw_upload(struct intel_guc *guc);
>> #endif
>> diff --git a/drivers/gpu/drm/i915/intel_huc.c 
>> b/drivers/gpu/drm/i915/intel_huc.c
>> index 98d1725..7f4bbc1 100644
>> --- a/drivers/gpu/drm/i915/intel_huc.c
>> +++ b/drivers/gpu/drm/i915/intel_huc.c
>> @@ -108,7 +108,8 @@ 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");
>> +        if (HAS_HUC(dev_priv))
>
> Can we move this check to the caller ? see intel_uc_init_fw
> And what about GuC ?
>
Will do. Similar changes to guc as well.
>> +            DRM_ERROR("No HuC firmware known for platform with 
>> HuC!\n");
>>          return;
>>      }
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 648e59c..320165a 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -82,11 +82,18 @@ void intel_uc_sanitize_options(struct 
>> drm_i915_private *dev_priv)
>> void intel_uc_init_early(struct drm_i915_private *dev_priv)
>>  {
>> +    struct intel_guc *guc = &dev_priv->guc;
>> +    struct intel_huc *huc = &dev_priv->huc;
>> +
>>      intel_guc_init_early(&dev_priv->guc);
>
> Btw, now you can pass 'guc' as param
>

Will do.
>> +    intel_guc_fw_select(guc);
>> +    intel_huc_select_fw(huc);
>>  }
>> void intel_uc_init_fw(struct drm_i915_private *dev_priv)
>>  {
>> +    if (!HAS_GUC(dev_priv))
>> +        return;
>>      intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw);
>>      intel_uc_fw_fetch(dev_priv, &dev_priv->guc.fw);
>>  }

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 v9 4/8] drm/i915/guc : Updating GuC logs to remove enable_guc_submission parameter

[Sujaritha]

On 11/12/2017 08:29 AM, Michal Wajdeczko wrote:
> On Sat, 11 Nov 2017 01:06:34 +0100, Sujaritha Sundaresan 
> <sujaritha.sundaresan@intel.com> wrote:
>
>> Replacing conditions to remove dependance on enable_guc_submission
>
> typo ;)
>

Oops. :)
>>
>> v9: Including guc_log_level in the condition (Sagar)
>>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_guc_log.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index 76d3eb1..4dbe5be 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_FW(dev_priv) ||
>
> Hmm, maybe in all these places we should rather check GuC firmware load
> status directly? We don't care here why it was loaded, we just want to
> verify that it is available.

Will do.
>
>>          (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_FW(dev_priv) ||
>>          (i915_modparams.guc_log_level < 0))
>>          return;
>> @@ -657,7 +657,7 @@ void i915_guc_log_register(struct 
>> drm_i915_private *dev_priv)
>> void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>>  {
>> -    if (!i915_modparams.enable_guc_submission)
>> +    if (!NEEDS_GUC_FW(dev_priv))
>>          return;
>>     mutex_lock(&dev_priv->drm.struct_mutex);

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 v9 6/8] drm/i915/guc : Introducing enable_guc module parameter

[Sujaritha]

On 11/12/2017 08:52 AM, Michal Wajdeczko wrote:
> On Sat, 11 Nov 2017 01:06:36 +0100, Sujaritha Sundaresan 
> <sujaritha.sundaresan@intel.com> wrote:
>
>> Replacing enable_guc_submission with enable_guc modparam.
>> In effect enable_guc is replacing enable_guc_loading and
>> enable_guc_submission.
>
> Maybe it will be better if we replace guc_loading and guc_submission
> modparams with single guc_enable in one patch, as now you are touching
> almost the same places again...
>
I will merge this with removing enable_guc_loading :)
>>
>> Suggested by : Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h         |  2 +-
>>  drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
>>  drivers/gpu/drm/i915/i915_irq.c         |  2 +-
>>  drivers/gpu/drm/i915/i915_params.c      |  7 ++++---
>>  drivers/gpu/drm/i915/i915_params.h      |  2 +-
>>  drivers/gpu/drm/i915/intel_guc.c        |  2 +-
>>  drivers/gpu/drm/i915/intel_uc.c         | 35 
>> +++++++++++++++++----------------
>>  7 files changed, 27 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 798fa8a..ad73cee 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3225,7 +3225,7 @@ static inline unsigned int 
>> i915_sg_segment_size(void)
>>  #define HAS_HUC_UCODE(dev_priv)    ((dev_priv)->huc.fw.path != NULL)
>> #define NEEDS_GUC_FW(dev_priv) \
>> -        (HAS_GUC(dev_priv) && i915_modparams.enable_guc_submission)
>> +        (HAS_GUC(dev_priv) && i915_modparams.enable_guc)
>> #define HAS_RESOURCE_STREAMER(dev_priv) 
>> ((dev_priv)->info.has_resource_streamer)
>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
>> b/drivers/gpu/drm/i915/i915_gem_context.c
>> index 6a819c0..43210df 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>> @@ -409,7 +409,7 @@ struct i915_gem_context *
>>      i915_gem_context_set_closed(ctx); /* not user accessible */
>>      i915_gem_context_clear_bannable(ctx);
>>      i915_gem_context_set_force_single_submission(ctx);
>> -    if (!i915_modparams.enable_guc_submission)
>> +    if (!i915_modparams.enable_guc)
>>          ctx->ring_size = 512 * PAGE_SIZE; /* Max ring buffer size */
>>     GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c 
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index a414bca..693b345 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -1400,7 +1400,7 @@ static void snb_gt_irq_handler(struct 
>> drm_i915_private *dev_priv,
>>     if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) {
>>          notify_ring(engine);
>> -        tasklet |= i915_modparams.enable_guc_submission;
>> +        tasklet |= i915_modparams.enable_guc;
>>      }
>>     if (tasklet)
>> diff --git a/drivers/gpu/drm/i915/i915_params.c 
>> b/drivers/gpu/drm/i915/i915_params.c
>> index 1c25f45..51cf6bd 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -162,9 +162,10 @@ struct i915_params i915_modparams __read_mostly = {
>>      "(0=use value from vbt [default], 1=low power swing(200mV),"
>>      "2=default swing(400mV))");
>> -i915_param_named_unsafe(enable_guc_submission, int, 0400,
>> -    "Enable GuC submission "
>> -    "(-1=auto, 0=never [default], 1=if available, 2=required)");
>> +i915_param_named_unsafe(enable_guc, int, 0400,
>> +    "Enable GuC submission and loading "
>> +    "(-1=auto [default], 0=No GuC or HuC, 1=Load & use GuC, HuC on 
>> the side"
>> +    " 2=Load GuC only for HuC)");
>
> What about direct description:
>
> "Enable GuC"
> -1=auto [default]
>  0=disable GuC loading
>  1=enable GuC submission
>  2=enable HuC
>  3=enable GuC submission and HuC
>
>> i915_param_named(guc_log_level, int, 0400,
>>      "GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
>> diff --git a/drivers/gpu/drm/i915/i915_params.h 
>> b/drivers/gpu/drm/i915/i915_params.h
>> index 9e1e231..7bf4dce 100644
>> --- a/drivers/gpu/drm/i915/i915_params.h
>> +++ b/drivers/gpu/drm/i915/i915_params.h
>> @@ -44,7 +44,7 @@
>>      param(int, disable_power_well, -1) \
>>      param(int, enable_ips, 1) \
>>      param(int, invert_brightness, 0) \
>> -    param(int, enable_guc_submission, 0) \
>> +    param(int, enable_guc, -1) \
>>      param(int, guc_log_level, -1) \
>>      param(char *, guc_firmware_path, NULL) \
>>      param(char *, huc_firmware_path, NULL) \
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c 
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index 823d0c2..629ef5d 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -128,7 +128,7 @@ void intel_guc_init_params(struct intel_guc *guc)
>>      }
>>     /* If GuC submission is enabled, set up additional parameters 
>> here */
>> -    if (i915_modparams.enable_guc_submission) {
>> +    if (i915_modparams.enable_guc) {
>>          u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
>>          u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool);
>>          u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 320165a..980de7a 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -51,33 +51,34 @@ void intel_uc_sanitize_options(struct 
>> drm_i915_private *dev_priv)
>>  {
>>      /* Verify Hardware version */
>>      if (!HAS_GUC(dev_priv)) {
>> -        if (i915_modparams.enable_guc_submission > 0)
>> +        if (i915_modparams.enable_guc > 0)
>> -            DRM_INFO("Ignoring option %s - no hardware", 
>> "enable_guc_submission");
>> +            DRM_INFO("Ignoring option %s - no hardware", "enable_guc");
>> -        i915_modparams.enable_guc_submission = 0;
>> +        i915_modparams.enable_guc = 0;
>>          return;
>>      }
>>     /* Verify Firmware version */
>>      if (!HAS_HUC_UCODE(dev_priv)) {
>> -        if (i915_modparams.enable_guc_submission > 0) {
>> +        if (i915_modparams.enable_guc > 0) {
>> -            DRM_INFO("Ignoring option %s - no firmware", 
>> "enable_guc_submission");
>> +            DRM_INFO("Ignoring option %s - no firmware", "enable_guc");
>> -            i915_modparams.enable_guc_submission = 0;
>> +            i915_modparams.enable_guc = 0;
>>          return;
>>          }
>>
>> -        if (i915_modparams.enable_guc_submission < 0) {
>> -            i915_modparams.enable_guc_submission = 0;
>> -        return;
>> +        if (i915_modparams.enable_guc < 0) {
>> +            i915_modparams.enable_guc = 0;
>> +            return;
>>          }
>>      }
>> +
>>     /*
>>       * A negative value means "use platform default" (enabled if we 
>> have
>>       * survived to get here)
>>       */
>> -    if (i915_modparams.enable_guc_submission == 1)
>> -        i915_modparams.enable_guc_submission = HAS_GUC(dev_priv);
>> +    if (i915_modparams.enable_guc < 0)
>> +        i915_modparams.enable_guc = 1;
>>  }
>> void intel_uc_init_early(struct drm_i915_private *dev_priv)
>> @@ -169,7 +170,7 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>      /* We need to notify the guc whenever we change the GGTT */
>>      i915_ggtt_enable_guc(dev_priv);
>> -    if (i915_modparams.enable_guc_submission) {
>> +    if (i915_modparams.enable_guc) {
>>          /*
>>           * This is stuff we need to have available at fw load time
>>           * if we are planning to enable submission later
>> @@ -219,7 +220,7 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>          goto err_log_capture;
>>     intel_huc_auth(&dev_priv->huc);
>> -    if (i915_modparams.enable_guc_submission) {
>> +    if (i915_modparams.enable_guc) {
>>          if (i915_modparams.guc_log_level >= 0)
>>              gen9_enable_guc_interrupts(dev_priv);
>> @@ -229,7 +230,7 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>      }
>>     dev_info(dev_priv->drm.dev, "GuC %s (firmware %s [version 
>> %u.%u])\n",
>> -         i915_modparams.enable_guc_submission ? "submission enabled" :
>> +         i915_modparams.enable_guc ? "submission enabled" :
>>                              "loaded",
>>           guc->fw.path,
>>           guc->fw.major_ver_found, guc->fw.minor_ver_found);
>> @@ -251,15 +252,15 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>  err_log_capture:
>>      guc_capture_load_err_log(guc);
>>  err_submission:
>> -    if (i915_modparams.enable_guc_submission)
>> +    if (i915_modparams.enable_guc)
>>          intel_guc_submission_fini(guc);
>>  err_guc:
>>      i915_ggtt_disable_guc(dev_priv);
>> -    if (i915_modparams.enable_guc_submission > 1) {
>> +    if (i915_modparams.enable_guc > 1) {
>>          DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
>>          ret = -EIO;
>> -    } else if (i915_modparams.enable_guc_submission == 1) {
>> +    } else if (i915_modparams.enable_guc == 1) {
>>          DRM_NOTE("Falling back from GuC submission to execlist 
>> mode\n");
>>          ret = 0;
>>      } else {
>> @@ -276,12 +277,12 @@ void intel_uc_fini_hw(struct drm_i915_private 
>> *dev_priv)
>>      if (!NEEDS_GUC_FW(dev_priv))
>>          return;
>> -    if (i915_modparams.enable_guc_submission)
>> +    if (i915_modparams.enable_guc)
>>          intel_guc_submission_disable(&dev_priv->guc);
>>     guc_disable_communication(&dev_priv->guc);
>> -    if (i915_modparams.enable_guc_submission) {
>> +    if (i915_modparams.enable_guc) {
>>          gen9_disable_guc_interrupts(dev_priv);
>>          intel_guc_submission_fini(&dev_priv->guc);
>>      }

Thanks for the review.

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