[Intel-gfx] [PATCH v10 1/2] drm/i915/guc : Removing enable_guc_loading and enable_guc_submission module parameters
Sagar Arun Kamble
sagar.a.kamble at intel.com
Tue Nov 28 10:41:57 UTC 2017
On 11/28/2017 1:24 AM, Sujaritha Sundaresan 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).
>
> The above module parameters are being replaced by a single
> enable_guc modparam.
>
> 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)
>
> v9: Applying review comments (Michal)
>
> v10: Introducing enable_guc modparam
> Applying review comments (Michal)
>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> 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 | 6 +--
> drivers/gpu/drm/i915/i915_drv.h | 12 +++--
> 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 | 4 +-
> 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 | 6 +--
> drivers/gpu/drm/i915/intel_uc.c | 96 ++++++++++++++++++++++----------------
> 10 files changed, 77 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index cb3e5aa..c12452d 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2360,7 +2360,7 @@ static int i915_huc_load_status_info(struct seq_file *m, void *data)
> struct drm_i915_private *dev_priv = node_to_i915(m->private);
> struct drm_printer p;
>
> - if (!HAS_HUC_UCODE(dev_priv)) {
> + if (!HAS_HUC(dev_priv)) {
> seq_puts(m, "not supported\n");
> return 0;
> }
> @@ -2381,7 +2381,7 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
> struct drm_printer p;
> u32 tmp, i;
>
> - if (!HAS_GUC_UCODE(dev_priv)) {
> + if (!HAS_GUC(dev_priv)) {
> seq_puts(m, "not supported\n");
> return 0;
> }
> @@ -2466,7 +2466,7 @@ static bool check_guc_submission(struct seq_file *m)
> seq_printf(m, "GuC submission %s\n",
> HAS_GUC(dev_priv) ?
This HAS_GUC check is not present in drm-tip. Please rebase the patch.
> "not supported" :
> - HAS_GUC_SCHED(dev_priv) ?
> + NEEDS_GUC_LOAD(dev_priv) ?
> "disabled" :
> "failed");
> return false;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2b76625..c4e1c7e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3220,10 +3220,16 @@ static inline unsigned int i915_sg_segment_size(void)
> * properties, so we have separate macros to test them.
> */
> #define HAS_GUC(dev_priv) ((dev_priv)->info.has_guc)
> +#define HAS_HUC(dev_priv) (HAS_GUC(dev_priv))
> #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_FW(dev_priv) \
> + ((dev_priv)->guc.fw.fetch_status == INTEL_UC_FIRMWARE_SUCCESS)
> +#define HAS_HUC_FW(dev_priv) \
> + ((dev_priv)->huc.fw.fetch_status == INTEL_UC_FIRMWARE_SUCCESS)
> +
> +#define NEEDS_GUC_LOAD(dev_priv) \
> + (HAS_GUC(dev_priv) && HAS_GUC_FW(dev_priv) && \
> + (HAS_HUC_FW(dev_priv) || i915_modparams.enable_guc))
Indentation does not look right. A space needed before "(HAS_HUC_FW*"
>
> #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 c1efbaf..f9240dd 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -316,7 +316,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_LOAD(dev_priv))
> ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
> else
> ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
> @@ -409,7 +409,7 @@ struct i915_gem_context *
> 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 (!i915_modparams.enable_guc)
This case applies only if GuC is being used for submission. So we should
check
(enable_guc & 1).
Defining macro for 1, 2 will be good. something like:
#define NEEDS_GUC_SUBMISSION BIT(0)
#define NEEDS_HUC_LOADING BIT(1)
> 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 f92a39f..9892239 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 (NEEDS_GUC_LOAD(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..e3b6fe8 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1400,7 +1400,7 @@ static void snb_gt_irq_handler(struct drm_i915_private *dev_priv,
>
> if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) {
> notify_ring(engine);
> - tasklet |= i915_modparams.enable_guc_submission;
> + tasklet |= i915_modparams.enable_guc;
Also here.
> }
>
> if (tasklet)
> @@ -4032,7 +4032,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..6a5ce47 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -162,13 +162,10 @@ 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)");
> +i915_param_named_unsafe(enable_guc, int, 0400,
> + "Enable GuC submission and loading"
How about "Enable GuC load for submission and/or HuC load"
> + "(-1=auto [default], 0=disable GuC loading, 1=enable GuC submision"
> + "2=enable HuC, 3=enable GuC submission and HuC)");
>
> 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 c729226..7bf4dce 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -44,8 +44,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, -1) \
> 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 823d0c2..629ef5d 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -128,7 +128,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 (i915_modparams.enable_guc) {
This check should check only submission bit from this parameter.
> 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..6d7823b 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -505,7 +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 ||
> + if (!HAS_GUC_FW(dev_priv) ||
> (i915_modparams.guc_log_level < 0))
> return;
>
> @@ -646,7 +646,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 ||
> + if (!HAS_GUC_FW(dev_priv) ||
> (i915_modparams.guc_log_level < 0))
> return;
>
> @@ -657,7 +657,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 (!HAS_GUC_FW(dev_priv))
> return;
>
> mutex_lock(&dev_priv->drm.struct_mutex);
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 1e2a30a..233f680 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -47,38 +47,45 @@ static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv)
> return ret;
> }
>
> +/*
> + * With enable_guc defined as follows:
> + *
> + * -1=auto [default]
> + * 0=disable GuC loading
> + * 1=enable GuC submission
> + * 2=enable HuC
> + * 3=enable GuC submission and HuC
> + */
> +
> void intel_uc_sanitize_options(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");
> -
> - i915_modparams.enable_guc_loading = 0;
> - i915_modparams.enable_guc_submission = 0;
> - return;
> + int auto_enable_guc = 0;
> + // note that in the future this can be defined in more granular way
> + // int auto_enable_guc = !HAS_GUC_FW(dev_priv) ? 0 :
> + // IS_GEN9(dev_priv) ? 1 :
> + // IS_GEN10(dev_priv) ? 2:
> + // 3;
Change the comment to comply with linux multi-line comment format.
> +
> + /* Making sure that our auto is well defined */
> + GEM_BUG_ON(auto_enable_guc < 0);
> + GEM_BUG_ON((auto_enable_guc > 0) && !HAS_GUC_FW(dev_priv));
> + GEM_BUG_ON((auto_enable_guc & 2) && !HAS_HUC_FW(dev_priv));
> +
> + if (i915_modparams.enable_guc < 0)
> + i915_modparams.enable_guc = auto_enable_guc;
> +
> + if (i915_modparams.enable_guc > 0) {
> + if (!HAS_GUC(dev_priv) || !HAS_GUC_FW(dev_priv)) {
> + DRM_INFO("Ignoring option enable_guc=%d - %s\n",
> + i915_modparams.enable_guc,
> + !HAS_GUC(dev_priv) ? "no GuC hardware" :
> + "no GuC firmware");
> + i915_modparams.enable_guc = 0;
> + }
> }
>
> - /* 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 (intel_guc_fw_select(&dev_priv->guc))
> - i915_modparams.enable_guc_loading = 0;
> + if (i915_modparams.enable_guc & 2) {
> + if (!HAS_HUC_FW(dev_priv)) {
> + DRM_INFO("Ignoring option enable_guc=%d - %s\n",
> + i915_modparams.enable_guc, "no HuC firmware");
> + i915_modparams.enable_guc = 0;
Clearing only HuC status from enable_guc would be proper I guess. Sorry
I did not see this earlier.
> + }
> }
> -
> - /* 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" */
> - if (i915_modparams.enable_guc_submission < 0)
> - i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> }
>
> void intel_uc_init_early(struct drm_i915_private *dev_priv)
> @@ -154,7 +161,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_LOAD(dev_priv))
> return 0;
>
> guc_disable_communication(guc);
> @@ -163,7 +170,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 (i915_modparams.enable_guc) {
only NEEDS_GUC_SUBMISSION check? and similarly during fini
> /*
> * This is stuff we need to have available at fw load time
> * if we are planning to enable submission later
> @@ -213,7 +220,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> goto err_log_capture;
>
> intel_huc_auth(&dev_priv->huc);
> - if (i915_modparams.enable_guc_submission) {
> + if (i915_modparams.enable_guc) {
> if (i915_modparams.guc_log_level >= 0)
> gen9_enable_guc_interrupts(dev_priv);
We need to enable GuC interrupts if enable_guc > 0
but need to enable submission only if (enable_guc & 1)
>
> @@ -223,7 +230,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> }
>
> dev_info(dev_priv->drm.dev, "GuC %s (firmware %s [version %u.%u])\n",
> - i915_modparams.enable_guc_submission ? "submission enabled" :
> + i915_modparams.enable_guc ? "submission enabled" :
> "loaded",
This condition also needs proper check and messages.
Move the load message to intel_uc_init_hw and only keep submission here.
> guc->fw.path,
> guc->fw.major_ver_found, guc->fw.minor_ver_found);
> @@ -245,27 +252,21 @@ 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 (i915_modparams.enable_guc)
> 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) {
> + if (i915_modparams.enable_guc > 1) {
> DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
> ret = -EIO;
> + } else if (i915_modparams.enable_guc == 1) {
> + DRM_NOTE("Falling back from GuC submission to execlist mode\n");
Not falling back to execlists mode if enable_guc=3? why?
We can't have -EIO now as the option of "GuC is required" is not present
anymore.
> + 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;
> }
>
> @@ -275,15 +276,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 (!NEEDS_GUC_LOAD(dev_priv))
> return;
>
> - if (i915_modparams.enable_guc_submission)
> + if (i915_modparams.enable_guc)
Add GuC submission check.
> intel_guc_submission_disable(guc);
>
> guc_disable_communication(guc);
>
> - if (i915_modparams.enable_guc_submission) {
> + if (i915_modparams.enable_guc) {
> gen9_disable_guc_interrupts(dev_priv);
> intel_guc_submission_fini(guc);
> }
More information about the Intel-gfx
mailing list