| Message ID | 20180409122332.24788-8-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Michal Wajdeczko (2018-04-09 13:23:28) > As we always call intel_uc_sanitize after every call to > intel_uc_fini_hw we may drop redundant call and sanitize > uC from the fini_hw function. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> Not that it matters, since doing it before losing control or on resume is the same from our pov, but I've always pencilled in sanitize as being done on takeover (i.e. before init). Why do you favour after fini? Gut feeling prefers keeping it as a separate step rather rolling it up into init/fini. But that's just because before we did sanitize elsewhere, we had many strange bugs and those bugs have left their scars (so I like seeing sanitize, it reminds me of dead bugs). -Chris
On Mon, 09 Apr 2018 14:47:24 +0200, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Michal Wajdeczko (2018-04-09 13:23:28) >> As we always call intel_uc_sanitize after every call to >> intel_uc_fini_hw we may drop redundant call and sanitize >> uC from the fini_hw function. >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Not that it matters, since doing it before losing control or on resume > is the same from our pov, but I've always pencilled in sanitize as being > done on takeover (i.e. before init). In intel_uc_init_hw we are already doing some semi-sanitization (thanks to __intel_uc_reset_hw), but maybe to be more explicit, we should add call to __uc_sanitize() in intel_uc_init_mmio() ? > Why do you favour after fini? Hmm, not at all, I would call it just more explicit > > Gut feeling prefers keeping it as a separate step rather rolling it up > into init/fini. But that's just because before we did sanitize > elsewhere, we had many strange bugs and those bugs have left their > scars (so I like seeing sanitize, it reminds me of dead bugs). As now we have symmetrical inits/finis that cover all critical paths, I don't think we need separate 'sanitize' step that could be called any time/any place. I assume extra __sanitize in uc_init_mmio should be enough reminder. /michal
On 4/9/2018 5:53 PM, Michal Wajdeczko wrote: > As we always call intel_uc_sanitize after every call to > intel_uc_fini_hw we may drop redundant call and sanitize > uC from the fini_hw function. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> With change to sanitize during uc_init_mmio this looks good to me. Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 2 -- > drivers/gpu/drm/i915/intel_uc.c | 9 +++------ > drivers/gpu/drm/i915/intel_uc.h | 1 - > 3 files changed, 3 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index ceec5a0..decda1a 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3077,7 +3077,6 @@ int i915_gem_reset_prepare(struct drm_i915_private *dev_priv) > } > > i915_gem_revoke_fences(dev_priv); > - intel_uc_sanitize(dev_priv); > > return err; > } > @@ -5062,7 +5061,6 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv) > * machine in an unusable condition. > */ > i915_gem_fini_hw(dev_priv); > - intel_uc_sanitize(dev_priv); > i915_gem_sanitize(dev_priv); > > intel_runtime_pm_put(dev_priv); > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 1cffaf7..0439966 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -322,18 +322,13 @@ void intel_uc_fini(struct drm_i915_private *dev_priv) > intel_guc_fini(guc); > } > > -void intel_uc_sanitize(struct drm_i915_private *i915) > +static void __uc_sanitize(struct drm_i915_private *i915) > { > struct intel_guc *guc = &i915->guc; > struct intel_huc *huc = &i915->huc; > > - if (!USES_GUC(i915)) > - return; > - > GEM_BUG_ON(!HAS_GUC(i915)); > > - guc_disable_communication(guc); > - > intel_huc_sanitize(huc); > intel_guc_sanitize(guc); > > @@ -445,6 +440,8 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) > intel_guc_submission_disable(guc); > > guc_disable_communication(guc); > + > + __uc_sanitize(dev_priv); > } > > int intel_uc_suspend(struct drm_i915_private *i915) > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > index 25d73ad..64aaf93 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -33,7 +33,6 @@ > void intel_uc_init_mmio(struct drm_i915_private *dev_priv); > int intel_uc_init_misc(struct drm_i915_private *dev_priv); > void intel_uc_fini_misc(struct drm_i915_private *dev_priv); > -void intel_uc_sanitize(struct drm_i915_private *dev_priv); > int intel_uc_init_hw(struct drm_i915_private *dev_priv); > void intel_uc_fini_hw(struct drm_i915_private *dev_priv); > int intel_uc_init(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index ceec5a0..decda1a 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3077,7 +3077,6 @@ int i915_gem_reset_prepare(struct drm_i915_private *dev_priv) } i915_gem_revoke_fences(dev_priv); - intel_uc_sanitize(dev_priv); return err; } @@ -5062,7 +5061,6 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv) * machine in an unusable condition. */ i915_gem_fini_hw(dev_priv); - intel_uc_sanitize(dev_priv); i915_gem_sanitize(dev_priv); intel_runtime_pm_put(dev_priv); diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 1cffaf7..0439966 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -322,18 +322,13 @@ void intel_uc_fini(struct drm_i915_private *dev_priv) intel_guc_fini(guc); } -void intel_uc_sanitize(struct drm_i915_private *i915) +static void __uc_sanitize(struct drm_i915_private *i915) { struct intel_guc *guc = &i915->guc; struct intel_huc *huc = &i915->huc; - if (!USES_GUC(i915)) - return; - GEM_BUG_ON(!HAS_GUC(i915)); - guc_disable_communication(guc); - intel_huc_sanitize(huc); intel_guc_sanitize(guc); @@ -445,6 +440,8 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) intel_guc_submission_disable(guc); guc_disable_communication(guc); + + __uc_sanitize(dev_priv); } int intel_uc_suspend(struct drm_i915_private *i915) diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h index 25d73ad..64aaf93 100644 --- a/drivers/gpu/drm/i915/intel_uc.h +++ b/drivers/gpu/drm/i915/intel_uc.h @@ -33,7 +33,6 @@ void intel_uc_init_mmio(struct drm_i915_private *dev_priv); int intel_uc_init_misc(struct drm_i915_private *dev_priv); void intel_uc_fini_misc(struct drm_i915_private *dev_priv); -void intel_uc_sanitize(struct drm_i915_private *dev_priv); int intel_uc_init_hw(struct drm_i915_private *dev_priv); void intel_uc_fini_hw(struct drm_i915_private *dev_priv); int intel_uc_init(struct drm_i915_private *dev_priv);
As we always call intel_uc_sanitize after every call to intel_uc_fini_hw we may drop redundant call and sanitize uC from the fini_hw function. Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem.c | 2 -- drivers/gpu/drm/i915/intel_uc.c | 9 +++------ drivers/gpu/drm/i915/intel_uc.h | 1 - 3 files changed, 3 insertions(+), 9 deletions(-)