[Intel-gfx] [PATCH] drm/i915: Show firmware URL once

[Michal Wajdeczko]

On Mon, 23 Oct 2017 14:15:06 +0200, Chris Wilson  
<chris at chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2017-10-23 13:04:49)
>> On Sun, 22 Oct 2017 12:15:50 +0200, Chris Wilson
>> <chris at chris-wilson.co.uk> wrote:
>>
>> > Just show the firmware URL on the first failure, not every failure.
>> >
>> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> > Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_csr.c   |  3 +--
>> >  drivers/gpu/drm/i915/intel_uc_fw.c | 13 +++++++++++--
>> >  drivers/gpu/drm/i915/intel_uc_fw.h |  5 ++---
>> >  3 files changed, 14 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_csr.c
>> > b/drivers/gpu/drm/i915/intel_csr.c
>> > index da9de47562b8..12a857acdb9b 100644
>> > --- a/drivers/gpu/drm/i915/intel_csr.c
>> > +++ b/drivers/gpu/drm/i915/intel_csr.c
>> > @@ -427,8 +427,7 @@ static void csr_load_work_fn(struct work_struct
>> > *work)
>> >                          "Failed to load DMC firmware %s."
>> >                          " Disabling runtime power management.\n",
>> >                          csr->fw_path);
>> > -             dev_notice(dev_priv->drm.dev, "DMC firmware homepage:  
>> %s",
>> > -                        INTEL_UC_FIRMWARE_URL);
>> > +             intel_uc_fw_show_url(dev_priv);
>> >       }
>> >       release_firmware(fw);
>> > diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c
>> > b/drivers/gpu/drm/i915/intel_uc_fw.c
>> > index 973888e94cba..4ee90fda326b 100644
>> > --- a/drivers/gpu/drm/i915/intel_uc_fw.c
>> > +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
>> > @@ -28,6 +28,16 @@
>> >  #include "intel_uc_fw.h"
>> >  #include "i915_drv.h"
>> > +/* Home of GuC, HuC and DMC firmwares */
>> > +#define INTEL_UC_FIRMWARE_URL
>> > "https://01.org/linuxgraphics/downloads/firmware"
>> > +
>> > +void intel_uc_fw_show_url(struct drm_i915_private *i915)
>> > +{
>> > +     dev_info_once(i915->drm.dev,
>> > +                   "Firmware can be downloaded from %s\n",
>> > +                   INTEL_UC_FIRMWARE_URL);
>> > +}
>> > +
>> >  /**
>> >   * intel_uc_fw_fetch - fetch uC firmware
>> >   *
>> > @@ -189,8 +199,7 @@ void intel_uc_fw_fetch(struct drm_i915_private
>> > *dev_priv,
>> >       DRM_WARN("%s: Failed to fetch firmware %s (error %d)\n",
>> >                intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
>> > -     DRM_INFO("%s: Firmware can be downloaded from %s\n",
>> > -              intel_uc_fw_type_repr(uc_fw->type),  
>> INTEL_UC_FIRMWARE_URL);
>> > +     intel_uc_fw_show_url(dev_priv);
>>
>> Hmm, intel_uc_fw_fetch() should be called only once per specific uC
>> and only as part of i915_driver_load() - so there shall be no repeated
>> fetch failures for the same uC.
>>
>> Also your changed message is little too generic (in old one we had uC
>> type, which may be helpful for the user).
>
> I had the same url multiple times; at different log levels, that was
> ugly. The url we give is the base for all firmwares, which one failed is
> given in the previous line (including expected version) so unless you
> plan on generating the url for the specific firmware, it is repeating
> itself.

Hmm, we can use query url (and this will work even without immediate  
support
 from 01.org site, user will just see homepage). Then your log will look  
like:

[    4.961005] i915 0000:00:02.0: Direct firmware load for  
i915/bxt_dmc_ver1_07.bin failed with error -2
[    4.961035] i915 0000:00:02.0: DMC firmware can be downloaded from:  
https://01.org/linuxgraphics/downloads/firmware?name=i915/bxt_dmc_ver1_07.bin
[    4.983194] i915 0000:00:02.0: Direct firmware load for  
i915/bxt_huc_ver01_07_1398.bin failed with error -2
[    4.983224] i915 0000:00:02.0: HuC firmware can be downloaded from:  
https://01.org/linuxgraphics/downloads/firmware?name=i915/bxt_huc_ver01_07_1398.bin

>
>> And in case of independent
>> fetch failures (lets say there is no DMC and HuC fw), we may wrongly
>> suggest that given url is for fw that failed first (DMC?), as messages
>>  from second failures (HuC) will not have such hint - user will have to
>> grep whole dmesg to guess that earlier url is applicable there too.
>
> Correct, but such warnings are clustered.

And they are clustered accidentally as DMC fw is loaded from separate
csr.work.

>
> [    4.961005] i915 0000:00:02.0: Direct firmware load for  
> i915/bxt_dmc_ver1_07.bin failed with error -2
> [    4.961028] i915 0000:00:02.0: Failed to load DMC firmware  
> i915/bxt_dmc_ver1_07.bin. Disabling runtime power management.
> [    4.961035] i915 0000:00:02.0: DMC firmware homepage:  
> https://01.org/linuxgraphics/downloads/firmware
> [    4.983194] i915 0000:00:02.0: Direct firmware load for  
> i915/bxt_huc_ver01_07_1398.bin failed with error -2
> [    4.983218] [drm] HuC: Failed to fetch firmware  
> i915/bxt_huc_ver01_07_1398.bin (error -2)
> [    4.983224] [drm] HuC: Firmware can be downloaded from  
> https://01.org/linuxgraphics/downloads/firmware
>
>> So can we limit this patch only to s/DRM_INFO/dev_notice ?
>
> This is info, not notice.

Correct.

> It's not a significant condition as that's the
> "ok, load failed, but we can survive with little change" before and this
> is just the information where to find the firmware (outside of their
> distro packing linux-firmwares which presumably was out of date).
> -Chris