diff mbox series

[03/11] drm/i915/uc: introduce intel_uc_fw_supported

Message ID 20190713100016.8026-3-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show
Series [01/11] drm/i915/guc: Use system workqueue for log capture | expand

Commit Message

Chris Wilson July 13, 2019, 10 a.m. UTC
From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Instead of always checking in the device config is GuC and HuC are
supported or not, we can save the state in the uc_fw structure and
avoid going through i915 every time from the low-level uc management
code. while at it FIRMWARE_NONE has been renamed to better indicate that
we haven't started the fetch/load yet, but we might have already selected
a blob.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_fw.c |  6 +++++-
 drivers/gpu/drm/i915/intel_huc_fw.c |  6 +++++-
 drivers/gpu/drm/i915/intel_uc.c     | 25 ++++++++++++------------
 drivers/gpu/drm/i915/intel_uc_fw.c  |  4 +++-
 drivers/gpu/drm/i915/intel_uc_fw.h  | 30 ++++++++++++++++++++++++-----
 5 files changed, 51 insertions(+), 20 deletions(-)

Comments

Chris Wilson July 13, 2019, 10:19 a.m. UTC | #1
Quoting Chris Wilson (2019-07-13 11:00:08)
> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> Instead of always checking in the device config is GuC and HuC are
> supported or not, we can save the state in the uc_fw structure and
> avoid going through i915 every time from the low-level uc management
> code. while at it FIRMWARE_NONE has been renamed to better indicate that
> we haven't started the fetch/load yet, but we might have already selected
> a blob.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>

Ok, but I'm not quite getting the feeling of a nice flow through a state
machine.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Daniele Ceraolo Spurio July 13, 2019, 4:51 p.m. UTC | #2
On 7/13/2019 3:19 AM, Chris Wilson wrote:
> Quoting Chris Wilson (2019-07-13 11:00:08)
>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>
>> Instead of always checking in the device config is GuC and HuC are
>> supported or not, we can save the state in the uc_fw structure and
>> avoid going through i915 every time from the low-level uc management
>> code. while at it FIRMWARE_NONE has been renamed to better indicate that
>> we haven't started the fetch/load yet, but we might have already selected
>> a blob.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Ok, but I'm not quite getting the feeling of a nice flow through a state
> machine.
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris

We had discussed a couple of possible different approaches with Michal 
on the other thread, including a better state machine that unifies the 
fetch/load cases, I just didn't have time to try them yet. Since the 
series is fully reviewed, if you want to get it in while it still 
applies I will follow up with that rework on top.

Daniele

> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson July 13, 2019, 5:06 p.m. UTC | #3
Quoting Daniele Ceraolo Spurio (2019-07-13 17:51:02)
> 
> 
> On 7/13/2019 3:19 AM, Chris Wilson wrote:
> > Quoting Chris Wilson (2019-07-13 11:00:08)
> >> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >>
> >> Instead of always checking in the device config is GuC and HuC are
> >> supported or not, we can save the state in the uc_fw structure and
> >> avoid going through i915 every time from the low-level uc management
> >> code. while at it FIRMWARE_NONE has been renamed to better indicate that
> >> we haven't started the fetch/load yet, but we might have already selected
> >> a blob.
> >>
> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Ok, but I'm not quite getting the feeling of a nice flow through a state
> > machine.
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > -Chris
> 
> We had discussed a couple of possible different approaches with Michal 
> on the other thread, including a better state machine that unifies the 
> fetch/load cases, I just didn't have time to try them yet. Since the 
> series is fully reviewed, if you want to get it in while it still 
> applies I will follow up with that rework on top.

Aye, more than happy with incremental improvements :)
-Chris
Michal Wajdeczko July 13, 2019, 5:43 p.m. UTC | #4
On Sat, 13 Jul 2019 19:06:40 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Daniele Ceraolo Spurio (2019-07-13 17:51:02)
>>
>>
>> On 7/13/2019 3:19 AM, Chris Wilson wrote:
>> > Quoting Chris Wilson (2019-07-13 11:00:08)
>> >> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> >>
>> >> Instead of always checking in the device config is GuC and HuC are
>> >> supported or not, we can save the state in the uc_fw structure and
>> >> avoid going through i915 every time from the low-level uc management
>> >> code. while at it FIRMWARE_NONE has been renamed to better indicate  
>> that
>> >> we haven't started the fetch/load yet, but we might have already  
>> selected
>> >> a blob.
>> >>
>> >> Signed-off-by: Daniele Ceraolo Spurio  
>> <daniele.ceraolospurio@intel.com>
>> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> > Ok, but I'm not quite getting the feeling of a nice flow through a  
>> state
>> > machine.
>> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > -Chris
>>
>> We had discussed a couple of possible different approaches with Michal
>> on the other thread, including a better state machine that unifies the
>> fetch/load cases, I just didn't have time to try them yet. Since the
>> series is fully reviewed, if you want to get it in while it still
>> applies I will follow up with that rework on top.
>
> Aye, more than happy with incremental improvements :)

Good to know, as few other improvements are waiting ;)

Michal
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
index db1e0daca7db..ee95d4960c5c 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/intel_guc_fw.c
@@ -79,8 +79,12 @@  static void guc_fw_select(struct intel_uc_fw *guc_fw)
 
 	GEM_BUG_ON(guc_fw->type != INTEL_UC_FW_TYPE_GUC);
 
-	if (!HAS_GUC(i915))
+	if (!HAS_GUC(i915)) {
+		guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
 		return;
+	}
+
+	guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
 
 	if (i915_modparams.guc_firmware_path) {
 		guc_fw->path = i915_modparams.guc_firmware_path;
diff --git a/drivers/gpu/drm/i915/intel_huc_fw.c b/drivers/gpu/drm/i915/intel_huc_fw.c
index 05cbf8338f53..06e726ba9863 100644
--- a/drivers/gpu/drm/i915/intel_huc_fw.c
+++ b/drivers/gpu/drm/i915/intel_huc_fw.c
@@ -73,8 +73,12 @@  static void huc_fw_select(struct intel_uc_fw *huc_fw)
 
 	GEM_BUG_ON(huc_fw->type != INTEL_UC_FW_TYPE_HUC);
 
-	if (!HAS_HUC(dev_priv))
+	if (!HAS_HUC(dev_priv)) {
+		huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
 		return;
+	}
+
+	huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
 
 	if (i915_modparams.huc_firmware_path) {
 		huc_fw->path = i915_modparams.huc_firmware_path;
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 00baaccc2f1c..e2b20f8e88cf 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -75,7 +75,8 @@  static int __get_default_guc_log_level(struct drm_i915_private *i915)
 {
 	int guc_log_level;
 
-	if (!HAS_GUC(i915) || !intel_uc_is_using_guc(i915))
+	if (!intel_uc_fw_supported(&i915->guc.fw) ||
+	    !intel_uc_is_using_guc(i915))
 		guc_log_level = GUC_LOG_LEVEL_DISABLED;
 	else if (IS_ENABLED(CONFIG_DRM_I915_DEBUG) ||
 		 IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
@@ -123,16 +124,16 @@  static void sanitize_options_early(struct drm_i915_private *i915)
 	if (intel_uc_is_using_guc(i915) && !intel_uc_fw_is_selected(guc_fw)) {
 		DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
 			 "enable_guc", i915_modparams.enable_guc,
-			 !HAS_GUC(i915) ? "no GuC hardware" :
-					  "no GuC firmware");
+			 !intel_uc_fw_supported(guc_fw) ?
+				"no GuC hardware" : "no GuC firmware");
 	}
 
 	/* Verify HuC firmware availability */
 	if (intel_uc_is_using_huc(i915) && !intel_uc_fw_is_selected(huc_fw)) {
 		DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
 			 "enable_guc", i915_modparams.enable_guc,
-			 !HAS_HUC(i915) ? "no HuC hardware" :
-					  "no HuC firmware");
+			 !intel_uc_fw_supported(huc_fw) ?
+				"no HuC hardware" : "no HuC firmware");
 	}
 
 	/* XXX: GuC submission is unavailable for now */
@@ -152,8 +153,8 @@  static void sanitize_options_early(struct drm_i915_private *i915)
 	if (i915_modparams.guc_log_level > 0 && !intel_uc_is_using_guc(i915)) {
 		DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
 			 "guc_log_level", i915_modparams.guc_log_level,
-			 !HAS_GUC(i915) ? "no GuC hardware" :
-					  "GuC not enabled");
+			 !intel_uc_fw_supported(guc_fw) ?
+				"no GuC hardware" : "GuC not enabled");
 		i915_modparams.guc_log_level = 0;
 	}
 
@@ -380,7 +381,7 @@  int intel_uc_init(struct drm_i915_private *i915)
 	if (!USES_GUC(i915))
 		return 0;
 
-	if (!HAS_GUC(i915))
+	if (!intel_uc_fw_supported(&guc->fw))
 		return -ENODEV;
 
 	/* XXX: GuC submission is unavailable for now */
@@ -423,7 +424,7 @@  void intel_uc_fini(struct drm_i915_private *i915)
 	if (!USES_GUC(i915))
 		return;
 
-	GEM_BUG_ON(!HAS_GUC(i915));
+	GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw));
 
 	if (USES_GUC_SUBMISSION(i915))
 		intel_guc_submission_fini(guc);
@@ -439,7 +440,7 @@  static void __uc_sanitize(struct drm_i915_private *i915)
 	struct intel_guc *guc = &i915->guc;
 	struct intel_huc *huc = &i915->huc;
 
-	GEM_BUG_ON(!HAS_GUC(i915));
+	GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw));
 
 	intel_huc_sanitize(huc);
 	intel_guc_sanitize(guc);
@@ -464,7 +465,7 @@  int intel_uc_init_hw(struct drm_i915_private *i915)
 	if (!USES_GUC(i915))
 		return 0;
 
-	GEM_BUG_ON(!HAS_GUC(i915));
+	GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw));
 
 	guc_reset_interrupts(guc);
 
@@ -561,7 +562,7 @@  void intel_uc_fini_hw(struct drm_i915_private *i915)
 	if (!intel_guc_is_loaded(guc))
 		return;
 
-	GEM_BUG_ON(!HAS_GUC(i915));
+	GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw));
 
 	if (USES_GUC_SUBMISSION(i915))
 		intel_guc_submission_disable(guc);
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
index f342ddd47df8..8ce7210907c0 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/intel_uc_fw.c
@@ -47,6 +47,8 @@  void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 	size_t size;
 	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",
@@ -328,7 +330,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_NONE;
+	uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h
index 24e66469153c..833d04d06576 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/intel_uc_fw.h
@@ -26,6 +26,7 @@ 
 #define _INTEL_UC_FW_H_
 
 #include <linux/types.h>
+#include "i915_gem.h"
 
 struct drm_printer;
 struct drm_i915_private;
@@ -34,8 +35,10 @@  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_NONE = 0,
+	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
 };
@@ -79,10 +82,14 @@  static inline
 const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
 {
 	switch (status) {
+	case INTEL_UC_FIRMWARE_NOT_SUPPORTED:
+		return "N/A - uc HW not available";
 	case INTEL_UC_FIRMWARE_FAIL:
 		return "FAIL";
-	case INTEL_UC_FIRMWARE_NONE:
-		return "NONE";
+	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:
@@ -106,9 +113,15 @@  static inline
 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
+	 * 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_NONE;
-	uc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
+	uc_fw->fetch_status = INTEL_UC_FIRMWARE_UNINITIALIZED;
+	uc_fw->load_status = INTEL_UC_FIRMWARE_NOT_STARTED;
 	uc_fw->type = type;
 }
 
@@ -122,6 +135,13 @@  static inline bool intel_uc_fw_is_loaded(struct intel_uc_fw *uc_fw)
 	return uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS;
 }
 
+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;
+}
+
 static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
 {
 	if (intel_uc_fw_is_loaded(uc_fw))