| Message ID | 1461780195-17434-1-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 27/04/16 19:03, 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. > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 1 - > 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 | 98 ++++++++++++++++-------------- > drivers/gpu/drm/i915/intel_uncore.c | 2 +- > 6 files changed, 70 insertions(+), 52 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index d493e79..b04effc 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4910,7 +4910,6 @@ int i915_gem_init_engines(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_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 72d6665..42d2efa 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -970,7 +970,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; > @@ -996,7 +996,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; Not terribly important and probably predates your work, but just spotted how this reads very redundant - guc->guc_fw.guc_fw_something, while it could be much more readable as guc->fw.load_status. Observation only. > > 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 876e5da..2ec9cf1 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -387,49 +387,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 retries, 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)); Should load status be anything other than GUC_FIRMWARE_NONE here? > - direct_interrupts_to_host(dev_priv); > + /* Loading forbidden, or no firmware to load? */ > + if (!i915.enable_guc_loading) Nitpick, == 0 would perhaps make it more obvious this is not a boolean. > + goto fail; > + if (fw_path == NULL) > + goto fail; > + if (*fw_path == '\0') { > + DRM_ERROR("No GuC firmware known for this platform\n"); It is not an error unless i915.enable_guc_loading == 2, no? And if best effort then it is probably debug or informational. Also, don't the checks against fw_path (together with the error or debug message) belong in the fw fetch function? If they are invalid fw fetch would have failed and this function would be able to inspect the high level status of that step here, no? > + 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; Leads back to the question of load status in this function. So it is expected we always enter here with load status of none? Is it possible to get here with the firmware already loaded already? > > - 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) > @@ -483,6 +471,7 @@ int intel_guc_ucode_load(struct drm_device *dev) > > fail: > DRM_ERROR("GuC firmware load failed, err %d\n", err); Same as above I think error must be dependent on the requested mode. Some customers are very sensitive to errors which are not really errors so it is bad to log them when they are not. > + > if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING) > guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL; > > @@ -490,6 +479,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; i915_gem_init_hw already guards the call to intel_guc_ucode_load with HAS_GUC_UCODE so at the moment at least this is a dead branch. I don't even understand what is this branch supposed to do? How can there be a platform with no guc fw but guc scheduling? > + } 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); > + This would log when i915.enable_guc_loading is set to 0 which would be confusing. I think in this case the function should bail out much earlier. > return err; > } > > @@ -631,8 +643,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); With this setup currently there is no difference between -1 and 1. But I can assume maybe in the future we could have -1 mean 2 on some platform which would then justify having four possible values for each? > > if (!HAS_GUC_UCODE(dev)) { > fw_path = NULL; > @@ -641,26 +656,21 @@ void intel_guc_ucode_init(struct drm_device *dev) > guc_fw->guc_fw_major_wanted = 6; > guc_fw->guc_fw_minor_wanted = 1; > } else { > - i915.enable_guc_submission = false; > fw_path = ""; /* unknown device */ > } Confusing block, HAS_GUC_UCODE is defined as (IS_GEN9(dev) && !IS_KABYLAKE(dev)) but then here we only support SKL here. Why the former is then not just IS_SKYLAKE? When BXT support is added this still needs to be modified and would only save touching HAS_GUC_UCODE in the header. But it must be a better reason? > > - 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; > - } I also do not understand the complications with fw_path (either NULL or ""). In the two cases fetch status will be either none or fail, respectively, which will equally cause intel_guc_ucode_load to hit the failure path (fw_fetch_status != success). So Is it really required for the fw_path to can either be NULL or ""? > > 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/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 4f1dfe6..df698d7 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -1758,7 +1758,7 @@ int intel_guc_reset(struct drm_i915_private *dev_priv) > int ret; > unsigned long irqflags; > > - if (!i915.enable_guc_submission) > + if (!HAS_GUC_UCODE(dev_priv)) > return -EINVAL; What if HAS_GUC_UCODE is true but the i915.load_guc_firmware has been set to zero? Should it skip the reset in that case as well? > > intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); > Regards, Tvrtko
On 29/04/16 16:03, Tvrtko Ursulin wrote: > > Hi, > > On 27/04/16 19:03, 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. >> >> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >> --- >> drivers/gpu/drm/i915/i915_gem.c | 1 - >> 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 | 98 >> ++++++++++++++++-------------- >> drivers/gpu/drm/i915/intel_uncore.c | 2 +- >> 6 files changed, 70 insertions(+), 52 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c >> b/drivers/gpu/drm/i915/i915_gem.c >> index d493e79..b04effc 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -4910,7 +4910,6 @@ int i915_gem_init_engines(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_guc_submission.c >> b/drivers/gpu/drm/i915/i915_guc_submission.c >> index 72d6665..42d2efa 100644 >> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >> @@ -970,7 +970,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; >> @@ -996,7 +996,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; > > Not terribly important and probably predates your work, but just spotted > how this reads very redundant - guc->guc_fw.guc_fw_something, while it > could be much more readable as guc->fw.load_status. Observation only. That's almost the naming we had in the days when we had a generic (non-GuC-specific) firmware loader. But when that was rejected, each custom version had all the variables qualified with the specific target, hence the redundant naming :( [snip] >> --- a/drivers/gpu/drm/i915/intel_guc_loader.c >> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c >> @@ -387,49 +387,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 retries, 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)); > > Should load status be anything other than GUC_FIRMWARE_NONE here? During resume, it *could* be FAIL, meaning we've tried to load it before and it didn't work then. In that case we won't try again, as presumably it still won't work. >> - direct_interrupts_to_host(dev_priv); >> + /* Loading forbidden, or no firmware to load? */ >> + if (!i915.enable_guc_loading) > > Nitpick, == 0 would perhaps make it more obvious this is not a boolean. But here we are specifically choosing to treat it as a boolean. This reads: if GuC loading is NOT enabled (i.e. IS disabled) ... The value 0 for disabled was not accidental. And note we don't create #defines for this sort of kernel parameter, because you have to give a literal constant on the kernel command line. >> + goto fail; >> + if (fw_path == NULL) >> + goto fail; >> + if (*fw_path == '\0') { >> + DRM_ERROR("No GuC firmware known for this platform\n"); > > It is not an error unless i915.enable_guc_loading == 2, no? And if best > effort then it is probably debug or informational. No, it's still an ERROR. You're running the driver on a platform for which we don't know what firmware is required. That probably means an old driver on new hardware, so it might not work at all. You can suppress the error by setting i915.enable_guc_loading=0 if you want to try this version of the driver anyway. Also note the difference between path == NULL (no GuC, or no firmware required => not an error) vs. path == "" (has GuC, presumably needs firmware, but we don't know where to look => ERROR). > Also, don't the checks against fw_path (together with the error or debug > message) belong in the fw fetch function? If they are invalid fw fetch > would have failed and this function would be able to inspect the high > level status of that step here, no? The checks are done in intel_guc_ucode_init(), before fw_fetch() is even called; but that function is void, so can't return failure. (Also, we originally supported asynchronous loading, which also can't return failure). So this function will get called even when we already know that we haven't got any firmware to load, and these tests are indeed checking the high-level status from _init(). >> + 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; > > Leads back to the question of load status in this function. So it is > expected we always enter here with load status of none? Is it possible > to get here with the firmware already loaded already? Not *actually* loaded, because it's been erased by poweroff. But the status tracking variables are persistent, so they reflect the last attempt. So on resume, we actually expect "SUCCESS" at this point, and therefore change it back to PENDING below. >> - 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) >> @@ -483,6 +471,7 @@ int intel_guc_ucode_load(struct drm_device *dev) >> >> fail: >> DRM_ERROR("GuC firmware load failed, err %d\n", err); > > Same as above I think error must be dependent on the requested mode. > Some customers are very sensitive to errors which are not really errors > so it is bad to log them when they are not. No, it's still an ERROR. enable_guc_loading must be nonzero, so we've been asked to *try* to load the GuC. If the load fails, that means broken hardware or a corrupt firmware blob, or some other form of system misconfiguration. Even if we're going to fall back to execlist mode, that needs to be reported; for example, it may mean that SLPC is disabled. The user can avoid the error by booting with GuC loading disabled, but they should probably fix the problem instead. >> + >> if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING) >> guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL; >> >> @@ -490,6 +479,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; > > i915_gem_init_hw already guards the call to intel_guc_ucode_load with > HAS_GUC_UCODE so at the moment at least this is a dead branch. > > I don't even understand what is this branch supposed to do? How can > there be a platform with no guc fw but guc scheduling? Imagine a GuC with firmware in ROM :) Or at least flash ... (it already has a BootROM) >> + } 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); >> + > > This would log when i915.enable_guc_loading is set to 0 which would be > confusing. I think in this case the function should bail out much earlier. That was tested right back at the top of the function! It bailed out very early, so you can't get here with GuC loading disabled. Also, that's a DEBUG message, so users won't see it by default. >> return err; >> } >> >> @@ -631,8 +643,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); > > With this setup currently there is no difference between -1 and 1. But I > can assume maybe in the future we could have -1 mean 2 on some platform > which would then justify having four possible values for each? Yes. 0, 1, and 2 are "user preference". -1 is "system's choice". So there *is* a difference between -1 and 1, because -1 means the same as 0 on non-GuC platforms, but the same as 1 on those that have a GuC, or could mean the same as 2 on GuC-only platforms. >> if (!HAS_GUC_UCODE(dev)) { >> fw_path = NULL; >> @@ -641,26 +656,21 @@ void intel_guc_ucode_init(struct drm_device *dev) >> guc_fw->guc_fw_major_wanted = 6; >> guc_fw->guc_fw_minor_wanted = 1; >> } else { >> - i915.enable_guc_submission = false; >> fw_path = ""; /* unknown device */ >> } > > Confusing block, HAS_GUC_UCODE is defined as (IS_GEN9(dev) && > !IS_KABYLAKE(dev)) but then here we only support SKL here. Why the > former is then not just IS_SKYLAKE? > > When BXT support is added this still needs to be modified and would only > save touching HAS_GUC_UCODE in the header. But it must be a better reason? I don't see anything confusing. The logic does not depend on how somebody has defined HAS_GUC_UCODE(), and we shouldn't assume any relation between it and the platform macros. What it says here is: 1. if this platform *doesn't* have GuC firmware, fw_path is NULL 2a. else if this is SKYLAKE, look for f/w version 6.1 2b. (repeat 2a for each supported platform) 3. (else) unknown device, path is "" which triggers ERROR later. Imagine a SKL version with GuC uCode in ROM - the HAS_GUC_UCODE() test must take precedence. >> - 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; >> - } > > I also do not understand the complications with fw_path (either NULL or > ""). In the two cases fetch status will be either none or fail, > respectively, which will equally cause intel_guc_ucode_load to hit the > failure path (fw_fetch_status != success). > > So Is it really required for the fw_path to can either be NULL or ""? As mentioned before, it's so that the status gets propagated from _init() to _load(). A Previous Reviewer didn't like issuing errors here, so the error report got deferred until we actually try to load the firmware. (Also, someday, we might reenable asynchronous (deferred) firmware loading. especially on Android). >> 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/intel_uncore.c >> b/drivers/gpu/drm/i915/intel_uncore.c >> index 4f1dfe6..df698d7 100644 >> --- a/drivers/gpu/drm/i915/intel_uncore.c >> +++ b/drivers/gpu/drm/i915/intel_uncore.c >> @@ -1758,7 +1758,7 @@ int intel_guc_reset(struct drm_i915_private >> *dev_priv) >> int ret; >> unsigned long irqflags; >> >> - if (!i915.enable_guc_submission) >> + if (!HAS_GUC_UCODE(dev_priv)) >> return -EINVAL; > > What if HAS_GUC_UCODE is true but the i915.load_guc_firmware has been > set to zero? Should it skip the reset in that case as well? Probably not. This test should probably be HAS_GUC(), regardless of whether it has f/w or whether you want the f/w loaded. But there isn't any such macro, so this was the closest. .Dave. >> intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); >> > > Regards, > > Tvrtko
On 06/05/16 17:39, Dave Gordon wrote: > On 29/04/16 16:03, Tvrtko Ursulin wrote: [snip] >>> + goto fail; >>> + if (fw_path == NULL) >>> + goto fail; >>> + if (*fw_path == '\0') { >>> + DRM_ERROR("No GuC firmware known for this platform\n"); >> >> It is not an error unless i915.enable_guc_loading == 2, no? And if best >> effort then it is probably debug or informational. > > No, it's still an ERROR. You're running the driver on a platform for > which we don't know what firmware is required. That probably means an > old driver on new hardware, so it might not work at all. You can > suppress the error by setting i915.enable_guc_loading=0 if you want to > try this version of the driver anyway. Also note the difference between > path == NULL (no GuC, or no firmware required => not an error) vs. path > == "" (has GuC, presumably needs firmware, but we don't know where to > look => ERROR). I think that if i915.enable_guc_loading == 1 then no error should be logged. Documentation says that value meand "try to load/use the GuC, fallback if not available" and to me that means it is not an error and an informational message only should be logged. >> Also, don't the checks against fw_path (together with the error or debug >> message) belong in the fw fetch function? If they are invalid fw fetch >> would have failed and this function would be able to inspect the high >> level status of that step here, no? > > The checks are done in intel_guc_ucode_init(), before fw_fetch() is even > called; but that function is void, so can't return failure. (Also, we > originally supported asynchronous loading, which also can't return > failure). So this function will get called even when we already know > that we haven't got any firmware to load, and these tests are indeed > checking the high-level status from _init(). Anyhow the special meanings conveyed in fw_path == NULL and *fw_path == 0 are imho just too hard to follow. I see it is not your code, but reworking this looked like an opportunity to clean that up. Never mind. >>> + 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; >> >> Leads back to the question of load status in this function. So it is >> expected we always enter here with load status of none? Is it possible >> to get here with the firmware already loaded already? > > Not *actually* loaded, because it's been erased by poweroff. But the > status tracking variables are persistent, so they reflect the last > attempt. So on resume, we actually expect "SUCCESS" at this point, and > therefore change it back to PENDING below. Shouldn't the code then update the status variables on suspend/whatever? Same as above, I find it very hard to follow the logic here. >>> - 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) >>> @@ -483,6 +471,7 @@ int intel_guc_ucode_load(struct drm_device *dev) >>> >>> fail: >>> DRM_ERROR("GuC firmware load failed, err %d\n", err); >> >> Same as above I think error must be dependent on the requested mode. >> Some customers are very sensitive to errors which are not really errors >> so it is bad to log them when they are not. > > No, it's still an ERROR. enable_guc_loading must be nonzero, so we've > been asked to *try* to load the GuC. If the load fails, that means > broken hardware or a corrupt firmware blob, or some other form of system > misconfiguration. Even if we're going to fall back to execlist mode, > that needs to be reported; for example, it may mean that SLPC is > disabled. The user can avoid the error by booting with GuC loading > disabled, but they should probably fix the problem instead. Yes it needs to be reported, but if we are in best effort mode it should be informational I think. Depends how you view the errors in the kernel log - it may be a firmware loading error, but from the point of view of the system log, it is not an error. Everything still works as intended, more so, the user has asked us to try and carry on with the alternative. So the system (log) experienced no error. >>> + >>> if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING) >>> guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL; >>> >>> @@ -490,6 +479,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; >> >> i915_gem_init_hw already guards the call to intel_guc_ucode_load with >> HAS_GUC_UCODE so at the moment at least this is a dead branch. >> >> I don't even understand what is this branch supposed to do? How can >> there be a platform with no guc fw but guc scheduling? > > Imagine a GuC with firmware in ROM :) Or at least flash ... > > (it already has a BootROM) Ok but it is still a dead branch, no? Should the HAS_GUC_UCODE guard in i915_gem_init_hw be removed and let be handled by this code only? >>> + } 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); >>> + >> >> This would log when i915.enable_guc_loading is set to 0 which would be >> confusing. I think in this case the function should bail out much >> earlier. > > That was tested right back at the top of the function! It bailed out > very early, so you can't get here with GuC loading disabled. AFAICS the patch removes the early bailout?! > Also, that's a DEBUG message, so users won't see it by default. So it is OK to confuse fellow developers? :D >>> if (!HAS_GUC_UCODE(dev)) { >>> fw_path = NULL; >>> @@ -641,26 +656,21 @@ void intel_guc_ucode_init(struct drm_device *dev) >>> guc_fw->guc_fw_major_wanted = 6; >>> guc_fw->guc_fw_minor_wanted = 1; >>> } else { >>> - i915.enable_guc_submission = false; >>> fw_path = ""; /* unknown device */ >>> } >> >> Confusing block, HAS_GUC_UCODE is defined as (IS_GEN9(dev) && >> !IS_KABYLAKE(dev)) but then here we only support SKL here. Why the >> former is then not just IS_SKYLAKE? >> >> When BXT support is added this still needs to be modified and would only >> save touching HAS_GUC_UCODE in the header. But it must be a better >> reason? > > I don't see anything confusing. The logic does not depend on how > somebody has defined HAS_GUC_UCODE(), and we shouldn't assume any > relation between it and the platform macros. What it says here is: > > 1. if this platform *doesn't* have GuC firmware, fw_path is NULL > 2a. else if this is SKYLAKE, look for f/w version 6.1 > 2b. (repeat 2a for each supported platform) > 3. (else) unknown device, path is "" which triggers ERROR later. > > Imagine a SKL version with GuC uCode in ROM - the HAS_GUC_UCODE() test > must take precedence. My question was, why isn't HAS_GUC_UCODE == IS_SKYLAKE to start with ? Then when BXT support is added, which will need code changes anyway, we change HAS_GUC_UCODE only. >>> - 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; >>> - } >> >> I also do not understand the complications with fw_path (either NULL or >> ""). In the two cases fetch status will be either none or fail, >> respectively, which will equally cause intel_guc_ucode_load to hit the >> failure path (fw_fetch_status != success). >> >> So Is it really required for the fw_path to can either be NULL or ""? > > As mentioned before, it's so that the status gets propagated from > _init() to _load(). A Previous Reviewer didn't like issuing errors here, > so the error report got deferred until we actually try to load the > firmware. (Also, someday, we might reenable asynchronous (deferred) > firmware loading. especially on Android). It is all very complicated, do we have an IGT to test all these new options? I would trust that more than my ability to figure out the logic here. Regards, Tvrtko
On 10/05/16 15:37, Tvrtko Ursulin wrote: > > On 06/05/16 17:39, Dave Gordon wrote: >> On 29/04/16 16:03, Tvrtko Ursulin wrote: > > [snip] > >>>> + goto fail; >>>> + if (fw_path == NULL) >>>> + goto fail; >>>> + if (*fw_path == '\0') { >>>> + DRM_ERROR("No GuC firmware known for this platform\n"); >>> >>> It is not an error unless i915.enable_guc_loading == 2, no? And if best >>> effort then it is probably debug or informational. >> >> No, it's still an ERROR. You're running the driver on a platform for >> which we don't know what firmware is required. That probably means an >> old driver on new hardware, so it might not work at all. You can >> suppress the error by setting i915.enable_guc_loading=0 if you want to >> try this version of the driver anyway. Also note the difference between >> path == NULL (no GuC, or no firmware required => not an error) vs. path >> == "" (has GuC, presumably needs firmware, but we don't know where to >> look => ERROR). > > I think that if i915.enable_guc_loading == 1 then no error should be > logged. Documentation says that value meand "try to load/use the GuC, > fallback if not available" and to me that means it is not an error and > an informational message only should be logged. OK, this message is now DRM_INFO >>> Also, don't the checks against fw_path (together with the error or debug >>> message) belong in the fw fetch function? If they are invalid fw fetch >>> would have failed and this function would be able to inspect the high >>> level status of that step here, no? >> >> The checks are done in intel_guc_ucode_init(), before fw_fetch() is even >> called; but that function is void, so can't return failure. (Also, we >> originally supported asynchronous loading, which also can't return >> failure). So this function will get called even when we already know >> that we haven't got any firmware to load, and these tests are indeed >> checking the high-level status from _init(). > > Anyhow the special meanings conveyed in fw_path == NULL and *fw_path == > 0 are imho just too hard to follow. I see it is not your code, but > reworking this looked like an opportunity to clean that up. Never mind. For fw_path: == NULL means no firmware needed, not an error. == "" means the driver actually doesn't know what to load in this situation - the hardware is not recognised. IMHO this should be an ERROR, but I've made it just informational now. == path the file we want. Those are the only three possibilities, it's set just once according the hardware type and is constant thereafter. It doesn't depend on the actual load status. That really isn't very complicated. >>>> + 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; >>> >>> Leads back to the question of load status in this function. So it is >>> expected we always enter here with load status of none? Is it possible >>> to get here with the firmware already loaded already? >> >> Not *actually* loaded, because it's been erased by poweroff. But the >> status tracking variables are persistent, so they reflect the last >> attempt. So on resume, we actually expect "SUCCESS" at this point, and >> therefore change it back to PENDING below. > > Shouldn't the code then update the status variables on suspend/whatever? > Same as above, I find it very hard to follow the logic here. It proceeds by identifying all the ways in which the loading process can fail; what's left must be The Path To Success (tm) :) 1. Loading forbidden => fail 2. No firmware wanted => fail 3. No firmware known => fail 4. Firmware not fetched => fail 5. Previous load failed => fail At this point we commit to trying to load the firmware into the h/w. Then we try three times to reset-and-load the GuC. >>>> - 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; >>>> - } Look how many lines of complicated logic this patch removes :) >>>> + 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) >>>> @@ -483,6 +471,7 @@ int intel_guc_ucode_load(struct drm_device *dev) >>>> >>>> fail: >>>> DRM_ERROR("GuC firmware load failed, err %d\n", err); >>> >>> Same as above I think error must be dependent on the requested mode. >>> Some customers are very sensitive to errors which are not really errors >>> so it is bad to log them when they are not. >> >> No, it's still an ERROR. enable_guc_loading must be nonzero, so we've >> been asked to *try* to load the GuC. If the load fails, that means >> broken hardware or a corrupt firmware blob, or some other form of system >> misconfiguration. Even if we're going to fall back to execlist mode, >> that needs to be reported; for example, it may mean that SLPC is >> disabled. The user can avoid the error by booting with GuC loading >> disabled, but they should probably fix the problem instead. > > Yes it needs to be reported, but if we are in best effort mode it should > be informational I think. Depends how you view the errors in the kernel > log - it may be a firmware loading error, but from the point of view of > the system log, it is not an error. Everything still works as intended, > more so, the user has asked us to try and carry on with the alternative. > So the system (log) experienced no error. OK, I've moved it later, and it's now DRM_ERROR if we're going to return -EIO (GPU wedged) and DRM_INFO otherwise. >>>> + >>>> if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING) >>>> guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL; >>>> >>>> @@ -490,6 +479,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; >>> >>> i915_gem_init_hw already guards the call to intel_guc_ucode_load with >>> HAS_GUC_UCODE so at the moment at least this is a dead branch. >>> >>> I don't even understand what is this branch supposed to do? How can >>> there be a platform with no guc fw but guc scheduling? >> >> Imagine a GuC with firmware in ROM :) Or at least flash ... >> >> (it already has a BootROM) > > Ok but it is still a dead branch, no? Should the HAS_GUC_UCODE guard in > i915_gem_init_hw be removed and let be handled by this code only? Yep, I've changed the call condition :) >>>> + } 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); >>> >>> This would log when i915.enable_guc_loading is set to 0 which would be >>> confusing. I think in this case the function should bail out much >>> earlier. >> >> That was tested right back at the top of the function! It bailed out >> very early, so you can't get here with GuC loading disabled. > > AFAICS the patch removes the early bailout?! > >> Also, that's a DEBUG message, so users won't see it by default. > > So it is OK to confuse fellow developers? :D With the new reporting as described above, this message is promoted to DRM_INFO, but only if enable_guc_submission is not already zero. If GuC submission is already disabled, we won't report that we're falling back to execlist mode. You can still confuse yourself by disabling GuC loading *without* also disabling GuC submission, but in that case the message should help you realise you asked for something rather strange :) >>>> if (!HAS_GUC_UCODE(dev)) { >>>> fw_path = NULL; >>>> @@ -641,26 +656,21 @@ void intel_guc_ucode_init(struct drm_device *dev) >>>> guc_fw->guc_fw_major_wanted = 6; >>>> guc_fw->guc_fw_minor_wanted = 1; >>>> } else { >>>> - i915.enable_guc_submission = false; >>>> fw_path = ""; /* unknown device */ >>>> } >>> >>> Confusing block, HAS_GUC_UCODE is defined as (IS_GEN9(dev) && >>> !IS_KABYLAKE(dev)) but then here we only support SKL here. Why the >>> former is then not just IS_SKYLAKE? >>> >>> When BXT support is added this still needs to be modified and would only >>> save touching HAS_GUC_UCODE in the header. But it must be a better >>> reason? >> >> I don't see anything confusing. The logic does not depend on how >> somebody has defined HAS_GUC_UCODE(), and we shouldn't assume any >> relation between it and the platform macros. What it says here is: >> >> 1. if this platform *doesn't* have GuC firmware, fw_path is NULL >> 2a. else if this is SKYLAKE, look for f/w version 6.1 >> 2b. (repeat 2a for each supported platform) >> 3. (else) unknown device, path is "" which triggers ERROR later. >> >> Imagine a SKL version with GuC uCode in ROM - the HAS_GUC_UCODE() test >> must take precedence. > > My question was, why isn't HAS_GUC_UCODE == IS_SKYLAKE to start with ? > Then when BXT support is added, which will need code changes anyway, we > change HAS_GUC_UCODE only. Probably because the original definition of HAS_GUC_UCODE() was simply IS_GEN9() -- POR was that *ALL* Gen9 platforms should have a GuC and firmware for it. Then someone else added KBL as an exception. Anyway, we now have BXT support, so the if-else-if condition ladder has a more obvious structure now :) .Dave.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index d493e79..b04effc 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4910,7 +4910,6 @@ int i915_gem_init_engines(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_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 72d6665..42d2efa 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -970,7 +970,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; @@ -996,7 +996,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 876e5da..2ec9cf1 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -387,49 +387,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 retries, 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) @@ -483,6 +471,7 @@ int intel_guc_ucode_load(struct drm_device *dev) 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; @@ -490,6 +479,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; } @@ -631,8 +643,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; @@ -641,26 +656,21 @@ void intel_guc_ucode_init(struct drm_device *dev) guc_fw->guc_fw_major_wanted = 6; guc_fw->guc_fw_minor_wanted = 1; } 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/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 4f1dfe6..df698d7 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -1758,7 +1758,7 @@ int intel_guc_reset(struct drm_i915_private *dev_priv) int ret; unsigned long irqflags; - if (!i915.enable_guc_submission) + if (!HAS_GUC_UCODE(dev_priv)) return -EINVAL; intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
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. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 1 - 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 | 98 ++++++++++++++++-------------- drivers/gpu/drm/i915/intel_uncore.c | 2 +- 6 files changed, 70 insertions(+), 52 deletions(-)