Re: [Intel-gfx] [PATCH v2 5/7] drm/i915/guc: Combine enable_guc_loading|submission modparams
On Fri, 01 Dec 2017 16:47:40 +0100, Sagar Arun Kamblewrote: On 12/1/2017 4:03 PM, Michal Wajdeczko wrote: We currently have two module parameters that control GuC: "enable_guc_loading" and "enable_guc_submission". Whenever we need submission=1, we also need loading=1. We also need loading=1 when we want to want to load and verify the HuC. Lets combine above module parameters into one "enable_guc" modparam. New supported bit values are: 0=disable GuC (no GuC submission, no HuC) 1=enable GuC submission 2=enable HuC load Special value "-1" can be used to let driver decide what option should be enabled for given platform based on hardware/firmware availability or preference. Explicit enabling any of the GuC features makes GuC load a required step, fallback to non-GuC mode will not be supported. v2: Don't use -EIO Signed-off-by: Michal Wajdeczko Cc: Chris Wilson Cc: Joonas Lahtinen Cc: Sagar Arun Kamble Cc: Sujaritha Sundaresan +void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) +{ + struct intel_uc_fw *guc_fw = _priv->guc.fw; + struct intel_uc_fw *huc_fw = _priv->huc.fw; + int enable_guc = __get_default_enable_guc(dev_priv); /* A negative value means "use platform default" */ - if (i915_modparams.enable_guc_submission < 0) - i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv); + if (i915_modparams.enable_guc < 0) + i915_modparams.enable_guc = enable_guc; + + /* Verify GuC firmware availability */ + if (USES_GUC(dev_priv) && !intel_uc_fw_is_selected(guc_fw)) { + DRM_WARN("Incompatible option enable_guc=%d - %s\n", +i915_modparams.enable_guc, +!HAS_GUC(dev_priv) ? "no GuC hardware" : + "no GuC firmware"); + } + + /* Verify HuC firmware availability */ + if (USES_GUC_SUBMISSION(dev_priv) && !intel_uc_fw_is_selected(huc_fw)) { should be USES_HUC here good catch! thanks ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 5/7] drm/i915/guc: Combine enable_guc_loading|submission modparams
On 12/1/2017 4:03 PM, Michal Wajdeczko wrote: We currently have two module parameters that control GuC: "enable_guc_loading" and "enable_guc_submission". Whenever we need submission=1, we also need loading=1. We also need loading=1 when we want to want to load and verify the HuC. Lets combine above module parameters into one "enable_guc" modparam. New supported bit values are: 0=disable GuC (no GuC submission, no HuC) 1=enable GuC submission 2=enable HuC load Special value "-1" can be used to let driver decide what option should be enabled for given platform based on hardware/firmware availability or preference. Explicit enabling any of the GuC features makes GuC load a required step, fallback to non-GuC mode will not be supported. v2: Don't use -EIO Signed-off-by: Michal WajdeczkoCc: Chris Wilson Cc: Joonas Lahtinen Cc: Sagar Arun Kamble Cc: Sujaritha Sundaresan +void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) +{ + struct intel_uc_fw *guc_fw = _priv->guc.fw; + struct intel_uc_fw *huc_fw = _priv->huc.fw; + int enable_guc = __get_default_enable_guc(dev_priv); /* A negative value means "use platform default" */ - if (i915_modparams.enable_guc_submission < 0) - i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv); + if (i915_modparams.enable_guc < 0) + i915_modparams.enable_guc = enable_guc; + + /* Verify GuC firmware availability */ + if (USES_GUC(dev_priv) && !intel_uc_fw_is_selected(guc_fw)) { + DRM_WARN("Incompatible option enable_guc=%d - %s\n", +i915_modparams.enable_guc, +!HAS_GUC(dev_priv) ? "no GuC hardware" : + "no GuC firmware"); + } + + /* Verify HuC firmware availability */ + if (USES_GUC_SUBMISSION(dev_priv) && !intel_uc_fw_is_selected(huc_fw)) { should be USES_HUC here ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 5/7] drm/i915/guc: Combine enable_guc_loading|submission modparams
Quoting Michal Wajdeczko (2017-12-01 14:09:31) > On Fri, 01 Dec 2017 13:36:06 +0100, Chris Wilson >wrote: > > > Quoting Michal Wajdeczko (2017-12-01 10:33:15) > >> We currently have two module parameters that control GuC: > >> "enable_guc_loading" and "enable_guc_submission". Whenever > >> we need submission=1, we also need loading=1. We also need > >> loading=1 when we want to want to load and verify the HuC. > >> > >> Lets combine above module parameters into one "enable_guc" > >> modparam. New supported bit values are: > >> > >> 0=disable GuC (no GuC submission, no HuC) > >> 1=enable GuC submission > >> 2=enable HuC load > >> > >> Special value "-1" can be used to let driver decide what > >> option should be enabled for given platform based on > >> hardware/firmware availability or preference. > >> > >> Explicit enabling any of the GuC features makes GuC load > >> a required step, fallback to non-GuC mode will not be > >> supported. > >> > >> v2: Don't use -EIO > >> > >> Signed-off-by: Michal Wajdeczko > >> Cc: Chris Wilson > >> Cc: Joonas Lahtinen > >> Cc: Sagar Arun Kamble > >> Cc: Sujaritha Sundaresan > >> --- > >> drivers/gpu/drm/i915/i915_drv.h| 5 +- > >> drivers/gpu/drm/i915/i915_params.c | 11 ++-- > >> drivers/gpu/drm/i915/i915_params.h | 3 +- > >> drivers/gpu/drm/i915/intel_uc.c| 100 > >> + > >> 4 files changed, 65 insertions(+), 54 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h > >> b/drivers/gpu/drm/i915/i915_drv.h > >> index 2c386c7..9162a60 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.h > >> +++ b/drivers/gpu/drm/i915/i915_drv.h > >> @@ -3238,8 +3238,9 @@ intel_info(const struct drm_i915_private > >> *dev_priv) > >> #define HAS_HUC_UCODE(dev_priv)(HAS_GUC(dev_priv)) > >> > >> /* Having a GuC is not the same as using a GuC */ > >> -#define USES_GUC(dev_priv) > >> (i915_modparams.enable_guc_loading) > >> -#define USES_GUC_SUBMISSION(dev_priv) > >> (i915_modparams.enable_guc_submission) > >> +#define USES_GUC(dev_priv) (!!(i915_modparams.enable_guc > > >> 0)) > >> +#define USES_GUC_SUBMISSION(dev_priv) (!!(i915_modparams.enable_guc & > >> 1)) > >> +#define USES_HUC(dev_priv) (!!(i915_modparams.enable_guc & > >> 2)) > > > > * channelling Joonas > > > > Please use a magic name for each bit and so > > > > #define USE_GUC_SUBMISSION_BIT 0 > > I was considering that, but then I decided to follow existing code > (see USES_PPGTT* and enable_ppgtt where we use plain numbers only) enable_ppgtt is on my list to kill. If vgpu didn't conspire against us, it would be simpler! > But if we want to start using magic values, then these values should > be defined in i915_params.h and rather in this way: > > #define ENABLE_GUC_SUBMISSION BIT(0) > #define ENABLE_GUC_HUC_LOAD BIT(1) > ^^ > this part matches modparam name Reasonable. > > #define USES_GUC_SUBMISSION (!!(i915_modparams.enable_guc & > > BIT(USE_GUC_SUBMISSION_BIT))) > > > > Gah, that's so ugly. > > > > or > > > > static inline bool intel_use_guc_submission(void) > > "intel_" ? maybe correct prefix should be "i915_" ? > > > { > > return i915_modparams.enable_guc & BIT(USE_GUC_SUBMISSION_BIT); > > } > > I assume that above will still be wrapped inside macro > > #define USES_GUC_SUBMISSION(dev_priv) intel_uses_guc_submission() Why? The compiler will dce functions just as well as macros; the age of the macro is long past, so the question is just a matter of how much is it worth continuing to cargo-cult a pattern that is long past requirement. Even its placement in i915_drv.h is up for debate. Maintaining the status quo is a valid argument, we just need a good reason to switch patterns (splitting up X-thousand lines of code into manageable chunks with consistent forward-facing interfaces is one such :). -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 5/7] drm/i915/guc: Combine enable_guc_loading|submission modparams
On Fri, 01 Dec 2017 13:36:06 +0100, Chris Wilsonwrote: Quoting Michal Wajdeczko (2017-12-01 10:33:15) We currently have two module parameters that control GuC: "enable_guc_loading" and "enable_guc_submission". Whenever we need submission=1, we also need loading=1. We also need loading=1 when we want to want to load and verify the HuC. Lets combine above module parameters into one "enable_guc" modparam. New supported bit values are: 0=disable GuC (no GuC submission, no HuC) 1=enable GuC submission 2=enable HuC load Special value "-1" can be used to let driver decide what option should be enabled for given platform based on hardware/firmware availability or preference. Explicit enabling any of the GuC features makes GuC load a required step, fallback to non-GuC mode will not be supported. v2: Don't use -EIO Signed-off-by: Michal Wajdeczko Cc: Chris Wilson Cc: Joonas Lahtinen Cc: Sagar Arun Kamble Cc: Sujaritha Sundaresan --- drivers/gpu/drm/i915/i915_drv.h| 5 +- drivers/gpu/drm/i915/i915_params.c | 11 ++-- drivers/gpu/drm/i915/i915_params.h | 3 +- drivers/gpu/drm/i915/intel_uc.c| 100 + 4 files changed, 65 insertions(+), 54 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2c386c7..9162a60 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3238,8 +3238,9 @@ intel_info(const struct drm_i915_private *dev_priv) #define HAS_HUC_UCODE(dev_priv)(HAS_GUC(dev_priv)) /* Having a GuC is not the same as using a GuC */ -#define USES_GUC(dev_priv) (i915_modparams.enable_guc_loading) -#define USES_GUC_SUBMISSION(dev_priv) (i915_modparams.enable_guc_submission) +#define USES_GUC(dev_priv) (!!(i915_modparams.enable_guc > 0)) +#define USES_GUC_SUBMISSION(dev_priv) (!!(i915_modparams.enable_guc & 1)) +#define USES_HUC(dev_priv) (!!(i915_modparams.enable_guc & 2)) * channelling Joonas Please use a magic name for each bit and so #define USE_GUC_SUBMISSION_BIT 0 I was considering that, but then I decided to follow existing code (see USES_PPGTT* and enable_ppgtt where we use plain numbers only) But if we want to start using magic values, then these values should be defined in i915_params.h and rather in this way: #define ENABLE_GUC_SUBMISSION BIT(0) #define ENABLE_GUC_HUC_LOAD BIT(1) ^^ this part matches modparam name #define USES_GUC_SUBMISSION (!!(i915_modparams.enable_guc & BIT(USE_GUC_SUBMISSION_BIT))) Gah, that's so ugly. or static inline bool intel_use_guc_submission(void) "intel_" ? maybe correct prefix should be "i915_" ? { return i915_modparams.enable_guc & BIT(USE_GUC_SUBMISSION_BIT); } I assume that above will still be wrapped inside macro #define USES_GUC_SUBMISSION(dev_priv) intel_uses_guc_submission() which at least stops being so shouty. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 5/7] drm/i915/guc: Combine enable_guc_loading|submission modparams
Quoting Michal Wajdeczko (2017-12-01 10:33:15) > We currently have two module parameters that control GuC: > "enable_guc_loading" and "enable_guc_submission". Whenever > we need submission=1, we also need loading=1. We also need > loading=1 when we want to want to load and verify the HuC. > > Lets combine above module parameters into one "enable_guc" > modparam. New supported bit values are: > > 0=disable GuC (no GuC submission, no HuC) > 1=enable GuC submission > 2=enable HuC load > > Special value "-1" can be used to let driver decide what > option should be enabled for given platform based on > hardware/firmware availability or preference. > > Explicit enabling any of the GuC features makes GuC load > a required step, fallback to non-GuC mode will not be > supported. > > v2: Don't use -EIO > > Signed-off-by: Michal Wajdeczko> Cc: Chris Wilson > Cc: Joonas Lahtinen > Cc: Sagar Arun Kamble > Cc: Sujaritha Sundaresan > --- > drivers/gpu/drm/i915/i915_drv.h| 5 +- > drivers/gpu/drm/i915/i915_params.c | 11 ++-- > drivers/gpu/drm/i915/i915_params.h | 3 +- > drivers/gpu/drm/i915/intel_uc.c| 100 > + > 4 files changed, 65 insertions(+), 54 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 2c386c7..9162a60 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3238,8 +3238,9 @@ intel_info(const struct drm_i915_private *dev_priv) > #define HAS_HUC_UCODE(dev_priv)(HAS_GUC(dev_priv)) > > /* Having a GuC is not the same as using a GuC */ > -#define USES_GUC(dev_priv) (i915_modparams.enable_guc_loading) > -#define USES_GUC_SUBMISSION(dev_priv) (i915_modparams.enable_guc_submission) > +#define USES_GUC(dev_priv) (!!(i915_modparams.enable_guc > 0)) > +#define USES_GUC_SUBMISSION(dev_priv) (!!(i915_modparams.enable_guc & 1)) > +#define USES_HUC(dev_priv) (!!(i915_modparams.enable_guc & 2)) * channelling Joonas Please use a magic name for each bit and so #define USE_GUC_SUBMISSION_BIT 0 #define USES_GUC_SUBMISSION (!!(i915_modparams.enable_guc & BIT(USE_GUC_SUBMISSION_BIT))) Gah, that's so ugly. or static inline bool intel_use_guc_submission(void) { return i915_modparams.enable_guc & BIT(USE_GUC_SUBMISSION_BIT); } which at least stops being so shouty. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 5/7] drm/i915/guc: Combine enable_guc_loading|submission modparams
We currently have two module parameters that control GuC: "enable_guc_loading" and "enable_guc_submission". Whenever we need submission=1, we also need loading=1. We also need loading=1 when we want to want to load and verify the HuC. Lets combine above module parameters into one "enable_guc" modparam. New supported bit values are: 0=disable GuC (no GuC submission, no HuC) 1=enable GuC submission 2=enable HuC load Special value "-1" can be used to let driver decide what option should be enabled for given platform based on hardware/firmware availability or preference. Explicit enabling any of the GuC features makes GuC load a required step, fallback to non-GuC mode will not be supported. v2: Don't use -EIO Signed-off-by: Michal WajdeczkoCc: Chris Wilson Cc: Joonas Lahtinen Cc: Sagar Arun Kamble Cc: Sujaritha Sundaresan --- drivers/gpu/drm/i915/i915_drv.h| 5 +- drivers/gpu/drm/i915/i915_params.c | 11 ++-- drivers/gpu/drm/i915/i915_params.h | 3 +- drivers/gpu/drm/i915/intel_uc.c| 100 + 4 files changed, 65 insertions(+), 54 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2c386c7..9162a60 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3238,8 +3238,9 @@ intel_info(const struct drm_i915_private *dev_priv) #define HAS_HUC_UCODE(dev_priv)(HAS_GUC(dev_priv)) /* Having a GuC is not the same as using a GuC */ -#define USES_GUC(dev_priv) (i915_modparams.enable_guc_loading) -#define USES_GUC_SUBMISSION(dev_priv) (i915_modparams.enable_guc_submission) +#define USES_GUC(dev_priv) (!!(i915_modparams.enable_guc > 0)) +#define USES_GUC_SUBMISSION(dev_priv) (!!(i915_modparams.enable_guc & 1)) +#define USES_HUC(dev_priv) (!!(i915_modparams.enable_guc & 2)) #define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)->info.has_resource_streamer) diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 3328147..8654e45 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -154,13 +154,10 @@ i915_param_named_unsafe(edp_vswing, int, 0400, "(0=use value from vbt [default], 1=low power swing(200mV)," "2=default swing(400mV))"); -i915_param_named_unsafe(enable_guc_loading, int, 0400, - "Enable GuC firmware loading " - "(-1=auto, 0=never [default], 1=if available, 2=required)"); - -i915_param_named_unsafe(enable_guc_submission, int, 0400, - "Enable GuC submission " - "(-1=auto, 0=never [default], 1=if available, 2=required)"); +i915_param_named_unsafe(enable_guc, int, 0400, + "Enable GuC load for GuC submission and/or HuC load. " + "Required functionality can be selected using bitmask values. " + "(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)"); i915_param_named(guc_log_level, int, 0400, "GuC firmware logging level (-1:disabled (default), 0-3:enabled)"); diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index 8321bd8..10e2f74 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -42,8 +42,7 @@ param(int, disable_power_well, -1) \ param(int, enable_ips, 1) \ param(int, invert_brightness, 0) \ - param(int, enable_guc_loading, 0) \ - param(int, enable_guc_submission, 0) \ + param(int, enable_guc, 0) \ param(int, guc_log_level, -1) \ param(char *, guc_firmware_path, NULL) \ param(char *, huc_firmware_path, NULL) \ diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index ed2dd76..8a761af 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -47,35 +47,59 @@ static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv) return ret; } -void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) +static int __get_default_enable_guc(struct drm_i915_private *dev_priv) { - if (!HAS_GUC(dev_priv)) { - if (i915_modparams.enable_guc_loading > 0 || - i915_modparams.enable_guc_submission > 0) - DRM_INFO("Ignoring GuC options, no hardware\n"); + struct intel_uc_fw *guc_fw = _priv->guc.fw; + struct intel_uc_fw *huc_fw = _priv->huc.fw; + /* Default is to enable GuC/HuC if we know their firmwares */ + int enable_guc = (intel_uc_fw_is_selected(guc_fw) ? 1 : 0) | +(intel_uc_fw_is_selected(huc_fw) ? 2 : 0); - i915_modparams.enable_guc_loading = 0; - i915_modparams.enable_guc_submission = 0; - return; - } + /* Any platform specific fine-tuning can be done here */ - /* A