diff mbox series

[1/3] drm/i915/uc: Consider enable_guc modparam during fw selection

Message ID 20190730181903.17820-2-michal.wajdeczko@intel.com (mailing list archive)
State New, archived
Headers show
Series Don't sanitize enable_guc | expand

Commit Message

Michal Wajdeczko July 30, 2019, 6:19 p.m. UTC 
We can use value of enable_guc modparam during firmware path selection
and start using firmware status to see if GuC/HuC is being used.
This is first step to make enable_guc modparam read-only.

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

Comments

Chris Wilson July 30, 2019, 7:07 p.m. UTC | #1 
Quoting Michal Wajdeczko (2019-07-30 19:19:01)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> index fe3362fd7706..c8e5ad9807db 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> @@ -50,8 +50,7 @@ int intel_uc_resume(struct intel_uc *uc);
>  
>  static inline bool intel_uc_is_using_guc(struct intel_uc *uc)
>  {
> -       GEM_BUG_ON(i915_modparams.enable_guc < 0);
> -       return i915_modparams.enable_guc > 0;
> +       return intel_guc_is_supported(&uc->guc);

is_using_guc sounds like it should be looking at guc_is_running

I think the callers read better for me if I
s/intel_uc_is_using_guc/intel_uc_uses_guc/
or even better if intel_uc_supports_guc().

With that in mind,

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> 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 ac91e3efd02b..3f051451caba 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -132,6 +132,27 @@ __uc_fw_auto_select(struct intel_uc_fw *uc_fw, enum intel_platform p, u8 rev)
>                         uc_fw->path = NULL;
>                 }
>         }
> +
> +       /* We don't want to enable GuC/HuC on pre-Gen11 by default */
> +       if ((i915_modparams.enable_guc < 0) && (p < INTEL_ICELAKE))
> +               uc_fw->path = NULL;

(Bonus) (brackets)
> +}
> +
> +static const char* __override_guc_firmware_path(void)
> +{
> +       /* XXX: don't check for GuC submission as it is unavailable for now */
> +       if ((i915_modparams.enable_guc < 0) ||
> +           (i915_modparams.enable_guc & ENABLE_GUC_LOAD_HUC))
> +               return i915_modparams.guc_firmware_path;
> +       return "";
> +}
> +
> +static const char* __override_huc_firmware_path(void)
> +{
> +       if ((i915_modparams.enable_guc < 0) ||
> +           (i915_modparams.enable_guc & ENABLE_GUC_LOAD_HUC))
> +               return i915_modparams.huc_firmware_path;

Looks habitual.

We can even lose the <0. No negative value other than -1 is documented.
-Chris
Michal Wajdeczko July 31, 2019, 1:19 p.m. UTC | #2 
On Tue, 30 Jul 2019 21:07:28 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

>> +static const char* __override_huc_firmware_path(void)
>> +{
>> +       if ((i915_modparams.enable_guc < 0) ||
>> +           (i915_modparams.enable_guc & ENABLE_GUC_LOAD_HUC))
>> +               return i915_modparams.huc_firmware_path;
>
> We can even lose the <0. No negative value other than -1 is documented.

I used <0 to match existing implementation in sanitize_options_early()

	/* A negative value means "use platform default" */
	if (i915_modparams.enable_guc < 0)
		i915_modparams.enable_guc = __get_platform_enable_guc(uc);

if we lose <0 condition there are questions how to treat undocumented  
values:
-2 is disabled(0) or auto but without submission aka huc-only(2)
-3 is disabled(0) or auto but without huc aka submission_only(1)
...
Chris Wilson July 31, 2019, 1:22 p.m. UTC | #3 
Quoting Michal Wajdeczko (2019-07-31 14:19:06)
> On Tue, 30 Jul 2019 21:07:28 +0200, Chris Wilson  
> <chris@chris-wilson.co.uk> wrote:
> 
> >> +static const char* __override_huc_firmware_path(void)
> >> +{
> >> +       if ((i915_modparams.enable_guc < 0) ||
> >> +           (i915_modparams.enable_guc & ENABLE_GUC_LOAD_HUC))
> >> +               return i915_modparams.huc_firmware_path;
> >
> > We can even lose the <0. No negative value other than -1 is documented.
> 
> I used <0 to match existing implementation in sanitize_options_early()
> 
>         /* A negative value means "use platform default" */
>         if (i915_modparams.enable_guc < 0)
>                 i915_modparams.enable_guc = __get_platform_enable_guc(uc);
> 
> if we lose <0 condition there are questions how to treat undocumented  
> values:
> -2 is disabled(0) or auto but without submission aka huc-only(2)
> -3 is disabled(0) or auto but without huc aka submission_only(1)
> ...

I'm willing to let users shoot themselves in the foot for undocumented
values for an unsafe parameter. They already snatched hold of the
shotgun for using an unsafe parameter in the first place.
-Chris
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index 714e9892aaff..5901506672cd 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -172,6 +172,11 @@  int intel_guc_suspend(struct intel_guc *guc);
 int intel_guc_resume(struct intel_guc *guc);
 struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
 
+static inline bool intel_guc_is_supported(struct intel_guc *guc)
+{
+	return intel_uc_fw_supported(&guc->fw);
+}
+
 static inline bool intel_guc_is_running(struct intel_guc *guc)
 {
 	return intel_uc_fw_is_running(&guc->fw);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
index 4465209ce233..a6ae59b8cb77 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
@@ -55,6 +55,11 @@  static inline int intel_huc_sanitize(struct intel_huc *huc)
 	return 0;
 }
 
+static inline bool intel_huc_is_supported(struct intel_huc *huc)
+{
+	return intel_uc_fw_supported(&huc->fw);
+}
+
 static inline bool intel_huc_is_authenticated(struct intel_huc *huc)
 {
 	return intel_uc_fw_is_running(&huc->fw);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
index fe3362fd7706..c8e5ad9807db 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
@@ -50,8 +50,7 @@  int intel_uc_resume(struct intel_uc *uc);
 
 static inline bool intel_uc_is_using_guc(struct intel_uc *uc)
 {
-	GEM_BUG_ON(i915_modparams.enable_guc < 0);
-	return i915_modparams.enable_guc > 0;
+	return intel_guc_is_supported(&uc->guc);
 }
 
 static inline bool intel_uc_is_using_guc_submission(struct intel_uc *uc)
@@ -62,8 +61,7 @@  static inline bool intel_uc_is_using_guc_submission(struct intel_uc *uc)
 
 static inline bool intel_uc_is_using_huc(struct intel_uc *uc)
 {
-	GEM_BUG_ON(i915_modparams.enable_guc < 0);
-	return i915_modparams.enable_guc & ENABLE_GUC_LOAD_HUC;
+	return intel_huc_is_supported(&uc->huc);
 }
 
 #endif
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 ac91e3efd02b..3f051451caba 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -132,6 +132,27 @@  __uc_fw_auto_select(struct intel_uc_fw *uc_fw, enum intel_platform p, u8 rev)
 			uc_fw->path = NULL;
 		}
 	}
+
+	/* We don't want to enable GuC/HuC on pre-Gen11 by default */
+	if ((i915_modparams.enable_guc < 0) && (p < INTEL_ICELAKE))
+		uc_fw->path = NULL;
+}
+
+static const char* __override_guc_firmware_path(void)
+{
+	/* XXX: don't check for GuC submission as it is unavailable for now */
+	if ((i915_modparams.enable_guc < 0) ||
+	    (i915_modparams.enable_guc & ENABLE_GUC_LOAD_HUC))
+		return i915_modparams.guc_firmware_path;
+	return "";
+}
+
+static const char* __override_huc_firmware_path(void)
+{
+	if ((i915_modparams.enable_guc < 0) ||
+	    (i915_modparams.enable_guc & ENABLE_GUC_LOAD_HUC))
+		return i915_modparams.huc_firmware_path;
+	return "";
 }
 
 static bool
@@ -139,10 +160,10 @@  __uc_fw_override(struct intel_uc_fw *uc_fw)
 {
 	switch (uc_fw->type) {
 	case INTEL_UC_FW_TYPE_GUC:
-		uc_fw->path = i915_modparams.guc_firmware_path;
+		uc_fw->path = __override_guc_firmware_path();
 		break;
 	case INTEL_UC_FW_TYPE_HUC:
-		uc_fw->path = i915_modparams.huc_firmware_path;
+		uc_fw->path = __override_huc_firmware_path();
 		break;
 	}