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

[Michal Wajdeczko]

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

> 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 ?

>
> 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;
>>   }