Re: [PATCH] drm/i915/uc: Fini hw even if GuC is not running

[Fernando Pacheco <fernando.pacheco@intel.com>]

On 8/13/19 1:16 PM, Daniele Ceraolo Spurio wrote:
>
>
> On 8/13/19 9:26 AM, Fernando Pacheco wrote:
>> We should not be skipping uc_fini_hw on finding GuC
>> is no longer running. There is plenty of hw and internal
>> state that can be cleaned up without having to communicate
>> with GuC.
>>
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110943
>
>> Signed-off-by: Fernando Pacheco <fernando.pacheco@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/uc/intel_uc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> index 0dc2b0cf4604..c698cddc14dc 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> @@ -521,7 +521,7 @@ void intel_uc_fini_hw(struct intel_uc *uc)
>>   {
>>       struct intel_guc *guc = &uc->guc;
>>   -    if (!intel_guc_is_running(guc))
>> +    if (!intel_uc_supports_guc(uc))
>
> Both calls below handle the case where GuC is already not running so we're safe, but now that we wedge when we hit a guc error we can also do something like:
>
>     -EIO load error -> uc_fini_hw() -> wedge
> and then
>     unload -> uc_fini_hw()
>
> it should be all be handled safely (the fault injection test is passing where we've run it), but we end up with "GuC communication disabled" multiple times in the logs:
>
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13999/fi-skl-guc/igt@i915_module_load@reload-with-fault-injection.html
>
> <6> [237.818905] [drm] GuC communication enabled
> ....
> <6> [237.822739] i915 0000:00:02.0: [drm:__i915_inject_load_error [i915]] Injecting failure -5 at checkpoint 44 [i915_gem_init:1503]
> <6> [237.840808] [drm] GuC communication disabled
> ...
> <6> [238.160935] [drm] GuC communication disabled
>
> Maybe return early from guc_disable_communication if the communication was already disabled?

As discussed offline, an early return might also require changes to the stop communication phase.
I'll work on this separately as for now the extra disable only results in the extra logging.

Thanks,
Fernando

>
>
> Daniele
>
>>           return;
>>         if (intel_uc_supports_guc_submission(uc))
>>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx