[Intel-gfx] [PATCH v2 15/22] drm/i915/huc: New HuC status register for Gen11
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Mon Apr 15 22:10:48 UTC 2019
On 4/15/19 2:44 PM, Michal Wajdeczko wrote:
> On Mon, 15 Apr 2019 23:19:40 +0200, Daniele Ceraolo Spurio
> <daniele.ceraolospurio at 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 at intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>> Cc: Tony Ye <tony.ye at intel.com>
>>> Cc: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
>>> Cc: John Spotswood <john.a.spotswood at intel.com>
>>> Cc: Anusha Srivatsa <anusha.srivatsa at 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;
>>> }
More information about the Intel-gfx
mailing list