| Message ID | 20190828004558.11903-2-fernando.pacheco@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add/modify checks within intel_uc_fini_hw | expand |
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); > } > >
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
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); }
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(-)