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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@intel.com>

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.

Regards,

Tvrtko

> 
> [1] https://lists.freedesktop.org/archives/intel-gfx/2022-July/300990.html
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Tony Ye <tony.ye@intel.com>
> ---
>   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