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

[Michal Wajdeczko]

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