[Intel-gfx] [PATCH 3/3] drm/i915/guc: revisit GuC loader message levels

[Dave Gordon]

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 at 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.