|
-------- Forwarded Message --------
On 11/12/2017 08:21 AM, Michal Wajdeczko wrote: > On Sat, 11 Nov 2017 01:06:32 +0100, Sujaritha Sundaresan > <sujaritha.sundaresan@xxxxxxxxx> 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. >> >> 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) >> >> Suggested by; Oscar Mateo <oscar.mateo@xxxxxxxxx> >> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@xxxxxxxxx> >> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx> >> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> >> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> >> Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx> >> Cc: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx> >> --- >> 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 | 59 >> ++++++++++++++++------------------ >> 7 files changed, 35 insertions(+), 44 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h >> index c94f34f..798fa8a 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -3219,10 +3219,13 @@ 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_UCODE(dev_priv) ((dev_priv)->guc.fw.path != NULL) >> +#define HAS_HUC_UCODE(dev_priv) ((dev_priv)->huc.fw.path != NULL) > > Btw, can we avoid adding ucode as alias for firmware and just use fw: > > #define HAS_GUC_FW(dev_priv) ((dev_priv)->guc.fw.path != NULL) > #define HAS_HUC_FW(dev_priv) ((dev_priv)->huc.fw.path != NULL) > > Also, maybe we should rather rely on fetch status instead of path: > > #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_FW(dev_priv) \ >> + (HAS_GUC(dev_priv) && i915_modparams.enable_guc_submission) > > With updated earlier macro names, our new macro can be defined as: > > #define NEEDS_GUC_LOAD(dev_priv) \ > (HAS_GUC(dev_priv) && \ > HAS_GUC_FW(dev_priv) && \ > ( HAS_HUC_FW(dev_priv) || i915_modparams.enable_guc_submission ) ) > I will make these macro changes. >> #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 c05c3d7..6a819c0 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_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 1e40eeb..b634edf 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c >> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c >> @@ -3476,7 +3476,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 ff00e46..a414bca 100644 >> --- a/drivers/gpu/drm/i915/i915_irq.c >> +++ b/drivers/gpu/drm/i915/i915_irq.c >> @@ -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..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 e85b268..648e59c 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 version */ >> if (!HAS_GUC(dev_priv)) { >> - if (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_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); >> - >> - /* 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; >> + /* Verify Firmware version */ >> + if (!HAS_HUC_UCODE(dev_priv)) { > > s/HUC/GUC as we rather should check for GuC fw code here > >> + 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 (i915_modparams.enable_guc_submission < 0) { >> + i915_modparams.enable_guc_submission = 0; >> + return; >> + } > > Hmm, maybe we combine both above cases into: > > if (!HAS_GUC_UCODE(dev_priv)) { > if (i915_modparams.enable_guc_submission > 0) { > DRM_INFO("Ignoring option %s - %s\n", > "enable_guc_submission", > HAS_GUC(dev_priv) ? > "no firmware" : "no hardware"); > With the new macro definitions as suggested earlier, I should be able to for FW with the earlier conditions. >> } >> - /* 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) >> + */ > > Hmm, this comment is little misleading, as we don't have any per > platform flag > for enabling GuC submission. For now it will be always enabled or > disabled... > >> - /* 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_submission == 1) >> + i915_modparams.enable_guc_submission = HAS_GUC(dev_priv); > > Hmm, HAS_GUC is always 1 at this point, so above looks like NOP Will do. > >> } >> 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) Thanks for the review, Regards, Sujaritha |
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
