| Message ID | 20171022101550.14310-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 22 Oct 2017 12:15:50 +0200, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Just show the firmware URL on the first failure, not every failure. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@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). 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. So can we limit this patch only to s/DRM_INFO/dev_notice ? Michal > release_firmware(fw); /* OK even if fw is NULL */ > } > diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h > b/drivers/gpu/drm/i915/intel_uc_fw.h > index 132903669391..e18b9bedab02 100644 > --- a/drivers/gpu/drm/i915/intel_uc_fw.h > +++ b/drivers/gpu/drm/i915/intel_uc_fw.h > @@ -29,9 +29,6 @@ struct drm_printer; > struct drm_i915_private; > struct i915_vma; > -/* Home of GuC, HuC and DMC firmwares */ > -#define INTEL_UC_FIRMWARE_URL > "https://01.org/linuxgraphics/downloads/firmware" > - > enum intel_uc_fw_status { > INTEL_UC_FIRMWARE_FAIL = -1, > INTEL_UC_FIRMWARE_NONE = 0, > @@ -118,4 +115,6 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, > void intel_uc_fw_fini(struct intel_uc_fw *uc_fw); > void intel_uc_fw_dump(struct intel_uc_fw *uc_fw, struct drm_printer *p); > +void intel_uc_fw_show_url(struct drm_i915_private *i915); > + > #endif
Quoting Michal Wajdeczko (2017-10-23 13:04:49) > On Sun, 22 Oct 2017 12:15:50 +0200, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > > > Just show the firmware URL on the first failure, not every failure. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Michal Wajdeczko <michal.wajdeczko@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
On Mon, 23 Oct 2017 14:15:06 +0200, Chris Wilson <chris@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@chris-wilson.co.uk> wrote: >> >> > Just show the firmware URL on the first failure, not every failure. >> > >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> > Cc: Michal Wajdeczko <michal.wajdeczko@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
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); release_firmware(fw); /* OK even if fw is NULL */ } diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h index 132903669391..e18b9bedab02 100644 --- a/drivers/gpu/drm/i915/intel_uc_fw.h +++ b/drivers/gpu/drm/i915/intel_uc_fw.h @@ -29,9 +29,6 @@ struct drm_printer; struct drm_i915_private; struct i915_vma; -/* Home of GuC, HuC and DMC firmwares */ -#define INTEL_UC_FIRMWARE_URL "https://01.org/linuxgraphics/downloads/firmware" - enum intel_uc_fw_status { INTEL_UC_FIRMWARE_FAIL = -1, INTEL_UC_FIRMWARE_NONE = 0, @@ -118,4 +115,6 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, void intel_uc_fw_fini(struct intel_uc_fw *uc_fw); void intel_uc_fw_dump(struct intel_uc_fw *uc_fw, struct drm_printer *p); +void intel_uc_fw_show_url(struct drm_i915_private *i915); + #endif
Just show the firmware URL on the first failure, not every failure. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@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(-)