[PATCH 3/4] drm/i915/guc: Combine enable_guc_loading|submission modparams
Sagar Arun Kamble
sagar.a.kamble at intel.com
Thu Nov 30 07:14:42 UTC 2017
Spotted enable_guc_submission in intel_guc_live_selftest.
Mentioning that enable_guc is now immutable for non-negative values in commit or param description would be helpful
On 11/29/2017 10:57 PM, Michal Wajdeczko wrote:
> WIP
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 5 ++
> drivers/gpu/drm/i915/i915_gem_context.c | 4 +-
> drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
> drivers/gpu/drm/i915/i915_irq.c | 2 +-
> drivers/gpu/drm/i915/i915_params.c | 11 ++-
> drivers/gpu/drm/i915/i915_params.h | 3 +-
> drivers/gpu/drm/i915/intel_guc.c | 2 +-
> drivers/gpu/drm/i915/intel_guc_log.c | 8 +--
> drivers/gpu/drm/i915/intel_gvt.c | 2 +-
> drivers/gpu/drm/i915/intel_uc.c | 123 +++++++++++++++++---------------
> 10 files changed, 83 insertions(+), 79 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4c3616b..05d02c5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3237,6 +3237,11 @@ intel_info(const struct drm_i915_private *dev_priv)
> #define HAS_HUC(dev_priv) (HAS_GUC(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 > 0)
> +#define USES_GUC_SUBMISSION(dev_priv) (i915_modparams.enable_guc & 1)
> +#define USES_HUC(dev_priv) (i915_modparams.enable_guc & 2)
macro for bits will be useful. probably you might have planned already :)
#define ENABLE_GUC_SUBMISSION BIT(0)
#define ENABLE_HUC BIT(1)
in future possibly
#define ENABLE_SLPC BIT(2)
> +
> #define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)->info.has_resource_streamer)
>
> #define HAS_POOLED_EU(dev_priv) ((dev_priv)->info.has_pooled_eu)
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index ce3139e..21ce374 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -316,7 +316,7 @@ __create_hw_context(struct drm_i915_private *dev_priv,
> * 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 (USES_GUC(dev_priv))
> ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
> else
> ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
> @@ -409,7 +409,7 @@ i915_gem_context_create_gvt(struct drm_device *dev)
> i915_gem_context_set_closed(ctx); /* not user accessible */
> i915_gem_context_clear_bannable(ctx);
> i915_gem_context_set_force_single_submission(ctx);
> - if (!i915_modparams.enable_guc_submission)
> + if (!USES_GUC_SUBMISSION(to_i915(dev)))
> ctx->ring_size = 512 * PAGE_SIZE; /* Max ring buffer size */
>
> GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index f3c35e8..209bb11 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3503,7 +3503,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 (USES_GUC(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 4fb183a..5e4f44f 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1400,7 +1400,7 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir, int test_shift)
>
> if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) {
> notify_ring(engine);
> - tasklet |= i915_modparams.enable_guc_submission;
> + tasklet |= USES_GUC_SUBMISSION(engine->i915);
> }
>
> if (tasklet)
> 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_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 8136478..39919e9 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -129,7 +129,7 @@ void intel_guc_init_params(struct intel_guc *guc)
> }
>
> /* If GuC submission is enabled, set up additional parameters here */
> - if (i915_modparams.enable_guc_submission) {
> + if (USES_GUC_SUBMISSION(dev_priv)) {
> u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
> u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool);
> u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 76d3eb1..fba636b 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -505,8 +505,7 @@ static void guc_flush_logs(struct intel_guc *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
>
> - if (!i915_modparams.enable_guc_submission ||
> - (i915_modparams.guc_log_level < 0))
> + if (!USES_GUC(dev_priv) || (i915_modparams.guc_log_level < 0))
This actually is good change but since log setup is still under USES_GUC_SUBMISSION, this too
should stay under USES_GUC_SUBMISSION for now. Same for below two functions.
Will change this once we decouple logging from submission.
> return;
>
> /* First disable the interrupts, will be renabled afterwards */
> @@ -646,8 +645,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
>
> void i915_guc_log_register(struct drm_i915_private *dev_priv)
> {
> - if (!i915_modparams.enable_guc_submission ||
> - (i915_modparams.guc_log_level < 0))
> + if (!USES_GUC(dev_priv) || (i915_modparams.guc_log_level < 0))
> return;
>
> mutex_lock(&dev_priv->drm.struct_mutex);
> @@ -657,7 +655,7 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv)
>
> void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
> {
> - if (!i915_modparams.enable_guc_submission)
> + if (!USES_GUC(dev_priv))
> return;
>
> mutex_lock(&dev_priv->drm.struct_mutex);
> diff --git a/drivers/gpu/drm/i915/intel_gvt.c b/drivers/gpu/drm/i915/intel_gvt.c
> index 126f7c7..a2fe7c8 100644
> --- a/drivers/gpu/drm/i915/intel_gvt.c
> +++ b/drivers/gpu/drm/i915/intel_gvt.c
> @@ -95,7 +95,7 @@ int intel_gvt_init(struct drm_i915_private *dev_priv)
> return 0;
> }
>
> - if (i915_modparams.enable_guc_submission) {
> + if (USES_GUC_SUBMISSION(dev_priv)) {
> DRM_ERROR("i915 GVT-g loading failed due to Graphics virtualization is not yet supported with GuC submission\n");
> return -EIO;
> }
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index ccc89869..bd655d4 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -47,32 +47,52 @@ static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv)
> return ret;
> }
>
> +/**
> + * intel_uc_sanitize_options - sanitize uC related modparam options
> + * @dev_priv: device private
> + *
> + * 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.
> + */
> void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
> {
> - /* Verify GuC firmware availability */
> - if (!intel_uc_fw_is_defined(&dev_priv->guc.fw)) {
> - if (i915_modparams.enable_guc_loading > 0 ||
> - i915_modparams.enable_guc_submission > 0)
> - DRM_INFO("Ignoring GuC options, %s\n",
> - !HAS_GUC(dev_priv) ? "no hardware" :
> - "no firmware");
> -
> - i915_modparams.enable_guc_loading = 0;
> - 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);
> -
> - /* Can't enable guc submission without guc loaded */
> - if (!i915_modparams.enable_guc_loading)
> - i915_modparams.enable_guc_submission = 0;
> -
> + struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
> + struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
> + int auto_enable_guc =
> + (intel_uc_fw_is_defined(guc_fw) ? 1 : 0) |
> + (intel_uc_fw_is_defined(huc_fw) ? 2 : 0);
> +
> + /* Make sure that our "platform default" is well-defined */
> + GEM_BUG_ON(auto_enable_guc < 0);
> + GEM_BUG_ON((auto_enable_guc > 0) && !intel_uc_fw_is_defined(guc_fw));
> + GEM_BUG_ON((auto_enable_guc & 2) && !intel_uc_fw_is_defined(huc_fw));
> +
> /* 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 = auto_enable_guc;
> +
> + /* Verify GuC firmware availability */
> + if (i915_modparams.enable_guc > 0) {
> + if (!intel_uc_fw_is_defined(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 (i915_modparams.enable_guc & 2) {
> + if (!intel_uc_fw_is_defined(huc_fw)) {
> + DRM_WARN("Incompatible option enable_guc=%d - %s\n",
> + i915_modparams.enable_guc,
> + !HAS_HUC(dev_priv) ? "no HuC hardware" :
> + "no HuC firmware");
> + }
This inner if can go in above if.
> + }
> }
>
> void intel_uc_init_early(struct drm_i915_private *dev_priv)
> @@ -147,9 +167,10 @@ static void guc_disable_communication(struct intel_guc *guc)
> int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> {
> struct intel_guc *guc = &dev_priv->guc;
> + struct intel_huc *huc = &dev_priv->huc;
> int ret, attempts;
>
> - if (!i915_modparams.enable_guc_loading)
> + if (!USES_GUC(dev_priv))
> return 0;
>
> guc_disable_communication(guc);
> @@ -158,7 +179,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> /* We need to notify the guc whenever we change the GGTT */
> i915_ggtt_enable_guc(dev_priv);
>
> - if (i915_modparams.enable_guc_submission) {
> + if (USES_GUC_SUBMISSION(dev_priv)) {
> /*
> * This is stuff we need to have available at fw load time
> * if we are planning to enable submission later
> @@ -189,7 +210,9 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> if (ret)
> goto err_submission;
>
> - intel_huc_init_hw(&dev_priv->huc);
> + if (USES_HUC(dev_priv))
> + intel_huc_init_hw(huc);
> +
> intel_guc_init_params(guc);
> ret = intel_guc_fw_upload(guc);
> if (ret == 0 || ret != -EAGAIN)
> @@ -207,8 +230,10 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> if (ret)
> goto err_log_capture;
>
> - intel_huc_auth(&dev_priv->huc);
> - if (i915_modparams.enable_guc_submission) {
> + if (USES_HUC(dev_priv))
> + intel_huc_auth(huc);
> +
> + if (USES_GUC_SUBMISSION(dev_priv)) {
> if (i915_modparams.guc_log_level >= 0)
> gen9_enable_guc_interrupts(dev_priv);
>
> @@ -217,22 +242,17 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> goto err_interrupts;
> }
>
> - dev_info(dev_priv->drm.dev, "GuC %s (firmware %s [version %u.%u])\n",
> - i915_modparams.enable_guc_submission ? "submission enabled" :
> - "loaded",
> - guc->fw.path,
> + dev_info(dev_priv->drm.dev, "GuC firmware version %u.%u\n",
> guc->fw.major_ver_found, guc->fw.minor_ver_found);
> + dev_info(dev_priv->drm.dev, "GuC submission %s\n",
> + enableddisabled(USES_GUC_SUBMISSION(dev_priv)));
> + dev_info(dev_priv->drm.dev, "HuC %s\n",
> + enableddisabled(USES_HUC(dev_priv)));
>
> return 0;
>
> /*
> * We've failed to load the firmware :(
> - *
> - * Decide whether to disable GuC submission and fall back to
> - * execlist mode, and whether to hide the error by returning
> - * zero or to return -EIO, which the caller will treat as a
> - * nonfatal error (i.e. it doesn't prevent driver load, but
> - * marks the GPU as wedged until reset).
> */
> err_interrupts:
> guc_disable_communication(guc);
> @@ -240,28 +260,13 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> err_log_capture:
> guc_capture_load_err_log(guc);
> err_submission:
> - if (i915_modparams.enable_guc_submission)
> + if (USES_GUC_SUBMISSION(dev_priv))
> intel_guc_submission_fini(guc);
> err_guc:
> i915_ggtt_disable_guc(dev_priv);
>
> - if (i915_modparams.enable_guc_loading > 1 ||
> - i915_modparams.enable_guc_submission > 1) {
> - DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
> - ret = -EIO;
> - } 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;
> + dev_err(dev_priv->drm.dev, "GuC initialization failed.\n");
> + return -EIO;
In case of enable_guc=-1 if we fail should we fall back to execlists.
> }
>
> void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
> @@ -270,15 +275,15 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>
> guc_free_load_err_log(guc);
>
> - if (!i915_modparams.enable_guc_loading)
> + if (!USES_GUC(dev_priv))
> return;
>
> - if (i915_modparams.enable_guc_submission)
> + if (USES_GUC_SUBMISSION(dev_priv))
> intel_guc_submission_disable(guc);
>
> guc_disable_communication(guc);
>
> - if (i915_modparams.enable_guc_submission) {
> + if (USES_GUC_SUBMISSION(dev_priv)) {
> gen9_disable_guc_interrupts(dev_priv);
> intel_guc_submission_fini(guc);
> }
More information about the Intel-gfx-trybot
mailing list