On Mon, 20 May 2019 11:35:26 +0200, Chris Wilson
<chris@xxxxxxxxxxxxxxxxxx> wrote:
Quoting Michal Wajdeczko (2019-05-19 22:50:43)
If we never attempted to load HuC firmware, or we already wedged
or reset GuC/HuC, then there is no reason to wake up the device
to check one bit in the register that will be for sure cleared.
v2: check if HuC was enabled; subtle change in ABI
reuse hus_is_load helper
Suggested-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Tony Ye <tony.ye@xxxxxxxxx>
---
drivers/gpu/drm/i915/intel_huc.c | 20 ++++++++++++--------
drivers/gpu/drm/i915/intel_huc.h | 5 +++++
2 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_huc.c
b/drivers/gpu/drm/i915/intel_huc.c
index 1ff1fb015e58..bfdebec1cfc8 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -113,7 +113,7 @@ int intel_huc_auth(struct intel_huc *huc)
u32 status;
int ret;
- if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
+ if (!intel_huc_is_loaded(huc))
return -ENOEXEC;
ret = intel_guc_auth_huc(guc,
@@ -150,21 +150,25 @@ int intel_huc_auth(struct intel_huc *huc)
* This function reads status register to verify if HuC
* firmware was successfully loaded.
*
- * Returns: 1 if HuC firmware is loaded and verified,
- * 0 if HuC firmware is not loaded and -ENODEV if HuC
- * is not present on this platform.
+ * Returns: 1 if HuC firmware is loaded and verified, 0 if HuC
firmware is not
+ * enabled, -ENOPKG if HuC firmware is not loaded and -ENODEV if HuC
is not
+ * present on this platform.
*/
int intel_huc_check_status(struct intel_huc *huc)
{
struct drm_i915_private *dev_priv = huc_to_i915(huc);
intel_wakeref_t wakeref;
- bool status = false;
+ bool verified = false;
if (!HAS_HUC(dev_priv))
return -ENODEV;
- with_intel_runtime_pm(dev_priv, wakeref)
- status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
+ if (!USES_HUC(dev_priv))
+ return 0;
Hmm, EOPNOTSUPP here for the user disabled case?
I'm not sure that user disabled case shall be reported as error,
since it perfectly fits into legacy ABI 0="not loaded".
The only small change is that now 0="not loaded (not enabled by user)"
Then
if (!intel_huc_is_loaded(huc))
return -ENOPKG;
bool verified = false;
with_intel_runtime_pm(dev_priv, wakeref)
verified = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
return verified.
That keeps it a bit flatter with the special casing separate and the 0/1
result as given by the register status. Then if negative, we know one of
the preconditions for using HuC is not available, and if 0 something went
wrong in mmio setup.
Does that make sense?
IMHO, if something went wrong it is better to report it as error rather
then weak 0 status. Compare:
+---------------+-------+-------+-------+
no hardware -ENODEV -ENODEV -ENODEV
fw disabled 0 0 -ENOTSUP
fw not loaded 0 -ENOPKG -ENOPKG
fw not verified* 0 -ENOPKG 0
fw verified* 1 1 1
+---------------+-------+-------+-------+
*) registry access
Note that in your case, fw verification problem can be reported both
as -ENOPKG or as 0 (former when verification fails at load time, latter
as result of runtime reg read). Very likely we will never see 0 at all,
since probably that can't change once fw was verified at load time.
That might be viewed as undesired ABI change.
- return status;
+ if (intel_huc_is_loaded(huc))
+ with_intel_runtime_pm(dev_priv, wakeref)
+ verified = I915_READ(HUC_STATUS2) &
HUC_FW_VERIFIED;
+
+ return verified ? 1 : -ENOPKG;
}
diff --git a/drivers/gpu/drm/i915/intel_huc.h
b/drivers/gpu/drm/i915/intel_huc.h
index a0c21ae02a99..cde3d303718d 100644
--- a/drivers/gpu/drm/i915/intel_huc.h
+++ b/drivers/gpu/drm/i915/intel_huc.h
@@ -44,6 +44,11 @@ void intel_huc_fini(struct intel_huc *huc);
int intel_huc_auth(struct intel_huc *huc);
int intel_huc_check_status(struct intel_huc *huc);
+static inline bool intel_huc_is_loaded(struct intel_huc *huc)
+{
+ return intel_uc_fw_is_loaded(&huc->fw);
+}
+
static inline void intel_huc_fini_misc(struct intel_huc *huc)
{
intel_uc_fw_cleanup_fetch(&huc->fw);
--
2.19.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
