| Message ID | 1468260090-2756-3-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/07/16 19:01, Dave Gordon wrote: > Some downgraded from DRM_ERROR() to DRM_WARN(), some eliminated, > and a few upgraded from DRM_INFO() to DRM_NOTE() or DRM_WARN(). > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > --- > drivers/gpu/drm/i915/intel_guc_loader.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index 605c696..fd032eb 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -140,12 +140,14 @@ static u32 get_gttype(struct drm_i915_private *dev_priv) > > static u32 get_core_family(struct drm_i915_private *dev_priv) > { > - switch (INTEL_INFO(dev_priv)->gen) { > + u32 gen = INTEL_GEN(dev_priv); > + > + switch (gen) { > case 9: > return GFXCORE_FAMILY_GEN9; > > default: > - DRM_ERROR("GUC: unsupported core family\n"); > + DRM_WARN("GEN%d does not support GuC operation\n", gen); > return GFXCORE_FAMILY_UNKNOWN; > } > } > @@ -433,7 +435,7 @@ int intel_guc_setup(struct drm_device *dev) > goto fail; > } else if (*fw_path == '\0') { > /* Device has a GuC but we don't know what f/w to load? */ > - DRM_INFO("No GuC firmware known for this platform\n"); > + DRM_WARN("No GuC firmware known for this platform\n"); > err = -ENODEV; > goto fail; > } > @@ -471,10 +473,8 @@ int intel_guc_setup(struct drm_device *dev) > * that the state and timing are fairly predictable > */ > err = i915_reset_guc(dev_priv); > - if (err) { > - DRM_ERROR("GuC reset failed: %d\n", err); > + if (err) > goto fail; > - } > > err = guc_ucode_xfer(dev_priv); > if (!err) > @@ -532,15 +532,15 @@ int intel_guc_setup(struct drm_device *dev) > else if (err == 0) > DRM_INFO("GuC firmware load skipped\n"); > else if (ret != -EIO) > - DRM_INFO("GuC firmware load failed: %d\n", err); > + DRM_NOTE("GuC firmware load failed: %d\n", err); > else > - DRM_ERROR("GuC firmware load failed: %d\n", err); > + DRM_WARN("GuC firmware load failed: %d\n", err); > > if (i915.enable_guc_submission) { > if (fw_path == NULL) > DRM_INFO("GuC submission without firmware not supported\n"); > if (ret == 0) > - DRM_INFO("Falling back from GuC submission to execlist mode\n"); > + DRM_NOTE("Falling back from GuC submission to execlist mode\n"); > else > DRM_ERROR("GuC init failed: %d\n", ret); > } > @@ -656,7 +656,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) > fail: > DRM_DEBUG_DRIVER("GuC fw fetch status FAIL; err %d, fw %p, obj %p\n", > err, fw, guc_fw->guc_fw_obj); > - DRM_ERROR("Failed to fetch GuC firmware from %s (error %d)\n", > + DRM_WARN("Failed to fetch GuC firmware from %s (error %d)\n", > guc_fw->guc_fw_path, err); > > mutex_lock(&dev->struct_mutex); > R-b if you also change all the other DRM_ERRORs in guc_fw_fetch to DRM_DEBUG_DRIVER and merge this last two log lines (DRM_DEBUG_DRIVER + DRM_WARN) to one. :) Regards, Tvrtko
On 12/07/16 10:26, Tvrtko Ursulin wrote: > > On 11/07/16 19:01, Dave Gordon wrote: >> Some downgraded from DRM_ERROR() to DRM_WARN(), some eliminated, >> and a few upgraded from DRM_INFO() to DRM_NOTE() or DRM_WARN(). >> >> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >> --- >> drivers/gpu/drm/i915/intel_guc_loader.c | 20 ++++++++++---------- >> 1 file changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c >> b/drivers/gpu/drm/i915/intel_guc_loader.c >> index 605c696..fd032eb 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_loader.c >> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c >> @@ -140,12 +140,14 @@ static u32 get_gttype(struct drm_i915_private >> *dev_priv) >> >> static u32 get_core_family(struct drm_i915_private *dev_priv) >> { >> - switch (INTEL_INFO(dev_priv)->gen) { >> + u32 gen = INTEL_GEN(dev_priv); >> + >> + switch (gen) { >> case 9: >> return GFXCORE_FAMILY_GEN9; >> >> default: >> - DRM_ERROR("GUC: unsupported core family\n"); >> + DRM_WARN("GEN%d does not support GuC operation\n", gen); >> return GFXCORE_FAMILY_UNKNOWN; >> } >> } >> @@ -433,7 +435,7 @@ int intel_guc_setup(struct drm_device *dev) >> goto fail; >> } else if (*fw_path == '\0') { >> /* Device has a GuC but we don't know what f/w to load? */ >> - DRM_INFO("No GuC firmware known for this platform\n"); >> + DRM_WARN("No GuC firmware known for this platform\n"); >> err = -ENODEV; >> goto fail; >> } >> @@ -471,10 +473,8 @@ int intel_guc_setup(struct drm_device *dev) >> * that the state and timing are fairly predictable >> */ >> err = i915_reset_guc(dev_priv); >> - if (err) { >> - DRM_ERROR("GuC reset failed: %d\n", err); >> + if (err) >> goto fail; >> - } >> >> err = guc_ucode_xfer(dev_priv); >> if (!err) >> @@ -532,15 +532,15 @@ int intel_guc_setup(struct drm_device *dev) >> else if (err == 0) >> DRM_INFO("GuC firmware load skipped\n"); >> else if (ret != -EIO) >> - DRM_INFO("GuC firmware load failed: %d\n", err); >> + DRM_NOTE("GuC firmware load failed: %d\n", err); >> else >> - DRM_ERROR("GuC firmware load failed: %d\n", err); >> + DRM_WARN("GuC firmware load failed: %d\n", err); >> >> if (i915.enable_guc_submission) { >> if (fw_path == NULL) >> DRM_INFO("GuC submission without firmware not >> supported\n"); >> if (ret == 0) >> - DRM_INFO("Falling back from GuC submission to execlist >> mode\n"); >> + DRM_NOTE("Falling back from GuC submission to execlist >> mode\n"); >> else >> DRM_ERROR("GuC init failed: %d\n", ret); >> } >> @@ -656,7 +656,7 @@ static void guc_fw_fetch(struct drm_device *dev, >> struct intel_guc_fw *guc_fw) >> fail: >> DRM_DEBUG_DRIVER("GuC fw fetch status FAIL; err %d, fw %p, obj >> %p\n", >> err, fw, guc_fw->guc_fw_obj); >> - DRM_ERROR("Failed to fetch GuC firmware from %s (error %d)\n", >> + DRM_WARN("Failed to fetch GuC firmware from %s (error %d)\n", >> guc_fw->guc_fw_path, err); >> >> mutex_lock(&dev->struct_mutex); >> > > R-b if you also change all the other DRM_ERRORs in guc_fw_fetch to > DRM_DEBUG_DRIVER and merge this last two log lines (DRM_DEBUG_DRIVER + > DRM_WARN) to one. :) > > Regards, > Tvrtko No, that wouldn't be appropriate. We want the user to be informed if any of these failures occurs, because it means their system is in some way misconfigured e.g. corrupted firmware file. That's definitely not a DEBUG-only event, and it must be logged even if we're going to try to continue in fallback mode. I could change all the earlier ERRORs to NOTEs and leave just the last one as an ERROR i.e. explanation first, consequence after. As for the DEBUG, that's for a different purpose. Whereas the various ERROR/NOTE/INFO messages relate to the existence, format, or content of the required firmware file in the filesystem or ramdisk, the DEBUG is about internal failures such as not being able to allocate memory, over which the user/administrator has no direct control. I might swap them round though (i.e. DEBUG after the ERROR, to explain further than I want to in a user-facing message). .Dave.
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 605c696..fd032eb 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -140,12 +140,14 @@ static u32 get_gttype(struct drm_i915_private *dev_priv) static u32 get_core_family(struct drm_i915_private *dev_priv) { - switch (INTEL_INFO(dev_priv)->gen) { + u32 gen = INTEL_GEN(dev_priv); + + switch (gen) { case 9: return GFXCORE_FAMILY_GEN9; default: - DRM_ERROR("GUC: unsupported core family\n"); + DRM_WARN("GEN%d does not support GuC operation\n", gen); return GFXCORE_FAMILY_UNKNOWN; } } @@ -433,7 +435,7 @@ int intel_guc_setup(struct drm_device *dev) goto fail; } else if (*fw_path == '\0') { /* Device has a GuC but we don't know what f/w to load? */ - DRM_INFO("No GuC firmware known for this platform\n"); + DRM_WARN("No GuC firmware known for this platform\n"); err = -ENODEV; goto fail; } @@ -471,10 +473,8 @@ int intel_guc_setup(struct drm_device *dev) * that the state and timing are fairly predictable */ err = i915_reset_guc(dev_priv); - if (err) { - DRM_ERROR("GuC reset failed: %d\n", err); + if (err) goto fail; - } err = guc_ucode_xfer(dev_priv); if (!err) @@ -532,15 +532,15 @@ int intel_guc_setup(struct drm_device *dev) else if (err == 0) DRM_INFO("GuC firmware load skipped\n"); else if (ret != -EIO) - DRM_INFO("GuC firmware load failed: %d\n", err); + DRM_NOTE("GuC firmware load failed: %d\n", err); else - DRM_ERROR("GuC firmware load failed: %d\n", err); + DRM_WARN("GuC firmware load failed: %d\n", err); if (i915.enable_guc_submission) { if (fw_path == NULL) DRM_INFO("GuC submission without firmware not supported\n"); if (ret == 0) - DRM_INFO("Falling back from GuC submission to execlist mode\n"); + DRM_NOTE("Falling back from GuC submission to execlist mode\n"); else DRM_ERROR("GuC init failed: %d\n", ret); } @@ -656,7 +656,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) fail: DRM_DEBUG_DRIVER("GuC fw fetch status FAIL; err %d, fw %p, obj %p\n", err, fw, guc_fw->guc_fw_obj); - DRM_ERROR("Failed to fetch GuC firmware from %s (error %d)\n", + DRM_WARN("Failed to fetch GuC firmware from %s (error %d)\n", guc_fw->guc_fw_path, err); mutex_lock(&dev->struct_mutex);
Some downgraded from DRM_ERROR() to DRM_WARN(), some eliminated, and a few upgraded from DRM_INFO() to DRM_NOTE() or DRM_WARN(). Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/intel_guc_loader.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)