[Intel-gfx] [PATCH v12 01/17] drm/i915/guc/slpc: Add SLPC control to enable_guc modparam
Michal Wajdeczko
michal.wajdeczko at intel.com
Fri Mar 30 12:37:30 UTC 2018
On Fri, 30 Mar 2018 10:31:46 +0200, Sagar Arun Kamble
<sagar.a.kamble at intel.com> wrote:
> From: Tom O'Rourke <Tom.O'Rourke at intel.com>
>
> GuC is currently being used for submission and HuC authentication.
> Choices can be configured through enable_guc modparam. GuC SLPC is GT
> Power and Performance management feature in GuC. Add another option to
> enable_guc modparam to control SLPC.
>
> v1: Add early call to sanitize enable_guc_slpc in intel_guc_ucode_init
> Remove sanitize enable_guc_slpc call before firmware version check
> is performed. (ChrisW)
> Version check is added in next patch and that will be done as part
> of slpc_enable_sanitize function in the next patch. (Sagar) Updated
> slpc option sanitize function call for platforms without GuC support.
> This was caught by CI BAT.
>
> v2: Changed parameter to dev_priv for HAS_SLPC macro. (David)
> Code indentation based on checkpatch.
>
> v3: Rebase.
>
> v4: Moved sanitization of SLPC option post GuC load.
>
> v5: Removed function intel_slpc_enabled. Planning to rely only on kernel
> parameter. Moved sanitization prior to GuC load to use the parameter
> during SLPC state setup during to GuC load. (Sagar)
>
> v6: Commit message update. Rebase.
>
> v7: Moved SLPC option sanitization to intel_uc_sanitize_options.
>
> v8: Clearing SLPC option on GuC load failure. Change moved from later
> patch. (Sagar)
>
> v9: s/enable_slpc/enable_guc_slpc. Rebase w.r.t modparam change.
>
> v10: Rebase. Separate modparam is not needed now that we maintain all
> options in single param enable_guc.
>
> Suggested-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Signed-off-by: Tom O'Rourke <Tom.O'Rourke at intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan at intel.com>
> Cc: Jeff McGee <jeff.mcgee at intel.com>
> ---
> drivers/gpu/drm/i915/i915_params.c | 5 +++--
> drivers/gpu/drm/i915/i915_params.h | 1 +
> drivers/gpu/drm/i915/intel_uc.c | 23 +++++++++++++++++++----
> drivers/gpu/drm/i915/intel_uc.h | 6 ++++++
> 4 files changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_params.c
> b/drivers/gpu/drm/i915/i915_params.c
> index 08108ce..40b799b 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -150,9 +150,10 @@ i915_param_named_unsafe(edp_vswing, int, 0400,
> "2=default swing(400mV))");
> i915_param_named_unsafe(enable_guc, int, 0400,
> - "Enable GuC load for GuC submission and/or HuC load. "
> + "Enable GuC load for GuC submission and/or HuC load and/or GuC SLPC. "
> "Required functionality can be selected using bitmask values. "
> - "(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
> + "(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load, "
> + "4=GuC SLPC)");
Maybe to avoid later surprise, we should explicitly say that:
+ "4=GuC SLPC [requires GuC submission])");
> i915_param_named(guc_log_level, int, 0400,
> "GuC firmware logging level. Requires GuC to be loaded. "
> diff --git a/drivers/gpu/drm/i915/i915_params.h
> b/drivers/gpu/drm/i915/i915_params.h
> index c963603..2484925 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_SLPC BIT(2)
> #define I915_PARAMS_FOR_EACH(param) \
> param(char *, vbt_firmware, NULL) \
> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> b/drivers/gpu/drm/i915/intel_uc.c
> index 1cffaf7..0e4a97f 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -56,9 +56,15 @@ static int __get_platform_enable_guc(struct
> drm_i915_private *dev_priv)
> struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
> int enable_guc = 0;
> - /* Default is to enable GuC/HuC if we know their firmwares */
> - if (intel_uc_fw_is_selected(guc_fw))
> + /*
> + * Default is to enable GuC submission/SLPC/HuC if we know their
> + * firmwares
> + */
> + if (intel_uc_fw_is_selected(guc_fw)) {
> enable_guc |= ENABLE_GUC_SUBMISSION;
> + enable_guc |= ENABLE_GUC_SLPC;
> + }
> +
> if (intel_uc_fw_is_selected(huc_fw))
> enable_guc |= ENABLE_GUC_LOAD_HUC;
> @@ -110,10 +116,11 @@ static void sanitize_options_early(struct
> drm_i915_private *dev_priv)
> if (i915_modparams.enable_guc < 0)
> i915_modparams.enable_guc = __get_platform_enable_guc(dev_priv);
> - DRM_DEBUG_DRIVER("enable_guc=%d (submission:%s huc:%s)\n",
> + DRM_DEBUG_DRIVER("enable_guc=%d (submission:%s huc:%s slpc:%s)\n",
> i915_modparams.enable_guc,
> yesno(intel_uc_is_using_guc_submission()),
> - yesno(intel_uc_is_using_huc()));
> + yesno(intel_uc_is_using_huc()),
> + yesno(intel_uc_is_using_guc_slpc()));
> /* Verify GuC firmware availability */
> if (intel_uc_is_using_guc() && !intel_uc_fw_is_selected(guc_fw)) {
> @@ -123,6 +130,14 @@ static void sanitize_options_early(struct
> drm_i915_private *dev_priv)
> "no GuC firmware");
> }
> + /* Verify GuC submission and SLPC dependency */
> + if (intel_uc_is_using_guc_slpc() &&
> + !intel_uc_is_using_guc_submission()) {
> + DRM_WARN("Incompatible option detected: %s=%d, "
> + "GuC SLPC enabled without enabling GuC submission!\n",
> + "enable_guc", i915_modparams.enable_guc);
If this is unsupported variant, then maybe we should clear slpc bit:
i915_modparams.enable_guc &= ~ENABLE_GUC_SLPC;
> + }
> +
> /* Verify HuC firmware availability */
> if (intel_uc_is_using_huc() && !intel_uc_fw_is_selected(huc_fw)) {
> DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
> diff --git a/drivers/gpu/drm/i915/intel_uc.h
> b/drivers/gpu/drm/i915/intel_uc.h
> index 25d73ad..76139d3 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -59,4 +59,10 @@ static inline bool intel_uc_is_using_huc(void)
> return i915_modparams.enable_guc & ENABLE_GUC_LOAD_HUC;
> }
> +static inline bool intel_uc_is_using_guc_slpc(void)
> +{
> + GEM_BUG_ON(i915_modparams.enable_guc < 0);
> + return i915_modparams.enable_guc & ENABLE_GUC_SLPC;
> +}
> +
> #endif
In intel_uc_init_hw() we print summary, so maybe add there:
dev_info(dev_priv->drm.dev, "GuC SLPC %s\n",
enableddisabled(USES_GUC_SLPC(dev_priv)));
Then we can move USES_GUC_SLPC() definition from patch 2 to 1.
With all that,
Reviewed-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
More information about the Intel-gfx
mailing list