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

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com>]

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

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@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