[Intel-gfx] [PATCH v8 2/6] drm/i915/guc : Removing i915_modparams.enable_guc_loading module parameter
Michal Wajdeczko
michal.wajdeczko at intel.com
Wed Oct 25 15:26:29 UTC 2017
On Tue, 24 Oct 2017 19:21:21 +0200, Sujaritha Sundaresan
<sujaritha.sundaresan at intel.com> 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 verify the HuC, which
> is every time we have a HuC (but all platforms with HuC
> have a GuC and viceversa).
>
> Also if we have HuC have firmware to be loaded, we need to
> have GuC to actually load it. So if the user wants to avoid
> the GuC from getting loaded, they must not have a HuC
> firmware to be loaded, in addition to not using submission.
Hmm, I'm not sure that removal of HuC firmware file is the best
way for the user to stop undesired GuC loading.
I know that we want to minimize number of modparams, but maybe
new i915.enable_huc=auto(-1)|never(0)|if available(1)|required(2)
will solve here ...
Alternatively we can replace both existing modparams with single:
i915.enable_guc = off(0) | auto(1) | submission(2) | huc(4)
then we could cover almost all cases:
0 = GuC loading disabled (no GuC submission, no HuC)
1 = GuC loading auto
2 = GuC loading enabled, GuC submission required, HuC disabled
3 = GuC loading enabled, GuC submission enabled, HuC disabled
4 = GuC loading enabled, GuC submission disabled, HuC required
5 = GuC loading enabled, GuC submission disabled, HuC enabled
6 = GuC loading enabled, GuC submission required, HuC required
7 = GuC loading enabled, GuC submission enabled, HuC enabled
>
> v2: Clarifying the commit message (Anusha)
>
> v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
>
> v4: Rebase
>
> v5: Separating message unification into a separate patch
>
> v6: Re-factoring code (Sagar, Michal)
> Rebase
>
> v7: Applying review comments (Sagar)
> Rebase
>
> v8: Change to NEEDS_GUC_FW (Chris)
> Applying review comments (Michal)
> Clarifying commit message (Joonas)
>
> Suggested by: Oscar Mateo <oscar.mateo at intel.com>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan at intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Oscar Mateo <oscar.mateo at intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
> drivers/gpu/drm/i915/i915_drv.h | 9 +++--
> drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
> drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
> drivers/gpu/drm/i915/i915_irq.c | 2 +-
> drivers/gpu/drm/i915/i915_params.c | 4 ---
> drivers/gpu/drm/i915/i915_params.h | 1 -
> drivers/gpu/drm/i915/intel_uc.c | 57
> +++++++++++++++------------------
> 8 files changed, 34 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 8edd029..25c47a0 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2465,7 +2465,7 @@ static bool check_guc_submission(struct seq_file
> *m)
> if (!guc->execbuf_client) {
> seq_printf(m, "GuC submission %s\n",
> - HAS_GUC_SCHED(dev_priv) ?
> + HAS_GUC(dev_priv) ?
> "disabled" :
> "not supported");
As I already said before, there is also 3rd possible status "failed"
!HAS_GUC(dev_priv) ? "not supported" :
!HAS_GUC_SCHED(dev_priv) ? "disabled" :
"failed"
where HAS_GUC_SCHED is
#define HAS_GUC_SCHED(dev_priv) \
(HAS_GUC(dev_priv) && i915_modparams.enable_guc_submission)
or something similar
> return false;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index f01c800..ede5004 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3205,9 +3205,11 @@ static inline unsigned int
> i915_sg_segment_size(void)
> */
> #define HAS_GUC(dev_priv) ((dev_priv)->info.has_guc)
> #define HAS_GUC_CT(dev_priv) ((dev_priv)->info.has_guc_ct)
> -#define HAS_GUC_UCODE(dev_priv) (HAS_GUC(dev_priv))
> -#define HAS_GUC_SCHED(dev_priv) (HAS_GUC(dev_priv))
> -#define HAS_HUC_UCODE(dev_priv) (HAS_GUC(dev_priv))
> +#define HAS_GUC_UCODE(dev_priv) ((dev_priv)->guc.fw.path != NULL)
> +#define HAS_HUC_UCODE(dev_priv) ((dev_priv)->huc.fw.path != NULL)
> +
> +#define NEEDS_GUC_FW(dev_priv) \
Hmm, based on its usage, this name is now little confusing.
Maybe USES_GUC ? See USES_PPGTT|USES_FULL_PPGTT|USES_FULL_48BIT_PPGTT
> + (HAS_GUC(dev_priv) && \
> + (i915_modparams.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))
While unlikely, above will be true even with guc.fw.path == NULL
Also, based on your statement "all platforms with HuC have a GuC
and viceversa" I would assume that corresponding firmwares will
be delivered also in pairs (except short enabling periods).
Thus on every platform with has_guc=1 there will be guc.fw.path != NULL
and huc.fw.path != NULL, effectively making this macro almost the
same as HAS_GUC (as other cases are just error cases).
If we want to make GuC loading conditional, we should use better
condition ;)
> #define HAS_RESOURCE_STREAMER(dev_priv)
> ((dev_priv)->info.has_resource_streamer)
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 5bf96a2..4f0692e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -314,7 +314,7 @@ static u32 default_desc_template(const struct
> drm_i915_private *i915,
> * present or not in use we still need a small bias as ring wraparound
> * at offset 0 sometimes hangs. No idea why.
> */
> - if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
> + if (NEEDS_GUC_FW(dev_priv))
> ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
> else
> ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 527a2d2..9d78233 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3481,7 +3481,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private
> *dev_priv)
> * currently don't have any bits spare to pass in this upper
> * restriction!
> */
> - if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading) {
> + if (NEEDS_GUC_FW(dev_priv)) {
> ggtt->base.total = min_t(u64, ggtt->base.total, GUC_GGTT_TOP);
> ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total);
> }
> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> b/drivers/gpu/drm/i915/i915_irq.c
> index b1296a5..ec76aac 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4026,7 +4026,7 @@ void intel_irq_init(struct drm_i915_private
> *dev_priv)
> for (i = 0; i < MAX_L3_SLICES; ++i)
> dev_priv->l3_parity.remap_info[i] = NULL;
> - if (HAS_GUC_SCHED(dev_priv))
> + if (HAS_GUC(dev_priv))
> dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
> /* Let's track the enabled rps events */
> diff --git a/drivers/gpu/drm/i915/i915_params.c
> b/drivers/gpu/drm/i915/i915_params.c
> index b4faeb6..1c25f45 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -162,10 +162,6 @@ struct i915_params i915_modparams __read_mostly = {
> "(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)");
> diff --git a/drivers/gpu/drm/i915/i915_params.h
> b/drivers/gpu/drm/i915/i915_params.h
> index c729226..9e1e231 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -44,7 +44,6 @@
> 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, guc_log_level, -1) \
> param(char *, guc_firmware_path, NULL) \
> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> b/drivers/gpu/drm/i915/intel_uc.c
> index 25bd162..9369ade 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -49,36 +49,35 @@ static int __intel_uc_reset_hw(struct
> drm_i915_private *dev_priv)
> void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
> {
> + /* Verify Hardware support */
> 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");
> -
> - i915_modparams.enable_guc_loading = 0;
> - i915_modparams.enable_guc_submission = 0;
> + if (i915_modparams.enable_guc_submission > 0)
> + DRM_INFO("Ignoring option %s - no hardware",
> "enable_guc_submission");
> + i915_modparams.enable_guc_submission = 0;
> return;
> }
> - /* A negative value means "use platform default" */
> - if (i915_modparams.enable_guc_loading < 0)
> - i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
> -
> /* Verify firmware version */
> - if (i915_modparams.enable_guc_loading) {
> - if (HAS_HUC_UCODE(dev_priv))
> - intel_huc_select_fw(&dev_priv->huc);
> + if (HAS_GUC_UCODE(dev_priv)) {
Hmm, if !HAS_UCODE ?
> + if (i915_modparams.enable_guc_submission > 0) {
> + DRM_INFO("Ignoring option %s - no firmware",
> "enable_guc_submission");
> + i915_modparams.enable_guc_submission = 0;
> + return;
> + }
> - if (intel_guc_fw_select(&dev_priv->guc))
Hmm, I can't see now when fw will be selected...
Note that you are using here HAS_GUC_UCODE that depends on fw path
> - i915_modparams.enable_guc_loading = 0;
> + if (i915_modparams.enable_guc_submission < 0) {
> + i915_modparams.enable_guc_submission = 0;
> + return;
> + }
> }
> - /* Can't enable guc submission without guc loaded */
> - if (!i915_modparams.enable_guc_loading)
> - i915_modparams.enable_guc_submission = 0;
> + /*
> + * A negative value means "use platform default" (enabled if we have
> + * survived to get here)
> + */
> - /* A negative value means "use platform default" */
> if (i915_modparams.enable_guc_submission < 0)
> - i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> + i915_modparams.enable_guc_submission = HAS_GUC(dev_priv);
At this point we are sure that we have GuC (with fw) so we can use
i915_modparams.enable_guc_submission = 1;
> }
> void intel_uc_init_early(struct drm_i915_private *dev_priv)
> @@ -154,7 +153,7 @@ int intel_uc_init_hw(struct drm_i915_private
> *dev_priv)
> struct intel_guc *guc = &dev_priv->guc;
> int ret, attempts;
> - if (!i915_modparams.enable_guc_loading)
> + if (!NEEDS_GUC_FW(dev_priv))
> return 0;
> guc_disable_communication(guc);
> @@ -250,22 +249,16 @@ int intel_uc_init_hw(struct drm_i915_private
> *dev_priv)
> err_guc:
> i915_ggtt_disable_guc(dev_priv);
> - if (i915_modparams.enable_guc_loading > 1 ||
> - i915_modparams.enable_guc_submission > 1) {
> + if (i915_modparams.enable_guc_submission > 1) {
> DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
> ret = -EIO;
> + } else if (i915_modparams.enable_guc_submission == 1) {
> + DRM_NOTE("Falling back from GuC submission to execlist mode.\n");
> + ret = 0;
> } else {
> - DRM_NOTE("GuC init failed. Firmware loading disabled.\n");
> ret = 0;
> }
> - if (i915_modparams.enable_guc_submission) {
> - i915_modparams.enable_guc_submission = 0;
> - DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> - }
> -
> - i915_modparams.enable_guc_loading = 0;
> -
> return ret;
> }
> @@ -273,7 +266,7 @@ void intel_uc_fini_hw(struct drm_i915_private
> *dev_priv)
> {
> guc_free_load_err_log(&dev_priv->guc);
> - if (!i915_modparams.enable_guc_loading)
> + if (!NEEDS_GUC_FW(dev_priv))
> return;
> if (i915_modparams.enable_guc_submission)
More information about the Intel-gfx
mailing list