| Message ID | 20170224154004.17821-8-arkadiusz.hiler@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 24, 2017 at 04:40:01PM +0100, Arkadiusz Hiler wrote: > Current version of intel_guc_init_hw() does a lot: > - cares about submission > - loads huc > - implement WA > > This change offloads some of the logic to intel_uc_init_hw(), which now > cares about the above. > > v2: rename guc_hw_reset and fix typo in define name (M. Wajdeczko) > v3: rename once again > v4: remove spurious comments and add some style (J. Lahtinen) > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > Cc: Michal Winiarski <michal.winiarski@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 2 +- > drivers/gpu/drm/i915/intel_guc_loader.c | 144 ++++---------------------------- > drivers/gpu/drm/i915/intel_uc.c | 110 ++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_uc.h | 3 + > 4 files changed, 128 insertions(+), 131 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 5b36524..8943c4e 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4452,7 +4452,7 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv) > intel_mocs_init_l3cc_table(dev_priv); > > /* We can't enable contexts until all firmware is loaded */ > - ret = intel_guc_init_hw(dev_priv); > + ret = intel_uc_init_hw(dev_priv); > if (ret) > goto out; > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index 5bd1e4a..8e9a2e29 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -90,7 +90,7 @@ const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status) > } > }; > > -static void guc_interrupts_release(struct drm_i915_private *dev_priv) > +void intel_guc_interrupts_release(struct drm_i915_private *dev_priv) Hmm, maybe better name for this function would be "intel_release_guc_interrupts" ? Then it will match your subject-verb-object pattern. > { > struct intel_engine_cs *engine; > enum intel_engine_id id; > @@ -108,7 +108,7 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv) > I915_WRITE(GUC_WD_VECS_IER, 0); > } > > -static void guc_interrupts_capture(struct drm_i915_private *dev_priv) > +void intel_guc_interrupts_capture(struct drm_i915_private *dev_priv) > { > struct intel_engine_cs *engine; > enum intel_engine_id id; > @@ -408,24 +408,6 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv) > return ret; > } > > -static int guc_hw_reset(struct drm_i915_private *dev_priv) > -{ > - int ret; > - u32 guc_status; > - > - ret = intel_guc_reset(dev_priv); > - if (ret) { > - DRM_ERROR("GuC reset failed, ret = %d\n", ret); > - return ret; > - } > - > - guc_status = I915_READ(GUC_STATUS); > - WARN(!(guc_status & GS_MIA_IN_RESET), > - "GuC status: 0x%x, MIA core expected to be in reset\n", guc_status); > - > - return ret; > -} > - > /** > * intel_guc_init_hw() - finish preparing the GuC for activity > * @dev_priv: i915 device private > @@ -443,42 +425,24 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv) > { > struct intel_uc_fw *guc_fw = &dev_priv->guc.fw; > const char *fw_path = guc_fw->path; > - int retries, ret, err; > + int ret; > > DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n", > fw_path, > intel_uc_fw_status_repr(guc_fw->fetch_status), > intel_uc_fw_status_repr(guc_fw->load_status)); > > - /* Loading forbidden, or no firmware to load? */ > - if (!i915.enable_guc_loading) { > - err = 0; > - goto fail; > - } else if (fw_path == NULL) { > - /* Device is known to have no uCode (e.g. no GuC) */ > - err = -ENXIO; > - goto fail; > + if (!fw_path) { > + return -ENXIO; > } else if (*fw_path == '\0') { Hmm, is this case still possible? > - /* Device has a GuC but we don't know what f/w to load? */ > WARN(1, "No GuC firmware known for this platform!\n"); > - err = -ENODEV; > - goto fail; > + return -ENODEV; > } > > - /* Fetch failed, or already fetched but failed to load? */ > - if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS) { > - err = -EIO; > - goto fail; > - } else if (guc_fw->load_status == INTEL_UC_FIRMWARE_FAIL) { > - err = -ENOEXEC; > - goto fail; > - } > - > - guc_interrupts_release(dev_priv); > - gen9_reset_guc_interrupts(dev_priv); > - > - /* We need to notify the guc whenever we change the GGTT */ > - i915_ggtt_enable_guc(dev_priv); > + if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS) > + return -EIO; > + else if (guc_fw->load_status == INTEL_UC_FIRMWARE_FAIL) > + return -ENOEXEC; Hmm, it looks like you're checking for load failure here, but actual load is about to start below ? Did I missed something ? > > guc_fw->load_status = INTEL_UC_FIRMWARE_PENDING; I guess this status can be set in guc_ucode_xfer() as it uses guc_fw object. > > @@ -486,104 +450,24 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv) > intel_uc_fw_status_repr(guc_fw->fetch_status), > intel_uc_fw_status_repr(guc_fw->load_status)); > > - err = i915_guc_submission_init(dev_priv); > - if (err) > - goto fail; > - > /* > * WaEnableuKernelHeaderValidFix:skl,bxt > * For BXT, this is only upto B0 but below WA is required for later > * steppings also so this is extended as well. > */ Rebase issue? > - /* WaEnableGuCBootHashCheckNotSet:skl,bxt */ > - for (retries = 3; ; ) { > - /* > - * Always reset the GuC just before (re)loading, so > - * that the state and timing are fairly predictable > - */ > - err = guc_hw_reset(dev_priv); > - if (err) > - goto fail; > + ret = guc_ucode_xfer(dev_priv); > > - intel_huc_init_hw(dev_priv); > - err = guc_ucode_xfer(dev_priv); > - if (!err) > - break; > - > - if (--retries == 0) > - goto fail; > - > - DRM_INFO("GuC fw load failed: %d; will reset and " > - "retry %d more time(s)\n", err, retries); > - } > + if (ret) > + return -EAGAIN; > > guc_fw->load_status = INTEL_UC_FIRMWARE_SUCCESS; > > - intel_guc_auth_huc(dev_priv); > - > - if (i915.enable_guc_submission) { > - if (i915.guc_log_level >= 0) > - gen9_enable_guc_interrupts(dev_priv); > - > - err = i915_guc_submission_enable(dev_priv); > - if (err) > - goto fail; > - guc_interrupts_capture(dev_priv); > - } > - > DRM_INFO("GuC %s (firmware %s [version %u.%u])\n", > i915.enable_guc_submission ? "submission enabled" : "loaded", > guc_fw->path, > guc_fw->major_ver_found, guc_fw->minor_ver_found); > > return 0; > - > -fail: > - if (guc_fw->load_status == INTEL_UC_FIRMWARE_PENDING) > - guc_fw->load_status = INTEL_UC_FIRMWARE_FAIL; > - > - guc_interrupts_release(dev_priv); > - i915_guc_submission_disable(dev_priv); > - i915_guc_submission_fini(dev_priv); > - i915_ggtt_disable_guc(dev_priv); > - > - /* > - * 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). > - */ > - if (i915.enable_guc_loading > 1) { > - ret = -EIO; > - } else if (i915.enable_guc_submission > 1) { > - ret = -EIO; > - } else { > - ret = 0; > - } > - > - if (err == 0 && !HAS_GUC_UCODE(dev_priv)) > - ; /* Don't mention the GuC! */ > - else if (err == 0) > - DRM_INFO("GuC firmware load skipped\n"); > - else if (ret != -EIO) > - DRM_NOTE("GuC firmware load failed: %d\n", err); > - else > - DRM_WARN("GuC firmware load failed: %d\n", err); > - > - if (i915.enable_guc_submission) { > - if (fw_path == NULL) > - DRM_INFO("GuC submission without firmware not supported\n"); > - if (ret == 0) > - DRM_NOTE("Falling back from GuC submission to execlist mode\n"); > - else > - DRM_ERROR("GuC init failed: %d\n", ret); > - } > - i915.enable_guc_submission = 0; > - > - return ret; > } > > > @@ -644,7 +528,7 @@ void intel_guc_fini(struct drm_i915_private *dev_priv) > struct drm_i915_gem_object *obj; > > mutex_lock(&dev_priv->drm.struct_mutex); > - guc_interrupts_release(dev_priv); > + intel_guc_interrupts_release(dev_priv); > i915_guc_submission_disable(dev_priv); > i915_guc_submission_fini(dev_priv); > mutex_unlock(&dev_priv->drm.struct_mutex); > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 8f5aa58..709f964 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -26,6 +26,27 @@ > #include "intel_uc.h" > #include <linux/firmware.h> > > +/* Reset GuC providing us with fresh state for both GuC and HuC. > + */ > +static int __uc_hw_reset(struct drm_i915_private *dev_priv) Hmm, maybe "intel_uc_reset_hw" to match other function names? > +{ > + int ret; > + u32 guc_status; > + > + ret = intel_guc_reset(dev_priv); > + if (ret) { > + DRM_ERROR("GuC reset failed, ret = %d\n", ret); > + return ret; > + } > + > + guc_status = I915_READ(GUC_STATUS); > + WARN(!(guc_status & GS_MIA_IN_RESET), > + "GuC status: 0x%x, MIA core expected to be in reset\n", > + guc_status); > + > + return ret; > +} > + > void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) > { > if (!HAS_GUC(dev_priv)) { > @@ -63,6 +84,92 @@ void intel_uc_init_fw(struct drm_i915_private *dev_priv) > intel_guc_init_fw(&dev_priv->guc); > } > > +int intel_uc_init_hw(struct drm_i915_private *dev_priv) > +{ > + int ret, retries; > + > + /* guc not enabled, nothing to do */ > + if (!i915.enable_guc_loading) > + return 0; > + > + intel_guc_interrupts_release(dev_priv); > + gen9_reset_guc_interrupts(dev_priv); > + > + /* We need to notify the guc whenever we change the GGTT */ > + i915_ggtt_enable_guc(dev_priv); > + > + if (i915.enable_guc_submission) { > + ret = i915_guc_submission_init(dev_priv); > + if (ret) > + goto fail; > + } > + > + /* WaEnableGuCBootHashCheckNotSet:skl,bxt */ > + retries = GUC_WA_HASH_CHECK_NOT_SET_ATTEMPTS; > + while (retries--) { > + /* > + * Always reset the GuC just before (re)loading, so > + * that the state and timing are fairly predictable > + */ > + ret = __uc_hw_reset(dev_priv); > + if (ret) > + goto fail; > + > + intel_huc_init_hw(dev_priv); > + ret = intel_guc_init_hw(dev_priv); > + if (ret == 0 || ret != -EAGAIN) > + break; > + > + DRM_INFO("GuC fw load failed: %d; will reset and " > + "retry %d more time(s)\n", ret, retries); > + } > + > + /* did we succeded or run out of retries? */ > + if (ret) > + goto fail; > + > + intel_guc_auth_huc(dev_priv); > + if (i915.enable_guc_submission) { > + if (i915.guc_log_level >= 0) > + gen9_enable_guc_interrupts(dev_priv); > + > + ret = i915_guc_submission_enable(dev_priv); > + if (ret) > + goto fail; > + intel_guc_interrupts_capture(dev_priv); > + } > + > + return 0; > + > +fail: > + /* > + * 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). > + */ > + DRM_ERROR("GuC init failed\n"); > + if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1) Nonzero i915.enable_guc_loading is guaranteed (see your first "if" in this function) > + ret = -EIO; > + else > + ret = 0; > + > + if (i915.enable_guc_submission) { > + i915.enable_guc_submission = 0; > + DRM_NOTE("Falling back from GuC submission to execlist mode\n"); > + } > + > + i915_ggtt_disable_guc(dev_priv); > + intel_guc_interrupts_release(dev_priv); > + i915_guc_submission_disable(dev_priv); > + i915_guc_submission_fini(dev_priv); > + > + return ret; > +} > + > /* > * Read GuC command/status register (SOFT_SCRATCH_0) > * Return true if it contains a response rather than a command > @@ -76,6 +183,9 @@ static bool intel_guc_recv(struct intel_guc *guc, u32 *status) > return INTEL_GUC_RECV_IS_RESPONSE(val); > } > > +/* WaEnableGuCBootHashCheckNotSet:skl,bxt */ > +#define GUC_WA_HASH_CHECK_NOT_SET_ATTEMPTS 3 > + Hmm, is it worth to define this macro here as it is used only once and it is very specific ? Do you plan to define more similar macros? -Michal > int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > index f8d8816..d19c95e 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -187,6 +187,7 @@ struct intel_huc { > void intel_uc_sanitize_options(struct drm_i915_private *dev_priv); > void intel_uc_init_early(struct drm_i915_private *dev_priv); > void intel_uc_init_fw(struct drm_i915_private *dev_priv); > +int intel_uc_init_hw(struct drm_i915_private *dev_priv); > int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len); > int intel_guc_sample_forcewake(struct intel_guc *guc); > void intel_uc_prepare_fw(struct drm_i915_private *dev_priv, > @@ -201,6 +202,8 @@ void intel_uc_prepare_fw(struct drm_i915_private *dev_priv, > struct intel_uc_fw *uc_fw); > int intel_guc_suspend(struct drm_i915_private *dev_priv); > int intel_guc_resume(struct drm_i915_private *dev_priv); > +void intel_guc_interrupts_release(struct drm_i915_private *dev_priv); > +void intel_guc_interrupts_capture(struct drm_i915_private *dev_priv); > u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv); > > /* i915_guc_submission.c */ > -- > 2.9.3 >
On Fri, Feb 24, 2017 at 06:26:10PM +0100, Michal Wajdeczko wrote: > On Fri, Feb 24, 2017 at 04:40:01PM +0100, Arkadiusz Hiler wrote: > > Current version of intel_guc_init_hw() does a lot: > > - cares about submission > > - loads huc > > - implement WA > > > > This change offloads some of the logic to intel_uc_init_hw(), which now > > cares about the above. > > > > v2: rename guc_hw_reset and fix typo in define name (M. Wajdeczko) > > v3: rename once again > > v4: remove spurious comments and add some style (J. Lahtinen) > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > > Cc: Michal Winiarski <michal.winiarski@intel.com> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 2 +- > > drivers/gpu/drm/i915/intel_guc_loader.c | 144 ++++---------------------------- > > drivers/gpu/drm/i915/intel_uc.c | 110 ++++++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_uc.h | 3 + > > 4 files changed, 128 insertions(+), 131 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 5b36524..8943c4e 100644 > > @@ -443,42 +425,24 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv) > > { > <SNIP> > > - /* Loading forbidden, or no firmware to load? */ > > - if (!i915.enable_guc_loading) { > > - err = 0; > > - goto fail; > > - } else if (fw_path == NULL) { > > - /* Device is known to have no uCode (e.g. no GuC) */ > > - err = -ENXIO; > > - goto fail; > > + if (!fw_path) { > > + return -ENXIO; > > } else if (*fw_path == '\0') { > > Hmm, is this case still possible? In this revision - yes, but unlikely. HAS_GUC() is true but firmware for the device is not known (see init_fw()). I think that the initial flow was designed in this case in mind. > > - /* Device has a GuC but we don't know what f/w to load? */ > > WARN(1, "No GuC firmware known for this platform!\n"); > > - err = -ENODEV; > > - goto fail; > > + return -ENODEV; > > } > > > > - /* Fetch failed, or already fetched but failed to load? */ > > - if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS) { > > - err = -EIO; > > - goto fail; > > - } else if (guc_fw->load_status == INTEL_UC_FIRMWARE_FAIL) { > > - err = -ENOEXEC; > > - goto fail; > > - } > > - > > - guc_interrupts_release(dev_priv); > > - gen9_reset_guc_interrupts(dev_priv); > > - > > - /* We need to notify the guc whenever we change the GGTT */ > > - i915_ggtt_enable_guc(dev_priv); > > + if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS) > > + return -EIO; > > + else if (guc_fw->load_status == INTEL_UC_FIRMWARE_FAIL) > > + return -ENOEXEC; > > Hmm, it looks like you're checking for load failure here, but actual > load is about to start below ? Did I missed something ? The status FIRMWARE_FAIL is not used at all in the GuC path (HuC) uses it. Noted down to give it a closer look. The check is gone, as it does not make sense, thanks. > > guc_fw->load_status = INTEL_UC_FIRMWARE_PENDING; > > I guess this status can be set in guc_ucode_xfer() as it uses guc_fw object. > > > > @@ -486,104 +450,24 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv) > > intel_uc_fw_status_repr(guc_fw->fetch_status), > > intel_uc_fw_status_repr(guc_fw->load_status)); > > > > - err = i915_guc_submission_init(dev_priv); > > - if (err) > > - goto fail; > > - > > /* > > * WaEnableuKernelHeaderValidFix:skl,bxt > > * For BXT, this is only upto B0 but below WA is required for later > > * steppings also so this is extended as well. > > */ > > Rebase issue? Yeah. I've streamlined the whole thing to be: ---------------------------------------------------------- int attempts; const int guc_wa_hash_check_not_set_attempts = 3; <SNIP> /* WaEnableuKernelHeaderValidFix:skl */ /* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */ if (IS_GEN9(dev_priv)) attempts = guc_wa_hash_check_not_set_attempts; else attempts = 1; while (attempts--) ... ---------------------------------------------------------- and dropped the WA comments in the guc_init_hw() > > +int intel_uc_init_hw(struct drm_i915_private *dev_priv) > > +{ > <SNIP> > > +fail: > > + /* > > + * 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). > > + */ > > + DRM_ERROR("GuC init failed\n"); > > + if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1) > > Nonzero i915.enable_guc_loading is guaranteed (see your first "if" in this function) That's why this not a check for "non-zeroness". It's cheeking for "greater than one", adhering to existing behavior: 0 = disabled 1 = enabled with possible fallback (i.e. ignore the error and use execlists) > 1 = wedge if this fail > > +/* WaEnableGuCBootHashCheckNotSet:skl,bxt */ > > +#define GUC_WA_HASH_CHECK_NOT_SET_ATTEMPTS 3 > > + > > Hmm, is it worth to define this macro here as it is used only once and > it is very specific ? Do you plan to define more similar macros? Changed to const, but i915 seems to be generally define-biased for similar things.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 5b36524..8943c4e 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4452,7 +4452,7 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv) intel_mocs_init_l3cc_table(dev_priv); /* We can't enable contexts until all firmware is loaded */ - ret = intel_guc_init_hw(dev_priv); + ret = intel_uc_init_hw(dev_priv); if (ret) goto out; diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 5bd1e4a..8e9a2e29 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -90,7 +90,7 @@ const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status) } }; -static void guc_interrupts_release(struct drm_i915_private *dev_priv) +void intel_guc_interrupts_release(struct drm_i915_private *dev_priv) { struct intel_engine_cs *engine; enum intel_engine_id id; @@ -108,7 +108,7 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv) I915_WRITE(GUC_WD_VECS_IER, 0); } -static void guc_interrupts_capture(struct drm_i915_private *dev_priv) +void intel_guc_interrupts_capture(struct drm_i915_private *dev_priv) { struct intel_engine_cs *engine; enum intel_engine_id id; @@ -408,24 +408,6 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv) return ret; } -static int guc_hw_reset(struct drm_i915_private *dev_priv) -{ - int ret; - u32 guc_status; - - ret = intel_guc_reset(dev_priv); - if (ret) { - DRM_ERROR("GuC reset failed, ret = %d\n", ret); - return ret; - } - - guc_status = I915_READ(GUC_STATUS); - WARN(!(guc_status & GS_MIA_IN_RESET), - "GuC status: 0x%x, MIA core expected to be in reset\n", guc_status); - - return ret; -} - /** * intel_guc_init_hw() - finish preparing the GuC for activity * @dev_priv: i915 device private @@ -443,42 +425,24 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv) { struct intel_uc_fw *guc_fw = &dev_priv->guc.fw; const char *fw_path = guc_fw->path; - int retries, ret, err; + int ret; DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n", fw_path, intel_uc_fw_status_repr(guc_fw->fetch_status), intel_uc_fw_status_repr(guc_fw->load_status)); - /* Loading forbidden, or no firmware to load? */ - if (!i915.enable_guc_loading) { - err = 0; - goto fail; - } else if (fw_path == NULL) { - /* Device is known to have no uCode (e.g. no GuC) */ - err = -ENXIO; - goto fail; + if (!fw_path) { + return -ENXIO; } else if (*fw_path == '\0') { - /* Device has a GuC but we don't know what f/w to load? */ WARN(1, "No GuC firmware known for this platform!\n"); - err = -ENODEV; - goto fail; + return -ENODEV; } - /* Fetch failed, or already fetched but failed to load? */ - if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS) { - err = -EIO; - goto fail; - } else if (guc_fw->load_status == INTEL_UC_FIRMWARE_FAIL) { - err = -ENOEXEC; - goto fail; - } - - guc_interrupts_release(dev_priv); - gen9_reset_guc_interrupts(dev_priv); - - /* We need to notify the guc whenever we change the GGTT */ - i915_ggtt_enable_guc(dev_priv); + if (guc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS) + return -EIO; + else if (guc_fw->load_status == INTEL_UC_FIRMWARE_FAIL) + return -ENOEXEC; guc_fw->load_status = INTEL_UC_FIRMWARE_PENDING; @@ -486,104 +450,24 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv) intel_uc_fw_status_repr(guc_fw->fetch_status), intel_uc_fw_status_repr(guc_fw->load_status)); - err = i915_guc_submission_init(dev_priv); - if (err) - goto fail; - /* * WaEnableuKernelHeaderValidFix:skl,bxt * For BXT, this is only upto B0 but below WA is required for later * steppings also so this is extended as well. */ - /* WaEnableGuCBootHashCheckNotSet:skl,bxt */ - for (retries = 3; ; ) { - /* - * Always reset the GuC just before (re)loading, so - * that the state and timing are fairly predictable - */ - err = guc_hw_reset(dev_priv); - if (err) - goto fail; + ret = guc_ucode_xfer(dev_priv); - intel_huc_init_hw(dev_priv); - err = guc_ucode_xfer(dev_priv); - if (!err) - break; - - if (--retries == 0) - goto fail; - - DRM_INFO("GuC fw load failed: %d; will reset and " - "retry %d more time(s)\n", err, retries); - } + if (ret) + return -EAGAIN; guc_fw->load_status = INTEL_UC_FIRMWARE_SUCCESS; - intel_guc_auth_huc(dev_priv); - - if (i915.enable_guc_submission) { - if (i915.guc_log_level >= 0) - gen9_enable_guc_interrupts(dev_priv); - - err = i915_guc_submission_enable(dev_priv); - if (err) - goto fail; - guc_interrupts_capture(dev_priv); - } - DRM_INFO("GuC %s (firmware %s [version %u.%u])\n", i915.enable_guc_submission ? "submission enabled" : "loaded", guc_fw->path, guc_fw->major_ver_found, guc_fw->minor_ver_found); return 0; - -fail: - if (guc_fw->load_status == INTEL_UC_FIRMWARE_PENDING) - guc_fw->load_status = INTEL_UC_FIRMWARE_FAIL; - - guc_interrupts_release(dev_priv); - i915_guc_submission_disable(dev_priv); - i915_guc_submission_fini(dev_priv); - i915_ggtt_disable_guc(dev_priv); - - /* - * 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). - */ - if (i915.enable_guc_loading > 1) { - ret = -EIO; - } else if (i915.enable_guc_submission > 1) { - ret = -EIO; - } else { - ret = 0; - } - - if (err == 0 && !HAS_GUC_UCODE(dev_priv)) - ; /* Don't mention the GuC! */ - else if (err == 0) - DRM_INFO("GuC firmware load skipped\n"); - else if (ret != -EIO) - DRM_NOTE("GuC firmware load failed: %d\n", err); - else - DRM_WARN("GuC firmware load failed: %d\n", err); - - if (i915.enable_guc_submission) { - if (fw_path == NULL) - DRM_INFO("GuC submission without firmware not supported\n"); - if (ret == 0) - DRM_NOTE("Falling back from GuC submission to execlist mode\n"); - else - DRM_ERROR("GuC init failed: %d\n", ret); - } - i915.enable_guc_submission = 0; - - return ret; } @@ -644,7 +528,7 @@ void intel_guc_fini(struct drm_i915_private *dev_priv) struct drm_i915_gem_object *obj; mutex_lock(&dev_priv->drm.struct_mutex); - guc_interrupts_release(dev_priv); + intel_guc_interrupts_release(dev_priv); i915_guc_submission_disable(dev_priv); i915_guc_submission_fini(dev_priv); mutex_unlock(&dev_priv->drm.struct_mutex); diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 8f5aa58..709f964 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -26,6 +26,27 @@ #include "intel_uc.h" #include <linux/firmware.h> +/* Reset GuC providing us with fresh state for both GuC and HuC. + */ +static int __uc_hw_reset(struct drm_i915_private *dev_priv) +{ + int ret; + u32 guc_status; + + ret = intel_guc_reset(dev_priv); + if (ret) { + DRM_ERROR("GuC reset failed, ret = %d\n", ret); + return ret; + } + + guc_status = I915_READ(GUC_STATUS); + WARN(!(guc_status & GS_MIA_IN_RESET), + "GuC status: 0x%x, MIA core expected to be in reset\n", + guc_status); + + return ret; +} + void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) { if (!HAS_GUC(dev_priv)) { @@ -63,6 +84,92 @@ void intel_uc_init_fw(struct drm_i915_private *dev_priv) intel_guc_init_fw(&dev_priv->guc); } +int intel_uc_init_hw(struct drm_i915_private *dev_priv) +{ + int ret, retries; + + /* guc not enabled, nothing to do */ + if (!i915.enable_guc_loading) + return 0; + + intel_guc_interrupts_release(dev_priv); + gen9_reset_guc_interrupts(dev_priv); + + /* We need to notify the guc whenever we change the GGTT */ + i915_ggtt_enable_guc(dev_priv); + + if (i915.enable_guc_submission) { + ret = i915_guc_submission_init(dev_priv); + if (ret) + goto fail; + } + + /* WaEnableGuCBootHashCheckNotSet:skl,bxt */ + retries = GUC_WA_HASH_CHECK_NOT_SET_ATTEMPTS; + while (retries--) { + /* + * Always reset the GuC just before (re)loading, so + * that the state and timing are fairly predictable + */ + ret = __uc_hw_reset(dev_priv); + if (ret) + goto fail; + + intel_huc_init_hw(dev_priv); + ret = intel_guc_init_hw(dev_priv); + if (ret == 0 || ret != -EAGAIN) + break; + + DRM_INFO("GuC fw load failed: %d; will reset and " + "retry %d more time(s)\n", ret, retries); + } + + /* did we succeded or run out of retries? */ + if (ret) + goto fail; + + intel_guc_auth_huc(dev_priv); + if (i915.enable_guc_submission) { + if (i915.guc_log_level >= 0) + gen9_enable_guc_interrupts(dev_priv); + + ret = i915_guc_submission_enable(dev_priv); + if (ret) + goto fail; + intel_guc_interrupts_capture(dev_priv); + } + + return 0; + +fail: + /* + * 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). + */ + DRM_ERROR("GuC init failed\n"); + if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1) + ret = -EIO; + else + ret = 0; + + if (i915.enable_guc_submission) { + i915.enable_guc_submission = 0; + DRM_NOTE("Falling back from GuC submission to execlist mode\n"); + } + + i915_ggtt_disable_guc(dev_priv); + intel_guc_interrupts_release(dev_priv); + i915_guc_submission_disable(dev_priv); + i915_guc_submission_fini(dev_priv); + + return ret; +} + /* * Read GuC command/status register (SOFT_SCRATCH_0) * Return true if it contains a response rather than a command @@ -76,6 +183,9 @@ static bool intel_guc_recv(struct intel_guc *guc, u32 *status) return INTEL_GUC_RECV_IS_RESPONSE(val); } +/* WaEnableGuCBootHashCheckNotSet:skl,bxt */ +#define GUC_WA_HASH_CHECK_NOT_SET_ATTEMPTS 3 + int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len) { struct drm_i915_private *dev_priv = guc_to_i915(guc); diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h index f8d8816..d19c95e 100644 --- a/drivers/gpu/drm/i915/intel_uc.h +++ b/drivers/gpu/drm/i915/intel_uc.h @@ -187,6 +187,7 @@ struct intel_huc { void intel_uc_sanitize_options(struct drm_i915_private *dev_priv); void intel_uc_init_early(struct drm_i915_private *dev_priv); void intel_uc_init_fw(struct drm_i915_private *dev_priv); +int intel_uc_init_hw(struct drm_i915_private *dev_priv); int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len); int intel_guc_sample_forcewake(struct intel_guc *guc); void intel_uc_prepare_fw(struct drm_i915_private *dev_priv, @@ -201,6 +202,8 @@ void intel_uc_prepare_fw(struct drm_i915_private *dev_priv, struct intel_uc_fw *uc_fw); int intel_guc_suspend(struct drm_i915_private *dev_priv); int intel_guc_resume(struct drm_i915_private *dev_priv); +void intel_guc_interrupts_release(struct drm_i915_private *dev_priv); +void intel_guc_interrupts_capture(struct drm_i915_private *dev_priv); u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv); /* i915_guc_submission.c */
Current version of intel_guc_init_hw() does a lot: - cares about submission - loads huc - implement WA This change offloads some of the logic to intel_uc_init_hw(), which now cares about the above. v2: rename guc_hw_reset and fix typo in define name (M. Wajdeczko) v3: rename once again v4: remove spurious comments and add some style (J. Lahtinen) Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> Cc: Michal Winiarski <michal.winiarski@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 2 +- drivers/gpu/drm/i915/intel_guc_loader.c | 144 ++++---------------------------- drivers/gpu/drm/i915/intel_uc.c | 110 ++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_uc.h | 3 + 4 files changed, 128 insertions(+), 131 deletions(-)