diff mbox series

drm/i915/uc: Add function to define defaults for GuC/HuC enable

Message ID 20210105211345.6990-1-John.C.Harrison@Intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/uc: Add function to define defaults for GuC/HuC enable | expand

Commit Message

John Harrison Jan. 5, 2021, 9:13 p.m. UTC
From: John Harrison <John.C.Harrison@Intel.com>

There is a module parameter for controlling what GuC/HuC features are
enabled. Setting to -1 means 'use the default'. However, the default
is not well defined, out of date and needs to be different across
platforms.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
CC: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_uc.c    | 28 ++++++++++++++++++++++--
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c |  7 +-----
 drivers/gpu/drm/i915/i915_params.h       |  1 +
 3 files changed, 28 insertions(+), 8 deletions(-)

Comments

Daniele Ceraolo Spurio Jan. 8, 2021, 12:51 a.m. UTC | #1
On 1/5/2021 1:13 PM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> There is a module parameter for controlling what GuC/HuC features are
> enabled. Setting to -1 means 'use the default'. However, the default
> is not well defined, out of date and needs to be different across
> platforms.

I believe this needs a bit more detail about the change in behavior. -1 
used to map to HuC loading on all gen11+ platforms, while after this 
change it will map to disabled on all current platforms and HuC loading 
on dg1 and all future platforms.

> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> CC: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_uc.c    | 28 ++++++++++++++++++++++--
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c |  7 +-----
>   drivers/gpu/drm/i915/i915_params.h       |  1 +
>   3 files changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index 6a0452815c41..2c08db58cf12 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -15,6 +15,29 @@
>   static const struct intel_uc_ops uc_ops_off;
>   static const struct intel_uc_ops uc_ops_on;
>   
> +static void uc_expand_default_options(struct intel_uc *uc)
> +{
> +	struct drm_i915_private *i915 = uc_to_gt(uc)->i915;
> +
> +	if (i915->params.enable_guc != -1)
> +		return;
> +
> +	/* Don't enable GuC/HuC on pre-Gen12 */
> +	if (INTEL_GEN(i915) < 12) {
> +		i915->params.enable_guc = 0;
> +		return;
> +	}
> +
> +	/* Don't enable GuC/HuC on older Gen12 platforms */
> +	if (IS_TIGERLAKE(i915) || IS_ROCKETLAKE(i915)) {
> +		i915->params.enable_guc = 0;
> +		return;
> +	}
> +
> +	/* Default: enable HuC authentication only */
> +	i915->params.enable_guc = ENABLE_GUC_LOAD_HUC;
> +}
> +
>   /* Reset GuC providing us with fresh state for both GuC and HuC.
>    */
>   static int __intel_uc_reset_hw(struct intel_uc *uc)
> @@ -79,8 +102,7 @@ static void __confirm_options(struct intel_uc *uc)
>   			 "Incompatible option enable_guc=%d - %s\n",
>   			 i915->params.enable_guc, "GuC submission is N/A");
>   
> -	if (i915->params.enable_guc & ~(ENABLE_GUC_SUBMISSION |
> -					  ENABLE_GUC_LOAD_HUC))
> +	if (i915->params.enable_guc & ~ENABLE_GUC_MASK)
>   		drm_info(&i915->drm,
>   			 "Incompatible option enable_guc=%d - %s\n",
>   			 i915->params.enable_guc, "undocumented flag");
> @@ -88,6 +110,8 @@ static void __confirm_options(struct intel_uc *uc)
>   
>   void intel_uc_init_early(struct intel_uc *uc)
>   {
> +	uc_expand_default_options(uc);
> +
>   	intel_guc_init_early(&uc->guc);
>   	intel_huc_init_early(&uc->huc);

here there is a call to __confirm_options() that has a check for 
enable_guc == -1, which can be dropped since we can't reach here with 
that value anymore.

with these addressed:
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

>   
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> index 602f1a0bc587..67b06fde1225 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -152,16 +152,11 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
>   			uc_fw->path = NULL;
>   		}
>   	}
> -
> -	/* We don't want to enable GuC/HuC on pre-Gen11 by default */
> -	if (i915->params.enable_guc == -1 && p < INTEL_ICELAKE)
> -		uc_fw->path = NULL;
>   }
>   
>   static const char *__override_guc_firmware_path(struct drm_i915_private *i915)
>   {
> -	if (i915->params.enable_guc & (ENABLE_GUC_SUBMISSION |
> -				       ENABLE_GUC_LOAD_HUC))
> +	if (i915->params.enable_guc & ENABLE_GUC_MASK)
>   		return i915->params.guc_firmware_path;
>   	return "";
>   }
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 330c03e2b4f7..f031966af5b7 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -32,6 +32,7 @@ struct drm_printer;
>   
>   #define ENABLE_GUC_SUBMISSION		BIT(0)
>   #define ENABLE_GUC_LOAD_HUC		BIT(1)
> +#define ENABLE_GUC_MASK			GENMASK(1, 0)
>   
>   /*
>    * Invoke param, a function-like macro, for each i915 param, with arguments:
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 6a0452815c41..2c08db58cf12 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -15,6 +15,29 @@ 
 static const struct intel_uc_ops uc_ops_off;
 static const struct intel_uc_ops uc_ops_on;
 
+static void uc_expand_default_options(struct intel_uc *uc)
+{
+	struct drm_i915_private *i915 = uc_to_gt(uc)->i915;
+
+	if (i915->params.enable_guc != -1)
+		return;
+
+	/* Don't enable GuC/HuC on pre-Gen12 */
+	if (INTEL_GEN(i915) < 12) {
+		i915->params.enable_guc = 0;
+		return;
+	}
+
+	/* Don't enable GuC/HuC on older Gen12 platforms */
+	if (IS_TIGERLAKE(i915) || IS_ROCKETLAKE(i915)) {
+		i915->params.enable_guc = 0;
+		return;
+	}
+
+	/* Default: enable HuC authentication only */
+	i915->params.enable_guc = ENABLE_GUC_LOAD_HUC;
+}
+
 /* Reset GuC providing us with fresh state for both GuC and HuC.
  */
 static int __intel_uc_reset_hw(struct intel_uc *uc)
@@ -79,8 +102,7 @@  static void __confirm_options(struct intel_uc *uc)
 			 "Incompatible option enable_guc=%d - %s\n",
 			 i915->params.enable_guc, "GuC submission is N/A");
 
-	if (i915->params.enable_guc & ~(ENABLE_GUC_SUBMISSION |
-					  ENABLE_GUC_LOAD_HUC))
+	if (i915->params.enable_guc & ~ENABLE_GUC_MASK)
 		drm_info(&i915->drm,
 			 "Incompatible option enable_guc=%d - %s\n",
 			 i915->params.enable_guc, "undocumented flag");
@@ -88,6 +110,8 @@  static void __confirm_options(struct intel_uc *uc)
 
 void intel_uc_init_early(struct intel_uc *uc)
 {
+	uc_expand_default_options(uc);
+
 	intel_guc_init_early(&uc->guc);
 	intel_huc_init_early(&uc->huc);
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index 602f1a0bc587..67b06fde1225 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -152,16 +152,11 @@  __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
 			uc_fw->path = NULL;
 		}
 	}
-
-	/* We don't want to enable GuC/HuC on pre-Gen11 by default */
-	if (i915->params.enable_guc == -1 && p < INTEL_ICELAKE)
-		uc_fw->path = NULL;
 }
 
 static const char *__override_guc_firmware_path(struct drm_i915_private *i915)
 {
-	if (i915->params.enable_guc & (ENABLE_GUC_SUBMISSION |
-				       ENABLE_GUC_LOAD_HUC))
+	if (i915->params.enable_guc & ENABLE_GUC_MASK)
 		return i915->params.guc_firmware_path;
 	return "";
 }
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 330c03e2b4f7..f031966af5b7 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -32,6 +32,7 @@  struct drm_printer;
 
 #define ENABLE_GUC_SUBMISSION		BIT(0)
 #define ENABLE_GUC_LOAD_HUC		BIT(1)
+#define ENABLE_GUC_MASK			GENMASK(1, 0)
 
 /*
  * Invoke param, a function-like macro, for each i915 param, with arguments: