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 3:10 PM, Daniele Ceraolo Spurio wrote:
> 
> 
> 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.
> 

Scratch this, the status variable that we return is a bool, so the 
result should already be  automatically casted to the appropriate value 
(0 or 1).

Daniele

> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx