| Message ID | 20161215154708.31521-4-arkadiusz.hiler@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 15, 2016 at 04:47:06PM +0100, Arkadiusz Hiler wrote: > Current version of intel_guc_load() does a lot: > - cares about submission > - loads huc > - implement WA > > This change offloads some of the logic to intel_uc_load(), which now > cares about the above. > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > Cc: Jeff McGee <jeff.mcgee@intel.com> > Cc: Michal Winiarski <michal.winiarski@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 | 126 +++++--------------------------- > drivers/gpu/drm/i915/intel_uc.c | 83 +++++++++++++++++++++ > drivers/gpu/drm/i915/intel_uc.h | 8 ++ > 4 files changed, 110 insertions(+), 109 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 6af4e85..76b52c6 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4412,7 +4412,7 @@ 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_load(dev_priv); > + ret = intel_uc_load(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 f8b28b1..b76b556 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -97,7 +97,7 @@ const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status) > } > }; > > -static void guc_interrupts_release(struct drm_i915_private *dev_priv) > +void guc_interrupts_release(struct drm_i915_private *dev_priv) > { > struct intel_engine_cs *engine; > enum intel_engine_id id; > @@ -115,7 +115,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 guc_interrupts_capture(struct drm_i915_private *dev_priv) > { > struct intel_engine_cs *engine; > enum intel_engine_id id; > @@ -334,7 +334,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv, > return ret; > } > > -static u32 guc_wopcm_size(struct drm_i915_private *dev_priv) > +u32 guc_wopcm_size(struct drm_i915_private *dev_priv) > { > u32 wopcm_size = GUC_WOPCM_TOP; > > @@ -417,7 +417,7 @@ 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 guc_hw_reset(struct drm_i915_private *dev_priv) > { > int ret; > u32 guc_status; > @@ -452,75 +452,37 @@ int intel_guc_load(struct drm_i915_private *dev_priv) > { > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > const char *fw_path = guc_fw->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_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), > intel_guc_fw_status_repr(guc_fw->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) { > + if (fw_path == NULL) { > /* Device is known to have no uCode (e.g. no GuC) */ > - err = -ENXIO; > - goto fail; > + 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->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) { > - err = -EIO; > - goto fail; > + return -EIO; > } else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) { > - err = -ENOEXEC; > - goto fail; > + return -ENOEXEC; > } > > - guc_interrupts_release(dev_priv); > - gen9_reset_guc_interrupts(dev_priv); > - > guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING; > > - DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n", > - intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), > - intel_guc_fw_status_repr(guc_fw->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; > + /* we may want to retry guc ucode transfer */ > + ret = guc_ucode_xfer(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->guc_fw_load_status = GUC_FIRMWARE_SUCCESS; > > @@ -528,63 +490,7 @@ int intel_guc_load(struct drm_i915_private *dev_priv) > intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), > intel_guc_fw_status_repr(guc_fw->guc_fw_load_status)); > > - 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); > - } > - > return 0; > - > -fail: > - if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING) > - guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL; > - > - guc_interrupts_release(dev_priv); > - i915_guc_submission_disable(dev_priv); > - i915_guc_submission_fini(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; > } > > static void guc_fw_fetch(struct drm_i915_private *dev_priv, > @@ -757,6 +663,10 @@ void intel_guc_init(struct drm_i915_private *dev_priv) > guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE; > guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE; > > + /* can't enable guc submission without guc */ > + if (!i915.enable_guc_loading) > + i915.enable_guc_submission = 0; > + > /* Early (and silent) return if GuC loading is disabled */ > if (!i915.enable_guc_loading) > return; > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 8eec035..4e184edb 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -35,6 +35,89 @@ void intel_uc_init(struct drm_i915_private *dev_priv) > intel_guc_init(dev_priv); > } > > +int intel_uc_load(struct drm_i915_private *dev_priv) > +{ > + struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > + int ret, retries; > + > + /* guc not enabled, nothing to do */ > + if (!i915.enable_guc_loading) > + return 0; > + > + guc_interrupts_release(dev_priv); > + gen9_reset_guc_interrupts(dev_priv); > + > + guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING; > + > + 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_ATTEPMTS; > + while (retries--) { > + /* > + * Always reset the GuC just before (re)loading, so > + * that the state and timing are fairly predictable > + */ > + ret = guc_hw_reset(dev_priv); > + if (ret) > + goto fail; > + > + ret = intel_guc_load(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; > + > + if (i915.enable_guc_submission) { > + if (i915.guc_log_level >= 0) > + gen9_enable_guc_interrupts(dev_priv); > + > + ret = i915_guc_submission_enable(dev_priv); Hmm, enabling guc submission in this function is still something more than one can expect from function name suggesting pure "load"... Do you plan to offload this function in the future ? Michal > + if (ret) > + goto fail; > + 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). > + */ > + 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_INFO("GuC submission without firmware not supported\n"); > + DRM_NOTE("Falling back from GuC submission to execlist mode\n"); > + } > + > + 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 > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > index ec1a5b2..89c7e1f 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -28,6 +28,9 @@ > #include "i915_guc_reg.h" > #include "intel_ringbuffer.h" > > +/* WaEnableGuCBootHashCheckNotSet:skl,bxt */ > +#define GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS 3 > + > struct drm_i915_gem_request; > > /* > @@ -171,6 +174,7 @@ struct intel_guc { > /* intel_uc.c */ > void intel_uc_init_early(struct drm_i915_private *dev_priv); > void intel_uc_init(struct drm_i915_private *dev_priv); > +int intel_uc_load(struct drm_i915_private *dev_priv); > bool intel_guc_recv(struct drm_i915_private *dev_priv, u32 *status); > int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len); > int intel_guc_sample_forcewake(struct intel_guc *guc); > @@ -185,6 +189,10 @@ extern void intel_guc_fini(struct drm_i915_private *dev_priv); > extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status); > extern int intel_guc_suspend(struct drm_i915_private *dev_priv); > extern int intel_guc_resume(struct drm_i915_private *dev_priv); > +void guc_interrupts_release(struct drm_i915_private *dev_priv); > +void guc_interrupts_capture(struct drm_i915_private *dev_priv); > +int guc_hw_reset(struct drm_i915_private *dev_priv); > +u32 guc_wopcm_size(struct drm_i915_private *dev_priv); > > /* i915_guc_submission.c */ > int i915_guc_submission_init(struct drm_i915_private *dev_priv); > -- > 2.9.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 15/12/16 07:47, Arkadiusz Hiler wrote: > Current version of intel_guc_load() does a lot: > - cares about submission > - loads huc > - implement WA > > This change offloads some of the logic to intel_uc_load(), which now > cares about the above. > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > Cc: Jeff McGee <jeff.mcgee@intel.com> > Cc: Michal Winiarski <michal.winiarski@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 | 126 +++++--------------------------- > drivers/gpu/drm/i915/intel_uc.c | 83 +++++++++++++++++++++ > drivers/gpu/drm/i915/intel_uc.h | 8 ++ > 4 files changed, 110 insertions(+), 109 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 6af4e85..76b52c6 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4412,7 +4412,7 @@ 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_load(dev_priv); > + ret = intel_uc_load(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 f8b28b1..b76b556 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -97,7 +97,7 @@ const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status) > } > }; > > -static void guc_interrupts_release(struct drm_i915_private *dev_priv) > +void guc_interrupts_release(struct drm_i915_private *dev_priv) > { > struct intel_engine_cs *engine; > enum intel_engine_id id; > @@ -115,7 +115,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 guc_interrupts_capture(struct drm_i915_private *dev_priv) > { > struct intel_engine_cs *engine; > enum intel_engine_id id; > @@ -334,7 +334,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv, > return ret; > } > > -static u32 guc_wopcm_size(struct drm_i915_private *dev_priv) > +u32 guc_wopcm_size(struct drm_i915_private *dev_priv) > { > u32 wopcm_size = GUC_WOPCM_TOP; > > @@ -417,7 +417,7 @@ 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 guc_hw_reset(struct drm_i915_private *dev_priv) If I haven't missed anything, guc_hw_reset is only called in 1 place, so we could keep the function static and move it to intel_uc.c. > { > int ret; > u32 guc_status; > @@ -452,75 +452,37 @@ int intel_guc_load(struct drm_i915_private *dev_priv) > { > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > const char *fw_path = guc_fw->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_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), > intel_guc_fw_status_repr(guc_fw->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) { > + if (fw_path == NULL) { > /* Device is known to have no uCode (e.g. no GuC) */ > - err = -ENXIO; > - goto fail; > + 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->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) { > - err = -EIO; > - goto fail; > + return -EIO; > } else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) { > - err = -ENOEXEC; > - goto fail; > + return -ENOEXEC; > } > > - guc_interrupts_release(dev_priv); > - gen9_reset_guc_interrupts(dev_priv); > - > guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING; > > - DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n", > - intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), > - intel_guc_fw_status_repr(guc_fw->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. > - */ This comment is removed, but the WA is applicable to all SKL steppings and is also applicable to HuC according to the specs so I suggest to retain the comment and move it to intel_uc_load(). > /* WaEnableGuCBootHashCheckNotSet:skl,bxt */ The implementation of this WA is now outside this function and it is marked as such there. I'd personally prefer to remove this comment from here as it might cause confusion, but no strong feelings either way. > - 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; > + /* we may want to retry guc ucode transfer */ > + ret = guc_ucode_xfer(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->guc_fw_load_status = GUC_FIRMWARE_SUCCESS; > > @@ -528,63 +490,7 @@ int intel_guc_load(struct drm_i915_private *dev_priv) > intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), > intel_guc_fw_status_repr(guc_fw->guc_fw_load_status)); > > - 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); > - } > - > return 0; > - > -fail: > - if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING) > - guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL; > - > - guc_interrupts_release(dev_priv); > - i915_guc_submission_disable(dev_priv); > - i915_guc_submission_fini(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; > } > > static void guc_fw_fetch(struct drm_i915_private *dev_priv, > @@ -757,6 +663,10 @@ void intel_guc_init(struct drm_i915_private *dev_priv) > guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE; > guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE; > > + /* can't enable guc submission without guc */ > + if (!i915.enable_guc_loading) > + i915.enable_guc_submission = 0; > + > /* Early (and silent) return if GuC loading is disabled */ > if (!i915.enable_guc_loading) > return; > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 8eec035..4e184edb 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -35,6 +35,89 @@ void intel_uc_init(struct drm_i915_private *dev_priv) > intel_guc_init(dev_priv); > } > > +int intel_uc_load(struct drm_i915_private *dev_priv) > +{ > + struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > + int ret, retries; > + > + /* guc not enabled, nothing to do */ > + if (!i915.enable_guc_loading) > + return 0; > + > + guc_interrupts_release(dev_priv); > + gen9_reset_guc_interrupts(dev_priv); > + > + guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING; > + > + 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_ATTEPMTS; > + while (retries--) { > + /* > + * Always reset the GuC just before (re)loading, so > + * that the state and timing are fairly predictable > + */ > + ret = guc_hw_reset(dev_priv); > + if (ret) > + goto fail; > + > + ret = intel_guc_load(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; > + > + 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; > + 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). > + */ > + 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_INFO("GuC submission without firmware not supported\n"); > + DRM_NOTE("Falling back from GuC submission to execlist mode\n"); If i915.enable_guc_submission > 1 we will mark the GPU as wedged so it might be worth retaining an error level message here in that scenario. Apart from the minor comments above, the code re-org looks sensible (and required :)) and the patch lgtm. Thanks, Daniele > + } > + > + 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 > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > index ec1a5b2..89c7e1f 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -28,6 +28,9 @@ > #include "i915_guc_reg.h" > #include "intel_ringbuffer.h" > > +/* WaEnableGuCBootHashCheckNotSet:skl,bxt */ > +#define GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS 3 > + > struct drm_i915_gem_request; > > /* > @@ -171,6 +174,7 @@ struct intel_guc { > /* intel_uc.c */ > void intel_uc_init_early(struct drm_i915_private *dev_priv); > void intel_uc_init(struct drm_i915_private *dev_priv); > +int intel_uc_load(struct drm_i915_private *dev_priv); > bool intel_guc_recv(struct drm_i915_private *dev_priv, u32 *status); > int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len); > int intel_guc_sample_forcewake(struct intel_guc *guc); > @@ -185,6 +189,10 @@ extern void intel_guc_fini(struct drm_i915_private *dev_priv); > extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status); > extern int intel_guc_suspend(struct drm_i915_private *dev_priv); > extern int intel_guc_resume(struct drm_i915_private *dev_priv); > +void guc_interrupts_release(struct drm_i915_private *dev_priv); > +void guc_interrupts_capture(struct drm_i915_private *dev_priv); > +int guc_hw_reset(struct drm_i915_private *dev_priv); > +u32 guc_wopcm_size(struct drm_i915_private *dev_priv); > > /* i915_guc_submission.c */ > int i915_guc_submission_init(struct drm_i915_private *dev_priv); >
On Thu, Dec 15, 2016 at 05:38:16PM +0100, Michal Wajdeczko wrote: > On Thu, Dec 15, 2016 at 04:47:06PM +0100, Arkadiusz Hiler wrote: > > Current version of intel_guc_load() does a lot: > > - cares about submission > > - loads huc > > - implement WA > > > > This change offloads some of the logic to intel_uc_load(), which now > > cares about the above. > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > > Cc: Jeff McGee <jeff.mcgee@intel.com> > > Cc: Michal Winiarski <michal.winiarski@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 | 126 +++++--------------------------- > > drivers/gpu/drm/i915/intel_uc.c | 83 +++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_uc.h | 8 ++ > > 4 files changed, 110 insertions(+), 109 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 6af4e85..76b52c6 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -4412,7 +4412,7 @@ 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_load(dev_priv); > > + ret = intel_uc_load(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 f8b28b1..b76b556 100644 > > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > > @@ -97,7 +97,7 @@ const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status) > > } > > }; > > > > -static void guc_interrupts_release(struct drm_i915_private *dev_priv) > > +void guc_interrupts_release(struct drm_i915_private *dev_priv) > > { > > struct intel_engine_cs *engine; > > enum intel_engine_id id; > > @@ -115,7 +115,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 guc_interrupts_capture(struct drm_i915_private *dev_priv) > > { > > struct intel_engine_cs *engine; > > enum intel_engine_id id; > > @@ -334,7 +334,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv, > > return ret; > > } > > > > -static u32 guc_wopcm_size(struct drm_i915_private *dev_priv) > > +u32 guc_wopcm_size(struct drm_i915_private *dev_priv) > > { > > u32 wopcm_size = GUC_WOPCM_TOP; > > > > @@ -417,7 +417,7 @@ 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 guc_hw_reset(struct drm_i915_private *dev_priv) > > { > > int ret; > > u32 guc_status; > > @@ -452,75 +452,37 @@ int intel_guc_load(struct drm_i915_private *dev_priv) > > { > > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > > const char *fw_path = guc_fw->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_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), > > intel_guc_fw_status_repr(guc_fw->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) { > > + if (fw_path == NULL) { > > /* Device is known to have no uCode (e.g. no GuC) */ > > - err = -ENXIO; > > - goto fail; > > + 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->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) { > > - err = -EIO; > > - goto fail; > > + return -EIO; > > } else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) { > > - err = -ENOEXEC; > > - goto fail; > > + return -ENOEXEC; > > } > > > > - guc_interrupts_release(dev_priv); > > - gen9_reset_guc_interrupts(dev_priv); > > - > > guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING; > > > > - DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n", > > - intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), > > - intel_guc_fw_status_repr(guc_fw->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; > > + /* we may want to retry guc ucode transfer */ > > + ret = guc_ucode_xfer(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->guc_fw_load_status = GUC_FIRMWARE_SUCCESS; > > > > @@ -528,63 +490,7 @@ int intel_guc_load(struct drm_i915_private *dev_priv) > > intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), > > intel_guc_fw_status_repr(guc_fw->guc_fw_load_status)); > > > > - 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); > > - } > > - > > return 0; > > - > > -fail: > > - if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING) > > - guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL; > > - > > - guc_interrupts_release(dev_priv); > > - i915_guc_submission_disable(dev_priv); > > - i915_guc_submission_fini(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; > > } > > > > static void guc_fw_fetch(struct drm_i915_private *dev_priv, > > @@ -757,6 +663,10 @@ void intel_guc_init(struct drm_i915_private *dev_priv) > > guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE; > > guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE; > > > > + /* can't enable guc submission without guc */ > > + if (!i915.enable_guc_loading) > > + i915.enable_guc_submission = 0; > > + > > /* Early (and silent) return if GuC loading is disabled */ > > if (!i915.enable_guc_loading) > > return; > > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > > index 8eec035..4e184edb 100644 > > --- a/drivers/gpu/drm/i915/intel_uc.c > > +++ b/drivers/gpu/drm/i915/intel_uc.c > > @@ -35,6 +35,89 @@ void intel_uc_init(struct drm_i915_private *dev_priv) > > intel_guc_init(dev_priv); > > } > > > > +int intel_uc_load(struct drm_i915_private *dev_priv) > > +{ > > + struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > > + int ret, retries; > > + > > + /* guc not enabled, nothing to do */ > > + if (!i915.enable_guc_loading) > > + return 0; > > + > > + guc_interrupts_release(dev_priv); > > + gen9_reset_guc_interrupts(dev_priv); > > + > > + guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING; > > + > > + 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_ATTEPMTS; > > + while (retries--) { > > + /* > > + * Always reset the GuC just before (re)loading, so > > + * that the state and timing are fairly predictable > > + */ > > + ret = guc_hw_reset(dev_priv); > > + if (ret) > > + goto fail; > > + > > + ret = intel_guc_load(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; > > + > > + if (i915.enable_guc_submission) { > > + if (i915.guc_log_level >= 0) > > + gen9_enable_guc_interrupts(dev_priv); > > + > > + ret = i915_guc_submission_enable(dev_priv); > > Hmm, enabling guc submission in this function is still something more > than one can expect from function name suggesting pure "load"... > Do you plan to offload this function in the future ? Yes. With vol. 1 I wanted to push things in the right direction without really finalizing them. I could have spend month and grow the series to dozens of patches, but I think it's better to improve things incrementally, with smaller, digestible patches and series. I also anticipate a lot of valuable feedback that I can incorporate in the future vols. > > + if (ret) > > + goto fail; > > + 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). > > + */ > > + 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_INFO("GuC submission without firmware not supported\n"); > > + DRM_NOTE("Falling back from GuC submission to execlist mode\n"); > > + } > > + > > + 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 > > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > > index ec1a5b2..89c7e1f 100644 > > --- a/drivers/gpu/drm/i915/intel_uc.h > > +++ b/drivers/gpu/drm/i915/intel_uc.h > > @@ -28,6 +28,9 @@ > > #include "i915_guc_reg.h" > > #include "intel_ringbuffer.h" > > > > +/* WaEnableGuCBootHashCheckNotSet:skl,bxt */ > > +#define GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS 3 > > + > > struct drm_i915_gem_request; > > > > /* > > @@ -171,6 +174,7 @@ struct intel_guc { > > /* intel_uc.c */ > > void intel_uc_init_early(struct drm_i915_private *dev_priv); > > void intel_uc_init(struct drm_i915_private *dev_priv); > > +int intel_uc_load(struct drm_i915_private *dev_priv); > > bool intel_guc_recv(struct drm_i915_private *dev_priv, u32 *status); > > int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len); > > int intel_guc_sample_forcewake(struct intel_guc *guc); > > @@ -185,6 +189,10 @@ extern void intel_guc_fini(struct drm_i915_private *dev_priv); > > extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status); > > extern int intel_guc_suspend(struct drm_i915_private *dev_priv); > > extern int intel_guc_resume(struct drm_i915_private *dev_priv); > > +void guc_interrupts_release(struct drm_i915_private *dev_priv); > > +void guc_interrupts_capture(struct drm_i915_private *dev_priv); > > +int guc_hw_reset(struct drm_i915_private *dev_priv); > > +u32 guc_wopcm_size(struct drm_i915_private *dev_priv); > > > > /* i915_guc_submission.c */ > > int i915_guc_submission_init(struct drm_i915_private *dev_priv); > > -- > > 2.9.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Dec 15, 2016 at 02:26:29PM -0800, Daniele Ceraolo Spurio wrote: > > > On 15/12/16 07:47, Arkadiusz Hiler wrote: > > Current version of intel_guc_load() does a lot: > > - cares about submission > > - loads huc > > - implement WA > > > > This change offloads some of the logic to intel_uc_load(), which now > > cares about the above. > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > > Cc: Jeff McGee <jeff.mcgee@intel.com> > > Cc: Michal Winiarski <michal.winiarski@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 | 126 +++++--------------------------- > > drivers/gpu/drm/i915/intel_uc.c | 83 +++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_uc.h | 8 ++ > > 4 files changed, 110 insertions(+), 109 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 6af4e85..76b52c6 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -417,7 +417,7 @@ 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 guc_hw_reset(struct drm_i915_private *dev_priv) > If I haven't missed anything, guc_hw_reset is only called in 1 place, so we > could keep the function static and move it to intel_uc.c. Okay. > > { > > int ret; > > u32 guc_status; > > @@ -452,75 +452,37 @@ int intel_guc_load(struct drm_i915_private *dev_priv) > > { > > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > > const char *fw_path = guc_fw->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_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), > > intel_guc_fw_status_repr(guc_fw->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) { > > + if (fw_path == NULL) { > > /* Device is known to have no uCode (e.g. no GuC) */ > > - err = -ENXIO; > > - goto fail; > > + 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->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) { > > - err = -EIO; > > - goto fail; > > + return -EIO; > > } else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) { > > - err = -ENOEXEC; > > - goto fail; > > + return -ENOEXEC; > > } > > > > - guc_interrupts_release(dev_priv); > > - gen9_reset_guc_interrupts(dev_priv); > > - > > guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING; > > > > - DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n", > > - intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), > > - intel_guc_fw_status_repr(guc_fw->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. > > - */ > > This comment is removed, but the WA is applicable to all SKL steppings and > is also applicable to HuC according to the specs so I suggest to retain the > comment and move it to intel_uc_load(). I missread the commend. I'll leave this as a WaEnableuKernelHeaderValidFix:skl since it is fixed for BXT. > > /* WaEnableGuCBootHashCheckNotSet:skl,bxt */ > > The implementation of this WA is now outside this function and it is marked > as such there. I'd personally prefer to remove this comment from here as it > might cause confusion, but no strong feelings either way. The implementation is twofold now - the the function which returns -EAGAIN if we failed at the step we know may fail and we may want to retry on it. Then you have functions that handles the actuall three attempts. So I prefer to keep the note on the WA in both places. > > - 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; > > + /* we may want to retry guc ucode transfer */ > > + ret = guc_ucode_xfer(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->guc_fw_load_status = GUC_FIRMWARE_SUCCESS; > > > > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > > index 8eec035..4e184edb 100644 > > --- a/drivers/gpu/drm/i915/intel_uc.c > > +++ b/drivers/gpu/drm/i915/intel_uc.c > > @@ -35,6 +35,89 @@ void intel_uc_init(struct drm_i915_private *dev_priv) > > intel_guc_init(dev_priv); > > } > > > > +int intel_uc_load(struct drm_i915_private *dev_priv) > > +{ > > + struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > > + int ret, retries; > > + > > + /* guc not enabled, nothing to do */ > > + if (!i915.enable_guc_loading) > > + return 0; > > + > > + guc_interrupts_release(dev_priv); > > + gen9_reset_guc_interrupts(dev_priv); > > + > > + guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING; > > + > > + 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_ATTEPMTS; > > + while (retries--) { > > + /* > > + * Always reset the GuC just before (re)loading, so > > + * that the state and timing are fairly predictable > > + */ > > + ret = guc_hw_reset(dev_priv); > > + if (ret) > > + goto fail; > > + > > + ret = intel_guc_load(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; > > + > > + 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; > > + 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). > > + */ > > + 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_INFO("GuC submission without firmware not supported\n"); > > + DRM_NOTE("Falling back from GuC submission to execlist mode\n"); > > If i915.enable_guc_submission > 1 we will mark the GPU as wedged so it might > be worth retaining an error level message here in that scenario. If we are wedging the GPU you do not really care about the fallback, so theres no real use in having that promoted + those are the original levels that were already here. Anyway, it seems like the `enable_guc_* > 1` are likely to be gone. I've discussed that on IRC yesterday and no one seems to really remember why we've got it in the first place. Anusha posted similar concern here with her HuC series as well. > Apart from the minor comments above, the code re-org looks sensible (and > required :)) and the patch lgtm. > > Thanks, > Daniele > > > + } > > + > > + 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
On 15/12/2016 15:47, Arkadiusz Hiler wrote: > Current version of intel_guc_load() does a lot: > - cares about submission > - loads huc Not yet, no? So instead you could say that you are preparing the groundworks to make adding in the HuC fit better. > - implement WA > > This change offloads some of the logic to intel_uc_load(), which now > cares about the above. > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > Cc: Jeff McGee <jeff.mcgee@intel.com> > Cc: Michal Winiarski <michal.winiarski@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 | 126 +++++--------------------------- > drivers/gpu/drm/i915/intel_uc.c | 83 +++++++++++++++++++++ > drivers/gpu/drm/i915/intel_uc.h | 8 ++ > 4 files changed, 110 insertions(+), 109 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 6af4e85..76b52c6 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4412,7 +4412,7 @@ 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_load(dev_priv); > + ret = intel_uc_load(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 f8b28b1..b76b556 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -97,7 +97,7 @@ const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status) > } > }; > > -static void guc_interrupts_release(struct drm_i915_private *dev_priv) > +void guc_interrupts_release(struct drm_i915_private *dev_priv) > { > struct intel_engine_cs *engine; > enum intel_engine_id id; > @@ -115,7 +115,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 guc_interrupts_capture(struct drm_i915_private *dev_priv) > { > struct intel_engine_cs *engine; > enum intel_engine_id id; > @@ -334,7 +334,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv, > return ret; > } > > -static u32 guc_wopcm_size(struct drm_i915_private *dev_priv) > +u32 guc_wopcm_size(struct drm_i915_private *dev_priv) > { > u32 wopcm_size = GUC_WOPCM_TOP; > > @@ -417,7 +417,7 @@ 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 guc_hw_reset(struct drm_i915_private *dev_priv) > { > int ret; > u32 guc_status; > @@ -452,75 +452,37 @@ int intel_guc_load(struct drm_i915_private *dev_priv) > { > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > const char *fw_path = guc_fw->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_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), > intel_guc_fw_status_repr(guc_fw->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) { > + if (fw_path == NULL) { > /* Device is known to have no uCode (e.g. no GuC) */ > - err = -ENXIO; > - goto fail; > + 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->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) { > - err = -EIO; > - goto fail; > + return -EIO; > } else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) { > - err = -ENOEXEC; > - goto fail; > + return -ENOEXEC; > } > > - guc_interrupts_release(dev_priv); > - gen9_reset_guc_interrupts(dev_priv); > - > guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING; > > - DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n", > - intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), > - intel_guc_fw_status_repr(guc_fw->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; > + /* we may want to retry guc ucode transfer */ > + ret = guc_ucode_xfer(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->guc_fw_load_status = GUC_FIRMWARE_SUCCESS; > > @@ -528,63 +490,7 @@ int intel_guc_load(struct drm_i915_private *dev_priv) > intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), > intel_guc_fw_status_repr(guc_fw->guc_fw_load_status)); > > - 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); > - } > - > return 0; > - > -fail: > - if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING) > - guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL; You seem to have lost this state transition at least in the enable_guc_loading = 0 case. Was that deliberate? I have no idea if that is important so just an observation. > - > - guc_interrupts_release(dev_priv); > - i915_guc_submission_disable(dev_priv); > - i915_guc_submission_fini(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; > } > > static void guc_fw_fetch(struct drm_i915_private *dev_priv, > @@ -757,6 +663,10 @@ void intel_guc_init(struct drm_i915_private *dev_priv) > guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE; > guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE; > > + /* can't enable guc submission without guc */ > + if (!i915.enable_guc_loading) > + i915.enable_guc_submission = 0; > + > /* Early (and silent) return if GuC loading is disabled */ > if (!i915.enable_guc_loading) > return; You got two same conditions one after another, maybe merge them? > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 8eec035..4e184edb 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -35,6 +35,89 @@ void intel_uc_init(struct drm_i915_private *dev_priv) > intel_guc_init(dev_priv); > } > > +int intel_uc_load(struct drm_i915_private *dev_priv) > +{ > + struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > + int ret, retries; > + > + /* guc not enabled, nothing to do */ > + if (!i915.enable_guc_loading) > + return 0; > + > + guc_interrupts_release(dev_priv); > + gen9_reset_guc_interrupts(dev_priv); > + > + guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING; > + > + 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_ATTEPMTS; > + while (retries--) { > + /* > + * Always reset the GuC just before (re)loading, so > + * that the state and timing are fairly predictable > + */ > + ret = guc_hw_reset(dev_priv); > + if (ret) > + goto fail; > + > + ret = intel_guc_load(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? */ "Have we succeeded" I think. > + if (ret) > + goto fail; There was a debug message round about here I think which logged that the firmware was successfully loaded. I think it is good to have it. I think even logging the major and minor would be good and maybe even at informational level? > + > + 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; > + 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). > + */ > + 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_INFO("GuC submission without firmware not supported\n"); > + DRM_NOTE("Falling back from GuC submission to execlist mode\n"); > + } > + > + 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 > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > index ec1a5b2..89c7e1f 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -28,6 +28,9 @@ > #include "i915_guc_reg.h" > #include "intel_ringbuffer.h" > > +/* WaEnableGuCBootHashCheckNotSet:skl,bxt */ > +#define GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS 3 > + > struct drm_i915_gem_request; > > /* > @@ -171,6 +174,7 @@ struct intel_guc { > /* intel_uc.c */ > void intel_uc_init_early(struct drm_i915_private *dev_priv); > void intel_uc_init(struct drm_i915_private *dev_priv); > +int intel_uc_load(struct drm_i915_private *dev_priv); > bool intel_guc_recv(struct drm_i915_private *dev_priv, u32 *status); > int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len); > int intel_guc_sample_forcewake(struct intel_guc *guc); > @@ -185,6 +189,10 @@ extern void intel_guc_fini(struct drm_i915_private *dev_priv); > extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status); > extern int intel_guc_suspend(struct drm_i915_private *dev_priv); > extern int intel_guc_resume(struct drm_i915_private *dev_priv); > +void guc_interrupts_release(struct drm_i915_private *dev_priv); > +void guc_interrupts_capture(struct drm_i915_private *dev_priv); > +int guc_hw_reset(struct drm_i915_private *dev_priv); > +u32 guc_wopcm_size(struct drm_i915_private *dev_priv); > > /* i915_guc_submission.c */ > int i915_guc_submission_init(struct drm_i915_private *dev_priv); > I was just reading it so I can review 5/5 as you have asked me to look at that one. Regards, Tvrtko
<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). >>> + */ >>> + 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_INFO("GuC submission without firmware not supported\n"); >>> + DRM_NOTE("Falling back from GuC submission to execlist mode\n"); >> >> If i915.enable_guc_submission > 1 we will mark the GPU as wedged so it might >> be worth retaining an error level message here in that scenario. > > If we are wedging the GPU you do not really care about the fallback, so > theres no real use in having that promoted + those are the original > levels that were already here. > > Anyway, it seems like the `enable_guc_* > 1` are likely to be gone. I've > discussed that on IRC yesterday and no one seems to really remember why > we've got it in the first place. > > Anusha posted similar concern here with her HuC series as well. > Just to clarify (because as you said the case will probably go away), what I meant was an extra log for the > 1 case like we had in the original code, i.e: DRM_ERROR("GuC init failed: %d\n", ret); as otherwise we would have declared the GPU wedged without printing any error-level message to explain why. Daniele
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 6af4e85..76b52c6 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4412,7 +4412,7 @@ 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_load(dev_priv); + ret = intel_uc_load(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 f8b28b1..b76b556 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -97,7 +97,7 @@ const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status) } }; -static void guc_interrupts_release(struct drm_i915_private *dev_priv) +void guc_interrupts_release(struct drm_i915_private *dev_priv) { struct intel_engine_cs *engine; enum intel_engine_id id; @@ -115,7 +115,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 guc_interrupts_capture(struct drm_i915_private *dev_priv) { struct intel_engine_cs *engine; enum intel_engine_id id; @@ -334,7 +334,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv, return ret; } -static u32 guc_wopcm_size(struct drm_i915_private *dev_priv) +u32 guc_wopcm_size(struct drm_i915_private *dev_priv) { u32 wopcm_size = GUC_WOPCM_TOP; @@ -417,7 +417,7 @@ 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 guc_hw_reset(struct drm_i915_private *dev_priv) { int ret; u32 guc_status; @@ -452,75 +452,37 @@ int intel_guc_load(struct drm_i915_private *dev_priv) { struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; const char *fw_path = guc_fw->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_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), intel_guc_fw_status_repr(guc_fw->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) { + if (fw_path == NULL) { /* Device is known to have no uCode (e.g. no GuC) */ - err = -ENXIO; - goto fail; + 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->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) { - err = -EIO; - goto fail; + return -EIO; } else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) { - err = -ENOEXEC; - goto fail; + return -ENOEXEC; } - guc_interrupts_release(dev_priv); - gen9_reset_guc_interrupts(dev_priv); - guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING; - DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n", - intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), - intel_guc_fw_status_repr(guc_fw->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; + /* we may want to retry guc ucode transfer */ + ret = guc_ucode_xfer(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->guc_fw_load_status = GUC_FIRMWARE_SUCCESS; @@ -528,63 +490,7 @@ int intel_guc_load(struct drm_i915_private *dev_priv) intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), intel_guc_fw_status_repr(guc_fw->guc_fw_load_status)); - 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); - } - return 0; - -fail: - if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING) - guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL; - - guc_interrupts_release(dev_priv); - i915_guc_submission_disable(dev_priv); - i915_guc_submission_fini(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; } static void guc_fw_fetch(struct drm_i915_private *dev_priv, @@ -757,6 +663,10 @@ void intel_guc_init(struct drm_i915_private *dev_priv) guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE; guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE; + /* can't enable guc submission without guc */ + if (!i915.enable_guc_loading) + i915.enable_guc_submission = 0; + /* Early (and silent) return if GuC loading is disabled */ if (!i915.enable_guc_loading) return; diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 8eec035..4e184edb 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -35,6 +35,89 @@ void intel_uc_init(struct drm_i915_private *dev_priv) intel_guc_init(dev_priv); } +int intel_uc_load(struct drm_i915_private *dev_priv) +{ + struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; + int ret, retries; + + /* guc not enabled, nothing to do */ + if (!i915.enable_guc_loading) + return 0; + + guc_interrupts_release(dev_priv); + gen9_reset_guc_interrupts(dev_priv); + + guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING; + + 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_ATTEPMTS; + while (retries--) { + /* + * Always reset the GuC just before (re)loading, so + * that the state and timing are fairly predictable + */ + ret = guc_hw_reset(dev_priv); + if (ret) + goto fail; + + ret = intel_guc_load(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; + + 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; + 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). + */ + 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_INFO("GuC submission without firmware not supported\n"); + DRM_NOTE("Falling back from GuC submission to execlist mode\n"); + } + + 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 diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h index ec1a5b2..89c7e1f 100644 --- a/drivers/gpu/drm/i915/intel_uc.h +++ b/drivers/gpu/drm/i915/intel_uc.h @@ -28,6 +28,9 @@ #include "i915_guc_reg.h" #include "intel_ringbuffer.h" +/* WaEnableGuCBootHashCheckNotSet:skl,bxt */ +#define GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS 3 + struct drm_i915_gem_request; /* @@ -171,6 +174,7 @@ struct intel_guc { /* intel_uc.c */ void intel_uc_init_early(struct drm_i915_private *dev_priv); void intel_uc_init(struct drm_i915_private *dev_priv); +int intel_uc_load(struct drm_i915_private *dev_priv); bool intel_guc_recv(struct drm_i915_private *dev_priv, u32 *status); int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len); int intel_guc_sample_forcewake(struct intel_guc *guc); @@ -185,6 +189,10 @@ extern void intel_guc_fini(struct drm_i915_private *dev_priv); extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status); extern int intel_guc_suspend(struct drm_i915_private *dev_priv); extern int intel_guc_resume(struct drm_i915_private *dev_priv); +void guc_interrupts_release(struct drm_i915_private *dev_priv); +void guc_interrupts_capture(struct drm_i915_private *dev_priv); +int guc_hw_reset(struct drm_i915_private *dev_priv); +u32 guc_wopcm_size(struct drm_i915_private *dev_priv); /* i915_guc_submission.c */ int i915_guc_submission_init(struct drm_i915_private *dev_priv);
Current version of intel_guc_load() does a lot: - cares about submission - loads huc - implement WA This change offloads some of the logic to intel_uc_load(), which now cares about the above. Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> Cc: Jeff McGee <jeff.mcgee@intel.com> Cc: Michal Winiarski <michal.winiarski@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 | 126 +++++--------------------------- drivers/gpu/drm/i915/intel_uc.c | 83 +++++++++++++++++++++ drivers/gpu/drm/i915/intel_uc.h | 8 ++ 4 files changed, 110 insertions(+), 109 deletions(-)