| Message ID | 20181012132609.35968-1-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Introduce i915_gem_fini_hw for symmetry with i915_gem_init_hw | expand |
Quoting Michal Wajdeczko (2018-10-12 14:26:09) > In function i915_gem_init_hw we are initializing some uC code that i915_gem_init_hw really shouldn't be called such, at least it doesn't fit in with the global init_hw phase. Suggestions for a clearer name welcome. > requires some cleanup. Then during unwind we call this uC cleanup > function directly which breaks symmetry and layering. Fix that by > adding i915_gem_fini_hw for symmetry with i915_gem_init_hw. There's a lot that we do in init_hw that we presume will be undone by reset. The asymmetry looks inherent, but I wonder if it would be better expressed as we are missing a global reset along the error path. It also has to be said that we cannot support intel_uc_init_hw() from within a struct_mutex-less i915_reset. (I have a patch to disable global reset while guc is enabled to avoid the deadlock, forcing us to rely on per-engine resets in that case). Anyway, I'm hesitant to call the suggested i915_gem_fini_hw() as the compliment to i915_gem_init_hw() and feel a little more convincing is in order. -Chris
On 12/10/18 06:50, Chris Wilson wrote: > Quoting Michal Wajdeczko (2018-10-12 14:26:09) >> In function i915_gem_init_hw we are initializing some uC code that > > i915_gem_init_hw really shouldn't be called such, at least it doesn't > fit in with the global init_hw phase. Suggestions for a clearer name > welcome. > >> requires some cleanup. Then during unwind we call this uC cleanup >> function directly which breaks symmetry and layering. Fix that by >> adding i915_gem_fini_hw for symmetry with i915_gem_init_hw. > > There's a lot that we do in init_hw that we presume will be undone by > reset. The asymmetry looks inherent, but I wonder if it would be better > expressed as we are missing a global reset along the error path. > > It also has to be said that we cannot support intel_uc_init_hw() from > within a struct_mutex-less i915_reset. (I have a patch to disable global > reset while guc is enabled to avoid the deadlock, forcing us to rely on > per-engine resets in that case). > Can we really not have global reset with GuC? Or is your patch just a temporary WA? I know per-engine reset should work for 99% of cases, but what if, for example, the GuC itself hangs? There is also some other HW blocks in GT that are not covered by engine reset. Thanks, Daniele > Anyway, I'm hesitant to call the suggested i915_gem_fini_hw() as the > compliment to i915_gem_init_hw() and feel a little more convincing is in > order. > -Chris >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 3017ef0..e31e145 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3225,6 +3225,7 @@ void i915_gem_reset_engine(struct intel_engine_cs *engine, int __must_check i915_gem_init_hw(struct drm_i915_private *dev_priv); void i915_gem_init_swizzling(struct drm_i915_private *dev_priv); void i915_gem_fini(struct drm_i915_private *dev_priv); +void i915_gem_fini_hw(struct drm_i915_private *i915); void i915_gem_cleanup_engines(struct drm_i915_private *dev_priv); int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv, unsigned int flags, long timeout); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 7d45e71..8c7ebe4 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -5360,6 +5360,11 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv) return ret; } +void i915_gem_fini_hw(struct drm_i915_private *i915) +{ + intel_uc_fini_hw(i915); +} + static int __intel_engines_record_defaults(struct drm_i915_private *i915) { struct i915_gem_context *ctx; @@ -5612,7 +5617,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv) i915_gem_drain_workqueue(dev_priv); mutex_lock(&dev_priv->drm.struct_mutex); - intel_uc_fini_hw(dev_priv); + i915_gem_fini_hw(dev_priv); err_uc_init: intel_uc_fini(dev_priv); err_pm: @@ -5670,7 +5675,7 @@ void i915_gem_fini(struct drm_i915_private *dev_priv) i915_gem_drain_workqueue(dev_priv); mutex_lock(&dev_priv->drm.struct_mutex); - intel_uc_fini_hw(dev_priv); + i915_gem_fini_hw(dev_priv); intel_uc_fini(dev_priv); i915_gem_cleanup_engines(dev_priv); i915_gem_contexts_fini(dev_priv);
In function i915_gem_init_hw we are initializing some uC code that requires some cleanup. Then during unwind we call this uC cleanup function directly which breaks symmetry and layering. Fix that by adding i915_gem_fini_hw for symmetry with i915_gem_init_hw. Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem.c | 9 +++++++-- 2 files changed, 8 insertions(+), 2 deletions(-)