On 4/15/19 3:10 PM, Daniele Ceraolo Spurio wrote:
On 4/15/19 2:44 PM, Michal Wajdeczko wrote:
On Mon, 15 Apr 2019 23:19:40 +0200, Daniele Ceraolo Spurio
<daniele.ceraolospurio@xxxxxxxxx> wrote:
On 4/11/19 1:44 AM, Michal Wajdeczko wrote:
Gen11 defines new register for checking HuC authentication status.
Look into the right register and bit.
BSpec: 19686
Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
Cc: Tony Ye <tony.ye@xxxxxxxxx>
Cc: Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx>
Cc: John Spotswood <john.a.spotswood@xxxxxxxxx>
Cc: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
---
drivers/gpu/drm/i915/intel_guc_reg.h | 3 ++
drivers/gpu/drm/i915/intel_huc.c | 56
++++++++++++++++++++++++----
2 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_guc_reg.h
b/drivers/gpu/drm/i915/intel_guc_reg.h
index d26de5193568..7eba65795b58 100644
--- a/drivers/gpu/drm/i915/intel_guc_reg.h
+++ b/drivers/gpu/drm/i915/intel_guc_reg.h
@@ -79,6 +79,9 @@
#define HUC_STATUS2 _MMIO(0xD3B0)
#define HUC_FW_VERIFIED (1<<7)
+#define GEN11_HUC_KERNEL_LOAD_INFO _MMIO(0xC1DC)
+#define HUC_LOAD_SUCCESSFUL (1 << 0)
+
#define GUC_WOPCM_SIZE _MMIO(0xc050)
#define GUC_WOPCM_SIZE_LOCKED (1<<0)
#define GUC_WOPCM_SIZE_SHIFT 12
diff --git a/drivers/gpu/drm/i915/intel_huc.c
b/drivers/gpu/drm/i915/intel_huc.c
index 94c04f16a2ad..708a4b387259 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -40,6 +40,47 @@ int intel_huc_init_misc(struct intel_huc *huc)
return 0;
}
+static int gen8_huc_wait_verified(struct intel_huc *huc)
why gen8?
+{
+ struct drm_i915_private *i915 = huc_to_i915(huc);
+ u32 status;
+ int ret;
+
+ ret = __intel_wait_for_register(&i915->uncore,
+ HUC_STATUS2,
+ HUC_FW_VERIFIED,
+ HUC_FW_VERIFIED,
+ 2, 50, &status);
+ if (ret)
+ DRM_ERROR("HuC: status %#x\n", status);
+ return ret;
+}
+
+static int gen11_huc_wait_verified(struct intel_huc *huc)
+{
+ struct drm_i915_private *i915 = huc_to_i915(huc);
+ int ret;
+
+ ret = __intel_wait_for_register(&i915->uncore,
+ GEN11_HUC_KERNEL_LOAD_INFO,
+ HUC_LOAD_SUCCESSFUL,
+ HUC_LOAD_SUCCESSFUL,
+ 2, 50, NULL);
+ return ret;
+}
+
+static int huc_wait_verified(struct intel_huc *huc)
We do call this only once, so maybe we can just avoid having a
separate function and just have it directly in intel_huc_auth? the
code is simple enough. Otherwise, to avoid 2 identical functions
which diff only in the register details,
There was one small diff: in case of timeout, pre-gen11 variant was
printing
whole HuC status value. But maybe we don't care any more...
AFAICS the other bits in the pre-gen11 register are unrelated to
authentication, so there isn't really any value in printing that on an
auth fail. Some of the bits are loading failure related, so we could
think about printing the register if the dma fails.
we could save the register and the expected value in the huc struct
during init_early and just wait on (huc->auth.reg & huc->auth.mask),
which we could also use in intel_huc_check_status().
To be more future ready, we should store reg/mask/value tuple.
Btw, is it ok that intel_huc_check_status() will now return different
values depending on gen (was 1<<7, now 1<<0) for status ?
Note that intel_huc_check_status() is used directly in
I915_PARAM_HUC_STATUS.
Maybe we should try to unify these and always return just 0 and fixed 1 ?
Does it count as uABI change ?
It is in theory an ABI change, but the documentation above
intel_huc_check_status says:
* 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.
So I'm guessing there is already a disconnect between expectation and
actual returned value. I doubt anyone is using the parameter as
something different than a bool so we should be able to get away with
"fixing" the ABI like we did with other calls in the past, but we should
double-check with the user the call was added for.
Scratch this, the status variable that we return is a bool, so the
result should already be automatically casted to the appropriate value
(0 or 1).
Daniele
Daniele
Apart from this, register values do match the FW and the specs.
Daniele
+{
+ struct drm_i915_private *i915 = huc_to_i915(huc);
+ int ret;
+
+ if (INTEL_GEN(i915) >= 11)
+ ret = gen11_huc_wait_verified(huc);
+ else
+ ret = gen8_huc_wait_verified(huc);
+ return ret;
+}
+
/**
* intel_huc_auth() - Authenticate HuC uCode
* @huc: intel_huc structure
@@ -56,7 +97,6 @@ int intel_huc_auth(struct intel_huc *huc)
struct drm_i915_private *i915 = huc_to_i915(huc);
struct intel_guc *guc = &i915->guc;
struct i915_vma *vma;
- u32 status;
int ret;
if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
@@ -79,13 +119,9 @@ int intel_huc_auth(struct intel_huc *huc)
}
/* Check authentication status, it should be done by now */
- ret = __intel_wait_for_register(&i915->uncore,
- HUC_STATUS2,
- HUC_FW_VERIFIED,
- HUC_FW_VERIFIED,
- 2, 50, &status);
+ ret = huc_wait_verified(huc);
if (ret) {
- DRM_ERROR("HuC: Firmware not verified %#x\n", status);
+ DRM_ERROR("HuC: Firmware not verified %d\n", ret);
goto fail_unpin;
}
@@ -122,7 +158,11 @@ int intel_huc_check_status(struct intel_huc
*huc)
return -ENODEV;
with_intel_runtime_pm(dev_priv, wakeref)
- status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
+ if (INTEL_GEN(dev_priv) >= 11)
+ status = I915_READ(GEN11_HUC_KERNEL_LOAD_INFO) &
+ HUC_LOAD_SUCCESSFUL;
+ else
+ status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
return status;
}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
