| Message ID | 1463425945-34047-1-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16/05/16 20:12, Dave Gordon wrote: > Split the function of "enable_guc_submission" into two separate > options. The new one ("enable_guc_loading") controls only the > *fetching and loading* of the GuC firmware image. The existing > one is redefined to control only the *use* of the GuC for batch > submission once the firmware is loaded. > > In addition, the degree of control has been refined from a simple > bool to an integer key, allowing several options: > -1 (default) whatever the platform default is > 0 DISABLE don't load/use the GuC > 1 BEST EFFORT try to load/use the GuC, fallback if not available > 2 REQUIRE must load/use the GuC, else leave the GPU wedged > > The new platform default (as coded here) will be to attempt to > load the GuC iff the device has a GuC that requires firmware, > but not yet to use it for submission. A later patch will change > to enable it if appropriate. > > v4: > Changed some error-message levels, mostly ERROR->INFO, per > review comments by Tvrtko Ursulin. > > v5: > Dropped one more error message, disabled GuC submission on > hypothetical firmware-free devices [Tvrtko Ursulin]. Ok, hope it works. :) Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Has someone at least done some manual testing on the various combination of new option values? Please have them splat a Tested-by just because the logic is not straight forward for me. Regards, Tvrtko > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 5 +- > drivers/gpu/drm/i915/i915_guc_submission.c | 4 +- > drivers/gpu/drm/i915/i915_params.c | 14 +++- > drivers/gpu/drm/i915/i915_params.h | 3 +- > drivers/gpu/drm/i915/intel_guc_loader.c | 108 ++++++++++++++++------------- > 5 files changed, 77 insertions(+), 57 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 2cc8c24..1e63744 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4868,11 +4868,8 @@ int i915_gem_init_engines(struct drm_device *dev) > /* We can't enable contexts until all firmware is loaded */ > if (HAS_GUC(dev)) { > ret = intel_guc_setup(dev); > - if (ret) { > - DRM_ERROR("Failed to initialize GuC, error %d\n", ret); > - ret = -EIO; > + if (ret) > goto out; > - } > } > > /* > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 169242a..916cd67 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -969,7 +969,7 @@ int intel_guc_suspend(struct drm_device *dev) > struct intel_context *ctx; > u32 data[3]; > > - if (!i915.enable_guc_submission) > + if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS) > return 0; > > ctx = dev_priv->kernel_context; > @@ -995,7 +995,7 @@ int intel_guc_resume(struct drm_device *dev) > struct intel_context *ctx; > u32 data[3]; > > - if (!i915.enable_guc_submission) > + if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS) > return 0; > > ctx = dev_priv->kernel_context; > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index 383c076..6a5578c 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -54,7 +54,8 @@ struct i915_params i915 __read_mostly = { > .verbose_state_checks = 1, > .nuclear_pageflip = 0, > .edp_vswing = 0, > - .enable_guc_submission = false, > + .enable_guc_loading = -1, > + .enable_guc_submission = 0, > .guc_log_level = -1, > .enable_dp_mst = true, > .inject_load_failure = 0, > @@ -198,8 +199,15 @@ struct i915_params i915 __read_mostly = { > "(0=use value from vbt [default], 1=low power swing(200mV)," > "2=default swing(400mV))"); > > -module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, bool, 0400); > -MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:false)"); > +module_param_named_unsafe(enable_guc_loading, i915.enable_guc_loading, int, 0400); > +MODULE_PARM_DESC(enable_guc_loading, > + "Enable GuC firmware loading " > + "(-1=auto [default], 0=never, 1=if available, 2=required)"); > + > +module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, int, 0400); > +MODULE_PARM_DESC(enable_guc_submission, > + "Enable GuC submission " > + "(-1=auto, 0=never [default], 1=if available, 2=required)"); > > module_param_named(guc_log_level, i915.guc_log_level, int, 0400); > MODULE_PARM_DESC(guc_log_level, > diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h > index 65e73dd..1323261 100644 > --- a/drivers/gpu/drm/i915/i915_params.h > +++ b/drivers/gpu/drm/i915/i915_params.h > @@ -45,6 +45,8 @@ struct i915_params { > int enable_ips; > int invert_brightness; > int enable_cmd_parser; > + int enable_guc_loading; > + int enable_guc_submission; > int guc_log_level; > int use_mmio_flip; > int mmio_debug; > @@ -57,7 +59,6 @@ struct i915_params { > bool load_detect_test; > bool reset; > bool disable_display; > - bool enable_guc_submission; > bool verbose_state_checks; > bool nuclear_pageflip; > bool enable_dp_mst; > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index b14a3a3..7e67f73 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -391,49 +391,37 @@ int intel_guc_setup(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > - int retries, err = 0; > + const char *fw_path = guc_fw->guc_fw_path; > + int retries, ret, err = 0; > > - if (!i915.enable_guc_submission) > - return 0; > - > - DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n", > + 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)); > > - direct_interrupts_to_host(dev_priv); > + /* Loading forbidden, or no firmware to load? */ > + if (!i915.enable_guc_loading) > + goto fail; > + if (fw_path == NULL) > + goto fail; > + if (*fw_path == '\0') { > + DRM_INFO("No GuC firmware known for this platform\n"); > + goto fail; > + } > > - if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_NONE) > - return 0; > + /* Fetch failed, or already fetched but failed to load? */ > + if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) > + goto fail; > + if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) > + goto fail; > > - if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_SUCCESS && > - guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) > - return -ENOEXEC; > + direct_interrupts_to_host(dev_priv); > > guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING; > > - DRM_DEBUG_DRIVER("GuC fw fetch status %s\n", > - intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status)); > - > - switch (guc_fw->guc_fw_fetch_status) { > - case GUC_FIRMWARE_FAIL: > - /* something went wrong :( */ > - err = -EIO; > - goto fail; > - > - case GUC_FIRMWARE_NONE: > - case GUC_FIRMWARE_PENDING: > - default: > - /* "can't happen" */ > - WARN_ONCE(1, "GuC fw %s invalid guc_fw_fetch_status %s [%d]\n", > - guc_fw->guc_fw_path, > - intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), > - guc_fw->guc_fw_fetch_status); > - err = -ENXIO; > - goto fail; > - > - case GUC_FIRMWARE_SUCCESS: > - break; > - } > + 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); > if (err) > @@ -486,7 +474,6 @@ int intel_guc_setup(struct drm_device *dev) > return 0; > > fail: > - DRM_ERROR("GuC firmware load failed, err %d\n", err); > if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING) > guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL; > > @@ -494,7 +481,36 @@ int intel_guc_setup(struct drm_device *dev) > i915_guc_submission_disable(dev); > i915_guc_submission_fini(dev); > > - return err; > + /* > + * 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 (ret == -EIO) > + DRM_ERROR("GuC firmware load failed, err %d\n", err); > + else > + DRM_INFO("GuC firmware load failed, err %d\n", err); > + > + if (i915.enable_guc_submission) { > + if (fw_path == NULL) > + DRM_INFO("GuC submission without firmware not supported\n"); > + DRM_INFO("falling back to execlist mode, ret %d\n", ret); > + } > + i915.enable_guc_submission = 0; > + > + return ret; > } > > static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) > @@ -635,8 +651,11 @@ void intel_guc_init(struct drm_device *dev) > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > const char *fw_path; > > - if (!HAS_GUC_SCHED(dev)) > - i915.enable_guc_submission = false; > + /* A negative value means "use platform default" */ > + if (i915.enable_guc_loading < 0) > + i915.enable_guc_loading = HAS_GUC_UCODE(dev); > + if (i915.enable_guc_submission < 0) > + i915.enable_guc_submission = HAS_GUC_SCHED(dev); > > if (!HAS_GUC_UCODE(dev)) { > fw_path = NULL; > @@ -649,26 +668,21 @@ void intel_guc_init(struct drm_device *dev) > guc_fw->guc_fw_major_wanted = 8; > guc_fw->guc_fw_minor_wanted = 7; > } else { > - i915.enable_guc_submission = false; > fw_path = ""; /* unknown device */ > } > > - if (!i915.enable_guc_submission) > - return; > - > guc_fw->guc_dev = dev; > guc_fw->guc_fw_path = fw_path; > guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE; > guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE; > > + /* Early (and silent) return if GuC loading is disabled */ > + if (!i915.enable_guc_loading) > + return; > if (fw_path == NULL) > return; > - > - if (*fw_path == '\0') { > - DRM_ERROR("No GuC firmware known for this platform\n"); > - guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL; > + if (*fw_path == '\0') > return; > - } > > guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING; > DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path); >
Tested-by: Fiedorowicz, Lukasz <lukasz.fiedorowicz@intel.com> I've done some manual testing on patches mentioned below. Results looks good. I did never encountered any kernel panic or hangs. GPU was wedged when scenario expected it. My testing scenario would look like this: - boot with different parameters combination - run gem_exec_nop - check dmesg and guc debugfs files When GuC was required (option 2) GPU was wedged as expected. When GuC wasn't required in successfully fall back to legacy submission. I've checked scenarios with and without valid GuC firmware present. -----Original Message----- From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Tvrtko Ursulin Sent: Tuesday, May 17, 2016 11:09 AM To: Gordon, David S <david.s.gordon@intel.com>; intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH v5 3/7] drm/i915/guc: add enable_guc_loading parameter On 16/05/16 20:12, Dave Gordon wrote: > Split the function of "enable_guc_submission" into two separate > options. The new one ("enable_guc_loading") controls only the > *fetching and loading* of the GuC firmware image. The existing one is > redefined to control only the *use* of the GuC for batch submission > once the firmware is loaded. > > In addition, the degree of control has been refined from a simple bool > to an integer key, allowing several options: > -1 (default) whatever the platform default is > 0 DISABLE don't load/use the GuC > 1 BEST EFFORT try to load/use the GuC, fallback if not available > 2 REQUIRE must load/use the GuC, else leave the GPU wedged > > The new platform default (as coded here) will be to attempt to load > the GuC iff the device has a GuC that requires firmware, but not yet > to use it for submission. A later patch will change to enable it if > appropriate. > > v4: > Changed some error-message levels, mostly ERROR->INFO, per > review comments by Tvrtko Ursulin. > > v5: > Dropped one more error message, disabled GuC submission on > hypothetical firmware-free devices [Tvrtko Ursulin]. Ok, hope it works. :) Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Has someone at least done some manual testing on the various combination of new option values? Please have them splat a Tested-by just because the logic is not straight forward for me. Regards, Tvrtko > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 5 +- > drivers/gpu/drm/i915/i915_guc_submission.c | 4 +- > drivers/gpu/drm/i915/i915_params.c | 14 +++- > drivers/gpu/drm/i915/i915_params.h | 3 +- > drivers/gpu/drm/i915/intel_guc_loader.c | 108 ++++++++++++++++------------- > 5 files changed, 77 insertions(+), 57 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c > b/drivers/gpu/drm/i915/i915_gem.c index 2cc8c24..1e63744 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4868,11 +4868,8 @@ int i915_gem_init_engines(struct drm_device *dev) > /* We can't enable contexts until all firmware is loaded */ > if (HAS_GUC(dev)) { > ret = intel_guc_setup(dev); > - if (ret) { > - DRM_ERROR("Failed to initialize GuC, error %d\n", ret); > - ret = -EIO; > + if (ret) > goto out; > - } > } > > /* > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c > b/drivers/gpu/drm/i915/i915_guc_submission.c > index 169242a..916cd67 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -969,7 +969,7 @@ int intel_guc_suspend(struct drm_device *dev) > struct intel_context *ctx; > u32 data[3]; > > - if (!i915.enable_guc_submission) > + if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS) > return 0; > > ctx = dev_priv->kernel_context; > @@ -995,7 +995,7 @@ int intel_guc_resume(struct drm_device *dev) > struct intel_context *ctx; > u32 data[3]; > > - if (!i915.enable_guc_submission) > + if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS) > return 0; > > ctx = dev_priv->kernel_context; > diff --git a/drivers/gpu/drm/i915/i915_params.c > b/drivers/gpu/drm/i915/i915_params.c > index 383c076..6a5578c 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -54,7 +54,8 @@ struct i915_params i915 __read_mostly = { > .verbose_state_checks = 1, > .nuclear_pageflip = 0, > .edp_vswing = 0, > - .enable_guc_submission = false, > + .enable_guc_loading = -1, > + .enable_guc_submission = 0, > .guc_log_level = -1, > .enable_dp_mst = true, > .inject_load_failure = 0, > @@ -198,8 +199,15 @@ struct i915_params i915 __read_mostly = { > "(0=use value from vbt [default], 1=low power swing(200mV)," > "2=default swing(400mV))"); > > -module_param_named_unsafe(enable_guc_submission, > i915.enable_guc_submission, bool, 0400); > -MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission > (default:false)"); > +module_param_named_unsafe(enable_guc_loading, > +i915.enable_guc_loading, int, 0400); MODULE_PARM_DESC(enable_guc_loading, > + "Enable GuC firmware loading " > + "(-1=auto [default], 0=never, 1=if available, 2=required)"); > + > +module_param_named_unsafe(enable_guc_submission, > +i915.enable_guc_submission, int, 0400); MODULE_PARM_DESC(enable_guc_submission, > + "Enable GuC submission " > + "(-1=auto, 0=never [default], 1=if available, 2=required)"); > > module_param_named(guc_log_level, i915.guc_log_level, int, 0400); > MODULE_PARM_DESC(guc_log_level, > diff --git a/drivers/gpu/drm/i915/i915_params.h > b/drivers/gpu/drm/i915/i915_params.h > index 65e73dd..1323261 100644 > --- a/drivers/gpu/drm/i915/i915_params.h > +++ b/drivers/gpu/drm/i915/i915_params.h > @@ -45,6 +45,8 @@ struct i915_params { > int enable_ips; > int invert_brightness; > int enable_cmd_parser; > + int enable_guc_loading; > + int enable_guc_submission; > int guc_log_level; > int use_mmio_flip; > int mmio_debug; > @@ -57,7 +59,6 @@ struct i915_params { > bool load_detect_test; > bool reset; > bool disable_display; > - bool enable_guc_submission; > bool verbose_state_checks; > bool nuclear_pageflip; > bool enable_dp_mst; > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c > b/drivers/gpu/drm/i915/intel_guc_loader.c > index b14a3a3..7e67f73 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -391,49 +391,37 @@ int intel_guc_setup(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > - int retries, err = 0; > + const char *fw_path = guc_fw->guc_fw_path; > + int retries, ret, err = 0; > > - if (!i915.enable_guc_submission) > - return 0; > - > - DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n", > + 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)); > > - direct_interrupts_to_host(dev_priv); > + /* Loading forbidden, or no firmware to load? */ > + if (!i915.enable_guc_loading) > + goto fail; > + if (fw_path == NULL) > + goto fail; > + if (*fw_path == '\0') { > + DRM_INFO("No GuC firmware known for this platform\n"); > + goto fail; > + } > > - if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_NONE) > - return 0; > + /* Fetch failed, or already fetched but failed to load? */ > + if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) > + goto fail; > + if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) > + goto fail; > > - if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_SUCCESS && > - guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) > - return -ENOEXEC; > + direct_interrupts_to_host(dev_priv); > > guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING; > > - DRM_DEBUG_DRIVER("GuC fw fetch status %s\n", > - intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status)); > - > - switch (guc_fw->guc_fw_fetch_status) { > - case GUC_FIRMWARE_FAIL: > - /* something went wrong :( */ > - err = -EIO; > - goto fail; > - > - case GUC_FIRMWARE_NONE: > - case GUC_FIRMWARE_PENDING: > - default: > - /* "can't happen" */ > - WARN_ONCE(1, "GuC fw %s invalid guc_fw_fetch_status %s [%d]\n", > - guc_fw->guc_fw_path, > - intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), > - guc_fw->guc_fw_fetch_status); > - err = -ENXIO; > - goto fail; > - > - case GUC_FIRMWARE_SUCCESS: > - break; > - } > + 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); > if (err) > @@ -486,7 +474,6 @@ int intel_guc_setup(struct drm_device *dev) > return 0; > > fail: > - DRM_ERROR("GuC firmware load failed, err %d\n", err); > if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING) > guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL; > > @@ -494,7 +481,36 @@ int intel_guc_setup(struct drm_device *dev) > i915_guc_submission_disable(dev); > i915_guc_submission_fini(dev); > > - return err; > + /* > + * 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 (ret == -EIO) > + DRM_ERROR("GuC firmware load failed, err %d\n", err); > + else > + DRM_INFO("GuC firmware load failed, err %d\n", err); > + > + if (i915.enable_guc_submission) { > + if (fw_path == NULL) > + DRM_INFO("GuC submission without firmware not supported\n"); > + DRM_INFO("falling back to execlist mode, ret %d\n", ret); > + } > + i915.enable_guc_submission = 0; > + > + return ret; > } > > static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw > *guc_fw) @@ -635,8 +651,11 @@ void intel_guc_init(struct drm_device *dev) > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > const char *fw_path; > > - if (!HAS_GUC_SCHED(dev)) > - i915.enable_guc_submission = false; > + /* A negative value means "use platform default" */ > + if (i915.enable_guc_loading < 0) > + i915.enable_guc_loading = HAS_GUC_UCODE(dev); > + if (i915.enable_guc_submission < 0) > + i915.enable_guc_submission = HAS_GUC_SCHED(dev); > > if (!HAS_GUC_UCODE(dev)) { > fw_path = NULL; > @@ -649,26 +668,21 @@ void intel_guc_init(struct drm_device *dev) > guc_fw->guc_fw_major_wanted = 8; > guc_fw->guc_fw_minor_wanted = 7; > } else { > - i915.enable_guc_submission = false; > fw_path = ""; /* unknown device */ > } > > - if (!i915.enable_guc_submission) > - return; > - > guc_fw->guc_dev = dev; > guc_fw->guc_fw_path = fw_path; > guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE; > guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE; > > + /* Early (and silent) return if GuC loading is disabled */ > + if (!i915.enable_guc_loading) > + return; > if (fw_path == NULL) > return; > - > - if (*fw_path == '\0') { > - DRM_ERROR("No GuC firmware known for this platform\n"); > - guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL; > + if (*fw_path == '\0') > return; > - } > > guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING; > DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path); > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 2cc8c24..1e63744 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4868,11 +4868,8 @@ int i915_gem_init_engines(struct drm_device *dev) /* We can't enable contexts until all firmware is loaded */ if (HAS_GUC(dev)) { ret = intel_guc_setup(dev); - if (ret) { - DRM_ERROR("Failed to initialize GuC, error %d\n", ret); - ret = -EIO; + if (ret) goto out; - } } /* diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 169242a..916cd67 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -969,7 +969,7 @@ int intel_guc_suspend(struct drm_device *dev) struct intel_context *ctx; u32 data[3]; - if (!i915.enable_guc_submission) + if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS) return 0; ctx = dev_priv->kernel_context; @@ -995,7 +995,7 @@ int intel_guc_resume(struct drm_device *dev) struct intel_context *ctx; u32 data[3]; - if (!i915.enable_guc_submission) + if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS) return 0; ctx = dev_priv->kernel_context; diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 383c076..6a5578c 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -54,7 +54,8 @@ struct i915_params i915 __read_mostly = { .verbose_state_checks = 1, .nuclear_pageflip = 0, .edp_vswing = 0, - .enable_guc_submission = false, + .enable_guc_loading = -1, + .enable_guc_submission = 0, .guc_log_level = -1, .enable_dp_mst = true, .inject_load_failure = 0, @@ -198,8 +199,15 @@ struct i915_params i915 __read_mostly = { "(0=use value from vbt [default], 1=low power swing(200mV)," "2=default swing(400mV))"); -module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, bool, 0400); -MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:false)"); +module_param_named_unsafe(enable_guc_loading, i915.enable_guc_loading, int, 0400); +MODULE_PARM_DESC(enable_guc_loading, + "Enable GuC firmware loading " + "(-1=auto [default], 0=never, 1=if available, 2=required)"); + +module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, int, 0400); +MODULE_PARM_DESC(enable_guc_submission, + "Enable GuC submission " + "(-1=auto, 0=never [default], 1=if available, 2=required)"); module_param_named(guc_log_level, i915.guc_log_level, int, 0400); MODULE_PARM_DESC(guc_log_level, diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index 65e73dd..1323261 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -45,6 +45,8 @@ struct i915_params { int enable_ips; int invert_brightness; int enable_cmd_parser; + int enable_guc_loading; + int enable_guc_submission; int guc_log_level; int use_mmio_flip; int mmio_debug; @@ -57,7 +59,6 @@ struct i915_params { bool load_detect_test; bool reset; bool disable_display; - bool enable_guc_submission; bool verbose_state_checks; bool nuclear_pageflip; bool enable_dp_mst; diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index b14a3a3..7e67f73 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -391,49 +391,37 @@ int intel_guc_setup(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; - int retries, err = 0; + const char *fw_path = guc_fw->guc_fw_path; + int retries, ret, err = 0; - if (!i915.enable_guc_submission) - return 0; - - DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n", + 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)); - direct_interrupts_to_host(dev_priv); + /* Loading forbidden, or no firmware to load? */ + if (!i915.enable_guc_loading) + goto fail; + if (fw_path == NULL) + goto fail; + if (*fw_path == '\0') { + DRM_INFO("No GuC firmware known for this platform\n"); + goto fail; + } - if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_NONE) - return 0; + /* Fetch failed, or already fetched but failed to load? */ + if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) + goto fail; + if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) + goto fail; - if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_SUCCESS && - guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) - return -ENOEXEC; + direct_interrupts_to_host(dev_priv); guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING; - DRM_DEBUG_DRIVER("GuC fw fetch status %s\n", - intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status)); - - switch (guc_fw->guc_fw_fetch_status) { - case GUC_FIRMWARE_FAIL: - /* something went wrong :( */ - err = -EIO; - goto fail; - - case GUC_FIRMWARE_NONE: - case GUC_FIRMWARE_PENDING: - default: - /* "can't happen" */ - WARN_ONCE(1, "GuC fw %s invalid guc_fw_fetch_status %s [%d]\n", - guc_fw->guc_fw_path, - intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status), - guc_fw->guc_fw_fetch_status); - err = -ENXIO; - goto fail; - - case GUC_FIRMWARE_SUCCESS: - break; - } + 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); if (err) @@ -486,7 +474,6 @@ int intel_guc_setup(struct drm_device *dev) return 0; fail: - DRM_ERROR("GuC firmware load failed, err %d\n", err); if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING) guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL; @@ -494,7 +481,36 @@ int intel_guc_setup(struct drm_device *dev) i915_guc_submission_disable(dev); i915_guc_submission_fini(dev); - return err; + /* + * 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 (ret == -EIO) + DRM_ERROR("GuC firmware load failed, err %d\n", err); + else + DRM_INFO("GuC firmware load failed, err %d\n", err); + + if (i915.enable_guc_submission) { + if (fw_path == NULL) + DRM_INFO("GuC submission without firmware not supported\n"); + DRM_INFO("falling back to execlist mode, ret %d\n", ret); + } + i915.enable_guc_submission = 0; + + return ret; } static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) @@ -635,8 +651,11 @@ void intel_guc_init(struct drm_device *dev) struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; const char *fw_path; - if (!HAS_GUC_SCHED(dev)) - i915.enable_guc_submission = false; + /* A negative value means "use platform default" */ + if (i915.enable_guc_loading < 0) + i915.enable_guc_loading = HAS_GUC_UCODE(dev); + if (i915.enable_guc_submission < 0) + i915.enable_guc_submission = HAS_GUC_SCHED(dev); if (!HAS_GUC_UCODE(dev)) { fw_path = NULL; @@ -649,26 +668,21 @@ void intel_guc_init(struct drm_device *dev) guc_fw->guc_fw_major_wanted = 8; guc_fw->guc_fw_minor_wanted = 7; } else { - i915.enable_guc_submission = false; fw_path = ""; /* unknown device */ } - if (!i915.enable_guc_submission) - return; - guc_fw->guc_dev = dev; guc_fw->guc_fw_path = fw_path; guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE; guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE; + /* Early (and silent) return if GuC loading is disabled */ + if (!i915.enable_guc_loading) + return; if (fw_path == NULL) return; - - if (*fw_path == '\0') { - DRM_ERROR("No GuC firmware known for this platform\n"); - guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_FAIL; + if (*fw_path == '\0') return; - } guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING; DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
Split the function of "enable_guc_submission" into two separate options. The new one ("enable_guc_loading") controls only the *fetching and loading* of the GuC firmware image. The existing one is redefined to control only the *use* of the GuC for batch submission once the firmware is loaded. In addition, the degree of control has been refined from a simple bool to an integer key, allowing several options: -1 (default) whatever the platform default is 0 DISABLE don't load/use the GuC 1 BEST EFFORT try to load/use the GuC, fallback if not available 2 REQUIRE must load/use the GuC, else leave the GPU wedged The new platform default (as coded here) will be to attempt to load the GuC iff the device has a GuC that requires firmware, but not yet to use it for submission. A later patch will change to enable it if appropriate. v4: Changed some error-message levels, mostly ERROR->INFO, per review comments by Tvrtko Ursulin. v5: Dropped one more error message, disabled GuC submission on hypothetical firmware-free devices [Tvrtko Ursulin]. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 5 +- drivers/gpu/drm/i915/i915_guc_submission.c | 4 +- drivers/gpu/drm/i915/i915_params.c | 14 +++- drivers/gpu/drm/i915/i915_params.h | 3 +- drivers/gpu/drm/i915/intel_guc_loader.c | 108 ++++++++++++++++------------- 5 files changed, 77 insertions(+), 57 deletions(-)