[Intel-gfx] [PATCH] drm/i915/uc: Don't fail on HuC firmware failure

[Michal Wajdeczko]

On Fri, 26 Jul 2019 20:57:12 +0200, Chris Wilson  
<chris at chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2019-07-26 18:12:40)
>> HuC is usually not a critical component, so we can safely ignore
>> firmware load or authentication failures unless HuC was explicitly
>> requested by the user.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/gt/uc/intel_uc.c    | 8 ++++----
>>  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 3 ++-
>>  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 6 ++++++
>>  3 files changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c  
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> index 5b9b20d1cb6d..99419c5c0ad3 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> @@ -472,7 +472,7 @@ int intel_uc_init_hw(struct intel_uc *uc)
>>
>>                 if (intel_uc_is_using_huc(uc)) {
>>                         ret = intel_huc_fw_upload(huc);
>> -                       if (ret)
>> +                       if (ret && intel_uc_fw_is_overridden(&huc->fw))
>>                                 goto err_out;
>>                 }
>>
>> @@ -494,9 +494,9 @@ int intel_uc_init_hw(struct intel_uc *uc)
>>         if (ret)
>>                 goto err_log_capture;
>>
>> -       if (intel_uc_is_using_huc(uc)) {
>> +       if (intel_uc_is_using_huc(uc) &&  
>> intel_uc_fw_is_loaded(&huc->fw)) {
>
> Can we even load the huc fw is !using_huc()?

as of today, meaning of "intel_uc_is_using_huc" is that HuC was
requested to run (ie. enabled via modparam) and not that is now
being used.

>
>>                 ret = intel_huc_auth(huc);
>> -               if (ret)
>> +               if (ret && intel_uc_fw_is_overridden(&huc->fw))
>>                         goto err_communication;
>>         }
>>
>> @@ -515,7 +515,7 @@ int intel_uc_init_hw(struct intel_uc *uc)
>>         dev_info(i915->drm.dev, "GuC submission %s\n",
>>                  enableddisabled(intel_uc_is_using_guc_submission(uc)));
>>         dev_info(i915->drm.dev, "HuC %s\n",
>> -                enableddisabled(intel_uc_is_using_huc(uc)));
>> +                enableddisabled(intel_huc_is_authenticated(huc)));
>
> Seems reasonable by itself.
>
>>
>>         return 0;
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c  
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> index 5fbdd17a864b..1e9ae38e5b10 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> @@ -146,7 +146,8 @@ __uc_fw_override(struct intel_uc_fw *uc_fw)
>>                 break;
>>         }
>>
>> -       return uc_fw->path;
>> +       uc_fw->user_overridden = uc_fw->path;
>
> uc_fw->user_overridden = uc_fw->path && *uc_fw->path;
>
> That is i915.huc_firmware="" should be a convenient way to disable
> loading.

hmm, to be working as expected this should done init_early as:

	if (uc_fw->path && *uc_fw->path)
		uc_fw->status = INTEL_UC_FIRMWARE_SELECTED;
	else
		uc_fw->status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;

>
> If we agree on that "creative misuse" of the modparam, or if you can
> reassure me that it works correctly anyway,
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> -Chris