Re: [PATCH 3/3] drm/i915/huc: Update HuC status codes

["Michal Wajdeczko" <michal.wajdeczko@intel.com>]

On Wed, 22 May 2019 22:19:47 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2019-05-22 20:00:57)
>> Without breaking existing usage, slightly update HuC status codes
>> to provide more info to the clients:
>>  1 if HuC firmware is loaded and verified,
>>  0 if HuC firmware is not enabled,
>>  -ENOPKG if HuC firmware is not loaded,
>>  -ENODEV if HuC is not present on this platform.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tony Ye <tony.ye@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_huc.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_huc.c  
>> b/drivers/gpu/drm/i915/intel_huc.c
>> index aac17916e130..98deb4ee60a7 100644
>> --- a/drivers/gpu/drm/i915/intel_huc.c
>> +++ b/drivers/gpu/drm/i915/intel_huc.c
>> @@ -150,9 +150,11 @@ int intel_huc_auth(struct intel_huc *huc)
>>   * intel_huc_check_status() - check HuC status
>>   * @huc: intel_huc structure
>>   *
>> - * 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.
>> + * Return:
>> + * * 1 if HuC firmware is loaded and verified,
>> + * * 0 if HuC firmware is not enabled,
>> + * * -ENOPKG if HuC firmware is not loaded,
>> + * * -ENODEV if HuC is not present on this platform.
>>   */
>>  int intel_huc_check_status(struct intel_huc *huc)
>>  {
>> @@ -161,5 +163,8 @@ int intel_huc_check_status(struct intel_huc *huc)
>>         if (!HAS_HUC(i915))
>>                 return -ENODEV;
>>
>> -       return huc->verified;
>> +       if (!USES_HUC(i915))
>> +               return 0;
>> +
>> +       return huc->verified ? 1 : -ENOPKG;
>
> I still think EOPNOTSUPP is a better error though for the user
> preventing the huc being loaded -- as opposed to the result of
> verification being the non-error value.
>
> error == unable to setup huc
> 0/1 == result from talking to huc

but your 0 here overlaps with unable to setup huc error,
so from the ABI perspective, imho, is bad.

also, from media team pov, as they want to have HuC always on,
the only non-error case is when user explicitly decided otherwise.

>
> Better ask someone else for a third opinion.

Check end of the message [1] but I'm fine waiting for more opinions.

[1] https://lists.freedesktop.org/archives/intel-gfx/2019-May/199066.html

Michal
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx