[1/3] drm/i915/uc: Extract common code from GuC stop/disable comm
diff mbox series

Message ID 20190828004558.11903-2-fernando.pacheco@intel.com
State New
Headers show
Series
  • Add/modify checks within intel_uc_fini_hw
Related show

Commit Message

Fernando Pacheco Aug. 28, 2019, 12:45 a.m. UTC
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(-)

Comments

Daniele Ceraolo Spurio Aug. 28, 2019, 8:47 p.m. UTC | #1
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);
>   }
>   
>
Daniele Ceraolo Spurio Aug. 28, 2019, 10:09 p.m. UTC | #2
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

Patch
diff mbox series

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);
+
+	__guc_stop_communication(guc);
+
+	DRM_INFO("GuC communication stopped\n");
+}
+
+static void guc_disable_communication(struct intel_guc *guc)
+{
+	__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);
 }