Re: [P v4 08/11] drm/i915/uc: Improve debug messages in firmware fetch

["Michal Wajdeczko" <michal.wajdeczko@intel.com>]

On Thu, 12 Oct 2017 11:07:21 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2017-10-10 15:51:32)
>> Time to remove less important info and make messages clear
>> and consistent.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_uc_fw.c | 73  
>> +++++++++++++++++++++++---------------
>>  1 file changed, 44 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c  
>> b/drivers/gpu/drm/i915/intel_uc_fw.c
>> index 482115b..b52d6b6 100644
>> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
>> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
>> @@ -45,26 +45,33 @@ void intel_uc_fw_fetch(struct drm_i915_private  
>> *dev_priv,
>>         size_t size;
>>         int err;
>>
>> +       DRM_DEBUG_DRIVER("%s fw fetch %s\n",
>> +                        intel_uc_fw_type_repr(uc_fw->type),  
>> uc_fw->path);
>> +
>>         if (!uc_fw->path)
>>                 return;
>>
>>         uc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
>> -
>> -       DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch  
>> status %s\n",
>> +       DRM_DEBUG_DRIVER("%s fw fetch %s\n",
>> +                        intel_uc_fw_type_repr(uc_fw->type),
>>                          intel_uc_fw_status_repr(uc_fw->fetch_status));
>>
>>         err = request_firmware(&fw, uc_fw->path, &pdev->dev);
>> -       if (err)
>> -               goto fail;
>> -       if (!fw)
>> +       if (err) {
>> +               DRM_NOTE("%s: Error while requesting firmware\n",
>> +                        intel_uc_fw_type_repr(uc_fw->type));
>
> So what am I, the user, meant to do? Do I need to worry? What are the
> consequences of this?

Yes, you can be little worried.
Being here means that driver decided to install *desired* firmware.

We don't know the consequences yet, as there might be a fallback
scenario available (like execlist submission, huc not used)

As we are jumping into fail label, which will start with similar
message, I can downgrade this message into DRM_DEBUG_DRIVER to
avoid duplication.

>
>>                 goto fail;
>> +       }
>
> ...
> fail:
>> +       DRM_WARN("%s: Failed to fetch firmware %s (error %d)\n",
>> +                intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
>
> And then my "significant but normal" message has suddenly become a
> warning the driver is crippled. Make up your mind, do I need to panic or
> not?

Good point. This can be DRM_NOTE.

But maybe we should promote some other existing DRM_NOTEs into:

DRM_WARN("%s: Unexpected firmware size (%zu, min %zu)\n",
DRM_WARN("%s: Mismatched firmware header definition\n",
DRM_WARN("%s: Mismatched firmware header definition\n",
DRM_WARN("%s: Mismatched firmware RSA key size (%u)\n",
DRM_WARN("%s: Truncated firmware (%zu, expected %zu)\n",

as these indicates corrupted/mismatched data (and it's not normal)

Michal
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx