[Intel-gfx] [PATCH v2] drm/i915/huc: Don't try to check HuC status if it's not loaded

[Michal Wajdeczko]

On Mon, 20 May 2019 11:35:26 +0200, Chris Wilson  
<chris at chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2019-05-19 22:50:43)
>> If we never attempted to load HuC firmware, or we already wedged
>> or reset GuC/HuC, then there is no reason to wake up the device
>> to check one bit in the register that will be for sure cleared.
>>
>> v2: check if HuC was enabled; subtle change in ABI
>>     reuse hus_is_load helper
>>
>> Suggested-by: Chris Wilson <chris at chris-wilson.co.uk>
>> 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 | 20 ++++++++++++--------
>>  drivers/gpu/drm/i915/intel_huc.h |  5 +++++
>>  2 files changed, 17 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_huc.c  
>> b/drivers/gpu/drm/i915/intel_huc.c
>> index 1ff1fb015e58..bfdebec1cfc8 100644
>> --- a/drivers/gpu/drm/i915/intel_huc.c
>> +++ b/drivers/gpu/drm/i915/intel_huc.c
>> @@ -113,7 +113,7 @@ int intel_huc_auth(struct intel_huc *huc)
>>         u32 status;
>>         int ret;
>>
>> -       if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>> +       if (!intel_huc_is_loaded(huc))
>>                 return -ENOEXEC;
>>
>>         ret = intel_guc_auth_huc(guc,
>> @@ -150,21 +150,25 @@ int intel_huc_auth(struct intel_huc *huc)
>>   * This function reads status register to verify if HuC
>>   * firmware was successfully loaded.
>>   *
>> - * 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.
>> + * Returns: 1 if HuC firmware is loaded and verified, 0 if HuC  
>> firmware is not
>> + * enabled, -ENOPKG if HuC firmware is not loaded and -ENODEV if HuC  
>> is not
>> + * present on this platform.
>>   */
>>  int intel_huc_check_status(struct intel_huc *huc)
>>  {
>>         struct drm_i915_private *dev_priv = huc_to_i915(huc);
>>         intel_wakeref_t wakeref;
>> -       bool status = false;
>> +       bool verified = false;
>>
>>         if (!HAS_HUC(dev_priv))
>>                 return -ENODEV;
>>
>> -       with_intel_runtime_pm(dev_priv, wakeref)
>> -               status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
>> +       if (!USES_HUC(dev_priv))
>> +               return 0;
>
> Hmm, EOPNOTSUPP here for the user disabled case?

I'm not sure that user disabled case shall be reported as error,
since it perfectly fits into legacy ABI 0="not loaded".
The only small change is that now 0="not loaded (not enabled by user)"

>
> Then
>
> if (!intel_huc_is_loaded(huc))
> 	return -ENOPKG;
>
> bool verified = false;
> with_intel_runtime_pm(dev_priv, wakeref)
> 	verified = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
> return verified.
>
> That keeps it a bit flatter with the special casing separate and the 0/1
> result as given by the register status. Then if negative, we know one of
> the preconditions for using HuC is not available, and if 0 something went
> wrong in mmio setup.
>
> Does that make sense?

IMHO, if something went wrong it is better to report it as error rather
then weak 0 status. Compare:

+---------------+-------+-------+-------+
  no hardware     -ENODEV -ENODEV -ENODEV
  fw disabled        0       0    -ENOTSUP
  fw not loaded      0    -ENOPKG -ENOPKG
  fw not verified*   0    -ENOPKG    0
  fw verified*       1       1       1
+---------------+-------+-------+-------+
*) registry access

Note that in your case, fw verification problem can be reported both
as -ENOPKG or as 0 (former when verification fails at load time, latter
as result of runtime reg read). Very likely we will never see 0 at all,
since probably that can't change once fw was verified at load time.
That might be viewed as undesired ABI change.

>
>> -       return status;
>> +       if (intel_huc_is_loaded(huc))
>> +               with_intel_runtime_pm(dev_priv, wakeref)
>> +                       verified = I915_READ(HUC_STATUS2) &  
>> HUC_FW_VERIFIED;
>> +
>> +       return verified ? 1 : -ENOPKG;
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_huc.h  
>> b/drivers/gpu/drm/i915/intel_huc.h
>> index a0c21ae02a99..cde3d303718d 100644
>> --- a/drivers/gpu/drm/i915/intel_huc.h
>> +++ b/drivers/gpu/drm/i915/intel_huc.h
>> @@ -44,6 +44,11 @@ void intel_huc_fini(struct intel_huc *huc);
>>  int intel_huc_auth(struct intel_huc *huc);
>>  int intel_huc_check_status(struct intel_huc *huc);
>>
>> +static inline bool intel_huc_is_loaded(struct intel_huc *huc)
>> +{
>> +       return intel_uc_fw_is_loaded(&huc->fw);
>> +}
>> +
>>  static inline void intel_huc_fini_misc(struct intel_huc *huc)
>>  {
>>         intel_uc_fw_cleanup_fetch(&huc->fw);
>> --
>> 2.19.2