[Intel-gfx] [PATCH 5/5] drm/i915/guc: Simplify guc_fw_path

[Tvrtko Ursulin]

On 15/12/2016 15:47, Arkadiusz Hiler wrote:
> Currently guc_fw_path values can represent one of three possible states:
>
>  1) NULL - device without GuC
>  2) '\0' - device with GuC but no known firmware
>  3) else - device with GuC and known firmware
>
> Second case is used only to WARN at the later stage.
>
> We can WARN right away and merge cases 1 and 2 for later handling.
>
> Cc: Anusha Srivatsa <anusha.srivatsa at intel.com>
> Cc: Jeff McGee <jeff.mcgee at intel.com>
> Cc: Michal Winiarski <michal.winiarski at intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_loader.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 0bb5fd1..075a103 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -460,12 +460,8 @@ int intel_guc_load(struct drm_i915_private *dev_priv)
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>
>  	if (fw_path == NULL) {
> -		/* Device is known to have no uCode (e.g. no GuC) */
> +		/* We do not have uCode for the device */
>  		return -ENXIO;
> -	} else if (*fw_path == '\0') {
> -		/* Device has a GuC but we don't know what f/w to load? */
> -		WARN(1, "No GuC firmware known for this platform!\n");
> -		return -ENODEV;
>  	}
>
>  	/* Fetch failed, or already fetched but failed to load? */
> @@ -628,11 +624,9 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
>  void intel_guc_init(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> -	const char *fw_path;
> +	const char *fw_path = NULL;
>
> -	if (!HAS_GUC_UCODE(dev_priv)) {
> -		fw_path = NULL;
> -	} else if (IS_SKYLAKE(dev_priv)) {
> +	if (IS_SKYLAKE(dev_priv)) {
>  		fw_path = I915_SKL_GUC_UCODE;
>  		guc_fw->guc_fw_major_wanted = SKL_FW_MAJOR;
>  		guc_fw->guc_fw_minor_wanted = SKL_FW_MINOR;
> @@ -644,24 +638,22 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
>  		fw_path = I915_KBL_GUC_UCODE;
>  		guc_fw->guc_fw_major_wanted = KBL_FW_MAJOR;
>  		guc_fw->guc_fw_minor_wanted = KBL_FW_MINOR;
> -	} else {
> -		fw_path = "";	/* unknown device */
> +	} else if (HAS_GUC_UCODE(dev_priv)) {
> +		WARN(1, "No GuC firmware known for platform with GuC!\n");

I think you should also set i915.enable_guc_loading to zero here to 
avoid the later stages bothering with partial setup and cleanup.

intel_uc_load specifically I think would with this patch run some setup 
then immediately fail and have to unwind.

Regards,

Tvrtko

>  	}
>
>  	guc_fw->guc_fw_path = fw_path;
>  	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
>  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
>
> -	/* Early (and silent) return if GuC loading is disabled */
> +	/* Early return if we do not have firmware to fetch */
>  	if (fw_path == NULL)
>  		return;
> -	if (*fw_path == '\0')
> -		return;
>
>  	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING;
>  	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
> +
>  	guc_fw_fetch(dev_priv, guc_fw);
> -	/* status must now be FAIL or SUCCESS */
>  }
>
>  /**
>