diff mbox series

[RFC] drm/i915/uc: Log fw status changes only under debug config

Message ID 20190812195125.18924-1-michal.wajdeczko@intel.com (mailing list archive)
State New, archived
Headers show
Series [RFC] drm/i915/uc: Log fw status changes only under debug config | expand

Commit Message

Michal Wajdeczko Aug. 12, 2019, 7:51 p.m. UTC 
We don't care about internal firmware status changes unless
we are doing some real debugging. Note that our CI is not
using DRM_I915_DEBUG_GUC config by default so use it.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c |  4 +-
 drivers/gpu/drm/i915/gt/uc/intel_huc.c    |  4 +-
 drivers/gpu/drm/i915/gt/uc/intel_uc.c     |  2 +-
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c  | 56 ++++++++++++++---------
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h  | 19 ++++++--
 5 files changed, 55 insertions(+), 30 deletions(-)

Comments

Chris Wilson Aug. 12, 2019, 8:08 p.m. UTC | #1 
Quoting Michal Wajdeczko (2019-08-12 20:51:25)
> 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 20a5ddb753c3..885a4d7e4d37 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> @@ -42,7 +42,7 @@ enum intel_uc_fw_type {
>   */
>  struct intel_uc_fw {
>         enum intel_uc_fw_type type;
> -       enum intel_uc_fw_status status;
> +       enum intel_uc_fw_status _status;

What you can do to prevent accidentally writing into the variable, while
keeping fw->status for convenience, is

-       enum intel_uc_fw_status _status;
+       union {
+               const enum intel_uc_fw_status status;
+               enum intel_uc_fw_status __status; /* no accidental overwrites */
+       };

The only catch is that is disallows struct assignment (but memcpy is
still fine).
-Chris
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
index 445dcea51e51..5528224448f6 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
@@ -157,10 +157,10 @@  int intel_guc_fw_upload(struct intel_guc *guc)
 	if (ret)
 		goto out;
 
-	guc->fw.status = INTEL_UC_FIRMWARE_RUNNING;
+	intel_uc_fw_change_status(&guc->fw, INTEL_UC_FIRMWARE_RUNNING);
 	return 0;
 
 out:
-	guc->fw.status = INTEL_UC_FIRMWARE_FAIL;
+	intel_uc_fw_change_status(&guc->fw, INTEL_UC_FIRMWARE_FAIL);
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
index 75dade1cfbca..1edda1657411 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
@@ -147,12 +147,12 @@  int intel_huc_auth(struct intel_huc *huc)
 		goto fail;
 	}
 
-	huc->fw.status = INTEL_UC_FIRMWARE_RUNNING;
+	intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_RUNNING);
 	return 0;
 
 fail:
 	i915_probe_error(gt->i915, "HuC: Authentication failed %d\n", ret);
-	huc->fw.status = INTEL_UC_FIRMWARE_FAIL;
+	intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_FAIL);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 0dc2b0cf4604..73bf673e153b 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -418,7 +418,7 @@  int intel_uc_init_hw(struct intel_uc *uc)
 		return 0;
 
 	if (!intel_uc_fw_is_available(&guc->fw)) {
-		ret = intel_uc_fw_status_to_error(guc->fw.status);
+		ret = intel_uc_fw_status_to_error(guc->fw._status);
 		goto err_out;
 	}
 
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 302eb1b909dc..b71197f3da8a 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -11,6 +11,29 @@ 
 #include "intel_uc_fw_abi.h"
 #include "i915_drv.h"
 
+#ifdef CONFIG_DRM_I915_DEBUG_GUC
+static inline struct intel_gt *__uc_fw_to_gt(struct intel_uc_fw *uc_fw)
+{
+	GEM_BUG_ON(uc_fw->_status == INTEL_UC_FIRMWARE_UNINITIALIZED);
+	if (uc_fw->type == INTEL_UC_FW_TYPE_GUC)
+		return container_of(uc_fw, struct intel_gt, uc.guc.fw);
+
+	GEM_BUG_ON(uc_fw->type != INTEL_UC_FW_TYPE_HUC);
+	return container_of(uc_fw, struct intel_gt, uc.huc.fw);
+}
+
+void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw,
+			       enum intel_uc_fw_status status)
+{
+	uc_fw->_status =  status;
+	DRM_DEV_DEBUG_DRIVER(__uc_fw_to_gt(uc_fw)->i915->drm.dev,
+			     "%s firmware -> %s\n",
+			     intel_uc_fw_type_repr(uc_fw->type),
+			     status == INTEL_UC_FIRMWARE_SELECTED ?
+			     uc_fw->path : intel_uc_fw_status_repr(status));
+}
+#endif
+
 /*
  * List of required GuC and HuC binaries per-platform.
  * Must be ordered based on platform + revid, from newer to older.
@@ -173,7 +196,7 @@  void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
 	 * before we're looked at the HW caps to see if we have uc support
 	 */
 	BUILD_BUG_ON(INTEL_UC_FIRMWARE_UNINITIALIZED);
-	GEM_BUG_ON(uc_fw->status);
+	GEM_BUG_ON(uc_fw->_status);
 	GEM_BUG_ON(uc_fw->path);
 
 	uc_fw->type = type;
@@ -183,10 +206,9 @@  void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
 		__uc_fw_user_override(uc_fw);
 	}
 
-	if (uc_fw->path && *uc_fw->path)
-		uc_fw->status = INTEL_UC_FIRMWARE_SELECTED;
-	else
-		uc_fw->status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
+	intel_uc_fw_change_status(uc_fw, uc_fw->path && *uc_fw->path ?
+				  INTEL_UC_FIRMWARE_SELECTED :
+				  INTEL_UC_FIRMWARE_NOT_SUPPORTED);
 }
 
 static void __force_fw_fetch_failures(struct intel_uc_fw *uc_fw,
@@ -343,20 +365,15 @@  int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw, struct drm_i915_private *i915)
 
 	uc_fw->obj = obj;
 	uc_fw->size = fw->size;
-	uc_fw->status = INTEL_UC_FIRMWARE_AVAILABLE;
-
-	DRM_DEV_DEBUG_DRIVER(dev, "%s firmware %s: %s\n",
-			     intel_uc_fw_type_repr(uc_fw->type), uc_fw->path,
-			     intel_uc_fw_status_repr(uc_fw->status));
+	intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_AVAILABLE);
 
 	release_firmware(fw);
 	return 0;
 
 fail:
-	if (err == -ENOENT)
-		uc_fw->status = INTEL_UC_FIRMWARE_MISSING;
-	else
-		uc_fw->status = INTEL_UC_FIRMWARE_ERROR;
+	intel_uc_fw_change_status(uc_fw, err == -ENOENT ?
+				  INTEL_UC_FIRMWARE_MISSING :
+				  INTEL_UC_FIRMWARE_ERROR);
 
 	dev_notice(dev, "%s firmware %s: fetch failed with error %d\n",
 		   intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
@@ -491,17 +508,14 @@  int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, struct intel_gt *gt,
 	if (err)
 		goto fail;
 
-	uc_fw->status = INTEL_UC_FIRMWARE_TRANSFERRED;
-	DRM_DEV_DEBUG_DRIVER(gt->i915->drm.dev, "%s firmware %s: %s\n",
-			     intel_uc_fw_type_repr(uc_fw->type), uc_fw->path,
-			     intel_uc_fw_status_repr(uc_fw->status));
+	intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_TRANSFERRED);
 	return 0;
 
 fail:
 	i915_probe_error(gt->i915, "Failed to load %s firmware %s (%d)\n",
 			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path,
 			 err);
-	uc_fw->status = INTEL_UC_FIRMWARE_FAIL;
+	intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_FAIL);
 	return err;
 }
 
@@ -546,7 +560,7 @@  void intel_uc_fw_cleanup_fetch(struct intel_uc_fw *uc_fw)
 	if (obj)
 		i915_gem_object_put(obj);
 
-	uc_fw->status = INTEL_UC_FIRMWARE_SELECTED;
+	intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_SELECTED);
 }
 
 /**
@@ -581,7 +595,7 @@  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: %s\n",
-		   intel_uc_fw_status_repr(uc_fw->status));
+		   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 20a5ddb753c3..885a4d7e4d37 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
@@ -42,7 +42,7 @@  enum intel_uc_fw_type {
  */
 struct intel_uc_fw {
 	enum intel_uc_fw_type type;
-	enum intel_uc_fw_status status;
+	enum intel_uc_fw_status _status;
 	const char *path;
 	bool user_overridden;
 	size_t size;
@@ -62,6 +62,17 @@  struct intel_uc_fw {
 	u32 ucode_size;
 };
 
+#ifdef CONFIG_DRM_I915_DEBUG_GUC
+void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw,
+			       enum intel_uc_fw_status status);
+#else
+static inline void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw,
+					     enum intel_uc_fw_status status)
+{
+	uc_fw->_status = status;
+}
+#endif
+
 static inline
 const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
 {
@@ -124,8 +135,8 @@  static inline enum intel_uc_fw_status
 __intel_uc_fw_status(struct intel_uc_fw *uc_fw)
 {
 	/* shouldn't call this before checking hw/blob availability */
-	GEM_BUG_ON(uc_fw->status == INTEL_UC_FIRMWARE_UNINITIALIZED);
-	return uc_fw->status;
+	GEM_BUG_ON(uc_fw->_status == INTEL_UC_FIRMWARE_UNINITIALIZED);
+	return uc_fw->_status;
 }
 
 static inline bool intel_uc_fw_is_available(struct intel_uc_fw *uc_fw)
@@ -156,7 +167,7 @@  static inline bool intel_uc_fw_is_overridden(const struct intel_uc_fw *uc_fw)
 static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
 {
 	if (intel_uc_fw_is_loaded(uc_fw))
-		uc_fw->status = INTEL_UC_FIRMWARE_AVAILABLE;
+		intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_AVAILABLE);
 }
 
 /**