[Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS

[Michal Wajdeczko]

On Tue, 11 Feb 2020 18:53:05 +0100, Fosha, Robert M  
<robert.m.fosha at intel.com> wrote:

>
>
> On 2/4/20 4:43 PM, Ye, Tony wrote:
>>
>>
>> On 1/27/2020 1:41 AM, Michal Wajdeczko wrote:
>>> On Thu, 23 Jan 2020 16:51:58 +0100, Chris Wilson  
>>> <chris at chris-wilson.co.uk> wrote:
>>>
>>>> Quoting Michal Wajdeczko (2020-01-23 15:38:52)
>>>>> On Thu, 23 Jan 2020 16:02:17 +0100, Chris Wilson
>>>>> <chris at chris-wilson.co.uk> wrote:
>>>>>
>>>>> > Quoting Daniele Ceraolo Spurio (2020-01-22 23:52:33)
>>>>> >>
>>>>> >>
>>>>> >> On 1/22/20 11:48 AM, Michal Wajdeczko wrote:
>>>>> >> >  From commit 84b1ca2f0e68 ("drm/i915/uc: prefer intel_gt over  
>>>>> i915
>>>>> >> > in GuC/HuC paths") we stopped using HUC_STATUS error -ENODEV  
>>>>> only
>>>>> >> > to indicate lack of HuC hardware and we started to use this  
>>>>> error
>>>>> >> > also for all other cases when HuC was not in use or supported.
>>>>> >> >
>>>>> >> > Fix that by relying again on HAS_GT_UC macro, since currently
>>>>> >> > used function intel_huc_is_supported() is based on HuC firmware
>>>>> >> > support which could be unsupported also due to force disabled
>>>>> >> > GuC firmware.
>>>>> >> >
>>>>> >> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>>>> >> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>>>> >> > Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>>>> >> > Cc: Tony Ye <tony.ye at intel.com>
>>>>> >>
>>>>> >> Reviewed-by: Daniele Ceraolo Spurio  
>>>>> <daniele.ceraolospurio at intel.com>
>>>>> >
>>>>> > Once upon a time did you (Michal) not argue we should indicate the  
>>>>> lack
>>>>> > of firmware in the error code? Something like
>>>>> >
>>>>> > if (!HAS_GT_UC(gt->i915))
>>>>> >       return -ENODEV;
>>>>> >
>>>>> > if (!intel_huc_is_supported(huc))
>>>>> >       return -ENOEXEC;
>>>>>
>>>>> Yes, we discussed this here [1] together with [2] but we didn't
>>>>> conclude our discussion due to different opinions on how represent
>>>>> some states, in particular "manually disabled" state.
>>>>>
>>>>> In this patch I just wanted to restore old notation.
>>>>>
>>>>> But we can start new discussion, here is summary:
>>>>>
>>>>> ------------------+----------+----------+----------
>>>>>   HuC state        | today*   | option A | option B
>>>>> ------------------+----------+----------+----------
>>>>> no HuC hardware   | -ENODEV  | -ENODEV  | -ENODEV
>>>>> GuC fw disabled   |   0      |     0    | -EOPNOTSUPP
>>>>> HuC fw disabled   |   0      |     0    | -EOPNOTSUPP
>>>>> HuC fw missing    |   0      | -ENOPKG  | -ENOEXEC
>>>>> HuC fw error      |   0      | -ENOEXEC | -ENOEXEC
>>>>> HuC fw fail       |   0      | -EACCES  |    0
>>>>> HuC authenticated |   1      |     1    |    1
>>>>> ------------------+----------+----------+----------
>>>>
>>>> By fw fail, you mean we loaded the firmware (to our knowledge)
>>>> correctly, but HUC_STATUS is not reported as valid?
>>>>
>>>> If so, I support option B. I like the idea of saying
>>>> "no HuC" (machine too old)
>>>> "no firmware" (user action, or lack thereof)
>>>> 0 (fw unhappy)
>>>> 1 (fw reports success)
>>>>
>>>> In between states for failures in fw loading? Not so sure. But I can  
>>>> see
>>>> the nicety in distinguishing between lack of firmware and some random
>>>> failure in loading the firmware (the former being user action required
>>>> to rectify, command line parameter whatever and the latter being the
>>>> firmware file is either invalid or a stray neutrino prevented  
>>>> loading).
>>>>
>>>> Imo the error messages should be about why we cannot probe/trust the
>>>> HUC_STATUS register. If everything is setup correctly then the  
>>>> returned
>>>> value should be from reading the register. I dislike only returning 1  
>>>> if
>>>> supported, and converting a valid read of 0 into another error.
>>>>
>>>> So Option B :)
>>>
>>> But I'm not sure that option B is consistent in error reporting, as
>>> "fw unhappy" is definitely an serious error but is represented as plain
>>> non-error "0" status, while "fw disabled" (user action) is treated as  
>>> error
>>>
>>> ------------------+----------
>>>    HuC state       | option B
>>> ------------------+----------
>>> no HuC hardware   | -ENODEV
>>> GuC fw disabled   | -EOPNOTSUPP -> user decision, why error?
>>> HuC fw disabled   | -EOPNOTSUPP -> user decision, why error?
>>> HuC fw missing    | -ENOEXEC
>>> HuC fw error      | -ENOEXEC
>>> HuC fw fail       |    0        -> unlikely, but still fw/hw error
>>> HuC authenticated |    1
>>> ------------------+----------
>>>
>>> On other hand, option A treats all error conditions as errors, leaving
>>> status codes only for normal operations: disabled(0)/authenticated(1):
>>>
>>> ------------------+----------
>>>    HuC state       | option A
>>> ------------------+----------
>>> no HuC hardware   | -ENODEV  -> you shouldn't ask
>>> GuC fw disabled   |     0    -> user decision, not an error
>>> HuC fw disabled   |     0    -> user decision, not an error
>>> HuC fw missing    | -ENOPKG  -> fw not installed correctly
>>> HuC fw error      | -ENOEXEC -> bad/wrong fw
>>> HuC fw fail       | -EACCES  -> fw/hw error
>>> HuC authenticated |     1
>>> ------------------+----------
>>
>> Vote for Option A.
>>
>> Regards,
>> Tony
>>
>
> Are we ok to move forward on this? Michal, are you working on updating  
> the patch?

I can update the patch, but I don't know which option to implement:
Tony votes for Option A, Chris for Option B, what's your choice?

Michal

>
> -Rob
>
>>>
>>> But since I'm not an active HuC user, will leave final decision to  
>>> others.
>>>
>>> /Michal
>>>
>>>
>>>>
>>>>> Note that all above should be compatible with media driver,
>>>>> which explicitly looks for no error and value 1
>>>>
>>>> Cool.
>>>> -Chris
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx