[Intel-gfx] [PATCH 3/3] drm/i915/huc: Update HuC status codes

[Michal Wajdeczko]

On Fri, 31 May 2019 12:13:20 +0200, Joonas Lahtinen  
<joonas.lahtinen at linux.intel.com> wrote:

> Quoting Michal Wajdeczko (2019-05-23 01:25:00)
>> On Wed, 22 May 2019 22:19:47 +0200, Chris Wilson
>> <chris at 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 at intel.com>
>> >> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> >> Cc: Tony Ye <tony.ye at 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.
>
> Trying to look things from external perspective, if  
> HUC_(AUTHENTICATION_)STATUS
> is queried 1 => "authenticated", 0 => "not authenticated" makes most  
> sense.

If you want to treat that as "authenticated" status then I fully agree.

But authentication status is just a one technical detail how we load
the firmware on hardware. Users mainly want to check if HuC was correctly
enabled on their system, so this is more like HUC_(ENABLED_)STATUS.

Then we have 1 => "enabled/active" and 0 => "disabled"

>
> Both media drivers also first check for errors, then convert the return
> value to boolean, so it would be compatible with that.

Both solutions are compatible with current media drivers implementations,
we don't want to break ABI, just to make it more useful.

Now these drivers just checks for success of the ioctl() call alone and
ignore any reported error since we don't provide any meaningful codes
why HuC is not working. We forced them to check dmesg for any hint.

Providing clear notation of 1(active) 0(disabled) and set of possible
errno codes why not explicitly disabled HuC is not working well, will,
imho, be much more useful than 1(authenticated) and 0(not authenticated,
dunno why)

Again, proposed here approach is aligned with Tony's opinion.

Regards,
Michal

>
> Regards, Joonas