diff mbox series

[5/9] drm/i915/uc: Unify uc_fw status tracking

Message ID 20190722232048.9970-6-daniele.ceraolospurio@intel.com (mailing list archive)
State New, archived
Headers show
Series uC fw path unification + misc clean-up | expand

Commit Message

Daniele Ceraolo Spurio July 22, 2019, 11:20 p.m. UTC 
We currently track fetch and load status separately, but the 2 are
actually sequential in the uc lifetime (fetch must complete before we
can attempt the load!). Unifying the 2 variables we can better follow
the sequential states and improve our trackng of the uC state.

Also, sprinkle some GEM_BUG_ON to make sure we transition correctly
between states.

Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/uc/intel_huc.c   |  4 +-
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 74 ++++++++++--------------
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 50 ++++++++--------
 3 files changed, 57 insertions(+), 71 deletions(-)

Comments

Chris Wilson July 23, 2019, 8:28 a.m. UTC | #1 
Quoting Daniele Ceraolo Spurio (2019-07-23 00:20:44)
> @@ -219,19 +207,17 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>  
>         uc_fw->obj = obj;
>         uc_fw->size = fw->size;
> -       uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
> -       DRM_DEBUG_DRIVER("%s fw fetch %s\n",
> -                        intel_uc_fw_type_repr(uc_fw->type),
> -                        intel_uc_fw_status_repr(uc_fw->fetch_status));
> +       uc_fw->status = INTEL_UC_FIRMWARE_FETCHED;
> +       DRM_DEBUG_DRIVER("%s fw fetch done\n",
> +                        intel_uc_fw_type_repr(uc_fw->type));

I don't see much value in the success message, they just get followed by
more success messages. Pointless spam imo.
-Chris
Michal Wajdeczko July 23, 2019, 2:20 p.m. UTC | #2 
On Tue, 23 Jul 2019 01:20:44 +0200, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@intel.com> wrote:

> We currently track fetch and load status separately, but the 2 are
> actually sequential in the uc lifetime (fetch must complete before we
> can attempt the load!). Unifying the 2 variables we can better follow
> the sequential states and improve our trackng of the uC state.
>
> Also, sprinkle some GEM_BUG_ON to make sure we transition correctly
> between states.
>
> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_huc.c   |  4 +-
>  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 74 ++++++++++--------------
>  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 50 ++++++++--------
>  3 files changed, 57 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c  
> b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> index bc14439173d7..1868f676d890 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> @@ -117,7 +117,7 @@ int intel_huc_auth(struct intel_huc *huc)
>  	struct intel_guc *guc = &gt->uc.guc;
>  	int ret;
> -	if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> +	if (huc->fw.status != INTEL_UC_FIRMWARE_LOADED)

iirc we have dedicated helper intel_uc_fw_is_loaded() for this

>  		return -ENOEXEC;
> 	ret = intel_guc_auth_huc(guc,
> @@ -141,7 +141,7 @@ int intel_huc_auth(struct intel_huc *huc)
>  	return 0;
> fail:
> -	huc->fw.load_status = INTEL_UC_FIRMWARE_FAIL;
> +	huc->fw.status = INTEL_UC_FIRMWARE_LOAD_FAIL;

hmm, so after we load bad HuC fw but before we authenticate it
we will report it as loaded (aka successful)?

maybe in addition to 'loaded' status there should add extra
'authenticated/running' status ? note that we can also load
the guc but then it can still not boot due to bad signature

> 	DRM_ERROR("HuC: Authentication failed %d\n", ret);
>  	return ret;

/.../

> @@ -95,23 +95,11 @@ void intel_uc_fw_fetch(struct drm_i915_private  
> *dev_priv,
>  	int err;
> 	GEM_BUG_ON(!intel_uc_fw_supported(uc_fw));
> -
> -	if (!uc_fw->path) {
> -		dev_info(dev_priv->drm.dev,
> -			 "%s: No firmware was defined for %s!\n",
> -			 intel_uc_fw_type_repr(uc_fw->type),
> -			 intel_platform_name(INTEL_INFO(dev_priv)->platform));
> -		return;
> -	}
> +	GEM_BUG_ON(!intel_uc_fw_is_selected(uc_fw));
> 	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
>  			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);

I guess we can drop this debug log as request_firmware() will print
fw path on failure and actual loaded fw is printed elsewhere later

> -	uc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
> -	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
> -			 intel_uc_fw_type_repr(uc_fw->type),
> -			 intel_uc_fw_status_repr(uc_fw->fetch_status));
> -
>  	err = request_firmware(&fw, uc_fw->path, &pdev->dev);
>  	if (err) {
>  		DRM_DEBUG_DRIVER("%s fw request_firmware err=%d\n",
> @@ -219,19 +207,17 @@ void intel_uc_fw_fetch(struct drm_i915_private  
> *dev_priv,
> 	uc_fw->obj = obj;
>  	uc_fw->size = fw->size;
> -	uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
> -	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
> -			 intel_uc_fw_type_repr(uc_fw->type),
> -			 intel_uc_fw_status_repr(uc_fw->fetch_status));
> +	uc_fw->status = INTEL_UC_FIRMWARE_FETCHED;
> +	DRM_DEBUG_DRIVER("%s fw fetch done\n",
> +			 intel_uc_fw_type_repr(uc_fw->type));
> 	release_firmware(fw);
>  	return;
> fail:
> -	uc_fw->fetch_status = INTEL_UC_FIRMWARE_FAIL;
> -	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
> -			 intel_uc_fw_type_repr(uc_fw->type),
> -			 intel_uc_fw_status_repr(uc_fw->fetch_status));
> +	uc_fw->status = INTEL_UC_FIRMWARE_FETCH_FAIL;
> +	DRM_DEBUG_DRIVER("%s fw fetch failed\n",
> +			 intel_uc_fw_type_repr(uc_fw->type));
> 	DRM_WARN("%s: Failed to fetch firmware %s (error %d)\n",

I'm wondering if we want to keep our above messages as request_firmware()
will report something like this

[   12.198037] i915 0000:00:02.0: Direct firmware load for i915/XXX failed  
with error -2


>  		 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
> @@ -287,13 +273,11 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
>  	DRM_DEBUG_DRIVER("%s fw load %s\n",
>  			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
> -	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> -		return -ENOEXEC;
> +	/* make sure the status was cleared the last time we reset the uc */
> +	GEM_BUG_ON(uc_fw->status == INTEL_UC_FIRMWARE_LOADED);
> -	uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
> -	DRM_DEBUG_DRIVER("%s fw load %s\n",
> -			 intel_uc_fw_type_repr(uc_fw->type),
> -			 intel_uc_fw_status_repr(uc_fw->load_status));
> +	if (uc_fw->status < INTEL_UC_FIRMWARE_FETCHED)
> +		return -ENOEXEC;
> 	/* Call custom loader */
>  	intel_uc_fw_ggtt_bind(uc_fw);
> @@ -302,10 +286,9 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
>  	if (err)
>  		goto fail;
> -	uc_fw->load_status = INTEL_UC_FIRMWARE_SUCCESS;
> -	DRM_DEBUG_DRIVER("%s fw load %s\n",
> -			 intel_uc_fw_type_repr(uc_fw->type),
> -			 intel_uc_fw_status_repr(uc_fw->load_status));
> +	uc_fw->status = INTEL_UC_FIRMWARE_LOADED;
> +	DRM_DEBUG_DRIVER("%s fw load completed\n",
> +			 intel_uc_fw_type_repr(uc_fw->type));
> 	DRM_INFO("%s: Loaded firmware %s (version %u.%u)\n",
>  		 intel_uc_fw_type_repr(uc_fw->type),
> @@ -315,10 +298,9 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
>  	return 0;
> fail:
> -	uc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;
> -	DRM_DEBUG_DRIVER("%s fw load %s\n",
> -			 intel_uc_fw_type_repr(uc_fw->type),
> -			 intel_uc_fw_status_repr(uc_fw->load_status));
> +	uc_fw->status = INTEL_UC_FIRMWARE_LOAD_FAIL;
> +	DRM_DEBUG_DRIVER("%s fw load failed\n",
> +			 intel_uc_fw_type_repr(uc_fw->type));

maybe for debug purposes we can add helper like

static inline void intel_uc_fw_set_status(uc_fw, status)
{
#ifdef CONFIG_DRM_I915_DEBUG_GUC
	DRM_DEBUG_DRIVER("%s: %s -> %s\n",
		intel_uc_fw_type_repr(uc_fw->type),
		intel_uc_fw_type_repr(uc_fw->status)
		intel_uc_fw_type_repr(status));
#endif
	uc_fw->status = status;
}


> 	DRM_WARN("%s: Failed to load firmware %s (error %d)\n",
>  		 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
> @@ -330,7 +312,10 @@ int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
>  {
>  	int err;
> -	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> +	/* this should happen before the load! */
> +	GEM_BUG_ON(uc_fw->status == INTEL_UC_FIRMWARE_LOADED);
> +
> +	if (uc_fw->status < INTEL_UC_FIRMWARE_FETCHED)
>  		return -ENOEXEC;
> 	err = i915_gem_object_pin_pages(uc_fw->obj);
> @@ -343,7 +328,7 @@ int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
> void intel_uc_fw_fini(struct intel_uc_fw *uc_fw)
>  {
> -	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> +	if (uc_fw->status < INTEL_UC_FIRMWARE_FETCHED)
>  		return;
> 	i915_gem_object_unpin_pages(uc_fw->obj);
> @@ -377,7 +362,7 @@ void intel_uc_fw_cleanup_fetch(struct intel_uc_fw  
> *uc_fw)
>  	if (obj)
>  		i915_gem_object_put(obj);
> -	uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
> +	uc_fw->status = INTEL_UC_FIRMWARE_SELECTION_DONE;
>  }
> /**
> @@ -391,9 +376,8 @@ void intel_uc_fw_dump(const struct intel_uc_fw  
> *uc_fw, struct drm_printer *p)
>  {
>  	drm_printf(p, "%s firmware: %s\n",
>  		   intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
> -	drm_printf(p, "\tstatus: fetch %s, load %s\n",
> -		   intel_uc_fw_status_repr(uc_fw->fetch_status),
> -		   intel_uc_fw_status_repr(uc_fw->load_status));
> +	drm_printf(p, "\tstatus: %s\n",
> +		   intel_uc_fw_status_repr(uc_fw->status));
>  	drm_printf(p, "\tversion: wanted %u.%u, found %u.%u\n",
>  		   uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted,
>  		   uc_fw->major_ver_found, uc_fw->minor_ver_found);
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h  
> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> index 550b626afb15..e0c5a38685e6 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> @@ -36,12 +36,13 @@ struct drm_i915_private;
>  #define INTEL_UC_FIRMWARE_URL  
> "https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/i915"
> enum intel_uc_fw_status {
> -	INTEL_UC_FIRMWARE_NOT_SUPPORTED = -2, /* no uc HW */
> -	INTEL_UC_FIRMWARE_FAIL = -1,
> +	INTEL_UC_FIRMWARE_LOAD_FAIL = -3,
> +	INTEL_UC_FIRMWARE_FETCH_FAIL = -2,
> +	INTEL_UC_FIRMWARE_NOT_SUPPORTED = -1, /* no uc HW */
>  	INTEL_UC_FIRMWARE_UNINITIALIZED = 0, /* used to catch checks done too  
> early */
> -	INTEL_UC_FIRMWARE_NOT_STARTED = 1,
> -	INTEL_UC_FIRMWARE_PENDING,
> -	INTEL_UC_FIRMWARE_SUCCESS
> +	INTEL_UC_FIRMWARE_SELECTION_DONE, /* selection include the "no FW"  
> case */
> +	INTEL_UC_FIRMWARE_FETCHED,
> +	INTEL_UC_FIRMWARE_LOADED

I was working on something similar but my names were:

	UNINITIALIZED
	|	\--> N/A (no hw or disabled)
	DEFINED (aka selection done)
	|	\-->  MISSING (aka fetch failed)
	AVAILABLE (aka fetched)
	||	\--> FAILED
	UPLOADED
	RUNNING (authenticated)

I was also trying to add extra flag like .auto_selected to decide
if continue with graceful fallback if something went wrong

/.../

> @@ -179,7 +181,7 @@ static inline void intel_uc_fw_sanitize(struct  
> intel_uc_fw *uc_fw)
>   */
>  static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw)
>  {
> -	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> +	if (uc_fw->status < INTEL_UC_FIRMWARE_FETCHED)
>  		return 0;
> 	return uc_fw->header_size + uc_fw->ucode_size;

btw, maybe we should add GEM_BUG_ON here too ?
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
index bc14439173d7..1868f676d890 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
@@ -117,7 +117,7 @@  int intel_huc_auth(struct intel_huc *huc)
 	struct intel_guc *guc = &gt->uc.guc;
 	int ret;
 
-	if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
+	if (huc->fw.status != INTEL_UC_FIRMWARE_LOADED)
 		return -ENOEXEC;
 
 	ret = intel_guc_auth_huc(guc,
@@ -141,7 +141,7 @@  int intel_huc_auth(struct intel_huc *huc)
 	return 0;
 
 fail:
-	huc->fw.load_status = INTEL_UC_FIRMWARE_FAIL;
+	huc->fw.status = INTEL_UC_FIRMWARE_LOAD_FAIL;
 
 	DRM_ERROR("HuC: Authentication failed %d\n", ret);
 	return ret;
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index faa96748aed3..91b1d9663481 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -45,15 +45,13 @@  void intel_uc_fw_select(struct drm_i915_private *i915,
 			const struct intel_uc_platform_requirement *list,
 			unsigned int length)
 {
-	GEM_BUG_ON(uc_fw->fetch_status != INTEL_UC_FIRMWARE_UNINITIALIZED);
+	GEM_BUG_ON(uc_fw->status != INTEL_UC_FIRMWARE_UNINITIALIZED);
 
 	if (!HAS_UC(i915)) {
-		uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
+		uc_fw->status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
 		return;
 	}
 
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
-
 	if (unlikely(i915_modparams.guc_firmware_path &&
 		     uc_fw->type == INTEL_UC_FW_TYPE_GUC)) {
 		uc_fw->path = i915_modparams.guc_firmware_path;
@@ -74,6 +72,8 @@  void intel_uc_fw_select(struct drm_i915_private *i915,
 			}
 		}
 	}
+
+	uc_fw->status = INTEL_UC_FIRMWARE_SELECTION_DONE;
 }
 
 /**
@@ -95,23 +95,11 @@  void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 	int err;
 
 	GEM_BUG_ON(!intel_uc_fw_supported(uc_fw));
-
-	if (!uc_fw->path) {
-		dev_info(dev_priv->drm.dev,
-			 "%s: No firmware was defined for %s!\n",
-			 intel_uc_fw_type_repr(uc_fw->type),
-			 intel_platform_name(INTEL_INFO(dev_priv)->platform));
-		return;
-	}
+	GEM_BUG_ON(!intel_uc_fw_is_selected(uc_fw));
 
 	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
 			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
 
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
-	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
-			 intel_uc_fw_type_repr(uc_fw->type),
-			 intel_uc_fw_status_repr(uc_fw->fetch_status));
-
 	err = request_firmware(&fw, uc_fw->path, &pdev->dev);
 	if (err) {
 		DRM_DEBUG_DRIVER("%s fw request_firmware err=%d\n",
@@ -219,19 +207,17 @@  void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 
 	uc_fw->obj = obj;
 	uc_fw->size = fw->size;
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
-	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
-			 intel_uc_fw_type_repr(uc_fw->type),
-			 intel_uc_fw_status_repr(uc_fw->fetch_status));
+	uc_fw->status = INTEL_UC_FIRMWARE_FETCHED;
+	DRM_DEBUG_DRIVER("%s fw fetch done\n",
+			 intel_uc_fw_type_repr(uc_fw->type));
 
 	release_firmware(fw);
 	return;
 
 fail:
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_FAIL;
-	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
-			 intel_uc_fw_type_repr(uc_fw->type),
-			 intel_uc_fw_status_repr(uc_fw->fetch_status));
+	uc_fw->status = INTEL_UC_FIRMWARE_FETCH_FAIL;
+	DRM_DEBUG_DRIVER("%s fw fetch failed\n",
+			 intel_uc_fw_type_repr(uc_fw->type));
 
 	DRM_WARN("%s: Failed to fetch firmware %s (error %d)\n",
 		 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
@@ -287,13 +273,11 @@  int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
 	DRM_DEBUG_DRIVER("%s fw load %s\n",
 			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
 
-	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
-		return -ENOEXEC;
+	/* make sure the status was cleared the last time we reset the uc */
+	GEM_BUG_ON(uc_fw->status == INTEL_UC_FIRMWARE_LOADED);
 
-	uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
-	DRM_DEBUG_DRIVER("%s fw load %s\n",
-			 intel_uc_fw_type_repr(uc_fw->type),
-			 intel_uc_fw_status_repr(uc_fw->load_status));
+	if (uc_fw->status < INTEL_UC_FIRMWARE_FETCHED)
+		return -ENOEXEC;
 
 	/* Call custom loader */
 	intel_uc_fw_ggtt_bind(uc_fw);
@@ -302,10 +286,9 @@  int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
 	if (err)
 		goto fail;
 
-	uc_fw->load_status = INTEL_UC_FIRMWARE_SUCCESS;
-	DRM_DEBUG_DRIVER("%s fw load %s\n",
-			 intel_uc_fw_type_repr(uc_fw->type),
-			 intel_uc_fw_status_repr(uc_fw->load_status));
+	uc_fw->status = INTEL_UC_FIRMWARE_LOADED;
+	DRM_DEBUG_DRIVER("%s fw load completed\n",
+			 intel_uc_fw_type_repr(uc_fw->type));
 
 	DRM_INFO("%s: Loaded firmware %s (version %u.%u)\n",
 		 intel_uc_fw_type_repr(uc_fw->type),
@@ -315,10 +298,9 @@  int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
 	return 0;
 
 fail:
-	uc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;
-	DRM_DEBUG_DRIVER("%s fw load %s\n",
-			 intel_uc_fw_type_repr(uc_fw->type),
-			 intel_uc_fw_status_repr(uc_fw->load_status));
+	uc_fw->status = INTEL_UC_FIRMWARE_LOAD_FAIL;
+	DRM_DEBUG_DRIVER("%s fw load failed\n",
+			 intel_uc_fw_type_repr(uc_fw->type));
 
 	DRM_WARN("%s: Failed to load firmware %s (error %d)\n",
 		 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
@@ -330,7 +312,10 @@  int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
 {
 	int err;
 
-	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
+	/* this should happen before the load! */
+	GEM_BUG_ON(uc_fw->status == INTEL_UC_FIRMWARE_LOADED);
+
+	if (uc_fw->status < INTEL_UC_FIRMWARE_FETCHED)
 		return -ENOEXEC;
 
 	err = i915_gem_object_pin_pages(uc_fw->obj);
@@ -343,7 +328,7 @@  int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
 
 void intel_uc_fw_fini(struct intel_uc_fw *uc_fw)
 {
-	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
+	if (uc_fw->status < INTEL_UC_FIRMWARE_FETCHED)
 		return;
 
 	i915_gem_object_unpin_pages(uc_fw->obj);
@@ -377,7 +362,7 @@  void intel_uc_fw_cleanup_fetch(struct intel_uc_fw *uc_fw)
 	if (obj)
 		i915_gem_object_put(obj);
 
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
+	uc_fw->status = INTEL_UC_FIRMWARE_SELECTION_DONE;
 }
 
 /**
@@ -391,9 +376,8 @@  void intel_uc_fw_dump(const struct intel_uc_fw *uc_fw, struct drm_printer *p)
 {
 	drm_printf(p, "%s firmware: %s\n",
 		   intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
-	drm_printf(p, "\tstatus: fetch %s, load %s\n",
-		   intel_uc_fw_status_repr(uc_fw->fetch_status),
-		   intel_uc_fw_status_repr(uc_fw->load_status));
+	drm_printf(p, "\tstatus: %s\n",
+		   intel_uc_fw_status_repr(uc_fw->status));
 	drm_printf(p, "\tversion: wanted %u.%u, found %u.%u\n",
 		   uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted,
 		   uc_fw->major_ver_found, uc_fw->minor_ver_found);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
index 550b626afb15..e0c5a38685e6 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
@@ -36,12 +36,13 @@  struct drm_i915_private;
 #define INTEL_UC_FIRMWARE_URL "https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/i915"
 
 enum intel_uc_fw_status {
-	INTEL_UC_FIRMWARE_NOT_SUPPORTED = -2, /* no uc HW */
-	INTEL_UC_FIRMWARE_FAIL = -1,
+	INTEL_UC_FIRMWARE_LOAD_FAIL = -3,
+	INTEL_UC_FIRMWARE_FETCH_FAIL = -2,
+	INTEL_UC_FIRMWARE_NOT_SUPPORTED = -1, /* no uc HW */
 	INTEL_UC_FIRMWARE_UNINITIALIZED = 0, /* used to catch checks done too early */
-	INTEL_UC_FIRMWARE_NOT_STARTED = 1,
-	INTEL_UC_FIRMWARE_PENDING,
-	INTEL_UC_FIRMWARE_SUCCESS
+	INTEL_UC_FIRMWARE_SELECTION_DONE, /* selection include the "no FW" case */
+	INTEL_UC_FIRMWARE_FETCHED,
+	INTEL_UC_FIRMWARE_LOADED
 };
 
 enum intel_uc_fw_type {
@@ -77,8 +78,7 @@  struct intel_uc_fw {
 	const char *path;
 	size_t size;
 	struct drm_i915_gem_object *obj;
-	enum intel_uc_fw_status fetch_status;
-	enum intel_uc_fw_status load_status;
+	enum intel_uc_fw_status status;
 
 	/*
 	 * The firmware build process will generate a version header file with major and
@@ -103,18 +103,20 @@  static inline
 const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
 {
 	switch (status) {
+	case INTEL_UC_FIRMWARE_LOAD_FAIL:
+		return "LOAD FAIL";
+	case INTEL_UC_FIRMWARE_FETCH_FAIL:
+		return "FETCH FAIL";
 	case INTEL_UC_FIRMWARE_NOT_SUPPORTED:
-		return "N/A - uc HW not available";
-	case INTEL_UC_FIRMWARE_FAIL:
-		return "FAIL";
+		return "N/A";
 	case INTEL_UC_FIRMWARE_UNINITIALIZED:
 		return "UNINITIALIZED";
-	case INTEL_UC_FIRMWARE_NOT_STARTED:
-		return "NOT_STARTED";
-	case INTEL_UC_FIRMWARE_PENDING:
-		return "PENDING";
-	case INTEL_UC_FIRMWARE_SUCCESS:
-		return "SUCCESS";
+	case INTEL_UC_FIRMWARE_SELECTION_DONE:
+		return "SELECTION DONE";
+	case INTEL_UC_FIRMWARE_FETCHED:
+		return "FETCHED";
+	case INTEL_UC_FIRMWARE_LOADED:
+		return "LOADED";
 	}
 	return "<invalid>";
 }
@@ -135,38 +137,38 @@  void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
 			    enum intel_uc_fw_type type)
 {
 	/*
-	 * we use FIRMWARE_UNINITIALIZED to detect checks against fetch_status
+	 * we use FIRMWARE_UNINITIALIZED to detect checks against uc_fw->status
 	 * before we're looked at the HW caps to see if we have uc support
 	 */
 	BUILD_BUG_ON(INTEL_UC_FIRMWARE_UNINITIALIZED);
 
 	uc_fw->path = NULL;
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_UNINITIALIZED;
-	uc_fw->load_status = INTEL_UC_FIRMWARE_NOT_STARTED;
+	uc_fw->status = INTEL_UC_FIRMWARE_UNINITIALIZED;
 	uc_fw->type = type;
 }
 
 static inline bool intel_uc_fw_is_selected(struct intel_uc_fw *uc_fw)
 {
+	GEM_BUG_ON(uc_fw->path && uc_fw->status < INTEL_UC_FIRMWARE_SELECTION_DONE);
 	return uc_fw->path != NULL;
 }
 
 static inline bool intel_uc_fw_is_loaded(struct intel_uc_fw *uc_fw)
 {
-	return uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS;
+	return uc_fw->status == INTEL_UC_FIRMWARE_LOADED;
 }
 
 static inline bool intel_uc_fw_supported(struct intel_uc_fw *uc_fw)
 {
 	/* shouldn't call this before checking hw/blob availability */
-	GEM_BUG_ON(uc_fw->fetch_status == INTEL_UC_FIRMWARE_UNINITIALIZED);
-	return uc_fw->fetch_status != INTEL_UC_FIRMWARE_NOT_SUPPORTED;
+	GEM_BUG_ON(uc_fw->status == INTEL_UC_FIRMWARE_UNINITIALIZED);
+	return uc_fw->status != INTEL_UC_FIRMWARE_NOT_SUPPORTED;
 }
 
 static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
 {
 	if (intel_uc_fw_is_loaded(uc_fw))
-		uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
+		uc_fw->status = INTEL_UC_FIRMWARE_FETCHED;
 }
 
 /**
@@ -179,7 +181,7 @@  static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
  */
 static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw)
 {
-	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
+	if (uc_fw->status < INTEL_UC_FIRMWARE_FETCHED)
 		return 0;
 
 	return uc_fw->header_size + uc_fw->ucode_size;