| Message ID | 1454504168-29891-1-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
LGTM. Reviewed-by: Alex Dai <yu.dai@intel.com> Thanks, Alex On 02/03/2016 04:56 AM, 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, to attempt to > use it iff the device has a GuC that supports the submission protocol > (with or without firmware), and to fall back to execlist mode if any > required firmware cannot be found or fails to load. > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 1 - > drivers/gpu/drm/i915/i915_params.c | 14 ++++- > drivers/gpu/drm/i915/i915_params.h | 3 +- > drivers/gpu/drm/i915/intel_guc_loader.c | 99 ++++++++++++++++++--------------- > 4 files changed, 68 insertions(+), 49 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index a928823..cd7aada 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4892,7 +4892,6 @@ int i915_gem_init_rings(struct drm_device *dev) > ret = intel_guc_ucode_load(dev); > if (ret) { > DRM_ERROR("Failed to initialize GuC, error %d\n", ret); > - ret = -EIO; > goto out; > } > } > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index 8b9f368..ad785ad 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -55,7 +55,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 = -1, > .guc_log_level = -1, > }; > > @@ -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_submission, > + "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 [default], 0=never, 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 5299290..7180261 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 reset; > bool disable_display; > bool disable_vtd_wa; > - bool enable_guc_submission; > bool verbose_state_checks; > bool nuclear_pageflip; > }; > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index 3accd91..e7eb3db 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -369,49 +369,37 @@ int intel_guc_ucode_load(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > + const char *fw_path = guc_fw->guc_fw_path; > int 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_ERROR("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) > @@ -440,6 +428,8 @@ int intel_guc_ucode_load(struct drm_device *dev) > return 0; > > fail: > + DRM_DEBUG_DRIVER("GuC firmware load fail, err %d\n", err); > + > if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING) > guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL; > > @@ -447,6 +437,29 @@ int intel_guc_ucode_load(struct drm_device *dev) > i915_guc_submission_disable(dev); > i915_guc_submission_fini(dev); > > + /* > + * 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) { > + err = -EIO; > + } else if (HAS_GUC_SCHED(dev) && !HAS_GUC_UCODE(dev)) { > + return 0; > + } else if (i915.enable_guc_submission > 1) { > + err = -EIO; > + } else { > + err = 0; > + } > + > + i915.enable_guc_submission = 0; > + > + DRM_DEBUG_DRIVER("falling back to execlist mode, err %d\n", err); > + > return err; > } > > @@ -588,8 +601,11 @@ void intel_guc_ucode_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; > @@ -598,26 +614,21 @@ void intel_guc_ucode_init(struct drm_device *dev) > guc_fw->guc_fw_major_wanted = 4; > guc_fw->guc_fw_minor_wanted = 3; > } 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);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index a928823..cd7aada 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4892,7 +4892,6 @@ int i915_gem_init_rings(struct drm_device *dev) ret = intel_guc_ucode_load(dev); if (ret) { DRM_ERROR("Failed to initialize GuC, error %d\n", ret); - ret = -EIO; goto out; } } diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 8b9f368..ad785ad 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -55,7 +55,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 = -1, .guc_log_level = -1, }; @@ -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_submission, + "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 [default], 0=never, 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 5299290..7180261 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 reset; bool disable_display; bool disable_vtd_wa; - bool enable_guc_submission; bool verbose_state_checks; bool nuclear_pageflip; }; diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 3accd91..e7eb3db 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -369,49 +369,37 @@ int intel_guc_ucode_load(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; + const char *fw_path = guc_fw->guc_fw_path; int 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_ERROR("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) @@ -440,6 +428,8 @@ int intel_guc_ucode_load(struct drm_device *dev) return 0; fail: + DRM_DEBUG_DRIVER("GuC firmware load fail, err %d\n", err); + if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING) guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL; @@ -447,6 +437,29 @@ int intel_guc_ucode_load(struct drm_device *dev) i915_guc_submission_disable(dev); i915_guc_submission_fini(dev); + /* + * 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) { + err = -EIO; + } else if (HAS_GUC_SCHED(dev) && !HAS_GUC_UCODE(dev)) { + return 0; + } else if (i915.enable_guc_submission > 1) { + err = -EIO; + } else { + err = 0; + } + + i915.enable_guc_submission = 0; + + DRM_DEBUG_DRIVER("falling back to execlist mode, err %d\n", err); + return err; } @@ -588,8 +601,11 @@ void intel_guc_ucode_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; @@ -598,26 +614,21 @@ void intel_guc_ucode_init(struct drm_device *dev) guc_fw->guc_fw_major_wanted = 4; guc_fw->guc_fw_minor_wanted = 3; } 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, to attempt to use it iff the device has a GuC that supports the submission protocol (with or without firmware), and to fall back to execlist mode if any required firmware cannot be found or fails to load. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 1 - drivers/gpu/drm/i915/i915_params.c | 14 ++++- drivers/gpu/drm/i915/i915_params.h | 3 +- drivers/gpu/drm/i915/intel_guc_loader.c | 99 ++++++++++++++++++--------------- 4 files changed, 68 insertions(+), 49 deletions(-)