[Intel-gfx] [PATCH 1/1] drm/i915/uc: Turn on GuC/HuC auto mode again

[Michal Wajdeczko]

On Mon, 19 Aug 2019 10:09:15 +0200, Martin Peres  
<martin.peres at linux.intel.com> wrote:

> On 18/08/2019 18:51, Michal Wajdeczko wrote:
>> We hope that now all issues related to missing uC firmwares
>> are fixed and that driver can continue to load without GuC
>> or HuC firmware available and running:
>>
>>  - missing or corrupted HuC firmware has no impact to driver
>>    load (clients should continue to use HUC_STATUS to check
>>    if HuC firmware was loaded and authenticated)
>>
>>  - missing or corrupted GuC firmware has no impact to driver
>>    load (unless GuC firmware blob was overridden by the user
>>    or GuC submission was requested or GuC was previously
>>    enabled on this system without reboot - then we will mark
>>    GPU as wedged and continue with KMS only)
>
> Please hold merging this patch, as many more items need to be crossed
> off before such a patch can land.
>
> Such items include:
>
>  - Assess all the existing GUC-related bugs, and prove they won't
> suddenly get seen by more users

Yep, this is right time to double check that.

>  - add fault injection to the FW loading path

About 40 new failure points were added to firmware loading path by
recent commits like [1] ("drm/i915/uc: Hardening firmware fetch")
and [2] ("drm/i915/uc: Inject probe errors into intel_uc_init_hw")
so I assume we could mark this item as done.

>  - add IGT tests to make sure driver behaves well on different FW
> loading errors

Not sure which if we need another set of tests beyond existing and
already used igt at i915_module_load@reload-with-fault-injection.

Maybe all we need is to add dedicated CI machines that will try to
use GuC but without valid firmware on the system? Let CI team speak
here. Note I was already trying to mimic that scenario with [3] and
[4] and they we all good.

Thanks,
Michal

[1]  
https://cgit.freedesktop.org/drm/drm-tip/commit/?id=5e0a809af2a2b577934ec54c0e50897b806f0732
[2]  
https://cgit.freedesktop.org/drm/drm-tip/commit/?id=5d1ef2b4270de45e1b1b40a00838e3b6196eefd7
[3] https://patchwork.freedesktop.org/series/65378/#rev1
[4] https://patchwork.freedesktop.org/series/65379/#rev1

>
> Martin
>
>>
>> References: commit f774f0964919 ("drm/i915/guc: Turn on GuC/HuC auto  
>> mode")
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Cc: Jani Nikula <jani.nikula at intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_params.c | 2 +-
>>  drivers/gpu/drm/i915/i915_params.h | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_params.c  
>> b/drivers/gpu/drm/i915/i915_params.c
>> index 296452f9efe4..b4f481e1e6b6 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -146,7 +146,7 @@ i915_param_named_unsafe(edp_vswing, int, 0400,
>>  i915_param_named_unsafe(enable_guc, int, 0400,
>>  	"Enable GuC load for GuC submission and/or HuC load. "
>>  	"Required functionality can be selected using bitmask values. "
>> -	"(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
>> +	"(-1=auto [default], 0=disable, 1=GuC submission, 2=HuC load)");
>>
>>  i915_param_named(guc_log_level, int, 0400,
>>  	"GuC firmware logging level. Requires GuC to be loaded. "
>> diff --git a/drivers/gpu/drm/i915/i915_params.h  
>> b/drivers/gpu/drm/i915/i915_params.h
>> index d29ade3b7de6..5736c55694fe 100644
>> --- a/drivers/gpu/drm/i915/i915_params.h
>> +++ b/drivers/gpu/drm/i915/i915_params.h
>> @@ -54,7 +54,7 @@ struct drm_printer;
>>  	param(int, disable_power_well, -1) \
>>  	param(int, enable_ips, 1) \
>>  	param(int, invert_brightness, 0) \
>> -	param(int, enable_guc, 0) \
>> +	param(int, enable_guc, -1) \
>>  	param(int, guc_log_level, -1) \
>>  	param(char *, guc_firmware_path, NULL) \
>>  	param(char *, huc_firmware_path, NULL) \