Yup - simple stuff - LGTM:
Reviewed-by: Alan Previn <alan.previn.teres.alexis@xxxxxxxxx>
On Fri, 2022-08-19 at 15:53 -0700, 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.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> Cc: Tony Ye <tony.ye@xxxxxxxxx>
> Acked-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Acked-by: 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 01f2705cb94a..10b2da810a8f 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> @@ -443,6 +443,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 9a97b8cc90c7..1a34c902d081 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> @@ -284,6 +284,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;
> }
> @@ -403,13 +404,8 @@ bool intel_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)
> {
> @@ -422,6 +418,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 58547292efa0..cec6bf6bad3f 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -749,7 +749,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 520ad2691a99..629198f1d8d8 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
> --
> 2.37.2
>
