[v2,6/8] drm/i915/guc: Set GuC init params only once
diff mbox series

Message ID 20190724022153.8927-7-daniele.ceraolospurio@intel.com
State New
Headers show
Series
  • uC fw path unification + misc clean-up
Related show

Commit Message

Daniele Ceraolo Spurio July 24, 2019, 2:21 a.m. UTC
All the GuC objects are perma-pinned, so their offset can't change at
runtime. We can therefore set (and log!) the parameters only once during
boot.

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc.c | 143 ++++++++++++++-----------
 drivers/gpu/drm/i915/gt/uc/intel_guc.h |   5 +-
 drivers/gpu/drm/i915/gt/uc/intel_uc.c  |   2 +-
 3 files changed, 83 insertions(+), 67 deletions(-)

Comments

Chris Wilson July 24, 2019, 8:19 a.m. UTC | #1
Quoting Daniele Ceraolo Spurio (2019-07-24 03:21:51)
> All the GuC objects are perma-pinned, so their offset can't change at
> runtime. We can therefore set (and log!) the parameters only once during
> boot.

Much better, but I still have the same information (fw loaded) repeated.

<7>[  345.432995] [drm:intel_uc_fw_upload [i915]] HuC fw load i915/kbl_huc_ver02_00_1810.bin
<7>[  345.433589] [drm:intel_uc_fw_upload [i915]] HuC fw load completed
<6>[  345.433592] [drm] HuC: Loaded firmware i915/kbl_huc_ver02_00_1810.bin (version 2.0)
<7>[  345.433686] [drm:intel_uc_fw_upload [i915]] GuC fw load i915/kbl_guc_33.0.0.bin
<7>[  345.434046] [drm:intel_uc_fw_upload [i915]] GuC fw load completed
<6>[  345.434048] [drm] GuC: Loaded firmware i915/kbl_guc_33.0.0.bin (version 33.0)
<7>[  345.436578] [drm:intel_guc_fw_upload [i915]] GuC status 0x8002f0ec
<6>[  345.436643] [drm] GuC communication enabled
<6>[  345.436718] i915 0000:00:02.0: GuC firmware version 33.0
<6>[  345.436719] i915 0000:00:02.0: GuC submission disabled
<6>[  345.436720] i915 0000:00:02.0: HuC enabled
<7>[  345.437910] [drm:intel_uc_fw_upload [i915]] HuC fw load i915/kbl_huc_ver02_00_1810.bin
<7>[  345.438591] [drm:intel_uc_fw_upload [i915]] HuC fw load completed
<6>[  345.438593] [drm] HuC: Loaded firmware i915/kbl_huc_ver02_00_1810.bin (version 2.0)
<7>[  345.438724] [drm:intel_uc_fw_upload [i915]] GuC fw load i915/kbl_guc_33.0.0.bin
<7>[  345.439085] [drm:intel_uc_fw_upload [i915]] GuC fw load completed
<6>[  345.439086] [drm] GuC: Loaded firmware i915/kbl_guc_33.0.0.bin (version 33.0)
<7>[  345.441594] [drm:intel_guc_fw_upload [i915]] GuC status 0x8002f0ec
<6>[  345.441652] [drm] GuC communication enabled
<6>[  345.441727] i915 0000:00:02.0: GuC firmware version 33.0
<6>[  345.441728] i915 0000:00:02.0: GuC submission disabled
<6>[  345.441729] i915 0000:00:02.0: HuC enabled

Also note that we are repeating at KERN_INFO the same information (fw
loaded).

Having the path before loading is reasonable (or rather I expect
reasonable object to any request to drop it until error), but the
repeated completion information could be whittled down :)

> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---

> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index f51c4c3c1d0b..714e9892aaff 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> @@ -76,6 +76,9 @@ struct intel_guc {
>         /* Cyclic counter mod pagesize  */
>         u32 db_cacheline;
>  
> +       /* Control params for fw initialization */
> +       u32 params[GUC_CTL_MAX_DWORDS];
> +

Upon init, save parameters to intel_guc. Then write from guc->params
instead of the local stack.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Chris Wilson July 24, 2019, 10:29 a.m. UTC | #2
Quoting Chris Wilson (2019-07-24 09:19:44)
> Quoting Daniele Ceraolo Spurio (2019-07-24 03:21:51)
> > All the GuC objects are perma-pinned, so their offset can't change at
> > runtime. We can therefore set (and log!) the parameters only once during
> > boot.
> > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

This was standalone and I hope noncontroversial, so pushed.
-Chris

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
index 83f2c197375f..1ea6a9e50c02 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
@@ -126,66 +126,6 @@  static void guc_shared_data_destroy(struct intel_guc *guc)
 	i915_vma_unpin_and_release(&guc->shared_data, I915_VMA_RELEASE_MAP);
 }
 
-int intel_guc_init(struct intel_guc *guc)
-{
-	struct intel_gt *gt = guc_to_gt(guc);
-	int ret;
-
-	ret = intel_uc_fw_init(&guc->fw);
-	if (ret)
-		goto err_fetch;
-
-	ret = guc_shared_data_create(guc);
-	if (ret)
-		goto err_fw;
-	GEM_BUG_ON(!guc->shared_data);
-
-	ret = intel_guc_log_create(&guc->log);
-	if (ret)
-		goto err_shared;
-
-	ret = intel_guc_ads_create(guc);
-	if (ret)
-		goto err_log;
-	GEM_BUG_ON(!guc->ads_vma);
-
-	ret = intel_guc_ct_init(&guc->ct);
-	if (ret)
-		goto err_ads;
-
-	/* We need to notify the guc whenever we change the GGTT */
-	i915_ggtt_enable_guc(gt->ggtt);
-
-	return 0;
-
-err_ads:
-	intel_guc_ads_destroy(guc);
-err_log:
-	intel_guc_log_destroy(&guc->log);
-err_shared:
-	guc_shared_data_destroy(guc);
-err_fw:
-	intel_uc_fw_fini(&guc->fw);
-err_fetch:
-	intel_uc_fw_cleanup_fetch(&guc->fw);
-	return ret;
-}
-
-void intel_guc_fini(struct intel_guc *guc)
-{
-	struct intel_gt *gt = guc_to_gt(guc);
-
-	i915_ggtt_disable_guc(gt->ggtt);
-
-	intel_guc_ct_fini(&guc->ct);
-
-	intel_guc_ads_destroy(guc);
-	intel_guc_log_destroy(&guc->log);
-	guc_shared_data_destroy(guc);
-	intel_uc_fw_fini(&guc->fw);
-	intel_uc_fw_cleanup_fetch(&guc->fw);
-}
-
 static u32 guc_ctl_debug_flags(struct intel_guc *guc)
 {
 	u32 level = intel_guc_log_get_level(&guc->log);
@@ -281,13 +221,12 @@  static u32 guc_ctl_ads_flags(struct intel_guc *guc)
  * transfer. These parameters are read by the firmware on startup
  * and cannot be changed thereafter.
  */
-void intel_guc_init_params(struct intel_guc *guc)
+static void guc_init_params(struct intel_guc *guc)
 {
-	struct intel_uncore *uncore = guc_to_gt(guc)->uncore;
-	u32 params[GUC_CTL_MAX_DWORDS];
+	u32 *params = guc->params;
 	int i;
 
-	memset(params, 0, sizeof(params));
+	BUILD_BUG_ON(sizeof(guc->params) != GUC_CTL_MAX_DWORDS * sizeof(u32));
 
 	params[GUC_CTL_CTXINFO] = guc_ctl_ctxinfo_flags(guc);
 	params[GUC_CTL_LOG_PARAMS] = guc_ctl_log_params_flags(guc);
@@ -297,6 +236,17 @@  void intel_guc_init_params(struct intel_guc *guc)
 
 	for (i = 0; i < GUC_CTL_MAX_DWORDS; i++)
 		DRM_DEBUG_DRIVER("param[%2d] = %#x\n", i, params[i]);
+}
+
+/*
+ * Initialise the GuC parameter block before starting the firmware
+ * transfer. These parameters are read by the firmware on startup
+ * and cannot be changed thereafter.
+ */
+void intel_guc_write_params(struct intel_guc *guc)
+{
+	struct intel_uncore *uncore = guc_to_gt(guc)->uncore;
+	int i;
 
 	/*
 	 * All SOFT_SCRATCH registers are in FORCEWAKE_BLITTER domain and
@@ -308,11 +258,74 @@  void intel_guc_init_params(struct intel_guc *guc)
 	intel_uncore_write(uncore, SOFT_SCRATCH(0), 0);
 
 	for (i = 0; i < GUC_CTL_MAX_DWORDS; i++)
-		intel_uncore_write(uncore, SOFT_SCRATCH(1 + i), params[i]);
+		intel_uncore_write(uncore, SOFT_SCRATCH(1 + i), guc->params[i]);
 
 	intel_uncore_forcewake_put(uncore, FORCEWAKE_BLITTER);
 }
 
+int intel_guc_init(struct intel_guc *guc)
+{
+	struct intel_gt *gt = guc_to_gt(guc);
+	int ret;
+
+	ret = intel_uc_fw_init(&guc->fw);
+	if (ret)
+		goto err_fetch;
+
+	ret = guc_shared_data_create(guc);
+	if (ret)
+		goto err_fw;
+	GEM_BUG_ON(!guc->shared_data);
+
+	ret = intel_guc_log_create(&guc->log);
+	if (ret)
+		goto err_shared;
+
+	ret = intel_guc_ads_create(guc);
+	if (ret)
+		goto err_log;
+	GEM_BUG_ON(!guc->ads_vma);
+
+	ret = intel_guc_ct_init(&guc->ct);
+	if (ret)
+		goto err_ads;
+
+	/* now that everything is perma-pinned, initialize the parameters */
+	guc_init_params(guc);
+
+	/* We need to notify the guc whenever we change the GGTT */
+	i915_ggtt_enable_guc(gt->ggtt);
+
+	return 0;
+
+err_ads:
+	intel_guc_ads_destroy(guc);
+err_log:
+	intel_guc_log_destroy(&guc->log);
+err_shared:
+	guc_shared_data_destroy(guc);
+err_fw:
+	intel_uc_fw_fini(&guc->fw);
+err_fetch:
+	intel_uc_fw_cleanup_fetch(&guc->fw);
+	return ret;
+}
+
+void intel_guc_fini(struct intel_guc *guc)
+{
+	struct intel_gt *gt = guc_to_gt(guc);
+
+	i915_ggtt_disable_guc(gt->ggtt);
+
+	intel_guc_ct_fini(&guc->ct);
+
+	intel_guc_ads_destroy(guc);
+	intel_guc_log_destroy(&guc->log);
+	guc_shared_data_destroy(guc);
+	intel_uc_fw_fini(&guc->fw);
+	intel_uc_fw_cleanup_fetch(&guc->fw);
+}
+
 int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len,
 		       u32 *response_buf, u32 response_buf_size)
 {
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index f51c4c3c1d0b..714e9892aaff 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -76,6 +76,9 @@  struct intel_guc {
 	/* Cyclic counter mod pagesize	*/
 	u32 db_cacheline;
 
+	/* Control params for fw initialization */
+	u32 params[GUC_CTL_MAX_DWORDS];
+
 	/* GuC's FW specific registers used in MMIO send */
 	struct {
 		u32 base;
@@ -152,7 +155,7 @@  static inline u32 intel_guc_ggtt_offset(struct intel_guc *guc,
 
 void intel_guc_init_early(struct intel_guc *guc);
 void intel_guc_init_send_regs(struct intel_guc *guc);
-void intel_guc_init_params(struct intel_guc *guc);
+void intel_guc_write_params(struct intel_guc *guc);
 int intel_guc_init(struct intel_guc *guc);
 void intel_guc_fini(struct intel_guc *guc);
 int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len,
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index b761809946b1..35f621352f9b 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -492,7 +492,7 @@  int intel_uc_init_hw(struct intel_uc *uc)
 		}
 
 		intel_guc_ads_reset(guc);
-		intel_guc_init_params(guc);
+		intel_guc_write_params(guc);
 		ret = intel_guc_fw_upload(guc);
 		if (ret == 0)
 			break;