Re: [PATCH 1/3] drm/i915/uc: Extract common code from GuC stop/disable comm

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

BTW, this patch should be enough for 
https://bugs.freedesktop.org/show_bug.cgi?id=110943, because we disable 
interrupts on _stop and therefore we don't leave them enabled even if we 
skip fini_hw (which still needs to be fixed for the submission side of 
things).

Daniele

On 8/28/19 1:47 PM, Daniele Ceraolo Spurio wrote:
> 
> 
> On 8/27/19 5:45 PM, Fernando Pacheco wrote:
>> During normal driver unload we attempt to disable GuC communication
>> while it is currently stopped. This results in a nop'd call to
>> intel_guc_ct_disable within guc_disable_communication because
>> stop/disable rely on the same flag to prevent further comms with CT.
>>
>> We can avoid the call to disable and still leave communication in a
>> satisfactory state by extracting a set of shared steps from stop/disable.
>> This set can include guc_disable_interrupts as we do not require the
>> single caller of guc_stop_communication to be atomic:
>> "drm/i915/selftests: Fixup atomic reset checking".
>>
>> This situation (stop -> disable) only occurs during intel_uc_fini_hw,
>> so during fini, call guc_disable_communication only if currently enabled.
>> The symmetric calls to enable/disable remain unmodified for all other
>> scenarios.
>>
>> Signed-off-by: Fernando Pacheco <fernando.pacheco@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/uc/intel_uc.c | 30 ++++++++++++++++-----------
>>   1 file changed, 18 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c 
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> index 71ee7ab035cc..29a9eec60d2e 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> @@ -224,17 +224,7 @@ static int guc_enable_communication(struct 
>> intel_guc *guc)
>>       return 0;
>>   }
>> -static void guc_stop_communication(struct intel_guc *guc)
>> -{
>> -    intel_guc_ct_stop(&guc->ct);
>> -
>> -    guc->send = intel_guc_send_nop;
>> -    guc->handler = intel_guc_to_host_event_handler_nop;
>> -
>> -    guc_clear_mmio_msg(guc);
>> -}
>> -
>> -static void guc_disable_communication(struct intel_guc *guc)
>> +static void __guc_stop_communication(struct intel_guc *guc)
>>   {
>>       /*
>>        * Events generated during or after CT disable are logged by guc in
>> @@ -247,6 +237,20 @@ static void guc_disable_communication(struct 
>> intel_guc *guc)
>>       guc->send = intel_guc_send_nop;
>>       guc->handler = intel_guc_to_host_event_handler_nop;
>> +}
>> +
>> +static void guc_stop_communication(struct intel_guc *guc)
>> +{
>> +    intel_guc_ct_stop(&guc->ct);
>> +
> 
> The only difference between intel_guc_ct_stop() and 
> intel_guc_ct_disable() is that in the latter we also tell guc that we've 
> disabled the buffers. We could probably just add a check to return early 
> if !intel_guc_is_running() in intel_guc_ct_disable() and drop the 
> stop/disable differentiation entirely, but that doesn't need to happen 
> in this patch.
> 
> 
>> +    __guc_stop_communication(guc);
>> +
>> +    DRM_INFO("GuC communication stopped\n");
>> +}
>> +
>> +static void guc_disable_communication(struct intel_guc *guc)
>> +{
> 
> Are we now guaranteed that guc_disable_communication() is called only of 
> communication is actually enabled? if so, we could add here a:
> 
>      GEM_BUG_ON(!guc_communication_enabled(guc));
> 
> with or without that:
> 
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> Daniele
> 
>> +    __guc_stop_communication(guc);
>>       intel_guc_ct_disable(&guc->ct);
>> @@ -537,7 +541,9 @@ void intel_uc_fini_hw(struct intel_uc *uc)
>>       if (intel_uc_supports_guc_submission(uc))
>>           intel_guc_submission_disable(guc);
>> -    guc_disable_communication(guc);
>> +    if (guc_communication_enabled(guc))
>> +        guc_disable_communication(guc);
>> +
>>       __uc_sanitize(uc);
>>   }
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx