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

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

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