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@xxxxxxxxx>
> > Cc: Jeff McGee <jeff.mcgee@xxxxxxxxx>
> > Cc: Michal Winiarski <michal.winiarski@xxxxxxxxx>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Cheers,
Arek
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
