| Message ID | 1522398722-12161-2-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 30 Mar 2018 10:31:46 +0200, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote: > From: Tom O'Rourke <Tom.O'Rourke@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@intel.com> > Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> > Cc: Jeff McGee <jeff.mcgee@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@intel.com>
Thanks for the review. Will update with all suggestions in the next rev. On 3/30/2018 6:07 PM, Michal Wajdeczko wrote: > On Fri, 30 Mar 2018 10:31:46 +0200, Sagar Arun Kamble > <sagar.a.kamble@intel.com> wrote: > >> From: Tom O'Rourke <Tom.O'Rourke@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@intel.com> >> Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> >> Cc: Jeff McGee <jeff.mcgee@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@intel.com>
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)"); 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); + } + /* 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