[Intel-gfx] [PATCH 3/5] drm/i915/guc: Extract param logic form guc_init

[Michal Wajdeczko]

On Tue, Feb 14, 2017 at 05:15:39PM +0100, Arkadiusz Hiler wrote:
> Let intel_guc_init() focus on determining and fetching the correct
> firmware.
> 
> This patch introduces intel_sanitize_uc_params() that is called from
> intel_uc_init().
> 
> Then, if we have GuC, we can call intel_guc_init() conditionally and we
> do not have to do internal checks.
> 
> Cc: Michal Winiarski <michal.winiarski at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c         |  2 ++
>  drivers/gpu/drm/i915/intel_guc_loader.c | 15 +--------------
>  drivers/gpu/drm/i915/intel_uc.c         | 25 ++++++++++++++++++++++++-
>  3 files changed, 27 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6d0798e..e520895 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -994,6 +994,8 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
>  
>  	i915.semaphores = intel_sanitize_semaphores(dev_priv, i915.semaphores);
>  	DRM_DEBUG_DRIVER("use GPU sempahores? %s\n", yesno(i915.semaphores));
> +
> +	intel_uc_sanitize_params(dev_priv);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index f5efe28..db5713c 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -726,7 +726,7 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>  }
>  
>  /**
> - * intel_guc_init() - define parameters and fetch firmware
> + * intel_guc_init() - determine and fetch firmware

Again, can we have function name that corresponds to its purpose.
Comment alone is not sufficient


>   * @dev_priv:	i915 device private
>   *
>   * Called early during driver load, but after GEM is initialised.
> @@ -739,17 +739,6 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
>  	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>  	const char *fw_path;
>  
> -	if (!HAS_GUC(dev_priv)) {
> -		i915.enable_guc_loading = 0;
> -		i915.enable_guc_submission = 0;
> -	} else {
> -		/* A negative value means "use platform default" */
> -		if (i915.enable_guc_loading < 0)
> -			i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
> -		if (i915.enable_guc_submission < 0)
> -			i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> -	}
> -
>  	if (!HAS_GUC_UCODE(dev_priv)) {
>  		fw_path = NULL;
>  	} else if (IS_SKYLAKE(dev_priv)) {
> @@ -773,8 +762,6 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
>  	guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
>  
>  	/* Early (and silent) return if GuC loading is disabled */

I think this comment was related to the line that you just deleted
 

> -	if (!i915.enable_guc_loading)
> -		return;
>  	if (fw_path == NULL)
>  		return;
>  	if (*fw_path == '\0')
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index d9d0566..d1ca41d 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -25,6 +25,24 @@
>  #include "i915_drv.h"
>  #include "intel_uc.h"
>  
> +void intel_uc_sanitize_params(struct drm_i915_private *dev_priv)
> +{
> +	if (!HAS_GUC(dev_priv)) {

Maybe we should warn user if specified explicit guc params need to be nuked?
 

> +		i915.enable_guc_loading = 0;
> +		i915.enable_guc_submission = 0;
> +	} else {
> +		/* A negative value means "use platform default" */
> +		if (i915.enable_guc_loading < 0)
> +			i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
> +		if (i915.enable_guc_submission < 0)
> +			i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> +	}
> +
> +	/* can't enable guc submission without guc loaded */
> +	if (!i915.enable_guc_loading)
> +		i915.enable_guc_submission = 0;
> +}
> +
>  void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  {
>  	mutex_init(&dev_priv->guc.send_mutex);
> @@ -32,7 +50,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  
>  void intel_uc_init(struct drm_i915_private *dev_priv)
>  {
> -	intel_huc_init(dev_priv);
> +	if (!i915.enable_guc_loading)
> +		return;
> +
> +	if (HAS_HUC_UCODE(dev_priv))

Hmm, if I recall correctly, same condition is checked in huc_init()...
Btw, what approach is more preferred? check before call or inside?


Regards,
Michal   

> +		intel_huc_init(dev_priv);
> +
>  	intel_guc_init(dev_priv);
>  }
>  
> -- 
> 2.9.3
>