Re: [PATCH v2 15/22] drm/i915/huc: New HuC status register for Gen11

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

On 4/15/19 2:44 PM, Michal Wajdeczko wrote:
> On Mon, 15 Apr 2019 23:19:40 +0200, Daniele Ceraolo Spurio 
> <daniele.ceraolospurio@intel.com> wrote:
> 
>>
>>
>> On 4/11/19 1:44 AM, Michal Wajdeczko wrote:
>>> Gen11 defines new register for checking HuC authentication status.
>>> Look into the right register and bit.
>>>  BSpec: 19686
>>>  Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Cc: Tony Ye <tony.ye@intel.com>
>>> Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>>> Cc: John Spotswood <john.a.spotswood@intel.com>
>>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_guc_reg.h |  3 ++
>>>   drivers/gpu/drm/i915/intel_huc.c     | 56 ++++++++++++++++++++++++----
>>>   2 files changed, 51 insertions(+), 8 deletions(-)
>>>  diff --git a/drivers/gpu/drm/i915/intel_guc_reg.h 
>>> b/drivers/gpu/drm/i915/intel_guc_reg.h
>>> index d26de5193568..7eba65795b58 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc_reg.h
>>> +++ b/drivers/gpu/drm/i915/intel_guc_reg.h
>>> @@ -79,6 +79,9 @@
>>>   #define HUC_STATUS2             _MMIO(0xD3B0)
>>>   #define   HUC_FW_VERIFIED       (1<<7)
>>>   +#define GEN11_HUC_KERNEL_LOAD_INFO    _MMIO(0xC1DC)
>>> +#define   HUC_LOAD_SUCCESSFUL          (1 << 0)
>>> +
>>>   #define GUC_WOPCM_SIZE            _MMIO(0xc050)
>>>   #define   GUC_WOPCM_SIZE_LOCKED          (1<<0)
>>>   #define   GUC_WOPCM_SIZE_SHIFT        12
>>> diff --git a/drivers/gpu/drm/i915/intel_huc.c 
>>> b/drivers/gpu/drm/i915/intel_huc.c
>>> index 94c04f16a2ad..708a4b387259 100644
>>> --- a/drivers/gpu/drm/i915/intel_huc.c
>>> +++ b/drivers/gpu/drm/i915/intel_huc.c
>>> @@ -40,6 +40,47 @@ int intel_huc_init_misc(struct intel_huc *huc)
>>>       return 0;
>>>   }
>>>   +static int gen8_huc_wait_verified(struct intel_huc *huc)
>>
>> why gen8?
>>
>>> +{
>>> +    struct drm_i915_private *i915 = huc_to_i915(huc);
>>> +    u32 status;
>>> +    int ret;
>>> +
>>> +    ret = __intel_wait_for_register(&i915->uncore,
>>> +                    HUC_STATUS2,
>>> +                    HUC_FW_VERIFIED,
>>> +                    HUC_FW_VERIFIED,
>>> +                    2, 50, &status);
>>> +    if (ret)
>>> +        DRM_ERROR("HuC: status %#x\n", status);
>>> +    return ret;
>>> +}
>>> +
>>> +static int gen11_huc_wait_verified(struct intel_huc *huc)
>>> +{
>>> +    struct drm_i915_private *i915 = huc_to_i915(huc);
>>> +    int ret;
>>> +
>>> +    ret = __intel_wait_for_register(&i915->uncore,
>>> +                    GEN11_HUC_KERNEL_LOAD_INFO,
>>> +                    HUC_LOAD_SUCCESSFUL,
>>> +                    HUC_LOAD_SUCCESSFUL,
>>> +                    2, 50, NULL);
>>> +    return ret;
>>> +}
>>> +
>>> +static int huc_wait_verified(struct intel_huc *huc)
>>
>> We do call this only once, so maybe we can just avoid having a 
>> separate function and just have it directly in intel_huc_auth? the 
>> code is simple enough. Otherwise, to avoid 2 identical functions which 
>> diff only in the register details,
> 
> There was one small diff: in case of timeout, pre-gen11 variant was 
> printing
> whole HuC status value. But maybe we don't care any more...

AFAICS the other bits in the pre-gen11 register are unrelated to 
authentication, so there isn't really any value in printing that on an 
auth fail. Some of the bits are loading failure related, so we could 
think about printing the register if the dma fails.

> 
>> we could save the register and the expected value in the huc struct 
>> during init_early and just wait on (huc->auth.reg & huc->auth.mask), 
>> which we could also use in intel_huc_check_status().
> 
> To be more future ready, we should store reg/mask/value tuple.
> 
> Btw, is it ok that intel_huc_check_status() will now return different
> values depending on gen (was 1<<7, now 1<<0) for status ?
> 
> Note that intel_huc_check_status() is used directly in 
> I915_PARAM_HUC_STATUS.
> Maybe we should try to unify these and always return just 0 and fixed 1 ?
> Does it count as uABI change ?
> 

It is in theory an ABI change, but the documentation above 
intel_huc_check_status says:

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

So I'm guessing there is already a disconnect between expectation and 
actual returned value. I doubt anyone is using the parameter as 
something different than a bool so we should be able to get away with 
"fixing" the ABI like we did with other calls in the past, but we should 
double-check with the user the call was added for.

Daniele

>>
>> Apart from this, register values do match the FW and the specs.
>>
>> Daniele
>>
>>> +{
>>> +    struct drm_i915_private *i915 = huc_to_i915(huc);
>>> +    int ret;
>>> +
>>> +    if (INTEL_GEN(i915) >= 11)
>>> +        ret = gen11_huc_wait_verified(huc);
>>> +    else
>>> +        ret = gen8_huc_wait_verified(huc);
>>> +    return ret;
>>> +}
>>> +
>>>   /**
>>>    * intel_huc_auth() - Authenticate HuC uCode
>>>    * @huc: intel_huc structure
>>> @@ -56,7 +97,6 @@ int intel_huc_auth(struct intel_huc *huc)
>>>       struct drm_i915_private *i915 = huc_to_i915(huc);
>>>       struct intel_guc *guc = &i915->guc;
>>>       struct i915_vma *vma;
>>> -    u32 status;
>>>       int ret;
>>>         if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>>> @@ -79,13 +119,9 @@ int intel_huc_auth(struct intel_huc *huc)
>>>       }
>>>         /* Check authentication status, it should be done by now */
>>> -    ret = __intel_wait_for_register(&i915->uncore,
>>> -                    HUC_STATUS2,
>>> -                    HUC_FW_VERIFIED,
>>> -                    HUC_FW_VERIFIED,
>>> -                    2, 50, &status);
>>> +    ret = huc_wait_verified(huc);
>>>       if (ret) {
>>> -        DRM_ERROR("HuC: Firmware not verified %#x\n", status);
>>> +        DRM_ERROR("HuC: Firmware not verified %d\n", ret);
>>>           goto fail_unpin;
>>>       }
>>>   @@ -122,7 +158,11 @@ int intel_huc_check_status(struct intel_huc *huc)
>>>           return -ENODEV;
>>>         with_intel_runtime_pm(dev_priv, wakeref)
>>> -        status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
>>> +        if (INTEL_GEN(dev_priv) >= 11)
>>> +            status = I915_READ(GEN11_HUC_KERNEL_LOAD_INFO) &
>>> +                HUC_LOAD_SUCCESSFUL;
>>> +        else
>>> +            status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
>>>         return status;
>>>   }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx