[Intel-gfx] [PATCH v7 05/21] drm/i915/slpc: Sanitize GuC version

[Michal Wajdeczko]

On Wed, Mar 22, 2017 at 03:33:38PM +0530, Sagar Arun Kamble wrote:
> From: Tom O'Rourke <Tom.O'Rourke at intel.com>
> 
> The SLPC interface is dependent on GuC version.
> Only GuC versions known to be compatible are supported here.
> 
> SLPC with GuC firmware v9 is supported with this series.
> 
> v1: Updated with modified sanitize_slpc_option in earlier patch.
> 
> v2-v3: Rebase.
> 
> v4: Updated support for GuC firmware v9.
> 
> v5: Commit subject updated.
> 
> v6: Commit subject and message update. Add support condition as >=v9.
> 
> v7: Sanitizing GuC version in intel_uc_init_fw for SLPC compatibility.
>     Added info. print for needed version and pointer to 01.org.
> 
> Signed-off-by: Tom O'Rourke <Tom.O'Rourke at intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_csr.c |  5 -----
>  drivers/gpu/drm/i915/intel_uc.c  | 14 ++++++++++++--
>  drivers/gpu/drm/i915/intel_uc.h  |  2 ++
>  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 3683225..b784b7b 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -49,11 +49,6 @@
>  MODULE_FIRMWARE(I915_CSR_BXT);
>  #define BXT_CSR_VERSION_REQUIRED	CSR_VERSION(1, 7)
>  
> -#define FIRMWARE_URL  "https://01.org/linuxgraphics/intel-linux-graphics-firmwares"
> -
> -
> -
> -
>  #define CSR_MAX_FW_SIZE			0x2FFF
>  #define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
>  
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index c0a1d14..8502ead 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -101,11 +101,21 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  
>  void intel_uc_init_fw(struct drm_i915_private *dev_priv)
>  {
> +	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
> +
>  	if (dev_priv->huc.fw.path)
>  		intel_uc_prepare_fw(dev_priv, &dev_priv->huc.fw);
>  
> -	if (dev_priv->guc.fw.path)
> -		intel_uc_prepare_fw(dev_priv, &dev_priv->guc.fw);
> +	if (guc_fw->path) {
> +		intel_uc_prepare_fw(dev_priv, guc_fw);
> +
> +		if (guc_fw->major_ver_found < 9) {

In intel_uc_prepare_fw() there is explicit check that 'found' major version
match the 'wanted' major version, thus I think you can do this slpc check
in intel_uc_sanitize_options() right after sucessful call to intel_guc_select_fw()


> +			DRM_INFO("SLPC not supported with GuC firmware v%u,"
> +				 " please use v9+ [" FIRMWARE_URL "].\n",

Hmm, as "9" is used here for the second time, maybe there shall be a definition

	#define I915_SLPC_REQUIRED_GUC_MAJOR 9

Also, if FIRMWARE_URL is used more than once in the driver, maybe to save some driver
footprint it would be better to use %s ?

> +				 guc_fw->major_ver_found);
> +			i915.enable_slpc = 0;
> +		}
> +	}
>  }
>  
>  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index a35eded..928c214 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -130,6 +130,8 @@ struct intel_uc_fw {
>  	uint32_t ucode_offset;
>  };
>  
> +#define FIRMWARE_URL  "https://01.org/linuxgraphics/intel-linux-graphics-firmwares"

Is it ok that we use such generic macro name without any intel/i915 prefix?

Regards,
-Michal