Re: [RFC] drm/i915/huc: better define HuC status getparam possible return values.

"Ceraolo Spurio, Daniele" <daniele.ceraolospurio@xxxxxxxxx> · Mon, 18 Jul 2022 09:48:39 -0700

On 7/18/2022 4:26 AM, Tvrtko Ursulin wrote:

On 09/07/2022 00:48, Daniele Ceraolo Spurio wrote:
The current HuC status getparam return values are a bit confusing in
regards to what happens in some scenarios. In particular, most of the
error cases cause the ioctl to return an error, but a couple of them,
INIT_FAIL and LOAD_FAIL, are not explicitly handled and neither is
their expected return value documented; these 2 error cases therefore
end up into the catch-all umbrella of the "HuC not loaded" case, with
this case therefore including both some error scenarios and the load
in progress one.

The updates included in this patch change the handling so that all
error cases behave the same way, i.e. return an errno code, and so
that the HuC load in progress case is unambiguous.

The patch also includes a small change to the FW init path to make sure
we always transition to an error state if something goes wrong.

This is an RFC because this is a minor change in behavior for an ioctl.
I'm arguing that this is not an API breakage because the expected return
for the cases I've changed was not well defined, but I want to make sure
no one is in opposition to this. From comments from media driver devs
on a different patch [1], it sounds like the media driver already
expected an errno value for all errors cases and is therefore already
compatible with the proposed changes.

I also think this is fine - just more error cases. And I don't see 
that it could break something. So from me:

Acked-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

And most importantly, with this change are you able to omit the patch 
I did not like, the one which was returning a fake status while the 
load was in progress? I can't remember if it was faking running while 
loading or what exactly.

Yes, I'll replace the one you didn't like (returning 1 when load is in 
progress) with this one.

Daniele


Regards,

Tvrtko


[1] 
https://lists.freedesktop.org/archives/intel-gfx/2022-July/300990.html

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
Cc: Tony Ye <tony.ye@xxxxxxxxx>
---
  drivers/gpu/drm/i915/gt/uc/intel_guc.c   |  1 +
  drivers/gpu/drm/i915/gt/uc/intel_huc.c   | 14 +++++++-------
  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c |  1 -
  include/uapi/drm/i915_drm.h              | 16 ++++++++++++++++
  4 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
index 2706a8c65090..42cb244587f1 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
@@ -455,6 +455,7 @@ int intel_guc_init(struct intel_guc *guc)
  err_fw:
      intel_uc_fw_fini(&guc->fw);
  out:
+    intel_uc_fw_change_status(&guc->fw, INTEL_UC_FIRMWARE_INIT_FAIL);
      i915_probe_error(gt->i915, "failed with %d\n", ret);
      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 3bb8838e325a..bddcd3242ad0 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
@@ -113,6 +113,7 @@ int intel_huc_init(struct intel_huc *huc)
      return 0;
    out:
+    intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_INIT_FAIL);
      drm_info(&i915->drm, "HuC init failed with %d\n", err);
      return err;
  }
@@ -200,13 +201,8 @@ static bool huc_is_authenticated(struct 
intel_huc *huc)
   * This function reads status register to verify if HuC
   * firmware was successfully loaded.
   *
- * Returns:
- *  * -ENODEV if HuC is not present on this platform,
- *  * -EOPNOTSUPP if HuC firmware is disabled,
- *  * -ENOPKG if HuC firmware was not installed,
- *  * -ENOEXEC if HuC firmware is invalid or mismatched,
- *  * 0 if HuC firmware is not running,
- *  * 1 if HuC firmware is authenticated and running.
+ * The return values match what is expected for the 
I915_PARAM_HUC_STATUS
+ * getparam.
   */
  int intel_huc_check_status(struct intel_huc *huc)
  {
@@ -219,6 +215,10 @@ int intel_huc_check_status(struct intel_huc *huc)
          return -ENOPKG;
      case INTEL_UC_FIRMWARE_ERROR:
          return -ENOEXEC;
+    case INTEL_UC_FIRMWARE_INIT_FAIL:
+        return -ENOMEM;
+    case INTEL_UC_FIRMWARE_LOAD_FAIL:
+        return -EIO;
      default:
          break;
      }
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 27363091e1af..007401397935 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -707,7 +707,6 @@ int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
  out_unpin:
      i915_gem_object_unpin_pages(uc_fw->obj);
  out:
-    intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_INIT_FAIL);
      return err;
  }
  diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 094f6e377793..0950ef0d598c 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -645,6 +645,22 @@ typedef struct drm_i915_irq_wait {
   */
  #define   I915_SCHEDULER_CAP_STATIC_PRIORITY_MAP    (1ul << 5)
  +/*
+ * Query the status of HuC load.
+ *
+ * The query can fail in the following scenarios with the listed 
error codes:
+ *  -ENODEV if HuC is not present on this platform,
+ *  -EOPNOTSUPP if HuC firmware usage is disabled,
+ *  -ENOPKG if HuC firmware fetch failed,
+ *  -ENOEXEC if HuC firmware is invalid or mismatched,
+ *  -ENOMEM if i915 failed to prepare the FW objects for transfer to 
the uC,
+ *  -EIO if the FW transfer or the FW authentication failed.
+ *
+ * If the IOCTL is successful, the returned parameter will be set to 
one of the
+ * following values:
+ *  * 0 if HuC firmware load is not complete,
+ *  * 1 if HuC firmware is authenticated and running.
+ */
  #define I915_PARAM_HUC_STATUS         42
    /* Query whether DRM_I915_GEM_EXECBUFFER2 supports the ability to 
opt-out of







