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

[Chris Wilson]

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.

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

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