[PATCH 0/3] Don't sanitize enable_guc

[PATCH 0/3] Don't sanitize enable_guc

[Michal Wajdeczko]

We want to stop relying on modparam for runtime uC status

Michal Wajdeczko (3):
  drm/i915/uc: Consider enable_guc modparam during fw selection
  drm/i915/guc: Use dedicated flag to track submission mode
  drm/i915/uc: Stop sanitizing enable_guc modparam

 drivers/gpu/drm/i915/gt/uc/intel_guc.c        |  1 +
 drivers/gpu/drm/i915/gt/uc/intel_guc.h        | 12 ++++
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 16 +++++
 .../gpu/drm/i915/gt/uc/intel_guc_submission.h |  1 +
 drivers/gpu/drm/i915/gt/uc/intel_huc.h        |  5 ++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c         | 70 +++++--------------
 drivers/gpu/drm/i915/gt/uc/intel_uc.h         |  9 +--
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c      | 25 ++++++-
 8 files changed, 78 insertions(+), 61 deletions(-)

-- 
2.19.2

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

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

[Michal Wajdeczko]

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(-)

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;
 	}
 
-- 
2.19.2

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

[PATCH 2/3] drm/i915/guc: Use dedicated flag to track submission mode

[Michal Wajdeczko]

Instead of relying on enable_guc modparam to represent actual
GuC submission mode, use dedicated flag and look at modparam
only to check if submission was explicitly disabled by the user.

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.c           |  1 +
 drivers/gpu/drm/i915/gt/uc/intel_guc.h           |  7 +++++++
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c    | 16 ++++++++++++++++
 .../gpu/drm/i915/gt/uc/intel_guc_submission.h    |  1 +
 drivers/gpu/drm/i915/gt/uc/intel_uc.h            |  3 +--
 5 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
index 13fbbffd05c7..a75ef0096f15 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
@@ -82,6 +82,7 @@ void intel_guc_init_early(struct intel_guc *guc)
 	intel_guc_fw_init_early(guc);
 	intel_guc_ct_init_early(&guc->ct);
 	intel_guc_log_init_early(&guc->log);
+	intel_guc_submission_init_early(guc);
 
 	mutex_init(&guc->send_mutex);
 	spin_lock_init(&guc->irq_lock);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index 5901506672cd..680625a0c722 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -64,6 +64,8 @@ struct intel_guc {
 	struct i915_vma *ads_vma;
 	struct __guc_ads_blob *ads_blob;
 
+	bool using_submission;
+
 	struct i915_vma *stage_desc_pool;
 	void *stage_desc_pool_vaddr;
 	struct ida stage_ids;
@@ -190,6 +192,11 @@ static inline int intel_guc_sanitize(struct intel_guc *guc)
 	return 0;
 }
 
+static inline bool intel_guc_is_submission_supported(struct intel_guc *guc)
+{
+	return guc->using_submission;
+}
+
 static inline void intel_guc_enable_msg(struct intel_guc *guc, u32 mask)
 {
 	spin_lock_irq(&guc->irq_lock);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index b4238fe16a03..b0cdbd9b2365 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -1163,6 +1163,22 @@ void intel_guc_submission_disable(struct intel_guc *guc)
 	guc_clients_disable(guc);
 }
 
+static bool __force_no_guc_submission(void)
+{
+	/* XXX: GuC submission is unavailable for now */
+	return true;
+
+	return (i915_modparams.enable_guc == 0) ||
+	       ((i915_modparams.enable_guc > 0) &&
+		!(i915_modparams.enable_guc & ENABLE_GUC_SUBMISSION));
+}
+
+void intel_guc_submission_init_early(struct intel_guc *guc)
+{
+	guc->using_submission = intel_guc_is_supported(guc) &&
+				!__force_no_guc_submission();
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftest_guc.c"
 #endif
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
index 87a38cb6faf3..c4ad2702ec8d 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
@@ -77,6 +77,7 @@ struct intel_guc_client {
 	I915_SELFTEST_DECLARE(bool use_nop_wqi);
 };
 
+void intel_guc_submission_init_early(struct intel_guc *guc);
 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);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
index c8e5ad9807db..96bd9a461c93 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
@@ -55,8 +55,7 @@ static inline bool intel_uc_is_using_guc(struct intel_uc *uc)
 
 static inline bool intel_uc_is_using_guc_submission(struct intel_uc *uc)
 {
-	GEM_BUG_ON(i915_modparams.enable_guc < 0);
-	return i915_modparams.enable_guc & ENABLE_GUC_SUBMISSION;
+	return intel_guc_is_submission_supported(&uc->guc);
 }
 
 static inline bool intel_uc_is_using_huc(struct intel_uc *uc)
-- 
2.19.2

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

[PATCH 3/3] drm/i915/uc: Stop sanitizing enable_guc modparam

[Michal Wajdeczko]

As we already track GuC/HuC uses by other means than modparam
there is no point in sanitizing it. Just scan modparam for
major discrepancies between what was requested vs actual.

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_uc.c | 70 +++++++--------------------
 1 file changed, 17 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 6eb8bb3fa252..34201d156271 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -55,78 +55,42 @@ static int __intel_uc_reset_hw(struct intel_uc *uc)
 	return ret;
 }
 
-static int __get_platform_enable_guc(struct intel_uc *uc)
+static void __confirm_options(struct intel_uc *uc)
 {
-	struct intel_uc_fw *guc_fw = &uc->guc.fw;
-	struct intel_uc_fw *huc_fw = &uc->huc.fw;
-	int enable_guc = 0;
-
-	if (!HAS_GT_UC(uc_to_gt(uc)->i915))
-		return 0;
-
-	/* We don't want to enable GuC/HuC on pre-Gen11 by default */
-	if (INTEL_GEN(uc_to_gt(uc)->i915) < 11)
-		return 0;
-
-	if (intel_uc_fw_supported(guc_fw) && intel_uc_fw_supported(huc_fw))
-		enable_guc |= ENABLE_GUC_LOAD_HUC;
-
-	return enable_guc;
-}
-
-/**
- * sanitize_options_early - sanitize uC related modparam options
- * @uc: the intel_uc structure
- *
- * In case of "enable_guc" option this function will attempt to modify
- * it only if it was initially set to "auto(-1)". Default value for this
- * modparam varies between platforms and it is hardcoded in driver code.
- * Any other modparam value is only monitored against availability of the
- * related hardware or firmware definitions.
- */
-static void sanitize_options_early(struct intel_uc *uc)
-{
-	struct intel_uc_fw *guc_fw = &uc->guc.fw;
-	struct intel_uc_fw *huc_fw = &uc->huc.fw;
-
-	/* A negative value means "use platform default" */
-	if (i915_modparams.enable_guc < 0)
-		i915_modparams.enable_guc = __get_platform_enable_guc(uc);
-
 	DRM_DEBUG_DRIVER("enable_guc=%d (submission:%s huc:%s)\n",
 			 i915_modparams.enable_guc,
 			 yesno(intel_uc_is_using_guc_submission(uc)),
 			 yesno(intel_uc_is_using_huc(uc)));
 
-	/* Verify GuC firmware availability */
-	if (intel_uc_is_using_guc(uc) && !intel_uc_fw_supported(guc_fw)) {
+	if (i915_modparams.enable_guc < 0)
+		return;
+
+	if (i915_modparams.enable_guc == 0) {
+		GEM_BUG_ON(intel_uc_is_using_guc(uc));
+		GEM_BUG_ON(intel_uc_is_using_guc_submission(uc));
+		GEM_BUG_ON(intel_uc_is_using_huc(uc));
+		return;
+	}
+
+	if (!intel_uc_is_using_guc(uc)) {
 		DRM_WARN("Incompatible option detected: enable_guc=%d, "
 			 "but GuC is not supported!\n",
 			 i915_modparams.enable_guc);
-		DRM_INFO("Disabling GuC/HuC loading!\n");
-		i915_modparams.enable_guc = 0;
 	}
 
-	/* Verify HuC firmware availability */
-	if (intel_uc_is_using_huc(uc) && !intel_uc_fw_supported(huc_fw)) {
+	if (i915_modparams.enable_guc & ENABLE_GUC_LOAD_HUC &&
+	    !intel_uc_is_using_huc(uc)) {
 		DRM_WARN("Incompatible option detected: enable_guc=%d, "
 			 "but HuC is not supported!\n",
 			 i915_modparams.enable_guc);
-		DRM_INFO("Disabling HuC loading!\n");
-		i915_modparams.enable_guc &= ~ENABLE_GUC_LOAD_HUC;
 	}
 
-	/* XXX: GuC submission is unavailable for now */
-	if (intel_uc_is_using_guc_submission(uc)) {
+	if (i915_modparams.enable_guc & ENABLE_GUC_SUBMISSION &&
+	    !intel_uc_is_using_guc_submission(uc)) {
 		DRM_INFO("Incompatible option detected: enable_guc=%d, "
 			 "but GuC submission is not supported!\n",
 			 i915_modparams.enable_guc);
-		DRM_INFO("Switching to non-GuC submission mode!\n");
-		i915_modparams.enable_guc &= ~ENABLE_GUC_SUBMISSION;
 	}
-
-	/* Make sure that sanitization was done */
-	GEM_BUG_ON(i915_modparams.enable_guc < 0);
 }
 
 void intel_uc_init_early(struct intel_uc *uc)
@@ -134,7 +98,7 @@ void intel_uc_init_early(struct intel_uc *uc)
 	intel_guc_init_early(&uc->guc);
 	intel_huc_init_early(&uc->huc);
 
-	sanitize_options_early(uc);
+	__confirm_options(uc);
 }
 
 void intel_uc_cleanup_early(struct intel_uc *uc)
-- 
2.19.2

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

Re: [PATCH 3/3] drm/i915/uc: Stop sanitizing enable_guc modparam

[Chris Wilson]

Quoting Michal Wajdeczko (2019-07-30 19:19:03)
> As we already track GuC/HuC uses by other means than modparam
> there is no point in sanitizing it. Just scan modparam for
> major discrepancies between what was requested vs actual.

Note that igt is using this modparam to discover if guc submission is
active. :|

Fortunately, it appears that no one is using that facility so quickly
remove it!
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/guc: Use dedicated flag to track submission mode

[Chris Wilson]

Quoting Michal Wajdeczko (2019-07-30 19:19:02)
> Instead of relying on enable_guc modparam to represent actual
> GuC submission mode, use dedicated flag and look at modparam
> only to check if submission was explicitly disabled by the user.
> 
> 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.c           |  1 +
>  drivers/gpu/drm/i915/gt/uc/intel_guc.h           |  7 +++++++
>  .../gpu/drm/i915/gt/uc/intel_guc_submission.c    | 16 ++++++++++++++++
>  .../gpu/drm/i915/gt/uc/intel_guc_submission.h    |  1 +
>  drivers/gpu/drm/i915/gt/uc/intel_uc.h            |  3 +--
>  5 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> index 13fbbffd05c7..a75ef0096f15 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> @@ -82,6 +82,7 @@ void intel_guc_init_early(struct intel_guc *guc)
>         intel_guc_fw_init_early(guc);
>         intel_guc_ct_init_early(&guc->ct);
>         intel_guc_log_init_early(&guc->log);
> +       intel_guc_submission_init_early(guc);
>  
>         mutex_init(&guc->send_mutex);
>         spin_lock_init(&guc->irq_lock);
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index 5901506672cd..680625a0c722 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> @@ -64,6 +64,8 @@ struct intel_guc {
>         struct i915_vma *ads_vma;
>         struct __guc_ads_blob *ads_blob;
>  
> +       bool using_submission;

First boolean, but not necessarily the last?

> +
>         struct i915_vma *stage_desc_pool;
>         void *stage_desc_pool_vaddr;
>         struct ida stage_ids;
> @@ -190,6 +192,11 @@ static inline int intel_guc_sanitize(struct intel_guc *guc)
>         return 0;
>  }
>  
> +static inline bool intel_guc_is_submission_supported(struct intel_guc *guc)
> +{
> +       return guc->using_submission;

I still suffer a logical disconnect between using and is.

> +}
> +
>  static inline void intel_guc_enable_msg(struct intel_guc *guc, u32 mask)
>  {
>         spin_lock_irq(&guc->irq_lock);
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index b4238fe16a03..b0cdbd9b2365 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -1163,6 +1163,22 @@ void intel_guc_submission_disable(struct intel_guc *guc)
>         guc_clients_disable(guc);
>  }
>  
> +static bool __force_no_guc_submission(void)
> +{
> +       /* XXX: GuC submission is unavailable for now */
> +       return true;
> +
> +       return (i915_modparams.enable_guc == 0) ||
> +              ((i915_modparams.enable_guc > 0) &&
> +               !(i915_modparams.enable_guc & ENABLE_GUC_SUBMISSION));

enable_guc <= 0 || !(enable_guc & ENABLE_SUBMISSION)

?

Then in future if you want <-1 for auto enable, s/<=/==/
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for Don't sanitize enable_guc

[Patchwork]

== Series Details ==

Series: Don't sanitize enable_guc
URL   : https://patchwork.freedesktop.org/series/64446/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
26060db5e380 drm/i915/uc: Consider enable_guc modparam during fw selection
-:81: CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'i915_modparams.enable_guc < 0'
#81: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c:137:
+	if ((i915_modparams.enable_guc < 0) && (p < INTEL_ICELAKE))

-:81: CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'p < INTEL_ICELAKE'
#81: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c:137:
+	if ((i915_modparams.enable_guc < 0) && (p < INTEL_ICELAKE))

-:85: ERROR:POINTER_LOCATION: "foo* bar" should be "foo *bar"
#85: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c:141:
+static const char* __override_guc_firmware_path(void)

-:88: CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'i915_modparams.enable_guc < 0'
#88: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c:144:
+	if ((i915_modparams.enable_guc < 0) ||
+	    (i915_modparams.enable_guc & ENABLE_GUC_LOAD_HUC))

-:94: ERROR:POINTER_LOCATION: "foo* bar" should be "foo *bar"
#94: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c:150:
+static const char* __override_huc_firmware_path(void)

-:96: CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'i915_modparams.enable_guc < 0'
#96: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c:152:
+	if ((i915_modparams.enable_guc < 0) ||
+	    (i915_modparams.enable_guc & ENABLE_GUC_LOAD_HUC))

total: 2 errors, 0 warnings, 4 checks, 79 lines checked
b73f7436e919 drm/i915/guc: Use dedicated flag to track submission mode
8b60c2d8e654 drm/i915/uc: Stop sanitizing enable_guc modparam

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

✓ Fi.CI.BAT: success for Don't sanitize enable_guc

[Patchwork]

== Series Details ==

Series: Don't sanitize enable_guc
URL   : https://patchwork.freedesktop.org/series/64446/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6586 -> Patchwork_13809
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13809/

Known issues
------------

  Here are the changes found in Patchwork_13809 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_busy@basic-flip-c:
    - fi-kbl-7500u:       [PASS][1] -> [SKIP][2] ([fdo#109271] / [fdo#109278]) +2 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6586/fi-kbl-7500u/igt@kms_busy@basic-flip-c.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13809/fi-kbl-7500u/igt@kms_busy@basic-flip-c.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u2:          [PASS][3] -> [FAIL][4] ([fdo#103167])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6586/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13809/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html

  
#### Possible fixes ####

  * igt@gem_ctx_create@basic-files:
    - fi-icl-dsi:         [INCOMPLETE][5] ([fdo#107713] / [fdo#109100]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6586/fi-icl-dsi/igt@gem_ctx_create@basic-files.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13809/fi-icl-dsi/igt@gem_ctx_create@basic-files.html

  * igt@gem_exec_reloc@basic-write-gtt-noreloc:
    - fi-icl-u3:          [DMESG-WARN][7] ([fdo#107724]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6586/fi-icl-u3/igt@gem_exec_reloc@basic-write-gtt-noreloc.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13809/fi-icl-u3/igt@gem_exec_reloc@basic-write-gtt-noreloc.html

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-blb-e6850:       [INCOMPLETE][9] ([fdo#107718]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6586/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13809/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html

  * igt@i915_module_load@reload-no-display:
    - fi-bwr-2160:        [INCOMPLETE][11] ([fdo#111174]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6586/fi-bwr-2160/igt@i915_module_load@reload-no-display.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13809/fi-bwr-2160/igt@i915_module_load@reload-no-display.html

  * igt@prime_vgem@basic-fence-flip:
    - fi-kbl-7500u:       [SKIP][13] ([fdo#109271]) -> [PASS][14] +23 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6586/fi-kbl-7500u/igt@prime_vgem@basic-fence-flip.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13809/fi-kbl-7500u/igt@prime_vgem@basic-fence-flip.html

  
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#111174]: https://bugs.freedesktop.org/show_bug.cgi?id=111174


Participating hosts (54 -> 46)
------------------------------

  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-icl-y fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_6586 -> Patchwork_13809

  CI-20190529: 20190529
  CI_DRM_6586: 066993443a56467f54fdcf560e89378f8e93a15b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5116: d2e6dd2f789596da5bd06efc2e9448e3160583b6 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13809: 8b60c2d8e654529e5514066e6dd905b73a2b2bad @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

8b60c2d8e654 drm/i915/uc: Stop sanitizing enable_guc modparam
b73f7436e919 drm/i915/guc: Use dedicated flag to track submission mode
26060db5e380 drm/i915/uc: Consider enable_guc modparam during fw selection

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13809/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Michal Wajdeczko]

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)
...
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 2/4] drm/i915/uc: Consider enable_guc modparam during fw selection

[Michal Wajdeczko]

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.

v2: rebased, don't care about <0 (Chris)
v3: oops

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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> #v1
---
 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 | 23 +++++++++++++++++++++--
 4 files changed, 33 insertions(+), 6 deletions(-)

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 66d8b1ee6f1d..cf6c60cffdfb 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
@@ -51,8 +51,7 @@ int intel_uc_runtime_resume(struct intel_uc *uc);
 
 static inline bool intel_uc_supports_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_supports_guc_submission(struct intel_uc *uc)
@@ -63,8 +62,7 @@ static inline bool intel_uc_supports_guc_submission(struct intel_uc *uc)
 
 static inline bool intel_uc_supports_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..650ad6037b74 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,25 @@ __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 == -1 && p < INTEL_ICELAKE)
+		uc_fw->path = NULL;
+}
+
+static const char *__override_guc_firmware_path(void)
+{
+	if (i915_modparams.enable_guc & (ENABLE_GUC_SUBMISSION |
+					 ENABLE_GUC_LOAD_HUC))
+		return i915_modparams.guc_firmware_path;
+	return "";
+}
+
+static const char *__override_huc_firmware_path(void)
+{
+	if (i915_modparams.enable_guc & ENABLE_GUC_LOAD_HUC)
+		return i915_modparams.huc_firmware_path;
+	return "";
 }
 
 static bool
@@ -139,10 +158,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;
 	}
 
-- 
2.19.2

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

Re: [PATCH v3 2/4] drm/i915/uc: Consider enable_guc modparam during fw selection

[Chris Wilson]

Quoting Michal Wajdeczko (2019-07-31 19:00:35)
> 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.
> 
> v2: rebased, don't care about <0 (Chris)
> v3: oops

Replied to wrong thread?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx