On 13/05/16 16:31, Tvrtko Ursulin wrote:
On 13/05/16 15:36, 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. Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx> --- 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 | 108 ++++++++++++++++------------- 5 files changed, 76 insertions(+), 54 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 2a7be71..2bf8743 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4870,7 +4870,6 @@ int i915_gem_init_engines(struct drm_device *dev) ret = intel_guc_setup(dev); if (ret) { DRM_ERROR("Failed to initialize GuC, error %d\n", ret);This error msg should go as well I think.
Yes, if we reach this we'll have just printed a message in intel_guc_setup() so we don't really need both.
- 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 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;Will the above two do the right thing for the fw_path == NULL case? That is !HAS_GUC_UCODE && HAS_GUC_SCHED? Looks like load status will bt NONE in that case, GuC submission will be enabled and suspend and resume hooks will be skipped? Maybe fetch and load status should be set to success on such platforms?
I think probably fetch==NONE but load==SUCCESS, in which case the code above will be correct already. OTOH there aren't actually any such platforms yet, and intel_guc_setup() doesn't really support this fully; for instance, we don't know whether the correct behaviour of _setup() on such a hypothetical platform would be to reset the GuC and just skip the DMA, or to skip the reset as well. Certainly some of the setup would still be required.
So for now I've forced GuC submission off for any such platform, so the code above should be OK for the four possibilities (no GuC, GuC not loaded, GuC loaded but not used for submission, GuC loaded and in use).
We can revisit this in the event that a firmware-free GuC ever appears! [snip]
@@ -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 (HAS_GUC_SCHED(dev) && !HAS_GUC_UCODE(dev)) { + /* This GuC works even without firmware :) */ + return 0; + } 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) + DRM_INFO("falling back to execlist mode, ret %d\n", ret);ret is always zero in this msg?
I shouldn't think so; AFAICS it could also be -EIO. This message can be printed in conjunction with either variant of the message above.
.Dave. _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
